From 2d8fe7febcd0161b1df1827d720694f9ca479a8b Mon Sep 17 00:00:00 2001 From: Roomote Date: Wed, 21 Oct 2026 04:06:19 +0000 Subject: [PATCH] fix(persistence): keep ui_messages readable when a write is interrupted safeWriteJson replaced a file with two renames (target to backup, then replacement to target). A process exit between the renames left the canonical path absent, so readers saw a permanent ENOENT that no retry could repair. The replacement is now committed with a single atomic rename. Only a Windows-style EPERM (target held open by a reader) falls back to the two-step sequence, and a recovery pass under the advisory lock restores the newest orphaned backup and removes leftover temp files before any write. Task-message saves emit trace points with the task ID and process ID, the marker check and shutdown are traced, and a failed restart-persistence scenario now preserves phase results, VS Code logs, and task files for CI artifact upload. --- .github/workflows/e2e.yml | 12 ++ .gitignore | 1 + .../src/restart/scenarioWorkspace.ts | 53 +++++ apps/vscode-e2e/src/runTest.ts | 22 +- .../__tests__/persistenceTrace.spec.ts | 59 ++++++ .../__tests__/taskMessages.spec.ts | 41 +++- src/core/task-persistence/persistenceTrace.ts | 60 ++++++ .../readFileWithMissingRetry.ts | 8 +- src/core/task-persistence/taskMessages.ts | 39 ++-- src/extension.ts | 9 + src/extension/api.ts | 6 +- .../safeWriteJson.interruption.test.ts | 166 +++++++++++++++ src/utils/__tests__/safeWriteJson.test.ts | 186 +++++++++++++--- src/utils/safeWriteJson.ts | 199 +++++++++++++++--- 14 files changed, 780 insertions(+), 81 deletions(-) create mode 100644 src/core/task-persistence/__tests__/persistenceTrace.spec.ts create mode 100644 src/core/task-persistence/persistenceTrace.ts create mode 100644 src/utils/__tests__/safeWriteJson.interruption.test.ts diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 4b33fc678b..ba78065550 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -156,6 +156,18 @@ jobs: if: github.event_name != 'pull_request' || steps.e2e-marker.outputs.cache-hit != 'true' run: TEST_FILE=restart-persistence.test USE_MOCK=true xvfb-run -a pnpm --filter @roo-code/vscode-e2e exec node ./out/runTest.js + - name: Upload restart-persistence failure artifacts + # runTest.js preserves a failed scenario workspace (phase results, + # VS Code logs, task files) under apps/vscode-e2e/test-failures/ + # instead of deleting it, so the interrupted-write state from + # issue #2130-style failures stays inspectable. + if: failure() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: restart-persistence-failure + path: apps/vscode-e2e/test-failures/ + if-no-files-found: ignore + - name: Explain skipped mocked E2E pass marker if: steps.e2e-marker.outputs.cache-hit != 'true' && steps.run-e2e.outcome == 'success' && steps.vscode-fallback.outputs.used == 'true' run: echo "Skipping mocked E2E pass marker because tests ran against a stale cached VS Code binary (VS Code download endpoints unreachable)." diff --git a/.gitignore b/.gitignore index 3961778d5e..a6ad55ed8a 100644 --- a/.gitignore +++ b/.gitignore @@ -27,6 +27,7 @@ AGENTS.local.md .vscode-test/ apps/vscode-e2e/playwright-report-electron/ apps/vscode-e2e/test-results-electron/ +apps/vscode-e2e/test-failures/ # Docs docs/_site/ diff --git a/apps/vscode-e2e/src/restart/scenarioWorkspace.ts b/apps/vscode-e2e/src/restart/scenarioWorkspace.ts index 9ce49deaf0..b8b768ef31 100644 --- a/apps/vscode-e2e/src/restart/scenarioWorkspace.ts +++ b/apps/vscode-e2e/src/restart/scenarioWorkspace.ts @@ -56,3 +56,56 @@ export async function removeScenarioWorkspace(scenarioWorkspace: ScenarioWorkspa await fs.rm(root, { recursive: true, force: true }) } + +const FAILURE_ARTIFACTS_DIR = "test-failures" + +/** + * Copies the diagnosable parts of a failed scenario workspace into a stable + * directory inside the checkout, then removes the temporary scenario root. + * CI uploads that directory as a run artifact, so the failure that used to be + * deleted in the cleanup path stays inspectable: phase results, VS Code logs, + * and the extension's global storage with the task files. + * + * The directory is keyed by the unique scenario root name and cleared before + * copying, so retried runs cannot mix artifacts from different failures. + */ +export async function preserveScenarioWorkspace(scenarioWorkspace: ScenarioWorkspace): Promise { + const destinationRoot = path.resolve( + __dirname, + "../../", + FAILURE_ARTIFACTS_DIR, + path.basename(scenarioWorkspace.root), + ) + await fs.rm(destinationRoot, { recursive: true, force: true }) + await fs.mkdir(destinationRoot, { recursive: true }) + + const selections: Array<{ label: string; source: string; destination: string }> = [ + { + label: "phase results", + source: scenarioWorkspace.results, + destination: path.join(destinationRoot, "phase-results"), + }, + { + label: "VS Code logs", + source: path.join(scenarioWorkspace.userData, "logs"), + destination: path.join(destinationRoot, "vscode-logs"), + }, + { + label: "task files", + source: path.join(scenarioWorkspace.userData, "User", "globalStorage"), + destination: path.join(destinationRoot, "global-storage"), + }, + ] + + for (const selection of selections) { + try { + await fs.cp(selection.source, selection.destination, { recursive: true, force: true }) + } catch (error) { + // A missing subdirectory must not hide the other artifacts. + console.error(`Failed to preserve ${selection.label} from ${selection.source}:`, error) + } + } + + await removeScenarioWorkspace(scenarioWorkspace) + return destinationRoot +} diff --git a/apps/vscode-e2e/src/runTest.ts b/apps/vscode-e2e/src/runTest.ts index 88c687bc76..f24f37d388 100644 --- a/apps/vscode-e2e/src/runTest.ts +++ b/apps/vscode-e2e/src/runTest.ts @@ -21,7 +21,11 @@ import { addSearchFilesResultFixtures } from "./fixtures/search-files" import { addSubtaskFixtures } from "./fixtures/subtasks" import { addUseMcpToolResultFixtures } from "./fixtures/use-mcp-tool" import { addWriteToFileResultFixtures } from "./fixtures/write-to-file" -import { createScenarioWorkspace, removeScenarioWorkspace } from "./restart/scenarioWorkspace" +import { + createScenarioWorkspace, + preserveScenarioWorkspace, + removeScenarioWorkspace, +} from "./restart/scenarioWorkspace" import { runRestartScenario } from "./restart/vscodeCoordinator" function getCliFlagValue(flag: string) { @@ -95,6 +99,7 @@ async function main() { let testWorkspace: string | undefined let scenarioWorkspace: Awaited> | undefined + let runFailed = false try { // Create a temporary workspace folder for regular tests. Restart scenarios own @@ -218,6 +223,7 @@ async function main() { }) } } catch (error) { + runFailed = true console.error("Failed to run tests", error) process.exitCode = 1 } finally { @@ -225,7 +231,19 @@ async function main() { await fs.rm(testWorkspace, { recursive: true, force: true }) } if (scenarioWorkspace) { - await removeScenarioWorkspace(scenarioWorkspace) + // A failed restart scenario must stay inspectable: the phase + // results, VS Code logs, and task files are copied to a stable + // directory for CI artifact upload before the temp root goes away. + if (runFailed) { + try { + const preservedAt = await preserveScenarioWorkspace(scenarioWorkspace) + console.error(`Restart scenario workspace preserved for failure inspection: ${preservedAt}`) + } catch (preserveError) { + console.error("Failed to preserve the restart scenario workspace:", preserveError) + } + } else { + await removeScenarioWorkspace(scenarioWorkspace) + } } await mock?.stop() } diff --git a/src/core/task-persistence/__tests__/persistenceTrace.spec.ts b/src/core/task-persistence/__tests__/persistenceTrace.spec.ts new file mode 100644 index 0000000000..c4a58afce4 --- /dev/null +++ b/src/core/task-persistence/__tests__/persistenceTrace.spec.ts @@ -0,0 +1,59 @@ +import { describe, it, expect, vi, afterEach } from "vitest" + +import { setPersistenceTraceSink, tracePersistenceEvent } from "../persistenceTrace" + +function parseTraceLine(line: string): Record { + return JSON.parse(line.replace("[persistence-trace] ", "")) as Record +} + +describe("persistenceTrace", () => { + afterEach(() => { + setPersistenceTraceSink(undefined) + vi.restoreAllMocks() + }) + + it("emits a JSON line with the timestamp, process ID, stage, and fields to the sink", () => { + const lines: string[] = [] + setPersistenceTraceSink((line) => lines.push(line)) + + tracePersistenceEvent("ui-messages-write-start", { taskId: "task-1", filePath: "/tmp/ui_messages.json" }) + + expect(lines).toHaveLength(1) + const event = parseTraceLine(lines[0]) + expect(event.stage).toBe("ui-messages-write-start") + expect(event.taskId).toBe("task-1") + expect(event.pid).toBe(process.pid) + expect(typeof event.ts).toBe("string") + expect(event.filePath).toBe("/tmp/ui_messages.json") + }) + + it("marks the marker-check result", () => { + const lines: string[] = [] + setPersistenceTraceSink((line) => lines.push(line)) + + tracePersistenceEvent("ui-messages-marker-check", { taskId: "task-2", result: "match" }) + + const event = parseTraceLine(lines[0]) + expect(event.stage).toBe("ui-messages-marker-check") + expect(event.result).toBe("match") + }) + + it("falls back to the console when no sink is installed", () => { + const consoleSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + + tracePersistenceEvent("shutdown") + + expect(consoleSpy).toHaveBeenCalledTimes(1) + const line = String(consoleSpy.mock.calls[0]?.[0]) + expect(line).toContain("[persistence-trace]") + expect(parseTraceLine(line).stage).toBe("shutdown") + }) + + it("never lets a failing sink break the caller", () => { + setPersistenceTraceSink(() => { + throw new Error("sink failed") + }) + + expect(() => tracePersistenceEvent("ui-messages-marker-check", { result: "match" })).not.toThrow() + }) +}) diff --git a/src/core/task-persistence/__tests__/taskMessages.spec.ts b/src/core/task-persistence/__tests__/taskMessages.spec.ts index 6b968fe96c..5917997931 100644 --- a/src/core/task-persistence/__tests__/taskMessages.spec.ts +++ b/src/core/task-persistence/__tests__/taskMessages.spec.ts @@ -21,6 +21,7 @@ vi.mock("../../../utils/safeWriteJson", () => ({ // Import after mocks import { saveTaskMessages, readTaskMessages } from "../taskMessages" import { formatErrorDetail } from "../readFileWithMissingRetry" +import { setPersistenceTraceSink } from "../persistenceTrace" let tmpBaseDir: string @@ -35,6 +36,7 @@ beforeEach(async () => { afterEach(() => { vi.useRealTimers() vi.restoreAllMocks() + setPersistenceTraceSink(undefined) }) describe("taskMessages.saveTaskMessages", () => { @@ -63,7 +65,11 @@ describe("taskMessages.saveTaskMessages", () => { expect(hoisted.safeWriteJsonMock).toHaveBeenCalledTimes(1) const [, persisted] = hoisted.safeWriteJsonMock.mock.calls[0] expect(persisted).toEqual(messages) - expect(hoisted.safeWriteJsonMock.mock.calls[0][2]).toBeUndefined() + const options = hoisted.safeWriteJsonMock.mock.calls[0][2] as { merge?: unknown; trace?: unknown } | undefined + expect(options?.merge).toBeUndefined() + // The trace callback is always installed so restart logs can follow + // the write stages. + expect(options?.trace).toBeTypeOf("function") }) it("persists messages without modification when no metadata", async () => { @@ -101,6 +107,39 @@ describe("taskMessages.saveTaskMessages", () => { ]) }) + it("emits persistence trace events with the task ID for the write stages", async () => { + const traceLines: string[] = [] + hoisted.safeWriteJsonMock.mockImplementationOnce(async (_path, _data, options) => { + for (const stage of ["recover", "write-start", "backup-rename", "replacement-rename", "write-complete"]) { + options.trace({ stage, detail: stage === "backup-rename" ? "eperm-fallback" : undefined }) + } + }) + setPersistenceTraceSink((line) => traceLines.push(line)) + + await saveTaskMessages({ + messages: [{ ts: 1, type: "say", say: "text", text: "traced" }], + taskId: "task-trace", + globalStoragePath: tmpBaseDir, + }) + + const events = traceLines.map( + (line) => JSON.parse(line.replace("[persistence-trace] ", "")) as Record, + ) + expect(events.map((event) => event.stage)).toEqual([ + "ui-messages-recovery", + "ui-messages-write-start", + "ui-messages-backup-rename", + "ui-messages-replacement-rename", + "ui-messages-write-complete", + ]) + for (const event of events) { + expect(event.taskId).toBe("task-trace") + expect(event.pid).toBe(process.pid) + } + const backupRename = events.find((event) => event.stage === "ui-messages-backup-rename") + expect(backupRename?.detail).toBe("eperm-fallback") + }) + it("propagates a forced write failure without swallowing or replacing it", async () => { // Fault injection: simulate an interrupted/failed final write (e.g. the // extension host quitting mid-save or the disk refusing the write). The diff --git a/src/core/task-persistence/persistenceTrace.ts b/src/core/task-persistence/persistenceTrace.ts new file mode 100644 index 0000000000..06b85716e8 --- /dev/null +++ b/src/core/task-persistence/persistenceTrace.ts @@ -0,0 +1,60 @@ +/** + * Structured trace points for task-persistence file writes and shutdown. + * + * The restart-persistence investigation in issue #2130 could not tell which + * writer touched ui_messages.json or when, because nothing logged the + * replacement-protocol steps. These trace lines record the write stages, the + * marker check, and shutdown with the task ID and the process ID, so CI logs + * and extension logs can order writer activity across a restart. + * + * The default sink is the console. Extension activation installs a sink that + * also mirrors every line into the extension output channel; tests can + * install their own sink to capture events. Tracing must never break + * persistence, so sink failures are swallowed. + */ + +export type PersistenceTraceStage = + | "ui-messages-recovery" + | "ui-messages-write-start" + | "ui-messages-backup-rename" + | "ui-messages-replacement-rename" + | "ui-messages-write-complete" + | "ui-messages-marker-check" + | "shutdown" + +export type PersistenceTraceFields = { + taskId?: string + filePath?: string + result?: string + detail?: string +} + +type PersistenceTraceSink = (line: string) => void + +let traceSink: PersistenceTraceSink | undefined + +/** + * Installs the sink used for every subsequent trace event. Pass undefined to + * restore the default console sink. + */ +export function setPersistenceTraceSink(sink: PersistenceTraceSink | undefined): void { + traceSink = sink +} + +export function tracePersistenceEvent(stage: PersistenceTraceStage, fields: PersistenceTraceFields = {}): void { + const line = `[persistence-trace] ${JSON.stringify({ + ts: new Date().toISOString(), + pid: process.pid, + stage, + ...fields, + })}` + try { + if (traceSink) { + traceSink(line) + } else { + console.log(line) + } + } catch { + // A failing sink must never break the write, read, or shutdown path. + } +} diff --git a/src/core/task-persistence/readFileWithMissingRetry.ts b/src/core/task-persistence/readFileWithMissingRetry.ts index 1036049470..67db76d0d0 100644 --- a/src/core/task-persistence/readFileWithMissingRetry.ts +++ b/src/core/task-persistence/readFileWithMissingRetry.ts @@ -1,8 +1,10 @@ import * as fs from "fs/promises" -// safeWriteJson replaces a file in two renames (target to backup, new file to -// target) because a single rename over an open file fails with EPERM on -// Windows. A reader can see ENOENT between the two renames, so retry once. +// safeWriteJson commits a replacement with a single atomic rename. Only the +// Windows EPERM fallback replaces a file in two renames (target to backup, +// new file to target), because a single rename over an open file fails with +// EPERM on Windows. A reader can see ENOENT between those two renames, so +// retry once. const READ_RETRY_MIN_MS = 1 const READ_RETRY_MAX_MS = 10 const READ_RETRY_RANGE_MS = READ_RETRY_MAX_MS - READ_RETRY_MIN_MS + 1 diff --git a/src/core/task-persistence/taskMessages.ts b/src/core/task-persistence/taskMessages.ts index b67ad2b7f8..bb89bbf4f2 100644 --- a/src/core/task-persistence/taskMessages.ts +++ b/src/core/task-persistence/taskMessages.ts @@ -2,11 +2,20 @@ import * as path from "path" import type { ClineMessage } from "@roo-code/types" -import { safeWriteJson } from "../../utils/safeWriteJson" +import { safeWriteJson, type SafeWriteJsonTraceStage } from "../../utils/safeWriteJson" import { GlobalFileNames } from "../../shared/globalFileNames" import { getTaskDirectoryPath } from "../../utils/storage" import { ensureMessageIdentifiers, mergeClineMessageSnapshots } from "./mergeMessageSnapshots" import { formatErrorDetail, getErrorCode, readFileWithMissingRetry } from "./readFileWithMissingRetry" +import { tracePersistenceEvent, type PersistenceTraceStage } from "./persistenceTrace" + +const uiMessagesTraceStages: Record = { + recover: "ui-messages-recovery", + "write-start": "ui-messages-write-start", + "backup-rename": "ui-messages-backup-rename", + "replacement-rename": "ui-messages-replacement-rename", + "write-complete": "ui-messages-write-complete", +} export type TaskMessagesReadErrorKind = "not_found" | "invalid" | "io_error" @@ -83,17 +92,21 @@ export async function saveTaskMessages({ const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId) const filePath = path.join(taskDir, GlobalFileNames.uiMessages) let savedMessages = messages - await safeWriteJson( - filePath, - messages, - merge - ? { - merge: (existing, incoming) => { - savedMessages = mergeClineMessageSnapshots(existing, incoming) as ClineMessage[] - return savedMessages - }, - } - : undefined, - ) + await safeWriteJson(filePath, messages, { + ...(merge && { + merge: (existing, incoming) => { + savedMessages = mergeClineMessageSnapshots(existing, incoming) as ClineMessage[] + return savedMessages + }, + }), + // Trace the replacement-protocol stages with the task ID so restart + // logs can order UI-message writer activity across processes. + trace: (event) => + tracePersistenceEvent(uiMessagesTraceStages[event.stage], { + taskId, + filePath, + detail: event.detail, + }), + }) return savedMessages } diff --git a/src/extension.ts b/src/extension.ts index 6ce42aacd5..e6dce2f668 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -27,6 +27,7 @@ import { initializeNetworkProxy } from "./utils/networkProxy" import { Package } from "./shared/package" import { formatLanguage } from "./shared/language" import { ContextProxy } from "./core/config/ContextProxy" +import { setPersistenceTraceSink, tracePersistenceEvent } from "./core/task-persistence/persistenceTrace" import { ClineProvider } from "./core/webview/ClineProvider" import { registerCopilotModelBroadcast } from "./services/language-models/copilotModelBroadcast" import { ClineProviderFactory } from "./core/webview/ClineProviderFactory" @@ -125,6 +126,13 @@ export async function activate(context: vscode.ExtensionContext) { extensionContext = context outputChannel = vscode.window.createOutputChannel(Package.outputChannel) context.subscriptions.push(outputChannel) + // Mirror persistence trace lines (task ID + process ID) into the output + // channel and the console, so a restart-persistence investigation can + // order UI-message writer activity across extension-host restarts. + setPersistenceTraceSink((line) => { + outputChannel.appendLine(line) + console.log(line) + }) outputChannel.appendLine(`${Package.name} extension activated - ${JSON.stringify(Package)}`) // Initialize network proxy configuration early, before any network requests. @@ -402,6 +410,7 @@ export async function activate(context: vscode.ExtensionContext) { // This method is called when your extension is deactivated. export async function deactivate() { + tracePersistenceEvent("shutdown") outputChannel.appendLine(`${Package.name} extension deactivated`) if (cloudService && CloudService.hasInstance()) { diff --git a/src/extension/api.ts b/src/extension/api.ts index 6a8ed8dfdd..d86dd7f3e1 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -29,6 +29,7 @@ import type { Mode } from "../shared/modes" import type { ClineProvider } from "../core/webview/ClineProvider" import type { ClineProviderFactory } from "../core/webview/ClineProviderFactory" import { readTaskMessages } from "../core/task-persistence/taskMessages" +import { tracePersistenceEvent } from "../core/task-persistence/persistenceTrace" import { Terminal } from "../integrations/terminal/Terminal" import { TerminalRegistry } from "../integrations/terminal/TerminalRegistry" import { getCommands } from "../services/command/commands" @@ -317,9 +318,12 @@ export class API extends EventEmitter implements RooCodeAPI { globalStoragePath: this.sidebarProvider.contextProxy.globalStorageUri.fsPath, }) } catch { + tracePersistenceEvent("ui-messages-marker-check", { taskId, result: "read-error" }) return false } - return uiMessages.some((message) => message.text?.includes(text)) + const hasText = uiMessages.some((message) => message.text?.includes(text)) + tracePersistenceEvent("ui-messages-marker-check", { taskId, result: hasText ? "match" : "no-match" }) + return hasText } public getCurrentTaskStack() { diff --git a/src/utils/__tests__/safeWriteJson.interruption.test.ts b/src/utils/__tests__/safeWriteJson.interruption.test.ts new file mode 100644 index 0000000000..9dc142941c --- /dev/null +++ b/src/utils/__tests__/safeWriteJson.interruption.test.ts @@ -0,0 +1,166 @@ +import { spawn, type ChildProcess } from "child_process" +import { createRequire } from "module" +import { fileURLToPath } from "url" +import * as fs from "fs/promises" +import * as os from "os" +import * as path from "path" + +import { safeWriteJson } from "../safeWriteJson" +import { readFileWithMissingRetry } from "../../core/task-persistence/readFileWithMissingRetry" + +const requireFromTest = createRequire(import.meta.url) +const esbuildPath = requireFromTest.resolve("esbuild") +const safeWriteJsonEntryPath = fileURLToPath(new URL("../safeWriteJson.ts", import.meta.url)) + +// The child performs a real safeWriteJson from this repository. The source is +// bundled to CommonJS at child startup, so the bundle's require("fs/promises") +// resolves to the same exports object the script patched above, and the +// patched rename is the one the writer actually calls. +const CHILD_SCRIPT = `"use strict" +const fs = require("fs") +const path = require("path") + +const targetPath = path.resolve(process.env.INTERRUPTION_TARGET_PATH) +const fsPromises = require("fs/promises") +const actualRename = fsPromises.rename + +// Fault injection: die with code 0 before the replacement lands. Any rename +// whose destination is the canonical path is the commit step, whatever +// replacement protocol the writer uses; exit before performing it. +fsPromises.rename = function interruptionRename(oldPath, newPath) { + if (path.resolve(String(newPath)) === targetPath) { + fs.writeSync(1, "EXIT_BEFORE_REPLACEMENT_RENAME\\n") + process.exit(0) + } + return actualRename.apply(fsPromises, arguments) +} + +require(process.env.ESBUILD_PATH).buildSync({ + entryPoints: [process.env.SAFE_WRITE_JSON_ENTRY], + bundle: true, + platform: "node", + format: "cjs", + target: "node22", + outfile: process.env.INTERRUPTION_BUNDLE_PATH, + logLevel: "silent", +}) + +const payload = JSON.parse(process.env.INTERRUPTION_PAYLOAD) +require(process.env.INTERRUPTION_BUNDLE_PATH) + .safeWriteJson(targetPath, payload) + .then(() => { + fs.writeSync(1, "WRITE_COMPLETED\\n") + process.exit(0) + }) + .catch((error) => { + fs.writeSync(2, "WRITE_FAILED: " + ((error && error.message) || String(error)) + "\\n") + process.exit(1) + }) +` + +async function waitForChildExit(child: ChildProcess): Promise<{ code: number | null; stdout: string; stderr: string }> { + const stdoutChunks: Buffer[] = [] + const stderrChunks: Buffer[] = [] + child.stdout?.on("data", (chunk: Buffer) => stdoutChunks.push(chunk)) + child.stderr?.on("data", (chunk: Buffer) => stderrChunks.push(chunk)) + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + child.kill("SIGKILL") + reject(new Error("Interruption child did not exit within 20 seconds")) + }, 20_000) + child.once("error", reject) + child.once("close", (code) => { + clearTimeout(timeout) + resolve({ + code, + stdout: Buffer.concat(stdoutChunks).toString("utf8"), + stderr: Buffer.concat(stderrChunks).toString("utf8"), + }) + }) + }) +} + +describe("safeWriteJson subprocess interruption fault injection", () => { + let tempDir: string + let storageDir: string + let childDir: string + let targetPath: string + let childScriptPath: string + let childBundlePath: string + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-interruption-")) + storageDir = path.join(tempDir, "storage") + childDir = path.join(tempDir, "child") + await fs.mkdir(storageDir) + await fs.mkdir(childDir) + targetPath = path.join(storageDir, "ui_messages.json") + childScriptPath = path.join(childDir, "interrupted-writer.cjs") + childBundlePath = path.join(childDir, "safeWriteJson.bundle.cjs") + await fs.writeFile(childScriptPath, CHILD_SCRIPT, "utf8") + }) + + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }) + }) + + it.each([1, 2, 3])( + "keeps the committed file readable when the writer dies before the replacement lands (run %i)", + async (run) => { + // A previously committed write contains the restart marker. + const committed = { ui_messages: [{ ts: 1, type: "say", text: "RESTART_PERSISTENCE_MARKER" }] } + await fs.writeFile(targetPath, JSON.stringify(committed), "utf8") + + // The condition introduced by the previous fix: the committed + // content is readable before the interrupted write starts. + const before = await readFileWithMissingRetry(targetPath) + expect(before).toContain("RESTART_PERSISTENCE_MARKER") + + // A real child process writes the same path and terminates right + // before the replacement rename, mirroring the reported crash + // window between the two renames of the old protocol. + const child = spawn(process.execPath, [childScriptPath], { + env: { + ...process.env, + INTERRUPTION_TARGET_PATH: targetPath, + INTERRUPTION_PAYLOAD: JSON.stringify({ + ui_messages: [{ ts: 2, type: "say", text: `INTERRUPTED_WRITE_${run}` }], + }), + ESBUILD_PATH: esbuildPath, + SAFE_WRITE_JSON_ENTRY: safeWriteJsonEntryPath, + INTERRUPTION_BUNDLE_PATH: childBundlePath, + }, + stdio: ["ignore", "pipe", "pipe"], + }) + const exit = await waitForChildExit(child) + + expect(exit.stdout).toContain("EXIT_BEFORE_REPLACEMENT_RENAME") + expect(exit.stdout).not.toContain("WRITE_COMPLETED") + expect(exit.stderr).not.toContain("WRITE_FAILED") + expect(exit.code).toBe(0) + + // The writer terminated before the replacement rename, so the + // canonical path must still hold the last committed content. This + // is the permanent-ENOENT regression: with the old two-rename + // protocol the committed target was already renamed away, and no + // read retry could bring it back. + const after = JSON.parse(await readFileWithMissingRetry(targetPath)) + expect(after).toEqual(committed) + + // Restart recovery: remove the advisory lock file orphaned by the + // dead writer (proper-lockfile would only treat it as stale after + // 31 seconds), then a fresh write must succeed and consume the + // interrupted write's leftovers. + await fs.rm(`${targetPath}.lock`, { force: true }) + const recovered = { ui_messages: [{ ts: 3, type: "say", text: "RECOVERED_AFTER_RESTART" }] } + await safeWriteJson(targetPath, recovered) + + expect(JSON.parse(await readFileWithMissingRetry(targetPath))).toEqual(recovered) + const leftovers = (await fs.readdir(storageDir)).filter( + (entry) => entry.includes(".new_") || entry.includes(".bak_"), + ) + expect(leftovers).toEqual([]) + }, + 30_000, + ) +}) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 0ebdea281d..3bb22da3f1 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -158,26 +158,33 @@ describe("safeWriteJson", () => { expect(content).toEqual({ initial: "content" }) }) - test("should handle failure when renaming filePath to tempBackupFilePath (filePath exists)", async () => { - const initialData = { message: "Initial content, should remain" } - const newData = { message: "New content, should not be written" } + test("should fall back to the two-step replacement when the commit rename fails with EPERM", async () => { + const initialData = { message: "Initial content" } + const newData = { message: "Windows-style EPERM replacement" } // Overwrite the pre-created file with specific initial data await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // First call: the single-rename commit fails with the Windows-style EPERM + // (a reader holds the target open). The fallback then renames the target + // to a backup and the replacement into place, using real renames. vi.mocked(fs.rename).mockImplementationOnce(async () => { - throw new Error("Rename to backup failed") + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) }) - await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Rename to backup failed") + await safeWriteJson(currentTestFilePath, newData) - // Verify the original file still exists with initial content + // Verify the new content was committed by the fallback const content = await readFileContent(currentTestFilePath) - expect(content).toEqual(initialData) + expect(content).toEqual(newData) + + // Verify the fallback backup was cleaned up + const leftovers = (await fs.readdir(tempDir)).filter((entry) => entry.includes(".bak_")) + expect(leftovers).toEqual([]) }) - test("should handle failure when renaming tempNewFilePath to filePath (filePath exists, backup succeeded)", async () => { + test("should restore the committed file when the EPERM fallback's replacement rename fails", async () => { const initialData = { message: "Initial content, should be restored" } const newData = { message: "New content" } @@ -191,20 +198,21 @@ describe("safeWriteJson", () => { vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { renameCallCount++ if (renameCallCount === 1) { - // First call: filePath -> tempBackupFilePath (should succeed) - return fsPromisesActuals.rename!(oldPath, newPath) + // First call: the single-rename commit fails with EPERM, which + // triggers the two-step fallback. + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) } else if (renameCallCount === 2) { - // Second call: tempNewFilePath -> filePath (should fail) - throw new Error("Rename from temp to final failed") - } else if (renameCallCount === 3) { - // Third call: tempBackupFilePath -> filePath (rollback, should succeed) + // Second call: filePath -> tempBackupFilePath (fallback backup, succeeds) return fsPromisesActuals.rename!(oldPath, newPath) + } else if (renameCallCount === 3) { + // Third call: tempNewFilePath -> filePath (fallback replacement, fails) + throw new Error("Fallback replacement rename failed") } - // Default: use original implementation + // Fourth call: tempBackupFilePath -> filePath (rollback, succeeds) return fsPromisesActuals.rename!(oldPath, newPath) }) - await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Rename from temp to final failed") + await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Fallback replacement rename failed") // Verify the file was restored to initial content const content = await readFileContent(currentTestFilePath) @@ -292,13 +300,18 @@ describe("safeWriteJson", () => { expect(content).toEqual(data) }) - test("should handle failure when deleting tempBackupFilePath (filePath exists, all renames succeed)", async () => { + test("should handle failure when deleting tempBackupFilePath (EPERM fallback, all renames succeed)", async () => { const initialData = { message: "Initial content" } const newData = { message: "Successfully written new content" } // Overwrite the pre-created file with specific initial data await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) + // Force the two-step fallback so a backup exists, then let its deletion fail + vi.mocked(fs.rename).mockImplementationOnce(async () => { + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + }) + // fs.unlink is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.unlink).mockImplementationOnce(async () => { throw new Error("Failed to delete backup file") @@ -320,6 +333,11 @@ describe("safeWriteJson", () => { await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) + // Force the two-step fallback so a backup exists, then fail its deletion + vi.mocked(fs.rename).mockImplementationOnce(async () => { + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + }) + // fs.unlink is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.unlink).mockImplementation(async (filePath: any) => { if (filePath.toString().includes(".bak_")) { @@ -338,7 +356,7 @@ describe("safeWriteJson", () => { }) // The expected error message might need to change if the mock behaves differently. - test("should handle failure when renaming tempNewFilePath to filePath (filePath initially exists)", async () => { + test("should keep the committed file when the commit rename fails with a non-EPERM error", async () => { // currentTestFilePath exists due to beforeEach. const initialData = { message: "Initial content" } const newData = { message: "New content" } @@ -346,20 +364,14 @@ describe("safeWriteJson", () => { await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn - let renameCallCount = 0 - vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { - renameCallCount++ - if (renameCallCount === 2) { - // Second call: tempNewFilePath -> filePath (should fail) - throw new Error("Rename failed") - } - // For all other calls, use the original implementation - return fsPromisesActuals.rename!(oldPath, newPath) + vi.mocked(fs.rename).mockImplementationOnce(async () => { + throw new Error("Rename failed") }) await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Rename failed") - // The file should be restored to its initial content + // No backup is made on the single-rename path, and the committed file + // keeps its previous content const content = await readFileContent(currentTestFilePath) expect(content).toEqual(initialData) }) @@ -447,18 +459,26 @@ describe("safeWriteJson", () => { let renameCallCount = 0 vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { renameCallCount++ - if (renameCallCount === 2) { - // Second call: tempNewFilePath -> filePath (fail) - throw new Error("Primary rename failed") + if (renameCallCount === 1) { + // First call: the single-rename commit fails with EPERM, which + // triggers the two-step fallback. + throw Object.assign(new Error("Primary rename failed"), { code: "EPERM" }) + } else if (renameCallCount === 2) { + // Second call: filePath -> tempBackupFilePath (fallback backup, succeeds) + return fsPromisesActuals.rename!(oldPath, newPath) } else if (renameCallCount === 3) { - // Third call: tempBackupFilePath -> filePath (rollback, also fail) + // Third call: tempNewFilePath -> filePath (fallback replacement, fails) + throw new Error("Fallback replacement rename failed") + } else if (renameCallCount === 4) { + // Fourth call: tempBackupFilePath -> filePath (rollback, also fails) throw new Error("Rollback rename failed") } return fsPromisesActuals.rename!(oldPath, newPath) }) - // Should throw the original error, not the rollback error - await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Primary rename failed") + // The fallback's replacement error rejects the write; the rollback error + // is only logged. + await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Fallback replacement rename failed") // Verify console.error was called for the rollback failure expect(consoleErrorSpy).toHaveBeenCalledWith( @@ -542,4 +562,102 @@ describe("safeWriteJson", () => { const content = await readFileContent(currentTestFilePath) expect(content).toEqual({ c: 3 }) }) + + // Interrupted-write recovery tests + test("should restore an orphaned backup when the canonical path is missing after an interrupted write", async () => { + const committedData = { message: "Committed before the interruption" } + + // Simulate the crash state of the two-step replacement: the target was + // renamed to a backup, the replacement never landed, and the + // replacement temp file was left behind. The canonical path is absent. + await fsPromisesActuals.unlink!(currentTestFilePath) + const backupPath = path.join(tempDir, `.${path.basename(currentTestFilePath)}.bak_1_abc.tmp`) + const orphanedNewPath = path.join(tempDir, `.${path.basename(currentTestFilePath)}.new_1_def.tmp`) + await fsPromisesActuals.writeFile!(backupPath, JSON.stringify(committedData)) + await fsPromisesActuals.writeFile!(orphanedNewPath, "{}") + + // A merge write must observe the restored committed content, not null. + const incoming = { message: "New content" } + let mergedWith: unknown + await safeWriteJson(currentTestFilePath, incoming, { + merge: (existing) => { + mergedWith = existing + return incoming + }, + }) + + expect(mergedWith).toEqual(committedData) + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual(incoming) + + // The restored backup is consumed and the leftover replacement file is + // garbage collected. + const leftovers = (await fs.readdir(tempDir)).filter( + (entry) => entry.includes(".new_") || entry.includes(".bak_"), + ) + expect(leftovers).toEqual([]) + }) + + test("should garbage collect orphaned replacement files when the canonical path exists", async () => { + const initialData = { message: "Initial content" } + + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) + + // Leftovers from an interrupted write whose commit rename never ran. + const backupPath = path.join(tempDir, `.${path.basename(currentTestFilePath)}.bak_1_abc.tmp`) + const orphanedNewPath = path.join(tempDir, `.${path.basename(currentTestFilePath)}.new_1_def.tmp`) + await fsPromisesActuals.writeFile!(backupPath, JSON.stringify({ message: "stale backup" })) + await fsPromisesActuals.writeFile!(orphanedNewPath, "{}") + + const newData = { message: "New content" } + await safeWriteJson(currentTestFilePath, newData) + + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual(newData) + + const leftovers = (await fs.readdir(tempDir)).filter( + (entry) => entry.includes(".new_") || entry.includes(".bak_"), + ) + expect(leftovers).toEqual([]) + }) + + test("should report replacement-protocol trace events when a trace callback is provided", async () => { + const traceEvents: Array<{ stage: string; detail?: string }> = [] + const newData = { message: "traced write" } + + await safeWriteJson(currentTestFilePath, newData, { + trace: (event) => traceEvents.push(event), + }) + + expect(traceEvents.map((event) => event.stage)).toEqual(["write-start", "replacement-rename", "write-complete"]) + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual(newData) + }) + + test("should trace the EPERM fallback backup rename", async () => { + const traceEvents: Array<{ stage: string; detail?: string }> = [] + const initialData = { message: "Initial content" } + const newData = { message: "Traced fallback write" } + + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) + + vi.mocked(fs.rename).mockImplementationOnce(async () => { + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + }) + + await safeWriteJson(currentTestFilePath, newData, { + trace: (event) => traceEvents.push(event), + }) + + expect(traceEvents.map((event) => event.stage)).toEqual([ + "write-start", + "replacement-rename", + "backup-rename", + "replacement-rename", + "write-complete", + ]) + expect(traceEvents.find((event) => event.stage === "backup-rename")?.detail).toBe("eperm-fallback") + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual(newData) + }) }) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 7da68b2a7a..b7274125bf 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -5,6 +5,17 @@ import { JsonStreamStringify } from "json-stream-stringify" import { acquireFileLock } from "./fileLock" +/** + * Stages reported through the optional `trace` callback. They mark the + * replacement-protocol steps that matter when diagnosing interrupted or + * failing writes: the recovery pass, the temp-file write, the backup rename + * of the EPERM fallback, the commit rename, and completion. + */ +export type SafeWriteJsonTraceStage = + "recover" | "write-start" | "backup-rename" | "replacement-rename" | "write-complete" + +export type SafeWriteJsonTrace = (event: { stage: SafeWriteJsonTraceStage; detail?: string }) => void + /** * Options for safeWriteJson function */ @@ -26,14 +37,118 @@ export interface SafeWriteJsonOptions { * cannot be parsed. */ merge?: (existing: unknown, incoming: unknown) => unknown + + /** + * When provided, receives one event per replacement-protocol step (see + * SafeWriteJsonTraceStage). Callers such as the task-message save use it + * to emit diagnostic trace lines; it never influences the write itself. + */ + trace?: SafeWriteJsonTrace +} + +function createTempFilePath(absoluteFilePath: string, kind: "new" | "bak"): string { + return path.join( + path.dirname(absoluteFilePath), + `.${path.basename(absoluteFilePath)}.${kind}_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, + ) +} + +function getTempFilePrefix(absoluteFilePath: string, kind: "new" | "bak"): string { + return `.${path.basename(absoluteFilePath)}.${kind}_` +} + +/** + * 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 + * backup files are only ever created while that lock is held, so anything + * matching their name patterns belongs to a writer that can no longer + * commit or clean it up (a crashed process, or a stale lock that was + * stolen). + * + * The two-step replacement used by the EPERM fallback (and by versions + * before the single-rename commit) can be interrupted after the target was + * 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. + * + * @returns Whether leftover files were found, and the backup path that was + * restored, if any. + */ +async function recoverInterruptedReplacement( + absoluteFilePath: string, +): Promise<{ foundLeftovers: boolean; restoredFrom: string | null }> { + const dirPath = path.dirname(absoluteFilePath) + const newPrefix = getTempFilePrefix(absoluteFilePath, "new") + const backupPrefix = getTempFilePrefix(absoluteFilePath, "bak") + + let entries: string[] + try { + entries = await fs.readdir(dirPath) + } catch { + // The directory is unreadable or was removed; the write itself + // reports that error below. + return { foundLeftovers: false, restoredFrom: null } + } + + const orphanedPaths = entries + .filter((entry) => entry.endsWith(".tmp") && (entry.startsWith(newPrefix) || entry.startsWith(backupPrefix))) + .map((entry) => path.join(dirPath, entry)) + + if (orphanedPaths.length === 0) { + return { foundLeftovers: false, restoredFrom: null } + } + + let restoredFrom: string | null = null + + try { + await fs.access(absoluteFilePath) + } catch { + // The canonical path is absent. An interrupted two-step replacement + // leaves the committed content in the newest backup, so restore it + // and keep readers on the last committed state. + const backupStats = await Promise.all( + orphanedPaths + .filter((orphanPath) => path.basename(orphanPath).startsWith(backupPrefix)) + .map(async (backupPath) => ({ backupPath, mtimeMs: (await fs.stat(backupPath)).mtimeMs })), + ) + if (backupStats.length > 0) { + backupStats.sort((a, b) => b.mtimeMs - a.mtimeMs) + const newestBackupPath = backupStats[0].backupPath + try { + await fs.rename(newestBackupPath, absoluteFilePath) + restoredFrom = newestBackupPath + } catch (restoreError) { + console.error( + `Failed to restore interrupted-write backup ${newestBackupPath} to ${absoluteFilePath}:`, + restoreError, + ) + } + } + } + + for (const orphanPath of orphanedPaths) { + if (orphanPath === restoredFrom) { + continue + } + try { + await fs.unlink(orphanPath) + } catch (cleanupError) { + console.error(`Failed to clean up interrupted-write leftover ${orphanPath}:`, cleanupError) + } + } + + return { foundLeftovers: true, restoredFrom } } /** * Safely writes JSON data to a file. * - Creates parent directories if they don't exist * - Uses 'proper-lockfile' for inter-process advisory locking to prevent concurrent writes to the same path. - * - Writes to a temporary file first. - * - If the target file exists, it's backed up before being replaced. + * - Recovers an interrupted previous write before touching the file: restores an orphaned backup when the canonical path is absent, and removes leftover temp files. + * - Writes to a temporary file first, then commits it with a single atomic rename, so the committed target stays readable through an interrupted write. + * - When a single rename over the target fails with EPERM (a reader holds the file open on Windows), falls back to backing the target up before replacing it. * - Attempts to roll back and clean up in case of errors. * - Supports pretty-printing with indentation while maintaining streaming efficiency. * @@ -76,6 +191,14 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso let actualTempBackupFilePath: string | null = null try { + // Heal any state an interrupted previous write left behind: an absent + // canonical path with an orphaned backup, or leftover replacement + // files. Runs first so a merge callback reads the restored content. + const recovery = await recoverInterruptedReplacement(absoluteFilePath) + if (recovery.foundLeftovers) { + options?.trace?.({ stage: "recover", detail: recovery.restoredFrom ?? "cleaned-leftovers" }) + } + // If a merge callback was provided, read the current file under the lock // and let the caller merge before we write. Must be inside try/finally // so a throwing merge still releases the lock. @@ -93,43 +216,46 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso data = options.merge(existing, data) } - // Step 1: Write data to a new temporary file. - actualTempNewFilePath = path.join( - path.dirname(absoluteFilePath), - `.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, - ) + // Step 1: Write data to a new temporary file and flush it to storage, + // so the replacement content is durable before it is committed. + options?.trace?.({ stage: "write-start" }) + actualTempNewFilePath = createTempFilePath(absoluteFilePath, "new") await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint) + await _flushFileToDisk(actualTempNewFilePath) - // Step 2: Check if the target file exists. If so, rename it to a backup path. + // Step 2: Commit the replacement with a single atomic rename. The + // committed target stays in place until this rename lands, so a + // process exit before or during the commit can no longer leave the + // canonical path absent. try { - // Check for target file existence - await fs.access(absoluteFilePath) - // Target exists, create a backup path and rename. - actualTempBackupFilePath = path.join( - path.dirname(absoluteFilePath), - `.${path.basename(absoluteFilePath)}.bak_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, - ) - await fs.rename(absoluteFilePath, actualTempBackupFilePath) - } catch (accessError: any) { - // Explicitly type accessError - if (accessError.code !== "ENOENT") { - // An error other than "file not found" occurred during access check. - throw accessError + options?.trace?.({ stage: "replacement-rename" }) + await fs.rename(actualTempNewFilePath, absoluteFilePath) + } catch (renameError: any) { + if (renameError?.code !== "EPERM") { + throw renameError } - // Target file does not exist, so no backup is made. actualTempBackupFilePath remains null. - } - // Step 3: Rename the new temporary file to the target file path. - // This is the main "commit" step. - await fs.rename(actualTempNewFilePath, absoluteFilePath) + // A single rename over a file that a reader holds open fails with + // EPERM on Windows, so fall back to the two-step replacement: + // rename the target to a backup, then the replacement into place. + // The rollback in the catch below restores the backup when the + // second rename fails, and the recovery pass at the start of the + // next write repairs a crash between the two renames. + options?.trace?.({ stage: "backup-rename", detail: "eperm-fallback" }) + actualTempBackupFilePath = createTempFilePath(absoluteFilePath, "bak") + await fs.rename(absoluteFilePath, actualTempBackupFilePath) + options?.trace?.({ stage: "replacement-rename", detail: "eperm-fallback" }) + await fs.rename(actualTempNewFilePath, absoluteFilePath) + } // If we reach here, the new file is successfully in place. // The original actualTempNewFilePath is now the main file, so we shouldn't try to clean it up as "temp". // Mark as "used" or "committed" actualTempNewFilePath = null + options?.trace?.({ stage: "write-complete" }) - // Step 4: If a backup was created, attempt to delete it. + // Step 3: If a backup was created, attempt to delete it. if (actualTempBackupFilePath) { try { await fs.unlink(actualTempBackupFilePath) @@ -230,4 +356,23 @@ async function _streamDataToFile(targetPath: string, data: any, prettyPrint = fa }) } +/** + * Flushes a fully written temp file to storage before it is committed. + * Best effort: durability against power loss is a bonus here, process-crash + * safety comes from the rename protocol itself, and a filesystem that + * refuses fsync must not block the write. + */ +async function _flushFileToDisk(targetPath: string): Promise { + try { + const handle = await fs.open(targetPath, "r") + try { + await handle.sync() + } finally { + await handle.close() + } + } catch (flushError) { + console.error(`Failed to flush ${targetPath} to storage before commit:`, flushError) + } +} + export { safeWriteJson }