mirror of
https://github.com/stablyai/orca.git
synced 2026-10-25 00:03:59 +00:00
Jinwoo HongandOrcaWin
f7b49ad77b
* 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>
68 lines
2.6 KiB
TypeScript
68 lines
2.6 KiB
TypeScript
// 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<typeof NodeFs>('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 })
|
|
}
|
|
})
|