diff --git a/client/internal/peer/conn.go b/client/internal/peer/conn.go index d73144773..17823e043 100644 --- a/client/internal/peer/conn.go +++ b/client/internal/peer/conn.go @@ -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 } diff --git a/client/internal/peer/conn_status.go b/client/internal/peer/conn_status.go index d6ad37b70..acf271534 100644 --- a/client/internal/peer/conn_status.go +++ b/client/internal/peer/conn_status.go @@ -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 diff --git a/client/internal/peer/conn_status_eval_test.go b/client/internal/peer/conn_status_eval_test.go index 66393cafe..a239196dc 100644 --- a/client/internal/peer/conn_status_eval_test.go +++ b/client/internal/peer/conn_status_eval_test.go @@ -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, diff --git a/client/internal/peer/guard/guard.go b/client/internal/peer/guard/guard.go index 73bab2a89..15028d91c 100644 --- a/client/internal/peer/guard/guard.go +++ b/client/internal/peer/guard/guard.go @@ -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. diff --git a/client/internal/peer/worker_relay.go b/client/internal/peer/worker_relay.go index fc3489992..694207847 100644 --- a/client/internal/peer/worker_relay.go +++ b/client/internal/peer/worker_relay.go @@ -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