state: rename persist helpers to say what they do

Updates #3417

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

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

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)

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.

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)

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