From 79cc581409994578a9bfb7eb6e99518f45d8545f Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 11:05:25 +0000 Subject: [PATCH] change: coalesce policy recomputes that carry an origin node The recompute render ignores the origin and always carries the node's own entry, so a burst of tag writes cost every node one per write. --- hscontrol/mapper/batcher_test.go | 13 +++++++++++-- hscontrol/state/state.go | 7 +++---- hscontrol/types/change/change.go | 12 ++++++------ hscontrol/types/change/change_test.go | 16 ++++++++-------- 4 files changed, 28 insertions(+), 20 deletions(-) diff --git a/hscontrol/mapper/batcher_test.go b/hscontrol/mapper/batcher_test.go index c01ef8c7e..fa59e1a2e 100644 --- a/hscontrol/mapper/batcher_test.go +++ b/hscontrol/mapper/batcher_test.go @@ -1135,8 +1135,17 @@ func TestBatcherCoalescesPolicyRecomputesPerTick(t *testing.T) { // a sentinel. All land in one tick; the sentinel rides the same work // item after the recompute(s), so its arrival marks the end of this // tick's policy responses for a node. - for range policyChangesPerTick { - batcher.AddWork(change.PolicyChange()) + for i := range policyChangesPerTick { + c := change.PolicyChange() + + // Every other one carries an origin, as a tag write's + // recompute does (State.SetNodeTags). The mapper renders it + // like any other recompute, so it must coalesce like one. + if i%2 == 1 { + c.OriginNode = testData.Nodes[0].n.ID + } + + batcher.AddWork(c) } batcher.AddWork(change.DERPMap()) diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 5563cc9f6..568ac8ce3 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1211,10 +1211,9 @@ func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView, 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. - // Setting OriginNode ensures the node gets a self-update with the new tags. + // Records the retagged node. Its new tags reach it either way: a policy + // recompute always renders the node's own entry, and NodeAdded already + // carries this origin. c.OriginNode = nodeID // The new tags may approve announced routes, as on Tailscale SaaS. This diff --git a/hscontrol/types/change/change.go b/hscontrol/types/change/change.go index 4c757a058..35c94618b 100644 --- a/hscontrol/types/change/change.go +++ b/hscontrol/types/change/change.go @@ -277,13 +277,13 @@ func FilterForNode(nodeID types.NodeID, rs []Change) []Change { return result } -// IsBroadcastPolicyChange reports whether r is a tailnet-wide policy recompute -// with no per-node payload. A recompute reads the current snapshot, so every -// such change is interchangeable and same-tick duplicates are redundant. A -// targeted or self-update ([Change.OriginNode]) recompute is per-node, so it is -// not one of these. +// IsBroadcastPolicyChange reports whether r is a tailnet-wide policy recompute. +// A recompute reads the current snapshot and always carries the node's own +// entry, so every such change is interchangeable and same-tick duplicates are +// redundant. [Change.OriginNode] does not make one per-node: the render never +// reads it. A targeted recompute is per-node, so it is not one of these. func (r Change) IsBroadcastPolicyChange() bool { - return r.RequiresRuntimePeerComputation && !r.IsTargetedToNode() && r.OriginNode == 0 + return r.RequiresRuntimePeerComputation && !r.IsTargetedToNode() } // DedupePolicyChanges keeps the first broadcast policy change in a tick and diff --git a/hscontrol/types/change/change_test.go b/hscontrol/types/change/change_test.go index 0ab10d066..7079bc642 100644 --- a/hscontrol/types/change/change_test.go +++ b/hscontrol/types/change/change_test.go @@ -407,7 +407,7 @@ func TestChange_IsBroadcastPolicyChange(t *testing.T) { want bool }{ {name: "policy change", c: PolicyChange(), want: true}, - {name: "self-update recompute", c: originUpdate, want: false}, + {name: "self-update recompute", c: originUpdate, want: true}, {name: "targeted recompute", c: targeted, want: false}, {name: "online patch", c: NodeOnline(1), want: false}, {name: "full update", c: FullUpdate(), want: false}, @@ -422,9 +422,9 @@ func TestChange_IsBroadcastPolicyChange(t *testing.T) { } func TestDedupePolicyChanges(t *testing.T) { - // originRecompute is a runtime recompute carrying node-specific payload - // (OriginNode), so it is not the canonical broadcast PolicyChange and must - // never be coalesced away. + // originRecompute is a runtime recompute stamped with the node that caused + // it, as a tag write returns it. The mapper renders it like any other + // recompute, so it must coalesce like any other. originRecompute := PolicyChange() originRecompute.OriginNode = 7 @@ -463,14 +463,14 @@ func TestDedupePolicyChanges(t *testing.T) { want: []Change{PolicyChange(), NodeAdded(5)}, }, { - name: "recompute carrying OriginNode is kept alongside the canonical one", + name: "recompute carrying OriginNode collapses into the canonical one", changes: []Change{PolicyChange(), originRecompute, PolicyChange()}, - want: []Change{PolicyChange(), originRecompute}, + want: []Change{PolicyChange()}, }, { - name: "non-canonical recomputes are not collapsed", + name: "recomputes carrying OriginNode collapse to one", changes: []Change{originRecompute, originRecompute}, - want: []Change{originRecompute, originRecompute}, + want: []Change{originRecompute}, }, { name: "changes without any recompute are unchanged",