WIP

shared/relay/client/manager.go

@@ -30,11 +30,17 @@ type RelayTrack struct {
 	relayClient *Client
 	err         error
 	created     time.Time
+	// ready is closed once the dial started by openConnVia finishes, at which
+	// point exactly one of relayClient/err is set. Callers that find an existing
+	// track wait on this channel instead of the track lock, so the network dial
+	// is never performed while holding rt.Lock(). See openConnVia.
+	ready chan struct{}
 }
 
 func NewRelayTrack() *RelayTrack {
 	return &RelayTrack{
 		created: time.Now(),
+		ready:   make(chan struct{}),
 	}
 }
 
@@ -294,8 +300,17 @@ func (m *Manager) RelayStates() []RelayConnState {
 
 	// Only connected foreign relays carry state; a failed connect is evicted
 	// immediately (openConnVia), so there is no error state to surface.
+	//
+	// Query each track without blocking: openConnVia holds a track's write-lock
+	// for the whole of relayClient.Connect() (the network dial). A blocking
+	// RLock here would stall the status path (GetFullStatus -> GetRelayStates)
+	// for the full dial timeout. A track mid-Connect has no relayClient set yet
+	// and would be skipped anyway, so TryRLock + skip preserves the result while
+	// keeping status responsive.
 	for _, rt := range tracks {
-		rt.RLock()
+		if !rt.TryRLock() {
+			continue
+		}
 		rc := rt.relayClient
 		rt.RUnlock()
 		if rc != nil {
@@ -326,34 +341,27 @@ func (m *Manager) openConnVia(ctx context.Context, serverAddress, peerKey string
 	// check if already has a connection to the desired relay server
 	m.relayClientsMutex.RLock()
 	rt, ok := m.relayClients[serverAddress]
-	if ok {
-		rt.RLock()
-		m.relayClientsMutex.RUnlock()
-		defer rt.RUnlock()
-		if rt.err != nil {
-			return nil, rt.err
-		}
-		return rt.relayClient.OpenConn(ctx, peerKey)
-	}
 	m.relayClientsMutex.RUnlock()
+	if ok {
+		return m.openConnOnTrack(ctx, rt, peerKey)
+	}
 
 	// if not, establish a new connection but check it again (because changed the lock type) before starting the
 	// connection
 	m.relayClientsMutex.Lock()
 	rt, ok = m.relayClients[serverAddress]
 	if ok {
-		rt.RLock()
 		m.relayClientsMutex.Unlock()
-		defer rt.RUnlock()
-		if rt.err != nil {
-			return nil, rt.err
-		}
-		return rt.relayClient.OpenConn(ctx, peerKey)
+		return m.openConnOnTrack(ctx, rt, peerKey)
 	}
 
-	// create a new relay client and store it in the relayClients map
+	// Create the track, publish it, and release the map lock BEFORE dialing. The
+	// dial must not run while holding any track lock: RelayStates() and the
+	// cleanup loop take the track lock, and blocking them for the whole dial
+	// timeout is what stalls `netbird status -d`. Concurrent callers find this
+	// track in the map and wait on rt.ready (see openConnOnTrack), so only this
+	// goroutine performs the dial and the others reuse its result.
 	rt = NewRelayTrack()
-	rt.Lock()
 	m.relayClients[serverAddress] = rt
 	m.relayClientsMutex.Unlock()
 
@@ -361,8 +369,10 @@ func (m *Manager) openConnVia(ctx context.Context, serverAddress, peerKey string
 	relayClient.SetTransportFallback(m.transportFallback)
 	err := relayClient.Connect(m.ctx)
 	if err != nil {
+		rt.Lock()
 		rt.err = err
 		rt.Unlock()
+		close(rt.ready)
 		m.relayClientsMutex.Lock()
 		delete(m.relayClients, serverAddress)
 		m.relayClientsMutex.Unlock()
@@ -370,8 +380,10 @@ func (m *Manager) openConnVia(ctx context.Context, serverAddress, peerKey string
 	}
 	// if connection closed then delete the relay client from the list
 	relayClient.SetOnDisconnectListener(m.onServerDisconnected)
+	rt.Lock()
 	rt.relayClient = relayClient
 	rt.Unlock()
+	close(rt.ready)
 
 	conn, err := relayClient.OpenConn(ctx, peerKey)
 	if err != nil {
@@ -380,6 +392,31 @@ func (m *Manager) openConnVia(ctx context.Context, serverAddress, peerKey string
 	return conn, nil
 }
 
+// openConnOnTrack opens a peer connection through an existing relay track,
+// waiting for the dial started by another openConnVia call (if still running)
+// to finish. It waits on rt.ready rather than the track lock, so it neither
+// holds nor contends the track lock across the dial; the RLock it takes
+// afterwards only guards the brief relayClient read + OpenConn, matching the
+// previous behaviour of protecting the client against a concurrent cleanup
+// close.
+func (m *Manager) openConnOnTrack(ctx context.Context, rt *RelayTrack, peerKey string) (net.Conn, error) {
+	select {
+	case <-rt.ready:
+	case <-ctx.Done():
+		return nil, ctx.Err()
+	}
+
+	rt.RLock()
+	defer rt.RUnlock()
+	if rt.err != nil {
+		return nil, rt.err
+	}
+	if rt.relayClient == nil {
+		return nil, ErrRelayClientNotConnected
+	}
+	return rt.relayClient.OpenConn(ctx, peerKey)
+}
+
 func (m *Manager) onServerConnected() {
 	m.listenerLock.Lock()
 	defer m.listenerLock.Unlock()
@@ -476,6 +513,15 @@ func (m *Manager) cleanUpUnusedRelays() {
 			continue
 		}
 
+		// The dial started by openConnVia is still in progress: the track is
+		// published before Connect() completes and no longer runs under rt.Lock,
+		// so relayClient is not set yet. Nothing to clean up, and it must not be
+		// evicted out from under the in-flight dial.
+		if rt.relayClient == nil {
+			rt.Unlock()
+			continue
+		}
+
 		if time.Since(rt.created) <= m.keepUnusedServerTime {
 			rt.Unlock()
 			continue

shared/relay/client/manager_dial_test.go

@@ -0,0 +1,72 @@
+package client
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestManager_InProgressDialIsSafeForReadersAndCleanup covers the new state that
+// option 1 introduces: openConnVia now publishes a relay track in the map and
+// releases the map lock BEFORE dialing, so a track can legitimately exist with
+// relayClient == nil and no track lock held while its Connect() is in flight.
+//
+// The status path (RelayStates) and the cleanup loop both touch every track, so
+// both must tolerate that mid-dial state — neither deref the nil relayClient nor
+// evict a track whose dial is still running.
+func TestManager_InProgressDialIsSafeForReadersAndCleanup(t *testing.T) {
+	m := &Manager{
+		relayClients: make(map[string]*RelayTrack),
+		// 0 so the created-time grace does not mask the mid-dial (nil relayClient)
+		// guard in cleanUpUnusedRelays.
+		keepUnusedServerTime: 0,
+	}
+
+	const addr = "relay.example.com:443"
+	rt := NewRelayTrack() // ready open, relayClient nil, unlocked == dial in progress
+	m.relayClients[addr] = rt
+
+	// A status call must not block or panic on a relay still being dialed; it has
+	// no state to report yet.
+	require.Empty(t, m.RelayStates(), "a relay still being dialed has no state to report")
+
+	// Cleanup must not deref the nil relayClient, and must not evict an in-flight dial.
+	m.cleanUpUnusedRelays()
+	m.relayClientsMutex.RLock()
+	_, stillTracked := m.relayClients[addr]
+	m.relayClientsMutex.RUnlock()
+	require.True(t, stillTracked, "an in-progress dial must not be cleaned up")
+}
+
+// TestOpenConnOnTrack_ReleasesOnContextCancelDuringDial verifies the core option-1
+// property: a caller that finds a track whose dial is in progress waits on
+// rt.ready, not on the track lock, and can be released by its own context. This
+// is what keeps a slow relay dial from serializing behind the track lock.
+func TestOpenConnOnTrack_ReleasesOnContextCancelDuringDial(t *testing.T) {
+	m := &Manager{relayClients: make(map[string]*RelayTrack)}
+
+	rt := NewRelayTrack() // ready deliberately left open == dial in progress
+
+	ctx, cancel := context.WithCancel(context.Background())
+	errCh := make(chan error, 1)
+	go func() {
+		_, err := m.openConnOnTrack(ctx, rt, "peerKey")
+		errCh <- err
+	}()
+
+	// While waiting for the dial the caller holds no track lock, so a concurrent
+	// reader (e.g. RelayStates) can still take it.
+	require.True(t, rt.TryRLock(), "waiter must not hold the track lock while the dial is in progress")
+	rt.RUnlock()
+
+	// Caller gives up: openConnOnTrack must return via ctx rather than hang on the dial.
+	cancel()
+	select {
+	case err := <-errCh:
+		require.ErrorIs(t, err, context.Canceled)
+	case <-time.After(2 * time.Second):
+		t.Fatal("openConnOnTrack did not return on ctx cancellation while a dial was in progress")
+	}
+}

shared/relay/client/manager_relaystates_test.go

@@ -0,0 +1,62 @@
+package client
+
+import (
+	"testing"
+	"time"
+)
+
+// TestRelayStates_DoesNotBlockWhileForeignRelayConnecting is a regression test for
+// status calls hanging behind an in-progress relay dial.
+//
+// openConnVia establishes a new foreign relay like this:
+//
+//	rt = NewRelayTrack()
+//	rt.Lock()                          // track write-lock
+//	m.relayClients[serverAddress] = rt
+//	m.relayClientsMutex.Unlock()
+//	...
+//	err := relayClient.Connect(m.ctx)  // network dial, held UNDER rt.Lock()
+//	...
+//	rt.Unlock()                        // released only after Connect returns/times out
+//
+// So 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).
+//
+// RelayStates() — reached from the daemon status path via
+// peer.Status.GetFullStatus() -> GetRelayStates() -> Manager.RelayStates() — takes
+// rt.RLock() on every tracked relay. A reader lock blocks while a writer holds the
+// lock, so a single foreign relay mid-Connect stalls RelayStates(), and therefore
+// `netbird status -d`, for the full dial timeout. #6547 moved this off the shared
+// map lock but the per-track RLock still blocks the status path.
+//
+// This test recreates the exact in-progress-dial state (track present in the map
+// with its write-lock held) and asserts RelayStates() does not wait on it.
+func TestRelayStates_DoesNotBlockWhileForeignRelayConnecting(t *testing.T) {
+	m := &Manager{
+		relayClients: make(map[string]*RelayTrack),
+	}
+
+	// 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
+	// Release at the end so a (buggy) blocked RelayStates goroutine can unwind
+	// instead of leaking past the test.
+	t.Cleanup(rt.Unlock)
+
+	done := make(chan []RelayConnState, 1)
+	go func() {
+		done <- m.RelayStates()
+	}()
+
+	select {
+	case <-done:
+		// RelayStates returned without waiting for the in-progress dial. Good.
+	case <-time.After(2 * time.Second):
+		t.Fatal("RelayStates() blocked on a relay track whose Connect() is in progress " +
+			"(rt.Lock held across the dial in openConnVia); `netbird status -d` hangs for " +
+			"the relay dial timeout")
+	}
+}