hscontrol: never garbage collect an ephemeral node with a live session

A session arming the GC after a reconnect cancelled it left a stale timer.

Fixes #3535
Signed-off-by: Dan Cunningham <dan@digitaldan.com>

CHANGELOG.md

@@ -148,6 +148,7 @@ clients, and how to run the same setup without Nix.
 - Fix SSH `check` periods and app grants dropping a group that names an unknown user [#3516](https://github.com/juanfont/headscale/pull/3516)
 - Fix policy `tests` and `sshTests` failing for a group that names an unknown user [#3516](https://github.com/juanfont/headscale/pull/3516)
 - Fix an unknown user in `nodeAttrs` rejecting the policy [#3516](https://github.com/juanfont/headscale/pull/3516)
+- Fix an ephemeral node being deleted while online, when it reconnected while its previous session was being marked offline [#3539](https://github.com/juanfont/headscale/pull/3539)
 
 - Fix an exit node or subnet router not seeing its own approved routes until it reconnected, so `tailscale status` did not show it offering an exit node [#3518](https://github.com/juanfont/headscale/pull/3518)
 

hscontrol/app.go

@@ -156,6 +156,20 @@ func NewHeadscale(cfg *types.Config) (*Headscale, error) {
 			return
 		}
 
+		// Schedule and Cancel run outside the session transition, so a
+		// disconnecting session can arm a timer after a reconnect already
+		// cancelled it. The session count is authoritative; a stale timer
+		// is dropped and the next disconnect arms a fresh one. Not Online():
+		// an expired node keeps polling while offline.
+		// ponytail: a Connect between this check and DeleteNode still loses
+		// the node; only reachable after a full inactivity timeout. Needs a
+		// delete-if-idle in State if that edge matters.
+		if node.ActiveSessions() > 0 {
+			log.Debug().Caller().EmbedObject(node).Msg("ephemeral node has a live session, skipping garbage collection")
+
+			return
+		}
+
 		changes, err := app.state.DeleteNode(node)
 		app.Change(changes...)
 

hscontrol/poll_test.go

@@ -342,6 +342,88 @@ func TestFailedReconnectDoesNotCancelEphemeralGC(t *testing.T) {
 		"failed reconnect must not cancel the ephemeral GC timer (issue #3382)")
 }
 
+// TestEphemeralGCDoesNotDeleteReconnectedNode replays, in order, the
+// interleaving of two sessions from
+// https://github.com/juanfont/headscale/issues/3535: the old session takes the
+// node offline, a reconnect brings it back online and cancels the GC before
+// the old session arms its timer. The armed timer is stale; firing it must not
+// delete a node that has a live session.
+func TestEphemeralGCDoesNotDeleteReconnectedNode(t *testing.T) {
+	t.Parallel()
+
+	app := createTestApp(t)
+	app.cfg.Node.Ephemeral.InactivityTimeout = 50 * time.Millisecond
+	app.StartEphemeralGCForTest(t)
+
+	user := app.state.CreateUserForTest("eph-gc-reconnect-user")
+	pak, err := app.state.CreatePreAuthKey(user.TypedID(), false, true, nil, nil)
+	require.NoError(t, err)
+
+	nodeKey := key.NewNode()
+
+	_, err = app.handleRegister(context.Background(), tailcfg.RegisterRequest{
+		Auth: &tailcfg.RegisterResponseAuth{
+			AuthKey: pak.Key,
+		},
+		NodeKey: nodeKey.Public(),
+		Hostinfo: &tailcfg.Hostinfo{
+			Hostname: "eph-gc-reconnect-node",
+		},
+		Expiry: time.Now().Add(24 * time.Hour),
+	}, key.NewMachine().Public())
+	require.NoError(t, err)
+
+	nodeView, ok := app.state.GetNodeByNodeKey(nodeKey.Public())
+	require.True(t, ok)
+	require.True(t, nodeView.IsEphemeral(), "test sanity: node must be ephemeral")
+
+	node := nodeView.AsStruct()
+	session := app.newMapSession(context.Background(), tailcfg.MapRequest{
+		Stream:  true,
+		Version: tailcfg.CapabilityVersion(100),
+	}, &recordingResponseWriter{}, node)
+
+	_, oldGen := app.state.Connect(node.ID)
+
+	// Old session's cleanup releases the last session.
+	offline, err := app.state.Disconnect(node.ID, oldGen)
+	require.NoError(t, err)
+	require.NotEmpty(t, offline, "test sanity: old session must take the node offline")
+
+	// Reconnect: what serveLongPoll does at Connect.
+	_, newGen := app.state.Connect(node.ID)
+	app.ephemeralGC.Cancel(node.ID)
+
+	// Old session's cleanup continues and arms the GC.
+	session.afterServeLongPoll()
+
+	online, ok := app.state.GetNodeByID(node.ID)
+	require.True(t, ok)
+	require.True(t, online.Online(), "test sanity: reconnected node must be online")
+
+	// The GC's delete lands only after a NodeStore batch flush, so watch
+	// well past the inactivity timeout.
+	assert.Never(t, func() bool {
+		_, ok := app.state.GetNodeByID(node.ID)
+
+		return !ok
+	}, 2*time.Second, 10*time.Millisecond,
+		"ephemeral GC must not delete a node with a live session (issue #3535)")
+
+	// Once the reconnected session ends, the node is idle and must still be
+	// collected.
+	_, err = app.state.Disconnect(node.ID, newGen)
+	require.NoError(t, err)
+	session.afterServeLongPoll()
+
+	assert.Eventually(t, func() bool {
+		_, ok := app.state.GetNodeByID(node.ID)
+
+		return !ok
+	}, 5*time.Second, 10*time.Millisecond,
+		"an idle ephemeral node must still be garbage collected")
+}
+
 // TestGitHubIssue3129_TransientlyBlockedWriteDoesNotLeaveLiveStaleSession
 // tests the scenario reported in
 // https://github.com/juanfont/headscale/issues/3129.