Skip to content

Commit e5e71aa

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix(file-safety): fail closed when the volume has no atomic no-replace primitive
The guarded create published through an exclusive copy when link(2) said the volume has no hard links. A copy is not atomic: readers can observe a partial file at the final name, and a crash or EIO mid-copy leaves damaged bytes there with the 'already exists' verdict on retry - the copy's own fsync and cleanup bound the damage but cannot bound the visibility window. No atomic no-replace primitive exists on those volumes, so the commit now refuses to publish at all and reports an explicit UnsupportedFilesystemError instead; the target name stays absent for a retry on a capable volume. The five fallback tests are replaced by one fail-closed test covering all three no-hard-link codes (EPERM/ENOTSUP/ENOSYS).
1 parent 8bb7a32 commit e5e71aa

2 files changed

Lines changed: 47 additions & 126 deletions

File tree

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

Lines changed: 20 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
safeWriteText,
1212
StagingPathError,
1313
TargetExistsError,
14+
UnsupportedFilesystemError,
1415
TargetMovedError,
1516
AncestorReplacedError,
1617
OrphanedBackupError,
@@ -1158,97 +1159,34 @@ describe("safeWriteText", () => {
11581159
expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText"), canonical)
11591160
})
11601161

1161-
it("falls back to an exclusive copy when the filesystem has no hard links", async () => {
1162-
// FAT32/exFAT and some SMB mounts reject link(2); the create must still work, and
1163-
// COPYFILE_EXCL keeps the no-replace verdict identical.
1162+
it("fails closed when the filesystem has no atomic no-replace primitive", async () => {
1163+
// FAT32/exFAT and some SMB mounts reject link(2). Publishing by an exclusive
1164+
// copy would put bytes at the final name that readers can observe mid-copy,
1165+
// and a crash or an EIO mid-copy leaves damaged bytes there with the
1166+
// 'already exists' verdict on retry. No atomic no-replace primitive exists
1167+
// on those volumes, so the guarded create refuses to publish at all.
11641168
const targetPath = "/tmp/test-dir/target.txt"
11651169
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
11661170
vi.mocked(fs.link).mockRejectedValue(Object.assign(new Error("ENOTSUP"), { code: "ENOTSUP" }))
11671171

1168-
await safeWriteText(targetPath, "new data", { failIfExist: true, platform: "linux" })
1169-
1170-
expect(fs.copyFile).toHaveBeenCalledWith(
1171-
expect.stringContaining("safeWriteText"),
1172-
targetPath,
1173-
fsSync.constants.COPYFILE_EXCL,
1174-
)
1175-
expect(fs.rename).not.toHaveBeenCalled()
1176-
})
1177-
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-
// with write access: Windows FlushFileBuffers requires GENERIC_WRITE, so a
1193-
// read-only handle would fail the flush and report a false durability error
1194-
// on bytes that are already published.
1195-
expect(fsSync.openSync).toHaveBeenCalledWith(targetPath, "r+")
1196-
const copyOrder = vi.mocked(fs.copyFile).mock.invocationCallOrder[0]
1197-
const openCalls = vi.mocked(fsSync.openSync).mock.calls
1198-
const targetOpenIndex = openCalls.findIndex((call) => String(call[0]) === targetPath)
1199-
expect(targetOpenIndex).toBeGreaterThanOrEqual(0)
1200-
const targetOpenOrder = vi.mocked(fsSync.openSync).mock.invocationCallOrder[targetOpenIndex]
1201-
expect(targetOpenOrder).toBeGreaterThan(copyOrder)
1202-
// ...and the write still reports success.
1203-
})
1204-
1205-
it("reports a durability failure when the copied target cannot be fsynced", async () => {
1206-
// The bytes are at the target but not known to be on disk: the same
1207-
// durability verdict the directory fsync failure gets, never a swallowed
1208-
// success.
1209-
const targetPath = "/tmp/test-dir/target.txt"
1210-
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1211-
vi.mocked(fs.link).mockRejectedValue(Object.assign(new Error("ENOTSUP"), { code: "ENOTSUP" }))
1212-
vi.mocked(fsSync.openSync).mockImplementation((target) => {
1213-
if (String(target) === targetPath) throw new Error("EIO")
1214-
return 1
1215-
})
1216-
1217-
await expect(safeWriteText(targetPath, "new data", { failIfExist: true, platform: "linux" })).rejects.toThrow(
1218-
PostCommitDurabilityError,
1219-
)
1220-
})
1221-
1222-
it("removes the partially copied target when the exclusive copy fails mid-copy", async () => {
1223-
// The copy writes directly to the final name: an EIO/ENOSPC mid-copy leaves a
1224-
// partial file that readers would observe as content and a retry would report
1225-
// as 'already exists' on damaged bytes. COPYFILE_EXCL guarantees this write
1226-
// created the name, so the failure path must remove it.
1227-
const targetPath = "/tmp/test-dir/target.txt"
1228-
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1229-
vi.mocked(fs.link).mockRejectedValue(Object.assign(new Error("ENOTSUP"), { code: "ENOTSUP" }))
1230-
vi.mocked(fs.copyFile).mockRejectedValue(Object.assign(new Error("EIO"), { code: "EIO" }))
1231-
vi.mocked(fs.unlink).mockClear()
1232-
12331172
await expect(
12341173
safeWriteText(targetPath, "new data", { failIfExist: true, platform: "linux" }),
1235-
).rejects.toMatchObject({ code: "EIO" })
1174+
).rejects.toBeInstanceOf(UnsupportedFilesystemError)
12361175

1237-
// The partial target was removed - the name is absent again for the retry.
1238-
expect(vi.mocked(fs.unlink).mock.calls.some((call) => String(call[0]) === targetPath)).toBe(true)
1239-
})
1240-
1241-
it("refuses the write when the exclusive copy finds an existing target", async () => {
1242-
const targetPath = "/tmp/test-dir/target.txt"
1243-
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1244-
vi.mocked(fs.link).mockRejectedValue(Object.assign(new Error("EPERM"), { code: "EPERM" }))
1245-
vi.mocked(fs.copyFile).mockRejectedValue(Object.assign(new Error("EEXIST"), { code: "EEXIST" }))
1176+
// Nothing was published: no copy at the final name, no rename - the name
1177+
// stays absent for a retry on a capable volume.
1178+
// Every "no hard links here" code takes the same fail-closed path.
1179+
for (const code of ["EPERM", "ENOTSUP", "ENOSYS"]) {
1180+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1181+
vi.mocked(fs.link).mockRejectedValue(Object.assign(new Error(code), { code }))
12461182

1247-
await expect(
1248-
safeWriteText(targetPath, "new data", { failIfExist: true, platform: "linux" }),
1249-
).rejects.toBeInstanceOf(TargetExistsError)
1183+
await expect(
1184+
safeWriteText(targetPath, "new data", { failIfExist: true, platform: "linux" }),
1185+
).rejects.toBeInstanceOf(UnsupportedFilesystemError)
1186+
}
1187+
expect(fs.copyFile).not.toHaveBeenCalled()
1188+
expect(fs.rename).not.toHaveBeenCalled()
12501189
})
1251-
12521190
it("still reports success when the staged copy cannot be unlinked after the link", async () => {
12531191
// An antivirus handle on Windows can make unlink fail with EBUSY after link(2)
12541192
// published the name. The content is readable at the target, so the write must

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

Lines changed: 27 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,25 @@ export class TargetExistsError extends Error {
158158
}
159159
}
160160

161+
/**
162+
* A conditional create (failIfExist) found that the volume hosting the target has
163+
* no atomic no-replace primitive: link(2) reported that hard links do not exist
164+
* here, and publishing by copy would put bytes at the final name that readers can
165+
* observe mid-copy. Nothing is published - the write fails closed so a retry on a
166+
* capable volume (or a caller that accepts non-atomic publication) decides.
167+
*/
168+
export class UnsupportedFilesystemError extends Error {
169+
readonly targetPath: string
170+
constructor(targetPath: string, cause: unknown) {
171+
super(
172+
`Filesystem hosting ${targetPath} has no atomic no-replace commit primitive -- the guarded create refused to publish by copy.`,
173+
{ cause },
174+
)
175+
this.name = "UnsupportedFilesystemError"
176+
this.targetPath = targetPath
177+
}
178+
}
179+
161180
/**
162181
* The path resolved to something other than the target the caller authorized.
163182
* Nothing is staged or published: a link swapped in after the caller's containment
@@ -653,53 +672,17 @@ export async function safeWriteText(
653672
throw new TargetExistsError(targetPath)
654673
}
655674
// FAT32/exFAT volumes and some SMB/network mounts have no hard links, where
656-
// link(2) fails EPERM/ENOTSUP/ENOSYS. Fall back to an exclusive copy so a
657-
// create still works there: copyFile with COPYFILE_EXCL still fails EEXIST
658-
// when the name exists, so the no-replace verdict is unchanged. Unlike link(2)
659-
// the copy is not atomic - a reader can observe a partial file - so it is only
660-
// used when the atomic primitive is unavailable.
675+
// link(2) fails EPERM/ENOTSUP/ENOSYS. The fallback this replaces published by
676+
// an exclusive copy directly at the final name, but a copy is not atomic:
677+
// readers can observe a partial file, and a crash or an EIO mid-copy leaves
678+
// damaged bytes under the final name with the 'already exists' verdict on
679+
// retry. No atomic no-replace primitive exists on those volumes, so the
680+
// guarded create fails closed - nothing is published and the caller learns
681+
// the volume cannot honor the contract.
661682
if (code !== "EPERM" && code !== "ENOTSUP" && code !== "ENOSYS") {
662683
throw error
663684
}
664-
try {
665-
await fs.copyFile(tempPath, targetPath, fsSync.constants.COPYFILE_EXCL)
666-
} catch (copyError: unknown) {
667-
if (errorCode(copyError) === "EEXIST") {
668-
throw new TargetExistsError(targetPath)
669-
}
670-
// The copy writes directly to the final name: a mid-copy failure
671-
// (EIO, ENOSPC) leaves a partial file at the target that readers
672-
// would observe as content, and a retry would then get the
673-
// 'already exists' verdict on damaged bytes. COPYFILE_EXCL
674-
// guarantees this write created the name, so removing it restores
675-
// the pre-write absence; the staged copy is removed by the outer
676-
// cleanup.
677-
await fs.unlink(targetPath).catch(() => undefined)
678-
throw copyError
679-
}
680-
// The copy created a NEW inode at the target: the staged inode was
681-
// fsynced before the commit, but the copied bytes live on an inode
682-
// nobody has flushed, and step 4b fsyncs only the parent directory.
683-
// On FAT32/exFAT and SMB mounts a crash right after the publish can
684-
// then leave a zero-length or partial target while the write reported
685-
// success - and a retry would get the 'already exists' verdict on
686-
// damaged bytes. Flush the copied target here; a failure is reported
687-
// like the directory fsync failure, as a durability verdict on bytes
688-
// that are already at the target.
689-
// Open with write access: on Windows FlushFileBuffers requires
690-
// GENERIC_WRITE on the handle, so a read-only handle can fail the flush
691-
// even though the bytes are at the target - the same reason the backup
692-
// fsync path opens "r+".
693-
try {
694-
const copiedFd = fsSync.openSync(targetPath, "r+")
695-
try {
696-
_fsyncFile(copiedFd)
697-
} finally {
698-
fsSync.closeSync(copiedFd)
699-
}
700-
} catch (error: unknown) {
701-
throw new PostCommitDurabilityError(targetPath, error, true)
702-
}
685+
throw new UnsupportedFilesystemError(targetPath, error)
703686
}
704687
} else {
705688
await fs.rename(tempPath, targetPath)

0 commit comments

Comments
 (0)