online status from nodestore

hscontrol/mapper/mapper.go

@@ -18,7 +18,6 @@ import (
 	"tailscale.com/envknob"
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/dnstype"
-	"tailscale.com/types/ptr"
 	"tailscale.com/types/views"
 )
 
@@ -50,37 +49,6 @@ type mapper struct {
 	created time.Time
 }
 
-// addOnlineStatusToPeers adds fresh online status from batcher to peer nodes.
-//
-// We do a last-minute copy-and-write on the NodeView to inject current online status
-// from the batcher's connection map. Online status is not populated upstream in NodeStore
-// for consistency reasons - it's runtime connection state that should come from the
-// connection manager (batcher) to ensure map responses have the freshest data.
-func (m *mapper) addOnlineStatusToPeers(peers views.Slice[types.NodeView]) views.Slice[types.NodeView] {
-	if peers.Len() == 0 || m.batcher == nil {
-		return peers
-	}
-
-	result := make([]types.NodeView, 0, peers.Len())
-	for _, peer := range peers.All() {
-		if !peer.Valid() {
-			result = append(result, peer)
-			continue
-		}
-
-		// Get online status from batcher connection map
-		// The batcher respects grace periods for logout scenarios
-		isOnline := m.batcher.IsConnected(peer.ID())
-
-		// Create a mutable copy and set online status
-		peerCopy := peer.AsStruct()
-		peerCopy.IsOnline = ptr.To(isOnline)
-		result = append(result, peerCopy.View())
-	}
-
-	return views.SliceOf(result)
-}
-
 type patch struct {
 	timestamp time.Time
 	change    *tailcfg.PeerChange
@@ -174,9 +142,6 @@ func (m *mapper) fullMapResponse(
 ) (*tailcfg.MapResponse, error) {
 	peers := m.state.ListPeers(nodeID)
 
-	// Add fresh online status to peers from batcher connection state
-	// peersWithOnlineStatus := m.addOnlineStatusToPeers(peers)
-
 	return m.NewMapResponseBuilder(nodeID).
 		WithCapabilityVersion(capVer).
 		WithSelfNode().
@@ -219,9 +184,6 @@ func (m *mapper) peerChangeResponse(
 ) (*tailcfg.MapResponse, error) {
 	peers := m.state.ListPeers(nodeID, changedNodeID)
 
-	// Add fresh online status to peers from batcher connection state
-	// peersWithOnlineStatus := m.addOnlineStatusToPeers(peers)
-
 	return m.NewMapResponseBuilder(nodeID).
 		WithCapabilityVersion(capVer).
 		WithSelfNode().

hscontrol/state/state.go

@@ -439,10 +439,22 @@ func (s *State) Connect(id types.NodeID) change.ChangeSet {
 	c := change.NodeOnline(id)
 
 	// Update the online status in NodeStore
+	now := time.Now()
 	s.nodeStore.UpdateNode(id, func(n *types.Node) {
 		n.IsOnline = ptr.To(true)
+		n.LastSeen = ptr.To(now)
 	})
 	
+	// Also persist the last seen time to the database
+	// Note: IsOnline is managed only in NodeStore (marked with gorm:"-"), not persisted to database
+	_, err := s.updateNodeTx(id, func(tx *gorm.DB) error {
+		// Update last_seen in the database
+		return hsdb.SetLastSeen(tx, id, now)
+	})
+	if err != nil {
+		log.Error().Err(err).Uint64("node.id", id.Uint64()).Msg("Failed to update last seen time in database")
+	}
+
 	// Get fresh node data from NodeStore after the online status update
 	node, found := s.GetNodeByID(id)
 	if !found {
@@ -470,22 +482,28 @@ func (s *State) Disconnect(id types.NodeID) (change.ChangeSet, error) {
 	now := time.Now()
 	s.nodeStore.UpdateNode(id, func(n *types.Node) {
 		n.LastSeen = ptr.To(now)
-		// Mark as offline immediately in NodeStore - this is the source of truth
+		// CRITICAL: Mark as offline immediately in NodeStore.
-		// The batcher's grace period will still apply when sending to clients
+		// NodeStore is the source of truth for all node state including online status.
 		n.IsOnline = ptr.To(false)
 	})
 
 	_, err := s.updateNodeTx(id, func(tx *gorm.DB) error {
+		// Update last_seen in the database
+		// Note: IsOnline is managed only in NodeStore (marked with gorm:"-"), not persisted to database
 		return hsdb.SetLastSeen(tx, id, now)
 	})
 	if err != nil {
-		return change.EmptySet, fmt.Errorf("setting last seen: %w", err)
+		// Log error but don't fail the disconnection - NodeStore is already updated
+		// and we need to send change notifications to peers
+		log.Error().Err(err).Uint64("node.id", id.Uint64()).Msg("Failed to update last seen in database")
 	}
 
 	// Check if policy manager needs updating
 	c, err := s.updatePolicyManagerNodes()
 	if err != nil {
-		return change.EmptySet, fmt.Errorf("failed to update policy manager after node update: %w", err)
+		// Log error but continue - disconnection must proceed
+		log.Error().Err(err).Uint64("node.id", id.Uint64()).Msg("Failed to update policy manager after node disconnect")
+		c = change.EmptySet
 	}
 
 	// The node is disconnecting so make sure that none of the routes it