diff --git a/src/utils/__tests__/jsonFileRecovery.test.ts b/src/utils/__tests__/jsonFileRecovery.test.ts index 1bd90d8935..ce33e739b4 100644 --- a/src/utils/__tests__/jsonFileRecovery.test.ts +++ b/src/utils/__tests__/jsonFileRecovery.test.ts @@ -4,7 +4,7 @@ import * as path from "path" import { acquireFileLock } from "../fileLock" import { setPersistenceTraceSink } from "../../core/task-persistence/persistenceTrace" -import { deleteJsonFile, recoverMissingJsonFile } from "../jsonFileRecovery" +import { recoverInterruptedReplacement, deleteJsonFile, recoverMissingJsonFile } from "../jsonFileRecovery" import { readFileWithMissingRetry } from "../../core/task-persistence/readFileWithMissingRetry" vi.mock("fs/promises", async () => { @@ -57,6 +57,57 @@ describe("reader recovery", () => { await expect(fs.readFile(backup)).rejects.toMatchObject({ code: "ENOENT" }) }) + // The recency choice is `sort((a, b) => b.mtimeMs - a.mtimeMs)` followed by renaming + // the head of that list. mtime is the only signal recovery has, so an exact tie says + // nothing about which backup holds the committed content - the winner would be readdir + // order, and the loser is unlinked. The tie is set with utimes rather than raced + // against the clock, so the row does not depend on how fast two writes land; the + // second row is the control that keeps this from being a test which merely asserts + // "two backups means reject". + it("refuses an exact mtime tie between two backups instead of picking by directory order", async () => { + const otherBackup = path.join(dir, ".history_item.json.bak_2_other.tmp") + await fs.writeFile(otherBackup, JSON.stringify({ other: true })) + const shared = new Date(1_700_000_000_000) + await fs.utimes(backup, shared, shared) + await fs.utimes(otherBackup, shared, shared) + + const error = await recoverInterruptedReplacement(target).then( + () => undefined, + (reason: unknown) => reason, + ) + expect(error).toMatchObject({ + name: "AmbiguousBackupRecencyError", + code: "AMBIGUOUS_BACKUP_RECENCY", + mtimeMs: 1_700_000_000_000, + }) + // The diagnostic has to name the magnitude it refused to guess about. + expect(String((error as Error).message)).toContain("share mtime 1700000000000") + + // Fail closed: nothing restored, nothing deleted, so both copies survive for a + // human to read. + await expect(fs.access(target)).rejects.toMatchObject({ code: "ENOENT" }) + expect(JSON.parse(await fs.readFile(backup, "utf8"))).toEqual({ committed: true }) + expect(JSON.parse(await fs.readFile(otherBackup, "utf8"))).toEqual({ other: true }) + }) + + it("restores the newer backup when two backups have different mtimes", async () => { + const otherBackup = path.join(dir, ".history_item.json.bak_2_other.tmp") + await fs.writeFile(otherBackup, JSON.stringify({ other: true })) + const older = new Date(1_700_000_000_000) + const newer = new Date(1_700_000_000_001) + await fs.utimes(otherBackup, older, older) + await fs.utimes(backup, newer, newer) + + const recovery = await recoverInterruptedReplacement(target) + + expect(recovery.foundLeftovers).toBe(true) + expect(recovery.restoredFrom).toBe(backup) + expect(JSON.parse(await fs.readFile(target, "utf8"))).toEqual({ committed: true }) + // The loser of a decided race is still garbage-collected, which is the contract the + // tie row must not break. + await expect(fs.access(otherBackup)).rejects.toMatchObject({ code: "ENOENT" }) + }) + it("traces reader restoration and lock waiting", async () => { const sink = vi.fn() setPersistenceTraceSink(sink) @@ -106,4 +157,83 @@ describe("reader recovery", () => { await expect(recoverMissingJsonFile(target)).resolves.toBe(false) await expect(fs.stat(dir)).rejects.toMatchObject({ code: "ENOENT" }) }) + + it("restores the newest backup when two OLDER backups share a tie", async () => { + // Three backups: 200, 100, 100. The tie sits BELOW the winner, so recency is decidable + // and recovery must restore the newest copy and remove both losers. A guard that refused + // every duplicate mtime - not just a tie for the newest slot - would fail this row, which + // is what stops the two tie rows above from standing in for "any duplicate means refuse". + const otherBackup = path.join(dir, ".history_item.json.bak_2_other.tmp") + const thirdBackup = path.join(dir, ".history_item.json.bak_3_third.tmp") + await fs.writeFile(otherBackup, JSON.stringify({ other: true })) + await fs.writeFile(thirdBackup, JSON.stringify({ third: true })) + const older = new Date(1_700_000_000_100) + const newest = new Date(1_700_000_000_200) + await fs.utimes(otherBackup, older, older) + await fs.utimes(thirdBackup, older, older) + await fs.utimes(backup, newest, newest) + + const recovery = await recoverInterruptedReplacement(target) + + expect(recovery.foundLeftovers).toBe(true) + expect(recovery.restoredFrom).toBe(backup) + expect(JSON.parse(await fs.readFile(target, "utf8"))).toEqual({ committed: true }) + await expect(fs.access(otherBackup)).rejects.toMatchObject({ code: "ENOENT" }) + await expect(fs.access(thirdBackup)).rejects.toMatchObject({ code: "ENOENT" }) + }) + + it("refuses a tie through the reader entry point and keeps both copies", async () => { + // The refusal has to survive the path a reader actually takes: recoverMissingJsonFile runs + // recovery under the lock and reports the failure through the persistence trace. Asserting + // it here as well keeps a propagation or trace regression from passing while the internal + // helper row still goes green. + const otherBackup = path.join(dir, ".history_item.json.bak_2_other.tmp") + await fs.writeFile(otherBackup, JSON.stringify({ other: true })) + const shared = new Date(1_700_000_000_000) + await fs.utimes(backup, shared, shared) + await fs.utimes(otherBackup, shared, shared) + const sink = vi.fn() + setPersistenceTraceSink(sink) + + await expect(recoverMissingJsonFile(target)).rejects.toMatchObject({ + name: "AmbiguousBackupRecencyError", + code: "AMBIGUOUS_BACKUP_RECENCY", + }) + + expect(sink).toHaveBeenCalledWith(expect.stringContaining('"stage":"json-recovery"')) + expect(sink).toHaveBeenCalledWith(expect.stringContaining('"result":"failed"')) + // Fail closed: the tie is still there for a human to decide about. + await expect(fs.access(target)).rejects.toMatchObject({ code: "ENOENT" }) + expect(JSON.parse(await fs.readFile(backup, "utf8"))).toEqual({ committed: true }) + expect(JSON.parse(await fs.readFile(otherBackup, "utf8"))).toEqual({ other: true }) + }) + + it("keeps the tied paths out of the tie error message and on the error properties", async () => { + // Error telemetry captures message and stack, and a workspace path carries the local + // username into that capture. The message names the basenames it refused to choose between; + // the full paths stay on the properties, which is where a human acts on them. + const otherBackup = path.join(dir, ".history_item.json.bak_2_other.tmp") + await fs.writeFile(otherBackup, JSON.stringify({ other: true })) + const shared = new Date(1_700_000_000_000) + await fs.utimes(backup, shared, shared) + await fs.utimes(otherBackup, shared, shared) + + const error = await recoverInterruptedReplacement(target).then( + () => undefined, + (reason: unknown) => reason, + ) + const tie = error as Error & { targetPath: string; firstBackupPath: string; secondBackupPath: string } + + expect(tie.name).toBe("AmbiguousBackupRecencyError") + expect(tie.message).not.toContain(path.sep) + expect(tie.message).not.toContain(dir) + expect(tie.message).toContain("history_item.json") + expect(tie.message).toContain("share mtime 1700000000000") + expect(tie.targetPath).toBe(target) + // The two tied files, whatever order readdir handed them in: the guard reports both, + // and which one lands on which property is not something the caller can rely on. + expect([tie.firstBackupPath, tie.secondBackupPath].sort()).toEqual([backup, otherBackup].sort()) + expect(tie.message).toContain(path.basename(backup)) + expect(tie.message).toContain(path.basename(otherBackup)) + }) }) diff --git a/src/utils/jsonFileRecovery.ts b/src/utils/jsonFileRecovery.ts index 2350f30685..1ad95dbdd8 100644 --- a/src/utils/jsonFileRecovery.ts +++ b/src/utils/jsonFileRecovery.ts @@ -99,6 +99,36 @@ export async function deleteJsonFile(filePath: string): Promise { }) } +/** + * Two leftover backups whose mtimes are exactly equal carry no evidence about which + * one is the newer committed state - mtime is the only recency signal recovery has - + * so choosing between them would be the order readdir happened to list them in, and + * the loser of that order is garbage-collected below. Recovery refuses instead, the + * same fail-closed decision it already makes for a backup it cannot stat: nothing is + * restored and nothing is deleted, so both copies stay on disk for a human to read. + */ +class AmbiguousBackupRecencyError extends Error { + readonly code = "AMBIGUOUS_BACKUP_RECENCY" + readonly targetPath: string + readonly mtimeMs: number + readonly firstBackupPath: string + readonly secondBackupPath: string + constructor(targetPath: string, mtimeMs: number, firstBackupPath: string, secondBackupPath: string) { + super( + // Basenames, not absolute paths: this error travels to callers that report it, and error + // telemetry captures message and stack, so a workspace path here would carry the local + // username into that capture. The full paths stay on the properties for whoever has to + // decide between the two tied copies. + `Cannot tell which backup of ${path.basename(targetPath)} is the newest committed state: ${path.basename(firstBackupPath)} and ${path.basename(secondBackupPath)} share mtime ${mtimeMs}. Nothing was restored or deleted.`, + ) + this.name = "AmbiguousBackupRecencyError" + this.targetPath = targetPath + this.mtimeMs = mtimeMs + this.firstBackupPath = firstBackupPath + this.secondBackupPath = secondBackupPath + } +} + /** * Heals the canonical path after an interrupted previous write. Must run * under the advisory lock and before any data is read or written: temp and @@ -112,8 +142,9 @@ export async function deleteJsonFile(filePath: string): Promise { * renamed to its backup but before the replacement landed. A process exit * at that point leaves the canonical path absent, which readers see as a * permanent ENOENT that no retry can repair, so the newest backup is - * restored first. Leftover replacement and backup files are garbage - * collected afterwards. + * restored first, chosen by mtime. Two backups that share an exact mtime are + * refused rather than split by directory order; see AmbiguousBackupRecencyError. + * Leftover replacement and backup files are garbage collected afterwards. * * @returns Whether leftover files were found, and the backup path that was * restored, if any. @@ -181,6 +212,18 @@ export async function recoverInterruptedReplacement( ).filter((entry): entry is { backupPath: string; mtimeMs: number } => entry !== null) if (backupStats.length > 0) { backupStats.sort((a, b) => b.mtimeMs - a.mtimeMs) + if (backupStats.length > 1 && backupStats[1].mtimeMs === backupStats[0].mtimeMs) { + // mtime is the only recency signal here, and an exact tie carries none: the + // winner would be the order readdir happened to list the two in, and the + // loser is unlinked below. Refuse, as recovery already refuses a backup it + // cannot stat. + throw new AmbiguousBackupRecencyError( + absoluteFilePath, + backupStats[0].mtimeMs, + backupStats[0].backupPath, + backupStats[1].backupPath, + ) + } const newestBackupPath = backupStats[0].backupPath try { await fs.rename(newestBackupPath, absoluteFilePath)