diff --git a/CHANGELOG.md b/CHANGELOG.md index 45dae4f..755429b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Security and Correctness + +- Retry a contended file-lock acquisition when Windows denies access to a lock file whose directory entry is still being torn down. Both the exclusive create and the holder's snapshot read reported that transient `EPERM` as a hard failure, so concurrent `acquireFileLock()` calls failed intermittently on Windows even though the very next attempt would have succeeded. Retries stay bounded, so a genuine permission denial still surfaces as `EPERM` rather than a lock timeout. + ### Features - Suffix Windows reserved basenames with `_` in `sanitizeUntrustedFileName()` while preserving case and extensions on every platform, including dollar names and superscript COM/LPT variants; thanks @SebTardif (#67). diff --git a/scripts/check-file-size.mjs b/scripts/check-file-size.mjs index 9a35075..deb4492 100644 --- a/scripts/check-file-size.mjs +++ b/scripts/check-file-size.mjs @@ -7,8 +7,10 @@ const LINE_BUDGETS = new Map([ ["src/permissions.ts", 566], ["src/root-impl.ts", 1750], ["src/root-path.ts", 862], + ["src/sidecar-lock.ts", 540], ["test/api-coverage.test.ts", 983], ["test/new-primitives.test.ts", 1500], + ["test/sidecar-lock-regression.test.ts", 540], ]); function walk(dir) { diff --git a/src/sidecar-lock-policy.ts b/src/sidecar-lock-policy.ts index e8fbef4..2a6ac65 100644 --- a/src/sidecar-lock-policy.ts +++ b/src/sidecar-lock-policy.ts @@ -10,6 +10,20 @@ export function computeSidecarLockDelayMs(retry: SidecarLockRetryOptions, attemp return Math.min(maxTimeout, Math.round(base * jitter)); } +// Windows denies access to a lock file while a just-unlinked directory entry +// is still being torn down, so a contended acquire sees EPERM on a name that is +// already gone -- both when creating it exclusively and when reading the +// holder's snapshot. The next attempt succeeds, so this is contention rather +// than a permission failure. The error must name the lock file itself: the +// exclusive-create helper opens the parent directory first, and a denial from +// that setup step carries no teardown evidence and has to reach the caller. +export const maxTransientLockDenials = 8; + +export function isTransientLockFileDenial(error: unknown, lockPath: string): boolean { + const denial = error as NodeJS.ErrnoException | null; + return process.platform === "win32" && denial?.code === "EPERM" && denial.path === lockPath; +} + export function sidecarLockPayloadIsStale( payload: unknown, staleMs: number, diff --git a/src/sidecar-lock.ts b/src/sidecar-lock.ts index e6f0c28..6ec7d9f 100644 --- a/src/sidecar-lock.ts +++ b/src/sidecar-lock.ts @@ -30,6 +30,8 @@ import type { import { computeSidecarLockDelayMs, defaultSidecarLockShouldReclaim, + isTransientLockFileDenial, + maxTransientLockDenials, } from "./sidecar-lock-policy.js"; export type { SidecarLockStaleSnapshot } from "./sidecar-lock-reclaim.js"; export type { @@ -261,6 +263,20 @@ export function createSidecarLockManager(key: string) { const reclaimGuardPath = `${lockPath}.reclaim`; let ownsReclaimGuard = false; let attempt = 0; + // Bounded so a genuine denial still surfaces as EPERM, not a lock timeout. + let transientDenials = 0; + const withinDenialBudget = (): boolean => ++transientDenials <= maxTransientLockDenials; + // Waiting can fail on the caller's own retry or deadline limits. Classifying + // a denial as contention must not cost them the original diagnosis, so hand + // the denial back when no further attempt can be scheduled. + const retryOrRethrowDenial = async (denial: unknown): Promise => { + try { + await waitForRetry(); + } catch (waitError) { + if ((waitError as NodeJS.ErrnoException).code === "file_lock_timeout") throw denial; + throw waitError; + } + }; const waitForRetry = async (): Promise => { const elapsed = Date.now() - startedAt; if ( @@ -290,6 +306,7 @@ export function createSidecarLockManager(key: string) { continue; } let handle: SidecarFileHandle | null = null; + let lockFileCreateDenied = false; try { const payload = await options.payload(); const { raw, ownershipToken } = serializeSidecarLockPayload(payload); @@ -305,7 +322,13 @@ export function createSidecarLockManager(key: string) { } handle = (await options.lockRoot.open(relativeLockPath)).handle; } else { - handle = (await createNativeExclusiveFile(lockPath, 0o600)) ?? await fs.open(lockPath, "wx"); + try { + handle = + (await createNativeExclusiveFile(lockPath, 0o600)) ?? (await fs.open(lockPath, "wx")); + } catch (createError) { + lockFileCreateDenied = isTransientLockFileDenial(createError, lockPath); + throw createError; + } await handle.writeFile(raw, "utf8"); } const snapshot = { raw, payload, stat: await handle.stat(), ownershipToken }; @@ -370,6 +393,10 @@ export function createSidecarLockManager(key: string) { parsePayload: options.parsePayload, }); } + if (lockFileCreateDenied && withinDenialBudget()) { + await retryOrRethrowDenial(err); + continue; + } if ((err as { code?: unknown }).code !== "EEXIST") { throw err; } @@ -379,10 +406,18 @@ export function createSidecarLockManager(key: string) { continue; } const nowMs = Date.now(); - const snapshot = await readSidecarLockSnapshot(lockPath, { - lockRoot: options.lockRoot, - parsePayload: options.parsePayload, - }); + let snapshot: SidecarLockSnapshot | null; + try { + snapshot = await readSidecarLockSnapshot(lockPath, { + lockRoot: options.lockRoot, + parsePayload: options.parsePayload, + }); + } catch (readErr) { + if (!isTransientLockFileDenial(readErr, lockPath) || !withinDenialBudget()) + throw readErr; + await retryOrRethrowDenial(readErr); + continue; + } if (!snapshot) { continue; } diff --git a/test/sidecar-lock-regression.test.ts b/test/sidecar-lock-regression.test.ts index 4c87536..a210432 100644 --- a/test/sidecar-lock-regression.test.ts +++ b/test/sidecar-lock-regression.test.ts @@ -5,6 +5,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { fileStore } from "../src/file-store.js"; import { acquireFileLock } from "../src/file-lock.js"; import { configureFsSafeLocks, getFsSafeLockConfig } from "../src/lock-config.js"; +import { configureFsSafeNative } from "../src/native-config.js"; import { createSidecarLockManager } from "../src/sidecar-lock.js"; const tempDirs: string[] = []; @@ -17,6 +18,7 @@ async function tempRoot(prefix: string): Promise { afterEach(async () => { vi.restoreAllMocks(); + configureFsSafeNative({ mode: "auto" }); configureFsSafeLocks({ retry: undefined, staleMs: undefined, @@ -27,6 +29,153 @@ afterEach(async () => { }); describe("sidecar lock regressions", () => { + // Windows denies access to a lock file while a just-unlinked directory entry is + // still being torn down. Both touch points below observed it in CI as EPERM. + const denial = (lockPath: string) => + Object.assign( + new Error(`EPERM: operation not permitted, open '${lockPath}'`), + { code: "EPERM", errno: -4048, syscall: "open", path: lockPath }, + ); + + it.runIf(process.platform === "win32")("keeps the lock-file EPERM when no retry is left", async () => { + // Classifying the denial as contention must not cost the caller the + // diagnosis: with no retry budget the original EPERM has to survive + // instead of being replaced by the loop's timeout error. + const base = await fsp.realpath(await tempRoot("fs-safe-sidecar-eperm-exhausted-")); + const targetPath = path.join(base, "state.json"); + const lockPath = `${targetPath}.lock`; + configureFsSafeNative({ mode: "off" }); + const realOpen = fsp.open.bind(fsp) as typeof fsp.open; + + vi.spyOn(fsp, "open").mockImplementation((async (...args: Parameters) => { + if (args[0] === lockPath && args[1] === "wx") throw denial(lockPath); + return await realOpen(...args); + }) as typeof fsp.open); + + await expect( + acquireFileLock(targetPath, { + managerKey: `eperm-exhausted-${Date.now()}-${Math.random()}`, + staleMs: 60_000, + timeoutMs: 1_000, + retry: { retries: 0 }, + payload: async () => ({ pid: process.pid }), + }), + ).rejects.toMatchObject({ code: "EPERM", path: lockPath }); + }); + + it.runIf(process.platform === "win32")("propagates an EPERM that names the lock parent", async () => { + // createNativeExclusiveFile() opens dirname(lockPath) before the exclusive + // create, so a denial can name the parent. Only the teardown window on the + // lock file itself is contention; a parent denial must reach the caller. + const base = await fsp.realpath(await tempRoot("fs-safe-sidecar-eperm-parent-")); + const targetPath = path.join(base, "state.json"); + configureFsSafeNative({ mode: "off" }); + const realOpen = fsp.open.bind(fsp) as typeof fsp.open; + let payloadCalls = 0; + + vi.spyOn(fsp, "open").mockImplementation((async (...args: Parameters) => { + if (args[0] === `${targetPath}.lock` && args[1] === "wx") throw denial(base); + return await realOpen(...args); + }) as typeof fsp.open); + + await expect( + acquireFileLock(targetPath, { + managerKey: `eperm-parent-${Date.now()}-${Math.random()}`, + staleMs: 60_000, + timeoutMs: 1_000, + retry: { retries: 0 }, + payload: async () => { + payloadCalls += 1; + return { pid: process.pid }; + }, + }), + ).rejects.toMatchObject({ code: "EPERM", path: base }); + expect(payloadCalls).toBe(1); + }); + + it.runIf(process.platform === "win32")("propagates an EPERM raised outside the lock file", async () => { + // Only the lock-file create and snapshot read see the teardown window. An + // EPERM from the caller's payload must reach the caller unchanged instead + // of being retried, which would rerun the callback and hide the error. + const base = await fsp.realpath(await tempRoot("fs-safe-sidecar-eperm-payload-")); + const targetPath = path.join(base, "state.json"); + configureFsSafeNative({ mode: "off" }); + let payloadCalls = 0; + + await expect( + acquireFileLock(targetPath, { + managerKey: `eperm-payload-${Date.now()}-${Math.random()}`, + staleMs: 60_000, + timeoutMs: 1_000, + retry: { retries: 0 }, + payload: async () => { + payloadCalls += 1; + throw denial(`${targetPath}.lock`); + }, + }), + ).rejects.toMatchObject({ code: "EPERM" }); + expect(payloadCalls).toBe(1); + }); + + it.runIf(process.platform === "win32")("retries an exclusive create denied mid-teardown", async () => { + const base = await fsp.realpath(await tempRoot("fs-safe-sidecar-eperm-create-")); + const targetPath = path.join(base, "state.json"); + const lockPath = `${targetPath}.lock`; + configureFsSafeNative({ mode: "off" }); + const realOpen = fsp.open.bind(fsp) as typeof fsp.open; + let injected = 0; + vi.spyOn(fsp, "open").mockImplementation((async (...args: Parameters) => { + if (args[0] === lockPath && args[1] === "wx" && injected === 0) { + injected += 1; + throw denial(lockPath); + } + return await realOpen(...args); + }) as typeof fsp.open); + + const lock = await acquireFileLock(targetPath, { + managerKey: `eperm-create-${Date.now()}-${Math.random()}`, + staleMs: 60_000, + timeoutMs: 1_000, + retry: { minTimeout: 1, maxTimeout: 2 }, + payload: async () => ({ pid: process.pid }), + }); + await lock.release(); + + expect(injected).toBe(1); + await expect(fsp.stat(lockPath)).rejects.toMatchObject({ code: "ENOENT" }); + }); + + it.runIf(process.platform === "win32")("retries a contended snapshot read denied mid-teardown", async () => { + const base = await fsp.realpath(await tempRoot("fs-safe-sidecar-eperm-read-")); + const targetPath = path.join(base, "state.json"); + const lockPath = `${targetPath}.lock`; + configureFsSafeNative({ mode: "off" }); + await fsp.writeFile(lockPath, JSON.stringify({ pid: process.pid, createdAt: new Date().toISOString() })); + const realReadFile = fsp.readFile.bind(fsp) as typeof fsp.readFile; + let injected = 0; + vi.spyOn(fsp, "readFile").mockImplementation((async (...args: Parameters) => { + if (args[0] === lockPath && injected === 0) { + injected += 1; + // The holder's unlink lands while the reader is being denied. + await fsp.rm(lockPath, { force: true }); + throw denial(lockPath); + } + return await realReadFile(...args); + }) as typeof fsp.readFile); + + const lock = await acquireFileLock(targetPath, { + managerKey: `eperm-read-${Date.now()}-${Math.random()}`, + staleMs: 60_000, + timeoutMs: 1_000, + retry: { minTimeout: 1, maxTimeout: 2 }, + payload: async () => ({ pid: process.pid }), + }); + await lock.release(); + + expect(injected).toBe(1); + await expect(fsp.stat(lockPath)).rejects.toMatchObject({ code: "ENOENT" }); + }); + it("does not delete a fresh sidecar lock during stale reclaim or old release", async () => { const base = await tempRoot("fs-safe-sidecar-token-"); const targetPath = path.join(base, "state.json");