diff --git a/hscontrol/mapper/batch_coalesce_test.go b/hscontrol/mapper/batch_coalesce_test.go index c54214b26..9c79911bd 100644 --- a/hscontrol/mapper/batch_coalesce_test.go +++ b/hscontrol/mapper/batch_coalesce_test.go @@ -125,7 +125,7 @@ func TestAppendPendingBoundsFoldedPatches(t *testing.T) { if i%2 == 0 { nc.appendPending(change.NodeOnline(2)) } else { - nc.appendPending(change.NodeOffline(2)) + nc.appendPending(change.NodeOffline(2, time.Time{})) } } diff --git a/hscontrol/mapper/batcher_concurrency_test.go b/hscontrol/mapper/batcher_concurrency_test.go index 066d1ac43..0856a315f 100644 --- a/hscontrol/mapper/batcher_concurrency_test.go +++ b/hscontrol/mapper/batcher_concurrency_test.go @@ -287,7 +287,7 @@ func TestAddToBatchDifferential(t *testing.T) { change.SelfUpdate(88), ping, change.NodeOnline(2), }}, {"repeated_kinds", []change.Change{ - change.NodeOnline(2), change.NodeOffline(2), change.NodeOnline(2), + change.NodeOnline(2), change.NodeOffline(2, time.Time{}), change.NodeOnline(2), change.PeersChanged("several peers", 3, 1), }}, {"deletion", []change.Change{ @@ -359,7 +359,7 @@ func TestAddToBatchRecipientStorage(t *testing.T) { lb := setupLightweightBatcher(t, 2, 1) t.Cleanup(lb.cleanup) - incoming := []change.Change{change.NodeOnline(1), change.NodeOffline(1)} + incoming := []change.Change{change.NodeOnline(1), change.NodeOffline(1, time.Time{})} impl.add(lb.b, incoming...) incoming[0] = change.FullUpdate() @@ -370,7 +370,7 @@ func TestAddToBatchRecipientStorage(t *testing.T) { assert.Equal( t, - []change.Change{change.NodeOnline(1), change.NodeOffline(1)}, + []change.Change{change.NodeOnline(1), change.NodeOffline(1, time.Time{})}, getPendingForNode(lb.b, 2), "recipients must own their pending slices", ) diff --git a/hscontrol/mapper/batcher_test.go b/hscontrol/mapper/batcher_test.go index d629be7b5..6c4bfcc87 100644 --- a/hscontrol/mapper/batcher_test.go +++ b/hscontrol/mapper/batcher_test.go @@ -94,7 +94,7 @@ func (t *testBatcherWrapper) RemoveNode(id types.NodeID, c chan<- *tailcfg.MapRe // Do this BEFORE removing from batcher so the change can be processed node, ok := t.state.GetNodeByID(id) if ok { - t.AddWork(change.NodeOffline(node.ID())) + t.AddWork(change.NodeOffline(node.ID(), time.Time{})) } // Finally remove from the real batcher diff --git a/hscontrol/mapper/initialmap_phantom_test.go b/hscontrol/mapper/initialmap_phantom_test.go index ec81b79da..b6dd1a0e6 100644 --- a/hscontrol/mapper/initialmap_phantom_test.go +++ b/hscontrol/mapper/initialmap_phantom_test.go @@ -186,7 +186,7 @@ func TestHeldChangesKeepOrderWhenStreamTurnsReady(t *testing.T) { name: "online_then_offline", bundle: func(p, _ types.NodeID) []change.Change { return []change.Change{ - change.NodeOnline(p), change.DERPMap(), change.NodeOffline(p), + change.NodeOnline(p), change.DERPMap(), change.NodeOffline(p, time.Time{}), } }, want: func(p, _ types.NodeID) []string { diff --git a/hscontrol/mapper/mapper_test.go b/hscontrol/mapper/mapper_test.go index 603ada442..0c37ba133 100644 --- a/hscontrol/mapper/mapper_test.go +++ b/hscontrol/mapper/mapper_test.go @@ -921,7 +921,7 @@ func TestMapResponseNeverContainsSelfAsPeer(t *testing.T) { "policy change": change.PolicyChange(), "self added": change.NodeAdded(recipient), "self online": change.NodeOnline(recipient), - "self offline": change.NodeOffline(recipient), + "self offline": change.NodeOffline(recipient, time.Time{}), // A batch naming every node, the recipient included. // change.PeersChanged carries no OriginNode, so the // self-update short circuit in buildFromChange never fires diff --git a/hscontrol/servertest/client.go b/hscontrol/servertest/client.go index 1f45f9f1a..53444d69d 100644 --- a/hscontrol/servertest/client.go +++ b/hscontrol/servertest/client.go @@ -70,9 +70,10 @@ type TestClient struct { // streams and streamMaps: see [TestClient.Stream]. streams, streamMaps int - // deltaUpdates and deltaRemoved: see [WithDeltaUpdates]. + // deltaUpdates, deltaRemoved and deltaSeen: see [WithDeltaUpdates]. deltaUpdates bool deltaRemoved []tailcfg.NodeID + deltaSeen []time.Time // registers: see [TestClient.RegisterAttempts]. registers []testcapture.RegisterAttempt @@ -482,8 +483,8 @@ func (c *TestClient) UpdateFullNetmap(nm *netmap.NetworkMap) { // deltaRecorder is the [controlclient.NetmapUpdater] of a [WithDeltaUpdates] // client. Like tailscaled it takes packet filter and user profile updates // narrowly, which lets [controlclient.Direct] try the delta path. It -// records removals and declines the mutations, so the full netmap is -// still delivered and the client's peer state stays exact. +// records removals and last seen times and declines the mutations, so the +// full netmap is still delivered and the client's peer state stays exact. type deltaRecorder struct{ *TestClient } func (d deltaRecorder) UpdatePacketFilter(views.Slice[tailcfg.FilterRule], []filter.Match) bool { @@ -499,14 +500,26 @@ func (d deltaRecorder) UpdateNetmapDelta(muts []netmap.NodeMutation) bool { defer d.mu.Unlock() for _, m := range muts { - if _, ok := m.(netmap.NodeMutationRemove); ok { + switch m := m.(type) { + case netmap.NodeMutationRemove: d.deltaRemoved = append(d.deltaRemoved, m.NodeIDBeingMutated()) + case netmap.NodeMutationLastSeen: + d.deltaSeen = append(d.deltaSeen, m.LastSeen) } } return false } +// DeltaLastSeen returns the last seen times that arrived in a response the +// client could apply incrementally; see [WithDeltaUpdates]. +func (c *TestClient) DeltaLastSeen() []time.Time { + c.mu.RLock() + defer c.mu.RUnlock() + + return slices.Clone(c.deltaSeen) +} + // DeltaRemovedPeers returns the peers whose removal arrived in a response // the client could apply incrementally; see [WithDeltaUpdates]. func (c *TestClient) DeltaRemovedPeers() []tailcfg.NodeID { diff --git a/hscontrol/servertest/lifecycle_compat_test.go b/hscontrol/servertest/lifecycle_compat_test.go index f4621a09b..b22ef322f 100644 --- a/hscontrol/servertest/lifecycle_compat_test.go +++ b/hscontrol/servertest/lifecycle_compat_test.go @@ -516,6 +516,15 @@ func peerOnline(p tailcfg.NodeView, missing string) string { return strconv.FormatBool(p.Online().Get()) } +// peerSeen compares presence only: SaaS sends a coarser time than headscale. +func peerSeen(p tailcfg.NodeView, missing string) string { + if missing != "" { + return missing + } + + return strconv.FormatBool(p.LastSeen().Valid()) +} + // saasRemoved reports whether the peer, present before the step or once a // frame named it, was then removed or left out of a full peer list. func saasRemoved(before *netmap.NetworkMap, frames []testcapture.Frame, id tailcfg.NodeID) string { diff --git a/hscontrol/servertest/lifecycle_test.go b/hscontrol/servertest/lifecycle_test.go index 260da384e..409fc9c23 100644 --- a/hscontrol/servertest/lifecycle_test.go +++ b/hscontrol/servertest/lifecycle_test.go @@ -917,6 +917,43 @@ func describeNetmap(c *servertest.TestClient) string { return out.String() } +// TestPeerLastSeenPatches takes a node down and up. As on Tailscale SaaS, its +// peer gets a last seen time with the offline patch and none with the online +// one. The recorder runs before the netmap lands, so a netmap settles both. +func TestPeerLastSeenPatches(t *testing.T) { + t.Parallel() + + srv := servertest.NewServer(t) + user := srv.CreateUser(t, "seen-user") + + watcher := servertest.NewClient(t, srv, "seen-watcher", + servertest.WithUser(user), servertest.WithDeltaUpdates()) + node := servertest.NewClient(t, srv, "seen-node", servertest.WithUser(user)) + + peerOnline := func(want bool) func(*netmap.NetworkMap) bool { + return func(nm *netmap.NetworkMap) bool { + return len(nm.Peers) == 1 && nm.Peers[0].Online().Valid() && nm.Peers[0].Online().Get() == want + } + } + + // A new peer can first appear offline, which must not pass for going down. + watcher.WaitForCondition(t, "peer online", 15*time.Second, peerOnline(true)) + + down := time.Now() + + node.Disconnect(t) + watcher.WaitForCondition(t, "peer offline", 30*time.Second, peerOnline(false)) + + seen := watcher.DeltaLastSeen() + require.Len(t, seen, 1, "offline patch carried no last seen time") + assert.WithinRange(t, seen[0], down, time.Now()) + + node.Reconnect(t) + watcher.WaitForCondition(t, "peer online", 15*time.Second, peerOnline(true)) + + assert.Len(t, watcher.DeltaLastSeen(), 1, "online patch carried a last seen time") +} + func TestServerStateCleanup(t *testing.T) { var srv *servertest.TestServer diff --git a/hscontrol/servertest/mapstream_compat_test.go b/hscontrol/servertest/mapstream_compat_test.go index e593300e7..c05e10464 100644 --- a/hscontrol/servertest/mapstream_compat_test.go +++ b/hscontrol/servertest/mapstream_compat_test.go @@ -743,9 +743,9 @@ func deliveryChecks(d *deliveryStep) []stepCheck { return checks } -// subjectChecks compares how v sees x: online, its DERP home, endpoints, -// name, routes, whether its disco key changed during the step and whether -// its endpoints were cleared on the way. +// subjectChecks compares how v sees x: online, last seen while offline, its +// DERP home, endpoints, name, routes, whether its disco key changed during +// the step and whether its endpoints were cleared on the way. func (d *deliveryStep) subjectChecks(x, v string) []stepCheck { r := d.r saasID := r.c.NodeIDs[x] @@ -773,7 +773,7 @@ func (d *deliveryStep) subjectChecks(x, v string) []stepCheck { hsBefore = peerDisco(d.hsBefore[v], id) } - return []stepCheck{ + checks := []stepCheck{ {x + "/online-" + v, peerOnline(saasPeer, saasMissing), func() string { return peerOnline(hsPeer()) }}, {x + "/derp-" + v, peerDERP(saasPeer, saasMissing), func() string { return peerDERP(hsPeer()) }}, {x + "/endpoints-" + v, peerEndpoints(saasPeer, saasMissing), func() string { return peerEndpoints(hsPeer()) }}, @@ -800,6 +800,14 @@ func (d *deliveryStep) subjectChecks(x, v string) []stepCheck { return hsEndpointsCleared(r.clients[v].History()[d.updates[v]:], id) }}, } + + // A client can keep an old last-seen time through an online patch, so only + // an offline peer's says anything. + if saasMissing == "" && !saasPeer.Online().Get() { + checks = append(checks, stepCheck{x + "/seen-" + v, peerSeen(saasPeer, saasMissing), func() string { return peerSeen(hsPeer()) }}) + } + + return checks } // topologyAddrNames names each SaaS address in c's topology by alias, so a diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 138b9eee8..c68202067 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1046,7 +1046,7 @@ func (s *State) Disconnect(id types.NodeID, epoch uint64) ([]change.Change, erro // An ordinary node going offline only needs the lightweight offline // patch. Subnet routers, relay targets, and via targets change what // peers compute, so they additionally force a peer recompute. - cs := []change.Change{change.NodeOffline(node.ID())} + cs := []change.Change{change.NodeOffline(node.ID(), node.LastSeen().Get())} if s.polMan.NodeNeedsPeerRecompute(node) { cs = append(cs, change.PolicyChange()) } diff --git a/hscontrol/types/change/change.go b/hscontrol/types/change/change.go index 97bbe55c3..11c22d580 100644 --- a/hscontrol/types/change/change.go +++ b/hscontrol/types/change/change.go @@ -530,14 +530,17 @@ func NodeOnline(nodeID types.NodeID) Change { } } -// NodeOffline creates a patch response for a node going offline. -func NodeOffline(nodeID types.NodeID) Change { +// NodeOffline creates a patch response for a node going offline. As on +// Tailscale SaaS it says when, so peers need no full map to show it. SaaS +// sends a coarser time; the exact one here is deliberate. +func NodeOffline(nodeID types.NodeID, lastSeen time.Time) Change { return Change{ Reason: "node offline", PeerPatches: []*tailcfg.PeerChange{ { - NodeID: nodeID.NodeID(), - Online: new(false), + NodeID: nodeID.NodeID(), + Online: new(false), + LastSeen: &lastSeen, }, }, } diff --git a/hscontrol/types/change/change_test.go b/hscontrol/types/change/change_test.go index e5c99de1c..596695c05 100644 --- a/hscontrol/types/change/change_test.go +++ b/hscontrol/types/change/change_test.go @@ -451,10 +451,10 @@ func TestDedupePolicyChanges(t *testing.T) { { name: "peer patches survive between collapsed policy changes", changes: []Change{ - NodeOnline(1), PolicyChange(), NodeOnline(2), PolicyChange(), NodeOffline(3), + NodeOnline(1), PolicyChange(), NodeOnline(2), PolicyChange(), NodeOffline(3, time.Time{}), }, want: []Change{ - NodeOnline(1), PolicyChange(), NodeOnline(2), NodeOffline(3), + NodeOnline(1), PolicyChange(), NodeOnline(2), NodeOffline(3, time.Time{}), }, }, { @@ -514,8 +514,8 @@ func TestCoalescePeerDeltas(t *testing.T) { }, { name: "patches for peers sent whole are dropped, others kept in order", - changes: []Change{NodeOnline(4), NodeAdded(2), NodeOffline(4), NodeOnline(2)}, - want: []Change{merged([]types.NodeID{2}, patch(NodeOnline(4)), patch(NodeOffline(4)))}, + changes: []Change{NodeOnline(4), NodeAdded(2), NodeOffline(4, time.Time{}), NodeOnline(2)}, + want: []Change{merged([]types.NodeID{2}, patch(NodeOnline(4)), patch(NodeOffline(4, time.Time{})))}, }, { name: "runs do not cross other changes", @@ -940,13 +940,16 @@ func TestNodeOnlineOfflineForSubnetRouter(t *testing.T) { view := router.View() require.True(t, view.IsSubnetRouter(), "test node must be a subnet router") + seen := time.Date(2026, 1, 2, 3, 4, 5, 123456789, time.UTC) + tests := []struct { name string got Change wantOnline bool + wantSeen *time.Time }{ {name: "online", got: NodeOnline(view.ID()), wantOnline: true}, - {name: "offline", got: NodeOffline(view.ID()), wantOnline: false}, + {name: "offline", got: NodeOffline(view.ID(), seen), wantOnline: false, wantSeen: &seen}, } for _, tt := range tests { @@ -965,6 +968,7 @@ func TestNodeOnlineOfflineForSubnetRouter(t *testing.T) { require.NotNil(t, patch.Online) assert.Equal(t, tt.wantOnline, *patch.Online) + assert.Equal(t, tt.wantSeen, patch.LastSeen) }) } } diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 69dcdcc14..b42ea6573 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -1487,9 +1487,8 @@ func (nv NodeView) TailNode( Expired: nv.IsExpired(), } - // Set LastSeen only for offline nodes to avoid confusing Tailscale clients - // during rapid reconnection cycles. Online nodes should not have LastSeen set - // as this can make clients interpret them as "not online" despite Online=true. + // Only an offline entry has LastSeen, as on Tailscale SaaS. An online peer + // can still show one on a client, which keeps it through an online patch. if nv.LastSeen().Valid() && nv.IsOnline().Valid() && !nv.IsOnline().Get() { lastSeen := nv.LastSeen().Get() tNode.LastSeen = &lastSeen