Skip to content

[client] Extend WG watcher for ICE connection too#5133

Merged
pappz merged 4 commits intomainfrom
feature/ice-wg-watcher
Jan 21, 2026
Merged

[client] Extend WG watcher for ICE connection too#5133
pappz merged 4 commits intomainfrom
feature/ice-wg-watcher

Conversation

@pappz
Copy link
Copy Markdown
Collaborator

@pappz pappz commented Jan 19, 2026

Describe your changes

Extend the WireGuard watcher to monitor handshakes over ICE and relayed connections as well.

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

https://github.com/netbirdio/docs/pull/__

Summary by CodeRabbit

  • Bug Fixes

    • Improved WireGuard timeout handling and graceful failover between connection paths.
  • Refactor

    • Reworked watchdog lifecycle and state-dump integration to centralize enable/disable control and reduce duplicated goroutine/context management.
    • Relay worker no longer directly manages watcher lifecycle.
  • Tests

    • Updated watcher tests to use synchronized re-enable flows and context cancellation scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 19, 2026

📝 Walkthrough

Walkthrough

Moves WireGuard watcher lifecycle out of WorkerRelay into Conn, replacing per-watcher cancellable contexts with an explicit enabled flag and mutex, updates constructor signatures (NewWGWatcher, NewWorkerRelay), adds Conn-level enable/disable helpers and disconnect handlers, and adapts tests accordingly.

Changes

Cohort / File(s) Summary
WGWatcher lifecycle refactoring
client/internal/peer/wg_watcher.go
Replace context-based lifecycle fields with enabled + muEnabled; change EnableWgWatcher(ctx, onDisconnectedFn) signature; remove DisableWgWatcher; add IsEnabled() and calcElapsed(); update periodicHandshakeCheck signature/logic.
Conn-level watcher orchestration
client/internal/peer/conn.go
Add WGWatcher field and dumpState; introduce enableWgWatcherIfNeeded / disableWgWatcherIfNeeded; coordinate watcher lifecycle and wire WG disconnect handlers (onWGDisconnected, handleRelayDisconnectedLocked); remove dumpState from NewWorkerRelay call.
WorkerRelay watcher removal
client/internal/peer/worker_relay.go
Remove wgWatcher field and methods (EnableWgWatcher, DisableWgWatcher, onWGDisconnected); shorten NewWorkerRelay signature (drop stateDump); stop managing watcher in onRelayClientDisconnected.
Tests updated
client/internal/peer/wg_watcher_test.go
Remove direct DisableWgWatcher calls; refactor re-enable test to use goroutine + sync.WaitGroup and context cancellation; add sync import.

Sequence Diagram

sequenceDiagram
    participant Conn as Conn
    participant WGWatcher as WGWatcher
    participant WorkerRelay as WorkerRelay

    Note over WorkerRelay,WGWatcher: Old: WorkerRelay owned watcher
    WorkerRelay->>WGWatcher: EnableWgWatcher()
    WorkerRelay->>WGWatcher: DisableWgWatcher()

    Note over Conn,WGWatcher: New: Conn owns watcher lifecycle
    Conn->>WorkerRelay: NewWorkerRelay() (no watcher)
    Conn->>WGWatcher: NewWGWatcher(dumpState)
    Conn->>WGWatcher: enableWgWatcherIfNeeded(ctx,onDisconnected)
    WGWatcher->>WGWatcher: Set enabled = true
    WorkerRelay-->>Conn: relay established
    WGWatcher->>Conn: onDisconnected event
    Conn->>Conn: handleRelayDisconnectedLocked()
    Conn->>WGWatcher: disableWgWatcherIfNeeded()
    WGWatcher->>WGWatcher: Set enabled = false
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • lixmal

Poem

🐰 I hopped from relay to Conn with glee,
I swapped my cancels for a guarded key.
Flags set true beneath the moonlight's cheer,
I watch the wire and nudge connections clear—
A tidy hop, a careful ear.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: extending the WG watcher to monitor handshakes over ICE connections, which aligns with the substantial refactoring across conn.go, wg_watcher.go, and worker_relay.go.
Description check ✅ Passed The description is mostly complete with a clear summary of changes, appropriate refactor classification, and documentation justification, though it could be slightly more detailed about the architectural changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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 and usage tips.

@pappz pappz marked this pull request as ready for review January 19, 2026 17:02
@pappz pappz mentioned this pull request Jan 19, 2026
7 tasks
Comment thread client/internal/peer/wg_watcher.go Outdated
Comment thread client/internal/peer/wg_watcher.go Outdated
@pappz pappz requested a review from lixmal January 21, 2026 08:27
@sonarqubecloud
Copy link
Copy Markdown

@pappz pappz merged commit e908dea into main Jan 21, 2026
37 of 38 checks passed
@pappz pappz deleted the feature/ice-wg-watcher branch January 21, 2026 09:42
@coderabbitai coderabbitai Bot mentioned this pull request Mar 10, 2026
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants