Merge pull request #1478 from corbitsdev/cl-10757-session-persistence-hardening

feat(session): gate saveCrashState on crashed state

Author: TheGreatAxios

src/session/active-run.ts

@@ -84,6 +84,14 @@ export function isCrashed(): boolean {
   return crashed;
 }
 
+// Test-only seam: the crashed flag is process-global and sticky by design, so
+// a test that flips it must restore the not-crashed state — otherwise every
+// later saveState chain in the test process would no-op. Follows the
+// setTestWriteGate precedent below.
+export function resetCrashedForTests(): void {
+  crashed = false;
+}
+
 // Test-only seam: lets an integration test hold a chained write open past
 // markCrashed(), so it can deterministically prove a write still queued in
 // the chain sees isCrashed() before it fires — rather than hoping filesystem

src/session/state.test.ts

@@ -21,8 +21,10 @@ await withMockedModule(
   }),
 );
 
-const { finalizeRunState, loadState, saveState } = await import("./state.js");
-const { getActiveRun, setActiveRun } = await import("./active-run.js");
+const { finalizeRunState, loadState, saveCrashState, saveState } =
+  await import("./state.js");
+const { getActiveRun, markCrashed, resetCrashedForTests, setActiveRun } =
+  await import("./active-run.js");
 type RunState = import("./state.js").RunState;
 
 let cwd = "";
@@ -105,3 +107,34 @@ test("saveState calls for different sessions do not block each other", async ()
   expect(a).toMatchObject({ kind: "ok", state: { task: "a" } });
   expect(b).toMatchObject({ kind: "ok", state: { task: "b" } });
 });
+
+test("saveCrashState fails closed without the crash fence and lands nothing", async () => {
+  const sessionId = "sess-gated";
+  await expect(
+    saveCrashState(cwd, sessionId, state({ status: "crashed" }), home),
+  ).rejects.toThrow("not in crashed state");
+  expect(await loadState(cwd, sessionId, home)).toMatchObject({
+    kind: "missing",
+  });
+});
+
+test("saveCrashState lands the terminal record once crashed", async () => {
+  const sessionId = "sess-crash-lands";
+  markCrashed();
+  try {
+    await saveCrashState(
+      cwd,
+      sessionId,
+      state({ status: "crashed", finishedAt: 7, error: "boom" }),
+      home,
+    );
+  } finally {
+    // The flag is process-global and sticky: restore it so later tests in
+    // this process keep their saveState chains live.
+    resetCrashedForTests();
+  }
+  expect(await loadState(cwd, sessionId, home)).toMatchObject({
+    kind: "ok",
+    state: { status: "crashed", finishedAt: 7, error: "boom" },
+  });
+});

src/session/state.ts

@@ -205,9 +205,11 @@ export async function finalizeRunState(
 }
 
 // Crash-time terminal write that bypasses writeChains: awaiting a hung queued
-// write would block the crash handler's process.exit. Callers must call
-// markCrashed() (src/session/active-run.ts) first so queued snapshot writes
-// step aside instead of racing this rename().
+// write would block the crash handler's process.exit. The crash fence is
+// enforced here, not left to caller discipline: both production callers
+// (process-handlers.ts handleFatal and the signal handlers) flip it via
+// markCrashed() before finalizing, and any other caller fails closed instead
+// of landing a terminal record that queued snapshot writes may then clobber.
 //
 // Second terminal path, separate on purpose: only index.ts's process-level
 // uncaughtException/unhandledRejection and signal handlers reach it (a crash
@@ -219,6 +221,11 @@ export async function saveCrashState(
   state: RunState,
   home?: string,
 ): Promise<void> {
+  if (!isCrashed()) {
+    throw new Error(
+      `saveCrashState refused for session ${sessionId}: not in crashed state (call markCrashed() first)`,
+    );
+  }
   const path = statePath(cwd, sessionId, home);
   await atomicWrite(path, JSON.stringify(state, null, 2));
 }