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...)