perf(room-worker): marshal room-key once per fan-out + projected survivor list#242
Conversation
…ivor accounts
Two hot-path efficiency fixes in room-worker, no behavioral change:
1. Marshal-once fan-out. fanOutKey previously called Sender.Send per
account, which re-marshaled the identical RoomKeyEvent once per
recipient (10k marshals for a 10k-member room create/rotation). Split
Send into Marshal (stamp + serialize once) and SendData (publish
pre-marshaled bytes); fanOutKey now marshals once and publishes the
same bytes to every account. Send is retained as a thin wrapper.
2. Projected survivor list. The remove-flow rotate path called
ListByRoom, decoding every field of every subscription in the room
just to extract accounts. It now uses the projected
GetSubscriptionAccounts ({u.account:1}); rotateAndFanOut and
fanOutRoomKeyToSurvivors take []string accounts directly. ListByRoom
is dropped from the SubscriptionStore interface (handler no longer
needs it) but kept on the concrete store for integration tests.
https://claude.ai/code/session_01KyEPakZnVkZKrPL5j9cjpw
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR refactors room key delivery: Sender now exposes Marshal and SendData, member-removal flows use GetSubscriptionAccounts for survivor account lists instead of ListByRoom, and fanout marshals the event once and reuses the payload across recipients. ChangesRoom Key Sender Refactoring and Account Lookup Optimization
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
room-worker/handler_test.go (1)
398-398: ⚡ Quick winExercise a non-empty survivor projection in one remove-flow test.
These updated expectations all return
nil, so the suite now only provesGetSubscriptionAccountsis called. It does not prove the returned[]stringsurvivors are actually passed through to the rotate/fan-out path. Giving one happy-path remove test a non-empty survivor list and asserting the correspondingchat.user.<acct>.event.room.keypublish would catch wiring regressions in this refactor.As per coding guidelines, "Tests must cover: happy path, error paths, edge cases (empty collections, boundary conditions), and invalid input".
Also applies to: 585-585, 1171-1171, 1245-1245, 1537-1537, 1576-1576, 4116-4116, 4146-4146
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@room-worker/keyfanout_test.go`:
- Around line 159-183: The test TestFanOutKey_MarshalsOnce relies on
byte-equality but can false-pass when per-recipient re-marshals happen within
the same millisecond; change the test to deterministically detect multiple
marshals by either (A) making the dataRecordingPublisher used by
newFanoutTestHandler introduce an artificial per-publish delay so that different
publishes land in different milliseconds (e.g. sleep briefly in
dataRecordingPublisher.Publish) and then assert payload bytes differ when
broken, or (B) add a marshal spy around the room key serialization (wrap/replace
the marshal function used by roomkeysender.NewSender or inject a spy into
h/fanOutKey) that increments a counter each time the event is marshaled and
assert the counter == 1 after calling h.fanOutKey; reference
dataRecordingPublisher, newFanoutTestHandler, roomkeysender.NewSender,
h.fanOutKey and dp.snapshot when locating where to add the delay or spy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de3e4358-0d27-402e-b879-5af788ab7ddb
📒 Files selected for processing (8)
pkg/roomkeysender/roomkeysender.gopkg/roomkeysender/roomkeysender_test.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/keyfanout_test.goroom-worker/mock_store_test.goroom-worker/store.goroom-worker/store_mongo.go
💤 Files with no reviewable changes (2)
- room-worker/store.go
- room-worker/mock_store_test.go
| // of SubscriptionStore — the handler's hot paths only need accounts (see | ||
| // GetSubscriptionAccounts); this full-document read is retained for integration | ||
| // test verification. | ||
| func (s *MongoStore) ListByRoom(ctx context.Context, roomID string) ([]model.Subscription, error) { |
There was a problem hiding this comment.
Since we are not using this method anywhere, we can remove it and related tests too. Can be done in a repo-wide refactor later
There was a problem hiding this comment.
Agreed it's a good cleanup candidate — deferring to a later refactor as you suggest rather than widening this PR.
One note so it doesn't get deleted as "dead" by accident: it's no longer in the SubscriptionStore interface (the handler's hot paths now use the projected GetSubscriptionAccounts), but the concrete method is still exercised by the room-worker integration tests as a verification probe (TestMongoStore_..._Integration assert on full subscription docs / User.ID). So removing it is a small test rework, not a one-line delete — which is why I kept it here with the explanatory comment.
Generated by Claude Code
TestFanOutKey_MarshalsOnce compared payload bytes, which a per-recipient re-marshal could still pass when all marshals land in the same millisecond and produce identical JSON. Compare backing-array identity instead: two json.Marshal calls always allocate distinct buffers, so a re-marshal regression now fails deterministically regardless of timing. Addresses CodeRabbit review feedback on #242. https://claude.ai/code/session_01KyEPakZnVkZKrPL5j9cjpw
3 participants
Summary
Two behavior-preserving efficiency fixes on room-worker's highest-frequency path (room-key fan-out, which runs on every create / add / remove). No functional change — the same accounts receive the same key event.
Fix #1 — marshal the room-key event once per fan-out, not once per recipient
fanOutKeyspawned a worker per account and each calledkeySender.Send, which did its ownjson.Marshal. The payload is identical for every recipient (the only per-recipient difference was aTimestampthatSendstamped itself and which is meaningless per-recipient). On a 10k-member room that was 10k marshals of the same struct.Sender.SendintoMarshal(evt) ([]byte, error)(stamp + serialize once) andSendData(account, data)(publish pre-marshaled bytes).Sendis kept as a thin wrapper, so its contract/tests are unchanged.fanOutKeynow marshals once and publishes the same bytes to every account.Fix #2 — fetch only accounts for the survivor fan-out, not full subscription docs
The remove-flow rotate path called
ListByRoom, whichFinds with no projection and decodes every field of every subscription in the room — then discarded everything exceptUser.Account. On every single-member removal from a large room, the whole roster was pulled into memory and decoded.GetSubscriptionAccounts({u.account:1}).rotateAndFanOut/fanOutRoomKeyToSurvivorstake[]stringaccounts directly.ListByRoomfrom theSubscriptionStoreinterface (the handler no longer needs it); kept the concrete method for integration-test verification and regenerated the mock.Tests
TestSender_Marshal,TestSender_SendData, andTestFanOutKey_MarshalsOnce(asserts every recipient gets byte-identical payload), all written test-first (red → green).ListByRoom→GetSubscriptionAccounts.make test(whole repo,-race): 0 failures.make lint: 0 issues.gosec: 0 issues (the#nosec G117on the key payload was preserved).🤖 Finding #1/#2 from the room-worker performance review. Finding #3 is in a separate PR (#241).
Generated by Claude Code
Summary by CodeRabbit
Tests
Refactor