state: backfill only IPs into NodeStore

PutNode of the DB row dropped session state, stranding connected nodes
offline, and nil-dereferenced nodes without Hostinfo.

hscontrol/state/connect_test.go

@@ -331,3 +331,45 @@ func TestExpiredNodeSessionAccounting(t *testing.T) {
 		})
 	}
 }
+
+// TestBackfillNodeIPsKeepsLiveSessions guards the IP backfill against
+// replacing NodeStore nodes with their database rows, which drops the
+// runtime-only session count and strands a connected node offline.
+func TestBackfillNodeIPsKeepsLiveSessions(t *testing.T) {
+	dbPath, s, nodeID := persistTestSetup(t)
+	require.NoError(t, s.Close())
+
+	// Dropping the IPv6 prefix gives the backfill work to do.
+	cfg := persistTestConfig(dbPath)
+	cfg.PrefixV6 = nil
+
+	s, err := NewState(cfg)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = s.Close() })
+
+	_, firstGen := s.Connect(nodeID)
+	s.Connect(nodeID)
+
+	// Connected nodes carry the Hostinfo of their first MapRequest.
+	s.nodeStore.UpdateNode(nodeID, func(n *types.Node) {
+		n.Hostinfo = &tailcfg.Hostinfo{Hostname: "persist-node"}
+	})
+
+	changes, _, err := s.BackfillNodeIPs()
+	require.NoError(t, err)
+	require.NotEmpty(t, changes, "precondition: backfill must change the node")
+
+	nv, ok := s.GetNodeByID(nodeID)
+	require.True(t, ok)
+	assert.Len(t, nv.IPs(), 1, "backfill must drop the IPv6 address")
+
+	_, err = s.Disconnect(nodeID, firstGen)
+	require.NoError(t, err)
+
+	nv, ok = s.GetNodeByID(nodeID)
+	require.True(t, ok)
+
+	online, known := nv.IsOnline().GetOk()
+	require.True(t, known)
+	assert.True(t, online, "node must stay online while its second session lives")
+}

hscontrol/state/state.go

@@ -1149,35 +1149,28 @@ func (s *State) BackfillNodeIPs() ([]string, []change.Change, error) {
 
 	var readdressed []types.NodeID
 
-	// Refresh [NodeStore] after IP changes to ensure consistency
+	// Copy only the IPs into [NodeStore]: the database rows lack
+	// runtime-only state such as sessions and online status.
 	if len(changes) > 0 {
 		nodes, err := s.db.ListNodes()
 		if err != nil {
 			return changes, nil, fmt.Errorf("refreshing NodeStore after IP backfill: %w", err)
 		}
 
+		updates := make(map[types.NodeID]UpdateNodeFunc, len(nodes))
 		for _, node := range nodes {
-			// Preserve online status and NetInfo when refreshing from database
 			existingNode, exists := s.nodeStore.GetNode(node.ID)
-			if !exists || !slices.Equal(existingNode.IPs(), node.IPs()) {
+			if exists && !slices.Equal(existingNode.IPs(), node.IPs()) {
 				readdressed = append(readdressed, node.ID)
 			}
 
-			if exists && existingNode.Valid() {
-				node.IsOnline = new(existingNode.IsOnline().Get())
-
-				// TODO(kradalby): We should ensure we use the same hostinfo and node merge semantics
-				// when a node re-registers as we do when it sends a map request (UpdateNodeFromMapRequest).
-
-				// Preserve NetInfo from existing node to prevent loss during backfill
-				netInfo := netInfoFromMapRequest(node.ID, existingNode.Hostinfo().AsStruct(), node.Hostinfo)
-				node.Hostinfo = existingNode.Hostinfo().AsStruct()
-				node.Hostinfo.NetInfo = netInfo
+			updates[node.ID] = func(n *types.Node) {
+				n.IPv4 = node.IPv4
+				n.IPv6 = node.IPv6
 			}
-			// TODO(kradalby): This should just update the IP addresses, nothing else in the node store.
-			// We should avoid [NodeStore.PutNode] here.
-			_ = s.nodeStore.PutNode(*node)
 		}
+
+		s.nodeStore.UpdateNodes(updates)
 	}
 
 	// IPs are policy inputs: without this, clients only learned the new