diff --git a/hscontrol/api/v1/nodes.go b/hscontrol/api/v1/nodes.go index c082ffa0c..afe600397 100644 --- a/hscontrol/api/v1/nodes.go +++ b/hscontrol/api/v1/nodes.go @@ -263,10 +263,8 @@ func registerNodeWriteOps(api huma.API, b Backend) { return nil, huma.Error404NotFound("node not found") } - nodeChange, err := b.State.DeleteNode(node) - if !nodeChange.IsEmpty() { - b.Change(nodeChange) - } + changes, err := b.State.DeleteNode(node) + b.Change(changes...) if err != nil { return nil, huma.Error500InternalServerError("deleting node", err) diff --git a/hscontrol/api/v2/devices.go b/hscontrol/api/v2/devices.go index 8eeb3d258..d5afd78b4 100644 --- a/hscontrol/api/v2/devices.go +++ b/hscontrol/api/v2/devices.go @@ -177,10 +177,8 @@ func registerDevices(api huma.API, b Backend) { return nil, err } - nodeChange, err := b.State.DeleteNode(node) - if !nodeChange.IsEmpty() { - b.Change(nodeChange) - } + changes, err := b.State.DeleteNode(node) + b.Change(changes...) if err != nil { return nil, mapError("deleting device", err) diff --git a/hscontrol/app.go b/hscontrol/app.go index de8338cd1..16d5c26be 100644 --- a/hscontrol/app.go +++ b/hscontrol/app.go @@ -155,10 +155,8 @@ func NewHeadscale(cfg *types.Config) (*Headscale, error) { return } - policyChanged, err := app.state.DeleteNode(node) - if !policyChanged.IsEmpty() { - app.Change(policyChanged) - } + changes, err := app.state.DeleteNode(node) + app.Change(changes...) if err != nil { log.Error().Err(err).EmbedObject(node).Msg("ephemeral node deletion failed") diff --git a/hscontrol/auth.go b/hscontrol/auth.go index 69985e2bc..b2b0c6e8f 100644 --- a/hscontrol/auth.go +++ b/hscontrol/auth.go @@ -215,10 +215,8 @@ func (h *Headscale) handleLogout( EmbedObject(node). Msg("Deleting ephemeral node during logout") - c, err := h.state.DeleteNode(node) - if !c.IsEmpty() { - h.Change(c) - } + changes, err := h.state.DeleteNode(node) + h.Change(changes...) if err != nil { return nil, fmt.Errorf("deleting ephemeral node: %w", err) diff --git a/hscontrol/mapper/batcher_test.go b/hscontrol/mapper/batcher_test.go index 8a6cb3b51..746661f54 100644 --- a/hscontrol/mapper/batcher_test.go +++ b/hscontrol/mapper/batcher_test.go @@ -2160,9 +2160,8 @@ func TestNodeDeletedWhileChangesPending(t *testing.T) { // Delete the node from state - this returns a NodeRemoved change // In production, this change is sent to batcher via app.Change() - nodeChange, err := st.DeleteNode(nodeToDelete) + nodeChanges, err := st.DeleteNode(nodeToDelete) require.NoError(t, err, "should be able to delete node from state") - t.Logf("Deleted node %d from state, change: %s", node3.n.ID, nodeChange.Reason) // Verify node is deleted from state _, exists := st.GetNodeByID(node3.n.ID) @@ -2170,7 +2169,7 @@ func TestNodeDeletedWhileChangesPending(t *testing.T) { // Send the NodeRemoved change to batcher (this is what app.Change() does) // With the fix, this should clean up node3 from batcher's internal state - batcher.AddWork(nodeChange) + batcher.AddWork(nodeChanges...) // Wait for the batcher to process the removal and clean up the node assert.EventuallyWithT(t, func(c *assert.CollectT) { diff --git a/hscontrol/poll_test.go b/hscontrol/poll_test.go index 737cd4e00..a33b102a8 100644 --- a/hscontrol/poll_test.go +++ b/hscontrol/poll_test.go @@ -514,9 +514,9 @@ func TestDeletedNodeEndsLongPoll(t *testing.T) { t.Fatal("expected the initial map write to start") } - c, err := app.state.DeleteNode(nodeView) + cs, err := app.state.DeleteNode(nodeView) require.NoError(t, err) - app.Change(c) + app.Change(cs...) // The reconnect grace period is 10s, so a generous bound here still fails // if teardown waits for a node that can never come back. @@ -595,9 +595,9 @@ func TestDeletedNodeInterruptsBlockedWrite(t *testing.T) { t.Fatal("expected the initial map write to block") } - c, err := app.state.DeleteNode(nodeView) + cs, err := app.state.DeleteNode(nodeView) require.NoError(t, err) - app.Change(c) + app.Change(cs...) select { case <-serveDone: diff --git a/hscontrol/servertest/issues_test.go b/hscontrol/servertest/issues_test.go index d505a4753..0ac3eabc5 100644 --- a/hscontrol/servertest/issues_test.go +++ b/hscontrol/servertest/issues_test.go @@ -556,7 +556,7 @@ func TestIssuesServerMutations(t *testing.T) { deleteChange, err := srv.State().DeleteNode(node2View) require.NoError(t, err) - srv.App.Change(deleteChange) + srv.App.Change(deleteChange...) c1.WaitForCondition(t, "deleted peer gone", 10*time.Second, func(nm *netmap.NetworkMap) bool { @@ -1041,7 +1041,7 @@ func TestPeerRemovedAsDelta(t *testing.T) { changes, err := srv.State().DeleteNode(node2) require.NoError(t, err) - srv.App.Change(changes) + srv.App.Change(changes...) assertRemovedAsDelta(t, c1, nodeID2) }) @@ -1060,7 +1060,7 @@ func TestPeerRemovedAsDelta(t *testing.T) { changes, err := srv.State().DeleteNode(node2) require.NoError(t, err) - srv.App.Change(changes) + srv.App.Change(changes...) c1.Reconnect(t) diff --git a/hscontrol/servertest/race_test.go b/hscontrol/servertest/race_test.go index 8c5ec778c..d742d43c6 100644 --- a/hscontrol/servertest/race_test.go +++ b/hscontrol/servertest/race_test.go @@ -197,7 +197,7 @@ func TestRaceConcurrentServerMutations(t *testing.T) { wg.Go(func() { delChange, err := srv.State().DeleteNode(nv2) if err == nil { - srv.App.Change(delChange) + srv.App.Change(delChange...) } }) diff --git a/hscontrol/servertest/stress_test.go b/hscontrol/servertest/stress_test.go index e330aa3d8..d547debfc 100644 --- a/hscontrol/servertest/stress_test.go +++ b/hscontrol/servertest/stress_test.go @@ -121,7 +121,7 @@ func TestStressConnectDisconnect(t *testing.T) { deleteChange, err := srv.State().DeleteNode(nv) require.NoError(t, err) - srv.App.Change(deleteChange) + srv.App.Change(deleteChange...) // c2 should see c1 disappear. c2.WaitForCondition(t, "deleted node gone", 10*time.Second, diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index f2b532384..c43ecdad4 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -512,7 +512,7 @@ func TestDeleteNodeKeepsStoreOnDBFailure(t *testing.T) { c, err := s.DeleteNode(node) require.NoError(t, s.db.DB.Callback().Delete().Remove("fail_node_delete")) require.ErrorIs(t, err, errInjectedNodeDelete) - assert.True(t, c.IsEmpty(), "an uncommitted deletion must not stop the node's session") + assert.Empty(t, c, "an uncommitted deletion must not stop the node's session") _, ok = s.GetNodeByID(nodeID) assert.True(t, ok, "a database failure must leave the in-memory node available") @@ -530,11 +530,12 @@ func TestDeleteNodeReturnsRemovalOnPolicyFailure(t *testing.T) { s.polMan = failingSetNodesPolicyManager{PolicyManager: s.polMan} - c, err := s.DeleteNode(node) + cs, err := s.DeleteNode(node) require.ErrorIs(t, err, errInjectedPolicyNodeUpdate) - assert.Equal(t, []types.NodeID{nodeID}, c.PeersRemoved, + require.NotEmpty(t, cs) + assert.Equal(t, []types.NodeID{nodeID}, cs[0].PeersRemoved, "a committed deletion must still notify peers and stop the node's session") - assert.Equal(t, []types.NodeID{nodeID}, c.DeletedNodes, + assert.Equal(t, []types.NodeID{nodeID}, cs[0].DeletedNodes, "a committed deletion must identify the session to stop") _, ok = s.GetNodeByID(nodeID) diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index f41fa4298..e93af37e8 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -612,11 +612,11 @@ func (s *State) SaveNode(node types.NodeView) (types.NodeView, change.Change, er } // DeleteNode permanently removes a node and cleans up associated resources. -// Once the database deletion commits, the returned change always contains the -// node-removal notification, even if a later policy refresh fails. Callers must -// publish a non-empty change before handling the error so live sessions are +// Once the database deletion commits, the returned changes always start with +// the node-removal notification, even if a later policy refresh fails. Callers +// must publish the changes before handling the error so live sessions are // still torn down after a committed deletion. -func (s *State) DeleteNode(node types.NodeView) (change.Change, error) { +func (s *State) DeleteNode(node types.NodeView) ([]change.Change, error) { genBefore := s.polMan.NodesGeneration() s.persistMu.Lock() @@ -625,7 +625,7 @@ func (s *State) DeleteNode(node types.NodeView) (change.Change, error) { if err != nil { s.persistMu.Unlock() - return change.Change{}, err + return nil, err } // The database is the durable source of truth. Only remove the in-memory @@ -636,21 +636,16 @@ func (s *State) DeleteNode(node types.NodeView) (change.Change, error) { s.ipAlloc.FreeIPs(node.IPs()) - c := change.NodeRemoved(node.ID()) + // An explicit removal of its own, ahead of the policy refresh, so peers + // learn of the deletion without depending on their sent-peers tracking. + removed := change.NodeRemoved(node.ID()) - // Check if policy manager needs updating after node deletion policyChange, err := s.updatePolicyManagerNodes(genBefore) if err != nil { - return c.Merge(policyChange), fmt.Errorf("updating policy manager after node deletion: %w", err) + return []change.Change{removed, policyChange}, fmt.Errorf("updating policy manager after node deletion: %w", err) } - if !policyChange.IsEmpty() { - // Merge policy change with NodeRemoved to preserve PeersRemoved info - // This ensures the batcher cleans up the deleted node from its state - c = c.Merge(policyChange) - } - - return c, nil + return []change.Change{removed, policyChange}, nil } // Connect acquires a control session and returns the resulting changes