fix: Reopen a frozen DDP socket by correlating the health round trip - #7549
fix: Reopen a frozen DDP socket by correlating the health round trip#7549Rohit3523 wants to merge 3 commits into
Conversation
…ened
After the OS freezes a backgrounded app past the ping deadline, the server drops
the DDP session. On resume the app decided the socket was still usable, so it was
never reopened: no `close`, no `connecting`, no resume login, and `subscribeAll()`
never re-sent the streams. An open room went deaf for the rest of the session and
only appeared to heal because re-entering it refetches over REST.
Two pieces of evidence were wrong.
`probe()` had no request/response correlation. It sent `{msg:'ping'}` with no id
and resolved on the first `pong` event, so a pong flushed from the pre-freeze
receive buffer — or one the client provoked by auto-replying to a queued server
ping — vouched for a session that was already gone. Its `lastPing <=
lastPingAtStart` check could not close this: `onMessage` refreshes `lastPing`
before emitting, so it only proved something arrived. The ping now carries an id
and only the pong echoing it counts, registered with `on` rather than `once` so an
uncorrelated pong cannot consume the listener.
`classifySocketHealth` aged the socket against `lastPing`, which `onMessage`
refreshes for every inbound frame. The backlog the OS flushes on resume made a
socket frozen for minutes look seconds old, sending it to a round trip instead of
a reopen. It now ages against `lastPongAt`, advanced only by a pong and by the
handshake reply, and falls back to `lastPing` on a driver without the patch.
Verified against a live server, which echoes the ping id, and on an Android
emulator: the freeze now produces connect -> login {resume} -> stream-room-messages
re-subscribed, and messages sent during and after the freeze both arrive.
Note the reported cause — that the reopen emits no `close`, leaving redux
connected and skipping the login — does not hold: `createConnection` emits
`connecting`, which clears `meteor.connected`, so a real reopen does run the
login. Tests in connect.test.ts pin that behaviour against the real reducer.
GitHub Actions
Error
WalkthroughThe patch adds pong-aware socket health checks, correlated liveness probes, guarded socket replacement, immediate reconnection, resume-login coverage, and media subscription resynchronization. ChangesDDP liveness and recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConnectReducer
participant DDPDriver
participant Socket
participant DDPServer
ConnectReducer->>DDPDriver: detect silent socket replacement
DDPDriver->>Socket: probe()
Socket->>DDPServer: send ping with unique ID
DDPServer-->>Socket: return matching pong
Socket-->>DDPDriver: resolve probe
DDPDriver->>Socket: reopenNow() when health remains stale
DDPDriver-->>ConnectReducer: emit disconnected and connected
ConnectReducer->>DDPDriver: dispatch resume login
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 (2)
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 review |
✅ Action performedReview finished.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patches/@rocket.chat+sdk+1.3.3-mobile.patch (1)
159-181: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep the reconnect guard until the connection attempt terminates.
Line 179 discards a
createConnection()failure. Line 181 then callscleanup(), which deletesreopenPromiseand resolves waiting callers even when noopenevent occurred. A later health check can start anothercreateConnection()while the first WebSocket is still connecting. Lines 70-76 then close that first socket.Make the deadline cancel or fail the active attempt. Resolve only after
open. Reject or return an explicit failure result on error and timeout. Add coverage for a replacement socket that never opens.🤖 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 `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch around lines 159 - 181, Update the reconnect flow around reopenPromise and createConnection so the reconnect guard remains active until the connection attempt actually terminates. Propagate createConnection failures and make the deadline cancel or fail the active attempt; do not resolve or delete reopenPromise unless open occurs, and reject or return an explicit failure on error or timeout. Add coverage for a replacement socket that never emits open, ensuring a later health check cannot start a concurrent connection attempt.
🧹 Nitpick comments (1)
app/lib/services/connect.test.ts (1)
428-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd return types to the new named test helpers.
app/lib/services/connect.test.ts#L428-L440: declare(): () => IConnect.app/lib/services/connect.test.ts#L454-L457: declare(): Array<{ type: string; credentials?: unknown }>.app/lib/services/__tests__/socketHealth.integration.test.ts#L110-L112: declare: void.app/lib/services/__tests__/socketHealth.integration.test.ts#L259-L261: declare: void.As per coding guidelines, add explicit type annotations to function parameters and return types.
🤖 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/services/connect.test.ts` around lines 428 - 440, Explicitly annotate the named test helpers’ return types: app/lib/services/connect.test.ts lines 428-440 should return () => IConnect, and lines 454-457 should return Array<{ type: string; credentials?: unknown }>; app/lib/services/__tests__/socketHealth.integration.test.ts lines 110-112 and 259-261 should return void. Add parameter annotations where required by the coding guidelines without changing helper behavior.Source: Coding guidelines
🤖 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/lib/services/ddpSocket.test.ts`:
- Around line 148-159: Update the test around socket.probe() to store both
returned promises, then await/assert their timeout results after
advanceTimersByTimeAsync(2000). Preserve the distinct-ID assertion while
ensuring both Promise<boolean> values are explicitly handled so rejections fail
the test.
---
Outside diff comments:
In `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch:
- Around line 159-181: Update the reconnect flow around reopenPromise and
createConnection so the reconnect guard remains active until the connection
attempt actually terminates. Propagate createConnection failures and make the
deadline cancel or fail the active attempt; do not resolve or delete
reopenPromise unless open occurs, and reject or return an explicit failure on
error or timeout. Add coverage for a replacement socket that never emits open,
ensuring a later health check cannot start a concurrent connection attempt.
---
Nitpick comments:
In `@app/lib/services/connect.test.ts`:
- Around line 428-440: Explicitly annotate the named test helpers’ return types:
app/lib/services/connect.test.ts lines 428-440 should return () => IConnect, and
lines 454-457 should return Array<{ type: string; credentials?: unknown }>;
app/lib/services/__tests__/socketHealth.integration.test.ts lines 110-112 and
259-261 should return void. Add parameter annotations where required by the
coding guidelines without changing helper behavior.
🪄 Autofix
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: 73eaccb9-bc86-441c-b0a7-b6f295bb3671
📒 Files selected for processing (6)
app/lib/services/__tests__/socketHealth.integration.test.tsapp/lib/services/__tests__/socketHealth.test.tsapp/lib/services/connect.test.tsapp/lib/services/ddpSocket.test.tsapp/lib/services/socketHealth.tspatches/@rocket.chat+sdk+1.3.3-mobile.patch
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/socketHealth.tsapp/lib/services/__tests__/socketHealth.test.tsapp/lib/services/connect.test.tsapp/lib/services/__tests__/socketHealth.integration.test.tsapp/lib/services/ddpSocket.test.ts
**/*.{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 numbers
Files:
app/lib/services/socketHealth.tsapp/lib/services/__tests__/socketHealth.test.tsapp/lib/services/connect.test.tsapp/lib/services/__tests__/socketHealth.integration.test.tsapp/lib/services/ddpSocket.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Format JavaScript and TypeScript code with Oxfmt using the repository configuration: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where possible, and same-line brackets.
Follow Oxlint rules configured in.oxlintrc.json, including the import, React, Jest, TypeScript, and React Native plugins.
Files:
app/lib/services/socketHealth.tsapp/lib/services/__tests__/socketHealth.test.tsapp/lib/services/connect.test.tsapp/lib/services/__tests__/socketHealth.integration.test.tsapp/lib/services/ddpSocket.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/socketHealth.tsapp/lib/services/__tests__/socketHealth.test.tsapp/lib/services/connect.test.tsapp/lib/services/__tests__/socketHealth.integration.test.tsapp/lib/services/ddpSocket.test.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/__tests__/socketHealth.test.tsapp/lib/services/connect.test.tsapp/lib/services/__tests__/socketHealth.integration.test.tsapp/lib/services/ddpSocket.test.ts
📚 Learning: 2026-05-05T21:08:33.177Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7298
File: patches/@rocket.chat+sdk+1.3.3-mobile.patch:79-79
Timestamp: 2026-05-05T21:08:33.177Z
Learning: In the RocketChat/Rocket.Chat.ReactNative repo, for patches under patches/*.patch (especially those touching rocket.chat/sdk), remember that the patching sandbox's node_modules reflects the pre-patch state. Grepping node_modules for symbols (e.g., userDisconnectCloseCode = 4000 in node_modules/rocket.chat/sdk/lib/drivers/ddp.ts) can yield false positives. Review patches by inspecting the diff and applying it to a fresh copy of the SDK or diffing against the SDK source, rather than relying on node_modules. Ensure the patch actually introduces/updates symbols in the SDK source and run the test suite to validate behavior after applying the patch.
Applied to files:
patches/@rocket.chat+sdk+1.3.3-mobile.patch
🔇 Additional comments (6)
patches/@rocket.chat+sdk+1.3.3-mobile.patch (1)
5-142: LGTM!Also applies to: 187-359, 372-384
app/lib/services/ddpSocket.test.ts (1)
15-16: LGTM!Also applies to: 73-83, 109-146
app/lib/services/socketHealth.ts (1)
12-13: LGTM!Also applies to: 37-41
app/lib/services/__tests__/socketHealth.test.ts (1)
18-18: LGTM!Also applies to: 75-91
app/lib/services/__tests__/socketHealth.integration.test.ts (1)
2-2: LGTM!Also applies to: 29-35, 54-56, 106-109, 144-144, 156-156, 190-190, 206-206, 235-235, 248-258, 262-327, 329-330
app/lib/services/connect.test.ts (1)
1-8: LGTM!Also applies to: 422-427, 442-452, 459-489
The two `socket.probe()` promises were discarded, so a rejection would surface as an unhandled rejection instead of failing the test, and neither timeout result was asserted. Hold both and assert each resolves false.
GitHub Actions
Error
|
@coderabbitai review |
✅ Action performedReview finished.
|
GitHub Actions
Inactive
Baseline results on unfixed
|
| blind cycles | |
|---|---|
unfixed develop (clean install from lockfile) |
1 / 30 (cycle 3) |
| this branch | 0 / 30 |
"Blind" = a message posted after unlock never arrived over stream-room-messages, i.e. the ticket's worst-case symptom.
This does not statistically demonstrate the fix
0/30 vs 1/30 is a single event of difference; Fisher's exact gives p ≈ 1.0. At the ticket's own rate (3/28 ≈ 11%) you would need far more cycles per arm to separate the two. So the soak shows no regression and confirms the defect is still reachable on develop, but the evidence that this change fixes it rests on the deterministic unit/integration repros, which fail without it and pass with it.
And the one baseline failure is probably a different defect
Cycle 3's wire trace:
517s OUT connect <- unfroze exactly on schedule (125s freeze, valid window)
519s OUT login
519s OUT sub-room <- stream re-subscribed
... BLIVE-3 posted ~110s later, never received ...
827s OUT connect <- cycle 4
It reopened, re-authenticated and re-subscribed, and still lost the message. This PR makes a frozen socket reopen; it cannot help a case where the reopen already happened. Candidate causes: the sub frame written but never registered server-side, a re-subscribe rejected as a duplicate id, or the sync/resubscribe ordering handled on room-resync-after-resubscribe. Not diagnosed — I did not record ready/nosub frames.
Harness caveat
11 of 30 baseline cycles overran their script because adb/curl intermittently stall on a dozing device. Cycle 3 was one of them, but its stall landed after unfreeze (proven by the on-time connect at 517s), so its freeze was a valid 125s and the result stands. For other stalled cycles the freeze itself overran, putting them outside the 90–170s window. Any future run should wrap every adb/curl in timeout so a hang cannot reshape a cycle.
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.76.0.109500 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNS63USN3e6gEwOGU0hw58LUtTdWNIWjM0zY5cgOw8iOG6yBgL4mnEQO5yddfTvJ-y97PPdOqsNRmlHpSiOX |
|
iOS Build Available Rocket.Chat 4.76.0.109501 |
GitHub Actions
Waiting
1 participant
Proposed changes
After the phone sleeps long enough for the DDP ping to go stale, an open room stops showing new messages for the rest of the session. Leaving and re-entering makes them appear — which looks like a rendering bug but is not: that is
RoomViewrefetching over REST while the server-side stream stays dead.Two pieces of evidence used to answer "is this socket still usable?" were wrong. Either one alone can leave a dead session in place.
1.
probe()had no request/response correlation. It sent{msg:'ping'}with no id and resolved on the firstpongevent. When the OS unfreezes a backgrounded app it flushes the frames buffered during the freeze, so a pong answering a pre-freeze ping — or one the client provoked by auto-replying to a queued server ping — vouched for a session the server had already dropped. ThelastPing <= lastPingAtStartcheck could not close this, becauseonMessagerefresheslastPingbefore emitting: it only proved something arrived, not that the server answered us.2.
classifySocketHealthaged the socket againstlastPing, whichonMessagerefreshes for every inbound frame — so it measures traffic, not heartbeat health. One flushed message made a socket frozen for minutes look seconds old, sending it to a round trip instead of straight to a reopen.When both misfire nothing is reopened: no
close, noconnecting, redux staysconnected, the resume login never runs,subscribeAll()never re-sends the streams, and the room is deaf until something else triggers a sync.This PR makes the ping carry an id and accept only the pong echoing it, and adds a
lastPongAtclock that only a real server answer advances.patches/@rocket.chat+sdk+1.3.3-mobile.patchprobe()puts an id on its ping and accepts only the matching pong, registered withonrather thanonceso an uncorrelated pong cannot consume the listener. AddslastPongAt— advanced only by a pong and by the handshake reply, zeroed onreopenNow— exposed via aDDPDrivergetter.app/lib/services/socketHealth.tslastPongAt, falling back tolastPingon a driver without the patch.The patch was regenerated from a pristine package extract and verified to reproduce
node_modulesbyte-for-byte.Correction to the root cause on the ticket
The ticket attributes this to the reopen emitting no
close, leaving redux connected and skipping the login. That does not hold.createConnection()emitsconnecting, whichconnect.tsmaps toconnectRequest()and clearsmeteor.connected— so a real reopen does run the resume login. Verified two ways: a test inconnect.test.tsdriving the realconnectreducer, and a device wire trace showingmethod: login {resume}followed by 25subframes.The observations on the ticket are right; the attribution is one layer too low. The truth is simpler — no reopen happened at all, which by itself explains both the missing
closeand the missing login request. Flagging it because a fix aimed at the stated cause would have changed nothing.Issue(s)
NATIVE-1471
How to test or reproduce
Affected screen: RoomView (any open room). Android emulator:
adb shell dumpsys battery unplug, thenadb shell dumpsys deviceidle force-idle. Plain screen-off does not starve the socket.adb shell wm dismiss-keyguard. (input keyevent 82opens the RN dev menu on debug builds.)Before: the room does not show B's message, and messages sent live after unlock are also missing. Leaving and re-entering the room makes them appear.
After: B's message appears in the open room without leaving it, and messages sent live after unlock arrive too.
Signals beyond the UI — after unlock the wire shows, in order:
Tip for repeated cycles: use
adb shell input keyevent 223/224(SLEEP / WAKEUP) rather than26(POWER). 26 is a toggle and drifts out of phase, which silently inverts the freeze phases.Screenshots
Not applicable — no visual change. The observable difference is message delivery; evidence is the wire trace above and the soak results below.
Types of changes
Checklist
patches/Further comments
Why this approach
A liveness check that cannot recognise its own answer is not a liveness check. DDP ping/pong carries an optional id and the server echoes it, so correlating is the smallest change that makes the round trip mean what its name says. It fails safe: an unanswered probe reopens rather than trusting a dead socket.
lastPongAtis defence in depth — with correlation in place, either fix alone resolves the reported symptom. It also stops a misleadingly-named field from being read as "last heartbeat" when it means "last frame".Alternative considered and rejected: always reopen after any background gap. That discards the round-trip check and pays a full resume login,
subscribeAll()and rooms refetch on every app switch.Verification
HeadersWithFormatting) also fails on cleandevelop.{msg:'ping',id:'probe-0'}→{msg:'pong',id:'probe-0'}. This is a prerequisite: without the echo,probe()would always time out and every gray-zone foreground would pay a needless reconnect.Summary by CodeRabbit
Bug Fixes
Improvements