mapper: send removed peers as their own delta

Removals derived from a policy change, full update or reconnect rode a
response whose DNSConfig, SSHPolicy, Node or Peers force a full rebuild.

Updates tailscale/tailscale#15660

CHANGELOG.md

@@ -116,6 +116,7 @@ clients, and how to run the same setup without Nix.
 - Improve systemd service file hardening [#3341](https://github.com/juanfont/headscale/pull/3341)
 - Fix `headscale users destroy`/`rename` reporting "multiple users match query" when no user matches; an ambiguous match now lists the matching users [#3476](https://github.com/juanfont/headscale/pull/3476)
 - Deleting a user that still owns nodes now lists the nodes (ID and hostname) that must be deleted first [#3475](https://github.com/juanfont/headscale/pull/3475)
+- Fix deleted nodes, and peers hidden by a policy change, staying listed in the Tailscale Android app; removed peers are now sent as their own incremental map update [#3492](https://github.com/juanfont/headscale/pull/3492)
 - Headscale now requires Go 1.27 to build
 - `headscale preauthkeys create --user` accepts a user name as well as an ID
 - `derp.paths` files may be Tailscale JSON or HuJSON DERP maps as well as YAML

hscontrol/capver/capver.go

@@ -15,14 +15,22 @@ import (
 // minVersionParts is the minimum number of version parts needed for major.minor.
 const minVersionParts = 2
 
+// fullNetmapRemovalsCapVer is the capability version of Tailscale main when
+// clients started reporting peers a full netmap drops as removed
+// (tailscale/tailscale#15660); v1.104 is the first release at or above it.
+const fullNetmapRemovalsCapVer tailcfg.CapabilityVersion = 148
+
 // CanOldCodeBeCleanedUp is called at server startup to panic when
 // [MinSupportedCapabilityVersion] has crossed a threshold at which a
 // backwards-compat emit path can be deleted. Each entry pairs a
 // [tailcfg.CapabilityVersion] threshold with the message identifying
-// the code to remove; today there are none.
+// the code to remove.
 //
 // All capability-version-gated cleanups should be registered here.
 func CanOldCodeBeCleanedUp() {
+	if MinSupportedCapabilityVersion >= fullNetmapRemovalsCapVer {
+		panic("the tailscale/tailscale#15660 compat can be cleaned up: grep for it (hscontrol/mapper/batcher.go and its tests)")
+	}
 }
 
 func tailscaleVersSorted() []string {

hscontrol/mapper/batcher.go

@@ -78,13 +78,18 @@ type nodeConnection interface {
 	updateSentPeers(resp *tailcfg.MapResponse)
 }
 
-// generateMapResponse generates a [tailcfg.MapResponse] for the given [types.NodeID] based on the provided [change.Change].
-func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*tailcfg.MapResponse, error) {
+// generateMapResponse generates the [tailcfg.MapResponse] values to send, in
+// order, for the given [types.NodeID] based on the provided [change.Change].
+func generateMapResponse(
+	nc nodeConnection,
+	mapper *mapper,
+	r change.Change,
+) ([]*tailcfg.MapResponse, error) {
 	nodeID := nc.nodeID()
 	version := nc.version()
 
 	if r.IsEmpty() {
-		return nil, nil //nolint:nilnil // Empty response means nothing to send
+		return nil, nil
 	}
 
 	if nodeID == 0 {
@@ -97,7 +102,7 @@ func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*t
 
 	// Handle self-only responses
 	if r.IsSelfOnly() && r.TargetNode != nodeID {
-		return nil, nil //nolint:nilnil // No response needed for other nodes when self-only
+		return nil, nil
 	}
 
 	// Check if this is a self-update (the changed node is the receiving node).
@@ -106,7 +111,8 @@ func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*t
 	isSelfUpdate := r.OriginNode != 0 && r.OriginNode == nodeID
 
 	var (
-		mapResp *tailcfg.MapResponse
+		resp    *tailcfg.MapResponse
+		removed []tailcfg.NodeID
 		err     error
 	)
 
@@ -122,51 +128,60 @@ func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*t
 			currentPeerIDs = append(currentPeerIDs, peer.ID().NodeID())
 		}
 
-		removedPeers := nc.computePeerDiff(currentPeerIDs)
+		removed = nc.computePeerDiff(currentPeerIDs)
 		// Include self node when this is a self-update (e.g., node's own tags changed)
 		// so the node sees its updated self info along with new packet filters.
-		mapResp, err = mapper.policyChangeResponse(nodeID, version, removedPeers, currentPeers, isSelfUpdate)
+		resp, err = mapper.policyChangeResponse(nodeID, version, currentPeers, isSelfUpdate)
 	} else if isSelfUpdate {
 		// Non-policy self-update: just send the self node info
-		mapResp, err = mapper.selfMapResponse(nodeID, version)
+		resp, err = mapper.selfMapResponse(nodeID, version)
 	} else {
-		mapResp, err = mapper.buildFromChange(nodeID, version, &r)
+		resp, err = mapper.buildFromChange(nodeID, version, &r)
 	}
 
 	if err != nil {
 		return nil, fmt.Errorf("generating map response for nodeID %d: %w", nodeID, err)
 	}
 
-	// When a full update (SendAllPeers=true) produces zero visible peers
-	// (e.g., a restrictive policy isolates this node), the resulting
-	// [tailcfg.MapResponse] has Peers: []*tailcfg.Node{} (empty non-nil slice).
-	//
-	// The Tailscale client only treats Peers as a full authoritative
-	// replacement when len(Peers) > 0 (controlclient/map.go:462).
-	// An empty Peers slice is indistinguishable from a delta response,
-	// so the client silently preserves its existing peer state.
-	//
-	// This matters when a [change.FullUpdate] replaces a pending
-	// [change.PolicyChange] in the batcher ([Batcher.addToBatch]
-	// short-circuits on [change.HasFull]). The [change.PolicyChange]
-	// would have computed PeersRemoved via
-	// [multiChannelNodeConn.computePeerDiff], but the [change.FullUpdate]
-	// path uses [MapResponseBuilder.WithPeers] which sets Peers: [].
-	//
-	// Fix: when a full update results in zero peers, compute the diff
-	// against lastSentPeers and add explicit PeersRemoved entries so
-	// the client correctly clears its stale peer state.
-	if mapResp != nil && r.SendAllPeers && len(mapResp.Peers) == 0 {
-		removedPeers := nc.computePeerDiff(nil)
-		if len(removedPeers) > 0 {
-			mapResp.PeersRemoved = removedPeers
-		}
+	if resp == nil {
+		return nil, nil
 	}
 
-	return mapResp, nil
+	// A full peer list replaces the node's peer set, so peers missing from
+	// it are removals. Clients take Peers as authoritative only when it is
+	// non-empty, so a full update that isolates a node (e.g.
+	// [Batcher.addToBatch] replacing a pending [change.PolicyChange] with a
+	// [change.FullUpdate]) needs them sent explicitly.
+	if resp.Peers != nil {
+		peerIDs := make([]tailcfg.NodeID, 0, len(resp.Peers))
+		for _, peer := range resp.Peers {
+			peerIDs = append(peerIDs, peer.ID)
+		}
+
+		removed = nc.computePeerDiff(peerIDs)
+	}
+
+	return withRemovals(resp, removed), nil
 }
 
-// handleNodeChange generates and sends a [tailcfg.MapResponse] for a given node and [change.Change].
+// withRemovals returns resp 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 {
+		return []*tailcfg.MapResponse{resp}
+	}
+
+	return []*tailcfg.MapResponse{{PeersRemoved: removed}, resp}
+}
+
+// handleNodeChange generates and sends the [tailcfg.MapResponse] values for a given node and [change.Change].
 func handleNodeChange(nc nodeConnection, mapper *mapper, r change.Change) error {
 	if nc == nil {
 		return ErrNodeConnectionNil
@@ -176,35 +191,31 @@ func handleNodeChange(nc nodeConnection, mapper *mapper, r change.Change) error
 
 	log.Debug().Caller().Uint64(zf.NodeID, nodeID.Uint64()).Str(zf.Reason, r.Reason).Msg("node change processing started")
 
-	data, err := generateMapResponse(nc, mapper, r)
+	resps, err := generateMapResponse(nc, mapper, r)
 	if err != nil {
 		return fmt.Errorf("generating map response for node %d: %w", nodeID, err)
 	}
 
-	if data == nil {
-		// No data to send is valid for some response types
-		return nil
-	}
+	for _, resp := range resps {
+		err = nc.send(resp)
+		if err != nil {
+			// If the node has no active connections, the data was not
+			// delivered. Do not update lastSentPeers — recording phantom
+			// peer state would corrupt future computePeerDiff calculations,
+			// causing the node to miss peer additions or removals after
+			// reconnection.
+			if errors.Is(err, errNoActiveConnections) {
+				return nil
+			}
 
-	// Send the map response
-	err = nc.send(data)
-	if err != nil {
-		// If the node has no active connections, the data was not
-		// delivered. Do not update lastSentPeers — recording phantom
-		// peer state would corrupt future computePeerDiff calculations,
-		// causing the node to miss peer additions or removals after
-		// reconnection.
-		if errors.Is(err, errNoActiveConnections) {
-			return nil
+			return fmt.Errorf("sending map response to node %d: %w", nodeID, err)
 		}
 
-		return fmt.Errorf("sending map response to node %d: %w", nodeID, err)
+		// Update peer tracking only after confirmed delivery to at
+		// least one active connection.
+		nc.updateSentPeers(resp)
 	}
 
-	// Update peer tracking only after confirmed delivery to at
-	// least one active connection.
-	nc.updateSentPeers(data)
-
 	return nil
 }
 
@@ -337,6 +348,29 @@ func (b *Batcher) AddNode(
 		// path, and under workMu so a concurrent async bundle for this node
 		// cannot interleave its own lastSentPeers update.
 		nodeConn.workMu.Lock()
+
+		// Peers removed while the node had no stream: the initial map is a
+		// full rebuild, so follow it with the removal as a delta, see
+		// [withRemovals].
+		//
+		// TODO(kradalby): delete with the tailscale/tailscale#15660 compat, see
+		// capver.CanOldCodeBeCleanedUp.
+		peerIDs := make([]tailcfg.NodeID, 0, len(initialMap.Peers))
+		for _, peer := range initialMap.Peers {
+			peerIDs = append(peerIDs, peer.ID)
+		}
+
+		removed := make([]types.NodeID, 0)
+		for _, peerID := range nodeConn.computePeerDiff(peerIDs) {
+			removed = append(removed, types.NodeID(peerID)) //nolint:gosec // NodeID types are equivalent
+		}
+
+		if len(removed) > 0 {
+			rm := change.PeersRemoved(removed...)
+			rm.TargetNode = id
+			b.AddWork(rm)
+		}
+
 		nodeConn.updateSentPeers(initialMap)
 		nodeConn.workMu.Unlock()
 
@@ -506,9 +540,12 @@ func (b *Batcher) worker(workerID int) {
 					// node waits until the initial map is sent.
 					nc.workMu.Lock()
 
-					var err error
-
-					result.mapResponse, err = generateMapResponse(nc, b.mapper, w.changes[0])
+					resps, err := generateMapResponse(nc, b.mapper, w.changes[0])
+					if len(resps) > 0 {
+						// The initial map must be the stream's first frame; AddNode
+						// sends any removals ahead of it after it instead.
+						result.mapResponse = resps[len(resps)-1]
+					}
 
 					result.err = err
 					if result.err != nil {

hscontrol/mapper/batcher_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"tailscale.com/tailcfg"
+	"tailscale.com/types/netmap"
 )
 
 var errNodeNotFoundAfterAdd = errors.New("node not found after adding to batcher")
@@ -2358,3 +2359,91 @@ func TestAddWorkPeersRemovedNotTreatedAsEmpty(t *testing.T) {
 	require.Equal(t, 3, countNodesPending(lb.b),
 		"surviving 3 nodes must have the removal pending")
 }
+
+// TestHandleNodeChangeSendsRemovalsAsDelta checks that peers missing from a
+// response listing the node's complete peer set go out first as a response
+// of their own that clients can apply as a delta, and that the full-set
+// response never carries them. Clients only report removals to IPN bus
+// watchers opted out of full netmaps from delta responses.
+//
+// TODO(kradalby): with the tailscale/tailscale#15660 compat gone, removals
+// ride the full-set response; adapt this and TestHandleNodeChangeRetryAfterRemoval.
+func TestHandleNodeChangeSendsRemovalsAsDelta(t *testing.T) {
+	tests := []struct {
+		name  string
+		nodes int
+		ch    change.Change
+	}{
+		{"policy_change", 2, change.PolicyChange()},
+		{"full_update", 2, change.FullUpdate()},
+		// Clients ignore an empty Peers list, so an isolating full update
+		// relies on the removal entirely.
+		{"full_update_isolated", 1, change.FullUpdate()},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			testData, cleanup := setupBatcherWithTestData(t, NewBatcherAndMapper, 1, tt.nodes, normalBufferSize)
+			defer cleanup()
+
+			gone := tailcfg.NodeID(999)
+
+			mc := newMockNodeConnection(testData.Nodes[0].n.ID)
+			mc.peers.Store(gone, struct{}{})
+
+			for i := range testData.Nodes[1:] {
+				mc.peers.Store(testData.Nodes[i+1].n.ID.NodeID(), struct{}{})
+			}
+
+			require.NoError(t, handleNodeChange(mc, testData.Batcher.mapper, tt.ch))
+
+			sent := mc.getSent()
+			require.Len(t, sent, 2)
+
+			assert.Equal(t, []tailcfg.NodeID{gone}, sent[0].PeersRemoved)
+			_, ok := netmap.MutationsFromMapResponse(sent[0], time.Time{})
+			assert.True(t, ok, "removal must be applicable as a delta: %+v", sent[0])
+
+			assert.Empty(t, sent[1].PeersRemoved, "full-set response must not carry removals")
+
+			_, tracked := mc.peers.Load(gone)
+			assert.False(t, tracked, "removed peer must leave lastSentPeers")
+		})
+	}
+}
+
+// TestHandleNodeChangeRetryAfterRemoval checks that a change retried after
+// its removal was delivered but its content was not does not repeat the
+// removal.
+func TestHandleNodeChangeRetryAfterRemoval(t *testing.T) {
+	testData, cleanup := setupBatcherWithTestData(t, NewBatcherAndMapper, 1, 2, normalBufferSize)
+	defer cleanup()
+
+	gone := tailcfg.NodeID(999)
+
+	mc := newMockNodeConnection(testData.Nodes[0].n.ID)
+	mc.peers.Store(gone, struct{}{})
+
+	var sends int
+
+	mc.sendFn = func(resp *tailcfg.MapResponse) error {
+		sends++
+		if sends == 2 {
+			return errNoReadyConnections
+		}
+
+		mc.sent = append(mc.sent, resp)
+
+		return nil
+	}
+
+	err := handleNodeChange(mc, testData.Batcher.mapper, change.PolicyChange())
+	require.ErrorIs(t, err, errNoReadyConnections)
+	require.NoError(t, handleNodeChange(mc, testData.Batcher.mapper, change.PolicyChange()))
+
+	sent := mc.getSent()
+	require.Len(t, sent, 2, "removal once, then the retried content")
+	assert.Equal(t, []tailcfg.NodeID{gone}, sent[0].PeersRemoved)
+	assert.Empty(t, sent[1].PeersRemoved)
+	assert.NotEmpty(t, sent[1].PacketFilters)
+}

hscontrol/mapper/mapper.go

@@ -304,8 +304,8 @@ func (m *mapper) selfMapResponse(
 }
 
 // policyChangeResponse creates a [tailcfg.MapResponse] for policy changes.
-// It sends:
-//   - PeersRemoved for peers that are no longer visible after the policy change
+// Peers no longer visible after the change are sent separately, see
+// [handleNodeChange]. It sends:
 //   - PeersChanged for remaining peers (their AllowedIPs may have changed due to policy)
 //   - Updated PacketFilters
 //   - Updated SSHPolicy (SSH rules may reference users/groups that changed)
@@ -325,7 +325,6 @@ func (m *mapper) selfMapResponse(
 func (m *mapper) policyChangeResponse(
 	nodeID types.NodeID,
 	capVer tailcfg.CapabilityVersion,
-	removedPeers []tailcfg.NodeID,
 	currentPeers views.Slice[types.NodeView],
 	includeSelf bool,
 ) (*tailcfg.MapResponse, error) {
@@ -340,16 +339,6 @@ func (m *mapper) policyChangeResponse(
 		builder = builder.WithSelfNode()
 	}
 
-	if len(removedPeers) > 0 {
-		// Convert [tailcfg.NodeID] to [types.NodeID] for [MapResponseBuilder.WithPeersRemoved]
-		removedIDs := make([]types.NodeID, len(removedPeers))
-		for i, id := range removedPeers {
-			removedIDs[i] = types.NodeID(id) //nolint:gosec // NodeID types are equivalent
-		}
-
-		builder.WithPeersRemoved(removedIDs...)
-	}
-
 	// Send remaining peers in PeersChanged - their AllowedIPs may have
 	// changed due to the policy update (e.g., different routes allowed).
 	// Cross-user peers must also carry their user profile, otherwise the

hscontrol/mapper/mapper_test.go

@@ -872,11 +872,13 @@ func TestBackfillNodeIPsReachesBackfilledNode(t *testing.T) {
 	var self *tailcfg.Node
 
 	for _, ch := range change.FilterForNode(target, cs) {
-		resp, err := generateMapResponse(nc, m, ch)
+		resps, err := generateMapResponse(nc, m, ch)
 		require.NoError(t, err)
 
-		if resp != nil && resp.Node != nil {
-			self = resp.Node
+		for _, resp := range resps {
+			if resp.Node != nil {
+				self = resp.Node
+			}
 		}
 	}
 
@@ -963,16 +965,14 @@ func TestFailedExpiryOfPrimaryAnnouncesBackup(t *testing.T) {
 	var backupRoutes []netip.Prefix
 
 	for _, ch := range change.FilterForNode(client, []change.Change{c}) {
-		resp, err := generateMapResponse(nc, m, ch)
+		resps, err := generateMapResponse(nc, m, ch)
 		require.NoError(t, err)
 
-		if resp == nil {
-			continue
-		}
-
-		for _, p := range append(resp.Peers, resp.PeersChanged...) {
-			if p.ID == backup.NodeID() {
-				backupRoutes = p.PrimaryRoutes
+		for _, resp := range resps {
+			for _, p := range append(resp.Peers, resp.PeersChanged...) {
+				if p.ID == backup.NodeID() {
+					backupRoutes = p.PrimaryRoutes
+				}
 			}
 		}
 	}

hscontrol/servertest/issues_test.go

@@ -1000,6 +1000,9 @@ func TestIssuesIdentity(t *testing.T) {
 // SSHPolicy, the removal forces a full netmap rebuild, which never tells IPN
 // bus watchers opted out of full netmaps (the Android app since 1.100) that
 // the peer is gone, so they keep showing it.
+//
+// TODO(kradalby): with the tailscale/tailscale#15660 compat gone, removals
+// ride full rebuilds; assert they reach the netmap instead.
 func TestPeerRemovedAsDelta(t *testing.T) {
 	t.Parallel()