mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-06 13:39:07 +02:00
[client] stop offering to every peer when the relay transport drops (#7092)
* [client] stop offering to every peer when the relay transport drops The relay transport is shared: one connection per relay server carries the streams of every peer using it. When it drops, each of those peers gets a Disconnected verdict from evalConnStatus even when ICE is still carrying its traffic, because peerUsesRelay comes from HasRelayAddress(), which only reports that management offered relay servers, not that we are connected to one. The guard answers Disconnected with the aggressive retry, so every peer starts sending offers over signal for a transport that no offer can restore: the relay client's own guard is what reconnects it. Feed relayManager.Ready() into the status inputs and return PartiallyConnected when ICE is up and the missing side is the shared transport. That is the existing "one path works, the other does not" branch, which retries three times and then hourly instead of walking the exponential ladder forever. Peers are not left waiting for the hourly tick: when the transport comes back, Manager.onServerConnected notifies srWatcher, the guard resets the ticker to 800ms and iceState.reset() clears the hourly mode. The verdict is unchanged when the transport is up but this peer is unreachable over relay - it may have moved to another server, and only an offer carries its new relay address - and in force-relay mode, where relay is the only transport. * Renaming according to actual meanings * Don't consider an in progress ICE as "partially connected" when the relay is not.. * Aligns tests * Address wrong comments
This commit is contained in:
@@ -828,7 +828,8 @@ func (conn *Conn) evalStatus() ConnStatus {
|
||||
//
|
||||
// The result is a tri-state:
|
||||
// - ConnStatusConnected: all available transports are up
|
||||
// - ConnStatusPartiallyConnected: relay is up but ICE is still pending/reconnecting
|
||||
// - ConnStatusPartiallyConnected: one transport carries the traffic and the other does
|
||||
// not: relay up with ICE down, or ICE up with the shared relay transport down
|
||||
// - ConnStatusDisconnected: no working transport
|
||||
func (conn *Conn) isConnectedOnAllWay() (status guard.ConnStatus) {
|
||||
defer func() {
|
||||
@@ -845,13 +846,14 @@ func (conn *Conn) isConnectedOnAllWay() (status guard.ConnStatus) {
|
||||
}
|
||||
|
||||
return evalConnStatus(connStatusInputs{
|
||||
forceRelay: IsForceRelayed(),
|
||||
peerUsesRelay: conn.workerRelay.IsRelayConnectionSupportedWithPeer(),
|
||||
relayConnected: conn.statusRelay.Get() == worker.StatusConnected,
|
||||
remoteSupportsICE: conn.handshaker.RemoteICESupported(),
|
||||
iceWorkerCreated: iceWorkerCreated,
|
||||
iceStatusConnecting: conn.statusICE.Get() != worker.StatusDisconnected,
|
||||
iceInProgress: iceInProgress,
|
||||
forceRelay: IsForceRelayed(),
|
||||
peerUsesRelay: conn.workerRelay.IsRelayConnectionSupportedWithPeer(),
|
||||
relayConnected: conn.statusRelay.Get() == worker.StatusConnected,
|
||||
relayTransportConnected: conn.workerRelay.IsTransportConnected(),
|
||||
remoteSupportsICE: conn.handshaker.RemoteICESupported(),
|
||||
iceWorkerCreated: iceWorkerCreated,
|
||||
iceStatusConnected: conn.statusICE.Get() == worker.StatusConnected,
|
||||
iceInProgress: iceInProgress,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -1060,19 +1062,21 @@ func evalConnStatus(in connStatusInputs) guard.ConnStatus {
|
||||
return boolToConnStatus(relayUsedAndUp)
|
||||
}
|
||||
|
||||
// ICE counts as "up" when the status is anything other than Disconnected, OR
|
||||
// when a negotiation is currently in progress (so we don't spam offers while one is in flight).
|
||||
iceUp := in.iceStatusConnecting || in.iceInProgress
|
||||
// ICE counts as "running" when either connected or attempting to connect.
|
||||
iceRunning := in.iceStatusConnected || in.iceInProgress
|
||||
|
||||
// Relay side is acceptable if the peer doesn't rely on relay, or relay is connected.
|
||||
relayOK := !in.peerUsesRelay || in.relayConnected
|
||||
|
||||
switch {
|
||||
case iceUp && relayOK:
|
||||
case iceRunning && relayOK:
|
||||
return guard.ConnStatusConnected
|
||||
case relayUsedAndUp:
|
||||
// Relay is up but ICE is down — partially connected.
|
||||
return guard.ConnStatusPartiallyConnected
|
||||
case in.iceStatusConnected && !in.relayTransportConnected:
|
||||
// ICE is up and the shared relay transport is down — offers cannot restore it.
|
||||
return guard.ConnStatusPartiallyConnected
|
||||
default:
|
||||
return guard.ConnStatusDisconnected
|
||||
}
|
||||
|
||||
@@ -17,13 +17,14 @@ const (
|
||||
// tri-state connection classification. Extracted so the decision logic can be unit-tested
|
||||
// without constructing full Worker/Handshaker objects.
|
||||
type connStatusInputs struct {
|
||||
forceRelay bool // NB_FORCE_RELAY or JS/WASM
|
||||
peerUsesRelay bool // remote peer advertises relay support AND local has relay
|
||||
relayConnected bool // statusRelay reports Connected (independent of whether peer uses relay)
|
||||
remoteSupportsICE bool // remote peer sent ICE credentials
|
||||
iceWorkerCreated bool // local WorkerICE exists (false in force-relay mode)
|
||||
iceStatusConnecting bool // statusICE is anything other than Disconnected
|
||||
iceInProgress bool // a negotiation is currently in flight
|
||||
forceRelay bool // NB_FORCE_RELAY or JS/WASM
|
||||
peerUsesRelay bool // remote peer advertises relay support AND local has relay
|
||||
relayConnected bool // statusRelay reports Connected (independent of whether peer uses relay)
|
||||
relayTransportConnected bool // the relay transport shared by all peers on that server is up
|
||||
remoteSupportsICE bool // remote peer sent ICE credentials
|
||||
iceWorkerCreated bool // local WorkerICE exists (false in force-relay mode)
|
||||
iceStatusConnected bool // statusICE reports Connected
|
||||
iceInProgress bool // a negotiation is currently in flight
|
||||
}
|
||||
|
||||
// ConnStatus describe the status of a peer's connection
|
||||
|
||||
@@ -30,6 +30,21 @@ func TestEvalConnStatus_ForceRelay(t *testing.T) {
|
||||
},
|
||||
want: guard.ConnStatusDisconnected,
|
||||
},
|
||||
{
|
||||
name: "force relay, relay up but the shared transport reports down",
|
||||
in: connStatusInputs{
|
||||
forceRelay: true,
|
||||
peerUsesRelay: true,
|
||||
relayConnected: true,
|
||||
relayTransportConnected: false,
|
||||
// The ICE inputs are set so that the force-relay return is the only branch
|
||||
// that can produce Connected here: without it the peer would fall through to
|
||||
// relayUsedAndUp and report PartiallyConnected.
|
||||
remoteSupportsICE: true,
|
||||
iceWorkerCreated: true,
|
||||
},
|
||||
want: guard.ConnStatusConnected,
|
||||
},
|
||||
{
|
||||
name: "force relay, peer does NOT use relay - disconnected forever",
|
||||
in: connStatusInputs{
|
||||
@@ -123,24 +138,28 @@ func TestEvalConnStatus_FullyAvailable(t *testing.T) {
|
||||
mutator: func(in *connStatusInputs) {
|
||||
in.peerUsesRelay = true
|
||||
in.relayConnected = true
|
||||
in.iceStatusConnecting = true
|
||||
in.relayTransportConnected = true
|
||||
in.iceStatusConnected = true
|
||||
},
|
||||
want: guard.ConnStatusConnected,
|
||||
},
|
||||
{
|
||||
name: "ICE connected, peer does NOT use relay",
|
||||
name: "ICE connected, peer does NOT use relay, shared transport down",
|
||||
mutator: func(in *connStatusInputs) {
|
||||
in.peerUsesRelay = false
|
||||
in.relayConnected = false
|
||||
in.iceStatusConnecting = true
|
||||
in.relayTransportConnected = false
|
||||
in.iceStatusConnected = true
|
||||
},
|
||||
// A peer that does not rely on relay is unaffected by the shared transport:
|
||||
// relayOK is true, so the first arm matches before the transport is considered.
|
||||
want: guard.ConnStatusConnected,
|
||||
},
|
||||
{
|
||||
name: "ICE InProgress only, peer does NOT use relay",
|
||||
mutator: func(in *connStatusInputs) {
|
||||
in.peerUsesRelay = false
|
||||
in.iceStatusConnecting = false
|
||||
in.iceStatusConnected = false
|
||||
in.iceInProgress = true
|
||||
},
|
||||
want: guard.ConnStatusConnected,
|
||||
@@ -150,7 +169,8 @@ func TestEvalConnStatus_FullyAvailable(t *testing.T) {
|
||||
mutator: func(in *connStatusInputs) {
|
||||
in.peerUsesRelay = true
|
||||
in.relayConnected = true
|
||||
in.iceStatusConnecting = false
|
||||
in.relayTransportConnected = true
|
||||
in.iceStatusConnected = false
|
||||
in.iceInProgress = false
|
||||
},
|
||||
want: guard.ConnStatusPartiallyConnected,
|
||||
@@ -160,21 +180,60 @@ func TestEvalConnStatus_FullyAvailable(t *testing.T) {
|
||||
mutator: func(in *connStatusInputs) {
|
||||
in.peerUsesRelay = false
|
||||
in.relayConnected = false
|
||||
in.iceStatusConnecting = false
|
||||
in.iceStatusConnected = false
|
||||
in.iceInProgress = false
|
||||
},
|
||||
want: guard.ConnStatusDisconnected,
|
||||
},
|
||||
{
|
||||
name: "ICE up, peer uses relay but relay down -> partial (relay required, ICE ignored)",
|
||||
name: "ICE connected, relay down for this peer but the shared transport is up -> disconnected",
|
||||
mutator: func(in *connStatusInputs) {
|
||||
in.peerUsesRelay = true
|
||||
in.relayConnected = false
|
||||
in.iceStatusConnecting = true
|
||||
in.relayTransportConnected = true
|
||||
in.iceStatusConnected = true
|
||||
},
|
||||
// The transport is fine, so the peer itself is unreachable over relay: it may have
|
||||
// moved to another server, and only an offer carries its new relay address.
|
||||
want: guard.ConnStatusDisconnected,
|
||||
},
|
||||
{
|
||||
name: "ICE connected, the shared relay transport is down -> partial",
|
||||
mutator: func(in *connStatusInputs) {
|
||||
in.peerUsesRelay = true
|
||||
in.relayConnected = false
|
||||
in.relayTransportConnected = false
|
||||
in.iceStatusConnected = true
|
||||
},
|
||||
// ICE carries the traffic and the relay transport is restored by the relay client's
|
||||
// own guard, not by offers, so this must not trigger the aggressive retry.
|
||||
want: guard.ConnStatusPartiallyConnected,
|
||||
},
|
||||
{
|
||||
name: "ICE only negotiating while the shared relay transport is down -> disconnected",
|
||||
mutator: func(in *connStatusInputs) {
|
||||
in.peerUsesRelay = true
|
||||
in.relayConnected = false
|
||||
in.relayTransportConnected = false
|
||||
in.iceStatusConnected = false
|
||||
in.iceInProgress = true
|
||||
},
|
||||
// A negotiation in flight is not a working transport, so this peer has no path at
|
||||
// all and must keep the aggressive retry. Calling it partially connected spends the
|
||||
// ICE retry budget and parks the guard on the hourly ticker, and nothing wakes it
|
||||
// when the negotiation then fails: onICEStateDisconnected is only reached once ICE
|
||||
// has reached Connected (worker_ice.go onConnectionStateChange).
|
||||
want: guard.ConnStatusDisconnected,
|
||||
},
|
||||
{
|
||||
name: "ICE down and the shared relay transport is down -> disconnected",
|
||||
mutator: func(in *connStatusInputs) {
|
||||
in.peerUsesRelay = true
|
||||
in.relayConnected = false
|
||||
in.relayTransportConnected = false
|
||||
in.iceStatusConnected = false
|
||||
in.iceInProgress = false
|
||||
},
|
||||
// relayOK = false (peer uses relay but it's down), iceUp = true
|
||||
// first switch arm fails (relayOK false), relayUsedAndUp = false (relay down),
|
||||
// falls into default: Disconnected.
|
||||
want: guard.ConnStatusDisconnected,
|
||||
},
|
||||
{
|
||||
@@ -182,7 +241,7 @@ func TestEvalConnStatus_FullyAvailable(t *testing.T) {
|
||||
mutator: func(in *connStatusInputs) {
|
||||
in.peerUsesRelay = false
|
||||
in.relayConnected = true // not actually used since peer doesn't rely on it
|
||||
in.iceStatusConnecting = false
|
||||
in.iceStatusConnected = false
|
||||
in.iceInProgress = false
|
||||
},
|
||||
want: guard.ConnStatusDisconnected,
|
||||
|
||||
@@ -14,7 +14,8 @@ type ConnStatus int
|
||||
const (
|
||||
// ConnStatusDisconnected means neither ICE nor Relay is connected.
|
||||
ConnStatusDisconnected ConnStatus = iota
|
||||
// ConnStatusPartiallyConnected means Relay is connected but ICE is not.
|
||||
// ConnStatusPartiallyConnected means one transport is usable and the other is not:
|
||||
// relay connected with ICE down, or ICE connected with the shared relay transport down.
|
||||
ConnStatusPartiallyConnected
|
||||
// ConnStatusConnected means all required connections are established.
|
||||
ConnStatusConnected
|
||||
@@ -87,8 +88,9 @@ func (g *Guard) SetICEConnDisconnected() {
|
||||
// - Connected: no action, the peer is fully reachable.
|
||||
// - Disconnected (neither ICE nor Relay): retries aggressively with exponential backoff (800ms doubling
|
||||
// up to timeout), never gives up. This ensures rapid recovery when the peer has no connectivity at all.
|
||||
// - PartiallyConnected (Relay up, ICE not): retries up to 3 times with exponential backoff, then switches
|
||||
// to one attempt per hour. This limits signaling traffic when relay already provides connectivity.
|
||||
// - PartiallyConnected (one transport usable, the other not): retries up to 3 times
|
||||
// with exponential backoff, then switches to one attempt per hour. This limits
|
||||
// signaling traffic while the peer still has a working path.
|
||||
//
|
||||
// External events (relay/ICE disconnect, signal/relay reconnect, candidate changes) reset the retry
|
||||
// counter and backoff ticker, giving ICE a fresh chance after network conditions change.
|
||||
|
||||
@@ -101,6 +101,10 @@ func (w *WorkerRelay) RelayIsSupportedLocally() bool {
|
||||
return w.relayManager.HasRelayAddress()
|
||||
}
|
||||
|
||||
func (w *WorkerRelay) IsTransportConnected() bool {
|
||||
return w.relayManager.Ready()
|
||||
}
|
||||
|
||||
func (w *WorkerRelay) CloseConn() {
|
||||
w.relayLock.Lock()
|
||||
conn := w.relayedConn
|
||||
|
||||
Reference in New Issue
Block a user