From 50ca55fdc8c9a26b23beb2cda2290a49aa739f4c Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 00:19:10 -0700 Subject: [PATCH 1/3] Finalize run.json when the process crashes Process-level uncaughtException/unhandledRejection handlers wrote a crash report but left run.json stuck at status: running, so crashed sessions kept reappearing in the resume picker as in-progress. runTUI now registers a narrow handle (session id, cwd, active flag) in a module-level slot the moment a run starts, clearing it on any finalize path it already owns. The top-level crash handler reads that slot and writes status: crashed plus finishedAt through a new saveCrashState that bypasses the per-session write chain entirely, so a write that never settles can't block process.exit. --- src/index.ts | 59 ++++++++++++++++++---- src/session/active-run.ts | 23 +++++++++ src/session/state.ts | 18 ++++++- src/tui/runner.ts | 15 ++++++ tests/fixtures/crash-run/simulate-crash.ts | 31 ++++++++++++ tests/integration/crash-finalize.test.ts | 48 ++++++++++++++++++ 6 files changed, 183 insertions(+), 11 deletions(-) create mode 100644 src/session/active-run.ts create mode 100644 tests/fixtures/crash-run/simulate-crash.ts create mode 100644 tests/integration/crash-finalize.test.ts diff --git a/src/index.ts b/src/index.ts index 58f5cddca..0dabe2a57 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,6 +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 { loadConfig } from "./config/index.js"; import { ensureTelemetrySettings, globalSettingsPath } from "./config/settings.js"; import { installFileLogSink } from "./logging/sink.js"; @@ -113,7 +115,9 @@ export async function main(argv: readonly string[]): Promise { }); } -async function handleFatal(kind: CrashKind, error: unknown): Promise { +// 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 { process.stderr.write(`${kind}: ${error instanceof Error ? (error.stack ?? error.message) : String(error)}\n`); const file = await writeCrashReport(kind, error); if (file !== null) { @@ -121,24 +125,59 @@ async function handleFatal(kind: CrashKind, error: unknown): Promise { } else { process.stderr.write("failed to write crash report\n"); } + await finalizeActiveRunOnCrash(error); process.exit(1); } -if (import.meta.main) { - // OpenTUI installs a process-global uncaughtException/unhandledRejection - // handler that only logs (opentui/core's Renderer.handleError), which - // suppresses Bun's default print-and-exit. Combined with raw-mode stdin - // holding the event loop open, an escaped throw would otherwise hang the - // process forever with the terminal still in the alternate screen. Node - // invokes every registered listener for the event regardless of order, so - // these still run and terminate the process even though OpenTUI's own - // listener never exits or rethrows. +// 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. +async function finalizeActiveRunOnCrash(error: unknown): Promise { + 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(), + finishedAt: Date.now(), + error: message, + ...(prior?.model !== undefined ? { model: prior.model } : {}), + ...(prior?.mcpServers !== undefined ? { mcpServers: prior.mcpServers } : {}), + }); + } catch (saveErr: unknown) { + process.stderr.write( + `failed to finalize run state after crash: ${saveErr instanceof Error ? saveErr.message : String(saveErr)}\n`, + ); + } +} + +// OpenTUI installs a process-global uncaughtException/unhandledRejection +// handler that only logs (opentui/core's Renderer.handleError), which +// suppresses Bun's default print-and-exit. Combined with raw-mode stdin +// holding the event loop open, an escaped throw would otherwise hang the +// process forever with the terminal still in the alternate screen. Node +// invokes every registered listener for the event regardless of order, so +// these still run and terminate the process even though OpenTUI's own +// listener never exits or rethrows. +export function installCrashHandlers(): void { process.on("uncaughtException", (err) => { void handleFatal("uncaughtException", err); }); process.on("unhandledRejection", (reason) => { void handleFatal("unhandledRejection", reason); }); +} + +if (import.meta.main) { + installCrashHandlers(); let code: number; try { diff --git a/src/session/active-run.ts b/src/session/active-run.ts new file mode 100644 index 000000000..8e0679421 --- /dev/null +++ b/src/session/active-run.ts @@ -0,0 +1,23 @@ +// A module-level slot the top-level uncaughtException/unhandledRejection +// 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. +export type RunStateHandle = { + sessionId: string; + cwd: string; + active: boolean; +}; + +let activeRun: RunStateHandle | null = null; + +export function setActiveRun(handle: RunStateHandle): void { + activeRun = handle; +} + +export function clearActiveRun(): void { + activeRun = null; +} + +export function getActiveRun(): RunStateHandle | null { + return activeRun; +} diff --git a/src/session/state.ts b/src/session/state.ts index d9cc2ce7e..8cfa2d80b 100644 --- a/src/session/state.ts +++ b/src/session/state.ts @@ -14,7 +14,7 @@ const ConnectedMcpServerSchema = type({ export type ConnectedMcpServer = typeof ConnectedMcpServerSchema.infer; const RunStateSchema = type({ - status: "'running' | 'done' | 'failed' | 'cancelled'", + status: "'running' | 'done' | 'failed' | 'cancelled' | 'crashed'", turnsUsed: "number", task: "string", startedAt: "number", @@ -92,6 +92,22 @@ export async function saveState( } +// Crash-time terminal write. Deliberately bypasses writeChains: a hung or +// 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. +export async function saveCrashState( + cwd: string, + sessionId: string, + state: RunState, + home?: string, +): Promise { + const path = statePath(cwd, sessionId, home); + await atomicWrite(path, JSON.stringify(state, null, 2)); +} + // Returns the parsed state, or the arktype error summary when the shape is // invalid, so callers can surface a specific reason rather than "invalid shape". function parseRunState(data: unknown): RunState | { error: string } { diff --git a/src/tui/runner.ts b/src/tui/runner.ts index cf76dd692..88f83601a 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -163,6 +163,7 @@ import { createRunSink } from "../session/run-sink.js"; import { generateSessionId, initSessionDir, renameSession, sessionContextDir, sessionDir } from "../session/index.js"; import { resolveSessionLabel, truncateSessionLabel } from "../session/session-label.js"; import { loadState, saveState, type ConnectedMcpServer, type RunState } from "../session/state.js"; +import { setActiveRun, clearActiveRun, type RunStateHandle } from "../session/active-run.js"; import { openInBrowser } from "../auth/oauth/browser.js"; import { pickSession } from "./pick-session.js"; import { RESUME_TRANSCRIPT_BLOCK_LIMIT, turnsToContentBlocks } from "./turns-to-blocks.js"; @@ -470,6 +471,15 @@ export async function runTUI(initialConfig: Config): Promise { mcpServers: resumeSeed.mcpServers, }); + // Registered the moment a run starts so the top-level uncaughtException / + // unhandledRejection handler in index.ts (which cannot see any local state + // in this function) can finalize run.json for crashes that escape without + // 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 }; + setActiveRun(activeRunHandle); + // Crash guard: if anything from setup onward throws all the way out of // runTUI instead of reaching the normal finalize block, this still closes // out run.json so status and finishedAt never disagree. Declared before the @@ -496,6 +506,8 @@ export async function runTUI(initialConfig: Config): Promise { const finalizeOnCrash = async (err: unknown): Promise => { if (finalized) return; finalized = true; + activeRunHandle.active = false; + clearActiveRun(); await flushPartialOnCrash().catch((flushErr: unknown) => { // Best-effort only — still attempt saveState below. Log so a flush // failure is not invisible when diagnosing a crash exit. @@ -1640,6 +1652,7 @@ export async function runTUI(initialConfig: Config): Promise { }); await persistRunSnapshot("done", { finishedAt: Date.now() }); sessionId = generateSessionId(); + activeRunHandle.sessionId = sessionId; startedAt = Date.now(); runTaskTitle = config.task; emitter.emit("session.title", runTaskTitle.trim().length > 0 ? truncateSessionLabel(runTaskTitle) : "Untitled session"); @@ -2254,6 +2267,8 @@ export async function runTUI(initialConfig: Config): Promise { // finished run (finishedAt set) can be left reading as still in progress. const persistedStatus: RunState["status"] = summaryStatus; finalized = true; + activeRunHandle.active = false; + clearActiveRun(); await writeRunSnapshot(persistedStatus, { finishedAt, ...(sinkError !== undefined ? { error: sinkError } : {}), diff --git a/tests/fixtures/crash-run/simulate-crash.ts b/tests/fixtures/crash-run/simulate-crash.ts new file mode 100644 index 000000000..a1edcbfb2 --- /dev/null +++ b/tests/fixtures/crash-run/simulate-crash.ts @@ -0,0 +1,31 @@ +// Spawned as a subprocess by tests/integration/crash-finalize.test.ts. Mimics +// what runTUI does at startup (register the active run, write the initial +// "running" run.json) and what index.ts does at process entry (install the +// crash handlers), then throws asynchronously so it surfaces as a genuine +// uncaughtException rather than a synchronous throw the caller could catch. +import { installCrashHandlers } from "../../../src/index.js"; +import { setActiveRun } from "../../../src/session/active-run.js"; +import { sessionDir } from "../../../src/session/index.js"; +import { saveState } from "../../../src/session/state.js"; + +const cwd = process.cwd(); +const sessionId = process.env["CRASH_TEST_SESSION_ID"]; +if (sessionId === undefined) { + throw new Error("CRASH_TEST_SESSION_ID must be set"); +} + +await saveState(cwd, sessionId, { + status: "running", + turnsUsed: 3, + task: "simulated crash task", + startedAt: Date.now(), +}); + +setActiveRun({ sessionId, cwd, active: true }); +installCrashHandlers(); + +process.stdout.write(`${sessionDir(cwd, sessionId)}\n`); + +setImmediate(() => { + throw new Error("simulated crash"); +}); diff --git a/tests/integration/crash-finalize.test.ts b/tests/integration/crash-finalize.test.ts new file mode 100644 index 000000000..444652a04 --- /dev/null +++ b/tests/integration/crash-finalize.test.ts @@ -0,0 +1,48 @@ +import { mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { describe, expect, test } from "bun:test"; + +import { generateSessionId } from "../../src/session/index.js"; +import type { RunState } from "../../src/session/state.js"; +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 () => { + const cwd = mkdtempSync(join(tmpdir(), "corbits-crash-cwd-")); + const home = mkdtempSync(join(tmpdir(), "corbits-crash-home-")); + const sessionId = generateSessionId(); + + try { + const proc = Bun.spawn(["bun", "run", FIXTURE], { + cwd, + env: { ...process.env, HOME: home, CRASH_TEST_SESSION_ID: sessionId }, + stdout: "pipe", + stderr: "pipe", + }); + + const exitCode = await proc.exited; + const stdout = await new Response(proc.stdout).text(); + const stderr = await new Response(proc.stderr).text(); + + expect(exitCode).toBe(1); + expect(stderr).toContain("uncaughtException: Error: simulated crash"); + + const runJsonPath = join(stdout.trim(), "run.json"); + const raw = readFileSync(runJsonPath, "utf8"); + const state = JSON.parse(raw) as RunState; + + expect(state.status).toBe("crashed"); + expect(state.finishedAt).toBeGreaterThan(0); + expect(state.error).toContain("simulated crash"); + expect(state.task).toBe("simulated crash task"); + expect(isResumableByDefault(state)).toBe(false); + } finally { + rmSync(cwd, { recursive: true, force: true }); + rmSync(home, { recursive: true, force: true }); + } + }, 15_000); +}); From f90129508d8908f294905cdc45fa76e3277fb46a Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 00:32:29 -0700 Subject: [PATCH 2/3] 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. --- src/index.ts | 34 +++++++++++++--------- src/session/active-run.ts | 28 ++++++++++++++++++ src/session/state.ts | 22 +++++++++++--- src/tui/runner.ts | 20 +++++++++++-- tests/fixtures/crash-run/simulate-crash.ts | 28 ++++++++++++++++-- tests/integration/crash-finalize.test.ts | 7 ++++- 6 files changed, 115 insertions(+), 24 deletions(-) diff --git a/src/index.ts b/src/index.ts index 0dabe2a57..0ce70559e 100644 --- a/src/index.ts +++ b/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 { // 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 { + // 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 { 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( diff --git a/src/session/active-run.ts b/src/session/active-run.ts index 8e0679421..f929310dd 100644 --- a/src/session/active-run.ts +++ b/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; +} diff --git a/src/session/state.ts b/src/session/state.ts index 8cfa2d80b..bf63dcf93 100644 --- a/src/session/state.ts +++ b/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>(); +// 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 { + 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, diff --git a/src/tui/runner.ts b/src/tui/runner.ts index 88f83601a..4fe5821c9 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -477,7 +477,14 @@ export async function runTUI(initialConfig: Config): Promise { // 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 { status: RunState["status"], extra?: Pick, ): Promise => { + 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, }); diff --git a/tests/fixtures/crash-run/simulate-crash.ts b/tests/fixtures/crash-run/simulate-crash.ts index a1edcbfb2..b62b18330 100644 --- a/tests/fixtures/crash-run/simulate-crash.ts +++ b/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"); }); diff --git a/tests/integration/crash-finalize.test.ts b/tests/integration/crash-finalize.test.ts index 444652a04..f3ed9e635 100644 --- a/tests/integration/crash-finalize.test.ts +++ b/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 }); From 13584dd7ac385b0efbb1e1e7665d3b8dc9a8d98f Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 00:48:31 -0700 Subject: [PATCH 3/3] Make the crash write-chain race test deterministic Firing 50 unawaited writes and hoping enough were still queued when isCrashed() flipped was a coin flip in practice (5/13 passed with the guard removed, when it should fail every time). A test-only write gate in active-run.ts now lets the fixture park writes before they reach the isCrashed() check and release them only after the crash handler has flipped the flag, so the ordering is controlled instead of hoped for. 25/25 passes with the guard in place; removing the guard reliably lets the parked writes win the race again. --- src/session/active-run.ts | 15 +++++++ src/session/state.ts | 6 ++- tests/fixtures/crash-run/simulate-crash.ts | 46 ++++++++++++++-------- tests/integration/crash-finalize.test.ts | 11 ++++-- 4 files changed, 56 insertions(+), 22 deletions(-) diff --git a/src/session/active-run.ts b/src/session/active-run.ts index f929310dd..749dd4162 100644 --- a/src/session/active-run.ts +++ b/src/session/active-run.ts @@ -49,3 +49,18 @@ export function markCrashed(): void { export function isCrashed(): boolean { return crashed; } + +// Test-only seam: lets an integration test hold a chained write open past the +// moment markCrashed() fires, so it can deterministically prove a write still +// queued in the chain sees isCrashed() before it fires — rather than hoping +// real filesystem timing happens to interleave that way. No effect on +// production callers, which never install a gate. +let testWriteGate: Promise | null = null; + +export function setTestWriteGate(gate: Promise | null): void { + testWriteGate = gate; +} + +export function getTestWriteGate(): Promise | null { + return testWriteGate; +} diff --git a/src/session/state.ts b/src/session/state.ts index bf63dcf93..545be6a82 100644 --- a/src/session/state.ts +++ b/src/session/state.ts @@ -4,7 +4,7 @@ import { dirname, join } from "node:path"; import { type } from "arktype"; import { sessionDir } from "./index.js"; -import { isCrashed } from "./active-run.js"; +import { getTestWriteGate, isCrashed } from "./active-run.js"; import { COMMAND_NAME } from "../branding.js"; const ConnectedMcpServerSchema = type({ @@ -74,6 +74,10 @@ const writeChains = new Map>(); // 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 { + // No-op in production; lets a test hold this write open past the moment + // isCrashed() flips, so the check below is proven rather than assumed. + const gate = getTestWriteGate(); + if (gate !== null) await gate; if (isCrashed()) return; await atomicWrite(path, content); } diff --git a/tests/fixtures/crash-run/simulate-crash.ts b/tests/fixtures/crash-run/simulate-crash.ts index b62b18330..00ecb52b9 100644 --- a/tests/fixtures/crash-run/simulate-crash.ts +++ b/tests/fixtures/crash-run/simulate-crash.ts @@ -4,7 +4,7 @@ // crash handlers), then throws asynchronously so it surfaces as a genuine // uncaughtException rather than a synchronous throw the caller could catch. import { installCrashHandlers } from "../../../src/index.js"; -import { setActiveRun } from "../../../src/session/active-run.js"; +import { setActiveRun, setTestWriteGate } from "../../../src/session/active-run.js"; import { sessionDir } from "../../../src/session/index.js"; import { saveState } from "../../../src/session/state.js"; @@ -31,23 +31,35 @@ 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, - }); -} +// Hold every write issued from here on at the gate, before it reaches +// isCrashed(). This makes the race deterministic instead of hoping real +// filesystem timing interleaves the right way: the two straggler writes +// below are guaranteed to still be queued, not dispatched to the kernel, +// when the crash handler flips isCrashed() — the exact scenario the guard +// exists for. +let releaseGate: () => void; +const gate = new Promise((resolve) => { + releaseGate = resolve; +}); +setTestWriteGate(gate); + +// Two unawaited straggler snapshot writes, chained behind each other in +// state.ts's per-session queue — what persistRunSnapshot fires on every +// turn/model-switch/MCP-connect event. Both are parked at the gate. +void saveState(cwd, sessionId, { status: "running", turnsUsed: 1, task, startedAt, model }); +void saveState(cwd, sessionId, { status: "running", turnsUsed: 2, task, startedAt, model }); +// Throws inside setImmediate so it surfaces as a real uncaughtException. +// Node/Bun run the exception's own uncaughtException dispatch — including +// handleFatal's synchronous markCrashed() call, which precedes its first +// await — to completion before the event loop reaches the next queued +// setImmediate callback. The second setImmediate below is therefore +// guaranteed to run after isCrashed() has flipped to true, so releasing the +// gate there always lets the two parked writes observe the flag rather than +// racing it. setImmediate(() => { throw new Error("simulated crash"); }); +setImmediate(() => { + releaseGate(); +}); diff --git a/tests/integration/crash-finalize.test.ts b/tests/integration/crash-finalize.test.ts index f3ed9e635..5e85432b9 100644 --- a/tests/integration/crash-finalize.test.ts +++ b/tests/integration/crash-finalize.test.ts @@ -35,10 +35,13 @@ 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". + // The fixture also parks two unawaited straggler "running" snapshot + // writes behind a test-only gate (setTestWriteGate) that it releases + // only after the crash handler has flipped isCrashed(), guaranteeing + // both are still queued — not dispatched to the kernel — at that + // moment. Without the isCrashed() guard in saveState + // (src/session/state.ts), one of those would win the rename() race + // once released and this would read back "running". expect(state.status).toBe("crashed"); expect(state.finishedAt).toBeGreaterThan(0); expect(state.error).toContain("simulated crash");