Remove unbounded read and write-chain race from crash finalize

The crash handler awaited loadState (a plain readFile) to recover
task/startedAt/model before writing, the same unbounded-I/O hazard
primeCrashReporting exists to avoid for git. active-run.ts now carries
those fields directly, updated by runTUI wherever it already tracks
them, so the handler needs no read.

Bypassing writeChains for the crash write also reopened the exact
race CL-5567 closed: an in-flight progress snapshot for the same
session could still land after the crash write and resurrect
status: running. saveState now checks a synchronous isCrashed() flag
right before each queued write fires, so anything still waiting in
the chain when the crash handler marks the process crashed steps
aside instead of racing it.

Author: TheGreatAxios

src/index.ts

@@ -1,8 +1,8 @@
 import { getLogger } from "@intx/log";
 import { LOG_NAMESPACE_ROOT } from "./branding.js";
 import { primeCrashReporting, writeCrashReport, type CrashKind } from "./crash/report.js";
-import { getActiveRun } from "./session/active-run.js";
-import { loadState, saveCrashState } from "./session/state.js";
+import { getActiveRun, markCrashed } from "./session/active-run.js";
+import { saveCrashState } from "./session/state.js";
 import { loadConfig } from "./config/index.js";
 import { ensureTelemetrySettings, globalSettingsPath } from "./config/settings.js";
 import { installFileLogSink } from "./logging/sink.js";
@@ -118,6 +118,12 @@ export async function main(argv: readonly string[]): Promise<number> {
 // Exported so an integration test can register these process-level handlers
 // and inject a crash without spawning the full TUI stack.
 export async function handleFatal(kind: CrashKind, error: unknown): Promise<void> {
+  // Flip this before any awaits below so any snapshot write still queued
+  // behind another one in state.ts's per-session chain sees it and steps
+  // aside the moment it's next in line, rather than racing saveCrashState's
+  // rename() below. See markCrashed's doc comment for the residual window
+  // this cannot close.
+  markCrashed();
   process.stderr.write(`${kind}: ${error instanceof Error ? (error.stack ?? error.message) : String(error)}\n`);
   const file = await writeCrashReport(kind, error);
   if (file !== null) {
@@ -131,26 +137,28 @@ export async function handleFatal(kind: CrashKind, error: unknown): Promise<void
 
 // A crash reaching here escaped without ever hitting runTUI's own try/catch
 // (e.g. a throw inside a fire-and-forget `void` call), so run.json was never
-// closed out. getActiveRun surfaces the in-flight session set by runTUI; the
-// write itself goes through saveCrashState, which bypasses the per-session
-// write chain in state.ts on purpose — chaining behind a write that never
-// settles (possibly the very write that triggered this crash) would block
-// process.exit indefinitely, defeating this handler's one job.
+// closed out. getActiveRun surfaces the in-flight session set by runTUI, with
+// enough (task, startedAt, model) carried on the handle itself that no read
+// of run.json is needed — a readFile here would be exactly the kind of
+// unbounded crash-path I/O primeCrashReporting (src/crash/report.ts) exists
+// to avoid for git: a stalled disk or network mount would block process.exit
+// forever. The write itself goes through saveCrashState, which bypasses the
+// per-session write chain in state.ts on purpose — chaining behind a write
+// that never settles (possibly the very write that triggered this crash)
+// would block process.exit indefinitely, defeating this handler's one job.
 async function finalizeActiveRunOnCrash(error: unknown): Promise<void> {
   const run = getActiveRun();
   if (run === null || !run.active) return;
   const message = error instanceof Error ? error.message : String(error);
   try {
-    const prior = await loadState(run.cwd, run.sessionId);
     await saveCrashState(run.cwd, run.sessionId, {
       status: "crashed",
-      turnsUsed: prior?.turnsUsed ?? 0,
-      task: prior?.task ?? "(conversation)",
-      startedAt: prior?.startedAt ?? Date.now(),
+      turnsUsed: 0,
+      task: run.task,
+      startedAt: run.startedAt,
       finishedAt: Date.now(),
       error: message,
-      ...(prior?.model !== undefined ? { model: prior.model } : {}),
-      ...(prior?.mcpServers !== undefined ? { mcpServers: prior.mcpServers } : {}),
+      ...(run.model !== undefined ? { model: run.model } : {}),
     });
   } catch (saveErr: unknown) {
     process.stderr.write(

src/session/active-run.ts

@@ -2,10 +2,20 @@
 // handler (src/index.ts) can reach even though persistRunSnapshot is a
 // closure local to runTUI. Only ever consulted from the crash path: a run
 // that never crashes never has this read.
+//
+// Carries enough of the live run state (task, startedAt, model) that the
+// crash handler can build a full RunState record itself. It must not read
+// run.json back off disk to fill these in — an unbounded readFile on the
+// crash path has the exact failure mode primeCrashReporting (src/crash/
+// report.ts) exists to avoid for git: a stalled disk or network mount would
+// block process.exit forever.
 export type RunStateHandle = {
   sessionId: string;
   cwd: string;
   active: boolean;
+  task: string;
+  startedAt: number;
+  model?: string;
 };
 
 let activeRun: RunStateHandle | null = null;
@@ -21,3 +31,21 @@ export function clearActiveRun(): void {
 export function getActiveRun(): RunStateHandle | null {
   return activeRun;
 }
+
+// Set once, by the crash handler, immediately before it writes the terminal
+// "crashed" record. saveState (src/session/state.ts) reads this synchronously
+// right before each queued write actually fires, so any snapshot write still
+// waiting behind another one in its per-session chain sees the flag and
+// no-ops instead of firing after (and clobbering) the crash write. It cannot
+// stop a write whose writeFile/rename has already been dispatched to the
+// kernel at the moment the flag flips — that window is one atomicWrite call
+// wide, not the full remaining lifetime of the process.
+let crashed = false;
+
+export function markCrashed(): void {
+  crashed = true;
+}
+
+export function isCrashed(): boolean {
+  return crashed;
+}

src/session/state.ts

@@ -4,6 +4,7 @@ import { dirname, join } from "node:path";
 import { type } from "arktype";
 
 import { sessionDir } from "./index.js";
+import { isCrashed } from "./active-run.js";
 import { COMMAND_NAME } from "../branding.js";
 
 const ConnectedMcpServerSchema = type({
@@ -65,6 +66,18 @@ export function warnUnreadableState(path: string, reason: string): void {
 // only ever address one file per session.
 const writeChains = new Map<string, Promise<void>>();
 
+// Checked right before a chained write actually fires (not at saveState()
+// call time) so a snapshot write still queued behind another one, at the
+// moment the crash handler flips this flag, sees it and no-ops instead of
+// landing after (and clobbering) the crash write issued via saveCrashState.
+// This cannot recall a write whose writeFile/rename has already been
+// dispatched to the kernel — that residual window is one atomicWrite call
+// wide (a small local JSON write), not the remaining lifetime of the process.
+async function atomicWriteUnlessCrashed(path: string, content: string): Promise<void> {
+  if (isCrashed()) return;
+  await atomicWrite(path, content);
+}
+
 export async function saveState(
   cwd: string,
   sessionId: string,
@@ -75,8 +88,8 @@ export async function saveState(
   const content = JSON.stringify(state, null, 2);
   const previous = writeChains.get(sessionId) ?? Promise.resolve();
   const write = previous.then(
-    () => atomicWrite(path, content),
-    () => atomicWrite(path, content),
+    () => atomicWriteUnlessCrashed(path, content),
+    () => atomicWriteUnlessCrashed(path, content),
   );
   // Swallow the error in the chain tail (not in `write`, which still rejects
   // for this caller) so one failed save doesn't permanently wedge later
@@ -96,8 +109,9 @@ export async function saveState(
 // still-pending write for this session (possibly the very write mid-flight
 // when the process crashed) must never be awaited here, or a queued write
 // that never settles would block the crash handler's process.exit forever.
-// There is no later write to order against once the process is exiting, so
-// per-session ordering has nothing left to protect.
+// Callers must call markCrashed() (src/session/active-run.ts) before this, so
+// any snapshot write still queued behind another one in the chain steps
+// aside instead of racing this write's rename().
 export async function saveCrashState(
   cwd: string,
   sessionId: string,

src/tui/runner.ts

@@ -477,7 +477,14 @@ export async function runTUI(initialConfig: Config): Promise<number> {
   // ever reaching this function's own try/catch — e.g. a throw inside a
   // fire-and-forget `void` call. Cleared wherever `finalized` below flips
   // true, since those paths already write a terminal run.json themselves.
-  const activeRunHandle: RunStateHandle = { sessionId, cwd: config.cwd, active: true };
+  const activeRunHandle: RunStateHandle = {
+    sessionId,
+    cwd: config.cwd,
+    active: true,
+    task: runTaskTitle.trim().length > 0 ? runTaskTitle.trim() : "(conversation)",
+    startedAt,
+    model: `${config.providerName}:${config.model}`,
+  };
   setActiveRun(activeRunHandle);
 
   // Crash guard: if anything from setup onward throws all the way out of
@@ -1387,12 +1394,19 @@ export async function runTUI(initialConfig: Config): Promise<number> {
     status: RunState["status"],
     extra?: Pick<RunState, "finishedAt" | "error">,
   ): Promise<void> => {
+    const task = runTaskTitle.trim().length > 0 ? runTaskTitle.trim() : "(conversation)";
+    const model = `${liveSource.id}:${liveSource.model}`;
+    // Kept in step with every persisted snapshot so the crash handler's copy
+    // (activeRunHandle, read by index.ts) never lags what's actually on disk.
+    activeRunHandle.task = task;
+    activeRunHandle.startedAt = startedAt;
+    activeRunHandle.model = model;
     await saveState(config.cwd, sessionId, {
       status,
       turnsUsed: runSink.getTurnCount(),
-      task: runTaskTitle.trim().length > 0 ? runTaskTitle.trim() : "(conversation)",
+      task,
       startedAt,
-      model: `${liveSource.id}:${liveSource.model}`,
+      model,
       mcpServers: connectedMcpServers,
       ...extra,
     });

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

@@ -14,18 +14,40 @@ if (sessionId === undefined) {
   throw new Error("CRASH_TEST_SESSION_ID must be set");
 }
 
+const startedAt = Date.now();
+const task = "simulated crash task";
+const model = "test-provider:test-model";
+
 await saveState(cwd, sessionId, {
   status: "running",
   turnsUsed: 3,
-  task: "simulated crash task",
-  startedAt: Date.now(),
+  task,
+  startedAt,
+  model,
 });
 
-setActiveRun({ sessionId, cwd, active: true });
+setActiveRun({ sessionId, cwd, active: true, task, startedAt, model });
 installCrashHandlers();
 
 process.stdout.write(`${sessionDir(cwd, sessionId)}\n`);
 
+// Queue a burst of unawaited straggler snapshot writes (what
+// persistRunSnapshot does on every turn/model-switch/MCP-connect event) right
+// before crashing. Each is chained onto the previous one in state.ts's
+// per-session write queue, so most of these are still waiting their turn —
+// not yet dispatched to the kernel — at the moment the crash handler flips
+// the isCrashed() flag. Without that guard, one of these landing after
+// saveCrashState's rename() would resurrect status: "running".
+for (let i = 0; i < 50; i++) {
+  void saveState(cwd, sessionId, {
+    status: "running",
+    turnsUsed: i,
+    task,
+    startedAt,
+    model,
+  });
+}
+
 setImmediate(() => {
   throw new Error("simulated crash");
 });

tests/integration/crash-finalize.test.ts

@@ -11,7 +11,7 @@ import { isResumableByDefault } from "../../src/tui/pick-session.js";
 const FIXTURE = join(import.meta.dirname, "../fixtures/crash-run/simulate-crash.ts");
 
 describe("integration — crash finalizes run.json", () => {
-  test("uncaughtException writes status: crashed with finishedAt", async () => {
+  test("uncaughtException writes status: crashed with finishedAt, racing in-flight snapshot writes", async () => {
     const cwd = mkdtempSync(join(tmpdir(), "corbits-crash-cwd-"));
     const home = mkdtempSync(join(tmpdir(), "corbits-crash-home-"));
     const sessionId = generateSessionId();
@@ -35,10 +35,15 @@ describe("integration — crash finalizes run.json", () => {
       const raw = readFileSync(runJsonPath, "utf8");
       const state = JSON.parse(raw) as RunState;
 
+      // The fixture also fires 50 unawaited straggler "running" snapshot
+      // writes for the same session immediately before crashing. Without the
+      // isCrashed() guard in saveState (src/session/state.ts), one of those
+      // could win the rename() race and this would read back "running".
       expect(state.status).toBe("crashed");
       expect(state.finishedAt).toBeGreaterThan(0);
       expect(state.error).toContain("simulated crash");
       expect(state.task).toBe("simulated crash task");
+      expect(state.model).toBe("test-provider:test-model");
       expect(isResumableByDefault(state)).toBe(false);
     } finally {
       rmSync(cwd, { recursive: true, force: true });