From 6af843f38e1d1e07356e68ad45d6e7b8c7a72cd0 Mon Sep 17 00:00:00 2001 From: JUN Date: Tue, 20 Oct 2026 07:42:07 +0900 Subject: [PATCH 1/3] fix(codex): pin Windows shim refresh identities with open descriptors On Linux (ext4/tmpfs) a freed inode number is handed straight to the next file or symlink, and link(2) of a symlink source publishes the symlink itself. The refresh compared bare dev/ino, so a stage or aside swapped for a symlink or new file could carry the recorded numbers and be published or restored as the launcher. The stage and the moved wrapper are now held open for the whole transaction (an open inode cannot be reused) and identities include whether the entry is a symlink, so the guarantee no longer depends on platform link(2) or inode allocation. Fixes #7401 --- src/codex/shim-windows-refresh.ts | 171 ++++++++++++++++++------------ 1 file changed, 106 insertions(+), 65 deletions(-) diff --git a/src/codex/shim-windows-refresh.ts b/src/codex/shim-windows-refresh.ts index e80eedd0f8f..962e67090e5 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,77 @@ 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; } + 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)) { + try { closeSync(fd); } catch { /* already closed */ } + return null; + } + return { fd, identity }; +} + /** * 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 +107,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 +125,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 +145,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 +156,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; } /** From 7060bbfb045784926b67b10bf1c2599ebf11286e Mon Sep 17 00:00:00 2001 From: JUN Date: Tue, 20 Oct 2026 07:42:07 +0900 Subject: [PATCH 2/3] test(ci): record the cold-spawn disposition for the shim bypass-token test Fixes #7400 --- tests/ci-workflows/cold-spawn-warmup.test.ts | 9 +++++++++ 1 file changed, 9 insertions(+) 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.", + }, }; /** From 5f66c1aefcef8ff34810fdaa4b1f294183e3d38c Mon Sep 17 00:00:00 2001 From: JUN Date: Tue, 20 Oct 2026 07:45:49 +0900 Subject: [PATCH 3/3] fix(codex): release the aside descriptor when its stat fails holdExisting now closes the descriptor on every unsuccessful path, including a throwing fstat, so the refresh hands the moved wrapper back instead of leaking the descriptor and leaving the launcher missing. Refs #7401 --- src/codex/shim-windows-refresh.ts | 15 +++++----- .../codex-shim-windows-revision.test.ts | 30 +++++++++++++++++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/codex/shim-windows-refresh.ts b/src/codex/shim-windows-refresh.ts index 962e67090e5..2821fb3b372 100644 --- a/src/codex/shim-windows-refresh.ts +++ b/src/codex/shim-windows-refresh.ts @@ -74,13 +74,14 @@ function holdExisting(path: string): HeldFile | null { : constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK; let fd: number; try { fd = openSync(path, flags); } catch { return null; } - 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)) { - try { closeSync(fd); } catch { /* already closed */ } - return null; - } - return { fd, identity }; + 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; } /** 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([]); + }); +});