state: gate reconnect PolicyChange on NodeNeedsPeerRecompute

Connect and Disconnect appended change.PolicyChange() on every reconnect. PolicyChange sets RequiresRuntimePeerComputation, so the batcher rebuilt a full netmap (packet filters, SSH policy, peer serialization) for every connected node — O(N) per reconnect, O(N^2) on a restart storm. On a small VM this saturated CPU after the v0.28 -> v0.29 upgrade.

Emit it only when the node's online state changes what peers compute: subnet routers, relay targets, and via targets. An ordinary reconnect now sends just the lightweight online/offline peer patch. Relay and via targets still recompute, so peers drop a stale PeerRelay allocation when a relay goes offline.

Fixes #3293

hscontrol/state/connect_test.go

@@ -0,0 +1,168 @@
+package state
+
+import (
+	"net/netip"
+	"testing"
+
+	"github.com/juanfont/headscale/hscontrol/types"
+	"github.com/juanfont/headscale/hscontrol/types/change"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"tailscale.com/tailcfg"
+)
+
+// runtimePeerComputationReasons returns the Reason of every change in cs that
+// carries RequiresRuntimePeerComputation. Each such change makes the batcher
+// rebuild a full netmap (packet filters, SSH policy, peer serialization) for
+// every connected node, so it is the expensive fan-out the issue is about.
+func runtimePeerComputationReasons(cs []change.Change) []string {
+	var reasons []string
+
+	for _, c := range cs {
+		if c.RequiresRuntimePeerComputation {
+			reasons = append(reasons, c.Reason)
+		}
+	}
+
+	return reasons
+}
+
+// hasPeerPatch reports whether any change carries a lightweight PeerChange
+// patch (e.g. the online/offline indicator). This is the cheap notification a
+// reconnect should produce instead of a full runtime recompute.
+func hasPeerPatch(cs []change.Change) bool {
+	for _, c := range cs {
+		if len(c.PeerPatches) > 0 {
+			return true
+		}
+	}
+
+	return false
+}
+
+// forcesPeerRecompute reports whether any change makes peers rebuild a full
+// netmap, whether via a full update (subnet-router path) or a runtime peer
+// computation (relay/via path).
+func forcesPeerRecompute(cs []change.Change) bool {
+	for _, c := range cs {
+		if c.IsFull() || c.RequiresRuntimePeerComputation {
+			return true
+		}
+	}
+
+	return false
+}
+
+// TestConnectDisconnectOrdinaryNodeNoRuntimeRecompute asserts that an ordinary
+// node coming online or going offline only sends the lightweight online/offline
+// peer patch and does not trigger a runtime peer recompute.
+//
+// State.Connect and State.Disconnect gate change.PolicyChange() (which sets
+// RequiresRuntimePeerComputation, forcing the batcher to rebuild a full netmap
+// for every connected node) on NodeNeedsPeerRecompute. An ordinary node is
+// neither a subnet router, a relay target, nor a via target, so the gate is
+// false and no recompute is emitted.
+//
+// Emitting that recompute unconditionally turned each reconnect into O(N) full
+// netmap rebuilds (and a reconnect storm into O(N^2)), which saturated CPU
+// after the v0.28 -> v0.29 upgrade.
+func TestConnectDisconnectOrdinaryNodeNoRuntimeRecompute(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	t.Run("connect", func(t *testing.T) {
+		cs, epoch := s.Connect(nodeID)
+		require.NotZero(t, epoch, "Connect should return a session epoch")
+
+		assert.True(t, hasPeerPatch(cs),
+			"Connect should still emit a lightweight online peer patch")
+
+		reasons := runtimePeerComputationReasons(cs)
+		assert.Empty(t, reasons,
+			"ordinary node connect must not trigger a runtime peer recompute; "+
+				"got RequiresRuntimePeerComputation changes: %v", reasons)
+	})
+
+	t.Run("disconnect", func(t *testing.T) {
+		// Connect first so Disconnect's epoch check passes.
+		_, epoch := s.Connect(nodeID)
+
+		cs, err := s.Disconnect(nodeID, epoch)
+		require.NoError(t, err)
+
+		assert.True(t, hasPeerPatch(cs),
+			"Disconnect should still emit a lightweight offline peer patch")
+
+		reasons := runtimePeerComputationReasons(cs)
+		assert.Empty(t, reasons,
+			"ordinary node disconnect must not trigger a runtime peer recompute; "+
+				"got RequiresRuntimePeerComputation changes: %v", reasons)
+	})
+}
+
+// TestConnectDisconnectRelayTargetTriggersRecompute locks the cap/relay case:
+// a relay target is not a subnet router, so the only thing that makes its
+// connect/disconnect emit a runtime peer recompute is the
+// NodeNeedsPeerRecompute gate. Peers must still receive that recompute so they
+// drop a stale PeerRelay allocation when the relay goes offline.
+func TestConnectDisconnectRelayTargetTriggersRecompute(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	// A cap/relay grant whose destination resolves to the node's owning
+	// user makes the node a relay target without making it a subnet router.
+	relayPolicy := `{"grants":[{"src":["*"],"dst":["persist-user@"],"app":{"tailscale.com/cap/relay":[{}]}}]}`
+	_, err := s.SetPolicy([]byte(relayPolicy))
+	require.NoError(t, err)
+
+	t.Run("connect", func(t *testing.T) {
+		cs, epoch := s.Connect(nodeID)
+		require.NotZero(t, epoch)
+
+		assert.NotEmpty(t, runtimePeerComputationReasons(cs),
+			"relay-target node connect must trigger a runtime peer recompute")
+	})
+
+	t.Run("disconnect", func(t *testing.T) {
+		_, epoch := s.Connect(nodeID)
+
+		cs, err := s.Disconnect(nodeID, epoch)
+		require.NoError(t, err)
+
+		assert.NotEmpty(t, runtimePeerComputationReasons(cs),
+			"relay-target node disconnect must trigger a runtime peer recompute")
+	})
+}
+
+// TestConnectDisconnectSubnetRouterForcesRecompute guards that a subnet router
+// still forces peers to recompute on connect/disconnect (primary-route
+// failover changes their AllowedIPs), so the gate does not over-suppress.
+func TestConnectDisconnectSubnetRouterForcesRecompute(t *testing.T) {
+	_, s, nodeID := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	route := netip.MustParsePrefix("10.0.0.0/24")
+	_, ok := s.nodeStore.UpdateNode(nodeID, func(n *types.Node) {
+		n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{route}}
+		n.ApprovedRoutes = []netip.Prefix{route}
+	})
+	require.True(t, ok)
+
+	t.Run("connect", func(t *testing.T) {
+		cs, epoch := s.Connect(nodeID)
+		require.NotZero(t, epoch)
+
+		assert.True(t, forcesPeerRecompute(cs),
+			"subnet router connect must force a peer recompute")
+	})
+
+	t.Run("disconnect", func(t *testing.T) {
+		_, epoch := s.Connect(nodeID)
+
+		cs, err := s.Disconnect(nodeID, epoch)
+		require.NoError(t, err)
+
+		assert.True(t, forcesPeerRecompute(cs),
+			"subnet router disconnect must force a peer recompute")
+	})
+}

hscontrol/state/state.go

@@ -594,9 +594,14 @@ func (s *State) Connect(id types.NodeID) ([]change.Change, uint64) {
 		c = append(c, change.NodeAdded(id))
 	}
 
-	// Coming online may re-enable cap/relay grants and identity-based
-	// aliases targeting this node, so peers need a fresh netmap.
-	c = append(c, change.PolicyChange())
+	// Only a node whose online state changes what peers compute (a subnet
+	// router, relay target, or via target) needs a full peer recompute.
+	// An ordinary node coming online just sends the lightweight online
+	// patch above; emitting a PolicyChange for it would force every peer
+	// to rebuild its netmap on every reconnect.
+	if s.polMan.NodeNeedsPeerRecompute(node) {
+		c = append(c, change.PolicyChange())
+	}
 
 	return c, epoch
 }
@@ -648,13 +653,15 @@ func (s *State) Disconnect(id types.NodeID, epoch uint64) ([]change.Change, erro
 		c = change.Change{}
 	}
 
-	// Going offline can affect policy compilation beyond subnet routes
-	// (cap/relay grants, tag/group aliases, via routes), so peers need
-	// a fresh netmap regardless of whether the primary moved.
-	//
-	// TODO(kradalby): fires one full netmap recompute per peer on
-	// every connect/disconnect. Coalesce in mapper/batcher.go:addToBatch.
-	cs := []change.Change{change.NodeOfflineFor(node), c, change.PolicyChange()}
+	// Only a node whose online state changes what peers compute (a subnet
+	// router, relay target, or via target) needs a full peer recompute.
+	// An ordinary node going offline just sends the lightweight offline
+	// patch; emitting a PolicyChange for it would force every peer to
+	// rebuild its netmap on every disconnect.
+	cs := []change.Change{change.NodeOfflineFor(node), c}
+	if s.polMan.NodeNeedsPeerRecompute(node) {
+		cs = append(cs, change.PolicyChange())
+	}
 
 	return cs, nil
 }