mapper: skip responses left empty by visibility filtering

A peer change hidden from a recipient still produced a frame carrying only
ControlTime, one per sender per recipient. Drop it once the response is final.

hscontrol/mapper/batcher.go

@@ -3,6 +3,7 @@ package mapper
 import (
 	"errors"
 	"fmt"
+	"reflect"
 	"slices"
 	"sync"
 	"sync/atomic"
@@ -159,24 +160,49 @@ func generateMapResponse(
 		removed = nc.computePeerDiff(peerIDs)
 	}
 
+	// Visibility filtering can leave a broadcast with nothing for this
+	// recipient; the frame would only cost encode, write and client decode.
+	if emptyMapResponse(resp) {
+		resp = nil
+	}
+
 	return withRemovals(resp, removed), nil
 }
 
-// withRemovals returns resp and the peers removed from the node's view as the
+// emptyMapResponse reports whether resp carries nothing but ControlTime,
-// responses to send. The removals lead, in a response of their own: clients
+// which every built response has and clients only sample for clock skew.
-// before Tailscale v1.104 only report removed peers to IPN bus watchers opted
+// Any other non-zero field counts, an empty slice or map included, and so
-// out of full netmaps (the Android app since 1.100) from responses they apply
+// does a field headscale does not set today: unknown is sent, not dropped.
-// as deltas, and resp can carry DNSConfig, SSHPolicy, Node or Peers, which
+// Per response on the hot path, so it must stay allocation-free; see
-// force a full rebuild.
+// [BenchmarkEmptyMapResponse].
+func emptyMapResponse(resp *tailcfg.MapResponse) bool {
+	stripped := *resp
+	stripped.ControlTime = nil
+
+	return reflect.ValueOf(stripped).IsZero()
+}
+
+// withRemovals returns resp, if any, and the peers removed from the node's
+// view as the responses to send. The removals lead, in a response of their
+// own: clients before Tailscale v1.104 only report removed peers to IPN bus
+// watchers opted out of full netmaps (the Android app since 1.100) from
+// responses they apply as deltas, and resp can carry DNSConfig, SSHPolicy,
+// Node or Peers, which force a full rebuild.
 //
 // TODO(kradalby): when the tailscale/tailscale#15660 compat goes (see
 // capver.CanOldCodeBeCleanedUp), set resp.PeersRemoved and return resp alone.
 func withRemovals(resp *tailcfg.MapResponse, removed []tailcfg.NodeID) []*tailcfg.MapResponse {
-	if len(removed) == 0 {
+	var resps []*tailcfg.MapResponse
-		return []*tailcfg.MapResponse{resp}
+
+	if len(removed) > 0 {
+		resps = append(resps, &tailcfg.MapResponse{PeersRemoved: removed})
 	}
 
-	return []*tailcfg.MapResponse{{PeersRemoved: removed}, resp}
+	if resp != nil {
+		resps = append(resps, resp)
+	}
+
+	return resps
 }
 
 // handleNodeChange generates and sends the [tailcfg.MapResponse] values for a given node and [change.Change].

hscontrol/mapper/batcher_bench_test.go

@@ -65,6 +65,26 @@ func BenchmarkMultiChannelSend(b *testing.B) {
 	}
 }
 
+// BenchmarkEmptyMapResponse pins that the per-response emptiness check
+// does not allocate, on a frame it drops and on one it keeps.
+func BenchmarkEmptyMapResponse(b *testing.B) {
+	for _, tt := range []struct {
+		name string
+		resp *tailcfg.MapResponse
+	}{
+		{"empty", testMapResponse()},
+		{"peers", testMapResponseWithPeers(1, 2, 3)},
+	} {
+		b.Run(tt.name, func(b *testing.B) {
+			b.ReportAllocs()
+
+			for range b.N {
+				_ = emptyMapResponse(tt.resp)
+			}
+		})
+	}
+}
+
 // BenchmarkComputePeerDiff measures the cost of computing peer diffs at scale.
 func BenchmarkComputePeerDiff(b *testing.B) {
 	for _, peerCount := range []int{10, 100, 1000} {

hscontrol/mapper/batcher_test.go

@@ -1071,8 +1071,10 @@ func TestBatcherWorkQueueBatching(t *testing.T) {
 						t.Logf("Update %d: nil update", updateCount)
 					}
 				case <-timeout:
-					// Expected: 5 explicit changes + 1 initial from AddNode + 1 NodeOnline from wrapper = 7 updates
+					// Expected: 5 explicit changes + 1 initial from AddNode = 6 updates.
-					expectedUpdates := 7
+					// The wrapper's NodeOnline is about this node itself, which is
+					// not among its peers, so it leaves nothing to send.
+					expectedUpdates := 6
 					t.Logf("Received %d updates from %d changes (expected %d)",
 						updateCount, 5, expectedUpdates)
 
@@ -2976,3 +2978,76 @@ func TestSSHPolicySentOnlyWhenChanged(t *testing.T) {
 		assert.Nil(t, frame.SSHPolicy, "connection %d already holds the policy", i)
 	}
 }
+
+// TestInvisiblePeerChangeSendsNoFrame pins that a broadcast whose every
+// changed peer is hidden from the recipient produces no frame for it, while
+// the same change still reaches a recipient that sees the peer, and content
+// that does not depend on seeing the peer still reaches everyone.
+func TestInvisiblePeerChangeSendsNoFrame(t *testing.T) {
+	testData, cleanup := setupBatcherWithTestData(t, NewBatcherAndMapper, 2, 2, normalBufferSize)
+	defer cleanup()
+
+	b := testData.Batcher.Batcher
+
+	_, err := testData.State.SetPolicy([]byte(
+		`{"acls":[{"action":"accept","src":["autogroup:member"],"dst":["autogroup:self:*"]}]}`,
+	))
+	require.NoError(t, err)
+
+	// sender and sibling share a user; other belongs to the second user, so
+	// it cannot see either.
+	sender, sibling, other := &testData.Nodes[0], &testData.Nodes[1], &testData.Nodes[2]
+
+	for _, n := range []*node{sender, sibling, other} {
+		require.NoError(t, b.AddNode(n.n.ID, n.ch, tailcfg.CapabilityVersion(100), nil))
+		require.NotNil(t, expectReceive(t, n.ch, "initial map").Node)
+	}
+
+	deliver := func(to *node, c change.Change) {
+		t.Helper()
+
+		nc, ok := b.nodes.Load(to.n.ID)
+		require.True(t, ok)
+
+		nc.workMu.Lock()
+		defer nc.workMu.Unlock()
+
+		require.NoError(t, handleNodeChange(nc, b.mapper, c))
+	}
+
+	peerChanges := []struct {
+		name    string
+		c       change.Change
+		carries func(*tailcfg.MapResponse) bool
+	}{
+		{"changed", change.NodeAdded(sender.n.ID), func(r *tailcfg.MapResponse) bool {
+			return len(r.PeersChanged) == 1 && r.PeersChanged[0].ID == sender.n.ID.NodeID()
+		}},
+		{"patched", change.NodeOnline(sender.n.ID), func(r *tailcfg.MapResponse) bool {
+			return len(r.PeersChangedPatch) == 1 && r.PeersChangedPatch[0].NodeID == sender.n.ID.NodeID()
+		}},
+	}
+
+	for _, tt := range peerChanges {
+		deliver(other, tt.c)
+		expectNoReceive(t, other.ch, 20*time.Millisecond, tt.name+": other cannot see sender")
+
+		deliver(sibling, tt.c)
+		assert.True(t, tt.carries(expectReceive(t, sibling.ch, tt.name+": sibling sees sender")),
+			"%s: sibling must get the peer", tt.name)
+	}
+
+	deliver(other, change.PeersRemoved(sender.n.ID))
+	assert.NotEmpty(t, expectReceive(t, other.ch, "removal").PeersRemoved)
+
+	deliver(other, change.PolicyChange())
+	assert.NotNil(t, expectReceive(t, other.ch, "policy").PacketFilters)
+
+	_, _, err = testData.State.RenameNode(other.n.ID, "renamed")
+	require.NoError(t, err)
+	deliver(other, change.NodeAdded(other.n.ID))
+	assert.NotNil(t, expectReceive(t, other.ch, "self").Node)
+
+	deliver(other, change.PingNode(other.n.ID, &tailcfg.PingRequest{URL: "https://ping.test"}))
+	assert.NotNil(t, expectReceive(t, other.ch, "ping").PingRequest)
+}

hscontrol/mapper/batcher_unit_test.go

@@ -1277,3 +1277,31 @@ func TestMultiChannelSend_SSHPolicyPerConnection(t *testing.T) {
 	assert.Nil(t, expectReceive(t, chA, "a").SSHPolicy, "a already holds this policy")
 	assert.Same(t, data.SSHPolicy, expectReceive(t, chB, "b").SSHPolicy, "b holds an older policy")
 }
+
+func TestEmptyMapResponse(t *testing.T) {
+	now := time.Now()
+
+	tests := []struct {
+		name string
+		resp tailcfg.MapResponse
+		want bool
+	}{
+		{"zero", tailcfg.MapResponse{}, true},
+		{"control_time_only", tailcfg.MapResponse{ControlTime: &now}, true},
+		{"self", tailcfg.MapResponse{Node: &tailcfg.Node{}}, false},
+		{"peers_changed", tailcfg.MapResponse{PeersChanged: []*tailcfg.Node{{}}}, false},
+		{"empty_slice_counts", tailcfg.MapResponse{PeersChanged: []*tailcfg.Node{}}, false},
+		{"peers_removed", tailcfg.MapResponse{PeersRemoved: ids(1)}, false},
+		{"patch", tailcfg.MapResponse{PeersChangedPatch: []*tailcfg.PeerChange{{}}}, false},
+		{"packet_filters", tailcfg.MapResponse{PacketFilters: map[string][]tailcfg.FilterRule{"base": nil}}, false},
+		{"ping", tailcfg.MapResponse{PingRequest: &tailcfg.PingRequest{}}, false},
+		{"keepalive", tailcfg.MapResponse{KeepAlive: true}, false},
+		{"debug", tailcfg.MapResponse{Debug: &tailcfg.Debug{}}, false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, emptyMapResponse(&tt.resp))
+		})
+	}
+}

hscontrol/mapper/builder.go

@@ -345,8 +345,9 @@ func (b *MapResponseBuilder) Build() (*tailcfg.MapResponse, error) {
 		return nil, multierr.New(b.errs...)
 	}
 
-	// Dumps show the generated response; an unchanged SSHPolicy is
+	// Dumps show the generated response; [generateMapResponse] drops it
-	// dropped later, per connection, before it reaches the wire.
+	// if it is left empty, and an unchanged SSHPolicy is dropped later,
+	// per connection, before it reaches the wire.
 	if debugDumpMapResponsePath != "" {
 		writeDebugMapResponse(b.resp, b.debugType, b.nodeID)
 	}

hscontrol/mapper/mapper.go

@@ -401,9 +401,13 @@ func (m *mapper) buildFromChange(
 		builder.WithPeers(peers)
 	} else {
 		if len(resp.PeersChanged) > 0 {
-			peers := m.state.ListPeers(nodeID, resp.PeersChanged...)
+			// Visibility may leave none of the changed peers for this
-			builder.WithUserProfiles(peers)
+			// recipient; the profiles would then carry only its own
-			builder.WithPeerChanges(peers)
+			// user, which it already holds.
+			if peers := m.state.ListPeers(nodeID, resp.PeersChanged...); peers.Len() > 0 {
+				builder.WithUserProfiles(peers)
+				builder.WithPeerChanges(peers)
+			}
 		}
 
 		if len(resp.PeersRemoved) > 0 {

hscontrol/mapper/mapper_test.go

@@ -1017,6 +1017,9 @@ func TestGenerateMapResponseDebugOnlyOnFullMaps(t *testing.T) {
 				Regions: map[tailcfg.DERPRegionID]*tailcfg.DERPRegion{999: {RegionID: 999}},
 			},
 		},
+		// Without these the DERP map and DNS config renders carry
+		// nothing and no frame is generated at all.
+		TailcfgDNSConfig: &tailcfg.DNSConfig{Proxied: true},
 		Tuning: types.Tuning{
 			NodeStoreBatchSize:    state.TestBatchSize,
 			NodeStoreBatchTimeout: state.TestBatchTimeout,
@@ -1035,6 +1038,8 @@ func TestGenerateMapResponseDebugOnlyOnFullMaps(t *testing.T) {
 	require.NoError(t, err)
 	t.Cleanup(func() { _ = s.Close() })
 
+	s.SetDERPMap(cfg.DERP.DERPMap)
+
 	_, err = s.SetPolicy([]byte(`{"acls":[{"action":"accept","src":["*"],"dst":["*:*"]}]}`))
 	require.NoError(t, err)