From 570fa9710693b4fa209cd2441585b518e43752ef Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 3 Sep 2026 08:46:25 -0700 Subject: [PATCH] fix(lock): retry Root fallback exclusive-create denials Record exclusive-open failures within each Root create observation and route only matching Windows lock-file denials through the existing bounded retry policy. Preserve caller budgets, denial caps, original errors, and replay isolation. --- CHANGELOG.md | 1 + docs/sidecar-lock.md | 7 +- src/file-observation.ts | 2 +- src/opened-file-failure.ts | 5 + src/root-impl.ts | 4 +- src/sidecar-lock-acquire.ts | 7 +- test/sidecar-lock-root-create-denial.test.ts | 264 +++++++++++++++++++ 7 files changed, 284 insertions(+), 6 deletions(-) create mode 100644 test/sidecar-lock-root-create-denial.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 2df1a20..afc59a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## 0.7.3 - Unreleased +- Retry Windows Root-backed sidecar exclusive-create denials within the existing eight-retry and caller budgets, using per-call provenance while preserving callback errors and rejecting replayed failure evidence. - Enforce portable FileStore keys consistently across methods: async reads, `exists`, and `remove` now reject parent-segment and backslash aliases with `invalid-path` for existing roots, matching sync and write methods while preserving missing-root error precedence and Root's confined existing-object compatibility. - Resync durable queue acknowledgement retries after final marker unlink before reporting completion or a newer-generation mismatch. - Propagate durable queue batch claim and migration failures instead of returning empty or partial success, and strictly sync migration publication in both loaders while preserving retry state. diff --git a/docs/sidecar-lock.md b/docs/sidecar-lock.md index 7a5a5fe..2bcf380 100644 --- a/docs/sidecar-lock.md +++ b/docs/sidecar-lock.md @@ -122,8 +122,11 @@ short teardown race after another holder unlinks it. Both async and sync locks retry that specific open denial at most eight times per acquisition, within the caller's retry/deadline budget. A parent-directory denial, a callback/read/stat failure, or exhaustion of either budget surfaces the original error; a denied -open is not converted to `file_lock_timeout`. Retrying always requires fresh -exclusive creation and grants no ownership or removal authority. +open is not converted to `file_lock_timeout`. Root-backed async creation uses +this same policy for the Windows fallback's exclusive-open denial, captured +within that individual create call. A generic `Root.create()` error or an error +replayed from an earlier call is not exclusive-open evidence. Retrying always +requires fresh exclusive creation and grants no ownership or removal authority. ## Owner-scoped reentrancy diff --git a/src/file-observation.ts b/src/file-observation.ts index 5464a5c..98079e3 100644 --- a/src/file-observation.ts +++ b/src/file-observation.ts @@ -1,6 +1,6 @@ import { AsyncLocalStorage } from "node:async_hooks"; -type FailureKind = "identity" | "resolution" | `open:${string}` | `unlinked:${string}` | `changed:${string}`; +type FailureKind = "identity" | "resolution" | `open:${string}` | `exclusive-create:${string}` | `unlinked:${string}` | `changed:${string}`; const active = new AsyncLocalStorage>>(); export function recordFileObservationFailure(error: unknown, kind: FailureKind): void { diff --git a/src/opened-file-failure.ts b/src/opened-file-failure.ts index 0ac7c9a..266cf48 100644 --- a/src/opened-file-failure.ts +++ b/src/opened-file-failure.ts @@ -9,6 +9,11 @@ export function recordFileOpenFailure(error: unknown, filePath: string): never { throw error; } +export function recordExclusiveCreateFailure(error: unknown, filePath: string): never { + recordFileObservationFailure(error, `exclusive-create:${filePath}`); + throw error; +} + export function openedPathResolutionError( error: Error = new FsSafeError("path-mismatch", "unable to resolve opened file path"), ): Error { diff --git a/src/root-impl.ts b/src/root-impl.ts index 1349146..66612f5 100644 --- a/src/root-impl.ts +++ b/src/root-impl.ts @@ -22,7 +22,7 @@ import { type DenyMutationPolicy, } from "./deny-mutations.js"; import { resolveOpenedFileRealPathForFd, resolveOpenedFileRealPathForHandle } from "./opened-realpath.js"; -import { openedPathResolutionError, recordFileOpenFailure, recordOpenedFileFailure, recordPreOpenFileChange } from "./opened-file-failure.js"; +import { openedPathResolutionError, recordExclusiveCreateFailure, recordFileOpenFailure, recordOpenedFileFailure, recordPreOpenFileChange } from "./opened-file-failure.js"; import { type RenameIdentityPolicy, runPinnedWriteHelper, @@ -1699,7 +1699,7 @@ async function writeMissingFileFallback( const { handle, writtenStat } = await withAsyncDirectoryGuards( [parentGuard], async () => { - const handle = await fs.open(targetPath, OPEN_WRITE_CREATE_FLAGS, params.mode ?? 0o600); + const handle = await fs.open(targetPath, OPEN_WRITE_CREATE_FLAGS, params.mode ?? 0o600).catch((error) => recordExclusiveCreateFailure(error, targetPath)); created = true; try { createdIdentity = await handle.stat(); diff --git a/src/sidecar-lock-acquire.ts b/src/sidecar-lock-acquire.ts index 8cf6e32..3225b1a 100644 --- a/src/sidecar-lock-acquire.ts +++ b/src/sidecar-lock-acquire.ts @@ -1,6 +1,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import { FsSafeError } from "./errors.js"; +import { fileObservation } from "./file-observation.js"; import { readFileHandleBounded } from "./bounded-read.js"; import { openSidecarRoot } from "./sidecar-lock-root.js"; import { createNativeExclusiveFile, type NativeFileHandle } from "./native-operations.js"; @@ -160,9 +161,13 @@ export async function acquireSidecarLock lockRoot.create(relativeLockPath, raw, { mkdir: true, mode: 0o600 })); } catch (error) { + // Only this invocation's failed exclusive open grants denial retry authority. + lockFileCreateDenied = observation.has(error, `exclusive-create:${lockPath}`) && + isTransientLockFileDenial(error, lockPath); if (error instanceof FsSafeError && error.code === "already-exists") { throw Object.assign(new Error("sidecar lock exists"), { code: "EEXIST" }); } diff --git a/test/sidecar-lock-root-create-denial.test.ts b/test/sidecar-lock-root-create-denial.test.ts new file mode 100644 index 0000000..8e71925 --- /dev/null +++ b/test/sidecar-lock-root-create-denial.test.ts @@ -0,0 +1,264 @@ +import fs from "node:fs"; +import fsp from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { createFileLockManager } from "../src/file-lock.js"; +import { configureFsSafeNative } from "../src/native-config.js"; +import { root } from "../src/root.js"; +import { useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +const platform = Object.getOwnPropertyDescriptor(process, "platform")!; +const retry = { minTimeout: 0, maxTimeout: 0 }; +const denial = (filePath: string, code = "EPERM") => + Object.assign(new Error("synthetic exclusive-create denial"), { code, syscall: "open", path: filePath }); +const exclusive = (flags: unknown) => typeof flags === "number" && + (flags & (fs.constants.O_CREAT | fs.constants.O_EXCL)) === (fs.constants.O_CREAT | fs.constants.O_EXCL); + +afterEach(() => { + vi.restoreAllMocks(); + Object.defineProperty(process, "platform", platform); + configureFsSafeNative({ mode: "auto" }); +}); + +async function fixture() { + const directory = await tempRoot("fs-safe-root-create-denial-"); + configureFsSafeNative({ mode: "off" }); + const lockRoot = await root(directory); + const target = path.join(directory, "state"); + const lockPath = `${target}.lock`; + const manager = createFileLockManager(`root-create-denial:${target}`); + Object.defineProperty(process, "platform", { value: "win32" }); + const options = { lockRoot, payload: () => ({ pid: process.pid }), staleMs: 60_000, timeoutMs: Infinity, retry }; + return { directory, target, lockPath, lockRoot, manager, options }; +} + +describe("Root exclusive-create denial (synthetic Windows/errno; real files)", () => { + it("retries a genuine fallback exclusive-open denial and admits only the fresh creator", async () => { + const { target, lockPath, manager, options } = await fixture(); + const error = denial(lockPath); + const realOpen = fsp.open.bind(fsp); + let attempts = 0; + vi.spyOn(fsp, "open").mockImplementation(async (...args) => { + if (args[0] === lockPath && exclusive(args[1]) && ++attempts === 1) throw error; + return await realOpen(...args); + }); + + const handle = await manager.acquire(target, options); + try { + expect(attempts).toBe(2); + expect(await handle.verifyStillHeld()).toBe(true); + expect(JSON.parse(await fsp.readFile(lockPath, "utf8"))).toEqual({ pid: process.pid }); + } finally { + await handle.release(); + } + expect(manager.heldEntries()).toEqual([]); + await expect(fsp.lstat(lockPath)).rejects.toMatchObject({ code: "ENOENT" }); + }); + + it.each([{ retries: 0, attempts: 1 }, { retries: 2, attempts: 3 }, { retries: 20, attempts: 9 }])( + "preserves the original denial with $retries caller retries", async ({ retries, attempts }) => { + const { directory, target, lockPath, manager, options } = await fixture(); + const error = denial(lockPath); + const keys = Reflect.ownKeys(error); + const realOpen = fsp.open.bind(fsp); + let denied = 0; + vi.spyOn(fsp, "open").mockImplementation(async (...args) => { + if (args[0] === lockPath && exclusive(args[1])) { denied += 1; throw error; } + return await realOpen(...args); + }); + + await expect(manager.acquire(target, { ...options, retry: { ...retry, retries } })).rejects.toBe(error); + expect(denied).toBe(attempts); + expect(Reflect.ownKeys(error)).toEqual(keys); + expect(manager.heldEntries()).toEqual([]); + expect(await fsp.readdir(directory)).toEqual([]); + }, + ); + + it("preserves the denial when the finite deadline expires", async () => { + const { directory, target, lockPath, manager, options } = await fixture(); + const error = denial(lockPath); + const now = Date.now(); + const clock = vi.spyOn(Date, "now").mockReturnValue(now); + const realOpen = fsp.open.bind(fsp); + let attempts = 0; + vi.spyOn(fsp, "open").mockImplementation(async (...args) => { + if (args[0] === lockPath && exclusive(args[1])) { + attempts += 1; + clock.mockReturnValue(now + 100); + throw error; + } + return await realOpen(...args); + }); + await expect(manager.acquire(target, { ...options, timeoutMs: 10 })).rejects.toBe(error); + expect(attempts).toBe(1); + expect(await fsp.readdir(directory)).toEqual([]); + }); + + it.each(["parent", "other-file", "unpathed", "EACCES", "EIO"])( + "does not retry an exclusive-open error classified as %s", async (kind) => { + const { directory, target, lockPath, manager, options } = await fixture(); + const error = denial(kind === "parent" ? directory : kind === "other-file" ? `${lockPath}.other` : lockPath, + kind === "EACCES" || kind === "EIO" ? kind : "EPERM"); + if (kind === "unpathed") Reflect.deleteProperty(error, "path"); + const realOpen = fsp.open.bind(fsp); + let attempts = 0; + vi.spyOn(fsp, "open").mockImplementation(async (...args) => { + if (args[0] === lockPath && exclusive(args[1])) { attempts += 1; throw error; } + return await realOpen(...args); + }); + await expect(manager.acquire(target, options)).rejects.toBe(error); + expect(attempts).toBe(1); + expect(manager.heldEntries()).toEqual([]); + }, + ); + + it("shares the eight-denial cap between Root creation and snapshot opens", async () => { + const { target, lockPath, manager, options } = await fixture(); + const error = denial(lockPath); + const foreign = '{"owner":"foreign"}\n'; + const realOpen = fsp.open.bind(fsp); + let createDenials = 0; + let readDenials = 0; + vi.spyOn(fsp, "open").mockImplementation(async (...args) => { + if (args[0] === lockPath) { + if (exclusive(args[1]) && createDenials < 4) { + if (++createDenials === 4) await fsp.writeFile(lockPath, foreign); + throw error; + } + if (typeof args[1] === "number" && !(args[1] & fs.constants.O_EXCL)) { + readDenials += 1; + throw error; + } + } + return await realOpen(...args); + }); + const shouldReclaim = vi.fn(() => true); + await expect(manager.acquire(target, { ...options, shouldReclaim })).rejects.toBe(error); + expect(createDenials).toBe(4); + expect(readDenials).toBe(5); + expect(shouldReclaim).not.toHaveBeenCalled(); + expect(manager.heldEntries()).toEqual([]); + await expect(fsp.readFile(lockPath, "utf8")).resolves.toBe(foreign); + }); + + it("does not accept a fresh wrapper error without exclusive-open provenance", async () => { + const { target, lockPath, lockRoot, manager, options } = await fixture(); + const error = denial(lockPath); + const create = vi.spyOn(lockRoot, "create").mockRejectedValue(error); + await expect(manager.acquire(target, options)).rejects.toBe(error); + expect(create).toHaveBeenCalledTimes(1); + expect(manager.heldEntries()).toEqual([]); + }); + + it("does not reuse a genuine denial receipt on the next attempt", async () => { + const { target, lockPath, lockRoot, manager, options } = await fixture(); + const error = denial(lockPath); + const realOpen = fsp.open.bind(fsp); + let opens = 0; + vi.spyOn(fsp, "open").mockImplementation(async (...args) => { + if (args[0] === lockPath && exclusive(args[1])) { opens += 1; throw error; } + return await realOpen(...args); + }); + const create = lockRoot.create.bind(lockRoot); + const wrapper = vi.spyOn(lockRoot, "create") + .mockImplementationOnce(create) + .mockRejectedValue(error); + await expect(manager.acquire(target, options)).rejects.toBe(error); + expect(wrapper).toHaveBeenCalledTimes(2); + expect(opens).toBe(1); + expect(manager.heldEntries()).toEqual([]); + }); + + it("does not reinterpret an ordinary Root open as an exclusive create", async () => { + const { target, lockPath, lockRoot, manager, options } = await fixture(); + const error = denial(lockPath); + const foreign = '{"owner":"foreign"}\n'; + await fsp.writeFile(lockPath, foreign); + const realOpen = fsp.open.bind(fsp); + vi.spyOn(fsp, "open").mockImplementation(async (...args) => { + if (args[0] === lockPath && typeof args[1] === "number" && !exclusive(args[1])) throw error; + return await realOpen(...args); + }); + const create = vi.spyOn(lockRoot, "create").mockImplementation(async () => { + const opened = await lockRoot.open("state.lock"); + await opened.handle.close(); + }); + await expect(manager.acquire(target, options)).rejects.toBe(error); + expect(create).toHaveBeenCalledTimes(1); + expect(manager.heldEntries()).toEqual([]); + await expect(fsp.readFile(lockPath, "utf8")).resolves.toBe(foreign); + }); + + it("keeps another Root target's exclusive-create failure out of this receipt", async () => { + const { directory, target, lockPath, lockRoot, manager, options } = await fixture(); + const error = denial(lockPath); + const realOpen = fsp.open.bind(fsp); + const otherPath = path.join(directory, "other.lock"); + vi.spyOn(fsp, "open").mockImplementation(async (...args) => { + if (args[0] === otherPath && exclusive(args[1])) throw error; + return await realOpen(...args); + }); + const create = lockRoot.create.bind(lockRoot); + const wrapper = vi.spyOn(lockRoot, "create").mockImplementation(async () => create("other.lock", "other")); + await expect(manager.acquire(target, options)).rejects.toBe(error); + expect(wrapper).toHaveBeenCalledTimes(1); + expect(await fsp.readdir(directory)).toEqual([]); + }); + + it.each(["wrapper", "payload", "toJSON", "parsePayload"] as const)( + "does not replay a prior acquisition's exclusive-create receipt through %s", async (callback) => { + const { target, lockPath, lockRoot, manager, options } = await fixture(); + const error = denial(lockPath); + const realOpen = fsp.open.bind(fsp); + const open = vi.spyOn(fsp, "open").mockImplementation(async (...args) => { + if (args[0] === lockPath && exclusive(args[1])) throw error; + return await realOpen(...args); + }); + await expect(manager.acquire(target, { ...options, retry: { ...retry, retries: 0 } })).rejects.toBe(error); + open.mockRestore(); + const foreign = '{"owner":"foreign"}\n'; + await fsp.writeFile(lockPath, foreign); + const fail = vi.fn((): never => { throw error; }); + if (callback === "wrapper") vi.spyOn(lockRoot, "create").mockImplementation(fail); + const shouldReclaim = vi.fn(() => true); + await expect(manager.acquire(target, { + ...options, + payload: callback === "payload" ? fail : () => callback === "toJSON" ? { toJSON: fail } : {}, + parsePayload: callback === "parsePayload" ? fail : JSON.parse, + shouldReclaim, + })).rejects.toBe(error); + expect(fail).toHaveBeenCalledTimes(1); + expect(shouldReclaim).not.toHaveBeenCalled(); + expect(manager.heldEntries()).toEqual([]); + await expect(fsp.readFile(lockPath, "utf8")).resolves.toBe(foreign); + }, + ); + + it.each(["write", "stat"])("does not retry a pathed EPERM from a post-create %s failure", async (operation) => { + const { directory, target, lockPath, manager, options } = await fixture(); + const error = denial(lockPath); + const realOpen = fsp.open.bind(fsp); + let attempts = 0; + vi.spyOn(fsp, "open").mockImplementation(async (...args) => { + const handle = await realOpen(...args); + if (args[0] === lockPath && exclusive(args[1])) { + attempts += 1; + if (operation === "write") vi.spyOn(handle, "writeFile").mockRejectedValue(error); + else { + const realStat = handle.stat.bind(handle); + vi.spyOn(handle, "stat").mockImplementation(async (options) => { + if (options?.bigint) throw error; + return await realStat(options); + }); + } + } + return handle; + }); + await expect(manager.acquire(target, options)).rejects.toBe(error); + expect(attempts).toBe(1); + expect(manager.heldEntries()).toEqual([]); + expect(await fsp.readdir(directory)).toEqual([]); + }); +});