fix(mobile): reconnect faster and stop resyncing more than needed after backgrounding - #5154
fix(mobile): reconnect faster and stop resyncing more than needed after backgrounding#5154t3dotgg wants to merge 4 commits into
Conversation
Five targeted fixes for mobile clients returning from iOS suspension: - Jitter the supervisor backoff ladder (equal jitter, [rung/2, rung]) so clients sharing an environment stop reconnecting in lockstep. - A failed foreground liveness probe now replaces the session immediately instead of paying the first backoff delay for a failure the probe just diagnosed. - New advisory `network-path-changed` wakeup: WiFi<->cellular keeps isConnected true while invalidating the socket's path, so the mobile adapter now emits a wakeup on interface-type changes while active and the supervisor probes the session instead of waiting ~5-10s for the ping timeout. - Report client activity on every newly connected session generation; the AppState-triggered report races the reconnect and is dropped, which left the server lease stale (provider/VCS work paused) for up to 25s after a resume. - Shell resumes a warm cache via afterSequence without the previously unconditional HTTP snapshot fetch, mirroring the thread-detail guard; foreground returns no longer pay an HTTP round trip. Supporting changes: wire the patched RPC onPingTimeout hook to logging so zombie-socket disconnects are distinguishable from ordinary closes, and set the server websocket idleTimeout to 30s (Bun defaults to 120s) so a suspended phone's half-dead socket and its server-side buffers are released four times sooner; live clients ping every 5s and never idle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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
Bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4abafa4. Configure here.
ApprovabilityVerdict: Needs human review This PR introduces significant behavioral changes to mobile connection handling including new network path change detection, retry jitter, immediate session replacement on failed probes, and changes to shell synchronization strategy. While well-tested, the scope of runtime behavior modifications warrants human review. You can customize Macroscope's approvability policy. Learn more. |
- Warm-cache shell resume now requires the completion marker; without it nothing promotes an already-caught-up resume to live, so legacy servers keep the HTTP snapshot path. - Seed the initial interface type from getNetworkStateAsync so the first WiFi/cellular flip after startup emits a wakeup; the listener only reports changes. - network-path-changed is ignored while waiting in backoff/offline/ blocked states: it is advisory (probe a connected session) and a flapping interface must not cut retry delays short. Regression test added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…shell path A replacement supervisor restarts its generation counter, so deduping connected events outside followStream could suppress the first connected event of a new supervisor and reintroduce the up-to-25s lease lag. The generation dedup now lives inside the per-supervisor stream, which switchMap resets when the supervisor is replaced. Also adds a test locking the legacy (no completion marker) shell resume to the HTTP snapshot path so the warm-cache short-circuit cannot regress it silently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
- Blocked probe failures (auth, permissions) keep their classification:
the supervisor now parks in blocked instead of churning immediate
reconnects when a foreground probe fails with ConnectionBlockedError.
- The environment-set dedup compares elements instead of join(","),
which was ambiguous for ids containing commas.
- switchMap uses its default concurrency so each environment-set change
interrupts the previous subscription set instead of leaking one per
change; mergeAll inside stays unbounded.
- The backoff ladder test reads the scheduled retryAt per rung and
asserts it lands in the jitter bounds [rung/2, rung], restoring real
coverage of ladder progression and the 16s cap.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patches/@effect__platform-bun@4.0.0-beta.102.patch (1)
5-22: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd focused tests for the Bun WebSocket patch behavior.
The current
perMessageDeflatetest runs withNodeHttpServer.layerTest, so it does not cover the Bun runtime. Add a companion test for both patched Bun server variants that exercises compression negotiation plus a compressed round trip, and add a separateidleTimeout: 30close-path test with deterministic close receipts or configuration.🤖 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/`@effect__platform-bun@4.0.0-beta.102.patch around lines 5 - 22, Add focused Bun runtime tests for both patched Bun server variants, alongside the existing perMessageDeflate coverage, verifying negotiation and a compressed client/server round trip. Add a separate Bun test for idleTimeout: 30 that deterministically verifies the connection-close receipt or configured timeout behavior, using the relevant Bun server test symbols and avoiding NodeHttpServer.layerTest.Source: Coding guidelines
🧹 Nitpick comments (2)
packages/client-runtime/src/state/shell.ts (1)
190-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the implementation narrative.
Replace this branch-by-branch comment with a short caller-contract comment, or extract the branch into a named helper. The current comment describes local behavior in detail instead of how
makeSubscribeInputis used.As per coding guidelines, “Use comments mainly to describe how a function is used; avoid annotating every line of behavior, and move comments when code moves.”
🤖 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 `@packages/client-runtime/src/state/shell.ts` around lines 190 - 196, Replace the detailed branch-by-branch comment near makeSubscribeInput with a concise caller-contract comment describing when this subscription input is used and its expected completion behavior. Do not document internal replay, snapshot, or HTTP-path mechanics there; preserve the existing implementation unchanged.Source: Coding guidelines
packages/client-runtime/src/state/shell-sync.test.ts (1)
50-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInfer the
sessionhelper return type.Remove
: RpcSession.RpcSession. If validation is required, applysatisfies RpcSession.RpcSessionto the returned object. The helper is local to this test file, so TypeScript can infer its return type.As per coding guidelines, “Prefer inferred types over explicit annotations and do not use
any.”🤖 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 `@packages/client-runtime/src/state/shell-sync.test.ts` around lines 50 - 58, Update the local session helper to remove the explicit RpcSession.RpcSession return annotation and let TypeScript infer the returned object type. If structural validation is needed, apply satisfies RpcSession.RpcSession to the returned object without changing its 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 `@packages/client-runtime/src/connection/supervisor.test.ts`:
- Around line 912-937: Replace the fixed Effect.yieldNow loop in the “does not
cut backoff short when the network path flaps” test with a deterministic typed
wait. Extend the visible harness or use TestClock so the test awaits
confirmation that both network-path-changed signals were consumed before
asserting prepareCount and supervisor.state.phase, while preserving the backoff
and no-early-retry assertions.
In `@packages/client-runtime/src/state/shell-sync.test.ts`:
- Around line 397-419: Replace the polling loops around the
“application-active”, “application-active-probe”, and
“application-active-reconnect” wakeups with typed resubscription receipts
emitted by the subscription handler, including the attempt number and
afterSequence. Await the corresponding receipt after each Queue.offer, then
retain the loaderCalls and subscriptionCount assertions; remove the arbitrary
scheduler-turn polling.
---
Outside diff comments:
In `@patches/`@effect__platform-bun@4.0.0-beta.102.patch:
- Around line 5-22: Add focused Bun runtime tests for both patched Bun server
variants, alongside the existing perMessageDeflate coverage, verifying
negotiation and a compressed client/server round trip. Add a separate Bun test
for idleTimeout: 30 that deterministically verifies the connection-close receipt
or configured timeout behavior, using the relevant Bun server test symbols and
avoiding NodeHttpServer.layerTest.
---
Nitpick comments:
In `@packages/client-runtime/src/state/shell-sync.test.ts`:
- Around line 50-58: Update the local session helper to remove the explicit
RpcSession.RpcSession return annotation and let TypeScript infer the returned
object type. If structural validation is needed, apply satisfies
RpcSession.RpcSession to the returned object without changing its behavior.
In `@packages/client-runtime/src/state/shell.ts`:
- Around line 190-196: Replace the detailed branch-by-branch comment near
makeSubscribeInput with a concise caller-contract comment describing when this
subscription input is used and its expected completion behavior. Do not document
internal replay, snapshot, or HTTP-path mechanics there; preserve the existing
implementation unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 697a6784-5144-44fc-9bb0-7f90021f9812
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/mobile/src/connection/background-activity.tsapps/mobile/src/connection/platform.tspackages/client-runtime/src/connection/supervisor.test.tspackages/client-runtime/src/connection/supervisor.tspackages/client-runtime/src/connection/wakeups.tspackages/client-runtime/src/rpc/session.tspackages/client-runtime/src/state/shell-sync.test.tspackages/client-runtime/src/state/shell.tspatches/@effect__platform-bun@4.0.0-beta.102.patch
| it.effect("does not cut backoff short when the network path flaps", () => | ||
| Effect.gen(function* () { | ||
| const harness = yield* makeHarness({ | ||
| prepare: () => Effect.fail(transient()), | ||
| }); | ||
| const supervisor = yield* EnvironmentSupervisor.make(TARGET_ENTRY, { | ||
| initiallyDesired: true, | ||
| }).pipe(Effect.provide(harness.dependencies)); | ||
|
|
||
| yield* awaitState( | ||
| supervisor.state, | ||
| (state) => state.phase === "backoff" && state.attempt === 1, | ||
| ); | ||
| expect(yield* Ref.get(harness.prepareCount)).toBe(1); | ||
|
|
||
| // Advisory path-change wakeups have no session to probe during backoff | ||
| // and must not trigger an early retry. | ||
| yield* harness.wake("network-path-changed"); | ||
| yield* harness.wake("network-path-changed"); | ||
| for (let attempt = 0; attempt < 20; attempt += 1) { | ||
| yield* Effect.yieldNow; | ||
| } | ||
| expect(yield* Ref.get(harness.prepareCount)).toBe(1); | ||
| expect((yield* SubscriptionRef.get(supervisor.state)).phase).toBe("backoff"); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the fixed Effect.yieldNow loop with a typed wait.
This test waits for two queued "network-path-changed" wakeups to be processed by looping Effect.yieldNow a fixed 20 times before asserting prepareCount and phase. This does not wait for a typed receipt that the supervisor consumed both signals; it relies on an arbitrary iteration count matching the current fiber-scheduling depth. If the internal signal-processing chain gets deeper (e.g., another yield* is added upstream), this test can pass or fail non-deterministically without a related regression in production code.
Use a typed receipt instead, for example by extending the harness to expose an effect that completes once the signals queue is drained, or by using TestClock.adjust to a known point (since waitForRetrySignal races against Effect.sleep(delayMs), advancing time deterministically resolves the race without a magic iteration count).
As per coding guidelines, "Tests must wait for typed receipts and worker drains in event-sourced async flows; do not use sleeps, polling, or arbitrary timeouts to make tests pass."
🤖 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 `@packages/client-runtime/src/connection/supervisor.test.ts` around lines 912 -
937, Replace the fixed Effect.yieldNow loop in the “does not cut backoff short
when the network path flaps” test with a deterministic typed wait. Extend the
visible harness or use TestClock so the test awaits confirmation that both
network-path-changed signals were consumed before asserting prepareCount and
supervisor.state.phase, while preserving the backoff and no-early-retry
assertions.
Source: Coding guidelines
| // Foreground wakeups resubscribe from the cached sequence and still | ||
| // skip the HTTP loader; catch-up is the server's sequence replay. | ||
| yield* Queue.offer(wakeups, "application-active"); | ||
| yield* SubscriptionRef.changes(shellState).pipe( | ||
| Stream.filter( | ||
| (value) => | ||
| value.status === "synchronizing" && | ||
| Option.isSome(value.snapshot) && | ||
| value.snapshot.value.snapshotSequence === 20, | ||
| ), | ||
| Stream.runHead, | ||
| ); | ||
|
|
||
| for (let attempt = 0; attempt < 100; attempt += 1) { | ||
| if ((yield* Ref.get(subscriptionCount)) >= 2) break; | ||
| yield* Effect.yieldNow; | ||
| } | ||
|
|
||
| expect(yield* Ref.get(loaderCalls)).toBe(2); | ||
| expect(yield* Ref.get(loaderCalls)).toBe(0); | ||
| expect(yield* Ref.get(subscriptionCount)).toBe(2); | ||
|
|
||
| yield* Queue.offer(wakeups, "application-active-probe"); | ||
| for (let attempt = 0; attempt < 100; attempt += 1) { | ||
| if ((yield* Ref.get(subscriptionCount)) >= 3) break; | ||
| yield* Effect.yieldNow; | ||
| } | ||
| expect(yield* Ref.get(loaderCalls)).toBe(3); | ||
| expect(yield* Ref.get(loaderCalls)).toBe(0); | ||
| expect(yield* Ref.get(subscriptionCount)).toBe(3); | ||
|
|
||
| yield* Queue.offer(wakeups, "application-active-reconnect"); | ||
| for (let attempt = 0; attempt < 10; attempt += 1) { | ||
| yield* Effect.yieldNow; | ||
| } | ||
| expect(yield* Ref.get(loaderCalls)).toBe(3); | ||
| expect(yield* Ref.get(loaderCalls)).toBe(0); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Wait for typed resubscription receipts.
The loops poll subscriptionCount for an arbitrary number of scheduler turns. A slow scheduler can run the subscription after the loop and cause a false test failure. Emit a typed receipt from the subscription handler, such as its attempt number and afterSequence, then await that receipt after each wakeup.
As per coding guidelines, “Tests must wait for typed receipts and worker drains in event-sourced async flows; do not use sleeps, polling, or arbitrary timeouts to make tests pass.”
🤖 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 `@packages/client-runtime/src/state/shell-sync.test.ts` around lines 397 - 419,
Replace the polling loops around the “application-active”,
“application-active-probe”, and “application-active-reconnect” wakeups with
typed resubscription receipts emitted by the subscription handler, including the
attempt number and afterSequence. Await the corresponding receipt after each
Queue.offer, then retain the loaderCalls and subscriptionCount assertions;
remove the arbitrary scheduler-turn polling.
Source: Coding guidelines
1 participant
Coming back to the app after backgrounding on iOS was slower than it needed to be: a resume whose health probe failed still waited out a backoff delay before reconnecting, every foreground return refetched the full shell snapshot over HTTP even with a warm cache, and a WiFi-to-cellular switch left the app hanging 5-10s until the ping timeout noticed the dead socket. On top of that, all clients of an environment retried in lockstep after a restart, and the server kept a suspended phone's half-dead socket (and the unbounded buffers feeding it) alive for Bun's default 120s.
This PR fixes each of those:
afterSequence(the server replays the gap, or sends a fresh socket snapshot pastSHELL_RESUME_MAX_GAP), mirroring the existing thread-detail guard. Foreground returns no longer pay an HTTP round trip.network-path-changedwakeup: the mobile adapter watches the interface type and the supervisor probes the session (3s bound) on WiFi/cellular flips instead of waiting for the ping timeout.[rung/2, rung]) so clients stop reconnecting in lockstep. Active and network-restored fast paths stay immediate.idleTimeoutis now 30s instead of Bun's 120s default (clients ping every 5s, so live connections never idle), and the RPConPingTimeouthook is wired to logging so zombie-socket disconnects are distinguishable from ordinary closes.No native changes; everything ships over Metro. Client-runtime suite passes (510 tests, including new coverage for the path-change probe and immediate replacement), server suite passes (1825), repo typecheck clean.
Written by Claude Fable 5 via Claude Code, directed by Theo.
🤖 Generated with Claude Code
Note
Medium Risk
Touches core connection lifecycle, shell resume semantics, and server WebSocket idle behavior; changes are broad but heavily covered by supervisor and shell-sync tests.
Overview
Improves mobile resume and network churn by changing how the connection supervisor and shell sync behave, plus tighter server WebSocket idle handling.
Connection supervisor probes the live session on
network-path-changed(WiFi ↔ cellular) and on foreground wakeups; a transient or timed-out probe failure replaces the session immediately instead of waiting in backoff. Blocked probe failures still park inblocked. Path-change wakeups are advisory during backoff so flapping interfaces do not shorten retry delays. Retry delays use equal jitter on the existing ladder so clients do not reconnect in lockstep.Mobile platform emits
network-path-changedwhen the interface type changes while the app is active and still “online.” Background activity re-reports client activity on each new connected session generation, because the AppState report can fail mid-reconnect and leave server leases stale for up to the report interval.Shell state with a warm cache and
shellResumeCompletionMarkernow resubscribes via WebSocketafterSequencefrom the cached sequence (server replays the gap); HTTP snapshot fetch remains for cold start or servers without the marker. Foreground resubscribes stay off the HTTP path.RPC session logs ping timeouts for clearer telemetry. Bun WebSocket patch adds 30s
idleTimeout(vs 120s) so suspended clients release sooner; lockfile reflects the updated patch hash.Reviewed by Cursor Bugbot for commit b889ec4. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix mobile reconnection to probe on network changes and skip HTTP resync on warm cache
shellResumeCompletionMarkersupport, shell state resumes from the cached sequence over the socket, skipping an HTTP snapshot fetch on foreground resubscribes.network-path-changedis ignored as a backoff-shortening signal when not yet connected, and probe timeouts now vary by wakeup reason.Macroscope summarized b889ec4.
Summary by CodeRabbit
New Features
Bug Fixes