state: preserve committed node deletion changes

Updates #3410

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
 	})
 

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
 	})
 

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

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,

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

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
+	// persistMu serialises node-row persistence and deletion so the database
-	// persistNodeToDB so the database row always converges on [NodeStore]
+	// always converges on [NodeStore] rather than being clobbered by a stale
-	// rather than being clobbered by a stale caller snapshot.
+	// 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() {