From f7b49ad77b1cee96a93377ce0c766526d17b1dcd Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Sun, 26 Jul 2026 00:13:48 -0700 Subject: [PATCH] fix(persistence): fsync state writes so a rename is actually durable (#10631) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(persistence): fsync state writes so a rename is actually durable `Store` wrote `orca-data.json` to a temp file and renamed it. rename() is atomic for readers but says nothing about durability: without an fsync the directory entry can reach disk before the data does. After power loss or a hard crash the file can come back holding the previous state or, worse, zero bytes — and `JSON.parse('')` throws, so an empty file takes the full corrupt-file path rather than degrading. This is the same empty-file symptom as #1158 from a different cause. That issue fixed a logic path that persisted empty state and added the .bak ring as a safety net; the ring also catches this, which is why it went unnoticed. Recovery costs up to an hour of tabs/layouts/session state (backups are throttled to >=1h spacing), and a user in their first hour has no backup slot yet, so they land on defaults indistinguishable from a fresh install. Both write paths now fsync the temp file *before* the rename, then fsync the containing directory. Directory fsync is best-effort by design: Windows cannot open a directory for fsync and some filesystems reject it, so it is swallowed. The file fsync is the load-bearing part and works everywhere. Measured cost on a 3 MB payload: ~0.2 ms per write, against a 1s debounce. The async path does not block the main thread. The syscall-order test mocks `node:fs` and counts fsync targets at the module boundary, asserting ['file', 'directory'] — proving the ordering rather than inferring it from reading the implementation, since a fsync after the rename would still pass every content assertion. * test(persistence): make the syscall proof platform-aware and actually prove the order Two problems, both found from CodeRabbit's Windows observation. The assertion hardcoded ['file', 'directory']. Directory fsync is deliberately best-effort — Windows cannot open a directory for fsync and some filesystems reject it — so on Windows the helper swallows the failure, only the file fsync is observed, and the test fails. The expectation now probes the real platform instead of assuming, keeping the guarantee tight where directory fsync works rather than dropping it everywhere. Worse, the test did not prove what its name claimed. Moving the fsync to *after* the rename still passes: the file is fsynced either way, and only fsyncs were recorded, so the correct and broken orders produced an identical log. Mutation-testing the "before rename" claim is what surfaced this — the mutation passed. The rename is now recorded in the same sequence, since it is the boundary the ordering is defined against. Re-running the same mutation fails, so the ordering claim is now backed by the test rather than asserted in a comment. --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- .../durable-file-write-syscall-proof.test.ts | 67 ++++++++++++++ src/main/durable-file-write.test.ts | 87 +++++++++++++++++++ src/main/durable-file-write.ts | 83 ++++++++++++++++++ src/main/persistence.ts | 19 ++-- 4 files changed, 251 insertions(+), 5 deletions(-) create mode 100644 src/main/durable-file-write-syscall-proof.test.ts create mode 100644 src/main/durable-file-write.test.ts create mode 100644 src/main/durable-file-write.ts diff --git a/src/main/durable-file-write-syscall-proof.test.ts b/src/main/durable-file-write-syscall-proof.test.ts new file mode 100644 index 000000000..bf18d6c00 --- /dev/null +++ b/src/main/durable-file-write-syscall-proof.test.ts @@ -0,0 +1,67 @@ +// Empirical proof that the durable write fsyncs the file, and the directory where the platform +// allows it. Counted at the module boundary rather than inferred from reading the implementation. +import { closeSync, fsyncSync, mkdtempSync, openSync, readFileSync, rmSync } from 'node:fs' +import type * as NodeFs from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { expect, it, vi } from 'vitest' + +/** Why the rename is recorded too: an fsync moved after the rename still fsyncs a file, so a + * fsync-only log reads identically for the correct and the broken order. The rename is the boundary + * the ordering is defined against, so it has to appear in the same sequence. */ +const syscalls: ('fsync:file' | 'fsync:directory' | 'rename')[] = [] + +vi.mock('node:fs', async () => { + const actual = await vi.importActual('node:fs') + return { + ...actual, + fsyncSync: (fd: number) => { + syscalls.push(actual.fstatSync(fd).isDirectory() ? 'fsync:directory' : 'fsync:file') + return actual.fsyncSync(fd) + }, + renameSync: (from: NodeFs.PathLike, to: NodeFs.PathLike) => { + syscalls.push('rename') + return actual.renameSync(from, to) + } + } +}) + +/** Windows cannot open a directory for fsync, and some filesystems reject it; probe rather than + * assume, so the expectation tracks the real platform instead of a hardcoded OS list. */ +function directoryFsyncSupported(directory: string): boolean { + let fd: number | null = null + try { + fd = openSync(directory, 'r') + fsyncSync(fd) + return true + } catch { + return false + } finally { + if (fd !== null) { + try { + closeSync(fd) + } catch { + // Nothing actionable in a probe. + } + } + } +} + +it('fsyncs the file before rename, and the directory after where supported', async () => { + const { writeFileDurableSync } = await import('./durable-file-write') + const dir = mkdtempSync(join(tmpdir(), 'orca-fsync-')) + try { + const supported = directoryFsyncSupported(dir) + syscalls.length = 0 // Discard the probe's own fsync. + const target = join(dir, 'x.json') + writeFileDurableSync(`${target}.tmp`, target, '{"ok":1}') + expect(readFileSync(target, 'utf-8')).toBe('{"ok":1}') + // The data fsync must precede the rename: that ordering is the entire fix. Publishing the name + // first is what lets a crash expose a stale or zero-length file. + expect(syscalls).toEqual( + supported ? ['fsync:file', 'rename', 'fsync:directory'] : ['fsync:file', 'rename'] + ) + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) diff --git a/src/main/durable-file-write.test.ts b/src/main/durable-file-write.test.ts new file mode 100644 index 000000000..c2a44bf38 --- /dev/null +++ b/src/main/durable-file-write.test.ts @@ -0,0 +1,87 @@ +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { writeFileDurable, writeFileDurableSync } from './durable-file-write' + +describe('durable file write', () => { + let dir: string + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'orca-durable-')) + }) + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }) + }) + + for (const [label, write] of [ + ['async', (t: string, f: string, p: string) => writeFileDurable(t, f, p)], + [ + 'sync', + (t: string, f: string, p: string) => { + writeFileDurableSync(t, f, p) + return Promise.resolve() + } + ] + ] as const) { + describe(label, () => { + it('publishes the payload at the final path', async () => { + const final = join(dir, 'state.json') + await write(`${final}.tmp`, final, '{"a":1}') + expect(readFileSync(final, 'utf-8')).toBe('{"a":1}') + }) + + it('replaces existing content atomically', async () => { + const final = join(dir, 'state.json') + writeFileSync(final, 'stale', 'utf-8') + await write(`${final}.tmp`, final, 'fresh') + expect(readFileSync(final, 'utf-8')).toBe('fresh') + }) + + it('leaves no temp file behind on success', async () => { + const final = join(dir, 'state.json') + const tmp = `${final}.tmp` + await write(tmp, final, 'x') + expect(() => readFileSync(tmp, 'utf-8')).toThrow() + }) + + it('round-trips a multi-megabyte payload without truncation', async () => { + // Why: the real orca-data.json is large; a partial fsync would surface here. + const final = join(dir, 'big.json') + const payload = JSON.stringify({ blob: 'x'.repeat(4 * 1024 * 1024) }) + await write(`${final}.tmp`, final, payload) + expect(readFileSync(final, 'utf-8')).toHaveLength(payload.length) + }) + + it('preserves exact bytes for multibyte and escape-sensitive content', async () => { + const final = join(dir, 'utf8.json') + const payload = JSON.stringify({ s: 'emoji 🚀 + 日本語 + \u0000 + "quotes"' }) + await write(`${final}.tmp`, final, payload) + expect(readFileSync(final, 'utf-8')).toBe(payload) + }) + + it('surfaces an unwritable temp path instead of silently succeeding', async () => { + const final = join(dir, 'state.json') + const tmp = join(dir, 'missing-subdir', 'state.json.tmp') + // The sync variant throws synchronously and the async one rejects; both must fail loudly + // and neither may publish a partial file. + let failed = false + try { + await write(tmp, final, 'x') + } catch { + failed = true + } + expect(failed).toBe(true) + expect(() => readFileSync(final, 'utf-8')).toThrow() + }) + }) + } + + it('keeps the last writer when async and sync paths target one file', async () => { + const final = join(dir, 'state.json') + await writeFileDurable(`${final}.a.tmp`, final, 'from-async') + writeFileDurableSync(`${final}.b.tmp`, final, 'from-sync') + expect(readFileSync(final, 'utf-8')).toBe('from-sync') + }) +}) diff --git a/src/main/durable-file-write.ts b/src/main/durable-file-write.ts new file mode 100644 index 000000000..7f827ba8a --- /dev/null +++ b/src/main/durable-file-write.ts @@ -0,0 +1,83 @@ +// Why: rename() is atomic for readers but not durable. Without fsync on the file and its directory, +// a power loss after a successful rename can leave the old contents, or an empty inode — the same +// empty-file symptom as issue #1158, from a different cause. The .bak ring recovers it at up to an +// hour's loss; fsync stops it from happening. + +import { closeSync, fsyncSync, openSync, renameSync, writeFileSync } from 'node:fs' +import { open, rename } from 'node:fs/promises' +import { dirname } from 'node:path' + +/** + * fsync a directory so a rename within it is durable. Best-effort by design: Windows cannot open a + * directory for fsync, and some filesystems reject it. The file fsync above it is the load-bearing + * part; this closes the "rename recorded but not persisted" window where the platform allows it. + */ +async function syncDirectory(directory: string): Promise { + let handle: Awaited> | null = null + try { + handle = await open(directory, 'r') + await handle.sync() + } catch { + // Expected on Windows and on filesystems without directory fsync. + } finally { + await handle?.close().catch(() => {}) + } +} + +function syncDirectorySync(directory: string): void { + let fd: number | null = null + try { + fd = openSync(directory, 'r') + fsyncSync(fd) + } catch { + // Same platform caveats as syncDirectory. + } finally { + if (fd !== null) { + try { + closeSync(fd) + } catch { + // Nothing actionable; the fsync already happened or the open failed. + } + } + } +} + +/** + * Rename and then fsync the containing directory. For callers that already fsynced the temp file + * themselves and need the rename made durable. + */ +export async function renameDurable(tmpPath: string, finalPath: string): Promise { + await rename(tmpPath, finalPath) + await syncDirectory(dirname(finalPath)) +} + +/** Write `payload` to `tmpPath`, fsync it, then rename onto `finalPath` and fsync the directory. */ +export async function writeFileDurable( + tmpPath: string, + finalPath: string, + payload: string +): Promise { + const handle = await open(tmpPath, 'w') + try { + await handle.writeFile(payload, 'utf-8') + // Why: fsync BEFORE rename. A rename that lands first can expose a zero-length file. + await handle.sync() + } finally { + await handle.close() + } + await rename(tmpPath, finalPath) + await syncDirectory(dirname(finalPath)) +} + +/** Synchronous counterpart for quit and crash paths that cannot await. */ +export function writeFileDurableSync(tmpPath: string, finalPath: string, payload: string): void { + writeFileSync(tmpPath, payload, 'utf-8') + const fd = openSync(tmpPath, 'r+') + try { + fsyncSync(fd) + } finally { + closeSync(fd) + } + renameSync(tmpPath, finalPath) + syncDirectorySync(dirname(finalPath)) +} diff --git a/src/main/persistence.ts b/src/main/persistence.ts index b26c68a9a..48495b7a1 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -11,7 +11,8 @@ import { statSync, realpathSync } from 'node:fs' -import { writeFile, rename, mkdir, rm, copyFile } from 'node:fs/promises' +import { rename, mkdir, rm, copyFile, open } from 'node:fs/promises' +import { renameDurable, writeFileDurableSync } from './durable-file-write' import { join, dirname, isAbsolute, resolve, sep } from 'node:path' import { homedir } from 'node:os' import { createHash, randomUUID } from 'node:crypto' @@ -3686,12 +3687,19 @@ export class Store { // Why: on any write/rename failure, remove the tmp file so it doesn't leave a multi-MB orphan. let renamed = false try { - await writeFile(tmpFile, payload, 'utf-8') + // Why: fsync before rename, then fsync the directory; see writeFileDurable. + const handle = await open(tmpFile, 'w') + try { + await handle.writeFile(payload, 'utf-8') + await handle.sync() + } finally { + await handle.close() + } // Why: if flush() bumped writeGeneration mid-write, it already wrote fresher state; don't overwrite it. if (this.writeGeneration !== gen) { return } - await rename(tmpFile, dataFile) + await renameDurable(tmpFile, dataFile) renamed = true // Why re-check gen: a sync flush during the rename await may have written fresher state; don't record a stale hash over it. if (this.writeGeneration === gen) { @@ -3732,8 +3740,9 @@ export class Store { // Why: on any write/rename failure, remove the tmp file so shutdown crashes don't leak orphans. let renamed = false try { - writeFileSync(tmpFile, payload, 'utf-8') - renameSync(tmpFile, dataFile) + // Why: fsync the temp file and the directory; a bare rename can survive as stale or empty + // content after power loss, losing projects/tabs back to the newest usable .bak slot. + writeFileDurableSync(tmpFile, dataFile, payload) renamed = true this.lastWrittenStateHash = stateHash } finally {