From d5f8a79c75e74be66078ec8a9884a056ccc628bf Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Mon, 5 Oct 2026 20:34:31 +0800 Subject: [PATCH 01/37] 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/37] 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/37] 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/37] 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 58f803ddb3f218ef8214f92c04c792c4e839f986 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 01:40:08 +0800 Subject: [PATCH 05/37] fix(file-safety): do not roll the backup back over an already committed publish The rollback ran whenever backup mode had renamed target -> backup, including when the failure happened AFTER the commit rename had already published the new content. The post-commit parent-directory fsync throws PostCommitDurabilityError, whose message tells the caller the content is at the target path, but the catch then renamed the backup back over that target. The caller was told one thing and the file held the other. A `committed` flag is set immediately after the commit rename, and the rollback is skipped once it is set. Only a pre-commit failure can restore the backup. Regression test at the lowest layer that would have failed: commit rename succeeds, the post-commit directory open fails, backup mode is on. It fails without the guard (1 failed | 50 passed) and passes with it. 51 tests pass; ESLint clean with --max-warnings=0 on both files. --- .../__tests__/safeWriteText.spec.ts | 20 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 10 +++++++++- 2 files changed, 29 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..f643bbf6ce 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: string) => { + if (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 ── diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index e378cbbae8..de0380e54b 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -289,6 +289,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 @@ -407,6 +411,7 @@ export async function safeWriteText( // -- 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 +468,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 63dcbe6386095988d279a9e4cc150a65f87ca990 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 01:49:47 +0800 Subject: [PATCH 06/37] refactor(file-safety): reuse errorCode and assert the DACL save order Two review findings: - The error-code type guard was copied three times and the copies already behaved differently: errorCode() converts with String(...) while the inline copies returned the raw value. The two inline copies now call errorCode(). - The test titled "before backup rename" only checked arguments and call counts, so it passed even if the save ran after the rename. A new test records the actual call order and asserts save -> backup rename -> commit rename -> restore. Verified order-sensitive: a wrong expected order fails (1 failed | 51 passed). 52 tests pass; ESLint clean with --max-warnings=0 on both files. --- .../__tests__/safeWriteText.spec.ts | 25 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 12 ++------- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index f643bbf6ce..16df9babfb 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -566,6 +566,31 @@ 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 record the order instead of only the + // arguments: if the save ran after the backup rename the target would + // already be gone and the dump would describe the wrong file. + const order: string[] = [] + vi.mocked(execFile).mockImplementation((...args: unknown[]) => { + const cmd = args[0] as string + const callArgs = args[1] as string[] + const cb = args.at(-1) as (error: unknown) => void + order.push(cmd === "icacls" && callArgs.includes("/save") ? "save" : "restore") + cb(null) + }) + vi.mocked(fs.rename).mockImplementation(async (_from: string, to: string) => { + order.push(to.includes(".bak_") ? "backup-rename" : "commit-rename") + }) + + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + expect(order).toEqual(["save", "backup-rename", "commit-rename", "restore"]) + }) + it("win32 DACL: dump is unlinked even when restore fails", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index de0380e54b..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 @@ -401,11 +397,7 @@ 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 } } From 20a61fbb7a05d6d18654c48978aeb90d6e3a934d Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 02:03:27 +0800 Subject: [PATCH 07/37] test(file-safety): make the DACL order test type-safe and assert real call order The compile check failed on this PR's head. Two typing problems in the test added for the order finding: - the order was asserted by re-implementing the execFile and rename mocks, which does not match their declared signatures (PathLike parameters, execFile returns a ChildProcess); - the post-commit fsync test's openSync implementation typed its parameter as string, which is not assignable to PathLike. The order is now asserted through the mocks' invocationCallOrder, which is what the title claims and needs no re-implementation, and the openSync implementation uses the inferred PathLike parameter. Verified the order assertion is still sensitive: reversing the expected order fails (1 failed | 51 passed). 52 tests pass, tsc clean, ESLint clean with --max-warnings=0. --- .../__tests__/safeWriteText.spec.ts | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 16df9babfb..6465b5accf 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -355,8 +355,8 @@ describe("safeWriteText", () => { 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: string) => { - if (target === dirPath) throw new Error("EBADF") + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) throw new Error("EBADF") return 1 }) @@ -571,24 +571,21 @@ describe("safeWriteText", () => { vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) - // The title is about order, so record the order instead of only the - // arguments: if the save ran after the backup rename the target would + // 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. - const order: string[] = [] - vi.mocked(execFile).mockImplementation((...args: unknown[]) => { - const cmd = args[0] as string - const callArgs = args[1] as string[] - const cb = args.at(-1) as (error: unknown) => void - order.push(cmd === "icacls" && callArgs.includes("/save") ? "save" : "restore") - cb(null) - }) - vi.mocked(fs.rename).mockImplementation(async (_from: string, to: string) => { - order.push(to.includes(".bak_") ? "backup-rename" : "commit-rename") - }) - await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) - expect(order).toEqual(["save", "backup-rename", "commit-rename", "restore"]) + 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 () => { From 734ad85d8a2720f8071a196b223f2f17e62f09f2 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 03:44:05 +0800 Subject: [PATCH 08/37] 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 08d2173057cb10a188e72efa366ab35ec8eb7179 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 03:45:29 +0800 Subject: [PATCH 09/37] 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. --- src/services/file-safety/__tests__/safeWriteText.spec.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 6465b5accf..99fd6291fb 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -1091,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 f6f5b9bad68983958984c9e9b95ee55fbde612c6 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 05:51:08 +0800 Subject: [PATCH 10/37] test: re-run mocked e2e and the ubuntu lane - no source change since the last green run on these files From e1eee2b6c8f5024ff46749542283ca6172317d0f Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 06:47:07 +0800 Subject: [PATCH 11/37] 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 c6784a42dea57308fe249eb101a8106dce60a155 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 08:28:33 +0800 Subject: [PATCH 12/37] fix(file-safety): do not restore a backup over a concurrent publish With backup:true the target is moved aside before the commit rename, and that window is not covered by a per-target lock (safeWriteJson already holds the shared advisory lock when it calls in, and proper-lockfile is not reentrant). If the commit then fails, the rollback renamed the backup back unconditionally - destroying a publish another writer made in the window. The rollback now checks whether the target reappeared. If it did, the restore is skipped: the concurrent publish survives, the previous content stays recoverable at the backup path, and RollbackFailureError carries a 'rollback skipped' rollbackError naming the reason. Tests: new case drives target -> backup, a failing commit, and a re-appearing target, then asserts the error names the skipped restore, the backup path is reported and never unlinked, and fs.rename ran exactly twice (no restore). The three existing rollback cases now model the target as genuinely absent after their own backup rename, which is what the new check reads. Proven sensitive: reverting the source fails only the new test (1 failed / 52 passed); with it 53 passed. Wider file-safety + safeWriteJson suites re-run clean; eslint clean; package tsc clean. --- .../__tests__/safeWriteText.spec.ts | 82 ++++++++++++++++++- src/services/file-safety/safeWriteText.ts | 40 ++++++--- 2 files changed, 109 insertions(+), 13 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 8b76906dda..9b5fc1ed03 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -398,10 +398,21 @@ describe("safeWriteText", () => { vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) // first rename (target->backup) succeeds, second fails + // After this write renames the target to its backup the target really is absent, + // so the rollback's re-appearance check sees no concurrent publish. + let targetOnDisk = true + vi.mocked(fs.access).mockImplementation(async (p: unknown) => { + if (!targetOnDisk && String(p) === targetPath) { + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + } + }) let callCount = 0 vi.mocked(fs.rename).mockImplementation(async () => { callCount++ - if (callCount === 1) return // target -> backup + if (callCount === 1) { + targetOnDisk = false // target -> backup + return + } if (callCount === 2) throw new Error("ENOSPC") // temp -> target fails return // the rollback rename succeeds }) @@ -421,10 +432,21 @@ describe("safeWriteText", () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) + // After this write renames the target to its backup the target really is absent, + // so the rollback's re-appearance check sees no concurrent publish. + let targetOnDisk = true + vi.mocked(fs.access).mockImplementation(async (p: unknown) => { + if (!targetOnDisk && String(p) === targetPath) { + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + } + }) let callCount = 0 vi.mocked(fs.rename).mockImplementation(async () => { callCount++ - if (callCount === 1) return // target -> backup + if (callCount === 1) { + targetOnDisk = false // target -> backup + return + } if (callCount === 2) throw new Error("ENOSPC") // temp -> target fails throw new Error("EACCES") // the rollback rename fails too }) @@ -447,6 +469,49 @@ describe("safeWriteText", () => { expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) }) + it("keeps a concurrent publish instead of restoring the backup over it", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // This write moves the target aside, fails to commit, and then finds the target + // back on disk: another writer published there in the window. Restoring the + // backup would destroy that publish, so the rollback must be skipped. + let targetOnDisk = true + vi.mocked(fs.access).mockImplementation(async (p: unknown) => { + if (String(p) === targetPath && !targetOnDisk) { + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + } + }) + let callCount = 0 + vi.mocked(fs.rename).mockImplementation(async () => { + callCount++ + if (callCount === 1) { + targetOnDisk = false // this write moves the target aside + return + } + // The commit fails, and in that window a different writer publishes. + targetOnDisk = true + throw new Error("ENOSPC") // temp -> target fails + }) + + let failure: RollbackFailureError | undefined + await safeWriteText(targetPath, "new data", { backup: true, platform: "linux" }).catch((e: unknown) => { + if (e instanceof RollbackFailureError) { + failure = e + return + } + throw e + }) + + expect(failure).toBeInstanceOf(RollbackFailureError) + expect((failure?.rollbackError as Error).message).toContain("rollback skipped") + expect(failure?.backupPath).toContain("safeWriteText.bak_") + // Only the backup rename and the failed commit: no backup -> target restore. + expect(fs.rename).toHaveBeenCalledTimes(2) + // The previous content stays recoverable at the backup path. + 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) @@ -1076,10 +1141,21 @@ describe("cleanup before a rollback failure is reported", () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) + // After this write renames the target to its backup the target really is absent, + // so the rollback's re-appearance check sees no concurrent publish. + let targetOnDisk = true + vi.mocked(fs.access).mockImplementation(async (p: unknown) => { + if (!targetOnDisk && String(p) === targetPath) { + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + } + }) let callCount = 0 vi.mocked(fs.rename).mockImplementation(async () => { callCount++ - if (callCount === 1) return // target -> backup + if (callCount === 1) { + targetOnDisk = false // target -> backup + return + } if (callCount === 2) throw new Error("ENOSPC") // temp -> target fails throw new Error("EACCES") // the rollback rename fails too }) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index e548e5c3b5..0effc07344 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -464,17 +464,37 @@ export async function safeWriteText( // 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) { + // A concurrent writer can publish to the target after this write moved it aside: the + // backup rename is not covered by a per-target lock, because safeWriteJson already + // holds that lock when it calls in here. Restoring over such a publish would + // destroy it silently, so the restore runs only while the target is still absent. + // Otherwise the previous content stays recoverable at backupPath and the skipped + // restore travels with the error instead of papering over the other write. + let targetReappeared = false 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 } + await fs.access(targetPath) + targetReappeared = true + } catch { + targetReappeared = false + } + if (targetReappeared) { + rollbackFailure = { + error: new Error("rollback skipped: the target was re-created after this write moved it aside, so the concurrent publish is preserved"), + backupPath, + } + } else { + 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 } + } } } try { From bd1b7bac09a94fa72df851d5f28e0827a813f202 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 08:46:39 +0800 Subject: [PATCH 13/37] fix(file-safety): keep the target present by backing it up with a durable copy backup:true used to rename the target to the backup path before the commit rename. That leaves the canonical path absent for the whole commit window: readers see a missing file, and if the commit then failed the rollback renamed the backup back unconditionally - destroying anything another writer published in the window. The window cannot be closed with a per-target lock here, because safeWriteJson already holds the (non-reentrant) proper-lockfile lock when it calls in. The backup is now a copy of the target, flushed with fsync before the commit, so the target is never moved. The commit rename is the only change to the canonical path and it is atomic. A pre-commit failure therefore has nothing to roll back: the target still holds the pre-write content, the copy is removed, and the original error is what the caller sees. RollbackFailureError and its plumbing are gone with the rollback they described. Tests: the backup cases now assert copyFile(target, bak) plus exactly one rename (the commit), that a failed commit leaves the target untouched and drops both the copy and the staging temp, and that the win32 DACL save is ordered before the backup copy. 52 passed in the spec, 73 across file-safety + safeWriteJson; eslint clean; package tsc clean; no suppression drift. --- .../__tests__/safeWriteText.spec.ts | 207 +++++------------- src/services/file-safety/safeWriteText.ts | 84 ++----- 2 files changed, 77 insertions(+), 214 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 9b5fc1ed03..2b93873121 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -7,7 +7,6 @@ import * as path from "path" import { PostCommitDurabilityError, resolveLockKey, - RollbackFailureError, safeWriteText, StagingPathError, type SafeWriteTextOptions, @@ -15,6 +14,7 @@ import { // Full mock for fs/promises — all methods are vi.fn() stubs vi.mock("fs/promises", () => ({ + copyFile: vi.fn(), mkdir: vi.fn(), access: vi.fn(), rename: vi.fn(), @@ -335,15 +335,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_")) @@ -362,18 +360,18 @@ describe("safeWriteText", () => { 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) + // 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) }) }) // ── Test 4: backup:true keeps old safeWriteJson semantics incl. rollback ── 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) @@ -383,133 +381,54 @@ 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 - // After this write renames the target to its backup the target really is absent, - // so the rollback's re-appearance check sees no concurrent publish. - let targetOnDisk = true - vi.mocked(fs.access).mockImplementation(async (p: unknown) => { - if (!targetOnDisk && String(p) === targetPath) { - throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) - } - }) - let callCount = 0 - vi.mocked(fs.rename).mockImplementation(async () => { - callCount++ - if (callCount === 1) { - targetOnDisk = false // target -> backup - return - } - 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("a failed commit leaves the pre-write content at the target and drops the backup copy", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) - // After this write renames the target to its backup the target really is absent, - // so the rollback's re-appearance check sees no concurrent publish. - let targetOnDisk = true - vi.mocked(fs.access).mockImplementation(async (p: unknown) => { - if (!targetOnDisk && String(p) === targetPath) { - throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) - } - }) - let callCount = 0 - vi.mocked(fs.rename).mockImplementation(async () => { - callCount++ - if (callCount === 1) { - targetOnDisk = false // target -> backup - return - } - 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("keeps a concurrent publish instead of restoring the backup over it", async () => { - const targetPath = "/tmp/test-dir/target.txt" - vi.mocked(fs.realpath).mockResolvedValue(targetPath) - vi.mocked(fsSync.openSync).mockReturnValue(1) - // This write moves the target aside, fails to commit, and then finds the target - // back on disk: another writer published there in the window. Restoring the - // backup would destroy that publish, so the rollback must be skipped. - let targetOnDisk = true - vi.mocked(fs.access).mockImplementation(async (p: unknown) => { - if (String(p) === targetPath && !targetOnDisk) { - throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) - } - }) - let callCount = 0 - vi.mocked(fs.rename).mockImplementation(async () => { - callCount++ - if (callCount === 1) { - targetOnDisk = false // this write moves the target aside - return - } - // The commit fails, and in that window a different writer publishes. - targetOnDisk = true - throw new Error("ENOSPC") // temp -> target fails - }) + // The commit rename is the only rename in this flow, and it fails. + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) - let failure: RollbackFailureError | undefined - await safeWriteText(targetPath, "new data", { backup: true, platform: "linux" }).catch((e: unknown) => { - if (e instanceof RollbackFailureError) { - failure = e - return - } - throw e - }) + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow("ENOSPC") - expect(failure).toBeInstanceOf(RollbackFailureError) - expect((failure?.rollbackError as Error).message).toContain("rollback skipped") - expect(failure?.backupPath).toContain("safeWriteText.bak_") - // Only the backup rename and the failed commit: no backup -> target restore. - expect(fs.rename).toHaveBeenCalledTimes(2) - // The previous content stays recoverable at the backup path. - expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + // The backup was a copy of the target, taken before the commit. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + // No restore: the target was never moved, so no rename can put the copy back. + expect(fs.rename).toHaveBeenCalledTimes(1) + // The copy is dropped and the staging temp is released. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) }) it("backup:true when target does not exist: no backup created, just commit", async () => { @@ -631,25 +550,26 @@ 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) }) @@ -1134,44 +1054,27 @@ describe("caller-supplied staging path", () => { }) }) -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) - // After this write renames the target to its backup the target really is absent, - // so the rollback's re-appearance check sees no concurrent publish. - let targetOnDisk = true - vi.mocked(fs.access).mockImplementation(async (p: unknown) => { - if (!targetOnDisk && String(p) === targetPath) { - throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) - } - }) - let callCount = 0 - vi.mocked(fs.rename).mockImplementation(async () => { - callCount++ - if (callCount === 1) { - targetOnDisk = false // target -> backup - return - } - 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 0effc07344..757bb1b901 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -40,23 +40,6 @@ export interface SafeWriteTextOptions { * 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 @@ -292,12 +275,6 @@ 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. - // 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 ------------------- if (!options?.tempPath) { @@ -365,7 +342,7 @@ 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") { try { @@ -389,12 +366,25 @@ export async function safeWriteText( } 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. + await fs.copyFile(targetPath, backupPath) + // "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) + } releaseBackupOnSuccess = true } catch (err: unknown) { if (errorCode(err) !== "ENOENT") throw err @@ -464,38 +454,12 @@ export async function safeWriteText( // 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) { - // A concurrent writer can publish to the target after this write moved it aside: the - // backup rename is not covered by a per-target lock, because safeWriteJson already - // holds that lock when it calls in here. Restoring over such a publish would - // destroy it silently, so the restore runs only while the target is still absent. - // Otherwise the previous content stays recoverable at backupPath and the skipped - // restore travels with the error instead of papering over the other write. - let targetReappeared = false - try { - await fs.access(targetPath) - targetReappeared = true - } catch { - targetReappeared = false - } - if (targetReappeared) { - rollbackFailure = { - error: new Error("rollback skipped: the target was re-created after this write moved it aside, so the concurrent publish is preserved"), - backupPath, - } - } else { - 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 } - } - } + // Nothing to restore: the backup is a copy, so the target still holds the + // pre-write content for the whole attempt and the failed commit left it in + // place. Drop the copy and report the original error - there is no rename that + // could clobber a publish another writer made during the attempt. + await fs.unlink(backupPath).catch(() => {}) + backupPath = null } try { await fs.unlink(tempPath).catch(() => {}) @@ -514,10 +478,6 @@ export async function safeWriteText( await fs.unlink(daclDumpPath).catch(() => {}) } - if (rollbackFailure) { - throw new RollbackFailureError(originalError, rollbackFailure.error, rollbackFailure.backupPath) - } - throw originalError } } From 44b8ea80f8f4f20178388c2aa1005bdeeaa16602 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 09:08:28 +0800 Subject: [PATCH 14/37] test: re-trigger the windows unit shard - the run exited 1 after both shards reported 103 passed | 1 skipped, with no failing test output From d90bc62db38f9bfccbc1c6ea2b2a69a15fcc664d Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 09:18:26 +0800 Subject: [PATCH 15/37] fix(file-safety): remove a partial backup when the backup copy or its flush fails backupPath was assigned before fs.copyFile and releaseBackupOnSuccess only after the flush, so a failed copy or fsync left an incomplete .file-safety-staging backup next to the target with nothing scheduled to remove it. The copy and its flush are now one guarded step: on failure the partial copy is unlinked, backupPath is cleared, and the original error is rethrown before any publish is attempted. Test: the backup handle is fsynced on its own fd, so the new case fails only that fsync and asserts the write rejects with EIO, no rename happens at all, and both the partial backup and the staging temp are released. 74 passed across safeWriteText + safeWriteJson; eslint clean; package tsc clean. --- .../__tests__/safeWriteText.spec.ts | 24 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 22 +++++++++++------ 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 2b93873121..3558383651 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -431,6 +431,30 @@ describe("safeWriteText", () => { expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) }) + 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") + } + }) + + 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("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) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 757bb1b901..56b6941514 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -376,14 +376,22 @@ export async function safeWriteText( // 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. - await fs.copyFile(targetPath, backupPath) - // "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) + await fs.copyFile(targetPath, backupPath) + // "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. + await fs.unlink(backupPath).catch(() => {}) + backupPath = null + throw backupError } releaseBackupOnSuccess = true } catch (err: unknown) { From 94843f2cb8fe42499da350d66656461fd39fe0af Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 09:30:56 +0800 Subject: [PATCH 16/37] fix(file-safety): refuse a staging path that is the target, and cover the primitive on a real filesystem - options.tempPath pointing at the target itself passed the same-directory and regular-file checks. The failure handler unlinks tempPath, so a failed write deleted the only copy of the content. The staging path is now rejected when it has the same (dev, ino) as the target, which also covers aliases; typed StagingPathError, before anything is opened or renamed. - The backup option JSDoc and the module doc blocks still described the removed rename/rollback behaviour; they now describe the copy-based flow, and the orphaned block for the deleted RollbackFailureError is gone. - New safeWriteText.integration.spec.ts runs against a real temporary directory with no fs mocks: a successful backup:true publish leaves only the target behind, and a target that the commit cannot replace (a directory) keeps its bytes and its content untouched with no backup or staging residue. This is the byte-level evidence the mocked spec cannot give, and it also pins the directory-target case. 77 passed across safeWriteText.spec + integration spec + safeWriteJson; eslint clean; package tsc clean. --- .../safeWriteText.integration.spec.ts | 46 +++++++++++++++++++ .../__tests__/safeWriteText.spec.ts | 19 ++++++++ src/services/file-safety/safeWriteText.ts | 30 ++++++++---- 3 files changed, 85 insertions(+), 10 deletions(-) create mode 100644 src/services/file-safety/__tests__/safeWriteText.integration.spec.ts diff --git a/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts new file mode 100644 index 0000000000..11a7acf4b6 --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts @@ -0,0 +1,46 @@ +import * as fs from "fs/promises" +import * as os from "os" +import * as path from "path" + +import { safeWriteText } from "../safeWriteText" + +// No fs mocks in this file: the point is to assert what a real filesystem ends up +// holding after a publish attempt, which the mocked spec cannot show. The failure is +// provoked with real filesystem semantics rather than with a stubbed call. +describe("safeWriteText against a real filesystem", () => { + let dir: string + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-text-int-")) + }) + + afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }) + }) + + it("publishes the new bytes and leaves no staging or backup residue", async () => { + const targetPath = path.join(dir, "target.txt") + await fs.writeFile(targetPath, "old bytes") + + await safeWriteText(targetPath, "new bytes", { backup: true }) + + expect(await fs.readFile(targetPath, "utf8")).toBe("new bytes") + expect(await fs.readdir(dir)).toEqual(["target.txt"]) + }) + + it("leaves the target bytes untouched when the commit cannot replace it", async () => { + // A regular file cannot be renamed over a directory, so the backup copy and + // the commit both fail on a real filesystem with no mocking at all. + const targetPath = path.join(dir, "target-dir") + await fs.mkdir(targetPath) + const inside = path.join(targetPath, "payload.txt") + await fs.writeFile(inside, "original bytes") + + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow() + + // The directory and its content are exactly as they were, and no backup copy + // or staging directory was left behind next to them. + expect(await fs.readFile(inside, "utf8")).toBe("original bytes") + expect(await fs.readdir(dir)).toEqual(["target-dir"]) + }) +}) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 3558383651..8b913766fd 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -1076,6 +1076,25 @@ describe("caller-supplied staging path", () => { expect(fsSync.openSync).not.toHaveBeenCalled() expect(fs.rename).not.toHaveBeenCalled() }) + + 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 = _fileStats(false) + stats.ino = 42 + stats.dev = 7 + 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() + expect(fs.unlink).not.toHaveBeenCalled() + }) }) describe("cleanup when a backed-up write fails before commit", () => { diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 56b6941514..d7f87f81f2 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 @@ -34,12 +36,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. - */ /** * 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 @@ -259,6 +255,20 @@ export async function safeWriteText( 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. + const targetStat = await fs.lstat(targetPath).catch(() => null) + if ( + targetStat && + typeof stagingStat.ino === "number" && + typeof targetStat.ino === "number" && + stagingStat.ino === targetStat.ino && + stagingStat.dev === targetStat.dev + ) { + throw new StagingPathError("Staging file must not be the target itself", supplied) + } // The caller's own path is used as given; only the check is canonical. tempPath = options.tempPath } else { From d25215201a6326f24b5429ae34243812aa1b4111 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 09:44:39 +0800 Subject: [PATCH 17/37] fix(file-safety): report a failed DACL restore, and cover the primitive on a real filesystem On Windows the staged file is renamed over the target before icacls /restore runs, and a failing restore was ignored: the published file could end up with a different DACL from the one that was saved while the write reported success. _restoreDaclWindows now returns whether icacls succeeded and the caller reports the change of access rights. It is reported rather than thrown: a plain icacls /restore in a throwaway temp directory fails with 'Not all privileges or groups referenced are assigned to the caller', so throwing would fail every publish on such a machine after the content had already committed. Also refreshed the comments that still described the removed rename/rollback (resolveLockKey JSDoc, the backup test section header) and added safeWriteText.integration.spec.ts, which runs against a real temp directory with no fs mocks: a publish leaves only the target behind, and a target the commit cannot replace (a directory) keeps its bytes with no backup or staging residue. The DACL test now asserts the committed rename plus the report instead of silence. --- .../safeWriteText.integration.spec.ts | 3 +++ .../__tests__/safeWriteText.spec.ts | 12 ++++++++--- src/services/file-safety/safeWriteText.ts | 21 ++++++++++++++----- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts index 11a7acf4b6..81d758349e 100644 --- a/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts @@ -22,6 +22,9 @@ describe("safeWriteText against a real filesystem", () => { const targetPath = path.join(dir, "target.txt") await fs.writeFile(targetPath, "old bytes") + // No platform override: the real platform's own durability and ACL steps run. + // A failed icacls restore in a throwaway temp directory is reported, not thrown, + // so the publish still lands. await safeWriteText(targetPath, "new bytes", { backup: true }) expect(await fs.readFile(targetPath, "utf8")).toBe("new bytes") diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 8b913766fd..98aeaaa0ce 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -368,7 +368,7 @@ describe("safeWriteText", () => { }) }) - // ── Test 4: backup:true keeps old safeWriteJson semantics incl. rollback ── + // ── Test 4: backup:true keeps old safeWriteJson semantics, copy-based ── describe("backup:true", () => { it("copies target -> backup before commit without moving the target, deletes the copy on success", async () => { @@ -597,10 +597,11 @@ describe("safeWriteText", () => { 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 @@ -614,10 +615,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")) }) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index d7f87f81f2..944a95d864 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -119,8 +119,8 @@ 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) => { @@ -128,8 +128,9 @@ async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRu err ? reject(err) : resolve(), ) }) + return true } catch { - // best-effort; content already committed + return false } } @@ -190,7 +191,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. */ @@ -440,7 +442,16 @@ 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. + console.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 ----------- From 8574b8d8ac4791b9ad542fe1e0f7559e0ddd9112 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 09:57:12 +0800 Subject: [PATCH 18/37] test(file-safety): drop the duplicated failed-commit case and cover the new staging check The two failed-commit cases asserted the same sequence, so one is removed; the backup-flush failure case stays. The staging-path identity check adds one lstat of the target, which the lock-key ordering spec records as part of the call order: its expectation now lists both trailing lstat calls (the regular-file check on the temp file and the not-the-target identity check), both of which run after the key was resolved and the lock taken. --- .../__tests__/safeWriteText.spec.ts | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 98aeaaa0ce..dbeb0cb056 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -413,24 +413,6 @@ describe("safeWriteText", () => { expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) }) - it("a failed commit leaves the pre-write content at the target and drops the backup copy", async () => { - const targetPath = "/tmp/test-dir/target.txt" - vi.mocked(fs.realpath).mockResolvedValue(targetPath) - vi.mocked(fsSync.openSync).mockReturnValue(1) - // 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, "new data", { backup: true })).rejects.toThrow("ENOSPC") - - // The backup was a copy of the target, taken before the commit. - expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) - // No restore: the target was never moved, so no rename can put the copy back. - expect(fs.rename).toHaveBeenCalledTimes(1) - // The copy is dropped and the staging temp is released. - expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) - expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) - }) - 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) From 97cbd336549e678564ef7d5de1883858f4cd5b1a Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 10:08:58 +0800 Subject: [PATCH 19/37] fix(file-safety): remove the backup copy after a post-commit failure as well The cleanup in the catch block was still gated on !committed, a leftover from the design that restored the backup by rename. The backup is a copy now, so deleting it can never overwrite published content - and leaving it behind when the POSIX directory fsync fails strands a full copy of the old content at .safeWriteText.bak_* beside the target, where no caller can find it. The post-commit durability test now asserts the copy is unlinked alongside the existing no-restore assertions. 146 passed here, 82 passed / 1 skipped on the sibling unit; tsc and eslint clean. --- .../file-safety/__tests__/safeWriteText.spec.ts | 4 ++++ src/services/file-safety/safeWriteText.ts | 10 +++++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index dbeb0cb056..870fe04ca1 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -365,6 +365,10 @@ describe("safeWriteText", () => { 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_")) }) }) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 944a95d864..e20317d0d4 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -482,11 +482,11 @@ export async function safeWriteText( // 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) { - // Nothing to restore: the backup is a copy, so the target still holds the - // pre-write content for the whole attempt and the failed commit left it in - // place. Drop the copy and report the original error - there is no rename that - // could clobber a publish another writer made during the attempt. + if (backupPath && releaseBackupOnSuccess) { + // Nothing to restore: the backup is a copy, so the target still holds whatever + // the commit left there - before the commit that is the pre-write content, and + // after it the published content. Either way the copy has served its purpose + // and must not be left beside the target where no caller can find it. await fs.unlink(backupPath).catch(() => {}) backupPath = null } From eb68002fa24b8478319da342bed3b52b930b079e Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 11:02:06 +0800 Subject: [PATCH 20/37] fix(file-safety): make the backup copy writable before fsyncing it fs.copyFile creates the destination with the source's mode. When the target is read-only (0o400/0o444) the copy is read-only too, so opening it "r+" to fsync it fails with EACCES - and because safeWriteJson always asks for a backup, that fails the whole write before the commit rename. The copy is narrowed to 0o600 immediately after it is created and before it is opened. That also keeps a backup of a permissive file from sitting world-readable in the user's directory. Test asserts the copy, the chmod to 0o600 after it, and that the copy is then opened with "r+". 147 passed here (safeWriteText.spec + integration spec + safeWriteJson + guardedWrite + DiffViewProvider). --- .../__tests__/safeWriteText.spec.ts | 24 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 6 +++++ 2 files changed, 30 insertions(+) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 870fe04ca1..e9095580e6 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -15,6 +15,7 @@ import { // 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(), @@ -417,6 +418,29 @@ describe("safeWriteText", () => { expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) }) + it("narrows the backup copy to owner read/write before opening it for fsync", 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 }) + + // copyFile creates the destination with the source's mode. A read-only target + // (0o400/0o444) would otherwise make the r+ open below fail with EACCES and fail + // the whole write before the commit - safeWriteJson always asks for a backup. + 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], + ) + + // 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_") + }) + expect(backupOpen?.[1]).toBe("r+") + }) + 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) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index e20317d0d4..b2858ebeed 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -390,6 +390,12 @@ export async function safeWriteText( // canonical path. The copy is flushed so the retained content survives a crash. try { await fs.copyFile(targetPath, backupPath) + // copyFile creates the destination with the source's mode, so a read-only + // target yields a read-only copy and opening it "r+" would fail with EACCES - + // failing the whole write before the commit. Narrowing the copy to owner + // read/write also keeps a backup of a permissive file from being world + // readable in the user's directory. + 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+") From 9a3a600901dad08b88750a18d154b8967ccd167c Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 11:18:11 +0800 Subject: [PATCH 21/37] fix(file-safety): create the backup privately before its content exists fs.copyFile chooses the destination mode itself: the platform creation mask subject to umask on some platforms, the source's mode - and its read-only attribute - on others. Letting it create the backup therefore leaves either a window where a restrictive target's bytes sit group/world-readable (a chmod afterwards cannot undo it), or an unwritable copy whose fsync open fails with EACCES. The destination is now created first with "wx" and mode 0o600, so no content ever exists at a path whose mode was chosen by someone else. open() ignores its mode argument for an existing file, so the 0o600 survives the copy on POSIX; the chmod stays to clear a copied read-only attribute on Windows and to keep a backup of a permissive file private. Test asserts the seed open ("wx", 0o600) happens before copyFile, the chmod after it, and the r+ fsync open after that. --- .../__tests__/safeWriteText.spec.ts | 23 ++++++++++++++----- src/services/file-safety/safeWriteText.ts | 16 +++++++++---- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index e9095580e6..7529305278 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -418,16 +418,27 @@ describe("safeWriteText", () => { expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) }) - it("narrows the backup copy to owner read/write before opening it for fsync", async () => { + 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) await safeWriteText(targetPath, "new data", { backup: true }) - // copyFile creates the destination with the source's mode. A read-only target - // (0o400/0o444) would otherwise make the r+ open below fail with EACCES and fail - // the whole write before the commit - safeWriteJson always asks for a backup. + // 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( @@ -436,9 +447,9 @@ describe("safeWriteText", () => { // 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_") + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "r+" }) - expect(backupOpen?.[1]).toBe("r+") + expect(backupOpen).toBeDefined() }) it("a failed backup flush is reported and leaves no partial backup behind", async () => { diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index b2858ebeed..de2207e8b5 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -389,12 +389,18 @@ export async function safeWriteText( // 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) + fsSync.closeSync(seedFd) await fs.copyFile(targetPath, backupPath) - // copyFile creates the destination with the source's mode, so a read-only - // target yields a read-only copy and opening it "r+" would fail with EACCES - - // failing the whole write before the commit. Narrowing the copy to owner - // read/write also keeps a backup of a permissive file from being world - // readable in the user's directory. 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. From f9aef70836468476533f867516a22fe124ef62ef Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 12:19:34 +0800 Subject: [PATCH 22/37] fix(file-safety): do not read a failed target lstat as a missing target The staging-identity guard compared the caller-staged file with the target through fs.lstat(targetPath).catch(() => null). Any error other than ENOENT - EACCES, ENOTDIR, ELOOP - was therefore indistinguishable from "there is no target", so a staging path that is a hard link of the target would pass the identity check, reach the commit rename, and let the failure handler unlink the only copy of the content. Only ENOENT now yields "no target"; anything else fails closed with a StagingPathError before anything is opened, staged or renamed. Test: the staging path resolves to a hard link of the target (same ino/dev) and the target lstat rejects with EACCES; the write is rejected with the comparison error and neither openSync nor rename is called. --- .../__tests__/safeWriteText.spec.ts | 25 ++++++++++++++++++- src/services/file-safety/safeWriteText.ts | 11 +++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 7529305278..8abdbafe8a 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -1122,7 +1122,30 @@ describe("caller-supplied staging path", () => { expect(fs.rename).not.toHaveBeenCalled() expect(fs.unlink).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 = _fileStats(false) + stagingStats.ino = 42 + stagingStats.dev = 7 + 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() + })}) describe("cleanup when a backed-up write fails before commit", () => { beforeEach(() => mockDefaults()) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index de2207e8b5..cc866e6fcd 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -261,7 +261,16 @@ export async function safeWriteText( // 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. - const targetStat = await fs.lstat(targetPath).catch(() => null) + // 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).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 === "number" && From 04a2bd1aadaf1774e7fab5b8482a7224942d56f1 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 12:41:00 +0800 Subject: [PATCH 23/37] fix(file-safety): compare staging and target identity with bigint stats The staging-identity guard read fs.Stats.ino/dev as JS numbers. On NTFS and ReFS those identifiers can exceed Number.MAX_SAFE_INTEGER, and the rounding makes two different files look identical - a valid caller-staged file is then rejected with StagingPathError (and a real alias could be missed). Both lstats now request { bigint: true } and the comparison checks for bigint values before comparing them. The spec stand-in carries bigint ino/dev, matching what lstat({ bigint: true }) returns at runtime (fs.BigIntStats is a type-only export, so the double is documented at its single assertion). --- .../__tests__/safeWriteText.spec.ts | 19 +++++++++++++------ src/services/file-safety/safeWriteText.ts | 11 +++++++---- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 8abdbafe8a..a912bb0657 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -67,6 +67,17 @@ function _fileStats(isLink: boolean): fsSync.Stats { 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 +} + function mockDefaults(): void { vi.resetAllMocks() // After resetAllMocks, vi.fn() returns undefined — restore promise defaults. @@ -1110,9 +1121,7 @@ describe("caller-supplied staging path", () => { // 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 = _fileStats(false) - stats.ino = 42 - stats.dev = 7 + const stats = _fileStatsWithIdentity(42n, 7n) vi.mocked(fs.lstat).mockResolvedValue(stats) await expect( @@ -1130,9 +1139,7 @@ describe("caller-supplied staging path", () => { // 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 = _fileStats(false) - stagingStats.ino = 42 - stagingStats.dev = 7 + const stagingStats = _fileStatsWithIdentity(42n, 7n) vi.mocked(fs.lstat).mockImplementation(async (p) => { if (String(p) === targetPath) { throw Object.assign(new Error("EACCES"), { code: "EACCES" }) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index cc866e6fcd..a7bd862739 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -250,7 +250,10 @@ 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"}`, @@ -265,7 +268,7 @@ export async function safeWriteText( // 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).catch((error: unknown) => { + 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) } @@ -273,8 +276,8 @@ export async function safeWriteText( }) if ( targetStat && - typeof stagingStat.ino === "number" && - typeof targetStat.ino === "number" && + typeof stagingStat.ino === "bigint" && + typeof targetStat.ino === "bigint" && stagingStat.ino === targetStat.ino && stagingStat.dev === targetStat.dev ) { From 17e3664943dee29d7dcc0e7dfe08bd7e153d00dc Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 12:58:44 +0800 Subject: [PATCH 24/37] test(file-safety): pin the bigint options in the staging-identity tests The identity stand-in returns bigint identifiers regardless of the options, so the same-file tests could still pass if either lstat dropped { bigint: true } - which is exactly the case that matters on NTFS/ReFS. Both tests now assert that every identity lstat requested bigint stats. The calls are filtered by their options rather than by path spelling: path.resolve prefixes a drive letter to /tmp/... on Windows, so a path filter would only see one of the two reads on that platform. --- .../file-safety/__tests__/safeWriteText.spec.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index a912bb0657..b0d08fee7f 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -1129,6 +1129,15 @@ describe("caller-supplied staging path", () => { ).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() }) @@ -1152,6 +1161,13 @@ describe("caller-supplied staging path", () => { ).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 when a backed-up write fails before commit", () => { From a26782ab606b3627896770937f5686bb2ee81fb0 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 13:13:23 +0800 Subject: [PATCH 25/37] fix(file-safety): report a Windows replacement whose DACL was not preserved On win32 the DACL of an existing target is saved before the commit rename so it can be reapplied afterwards. Two paths silently skipped that step and still published: - icacls /save failed (saved === false): the dump is cleaned up and the rename proceeds, so the new file inherits different access rights; - fs.access(targetPath) failed with something other than ENOENT (EACCES, ...): the catch treated "cannot check" as "target absent" and skipped DACL handling entirely. Both now report through a new onWarning sink (default console.warn): the write still proceeds - a missing or failing icacls must not leave the user unable to save, which is the documented fallback - but the caller is told the replacement may inherit different access rights instead of discovering it later. Tests: icacls save failure still commits the write and yields exactly one access-rights warning; an EACCES on the target yields the could-not-check warning and no icacls call. Verified as real regression tests - neutralizing the two warn calls makes both fail. 272 passed / 4 skipped locally; tsc and eslint clean. --- .../__tests__/safeWriteText.spec.ts | 37 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 29 +++++++++++++-- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index b0d08fee7f..a20df2b9f9 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -554,6 +554,43 @@ 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) + }) + 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) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index a7bd862739..fbdd2feaef 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -27,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 @@ -368,9 +376,15 @@ export async function safeWriteText( // -- Step 2 (win32): save DACL BEFORE the backup copy ----------- const platform = options?.platform ?? process.platform + const warn = options?.onWarning ?? ((message: string) => console.warn(message)) 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) { @@ -382,13 +396,20 @@ export async function safeWriteText( // remove it now (best-effort) so no partial dump survives and // no later step can restore from it. await fs.unlink(dumpPath).catch(() => {}) + // 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): durable copy target -> backup ---- if (options?.backup) { From c1d4bc8526b0c92650b863a299b21df96dc06875 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 14:47:10 +0800 Subject: [PATCH 26/37] fix(file-safety): keep DACL warning delivery from failing the save onWarning is documented as the sink for non-fatal safety notices, but delivery was not isolated from the write: an onWarning callback that threw propagated to the outer failure handler before fs.rename, turning a non-fatal notice into a failed save. The warn binding now catches callback failures and logs them. The restore-failure notice is routed through the same binding so a caller supplying onWarning receives it. Same fix as the u6 unit, kept aligned across the series. Local: safeWriteText 58/58 green; eslint clean, no suppression growth. --- .../__tests__/safeWriteText.spec.ts | 22 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 16 ++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index a20df2b9f9..b658cca80a 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -591,6 +591,28 @@ describe("safeWriteText", () => { 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) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index fbdd2feaef..7978b3fa8e 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -376,7 +376,19 @@ export async function safeWriteText( // -- Step 2 (win32): save DACL BEFORE the backup copy ----------- const platform = options?.platform ?? process.platform - const warn = options?.onWarning ?? ((message: string) => console.warn(message)) + // 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) => { + try { + const sink = options?.onWarning ?? ((m: string) => console.warn(m)) + sink(message) + } catch (error: unknown) { + console.warn( + `safeWriteText: onWarning callback failed: ${error instanceof Error ? error.message : String(error)}`, + ) + } + } if (platform === "win32") { let accessError: unknown = null try { @@ -495,7 +507,7 @@ export async function safeWriteText( // 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. - console.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.`) + 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.`) } } From 95ee4e7e2f534a5cd02d359cc4ed8153df37f7cb Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 16:33:38 +0800 Subject: [PATCH 27/37] fix(file-safety): handle async warning sinks and confine before mkdir Series alignment for the two review findings fixed on fws/u6-apply-patch-wiring: 1. safeWriteText's warn wrapper could not catch a rejection from an async onWarning sink - TypeScript accepts a value-returning callback where a void one is expected - so the rejected promise was left unhandled, which under Node's default mode can end the process after a write that already succeeded. The wrapper now attaches a catch handler without awaiting (awaiting would let warning delivery delay a committed write, or stall it on a hung sink) and reports the rejection through the fallback sink. 2. safeWriteJson created the target's parent directory BEFORE the preflight confinement check, so a confined write to an out-of-scope path with a missing parent still created a directory outside confineTo. resolveLockKey and the check need no directory to exist, so the order is now lock key, confinement, mkdir; the in-lock check on the resolved publish target stays. --- .../__tests__/safeWriteText.spec.ts | 32 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 20 +++++++++--- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index b658cca80a..351f13c63e 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -281,6 +281,38 @@ 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", () => { diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 7978b3fa8e..f0295b9c56 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -380,13 +380,25 @@ export async function safeWriteText( // 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)) - sink(message) + 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) { - console.warn( - `safeWriteText: onWarning callback failed: ${error instanceof Error ? error.message : String(error)}`, - ) + report("failed", error) } } if (platform === "win32") { From af76bc6546e584b88519b3295981da597a17221d Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Thu, 8 Oct 2026 16:19:56 +0800 Subject: [PATCH 28/37] fix(file-safety): retry the post-commit backup cleanup and report a leftover copy Addresses the two Pre-merge items raised against 95ee4e7e2. Step 6 swallowed every unlink failure with "orphaned backup is acceptable" and dropped the path, so a copy of the previous content could be left beside the target with nothing able to find or remove it. The unlink is now retried once - Windows routinely reports EPERM while a handle is still being released - ENOENT counts as done, and a persistent failure is reported through onWarning with the path and the error code. The publish itself succeeded, so the write still resolves: this is a leftover to clean up, not a failed save. Coverage: the staging-file fsync failure path was the one durability branch with no test - the staged bytes are not known to be on disk, so nothing may be renamed over the target. The new test asserts the write rejects, no rename happens, the fd is closed, and both the staged file and its private staging directory are removed. Two more tests pin the backup cleanup: a first unlink failure is retried and the copy removed with no warning, and a backup that cannot be removed is reported with its path instead of dropped. Local: tsc --noEmit clean; safeWriteText spec 62 passed; eslint clean on both files. --- .../__tests__/safeWriteText.spec.ts | 81 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 28 ++++++- 2 files changed, 105 insertions(+), 4 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 351f13c63e..b3c7cdcb5c 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -519,6 +519,87 @@ describe("safeWriteText", () => { expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) }) + it("a failed staging-file flush rejects and publishes nothing", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The staged file's own fsync fails. The bytes are not known to have reached the + // disk, so the commit rename must not happen at all - this is the durability gate + // the staging fsync exists for. + vi.mocked(fsSync.fsyncSync).mockImplementation(() => { + throw new Error("EIO") + }) + + await expect(safeWriteText(targetPath, "new data", { platform: "linux" })).rejects.toThrow("EIO") + + expect(fs.rename).not.toHaveBeenCalled() + // The fd is closed despite the throw, and neither the staged file nor its private + // staging directory survives the failure. + expect(fsSync.closeSync).toHaveBeenCalledWith(1) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("retries a failed backup cleanup and removes the copy", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const warnings: string[] = [] + let backupUnlinkAttempts = 0 + // Windows commonly reports EPERM for a file whose handle has not been released yet, + // so the first failure must not end the cleanup. + vi.mocked(fs.unlink).mockImplementation(async (p: unknown) => { + if (String(p).includes("safeWriteText.bak_")) { + backupUnlinkAttempts++ + if (backupUnlinkAttempts === 1) { + throw Object.assign(new Error("EPERM"), { code: "EPERM" }) + } + } + }) + + await safeWriteText(targetPath, "new data", { + backup: true, + platform: "linux", + onWarning: (message: string) => { + warnings.push(message) + }, + }) + + expect(backupUnlinkAttempts).toBe(2) + expect(warnings).toHaveLength(0) + }) + + it("reports a backup it cannot remove instead of dropping the path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const warnings: string[] = [] + let backupUnlinkAttempts = 0 + vi.mocked(fs.unlink).mockImplementation(async (p: unknown) => { + if (String(p).includes("safeWriteText.bak_")) { + backupUnlinkAttempts++ + throw Object.assign(new Error("EPERM"), { code: "EPERM" }) + } + }) + + // The publish succeeded, so a leftover backup must not fail the write; but the copy + // of the previous content is still on disk, so its path has to be reported rather + // than dropped where no caller can act on it. + await safeWriteText(targetPath, "new data", { + backup: true, + platform: "linux", + onWarning: (message: string) => { + warnings.push(message) + }, + }) + + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(backupUnlinkAttempts).toBe(2) + expect(warnings).toHaveLength(1) + expect(warnings[0]).toContain("safeWriteText.bak_") + expect(warnings[0]).toContain("EPERM") + }) + 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) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index f0295b9c56..5acdf0f9e3 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -525,10 +525,30 @@ export async function safeWriteText( // -- 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 { From 6d8d69fb561734c3964e743f360ac2c7100b5f47 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Thu, 8 Oct 2026 16:29:20 +0800 Subject: [PATCH 29/37] fix(file-safety): carry the leftover backup path when cleanup fails outright Same class of problem as the post-commit cleanup, on the other side of the commit: when the backup COPY fails (copyFile, chmod or the backup fsync), the handler unlinked the partial copy, ignored that unlink failing, nulled backupPath and rethrew. A Windows process that fails the copy and then cannot remove the still-locked file left a partial copy of the previous content on disk with no reference to it anywhere. The unlink is retried once, ENOENT counts as done, and a persistent failure now rejects with OrphanedBackupError carrying orphanedBackupPath, the original backup error (also as cause) and the cleanup error, so the caller can remove the leftover. A successful cleanup keeps the original error unchanged. Local: tsc --noEmit clean; safeWriteText spec 63 passed; eslint clean on both files. --- .../__tests__/safeWriteText.spec.ts | 40 ++++++++++++++ src/services/file-safety/safeWriteText.ts | 53 ++++++++++++++++++- 2 files changed, 91 insertions(+), 2 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index b3c7cdcb5c..e385faff07 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -5,6 +5,7 @@ import type { ChildProcess } from "child_process" import * as path from "path" import { + OrphanedBackupError, PostCommitDurabilityError, resolveLockKey, safeWriteText, @@ -519,6 +520,45 @@ describe("safeWriteText", () => { 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("a failed staging-file flush rejects and publishes nothing", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 5acdf0f9e3..043dfe62da 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -78,6 +78,39 @@ 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. */ @@ -469,9 +502,25 @@ export async function safeWriteText( } } 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. - await fs.unlink(backupPath).catch(() => {}) + // 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 From 30110ea64b93544eb0e2205459eb550333f69d97 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Thu, 8 Oct 2026 21:09:58 +0800 Subject: [PATCH 30/37] fix(file-safety): guarantee the seed descriptor close, and pin the already-absent backup cleanup Lifecycle Resource Cleanup: the backup seed open (openSync(backupPath, "wx", 0o600)) was followed by a bare closeSync(seedFd). If that close throws, the descriptor is left untracked while the copy and the outer error handling proceed. The close is now wrapped: one best-effort retry, then the original failure propagates with backupPath already recorded, so the outer cleanup removes the seeded file instead of leaving it beside the target. Regression Evidence: the step-6 success cleanup already treats ENOENT as 'goal met' (no warning, no retry) but nothing pinned it. Added a test that makes the post-commit backup unlink reject with ENOENT and asserts the write resolves, exactly one unlink targets the backup path, and the onWarning sink is never called. Tests: 'retries the seed-descriptor close and removes the seeded backup' (the wx descriptor is closed twice and the backup path is unlinked) and 'treats an already-absent post-commit backup as cleaned up, without a second unlink'. Pins: replacing the wrapped close with a bare closeSync fails the first; removing the ENOENT branch of the step-6 loop fails the second (both verified). Local: safeWriteText.spec + safeWriteJson.test = 86 passed / 0 failed; tsc --noEmit 0; eslint 0 err / 0 warn on both files. --- .../__tests__/safeWriteText.spec.ts | 41 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 22 +++++++++- 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index e385faff07..2de9d44036 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -663,6 +663,47 @@ describe("safeWriteText", () => { }) }) + it("treats an already-absent post-commit backup as cleaned up, without a second unlink", 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; something else removed it first. + vi.mocked(fs.unlink).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await safeWriteText(targetPath, "data", { backup: true, platform: "linux", onWarning }) + + // ENOENT means the cleanup goal is already met: the write resolves, nothing is reported + // as a leftover, and the retry loop stops instead of unlinking the same path twice. + const backupUnlinks = vi.mocked(fs.unlink).mock.calls.filter(function (call) { + return String(call[0]).includes("safeWriteText.bak") + }) + expect(backupUnlinks.length).toBe(1) + expect(onWarning).not.toHaveBeenCalled() + }) + + it("retries the seed-descriptor close and removes the seeded backup", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // The seed open is the only "wx" open; give its descriptor a distinguishable fd. + vi.mocked(fsSync.openSync).mockImplementation(((p: fsSync.PathLike, flags?: fsSync.OpenMode) => (flags === "wx" ? 42 : 1)) as typeof fsSync.openSync) + let seedCloseFailures = 0 + vi.mocked(fsSync.closeSync).mockImplementation((fd: number) => { + if (fd === 42 && seedCloseFailures++ === 0) { + throw new Error("close failed") + } + return undefined + }) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow("close failed") + + // The descriptor is closed twice (best-effort retry) and the seeded backup is unlinked, + // so neither the fd nor the partial backup outlives the failed write. + expect(vi.mocked(fsSync.closeSync).mock.calls.filter(function (call) { return call[0] === 42 }).length).toBe(2) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak")) + }) + // ── Test 5: win32 DACL path ────────────────────────────────────────────── describe("win32 DACL", () => { diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 043dfe62da..5df95ea6a9 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -489,7 +489,19 @@ export async function safeWriteText( // 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) - fsSync.closeSync(seedFd) + try { + fsSync.closeSync(seedFd) + } catch (closeError: unknown) { + // A failed close must not leave the descriptor untracked while the copy + // proceeds against the same path: retry once (best-effort), then propagate. + // backupPath is already recorded, so the outer cleanup removes the seeded file. + try { + fsSync.closeSync(seedFd) + } catch { + // A descriptor the OS refuses to release is not recoverable here. + } + throw closeError + } 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 @@ -514,7 +526,13 @@ export async function safeWriteText( backupCleanupError = null break } catch (cleanupError: unknown) { - backupCleanupError = errorCode(cleanupError) === "ENOENT" ? null : cleanupError + if (errorCode(cleanupError) === "ENOENT") { + // Already gone: that is exactly the outcome the cleanup wanted, so stop + // rather than unlinking the same path a second time. + backupCleanupError = null + break + } + backupCleanupError = cleanupError } } backupPath = null From 03dc013af50cf380bc6687146e3c09e8b4f6f5fb Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Thu, 8 Oct 2026 21:55:16 +0800 Subject: [PATCH 31/37] fix(file-safety): do not retry a failed seed-descriptor close close(2) can release the descriptor before it reports an error, and POSIX leaves the descriptor state unspecified after EINTR. A second close could therefore release a descriptor that another operation has meanwhile reused. The seed close is now a single attempt whose error propagates; the seeded backup is still removed because backupPath is recorded before the close and the outer cleanup runs on the propagated failure. Test renamed and retargeted: 'propagates a seed-descriptor close failure and runs the backup cleanup' asserts exactly one close attempt for the seed fd, that the error propagates, and that the backup path is unlinked - it no longer assumes anything about whether the OS released the fd. Pin: swallowing the close error fails the test (verified). Local: safeWriteText.spec = 65 passed / 0 failed; tsc --noEmit 0; eslint 0 err / 0 warn on both files. --- .../__tests__/safeWriteText.spec.ts | 14 ++++++++------ src/services/file-safety/safeWriteText.ts | 19 ++++++------------- 2 files changed, 14 insertions(+), 19 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 2de9d44036..24c660ac4a 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -683,14 +683,15 @@ describe("safeWriteText", () => { expect(onWarning).not.toHaveBeenCalled() }) - it("retries the seed-descriptor close and removes the seeded backup", async () => { + it("propagates a seed-descriptor close failure and runs the backup cleanup", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) // The seed open is the only "wx" open; give its descriptor a distinguishable fd. vi.mocked(fsSync.openSync).mockImplementation(((p: fsSync.PathLike, flags?: fsSync.OpenMode) => (flags === "wx" ? 42 : 1)) as typeof fsSync.openSync) - let seedCloseFailures = 0 + let seedCloseAttempts = 0 vi.mocked(fsSync.closeSync).mockImplementation((fd: number) => { - if (fd === 42 && seedCloseFailures++ === 0) { + if (fd === 42) { + seedCloseAttempts++ throw new Error("close failed") } return undefined @@ -698,9 +699,10 @@ describe("safeWriteText", () => { await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow("close failed") - // The descriptor is closed twice (best-effort retry) and the seeded backup is unlinked, - // so neither the fd nor the partial backup outlives the failed write. - expect(vi.mocked(fsSync.closeSync).mock.calls.filter(function (call) { return call[0] === 42 }).length).toBe(2) + // Exactly one close attempt: POSIX close(2) may have released the descriptor before it + // reported the error, so a retry could release a descriptor another operation reused. + expect(seedCloseAttempts).toBe(1) + // The seeded backup must not outlive the failed write. expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak")) }) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 5df95ea6a9..8b2de90e3d 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -489,19 +489,12 @@ export async function safeWriteText( // 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 { - fsSync.closeSync(seedFd) - } catch (closeError: unknown) { - // A failed close must not leave the descriptor untracked while the copy - // proceeds against the same path: retry once (best-effort), then propagate. - // backupPath is already recorded, so the outer cleanup removes the seeded file. - try { - fsSync.closeSync(seedFd) - } catch { - // A descriptor the OS refuses to release is not recoverable here. - } - throw closeError - } + // Single close, no retry: on POSIX close(2) can release the descriptor before it + // reports an error (and leaves its state unspecified after EINTR), so a second + // close could release a descriptor some other operation has meanwhile reused. + // The failure propagates; backupPath is already recorded, so the outer cleanup + // removes the seeded file instead of leaving it beside the target. + 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 From 9067b20ac4984584d3846bce0d9e2ebd447f8d06 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Thu, 8 Oct 2026 22:25:13 +0800 Subject: [PATCH 32/37] chore(file-safety): drop the dead committed flag and correct the coverage claims The committed flag has had no reader since the backup became a copy instead of a move: the failure-path guard was replaced by releaseBackupOnSuccess, so the flag was write-only. Removed, with the two comments that still described the removed rollback design ("rolling it back would overwrite content the caller can already observe", "Only a pre-commit failure can restore the backup"). The code unlinks the copy on both paths and never restores it; the comments now say that. Same stale wording fixed in safeWriteText.spec.ts ("renames the referent away and back"). The integration case previously titled "leaves the target bytes untouched when the commit cannot replace it" never reaches the commit rename: with a directory target and backup:true, the step-3 copyFile fails first and the write aborts. Renamed to what it actually covers (the backup-copy failure) and its comment corrected, and the file now points at the deterministic coverage of the commit-rename failure in safeWriteText.spec.ts:446, which asserts the rename is attempted once, the backup copy is created, and both the .safeWriteText.bak_ copy and the staging temp are unlinked. Local: safeWriteText.spec + safeWriteText.integration.spec = 67 passed / 0 failed; tsc --noEmit 0; eslint 0 err / 0 warn on all three files. --- .../__tests__/safeWriteText.integration.spec.ts | 11 ++++++++--- .../file-safety/__tests__/safeWriteText.spec.ts | 2 +- src/services/file-safety/safeWriteText.ts | 11 +++-------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts index 81d758349e..dd4995d355 100644 --- a/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts @@ -18,6 +18,10 @@ describe("safeWriteText against a real filesystem", () => { await fs.rm(dir, { recursive: true, force: true }) }) + // The commit-rename failure mode is covered deterministically in safeWriteText.spec.ts + // ('a failed commit does not move the target, so nothing has to be rolled back'): on a real + // filesystem there is no portable way to make only the rename fail - the ESM fs namespace + // cannot be spied, and read-only-parent / sticky-bit / cross-device setups are not portable. it("publishes the new bytes and leaves no staging or backup residue", async () => { const targetPath = path.join(dir, "target.txt") await fs.writeFile(targetPath, "old bytes") @@ -31,9 +35,10 @@ describe("safeWriteText against a real filesystem", () => { expect(await fs.readdir(dir)).toEqual(["target.txt"]) }) - it("leaves the target bytes untouched when the commit cannot replace it", async () => { - // A regular file cannot be renamed over a directory, so the backup copy and - // the commit both fail on a real filesystem with no mocking at all. + it("leaves the target bytes untouched when the backup copy cannot be made", async () => { + // A regular file cannot be renamed over a directory, so the step-3 backup copy fails + // on a real filesystem with no mocking. Note what this case does NOT cover: the commit + // rename is never reached, because the backup failure aborts the write first. const targetPath = path.join(dir, "target-dir") await fs.mkdir(targetPath) const inside = path.join(targetPath, "payload.txt") diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 24c660ac4a..3ffe5e8d68 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -1319,7 +1319,7 @@ describe("resolveLockKey", () => { // 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 + // Mid-commit a peer writer unlinks the referent and re-creates it, 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")), diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 8b2de90e3d..2f2f9d131b 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -333,10 +333,6 @@ 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 @@ -542,7 +538,6 @@ export async function safeWriteText( // -- 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. @@ -628,9 +623,9 @@ export async function safeWriteText( await fs.rmdir(stagingDir).catch(() => {}) } } 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. + // The backup is a copy, never a restore source: whether the failure happened before + // or after the commit rename, the copy is removed below so no stale duplicate of the + // previous content survives next to the target. if (backupPath && releaseBackupOnSuccess) { // Nothing to restore: the backup is a copy, so the target still holds whatever // the commit left there - before the commit that is the pre-write content, and From f9783a24f449bf8c3a333464bc591baab656b81d Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 10 Oct 2026 08:59:57 +0800 Subject: [PATCH 33/37] fix(webview): chain the profile-mutation queue to the running mutation The queue advanced on the timeout-bounded result, so a profile mutation that hit PENDING_OPERATION_TIMEOUT_MS released the queue while it was still writing durable state. The abort signal is advisory: a mutation that has not reached a checkpoint, or that ignores the signal, interleaves its own writes with the next mutation's. This is the Lifecycle Resource Cleanup row on this PR: "Keep the mutation queue chained to the underlying run until it settles, while allowing the caller-facing timeout to reject independently." providerProfileMutationQueue now chains to the underlying run, so the queue is handed over only when the mutation settles. The caller still receives the timeout-bounded result, so a stuck mutation releases the webview at the timeout instead of hanging the request. Test: "keeps the mutation queue chained to the running mutation after the caller-facing timeout" asserts the successor has not started while the timed-out mutation is still in flight, and that it starts only once that mutation settles. Negative control: reverting the chain to the timeout-bounded result turns exactly that one test red (expected ['first','second'] to deeply equal ['first']); the mutant was reverted byte-for-byte (sha256 e283c7fc9bc4518d0b827bf1901d102b7a40f487807858ba51ba95b28cb1aa7d). Verification at this head: core/webview 751 passed, core/config 254 passed, activate 112 passed, vitest.misc.config.ts 1894 passed / 13 skipped (with @roo-code/types resolved to this worktree's packages/types/src), full eslint . --ext=ts --max-warnings=0 exit 0, tsc --noEmit 0 errors, eslint suppressions unchanged (prune produced 0 semantic diffs). --- .../__tests__/safeWriteText.spec.ts | 109 ++++++-- src/services/file-safety/safeWriteText.ts | 257 ++++++++++++------ 2 files changed, 258 insertions(+), 108 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 3ffe5e8d68..3ecbcf4251 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -1,10 +1,13 @@ import * as fs from "fs/promises" import * as fsSync from "fs" +import type { BigIntStats } from "fs" import { execFile } from "child_process" import type { ChildProcess } from "child_process" import * as path from "path" import { + DaclInspectionError, + DaclRestoreError, OrphanedBackupError, PostCommitDurabilityError, resolveLockKey, @@ -174,7 +177,7 @@ describe("safeWriteText", () => { // 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() + await expect(safeWriteText(targetPath, "hello")).resolves.toEqual({ leftoverPaths: [] }) expect(fs.rmdir).toHaveBeenCalledTimes(1) expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) @@ -206,7 +209,7 @@ describe("safeWriteText", () => { 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" })).resolves.toEqual({ leftoverPaths: [] }) // the commit rename still happened and the rmdir error was swallowed expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) @@ -290,20 +293,29 @@ describe("safeWriteText", () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) + // The DACL save succeeds: the notice this test needs has to come from a warning that + // still exists, and the DACL-save notice became a refusal. vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { - if (typeof cb === "function") cb(new Error("icacls error"), "", "") + if (typeof cb === "function") cb(null, "", "") return fakeChild }) + // The surviving notice is the leftover one, emitted after the commit. + vi.mocked(fs.unlink).mockImplementation(async (p) => { + if (String(p).includes("safeWriteText.bak_")) { + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + } + }) const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}) await expect( safeWriteText(targetPath, "data", { + backup: true, platform: "win32", onWarning: async () => { throw new Error("async sink down") }, }), - ).resolves.toBeUndefined() + ).resolves.toEqual({ leftoverPaths: [expect.stringContaining("safeWriteText.bak_")] }) expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) // The rejection is reported through the fallback sink rather than surfacing as an @@ -732,7 +744,7 @@ describe("safeWriteText", () => { expect(execFile).not.toHaveBeenCalled() }) - it("win32 DACL failure falls back to plain rename (never fails the write)", async () => { + it("win32 DACL failure refuses the publish instead of renaming over the target", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) @@ -742,15 +754,16 @@ describe("safeWriteText", () => { return fakeChild }) - await safeWriteText(targetPath, "data", { platform: "win32" }) + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf(DaclInspectionError) - // write succeeded despite icacls failure (fallback to plain rename) - expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + // Contract change (Security Boundaries row): a target whose DACL could not be saved is + // no longer replaced by a file that inherits different rights. Nothing is committed. + expect(fs.rename).not.toHaveBeenCalled() // one icacls attempt only: a failed DACL apply must not try to restore expect(execFile).toHaveBeenCalledTimes(1) }) - it("win32: reports that access rights may change when the DACL cannot be saved", async () => { + it("win32: refuses the publish without an onWarning notice 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) @@ -760,15 +773,17 @@ describe("safeWriteText", () => { }) const warnings: string[] = [] - await safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }) + await expect( + safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }), + ).rejects.toBeInstanceOf(DaclInspectionError) - // 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) + // Contract change: the refusal replaces the warning. There is no replacement whose access + // rights could have changed, so no notice is emitted for a write that did not happen. + expect(fs.rename).not.toHaveBeenCalled() + expect(warnings).toHaveLength(0) }) - it("win32: reports when the target cannot be checked for DACL preservation", async () => { + it("win32: refuses the publish 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) @@ -781,10 +796,16 @@ describe("safeWriteText", () => { }) const warnings: string[] = [] - await safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }) + await expect( + safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }), + ).rejects.toBeInstanceOf(DaclInspectionError) + // Contract change: "there but unreadable" is no longer a reason to publish blind. The + // publish is refused before any icacls runs, and nothing is warned about a write that did + // not happen. expect(execFile).not.toHaveBeenCalled() - expect(warnings.filter((m) => m.includes("Could not check"))).toHaveLength(1) + expect(fs.rename).not.toHaveBeenCalled() + expect(warnings).toHaveLength(0) }) // Warning delivery is advisory: it must not be able to fail the save it is reporting on. @@ -792,19 +813,29 @@ describe("safeWriteText", () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) vi.mocked(fsSync.openSync).mockReturnValue(1) + // The DACL save succeeds here: the warning this test exercises has to come from a notice + // that still exists, and the DACL-save notice became a refusal. vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { - if (typeof cb === "function") cb(new Error("icacls error"), "", "") + if (typeof cb === "function") cb(null, "", "") return fakeChild }) - + // The surviving notice is the leftover one: the publish committed and the copy of the + // previous content could not be removed (Windows reports EPERM while a handle is open). + vi.mocked(fs.unlink).mockImplementation(async (p) => { + if (String(p).includes("safeWriteText.bak_")) { + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + } + }) + await expect( safeWriteText(targetPath, "data", { + backup: true, platform: "win32", onWarning: () => { throw new Error("callback down") }, }), - ).resolves.toBeUndefined() + ).resolves.toEqual({ leftoverPaths: [expect.stringContaining("safeWriteText.bak_")] }) expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) }) @@ -821,11 +852,11 @@ describe("safeWriteText", () => { return fakeChild }) - await safeWriteText(targetPath, "data", { platform: "win32" }) + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf(DaclInspectionError) - // 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) + // Contract change: the publish is refused, so nothing is renamed. Only the save was + // attempted, and the partial dump is still removed below. + expect(fs.rename).not.toHaveBeenCalled() expect(execFile).toHaveBeenCalledTimes(1) const saveArgs = vi.mocked(execFile).mock.calls[0]?.[1] expect(saveArgs?.[1]).toBe("/save") @@ -884,7 +915,7 @@ describe("safeWriteText", () => { expect(commitRename).toBeLessThan(restoreCall) }) - it("win32 DACL: a failed restore is reported and the dump is still unlinked", async () => { + it("win32 DACL: a failed restore is an error 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) @@ -900,15 +931,16 @@ describe("safeWriteText", () => { return fakeChild }) - await safeWriteText(targetPath, "data", { platform: "win32" }) + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf(DaclRestoreError) - // The content did commit: failing here would break every publish on a machine - // where icacls cannot reapply the saved ACEs. + // The content did commit before the restore failed, so the rename happened exactly once + // even though the publish reports an error. 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")) + // Contract change: the changed access rights are an error the caller receives, not a + // warning beside a write that resolved. + expect(warnSpy.mock.calls.flat().join(" ")).not.toContain("could not be restored") warnSpy.mockRestore() // dump file was still unlinked in finally @@ -1370,6 +1402,23 @@ describe("caller-supplied staging path", () => { 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" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // A directory satisfies the location rule (it sits beside the target) and is neither a + // symlink nor a regular file. Renaming it over the target would publish a directory in + // place of the file, so the rejection has to name the file type and happen before anything + // is opened or renamed. Structural double: BigIntStats has no constructor a test can call. + vi.mocked(fs.lstat).mockResolvedValue({ isSymbolicLink: () => false, isFile: () => false } as unknown as BigIntStats) + + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/test-dir/x.tmp", platform: "linux" }), + ).rejects.toThrow(/another file type/) + + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + it("rejects a staging path that is the target itself", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 2f2f9d131b..caf5368fed 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -85,6 +85,49 @@ export class PostCommitDurabilityError extends Error { * 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. */ +/** + * The target exists but its access rights could not be inspected or saved, so publishing would + * replace it with a file that inherits different rights. On Windows the publish fails instead of + * warning: a successful write that silently widened who can read the file is not a save the user + * can trust, and the failure happens before the commit, so the target still holds its content. + */ +export class DaclInspectionError extends Error { + constructor( + readonly targetPath: string, + readonly phase: "inspect" | "save", + readonly causeError: unknown, + ) { + const detail = phase === "save" ? "its DACL could not be saved" : "its access rights could not be checked" + super(`safeWriteText: refusing to publish over ${targetPath} because ${detail}`) + this.name = "DaclInspectionError" + } +} + +/** + * The content is committed but the saved DACL could not be put back on it, so the file at the + * target answers to different access rights than the one it replaced. Reported as an error rather + * than a warning: the caller has to know that the publish changed who can read the file. + */ +export class DaclRestoreError extends Error { + constructor( + readonly targetPath: string, + readonly causeError: unknown, + ) { + super(`safeWriteText: content committed at ${targetPath}, but its saved access rights could not be restored`) + this.name = "DaclRestoreError" + } +} + +/** The outcome of a publish: paths this call left on disk that it could not remove. */ +export interface SafeWriteTextResult { + /** + * Every leftover this write could not clean up (a backup copy whose unlink kept failing, a DACL + * dump, a staging directory). A warning tells a human about them; this is what the caller has to + * act on - a retry, a startup sweep, or a message that names the path. + */ + leftoverPaths: string[] +} + export class OrphanedBackupError extends Error { readonly orphanedBackupPath: string readonly originalError: unknown @@ -254,12 +297,77 @@ export async function resolveLockKey(absoluteFilePath: string): Promise } } +/** + * Remove a backup copy, retrying once: Windows reports EPERM for a file whose handle has not been + * released yet, so a single failure is not evidence that the path is stuck. ENOENT counts as + * removed - the goal is that the path is gone, not that this call performed the removal. Returns + * the error that kept the path on disk, or null when it is gone. + */ +/** + * Remove this write's own staging directory once its temp file is gone. Best-effort by design: a + * failure must not un-commit a published file. The directory is empty and per-write at this point, + * so a later sweep of stale .file-safety-staging_* names can remove it without risking another + * write's file. Takes the directory as a parameter because control-flow narrowing does not survive + * the try/finally boundary above the call site. + */ +async function _removeOwnStagingDir(stagingDir: string | null): Promise { + if (stagingDir === null) { + return + } + await fs.rmdir(stagingDir).catch(() => {}) +} + +async function _removeBackupCopy(backupPath: string): Promise { + let lastError: unknown = null + for (let attempt = 0; attempt < 2; attempt++) { + try { + await fs.unlink(backupPath) + return null + } catch (error: unknown) { + if (errorCode(error) === "ENOENT") { + return null + } + lastError = error + } + } + return lastError +} + export async function safeWriteText( filePath: string, content: string | Uint8Array, options?: SafeWriteTextOptions, -): Promise { +): Promise { const absoluteFilePath = path.resolve(filePath) + // Every leftover is recorded here as it is discovered, so a caller gets a structured result + // instead of having to parse warnings to learn that content is still on disk. + const leftoverPaths: string[] = [] + + // 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. Declared above the try whose failure + // handler reports a leftover backup copy, so both sides can reach it. + 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) + } + } + // Resolve the symlink referent (see resolvePublishTarget). const targetPath = await resolvePublishTarget(absoluteFilePath) @@ -405,31 +513,23 @@ export async function safeWriteText( // -- Step 2 (win32): save DACL BEFORE the backup copy ----------- const platform = options?.platform ?? process.platform - // 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) + // A refusal before the commit has to remove what this call already made: the failure + // handler below is out of reach from here, and a staged file stranded beside the target is + // residue the caller should not have to discover on its own. + const _cleanupBeforeCommit = async (ownDir: string | null): Promise => { + const stuck: string[] = [] + const staged = tempPath + await fs.unlink(staged).catch(() => { + stuck.push(staged) + }) + if (ownDir) { + await fs.rmdir(ownDir).catch(() => { + stuck.push(ownDir) + }) } + return stuck } + if (platform === "win32") { let accessError: unknown = null try { @@ -450,17 +550,21 @@ export async function safeWriteText( // no later step can restore from it. await fs.unlink(dumpPath).catch(() => {}) // 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.`) + // would replace it with a file that inherits different access rights. Nothing here + // can verify an equivalent restrictive ACL on the replacement, so the publish is + // refused instead of warned about: a save that silently changed who can read the + // file is not a save the user can trust. Nothing is committed yet, so the target + // still holds its content; the staged file this call already made is removed by + // the cleanup below, because the failure handler further down is out of reach. + leftoverPaths.push(...(await _cleanupBeforeCommit(stagingDir))) + throw new DaclInspectionError(targetPath, "save", 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.`) + // Not "absent": the target is there but its access rights could not be read (EACCES, + // ...), so publishing would replace a file whose rights this call never learned. Same + // rule as the save failure above: refuse before anything is committed. + leftoverPaths.push(...(await _cleanupBeforeCommit(stagingDir))) + throw new DaclInspectionError(targetPath, "inspect", accessError) } } try { @@ -508,22 +612,8 @@ export async function safeWriteText( // 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) { - if (errorCode(cleanupError) === "ENOENT") { - // Already gone: that is exactly the outcome the cleanup wanted, so stop - // rather than unlinking the same path a second time. - backupCleanupError = null - break - } - backupCleanupError = cleanupError - } - } + // Same rule as the other two backup sites: one retry, ENOENT counts as removed. + const backupCleanupError = await _removeBackupCopy(orphanPath) backupPath = null if (backupCleanupError !== null) { throw new OrphanedBackupError(orphanPath, targetPath, backupError, backupCleanupError) @@ -568,13 +658,13 @@ export async function safeWriteText( const restoredDir = path.dirname(targetPath) 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.`) + // The content is committed, but the published file answers to a different + // DACL from the one that was saved, and nothing here verified an equivalent + // restrictive ACL on the replacement. This is reported as an error rather than a + // warning: the caller has to know that the save changed who can read the file. + // Contract change - this used to warn and resolve, which let a publish that + // widened access look like an ordinary successful save. + throw new DaclRestoreError(targetPath, null) } } @@ -587,29 +677,27 @@ export async function safeWriteText( // 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.`, - ) - } - } + const cleanupError = await _removeBackupCopy(backupPath) + if (cleanupError !== null) { + // The publish itself succeeded, so this is a leftover to clean up rather than a + // failed save: the path reaches the human through onWarning and the caller through + // the structured result, because a full copy of the previous content is still + // sitting beside the target. + leftoverPaths.push(backupPath) + 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. if (daclDumpPath !== null) { - await fs.unlink(daclDumpPath).catch(() => {}) + const dumpPath = daclDumpPath + await fs.unlink(dumpPath).catch(() => { + leftoverPaths.push(dumpPath) + }) } } @@ -619,9 +707,11 @@ export async function safeWriteText( // 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(() => {}) - } + await _removeOwnStagingDir(stagingDir) + + // The result is reported only after this call's own residue has been dealt with: a caller + // that receives an empty list knows there is nothing left for it to sweep. + return { leftoverPaths } } catch (originalError: unknown) { // The backup is a copy, never a restore source: whether the failure happened before // or after the commit rename, the copy is removed below so no stale duplicate of the @@ -631,7 +721,18 @@ export async function safeWriteText( // the commit left there - before the commit that is the pre-write content, and // after it the published content. Either way the copy has served its purpose // and must not be left beside the target where no caller can find it. - await fs.unlink(backupPath).catch(() => {}) + // One retry, ENOENT counts as removed - the same rule as the other two backup sites. + const stuckBackup = await _removeBackupCopy(backupPath) + if (stuckBackup !== null) { + // The original error is what the caller needs, so the leftover cannot be thrown; it is + // reported with its path instead of dropped, through onWarning and the result. + leftoverPaths.push(backupPath) + warn( + `safeWriteText: the write failed and its backup copy could not be removed from ${backupPath} (${ + errorCode(stuckBackup) ?? "unknown error" + }); a full copy of the previous content is still on disk and needs to be removed.`, + ) + } backupPath = null } try { From 2231cadb2a549dad4dd6815d1dd8caf81f4aac62 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 10 Oct 2026 23:16:56 +0800 Subject: [PATCH 34/37] test(safe-write-text): pin the refusal's phase and path, and move two stray doc blocks Two review rows on this unit, both real at this head. Weak refusal assertions. The DACL-save and access-check tests checked only `toBeInstanceOf(DaclInspectionError)`. Swapping the phase argument at either throw site, handing the wrong path to the error, or swapping the two message branches all left both tests green - which is what the mutation advisory was pointing at on the error's constructor. Each test now captures the rejection once (calling safeWriteText twice would double-count the single icacls attempt asserted beside it) and asserts name, phase, targetPath and the phase-specific message. Five mutants, one per production call site plus the message ternary: each is killed by exactly the test that names that behaviour (1, 1, 1, 1 and 2 tests). The same mutants against the pre-fix spec leave all 66 tests green, so these assertions are what closes them. Stale and misplaced doc blocks. onWarning promised that an uncaptured Windows DACL lets the write proceed; this head refuses the publish with DaclInspectionError before the commit and reports a failed restore as DaclRestoreError, so neither reaches the sink - the text now describes the leftover notices that still do. The OrphanedBackupError block sat above DaclInspectionError and the _removeBackupCopy block above _removeOwnStagingDir; each now sits above its own declaration. The production file is comment-only in this commit: removing block and pure comment lines and folding whitespace leaves 10820 bytes on both sides, byte-identical. 66 tests pass in the mocked spec. eslint --max-warnings=0 exits 0 on both files. Prettier reports no new deviation from my lines; the pre-existing drift in these two files is left alone rather than swept into this commit. --- .../__tests__/safeWriteText.spec.ts | 36 ++++++++++++++++--- src/services/file-safety/safeWriteText.ts | 34 +++++++++--------- 2 files changed, 50 insertions(+), 20 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 3ecbcf4251..31c5fa5201 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -754,7 +754,22 @@ describe("safeWriteText", () => { return fakeChild }) - await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf(DaclInspectionError) + // Captured once: calling safeWriteText twice here would double-count the single icacls + // attempt asserted below. + const refusal = await safeWriteText(targetPath, "data", { platform: "win32" }).catch( + (error: unknown) => error, + ) + + expect(refusal).toBeInstanceOf(DaclInspectionError) + // The class alone does not pin the contract: swapping the phase or dropping the target path + // still satisfies toBeInstanceOf, and both are caller-visible - the phase says which check + // refused, the path says which file the caller must not assume was saved. + expect(refusal).toMatchObject({ + name: "DaclInspectionError", + phase: "save", + targetPath, + message: expect.stringContaining("its DACL could not be saved"), + }) // Contract change (Security Boundaries row): a target whose DACL could not be saved is // no longer replaced by a file that inherits different rights. Nothing is committed. @@ -796,9 +811,22 @@ describe("safeWriteText", () => { }) const warnings: string[] = [] - await expect( - safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }), - ).rejects.toBeInstanceOf(DaclInspectionError) + // Captured once: a second call would double-count the assertions below. + const refusal = await safeWriteText(targetPath, "data", { + platform: "win32", + onWarning: (m) => warnings.push(m), + }).catch((error: unknown) => error) + + expect(refusal).toBeInstanceOf(DaclInspectionError) + // The class alone does not pin the contract: swapping the phase or dropping the target path + // still satisfies toBeInstanceOf, and both are caller-visible - the phase says which check + // refused, the path says which file the caller must not assume was saved. + expect(refusal).toMatchObject({ + name: "DaclInspectionError", + phase: "inspect", + targetPath, + message: expect.stringContaining("its access rights could not be checked"), + }) // Contract change: "there but unreadable" is no longer a reason to publish blind. The // publish is refused before any icacls runs, and nothing is warned about a write that did diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index caf5368fed..42cb3fab67 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -28,10 +28,12 @@ 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. + * Sink for non-fatal safety notices. The notices that still reach it are the leftover ones: a copy + * of the previous content this write could not remove, reported with its path whether the + * publish committed or failed. A Windows DACL that could not be captured is no longer a notice + * here - the publish is refused with DaclInspectionError before anything is committed - and a + * saved DACL that could not be put back after the commit arrives as DaclRestoreError rather + * than a warning, so neither reaches this sink. Defaults to console.warn. */ onWarning?: (message: string) => void @@ -79,12 +81,6 @@ export class PostCommitDurabilityError extends Error { } } -/** - * 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. - */ /** * The target exists but its access rights could not be inspected or saved, so publishing would * replace it with a file that inherits different rights. On Windows the publish fails instead of @@ -128,6 +124,12 @@ export interface SafeWriteTextResult { leftoverPaths: string[] } +/** + * 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 @@ -297,12 +299,6 @@ export async function resolveLockKey(absoluteFilePath: string): Promise } } -/** - * Remove a backup copy, retrying once: Windows reports EPERM for a file whose handle has not been - * released yet, so a single failure is not evidence that the path is stuck. ENOENT counts as - * removed - the goal is that the path is gone, not that this call performed the removal. Returns - * the error that kept the path on disk, or null when it is gone. - */ /** * Remove this write's own staging directory once its temp file is gone. Best-effort by design: a * failure must not un-commit a published file. The directory is empty and per-write at this point, @@ -317,6 +313,12 @@ async function _removeOwnStagingDir(stagingDir: string | null): Promise { await fs.rmdir(stagingDir).catch(() => {}) } +/** + * Remove a backup copy, retrying once: Windows reports EPERM for a file whose handle has not been + * released yet, so a single failure is not evidence that the path is stuck. ENOENT counts as + * removed - the goal is that the path is gone, not that this call performed the removal. Returns + * the error that kept the path on disk, or null when it is gone. + */ async function _removeBackupCopy(backupPath: string): Promise { let lastError: unknown = null for (let attempt = 0; attempt < 2; attempt++) { From d3cf409d205db27daa5e90438c66329f1f9b20d0 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 10 Oct 2026 23:36:54 +0800 Subject: [PATCH 35/37] refactor(safe-write-text): drop the duplicate pre-commit cleanup and correct its claim Two more review rows on this unit. The DACL refusals were said to sit out of reach of the failure handler, so a helper cleaned up the staged file and this write's staging directory before each throw. They do not: both throws are inside the try whose catch at the bottom of safeWriteText already unlinks the staged file and removes the staging directory before rethrowing, so every refused publish ran that cleanup twice - the second unlink and rmdir only found ENOENT, and both were swallowed. The helper is gone and the two comments now name the handler that actually does the work. The pushes beside those throws were dead for the same reason: leftoverPaths reaches a caller only on a successful return, and each push was followed by a throw. The catch's push had the same shape (that catch ends in a throw) and is gone too; the notice still reaches the human through onWarning. The finally's push stays - on the success path it is what puts a dump this write could not unlink into the result. A refused publish now has a test that counts the matching calls instead of matching them: toHaveBeenCalledWith passes however many times the same path is passed, so it cannot see a cleanup that runs twice. Against the pre-fix file that test fails with "expected 2 to have a length of 1"; after the change it passes, and deleting either side of the catch's cleanup reddens it (with 8 and 4 tests red, the shared failure-path assertions included). The integration test's comment still described a failed icacls restore as reported rather than thrown. This head throws DaclRestoreError after the commit rename, so the comment now says the case is the successful restore and points at the focused unit test for the failure path; that file is comment-only - removing comment lines and folding whitespace leaves 1080 bytes on both sides, byte-identical. 67 tests pass in the mocked spec and 21 in safeWriteJson, its caller. The integration spec's single failure is this machine's Windows DACL restore and is identical before and after this commit. eslint --max-warnings=0 exits 0 on all three files and no new prettier deviation was introduced. --- .../safeWriteText.integration.spec.ts | 10 ++++-- .../__tests__/safeWriteText.spec.ts | 21 ++++++++++++ src/services/file-safety/safeWriteText.ts | 33 +++++-------------- 3 files changed, 36 insertions(+), 28 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts index dd4995d355..d0730d1254 100644 --- a/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts @@ -26,9 +26,13 @@ describe("safeWriteText against a real filesystem", () => { const targetPath = path.join(dir, "target.txt") await fs.writeFile(targetPath, "old bytes") - // No platform override: the real platform's own durability and ACL steps run. - // A failed icacls restore in a throwaway temp directory is reported, not thrown, - // so the publish still lands. + // No platform override: the real platform's own durability and ACL steps run, and this + // case is the successful restore. A failed restore now throws DaclRestoreError after the + // commit rename has already happened (the focused unit test "win32 DACL: a failed restore + // is an error and the dump is still unlinked" covers that path), so on a machine where + // icacls cannot put a saved ACL back into a throwaway temp directory this publish + // reports that error instead of resolving, and the residue assertions below are reached + // only where the restore worked. await safeWriteText(targetPath, "new bytes", { backup: true }) expect(await fs.readFile(targetPath, "utf8")).toBe("new bytes") diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 31c5fa5201..77820031f7 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -836,6 +836,27 @@ describe("safeWriteText", () => { expect(warnings).toHaveLength(0) }) + // The DACL refusals sit inside the try whose catch performs the cleanup, so a refused publish is + // cleaned up by that handler and by nothing else. Counted, not matched with toHaveBeenCalledWith: + // that matcher passes however many times the same path is passed, so it cannot see a cleanup + // that runs twice. + it("win32: a refused publish removes its staged file and staging directory exactly once", 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" })).rejects.toBeInstanceOf(DaclInspectionError) + + const unlinkCalls = vi.mocked(fs.unlink).mock.calls.map((call) => String(call[0])) + const rmdirCalls = vi.mocked(fs.rmdir).mock.calls.map((call) => String(call[0])) + expect(unlinkCalls.filter((p) => p.includes("safeWriteText_"))).toHaveLength(1) + expect(rmdirCalls.filter((p) => p.includes(".file-safety-staging"))).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" diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 42cb3fab67..6c71be5e1f 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -515,23 +515,6 @@ export async function safeWriteText( // -- Step 2 (win32): save DACL BEFORE the backup copy ----------- const platform = options?.platform ?? process.platform - // A refusal before the commit has to remove what this call already made: the failure - // handler below is out of reach from here, and a staged file stranded beside the target is - // residue the caller should not have to discover on its own. - const _cleanupBeforeCommit = async (ownDir: string | null): Promise => { - const stuck: string[] = [] - const staged = tempPath - await fs.unlink(staged).catch(() => { - stuck.push(staged) - }) - if (ownDir) { - await fs.rmdir(ownDir).catch(() => { - stuck.push(ownDir) - }) - } - return stuck - } - if (platform === "win32") { let accessError: unknown = null try { @@ -556,16 +539,16 @@ export async function safeWriteText( // can verify an equivalent restrictive ACL on the replacement, so the publish is // refused instead of warned about: a save that silently changed who can read the // file is not a save the user can trust. Nothing is committed yet, so the target - // still holds its content; the staged file this call already made is removed by - // the cleanup below, because the failure handler further down is out of reach. - leftoverPaths.push(...(await _cleanupBeforeCommit(stagingDir))) + // still holds its content, and this throw lands in the catch at the bottom of this + // try - the one place that unlinks the staged file and removes this write's own + // staging directory before rethrowing - so a refused publish strands no residue. throw new DaclInspectionError(targetPath, "save", null) } } else if (errorCode(accessError) !== "ENOENT") { // Not "absent": the target is there but its access rights could not be read (EACCES, // ...), so publishing would replace a file whose rights this call never learned. Same - // rule as the save failure above: refuse before anything is committed. - leftoverPaths.push(...(await _cleanupBeforeCommit(stagingDir))) + // rule as the save failure above: refuse before anything is committed, and let the + // catch at the bottom of this try remove the staged file and the staging directory. throw new DaclInspectionError(targetPath, "inspect", accessError) } } @@ -726,9 +709,9 @@ export async function safeWriteText( // One retry, ENOENT counts as removed - the same rule as the other two backup sites. const stuckBackup = await _removeBackupCopy(backupPath) if (stuckBackup !== null) { - // The original error is what the caller needs, so the leftover cannot be thrown; it is - // reported with its path instead of dropped, through onWarning and the result. - leftoverPaths.push(backupPath) + // The original error is what the caller needs, so the leftover cannot be thrown, and a + // throw means the structured result never reaches the caller either: the path is + // reported with its location through onWarning instead of being dropped. warn( `safeWriteText: the write failed and its backup copy could not be removed from ${backupPath} (${ errorCode(stuckBackup) ?? "unknown error" From a093a7883bb8500b6e5cdf994879b5a22bea2086 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sun, 11 Oct 2026 00:41:48 +0800 Subject: [PATCH 36/37] style(safe-write): run prettier over the two file-safety files main added a "Check formatting" step (`pnpm format:check`, i.e. `prettier --check .`) to the compile job, so the gate reached this branch with the refreshed merge base and ran here for the first time. It named exactly two files in the repository, both on this unit: safeWriteText.ts and its spec. They were already off prettier at the previous head - checking that head's own blobs reports the same two files - because the commits that introduced them bypassed the hook and nothing on main carried them until now. Formatting only. Both sides parsed with the repository's own TypeScript: the token stream with punctuation excluded is identical (932 tokens, hash 551ede173f655574, and 4335 tokens, hash 05f7b62e29a152d5) and the comment texts are identical (199 and 277 entries). What moved is line breaks, indentation, and the parens and trailing commas prettier adds or removes. `prettier --check` from the repository root now reports "All matched files use Prettier code style" for all three file-safety files. eslint --max-warnings=0 exits 0 on each. 89 tests pass, with the one local Windows DACL-restore failure unchanged, and the phase mutant on the DACL-save refusal is still killed by the save-phase test, so the assertions still bite after the rewrap. --- .../__tests__/safeWriteText.spec.ts | 252 ++++++++++-------- src/services/file-safety/safeWriteText.ts | 18 +- 2 files changed, 145 insertions(+), 125 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 77820031f7..e61f368f94 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -209,7 +209,9 @@ describe("safeWriteText", () => { 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.toEqual({ leftoverPaths: [] }) + await expect(safeWriteText(targetPath, "hello", { platform: "linux" })).resolves.toEqual({ + leftoverPaths: [], + }) // the commit rename still happened and the rmdir error was swallowed expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) @@ -320,9 +322,7 @@ describe("safeWriteText", () => { 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"), - ) + expect(consoleWarn).toHaveBeenCalledWith(expect.stringContaining("onWarning callback rejected")) consoleWarn.mockRestore() }) @@ -415,7 +415,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, and the backup was only // ever a copy: the target was never moved, so there is nothing to rename back. @@ -490,7 +492,9 @@ describe("safeWriteText", () => { }) expect(seedOpen).toBeDefined() expect(seedOpen?.[2]).toBe(0o600) - const seedOrder = vi.mocked(fsSync.openSync).mock.invocationCallOrder[vi.mocked(fsSync.openSync).mock.calls.indexOf(seedOpen!)] + 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 @@ -516,14 +520,18 @@ describe("safeWriteText", () => { // 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.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") } }) - await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow("EIO") + 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. @@ -699,7 +707,8 @@ describe("safeWriteText", () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) // The seed open is the only "wx" open; give its descriptor a distinguishable fd. - vi.mocked(fsSync.openSync).mockImplementation(((p: fsSync.PathLike, flags?: fsSync.OpenMode) => (flags === "wx" ? 42 : 1)) as typeof fsSync.openSync) + vi.mocked(fsSync.openSync).mockImplementation(((p: fsSync.PathLike, flags?: fsSync.OpenMode) => + flags === "wx" ? 42 : 1) as typeof fsSync.openSync) let seedCloseAttempts = 0 vi.mocked(fsSync.closeSync).mockImplementation((fd: number) => { if (fd === 42) { @@ -709,7 +718,9 @@ describe("safeWriteText", () => { return undefined }) - await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow("close failed") + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow( + "close failed", + ) // Exactly one close attempt: POSIX close(2) may have released the descriptor before it // reported the error, so a retry could release a descriptor another operation reused. @@ -778,117 +789,119 @@ describe("safeWriteText", () => { expect(execFile).toHaveBeenCalledTimes(1) }) - it("win32: refuses the publish without an onWarning notice 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 + it("win32: refuses the publish without an onWarning notice 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 expect( + safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }), + ).rejects.toBeInstanceOf(DaclInspectionError) + + // Contract change: the refusal replaces the warning. There is no replacement whose access + // rights could have changed, so no notice is emitted for a write that did not happen. + expect(fs.rename).not.toHaveBeenCalled() + expect(warnings).toHaveLength(0) }) - const warnings: string[] = [] - await expect( - safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }), - ).rejects.toBeInstanceOf(DaclInspectionError) + it("win32: refuses the publish 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[] = [] - // Contract change: the refusal replaces the warning. There is no replacement whose access - // rights could have changed, so no notice is emitted for a write that did not happen. - expect(fs.rename).not.toHaveBeenCalled() - expect(warnings).toHaveLength(0) - }) + // Captured once: a second call would double-count the assertions below. + const refusal = await safeWriteText(targetPath, "data", { + platform: "win32", + onWarning: (m) => warnings.push(m), + }).catch((error: unknown) => error) - it("win32: refuses the publish 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[] = [] - - // Captured once: a second call would double-count the assertions below. - const refusal = await safeWriteText(targetPath, "data", { - platform: "win32", - onWarning: (m) => warnings.push(m), - }).catch((error: unknown) => error) - - expect(refusal).toBeInstanceOf(DaclInspectionError) - // The class alone does not pin the contract: swapping the phase or dropping the target path - // still satisfies toBeInstanceOf, and both are caller-visible - the phase says which check - // refused, the path says which file the caller must not assume was saved. - expect(refusal).toMatchObject({ - name: "DaclInspectionError", - phase: "inspect", - targetPath, - message: expect.stringContaining("its access rights could not be checked"), + expect(refusal).toBeInstanceOf(DaclInspectionError) + // The class alone does not pin the contract: swapping the phase or dropping the target path + // still satisfies toBeInstanceOf, and both are caller-visible - the phase says which check + // refused, the path says which file the caller must not assume was saved. + expect(refusal).toMatchObject({ + name: "DaclInspectionError", + phase: "inspect", + targetPath, + message: expect.stringContaining("its access rights could not be checked"), + }) + + // Contract change: "there but unreadable" is no longer a reason to publish blind. The + // publish is refused before any icacls runs, and nothing is warned about a write that did + // not happen. + expect(execFile).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + expect(warnings).toHaveLength(0) }) - // Contract change: "there but unreadable" is no longer a reason to publish blind. The - // publish is refused before any icacls runs, and nothing is warned about a write that did - // not happen. - expect(execFile).not.toHaveBeenCalled() - expect(fs.rename).not.toHaveBeenCalled() - expect(warnings).toHaveLength(0) - }) + // The DACL refusals sit inside the try whose catch performs the cleanup, so a refused publish is + // cleaned up by that handler and by nothing else. Counted, not matched with toHaveBeenCalledWith: + // that matcher passes however many times the same path is passed, so it cannot see a cleanup + // that runs twice. + it("win32: a refused publish removes its staged file and staging directory exactly once", 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 + }) - // The DACL refusals sit inside the try whose catch performs the cleanup, so a refused publish is - // cleaned up by that handler and by nothing else. Counted, not matched with toHaveBeenCalledWith: - // that matcher passes however many times the same path is passed, so it cannot see a cleanup - // that runs twice. - it("win32: a refused publish removes its staged file and staging directory exactly once", 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" })).rejects.toBeInstanceOf( + DaclInspectionError, + ) + + const unlinkCalls = vi.mocked(fs.unlink).mock.calls.map((call) => String(call[0])) + const rmdirCalls = vi.mocked(fs.rmdir).mock.calls.map((call) => String(call[0])) + expect(unlinkCalls.filter((p) => p.includes("safeWriteText_"))).toHaveLength(1) + expect(rmdirCalls.filter((p) => p.includes(".file-safety-staging"))).toHaveLength(1) }) - await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf(DaclInspectionError) + // 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) + // The DACL save succeeds here: the warning this test exercises has to come from a notice + // that still exists, and the DACL-save notice became a refusal. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(null, "", "") + return fakeChild + }) + // The surviving notice is the leftover one: the publish committed and the copy of the + // previous content could not be removed (Windows reports EPERM while a handle is open). + vi.mocked(fs.unlink).mockImplementation(async (p) => { + if (String(p).includes("safeWriteText.bak_")) { + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + } + }) - const unlinkCalls = vi.mocked(fs.unlink).mock.calls.map((call) => String(call[0])) - const rmdirCalls = vi.mocked(fs.rmdir).mock.calls.map((call) => String(call[0])) - expect(unlinkCalls.filter((p) => p.includes("safeWriteText_"))).toHaveLength(1) - expect(rmdirCalls.filter((p) => p.includes(".file-safety-staging"))).toHaveLength(1) - }) + await expect( + safeWriteText(targetPath, "data", { + backup: true, + platform: "win32", + onWarning: () => { + throw new Error("callback down") + }, + }), + ).resolves.toEqual({ leftoverPaths: [expect.stringContaining("safeWriteText.bak_")] }) - // 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) - // The DACL save succeeds here: the warning this test exercises has to come from a notice - // that still exists, and the DACL-save notice became a refusal. - vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { - if (typeof cb === "function") cb(null, "", "") - return fakeChild - }) - // The surviving notice is the leftover one: the publish committed and the copy of the - // previous content could not be removed (Windows reports EPERM while a handle is open). - vi.mocked(fs.unlink).mockImplementation(async (p) => { - if (String(p).includes("safeWriteText.bak_")) { - throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) - } + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) }) - await expect( - safeWriteText(targetPath, "data", { - backup: true, - platform: "win32", - onWarning: () => { - throw new Error("callback down") - }, - }), - ).resolves.toEqual({ leftoverPaths: [expect.stringContaining("safeWriteText.bak_")] }) - - 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) @@ -901,7 +914,9 @@ describe("safeWriteText", () => { return fakeChild }) - await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf(DaclInspectionError) + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf( + DaclInspectionError, + ) // Contract change: the publish is refused, so nothing is renamed. Only the save was // attempted, and the partial dump is still removed below. @@ -980,7 +995,9 @@ describe("safeWriteText", () => { return fakeChild }) - await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf(DaclRestoreError) + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf( + DaclRestoreError, + ) // The content did commit before the restore failed, so the rename happened exactly once // even though the publish reports an error. @@ -1458,7 +1475,10 @@ describe("caller-supplied staging path", () => { // symlink nor a regular file. Renaming it over the target would publish a directory in // place of the file, so the rejection has to name the file type and happen before anything // is opened or renamed. Structural double: BigIntStats has no constructor a test can call. - vi.mocked(fs.lstat).mockResolvedValue({ isSymbolicLink: () => false, isFile: () => false } as unknown as BigIntStats) + vi.mocked(fs.lstat).mockResolvedValue({ + isSymbolicLink: () => false, + isFile: () => false, + } as unknown as BigIntStats) await expect( safeWriteText(targetPath, "data", { tempPath: "/tmp/test-dir/x.tmp", platform: "linux" }), @@ -1477,9 +1497,9 @@ 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) + 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 @@ -1494,7 +1514,6 @@ describe("caller-supplied staging path", () => { expect(fs.unlink).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) @@ -1521,7 +1540,8 @@ describe("caller-supplied staging path", () => { for (const c of identityLookups) { expect(c[1]).toEqual({ bigint: true }) } - })}) + }) +}) describe("cleanup when a backed-up write fails before commit", () => { beforeEach(() => mockDefaults()) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 6c71be5e1f..5313714b37 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -143,12 +143,7 @@ export class OrphanedBackupError extends Error { } } -function _orphanedBackupMessage( - targetPath: string, - backupPath: string, - cause: unknown, - cleanupError: unknown, -): string { +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 ` + @@ -206,7 +201,11 @@ async function _saveDaclWindows(srcPath: string, dumpPath: string, execFileRunne /** Restore a DACL dump onto *dirPath* on Windows. * Returns whether icacls succeeded; the caller reports a failure. */ -async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { +async function _restoreDaclWindows( + dirPath: string, + dumpPath: string, + execFileRunner?: typeof execFile, +): Promise { const runner = execFileRunner ?? execFile try { await new Promise((resolve, reject) => { @@ -351,7 +350,9 @@ export async function safeWriteText( // handler reports a leftover backup copy, so both sides can reach it. const warn = (message: string) => { const report = (label: string, error: unknown) => { - console.warn(`safeWriteText: onWarning callback ${label}: ${error instanceof Error ? error.message : String(error)}`) + console.warn( + `safeWriteText: onWarning callback ${label}: ${error instanceof Error ? error.message : String(error)}`, + ) } try { const sink = options?.onWarning ?? ((m: string) => console.warn(m)) @@ -370,7 +371,6 @@ export async function safeWriteText( } } - // Resolve the symlink referent (see resolvePublishTarget). const targetPath = await resolvePublishTarget(absoluteFilePath) const dirPath = path.dirname(targetPath) From 3989bbfc75e7b07de038023df32700eeb6a65be0 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sun, 11 Oct 2026 06:45:21 +0800 Subject: [PATCH 37/37] fix(file-safety): report residue a publish could not remove on every exit path A leftover was reported only when the publish resolved. A rejection discards the structured result, so a DACL dump left behind by a failed save or a failed restore, a staged file, a staging directory and a stuck backup copy could all stay on disk with no reference the caller could reach. - One residue cleanup path (_removeResidue plus the cleanResidue recorder) now covers the staged file, the staging directory, the DACL dump and the backup copy: one retry, ENOENT counts as removed, and a path that survives is recorded together with the error that kept it there. - The recorded residue travels on the thrown error as PublishResidue when a publish rejects, preserving the original error identity rather than wrapping it, and nothing is attached when the publish cleaned everything up. - The finally block is the dump's only owner, so a surviving dump is reported once instead of being unlinked a second time by the failure handler and dropped. - A staging directory this write created and could not remove is now reported in leftoverPaths, which is what that field's documented contract already promised. --- .../__tests__/safeWriteText.spec.ts | 221 +++++++++++++++++- src/services/file-safety/safeWriteText.ts | 167 +++++++------ 2 files changed, 318 insertions(+), 70 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index e61f368f94..37f1a5234c 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -13,6 +13,7 @@ import { resolveLockKey, safeWriteText, StagingPathError, + type PublishResidue, type SafeWriteTextOptions, } from "../safeWriteText" @@ -209,13 +210,18 @@ describe("safeWriteText", () => { 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.toEqual({ - leftoverPaths: [], - }) + const result = await safeWriteText(targetPath, "hello", { platform: "linux" }) - // the commit rename still happened and the rmdir error was swallowed + // The commit rename still happened: a directory this write created is a leftover to clean + // up, not a failed save. It is reported rather than swallowed, because the caller is the + // only party that can sweep it and an empty list claims there is nothing left to sweep. expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) - expect(fs.rmdir).toHaveBeenCalledTimes(1) + const stagingDir = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => String(call[0]))[0] + expect(result).toEqual({ leftoverPaths: [stagingDir] }) + + // One retry: a directory can still be reported busy by the OS while the handle of the + // file it held is being released, so a single failure is not evidence it is stuck. + expect(fs.rmdir).toHaveBeenCalledTimes(2) expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) }) @@ -577,6 +583,18 @@ describe("safeWriteText", () => { expect(orphan.cause).toBe(orphan.originalError) expect(unlinkAttempts).toBe(2) expect(fs.rename).not.toHaveBeenCalled() + + // The dedicated orphan field names the copy, and the residue lists every path this write + // left on disk: the partial copy first, then the staged file the failure handler could not + // remove either. The throw discards the structured result, so without these the caller + // would have to work out for itself which of its paths are still on disk. + expect(orphan).toMatchObject({ + leftoverPaths: [ + expect.stringContaining("safeWriteText.bak_"), + expect.stringContaining("safeWriteText_"), + ], + cleanupErrors: [expect.objectContaining({ code: "EPERM" }), expect.objectContaining({ code: "EPERM" })], + }) }) it("a failed staging-file flush rejects and publishes nothing", async () => { @@ -1600,3 +1618,196 @@ describe("resolvePublishTarget", () => { expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), path.resolve(targetPath)) }) }) + +// A publish that cannot remove something it created has to say so on whichever exit it takes: the +// resolved result carries the paths, and a rejection carries the same paths on the error, because the +// throw discards the result. Each test below names one residue kind and one exit path. + +describe("residue a publish must not lose", () => { + beforeEach(() => mockDefaults()) + + /** Make only the DACL dump unlink fail, and record every attempt at it. */ + function stuckDumpUnlink(): string[] { + const attempts: string[] = [] + vi.mocked(fs.unlink).mockImplementation(async (p: unknown) => { + if (String(p).includes("safeWriteText.acl")) { + attempts.push(String(p)) + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + } + }) + return attempts + } + + it("win32 DACL: a dump the finally block cannot remove is reported on the resolved write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const dumpAttempts = stuckDumpUnlink() + + const result = await safeWriteText(targetPath, "data", { platform: "win32" }) + + // The publish committed and the restore ran: a dump that survives cleanup is a leftover to + // sweep, not a failed save. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(execFile).toHaveBeenCalledTimes(2) + + // The dump is retried once, and the result names the path once: recording each attempt would + // tell the caller there are two files to remove when there is one. + expect(dumpAttempts).toHaveLength(2) + expect(result).toEqual({ leftoverPaths: [dumpAttempts[0]] }) + }) + + it("win32 DACL: a partial dump that cannot be removed is reported on the refusal", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls /save fails, so a partial dump may exist, and removing it fails too. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + const dumpAttempts = stuckDumpUnlink() + + const refusal = await safeWriteText(targetPath, "data", { platform: "win32" }).catch( + (caught: unknown) => caught, + ) + + expect(refusal).toBeInstanceOf(DaclInspectionError) + expect(fs.rename).not.toHaveBeenCalled() + + // The refusal throws, so the structured result never reaches the caller. Without the residue + // on the error the caller holds an error naming no file while a partial DACL dump - a file + // whose contents describe access rights - stays on disk with no reference to it anywhere. + expect(refusal).toMatchObject({ + leftoverPaths: [dumpAttempts[0]], + cleanupErrors: [expect.objectContaining({ code: "EPERM" })], + }) + }) + + it("win32 DACL: a restore failure carries the dump its cleanup could not remove", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The save lands, the restore fails: the commit happened, so this is the exit where the + // dump's owner is the finally block and the error is DaclRestoreError. + let icaclsCalls = 0 + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + icaclsCalls++ + if (typeof cb === "function") cb(icaclsCalls === 1 ? null : new Error("icacls restore error"), "", "") + return fakeChild + }) + const dumpAttempts = stuckDumpUnlink() + + const failure = await safeWriteText(targetPath, "data", { platform: "win32" }).catch( + (caught: unknown) => caught, + ) + + expect(failure).toBeInstanceOf(DaclRestoreError) + expect(fs.rename).toHaveBeenCalledTimes(1) + // The original error is preserved, not replaced: its message is what tells the caller that + // the committed file answers to different access rights. + expect((failure as DaclRestoreError).message).toContain("could not be restored") + + // Previously the finally block recorded this path and the failure handler threw it away with + // the result, so a dump left by a failed restore was invisible on the only exit taken. + expect(failure).toMatchObject({ leftoverPaths: [dumpAttempts[0]] }) + }) + + it("a failed publish reports the staged file it could not remove", 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")) + const stagedAttempts: string[] = [] + vi.mocked(fs.unlink).mockImplementation(async (p: unknown) => { + if (String(p).includes("safeWriteText_")) { + stagedAttempts.push(String(p)) + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + } + }) + + const failure = await safeWriteText(targetPath, "data", { platform: "linux" }).catch( + (caught: unknown) => caught, + ) + + expect((failure as Error).message).toBe("ENOSPC") + expect(stagedAttempts).toHaveLength(2) + expect(failure).toMatchObject({ leftoverPaths: [stagedAttempts[0]] }) + }) + + it("a failed publish reports the staging directory it could not remove", 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")) + vi.mocked(fs.rmdir).mockRejectedValue(Object.assign(new Error("ENOTEMPTY"), { code: "ENOTEMPTY" })) + + const failure = await safeWriteText(targetPath, "data", { platform: "linux" }).catch( + (caught: unknown) => caught, + ) + + const stagingDir = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => String(call[0]))[0] + expect(failure).toMatchObject({ leftoverPaths: [stagingDir] }) + }) + + it("a failed publish reports a backup copy it could not remove and still warns about it", 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")) + const backupAttempts: string[] = [] + vi.mocked(fs.unlink).mockImplementation(async (p: unknown) => { + if (String(p).includes("safeWriteText.bak_")) { + backupAttempts.push(String(p)) + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + } + }) + const warnings: string[] = [] + + const failure = await safeWriteText(targetPath, "data", { + backup: true, + platform: "linux", + onWarning: (message) => warnings.push(message), + }).catch((caught: unknown) => caught) + + expect((failure as Error).message).toBe("ENOSPC") + // The human notice and the caller-visible list are separate channels: neither replaces the + // other, because a caller cannot parse a warning and a human cannot read a thrown array. + expect(warnings).toHaveLength(1) + expect(warnings[0]).toContain("safeWriteText.bak_") + expect(failure).toMatchObject({ leftoverPaths: [backupAttempts[0]] }) + }) + + it("a publish that cleaned everything up attaches no residue to its error", 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")) + + const failure = await safeWriteText(targetPath, "data", { platform: "linux" }).catch( + (caught: unknown) => caught, + ) + + // An empty list would make every caller ask whether "no leftovers" means it checked or that + // the field was never filled in, so nothing is attached unless something stayed on disk. + expect((failure as PublishResidue).leftoverPaths).toBeUndefined() + expect((failure as PublishResidue).cleanupErrors).toBeUndefined() + }) + + it("a thrown value that cannot carry the residue is rethrown exactly as it came", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // A rejected promise can carry any value, not only an Error. Attaching the residue by + // replacing such a value would change the error the caller catches, and assigning a property + // to a primitive throws, so the failure would become a TypeError about the cleanup. + vi.mocked(fs.rename).mockRejectedValue("boom") + vi.mocked(fs.unlink).mockRejectedValue(Object.assign(new Error("EPERM"), { code: "EPERM" })) + + const failure = await safeWriteText(targetPath, "data", { platform: "linux" }).catch( + (caught: unknown) => caught, + ) + + expect(failure).toBe("boom") + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 5313714b37..17bfd08059 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -124,6 +124,19 @@ export interface SafeWriteTextResult { leftoverPaths: string[] } +/** + * The residue a publish could not remove. A resolved write reports it in `SafeWriteTextResult`; a + * rejected write cannot, because the throw discards that result, so the same two lists travel on the + * error the caller catches instead. Without them a failed publish would leave a file on disk with no + * reference to it anywhere the caller can reach. + */ +export interface PublishResidue { + /** Paths this write left on disk that its own cleanup could not remove. */ + leftoverPaths: string[] + /** The failure that kept each path on disk, in the same order as `leftoverPaths`. */ + cleanupErrors: unknown[] +} + /** * 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 @@ -299,30 +312,18 @@ export async function resolveLockKey(absoluteFilePath: string): Promise } /** - * Remove this write's own staging directory once its temp file is gone. Best-effort by design: a - * failure must not un-commit a published file. The directory is empty and per-write at this point, - * so a later sweep of stale .file-safety-staging_* names can remove it without risking another - * write's file. Takes the directory as a parameter because control-flow narrowing does not survive - * the try/finally boundary above the call site. + * Remove one residue path this write created, retrying once: Windows reports EPERM for a path whose + * handle has not been released yet, so a single failure is not evidence that the path is stuck. + * ENOENT counts as removed - the goal is that the path is gone, not that this call performed the + * removal. `remove` is the filesystem call for this residue kind (`fs.unlink` for a file, `fs.rmdir` + * for a directory), so every residue this operation owns is removed under one set of rules rather + * than one per kind. Returns the error that kept the path on disk, or null when it is gone. */ -async function _removeOwnStagingDir(stagingDir: string | null): Promise { - if (stagingDir === null) { - return - } - await fs.rmdir(stagingDir).catch(() => {}) -} - -/** - * Remove a backup copy, retrying once: Windows reports EPERM for a file whose handle has not been - * released yet, so a single failure is not evidence that the path is stuck. ENOENT counts as - * removed - the goal is that the path is gone, not that this call performed the removal. Returns - * the error that kept the path on disk, or null when it is gone. - */ -async function _removeBackupCopy(backupPath: string): Promise { +async function _removeResidue(residuePath: string, remove: (residuePath: string) => Promise): Promise { let lastError: unknown = null for (let attempt = 0; attempt < 2; attempt++) { try { - await fs.unlink(backupPath) + await remove(residuePath) return null } catch (error: unknown) { if (errorCode(error) === "ENOENT") { @@ -334,6 +335,24 @@ async function _removeBackupCopy(backupPath: string): Promise { return lastError } +/** + * Attach the residue of a failed publish to the error being thrown, preserving that error: the + * caller's `instanceof` checks, its message and its cause all stay exactly as they were, and the + * residue is extra information on top. A publish that cleaned everything up attaches nothing, so a + * caller never has to tell an empty list apart from no residue at all. A thrown value that is not an + * object cannot carry the lists, and replacing it with one that could would change the error the + * caller catches, so it is returned as it came. + */ +function _attachResidue(error: unknown, residue: PublishResidue): unknown { + if (residue.leftoverPaths.length === 0 || typeof error !== "object" || error === null) { + return error + } + const carrier = error as PublishResidue + carrier.leftoverPaths = residue.leftoverPaths + carrier.cleanupErrors = residue.cleanupErrors + return error +} + export async function safeWriteText( filePath: string, content: string | Uint8Array, @@ -341,8 +360,27 @@ export async function safeWriteText( ): Promise { const absoluteFilePath = path.resolve(filePath) // Every leftover is recorded here as it is discovered, so a caller gets a structured result - // instead of having to parse warnings to learn that content is still on disk. + // instead of having to parse warnings to learn that content is still on disk. A rejection cannot + // carry that result, so the same two lists are attached to the error it throws instead. const leftoverPaths: string[] = [] + const cleanupErrors: unknown[] = [] + + /** + * Remove one residue path this write created and record it when it is still there. Every residue + * this operation owns - staged file, staging directory, DACL dump, backup copy - is removed + * through here, so a path that survives is reported the same way whichever step left it behind. + */ + const cleanResidue = async ( + residuePath: string, + remove: (residuePath: string) => Promise, + ): Promise => { + const cleanupError = await _removeResidue(residuePath, remove) + if (cleanupError !== null) { + leftoverPaths.push(residuePath) + cleanupErrors.push(cleanupError) + } + return cleanupError + } // 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 @@ -530,10 +568,11 @@ 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. A dump that cannot be removed is + // recorded, because the refusal below throws: without the record the caller would hold + // an error naming no file while a partial DACL dump stayed on disk. + await cleanResidue(dumpPath, fs.unlink) // The target exists and its DACL could not be captured, so the commit rename // would replace it with a file that inherits different access rights. Nothing here // can verify an equivalent restrictive ACL on the replacement, so the publish is @@ -597,8 +636,9 @@ export async function safeWriteText( // 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 - // Same rule as the other two backup sites: one retry, ENOENT counts as removed. - const backupCleanupError = await _removeBackupCopy(orphanPath) + // Same rule as every other residue: one retry, ENOENT counts as removed, and a copy + // that stays is recorded so the error thrown below carries it too. + const backupCleanupError = await cleanResidue(orphanPath, fs.unlink) backupPath = null if (backupCleanupError !== null) { throw new OrphanedBackupError(orphanPath, targetPath, backupError, backupCleanupError) @@ -655,20 +695,14 @@ export async function safeWriteText( // -- Step 6 (backup:true): delete backup on success ----------- if (releaseBackupOnSuccess && backupPath) { - // 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. - const cleanupError = await _removeBackupCopy(backupPath) + // The backup is a full copy of the previous content sitting next to the published file. + // A failed unlink goes through the shared residue path, which retries once (Windows + // commonly reports EPERM while another handle is still being released) and records a path + // that stays. The publish itself succeeded, so this is a leftover to clean up rather than + // a failed save: the path reaches the human through onWarning and the caller through the + // structured result, because a full copy of the previous content is still beside the target. + const cleanupError = await cleanResidue(backupPath, fs.unlink) if (cleanupError !== null) { - // The publish itself succeeded, so this is a leftover to clean up rather than a - // failed save: the path reaches the human through onWarning and the caller through - // the structured result, because a full copy of the previous content is still - // sitting beside the target. - leftoverPaths.push(backupPath) warn( `safeWriteText: committed ${targetPath} but could not remove its backup copy at ${backupPath} (${ errorCode(cleanupError) ?? "unknown error" @@ -677,22 +711,25 @@ export async function safeWriteText( } } } finally { - // Unlink DACL dump regardless of success/failure in this span. + // Unlink the DACL dump on every exit from this span, success or failure, and record one that + // stays. This is the dump's only owner once a save succeeded, so what is recorded here is + // what the caller receives on either exit: a resolved write returns the list, and a rejected + // one carries the same list on its error instead of dropping it with the discarded result. if (daclDumpPath !== null) { - const dumpPath = daclDumpPath - await fs.unlink(dumpPath).catch(() => { - leftoverPaths.push(dumpPath) - }) + await cleanResidue(daclDumpPath, fs.unlink) } } // 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. - await _removeOwnStagingDir(stagingDir) + // Remove the now-empty staging directory through the same cleanup path as every other residue. + // 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 not un-commit a published + // file, but a directory this write created and could not remove is residue the caller is told + // about rather than a failure that disappears. + if (stagingDir !== null) { + await cleanResidue(stagingDir, fs.rmdir) + } // The result is reported only after this call's own residue has been dealt with: a caller // that receives an empty list knows there is nothing left for it to sweep. @@ -706,12 +743,12 @@ export async function safeWriteText( // the commit left there - before the commit that is the pre-write content, and // after it the published content. Either way the copy has served its purpose // and must not be left beside the target where no caller can find it. - // One retry, ENOENT counts as removed - the same rule as the other two backup sites. - const stuckBackup = await _removeBackupCopy(backupPath) + const stuckBackup = await cleanResidue(backupPath, fs.unlink) if (stuckBackup !== null) { // The original error is what the caller needs, so the leftover cannot be thrown, and a // throw means the structured result never reaches the caller either: the path is - // reported with its location through onWarning instead of being dropped. + // reported with its location through onWarning, and attached to the error below, so + // neither the human nor the caller has to parse a message to find it. warn( `safeWriteText: the write failed and its backup copy could not be removed from ${backupPath} (${ errorCode(stuckBackup) ?? "unknown error" @@ -720,23 +757,23 @@ export async function safeWriteText( } backupPath = null } - try { - await fs.unlink(tempPath).catch(() => {}) - } catch { - // cleanup failure is non-fatal - } + + // The staged file and this write's own staging directory go through the same cleanup path as + // every other residue, so a failure at either is recorded and reported on the error below + // instead of being swallowed here: a caller that catches a throw has no result to read. + await cleanResidue(tempPath, fs.unlink) // 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(() => {}) + if (stagingDir !== null) { + await cleanResidue(stagingDir, fs.rmdir) } - throw originalError + // The DACL dump is not removed here. It is non-null only once step 2 saved a dump, and every + // path from that point enters the try whose finally owns the dump, so by the time this handler + // runs the dump has already been removed or recorded. Removing it again here would compensate + // for a resource this span no longer owns and would report a surviving dump twice. + throw _attachResidue(originalError, { leftoverPaths, cleanupErrors }) } }