Avoid locking on clientRunning bool when the connection is aborted for whatever reason

We want to just signal this through the giveUpChan, we will manage the signal from
the waiter side and in case set it to false there. THis way we avoid locking,
which should allow the MDM down+wait_for_term_chan_signal_+up procedure

clientRunning is used to signal two different conditions here:

1. the initialization procedure is over (we have an engine)
2. the connection being up (or being attempted)

Probably these two functionalities should not alias, and the failure of the second condition
(because of any error) should just drive a reconnection (currently it's not happening,
and we silently go idle).
OR, mor probably, the two things are the SAME and there should not exist a case where
we did the "Up" initialization and connection attempt but we are not still attempting it.

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)

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 {

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