fix(client-runtime): re-read network status when the app resumes#4528
fix(client-runtime): re-read network status when the app resumes#4528RestartFU wants to merge 6 commits into
Conversation
The supervisor read the live connectivity status once at construction and tracked it afterwards only through the connectivity change stream. Platform listeners drop transitions while an app is suspended, so a device that went offline and came back while backgrounded left `intent.network` stuck on "offline". The offline branch of the run loop only waits for a signal and re-checks the same stale value, so later wakeups could not recover and restarting the app was the only way back online. Re-read the live status on an "application-active" wakeup and reconcile the intent before forwarding the signal, so a resume recovers on its own. Co-Authored-By: Claude Opus 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:
✨ Finishing Touches🧪 Generate unit tests (beta)
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:unvouched
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review This PR introduces new network status recovery behavior with complex concurrency handling. There are unresolved review comments including a P1 identifying a potential race condition that could cause stale state - the exact problem this change aims to fix. You can customize Macroscope's approvability policy. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ec32da0df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review pointed out that reconciling only the supervisor's intent left `EnvironmentRegistry.networkStatus` stale, and that is the value the connection UI and readiness checks read. A dropped transition would reconnect the environment while the home status still reported "You are offline". Both consumers now share `followNetworkStatus`, which applies reported changes and re-reads the live status on resume. Status reads are asynchronous, so the resume path discards a read that a newer reported change superseded rather than applying an older snapshot over it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa6645ca7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review pointed out that the resume guard was not atomic: a connectivity change could arrive after `reportedCount` was compared but before the resumed status was applied, so the older read landed on top of the newer reported status and left the supervisor and UI on the wrong network status. Serialize both branches on a semaphore so counting and applying a reported change cannot interleave with the resume branch's guard and apply. The status read stays outside the permit, so a slow read still cannot block the listener. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07330bdf87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…covery Follow-up review raised three gaps in followNetworkStatus: Counting received events meant a listener repeating the status already in effect discarded a resumed read that carried genuinely newer information, leaving the consumer offline. Count applied status changes instead. Forking the listeners without `startImmediately` let a transition reported during setup slip through before the subscription existed — the same dropped transition this helper exists to repair. `RelayEnvironmentDiscovery` still followed `connectivity.changes` alone, so a transition dropped while suspended left `offline` set and the environment list stale until another event or a manual refresh. It now shares the same reconciliation; since only real changes are reported, a resume that reads the current status does not trigger a refresh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
size:L
Review asked for a generation guard so an older resume read cannot overwrite a newer one. Resume reads cannot overlap: `Stream.runForEach` consumes wakeups sequentially, so a second resume does not start a read until the first has applied. Pin that with a test rather than adding a guard for an unreachable interleaving. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5a0495158
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const changed = yield* Ref.modify(applied, (current) => | ||
| Option.isSome(current.status) && current.status.value === status | ||
| ? ([false, current] as const) | ||
| : ([true, { status: Option.some(status), changeCount: current.changeCount + 1 }] as const), |
There was a problem hiding this comment.
When the cached status is offline, a resume read can observe a transient online, after which the Expo listener can publish a newer offline report that equals the cached value. This branch treats that report as redundant, leaves changeCount unchanged, and allows the older online snapshot to pass the guard and overwrite it, leaving consumers incorrectly online until another event or resume. The later status-deduplication in this block is fresh evidence beyond the previously fixed race: track a separate generation for every received listener report while still deduplicating calls to options.apply.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9ed60fd, implemented exactly as suggested: a separate generation for every received listener report, with deduplication still applied to options.apply.
reportCount now increments on every report the listener delivers, repeats included, because a repeat still proves the listener spoke after an in-flight read began. appliedStatus separately tracks what last reached options.apply, so a repeat is counted for staleness without becoming a redundant consumer update.
Two tests cover the split: discards a resumed read that a repeated report raced (fails against the old code, applying the stale snapshot over the repeat) and still deduplicates a repeated report before it reaches consumers, which pins that counting repeats did not reintroduce duplicate applies. 480 tests pass, typecheck and lint clean.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 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 d5a0495. Configure here.
Counting only applied status changes let a repeated report pass unnoticed by the resume guard. With "online" in effect, a resume that sampled a brief "offline" could land after the listener repeated "online", stranding consumers on offline while the platform reported otherwise — the failure this helper exists to prevent, and the symmetric case leaves consumers wrongly online. Count every report the listener delivers, repeats included, since a repeat still proves the listener spoke after an in-flight read began. Deduplication now tracks only what reached `options.apply`, so repeats stay out of consumer updates without weakening the staleness guard. This reverses the invariant pinned by the earlier resume-read test: a repeated report now discards a racing read instead of yielding to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ed60fd0ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Tracks what was last handed to `options.apply` purely to keep repeats from | ||
| // reaching consumers. Deduplication is deliberately kept separate from the | ||
| // staleness guard above. | ||
| const appliedStatus = yield* Ref.make<Option.Option<NetworkStatus>>(Option.none()); |
There was a problem hiding this comment.
When relay discovery has already refreshed but no connectivity event has fired since startup, appliedStatus remains None; both platform changes streams only install event listeners and do not emit the initial status. The first application-active read is therefore always treated as a change even when it returns the same "online" state, causing RelayEnvironmentDiscovery to invoke refresh, clear its environment map at discovery.ts:218-223, and repeat all relay requests merely because the app was foregrounded. Initialize the deduplication state from the current status, or let the discovery consumer compare against its existing state, so an unchanged first resume does not trigger this user-visible refresh.
Useful? React with 👍 / 👎.
| Effect.forkScoped({ startImmediately: true }), | ||
| ); | ||
|
|
||
| yield* options.wakeups.changes.pipe( |
There was a problem hiding this comment.
When a user has multiple persisted environments, every EnvironmentSupervisor.make calls this helper, in addition to the registry and relay discovery. Because the platform wakeup services are cold Stream.callback streams, each call installs another AppState/visibility listener and independently runs getNetworkStateAsync() on every resume; registry.start eagerly creates a supervisor for every persisted target, so N environments produce N+2 native status reads and N+2 newly added wakeup subscriptions each time the runtime starts. Centralize the resume-aware status stream or share its result so foregrounding does not fan out native bridge work with the number of saved environments.
Useful? React with 👍 / 👎.
| applyLock.withPermits(1)( | ||
| Ref.update(reportCount, (count) => count + 1).pipe(Effect.andThen(applyStatus(status))), |
There was a problem hiding this comment.
When an offline connectivity notification was buffered around suspension but is delivered after application-active, this increments reportCount only when runForEach consumes that notification, so the buffered payload is incorrectly considered newer than the fresh resume read. For example, the device can report offline before suspension, return online while the online transition is dropped, and then deliver the buffered offline payload after the resume query has read online; that payload either invalidates the online result or applies offline afterward, leaving all consumers stuck on the stale state this change is meant to repair. The fresh evidence beyond the previously fixed in-flight-read race is that the generation is advanced at stream consumption rather than at the platform observation source; carry source ordering/version information or otherwise prevent pre-resume reports from superseding the resume snapshot.
Useful? React with 👍 / 👎.
1 participant
Problem
Leaving the mobile app backgrounded for a while and returning to it shows "You are offline", and the only way back online is to restart the app.
Cause
EnvironmentSupervisorreads the live connectivity status exactly once, when it is constructed, and from then on tracks it only through the connectivity change stream:Platform listeners drop transitions while an app is suspended, so a device that went offline and came back while backgrounded leaves
intent.networkstuck on"offline". The offline branch of the run loop only waits for a signal and then re-checks that same stale value:Every subsequent wakeup re-enters this branch, so the supervisor never attempts a connection. Restarting the app is the only recovery because construction is the one place the live status is read.
"You are offline"is driven by this value viaworkspaceConnectionStatusLabel.The rest of the retry machinery already handles its own cases: backoff retries forever capped at 16s, and the foreground liveness probe catches sockets that died silently. Stale network status is the one path that needs a restart, which is why this fix is small.
Fix
Re-read the live status on an
application-activewakeup and reconcile the intent before forwarding the signal. Network reconciliation is factored into a sharedapplyNetworkStatusused by both the change stream and the resume path, so a stale value corrects itself and the run loop leaves the offline branch on its own.Testing
recovers from a network change the platform dropped while suspended, which sets the live status without emitting a change event and asserts the supervisor reconnects on resume. It times out against the old code and passes with the fix.statusandchangeswith separate refs so a dropped listener event can be simulated;setNetworkStatusstill updates both.packages/client-runtime: 472 tests pass, typecheck and lint clean.No UI change, so no before/after images. This was diagnosed from the supervisor's state machine and covered by the regression test rather than reproduced on hardware, so the real-world timing of a suspended platform listener is not verified on a device.
The fix is in shared client-runtime, so desktop and web (which resume on
visibilitychange) get the same self-healing behavior.Note
Medium Risk
Touches core connection/registry/supervisor/discovery networking paths with async resume reads and concurrency guards; behavior change on every foreground resume but scoped to connectivity reconciliation.
Overview
Fixes mobile (and other platforms) staying stuck on "You are offline" after backgrounding when the OS connectivity listener never delivered a transition that happened while suspended.
Adds
Connectivity.followNetworkStatus, which still applies platformchangesbut also re-reads live status onapplication-activewakeups. Resume snapshots are dropped if a newer listener report arrived while the read was in flight (via a report counter and semaphore), and duplicate statuses are not pushed to consumers again.EnvironmentRegistry,EnvironmentSupervisor, and relay discovery now use this helper instead of wiringconnectivity.changesdirectly, so registry UI/readiness, connection supervisors, and relay environment lists can recover without an app restart.Regression coverage includes dedicated
followNetworkStatustests (staleness, dedup, ordering) and integration tests that simulate a missed transition by separating live status from the change stream.Reviewed by Cursor Bugbot for commit 9ed60fd. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Re-read network status on app resume to recover transitions missed while suspended
Connectivity.followNetworkStatusinconnectivity.ts, which subscribes to both connectivity change events and wakeup events, re-reads live status on resume, and discards stale reads that are superseded by newer reports.EnvironmentRegistry,EnvironmentSupervisor, andRelayEnvironmentDiscoveryto usefollowNetworkStatusinstead of subscribing directly toconnectivity.changes, so all three recover from network transitions that occurred while the app was suspended.Macroscope summarized 9ed60fd.