From cfd845cb53efb2f953ff30c7d2ee70329b74f1ce Mon Sep 17 00:00:00 2001 From: Igor Serganov Date: Fri, 24 Jul 2026 14:15:54 -0700 Subject: [PATCH] poll: do not cancel ephemeral GC until Connect succeeds With node.ephemeral.inactivity_timeout set, ephemeral nodes are usually deleted after they go offline, but under reconnect churn some departed nodes stayed in the node list as disconnected indefinitely until removed manually or until Headscale restarted. Ephemeral cleanup is timer-based via EphemeralGarbageCollector, not a periodic LastSeen scan. serveLongPoll cancelled any pending GC timer at the very start of a long-poll attempt and only rescheduled on a clean disconnect after Connect. If a reconnect cancelled the timer and then failed before Connect (for example an UpdateNodeFromMapRequest error), the deferred cleanup saw connectGen == 0 and returned without Schedule. The node remained offline with no deletion timer and no reconciler to recover it. Cancel the ephemeral GC timer only after a successful Connect, so a failed reconnect leaves an already-armed inactivity timer intact. Successful reconnects still cancel GC once the node is online, and a later disconnect reschedules as before. Add TestFailedReconnectDoesNotCancelEphemeralGC to lock in the ordering, plus IsScheduled and DeleteNodeFromStoreForTest helpers for the test. Fixes #3382 Co-authored-by: Cursor --- hscontrol/db/node.go | 10 +++++++ hscontrol/poll.go | 15 +++++----- hscontrol/poll_test.go | 62 ++++++++++++++++++++++++++++++++++++++++ hscontrol/state/state.go | 7 +++++ 4 files changed, 86 insertions(+), 8 deletions(-) diff --git a/hscontrol/db/node.go b/hscontrol/db/node.go index 002bde59..c983c841 100644 --- a/hscontrol/db/node.go +++ b/hscontrol/db/node.go @@ -498,6 +498,16 @@ func (e *EphemeralGarbageCollector) Cancel(nodeID types.NodeID) { } } +// IsScheduled reports whether a deletion timer is currently armed for nodeID. +func (e *EphemeralGarbageCollector) IsScheduled(nodeID types.NodeID) bool { + e.mu.Lock() + defer e.mu.Unlock() + + _, ok := e.toBeDeleted[nodeID] + + return ok +} + // Start starts the garbage collector. func (e *EphemeralGarbageCollector) Start() { for { diff --git a/hscontrol/poll.go b/hscontrol/poll.go index 35657026..656a2dad 100644 --- a/hscontrol/poll.go +++ b/hscontrol/poll.go @@ -96,12 +96,6 @@ func (m *mapSession) stopFromBatcher() { } } -func (m *mapSession) beforeServeLongPoll() { - if m.node.IsEphemeral() { - m.h.ephemeralGC.Cancel(m.node.ID) - } -} - // afterServeLongPoll is called when a long-polling session ends and the node // is disconnected. func (m *mapSession) afterServeLongPoll() { @@ -144,8 +138,6 @@ func (m *mapSession) serve() { // //nolint:gocyclo func (m *mapSession) serveLongPoll() { - m.beforeServeLongPoll() - m.log.Trace().Caller().Msg("long poll session started") // connectGen is set by [state.State.Connect] below and captured by the deferred cleanup closure. @@ -248,6 +240,13 @@ func (m *mapSession) serveLongPoll() { connectChanges, connectGen = m.h.state.Connect(m.node.ID) + // Cancel ephemeral GC only after Connect succeeds. Cancelling at the start + // of serveLongPoll left departed nodes without a deletion timer when a + // reconnect attempt failed before Connect (issue #3382). + if m.node.IsEphemeral() { + m.h.ephemeralGC.Cancel(m.node.ID) + } + m.log.Info().Caller().Str(zf.Chan, fmt.Sprintf("%p", m.ch)).Msg("node has connected") // TODO(kradalby): Redo the comments here diff --git a/hscontrol/poll_test.go b/hscontrol/poll_test.go index 71ce08d9..ae936ec2 100644 --- a/hscontrol/poll_test.go +++ b/hscontrol/poll_test.go @@ -15,6 +15,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "tailscale.com/tailcfg" + "tailscale.com/types/key" ) type delayedSuccessResponseWriter struct { @@ -216,6 +217,67 @@ func TestServeLongPollWritesErrorWhenInitialMapFails(t *testing.T) { "serveLongPoll must write an HTTP error response when the initial map cannot be built, not an empty 200") } +// TestFailedReconnectDoesNotCancelEphemeralGC proves that a +// long-poll reconnect attempt which fails before [state.State.Connect] must +// not cancel a previously armed ephemeral GC timer. Cancelling at the start of +// [mapSession.serveLongPoll] left departed ephemeral nodes stuck offline with +// no deletion scheduled (https://github.com/juanfont/headscale/issues/3382). +func TestFailedReconnectDoesNotCancelEphemeralGC(t *testing.T) { + t.Parallel() + + app := createTestApp(t) + app.StartEphemeralGCForTest(t) + + user := app.state.CreateUserForTest("eph-gc-cancel-user") + pak, err := app.state.CreatePreAuthKey(user.TypedID(), false, true, nil, nil) + require.NoError(t, err) + + machineKey := key.NewMachine() + 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-cancel-node", + }, + Expiry: time.Now().Add(24 * time.Hour), + }, machineKey.Public()) + require.NoError(t, err) + + nodeView, ok := app.state.GetNodeByNodeKey(nodeKey.Public()) + require.True(t, ok) + require.True(t, nodeView.IsEphemeral(), "node must be ephemeral so Cancel would arm on long-poll") + + node := nodeView.AsStruct() + + // Arm a long-lived deletion timer — the state after a normal disconnect + // has called afterServeLongPoll. A long expiry avoids racing the + // fail-before-Connect path below. + app.ephemeralGC.Schedule(node.ID, time.Hour) + require.True(t, app.ephemeralGC.IsScheduled(node.ID), "test sanity: GC timer must be armed") + + // Drop the node from the NodeStore so UpdateNodeFromMapRequest fails before + // Connect, while the session still carries an ephemeral AuthKey (so the + // old Cancel-on-entry path would clear the timer). + app.state.DeleteNodeFromStoreForTest(node.ID) + + writer := &recordingResponseWriter{} + session := app.newMapSession(context.Background(), tailcfg.MapRequest{ + Stream: true, + Version: tailcfg.CapabilityVersion(100), + }, writer, node) + + session.serveLongPoll() + + assert.GreaterOrEqual(t, writer.statusCode(), http.StatusInternalServerError, + "failed reconnect must write an HTTP error before Connect") + assert.True(t, app.ephemeralGC.IsScheduled(node.ID), + "failed reconnect must not cancel the ephemeral GC timer (issue #3382)") +} + // TestGitHubIssue3129_TransientlyBlockedWriteDoesNotLeaveLiveStaleSession // tests the scenario reported in // https://github.com/juanfont/headscale/issues/3129. diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 88a38887..5b34c5d6 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1458,6 +1458,13 @@ func (s *State) PutNodeInStoreForTest(node types.Node) types.NodeView { return s.nodeStore.PutNode(node) } +// DeleteNodeFromStoreForTest removes a node from the in-memory [NodeStore] +// without touching the database. Used to force [State.UpdateNodeFromMapRequest] +// failures in poll-session tests while keeping the DB row intact for later restore. +func (s *State) DeleteNodeFromStoreForTest(id types.NodeID) { + s.nodeStore.DeleteNode(id) +} + // CreateRegisteredNodeForTest creates a test node with allocated IPs. This is a convenience wrapper around the database layer. func (s *State) CreateRegisteredNodeForTest(user *types.User, hostname ...string) *types.Node { return s.db.CreateRegisteredNodeForTest(user, hostname...)