Share My Connection: unbounded integration (collapses #501, #512) - #590
Open
myleshorton wants to merge 15 commits into
Open
Share My Connection: unbounded integration (collapses #501, #512)#590myleshorton wants to merge 15 commits into
myleshorton wants to merge 15 commits into
Conversation
Adds the radiance side of the Unbounded ("Basic mode" in the SmC UI)
WebRTC donor-mode integration. Self-contained package under
radiance/unbounded; drops cleanly onto the current branch without
needing the larger vpn/ refactor that adam/unbounded-widget-proxy
ships with.
unbounded.SetEnabled(bool) toggles the local opt-in
(settings.UnboundedKey). InitSubscription wires the manager to
config.NewConfigEvent — the broflake widget actually runs only when:
1. settings.UnboundedKey is true (local opt-in)
2. server cfg.Features[UNBOUNDED] is on
3. server provides cfg.Unbounded discovery + egress URLs
Each consumer connection change emits unbounded.ConnectionEvent on
the radiance event bus, mirroring the shape of peer.ConnectionEvent
so lantern-core subscribers can feed both into one Flutter event
stream.
Wired into LocalBackend.Start so the manager is live for the process
lifetime; sync.Once-guarded against double-subscribe.
Mostly a port of the unbounded.go file from adam/unbounded-widget-proxy
(#336), with the package moved out of vpn/ since
that branch's vpn/ is undergoing a separate refactor and we want the
unbounded code to land independently.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four substantive findings + one stale comment (settings.go vpn.Init…
reference) answered via reply.
1. PatchSettings now dispatches settings.UnboundedKey to
unbounded.SetEnabled the same way it dispatches PeerShareEnabledKey
to applyPeerShare. The UI toggle now starts/stops the widget proxy
immediately rather than persisting the value and waiting for the
next NewConfigEvent.
2. Close now invokes unbounded.Stop(...) before r.cancel(). unbounded
workers run on a context.Background-derived ctx (they must outlive
any single NewConfigEvent), so without an explicit shutdown hook
the broflake widget goroutine survived backend close. Uses a fresh
5s-bounded ctx so a cancelled shutdown path doesn't skip the Stop.
3. SetEnabled now respects the full three-condition predicate before
starting. Previously it called manager.start whenever cached
*UnboundedConfig was non-nil, ignoring the cached
Features[UNBOUNDED] flag — meaning a local toggle could start the
proxy even when the server had said don't. Added lastFeatureOn to
unboundedManager (set alongside lastCfg on every NewConfigEvent)
and a shouldStart() method that re-checks all three conditions.
SetEnabled now uses shouldStart(); the standalone
shouldRunUnbounded function is removed (its job is now done by the
method).
4. unbounded.ConnectionEvent shape aligned with peer.ConnectionEvent:
{State, Source, Timestamp}. WorkerIdx is dropped from the wire
shape (it remains in the per-callback Debug log for diagnostics
but isn't part of the event contract). Doc comment updated to
spell out the field semantics. Consumers that need to pair
accept/close events for the same arc now key off Source (or
arrival sequence within a single connection's lifetime, which is
what the globe currently does).
The fifth Copilot comment cited a 'vpn.InitUnboundedSubscription'
reference in common/settings/settings.go:75 that no longer exists —
the consolidation cascade rewrote that doc to say 'the widget proxy
starts' without naming the lifecycle function. Answered in the thread
rather than re-edited.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Four substantive findings on the cross-process / lifecycle surface: 1. PatchSettings's call to SetEnabled was a no-op for UI toggles because settings.Patch above already persisted UnboundedKey, and SetEnabled's early-return (`Enabled() == enable`) then short- circuited before evaluating the manager. Split SetEnabled into two paths: SetEnabled (persist if changed + Apply) for direct callers, and Apply() for callers that have already persisted. PatchSettings now calls Apply() directly. Apply also has a guard so a second 'enable=true' call when already running is a no-op (the manager.start path is also guarded but the manager-running check is cheaper). 2. unbounded.ConnectionEvent had no IPC bridge — events.Subscribe is process-local, so cross-process consumers (Flutter via Liblantern) could not see them despite the JSON shape matching peer.ConnectionEvent. Added /unbounded/connection/events SSE endpoint mirroring /peer/connection/events. Two separate streams client-side because events.Emit dispatches by concrete Go type; the Flutter side multiplexes them into one globe feed. 3. unbounded.Stop ignored its ctx and returned as soon as the cancel signal was queued, so LocalBackend.Close could proceed while the broflake goroutine was still inside NewBroflake or ui.Stop. Added a 'done' channel that the worker closes on exit; Stop now selects on done OR ctx.Done(). The 5s timeout LocalBackend.Close passes now actually bounds the wait. 4. ConnectionEvent doc claimed 'single subscriber can handle both streams' — true at the JSON level but not at the Go-type level (events.Subscribe is type-keyed; Subscribe[peer.ConnectionEvent] does not receive unbounded.ConnectionEvent). Reworded to be honest about the in-process distinction and to point at the IPC bridge as the fan-in point for cross-process consumers. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
unbounded/unbounded.go:
- Stop/Start race fix. Round-2's stop() cleared m.cancel
immediately after signalling cancel, while the worker goroutine
continued running through ui.Stop. During that window a
concurrent Apply() or config event could see m.cancel == nil and
spin up a second broflake widget while the first was still
tearing down. Stop() likewise only waited on the done channel it
captured BEFORE the restart.
Fix: serialize start/stop transitions via a separate
transitionMu. start() holds it for the brief window of setting up
cancel/done. stop() holds it for the full cancel-then-wait-on-done
cycle. Public Stop(ctx) does the same with a context-bounded wait.
m.mu now only protects field reads/writes; never held across the
done wait or any broflake call. Lock order is transitionMu →
mu, consistent across all callers.
- Drop ipc/server.go reference from the ConnectionEvent doc;
rephrase to describe the in-process event-bus contract directly
without naming the bridge's source location.
ipc/client_events_{nonmobile,mobile}.go:
- Add UnboundedConnectionEvents method mirroring
PeerConnectionEvents. The server SSE route landed without a
corresponding client method, leaving Liblantern / Flutter
callers to hard-code the private path. Mobile path follows the
existing localOnly+SSE dual-pattern.
go.mod:
- go mod tidy promoted broflake from // indirect to direct since
unbounded/unbounded.go now imports clientcore directly.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two Copilot findings on round 3:
1) Cold-start race: InitSubscription only reacted to future
NewConfigEvents, but ConfigHandler typically has a cached
config loaded from disk by the time Start runs. An already-
opted-in user wouldn't auto-start the widget on launch — the
three-condition predicate stayed stuck at lastCfg=nil/
lastFeatureOn=false until the next config refresh arrived.
Fix: InitSubscription now takes an optional initial *config.Config.
If non-nil, the same applyConfig handler runs immediately on
subscribe so the cached config seeds lastCfg/lastFeatureOn and
the start transition fires synchronously. backend/radiance.go
passes confHandler.GetConfig() (ignoring the not-yet-fetched
error) so the seed is best-effort.
2) No tests for the Unbounded lifecycle. The manager coordinates
persisted settings, server config gates, and start/stop
concurrency — exactly the kind of code that regresses
silently. Added unbounded_test.go covering:
- shouldStart predicate, all 6 (toggle, feature, cfg) combos
- Apply: disabled is no-op; with all conditions, exactly one
widget starts; double-Apply is idempotent
- Stop_WaitsForWorker: stop blocks until worker's ui.Stop
returns (the round-2 fix's invariant)
- StartDuringStop_NoOverlap: the round-2 regression guard —
a concurrent start while a stop is in flight must NOT spin
up a second widget; transitionMu serializes them
- InitSubscription_SeedsCachedConfig: passing initial cfg
kicks off applyConfig immediately
- InitSubscription_FutureEventStillFires: live NewConfigEvent
handler still works when initial cfg is nil
- StopCtx_TimesOut: Stop(ctx) returns DeadlineExceeded when
the worker can't unwind in time
To make this testable without spinning up real WebRTC,
extracted a small widget interface (Stop()) and an injectable
newWidget factory. Default still calls clientcore.NewBroflake;
tests swap in a fake that records start/stop calls under their
own gates.
TestMain initializes settings once for the binary — calling
InitSettings per t.TempDir() runs into its sync.Once guard,
leaving the package pointing at a since-cleaned-up temp dir.
resetManager's cleanup waits on the worker's done channel
before swapping newWidget back, so the race detector doesn't
flag the worker's earlier read of newWidget against the
cleanup's later write.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
unbounded/unbounded.go:
- Round-3 fix serialized start/stop via transitionMu, but a
config event firing during public Stop's <-done wait could
still queue at transitionMu and start a fresh widget the
moment Stop's caller returned — silently keeping broflake
alive past the documented backend shutdown contract.
Added an explicit armed gate on the manager:
- InitSubscription sets armed=true (every call, not just the
first — the underlying subscription is still sync.Once-
guarded, but Start-after-Close needs to re-arm).
- Public Stop sets armed=false inside transitionMu before
waiting on done.
- start, Apply, applyConfig all bail when !armed.
- start re-checks armed AFTER acquiring transitionMu, so
even a transition queued at transitionMu during Stop's
wait stays a no-op.
Net effect: once Stop returns, the manager is permanently
off until the next InitSubscription. No further start path
(Apply, applyConfig, manager.start) can revive the widget.
ipc/client_events_mobile.go:
- Per AGENTS.md, comments must not reference code locations.
Inlined the full docstrings for PeerStatusEvents,
PeerConnectionEvents, and UnboundedConnectionEvents
instead of pointing at client_events_nonmobile.go.
unbounded/unbounded_test.go:
- TestMain: os.Exit bypasses deferred calls, so the tmp dir
was leaked on every test invocation. Capture m.Run's exit
code, RemoveAll, then exit with that code.
- resetManager: initialize manager with armed=true so tests
that call manager.start directly aren't blocked by the new
gate. Tests exercising Stop's disarm path flip it
explicitly.
- Added TestStopDisarmsManager: after public Stop, Apply +
applyConfig + manager.start are all no-ops; a fresh
InitSubscription re-arms.
- Added TestStartAfterStopWaiting_NoRevival: pin Stop in its
<-done wait, queue a start at transitionMu, confirm the
queued start sees armed=false after Stop releases and bails
without creating a second widget.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
unbounded/unbounded.go:
- broflake consumes its discovery / egress endpoints and table-size
options once in clientcore.NewBroflake. Round-5's applyConfig
only handled start/stop transitions: when the widget was already
running and a server config refresh changed the UnboundedConfig
parameters, the running proxy stayed on stale settings until a
manual toggle or process restart.
Manager now tracks runningCfg — the snapshot of UnboundedConfig
the live worker was started with. applyConfig compares it
against the freshly-cached lastCfg via cfgEqual (value equality
on the flat string/int struct) and adds a new branch:
shouldRun && running && cfgChanged → stop + start
stop() blocks until the prior worker fully exits (transitionMu
+ <-done), so the restart-start always sees a clean slate.
runningCfg is set in start() and cleared by the worker on both
the success exit and the newWidget-error early-return paths.
unbounded/unbounded_test.go:
- Rewrote TestStartDuringStop_NoOverlap and TestStopDisarmsManager
doc comments to state the invariant directly, dropping the
'round-2 / round-4 regression guard' framing. The test names
+ bodies are the durable record; the review-history framing
was noise.
- Added TestApplyConfig_RestartsOnParamChange covering the new
restart branch:
1. apply cfg{DiscoverySrv: a} → widget #1 starts
2. apply same cfg → no restart (starts stays 1)
3. apply cfg{DiscoverySrv: b} → widget #2 starts (starts == 2)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
unbounded/unbounded.go:
- start() now re-checks the full enablement predicate
(shouldStart()) under m.mu, not just the armed gate. Without
this, a caller could decide to start based on its earlier
snapshot, get queued behind a stop or another transition at
transitionMu, and then create a worker even though a
concurrent SetEnabled(false) or config update had since
disabled Unbounded. The widget would run with stale state
until some later event stopped it.
- start() also drops its ucfg parameter and reads m.lastCfg
directly under the lock. applyConfig sets m.lastCfg before
calling start, so the normal path is unchanged; for the race
case (cfg updated between the caller's read and start's lock
acquisition), the worker comes up with the latest parameters
rather than a stale snapshot.
unbounded/unbounded_test.go:
- New primeManager(t, cfg) helper seeds the predicate fields
(toggle on, lastFeatureOn=true, lastCfg=cfg) so the four
tests that drive manager.start directly satisfy shouldStart()
inside the new gate.
- Added TestConnectionEventBridge: captures the
OnConnectionChangeFunc that start installs on bfOpt via the
fake newWidget, invokes it with both a non-nil and a nil
net.IP, and asserts the resulting events.ConnectionEvent
payloads via events.Subscribe — pins {state, source,
timestamp} including the addr.String() / empty conversion
for nil. Set-membership keyed by State because events.Emit
dispatches each subscriber on a per-callback goroutine, so
arrival order is not deterministic.
Stress-tested 20x under -race -count=1, clean every run.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
manager.stop() (called from Apply and applyConfig) waited on the worker's done channel with no deadline. If broflake's ui.Stop hung, toggling Unbounded off via PatchSettings or receiving a disabling config event would block the caller indefinitely while holding transitionMu. Only the public Stop(ctx) was bounded. Extracted stopCtx(ctx, disarm) as the shared implementation for both paths: - Public Stop(ctx) calls stopCtx(ctx, true) — disarm + caller's own deadline (5s from LocalBackend.Close). - Internal manager.stop() calls stopCtx with a fresh context bounded by internalStopTimeout (default 5s). The timeout error is logged; callers have no useful action to take, and a subsequent start observes 'already running' until the worker eventually exits. internalStopTimeout is a var (not const) so tests can install a short timeout (50ms) and exercise the timeout path without holding up the suite. TestInternalStop_TimesOut: pins ui.Stop past the short timeout via stopBlock, flips the toggle off so Apply calls manager.stop, asserts Apply returns within 2s rather than blocking forever. Stress-tested 20x under -race -count=1, all clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
A typo on the diff key (settings.UnboundedKey →
settings.UnboundedKye) or a removal of the unbounded.Apply()
call would silently leave the UI toggle persisted but inert.
The peer-share side has TestPatchSettings_PeerShareDispatches
catching exactly this class of regression; mirror it for
Unbounded.
unbounded/unbounded.go:
- Add applyHook: a nil-in-production function pointer invoked
at the top of Apply(), before any state check. Backend tests
install a counter via SetApplyHookForTest to verify dispatch.
This is the smallest possible test surface — exposing the
manager's internals across packages just for the test wiring
would be a much bigger surface, and the hook fires regardless
of the Enabled() gate so tests catch dispatch even when no
transition results.
backend/radiance_test.go:
- TestPatchSettings_UnboundedDispatches: install the hook,
PATCH {UnboundedKey:true}, assert hook fired once; PATCH
{UnboundedKey:false}, assert hook fired twice. Then PATCH
{PeerShareEnabledKey:false} (a key OTHER than UnboundedKey)
and assert the counter doesn't move — confirms the diff
check is in place rather than always firing.
Sanity-checked the test by removing the Apply call from
PatchSettings: test fails with 'expected 2, actual 0'.
Restored: test passes.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two Copilot findings:
1. shouldStart treated any non-nil UnboundedConfig as runnable. A
config refresh carrying an empty or partially-populated cfg
block would start broflake with clientcore defaults — pointing
at the upstream maintainer's discovery/egress infra rather than
the per-environment URLs the server sends. That bypasses the
server-supplied endpoint gate and the 'is this user opted in?'
contract.
Added cfgUsable(cfg) requiring all four URL fields
(DiscoverySrv + DiscoveryEndpoint + EgressAddr + EgressEndpoint)
non-empty. CTableSize / PTableSize stay optional — the server
only sends them when it wants to override defaults. shouldStart
now gates on cfgUsable rather than 'cfg != nil'.
Tests primeManager and the rest of the suite now use a new
testCfg() helper that returns a populated, predicate-passing
cfg. Bare &C.UnboundedConfig{} literals were dropped (they'd
now fail the runnable check). New TestShouldStart row covers
the partial-cfg case.
2. OnConnectionChangeFunc kept emitting after stop signalled
cancel — broflake's per-worker connection-change goroutines can
fire concurrently with ui.Stop, and the broflake API doesn't
promise no-callbacks-after-Stop. Callbacks that landed in this
window pushed stale ConnectionEvents onto the event bus after
the consumer thought Unbounded was off.
Added a ctx.Err() check at the top of the callback closure.
ctx is the same one cancelled by stop, so post-cancel
callbacks short-circuit before reaching events.Emit. Mirrors
peer.go's listenerDraining pattern; broflake doesn't expose an
equivalent registration point, so the inline check is the
next-best place.
Stress-tested 10x under -race -count=1, all clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three small comment cleanups per AGENTS.md (no code-location references in comments): - unbounded.go: drop 'peer.go listenerDraining' phrasing from the cancellation-drain comment; describe the broflake-side constraint directly (no registration point to disarm, so the inline ctx check is the next-best place). - backend/radiance_test.go: drop 'Companion to TestPatchSettings_ PeerShareDispatches' framing from TestPatchSettings_ UnboundedDispatches docstring; state the invariant directly. - unbounded/test_helpers_test.go: drop 'Used by TestStartDuringStop' framing from countingWidget docstring; describe the helper's role (callback lets test observe shutdown ordering) without naming a consumer. No behavior changes; tests unchanged + still clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
broflake's OnConnectionChange delivers the consumer addr on accept but a nil addr on close (the WebRTC session is already torn down, so the remote IP is gone). The close ConnectionEvent therefore carried an empty Source, and downstream consumers couldn't pair it with its accept: the Flutter globe matches the arc to remove by source IP, and the "people helped" counter decrements the same way. So closes were dropped — arcs orphaned and accumulated, and the live counter only ever grew (it equalled the lifetime total). Track each consumer slot's addr on accept (connSources, keyed by broflake's workerIdx — stable across a connection's accept->close) and restore it on close, so every -1 carries the same Source its +1 did. No event-shape change; downstream consumers already key off Source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Stacked on smc/peer-core. Brings the WebRTC/broflake donor path in as a parallel peer source, plus #512's fix keeping the consumer Source on close events. Conflict resolutions: - broflake was already an indirect dependency on main at a newer version (20260717) than #501 pinned directly (20260504). Kept main's version and let go mod tidy promote it to direct rather than downgrading. - settings: UnboundedKey added alongside the keys main and the peer-core branch introduced. - unbounded.InitSubscription is seeded before the country-code SubscribeOnce, preserving main's rewritten comment on the latter.
Contributor
|
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:
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Integrates the Unbounded (broflake widget-proxy) donor mode into radiance, including lifecycle management tied to config + settings, connection-event emission, and IPC/SSE plumbing so downstream consumers can render/aggregate Unbounded connections alongside Share My Connection.
Changes:
- Added a new
unbounded/package that manages the widget-proxy start/stop lifecycle and emitsunbounded.ConnectionEvent(with close-event source backfill). - Exposed Unbounded connection events over local IPC via a new SSE endpoint and client APIs (mobile + nonmobile).
- Wired Unbounded into backend startup/shutdown and settings patch flow, and promoted
broflaketo a direct module dependency.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| unbounded/unbounded.go | New lifecycle manager for broflake widget proxy; emits unbounded.ConnectionEvent and handles config/toggle-driven transitions safely. |
| unbounded/unbounded_test.go | Adds unit tests covering lifecycle transitions, restart semantics on config changes, shutdown/disarm behavior, and SSE event bridging expectations. |
| unbounded/test_helpers_test.go | Adds small test helper widget type for shutdown-order assertions. |
| unbounded/conn_sources_test.go | Adds focused tests for accept→close source backfill logic. |
| ipc/server.go | Adds /unbounded/connection/events SSE endpoint bridging unbounded.ConnectionEvent to IPC clients. |
| ipc/client_events_nonmobile.go | Adds Client.UnboundedConnectionEvents SSE consumer API on nonmobile builds. |
| ipc/client_events_mobile.go | Adds Client.UnboundedConnectionEvents with the same localOnly/SSE dual-path as other mobile event streams. |
| go.mod | Promotes github.com/getlantern/broflake from indirect to direct dependency. |
| common/settings/settings.go | Adds settings.UnboundedKey and documents the three-condition start predicate. |
| backend/radiance.go | Wires Unbounded subscription initialization at startup, explicit shutdown on Close(), and immediate Apply on PatchSettings toggles. |
| backend/radiance_test.go | Adds a regression test ensuring PatchSettings dispatches Unbounded toggle changes into unbounded.Apply(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced Aug 5, 2026
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
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.
Collapses the Unbounded / broflake donor path onto the peer-core integration branch.
Supersedes #501 and #512, which can be closed once this merges.
Stacked — merge order matters
This targets
smc/peer-core(radiance#589), notmain. Merge #589 first;GitHub will retarget this to
mainautomatically.Full chain, bottom-up:
v0.0.108v0.0.108firstConflict resolutions worth reviewing
broflakedependencymainalready carried it as an indirect atv0.0.0-20260717153233— newer than the20260504version #501 pinned as a direct require. Kept main's newer version and letgo mod tidypromote it to direct, rather than downgrading a dependency that main had already moved forward.settings.UnboundedKeymainand the peer-core branch introduced; all three coexist.unbounded.InitSubscriptionSubscribeOnce, preservingmain's rewritten comment on the latter (the branch still had the older one-line version).backend/radiance_test.goimportsservers,unbounded, andvpnall needed.Verification
go build -tags "with_clash_api standalone" ./...— clean.go test -tags "with_clash_api standalone" ./...— all 21 packages pass,including the new
unboundedpackage.gofmtclean across all changed files.Note on #336
radiance#336 (
adam/unbounded-widget-proxy) is superseded by this work: it putthe widget proxy in
vpn/unbounded.go, whereas #501 moved it to its ownunbounded/package. #336 isCONFLICTING/DIRTYagainst main and is beingclosed separately.