From 1bbe59b98d858c2943e531285d4dc4fe0b1b470b Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 9 Sep 2026 14:35:28 +0000 Subject: [PATCH] state: rename persist helpers to say what they do Updates #3417 --- hscontrol/auth_test.go | 4 +-- hscontrol/state/ephemeral_test.go | 40 ++++++++++++------------- hscontrol/state/maprequest_noop_test.go | 2 +- hscontrol/state/persist_clobber_test.go | 2 +- hscontrol/state/persist_test.go | 10 +++---- hscontrol/state/state.go | 36 +++++++++++----------- 6 files changed, 47 insertions(+), 47 deletions(-) diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index 9549fd1f..a4c758eb 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -3868,7 +3868,7 @@ func TestDeletedPreAuthKeyNotRecreatedOnNodeUpdate(t *testing.T) { // The [state.NodeStore] may still have stale AuthKey data in memory. // Now simulate what happens when the node sends a [tailcfg.MapRequest] after a tailscaled restart. - // This triggers [state.State.persistNodeToDB] which calls GORM's Updates(). + // This triggers [state.State.persistNodeAndRefreshPolicy] which calls GORM's Updates(). // Simulate a [tailcfg.MapRequest] by updating the node through the state layer // This mimics what poll.go does when processing MapRequests @@ -3882,7 +3882,7 @@ func TestDeletedPreAuthKeyNotRecreatedOnNodeUpdate(t *testing.T) { } // Process the [tailcfg.MapRequest]-like update - // This calls [state.State.UpdateNodeFromMapRequest] which eventually calls [state.State.persistNodeToDB] + // This calls [state.State.UpdateNodeFromMapRequest] which eventually calls [state.State.persistNodeAndRefreshPolicy] _, err = app.state.UpdateNodeFromMapRequest(node.ID(), mapReq) require.NoError(t, err, "UpdateNodeFromMapRequest should succeed") t.Log("Simulated MapRequest update completed") diff --git a/hscontrol/state/ephemeral_test.go b/hscontrol/state/ephemeral_test.go index 5c755687..b7a69193 100644 --- a/hscontrol/state/ephemeral_test.go +++ b/hscontrol/state/ephemeral_test.go @@ -148,9 +148,9 @@ func TestUpdateNodeReturnsInvalidWhenDeletedInSameBatch(t *testing.T) { } } -// TestPersistNodeToDBPreventsRaceCondition tests that persistNodeToDB correctly handles +// TestPersistNodeToDBPreventsRaceCondition tests that persistNodeAndRefreshPolicy correctly handles // the race condition where a node is deleted after UpdateNode returns but before -// persistNodeToDB is called. This reproduces the ephemeral node deletion bug. +// persistNodeAndRefreshPolicy is called. This reproduces the ephemeral node deletion bug. func TestPersistNodeToDBPreventsRaceCondition(t *testing.T) { node := createTestNode(3, 1, "test-user", "test-node-3") @@ -180,19 +180,19 @@ func TestPersistNodeToDBPreventsRaceCondition(t *testing.T) { // Now try to use the updatedNode from before the deletion // In the old code, this would re-insert the node into the database - // With our fix, GetNode check in persistNodeToDB should prevent this + // With our fix, GetNode check in persistNodeAndRefreshPolicy should prevent this - // Simulate what persistNodeToDB does - check if node still exists + // Simulate what persistNodeAndRefreshPolicy does - check if node still exists _, exists := store.GetNode(updatedNode.ID()) if !exists { - t.Log("SUCCESS: persistNodeToDB check would prevent re-insertion of deleted node") + t.Log("SUCCESS: persistNodeAndRefreshPolicy check would prevent re-insertion of deleted node") } else { t.Error("BUG: Node still exists in NodeStore after deletion") } // The key assertion: after deletion, attempting to persist the old updatedNode // should fail because the node no longer exists in NodeStore - assert.False(t, exists, "persistNodeToDB should detect node was deleted and refuse to persist") + assert.False(t, exists, "persistNodeAndRefreshPolicy should detect node was deleted and refuse to persist") } // TestEphemeralNodeLogoutRaceCondition tests the specific race condition that occurs @@ -200,7 +200,7 @@ func TestPersistNodeToDBPreventsRaceCondition(t *testing.T) { // 1. UpdateNodeFromMapRequest calls UpdateNode and receives a node view // 2. Concurrently, handleLogout is called for the ephemeral node and calls DeleteNode // 3. UpdateNode and DeleteNode get batched together -// 4. If UpdateNode's result is used to call persistNodeToDB after the deletion, +// 4. If UpdateNode's result is used to call persistNodeAndRefreshPolicy after the deletion, // the node could be re-inserted into the database even though it was deleted func TestEphemeralNodeLogoutRaceCondition(t *testing.T) { ephemeralNode := createTestNode(4, 1, "test-user", "ephemeral-node") @@ -263,10 +263,10 @@ func TestEphemeralNodeLogoutRaceCondition(t *testing.T) { if updateOk && updatedNode.Valid() { t.Log("UpdateNode returned valid node, but node is deleted - this is the race condition") - // In the real code, this would cause persistNodeToDB to be called with updatedNode - // The fix in persistNodeToDB checks if the node still exists: + // In the real code, this would cause persistNodeAndRefreshPolicy to be called with updatedNode + // The fix in persistNodeAndRefreshPolicy checks if the node still exists: _, stillExists := store.GetNode(updatedNode.ID()) - assert.False(t, stillExists, "persistNodeToDB should check NodeStore and find node deleted") + assert.False(t, stillExists, "persistNodeAndRefreshPolicy should check NodeStore and find node deleted") } else if !updateOk || !updatedNode.Valid() { t.Log("UpdateNode correctly returned invalid/not-ok result (delete happened in same batch)") } @@ -280,7 +280,7 @@ func TestEphemeralNodeLogoutRaceCondition(t *testing.T) { // 4. handleLogout calls DeleteNode for ephemeral node // 5. UpdateNode and DeleteNode batch together // 6. UpdateNode returns a valid node (from before delete in batch) -// 7. persistNodeToDB is called with the stale valid node +// 7. persistNodeAndRefreshPolicy is called with the stale valid node // 8. Node gets re-inserted into database instead of staying deleted. func TestUpdateNodeFromMapRequestEphemeralLogoutSequence(t *testing.T) { ephemeralNode := createTestNode(5, 1, "test-user", "ephemeral-node-5") @@ -340,12 +340,12 @@ func TestUpdateNodeFromMapRequestEphemeralLogoutSequence(t *testing.T) { // but the node was deleted in the same batch t.Log("UpdateNode returned valid node even though node was deleted") - // The fix: persistNodeToDB must check NodeStore before persisting + // The fix: persistNodeAndRefreshPolicy must check NodeStore before persisting _, checkExists := store.GetNode(result.node.ID()) if checkExists { t.Error("BUG: Node still exists in NodeStore after deletion - should be impossible") } else { - t.Log("SUCCESS: persistNodeToDB would detect node is deleted and refuse to persist") + t.Log("SUCCESS: persistNodeAndRefreshPolicy would detect node is deleted and refuse to persist") } } else { t.Log("UpdateNode correctly indicated node was deleted (returned invalid or not-ok)") @@ -407,15 +407,15 @@ func TestUpdateNodeDeletedInSameBatchReturnsInvalid(t *testing.T) { assert.False(t, result.node.Valid(), "UpdateNode should return invalid node when node deleted in same batch") } -// TestPersistNodeToDBChecksNodeStoreBeforePersist verifies that persistNodeToDB +// TestPersistNodeToDBChecksNodeStoreBeforePersist verifies that persistNodeAndRefreshPolicy // checks if the node still exists in NodeStore before persisting to database. // This prevents the race condition where: // 1. UpdateNodeFromMapRequest calls UpdateNode and gets a valid node // 2. Ephemeral node logout calls DeleteNode // 3. UpdateNode and DeleteNode batch together // 4. UpdateNode returns a valid node (from before delete in batch) -// 5. UpdateNodeFromMapRequest calls persistNodeToDB with the stale node -// 6. persistNodeToDB must detect the node is deleted and refuse to persist. +// 5. UpdateNodeFromMapRequest calls persistNodeAndRefreshPolicy with the stale node +// 6. persistNodeAndRefreshPolicy must detect the node is deleted and refuse to persist. func TestPersistNodeToDBChecksNodeStoreBeforePersist(t *testing.T) { ephemeralNode := createTestNode(7, 1, "test-user", "ephemeral-node-7") ephemeralNode.AuthKey = &types.PreAuthKey{ @@ -448,8 +448,8 @@ func TestPersistNodeToDBChecksNodeStoreBeforePersist(t *testing.T) { assert.False(c, exists, "node should be deleted from NodeStore") }, 1*time.Second, 10*time.Millisecond, "waiting for node to be deleted") - // 4. Simulate what persistNodeToDB does - check if node still exists - // The fix in persistNodeToDB checks NodeStore before persisting: + // 4. Simulate what persistNodeAndRefreshPolicy does - check if node still exists + // The fix in persistNodeAndRefreshPolicy checks NodeStore before persisting: // if !exists { return error } // This prevents re-inserting the deleted node into the database @@ -458,9 +458,9 @@ func TestPersistNodeToDBChecksNodeStoreBeforePersist(t *testing.T) { _, stillExists := store.GetNode(updatedNode.ID()) assert.False(t, stillExists, "but node should be deleted from NodeStore") - // This is the critical test: persistNodeToDB must check NodeStore + // This is the critical test: persistNodeAndRefreshPolicy must check NodeStore // and refuse to persist if the node doesn't exist anymore - // The actual persistNodeToDB implementation does: + // The actual persistNodeAndRefreshPolicy implementation does: // _, exists := s.nodeStore.GetNode(node.ID()) // if !exists { return error } } diff --git a/hscontrol/state/maprequest_noop_test.go b/hscontrol/state/maprequest_noop_test.go index 6783e540..ff2f8517 100644 --- a/hscontrol/state/maprequest_noop_test.go +++ b/hscontrol/state/maprequest_noop_test.go @@ -12,7 +12,7 @@ import ( // TestNoOpMapRequestSkipsPersist ensures an identical, no-op MapRequest does // not issue a database UPDATE (nor the O(n) policy SetNodes scan that follows -// persistNodeToDB). The node state is unchanged, so persisting is pure waste on +// persistNodeAndRefreshPolicy). The node state is unchanged, so persisting is pure waste on // the hot map-request path. func TestNoOpMapRequestSkipsPersist(t *testing.T) { _, s, nodeID := persistTestSetup(t) diff --git a/hscontrol/state/persist_clobber_test.go b/hscontrol/state/persist_clobber_test.go index b2ec961d..860f578d 100644 --- a/hscontrol/state/persist_clobber_test.go +++ b/hscontrol/state/persist_clobber_test.go @@ -47,7 +47,7 @@ func TestPersistNodeDoesNotClobberConcurrentAdminWrite(t *testing.T) { "precondition: admin SetNodeTags must have written the tag to the DB") // (3) Map-request persists its stale snapshot. - _, _, err = s.persistNodeToDB(staleView) + _, _, err = s.persistNodeAndRefreshPolicy(staleView) require.NoError(t, err) // The admin write must survive. diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index def1ae73..762f1bc7 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -127,7 +127,7 @@ func TestPersistEmptyApprovedRoutes(t *testing.T) { // TestPersistEmptyTags exercises the same persist path for the tags // column. State.SetNodeTags rejects an empty slice at the API level // (tags are one-way), so the test drives the bug surface directly via -// NodeStore + persistNodeToDB, which is the same code path the public +// NodeStore + persistNodeAndRefreshPolicy, which is the same code path the public // SetApprovedRoutes call exercises. func TestPersistEmptyTags(t *testing.T) { dbPath, s, nodeID := persistTestSetup(t) @@ -140,7 +140,7 @@ func TestPersistEmptyTags(t *testing.T) { seeded, ok := s.nodeStore.GetNode(nodeID) require.True(t, ok) - _, _, err := s.persistNodeToDB(seeded) + _, _, err := s.persistNodeAndRefreshPolicy(seeded) require.NoError(t, err) gotAfterSeed, err := s.DB().GetNodeByID(nodeID) @@ -153,7 +153,7 @@ func TestPersistEmptyTags(t *testing.T) { }) require.True(t, ok) - _, _, err = s.persistNodeToDB(cleared) + _, _, err = s.persistNodeAndRefreshPolicy(cleared) require.NoError(t, err) gotAfterClear, err := s.DB().GetNodeByID(nodeID) @@ -188,7 +188,7 @@ func TestPersistEmptyEndpoints(t *testing.T) { seeded, ok := s.nodeStore.GetNode(nodeID) require.True(t, ok) - _, _, err := s.persistNodeToDB(seeded) + _, _, err := s.persistNodeAndRefreshPolicy(seeded) require.NoError(t, err) gotAfterSeed, err := s.DB().GetNodeByID(nodeID) @@ -201,7 +201,7 @@ func TestPersistEmptyEndpoints(t *testing.T) { }) require.True(t, ok) - _, _, err = s.persistNodeToDB(cleared) + _, _, err = s.persistNodeAndRefreshPolicy(cleared) require.NoError(t, err) gotAfterClear, err := s.DB().GetNodeByID(nodeID) diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index a6f94d00..1fede1d6 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -90,7 +90,7 @@ var ErrNodeNameNotUnique = errors.New("node name is not unique") // - IsOnline: runtime-only field (gorm:"-"). // // Expiry is included here but may be omitted at call sites that must -// not touch it (see persistNodeToDB). +// not touch it (see persistNodeAndRefreshPolicy). var nodeUpdateColumns = []string{ "MachineKey", "NodeKey", @@ -509,11 +509,11 @@ func (s *State) ListAllUsers() ([]types.User, error) { return s.db.ListUsers(nil) } -// persistNodeRowToDB writes the node's database row, re-reading the +// persistNode writes the node's database row, re-reading the // authoritative copy from [NodeStore], without touching the policy manager. // Batch callers (e.g. autoApproveNodes) use it to write many rows and then // trigger a single policy rebuild instead of one per node. -func (s *State) persistNodeRowToDB(node types.NodeView) (types.NodeView, error) { +func (s *State) persistNode(node types.NodeView) (types.NodeView, error) { if !node.Valid() { return types.NodeView{}, ErrInvalidNodeView } @@ -559,11 +559,11 @@ func (s *State) persistNodeRowToDB(node types.NodeView) (types.NodeView, error) return fresh, nil } -// persistNodeToDB saves the given node state to the database and refreshes the +// persistNodeAndRefreshPolicy saves the given node state to the database and refreshes the // policy manager. The exact row written comes from [NodeStore]; see -// [State.persistNodeRowToDB]. -func (s *State) persistNodeToDB(node types.NodeView) (types.NodeView, change.Change, error) { - fresh, err := s.persistNodeRowToDB(node) +// [State.persistNode]. +func (s *State) persistNodeAndRefreshPolicy(node types.NodeView) (types.NodeView, change.Change, error) { + fresh, err := s.persistNode(node) if err != nil { return types.NodeView{}, change.Change{}, err } @@ -588,7 +588,7 @@ func (s *State) SaveNode(node types.NodeView) (types.NodeView, change.Change, er resultNode := s.nodeStore.PutNode(*nodePtr) // Then save to database using the result from [NodeStore.PutNode] - return s.persistNodeToDB(resultNode) + return s.persistNodeAndRefreshPolicy(resultNode) } // DeleteNode permanently removes a node and cleans up associated resources. @@ -727,7 +727,7 @@ func (s *State) Disconnect(id types.NodeID, epoch uint64) ([]change.Change, erro // Persist LastSeen best-effort: [NodeStore] already reflects offline // and peers still need the change notifications below. - _, c, err := s.persistNodeToDB(node) + _, c, err := s.persistNodeAndRefreshPolicy(node) if err != nil { log.Error().Err(err).EmbedObject(node).Msg("failed to update last seen in database") @@ -934,7 +934,7 @@ func (s *State) SetNodeExpiry(nodeID types.NodeID, expiry *time.Time) (types.Nod return types.NodeView{}, change.Change{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, nodeID) } - // Persist expiry change to database directly since persistNodeToDB omits expiry. + // Persist expiry change to database directly since persistNodeAndRefreshPolicy omits expiry. err := s.db.NodeSetExpiry(nodeID, expiry) if err != nil { return types.NodeView{}, change.Change{}, fmt.Errorf("setting node expiry in database: %w", err) @@ -1005,13 +1005,13 @@ func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView, return types.NodeView{}, change.Change{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, nodeID) } - nodeView, c, err := s.persistNodeToDB(n) + nodeView, c, err := s.persistNodeAndRefreshPolicy(n) if err != nil { return nodeView, c, err } // Set OriginNode so the mapper knows to include self info for this node. - // When tags change, persistNodeToDB returns PolicyChange which doesn't set OriginNode, + // When tags change, persistNodeAndRefreshPolicy returns PolicyChange which doesn't set OriginNode, // so the mapper's self-update check fails and the node never sees its new tags. // Setting OriginNode ensures the node gets a self-update with the new tags. c.OriginNode = nodeID @@ -1041,7 +1041,7 @@ func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (t } // Persist the node changes to the database - nodeView, c, err := s.persistNodeToDB(n) + nodeView, c, err := s.persistNodeAndRefreshPolicy(n) if err != nil { return types.NodeView{}, change.Change{}, err } @@ -1081,7 +1081,7 @@ func (s *State) RenameNode(nodeID types.NodeID, newName string) (types.NodeView, } } - return s.persistNodeToDB(view) + return s.persistNodeAndRefreshPolicy(view) } // BackfillNodeIPs assigns IP addresses to nodes that don't have them. @@ -3000,7 +3000,7 @@ func (s *State) autoApproveNodes() ([]change.Change, error) { continue } - _, err := s.persistNodeRowToDB(fresh) + _, err := s.persistNode(fresh) if err != nil { return nil, err } @@ -3098,7 +3098,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest hostinfoChanged = !hostinfoEqual(currentNode.View(), req.Hostinfo) // A change carrying only an updated LastSeen is not worth a full-row - // database UPDATE plus the O(n) policy rescan persistNodeToDB triggers: + // database UPDATE plus the O(n) policy rescan persistNodeAndRefreshPolicy triggers: // LastSeen is best-effort and rides along the next substantive write. // PeerChangeFromMapRequest always stamps LastSeen, so test the other // fields explicitly. @@ -3241,13 +3241,13 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest // A no-op MapRequest (identical re-send / reconnect with matching state) // leaves the node untouched, so skip the full-row UPDATE and the O(n) - // policy SetNodes scan that persistNodeToDB performs. + // policy SetNodes scan that persistNodeAndRefreshPolicy performs. policyChange := change.Change{} if persistWorthy { var err error - _, policyChange, err = s.persistNodeToDB(updatedNode) + _, policyChange, err = s.persistNodeAndRefreshPolicy(updatedNode) if err != nil { return change.Change{}, fmt.Errorf("saving to database: %w", err) }