Skip to content

fix(peer): keep WG peer entry across lazy-suspend so route-manager AllowedIPs survive - #6251

Open
vpsie wants to merge 1 commit into
netbirdio:mainfrom
vpsie:fix/lazy-suspend-keep-wg-peer
Open

fix(peer): keep WG peer entry across lazy-suspend so route-manager AllowedIPs survive#6251
vpsie wants to merge 1 commit into
netbirdio:mainfrom
vpsie:fix/lazy-suspend-keep-wg-peer

Conversation

@vpsie

@vpsie vpsie commented May 22, 2026

Copy link
Copy Markdown

Describe your changes

Fixes #6250. Gate peer.Conn.Close's call to endpointUpdater.RemoveWgPeer on a new keepWgPeer bool parameter so that AllowedIPs the route-manager has appended in place (via WgInterface.AddAllowedIP through its allowedIPsRefCounter) survive a lazy wake/sleep cycle.

Caller intent

Caller keepWgPeer Why
peerstore.PeerConnIdle true Lazy-suspend: data path off, WG entry stays
peerstore.PeerConnClose true Lazy excluded: same suspend semantics
engine.addNewPeer race-loser cleanup true The other Conn for this peer owns the WG entry
conn_mgr.RemovePeerConn false Permanent removal: drop the WG entry too

Without this change, the lazy path tears down the entire WG peer entry on every idle cycle, including the route-manager-appended prefixes. The next wake re-opens with only the peer's base /32 from PeerConfig.AllowedIps. The route-manager's refcounter is unaware of the round-trip and does not re-apply the prefixes until a management-side reconcile fires, so routed-subnet traffic is silently dropped by WG until then.

The visible symptom is a peer that shows Status: Connected with a fresh WireGuard handshake but Networks: - in netbird status -d, with manual uncheck/re-check of the network in the GUI being the only client-side workaround.

Issue ticket number and link

#6250

Also related: #4769 (multiple reporters of the same observable symptom requiring netbird down && netbird up on the routing peer).

Stack

This PR is intentionally standalone against main. The broader p2p-dynamic rework in #6084 contains a keepWgPeer commit that addresses the same code path as part of a 4-PR stack. This PR extracts the equivalent minimum change so the fix can land ahead of that full stack. If #6084 lands first, this PR can be dropped.

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)
  • Extended the README / documentation, if necessary

Tests

New regression file client/internal/peer/conn_close_keepwgpeer_test.go:

  • TestConn_Close_KeepWgPeerSignature — uses reflection on (*Conn).Close to pin the second parameter as bool. Survives parameter renaming as long as the shape stays.
  • TestConn_Close_KeepWgPeerGate — textual landmark check that the RemoveWgPeer call remains gated by keepWgPeer within the close body. Catches accidental removal of the gate.

Both pass with this change; both fail on main. The existing client/internal/peer/... and client/internal/lazyconn/... suites continue to pass with no other changes.

$ go test ./client/internal/peer/
ok  	github.com/netbirdio/netbird/client/internal/peer	6.551s

$ go test ./client/internal/lazyconn/...
ok  	github.com/netbirdio/netbird/client/internal/lazyconn	0.253s
ok  	github.com/netbirdio/netbird/client/internal/lazyconn/activity	1.808s
ok  	github.com/netbirdio/netbird/client/internal/lazyconn/inactivity	1.505s

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

Summary by CodeRabbit

  • Bug Fixes

    • Improved WireGuard peer state preservation during connection management transitions, ensuring route configuration survives across reconnection cycles and lazy-suspend operations.
  • Tests

    • Added regression tests for peer connection closure behavior.

Review Change Stack

…lowedIPs survive

Pre-existing lazy-connection bug: when the lazy-manager deactivated a
peer that was also a routing peer (advertises subnets via NetBird
Networks), peer.Conn.Close called endpointUpdater.RemoveWgPeer
unconditionally, wiping the entire WG peer entry, including any
AllowedIPs the route-manager had appended in place via
WgInterface.AddAllowedIP through its allowedIPsRefCounter.

The next lazy-wake re-opened the connection with only the basic
peer-IP /32 from the original PeerConfig. The route-manager's
refcounter was unaware of the round-trip and did not re-apply the
routed prefixes, so traffic to those prefixes was silently dropped by
WG until the next mgmt-side reconcile re-attached them. Users observe
this as: the routing peer shows Connected with a fresh WireGuard
handshake but no Networks in the status output, and traffic to its
advertised subnets times out until the network is manually
re-selected in the GUI.

Fix: add a keepWgPeer bool parameter to peer.Conn.Close. Lazy-suspend
callers (peerstore.PeerConnIdle, peerstore.PeerConnClose, and the
addNewPeer race-loser cleanup in engine) pass true so the WG peer
entry stays in place across the wake/sleep cycle. The permanent-
removal caller (conn_mgr.RemovePeerConn) passes false so the peer is
fully dropped from the WG iface when it leaves the network.

Tests: client/internal/peer/conn_close_keepwgpeer_test.go pins the
new signature via reflection and gates the RemoveWgPeer call with a
textual landmark check. Both pass with this change and fail on main.

Related: netbirdio#4769. The broader p2p-dynamic rework in PR netbirdio#6084 addresses
the same bug as part of its keepWgPeer commit; this PR extracts the
equivalent minimal change for main so the fix can land ahead of the
full stack.
@CLAassistant

CLAassistant commented May 22, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 53e9b89d-f929-415d-b8ba-7fb2e8cd0fe1

📥 Commits

Reviewing files that changed from the base of the PR and between 0358be2 and 090f196.

📒 Files selected for processing (5)
  • client/internal/conn_mgr.go
  • client/internal/engine.go
  • client/internal/peer/conn.go
  • client/internal/peer/conn_close_keepwgpeer_test.go
  • client/internal/peerstore/store.go

📝 Walkthrough

Walkthrough

The PR introduces a keepWgPeer parameter to the Conn.Close() method to conditionally preserve WireGuard peer state during connection lifecycle events. Callers now specify whether to remove or retain the peer endpoint: permanent removal uses false, while race-condition cleanup and lazy-suspend cycles use true to maintain route-manager-appended AllowedIPs.

Changes

WireGuard Peer Lifecycle Management

Layer / File(s) Summary
Conn.Close signature and removal logic
client/internal/peer/conn.go
Conn.Close(signalToRemote bool)Conn.Close(signalToRemote, keepWgPeer bool). When keepWgPeer=true, skips endpointUpdater.RemoveWgPeer() to preserve AllowedIPs; when false, removes the peer entry. Logging updated to report preservation status.
Race-condition cleanup in peer addition
client/internal/engine.go
addNewPeer race-loser path closes rejected connections with keepWgPeer=true, preserving the winner's WireGuard peer state and avoiding teardown of already-registered route-manager entries.
Permanent peer removal
client/internal/conn_mgr.go
ConnMgr.RemovePeerConn calls Close(false, false) for permanent removal, ensuring the WG peer entry is dropped and AllowedIPs refcount teardown is triggered.
Lazy-suspend lifecycle preservation
client/internal/peerstore/store.go
PeerConnIdle and PeerConnClose invoke Conn.Close with keepWgPeer=true to retain WG peer entries across idle/wake-sleep cycles, preserving route-manager-appended AllowedIPs.
Regression prevention tests
client/internal/peer/conn_close_keepwgpeer_test.go
Reflection-based signature test and source-text verification test guard against silent removal of the keepWgPeer parameter or unconditional peer removal.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • lixmal

Poem

🐰 A peer's journey now has choice—
Keep or drop with steady voice!
When races clash, we let one stay,
While lazy sleeps through night and day,
AllowedIPs dance, preserved with care.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding a mechanism to preserve WireGuard peer entries and their AllowedIPs across lazy-suspend cycles.
Description check ✅ Passed The description provides comprehensive details on the changes, includes the issue ticket (#6250), explains caller intent with a clear table, describes the visible symptom, documents tests, and confirms CLA agreement.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@sonarqubecloud

Copy link
Copy Markdown

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.

[client] Lazy-suspend wipes route-manager AllowedIPs; routed-subnet traffic blackholes until manual route resync

2 participants