diff --git a/src/codex/shim-windows-refresh.ts b/src/codex/shim-windows-refresh.ts index e80eedd0f8f..2821fb3b372 100644 --- a/src/codex/shim-windows-refresh.ts +++ b/src/codex/shim-windows-refresh.ts @@ -3,10 +3,20 @@ import { closeSync, constants, existsSync, fstatSync, linkSync, lstatSync, openS import { dirname, extname, join } from "node:path"; import { sameFingerprintAfterRename, sameStableShimPathProbe, shimPathFingerprint, stableShimPathProbe, type StableShimPathProbe } from "./shim-fingerprint"; -interface FileIdentity { dev: bigint; ino: bigint } +/** + * Identity of a directory entry: device, inode and whether the entry is itself a symbolic link. + * + * dev/ino alone is not an identity on Linux: ext4 and tmpfs hand a freed inode number straight to the + * next file or link created, so a stage unlinked and replaced by a link (or a new file) can carry the + * very numbers this refresh recorded. Every identity this module compares therefore belongs to a file + * it still holds open (an open inode cannot be freed, so its number cannot be reused), and the link + * flag is compared too, so a symlink never matches a regular file whatever the platform's link(2) + * does with a symlink source. + */ +interface EntryIdentity { dev: bigint; ino: bigint; link: boolean } /** Identity of the directory entry itself (a link is its own entry); null when absent. */ -function entryIdentity(path: string): (FileIdentity & { link: boolean }) | null { +function entryIdentity(path: string): EntryIdentity | null { try { const entry = lstatSync(path, { bigint: true }); return { dev: entry.dev, ino: entry.ino, link: entry.isSymbolicLink() }; @@ -16,52 +26,78 @@ function entryIdentity(path: string): (FileIdentity & { link: boolean }) | null } /** Identity of what a path resolves to; null when absent or dangling. */ -function targetIdentity(path: string): FileIdentity | null { +function targetIdentity(path: string): EntryIdentity | null { try { const target = statSync(path, { bigint: true }); - return { dev: target.dev, ino: target.ino }; + return { dev: target.dev, ino: target.ino, link: false }; } catch { return null; } } -const same = (left: FileIdentity | null, right: FileIdentity | null): boolean => - !!left && !!right && left.dev === right.dev && left.ino === right.ino; +const same = (left: EntryIdentity | null, right: EntryIdentity | null): boolean => + !!left && !!right && left.dev === right.dev && left.ino === right.ino && left.link === right.link; -/** A regular, non-link entry at `path` that is exactly `identity`. */ -function entryIs(path: string, identity: FileIdentity): boolean { - const entry = entryIdentity(path); - return !!entry && !entry.link && same(entry, identity); +/** The entry at `path` is exactly `identity` (same inode, same link-ness). */ +function entryIs(path: string, identity: EntryIdentity): boolean { + return same(entryIdentity(path), identity); +} + +/** An open regular file and its identity; the descriptor pins the inode for as long as it is open. */ +interface HeldFile { fd: number; identity: EntryIdentity } + +function release(held: HeldFile | null): void { + if (held) { try { closeSync(held.fd); } catch { /* already closed */ } } } -/** Create `path` exclusively (never through an existing entry or link) and return the written file's identity. */ -function createExclusive(path: string, content: string): FileIdentity { - const flags = constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL +/** Create `path` exclusively (never through an existing entry or link), write it, and keep it open. */ +function createExclusive(path: string, content: string): HeldFile { + const flags = constants.O_RDWR | constants.O_CREAT | constants.O_EXCL | (process.platform === "win32" ? 0 : constants.O_NOFOLLOW); const fd = openSync(path, flags, 0o644); try { writeFileSync(fd, content, "utf8"); const stat = fstatSync(fd, { bigint: true }); - return { dev: stat.dev, ino: stat.ino }; - } finally { - closeSync(fd); + return { fd, identity: { dev: stat.dev, ino: stat.ino, link: false } }; + } catch (error) { + try { closeSync(fd); } catch { /* already closed */ } + throw error; } } +/** Open the regular file at `path` without following a link and confirm the entry is the opened file. */ +function holdExisting(path: string): HeldFile | null { + const entry = entryIdentity(path); + if (!entry || entry.link) return null; + const flags = process.platform === "win32" + ? constants.O_RDONLY + : constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK; + let fd: number; + try { fd = openSync(path, flags); } catch { return null; } + try { + const stat = fstatSync(fd, { bigint: true }); + const identity: EntryIdentity = { dev: stat.dev, ino: stat.ino, link: false }; + if (stat.isFile() && same(identity, entry) && entryIs(path, identity)) return { fd, identity }; + } catch { /* unreadable descriptor: treated like a mismatch */ } + // Every unsuccessful path releases the descriptor; the caller then hands the moved file back. + try { closeSync(fd); } catch { /* already closed */ } + return null; +} + /** * Hard-link `source` onto the empty `destination` and keep the result only if the published entry * is exactly `expected`. If `source` was swapped (for another file or a link) between the caller's * check and the link, the entry this call created is withdrawn; an entry that matches neither the * source nor its target belongs to an independent later publisher and stays. */ -function linkExpected(source: string, destination: string, expected: FileIdentity): "published" | "occupied" | "refused" { +function linkExpected(source: string, destination: string, expected: EntryIdentity): "published" | "occupied" | "refused" { try { linkSync(source, destination); } catch (error) { return (error as NodeJS.ErrnoException).code === "EEXIST" ? "occupied" : "refused"; } const published = entryIdentity(destination); - if (published && same(published, expected)) return "published"; + if (same(published, expected)) return "published"; if (published && (same(published, entryIdentity(source)) || same(published, targetIdentity(source)))) { try { unlinkSync(destination); } catch { /* nothing to withdraw */ } } @@ -72,15 +108,16 @@ function linkExpected(source: string, destination: string, expected: FileIdentit function hardLinksWork(directory: string): boolean { const probe = join(directory, `.ocx-link-probe-${randomUUID()}`); const linked = `${probe}-linked`; - let identity: FileIdentity; - try { identity = createExclusive(probe, ""); } catch { return false; } + let held: HeldFile; + try { held = createExclusive(probe, ""); } catch { return false; } try { linkSync(probe, linked); - return entryIs(linked, identity); + return entryIs(linked, held.identity); } catch { return false; } finally { - for (const path of [linked, probe]) if (entryIs(path, identity)) { try { unlinkSync(path); } catch { /* best effort */ } } + for (const path of [linked, probe]) if (entryIs(path, held.identity)) { try { unlinkSync(path); } catch { /* best effort */ } } + release(held); } } @@ -89,15 +126,15 @@ function hardLinksWork(directory: string): boolean { * a file someone else published (an npm or version-manager install racing this refresh, or an entry * planted beside the wrapper). * - * 1. Move the observed wrapper aside and prove the moved file is the one that was probed; a - * different file means a concurrent replacement, which is put back untouched. - * 2. Create the stage exclusively under an unpredictable name and record its identity. + * 1. Move the observed wrapper aside, open it, and prove the moved file is the one that was probed; + * a different file means a concurrent replacement, which is put back untouched. + * 2. Create the stage exclusively under an unpredictable name and keep it open. * 3. Publish by hard link onto the empty path, keeping the result only if it is that stage file. * - * Every restore, withdrawal and cleanup is bound to identity: only the stage this refresh created - * and only the moved copy of the observed wrapper are ever published or unlinked. Returns true only - * when the current wrapper was published; a filesystem that refuses hard links leaves the obsolete - * wrapper restored and the refresh deferred. + * Every restore, withdrawal and cleanup is bound to the identity of a file this refresh holds open: + * only the stage it created and only the moved copy of the observed wrapper are ever published or + * unlinked. Returns true only when the current wrapper was published; a filesystem that refuses hard + * links leaves the obsolete wrapper restored and the refresh deferred. */ export function refreshObsoleteWindowsShim( wrapperPath: string, @@ -109,9 +146,10 @@ export function refreshObsoleteWindowsShim( const extension = extname(wrapperPath); const aside = `${wrapperPath}.ocx-obsolete-${randomUUID()}${extension}`; const staged = stagedPathForTests ?? `${wrapperPath}.ocx-refresh-${randomUUID()}${extension}`; - let moved: FileIdentity | null = null; - const discard = (path: string, identity: FileIdentity | null): void => { - if (identity && entryIs(path, identity)) { try { unlinkSync(path); } catch { /* already gone */ } } + let moved: HeldFile | null = null; + let stage: HeldFile | null = null; + const discard = (path: string, held: HeldFile | null): void => { + if (held && entryIs(path, held.identity)) { try { unlinkSync(path); } catch { /* already gone */ } } }; /** * Put the moved wrapper back only onto an empty path, and only while the aside is still it. If the @@ -119,47 +157,51 @@ export function refreshObsoleteWindowsShim( * launcher from staying missing. */ const restoreAside = (): void => { - if (!moved || existsSync(wrapperPath) || !entryIs(aside, moved)) return; - if (linkExpected(aside, wrapperPath, moved) === "published") { discard(aside, moved); return; } - if (!existsSync(wrapperPath) && entryIs(aside, moved)) { try { renameSync(aside, wrapperPath); } catch { /* the aside copy stays recoverable */ } } + if (!moved || existsSync(wrapperPath) || !entryIs(aside, moved.identity)) return; + if (linkExpected(aside, wrapperPath, moved.identity) === "published") { discard(aside, moved); return; } + if (!existsSync(wrapperPath) && entryIs(aside, moved.identity)) { try { renameSync(aside, wrapperPath); } catch { /* the aside copy stays recoverable */ } } }; // Never displace a working wrapper on a filesystem that cannot publish its replacement. if (!hardLinksWork(dirname(wrapperPath))) return false; try { renameSync(wrapperPath, aside); } catch { return false; } - const movedFingerprint = shimPathFingerprint(aside); - const movedEntry = entryIdentity(aside); - if (!movedFingerprint || !movedEntry || movedEntry.link || !sameFingerprintAfterRename(observed.fingerprint, movedFingerprint)) { - // Not the observed wrapper: hand the path back to whatever was moved, untouched. - if (movedEntry && !existsSync(wrapperPath)) { - if (linkExpected(aside, wrapperPath, movedEntry) === "published") discard(aside, movedEntry); - } - return false; - } - moved = movedEntry; - let created: FileIdentity; try { - created = createExclusive(staged, render()); - } catch (error) { - restoreAside(); - const code = (error as NodeJS.ErrnoException).code; - if (code === "EEXIST" || code === "ELOOP") return false; - throw error; - } - if (!entryIs(staged, created)) { - restoreAside(); + const movedFingerprint = shimPathFingerprint(aside); + moved = holdExisting(aside); + if (!moved || !movedFingerprint || !sameFingerprintAfterRename(observed.fingerprint, movedFingerprint)) { + // Not the observed wrapper: hand the path back to whatever was moved, untouched. + const foreign = entryIdentity(aside); + if (foreign && !existsSync(wrapperPath) && linkExpected(aside, wrapperPath, foreign) === "published" && entryIs(aside, foreign)) { + try { unlinkSync(aside); } catch { /* the restored link keeps the content */ } + } + return false; + } + try { + stage = createExclusive(staged, render()); + } catch (error) { + restoreAside(); + const code = (error as NodeJS.ErrnoException).code; + if (code === "EEXIST" || code === "ELOOP") return false; + throw error; + } + if (!entryIs(staged, stage.identity)) { + restoreAside(); + return false; + } + const outcome = linkExpected(staged, wrapperPath, stage.identity); + discard(staged, stage); + if (outcome === "published") { + discard(aside, moved); + return true; + } + // Occupied: a concurrent publisher owns the path now; only our obsolete copy is discarded. + if (outcome === "occupied") discard(aside, moved); + else restoreAside(); return false; + } finally { + release(stage); + release(moved); } - const outcome = linkExpected(staged, wrapperPath, created); - discard(staged, created); - if (outcome === "published") { - discard(aside, moved); - return true; - } - // Occupied: a concurrent publisher owns the path now; only our obsolete copy is discarded. - if (outcome === "occupied") discard(aside, moved); - else restoreAside(); - return false; } /** diff --git a/tests/ci-workflows/cold-spawn-warmup.test.ts b/tests/ci-workflows/cold-spawn-warmup.test.ts index 25cece9211f..4410a2f110a 100644 --- a/tests/ci-workflows/cold-spawn-warmup.test.ts +++ b/tests/ci-workflows/cold-spawn-warmup.test.ts @@ -109,6 +109,15 @@ const DISPOSITIONS: Readonly> = { + "graph, and an import scan has nothing to warm. The generated shim never loads a repository " + "module in the child: the point of the file is what the shell does with an exit status.", }, + "tests/codex-integration/codex-shim-bypass-token.test.ts": { + warmed: false, + why: + "Its children are the generated POSIX shim run by /bin/sh plus two throwaway /bin/sh scripts " + + "standing in for Bun's ensure and for the real Codex launcher, so the cold cost is shell and " + + "process startup rather than a repository module graph, and an import scan has nothing to " + + "warm. No repository module loads in the child: the file asserts only which environment the " + + "shell hands to the fake Codex under OCX_SHIM_BYPASS.", + }, }; /** diff --git a/tests/codex-integration/codex-shim-windows-revision.test.ts b/tests/codex-integration/codex-shim-windows-revision.test.ts index 8b07b3d5d10..1143f7a44e7 100644 --- a/tests/codex-integration/codex-shim-windows-revision.test.ts +++ b/tests/codex-integration/codex-shim-windows-revision.test.ts @@ -255,3 +255,33 @@ describe("round-5 availability", () => { expect(fs.readdirSync(dir).filter(name => name.includes(".ocx-"))).toEqual([]); }); }); + +describe("descriptor hygiene", () => { + test("a failing stat of the moved wrapper closes its descriptor and restores the launcher", () => { + const wrapper = join(dir, "codex.cmd"); + const obsolete = "@echo off\r\nrem obsolete\r\n"; + fs.writeFileSync(wrapper, obsolete); + const observed = stableShimPathProbe(wrapper)!; + const open = fs.openSync, close = fs.closeSync, fstat = fs.fstatSync; + const opened = new Set(); + const opening = spyOn(fs, "openSync").mockImplementation(((...args: Parameters) => { + const fd = open(...args); opened.add(fd); return fd; + }) as typeof fs.openSync); + const closing = spyOn(fs, "closeSync").mockImplementation(((fd: number) => { opened.delete(fd); return close(fd); }) as typeof fs.closeSync); + let injected = false; + const stating = spyOn(fs, "fstatSync").mockImplementation(((...args: Parameters) => { + if (!injected && fs.readdirSync(dir).some(name => name.includes(".ocx-obsolete-"))) { + injected = true; + throw Object.assign(new Error("injected fstat failure"), { code: "EIO" }); + } + return (fstat as (...a: unknown[]) => unknown)(...args); + }) as typeof fs.fstatSync); + try { + expect(refreshObsoleteWindowsShim(wrapper, observed, () => "current wrapper")).toBe(false); + } finally { stating.mockRestore(); closing.mockRestore(); opening.mockRestore(); } + expect(injected).toBe(true); + expect(opened.size).toBe(0); + expect(fs.readFileSync(wrapper, "utf8")).toBe(obsolete); + expect(fs.readdirSync(dir).filter(name => name.includes(".ocx-"))).toEqual([]); + }); +});