From d183e29c8cdfad47dd7604c3513d19adb46566f1 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sun, 6 Sep 2026 04:03:38 +0800 Subject: [PATCH] refactor(file-safety): delegate safeWriteJson to safeWriteText atomic publish (S3 v2-2, epic #1375) --- src/eslint-suppressions.json | 4 +- src/utils/__tests__/safeWriteJson.test.ts | 89 ++++++++++++--- src/utils/safeWriteJson.ts | 127 +++++----------------- 3 files changed, 104 insertions(+), 116 deletions(-) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 393e108645..4d715fb646 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1686,7 +1686,7 @@ }, "utils/__tests__/safeWriteJson.test.ts": { "@typescript-eslint/no-explicit-any": { - "count": 27 + "count": 26 } }, "utils/__tests__/shell.spec.ts": { @@ -1716,7 +1716,7 @@ }, "utils/safeWriteJson.ts": { "@typescript-eslint/no-explicit-any": { - "count": 4 + "count": 3 } }, "utils/tts.ts": { diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 79d08678a0..003e9cbbf1 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -4,6 +4,7 @@ import * as path from "path" import * as os from "os" import { safeWriteJson } from "../safeWriteJson" +import { safeWriteText } from "../../services/file-safety/safeWriteText" // Capture actual implementations before the vi.mock factory runs, // so they are never wrapped by vi.fn() — avoids infinite recursion when @@ -48,6 +49,18 @@ vi.mock("fs", async () => { } }) +// Spy the atomic-publish boundary: the wrapper delegates to the real +// implementation, so every delegation still executes for real — the spy only +// records the exact boundary arguments so the delegation contract (empty +// content string + staged tempPath + backup) can be pinned. +vi.mock("../../services/file-safety/safeWriteText", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + safeWriteText: vi.fn(actual.safeWriteText), + } +}) + import * as fs from "fs/promises" // This will now be the mocked version describe("safeWriteJson", () => { @@ -127,6 +140,44 @@ describe("safeWriteJson", () => { expect(content).toEqual(newData) }) + test("should stage the serialized temp beside the target with a deterministic name", async () => { + // Pin the name inputs so the staged path is deterministic: the generated + // name must be exactly "..new__.tmp" inside the target's + // own directory (same volume as the commit rename — no EXDEV). + vi.spyOn(Date, "now").mockReturnValue(1700000000000) + vi.spyOn(Math, "random").mockReturnValue(0.123456789) + + await safeWriteJson(currentTestFilePath, { pinned: "name" }) + + // The implementation stages beside the *resolved* target (fs.realpath), + // so mirror that resolution when building the expectation: on Windows CI + // runners os.tmpdir() is the 8.3 short form (C:\Users\RUNNER~1\...) while + // realpath returns the long form (C:\Users\runneradmin\...). + const resolvedDir = path.dirname(await fs.realpath(currentTestFilePath)) + const expectedTemp = path.join(resolvedDir, ".test-file.json.new_1700000000000_4fzzzxjylrx.tmp") + expect(vi.mocked(fsSyncActual.createWriteStream)).toHaveBeenCalledWith(expectedTemp, { encoding: "utf8" }) + }) + + test("delegates the staged temp to safeWriteText with an empty content string", async () => { + // When tempPath is supplied, the primitive's content argument is not + // used — the staged file is the content source. Pin the exact boundary + // call so that contract (and the delegation itself) stays covered. + const spy = vi.mocked(safeWriteText) + spy.mockClear() + + await safeWriteJson(currentTestFilePath, { delegated: true }) + + expect(spy).toHaveBeenCalledTimes(1) + expect(spy).toHaveBeenCalledWith( + path.resolve(currentTestFilePath), + "", + expect.objectContaining({ + tempPath: expect.stringContaining(".new_"), + backup: true, + }), + ) + }) + // Failure Scenarios test("should handle failure when writing to tempNewFilePath", async () => { // currentTestFilePath exists due to beforeEach, allowing lock acquisition. @@ -312,8 +363,10 @@ describe("safeWriteJson", () => { expect(content).toEqual(newData) }) - // Test for console error suppression during backup deletion - test("should suppress console.error when backup deletion fails", async () => { + // Backup cleanup is delegated to safeWriteText, which swallows a + // non-fatal backup-deletion failure silently (the content is already + // committed; an orphaned backup is acceptable) — no console.error here. + test("should not log when delegated backup deletion fails", async () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error const initialData = { message: "Initial" } const newData = { message: "New" } @@ -321,17 +374,17 @@ describe("safeWriteJson", () => { await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) // 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_")) { - throw new Error("Backup deletion failed") - } - return fsPromisesActuals.unlink!(filePath) + vi.mocked(fs.unlink).mockImplementationOnce(async () => { + throw new Error("Backup deletion failed") }) await safeWriteJson(currentTestFilePath, newData) - // Verify console.error was called with the expected message - expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Successfully wrote"), expect.any(Error)) + // The write succeeds and the new content is in place... + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual(newData) + // ...and the delegated cleanup failure is silent. + expect(consoleErrorSpy).not.toHaveBeenCalled() consoleErrorSpy.mockRestore() vi.mocked(fs.unlink).mockRestore() @@ -434,8 +487,11 @@ describe("safeWriteJson", () => { expect(vi.mocked(fs.access)).toHaveBeenCalled() }) - // Test for rollback failure scenario - test("should log error and re-throw original if rollback fails", async () => { + // Test for rollback failure scenario. The rollback rename happens inside + // safeWriteText, which swallows a failed rollback silently so the original + // error is never masked — but the original content then only exists under + // the (orphaned) backup name. + test("should re-throw original error if rollback fails (backup is orphaned)", async () => { const initialData = { message: "Initial, should be lost if rollback fails" } const newData = { message: "New content" } @@ -460,11 +516,12 @@ describe("safeWriteJson", () => { // Should throw the original error, not the rollback error await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Primary rename failed") - // Verify console.error was called for the rollback failure - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining("Failed to restore backup"), - expect.objectContaining({ message: "Rollback rename failed" }), - ) + // The outer handler still logs the original error (loud failure)... + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Operation failed for"), expect.any(Error)) + // ...and the target path is gone: the original file was renamed to the + // backup, the commit failed, and the rollback rename failed too, leaving + // the backup orphaned at its generated name. + await expect(fs.access(currentTestFilePath)).rejects.toThrow() consoleErrorSpy.mockRestore() }) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 957a0bb20f..6777dbd2b2 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -3,6 +3,7 @@ import * as fsSync from "fs" import * as path from "path" import * as lockfile from "proper-lockfile" import { JsonStreamStringify } from "json-stream-stringify" +import { resolvePublishTarget, safeWriteText } from "../services/file-safety/safeWriteText" /** * Options for safeWriteJson function @@ -31,10 +32,11 @@ export interface SafeWriteJsonOptions { * 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. - * - Attempts to roll back and clean up in case of errors. - * - Supports pretty-printing with indentation while maintaining streaming efficiency. + * - Serializes to a temp file beside the resolved target (streaming, so large + * payloads stay out of memory), then delegates the backup/commit/rollback + * dance to the atomic text publish primitive (safeWriteText) with + * tempPath + backup:true — the same crash-safe, mode-preserving, + * DACL-preserving, symlink-referent semantics as every other file-safety write. * * @param {string} filePath - The absolute path to the target file. * @param {any} data - The data to serialize to JSON and write. @@ -46,8 +48,10 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso const absoluteFilePath = path.resolve(filePath) let releaseLock = async () => {} // Initialized to a no-op - // For directory creation - const dirPath = path.dirname(absoluteFilePath) + // Resolve the symlink referent so the staged temp file is created beside + // the file that will actually be replaced (a rename across volumes fails EXDEV). + const targetPath = await resolvePublishTarget(absoluteFilePath) + const dirPath = path.dirname(targetPath) // Ensure directory structure exists with improved reliability try { @@ -88,10 +92,6 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso throw lockError } - // Variables to hold the actual paths of temp files if they are created. - let actualTempNewFilePath: string | null = null - let actualTempBackupFilePath: string | null = null - try { // 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 @@ -110,101 +110,32 @@ 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`, + // Stage the serialized JSON in a temp file beside the resolved target. + // Streaming keeps large payloads out of memory; the safeWriteText commit + // is a rename on the same volume, so no EXDEV. + const tempPath = path.join( + dirPath, + `.${path.basename(targetPath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, ) - await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint) - - // Step 2: Check if the target file exists. If so, rename it to a backup path. 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 - } - // Target file does not exist, so no backup is made. actualTempBackupFilePath remains null. + await _streamDataToFile(tempPath, data, options?.prettyPrint) + } catch (streamError) { + // A failed stream can leave a partial (or empty) temp file behind — + // unlink it before propagating so no orphan staging file remains. + await fs.unlink(tempPath).catch(() => {}) + throw streamError } - // Step 3: Rename the new temporary file to the target file path. - // This is the main "commit" step. - 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 - - // Step 4: If a backup was created, attempt to delete it. - if (actualTempBackupFilePath) { - try { - await fs.unlink(actualTempBackupFilePath) - // Mark backup as handled - actualTempBackupFilePath = null - } catch (unlinkBackupError) { - // Log this error, but do not re-throw. The main operation was successful. - // actualTempBackupFilePath remains set, indicating an orphaned backup. - console.error( - `Successfully wrote ${absoluteFilePath}, but failed to clean up backup ${actualTempBackupFilePath}:`, - unlinkBackupError, - ) - } - } + // Delegate the backup/commit/rollback to the atomic text publish + // primitive: it applies the target's mode to the staged temp, fsyncs, + // backs up the existing target (backup:true), atomically renames + // temp -> target, and rolls the backup back on failure. The content + // argument is not used when tempPath is supplied — the staged file is + // the content source. + await safeWriteText(absoluteFilePath, "", { tempPath, backup: true }) } catch (originalError) { console.error(`Operation failed for ${absoluteFilePath}: [Original Error Caught]`, originalError) - - const newFileToCleanupWithinCatch = actualTempNewFilePath - const backupFileToRollbackOrCleanupWithinCatch = actualTempBackupFilePath - - // Attempt rollback if a backup was made - if (backupFileToRollbackOrCleanupWithinCatch) { - try { - await fs.rename(backupFileToRollbackOrCleanupWithinCatch, absoluteFilePath) - // Mark as handled, prevent later unlink of this path - actualTempBackupFilePath = null - } catch (rollbackError) { - // actualTempBackupFilePath (outer scope) remains pointing to backupFileToRollbackOrCleanupWithinCatch - console.error( - `[Catch] Failed to restore backup ${backupFileToRollbackOrCleanupWithinCatch} to ${absoluteFilePath}:`, - rollbackError, - ) - } - } - - // Cleanup the .new file if it exists - if (newFileToCleanupWithinCatch) { - try { - await fs.unlink(newFileToCleanupWithinCatch) - } catch (cleanupError) { - console.error( - `[Catch] Failed to clean up temporary new file ${newFileToCleanupWithinCatch}:`, - cleanupError, - ) - } - } - - // Cleanup the .bak file if it still needs to be (i.e., wasn't successfully restored) - if (actualTempBackupFilePath) { - try { - await fs.unlink(actualTempBackupFilePath) - } catch (cleanupError) { - console.error( - `[Catch] Failed to clean up temporary backup file ${actualTempBackupFilePath}:`, - cleanupError, - ) - } - } throw originalError // This MUST be the error that rejects the promise. } finally { // Release the lock in the main finally block.