From fc6a16fdfc787ba2968cc27ed6639e8ea32c7853 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 4 Sep 2026 10:22:43 +0000 Subject: [PATCH] state: preserve committed node deletion changes Updates #3410 --- hscontrol/api/v1/nodes.go | 6 ++- hscontrol/api/v2/devices.go | 6 ++- hscontrol/app.go | 5 ++- hscontrol/auth.go | 6 ++- hscontrol/state/persist_test.go | 65 ++++++++++++++++++++++++++++++++- hscontrol/state/state.go | 23 +++++++++--- 6 files changed, 97 insertions(+), 14 deletions(-) diff --git a/hscontrol/api/v1/nodes.go b/hscontrol/api/v1/nodes.go index dbc1adfc5..5659f25d2 100644 --- a/hscontrol/api/v1/nodes.go +++ b/hscontrol/api/v1/nodes.go @@ -264,12 +264,14 @@ func registerNodeWriteOps(api huma.API, b Backend) { } nodeChange, err := b.State.DeleteNode(node) + if !nodeChange.IsEmpty() { + b.Change(nodeChange) + } + if err != nil { return nil, huma.Error500InternalServerError("deleting node", err) } - b.Change(nodeChange) - return &deleteNodeOutput{}, nil }) diff --git a/hscontrol/api/v2/devices.go b/hscontrol/api/v2/devices.go index 5bdb553f8..efef676b6 100644 --- a/hscontrol/api/v2/devices.go +++ b/hscontrol/api/v2/devices.go @@ -178,12 +178,14 @@ func registerDevices(api huma.API, b Backend) { } nodeChange, err := b.State.DeleteNode(node) + if !nodeChange.IsEmpty() { + b.Change(nodeChange) + } + if err != nil { return nil, mapError("deleting device", err) } - b.Change(nodeChange) - return &emptyOutput{}, nil }) diff --git a/hscontrol/app.go b/hscontrol/app.go index 12c89f6a1..aad241c21 100644 --- a/hscontrol/app.go +++ b/hscontrol/app.go @@ -149,12 +149,15 @@ func NewHeadscale(cfg *types.Config) (*Headscale, error) { } policyChanged, err := app.state.DeleteNode(node) + if !policyChanged.IsEmpty() { + app.Change(policyChanged) + } + if err != nil { log.Error().Err(err).EmbedObject(node).Msg("ephemeral node deletion failed") return } - app.Change(policyChanged) log.Debug().Caller().EmbedObject(node).Msg("ephemeral node deleted because garbage collection timeout reached") }) app.ephemeralGC = ephemeralGC diff --git a/hscontrol/auth.go b/hscontrol/auth.go index 35b98df7a..387f88062 100644 --- a/hscontrol/auth.go +++ b/hscontrol/auth.go @@ -216,12 +216,14 @@ func (h *Headscale) handleLogout( Msg("Deleting ephemeral node during logout") c, err := h.state.DeleteNode(node) + if !c.IsEmpty() { + h.Change(c) + } + if err != nil { return nil, fmt.Errorf("deleting ephemeral node: %w", err) } - h.Change(c) - return &tailcfg.RegisterResponse{ NodeKeyExpired: true, MachineAuthorized: false, diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 0be0b4115..76623735b 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/juanfont/headscale/hscontrol/db" + "github.com/juanfont/headscale/hscontrol/policy" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" "github.com/stretchr/testify/assert" @@ -15,6 +16,7 @@ import ( "gorm.io/gorm" "tailscale.com/tailcfg" "tailscale.com/types/key" + "tailscale.com/types/views" ) // persistTestSetup pre-creates a sqlite database on disk with a single @@ -476,7 +478,68 @@ func TestPreAuthKeyReauthRejectsNodeKeyClaimedByAnotherMachine(t *testing.T) { "victim's NodeKey index entry must be untouched") } -var errInjectedNodeUpdate = errors.New("injected node update failure") +var ( + errInjectedNodeUpdate = errors.New("injected node update failure") + errInjectedNodeDelete = errors.New("injected node delete failure") + errInjectedPolicyNodeUpdate = errors.New("injected policy node update failure") +) + +type failingSetNodesPolicyManager struct { + policy.PolicyManager +} + +func (failingSetNodesPolicyManager) SetNodes(views.Slice[types.NodeView]) (bool, error) { + return false, errInjectedPolicyNodeUpdate +} + +func TestDeleteNodeKeepsStoreOnDBFailure(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + node, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + require.NoError(t, s.db.DB.Callback().Delete().Before("gorm:delete"). + Register("fail_node_delete", func(tx *gorm.DB) { + if tx.Statement.Table == "nodes" { + _ = tx.AddError(errInjectedNodeDelete) + } + })) + t.Cleanup(func() { _ = s.db.DB.Callback().Delete().Remove("fail_node_delete") }) + + 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") + + _, ok = s.GetNodeByID(nodeID) + assert.True(t, ok, "a database failure must leave the in-memory node available") + + _, err = s.db.GetNodeByID(nodeID) + assert.NoError(t, err, "a failed deletion must leave the durable node row available") +} + +func TestDeleteNodeReturnsRemovalOnPolicyFailure(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + node, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + s.polMan = failingSetNodesPolicyManager{PolicyManager: s.polMan} + + c, err := s.DeleteNode(node) + require.ErrorIs(t, err, errInjectedPolicyNodeUpdate) + assert.Equal(t, []types.NodeID{nodeID}, c.PeersRemoved, + "a committed deletion must still notify peers and stop the node's session") + + _, ok = s.GetNodeByID(nodeID) + assert.False(t, ok, "a committed deletion must remove the in-memory node") + + _, err = s.db.GetNodeByID(nodeID) + require.ErrorIs(t, err, gorm.ErrRecordNotFound, + "a committed deletion must remove the durable node row") +} // TestPreAuthKeyReauthRevertsNodeStoreOnDBFailure ensures a failed database // write during pre-auth-key re-registration does not leave the NodeStore diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 5b34c5d61..00703893d 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -183,9 +183,9 @@ type State struct { sshCheckAuth map[sshCheckPair]time.Time sshCheckMu sync.RWMutex - // persistMu serialises the re-read-and-write critical section in - // persistNodeToDB so the database row always converges on [NodeStore] - // rather than being clobbered by a stale caller snapshot. + // persistMu serialises node-row persistence and deletion so the database + // always converges on [NodeStore] rather than being clobbered by a stale + // caller snapshot or resurrected by an update racing with deletion. persistMu sync.Mutex // registerLocks serialises registration per machine key so concurrent @@ -592,15 +592,26 @@ func (s *State) SaveNode(node types.NodeView) (types.NodeView, change.Change, er } // DeleteNode permanently removes a node and cleans up associated resources. -// Returns whether policies changed and any error. This operation is irreversible. +// 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 +// still torn down after a committed deletion. func (s *State) DeleteNode(node types.NodeView) (change.Change, error) { - s.nodeStore.DeleteNode(node.ID()) + s.persistMu.Lock() err := s.db.DeleteNode(node.AsStruct()) if err != nil { + s.persistMu.Unlock() + return change.Change{}, err } + // The database is the durable source of truth. Only remove the in-memory + // node after its row is gone so a failed database write cannot make a live + // node look deleted until the next restart. + s.nodeStore.DeleteNode(node.ID()) + s.persistMu.Unlock() + s.ipAlloc.FreeIPs(node.IPs()) c := change.NodeRemoved(node.ID()) @@ -608,7 +619,7 @@ func (s *State) DeleteNode(node types.NodeView) (change.Change, error) { // Check if policy manager needs updating after node deletion policyChange, err := s.updatePolicyManagerNodes() if err != nil { - return change.Change{}, fmt.Errorf("updating policy manager after node deletion: %w", err) + return c, fmt.Errorf("updating policy manager after node deletion: %w", err) } if !policyChange.IsEmpty() {