[client] Merge main into peer event bus refactor

2026-09-21 22:29:08 +02:00 · 2026-09-10 01:48:41 +02:00
parent 28d3a478b8 27991aab98
commit c31f73cfbd
1102 changed files with 94422 additions and 19596 deletions
@@ -74,6 +74,9 @@ type ICE struct {

 	// portForwardAttempted tracks if we've already tried port forwarding this session
 	portForwardAttempted bool
+
+	// dialFunc, when non-nil, replaces agentDial in connect(). Only for tests.
+	dialFunc func(ctx context.Context, agent *icemaker.ThreadSafeAgent, remoteOfferAnswer *signaling.OfferAnswer) (net.Conn, error)
 }

 func NewICE(log *log.Entry, key string, iceConfig icemaker.Config, isController bool, onConnReady func(ConnPriority, ICEConnInfo), onStatusDisconnect func(bool), services ICEDependencies, hasRelayOnLocally bool) (*ICE, error) {
@@ -135,7 +138,7 @@ func (w *ICE) OnNewOffer(ctx context.Context, remoteOfferAnswer *signaling.Offer
 			w.log.Errorf("failed to create new session ID: %s", err)
 		}
 		w.sessionID = sessionID
-		w.agent = nil
+		w.abandonNegotiation()
 	}

 	var preferredCandidateTypes []ice.CandidateType
@@ -163,6 +166,8 @@ func (w *ICE) OnNewOffer(ctx context.Context, remoteOfferAnswer *signaling.Offer
 		w.remoteSessionID = ""
 	}

+	// Capture the cancel func at spawn time: connect reads it from the argument
+	// instead of the field, which a newer OnNewOffer may already have replaced.
 	go w.connect(dialerCtx, dialerCancel, agent, remoteOfferAnswer)
 }

@@ -218,16 +223,16 @@ func (w *ICE) Close() {
 	w.muxAgent.Lock()
 	defer w.muxAgent.Unlock()

-	if w.agent == nil {
-		return
+	if w.agent != nil {
+		w.agentDialerCancel()
+		if err := w.agent.Close(); err != nil {
+			w.log.Warnf("failed to close ICE agent: %s", err)
+		}
 	}
-
-	w.agentDialerCancel()
-	if err := w.agent.Close(); err != nil {
-		w.log.Warnf("failed to close ICE agent: %s", err)
-	}
-
-	w.agent = nil
+	// Unconditional: a dial goroutine racing this Close skips its own cleanup
+	// (closeAgent finds a nil agent), so the flags must be dropped here too or
+	// the reconnection guard reads the stale state as Connected forever.
+	w.abandonNegotiation()
 }

 func (w *ICE) reCreateAgent(ctx context.Context, dialerCancel context.CancelFunc, candidates []ice.CandidateType) (*icemaker.ThreadSafeAgent, error) {
@@ -273,8 +278,14 @@ func (w *ICE) connect(ctx context.Context, dialerCancel context.CancelFunc, agen
 		return
 	}

-	w.log.Debugf("turn agent dial")
-	remoteConn, err := w.turnAgentDial(ctx, agent, remoteOfferAnswer)
+	w.log.Debugf("agent dial")
+	dial := func(ctx context.Context, agent *icemaker.ThreadSafeAgent, remoteOfferAnswer *signaling.OfferAnswer) (net.Conn, error) {
+		return w.agentDial(ctx, agent, remoteOfferAnswer)
+	}
+	if w.dialFunc != nil {
+		dial = w.dialFunc
+	}
+	remoteConn, err := dial(ctx, agent, remoteOfferAnswer)
 	if err != nil {
 		w.log.Debugf("failed to dial the remote peer: %s", err)
 		w.closeAgent(agent, dialerCancel)
@@ -282,6 +293,19 @@ func (w *ICE) connect(ctx context.Context, dialerCancel context.CancelFunc, agen
 	}
 	w.log.Debugf("agent dial succeeded")

+	// A newer negotiation may have replaced this agent during the dial.
+	// Discard its connection before querying candidates or punching ports.
+	w.muxAgent.Lock()
+	stale := w.agent != agent
+	w.muxAgent.Unlock()
+	if stale {
+		if err := remoteConn.Close(); err != nil {
+			w.log.Warnf("failed to close stale ICE connection: %s", err)
+		}
+		w.log.Warnf("discarding connection from a stale ICE negotiation")
+		return
+	}
+
 	pair, err := agent.GetSelectedCandidatePair()
 	if err != nil {
 		w.closeAgent(agent, dialerCancel)
@@ -318,9 +342,14 @@ func (w *ICE) connect(ctx context.Context, dialerCancel context.CancelFunc, agen
 	w.log.Debugf("on ICE conn is ready to use")

 	w.muxAgent.Lock()
+	// Keep the ownership check atomic with the state update so a stale dial
+	// cannot overwrite a newer negotiation.
 	if w.agent != agent {
 		w.muxAgent.Unlock()
-		w.log.Debugf("agent has been replaced during connect, dropping obsolete connection")
+		if err := remoteConn.Close(); err != nil {
+			w.log.Warnf("failed to close stale ICE connection: %s", err)
+		}
+		w.log.Warnf("discarding connection from a stale ICE negotiation")
 		return
 	}
 	w.agentConnecting = false
@@ -344,20 +373,27 @@ func (w *ICE) closeAgent(agent *icemaker.ThreadSafeAgent, cancel context.CancelF
 	sessionChanged := w.remoteSessionChanged
 	w.remoteSessionChanged = false

+	// Only the owner of the current session may reset its state: a stale dial
+	// goroutine waking after a newer attempt must not clobber it.
 	if w.agent == agent {
-		// consider to remove from here and move to the OnNewOffer
 		sessionID, err := icemaker.NewSessionID()
 		if err != nil {
 			w.log.Errorf("failed to create new session ID: %s", err)
 		}
 		w.sessionID = sessionID
-		w.agent = nil
-		w.agentConnecting = false
-		w.remoteSessionID = ""
+		w.abandonNegotiation()
 	}
 	return sessionChanged
 }

+// Clearing the agent and connecting flag together keeps retries from stalling.
+// Callers must dispose of the agent first and hold muxAgent.
+func (w *ICE) abandonNegotiation() {
+	w.agent = nil
+	w.agentConnecting = false
+	w.remoteSessionID = ""
+}
+
 func (w *ICE) punchRemoteWGPort(pair *ice.CandidatePair, remoteWgPort int) {
 	// wait local endpoint configuration
 	time.Sleep(time.Second)
@@ -412,6 +448,17 @@ func (w *ICE) injectPortForwardedCandidate(srflxCandidate ice.Candidate) {
 		return
 	}

+	// A forwarded candidate only makes sense for an IPv4 mapping, which
+	// translates a port on the gateway's address. An IPv6 pinhole translates
+	// nothing: it unblocks the address ICE already gathers as a host candidate,
+	// so there is no second address to advertise. Injecting one here would also
+	// paste an IPv6 address onto whichever server-reflexive candidate arrived
+	// first, which is usually IPv4.
+	if mapping.ExternalIP != nil && mapping.ExternalIP.To4() == nil {
+		w.log.Debugf("skipping port-forwarded candidate: %s mapping is IPv6-only", mapping.NATType)
+		return
+	}
+
 	w.muxAgent.Lock()
 	if w.portForwardAttempted {
 		w.muxAgent.Unlock()
@@ -541,8 +588,8 @@ func (w *ICE) onConnectionStateChange(agent *icemaker.ThreadSafeAgent, dialerCan
 			connected = true
 			w.logSuccessfulPaths(agent)
 		case ice.ConnectionStateFailed, ice.ConnectionStateDisconnected, ice.ConnectionStateClosed:
-			// ice.ConnectionStateClosed happens when we recreate the agent. For the P2P to TURN switch important to
-			// notify the conn.onICEStateDisconnected changes to update the current used priority
+			// ice.ConnectionStateClosed happens when we recreate the agent. The P2P to relay switch requires
+			// notifying conn.onICEStateDisconnected so it can update the currently used priority.

 			sessionChanged := w.closeAgent(agent, dialerCancel)

@@ -567,7 +614,7 @@ func (w *ICE) onConnectionStateChange(agent *icemaker.ThreadSafeAgent, dialerCan
 	}
 }

-func (w *ICE) turnAgentDial(ctx context.Context, agent *icemaker.ThreadSafeAgent, remoteOfferAnswer *signaling.OfferAnswer) (*ice.Conn, error) {
+func (w *ICE) agentDial(ctx context.Context, agent *icemaker.ThreadSafeAgent, remoteOfferAnswer *signaling.OfferAnswer) (*ice.Conn, error) {
 	if w.isController {
 		return agent.Dial(ctx, remoteOfferAnswer.IceCredentials.UFrag, remoteOfferAnswer.IceCredentials.Pwd)
 	} else {
@@ -0,0 +1,258 @@
+package worker
+
+import (
+	"context"
+	"net"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	log "github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.zx2c4.com/wireguard/wgctrl/wgtypes"
+
+	icemaker "github.com/netbirdio/netbird/client/internal/peer/ice"
+	"github.com/netbirdio/netbird/client/internal/peer/signaling"
+	signal "github.com/netbirdio/netbird/shared/signal/client"
+	sProto "github.com/netbirdio/netbird/shared/signal/proto"
+)
+
+// stubSignalClient satisfies signal.Client as a no-op so the candidate
+// goroutine spawned by a real GatherCandidates never dereferences a nil
+// signaler in tests.
+type stubSignalClient struct{}
+
+func (stubSignalClient) Close() error                                               { return nil }
+func (stubSignalClient) StreamConnected() bool                                      { return false }
+func (stubSignalClient) GetStatus() signal.Status                                   { return signal.StreamDisconnected }
+func (stubSignalClient) Receive(context.Context, func(*sProto.Message) error) error { return nil }
+func (stubSignalClient) Ready() bool                                                { return false }
+func (stubSignalClient) IsHealthy() bool                                            { return false }
+func (stubSignalClient) WaitStreamConnected(context.Context)                        {}
+func (stubSignalClient) SendToStream(*sProto.EncryptedMessage) error                { return nil }
+func (stubSignalClient) Send(*sProto.Message) error                                 { return nil }
+func (stubSignalClient) SetOnReconnectedListener(func())                            {}
+
+// newTestWorkerICE builds a worker with real pion plumbing and no-op signaling.
+func newTestWorkerICE(t *testing.T) *ICE {
+	t.Helper()
+
+	config := icemaker.Config{}
+	stunTurn := &icemaker.StunTurn{}
+	stunTurn.Store(nil)
+	config.StunTurn = stunTurn
+
+	w, err := NewICE(log.WithField("test", t.Name()), "test-peer", config, true, nil, nil,
+		ICEDependencies{Signaler: signaling.NewSignaler(stubSignalClient{}, wgtypes.Key{})}, false)
+	require.NoError(t, err, "worker setup must succeed")
+	return w
+}
+
+// TestWorkerICE_CloseDuringDial_ClearsConnectingFlag drives the teardown race
+// through the real dial goroutine instead of simulating its cleanup.
+//
+// The real-world sequence this models:
+//  1. OnNewOffer starts a negotiation: agent set, agentConnecting = true,
+//     go connect()
+//  2. The network dies and connect() stays blocked inside GatherCandidates/Dial
+//  3. A WG handshake timeout calls Close(): the agent is released and the dial
+//     context cancelled, but agentConnecting is not reset
+//  4. The real goroutine wakes with an error and runs its own cleanup
+//     (closeAgent), where `w.agent == agent` is now false, so the flag reset
+//     is skipped
+//
+// There is no remote responder, so Dial can never succeed: whatever point the
+// goroutine is at, closing first forces it down the error path. Before the fix
+// the flag stays true forever and the deadline below expires.
+func TestWorkerICE_CloseDuringDial_ClearsConnectingFlag(t *testing.T) {
+	w := newTestWorkerICE(t)
+
+	sid := icemaker.SessionID("test-session-id")
+	w.OnNewOffer(t.Context(), &signaling.OfferAnswer{
+		IceCredentials: signaling.IceCredentials{
+			UFrag: "testufrag",
+			Pwd:   "testpwdtestpwdtestpwd12",
+		},
+		SessionID: &sid,
+	})
+	require.True(t, w.InProgress(), "OnNewOffer must mark the negotiation as in progress")
+
+	// Teardown wins the race while connect() is still running.
+	w.Close()
+
+	// Close drops the flags synchronously, so the assertion below does not
+	// converge on the goroutine: the deadline only absorbs the dial goroutine
+	// waking up in the background, proving nothing re-wedges it afterwards.
+	require.Eventually(t, func() bool {
+		return !w.InProgress()
+	}, 10*time.Second, 50*time.Millisecond,
+		"Close must leave the negotiation idle even while the dial goroutine is still winding down")
+
+	// abandonNegotiation owns these three fields together; the worker is idle
+	// only when all of them are dropped.
+	w.muxAgent.Lock()
+	defer w.muxAgent.Unlock()
+	assert.Nil(t, w.agent, "no agent may survive the teardown")
+	assert.False(t, w.agentConnecting, "the connecting flag must match the nil agent")
+	assert.Empty(t, w.remoteSessionID, "a dead session's remote ID must not linger")
+}
+
+// TestWorkerICE_CloseClearsResidualConnectingState covers Close on a worker whose
+// agent is already gone but whose flag is stuck on true, e.g. after an aborted
+// recreate in OnNewOffer or after a first Close raced a dial goroutine.
+func TestWorkerICE_CloseClearsResidualConnectingState(t *testing.T) {
+	w := newTestWorkerICE(t)
+
+	w.muxAgent.Lock()
+	w.agentConnecting = true
+	w.muxAgent.Unlock()
+
+	w.Close()
+
+	assert.False(t, w.InProgress(), "Close must drop residual connecting state even without a live agent")
+
+	w.muxAgent.Lock()
+	defer w.muxAgent.Unlock()
+	assert.Nil(t, w.agent)
+	assert.False(t, w.agentConnecting)
+	assert.Empty(t, w.remoteSessionID)
+}
+
+// TestWorkerICE_StaleCloseAgentKeepsCurrentSession pins the ownership guard in
+// closeAgent: a late-waking dial goroutine from an older session must not reset
+// the state of a newer negotiation that reused the worker. The newer session
+// must survive wholesale - agent, flag and remote session identity alike.
+func TestWorkerICE_StaleCloseAgentKeepsCurrentSession(t *testing.T) {
+	w := newTestWorkerICE(t)
+	t.Cleanup(w.Close)
+
+	sidA := icemaker.SessionID("session-a")
+	w.OnNewOffer(t.Context(), &signaling.OfferAnswer{
+		IceCredentials: signaling.IceCredentials{UFrag: "ufragaaaa", Pwd: "pwdpwdpwdpwdpwdpwdpwdp1"},
+		SessionID:      &sidA,
+	})
+	w.muxAgent.Lock()
+	oldAgent := w.agent
+	oldCancel := w.agentDialerCancel
+	w.muxAgent.Unlock()
+	require.NotNil(t, oldAgent, "OnNewOffer must have created an ICE agent")
+
+	w.Close()
+
+	sidB := icemaker.SessionID("session-b")
+	w.OnNewOffer(t.Context(), &signaling.OfferAnswer{
+		IceCredentials: signaling.IceCredentials{UFrag: "ufragbbbb", Pwd: "pwdpwdpwdpwdpwdpwdpwdp2"},
+		SessionID:      &sidB,
+	})
+	require.True(t, w.InProgress(), "the second negotiation must be in flight")
+
+	w.muxAgent.Lock()
+	newAgent := w.agent
+	w.muxAgent.Unlock()
+
+	// The old dial goroutine finally wakes and cleans up its captured agent.
+	w.closeAgent(oldAgent, oldCancel)
+
+	w.muxAgent.Lock()
+	defer w.muxAgent.Unlock()
+	assert.Same(t, newAgent, w.agent, "the current agent must be untouched by the stale cleanup")
+	assert.True(t, w.agentConnecting, "the current negotiation must stay in flight")
+	// Read live under the lock: a snapshot captured before the stale cleanup
+	// would pass even if the cleanup wiped current state.
+	assert.Equal(t, sidB, w.remoteSessionID, "the remote session identity must be preserved")
+}
+
+// closeTrackConn records Close calls so a test can assert that a discarded
+// connection was actually released.
+type closeTrackConn struct {
+	net.Conn
+	closed atomic.Bool
+}
+
+func (c *closeTrackConn) Close() error {
+	c.closed.Store(true)
+	return c.Conn.Close()
+}
+
+// TestWorkerICE_StaleDialSuccessKeepsNewerNegotiation pins the ownership guard
+// in connect()'s success path: a dial that came back after a newer negotiation
+// replaced the agent must discard its connection and leave the newer session's
+// state - agent, agentConnecting, remoteSessionID, lastSuccess - intact.
+//
+// The dial hook holds session A's goroutine open until session B is installed,
+// then returns a live connection, mimicking the vendored pion dial which hands
+// out a live *ice.Conn when a pair is selected without checking afterwards
+// whether the agent was replaced meanwhile. Releasing A's dial therefore
+// exercises the stale-success commit path deterministically instead of racing
+// real ICE.
+func TestWorkerICE_StaleDialSuccessKeepsNewerNegotiation(t *testing.T) {
+	w := newTestWorkerICE(t)
+	t.Cleanup(w.Close)
+
+	dialStarted := make(chan struct{})
+	releaseDial := make(chan struct{})
+	staleConn := &closeTrackConn{}
+
+	var calls atomic.Int32
+	w.dialFunc = func(ctx context.Context, _ *icemaker.ThreadSafeAgent, _ *signaling.OfferAnswer) (net.Conn, error) {
+		if calls.Add(1) == 1 {
+			// Session A: hold the goroutine open until session B is installed,
+			// then return a live connection, mimicking the vendored pion dial
+			// which hands out a live *ice.Conn once a pair is selected without
+			// re-checking whether the agent was replaced meanwhile. Releasing
+			// the dial therefore exercises the stale-success commit path
+			// deterministically instead of racing real ICE.
+			close(dialStarted)
+			<-releaseDial
+			client, _ := net.Pipe()
+			staleConn.Conn = client
+			return staleConn, nil
+		}
+		// A newer negotiation parks on its dialer context, cancelled by the
+		// t.Cleanup Close at test end.
+		<-ctx.Done()
+		return nil, ctx.Err()
+	}
+
+	sidA := icemaker.SessionID("session-a")
+	w.OnNewOffer(t.Context(), &signaling.OfferAnswer{
+		IceCredentials: signaling.IceCredentials{UFrag: "ufragaaaa", Pwd: "pwdpwdpwdpwdpwdpwdpwdp1"},
+		SessionID:      &sidA,
+	})
+	require.True(t, w.InProgress(), "session A must be in flight")
+
+	// Session A's goroutine is now parked in the dial hook.
+	<-dialStarted
+
+	sidB := icemaker.SessionID("session-b")
+	w.OnNewOffer(t.Context(), &signaling.OfferAnswer{
+		IceCredentials: signaling.IceCredentials{UFrag: "ufragbbbb", Pwd: "pwdpwdpwdpwdpwdpwdpwdp2"},
+		SessionID:      &sidB,
+	})
+
+	w.muxAgent.Lock()
+	agentB := w.agent
+	w.lastSuccess = time.Time{}
+	w.muxAgent.Unlock()
+	require.NotNil(t, agentB, "session B must have created an ICE agent")
+	require.True(t, w.InProgress(), "session B must be in flight")
+
+	// Release session A's dial: it must be recognized as stale and discarded.
+	close(releaseDial)
+	require.Eventually(t, func() bool {
+		return staleConn.closed.Load()
+	}, 10*time.Second, 10*time.Millisecond,
+		"the stale connection must be closed by the ownership guard")
+
+	w.muxAgent.Lock()
+	defer w.muxAgent.Unlock()
+	assert.Same(t, agentB, w.agent, "session A must not uninstall session B's agent")
+	assert.True(t, w.agentConnecting, "session A must not clear session B's connecting flag")
+	assert.Equal(t, sidB, w.remoteSessionID, "session A must not clear session B's remote session identity")
+	assert.True(t, w.lastSuccess.IsZero(), "session A must not record a success for session B")
+	// The commit block guards agentConnecting, lastSuccess and
+	// onICEConnectionIsReady together, so the state assertions above imply the
+	// callback never ran for session A; the nil conn would have panicked the
+	// stale goroutine on any invocation.
+}