fix(mobile): probe on network path changes, restore server leases on reconnect - #5154
fix(mobile): probe on network path changes, restore server leases on reconnect#5154t3dotgg wants to merge 1 commit into
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 adds new mobile network path change detection, modifies server idle timeout from 120s to 30s, and introduces connection probing/lease restoration logic - significant runtime behavior changes affecting mobile connectivity that warrant human review despite being well-tested. You can customize Macroscope's approvability policy. Learn more. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![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
b889ec4 to
9193ab0
Compare
Rebuilt on main after #5561 and #5404 landed overlapping reconnect work. What main already covers is dropped from this branch: the backoff ladder rework replaces our jitter, wakeProbeFailed replaces our probe-failure fast path, and the authoritative-session guard replaces our shell warm-cache short-circuit. What remains is the mobile-specific delta: - New advisory `network-path-changed` wakeup: WiFi<->cellular keeps isConnected true while invalidating the socket's path, so the mobile adapter emits a wakeup on interface-type changes while active and the supervisor probes the session (3s bound) instead of waiting for the ping timeout. A failed probe rides main's wakeProbeFailed fast path; flapping interfaces cannot cut backoff delays short. - Client activity is re-reported on every newly connected session generation; the AppState-triggered report races the reconnect and is dropped, leaving provider/VCS work paused server-side for up to 25s after a resume. Generation dedup is per-supervisor since replacements restart the counter. - The RPC onPingTimeout hook is wired to logging so zombie-socket disconnects are distinguishable from ordinary closes. - Server websocket idleTimeout drops to 30s from Bun's 120s default (clients ping every 5s, so live connections never idle), releasing a suspended phone's half-dead socket and buffers four times sooner. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9193ab0 to
8ab3326
Compare
t3dotgg
changed the title
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
1 participant
Mobile clients coming back from iOS backgrounding had two gaps left after #5561 and #5404 landed: a WiFi-to-cellular switch left the app hanging 5-10s until the ping timeout noticed the dead socket, and the server's activity lease lagged a resume by up to 25s because the AppState-triggered report races the reconnect and gets dropped — leaving provider/VCS work paused server-side.
This PR is the mobile-specific delta on top of main's reconnect work:
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. A failed probe rides the existingwakeProbeFailedfast path from fix: prevent reconnect loops during server stalls #5561; flapping interfaces cannot cut backoff delays short.onPingTimeouthook is wired to logging so zombie-socket disconnects are distinguishable from ordinary closes in telemetry.idleTimeoutdrops to 30s from Bun's 120s default — clients ping every 5s so live connections never idle, and a suspended phone's half-dead socket and its server-side buffers are released four times sooner.Earlier revisions of this branch also carried backoff jitter, a probe-failure fast path, and a shell warm-cache resume; those were superseded by #5561's ladder rework,
wakeProbeFailed, and authoritative-session guard, and have been dropped rather than re-fought.No native changes; everything ships over Metro. 587 client-runtime tests pass including three new network-path cases, repo typecheck clean.
Written by Claude Fable 5 via Claude Code, directed by Theo.
🤖 Generated with Claude Code
Note
Medium Risk
Touches connection lifecycle (supervisor wakeups, probes, backoff) and server WebSocket idle behavior; well-covered by new tests but affects all mobile reconnect paths.
Overview
Improves mobile reconnect and server-side lease freshness when the app stays “online” but the transport is stale.
Network path changes: Mobile emits an advisory
network-path-changedwakeup when the interface type flips (e.g. Wi‑Fi ↔ cellular) while active and still connected.EnvironmentSupervisortreats it like other foreground health checks: probe the live session with the shorter mobile probe timeout; on failure, reconnect without the first backoff rung. Path-change wakeups are ignored during backoff so flapping interfaces do not shorten retry delays.Activity leases: The background activity reporter now triggers a client-activity report on each new connected session generation, so a resume report that races a reconnect and fails no longer leaves provider/VCS work paused until the next 25s tick.
Server / RPC: Bun WebSocket
idleTimeoutdrops from 120s to 30s to drop suspended clients’ half-dead sockets sooner. RPC sessions log when a ping timeout kills a zombie socket.New supervisor tests cover path-change probe, immediate reconnect after a failed probe, and no backoff shortcut on path flaps during backoff.
Reviewed by Cursor Bugbot for commit 8ab3326. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Reconnect faster and reduce unnecessary resyncs after mobile backgrounding
network-path-changedwakeup type that fires when the device's network interface changes (e.g. WiFi → cellular) while online and active, via platform.tsEnvironmentSupervisornow probes the current session onnetwork-path-changed; if the probe fails, the subsequent reconnect skips the initial backoff rung for faster recoverynetwork-path-changedwakeups are ignored so they cannot shorten existing retry delayswebsocket.idleTimeout: 30son the Bun HTTP server, down from the default 120s, to reap stale WebSocket connections fasterMacroscope summarized 8ab3326.