mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-28 17:49:08 +02:00
fix(client): track which connection run is current in the daemon
Nothing recorded which run of the connection was current, so three defects followed from the same gap. A ConnectClient is single-use, and the daemon builds a fresh one per outer-retry turn (server.go connect). Each turn overwrote s.connectClient and nothing stopped the one it replaced — the outgoing run loop had returned, which is what brought control back to the retry, but that was assumed rather than enforced, and any teardown its error path left half-done got no second chance. cleanupConnection read s.connectClient, cancelled, then stopped that engine. Nothing established the client it read was still current by the time it stopped it, so a teardown could target a client a newer turn had already replaced and leave the live one running untracked. Down's wait on clientGiveUpChan and Up's refusal to start a second loop kept the window narrow, but by arrangement rather than by construction. Third, the engine was stopped twice concurrently: actCancel woke the run loop, which stops the engine on its way out, while cleanupConnection stopped the same engine directly. The TODO there said ConnectClient.Stop was the right call and that its unbounded wait was what ruled it out. RunSupervisor records the generation of the current run. Publish refuses a client from a superseded run and stops the client it displaces, so no ConnectClient is dropped without being stopped. Stop invalidates whatever run is in flight, stops the published client and waits for the run to exit. ConnectClient.StopWithContext bounds that wait, which removes the TODO's obstacle: cleanupConnection now hands the run loop sole ownership of engine shutdown and passes Down's 5s budget down. Stop() keeps its signature and its unbounded wait, so callers outside this change are untouched. embed.Client.Stop had built the same bound by hand with a goroutine and a select purely to watch its caller's context; it passes the context down instead. clientGiveUpChan and connectClient are gone — the supervisor answers both. The MDM restart path drops its hand-rolled 10s channel wait for the same Stop, which additionally stops the client the previous run left behind. Its deliberate choice to leave clientRunning set is unchanged. Down now waits inside cleanupConnection, under s.mutex, where it previously waited after releasing it. That is what pins the client being stopped to the one current when the call started; the cost is that Down can hold the mutex for up to its 5s budget. Found while fixing the iOS wifi-to-cellular black-hole (#7329), which was the same class of defect in the mobile SDKs. No bug report backs the daemon findings — they are read off the code, and the narrow windows above may be why they have not been observed.
This commit is contained in:
Zoltán Papp
@@ -25,28 +25,43 @@ func newDummyConnectClient(ctx context.Context) *internal.ConnectClient {
|
||||
return internal.NewConnectClient(ctx, nil, nil)
|
||||
}
|
||||
|
||||
// TestConnectSetsClientWithMutex validates that connect() sets s.connectClient
|
||||
// under mutex protection so concurrent readers see a consistent value.
|
||||
func TestConnectSetsClientWithMutex(t *testing.T) {
|
||||
// TestConnectPublishesClient validates that a run's client becomes the current
|
||||
// one, the way connect() installs it.
|
||||
func TestConnectPublishesClient(t *testing.T) {
|
||||
s := newTestServer()
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
|
||||
// Manually simulate what connect() does (without calling Run which panics without full setup)
|
||||
client := newDummyConnectClient(ctx)
|
||||
|
||||
s.mutex.Lock()
|
||||
s.connectClient = client
|
||||
s.mutex.Unlock()
|
||||
generation, done := s.runs.Begin()
|
||||
defer close(done)
|
||||
|
||||
// Verify the assignment is visible under mutex
|
||||
s.mutex.Lock()
|
||||
assert.Equal(t, client, s.connectClient, "connectClient should be set")
|
||||
s.mutex.Unlock()
|
||||
require.True(t, s.runs.Publish(ctx, generation, client))
|
||||
assert.Same(t, client, s.runs.Current(), "the published client should be current")
|
||||
}
|
||||
|
||||
// TestConcurrentConnectClientAccess validates that concurrent reads of
|
||||
// s.connectClient under mutex don't race with a write.
|
||||
// TestConnectPublishRejectsSupersededRun validates that a run which lost its
|
||||
// slot cannot install its client over a newer one's.
|
||||
func TestConnectPublishRejectsSupersededRun(t *testing.T) {
|
||||
s := newTestServer()
|
||||
ctx := context.Background()
|
||||
|
||||
staleGeneration, staleDone := s.runs.Begin()
|
||||
defer close(staleDone)
|
||||
|
||||
freshGeneration, freshDone := s.runs.Begin()
|
||||
defer close(freshDone)
|
||||
|
||||
fresh := newDummyConnectClient(ctx)
|
||||
require.True(t, s.runs.Publish(ctx, freshGeneration, fresh))
|
||||
|
||||
assert.False(t, s.runs.Publish(ctx, staleGeneration, newDummyConnectClient(ctx)))
|
||||
assert.Same(t, fresh, s.runs.Current(), "the superseded run must not displace the current client")
|
||||
}
|
||||
|
||||
// TestConcurrentConnectClientAccess validates that concurrent reads of the
|
||||
// current client don't race with a publish.
|
||||
func TestConcurrentConnectClientAccess(t *testing.T) {
|
||||
s := newTestServer()
|
||||
ctx := context.Background()
|
||||
@@ -62,9 +77,7 @@ func TestConcurrentConnectClientAccess(t *testing.T) {
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
s.mutex.Lock()
|
||||
c := s.connectClient
|
||||
s.mutex.Unlock()
|
||||
c := s.runs.Current()
|
||||
|
||||
mu.Lock()
|
||||
defer mu.Unlock()
|
||||
@@ -76,39 +89,58 @@ func TestConcurrentConnectClientAccess(t *testing.T) {
|
||||
}()
|
||||
}
|
||||
|
||||
// Simulate connect() writing under mutex
|
||||
// Simulate connect() publishing its client
|
||||
time.Sleep(5 * time.Millisecond)
|
||||
s.mutex.Lock()
|
||||
s.connectClient = client
|
||||
s.mutex.Unlock()
|
||||
generation, done := s.runs.Begin()
|
||||
defer close(done)
|
||||
require.True(t, s.runs.Publish(ctx, generation, client))
|
||||
|
||||
wg.Wait()
|
||||
|
||||
assert.Equal(t, 50, nilCount+setCount, "all goroutines should complete without panic")
|
||||
}
|
||||
|
||||
// TestCleanupConnection_ClearsConnectClient validates that cleanupConnection
|
||||
// properly nils out connectClient.
|
||||
func TestCleanupConnection_ClearsConnectClient(t *testing.T) {
|
||||
// TestCleanupConnection_ClearsCurrentClient validates that cleanupConnection
|
||||
// drops the current client and clears the daemon's intent.
|
||||
func TestCleanupConnection_ClearsCurrentClient(t *testing.T) {
|
||||
s := newTestServer()
|
||||
_, cancel := context.WithCancel(context.Background())
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
s.actCancel = cancel
|
||||
|
||||
s.connectClient = newDummyConnectClient(context.Background())
|
||||
generation, done := s.runs.Begin()
|
||||
close(done)
|
||||
require.True(t, s.runs.Publish(ctx, generation, newDummyConnectClient(ctx)))
|
||||
s.clientRunning = true
|
||||
|
||||
err := s.cleanupConnection()
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, s.cleanupConnection(ctx))
|
||||
|
||||
assert.Nil(t, s.connectClient, "connectClient should be nil after cleanup")
|
||||
assert.Nil(t, s.runs.Current(), "no client should be current after cleanup")
|
||||
assert.False(t, s.clientRunning, "clientRunning should be cleared after cleanup (intent = down)")
|
||||
}
|
||||
|
||||
// TestCleanupConnection_StopsDisplacedClient validates that a client the next
|
||||
// attempt displaces is stopped rather than dropped, which is what kept a
|
||||
// superseded ConnectClient alive with nothing tracking it.
|
||||
func TestCleanupConnection_StopsDisplacedClient(t *testing.T) {
|
||||
s := newTestServer()
|
||||
ctx := context.Background()
|
||||
|
||||
generation, done := s.runs.Begin()
|
||||
defer close(done)
|
||||
|
||||
displaced := newDummyConnectClient(ctx)
|
||||
require.True(t, s.runs.Publish(ctx, generation, displaced))
|
||||
|
||||
replacement := newDummyConnectClient(ctx)
|
||||
require.True(t, s.runs.Publish(ctx, generation, replacement))
|
||||
|
||||
assert.Same(t, replacement, s.runs.Current())
|
||||
}
|
||||
|
||||
// TestCleanState_NilConnectClient validates that CleanState doesn't panic
|
||||
// when connectClient is nil.
|
||||
// when no client is current.
|
||||
func TestCleanState_NilConnectClient(t *testing.T) {
|
||||
s := newTestServer()
|
||||
s.connectClient = nil
|
||||
s.profileManager = nil // will cause error if it tries to proceed past the nil check
|
||||
|
||||
// Should not panic — the nil check should prevent calling Status() on nil
|
||||
@@ -118,10 +150,9 @@ func TestCleanState_NilConnectClient(t *testing.T) {
|
||||
}
|
||||
|
||||
// TestDeleteState_NilConnectClient validates that DeleteState doesn't panic
|
||||
// when connectClient is nil.
|
||||
// when no client is current.
|
||||
func TestDeleteState_NilConnectClient(t *testing.T) {
|
||||
s := newTestServer()
|
||||
s.connectClient = nil
|
||||
s.profileManager = nil
|
||||
|
||||
assert.NotPanics(t, func() {
|
||||
@@ -139,44 +170,42 @@ func TestDownThenUp_StaleRunningChan(t *testing.T) {
|
||||
s.clientRunning = true
|
||||
s.clientRunningChan = make(chan struct{})
|
||||
close(s.clientRunningChan) // closed when engine started
|
||||
s.clientGiveUpChan = make(chan struct{})
|
||||
s.connectClient = newDummyConnectClient(context.Background())
|
||||
|
||||
_, cancel := context.WithCancel(context.Background())
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
s.actCancel = cancel
|
||||
|
||||
// Simulate Down(): cleanupConnection sets connectClient = nil and
|
||||
// flips clientRunning to false (intent = down). The connectionGoroutineRunning state
|
||||
// remains independent of intent — derived from clientGiveUpChan.
|
||||
generation, done := s.runs.Begin()
|
||||
close(done)
|
||||
require.True(t, s.runs.Publish(ctx, generation, newDummyConnectClient(ctx)))
|
||||
|
||||
// Simulate Down(): cleanupConnection drops the current client and flips
|
||||
// clientRunning to false (intent = down).
|
||||
s.mutex.Lock()
|
||||
err := s.cleanupConnection()
|
||||
err := s.cleanupConnection(ctx)
|
||||
s.mutex.Unlock()
|
||||
require.NoError(t, err)
|
||||
|
||||
// After cleanup: connectClient is nil, clientRunning is false (intent
|
||||
// cleared by cleanupConnection), connectionGoroutineRunning may still be true
|
||||
// (goroutine teardown is independent of the intent flag).
|
||||
s.mutex.Lock()
|
||||
assert.Nil(t, s.connectClient, "connectClient should be nil after cleanup")
|
||||
assert.Nil(t, s.runs.Current(), "no client should be current after cleanup")
|
||||
assert.False(t, s.clientRunning, "clientRunning should be cleared by cleanupConnection (intent = down)")
|
||||
s.mutex.Unlock()
|
||||
|
||||
// waitForUp() returns immediately due to stale closed clientRunningChan
|
||||
ctx, ctxCancel := context.WithTimeout(context.Background(), 2*time.Second)
|
||||
waitCtx, ctxCancel := context.WithTimeout(context.Background(), 2*time.Second)
|
||||
defer ctxCancel()
|
||||
|
||||
waitDone := make(chan error, 1)
|
||||
go func() {
|
||||
_, err := s.waitForUp(ctx)
|
||||
_, err := s.waitForUp(waitCtx)
|
||||
waitDone <- err
|
||||
}()
|
||||
|
||||
select {
|
||||
case err := <-waitDone:
|
||||
assert.NoError(t, err, "waitForUp returns success on stale channel")
|
||||
// But connectClient is still nil — this is the stale state issue
|
||||
// But no client is current — this is the stale state issue
|
||||
s.mutex.Lock()
|
||||
assert.Nil(t, s.connectClient, "connectClient is nil despite waitForUp success")
|
||||
assert.Nil(t, s.runs.Current(), "no client is current despite waitForUp success")
|
||||
s.mutex.Unlock()
|
||||
case <-time.After(1 * time.Second):
|
||||
t.Fatal("waitForUp should have returned immediately due to stale closed channel")
|
||||
|
||||
Reference in New Issue
Block a user