diff --git a/hscontrol/state/maprequest_noop_test.go b/hscontrol/state/maprequest_noop_test.go new file mode 100644 index 00000000..6783e540 --- /dev/null +++ b/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()) +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index d21e0c02..cb9cf508 100644 --- a/hscontrol/state/state.go +++ b/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 +}