state: persist live NodeStore node in persistNodeToDB

Avoids clobbering concurrent admin writes (tags, routes, rename).

hscontrol/state/persist_clobber_test.go

@@ -0,0 +1,71 @@
+package state
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"tailscale.com/tailcfg"
+)
+
+// TestPersistNodeDoesNotClobberConcurrentAdminWrite ensures that persisting a
+// node snapshot captured earlier (as UpdateNodeFromMapRequest does at the top
+// of its body) cannot overwrite a concurrent admin write (SetNodeTags) that
+// landed in between. NodeStore is the source of truth; the database row must
+// converge on it rather than reverting to the stale snapshot.
+func TestPersistNodeDoesNotClobberConcurrentAdminWrite(t *testing.T) {
+	dbPath, s, nodeID := persistTestSetup(t)
+
+	pol := `{
+		"tagOwners": {"tag:foo": ["persist-user@"]},
+		"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}]
+	}`
+	_, err := s.SetPolicy([]byte(pol))
+	require.NoError(t, err)
+
+	before, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok)
+	require.False(t, before.IsTagged(), "node should start user-owned")
+	require.True(t, before.UserID().Valid(), "node should start with a UserID")
+
+	// (1) Map-request captures the node snapshot (the stale view).
+	staleView, ok := s.nodeStore.GetNode(nodeID)
+	require.True(t, ok)
+
+	staleNode := staleView.AsStruct()
+	staleNode.Hostinfo = &tailcfg.Hostinfo{Hostname: "persist-node"}
+	staleView = staleNode.View()
+
+	// (2) Concurrent admin SetNodeTags lands in the window: NodeStore + DB
+	// become tagged and user ownership is cleared.
+	_, _, err = s.SetNodeTags(nodeID, []string{"tag:foo"})
+	require.NoError(t, err)
+
+	dbAfterAdmin, err := s.DB().GetNodeByID(nodeID)
+	require.NoError(t, err)
+	require.Equal(t, []string{"tag:foo"}, dbAfterAdmin.Tags.List(),
+		"precondition: admin SetNodeTags must have written the tag to the DB")
+
+	// (3) Map-request persists its stale snapshot.
+	_, _, err = s.persistNodeToDB(staleView)
+	require.NoError(t, err)
+
+	// The admin write must survive.
+	dbFinal, err := s.DB().GetNodeByID(nodeID)
+	require.NoError(t, err)
+	assert.Equal(t, []string{"tag:foo"}, dbFinal.Tags.List(),
+		"DB tags must reflect the admin SetNodeTags, not the stale persist")
+	assert.Nil(t, dbFinal.UserID,
+		"DB UserID must stay nil after tagging (tags XOR user ownership)")
+
+	// Restart: the divergence would surface here in production.
+	require.NoError(t, s.Close())
+	s2 := persistTestReopen(t, dbPath)
+
+	reloaded, ok := s2.GetNodeByID(nodeID)
+	require.True(t, ok, "node should reload from DB after restart")
+	assert.True(t, reloaded.IsTagged(),
+		"after restart the node must still be tagged")
+	assert.Equal(t, []string{"tag:foo"}, reloaded.AsStruct().Tags.List(),
+		"after restart the node must still carry tag:foo")
+}

hscontrol/state/state.go

@@ -170,6 +170,11 @@ type State struct {
 	// Ref: https://github.com/tailscale/tailscale/issues/7125
 	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 sync.Mutex
 }
 
 // NewState creates and initializes a new [State] instance, setting up the database,
@@ -487,13 +492,23 @@ func (s *State) persistNodeToDB(node types.NodeView) (types.NodeView, change.Cha
 		return types.NodeView{}, change.Change{}, ErrInvalidNodeView
 	}
 
-	// Verify the node still exists in [NodeStore] before persisting to database.
+	// [NodeStore] is the source of truth and every caller updates it before
-	// Without this check, we could hit a race condition where [NodeStore.UpdateNode]
+	// persisting. Re-read the authoritative node under persistMu and write
-	// returns a valid node from a batch update, then the node gets deleted (e.g.,
+	// that, rather than the caller's `node` view which may have been captured
-	// ephemeral node logout), and persistNodeToDB would incorrectly re-insert the
+	// earlier (e.g. at the top of UpdateNodeFromMapRequest) and gone stale
-	// deleted node into the database.
+	// behind a concurrent admin write such as SetNodeTags. Serialising the
-	_, exists := s.nodeStore.GetNode(node.ID())
+	// read+write keeps the database row converging on [NodeStore] instead of
+	// reverting it to an out-of-date column set.
+	//
+	// The same re-read also guards against the node having been deleted (e.g.
+	// ephemeral logout) between the caller's update and this persist: a missing
+	// node means we must not re-insert it.
+	s.persistMu.Lock()
+
+	fresh, exists := s.nodeStore.GetNode(node.ID())
 	if !exists {
+		s.persistMu.Unlock()
+
 		log.Warn().
 			EmbedObject(node).
 			Bool("is_ephemeral", node.IsEphemeral()).
@@ -502,13 +517,14 @@ func (s *State) persistNodeToDB(node types.NodeView) (types.NodeView, change.Cha
 		return types.NodeView{}, change.Change{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, node.ID())
 	}
 
-	nodePtr := node.AsStruct()
+	nodePtr := fresh.AsStruct()
 
 	// Explicitly select all node columns so GORM includes nil/zero-value
 	// fields (e.g. UserID=nil when converting a user-owned node to tagged).
 	// Omit "Expiry" here: expiry is only updated through explicit
 	// SetNodeExpiry calls or re-registration, not during MapRequest updates.
 	err := s.db.DB.Select(nodeUpdateColumns).Omit("Expiry").Updates(nodePtr).Error
+	s.persistMu.Unlock()
 	if err != nil {
 		return types.NodeView{}, change.Change{}, fmt.Errorf("saving node: %w", err)
 	}
@@ -516,14 +532,14 @@ func (s *State) persistNodeToDB(node types.NodeView) (types.NodeView, change.Cha
 	// Check if policy manager needs updating
 	c, err := s.updatePolicyManagerNodes()
 	if err != nil {
-		return nodePtr.View(), change.Change{}, fmt.Errorf("updating policy manager after node save: %w", err)
+		return fresh, change.Change{}, fmt.Errorf("updating policy manager after node save: %w", err)
 	}
 
 	if c.IsEmpty() {
 		c = change.NodeAdded(node.ID())
 	}
 
-	return node, c, nil
+	return fresh, c, nil
 }
 
 func (s *State) SaveNode(node types.NodeView) (types.NodeView, change.Change, error) {