From 3edac64036eb6666aa177c7ec502c36d2a7379fb Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Mon, 9 Nov 2026 20:02:04 +0800 Subject: [PATCH 1/2] unit 1 of the #1914 split (6 files, whole-file module cut) --- .../safeWriteText.integration.spec.ts | 67 + .../__tests__/safeWriteText.spec.ts | 1676 +++++++++++++++++ src/services/file-safety/safeWriteText.ts | 865 +++++++++ .../__tests__/fileLock.canonicalKey.spec.ts | 67 + src/utils/__tests__/fileLock.spec.ts | 169 ++ src/utils/fileLock.ts | 98 +- 6 files changed, 2936 insertions(+), 6 deletions(-) create mode 100644 src/services/file-safety/__tests__/safeWriteText.integration.spec.ts create mode 100644 src/services/file-safety/__tests__/safeWriteText.spec.ts create mode 100644 src/services/file-safety/safeWriteText.ts create mode 100644 src/utils/__tests__/fileLock.canonicalKey.spec.ts create mode 100644 src/utils/__tests__/fileLock.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..4fc5bb2647 --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts @@ -0,0 +1,67 @@ +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") + + // No platform override: the real platform's own durability and ACL steps run, and this + // case is the successful restore. A failed restore is best-effort: it is logged and + // the publish still resolves after the commit rename, so the residue assertions + // below hold whichever way icacls went on the machine running the test. + 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"]) + }) + + it("cleans up the staging file when the commit rename itself fails", async () => { + // With backup:false the backup step is skipped, so this is the only shape of + // this scenario that reaches Step 4: the rename of the staging FILE over the + // target DIRECTORY fails (EISDIR / ENOTDIR / EPERM depending on platform), and + // the cleanup must remove the staging file. The backup:true case above bails + // out in the backup step and never exercises the commit-failure path. + 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: false })).rejects.toThrow() + + 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 new file mode 100644 index 0000000000..14b9c9ac9d --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,1676 @@ +import * as fs from "fs/promises" +import type { BigIntStats } from "fs" +import * as fsSync from "fs" +import { execFile } from "child_process" +import type { ChildProcess } from "child_process" +import * as path from "path" + +import { + PostCommitDurabilityError, + resolveLockKey, + safeWriteText, + StagingPathError, + TargetExistsError, + UnsupportedFilesystemError, + TargetMovedError, + AncestorReplacedError, + OrphanedBackupError, + OrphanedPublishedBackupError, +} 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(), + link: vi.fn(), + copyFile: vi.fn(), + chmod: vi.fn(), + unlink: vi.fn(), + rmdir: vi.fn(), + realpath: vi.fn(), + stat: vi.fn(), + lstat: vi.fn(), + readlink: 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", () => ({ + // Mirrors Node's value; the no-replace fallback passes it to copyFile. + constants: { COPYFILE_EXCL: 1 }, + 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. +// Async lstat stand-in: the SUT only asks whether the path is a link or a file. +// 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 { + 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.link).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)) + // Ancestor-identity pin: default to one stable directory identity so the pin + // passes unless a test overrides it. + vi.mocked(fs.stat).mockResolvedValue(_dirIdentity(100n)) +} + +// BigIntStats stand-in for the ancestor pin: the SUT reads only dev/ino off it. +// BigIntStats is class-backed with no public constructor, so this is a +// last-resort double assertion (test-local, per AGENTS.md). +function _dirIdentity(ino: bigint): BigIntStats { + return { dev: 1n, ino } as unknown as BigIntStats +} +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(() => { + 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. + 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("safeWriteText.acl")) + } + }) + + 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() + }) + + it("rejects a supplied staging path that names the publish target", async () => { + // resolvePublishTarget realpaths the target (identity here) and compares + // the supplied staging path against that resolved target, so both sides + // must use this platform's path module rather than a posix literal. + vi.mocked(fs.realpath).mockImplementation(async (p) => p.toString()) + const targetPath = path.resolve("/tmp/test-dir/target.txt") + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await expect( + safeWriteText(targetPath, "hello", { platform: "linux", tempPath: targetPath }), + ).rejects.toThrow(StagingPathError) + + // Rejected before staging touches anything: the target file stays the + // target's own, never a staging candidate that could rename onto itself. + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).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; 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_")) + + // 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("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) + + await safeWriteText(targetPath, "new data", { backup: true }) + + // target was accessed (exists check) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // 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_")) + + // the only rename is the atomic commit temp -> target + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // backup copy was deleted on success + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + 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) + // 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") + + // 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_")) + + // 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("retries the post-commit backup removal and succeeds when the retry clears", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const backupUnlinks: string[] = [] + vi.mocked(fs.unlink).mockImplementation(async (candidate: unknown) => { + const asString = String(candidate) + if (asString.includes("safeWriteText.bak_")) { + backupUnlinks.push(asString) + // The first attempt hits a transient Windows handle; the retry succeeds. + if (backupUnlinks.length === 1) { + throw Object.assign(new Error("EBUSY"), { code: "EBUSY" }) + } + } + }) + + await safeWriteText(targetPath, "new data", { backup: true }) + + expect(backupUnlinks).toHaveLength(2) + }) + + it("surfaces an orphaned pre-write copy instead of returning success", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.unlink).mockImplementation(async (candidate: unknown) => { + if (String(candidate).includes("safeWriteText.bak_")) { + throw Object.assign(new Error("EPERM"), { code: "EPERM" }) + } + }) + + const error = await safeWriteText(targetPath, "new data", { backup: true }).catch( + (caught: unknown) => caught, + ) + + expect(error).toBeInstanceOf(OrphanedPublishedBackupError) + const orphan = error as OrphanedPublishedBackupError + expect(orphan.orphanedBackupPath).toContain("safeWriteText.bak_") + expect(orphan.publishedPath).toBe(targetPath) + // Two attempts, then the failure is reported instead of swallowed. + expect( + vi.mocked(fs.unlink).mock.calls.filter((call) => String(call[0]).includes("safeWriteText.bak_")), + ).toHaveLength(2) + }) + + 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 }) + + // The destination must exist with a private mode before copyFile writes anything + // into it: copyFile chooses the destination mode itself, so a restrictive target + // could otherwise leave a group/world-readable copy that a later chmod cannot + // undo. "wx" also means a pre-existing path is never silently reused. + const seedOpen = vi.mocked(fsSync.openSync).mock.calls.find(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "wx" + }) + expect(seedOpen).toBeDefined() + expect(seedOpen?.[2]).toBe(0o600) + const seedOrder = vi.mocked(fsSync.openSync).mock.invocationCallOrder[ + vi.mocked(fsSync.openSync).mock.calls.indexOf(seedOpen!) + ] + expect(seedOrder).toBeLessThan(vi.mocked(fs.copyFile).mock.invocationCallOrder[0]) + + // The chmod keeps a copied read-only attribute (Windows) from breaking the fsync + // open, and keeps a backup of a permissive file private. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.chmod).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_"), 0o600) + expect(vi.mocked(fs.chmod).mock.invocationCallOrder[0]).toBeGreaterThan( + vi.mocked(fs.copyFile).mock.invocationCallOrder[0], + ) + + // The copy is then opened for fsync with the writable flag. + const backupOpen = vi.mocked(fsSync.openSync).mock.calls.find(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "r+" + }) + expect(backupOpen).toBeDefined() + }) + + it("a failed backup flush is reported and leaves no partial backup behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The copy lands, but the fsync of the copy fails: the retained content is not + // known to be durable, so the write must not proceed on a half-written backup. + // The staged temp is fsynced earlier with a different handle, so target the + // backup's fd specifically. + vi.mocked(fsSync.openSync).mockImplementation((p: unknown) => + String(p).includes("safeWriteText.bak_") ? 7 : 1, + ) + vi.mocked(fsSync.fsyncSync).mockImplementation((fd: unknown) => { + if (fd === 7) { + throw new Error("EIO") + } + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow( + "EIO", + ) + + // Nothing was published, and the incomplete copy is removed rather than left + // next to the target looking like a usable backup. + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("carries the leftover path when the partial backup cannot be removed either", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockImplementation((p: unknown) => + String(p).includes("safeWriteText.bak_") ? 7 : 1, + ) + vi.mocked(fsSync.fsyncSync).mockImplementation((fd: unknown) => { + if (fd === 7) { + throw new Error("EIO") + } + }) + let unlinkAttempts = 0 + vi.mocked(fs.unlink).mockImplementation(async (p: unknown) => { + if (String(p).includes("safeWriteText.bak_")) { + unlinkAttempts++ + } + throw Object.assign(new Error("EPERM"), { code: "EPERM" }) + }) + + // The copy failed and the locked leftover could not be unlinked even after the + // retry. Dropping the path would leave a partial copy of the previous content on + // disk with no reference to it anywhere, so the error carries it. + const error = await safeWriteText(targetPath, "new data", { + backup: true, + platform: "linux", + }).catch((caught: unknown) => caught) + + expect(error).toBeInstanceOf(OrphanedBackupError) + const orphan = error as OrphanedBackupError + expect(orphan.orphanedBackupPath).toContain("safeWriteText.bak_") + expect(orphan.message).toContain("safeWriteText.bak_") + expect(orphan.message).toContain("EPERM") + expect(orphan.originalError).toBeInstanceOf(Error) + expect((orphan.originalError as Error).message).toBe("EIO") + expect(orphan.cause).toBe(orphan.originalError) + expect(unlinkAttempts).toBe(2) + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("a backup copy that fails part-way is removed, not left as a usable-looking backup", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The destination is seeded with openSync("wx") BEFORE any content exists at it, + // and the copy then fails part-way: the name is there, holding a partial copy of + // nothing usable. Cleanup has to key off the attempt, not off a copy that + // succeeded, or this file outlives the failed write beside the target. + vi.mocked(fs.copyFile).mockRejectedValue(Object.assign(new Error("EIO"), { code: "EIO" })) + + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow( + "EIO", + ) + + // Nothing was published, and the half-written copy is removed rather than left + // next to the target looking like a backup someone could restore. + 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) + 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("safeWriteText.acl")) + }) + + 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("safeWriteText.acl"), "/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("safeWriteText.acl"), + ]) + + // dump file was unlinked after restore + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) + }) + + 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("safeWriteText.acl")) + }) + + 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/test-dir/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/test-dir/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/test-dir/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/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. + 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/test-dir/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("a failed post-commit directory fsync does not roll the backup back over the published content", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const dirPath = path.dirname(targetPath) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // The file fd opens normally; the parent-directory open after the commit + // rename fails, which is the post-commit durability failure. + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) throw new Error("EBADF") + return 1 + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow( + PostCommitDurabilityError, + ) + + // The commit rename already published the new content, and the backup was only + // ever a copy: the target was never moved, so there is nothing to rename back. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // The durability failure is reported, not swallowed - and the backup copy is not + // left beside the target where no caller could find it. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("does not 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("fails the write outright when a staging write makes no progress", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // A zero-byte result means the filesystem stopped accepting progress; + // the loop must fail instead of spinning, so cleanup can run. + vi.mocked(fsSync.writeSync).mockImplementation(() => 0) + + await expect(safeWriteText(targetPath, "0123456789", { platform: "linux" })).rejects.toThrow( + /made no progress/, + ) + + // One attempt only: no fsync, no publish, and the staged file is torn down. + expect(fsSync.writeSync).toHaveBeenCalledTimes(1) + expect(fsSync.fsyncSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + 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("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) + .mockReturnValueOnce(1) + .mockImplementationOnce(() => { + throw new Error("EBADF") + }) + + // 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) + }) + + 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() + }) + }) + + it("a failed post-commit directory fsync never unlinks the committed target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const dirPath = path.dirname(targetPath) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) throw new Error("EBADF") + return 1 + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow( + PostCommitDurabilityError, + ) + + // The commit state is what cleanup is allowed to act on: the staging file may be + // unlinked only while it is still the staging file. Once the rename has committed, + // the only path this write owns at the target is the published content itself, and + // removing it would turn a durability warning into data loss. + expect(fs.unlink).not.toHaveBeenCalledWith(targetPath) + }) + + it("a failed post-commit directory fsync does not unlink the staging path this write no longer owns", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const dirPath = path.dirname(targetPath) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // A caller-supplied staging file: the name is the caller's, not one this write made + // up, so after the commit it is a path another writer may already be using again. + // The caller's staging file sits beside the target, as this contract requires. + const suppliedTemp = path.join(dirPath, "caller.new_data.json") + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) throw new Error("EBADF") + return 1 + }) + + await expect( + safeWriteText(targetPath, "new data", { backup: true, platform: "linux", tempPath: suppliedTemp }), + ).rejects.toThrow(PostCommitDurabilityError) + + // The rename consumed the staging file; the commit is what says so. Unlinking the + // staging NAME afterwards is an unlink of whatever now answers to that name. + expect(fs.unlink).not.toHaveBeenCalledWith(suppliedTemp) + expect(fs.unlink).not.toHaveBeenCalledWith(targetPath) + }) + + 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) + }) + }) + it("a no-replace commit links the staged file into place when the target is absent", async () => { + // Isolate the commit-phase counters from any async cleanup a previous + // test left in flight. + vi.mocked(fs.rename).mockClear() + vi.mocked(fs.unlink).mockClear() + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + + await safeWriteText(targetPath, "new data", { failIfExist: true, platform: "linux" }) + + // link(2) is the only atomic no-replace publish: a rename would displace a + // target that appeared after the caller's absence check. + expect(fs.link).toHaveBeenCalledWith(expect.stringContaining("safeWriteText"), targetPath) + expect(fs.rename).not.toHaveBeenCalled() + // The staged copy is dropped once the name points at it. + expect( + vi.mocked(fs.unlink).mock.calls.some(function (call) { + return String(call[0]).includes("safeWriteText") + }), + ).toBe(true) + }) + + it("a no-replace commit refuses a target that appeared after the caller checked", async () => { + // Isolate the commit-phase counters from any async cleanup a previous + // test left in flight. + vi.mocked(fs.rename).mockClear() + vi.mocked(fs.unlink).mockClear() + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.link).mockRejectedValue(Object.assign(new Error("EEXIST"), { code: "EEXIST" })) + // The failure path still cleans up its staging file and directory; keep those + // promises real so the cleanup does not mask the rejection under test. + vi.mocked(fs.unlink).mockResolvedValue(undefined) + vi.mocked(fs.rmdir).mockResolvedValue(undefined) + + const error = await safeWriteText(targetPath, "new data", { + failIfExist: true, + platform: "linux", + }).catch((caught: unknown) => caught) + + expect(error).toBeInstanceOf(TargetExistsError) + expect((error as TargetExistsError).targetPath).toBe(targetPath) + // Nothing replaced the newer file, and the staged copy is not left behind. + expect(fs.rename).not.toHaveBeenCalled() + expect( + vi.mocked(fs.unlink).mock.calls.some(function (call) { + return String(call[0]).includes("safeWriteText") + }), + ).toBe(true) + }) + + it("resolves an absent target through a symlinked ancestor to the canonical path", async () => { + // The guard pins the publish to the canonical nearest-ancestor path. An absent + // target has to resolve to that same path, or a create through a symlinked + // ancestor aborts with TargetMovedError although nothing moved. + const linkDir = path.resolve("/tmp/link-dir") + const canonicalDir = path.resolve("/real/dir") + const canonical = path.join(canonicalDir, "target.txt") + vi.mocked(fs.realpath).mockImplementation(async (p: unknown) => { + if (String(p) === path.join(linkDir, "target.txt")) { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + } + if (String(p) === linkDir) return canonicalDir + return String(p) + }) + vi.mocked(fs.lstat).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await safeWriteText(path.join(linkDir, "target.txt"), "new data", { + expectedResolvedPath: canonical, + platform: "linux", + }) + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText"), canonical) + }) + + it("fails closed when the filesystem has no atomic no-replace primitive", async () => { + // FAT32/exFAT and some SMB mounts reject link(2). Publishing by an exclusive + // copy would put bytes at the final name that readers can observe mid-copy, + // and a crash or an EIO mid-copy leaves damaged bytes there with the + // 'already exists' verdict on retry. No atomic no-replace primitive exists + // on those volumes, so the guarded create refuses to publish at all. + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.link).mockRejectedValue(Object.assign(new Error("ENOTSUP"), { code: "ENOTSUP" })) + + await expect( + safeWriteText(targetPath, "new data", { failIfExist: true, platform: "linux" }), + ).rejects.toBeInstanceOf(UnsupportedFilesystemError) + + // Nothing was published: no copy at the final name, no rename - the name + // stays absent for a retry on a capable volume. + // Every "no hard links here" code takes the same fail-closed path. + for (const code of ["EPERM", "ENOTSUP", "ENOSYS"]) { + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.link).mockRejectedValue(Object.assign(new Error(code), { code })) + + await expect( + safeWriteText(targetPath, "new data", { failIfExist: true, platform: "linux" }), + ).rejects.toBeInstanceOf(UnsupportedFilesystemError) + } + expect(fs.copyFile).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + // Every code reached the link path (the first call plus the three codes + // below), so the loop above really exercised all four refusals. + expect(fs.link).toHaveBeenCalledTimes(4) + // The refusal must not leave its staging file or directory beside the + // target: a regression here would drop a .file-safety-staging_* directory + // on FAT32/exFAT/SMB volumes after every refused create. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + const stagedDirs = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => String(call[0])) + expect(vi.mocked(fs.rmdir).mock.calls.map((call) => String(call[0]))).toEqual(stagedDirs) + }) + it("still reports success when the staged copy cannot be unlinked after the link", async () => { + // An antivirus handle on Windows can make unlink fail with EBUSY after link(2) + // published the name. The content is readable at the target, so the write must + // not be reported as a failure the model then retries into an 'already exists'. + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.unlink).mockRejectedValue(Object.assign(new Error("EBUSY"), { code: "EBUSY" })) + + await expect( + safeWriteText(targetPath, "new data", { failIfExist: true, platform: "linux" }), + ).resolves.toBeUndefined() + expect(fs.link).toHaveBeenCalledWith(expect.stringContaining("safeWriteText"), targetPath) + }) + + it("refuses to publish when the path no longer resolves to the authorized target", async () => { + // Isolate the commit-phase counters from any async cleanup a previous test left + // in flight. + vi.mocked(fs.rename).mockClear() + vi.mocked(fs.link).mockClear() + const authorized = "/tmp/test-dir/target.txt" + // The caller authorized this path while it was a real file in the workspace; by the + // time the publish resolves it, a local process has swapped in a link to somewhere + // else. The decision was never made about that other file, so nothing is published. + vi.mocked(fs.realpath).mockResolvedValue("/outside/the-victim.txt") + + const error = await safeWriteText(authorized, "new data", { + expectedResolvedPath: authorized, + platform: "linux", + }).catch((caught: unknown) => caught) + + expect(error).toBeInstanceOf(TargetMovedError) + const moved = error as TargetMovedError + expect(moved.authorizedPath).toBe(authorized) + expect(moved.resolvedPath).toBe("/outside/the-victim.txt") + expect(moved.message).toContain("/outside/the-victim.txt") + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.link).not.toHaveBeenCalled() + // Nothing was staged either: the check runs before the staging directory is made. + expect(fs.mkdir).not.toHaveBeenCalled() + }) + + it("refuses to publish when an authorized parent directory was replaced after the check", async () => { + // expectedResolvedPath pins the NAME being published, so it cannot notice the + // directory the name lives in being swapped for another one (a link to outside the + // workspace looks exactly like this). The recorded ancestor identities are what + // catch it, and the commit is aborted before anything is moved into place. + const dir = path.resolve("/tmp/test-dir") + const targetPath = path.join(dir, "target.txt") + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.rename).mockClear() + vi.mocked(fs.link).mockClear() + vi.mocked(fs.stat).mockResolvedValue(_dirIdentity(999n)) // was 100n when authorized + + const error = await safeWriteText(targetPath, "new data", { + expectedResolvedPath: targetPath, + expectedAncestorIdentities: [{ dir, dev: 1n, ino: 100n }], + platform: "linux", + }).catch((caught: unknown) => caught) + + expect(error).toBeInstanceOf(AncestorReplacedError) + expect((error as AncestorReplacedError).directory).toBe(dir) + expect((error as AncestorReplacedError).message).toContain("no longer the directory that was authorized") + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.link).not.toHaveBeenCalled() + // The staged copy is cleaned up rather than left beside the target. + expect(vi.mocked(fs.unlink).mock.calls.some((call) => String(call[0]).includes("safeWriteText"))).toBe(true) + }) + + it("refuses to publish when an authorized ancestor name was moved and replaced by a link", async () => { + // Moving a directory preserves (dev, ino), and replacing the old name with a + // symlink to the moved location keeps the identity check matching - stat + // follows the link. The commit takes path strings and would follow it too, + // publishing at the moved location outside the chain the caller walked, so + // the authorized name must still RESOLVE to the authorized location. + const dir = path.resolve("/tmp/test-dir") + const movedDir = path.resolve("/outside/workspace") + const targetPath = path.join(dir, "target.txt") + vi.mocked(fs.realpath).mockImplementation(async (p) => + path.resolve(String(p)) === dir ? movedDir : targetPath, + ) + vi.mocked(fs.rename).mockClear() + vi.mocked(fs.link).mockClear() + vi.mocked(fs.stat).mockResolvedValue(_dirIdentity(100n)) // SAME identity as authorized + + const error = await safeWriteText(targetPath, "new data", { + expectedResolvedPath: targetPath, + expectedAncestorIdentities: [{ dir, dev: 1n, ino: 100n }], + platform: "linux", + }).catch((caught: unknown) => caught) + + expect(error).toBeInstanceOf(AncestorReplacedError) + expect((error as AncestorReplacedError).message).toContain("no longer resolves to the authorized location") + // Nothing was published at either location. + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.link).not.toHaveBeenCalled() + }) + + it("propagates a transient realpath failure instead of reporting a replaced ancestor", async () => { + // EACCES/EIO while re-validating the authorized ancestry is not a containment + // violation: reporting AncestorReplacedError would point the user (and the + // model) at a security problem that is not there. + const dir = path.resolve("/tmp/test-dir") + const targetPath = path.join(dir, "target.txt") + const denied = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fs.realpath).mockImplementation(async (p) => { + if (path.resolve(String(p)) === dir) throw denied + return targetPath + }) + vi.mocked(fs.stat).mockResolvedValue(_dirIdentity(100n)) + + const error = await safeWriteText(targetPath, "new data", { + expectedResolvedPath: targetPath, + expectedAncestorIdentities: [{ dir, dev: 1n, ino: 100n }], + platform: "linux", + }).catch((caught: unknown) => caught) + + expect(error).toBe(denied) + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.link).not.toHaveBeenCalled() + }) + + it("publishes when the recorded ancestor identities are unchanged", async () => { + // The pin must not turn every guarded write into a false rejection: an unchanged + // directory chain publishes normally. + const dir = path.resolve("/tmp/test-dir") + const targetPath = path.join(dir, "target.txt") + // The unchanged chain resolves where it names: the ancestor realpath check + // sees the authorized directory itself, not a moved location. + vi.mocked(fs.realpath).mockImplementation(async (p) => (path.resolve(String(p)) === dir ? dir : targetPath)) + vi.mocked(fs.stat).mockResolvedValue(_dirIdentity(100n)) + + await safeWriteText(targetPath, "new data", { + expectedResolvedPath: targetPath, + expectedAncestorIdentities: [{ dir, dev: 1n, ino: 100n }], + platform: "linux", + }) + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText"), targetPath) + }) + + it("refuses the publish when an authorized ancestor disappears before the commit", async () => { + const dir = path.resolve("/tmp/test-dir") + const targetPath = path.join(dir, "target.txt") + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.stat).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + const error = await safeWriteText(targetPath, "new data", { + expectedResolvedPath: targetPath, + expectedAncestorIdentities: [{ dir, dev: 1n, ino: 100n }], + platform: "linux", + }).catch((caught: unknown) => caught) + + expect(error).toBeInstanceOf(AncestorReplacedError) + expect((error as AncestorReplacedError).message).toContain("no longer exists") + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.link).not.toHaveBeenCalled() + }) +}) + +// ── 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) => { + 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 + // 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)) + // The walk ends when readlink throws, because a plain file is not a link. + // Answering every call with the same value would let a walk that never + // stopped return the same key, so the mock has to distinguish the link from + // the referent and the test has to check that the walk stopped. + const notALink = Object.assign(new Error("EINVAL: not a link"), { code: "EINVAL" }) + vi.mocked(fs.readlink).mockImplementation(async (target) => { + if (String(target) === "/tmp/linkdir/file.json") return "referent.json" + throw notALink + }) + + // 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")), + ) + // Two calls means the walk stopped at the referent: the link, then one more + // on the referent that reports it is not a link. A walk that never stopped + // would run the full 8 steps. + expect(fs.readlink).toHaveBeenCalledTimes(2) + }) + + 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, 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) + // 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("ENOSPC") + + // 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[0] + 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) + }) +}) + +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)) + }) +}) + +describe("resolveLockKey when the parent directory does not exist yet", () => { + beforeEach(() => mockDefaults()) + + it("canonicalizes through the nearest existing ancestor instead of the literal parent", async () => { + // Two writers must take ONE lock: the one whose parent directory is already there, + // and the one racing to create it. Resolving only the immediate parent and falling + // back to its literal spelling on ENOENT gave them different keys whenever an + // ancestor was a symlink or a Windows short name, so a read-modify-write under the + // advisory lock lost one side. + const aliasDir = path.resolve("/tmp/alias-parent") + const canonicalDir = path.resolve("/tmp/real-parent") + const nested = path.join(aliasDir, "nested") + const target = path.join(nested, "history_item.json") + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(false)) + vi.mocked(fs.realpath).mockImplementation(async (p) => { + const s = String(p) + if (s === target || s === nested) { + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + } + if (s === aliasDir) { + return canonicalDir + } + return s + }) + + expect(await resolveLockKey(target)).toBe(path.join(canonicalDir, "nested", "history_item.json")) + }) + + it("propagates a realpath failure that is not ENOENT instead of guessing a key", async () => { + // A realpath that fails for another reason says nothing about the canonical form; + // returning a literal key would silently put this writer on a different lock. + const target = path.join(path.resolve("/tmp/test-dir"), "history_item.json") + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(false)) + // Not a link: the walk must reach the canonicalization step, which is where the + // non-ENOENT failure has to surface. + vi.mocked(fs.readlink).mockRejectedValue( + Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }), + ) + vi.mocked(fs.realpath).mockImplementation(async (p) => { + if (String(p) === target) return target + throw Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + }) + + await expect(resolveLockKey(target)).rejects.toThrow("EACCES") + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts new file mode 100644 index 0000000000..9701d4b3b6 --- /dev/null +++ b/src/services/file-safety/safeWriteText.ts @@ -0,0 +1,865 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import * as path from "path" +import { execFile } from "child_process" + +import { getErrorCode } from "../../utils/errorCode" + +export interface SafeWriteTextOptions { + /** + * When true, preserve the old-file semantics: rename target -> backup first, + * then stage and commit; after a successful commit delete the backup copy. + * On failure the backup is never rolled back to the target: it is a copy taken + * before staging began, not a restore source - the teardown discards it and the + * original error propagates. What the target holds after a rejected write + * depends on where the failure landed: the original file, the staged + * replacement, or - if the process died between the two renames - nothing, + * until a later writer's recovery pass restores the orphaned backup. + * 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 + + /** + * Commit only if the target name does NOT exist at commit time. + * rename(2) has no no-replace form - it silently displaces a target that another + * writer created after the caller's absence check - so the commit is done with + * link(2) instead, which fails EEXIST for an existing name (and does not follow a + * symlink placed at that name). A conditional create therefore cannot overwrite a + * file it never saw. + */ + failIfExist?: boolean + + /** + * The publish target the caller already authorized. + * A caller that checks workspace containment and then calls this primitive resolves + * the path twice: once for its own decision and once here. If a link is swapped in + * between the two, the decision was about a different file than the one published. + * Passing the authorized value makes that drift fatal instead of silent. + */ + expectedResolvedPath?: string + + /** + * The directories the caller walked when it authorized this target, with the + * (dev, ino) identity each had at that moment. + * + * expectedResolvedPath pins the NAME this write publishes; it cannot see a parent + * directory being replaced by a link between the caller's containment check and the + * commit. Re-checking the ancestor identities immediately before the commit makes a + * swapped parent fatal too: the publish then aborts instead of writing through the new + * link. Node has no descriptor-relative rename, so a swap that lands after this check + * and before the rename is not eliminable here - it narrows the window to the commit + * itself rather than the whole guard. + * The identity check is paired with a realpath comparison: moving a directory + * preserves (dev, ino), and replacing its old name with a symlink to the moved + * location keeps stat() matching while sending the commit to the new location. + * The name must still resolve to the authorized location, not merely to the + * same inode. + */ + expectedAncestorIdentities?: DirectoryIdentity[] +} + +/** One directory's on-disk identity, read with bigint stats so NTFS 64-bit values survive. */ +export type DirectoryIdentity = { + dir: string + dev: bigint + ino: bigint +} + +/** + * 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 + } +} + +/** + * The write committed and the content is published, but the pre-write backup could not be + * removed after two attempts (a transient handle - antivirus scanner, indexer - usually + * causes this). Returning success would hide a stale copy of the previous content that + * only a recovery tool can find, so the path travels on this error instead. + */ +export class OrphanedPublishedBackupError extends Error { + readonly publishedPath: string + readonly orphanedBackupPath: string + readonly cleanupError: unknown + + constructor(targetPath: string, backupPath: string, cleanupError: unknown) { + const reason = cleanupError instanceof Error ? cleanupError.message : String(cleanupError) + super( + `safeWriteText: the content was published at ${targetPath}, but the pre-write copy at ${backupPath} ` + + `could not be removed (${reason}). The copy is still on disk and should be removed by the caller or a recovery tool.`, + { cause: cleanupError }, + ) + this.name = "OrphanedPublishedBackupError" + this.publishedPath = targetPath + this.orphanedBackupPath = backupPath + 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.` + ) +} + +/** + * 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, copiedTargetFsync = false) { + super( + copiedTargetFsync + ? "The exclusive copy committed but the copied target could not be fsynced -- the content is at the target path reported on this error, and it may not survive a crash on a volume without hard links." + : "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 + } +} + +/** + * A conditional create (failIfExist) found a target at commit time. The write is + * refused without touching it: whatever is at the path was created by someone else + * and stays exactly as it is. + */ +export class TargetExistsError extends Error { + readonly targetPath: string + constructor(targetPath: string) { + super(`A file already exists at ${targetPath} -- the no-replace commit refused it.`) + this.name = "TargetExistsError" + this.targetPath = targetPath + } +} + +/** + * A conditional create (failIfExist) found that the volume hosting the target has + * no atomic no-replace primitive: link(2) reported that hard links do not exist + * here, and publishing by copy would put bytes at the final name that readers can + * observe mid-copy. Nothing is published - the write fails closed so a retry on a + * capable volume (or a caller that accepts non-atomic publication) decides. + */ +export class UnsupportedFilesystemError extends Error { + readonly targetPath: string + constructor(targetPath: string, cause: unknown) { + super( + `Filesystem hosting ${targetPath} has no atomic no-replace commit primitive -- the guarded create refused to publish by copy.`, + { cause }, + ) + this.name = "UnsupportedFilesystemError" + this.targetPath = targetPath + } +} + +/** + * The path resolved to something other than the target the caller authorized. + * Nothing is staged or published: a link swapped in after the caller's containment + * check would otherwise send the write to a file that check never covered. + */ +export class TargetMovedError extends Error { + readonly authorizedPath: string + readonly resolvedPath: string + constructor(authorizedPath: string, resolvedPath: string) { + super( + `The write was authorized for ${authorizedPath}, but that path now resolves to ${resolvedPath} -- nothing was published.`, + ) + this.name = "TargetMovedError" + this.authorizedPath = authorizedPath + this.resolvedPath = resolvedPath + } +} + +/** + * A directory the caller walked on its way to the authorized target is no longer the + * directory it was: it was removed, replaced, or turned into a link to somewhere else. + * Publishing through it would act on a decision that was never made about the file now + * reachable there, so the commit is aborted. + */ +export class AncestorReplacedError extends Error { + readonly directory: string + constructor(directory: string, reason: string) { + super(`A directory on the authorized path (${directory}) ${reason} -- nothing was published.`) + this.name = "AncestorReplacedError" + this.directory = directory + } +} +// -- 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 +} + +/** 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) => { + if (getErrorCode(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 + // 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 (getErrorCode(lstatError) === "ENOENT") return undefined + throw lstatError + }) + if (linkStat?.isSymbolicLink()) throw error + return await canonicalizeNearestAncestor(absoluteFilePath) + }) +} + +/** + * 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) + // Walk up to the nearest ancestor that EXISTS, canonicalize that, and re-join the + // components that are not there yet. Falling back to the unresolved spelling of the + // whole parent - what a single realpath(...).catch(() => dirPath) used to do - makes + // the key depend on whether the directory happens to exist: a writer whose parent is + // already there canonicalizes through a symlinked ancestor (or a short name) while a + // writer racing to create the same directory gets the literal spelling, so the two + // take different locks for one file and a read-modify-write loses one side. + // A realpath failure that is not "not there yet" says nothing about the canonical + // form, so it is propagated rather than papered over with a key that may be wrong. + let cursor = dirPath + const missing: string[] = [] + for (;;) { + const canonical = await fs.realpath(cursor).catch((error: unknown) => { + if (getErrorCode(error) === "ENOENT") return undefined + throw error + }) + if (canonical !== undefined) { + return path.join(canonical, ...missing.reverse(), path.basename(absoluteFilePath)) + } + missing.push(path.basename(cursor)) + const parent = path.dirname(cursor) + if (parent === cursor) { + // Every component up to the root is missing: there is nothing to canonicalize + // against, and the literal path is the only key left. + return path.join(dirPath, path.basename(absoluteFilePath)) + } + cursor = parent + } +} + +/** + * Canonicalize the nearest EXISTING ancestor and re-append the missing components. + * The guard hands back a pin produced exactly this way (guardedWrite#realpathNearest), + * so an absent target has to resolve identically here: a lexical fallback differs from + * that pin whenever an ancestor is a symlink, and the create would then abort with + * TargetMovedError even though nothing moved. + */ +async function canonicalizeNearestAncestor(absoluteFilePath: string): Promise { + const missing: string[] = [] + let cursor = absoluteFilePath + for (;;) { + try { + const realPath = await fs.realpath(cursor) + return missing.length > 0 ? path.join(realPath, ...missing.reverse()) : realPath + } catch (error: unknown) { + const code = getErrorCode(error) + if (code !== "ENOENT" && code !== "ENOTDIR") { + // A real inspection failure (EACCES, EIO) says nothing about the path's + // identity. Returning a lexical key here would silently canonicalize a + // different path than the guard's pin, so fail closed instead. + throw error + } + const parent = path.dirname(cursor) + if (parent === cursor) return absoluteFilePath + missing.push(path.basename(cursor)) + cursor = parent + } + } +} + +/** + * 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) + + // The caller authorized a specific resolved path; publishing at a different one + // would act on a decision that was never made about this file. + if (options?.expectedResolvedPath && path.resolve(options.expectedResolvedPath) !== targetPath) { + throw new TargetMovedError(options.expectedResolvedPath, targetPath) + } + + 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 + // Whether the commit rename has consumed the staging file. Cleanup is allowed to unlink + // the staging path only while this write still owns it: after the rename the name belongs to + // whatever is filed under it next, and a caller-supplied staging name is reused by the next + // write to the same target. + let stagingCommitted = false + 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, + ) + } + // Naming the target is not staging it: a staging file that resolves onto + // the publish target would let the commit rename move the target file + // onto itself, and the fsync and permission steps below would then act on + // the target as if it were the staged bytes. + if (supplied === targetPath) { + throw new StagingPathError( + `Staging file must not name the publish target (${targetPath}), 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) + 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 (getErrorCode(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 + // A zero-byte result means the filesystem stopped accepting progress; + // advancing nothing would spin here forever and the cleanup below would + // never run. Fail the write so the staged file is torn down instead. + while (offset < buffer.length) { + const written = fsSync.writeSync(fd, buffer, offset, buffer.length - offset) + if (written <= 0) { + throw new Error(`Staging write made no progress at offset ${offset} of ${buffer.length} bytes`) + } + offset += written + } + fsSync.fsyncSync(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 (getErrorCode(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) + } + fsSync.fsyncSync(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 = _tempName(dirPath, "safeWriteText.acl") + 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 2b: re-validate the authorized ancestry before committing -- + // The caller decided containment by walking this directory chain. A parent + // swapped for a link to outside that chain would send the commit somewhere the + // caller never authorized, even though expectedResolvedPath still matches (the + // name is unchanged). Compare each recorded identity now, before anything is + // moved into place. + if (options?.expectedAncestorIdentities) { + for (const expected of options.expectedAncestorIdentities) { + const current = await fs.stat(expected.dir, { bigint: true }).catch((error: unknown) => { + if (getErrorCode(error) === "ENOENT") { + throw new AncestorReplacedError(expected.dir, "no longer exists") + } + throw error + }) + if (current.dev !== expected.dev || current.ino !== expected.ino) { + throw new AncestorReplacedError(expected.dir, "is no longer the directory that was authorized") + } + // A move preserves dev+ino, so the identity check above passes for a + // directory that was relocated and whose old name was replaced by a + // symlink to the moved location - stat follows that link. The commit + // below takes path strings and would follow it too, publishing at the + // moved location outside the chain the caller walked. Require that the + // authorized name still resolves to the very location it named. + // Only a vanished name is a containment violation. A transient EACCES + // or EIO would otherwise be reported as a replaced ancestor, pointing + // the user at a security problem that is not there - mirror the stat + // call above, which maps only ENOENT and rethrows the rest. + const resolvedDir = await fs.realpath(expected.dir).catch((error: unknown) => { + const code = getErrorCode(error) + if (code === "ENOENT" || code === "ENOTDIR") { + return undefined + } + throw error + }) + if (resolvedDir === undefined || path.resolve(resolvedDir) !== path.resolve(expected.dir)) { + throw new AncestorReplacedError(expected.dir, "no longer resolves to the authorized location") + } + } + } + + // -- Step 3 (backup:true): durable copy target -> backup ---- + if (options?.backup) { + try { + await fs.access(targetPath) + backupPath = _tempName(dirPath, "safeWriteText.bak") + // Copy, never move. Renaming the target away leaves the canonical path absent for + // the whole commit window: readers see a missing file, and a concurrent + // writer can create a new target that a later rollback would destroy. A copy + // keeps the target present, so the step 4 rename is the only change to the + // canonical path. The copy is flushed so the retained content survives a crash. + try { + // Create the destination BEFORE any content exists at it, with the mode fixed + // at open time. fs.copyFile picks the destination mode itself (the platform + // creation mask subject to umask on some platforms, the source's mode - or its + // read-only attribute - on others), so letting it create the file would either + // leave a restrictive target's bytes briefly readable to others, or leave the + // copy unwritable so the fsync open below fails with EACCES. open() ignores its + // mode argument for an existing file, so this 0o600 survives the copy on POSIX; + // the chmod afterwards is what clears a copied read-only attribute on Windows + // and keeps a backup of a permissive file private. + const seedFd = fsSync.openSync(backupPath, "wx", 0o600) + // 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 + // flag the staged temp file uses above. + const backupFd = fsSync.openSync(backupPath, "r+") + try { + fsSync.fsyncSync(backupFd) + } finally { + fsSync.closeSync(backupFd) + } + } catch (backupError: unknown) { + // A partial backup must not outlive this attempt: it is not a complete copy + // of anything, and once the write fails nothing else removes it. The unlink is + // retried once (Windows reports EPERM for a file whose handle has not been + // released yet); if it still fails the path is carried on the thrown error + // instead of being dropped where no caller can act on it. + const orphanPath = backupPath + let backupCleanupError: unknown = null + for (let attempt = 0; attempt < 2; attempt++) { + try { + await fs.unlink(orphanPath) + backupCleanupError = null + break + } catch (cleanupError: unknown) { + if (getErrorCode(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 + if (backupCleanupError !== null) { + throw new OrphanedBackupError(orphanPath, targetPath, backupError, backupCleanupError) + } + throw backupError + } + releaseBackupOnSuccess = true + } catch (err: unknown) { + if (getErrorCode(err) !== "ENOENT") throw err + } + } + + // -- Step 4: atomic commit temp -> target --------------------- + if (options?.failIfExist) { + // No-replace commit: link(2) fails EEXIST when the name already exists, + // where a rename would silently displace whatever a non-participating + // writer put there after the caller checked. The staged copy is removed + // once the name points at it. + try { + await fs.link(tempPath, targetPath) + } catch (error: unknown) { + const code = getErrorCode(error) + if (code === "EEXIST") { + throw new TargetExistsError(targetPath) + } + // FAT32/exFAT volumes and some SMB/network mounts have no hard links, where + // link(2) fails EPERM/ENOTSUP/ENOSYS. The fallback this replaces published by + // an exclusive copy directly at the final name, but a copy is not atomic: + // readers can observe a partial file, and a crash or an EIO mid-copy leaves + // damaged bytes under the final name with the 'already exists' verdict on + // retry. No atomic no-replace primitive exists on those volumes, so the + // guarded create fails closed - nothing is published and the caller learns + // the volume cannot honor the contract. + if (code !== "EPERM" && code !== "ENOTSUP" && code !== "ENOSYS") { + throw error + } + throw new UnsupportedFilesystemError(targetPath, error) + } + } else { + await fs.rename(tempPath, targetPath) + } + + if (options?.failIfExist) { + // The target name is published at this point; the staged name is only a second + // link to the same inode (or a copy of it). Removal is best-effort: on Windows + // an antivirus handle can make unlink fail with EBUSY/EPERM after the content + // is already visible, and the write must not be reported as failed for content + // the caller can now read back. + await fs.unlink(tempPath).catch(() => undefined) + } + + // Marked once the commit has consumed the staging name: on the rename path the + // rename consumed it, on the link path the removal just did. Every step after + // this one is a post-commit step, and none of them may remove the staging path + // or the published content. + stagingCommitted = true + // -- 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 { + fsSync.fsyncSync(dirFd) + } finally { + fsSync.closeSync(dirFd) + } + } 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) + } + } + + // -- 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) { + const orphanedBackupPath = backupPath + let backupCleanupError: unknown = null + // A transient handle (antivirus scanner, indexer) can block the first unlink + // on Windows; one retry usually clears it. + for (let attempt = 0; attempt < 2; attempt++) { + try { + await fs.unlink(orphanedBackupPath) + backupCleanupError = null + break + } catch (cleanupError: unknown) { + if (getErrorCode(cleanupError) === "ENOENT") { + backupCleanupError = null + break + } + backupCleanupError = cleanupError + } + } + backupPath = null + if (backupCleanupError !== null) { + // The content is published: reporting success here would abandon a stale + // copy of the previous content, so the failure travels as its own error. + throw new OrphanedPublishedBackupError(targetPath, orphanedBackupPath, backupCleanupError) + } + } + } 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) { + // 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 + // 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 + } + if (!stagingCommitted) { + 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 + } +} diff --git a/src/utils/__tests__/fileLock.canonicalKey.spec.ts b/src/utils/__tests__/fileLock.canonicalKey.spec.ts new file mode 100644 index 0000000000..1dab157057 --- /dev/null +++ b/src/utils/__tests__/fileLock.canonicalKey.spec.ts @@ -0,0 +1,67 @@ +// npx vitest run utils/__tests__/fileLock.canonicalKey.spec.ts + +import path from "path" +import type { BigIntStats } from "fs" +import * as fs from "fs/promises" +import * as lockfile from "proper-lockfile" +import { withFileLock } from "../fileLock" +import { resolveLockKey } from "../../services/file-safety/safeWriteText" + +vi.mock("proper-lockfile", () => ({ + lock: vi.fn(async () => async () => {}), + unlock: vi.fn(async () => {}), +})) + +vi.mock("fs/promises", async () => { + const actual = await vi.importActual("fs/promises") + return { ...actual, realpath: vi.fn(), lstat: vi.fn(), readlink: vi.fn() } +}) + +const mockedRealpath = vi.mocked(fs.realpath) +const mockedLstat = vi.mocked(fs.lstat) +const mockedReadlink = vi.mocked(fs.readlink) +const mockedLock = vi.mocked(lockfile.lock) + +const dir = path.resolve(path.join(path.sep + "work", "history")) +const referent = path.join(dir, "item.json") +const alias = path.join(dir, "alias.json") + +beforeEach(() => { + mockedRealpath.mockReset() + mockedLstat.mockReset() + mockedReadlink.mockReset() + mockedLock.mockReset() + // alias.json is a symlink to item.json; everything else resolves to itself. + mockedRealpath.mockImplementation(async (target) => (target === alias ? referent : String(target))) + mockedLstat.mockImplementation(async () => ({ isSymbolicLink: () => false }) as unknown as BigIntStats) + mockedReadlink.mockRejectedValue(Object.assign(new Error("not a link"), { code: "EINVAL" })) +}) + +describe("withFileLock lock-key canonicalization", () => { + it("locks the canonical referent that safeWriteJson locks, not the raw alias", async () => { + // The compound-atomicity contract of task-history deletion is that an + // unlink under this helper and a safeWriteJson read-modify-write on the + // same file serialize. That holds only while both compute ONE key: + // safeWriteJson locks resolveLockKey(target), so withFileLock must too. + // Locking the raw alias here would watch a different lock file and let + // the deletion interleave with the merge write. + await withFileLock(alias, async () => undefined) + + expect(mockedLock).toHaveBeenCalledTimes(1) + expect(mockedLock.mock.calls[0][0]).toBe(referent) + // The shared-key contract stated directly: whatever safeWriteJson would + // lock for the same path is exactly what this helper locked. + expect(mockedLock.mock.calls[0][0]).toBe(await resolveLockKey(alias)) + }) + + it("hands the operation the caller's own path so its unlink keeps alias semantics", async () => { + // unlink(alias) removes the link; unlink(referent) removes the target + // file. Deletion must act on the path the caller named, even though the + // lock is taken on the canonical key. + const seen: string[] = [] + await withFileLock(alias, async (absoluteFilePath) => { + seen.push(absoluteFilePath) + }) + expect(seen).toEqual([alias]) + }) +}) diff --git a/src/utils/__tests__/fileLock.spec.ts b/src/utils/__tests__/fileLock.spec.ts new file mode 100644 index 0000000000..851094a5f5 --- /dev/null +++ b/src/utils/__tests__/fileLock.spec.ts @@ -0,0 +1,169 @@ +// npx vitest run utils/__tests__/fileLock.spec.ts + +import * as path from "path" + +import * as fsPromises from "fs/promises" + +import * as lockfile from "proper-lockfile" + +import { acquireFileLock, withFileLock } from "../fileLock" + +vi.mock("fs/promises", () => ({ + realpath: vi.fn(async (target: unknown) => { + const asString = String(target) + // A path that does not exist yet reports ENOENT, as the real fs does. + if (asString.includes("missing")) { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + } + return asString.replace("aliasDir", "realDir") + }), + // A non-link rejects readlink, exactly like the real fs; the link tests + // override this per path. + readlink: vi.fn(async () => { + throw Object.assign(new Error("EINVAL"), { code: "EINVAL" }) + }), +})) + +vi.mock("proper-lockfile", () => ({ + lock: vi.fn(async () => async () => {}), +})) + +/** + * proper-lockfile derives its lock file from the path it is handed, and this module + * turns off the library's own realpath step because the file may not exist yet. Without + * a canonical lock key, a writer that reaches a file through a symlinked directory and a + * deleter that reaches the same file lexically take DIFFERENT locks and silently lose + * updates against each other - the shape a task file under a symlinked task directory + * has against a task-history delete. + */ +const STORE = path.resolve("/tmp/store") + +describe("fileLock - canonical lock keys", () => { + beforeEach(() => { + vi.mocked(lockfile.lock).mockClear() + }) + + test("locks the referent when the path runs through a symlinked directory", async () => { + const releaseLock = await acquireFileLock(path.join(STORE, "aliasDir", "task.json")) + + expect(lockfile.lock).toHaveBeenCalledWith( + path.join(STORE, "realDir", "task.json"), + expect.objectContaining({ realpath: false }), + ) + + await releaseLock() + }) + + test("locks the referent but hands the operation the caller's own path", async () => { + // The mutex key is canonical; the path the caller works on is left alone. On Windows + // realpath can answer with the 8.3 short form, and rewriting the path a caller + // unlinks or compares would change behavior for every caller for no mutex gain. + const callerPath = path.join(STORE, "aliasDir", "task.json") + const seen: string[] = [] + await withFileLock(callerPath, async (absoluteFilePath) => { + seen.push(absoluteFilePath) + }) + + expect(seen).toEqual([callerPath]) + expect(lockfile.lock).toHaveBeenCalledWith( + path.join(STORE, "realDir", "task.json"), + expect.objectContaining({ realpath: false }), + ) + }) + + test("falls back to the lexical path when realpath fails with EACCES", async () => { + // A permission failure says nothing about the path's identity; failing closed to the + // lexical path keeps the lock key stable instead of inventing a canonical form. + vi.mocked(fsPromises.realpath).mockRejectedValueOnce( + Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }), + ) + const callerPath = path.join(STORE, "aliasDir", "task.json") + + await withFileLock(callerPath, async (absoluteFilePath) => { + expect(absoluteFilePath).toBe(callerPath) + }) + + expect(lockfile.lock).toHaveBeenCalledWith(callerPath, expect.objectContaining({ realpath: false })) + }) + + test("falls back to the lexical path when realpath fails without an error code", async () => { + vi.mocked(fsPromises.realpath).mockRejectedValueOnce(new Error("unknown failure")) + const callerPath = path.join(STORE, "aliasDir", "task.json") + + await withFileLock(callerPath, async (absoluteFilePath) => { + expect(absoluteFilePath).toBe(callerPath) + }) + + expect(lockfile.lock).toHaveBeenCalledWith(callerPath, expect.objectContaining({ realpath: false })) + }) + + test("canonicalizes the nearest existing ancestor when the file does not exist yet", async () => { + // The file and its parent are absent, so realpath reports ENOENT for them; the deepest + // existing ancestor is resolved and the missing components are re-appended. + const seen: string[] = [] + await withFileLock(path.join(STORE, "aliasDir", "missing", "new.json"), async (absoluteFilePath) => { + seen.push(absoluteFilePath) + }) + + expect(seen).toEqual([path.join(STORE, "aliasDir", "missing", "new.json")]) + expect(lockfile.lock).toHaveBeenCalledWith( + path.join(STORE, "realDir", "missing", "new.json"), + expect.objectContaining({ realpath: false }), + ) + }) +}) + +test("locks the referent of a file symlink whose target is temporarily absent", async () => { + // A backup-mode commit renames the referent away and back. During that window the + // link is dangling, realpath fails, and a naive fallback re-appends the LINK's + // basename onto the canonical parent - a different key from the one safeWriteJson + // holds through resolveLockKey, so the two writers stop excluding each other. + const linkPath = path.join(STORE, "link.json") + const referent = path.join(STORE, "referent.json") + vi.mocked(fsPromises.readlink).mockImplementation(async (p: unknown) => { + if (String(p) === linkPath) return referent + throw Object.assign(new Error("EINVAL"), { code: "EINVAL" }) + }) + + const releaseLock = await acquireFileLock(linkPath) + + expect(lockfile.lock).toHaveBeenCalledWith(referent, expect.objectContaining({ realpath: false })) + + await releaseLock() +}) +test("terminates on a three-link cycle instead of recursing forever", async () => { + // a -> b -> c -> a: the cycle length does not divide the per-frame walk bound, + // so a recursion that restarts its hop budget on every frame never terminates - + // each frame walks 8 hops, lands on a different cycle member, and recurses. + // resolveLockKey hands a cycle member to withFileLock, so an unbounded + // canonicalLockPath hangs every guarded writer on the lock path, not just the + // walker. The readlink mock yields to a timer so a regression fails on the + // test timeout instead of starving the event loop. + // Top-level test: clear the shared doubles so the counts below are this test's own. + vi.mocked(lockfile.lock).mockClear() + vi.mocked(fsPromises.readlink).mockClear() + vi.mocked(fsPromises.realpath).mockClear() + + const a = path.join(STORE, "cyc", "a.json") + const b = path.join(STORE, "cyc", "b.json") + const c = path.join(STORE, "cyc", "c.json") + const cycle: Record = { [a]: b, [b]: c, [c]: a } + vi.mocked(fsPromises.readlink).mockImplementation(async (target: unknown) => { + await new Promise((resolve) => setTimeout(resolve, 0)) + const referent = cycle[String(target)] + if (referent === undefined) throw Object.assign(new Error("EINVAL"), { code: "EINVAL" }) + return referent + }) + vi.mocked(fsPromises.realpath).mockImplementation(async () => { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + + const releaseLock = await acquireFileLock(a) + + // Terminated with exactly one lock acquisition on a cycle member, and the + // total hop budget held across all frames. + expect(lockfile.lock).toHaveBeenCalledTimes(1) + expect(vi.mocked(fsPromises.readlink).mock.calls.length).toBeLessThanOrEqual(64) + + await releaseLock() +}) diff --git a/src/utils/fileLock.ts b/src/utils/fileLock.ts index 49807ea154..2ab8179b00 100644 --- a/src/utils/fileLock.ts +++ b/src/utils/fileLock.ts @@ -1,6 +1,7 @@ import * as path from "path" import { tracePersistenceEvent } from "../core/task-persistence/persistenceTrace" import * as lockfile from "proper-lockfile" +import * as fs from "fs/promises" /** * Shared staleness window for per-file advisory locks. This module owns the @@ -19,6 +20,78 @@ export interface FileLockOptions { immediate?: boolean } +/** + * Canonical lock key for a path. + * + * proper-lockfile derives its lock file from the path it is handed, and this module + * turns off the library's own realpath step because the file may not exist yet. Two + * callers that reach the same file by different routes - one lexical, one through a + * symlinked directory - would then take DIFFERENT locks and silently lose updates + * against each other, which is exactly how a task file written through a symlinked + * task directory ends up unlocked against a task-history delete. + * + * The file itself may be absent (a create), so the nearest EXISTING ancestor is + * canonicalized and the missing components are re-appended. If nothing can be + * canonicalized the lexical absolute path is kept: lock keys stay stable and the + * write's own resolution still decides where content lands. + */ +/** + * Total link hops canonicalLockPath may follow across ALL its frames. The + * per-frame walk bound alone does not terminate: a cycle whose length does not + * divide that bound (a 3-link cycle a -> b -> c -> a against a bound of 8) + * makes every frame land on a different cycle member, so the recursion below + * restarts its budget forever. This runs on the lock path - withFileLock and every + * direct acquireFileLock caller hands it a cycle member - so an unbounded walk hangs + * guardedWrite and safeWriteJson, not just the walker. + */ +const MAX_TOTAL_LINK_HOPS = 64 + +async function canonicalLockPath(filePath: string, remainingLinkHops = MAX_TOTAL_LINK_HOPS): Promise { + const absoluteFilePath = path.resolve(filePath) + + // A file symlink has to produce the same key the writer's lock uses + // (resolveLockKey in safeWriteText). Walking readlink even when the referent is + // temporarily absent - which is exactly the window a backup-mode commit creates - + // keeps a raw withFileLock caller on the link path excluding the peer writing + // through the referent; re-appending the link's basename onto the canonical + // parent would take a different lock and the two operations would stop excluding + // each other. The walk is bounded so a two-link cycle terminates. + let linkCursor = absoluteFilePath + const frameBudget = Math.min(8, remainingLinkHops) + for (let depth = 0; depth < frameBudget; depth++) { + const referent = await fs.readlink(linkCursor).catch(() => undefined) + if (referent === undefined) { + break + } + linkCursor = path.resolve(path.dirname(linkCursor), referent) + } + if (linkCursor !== absoluteFilePath) { + // Recurse with the REMAINING hop budget, not a fresh one: once the total + // is spent the next frame walks zero hops and canonicalizes the ancestors + // of the cycle member it stopped on, so a cycle terminates on a stable key + // instead of recursing forever. + return await canonicalLockPath(linkCursor, Math.max(0, remainingLinkHops - frameBudget)) + } + const missing: string[] = [] + let cursor = absoluteFilePath + for (;;) { + try { + const realPath = await fs.realpath(cursor) + return missing.length > 0 ? path.join(realPath, ...missing.reverse()) : realPath + } catch (error) { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined + if (code !== "ENOENT" && code !== "ENOTDIR") return absoluteFilePath + const parent = path.dirname(cursor) + if (parent === cursor) return absoluteFilePath + missing.push(path.basename(cursor)) + cursor = parent + } + } +} + /** * Acquire the advisory lock for one file path using the exact protocol * `safeWriteJson` uses, so operations that hold this lock serialize with @@ -27,7 +100,7 @@ export interface FileLockOptions { * while holding it. */ export async function acquireFileLock(filePath: string, options?: FileLockOptions): Promise<() => Promise> { - const absoluteFilePath = path.resolve(filePath) + const absoluteFilePath = await canonicalLockPath(filePath) if (options?.waitForStaleLock) tracePersistenceEvent("json-lock-wait", { filePath: absoluteFilePath }) try { return await lockfile.lock(absoluteFilePath, { @@ -72,19 +145,32 @@ export async function withFileLock( operation: (absoluteFilePath: string) => Promise, options?: FileLockOptions, ): Promise { - const absoluteFilePath = path.resolve(filePath) - const releaseLock = await acquireFileLock(absoluteFilePath, options) + // The operation receives the caller's own resolved path. On Windows realpath can answer + // with the 8.3 short form (C:\Users\RUNNER~1\... for a temp dir under + // C:\Users\runneradmin\...), and the operation's own file calls (e.g. the task-history + // unlink) must keep the caller's alias semantics: unlinking the referent instead of the + // alias would delete a different directory entry. + const operationPath = path.resolve(filePath) + // The LOCK key is canonical - the same one safeWriteJson computes - so a caller that + // reaches the file through a symlinked directory and one that reaches it lexically + // contend for ONE lock file. Locking the raw path here would let a deletion through this + // helper run concurrently with a safeWriteJson read-modify-write on the referent, breaking + // the compound atomicity the deletion contract promises. The canonicalization runs inside + // acquireFileLock, so the walk happens once and direct acquireFileLock callers (safeWriteJson + // passes an already-canonical key, jsonFileRecovery and the specs pass raw paths) are all + // covered by the same rule. + const releaseLock = await acquireFileLock(operationPath, options) let result: T try { - result = await operation(absoluteFilePath) + result = await operation(operationPath) } catch (operationError) { // The operation error is the primary failure. Release without // reporting a secondary release error over it. try { await releaseLock() } catch (releaseError) { - console.error(`Failed to release lock for ${absoluteFilePath}:`, releaseError) + console.error(`Failed to release lock for ${operationPath}:`, releaseError) } throw operationError } @@ -94,7 +180,7 @@ export async function withFileLock( } catch (releaseError) { // The operation already succeeded, so a release failure is only // logged, matching how `safeWriteJson` handles release failures. - console.error(`Failed to release lock for ${absoluteFilePath}:`, releaseError) + console.error(`Failed to release lock for ${operationPath}:`, releaseError) } return result } From 1f656e3393f044ba388e997bac7b07448b128007 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 10 Nov 2026 21:13:08 +0800 Subject: [PATCH 2/2] chore(knip): declare the safeWriteText module an entry of the src workspace Production-mode knip is what fails this PR: knip --production --treat-config-hints-as-errors prints `Unused files (1)` / src/services/file-safety/safeWriteText.ts (check-run 129160792900 at 2026-11-09T12:30:46Z), while the development-mode run of the same command is clean at the same head. Nothing outside __tests__ imports the module here - a fixed-string search finds 148 spec rows and 8 self-references and no production importer - because the consumers are the sibling unit #2359: src/core/tools/guardedWrite.ts and src/utils/safeWriteJson.ts. Naming the module in workspaces.src.entry tells knip it is a root of this unit while no production caller exists yet, which is exactly why production mode reports it and development mode does not. This declaration is scaffolding: once the consumer content from #2359 lands the module has a real caller and the entry line becomes unnecessary. The resulting config object is byte-identical to the one the sibling unit #2385 already ships (blob 111f022102043c67fb581407a315ae16cca305b7 at fc2dc75ab3d1 and at c4fc4211356ab52ee44c653fee9949d18c3a7d13), so the two units converge on one spelling of the rule instead of diverging into two. knip.json is blob 1f4956faa6cc789a34d0d2d0e9f4397ce0ca1bc6 at this head, at merge base e487cd7b2a497aa1d9b60f9b0cb497f7839bb629 and at main tip 18861b2e357203e6cb4d93930aa2ed122b1eecea, so refreshing onto main cannot clear this required context. Price, with the CI gate's own selector (scripts/stryker-diff.mjs blob 7dfe0006d2d5, identical at this head and at the tip): the commit leg against parent 3edac64036eb prices 0 counted executable lines and 0 mutant selectors, and the CI-shape leg against tip 18861b2e3572 through merge-tree and commit-tree prices extension 386 changed executable lines / 119 selectors, unchanged from the pristine head, which matches CI's own mutation job 129160793410 printing `Mutation-testing 1 package(s) from merge base e487cd7b2a49: extension (386 lines)`. That job's success is not a clean pass: it printed `extension timed out 12 of 372 valid mutants` and a non-empty advisory (survived mutants at safeWriteText.ts:102, 123, 124, 125, 127, 137, 139 and fileLock.ts:61, 73, 80, 86, 173, 183), and timed-out mutants are not counted as kills. --- knip.json | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/knip.json b/knip.json index 1f4956faa6..111f022102 100644 --- a/knip.json +++ b/knip.json @@ -16,7 +16,12 @@ "playwright-ct": false, "workspaces": { "src": { - "entry": ["extension.ts!", "extension/api.ts!", "workers/countTokens.ts!"], + "entry": [ + "extension.ts!", + "extension/api.ts!", + "workers/countTokens.ts!", + "services/file-safety/safeWriteText.ts!" + ], "project": [ "**/*.ts!", "!**/__tests__/**!",