From 26d26b350f6f19cc32b7f6ed4c7c1d14608820d7 Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Thu, 27 Aug 2026 19:22:10 +0000 Subject: [PATCH 1/5] fix(hub): verify runner before trusting archiveSession's dead-CLI fallback tiann/hapi#916 treats any RpcTargetMissingError from the session-scoped killSession RPC as proof the CLI is gone and archives the row. That RPC is keyed on whatever session id the CLI last registered under, which goes stale independently of the process itself (SessionCache.mergeSessions rotates the canonical id on resume; the session socket can also just be reconnecting), so a missing target does not prove the process is dead. Fall back to the machine-level stopRunnerSession RPC first, which resolves the child by PID and checks both the requested and confirmed session ids, before trusting archiveSession's fallback path. Fixes tiann/hapi#1705 --- hub/src/sync/syncEngine.ts | 30 +++++ hub/src/sync/syncEngineArchiveSession.test.ts | 113 ++++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 hub/src/sync/syncEngineArchiveSession.test.ts diff --git a/hub/src/sync/syncEngine.ts b/hub/src/sync/syncEngine.ts index baaf403a00..6245bee936 100644 --- a/hub/src/sync/syncEngine.ts +++ b/hub/src/sync/syncEngine.ts @@ -1673,6 +1673,36 @@ export class SyncEngine { await this.rpcGateway.killSession(sessionId) } catch (error) { if (error instanceof RpcTargetMissingError) { + // `killSession` addresses the CLI's own session-scoped socket + // registration (`${sessionId}:KillSession`). That registration + // can go stale independently of the actual runner child + // process — e.g. SessionCache.mergeSessions rotates the + // hub's canonical session id on resume without the CLI + // re-registering under it, or the socket is mid-reconnect — + // so a missing target does NOT by itself prove the process + // is dead. Before trusting that, ask the runner directly via + // the machine-level StopSession RPC, which resolves the + // child by PID and checks both the requested and confirmed + // session ids (see `stopSession` in cli/src/runner/run.ts), + // so it survives the exact id mismatch that just defeated + // `killSession`. Without this check, a live orphaned child + // gets marked `archived` and loses all runner-side + // supervision while continuing to run. + const machineId = this.sessionCache.getSession(sessionId)?.metadata?.machineId + if (machineId) { + let status: 'stopped' | 'already_gone' | 'still_alive' + try { + status = await this.rpcGateway.stopRunnerSession(machineId, sessionId) + } catch { + // Machine itself unreachable; no stronger signal available + // than the original RpcTargetMissingError, so fall back + // to the prior best-effort behavior. + status = 'already_gone' + } + if (status === 'still_alive') { + throw new Error('Session process is still running and could not be stopped') + } + } this.sessionCache.markSessionArchivedFromHub(sessionId, 'Archived from hub (CLI unreachable)') } else { throw error diff --git a/hub/src/sync/syncEngineArchiveSession.test.ts b/hub/src/sync/syncEngineArchiveSession.test.ts new file mode 100644 index 0000000000..7b51ce0024 --- /dev/null +++ b/hub/src/sync/syncEngineArchiveSession.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it, beforeEach } from 'bun:test' +import { Store } from '../store' +import { RpcRegistry } from '../socket/rpcRegistry' +import { SyncEngine } from './syncEngine' +import { RpcTargetMissingError } from './rpcGateway' +import type { SessionCache } from './sessionCache' + +/** + * `archiveSession`'s only kill mechanism is `rpcGateway.killSession`, a + * session-scoped socket RPC keyed on `${sessionId}:KillSession`. That + * registration goes stale independently of the actual runner child process + * — e.g. `SessionCache.mergeSessions` rotates the hub's canonical session id + * on resume without the CLI re-registering under it, or the session socket + * is mid-reconnect — so `RpcTargetMissingError` alone does not prove the + * process is dead. + * + * Before this fix, any `RpcTargetMissingError` was treated as "CLI already + * gone" and the row was unconditionally marked `archived`, silently + * orphaning a still-running child with no runner-side supervision left + * pointed at it. The fix confirms with the runner's machine-level + * `StopSession` RPC (which resolves the child by PID and checks both the + * requested and confirmed session ids) before trusting that the process is + * actually gone. + */ +describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { + let store: Store + let engine: SyncEngine + const NAMESPACE = 'default' + + function cache(): SessionCache { + return (engine as unknown as { sessionCache: SessionCache }).sessionCache + } + + function insertActiveSession(tag: string, machineId?: string): string { + const created = cache().getOrCreateSession( + tag, + { path: '/tmp/proj', host: 'localhost', flavor: 'claude', ...(machineId ? { machineId } : {}) }, + null, + NAMESPACE + ) + cache().markSessionActive(created.id) + return created.id + } + + function setKillSessionMissingTarget(): void { + ;(engine as unknown as { rpcGateway: { killSession: unknown } }).rpcGateway.killSession = + async () => { throw new RpcTargetMissingError('KillSession', 'handler-not-registered') } + } + + beforeEach(() => { + store = new Store(':memory:') + engine = new SyncEngine(store, {} as never, new RpcRegistry(), { broadcast() {} } as never) + }) + + it('does not archive a session the runner confirms is still alive', async () => { + const sessionId = insertActiveSession('sess-still-alive', 'machine-x') + setKillSessionMissingTarget() + ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = + async () => 'still_alive' + + await expect(engine.archiveSession(sessionId)).rejects.toThrow() + + const session = cache().getSession(sessionId) + expect(session?.active).toBe(true) + expect(session?.metadata?.lifecycleState).not.toBe('archived') + }) + + it('archives the session once the runner confirms the process is gone', async () => { + const sessionId = insertActiveSession('sess-confirmed-gone', 'machine-x') + setKillSessionMissingTarget() + let calledWith: [string, string] | undefined + ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = + async (machineId: string, sid: string) => { + calledWith = [machineId, sid] + return 'already_gone' + } + + await engine.archiveSession(sessionId) + + expect(calledWith).toEqual(['machine-x', sessionId]) + const session = cache().getSession(sessionId) + expect(session?.active).toBe(false) + expect(session?.metadata?.lifecycleState).toBe('archived') + }) + + it('falls back to archiving when the session has no known machine to verify against', async () => { + const sessionId = insertActiveSession('sess-no-machine') + setKillSessionMissingTarget() + let stopRunnerSessionCalled = false + ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = + async () => { stopRunnerSessionCalled = true; return 'already_gone' } + + await engine.archiveSession(sessionId) + + expect(stopRunnerSessionCalled).toBe(false) + const session = cache().getSession(sessionId) + expect(session?.active).toBe(false) + expect(session?.metadata?.lifecycleState).toBe('archived') + }) + + it('archives best-effort when the machine itself is unreachable', async () => { + const sessionId = insertActiveSession('sess-machine-unreachable', 'machine-x') + setKillSessionMissingTarget() + ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = + async () => { throw new Error('machine offline') } + + await engine.archiveSession(sessionId) + + const session = cache().getSession(sessionId) + expect(session?.active).toBe(false) + expect(session?.metadata?.lifecycleState).toBe('archived') + }) +}) From 04a9177a3fd05f96e826d0c50b4c376b1dd01546 Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Thu, 27 Aug 2026 19:33:40 +0000 Subject: [PATCH 2/5] fix(hub): don't coerce stopRunnerSession RPC failures into already_gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review caught a regression of this PR's own bug one RPC layer down: any failure of stopRunnerSession (ack timeout, protocol error — not just a genuinely offline machine) was being coerced to 'already_gone' and archiving the session, exactly the false-positive this fix exists to prevent. Distinguish "no runner connected to ask" (machineCache reports the machine offline — nothing stronger to check than the original RpcTargetMissingError, so fall back to archiving as before) from "the machine is online but the RPC itself failed" (treat as still_alive, the same conservative default terminateInPlacePiResume / terminateUnexpectedPiTemp already use for this exact RPC elsewhere in this file). --- hub/src/sync/syncEngine.ts | 27 +++++++++--- hub/src/sync/syncEngineArchiveSession.test.ts | 42 +++++++++++++++++-- 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/hub/src/sync/syncEngine.ts b/hub/src/sync/syncEngine.ts index 6245bee936..b096704c8a 100644 --- a/hub/src/sync/syncEngine.ts +++ b/hub/src/sync/syncEngine.ts @@ -1691,13 +1691,28 @@ export class SyncEngine { const machineId = this.sessionCache.getSession(sessionId)?.metadata?.machineId if (machineId) { let status: 'stopped' | 'already_gone' | 'still_alive' - try { - status = await this.rpcGateway.stopRunnerSession(machineId, sessionId) - } catch { - // Machine itself unreachable; no stronger signal available - // than the original RpcTargetMissingError, so fall back - // to the prior best-effort behavior. + if (this.machineCache.getMachine(machineId)?.active !== true) { + // No runner connected to ask at all; nothing stronger to + // check than the original RpcTargetMissingError, so fall + // back to the prior best-effort behavior. status = 'already_gone' + } else { + try { + status = await this.rpcGateway.stopRunnerSession(machineId, sessionId) + } catch { + // The machine IS connected but the RPC itself failed + // (ack timeout, protocol error). Unlike an offline + // machine, that does NOT mean the process is gone — + // treat it as still alive, mirroring the conservative + // default `terminateInPlacePiResume` / + // `terminateUnexpectedPiTemp` use for the same RPC + // elsewhere in this file. Coercing an ambiguous + // failure to "already gone" here would silently + // archive a session whose runner just didn't answer + // in time — the exact bug this fallback exists to + // prevent, one RPC layer down. + status = 'still_alive' + } } if (status === 'still_alive') { throw new Error('Session process is still running and could not be stopped') diff --git a/hub/src/sync/syncEngineArchiveSession.test.ts b/hub/src/sync/syncEngineArchiveSession.test.ts index 7b51ce0024..a5ba0a062c 100644 --- a/hub/src/sync/syncEngineArchiveSession.test.ts +++ b/hub/src/sync/syncEngineArchiveSession.test.ts @@ -4,6 +4,7 @@ import { RpcRegistry } from '../socket/rpcRegistry' import { SyncEngine } from './syncEngine' import { RpcTargetMissingError } from './rpcGateway' import type { SessionCache } from './sessionCache' +import type { MachineCache } from './machineCache' /** * `archiveSession`'s only kill mechanism is `rpcGateway.killSession`, a @@ -31,6 +32,15 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { return (engine as unknown as { sessionCache: SessionCache }).sessionCache } + function machineCache(): MachineCache { + return (engine as unknown as { machineCache: MachineCache }).machineCache + } + + function registerOnlineMachine(machineId: string): void { + machineCache().getOrCreateMachine(machineId, {}, {}, NAMESPACE) + machineCache().handleMachineAlive({ machineId, time: Date.now() }) + } + function insertActiveSession(tag: string, machineId?: string): string { const created = cache().getOrCreateSession( tag, @@ -53,6 +63,7 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { }) it('does not archive a session the runner confirms is still alive', async () => { + registerOnlineMachine('machine-x') const sessionId = insertActiveSession('sess-still-alive', 'machine-x') setKillSessionMissingTarget() ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = @@ -66,6 +77,7 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { }) it('archives the session once the runner confirms the process is gone', async () => { + registerOnlineMachine('machine-x') const sessionId = insertActiveSession('sess-confirmed-gone', 'machine-x') setKillSessionMissingTarget() let calledWith: [string, string] | undefined @@ -98,16 +110,40 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { expect(session?.metadata?.lifecycleState).toBe('archived') }) - it('archives best-effort when the machine itself is unreachable', async () => { - const sessionId = insertActiveSession('sess-machine-unreachable', 'machine-x') + it('falls back to archiving when the known machine has never connected', async () => { + // Deliberately does NOT register 'machine-x' in machineCache, so it + // is not online — mirrors the #916 hub-restart-cascade scenario the + // fallback was originally built for. There is no runner to ask, so + // this is the one case where "already gone" is the right guess. + const sessionId = insertActiveSession('sess-machine-never-connected', 'machine-x') setKillSessionMissingTarget() + let stopRunnerSessionCalled = false ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = - async () => { throw new Error('machine offline') } + async () => { stopRunnerSessionCalled = true; return 'already_gone' } await engine.archiveSession(sessionId) + expect(stopRunnerSessionCalled).toBe(false) const session = cache().getSession(sessionId) expect(session?.active).toBe(false) expect(session?.metadata?.lifecycleState).toBe('archived') }) + + it('does NOT archive when the machine is online but the StopSession RPC itself fails', async () => { + // Regression guard: an online machine whose RPC call throws (ack + // timeout, protocol error) must NOT be coerced into "already gone" — + // that would silently archive a session whose runner simply didn't + // answer in time, reproducing this fix's own bug one RPC layer down. + registerOnlineMachine('machine-x') + const sessionId = insertActiveSession('sess-machine-online-rpc-fails', 'machine-x') + setKillSessionMissingTarget() + ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = + async () => { throw new Error('ack timeout') } + + await expect(engine.archiveSession(sessionId)).rejects.toThrow() + + const session = cache().getSession(sessionId) + expect(session?.active).toBe(true) + expect(session?.metadata?.lifecycleState).not.toBe('archived') + }) }) From 7e9e85af29c3323f2f0ddc2f9e0bea967e22984c Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Thu, 27 Aug 2026 19:45:04 +0000 Subject: [PATCH 3/5] fix(runner): distinguish unknown session id from confirmed still_alive Codex review caught a real gap: cli/src/runner/run.ts's stopSession returns 'still_alive' both when it finds a PID that's confirmed still running AND when no PID matches the id anywhere and there's no verified-exit tombstone (a fail-closed default for a genuinely unknown id). archiveSession's new fallback treated both cases identically, so a stale row whose original runner generation lost track of it (the exact 235-row backlog this investigation started from) would permanently fail to archive on an otherwise-healthy machine. Add a fourth 'unknown' status distinct from 'still_alive' for the no-match case, threaded through the runner<->hub RPC contract (cli/src/runner/{run,controlServer,controlClient}.ts, cli/src/api/apiMachine.ts, hub/src/sync/rpcGateway.ts). archiveSession now treats 'unknown' like the offline-machine case (best-effort archive); every other existing stopRunnerSession call site (fork-child cleanup, OpenCode reserved-clear recovery, PTY/Pi resume termination and reconciliation) keeps its prior conservative behavior by treating 'unknown' the same as 'still_alive', since those all deal with sessions the hub has fresh, concrete knowledge of. Updated the existing runner.integration.test.ts assertion for an unknown id (was 'still_alive', now correctly 'unknown') and added a hub-side regression test for the archive path. --- cli/src/api/apiMachine.ts | 2 +- cli/src/runner/controlClient.ts | 4 +- cli/src/runner/controlServer.ts | 4 +- cli/src/runner/run.ts | 11 ++++- cli/src/runner/runner.integration.test.ts | 5 ++- hub/src/sync/rpcGateway.ts | 4 +- hub/src/sync/syncEngine.ts | 40 +++++++++++++------ hub/src/sync/syncEngineArchiveSession.test.ts | 21 ++++++++++ 8 files changed, 69 insertions(+), 22 deletions(-) diff --git a/cli/src/api/apiMachine.ts b/cli/src/api/apiMachine.ts index 4d12c02749..611ee58746 100644 --- a/cli/src/api/apiMachine.ts +++ b/cli/src/api/apiMachine.ts @@ -60,7 +60,7 @@ export { normalizeWindowsDriveRoot } from './machinePathPolicy' type MachineRpcHandlers = { spawnSession: (options: SpawnSessionOptions) => Promise - stopSession: (sessionId: string) => Promise<'stopped' | 'already_gone' | 'still_alive'> + stopSession: (sessionId: string) => Promise<'stopped' | 'already_gone' | 'still_alive' | 'unknown'> requestShutdown: () => void } diff --git a/cli/src/runner/controlClient.ts b/cli/src/runner/controlClient.ts index cd5868ce8a..375d92704d 100644 --- a/cli/src/runner/controlClient.ts +++ b/cli/src/runner/controlClient.ts @@ -96,9 +96,9 @@ export async function listRunnerSessions(): Promise { return result.children || []; } -export async function stopRunnerSession(sessionId: string): Promise<'stopped' | 'already_gone' | 'still_alive'> { +export async function stopRunnerSession(sessionId: string): Promise<'stopped' | 'already_gone' | 'still_alive' | 'unknown'> { const result = await runnerPost('/stop-session', { sessionId }); - return result.status === 'stopped' || result.status === 'already_gone' || result.status === 'still_alive' + return result.status === 'stopped' || result.status === 'already_gone' || result.status === 'still_alive' || result.status === 'unknown' ? result.status : 'still_alive'; } diff --git a/cli/src/runner/controlServer.ts b/cli/src/runner/controlServer.ts index 358f863e2e..1a790cae63 100644 --- a/cli/src/runner/controlServer.ts +++ b/cli/src/runner/controlServer.ts @@ -19,7 +19,7 @@ export function startRunnerControlServer({ onHappySessionWebhook }: { getChildren: () => TrackedSession[]; - stopSession: (sessionId: string) => Promise<'stopped' | 'already_gone' | 'still_alive'>; + stopSession: (sessionId: string) => Promise<'stopped' | 'already_gone' | 'still_alive' | 'unknown'>; spawnSession: (options: SpawnSessionOptions) => Promise; requestShutdown: () => void; onHappySessionWebhook: (sessionId: string, metadata: Metadata) => void; @@ -91,7 +91,7 @@ export function startRunnerControlServer({ }), response: { 200: z.object({ - status: z.enum(['stopped', 'already_gone', 'still_alive']) + status: z.enum(['stopped', 'already_gone', 'still_alive', 'unknown']) }) } } diff --git a/cli/src/runner/run.ts b/cli/src/runner/run.ts index 245dc398fe..6a84ae50a6 100644 --- a/cli/src/runner/run.ts +++ b/cli/src/runner/run.ts @@ -937,7 +937,7 @@ export async function startRunner(options: { workspaceRoots?: string[] } = {}): } // Stop a session by sessionId or PID fallback - const stopSession = async (sessionId: string): Promise<'stopped' | 'already_gone' | 'still_alive'> => { + const stopSession = async (sessionId: string): Promise<'stopped' | 'already_gone' | 'still_alive' | 'unknown'> => { logger.debug(`[RUNNER RUN] Attempting to stop session ${sessionId}`); // Try to find by sessionId first @@ -1046,8 +1046,15 @@ export async function startRunner(options: { workspaceRoots?: string[] } = {}): logger.debug(`[RUNNER RUN] Session ${sessionId} was previously observed exited`); return 'already_gone'; } + // Distinct from 'still_alive': no PID matched this id anywhere (not + // tracked, not persisted, no verified-exit tombstone), so there is + // nothing here to confirm as alive OR dead. Callers that already have + // concrete knowledge of this session (e.g. one they just spawned) should + // treat this the same as 'still_alive' defensively; callers reconciling + // an old/stale row this runner instance never knew about should not be + // blocked forever by a status that never actually confirmed a live process. logger.debug(`[RUNNER RUN] Session ${sessionId} not found without verified exit`); - return 'still_alive'; + return 'unknown'; }; // Handle child process exit diff --git a/cli/src/runner/runner.integration.test.ts b/cli/src/runner/runner.integration.test.ts index f0941a4012..0a51a3081e 100644 --- a/cli/src/runner/runner.integration.test.ts +++ b/cli/src/runner/runner.integration.test.ts @@ -219,7 +219,10 @@ describe.skipIf(!await isServerHealthy())('Runner Integration Tests', { timeout: expect(spawnedSession.happySessionId).toBeDefined(); expect(await stopRunnerSession(spawnedSession.happySessionId)).toBe('stopped'); expect(await stopRunnerSession(spawnedSession.happySessionId)).toBe('already_gone'); - expect(await stopRunnerSession('unknown-session-id')).toBe('still_alive'); + // Distinct from 'still_alive': no PID matched this id and there is no + // verified-exit tombstone, so the runner has no basis to call it either + // alive or dead (see cli/src/runner/run.ts's stopSession fallback). + expect(await stopRunnerSession('unknown-session-id')).toBe('unknown'); }); it.skipIf(process.env.HAPI_RUN_STRESS_TESTS !== 'true')( diff --git a/hub/src/sync/rpcGateway.ts b/hub/src/sync/rpcGateway.ts index b749647f24..8f362d8882 100644 --- a/hub/src/sync/rpcGateway.ts +++ b/hub/src/sync/rpcGateway.ts @@ -156,10 +156,10 @@ export class RpcGateway { await this.sessionRpc(sessionId, RPC_METHODS.KillSession, {}) } - async stopRunnerSession(machineId: string, sessionId: string): Promise<'stopped' | 'already_gone' | 'still_alive'> { + async stopRunnerSession(machineId: string, sessionId: string): Promise<'stopped' | 'already_gone' | 'still_alive' | 'unknown'> { const result = await this.machineRpc(machineId, RPC_METHODS.StopSession, { sessionId }) const status = result && typeof result === 'object' ? (result as { status?: unknown }).status : undefined - if (status === 'stopped' || status === 'already_gone' || status === 'still_alive') return status + if (status === 'stopped' || status === 'already_gone' || status === 'still_alive' || status === 'unknown') return status throw new Error('Unexpected stop-session response') } diff --git a/hub/src/sync/syncEngine.ts b/hub/src/sync/syncEngine.ts index b096704c8a..0bc1a1f44f 100644 --- a/hub/src/sync/syncEngine.ts +++ b/hub/src/sync/syncEngine.ts @@ -1567,7 +1567,13 @@ export class SyncEngine { ): Promise { if (spawnAttempted) { const status = await this.rpcGateway.stopRunnerSession(machineId, childId) - if (status === 'still_alive') { + // Treat 'unknown' (no PID matched, no verified-exit tombstone) the + // same as 'still_alive' here: this child was supposedly just + // spawned this turn, so the runner not recognizing it at all is + // itself a sign something is wrong, not proof it's gone. Unlike + // archiveSession's stale-row fallback, there is no long-lived-row + // case here that needs unblocking. + if (status === 'still_alive' || status === 'unknown') { throw new Error('Fork child termination was not confirmed') } } @@ -1690,7 +1696,7 @@ export class SyncEngine { // supervision while continuing to run. const machineId = this.sessionCache.getSession(sessionId)?.metadata?.machineId if (machineId) { - let status: 'stopped' | 'already_gone' | 'still_alive' + let status: 'stopped' | 'already_gone' | 'still_alive' | 'unknown' if (this.machineCache.getMachine(machineId)?.active !== true) { // No runner connected to ask at all; nothing stronger to // check than the original RpcTargetMissingError, so fall @@ -1714,6 +1720,16 @@ export class SyncEngine { status = 'still_alive' } } + // `'unknown'` means the runner has no PID and no verified-exit + // tombstone for this id at all (cli/src/runner/run.ts's + // stopSession fail-closed default) — it is NOT confirmation + // that a process is running, just that this runner instance + // never tracked it. That's exactly the shape of an old, stale + // row whose original runner generation has long since rotated + // its bookkeeping, which is the archival case this whole + // fallback exists to unblock — so treat it like the + // machine-offline branch above, not like a confirmed-alive + // `'still_alive'`. if (status === 'still_alive') { throw new Error('Session process is still running and could not be stopped') } @@ -2819,7 +2835,7 @@ export class SyncEngine { if (session.active || operation?.state !== 'reserved' || !machineId) return false try { const status = await this.rpcGateway.stopRunnerSession(machineId, session.id) - if (status === 'still_alive') return false + if (status === 'still_alive' || status === 'unknown') return false return this.abortOpenCodeClearSession( session.id, namespace, operation.replacementSessionId, 'reserved', true ).type === 'success' @@ -3106,7 +3122,7 @@ export class SyncEngine { const readyResult = await this.waitForSessionReady(spawnResult.sessionId) if (readyResult !== 'ready') { if (resumedStartingMode === 'pty' && readyResult === 'timeout') { - let status: 'stopped' | 'already_gone' | 'still_alive' + let status: 'stopped' | 'already_gone' | 'still_alive' | 'unknown' try { status = await this.rpcGateway.stopRunnerSession( targetMachine.id, @@ -3624,7 +3640,7 @@ export class SyncEngine { machineId, startedAt: Date.now(), }) - let status: 'stopped' | 'already_gone' | 'still_alive' + let status: 'stopped' | 'already_gone' | 'still_alive' | 'unknown' try { status = await this.rpcGateway.stopRunnerSession(machineId, sessionId) } catch { @@ -3634,7 +3650,7 @@ export class SyncEngine { await new Promise((resolve) => setTimeout(resolve, 0)) const session = this.sessionCache.refreshSession(sessionId) ?? this.sessionCache.getSession(sessionId) const attemptClearedByEnd = session?.metadata?.piResumeAttempt === undefined - if (status === 'still_alive') { + if (status === 'still_alive' || status === 'unknown') { if (attemptClearedByEnd) return true return false } @@ -3658,7 +3674,7 @@ export class SyncEngine { startedAt: Date.now(), childSessionId: sessionId, }) - let status: 'stopped' | 'already_gone' | 'still_alive' + let status: 'stopped' | 'already_gone' | 'still_alive' | 'unknown' try { status = await this.rpcGateway.stopRunnerSession(machineId, sessionId) } catch { @@ -3669,7 +3685,7 @@ export class SyncEngine { const session = this.sessionCache.refreshSession(sessionId) ?? this.sessionCache.getSession(sessionId) const original = this.sessionCache.refreshSession(originalSessionId) ?? this.sessionCache.getSession(originalSessionId) const attemptClearedByEnd = original?.metadata?.piResumeAttempt === undefined - if (status === 'still_alive' && !attemptClearedByEnd) { + if ((status === 'still_alive' || status === 'unknown') && !attemptClearedByEnd) { await this.writePiResumeAttempt(originalSessionId, namespace, { ...existingAttempt, state: 'quarantined', @@ -3787,13 +3803,13 @@ export class SyncEngine { private async reconcilePersistedPtyResumeAttempt(session: Session): Promise { const attempt = session.metadata?.ptyResumeAttempt if (!attempt) return true - let status: 'stopped' | 'already_gone' | 'still_alive' + let status: 'stopped' | 'already_gone' | 'still_alive' | 'unknown' try { status = await this.rpcGateway.stopRunnerSession(attempt.machineId, session.id) } catch { return false } - if (status === 'still_alive') return false + if (status === 'still_alive' || status === 'unknown') return false const current = this.sessionCache.getSession(session.id) if (current?.active) { @@ -3813,13 +3829,13 @@ export class SyncEngine { const attempt = session.metadata?.piResumeAttempt if (!attempt) return true const childSessionId = attempt.childSessionId ?? session.id - let status: 'stopped' | 'already_gone' | 'still_alive' + let status: 'stopped' | 'already_gone' | 'still_alive' | 'unknown' try { status = await this.rpcGateway.stopRunnerSession(attempt.machineId, childSessionId) } catch { return false } - if (status === 'still_alive') return false + if (status === 'still_alive' || status === 'unknown') return false const child = this.sessionCache.getSession(childSessionId) if (child?.active) this.handleSessionEnd({ sid: childSessionId, time: Date.now(), reason: 'error' }) diff --git a/hub/src/sync/syncEngineArchiveSession.test.ts b/hub/src/sync/syncEngineArchiveSession.test.ts index a5ba0a062c..44e0069f7a 100644 --- a/hub/src/sync/syncEngineArchiveSession.test.ts +++ b/hub/src/sync/syncEngineArchiveSession.test.ts @@ -129,6 +129,27 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { expect(session?.metadata?.lifecycleState).toBe('archived') }) + it('archives a stale row when the online machine no longer tracks this session id at all', async () => { + // cli/src/runner/run.ts's stopSession returns 'unknown' — not + // 'still_alive' — when no PID matches this id anywhere and there is + // no verified-exit tombstone (e.g. a row whose original runner + // generation rotated its bookkeeping long ago). That is NOT + // confirmation of a live process, so it must not be treated like a + // genuine 'still_alive' — this is exactly the stale-row case this + // fallback exists to unblock. + registerOnlineMachine('machine-x') + const sessionId = insertActiveSession('sess-unknown-to-runner', 'machine-x') + setKillSessionMissingTarget() + ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = + async () => 'unknown' + + await engine.archiveSession(sessionId) + + const session = cache().getSession(sessionId) + expect(session?.active).toBe(false) + expect(session?.metadata?.lifecycleState).toBe('archived') + }) + it('does NOT archive when the machine is online but the StopSession RPC itself fails', async () => { // Regression guard: an online machine whose RPC call throws (ack // timeout, protocol error) must NOT be coerced into "already gone" — From ac463e2702571d4e4beefdbe20d69893a0b95b71 Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Thu, 27 Aug 2026 19:54:47 +0000 Subject: [PATCH 4/5] fix(hub): don't use heartbeat-derived machine.active to gate the RPC Codex review caught a third variant of this PR's own bug: MachineCache active flips false 45s after the last machine-alive heartbeat (machineCache.ts expireInactive), which is a different signal from whether the machine's RPC target is actually registered (that only clears on socket disconnect, rpcRegistry.unregisterAll). A delayed or dropped heartbeat could leave stopRunnerSession fully reachable while this fallback's `active !== true` pre-check skipped it and archived a live session anyway. Drop the machineCache pre-check entirely. Just attempt stopRunnerSession and let its own failure mode decide: a thrown RpcTargetMissingError means there is genuinely no RPC target to check (fall back to archiving, same as before); any other failure means the machine answered the transport layer but this call didn't resolve, which is not evidence the process is gone (treat as still_alive). Rewrote the regression tests to match: no longer registers a machine in machineCache at all (proving the decision doesn't depend on it), and asserts on the RpcTargetMissingError-vs-other-error distinction directly. --- hub/src/sync/syncEngine.ts | 45 ++++++++-------- hub/src/sync/syncEngineArchiveSession.test.ts | 53 ++++++++----------- 2 files changed, 45 insertions(+), 53 deletions(-) diff --git a/hub/src/sync/syncEngine.ts b/hub/src/sync/syncEngine.ts index 0bc1a1f44f..30306c4b09 100644 --- a/hub/src/sync/syncEngine.ts +++ b/hub/src/sync/syncEngine.ts @@ -1697,28 +1697,29 @@ export class SyncEngine { const machineId = this.sessionCache.getSession(sessionId)?.metadata?.machineId if (machineId) { let status: 'stopped' | 'already_gone' | 'still_alive' | 'unknown' - if (this.machineCache.getMachine(machineId)?.active !== true) { - // No runner connected to ask at all; nothing stronger to - // check than the original RpcTargetMissingError, so fall - // back to the prior best-effort behavior. - status = 'already_gone' - } else { - try { - status = await this.rpcGateway.stopRunnerSession(machineId, sessionId) - } catch { - // The machine IS connected but the RPC itself failed - // (ack timeout, protocol error). Unlike an offline - // machine, that does NOT mean the process is gone — - // treat it as still alive, mirroring the conservative - // default `terminateInPlacePiResume` / - // `terminateUnexpectedPiTemp` use for the same RPC - // elsewhere in this file. Coercing an ambiguous - // failure to "already gone" here would silently - // archive a session whose runner just didn't answer - // in time — the exact bug this fallback exists to - // prevent, one RPC layer down. - status = 'still_alive' - } + try { + status = await this.rpcGateway.stopRunnerSession(machineId, sessionId) + } catch (stopError) { + // `MachineCache.active` is a 45s heartbeat-derived + // heuristic (machineCache.ts's expireInactive), NOT the + // same signal as "this machine's RPC target is + // registered" — a socket only deregisters on actual + // disconnect (rpcRegistry.unregisterAll). A delayed + // heartbeat can leave `active` false while the socket, + // and this exact RPC, are still perfectly reachable. + // So don't pre-check `active` at all — just attempt the + // call and let its own failure mode tell us what + // happened: `RpcTargetMissingError` means the machine + // genuinely has no RPC target (nothing stronger to + // check than the original error), while any other + // failure (ack timeout, protocol error) means the + // machine IS reachable but this particular call didn't + // resolve — which does NOT mean the process is gone. + // Coercing that ambiguous case to "already gone" would + // silently archive a session whose runner just didn't + // answer in time — the exact bug this fallback exists + // to prevent, one RPC layer down. + status = stopError instanceof RpcTargetMissingError ? 'already_gone' : 'still_alive' } // `'unknown'` means the runner has no PID and no verified-exit // tombstone for this id at all (cli/src/runner/run.ts's diff --git a/hub/src/sync/syncEngineArchiveSession.test.ts b/hub/src/sync/syncEngineArchiveSession.test.ts index 44e0069f7a..6832ce490e 100644 --- a/hub/src/sync/syncEngineArchiveSession.test.ts +++ b/hub/src/sync/syncEngineArchiveSession.test.ts @@ -4,7 +4,6 @@ import { RpcRegistry } from '../socket/rpcRegistry' import { SyncEngine } from './syncEngine' import { RpcTargetMissingError } from './rpcGateway' import type { SessionCache } from './sessionCache' -import type { MachineCache } from './machineCache' /** * `archiveSession`'s only kill mechanism is `rpcGateway.killSession`, a @@ -21,7 +20,12 @@ import type { MachineCache } from './machineCache' * pointed at it. The fix confirms with the runner's machine-level * `StopSession` RPC (which resolves the child by PID and checks both the * requested and confirmed session ids) before trusting that the process is - * actually gone. + * actually gone. It deliberately does NOT pre-check `MachineCache.active` + * (a 45s heartbeat heuristic) before attempting that RPC — a delayed + * heartbeat can leave `active` false while the machine's RPC target is + * still fully registered and reachable — so the RPC's own + * `RpcTargetMissingError` failure mode is the only signal trusted for + * "nothing to check." */ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { let store: Store @@ -32,15 +36,6 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { return (engine as unknown as { sessionCache: SessionCache }).sessionCache } - function machineCache(): MachineCache { - return (engine as unknown as { machineCache: MachineCache }).machineCache - } - - function registerOnlineMachine(machineId: string): void { - machineCache().getOrCreateMachine(machineId, {}, {}, NAMESPACE) - machineCache().handleMachineAlive({ machineId, time: Date.now() }) - } - function insertActiveSession(tag: string, machineId?: string): string { const created = cache().getOrCreateSession( tag, @@ -63,7 +58,6 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { }) it('does not archive a session the runner confirms is still alive', async () => { - registerOnlineMachine('machine-x') const sessionId = insertActiveSession('sess-still-alive', 'machine-x') setKillSessionMissingTarget() ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = @@ -77,7 +71,6 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { }) it('archives the session once the runner confirms the process is gone', async () => { - registerOnlineMachine('machine-x') const sessionId = insertActiveSession('sess-confirmed-gone', 'machine-x') setKillSessionMissingTarget() let calledWith: [string, string] | undefined @@ -110,26 +103,24 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { expect(session?.metadata?.lifecycleState).toBe('archived') }) - it('falls back to archiving when the known machine has never connected', async () => { - // Deliberately does NOT register 'machine-x' in machineCache, so it - // is not online — mirrors the #916 hub-restart-cascade scenario the - // fallback was originally built for. There is no runner to ask, so - // this is the one case where "already gone" is the right guess. - const sessionId = insertActiveSession('sess-machine-never-connected', 'machine-x') + it('falls back to archiving when the machine has no RPC target at all (RpcTargetMissingError)', async () => { + // This is the genuinely-unreachable case — mirrors the #916 + // hub-restart-cascade scenario the fallback was originally built + // for. There is no runner to ask, so "already gone" is the right + // guess. + const sessionId = insertActiveSession('sess-machine-unreachable', 'machine-x') setKillSessionMissingTarget() - let stopRunnerSessionCalled = false ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = - async () => { stopRunnerSessionCalled = true; return 'already_gone' } + async () => { throw new RpcTargetMissingError('StopSession', 'handler-not-registered') } await engine.archiveSession(sessionId) - expect(stopRunnerSessionCalled).toBe(false) const session = cache().getSession(sessionId) expect(session?.active).toBe(false) expect(session?.metadata?.lifecycleState).toBe('archived') }) - it('archives a stale row when the online machine no longer tracks this session id at all', async () => { + it('archives a stale row when the reachable machine no longer tracks this session id at all', async () => { // cli/src/runner/run.ts's stopSession returns 'unknown' — not // 'still_alive' — when no PID matches this id anywhere and there is // no verified-exit tombstone (e.g. a row whose original runner @@ -137,7 +128,6 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { // confirmation of a live process, so it must not be treated like a // genuine 'still_alive' — this is exactly the stale-row case this // fallback exists to unblock. - registerOnlineMachine('machine-x') const sessionId = insertActiveSession('sess-unknown-to-runner', 'machine-x') setKillSessionMissingTarget() ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = @@ -150,13 +140,13 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { expect(session?.metadata?.lifecycleState).toBe('archived') }) - it('does NOT archive when the machine is online but the StopSession RPC itself fails', async () => { - // Regression guard: an online machine whose RPC call throws (ack - // timeout, protocol error) must NOT be coerced into "already gone" — - // that would silently archive a session whose runner simply didn't - // answer in time, reproducing this fix's own bug one RPC layer down. - registerOnlineMachine('machine-x') - const sessionId = insertActiveSession('sess-machine-online-rpc-fails', 'machine-x') + it('does NOT archive when the StopSession RPC fails ambiguously (not a target-missing error)', async () => { + // Regression guard: an ack timeout / protocol error from an + // otherwise-reachable machine must NOT be coerced into "already + // gone" — that would silently archive a session whose runner + // simply didn't answer in time, reproducing this fix's own bug one + // RPC layer down. + const sessionId = insertActiveSession('sess-machine-ambiguous-failure', 'machine-x') setKillSessionMissingTarget() ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = async () => { throw new Error('ack timeout') } @@ -167,4 +157,5 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { expect(session?.active).toBe(true) expect(session?.metadata?.lifecycleState).not.toBe('archived') }) + }) From 26c1615ae754ddb16b43fdbae4850f90ea7d6d85 Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Thu, 27 Aug 2026 20:01:19 +0000 Subject: [PATCH 5/5] fix(hub): treat 'unknown' the same as still_alive in archiveSession MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review caught that treating 'unknown' as archivable was itself unsafe: terminal-started sessions are tracked only in-memory (pidToTrackedSession), never persisted to the resume-process file unlike runner-spawned children. A still-alive terminal session can therefore return 'unknown' right after this exact runner process restarts and loses that in-memory tracking — indistinguishable, from this RPC alone, from a row this runner generation genuinely never knew about (the stale-row case 'unknown' was introduced for). Given that ambiguity can't be resolved from this RPC's response alone, treat 'unknown' conservatively — the same as 'still_alive' — matching every other stopRunnerSession call site in this file. The 'unknown' type distinction stays (it's still the runner's accurate contract, and fixes the pre-existing integration test's assertion), it just doesn't unblock archival on its own; that's now squarely #842's reconciler territory, not this fallback's job. --- hub/src/sync/syncEngine.ts | 21 ++++++++++++------- hub/src/sync/syncEngineArchiveSession.test.ts | 20 ++++++++++-------- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/hub/src/sync/syncEngine.ts b/hub/src/sync/syncEngine.ts index 30306c4b09..5c24487182 100644 --- a/hub/src/sync/syncEngine.ts +++ b/hub/src/sync/syncEngine.ts @@ -1724,14 +1724,19 @@ export class SyncEngine { // `'unknown'` means the runner has no PID and no verified-exit // tombstone for this id at all (cli/src/runner/run.ts's // stopSession fail-closed default) — it is NOT confirmation - // that a process is running, just that this runner instance - // never tracked it. That's exactly the shape of an old, stale - // row whose original runner generation has long since rotated - // its bookkeeping, which is the archival case this whole - // fallback exists to unblock — so treat it like the - // machine-offline branch above, not like a confirmed-alive - // `'still_alive'`. - if (status === 'still_alive') { + // that a process is running, but it is also NOT confirmation + // that it's gone: terminal-started sessions are tracked only + // in-memory (pidToTrackedSession), never persisted, so a + // still-alive terminal session can return 'unknown' right + // after this exact runner process restarts and loses that + // in-memory tracking — indistinguishable, from this RPC + // alone, from a session this runner generation never knew + // about at all. Given that ambiguity, treat it the same as + // `'still_alive'` — conservative, matching every other + // stopRunnerSession call site in this file — rather than + // risk silently archiving a session that's actually still + // running. + if (status === 'still_alive' || status === 'unknown') { throw new Error('Session process is still running and could not be stopped') } } diff --git a/hub/src/sync/syncEngineArchiveSession.test.ts b/hub/src/sync/syncEngineArchiveSession.test.ts index 6832ce490e..fd5b016062 100644 --- a/hub/src/sync/syncEngineArchiveSession.test.ts +++ b/hub/src/sync/syncEngineArchiveSession.test.ts @@ -120,24 +120,26 @@ describe('SyncEngine.archiveSession RpcTargetMissingError fallback', () => { expect(session?.metadata?.lifecycleState).toBe('archived') }) - it('archives a stale row when the reachable machine no longer tracks this session id at all', async () => { + it('does NOT archive when the runner reports the session id as unknown', async () => { // cli/src/runner/run.ts's stopSession returns 'unknown' — not // 'still_alive' — when no PID matches this id anywhere and there is - // no verified-exit tombstone (e.g. a row whose original runner - // generation rotated its bookkeeping long ago). That is NOT - // confirmation of a live process, so it must not be treated like a - // genuine 'still_alive' — this is exactly the stale-row case this - // fallback exists to unblock. + // no verified-exit tombstone. That is NOT confirmation the process + // is gone: terminal-started sessions are tracked only in-memory + // (never persisted across a runner restart), so a still-alive + // terminal session can return 'unknown' right after this exact + // runner process restarts — indistinguishable, from this RPC alone, + // from a row this runner generation genuinely never knew about. + // Treat it conservatively, the same as 'still_alive'. const sessionId = insertActiveSession('sess-unknown-to-runner', 'machine-x') setKillSessionMissingTarget() ;(engine as unknown as { rpcGateway: { stopRunnerSession: unknown } }).rpcGateway.stopRunnerSession = async () => 'unknown' - await engine.archiveSession(sessionId) + await expect(engine.archiveSession(sessionId)).rejects.toThrow() const session = cache().getSession(sessionId) - expect(session?.active).toBe(false) - expect(session?.metadata?.lifecycleState).toBe('archived') + expect(session?.active).toBe(true) + expect(session?.metadata?.lifecycleState).not.toBe('archived') }) it('does NOT archive when the StopSession RPC fails ambiguously (not a target-missing error)', async () => {