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/2] 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/2] 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