diff --git a/hscontrol/state/connect_test.go b/hscontrol/state/connect_test.go index b41c86ee4..71fc06c30 100644 --- a/hscontrol/state/connect_test.go +++ b/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") +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 68cb135c1..d56e35050 100644 --- a/hscontrol/state/state.go +++ b/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