From b5bebf9b32bc0a99b310f07e07dccccd7944c29c Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 28 Oct 2026 13:39:23 +0000 Subject: [PATCH] db, state: let only the delete that removed a node free its addresses Fixes ANT-2026-NWTIBB02 --- CHANGELOG.md | 1 + hscontrol/db/node.go | 28 ++++++++--------------- hscontrol/db/node_test.go | 2 ++ hscontrol/state/persist_test.go | 39 +++++++++++++++++++++++++++++++++ hscontrol/state/state.go | 8 ++++++- 5 files changed, 58 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36f123641..0be972e52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -166,6 +166,7 @@ the ACME DNS-01 challenge through a built-in DNS provider: `acmedns`, - A registration follow-up from a machine other than the one that started the registration is refused before it waits - Only the newest follow-up waits on a pending registration or SSH check; older ones are answered at once - Registration locks are freed once no registration uses them +- Concurrent deletes of one node free its addresses and announce its removal once ## 0.29.7 (202x-xx-xx) diff --git a/hscontrol/db/node.go b/hscontrol/db/node.go index 31c1fa7ef..1a4578dd5 100644 --- a/hscontrol/db/node.go +++ b/hscontrol/db/node.go @@ -232,34 +232,24 @@ func (hsdb *HSDatabase) DeleteNode(node *types.Node) error { // DeleteNode deletes a [types.Node] from the database. // Caller is responsible for notifying all of change. +// Returns [ErrNodeNotFound] when the row is already gone, so only the call +// that removed it acts on the deletion. func DeleteNode(tx *gorm.DB, node *types.Node, ) error { // Unscoped causes the node to be fully removed from the database. - err := tx.Unscoped().Delete(&types.Node{}, node.ID).Error - if err != nil { - return err + result := tx.Unscoped().Delete(&types.Node{}, node.ID) + if result.Error != nil { + return result.Error + } + + if result.RowsAffected == 0 { + return ErrNodeNotFound } return nil } -// DeleteEphemeralNode deletes a [types.Node] from the database, note that this method -// will remove it straight, and not notify any changes or consider any routes. -// It is intended for Ephemeral nodes. -func (hsdb *HSDatabase) DeleteEphemeralNode( - nodeID types.NodeID, -) error { - return hsdb.Write(func(tx *gorm.DB) error { - err := tx.Unscoped().Delete(&types.Node{}, nodeID).Error - if err != nil { - return err - } - - return nil - }) -} - // RegisterNodeForTest is used only for testing purposes to register a node directly in the database. // Production code should use [state.State.HandleNodeFromAuthPath] or [state.State.HandleNodeFromPreAuthKey]. func RegisterNodeForTest(tx *gorm.DB, node types.Node, ipv4 *netip.Addr, ipv6 *netip.Addr) (*types.Node, error) { diff --git a/hscontrol/db/node_test.go b/hscontrol/db/node_test.go index b628bf71f..9f72afc38 100644 --- a/hscontrol/db/node_test.go +++ b/hscontrol/db/node_test.go @@ -67,6 +67,8 @@ func TestHardDeleteNode(t *testing.T) { _, err = db.getNode(types.UserID(user.ID), "testnode3") require.Error(t, err) + + require.ErrorIs(t, db.DeleteNode(node), ErrNodeNotFound) } func TestListPeersManyNodes(t *testing.T) { diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 4394d1173..9e0c38e7d 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -573,6 +573,45 @@ func (failingSetNodesPolicyManager) SetNodes(views.Slice[types.NodeView]) (bool, return false, errInjectedPolicyNodeUpdate } +func TestDeleteNodeTwiceReleasesAddressOnce(t *testing.T) { + prefix := netip.MustParsePrefix("100.64.0.0/30") + cfg := persistTestConfig(t.TempDir() + "/headscale.db") + cfg.PrefixV4 = &prefix + cfg.PrefixV6 = nil + cfg.IPAllocation = types.IPAllocationStrategyRandom + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + user := s.CreateUserForTest("delete-user") + pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + node, _, err := s.HandleNodeFromPreAuthKey(tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "delete-node"}, + }, key.NewMachine().Public()) + require.NoError(t, err) + + stale := node + + _, _, err = s.ipAlloc.Next() + require.NoError(t, err) + _, err = s.DeleteNode(node) + require.NoError(t, err) + reissued, _, err := s.ipAlloc.Next() + require.NoError(t, err) + require.Equal(t, node.IPv4().Get(), *reissued) + + changes, err := s.DeleteNode(stale) + require.NoError(t, err) + assert.Empty(t, changes) + + _, _, err = s.ipAlloc.Next() + require.ErrorIs(t, err, db.ErrCouldNotAllocateIP) +} + func TestDeleteNodeKeepsStoreOnDBFailure(t *testing.T) { _, s, nodeID := persistTestSetup(t) t.Cleanup(func() { _ = s.Close() }) diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index a48a53699..cf38bf8e8 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -710,7 +710,9 @@ func (s *State) SaveNode(node types.NodeView) (types.NodeView, change.Change, er // 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. +// still torn down after a committed deletion. Concurrent deletes of one node +// act once: a call that finds the row already gone returns no changes, and +// only the call that removed the row frees the addresses. func (s *State) DeleteNode(node types.NodeView) ([]change.Change, error) { genBefore := s.polMan.NodesGeneration() @@ -720,6 +722,10 @@ func (s *State) DeleteNode(node types.NodeView) ([]change.Change, error) { s.persistMu.Unlock() if err != nil { + if errors.Is(err, hsdb.ErrNodeNotFound) { + return nil, nil + } + return nil, err }