fix(utils): refuse ambiguous backup recency in jsonFileRecovery

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))
+	})
 })

src/utils/jsonFileRecovery.ts

@@ -99,6 +99,36 @@ export async function deleteJsonFile(filePath: string): Promise<void> {
 	})
 }
 
+/**
+ * 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<void> {
  * 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)