Skip to content

Commit fd0ca83

Browse files
TheGreatAxiosTheGreatAxios
authored
Merge pull request #404 from corbitsdev/cl-5675-runstatehandleactive-in-memory-and-runstatestatus-on-disk
Tie active-run liveness to a single write instead of two independent ones
2 parents f19d041 + 5aa65e2 commit fd0ca83

11 files changed

Lines changed: 422 additions & 29 deletions

File tree

‎src/index.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ export async function handleFatal(kind: CrashKind, error: unknown): Promise<void
177177
// would block process.exit indefinitely, defeating this handler's one job.
178178
async function finalizeActiveRunOnCrash(error: unknown): Promise<void> {
179179
const run = getActiveRun();
180-
if (run === null || !run.active) return;
180+
if (run === null) return;
181181
const message = error instanceof Error ? error.message : String(error);
182182
try {
183183
await saveCrashState(run.cwd, run.sessionId, {
@@ -216,10 +216,12 @@ export function installCrashHandlers(): void {
216216
// Mirrors finalizeActiveRunOnCrash but is not itself a crash — a signal is a
217217
// clean, externally-requested termination (operator, shell, orchestrator),
218218
// so the run is left "failed" (interrupted) rather than "crashed", and no
219-
// crash report is written for it.
219+
// crash report is written for it. Callers must markCrashed() before this so
220+
// chained saveState renames cannot clobber the terminal write (same contract
221+
// as the uncaughtException path).
220222
async function finalizeActiveRunOnSignal(signal: NodeJS.Signals): Promise<void> {
221223
const run = getActiveRun();
222-
if (run === null || !run.active) return;
224+
if (run === null) return;
223225
try {
224226
await saveCrashState(run.cwd, run.sessionId, {
225227
status: "failed",
@@ -277,6 +279,9 @@ export function installSignalHandlers(): void {
277279
`host dispose failed handling ${signal}: ${disposeErr instanceof Error ? disposeErr.message : String(disposeErr)}\n`,
278280
);
279281
}
282+
// Same fence as handleFatal: any snapshot still queued in writeChains must
283+
// see isCrashed and step aside before saveCrashState renames run.json.
284+
markCrashed();
280285
void finalizeActiveRunOnSignal(signal).finally(() => {
281286
process.exit(128 + SIGNAL_EXIT_NUMBER[signal]);
282287
});

‎src/session/active-run.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,14 @@
99
// crash path has the exact failure mode primeCrashReporting (src/crash/
1010
// report.ts) exists to avoid for git: a stalled disk or network mount would
1111
// block process.exit forever.
12+
//
13+
// Liveness has exactly one representation: presence of this handle in the
14+
// module-level slot (see getActiveRun below). There is no separate "active"
15+
// flag on the handle itself — a second field would just be a copy of the
16+
// same fact, free to drift from the slot it's meant to describe.
1217
export type RunStateHandle = {
1318
sessionId: string;
1419
cwd: string;
15-
active: boolean;
1620
task: string;
1721
startedAt: number;
1822
model?: string;

‎src/session/state.test.ts‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@ afterAll(() => {
2929
mock.module("node:fs/promises", () => realFs);
3030
});
3131

32-
const { loadState, saveState } = await import("./state.js");
32+
const { finalizeRunState, loadState, saveState } = await import("./state.js");
33+
const { getActiveRun, setActiveRun } = await import("./active-run.js");
3334
type RunState = Awaited<ReturnType<typeof loadState>>;
3435

3536
let cwd = "";
@@ -72,6 +73,19 @@ test("a straggler snapshot started before a terminal write does not overwrite it
7273
expect(final?.finishedAt).toBe(999);
7374
});
7475

76+
test("a persisted terminal status agrees with the active-run handle without a second call site", async () => {
77+
const sessionId = "sess-terminal";
78+
setActiveRun({ sessionId, cwd, task: "task", startedAt: 1 });
79+
80+
await finalizeRunState(cwd, sessionId, state({ status: "done", finishedAt: 1000 }), home);
81+
82+
const persisted = await loadState(cwd, sessionId, home);
83+
expect(persisted?.status).toBe("done");
84+
// The only liveness representation left is presence in the active-run
85+
// slot -- a terminal RunState.status must leave nothing there to read.
86+
expect(getActiveRun()).toBeNull();
87+
});
88+
7589
test("saveState calls for different sessions do not block each other", async () => {
7690
await Promise.all([
7791
saveState(cwd, "session-a", state({ task: "a" }), home),

‎src/session/state.ts‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { dirname, join } from "node:path";
44
import { type } from "arktype";
55

66
import { sessionDir } from "./index.js";
7-
import { getTestWriteGate, isCrashed } from "./active-run.js";
7+
import { clearActiveRun, getTestWriteGate, isCrashed } from "./active-run.js";
88
import { COMMAND_NAME } from "../branding.js";
99

1010
const ConnectedMcpServerSchema = type({
@@ -108,6 +108,29 @@ export async function saveState(
108108
return write;
109109
}
110110

111+
// Single write path for a terminal RunState: pairs the on-disk status with
112+
// clearing the in-memory active-run handle (active-run.ts) so the two facts
113+
// are set together instead of at two independent call sites that could drift.
114+
// Callers writing a non-terminal ("running") snapshot should call saveState
115+
// directly — clearing the active-run handle on a running snapshot would be
116+
// wrong, not merely redundant.
117+
//
118+
// The clear happens before the saveState await, not after: this run is
119+
// closing out regardless of whether the write below succeeds, and a signal
120+
// or uncaught exception landing during that await must see the handle
121+
// already gone, or it races a second "crashed" write (src/index.ts's process
122+
// handlers, via saveCrashState) against the terminal write in flight here.
123+
// Clearing after the await leaves that exact window open on every terminal
124+
// write, not only the crash path's own.
125+
export async function finalizeRunState(
126+
cwd: string,
127+
sessionId: string,
128+
state: RunState,
129+
home?: string,
130+
): Promise<void> {
131+
clearActiveRun();
132+
await saveState(cwd, sessionId, state, home);
133+
}
111134

112135
// Crash-time terminal write. Deliberately bypasses writeChains: a hung or
113136
// still-pending write for this session (possibly the very write mid-flight
@@ -116,6 +139,15 @@ export async function saveState(
116139
// Callers must call markCrashed() (src/session/active-run.ts) before this, so
117140
// any snapshot write still queued behind another one in the chain steps
118141
// aside instead of racing this write's rename().
142+
//
143+
// This is a second terminal write path alongside finalizeRunState, and stays
144+
// separate on purpose: its only callers are index.ts's process-level
145+
// uncaughtException/unhandledRejection and signal handlers, reached when a
146+
// crash escapes runTUI's own try/catch entirely. finalizeRunState routes
147+
// through saveState's per-session write chain so writes apply in call order;
148+
// that chain is exactly what a crash exit cannot afford to wait on, since
149+
// process.exit must happen deterministically and a stuck earlier write
150+
// (possibly the one that caused the crash) would otherwise hang it.
119151
export async function saveCrashState(
120152
cwd: string,
121153
sessionId: string,

‎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: 62 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ import {
169169
import { createRunSink } from "../session/run-sink.js";
170170
import { generateSessionId, initSessionDir, renameSession, sessionContextDir, sessionDir } from "../session/index.js";
171171
import { resolveSessionLabel, truncateSessionLabel } from "../session/session-label.js";
172-
import { loadState, saveState, type ConnectedMcpServer, type RunState } from "../session/state.js";
172+
import { finalizeRunState, loadState, saveState, type ConnectedMcpServer, type RunState } from "../session/state.js";
173173
import { setActiveRun, clearActiveRun, type RunStateHandle } from "../session/active-run.js";
174174
import { setActiveDisposeHost, clearActiveDisposeHost } from "../session/active-host.js";
175175
import { openInBrowser } from "../auth/oauth/browser.js";
@@ -244,6 +244,24 @@ export function resolveResumeSeed(pickedState: RunState | null): ResumeSeed {
244244
};
245245
}
246246

247+
/**
248+
* Why a run.json snapshot is being written. Only "run-end" ends the run
249+
* itself and so clears the active-run handle that the crash handler in
250+
* index.ts reads.
251+
*
252+
* RunState.status cannot stand in for this. A /clear or /new rotation
253+
* persists a terminal "done" for the outgoing session while the process
254+
* keeps running under a fresh session id, so inferring "the run is over"
255+
* from a non-"running" status disarms crash finalization for everything
256+
* after the first rotation -- the session that dies then never gets its
257+
* terminal record and reads as "running" forever.
258+
*/
259+
export type SnapshotKind = "progress" | "session-rotation" | "run-end";
260+
261+
export function clearsActiveRun(kind: SnapshotKind): boolean {
262+
return kind === "run-end";
263+
}
264+
247265
const GRANT_SCOPE_LABEL: Record<GrantScope, string> = {
248266
session: "This session",
249267
project: "This project",
@@ -507,7 +525,6 @@ export async function runTUI(initialConfig: Config): Promise<number> {
507525
const activeRunHandle: RunStateHandle = {
508526
sessionId,
509527
cwd: config.cwd,
510-
active: true,
511528
task: runTaskTitle.trim().length > 0 ? runTaskTitle.trim() : "(conversation)",
512529
startedAt,
513530
model: `${config.providerName}:${config.model}`,
@@ -540,7 +557,15 @@ export async function runTUI(initialConfig: Config): Promise<number> {
540557
const finalizeOnCrash = async (err: unknown): Promise<void> => {
541558
if (finalized) return;
542559
finalized = true;
543-
activeRunHandle.active = false;
560+
// Clear the active-run handle up front, before the awaits below. This
561+
// handler isn't the only reader of the handle: index.ts installs its own
562+
// uncaughtException/unhandledRejection listeners that call getActiveRun()
563+
// directly and, if it's still set, write a competing "crashed" record via
564+
// saveCrashState. finalizeRunState (state.ts) also clears the handle
565+
// before its own saveState await, but only once it's called below — an
566+
// escaped throw during the flushPartialOnCrash await just above would
567+
// still reach that listener with the handle live, so it's cleared here
568+
// too to close that earlier window.
544569
clearActiveRun();
545570
clearActiveDisposeHost();
546571
await flushPartialOnCrash().catch((flushErr: unknown) => {
@@ -551,7 +576,7 @@ export async function runTUI(initialConfig: Config): Promise<number> {
551576
process.stderr.write(`${COMMAND_NAME}: crash finalize partial flush failed: ${flushMessage}\n`);
552577
});
553578
const message = err instanceof Error ? err.message : String(err);
554-
await saveState(config.cwd, sessionId, {
579+
await finalizeRunState(config.cwd, sessionId, {
555580
status: "failed",
556581
turnsUsed: 0,
557582
task: runTaskTitle.trim().length > 0 ? runTaskTitle.trim() : "(conversation)",
@@ -1361,6 +1386,7 @@ export async function runTUI(initialConfig: Config): Promise<number> {
13611386
const writeRunSnapshot = async (
13621387
status: RunState["status"],
13631388
extra?: Pick<RunState, "finishedAt" | "error">,
1389+
kind: SnapshotKind = "progress",
13641390
): Promise<void> => {
13651391
const task = runTaskTitle.trim().length > 0 ? runTaskTitle.trim() : "(conversation)";
13661392
const model = `${liveSource.id}:${liveSource.model}`;
@@ -1369,28 +1395,38 @@ export async function runTUI(initialConfig: Config): Promise<number> {
13691395
activeRunHandle.task = task;
13701396
activeRunHandle.startedAt = startedAt;
13711397
activeRunHandle.model = model;
1372-
await saveState(config.cwd, sessionId, {
1398+
const state: RunState = {
13731399
status,
13741400
turnsUsed: runSink.getTurnCount(),
13751401
task,
13761402
startedAt,
13771403
model,
13781404
mcpServers: connectedMcpServers,
13791405
...extra,
1380-
});
1406+
};
1407+
if (clearsActiveRun(kind)) {
1408+
await finalizeRunState(config.cwd, sessionId, state);
1409+
} else {
1410+
await saveState(config.cwd, sessionId, state);
1411+
}
13811412
};
13821413

13831414
// Progress snapshots are fired unsequenced (model switch, MCP connect, turn
13841415
// completion), so a straggler could otherwise land after the terminal write
13851416
// and resurrect status "running" — atomicWrite is last-rename-wins. Once the
1386-
// run is finalized, drop them; the terminal paths write through
1417+
// run is finalized, drop them; the run-ending path writes through
13871418
// writeRunSnapshot directly.
1419+
//
1420+
// Never a "run-end" write: everything routed here happens while the process
1421+
// is still alive and must stay crash-coverable, including the rotation
1422+
// "done" that closes out a session on /clear or /new.
13881423
const persistRunSnapshot = async (
13891424
status: RunState["status"],
13901425
extra?: Pick<RunState, "finishedAt" | "error">,
1426+
kind: Exclude<SnapshotKind, "run-end"> = "progress",
13911427
): Promise<void> => {
13921428
if (finalized) return;
1393-
await writeRunSnapshot(status, extra);
1429+
await writeRunSnapshot(status, extra, kind);
13941430
};
13951431

13961432
// Cycles persist to the context store only on inference.done; the recorder
@@ -1629,8 +1665,10 @@ export async function runTUI(initialConfig: Config): Promise<number> {
16291665
error: err instanceof Error ? err.message : String(err),
16301666
});
16311667
});
1632-
await persistRunSnapshot("done", { finishedAt: Date.now() });
1668+
await persistRunSnapshot("done", { finishedAt: Date.now() }, "session-rotation");
16331669
sessionId = generateSessionId();
1670+
// Repointed, not cleared: the process lives on, so the crash handler
1671+
// must keep finding this handle and close out the *new* session.
16341672
activeRunHandle.sessionId = sessionId;
16351673
startedAt = Date.now();
16361674
runTaskTitle = config.task;
@@ -2308,13 +2346,22 @@ export async function runTUI(initialConfig: Config): Promise<number> {
23082346
// finished run (finishedAt set) can be left reading as still in progress.
23092347
const persistedStatus: RunState["status"] = summaryStatus;
23102348
finalized = true;
2311-
activeRunHandle.active = false;
2312-
clearActiveRun();
2349+
// The run itself is over here, so this write clears the active-run handle
2350+
// (via finalizeRunState in state.ts) in the same call, rather than pairing
2351+
// the on-disk write with a separate in-memory statement at this call site.
2352+
// The dispose host has no on-disk counterpart to piggyback on, so it still
2353+
// needs its own clear here, mirroring finalizeOnCrash — otherwise a signal
2354+
// arriving after this normal exit would find a handle pointing at a
2355+
// torn-down closure.
23132356
clearActiveDisposeHost();
2314-
await writeRunSnapshot(persistedStatus, {
2315-
finishedAt,
2316-
...(sinkError !== undefined ? { error: sinkError } : {}),
2317-
});
2357+
await writeRunSnapshot(
2358+
persistedStatus,
2359+
{
2360+
finishedAt,
2361+
...(sinkError !== undefined ? { error: sinkError } : {}),
2362+
},
2363+
"run-end",
2364+
);
23182365
const runSummary = createRunSummary({
23192366
task: runTaskTitle.length > 0 ? runTaskTitle : config.task,
23202367
status: summaryStatus,

0 commit comments

Comments
 (0)