From 3023216b87ef81a07f37210230e11575d756abc0 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 14 Aug 2026 19:33:58 -0700 Subject: [PATCH] =?UTF-8?q?feat(lib):=20fs-atomic=20=E2=80=94=20one=20atom?= =?UTF-8?q?ic-write=20implementation,=20with=20the=20race=20actually=20fix?= =?UTF-8?q?ed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- lib/fs-atomic.ts | 76 +++++++++++++++++++++++++++ lib/gbrain-local-status.ts | 7 +-- lib/gstack-decision.ts | 11 ++-- lib/worktree.ts | 7 +-- test/fs-atomic.test.ts | 105 +++++++++++++++++++++++++++++++++++++ 5 files changed, 191 insertions(+), 15 deletions(-) create mode 100644 lib/fs-atomic.ts create mode 100644 test/fs-atomic.test.ts diff --git a/lib/fs-atomic.ts b/lib/fs-atomic.ts new file mode 100644 index 000000000..641061896 --- /dev/null +++ b/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; + } +} diff --git a/lib/gbrain-local-status.ts b/lib/gbrain-local-status.ts index 174409ca1..a421b281f 100644 --- a/lib/gbrain-local-status.ts +++ b/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. } diff --git a/lib/gstack-decision.ts b/lib/gstack-decision.ts index 43270cb5a..87a0f35cb 100644 --- a/lib/gstack-decision.ts +++ b/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 }; diff --git a/lib/worktree.ts b/lib/worktree.ts index 8854a8404..0a8e507ec 100644 --- a/lib/worktree.ts +++ b/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 --- diff --git a/test/fs-atomic.test.ts b/test/fs-atomic.test.ts new file mode 100644 index 000000000..b41d58a19 --- /dev/null +++ b/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(); + 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); + }); +});