Serialize run.json writes per session to survive stragglers

Concurrent saveState calls for one session had no ordering guarantee on
their underlying rename()s, so a late progress snapshot could land after
the terminal finalize write and flip a finished session back to
status: "running" with no finishedAt. Chain writes per sessionId so they
always apply in call order.

Documents why runner.ts's finalized flag still earns its place now that
saveState serializes writes per session: the write chain only orders
writes that are already issued, it has no way to know a stale
post-finalize snapshot shouldn't be issued at all.

Author: TheGreatAxios

src/session/state.test.ts

@@ -0,0 +1,76 @@
+import { afterEach, beforeEach, expect, mock, test } from "bun:test";
+import * as realFs from "node:fs/promises";
+import { mkdir, rm } from "node:fs/promises";
+import { join } from "node:path";
+import { tmpdir } from "node:os";
+
+// Simulates the straggler write's real await point (e.g. cycleRecorder.dispose
+// during the terminal path) landing its writeFile after a later-issued
+// terminal write's writeFile, so rename-order alone would let it win.
+const realWriteFile = realFs.writeFile;
+let delayNextWrite = false;
+mock.module("node:fs/promises", () => ({
+  ...realFs,
+  writeFile: async (path: string, data: string) => {
+    if (delayNextWrite) {
+      delayNextWrite = false;
+      await new Promise((resolve) => setTimeout(resolve, 30));
+    }
+    return realWriteFile(path, data);
+  },
+}));
+
+const { loadState, saveState } = await import("./state.js");
+type RunState = Awaited<ReturnType<typeof loadState>>;
+
+let cwd = "";
+let home = "";
+
+beforeEach(async () => {
+  const stamp = `${Date.now()}-${Math.random().toString(16).slice(2)}`;
+  cwd = join(tmpdir(), `corbits-state-${stamp}`);
+  home = join(tmpdir(), `corbits-state-home-${stamp}`);
+  await mkdir(cwd, { recursive: true });
+  await mkdir(home, { recursive: true });
+});
+
+afterEach(async () => {
+  await rm(cwd, { recursive: true, force: true });
+  await rm(home, { recursive: true, force: true });
+});
+
+function state(overrides: Partial<NonNullable<RunState>>): NonNullable<RunState> {
+  return {
+    status: "running",
+    turnsUsed: 0,
+    task: "task",
+    startedAt: 1,
+    ...overrides,
+  };
+}
+
+test("a straggler snapshot started before a terminal write does not overwrite it", async () => {
+  const sessionId = "sess-race";
+
+  delayNextWrite = true;
+  const straggler = saveState(cwd, sessionId, state({ status: "running" }), home);
+  const terminal = saveState(cwd, sessionId, state({ status: "done", finishedAt: 999 }), home);
+
+  await Promise.all([straggler, terminal]);
+
+  const final = await loadState(cwd, sessionId, home);
+  expect(final?.status).toBe("done");
+  expect(final?.finishedAt).toBe(999);
+});
+
+test("saveState calls for different sessions do not block each other", async () => {
+  await Promise.all([
+    saveState(cwd, "session-a", state({ task: "a" }), home),
+    saveState(cwd, "session-b", state({ task: "b" }), home),
+  ]);
+
+  const a = await loadState(cwd, "session-a", home);
+  const b = await loadState(cwd, "session-b", home);
+  expect(a?.task).toBe("a");
+  expect(b?.task).toBe("b");
+});

src/session/state.ts

@@ -55,13 +55,40 @@ export function warnUnreadableState(path: string, reason: string): void {
   process.stderr.write(`${COMMAND_NAME}: ignoring unreadable state at ${path} (${reason}); starting fresh\n`);
 }
 
+// Concurrent saveState calls for the same session (a straggler progress
+// snapshot racing a terminal finalize write) have no ordering guarantee
+// between their underlying rename()s — the later call could still finish
+// first and resurrect a closed run.json as "running". Chaining each session's
+// writes onto the previous one forces them to apply in call order, so a
+// write issued after another always lands after it regardless of how long
+// either write's fs calls take. Keyed by sessionId, not path, since callers
+// only ever address one file per session.
+const writeChains = new Map<string, Promise<void>>();
+
 export async function saveState(
   cwd: string,
   sessionId: string,
   state: RunState,
   home?: string,
 ): Promise<void> {
-  await atomicWrite(statePath(cwd, sessionId, home), JSON.stringify(state, null, 2));
+  const path = statePath(cwd, sessionId, home);
+  const content = JSON.stringify(state, null, 2);
+  const previous = writeChains.get(sessionId) ?? Promise.resolve();
+  const write = previous.then(
+    () => atomicWrite(path, content),
+    () => atomicWrite(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
+  // saves for the same session.
+  const tail = write.catch(() => {});
+  writeChains.set(sessionId, tail);
+  // Once this is the last write for the session, drop the entry so a
+  // long-lived process doesn't retain a chain per session forever.
+  void tail.then(() => {
+    if (writeChains.get(sessionId) === tail) writeChains.delete(sessionId);
+  });
+  return write;
 }
 
 

src/tui/runner.ts

@@ -447,8 +447,16 @@ export async function runTUI(initialConfig: Config): Promise<number> {
   // out run.json so status and finishedAt never disagree. Declared before the
   // try so every fallible step after the minimal write above is covered.
   // `finalized` is set by the normal finalize path so this never double-writes
-  // on a clean exit; it also gates straggler snapshot writes (see
-  // persistRunSnapshot) from resurrecting a closed record.
+  // on a clean exit. It also gates persistRunSnapshot (below) from *issuing*
+  // a straggler write at all once the run is closed — a different job from
+  // saveState's per-session write ordering in state.ts. That ordering only
+  // decides which already-issued write lands last; it has no way to know a
+  // "running" snapshot fired after finalize is stale and should never be
+  // written in the first place. Without this flag such a snapshot would
+  // still queue behind the terminal write and legitimately "win" the
+  // ordering, resurrecting a closed run.json. Two different constraints
+  // (don't issue a stale write vs. order the writes you do issue), each
+  // owned by its own layer — not a duplicate check.
   let finalized = false;
   // Bound after the cycle recorder exists (it needs the session workdir); the
   // crash guard is declared first so it covers every fallible step below.