Skip to content

Commit 8c8db37

Browse files
fix(file-safety): keep the published target across a post-commit durability error
Port of the U3 commit-state tracking fix to this unit's copies (same PI row family, identical catch shape): - safeWriteJson clears the staged path when safeWriteText reports PostCommitDurabilityError. The rename landed, so the staged path is the published target's former name; the catch cleanup must not unlink a path this write no longer owns (a concurrent writer reusing the staging name would lose its temp file). The durability error rethrows, so the write is never observed as a confirmed-durable success. - safeWriteText's failure cleanup only removes the staged temp when the commit rename never landed (the committed flag it already tracks); after the commit the staged path names the published target. Regression test: the parent-directory fsync fails after the commit rename; safeWriteJson rejects with PostCommitDurabilityError, the target stays present with the new JSON, and no unlink fires at the consumed staging path. Negative controls: removing either guard reddens exactly that test. 80/81 with the file-safety suites (1 pre-existing skip).
1 parent 2945ab2 commit 8c8db37

3 files changed

Lines changed: 63 additions & 7 deletions

File tree

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

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -477,10 +477,16 @@ export async function safeWriteText(
477477
rollbackFailure = { error: rollbackError, backupPath }
478478
}
479479
}
480-
try {
481-
await fs.unlink(tempPath).catch(() => {})
482-
} catch {
483-
// cleanup failure is non-fatal
480+
// Only a write that never committed has a staged file to remove. After the
481+
// commit rename the staged path names the published target: unlinking it
482+
// here would at best miss with ENOENT, and at worst delete a file a
483+
// concurrent writer has already claimed under the same staging name.
484+
if (!committed) {
485+
try {
486+
await fs.unlink(tempPath).catch(() => {})
487+
} catch {
488+
// cleanup failure is non-fatal
489+
}
484490
}
485491

486492
// A failed self-staged write must not leave its staging directory behind.

‎src/utils/__tests__/safeWriteJson.test.ts‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import * as path from "path"
44
import * as os from "os"
55

66
import { safeWriteJson } from "../safeWriteJson"
7-
import { RollbackFailureError } from "../../services/file-safety/safeWriteText"
7+
import { PostCommitDurabilityError, RollbackFailureError } from "../../services/file-safety/safeWriteText"
88
import * as lockfile from "proper-lockfile"
99

1010
// Capture actual implementations before the vi.mock factory runs,
@@ -441,7 +441,7 @@ describe("safeWriteJson", () => {
441441
})
442442

443443
// Test for rollback failure scenario (the rollback rename now lives in safeWriteText)
444-
test("re-throws the original error when the rollback rename fails, leaving an orphaned backup", async () => {
444+
test("throws RollbackFailureError with the publish failure as cause when the rollback rename also fails, leaving an orphaned backup", async () => {
445445
const initialData = { message: "Initial, orphaned when rollback fails" }
446446
const newData = { message: "New content" }
447447

@@ -463,7 +463,6 @@ describe("safeWriteJson", () => {
463463
return fsPromisesActuals.rename!(oldPath, newPath)
464464
})
465465

466-
// The original error must propagate, not the rollback error
467466
// The rollback also failed, so the error reports the partial state: the publish
468467
// failure stays the cause and the backup location is named.
469468
let failure: RollbackFailureError | undefined
@@ -702,4 +701,44 @@ describe("safeWriteJson", () => {
702701
expect(await readFileContent(currentTestFilePath)).toEqual({ after: true })
703702
},
704703
)
704+
705+
// ── post-commit durability: the published target must survive (PI regression) ──
706+
707+
it("keeps the published target with its new content when the post-commit directory fsync fails", async () => {
708+
const fsSyncMod = await import("fs")
709+
const realOpen = fsSyncMod.openSync
710+
const realFsync = fsSyncMod.fsyncSync
711+
// A sentinel fd identifies the parent-directory open that safeWriteText
712+
// performs after the commit rename; only that fsync fails.
713+
const sentinel = 987654
714+
vi.spyOn(process, "platform", "get").mockReturnValue("linux")
715+
vi.spyOn(fsSyncMod, "openSync").mockImplementation(((...args: unknown[]) => {
716+
if (String(args[0]) === tempDir) {
717+
return sentinel
718+
}
719+
return (realOpen as (...a: unknown[]) => unknown)(...args)
720+
}) as never)
721+
vi.spyOn(fsSyncMod, "fsyncSync").mockImplementation(((fd: number) => {
722+
if (fd === sentinel) {
723+
throw new Error("EIO fsync")
724+
}
725+
return (realFsync as (f: number) => unknown)(fd)
726+
}) as never)
727+
728+
// Earlier tests in this file leave unlink history on the shared mock.
729+
vi.mocked(fs.unlink).mockClear()
730+
731+
await expect(safeWriteJson(currentTestFilePath, { fresh: "content" })).rejects.toBeInstanceOf(
732+
PostCommitDurabilityError,
733+
)
734+
735+
// The rename committed: the target must still hold the new JSON, not be
736+
// unlinked away by the catch cleanup of the consumed staging path.
737+
const content = await readFileContent(currentTestFilePath)
738+
expect(content).toEqual({ fresh: "content" })
739+
// The cleanup must not unlink the consumed staging path: after the commit
740+
// rename it is no longer this write's file to remove.
741+
const stagingUnlinks = vi.mocked(fs.unlink).mock.calls.filter((call) => /\.new_.*\.tmp$/.test(String(call[0])))
742+
expect(stagingUnlinks).toHaveLength(0)
743+
})
705744
})

‎src/utils/safeWriteJson.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { JsonStreamStringify } from "json-stream-stringify"
55

66
import { acquireFileLock } from "./fileLock"
77
import {
8+
PostCommitDurabilityError,
89
resolveLockKey,
910
resolvePublishTarget,
1011
safeWriteText,
@@ -139,6 +140,16 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso
139140
originalError,
140141
)
141142

143+
// PostCommitDurabilityError means the commit rename landed: the staged
144+
// path was renamed onto the target, so it is no longer a leftover temp
145+
// file. Unlinking it here would at best race a concurrent writer that has
146+
// already reused the staging name, and the published target must survive
147+
// the durability failure with its new content. The error rethrows below,
148+
// so the write is never observed as a confirmed-durable success.
149+
if (originalError instanceof PostCommitDurabilityError) {
150+
actualTempNewFilePath = null
151+
}
152+
142153
const newFileToCleanupWithinCatch = actualTempNewFilePath
143154

144155
// A failed safeWriteText already rolled the backup (if any) back to

0 commit comments

Comments
 (0)