From d5f8a79c75e74be66078ec8a9884a056ccc628bf Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Mon, 5 Oct 2026 20:34:31 +0800 Subject: [PATCH 01/23] split unit U1 of PR 1833 (issue 1375) --- .../__tests__/safeWriteText.spec.ts | 922 ++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 420 ++++++++ 2 files changed, 1342 insertions(+) create mode 100644 src/services/file-safety/__tests__/safeWriteText.spec.ts create mode 100644 src/services/file-safety/safeWriteText.ts diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts new file mode 100644 index 0000000000..e2f947c8bc --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,922 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import { execFile } from "child_process" +import type { ChildProcess } from "child_process" +import * as path from "path" + +import { RollbackFailureError, safeWriteText, type SafeWriteTextOptions } from "../safeWriteText" + +// Full mock for fs/promises — all methods are vi.fn() stubs +vi.mock("fs/promises", () => ({ + mkdir: vi.fn(), + access: vi.fn(), + rename: vi.fn(), + unlink: vi.fn(), + rmdir: vi.fn(), + realpath: vi.fn(), + lstat: vi.fn(), +})) + +// Full mock for fs — all sync methods are vi.fn() stubs. Stats is a bare +// class stub so tests can build minimal Stats stand-ins via its prototype. +vi.mock("fs", () => ({ + openSync: vi.fn(), + writeSync: vi.fn(), + closeSync: vi.fn(), + mkdirSync: vi.fn(), + fsyncSync: vi.fn(), + chmodSync: vi.fn(), + fchmodSync: vi.fn(), + statSync: vi.fn(), + Stats: class Stats {}, +})) + +// Mock child_process.execFile (callback-based — must invoke callback to resolve) +vi.mock("child_process", () => ({ + execFile: vi.fn((cmd, args, opts, cb) => { + if (typeof cb === "function") cb(null) + }), +})) + +// Minimal stand-in for the ChildProcess that callback-form execFile returns. +const fakeChild = { kill: () => true } as unknown as ChildProcess + +// Helper that mirrors safeWriteText's path resolution exactly +function _resolvedTarget(filePath: string): string { + return path.resolve(filePath) +} +function _dirPath(filePath: string): string { + return path.dirname(_resolvedTarget(filePath)) +} +// Minimal Stats stand-in: the SUT only reads `.mode` from it. +function _stats(mode: number): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats + Object.assign(s, { mode }) + return s +} + +// ── Test 1: staging file created then cleaned after success ──────────────── + +describe("safeWriteText", () => { + beforeEach(() => { + vi.resetAllMocks() + // After resetAllMocks, vi.fn() returns undefined — restore promise defaults. + vi.mocked(fs.mkdir).mockResolvedValue(undefined) + vi.mocked(fs.access).mockResolvedValue(undefined) + vi.mocked(fs.rename).mockResolvedValue(undefined) + vi.mocked(fs.unlink).mockResolvedValue(undefined) + vi.mocked(fs.rmdir).mockResolvedValue(undefined) + // Existing-target default: a regular 0o644 file. + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + // Default sync-write behaviour: report that all requested bytes were + // written. The Buffer overload passes (fd, buffer, offset, length), + // so the fourth argument is the requested length. + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + typeof args[3] === "number" ? args[3] : 0, + ) + }) + + describe("staging and cleanup", () => { + it("creates a temp file in the staging dir, fsyncs it, renames to target, and cleans up on success", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) // fd=1 + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + + await safeWriteText(targetPath, "hello world", { platform: "linux" }) + + // staging dir was created with private permissions — use + // stringContaining to handle Windows path resolution + expect(fsSync.mkdirSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), { + recursive: true, + mode: 0o700, + }) + // a pre-existing staging dir is repaired to private permissions too + expect(fsSync.chmodSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), 0o700) + + // temp file was opened for writing with the existing target's mode + // (default 0o644 from the statSync default mock) + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + + // content was written as a buffer (partial-write loop, full write) + expect(fsSync.writeSync).toHaveBeenCalledWith(1, Buffer.from("hello world", "utf8"), 0, 11) + + // fsync (sync form) was called on the fd + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + + // file was closed + expect(fsSync.closeSync).toHaveBeenCalledWith(1) + + // atomic rename happened — realpath mock returns targetPath, so that's the dest + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // no unlink of temp (it's now the committed file; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("removes the now-empty staging directory after a successful self-staged commit", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "hello", { platform: "linux" }) + + // the staging subdir is removed best-effort after the commit rename + // (stringContaining: the SUT and the test helper resolve Windows + // drive-relative paths differently, as in the existing staging tests) + expect(fs.rmdir).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + // the win32 DACL restore gate must stay closed on other platforms: + // no icacls save or restore is attempted + expect(execFile).not.toHaveBeenCalled() + }) + + it("still removes the staging directory when no options are supplied at all", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // options is undefined: the self-staged check and the optional-chained + // DACL runner lookup must not dereference it + await expect(safeWriteText(targetPath, "hello")).resolves.toBeUndefined() + + expect(fs.rmdir).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + if (process.platform === "win32") { + // default platform is win32: the DACL save + restore still ran + // through the default icacls path (options?.execFileRunner must + // not throw when options is undefined) + expect(vi.mocked(execFile)).toHaveBeenCalledTimes(2) + expect(vi.mocked(fs.unlink)).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + } + }) + + it("does not remove the staging directory when the caller supplies its own tempPath", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const callerTemp = "/tmp/test-dir/caller-staged.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "hello", { platform: "linux", tempPath: callerTemp }) + + // the caller owns its temp file's directory; safeWriteText must not + // rmdir a directory it did not create + expect(fs.rmdir).not.toHaveBeenCalled() + }) + + it("a failed staging-dir removal never fails the committed write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rmdir).mockRejectedValue(Object.assign(new Error("ENOTEMPTY"), { code: "ENOTEMPTY" })) + + await expect(safeWriteText(targetPath, "hello", { platform: "linux" })).resolves.toBeUndefined() + + // the commit rename still happened and the rmdir error was swallowed + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + expect(fs.rmdir).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("gives each self-staged write its own staging directory so a concurrent write cannot remove it", async () => { + const targetA = "/tmp/test-dir/target-a.txt" + const targetB = "/tmp/test-dir/target-b.txt" + vi.mocked(fs.realpath).mockImplementation((p) => Promise.resolve(p as string)) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetA, "a", { platform: "linux" }) + await safeWriteText(targetB, "b", { platform: "linux" }) + + // Two self-staged writes in the same directory must not share one staging + // directory: the first write's best-effort rmdir would otherwise delete the + // directory the second write had created but not yet opened (ENOENT on openSync). + const created = vi.mocked(fsSync.mkdirSync).mock.calls.map((c) => String(c[0])) + const staging = created.filter((p) => p.includes(".file-safety-staging_")) + expect(staging).toHaveLength(2) + expect(staging[0]).not.toBe(staging[1]) + // Uniqueness comes from the documented name shape + // /.file-safety-staging__: pinning the shape + // keeps the separator and the random suffix meaningful, not just the prefix. + for (const dir of staging) { + expect(dir).toMatch(/\.file-safety-staging_\d+_[a-z0-9]+$/) + } + const removed = vi.mocked(fs.rmdir).mock.calls.map((c) => String(c[0])) + expect(removed).toEqual([staging[0], staging[1]]) + }) + + it("removes its own staging directory when a self-staged write fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(Object.assign(new Error("EACCES"), { code: "EACCES" })) + + await expect(safeWriteText(targetPath, "hello", { platform: "linux" })).rejects.toThrow("EACCES") + + // The failed write's temp file is unlinked, then the directory it + // created is removed — a failed write must not leave an empty + // .file-safety-staging directory behind. + // mkdirSync created this write's staging directory; the temp file lives + // inside it, so the unlink targets a path under that directory. + const staging = vi.mocked(fsSync.mkdirSync).mock.calls.map((c) => String(c[0])) + expect(staging).toHaveLength(1) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(staging[0])) + expect(fs.rmdir).toHaveBeenCalledWith(staging[0]) + // The directory is only empty after its temp file is gone, so the + // unlink must happen before the rmdir. + expect(vi.mocked(fs.unlink).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(fs.rmdir).mock.invocationCallOrder[0], + ) + }) + + it("does not remove a staging directory it did not create when a caller-staged write fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const callerTemp = "/tmp/test-dir/caller-staged.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(Object.assign(new Error("EACCES"), { code: "EACCES" })) + + await expect( + safeWriteText(targetPath, "hello", { platform: "linux", tempPath: callerTemp }), + ).rejects.toThrow("EACCES") + + // The caller owns that directory: only the caller's temp file is cleaned, + // never a rmdir of a directory safeWriteText never created. + expect(fs.unlink).toHaveBeenCalledWith(callerTemp) + expect(fs.rmdir).not.toHaveBeenCalled() + }) + }) + + // ── Test 2: fsync ordering ─────────────────────────────────────────────── + + describe("fsync ordering", () => { + it("calls fsync on the fd before close, and rename after close", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // Verify call order: openSync(temp) → writeSync → fsyncSync(temp) + // → closeSync(temp) → rename. On POSIX the parent directory is then + // opened and fsynced after the commit rename, so openSync/fsyncSync/ + // closeSync each have a second (directory) call. + expect(vi.mocked(fsSync.openSync).mock.calls.length).toBe(2) + expect(vi.mocked(fsSync.writeSync).mock.calls.length).toBe(1) + expect(vi.mocked(fsSync.fsyncSync).mock.calls.length).toBe(2) + expect(vi.mocked(fsSync.closeSync).mock.calls.length).toBe(2) + + // the temp file was fully closed before the commit rename + expect(vi.mocked(fsSync.closeSync).mock.calls[0][0]).toBe(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + // The title promises the order, so compare the invocations rather than + // only count them: a rename before closeSync, or a close before fsync, + // would not be a durable commit. + const fsyncOrder = vi.mocked(fsSync.fsyncSync).mock.invocationCallOrder[0] + const closeOrder = vi.mocked(fsSync.closeSync).mock.invocationCallOrder[0] + const renameOrder = vi.mocked(fs.rename).mock.invocationCallOrder[0] + expect(fsyncOrder).toBeLessThan(closeOrder) + expect(closeOrder).toBeLessThan(renameOrder) + }) + }) + + // ── Test 3: simulated failure between write and rename leaves target intact ── + + describe("crash/torn-write safety", () => { + it("simulated failure between fsync and rename leaves the target byte-identical and no temp left behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + + await expect(safeWriteText(targetPath, "new data", { platform: "linux" })).rejects.toThrow("ENOSPC") + + // rename was attempted (the failure point) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // temp file was cleaned up on failure + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + + // backup was NOT created (backup:false by default), so target is untouched + // The only rename call was temp→target, not a rollback rename + expect(fs.rename).toHaveBeenCalledTimes(1) + }) + + it("a post-commit backup cleanup failure is non-fatal: the target stays committed and no temp is left behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The post-commit backup unlink (SUT step 6) fails — the write must + // still succeed; an orphaned backup is the documented acceptable + // outcome, so the failure is swallowed instead of rolling back. + vi.mocked(fs.unlink).mockRejectedValueOnce(new Error("EPERM")) + + await safeWriteText(targetPath, "data", { backup: true, platform: "linux" }) + + // the commit rename (temp -> target) still happened + expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + + // the failing cleanup was the post-commit backup unlink + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + + // no rollback rename: the committed target is not restored from the backup + expect(fs.rename).toHaveBeenCalledTimes(2) + + // the staging temp was already committed by the rename; nothing + // temp-shaped is unlinked afterwards + expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + }) + + // ── Test 4: backup:true keeps old safeWriteJson semantics incl. rollback ── + + describe("backup:true", () => { + it("renames target -> backup before commit, deletes backup on success", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "new data", { backup: true }) + + // target was accessed (exists check) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // first rename: target -> backup + expect(fs.rename).toHaveBeenNthCalledWith(1, targetPath, expect.stringContaining("safeWriteText.bak_")) + + // second rename: temp -> target (realpath mock returns targetPath) + expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + + // backup was deleted on success + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("rollback: on failure after rename target->backup, restores backup to target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // first rename (target->backup) succeeds, second fails + let callCount = 0 + vi.mocked(fs.rename).mockImplementation(async () => { + callCount++ + if (callCount === 1) return // target -> backup + if (callCount === 2) throw new Error("ENOSPC") // temp -> target fails + return // the rollback rename succeeds + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow("ENOSPC") + + // rollback rename is the 3rd call (after target->backup and temp->target failure) + expect(fs.rename).toHaveBeenNthCalledWith(3, expect.stringContaining("safeWriteText.bak_"), targetPath) + + // temp was cleaned up on failure + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("a failed rollback reports the partial state, not only the publish error", async () => { + // The content is still on disk, but only at the backup path. A caller that gets + // just the publish error has data it cannot find at the expected path. + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + let callCount = 0 + vi.mocked(fs.rename).mockImplementation(async () => { + callCount++ + if (callCount === 1) return // target -> backup + if (callCount === 2) throw new Error("ENOSPC") // temp -> target fails + throw new Error("EACCES") // the rollback rename fails too + }) + + let failure: RollbackFailureError | undefined + await safeWriteText(targetPath, "new data", { backup: true }).catch((e: unknown) => { + if (e instanceof RollbackFailureError) { + failure = e + return + } + throw e + }) + + expect(failure).toBeInstanceOf(RollbackFailureError) + expect(failure?.publishError).toBeInstanceOf(Error) + expect((failure?.publishError as Error).message).toBe("ENOSPC") + expect((failure?.rollbackError as Error).message).toBe("EACCES") + expect(failure?.backupPath).toContain("safeWriteText.bak_") + // The backup is what the caller can still recover, so it must stay on disk. + expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("backup:true when target does not exist: no backup created, just commit", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // fs.access resolves for dirPath check, but rejects for target check (backup path) + vi.mocked(fs.access).mockImplementation(async (p) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw { code: "ENOENT" } + }) + + await safeWriteText(targetPath, "new data", { backup: true, platform: "linux" }) + + // no backup rename (target didn't exist) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // only one rename: temp -> target + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // no unlink (no backup to delete; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + // ── Test 5: win32 DACL path ────────────────────────────────────────────── + + describe("win32 DACL", () => { + it.skipIf(process.platform !== "win32")( + "copies target DACL onto staging file via icacls before rename on Windows", + async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // icacls dump + restore were called (execFile is callback-based mock) + expect(execFile).toHaveBeenCalledTimes(2) + }, + ) + + it("non-win32: DACL path is unreachable when platform is not win32", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // icacls was NOT called on non-win32 + expect(execFile).not.toHaveBeenCalled() + }) + + it("win32 DACL failure falls back to plain rename (never fails the write)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls dump fails — the callback-based mock must invoke cb with an error. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write succeeded despite icacls failure (fallback to plain rename) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + // one icacls attempt only: a failed DACL apply must not try to restore + expect(execFile).toHaveBeenCalledTimes(1) + }) + + it("win32 DACL: a partial dump left by a failed save is removed and never restored", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls save fails — a real icacls may have written a partial dump + // before erroring, so the dump path must be cleaned up and must never + // be used for a restore. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls save error"), "", "") + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write committed; only the save was attempted (no restore from a failed dump) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(execFile).toHaveBeenCalledTimes(1) + const saveArgs = vi.mocked(execFile).mock.calls[0]?.[1] + expect(saveArgs?.[1]).toBe("/save") + // the dump path (possibly partially created by icacls) was unlinked + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("win32 DACL save args are [targetPath, /save, dumpPath, /T] before backup rename", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + // icacls was called twice (save + restore) + expect(execFile).toHaveBeenCalledTimes(2) + + // First call: save DACL from target before backup rename + const firstCall = vi.mocked(execFile).mock.calls[0] + expect(firstCall[0]).toBe("icacls") + expect(firstCall[1]).toEqual([targetPath, "/save", expect.stringContaining(".acl.tmp"), "/T"]) + + // Second call: restore DACL onto directory after commit rename + const secondCall = vi.mocked(execFile).mock.calls[1] + expect(secondCall[0]).toBe("icacls") + expect(secondCall[1]).toEqual([ + expect.stringContaining("/tmp/test-dir"), + "/restore", + expect.stringContaining(".acl.tmp"), + ]) + + // dump file was unlinked after restore + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("win32 DACL: dump is unlinked even when restore fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // icacls save succeeds, restore fails + let callCount = 0 + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + callCount++ + if (typeof cb === "function") { + cb(callCount === 1 ? null : new Error("icacls restore error"), "", "") + } + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write succeeded despite restore failure (best-effort) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(fs.rename).toHaveBeenCalledTimes(1) + + // dump file was still unlinked in finally + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("win32 DACL: when target does not exist, no save/restore/dump", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // fs.access rejects for targetPath (ENOENT), but resolves for dirPath + vi.mocked(fs.access).mockImplementation(async (p) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw { code: "ENOENT" } + return undefined + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // icacls was NOT called (target absent → skip DACL entirely) + expect(execFile).not.toHaveBeenCalled() + + // no dump file created or unlinked + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + // ── Test 6: pre-written temp path (tempPath option) ────────────────────── + + describe("pre-written temp path", () => { + it("uses the provided tempPath, fsyncs it, and renames to target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + const customTempPath = "/tmp/custom-temp.tmp" + + // platform:linux skips DACL entirely so this test focuses on tempPath only + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // openSync was called on the custom temp path (r+ mode for fsync) + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + + // fsync was called + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + + // rename happened — realpath mock returns targetPath + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + + // no unlink of custom temp (caller's concern; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + + // a caller-supplied tempPath must not create the staging directory + expect(fsSync.mkdirSync).not.toHaveBeenCalled() + }) + + it("applies the existing target's mode to a caller-supplied tempPath before publishing", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // the caller-staged temp is fchmod'd to the restrictive target mode so + // the atomic rename cannot widen a 0o600 target (CWE-732 regression) + expect(fsSync.fchmodSync).toHaveBeenCalledWith(2, 0o600) + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + + it("keeps the temp's default mode when the target does not exist yet (ENOENT)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw enoent + }) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // no existing target, so nothing to preserve and no fchmod on the temp + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + + it("propagates a non-ENOENT stat failure rather than defaulting the mode (caller-staged)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw eacces + }) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/custom-temp.tmp" + + // A target that cannot be stat'd is not a fresh target: publishing with + // the default mode would widen a restrictive target through the rename. + await expect( + safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }), + ).rejects.toThrow("EACCES") + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("propagates a non-ENOENT stat failure rather than defaulting the mode (self-staged)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const eio = Object.assign(new Error("EIO: i/o error"), { code: "EIO" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw eio + }) + + // The mode is read before the temp is opened, so a real I/O failure stops + // the write before anything is staged. + await expect(safeWriteText(targetPath, "hello world", { platform: "linux" })).rejects.toThrow("EIO") + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("opens the temp before applying a read-only target's mode (0o444 does not block the open)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o444)) + vi.mocked(fsSync.openSync).mockReturnValue(3) + + const customTempPath = "/tmp/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // a 0o444 target must not make openSync(tempPath, "r+") fail: the mode + // is applied with fchmodSync on the already-open fd, after the open + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + expect(fsSync.fchmodSync).toHaveBeenCalledWith(3, 0o444) + const openIdx = vi.mocked(fsSync.openSync).mock.invocationCallOrder[0] + const fchmodIdx = vi.mocked(fsSync.fchmodSync).mock.invocationCallOrder[0] + expect(openIdx).toBeLessThan(fchmodIdx) + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + + it("applies the existing target's exact mode to the self-staged temp (umask must not narrow it)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o664)) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // openSync's creation mode is narrowed by the process umask (0o664 -> 0o644 with + // the common 0o022), and the rename publishes the temp's mode onto the target, + // so the existing target's mode must be applied on the fd before the commit. + expect(fsSync.fchmodSync).toHaveBeenCalledWith(1, 0o664) + const openIdx = vi.mocked(fsSync.openSync).mock.invocationCallOrder[0] + const fchmodIdx = vi.mocked(fsSync.fchmodSync).mock.invocationCallOrder[0] + expect(openIdx).toBeLessThan(fchmodIdx) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("does not fchmod the self-staged temp for a fresh target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw enoent + }) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // Nothing exists to preserve: the default creation mode is the intended one. + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + }) + }) + + // ── Test 7: symlink handling (Finding 4 regression test) ───────────────── + + describe("symlink handling", () => { + it("a write through a symlink commits onto the resolved referent, never the link path", async () => { + const linkPath = "/tmp/links/link.txt" + const referentPath = "/tmp/targets/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(referentPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(linkPath, "new-content", { platform: "linux" }) + + // The commit rename must target the realpath result (the referent), never the link itself — + // that is what guarantees a write through a symlink replaces the referent's content + // and preserves the link. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), referentPath) + expect(fs.rename).not.toHaveBeenCalledWith(expect.anything(), linkPath) + }) + + it("when realpath reports ENOENT (target absent), uses the given path as-is", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + // lstat reports the path itself as absent, so this is a new target and + // the fallback is allowed. + vi.mocked(fs.lstat).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // rename still happened with the fallback path (path.resolve on /tmp → C:\tmp) + const resolvedFallback = _resolvedTarget(targetPath) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), resolvedFallback) + }) + + it("propagates a dangling symlink instead of writing through the link path", async () => { + // realpath resolves the referent, so a link whose target is missing reports + // ENOENT. Falling back to the link path would replace the symlink with a + // regular file, so the error must propagate and nothing may be committed. + const linkPath = "/tmp/test-dir/dangling-link.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + const linkStats = Object.create(fsSync.Stats.prototype) as fsSync.Stats + linkStats.isSymbolicLink = () => true + vi.mocked(fs.lstat).mockResolvedValue(linkStats) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await expect(safeWriteText(linkPath, "data", { platform: "linux" })).rejects.toThrow("ENOENT") + + expect(fs.rename).not.toHaveBeenCalled() + }) + }) + + // ── Test 8: review fixes (permissions, partial writes, resolution, durability) ── + + describe("review fixes", () => { + it("preserves the target's restrictive mode and tolerates a failed staging-dir permission repair", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) + // a pre-existing staging dir may fail its best-effort permission repair + vi.mocked(fsSync.chmodSync).mockImplementationOnce(() => { + throw new Error("EACCES") + }) + + await safeWriteText(targetPath, "secret", { platform: "linux" }) + + // the staging file inherits the target's 0o600 mode and the write commits + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o600) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("falls back to the 0o644 default when the target does not exist yet", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + + await safeWriteText(targetPath, "fresh", { platform: "linux" }) + + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + }) + + it("loops on short writes until the full content is durable before fsync", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const content = "0123456789" // 10 bytes + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const buffer = Buffer.from(content, "utf8") + // first write (offset 0) reports 4 bytes (short write); the loop continues + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + args[2] === 0 ? 4 : typeof args[3] === "number" ? args[3] : 0, + ) + + await safeWriteText(targetPath, content, { platform: "linux" }) + + // [0,10) reports 4 bytes, then [4,10) writes the remaining 6 + expect(fsSync.writeSync).toHaveBeenCalledTimes(2) + expect(fsSync.writeSync).toHaveBeenNthCalledWith(1, 1, buffer, 0, 10) + expect(fsSync.writeSync).toHaveBeenNthCalledWith(2, 1, buffer, 4, 6) + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("fsyncs the parent directory after the commit rename on POSIX", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // temp fd=1 then parent-dir fd=2 - distinct fds prove the ordering + vi.mocked(fsSync.openSync).mockReturnValueOnce(1).mockReturnValue(2) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // the directory fsync (fd 2) happens only after the file fsync (fd 1); + // the dir path assertion is path-agnostic (stringContaining) because + // path.dirname renders the same input differently on Windows + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("test-dir"), "r") + expect(fsSync.fsyncSync).toHaveBeenNthCalledWith(1, 1) + expect(fsSync.fsyncSync).toHaveBeenNthCalledWith(2, 2) + expect(fsSync.closeSync).toHaveBeenCalledWith(2) + }) + + it("treats a failed parent-directory fsync as best-effort", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync) + .mockReturnValueOnce(1) + .mockImplementationOnce(() => { + throw new Error("EBADF") + }) + + // the content rename already committed; a missing directory fsync is not fatal + await safeWriteText(targetPath, "data", { platform: "linux" }) + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("propagates realpath errors (EACCES and code-less) instead of the fallback path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fs.realpath).mockRejectedValueOnce(eacces) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eacces) + expect(fs.rename).not.toHaveBeenCalled() + + const plain = new Error("resolution failed") + vi.mocked(fs.realpath).mockRejectedValueOnce(plain) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(plain) + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("backup:true propagates access errors (EACCES and code-less) instead of skipping the backup", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const eacces = Object.assign(new Error("EACCES"), { code: "EACCES" }) + const plain = new Error("access failed") + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // each write accesses dirPath then target; only the target access rejects + const rejectTarget = (error: Error) => async (p: unknown) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw error + } + vi.mocked(fs.access) + .mockImplementationOnce(rejectTarget(eacces)) + .mockImplementationOnce(rejectTarget(eacces)) + .mockImplementationOnce(rejectTarget(plain)) + .mockImplementationOnce(rejectTarget(plain)) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toEqual( + expect.objectContaining({ code: "EACCES" }), + ) + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow( + "access failed", + ) + expect(fs.rename).not.toHaveBeenCalled() + }) + }) + describe("content bytes", () => { + const targetPath = "/tmp/enc-dir/target.txt" + + beforeEach(() => { + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + }) + + it("stages UTF-8 bytes for string content", async () => { + await safeWriteText(targetPath, "héllo", { platform: "linux" }) + expect(fsSync.writeSync).toHaveBeenCalledWith(1, Buffer.from("héllo", "utf8"), 0, 6) + }) + + it("publishes caller-supplied bytes unchanged instead of re-encoding them", async () => { + // The extension host encodes a document with VS Code's own codec, which + // covers the legacy code pages and BOMs Node cannot represent, and hands + // the result over: those bytes must reach the commit rename exactly as + // they were given. + const bytes = Buffer.from([0x00, 0x68, 0x00, 0x69]) + await safeWriteText(targetPath, bytes, { platform: "linux" }) + expect(fsSync.writeSync).toHaveBeenCalledWith(1, bytes, 0, 4) + }) + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts new file mode 100644 index 0000000000..b5f82a4f81 --- /dev/null +++ b/src/services/file-safety/safeWriteText.ts @@ -0,0 +1,420 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import * as path from "path" +import { execFile } from "child_process" + +export interface SafeWriteTextOptions { + /** + * When true, preserve the old-file semantics: rename target -> backup first, + * after commit rename delete the backup; on failure roll the backup back to + * the target path. When false (default) the atomic rename simply replaces + * the target -- crash-safe window is zero. + */ + backup?: boolean + + /** + * Platform override for testing. When omitted the real process.platform + * value is used. Set to "win32" or "linux" / "darwin" from tests so that + * both branches are reachable without needing a real Windows runner. + */ + platform?: string + + /** + * Custom execFile runner for testing (e.g. vi.fn). When omitted the real + * child_process.execFile is used. + */ + execFileRunner?: typeof execFile + + /** + * Pre-written temp path to use for the commit phase. When provided, + * safeWriteText skips creating its own staging file and uses this path + * instead (it still fsyncs before rename). Useful when a caller has + * already written data to a temp file via a custom stream. + */ + tempPath?: string +} + +/** + * A publish that failed and whose rollback also failed: the content survives only + * at the backup path, not at the canonical target. The publish failure stays the + * cause, and the rollback failure plus the backup location travel with the error so + * the caller can tell what it is looking at. + */ +export class RollbackFailureError extends Error { + readonly publishError: unknown + readonly rollbackError: unknown + readonly backupPath: string + + constructor(publishError: unknown, rollbackError: unknown, backupPath: string) { + super( + "Publish failed and the backup could not be restored to its original path -- the content is preserved at the backup location reported on this error.", + { cause: publishError }, + ) + this.name = "RollbackFailureError" + this.publishError = publishError + this.rollbackError = rollbackError + this.backupPath = backupPath + } +} +// -- helpers --------------------------------------------------------------- + +/** Generate a unique temp file name in the given directory. */ +function _tempName(dir: string, prefix: string): string { + return path.join(dir, "." + prefix + "_" + Date.now() + "_" + Math.random().toString(36).substring(2) + ".tmp") +} + +/** Create a private per-write staging sub-directory inside *dir*. The name is + * unique per write, so concurrent writes never collide on their temp names and + * never remove a staging directory another write is still using: with one shared + * name, one write's best-effort rmdir could delete the directory another write + * had just created but not yet opened, failing its openSync with ENOENT. */ +function _stagingDir(dir: string): string { + const sd = path.join(dir, ".file-safety-staging_" + Date.now() + "_" + Math.random().toString(36).substring(2)) + // mode:0o700 protects a freshly created staging dir; the best-effort chmod + // repairs a pre-existing one (mkdirSync with recursive:true never chmods an + // existing directory), so staged temp files are never group/world readable. + fsSync.mkdirSync(sd, { recursive: true, mode: 0o700 }) + try { + fsSync.chmodSync(sd, 0o700) + } catch { + // best-effort: chmod denied or unavailable; a fresh dir was still + // created with the requested mode + } + return sd +} + +function _fsyncFile(fd: number): void { + fsSync.fsyncSync(fd) +} + +/** Save the DACL of *srcPath* to a dump file on Windows. + * Returns true when the dump was written successfully; false otherwise. + * Never throws — callers treat failure as "skip DACL handling". */ +async function _saveDaclWindows(srcPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + runner("icacls", [srcPath, "/save", dumpPath, "/T"], { windowsHide: true }, (err) => + err ? reject(err) : resolve(), + ) + }) + return true + } catch { + return false + } +} + +/** Restore a DACL dump onto *dirPath* on Windows. + * Best-effort: content is already committed, so failure is non-fatal. */ +async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + runner("icacls", [dirPath, "/restore", dumpPath], { windowsHide: true }, (err) => + err ? reject(err) : resolve(), + ) + }) + } catch { + // best-effort; content already committed + } +} + +// -- public API ------------------------------------------------------------ + +/** + * Resolve the publish target: the symlink referent when the given path is an + * existing symlink, the path itself otherwise. Only ENOENT (target absent yet) + * may fall back to the given path; any other resolution error (EACCES, EIO, ...) + * propagates so a broken or unreadable symlink is never written through its + * link path. Callers that stage a temp file themselves must stage it beside + * the resolved path: the commit is a rename onto the referent, and a rename + * across filesystems fails with EXDEV. + */ +export async function resolvePublishTarget(absoluteFilePath: string): Promise { + return fs.realpath(absoluteFilePath).catch(async (error: unknown) => { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined + if (code !== "ENOENT") throw error + // ENOENT also covers a dangling symlink, which must never be written through. + const linkStat = await fs.lstat(absoluteFilePath).catch(() => undefined) + if (linkStat?.isSymbolicLink()) throw error + return absoluteFilePath + }) +} +/** + * Distinguish "the target does not exist" from a real I/O failure (EACCES, + * EIO, ...). The mode-preservation path may only fall back to the fresh-file + * default on ENOENT; any other failure is propagated, otherwise a restrictive + * target (0o600) would be published with the default 0o644 through the rename. + */ +function errorCode(error: unknown): string | undefined { + return typeof error === "object" && error !== null && "code" in error + ? String((error as { code: unknown }).code) + : undefined +} + +/** + * Canonicalize the parent directory and re-join the basename. fs.realpath + * canonicalizes every component, including a symlinked ancestor directory or a + * Windows 8.3 short name, so a lock key must be canonical even when the file + * itself is not there yet -- otherwise the key for one file depends on whether + * the file exists when the key is computed, and two writers take two locks. + */ + +async function canonicalDirKey(absoluteFilePath: string): Promise { + const dirPath = path.dirname(absoluteFilePath) + const canonicalDir = await fs.realpath(dirPath).catch(() => dirPath) + return path.join(canonicalDir, path.basename(absoluteFilePath)) +} + +/** + * Lock key for a publish target: the symlink referent when the path is an + * existing symlink, the path itself otherwise. Unlike resolvePublishTarget this + * tolerates a dangling link, because the lock key has to be computable while a + * peer writer is mid-commit (backup mode renames the referent away and back). + * The walk is bounded so a two-link cycle terminates, and every key it returns is + * canonicalized through canonicalDirKey. + */ +export async function resolveLockKey(absoluteFilePath: string): Promise { + try { + return await canonicalDirKey(await resolvePublishTarget(absoluteFilePath)) + } catch { + // A real readlink throws for anything that is not a link, so a normal chain + // ends the walk. Two links that point at each other never would, so the + // walk is bounded and callers use the key they actually reached. + let key = absoluteFilePath + for (let depth = 0; depth < 8; depth++) { + const target = await fs.readlink(key).catch(() => undefined) + if (target === undefined) return await canonicalDirKey(key) + key = await canonicalDirKey(path.resolve(path.dirname(key), target)) + } + return await canonicalDirKey(key) + } +} + +export async function safeWriteText( + filePath: string, + content: string | Uint8Array, + options?: SafeWriteTextOptions, +): Promise { + const absoluteFilePath = path.resolve(filePath) + + // Resolve the symlink referent (see resolvePublishTarget). + const targetPath = await resolvePublishTarget(absoluteFilePath) + const dirPath = path.dirname(targetPath) + + // Ensure parent directory exists (mirrors safeWriteJson behaviour). + await fs.mkdir(dirPath, { recursive: true }) + await fs.access(dirPath) + + // Create the staging directory only when we generate the temp file there; + // callers supplying their own tempPath (e.g. safeWriteJson) must not be left + // with an empty .file-safety-staging directory behind. Track the directory this + // write created so its cleanup removes its own directory, not a shared one. + let stagingDir: string | null = null + let tempPath: string + if (options?.tempPath) { + tempPath = options.tempPath + } else { + stagingDir = _stagingDir(dirPath) + tempPath = _tempName(stagingDir, "safeWriteText") + } + + let backupPath: string | null = null + let releaseBackupOnSuccess = false + // Non-null only when the win32 step-2 block saved a successful DACL dump: + // it gates the step-5 restore and is tracked for the cleanup unlinks. + let daclDumpPath: string | null = null + + try { + // -- Step 1: write content to staging temp file ------------------- + if (!options?.tempPath) { + // Preserve the existing target's permissions: the staging file must + // not be published wider than the file it replaces (a 0o600 target + // must not become 0o644 through the atomic rename). + // Encode before opening the staging file: an encoding Node cannot + // represent must not leave a half-written temp file behind. + // A string is encoded as UTF-8; bytes handed in by the caller (the + // extension host encodes a document with VS Code's own codec, which + // covers the legacy code pages Node cannot represent) are published + // unchanged. + const buffer = Buffer.from(content) + let targetMode = 0o644 // default for a fresh target + let targetExists = false + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + targetExists = true + } catch (error: unknown) { + if (errorCode(error) !== "ENOENT") throw error + // target does not exist yet - keep the default + } + // openSync's creation mode is narrowed by the process umask, so an + // existing 0o664 target would be published as 0o644 through the + // rename. Apply the existing target's exact mode on the fd, as the + // caller-staged branch does; a fresh target keeps the default mode. + const fd = fsSync.openSync(tempPath, "w", targetMode) + try { + if (targetExists) { + fsSync.fchmodSync(fd, targetMode) + } + // Loop until every byte is written: writeSync can report a short + // (partial) write, and publishing a truncated staging file would + // commit corrupt content. + let offset = 0 + while (offset < buffer.length) { + offset += fsSync.writeSync(fd, buffer, offset, buffer.length - offset) + } + _fsyncFile(fd) + } finally { + fsSync.closeSync(fd) + } + } else { + // Preserve the existing target's mode (CWE-732): the caller-staged + // temp carries its own creation mode, and publishing it as-is would + // widen a restrictive target (e.g. 0o600 -> 0o644) through rename. + // The mode is applied with fchmodSync on the open fd (AFTER openSync): + // chmodSync on the path before the open would make a read-only target + // (0o400/0o444) fail openSync(tempPath, "r+") with EACCES. + let targetMode: number | null = null + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + } catch (error: unknown) { + if (errorCode(error) !== "ENOENT") throw error + // target does not exist yet - keep the temp's default mode + } + const fd = fsSync.openSync(tempPath, "r+") + try { + if (targetMode !== null) { + fsSync.fchmodSync(fd, targetMode) + } + _fsyncFile(fd) + } finally { + fsSync.closeSync(fd) + } + } + + // -- Step 2 (win32): save DACL BEFORE backup rename --------------- + const platform = options?.platform ?? process.platform + if (platform === "win32") { + try { + await fs.access(targetPath) // target exists? + const dumpPath = targetPath + ".acl.tmp" + const saved = await _saveDaclWindows(targetPath, dumpPath, options?.execFileRunner) + if (saved) { + // Only a successfully saved dump may be restored onto the + // committed file (step 5). + daclDumpPath = dumpPath + } else { + // A failed icacls may have left a partial dump behind; + // remove it now (best-effort) so no partial dump survives and + // no later step can restore from it. + await fs.unlink(dumpPath).catch(() => {}) + } + } catch { + // target does not exist or access failed — no DACL handling + daclDumpPath = null + } + } + + try { + // -- Step 3 (backup:true): rename target -> backup -------------- + if (options?.backup) { + try { + await fs.access(targetPath) + backupPath = _tempName(dirPath, "safeWriteText.bak") + await fs.rename(targetPath, backupPath) + releaseBackupOnSuccess = true + } catch (err: unknown) { + const code = + typeof err === "object" && err !== null && "code" in err + ? (err as { code?: string }).code + : undefined + if (code !== "ENOENT") throw err + } + } + + // -- Step 4: atomic rename temp -> target --------------------- + await fs.rename(tempPath, targetPath) + + // -- Step 4b (POSIX): fsync the parent directory so the directory entry + // changed by the commit rename is durable, not just the file content. + if (platform !== "win32") { + try { + const dirFd = fsSync.openSync(dirPath, "r") + try { + _fsyncFile(dirFd) + } finally { + fsSync.closeSync(dirFd) + } + } catch { + // best-effort: the content rename already committed + } + } + + // -- Step 5 (win32): restore DACL AFTER commit rename --------- + // daclDumpPath is non-null only when the win32 step-2 block saved a + // successful dump, so this gate is closed on every other platform + // and on every failed save. + if (daclDumpPath !== null) { + const restoredDir = path.dirname(targetPath) + await _restoreDaclWindows(restoredDir, daclDumpPath, options?.execFileRunner) + } + + // -- Step 6 (backup:true): delete backup on success ----------- + if (releaseBackupOnSuccess && backupPath) { + try { + await fs.unlink(backupPath) + } catch { + // non-fatal — orphaned backup is acceptable + } + } + } finally { + // Unlink DACL dump regardless of success/failure in this span. + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + } + + // tempPath is now the committed file; no cleanup needed. + + // Best-effort: remove the now-empty staging directory. Self-staged + // writes only, and only this write's own directory: a per-write directory + // cannot be the one another concurrent write is still using. A failure must + // never un-commit a published file, so the removal swallows all errors. + if (stagingDir) { + await fs.rmdir(stagingDir).catch(() => {}) + } + } catch (originalError: unknown) { + if (backupPath && releaseBackupOnSuccess) { + try { + await fs.rename(backupPath, targetPath) + } catch (rollbackError: unknown) { + // The content survives only at the backup path now, and the canonical + // target is gone. Reporting just the publish failure would leave the + // caller with data it cannot find at the expected path, so the + // partial-failure state travels with the error. + throw new RollbackFailureError(originalError, rollbackError, backupPath) + } + } + try { + await fs.unlink(tempPath).catch(() => {}) + } catch { + // cleanup failure is non-fatal + } + + // A failed self-staged write must not leave its staging directory behind. + // Only the directory this write created, and only after its temp file is + // gone, so the directory is empty and the removal stays best-effort. + if (stagingDir) { + await fs.rmdir(stagingDir).catch(() => {}) + } + + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + + throw originalError + } +} From aa0cdaba0a6741daab9f3c06cf1eea9ad16f481a Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Mon, 5 Oct 2026 20:49:29 +0800 Subject: [PATCH 02/23] fix(file-safety): close the pre-merge findings on the publish primitive (U1, issue 1375) Split unit U1 of PR 1833. Three changes, each with a test that fails without it: - a caller-supplied staging path is checked for location and file type before anything is written, so an arbitrary path or a symlink cannot be published onto the target; - a failed parent-directory fsync on POSIX is reported as PostCommitDurabilityError instead of being swallowed, so a successful return never claims durability the filesystem did not grant; - the staged file and this write's own staging directory are released before RollbackFailureError is thrown. Focused coverage for resolveLockKey added: canonical parent directory, a dangling-link chain, and termination at the bounded depth on a two-link cycle. --- .../__tests__/safeWriteText.spec.ts | 169 ++++++++++++++++-- src/services/file-safety/safeWriteText.ts | 82 ++++++++- 2 files changed, 229 insertions(+), 22 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index e2f947c8bc..66878d8b9d 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -4,7 +4,14 @@ import { execFile } from "child_process" import type { ChildProcess } from "child_process" import * as path from "path" -import { RollbackFailureError, safeWriteText, type SafeWriteTextOptions } from "../safeWriteText" +import { + PostCommitDurabilityError, + resolveLockKey, + RollbackFailureError, + safeWriteText, + StagingPathError, + type SafeWriteTextOptions, +} from "../safeWriteText" // Full mock for fs/promises — all methods are vi.fn() stubs vi.mock("fs/promises", () => ({ @@ -15,6 +22,7 @@ vi.mock("fs/promises", () => ({ rmdir: vi.fn(), realpath: vi.fn(), lstat: vi.fn(), + readlink: vi.fn(), })) // Full mock for fs — all sync methods are vi.fn() stubs. Stats is a bare @@ -49,6 +57,25 @@ function _dirPath(filePath: string): string { return path.dirname(_resolvedTarget(filePath)) } // Minimal Stats stand-in: the SUT only reads `.mode` from it. +// Async lstat stand-in: the SUT only asks whether the path is a link or a file. +function _fileStats(isLink: boolean) { + return { isSymbolicLink: () => isLink, isFile: () => !isLink } +} + +function mockDefaults(): void { + vi.resetAllMocks() + // After resetAllMocks, vi.fn() returns undefined — restore promise defaults. + vi.mocked(fs.mkdir).mockResolvedValue(undefined) + vi.mocked(fs.access).mockResolvedValue(undefined) + vi.mocked(fs.rename).mockResolvedValue(undefined) + vi.mocked(fs.unlink).mockResolvedValue(undefined) + vi.mocked(fs.rmdir).mockResolvedValue(undefined) + // Existing-target default: a regular 0o644 file. + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + // Staged-file default: a regular file, not a link, so a caller-supplied + // tempPath passes the location and file-type check by default. + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(false)) +} function _stats(mode: number): fsSync.Stats { const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats Object.assign(s, { mode }) @@ -59,15 +86,7 @@ function _stats(mode: number): fsSync.Stats { describe("safeWriteText", () => { beforeEach(() => { - vi.resetAllMocks() - // After resetAllMocks, vi.fn() returns undefined — restore promise defaults. - vi.mocked(fs.mkdir).mockResolvedValue(undefined) - vi.mocked(fs.access).mockResolvedValue(undefined) - vi.mocked(fs.rename).mockResolvedValue(undefined) - vi.mocked(fs.unlink).mockResolvedValue(undefined) - vi.mocked(fs.rmdir).mockResolvedValue(undefined) - // Existing-target default: a regular 0o644 file. - vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + mockDefaults() // Default sync-write behaviour: report that all requested bytes were // written. The Buffer overload passes (fd, buffer, offset, length), // so the fourth argument is the requested length. @@ -577,7 +596,7 @@ describe("safeWriteText", () => { vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) - const customTempPath = "/tmp/custom-temp.tmp" + const customTempPath = "/tmp/test-dir/custom-temp.tmp" // platform:linux skips DACL entirely so this test focuses on tempPath only await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) @@ -604,7 +623,7 @@ describe("safeWriteText", () => { vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) vi.mocked(fsSync.openSync).mockReturnValue(2) - const customTempPath = "/tmp/custom-temp.tmp" + const customTempPath = "/tmp/test-dir/custom-temp.tmp" await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) @@ -624,7 +643,7 @@ describe("safeWriteText", () => { }) vi.mocked(fsSync.openSync).mockReturnValue(2) - const customTempPath = "/tmp/custom-temp.tmp" + const customTempPath = "/tmp/test-dir/custom-temp.tmp" await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) @@ -642,7 +661,7 @@ describe("safeWriteText", () => { }) vi.mocked(fsSync.openSync).mockReturnValue(2) - const customTempPath = "/tmp/custom-temp.tmp" + const customTempPath = "/tmp/test-dir/custom-temp.tmp" // A target that cannot be stat'd is not a fresh target: publishing with // the default mode would widen a restrictive target through the rename. @@ -674,7 +693,7 @@ describe("safeWriteText", () => { vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o444)) vi.mocked(fsSync.openSync).mockReturnValue(3) - const customTempPath = "/tmp/custom-temp.tmp" + const customTempPath = "/tmp/test-dir/custom-temp.tmp" await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) @@ -843,7 +862,7 @@ describe("safeWriteText", () => { expect(fsSync.closeSync).toHaveBeenCalledWith(2) }) - it("treats a failed parent-directory fsync as best-effort", async () => { + it("reports a failed parent-directory fsync instead of claiming a durable write", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync) @@ -852,8 +871,13 @@ describe("safeWriteText", () => { throw new Error("EBADF") }) - // the content rename already committed; a missing directory fsync is not fatal - await safeWriteText(targetPath, "data", { platform: "linux" }) + // The content rename committed, so the caller can still find the data at + // the target; what the write cannot claim is that the directory entry + // reached the disk. Returning success here would claim durability the + // filesystem did not grant. + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toThrow( + PostCommitDurabilityError, + ) expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) }) @@ -920,3 +944,112 @@ describe("safeWriteText", () => { }) }) }) + +// ── Test 12: lock key, staging path, and post-commit durability ───────────── + +describe("resolveLockKey", () => { + beforeEach(() => mockDefaults()) + + it("canonicalizes the parent directory, not just the file", async () => { + vi.mocked(fs.realpath).mockImplementation(async (target: string) => { + if (target === "/tmp/linkdir/file.json") return "/real/dir/file.json" + if (target === "/real/dir") return "/real/dir" + return target + }) + + // The key is the canonical directory plus the basename, so a symlinked + // ancestor and its referent share one lock. + await expect(resolveLockKey("/tmp/linkdir/file.json")).resolves.toBe(path.join("/real/dir", "file.json")) + }) + + it("computes a key for a dangling link, which resolvePublishTarget refuses", async () => { + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(true)) + vi.mocked(fs.readlink).mockImplementation(async (target: string) => + target === "/tmp/linkdir/file.json" ? "referent.json" : undefined, + ) + + // Mid-commit a peer writer renames the referent away and back, so the key + // must still be computable while the link dangles. + await expect(resolveLockKey("/tmp/linkdir/file.json")).resolves.toBe( + path.resolve(path.join("/tmp/linkdir", "referent.json")), + ) + }) + + it("terminates on a two-link cycle instead of walking forever", async () => { + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(true)) + // Every readlink answers with the same link, so an unbounded walk would + // never end; the bounded walk returns the key it actually reached. + vi.mocked(fs.readlink).mockImplementation(async () => "a.json") + + await expect(resolveLockKey("/tmp/linkdir/a.json")).resolves.toBe( + path.resolve(path.join("/tmp/linkdir", "a.json")), + ) + expect(fs.readlink).toHaveBeenCalledTimes(8) + }) +}) + +describe("caller-supplied staging path", () => { + beforeEach(() => mockDefaults()) + + it("rejects a staging file outside the target's directory before writing anything", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + + // A rename across filesystems fails with EXDEV, and a path elsewhere lets + // a caller publish an unrelated file onto the target. + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/other-dir/x.tmp", platform: "linux" }), + ).rejects.toThrow(StagingPathError) + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("rejects a staging path that is a symlink", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(true)) + + // Renaming a link over the target publishes whatever the link points at. + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/test-dir/x.tmp", platform: "linux" }), + ).rejects.toThrow(StagingPathError) + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) +}) + +describe("cleanup before a rollback failure is reported", () => { + beforeEach(() => mockDefaults()) + + it("releases the staged file and its own staging directory before throwing", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + let callCount = 0 + vi.mocked(fs.rename).mockImplementation(async () => { + callCount++ + if (callCount === 1) return // target -> backup + if (callCount === 2) throw new Error("ENOSPC") // temp -> target fails + throw new Error("EACCES") // the rollback rename fails too + }) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow( + RollbackFailureError, + ) + + // The backup is what the caller can still recover, so it stays on disk; the + // staging file and this write's own directory must not leak alongside it. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + expect(fs.rmdir).toHaveBeenCalled() + + const failingRenameOrder = vi.mocked(fs.rename).mock.invocationCallOrder[2] + const unlinkOrder = vi.mocked(fs.unlink).mock.invocationCallOrder[0] + const rmdirOrder = vi.mocked(fs.rmdir).mock.invocationCallOrder[0] + expect(unlinkOrder).toBeGreaterThan(failingRenameOrder) + expect(rmdirOrder).toBeGreaterThan(failingRenameOrder) + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index b5f82a4f81..9f62d2aed4 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -56,6 +56,41 @@ export class RollbackFailureError extends Error { this.backupPath = backupPath } } + +/** + * A caller-supplied staging path that is not a file this write may publish: it + * sits outside the target's directory (so the commit rename would cross + * filesystems) or is not a regular file. Rejecting it before any write keeps the + * target from being replaced by whatever the path points at. + */ +export class StagingPathError extends Error { + readonly stagingPath: string + + constructor(message: string, stagingPath: string) { + super(message) + this.name = "StagingPathError" + this.stagingPath = stagingPath + } +} + +/** + * The commit rename succeeded but the parent-directory fsync did not, so the + * directory entry is not known to be durable. The content is at the target; the + * caller cannot assume it survives a crash. Reported as its own error so a + * successful return never claims durability the filesystem did not grant. + */ +export class PostCommitDurabilityError extends Error { + readonly targetPath: string + + constructor(targetPath: string, cause: unknown) { + super( + "The rename committed but the parent directory could not be fsynced -- the content is at the target path reported on this error, and the directory entry may not be durable.", + { cause }, + ) + this.name = "PostCommitDurabilityError" + this.targetPath = targetPath + } +} // -- helpers --------------------------------------------------------------- /** Generate a unique temp file name in the given directory. */ @@ -216,6 +251,29 @@ export async function safeWriteText( let stagingDir: string | null = null let tempPath: string if (options?.tempPath) { + // A caller-supplied staging file is only safe when it is the file this + // write is staging, not an arbitrary path. Two properties are checked: + // it must sit beside the resolved target (a rename across filesystems + // fails with EXDEV, and a path elsewhere lets a caller publish an + // unrelated file onto the target), and it must be a regular file rather + // than a link — renaming a link over the target publishes whatever the + // link points at, which is the same trust problem as writing through a + // dangling symlink in resolvePublishTarget. + const supplied = path.resolve(options.tempPath) + if (path.dirname(supplied) !== path.resolve(dirPath)) { + throw new StagingPathError( + `Staging file must sit in the target's directory (${dirPath}), got ${supplied}`, + supplied, + ) + } + const stagingStat = await fs.lstat(supplied) + if (stagingStat.isSymbolicLink() || !stagingStat.isFile()) { + throw new StagingPathError( + `Staging file must be a regular file, not ${stagingStat.isSymbolicLink() ? "a symlink" : "another file type"}`, + supplied, + ) + } + // The caller's own path is used as given; only the check is canonical. tempPath = options.tempPath } else { stagingDir = _stagingDir(dirPath) @@ -227,6 +285,9 @@ export async function safeWriteText( // Non-null only when the win32 step-2 block saved a successful DACL dump: // it gates the step-5 restore and is tracked for the cleanup unlinks. let daclDumpPath: string | null = null + // Set when the rollback itself fails, so cleanup runs before the error that + // reports the partial state is thrown. + let rollbackFailure: unknown = undefined try { // -- Step 1: write content to staging temp file ------------------- @@ -348,8 +409,14 @@ export async function safeWriteText( } finally { fsSync.closeSync(dirFd) } - } catch { - // best-effort: the content rename already committed + } catch (error: unknown) { + // The content rename committed, but the directory entry that + // points at it is not known to be durable. Reporting success + // here would let a caller believe the write survives a crash, + // so the failure is surfaced as its own error: the caller can + // still find the content at the target, it just cannot rely on + // the directory entry having reached the disk. + throw new PostCommitDurabilityError(targetPath, error) } } @@ -394,8 +461,11 @@ export async function safeWriteText( // The content survives only at the backup path now, and the canonical // target is gone. Reporting just the publish failure would leave the // caller with data it cannot find at the expected path, so the - // partial-failure state travels with the error. - throw new RollbackFailureError(originalError, rollbackError, backupPath) + // partial-failure state travels with the error. The staged temp file + // and this write's staging directory are released first: a rollback + // failure is already a hard enough state to reason about without also + // leaking the staging file. + rollbackFailure = rollbackError } } try { @@ -415,6 +485,10 @@ export async function safeWriteText( await fs.unlink(daclDumpPath).catch(() => {}) } + if (rollbackFailure) { + throw new RollbackFailureError(originalError, rollbackFailure, backupPath) + } + throw originalError } } From c4120b0578bb75756a24c7ed59fe331b590588ae Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Mon, 5 Oct 2026 21:14:20 +0800 Subject: [PATCH 03/23] fix(file-safety): propagate a non-ENOENT lstat failure in resolvePublishTarget (U1, issue 1375) The resolver may fall back to the given path only when lstat also reports the path as absent. An EACCES or EIO failure says nothing about whether the path is a link, so falling back would publish through a link we were not allowed to inspect. Focused tests added for both branches. --- .../__tests__/safeWriteText.spec.ts | 30 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 9 +++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 66878d8b9d..674df74070 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -1053,3 +1053,33 @@ describe("cleanup before a rollback failure is reported", () => { expect(rmdirOrder).toBeGreaterThan(failingRenameOrder) }) }) + +describe("resolvePublishTarget", () => { + beforeEach(() => mockDefaults()) + + it("propagates an lstat failure that is not ENOENT instead of falling back to the link path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fs.lstat).mockRejectedValue(eacces) + + // A failed lstat says nothing about whether the path is a link, so the + // fallback would publish through a link we were not allowed to inspect. + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eacces) + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("still falls back to the given path when lstat also reports the path as absent", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fs.lstat).mockRejectedValue(enoent) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // The fallback is the resolved path, not the string that was handed in. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), path.resolve(targetPath)) + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 9f62d2aed4..d2a55ab349 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -173,7 +173,14 @@ export async function resolvePublishTarget(absoluteFilePath: string): Promise undefined) + // Only a lstat that also reports the path as absent may fall back to the + // given path; a real lstat failure (EACCES, EIO) says nothing about whether + // the path is a link, so falling back would write through a link we were + // simply not allowed to inspect. + const linkStat = await fs.lstat(absoluteFilePath).catch((lstatError: unknown) => { + if (errorCode(lstatError) === "ENOENT") return undefined + throw lstatError + }) if (linkStat?.isSymbolicLink()) throw error return absoluteFilePath }) From 97b599d2e69243e48ff14e434a11cd689278d2a9 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Mon, 5 Oct 2026 22:30:44 +0800 Subject: [PATCH 04/23] fix(file-safety): keep the rollback pair typed and the mock stand-ins type-sound compile failed at the unit head on three points: - RollbackFailureError needs a string backupPath, but the throw now happens after cleanup, so the `string | null` narrowing was lost. The failure is now held as { error, backupPath }. - The async lstat stand-in is built on the Stats prototype so it satisfies fsSync.Stats. - The realpath/readlink mocks are typed to the real signatures; the readlink mock answers once because only the link path is read. tsc clean, 50 tests pass, ESLint --max-warnings=0 clean, no suppression change. --- .../__tests__/safeWriteText.spec.ts | 23 +++++++++++-------- src/services/file-safety/safeWriteText.ts | 8 ++++--- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 674df74070..007ff4d3c5 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -58,8 +58,12 @@ function _dirPath(filePath: string): string { } // Minimal Stats stand-in: the SUT only reads `.mode` from it. // Async lstat stand-in: the SUT only asks whether the path is a link or a file. -function _fileStats(isLink: boolean) { - return { isSymbolicLink: () => isLink, isFile: () => !isLink } +// Built on the Stats prototype so the mock value still satisfies fsSync.Stats. +function _fileStats(isLink: boolean): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats + s.isSymbolicLink = () => isLink + s.isFile = () => !isLink + return s } function mockDefaults(): void { @@ -951,10 +955,11 @@ describe("resolveLockKey", () => { beforeEach(() => mockDefaults()) it("canonicalizes the parent directory, not just the file", async () => { - vi.mocked(fs.realpath).mockImplementation(async (target: string) => { - if (target === "/tmp/linkdir/file.json") return "/real/dir/file.json" - if (target === "/real/dir") return "/real/dir" - return target + vi.mocked(fs.realpath).mockImplementation(async (target) => { + const key = String(target) + if (key === "/tmp/linkdir/file.json") return "/real/dir/file.json" + if (key === "/real/dir") return "/real/dir" + return key }) // The key is the canonical directory plus the basename, so a symlinked @@ -966,9 +971,9 @@ describe("resolveLockKey", () => { const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) vi.mocked(fs.realpath).mockRejectedValue(enoent) vi.mocked(fs.lstat).mockResolvedValue(_fileStats(true)) - vi.mocked(fs.readlink).mockImplementation(async (target: string) => - target === "/tmp/linkdir/file.json" ? "referent.json" : undefined, - ) + // Only the link path is read, so a single answer is enough and keeps the mock's + // return type matching fs.promises.readlink. + vi.mocked(fs.readlink).mockResolvedValue("referent.json") // Mid-commit a peer writer renames the referent away and back, so the key // must still be computable while the link dangles. diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index d2a55ab349..e378cbbae8 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -294,7 +294,9 @@ export async function safeWriteText( let daclDumpPath: string | null = null // Set when the rollback itself fails, so cleanup runs before the error that // reports the partial state is thrown. - let rollbackFailure: unknown = undefined + // Held as a pair so the reported error still names the path the content survived at; + // declaring it as `unknown` alone would lose the string narrowing at the throw site. + let rollbackFailure: { error: unknown; backupPath: string } | null = null try { // -- Step 1: write content to staging temp file ------------------- @@ -472,7 +474,7 @@ export async function safeWriteText( // and this write's staging directory are released first: a rollback // failure is already a hard enough state to reason about without also // leaking the staging file. - rollbackFailure = rollbackError + rollbackFailure = { error: rollbackError, backupPath } } } try { @@ -493,7 +495,7 @@ export async function safeWriteText( } if (rollbackFailure) { - throw new RollbackFailureError(originalError, rollbackFailure, backupPath) + throw new RollbackFailureError(originalError, rollbackFailure.error, rollbackFailure.backupPath) } throw originalError From 435be8b96b7620eefbf799aebfcf7000d33cb1da Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Mon, 5 Oct 2026 22:34:46 +0800 Subject: [PATCH 05/23] rebuild unit u2 on the fixed chain --- .../__tests__/safeWriteJson.lockKey.spec.ts | 183 ++++++++++++++++ src/utils/__tests__/safeWriteJson.test.ts | 196 ++++++++++++++++-- src/utils/safeWriteJson.ts | 167 +++++++-------- 3 files changed, 432 insertions(+), 114 deletions(-) create mode 100644 src/utils/__tests__/safeWriteJson.lockKey.spec.ts diff --git a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts new file mode 100644 index 0000000000..33989f8b81 --- /dev/null +++ b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts @@ -0,0 +1,183 @@ +// npx vitest run utils/__tests__/safeWriteJson.lockKey.spec.ts + +import * as os from "os" +import path from "path" +import type { BigIntStats } from "fs" +import * as fs from "fs/promises" +import { acquireFileLock } from "../fileLock" +import { safeWriteJson } from "../safeWriteJson" +import { resolveLockKey } from "../../services/file-safety/safeWriteText" + +vi.mock("../fileLock", () => ({ + acquireFileLock: vi.fn(async () => async () => {}), +})) + +vi.mock("fs/promises", async () => { + const actual = await vi.importActual("fs/promises") + return { ...actual, realpath: vi.fn(), lstat: vi.fn(), readlink: vi.fn() } +}) + +const mockedRealpath = vi.mocked(fs.realpath) +const mockedLstat = vi.mocked(fs.lstat) +const mockedReadlink = vi.mocked(fs.readlink) +const mockedAcquireFileLock = vi.mocked(acquireFileLock) + +const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + +// Each test creates a real temp directory so the real fs calls still work. +// doubles between tests so an implementation from one test cannot carry over. +const createdDirs: string[] = [] +async function makeDir(prefix: string): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)) + createdDirs.push(dir) + return dir +} + +beforeEach(() => { + mockedRealpath.mockReset() + mockedLstat.mockReset() + mockedReadlink.mockReset() + mockedAcquireFileLock.mockReset() +}) + +afterEach(async () => { + for (const dir of createdDirs) { + await fs.rm(dir, { recursive: true, force: true }).catch(() => undefined) + } + createdDirs.length = 0 +}) + +// Only isSymbolicLink() is consulted by the guard, so the double carries just +// that method. The mocks reject asynchronously: a synchronous throw would bypass +// resolvePublishTarget's catch and skip the ENOENT/symlink branch under test. +const symlinkStat = (target: unknown) => ({ + isSymbolicLink: () => target === currentLink, + // The staging-path check in safeWriteText also asks whether the path is a + // regular file, so the double carries that predicate as well. + isFile: () => target !== currentLink, +}) as unknown as BigIntStats +let currentLink = "" + +describe("safeWriteJson lock key under a peer commit", () => { + it("waits for the peer instead of rejecting, and locks the referent", async () => { + const order: string[] = [] + const dir = await makeDir("lockkey-") + const referent = path.join(dir, "history_item.json") + currentLink = path.join(dir, "link.json") + + // The peer writer has renamed the referent away and has not committed yet, + // so the first resolution fails with ENOENT while lstat still reports a + // symbolic link. A strict resolve here rejects the caller before it can ever + // queue behind the peer, and the caller's delta write is lost. + mockedRealpath + .mockImplementationOnce(async () => { + order.push("resolve-failed") + throw enoent + }) + .mockImplementation(async (target) => { + order.push("resolve") + // The second call happens under the lock, where the peer has committed. + return target === currentLink ? referent : String(target) + }) + mockedLstat.mockImplementation(async (target) => { + order.push("lstat") + return symlinkStat(target) + }) + mockedReadlink.mockImplementation(async (target) => + target === currentLink ? referent : Promise.reject(new Error("not a link")), + ) + mockedAcquireFileLock.mockImplementation(async () => { + order.push("lock") + return async () => {} + }) + + await safeWriteJson(currentLink, { id: "task-1" }) + + // The lock key is the key every other writer to this file uses, so the caller + // queued behind the peer instead of failing before the lock. + expect(mockedAcquireFileLock).toHaveBeenCalledWith(referent) + // The trailing lstat is safeWriteText's staging-path check on the temp file + // this write created: it runs after the key was resolved and the lock taken, + // so it does not change which lock the caller queued behind. + expect(order).toEqual(["resolve-failed", "lstat", "resolve", "resolve", "lock", "resolve", "resolve", "lstat"]) + expect(JSON.parse(await fs.readFile(referent, "utf8"))).toEqual({ id: "task-1" }) + }) + + it("releases the lock when the resolution under the lock rejects", async () => { + const order: string[] = [] + let released = false + const dir = await makeDir("lockkey-") + const referent = path.join(dir, "history_item.json") + currentLink = path.join(dir, "link.json") + + // A real dangling link: the walk tolerates it so the caller can queue behind + // the peer, but once the lock is held the strict rejection still applies. A + // rejection outside the protected block would leave the lock held until the + // stale timeout for every other writer to the same file. + mockedRealpath.mockImplementation(async () => { + throw enoent + }) + mockedLstat.mockImplementation(async (target) => { + order.push("lstat") + return symlinkStat(target) + }) + mockedReadlink.mockImplementation(async (target) => + target === currentLink ? referent : Promise.reject(new Error("not a link")), + ) + mockedAcquireFileLock.mockImplementation(async () => { + order.push("lock") + return async () => { + order.push("release") + released = true + } + }) + + await expect(safeWriteJson(currentLink, { id: "task-1" })).rejects.toThrow(enoent) + expect(released).toBe(true) + // The strict rejection is reached through the ENOENT + symlink branch, not + // through a synchronous throw that skips it. + expect(order).toEqual(["lstat", "lock", "lstat", "release"]) + }) + + it("canonicalizes the parent directory when the file itself is not there yet", async () => { + // fs.realpath canonicalizes every component, including a symlinked ancestor + // directory or a Windows 8.3 short name. If the fallback returns the alias + // directory, the key depends on whether the file exists at the moment the key + // is computed, and a writer that resolved the canonical directory takes a + // different lock for the same file. + const aliasDir = path.join(os.tmpdir(), "alias-dir") + const canonicalDir = path.join(os.tmpdir(), "canonical-dir") + const file = path.join(aliasDir, "history_item.json") + mockedRealpath.mockImplementation(async (target) => { + if (target === file) throw enoent + return canonicalDir + }) + mockedLstat.mockImplementation(async () => ({ isSymbolicLink: () => false, isFile: () => true }) as unknown as BigIntStats) + + expect(await resolveLockKey(file)).toBe(path.join(canonicalDir, "history_item.json")) + }) +}) + +it("does not log a cleanup error when the safety net finds the temp file already gone", async () => { + // safeWriteText removes its own temp file on failure, so the safety net in + // safeWriteJson normally finds it gone. That is the expected outcome, not a + // second failure, and it must not be logged as one. + const dir = await makeDir("cleanup-") + const target = path.join(dir, "history_item.json") + currentLink = "" + mockedRealpath.mockImplementation(async (t) => String(t)) + mockedLstat.mockImplementation(async (t) => symlinkStat(t)) + + const renameSpy = vi.spyOn(fs, "rename").mockRejectedValue(new Error("commit rename failed")) + const unlinkSpy = vi.spyOn(fs, "unlink").mockRejectedValue(enoent) + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(safeWriteJson(target, { id: "task-1" })).rejects.toThrow("commit rename failed") + + // Only the original failure is reported. + expect(consoleError).toHaveBeenCalledTimes(1) + + renameSpy.mockRestore() + unlinkSpy.mockRestore() + consoleError.mockRestore() +}) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 79d08678a0..245af61910 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -4,6 +4,8 @@ import * as path from "path" import * as os from "os" import { safeWriteJson } from "../safeWriteJson" +import { RollbackFailureError } from "../../services/file-safety/safeWriteText" +import * as lockfile from "proper-lockfile" // Capture actual implementations before the vi.mock factory runs, // so they are never wrapped by vi.fn() — avoids infinite recursion when @@ -312,9 +314,8 @@ describe("safeWriteJson", () => { expect(content).toEqual(newData) }) - // Test for console error suppression during backup deletion - test("should suppress console.error when backup deletion fails", async () => { - const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error + // Test for best-effort backup deletion (the backup lifecycle now lives in safeWriteText) + test("does not fail the write when backup deletion fails (orphaned backup is acceptable)", async () => { const initialData = { message: "Initial" } const newData = { message: "New" } @@ -322,18 +323,23 @@ describe("safeWriteJson", () => { // 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_")) { + if (filePath.toString().includes("safeWriteText.bak_")) { throw new Error("Backup deletion failed") } return fsPromisesActuals.unlink!(filePath) }) + // The write must still succeed: backup cleanup is best-effort inside + // safeWriteText and never masks the committed content. await safeWriteJson(currentTestFilePath, newData) - // Verify console.error was called with the expected message - expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Successfully wrote"), expect.any(Error)) + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual(newData) + + // The orphaned backup is still on disk because its deletion failed. + const entries = await fs.readdir(tempDir) + expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(true) - consoleErrorSpy.mockRestore() vi.mocked(fs.unlink).mockRestore() }) @@ -434,9 +440,9 @@ 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 () => { - const initialData = { message: "Initial, should be lost if rollback fails" } + // Test for rollback failure scenario (the rollback rename now lives in safeWriteText) + test("re-throws the original error when the rollback rename fails, leaving an orphaned backup", async () => { + const initialData = { message: "Initial, orphaned when rollback fails" } const newData = { message: "New content" } await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) @@ -451,20 +457,34 @@ describe("safeWriteJson", () => { // Second call: tempNewFilePath -> filePath (fail) throw new Error("Primary rename failed") } else if (renameCallCount === 3) { - // Third call: tempBackupFilePath -> filePath (rollback, also fail) + // Third call: backup -> filePath (rollback, also fail) 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 original error must propagate, not the rollback error + // The rollback also failed, so the error reports the partial state: the publish + // failure stays the cause and the backup location is named. + let failure: RollbackFailureError | undefined + await safeWriteJson(currentTestFilePath, newData).catch((e: unknown) => { + if (e instanceof RollbackFailureError) { + failure = e + return + } + throw e + }) + + expect(failure).toBeInstanceOf(RollbackFailureError) + expect(failure?.cause).toBeInstanceOf(Error) + expect(failure?.rollbackError).toBeInstanceOf(Error) + expect(failure?.backupPath).toContain("safeWriteText.bak_") - // 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 rollback failed inside safeWriteText, so the target is gone and + // the backup is orphaned on disk. + expect(await fileExists(currentTestFilePath)).toBe(false) + const entries = await fs.readdir(tempDir) + expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(true) consoleErrorSpy.mockRestore() }) @@ -542,4 +562,144 @@ describe("safeWriteJson", () => { const content = await readFileContent(currentTestFilePath) expect(content).toEqual({ c: 3 }) }) + + // The commit rename targets the symlink referent. The staged temp file must + // therefore be created beside the RESOLVED target — staging beside the link + // would make the commit rename fail with EXDEV when the referent is on + // another filesystem. (Real symlinks are unavailable in this CI lane, so the + // resolution is simulated by mocking fs.realpath the same way.) + test("stages the temp file beside the symlink referent and commits onto it", async () => { + const referentDir = path.join(tempDir, "referent") + const linkDir = path.join(tempDir, "link") + await fs.mkdir(referentDir, { recursive: true }) + await fs.mkdir(linkDir, { recursive: true }) + // caller-visible path (the link) vs the resolved referent path + const callerPath = path.join(linkDir, "test-file.json") + const referentPath = path.join(referentDir, "test-file.json") + // Seed the RESOLVED referent with real content (via the actual fs) so the + // write exercises replacement of an EXISTING referent: the lock, the + // backup, and the commit all target the resolved referent. + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: true })) + + // Only the file resolves through the link; the directory is already canonical, + // so the lock key is the referent rather than the alias directory + basename. + vi.spyOn(fs, "realpath").mockImplementation(async (target) => + target === callerPath ? referentPath : String(target), + ) + + await safeWriteJson(callerPath, { after: true }) + + // the temp file was created next to the resolved referent, NOT beside the link + const tempPaths = vi.mocked(fsSyncActual.createWriteStream).mock.calls.map((call) => String(call[0])) + expect(tempPaths.some((p) => p.startsWith(referentDir + path.sep) && p.includes(".new_"))).toBe(true) + expect(tempPaths.some((p) => p.startsWith(linkDir + path.sep))).toBe(false) + + // the content was committed onto the referent + expect(await readFileContent(referentPath)).toEqual({ after: true }) + }) + + // proper-lockfile with realpath:false keys the lock by the given path, so a + // symlink alias and its referent must coordinate through ONE lock on the + // resolved referent — otherwise a concurrent merge through both aliases + // reads the same JSON and overwrites one update. (Real symlinks are + // unavailable in this CI lane, so the resolution is simulated by mocking + // fs.realpath, the same way as the staging test above.) + test("acquires the lock on the resolved referent, not the caller alias", async () => { + vi.resetModules() // fresh module instances so the doMock below is picked up + + const referentDir = path.join(tempDir, "lock-referent") + const linkDir = path.join(tempDir, "lock-link") + await fs.mkdir(referentDir, { recursive: true }) + await fs.mkdir(linkDir, { recursive: true }) + // caller-visible path (the link) vs the resolved referent path + const callerPath = path.join(linkDir, "locked.json") + const referentPath = path.join(referentDir, "locked.json") + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: 1 })) + + // Only the file resolves through the link; the directory is already canonical, + // so the lock key is the referent rather than the alias directory + basename. + const realpathSpy = vi + .spyOn(fs, "realpath") + .mockImplementation(async (target) => (target === callerPath ? referentPath : String(target))) + + // Wrap the real lock in a capturing mock, and drive the two rare error paths + // (the onCompromised callback and a failing release) so they stay covered + // without real lockfile staleness. The callback rethrows by design, so + // the mock swallows that throw and lets the real lock proceed. + const realLockfile = await vi.importActual("proper-lockfile") + const lockMockFn = vi.fn( + async ( + file: Parameters[0], + options?: Parameters[1], + ) => { + try { + options?.onCompromised?.(new Error("lock compromised (test)")) + } catch { + // onCompromised rethrows by design; swallow so the real lock proceeds. + } + const release = await realLockfile.lock(file, options) + return async () => { + await release() + throw new Error("release failed (test)") + } + }, + ) + const lockMock = lockMockFn as unknown as typeof realLockfile.lock + vi.doMock("proper-lockfile", () => ({ + ...realLockfile, + lock: lockMock, + })) + + // Re-import safeWriteJson so it picks up the mocked proper-lockfile. + const { safeWriteJson: mockedSafeWriteJson } = await import("../safeWriteJson") + + const mergeFn = vi.fn((existing: unknown, incoming: unknown) => ({ + ...(existing as Record), + ...(incoming as Record), + })) + + // Capture the compromise + release-failure logs. + const consoleErrorSpy = vi.spyOn(console, "error") + try { + await mockedSafeWriteJson(callerPath, { added: true }, { merge: mergeFn }) + + // The lock was keyed by the resolved referent — every alias shares it. + expect(lockMock).toHaveBeenCalledTimes(1) + expect(String(lockMockFn.mock.calls[0][0])).toBe(referentPath) + // The merge read the referent's content through that single lock. + expect(mergeFn).toHaveBeenCalledWith({ seed: 1 }, { added: true }) + expect(await readFileContent(referentPath)).toEqual({ seed: 1, added: true }) + // The compromise callback and the failed release were logged, not thrown. + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("was compromised"), expect.any(Error)) + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("Failed to release lock"), + expect.any(Error), + ) + } finally { + // Cleanup must run even when an assertion fails: a leaked mock + // registration or console spy changes later tests, and vi.unmock + // alone does not reset a module that already imported the mock. + realpathSpy.mockRestore() + vi.unmock("proper-lockfile") + vi.resetModules() + consoleErrorSpy.mockRestore() + } + }) + + // CWE-732 regression: safeWriteJson stages the temp itself and passes it + // via tempPath, so safeWriteText must apply the existing target's mode to + // the staged temp before the atomic rename — otherwise a 0o600 target is + // published as 0o644. POSIX-only assertion (Windows ignores POSIX modes). + test.skipIf(process.platform === "win32")( + "preserves a restrictive 0o600 target mode through the atomic publish", + async () => { + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify({ before: true })) + fsSyncActual.chmodSync(currentTestFilePath, 0o600) + + await safeWriteJson(currentTestFilePath, { after: true }) + + expect(fsSyncActual.statSync(currentTestFilePath).mode & 0o777).toBe(0o600) + expect(await readFileContent(currentTestFilePath)).toEqual({ after: true }) + }, + ) }) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 7da68b2a7a..bae200dd38 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -4,6 +4,12 @@ import * as path from "path" import { JsonStreamStringify } from "json-stream-stringify" import { acquireFileLock } from "./fileLock" +import { + resolveLockKey, + resolvePublishTarget, + safeWriteText, + type SafeWriteTextOptions, +} from "../services/file-safety/safeWriteText" /** * Options for safeWriteJson function @@ -32,7 +38,7 @@ 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. + * - Writes to a temporary file first via JsonStreamStringify streaming. * - 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. @@ -42,7 +48,6 @@ export interface SafeWriteJsonOptions { * @param {SafeWriteJsonOptions} options - Optional configuration for JSON formatting. * @returns {Promise} */ - async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJsonOptions): Promise { const absoluteFilePath = path.resolve(filePath) let releaseLock = async () => {} // Initialized to a no-op @@ -51,38 +56,46 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso const dirPath = path.dirname(absoluteFilePath) // Ensure directory structure exists with improved reliability + // Declared outside the protected block so the catch and finally can still name + // the target when the resolution itself rejects. + let resolvedTargetPath: string | undefined + try { - // Create directory with recursive option await fs.mkdir(dirPath, { recursive: true }) - - // Verify directory exists after creation attempt await fs.access(dirPath) } catch (dirError: any) { console.error(`Failed to create or access directory for ${absoluteFilePath}:`, dirError) throw dirError } - // Acquire the lock before any file operations. `acquireFileLock` owns the - // shared advisory lock protocol, so callers that lock the same path with - // it (for example task-history deletion) serialize with this write. - // If lock acquisition fails, it throws immediately. The releaseLock - // remains a no-op, so the finally block in the main file operations - // try-catch-finally won't try to release an unacquired lock if this - // path is taken. - releaseLock = await acquireFileLock(absoluteFilePath) + // Lock key: the symlink referent when the path is an existing symlink, so a + // symlink alias and its referent share one lock. The key must be computable + // while a peer writer is mid-commit (backup mode renames the referent away and + // back), so the walk tolerates a dangling link instead of rejecting it here. + const lockKey = await resolveLockKey(absoluteFilePath) - // Variables to hold the actual paths of temp files if they are created. + // Acquire the lock before any file operations. If acquisition fails it throws + // immediately, and releaseLock stays a no-op so the finally block does not try + // to release an unacquired lock. + releaseLock = await acquireFileLock(lockKey) + + // Variables to hold the actual path of the temp file if it is created. let actualTempNewFilePath: string | null = null - let actualTempBackupFilePath: string | null = null try { + // Resolve the publish target under the lock: the peer has committed by now, so + // the strict dangling-link rejection still applies to a real dangling link. It + // must stay inside the protected block, otherwise a rejection here leaves the + // advisory lock held until the stale timeout for every other writer. + resolvedTargetPath = await resolvePublishTarget(absoluteFilePath) + // 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. if (options?.merge) { let existing: unknown = null try { - existing = JSON.parse(await fs.readFile(absoluteFilePath, "utf8")) + existing = JSON.parse(await fs.readFile(resolvedTargetPath, "utf8")) } catch (error: unknown) { const code = error && typeof error === "object" && "code" in error ? (error as { code: string }).code : undefined @@ -93,111 +106,73 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso data = options.merge(existing, data) } - // Step 1: Write data to a new temporary file. + // Step 1: Write data to a new temporary file via JSON streaming. + // Stage it beside the *resolved* target (the symlink referent when the path is + // a symlink; resolvedTargetPath above): safeWriteText commits by renaming + // onto that referent, and a rename across filesystems would fail with EXDEV. actualTempNewFilePath = path.join( - path.dirname(absoluteFilePath), - `.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, + path.dirname(resolvedTargetPath), + ".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. + // Step 2: Delegate backup + commit + rollback to safeWriteText with the + // pre-written temp path. backup:true keeps the old safeWriteJson + // semantics (target -> backup before commit, rollback on failure) and + // keeps the target in place until safeWriteText captures its Windows + // DACL (safeWriteText dumps the DACL before its own backup rename and + // restores it onto the directory after the commit rename). + const textOptions: SafeWriteTextOptions = { + tempPath: actualTempNewFilePath, + backup: true, } - // Step 3: Rename the new temporary file to the target file path. - // This is the main "commit" step. - await fs.rename(actualTempNewFilePath, absoluteFilePath) + await safeWriteText(resolvedTargetPath, "", textOptions) - // 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" + // If we reach here, the new file is successfully in place and any + // backup has already been handled by safeWriteText. 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, - ) - } - } } catch (originalError) { - console.error(`Operation failed for ${absoluteFilePath}: [Original Error Caught]`, originalError) + console.error( + `Operation failed for ${resolvedTargetPath ?? 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 + // A failed safeWriteText already rolled the backup (if any) back to + // the target path. Clean up the .new file if it still exists + // (safeWriteText also cleans up its tempPath on failure; this is a + // safety net in case its cleanup missed it). if (newFileToCleanupWithinCatch) { try { await fs.unlink(newFileToCleanupWithinCatch) - } catch (cleanupError) { - console.error( - `[Catch] Failed to clean up temporary new file ${newFileToCleanupWithinCatch}:`, - cleanupError, - ) + } catch (cleanupError: unknown) { + // The expected case: safeWriteText already removed its own temp file, so a + // missing file here is not a cleanup failure worth logging. Returning would + // also swallow the original error the caller needs. + const isAbsent = + typeof cleanupError === "object" && + cleanupError !== null && + "code" in cleanupError && + cleanupError.code === "ENOENT" + if (!isAbsent) { + 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. try { - // releaseLock will be the actual unlock function if lock was acquired, - // or the initial no-op if acquisition failed. await releaseLock() } catch (unlockError) { - // Do not re-throw here, as the originalError from the try/catch (if any) is more important. - console.error(`Failed to release lock for ${absoluteFilePath}:`, unlockError) + console.error(`Failed to release lock for ${resolvedTargetPath ?? absoluteFilePath}:`, unlockError) } } } From 625a976dc2bee3b7d3446e1fdaceaf4e2d4fc5ec Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Mon, 5 Oct 2026 22:47:25 +0800 Subject: [PATCH 06/23] chore(lint): prune the safeWriteJson suppression this unit earns The any usage this entry covered is gone in the rewritten file, so the count drops 4 -> 3. eslint --prune-suppressions --max-warnings=0 confirms it. --- src/eslint-suppressions.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 583485c628..53ce332bb3 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1716,7 +1716,7 @@ }, "utils/safeWriteJson.ts": { "@typescript-eslint/no-explicit-any": { - "count": 4 + "count": 3 } }, "utils/tts.ts": { From cf86a42d6057d9c6c9078f584a08a862e81a4cf3 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 02:48:40 +0800 Subject: [PATCH 07/23] fix(file-safety): inherit unit 1 committed guard and exact rmdir assertion This unit was rebuilt from the pre-fix content source, so its copy of safeWriteText.ts still restored the backup over a write whose commit rename had already succeeded when the parent-directory fsync failed, and its spec asserted rmdir generically rather than against the staging directory this write created. Both are already settled in unit 1 (fws/u1-atomic-publish). Taking those files here keeps the shared code byte-identical across the units, so merging the chain in order does not overwrite unit 1's fix. Tests: 52 passed in safeWriteText.spec.ts. --- src/services/file-safety/safeWriteText.ts | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index e378cbbae8..2aafd0cce0 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -167,11 +167,7 @@ async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRu */ export async function resolvePublishTarget(absoluteFilePath: string): Promise { return fs.realpath(absoluteFilePath).catch(async (error: unknown) => { - const code = - typeof error === "object" && error !== null && "code" in error - ? (error as { code?: string }).code - : undefined - if (code !== "ENOENT") throw error + if (errorCode(error) !== "ENOENT") throw error // ENOENT also covers a dangling symlink, which must never be written through. // Only a lstat that also reports the path as absent may fall back to the // given path; a real lstat failure (EACCES, EIO) says nothing about whether @@ -289,6 +285,10 @@ export async function safeWriteText( let backupPath: string | null = null let releaseBackupOnSuccess = false + // Set once the commit rename has published the new content. After that point the + // backup is no longer a safe restore source: rolling it back would overwrite + // content the caller can already observe at the target path. + let committed = false // Non-null only when the win32 step-2 block saved a successful DACL dump: // it gates the step-5 restore and is tracked for the cleanup unlinks. let daclDumpPath: string | null = null @@ -397,16 +397,13 @@ export async function safeWriteText( await fs.rename(targetPath, backupPath) releaseBackupOnSuccess = true } catch (err: unknown) { - const code = - typeof err === "object" && err !== null && "code" in err - ? (err as { code?: string }).code - : undefined - if (code !== "ENOENT") throw err + if (errorCode(err) !== "ENOENT") throw err } } // -- Step 4: atomic rename temp -> target --------------------- await fs.rename(tempPath, targetPath) + committed = true // -- Step 4b (POSIX): fsync the parent directory so the directory entry // changed by the commit rename is durable, not just the file content. @@ -463,7 +460,10 @@ export async function safeWriteText( await fs.rmdir(stagingDir).catch(() => {}) } } catch (originalError: unknown) { - if (backupPath && releaseBackupOnSuccess) { + // Only a pre-commit failure can restore the backup. Once the commit rename + // published, a later failure (for example the post-commit directory fsync) + // must not overwrite the published content with the old file. + if (backupPath && releaseBackupOnSuccess && !committed) { try { await fs.rename(backupPath, targetPath) } catch (rollbackError: unknown) { From a587a078595bfcd48676269ef0cfb158d16b5a67 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 03:04:44 +0800 Subject: [PATCH 08/23] test: rerun windows lane - local core suite is green (1073 tests), the failure did not reproduce From f7de7540c2f0ceb54096424970f21565bee40702 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 03:44:10 +0800 Subject: [PATCH 09/23] fix(file-safety): keep the publish error message in RollbackFailureError safeWriteJson rethrows RollbackFailureError and the telemetry callers record only error.message, so the generic wrapper message lost the filesystem errno text of the publish failure. Include the publish error message while keeping the rollback context (publishError, rollbackError, backupPath) unchanged. Tests: 52 passed in safeWriteText.spec.ts, ESLint clean with --max-warnings=0. --- src/services/file-safety/safeWriteText.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 2aafd0cce0..85c6a071ba 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -47,7 +47,7 @@ export class RollbackFailureError extends Error { constructor(publishError: unknown, rollbackError: unknown, backupPath: string) { super( - "Publish failed and the backup could not be restored to its original path -- the content is preserved at the backup location reported on this error.", + `Publish failed (${publishError instanceof Error ? publishError.message : String(publishError)}) and the backup could not be restored to its original path -- the content is preserved at the backup location reported on this error.`, { cause: publishError }, ) this.name = "RollbackFailureError" From 4ea379b6942ccf9f20744f8266cbd8c97603bf57 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 03:45:34 +0800 Subject: [PATCH 10/23] test(file-safety): assert the exact staging directory removed after a rollback failure The assertion accepted any rmdir argument, so a regression that removed a different directory still passed. Read this write's own staging directory from fsSync.mkdirSync and assert that exact path. Tests: 52 passed in safeWriteText.spec.ts, ESLint clean with --max-warnings=0. --- .../__tests__/safeWriteText.spec.ts | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 007ff4d3c5..99fd6291fb 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -348,6 +348,26 @@ describe("safeWriteText", () => { // temp-shaped is unlinked afterwards expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) }) + + it("a failed post-commit directory fsync does not roll the backup back over the published content", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const dirPath = path.dirname(targetPath) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // The file fd opens normally; the parent-directory open after the commit + // rename fails, which is the post-commit durability failure. + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) throw new Error("EBADF") + return 1 + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow(PostCommitDurabilityError) + + // The commit rename already published the new content, so the backup must + // not be renamed back over it: only target->backup and temp->target run. + expect(fs.rename).toHaveBeenNthCalledWith(1, targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + expect(fs.rename).toHaveBeenCalledTimes(2) + }) }) // ── Test 4: backup:true keeps old safeWriteJson semantics incl. rollback ── @@ -546,6 +566,28 @@ describe("safeWriteText", () => { expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) }) + it("win32 DACL save runs before the backup rename, not after it", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // The title is about order, so assert the order the mocks were actually + // called in. If the save ran after the backup rename the target would + // already be gone and the dump would describe the wrong file. + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + const callOrder = vi.mocked(execFile).mock.invocationCallOrder + const renameOrder = vi.mocked(fs.rename).mock.invocationCallOrder + const saveCall = callOrder[0] + const restoreCall = callOrder[1] + const backupRename = renameOrder[0] + const commitRename = renameOrder[1] + + expect(saveCall).toBeLessThan(backupRename) + expect(backupRename).toBeLessThan(commitRename) + expect(commitRename).toBeLessThan(restoreCall) + }) + it("win32 DACL: dump is unlinked even when restore fails", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) @@ -1049,7 +1091,9 @@ describe("cleanup before a rollback failure is reported", () => { // The backup is what the caller can still recover, so it stays on disk; the // staging file and this write's own directory must not leak alongside it. expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) - expect(fs.rmdir).toHaveBeenCalled() + const stagingDirs = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => String(call[0])) + expect(stagingDirs.length).toBe(1) + expect(fs.rmdir).toHaveBeenCalledWith(stagingDirs[0]) const failingRenameOrder = vi.mocked(fs.rename).mock.invocationCallOrder[2] const unlinkOrder = vi.mocked(fs.unlink).mock.invocationCallOrder[0] From 8ba6fcadab88d3f1048668068268f8d7fd1f91c6 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 05:21:34 +0800 Subject: [PATCH 11/23] test: re-trigger required checks - the queued runs were cancelled by the Actions queue, no source change From 2945ab23f4ce035c5640092196c97526e707eed2 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 06:47:12 +0800 Subject: [PATCH 12/23] fix(file-safety): give the Windows DACL dump a per-write name The dump path was the fixed sibling .acl.tmp. A pre-existing user file at that path is unlinked by the failed-save branch and by both cleanup paths, and two concurrent writes to the same target share one dump, so one write can restore or delete the other's. Use the per-write unique name like the staging file. Tests: 52 passed in safeWriteText.spec.ts, ESLint clean with --max-warnings=0. --- .../file-safety/__tests__/safeWriteText.spec.ts | 12 ++++++------ src/services/file-safety/safeWriteText.ts | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 99fd6291fb..8b76906dda 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -170,7 +170,7 @@ describe("safeWriteText", () => { // through the default icacls path (options?.execFileRunner must // not throw when options is undefined) expect(vi.mocked(execFile)).toHaveBeenCalledTimes(2) - expect(vi.mocked(fs.unlink)).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + expect(vi.mocked(fs.unlink)).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) } }) @@ -535,7 +535,7 @@ describe("safeWriteText", () => { const saveArgs = vi.mocked(execFile).mock.calls[0]?.[1] expect(saveArgs?.[1]).toBe("/save") // the dump path (possibly partially created by icacls) was unlinked - expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) }) it("win32 DACL save args are [targetPath, /save, dumpPath, /T] before backup rename", async () => { @@ -551,7 +551,7 @@ describe("safeWriteText", () => { // First call: save DACL from target before backup rename const firstCall = vi.mocked(execFile).mock.calls[0] expect(firstCall[0]).toBe("icacls") - expect(firstCall[1]).toEqual([targetPath, "/save", expect.stringContaining(".acl.tmp"), "/T"]) + expect(firstCall[1]).toEqual([targetPath, "/save", expect.stringContaining("safeWriteText.acl"), "/T"]) // Second call: restore DACL onto directory after commit rename const secondCall = vi.mocked(execFile).mock.calls[1] @@ -559,11 +559,11 @@ describe("safeWriteText", () => { expect(secondCall[1]).toEqual([ expect.stringContaining("/tmp/test-dir"), "/restore", - expect.stringContaining(".acl.tmp"), + expect.stringContaining("safeWriteText.acl"), ]) // dump file was unlinked after restore - expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) }) it("win32 DACL save runs before the backup rename, not after it", async () => { @@ -610,7 +610,7 @@ describe("safeWriteText", () => { expect(fs.rename).toHaveBeenCalledTimes(1) // dump file was still unlinked in finally - expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) }) it("win32 DACL: when target does not exist, no save/restore/dump", async () => { diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 85c6a071ba..e548e5c3b5 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -370,7 +370,7 @@ export async function safeWriteText( if (platform === "win32") { try { await fs.access(targetPath) // target exists? - const dumpPath = targetPath + ".acl.tmp" + const dumpPath = _tempName(dirPath, "safeWriteText.acl") const saved = await _saveDaclWindows(targetPath, dumpPath, options?.execFileRunner) if (saved) { // Only a successfully saved dump may be restored onto the From 8c8db375e18594f7bdbf4d207a5a79f155f31bfa Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 17 Oct 2026 01:53:10 +0800 Subject: [PATCH 13/23] fix(file-safety): keep the published target across a post-commit durability error Port of the U3 commit-state tracking fix to this unit's copies (same PI row family, identical catch shape): - safeWriteJson clears the staged path when safeWriteText reports PostCommitDurabilityError. The rename landed, so the staged path is the published target's former name; the catch cleanup must not unlink a path this write no longer owns (a concurrent writer reusing the staging name would lose its temp file). The durability error rethrows, so the write is never observed as a confirmed-durable success. - safeWriteText's failure cleanup only removes the staged temp when the commit rename never landed (the committed flag it already tracks); after the commit the staged path names the published target. Regression test: the parent-directory fsync fails after the commit rename; safeWriteJson rejects with PostCommitDurabilityError, the target stays present with the new JSON, and no unlink fires at the consumed staging path. Negative controls: removing either guard reddens exactly that test. 80/81 with the file-safety suites (1 pre-existing skip). --- src/services/file-safety/safeWriteText.ts | 14 +++++-- src/utils/__tests__/safeWriteJson.test.ts | 45 +++++++++++++++++++++-- src/utils/safeWriteJson.ts | 11 ++++++ 3 files changed, 63 insertions(+), 7 deletions(-) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index e548e5c3b5..4326ea2c3a 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -477,10 +477,16 @@ export async function safeWriteText( rollbackFailure = { error: rollbackError, backupPath } } } - try { - await fs.unlink(tempPath).catch(() => {}) - } catch { - // cleanup failure is non-fatal + // Only a write that never committed has a staged file to remove. After the + // commit rename the staged path names the published target: unlinking it + // here would at best miss with ENOENT, and at worst delete a file a + // concurrent writer has already claimed under the same staging name. + if (!committed) { + try { + await fs.unlink(tempPath).catch(() => {}) + } catch { + // cleanup failure is non-fatal + } } // A failed self-staged write must not leave its staging directory behind. diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 245af61910..451bfd3a82 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -4,7 +4,7 @@ import * as path from "path" import * as os from "os" import { safeWriteJson } from "../safeWriteJson" -import { RollbackFailureError } from "../../services/file-safety/safeWriteText" +import { PostCommitDurabilityError, RollbackFailureError } from "../../services/file-safety/safeWriteText" import * as lockfile from "proper-lockfile" // Capture actual implementations before the vi.mock factory runs, @@ -441,7 +441,7 @@ describe("safeWriteJson", () => { }) // Test for rollback failure scenario (the rollback rename now lives in safeWriteText) - test("re-throws the original error when the rollback rename fails, leaving an orphaned backup", async () => { + test("throws RollbackFailureError with the publish failure as cause when the rollback rename also fails, leaving an orphaned backup", async () => { const initialData = { message: "Initial, orphaned when rollback fails" } const newData = { message: "New content" } @@ -463,7 +463,6 @@ describe("safeWriteJson", () => { return fsPromisesActuals.rename!(oldPath, newPath) }) - // The original error must propagate, not the rollback error // The rollback also failed, so the error reports the partial state: the publish // failure stays the cause and the backup location is named. let failure: RollbackFailureError | undefined @@ -702,4 +701,44 @@ describe("safeWriteJson", () => { expect(await readFileContent(currentTestFilePath)).toEqual({ after: true }) }, ) + + // ── post-commit durability: the published target must survive (PI regression) ── + + it("keeps the published target with its new content when the post-commit directory fsync fails", async () => { + const fsSyncMod = await import("fs") + const realOpen = fsSyncMod.openSync + const realFsync = fsSyncMod.fsyncSync + // A sentinel fd identifies the parent-directory open that safeWriteText + // performs after the commit rename; only that fsync fails. + const sentinel = 987654 + vi.spyOn(process, "platform", "get").mockReturnValue("linux") + vi.spyOn(fsSyncMod, "openSync").mockImplementation(((...args: unknown[]) => { + if (String(args[0]) === tempDir) { + return sentinel + } + return (realOpen as (...a: unknown[]) => unknown)(...args) + }) as never) + vi.spyOn(fsSyncMod, "fsyncSync").mockImplementation(((fd: number) => { + if (fd === sentinel) { + throw new Error("EIO fsync") + } + return (realFsync as (f: number) => unknown)(fd) + }) as never) + + // Earlier tests in this file leave unlink history on the shared mock. + vi.mocked(fs.unlink).mockClear() + + await expect(safeWriteJson(currentTestFilePath, { fresh: "content" })).rejects.toBeInstanceOf( + PostCommitDurabilityError, + ) + + // The rename committed: the target must still hold the new JSON, not be + // unlinked away by the catch cleanup of the consumed staging path. + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual({ fresh: "content" }) + // The cleanup must not unlink the consumed staging path: after the commit + // rename it is no longer this write's file to remove. + const stagingUnlinks = vi.mocked(fs.unlink).mock.calls.filter((call) => /\.new_.*\.tmp$/.test(String(call[0]))) + expect(stagingUnlinks).toHaveLength(0) + }) }) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index bae200dd38..999f563091 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -5,6 +5,7 @@ import { JsonStreamStringify } from "json-stream-stringify" import { acquireFileLock } from "./fileLock" import { + PostCommitDurabilityError, resolveLockKey, resolvePublishTarget, safeWriteText, @@ -139,6 +140,16 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso originalError, ) + // PostCommitDurabilityError means the commit rename landed: the staged + // path was renamed onto the target, so it is no longer a leftover temp + // file. Unlinking it here would at best race a concurrent writer that has + // already reused the staging name, and the published target must survive + // the durability failure with its new content. The error rethrows below, + // so the write is never observed as a confirmed-durable success. + if (originalError instanceof PostCommitDurabilityError) { + actualTempNewFilePath = null + } + const newFileToCleanupWithinCatch = actualTempNewFilePath // A failed safeWriteText already rolled the backup (if any) back to From 628fcafa9b6a0921b0ae62c55262fecd34d64ef7 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 17 Oct 2026 02:20:05 +0800 Subject: [PATCH 14/23] style(file-safety): drop imports unused after the main refresh (lint gate) --- src/services/file-safety/__tests__/safeWriteText.spec.ts | 1 - src/utils/__tests__/safeWriteJson.test.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 8b76906dda..ee8627a310 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -10,7 +10,6 @@ import { RollbackFailureError, safeWriteText, StagingPathError, - type SafeWriteTextOptions, } from "../safeWriteText" // Full mock for fs/promises — all methods are vi.fn() stubs diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 7a59427d78..2847ab0319 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -5,7 +5,6 @@ import * as os from "os" import { safeWriteJson } from "../safeWriteJson" import { PostCommitDurabilityError, RollbackFailureError } from "../../services/file-safety/safeWriteText" -import * as lockfile from "proper-lockfile" // Capture actual implementations before the vi.mock factory runs, // so they are never wrapped by vi.fn() — avoids infinite recursion when From 75dd374f03be86fc9692b06b514abea51fec14c0 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 17 Oct 2026 02:42:36 +0800 Subject: [PATCH 15/23] style(file-safety): reformat specs for the prettier version main now pins (format gate) --- .../file-safety/__tests__/safeWriteText.spec.ts | 4 +++- .../__tests__/safeWriteJson.lockKey.spec.ts | 17 ++++++++++------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index ee8627a310..df2484ee47 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -359,7 +359,9 @@ describe("safeWriteText", () => { return 1 }) - await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow(PostCommitDurabilityError) + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow( + PostCommitDurabilityError, + ) // The commit rename already published the new content, so the backup must // not be renamed back over it: only target->backup and temp->target run. diff --git a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts index 33989f8b81..8e20682a50 100644 --- a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts +++ b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts @@ -50,12 +50,13 @@ afterEach(async () => { // Only isSymbolicLink() is consulted by the guard, so the double carries just // that method. The mocks reject asynchronously: a synchronous throw would bypass // resolvePublishTarget's catch and skip the ENOENT/symlink branch under test. -const symlinkStat = (target: unknown) => ({ - isSymbolicLink: () => target === currentLink, - // The staging-path check in safeWriteText also asks whether the path is a - // regular file, so the double carries that predicate as well. - isFile: () => target !== currentLink, -}) as unknown as BigIntStats +const symlinkStat = (target: unknown) => + ({ + isSymbolicLink: () => target === currentLink, + // The staging-path check in safeWriteText also asks whether the path is a + // regular file, so the double carries that predicate as well. + isFile: () => target !== currentLink, + }) as unknown as BigIntStats let currentLink = "" describe("safeWriteJson lock key under a peer commit", () => { @@ -152,7 +153,9 @@ describe("safeWriteJson lock key under a peer commit", () => { if (target === file) throw enoent return canonicalDir }) - mockedLstat.mockImplementation(async () => ({ isSymbolicLink: () => false, isFile: () => true }) as unknown as BigIntStats) + mockedLstat.mockImplementation( + async () => ({ isSymbolicLink: () => false, isFile: () => true }) as unknown as BigIntStats, + ) expect(await resolveLockKey(file)).toBe(path.join(canonicalDir, "history_item.json")) }) From 6d0bc87422ad93a9c808c1e7bfb98df8e16a64cc Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 17 Oct 2026 03:00:42 +0800 Subject: [PATCH 16/23] test(file-safety): pin the durability wrapper's cause and the exact staging release Port of the U3 review follow-ups to this unit's copies: the injected fsync failure targets a real directory descriptor and the test asserts the PostCommitDurabilityError carries it as its cause; the staging-release assertions compare the exact directory path read back from the create instead of a substring. --- .../__tests__/safeWriteText.spec.ts | 19 ++++++++++----- src/utils/__tests__/safeWriteJson.test.ts | 24 ++++++++++++------- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index df2484ee47..bd47ba29e2 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -143,11 +143,14 @@ describe("safeWriteText", () => { await safeWriteText(targetPath, "hello", { platform: "linux" }) - // the staging subdir is removed best-effort after the commit rename - // (stringContaining: the SUT and the test helper resolve Windows - // drive-relative paths differently, as in the existing staging tests) + // the staging subdir is removed best-effort after the commit rename. + // Exact path read back from the create: the SUT releases the same + // directory it staged in, and a substring match would also accept a + // release aimed at some other staging directory. expect(fs.rmdir).toHaveBeenCalledTimes(1) - expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + const stagingDir = String(vi.mocked(fsSync.mkdirSync).mock.calls[0]?.[0]) + expect(stagingDir).toContain(".file-safety-staging") + expect(fs.rmdir).toHaveBeenCalledWith(stagingDir) // the win32 DACL restore gate must stay closed on other platforms: // no icacls save or restore is attempted expect(execFile).not.toHaveBeenCalled() @@ -163,7 +166,9 @@ describe("safeWriteText", () => { await expect(safeWriteText(targetPath, "hello")).resolves.toBeUndefined() expect(fs.rmdir).toHaveBeenCalledTimes(1) - expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + const stagingDir = String(vi.mocked(fsSync.mkdirSync).mock.calls[0]?.[0]) + expect(stagingDir).toContain(".file-safety-staging") + expect(fs.rmdir).toHaveBeenCalledWith(stagingDir) if (process.platform === "win32") { // default platform is win32: the DACL save + restore still ran // through the default icacls path (options?.execFileRunner must @@ -197,7 +202,9 @@ describe("safeWriteText", () => { // the commit rename still happened and the rmdir error was swallowed expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) expect(fs.rmdir).toHaveBeenCalledTimes(1) - expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + const stagingDir = String(vi.mocked(fsSync.mkdirSync).mock.calls[0]?.[0]) + expect(stagingDir).toContain(".file-safety-staging") + expect(fs.rmdir).toHaveBeenCalledWith(stagingDir) }) it("gives each self-staged write its own staging directory so a concurrent write cannot remove it", async () => { diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 2847ab0319..7fd8c670b1 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -707,19 +707,23 @@ describe("safeWriteJson", () => { const fsSyncMod = await import("fs") const realOpen = fsSyncMod.openSync const realFsync = fsSyncMod.fsyncSync - // A sentinel fd identifies the parent-directory open that safeWriteText - // performs after the commit rename; only that fsync fails. - const sentinel = 987654 + // The parent-directory open that safeWriteText performs after the commit + // rename gets a REAL descriptor: safeWriteText closes it in its finally + // block, and a fabricated number would make closeSync fail with EBADF - + // a second failure behind the same wrapper, hiding which fsync broke. + const fsyncFailure = new Error("EIO fsync") + let dirFd: number | undefined vi.spyOn(process, "platform", "get").mockReturnValue("linux") vi.spyOn(fsSyncMod, "openSync").mockImplementation(((...args: unknown[]) => { if (String(args[0]) === tempDir) { - return sentinel + dirFd = (realOpen as (p: string, f: string) => number)(tempDir, "r") + return dirFd } return (realOpen as (...a: unknown[]) => unknown)(...args) }) as never) vi.spyOn(fsSyncMod, "fsyncSync").mockImplementation(((fd: number) => { - if (fd === sentinel) { - throw new Error("EIO fsync") + if (fd === dirFd) { + throw fsyncFailure } return (realFsync as (f: number) => unknown)(fd) }) as never) @@ -727,9 +731,13 @@ describe("safeWriteJson", () => { // Earlier tests in this file leave unlink history on the shared mock. vi.mocked(fs.unlink).mockClear() - await expect(safeWriteJson(currentTestFilePath, { fresh: "content" })).rejects.toBeInstanceOf( - PostCommitDurabilityError, + const failure: unknown = await safeWriteJson(currentTestFilePath, { fresh: "content" }).then( + () => null, + (error: unknown) => error, ) + expect(failure).toBeInstanceOf(PostCommitDurabilityError) + // The wrapper must carry the injected fsync failure, not a cleanup error. + expect((failure as Error).cause).toBe(fsyncFailure) // The rename committed: the target must still hold the new JSON, not be // unlinked away by the catch cleanup of the consumed staging path. From 4fa16f236e5abdc47224dd9d61b9fa30f60a8bbb Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 17 Oct 2026 03:09:27 +0800 Subject: [PATCH 17/23] test(file-safety): keep the durability probe platform-portable Opening a directory with openSync(path, "r") is not portable: on the Windows CI runner it fails, and the failed open - not the injected fsync - became the wrapped cause, reddening test:misc there. The spy now uses a real descriptor where the platform allows it and falls back to a sentinel with a stubbed close where it does not, so the injected EIO fsync is the only error in flight and the cause assertion holds on every platform (verified by forcing the fallback path locally). --- src/utils/__tests__/safeWriteJson.test.ts | 25 +++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 7fd8c670b1..e796006013 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -708,19 +708,36 @@ describe("safeWriteJson", () => { const realOpen = fsSyncMod.openSync const realFsync = fsSyncMod.fsyncSync // The parent-directory open that safeWriteText performs after the commit - // rename gets a REAL descriptor: safeWriteText closes it in its finally - // block, and a fabricated number would make closeSync fail with EBADF - - // a second failure behind the same wrapper, hiding which fsync broke. + // rename gets a REAL descriptor where the platform allows it: the SUT + // closes the fd in its finally block, and closing a fabricated number + // would raise EBADF there - a second failure behind the same wrapper, + // hiding which fsync broke. Where opening a directory is not allowed + // (Windows), a sentinel stands in and the close is stubbed for it, so + // the injected fsync failure is the only error in flight on every + // platform. const fsyncFailure = new Error("EIO fsync") + const realClose = fsSyncMod.closeSync let dirFd: number | undefined + let dirFdIsReal = false vi.spyOn(process, "platform", "get").mockReturnValue("linux") vi.spyOn(fsSyncMod, "openSync").mockImplementation(((...args: unknown[]) => { if (String(args[0]) === tempDir) { - dirFd = (realOpen as (p: string, f: string) => number)(tempDir, "r") + try { + dirFd = (realOpen as (p: string, f: string) => number)(tempDir, "r") + dirFdIsReal = true + } catch { + dirFd = 987654 + } return dirFd } return (realOpen as (...a: unknown[]) => unknown)(...args) }) as never) + vi.spyOn(fsSyncMod, "closeSync").mockImplementation(((fd: number) => { + if (fd === dirFd && !dirFdIsReal) { + return undefined + } + return (realClose as (f: number) => unknown)(fd) + }) as never) vi.spyOn(fsSyncMod, "fsyncSync").mockImplementation(((fd: number) => { if (fd === dirFd) { throw fsyncFailure From d5a68d13f3d1b462dc8f48e8ce800b059bfd5f82 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 17 Oct 2026 03:29:40 +0800 Subject: [PATCH 18/23] test(file-safety): inject the durability failure without fabricating a descriptor The Windows CI lane rejects the fabricated-descriptor probe: the openSync/closeSync spy pair around the parent-directory handle behaved differently on the runner than on any local configuration (node 22 and 25, forks and threads, one to many workers, CI=true, contention). The probe now watches the commit rename itself - the parent-directory fsync is the only fsync that runs after it - so the SUT opens and closes its own real handle, no descriptor is invented, and the injected EIO failure is the only error in flight on every platform. The cause assertion and the staging-release assertions are unchanged; removing the injection reddens exactly the durability test. --- src/utils/__tests__/safeWriteJson.test.ts | 43 +++++++---------------- 1 file changed, 12 insertions(+), 31 deletions(-) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index e796006013..8c08b8e9ac 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -705,41 +705,22 @@ describe("safeWriteJson", () => { it("keeps the published target with its new content when the post-commit directory fsync fails", async () => { const fsSyncMod = await import("fs") - const realOpen = fsSyncMod.openSync const realFsync = fsSyncMod.fsyncSync - // The parent-directory open that safeWriteText performs after the commit - // rename gets a REAL descriptor where the platform allows it: the SUT - // closes the fd in its finally block, and closing a fabricated number - // would raise EBADF there - a second failure behind the same wrapper, - // hiding which fsync broke. Where opening a directory is not allowed - // (Windows), a sentinel stands in and the close is stubbed for it, so - // the injected fsync failure is the only error in flight on every - // platform. + // The parent-directory fsync is the only fsync that runs after the + // commit rename. Watching the rename itself - rather than fabricating a + // descriptor - keeps the SUT in charge of opening and closing its own + // real directory handle, so the injected fsync failure is the only + // error in flight on every platform. const fsyncFailure = new Error("EIO fsync") - const realClose = fsSyncMod.closeSync - let dirFd: number | undefined - let dirFdIsReal = false + let committed = false + const actualRename = fsPromisesActuals.rename! + vi.mocked(fs.rename).mockImplementation((async (from: string, to: string) => { + await actualRename(from, to) + committed = true + }) as typeof fs.rename) vi.spyOn(process, "platform", "get").mockReturnValue("linux") - vi.spyOn(fsSyncMod, "openSync").mockImplementation(((...args: unknown[]) => { - if (String(args[0]) === tempDir) { - try { - dirFd = (realOpen as (p: string, f: string) => number)(tempDir, "r") - dirFdIsReal = true - } catch { - dirFd = 987654 - } - return dirFd - } - return (realOpen as (...a: unknown[]) => unknown)(...args) - }) as never) - vi.spyOn(fsSyncMod, "closeSync").mockImplementation(((fd: number) => { - if (fd === dirFd && !dirFdIsReal) { - return undefined - } - return (realClose as (f: number) => unknown)(fd) - }) as never) vi.spyOn(fsSyncMod, "fsyncSync").mockImplementation(((fd: number) => { - if (fd === dirFd) { + if (committed) { throw fsyncFailure } return (realFsync as (f: number) => unknown)(fd) From 5f938837bb3da7337822aea139f2bc69806bf536 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 17 Oct 2026 03:40:59 +0800 Subject: [PATCH 19/23] fix(file-safety): fall back to the link walk only on a dangling link resolveLockKey caught every resolution error and fell back to the bounded readlink walk, so a transient EACCES or EIO from realpath or lstat keyed the write to the alias path while a writer reaching the referent directly keyed the referent - two locks for one file, and the alias/referent serialization the unit promises did not hold. Only ENOENT (the dangling-link case the walk exists for) now falls through; every other error propagates, matching resolvePublishTarget's own policy. Regression test: EACCES from realpath rejects resolveLockKey instead of returning an alias key; removing the guard reddens exactly that test. Also restore the rename/unlink/console spies in a finally block in the cleanup-safety test: a failed assertion previously left the spies active for the afterEach rm and every later test. --- src/services/file-safety/safeWriteText.ts | 9 ++++- .../__tests__/safeWriteJson.lockKey.spec.ts | 38 +++++++++++++++---- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 4326ea2c3a..3a607a73ba 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -218,7 +218,14 @@ async function canonicalDirKey(absoluteFilePath: string): Promise { export async function resolveLockKey(absoluteFilePath: string): Promise { try { return await canonicalDirKey(await resolvePublishTarget(absoluteFilePath)) - } catch { + } catch (error: unknown) { + // Only a dangling link (ENOENT) may fall through to the walk: any other + // resolution error (EACCES, EIO) would key this writer to the alias path + // while a writer reaching the referent directly keys the referent, and + // the two writers would then take different locks for one file. + if (errorCode(error) !== "ENOENT") { + throw error + } // A real readlink throws for anything that is not a link, so a normal chain // ends the walk. Two links that point at each other never would, so the // walk is bounded and callers use the key they actually reached. diff --git a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts index 8e20682a50..89fb3af12c 100644 --- a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts +++ b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts @@ -159,6 +159,23 @@ describe("safeWriteJson lock key under a peer commit", () => { expect(await resolveLockKey(file)).toBe(path.join(canonicalDir, "history_item.json")) }) + + it("propagates a non-ENOENT resolution failure instead of keying on the alias", async () => { + // EACCES from realpath is not a dangling link: falling back to the walk + // would key this writer to the alias path while a writer reaching the + // referent directly keys the referent, and the two writers would take + // different locks for the same file. + const aliasDir = path.join(os.tmpdir(), "alias-dir") + const file = path.join(aliasDir, "history_item.json") + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + mockedRealpath.mockImplementation(async () => { + throw eacces + }) + mockedLstat.mockImplementation( + async () => ({ isSymbolicLink: () => false, isFile: () => true }) as unknown as BigIntStats, + ) + await expect(resolveLockKey(file)).rejects.toBe(eacces) + }) }) it("does not log a cleanup error when the safety net finds the temp file already gone", async () => { @@ -175,12 +192,17 @@ it("does not log a cleanup error when the safety net finds the temp file already const unlinkSpy = vi.spyOn(fs, "unlink").mockRejectedValue(enoent) const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}) - await expect(safeWriteJson(target, { id: "task-1" })).rejects.toThrow("commit rename failed") - - // Only the original failure is reported. - expect(consoleError).toHaveBeenCalledTimes(1) - - renameSpy.mockRestore() - unlinkSpy.mockRestore() - consoleError.mockRestore() + try { + await expect(safeWriteJson(target, { id: "task-1" })).rejects.toThrow("commit rename failed") + + // Only the original failure is reported. + expect(consoleError).toHaveBeenCalledTimes(1) + } finally { + // Restore even when an assertion fails: the afterEach cleanup runs fs.rm + // with these spies still in place otherwise, and later tests inherit a + // rejecting rename. + renameSpy.mockRestore() + unlinkSpy.mockRestore() + consoleError.mockRestore() + } }) From e20939c034b773a99317b9f368dce198ce84951f Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 17 Oct 2026 05:17:02 +0800 Subject: [PATCH 20/23] test(safety): drop the dead lstat mock in the EACCES lock-key test resolvePublishTarget rethrows the realpath error before lstat runs, so the mock could never affect the error-identity assertion (CodeRabbit thread). --- src/utils/__tests__/safeWriteJson.lockKey.spec.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts index 89fb3af12c..025aa898d6 100644 --- a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts +++ b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts @@ -171,9 +171,6 @@ describe("safeWriteJson lock key under a peer commit", () => { mockedRealpath.mockImplementation(async () => { throw eacces }) - mockedLstat.mockImplementation( - async () => ({ isSymbolicLink: () => false, isFile: () => true }) as unknown as BigIntStats, - ) await expect(resolveLockKey(file)).rejects.toBe(eacces) }) }) From 1190af8a57a282062163330ad4b90ba8820ba3c4 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 17 Oct 2026 06:59:31 +0800 Subject: [PATCH 21/23] fix(safety): port copy-based backup and harden caller-staged tempPath Adopts the lineage's evolved safeWriteText (the #1912 copy): the backup is now copied without ever moving the target, closing the crash-loss window between a backup rename and the commit rename (Persistence Integrity row), and a caller-staged tempPath must carry the .new_*.tmp staging convention and still be a single-named regular file when opened, so neither an arbitrary file beside the target nor a hard-link substitution between the check and the open can be published onto the target (Security Boundaries row). Specs are ported with the implementation; the confineTo coverage stays with #1912 (U3 scope). --- .../__tests__/safeWriteText.spec.ts | 843 ++++++++++++++++-- src/services/file-safety/safeWriteText.ts | 488 ++++++++-- .../__tests__/safeWriteJson.lockKey.spec.ts | 19 +- src/utils/__tests__/safeWriteJson.test.ts | 73 +- 4 files changed, 1189 insertions(+), 234 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index bd47ba29e2..f02bb8af2a 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -5,15 +5,17 @@ import type { ChildProcess } from "child_process" import * as path from "path" import { + OrphanedBackupError, PostCommitDurabilityError, resolveLockKey, - RollbackFailureError, safeWriteText, StagingPathError, } from "../safeWriteText" // Full mock for fs/promises — all methods are vi.fn() stubs vi.mock("fs/promises", () => ({ + copyFile: vi.fn(), + chmod: vi.fn(), mkdir: vi.fn(), access: vi.fn(), rename: vi.fn(), @@ -34,6 +36,7 @@ vi.mock("fs", () => ({ fsyncSync: vi.fn(), chmodSync: vi.fn(), fchmodSync: vi.fn(), + fstatSync: vi.fn(), statSync: vi.fn(), Stats: class Stats {}, })) @@ -62,6 +65,28 @@ function _fileStats(isLink: boolean): fsSync.Stats { const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats s.isSymbolicLink = () => isLink s.isFile = () => !isLink + // Single name by default: the post-open verification rejects a staged file that + // has gained a second (hard-link) name. + s.nlink = 1 + return s +} +// Directory stand-in: neither a symlink nor a regular file. The symlink double short-circuits +// on isSymbolicLink(), so only this double reaches the branch that rejects any other file type. +function _notARegularFileStats(): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats + s.isSymbolicLink = () => false + s.isFile = () => false + return s +} + +// fs.BigIntStats is a type-only export (fs.BigIntStats is undefined at runtime), so the stand-in is +// a Stats object carrying bigint ino/dev - exactly what fs.lstat(path, { bigint: true }) hands +// back at runtime. +function _fileStatsWithIdentity(ino: bigint, dev: bigint): fsSync.BigIntStats { + // Double assertion: the runtime value is the Stats stand-in, the type is the bigint variant. + const s = _fileStats(false) as unknown as fsSync.BigIntStats + s.ino = ino + s.dev = dev return s } @@ -78,6 +103,8 @@ function mockDefaults(): void { // Staged-file default: a regular file, not a link, so a caller-supplied // tempPath passes the location and file-type check by default. vi.mocked(fs.lstat).mockResolvedValue(_fileStats(false)) + // Post-open default: the opened file is the same single-named regular file. + vi.mocked(fsSync.fstatSync).mockReturnValue(_fileStats(false)) } function _stats(mode: number): fsSync.Stats { const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats @@ -180,7 +207,7 @@ describe("safeWriteText", () => { it("does not remove the staging directory when the caller supplies its own tempPath", async () => { const targetPath = "/tmp/test-dir/target.txt" - const callerTemp = "/tmp/test-dir/caller-staged.txt" + const callerTemp = "/tmp/test-dir/.new_caller-staged.tmp" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) @@ -191,20 +218,75 @@ describe("safeWriteText", () => { expect(fs.rmdir).not.toHaveBeenCalled() }) - it("a failed staging-dir removal never fails the committed write", async () => { + it("retries a failed staging-dir removal, never fails the committed write, and reports the exact path", async () => { const targetPath = "/tmp/test-dir/target.txt" + const warnings: string[] = [] vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) vi.mocked(fs.rmdir).mockRejectedValue(Object.assign(new Error("ENOTEMPTY"), { code: "ENOTEMPTY" })) - await expect(safeWriteText(targetPath, "hello", { platform: "linux" })).resolves.toBeUndefined() + await expect( + safeWriteText(targetPath, "hello", { platform: "linux", onWarning: (m) => warnings.push(m) }), + ).resolves.toBeUndefined() - // the commit rename still happened and the rmdir error was swallowed + // the commit rename still happened, and the removal was retried once expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + expect(fs.rmdir).toHaveBeenCalledTimes(2) + const stagingDirs = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => String(call[0])) + expect(stagingDirs.length).toBe(1) + expect(fs.rmdir).toHaveBeenNthCalledWith(1, stagingDirs[0]) + expect(fs.rmdir).toHaveBeenNthCalledWith(2, stagingDirs[0]) + // the leftover is not swallowed: the exact directory path reaches the warning sink + const reported = warnings.filter((m) => m.includes(stagingDirs[0])) + expect(reported).toHaveLength(1) + expect(reported[0]).toContain("could not remove the staging directory") + }) + + it("treats a staging directory that someone else already removed as a completed cleanup", async () => { + // ENOENT from the rmdir is the goal, not a leftover: reporting it would send the reader + // looking for a directory that is already gone. + const targetPath = "/tmp/test-dir/target.txt" + const warnings: string[] = [] + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rmdir).mockRejectedValue( + Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }), + ) + + await expect( + safeWriteText(targetPath, "hello", { platform: "linux", onWarning: (m) => warnings.push(m) }), + ).resolves.toBeUndefined() + expect(fs.rmdir).toHaveBeenCalledTimes(1) - const stagingDir = String(vi.mocked(fsSync.mkdirSync).mock.calls[0]?.[0]) - expect(stagingDir).toContain(".file-safety-staging") - expect(fs.rmdir).toHaveBeenCalledWith(stagingDir) + // The removal that reported ENOENT has to be the one aimed at this write's own + // staging directory: a cleanup that removed some other directory and got + // ENOENT is not this contract, and it would leave this directory behind. + const stagingDirs = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => String(call[0])) + expect(stagingDirs.length).toBe(1) + expect(fs.rmdir).toHaveBeenCalledWith(stagingDirs[0]) + expect(warnings).toEqual([]) + }) + + it("reports a staging directory it could not remove after a failed write, without replacing the write error", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const warnings: string[] = [] + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The commit rename fails, and the post-failure cleanup then fails twice. + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + vi.mocked(fs.rmdir).mockRejectedValue(Object.assign(new Error("EPERM"), { code: "EPERM" })) + + await expect( + safeWriteText(targetPath, "data", { platform: "linux", onWarning: (m) => warnings.push(m) }), + ).rejects.toThrow("ENOSPC") + + const stagingDirs = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => String(call[0])) + expect(stagingDirs.length).toBe(1) + expect(fs.rmdir).toHaveBeenCalledTimes(2) + expect(fs.rmdir).toHaveBeenNthCalledWith(2, stagingDirs[0]) + const reported = warnings.filter((m) => m.includes(stagingDirs[0])) + expect(reported).toHaveLength(1) + expect(reported[0]).toContain("could not remove the staging directory") }) it("gives each self-staged write its own staging directory so a concurrent write cannot remove it", async () => { @@ -259,7 +341,7 @@ describe("safeWriteText", () => { it("does not remove a staging directory it did not create when a caller-staged write fails", async () => { const targetPath = "/tmp/test-dir/target.txt" - const callerTemp = "/tmp/test-dir/caller-staged.txt" + const callerTemp = "/tmp/test-dir/.new_caller-staged.tmp" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) vi.mocked(fs.rename).mockRejectedValue(Object.assign(new Error("EACCES"), { code: "EACCES" })) @@ -275,6 +357,36 @@ describe("safeWriteText", () => { }) }) + it("win32: a rejecting async onWarning does not abort the write or leak an unhandled rejection", async () => { + // TypeScript accepts an async sink where a void callback is expected, so the + // wrapper has to attach a handler to the returned promise: an unhandled + // rejection can end the process under Node's default mode, after a write that + // already succeeded. + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await expect( + safeWriteText(targetPath, "data", { + platform: "win32", + onWarning: async () => { + throw new Error("async sink down") + }, + }), + ).resolves.toBeUndefined() + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + // The rejection is reported through the fallback sink rather than surfacing as an + // unhandled rejection. + expect(consoleWarn).toHaveBeenCalledWith(expect.stringContaining("onWarning callback rejected")) + consoleWarn.mockRestore() + }) + // ── Test 2: fsync ordering ─────────────────────────────────────────────── describe("fsync ordering", () => { @@ -341,15 +453,13 @@ describe("safeWriteText", () => { await safeWriteText(targetPath, "data", { backup: true, platform: "linux" }) - // the commit rename (temp -> target) still happened - expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + // the commit rename (temp -> target) still happened; it is the only rename + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) // the failing cleanup was the post-commit backup unlink expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) - // no rollback rename: the committed target is not restored from the backup - expect(fs.rename).toHaveBeenCalledTimes(2) - // the staging temp was already committed by the rename; nothing // temp-shaped is unlinked afterwards expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) @@ -370,18 +480,62 @@ describe("safeWriteText", () => { PostCommitDurabilityError, ) - // The commit rename already published the new content, so the backup must - // not be renamed back over it: only target->backup and temp->target run. - expect(fs.rename).toHaveBeenNthCalledWith(1, targetPath, expect.stringContaining("safeWriteText.bak_")) - expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) - expect(fs.rename).toHaveBeenCalledTimes(2) + // The commit rename already published the new content, and the backup was only + // ever a copy: the target was never moved, so there is nothing to rename back. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // The durability failure is reported, not swallowed - and the backup copy is not + // left beside the target where no caller could find it. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("does not let the failure cleanup unlink the file the commit already published", async () => { + // A tiny filesystem so that "the target is still there" is a STATE assertion: a cleanup + // that deleted the published file and a later step that recreated it would both satisfy + // a weaker "the content is at the target" check. + const targetPath = "/tmp/test-dir/target.txt" + const dirPath = path.dirname(targetPath) + const files = new Set([targetPath]) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.rename).mockImplementation(async (from: unknown, to: unknown) => { + files.delete(String(from)) + files.add(String(to)) + }) + vi.mocked(fs.unlink).mockImplementation(async (p: unknown) => { + files.delete(String(p)) + }) + // The post-commit parent-directory fsync is the failure under test. + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) throw new Error("EBADF") + return 1 + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow( + PostCommitDurabilityError, + ) + + expect(files.has(targetPath)).toBe(true) + // The staged path is the committed file from the rename onwards: the cleanup must not + // reach for it at all. Before the commit point was tracked, every post-commit + // failure (directory fsync, DACL restore, a throwing warning sink) ran the temp + // unlink against the name the target now lives under. + const stagedPath = String(vi.mocked(fs.rename).mock.calls[0][0]) + expect( + vi.mocked(fs.unlink).mock.calls.filter(function (call) { + return String(call[0]) === stagedPath + }), + ).toHaveLength(0) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(fs.rename).toHaveBeenCalledTimes(1) }) }) - // ── Test 4: backup:true keeps old safeWriteJson semantics incl. rollback ── + // ── Test 4: backup:true keeps old safeWriteJson semantics, copy-based ── describe("backup:true", () => { - it("renames target -> backup before commit, deletes backup on success", async () => { + it("copies target -> backup before commit without moving the target, deletes the copy on success", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) @@ -391,68 +545,139 @@ describe("safeWriteText", () => { // target was accessed (exists check) expect(fs.access).toHaveBeenCalledWith(targetPath) - // first rename: target -> backup - expect(fs.rename).toHaveBeenNthCalledWith(1, targetPath, expect.stringContaining("safeWriteText.bak_")) + // The backup is a copy: the canonical target is never moved away, so readers + // never see a missing file and no later step can clobber a concurrent publish. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.rename).not.toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) - // second rename: temp -> target (realpath mock returns targetPath) - expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + // the only rename is the atomic commit temp -> target + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) - // backup was deleted on success + // backup copy was deleted on success expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) }) - it("rollback: on failure after rename target->backup, restores backup to target", async () => { + it("a failed commit does not move the target, so nothing has to be rolled back", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) - // first rename (target->backup) succeeds, second fails - let callCount = 0 - vi.mocked(fs.rename).mockImplementation(async () => { - callCount++ - if (callCount === 1) return // target -> backup - if (callCount === 2) throw new Error("ENOSPC") // temp -> target fails - return // the rollback rename succeeds - }) + // The commit rename is the only rename in the flow and it fails. + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow("ENOSPC") - // rollback rename is the 3rd call (after target->backup and temp->target failure) - expect(fs.rename).toHaveBeenNthCalledWith(3, expect.stringContaining("safeWriteText.bak_"), targetPath) + // The target never left its path, so there is no restore rename and the + // pre-write content is still what a reader sees at targetPath. + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) - // temp was cleaned up on failure + // Both the backup copy and the staging temp are cleaned up on failure. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) }) - it("a failed rollback reports the partial state, not only the publish error", async () => { - // The content is still on disk, but only at the backup path. A caller that gets - // just the publish error has data it cannot find at the expected path. + it("creates the backup privately before its content exists, then fsyncs it", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) - let callCount = 0 - vi.mocked(fs.rename).mockImplementation(async () => { - callCount++ - if (callCount === 1) return // target -> backup - if (callCount === 2) throw new Error("ENOSPC") // temp -> target fails - throw new Error("EACCES") // the rollback rename fails too + + await safeWriteText(targetPath, "new data", { backup: true }) + + // The destination must exist with a private mode before copyFile writes anything + // into it: copyFile chooses the destination mode itself, so a restrictive target + // could otherwise leave a group/world-readable copy that a later chmod cannot + // undo. "wx" also means a pre-existing path is never silently reused. + const seedOpen = vi.mocked(fsSync.openSync).mock.calls.find(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "wx" }) + expect(seedOpen).toBeDefined() + expect(seedOpen?.[2]).toBe(0o600) + const seedOrder = vi.mocked(fsSync.openSync).mock.invocationCallOrder[ + vi.mocked(fsSync.openSync).mock.calls.indexOf(seedOpen!) + ] + expect(seedOrder).toBeLessThan(vi.mocked(fs.copyFile).mock.invocationCallOrder[0]) + + // The chmod keeps a copied read-only attribute (Windows) from breaking the fsync + // open, and keeps a backup of a permissive file private. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.chmod).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_"), 0o600) + expect(vi.mocked(fs.chmod).mock.invocationCallOrder[0]).toBeGreaterThan( + vi.mocked(fs.copyFile).mock.invocationCallOrder[0], + ) - let failure: RollbackFailureError | undefined - await safeWriteText(targetPath, "new data", { backup: true }).catch((e: unknown) => { - if (e instanceof RollbackFailureError) { - failure = e - return + // The copy is then opened for fsync with the writable flag. + const backupOpen = vi.mocked(fsSync.openSync).mock.calls.find(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "r+" + }) + expect(backupOpen).toBeDefined() + }) + + it("a failed backup flush is reported and leaves no partial backup behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The copy lands, but the fsync of the copy fails: the retained content is not + // known to be durable, so the write must not proceed on a half-written backup. + // The staged temp is fsynced earlier with a different handle, so target the + // backup's fd specifically. + vi.mocked(fsSync.openSync).mockImplementation((p: unknown) => + String(p).includes("safeWriteText.bak_") ? 7 : 1, + ) + vi.mocked(fsSync.fsyncSync).mockImplementation((fd: unknown) => { + if (fd === 7) { + throw new Error("EIO") } - throw e }) - expect(failure).toBeInstanceOf(RollbackFailureError) - expect(failure?.publishError).toBeInstanceOf(Error) - expect((failure?.publishError as Error).message).toBe("ENOSPC") - expect((failure?.rollbackError as Error).message).toBe("EACCES") - expect(failure?.backupPath).toContain("safeWriteText.bak_") - // The backup is what the caller can still recover, so it must stay on disk. - expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow( + "EIO", + ) + + // Nothing was published, and the incomplete copy is removed rather than left + // next to the target looking like a usable backup. + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("carries the leftover path when the partial backup cannot be removed either", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockImplementation((p: unknown) => + String(p).includes("safeWriteText.bak_") ? 7 : 1, + ) + vi.mocked(fsSync.fsyncSync).mockImplementation((fd: unknown) => { + if (fd === 7) { + throw new Error("EIO") + } + }) + let unlinkAttempts = 0 + vi.mocked(fs.unlink).mockImplementation(async (p: unknown) => { + if (String(p).includes("safeWriteText.bak_")) { + unlinkAttempts++ + } + throw Object.assign(new Error("EPERM"), { code: "EPERM" }) + }) + + // The copy failed and the locked leftover could not be unlinked even after the + // retry. Dropping the path would leave a partial copy of the previous content on + // disk with no reference to it anywhere, so the error carries it. + const error = await safeWriteText(targetPath, "new data", { + backup: true, + platform: "linux", + }).catch((caught: unknown) => caught) + + expect(error).toBeInstanceOf(OrphanedBackupError) + const orphan = error as OrphanedBackupError + expect(orphan.orphanedBackupPath).toContain("safeWriteText.bak_") + expect(orphan.message).toContain("safeWriteText.bak_") + expect(orphan.message).toContain("EPERM") + expect(orphan.originalError).toBeInstanceOf(Error) + expect((orphan.originalError as Error).message).toBe("EIO") + expect(orphan.cause).toBe(orphan.originalError) + expect(unlinkAttempts).toBe(2) + expect(fs.rename).not.toHaveBeenCalled() }) it("backup:true when target does not exist: no backup created, just commit", async () => { @@ -478,6 +703,240 @@ describe("safeWriteText", () => { }) }) + it("retries a failed post-commit backup cleanup once and reports the leftover path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const onWarning = vi.fn() + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + // Step 6 removes the backup copy after the commit; the unlink keeps failing. + vi.mocked(fs.unlink).mockRejectedValue(new Error("EPERM")) + + await safeWriteText(targetPath, "data", { backup: true, platform: "linux", onWarning }) + + // The publish succeeded, so the write still resolves - but the leftover copy of the + // previous content must be retried once and then reported with its path, never dropped. + const backupPath = vi.mocked(fs.copyFile).mock.calls[0][1] + const backupUnlinks = vi.mocked(fs.unlink).mock.calls.filter(function (call) { + return String(call[0]).includes("safeWriteText.bak") + }) + expect(backupUnlinks.length).toBe(2) + expect( + backupUnlinks.map(function (call) { + return call[0] + }), + ).toEqual([backupPath, backupPath]) + expect(onWarning).toHaveBeenCalledTimes(1) + expect(String(onWarning.mock.calls[0][0])).toContain(String(backupPath)) + }) + + it("reports the exact orphan paths when cleanup fails after a failed write, keeping the write error", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const onWarning = vi.fn() + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + // The commit rename fails, and every cleanup unlink fails as well. + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + vi.mocked(fs.unlink).mockRejectedValue(new Error("EACCES")) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux", onWarning })).rejects.toThrow( + "ENOSPC", + ) + + const backupPath = vi.mocked(fs.copyFile).mock.calls[0][1] + const tempArg = vi.mocked(fs.rename).mock.calls[0][0] + // Both leftovers are retried once, and each leftover is reported with its exact path + // instead of being dropped - the caller still gets the original write error. + expect( + vi.mocked(fs.unlink).mock.calls.filter(function (call) { + return String(call[0]) === String(backupPath) + }).length, + ).toBe(2) + expect( + vi.mocked(fs.unlink).mock.calls.filter(function (call) { + return String(call[0]) === String(tempArg) + }).length, + ).toBe(2) + expect(onWarning).toHaveBeenCalledTimes(2) + const messages = onWarning.mock.calls.map(function (call) { + return String(call[0]) + }) + expect( + messages.some(function (m) { + return m.includes(String(backupPath)) + }), + ).toBe(true) + expect( + messages.some(function (m) { + return m.includes(String(tempArg)) + }), + ).toBe(true) + }) + + it("treats an already-gone staging temp as a completed cleanup, not as a leftover", async () => { + // ENOENT from the temp unlink means the goal is already met. Retrying it and then + // warning claimed an orphan that did not exist - a false alarm that trains the reader + // to ignore the leftover reports that matter. + const targetPath = "/tmp/test-dir/target.txt" + const onWarning = vi.fn() + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + vi.mocked(fs.unlink).mockRejectedValue( + Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }), + ) + + await expect(safeWriteText(targetPath, "data", { platform: "linux", onWarning })).rejects.toThrow("ENOSPC") + + // ENOENT short-circuits: the goal is met, so there is nothing to retry and + // nothing to report. + expect(fs.unlink).toHaveBeenCalledTimes(1) + expect(onWarning).not.toHaveBeenCalled() + }) + + it("still reports a staging temp that could not be removed for any other reason", async () => { + // The tolerance is scoped to ENOENT. EACCES means the staged file is still on disk, and + // swallowing it would turn a false alarm into a false silence. + const targetPath = "/tmp/test-dir/target.txt" + const onWarning = vi.fn() + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + vi.mocked(fs.unlink).mockRejectedValue( + Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }), + ) + + await expect(safeWriteText(targetPath, "data", { platform: "linux", onWarning })).rejects.toThrow("ENOSPC") + + const tempArg = vi.mocked(fs.rename).mock.calls[0][0] + expect(onWarning).toHaveBeenCalledTimes(1) + expect(String(onWarning.mock.calls[0][0])).toContain("could not remove the staging temp") + expect(String(onWarning.mock.calls[0][0])).toContain(String(tempArg)) + expect(String(onWarning.mock.calls[0][0])).toContain("EACCES") + }) + + it("treats an already-gone backup copy as removed instead of reporting a phantom orphan", async () => { + // Same rule on the backup copy: ENOENT means there is nothing left to recover by hand, + // so the warning would point at a file that is not there. + const targetPath = "/tmp/test-dir/target.txt" + const onWarning = vi.fn() + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fs.unlink).mockImplementation(async (p: unknown) => { + if (String(p).includes("safeWriteText.bak_")) { + throw enoent + } + }) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux", onWarning })).rejects.toThrow( + "ENOSPC", + ) + + const backupPath = String(vi.mocked(fs.copyFile).mock.calls[0][1]) + expect( + onWarning.mock.calls.filter(function (call) { + return String(call[0]).includes(backupPath) + }), + ).toHaveLength(0) + }) + + it("retries a failed DACL-dump cleanup once and reports the leftover dump path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const onWarning = vi.fn() + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + // The dump unlink keeps failing. The published file is fine, but the dump is a text copy + // of the target's access rights that only this write knew the name of. + vi.mocked(fs.unlink).mockRejectedValue(new Error("EPERM")) + + await safeWriteText(targetPath, "data", { platform: "win32", onWarning }) + + const dumpUnlinks = vi.mocked(fs.unlink).mock.calls.filter(function (call) { + return String(call[0]).includes("safeWriteText.acl") + }) + expect(dumpUnlinks.length).toBe(2) + expect( + dumpUnlinks.map(function (call) { + return call[0] + }), + ).toEqual([dumpUnlinks[0][0], dumpUnlinks[0][0]]) + // Reported once, with the exact path, and the write still resolved. + const messages = onWarning.mock.calls.map(function (call) { + return String(call[0]) + }) + const dumpReports = messages.filter(function (m) { + return m.includes("could not remove the DACL dump") + }) + expect(dumpReports.length).toBe(1) + expect(dumpReports[0]).toContain(String(dumpUnlinks[0][0])) + }) + + it("reports the DACL dump path after a failed write without replacing the write error", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const onWarning = vi.fn() + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + vi.mocked(fs.unlink).mockRejectedValue(new Error("EPERM")) + + await expect(safeWriteText(targetPath, "data", { platform: "win32", onWarning })).rejects.toThrow("ENOSPC") + + const dumpUnlinks = vi.mocked(fs.unlink).mock.calls.filter(function (call) { + return String(call[0]).includes("safeWriteText.acl") + }) + // The commit span owns the dump removal, so the failure path must not retry the same file + // and report the same leftover twice. + expect(dumpUnlinks.length).toBe(2) + const messages = onWarning.mock.calls.map(function (call) { + return String(call[0]) + }) + expect( + messages.filter(function (m) { + return m.includes("could not remove the DACL dump") + }).length, + ).toBe(1) + expect( + messages.some(function (m) { + return m.includes("safeWriteText.acl") + }), + ).toBe(true) + }) + + it("backup:true closes the seed descriptor even when the copy fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + // Distinct descriptors, and the backup's own id recorded, so the assertion below cannot be + // satisfied by the close of some other descriptor. + let nextFd = 40 + const backupFds: number[] = [] + vi.mocked(fsSync.openSync).mockImplementation(function (p) { + const id = nextFd++ + if (String(p).includes("safeWriteText.bak")) backupFds.push(id) + return id + }) + // The copy is the statement that sits between the open and the close. + vi.mocked(fs.copyFile).mockRejectedValue(new Error("EIO: copy failed")) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow("EIO") + + expect(backupFds.length).toBe(1) + // Every successful openSync gets a close attempt, including this path, because an open + // descriptor holds the backup file and blocks the cleanup below. + expect(fsSync.closeSync).toHaveBeenCalledWith(backupFds[0]) + // And the half-made backup does not outlive the attempt. + expect( + vi.mocked(fs.unlink).mock.calls.some(function (call) { + return String(call[0]).includes("safeWriteText.bak") + }), + ).toBe(true) + }) + // ── Test 5: win32 DACL path ────────────────────────────────────────────── describe("win32 DACL", () => { @@ -522,6 +981,65 @@ describe("safeWriteText", () => { expect(execFile).toHaveBeenCalledTimes(1) }) + it("win32: reports that access rights may change when the DACL cannot be saved", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + const warnings: string[] = [] + + await safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }) + + // The write still commits - a failing icacls must not leave the user unable to save - + // but the caller is told the replacement may not carry the old ACL. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + expect(warnings.filter((m) => m.includes("different access rights"))).toHaveLength(1) + }) + + it("win32: reports when the target cannot be checked for DACL preservation", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The target exists but is not readable: that is not "absent", and skipping DACL + // preservation has to be visible. + vi.mocked(fs.access).mockImplementation(async (p) => { + if (String(p) === targetPath) { + throw Object.assign(new Error("EACCES"), { code: "EACCES" }) + } + }) + const warnings: string[] = [] + + await safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }) + + expect(execFile).not.toHaveBeenCalled() + expect(warnings.filter((m) => m.includes("Could not check"))).toHaveLength(1) + }) + + // Warning delivery is advisory: it must not be able to fail the save it is reporting on. + it("win32: a throwing onWarning does not abort the write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + + await expect( + safeWriteText(targetPath, "data", { + platform: "win32", + onWarning: () => { + throw new Error("callback down") + }, + }), + ).resolves.toBeUndefined() + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + }) + it("win32 DACL: a partial dump left by a failed save is removed and never restored", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) @@ -574,32 +1092,34 @@ describe("safeWriteText", () => { expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) }) - it("win32 DACL save runs before the backup rename, not after it", async () => { + it("win32 DACL save runs before the backup copy, not after it", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) // The title is about order, so assert the order the mocks were actually - // called in. If the save ran after the backup rename the target would - // already be gone and the dump would describe the wrong file. + // called in. If the save ran after the backup copy the dump could describe a + // file that a concurrent publish had already replaced. await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) const callOrder = vi.mocked(execFile).mock.invocationCallOrder + const copyOrder = vi.mocked(fs.copyFile).mock.invocationCallOrder const renameOrder = vi.mocked(fs.rename).mock.invocationCallOrder const saveCall = callOrder[0] const restoreCall = callOrder[1] - const backupRename = renameOrder[0] - const commitRename = renameOrder[1] + const backupCopy = copyOrder[0] + const commitRename = renameOrder[0] - expect(saveCall).toBeLessThan(backupRename) - expect(backupRename).toBeLessThan(commitRename) + expect(saveCall).toBeLessThan(backupCopy) + expect(backupCopy).toBeLessThan(commitRename) expect(commitRename).toBeLessThan(restoreCall) }) - it("win32 DACL: dump is unlinked even when restore fails", async () => { + it("win32 DACL: a failed restore is reported and the dump is still unlinked", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) // icacls save succeeds, restore fails let callCount = 0 @@ -613,10 +1133,15 @@ describe("safeWriteText", () => { await safeWriteText(targetPath, "data", { platform: "win32" }) - // write succeeded despite restore failure (best-effort) + // The content did commit: failing here would break every publish on a machine + // where icacls cannot reapply the saved ACEs. expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) expect(fs.rename).toHaveBeenCalledTimes(1) + // The changed access rights are reported instead of being swallowed. + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("could not be restored")) + warnSpy.mockRestore() + // dump file was still unlinked in finally expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) }) @@ -640,6 +1165,46 @@ describe("safeWriteText", () => { // no dump file created or unlinked expect(fs.unlink).not.toHaveBeenCalled() }) + + it("reports a partial DACL dump that the failed capture could not remove", async () => { + // A failed icacls /save can leave a partial dump behind. Dropping the path with a + // catch(() => {}) meant a transient EPERM - antivirus, a handle still being released - + // left a concrete file beside the target whose name nothing could recover. + const targetPath = "/tmp/test-dir/target.txt" + const onWarning = vi.fn() + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The capture itself fails, so nothing may be restored onto the committed file. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") { + cb(new Error("icacls save error"), "", "") + } + return fakeChild + }) + vi.mocked(fs.unlink).mockRejectedValue( + Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }), + ) + + await safeWriteText(targetPath, "data", { platform: "win32", onWarning }) + + // The write still proceeds: a failing icacls must not leave the user unable to save. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + // The partial dump is retried and then reported with its exact path. + const dumpUnlinks = vi.mocked(fs.unlink).mock.calls.filter(function (call) { + return String(call[0]).includes("safeWriteText.acl") + }) + expect(dumpUnlinks).toHaveLength(2) + expect( + onWarning.mock.calls.map(function (call) { + return String(call[0]) + }), + ).toEqual( + expect.arrayContaining([ + expect.stringContaining("could not remove the DACL dump"), + expect.stringContaining("safeWriteText.acl"), + ]), + ) + }) }) // ── Test 6: pre-written temp path (tempPath option) ────────────────────── @@ -650,7 +1215,7 @@ describe("safeWriteText", () => { vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) - const customTempPath = "/tmp/test-dir/custom-temp.tmp" + const customTempPath = "/tmp/test-dir/.new_custom-temp.tmp" // platform:linux skips DACL entirely so this test focuses on tempPath only await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) @@ -677,7 +1242,7 @@ describe("safeWriteText", () => { vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) vi.mocked(fsSync.openSync).mockReturnValue(2) - const customTempPath = "/tmp/test-dir/custom-temp.tmp" + const customTempPath = "/tmp/test-dir/.new_custom-temp.tmp" await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) @@ -697,7 +1262,7 @@ describe("safeWriteText", () => { }) vi.mocked(fsSync.openSync).mockReturnValue(2) - const customTempPath = "/tmp/test-dir/custom-temp.tmp" + const customTempPath = "/tmp/test-dir/.new_custom-temp.tmp" await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) @@ -715,7 +1280,7 @@ describe("safeWriteText", () => { }) vi.mocked(fsSync.openSync).mockReturnValue(2) - const customTempPath = "/tmp/test-dir/custom-temp.tmp" + const customTempPath = "/tmp/test-dir/.new_custom-temp.tmp" // A target that cannot be stat'd is not a fresh target: publishing with // the default mode would widen a restrictive target through the rename. @@ -747,7 +1312,7 @@ describe("safeWriteText", () => { vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o444)) vi.mocked(fsSync.openSync).mockReturnValue(3) - const customTempPath = "/tmp/test-dir/custom-temp.tmp" + const customTempPath = "/tmp/test-dir/.new_custom-temp.tmp" await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) @@ -1075,35 +1640,135 @@ describe("caller-supplied staging path", () => { expect(fsSync.openSync).not.toHaveBeenCalled() expect(fs.rename).not.toHaveBeenCalled() }) + + it("rejects a staging path that is a directory rather than a regular file", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const supplied = "/tmp/test-dir/staging-dir" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // The symlink double above short-circuits on isSymbolicLink(), so the "another file + // type" branch is only reachable through a path that is neither link nor file. A + // directory would otherwise be renamed over the target, publishing a directory in + // place of the file. + vi.mocked(fs.lstat).mockResolvedValue(_notARegularFileStats()) + + await expect(safeWriteText(targetPath, "data", { tempPath: supplied, platform: "linux" })).rejects.toThrow( + "Staging file must be a regular file, not another file type", + ) + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() + // The rejection names the path the caller has to clean up. + await expect( + safeWriteText(targetPath, "data", { tempPath: supplied, platform: "linux" }), + ).rejects.toMatchObject({ stagingPath: path.resolve(supplied) }) + }) + + it("rejects a staging path that is the target itself", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // Same inode and device for the supplied staging path and the target: the + // failure handler would unlink the only copy of the content, so a failed + // write would delete the file it was meant to protect. + const stats = _fileStatsWithIdentity(42n, 7n) + vi.mocked(fs.lstat).mockResolvedValue(stats) + + await expect(safeWriteText(targetPath, "data", { tempPath: targetPath, platform: "linux" })).rejects.toThrow( + StagingPathError, + ) + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + // The comparison is only sound when both stats are read as bigint: on NTFS/ReFS the file + // identifiers exceed Number.MAX_SAFE_INTEGER. + // Filter on the options, not the spelling: path.resolve prefixes a drive letter on Windows, + // so the two identity reads are the calls that asked for options at all. + const identityLookups = vi.mocked(fs.lstat).mock.calls.filter((c) => c[1] !== undefined) + expect(identityLookups.length).toBeGreaterThanOrEqual(2) + for (const c of identityLookups) { + expect(c[1]).toEqual({ bigint: true }) + } + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("rejects a caller-staged file that does not carry the staging name convention", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // An arbitrary regular file beside the target passes the location, file-type and + // target-identity checks, yet renaming it onto the target would publish an + // unrelated file's content and delete its original name. Only the staging name + // convention this library's own writers use may be published. + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/test-dir/credentials.json", platform: "linux" }), + ).rejects.toThrow("Staging file must carry the staging name convention") + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("rejects a caller-staged file that gained a second hard link before the open", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // Between the lstat check and the open, an attacker who can write the directory + // can replace the staging path with a hard link to another file in it: the + // opened inode then carries a second name, and the commit rename would publish + // that other file and delete its original name. + const linked = _fileStats(false) + linked.nlink = 2 + vi.mocked(fsSync.fstatSync).mockReturnValue(linked) + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/test-dir/.new_x_1.tmp", platform: "linux" }), + ).rejects.toThrow("Staging file changed between the check and the open") + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("rejects when the target identity cannot be compared for a reason other than a missing target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // A hard-linked staging file shares the target's inode, so the identity comparison is the only thing + // between this write and a rename onto the very file the guard protects. An EACCES from the target + // lstat must not be mistaken for "there is no target". + const stagingStats = _fileStatsWithIdentity(42n, 7n) + vi.mocked(fs.lstat).mockImplementation(async (p) => { + if (String(p) === targetPath) { + throw Object.assign(new Error("EACCES"), { code: "EACCES" }) + } + return stagingStats + }) + + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/test-dir/hardlink.txt", platform: "linux" }), + ).rejects.toThrow("Staging file could not be compared with the target") + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + // Both identity reads must ask for bigint stats, or the comparison silently falls + // back to rounded numbers on NTFS/ReFS. + const identityLookups = vi.mocked(fs.lstat).mock.calls.filter((c) => c[1] !== undefined) + expect(identityLookups).toHaveLength(2) + for (const c of identityLookups) { + expect(c[1]).toEqual({ bigint: true }) + } + }) }) -describe("cleanup before a rollback failure is reported", () => { +describe("cleanup when a backed-up write fails before commit", () => { beforeEach(() => mockDefaults()) - it("releases the staged file and its own staging directory before throwing", async () => { + it("releases the staged file, its copy and its own staging directory before throwing", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) - let callCount = 0 - vi.mocked(fs.rename).mockImplementation(async () => { - callCount++ - if (callCount === 1) return // target -> backup - if (callCount === 2) throw new Error("ENOSPC") // temp -> target fails - throw new Error("EACCES") // the rollback rename fails too - }) + // The commit rename is the only rename in this flow and it fails. + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) - await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow( - RollbackFailureError, - ) + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow("ENOSPC") - // The backup is what the caller can still recover, so it stays on disk; the - // staging file and this write's own directory must not leak alongside it. + // The staging file and this write's own directory must not leak, and neither may + // the backup copy: the target still holds the pre-write content on disk. expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) const stagingDirs = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => String(call[0])) expect(stagingDirs.length).toBe(1) expect(fs.rmdir).toHaveBeenCalledWith(stagingDirs[0]) - const failingRenameOrder = vi.mocked(fs.rename).mock.invocationCallOrder[2] + const failingRenameOrder = vi.mocked(fs.rename).mock.invocationCallOrder[0] const unlinkOrder = vi.mocked(fs.unlink).mock.invocationCallOrder[0] const rmdirOrder = vi.mocked(fs.rmdir).mock.invocationCallOrder[0] expect(unlinkOrder).toBeGreaterThan(failingRenameOrder) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 3a607a73ba..270e16f8dc 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -5,10 +5,12 @@ import { execFile } from "child_process" export interface SafeWriteTextOptions { /** - * When true, preserve the old-file semantics: rename target -> backup first, - * after commit rename delete the backup; on failure roll the backup back to - * the target path. When false (default) the atomic rename simply replaces - * the target -- crash-safe window is zero. + * When true, keep the old-file semantics without ever removing the target: the + * previous content is copied to a hidden backup path and flushed before the + * commit rename, the commit rename atomically replaces the target, and on success + * the backup copy is deleted. A failure before the commit leaves the target + * untouched (there is nothing to roll back) and removes the backup copy. When + * false (default) the atomic rename simply replaces the target. */ backup?: boolean @@ -25,6 +27,14 @@ export interface SafeWriteTextOptions { */ execFileRunner?: typeof execFile + /** + * Sink for non-fatal safety notices. A Windows DACL that could not be captured means the + * committed file may inherit different access rights: the write still proceeds (a missing or + * failing icacls must not block saving), but the caller is told instead of the change being + * silent. Defaults to console.warn. + */ + onWarning?: (message: string) => void + /** * Pre-written temp path to use for the commit phase. When provided, * safeWriteText skips creating its own staging file and uses this path @@ -34,29 +44,6 @@ export interface SafeWriteTextOptions { tempPath?: string } -/** - * A publish that failed and whose rollback also failed: the content survives only - * at the backup path, not at the canonical target. The publish failure stays the - * cause, and the rollback failure plus the backup location travel with the error so - * the caller can tell what it is looking at. - */ -export class RollbackFailureError extends Error { - readonly publishError: unknown - readonly rollbackError: unknown - readonly backupPath: string - - constructor(publishError: unknown, rollbackError: unknown, backupPath: string) { - super( - `Publish failed (${publishError instanceof Error ? publishError.message : String(publishError)}) and the backup could not be restored to its original path -- the content is preserved at the backup location reported on this error.`, - { cause: publishError }, - ) - this.name = "RollbackFailureError" - this.publishError = publishError - this.rollbackError = rollbackError - this.backupPath = backupPath - } -} - /** * A caller-supplied staging path that is not a file this write may publish: it * sits outside the target's directory (so the commit rename would cross @@ -91,6 +78,34 @@ export class PostCommitDurabilityError extends Error { this.targetPath = targetPath } } + +/** + * The backup copy could not be created AND the partial copy could not be removed. + * The write failed either way, but the leftover is a copy of the previous content that + * is still on disk: its path travels on the error so the caller can remove it, instead + * of the cleanup silently discarding the only reference to it. + */ +export class OrphanedBackupError extends Error { + readonly orphanedBackupPath: string + readonly originalError: unknown + readonly cleanupError: unknown + constructor(backupPath: string, targetPath: string, cause: unknown, cleanupError: unknown) { + super(_orphanedBackupMessage(targetPath, backupPath, cause, cleanupError), { cause }) + this.name = "OrphanedBackupError" + this.orphanedBackupPath = backupPath + this.originalError = cause + this.cleanupError = cleanupError + } +} + +function _orphanedBackupMessage(targetPath: string, backupPath: string, cause: unknown, cleanupError: unknown): string { + const reason = (error: unknown): string => (error instanceof Error ? error.message : String(error)) + return ( + `safeWriteText: could not create the backup of ${targetPath} (${reason(cause)}), and the ` + + `partial copy at ${backupPath} could not be removed (${reason(cleanupError)}). The copy is ` + + `still on disk and must be deleted.` + ) +} // -- helpers --------------------------------------------------------------- /** Generate a unique temp file name in the given directory. */ @@ -118,6 +133,66 @@ function _stagingDir(dir: string): string { return sd } +/** Remove this write's own staging directory, retrying once and reporting the exact path + * when it still fails. Shared by the success and the failure path so neither one silently + * discards a cleanup failure: an ENOTEMPTY from a racing writer, an EPERM while a handle + * inside the directory is still being released, or a transient filesystem error leaves a + * concrete directory on disk that no caller can find again. A cleanup failure must never + * un-commit a published file and must never replace the original write error, so it travels + * through the warning sink carrying the path a later cleanup pass needs. */ +async function _releaseStagingDir(stagingDir: string, warn: (message: string) => void): Promise { + let cleanupError: unknown = null + // Retry once: Windows reports EPERM while a handle inside the directory is still being + // released, and the second attempt usually succeeds. + for (let attempt = 0; attempt < 2; attempt++) { + try { + await fs.rmdir(stagingDir) + return + } catch (error: unknown) { + // A directory someone else already removed is exactly the goal, not a + // leftover to report. + if (errorCode(error) === "ENOENT") { + return + } + cleanupError = error + } + } + warn( + `safeWriteText: could not remove the staging directory ${stagingDir} (${ + cleanupError instanceof Error ? cleanupError.message : String(cleanupError) + }); it is left in place for a later cleanup pass`, + ) +} + +/** + * Remove this write's DACL dump. Retried once for the same reason the backup copy is: + * on Windows an unlink commonly reports EPERM while another handle to the file is still + * being released, although the file is gone a moment later. A dump that survives both + * attempts is a concrete file beside the target whose name no caller can recover, so the + * exact path is reported through the warning sink instead of being swallowed - on the + * success path as well as on the failure path, where it must also never replace the + * original write error. ENOENT means the goal is already met, so it is not a failure. + */ +async function _discardDaclDump(daclDumpPath: string, warn: (message: string) => void): Promise { + let cleanupError: unknown = null + for (let attempt = 0; attempt < 2; attempt++) { + try { + await fs.unlink(daclDumpPath) + return + } catch (error: unknown) { + if (errorCode(error) === "ENOENT") { + return + } + cleanupError = error + } + } + warn( + `safeWriteText: could not remove the DACL dump ${daclDumpPath} (${ + errorCode(cleanupError) ?? (cleanupError instanceof Error ? cleanupError.message : String(cleanupError)) + }); it is a text copy of the target's access rights and can be deleted.`, + ) +} + function _fsyncFile(fd: number): void { fsSync.fsyncSync(fd) } @@ -140,8 +215,12 @@ async function _saveDaclWindows(srcPath: string, dumpPath: string, execFileRunne } /** Restore a DACL dump onto *dirPath* on Windows. - * Best-effort: content is already committed, so failure is non-fatal. */ -async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { + * Returns whether icacls succeeded; the caller reports a failure. */ +async function _restoreDaclWindows( + dirPath: string, + dumpPath: string, + execFileRunner?: typeof execFile, +): Promise { const runner = execFileRunner ?? execFile try { await new Promise((resolve, reject) => { @@ -149,8 +228,9 @@ async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRu err ? reject(err) : resolve(), ) }) + return true } catch { - // best-effort; content already committed + return false } } @@ -211,7 +291,8 @@ async function canonicalDirKey(absoluteFilePath: string): Promise { * Lock key for a publish target: the symlink referent when the path is an * existing symlink, the path itself otherwise. Unlike resolvePublishTarget this * tolerates a dangling link, because the lock key has to be computable while a - * peer writer is mid-commit (backup mode renames the referent away and back). + * peer writer is mid-commit (a publish renames the staged file onto the referent, + * and backup mode keeps a copy beside it). * The walk is bounded so a two-link cycle terminates, and every key it returns is * canonicalized through canonicalDirKey. */ @@ -259,6 +340,12 @@ export async function safeWriteText( // with an empty .file-safety-staging directory behind. Track the directory this // write created so its cleanup removes its own directory, not a shared one. let stagingDir: string | null = null + // The commit point of this write. Set the moment the rename succeeds: from that instant + // tempPath IS the target (the rename moved the staged file onto it), so the failure cleanup + // below must not unlink tempPath any more - a post-commit failure (the parent-directory fsync, + // the DACL restore, a throwing warning sink) would otherwise delete the file that was just + // published. The success path already knows this; the catch is the side that needs the flag. + let committed = false let tempPath: string if (options?.tempPath) { // A caller-supplied staging file is only safe when it is the file this @@ -276,13 +363,51 @@ export async function safeWriteText( supplied, ) } - const stagingStat = await fs.lstat(supplied) + // BigInt stats: on NTFS/ReFS the file identity can exceed Number.MAX_SAFE_INTEGER, and + // a rounded number makes two different files look identical (rejecting a valid staging + // file) or hides a real alias. + const stagingStat = await fs.lstat(supplied, { bigint: true }) if (stagingStat.isSymbolicLink() || !stagingStat.isFile()) { throw new StagingPathError( `Staging file must be a regular file, not ${stagingStat.isSymbolicLink() ? "a symlink" : "another file type"}`, supplied, ) } + // A staging path that is the target would be unlinked by the failure handler + // while it still holds the only copy of the content, so a failed write would + // delete the file it was meant to protect. Compare identities, not spellings: + // an alias of the target is the same hazard. + // Only a missing target may be skipped: an EACCES/ELOOP/ENOTDIR here means the + // identity comparison could not be made, and treating that as "no target" would let a + // staging alias reach the commit and let cleanup delete the file it was meant to + // protect. + const targetStat = await fs.lstat(targetPath, { bigint: true }).catch((error: unknown) => { + if (errorCode(error) !== "ENOENT") { + throw new StagingPathError("Staging file could not be compared with the target", supplied) + } + return null + }) + if ( + targetStat && + typeof stagingStat.ino === "bigint" && + typeof targetStat.ino === "bigint" && + stagingStat.ino === targetStat.ino && + stagingStat.dev === targetStat.dev + ) { + throw new StagingPathError("Staging file must not be the target itself", supplied) + } + // Ownership convention: a caller-staged file must carry the staging name this + // library's own writers use (.new__.tmp). An arbitrary regular file + // beside the target (e.g. credentials.json) passes every check above, yet + // renaming it onto the target publishes an unrelated file's content and deletes + // its original name. Only files created under the staging convention may be + // published through this parameter. + if (!/^\.new_.*\.tmp$/.test(path.basename(supplied))) { + throw new StagingPathError( + `Staging file must carry the staging name convention .new_*.tmp, got ${path.basename(supplied)}`, + supplied, + ) + } // The caller's own path is used as given; only the check is canonical. tempPath = options.tempPath } else { @@ -292,18 +417,34 @@ export async function safeWriteText( let backupPath: string | null = null let releaseBackupOnSuccess = false - // Set once the commit rename has published the new content. After that point the - // backup is no longer a safe restore source: rolling it back would overwrite - // content the caller can already observe at the target path. - let committed = false // Non-null only when the win32 step-2 block saved a successful DACL dump: // it gates the step-5 restore and is tracked for the cleanup unlinks. let daclDumpPath: string | null = null - // Set when the rollback itself fails, so cleanup runs before the error that - // reports the partial state is thrown. - // Held as a pair so the reported error still names the path the content survived at; - // declaring it as `unknown` alone would lose the string narrowing at the throw site. - let rollbackFailure: { error: unknown; backupPath: string } | null = null + // Warning delivery must never abort the write: the notices below describe a + // committed-but-imperfect publish, and a caller whose callback throws (a UI sink, + // a logger that is mid-restart) must not turn that into a failed save. + const warn = (message: string) => { + const report = (label: string, error: unknown) => { + console.warn( + `safeWriteText: onWarning callback ${label}: ${error instanceof Error ? error.message : String(error)}`, + ) + } + try { + const sink = options?.onWarning ?? ((m: string) => console.warn(m)) + const result: unknown = sink(message) + // A sink may be async - TypeScript accepts a value-returning callback where + // a void one is expected. Awaiting it would let warning delivery delay a + // write that has already committed (and hang it if the sink never settles), + // while leaving the promise unhandled turns a rejection into an unhandled + // rejection, which under Node's default mode can end the process after a + // successful write. Attach a handler without awaiting. + if (result instanceof Promise) { + result.catch((error: unknown) => report("rejected", error)) + } + } catch (error: unknown) { + report("failed", error) + } + } try { // -- Step 1: write content to staging temp file ------------------- @@ -363,6 +504,21 @@ export async function safeWriteText( } const fd = fsSync.openSync(tempPath, "r+") try { + // Prove the opened file is not a borrowed name: between the lstat check + // and this open, a local attacker who can write the directory can replace + // the staging path with a hard link to another file in it, and the commit + // rename would then publish that file and delete its original name. While + // the other name exists the inode carries a second link, so nlink === 1 on + // the opened fd rules the substitution out; a swap that first unlinks the + // other name can only stage content the attacker already had, not a file + // the caller still references. + const opened = fsSync.fstatSync(fd) + if (!opened.isFile() || opened.nlink !== 1) { + throw new StagingPathError( + `Staging file changed between the check and the open: ${tempPath}`, + tempPath, + ) + } if (targetMode !== null) { fsSync.fchmodSync(fd, targetMode) } @@ -372,11 +528,16 @@ export async function safeWriteText( } } - // -- Step 2 (win32): save DACL BEFORE backup rename --------------- + // -- Step 2 (win32): save DACL BEFORE the backup copy ----------- const platform = options?.platform ?? process.platform if (platform === "win32") { + let accessError: unknown = null try { await fs.access(targetPath) // target exists? + } catch (error: unknown) { + accessError = error + } + if (accessError === null) { const dumpPath = _tempName(dirPath, "safeWriteText.acl") const saved = await _saveDaclWindows(targetPath, dumpPath, options?.execFileRunner) if (saved) { @@ -384,24 +545,95 @@ export async function safeWriteText( // committed file (step 5). daclDumpPath = dumpPath } else { - // A failed icacls may have left a partial dump behind; - // remove it now (best-effort) so no partial dump survives and - // no later step can restore from it. - await fs.unlink(dumpPath).catch(() => {}) + // A failed icacls may have left a partial dump behind; remove it now so + // no partial dump survives and no later step can restore from it. The + // cleanup is the same retrying, reporting one used for a successful dump: + // a partial file is still a concrete file beside the target, and a + // transient EPERM (antivirus, a handle still being released) that survives + // the retry has to leave the exact path behind, not vanish into a catch. + await _discardDaclDump(dumpPath, warn) + // The target exists and its DACL could not be captured, so the commit rename + // replaces it with a file that inherits different access rights. The write still + // proceeds - a missing or failing icacls must not leave the user unable to save - + // but the replacement is no longer ACL-identical and that has to be visible + // instead of silent. + warn( + `Could not save the DACL of ${targetPath}; the replacement may inherit different access rights.`, + ) } - } catch { - // target does not exist or access failed — no DACL handling - daclDumpPath = null + } else if (errorCode(accessError) !== "ENOENT") { + // Not "absent": the target is there but could not be checked (EACCES, ...), so + // DACL preservation was skipped for a reason the caller cannot infer from the + // successful write alone. + warn( + `Could not check ${targetPath} for DACL preservation (${errorCode(accessError) ?? "unknown error"}); the replacement may inherit different access rights.`, + ) } } - try { - // -- Step 3 (backup:true): rename target -> backup -------------- + // -- Step 3 (backup:true): durable copy target -> backup ---- if (options?.backup) { try { await fs.access(targetPath) backupPath = _tempName(dirPath, "safeWriteText.bak") - await fs.rename(targetPath, backupPath) + // Copy, never move. Renaming the target away leaves the canonical path absent for + // the whole commit window: readers see a missing file, and a concurrent + // writer can create a new target that a later rollback would destroy. A copy + // keeps the target present, so the step 4 rename is the only change to the + // canonical path. The copy is flushed so the retained content survives a crash. + try { + // Create the destination BEFORE any content exists at it, with the mode fixed + // at open time. fs.copyFile picks the destination mode itself (the platform + // creation mask subject to umask on some platforms, the source's mode - or its + // read-only attribute - on others), so letting it create the file would either + // leave a restrictive target's bytes briefly readable to others, or leave the + // copy unwritable so the fsync open below fails with EACCES. open() ignores its + // mode argument for an existing file, so this 0o600 survives the copy on POSIX; + // the chmod afterwards is what clears a copied read-only attribute on Windows + // and keeps a backup of a permissive file private. + const seedFd = fsSync.openSync(backupPath, "wx", 0o600) + try { + // Nothing runs here today: the descriptor exists only to create the destination with + // a fixed mode. The block is what guarantees the close below runs for every + // successful openSync, including any statement added between the open and the close - + // an unclosed descriptor holds the backup open and blocks the cleanup that has to + // remove that file. + } finally { + fsSync.closeSync(seedFd) + } + await fs.copyFile(targetPath, backupPath) + await fs.chmod(backupPath, 0o600) + // "r+" not "r": fsync on a read-only handle is EPERM on Windows, and the same + // flag the staged temp file uses above. + const backupFd = fsSync.openSync(backupPath, "r+") + try { + _fsyncFile(backupFd) + } finally { + fsSync.closeSync(backupFd) + } + } catch (backupError: unknown) { + // A partial backup must not outlive this attempt: it is not a complete copy + // of anything, and once the write fails nothing else removes it. The unlink is + // retried once (Windows reports EPERM for a file whose handle has not been + // released yet); if it still fails the path is carried on the thrown error + // instead of being dropped where no caller can act on it. + const orphanPath = backupPath + let backupCleanupError: unknown = null + for (let attempt = 0; attempt < 2; attempt++) { + try { + await fs.unlink(orphanPath) + backupCleanupError = null + break + } catch (cleanupError: unknown) { + backupCleanupError = errorCode(cleanupError) === "ENOENT" ? null : cleanupError + } + } + backupPath = null + if (backupCleanupError !== null) { + throw new OrphanedBackupError(orphanPath, targetPath, backupError, backupCleanupError) + } + throw backupError + } releaseBackupOnSuccess = true } catch (err: unknown) { if (errorCode(err) !== "ENOENT") throw err @@ -439,21 +671,56 @@ export async function safeWriteText( // and on every failed save. if (daclDumpPath !== null) { const restoredDir = path.dirname(targetPath) - await _restoreDaclWindows(restoredDir, daclDumpPath, options?.execFileRunner) + const restored = await _restoreDaclWindows(restoredDir, daclDumpPath, options?.execFileRunner) + if (!restored) { + // The content is committed, but the published file may carry a different DACL + // from the one that was saved. Failing the write here would break every + // publish on machines where icacls cannot reapply the saved ACEs (a plain + // temp directory restore fails with "Not all privileges or groups referenced + // are assigned to the caller"), so the change of access rights is reported + // rather than thrown. + warn( + `safeWriteText: content committed at ${targetPath}, but the saved DACL could not be restored from ${daclDumpPath}; the file may carry different access rights than the one it replaced.`, + ) + } } // -- Step 6 (backup:true): delete backup on success ----------- if (releaseBackupOnSuccess && backupPath) { - try { - await fs.unlink(backupPath) - } catch { - // non-fatal — orphaned backup is acceptable + // The backup is a full copy of the previous content sitting next to the + // published file. Dropping its path on a failed unlink would leave an artifact + // that no caller can find or remove, so the unlink is retried once (Windows + // commonly reports EPERM while another handle is still being released) and a + // persistent failure is reported with the path instead of swallowed. The publish + // itself succeeded, so the write still resolves: this is a leftover to clean up, + // not a failed save. + let backupRemoved = false + for (let attempt = 0; attempt < 2 && !backupRemoved; attempt++) { + try { + await fs.unlink(backupPath) + backupRemoved = true + } catch (cleanupError: unknown) { + if (errorCode(cleanupError) === "ENOENT") { + // Already gone: the cleanup goal is met, nothing to report. + backupRemoved = true + } else if (attempt === 1) { + warn( + `safeWriteText: committed ${targetPath} but could not remove its backup copy at ${backupPath} (${ + errorCode(cleanupError) ?? "unknown error" + }); the copy of the previous content is still on disk and needs to be removed.`, + ) + } + } } } } finally { - // Unlink DACL dump regardless of success/failure in this span. + // Unlink DACL dump regardless of success/failure in this span. A dump that survives is an + // artifact nobody else can name, so a failure is retried and reported with its path. if (daclDumpPath !== null) { - await fs.unlink(daclDumpPath).catch(() => {}) + await _discardDaclDump(daclDumpPath, warn) + // This span owns the removal: the catch below must not retry the same file and report + // the same leftover twice. + daclDumpPath = null } } @@ -462,53 +729,92 @@ export async function safeWriteText( // Best-effort: remove the now-empty staging directory. Self-staged // writes only, and only this write's own directory: a per-write directory // cannot be the one another concurrent write is still using. A failure must - // never un-commit a published file, so the removal swallows all errors. + // never un-commit a published file, so it is reported through the warning sink with + // the leftover path instead of being swallowed. if (stagingDir) { - await fs.rmdir(stagingDir).catch(() => {}) + await _releaseStagingDir(stagingDir, warn) } } catch (originalError: unknown) { - // Only a pre-commit failure can restore the backup. Once the commit rename - // published, a later failure (for example the post-commit directory fsync) - // must not overwrite the published content with the old file. - if (backupPath && releaseBackupOnSuccess && !committed) { - try { - await fs.rename(backupPath, targetPath) - } catch (rollbackError: unknown) { - // The content survives only at the backup path now, and the canonical - // target is gone. Reporting just the publish failure would leave the - // caller with data it cannot find at the expected path, so the - // partial-failure state travels with the error. The staged temp file - // and this write's staging directory are released first: a rollback - // failure is already a hard enough state to reason about without also - // leaking the staging file. - rollbackFailure = { error: rollbackError, backupPath } + // The backup is a COPY taken before the commit, never a rename of the target, + // so there is nothing to restore here: the target still holds whatever the + // commit left - the pre-write content when the commit never ran, the published + // content when it did. The copy has served its purpose and must not be left + // beside the target where no caller can find it. + if (backupPath && releaseBackupOnSuccess) { + // Retry once (Windows reports EPERM while a handle is still being released), and + // if it still fails keep the path: clearing it would drop the only reference to + // the orphan. The original write error is what propagates; the leftover is + // reported through the warning sink with both paths. + let cleanupError: unknown = null + for (let attempt = 0; attempt < 2; attempt++) { + try { + await fs.unlink(backupPath) + cleanupError = null + break + } catch (error: unknown) { + // A copy that is already gone is not a leftover: reporting it would claim a + // recoverable orphan that no longer exists, and leaving backupPath set would + // tell the reader the copy is still there. + if (errorCode(error) === "ENOENT") { + cleanupError = null + break + } + cleanupError = error + } + } + if (cleanupError) { + warn( + `safeWriteText: the write to ${targetPath} failed and its backup copy could not be removed at ${backupPath} (${ + cleanupError instanceof Error ? cleanupError.message : String(cleanupError) + }); the copy is left in place so the previous content is still recoverable by hand`, + ) + } else { + backupPath = null } } - // Only a write that never committed has a staged file to remove. After the - // commit rename the staged path names the published target: unlinking it - // here would at best miss with ENOENT, and at worst delete a file a - // concurrent writer has already claimed under the same staging name. + // Only a write that never committed has a staged file to remove. After the commit + // rename there is nothing at tempPath but the published target, and unlinking it here + // would un-commit the write the caller is being told about. if (!committed) { - try { - await fs.unlink(tempPath).catch(() => {}) - } catch { - // cleanup failure is non-fatal + let cleanupError: unknown = null + for (let attempt = 0; attempt < 2; attempt++) { + try { + await fs.unlink(tempPath) + cleanupError = null + break + } catch (error: unknown) { + // ENOENT means the goal is already met - the temp is gone - which is not a + // leftover and must not raise the warning below. + if (errorCode(error) === "ENOENT") { + cleanupError = null + break + } + cleanupError = error + } + } + if (cleanupError) { + // A leftover staged temp is not the caller's failure, but its path must not + // be dropped silently: report it and keep the original error propagating. + warn( + `safeWriteText: could not remove the staging temp ${tempPath} (${ + cleanupError instanceof Error ? cleanupError.message : String(cleanupError) + })`, + ) } } // A failed self-staged write must not leave its staging directory behind. // Only the directory this write created, and only after its temp file is - // gone, so the directory is empty and the removal stays best-effort. + // gone, so the directory is empty and the removal stays best-effort: the original + // write error keeps propagating, and a directory that still cannot be removed is + // reported with its path rather than discarded. if (stagingDir) { - await fs.rmdir(stagingDir).catch(() => {}) + await _releaseStagingDir(stagingDir, warn) } + // Reported through the warning sink with its path, and never in place of originalError. if (daclDumpPath !== null) { - await fs.unlink(daclDumpPath).catch(() => {}) - } - - if (rollbackFailure) { - throw new RollbackFailureError(originalError, rollbackFailure.error, rollbackFailure.backupPath) + await _discardDaclDump(daclDumpPath, warn) } throw originalError diff --git a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts index 025aa898d6..3ba08ecea0 100644 --- a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts +++ b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts @@ -97,10 +97,21 @@ describe("safeWriteJson lock key under a peer commit", () => { // The lock key is the key every other writer to this file uses, so the caller // queued behind the peer instead of failing before the lock. expect(mockedAcquireFileLock).toHaveBeenCalledWith(referent) - // The trailing lstat is safeWriteText's staging-path check on the temp file - // this write created: it runs after the key was resolved and the lock taken, - // so it does not change which lock the caller queued behind. - expect(order).toEqual(["resolve-failed", "lstat", "resolve", "resolve", "lock", "resolve", "resolve", "lstat"]) + // The two trailing lstat calls are safeWriteText's staging-path checks: the + // regular-file check on the temp file this write created, and the identity check + // that the staging path is not the target. Both run after the key was resolved + // and the lock was taken, so neither changes which lock the caller queued behind. + expect(order).toEqual([ + "resolve-failed", + "lstat", + "resolve", + "resolve", + "lock", + "resolve", + "resolve", + "lstat", + "lstat", + ]) expect(JSON.parse(await fs.readFile(referent, "utf8"))).toEqual({ id: "task-1" }) }) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 8c08b8e9ac..daf12245d3 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -3,8 +3,8 @@ import { Writable } from "stream" import * as path from "path" import * as os from "os" +import { PostCommitDurabilityError } from "../../services/file-safety/safeWriteText" import { safeWriteJson } from "../safeWriteJson" -import { PostCommitDurabilityError, RollbackFailureError } 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 @@ -159,7 +159,7 @@ describe("safeWriteJson", () => { expect(content).toEqual({ initial: "content" }) }) - test("should handle failure when renaming filePath to tempBackupFilePath (filePath exists)", async () => { + test("should handle failure when the commit rename fails (filePath exists)", async () => { const initialData = { message: "Initial content, should remain" } const newData = { message: "New content, should not be written" } @@ -178,7 +178,7 @@ describe("safeWriteJson", () => { expect(content).toEqual(initialData) }) - test("should handle failure when renaming tempNewFilePath to filePath (filePath exists, backup succeeded)", async () => { + test("should handle failure when renaming tempNewFilePath to filePath (filePath exists, backup copy taken)", async () => { const initialData = { message: "Initial content, should be restored" } const newData = { message: "New content" } @@ -192,14 +192,8 @@ 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) - } else if (renameCallCount === 2) { - // Second call: tempNewFilePath -> filePath (should fail) + // The commit rename is the only rename in this flow: it fails. throw new Error("Rename from temp to final failed") - } else if (renameCallCount === 3) { - // Third call: tempBackupFilePath -> filePath (rollback, should succeed) - return fsPromisesActuals.rename!(oldPath, newPath) } // Default: use original implementation return fsPromisesActuals.rename!(oldPath, newPath) @@ -350,16 +344,11 @@ 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) + // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn. + // Once-only so the override does not leak into later tests: the commit rename is + // the only rename in this flow. + vi.mocked(fs.rename).mockImplementationOnce(async () => { + throw new Error("Rename failed") }) await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Rename failed") @@ -439,9 +428,10 @@ describe("safeWriteJson", () => { expect(vi.mocked(fs.access)).toHaveBeenCalled() }) - // Test for rollback failure scenario (the rollback rename now lives in safeWriteText) - test("throws RollbackFailureError with the publish failure as cause when the rollback rename also fails, leaving an orphaned backup", async () => { - const initialData = { message: "Initial, orphaned when rollback fails" } + // The backup is a copy taken before the commit, so a failed commit has nothing to + // roll back: the target keeps its previous content and the copy is removed. + test("a failed commit keeps the previous content at the target and removes the backup copy", async () => { + const initialData = { message: "Initial, must survive a failed commit" } const newData = { message: "New content" } await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) @@ -452,37 +442,24 @@ describe("safeWriteJson", () => { let renameCallCount = 0 vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { renameCallCount++ - if (renameCallCount === 2) { - // Second call: tempNewFilePath -> filePath (fail) + if (renameCallCount === 1) { + // The commit rename fails; there is no rollback rename to fail. throw new Error("Primary rename failed") - } else if (renameCallCount === 3) { - // Third call: backup -> filePath (rollback, also fail) - throw new Error("Rollback rename failed") } return fsPromisesActuals.rename!(oldPath, newPath) }) - // The rollback also failed, so the error reports the partial state: the publish - // failure stays the cause and the backup location is named. - let failure: RollbackFailureError | undefined - await safeWriteJson(currentTestFilePath, newData).catch((e: unknown) => { - if (e instanceof RollbackFailureError) { - failure = e - return - } - throw e - }) + await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Primary rename failed") - expect(failure).toBeInstanceOf(RollbackFailureError) - expect(failure?.cause).toBeInstanceOf(Error) - expect(failure?.rollbackError).toBeInstanceOf(Error) - expect(failure?.backupPath).toContain("safeWriteText.bak_") + // Exactly one rename was attempted, and it was the commit. + expect(renameCallCount).toBe(1) - // The rollback failed inside safeWriteText, so the target is gone and - // the backup is orphaned on disk. - expect(await fileExists(currentTestFilePath)).toBe(false) + // The target never left its path, so the previous content is still what a + // reader sees, and no orphaned backup copy is left behind either. + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual(initialData) const entries = await fs.readdir(tempDir) - expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(true) + expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(false) consoleErrorSpy.mockRestore() }) @@ -684,10 +661,6 @@ describe("safeWriteJson", () => { } }) - // CWE-732 regression: safeWriteJson stages the temp itself and passes it - // via tempPath, so safeWriteText must apply the existing target's mode to - // the staged temp before the atomic rename — otherwise a 0o600 target is - // published as 0o644. POSIX-only assertion (Windows ignores POSIX modes). test.skipIf(process.platform === "win32")( "preserves a restrictive 0o600 target mode through the atomic publish", async () => { From 2139034f65a1105d467648abdff9d5eeff07d10b Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 17 Oct 2026 07:16:07 +0800 Subject: [PATCH 22/23] test(safety): pin the identity guard with a convention-conforming alias Port of the #1912 spec fix: the target-itself test passed a name the convention check also rejects, so the class-only assertion could not tell which guard fired. Stage a .new_*.tmp alias with the target's inode and assert the identity-specific message (CodeRabbit thread 4300138183). --- .../file-safety/__tests__/safeWriteText.spec.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index f02bb8af2a..4063217d4c 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -1672,9 +1672,13 @@ describe("caller-supplied staging path", () => { const stats = _fileStatsWithIdentity(42n, 7n) vi.mocked(fs.lstat).mockResolvedValue(stats) - await expect(safeWriteText(targetPath, "data", { tempPath: targetPath, platform: "linux" })).rejects.toThrow( - StagingPathError, - ) + // A convention-conforming name: only the identity guard can reject this alias + // of the target, so the assertion pins the identity message rather than the + // error class - a non-conforming name would be rejected by the name check for + // the wrong reason and the guard could be deleted without reddening this test. + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/test-dir/.new_alias_1.tmp", platform: "linux" }), + ).rejects.toThrow("Staging file must not be the target itself") expect(fsSync.openSync).not.toHaveBeenCalled() expect(fs.rename).not.toHaveBeenCalled() // The comparison is only sound when both stats are read as bigint: on NTFS/ReFS the file From ed9b23404642cafb8f99296c63d6bd866f54b55a Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 17 Oct 2026 08:06:22 +0800 Subject: [PATCH 23/23] test(safety): assert cleanup after the hard-link rejection Port of the #1912 test strengthening: the rejection lands after the open, so pin both outcomes: the descriptor is closed and the borrowed alias is unlinked (CodeRabbit thread 4300157108). --- src/services/file-safety/__tests__/safeWriteText.spec.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 4063217d4c..c9ebcadafe 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -1717,10 +1717,16 @@ describe("caller-supplied staging path", () => { const linked = _fileStats(false) linked.nlink = 2 vi.mocked(fsSync.fstatSync).mockReturnValue(linked) + vi.mocked(fsSync.openSync).mockReturnValue(7) await expect( safeWriteText(targetPath, "data", { tempPath: "/tmp/test-dir/.new_x_1.tmp", platform: "linux" }), ).rejects.toThrow("Staging file changed between the check and the open") expect(fs.rename).not.toHaveBeenCalled() + // The rejection lands after the open: the descriptor must be closed and the + // borrowed alias unlinked, so the failed write leaves no open handle and no + // leftover at the staging path. + expect(fsSync.closeSync).toHaveBeenCalledWith(7) + expect(fs.unlink).toHaveBeenCalledWith("/tmp/test-dir/.new_x_1.tmp") }) it("rejects when the target identity cannot be compared for a reason other than a missing target", async () => {