diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index d3f0a60be..9216b8d34 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -621,6 +621,66 @@ func TestDeleteNodeTwiceReleasesAddressOnce(t *testing.T) { require.ErrorIs(t, err, db.ErrCouldNotAllocateIP) } +func TestPersistDuringDeleteDoesNotResurrectNode(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + node, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + s.nodeStore.Stop() + + entered, release := make(chan struct{}), make(chan struct{}) + + var enteredOnce sync.Once + + s.nodeStore = NewNodeStore( + types.Nodes{node.AsStruct()}, + func(nodes []types.NodeView) map[types.NodeID][]types.NodeID { + if len(nodes) == 0 { + enteredOnce.Do(func() { close(entered) }) + <-release + } + + return nil + }, + TestBatchSize, + TestBatchTimeout, + ) + s.nodeStore.Start() + + var releaseOnce sync.Once + + t.Cleanup(func() { releaseOnce.Do(func() { close(release) }) }) + + deleteDone := make(chan error, 1) + + go func() { + _, err := s.DeleteNode(node) + deleteDone <- err + }() + + <-entered + + persistDone := make(chan error, 1) + + go func() { + _, err := s.persistNode(node) + persistDone <- err + }() + + select { + case err := <-persistDone: + require.NoError(t, err) + case <-time.After(time.Second): + t.Error("persist blocked behind the NodeStore delete") + } + + _, err := s.db.GetNodeByID(nodeID) + require.ErrorIs(t, err, gorm.ErrRecordNotFound) + releaseOnce.Do(func() { close(release) }) + require.NoError(t, <-deleteDone) +} + 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 9dd9c4d01..77f374dbf 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -236,6 +236,10 @@ type State struct { // 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. + // Every node's persist takes it, so paths a node can trigger must not + // hold it across a NodeStore write. Node persists must stay UPDATE-only; + // then a persist between the row delete and the NodeStore delete changes + // nothing. persistMu sync.Mutex // selfRefresh holds nodes whose own DNS config inputs changed (NextDNS