Skip to content

Commit b95efab

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix(file-safety): fsync the copied target after the no-replace copy fallback
The COPYFILE_EXCL fallback (FAT32/exFAT/SMB mounts without hard links) creates a NEW inode at the target: the staged inode's pre-commit fsync does not cover it, and step 4b fsyncs only the parent directory. A crash right after the publish could therefore leave a zero-length or partial target while the write had reported success - breaking the module's PostCommitDurabilityError contract - and the caller's retry would then get the 'already exists' verdict on damaged bytes. The copied target is now opened and fsynced immediately after the fallback copy; a failure is reported as PostCommitDurabilityError (new copied-target message variant on the existing class) - the same durability verdict the directory fsync failure gets, never a swallowed success. Also bounds canonicalLockPath's link-walk recursion with a TOTAL hop budget (64) across frames. The per-frame bound of 8 did not terminate: a cycle whose length does not divide 8 (a -> b -> c -> a) makes every frame land on a different cycle member and recurse forever, and this runs on the lock path - resolveLockKey hands a cycle member to withFileLock - so the hang took guardedWrite and safeWriteJson down with it. Once the budget is spent the next frame walks zero hops and canonicalizes the ancestors of the cycle member it stopped on. Regression tests: fsync-after-copy (order-pinned after copyFile), fsync-failure durability verdict, and a 3-cycle termination test with a timer-yielding readlink so a regression fails on the test timeout instead of starving the event loop. Negative controls: removing the fsync block reddens exactly the two new copy tests; restoring the unbounded recursion reddens exactly the cycle test (timeout). 73/73 green across the two specs, eslint clean (suppressions unchanged), prettier clean, tsc adds nothing beyond the known local donor drift.
1 parent 6375a09 commit b95efab

4 files changed

Lines changed: 119 additions & 5 deletions

File tree

‎src/services/file-safety/__tests__/safeWriteText.spec.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1175,6 +1175,47 @@ describe("safeWriteText", () => {
11751175
expect(fs.rename).not.toHaveBeenCalled()
11761176
})
11771177

1178+
it("fsyncs the copied target after the exclusive-copy fallback", async () => {
1179+
// The copy creates a NEW inode at the target: the staged inode's pre-commit
1180+
// fsync does not cover it and step 4b flushes only the parent directory.
1181+
// Without this flush a crash on FAT32/exFAT/SMB can leave a zero-length or
1182+
// partial target while the write reported success - and the retry then gets
1183+
// the 'already exists' verdict on damaged bytes.
1184+
const targetPath = "/tmp/test-dir/target.txt"
1185+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1186+
vi.mocked(fs.link).mockRejectedValue(Object.assign(new Error("ENOTSUP"), { code: "ENOTSUP" }))
1187+
vi.mocked(fsSync.openSync).mockReturnValue(1)
1188+
1189+
await safeWriteText(targetPath, "new data", { failIfExist: true, platform: "linux" })
1190+
1191+
// The copied target itself was opened and fsynced AFTER the copy landed...
1192+
expect(fsSync.openSync).toHaveBeenCalledWith(targetPath, "r")
1193+
const copyOrder = vi.mocked(fs.copyFile).mock.invocationCallOrder[0]
1194+
const openCalls = vi.mocked(fsSync.openSync).mock.calls
1195+
const targetOpenIndex = openCalls.findIndex((call) => String(call[0]) === targetPath)
1196+
expect(targetOpenIndex).toBeGreaterThanOrEqual(0)
1197+
const targetOpenOrder = vi.mocked(fsSync.openSync).mock.invocationCallOrder[targetOpenIndex]
1198+
expect(targetOpenOrder).toBeGreaterThan(copyOrder)
1199+
// ...and the write still reports success.
1200+
})
1201+
1202+
it("reports a durability failure when the copied target cannot be fsynced", async () => {
1203+
// The bytes are at the target but not known to be on disk: the same
1204+
// durability verdict the directory fsync failure gets, never a swallowed
1205+
// success.
1206+
const targetPath = "/tmp/test-dir/target.txt"
1207+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1208+
vi.mocked(fs.link).mockRejectedValue(Object.assign(new Error("ENOTSUP"), { code: "ENOTSUP" }))
1209+
vi.mocked(fsSync.openSync).mockImplementation((target) => {
1210+
if (String(target) === targetPath) throw new Error("EIO")
1211+
return 1
1212+
})
1213+
1214+
await expect(safeWriteText(targetPath, "new data", { failIfExist: true, platform: "linux" })).rejects.toThrow(
1215+
PostCommitDurabilityError,
1216+
)
1217+
})
1218+
11781219
it("refuses the write when the exclusive copy finds an existing target", async () => {
11791220
const targetPath = "/tmp/test-dir/target.txt"
11801221
vi.mocked(fs.realpath).mockResolvedValue(targetPath)

‎src/services/file-safety/safeWriteText.ts‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,11 @@ export class StagingPathError extends Error {
127127
export class PostCommitDurabilityError extends Error {
128128
readonly targetPath: string
129129

130-
constructor(targetPath: string, cause: unknown) {
130+
constructor(targetPath: string, cause: unknown, copiedTargetFsync = false) {
131131
super(
132-
"The rename committed but the parent directory could not be fsynced -- the content is at the target path reported on this error, and the directory entry may not be durable.",
132+
copiedTargetFsync
133+
? "The exclusive copy committed but the copied target could not be fsynced -- the content is at the target path reported on this error, and it may not survive a crash on a volume without hard links."
134+
: "The rename committed but the parent directory could not be fsynced -- the content is at the target path reported on this error, and the directory entry may not be durable.",
133135
{ cause },
134136
)
135137
this.name = "PostCommitDurabilityError"
@@ -652,6 +654,25 @@ export async function safeWriteText(
652654
}
653655
throw copyError
654656
}
657+
// The copy created a NEW inode at the target: the staged inode was
658+
// fsynced before the commit, but the copied bytes live on an inode
659+
// nobody has flushed, and step 4b fsyncs only the parent directory.
660+
// On FAT32/exFAT and SMB mounts a crash right after the publish can
661+
// then leave a zero-length or partial target while the write reported
662+
// success - and a retry would get the 'already exists' verdict on
663+
// damaged bytes. Flush the copied target here; a failure is reported
664+
// like the directory fsync failure, as a durability verdict on bytes
665+
// that are already at the target.
666+
try {
667+
const copiedFd = fsSync.openSync(targetPath, "r")
668+
try {
669+
_fsyncFile(copiedFd)
670+
} finally {
671+
fsSync.closeSync(copiedFd)
672+
}
673+
} catch (error: unknown) {
674+
throw new PostCommitDurabilityError(targetPath, error, true)
675+
}
655676
}
656677
} else {
657678
await fs.rename(tempPath, targetPath)

‎src/utils/__tests__/fileLock.spec.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,3 +105,39 @@ test("locks the referent of a file symlink whose target is temporarily absent",
105105

106106
await releaseLock()
107107
})
108+
test("terminates on a three-link cycle instead of recursing forever", async () => {
109+
// a -> b -> c -> a: the cycle length does not divide the per-frame walk bound,
110+
// so a recursion that restarts its hop budget on every frame never terminates -
111+
// each frame walks 8 hops, lands on a different cycle member, and recurses.
112+
// resolveLockKey hands a cycle member to withFileLock, so an unbounded
113+
// canonicalLockPath hangs every guarded writer on the lock path, not just the
114+
// walker. The readlink mock yields to a timer so a regression fails on the
115+
// test timeout instead of starving the event loop.
116+
// Top-level test: clear the shared doubles so the counts below are this test's own.
117+
vi.mocked(lockfile.lock).mockClear()
118+
vi.mocked(fsPromises.readlink).mockClear()
119+
vi.mocked(fsPromises.realpath).mockClear()
120+
121+
const a = path.join(STORE, "cyc", "a.json")
122+
const b = path.join(STORE, "cyc", "b.json")
123+
const c = path.join(STORE, "cyc", "c.json")
124+
const cycle: Record<string, string> = { [a]: b, [b]: c, [c]: a }
125+
vi.mocked(fsPromises.readlink).mockImplementation(async (target: unknown) => {
126+
await new Promise((resolve) => setTimeout(resolve, 0))
127+
const referent = cycle[String(target)]
128+
if (referent === undefined) throw Object.assign(new Error("EINVAL"), { code: "EINVAL" })
129+
return referent
130+
})
131+
vi.mocked(fsPromises.realpath).mockImplementation(async () => {
132+
throw Object.assign(new Error("ENOENT"), { code: "ENOENT" })
133+
})
134+
135+
const releaseLock = await acquireFileLock(a)
136+
137+
// Terminated with exactly one lock acquisition on a cycle member, and the
138+
// total hop budget held across all frames.
139+
expect(lockfile.lock).toHaveBeenCalledTimes(1)
140+
expect(fsPromises.readlink.mock.calls.length).toBeLessThanOrEqual(64)
141+
142+
await releaseLock()
143+
})

‎src/utils/fileLock.ts‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,18 @@ export const LOCK_STALE_MS = 31_000
2424
* canonicalized the lexical absolute path is kept: lock keys stay stable and the
2525
* write's own resolution still decides where content lands.
2626
*/
27-
async function canonicalLockPath(filePath: string): Promise<string> {
27+
/**
28+
* Total link hops canonicalLockPath may follow across ALL its frames. The
29+
* per-frame walk bound alone does not terminate: a cycle whose length does not
30+
* divide that bound (a 3-link cycle a -> b -> c -> a against a bound of 8)
31+
* makes every frame land on a different cycle member, so the recursion below
32+
* restarts its budget forever. This runs on the lock path - resolveLockKey
33+
* hands a cycle member to withFileLock - so an unbounded walk hangs
34+
* guardedWrite and safeWriteJson, not just the walker.
35+
*/
36+
const MAX_TOTAL_LINK_HOPS = 64
37+
38+
async function canonicalLockPath(filePath: string, remainingLinkHops = MAX_TOTAL_LINK_HOPS): Promise<string> {
2839
const absoluteFilePath = path.resolve(filePath)
2940

3041
// A file symlink has to produce the same key the writer's lock uses
@@ -35,15 +46,20 @@ async function canonicalLockPath(filePath: string): Promise<string> {
3546
// parent would take a different lock and the two operations would stop excluding
3647
// each other. The walk is bounded so a two-link cycle terminates.
3748
let linkCursor = absoluteFilePath
38-
for (let depth = 0; depth < 8; depth++) {
49+
const frameBudget = Math.min(8, remainingLinkHops)
50+
for (let depth = 0; depth < frameBudget; depth++) {
3951
const referent = await fs.readlink(linkCursor).catch(() => undefined)
4052
if (referent === undefined) {
4153
break
4254
}
4355
linkCursor = path.resolve(path.dirname(linkCursor), referent)
4456
}
4557
if (linkCursor !== absoluteFilePath) {
46-
return await canonicalLockPath(linkCursor)
58+
// Recurse with the REMAINING hop budget, not a fresh one: once the total
59+
// is spent the next frame walks zero hops and canonicalizes the ancestors
60+
// of the cycle member it stopped on, so a cycle terminates on a stable key
61+
// instead of recursing forever.
62+
return await canonicalLockPath(linkCursor, Math.max(0, remainingLinkHops - frameBudget))
4763
}
4864
const missing: string[] = []
4965
let cursor = absoluteFilePath

0 commit comments

Comments
 (0)