state: skip database persist for keepalive-only map requests

A lone LastSeen bump no longer triggers a full-row write and policy rescan.

hscontrol/state/maprequest_noop_test.go

@@ -0,0 +1,74 @@
+package state
+
+import (
+	"strings"
+	"sync/atomic"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
+	"tailscale.com/tailcfg"
+)
+
+// TestNoOpMapRequestSkipsPersist ensures an identical, no-op MapRequest does
+// not issue a database UPDATE (nor the O(n) policy SetNodes scan that follows
+// persistNodeToDB). The node state is unchanged, so persisting is pure waste on
+// the hot map-request path.
+func TestNoOpMapRequestSkipsPersist(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	var nodeUpdateCount atomic.Int64
+
+	gdb := s.DB().DB
+	cbName := "noop_count_node_updates"
+	err := gdb.Callback().Update().After("gorm:update").Register(cbName, func(tx *gorm.DB) {
+		if tx.Statement == nil {
+			return
+		}
+
+		if tx.Statement.Table == "nodes" ||
+			strings.Contains(strings.ToLower(tx.Statement.SQL.String()), "update \"nodes\"") {
+			nodeUpdateCount.Add(1)
+		}
+	})
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = gdb.Callback().Update().Remove(cbName) })
+
+	nv, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok, "node should exist in NodeStore")
+
+	stored := nv.AsStruct()
+
+	req := tailcfg.MapRequest{
+		NodeKey:  stored.NodeKey,
+		DiscoKey: stored.DiscoKey,
+		Hostinfo: &tailcfg.Hostinfo{
+			Hostname: stored.Hostname,
+			NetInfo:  &tailcfg.NetInfo{PreferredDERP: 1},
+		},
+	}
+
+	// First request establishes the Hostinfo/DERP state (expected to persist).
+	_, err = s.UpdateNodeFromMapRequest(nodeID, req)
+	require.NoError(t, err)
+
+	nodeUpdateCount.Store(0)
+
+	// Second request is value-identical: a no-op.
+	req2 := tailcfg.MapRequest{
+		NodeKey:  stored.NodeKey,
+		DiscoKey: stored.DiscoKey,
+		Hostinfo: &tailcfg.Hostinfo{
+			Hostname: stored.Hostname,
+			NetInfo:  &tailcfg.NetInfo{PreferredDERP: 1},
+		},
+	}
+
+	_, err = s.UpdateNodeFromMapRequest(nodeID, req2)
+	require.NoError(t, err)
+
+	require.Equalf(t, int64(0), nodeUpdateCount.Load(),
+		"no-op MapRequest should not issue any nodes-table UPDATE, got %d",
+		nodeUpdateCount.Load())
+}

hscontrol/state/state.go

@@ -2559,6 +2559,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 		autoApprovedRoutes []netip.Prefix
 		endpointChanged    bool
 		derpChanged        bool
+		persistWorthy      bool
 	)
 	// Snapshot the primary assignment so we can tell whether the
 	// Hostinfo + auto-approval that follows shifted any prefix.
@@ -2585,6 +2586,13 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 		// Re-check hostinfoChanged after potential NetInfo preservation
 		hostinfoChanged = !hostinfoEqual(currentNode.View(), req.Hostinfo)
 
+		// A change carrying only an updated LastSeen is not worth a full-row
+		// database UPDATE plus the O(n) policy rescan persistNodeToDB triggers:
+		// LastSeen is best-effort and rides along the next substantive write.
+		// PeerChangeFromMapRequest always stamps LastSeen, so test the other
+		// fields explicitly.
+		persistWorthy = peerChangePersistWorthy(peerChange) || hostinfoChanged
+
 		// If there is no changes and nothing to save,
 		// return early.
 		if peerChangeEmpty(peerChange) && !hostinfoChanged {
@@ -2720,9 +2728,18 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 		nodeRouteChange = change.PolicyChange()
 	}
 
-	_, policyChange, err := s.persistNodeToDB(updatedNode)
-	if err != nil {
-		return change.Change{}, fmt.Errorf("saving to database: %w", err)
+	// A no-op MapRequest (identical re-send / reconnect with matching state)
+	// leaves the node untouched, so skip the full-row UPDATE and the O(n)
+	// policy SetNodes scan that persistNodeToDB performs.
+	policyChange := change.Change{}
+
+	if persistWorthy {
+		var err error
+
+		_, policyChange, err = s.persistNodeToDB(updatedNode)
+		if err != nil {
+			return change.Change{}, fmt.Errorf("saving to database: %w", err)
+		}
 	}
 
 	if !policyChange.IsEmpty() {
@@ -2812,3 +2829,16 @@ func peerChangeEmpty(peerChange tailcfg.PeerChange) bool {
 		peerChange.LastSeen == nil &&
 		peerChange.KeyExpiry == nil
 }
+
+// peerChangePersistWorthy reports whether a peer change carries anything that
+// warrants a database write. It deliberately ignores LastSeen, which
+// [Node.PeerChangeFromMapRequest] always stamps: a keepalive that only bumps
+// LastSeen should not trigger a full-row UPDATE and policy rescan.
+func peerChangePersistWorthy(peerChange tailcfg.PeerChange) bool {
+	return peerChange.Key != nil ||
+		peerChange.DiscoKey != nil ||
+		peerChange.Online != nil ||
+		peerChange.Endpoints != nil ||
+		peerChange.DERPRegion != 0 ||
+		peerChange.KeyExpiry != nil
+}