fix(clientcore): init QUICLayer ctx/cancel in constructor (Close race + leak) by myleshorton · Pull Request #373 · getlantern/unbounded

myleshorton · 2026-07-17T14:56:46Z

Problem

ListenAndMaintainQUICConnection initialized c.ctx/c.cancel on its first line, but Close() reads c.cancel from a different goroutine. Two bugs:

Data race on c.ctx/c.cancel (writer = the maintain goroutine, reader = Close()), flagged by -race.
Lost cancel → permanent leak. If Close() runs before the maintain goroutine executes its first line (e.g. an unbounded outbound created and torn down in quick succession during a network change), c.cancel is still nil, so Close() no-ops. The goroutine then sets its own ctx/cancel and runs quic.Listen / Accept(c.ctx) forever on a context nobody holds the cancel for — orphaning the QUIC listener, the maintain goroutine, and the underlying bfconn (the WebRTC data channels). Under the connect/disconnect churn of a roaming iOS client this accumulates. Part of getlantern/engineering#3698.

Fix

Initialize ctx/cancel in NewQUICLayer, so they exist before the goroutine starts and before any Close() can race them. Close() keeps its nil guard for direct struct-literal construction.

Verification

go test -race ./clientcore/ passes.
Added TestQUICLayerCloseBeforeListen: constructs a layer, calls Close() before the maintain loop starts, asserts the context is cancelled and that ListenAndMaintainQUICConnection then returns promptly (via its existing ctx.Err() bailout) instead of spinning.

Refs getlantern/engineering#3698

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved QUIC connection lifecycle handling when connections are closed before listening begins.
- Prevented startup failures when the connection layer is not initialized through its standard setup process.
- Ensured the connection maintenance loop exits safely during early cancellation.
Tests
- Added coverage for early closure and safe startup behavior.

…routine ListenAndMaintainQUICConnection set c.ctx/c.cancel on its first line, but Close() reads c.cancel from another goroutine. That's a data race, and worse: a Close() that lands before the maintain goroutine's first line reads a nil cancel and no-ops — the goroutine then runs quic.Listen / Accept on a context that never gets cancelled, permanently leaking the listener, the maintain goroutine, and the underlying bfconn (the WebRTC data channels). On iOS this stacks across the connect/disconnect churn of a client roaming networks (getlantern/engineering#3698). Initialize ctx/cancel in NewQUICLayer so they exist before the goroutine starts and before any Close() can race them. Close() keeps its nil guard for struct-literal construction. Refs getlantern/engineering#3698 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

coderabbitai · 2026-07-17T14:56:56Z

Warning

Review limit reached

@myleshorton, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 50 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6f2c17ac-5d40-4342-86fe-6a9f80ce8fb2

📥 Commits

Reviewing files that changed from the base of the PR and between 71bcbc9 and 85e8bf5.

📒 Files selected for processing (2)

clientcore/quic.go
clientcore/quic_close_test.go

📝 Walkthrough

Walkthrough

QUIC lifecycle context and cancellation are initialized during construction. The maintenance loop now rejects layers without an initialized context, and tests cover both closing before listening and starting from a struct literal.

Changes

QUIC lifecycle safety

Layer / File(s)	Summary
Initialize lifecycle cancellation `clientcore/quic.go`, `clientcore/quic_close_test.go`	`NewQUICLayer` stores a context and cancel function immediately; tests verify that `Close()` before listening cancels the context and exits the maintenance loop.
Validate maintenance startup `clientcore/quic.go`, `clientcore/quic_close_test.go`	`ListenAndMaintainQUICConnection` returns when `ctx` is nil, and tests verify struct-literal layers do not panic.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: initializing QUICLayer ctx/cancel in the constructor to fix a Close race and leak.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fisk/quiclayer-close-race

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

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

Copilot

Pull request overview

This PR fixes a lifecycle race in clientcore’s QUICLayer by ensuring its context.Context and cancel function are initialized before any goroutine using them starts, preventing both a -race data race and a “lost cancel” leak scenario during fast teardown.

Changes:

Initialize QUICLayer’s ctx/cancel in NewQUICLayer instead of inside ListenAndMaintainQUICConnection.
Add a regression test that calls Close() before starting the maintain loop and asserts the loop returns promptly afterward.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
`clientcore/quic.go`	Moves context initialization into the constructor to remove Close-vs-goroutine initialization race/leak.
`clientcore/quic_close_test.go`	Adds a regression test for “Close before Listen” behavior to prevent reintroducing the leak/race.

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

…ral case) Addresses PR review: with ctx/cancel now set in NewQUICLayer instead of this method, a QUICLayer built via struct literal (no ctx) would nil-deref on c.ctx.Err(). Refuse to start with a logged error instead. Deliberately does NOT lazily init ctx/cancel here — that would move the write back into this goroutine and reintroduce the Close() race the constructor init exists to prevent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

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

…nnel Two PR-review follow-ups: - ListenAndMaintain's guard now checks c.cancel too, not just c.ctx: a layer with a non-nil ctx but nil cancel would pass the old guard yet be uncancellable (Close() no-ops), reintroducing the leak. Refuse to start if either is missing. - TestQUICLayerListenWithoutConstructor conveys any panic over a channel instead of calling t.Errorf from the goroutine, which could fire after the test completed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

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

myleshorton · 2026-07-17T15:26:33Z

@coderabbitai review

coderabbitai · 2026-07-17T15:26:57Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Bumps broflake dc7d5ec → f2cacf69, adding getlantern/unbounded#373 (init QUICLayer ctx/cancel in the constructor to fix the Close() data race and the lost-cancel leak) on top of #371 (teardown) and #372 (backoff) already pulled in. Completes the broflake-side fixes for getlantern/engineering#3698. Ran go mod tidy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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>

Copilot AI review requested due to automatic review settings July 17, 2026 14:56

Copilot started reviewing on behalf of myleshorton July 17, 2026 14:57 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

Comment thread clientcore/quic.go

myleshorton requested a review from Copilot July 17, 2026 15:17

Copilot started reviewing on behalf of myleshorton July 17, 2026 15:18 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

Comment thread clientcore/quic.go Outdated

Comment thread clientcore/quic_close_test.go

myleshorton requested a review from Copilot July 17, 2026 15:23

Copilot started reviewing on behalf of myleshorton July 17, 2026 15:24 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

myleshorton merged commit f2cacf6 into main Jul 17, 2026
2 checks passed

myleshorton mentioned this pull request Jul 17, 2026

deps: bump lantern-box to v0.0.103 (unbounded leak/retry fixes) getlantern/radiance#567

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(clientcore): init QUICLayer ctx/cancel in constructor (Close race + leak)#373

fix(clientcore): init QUICLayer ctx/cancel in constructor (Close race + leak)#373
myleshorton merged 3 commits into
mainfrom
fisk/quiclayer-close-race

myleshorton commented Jul 17, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

myleshorton commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

myleshorton commented Jul 17, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

myleshorton commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

myleshorton commented Jul 17, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading