From e81931625aeb7d88a8d3235d1bd731fd9fcf6fc8 Mon Sep 17 00:00:00 2001 From: deseltrus <101901449+deseltrus@users.noreply.github.com> Date: Sat, 21 Mar 2026 15:47:00 +0100 Subject: [PATCH] fix(gsd): make saveJsonFile atomic via write-tmp-rename pattern (#1719) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit saveJsonFile() used raw writeFileSync which could produce corrupt/partial files on crash or SIGKILL. This affected 4 callers: queue-order.ts, metrics.ts, routing-history.ts, and reactive-graph.ts. Fix: replace writeFileSync with write-to-tmp + renameSync (the same pattern already used by writeJsonFileAtomic). The rename is atomic on POSIX filesystems, ensuring the target file is always either the old valid content or the new valid content — never a partial write. Tests: 8 new tests covering: - File creation with valid JSON - No .tmp file leakage on success - Parent directory auto-creation - Atomic overwrite of existing files - Round-trip compatibility with loadJsonFile - Equivalence with writeJsonFileAtomic - Large data objects - Non-fatal on permission errors --- .../extensions/gsd/json-persistence.ts | 12 +- .../gsd/tests/json-persistence-atomic.test.ts | 183 ++++++++++++++++++ 2 files changed, 193 insertions(+), 2 deletions(-) create mode 100644 src/resources/extensions/gsd/tests/json-persistence-atomic.test.ts diff --git a/src/resources/extensions/gsd/json-persistence.ts b/src/resources/extensions/gsd/json-persistence.ts index c58c28cf1..8c6c2776c 100644 --- a/src/resources/extensions/gsd/json-persistence.ts +++ b/src/resources/extensions/gsd/json-persistence.ts @@ -39,13 +39,21 @@ export function loadJsonFileOrNull( } /** - * Save a JSON file, creating parent directories as needed. + * Save a JSON file atomically (write to .tmp, then rename). + * Creates parent directories as needed. * Non-fatal — swallows errors to prevent persistence from breaking operations. + * + * Uses atomic write-tmp-rename to prevent partial/corrupt files on crash. + * This is the canonical way to persist JSON state in GSD — all callers + * (queue-order, metrics, routing-history, reactive-graph) benefit from + * crash-safety without code changes. */ export function saveJsonFile(filePath: string, data: T): void { try { mkdirSync(dirname(filePath), { recursive: true }); - writeFileSync(filePath, JSON.stringify(data, null, 2) + "\n", "utf-8"); + const tmp = filePath + ".tmp"; + writeFileSync(tmp, JSON.stringify(data, null, 2) + "\n", "utf-8"); + renameSync(tmp, filePath); } catch { // Non-fatal — don't let persistence failures break operation } diff --git a/src/resources/extensions/gsd/tests/json-persistence-atomic.test.ts b/src/resources/extensions/gsd/tests/json-persistence-atomic.test.ts new file mode 100644 index 000000000..39bb169a9 --- /dev/null +++ b/src/resources/extensions/gsd/tests/json-persistence-atomic.test.ts @@ -0,0 +1,183 @@ +/** + * json-persistence-atomic.test.ts — Tests for atomic JSON persistence. + * + * Verifies that saveJsonFile() uses atomic write-tmp-rename pattern + * so that crashes mid-write don't corrupt the target file. + */ + +import test from "node:test"; +import assert from "node:assert/strict"; +import { + existsSync, + mkdtempSync, + readFileSync, + readdirSync, + rmSync, +} from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; + +import { + saveJsonFile, + loadJsonFile, + writeJsonFileAtomic, +} from "../json-persistence.ts"; + +// ─── Helpers ───────────────────────────────────────────────────────── + +function makeTempDir(): string { + return mkdtempSync(join(tmpdir(), "gsd-json-test-")); +} + +function cleanup(dir: string): void { + try { + rmSync(dir, { recursive: true, force: true }); + } catch { + // ignore + } +} + +// ─── Tests ─────────────────────────────────────────────────────────── + +test("saveJsonFile creates file with valid JSON content", () => { + const dir = makeTempDir(); + const filePath = join(dir, "test.json"); + + try { + const data = { foo: "bar", count: 42 }; + saveJsonFile(filePath, data); + + assert.ok(existsSync(filePath), "File should exist"); + const content = readFileSync(filePath, "utf-8"); + const parsed = JSON.parse(content); + assert.deepEqual(parsed, data); + } finally { + cleanup(dir); + } +}); + +test("saveJsonFile does not leave .tmp files on success", () => { + const dir = makeTempDir(); + const filePath = join(dir, "clean.json"); + + try { + saveJsonFile(filePath, { test: true }); + + // No .tmp files should remain + const files = readdirSync(dir); + const tmpFiles = files.filter(f => f.includes(".tmp")); + assert.equal(tmpFiles.length, 0, `Unexpected .tmp files: ${tmpFiles.join(", ")}`); + } finally { + cleanup(dir); + } +}); + +test("saveJsonFile creates parent directories", () => { + const dir = makeTempDir(); + const filePath = join(dir, "deep", "nested", "data.json"); + + try { + saveJsonFile(filePath, { nested: true }); + + assert.ok(existsSync(filePath), "File should exist in nested directory"); + const parsed = JSON.parse(readFileSync(filePath, "utf-8")); + assert.deepEqual(parsed, { nested: true }); + } finally { + cleanup(dir); + } +}); + +test("saveJsonFile overwrites existing file atomically", () => { + const dir = makeTempDir(); + const filePath = join(dir, "overwrite.json"); + + try { + // Write initial value + saveJsonFile(filePath, { version: 1, data: "initial" }); + assert.equal(JSON.parse(readFileSync(filePath, "utf-8")).version, 1); + + // Overwrite + saveJsonFile(filePath, { version: 2, data: "updated" }); + const result = JSON.parse(readFileSync(filePath, "utf-8")); + assert.equal(result.version, 2); + assert.equal(result.data, "updated"); + } finally { + cleanup(dir); + } +}); + +test("saveJsonFile produces valid content readable by loadJsonFile", () => { + const dir = makeTempDir(); + const filePath = join(dir, "roundtrip.json"); + + try { + interface TestData { items: string[]; count: number } + const original: TestData = { items: ["a", "b", "c"], count: 3 }; + + saveJsonFile(filePath, original); + + const loaded = loadJsonFile( + filePath, + (d): d is TestData => typeof d === "object" && d !== null && "items" in d, + () => ({ items: [], count: 0 }), + ); + + assert.deepEqual(loaded, original); + } finally { + cleanup(dir); + } +}); + +test("writeJsonFileAtomic and saveJsonFile produce equivalent results", () => { + const dir = makeTempDir(); + const atomicPath = join(dir, "atomic.json"); + const savePath = join(dir, "save.json"); + + try { + const data = { key: "value", num: 123 }; + + writeJsonFileAtomic(atomicPath, data); + saveJsonFile(savePath, data); + + // Both should produce valid JSON with same content + const atomicParsed = JSON.parse(readFileSync(atomicPath, "utf-8")); + const saveParsed = JSON.parse(readFileSync(savePath, "utf-8")); + + assert.deepEqual(atomicParsed, data); + assert.deepEqual(saveParsed, data); + } finally { + cleanup(dir); + } +}); + +test("saveJsonFile handles large data objects", () => { + const dir = makeTempDir(); + const filePath = join(dir, "large.json"); + + try { + // Create a large object to stress-test atomic write + const largeData = { + items: Array.from({ length: 1000 }, (_, i) => ({ + id: i, + name: `item-${i}`, + description: "x".repeat(100), + })), + }; + + saveJsonFile(filePath, largeData); + + const loaded = JSON.parse(readFileSync(filePath, "utf-8")); + assert.equal(loaded.items.length, 1000); + assert.equal(loaded.items[999].id, 999); + } finally { + cleanup(dir); + } +}); + +test("saveJsonFile is non-fatal on permission errors", () => { + // Write to a path that doesn't exist and can't be created + // saveJsonFile should swallow the error, not throw + assert.doesNotThrow(() => { + saveJsonFile("/nonexistent/deeply/nested/path/file.json", { test: true }); + }); +});