diff --git a/src/utils/__tests__/fileLock.artefactSpelling.spec.ts b/src/utils/__tests__/fileLock.artefactSpelling.spec.ts new file mode 100644 index 0000000000..e295f007a2 --- /dev/null +++ b/src/utils/__tests__/fileLock.artefactSpelling.spec.ts @@ -0,0 +1,171 @@ +// npx vitest run utils/__tests__/fileLock.artefactSpelling.spec.ts + +import * as fsSync from "fs" +import * as os from "os" +import * as path from "path" + +import * as lockfile from "proper-lockfile" + +import { acquireFileLock, lockArtefactSpelling } from "../fileLock" + +vi.mock("proper-lockfile", () => ({ + lock: vi.fn(async () => async () => {}), + unlock: vi.fn(async () => {}), +})) + +// The ancestor walk inside `lockArtefactSpelling` had no test at all: every row here locks a leaf +// that already exists, so the one realpathSync.native call answered on the first cursor and the +// walk-up, the re-join below it and the volume-root guard were never executed. The handler answers +// the walk cursor by cursor - including a canonical spelling no volume on this host would give - +// and defaults to the platform's own answer for the two rows above. +let nativeHandler: ((cursor: string) => string) | undefined + +vi.mock("fs", async (importOriginal) => { + const actual = await importOriginal() + const realpathSyncShim = Object.assign( + (target: string, options?: Parameters[1]) => actual.realpathSync(target, options), + { + native: (target: string, options?: Parameters[1]) => + nativeHandler ? nativeHandler(target) : actual.realpathSync.native(target, options), + }, + ) + + return { ...actual, realpathSync: realpathSyncShim } +}) +/** + * The artefact spelling is caller-owned: a writer that renames the leaf as part of its own write + * captures the spelling once, before the first rename, and hands it back per acquisition, because + * re-deriving it after the rename names a different artefact for one file. The two carried fileLock + * specs name the `.lock` suffix but never pass or assert a spelling, so the leg in `_lockCanonicalKey` + * that consumes `artefactSpelling` would ship untested. These acquisitions are two-sided: dropping + * the caller's spelling in favour of the derived one fails the first test, and deriving nothing + * fails the second. + * + * Nothing here spells a path by hand. `canonicalLockPath` canonicalizes with `fs.promises.realpath` + * while `lockArtefactSpelling` canonicalizes with `fsSync.realpathSync.native`, and on the windows + * runner those two routes answer with different spellings of the same directory - the profile temp + * directory came back as the 8.3 short form from one and as the long form from the other, which is + * what made the first version of this spec fail on platform-unit-test (windows-latest) while + * passing locally. So the canonical key, the derived artefact and the parent the caller-supplied + * spelling is built under are all read back from the code's own first acquisition: what stays + * asserted is the behaviour - the string handed in reaches the library verbatim, both acquisitions + * take the same key, and the derived artefact differs from the supplied one and repeats exactly. + */ +const dir = fsSync.mkdtempSync(path.join(os.tmpdir(), "filelock-artefact-spelling-")) +const target = path.join(dir, "task.json") + +fsSync.writeFileSync(target, "{}") + +afterAll(() => { + fsSync.rmSync(dir, { force: true, recursive: true }) +}) + +beforeEach(() => { + vi.mocked(lockfile.lock).mockReset() +}) + +/** One acquisition with no spelling handed in, which is where the code spells both the key and the artefact. */ +async function acquireOnceAndReadBack(): Promise<{ canonicalKey: string; derivedArtefact: string }> { + const releaseLock = await acquireFileLock(target) + const call = vi.mocked(lockfile.lock).mock.calls[0] + const canonicalKey = String(call?.[0]) + const derivedArtefact = String(call?.[1]?.lockfilePath) + await releaseLock() + + return { canonicalKey, derivedArtefact } +} + +describe("acquireFileLock artefact spelling", () => { + test("uses a caller-supplied spelling verbatim as the artefact", async () => { + const { canonicalKey, derivedArtefact } = await acquireOnceAndReadBack() + const supplied = path.join(path.dirname(canonicalKey), "captured-before-the-rename.json.lock") + + const releaseLock = await acquireFileLock(target, { artefactSpelling: supplied }) + + expect(lockfile.lock).toHaveBeenCalledTimes(2) + expect(lockfile.lock).toHaveBeenNthCalledWith( + 2, + // the same key the spelling-free acquisition took: one file, one key, whatever the volume spells it as + canonicalKey, + expect.objectContaining({ lockfilePath: supplied, realpath: false }), + ) + + await releaseLock() + + expect(derivedArtefact.endsWith(".lock")).toBe(true) + expect(path.basename(derivedArtefact)).toBe("task.json.lock") + expect(path.basename(supplied)).toBe("captured-before-the-rename.json.lock") + expect(supplied).not.toBe(derivedArtefact) + }) + + test("derives its own artefact when no spelling is handed in, and the two differ", async () => { + const { canonicalKey, derivedArtefact } = await acquireOnceAndReadBack() + const supplied = path.join(path.dirname(canonicalKey), "captured-before-the-rename.json.lock") + + // the artefact the library was handed is the exported helper applied to the key the code locked, + // which is what makes a caller-supplied spelling comparable with a derived one + expect(derivedArtefact).toBe(lockArtefactSpelling(canonicalKey)) + + const callerSpelled = await acquireFileLock(target, { artefactSpelling: supplied }) + await callerSpelled() + const derivedSpelled = await acquireFileLock(target) + await derivedSpelled() + const derivedAgain = await acquireFileLock(target) + await derivedAgain() + + const artefacts = vi + .mocked(lockfile.lock) + .mock.calls.slice(1) + .map((call) => String(call[1]?.lockfilePath)) + expect(artefacts).toEqual([supplied, derivedArtefact, derivedArtefact]) + }) + afterEach(() => { + nativeHandler = undefined + }) + + // The walk stops at the nearest ancestor the volume can spell and re-joins what the caller + // spelled below it, in order. `reverse()` is the ordering, `missing.push` is the existence, + // and taking the canonical answer at all is the difference between an artefact a peer that + // reached the same file through the canonical path also takes and one only this caller takes. + test("re-joins the missing components in the caller order under the canonicalized ancestor", () => { + const anchor = path.resolve(path.join("store", "anchor")) + const canonicalAnchor = path.resolve(path.join("store", "anchor-on-volume")) + nativeHandler = (cursor: string) => { + if (cursor === anchor) return canonicalAnchor + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + } + + const spelled = lockArtefactSpelling(path.join(anchor, "aa", "bb", "task.json")) + + expect(spelled).toBe(path.join(canonicalAnchor, "aa", "bb", "task.json") + ".lock") + }) + + // Nothing inspectable anywhere on the path: the conservative answer is the caller's own + // spelling plus the suffix, which is the artefact that writer already takes. + test("falls back to the caller spelling when nothing on the path can be inspected", () => { + nativeHandler = () => { + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + } + const nowhere = path.resolve(path.join("store", "gone", "deeper", "task.json")) + + expect(lockArtefactSpelling(nowhere)).toBe(nowhere + ".lock") + }) + + // The root is the one cursor the walk must not step past: `path.dirname` answers with the same + // string, so that guard is what ends the loop. The handler answers the root only on a second + // visit, which is what a guard that keeps walking reaches - turning a hang into an artefact this + // assertion can see. + test("stops the ancestor walk at the volume root instead of stepping past it", () => { + const nowhere = path.resolve(path.join("store", "gone", "task.json")) + const root = path.parse(nowhere).root + let rootVisits = 0 + nativeHandler = (cursor: string) => { + if (cursor === root && ++rootVisits > 1) return path.join(root, "root-on-volume") + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + } + + expect(lockArtefactSpelling(nowhere)).toBe(nowhere + ".lock") + // one ask of the root, and the walk is over; a second visit is the loop the guard prevents + expect(rootVisits).toBe(1) + }) +}) diff --git a/src/utils/__tests__/fileLock.lockWaitTrace.spec.ts b/src/utils/__tests__/fileLock.lockWaitTrace.spec.ts new file mode 100644 index 0000000000..68c5a2c02f --- /dev/null +++ b/src/utils/__tests__/fileLock.lockWaitTrace.spec.ts @@ -0,0 +1,95 @@ +// npx vitest run utils/__tests__/fileLock.lockWaitTrace.spec.ts + +import * as path from "path" + +import * as lockfile from "proper-lockfile" + +import { tracePersistenceEvent } from "../../core/task-persistence/persistenceTrace" +import { acquireFileLock, withFileLock } from "../fileLock" + +vi.mock("proper-lockfile", () => ({ + lock: vi.fn(async () => async () => {}), +})) + +vi.mock("../../core/task-persistence/persistenceTrace", () => ({ + tracePersistenceEvent: vi.fn(), +})) + +vi.mock("fs/promises", () => ({ + // The canonical route is mocked to move the parent, so the key the trace names is distinguishable + // from the spelling the caller arrived with. + realpath: vi.fn(async (target: unknown) => String(target).replace("aliasDir", "realDir")), + readlink: vi.fn(async () => { + throw Object.assign(new Error("EINVAL: not a symlink"), { code: "EINVAL" }) + }), +})) + +/** + * Both lock-wait trace calls were code no test executed. The mutation run reported the event name, + * the payload object and both `waitForStaleLock` conditions as survivors on the acquisition helper + * and on `withFileLock`, which is only possible when the waiting path was never taken and the + * waiting-free path was never checked for staying silent. Each pair below pins the event on one + * path and its absence on the other, so tracing always and tracing never both fail. + * + * The traced value is read back from the acquisition the code actually made instead of spelled out: + * canonical spellings belong to the volume, and this row is about which event carries which field. + */ +const STORE = path.resolve("/tmp/store") +const aliasPath = path.join(STORE, "aliasDir", "task.json") + +function tracedPayloads(): unknown[] { + return vi.mocked(tracePersistenceEvent).mock.calls.map((call) => call[1]) +} + +describe("fileLock lock-wait trace", () => { + beforeEach(() => { + vi.mocked(lockfile.lock).mockReset() + vi.mocked(tracePersistenceEvent).mockClear() + }) + + test("emits the lock-wait event with the canonical key acquireFileLock is about to take", async () => { + const releaseLock = await acquireFileLock(aliasPath, { waitForStaleLock: true }) + const canonicalKey = String(vi.mocked(lockfile.lock).mock.calls[0]?.[0]) + + expect(tracePersistenceEvent).toHaveBeenCalledTimes(1) + expect(tracePersistenceEvent).toHaveBeenCalledWith("json-lock-wait", { filePath: canonicalKey }) + // the traced path is the key the lock is taken on, not the spelling the caller arrived with + expect(tracedPayloads()).toEqual([{ filePath: canonicalKey }]) + expect(canonicalKey).not.toBe(aliasPath) + + await releaseLock() + }) + + test("emits nothing on the acquireFileLock path that does not wait for a stale lock", async () => { + const releaseLock = await acquireFileLock(aliasPath) + + expect(lockfile.lock).toHaveBeenCalledTimes(1) + expect(tracePersistenceEvent).not.toHaveBeenCalled() + + await releaseLock() + }) + + test("emits the lock-wait event with the canonical key withFileLock is about to take", async () => { + const seen: string[] = [] + await withFileLock( + aliasPath, + async (absoluteFilePath) => { + seen.push(absoluteFilePath) + }, + { waitForStaleLock: true }, + ) + const canonicalKey = String(vi.mocked(lockfile.lock).mock.calls[0]?.[0]) + + expect(tracePersistenceEvent).toHaveBeenCalledTimes(1) + expect(tracePersistenceEvent).toHaveBeenCalledWith("json-lock-wait", { filePath: canonicalKey }) + expect(tracedPayloads()).toEqual([{ filePath: canonicalKey }]) + expect(seen).toEqual([aliasPath]) + }) + + test("emits nothing on the withFileLock path that does not wait for a stale lock", async () => { + await withFileLock(aliasPath, async () => "written") + + expect(lockfile.lock).toHaveBeenCalledTimes(1) + expect(tracePersistenceEvent).not.toHaveBeenCalled() + }) +}) diff --git a/src/utils/__tests__/fileLock.spec.ts b/src/utils/__tests__/fileLock.spec.ts new file mode 100644 index 0000000000..3495aad9e1 --- /dev/null +++ b/src/utils/__tests__/fileLock.spec.ts @@ -0,0 +1,469 @@ +// 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("propagates a realpath failure that is not ENOENT instead of locking a guessed key", async () => { + // A permission failure says nothing about the path's identity, which is exactly the + // problem: the caller spelling is then a key we cannot prove anyone else is taking, and a + // writer that reaches the same file through the referent holds a DIFFERENT lock. The stack + // already refuses to guess in safeWriteText.resolveLockKey for the same reason; the lock + // path has to refuse too rather than let two writers run at once. + vi.mocked(fsPromises.realpath).mockRejectedValueOnce( + Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }), + ) + const callerPath = path.join(STORE, "aliasDir", "task.json") + + await expect( + withFileLock(callerPath, async (absoluteFilePath) => { + expect(absoluteFilePath).toBe(callerPath) + }), + ).rejects.toMatchObject({ code: "EACCES" }) + + // Nothing was locked, so nothing ran under a key no peer was contending for. + expect(lockfile.lock).not.toHaveBeenCalled() + }) + + // The rule keys off the codes that mean "nothing is here yet". An error that carries no code + // at all is not evidence of absence, so it propagates on the same rule as EACCES. + test("propagates a realpath failure that carries no error code", async () => { + vi.mocked(fsPromises.realpath).mockRejectedValueOnce(new Error("unknown failure")) + const callerPath = path.join(STORE, "aliasDir", "task.json") + + await expect( + withFileLock(callerPath, async (absoluteFilePath) => { + expect(absoluteFilePath).toBe(callerPath) + }), + ).rejects.toThrow("unknown failure") + + expect(lockfile.lock).not.toHaveBeenCalled() + }) + + // The two rows above reach the lock through withFileLock, which calls canonicalLockPath and + // _lockCanonicalKey directly and never passes through acquireFileLock. A fallback added only to + // that exported entry point - canonicalize, and on any failure lock the caller's own spelling - + // would leave both of them green while a direct caller (safeWriteJson, task-history deletion) + // started taking a lock no peer was contending for. Same rejections, direct entry point. + test("propagates an EACCES realpath failure through the acquireFileLock entry point", async () => { + vi.mocked(fsPromises.realpath).mockRejectedValueOnce( + Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }), + ) + const callerPath = path.join(STORE, "aliasDir", "task.json") + + await expect(acquireFileLock(callerPath)).rejects.toMatchObject({ code: "EACCES" }) + + // Nothing was locked, so nothing ran under a key no peer was contending for. + expect(lockfile.lock).not.toHaveBeenCalled() + }) // end acquireFileLock EACCES row + + test("propagates a code-less realpath failure through the acquireFileLock entry point", async () => { + vi.mocked(fsPromises.realpath).mockRejectedValueOnce(new Error("unknown failure")) + const callerPath = path.join(STORE, "aliasDir", "task.json") + + await expect(acquireFileLock(callerPath)).rejects.toThrow("unknown failure") + + expect(lockfile.lock).not.toHaveBeenCalled() + }) // end acquireFileLock code-less row + + 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 canonicalized 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. + // The referent sits under aliasDir and under a component the realpath double reports as + // absent, so this row actually walks the missing-referent path: realpath of the link and of + // the referent both report ENOENT, and the answer has to come back as the deepest + // canonicalized ancestor plus the components that are missing - not the link's own name. + const aliasDir = path.join(STORE, "aliasDir") + const linkPath = path.join(aliasDir, "link.json") + const referent = path.join(aliasDir, "missingReferent.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( + path.join(STORE, "realDir", "missingReferent.json"), + 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) + // Exactly the budget, not merely under it: eight hops per frame, eight frames left by the + // recursion, so 64 asks. A frame that walks nine hops, and a recursion that hands the next + // frame no budget, both keep a cycle green under `<= 64`; one of them also stops eight links + // short of the referent the lock is supposed to name. + expect(vi.mocked(fsPromises.readlink).mock.calls.length).toBe(64) + + await releaseLock() +}) + +test("resolves a relative link target from the canonical parent, not from the caller's spelling", async () => { + // A link under a symlinked directory that points "../referent.json": the referent is a + // sibling of the REAL parent, so joining the referent lexically onto aliasDir names a + // different file. The writer through the alias would then hold a lock the writer through + // the referent never contends for. The mock canonicalizes aliasDir to a directory at a + // different depth, so the two answers are distinguishable instead of both landing on the + // same store/referent.json. + vi.mocked(lockfile.lock).mockClear() + vi.mocked(fsPromises.readlink).mockClear() + vi.mocked(fsPromises.realpath).mockClear() + const aliasParent = path.join(STORE, "aliasDir") + const linkPath = path.join(aliasParent, "link.json") + vi.mocked(fsPromises.readlink).mockImplementation(async (p: unknown) => { + if (String(p) === linkPath) return "../referent.json" + throw Object.assign(new Error("EINVAL"), { code: "EINVAL" }) + }) + vi.mocked(fsPromises.realpath).mockImplementation(async (target: unknown) => { + const asString = String(target) + if (asString === aliasParent) return path.resolve("/tmp/outer/real") + return asString + }) + + const releaseLock = await acquireFileLock(linkPath) + + expect(lockfile.lock).toHaveBeenCalledWith( + path.resolve("/tmp/outer/referent.json"), + expect.objectContaining({ realpath: false }), + ) + + await releaseLock() +}) + +test("retakes the lock when the canonical key moves while the callback waits to run", async () => { + // A directory component can be swapped while this call waits for the lock, leaving it + // holding the OLD canonical key while a peer reaching the file through the NEW one takes a + // different lock. withFileLock re-derives the key before running the callback and, on a + // change, releases, retakes for the current key, and only then runs. The double below flips + // the canonical parent inside the first acquisition, which is that swap. + vi.mocked(lockfile.lock).mockClear() + vi.mocked(fsPromises.readlink).mockClear() + vi.mocked(fsPromises.realpath).mockClear() + const aliasPath = path.join(STORE, "aliasDir", "task.json") + let resolvedDir = "realDirOne" + let acquisitions = 0 + vi.mocked(fsPromises.realpath).mockImplementation(async (target: unknown) => + String(target).replace("aliasDir", resolvedDir), + ) + // Each acquisition and the one release are logged into ONE list around the same 30 ms + // hold, and the callback logs into it too, so the row can pin one order end to end instead of + // only the keys and a callback count. The release double throws away the second release on + // purpose: only the first acquisition's lock is still held when the retarget happens, and the + // row is about the order, not about releasing a lock the helper already dropped. + const events: string[] = [] + vi.mocked(lockfile.lock).mockImplementation(async () => { + const acquiredAs = ++acquisitions + events.push("acquire:" + acquiredAs + ":started") + if (acquiredAs === 1) resolvedDir = "realDirTwo" + await new Promise((resolve) => setTimeout(resolve, 30)) + events.push("acquire:" + acquiredAs + ":completed") + return async () => { + if (acquiredAs !== 1) { + return + } + events.push("release:started") + await new Promise((resolve) => setTimeout(resolve, 30)) + events.push("release:completed") + } + }) + + const seen: string[] = [] + await withFileLock(aliasPath, async (absoluteFilePath) => { + seen.push(absoluteFilePath) + events.push("callback") + }) + + const keys = vi.mocked(lockfile.lock).mock.calls.map((call) => String(call[0])) + expect(keys).toEqual([path.join(STORE, "realDirOne", "task.json"), path.join(STORE, "realDirTwo", "task.json")]) + // The callback still receives the caller's own spelling, and it ran exactly once, after the + // retarget - not under the key that had already become stale. + expect(seen).toEqual([aliasPath]) + // The order is the assertion this row was raised about. The stale key's release has to finish + // before the replacement is asked for, and the callback has to run only once the replacement is + // held: either half regressing on its own used to pass here with the key list and the callback + // count both unchanged, because the old double logged nothing. + expect(events).toEqual([ + "acquire:1:started", + "acquire:1:completed", + "release:started", + "release:completed", + "acquire:2:started", + "acquire:2:completed", + "callback", + ]) +}) + +// The rows below exist because the mutation run named lines in this module with no test behind +// them. Each one says which mutant it is there to kill, so the disposition list on the PR has an +// executable answer behind every line it names. +describe("fileLock - retarget bound, fail-closed recheck and release failures", () => { + beforeEach(() => { + vi.mocked(lockfile.lock).mockReset() + vi.mocked(fsPromises.realpath).mockReset() + vi.mocked(fsPromises.readlink).mockReset() + }) + + // The fail-closed rule has two codes. Every carried test walks an ENOENT ancestor, so + // emptying the ENOTDIR string changed nothing anybody executed. A parent that is a file is + // how a real volume answers, and the climb has to continue past it. + test("keeps climbing past a parent that is a file instead of failing closed on ENOTDIR", async () => { + vi.mocked(fsPromises.realpath).mockImplementation(async (target: unknown) => { + const asString = String(target) + if (asString.includes("fileLike")) { + throw Object.assign(new Error("ENOTDIR: not a directory"), { code: "ENOTDIR" }) + } + return asString + }) + const target = path.join(STORE, "fileLike", "nested", "task.json") + + const releaseLock = await acquireFileLock(target) + + expect(lockfile.lock).toHaveBeenCalledWith(target, expect.objectContaining({ realpath: false })) + await releaseLock() + }) + + // The hop budget was only ever exercised by a cycle, and a cycle spends it whatever the frame + // looks like. A chain that ends on its own names the key it has to land on, so a recursion + // that hands the next frame no budget stops eight links short of the referent the lock is + // supposed to name. + test("walks a 20-link chain to its final referent instead of stopping at a frame boundary", async () => { + const referents = new Map() + for (let i = 1; i < 20; i++) { + referents.set( + path.join(STORE, "chain", "link" + i, "task.json"), + path.join(STORE, "chain", "link" + (i + 1), "task.json"), + ) + } + referents.set(path.join(STORE, "chain", "link20", "task.json"), path.join(STORE, "chainFinal", "task.json")) + vi.mocked(fsPromises.readlink).mockImplementation(async (target: unknown) => { + const referent = referents.get(String(target)) + if (referent === undefined) throw Object.assign(new Error("EINVAL"), { code: "EINVAL" }) + return referent + }) + const firstLink = path.join(STORE, "chain", "link1", "task.json") + const finalKey = path.join(STORE, "chainFinal", "task.json") + + const releaseLock = await acquireFileLock(firstLink) + + expect(lockfile.lock).toHaveBeenCalledWith(finalKey, expect.objectContaining({ realpath: false })) + // 20 links answered plus two rejections: the one that says the walk has arrived at a file, + // and the terminal frame asking its own entry cursor once before it canonicalizes. + expect(vi.mocked(fsPromises.readlink).mock.calls.length).toBe(22) + await releaseLock() + }) + + // Nothing asserted on the retarget bound or on its message: `attempt--`, `attempt >` and an + // emptied error string all kept the carried tests green. The spelling below moves on every + // derivation until the sixth, so the bound has to fire after three retargets - four locks + // taken, all four handed back, callback never run. + test("gives up after three retargets, releases every lock it took and never runs the callback", async () => { + let derivations = 0 + vi.mocked(fsPromises.realpath).mockImplementation(async () => { + derivations++ + return path.join(STORE, derivations <= 5 ? "realDir" + derivations : "realDirHeld", "task.json") + }) + const releases: number[] = [] + let acquisitions = 0 + vi.mocked(lockfile.lock).mockImplementation(async () => { + const acquiredAs = ++acquisitions + return async () => { + releases.push(acquiredAs) + } + }) + const aliasPath = path.join(STORE, "aliasDir", "task.json") + const seen: string[] = [] + + await expect( + withFileLock(aliasPath, async () => { + seen.push("callback") + }), + ).rejects.toThrow(/kept moving across 3 retargets/) + + expect(seen).toEqual([]) + expect(acquisitions).toBe(4) + expect(releases).toEqual([1, 2, 3, 4]) + }) + + // No test ever let the recheck reject, which is how the fallback to the caller's spelling + // survived as a mutant and how the asymmetry against the walk that took the lock stayed + // unexercised: that walk fails closed on an uninspectable ancestor. + test("releases the held lock and rethrows when the post-acquisition recheck cannot canonicalize", async () => { + const release = vi.fn(async () => {}) + vi.mocked(lockfile.lock).mockImplementation(async () => release) + let derivations = 0 + vi.mocked(fsPromises.realpath).mockImplementation(async () => { + derivations++ + if (derivations === 1) return path.join(STORE, "realDir", "task.json") + throw Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + }) + const aliasPath = path.join(STORE, "aliasDir", "task.json") + const seen: string[] = [] + + await expect( + withFileLock(aliasPath, async () => { + seen.push("callback") + }), + ).rejects.toMatchObject({ code: "EACCES" }) + + expect(seen).toEqual([]) + expect(lockfile.lock).toHaveBeenCalledTimes(1) + expect(release).toHaveBeenCalledTimes(1) + }) + + // Both release-failure log lines were code no test executed. They disagree about what the caller + // sees - the operation's own error outranks a failed release, a completed operation is not + // undone by one - and that difference is what the two assertions below pin. + test("rethrows the operation error when the release fails too, and logs the release failure", async () => { + const releaseError = new Error("release failed") + vi.mocked(lockfile.lock).mockResolvedValue(async () => { + throw releaseError + }) + const operationError = new Error("operation failed") + const logged = vi.spyOn(console, "error").mockImplementation(() => {}) + const target = path.join(STORE, "plain", "task.json") + + await expect( + withFileLock(target, async () => { + throw operationError + }), + ).rejects.toBe(operationError) + + expect(logged).toHaveBeenCalledTimes(1) + expect(logged).toHaveBeenCalledWith(`Failed to release lock for ${target}:`, releaseError) + logged.mockRestore() + }) + + test("returns the result when only the release fails, and logs the release failure", async () => { + const releaseError = new Error("release failed") + vi.mocked(lockfile.lock).mockResolvedValue(async () => { + throw releaseError + }) + const logged = vi.spyOn(console, "error").mockImplementation(() => {}) + const target = path.join(STORE, "plain", "task.json") + + const result = await withFileLock(target, async () => "written") + + expect(result).toBe("written") + expect(logged).toHaveBeenCalledTimes(1) + expect(logged).toHaveBeenCalledWith(`Failed to release lock for ${target}:`, releaseError) + logged.mockRestore() + }) +}) diff --git a/src/utils/fileLock.ts b/src/utils/fileLock.ts index 49807ea154..a998680bec 100644 --- a/src/utils/fileLock.ts +++ b/src/utils/fileLock.ts @@ -1,6 +1,8 @@ import * as path from "path" +import * as fsSync from "fs" 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 @@ -12,11 +14,146 @@ export const LOCK_STALE_MS = 31_000 const RECOVERY_RETRY_MS = 1000 const RECOVERY_GRACE_MS = 4000 +/** + * The artefact a lock lives in: the caller's path with its nearest existing ancestor spelled + * the way the volume spells it, plus ".lock". Nothing in the name comes from what the leaf + * currently holds, because `safeWriteJson`'s EPERM fallback renames the leaf itself to a backup + * mid-write: a name derived from the leaf at that moment belongs to whichever spelling the + * volume happens to hold, so two writers that compute one identity take two artefacts and + * contend with nobody. The second then acquires while the first still holds, runs its recovery + * pass over the first writer's in-flight temp file, and the first fails its commit rename with + * ENOENT. Only the nearest EXISTING ancestor is canonicalized and the components below it are + * re-joined, which is what keeps the name computable while the leaf is absent or dangling. The + * basename below that ancestor is compared as written, so two spellings of the file itself (an + * 8.3 alias, a case variant on a case-insensitive volume) still take one artefact each: the + * same residual the identity key carries, and a residual this function does not remove. + */ +export function lockArtefactSpelling(absoluteFilePath: string): string { + const absolute = path.resolve(absoluteFilePath) + const dirPath = path.dirname(absolute) + let cursor = dirPath + const missing: string[] = [] + for (;;) { + let canonical: string | undefined + try { + canonical = fsSync.realpathSync.native(cursor) + } catch { + // An ancestor that is not there yet, or that cannot be inspected, says nothing about the + // canonical spelling. Falling back to the caller's own parent is the conservative + // direction here: it can only ever give a writer the same artefact it takes today, never a + // second writer one it would not have taken. + canonical = undefined + } + if (canonical !== undefined) { + return path.join(canonical, ...missing.reverse(), path.basename(absolute)) + ".lock" + } + missing.push(path.basename(cursor)) + const parent = path.dirname(cursor) + if (parent === cursor) { + // Nothing on the path exists yet, so the caller's spelling is the only name left. + return absolute + ".lock" + } + cursor = parent + } +} + export interface FileLockOptions { /** Recovery may wait out a crashed writer's staleness window without stealing a live lock. */ waitForStaleLock?: boolean /** Startup scans defer locked files instead of waiting under the store mutex. */ immediate?: boolean + /** + * An artefact spelling the caller captured before it started moving the leaf, used verbatim + * for this acquisition. A writer that renames the leaf as part of its own write must capture + * the spelling once, before the first rename, and hand it back here: re-deriving it after the + * rename names a different artefact for one file. + */ + artefactSpelling?: string +} + +/** + * 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 + } + // A relative referent is relative to the link's REAL parent, not to the spelling the + // caller arrived with. Under a symlinked directory `..` names the referent's parent, so + // joining lexically produces a key for a different file and the writer through the alias + // stops excluding the writer through the referent. When the parent itself cannot be + // canonicalized (a dangling chain) the lexical join is the only answer available. + const parentReal = path.isAbsolute(referent) + ? undefined + : await fs.realpath(path.dirname(linkCursor)).catch(() => undefined) + linkCursor = path.resolve(parentReal ?? 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 + // Only a missing component may be walked over. An ancestor that exists but cannot be + // inspected (EACCES on a parent, ELOOP, EIO) means the canonical key is UNKNOWN, and a + // lexical key is a different lock from the one a peer that reached the same file through + // the referent holds: taking it lets both run at once. Fail closed rather than invent a + // key - the fallback below is only for "nothing here yet". + if (code !== "ENOENT" && code !== "ENOTDIR") throw error + const parent = path.dirname(cursor) + if (parent === cursor) return absoluteFilePath + missing.push(path.basename(cursor)) + cursor = parent + } + } } /** @@ -26,11 +163,30 @@ export interface FileLockOptions { * returned function exactly once and must not acquire the same lock again * while holding it. */ -export async function acquireFileLock(filePath: string, options?: FileLockOptions): Promise<() => Promise> { - const absoluteFilePath = path.resolve(filePath) - if (options?.waitForStaleLock) tracePersistenceEvent("json-lock-wait", { filePath: absoluteFilePath }) +/** + * The number of times withFileLock will release and retake the lock when the canonical key + * of the path moves underneath it. A flapping link is pathological; the bound exists so a + * walk that keeps answering differently terminates with an error instead of spinning. + */ +const MAX_LOCK_KEY_RETARGETS = 3 + +/** + * Take the advisory lock for a key that is already canonical. + * + * Split out of acquireFileLock so withFileLock can retake the lock for a re-derived key + * without re-running the canonicalization inside the acquire step: that is what lets it prove + * the lock it holds while running the callback is the lock a peer reaching the same file + * through any other spelling would take. + */ +async function _lockCanonicalKey(absoluteFilePath: string, options?: FileLockOptions): Promise<() => Promise> { + // The artefact name is taken from the caller's captured spelling when one is handed in, and otherwise + // derived from the canonical key with the nearest existing ancestor spelled as the volume spells it. + // Nothing in it comes from what the leaf currently holds, which is what keeps one file under one + // artefact while the EPERM fallback renames the leaf mid-write. + const lockfilePath = options?.artefactSpelling ?? lockArtefactSpelling(absoluteFilePath) try { return await lockfile.lock(absoluteFilePath, { + lockfilePath, // named by the caller's path and its canonical ancestor, never by what the leaf currently holds stale: LOCK_STALE_MS, update: 10000, // Update mtime every 10 seconds to prevent staleness if operation is long realpath: false, // the file may not exist yet, which is acceptable @@ -61,6 +217,12 @@ export async function acquireFileLock(filePath: string, options?: FileLockOption } } +export async function acquireFileLock(filePath: string, options?: FileLockOptions): Promise<() => Promise> { + const absoluteFilePath = await canonicalLockPath(filePath) + if (options?.waitForStaleLock) tracePersistenceEvent("json-lock-wait", { filePath: absoluteFilePath }) + return _lockCanonicalKey(absoluteFilePath, options) +} + /** * Run one operation while holding the advisory lock for a file path. * Callers must not acquire this lock again from inside `operation`, and @@ -72,29 +234,77 @@ 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 key acquireFileLock derives for this path - 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 derivation + // is not shared between the two routes: this call derives the key itself through + // canonicalLockPath just below, and acquireFileLock derives it again inside itself for the + // callers that come through it - safeWriteJson hands it a path.resolve result, and + // jsonFileRecovery and the specs hand it raw paths - so both routes apply the same rule + // instead of reusing one route's answer. + let lockKey = await canonicalLockPath(operationPath) + if (options?.waitForStaleLock) tracePersistenceEvent("json-lock-wait", { filePath: lockKey }) + + for (let attempt = 0; ; attempt++) { + const releaseLock = await _lockCanonicalKey(lockKey, options) + + // A directory component on this path can be swapped while this call waits for the lock, + // so the key acquired above may no longer be the canonical key of the path the callback + // is about to mutate: the mutation would then run while holding a lock no peer is + // contending for, and a concurrent merge could recreate what the callback deletes. Re-derive + // the key under the same rules before running anything, and on a change release, retake the + // lock for the new key, and only then run the callback. + let currentKey: string + try { + currentKey = await canonicalLockPath(operationPath) + } catch (recheckError) { + // The walk that took this lock refuses to invent a key when an ancestor exists but + // cannot be inspected; the recheck has to refuse the same way. An EACCES here used + // to answer with the caller's own spelling, which is a key no peer is provably + // holding, and the callback then ran under a lock that excludes nobody. + await releaseLock().catch(() => undefined) + throw recheckError + } + if (currentKey !== lockKey) { + await releaseLock().catch(() => undefined) + if (attempt >= MAX_LOCK_KEY_RETARGETS) { + throw new Error( + `Lock key for ${operationPath} kept moving across ${MAX_LOCK_KEY_RETARGETS} retargets -- the operation was not run.`, + ) + } + lockKey = currentKey + continue + } + + let result: T + try { + 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 ${operationPath}:`, releaseError) + } + throw operationError + } - let result: T - try { - result = await operation(absoluteFilePath) - } 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) + // The operation already succeeded, so a release failure is only + // logged, matching how `safeWriteJson` handles release failures. + console.error(`Failed to release lock for ${operationPath}:`, releaseError) } - throw operationError - } - - try { - await releaseLock() - } 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) + return result } - return result }