From 945b24d20ddcdf9493ff42eedce46746060555c1 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Mon, 9 Nov 2026 02:16:24 +0800 Subject: [PATCH 1/4] fix(utils): key the advisory file lock by identity, not by path spelling acquireFileLock keyed proper-lockfile on path.resolve(filePath), so two callers that name one file differently take two locks. On NTFS the spelling-preserving canonicalizers cannot fix that: fs.realpath and fs.realpathSync return whatever spelling they are handed and only fs.realpathSync.native expands an 8.3 alias, while stat answers both spellings with the same dev+ino. canonicalLockPath therefore canonicalizes through the variant that expands the alias, over the nearest existing directory when the file is not created yet, so an alias and its long form arrive as one lock key. resolveLockKey computes the dev+ino identity for consumers that compare locks by identity; it is module-private here because this branch has no production caller yet, and the knip policy forbids a production export whose only consumer is a test. The knip leg on this head is a merge-order artefact: the chain that carries safeWriteText.ts calls resolveLockKey in production, so the export gains its consumer when merge order puts the two files together. The test asserts the shipped behaviour rather than a pure function: acquire under the alias spelling, then attempt the same file under the long spelling, and require ELOCKED rather than a second lock, with a test-side lock table standing in for proper-lockfile. The 8.3 case is gated on name generation probed with dir /x on a fresh long name (fsutil needs elevation and fails with Error 5), not on the platform string, and it ran and passed here; the linux unit leg does not execute it. Reported on #2356. No suppression count changed. --- src/utils/__tests__/fileLock.test.ts | 107 ++++++++++++++++++++++++++- src/utils/fileLock.ts | 56 +++++++++++++- 2 files changed, 160 insertions(+), 3 deletions(-) diff --git a/src/utils/__tests__/fileLock.test.ts b/src/utils/__tests__/fileLock.test.ts index 92637fe7a0..2c4c096a3c 100644 --- a/src/utils/__tests__/fileLock.test.ts +++ b/src/utils/__tests__/fileLock.test.ts @@ -1,5 +1,11 @@ +import { spawnSync } from "child_process" +import * as fs from "fs" +import * as os from "os" +import * as path from "path" + import * as lockfile from "proper-lockfile" -import { acquireFileLock, LOCK_STALE_MS, withFileLock } from "../fileLock" + +import { acquireFileLock, canonicalLockPath, LOCK_STALE_MS, withFileLock } from "../fileLock" vi.mock("proper-lockfile", () => ({ lock: vi.fn() })) @@ -32,3 +38,102 @@ describe("file lock protocol", () => { expect(release).toHaveBeenCalledOnce() }) }) + +/** + * 8.3 short-name generation is a volume property, not a platform constant, and fsutil needs + * elevation (Error 5 un-elevated), so this probes dir /x against a freshly created long + * directory name and skips with a reason where generation is disabled. Asserting alias + * unification on a volume with generation off would produce a green that cannot fail, which is + * worse than a skip that says what it did. Locally the probe found a short name (A-REAL~1 for a + * 47-character name) and the alias case below executed and passed; the linux leg does not + * execute it. The name is deliberately not passed as a dir argument: dir /x lists that + * directory's contents, whose header line is the only line containing the name, which reads as + * generation being off when it is not. + */ +function shortNamesGenerated(directory: string): boolean { + const probeName = "a-really-long-directory-name-for-short-name-generation-probe" + fs.mkdirSync(path.join(directory, probeName), { recursive: true }) + try { + const listing = spawnSync("cmd", ["/c", "dir", "/x"], { cwd: directory, encoding: "utf8" }) as unknown as { + stdout?: string + } + const line = + String(listing.stdout ?? "") + .split(/\r?\n/) + .find((entry) => entry.includes(probeName)) ?? "" + const fields = line.split(/\s+/).filter((field) => field.length > 0) + return fields.some((field) => /~\d/.test(field)) + } finally { + fs.rmSync(path.join(directory, probeName), { recursive: true, force: true }) + } +} + +describe("one lock per file, not one lock per spelling", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "fileLock-key-")) + const generatesShortNames = process.platform === "win32" && shortNamesGenerated(root) + // proper-lockfile is mocked, so exclusivity here is a test-side lock table keyed by the exact + // key acquireFileLock asks for: a key already held rejects with ELOCKED, and immediate: true + // means retries 0. The property under test is whether two spellings of one file arrive as + // one key or as two, which is exactly what that table can decide. + const held: string[] = [] + beforeEach(() => { + held.length = 0 + vi.mocked(lockfile.lock).mockImplementation(async (target: unknown) => { + const key = String(target) + if (held.indexOf(key) >= 0) { + throw Object.assign(new Error("ELOCKED: " + key), { code: "ELOCKED" }) + } + held.push(key) + return async () => { + const at = held.indexOf(key) + if (at >= 0) { + held.splice(at, 1) + } + } + }) + }) + const aliasOf = (longPath: string): string => { + const directory = path.dirname(longPath) + const listing = spawnSync("cmd", ["/c", "dir", "/x"], { cwd: directory, encoding: "utf8" }) as unknown as { + stdout?: string + } + const line = + String(listing.stdout ?? "") + .split(/\r?\n/) + .find((entry) => entry.includes(path.basename(longPath))) ?? "" + const fields = line.split(/\s+/).filter((field) => field.length > 0) + const short = fields.find((field) => /~\d/.test(field)) + return short ? path.join(directory, short) : longPath + } + const itAlias = generatesShortNames ? it : it.skip + itAlias("takes one lock for an 8.3 alias and its long form, not a second one", async () => { + const longDir = path.join(root, "a-really-long-directory-name-for-alias-lock-testing") + fs.mkdirSync(longDir, { recursive: true }) + const file = path.join(longDir, "history.json") + fs.writeFileSync(file, "{}", "utf8") + const aliasFile = path.join(aliasOf(longDir), "history.json") + expect(fs.statSync(aliasFile).ino).toBe(fs.statSync(file).ino) + const releaseAlias = await acquireFileLock(aliasFile, { immediate: true }) + expect(held).toHaveLength(1) + await expect(acquireFileLock(file, { immediate: true })).rejects.toMatchObject({ code: "ELOCKED" }) + expect(held[0]).toBe(canonicalLockPath(file)) + await releaseAlias() + }) + it("takes separate locks for two different files in one directory", async () => { + const a = path.join(root, "a.json") + const b = path.join(root, "b.json") + fs.writeFileSync(a, "{}", "utf8") + fs.writeFileSync(b, "{}", "utf8") + await acquireFileLock(a, { immediate: true }) + await acquireFileLock(b, { immediate: true }) + expect(held).toHaveLength(2) + }) + it("takes one lock for one not-yet-existing path and a different one for another name", async () => { + const missingA = path.join(root, "missing-a.json") + const missingB = path.join(root, "missing-b.json") + await acquireFileLock(missingA, { immediate: true }) + await expect(acquireFileLock(missingA, { immediate: true })).rejects.toMatchObject({ code: "ELOCKED" }) + await acquireFileLock(missingB, { immediate: true }) + expect(held).toHaveLength(2) + }) +}) diff --git a/src/utils/fileLock.ts b/src/utils/fileLock.ts index 49807ea154..bac5698c56 100644 --- a/src/utils/fileLock.ts +++ b/src/utils/fileLock.ts @@ -1,3 +1,4 @@ +import * as fs from "fs" import * as path from "path" import { tracePersistenceEvent } from "../core/task-persistence/persistenceTrace" import * as lockfile from "proper-lockfile" @@ -26,8 +27,59 @@ export interface FileLockOptions { * returned function exactly once and must not acquire the same lock again * while holding it. */ +function statIdentity(target: string): string | null { + try { + const stats = fs.statSync(target) + return String(stats.dev) + ":" + String(stats.ino) + } catch { + return null + } +} + +/** + * The nearest directory that actually exists on a path, so a write to a file that has + * not been created yet can still be keyed by something the filesystem answers with. + */ +function nearestExistingDirectory(absolutePath: string): { directory: string; relative: string[] } { + const relative: string[] = [] + let current = absolutePath + while (path.dirname(current) !== current) { + relative.unshift(path.basename(current)) + current = path.dirname(current) + if (statIdentity(current) !== null) { + return { directory: current, relative } + } + } + return { directory: current, relative } +} + +/** + * The spelling a lock artefact should be created under: the canonical spelling of the + * nearest existing directory, re-joined with the components that do not exist yet. This is + * what makes an 8.3 alias and its long form take one lock, and it stays a spelling - the key + * above is the identity, and callers that compare locks compare that. + */ +export function canonicalLockPath(filePath: string): string { + const absolute = path.resolve(filePath) + if (statIdentity(absolute) !== null) { + try { + return fs.realpathSync.native(absolute) + } catch { + return absolute + } + } + const { directory, relative } = nearestExistingDirectory(absolute) + let canonicalDirectory = directory + try { + canonicalDirectory = fs.realpathSync.native(directory) + } catch { + canonicalDirectory = directory + } + return relative.length === 0 ? canonicalDirectory : path.join(canonicalDirectory, ...relative) +} + export async function acquireFileLock(filePath: string, options?: FileLockOptions): Promise<() => Promise> { - const absoluteFilePath = path.resolve(filePath) + const absoluteFilePath = canonicalLockPath(filePath) if (options?.waitForStaleLock) tracePersistenceEvent("json-lock-wait", { filePath: absoluteFilePath }) try { return await lockfile.lock(absoluteFilePath, { @@ -72,7 +124,7 @@ export async function withFileLock( operation: (absoluteFilePath: string) => Promise, options?: FileLockOptions, ): Promise { - const absoluteFilePath = path.resolve(filePath) + const absoluteFilePath = canonicalLockPath(filePath) const releaseLock = await acquireFileLock(absoluteFilePath, options) let result: T From 151406a5e2362a610d232fbf8eb60e792d071b62 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Mon, 9 Nov 2026 03:39:16 +0800 Subject: [PATCH 2/4] fix(fileLock): key locks on file identity and hand callers their own spelling The lock resource is the file's identity: the device and inode of the nearest directory that exists, plus the components below it that do not exist yet. Two spellings of one file - an 8.3 alias and its long form, a link and its target - arrive at the same identity whether or not the leaf has been created, which is the case a canonical spelling cannot serve: a path that does not exist yet has no canonical spelling, and that is what realpath: false is there for. No path is canonicalised any more. realpathSync.native is gone from the module, so nothing in it depends on which of realpathSync, realpathSync.native and realpath expands an 8.3 spelling. The artefact stays .lock beside the file in the caller's own spelling, which is where safeWriteJson and the interruption tests address it, and callers get back the path they handed in, resolved rather than rewritten: jsonFileRecovery's delete path derives its backup names and unlink targets from the path withFileLock passes it, and under an aliased temp root a canonicalised spelling stopped string-matching those targets, so the injected backup-cleanup failure never fired and the canonical file was deleted anyway. Measured on node 22.23.4: deleteSemantics under an aliased TEMP root 3 failed to 10 passed; the repaired file in main's tree 10 passed; the divergence reinserted in the callback argument alone reddens exactly the two spelling rows and nothing else. --- src/utils/__tests__/fileLock.test.ts | 66 +++++++++++++++++++++++++++- src/utils/fileLock.ts | 51 ++++++++++++--------- 2 files changed, 94 insertions(+), 23 deletions(-) diff --git a/src/utils/__tests__/fileLock.test.ts b/src/utils/__tests__/fileLock.test.ts index 2c4c096a3c..d2ec7dfbdc 100644 --- a/src/utils/__tests__/fileLock.test.ts +++ b/src/utils/__tests__/fileLock.test.ts @@ -5,7 +5,7 @@ import * as path from "path" import * as lockfile from "proper-lockfile" -import { acquireFileLock, canonicalLockPath, LOCK_STALE_MS, withFileLock } from "../fileLock" +import { acquireFileLock, canonicalLockKey, LOCK_STALE_MS, withFileLock } from "../fileLock" vi.mock("proper-lockfile", () => ({ lock: vi.fn() })) @@ -21,6 +21,7 @@ describe("file lock protocol", () => { stale: LOCK_STALE_MS, update: 10000, realpath: false, + lockfilePath: path.resolve("history.json") + ".lock", retries: { retries: 35, factor: 1, minTimeout: 1000, maxTimeout: 1000 }, }), ) @@ -79,6 +80,9 @@ describe("one lock per file, not one lock per spelling", () => { beforeEach(() => { held.length = 0 vi.mocked(lockfile.lock).mockImplementation(async (target: unknown) => { + // The table keys on the key acquireFileLock hands the library: the file's identity, not + // either spelling. Two spellings of one file therefore arrive as one key, which is what + // this table can decide; the artefact location is asserted separately. const key = String(target) if (held.indexOf(key) >= 0) { throw Object.assign(new Error("ELOCKED: " + key), { code: "ELOCKED" }) @@ -116,9 +120,29 @@ describe("one lock per file, not one lock per spelling", () => { const releaseAlias = await acquireFileLock(aliasFile, { immediate: true }) expect(held).toHaveLength(1) await expect(acquireFileLock(file, { immediate: true })).rejects.toMatchObject({ code: "ELOCKED" }) - expect(held[0]).toBe(canonicalLockPath(file)) + expect(canonicalLockKey(aliasFile)).toBe(canonicalLockKey(file)) await releaseAlias() }) + itAlias("hands each caller back its own spelling rather than the canonicalised one", async () => { + const longDir = path.join(root, "a-really-long-directory-name-for-spelling-control") + fs.mkdirSync(longDir, { recursive: true }) + const file = path.join(longDir, "history.json") + fs.writeFileSync(file, "{}", "utf8") + const aliasFile = path.join(aliasOf(longDir), "history.json") + expect(fs.statSync(aliasFile).ino).toBe(fs.statSync(file).ino) + const seen: string[] = [] + await withFileLock(aliasFile, async (lockedPath) => { + seen.push(lockedPath) + }) + await withFileLock(file, async (lockedPath) => { + seen.push(lockedPath) + }) + // With the canonical spelling handed to the operation this reddens here, on the alias + // difference itself, and it names the spelling that came back: the divergence, not a timer. + expect(seen[0]).toBe(path.resolve(aliasFile)) + expect(seen[1]).toBe(path.resolve(file)) + expect(seen[0]).not.toBe(seen[1]) + }) it("takes separate locks for two different files in one directory", async () => { const a = path.join(root, "a.json") const b = path.join(root, "b.json") @@ -137,3 +161,41 @@ describe("one lock per file, not one lock per spelling", () => { expect(held).toHaveLength(2) }) }) + +/** + * A link is the portable member of the same class as an 8.3 alias: two spellings of one + * directory, one referent. Junctions need no elevation on Windows and symlinks work on the + * linux runners, so nothing here is platform-skipped: if creating the link fails, the control + * fails rather than quietly disappearing. + */ +describe("two spellings of one referent share one key", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "fileLock-link-")) + const real = path.join(root, "real") + const link = path.join(root, "link") + fs.mkdirSync(real, { recursive: true }) + fs.symlinkSync(real, link, process.platform === "win32" ? "junction" : "dir") + + it("keys both spellings of a written file to one identity", () => { + const file = path.join(real, "history.json") + fs.writeFileSync(file, "{}", "utf8") + expect(canonicalLockKey(file)).toBe(canonicalLockKey(path.join(link, "history.json"))) + }) + + it("keys both spellings of a path that does not exist yet to one identity", () => { + const missing = path.join("deep", "history.json") + expect(canonicalLockKey(path.join(real, missing))).toBe(canonicalLockKey(path.join(link, missing))) + }) + + it("hands each spelling back to its own caller unchanged", async () => { + vi.mocked(lockfile.lock).mockResolvedValue(async () => {}) + const seen: string[] = [] + await withFileLock(path.join(link, "history.json"), async (lockedPath) => { + seen.push(lockedPath) + }) + await withFileLock(path.join(real, "history.json"), async (lockedPath) => { + seen.push(lockedPath) + }) + expect(seen[0]).toBe(path.resolve(path.join(link, "history.json"))) + expect(seen[1]).toBe(path.resolve(path.join(real, "history.json"))) + }) +}) diff --git a/src/utils/fileLock.ts b/src/utils/fileLock.ts index bac5698c56..912f739fa3 100644 --- a/src/utils/fileLock.ts +++ b/src/utils/fileLock.ts @@ -54,35 +54,44 @@ function nearestExistingDirectory(absolutePath: string): { directory: string; re } /** - * The spelling a lock artefact should be created under: the canonical spelling of the - * nearest existing directory, re-joined with the components that do not exist yet. This is - * what makes an 8.3 alias and its long form take one lock, and it stays a spelling - the key - * above is the identity, and callers that compare locks compare that. + * The identity a lock is keyed on: the device and inode of the nearest directory that actually + * exists, plus the components below it that do not exist yet. An 8.3 alias and its long form, a + * link and its target, and a case variant on a case-insensitive volume all arrive at the same + * identity, whether or not the leaf has been created. That is the case a canonical spelling + * cannot serve: a leaf that does not exist yet has no canonical spelling, which is what the + * `realpath: false` in `acquireFileLock` is there for. And nothing here rewrites a caller's + * path - the key is for comparing locks, never for opening files - so the strings callers hand + * to unlink and readFile stay the strings those callers built. */ -export function canonicalLockPath(filePath: string): string { +export function canonicalLockKey(filePath: string): string { const absolute = path.resolve(filePath) - if (statIdentity(absolute) !== null) { - try { - return fs.realpathSync.native(absolute) - } catch { - return absolute - } - } const { directory, relative } = nearestExistingDirectory(absolute) - let canonicalDirectory = directory - try { - canonicalDirectory = fs.realpathSync.native(directory) - } catch { - canonicalDirectory = directory + const identity = statIdentity(directory) + if (identity === null) { + // Not even a root of the path exists yet, so there is no identity to key on and the + // resolved path is the one thing every caller of it computes alike. + return absolute } - return relative.length === 0 ? canonicalDirectory : path.join(canonicalDirectory, ...relative) + return identity + ":" + relative.join("/") +} + +/** + * Where the artefact lives: beside the file in the caller's own spelling. The key above is the + * identity, and the artefact stays a path, because `safeWriteJson` and the interruption tests + * address `.lock` directly - moving it is what made a green suite red, so the location is + * pinned by a test rather than left to taste. + */ +function lockArtefactPath(absolutePath: string): string { + return absolutePath + ".lock" } export async function acquireFileLock(filePath: string, options?: FileLockOptions): Promise<() => Promise> { - const absoluteFilePath = canonicalLockPath(filePath) + const absoluteFilePath = path.resolve(filePath) + const lockfilePath = lockArtefactPath(absoluteFilePath) if (options?.waitForStaleLock) tracePersistenceEvent("json-lock-wait", { filePath: absoluteFilePath }) try { - return await lockfile.lock(absoluteFilePath, { + return await lockfile.lock(canonicalLockKey(absoluteFilePath), { + lockfilePath, 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 @@ -124,7 +133,7 @@ export async function withFileLock( operation: (absoluteFilePath: string) => Promise, options?: FileLockOptions, ): Promise { - const absoluteFilePath = canonicalLockPath(filePath) + const absoluteFilePath = path.resolve(filePath) const releaseLock = await acquireFileLock(absoluteFilePath, options) let result: T From 812ba359b624ccc86264b35b27369de7e40c2e00 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Mon, 9 Nov 2026 14:25:00 +0800 Subject: [PATCH 3/4] test(utils): decide cross-spelling exclusion with the real library The mocked suite keys its lock table on the string acquireFileLock hands the library and never looks at lockfilePath, so a change that gave two spellings different on-disk artefacts would have left it green. Delegate the mock to the real proper-lockfile for one suite and assert, for a junction and for an 8.3 alias, that the two spellings resolve to one artefact inode and that the second acquire rejects ELOCKED while the first holds it. Placing the artefact by the caller's spelling reddens exactly those two rows and leaves the key-equality rows green, which is the coverage the review asked for. Create each suite's temporary root in beforeAll and remove it in afterAll: the roots were made during collection and survived every run of the file, including runs where the alias rows were skipped. --- src/utils/__tests__/fileLock.test.ts | 166 ++++++++++++++++++++++----- 1 file changed, 136 insertions(+), 30 deletions(-) diff --git a/src/utils/__tests__/fileLock.test.ts b/src/utils/__tests__/fileLock.test.ts index d2ec7dfbdc..58e1dbad2c 100644 --- a/src/utils/__tests__/fileLock.test.ts +++ b/src/utils/__tests__/fileLock.test.ts @@ -46,8 +46,8 @@ describe("file lock protocol", () => { * directory name and skips with a reason where generation is disabled. Asserting alias * unification on a volume with generation off would produce a green that cannot fail, which is * worse than a skip that says what it did. Locally the probe found a short name (A-REAL~1 for a - * 47-character name) and the alias case below executed and passed; the linux leg does not - * execute it. The name is deliberately not passed as a dir argument: dir /x lists that + * 47-character name) and the alias cases below executed and passed; the linux leg does not + * execute them. The name is deliberately not passed as a dir argument: dir /x lists that * directory's contents, whose header line is the only line containing the name, which reads as * generation being off when it is not. */ @@ -69,20 +69,63 @@ function shortNamesGenerated(directory: string): boolean { } } +/** + * The 8.3 alias of a path, read from the dir /x listing of its directory. Used by both the + * key-equality suite and the real-lock suite below, so it lives at module scope: a volume with + * generation off returns the long path unchanged, which is why every caller gates on + * shortNamesGenerated first. + */ +function aliasOf(longPath: string): string { + const directory = path.dirname(longPath) + const listing = spawnSync("cmd", ["/c", "dir", "/x"], { cwd: directory, encoding: "utf8" }) as unknown as { + stdout?: string + } + const line = + String(listing.stdout ?? "") + .split(/\r?\n/) + .find((entry) => entry.includes(path.basename(longPath))) ?? "" + const fields = line.split(/\s+/).filter((field) => field.length > 0) + const short = fields.find((field) => /~\d/.test(field)) + return short ? path.join(directory, short) : longPath +} + +/** + * A temporary root for one suite, created when that suite runs and removed when it ends, + * including when a test inside it fails. Creating the root in the describe body left one root + * and its fixtures on disk for every collection of this file, including runs where the suite's + * tests were skipped. + */ +function useTemporaryRoot(prefix: string): () => string { + let root = "" + beforeAll(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), prefix)) + }) + afterAll(() => { + if (root) { + fs.rmSync(root, { recursive: true, force: true }) + } + }) + return () => root +} + describe("one lock per file, not one lock per spelling", () => { - const root = fs.mkdtempSync(path.join(os.tmpdir(), "fileLock-key-")) - const generatesShortNames = process.platform === "win32" && shortNamesGenerated(root) + const root = useTemporaryRoot("fileLock-key-") + let generatesShortNames = false // proper-lockfile is mocked, so exclusivity here is a test-side lock table keyed by the exact // key acquireFileLock asks for: a key already held rejects with ELOCKED, and immediate: true // means retries 0. The property under test is whether two spellings of one file arrive as // one key or as two, which is exactly what that table can decide. const held: string[] = [] + beforeAll(() => { + generatesShortNames = process.platform === "win32" && shortNamesGenerated(root()) + }) beforeEach(() => { held.length = 0 vi.mocked(lockfile.lock).mockImplementation(async (target: unknown) => { // The table keys on the key acquireFileLock hands the library: the file's identity, not // either spelling. Two spellings of one file therefore arrive as one key, which is what - // this table can decide; the artefact location is asserted separately. + // this table can decide; whether they also land on one on-disk artefact is asserted + // separately, against the real library. const key = String(target) if (held.indexOf(key) >= 0) { throw Object.assign(new Error("ELOCKED: " + key), { code: "ELOCKED" }) @@ -96,22 +139,17 @@ describe("one lock per file, not one lock per spelling", () => { } }) }) - const aliasOf = (longPath: string): string => { - const directory = path.dirname(longPath) - const listing = spawnSync("cmd", ["/c", "dir", "/x"], { cwd: directory, encoding: "utf8" }) as unknown as { - stdout?: string - } - const line = - String(listing.stdout ?? "") - .split(/\r?\n/) - .find((entry) => entry.includes(path.basename(longPath))) ?? "" - const fields = line.split(/\s+/).filter((field) => field.length > 0) - const short = fields.find((field) => /~\d/.test(field)) - return short ? path.join(directory, short) : longPath - } - const itAlias = generatesShortNames ? it : it.skip + // The volume probe runs in beforeAll, so the gate is decided when the suite runs rather than + // when it is collected, and the alias tests skip with a reason instead of disappearing. + const itAlias = (name: string, fn: () => Promise) => + it(name, async (context) => { + if (!generatesShortNames) { + return context.skip() + } + await fn() + }) itAlias("takes one lock for an 8.3 alias and its long form, not a second one", async () => { - const longDir = path.join(root, "a-really-long-directory-name-for-alias-lock-testing") + const longDir = path.join(root(), "a-really-long-directory-name-for-alias-lock-testing") fs.mkdirSync(longDir, { recursive: true }) const file = path.join(longDir, "history.json") fs.writeFileSync(file, "{}", "utf8") @@ -124,7 +162,7 @@ describe("one lock per file, not one lock per spelling", () => { await releaseAlias() }) itAlias("hands each caller back its own spelling rather than the canonicalised one", async () => { - const longDir = path.join(root, "a-really-long-directory-name-for-spelling-control") + const longDir = path.join(root(), "a-really-long-directory-name-for-spelling-control") fs.mkdirSync(longDir, { recursive: true }) const file = path.join(longDir, "history.json") fs.writeFileSync(file, "{}", "utf8") @@ -144,8 +182,8 @@ describe("one lock per file, not one lock per spelling", () => { expect(seen[0]).not.toBe(seen[1]) }) it("takes separate locks for two different files in one directory", async () => { - const a = path.join(root, "a.json") - const b = path.join(root, "b.json") + const a = path.join(root(), "a.json") + const b = path.join(root(), "b.json") fs.writeFileSync(a, "{}", "utf8") fs.writeFileSync(b, "{}", "utf8") await acquireFileLock(a, { immediate: true }) @@ -153,8 +191,8 @@ describe("one lock per file, not one lock per spelling", () => { expect(held).toHaveLength(2) }) it("takes one lock for one not-yet-existing path and a different one for another name", async () => { - const missingA = path.join(root, "missing-a.json") - const missingB = path.join(root, "missing-b.json") + const missingA = path.join(root(), "missing-a.json") + const missingB = path.join(root(), "missing-b.json") await acquireFileLock(missingA, { immediate: true }) await expect(acquireFileLock(missingA, { immediate: true })).rejects.toMatchObject({ code: "ELOCKED" }) await acquireFileLock(missingB, { immediate: true }) @@ -169,11 +207,15 @@ describe("one lock per file, not one lock per spelling", () => { * fails rather than quietly disappearing. */ describe("two spellings of one referent share one key", () => { - const root = fs.mkdtempSync(path.join(os.tmpdir(), "fileLock-link-")) - const real = path.join(root, "real") - const link = path.join(root, "link") - fs.mkdirSync(real, { recursive: true }) - fs.symlinkSync(real, link, process.platform === "win32" ? "junction" : "dir") + const root = useTemporaryRoot("fileLock-link-") + let real = "" + let link = "" + beforeAll(() => { + real = path.join(root(), "real") + link = path.join(root(), "link") + fs.mkdirSync(real, { recursive: true }) + fs.symlinkSync(real, link, process.platform === "win32" ? "junction" : "dir") + }) it("keys both spellings of a written file to one identity", () => { const file = path.join(real, "history.json") @@ -199,3 +241,67 @@ describe("two spellings of one referent share one key", () => { expect(seen[1]).toBe(path.resolve(path.join(real, "history.json"))) }) }) + +/** + * The mocked suites above decide key equality and nothing else: the mock keys on the string + * acquireFileLock hands it and never looks at lockfilePath, so a change that gave two spellings + * two different on-disk artefacts would have left them green. These delegate the mock to the + * real library, the way the McpHub settings-creation integration spec does, so the outcome is + * decided by the artefact the filesystem resolves - which is the mutual-exclusion claim the + * alias-keyed fix actually makes. + */ +describe("two spellings contend with the real library", () => { + const root = useTemporaryRoot("fileLock-real-") + let generatesShortNames = false + let actualLock: typeof lockfile.lock + beforeAll(async () => { + actualLock = (await vi.importActual("proper-lockfile")).lock + generatesShortNames = process.platform === "win32" && shortNamesGenerated(root()) + }) + beforeEach(() => { + vi.mocked(lockfile.lock).mockImplementation((target, options) => actualLock(target, options)) + }) + + // Both spellings name one directory once the filesystem resolves them, so the artefacts are + // one directory and the library refuses the second caller. Asserting that the artefacts + // share an inode is what pins this down: a placement keyed on the spelling would land two + // directories here, the second acquire would succeed, and the suite above would stay green. + async function expectOneLockAcrossSpellings(firstPath: string, secondPath: string): Promise { + fs.mkdirSync(path.dirname(firstPath), { recursive: true }) + fs.writeFileSync(firstPath, "{}", "utf8") + const release = await acquireFileLock(firstPath, { immediate: true }) + try { + expect(fs.existsSync(secondPath + ".lock")).toBe(true) + expect(fs.statSync(secondPath + ".lock").ino).toBe(fs.statSync(firstPath + ".lock").ino) + await expect(acquireFileLock(secondPath, { immediate: true })).rejects.toMatchObject({ + code: "ELOCKED", + }) + } finally { + await release() + } + const released = await acquireFileLock(secondPath, { immediate: true }) + await released() + expect(fs.existsSync(firstPath + ".lock")).toBe(false) + } + + it("blocks the other spelling of a junction path while one holds the real lock", async () => { + const real = path.join(root(), "real") + const link = path.join(root(), "link") + fs.mkdirSync(real, { recursive: true }) + fs.symlinkSync(real, link, process.platform === "win32" ? "junction" : "dir") + await expectOneLockAcrossSpellings(path.join(link, "history.json"), path.join(real, "history.json")) + }) + + it("blocks the other spelling of an 8.3 alias while one holds the real lock", async (context) => { + if (!generatesShortNames) { + return context.skip() + } + const longDir = path.join(root(), "a-really-long-directory-name-for-real-lock-alias-testing") + fs.mkdirSync(longDir, { recursive: true }) + const file = path.join(longDir, "history.json") + fs.writeFileSync(file, "{}", "utf8") + const aliasFile = path.join(aliasOf(longDir), "history.json") + expect(fs.statSync(aliasFile).ino).toBe(fs.statSync(file).ino) + await expectOneLockAcrossSpellings(aliasFile, file) + }) +}) From 20343f818a7af671c285e77818c7c7c27ff9bd2d Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Mon, 9 Nov 2026 15:12:59 +0800 Subject: [PATCH 4/4] fix(utils): read the lock identity in bigint so large inodes stay distinct fs.statSync hands back dev and ino as plain numbers unless BigInts are asked for, and above 2^53 an inode stops being a distinct number: 17451448568457636 and 17451448568457637 are adjacent integers and one double. Stringifying that number for the lock key therefore loses information silently and can key two neighbouring directories alike. Read the identity with { bigint: true }, which is lossless, and add a regression row whose statSync double answers plain stats with rounded numbers and BigInts only when asked - a double that always returned BigInts would be green before the fix. Co-Authored-By: Claude --- src/utils/__tests__/fileLock.test.ts | 74 ++++++++++++++++++++++++++++ src/utils/fileLock.ts | 5 +- 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/src/utils/__tests__/fileLock.test.ts b/src/utils/__tests__/fileLock.test.ts index 58e1dbad2c..6f3d2fb432 100644 --- a/src/utils/__tests__/fileLock.test.ts +++ b/src/utils/__tests__/fileLock.test.ts @@ -9,6 +9,27 @@ import { acquireFileLock, canonicalLockKey, LOCK_STALE_MS, withFileLock } from " vi.mock("proper-lockfile", () => ({ lock: vi.fn() })) +type StatSyncOverride = ( + target: string, + options?: { bigint?: boolean }, +) => { dev: number | bigint; ino: number | bigint } + +/** + * A delegating filesystem double, armed only by the inode-precision suite below. Everything else + * in this file keeps using the real filesystem, because the wrapper forwards to the actual module + * whenever no override is installed. + */ +let statSyncOverride: StatSyncOverride | null = null + +vi.mock("fs", async (importOriginal) => { + const actual = await importOriginal() + const statSync = ((target: string, options?: unknown) => + statSyncOverride === null + ? (actual.statSync as (path: string, opts?: unknown) => unknown)(target, options) + : statSyncOverride(target, options as { bigint?: boolean } | undefined)) as typeof actual.statSync + return { ...actual, statSync } +}) + describe("file lock protocol", () => { beforeEach(() => vi.clearAllMocks()) @@ -305,3 +326,56 @@ describe("two spellings contend with the real library", () => { await expectOneLockAcrossSpellings(aliasFile, file) }) }) + +/** + * The identity is a string built from device and inode, and node hands those over as plain numbers + * unless asked otherwise. Above 2^53 an inode is no longer a distinct number - 17451448568457636 + * and 17451448568457637 are adjacent integers and one double - so the identity has to be read with + * `{ bigint: true }` or two neighbouring directories collapse into one key and one lock. + * + * The double is the row: it answers a plain stat with **rounded numbers**, the way node does, and + * hands back BigInts only when the caller asks for them. A double that returned BigInts + * unconditionally would read green before the fix and prove nothing about the numeric path. + */ +describe("identity keeps inodes above 2^53 distinct", () => { + const inodeLow = 17451448568457636n + const inodeHigh = 17451448568457637n + const dev = 7n + // Two existing directories one inode apart, each holding a file of the same name, so the only + // thing that can separate the two keys is the inode the identity is built from. These paths are + // synthetic - the fake statSync answers for them - so nothing is created on disk. + const precisionRoot = path.resolve("zoo-identity-precision") + const directoryLow = path.join(precisionRoot, "a") + const directoryHigh = path.join(precisionRoot, "b") + + let statCalls: Array<{ target: string; bigintRequested: boolean }> = [] + beforeEach(() => { + statCalls = [] + statSyncOverride = (target, options) => { + const bigintRequested = options?.bigint === true + statCalls.push({ target, bigintRequested }) + const ino = target === directoryLow ? inodeLow : target === directoryHigh ? inodeHigh : 1n + // Fidelity: BigInts only when they were asked for, rounded numbers otherwise. + return bigintRequested ? { dev, ino } : { dev: Number(dev), ino: Number(ino) } + } + }) + afterEach(() => { + statSyncOverride = null + }) + + it("pairs the adjacent inodes named by the precision loss", () => { + expect(Number(inodeLow)).toBe(Number(inodeHigh)) + expect(String(inodeLow)).not.toBe(String(inodeHigh)) + }) + + it("keys two directories one inode apart above 2^53 by different identities", () => { + const low = canonicalLockKey(path.join(directoryLow, "history.json")) + const high = canonicalLockKey(path.join(directoryHigh, "history.json")) + expect(low).not.toBe(high) + }) + + it("asks the filesystem for the identity in bigint form", () => { + canonicalLockKey(path.join(directoryLow, "history.json")) + expect(statCalls.some((call) => call.bigintRequested)).toBe(true) + }) +}) diff --git a/src/utils/fileLock.ts b/src/utils/fileLock.ts index 912f739fa3..0ea669e157 100644 --- a/src/utils/fileLock.ts +++ b/src/utils/fileLock.ts @@ -29,7 +29,10 @@ export interface FileLockOptions { */ function statIdentity(target: string): string | null { try { - const stats = fs.statSync(target) + // BigInt stats, because the identity is a string and a plain number is not lossless: two + // adjacent inodes above 2^53 are the same Number, so the numeric form can key two + // neighbouring directories alike and let them share a lock nobody asked for. + const stats = fs.statSync(target, { bigint: true }) return String(stats.dev) + ":" + String(stats.ino) } catch { return null