fix(clientcore): make consumer signaling backoff context-aware - #372
Merged
Conversation
The consumer WebRTC FSM's genesis/offer retry paths slept on a bare `<-time.After(options.ErrorBackoff)` (default 5s) that ignored the FSM's context. When the outbound is torn down mid-backoff — e.g. an unbounded outbound being rebuilt on a network change — the WorkerFSM goroutine lingered up to a full ErrorBackoff before the run loop could observe cancellation and exit. Under rapid outbound rebuilds (a client roaming across networks) this stacks overlapping FSMs, each still holding its signaling/connection resources, which contributed to the iOS memory/goroutine growth in getlantern/engineering#3698. Replace the 8 backoff sleeps with a sleepOrDone(ctx, d) helper that also returns on ctx.Done(), so a stopped FSM exits its backoff immediately. Steady-state reconnection cadence is unchanged; only the cancellation path is faster. Complements #371 (engine teardown leak): #371 closes connections on FSM cancel, this removes the backoff delay before that cancel is observed on the consumer path (consumer.go is not touched by #371). Refs getlantern/engineering#3698 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe consumer FSM adds a context-aware sleep helper and uses it for error-backoff waits across genesis, offer signaling, and ICE-candidate signaling. Unit tests verify cancellation and duration-based waiting. ChangesConsumer backoff cancellation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clientcore/consumer.go (1)
176-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplicitly ignore the error returned by
res.Body.Close().Static analysis flags that the error return value of
res.Body.Close()is not checked. Although ignoring errors from HTTP response body closures is generally safe, explicitly ignoring the error with a blank identifier properly silences the linter and signals intent to future maintainers.
clientcore/consumer.go#L176-L176: Explicitly ignore the error usingdefer func() { _ = res.Body.Close() }().clientcore/consumer.go#L319-L319: Explicitly ignore the error usingdefer func() { _ = res.Body.Close() }().♻️ Proposed fixes
clientcore/consumer.go#L176-L176- defer res.Body.Close() + defer func() { _ = res.Body.Close() }()
clientcore/consumer.go#L319-L319- defer res.Body.Close() + defer func() { _ = res.Body.Close() }()🤖 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 `@clientcore/consumer.go` at line 176, Explicitly discard the errors returned when closing response bodies by replacing both deferred res.Body.Close calls in clientcore/consumer.go at lines 176-176 and 319-319 with deferred closures that assign the Close result to the blank identifier.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@clientcore/consumer.go`:
- Line 176: Explicitly discard the errors returned when closing response bodies
by replacing both deferred res.Body.Close calls in clientcore/consumer.go at
lines 176-176 and 319-319 with deferred closures that assign the Close result to
the blank identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a03427c6-7ec7-4b0b-ad08-74cfbb9ae202
📒 Files selected for processing (2)
clientcore/consumer.goclientcore/consumer_test.go
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the consumer WebRTC WorkerFSM backoff sleeps context-aware so cancellation via WorkerFSM.Stop() can be observed promptly instead of waiting out ErrorBackoff, reducing overlapping FSM lifetimes during rapid outbound rebuilds.
Changes:
- Added
sleepOrDone(ctx, d)to wait for either a duration orctx.Done(). - Replaced
ErrorBackofftime.Aftersleeps in the consumer FSM withsleepOrDone. - Added unit tests validating
sleepOrDonereturns promptly on cancellation and waits otherwise.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| clientcore/consumer.go | Introduces sleepOrDone and applies it to error-backoff sleeps in the consumer FSM. |
| clientcore/consumer_test.go | Adds unit tests covering the new context-aware sleep helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses PR review: the genesis listen loop's decode-error path slept then `continue`d within the same state. With the backoff now context-aware, a cancel during that sleep returned immediately and the loop continued without ever handing control back to the FSM runner (which only observes cancellation between states) — delaying shutdown and risking a tight loop if more malformed messages arrived. Return early when ctx is done instead, matching the scanner.Err() path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
myleshorton
added a commit
to getlantern/lantern-box
that referenced
this pull request
Jul 17, 2026
Bumps github.com/getlantern/broflake c4d1516 → f2cacf69, pulling the consumer-side unbounded fixes into lantern-box's unbounded outbound: - getlantern/unbounded#371: engine teardown no longer leaks the bus observer, table routers, UI ticker, or a live PeerConnection per connect/disconnect cycle. - getlantern/unbounded#372: consumer signaling backoff is context-aware, so a torn-down outbound exits its retry backoff immediately instead of lingering a full ErrorBackoff. - getlantern/unbounded#373: QUICLayer initializes ctx/cancel in its constructor, fixing the Close() data race + lost-cancel leak. broflake is a direct dependency here (protocol/unbounded imports broflake/clientcore), so this is the correct home for the bump — radiance picks it up by bumping lantern-box rather than overriding the transitive version. Addresses getlantern/engineering#3698. Ran go mod tidy. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
Problem
The consumer WebRTC FSM's genesis/offer retry paths in
consumer.goslept on a bare<-time.After(options.ErrorBackoff)(default 5s) that ignored the FSM'sctx.WorkerFSM.Stop()cancels that ctx, but the run loop only checks it between states — so an FSM parked in a backoff sleep lingers up to a fullErrorBackoffbefore it can exit.That matters under rapid outbound rebuilds: when a client roams across networks, the unbounded outbound is torn down and re-created repeatedly. Each stopped FSM that's mid-backoff keeps its goroutine (and the signaling/connection resources it holds) alive for the remainder of the backoff, so overlapping FSMs stack up. This was a contributor to the iOS memory/goroutine growth in getlantern/engineering#3698 (ticket 180321: freddie signaling was failing 453×, so the FSM was almost always sitting in exactly these backoff sleeps).
Fix
Add a
sleepOrDone(ctx, d)helper that waits fordorctx.Done(), and use it for all 8 backoff sleeps in the consumer FSM. Steady-state reconnection cadence is unchanged; only the cancellation path is faster (prompt instead of up-to-5s).Relationship to #371
Complementary, non-overlapping. #371 (
garmr/fix-engine-teardown-leak) closes connections/goroutines on FSM cancel but does not touchconsumer.go. This PR removes the backoff delay before that cancel is observed on the consumer path. Both are wanted.Verification
go build ./clientcore/clean;go test ./clientcore/passes.TestSleepOrDoneReturnsEarlyOnCancel/TestSleepOrDoneWaitsWhenNotCancelled.Refs getlantern/engineering#3698
🤖 Generated with Claude Code
Summary by CodeRabbit