Skip to content

Commit 2ab3b39

Browse files
fix(hub,cli): four hub-restart-cascade cleanup bugs (#913 #914 #916 #919) (#923)
* fix(hub,cli): four hub-restart-cascade cleanup bugs (#913 #914 #916 #919) These four contained bugs were uncovered by a 2026-06-15 hub-restart incident where `hapi-restart-hub` SIGTERMed 23 cursor ACP sessions. Each fix lands independently of the architectural #915 (hub-restart cascade-archive) and the hypothesis-pending #917 (reopen creates dead session); audit-trail correctness and idempotency wins stand on their own. Fresh ACP sessions could be SIGTERMed during the async `update-metadata` ACK round-trip, stranding the on-disk ACP store with no DB handle. Add `ApiSessionClient.flushMetadata()` and await it after `onSessionFoundWithProtocol` on the fresh-session branch. Resume-path pre-registration (PR #834) is unchanged. Hub-restart-cascade SIGTERMs went through the same path as web-UI Archive clicks, both writing archiveReason='User terminated'. New default is 'Hub restart'; the KillSession RPC handler (the authoritative user-archive signal) now explicitly stamps 'User terminated' before cleanupAndExit. SIGINT (local-terminal Ctrl-C) keeps the 'User terminated' label too. `rpcGateway.killSession` threw a generic Error when no target socket was registered, and the archive route surfaced that as 500. Add typed `RpcTargetMissingError`, narrow on it in `syncEngine.archiveSession`, fall back to a hub-side `markSessionArchivedFromHub` write so lifecycleState still flips to 'archived'. Drop the requireActive guard on the route and 2xx-noop for already-archived rows. without refresh, producing forever-409 on rename/reopen until an unrelated event triggered a cache refresh. `renameSession`, `clearSessionArchiveMetadata`, `restoreSessionArchiveMetadata` now retry-with-refresh (5 attempts, then throw) mirroring the existing good pattern in `mergeSessions`. Refs #913 Refs #914 Refs #916 Refs #919 AI disclosure: implementation by Claude Sonnet 4.5 (Cursor agent peer) under operator supervision. Issue triage by a sibling discovery agent. Per CONTRIBUTING.md AI-assisted contributions policy. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): runner-spawned children use 'Stopped by runner' as default archive reason Addresses bot review of #923: with the #914 default-archiveReason flip to 'Hub restart', runner-driven SIGTERM paths (`hapi runner stop-session`, webhook-timeout cleanup at run.ts:587, orphan-cleanup at run.ts:267) all mislabel as 'Hub restart' which is also inaccurate audit-trail noise. Smallest defensible change: parameterise the lifecycle default via HAPI_DEFAULT_ARCHIVE_REASON env, and have the runner set 'Stopped by runner' on spawn. Terminal-launched sessions (no runner parent, no env var) still default to 'Hub restart' since hub-restart cascade documented at #915 is the most plausible SIGTERM source for those. Explicit overrides via setArchiveReason (KillSession RPC, SIGINT Ctrl-C, markCrash uncaught exception) still win. Two new unit tests cover the env-var default and the override precedence. Refs #914. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(hub): markSessionArchivedFromHub surfaces persistence failures as 5xx Addresses second-round bot review of #923 (Major): `markSessionArchivedFromHub` silently returned on DB write errors and on exhausted version-retry attempts, which would let `/archive` claim 200 OK while the row stayed unarchived. That regresses the #916 acceptance criterion that non-RPC errors during archive must still propagate as 5xx. Both fall-through paths now throw, matching the contract of the sibling writers in this file (renameSession, mergeSessions). The sessionModel test suite gains two cases that spy on `store.sessions.updateSessionMetadata` to force `error` and `version-mismatch` shapes and asserts the helper throws. The existing route test at `hub/src/web/routes/sessions.test.ts:1015` already covers the route-level 500 propagation for any error thrown out of `archiveSession`, so no new route test is needed. Imports `spyOn` from `bun:test` to match this test file's runtime (the rest of the hub package uses bun:test, not vitest). Refs #916. Co-authored-by: Cursor <cursoragent@cursor.com> * revert(cli): drop HAPI_DEFAULT_ARCHIVE_REASON env override Reverts `1c8972a3`. Bot review round 3 surfaced that the env-on-spawn approach (the bot's own round-1 suggestion shape) mislabels hub-restart-cascade SIGTERMs against runner-spawned children: systemd killcgroup on `hapi-runner.service` stop sends SIGTERM to all runner-children directly, and those would archive as 'Stopped by runner' instead of 'Hub restart'. The two suggestions are mutually incompatible without adding an IPC channel (stdio: 'ipc' on spawn) so the runner can stamp setArchiveReason via childProcess.send() before SIGTERMing. That is a refactor, not a smallest-defensible change. Going back to the simple shape: SIGTERM default is 'Hub restart' for everyone, runner-internal stop paths share that label. The audit-trail-correctness criterion from the #914 issue is met (SIGTERM no longer falsely labels as 'User terminated'). Finer attribution between cascade vs runner-stop is deferred as a follow-up. Refs #914. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): clean completions get 'Session completed', not 'Hub restart' Addresses bot review round 4 of #923 (Major): every agent runner (runClaude, runCodex, runCursor, runGemini, runKimi, runOpencode) calls setSessionEndReason('completed') on the natural exit path without touching archiveReason. With the SIGTERM default flipped to 'Hub restart', clean completions were now archived as restart cascades. Fix: setSessionEndReason flips archiveReason to 'Session completed' when it transitions to 'completed' AND no caller has already overridden the archive reason. This covers all six agent runners with a single setter change (no per-runner edits). Two new tests cover the natural-completion default and the override precedence (explicit setArchiveReason still wins). Refs #914. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(hub): restore inactive-session guard on /archive except split-brain Addresses post-rebase bot review Major on #923: dropping requireActive entirely let normal inactive non-archived rows (completed stubs, UI Delete/Reopen targets) fall through to archiveSession, which could stamp archivedBy=hub on sessions that were never active. Restore the 409 for inactive rows unless metadata.lifecycleState is still 'running' (hub-restart split-brain cleanup case from #916). Two route tests cover the guard and the exception. Refs #916. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): merge runnerLifecycle tests after upstream rebase Post-rebase fix: Session completed tests referenced makeFakeSession which was renamed to createMockApiSessionWithMetadataCapture when merging upstream hasExplicitSessionEndReason tests with #914 archive reason coverage. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): pass lifecycle object to KillSession handler in Pi runner Upstream #862 (Pi agent) landed after this branch was cut. runPi.ts still registered the legacy bare cleanupAndExit callback, so web Archive for Pi sessions would persist archiveReason: Hub restart instead of User terminated. One-line fix matching the other six agent runners. Refs #914. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent a0259b5 commit 2ab3b39

21 files changed

Lines changed: 1020 additions & 113 deletions

cli/src/agent/runnerLifecycle.test.ts

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,23 @@ function createMockApiSession() {
2323
} as unknown as Parameters<typeof createRunnerLifecycle>[0]['session'];
2424
}
2525

26+
function createMockApiSessionWithMetadataCapture() {
27+
const metadataWrites: Array<Record<string, unknown>> = []
28+
return {
29+
updateMetadata: vi.fn((handler: (m: Record<string, unknown>) => Record<string, unknown>) => {
30+
const next = handler({})
31+
metadataWrites.push(next)
32+
return next
33+
}),
34+
sendSessionDeath: vi.fn(),
35+
flush: vi.fn(async () => {}),
36+
close: vi.fn(async () => {}),
37+
metadataWrites
38+
} as unknown as Parameters<typeof createRunnerLifecycle>[0]['session'] & {
39+
metadataWrites: Array<Record<string, unknown>>
40+
}
41+
}
42+
2643
describe('createRunnerLifecycle', () => {
2744
let lifecycle: RunnerLifecycle;
2845

@@ -85,3 +102,95 @@ describe('createRunnerLifecycle', () => {
85102
});
86103
});
87104
});
105+
106+
// tiann/hapi#914: the runnerLifecycle's default archiveReason is now
107+
// 'Hub restart' (was 'User terminated'). Out-of-band SIGTERM from the
108+
// hub-restart cascade keeps that default. Explicit user actions
109+
// (clicking Archive in the web UI, Ctrl-C in a local terminal,
110+
// uncaught exception) reassign the reason before archive metadata is
111+
// written.
112+
describe('createRunnerLifecycle archiveReason defaults (tiann/hapi#914)', () => {
113+
it('uses Hub restart as the default archiveReason when no override is applied', async () => {
114+
const session = createMockApiSessionWithMetadataCapture()
115+
const lifecycle = createRunnerLifecycle({
116+
session,
117+
logTag: 'test'
118+
})
119+
120+
await lifecycle.cleanup()
121+
122+
expect(session.metadataWrites).toHaveLength(1)
123+
expect(session.metadataWrites[0]).toMatchObject({
124+
lifecycleState: 'archived',
125+
archivedBy: 'cli',
126+
archiveReason: 'Hub restart'
127+
})
128+
})
129+
130+
it('writes the operator-supplied reason when setArchiveReason is called (e.g. KillSession RPC)', async () => {
131+
const session = createMockApiSessionWithMetadataCapture()
132+
const lifecycle = createRunnerLifecycle({
133+
session,
134+
logTag: 'test'
135+
})
136+
137+
lifecycle.setArchiveReason('User terminated')
138+
await lifecycle.cleanup()
139+
140+
expect(session.metadataWrites[0]).toMatchObject({
141+
archiveReason: 'User terminated'
142+
})
143+
})
144+
145+
it('markCrash overrides the default reason to "Session crashed"', async () => {
146+
const session = createMockApiSessionWithMetadataCapture()
147+
const lifecycle = createRunnerLifecycle({
148+
session,
149+
logTag: 'test'
150+
})
151+
152+
lifecycle.markCrash(new Error('boom'))
153+
await lifecycle.cleanup()
154+
155+
expect(session.metadataWrites[0]).toMatchObject({
156+
archiveReason: 'Session crashed'
157+
})
158+
})
159+
160+
// tiann/hapi#914 review round 4: clean agent-loop completions
161+
// (runClaude / runCodex / runCursor / runGemini / runKimi /
162+
// runOpencode all call setSessionEndReason('completed') without
163+
// touching archiveReason) must not be archived as 'Hub restart'.
164+
// The setSessionEndReason setter flips the default when the runner
165+
// transitions to 'completed'.
166+
it('setSessionEndReason("completed") flips the default reason to "Session completed"', async () => {
167+
const session = createMockApiSessionWithMetadataCapture()
168+
const lifecycle = createRunnerLifecycle({
169+
session,
170+
logTag: 'test'
171+
})
172+
173+
lifecycle.setSessionEndReason('completed')
174+
await lifecycle.cleanup()
175+
176+
expect(session.metadataWrites[0]).toMatchObject({
177+
archiveReason: 'Session completed'
178+
})
179+
})
180+
181+
it('an explicit setArchiveReason before setSessionEndReason("completed") still wins', async () => {
182+
const session = createMockApiSessionWithMetadataCapture()
183+
const lifecycle = createRunnerLifecycle({
184+
session,
185+
logTag: 'test'
186+
})
187+
188+
lifecycle.setArchiveReason('User terminated')
189+
lifecycle.setSessionEndReason('completed')
190+
await lifecycle.cleanup()
191+
192+
expect(session.metadataWrites[0]).toMatchObject({
193+
archiveReason: 'User terminated'
194+
})
195+
})
196+
})

cli/src/agent/runnerLifecycle.ts

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,27 @@ export type RunnerLifecycle = {
2424

2525
export function createRunnerLifecycle(options: RunnerLifecycleOptions): RunnerLifecycle {
2626
let exitCode = 0
27-
let archiveReason = 'User terminated'
27+
// tiann/hapi#914: default reason is 'Hub restart' (parent-driven SIGTERM
28+
// is the most common non-user cause). Genuine user actions (clicking
29+
// Archive in the web UI, or Ctrl-C in a local terminal) explicitly
30+
// reassign this via `setArchiveReason` BEFORE `cleanupAndExit` runs:
31+
// - KillSession RPC handler → 'User terminated' (see registerKillSessionHandler)
32+
// - SIGINT handler → 'User terminated' (Ctrl-C in local terminal)
33+
// - uncaughtException/Reject → 'Session crashed' (via markCrash)
34+
//
35+
// Out-of-band SIGTERM (hub-restart cascade, systemd cgroup kill on
36+
// hapi-runner.service stop, `kill <pid>` from the operator) keeps the
37+
// default and is correctly labelled 'Hub restart' on the audit trail.
38+
//
39+
// Runner-internal stop paths (`hapi runner stop-session`, webhook-timeout
40+
// cleanup at run.ts:587, orphan cleanup at run.ts:267) also currently
41+
// hit this default - that is technically inaccurate but follows the
42+
// friction-mode "smallest defensible change" rule for this PR. Finer
43+
// attribution would require an IPC channel (stdio: 'ipc' on spawn) so
44+
// the runner can stamp `setArchiveReason` before SIGTERMing; tracked as
45+
// a follow-up to keep this PR focussed on the user-action lie that
46+
// motivated #914.
47+
let archiveReason = 'Hub restart'
2848
let sessionEndReason: SessionEndReason = 'terminated'
2949
let sessionEndReasonExplicit = false
3050
let cleanupStarted = false
@@ -98,6 +118,18 @@ export function createRunnerLifecycle(options: RunnerLifecycleOptions): RunnerLi
98118
const setSessionEndReason = (reason: SessionEndReason) => {
99119
sessionEndReason = reason
100120
sessionEndReasonExplicit = true
121+
// tiann/hapi#914 review round 4: every agent runner
122+
// (runClaude / runCodex / runCursor / runGemini / runKimi /
123+
// runOpencode) calls setSessionEndReason('completed') before
124+
// cleanupAndExit() on the natural-exit path without setting an
125+
// archive reason. With the SIGTERM-driven default of 'Hub restart',
126+
// clean completions would otherwise be audit-trailed as restart
127+
// cascades. Flip the default to 'Session completed' when the end
128+
// reason transitions to 'completed' AND no caller has already
129+
// overridden the archive reason.
130+
if (reason === 'completed' && archiveReason === 'Hub restart') {
131+
archiveReason = 'Session completed'
132+
}
101133
}
102134

103135
const hasExplicitSessionEndReason = () => sessionEndReasonExplicit
@@ -110,11 +142,19 @@ export function createRunnerLifecycle(options: RunnerLifecycleOptions): RunnerLi
110142
}
111143

112144
const registerProcessHandlers = () => {
145+
// tiann/hapi#914: SIGTERM is treated as the default reason ('Hub restart')
146+
// because the runner is restarted by systemd as part of hub restart in
147+
// production. If a future code path needs to distinguish "operator
148+
// killed the host process" from "hub restart", it can call
149+
// setArchiveReason() before the runner exits.
113150
process.on('SIGTERM', () => {
114151
void cleanupAndExit()
115152
})
116153

154+
// Ctrl-C in a local terminal is genuine user intent — keep the
155+
// pre-#914 label so the audit trail still shows it.
117156
process.on('SIGINT', () => {
157+
archiveReason = 'User terminated'
118158
void cleanupAndExit()
119159
})
120160

cli/src/api/apiSession.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -748,6 +748,21 @@ export class ApiSessionClient extends EventEmitter {
748748
})
749749
}
750750

751+
/**
752+
* tiann/hapi#913: wait until any pending `update-metadata` writes have
753+
* been acked by the hub (or the timeout elapses). `updateMetadata` is
754+
* fire-and-forget at the call site because it's invoked on the hot path
755+
* for every turn; this helper lets the few callers who actually need
756+
* durability — fresh ACP session-id pre-registration is the canonical
757+
* case — synchronously gate on persistence without changing every
758+
* caller's signature.
759+
*
760+
* Returns true when the lock drained, false when the timeout fired.
761+
*/
762+
async flushMetadata(timeoutMs: number = 5_000): Promise<boolean> {
763+
return await this.drainLock(this.metadataLock, timeoutMs)
764+
}
765+
751766
async flush(options?: { timeoutMs?: number }): Promise<void> {
752767
const deadlineMs = Date.now() + (options?.timeoutMs ?? 5_000)
753768

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
import { describe, expect, it, vi } from 'vitest'
2+
import { RPC_METHODS } from '@hapi/protocol/rpcMethods'
3+
import { registerKillSessionHandler } from './registerKillSessionHandler'
4+
5+
// tiann/hapi#914: the KillSession RPC is the authoritative "user-terminated"
6+
// signal because the hub only sends it when the operator clicks Archive in
7+
// the web UI. Out-of-band SIGTERM (hub-restart cascade, host-level `kill`)
8+
// hits the SIGTERM signal handler in runnerLifecycle, which now keeps the
9+
// default reason 'Hub restart' so the audit trail stays correct.
10+
describe('registerKillSessionHandler (tiann/hapi#914)', () => {
11+
function makeRegistry() {
12+
const handlers = new Map<string, (params?: unknown) => unknown>()
13+
return {
14+
registerHandler: (method: string, handler: (params: unknown) => unknown) => {
15+
handlers.set(method, handler as (params?: unknown) => unknown)
16+
},
17+
handlers
18+
}
19+
}
20+
21+
it('stamps archiveReason=User terminated before triggering cleanupAndExit', async () => {
22+
const registry = makeRegistry()
23+
const lifecycle = {
24+
setArchiveReason: vi.fn(),
25+
cleanupAndExit: vi.fn(async () => {})
26+
}
27+
28+
registerKillSessionHandler(
29+
registry as unknown as Parameters<typeof registerKillSessionHandler>[0],
30+
lifecycle
31+
)
32+
33+
const handler = registry.handlers.get(RPC_METHODS.KillSession)
34+
expect(handler).toBeDefined()
35+
36+
const result = await handler?.()
37+
expect(result).toEqual({ success: true, message: 'Killing hapi CLI process' })
38+
39+
// setArchiveReason MUST be called BEFORE cleanupAndExit so the archive
40+
// metadata write reads the correct reason.
41+
const setReasonOrder = lifecycle.setArchiveReason.mock.invocationCallOrder[0]
42+
const cleanupOrder = lifecycle.cleanupAndExit.mock.invocationCallOrder[0]
43+
expect(setReasonOrder).toBeLessThan(cleanupOrder)
44+
expect(lifecycle.setArchiveReason).toHaveBeenCalledWith('User terminated')
45+
expect(lifecycle.cleanupAndExit).toHaveBeenCalled()
46+
})
47+
48+
it('still works with the legacy `(cleanupAndExit: () => Promise<void>)` call shape', async () => {
49+
// Back-compat: runAgentSession.ts passes a bare closure as the second
50+
// argument instead of a lifecycle object. The handler should not crash
51+
// when setArchiveReason is absent.
52+
const registry = makeRegistry()
53+
const cleanupAndExit = vi.fn(async () => {})
54+
55+
registerKillSessionHandler(
56+
registry as unknown as Parameters<typeof registerKillSessionHandler>[0],
57+
cleanupAndExit
58+
)
59+
60+
const handler = registry.handlers.get(RPC_METHODS.KillSession)
61+
await handler?.()
62+
63+
expect(cleanupAndExit).toHaveBeenCalled()
64+
})
65+
})

cli/src/claude/registerKillSessionHandler.ts

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,18 +11,41 @@ interface KillSessionResponse {
1111
message: string;
1212
}
1313

14+
/**
15+
* tiann/hapi#914: callers can pass either a bare `cleanupAndExit` closure
16+
* (legacy) or an options object that lets the kill-RPC stamp an explicit
17+
* `archiveReason` before the lifecycle teardown runs. The hub only sends
18+
* KillSession when the operator clicked Archive in the UI, so this RPC is
19+
* the authoritative "user-terminated" signal; out-of-band SIGTERM from a
20+
* hub-restart cascade no longer collides with the default archive reason.
21+
*/
22+
export interface KillSessionLifecycle {
23+
cleanupAndExit: () => Promise<void>;
24+
setArchiveReason?: (reason: string) => void;
25+
}
1426

1527
export function registerKillSessionHandler(
1628
rpcHandlerManager: RpcHandlerManager,
17-
killThisHappy: () => Promise<void>
29+
lifecycleOrCleanup: KillSessionLifecycle | (() => Promise<void>)
1830
) {
31+
const lifecycle: KillSessionLifecycle = typeof lifecycleOrCleanup === 'function'
32+
? { cleanupAndExit: lifecycleOrCleanup }
33+
: lifecycleOrCleanup;
34+
1935
rpcHandlerManager.registerHandler<KillSessionRequest, KillSessionResponse>(RPC_METHODS.KillSession, async () => {
2036
logger.debug('Kill session request received');
2137

38+
// tiann/hapi#914: stamp the archive reason from the RPC path so the
39+
// default in `runnerLifecycle.ts` can be reassigned away from
40+
// 'User terminated'. A hub-restart-cascade SIGTERM does NOT go
41+
// through this handler — it hits the SIGTERM signal handler — so
42+
// those archives now stay labelled `'Hub restart'` (the new default).
43+
lifecycle.setArchiveReason?.('User terminated');
44+
2245
// This will start the cleanup process
23-
void killThisHappy();
46+
void lifecycle.cleanupAndExit();
2447

25-
// We should still be able to respond the the client, though they
48+
// We should still be able to respond to the client, though they
2649
// should optimistically assume the session is dead.
2750
return {
2851
success: true,

cli/src/claude/runClaude.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ export async function runClaude(options: StartOptions = {}): Promise<void> {
145145
});
146146

147147
lifecycle.registerProcessHandlers();
148-
registerKillSessionHandler(session.rpcHandlerManager, lifecycle.cleanupAndExit);
148+
registerKillSessionHandler(session.rpcHandlerManager, lifecycle);
149149
registerLocalHandoffHandler(session.rpcHandlerManager, lifecycle);
150150

151151
// Set initial agent state

cli/src/codex/runCodex.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ export async function runCodex(opts: {
9898
});
9999

100100
lifecycle.registerProcessHandlers();
101-
registerKillSessionHandler(session.rpcHandlerManager, lifecycle.cleanupAndExit);
101+
registerKillSessionHandler(session.rpcHandlerManager, lifecycle);
102102
registerLocalHandoffHandler(session.rpcHandlerManager, lifecycle);
103103

104104
const applyCurrentConfigToSession = (options?: { syncModel?: boolean }) => {

0 commit comments

Comments
 (0)