[client] Fix WGWatcher silently failing to restart on fast disconnect/reconnect#6664
Open
riccardomanfrin wants to merge 4 commits into
Open
[client] Fix WGWatcher silently failing to restart on fast disconnect/reconnect#6664riccardomanfrin wants to merge 4 commits into
riccardomanfrin wants to merge 4 commits into
Conversation
and its removal to the removal of such same conn. Avoid debouncing and cross lock dead locking
onWGDisconnected only checked conn.ctx (the engine-scoped context), never the watcher's own context. disableWgWatcherIfNeeded cancels the wgWatcherCtx, not conn.ctx, so a disabled watcher's timeout callback did not see the cancellation. handshakeCheck runs lock-free, so between the ctx check in periodicHandshakeCheck and acquiring conn.mu a fast disconnect/reconnect can slip in: the stale watcher then acquires the lock and tears down the *new*, healthy connection based on the old timeout, forcing the guard into an unnecessary reconnect (flap). Recheck watcherCtx.Err() under conn.mu so a superseded watcher exits without touching the connection that replaced it.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReworks WGWatcher lifecycle management in ChangesWGWatcher Lifecycle Rework
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
|
pappz
approved these changes
Jul 6, 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
2 participants
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.
Describe your changes
WGWatchermonitors WireGuard handshake liveness and tears down a dead transport so the guard reconnects. Fixes a race where the watcher could silently stop running, plus a related teardown race.Per-instance watcher (main fix). The watcher was a reused instance guarded by an
enabledbool;disableWgWatcherIfNeededonlycancel()ed the goroutine, which clearedenabledafter its loop returned (scheduler-dependent delay). On a fast disconnect + independent relay/ICE "ready", the re-enable saw a staleenabled == trueand started nothing → a "connected" peer with no liveness watcher, never retried. Fix: create a fresh watcher per connection attempt, dropped on disable; lifecycle fully owned byConnunderconn.mu, so enable/disable can't race the old goroutine's async shutdown. Removedenabled/muEnabled.Recheck watcher ctx under
conn.muinonWGDisconnected.handshakeCheckruns lock-free; between its ctx check and acquiringconn.mu, a fast reconnect can slip in.onWGDisconnectedchecked onlyconn.ctx(engine-scoped), but disable cancelswgWatcherCtx— so a stale watcher could tear down the new connection on the old timeout (flap). Now it also recheckswatcherCtx.Err()under the lock.Log clarity. Distinguish "handshake not updated" from an actual timeout.
Deadlock constraint preserved:
disableWgWatcherIfNeedednever waits for the goroutine (the timeout path reentrantly calls back underconn.mu); the cancelled goroutine drains via its ownctx.Done().Verified:
go test -run TestWGWatcher ./client/internal/peer/passes,go build ./client/...clean.Issue ticket number and link
No public GitHub issue — internal client-reliability fix for a scheduler-dependent race in the WireGuard liveness watcher. Pre-fix paths on
main:disableWgWatcherIfNeeded,onWGDisconnected.Stack
Checklist
Documentation
Select exactly one:
Internal concurrency fix in the client's WireGuard liveness watcher. No public CLI/API/gRPC/config surface changes; observable behavior is unchanged.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
N/A
Summary by CodeRabbit