Skip to content

Commit 25edb7a

Browse files
committed
fix: preserve committed backups when JSON recovery fails
1 parent fbd596e commit 25edb7a

3 files changed

Lines changed: 125 additions & 49 deletions

File tree

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

Lines changed: 37 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,18 @@ const targetPath = path.resolve(process.env.INTERRUPTION_TARGET_PATH)
2424
const fsPromises = require("fs/promises")
2525
const actualRename = fsPromises.rename
2626
27-
// Fault injection: die with code 0 before the replacement lands. Any rename
27+
// Fault injection: hard-kill before the replacement lands. Any rename
2828
// whose destination is the canonical path is the commit step, whatever
2929
// replacement protocol the writer uses; exit before performing it.
30+
let commitAttempts = 0
3031
fsPromises.rename = function interruptionRename(oldPath, newPath) {
3132
if (path.resolve(String(newPath)) === targetPath) {
33+
commitAttempts++
34+
if (process.env.INTERRUPTION_MODE === "fallback" && commitAttempts === 1) {
35+
return Promise.reject(Object.assign(new Error("Injected EPERM"), { code: "EPERM" }))
36+
}
3237
fs.writeSync(1, "EXIT_BEFORE_REPLACEMENT_RENAME\\n")
33-
process.exit(0)
38+
process.kill(process.pid, "SIGKILL")
3439
}
3540
return actualRename.apply(fsPromises, arguments)
3641
}
@@ -104,9 +109,9 @@ describe("safeWriteJson subprocess interruption fault injection", () => {
104109
await fs.rm(tempDir, { recursive: true, force: true })
105110
})
106111

107-
it.each([1, 2, 3])(
108-
"keeps the committed file readable when the writer dies before the replacement lands (run %i)",
109-
async (run) => {
112+
it.each(["atomic", "fallback"] as const)(
113+
"preserves committed history when the %s writer is killed before replacement",
114+
async (mode) => {
110115
// A previously committed write contains the restart marker.
111116
const committed = { ui_messages: [{ ts: 1, type: "say", text: "RESTART_PERSISTENCE_MARKER" }] }
112117
await fs.writeFile(targetPath, JSON.stringify(committed), "utf8")
@@ -123,8 +128,9 @@ describe("safeWriteJson subprocess interruption fault injection", () => {
123128
env: {
124129
...process.env,
125130
INTERRUPTION_TARGET_PATH: targetPath,
131+
INTERRUPTION_MODE: mode,
126132
INTERRUPTION_PAYLOAD: JSON.stringify({
127-
ui_messages: [{ ts: 2, type: "say", text: `INTERRUPTED_WRITE_${run}` }],
133+
ui_messages: [{ ts: 2, type: "say", text: `INTERRUPTED_WRITE_${mode}` }],
128134
}),
129135
ESBUILD_PATH: esbuildPath,
130136
SAFE_WRITE_JSON_ENTRY: safeWriteJsonEntryPath,
@@ -137,23 +143,32 @@ describe("safeWriteJson subprocess interruption fault injection", () => {
137143
expect(exit.stdout).toContain("EXIT_BEFORE_REPLACEMENT_RENAME")
138144
expect(exit.stdout).not.toContain("WRITE_COMPLETED")
139145
expect(exit.stderr).not.toContain("WRITE_FAILED")
140-
expect(exit.code).toBe(0)
141-
142-
// The writer terminated before the replacement rename, so the
143-
// canonical path must still hold the last committed content. This
144-
// is the permanent-ENOENT regression: with the old two-rename
145-
// protocol the committed target was already renamed away, and no
146-
// read retry could bring it back.
147-
const after = JSON.parse(await readFileWithMissingRetry(targetPath))
148-
expect(after).toEqual(committed)
149-
150-
// Restart recovery: remove the advisory lock file orphaned by the
151-
// dead writer (proper-lockfile would only treat it as stale after
152-
// 31 seconds), then a fresh write must succeed and consume the
153-
// interrupted write's leftovers.
154-
await fs.rm(`${targetPath}.lock`, { force: true })
146+
expect(exit.code).toBeNull()
147+
148+
// Atomic replacement retains the canonical file; the two-step
149+
// fallback retains committed content in its backup until recovery.
150+
const interruptedFiles = await fs.readdir(storageDir)
151+
expect(interruptedFiles.filter((entry) => entry.includes(".new_"))).toHaveLength(1)
152+
if (mode === "fallback") {
153+
await expect(fs.readFile(targetPath)).rejects.toMatchObject({ code: "ENOENT" })
154+
const backups = interruptedFiles.filter((entry) => entry.includes(".bak_"))
155+
expect(backups).toHaveLength(1)
156+
expect(JSON.parse(await fs.readFile(path.join(storageDir, backups[0]), "utf8"))).toEqual(committed)
157+
} else {
158+
expect(JSON.parse(await readFileWithMissingRetry(targetPath))).toEqual(committed)
159+
}
160+
161+
// SIGKILL leaves the lock behind. Age it to exercise real stale-lock
162+
// acquisition without making the test wait through the stale window.
163+
const staleTime = new Date(Date.now() - 60_000)
164+
await fs.utimes(`${targetPath}.lock`, staleTime, staleTime)
155165
const recovered = { ui_messages: [{ ts: 3, type: "say", text: "RECOVERED_AFTER_RESTART" }] }
156-
await safeWriteJson(targetPath, recovered)
166+
await safeWriteJson(targetPath, recovered, {
167+
merge: (existing, incoming) => {
168+
expect(existing).toEqual(committed)
169+
return incoming
170+
},
171+
})
157172

158173
expect(JSON.parse(await readFileWithMissingRetry(targetPath))).toEqual(recovered)
159174
const leftovers = (await fs.readdir(storageDir)).filter(

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

Lines changed: 67 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -439,7 +439,7 @@ describe("safeWriteJson", () => {
439439

440440
// Test for rollback failure scenario
441441
test("should log error and re-throw original if rollback fails", async () => {
442-
const initialData = { message: "Initial, should be lost if rollback fails" }
442+
const initialData = { message: "Initial, preserved if rollback fails" }
443443
const newData = { message: "New content" }
444444

445445
await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData))
@@ -471,6 +471,16 @@ describe("safeWriteJson", () => {
471471
// is only logged.
472472
await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Fallback replacement rename failed")
473473

474+
const backups = (await fs.readdir(tempDir)).filter((entry) => entry.includes(".bak_"))
475+
expect(backups).toHaveLength(1)
476+
expect(await readFileContent(path.join(tempDir, backups[0]))).toEqual(initialData)
477+
await safeWriteJson(currentTestFilePath, newData, {
478+
merge: (existing) => {
479+
expect(existing).toEqual(initialData)
480+
return newData
481+
},
482+
})
483+
474484
// Verify console.error was called for the rollback failure
475485
expect(consoleErrorSpy).toHaveBeenCalledWith(
476486
expect.stringContaining("Failed to restore backup"),
@@ -595,21 +605,20 @@ describe("safeWriteJson", () => {
595605
const backupPath = path.join(tempDir, `.${path.basename(currentTestFilePath)}.bak_1_stale.tmp`)
596606
await fs.writeFile(backupPath, JSON.stringify({ stale: true }))
597607
// Directory access succeeds; only the canonical-path check fails.
598-
vi.mocked(fs.access)
599-
.mockResolvedValueOnce(undefined)
600-
.mockRejectedValueOnce(Object.assign(new Error("Canonical access failed"), { code }))
608+
const error = Object.assign(new Error("Canonical access failed"), { code })
609+
vi.mocked(fs.access).mockResolvedValueOnce(undefined).mockRejectedValueOnce(error)
601610
const merge = vi.fn((existing: unknown) => existing)
602611

603-
await safeWriteJson(currentTestFilePath, {}, { merge })
612+
await expect(safeWriteJson(currentTestFilePath, {}, { merge })).rejects.toBe(error)
604613

605-
expect(merge).toHaveBeenCalledWith({ initial: "content" }, {})
614+
expect(merge).not.toHaveBeenCalled()
606615
expect(await readFileContent(currentTestFilePath)).toEqual({ initial: "content" })
607-
expect(await fs.readdir(tempDir)).toEqual([path.basename(currentTestFilePath)])
616+
expect(await readFileContent(backupPath)).toEqual({ stale: true })
608617
},
609618
)
610619

611620
it.each(["ENOENT", "EACCES", "EIO"])(
612-
"should recover from a readable backup when another backup stat fails with %s",
621+
"should preserve backups when another backup stat fails with %s",
613622
async (code) => {
614623
await fs.unlink(currentTestFilePath)
615624
const unreadableBackup = path.join(tempDir, `.${path.basename(currentTestFilePath)}.bak_1_bad.tmp`)
@@ -625,14 +634,60 @@ describe("safeWriteJson", () => {
625634
})
626635
const merge = vi.fn((existing: unknown) => existing)
627636

628-
await safeWriteJson(currentTestFilePath, {}, { merge })
637+
if (code === "ENOENT") {
638+
await safeWriteJson(currentTestFilePath, {}, { merge })
639+
expect(merge).toHaveBeenCalledWith({ committed: true }, {})
640+
} else {
641+
await expect(safeWriteJson(currentTestFilePath, {}, { merge })).rejects.toMatchObject({ code })
642+
expect(merge).not.toHaveBeenCalled()
643+
expect(await readFileContent(unreadableBackup)).toEqual({ stale: true })
644+
expect(await readFileContent(readableBackup)).toEqual({ committed: true })
645+
}
646+
},
647+
)
629648

630-
expect(merge).toHaveBeenCalledWith({ committed: true }, {})
631-
expect(await readFileContent(currentTestFilePath)).toEqual({ committed: true })
632-
expect(await fs.readdir(tempDir)).toEqual([path.basename(currentTestFilePath)])
649+
it.each(["rename", "stat"] as const)(
650+
"should abort recovery and preserve history when backup %s fails",
651+
async (operation) => {
652+
await fs.unlink(currentTestFilePath)
653+
const backupPath = path.join(tempDir, `.${path.basename(currentTestFilePath)}.bak_1_saved.tmp`)
654+
const committed = { history: ["saved"] }
655+
await fs.writeFile(backupPath, JSON.stringify(committed))
656+
const error = Object.assign(new Error("Recovery failed"), { code: "EACCES" })
657+
vi.mocked(fs[operation]).mockRejectedValueOnce(error)
658+
const merge = vi.fn()
659+
660+
await expect(safeWriteJson(currentTestFilePath, {}, { merge })).rejects.toBe(error)
661+
662+
expect(merge).not.toHaveBeenCalled()
663+
expect(await readFileContent(backupPath)).toEqual(committed)
664+
await expect(fs.readFile(currentTestFilePath)).rejects.toMatchObject({ code: "ENOENT" })
665+
await safeWriteJson(currentTestFilePath, {}, { merge: (existing) => existing })
666+
expect(await readFileContent(currentTestFilePath)).toEqual(committed)
633667
},
634668
)
635669

670+
test("should restore by mtime and preserve unrelated siblings", async () => {
671+
await fs.unlink(currentTestFilePath)
672+
const prefix = `.${path.basename(currentTestFilePath)}`
673+
const newest = path.join(tempDir, `${prefix}.bak_1_newest.tmp`)
674+
const older = path.join(tempDir, `${prefix}.bak_9_older.tmp`)
675+
const unrelated = path.join(tempDir, ".other.json.bak_1_saved.tmp")
676+
const wrongSuffix = path.join(tempDir, `${prefix}.bak_1_saved.txt`)
677+
await fs.writeFile(newest, JSON.stringify({ newest: true }))
678+
await fs.writeFile(older, JSON.stringify({ older: true }))
679+
await fs.writeFile(unrelated, "keep")
680+
await fs.writeFile(wrongSuffix, "keep")
681+
await fs.utimes(older, new Date(1000), new Date(1000))
682+
await fs.utimes(newest, new Date(2000), new Date(2000))
683+
684+
await safeWriteJson(currentTestFilePath, {}, { merge: (existing) => existing })
685+
686+
expect(await readFileContent(currentTestFilePath)).toEqual({ newest: true })
687+
expect(await fs.readFile(unrelated, "utf8")).toBe("keep")
688+
expect(await fs.readFile(wrongSuffix, "utf8")).toBe("keep")
689+
})
690+
636691
test("should open the replacement with write access for flushing", async () => {
637692
const open = vi.mocked(fs.open)
638693
open.mockClear()

‎src/utils/safeWriteJson.ts‎

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,11 @@ async function recoverInterruptedReplacement(
106106
try {
107107
await fs.access(absoluteFilePath)
108108
} catch (error: unknown) {
109-
canonicalPathMissing = typeof error === "object" && error !== null && "code" in error && error.code === "ENOENT"
109+
if (typeof error !== "object" || error === null || !("code" in error) || error.code !== "ENOENT") {
110+
// Unknown canonical state must not authorize deleting committed backups.
111+
throw error
112+
}
113+
canonicalPathMissing = true
110114
}
111115

112116
if (canonicalPathMissing) {
@@ -120,9 +124,19 @@ async function recoverInterruptedReplacement(
120124
.map(async (backupPath) => {
121125
try {
122126
return { backupPath, mtimeMs: (await fs.stat(backupPath)).mtimeMs }
123-
} catch {
124-
// Skip leftovers removed or made unreadable since the directory listing.
125-
return null
127+
} catch (error: unknown) {
128+
if (
129+
typeof error === "object" &&
130+
error !== null &&
131+
"code" in error &&
132+
error.code === "ENOENT"
133+
) {
134+
return null
135+
}
136+
// An unreadable backup could be the newest committed state.
137+
// Intentionally block writes until it becomes readable or is
138+
// explicitly removed, rather than risk losing committed history.
139+
throw error
126140
}
127141
}),
128142
)
@@ -138,6 +152,7 @@ async function recoverInterruptedReplacement(
138152
`Failed to restore interrupted-write backup ${newestBackupPath} to ${absoluteFilePath}:`,
139153
restoreError,
140154
)
155+
throw restoreError
141156
}
142157
}
143158
}
@@ -322,17 +337,8 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit
322337
}
323338
}
324339

325-
// Cleanup the .bak file if it still needs to be (i.e., wasn't successfully restored)
326-
if (actualTempBackupFilePath) {
327-
try {
328-
await fs.unlink(actualTempBackupFilePath)
329-
} catch (cleanupError) {
330-
console.error(
331-
`[Catch] Failed to clean up temporary backup file ${actualTempBackupFilePath}:`,
332-
cleanupError,
333-
)
334-
}
335-
}
340+
// A failed rollback leaves the backup as the only committed copy.
341+
// Preserve it for recovery by a later writer.
336342
throw originalError // This MUST be the error that rejects the promise.
337343
} finally {
338344
// Release the lock in the main finally block.

0 commit comments

Comments
 (0)