Skip to content

Commit c435140

Browse files
committed
Keep a rotated session crash-coverable instead of inferring it
Routing every non-"running" run.json write through finalizeRunState made the terminal status itself the signal that the run had ended, and so cleared the active-run handle. Session rotation (/clear, /new) breaks that equivalence: it persists a terminal "done" for the outgoing session while the process keeps running under a fresh session id. Clearing liveness there left getActiveRun() null for the rest of the process, so a crash after the first rotation never wrote a terminal record and the session read as "running" forever. Why a write happens is now explicit (SnapshotKind: progress, session-rotation, run-end) instead of inferred from what it writes. Only run-end clears the handle; persistRunSnapshot cannot request it, since everything routed there happens while the process is still alive. Also drops the deleted `active` field from the crash fixture, which sits outside tsconfig's include and so escaped typecheck.
1 parent 607f59f commit c435140

3 files changed

Lines changed: 120 additions & 18 deletions

File tree

src/tui/run-snapshot-kind.test.ts

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
import { mkdtempSync, rmSync } from "node:fs";
2+
import { tmpdir } from "node:os";
3+
import { join } from "node:path";
4+
5+
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
6+
7+
import { clearActiveRun, getActiveRun, setActiveRun } from "../session/active-run.js";
8+
import { finalizeRunState, loadState, saveState, type RunState } from "../session/state.js";
9+
import { clearsActiveRun, type SnapshotKind } from "./runner.js";
10+
11+
describe("clearsActiveRun", () => {
12+
test("only the run-ending write clears the active-run handle", () => {
13+
expect(clearsActiveRun("run-end")).toBe(true);
14+
expect(clearsActiveRun("progress")).toBe(false);
15+
// The regression this pins: a /clear or /new rotation persists a
16+
// terminal "done" for the outgoing session, but the process lives on.
17+
// Clearing liveness here leaves every later session uncovered by the
18+
// crash handler, so a crash after the first rotation never writes a
19+
// terminal record and the session reads as "running" forever.
20+
expect(clearsActiveRun("session-rotation")).toBe(false);
21+
});
22+
});
23+
24+
describe("a snapshot write dispatched by kind", () => {
25+
let cwd = "";
26+
let home = "";
27+
28+
// Mirrors writeRunSnapshot's dispatch in runner.ts so the rule above is
29+
// exercised against the real state writers, not just asserted in isolation.
30+
const write = async (sessionId: string, state: RunState, kind: SnapshotKind): Promise<void> => {
31+
if (clearsActiveRun(kind)) {
32+
await finalizeRunState(cwd, sessionId, state, home);
33+
return;
34+
}
35+
await saveState(cwd, sessionId, state, home);
36+
};
37+
38+
const runState = (over: Partial<RunState>): RunState => ({
39+
status: "running",
40+
turnsUsed: 0,
41+
task: "task",
42+
startedAt: 1,
43+
...over,
44+
});
45+
46+
beforeEach(() => {
47+
cwd = mkdtempSync(join(tmpdir(), "snapshot-kind-cwd-"));
48+
home = mkdtempSync(join(tmpdir(), "snapshot-kind-home-"));
49+
});
50+
51+
afterEach(() => {
52+
clearActiveRun();
53+
rmSync(cwd, { recursive: true, force: true });
54+
rmSync(home, { recursive: true, force: true });
55+
});
56+
57+
test("a rotation still records the outgoing session but leaves the run crash-coverable", async () => {
58+
setActiveRun({ sessionId: "old", cwd, task: "task", startedAt: 1 });
59+
60+
await write("old", runState({ status: "done", finishedAt: 10 }), "session-rotation");
61+
62+
expect((await loadState(cwd, "old", home))?.status).toBe("done");
63+
// The rotated-in session is repointed on the same handle, so the handle
64+
// must survive the write for the crash handler to have anything to close.
65+
expect(getActiveRun()).not.toBeNull();
66+
});
67+
68+
test("the run-ending write records the session and disarms the handle", async () => {
69+
setActiveRun({ sessionId: "last", cwd, task: "task", startedAt: 1 });
70+
71+
await write("last", runState({ status: "done", finishedAt: 20 }), "run-end");
72+
73+
expect((await loadState(cwd, "last", home))?.status).toBe("done");
74+
expect(getActiveRun()).toBeNull();
75+
});
76+
});

src/tui/runner.ts

Lines changed: 43 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,24 @@ export function resolveResumeSeed(pickedState: RunState | null): ResumeSeed {
248248
};
249249
}
250250

251+
/**
252+
* Why a run.json snapshot is being written. Only "run-end" ends the run
253+
* itself and so clears the active-run handle that the crash handler in
254+
* index.ts reads.
255+
*
256+
* RunState.status cannot stand in for this. A /clear or /new rotation
257+
* persists a terminal "done" for the outgoing session while the process
258+
* keeps running under a fresh session id, so inferring "the run is over"
259+
* from a non-"running" status disarms crash finalization for everything
260+
* after the first rotation -- the session that dies then never gets its
261+
* terminal record and reads as "running" forever.
262+
*/
263+
export type SnapshotKind = "progress" | "session-rotation" | "run-end";
264+
265+
export function clearsActiveRun(kind: SnapshotKind): boolean {
266+
return kind === "run-end";
267+
}
268+
251269
const GRANT_SCOPE_LABEL: Record<GrantScope, string> = {
252270
session: "This session",
253271
project: "This project",
@@ -1441,6 +1459,7 @@ export async function runTUI(initialConfig: Config): Promise<number> {
14411459
const writeRunSnapshot = async (
14421460
status: RunState["status"],
14431461
extra?: Pick<RunState, "finishedAt" | "error">,
1462+
kind: SnapshotKind = "progress",
14441463
): Promise<void> => {
14451464
const task = runTaskTitle.trim().length > 0 ? runTaskTitle.trim() : "(conversation)";
14461465
const model = `${liveSource.id}:${liveSource.model}`;
@@ -1458,28 +1477,29 @@ export async function runTUI(initialConfig: Config): Promise<number> {
14581477
mcpServers: connectedMcpServers,
14591478
...extra,
14601479
};
1461-
// "running" is the only non-terminal status writeRunSnapshot ever
1462-
// receives (progress snapshots); anything else closes the run out, so
1463-
// the active-run handle is cleared in the same call as the disk write
1464-
// rather than by a separate statement at each terminal call site.
1465-
if (status === "running") {
1466-
await saveState(config.cwd, sessionId, state);
1467-
} else {
1480+
if (clearsActiveRun(kind)) {
14681481
await finalizeRunState(config.cwd, sessionId, state);
1482+
} else {
1483+
await saveState(config.cwd, sessionId, state);
14691484
}
14701485
};
14711486

14721487
// Progress snapshots are fired unsequenced (model switch, MCP connect, turn
14731488
// completion), so a straggler could otherwise land after the terminal write
14741489
// and resurrect status "running" — atomicWrite is last-rename-wins. Once the
1475-
// run is finalized, drop them; the terminal paths write through
1490+
// run is finalized, drop them; the run-ending path writes through
14761491
// writeRunSnapshot directly.
1492+
//
1493+
// Never a "run-end" write: everything routed here happens while the process
1494+
// is still alive and must stay crash-coverable, including the rotation
1495+
// "done" that closes out a session on /clear or /new.
14771496
const persistRunSnapshot = async (
14781497
status: RunState["status"],
14791498
extra?: Pick<RunState, "finishedAt" | "error">,
1499+
kind: Exclude<SnapshotKind, "run-end"> = "progress",
14801500
): Promise<void> => {
14811501
if (finalized) return;
1482-
await writeRunSnapshot(status, extra);
1502+
await writeRunSnapshot(status, extra, kind);
14831503
};
14841504

14851505
// Cycles persist to the context store only on inference.done; the recorder
@@ -1718,8 +1738,10 @@ export async function runTUI(initialConfig: Config): Promise<number> {
17181738
error: err instanceof Error ? err.message : String(err),
17191739
});
17201740
});
1721-
await persistRunSnapshot("done", { finishedAt: Date.now() });
1741+
await persistRunSnapshot("done", { finishedAt: Date.now() }, "session-rotation");
17221742
sessionId = generateSessionId();
1743+
// Repointed, not cleared: the process lives on, so the crash handler
1744+
// must keep finding this handle and close out the *new* session.
17231745
activeRunHandle.sessionId = sessionId;
17241746
startedAt = Date.now();
17251747
runTaskTitle = config.task;
@@ -2388,13 +2410,17 @@ export async function runTUI(initialConfig: Config): Promise<number> {
23882410
// finished run (finishedAt set) can be left reading as still in progress.
23892411
const persistedStatus: RunState["status"] = summaryStatus;
23902412
finalized = true;
2391-
// writeRunSnapshot clears the active-run handle itself for a terminal
2392-
// status (via finalizeRunState in state.ts), pairing the on-disk write
2393-
// with the in-memory one instead of setting them at two call sites.
2394-
await writeRunSnapshot(persistedStatus, {
2395-
finishedAt,
2396-
...(sinkError !== undefined ? { error: sinkError } : {}),
2397-
});
2413+
// The run itself is over here, so this write clears the active-run handle
2414+
// (via finalizeRunState in state.ts) in the same call, rather than pairing
2415+
// the on-disk write with a separate in-memory statement at this call site.
2416+
await writeRunSnapshot(
2417+
persistedStatus,
2418+
{
2419+
finishedAt,
2420+
...(sinkError !== undefined ? { error: sinkError } : {}),
2421+
},
2422+
"run-end",
2423+
);
23982424
const runSummary = createRunSummary({
23992425
task: runTaskTitle.length > 0 ? runTaskTitle : config.task,
24002426
status: summaryStatus,

tests/fixtures/crash-run/simulate-crash.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ await saveState(cwd, sessionId, {
2626
model,
2727
});
2828

29-
setActiveRun({ sessionId, cwd, active: true, task, startedAt, model });
29+
setActiveRun({ sessionId, cwd, task, startedAt, model });
3030
installCrashHandlers();
3131

3232
process.stdout.write(`${sessionDir(cwd, sessionId)}\n`);

0 commit comments

Comments
 (0)