diff --git a/src/codex/shim-restore-lock.ts b/src/codex/shim-restore-lock.ts index 27d1eae8a78..1de3f257519 100644 --- a/src/codex/shim-restore-lock.ts +++ b/src/codex/shim-restore-lock.ts @@ -32,70 +32,107 @@ interface ShimRestoreLockRecord { createdAt: number; } -interface ShimRestoreLockSnapshot { +interface ShimRestoreLockOwner { record: ShimRestoreLockRecord; ownerPath: string; - lockIdentity: FileIdentity; fingerprint: ShimPathFingerprint; } +interface ShimRestoreLockSnapshot { + lockIdentity: FileIdentity; + /** Sorted by owner filename so two snapshots compare entry by entry. */ + owners: ShimRestoreLockOwner[]; +} + function restoreLockPath(): string { return join(getConfigDir(), "codex-shim.autorestore.lock"); } +function readShimRestoreLockOwner(path: string, entry: string): ShimRestoreLockOwner | null { + if (!entry.endsWith(".json")) return null; + const ownerPath = join(path, entry); + const probe = stableShimPathProbe(ownerPath); + if (!probe || probe.fingerprint.kind !== "file" || probe.fingerprint.size > 4096) return null; + try { + const value = JSON.parse(probe.prefix) as Partial; + if (value.version !== 1 || typeof value.token !== "string" || value.token.length === 0 + || typeof value.pid !== "number" || !Number.isSafeInteger(value.pid) || value.pid <= 0 + || typeof value.createdAt !== "number" || !Number.isFinite(value.createdAt)) return null; + if (entry !== `${value.token}.json`) return null; + return { record: value as ShimRestoreLockRecord, ownerPath, fingerprint: probe.fingerprint }; + } catch { + return null; + } +} + +/** + * Every entry must be a valid token-named owner record; one unaccounted entry (foreign file, + * unparsable or empty record, mismatched name) makes the whole lock unreadable. A normal lock + * holds one owner. Two or more are not produced by the current acquire path, but have been + * observed in the field (#8317) and must stay recoverable rather than block repair forever. + */ function readShimRestoreLockSnapshot(path: string): ShimRestoreLockSnapshot | null { let lockIdentity: BigIntStats; let entries: string[]; try { lockIdentity = lstatSync(path, { bigint: true }); if (!lockIdentity.isDirectory()) return null; - entries = readdirSync(path); + entries = readdirSync(path).sort(); } catch { return null; } - if (entries.length !== 1 || !entries[0].endsWith(".json")) return null; - const ownerPath = join(path, entries[0]); - const probe = stableShimPathProbe(ownerPath); - if (!probe || probe.fingerprint.kind !== "file" || probe.fingerprint.size > 4096) return null; + if (entries.length === 0) return null; + const owners: ShimRestoreLockOwner[] = []; + for (const entry of entries) { + const owner = readShimRestoreLockOwner(path, entry); + if (!owner) return null; + owners.push(owner); + } try { - const value = JSON.parse(probe.prefix) as Partial; - if (value.version !== 1 || typeof value.token !== "string" || value.token.length === 0 - || typeof value.pid !== "number" || !Number.isSafeInteger(value.pid) || value.pid <= 0 - || typeof value.createdAt !== "number" || !Number.isFinite(value.createdAt)) return null; - if (entries[0] !== `${value.token}.json`) return null; const currentLockIdentity = lstatSync(path, { bigint: true }); if (!currentLockIdentity.isDirectory() || !sameFileIdentity(lockIdentity, currentLockIdentity)) return null; - return { - record: value as ShimRestoreLockRecord, - ownerPath, - lockIdentity, - fingerprint: probe.fingerprint, - }; } catch { return null; } + return { lockIdentity, owners }; +} + +/** Release only ever removes a lock it solely owns; an ambiguous set is left to stale recovery. */ +function soleShimRestoreLockOwner(path: string): ShimRestoreLockOwner | null { + const snapshot = readShimRestoreLockSnapshot(path); + return snapshot?.owners.length === 1 ? snapshot.owners[0] : null; } function sameShimRestoreLock(left: ShimRestoreLockSnapshot, right: ShimRestoreLockSnapshot): boolean { - return left.record.token === right.record.token - && sameFileIdentity(left.lockIdentity, right.lockIdentity) - && sameFingerprint(left.fingerprint, right.fingerprint); + return sameFileIdentity(left.lockIdentity, right.lockIdentity) + && left.owners.length === right.owners.length + && left.owners.every((owner, index) => owner.record.token === right.owners[index].record.token + && sameFingerprint(owner.fingerprint, right.owners[index].fingerprint)); } +/** + * Reclaim requires every owner record to be past the stale window and to name a dead PID. + * Any live, fresh or unaccounted entry keeps the lock: age alone never evicts a live holder, + * and an entry this code cannot attribute might belong to an owner it cannot judge. + */ function reclaimStaleRestoreLock(path: string, beforeDelete?: () => void): boolean { const observed = readShimRestoreLockSnapshot(path); if (!observed) return false; - const createdAt = Math.max(observed.record.createdAt, observed.fingerprint.mtimeMs); - if (Date.now() - createdAt <= CODEX_SHIM_RESTORE_LOCK_STALE_MS) return false; - if (isProcessAlive(observed.record.pid)) return false; + const now = Date.now(); + for (const owner of observed.owners) { + const createdAt = Math.max(owner.record.createdAt, owner.fingerprint.mtimeMs); + if (now - createdAt <= CODEX_SHIM_RESTORE_LOCK_STALE_MS) return false; + if (isProcessAlive(owner.record.pid)) return false; + } const current = readShimRestoreLockSnapshot(path); if (!current || !sameShimRestoreLock(observed, current)) return false; beforeDelete?.(); try { - // The token is part of the owner filename. Even if the lock directory is - // replaced after the comparison, this unlink cannot target a successor's - // differently named owner record. - unlinkSync(observed.ownerPath); + // The token is part of each owner filename. Even if the lock directory is + // replaced after the comparison, these unlinks cannot target a successor's + // differently named owner record. A failure part-way leaves a smaller set + // of the same dead owners, which the next attempt reclaims. + for (const owner of observed.owners) unlinkSync(owner.ownerPath); rmdirSync(path); return true; } catch { @@ -132,7 +169,7 @@ function tryAcquireShimRestoreLock(beforeStaleDelete?: () => void): ShimRestoreL released = true; try { closeSync(fd!); } catch { /* stale recovery handles an uncertain lock */ } try { - const current = readShimRestoreLockSnapshot(path); + const current = soleShimRestoreLockOwner(path); if (identity && current && current.record.token === record.token && sameFileIdentity(identity, current.fingerprint.identity)) { unlinkSync(ownerPath); @@ -145,7 +182,7 @@ function tryAcquireShimRestoreLock(beforeStaleDelete?: () => void): ShimRestoreL if (fd !== null) { try { closeSync(fd); } catch { /* best-effort close before ownership cleanup */ } try { - const current = readShimRestoreLockSnapshot(path); + const current = soleShimRestoreLockOwner(path); if (identity && current && current.record.token === record.token && sameFileIdentity(identity, current.fingerprint.identity)) { unlinkSync(ownerPath); diff --git a/structure/runtime.md b/structure/runtime.md index 1aa77595d12..d6e4f4e3aeb 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -293,7 +293,7 @@ atomic OS guarantee against unobserved PID reuse. An installed Codex shim is checked on eligible CLI startup with a direct-regular-file/1 MiB state bound and bounded metadata/prefix reads. `ocx codex` skips repair and owns handoff: unknown Windows CMD wrappers require complete bypass-aware template equality, otherwise launch is refused; content equality grants no Windows backing provenance. Other changing launchers defer and repair failures warn without changing their command's exit behavior. All mutations hold the shared atomic-mkdir interprocess lock and reread state before final revalidation, publication and state commit. Unique token-named owner records prevent release from naming a successor. -Aged locks are reclaimed only when the owner PID is dead and the same token, directory identity and owner fingerprint remain immediately before deletion; age alone never evicts a live holder. +Aged locks are reclaimed only when every entry is a valid token-named owner record, every owner is past the stale window with a dead PID, and the same directory identity, token set and owner fingerprints remain immediately before deletion; reclaim unlinks each owner by its token name, then the directory. Age alone never evicts a live holder, and an unaccounted entry blocks reclaim. A directory holding several dead owners (#8317) is reclaimed the same way; release still removes only a lock its caller solely owns. Windows repair retains complete sibling-set preflight, identical fingerprints/prefixes across a 100 ms observation interval, and reverse-order rollback if a later sibling changes. It restores stable external replacements without PATH rediscovery. Windows service/shim writes first admit the frozen selected runtime under the [write-preflight contract](ops/service-and-sidecars.md#windows-selected-runtime-write-preflight); orchestrated installs refuse Desktop supervision before selection and admit once before cleanup. Repair refuses Desktop before diagnosis, then checks ownership and auth before admission and any native repair or scheduler stop. The Node launcher’s validated PATH Bun fallback precedes CLI startup; durable admission never discovers a replacement. The fresh scheduler path claims config ownership after registration and staging removal, then admits before `prepare()`; refusal rolls back the new registration and preserves the claimed config root. Standalone admission uses the current executable in-process, without child isolation or a timeout. Unix installation owns only the private home/bin/codex wrapper and codex-shell-env.sh. The native manager-owned launcher stays in place; its recorded durable entry follows package upgrades and version rollback. `src/codex/shim-state-file.ts` decodes schema 2, mode `path-overlay`, supported Unix platform, exact private `wrapperPath`, and distinct absolute `launcherPath`; contradictory compatibility fields are invalid. State can record wrapper/env inode identities and a transaction id. `src/codex/shim-overlay.ts` compares persisted wrapper/environment ownership by inode, allowing APFS device numbers to change across reboot. Exact private paths, current-user ownership, regular-file checks and group/world-write restrictions, wrapper marker and recorded launcher binding, and exact environment content remain required. Runnable wrappers also require current generated content and a usable native launcher. Inspection snapshots, namespace revalidation, activation and publication/recovery identities still compare device plus inode to refuse concurrent replacements. `tests/codex-integration/codex-shim-overlay.test.ts` covers persisted device drift and inode/content refusals; `tests/codex-integration/codex-shim-overlay-transaction.test.ts` covers same-inspection device drift and concurrent publication. diff --git a/tests/codex-integration/codex-shim.test.ts b/tests/codex-integration/codex-shim.test.ts index fc7819b90a7..3361ad40e6b 100644 --- a/tests/codex-integration/codex-shim.test.ts +++ b/tests/codex-integration/codex-shim.test.ts @@ -1858,6 +1858,54 @@ exit 127 }); }); + // #8317: a lock directory holding several aged owner records used to be unreadable, so it was never reclaimed. + function seedAgedLockEntries(lockPath: string, pids: number[], extra: string[] = []): string[] { + mkdirSync(lockPath); + pids.forEach((pid, index) => { + const ownerPath = join(lockPath, `aged-owner-${index}.json`); + writeFileSync(ownerPath, `${JSON.stringify({ version: 1, token: `aged-owner-${index}`, pid, createdAt: 0 })}\n`, "utf8"); + utimesSync(ownerPath, new Date(0), new Date(0)); + }); + for (const name of extra) { + writeFileSync(join(lockPath, name), "unaccounted\n", "utf8"); + utimesSync(join(lockPath, name), new Date(0), new Date(0)); + } + return readdirSync(lockPath).sort(); + } + + test("an aged lock whose owner records all name dead pids is reclaimed", () => { + withInstalledShim(({ home, wrappers }) => { + const lockPath = join(home, "codex-shim.autorestore.lock"); + wrappers.forEach((path, index) => process.platform === "win32" + ? writeFileSync(path, successfulLauncher(`two-dead-owners-${index}`), "utf8") : chmodSync(path, 0o644)); + seedAgedLockEntries(lockPath, [2_147_483_647, 2_147_483_646]); + + expect(autoRestoreCodexShim({ enabled: () => true, stabilitySleep: skipStabilityWait }).status).toBe("restored"); + expect(existsSync(lockPath)).toBe(false); + wrappers.forEach(path => expect(readFileSync(path, "utf8")).toContain(SHIM_MARKER)); + }); + }); + + for (const [label, pids, extra] of [ + ["one live owner", [2_147_483_647, process.pid], []], + ["an unaccounted entry", [2_147_483_647, 2_147_483_646], ["stray.tmp"]], + ] as const) { + test(`an aged multi-owner lock with ${label} is never reclaimed`, () => { + withInstalledShim(({ home, wrappers }) => { + const lockPath = join(home, "codex-shim.autorestore.lock"); + const oldWrappers = wrappers.map(path => readFileSync(path)); + wrappers.forEach((path, index) => process.platform === "win32" + ? writeFileSync(path, `replacement-${index}\n`, "utf8") : chmodSync(path, 0o644)); + const entries = seedAgedLockEntries(lockPath, [...pids], [...extra]); + + expect(autoRestoreCodexShim({ enabled: () => true, stabilitySleep: skipStabilityWait })).toEqual({ status: "deferred" }); + expect(readdirSync(lockPath).sort()).toEqual(entries); + wrappers.forEach((path, index) => process.platform === "win32" + ? expect(readFileSync(path, "utf8")).toBe(`replacement-${index}\n`) : expect(readFileSync(path)).toEqual(oldWrappers[index])); + }); + }); + } + test("stalled partial write changing during the observation interval is never promoted", () => { withInstalledShim(({ wrappers, backups }) => { if (process.platform !== "win32") {