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

Open
opened 2026-08-05 06:09:39 -04:00 by saavagebueno · 0 comments
Owner

Original Pull Request: https://github.com/netbirdio/netbird/pull/6251

State: open
Merged: No


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.

#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

**Original Pull Request:** https://github.com/netbirdio/netbird/pull/6251 **State:** open **Merged:** No --- ## 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 - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] 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](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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_stack_entry_start --> [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/netbirdio/netbird/pull/6251?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
saavagebueno added the pull-request label 2026-08-05 06:09:39 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#25187