DYNR/netbird
2
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2026-08-07 02:29:06 -04:00
Code Issues 23.5k Packages Projects Releases 10 Wiki Activity

[PR #5866] fix: close stale relay connection on ErrConnAlreadyExists to recover … #28666

New Issue
Open
opened 2026-08-05 08:06:43 -04:00 by saavagebueno · 0 comments
saavagebueno commented 2026-08-05 08:06:43 -04:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/5866
Author: @fpenezic
Created: 4/12/2026
Status: 🔄 Open

Base: main ← Head: fix/stale-relay-conn-recovery

📝 Commits (6)

  • 46a5a7d fix: close stale relay connection on ErrConnAlreadyExists to recover tunnel after NAT IP change
  • 0f1fc4f fix: route CloseConnByPeerKey to correct relay client for foreign servers
  • 159183b docs: explain Guard invariant for ErrConnAlreadyExists branch
  • 691331a fix: gate ErrConnAlreadyExists close-and-retry behind stale flag
  • c0b3e4e fix: rotate ICE session ID on WG timeout in ICE mode
  • 8c4ba8b fix: protect RelayTrack.relayClient read with rt lock in CloseConnByPeerKey

📊 Changes

5 files changed (+137 additions, -3 deletions)

View changed files

📝 client/internal/peer/conn.go (+22 -0)
📝 client/internal/peer/worker_ice.go (+18 -0)
📝 client/internal/peer/worker_relay.go (+39 -3)
📝 shared/relay/client/client.go (+21 -0)
📝 shared/relay/client/manager.go (+37 -0)

📄 Description

Describe your changes

Fix peer reconnection loops after a network event (PPPoE reconnect, NAT/conntrack flush, IP rotation) leaves transport state stale on either the relay or ICE path. Both manifest as the WireGuard tunnel going silent while the client UI reports Connected, with the connection never recovering on its own.

This PR addresses two distinct but related failure modes observed in production on a peer behind PPPoE NAT (Raspberry Pi) talking to a peer with a public IP (Oracle Cloud).

Problem 1 - Relay path: stale ErrConnAlreadyExists reuse

When the peer is running over relay and a network event invalidates the existing relay session, WorkerRelay.OnNewOffer calls relayManager.OpenConn which returns ErrConnAlreadyExists because the relay client still holds a map entry for the peer key. The previous behavior was to silently bail out and reuse that entry - which is dead.

Fix (commits 1–4, 6):

  • On ErrConnAlreadyExists, close the existing relay conn and reopen it.
  • Route CloseConnByPeerKey to the correct relay client (home vs. foreign server).
  • Take rt.RLock() when reading RelayTrack.relayClient to avoid a race with openConnVia() which initializes that field under rt.Lock().
  • Gate the close-and-retry behind a relayConnStale atomic.Bool flag so we only tear down when something has signaled that the entry is no longer backed by a live peer session. Without this gate, rapid successive offers from the remote peer cause an infinite tear-down/rebuild loop (observed: 3608 cycles in ~1 hour, ~9 Mbit/s of constant traffic).
  • Signal sources for the stale flag: conn.onWGDisconnected (Relay path), WorkerRelay.CloseConn, WorkerRelay.onRelayClientDisconnected.

Problem 2 - ICE path: session ID not rotated on WG timeout

When WireGuard handshake times out while running over ICE, onWGDisconnected calls workerICE.Close(). Close() sets w.agent = nil synchronously before pion's ICE library fires the asynchronous ConnectionStateClosed event. By the time onConnectionStateChange runs closeAgent(), the w.agent == agent guard fails (w.agent is already nil) and the session ID is not rotated.

Without rotation, the next ICE offer carries the same local session ID. The remote peer in OnNewOffer compares remoteSessionID against the incoming offer's SessionID, finds them equal, and skips agent recreation - reusing the existing agent and its stale candidates from the broken network state.

Observed in production: 30s reconnect loop with the same offer session ID logged 70+ times in a row, "ICE connection succeeded" on every cycle, WG handshake never recovering. UI shows Connected, P2P while last successful WG handshake is 40+ minutes old.

Fix (commit 5): Rotate the session ID explicitly in onWGDisconnected for the ICE case (mirroring the existing Relay-path behavior we added in commit 1) so the remote peer always recreates its ICE agent after a WG timeout on ICE.

Test plan

  • Local build passes (go build ./...)
  • Deployed on two production peers (RPi behind PPPoE NAT, OCI public IP) for live testing
  • Verified stable P2P connection with WG handshake refreshing on the normal 3-minute cadence (no regression)
  • Verified no infinite tear-down loop on relay-active peer (the bug introduced by the v1 fix and resolved in v2)
  • Validated against multiple real PPPoE reconnects with public IP rotation: tunnel recovered automatically in a single ICE → Relay → ICE transition (~4 seconds), no loop, WG handshake refreshed normally on the 3-minute cadence afterwards. Stable for hours post-event.

Caveat: unrelated Rosenpass interaction

During testing we hit a separate failure mode where Rosenpass (post-quantum PSK) was enabled in permissive mode on both peers. After an IP change, the WG tunnel could not recover because the local PSK state diverged from the remote peer's PSK (one side had a Rosenpass-managed PSK, the other had none). This produced the same external symptom (silent tunnel, UI says Connected) but is not addressed by this PR - the fixes here only cover the relay/ICE transport state. With Rosenpass disabled, both fixes recovered the tunnel cleanly across multiple PPPoE cycles. Filing the Rosenpass interaction as a separate issue.

Notes

The two problems share the same trigger (network event invalidates per-peer state without invalidating the local control plane) but live in different transports, so the fixes are orthogonal. Each is necessary on its own:

  • Without the relay fix, peers stuck on relay loop the relay close/reopen cycle.
  • Without the ICE fix, peers that fall back to relay then re-establish ICE get pinned to a stale ICE agent on the remote side.

Both fixes are strictly defensive - they can only fire on ErrConnAlreadyExists (relay) or after a WG handshake timeout (ICE), so there's no behavior change on the happy path.

Issue ticket number and link

N/A - bug discovered in production, no pre-existing issue.

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)

This is an internal recovery-path fix in the peer connection state machine. No public API, CLI flag, config field, or user-visible behavior changes - the fixes are strictly defensive and only fire on specific error conditions (ErrConnAlreadyExists for relay, WG handshake timeout for ICE). Nothing to document for end users or operators.

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Improved connection timeout handling to better recover from WireGuard handshake and ICE/TURN failures by resetting session negotiation and retrying stale relay connections.
    • Enhanced peer connection stability through more robust relay connection cleanup and retry logic during connection collisions.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/netbirdio/netbird/pull/5866 **Author:** [@fpenezic](https://github.com/fpenezic) **Created:** 4/12/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/stale-relay-conn-recovery` --- ### 📝 Commits (6) - [`46a5a7d`](https://github.com/netbirdio/netbird/commit/46a5a7d1ebdef37f572b915b0ffc5715f80060bb) fix: close stale relay connection on ErrConnAlreadyExists to recover tunnel after NAT IP change - [`0f1fc4f`](https://github.com/netbirdio/netbird/commit/0f1fc4f4beebaedc0ff5a17be62ca2411418e075) fix: route CloseConnByPeerKey to correct relay client for foreign servers - [`159183b`](https://github.com/netbirdio/netbird/commit/159183b55d58bf78d704fbfcfa5564e9ddaef071) docs: explain Guard invariant for ErrConnAlreadyExists branch - [`691331a`](https://github.com/netbirdio/netbird/commit/691331ade8564a9b922eb59fd912934af8cd8f33) fix: gate ErrConnAlreadyExists close-and-retry behind stale flag - [`c0b3e4e`](https://github.com/netbirdio/netbird/commit/c0b3e4ed3d59a0f8e10aeb4cb4fdf84a4acd1a14) fix: rotate ICE session ID on WG timeout in ICE mode - [`8c4ba8b`](https://github.com/netbirdio/netbird/commit/8c4ba8b91eb7376ab17bb26cd9cff4b31ce5a25c) fix: protect RelayTrack.relayClient read with rt lock in CloseConnByPeerKey ### 📊 Changes **5 files changed** (+137 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `client/internal/peer/conn.go` (+22 -0) 📝 `client/internal/peer/worker_ice.go` (+18 -0) 📝 `client/internal/peer/worker_relay.go` (+39 -3) 📝 `shared/relay/client/client.go` (+21 -0) 📝 `shared/relay/client/manager.go` (+37 -0) </details> ### 📄 Description ## Describe your changes Fix peer reconnection loops after a network event (PPPoE reconnect, NAT/conntrack flush, IP rotation) leaves transport state stale on either the relay or ICE path. Both manifest as the WireGuard tunnel going silent while the client UI reports `Connected`, with the connection never recovering on its own. This PR addresses two distinct but related failure modes observed in production on a peer behind PPPoE NAT (Raspberry Pi) talking to a peer with a public IP (Oracle Cloud). ### Problem 1 - Relay path: stale `ErrConnAlreadyExists` reuse When the peer is running over relay and a network event invalidates the existing relay session, `WorkerRelay.OnNewOffer` calls `relayManager.OpenConn` which returns `ErrConnAlreadyExists` because the relay client still holds a map entry for the peer key. The previous behavior was to silently bail out and reuse that entry - which is dead. **Fix (commits 1–4, 6):** - On `ErrConnAlreadyExists`, close the existing relay conn and reopen it. - Route `CloseConnByPeerKey` to the correct relay client (home vs. foreign server). - Take `rt.RLock()` when reading `RelayTrack.relayClient` to avoid a race with `openConnVia()` which initializes that field under `rt.Lock()`. - Gate the close-and-retry behind a `relayConnStale atomic.Bool` flag so we only tear down when something has signaled that the entry is no longer backed by a live peer session. Without this gate, rapid successive offers from the remote peer cause an infinite tear-down/rebuild loop (observed: 3608 cycles in ~1 hour, ~9 Mbit/s of constant traffic). - Signal sources for the stale flag: `conn.onWGDisconnected` (Relay path), `WorkerRelay.CloseConn`, `WorkerRelay.onRelayClientDisconnected`. ### Problem 2 - ICE path: session ID not rotated on WG timeout When WireGuard handshake times out while running over ICE, `onWGDisconnected` calls `workerICE.Close()`. `Close()` sets `w.agent = nil` synchronously **before** pion's ICE library fires the asynchronous `ConnectionStateClosed` event. By the time `onConnectionStateChange` runs `closeAgent()`, the `w.agent == agent` guard fails (`w.agent` is already `nil`) and the session ID is **not rotated**. Without rotation, the next ICE offer carries the same local session ID. The remote peer in `OnNewOffer` compares `remoteSessionID` against the incoming offer's `SessionID`, finds them equal, and **skips agent recreation** - reusing the existing agent and its stale candidates from the broken network state. **Observed in production:** 30s reconnect loop with the same offer session ID logged 70+ times in a row, "ICE connection succeeded" on every cycle, WG handshake never recovering. UI shows `Connected, P2P` while last successful WG handshake is 40+ minutes old. **Fix (commit 5):** Rotate the session ID explicitly in `onWGDisconnected` for the ICE case (mirroring the existing Relay-path behavior we added in commit 1) so the remote peer always recreates its ICE agent after a WG timeout on ICE. ### Test plan - [x] Local build passes (`go build ./...`) - [x] Deployed on two production peers (RPi behind PPPoE NAT, OCI public IP) for live testing - [x] Verified stable P2P connection with WG handshake refreshing on the normal 3-minute cadence (no regression) - [x] Verified no infinite tear-down loop on relay-active peer (the bug introduced by the v1 fix and resolved in v2) - [x] **Validated against multiple real PPPoE reconnects with public IP rotation**: tunnel recovered automatically in a single `ICE → Relay → ICE` transition (~4 seconds), no loop, WG handshake refreshed normally on the 3-minute cadence afterwards. Stable for hours post-event. ### Caveat: unrelated Rosenpass interaction During testing we hit a separate failure mode where Rosenpass (post-quantum PSK) was enabled in permissive mode on both peers. After an IP change, the WG tunnel could not recover because the local PSK state diverged from the remote peer's PSK (one side had a Rosenpass-managed PSK, the other had none). This produced the same external symptom (silent tunnel, UI says `Connected`) but is **not addressed by this PR** - the fixes here only cover the relay/ICE transport state. With Rosenpass disabled, both fixes recovered the tunnel cleanly across multiple PPPoE cycles. Filing the Rosenpass interaction as a separate issue. ### Notes The two problems share the same trigger (network event invalidates per-peer state without invalidating the local control plane) but live in different transports, so the fixes are orthogonal. Each is necessary on its own: - Without the relay fix, peers stuck on relay loop the relay close/reopen cycle. - Without the ICE fix, peers that fall back to relay then re-establish ICE get pinned to a stale ICE agent on the remote side. Both fixes are strictly defensive - they can only fire on `ErrConnAlreadyExists` (relay) or after a WG handshake timeout (ICE), so there's no behavior change on the happy path. ## Issue ticket number and link N/A - bug discovered in production, no pre-existing issue. ## Stack <!-- branch-stack --> ### Checklist - [x] 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](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) This is an internal recovery-path fix in the peer connection state machine. No public API, CLI flag, config field, or user-visible behavior changes - the fixes are strictly defensive and only fire on specific error conditions (`ErrConnAlreadyExists` for relay, WG handshake timeout for ICE). Nothing to document for end users or operators. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Improved connection timeout handling to better recover from WireGuard handshake and ICE/TURN failures by resetting session negotiation and retrying stale relay connections. * Enhanced peer connection stability through more robust relay connection cleanup and retry logic during connection collisions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 08:06:43 -04:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
2021 Q4
2021 Q4
2021 Q4
2022 Q1
2022 Q1
2022 Q1
2022 Q1
2022 Q1
2022 Q1
accessibility
accessibility
accessibility
acl
acl
acl
agent
agent
agent
agent
agent
agent
Android
Android
Android
Android
Android
Android
api
api
api
authentik
authentik
authentik
automation
automation
automation
azure
azure
azure
battery-usage
battery-usage
battery-usage
bug
cache
cache
cache
client
client
client
client-ui
client-ui
client-ui
cloud
cloud
cloud
cloud-only
cloud-only
cloud-only
cloudflare
cloudflare
cloudflare
community
community
community
compatibility
compatibility
compatibility
config-idp
config-idp
config-idp
config-issue
config-issue
config-issue
connection
connection
connection
contribution
contribution
contribution
coturn
coturn
coturn
cross-vpn
cross-vpn
cross-vpn
dashboard
dashboard
dashboard
data-usage
data-usage
data-usage
distribution
distribution
distribution
dns
dns
dns
docker
docker
docker
documentation
documentation
documentation
duplicate
duplicate
duplicate
enhancement
enhancement
event-stream
event-stream
event-stream
feature-request
feature-request
feature-request
freebsd
freebsd
freebsd
getting-started
getting-started
getting-started
go
go
go
good first issue
good first issue
good first issue
gui
gui
gui
help wanted
help wanted
help wanted
home-assistant
home-assistant
home-assistant
idp
idp
idp
inconsistency
inconsistency
inconsistency
integration
integration
integration
integrations
integrations
integrations
ios
ios
ios
ipv6
ipv6
ipv6
jwt
jwt
jwt
k8s
k8s
k8s
keycloak
keycloak
keycloak
linux
linux
linux
login
login
login
macos
macos
macos
management-service
management-service
management-service
Medium
Medium
Medium
missing-docs
missing-docs
missing-docs
mobile
mobile
mobile
moved-internal
moved-internal
moved-internal
needs-review
needs-review
needs-review
netbird-ui
netbird-ui
netbird-ui
networking
networking
networking
new-platform
new-platform
new-platform
nginx
nginx
nginx
notification
notification
notification
okta
okta
okta
openwrt
openwrt
openwrt
P2
P2
P2
packaging
packaging
packaging
peer-management
peer-management
peer-management
peer-management
peer-management
peer-management
peer-management
peer-management
peer-management
performance
performance
performance
postgres
postgres
postgres
posture-checks
posture-checks
posture-checks
psk
psk
psk
pull-request
Mirrored from GitHub Pull Request
 question
question
question
refactor
refactor
refactor
relay
relay
relay
release
release
release
rfc
rfc
rfc
routes
routes
routes
security
security
security
security-improvement
security-improvement
security-improvement
security-related
security-related
security-related
self-hosting
self-hosting
self-hosting
server
server
server
signal
signal
signal
sleep-issue
sleep-issue
sleep-issue
ssh
ssh
ssh
ssl
ssl
ssl
status
status
status
store
store
store
synology
synology
synology
system-compatibility-issue
system-compatibility-issue
system-compatibility-issue
test-suite
test-suite
test-suite
third-party-integration
third-party-integration
third-party-integration
triage
triage
triage
triage
triage
triage
triage-needed
triage-needed
triage-needed
troubleshooting
troubleshooting
troubleshooting
UX
UX
UX
waiting-feedback
waiting-feedback
waiting-feedback
windows
windows
windows
wontfix
wontfix
wontfix
zitadel
zitadel
zitadel
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
dynr-servers saavagebueno
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#28666
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?