state: classify map requests before broadcasting

Each request is reduced to the narrowest change it justifies, so a
keepalive or endpoint bump no longer resends the whole node to peers.

Updates #3417

hscontrol/state/endpoint_test.go

@@ -187,6 +187,23 @@ func TestEndpointBroadcastWorthy(t *testing.T) {
 			newType: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointSTUN, tailcfg.EndpointLocal},
 			want:    true,
 		},
+		{
+			// Peers hold no endpoints for this node yet, and a
+			// suppressed delta is never resent, so the first set is
+			// the only one they would ever get.
+			name:    "first endpoints ever, STUN only - broadcast",
+			stored:  nil,
+			newEPs:  []netip.AddrPort{stun},
+			newType: []tailcfg.EndpointType{tailcfg.EndpointSTUN},
+			want:    true,
+		},
+		{
+			name:    "no stored and none announced - suppress",
+			stored:  nil,
+			newEPs:  nil,
+			newType: nil,
+			want:    false,
+		},
 	}
 
 	for _, tt := range tests {

hscontrol/state/maprequest.go

@@ -6,10 +6,78 @@ package state
 
 import (
 	"github.com/juanfont/headscale/hscontrol/types"
+	"github.com/juanfont/headscale/hscontrol/util/zlog/zf"
+	"github.com/rs/zerolog"
 	"github.com/rs/zerolog/log"
 	"tailscale.com/tailcfg"
 )
 
+// mapRequestDelta carries the classified facts extracted from one MapRequest
+// against the currently-stored node. It separates the raw wire-level peer
+// change (what the client actually sent) from the broadcast/persist/policy
+// decisions made from it, so that each downstream decision (broadcast, persist,
+// policy refresh, relation rebuild) sees only its own input.
+//
+// Construction happens once per MapRequest inside the NodeStore write callback
+// (so comparisons run against serialized current state); classification happens
+// after the write succeeds. Splitting these avoids redoing comparisons and
+// avoids having the broadcast classifier depend on incidental branch order.
+type mapRequestDelta struct {
+	// peerChange is the wire-level delta produced by
+	// [types.Node.PeerChangeFromMapRequest] - LastSeen is always stamped.
+	peerChange tailcfg.PeerChange
+
+	// hostinfoChanged reports whether anything in Hostinfo changed apart
+	// from PreferredDERP (tracked by derpChanged) and DERP latency jitter.
+	// It gates storing and persisting the new Hostinfo.
+	hostinfoChanged bool
+
+	// peerHostinfoChanged reports whether a Hostinfo field that peers read
+	// changed (see [peerHostinfo]). Only that forces a whole-node resend.
+	peerHostinfoChanged bool
+
+	// routesChanged reports whether announced routes (RoutableIPs)
+	// changed. Routes are policy and election inputs, so they are tracked
+	// on their own.
+	routesChanged bool
+
+	// derpChanged reports whether PreferredDERP changed. oldDERP/newDERP
+	// carry the values; DERP zero means "unchanged" on the wire, so
+	// clearing DERP to zero cannot be sent as a patch and forces a
+	// whole-peer update.
+	derpChanged      bool
+	oldDERP, newDERP tailcfg.DERPRegionID
+
+	// endpointBroadcast reports whether the endpoint delta is worth fanning
+	// out (i.e., a newly-added useful non-STUN endpoint). Storage still
+	// happens regardless; this gates only broadcast.
+	endpointBroadcast bool
+
+	// keyChanged and discoKeyChanged report whether the node's wire keys
+	// changed. Key patches already carry the resulting endpoints/expiry,
+	// so a key change subsumes endpoint/DERP patches.
+	keyChanged      bool
+	discoKeyChanged bool
+
+	// persistWorthy reports whether the request carries data that should
+	// hit the database. LastSeen-only updates are not persist-worthy.
+	persistWorthy bool
+}
+
+// MarshalZerologObject implements [zerolog.LogObjectMarshaler].
+func (d mapRequestDelta) MarshalZerologObject(e *zerolog.Event) {
+	e.Bool("hostinfo.changed", d.hostinfoChanged).
+		Bool("hostinfo.peer_changed", d.peerHostinfoChanged).
+		Bool("routes.changed", d.routesChanged).
+		Bool("derp.changed", d.derpChanged).
+		Int("derp.old", int(d.oldDERP)).
+		Int("derp.new", int(d.newDERP)).
+		Bool("endpoint.broadcast", d.endpointBroadcast).
+		Bool("key.changed", d.keyChanged).
+		Bool("disco_key.changed", d.discoKeyChanged).
+		Bool("persist", d.persistWorthy)
+}
+
 // netInfoFromMapRequest determines the correct [tailcfg.NetInfo] to use.
 // Returns the [tailcfg.NetInfo] that should be used for this request.
 func netInfoFromMapRequest(
@@ -26,8 +94,8 @@ func netInfoFromMapRequest(
 	if currentHostinfo != nil && currentHostinfo.NetInfo != nil {
 		log.Debug().
 			Caller().
-			Uint64("node.id", nodeID.Uint64()).
-			Int64("preferredDERP", currentHostinfo.NetInfo.PreferredDERP.Int64()).
+			Uint64(zf.NodeID, nodeID.Uint64()).
+			Int64(zf.DERP, currentHostinfo.NetInfo.PreferredDERP.Int64()).
 			Msg("using NetInfo from previous Hostinfo in MapRequest")
 
 		return currentHostinfo.NetInfo
@@ -43,9 +111,87 @@ func netInfoFromMapRequest(
 
 	log.Debug().
 		Caller().
-		Uint64("node.id", nodeID.Uint64()).
-		Str("node.hostname", hostname).
+		Uint64(zf.NodeID, nodeID.Uint64()).
+		Str(zf.Hostname, hostname).
 		Msg("node sent update but has no NetInfo in request or database")
 
 	return nil
 }
+
+// hostinfoDERP returns the PreferredDERP value from a Hostinfo, or 0 when
+// either pointer is nil. 0 is the wire "unchanged" sentinel.
+func hostinfoDERP(hi *tailcfg.Hostinfo) tailcfg.DERPRegionID {
+	if hi == nil || hi.NetInfo == nil {
+		return 0
+	}
+
+	return hi.NetInfo.PreferredDERP
+}
+
+// hostinfoEqual reports whether two Hostinfo values are the same apart from
+// PreferredDERP, which the caller tracks on its own, and DERP latency
+// jitter, which [tailcfg.NetInfo.BasicallyEqual] skips.
+func hostinfoEqual(oldHI, newHI *tailcfg.Hostinfo) bool {
+	if oldHI == nil || newHI == nil {
+		return oldHI == newHI
+	}
+
+	if !netInfoEqualIgnoringDERP(oldHI.NetInfo, newHI.NetInfo) {
+		return false
+	}
+
+	// NetInfo is compared above; drop it so nil-vs-empty inside it does
+	// not leak through reflect.DeepEqual.
+	oldCopy := *oldHI
+	oldCopy.NetInfo = nil
+	newCopy := *newHI
+	newCopy.NetInfo = nil
+
+	return oldCopy.Equal(&newCopy)
+}
+
+// peerHostinfo keeps the Hostinfo fields another node's client reads from
+// a peer: what tailscale status shows, PeerAPI services, SSH known hosts,
+// exit-node location, app-connector eligibility, and the routes this server
+// feeds into policy. Everything else is stored but never fanned out, and a
+// peer's NetInfo is never read at all.
+func peerHostinfo(hi *tailcfg.Hostinfo) *tailcfg.Hostinfo {
+	if hi == nil {
+		return nil
+	}
+
+	return &tailcfg.Hostinfo{
+		Hostname:     hi.Hostname,
+		OS:           hi.OS,
+		Services:     hi.Services,
+		SSH_HostKeys: hi.SSH_HostKeys,
+		Location:     hi.Location,
+		AppConnector: hi.AppConnector,
+		RoutableIPs:  hi.RoutableIPs,
+	}
+}
+
+// peerHostinfoEqual reports whether the fields peers read are unchanged.
+func peerHostinfoEqual(oldHI, newHI *tailcfg.Hostinfo) bool {
+	return peerHostinfo(oldHI).Equal(peerHostinfo(newHI))
+}
+
+// netInfoEqualIgnoringDERP compares two NetInfo values via
+// [tailcfg.NetInfo.BasicallyEqual] after zeroing PreferredDERP on both, so
+// that DERP-only changes do not appear here (they are tracked separately).
+func netInfoEqualIgnoringDERP(old, current *tailcfg.NetInfo) bool {
+	if old == nil && current == nil {
+		return true
+	}
+
+	if (old == nil) != (current == nil) {
+		return false
+	}
+
+	oldCopy := *old
+	oldCopy.PreferredDERP = 0
+	currentCopy := *current
+	currentCopy.PreferredDERP = 0
+
+	return oldCopy.BasicallyEqual(&currentCopy)
+}

hscontrol/state/maprequest_noop_test.go

@@ -1,74 +0,0 @@
-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
-// persistNodeAndRefreshPolicy). 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/maprequest_test.go

@@ -1,12 +1,21 @@
 package state
 
 import (
+	"net/netip"
+	"strings"
+	"sync"
+	"sync/atomic"
 	"testing"
+	"time"
 
 	"github.com/juanfont/headscale/hscontrol/types"
+	"github.com/juanfont/headscale/hscontrol/types/change"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
 	"tailscale.com/tailcfg"
+	"tailscale.com/types/key"
+	"tailscale.com/types/opt"
 )
 
 func TestNetInfoFromMapRequest(t *testing.T) {
@@ -133,3 +142,869 @@ func TestNetInfoPreservationInRegistrationFlow(t *testing.T) {
 		assert.Equal(t, tailcfg.DERPRegionID(7), result.PreferredDERP, "Should preserve DERP region from existing node")
 	})
 }
+
+// TestNoOpMapRequestSkipsPersist ensures an identical, no-op MapRequest does
+// not issue a database UPDATE (nor the O(n) policy SetNodes scan that follows
+// persistNodeAndRefreshPolicy). 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())
+}
+
+// TestNoOpMapRequestEmitsNoPeerChange ensures an identical, no-op MapRequest
+// does not emit a peer-visible change.
+func TestNoOpMapRequestEmitsNoPeerChange(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	nv, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok, "node should exist in NodeStore")
+
+	stored := nv.AsStruct()
+
+	req := func() tailcfg.MapRequest {
+		return 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.
+	_, err := s.UpdateNodeFromMapRequest(nodeID, req())
+	require.NoError(t, err)
+
+	beforeSeen := mustLastSeen(t, s, nodeID)
+
+	// Second request is value-identical: a no-op.
+	c, err := s.UpdateNodeFromMapRequest(nodeID, req())
+	require.NoError(t, err)
+
+	require.Truef(t, c.IsEmpty(),
+		"no-op MapRequest must not emit a change, got reason=%q type=%q peersChanged=%v",
+		c.Reason, c.Type(), c.PeersChanged)
+
+	require.True(t, mustLastSeen(t, s, nodeID).After(beforeSeen),
+		"no-op request must still stamp LastSeen in the NodeStore")
+}
+
+// TestSTUNOnlyEndpointUpdateEmitsNoPeerChange ensures endpoint churn that
+// endpointBroadcastWorthy suppresses stays suppressed instead of escalating
+// to a whole-peer change.
+func TestSTUNOnlyEndpointUpdateEmitsNoPeerChange(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	nv, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok, "node should exist in NodeStore")
+
+	stored := nv.AsStruct()
+
+	hi := func() *tailcfg.Hostinfo {
+		return &tailcfg.Hostinfo{
+			Hostname: stored.Hostname,
+			NetInfo:  &tailcfg.NetInfo{PreferredDERP: 1},
+		}
+	}
+
+	local := netip.MustParseAddrPort("192.168.1.5:41641")
+
+	// First request establishes the Hostinfo/DERP state and a useful
+	// endpoint, so the delta below is churn on a set peers already hold
+	// rather than the node's first endpoints.
+	_, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{
+		NodeKey:       stored.NodeKey,
+		DiscoKey:      stored.DiscoKey,
+		Hostinfo:      hi(),
+		Endpoints:     []netip.AddrPort{local},
+		EndpointTypes: []tailcfg.EndpointType{tailcfg.EndpointLocal},
+	})
+	require.NoError(t, err)
+
+	// Second request adds a single STUN-derived endpoint and nothing else.
+	c, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{
+		NodeKey:       stored.NodeKey,
+		DiscoKey:      stored.DiscoKey,
+		Hostinfo:      hi(),
+		Endpoints:     []netip.AddrPort{local, netip.MustParseAddrPort("198.51.100.7:41641")},
+		EndpointTypes: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointSTUN},
+	})
+	require.NoError(t, err)
+
+	require.Truef(t, c.IsEmpty(),
+		"suppressed STUN-only endpoint delta must not emit a change, got reason=%q type=%q peersChanged=%v",
+		c.Reason, c.Type(), c.PeersChanged)
+
+	after, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok)
+	require.Contains(t, after.Endpoints().AsSlice(), netip.MustParseAddrPort("198.51.100.7:41641"),
+		"suppressed endpoint must still be stored")
+}
+
+// TestFirstEndpointsReachPeersEvenWhenSTUNOnly pins that the first endpoint
+// set a node announces is always broadcast. Peers hold no endpoints for it
+// yet, and a suppressed delta is never resent, so suppressing the first set
+// leaves peers with no direct path for the life of the node.
+func TestFirstEndpointsReachPeersEvenWhenSTUNOnly(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	nv, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok, "node should exist in NodeStore")
+	require.Empty(t, nv.Endpoints().AsSlice(), "precondition: node starts with no endpoints")
+
+	stored := nv.AsStruct()
+	stun := netip.MustParseAddrPort("198.51.100.7:41641")
+
+	c, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{
+		NodeKey:       stored.NodeKey,
+		DiscoKey:      stored.DiscoKey,
+		Endpoints:     []netip.AddrPort{stun},
+		EndpointTypes: []tailcfg.EndpointType{tailcfg.EndpointSTUN},
+	})
+	require.NoError(t, err)
+
+	require.Falsef(t, c.IsEmpty(),
+		"a node's first endpoints must reach peers, got reason=%q type=%q", c.Reason, c.Type())
+	require.Len(t, c.PeerPatches, 1, "expected a single endpoint patch")
+	require.Contains(t, c.PeerPatches[0].Endpoints, stun,
+		"the patch must carry the announced endpoint")
+}
+
+// TestMapRequestDERPLatencyJitterEmitsNoPeerChange pins the wiring between
+// [State.UpdateNodeFromMapRequest] and the Hostinfo comparison: DERP latency
+// is transport diagnostics that no peer reads, so jitter must not reach the
+// classifier as a change. hostinfoEqual covers the comparison itself; this
+// covers the path.
+func TestMapRequestDERPLatencyJitterEmitsNoPeerChange(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	nv, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok, "node should exist in NodeStore")
+
+	stored := nv.AsStruct()
+
+	hi := func(latency float64) *tailcfg.Hostinfo {
+		return &tailcfg.Hostinfo{
+			Hostname: stored.Hostname,
+			NetInfo: &tailcfg.NetInfo{
+				PreferredDERP: 1,
+				DERPLatency:   map[string]float64{"1-v4": latency},
+			},
+		}
+	}
+
+	// Establish the Hostinfo, DERP region and a first latency sample.
+	_, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{
+		NodeKey:  stored.NodeKey,
+		DiscoKey: stored.DiscoKey,
+		Hostinfo: hi(0.010),
+	})
+	require.NoError(t, err)
+
+	// Only the latency sample moves.
+	c, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{
+		NodeKey:  stored.NodeKey,
+		DiscoKey: stored.DiscoKey,
+		Hostinfo: hi(0.025),
+	})
+	require.NoError(t, err)
+
+	require.Truef(t, c.IsEmpty(),
+		"DERP latency jitter must not emit a change, got reason=%q type=%q peersChanged=%v",
+		c.Reason, c.Type(), c.PeersChanged)
+
+	after, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok)
+	require.Equal(t, 1, int(after.Hostinfo().NetInfo().PreferredDERP()),
+		"the DERP region must survive a latency-only request")
+}
+
+// TestMapRequestOmittedNetInfoIsNoChange pins that a MapRequest carrying
+// Hostinfo with NetInfo omitted is compared against the preserved NetInfo, not
+// against the bare request. Tailscale clients send NetInfo only when it
+// changed, so classifying the omission as a Hostinfo change turns
+// every routine map request into a database write, an O(n) policy rescan and a
+// whole-peer broadcast.
+func TestMapRequestOmittedNetInfoIsNoChange(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	var nodeUpdateCount atomic.Int64
+
+	gdb := s.DB().DB
+	cbName := "omitted_netinfo_count_node_updates"
+	err := gdb.Callback().Update().After("gorm:update").Register(cbName, func(tx *gorm.DB) {
+		if tx.Statement != nil && tx.Statement.Table == "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)
+
+	stored := nv.AsStruct()
+
+	// Establish Hostinfo with NetInfo.
+	_, err = s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{
+		NodeKey:  stored.NodeKey,
+		DiscoKey: stored.DiscoKey,
+		Hostinfo: &tailcfg.Hostinfo{
+			Hostname: stored.Hostname,
+			OS:       "linux",
+			NetInfo:  &tailcfg.NetInfo{PreferredDERP: 1},
+		},
+	})
+	require.NoError(t, err)
+	require.Positive(t, nodeUpdateCount.Load(), "first request must persist")
+
+	nodeUpdateCount.Store(0)
+
+	// Same Hostinfo, NetInfo omitted: the client is saying "unchanged".
+	c, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{
+		NodeKey:  stored.NodeKey,
+		DiscoKey: stored.DiscoKey,
+		Hostinfo: &tailcfg.Hostinfo{
+			Hostname: stored.Hostname,
+			OS:       "linux",
+		},
+	})
+	require.NoError(t, err)
+
+	require.True(t, c.IsEmpty(), "omitted NetInfo must not broadcast, got %+v", c)
+	require.Equalf(t, int64(0), nodeUpdateCount.Load(),
+		"omitted NetInfo must not persist, got %d updates", nodeUpdateCount.Load())
+
+	after, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok)
+	require.Equal(t, tailcfg.DERPRegionID(1), hostinfoDERP(after.AsStruct().Hostinfo),
+		"stored NetInfo must survive a request that omits it")
+}
+
+// TestMapRequestDERPClearToZeroIsStoredAndBroadcast pins that a node reporting
+// PreferredDERP 0 has its home region cleared. tailcfg.PeerChange.DERPRegion
+// zero means "unchanged" on the wire, so the clear cannot ride a patch and has
+// to escalate to a whole-peer update.
+func TestMapRequestDERPClearToZeroIsStoredAndBroadcast(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	var nodeUpdateCount atomic.Int64
+
+	gdb := s.DB().DB
+	cbName := "derp_clear_count_node_updates"
+	err := gdb.Callback().Update().After("gorm:update").Register(cbName, func(tx *gorm.DB) {
+		if tx.Statement != nil && tx.Statement.Table == "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)
+
+	stored := nv.AsStruct()
+
+	_, err = s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{
+		NodeKey:  stored.NodeKey,
+		DiscoKey: stored.DiscoKey,
+		Hostinfo: &tailcfg.Hostinfo{
+			Hostname: stored.Hostname,
+			NetInfo:  &tailcfg.NetInfo{PreferredDERP: 1},
+		},
+	})
+	require.NoError(t, err)
+
+	nodeUpdateCount.Store(0)
+
+	c, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{
+		NodeKey:  stored.NodeKey,
+		DiscoKey: stored.DiscoKey,
+		Hostinfo: &tailcfg.Hostinfo{
+			Hostname: stored.Hostname,
+			NetInfo:  &tailcfg.NetInfo{PreferredDERP: 0},
+		},
+	})
+	require.NoError(t, err)
+
+	after, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok)
+	require.Equal(t, tailcfg.DERPRegionID(0), hostinfoDERP(after.AsStruct().Hostinfo),
+		"clearing PreferredDERP must be stored")
+
+	require.Contains(t, c.PeersChanged, nodeID,
+		"clearing PreferredDERP cannot be a patch, got %+v", c)
+	require.Empty(t, c.PeerPatches,
+		"clearing PreferredDERP must not emit a DERP patch, got %+v", c)
+	require.Positive(t, nodeUpdateCount.Load(), "clearing PreferredDERP must be persisted")
+}
+
+// TestMapRequestDERPOnlyChangeKeepsGivenName pins that a request changing only
+// PreferredDERP does not re-derive GivenName. Peers only receive a DERP patch
+// for such a request, so a rename here would never reach them.
+func TestMapRequestDERPOnlyChangeKeepsGivenName(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	nv, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok)
+
+	stored := nv.AsStruct()
+
+	req := func(derp tailcfg.DERPRegionID) tailcfg.MapRequest {
+		return tailcfg.MapRequest{
+			NodeKey:  stored.NodeKey,
+			DiscoKey: stored.DiscoKey,
+			Hostinfo: &tailcfg.Hostinfo{
+				Hostname: stored.Hostname,
+				NetInfo:  &tailcfg.NetInfo{PreferredDERP: derp},
+			},
+		}
+	}
+
+	_, err := s.UpdateNodeFromMapRequest(nodeID, req(1))
+	require.NoError(t, err)
+
+	// A collision-bumped name whose base is free again is exactly what the
+	// auto-derive path rewrites.
+	bumped := stored.Hostname + "-1"
+	_, ok = s.nodeStore.UpdateNode(nodeID, func(n *types.Node) { n.GivenName = bumped })
+	require.True(t, ok)
+
+	c, err := s.UpdateNodeFromMapRequest(nodeID, req(2))
+	require.NoError(t, err)
+	require.NotEmpty(t, c.PeerPatches, "DERP-only change must be a patch, got %+v", c)
+
+	after, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok)
+	require.Equal(t, bumped, after.GivenName(), "DERP-only change must not touch GivenName")
+}
+
+func TestConcurrentMapRequestDERPUsesPersistedState(t *testing.T) {
+	for _, rotateKey := range []bool{false, true} {
+		for _, tt := range []struct {
+			name                string
+			firstDERP, lastDERP tailcfg.DERPRegionID
+		}{
+			{name: "newer region", firstDERP: 2, lastDERP: 3},
+			{name: "newer clear", firstDERP: 2, lastDERP: 0},
+			{name: "superseded clear", firstDERP: 0, lastDERP: 3},
+		} {
+			name := tt.name
+			if rotateKey {
+				name += " with key rotation"
+			}
+
+			t.Run(name, func(t *testing.T) {
+				_, s, nodeID := persistTestSetup(t)
+				t.Cleanup(func() { _ = s.Close() })
+
+				nv, ok := s.GetNodeByID(nodeID)
+				require.True(t, ok)
+
+				req := tailcfg.MapRequest{
+					NodeKey:  nv.NodeKey(),
+					DiscoKey: nv.DiscoKey(),
+					Hostinfo: &tailcfg.Hostinfo{
+						Hostname: nv.Hostname(),
+						NetInfo:  &tailcfg.NetInfo{PreferredDERP: 1},
+					},
+				}
+				_, err := s.UpdateNodeFromMapRequest(nodeID, req)
+				require.NoError(t, err)
+
+				if rotateKey {
+					req.NodeKey = key.NewNode().Public()
+					req.DiscoKey = key.NewDisco().Public()
+				}
+
+				// Let both requests publish their NodeStore writes before
+				// either can persist. Both must then use the last region,
+				// regardless of which response reaches peers first.
+				s.persistMu.Lock()
+				unlockPersist := sync.OnceFunc(s.persistMu.Unlock)
+
+				var wg sync.WaitGroup
+
+				t.Cleanup(func() {
+					unlockPersist()
+					wg.Wait()
+				})
+
+				type result struct {
+					change change.Change
+					err    error
+				}
+
+				results := make(chan result, 2)
+
+				for _, derp := range []tailcfg.DERPRegionID{tt.firstDERP, tt.lastDERP} {
+					request := req
+					request.Hostinfo = req.Hostinfo.Clone()
+					request.Hostinfo.NetInfo.PreferredDERP = derp
+
+					wg.Go(func() {
+						c, err := s.UpdateNodeFromMapRequest(nodeID, request)
+						results <- result{change: c, err: err}
+					})
+
+					require.EventuallyWithT(t, func(c *assert.CollectT) {
+						stored, exists := s.GetNodeByID(nodeID)
+						require.True(c, exists)
+						require.Equal(c, derp, stored.Hostinfo().NetInfo().PreferredDERP())
+					}, 5*time.Second, time.Millisecond, "request must update NodeStore before persisting")
+				}
+
+				unlockPersist()
+				wg.Wait()
+				close(results)
+
+				for result := range results {
+					require.NoError(t, result.err)
+
+					if tt.lastDERP == 0 {
+						require.Contains(t, result.change.PeersChanged, nodeID,
+							"the current region is zero, so neither response can use a DERP patch")
+						require.Empty(t, result.change.PeerPatches)
+
+						continue
+					}
+
+					require.Empty(t, result.change.PeersChanged)
+					require.Len(t, result.change.PeerPatches, 1)
+					require.Equal(t, tt.lastDERP, result.change.PeerPatches[0].DERPRegion,
+						"neither response may restore a superseded DERP region")
+				}
+
+				persisted, err := s.DB().GetNodeByID(nodeID)
+				require.NoError(t, err)
+				require.Equal(t, tt.lastDERP, persisted.Hostinfo.NetInfo.PreferredDERP)
+
+				// A resend is still a no-op; correct delivery cannot depend
+				// on a future identical request repairing the peer's state.
+				req.Hostinfo = req.Hostinfo.Clone()
+				req.Hostinfo.NetInfo.PreferredDERP = tt.lastDERP
+				c, err := s.UpdateNodeFromMapRequest(nodeID, req)
+				require.NoError(t, err)
+				require.True(t, c.IsEmpty())
+			})
+		}
+	}
+}
+
+// TestMapRequestPeerInvisibleHostinfoChangeIsStoredNotBroadcast pins that
+// a Hostinfo field no peer reads is stored and persisted but does not
+// resend the whole node to every peer.
+func TestMapRequestPeerInvisibleHostinfoChangeIsStoredNotBroadcast(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	var nodeUpdateCount atomic.Int64
+
+	gdb := s.DB().DB
+	cbName := "peer_invisible_count_node_updates"
+	err := gdb.Callback().Update().After("gorm:update").Register(cbName, func(tx *gorm.DB) {
+		if tx.Statement != nil && tx.Statement.Table == "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)
+
+	stored := nv.AsStruct()
+
+	req := func(model string) tailcfg.MapRequest {
+		return tailcfg.MapRequest{
+			NodeKey:  stored.NodeKey,
+			DiscoKey: stored.DiscoKey,
+			Hostinfo: &tailcfg.Hostinfo{
+				Hostname:    stored.Hostname,
+				OS:          "linux",
+				DeviceModel: model,
+				NetInfo:     &tailcfg.NetInfo{PreferredDERP: 1},
+			},
+		}
+	}
+
+	_, err = s.UpdateNodeFromMapRequest(nodeID, req("a"))
+	require.NoError(t, err)
+
+	nodeUpdateCount.Store(0)
+
+	c, err := s.UpdateNodeFromMapRequest(nodeID, req("b"))
+	require.NoError(t, err)
+	require.True(t, c.IsEmpty(), "peers read no DeviceModel, got %+v", c)
+	require.Positive(t, nodeUpdateCount.Load(), "the new DeviceModel must be persisted")
+
+	after, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok)
+	require.Equal(t, "b", after.Hostinfo().DeviceModel(), "the new DeviceModel must be stored")
+}
+
+// TestMapRequestPeerVisibleHostinfoChangeIsBroadcast pins that a Hostinfo
+// field peers read still resends the whole node.
+func TestMapRequestPeerVisibleHostinfoChangeIsBroadcast(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	nv, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok)
+
+	stored := nv.AsStruct()
+
+	req := func(os string) tailcfg.MapRequest {
+		return tailcfg.MapRequest{
+			NodeKey:  stored.NodeKey,
+			DiscoKey: stored.DiscoKey,
+			Hostinfo: &tailcfg.Hostinfo{
+				Hostname: stored.Hostname,
+				OS:       os,
+				NetInfo:  &tailcfg.NetInfo{PreferredDERP: 1},
+			},
+		}
+	}
+
+	_, err := s.UpdateNodeFromMapRequest(nodeID, req("linux"))
+	require.NoError(t, err)
+
+	c, err := s.UpdateNodeFromMapRequest(nodeID, req("windows"))
+	require.NoError(t, err)
+	require.Contains(t, c.PeersChanged, nodeID, "peers read OS, got %+v", c)
+}
+
+func mustLastSeen(t *testing.T, s *State, id types.NodeID) time.Time {
+	t.Helper()
+
+	nv, ok := s.GetNodeByID(id)
+	require.True(t, ok)
+
+	seen, ok := nv.LastSeen().GetOk()
+	require.True(t, ok, "LastSeen must be stamped")
+
+	return seen
+}
+
+func TestHostinfoEqual(t *testing.T) {
+	t.Run("both nil", func(t *testing.T) {
+		require.True(t, hostinfoEqual(nil, nil))
+	})
+
+	t.Run("one nil", func(t *testing.T) {
+		require.False(t, hostinfoEqual(&tailcfg.Hostinfo{}, nil))
+		require.False(t, hostinfoEqual(nil, &tailcfg.Hostinfo{}))
+	})
+
+	t.Run("identical", func(t *testing.T) {
+		a := &tailcfg.Hostinfo{Hostname: "node1", OS: "linux"}
+		b := &tailcfg.Hostinfo{Hostname: "node1", OS: "linux"}
+		require.True(t, hostinfoEqual(a, b))
+	})
+
+	t.Run("DERPLatency jitter is not a change", func(t *testing.T) {
+		a := &tailcfg.Hostinfo{
+			Hostname: "node1",
+			NetInfo: &tailcfg.NetInfo{
+				PreferredDERP: 1,
+				DERPLatency:   map[string]float64{"1-v4": 0.010},
+			},
+		}
+		b := &tailcfg.Hostinfo{
+			Hostname: "node1",
+			NetInfo: &tailcfg.NetInfo{
+				PreferredDERP: 1,
+				DERPLatency:   map[string]float64{"1-v4": 0.025}, // jitter
+			},
+		}
+		require.True(t, hostinfoEqual(a, b),
+			"DERPLatency jitter must not count as a change")
+	})
+
+	t.Run("PreferredDERP change is not a change here", func(t *testing.T) {
+		a := &tailcfg.Hostinfo{
+			Hostname: "node1",
+			NetInfo:  &tailcfg.NetInfo{PreferredDERP: 1},
+		}
+		b := &tailcfg.Hostinfo{
+			Hostname: "node1",
+			NetInfo:  &tailcfg.NetInfo{PreferredDERP: 2},
+		}
+		require.True(t, hostinfoEqual(a, b),
+			"PreferredDERP change is tracked separately")
+	})
+
+	t.Run("hostname change", func(t *testing.T) {
+		a := &tailcfg.Hostinfo{Hostname: "node1"}
+		b := &tailcfg.Hostinfo{Hostname: "node2"}
+		require.False(t, hostinfoEqual(a, b))
+	})
+
+	t.Run("route change", func(t *testing.T) {
+		a := &tailcfg.Hostinfo{Hostname: "node1", RoutableIPs: nil}
+		b := &tailcfg.Hostinfo{Hostname: "node1"}
+		// RoutableIPs nil vs nil: equal
+		require.True(t, hostinfoEqual(a, b))
+
+		// Add a route.
+		// (also tracked separately as routesChangedInput)
+		b.RoutableIPs = []netip.Prefix{netip.MustParsePrefix("10.0.0.0/24")}
+		require.False(t, hostinfoEqual(a, b))
+	})
+}
+
+func TestPeerHostinfoEqual(t *testing.T) {
+	base := func() *tailcfg.Hostinfo {
+		return &tailcfg.Hostinfo{
+			Hostname:    "node",
+			OS:          "linux",
+			DeviceModel: "laptop",
+			NetInfo:     &tailcfg.NetInfo{PreferredDERP: 1, WorkingUDP: opt.NewBool(true)},
+		}
+	}
+
+	tests := []struct {
+		name   string
+		mutate func(*tailcfg.Hostinfo)
+		want   bool
+	}{
+		{name: "no change", mutate: func(*tailcfg.Hostinfo) {}, want: true},
+		{name: "device model", mutate: func(hi *tailcfg.Hostinfo) { hi.DeviceModel = "desktop" }, want: true},
+		{name: "shields up", mutate: func(hi *tailcfg.Hostinfo) { hi.ShieldsUp = true }, want: true},
+		{name: "ipn version", mutate: func(hi *tailcfg.Hostinfo) { hi.IPNVersion = "1.99" }, want: true},
+		{name: "netinfo udp", mutate: func(hi *tailcfg.Hostinfo) { hi.NetInfo.WorkingUDP = opt.NewBool(false) }, want: true},
+		{name: "netinfo dropped", mutate: func(hi *tailcfg.Hostinfo) { hi.NetInfo = nil }, want: true},
+		{name: "hostname", mutate: func(hi *tailcfg.Hostinfo) { hi.Hostname = "other" }, want: false},
+		{name: "os", mutate: func(hi *tailcfg.Hostinfo) { hi.OS = "windows" }, want: false},
+		{
+			name:   "services",
+			mutate: func(hi *tailcfg.Hostinfo) { hi.Services = []tailcfg.Service{{Proto: "peerapi4", Port: 1}} },
+			want:   false,
+		},
+		{name: "ssh host keys", mutate: func(hi *tailcfg.Hostinfo) { hi.SSH_HostKeys = []string{"ssh-ed25519 AAAA"} }, want: false},
+		{name: "location", mutate: func(hi *tailcfg.Hostinfo) { hi.Location = &tailcfg.Location{Priority: 5} }, want: false},
+		{name: "app connector", mutate: func(hi *tailcfg.Hostinfo) { hi.AppConnector = opt.NewBool(true) }, want: false},
+		{
+			name:   "routes",
+			mutate: func(hi *tailcfg.Hostinfo) { hi.RoutableIPs = []netip.Prefix{netip.MustParsePrefix("10.0.0.0/24")} },
+			want:   false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			changed := base()
+			tt.mutate(changed)
+			require.Equal(t, tt.want, peerHostinfoEqual(base(), changed))
+		})
+	}
+
+	require.True(t, peerHostinfoEqual(nil, nil))
+	require.False(t, peerHostinfoEqual(nil, base()))
+}
+
+func TestNetInfoEqualIgnoringDERP(t *testing.T) {
+	t.Run("both nil", func(t *testing.T) {
+		require.True(t, netInfoEqualIgnoringDERP(nil, nil))
+	})
+
+	t.Run("ignores PreferredDERP", func(t *testing.T) {
+		a := &tailcfg.NetInfo{PreferredDERP: 1, WorkingUDP: opt.NewBool(true)}
+		b := &tailcfg.NetInfo{PreferredDERP: 9, WorkingUDP: opt.NewBool(true)}
+		require.True(t, netInfoEqualIgnoringDERP(a, b))
+	})
+
+	t.Run("ignores DERPLatency", func(t *testing.T) {
+		a := &tailcfg.NetInfo{DERPLatency: map[string]float64{"1": 0.01}}
+		b := &tailcfg.NetInfo{DERPLatency: map[string]float64{"1": 0.99}}
+		require.True(t, netInfoEqualIgnoringDERP(a, b))
+	})
+
+	t.Run("catches WorkingUDP change", func(t *testing.T) {
+		a := &tailcfg.NetInfo{WorkingUDP: opt.NewBool(true)}
+		b := &tailcfg.NetInfo{WorkingUDP: opt.NewBool(false)}
+		require.False(t, netInfoEqualIgnoringDERP(a, b))
+	})
+}
+
+func TestHostinfoDERP(t *testing.T) {
+	require.Equal(t, tailcfg.DERPRegionID(0), hostinfoDERP(nil))
+	require.Equal(t, tailcfg.DERPRegionID(0), hostinfoDERP(&tailcfg.Hostinfo{}))
+	require.Equal(t, tailcfg.DERPRegionID(0), hostinfoDERP(&tailcfg.Hostinfo{NetInfo: &tailcfg.NetInfo{}}))
+	require.Equal(t, tailcfg.DERPRegionID(5), hostinfoDERP(&tailcfg.Hostinfo{NetInfo: &tailcfg.NetInfo{PreferredDERP: 5}}))
+}
+
+func TestBuildMapRequestChangeResponse(t *testing.T) {
+	node := types.Node{
+		ID:        1,
+		NodeKey:   key.NewNode().Public(),
+		DiscoKey:  key.NewDisco().Public(),
+		Endpoints: []netip.AddrPort{netip.MustParseAddrPort("203.0.113.9:41641")},
+		Hostinfo:  &tailcfg.Hostinfo{NetInfo: &tailcfg.NetInfo{PreferredDERP: 2}},
+	}
+	tests := []struct {
+		name     string
+		delta    mapRequestDelta
+		nodeDERP tailcfg.DERPRegionID
+
+		wantEmpty     bool
+		wantWholePeer bool
+		wantKeys      bool
+		wantEndpoints bool
+		wantDERP      tailcfg.DERPRegionID
+	}{
+		{
+			name:  "nothing peer visible",
+			delta: mapRequestDelta{},
+
+			wantEmpty: true,
+		},
+		{
+			name:  "peer visible hostinfo",
+			delta: mapRequestDelta{peerHostinfoChanged: true, endpointBroadcast: true},
+
+			wantWholePeer: true,
+		},
+		{
+			name:  "derp clear to zero",
+			delta: mapRequestDelta{derpChanged: true, oldDERP: 1, newDERP: 0},
+
+			wantWholePeer: true,
+		},
+		{
+			name:  "disco key and derp clear to zero",
+			delta: mapRequestDelta{discoKeyChanged: true, derpChanged: true, oldDERP: 1, newDERP: 0},
+
+			wantWholePeer: true,
+		},
+		{
+			name:  "disco key only",
+			delta: mapRequestDelta{discoKeyChanged: true},
+
+			wantKeys:      true,
+			wantEndpoints: true,
+		},
+		{
+			name:     "disco key and derp move",
+			delta:    mapRequestDelta{discoKeyChanged: true, derpChanged: true, oldDERP: 1, newDERP: 2},
+			nodeDERP: 2,
+
+			wantKeys:      true,
+			wantEndpoints: true,
+			wantDERP:      2,
+		},
+		{
+			name:  "useful endpoint only",
+			delta: mapRequestDelta{endpointBroadcast: true},
+
+			wantEndpoints: true,
+		},
+		{
+			name:     "derp move only",
+			delta:    mapRequestDelta{derpChanged: true, oldDERP: 1, newDERP: 2},
+			nodeDERP: 2,
+
+			wantDERP: 2,
+		},
+		{
+			name:     "endpoint and derp move",
+			delta:    mapRequestDelta{endpointBroadcast: true, derpChanged: true, oldDERP: 1, newDERP: 2},
+			nodeDERP: 2,
+
+			wantEndpoints: true,
+			wantDERP:      2,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			current := node.Clone()
+			current.Hostinfo.NetInfo.PreferredDERP = tt.nodeDERP
+			c := buildMapRequestChangeResponse(node.ID, current.View(), tt.delta)
+
+			if tt.wantEmpty {
+				require.True(t, c.IsEmpty(), "got %+v", c)
+
+				return
+			}
+
+			if tt.wantWholePeer {
+				require.Contains(t, c.PeersChanged, node.ID, "got %+v", c)
+				require.Empty(t, c.PeerPatches)
+
+				return
+			}
+
+			require.Empty(t, c.PeersChanged, "got %+v", c)
+			require.Len(t, c.PeerPatches, 1)
+
+			patch := c.PeerPatches[0]
+			require.Equal(t, tt.wantKeys, patch.Key != nil && patch.DiscoKey != nil, "keys on patch")
+			require.Equal(t, tt.wantEndpoints, patch.Endpoints != nil, "endpoints on patch")
+			require.Equal(t, tt.wantDERP, patch.DERPRegion, "DERP on patch")
+		})
+	}
+}

hscontrol/state/state.go

@@ -31,6 +31,7 @@ import (
 	"github.com/juanfont/headscale/hscontrol/types"
 	"github.com/juanfont/headscale/hscontrol/types/change"
 	"github.com/juanfont/headscale/hscontrol/util"
+	"github.com/juanfont/headscale/hscontrol/util/zlog"
 	"github.com/juanfont/headscale/hscontrol/util/zlog/zf"
 	"github.com/puzpuzpuz/xsync/v4"
 	"github.com/rs/zerolog"
@@ -3051,25 +3052,22 @@ func isAutoDerivedGivenName(given, hostname string) bool {
 //
 // TODO(kradalby): This is essentially a patch update that could be sent directly to nodes,
 // which means we could shortcut the whole change thing if there are no other important updates.
-// When a field is added to this function, remember to also add it to:
-// - node.PeerChangeFromMapRequest
-// - node.ApplyPeerChange
-// - logTracePeerChange in poll.go.
+// When a field is added to a MapRequest that is stored on the node, also add
+// it to [types.Node.PeerChangeFromMapRequest], [types.Node.ApplyPeerChange],
+// the mapRequestDelta classification, and (if the policy reads it)
+// [types.NodeView.HasPolicyChange].
 func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest) (change.Change, error) { //nolint:gocyclo // central map-request reconciliation; the sequential branch flow reads clearer as one function than split across helpers
 	log.Trace().
 		Caller().
 		Uint64(zf.NodeID, id.Uint64()).
-		Interface("request", req).
+		EmbedObject(zlog.MapRequest(&req)).
 		Msg("Processing MapRequest for node")
 
 	var (
+		delta              mapRequestDelta
 		routeChange        bool
-		hostinfoChanged    bool
 		needsRouteApproval bool
 		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.
@@ -3078,49 +3076,84 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 	// We need to ensure we update the node as it is in the [NodeStore] at
 	// the time of the request.
 	updatedNode, ok := s.nodeStore.UpdateNode(id, func(currentNode *types.Node) {
-		peerChange := currentNode.PeerChangeFromMapRequest(req)
+		// Capture the raw wire-level peer change. LastSeen is always
+		// stamped here, so classification tests must not rely on it.
+		delta.peerChange = currentNode.PeerChangeFromMapRequest(req)
+		delta.keyChanged = delta.peerChange.Key != nil
+		delta.discoKeyChanged = delta.peerChange.DiscoKey != nil
 
-		// Track what specifically changed. An endpoint delta is only
-		// broadcast-worthy when it adds a useful (non-STUN) endpoint;
-		// STUN-only churn and pure shrinks are suppressed to reduce peer
-		// churn (see endpointBroadcastWorthy). The new set is still stored
-		// via ApplyPeerChange below regardless of this decision.
-		endpointChanged = peerChange.Endpoints != nil &&
-			endpointBroadcastWorthy(currentNode.Endpoints, req.Endpoints, req.EndpointTypes)
-		derpChanged = peerChange.DERPRegion != 0
-		hostinfoChanged = !hostinfoEqual(currentNode.View(), req.Hostinfo)
+		// Normalize before classifying. A nil req.Hostinfo means the client
+		// did not send one (e.g., endpoint-only/lite requests); we must NOT
+		// clobber the stored Hostinfo with a shell containing only NetInfo.
+		// When Hostinfo is present but NetInfo is omitted (Tailscale >= 1.66
+		// sends NetInfo only when it changed), the stored NetInfo is carried
+		// over. Every comparison below runs against this normalized value,
+		// otherwise an omitted NetInfo reads as a Hostinfo change and turns
+		// a routine map request into a whole-peer broadcast.
+		var newHostinfo *tailcfg.Hostinfo
 
-		// Get the correct NetInfo to use
-		netInfo := netInfoFromMapRequest(id, currentNode.Hostinfo, req.Hostinfo)
 		if req.Hostinfo != nil {
-			req.Hostinfo.NetInfo = netInfo
-		} else {
-			req.Hostinfo = &tailcfg.Hostinfo{NetInfo: netInfo}
+			// Copy so the classification never mutates the caller's request.
+			hi := *req.Hostinfo
+			hi.NetInfo = netInfoFromMapRequest(id, currentNode.Hostinfo, req.Hostinfo)
+			newHostinfo = &hi
 		}
 
-		// Re-check hostinfoChanged after potential NetInfo preservation
-		hostinfoChanged = !hostinfoEqual(currentNode.View(), req.Hostinfo)
+		// DERP comparison is independent of the rest of Hostinfo:
+		// PreferredDERP has its own wire patch representation, and
+		// DERP zero on the wire means "unchanged", so a clear-to-zero
+		// must be detected here and escalated to a whole-peer update
+		// during classification. A request without Hostinfo says nothing
+		// about DERP, so it compares equal.
+		storedDERP := hostinfoDERP(currentNode.Hostinfo)
+		requestedDERP := storedDERP
 
-		// A change carrying only an updated LastSeen is not worth a full-row
-		// database UPDATE plus the O(n) policy rescan persistNodeAndRefreshPolicy 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 {
-			return
+		if newHostinfo != nil {
+			requestedDERP = hostinfoDERP(newHostinfo)
 		}
 
-		// Calculate route approval before [NodeStore] update to avoid calling View() inside callback
+		delta.oldDERP = storedDERP
+		delta.newDERP = requestedDERP
+		delta.derpChanged = requestedDERP != storedDERP
+
+		// Endpoint broadcast-worthiness is gated separately from storage:
+		// the new set is always stored via ApplyPeerChange below, but only
+		// newly-added useful (non-STUN) endpoints justify a peer broadcast.
+		// STUN-only churn and pure shrinks are suppressed to keep peers'
+		// views stable. See endpointBroadcastWorthy.
+		delta.endpointBroadcast = delta.peerChange.Endpoints != nil &&
+			endpointBroadcastWorthy(currentNode.Endpoints, req.Endpoints, req.EndpointTypes)
+
+		// Routes are policy and election inputs, so they are compared on
+		// their own. Any other Hostinfo change is stored, but only fields
+		// peers read are worth resending the whole node for.
+		delta.routesChanged = newHostinfo != nil &&
+			routesChanged(currentNode.View(), newHostinfo)
+		delta.hostinfoChanged = newHostinfo != nil &&
+			!hostinfoEqual(currentNode.Hostinfo, newHostinfo)
+		delta.peerHostinfoChanged = newHostinfo != nil &&
+			!peerHostinfoEqual(currentNode.Hostinfo, newHostinfo)
+
+		// A change carrying only an updated LastSeen is not worth a
+		// full-row database UPDATE plus the O(n) policy rescan
+		// persistNodeAndRefreshPolicy triggers: LastSeen is best-effort and rides
+		// along the next substantive write. DERP is called out because
+		// peerChangePersistWorthy cannot see a clear-to-zero.
+		delta.persistWorthy = peerChangePersistWorthy(delta.peerChange) ||
+			delta.hostinfoChanged ||
+			delta.derpChanged
+
+		hostinfoToStore := delta.hostinfoChanged || delta.derpChanged
+
+		// Calculate route approval before [NodeStore] update to avoid
+		// calling View() inside callback
 		var hasNewRoutes bool
 		if hi := req.Hostinfo; hi != nil {
 			hasNewRoutes = len(hi.RoutableIPs) > 0
 		}
 
-		needsRouteApproval = hostinfoChanged && (routesChanged(currentNode.View(), req.Hostinfo) || (hasNewRoutes && len(currentNode.ApprovedRoutes) == 0))
+		needsRouteApproval = delta.hostinfoChanged &&
+			(delta.routesChanged || (hasNewRoutes && len(currentNode.ApprovedRoutes) == 0))
 		if needsRouteApproval {
 			// Extract announced routes from request
 			var announcedRoutes []netip.Prefix
@@ -3140,59 +3173,52 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 		}
 
 		// Log when routes change but approval doesn't
-		if hostinfoChanged && !routeChange {
+		if delta.routesChanged && !routeChange {
 			if hi := req.Hostinfo; hi != nil {
-				if routesChanged(currentNode.View(), hi) {
-					log.Debug().
-						Caller().
-						Uint64(zf.NodeID, id.Uint64()).
-						Strs(zf.OldAnnouncedRoutes, util.PrefixesToString(currentNode.AnnouncedRoutes())).
-						Strs(zf.NewAnnouncedRoutes, util.PrefixesToString(hi.RoutableIPs)).
-						Strs(zf.ApprovedRoutes, util.PrefixesToString(currentNode.ApprovedRoutes)).
-						Bool(zf.RouteChanged, routeChange).
-						Msg("announced routes changed but approved routes did not")
-				}
+				log.Debug().
+					Caller().
+					Uint64(zf.NodeID, id.Uint64()).
+					Strs(zf.OldAnnouncedRoutes, util.PrefixesToString(currentNode.AnnouncedRoutes())).
+					Strs(zf.NewAnnouncedRoutes, util.PrefixesToString(hi.RoutableIPs)).
+					Strs(zf.ApprovedRoutes, util.PrefixesToString(currentNode.ApprovedRoutes)).
+					Bool(zf.RouteChanged, routeChange).
+					Msg("announced routes changed but approved routes did not")
 			}
 		}
 
-		currentNode.ApplyPeerChange(&peerChange)
+		currentNode.ApplyPeerChange(&delta.peerChange)
 
-		if hostinfoChanged {
-			// The node might not set NetInfo if it has not changed and if
-			// the full HostInfo object is overwritten, the information is lost.
-			// If there is no NetInfo, keep the previous one.
-			// From 1.66 the client only sends it if changed:
-			// https://github.com/tailscale/tailscale/commit/e1011f138737286ecf5123ff887a7a5800d129a2
-			// TODO(kradalby): evaluate if we need better comparing of hostinfo
-			// before we take the changes.
-			// NetInfo preservation has already been handled above before early return check
-			currentNode.Hostinfo = req.Hostinfo
-			if req.Hostinfo != nil && req.Hostinfo.Hostname != "" {
-				// Preserve an admin-renamed GivenName: only auto-derive when the
-				// current GivenName is still what SanitizeHostname of the old
-				// Hostname would produce (possibly with a "-N" collision bump).
-				autoDerived := isAutoDerivedGivenName(currentNode.GivenName, currentNode.Hostname)
+		if hostinfoToStore {
+			currentNode.Hostinfo = newHostinfo
+		}
 
-				currentNode.Hostname = req.Hostinfo.Hostname
-				if autoDerived {
-					currentNode.GivenName = dnsname.SanitizeHostname(req.Hostinfo.Hostname)
-					// [NodeStore.UpdateNode] auto-bumps GivenName on collision.
-				}
+		// Only a real hostname change may re-derive GivenName: it is peer
+		// visible, so the whole node is resent and peers learn the name.
+		if newHostinfo != nil && newHostinfo.Hostname != "" &&
+			newHostinfo.Hostname != currentNode.Hostname {
+			// Preserve an admin-renamed GivenName: only auto-derive
+			// when the current GivenName is still what
+			// SanitizeHostname of the old Hostname would produce
+			// (possibly with a "-N" collision bump).
+			autoDerived := isAutoDerivedGivenName(currentNode.GivenName, currentNode.Hostname)
+
+			currentNode.Hostname = newHostinfo.Hostname
+			if autoDerived {
+				currentNode.GivenName = dnsname.SanitizeHostname(newHostinfo.Hostname)
+				// [NodeStore.UpdateNode] auto-bumps GivenName on collision.
 			}
+		}
 
-			if routeChange {
-				// Apply pre-calculated route approval
-				// Always apply the route approval result to ensure consistency,
-				// regardless of whether the policy evaluation detected changes.
-				// This fixes the bug where routes weren't properly cleared when
-				// auto-approvers were removed from the policy.
-				log.Info().
-					Uint64(zf.NodeID, id.Uint64()).
-					Strs(zf.OldApprovedRoutes, util.PrefixesToString(currentNode.ApprovedRoutes)).
-					Strs(zf.NewApprovedRoutes, util.PrefixesToString(autoApprovedRoutes)).
-					Bool(zf.RouteChanged, routeChange).
-					Msg("applying route approval results")
-			}
+		if routeChange {
+			// Always apply the route approval result so routes are
+			// cleared when auto-approvers are removed from the policy,
+			// even if the policy evaluation itself detected no change.
+			log.Info().
+				Uint64(zf.NodeID, id.Uint64()).
+				Strs(zf.OldApprovedRoutes, util.PrefixesToString(currentNode.ApprovedRoutes)).
+				Strs(zf.NewApprovedRoutes, util.PrefixesToString(autoApprovedRoutes)).
+				Bool(zf.RouteChanged, routeChange).
+				Msg("applying route approval results")
 		}
 
 		// AllApprovedRoutes is announced ∩ approved; a Hostinfo
@@ -3215,6 +3241,9 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 			Msg("Persisting auto-approved routes from MapRequest")
 
 		// [State.SetApprovedRoutes] will update both database and PrimaryRoutes table
+		// TODO(kradalby): approval should ride the map request write above.
+		// Writing it separately costs a second NodeStore write and a second
+		// peer-map rebuild for one request.
 		_, c, err := s.SetApprovedRoutes(id, autoApprovedRoutes)
 		if err != nil {
 			return change.Change{}, fmt.Errorf("persisting auto-approved routes: %w", err)
@@ -3246,15 +3275,32 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 	// 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 persistNodeAndRefreshPolicy performs.
+	//
+	// On the MapRequest path we deliberately bypass persistNodeAndRefreshPolicy's
+	// synthetic NodeAdded fallback: persistence must not fabricate a wire
+	// notification. We persist the row directly and refresh the policy
+	// manager only when node inputs visible to the policy actually changed
+	// (structural Hostinfo or routes), letting updatePolicyManagerNodes
+	// decide whether matchers changed.
 	policyChange := change.Change{}
 
-	if persistWorthy {
+	if delta.persistWorthy {
 		var err error
 
-		_, policyChange, err = s.persistNodeAndRefreshPolicy(updatedNode)
+		updatedNode, err = s.persistNode(updatedNode)
 		if err != nil {
 			return change.Change{}, fmt.Errorf("saving to database: %w", err)
 		}
+
+		// Only refresh the policy manager when something it depends on
+		// might have moved. Endpoint/key/DERP/LastSeen-only updates do not
+		// affect policy evaluation and are deliberately skipped here.
+		if delta.peerHostinfoChanged || delta.routesChanged {
+			policyChange, err = s.updatePolicyManagerNodes()
+			if err != nil {
+				return change.Change{}, fmt.Errorf("updating policy manager after node save: %w", err)
+			}
+		}
 	}
 
 	if !policyChange.IsEmpty() {
@@ -3265,9 +3311,20 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 		return nodeRouteChange, nil
 	}
 
-	// Determine the most specific change type based on what actually changed.
-	// This allows us to send lightweight patch updates instead of full map responses.
-	return buildMapRequestChangeResponse(id, updatedNode, hostinfoChanged, endpointChanged, derpChanged)
+	// Determine the most specific change type from the classified delta.
+	// This allows us to send lightweight patch updates instead of full
+	// map responses.
+	c := buildMapRequestChangeResponse(id, updatedNode, delta)
+
+	// One trace line per classified request so a "peer cannot reach me"
+	// report can be matched to the classification that narrowed it.
+	log.Trace().
+		Uint64(zf.NodeID, id.Uint64()).
+		Str(zf.Type, c.Type()).
+		EmbedObject(delta).
+		Msg("classified MapRequest")
+
+	return c, nil
 }
 
 // endpointBroadcastWorthy reports whether an endpoint-only delta is worth
@@ -3278,9 +3335,9 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 // telling peers about. Suppressing this churn keeps peers' views stable.
 //
 // The decision is intentionally conservative: it gates the broadcast only,
-// not storage. The node's full endpoint set (STUN included) is still stored
-// and rides along the next substantive change or full MapResponse, so no
-// reachable path is permanently hidden from peers.
+// not storage. A suppressed delta is never resent on its own, so suppression
+// is only safe once peers already hold an endpoint set to fall back on; the
+// first set a node announces is therefore always broadcast.
 //
 // Limitation: headscale stores bare []netip.AddrPort with no per-endpoint
 // type, so we can only classify the *new* request's endpoints (via the
@@ -3293,6 +3350,12 @@ func endpointBroadcastWorthy(
 	stored, newEPs []netip.AddrPort,
 	newTypes []tailcfg.EndpointType,
 ) bool {
+	// Peers hold no endpoints for this node yet, so the first set announced
+	// is the only one they would ever get. Type does not matter here.
+	if len(stored) == 0 {
+		return len(newEPs) > 0
+	}
+
 	storedSet := make(map[netip.AddrPort]struct{}, len(stored))
 	for _, ep := range stored {
 		storedSet[ep] = struct{}{}
@@ -3327,52 +3390,60 @@ func isUsefulEndpointType(t tailcfg.EndpointType) bool {
 	return t != tailcfg.EndpointSTUN && t != tailcfg.EndpointSTUN4LocalPort
 }
 
-// buildMapRequestChangeResponse determines the appropriate response type for a [tailcfg.MapRequest] update.
-// Hostinfo changes require a full update, while endpoint/DERP changes can use lightweight patches.
+// buildMapRequestChangeResponse picks the narrowest broadcast for a processed
+// MapRequest delta. Policy and route changes are handled by the caller.
+//
+// Two wire constraints shape the order: tailcfg.PeerChange.DERPRegion == 0
+// means "unchanged", so clearing DERP cannot ride a patch and needs a whole
+// peer; and a key patch already carries endpoints, key expiry, and DERP, so a
+// key change subsumes an endpoint or DERP patch in the same request.
+// The delta decides which fields to send; their values come from the fresh
+// node because concurrent requests may have superseded the captured values.
 func buildMapRequestChangeResponse(
 	id types.NodeID,
 	node types.NodeView,
-	hostinfoChanged, endpointChanged, derpChanged bool,
-) (change.Change, error) {
-	// Hostinfo changes require NodeAdded (full update) as they may affect many fields.
-	if hostinfoChanged {
-		return change.NodeAdded(id), nil
+	delta mapRequestDelta,
+) change.Change {
+	if delta.peerHostinfoChanged {
+		return change.NodeAdded(id)
 	}
 
-	// Return specific change types for endpoint and/or DERP updates.
-	if endpointChanged || derpChanged {
+	var currentDERP tailcfg.DERPRegionID
+
+	if delta.derpChanged {
+		if hi := node.Hostinfo(); hi.Valid() && hi.NetInfo().Valid() {
+			currentDERP = hi.NetInfo().PreferredDERP()
+		}
+
+		if currentDERP == 0 {
+			return change.NodeAdded(id)
+		}
+	}
+
+	if delta.keyChanged || delta.discoKeyChanged {
+		c := change.NodeKeyRotated(node)
+		if delta.derpChanged {
+			c.PeerPatches[0].DERPRegion = currentDERP
+		}
+
+		return c
+	}
+
+	if delta.endpointBroadcast || delta.derpChanged {
 		patch := &tailcfg.PeerChange{NodeID: id.NodeID()}
 
-		if endpointChanged {
+		if delta.endpointBroadcast {
 			patch.Endpoints = node.Endpoints().AsSlice()
 		}
 
-		if derpChanged {
-			if hi := node.Hostinfo(); hi.Valid() {
-				if ni := hi.NetInfo(); ni.Valid() {
-					patch.DERPRegion = ni.PreferredDERP()
-				}
-			}
+		if delta.derpChanged {
+			patch.DERPRegion = currentDERP
 		}
 
-		return change.EndpointOrDERPUpdate(id, patch), nil
+		return change.EndpointOrDERPUpdate(id, patch)
 	}
 
-	return change.NodeAdded(id), nil
-}
-
-func hostinfoEqual(oldNode types.NodeView, newHI *tailcfg.Hostinfo) bool {
-	if !oldNode.Valid() && newHI == nil {
-		return true
-	}
-
-	if !oldNode.Valid() || newHI == nil {
-		return false
-	}
-
-	old := oldNode.AsStruct().Hostinfo
-
-	return old.Equal(newHI)
+	return change.Change{}
 }
 
 func routesChanged(oldNode types.NodeView, newHI *tailcfg.Hostinfo) bool {
@@ -3392,16 +3463,6 @@ func routesChanged(oldNode types.NodeView, newHI *tailcfg.Hostinfo) bool {
 	return !slices.Equal(oldRoutes, newRoutes)
 }
 
-func peerChangeEmpty(peerChange tailcfg.PeerChange) bool {
-	return peerChange.Key == nil &&
-		peerChange.DiscoKey == nil &&
-		peerChange.Online == nil &&
-		peerChange.Endpoints == nil &&
-		peerChange.DERPRegion == 0 &&
-		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

hscontrol/types/node.go

@@ -623,9 +623,8 @@ func (node *Node) MarshalZerologObject(e *zerolog.Event) {
 // PeerChangeFromMapRequest takes a [tailcfg.MapRequest] and compares it to the node
 // to produce a [tailcfg.PeerChange] struct that can be used to updated the node and
 // inform peers about smaller changes to the node.
-// When a field is added to this function, remember to also add it to:
-// - [Node.ApplyPeerChange]
-// - logTracePeerChange in poll.go.
+// When a field is added to this function, also add it to
+// [Node.ApplyPeerChange].
 func (node *Node) PeerChangeFromMapRequest(req tailcfg.MapRequest) tailcfg.PeerChange {
 	ret := tailcfg.PeerChange{
 		NodeID: tailcfg.NodeID(node.ID), //nolint:gosec // NodeID is bounded