diff --git a/src/utils/__tests__/fileLock.test.ts b/src/utils/__tests__/fileLock.test.ts index 92637fe7a0..d2ec7dfbdc 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, canonicalLockKey, LOCK_STALE_MS, withFileLock } from "../fileLock" vi.mock("proper-lockfile", () => ({ lock: vi.fn() })) @@ -15,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 }, }), ) @@ -32,3 +39,163 @@ 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) => { + // 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" }) + } + 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(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 = 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 49807ea154..912f739fa3 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,71 @@ 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 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