fix: emit member events outside pdu process #239
Conversation
WalkthroughThe pull request extends the federation SDK to emit homeserver events (membership and room creation) through an EventEmitterService integrated into invite and room services. It introduces a new RoomInfoNotReadyError error type and extends event types to include room state and version information. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #239 +/- ##
==========================================
- Coverage 53.03% 52.73% -0.30%
==========================================
Files 96 96
Lines 12571 12608 +37
==========================================
- Hits 6667 6649 -18
- Misses 5904 5959 +55 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… using EventEmitterService
ggazzo
force-pushed
the
feat/emit-join
branch
2 times, most recently
from
8205956 to
7238484
Compare
ggazzo
force-pushed
the
feat/emit-join
branch
from
7238484 to
036bff2
Compare
…ngAreaService with direct type check
…load in StagingAreaService
ggazzo
changed the title
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/federation-sdk/src/index.ts(2 hunks)packages/federation-sdk/src/services/invite.service.ts(5 hunks)packages/federation-sdk/src/services/room.service.ts(4 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/staging-area.service.tspackages/federation-sdk/src/services/state.service.tspackages/federation-sdk/src/services/room.service.ts
🧬 Code graph analysis (2)
packages/federation-sdk/src/services/staging-area.service.ts (2)
packages/room/src/manager/v3.ts (1)
eventId(17-29)packages/room/src/manager/event-wrapper.ts (1)
event(129-138)
packages/federation-sdk/src/index.ts (1)
packages/room/src/types/_common.ts (2)
PduForType(26-26)EventID(8-8)
🔇 Additional comments (13)
packages/federation-sdk/src/services/state.service.ts (2)
58-69: LGTM! Improved error categorization.The new error classes properly set the
nameproperty for better stack traces and debugging.RoomInfoNotReadyErrorprovides more specific error handling for room information readiness issues.
85-102: LGTM! More specific error handling.Replacing the generic
ErrorwithRoomInfoNotReadyErrorimproves error categorization and makes it easier for callers to handle room information readiness issues specifically.packages/federation-sdk/src/services/staging-area.service.ts (2)
274-281: LGTM! Consistent event handling for room creation.The new case for
m.room.createevents follows the established pattern and aligns with the new event signature added toHomeserverEventSignatures.
271-271: The review comment is incorrect.The code at line 271 correctly destructures
roomIddirectly from theeventobject. TheEventStagingStoreinterface definesroomIdas a direct property, making this extraction valid. The other usages ofevent.event.room_idin the file are accessing a different nested property from the Matrix event structure, not an inconsistency with line 271.Likely an incorrect or invalid review comment.
packages/federation-sdk/src/index.ts (2)
4-4: LGTM! Cleaned up unused imports.The import statement now only includes
EventStagingStore, which is the only type from@rocket.chat/federation-coredirectly used in this file. The other types remain available through re-exports.
90-93: LGTM! Well-typed event signature for room creation.The new event signature for
homeserver.matrix.room.createis properly typed and consistent with other event signatures in the interface.packages/federation-sdk/src/services/invite.service.ts (4)
15-22: LGTM! Proper dependencies added for event emission and error handling.The imports correctly add
EventEmitterServicefor the new emission pathway andRoomInfoNotReadyErrorto support the updated error handling from state service.
41-47: LGTM! EventEmitterService properly injected.The dependency injection follows the established pattern and enables the dual emission pathway introduced in this PR.
148-151: LGTM! Consistent event emission across invite scenarios.The membership event emissions are consistently placed after invite processing in both remote invite and host server scenarios, matching the pattern established in the local invite flow.
Also applies to: 247-250
113-116: No issues with the emission architecture—both emissions are intentional and in separate code paths.The code contains two
emitterService.emit('homeserver.matrix.membership')calls within theinviteUserToRoommethod, but they are in mutually exclusive conditional branches:
- Lines 113-116: Executes when the invited user belongs to your server (local invite)
- Lines ~140-143: Executes when the invited user is from a remote server (federated invite)
Each path handles a different scenario and appropriately emits the membership event after its respective processing. This is not duplicate or problematic "dual emission"—it's proper architectural separation for two distinct invite workflows.
packages/federation-sdk/src/services/room.service.ts (3)
36-46: LGTM! Updated imports for error handling.The imports correctly add
RoomInfoNotReadyErrorto support the updated error handling from state service.
51-66: LGTM! EventEmitterService properly injected.The dependency injection follows the established pattern and is consistent with the changes in
InviteService.
888-902: Verify dual emission is intentional.The code emits the same membership event through both
eventEmitterService(line 888) andemitterService(line 899). Confirm this dual emission is intentional and required for the architecture, as it creates two separate event pathways.
| // TODO: check if all sendEventToAllServersInRoom should be followed by an emitter | ||
| void this.federationService.sendEventToAllServersInRoom(event); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Track the TODO for consistent event emission pattern.
The TODO comment suggests that the pattern of emitting events after sendEventToAllServersInRoom may need to be applied consistently throughout the codebase. Consider creating an issue to track this refactoring work.
Do you want me to scan the codebase for other occurrences of sendEventToAllServersInRoom that might need the same emission pattern?
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/room.service.ts around lines 1282-1283,
the TODO notes inconsistency about whether sendEventToAllServersInRoom should be
followed by an emitter; create a tracked fix: open an issue in the repo (include
context, affected method, and proposed change to always emit after
sendEventToAllServersInRoom), replace the inline TODO with a brief reference to
that issue (e.g., "see issue #NNN") and, as part of the issue or a follow-up PR,
scan the codebase for all usages of sendEventToAllServersInRoom and update each
callsite to consistently emit the event (or document justified exceptions), and
add unit/integration tests to validate the emission behavior where applicable.
As per FB-52, this PR fixes an issue where remote servers fail to join RC-originated rooms because membership events for local RC users were not being emitted at the correct time, preventing those events from being saved into our state.
When a room is created on RC and local users A and B join before a remote user is invited, their membership events were only emitted during the transaction flow. They were not persisted in a way that backfill or later federation queries could retrieve. As a result, remote homeservers (e.g., Synapse) receive an invite referencing users for whom they have no membership state, eventually failing with:
Fix: Membership events are now emitted immediately after processing invites/joins, outside of the PDU transaction-only path. This ensures the events are correctly stored in our state, allowing backfill to include them and enabling remote servers to obtain the complete membership graph required to successfully evaluate and join the room.
Release Notes
New Features
Improvements