state: decide whole-peer updates at each caller, not in persist

persistNodeAndRefreshPolicy no longer fabricates NodeAdded; RenameNode
and SetNodeTags add it since both are peer visible.

hscontrol/state/persist_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/juanfont/headscale/hscontrol/db"
 	"github.com/juanfont/headscale/hscontrol/policy"
 	"github.com/juanfont/headscale/hscontrol/types"
+	"github.com/juanfont/headscale/hscontrol/types/change"
 	"github.com/juanfont/headscale/hscontrol/util"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -713,3 +714,160 @@ func TestSingleUsePreAuthKeyUsedInNodeStore(t *testing.T) {
 	second := register(t)
 	assert.Equal(t, first.ID(), second.ID(), "second key re-registers the same node")
 }
+
+// TestPersistNodeAndRefreshPolicyEmptyForPayloadOnlyChange proves
+// persistNodeAndRefreshPolicy returns an empty change when the write did not
+// touch anything policy reads, so each caller can tell a genuinely empty
+// write apart and pick its own wire change (see
+// TestPersistCallerChangeDecisions).
+func TestPersistNodeAndRefreshPolicyEmptyForPayloadOnlyChange(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	view, ok := s.nodeStore.UpdateNode(nodeID, func(n *types.Node) {
+		n.Hostinfo = &tailcfg.Hostinfo{Hostname: "payload-only"}
+	})
+	require.True(t, ok)
+
+	_, c, err := s.persistNodeAndRefreshPolicy(view)
+	require.NoError(t, err)
+	assert.True(t, c.IsEmpty(), "a payload-only write must not fabricate a change")
+}
+
+// TestPersistCallerChangeDecisions proves each persistNodeAndRefreshPolicy
+// caller picks its own wire change when persist reports none. A caller that
+// returns nothing silently stops a node's peers from learning about it, so
+// every case here checks the exact change, not just that persist succeeded.
+func TestPersistCallerChangeDecisions(t *testing.T) {
+	taggingPolicy := `{
+		"tagOwners": {"tag:ci": ["persist-user@"]},
+		"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}]
+	}`
+
+	tests := []struct {
+		name             string
+		policy           string
+		setup            func(t *testing.T, s *State, nodeID types.NodeID)
+		run              func(t *testing.T, s *State, nodeID types.NodeID) change.Change
+		wantType         string
+		wantOriginNode   bool
+		wantPeersChanged bool
+	}{
+		{
+			name: "RenameNode resends the whole node when the rename does not affect policy",
+			run: func(t *testing.T, s *State, nodeID types.NodeID) change.Change {
+				t.Helper()
+
+				_, c, err := s.RenameNode(nodeID, "renamed")
+				require.NoError(t, err)
+
+				return c
+			},
+			wantType:         "peers",
+			wantOriginNode:   true,
+			wantPeersChanged: true,
+		},
+		{
+			name:   "SetNodeTags reports a policy change for a real tag assignment",
+			policy: taggingPolicy,
+			run: func(t *testing.T, s *State, nodeID types.NodeID) change.Change {
+				t.Helper()
+
+				_, c, err := s.SetNodeTags(nodeID, []string{"tag:ci"})
+				require.NoError(t, err)
+
+				return c
+			},
+			wantType:         "policy",
+			wantOriginNode:   true,
+			wantPeersChanged: false,
+		},
+		{
+			name:   "SetNodeTags resends the whole node when re-applying identical tags",
+			policy: taggingPolicy,
+			setup: func(t *testing.T, s *State, nodeID types.NodeID) {
+				t.Helper()
+
+				_, _, err := s.SetNodeTags(nodeID, []string{"tag:ci"})
+				require.NoError(t, err)
+			},
+			run: func(t *testing.T, s *State, nodeID types.NodeID) change.Change {
+				t.Helper()
+
+				_, c, err := s.SetNodeTags(nodeID, []string{"tag:ci"})
+				require.NoError(t, err)
+
+				return c
+			},
+			wantType:         "peers",
+			wantOriginNode:   true,
+			wantPeersChanged: true,
+		},
+		{
+			name: "SetApprovedRoutes keeps reporting a policy change (Task 6 narrows this)",
+			run: func(t *testing.T, s *State, nodeID types.NodeID) change.Change {
+				t.Helper()
+
+				_, c, err := s.SetApprovedRoutes(nodeID, []netip.Prefix{netip.MustParsePrefix("10.0.0.0/8")})
+				require.NoError(t, err)
+
+				return c
+			},
+			wantType:         "policy",
+			wantOriginNode:   false,
+			wantPeersChanged: false,
+		},
+		{
+			name: "SaveNode reports no change for a payload-only save",
+			run: func(t *testing.T, s *State, nodeID types.NodeID) change.Change {
+				t.Helper()
+
+				current, ok := s.nodeStore.GetNode(nodeID)
+				require.True(t, ok)
+
+				n := current.AsStruct()
+				n.Hostinfo = &tailcfg.Hostinfo{Hostname: "saved-payload"}
+
+				_, c, err := s.SaveNode(n.View())
+				require.NoError(t, err)
+
+				return c
+			},
+			wantType:         "empty",
+			wantOriginNode:   false,
+			wantPeersChanged: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, s, nodeID := persistTestSetup(t)
+			t.Cleanup(func() { _ = s.Close() })
+
+			if tt.policy != "" {
+				_, err := s.SetPolicy([]byte(tt.policy))
+				require.NoError(t, err)
+			}
+
+			if tt.setup != nil {
+				tt.setup(t, s, nodeID)
+			}
+
+			c := tt.run(t, s, nodeID)
+
+			assert.Equal(t, tt.wantType, c.Type())
+
+			if tt.wantOriginNode {
+				assert.Equal(t, nodeID, c.OriginNode)
+			} else {
+				assert.Zero(t, c.OriginNode)
+			}
+
+			if tt.wantPeersChanged {
+				assert.Equal(t, []types.NodeID{nodeID}, c.PeersChanged)
+			} else {
+				assert.Empty(t, c.PeersChanged)
+			}
+		})
+	}
+}

hscontrol/state/state.go

@@ -577,10 +577,6 @@ func (s *State) persistNodeAndRefreshPolicy(node types.NodeView) (types.NodeView
 		return fresh, change.Change{}, fmt.Errorf("updating policy manager after node save: %w", err)
 	}
 
-	if c.IsEmpty() {
-		c = change.NodeAdded(node.ID())
-	}
-
 	return fresh, c, nil
 }
 
@@ -1009,6 +1005,12 @@ func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView,
 		return nodeView, c, err
 	}
 
+	if c.IsEmpty() {
+		// Tags are peer visible (tag owner resolution, ACLs); resend the
+		// whole node even when re-applying the same tags didn't move policy.
+		c = change.NodeAdded(nodeID)
+	}
+
 	// Set OriginNode so the mapper knows to include self info for this node.
 	// When tags change, persistNodeAndRefreshPolicy returns PolicyChange which doesn't set OriginNode,
 	// so the mapper's self-update check fails and the node never sees its new tags.
@@ -1080,7 +1082,17 @@ func (s *State) RenameNode(nodeID types.NodeID, newName string) (types.NodeView,
 		}
 	}
 
-	return s.persistNodeAndRefreshPolicy(view)
+	nodeView, c, err := s.persistNodeAndRefreshPolicy(view)
+	if err != nil {
+		return nodeView, c, err
+	}
+
+	if c.IsEmpty() {
+		// A rename is peer visible; resend the whole node.
+		c = change.NodeAdded(nodeID)
+	}
+
+	return nodeView, c, nil
 }
 
 // BackfillNodeIPs assigns IP addresses to nodes that don't have them.
@@ -3351,10 +3363,8 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 	// 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
+	// Otherwise 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{}