From ce52d04464e31c7f5d127413d1f0aebe4598cf73 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 10 Nov 2026 05:41:52 +0800 Subject: [PATCH 1/4] fix(utils): one file, one lock artefact spelling, one canonical key Port of the fileLock lock-identity work from the PR 2357 chain (source commit pair 5319fe91fb0caf4aa4ec66aac68fa03744ebbc23..d6fa7117e07b70455da24e4722b47eb0b202fa6e), resolved against the artefact-spelling pin that PR 2385 landed so both survive: the canonical lock key from the split _lockCanonicalKey, and the caller-captured spelling passed verbatim to the acquisition. Adds src/utils/__tests__/fileLock.artefactSpelling.spec.ts: no carried spec distinguishes a caller-supplied artefact spelling from a derived one, and a unit carries the proof for the behaviour it moves. Unit U5c-1 of the split plan recorded on issues/1991#issuecomment-6751466344. 618 a+d / 3 files against base e487cd7b2a497aa1d9b60f9b0cb497f7839bb629, 81 counted executable lines of the 500 cap and 29 selectors of the 400 cap. --- .../fileLock.artefactSpelling.spec.ts | 68 ++++ src/utils/__tests__/fileLock.spec.ts | 308 ++++++++++++++++++ src/utils/fileLock.ts | 242 ++++++++++++-- 3 files changed, 596 insertions(+), 22 deletions(-) create mode 100644 src/utils/__tests__/fileLock.artefactSpelling.spec.ts create mode 100644 src/utils/__tests__/fileLock.spec.ts diff --git a/src/utils/__tests__/fileLock.artefactSpelling.spec.ts b/src/utils/__tests__/fileLock.artefactSpelling.spec.ts new file mode 100644 index 0000000000..1c2e12dc66 --- /dev/null +++ b/src/utils/__tests__/fileLock.artefactSpelling.spec.ts @@ -0,0 +1,68 @@ +// 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 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. + */ +const dir = fsSync.realpathSync(fsSync.mkdtempSync(path.join(os.tmpdir(), "filelock-artefact-spelling-"))) +const target = path.join(dir, "task.json") +const supplied = path.join(dir, "captured-before-the-rename.json.lock") + +fsSync.writeFileSync(target, "{}") + +afterAll(() => { + fsSync.rmSync(dir, { force: true, recursive: true }) +}) + +beforeEach(() => { + vi.mocked(lockfile.lock).mockReset() +}) + +describe("acquireFileLock artefact spelling", () => { + test("uses a caller-supplied spelling verbatim as the artefact", async () => { + const releaseLock = await acquireFileLock(target, { artefactSpelling: supplied }) + + expect(lockfile.lock).toHaveBeenCalledTimes(1) + expect(lockfile.lock).toHaveBeenCalledWith( + target, + expect.objectContaining({ lockfilePath: supplied, realpath: false }), + ) + + await releaseLock() + }) + + test("derives its own artefact when no spelling is handed in, and the two differ", async () => { + const derived = lockArtefactSpelling(target) + + expect(derived.endsWith(".lock")).toBe(true) + expect(derived).not.toBe(supplied) + + 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.map((call) => String(call[1]?.lockfilePath)) + expect(artefacts).toEqual([supplied, derived, derived]) + }) +}) diff --git a/src/utils/__tests__/fileLock.spec.ts b/src/utils/__tests__/fileLock.spec.ts new file mode 100644 index 0000000000..0c2bf8c03e --- /dev/null +++ b/src/utils/__tests__/fileLock.spec.ts @@ -0,0 +1,308 @@ +// 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) + expect(vi.mocked(fsPromises.readlink).mock.calls.length).toBeLessThanOrEqual(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", + ]) +}) diff --git a/src/utils/fileLock.ts b/src/utils/fileLock.ts index 49807ea154..757041f107 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,65 @@ 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. + 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. + const currentKey = await canonicalLockPath(operationPath).catch(() => lockKey) + 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 } From fb076148fdec38bc7ba38a0e581496771fe35989 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 10 Nov 2026 06:13:50 +0800 Subject: [PATCH 2/4] test(utils): take the artefact-spelling expectations from the code's own output The spec pinned its expectations to fsSync.realpathSync's answer while acquireFileLock canonicalizes with fs.promises.realpath, and on the windows runner those two routes answer with different spellings of the same directory: the temp directory arrived as its 8.3 short form and the canonical key came back long, so the assertion compared two spellings of one path. The canonical key and the derived artefact are read back from the code's first acquisition now, and the caller-supplied spelling is built under that same canonical parent, so every expectation compares code output with code output and none of them depends on which spelling realpath happens to hand the test. Co-Authored-By: Claude Opus 4.5 --- .../fileLock.artefactSpelling.spec.ts | 69 ++++++++++++++----- 1 file changed, 52 insertions(+), 17 deletions(-) diff --git a/src/utils/__tests__/fileLock.artefactSpelling.spec.ts b/src/utils/__tests__/fileLock.artefactSpelling.spec.ts index 1c2e12dc66..35f953a09a 100644 --- a/src/utils/__tests__/fileLock.artefactSpelling.spec.ts +++ b/src/utils/__tests__/fileLock.artefactSpelling.spec.ts @@ -14,17 +14,26 @@ vi.mock("proper-lockfile", () => ({ })) /** - * 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. + * 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.realpathSync(fsSync.mkdtempSync(path.join(os.tmpdir(), "filelock-artefact-spelling-"))) +const dir = fsSync.mkdtempSync(path.join(os.tmpdir(), "filelock-artefact-spelling-")) const target = path.join(dir, "task.json") -const supplied = path.join(dir, "captured-before-the-rename.json.lock") fsSync.writeFileSync(target, "{}") @@ -36,24 +45,47 @@ 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(1) - expect(lockfile.lock).toHaveBeenCalledWith( - target, + 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 derived = lockArtefactSpelling(target) + const { canonicalKey, derivedArtefact } = await acquireOnceAndReadBack() + const supplied = path.join(path.dirname(canonicalKey), "captured-before-the-rename.json.lock") - expect(derived.endsWith(".lock")).toBe(true) - expect(derived).not.toBe(supplied) + // 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() @@ -62,7 +94,10 @@ describe("acquireFileLock artefact spelling", () => { const derivedAgain = await acquireFileLock(target) await derivedAgain() - const artefacts = vi.mocked(lockfile.lock).mock.calls.map((call) => String(call[1]?.lockfilePath)) - expect(artefacts).toEqual([supplied, derived, derived]) + const artefacts = vi + .mocked(lockfile.lock) + .mock.calls.slice(1) + .map((call) => String(call[1]?.lockfilePath)) + expect(artefacts).toEqual([supplied, derivedArtefact, derivedArtefact]) }) }) From 9c08f7bafdd6467b5c2d4bff61751c5a2e201e88 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 10 Nov 2026 07:23:27 +0800 Subject: [PATCH 3/4] test(fileLock): kill the mutation run's blocking findings, fail the recheck closed The mutation-diff run on fb076148fdec listed 26 blocking findings in src/utils/fileLock.ts (22 survived, 4 no coverage). Each one is now either killed by a test that names the line it covers or declared equivalent with the mechanism named in the PR body. No Stryker disable comment was added. - lockArtefactSpelling's ancestor walk had no test at all: every carried row locked a leaf that already exists, so the walk-up, the re-join below the ancestor and the volume-root guard were never executed. - the hop budget was only exercised by a cycle, which spends it whatever the frame looks like; the cycle test now asserts exactly 64 asks and a 20-link chain names the referent a starving recursion stops eight links short of. - the ENOTDIR half of the fail-closed rule was never executed. - the retarget bound had no test behind it or behind its message: 3 retargets, 4 acquisitions, 4 releases, callback never run. - both lock-wait trace calls and both release-failure log lines were unexecuted code. CodeRabbit 4518779241 is right that the post-acquisition recheck reversed the fail-closed rule the walk above it states: `.catch(() => lockKey)` answered an EACCES with the caller's own spelling - a key no peer is provably holding - and ran the callback under it. The recheck now releases and rethrows, like the walk does. CodeRabbit 4518779232 is right that the lock-key comment misstated the contract: safeWriteJson hands acquireFileLock a path.resolve result (safeWriteJson.ts:75,100), and withFileLock derives its own key rather than sharing that derivation. --- .../fileLock.artefactSpelling.spec.ts | 68 ++++++++ .../__tests__/fileLock.lockWaitTrace.spec.ts | 96 +++++++++++ src/utils/__tests__/fileLock.spec.ts | 163 +++++++++++++++++- src/utils/fileLock.ts | 30 +++- 4 files changed, 347 insertions(+), 10 deletions(-) create mode 100644 src/utils/__tests__/fileLock.lockWaitTrace.spec.ts diff --git a/src/utils/__tests__/fileLock.artefactSpelling.spec.ts b/src/utils/__tests__/fileLock.artefactSpelling.spec.ts index 35f953a09a..e295f007a2 100644 --- a/src/utils/__tests__/fileLock.artefactSpelling.spec.ts +++ b/src/utils/__tests__/fileLock.artefactSpelling.spec.ts @@ -13,6 +13,25 @@ vi.mock("proper-lockfile", () => ({ 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 @@ -100,4 +119,53 @@ describe("acquireFileLock artefact spelling", () => { .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..34cf97089e --- /dev/null +++ b/src/utils/__tests__/fileLock.lockWaitTrace.spec.ts @@ -0,0 +1,96 @@ +// @ts-nocheck +// 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 index 0c2bf8c03e..3495aad9e1 100644 --- a/src/utils/__tests__/fileLock.spec.ts +++ b/src/utils/__tests__/fileLock.spec.ts @@ -207,7 +207,11 @@ test("terminates on a three-link cycle instead of recursing forever", async () = // 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) + // 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() }) @@ -306,3 +310,160 @@ test("retakes the lock when the canonical key moves while the callback waits to "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 757041f107..a998680bec 100644 --- a/src/utils/fileLock.ts +++ b/src/utils/fileLock.ts @@ -240,14 +240,16 @@ export async function withFileLock( // 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. + // 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 }) @@ -260,7 +262,17 @@ export async function withFileLock( // 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. - const currentKey = await canonicalLockPath(operationPath).catch(() => lockKey) + 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) { From 2c79cd47d8667623913dfc6b724b25a91a909318 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:18 +0800 Subject: [PATCH 4/4] fileLock: drop the unjustified @ts-nocheck from the lock-wait trace spec Review thread root 4519625959 asked for the directive to be removed or justified. It is removed, on a measurement: with tsc --noEmit -p tsconfig.json (typescript 5.9.3 under node v22.23.4, src/tsconfig.json sets strict:true and include [.], so specs are inside the checked set), the project reports 9 error lines with line 1 present and the same 9 lines with it deleted - identical sets, symmetric difference 0, none naming this file - and the spec itself still passes its 4 tests (exit 0). The directive was an escape hatch that suppressed nothing, so it is deleted rather than documented. Spec-only change: zero counted executable lines under the gate's /__tests__/ exclusion. Co-authored-by: u5a-2359 --- src/utils/__tests__/fileLock.lockWaitTrace.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/utils/__tests__/fileLock.lockWaitTrace.spec.ts b/src/utils/__tests__/fileLock.lockWaitTrace.spec.ts index 34cf97089e..68c5a2c02f 100644 --- a/src/utils/__tests__/fileLock.lockWaitTrace.spec.ts +++ b/src/utils/__tests__/fileLock.lockWaitTrace.spec.ts @@ -1,4 +1,3 @@ -// @ts-nocheck // npx vitest run utils/__tests__/fileLock.lockWaitTrace.spec.ts import * as path from "path"