From 7f4a40a58e2f88be65c60531919f87c53e154961 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 10 Nov 2026 08:31:56 +0800 Subject: [PATCH 1/2] fix(utils): refuse ambiguous backup recency in jsonFileRecovery 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 the previous code picked whichever readdir listed first and then garbage-collected the other one. That is a silent, order-dependent choice between two committed states, and the loser is deleted, so the evidence disappears with it. Recovery now refuses on an exact tie: it restores nothing and deletes nothing, and both copies stay on disk for a human to read. That is the same fail-closed decision recovery already makes for a backup it cannot stat, so it is a rule the file already states, applied to the one input where its recency signal says nothing. AmbiguousBackupRecencyError carries code AMBIGUOUS_BACKUP_RECENCY plus targetPath, mtimeMs and both backup paths; its message names basenames only, because error telemetry captures message and stack and a workspace path there 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. Proof, all five rows in jsonFileRecovery.test.ts: the tie between two backups rejects with AmbiguousBackupRecencyError / AMBIGUOUS_BACKUP_RECENCY and leaves both copies on disk with the canonical path still absent; the different-mtime pair still restores the newest backup and garbage-collects the loser, which is the contract the tie rows must not break; three backups whose tie sits BELOW the winner (200, 100, 100) still restore the newest and remove both losers, so the guard cannot be widened into 'any duplicate mtime means refuse'; the same tie driven through the reader entry point (recoverMissingJsonFile) rejects and reports through the persistence trace; and the error message names basenames only while the full paths stay on targetPath, firstBackupPath and secondBackupPath. Negative controls and the mutation band for this unit are recorded on the PR body with their exits and byte pairs, as is the price of the delta at the tip this branch was cut from. Unit U5c-2 of the planned split of #2357 (plan of record: https://github.com/Zoo-Code-Org/Zoo-Code/issues/1991#issuecomment-6751466344). Branch cut from main at d3d9cd48e630c91a85ad9ca2c8a1a7434f9d3e46; it does not depend on the fileLock unit that merges before it: this file only calls withFileLock, which exists at that tip. No changeset, no CHANGELOG row (AGENTS.md). Co-Authored-By: Claude --- src/utils/__tests__/jsonFileRecovery.test.ts | 129 ++++++++++++++++++- src/utils/jsonFileRecovery.ts | 47 ++++++- 2 files changed, 173 insertions(+), 3 deletions(-) diff --git a/src/utils/__tests__/jsonFileRecovery.test.ts b/src/utils/__tests__/jsonFileRecovery.test.ts index 1bd90d8935..57b2c33e9a 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,80 @@ 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) + expect(tie.firstBackupPath).toContain(dir) + expect(tie.secondBackupPath).toContain(dir) + }) }) 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) From e8dce4f5b4522ec3bd847968718467c996672f92 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 10 Nov 2026 09:01:00 +0800 Subject: [PATCH 2/2] test(utils): pin both tied backup paths on the tie error CodeRabbit's pre-merge walkthrough named lines 232-234 of the recovery spec: the two error properties were only checked to contain the directory, and nothing pinned that the message names the two backups it refused to choose between. The set comparison replaces the two toContain(dir) rows so the assertion holds whatever order readdir handed the tied pair in, and the two basename rows are the only rows that fail when the basenames drop out of the message. Production content is unchanged, so the counted-executable-line price does not move: 26 counted lines and 3 selectors against merge base d3d9cd48e630. Controls: pointing secondBackupPath at the first tied path fails the set row (exit 1, 1 failed / 14 passed); replacing both basenames in the message with a fixed phrase fails exactly the two basename rows (exit 1, 2 failed / 13 passed); restoring the production blob gives 15 passed (exit 0). Co-Authored-By: Claude --- src/utils/__tests__/jsonFileRecovery.test.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/utils/__tests__/jsonFileRecovery.test.ts b/src/utils/__tests__/jsonFileRecovery.test.ts index 57b2c33e9a..ce33e739b4 100644 --- a/src/utils/__tests__/jsonFileRecovery.test.ts +++ b/src/utils/__tests__/jsonFileRecovery.test.ts @@ -230,7 +230,10 @@ describe("reader recovery", () => { expect(tie.message).toContain("history_item.json") expect(tie.message).toContain("share mtime 1700000000000") expect(tie.targetPath).toBe(target) - expect(tie.firstBackupPath).toContain(dir) - expect(tie.secondBackupPath).toContain(dir) + // 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)) }) })