fix: re-arm reconnect ladder when a forced socket reopen fails - #7553
fix: re-arm reconnect ladder when a forced socket reopen fails#7553Rohit3523 wants to merge 5 commits into
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
WalkthroughThe DDP socket now supports bounded immediate reconnection, socket replacement, and ping-based liveness checks. ChangesDDP recovery flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DDPDriver
participant Socket
participant WebSocket
Client->>DDPDriver: reopenNow()
DDPDriver->>Socket: reopenNow()
Socket->>WebSocket: create replacement connection
WebSocket-->>Socket: open or timeout/error
Socket-->>DDPDriver: reconnect result
DDPDriver-->>Client: resolve operation
Client->>DDPDriver: probe()
DDPDriver->>Socket: probe()
Socket->>WebSocket: send ping
WebSocket-->>Socket: pong
Socket-->>DDPDriver: liveness result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
75-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not resolve
open()after a failed forced reconnect.If
reopenNow()fails, it resolvesreopenPromiseafter it re-arms the reconnect ladder. This branch then resolvesopen()withthis.connection, even when that socket is not open. A caller can treatopen()as successful and send on a failed or connecting socket.Reject when
this.connectedis false, or wait for a subsequentopenevent before resolving.Proposed fix
if (this.reopenPromise) { - return this.reopenPromise.then(() => resolve(this.connection)).catch(reject) + return this.reopenPromise + .then(() => { + if (!this.connected || !this.connection) { + throw new Error('Forced reconnect did not establish a socket') + } + resolve(this.connection) + }) + .catch(reject) }🤖 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 75 - 76, Update the reopenPromise branch in open() so it does not resolve after a failed forced reconnect: after reopenPromise settles, resolve only when this.connected is true, otherwise reject (or wait for a subsequent open event). Ensure open() never reports success with a failed or still-connecting this.connection.
🧹 Nitpick comments (2)
patches/@rocket.chat+sdk+1.3.3-mobile.patch (2)
334-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
anywithDDPDriver.
ddpexposes the new DDP-specific API.anydisables type checking at this public boundary. ImportDDPDriveras a type and declareddp?: DDPDriver.As per coding guidelines, TypeScript code must use type safety for object shapes.
🤖 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 at line 334, Replace the any type on the public ddp property with DDPDriver, importing DDPDriver as a type from the appropriate module. Preserve the property’s optional nature while restoring type checking for the DDP-specific API.Source: Coding guidelines
290-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
asyncandtry/catchfor subscription acknowledgements.
resubscribehandles an asynchronous operation with a.then()chain. Make it anasyncfunction and usetry/catch.As per coding guidelines, prefer
async/awaitand use explicittry/catchhandling for async operations.🤖 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 290 - 294, Update the resubscribe function to be async and await the Promise.all subscription acknowledgements inside a try/catch block. Return true after all subscriptions succeed and false from the catch path when any subscription fails, preserving the current behavior without the .then()/.catch() chain.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.
Outside diff comments:
In `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch:
- Around line 75-76: Update the reopenPromise branch in open() so it does not
resolve after a failed forced reconnect: after reopenPromise settles, resolve
only when this.connected is true, otherwise reject (or wait for a subsequent
open event). Ensure open() never reports success with a failed or
still-connecting this.connection.
---
Nitpick comments:
In `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch:
- Line 334: Replace the any type on the public ddp property with DDPDriver,
importing DDPDriver as a type from the appropriate module. Preserve the
property’s optional nature while restoring type checking for the DDP-specific
API.
- Around line 290-294: Update the resubscribe function to be async and await the
Promise.all subscription acknowledgements inside a try/catch block. Return true
after all subscriptions succeed and false from the catch path when any
subscription fails, preserving the current behavior without the .then()/.catch()
chain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e1b538ac-44e1-47cd-b356-0ac1bbfe8958
📒 Files selected for processing (2)
app/lib/services/ddpSocket.test.tspatches/@rocket.chat+sdk+1.3.3-mobile.patch
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: E2E Shard Preflight
🧰 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/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/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/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/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/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 (2)
patches/@rocket.chat+sdk+1.3.3-mobile.patch (1)
9-9: LGTM!Also applies to: 25-61, 96-103, 118-166, 172-208, 218-218, 227-227, 235-249, 259-262, 273-289, 295-321, 343-346
app/lib/services/ddpSocket.test.ts (1)
209-231: LGTM!Also applies to: 233-254, 256-275, 277-287
none are being changed in this PR, with reasoning below.
Correct about the contract, but out of scope here and not currently reachable. This PR only changes reopenNow; open() is pre-existing patch content from the earlier socket-health work. open() has three callers and only reopen() consumes its resolution — checkAndReopen ignores it, DDPDriver.connect only uses .catch. So a false resolve costs exactly one thing: reopen()'s catch doesn't re-arm the ladder. This PR makes reopenNow's cleanup re-arm the ladder unconditionally whenever the attempt didn't produce a live socket, which covers that window, and once cleanup has run reopenPromise is deleted so open() no longer short-circuits. I couldn't construct a case where the false resolve strands a session after this change. The suggested fix also needs a companion change: checkAndReopen calls this.open() with no .catch, so making open() reject would trade this wart for an unhandled rejection. I'll fold both into the next change that touches this area.
This wouldn't restore any type checking, so I'd rather not add an annotation that implies it did. The package ships index.d.ts containing only declare module '@rocketchat/sdk'; — not even the correct package name, the dot is missing — and package.json#types points at it, so the whole module resolves to any in app code. tsc --noEmit accepts all of the following without error: sdk.current.thisPropertyDoesNotExist
Pre-existing code inside waitForNotifyUserMediaSubs, untouched by this PR. Promise.all(...).then(() => true).catch(() => false) is behaviourally identical to the async/await form, so this is purely stylistic — better left to whoever next edits that function. |
GitHub Actions
Waiting
1 participant
Proposed changes
When the app was brought back to the foreground while still offline (e.g. airplane mode),
reopenNow()performs the immediate socket attempt but cancels the retry ladder timer first. If that attempt failed, nothing was left scheduled — and a stale timer id disabled the ladder for the rest of the session — so the session stayed disconnected until the next foreground event, even after connectivity returned.Now a failed forced reopen re-arms the retry ladder and drops the stale id, so the app reconnects on its own once the network is back.
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1461
How to test or reproduce
Expected: within one retry interval the room header stops showing "Waiting for network…" and the messages sent in step 2 load.
Without this fix the header stays on "Waiting for network…" indefinitely —
meteor.connectednever returns totrue— and the messages never arrive, until another background → foreground cycle forces a reconnect. Pull-to-refresh in RoomsListView loads the messages over REST but leaves the socket dead, which is why the room can look partly recovered while the banner persists.Note on timing: the retry interval is 5s on a release build and 20s in debug (
reopen: __DEV__ ? 20000 : 5000inapp/lib/services/sdk.ts), so on a debug build allow ~20s after disabling airplane mode before judging the result.Types of changes
Checklist
Further comments
Root cause.
reopenNow()in the patched DDP driver gets its "immediate" attempt by cancelling the pending ladder timer — but it only calledclearTimeout(), never deleting theopenTimeoutid.reopen()guards onif (this.openTimeout) return, soopenTimeoutis not merely a timer handle: it is the flag for "a tick is already scheduled". A cleared-but-still-set id therefore silenced the entire ladder for the rest of the session, including the re-armonCloseattempts on every later close. So a single foreground while offline did not just cancel one tick — it permanently removed the app's ability to schedule another.On top of that, when the forced attempt failed (device offline / server unreachable), the old cleanup path resolved the shared promise and left nothing scheduled, so that one failed reconnect meant the session sat disconnected with no retry path until the next explicit trigger.
Why this fix. The ladder was already the correct place to own retry scheduling; forcing a reopen just needed to not sabotage it. So instead of adding a parallel retry timer in the caller,
reopenNow()now (a) deletes the cancelled id soreopen()can schedule again, and (b) re-arms the ladder —this.reopen()— whenever the attempt did not produce a live socket (on create error or the 10s deadline). A successfulopendeliberately skips the re-arm so no redundant tick is left behind.reopenPromiseis deleted before the ladder is re-armed so a ladder tick callingopen()cannot short-circuit onto the already-settled reopen promise.The two parts are complementary. Deleting the id alone would let
onCloserecover the ladder, but a socket that never opens may never emitclose, so the re-arm covers that case. The re-arm alone does nothing at all while the stale id is still present — an earlier revision of this fix had only the re-arm, passed the whole unit suite, and was a complete no-op on device.Verification. Beyond the unit tests, verified on an Android emulator against a live server: three airplane-mode cycles following the steps above all recovered — banner cleared,
meteor.connectedback totrue, and every message sent during the offline window arrived. A control cycle with no background/foreground step confirms the path that already worked is not regressed.Summary by CodeRabbit