Share My Connection: peer-core integration (collapses #458, #460, #466, #472, #484, #499, #503)

[myleshorton]

Collapses the Share My Connection peer-core stack onto current main.

Supersedes #458 (1/4), #460 (2/4), #466, #472, #484, #499, #503 — all now closed.

The originals were cut ~2 months ago and sat 81 commits behind main. Each reported
MERGEABLE/CLEAN only because GitHub measures a PR against its own parent in the stack,
not against main — the stack tip actually conflicted in 6 files. Per the agreed plan the
stack is collapsed into one integration PR rather than 8 sequential rebase-and-merge cycles.

Dependency

go.mod pins github.com/getlantern/lantern-box v0.0.109 — the released tag, no longer a
staging pseudo-version. lantern-box #255 and #256 have merged; that repo auto-tags per merge,
so #255 produced v0.0.108 and #256 produced v0.0.109. v0.0.109 is required, not
v0.0.108: peerconn.Event arrives with #256, and peer/peer.go calls
SetListener(func(evt peerconn.Event) {...}).

Conflict resolutions worth reviewing

Site	Resolution
events.Emit	Combined, not picked. main added panic recovery around callbacks; the peer branch replaced whole-loop RLock with snapshot-then-release, fixing a real concurrent map iteration and map write race against Unsubscribe. Taking either side alone silently drops a fix and still compiles. Now: snapshot under RLock → release → dispatch each callback in a goroutine with recovery.
LocalBackend.stopChan	Gone on main; Close now drains via shutdownFuncs + closePeerClient's peerWG.Wait(). The peer shutdown test helper drops the field instead of reintroducing it.
peerconn.SetListener	Signature is func(peerconn.Event) as of lantern-box #256. Call site adapted.
Client.Stop state reset	Clears #472's rotation fields while keeping #503's phase-aware PhaseStopping status — the emitted stoppingSnapshot depends on it.
backend/radiance_test.go	Union of both sides' tests (5 from main + 8 peer tests); git could not align the files so the whole body conflicted. All 13 present and passing.
#484 × #472 interaction	#484's validator rejects any launch_cfg without a route block, which #472's rotation test did not supply. It now uses the shared minimalValidLaunchCfg fixture. Only visible once both siblings were in one tree.

Review fixes (Copilot + CodeRabbit, 3 rounds — 681f5eb, a4913ef)

Two of these are open-port leaks worth reading closely.

• portforward: mappings could outlive the Forwarder. runWithCtx returns the moment its
  ctx is cancelled but its goroutine keeps running, so an AddPortMapping could reach the
  gateway after we'd given up — leaving an inbound forward to the user's host that nothing
  removes (permanent on routers that ignore the requested lease). This affected both
  renewLoop (racing UnmapPort) and MapPort (racing caller cancellation; worse there,
  since f.mapping was never recorded so UnmapPort short-circuited). Both now wait for the
  call's outcome, bounded by a timeout treated as "may have landed", and delete the mapping if
  teardown or cancellation happened meanwhile. UnmapPort marks teardown under the lock so a
  blocked renewal is ordered strictly after it.
• peer: rotation bypassed the abuse-rule gate. validateAbuseRules ran only in Start,
  so a server-side regression would have reached every long-lived peer on its next hourly
  rotation — precisely what the gate exists to prevent. Rotation now re-validates; a rejected
  config deregisters the orphan route and leaves the current, already validated box serving.
• events: emitDebugLogger data race. A plain global read by Emit from arbitrary
  goroutines and written by SetEmitDebugLogger. Now an atomic.Pointer. The package had no
  test file; the new one reproduces the race and fails under -race without the fix
  (WARNING: DATA RACE).
• backend: construction no longer fatal on peer-client failure. Restores the invariant
  documented on NewLocalBackend — only common.Init is fatal, so a user can always come up
  and report an issue. PeerStatus and applyPeerShare guard nil, and applyPeerShare rolls
  PeerShareEnabledKey back so a persisted "on" can't outlive an unavailable client.
• backend: PatchSettings applies both handlers and joins their errors. settings.Patch
  has already persisted the whole diff by then, so an early return on a VPN-restart failure
  left PeerShareEnabledKey persisted but unapplied.
• peer: isUnconditionalReject tolerates "type":"default" on inlined rules.
  launch_cfg is authored server-side rather than round-tripped through sing-box's marshaller
  (which omits type for default rules), so the discriminator can legitimately appear and was
  making the peer refuse a config that does reject unconditionally. Non-default types are still
  rejected — they carry their own matching fields.
• MapPort also returns early on an already-cancelled ctx instead of enumerating interfaces first.

Every fix above has a test that was verified to fail with that fix reverted. Two added tests
pass either way and are labelled as guards rather than proofs: TestForwarder_RenewalAfterTeardown_DoesNotReAdd
and TestValidateAbuseRules_RejectsNonDefaultRuleType.

Not included

• peer: route the peer-box sing-box logger into radiance's slog #481 (peer box logger) is deliberately out, and now landed separately as peer: route the peer box's sing-box logger into radiance's slog (replaces #481) #591. A direct
  port would have been a silent no-op: boxRegistryCtx.Value falls through to
  box.BaseContext(), which builds a fresh registry per call, so a service.MustRegister
  through it writes into a registry discarded before libbox reads it.
• Event.Destination from lantern-box Adding kindling issue request integration test #256 has no consumer yet — staged for the abuse aggregator.

Verification

• go build -tags "with_clash_api standalone" ./... — clean
• go test -tags "with_clash_api standalone" ./... — all 21 packages pass
• go test -race on events, portforward, backend, peer — clean
• gofmt clean across all changed files

standalone is needed locally on darwin: ipc/client_nonmobile.go is built for
(!android && !ios && !darwin) || (darwin && standalone), so without it darwin selects the
mobile NewClient and cmd/lantern fails to compile. CI runs on ubuntu and takes the
nonmobile path.

Known pre-existing issue (not introduced here)

go vet ./peer/... reports a lostcancel finding on cancelRun in peer/peer.go. It
reproduces on the unmodified fisk/peer-manual-portforward branch, so it predates this
integration, and it is not CI-gating (lostcancel is outside go test's default vet subset).
Left as-is rather than widening scope.

Residual, disclosed

A UPnP call that exceeds its timeout has an unknowable outcome. We assume it landed and delete,
so the leak direction is closed, but a gateway that both hangs past the timeout and rejects
the delete would still keep the forward. Fully removing that needs cancellable UPnP calls,
which goupnp's blocking SOAP API doesn't offer — a change to runWithCtx's contract affecting
every call site, not this PR.

Summary by CodeRabbit

• New Features
  • Added peer sharing with lifecycle management, connection status, and live activity updates.
  • Added mobile and desktop event streams for peer status and connection changes.
  • Added automatic UPnP port forwarding with support for manually configured ports.
  • Added settings for peer sharing, manual ports, smart routing, split tunneling, ad blocking, and auto-connect.
• Bug Fixes
  • Improved startup rollback, shutdown cleanup, timeout handling, and resilience to unavailable networking components.
  • Added validation to prevent unsafe or incomplete peer routing configurations.
• Reliability
  • Improved event delivery safety and diagnostic logging.

[coderabbitai]

📝 Walkthrough

Walkthrough

Peer sharing now supports UPnP and manual port forwarding, remote registration, sing-box lifecycle management, validation, heartbeat recovery, credential rotation, backend settings integration, status reporting, and IPC event streams.

Changes

Peer sharing

Layer / File(s)	Summary
Port forwarding and configuration common/env/env.go, common/settings/settings.go, portforward/*, go.mod	Adds UPnP forwarding, manual port forwarding, renewal, external-IP lookup, configuration keys, and dependency updates.
Peer API and lifecycle peer/*	Adds registration APIs, startup and shutdown sequencing, abuse-rule validation, heartbeats, credential rotation, VPN bypass handling, and lifecycle tests.
Backend peer control backend/peer_share.go, backend/radiance.go, backend/radiance_test.go	LocalBackend creates and owns the peer client, resumes persisted sharing, applies setting changes, exposes status, and coordinates shutdown.
IPC status and connection streams ipc/*, events/*	Adds peer status and connection streams, mobile and non-mobile clients, event snapshotting, panic recovery, and diagnostic logging.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant LocalBackend
  participant peer.Client
  participant portforward.Forwarder
  participant peer.API
  participant BoxService
  LocalBackend->>peer.Client: Start(ctx)
  peer.Client->>portforward.Forwarder: MapPort()
  peer.Client->>peer.API: Register()
  peer.Client->>peer.API: Verify()
  peer.Client->>BoxService: Start()
  peer.Client->>peer.API: Heartbeat()
  peer.Client->>peer.Client: Rotate credentials or Stop()

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 58.97% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change as integrating the peer-core stack for connection sharing.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch smc/peer-core

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

Integrates the “Share My Connection” (peer-proxy) stack onto main by adding UPnP/manual port-forwarding, peer session lifecycle (register/verify/heartbeat/cred rotation), abuse-rule validation of server-provided launch_cfg, and IPC/SSE plumbing to expose peer status + connection events to consumers.

Changes:

• Add portforward package (UPnP IGD v1/v2 + manual override) with tests, and wire it into the peer client’s Start/Stop lifecycle.
• Add peer safety + lifecycle features: /peer/verify, abuse-rule validation gate for launch_cfg, credential rotation, and connection-event forwarding via lantern-box peerconn.
• Expose peer status and connection events over IPC (snapshot + SSE streams) and wire toggle/resume/close handling in LocalBackend.

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
portforward/portforward.go	Implements UPnP IGD discovery, mapping/unmapping, renewal, and external IP lookup for peer sessions.
portforward/portforward_test.go	Unit tests for UPnP forwarder behavior, ctx handling, renewal loop, and error wrapping.
portforward/manual.go	Adds manual port-forward implementation + parsing for UPnP-less networks.
portforward/manual_test.go	Tests manual port parsing and manual forwarder contract.
peer/validate.go	Adds structural validation to ensure server launch_cfg includes required abuse-blocking rules.
peer/validate_test.go	Comprehensive unit tests for abuse-rule validation (happy path + many failure modes).
peer/peer.go	Implements peer session orchestration, status phases, connection event forwarding, and credential rotation.
peer/peer_test.go	Extensive tests covering Start/Stop semantics, verify/unwind, header forwarding, rotation, and status events.
peer/api.go	Implements peer HTTP client (register/verify/heartbeat/deregister) using common headers + feature override.
events/events.go	Updates event emission to snapshot subscriber callbacks under lock + adds optional emit debug hook.
ipc/server.go	Adds peer status snapshot endpoint and SSE streams for peer status + connection events.
ipc/client_events_nonmobile.go	Adds client helpers to consume peer status/connection SSE streams on desktop.
ipc/client_events_mobile.go	Adds mobile dual-path (in-proc subscribe vs SSE) for peer status/connection events.
common/settings/settings.go	Adds PeerShareEnabledKey and PeerManualPortKey settings metadata.
common/env/env.go	Adds RADIANCE_PEER_EXTERNAL_PORT env var for manual port override.
backend/peer_share.go	Adds LocalBackend peer client construction, toggle apply/rollback, auto-resume, close coordination, and status accessor.
backend/radiance.go	Wires peer client into backend lifecycle: init, auto-resume on Start, stop on Close, and PatchSettings dispatch.
backend/radiance_test.go	Adds backend tests for peer toggle dispatch, rollback, auto-resume, and shutdown ordering.
go.mod	Updates lantern-box dependency and adds direct dependency on huin/goupnp.
go.sum	Updates module checksums for new/updated dependencies.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 7

🧹 Nitpick comments (7)

portforward/portforward_test.go (2)

128-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

This test does not cover the mid-call cancellation path.

runWithCtx returns at its ctx.Err() pre-check when ctx is already cancelled, so AddPortMapping is never called here. c.addBlock is never received on, and close(block) releases nothing. The comment on Line 128 describes a hung router, which is the mid-call case: the gateway call is in flight and ctx expires during the wait. Add a case that cancels after MapPort blocks, so the select branch in runWithCtx is exercised.

💚 Proposed additional test

// Cancelling while the gateway call is in flight must abort the wait rather
// than block until the router answers.
func TestForwarder_MapPort_CancelsMidCall(t *testing.T) {
	block := make(chan struct{})
	c := &fakeIGD{addBlock: block}
	f := newTestForwarder(t, c)

	ctx, cancel := context.WithCancel(context.Background())
	go func() {
		// Wait until AddPortMapping is actually blocked, then cancel.
		for c.addCalls.Load() == 0 {
			time.Sleep(time.Millisecond)
		}
		cancel()
	}()

	_, err := f.MapPort(ctx, 30001, "test")
	require.Error(t, err)
	assert.ErrorIs(t, err, context.Canceled)
	assert.Equal(t, int64(1), c.addCalls.Load(), "gateway call must have started")
	close(block) // release the in-flight goroutine
}

🤖 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 `@portforward/portforward_test.go` around lines 128 - 142, Add a separate
mid-call cancellation test alongside
TestForwarder_MapPort_RespectsContextCancellation that waits until
fakeIGD.AddPortMapping has started, cancels the context while it remains
blocked, and asserts MapPort returns context.Canceled. Keep the existing
pre-cancelled test, verify the gateway call started exactly once, and close the
blocking channel afterward to release the in-flight goroutine.

---

169-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

StartRenewal itself has no test, and the loop goroutine is not joined.

This test calls f.renewLoop directly, so f.cancel is never set. The guards in StartRenewal are untested: the early return when a renewal is already running, the early return when f.mapping is nil, and the 1-minute interval floor. UnmapPort depends on f.cancel being set to stop the loop.

The test also returns immediately after cancel() without waiting for renewLoop to exit. Join the goroutine so the loop cannot call the fake after the test finishes.

💚 Proposed changes

 	ctx, cancel := context.WithCancel(context.Background())
-	go f.renewLoop(ctx, 20*time.Millisecond)
+	loopDone := make(chan struct{})
+	go func() {
+		f.renewLoop(ctx, 20*time.Millisecond)
+		close(loopDone)
+	}()
 
 	deadline := time.After(2 * time.Second)
 	for c.addCalls.Load() < 3 {
 		select {
 		case <-deadline:
 			t.Fatalf("renewal fired only %d times", c.addCalls.Load())
 		case <-time.After(10 * time.Millisecond):
 		}
 	}
 	cancel()
+	<-loopDone
 }

Add coverage for StartRenewal:

// StartRenewal must set f.cancel so UnmapPort can stop the loop, and must
// refuse to start a second loop.
func TestForwarder_StartRenewal_Guards(t *testing.T) {
	c := &fakeIGD{}
	f := newTestForwarder(t, c)

	// No mapping yet: StartRenewal must not arm anything.
	f.StartRenewal(context.Background())
	f.mu.Lock()
	assert.Nil(t, f.cancel, "no mapping means no renewal loop")
	f.mu.Unlock()

	_, err := f.MapPort(context.Background(), 30001, "test")
	require.NoError(t, err)

	f.StartRenewal(context.Background())
	f.mu.Lock()
	first := f.cancel
	f.mu.Unlock()
	require.NotNil(t, first, "StartRenewal must store a cancel func")

	f.StartRenewal(context.Background())
	f.mu.Lock()
	assert.NotNil(t, f.cancel, "second call must not clear the existing loop")
	f.mu.Unlock()

	require.NoError(t, f.UnmapPort(context.Background()))
	f.mu.Lock()
	assert.Nil(t, f.cancel, "UnmapPort must clear the renewal cancel func")
	f.mu.Unlock()
}

🤖 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 `@portforward/portforward_test.go` around lines 169 - 190, Replace the direct
renewLoop invocation in TestForwarder_StartRenewal_ReissuesAddPortMapping with
StartRenewal so the test exercises f.cancel and UnmapPort integration, and add
coverage for StartRenewal’s nil-mapping guard, duplicate-start guard, and
one-minute interval floor. Ensure the renewal goroutine is joined after
cancellation before the test returns, using the existing renewal lifecycle
symbols StartRenewal, renewLoop, and UnmapPort.

portforward/portforward.go (2)

26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add doc comments to the exported Mapping type and LocalIP function.

Mapping at Line 26 and LocalIP at Line 319 are exported and carry no doc comment. Every other exported identifier in this file is documented. LocalIP is a bare re-export of localIP, so its purpose and its fallback behavior are not discoverable from the name alone.

As per coding guidelines: "Use Go doc comments (// Foo ...) for exported identifiers" and doc comments "must start with the identifier's name and a concise summary in the format // Foo does X.".

♻️ Proposed doc comments

+// Mapping describes an active port forward on the local gateway. Protocol is
+// always "TCP". LeaseDuration is the duration that was requested, not
+// necessarily the one the router applied.
 type Mapping struct {

+// LocalIP returns the LAN IPv4 address the OS would use to reach the gateway.
+// It is the exported form of localIP: it first tries a UDP-noop dial and
+// falls back to scanning interfaces for a private IPv4.
 func LocalIP() (string, error) { return localIP() }

Also applies to: 319-319

🤖 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 `@portforward/portforward.go` at line 26, Add Go doc comments for the exported
Mapping type and LocalIP function, placing each comment immediately before its
declaration and starting with the identifier name. Summarize Mapping’s purpose
and LocalIP’s behavior, including its fallback behavior when delegating to
localIP.

Source: Coding guidelines

---

84-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Log the discarded discovery errors.

NewForwarder drops the error from discoverIGDv2 and discoverIGDv1. A gateway that is present but returns a SOAP or transport error is then reported as ErrNoPortForwarding, which the doc comment reserves for "discovery completed without finding a usable gateway". UPnP failures are the main support case for this feature. A debug log of each discarded error makes them diagnosable from user logs.

♻️ Proposed refactor

-	if c, err := discoverIGDv2(ctx); err == nil && c != nil {
-		return &Forwarder{client: c, method: "upnp-igd2"}, nil
-	}
-	if c, err := discoverIGDv1(ctx); err == nil && c != nil {
-		return &Forwarder{client: c, method: "upnp-igd1"}, nil
-	}
+	c, err := discoverIGDv2(ctx)
+	if err == nil && c != nil {
+		return &Forwarder{client: c, method: "upnp-igd2"}, nil
+	}
+	if err != nil {
+		slog.Debug("portforward: IGDv2 discovery failed", "err", err)
+	}
+	c, err = discoverIGDv1(ctx)
+	if err == nil && c != nil {
+		return &Forwarder{client: c, method: "upnp-igd1"}, nil
+	}
+	if err != nil {
+		slog.Debug("portforward: IGDv1 discovery failed", "err", err)
+	}

🤖 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 `@portforward/portforward.go` around lines 84 - 89, Update NewForwarder’s
discoverIGDv2 and discoverIGDv1 fallback branches to retain and emit each
non-nil discovery error at debug level before trying the next protocol or
returning ErrNoPortForwarding. Preserve the existing successful-client selection
and fallback behavior while making both discarded errors available in user logs.

peer/peer.go (1)

96-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add Go doc comments to these exported identifiers.

Status (line 96), NewClient (line 204), IsActive (line 562), and CurrentStatus (line 568) have no doc comment. The neighbouring exported identifiers in this file already carry one.

As per coding guidelines: "Use Go doc comments (// Foo ...) for exported identifiers" and comments "must start with the identifier's name".

Also applies to: 204-204, 562-572

🤖 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 `@peer/peer.go` at line 96, Add Go doc comments immediately before the exported
identifiers Status, NewClient, IsActive, and CurrentStatus in peer.go. Each
comment must begin with the corresponding identifier name and briefly describe
its purpose, matching the style of neighboring exported declarations.

Source: Coding guidelines

peer/api.go (1)

17-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exported identifiers in the new peer package lack Go doc comments. The coding guidelines require a doc comment on every exported identifier, starting with the identifier's name. Both files document some exported identifiers and skip others, so the package's public surface is documented inconsistently.

• peer/api.go#L17-L31: add doc comments to RegisterRequest, RegisterResponse, LifecycleRequest, and to API (line 45), Register (line 60), and Deregister (line 90).
• peer/peer.go#L96-L96: add doc comments to Status, and to NewClient (line 204), IsActive (line 562), and CurrentStatus (line 568).

As per coding guidelines: "Use Go doc comments (// Foo ...) for exported identifiers" and "Go doc comments must start with the identifier's name and a concise summary in the format // Foo does X.".

🤖 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 `@peer/api.go` around lines 17 - 31, Add concise Go doc comments beginning with
each identifier’s name for the exported types RegisterRequest, RegisterResponse,
LifecycleRequest, API, Register, Deregister, Status, NewClient, IsActive, and
CurrentStatus. Update peer/api.go (lines 17-31 and the listed API methods) and
peer/peer.go (line 96 and the listed methods); no other changes are needed.

Source: Coding guidelines

peer/peer_test.go (1)

884-898: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

drainPhases reads a fixed count, so one stray event breaks the assertions.

events uses a process-global subscription map. The subscriber in TestClient_StatusEventEmittedOnStartAndStop receives every StatusEvent emitted anywhere in the package, including events from a client that a previous test left running (the rotation and heartbeat tests both stop clients from background goroutines).

drainPhases reads exactly n events. One unrelated event consumes a slot, so a wanted phase is never collected and assert.Contains fails. Drain until the wanted set is complete instead of counting.

♻️ Proposed refactor

-// drainPhases reads up to n StatusEvents from got and returns them
-// keyed by Phase (last event per phase wins). Used by tests that need
-// set-membership semantics rather than strict ordering because
-// events.Emit's per-callback goroutines deliver out of order under
-// the runtime's scheduling.
-func drainPhases(t *testing.T, got <-chan StatusEvent, n int) map[Phase]StatusEvent {
+// drainPhases reads StatusEvents from got until every phase in want has
+// been seen, and returns them keyed by Phase (last event per phase
+// wins). Reading until the wanted set is complete — rather than reading
+// a fixed count — keeps the test stable when an unrelated client emits
+// on the process-global event bus.
+func drainPhases(t *testing.T, got <-chan StatusEvent, want map[Phase]bool) map[Phase]StatusEvent {
 	t.Helper()
-	out := make(map[Phase]StatusEvent, n)
+	out := make(map[Phase]StatusEvent, len(want))
 	deadline := time.After(2 * time.Second)
-	for i := 0; i < n; i++ {
+	for {
+		missing := 0
+		for p := range want {
+			if _, ok := out[p]; !ok {
+				missing++
+			}
+		}
+		if missing == 0 {
+			return out
+		}
 		select {
 		case evt := <-got:
 			out[evt.Status.Phase] = evt
 		case <-deadline:
-			t.Fatalf("received only %d/%d status events within 2s; got phases: %v",
-				i, n, mapKeys(out))
+			t.Fatalf("missing %d/%d status phases within 2s; got phases: %v",
+				missing, len(want), mapKeys(out))
 		}
 	}
-	return out
 }

Update both call sites to pass the phase set:

startEvents := drainPhases(t, got, wantStartPhases)
stopEvents := drainPhases(t, got, wantStopPhases)

🤖 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 `@peer/peer_test.go` around lines 884 - 898, Update drainPhases to accept the
wanted phase set and continue receiving events until every requested phase has
been collected, ignoring unrelated events while retaining the timeout. Modify
both TestClient_StatusEventEmittedOnStartAndStop call sites to pass
wantStartPhases and wantStopPhases, respectively.

🤖 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 `@backend/peer_share.go`:
- Around line 122-126: Update LocalBackend.PeerStatus to guard against a nil
peerClient before calling CurrentStatus, returning the appropriate zero or
unavailable peer.Status value when absent. Preserve the existing CurrentStatus
result for initialized peer clients.

In `@backend/radiance.go`:
- Around line 189-193: Update newPeerClient in the backend construction flow to
log peer-client creation failures and continue with a nil client instead of
returning the error, preserving common.Init as the only fatal path. Add
nil-client guards to PeerStatus and applyPeerShare so peer sharing safely
returns an error when unavailable rather than panicking.
- Around line 606-614: Update PatchSettings around maybeRestartVPN and
applyPeerShare so both handlers run even when maybeRestartVPN fails, ensuring a
changed PeerShareEnabledKey is applied despite VPN restart errors. Capture each
handler error and join them before returning, preserving successful application
and the existing applyPeerShare rollback behavior.

In `@events/events.go`:
- Around line 152-167: Make the emitDebugLogger hook concurrency-safe by storing
it in an atomic pointer and loading it in Emit before invocation. Update
SetEmitDebugLogger to atomically replace the hook, including an atomic no-op
default for nil, and add the required sync/atomic support; remove the outdated
restriction that callers must avoid concurrent updates.

In `@go.mod`:
- Line 40: Update the github.com/getlantern/lantern-box dependency in go.mod
from the staging pseudo-version to the released v0.0.108 tag, preserving the
rest of the dependency declarations unchanged.

In `@peer/validate.go`:
- Around line 200-214: Update isUnconditionalReject to allow the top-level type
field in its allowed-key set, treating it as a non-match discriminator like
action and invert. Preserve the existing rejection behavior for all other
unexpected fields and the current unconditional reject checks.

In `@portforward/portforward.go`:
- Around line 211-237: Update Forwarder.renewLoop to track an explicit teardown
state, re-check that state while holding f.mu immediately before AddPortMapping,
and keep f.mu held through the renewal call so UnmapPort cannot delete
concurrently with a renewal. Set the teardown flag in UnmapPort before
cancellation/deletion, and preserve normal renewal behavior when teardown has
not started.

---

Nitpick comments:
In `@peer/api.go`:
- Around line 17-31: Add concise Go doc comments beginning with each
identifier’s name for the exported types RegisterRequest, RegisterResponse,
LifecycleRequest, API, Register, Deregister, Status, NewClient, IsActive, and
CurrentStatus. Update peer/api.go (lines 17-31 and the listed API methods) and
peer/peer.go (line 96 and the listed methods); no other changes are needed.

In `@peer/peer_test.go`:
- Around line 884-898: Update drainPhases to accept the wanted phase set and
continue receiving events until every requested phase has been collected,
ignoring unrelated events while retaining the timeout. Modify both
TestClient_StatusEventEmittedOnStartAndStop call sites to pass wantStartPhases
and wantStopPhases, respectively.

In `@peer/peer.go`:
- Line 96: Add Go doc comments immediately before the exported identifiers
Status, NewClient, IsActive, and CurrentStatus in peer.go. Each comment must
begin with the corresponding identifier name and briefly describe its purpose,
matching the style of neighboring exported declarations.

In `@portforward/portforward_test.go`:
- Around line 128-142: Add a separate mid-call cancellation test alongside
TestForwarder_MapPort_RespectsContextCancellation that waits until
fakeIGD.AddPortMapping has started, cancels the context while it remains
blocked, and asserts MapPort returns context.Canceled. Keep the existing
pre-cancelled test, verify the gateway call started exactly once, and close the
blocking channel afterward to release the in-flight goroutine.
- Around line 169-190: Replace the direct renewLoop invocation in
TestForwarder_StartRenewal_ReissuesAddPortMapping with StartRenewal so the test
exercises f.cancel and UnmapPort integration, and add coverage for
StartRenewal’s nil-mapping guard, duplicate-start guard, and one-minute interval
floor. Ensure the renewal goroutine is joined after cancellation before the test
returns, using the existing renewal lifecycle symbols StartRenewal, renewLoop,
and UnmapPort.

In `@portforward/portforward.go`:
- Line 26: Add Go doc comments for the exported Mapping type and LocalIP
function, placing each comment immediately before its declaration and starting
with the identifier name. Summarize Mapping’s purpose and LocalIP’s behavior,
including its fallback behavior when delegating to localIP.
- Around line 84-89: Update NewForwarder’s discoverIGDv2 and discoverIGDv1
fallback branches to retain and emit each non-nil discovery error at debug level
before trying the next protocol or returning ErrNoPortForwarding. Preserve the
existing successful-client selection and fallback behavior while making both
discarded errors available in user logs.

🪄 Autofix

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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7136f722-5b36-4ead-bf06-38528b79f076

📥 Commits

Reviewing files that changed from the base of the PR and between 0aebcab and 03dc83f.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (19)

• backend/peer_share.go
• backend/radiance.go
• backend/radiance_test.go
• common/env/env.go
• common/settings/settings.go
• events/events.go
• go.mod
• ipc/client_events_mobile.go
• ipc/client_events_nonmobile.go
• ipc/server.go
• peer/api.go
• peer/peer.go
• peer/peer_test.go
• peer/validate.go
• peer/validate_test.go
• portforward/manual.go
• portforward/manual_test.go
• portforward/portforward.go
• portforward/portforward_test.go

[Copilot]

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (2)

portforward/portforward.go:155

• MapPort’s addCallTimeout path can still leak an untracked port mapping: on timeout you return an error without setting f.mapping, but the in-flight AddPortMapping goroutine may later succeed, and the deferred Start rollback’s UnmapPort will short-circuit on nil mapping. Consider scheduling a best-effort background cleanup that waits for the AddPortMapping result and issues DeletePortMapping if it eventually succeeds.

	case <-time.After(addCallTimeout):
		// Outcome unknowable; assume it landed so the cleanup below removes it.
		addLanded = true
		err = fmt.Errorf("add port mapping timed out after %s", addCallTimeout)
	}

go.mod:41

• PR description says this is blocked on tagging lantern-box v0.0.108 and re-pinning to that tag before merge, but go.mod currently requires github.com/getlantern/lantern-box v0.0.109. Either the description needs updating or the dependency pin should be aligned with the intended lantern-box release/tag to avoid merging with an unexpected box version.

	github.com/getlantern/keepcurrent v0.0.0-20260616120552-f204338b01a3
	github.com/getlantern/kindling v0.0.0-20260727211028-573c1ef64464
	github.com/getlantern/lantern-box v0.0.109
	github.com/getlantern/pluriconfig v0.0.0-20251126214241-8cc8bc561535

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

portforward/portforward.go (2)

141-167: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

The add-timeout path can leave an untracked mapping and misreports it as ErrNoPortForwarding.

If AddPortMapping exceeds addCallTimeout but ctx is still live, the code sets addLanded = true and err to the timeout error, then skips the cleanup at Line 150 because ctx.Err() is nil. Control reaches Line 156, f.mapping stays nil, and UnmapPort short-circuits on a nil mapping. The mapping that may have landed on the gateway is then untracked, which is the exact leak this block prevents on the cancellation path.

The same path also wraps the timeout in ErrNoPortForwarding. Per the sentinel docstring, callers read that as "this network cannot host a peer". A wedged gateway is a transient condition and should stay retryable.

🔒️ Proposed fix: clean up on any timeout and keep the timeout retryable

 	var addLanded bool
+	var addTimedOut bool
 	select {
 	case err = <-addErrCh:
 		addLanded = err == nil
 	case <-time.After(addCallTimeout):
 		// Outcome unknowable; assume it landed so the cleanup below removes it.
 		addLanded = true
+		addTimedOut = true
 		err = fmt.Errorf("add port mapping timed out after %s", addCallTimeout)
 	}
-	if ctxErr := ctx.Err(); ctxErr != nil && addLanded {
+	ctxErr := ctx.Err()
+	if addLanded && (ctxErr != nil || addTimedOut) {
 		// The caller gave up but the gateway accepted the mapping. Remove it —
 		// otherwise it survives with no Forwarder tracking it.
 		f.deleteRacedMapping(&Mapping{ExternalPort: externalPort, Protocol: "TCP"}, client)
+	}
+	if ctxErr != nil && addLanded {
 		return nil, fmt.Errorf("add port mapping: %w", ctxErr)
 	}
+	if addTimedOut {
+		// Transient: do not join ErrNoPortForwarding, the caller may retry.
+		return nil, fmt.Errorf("add port mapping: %w", err)
+	}
 	if err != nil {

🤖 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 `@portforward/portforward.go` around lines 141 - 167, Update the add-timeout
handling around addLanded and err so every timeout triggers deleteRacedMapping
for the potentially landed mapping, regardless of whether ctx is canceled.
Return the timeout error as a retryable add-port-mapping failure without
wrapping it with ErrNoPortForwarding, while preserving the existing cancellation
propagation and sentinel wrapping for non-timeout gateway errors.

Source: Learnings

---

203-218: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reset closed when installing a new mapping

UnmapPort sets f.closed to true, and no code resets it. A mapping created after UnmapPort exits renewLoop at its first tick and expires when the router lease ends. Set f.closed = false when MapPort installs the new mapping, and add a test that starts renewal after remapping.

🤖 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 `@portforward/portforward.go` around lines 203 - 218, Reset f.closed to false
in MapPort when a new mapping is successfully installed, so renewal can run
after UnmapPort followed by remapping. Add a test covering the unmap/remap
sequence and verify the renewed mapping remains active beyond the first renewal
tick.

🧹 Nitpick comments (2)

peer/peer_test.go (1)

1111-1128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use require.Eventually for the deregister assertion instead of the fixed sleep.

time.Sleep(100 * time.Millisecond) bounds the negative assertions (built == 1, box not closed), which is reasonable. It also gates the positive assertion at Line 1126. On a loaded CI runner the rejected rotation may not have finished its deregister call within 100ms, so deregisterCount can still be 0 and the test flakes.

Wait for the deregister explicitly, then keep the sleep only for the negative assertions.

♻️ Proposed change: wait for the deregister before asserting

 	require.Eventually(t, func() bool { return srv.registerCount.Load() >= 2 },
 		3*time.Second, 10*time.Millisecond, "rotation never re-registered")
+	// The orphan route from the rejected rotation must be deregistered.
+	require.Eventually(t, func() bool { return srv.deregisterCount.Load() > 0 },
+		3*time.Second, 10*time.Millisecond,
+		"the orphan route created for the rejected rotation was never deregistered")
+	// Settle time for the negative assertions below.
 	time.Sleep(100 * time.Millisecond)
@@
 	assert.True(t, c.IsActive(), "the peer must stay active after a rejected rotation")
-	assert.Positive(t, srv.deregisterCount.Load(),
-		"the orphan route created for the rejected rotation must be deregistered")
 }

🤖 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 `@peer/peer_test.go` around lines 1111 - 1128, Replace the fixed-sleep gating
of the deregistration check in this test with require.Eventually polling
srv.deregisterCount.Load() until it is positive. Keep the existing
time.Sleep(100 * time.Millisecond) only for the negative assertions about boxes
built and the original box remaining open, then retain the positive
deregistration assertion after the explicit wait.

portforward/portforward_test.go (1)

318-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

This test does not exercise the closed check.

UnmapPort sets f.mapping = nil on success. renewLoop then returns on the pre-existing m == nil condition, so the test passes with closed removed. To pin the new semantics, make DeletePortMapping return an error. UnmapPort then retains f.mapping and sets closed, so only the closed check stops the renewal.

♻️ Proposed change: force the mapping to survive teardown

-	c := &fakeIGD{}
+	// A failed delete leaves f.mapping set, so only the closed flag can stop
+	// the renewal.
+	c := &fakeIGD{deleteErr: errors.New("gateway refused")}
 	f := newTestForwarder(t, c)
 	f.mapping = &Mapping{
 		ExternalPort: 15001, InternalPort: 15001, InternalIP: "192.168.1.10",
 		Protocol: "TCP", LeaseDuration: time.Hour,
 	}
-	require.NoError(t, f.UnmapPort(context.Background()))
+	require.Error(t, f.UnmapPort(context.Background()))
 	require.Equal(t, int64(1), c.deleteCalls.Load())

Confirm that fakeIGD exposes a delete-error hook with this field name.

🤖 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 `@portforward/portforward_test.go` around lines 318 - 340, Update
TestForwarder_RenewalAfterTeardown_DoesNotReAdd to configure the fakeIGD
delete-error hook before calling UnmapPort, causing DeletePortMapping to fail so
f.mapping remains set while teardown marks the forwarder closed. Retain the
existing renewal assertions, ensuring the test specifically verifies renewLoop
exits via the closed check without issuing AddPortMapping or a compensating
delete.

🤖 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 `@events/events_test.go`:
- Around line 13-19: Update the Go doc comments immediately preceding
TestSetEmitDebugLogger_SafeConcurrentlyWithEmit and
TestSetEmitDebugLogger_ReceivesTypeAndCountThenClears in events/events_test.go
(sites 13-19 and 49-52) so each comment starts with its corresponding test
function name; no other changes are required.

---

Outside diff comments:
In `@portforward/portforward.go`:
- Around line 141-167: Update the add-timeout handling around addLanded and err
so every timeout triggers deleteRacedMapping for the potentially landed mapping,
regardless of whether ctx is canceled. Return the timeout error as a retryable
add-port-mapping failure without wrapping it with ErrNoPortForwarding, while
preserving the existing cancellation propagation and sentinel wrapping for
non-timeout gateway errors.
- Around line 203-218: Reset f.closed to false in MapPort when a new mapping is
successfully installed, so renewal can run after UnmapPort followed by
remapping. Add a test covering the unmap/remap sequence and verify the renewed
mapping remains active beyond the first renewal tick.

---

Nitpick comments:
In `@peer/peer_test.go`:
- Around line 1111-1128: Replace the fixed-sleep gating of the deregistration
check in this test with require.Eventually polling srv.deregisterCount.Load()
until it is positive. Keep the existing time.Sleep(100 * time.Millisecond) only
for the negative assertions about boxes built and the original box remaining
open, then retain the positive deregistration assertion after the explicit wait.

In `@portforward/portforward_test.go`:
- Around line 318-340: Update TestForwarder_RenewalAfterTeardown_DoesNotReAdd to
configure the fakeIGD delete-error hook before calling UnmapPort, causing
DeletePortMapping to fail so f.mapping remains set while teardown marks the
forwarder closed. Retain the existing renewal assertions, ensuring the test
specifically verifies renewLoop exits via the closed check without issuing
AddPortMapping or a compensating delete.

🪄 Autofix

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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 21972017-ee43-40c8-ab1a-8931b72ea630

📥 Commits

Reviewing files that changed from the base of the PR and between bbb9508 and a25de28.

📒 Files selected for processing (12)

• backend/peer_share.go
• backend/radiance.go
• backend/radiance_test.go
• events/events.go
• events/events_test.go
• go.mod
• peer/peer.go
• peer/peer_test.go
• peer/validate.go
• peer/validate_test.go
• portforward/portforward.go
• portforward/portforward_test.go

🚧 Files skipped from review as they are similar to previous changes (8)

• events/events.go
• go.mod
• backend/radiance.go
• backend/peer_share.go
• peer/validate_test.go
• backend/radiance_test.go
• peer/validate.go
• peer/peer.go

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Start each Go doc comment with its test function name.

• events/events_test.go#L13-L19: Start the comment with TestSetEmitDebugLogger_SafeConcurrentlyWithEmit.
• events/events_test.go#L49-L52: Start the comment with TestSetEmitDebugLogger_ReceivesTypeAndCountThenClears.

📍 Affects 1 file

• events/events_test.go#L13-L19 (this comment)
• events/events_test.go#L49-L52

🤖 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 `@events/events_test.go` around lines 13 - 19, Update the Go doc comments
immediately preceding TestSetEmitDebugLogger_SafeConcurrentlyWithEmit and
TestSetEmitDebugLogger_ReceivesTypeAndCountThenClears in events/events_test.go
(sites 13-19 and 49-52) so each comment starts with its corresponding test
function name; no other changes are required.

Source: Coding guidelines