diff --git a/src/index.ts b/src/index.ts index 58f5cddca..0ce70559e 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, 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"; @@ -113,7 +115,15 @@ 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 { + // 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) { @@ -121,24 +131,61 @@ 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, 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 { + const run = getActiveRun(); + if (run === null || !run.active) return; + const message = error instanceof Error ? error.message : String(error); + try { + await saveCrashState(run.cwd, run.sessionId, { + status: "crashed", + turnsUsed: 0, + task: run.task, + startedAt: run.startedAt, + finishedAt: Date.now(), + error: message, + ...(run.model !== undefined ? { model: run.model } : {}), + }); + } 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..749dd4162 --- /dev/null +++ b/src/session/active-run.ts @@ -0,0 +1,66 @@ +// 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. +// +// 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; + +export function setActiveRun(handle: RunStateHandle): void { + activeRun = handle; +} + +export function clearActiveRun(): void { + activeRun = null; +} + +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; +} + +// 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 d9cc2ce7e..545be6a82 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 { getTestWriteGate, isCrashed } from "./active-run.js"; import { COMMAND_NAME } from "../branding.js"; const ConnectedMcpServerSchema = type({ @@ -14,7 +15,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", @@ -65,6 +66,22 @@ 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 { + // 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); +} + export async function saveState( cwd: string, sessionId: string, @@ -75,8 +92,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 @@ -92,6 +109,23 @@ 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. +// 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, + 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..4fe5821c9 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,22 @@ 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, + 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 // 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 +513,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. @@ -1375,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, }); @@ -1640,6 +1666,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 +2281,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..00ecb52b9 --- /dev/null +++ b/tests/fixtures/crash-run/simulate-crash.ts @@ -0,0 +1,65 @@ +// 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, setTestWriteGate } 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"); +} + +const startedAt = Date.now(); +const task = "simulated crash task"; +const model = "test-provider:test-model"; + +await saveState(cwd, sessionId, { + status: "running", + turnsUsed: 3, + task, + startedAt, + model, +}); + +setActiveRun({ sessionId, cwd, active: true, task, startedAt, model }); +installCrashHandlers(); + +process.stdout.write(`${sessionDir(cwd, sessionId)}\n`); + +// 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 new file mode 100644 index 000000000..5e85432b9 --- /dev/null +++ b/tests/integration/crash-finalize.test.ts @@ -0,0 +1,56 @@ +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, 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(); + + 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; + + // 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"); + 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 }); + rmSync(home, { recursive: true, force: true }); + } + }, 15_000); +});