fix: serialize mutating routes behind an in-process mutex (#60)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: ymansurozer

src/mutex.test.ts

@@ -0,0 +1,41 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { createSerializer } from "./mutex.js";
+
+test("serialize runs tasks in enqueue order — a later task waits for an earlier slow one", async () => {
+  const serialize = createSerializer();
+  const order: string[] = [];
+  // Enqueued synchronously, so enqueue order is guaranteed. The first task is deliberately slow;
+  // the second resolves immediately. Without serialization the fast one would finish first — the
+  // mutex forces completion order to match enqueue order regardless of duration.
+  const slow = serialize(async () => {
+    await new Promise((r) => setTimeout(r, 30));
+    order.push("slow");
+    return "slow";
+  });
+  const fast = serialize(async () => {
+    order.push("fast");
+    return "fast";
+  });
+  const results = await Promise.all([slow, fast]);
+  assert.deepEqual(order, ["slow", "fast"]);
+  assert.deepEqual(results, ["slow", "fast"]);
+});
+
+test("a rejecting task does not poison the chain, and its rejection reaches the caller", async () => {
+  const serialize = createSerializer();
+  const order: string[] = [];
+  const boom = serialize(async () => {
+    order.push("boom");
+    throw new Error("boom");
+  });
+  const after = serialize(async () => {
+    order.push("after");
+    return "ok";
+  });
+  // The failing task's own promise rejects (the caller sees its error)…
+  await assert.rejects(boom, /boom/);
+  // …but the chain keeps flowing — the next task still runs to completion, in order.
+  assert.equal(await after, "ok");
+  assert.deepEqual(order, ["boom", "after"]);
+});

src/mutex.ts

@@ -0,0 +1,21 @@
+// A minimal in-process serialization queue: a promise-chain mutex. `serialize(fn)` runs `fn`
+// only after every previously enqueued task has settled, so tasks execute one at a time in
+// enqueue order. Enqueue order (not completion order) is what determines the run order, so the
+// caller must enqueue synchronously to get a guaranteed sequence.
+//
+// A rejected task settles the chain WITHOUT poisoning it: the internal chain swallows the error
+// (`.then(undefined-ish, undefined-ish)`), so the next task still runs, while the promise handed
+// back to the caller still rejects — the caller sees its own failure.
+export type Serializer = <T>(fn: () => Promise<T>) => Promise<T>;
+
+export function createSerializer(): Serializer {
+  let chain: Promise<unknown> = Promise.resolve();
+  return <T>(fn: () => Promise<T>): Promise<T> => {
+    const next = chain.then(fn, fn);
+    chain = next.then(
+      () => undefined,
+      () => undefined,
+    );
+    return next;
+  };
+}

src/server.test.ts

@@ -6,7 +6,7 @@ import http from "node:http";
 import { tmpdir } from "node:os";
 import path from "node:path";
 import { startServer } from "./server.js";
-import { buildReviewState, writeGlobalSettings } from "./state.js";
+import { buildReviewState, hash, writeGlobalSettings } from "./state.js";
 import type { ReviewState } from "./types.js";
 
 function state(root: string): ReviewState {
@@ -742,6 +742,68 @@ test("origin guard accepts a same-origin POST and a POST with no Origin", async
   });
 });
 
+test("concurrent /api/reload and /api/save leave the desk internally consistent (issue 05)", async () => {
+  const root = await mkdtemp(path.join(tmpdir(), "galley-mutex-"));
+  const oldHome = process.env.HOME;
+  process.env.HOME = root;
+  const g = (args: string[]) => execFileSync("git", args, { cwd: root }).toString();
+  g(["init", "-q"]);
+  g(["config", "user.email", "t@t.co"]);
+  g(["config", "user.name", "tester"]);
+  await writeFile(path.join(root, "a.ts"), "one\ntwo\nthree\n");
+  g(["add", "."]);
+  g(["commit", "-qm", "init"]);
+  // A real working-tree diff so /api/reload does its full (git-bound) rebuild each round.
+  await writeFile(path.join(root, "a.ts"), "one\nCHANGED\nthree\n");
+  const st = await buildReviewState(root, { session: "s" });
+  assert.ok(st, "built a review state for the working diff");
+  const handle = await startServer({ state: st!, open: false, idleTimeoutMs: 0 });
+  try {
+    // Fire the two mutating routes concurrently, many rounds. The queue's FIFO/non-poisoning
+    // contract is unit-tested in mutex.test.ts; here we assert the end-to-end invariant it exists
+    // to protect — that a reload's Object.assign never lands mid-save to stitch a half-applied
+    // snapshot. After every round the desk stays internally consistent: baseDiffHash is exactly
+    // the hash of the rawDiff it reports, never a value carried over from a different reload's diff.
+    for (let i = 0; i < 20; i++) {
+      const [reloadRes, saveRes] = await Promise.all([
+        post(handle.url, "api/reload", {}),
+        post(handle.url, "api/save", { reviewedFiles: ["a.ts"] }),
+      ]);
+      assert.equal(reloadRes.status, 200, `reload ${i} ok`);
+      assert.equal(saveRes.status, 200, `save ${i} ok`);
+      const snap = await getState(handle.url);
+      assert.equal(
+        snap.baseDiffHash,
+        hash(snap.rawDiff),
+        `baseDiffHash matches rawDiff (round ${i})`,
+      );
+    }
+  } finally {
+    handle.server.close();
+    process.env.HOME = oldHome;
+    await rm(root, { recursive: true, force: true });
+  }
+});
+
+test("a throwing wrapped route settles the mutex without poisoning the chain (issue 05)", async () => {
+  await withServer(async (handle, _root, st) => {
+    // Invalid JSON reaches the serialized /api/save body, where JSON.parse throws — the wrapped
+    // fn rejects. The chain must swallow that rejection (not wedge behind a permanently-rejected
+    // promise) while the caller still sees the 500.
+    const bad = await fetch(`${handle.url}api/save`, {
+      method: "POST",
+      headers: { "content-type": "application/json" },
+      body: "{ not valid json",
+    });
+    assert.equal(bad.status, 500);
+    assert.equal(((await bad.json()) as { code?: string }).code, "INTERNAL");
+    // A following mutation still runs to completion — proof the queue kept flowing.
+    const ok = await post(handle.url, "api/save", { reviewedFiles: ["a.ts"] });
+    assert.equal(ok.status, 200);
+    assert.deepEqual(st.reviewedFiles, ["a.ts"]);
+  });
+});
+
 test("settings API round-trips editorCommand", async () => {
   await withServer(async (handle) => {
     await fetch(`${handle.url}api/settings`, {