fix: enhance invite handling for re-join scenarios and prevent duplicate events#391
fix: enhance invite handling for re-join scenarios and prevent duplicate events#391sampaiodiego wants to merge 2 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-10T22:18:31.655ZApplied to files:
🔇 Additional comments (3)
WalkthroughConditional invite handling now requires a local room-create and resolvable prev_events to process invites; otherwise invites are stored as outliers. State initialization preloads known event IDs to avoid re-notifying persisted events. Join flow now always calls processInitialState for re-joins. Changes
Sequence Diagram(s)sequenceDiagram
participant RoomService as RoomService
participant InviteService as InviteService
participant EventRepository as EventRepository
participant StateService as StateService
participant Notification as NotificationService
RoomService->>StateService: getRoomVersion(roomId)
StateService-->>RoomService: room version / UnknownRoomError
RoomService->>StateService: processInitialState(state, authChain)
StateService->>EventRepository: preload event IDs for authChain/eventCache
EventRepository-->>StateService: knownEventIds
loop per sorted event
alt eventId not in knownEventIds
StateService->>Notification: notify(event)
Notification-->>StateService: ack
else
StateService-->>StateService: skip notify
end
StateService->>StateService: persist state delta / update room graph
end
Note over InviteService,EventRepository: Invite handling flow
InviteService->>EventRepository: check room-create exists
InviteService->>EventRepository: canResolveEventState(prev_events)
alt room-create exists AND prev_events resolvable
InviteService->>StateService: handlePdu(inviteEvent)
StateService-->>InviteService: processed
else
InviteService->>EventRepository: insertOutlierEvent(inviteEvent)
EventRepository-->>InviteService: stored as outlier
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
sampaiodiego
changed the title
sampaiodiego
force-pushed
the
invite-after-leave
branch
from
40038d2 to
3812d75
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/federation-sdk/src/services/state.service.ts">
<violation number="1" location="packages/federation-sdk/src/services/state.service.ts:424">
P2: The dedupe query runs after saving the create event, so `m.room.create` is incorrectly treated as pre-existing and never notified in this flow.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/invite.service.spec.ts (1)
67-69: Hoist theEventServicestub to a variable for cleaner test setup.The spec files are excluded from TypeScript compilation (
tsconfig.jsonexcludes**/*.spec.ts), so the private member access here doesn't cause type errors. However, extracting the stub to a separate variable improves readability and test structure by clearly separating the mock setup from the service instantiation.Cleaner test setup
- const stateService = new StateService(stateGraphRepository, eventRepository, configServiceInstance, { + const eventService = { notify: () => Promise.resolve(), - } as unknown as EventService); + } as unknown as EventService; + + const stateService = new StateService(stateGraphRepository, eventRepository, configServiceInstance, eventService); ... - const notifySpy = spyOn(stateService.eventService as any, 'notify').mockImplementation( + const notifySpy = spyOn(eventService as any, 'notify').mockImplementation(Also applies to: 208-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/federation-sdk/src/services/invite.service.spec.ts` around lines 67 - 69, Extract the inline EventService stub into a named constant (e.g., const mockEventService = { notify: () => Promise.resolve() } as unknown as EventService) and use that variable when constructing StateService (the existing new StateService(...) call) to make the test setup clearer and reusable; do the same for the other occurrences of the inline stub in this spec so all StateService instantiations reference the shared mockEventService.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/federation-sdk/src/services/invite.service.ts`:
- Line 219: Prettier is failing on the if condition that uses an awaited call;
update the condition in invite.service.ts so the awaited expression is
parenthesized: change the line using createEvent and await
this.canResolveEventState(inviteEvent) to use (await
this.canResolveEventState(inviteEvent)) so the condition reads like if
(createEvent && (await this.canResolveEventState(inviteEvent))) { — this fixes
the formatting complaint while preserving logic in the createEvent /
canResolveEventState / inviteEvent check.
- Around line 239-252: canResolveEventState currently treats outliers as "found"
because findByIds includes events with stateId == '', and it also ignores
missing auth events; update canResolveEventState to fetch prev_event ids and
auth_event ids (use event.getPreviousEventIds() and event.getAuthEventIds()),
load both sets via eventRepository.findByIds(...).toArray(), and only return
true if the number of loaded prev events equals prev_event_ids.length AND the
number of loaded auth events equals auth_event_ids.length AND none of the loaded
events have empty or falsy stateId (i.e., fully materialized); additionally, in
handlePdu (where _resolveStateAtEvent is invoked), wrap the state
resolution/auth checks in a try/catch and on failure call
insertOutlierEvent(...) for the invite event so missing/malformed dependencies
fall back to storing an outlier instead of throwing.
In `@packages/federation-sdk/src/services/state.service.ts`:
- Around line 419-425: The code computes knownEventIds from
store.getEvents(allEventIds) after persisting the batch (including the newly
created m.room.create), which causes the just-created create event
(createEvent.eventId) to be treated as already-known and suppresses notify() —
fix processInitialState by fetching pre-existing event IDs before any writes
(compute knownEventIds from store.getEvents(allEventIds) prior to persisting the
batch) or, if simpler, explicitly remove createEvent.eventId from the
knownEventIds set after the fetch (ensure the guard that checks knownEventIds
before calling notify() uses the pre-write snapshot); reference sortedEvents,
allEventIds, store.getEvents, knownEventIds, createEvent and notify to locate
and update the logic.
---
Nitpick comments:
In `@packages/federation-sdk/src/services/invite.service.spec.ts`:
- Around line 67-69: Extract the inline EventService stub into a named constant
(e.g., const mockEventService = { notify: () => Promise.resolve() } as unknown
as EventService) and use that variable when constructing StateService (the
existing new StateService(...) call) to make the test setup clearer and
reusable; do the same for the other occurrences of the inline stub in this spec
so all StateService instantiations reference the shared mockEventService.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8075002b-41f9-45b8-9ed6-32916e6be4b0
📒 Files selected for processing (4)
packages/federation-sdk/src/services/invite.service.spec.tspackages/federation-sdk/src/services/invite.service.tspackages/federation-sdk/src/services/room.service.tspackages/federation-sdk/src/services/state.service.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
🧠 Learnings (1)
📚 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/state.service.tspackages/federation-sdk/src/services/room.service.tspackages/federation-sdk/src/services/invite.service.ts
🪛 ESLint
packages/federation-sdk/src/services/invite.service.ts
[error] 219-219: Replace await·this.canResolveEventState(inviteEvent with (await·this.canResolveEventState(inviteEvent)
(prettier/prettier)
🪛 GitHub Actions: my-workflow
packages/federation-sdk/src/services/invite.service.ts
[error] 219-219: prettier/prettier failed: Replace await·this.canResolveEventState(inviteEvent with (await·this.canResolveEventState(inviteEvent)
🔇 Additional comments (2)
packages/federation-sdk/src/services/room.service.ts (1)
734-750: LGTM — always replayingsend_joinstate on re-join is the right control flow.The narrowed
UnknownRoomErrorcatch avoids swallowing unrelated failures, and unconditionally callingprocessInitialState(state, authChain)matches the leave/re-join case this PR is fixing.packages/federation-sdk/src/services/invite.service.spec.ts (1)
301-495: Nice regression coverage for the re-invite/outlier paths.These cases exercise the exact combinations the production code now branches on: unknown
prev_events, knownprev_events, missingm.room.create, and the full invite → join → leave → re-invite cycle.
| /** | ||
| * Checks whether the invite event's prev_events exist locally so that | ||
| * handlePdu can resolve the state at the event. When the local server | ||
| * left a room and missed events, the invite's prev_events will reference | ||
| * events we never received, making state resolution impossible. | ||
| */ | ||
| private async canResolveEventState(event: PersistentEventBase): Promise<boolean> { | ||
| const prevEventIds = event.getPreviousEventIds(); | ||
| if (prevEventIds.length === 0) { | ||
| return true; | ||
| } | ||
|
|
||
| const found = await this.eventRepository.findByIds(prevEventIds).toArray(); | ||
| return found.length === prevEventIds.length; |
There was a problem hiding this comment.
canResolveEventState() still green-lights handlePdu() too early.
findByIds() counts outliers as "found", but insertOutlierEvent() stores them with stateId: '', so _resolveStateAtEvent() still cannot build state from those dependencies. handlePdu() also performs an auth check from event.getAuthEventIds() before state resolution, so missing auth events can still make the branch at Line 219 throw instead of falling back to insertOutlierEvent().
Please only return true for fully materialized prev/auth dependencies, or catch that failure and store the invite as an outlier.
🛡️ One way to tighten the preflight
private async canResolveEventState(event: PersistentEventBase): Promise<boolean> {
- const prevEventIds = event.getPreviousEventIds();
- if (prevEventIds.length === 0) {
+ const dependencyIds = [...new Set([...event.getPreviousEventIds(), ...event.getAuthEventIds()])];
+ if (dependencyIds.length === 0) {
return true;
}
- const found = await this.eventRepository.findByIds(prevEventIds).toArray();
- return found.length === prevEventIds.length;
+ const found = await this.eventRepository.findByIds(dependencyIds).toArray();
+ const records = new Map(found.map((record) => [record._id, record]));
+
+ return dependencyIds.every((id) => {
+ const record = records.get(id);
+ return record && !record.outlier && Boolean(record.stateId);
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Checks whether the invite event's prev_events exist locally so that | |
| * handlePdu can resolve the state at the event. When the local server | |
| * left a room and missed events, the invite's prev_events will reference | |
| * events we never received, making state resolution impossible. | |
| */ | |
| private async canResolveEventState(event: PersistentEventBase): Promise<boolean> { | |
| const prevEventIds = event.getPreviousEventIds(); | |
| if (prevEventIds.length === 0) { | |
| return true; | |
| } | |
| const found = await this.eventRepository.findByIds(prevEventIds).toArray(); | |
| return found.length === prevEventIds.length; | |
| /** | |
| * Checks whether the invite event's prev_events exist locally so that | |
| * handlePdu can resolve the state at the event. When the local server | |
| * left a room and missed events, the invite's prev_events will reference | |
| * events we never received, making state resolution impossible. | |
| */ | |
| private async canResolveEventState(event: PersistentEventBase): Promise<boolean> { | |
| const dependencyIds = [...new Set([...event.getPreviousEventIds(), ...event.getAuthEventIds()])]; | |
| if (dependencyIds.length === 0) { | |
| return true; | |
| } | |
| const found = await this.eventRepository.findByIds(dependencyIds).toArray(); | |
| const records = new Map(found.map((record) => [record._id, record])); | |
| return dependencyIds.every((id) => { | |
| const record = records.get(id); | |
| return record && !record.outlier && Boolean(record.stateId); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/federation-sdk/src/services/invite.service.ts` around lines 239 -
252, canResolveEventState currently treats outliers as "found" because findByIds
includes events with stateId == '', and it also ignores missing auth events;
update canResolveEventState to fetch prev_event ids and auth_event ids (use
event.getPreviousEventIds() and event.getAuthEventIds()), load both sets via
eventRepository.findByIds(...).toArray(), and only return true if the number of
loaded prev events equals prev_event_ids.length AND the number of loaded auth
events equals auth_event_ids.length AND none of the loaded events have empty or
falsy stateId (i.e., fully materialized); additionally, in handlePdu (where
_resolveStateAtEvent is invoked), wrap the state resolution/auth checks in a
try/catch and on failure call insertOutlierEvent(...) for the invite event so
missing/malformed dependencies fall back to storing an outlier instead of
throwing.
3812d75 to
2658e25
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #391 +/- ##
==========================================
- Coverage 50.83% 50.72% -0.12%
==========================================
Files 101 101
Lines 11475 11512 +37
==========================================
+ Hits 5833 5839 +6
- Misses 5642 5673 +31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary by CodeRabbit
Bug Fixes
Tests