From 5108a24bf0f63eff8d2dd92e661f668a148219c7 Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Thu, 17 Sep 2026 18:46:15 +0200 Subject: [PATCH] fix(sessions): never restore a session whose agent was already exited MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by a real reboot, which is the first thing to catch it. Typing `/exit` ends the CLI process and leaves the session record behind, and the process-exit handler persists `pid: null` with `status: 'idle'` before anything else runs. By status alone that is indistinguishable from a session sitting idle when the power went, so the boot pass offered those sessions back and a click spawned the agents the user had deliberately closed — the exact case the eligibility rule exists to exclude. The absent pid is what tells the two apart, and the plan step now refuses a record without one, under its own `not-running` reason so the boot log says why. On a healthy board every running session carries a pid; a record with none describes an agent that is already gone. Deliberately the conservative direction. A session that somehow persisted no pid while genuinely running is not offered, and its conversation stays reachable from the Resume list, which is where every session would be without this feature. The opposite error spawns processes nobody asked for. Ark0N/Codeman#446 covers the dead panes those exits leave behind, but this does not wait on it: the rule belongs here whether or not the record's shape changes later. Refs #411 Co-Authored-By: Claude Opus 5 (1M context) --- src/reboot-restore.ts | 25 ++++++++++++++++++++++++- test/reboot-restore.test.ts | 27 +++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/reboot-restore.ts b/src/reboot-restore.ts index e226138c..d295a34d 100644 --- a/src/reboot-restore.ts +++ b/src/reboot-restore.ts @@ -19,6 +19,13 @@ * touching its status, so a pinned session a reboot killed still reads `idle` or * `busy` and stays eligible. * + * Ending the AGENT rather than the session leaves a third shape, and it is the one + * a real reboot caught this module getting wrong. `/exit` ends the CLI process + * while the session record survives, and the process-exit handler persists + * `pid: null` with `status: 'idle'` — indistinguishable by status from a session + * that was merely idle when the power went. The absent pid is what tells them + * apart, so a record without one is refused. + * * @dependencies types (SessionState), config/cli-registry * @consumedby web/server (plan build at boot), web/routes/reboot-restore-routes * @@ -89,7 +96,7 @@ export function resolveResumeConversationId(state: SessionState): string { /** * Why one session was passed over. Reported for logging and shown to the user. * - * The first six are decided before anything is built. `capacity-reached` and + * The first seven are decided before anything is built. `capacity-reached` and * `rebuild-failed` can only happen once a click is spending the plan, and they * are the two the banner must not confuse with a missing workspace: one means * "try again after closing something", the other means the CLI would not start. @@ -99,6 +106,7 @@ export interface RebootRestoreRejection { reason: | 'no-persisted-record' | 'intentionally-ended' + | 'not-running' | 'respawn-blocked' | 'remote-or-docker' | 'unsupported-mode' @@ -163,6 +171,21 @@ export function planRebootRestore( skipped.push({ sessionId, reason: 'intentionally-ended' }); continue; } + if (state.pid === null || state.pid === undefined) { + // The agent had already exited when the machine went down: `/exit` ends the + // process, and its exit handler persists `pid: null` with `status: 'idle'` + // before anything else can. Status alone cannot tell that apart from a + // session that was simply sitting idle when the power went, so without this + // a reboot restore spawns the agents the user deliberately closed — the + // exact case the eligibility rule exists to exclude. + // + // A heuristic, and deliberately the conservative one. A session that somehow + // persisted no pid while genuinely running is not offered, and its + // conversation stays reachable from the Resume list, which is where every + // session would be without this feature. + skipped.push({ sessionId, reason: 'not-running' }); + continue; + } if (state.respawnBlocked === true) { // The crash-loop breaker tripped on this pane. Re-creating it restarts the loop. skipped.push({ sessionId, reason: 'respawn-blocked' }); diff --git a/test/reboot-restore.test.ts b/test/reboot-restore.test.ts index aaf9a83e..5ed989fd 100644 --- a/test/reboot-restore.test.ts +++ b/test/reboot-restore.test.ts @@ -39,6 +39,7 @@ const NOW = 1_760_000_000_000; function persistedSession(overrides: Partial & { id: string }): SessionState { return { + // A live agent's record carries its process id; `/exit` persists null instead. pid: 99999, status: 'idle', workingDir: '/tmp/spike', @@ -137,6 +138,32 @@ describe('which dead sessions may be rebuilt', () => { }); }); +describe('a session whose agent had already exited', () => { + it('is refused, because `/exit` leaves the record reading idle with no pid', () => { + // What the process-exit handler persists: the CLI is gone, the record is not, + // and its status is indistinguishable from a session that was merely idle. + const persisted = { exited: persistedSession({ id: 'exited', status: 'idle', pid: null }) }; + const plan = planRebootRestore(['exited'], persisted, () => true); + expect(plan.restore).toEqual([]); + expect(plan.skipped).toEqual([{ sessionId: 'exited', reason: 'not-running' }]); + }); + + it('still restores the session beside it that was running when the power went', () => { + const persisted = { + exited: persistedSession({ id: 'exited', pid: null }), + running: persistedSession({ id: 'running', pid: 4242 }), + }; + const plan = planRebootRestore(['exited', 'running'], persisted, () => true); + expect(plan.restore.map((entry) => entry.sessionId)).toEqual(['running']); + expect(plan.skipped.map((s) => s.reason)).toEqual(['not-running']); + }); + + it('refuses a record with no pid field at all', () => { + const persisted = { odd: persistedSession({ id: 'odd', pid: undefined as unknown as null }) }; + expect(planRebootRestore(['odd'], persisted, () => true).skipped[0].reason).toBe('not-running'); + }); +}); + describe('a workspace that is no longer on disk', () => { it('is kept out of the offer, so a click cannot scaffold a deleted repo', () => { const persisted = { gone: persistedSession({ id: 'gone', workingDir: '/tmp/deleted-repo' }) };