diff --git a/src/utils/__tests__/fileLock.test.ts b/src/utils/__tests__/fileLock.test.ts index 92637fe7a0..6f3d2fb432 100644 --- a/src/utils/__tests__/fileLock.test.ts +++ b/src/utils/__tests__/fileLock.test.ts @@ -1,8 +1,35 @@ +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, canonicalLockKey, LOCK_STALE_MS, withFileLock } from "../fileLock" 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()) @@ -15,6 +42,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 }, }), ) @@ -32,3 +60,322 @@ 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 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. + */ +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 }) + } +} + +/** + * 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 = 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; 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" }) + } + held.push(key) + return async () => { + const at = held.indexOf(key) + if (at >= 0) { + held.splice(at, 1) + } + } + }) + }) + // 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") + 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(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") + 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) + }) +}) + +/** + * 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 = 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") + 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"))) + }) +}) + +/** + * 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) + }) +}) + +/** + * 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 49807ea154..0ea669e157 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,11 +27,74 @@ 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 { + // 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 + } +} + +/** + * 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 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 canonicalLockKey(filePath: string): string { + const absolute = path.resolve(filePath) + const { directory, relative } = nearestExistingDirectory(absolute) + 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 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 = 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