[client] Fix engine shutdown deadlock and sync-signal message handling races (#4891)

* Fix engine shutdown deadlock and message handling races

- Release syncMsgMux before waiting for shutdownWg to prevent deadlock
- Check context inside lock in handleSync and receiveSignalEvents
- Prevents nil pointer access when messages arrive during engine stop

client/internal/connect.go

@@ -273,11 +273,12 @@ func (c *ConnectClient) run(mobileDependency MobileDependency, runningChan chan
 		checks := loginResp.GetChecks()
 
 		c.engineMutex.Lock()
-		c.engine = NewEngine(engineCtx, cancel, signalClient, mgmClient, relayManager, engineConfig, mobileDependency, c.statusRecorder, checks)
-		c.engine.SetSyncResponsePersistence(c.persistSyncResponse)
+		engine := NewEngine(engineCtx, cancel, signalClient, mgmClient, relayManager, engineConfig, mobileDependency, c.statusRecorder, checks)
+		engine.SetSyncResponsePersistence(c.persistSyncResponse)
+		c.engine = engine
 		c.engineMutex.Unlock()
 
-		if err := c.engine.Start(loginResp.GetNetbirdConfig(), c.config.ManagementURL); err != nil {
+		if err := engine.Start(loginResp.GetNetbirdConfig(), c.config.ManagementURL); err != nil {
 			log.Errorf("error while starting Netbird Connection Engine: %s", err)
 			return wrapErr(err)
 		}
@@ -293,12 +294,14 @@ func (c *ConnectClient) run(mobileDependency MobileDependency, runningChan chan
 		<-engineCtx.Done()
 
 		c.engineMutex.Lock()
-		engine := c.engine
 		c.engine = nil
 		c.engineMutex.Unlock()
 
-		if engine != nil && engine.wgInterface != nil {
+		// todo: consider to remove this condition. Is not thread safe.
+		// We should always call Stop(), but we need to verify that it is idempotent
+		if engine.wgInterface != nil {
 			log.Infof("ensuring %s is removed, Netbird engine context cancelled", engine.wgInterface.Name())
+
 			if err := engine.Stop(); err != nil {
 				log.Errorf("Failed to stop engine: %v", err)
 			}

client/internal/engine.go

@@ -280,7 +280,6 @@ func (e *Engine) Stop() error {
 		return nil
 	}
 	e.syncMsgMux.Lock()
-	defer e.syncMsgMux.Unlock()
 
 	if e.connMgr != nil {
 		e.connMgr.Close()
@@ -295,7 +294,7 @@ func (e *Engine) Stop() error {
 	if os.Getenv("NB_REMOVE_BEFORE_DNS") == "true" && os.Getenv("NB_REMOVE_BEFORE_ROUTES") != "true" {
 		log.Info("removing peers before dns")
 		if err := e.removeAllPeers(); err != nil {
-			return fmt.Errorf("failed to remove all peers: %s", err)
+			log.Errorf("failed to remove all peers: %s", err)
 		}
 	}
 	if err := e.stopSSHServer(); err != nil {
@@ -319,7 +318,7 @@ func (e *Engine) Stop() error {
 	if os.Getenv("NB_REMOVE_BEFORE_ROUTES") == "true" && os.Getenv("NB_REMOVE_BEFORE_DNS") != "true" {
 		log.Info("removing peers before routes")
 		if err := e.removeAllPeers(); err != nil {
-			return fmt.Errorf("failed to remove all peers: %s", err)
+			log.Errorf("failed to remove all peers: %s", err)
 		}
 	}
 
@@ -338,7 +337,7 @@ func (e *Engine) Stop() error {
 	if os.Getenv("NB_REMOVE_BEFORE_DNS") != "true" && os.Getenv("NB_REMOVE_BEFORE_ROUTES") != "true" {
 		log.Info("removing peers after dns and routes")
 		if err := e.removeAllPeers(); err != nil {
-			return fmt.Errorf("failed to remove all peers: %s", err)
+			log.Errorf("failed to remove all peers: %s", err)
 		}
 	}
 
@@ -353,16 +352,18 @@ func (e *Engine) Stop() error {
 		e.flowManager.Close()
 	}
 
-	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
-	defer cancel()
+	stateCtx, stateCancel := context.WithTimeout(context.Background(), 3*time.Second)
+	defer stateCancel()
 
-	if err := e.stateManager.Stop(ctx); err != nil {
-		return fmt.Errorf("failed to stop state manager: %w", err)
+	if err := e.stateManager.Stop(stateCtx); err != nil {
+		log.Errorf("failed to stop state manager: %v", err)
 	}
 	if err := e.stateManager.PersistState(context.Background()); err != nil {
 		log.Errorf("failed to persist state: %v", err)
 	}
 
+	e.syncMsgMux.Unlock()
+
 	timeout := e.calculateShutdownTimeout()
 	log.Debugf("waiting for goroutines to finish with timeout: %v", timeout)
 	shutdownCtx, cancel := context.WithTimeout(context.Background(), timeout)
@@ -448,8 +449,7 @@ func (e *Engine) Start(netbirdConfig *mgmProto.NetbirdConfig, mgmtURL *url.URL)
 		if err != nil {
 			return fmt.Errorf("create rosenpass manager: %w", err)
 		}
-		err := e.rpManager.Run()
-		if err != nil {
+		if err := e.rpManager.Run(); err != nil {
 			return fmt.Errorf("run rosenpass manager: %w", err)
 		}
 	}
@@ -501,6 +501,7 @@ func (e *Engine) Start(netbirdConfig *mgmProto.NetbirdConfig, mgmtURL *url.URL)
 	}
 
 	if err := e.createFirewall(); err != nil {
+		e.close()
 		return err
 	}
 
@@ -766,6 +767,11 @@ func (e *Engine) handleSync(update *mgmProto.SyncResponse) error {
 	e.syncMsgMux.Lock()
 	defer e.syncMsgMux.Unlock()
 
+	// Check context INSIDE lock to ensure atomicity with shutdown
+	if e.ctx.Err() != nil {
+		return e.ctx.Err()
+	}
+
 	if update.GetNetbirdConfig() != nil {
 		wCfg := update.GetNetbirdConfig()
 		err := e.updateTURNs(wCfg.GetTurns())
@@ -1385,6 +1391,11 @@ func (e *Engine) receiveSignalEvents() {
 			e.syncMsgMux.Lock()
 			defer e.syncMsgMux.Unlock()
 
+			// Check context INSIDE lock to ensure atomicity with shutdown
+			if e.ctx.Err() != nil {
+				return e.ctx.Err()
+			}
+
 			conn, ok := e.peerStore.PeerConn(msg.Key)
 			if !ok {
 				return fmt.Errorf("wrongly addressed message %s", msg.Key)

client/server/server.go

@@ -822,6 +822,7 @@ func (s *Server) Down(ctx context.Context, _ *proto.DownRequest) (*proto.DownRes
 	defer s.mutex.Unlock()
 
 	if err := s.cleanupConnection(); err != nil {
+		// todo review to update the status in case any type of error
 		log.Errorf("failed to shut down properly: %v", err)
 		return nil, err
 	}
@@ -914,6 +915,7 @@ func (s *Server) handleActiveProfileLogout(ctx context.Context) (*proto.LogoutRe
 	}
 
 	if err := s.cleanupConnection(); 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
 	}