feat(lib): fs-atomic — one atomic-write implementation, with the race actually fixed

Atomic tmp-write-then-rename was reimplemented ~20 times across lib/, bin/,
and browse/src with three tmp-suffix conventions. One of them was a latent
bug this commit closes: lib/worktree.ts used a bare '.tmp' suffix — the
deterministic-tmp collision race browse/src/server.ts documents having hit
in production (its fix, pid+random, was trapped in a comment at one site).

lib/fs-atomic.ts: atomicWriteSync (always throws, best-effort tmp cleanup,
pid+random suffix, optional mode applied at tmp creation so the file never
exists with looser permissions) + atomicWriteQuiet (shutdown paths only).
Unit tests pin the throw/quiet contracts, 0600 mode, tmp-name uniqueness
(captured via the read-only-dir failure path — Bun's fs exports are
readonly, no monkeypatching), and no-stray-tmp cleanup.

Migrated: lib/worktree.ts (the bare-.tmp bug), lib/gstack-decision.ts
(snapshot + compact log), lib/gbrain-local-status.ts (probe cache). browse
sites follow separately.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

lib/fs-atomic.ts

@@ -0,0 +1,76 @@
+/**
+ * Atomic file writes — the ONE implementation of tmp-write-then-rename.
+ *
+ * Before this module, the pattern was reimplemented ~20 times across lib/,
+ * bin/, and browse/src with three different tmp-suffix conventions — one of
+ * which (a bare `.tmp`) carries a real collision race that browse's
+ * server.ts documented after hitting it in production: two writers (batch
+ * subcommands, /tunnel/start handlers, or any combination) collide on the
+ * rename when the tmp filename is deterministic. The suffix here includes
+ * pid AND a random component so concurrent writers in the SAME process
+ * (async interleavings) can't collide either.
+ *
+ * Contract:
+ * - atomicWriteSync ALWAYS throws on failure, after best-effort tmp cleanup.
+ *   Callers own the error. Use it everywhere except shutdown paths.
+ * - atomicWriteQuiet swallows everything (returns false on failure). ONLY
+ *   for shutdown/emergency-cleanup paths where a throw would abort the rest
+ *   of cleanup — same philosophy as browse's safeUnlinkQuiet.
+ * - `mode` applies to the tmp file at creation (0600 for sensitive state),
+ *   so the final file never exists with looser permissions.
+ * - The tmp file is created in the target's directory (same filesystem, so
+ *   rename stays atomic). Parent dirs are NOT created — callers that need
+ *   mkdir own that decision (and its mode).
+ */
+import * as fs from 'fs';
+import * as crypto from 'crypto';
+
+export interface AtomicWriteOpts {
+  /** File mode for the tmp file at creation (e.g. 0o600). Default: umask. */
+  mode?: number;
+}
+
+function tmpPathFor(target: string): string {
+  return `${target}.tmp.${process.pid}.${crypto.randomBytes(4).toString('hex')}`;
+}
+
+/** Atomic write. Throws on failure (after best-effort tmp cleanup). */
+export function atomicWriteSync(
+  target: string,
+  data: string | NodeJS.ArrayBufferView,
+  opts: AtomicWriteOpts = {},
+): void {
+  const tmp = tmpPathFor(target);
+  try {
+    if (opts.mode !== undefined) {
+      fs.writeFileSync(tmp, data, { mode: opts.mode });
+    } else {
+      fs.writeFileSync(tmp, data);
+    }
+    fs.renameSync(tmp, target);
+  } catch (err) {
+    try {
+      fs.unlinkSync(tmp);
+    } catch {
+      // Best-effort cleanup; the original error is the one that matters.
+    }
+    throw err;
+  }
+}
+
+/**
+ * Atomic write that swallows all errors. Returns true on success.
+ * ONLY for shutdown/emergency paths — a throw there aborts remaining cleanup.
+ */
+export function atomicWriteQuiet(
+  target: string,
+  data: string | NodeJS.ArrayBufferView,
+  opts: AtomicWriteOpts = {},
+): boolean {
+  try {
+    atomicWriteSync(target, data, opts);
+    return true;
+  } catch {
+    return false;
+  }
+}

lib/gbrain-local-status.ts

@@ -41,10 +41,9 @@ import {
   existsSync,
   mkdirSync,
   readFileSync,
-  renameSync,
   statSync,
-  writeFileSync,
 } from "fs";
+import { atomicWriteSync } from "./fs-atomic";
 import { homedir } from "os";
 import { dirname, join } from "path";
 import { buildGbrainEnv, NEEDS_SHELL_ON_WINDOWS } from "./gbrain-exec";
@@ -254,9 +253,7 @@ function writeCache(status: LocalEngineStatus, key: CacheEntry["key"]): void {
   };
   try {
     mkdirSync(dirname(cacheFilePath()), { recursive: true });
-    const tmp = cacheFilePath() + ".tmp." + process.pid;
-    writeFileSync(tmp, JSON.stringify(entry, null, 2), "utf-8");
-    renameSync(tmp, cacheFilePath());
+    atomicWriteSync(cacheFilePath(), JSON.stringify(entry, null, 2));
   } catch {
     // Cache write failure is non-fatal — we re-probe next call.
   }

lib/gstack-decision.ts

@@ -16,7 +16,8 @@
 import { join } from "path";
 import { homedir } from "os";
 import { randomUUID } from "crypto";
-import { writeFileSync, renameSync, existsSync, readFileSync, appendFileSync, statSync, openSync, closeSync, unlinkSync } from "fs";
+import { existsSync, readFileSync, appendFileSync, statSync, openSync, closeSync, unlinkSync } from "fs";
+import { atomicWriteSync } from "./fs-atomic";
 import { appendJsonl, readJsonl, hasInjection } from "./jsonl-store";
 import { scan } from "./redact-engine";
 
@@ -224,9 +225,7 @@ export function readEvents(paths: DecisionPaths): DecisionEvent[] {
  * O(active), not O(history).
  */
 export function writeSnapshot(paths: DecisionPaths, active: ActiveDecision[]): void {
-  const tmp = `${paths.snapshot}.tmp.${process.pid}`;
-  writeFileSync(tmp, JSON.stringify(active), "utf-8");
-  renameSync(tmp, paths.snapshot);
+  atomicWriteSync(paths.snapshot, JSON.stringify(active));
 }
 
 /** Read the bounded active snapshot. Returns [] if missing/corrupt (caller may rebuild). */
@@ -308,9 +307,7 @@ export function compact(paths: DecisionPaths): CompactResult {
       appendFileSync(paths.archive, superseded.map((e) => JSON.stringify(e)).join("\n") + "\n", "utf-8");
     }
 
-    const tmp = `${paths.log}.tmp.${process.pid}`;
-    writeFileSync(tmp, active.map((d) => JSON.stringify(d)).join("\n") + (active.length ? "\n" : ""), "utf-8");
-    renameSync(tmp, paths.log);
+    atomicWriteSync(paths.log, active.map((d) => JSON.stringify(d)).join("\n") + (active.length ? "\n" : ""));
     writeSnapshot(paths, active);
 
     return { activeCount: active.length, archivedCount: superseded.length, expungedCount: redactedIds.size };

lib/worktree.ts

@@ -13,6 +13,7 @@ import { spawnSync } from 'child_process';
 import * as crypto from 'crypto';
 import * as fs from 'fs';
 import * as path from 'path';
+import { atomicWriteSync } from './fs-atomic';
 import * as os from 'os';
 
 // --- Interfaces ---
@@ -84,9 +85,9 @@ function loadDedupIndex(): DedupIndex {
 function saveDedupIndex(index: DedupIndex): void {
   const dir = path.dirname(getDedupPath());
   fs.mkdirSync(dir, { recursive: true });
-  const tmp = getDedupPath() + '.tmp';
-  fs.writeFileSync(tmp, JSON.stringify(index, null, 2));
-  fs.renameSync(tmp, getDedupPath());
+  // Was a bare '.tmp' suffix — the deterministic-tmp collision race the
+  // shared helper exists to prevent.
+  atomicWriteSync(getDedupPath(), JSON.stringify(index, null, 2));
 }
 
 // --- WorktreeManager ---

test/fs-atomic.test.ts

@@ -0,0 +1,105 @@
+/**
+ * Unit tests for lib/fs-atomic.ts — the single atomic-write implementation.
+ * Free (no API calls), runs with `bun test`.
+ */
+
+import { describe, test, expect, beforeEach, afterEach } from 'bun:test';
+import * as fs from 'fs';
+import * as path from 'path';
+import * as os from 'os';
+import { atomicWriteSync, atomicWriteQuiet } from '../lib/fs-atomic';
+
+let dir: string;
+
+beforeEach(() => {
+  dir = fs.mkdtempSync(path.join(os.tmpdir(), 'fs-atomic-'));
+});
+
+afterEach(() => {
+  fs.rmSync(dir, { recursive: true, force: true });
+});
+
+describe('atomicWriteSync', () => {
+  test('writes the content and leaves no tmp file behind', () => {
+    const target = path.join(dir, 'out.json');
+    atomicWriteSync(target, '{"a":1}');
+    expect(fs.readFileSync(target, 'utf-8')).toBe('{"a":1}');
+    const strays = fs.readdirSync(dir).filter(f => f.includes('.tmp.'));
+    expect(strays).toEqual([]);
+  });
+
+  test('overwrites an existing file atomically', () => {
+    const target = path.join(dir, 'out.json');
+    fs.writeFileSync(target, 'old');
+    atomicWriteSync(target, 'new');
+    expect(fs.readFileSync(target, 'utf-8')).toBe('new');
+  });
+
+  test('applies the mode option at creation (0600)', () => {
+    if (process.platform === 'win32') return; // POSIX mode bits
+    const target = path.join(dir, 'secret.json');
+    atomicWriteSync(target, 'shh', { mode: 0o600 });
+    const mode = fs.statSync(target).mode & 0o777;
+    expect(mode).toBe(0o600);
+  });
+
+  test('THROWS on failure and cleans up the tmp file (missing parent dir)', () => {
+    const target = path.join(dir, 'no-such-subdir', 'out.json');
+    expect(() => atomicWriteSync(target, 'x')).toThrow();
+    // Parent doesn't exist, so nothing to clean; the throw contract is the point.
+    expect(fs.existsSync(target)).toBe(false);
+  });
+
+  test('tmp suffixes are unique across calls (pid+random — the collision race)', () => {
+    if (process.platform === 'win32') return; // read-only dir trick is POSIX
+    // Two interleaved writers in the SAME process must never share a tmp
+    // name. Bun's fs exports are readonly (no monkeypatching), so capture
+    // the generated tmp names from the failure path: a read-only directory
+    // makes writeFileSync throw ENOENT/EACCES with the tmp path attached.
+    const roDir = path.join(dir, 'ro');
+    fs.mkdirSync(roDir);
+    const target = path.join(roDir, 'contended.json');
+    fs.chmodSync(roDir, 0o500);
+    const seen = new Set<string>();
+    try {
+      for (let i = 0; i < 3; i++) {
+        try {
+          atomicWriteSync(target, 'x');
+          throw new Error('expected atomicWriteSync to throw in read-only dir');
+        } catch (err: any) {
+          expect(String(err.path ?? err.message)).toContain('.tmp.');
+          seen.add(String(err.path ?? err.message));
+        }
+      }
+    } finally {
+      fs.chmodSync(roDir, 0o700);
+    }
+    expect(seen.size).toBe(3);
+    for (const name of seen) {
+      expect(name).toMatch(/\.tmp\.\d+\.[0-9a-f]{8}$/);
+    }
+  });
+
+  test('two-writer same-target: last rename wins, file is never partial', () => {
+    const target = path.join(dir, 'race.json');
+    const big = 'x'.repeat(64 * 1024);
+    atomicWriteSync(target, big);
+    atomicWriteSync(target, 'small');
+    const content = fs.readFileSync(target, 'utf-8');
+    expect(content === big || content === 'small').toBe(true);
+    expect(content).toBe('small');
+  });
+});
+
+describe('atomicWriteQuiet', () => {
+  test('returns true on success', () => {
+    const target = path.join(dir, 'q.json');
+    expect(atomicWriteQuiet(target, 'ok')).toBe(true);
+    expect(fs.readFileSync(target, 'utf-8')).toBe('ok');
+  });
+
+  test('returns false (never throws) on failure — the shutdown-path contract', () => {
+    const target = path.join(dir, 'no-such-subdir', 'q.json');
+    expect(atomicWriteQuiet(target, 'x')).toBe(false);
+  });
+});