diff --git a/hscontrol/types/change/change.go b/hscontrol/types/change/change.go index a0558cb9e..b9678edb4 100644 --- a/hscontrol/types/change/change.go +++ b/hscontrol/types/change/change.go @@ -327,16 +327,45 @@ func (r Change) isPeerDelta(nodeID types.NodeID) bool { return len(r.PeersChanged) > 0 || len(r.PeerPatches) > 0 } -// CoalescePeerDeltas folds each run of adjacent peer deltas in nodeID's -// changes for one tick into a single change, so a burst of joins renders as -// one [tailcfg.MapResponse] instead of one per change. Every change renders -// from live state after the drain, so a peer in the run's PeersChanged is sent -// whole and newer than any patch queued for it, and those patches are -// dropped. Runs never move across other changes, which keeps removals and -// policy recomputes in order. +// isDeletion reports whether r only announces deleted nodes, as +// [NodeRemoved] does: [CoalescePeerDeltas] can hoist and fold such changes. +func (r Change) isDeletion() bool { + rest := r + rest.Reason, rest.TargetNode, rest.PeersRemoved, rest.DeletedNodes = "", 0, nil, nil + + return len(r.DeletedNodes) > 0 && rest.IsEmpty() && rest.OriginNode == 0 +} + +// CoalescePeerDeltas cuts the responses nodeID's changes for one tick render +// to, so a burst of joins or deletes renders as a few [tailcfg.MapResponse] +// values instead of one per change. Every change renders from live state after +// the drain: +// - A deleted node can no longer render anywhere and its ID is never reused, +// so all deletions fold into one removal ahead of the tick. +// - A peer in a run's PeersChanged is sent whole and newer than any patch +// queued for it, so each run of adjacent peer deltas folds into one change +// and those patches are dropped. Runs never move across other changes, +// which keeps other removals and policy recomputes in order. func CoalescePeerDeltas(nodeID types.NodeID, changes []Change) []Change { out := make([]Change, 0, len(changes)) + var deleted Change + + for _, c := range changes { + if c.isDeletion() { + deleted.PeersRemoved = append(deleted.PeersRemoved, c.PeersRemoved...) + deleted.DeletedNodes = append(deleted.DeletedNodes, c.DeletedNodes...) + } + } + + if deleted.DeletedNodes != nil { + deleted.Reason = "node removed" + deleted.PeersRemoved = uniqueNodeIDs(deleted.PeersRemoved) + deleted.DeletedNodes = uniqueNodeIDs(deleted.DeletedNodes) + out = append(out, deleted) + changes = slices.DeleteFunc(slices.Clone(changes), Change.isDeletion) + } + for i := 0; i < len(changes); { j := i for j < len(changes) && changes[j].isPeerDelta(nodeID) { diff --git a/hscontrol/types/change/change_test.go b/hscontrol/types/change/change_test.go index 739888616..589efb277 100644 --- a/hscontrol/types/change/change_test.go +++ b/hscontrol/types/change/change_test.go @@ -519,10 +519,20 @@ func TestCoalescePeerDeltas(t *testing.T) { { name: "runs do not cross other changes", changes: []Change{ - NodeAdded(2), NodeOnline(2), PolicyChange(), NodeAdded(3), NodeRemoved(4), NodeOnline(5), ping, NodeOnline(6), + NodeAdded(2), NodeOnline(2), PolicyChange(), NodeAdded(3), PeersRemoved(4), NodeOnline(5), ping, NodeOnline(6), }, want: []Change{ - merged([]types.NodeID{2}), PolicyChange(), NodeAdded(3), NodeRemoved(4), NodeOnline(5), ping, NodeOnline(6), + merged([]types.NodeID{2}), PolicyChange(), NodeAdded(3), PeersRemoved(4), NodeOnline(5), ping, NodeOnline(6), + }, + }, + { + name: "deletions fold into one removal ahead of the tick", + changes: []Change{ + NodeAdded(2), NodeRemoved(5), NodeOnline(3), PolicyChange(), NodeRemoved(4), NodeAdded(6), + }, + want: []Change{ + {Reason: "node removed", PeersRemoved: []types.NodeID{4, 5}, DeletedNodes: []types.NodeID{4, 5}}, + merged([]types.NodeID{2}, patch(NodeOnline(3))), PolicyChange(), NodeAdded(6), }, }, { @@ -579,6 +589,46 @@ func TestCoalescePeerDeltasClassifiesEveryField(t *testing.T) { } } +// TestCoalescePeerDeltasHoistsOnlyDeletions classifies every [Change] field +// by whether a deletion setting it may still be hoisted ahead of the tick. A +// new field fails here until it is classified. +func TestCoalescePeerDeltasHoistsOnlyDeletions(t *testing.T) { + const self = types.NodeID(1) + + hoists := map[string]bool{ + "Reason": true, + "TargetNode": true, + "OriginNode": false, + "IncludeSelf": false, + "IncludeDERPMap": false, + "IncludeDNS": false, + "IncludeDomain": false, + "IncludePolicy": false, + "PeersChanged": false, + "PeersRemoved": true, + "PeerPatches": false, + "SendAllPeers": false, + "DeletedNodes": true, + "RequiresRuntimePeerComputation": false, + "PingRequest": false, + } + + typ := reflect.TypeFor[Change]() + require.Len(t, hoists, typ.NumField(), "classify every Change field") + + for field := range typ.Fields() { + t.Run(field.Name, func(t *testing.T) { + want, ok := hoists[field.Name] + require.True(t, ok, "field %s is not classified", field.Name) + + c := NodeRemoved(2) + setNonZero(t, reflect.ValueOf(&c).Elem().FieldByIndex(field.Index), self) + + assert.Equal(t, want, c.isDeletion()) + }) + } +} + func TestChange_Constructors(t *testing.T) { tests := []struct { name string