fix: restore DDP streams through a single restoration owner after reconnect#7511
fix: restore DDP streams through a single restoration owner after reconnect#7511diegolmello wants to merge 8 commits into
Conversation
Extract connect()'s 'connected'/'close' stream listeners into connectionListeners.ts factories (zero behavior change) and add a monotonic SDK instance generation id, so the promoted connection-lifecycle regression suite exercises the shipping meteor.connected guard instead of a drifting replica. Scenarios d/f/g assert the desired post-fix behavior as it.failing repros of the known reconnect bugs; the upcoming fixes flip them to plain tests.
Remove the stale meteor.connected guard from the connected listener so every real DDP handshake re-runs recovery, retry transient resume-login failures with bounded backoff, and re-dispatch resume login on foreground when a stored token exists but the session is unauthenticated.
… reconnect Streams died in different ways per lifecycle event because each consumer patched its own recovery: forceReopen wipes driver subscriptions, and a new SDK instance orphans listeners bound to the old emitter, so an open room went silently dead on server switch while the rooms list recovered. Introduce connectionRestore: consumers enroll a restorer callback, and a single 'login' listener — bound by connect() right after sdk.initialize and keyed to sdk.generation so a superseded instance can't fire it — re-runs every restorer on each successful (re)login. Rooms-list re-subscribes the notify-user set; RoomSubscription re-homes its stream listeners onto the live instance and re-sends its room subs. This subsumes the per-consumer patches: the close-listener rooms reset and RoomSubscription's own 'login' re-subscribe are removed. Lifecycle suite scenario f (SDK swap) now passes and a generation fan-out contract test covers all six transitions.
… server.connected state.server.connected was set only by SERVER.SELECT_* actions and never reset on socket death, so the deep-link same-server warm start read stale-true, skipped connect/login, and navigated onto a dead socket (the notification-tap funnel). Gate both warm-start sites on state.meteor.connected && state.login.isAuthenticated — flags that are actually reset by the close listener and by failed resume — and delete the now-unread connected field from the server reducer so it can't mislead future readers.
…restoration owner Migrates the remaining LOGIN.SUCCESS subscribe forks to generation-keyed registerStreamRestorer callbacks so every app-level DDP stream is re-established on any successful login, including forceReopen recovery and SDK instance swaps. getPermissions is split into fetch-only getPermissions plus subscribePermissions so the awaited permissions fetch keeps gating enterprise-modules and VoIP checks. Lifecycle suite grows four scenarios asserting all app streams re-land on the live socket after plain reopen, forceReopen with stale redux state, instance swap and transient resume-login failure.
GitHub Actions
Inactive
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (5)**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
app/sagas/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.test.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-04-30T17:07:51.020ZApplied to files:
📚 Learning: 2026-06-25T18:37:25.526ZApplied to files:
🔇 Additional comments (1)
WalkthroughThis PR adds generation-aware DDP stream restoration, re-homes room and app-stream subscriptions after reconnects, refactors connection listeners, adds resume-login retries, and gates deep-link recovery on authenticated socket state. ChangesConnection and session recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant connect
participant SDK
participant Restoration
participant Streams
Client->>connect: reconnect or initialize
connect->>SDK: initialize(server)
connect->>Restoration: bindStreamRestoration()
SDK->>Restoration: login event
Restoration->>Streams: run registered restorers
Streams->>SDK: re-subscribe and refresh data
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
GitHub Actions
Inactive
![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)
app/lib/methods/getRoles.ts (1)
135-138: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle the
sdk.subscribepromise inside the restorer.The
stream-rolessubscription promise is discarded, so a rejection surfaces as an unhandled rejection. UnlikegetPermissions.ts/getSettings.ts, which routesdk.subscribe(...)through asubscribeX()returned by the restorer (sorunRestorerscatches it), this callback only returnsgetRoles(). Await both so failures are logged rather than unhandled.Based on learnings: attach
.catch(...)(or await/handle) floating promises rather than leaving them unhandled.♻️ Proposed fix
-registerStreamRestorer(() => { - sdk.subscribe('stream-roles', 'roles'); - return getRoles(); -}); +registerStreamRestorer(async () => { + await sdk.subscribe('stream-roles', 'roles'); + await getRoles(); +});🤖 Prompt for 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. In `@app/lib/methods/getRoles.ts` around lines 135 - 138, Update the restorer callback containing `sdk.subscribe('stream-roles', 'roles')` so it returns or awaits the subscription promise together with `getRoles()`, matching the `subscribeX()` pattern used by `getPermissions.ts` and `getSettings.ts`; ensure both failures reach `runRestorers` handling instead of leaving `sdk.subscribe` unhandled.Source: Learnings
🤖 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 `@app/sagas/deepLinking.js`:
- Around line 172-179: Update ensureSameServerSession to wait for LOGIN.SUCCESS,
LOGIN.FAILURE, or LOGOUT after initiating authentication, and handle each
outcome so failure or logout exits the flow instead of blocking navigation.
Preserve the existing authenticated/session-ready path and selectServerRequest
behavior.
---
Nitpick comments:
In `@app/lib/methods/getRoles.ts`:
- Around line 135-138: Update the restorer callback containing
`sdk.subscribe('stream-roles', 'roles')` so it returns or awaits the
subscription promise together with `getRoles()`, matching the `subscribeX()`
pattern used by `getPermissions.ts` and `getSettings.ts`; ensure both failures
reach `runRestorers` handling instead of leaving `sdk.subscribe` unhandled.
🪄 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 Plus
Run ID: b01c7992-c242-4578-97eb-9498d9578bac
📒 Files selected for processing (22)
app/lib/methods/getPermissions.tsapp/lib/methods/getRoles.tsapp/lib/methods/getSettings.tsapp/lib/methods/getUsersPresence.tsapp/lib/methods/subscribeRooms.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/services/connect.tsapp/lib/services/connection.lifecycle.test.tsapp/lib/services/connectionListeners.tsapp/lib/services/connectionRestore.test.tsapp/lib/services/connectionRestore.tsapp/lib/services/sdk.tsapp/reducers/server.test.tsapp/reducers/server.tsapp/sagas/__tests__/deepLinking.test.tsapp/sagas/__tests__/login.test.tsapp/sagas/__tests__/state.test.tsapp/sagas/deepLinking.jsapp/sagas/login.jsapp/sagas/state.js
💤 Files with no reviewable changes (1)
- app/reducers/server.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/services/connectionRestore.test.tsapp/lib/services/connectionListeners.tsapp/lib/methods/subscribeRooms.tsapp/reducers/server.test.tsapp/sagas/state.jsapp/lib/methods/getUsersPresence.tsapp/lib/methods/getSettings.tsapp/lib/methods/getRoles.tsapp/lib/services/sdk.tsapp/sagas/deepLinking.jsapp/lib/services/connectionRestore.tsapp/sagas/__tests__/state.test.tsapp/lib/services/connect.test.tsapp/sagas/__tests__/deepLinking.test.tsapp/lib/methods/getPermissions.tsapp/sagas/__tests__/login.test.tsapp/lib/services/connection.lifecycle.test.tsapp/lib/services/connect.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/subscriptions/room.tsapp/sagas/login.js
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript strict mode; resolve application imports relative to the
app/base URL.
Files:
app/lib/services/connectionRestore.test.tsapp/lib/services/connectionListeners.tsapp/lib/methods/subscribeRooms.tsapp/reducers/server.test.tsapp/lib/methods/getUsersPresence.tsapp/lib/methods/getSettings.tsapp/lib/methods/getRoles.tsapp/lib/services/sdk.tsapp/lib/services/connectionRestore.tsapp/sagas/__tests__/state.test.tsapp/lib/services/connect.test.tsapp/sagas/__tests__/deepLinking.test.tsapp/lib/methods/getPermissions.tsapp/sagas/__tests__/login.test.tsapp/lib/services/connection.lifecycle.test.tsapp/lib/services/connect.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/subscriptions/room.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the repository Prettier style: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where allowed, and same-line brackets.
Files:
app/lib/services/connectionRestore.test.tsapp/lib/services/connectionListeners.tsapp/lib/methods/subscribeRooms.tsapp/reducers/server.test.tsapp/sagas/state.jsapp/lib/methods/getUsersPresence.tsapp/lib/methods/getSettings.tsapp/lib/methods/getRoles.tsapp/lib/services/sdk.tsapp/sagas/deepLinking.jsapp/lib/services/connectionRestore.tsapp/sagas/__tests__/state.test.tsapp/lib/services/connect.test.tsapp/sagas/__tests__/deepLinking.test.tsapp/lib/methods/getPermissions.tsapp/sagas/__tests__/login.test.tsapp/lib/services/connection.lifecycle.test.tsapp/lib/services/connect.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/subscriptions/room.tsapp/sagas/login.js
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Run Jest tests with
TZ=UTCto ensure deterministic timezone-dependent test behavior.
Files:
app/lib/services/connectionRestore.test.tsapp/reducers/server.test.tsapp/sagas/__tests__/state.test.tsapp/lib/services/connect.test.tsapp/sagas/__tests__/deepLinking.test.tsapp/sagas/__tests__/login.test.tsapp/lib/services/connection.lifecycle.test.tsapp/lib/methods/subscriptions/room.test.ts
app/reducers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use reducers to define and update application state shapes.
Files:
app/reducers/server.test.ts
app/lib/services/{sdk,restApi,connect}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the SDK service for WebSocket subscriptions, the REST API service for HTTP requests via
fetch, and the connect service for server connection management.
Files:
app/lib/services/sdk.tsapp/lib/services/connect.ts
app/sagas/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use sagas for side effects such as initialization, authentication, rooms, messages, encryption, deep linking, and video conferencing.
Files:
app/sagas/__tests__/state.test.tsapp/sagas/__tests__/deepLinking.test.tsapp/sagas/__tests__/login.test.ts
🧠 Learnings (3)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/services/connectionRestore.test.tsapp/lib/services/connectionListeners.tsapp/lib/methods/subscribeRooms.tsapp/reducers/server.test.tsapp/lib/methods/getUsersPresence.tsapp/lib/methods/getSettings.tsapp/lib/methods/getRoles.tsapp/lib/services/sdk.tsapp/lib/services/connectionRestore.tsapp/sagas/__tests__/state.test.tsapp/lib/services/connect.test.tsapp/sagas/__tests__/deepLinking.test.tsapp/lib/methods/getPermissions.tsapp/sagas/__tests__/login.test.tsapp/lib/services/connection.lifecycle.test.tsapp/lib/services/connect.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/subscriptions/room.ts
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/lib/services/connectionRestore.test.tsapp/reducers/server.test.tsapp/sagas/__tests__/state.test.tsapp/lib/services/connect.test.tsapp/sagas/__tests__/deepLinking.test.tsapp/sagas/__tests__/login.test.tsapp/lib/services/connection.lifecycle.test.tsapp/lib/methods/subscriptions/room.test.ts
📚 Learning: 2026-05-07T13:19:52.152Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7304
File: app/sagas/deepLinking.js:237-243
Timestamp: 2026-05-07T13:19:52.152Z
Learning: In this codebase’s Redux-Saga usage, remember that `yield put(action)` dispatches through the Redux store synchronously, and any saga(s) that synchronously react via action listeners (and synchronous `put` chains) will run to completion before the calling saga resumes at its next `yield`. As a result, within a single saga there is no scheduler interleaving between a `yield select(...)` and a subsequent `yield take(...)` at the next `yield` point, so a check-then-take pattern like `const state = yield select(...); if (state !== TARGET) { yield take(a => a.type === TARGET); }` is safe from TOCTOU races under the synchronous `put`/take model described above.
Applied to files:
app/sagas/state.jsapp/sagas/deepLinking.jsapp/sagas/login.js
🪛 ast-grep (0.44.1)
app/sagas/login.js
[warning] 91-91: Avoid using the initial state variable in setState
Context: setUser(result)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🔇 Additional comments (21)
app/sagas/__tests__/login.test.ts (1)
1-271: LGTM!app/reducers/server.test.ts (1)
27-49: LGTM!app/sagas/__tests__/state.test.ts (1)
1-101: LGTM!app/sagas/__tests__/deepLinking.test.ts (1)
773-774: 🎯 Functional CorrectnessNo duplicate
selectRequesteddeclaration found.app/sagas/state.js (1)
23-31: 🩺 Stability & AvailabilityNo change needed.
LOGIN.REQUESTis bound withtakeLatest, so foregrounding again during a resume-login backoff cancels the prior retry loop and starts a fresh attempt, so there is no concurrent resume risk.app/lib/services/connectionRestore.ts (1)
11-42: LGTM!app/lib/services/sdk.ts (1)
20-44: LGTM!app/lib/services/connectionRestore.test.ts (1)
1-111: LGTM!app/lib/methods/subscriptions/room.ts (1)
40-40: LGTM!Also applies to: 57-57, 78-79, 96-130
app/lib/methods/getSettings.ts (1)
147-152: LGTM!app/lib/services/connection.lifecycle.test.ts (1)
1-674: LGTM!app/lib/methods/getPermissions.ts (1)
209-215: 🩺 Stability & AvailabilityNo change needed.
app/index.tsximportsstore, which importssagasandreducers/index.ts; those imports statically bring inpermissions,login,init, andconnect, soconnect()registers stream restoration and the restorer-enrolling modules are enlisted before login/reconnect flows run.app/lib/services/connectionListeners.ts (1)
12-33: LGTM!app/lib/services/connect.ts (1)
102-140: LGTM!app/lib/services/connect.test.ts (2)
504-519: LGTM!
75-80: 🎯 Functional CorrectnessNo duplicate
mockBindStreamRestorationdeclaration is present.> Likely an incorrect or invalid review comment.app/lib/methods/subscriptions/room.test.ts (1)
20-24: LGTM!Also applies to: 236-290
app/lib/methods/getUsersPresence.ts (1)
182-187: LGTM!app/lib/methods/subscribeRooms.ts (1)
20-26: LGTM!app/sagas/login.js (2)
73-168: LGTM!
146-152: 🩺 Stability & AvailabilityNo change needed.
state.login.useris initialized to{}and is never null/undefined here; the 401 path falls through toyield put(loginFailure(e))as intended.
…of parking on LOGIN.SUCCESS A bare take(LOGIN.SUCCESS) after kicking the login pipeline parked the deep-link saga on LOGIN.FAILURE, SERVER.SELECT_FAILURE, or LOGOUT; a later unrelated LOGIN.SUCCESS could wake it and navigate with stale params. Race all terminal outcomes and skip navigation unless login actually succeeded.
|
|
||
| // Restorer owns only re-subscription; the awaited getPermissions() fetch stays in the login saga so | ||
| // permissions state is set before the enterprise-modules and VoIP checks that read it. | ||
| registerStreamRestorer(() => subscribePermissions()); |
There was a problem hiding this comment.
Shouldn't this be tied to a workspace?
| connecting: boolean; | ||
| connected: boolean; |
There was a problem hiding this comment.
Do we delete both? Not sure. I think connecting is an easier way to re-render.
|
iOS Build Available Rocket.Chat 4.75.0.109400 |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Waiting
|
Android Build Available Rocket.Chat 4.75.0.109403 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNQT9qjudPjLduWrFIzbdfezGu8xoPxhvjevG9pLsBdIYeTqx7MlsC0xX2c9c6NCzf-fOAXQ0C8TSF2oWFkS |
|
iOS Build Available Rocket.Chat 4.75.0.109404 |
1 participant
Proposed changes
After a transient disconnect (network blip, app background/foreground, server restart), DDP streams could die silently: the socket reconnected but stream subscriptions were never restored, so rooms stopped receiving messages until the user cleared cache or restarted the app. This traces back to
forceReopenbypassingconnect()(#7298), which was patched per-consumer over time (#7362, #7426, #7475) while the underlying ownership gap remained.This PR fixes the root causes and locks the behavior with a permanent test suite:
app/lib/services/connectionRestore.ts): every DDP stream consumer registers a restorer; the owner fans out on the DDPloginevent, keyed by socket generation, so all streams re-land on the current SDK instance after any reconnect — including SDK instance swaps that previously orphaned room listeners. The per-consumer patches from fix: re-subscribe rooms stream after forced socket reopen #7362/fix: re-subscribe room streams on DDP reconnect #7426/fix: re-subscribe room streams after re-authentication on reconnect #7475 are deleted, subsumed by the owner. Enrolled consumers: rooms list, open room, notify-user, settings, presence, permissions, roles.meteor.connectedguard that blocked recovery afterforceReopen(a real handshake now always re-runs the login pipeline), added bounded retry for transient resume-login failures, and made the foreground check dispatch a resume when a token exists but the session is unauthenticated.meteor.connected && login.isAuthenticated) instead of the staleserver.connectedredux flag, which was never reset on silent socket death. The lyingconnectedfield was removed from the server reducer.app/lib/services/connection.lifecycle.test.ts): binds the real production listeners (no copies) and asserts every enrolled stream survives every lifecycle transition — connect, close, forceReopen, resume-login failure/success, SDK swap, foreground reopen, deep-link warm start.Issue(s)
https://rocketchat.atlassian.net/browse/SUP-1078
How to test or reproduce
Automated:
Manual:
Screenshots
N/A — no UI changes.
Types of changes
Checklist
Further comments
The alternative — keep patching each consumer as its stream dies — was already tried three times (#7362, #7426, #7475) and each patch left holes proven by the new suite. Centralizing restoration in one generation-keyed owner makes "did my stream survive reconnect?" a contract the suite enforces for every consumer, present and future. Connection state duplicated across
meteor.connected,server.connectedand the socket was the shared enabler of these bugs; the redux flags are now display-only.Summary by CodeRabbit