diff --git a/shared/relay/client/manager.go b/shared/relay/client/manager.go index 77c55400d..d65ebb1ce 100644 --- a/shared/relay/client/manager.go +++ b/shared/relay/client/manager.go @@ -477,7 +477,10 @@ func (m *Manager) cleanUpUnusedRelays() { defer m.relayClientsMutex.Unlock() for addr, rt := range m.relayClients { - rt.Lock() + if !rt.TryRLock() { + // Skip while in-progress relay dial. + continue + } // if the connection failed to the server the relay client will be nil // but the instance will be kept in the relayClients until the next locking if rt.err != nil { diff --git a/shared/relay/client/manager_cleanup_test.go b/shared/relay/client/manager_cleanup_test.go new file mode 100644 index 000000000..6ac5daeac --- /dev/null +++ b/shared/relay/client/manager_cleanup_test.go @@ -0,0 +1,60 @@ +package client + +import ( + "context" + "net/netip" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// TestCleanUpUnusedRelays_DoesNotBlockOnRealHangingDial drives a real, hanging foreign +// relay dial and asserts cleanUpUnusedRelays does not stall behind it. +func TestCleanUpUnusedRelays_DoesNotBlockOnRealHangingDial(t *testing.T) { + serverAddr := stallingRelayListener(t) + + mCtx, mCancel := context.WithCancel(context.Background()) + t.Cleanup(mCancel) + + m := NewManager(mCtx, nil, "alice", 1280) + + dialDone := make(chan struct{}) + go func() { + defer close(dialDone) + _, _ = m.openConnVia(mCtx, serverAddr, "peerKey", netip.Addr{}) + }() + + // The track appears in the map once the dial is in flight. + require.Eventually(t, func() bool { + m.relayClientsMutex.RLock() + defer m.relayClientsMutex.RUnlock() + _, ok := m.relayClients[serverAddr] + return ok + }, 5*time.Second, 5*time.Millisecond, "relay dial did not start") + + cleanupDone := make(chan struct{}) + go func() { + defer close(cleanupDone) + m.cleanUpUnusedRelays() + }() + + select { + case <-cleanupDone: + case <-time.After(2 * time.Second): + t.Fatal("cleanUpUnusedRelays blocked on an in-progress relay dial while holding the relay map lock") + } + + m.relayClientsMutex.RLock() + _, stillTracked := m.relayClients[serverAddr] + m.relayClientsMutex.RUnlock() + require.True(t, stillTracked, "an in-progress relay dial must not be evicted by cleanup") + + // Release the hanging dial so the goroutine can exit cleanly. + mCancel() + select { + case <-dialDone: + case <-time.After(5 * time.Second): + t.Fatal("openConnVia did not return after context cancellation") + } +} diff --git a/shared/relay/client/manager_relaystates_test.go b/shared/relay/client/manager_relaystates_test.go index 453b1fc11..5d2d242da 100644 --- a/shared/relay/client/manager_relaystates_test.go +++ b/shared/relay/client/manager_relaystates_test.go @@ -1,33 +1,76 @@ package client import ( + "context" + "net" + "net/netip" + "sync" "testing" "time" + + "github.com/stretchr/testify/require" ) -// TestRelayStates_DoesNotBlockWhileForeignRelayConnecting is a regression test for +// stallingRelayListener accepts TCP connections and holds them open without ever +// responding, so a relay handshake dialed against it blocks until its context is +// cancelled. It returns the "rel://host:port" URL to dial. +func stallingRelayListener(t *testing.T) string { + t.Helper() + + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + + var mu sync.Mutex + var conns []net.Conn + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + mu.Lock() + conns = append(conns, c) + mu.Unlock() + } + }() + t.Cleanup(func() { + _ = ln.Close() + mu.Lock() + for _, c := range conns { + _ = c.Close() + } + mu.Unlock() + }) + + return "rel://" + ln.Addr().String() +} + +// TestRelayStates_DoesNotBlockOnRealHangingDial is a regression test for // status calls hanging behind an in-progress relay dial. // // While a relay is being dialed, its RelayTrack write-lock is held for the whole // dial (up to serverResponseTimeout per transport attempt, times the transport // fallback chain, times however many relays are being dialed at once) in openConnVia. -// -// RelayStates() is reached from the daemon status path via -// peer.Status.GetFullStatus() -> GetRelayStates() -> Manager.RelayStates(). -// It takes rt.RLock() on every tracked relay. A reader lock blocks while a -// writer holds the lock, so a single foreign relay mid-Connect in openConnVia -// stalls RelayStates(), and therefore `netbird status -d` hangs for the full dial timeout. -func TestRelayStates_DoesNotBlockWhileForeignRelayConnecting(t *testing.T) { - m := &Manager{ - relayClients: make(map[string]*RelayTrack), - } +func TestRelayStates_DoesNotBlockOnRealHangingDial(t *testing.T) { + serverAddr := stallingRelayListener(t) - // Mirror openConnVia's state during a live dial: a track in the map whose - // write-lock is held for the duration of relayClient.Connect(). - rt := NewRelayTrack() - rt.Lock() - m.relayClients["relay.example.com:443"] = rt - t.Cleanup(rt.Unlock) + mCtx, mCancel := context.WithCancel(context.Background()) + t.Cleanup(mCancel) + + m := NewManager(mCtx, nil, "alice", 1280) + + dialDone := make(chan struct{}) + go func() { + defer close(dialDone) + _, _ = m.openConnVia(mCtx, serverAddr, "peerKey", netip.Addr{}) + }() + + require.Eventually(t, func() bool { + m.relayClientsMutex.RLock() + defer m.relayClientsMutex.RUnlock() + _, ok := m.relayClients[serverAddr] + return ok + }, 5*time.Second, 5*time.Millisecond, "relay dial did not start") done := make(chan []RelayConnState, 1) go func() { @@ -35,8 +78,17 @@ func TestRelayStates_DoesNotBlockWhileForeignRelayConnecting(t *testing.T) { }() select { - case <-done: + case states := <-done: + require.Empty(t, states, "a relay still being dialed carries no state and must be omitted") case <-time.After(2 * time.Second): - t.Fatal("RelayStates() blocked on a relay track whose Connect() is in progress") + t.Fatal("RelayStates blocked on a foreign relay whose Connect() is in progress") + } + + // Release the hanging dial so the goroutine can exit cleanly. + mCancel() + select { + case <-dialDone: + case <-time.After(5 * time.Second): + t.Fatal("openConnVia did not return after context cancellation") } }