Prevent smc and Unbounded from racing during startup - #9020
Open
atavism wants to merge 18 commits into
Open
Conversation
Co-authored-by: jay <110402935+jay-418@users.noreply.github.com>
The card links out to the web version of Unbounded, but it only ever rendered when Features[unbounded] is true — which is exactly when the app already has Unbounded built in, with its own "Unbounded Settings" row three lines above it. So it offered a download for something the user was already running, and never appeared in the one case where a pointer elsewhere might have helped: censored regions, where the server sets the flag false and the whole block was hidden anyway. Removed rather than inverted. Showing it when the flag is false would mean advertising Unbounded precisely where it has been switched off on purpose. unboundedAvailable still gates the settings sub-page link, so the variable stays.
# Conflicts: # .github/workflows/release.yml
The peer-status event bus is edge-triggered on the in-process path this build uses: it carries transitions with no snapshot on subscribe. The peer client resumes from persisted settings at process start, before any UI is listening, so the UI never learns sharing is already running and renders whatever it assumed at startup — the "Configuring network" card that sits there while Share My Connection is actually serving peers. Adds PeerStatusJSON() on the core and GetPeerStatus() on the mobile FFI so the UI can read current state instead of inferring it. Both return "" on failure rather than a synthesized idle, keeping "not sharing" and "could not ask" distinguishable. Bumps radiance for ipc.Client.PeerStatus (getlantern/radiance#617), which brings lantern-box v0.0.116. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCGZykEHZWRaF5WMraDUcr
The share UI is built purely on the peer-status event stream, which is edge-triggered: transitions only, no snapshot on subscribe. Peer sharing resumes from persisted settings at process start, well before the UI exists, so the UI opens at mode=off while SmC is already serving peers. Toggling on from that state is what produces the stuck card: the UI writes a setting radiance already has, PatchSettings' diff gate skips Start, no phase event ever fires, and the card sits on "Configuring network" indefinitely while sharing works fine underneath. Adds a getPeerStatus path through every layer that carries one — the c-archive FFI export (Windows/Linux), the method channel plus its macOS, iOS and Android handlers, and the service interface — then reconciles ShareState from it on home mount and starts the event subscription so later transitions land too. Only a phase meaning sharing is genuinely up is adopted. idle is the backend agreeing we are off; error belongs to the toggle path, which owns the Unbounded fallback. An unreadable status leaves state alone: "not sharing" and "could not ask" must not render identically. The _StatusCard idle mapping is deliberately unchanged. After a toggle, idle really is mid-flight, so remapping it would claim "off" while SmC serves — the inverse of the bug being fixed. lantern_generated_bindings.dart carries the single new binding by hand. A full `make ffigen` on this machine rewrote 8264 lines of unrelated symbols from local SDK headers, which is toolchain drift, not this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCGZykEHZWRaF5WMraDUcr
- macOS/iOS: run MobileGetPeerStatus off the main actor. It is an IPC round-trip bounded by a 5s Go-side timeout, invoked from a post-frame callback at first paint, so evaluating it inside MainActor.run could stall rendering. Mirrors the existing probeUPnP detachment. Android was already correct — its scope is Dispatchers.IO. - Drop the redundant error-envelope check in syncFromBackend. peer.Status tags Error omitempty, so the key is absent unless the phase is error, which the switch already returns on; the requireCore envelope carries no phase and is caught by that check alone. - Normalize the requireCore envelope to "" at the FFI boundary so the documented "empty when unreadable" contract holds there too. - Match the platform-service log message to the method name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCGZykEHZWRaF5WMraDUcr
Review asked for coverage of syncFromBackend's parsing and phase gating. Split the decision out as adoptablePhase so it is reachable without a WidgetRef, which a ProviderContainer cannot supply, and test it directly: every phase meaning sharing is up is adopted; empty, malformed, non-object and phase-less payloads are declined, as are idle, stopping, error, and an unrecognized future phase. One test pins that declining leaves ShareState untouched — the "not sharing" vs "could not ask" distinction. Verified by mutation: adopting idle fails two tests. Dropping the missing-phase guard fails none, because fromWire maps null to idle, which the switch already declines — the guard is defensive redundancy, not distinct behavior, and the comment now says so rather than implying coverage that cannot exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCGZykEHZWRaF5WMraDUcr
The guard sat before the await only. That read is an IPC round-trip bounded by a 5s timeout and runs at first paint, so the user has a wide window to hit the toggle while it is in flight. Adopting the snapshot after that would stamp mode=smc over a session that had just started as Unbounded, so a later toggle-off would call setPeerProxy(false) and leave Unbounded running. It would also install a second event subscription: _startEventSubscription assigns _appEventSub without cancelling an existing one, so the first leaks and every event is handled twice. Found by CodeRabbit on #9017. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCGZykEHZWRaF5WMraDUcr
Home can be disposed before the frame settles (a fast route change in the same frame), and reading a provider from a disposed scope throws. Adds the if (!context.mounted) return; guard this file already uses on its other post-frame callbacks, on both the new reconciliation effect and the adjacent auto-enable one, which had the same gap. Also moves the reconciliation effect above the "Auto-enable Unbounded" comment block. It had been inserted between that comment and the effect it describes, so the comment's "1. App launch (useEffect below)" pointed at the wrong useEffect. Found by Copilot on #9017. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCGZykEHZWRaF5WMraDUcr
atavism
requested review from
myleshorton
and
a lite review from Copilot
and removed request for
Copilot
August 27, 2026 19:35
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults 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:
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Serialize backend status reconciliation before auto-starting Unbounded so an already-running smc session is detected first, preventing both sharing modes from running simultaneously and keeping the UI synchronized with the actual backend state