diff --git a/client/server/mdm.go b/client/server/mdm.go index c1286b64e..c22bf6979 100644 --- a/client/server/mdm.go +++ b/client/server/mdm.go @@ -36,18 +36,29 @@ var loadMDMPolicy = mdm.LoadPolicy func (s *Server) onMDMPolicyChange(_, curr *mdm.Policy) { log.Warn("MDM policy changed; restarting engine to apply new configuration") + // Hold s.mutex for the entire restart sequence (cancel + quiescence + // wait + re-spawn). Any concurrent Up/Down/Status arriving while + // MDM is restarting blocks on the Lock until we are done — they + // then observe the post-restart state coherently. This is safe + // because the connectWithRetryRuns goroutine no longer acquires + // s.mutex in its defer (intent vs. goroutine-alive concerns are + // fully separated; see the connectionGoroutineRunning helper). s.mutex.Lock() - cancel := s.actCancel - giveUpChan := s.clientGiveUpChan - s.mutex.Unlock() + defer s.mutex.Unlock() - if cancel != nil { - cancel() + if s.actCancel != nil { + s.actCancel() } // Wait for previous connectWithRetryRuns to exit so we don't end up // with two goroutines fighting over the same status recorder + engine. - if giveUpChan != nil { + // 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, DisableAutoConnect single-shot, + // backoff exhausted, panic) — see the defer in server.go. + if s.clientGiveUpChan != nil { + giveUpChan := s.clientGiveUpChan select { case <-giveUpChan: case <-time.After(5 * time.Second): @@ -55,15 +66,15 @@ func (s *Server) onMDMPolicyChange(_, curr *mdm.Policy) { } } - if err := s.restartEngineForMDM(); err != nil { + if err := s.restartEngineForMDMLocked(); err != nil { log.Errorf("MDM restart failed: %v", err) return } // publishConfigChangedEvent has already fired inside - // restartEngineForMDM with source="mdm". Here we additionally emit an - // MDM-specific user-visible toast so the operator knows their IT - // policy was applied (UserMessage != "" triggers the GUI notifier). + // restartEngineForMDMLocked with source="mdm". Emit an MDM-specific + // user-visible toast so the operator knows their IT policy was + // applied (UserMessage != "" triggers the GUI notifier). _ = curr s.statusRecorder.PublishEvent( proto.SystemEvent_INFO, @@ -105,11 +116,16 @@ func (s *Server) publishConfigChangedEvent(source string) { ) } -// restartEngineForMDM re-resolves the active profile config (re-running -// applyMDMPolicy via Config.apply) and re-spawns connectWithRetryRuns. -// Mirrors the tail of Server.Start so a runtime MDM change behaves -// identically to a fresh boot under the new policy. -func (s *Server) restartEngineForMDM() error { +// restartEngineForMDMLocked re-resolves the active profile config +// (re-running applyMDMPolicy via Config.apply) and re-spawns +// connectWithRetryRuns. Mirrors the tail of Server.Start so a runtime +// MDM change behaves identically to a fresh boot under the new policy. +// +// MUST be called with s.mutex held — onMDMPolicyChange holds the lock +// for the entire restart sequence (cancel + quiescence wait + re-spawn) +// so concurrent Up/Down/Status RPCs observe a coherent post-restart +// state. +func (s *Server) restartEngineForMDMLocked() error { activeProf, err := s.profileManager.GetActiveProfileState() if err != nil { return fmt.Errorf("get active profile state: %w", err) @@ -119,9 +135,6 @@ func (s *Server) restartEngineForMDM() error { return fmt.Errorf("get active profile config: %w", err) } - s.mutex.Lock() - defer s.mutex.Unlock() - s.config = config s.statusRecorder.UpdateManagementAddress(config.ManagementURL.String()) s.statusRecorder.UpdateRosenpass(config.RosenpassEnabled, config.RosenpassPermissive) diff --git a/client/server/server.go b/client/server/server.go index e00a93fff..7171ca187 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -72,7 +72,13 @@ type Server struct { mutex sync.Mutex config *profilemanager.Config proto.UnimplementedDaemonServiceServer - clientRunning bool // protected by mutex + // clientRunning tracks "the daemon wants to be connected" — set true by + // 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. + clientRunning bool clientRunningChan chan struct{} clientGiveUpChan chan struct{} // closed when connectWithRetryRuns goroutine exits @@ -237,11 +243,20 @@ func (s *Server) Start() error { // connectWithRetryRuns runs the client connection with a backoff strategy where we retry the operation as additional // mechanism to keep the client connected even when the connection is lost. // we cancel retry if the client receive a stop or down command, or if disable auto connect is configured. +// +// The goroutine's exit is signalled to the daemon via close(giveUpChan) +// — placed in the function-scope defer so every return path (panic, +// DisableAutoConnect early-exit, backoff exhausted, ctx cancel) closes +// it. Callers that need to observe "is the goroutine still alive?" use +// Server.connectionGoroutineRunning() which non-blockingly checks the close state +// 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{}) { defer func() { - s.mutex.Lock() - s.clientRunning = false - s.mutex.Unlock() + if giveUpChan != nil { + close(giveUpChan) + } }() if s.config.DisableAutoConnect { @@ -287,9 +302,26 @@ 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. +} - if giveUpChan != nil { - close(giveUpChan) +// 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. +func (s *Server) connectionGoroutineRunning() bool { + if s.clientGiveUpChan == nil { + return false + } + select { + case <-s.clientGiveUpChan: + return false + default: + return true } } @@ -709,7 +741,13 @@ func (s *Server) WaitSSOLogin(callerCtx context.Context, msg *proto.WaitSSOLogin // Up starts engine work in the daemon. func (s *Server) Up(callerCtx context.Context, msg *proto.UpRequest) (*proto.UpResponse, error) { s.mutex.Lock() - if s.clientRunning { + // clientRunning is the daemon-intent flag (set by previous Up/Start, cleared + // by Down). connectionGoroutineRunning() reports whether the previous retry-loop + // goroutine is still trying. When intent is up AND goroutine is alive, + // the existing engine is on the job — just wait for it. When intent + // is up but the goroutine has given up (backoff exhausted) OR when + // intent is down, fall through to spawn a fresh retry loop. + if s.clientRunning && s.connectionGoroutineRunning() { state := internal.CtxGetState(s.rootCtx) status, err := state.Status() if err != nil { @@ -929,6 +967,12 @@ func (s *Server) cleanupConnection() error { return ErrServiceNotUp } + // Daemon intent flips to "down" — all callers (Down RPC, + // Logout RPC handlers) tear down the connection because the user + // explicitly asked for it. MDM restart does NOT go through this + // 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() @@ -1132,10 +1176,14 @@ func (s *Server) Status( msg *proto.StatusRequest, ) (*proto.StatusResponse, error) { s.mutex.Lock() - clientRunning := s.clientRunning + // Only wait if the retry-loop goroutine is alive and making + // progress. clientRunning=true with connectionGoroutineRunning=false means the + // backoff has given up — there is nothing to wait for; let the + // caller observe the failed status directly. + alive := s.connectionGoroutineRunning() s.mutex.Unlock() - if msg.WaitForReady != nil && *msg.WaitForReady && clientRunning { + if msg.WaitForReady != nil && *msg.WaitForReady && alive { state := internal.CtxGetState(s.rootCtx) status, err := state.Status() if err != nil { diff --git a/client/server/server_connect_test.go b/client/server/server_connect_test.go index faea7da39..0c6e03a4a 100644 --- a/client/server/server_connect_test.go +++ b/client/server/server_connect_test.go @@ -101,6 +101,7 @@ func TestCleanupConnection_ClearsConnectClient(t *testing.T) { require.NoError(t, err) assert.Nil(t, s.connectClient, "connectClient should be nil after cleanup") + assert.False(t, s.clientRunning, "clientRunning should be cleared after cleanup (intent = down)") } // TestCleanState_NilConnectClient validates that CleanState doesn't panic @@ -144,17 +145,20 @@ func TestDownThenUp_StaleRunningChan(t *testing.T) { _, cancel := context.WithCancel(context.Background()) s.actCancel = cancel - // Simulate Down(): cleanupConnection sets connectClient = nil + // Simulate Down(): cleanupConnection sets connectClient = nil and + // flips clientRunning to false (intent = down). The connectionGoroutineRunning state + // remains independent of intent — derived from clientGiveUpChan. s.mutex.Lock() err := s.cleanupConnection() s.mutex.Unlock() require.NoError(t, err) - // After cleanup: connectClient is nil, clientRunning still true - // (goroutine hasn't exited yet) + // 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.True(t, s.clientRunning, "clientRunning still true until goroutine exits") + assert.False(t, s.clientRunning, "clientRunning should be cleared by cleanupConnection (intent = down)") s.mutex.Unlock() // waitForUp() returns immediately due to stale closed clientRunningChan