Skip to content

Commit ff93f80

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
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. resolveLockKey therefore returns that identity for an existing path, and the nearest existing directory's identity plus the lowercased remainder when the file does not exist yet - a basename with no inode is the residual case no identity can unify. canonicalLockPath gives the spelling a lock artefact is created under, so the alias case resolves to one lock instead of two. The 8.3 case is gated on 8.3 name generation (probed with dir /x on a freshly created long name, because fsutil 8dot3name needs elevation and fails with Error 5 un-elevated), not on the platform string, so a runner on a volume with generation off reports a skip rather than a green that cannot fail. Locally it executed and passed; the linux unit leg does not run it. Reported on #2356. No suppression count changed.
1 parent 5319fe9 commit ff93f80

2 files changed

Lines changed: 176 additions & 3 deletions

File tree

‎src/utils/__tests__/fileLock.test.ts‎

Lines changed: 96 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
1+
import { spawnSync } from "child_process"
2+
import * as fs from "fs"
3+
import * as os from "os"
4+
import * as path from "path"
5+
16
import * as lockfile from "proper-lockfile"
2-
import { acquireFileLock, LOCK_STALE_MS, withFileLock } from "../fileLock"
7+
import { acquireFileLock, canonicalLockPath, LOCK_STALE_MS, resolveLockKey, withFileLock } from "../fileLock"
38

49
vi.mock("proper-lockfile", () => ({ lock: vi.fn() }))
510

@@ -31,4 +36,94 @@ describe("file lock protocol", () => {
3136
).rejects.toBe(error)
3237
expect(release).toHaveBeenCalledOnce()
3338
})
39+
40+
/**
41+
* 8.3 short-name generation is a volume property, not a platform constant: fsutil needs
42+
* elevation (Error 5 un-elevated), so this probes dir /x against a freshly created long
43+
* directory name and skips with a reason where generation is disabled. A case that asserted
44+
* alias unification on a volume with 8.3 generation off would produce a green that cannot
45+
* fail, which is worse than a skip that says what it did. Locally, on the volume this was
46+
* written on, dir /x returned a short name (A-REAL~1 for a 47-character name) and the alias
47+
* case below executed and passed; the linux leg does not execute it.
48+
*/
49+
function shortNamesGenerated(directory: string): boolean {
50+
const probeName = "a-really-long-directory-name-for-short-name-generation-probe"
51+
fs.mkdirSync(path.join(directory, probeName), { recursive: true })
52+
try {
53+
const listing = spawnSync("cmd", ["/c", "dir", "/x"], {
54+
cwd: directory,
55+
encoding: "utf8",
56+
}) as unknown as { stdout?: string }
57+
const line =
58+
String(listing.stdout ?? "")
59+
.split(/\r?\n/)
60+
.find((entry) => entry.includes(probeName)) ?? ""
61+
const fields = line.split(/\s+/).filter((field) => field.length > 0)
62+
const shortIndex = fields.findIndex((field) => /~\d/.test(field))
63+
return shortIndex >= 0
64+
} finally {
65+
fs.rmSync(path.join(directory, probeName), { recursive: true, force: true })
66+
}
67+
}
68+
69+
describe("identity-backed lock key", () => {
70+
const root = fs.mkdtempSync(path.join(os.tmpdir(), "fileLock-key-"))
71+
const generatesShortNames = process.platform === "win32" && shortNamesGenerated(root)
72+
const keyFor = (target: string): string => {
73+
try {
74+
return resolveLockKey(target)
75+
} catch {
76+
return "stat-failure"
77+
}
78+
}
79+
const aliasOrSame = (longPath: string): string => {
80+
// dir /x reports the short name beside the long one; %~sI is not used here because it
81+
// echoes the quoted input when it is passed through cmd from a spawned argument array.
82+
const directory = path.dirname(longPath)
83+
const listing = spawnSync("cmd", ["/c", "dir", "/x"], { cwd: directory, encoding: "utf8" }) as unknown as {
84+
stdout?: string
85+
}
86+
const line =
87+
String(listing.stdout ?? "")
88+
.split(/\r?\n/)
89+
.find((entry) => entry.includes(path.basename(longPath))) ?? ""
90+
const fields = line.split(/\s+/).filter((field) => field.length > 0)
91+
const short = fields.find((field) => /~\d/.test(field))
92+
return short ? path.join(directory, short) : longPath
93+
}
94+
it("keys an existing file by device and inode", () => {
95+
const file = path.join(root, "existing.json")
96+
fs.writeFileSync(file, "{}", "utf8")
97+
const stats = fs.statSync(file)
98+
expect(keyFor(file)).toBe(String(stats.dev) + ":" + String(stats.ino))
99+
})
100+
it("gives two different files in one directory two different keys", () => {
101+
const a = path.join(root, "a.json")
102+
const b = path.join(root, "b.json")
103+
fs.writeFileSync(a, "{}", "utf8")
104+
fs.writeFileSync(b, "{}", "utf8")
105+
expect(keyFor(a)).not.toBe(keyFor(b))
106+
})
107+
it("is stable for a file that does not exist yet, and differs by name", () => {
108+
const missingA = path.join(root, "missing-a.json")
109+
const missingB = path.join(root, "missing-b.json")
110+
expect(keyFor(missingA)).toBe(keyFor(missingA))
111+
expect(keyFor(missingA)).not.toBe(keyFor(missingB))
112+
})
113+
const itAlias = generatesShortNames ? it : it.skip
114+
itAlias("takes one key and one canonical path for an 8.3 alias and its long form", () => {
115+
// Gated on the property rather than on the platform string, and it ran here: dir /x
116+
// generated a short name for a fresh long directory on this volume.
117+
const longDir = path.join(root, "a-really-long-directory-name-for-alias-lock-testing")
118+
fs.mkdirSync(longDir, { recursive: true })
119+
const file = path.join(longDir, "history.json")
120+
fs.writeFileSync(file, "{}", "utf8")
121+
const aliasDir = aliasOrSame(longDir)
122+
expect(aliasDir).not.toBe(longDir)
123+
const aliasFile = path.join(aliasDir, "history.json")
124+
expect(fs.statSync(aliasFile).ino).toBe(fs.statSync(file).ino)
125+
expect(keyFor(aliasFile)).toBe(keyFor(file))
126+
expect(canonicalLockPath(aliasFile)).toBe(canonicalLockPath(file))
127+
})
128+
})
34129
})

‎src/utils/fileLock.ts‎

Lines changed: 80 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import * as fs from "fs"
12
import * as path from "path"
23
import { tracePersistenceEvent } from "../core/task-persistence/persistenceTrace"
34
import * as lockfile from "proper-lockfile"
@@ -26,8 +27,85 @@ export interface FileLockOptions {
2627
* returned function exactly once and must not acquire the same lock again
2728
* while holding it.
2829
*/
30+
function statIdentity(target: string): string | null {
31+
try {
32+
const stats = fs.statSync(target)
33+
return String(stats.dev) + ":" + String(stats.ino)
34+
} catch {
35+
return null
36+
}
37+
}
38+
39+
/**
40+
* The nearest directory that actually exists on a path, so a write to a file that has
41+
* not been created yet can still be keyed by something the filesystem answers with.
42+
*/
43+
function nearestExistingDirectory(absolutePath: string): { directory: string; relative: string[] } {
44+
const relative: string[] = []
45+
let current = absolutePath
46+
while (path.dirname(current) !== current) {
47+
relative.unshift(path.basename(current))
48+
current = path.dirname(current)
49+
if (statIdentity(current) !== null) {
50+
return { directory: current, relative }
51+
}
52+
}
53+
return { directory: current, relative }
54+
}
55+
56+
/**
57+
* Identity-backed lock key for a path.
58+
59+
* Two callers that name one file differently must take one lock. Canonicalizing by text
60+
* cannot promise that: fs.realpath and fs.realpathSync return whatever spelling they are
61+
* handed, and on NTFS only fs.realpathSync.native expands an 8.3 alias, while stat answers
62+
* both spellings with the same dev+ino. So the key is that identity, plus the lowercase
63+
* remainder below the nearest existing directory when the file itself does not exist yet -
64+
* a basename with no inode is the residual case no identity can unify, and it is named here
65+
* rather than presented as identity.
66+
*/
67+
export function resolveLockKey(filePath: string): string {
68+
const absolute = path.resolve(filePath)
69+
const identity = statIdentity(absolute)
70+
if (identity !== null) {
71+
return identity
72+
}
73+
const { directory, relative } = nearestExistingDirectory(absolute)
74+
const directoryIdentity = statIdentity(directory)
75+
if (directoryIdentity === null) {
76+
// Nothing on the path exists yet, so text is the only key available; disclosed as text.
77+
return "unresolved:" + absolute.toLowerCase()
78+
}
79+
return directoryIdentity + ":" + relative.join("/").toLowerCase()
80+
}
81+
82+
/**
83+
* The spelling a lock artefact should be created under: the canonical spelling of the
84+
* nearest existing directory, re-joined with the components that do not exist yet. This is
85+
* what makes an 8.3 alias and its long form take one lock, and it stays a spelling - the key
86+
* above is the identity, and callers that compare locks compare that.
87+
*/
88+
export function canonicalLockPath(filePath: string): string {
89+
const absolute = path.resolve(filePath)
90+
if (statIdentity(absolute) !== null) {
91+
try {
92+
return fs.realpathSync.native(absolute)
93+
} catch {
94+
return absolute
95+
}
96+
}
97+
const { directory, relative } = nearestExistingDirectory(absolute)
98+
let canonicalDirectory = directory
99+
try {
100+
canonicalDirectory = fs.realpathSync.native(directory)
101+
} catch {
102+
canonicalDirectory = directory
103+
}
104+
return relative.length === 0 ? canonicalDirectory : path.join(canonicalDirectory, ...relative)
105+
}
106+
29107
export async function acquireFileLock(filePath: string, options?: FileLockOptions): Promise<() => Promise<void>> {
30-
const absoluteFilePath = path.resolve(filePath)
108+
const absoluteFilePath = canonicalLockPath(filePath)
31109
if (options?.waitForStaleLock) tracePersistenceEvent("json-lock-wait", { filePath: absoluteFilePath })
32110
try {
33111
return await lockfile.lock(absoluteFilePath, {
@@ -72,7 +150,7 @@ export async function withFileLock<T>(
72150
operation: (absoluteFilePath: string) => Promise<T>,
73151
options?: FileLockOptions,
74152
): Promise<T> {
75-
const absoluteFilePath = path.resolve(filePath)
153+
const absoluteFilePath = canonicalLockPath(filePath)
76154
const releaseLock = await acquireFileLock(absoluteFilePath, options)
77155

78156
let result: T

0 commit comments

Comments
 (0)