diff --git a/hscontrol/db/node.go b/hscontrol/db/node.go index 5f858b593..48f9fddda 100644 --- a/hscontrol/db/node.go +++ b/hscontrol/db/node.go @@ -226,34 +226,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 7bbcf3d95..3bc9a3302 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 e5c806249..10319d166 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -536,6 +536,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 edf90e7dc..be2ce4fc6 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -686,7 +686,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) { s.persistMu.Lock() @@ -694,6 +696,10 @@ func (s *State) DeleteNode(node types.NodeView) ([]change.Change, error) { if err != nil { s.persistMu.Unlock() + if errors.Is(err, hsdb.ErrNodeNotFound) { + return nil, nil + } + return nil, err }