diff --git a/client/embed/embed.go b/client/embed/embed.go index 5a3d11f24..ae21128e9 100644 --- a/client/embed/embed.go +++ b/client/embed/embed.go @@ -338,23 +338,14 @@ func (c *Client) Stop(ctx context.Context) error { c.cancel = nil } - done := make(chan error, 1) connect := c.connect - go func() { - done <- connect.Stop() - }() + c.connect = nil - select { - case <-ctx.Done(): - c.connect = nil - return ctx.Err() - case err := <-done: - c.connect = nil - if err != nil { - return fmt.Errorf("stop: %w", err) - } - return nil + if err := connect.StopWithContext(ctx); err != nil { + return fmt.Errorf("stop: %w", err) } + + return nil } // GetConfig returns a copy of the internal client config. diff --git a/client/internal/connect.go b/client/internal/connect.go index ca50f912f..46a9c188e 100644 --- a/client/internal/connect.go +++ b/client/internal/connect.go @@ -599,11 +599,25 @@ func (c *ConnectClient) Status() StatusType { } func (c *ConnectClient) Stop() error { + return c.StopWithContext(context.Background()) +} + +// StopWithContext cancels the run loop and waits for it to exit, giving up the +// wait when ctx is done and returning ctx.Err(). Giving up only abandons the +// wait: the run stays cancelled and finishes its teardown in the background, so +// a caller that returns early must not assume the engine is already gone. +func (c *ConnectClient) StopWithContext(ctx context.Context) error { c.runCancel() - if c.runStarted.Load() { - <-c.runExited + if !c.runStarted.Load() { + return nil + } + + select { + case <-c.runExited: + return nil + case <-ctx.Done(): + return ctx.Err() } - return nil } // SetSyncResponsePersistence enables or disables sync response persistence. diff --git a/client/internal/run_supervisor.go b/client/internal/run_supervisor.go new file mode 100644 index 000000000..4a4c51b7e --- /dev/null +++ b/client/internal/run_supervisor.go @@ -0,0 +1,132 @@ +package internal + +import ( + "context" + "sync" + + log "github.com/sirupsen/logrus" +) + +// RunSupervisor tracks which run of a logical connection is current. +// +// A ConnectClient is single-use, so every attempt builds a fresh one. Without a +// record of which attempt is current, a stale one can publish its client over a +// newer one's, and a teardown can target a client that has already been +// replaced — leaving the live one running with nothing tracking it. +// +// A run claims a generation with Begin, publishes its client with Publish, and +// closes the channel Begin returned when it exits. Publish refuses a client from +// a run that is no longer current and stops the client it displaces, so no +// ConnectClient is dropped without being stopped. +// +// The zero value is ready to use. +type RunSupervisor struct { + mu sync.Mutex + generation uint64 + current *ConnectClient + done chan struct{} +} + +// Begin claims a generation for a starting run. The caller must close the +// returned channel when the run exits, whether or not it published a client. +func (s *RunSupervisor) Begin() (uint64, chan struct{}) { + done := make(chan struct{}) + + s.mu.Lock() + defer s.mu.Unlock() + + s.generation++ + s.done = done + + return s.generation, done +} + +// Publish installs cc as the current client and reports whether it took effect. +// It returns false once a newer Begin or a Stop has superseded the generation, +// and the caller must then abandon its startup. A client it displaces within the +// same run is stopped, with stopCtx bounding that wait. +func (s *RunSupervisor) Publish(ctx context.Context, generation uint64, cc *ConnectClient) bool { + s.mu.Lock() + if s.generation != generation { + s.mu.Unlock() + return false + } + displaced := s.current + s.current = cc + s.mu.Unlock() + + if displaced != nil && displaced != cc { + if err := displaced.StopWithContext(ctx); err != nil { + log.Warnf("stopping the displaced connect client: %v", err) + } + } + + return true +} + +// Current returns the published client, or nil while no run has published one. +func (s *RunSupervisor) Current() *ConnectClient { + s.mu.Lock() + defer s.mu.Unlock() + + return s.current +} + +// Done returns the channel the current run closes when it exits, or nil when no +// run has been started. Callers that only need a yes/no answer should use Alive. +func (s *RunSupervisor) Done() <-chan struct{} { + s.mu.Lock() + defer s.mu.Unlock() + + return s.done +} + +// Alive reports whether a run has claimed a generation and not yet signalled its +// exit. +func (s *RunSupervisor) Alive() bool { + s.mu.Lock() + done := s.done + s.mu.Unlock() + + if done == nil { + return false + } + + select { + case <-done: + return false + default: + return true + } +} + +// Stop invalidates any run in flight, stops the published client, and waits for +// the run to signal its exit. It gives up the wait when ctx is done and returns +// ctx.Err(); the run stays cancelled and finishes tearing down in the +// background, so an early return does not mean the engine is gone. +func (s *RunSupervisor) Stop(ctx context.Context) error { + s.mu.Lock() + s.generation++ + cc := s.current + done := s.done + s.current = nil + s.done = nil + s.mu.Unlock() + + if cc != nil { + if err := cc.StopWithContext(ctx); err != nil { + return err + } + } + + if done == nil { + return nil + } + + select { + case <-done: + return nil + case <-ctx.Done(): + return ctx.Err() + } +} diff --git a/client/internal/run_supervisor_test.go b/client/internal/run_supervisor_test.go new file mode 100644 index 000000000..c4f5c9a24 --- /dev/null +++ b/client/internal/run_supervisor_test.go @@ -0,0 +1,116 @@ +package internal + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newSupervisedClient() *ConnectClient { + return NewConnectClient(context.Background(), nil, nil) +} + +func TestRunSupervisorPublishRejectsSupersededRun(t *testing.T) { + var s RunSupervisor + ctx := context.Background() + + staleGen, staleDone := s.Begin() + defer close(staleDone) + + freshGen, freshDone := s.Begin() + defer close(freshDone) + + fresh := newSupervisedClient() + require.True(t, s.Publish(ctx, freshGen, fresh)) + + assert.False(t, s.Publish(ctx, staleGen, newSupervisedClient())) + assert.Same(t, fresh, s.Current()) +} + +func TestRunSupervisorPublishStopsDisplacedClient(t *testing.T) { + var s RunSupervisor + ctx := context.Background() + + generation, done := s.Begin() + defer close(done) + + displaced := newSupervisedClient() + require.True(t, s.Publish(ctx, generation, displaced)) + + replacement := newSupervisedClient() + require.True(t, s.Publish(ctx, generation, replacement)) + + assert.Same(t, replacement, s.Current()) + assert.Error(t, displaced.ctx.Err(), "the displaced client's run context should be cancelled") +} + +func TestRunSupervisorStopInvalidatesRunInFlight(t *testing.T) { + var s RunSupervisor + ctx := context.Background() + + generation, done := s.Begin() + close(done) + + require.NoError(t, s.Stop(ctx)) + + assert.False(t, s.Publish(ctx, generation, newSupervisedClient())) + assert.Nil(t, s.Current()) +} + +func TestRunSupervisorStopWaitsForRunExit(t *testing.T) { + var s RunSupervisor + + _, done := s.Begin() + + stopped := make(chan error, 1) + go func() { stopped <- s.Stop(context.Background()) }() + + select { + case <-stopped: + t.Fatal("Stop returned before the run signalled its exit") + case <-time.After(50 * time.Millisecond): + } + + close(done) + + select { + case err := <-stopped: + require.NoError(t, err) + case <-time.After(time.Second): + t.Fatal("Stop did not return after the run exited") + } +} + +func TestRunSupervisorStopGivesUpWaitOnContext(t *testing.T) { + var s RunSupervisor + + _, done := s.Begin() + defer close(done) + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + assert.ErrorIs(t, s.Stop(ctx), context.DeadlineExceeded) +} + +func TestRunSupervisorAliveTracksRun(t *testing.T) { + var s RunSupervisor + + assert.False(t, s.Alive(), "no run has started") + + _, done := s.Begin() + assert.True(t, s.Alive(), "a run is in flight") + + close(done) + assert.False(t, s.Alive(), "the run signalled its exit") +} + +func TestRunSupervisorStopWithoutRunIsNoop(t *testing.T) { + var s RunSupervisor + + require.NoError(t, s.Stop(context.Background())) + assert.Nil(t, s.Current()) +} diff --git a/client/server/capture.go b/client/server/capture.go index 308c00338..989be0d16 100644 --- a/client/server/capture.go +++ b/client/server/capture.go @@ -344,10 +344,10 @@ func (s *Server) clearCaptureIfOwner(sess *capture.Session, engine *internal.Eng } func (s *Server) getCaptureEngineLocked() (*internal.Engine, error) { - if s.connectClient == nil { + if s.runs.Current() == nil { return nil, status.Error(codes.FailedPrecondition, "client not connected") } - engine := s.connectClient.Engine() + engine := s.runs.Current().Engine() if engine == nil { return nil, status.Error(codes.FailedPrecondition, "engine not initialized") } diff --git a/client/server/debug.go b/client/server/debug.go index 8f4a506b4..f54700f7a 100644 --- a/client/server/debug.go +++ b/client/server/debug.go @@ -73,8 +73,8 @@ func (s *Server) generateDebugBundle(req *proto.DebugBundleRequest, uiOpener deb } var clientMetrics debug.MetricsExporter - if s.connectClient != nil { - if engine := s.connectClient.Engine(); engine != nil { + if s.runs.Current() != nil { + if engine := s.runs.Current().Engine(); engine != nil { if cm := engine.GetClientMetrics(); cm != nil { clientMetrics = cm } @@ -93,8 +93,8 @@ func (s *Server) generateDebugBundle(req *proto.DebugBundleRequest, uiOpener deb defer s.cleanupBundleCapture() var refreshStatus func() - if s.connectClient != nil { - engine := s.connectClient.Engine() + if s.runs.Current() != nil { + engine := s.runs.Current().Engine() if engine != nil { refreshStatus = func() { log.Debug("refreshing system health status for debug bundle") @@ -162,8 +162,8 @@ func (s *Server) SetLogLevel(_ context.Context, req *proto.SetLogLevelRequest) ( log.SetLevel(level) - if s.connectClient != nil { - s.connectClient.SetLogLevel(level) + if s.runs.Current() != nil { + s.runs.Current().SetLogLevel(level) } log.Infof("Log level set to %s", level.String()) @@ -217,15 +217,15 @@ func (s *Server) SetSyncResponsePersistence(_ context.Context, req *proto.SetSyn enabled := req.GetEnabled() s.persistSyncResponse = enabled - if s.connectClient != nil { - s.connectClient.SetSyncResponsePersistence(enabled) + if s.runs.Current() != nil { + s.runs.Current().SetSyncResponsePersistence(enabled) } return &proto.SetSyncResponsePersistenceResponse{}, nil } func (s *Server) getLatestSyncResponse() (*mgmProto.SyncResponse, error) { - cClient := s.connectClient + cClient := s.runs.Current() if cClient == nil { return nil, errors.New("connect client is not initialized") } diff --git a/client/server/mdm.go b/client/server/mdm.go index 9836c6bea..2b6100dcf 100644 --- a/client/server/mdm.go +++ b/client/server/mdm.go @@ -21,6 +21,11 @@ import ( // a no-op echo, never as a conflict with the policy. const preSharedKeyRedactedSentinel = "**********" +// mdmRestartStopTimeout bounds how long an MDM restart waits for the previous +// run to exit before giving up, so two runs cannot fight over the same status +// recorder and engine. +const mdmRestartStopTimeout = 10 * time.Second + // loadMDMPolicy is the indirection used by server handlers to read the // active MDM policy. Tests override this to inject a fake policy. var loadMDMPolicy = mdm.LoadPolicy @@ -71,19 +76,16 @@ func (s *Server) onMDMPolicyChange(_, _ *mdm.Policy) error { s.actCancel() } - // Wait for previous connectWithRetryRuns to exit so we don't end up - // with two goroutines fighting over the same status recorder + engine. - // The teardown engages a fan-out of engine goroutines (peer workers, - // signal handler, route manager, ...). close(clientGiveUpChan) - // happens in the function-scope defer of connectWithRetryRuns, on - // every exit path (ctx cancel, backoff exhausted, panic) — see the - // defer in server.go. - if s.clientGiveUpChan != nil { - select { - case <-s.clientGiveUpChan: - case <-time.After(10 * time.Second): - return fmt.Errorf("failed to restart the engine due to timeout") - } + // Wait for the previous run to exit so we don't end up with two + // goroutines fighting over the same status recorder + engine. The teardown + // engages a fan-out of engine goroutines (peer workers, signal handler, + // route manager, ...). The supervisor also stops the client that run + // published, which the bare channel wait did not. + stopCtx, cancelStop := context.WithTimeout(s.rootCtx, mdmRestartStopTimeout) + defer cancelStop() + + if err := s.runs.Stop(stopCtx); err != nil { + return fmt.Errorf("failed to restart the engine: %w", err) } if err := s.restartEngineForMDMLocked(); err != nil { @@ -161,9 +163,9 @@ func (s *Server) restartEngineForMDMLocked() error { s.actCancel = cancel s.clientRunning = true s.clientRunningChan = make(chan struct{}) - s.clientGiveUpChan = make(chan struct{}) + generation, done := s.runs.Begin() log.Info("MDM restart: spawning connectWithRetryRuns with re-resolved config") - go s.connectWithRetryRuns(ctx, config, s.statusRecorder, s.clientRunningChan, s.clientGiveUpChan) + go s.connectWithRetryRuns(ctx, generation, config, s.statusRecorder, s.clientRunningChan, done) s.publishConfigChangedEvent(proto.MetadataSourceMDM) return nil } diff --git a/client/server/network.go b/client/server/network.go index c390b8180..b6c1fb506 100644 --- a/client/server/network.go +++ b/client/server/network.go @@ -33,11 +33,11 @@ func (s *Server) ListNetworks(context.Context, *proto.ListNetworksRequest) (*pro return nil, gstatus.Errorf(codes.Unavailable, errNetworksDisabled) } - if s.connectClient == nil { + if s.runs.Current() == nil { return nil, fmt.Errorf("not connected") } - engine := s.connectClient.Engine() + engine := s.runs.Current().Engine() if engine == nil { return nil, fmt.Errorf("not connected") } @@ -146,11 +146,11 @@ func (s *Server) SelectNetworks(_ context.Context, req *proto.SelectNetworksRequ return nil, gstatus.Errorf(codes.Unavailable, errNetworksDisabled) } - if s.connectClient == nil { + if s.runs.Current() == nil { return nil, fmt.Errorf("not connected") } - engine := s.connectClient.Engine() + engine := s.runs.Current().Engine() if engine == nil { return nil, fmt.Errorf("not connected") } @@ -190,11 +190,11 @@ func (s *Server) DeselectNetworks(_ context.Context, req *proto.SelectNetworksRe return nil, gstatus.Errorf(codes.Unavailable, errNetworksDisabled) } - if s.connectClient == nil { + if s.runs.Current() == nil { return nil, fmt.Errorf("not connected") } - engine := s.connectClient.Engine() + engine := s.runs.Current().Engine() if engine == nil { return nil, fmt.Errorf("not connected") } @@ -232,4 +232,3 @@ func toNetIDs(routes []string) []route.NetID { } return netIDs } - diff --git a/client/server/server.go b/client/server/server.go index f33e19075..c9feb36f0 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -53,6 +53,16 @@ const ( // JWT token cache TTL for the client daemon (disabled by default) defaultJWTCacheTTL = 0 + // downStopTimeout bounds how long a teardown waits for the run loop to + // exit before proceeding anyway. A timeout here typically means the loop + // is wedged inside a slow teardown step. + downStopTimeout = 5 * time.Second + + // displacedStopTimeout bounds how long a fresh connection attempt waits for + // the client the previous attempt left behind to stop, before it goes ahead + // with its own run. + displacedStopTimeout = 5 * time.Second + errRestoreResidualState = "failed to restore residual state: %v" errProfilesDisabled = "profiles are disabled, you cannot use this feature without profiles enabled" errUpdateSettingsDisabled = "update settings are disabled, you cannot use this feature without update settings enabled" @@ -98,13 +108,15 @@ type Server struct { // Start / Up, cleared by Down / Logout. Persists across retry // loops, signal disconnects, and ErrResetConnection cycles. NOT // changed by connectWithRetryRuns goroutine exit — for that - // (goroutine-still-alive) check, see connectionGoroutineRunning() which - // derives from clientGiveUpChan close state. Protected by s.mutex. + // (goroutine-still-alive) check, see connectionGoroutineRunning(), which + // asks the supervisor. Protected by s.mutex. clientRunning bool clientRunningChan chan struct{} - clientGiveUpChan chan struct{} // closed when connectWithRetryRuns goroutine exits - connectClient *internal.ConnectClient + // runs owns the connection lifecycle: which ConnectClient is current, and + // whether the goroutine running it has exited. It stops a client it + // displaces, so no client is dropped without being stopped. + runs internal.RunSupervisor statusRecorder *peer.Status sessionWatcher *internal.SessionWatcher @@ -273,8 +285,8 @@ func (s *Server) Start() error { s.clientRunning = true s.clientRunningChan = make(chan struct{}) - s.clientGiveUpChan = make(chan struct{}) - go s.connectWithRetryRuns(ctx, config, s.statusRecorder, s.clientRunningChan, s.clientGiveUpChan) + generation, done := s.runs.Begin() + go s.connectWithRetryRuns(ctx, generation, config, s.statusRecorder, s.clientRunningChan, done) s.publishConfigChangedEvent(proto.MetadataSourceStartup) return nil } @@ -291,19 +303,15 @@ func (s *Server) Start() error { // of clientGiveUpChan. The defer does NOT touch s.mutex; the daemon's // "intent" (clientRunning) is maintained by the RPC handlers, not by this // goroutine. -func (s *Server) connectWithRetryRuns(ctx context.Context, profileConfig *profilemanager.Config, statusRecorder *peer.Status, runningChan chan struct{}, giveUpChan chan struct{}) { - // close(giveUpChan) MUST run on every exit path (DisableAutoConnect - // return, backoff.Retry return, panic) — Down() blocks for up to 5s - // waiting on this signal before flipping the state to Idle, and a - // missed close leaves Down() always hitting the timeout. - defer func() { - if giveUpChan != nil { - close(giveUpChan) - } - }() +func (s *Server) connectWithRetryRuns(ctx context.Context, generation uint64, profileConfig *profilemanager.Config, statusRecorder *peer.Status, runningChan chan struct{}, done chan struct{}) { + // close(done) MUST run on every exit path (DisableAutoConnect return, + // backoff.Retry return, panic) — Down() blocks for up to 5s waiting on this + // signal before flipping the state to Idle, and a missed close leaves Down() + // always hitting the timeout. + defer close(done) if s.config.DisableAutoConnect { - if err := s.connect(ctx, s.config, s.statusRecorder, runningChan); err != nil { + if err := s.connect(ctx, generation, s.config, s.statusRecorder, runningChan); err != nil { log.Debugf("run client connection exited with error: %v", err) } log.Tracef("client connection exited") @@ -332,7 +340,7 @@ func (s *Server) connectWithRetryRuns(ctx context.Context, profileConfig *profil }() runOperation := func() error { - err := s.connect(ctx, profileConfig, statusRecorder, runningChan) + err := s.connect(ctx, generation, profileConfig, statusRecorder, runningChan) if err != nil { // PermissionDenied means the daemon transitioned to NeedsLogin // inside connect(). Without backoff.Permanent the outer retry @@ -354,27 +362,16 @@ func (s *Server) connectWithRetryRuns(ctx context.Context, profileConfig *profil if err := backoff.Retry(runOperation, backOff); err != nil { log.Errorf("operation failed: %v", err) } - // giveUpChan is closed by the function-scope defer. + // done is closed by the function-scope defer. } -// connectionGoroutineRunning reports whether the connectWithRetryRuns goroutine is -// still running. Returns false when no goroutine has ever been started -// AND when the most recent one has already closed clientGiveUpChan on -// exit (whether due to ctx cancel, DisableAutoConnect single-shot -// completion, or backoff retry exhaustion). -// -// MUST be called with s.mutex held — accesses s.clientGiveUpChan which -// is written by Start/Up under the same lock. +// connectionGoroutineRunning reports whether the connectWithRetryRuns goroutine +// is still running. Returns false when no goroutine has ever been started AND +// when the most recent one has already signalled its exit (whether due to ctx +// cancel, DisableAutoConnect single-shot completion, or backoff retry +// exhaustion). func (s *Server) connectionGoroutineRunning() bool { - if s.clientGiveUpChan == nil { - return false - } - select { - case <-s.clientGiveUpChan: - return false - default: - return true - } + return s.runs.Alive() } // attemptLogin runs a login round trip against Management, or the stand-in a @@ -1010,9 +1007,9 @@ func (s *Server) Up(callerCtx context.Context, msg *proto.UpRequest) (*proto.UpR s.clientRunning = true s.clientRunningChan = make(chan struct{}) - s.clientGiveUpChan = make(chan struct{}) + generation, done := s.runs.Begin() - go s.connectWithRetryRuns(ctx, s.config, s.statusRecorder, s.clientRunningChan, s.clientGiveUpChan) + go s.connectWithRetryRuns(ctx, generation, s.config, s.statusRecorder, s.clientRunningChan, done) s.publishConfigChangedEvent(proto.MetadataSourceUpRPC) s.mutex.Unlock() @@ -1028,7 +1025,7 @@ func (s *Server) waitForUp(callerCtx context.Context) (*proto.UpResponse, error) defer cancel() select { - case <-s.clientGiveUpChan: + case <-s.runs.Done(): return nil, fmt.Errorf("client gave up to connect") case <-s.clientRunningChan: s.isSessionActive.Store(true) @@ -1196,9 +1193,10 @@ func (s *Server) SwitchProfile(callerCtx context.Context, msg *proto.SwitchProfi func (s *Server) Down(ctx context.Context, _ *proto.DownRequest) (*proto.DownResponse, error) { s.mutex.Lock() - giveUpChan := s.clientGiveUpChan + stopCtx, cancelStop := context.WithTimeout(ctx, downStopTimeout) + defer cancelStop() - if err := s.cleanupConnection(); err != nil { + if err := s.cleanupConnection(stopCtx); err != nil { s.mutex.Unlock() if errors.Is(err, ErrServiceNotUp) { log.Debugf("Down called while service not up: %v", err) @@ -1210,20 +1208,6 @@ func (s *Server) Down(ctx context.Context, _ *proto.DownRequest) (*proto.DownRes s.mutex.Unlock() - // Wait for the connectWithRetryRuns goroutine to finish with a short timeout. - // This prevents the goroutine from setting ErrResetConnection after Down() returns. - // The giveUpChan is closed by the goroutine's deferred cleanup (see - // connectWithRetryRuns) on every exit path. A timeout here typically - // means the goroutine is still wedged inside a slow teardown step. - if giveUpChan != nil { - select { - case <-giveUpChan: - log.Debugf("client goroutine finished, giveUpChan closed") - case <-time.After(5 * time.Second): - log.Warnf("timeout waiting for client goroutine to finish, proceeding anyway") - } - } - // Set Idle only after the retry goroutine has exited (or timed out). // Setting it earlier races with the goroutine's own Set(StatusConnecting) // at the top of each retry attempt, which would leave the snapshot @@ -1242,7 +1226,14 @@ func (s *Server) Down(ctx context.Context, _ *proto.DownRequest) (*proto.DownRes return &proto.DownResponse{}, nil } -func (s *Server) cleanupConnection() error { +func (s *Server) cleanupConnectionWithTimeout() error { + ctx, cancel := context.WithTimeout(s.rootCtx, downStopTimeout) + defer cancel() + + return s.cleanupConnection(ctx) +} + +func (s *Server) cleanupConnection(ctx context.Context) error { s.oauthAuthFlow = oauthAuthFlow{} if s.actCancel == nil { @@ -1255,32 +1246,16 @@ func (s *Server) cleanupConnection() error { // path, so its clientRunning stays true. s.clientRunning = false - // Capture the engine reference before cancelling the context. - // After actCancel(), the connectWithRetryRuns goroutine wakes up - // and sets connectClient.engine = nil, causing connectClient.Stop() - // to skip the engine shutdown entirely. - var engine *internal.Engine - if s.connectClient != nil { - engine = s.connectClient.Engine() - } - s.actCancel() - if s.connectClient == nil { - return nil + // The run loop is the sole owner of engine shutdown: the supervisor cancels + // the client and waits for the loop to exit, rather than stopping the engine + // alongside it. Waiting here also pins the client being stopped to the one + // that was current when this call started, which a bare read could not. + if err := s.runs.Stop(ctx); err != nil { + log.Warnf("stopping the connection during cleanup: %v", err) } - // TODO: consider calling s.connectClient.Stop() instead of engine.Stop(). - // actCancel() lets the run loop stop the engine too, so both stop it - // concurrently; ConnectClient.Stop cancels and waits for the run loop, - // making the run loop the sole owner of engine shutdown. - if engine != nil { - if err := engine.Stop(); err != nil { - log.Errorf("failed to stop engine during cleanup: %v", err) - } - } - - s.connectClient = nil s.isSessionActive.Store(false) log.Infof("service is down") @@ -1327,7 +1302,7 @@ func (s *Server) handleProfileLogout(ctx context.Context, msg *proto.LogoutReque activeProf, _ := s.profileManager.GetActiveProfileState() if activeProf != nil && activeProf.ID == resolved.ID { - if err := s.cleanupConnection(); err != nil && !errors.Is(err, ErrServiceNotUp) { + if err := s.cleanupConnectionWithTimeout(); err != nil && !errors.Is(err, ErrServiceNotUp) { log.Errorf("failed to cleanup connection: %v", err) } state := internal.CtxGetState(s.rootCtx) @@ -1356,7 +1331,7 @@ func (s *Server) handleActiveProfileLogout(ctx context.Context) (*proto.LogoutRe return nil, err } - if err := s.cleanupConnection(); err != nil && !errors.Is(err, ErrServiceNotUp) { + if err := s.cleanupConnectionWithTimeout(); err != nil && !errors.Is(err, ErrServiceNotUp) { // todo review to update the status in case any type of error log.Errorf("failed to cleanup connection: %v", err) return nil, err @@ -1421,7 +1396,7 @@ func (s *Server) validateProfileOperation(id profilemanager.ID, allowActiveProfi func (s *Server) logoutFromProfile(ctx context.Context, profile *profilemanager.Profile) error { activeProf, err := s.profileManager.GetActiveProfileState() - if err == nil && activeProf.ID == profile.ID && s.connectClient != nil { + if err == nil && activeProf.ID == profile.ID && s.runs.Current() != nil { return s.sendLogoutRequest(ctx) } @@ -1507,10 +1482,11 @@ func (s *Server) Status( ticker := time.NewTicker(1 * time.Second) defer ticker.Stop() + runDone := s.runs.Done() loop: for { select { - case <-s.clientGiveUpChan: + case <-runDone: ticker.Stop() break loop case <-s.clientRunningChan: @@ -1582,7 +1558,7 @@ func (s *Server) buildStatusResponse(ctx context.Context, msg *proto.StatusReque // getSSHServerState retrieves the current SSH server state including enabled status and active sessions func (s *Server) getSSHServerState() *proto.SSHServerState { s.mutex.Lock() - connectClient := s.connectClient + connectClient := s.runs.Current() s.mutex.Unlock() if connectClient == nil { @@ -1622,7 +1598,7 @@ func (s *Server) GetPeerSSHHostKey( } s.mutex.Lock() - connectClient := s.connectClient + connectClient := s.runs.Current() statusRecorder := s.statusRecorder s.mutex.Unlock() @@ -1806,7 +1782,7 @@ func (s *Server) RequestExtendAuthSession( s.mutex.Lock() config := s.config - connectClient := s.connectClient + connectClient := s.runs.Current() s.mutex.Unlock() if config == nil { @@ -1865,7 +1841,7 @@ func (s *Server) WaitExtendAuthSession( oAuthFlow, authInfo, ok := s.extendAuthSessionFlow.Get() s.mutex.Lock() - connectClient := s.connectClient + connectClient := s.runs.Current() s.mutex.Unlock() if !ok || authInfo.DeviceCode != req.DeviceCode { @@ -1930,7 +1906,7 @@ func (s *Server) DismissSessionWarning( _ *proto.DismissSessionWarningRequest, ) (*proto.DismissSessionWarningResponse, error) { s.mutex.Lock() - connectClient := s.connectClient + connectClient := s.runs.Current() s.mutex.Unlock() if connectClient == nil { return &proto.DismissSessionWarningResponse{}, nil @@ -1948,7 +1924,7 @@ func (s *Server) ExposeService(req *proto.ExposeServiceRequest, srv proto.Daemon s.mutex.Unlock() return gstatus.Errorf(codes.FailedPrecondition, "client is not running, run 'netbird up' first") } - connectClient := s.connectClient + connectClient := s.runs.Current() s.mutex.Unlock() if connectClient == nil { @@ -2001,11 +1977,11 @@ func (s *Server) ExposeService(req *proto.ExposeServiceRequest, srv proto.Daemon } func (s *Server) runProbes(ctx context.Context, waitForProbeResult bool) { - if s.connectClient == nil { + if s.runs.Current() == nil { return } - engine := s.connectClient.Engine() + engine := s.runs.Current().Engine() if engine == nil { return } @@ -2345,20 +2321,25 @@ func (s *Server) checkDisableAdvancedView() *bool { return nil } -func (s *Server) connect(ctx context.Context, config *profilemanager.Config, statusRecorder *peer.Status, runningChan chan struct{}) error { +func (s *Server) connect(ctx context.Context, generation uint64, config *profilemanager.Config, statusRecorder *peer.Status, runningChan chan struct{}) error { log.Tracef("running client connection") client := internal.NewConnectClient(ctx, config, statusRecorder) client.SetUpdateManager(s.updateManager) client.SetSyncResponsePersistence(s.persistSyncResponse) - s.mutex.Lock() - s.connectClient = client - s.mutex.Unlock() + // Publishing before Run is deliberate: the daemon's RPCs reach the engine + // through the supervisor and must work while the connection comes up. + // Publish stops the client this attempt displaces, so the one the previous + // attempt left behind is torn down rather than dropped. + publishCtx, cancel := context.WithTimeout(ctx, displacedStopTimeout) + defer cancel() - if err := client.Run(runningChan, s.logFile); err != nil { - return err + if !s.runs.Publish(publishCtx, generation, client) { + log.Infof("connection attempt superseded, abandoning it") + return nil } - return nil + + return client.Run(runningChan, s.logFile) } // MDM authority: when the platform-native MDM source sets a kill switch diff --git a/client/server/server_connect_test.go b/client/server/server_connect_test.go index 0c6e03a4a..ba320ccbc 100644 --- a/client/server/server_connect_test.go +++ b/client/server/server_connect_test.go @@ -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") diff --git a/client/server/state.go b/client/server/state.go index a4e91468e..ee4bf59ce 100644 --- a/client/server/state.go +++ b/client/server/state.go @@ -38,7 +38,7 @@ func (s *Server) ListStates(_ context.Context, _ *proto.ListStatesRequest) (*pro // CleanState handles cleaning of states (performing cleanup operations) func (s *Server) CleanState(ctx context.Context, req *proto.CleanStateRequest) (*proto.CleanStateResponse, error) { - if s.connectClient != nil && (s.connectClient.Status() == internal.StatusConnected || s.connectClient.Status() == internal.StatusConnecting) { + if s.runs.Current() != nil && (s.runs.Current().Status() == internal.StatusConnected || s.runs.Current().Status() == internal.StatusConnecting) { return nil, status.Errorf(codes.FailedPrecondition, "cannot clean state while connecting or connected, run 'netbird down' first.") } @@ -81,7 +81,7 @@ func (s *Server) CleanState(ctx context.Context, req *proto.CleanStateRequest) ( // DeleteState handles deletion of states without cleanup func (s *Server) DeleteState(ctx context.Context, req *proto.DeleteStateRequest) (*proto.DeleteStateResponse, error) { - if s.connectClient != nil && (s.connectClient.Status() == internal.StatusConnected || s.connectClient.Status() == internal.StatusConnecting) { + if s.runs.Current() != nil && (s.runs.Current().Status() == internal.StatusConnected || s.runs.Current().Status() == internal.StatusConnecting) { return nil, status.Errorf(codes.FailedPrecondition, "cannot clean state while connecting or connected, run 'netbird down' first.") } diff --git a/client/server/trace.go b/client/server/trace.go index 7fea31c49..0bab5681c 100644 --- a/client/server/trace.go +++ b/client/server/trace.go @@ -62,11 +62,11 @@ func (s *Server) TracePacket(_ context.Context, req *proto.TracePacketRequest) ( } func (s *Server) getPacketTracer() (packetTracer, *internal.Engine, error) { - if s.connectClient == nil { + if s.runs.Current() == nil { return nil, nil, fmt.Errorf("connect client not initialized") } - engine := s.connectClient.Engine() + engine := s.runs.Current().Engine() if engine == nil { return nil, nil, fmt.Errorf("engine not initialized") }