Uh oh!
There was an error while loading. Please reload this page.
fix(mobile): probe network changes and refresh activity leases - #5154
fix(mobile): probe network changes and refresh activity leases#5154t3dotgg wants to merge 4 commits into
Conversation
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the main mobile issue, the intended fix, testing, limitations, and lack of UI changes. However, it omits several changes in the pull request, including warm shell resume, jittered backoff, RPC ping-timeout logging, and the 30-second Bun WebSocket idle timeout. It also conflicts with the changeset by stating that the Bun idle-timeout change was removed. Resolution Update the description using the repository template. Add explicit What Changed, Why, UI Changes, and Checklist sections. Document all current changes, including warm shell resume, jittered reconnect backoff, network-path wakeups, activity reporting, RPC ping-timeout logging, and the 30-second Bun WebSocket idle timeout. Resolve the conflicting statements about retry delays and the Bun idle timeout.
Comment |
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This change alters production connection behavior across mobile lifecycle handling, the shared supervisor, and activity leases: network transitions can trigger probes, reconnects, and additional reports. Although the behavior is covered by focused tests, the cross-component runtime impact merits human review. You can add or adjust custom eligibility rules. Learn more. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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
9193ab0 to
8ab3326CompareThread 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. |
8ab3326 to
68b85b9CompareUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
One change-discipline finding: documentation lost while moving the WebSocket options out of server.ts. Everything else in the changed Effect service code (supervisor wakeup handling, wakeups.ts union, mobile platform layer, rpc/session.ts hooks) follows the conventions.
Posted via Macroscope — Effect Service Conventions
Uh oh!
There was an error while loading. Please reload this page.
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> Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Keep advisory interface probes and activity reports after reconnect. Leave server socket idle timeouts unchanged. Test report recovery and the short probe timeout.
eff1bf7 to
1d5093bCompare
Mobile can miss Wi-Fi/cellular changes and leave activity leases stale after reconnecting.
Probe the current session when the active network interface changes. Keep retry delays unchanged, and report current activity after each newly connected session generation. Keep the current session owner and credential handling. The earlier Bun-wide idle-timeout change is removed.
Verified on current main with 77 focused tests, mobile and client-runtime typechecks, targeted lint, and formatting. The new activity test waits for typed debounce and report receipts.
Device background/resume, real network switching, and live relay/tunnel behavior were not tested. No UI changes.
Original work by Theo Browne and Julius Marminge with Claude Code and Codex. Original commit authors and co-authors are preserved. Updated by GPT-5.6 Sol in Codex.
Note
Medium Risk
Touches connection supervisor wakeup and probe/reconnect logic plus mobile lifecycle networking; mistakes could cause reconnect storms or stale leases, but behavior is heavily tested and path-change wakeups are explicitly non-disruptive during backoff.
Overview
Mobile now detects Wi‑Fi/cellular interface changes (even when
isConnectedstays true) and emits anetwork-path-changedwakeup while the app is active and online. The shared supervisor probes the current session with the 3s mobile probe timeout; a failed or timed-out probe reconnects immediately without shell resubscription. During backoff, path-change wakeups are ignored so flapping interfaces do not shorten retry delays.The mobile background activity reporter triggers another debounced
serverReportClientActivitywhen an environment reaches a new connected session generation, so activity leases are restored after reconnect if an earlier report had no RPC session.Supporting changes: pure
network-path-changehelpers and unit tests, ping timeout info logging on RPC sessions, supervisor/wakeup tests (including a queue-based test harness), shell-sync test tightening, and user/internal docs for mobile network behavior.Reviewed by Cursor Bugbot for commit 1d5093b. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Probe connected sessions on mobile network changes and refresh activity leases on reconnect
network-path-changedadvisory wakeup that fires when the mobile app's known network interface changes while online and active, prompting a liveness probe of the current session with a mobile timeoutnetwork-path-changedwithout interrupting connection establishment or shortening retry backoff; a failed probe triggers an immediate reconnect through the existing wake-probe pathNetworkPathBaselinemodel in network-path-change.ts that records the first known interface asynchronously and requests probes only on genuine interface transitions, not repeated or unknown observationsConnectionWakeupgains a newnetwork-path-changedvariant; any out-of-tree consumer pattern-matching on that type without a catch-all will fail to compile📊 Macroscope summarized 1d5093b. 8 files reviewed, 1 issue evaluated, 1 issue filtered, 0 comments posted
🗂️ Filtered Issues
packages/client-runtime/src/connection/supervisor.ts — 0 comments posted, 1 evaluated, 1 filtered
network-path-changedto the probe branch also routes a failed path-change probe throughwakeProbeFailed. After the failed probe tears down the lease, the run loop resets the retry ladder and starts the next connection immediately, so a network-change failure skips the normal first backoff delay despitewaitForRetrySignalexplicitly treating these wakeups as advisory. This can cause reconnect churn on a flapping interface. [ Out of scope (triage) ]