diff --git a/AGENTS.md b/AGENTS.md index f9793312..95516ddc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -136,7 +136,8 @@ npm run vendor:verify # vendored dependency provenance check - Global accounts: `~/.codex/multi-auth/openai-codex-accounts.json`. - Official Codex state: `~/.codex/auth.json`, `~/.codex/accounts.json`, `~/.codex/config.toml`. - Runtime observability: `~/.codex/multi-auth/runtime-observability.json`. -- App helper status: `~/.codex/multi-auth/runtime-rotation-app-helper.json`. +- App helper status: `~/.codex/multi-auth/runtime-rotation-app-helper..json` (per helper; legacy un-suffixed file still read). +- App helper owner identity: `~/.codex/multi-auth/runtime-rotation-app-helper-owner..json` (per helper; removed on exit, swept once the helper PID is dead). - App bind state/logs: `~/.codex/multi-auth/app-bind/`. - Prompt templates sync from Codex CLI GitHub releases with ETag caching. - Historical audit evidence under `docs/audits/evidence/` is snapshot evidence, not current architecture guidance. diff --git a/README.md b/README.md index 32f5ffa2..506bec29 100644 --- a/README.md +++ b/README.md @@ -257,7 +257,7 @@ For remote or headless shells, prefer `codex-multi-auth login --device-auth`. | Routing profiles | `~/.codex/multi-auth/routing-profiles.json` | | Budget guards | `~/.codex/multi-auth/budget-guards.json` | | Local client tokens | `~/.codex/multi-auth/local-client-tokens.json` | -| Runtime app helper status | `~/.codex/multi-auth/runtime-rotation-app-helper.json` | +| Runtime app helper status | `~/.codex/multi-auth/runtime-rotation-app-helper..json` | | Runtime app helper owner metadata | `~/.codex/multi-auth/runtime-rotation-app-helper-owner..json` | | Persistent app bind state/logs | `~/.codex/multi-auth/app-bind/` | | Logs | `~/.codex/multi-auth/logs/codex-plugin/` | diff --git a/docs/configuration.md b/docs/configuration.md index 5bbd2417..4b5c3b7a 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -73,6 +73,8 @@ These are safe for most operators and frequently used in day-to-day workflows. | `CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0/1` | Opt out/in of live Codex Responses routing through the localhost account-rotation proxy | | `CODEX_MULTI_AUTH_FORCE_ACCOUNT=` | Force one account for a single forwarded `codex-multi-auth-codex` run (equivalent to the `--account` flag, which wins when both are set). Ephemeral and fail-hard; requires the runtime rotation proxy. See [Force an account for one invocation](reference/commands.md#force-an-account-for-one-invocation) | | `CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS=` | Override idle shutdown for the wrapper-launched Codex app helper | +| `CODEX_MULTI_AUTH_APP_ROTATION_MAX_LIFETIME_MS=` | Absolute ceiling on a runtime helper's life regardless of activity (default 24h; `0` disables) | +| `CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS=` | Idle window that applies once a helper's launcher is gone, nothing is connected, and the helper has never served a request (default 15m; `0` restores the full idle timeout) | | `CODEX_MULTI_AUTH_APP_BIND=0/1` | Alias-style opt-out for first-run packaged Codex app bind (see also `CODEX_MULTI_AUTH_APP_BIND_INSTALL`) | | `CODEX_MULTI_AUTH_APP_BIND_INSTALL=0/1` | Opt out/in of packaged Codex app bind self-heal on first durable CLI run or rotation enable | | `CODEX_MULTI_AUTH_APP_LAUNCHER_INSTALL=0/1` | Opt out/in of supported user-level launcher routing on first durable CLI run or rotation enable | diff --git a/docs/development/ARCHITECTURE.md b/docs/development/ARCHITECTURE.md index 0fffddc5..cc679624 100644 --- a/docs/development/ARCHITECTURE.md +++ b/docs/development/ARCHITECTURE.md @@ -197,13 +197,28 @@ Because no shim means no `CODEX_MULTI_AUTH_APP_SERVER_ACCOUNT_LABEL` in the forw A helper that cannot start is a hard failure on all of these branches — unlike the shadow path, there is no rotation-off shape left to degrade into, and quietly serving a resident server unrotated is worse than not serving it. Hard means a diagnostic on stderr and exit 1, not an unhandled rejection: `createRuntimeRotationProxyContextIfEnabled` catches the launch failure, releases the compatibility home the caller already built, and returns a `startupError` that `forwardToRealCodex` turns into an exit code before the official CLI is ever spawned. +Helper self-reaping is identity-checked and bounded. A detached helper decides "is my launcher still alive" by PID **plus the launcher's kernel start time** (passed at spawn via `CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS`), and reaps itself on whichever of three deadlines comes first. + +| Rule | Behavior | +| --- | --- | +| Owner identity | PID plus kernel start time. A bare `kill(pid, 0)` cannot tell a launcher from a later process that recycled its PID, and because the idle deadline only ever moved forward, one false "alive" was never corrected — helpers were observed running 33 hours past a 12-hour idle timeout, hundreds deep. | +| Recheck cadence | At most once a minute; a `ps` spawn per tick would cost more than it saves. A *failed* re-read keeps the previous verdict rather than declaring a live owner dead — under the process-table pressure this exists for, `fork` itself can fail. | +| Degraded check | Where no start time is known at all, the check degrades to bare liveness. That is the *normal* case on Windows, not an edge case: the start time comes from `ps`, which does not exist there, so both probes short-circuit rather than spawning a process that could only fail. Windows therefore runs owner liveness on `kill(pid, 0)` alone, and the lifetime ceiling below is what bounds a leak there. | +| Idle timeout | `CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS`, default 12h, refreshed by traffic and by a live owner. | +| Detached window | `CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS`, default 15m, `0` disables. Applies from the moment the owner is *confirmed* dead — a helper with no recorded owner PID is not a helper whose owner is dead, and stays on the idle timeout. The detach grace hands helpers off optimistically — any launcher exiting cleanly within it leaves its helper running — so every short forwarded command stranded a helper that then held the full idle timeout with no owner, no traffic, and nothing connected. The window only reaps a helper that has **never served a request**: every leaked helper in #663 had `totalRequests: 0`, while a live `codex app` session that is merely idle between turns holds no socket either (the proxy leaves `keepAliveTimeout` at Node's 5s default), so reaping on the socket check alone would kill a working proxy under the desktop app. A helper that served anything falls back to the idle timeout and the lifetime ceiling. | +| Connection gating | The detached window fires only while the proxy reports zero open client connections *and* the helper has never served a request. The connection check alone is not enough, because the proxy leaves `keepAliveTimeout` at Node's 5s default: a `codex app` session idle between turns holds no socket, so socket-only gating would reap a working proxy out from under the desktop app. Having served anything is the durable evidence of a handoff, and every helper in the leak report had `totalRequests: 0`. An unreadable connection count or request counter fails open into reaping: treating "unknown" as "attached" would restore the leak for any shape that stopped answering. | +| Lifetime ceiling | `CODEX_MULTI_AUTH_APP_ROTATION_MAX_LIFETIME_MS`, default 24h, `0` disables. Unconditional on activity — the backstop that turns any future accounting bug into a bounded leak instead of an unbounded one. | +| Telemetry | Per process: each helper publishes `runtime-rotation-app-helper..json` (the un-suffixed legacy path is still read for pre-upgrade helpers), publishes only on change plus a heartbeat rather than every tick, and removes its owner file on exit. Each launcher sweeps metadata files whose helper PID is dead immediately *after* spawning its own helper — the sweep is synchronous and unbounded, and the state it cleans up is exactly the state that makes it slow, so it must never sit in front of `codex app` or TUI startup. Terminal status stamps survive until that sweep, long enough to be read without accumulating forever. `rotation unbind-app` reclaims the same metadata independently, which is the only repair path on a machine that stopped launching helpers. | + +The published `idleExpiresAt` reports whichever of these deadlines is actually enforced, so `rotation status` cannot advertise 12h to a helper minutes from being reaped. Terminal states are `idle-timeout`, `owner-gone`, `max-lifetime`, `stopped`, and `error`; only `running` means running. + Helper shutdown is bounded rather than best-effort. `stopRuntimeRotationAppHelper` sends `SIGTERM`, waits out the graceful window, escalates to `SIGKILL` if the helper is still running, and then unconditionally destroys the helper's stdio streams and unrefs the child. That last step is the load-bearing one: the helper is spawned with piped stdio, so a helper that outlives the window — or any process that inherited those pipes — keeps the wrapper's event loop referenced and the shell prompt never returns. On Windows the signals are emulated as unconditional termination, so the stream teardown is the only part that reliably frees the wrapper there. Two interactive sessions can therefore run concurrently against the same home — the same as running the official CLI twice — and **no lock is taken over session state**: neither session copies or syncs it, so there is nothing to clobber. Regression coverage lives in `test/codex-bin-wrapper.test.ts`. Scope that guarantee to session state only. It does **not** extend to `config.toml`: `ensureCodexCliFileAuthStore` (`lib/codex-cli/writer.ts`) still read-modify-writes the canonical file when the store is not already `"file"`, and the atomic write does not serialize cross-process writers. That is safe in practice rather than by locking — the operation is idempotent, converges on a single value, and lands via atomic rename, so concurrent invocations agree instead of interleaving. Anything added to that write path that is *not* idempotent would need a real lock. -Internal env used by these branches (not operator-facing): `CODEX_MULTI_AUTH_APP_ROTATION_USE_CANONICAL_HOME`, `CODEX_MULTI_AUTH_APP_ROTATION_INSTALL_APP_SERVER_SHIM`, `CODEX_MULTI_AUTH_APP_SERVER_CONFIG_ARGS_JSON`, `CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID`, `CODEX_MULTI_AUTH_REAL_CODEX_HOME`. +Internal env used by these branches (not operator-facing): `CODEX_MULTI_AUTH_APP_ROTATION_USE_CANONICAL_HOME`, `CODEX_MULTI_AUTH_APP_ROTATION_INSTALL_APP_SERVER_SHIM`, `CODEX_MULTI_AUTH_APP_SERVER_CONFIG_ARGS_JSON`, `CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID`, `CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS`, `CODEX_MULTI_AUTH_REAL_CODEX_HOME`. * * * @@ -273,7 +288,8 @@ Canonical multi-auth root: `~/.codex/multi-auth`. | `budget-guards.json` | Local request/token/cost limits | | `local-client-tokens.json` | Local bridge token hashes (no plaintext) | | `usage/usage-ledger.jsonl` | Append-only local usage metadata (+ rotated archives) | -| `runtime-rotation-app-helper.json` | Wrapper-launched Codex app helper status | +| `runtime-rotation-app-helper..json` | Wrapper-launched Codex app helper status, one per live helper (the un-suffixed name is the pre-per-PID legacy path, still read) | +| `runtime-rotation-app-helper-owner..json` | Owner identity token for a wrapper-launched helper, one per helper; removed by the helper on exit and swept when its PID is dead | | `app-bind/` | Packaged app bind state, backup metadata, router status/log | | `logs/` | Diagnostics when logging is enabled | | `cache/` | Prompt/cache artifacts | diff --git a/docs/development/CONFIG_FIELDS.md b/docs/development/CONFIG_FIELDS.md index 0f5d8946..67183728 100644 --- a/docs/development/CONFIG_FIELDS.md +++ b/docs/development/CONFIG_FIELDS.md @@ -267,7 +267,10 @@ Cross-process refresh lease knobs: `CODEX_AUTH_REFRESH_LEASE`, `CODEX_AUTH_REFRE | `CODEX_MODE` | Toggle Codex mode | | `CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY` | Toggle localhost Responses proxy for forwarded Codex sessions (`1`/`true` to enable, `0`/`false` to disable) | | `CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS` | Override idle timeout for the wrapper-launched Codex app runtime helper | +| `CODEX_MULTI_AUTH_APP_ROTATION_MAX_LIFETIME_MS` | Absolute ceiling on a runtime helper's life regardless of activity (default 24h; `0` disables). The backstop that bounds the leak if activity accounting is ever wrong again | +| `CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS` | Idle window applied from the moment a helper's launcher is confirmed dead, and only while no client connection is open and the helper has never served a request (default 15m; `0` keeps the full idle timeout). Bounds helpers stranded by the detach grace; a helper that served traffic, or one with no recorded owner PID, stays on the idle timeout | | `CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID` | Internal owner PID used by the wrapper-launched app helper | +| `CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS` | Internal owner process start time (epoch ms) the helper uses to tell its launcher from a later process that recycled the PID | | `CODEX_MULTI_AUTH_REAL_CODEX_HOME` | Internal original Codex home pointer used by runtime rotation helpers | | `CODEX_MULTI_AUTH_APP_BIND_INSTALL` | Opt out/in of packaged Codex app bind self-heal on first CLI run or rotation enable | | `CODEX_MULTI_AUTH_APP_BIND` | Legacy/manual app-bind override consumed by the first-run setup hook (`lib/runtime/first-run.ts`) | diff --git a/docs/privacy.md b/docs/privacy.md index ab1731b0..32256b7d 100644 --- a/docs/privacy.md +++ b/docs/privacy.md @@ -30,7 +30,8 @@ | Local bridge client tokens | `~/.codex/multi-auth/local-client-tokens.json` | SHA-256 token hashes plus prefixes and labels; plaintext tokens are shown only on create/rotate | | Named backups | `~/.codex/multi-auth/backups/` | Operator-exported named account-pool backups | | Project account pools | `~/.codex/multi-auth/projects//` | Per-repo account pools when project scope is enabled | -| Runtime app helper status | `~/.codex/multi-auth/runtime-rotation-app-helper.json` | Local helper status for wrapper-launched Codex app sessions | +| Runtime app helper status | `~/.codex/multi-auth/runtime-rotation-app-helper..json` (one per helper; plus the legacy un-suffixed file from older versions) | Local helper status for wrapper-launched Codex app sessions | +| Runtime app helper owner identity | `~/.codex/multi-auth/runtime-rotation-app-helper-owner..json` (one per helper) | Local identity token and launcher PID, so a helper can tell its own launcher from a recycled PID; removed on helper exit and swept once the PID is dead | | Persistent app bind state/logs | `~/.codex/multi-auth/app-bind/` | Reversible packaged-app router state, backup metadata, and local router log | | Logs | `~/.codex/multi-auth/logs/codex-plugin/` | Optional diagnostics | | Prompt/cache files | `~/.codex/multi-auth/cache/` | Cached prompt/template metadata | @@ -88,7 +89,7 @@ rm -rf ~/.codex/multi-auth/refresh-leases rm -rf ~/.codex/multi-auth/usage rm -rf ~/.codex/multi-auth/backups rm -rf ~/.codex/multi-auth/projects -rm -f ~/.codex/multi-auth/runtime-rotation-app-helper.json +rm -f ~/.codex/multi-auth/runtime-rotation-app-helper.json ~/.codex/multi-auth/runtime-rotation-app-helper.*.json ~/.codex/multi-auth/runtime-rotation-app-helper-owner.*.json rm -rf ~/.codex/multi-auth/app-bind rm -rf ~/.codex/multi-auth/logs/codex-plugin rm -rf ~/.codex/multi-auth/cache @@ -115,7 +116,7 @@ Remove-Item "$HOME\.codex\multi-auth\refresh-leases" -Recurse -Force -ErrorActio Remove-Item "$HOME\.codex\multi-auth\usage" -Recurse -Force -ErrorAction SilentlyContinue Remove-Item "$HOME\.codex\multi-auth\backups" -Recurse -Force -ErrorAction SilentlyContinue Remove-Item "$HOME\.codex\multi-auth\projects" -Recurse -Force -ErrorAction SilentlyContinue -Remove-Item "$HOME\.codex\multi-auth\runtime-rotation-app-helper.json" -Force -ErrorAction SilentlyContinue +Remove-Item "$HOME\.codex\multi-auth\runtime-rotation-app-helper*.json","$HOME\.codex\multi-auth\runtime-rotation-app-helper-owner*.json" -Force -ErrorAction SilentlyContinue Remove-Item "$HOME\.codex\multi-auth\app-bind" -Recurse -Force -ErrorAction SilentlyContinue Remove-Item "$HOME\.codex\multi-auth\logs\codex-plugin" -Recurse -Force -ErrorAction SilentlyContinue Remove-Item "$HOME\.codex\multi-auth\cache" -Recurse -Force -ErrorAction SilentlyContinue diff --git a/docs/reference/settings.md b/docs/reference/settings.md index 53e93eec..84a58d16 100644 --- a/docs/reference/settings.md +++ b/docs/reference/settings.md @@ -219,6 +219,8 @@ Common operator overrides (aligned with [../configuration.md](../configuration.m - `CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY` - `CODEX_MULTI_AUTH_FORCE_ACCOUNT` — force one account for a single forwarded `codex-multi-auth-codex` run (selector: index/email/id); `--account` wins when both are set - `CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS` +- `CODEX_MULTI_AUTH_APP_ROTATION_MAX_LIFETIME_MS` — absolute ceiling on a helper's life regardless of activity (default 24h; `0` disables) +- `CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS` — idle window once the helper's launcher is confirmed gone, nothing is connected, and the helper has never served a request (default 15m; `0` keeps the full idle timeout) - `CODEX_MULTI_AUTH_APP_BIND_INSTALL` - `CODEX_MULTI_AUTH_APP_LAUNCHER_INSTALL` - `CODEX_TUI_V2` diff --git a/docs/reference/storage-paths.md b/docs/reference/storage-paths.md index 11e94885..459ea729 100644 --- a/docs/reference/storage-paths.md +++ b/docs/reference/storage-paths.md @@ -39,7 +39,7 @@ Override root: | Budget guards | `~/.codex/multi-auth/budget-guards.json` | | Local bridge client tokens | `~/.codex/multi-auth/local-client-tokens.json` | | Cross-process refresh leases | `~/.codex/multi-auth/refresh-leases/` | -| Runtime app helper status | `~/.codex/multi-auth/runtime-rotation-app-helper.json` | +| Runtime app helper status | `~/.codex/multi-auth/runtime-rotation-app-helper..json` | | Runtime app helper owner metadata | `~/.codex/multi-auth/runtime-rotation-app-helper-owner..json` | | Persistent app bind directory | `~/.codex/multi-auth/app-bind/` | | Named pool backups | `~/.codex/multi-auth/backups/` | @@ -159,7 +159,7 @@ Runtime rotation adds local state only when enabled or when a helper has recentl | Path | Purpose | | --- | --- | | `~/.codex/multi-auth/runtime-observability.json` | request counters, last selected runtime account metadata, and cooldown context for status/report commands | -| `~/.codex/multi-auth/runtime-rotation-app-helper.json` | wrapper-launched `codex app` helper state, idle timeout, request count, and last-account metadata | +| `~/.codex/multi-auth/runtime-rotation-app-helper..json` | wrapper-launched `codex app` helper state, idle timeout, request count, and last-account metadata — one file per helper; the un-suffixed name is the legacy shared path from older versions, still read. A terminal stamp persists until the next helper launch sweeps files whose PID is dead; the owner file is removed on clean helper exit. `codex-multi-auth rotation unbind-app` sweeps the same metadata independently — including owner files with no surviving status record — so it is the recovery path when helpers have accumulated and nothing is launching new ones. It preserves, with a warning, any record whose PID is still live | | `~/.codex/multi-auth/app-bind/runtime-rotation-app-bind.json` | persistent packaged-app bind state | | `~/.codex/multi-auth/app-bind/codex-config-backup.json` | backup metadata for restoring the real Codex `config.toml` | | `~/.codex/multi-auth/app-bind/runtime-rotation-app-bind-status.json` | persistent app router status | diff --git a/lib/codex-manager/commands/rotation.ts b/lib/codex-manager/commands/rotation.ts index 28220e8e..21ad4bbd 100644 --- a/lib/codex-manager/commands/rotation.ts +++ b/lib/codex-manager/commands/rotation.ts @@ -1,5 +1,4 @@ -import { existsSync, readFileSync, statSync } from "node:fs"; -import { join } from "node:path"; +import { existsSync, readFileSync, readdirSync, statSync } from "node:fs"; import { AccountManager, formatAccountLabel, @@ -31,7 +30,7 @@ import { type AppBindResult, type AppBindStatus, } from "../../runtime/app-bind.js"; -import { APP_RUNTIME_HELPER_STATUS_FILE } from "../../runtime-constants.js"; +import { listRuntimeHelperStatusPaths } from "../../runtime-constants.js"; import { findQuotaCacheEntryForAccount, isQuotaCacheEntryExhausted, @@ -46,6 +45,12 @@ import { resolveAccountCurrentMarkers, resolveRuntimeCurrentAccount, } from "../../runtime/runtime-current-account.js"; +import { + isLiveRuntimeHelper, + liveRuntimeHelpers, + readRuntimeHelperPid, + selectRuntimeHelperStatus, +} from "../../runtime/app-helper-selection.js"; import { isRateLimitedMarker } from "../rate-limit-markers.js"; import type { PluginConfig } from "../../types.js"; import type { AccountMetadataV3, AccountStorageV3 } from "../../storage.js"; @@ -57,6 +62,9 @@ interface AppRuntimeHelperStatus { kind: string | null; state: string | null; pid: number | null; + // Parsed so liveness can be identity-checked rather than trusting + // `kill(pid, 0)` alone; see app-helper-selection.ts. + startedAt: number | null; idleExpiresAt: number | null; totalRequests: number | null; rotations: number | null; @@ -515,8 +523,9 @@ function readOptionalString(record: Record, key: string): strin const MAX_STATUS_FILE_BYTES = 1024 * 1024; // 1 MB sanity cap -function readAppRuntimeHelperStatus(): AppRuntimeHelperStatus | null { - const statusPath = join(getCodexMultiAuthDir(), APP_RUNTIME_HELPER_STATUS_FILE); +function readAppRuntimeHelperStatusFile( + statusPath: string, +): AppRuntimeHelperStatus | null { if (!existsSync(statusPath)) return null; try { const stat = statSync(statusPath); @@ -530,7 +539,8 @@ function readAppRuntimeHelperStatus(): AppRuntimeHelperStatus | null { return { state: readOptionalString(parsed, "state"), kind: readOptionalString(parsed, "kind"), - pid: readOptionalNumber(parsed, "pid"), + pid: readRuntimeHelperPid(parsed.pid), + startedAt: readOptionalNumber(parsed, "startedAt"), idleExpiresAt: readOptionalNumber(parsed, "idleExpiresAt"), totalRequests: readOptionalNumber(parsed, "totalRequests"), rotations: readOptionalNumber(parsed, "rotations"), @@ -546,16 +556,29 @@ function readAppRuntimeHelperStatus(): AppRuntimeHelperStatus | null { } } -function isProcessAlive(pid: number | null): boolean { - if (!pid) return false; +// One directory scan feeds both the status line and the live-helper count so +// the two cannot observe different moments; path discovery is shared with +// every other reader via listRuntimeHelperStatusPaths in runtime-constants. +function readAppRuntimeHelperStatuses(): AppRuntimeHelperStatus[] { + const multiAuthDir = getCodexMultiAuthDir(); + let entries: string[] = []; try { - process.kill(pid, 0); - return true; - } catch (error) { - const code = - error && typeof error === "object" && "code" in error ? error.code : null; - return code === "EPERM"; + entries = readdirSync(multiAuthDir); + } catch { + entries = []; } + return listRuntimeHelperStatusPaths(multiAuthDir, entries) + .map(readAppRuntimeHelperStatusFile) + .filter( + (status): status is AppRuntimeHelperStatus => + status !== null && status.kind === "codex-app-runtime-rotation-helper", + ); +} + +function readAppRuntimeHelperStatus( + now: number = Date.now(), +): AppRuntimeHelperStatus | null { + return selectRuntimeHelperStatus(readAppRuntimeHelperStatuses(), now); } function formatHelperLastAccount(status: AppRuntimeHelperStatus): string | null { @@ -575,14 +598,20 @@ function formatHelperLastAccount(status: AppRuntimeHelperStatus): string | null function formatAppRuntimeHelperStatus( now: number, - status = readAppRuntimeHelperStatus(), + status = readAppRuntimeHelperStatus(now), + liveHelperCount = status ? 1 : 0, ): string { if (!status) return "Codex app helper: not running"; if (status.kind !== "codex-app-runtime-rotation-helper") { return "Codex app helper: not running"; } - const alive = isProcessAlive(status.pid); - if (!alive || status.state === "stopped" || status.state === "idle-timeout") { + // Only "running" is running: "stopped", "idle-timeout", "max-lifetime", + // "owner-gone", "error", and anything a future helper invents are all + // terminal, and a live kill(pid, 0) on a terminal record proves nothing — + // the PID may be recycled, which is the exact gate this fix stopped + // trusting. `isLiveRuntimeHelper` folds both conditions together, and is + // the same predicate the selection above and the account marker below use. + if (!isLiveRuntimeHelper(status, now)) { return "Codex app helper: not running"; } const parts = [`running${status.pid ? ` pid=${status.pid}` : ""}`]; @@ -593,6 +622,12 @@ function formatAppRuntimeHelperStatus( if (status.idleExpiresAt !== null && status.idleExpiresAt > now) { parts.push(`idle-expires=${formatWaitTime(status.idleExpiresAt - now)}`); } + // The line shows the most recently active helper; with per-account + // app-servers several can be live at once, so say so instead of implying + // this one is the only one. + if (liveHelperCount > 1) { + parts.push(`(+${liveHelperCount - 1} more running)`); + } return `Codex app helper: ${parts.join(", ")}`; } @@ -647,8 +682,19 @@ async function printRotationStatus(deps: RotationCommandDeps): Promise { `Stored setting: ${config.codexRuntimeRotationProxy === true ? "enabled" : "disabled"}`, ); logInfo(`Env override: ${formatEnvOverride()}`); - const helperStatus = readAppRuntimeHelperStatus(); - logInfo(formatAppRuntimeHelperStatus(now, helperStatus)); + // One scan and one selection feed the status line, the live count and the + // account marker below. Selecting twice re-ran the liveness probes at a + // later instant, so the helper named on the line and the helper whose + // account is marked `current` could be different processes (#667). + const helperStatuses = readAppRuntimeHelperStatuses(); + const selectedHelperStatus = selectRuntimeHelperStatus(helperStatuses, now); + logInfo( + formatAppRuntimeHelperStatus( + now, + selectedHelperStatus, + liveRuntimeHelpers(helperStatuses, now).length, + ), + ); const appBindStatus = await printCodexAppBindStatus(deps); logInfo(`Storage: ${storagePath}`); @@ -671,7 +717,10 @@ async function printRotationStatus(deps: RotationCommandDeps): Promise { { runtimeSnapshot, appBindStatus: appBindStatus?.running ? appBindStatus.router : null, - appHelperStatus: appRuntimeHelperStatusToRuntimeSignal(helperStatus), + appHelperStatus: appRuntimeHelperStatusToRuntimeSignal( + selectedHelperStatus, + now, + ), }, { now }, ); diff --git a/lib/runtime-constants.ts b/lib/runtime-constants.ts index 9bf1c850..d72f7b83 100644 --- a/lib/runtime-constants.ts +++ b/lib/runtime-constants.ts @@ -1,3 +1,5 @@ +import { join } from "node:path"; + export const RUNTIME_ROTATION_PROXY_PROVIDER_ID = "codex-multi-auth-runtime-proxy" as const; @@ -7,3 +9,69 @@ export const APP_RUNTIME_HELPER_STATUS_FILE = /** Immutable launcher metadata used to verify ownership before stopping a helper. */ export const APP_RUNTIME_HELPER_OWNER_FILE = "runtime-rotation-app-helper-owner.json" as const; + +/** + * The one place the per-PID filename shape is written down: + * `..json`, matched case-insensitively with the PID + * captured. Status files and owner files share that shape, and so does the + * launcher-side sweep in `scripts/codex.js` — that file re-derives the pattern + * from the same two constants because it has to keep working before `dist/` is + * built, so a change to the shape here is a change there too. + */ +export function runtimeHelperPerPidPattern(baseName: string): RegExp { + const prefix = baseName.replace(/\.json$/i, ""); + return new RegExp( + `^${prefix.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\.(\\d+)\\.json$`, + "i", + ); +} + +/** + * Every path a helper status record can live at: the per-PID files + * (`runtime-rotation-app-helper..json`, one per helper) plus the + * un-suffixed legacy shared path from before the per-PID change, which is + * still read so a pre-upgrade helper stays visible. The filename contract + * lives here, next to the constant it derives from, so every reader agrees + * on it; callers supply the directory listing so this stays pure and their + * own error handling for the `readdir` stays theirs. + */ +export function listRuntimeHelperStatusPaths( + baseDir: string, + entries: readonly string[], +): string[] { + const perPidPattern = runtimeHelperPerPidPattern( + APP_RUNTIME_HELPER_STATUS_FILE, + ); + return [ + ...entries + .filter((name) => perPidPattern.test(name)) + .map((name) => join(baseDir, name)), + join(baseDir, APP_RUNTIME_HELPER_STATUS_FILE), + ]; +} + +/** + * Owner files paired with the helper PID they belong to. Unbind used to + * enumerate status files only, so an owner file whose status record had + * already been removed could never be rediscovered and accumulated under the + * multi-auth root forever (#666). Enumerating both is what lets a cleanup pass + * reclaim an owner file that outlived its status record. + */ +export function listRuntimeHelperOwnerPaths( + baseDir: string, + entries: readonly string[], +): { path: string; pid: number }[] { + const perPidPattern = runtimeHelperPerPidPattern( + APP_RUNTIME_HELPER_OWNER_FILE, + ); + const owners: { path: string; pid: number }[] = []; + for (const name of entries) { + const match = perPidPattern.exec(name); + const captured = match?.[1]; + if (captured === undefined) continue; + const pid = Number.parseInt(captured, 10); + if (!Number.isInteger(pid) || pid < 1) continue; + owners.push({ path: join(baseDir, name), pid }); + } + return owners; +} diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index c1df621c..f447b985 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -826,6 +826,10 @@ export async function startRuntimeRotationProxy( await closeServer(server, sockets); await state.activeAccountManager.flushPendingSave(); }, + // Live client connections, which the app helper reads as evidence that a + // detached consumer is still attached: a helper whose launcher is gone + // and whose socket set is empty has nobody left to serve. + getOpenConnectionCount: () => sockets.size, getStatus: () => ({ ...state.status, // Redact any email/token material that leaked into a raw upstream or diff --git a/lib/runtime/app-bind.ts b/lib/runtime/app-bind.ts index 3a78ecc3..c579b1e2 100644 --- a/lib/runtime/app-bind.ts +++ b/lib/runtime/app-bind.ts @@ -1,7 +1,7 @@ import { spawn } from "node:child_process"; import { createHash, randomBytes } from "node:crypto"; import { closeSync, existsSync, mkdirSync, openSync } from "node:fs"; -import { mkdir, open, readFile, rename, rm, unlink } from "node:fs/promises"; +import { mkdir, open, readFile, readdir, rename, rm, unlink } from "node:fs/promises"; import { homedir } from "node:os"; import { basename, dirname, join } from "node:path"; import process from "node:process"; @@ -10,7 +10,8 @@ import { withFileOperationRetry } from "../fs-retry.js"; import { getCodexMultiAuthDir } from "../runtime-paths.js"; import { APP_RUNTIME_HELPER_OWNER_FILE, - APP_RUNTIME_HELPER_STATUS_FILE, + listRuntimeHelperOwnerPaths, + listRuntimeHelperStatusPaths, } from "../runtime-constants.js"; import { configHasRuntimeRotationProvider, @@ -1266,6 +1267,58 @@ function resolveRuntimeHelperOwnerPath( ); } +/** + * How many helper records unbind processes at once. Exported so a regression + * test can observe the bound rather than only its effects — with a handful of + * records any width behaves identically, so an edit to `Infinity` would + * otherwise ship green. + */ +export const UNBIND_HELPER_CONCURRENCY = 8; + +interface HelperCleanupDecision { + statusPath: string; + ownerPath: string | null; + removeHelperStatus: boolean; + removeHelperOwner: boolean; +} + +/** + * `Promise.all` over `items` with at most `limit` in flight, preserving input + * order in the result. Used where the per-item work is independent but not + * free — a helper stop pays a SIGTERM, a graceful wait and possibly a SIGKILL — + * so serialising it multiplies a single stop window by the number of stale + * helpers, and unbounded parallelism signals every one of them at once. + */ +async function mapWithConcurrency( + items: readonly T[], + limit: number, + worker: (item: T, index: number) => Promise, +): Promise { + const results: R[] = new Array(items.length); + let next = 0; + const runners = Array.from( + { length: Math.max(1, Math.min(limit, items.length)) }, + async () => { + for (;;) { + const index = next; + next += 1; + // Only running past the end retires a runner. Folding the + // `undefined` check into the same `return` would make a sparse + // array or a `(T | undefined)[]` silently drop every item after + // the first hole — on a cleanup path whose failure mode is + // "helpers left running while the user is told the app was + // unbound". + if (index >= items.length) return; + const item = items[index]; + if (item === undefined) continue; + results[index] = await worker(item, index); + } + }, + ); + await Promise.all(runners); + return results; +} + export async function stopRuntimeRotationAppHelperProcess( helper: RuntimeRotationAppHelperStatus, options: DetachedProcessStopOptions & { platform?: NodeJS.Platform } = {}, @@ -1499,19 +1552,56 @@ async function unbindCodexAppRuntimeRotationLocked( ); } - const helperStatusPath = join( - dirname(paths.bindDir), - APP_RUNTIME_HELPER_STATUS_FILE, + // Helpers publish per-PID status files (`runtime-rotation-app-helper..json`); + // the un-suffixed name is the legacy shared path from before that change, + // still checked so a pre-upgrade helper is torn down too. Every candidate + // walks the same per-helper logic the single file used to get: stopping is + // gated on ownership verification (status/owner identity-token agreement + // plus process identity), so unbind reaps each helper it can prove is one + // of ours and preserves — with a warning — anything it cannot. + const helperBaseDir = dirname(paths.bindDir); + let helperDirEntries: string[] = []; + try { + helperDirEntries = await withFileOperationRetry(() => + readdir(helperBaseDir), + ); + } catch (error) { + // Degrading to legacy-only cleanup while reporting success would leave + // every per-PID helper running with the user told the app was unbound — + // say so. ENOENT just means no helper ever ran. + const code = + error && typeof error === "object" && "code" in error + ? String((error as { code?: unknown }).code) + : "unknown"; + if (code !== "ENOENT") { + options.log?.( + `Warning: could not enumerate runtime app helper status files (${code}); only the legacy helper path is checked`, + ); + } + helperDirEntries = []; + } + const helperStatusPaths = listRuntimeHelperStatusPaths( + helperBaseDir, + helperDirEntries, ); - const helperRead = await readRuntimeHelperStatus(helperStatusPath); - let removeHelperStatus = false; - let removeHelperOwner = false; - let helperOwnerPath: string | null = null; - if (helperRead.kind === "valid") { - const helper = helperRead.status; - if (helper.kind === "codex-app-runtime-rotation-helper") { - helperOwnerPath = resolveRuntimeHelperOwnerPath( - dirname(paths.bindDir), + // Each candidate is independent — a read, a liveness check, and at most one + // SIGTERM/graceful-wait/SIGKILL sequence — and on the machine from #663 there + // were 183 of them. Run them in a bounded pool rather than one after another, + // so unbind costs roughly one stop window instead of N of them; the bound + // keeps a machine full of stale helpers from being hit with 183 concurrent + // signal sequences. + const helperResults = await mapWithConcurrency( + helperStatusPaths, + UNBIND_HELPER_CONCURRENCY, + async (helperStatusPath): Promise => { + const helperRead = await readRuntimeHelperStatus(helperStatusPath); + if (helperRead.kind !== "valid") return null; + const helper = helperRead.status; + if (helper.kind !== "codex-app-runtime-rotation-helper") return null; + let removeHelperStatus = false; + let removeHelperOwner = false; + const helperOwnerPath = resolveRuntimeHelperOwnerPath( + helperBaseDir, helper.pid, ); const helperOwner = helperOwnerPath @@ -1529,9 +1619,15 @@ async function unbindCodexAppRuntimeRotationLocked( } else { const wasAlive = isProcessAlive(helper.pid); if (!wasAlive) { + // Decisive, and deliberately not gated on ownership (#666): a + // dead PID means both files describe a process that no longer + // exists, so keeping the owner file preserves nothing. It used + // to be gated, which deleted the status file and stranded the + // owner file — and because unbind then enumerated status paths + // only, nothing ever rediscovered it. This matches what the + // launcher-side sweep already does with a dead PID. removeHelperStatus = true; - removeHelperOwner = - helperOwnershipMatches && helperOwnerPath !== null; + removeHelperOwner = helperOwnerPath !== null; } else if (!helperOwnershipMatches) { options.log?.( "Warning: runtime app helper ownership metadata does not match; preserving status", @@ -1547,8 +1643,7 @@ async function unbindCodexAppRuntimeRotationLocked( }); const stillAlive = isProcessAlive(helper.pid); removeHelperStatus = stopped && !stillAlive; - removeHelperOwner = - removeHelperStatus && helperOwnerPath !== null; + removeHelperOwner = removeHelperStatus && helperOwnerPath !== null; if (!removeHelperStatus) { options.log?.( `Warning: runtime app helper (pid ${helper.pid}) did not stop; preserving status`, @@ -1560,14 +1655,59 @@ async function unbindCodexAppRuntimeRotationLocked( // A non-running, owned record is removable only when its PID is // absent or no longer alive. This avoids deleting a status file // while a helper is still serving despite a stale state value. - removeHelperStatus = - helper.pid === null || !isProcessAlive(helper.pid); - removeHelperOwner = - removeHelperStatus && - helperOwnershipMatches && - helperOwnerPath !== null; + // Ownership does not gate the owner file here either, for the same + // reason as above: the PID is gone, so neither file describes + // anything that can still be running. + removeHelperStatus = helper.pid === null || !isProcessAlive(helper.pid); + removeHelperOwner = removeHelperStatus && helperOwnerPath !== null; } + return { + statusPath: helperStatusPath, + ownerPath: helperOwnerPath, + removeHelperStatus, + removeHelperOwner, + }; + }, + ); + const helperCleanupPaths: string[] = []; + // Owner paths this pass already reasoned about, whether or not it decided to + // remove them — a preserved live helper's owner file must not then be swept + // by the orphan pass below. + const consideredOwnerPaths = new Set(); + for (const result of helperResults) { + if (!result) continue; + if (result.ownerPath !== null) consideredOwnerPaths.add(result.ownerPath); + if (result.removeHelperStatus) helperCleanupPaths.push(result.statusPath); + if (result.removeHelperOwner && result.ownerPath !== null) { + helperCleanupPaths.push(result.ownerPath); + } + } + // Owner files with no status record left to pair them with. Before #666 these + // were unreachable: every earlier pass walked status paths only, so an owner + // file that outlived its status file was never looked at again. A dead PID is + // the whole test — a live PID's owner file belongs to a helper that is still + // running, and was already considered above. + for (const owner of listRuntimeHelperOwnerPaths( + helperBaseDir, + helperDirEntries, + )) { + if (consideredOwnerPaths.has(owner.path)) continue; + if (isProcessAlive(owner.pid)) { + // An owner file with no status record whose PID is nonetheless live + // is the one shape this pass cannot reclaim. Either a helper is + // starting right now and has not published yet, or — the case that + // accumulates — the PID was recycled by an unrelated process after + // the status file was already gone. Telling those apart needs the + // recorded-start-time comparison the launcher-side sweep does, which + // unbind has no equivalent of; that is a scope decision, but it + // should not be a silent one. Every other preserve in this function + // warns, so this one does too. + options.log?.( + `Warning: runtime app helper owner metadata (pid ${owner.pid}) has no status record but its PID is live; preserving`, + ); + continue; } + helperCleanupPaths.push(owner.path); } await removeAppBindStartup(state ?? paths); @@ -1617,8 +1757,7 @@ async function unbindCodexAppRuntimeRotationLocked( paths.backupPath, paths.statusPath, state?.logPath ?? paths.logPath, - ...(removeHelperStatus ? [helperStatusPath] : []), - ...(removeHelperOwner && helperOwnerPath ? [helperOwnerPath] : []), + ...helperCleanupPaths, ]; for (const candidate of cleanupCandidates) { try { diff --git a/lib/runtime/app-helper-selection.ts b/lib/runtime/app-helper-selection.ts new file mode 100644 index 00000000..04a004a8 --- /dev/null +++ b/lib/runtime/app-helper-selection.ts @@ -0,0 +1,131 @@ +import process from "node:process"; + +/** + * The fields any helper status record has to expose for the shared selector to + * reason about it. `rotation status` and runtime account resolution carry + * different extra fields — request counters on one side, account identity on + * the other — but they answer "which helper is current" the same way, and used + * to answer it with two hand-rolled copies that could drift apart within a + * single command (#667). + */ +export interface RuntimeHelperSelectable { + state: string | null; + pid: number | null; + startedAt: number | null; + updatedAt: number | null; +} + +/** + * How stale a `running` record may be before it stops counting as live. + * + * A running helper republishes its status on every tick, and the publish path + * heartbeats at least once per `APP_RUNTIME_HELPER_STATUS_HEARTBEAT_MS` (60s) + * even when nothing in the payload changed. Ten heartbeats of silence is not a + * helper that is merely quiet — it is a record whose writer is gone. + * + * This is the identity check these readers were missing. `kill(pid, 0)` answers + * "does *a* process hold this integer", so a stale record — classically the + * legacy shared `runtime-rotation-app-helper.json` left behind by a SIGKILLed + * pre-upgrade helper — passes liveness as soon as an unrelated process is + * handed its PID, and can then win selection outright. Freshness is the half of + * identity available to a synchronous reader: the wrapper verifies identity by + * probing kernel start times, but that costs a `ps` per candidate, and these + * two call sites are read-only status paths reached from the interactive menu + * as well as the CLI. Whoever holds the PID now, they are not the process that + * last wrote this file. + */ +export const RUNTIME_HELPER_STATUS_STALE_MS = 10 * 60 * 1000; + +/** Tolerance for clock skew between the writing helper and the reader. */ +export const RUNTIME_HELPER_CLOCK_TOLERANCE_MS = 60 * 1000; + +/** + * A PID is a positive integer or it is nothing. Both readers used to accept any + * finite number, so a corrupt or hand-edited record carrying `-1234` reached + * `process.kill(-1234, 0)` — which on POSIX probes process *group* 1234 and + * succeeds on any busy machine, reporting a helper that does not exist as live. + * Fractional values were accepted the same way. + */ +export function readRuntimeHelperPid(value: unknown): number | null { + return typeof value === "number" && Number.isInteger(value) && value > 0 + ? value + : null; +} + +/** + * Best-effort liveness probe. `EPERM` means a process exists that this user may + * not signal, so it counts as alive; every other errno — including the `EINVAL` + * some platforms raise for a PID above their ceiling — counts as dead. + */ +export function isRuntimeHelperProcessAlive(pid: number | null): boolean { + const probePid = readRuntimeHelperPid(pid); + if (probePid === null) return false; + try { + process.kill(probePid, 0); + return true; + } catch (error) { + const code = + error && typeof error === "object" && "code" in error ? error.code : null; + return code === "EPERM"; + } +} + +/** + * The single definition of "this helper is currently serving": a running state, + * a live PID, and a record recent enough to have been written by that PID's + * current occupant. + */ +export function isLiveRuntimeHelper( + status: RuntimeHelperSelectable, + now: number = Date.now(), +): boolean { + if (status.state !== "running") return false; + if (!isRuntimeHelperProcessAlive(status.pid)) return false; + // A record that claims to have started after the current instant was not + // written by a process that is running now. + if ( + status.startedAt !== null && + status.startedAt > now + RUNTIME_HELPER_CLOCK_TOLERANCE_MS + ) { + return false; + } + // A record with no `updatedAt` at all predates the heartbeat contract and + // cannot be judged on freshness; it falls back to bare liveness rather than + // being discarded, which is no worse than the behaviour it replaces. + if (status.updatedAt === null) return true; + return now - status.updatedAt <= RUNTIME_HELPER_STATUS_STALE_MS; +} + +/** Every helper that passes {@link isLiveRuntimeHelper}, input order preserved. */ +export function liveRuntimeHelpers( + statuses: readonly T[], + now: number = Date.now(), +): T[] { + return statuses.filter((status) => isLiveRuntimeHelper(status, now)); +} + +function byRecency( + left: RuntimeHelperSelectable, + right: RuntimeHelperSelectable, +): number { + return (right.updatedAt ?? 0) - (left.updatedAt ?? 0); +} + +/** + * Prefer a live helper, and among several the most recently updated. Absent any + * live helper, fall back to the freshest record so the previous "reports the + * last helper's final state" behaviour survives. + * + * Callers that also need the live count must pass the same `statuses` array and + * the same `now` to {@link liveRuntimeHelpers}, so the selected helper and the + * count describe one instant rather than two. + */ +export function selectRuntimeHelperStatus( + statuses: readonly T[], + now: number = Date.now(), +): T | null { + if (statuses.length === 0) return null; + const live = liveRuntimeHelpers(statuses, now).sort(byRecency); + if (live.length > 0) return live[0] ?? null; + return [...statuses].sort(byRecency)[0] ?? null; +} diff --git a/lib/runtime/rotation-server-types.ts b/lib/runtime/rotation-server-types.ts index 6f3059ea..21cec7d3 100644 --- a/lib/runtime/rotation-server-types.ts +++ b/lib/runtime/rotation-server-types.ts @@ -7,6 +7,13 @@ export interface RuntimeRotationProxyServer { baseUrl: string; close: () => Promise; getStatus: () => RuntimeRotationProxyStatus; + /** + * Number of client sockets currently open against the proxy. The detached + * app helper uses it to tell a handed-off consumer from a stranded process; + * optional so a proxy shape without it degrades to activity-only accounting + * rather than failing to start. + */ + getOpenConnectionCount?: () => number; } export interface RuntimeRotationProxyStatus { diff --git a/lib/runtime/runtime-current-account.ts b/lib/runtime/runtime-current-account.ts index 04009d01..872b9ec0 100644 --- a/lib/runtime/runtime-current-account.ts +++ b/lib/runtime/runtime-current-account.ts @@ -1,9 +1,12 @@ -import { existsSync, readFileSync, statSync } from "node:fs"; -import { join } from "node:path"; -import process from "node:process"; +import { existsSync, readFileSync, readdirSync, statSync } from "node:fs"; import type { RuntimeObservabilitySnapshot } from "./runtime-observability.js"; import type { AppBindRouterStatus } from "./app-bind.js"; -import { APP_RUNTIME_HELPER_STATUS_FILE } from "../runtime-constants.js"; +import { + isLiveRuntimeHelper, + readRuntimeHelperPid, + selectRuntimeHelperStatus, +} from "./app-helper-selection.js"; +import { listRuntimeHelperStatusPaths } from "../runtime-constants.js"; import { getCodexMultiAuthDir } from "../runtime-paths.js"; import type { AccountStorageV3 } from "../storage.js"; import { isRecord } from "../utils.js"; @@ -56,6 +59,9 @@ export interface AppRuntimeHelperAccountStatus { kind: string | null; state: string | null; pid: number | null; + // Parsed so liveness can be identity-checked rather than trusting + // `kill(pid, 0)` alone; see app-helper-selection.ts. + startedAt: number | null; lastAccountIndex: number | null; lastAccountLabel: string | null; lastAccountEmail: string | null; @@ -110,22 +116,10 @@ function readOptionalString(record: Record, key: string): strin : null; } -// Best-effort liveness probe: process.kill(pid, 0) can report permission -// failures for live processes and cannot protect against rare PID reuse. -function isProcessAlive(pid: number | null): boolean { - if (!pid) return false; - try { - process.kill(pid, 0); - return true; - } catch (error) { - const code = - error && typeof error === "object" && "code" in error ? error.code : null; - return code === "EPERM"; - } -} -export function readAppRuntimeHelperStatus(): AppRuntimeHelperAccountStatus | null { - const statusPath = join(getCodexMultiAuthDir(), APP_RUNTIME_HELPER_STATUS_FILE); +function readAppRuntimeHelperStatusFile( + statusPath: string, +): AppRuntimeHelperAccountStatus | null { if (!existsSync(statusPath)) return null; try { const stat = statSync(statusPath); @@ -139,7 +133,8 @@ export function readAppRuntimeHelperStatus(): AppRuntimeHelperAccountStatus | nu return { kind: readOptionalString(parsed, "kind"), state: readOptionalString(parsed, "state"), - pid: readOptionalNumber(parsed, "pid"), + pid: readRuntimeHelperPid(parsed.pid), + startedAt: readOptionalNumber(parsed, "startedAt"), lastAccountIndex: readOptionalNumber(parsed, "lastAccountIndex"), lastAccountLabel: readOptionalString(parsed, "lastAccountLabel"), lastAccountEmail: readOptionalString(parsed, "lastAccountEmail"), @@ -152,13 +147,41 @@ export function readAppRuntimeHelperStatus(): AppRuntimeHelperAccountStatus | nu } } +// Helpers publish per-PID status files (`runtime-rotation-app-helper..json`) +// so N concurrent helpers stop overwriting one shared path; path discovery is +// shared with every other reader via listRuntimeHelperStatusPaths. +function listAppRuntimeHelperStatusPaths(multiAuthDir: string): string[] { + let entries: string[] = []; + try { + entries = readdirSync(multiAuthDir); + } catch { + entries = []; + } + return listRuntimeHelperStatusPaths(multiAuthDir, entries); +} + +export function readAppRuntimeHelperStatus( + now: number = Date.now(), +): AppRuntimeHelperAccountStatus | null { + const statuses = listAppRuntimeHelperStatusPaths(getCodexMultiAuthDir()) + .map(readAppRuntimeHelperStatusFile) + .filter( + (status): status is AppRuntimeHelperAccountStatus => + status !== null && status.kind === APP_RUNTIME_HELPER_KIND, + ); + // Selection is shared with `rotation status` so the helper named on the + // status line and the helper whose account is marked `current` can never be + // two different helpers (#667). + return selectRuntimeHelperStatus(statuses, now); +} + export function appRuntimeHelperStatusToSignal( status: AppRuntimeHelperAccountStatus | null, + now: number = Date.now(), ): RuntimeAccountSignal | null { if (!status) return null; if (status.kind !== APP_RUNTIME_HELPER_KIND) return null; - if (status.state !== "running") return null; - if (!isProcessAlive(status.pid)) return null; + if (!isLiveRuntimeHelper(status, now)) return null; return { source: "app-helper", lastAccountIndex: status.lastAccountIndex, @@ -171,7 +194,10 @@ export function appRuntimeHelperStatusToSignal( } export function readAppRuntimeHelperAccountSignal(): RuntimeAccountSignal | null { - return appRuntimeHelperStatusToSignal(readAppRuntimeHelperStatus()); + // One `now` for both the selection and the liveness verdict, so a helper + // cannot be selected against one instant and judged against another. + const now = Date.now(); + return appRuntimeHelperStatusToSignal(readAppRuntimeHelperStatus(now), now); } function runtimeSnapshotToSignal( diff --git a/scripts/codex.js b/scripts/codex.js index 111e40a1..1ddedc59 100755 --- a/scripts/codex.js +++ b/scripts/codex.js @@ -1,6 +1,6 @@ #!/usr/bin/env node -import { spawn } from "node:child_process"; +import { execFile, execFileSync, spawn } from "node:child_process"; import { createHash, randomBytes } from "node:crypto"; import { chmodSync, @@ -85,6 +85,29 @@ const APP_RUNTIME_HELPER_STATUS_FILE = const APP_RUNTIME_HELPER_OWNER_FILE = RUNTIME_CONSTANTS.APP_RUNTIME_HELPER_OWNER_FILE; const DEFAULT_APP_RUNTIME_HELPER_IDLE_MS = 12 * 60 * 60 * 1000; +// Absolute ceiling on a helper's life, independent of the idle tracker. The +// idle reaper depends on activity accounting being correct; any bug there — +// PID reuse briefly reviving a dead owner is the observed one — previously +// produced an *unbounded* leak because nothing else bounded the process. +const DEFAULT_APP_RUNTIME_HELPER_MAX_LIFETIME_MS = 24 * 60 * 60 * 1000; +// A helper whose launcher is gone is not idle in the same sense as one whose +// launcher is sitting at a prompt: nobody is coming back to it unless a +// detached consumer picked it up. The detach grace below hands helpers off +// optimistically — every launcher that exits cleanly within the window leaves +// its helper running — so short forwarded commands strand helpers that then +// hold the full idle timeout with no owner and no traffic. This window is the +// idle timeout that applies from the moment the owner is confirmed dead, and +// it only ever fires with zero open client connections, so a consumer that +// really did take the handoff is never reaped out from under. +const DEFAULT_APP_RUNTIME_HELPER_DETACHED_IDLE_MS = 15 * 60 * 1000; +const APP_RUNTIME_HELPER_OWNER_START_TIME_ENV = + "CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS"; +// Re-verify owner identity (not just PID liveness) at most this often; a +// process spawn per tick would cost more than the leak it prevents. +const APP_RUNTIME_HELPER_OWNER_IDENTITY_RECHECK_MS = 60_000; +// Status telemetry heartbeat: the tick's job is the timeout check, so status +// is republished only on change, plus a heartbeat so freshness readers work. +const APP_RUNTIME_HELPER_STATUS_HEARTBEAT_MS = 60_000; const DEFAULT_APP_RUNTIME_HELPER_DETACH_GRACE_MS = 5_000; const APP_RUNTIME_HELPER_LAUNCH_TIMEOUT_MS = 15_000; const APP_SERVER_SHIM_DIR_NAME = "app-server-shims"; @@ -94,37 +117,49 @@ const DEFAULT_STATUS_QUOTA_REFRESH_INTERVAL_MS = 10 * 60 * 1000; const STATUS_QUOTA_REFRESH_LOCK_STALE_MS = 10 * 60 * 1000; const STATUS_QUOTA_REFRESH_LOCK_DIR = "status-quota-refresh.lock"; const STARTUP_UPDATE_NOTICE_TIMED_OUT = Symbol("startup-update-notice-timed-out"); -let shadowHomeCleanupBusyFailuresRemaining = Number.parseInt( - process.env.CODEX_MULTI_AUTH_TEST_SHADOW_CLEANUP_BUSY_FAILURES ?? "0", - 10, -); -let shadowHomeCleanupPreflightReadBusyFailuresRemaining = Number.parseInt( - process.env.CODEX_MULTI_AUTH_TEST_SHADOW_PREFLIGHT_READ_BUSY_FAILURES ?? "0", - 10, -); -let shadowHomeSyncLockRecreateStaleCount = Number.parseInt( - process.env.CODEX_MULTI_AUTH_TEST_SHADOW_LOCK_RECREATE_STALE_COUNT ?? "0", - 10, + +// This wrapper is published (`package.json` ships `scripts/codex.js`), so every +// fault injector below runs in users' installs. Two guards keep them inert +// there. The first is an explicit opt-in: a single, greppable switch that must +// be set alongside any counter, so one stray counter in a shell profile or a CI +// environment cannot arm anything. The second is a strict parse — `parseInt` +// happily reads "2abc" as 2 and "1e3" as 1, which is how a value that was never +// meant to be a count arms an injector — so only a plain run of digits counts +// and everything else is zero. Zero means "never inject". +const TEST_FAULT_INJECTION_ENV = "CODEX_MULTI_AUTH_TEST_FAULT_INJECTION"; + +function resolveTestFaultInjectionCount(name, env = process.env) { + if (env[TEST_FAULT_INJECTION_ENV] !== "1") return 0; + const raw = (env[name] ?? "").trim(); + if (!/^\d+$/.test(raw)) return 0; + const parsed = Number.parseInt(raw, 10); + return Number.isSafeInteger(parsed) && parsed > 0 ? parsed : 0; +} + +let shadowHomeCleanupBusyFailuresRemaining = resolveTestFaultInjectionCount( + "CODEX_MULTI_AUTH_TEST_SHADOW_CLEANUP_BUSY_FAILURES", ); -let shadowHomeSyncMetadataBusyFailuresRemaining = Number.parseInt( - process.env.CODEX_MULTI_AUTH_TEST_SHADOW_SYNC_METADATA_BUSY_FAILURES ?? "0", - 10, +let shadowHomeCleanupPreflightReadBusyFailuresRemaining = + resolveTestFaultInjectionCount( + "CODEX_MULTI_AUTH_TEST_SHADOW_PREFLIGHT_READ_BUSY_FAILURES", + ); +let shadowHomeSyncLockRecreateStaleCount = resolveTestFaultInjectionCount( + "CODEX_MULTI_AUTH_TEST_SHADOW_LOCK_RECREATE_STALE_COUNT", ); -let shadowHomeSyncLockOwnerWriteFailuresRemaining = Number.parseInt( - process.env.CODEX_MULTI_AUTH_TEST_SHADOW_LOCK_OWNER_WRITE_FAILURES ?? "0", - 10, +let shadowHomeSyncMetadataBusyFailuresRemaining = resolveTestFaultInjectionCount( + "CODEX_MULTI_AUTH_TEST_SHADOW_SYNC_METADATA_BUSY_FAILURES", ); +let shadowHomeSyncLockOwnerWriteFailuresRemaining = + resolveTestFaultInjectionCount( + "CODEX_MULTI_AUTH_TEST_SHADOW_LOCK_OWNER_WRITE_FAILURES", + ); let appServerShimFileCleanupBusyFailuresRemaining = - Number.parseInt( - process.env.CODEX_MULTI_AUTH_TEST_APP_SERVER_SHIM_FILE_CLEANUP_BUSY_FAILURES ?? - "0", - 10, - ) || 0; -let appServerShimCopyBusyFailuresRemaining = - Number.parseInt( - process.env.CODEX_MULTI_AUTH_TEST_APP_SERVER_SHIM_COPY_BUSY_FAILURES ?? "0", - 10, - ) || 0; + resolveTestFaultInjectionCount( + "CODEX_MULTI_AUTH_TEST_APP_SERVER_SHIM_FILE_CLEANUP_BUSY_FAILURES", + ); +let appServerShimCopyBusyFailuresRemaining = resolveTestFaultInjectionCount( + "CODEX_MULTI_AUTH_TEST_APP_SERVER_SHIM_COPY_BUSY_FAILURES", +); const shadowHomeCleanupRetryMarkerDir = (process.env.CODEX_MULTI_AUTH_TEST_SHADOW_RETRY_MARKER_DIR ?? "").trim(); let warnedInvalidRuntimeRotationProxyEnv = false; @@ -3776,10 +3811,21 @@ function installRuntimeRotationAppServerCliShim(forwardedEnv, configArgs = []) { return shimDir; } -function resolveRuntimeRotationAppHelperStatusPath(env = process.env) { +// With a helper PID the path is per-helper, mirroring the owner files below — +// N concurrent helpers each publish their own status instead of last-writer- +// winning one shared file. Without a PID it is the legacy shared path, kept +// only so readers can still see a helper from before this change. +function resolveRuntimeRotationAppHelperStatusPath(env = process.env, helperPid) { const multiAuthDir = resolveOriginalMultiAuthDir(env) ?? join(resolveCodexHomeDir(env), "multi-auth"); - return join(multiAuthDir, APP_RUNTIME_HELPER_STATUS_FILE); + const statusFileName = + typeof helperPid === "number" && Number.isInteger(helperPid) && helperPid > 0 + ? APP_RUNTIME_HELPER_STATUS_FILE.replace( + /\.json$/i, + `.${helperPid}.json`, + ) + : APP_RUNTIME_HELPER_STATUS_FILE; + return join(multiAuthDir, statusFileName); } function resolveRuntimeRotationAppHelperOwnerPath(env = process.env, helperPid) { @@ -3852,6 +3898,96 @@ function resolveRuntimeRotationAppHelperOwnerPid(env = process.env) { return Number.isFinite(parsed) && parsed > 0 ? parsed : null; } +function resolveRuntimeRotationAppHelperMaxLifetimeMs(env = process.env) { + const parsed = Number.parseInt( + env.CODEX_MULTI_AUTH_APP_ROTATION_MAX_LIFETIME_MS ?? "", + 10, + ); + // 0 disables the ceiling explicitly; anything unset or invalid gets the + // default rather than unbounded life. + return Number.isFinite(parsed) && parsed >= 0 + ? parsed + : DEFAULT_APP_RUNTIME_HELPER_MAX_LIFETIME_MS; +} + +function resolveRuntimeRotationAppHelperOwnerStartTimeMs(env = process.env) { + const parsed = Number.parseInt( + env[APP_RUNTIME_HELPER_OWNER_START_TIME_ENV] ?? "", + 10, + ); + return Number.isFinite(parsed) && parsed > 0 ? parsed : null; +} + +// `lstart` is strftime-formatted and locale-sensitive; Date.parse on a +// localized string is implementation-defined and can yield NaN, which would +// silently disable the identity check. Both readers pin the C locale so both +// sides of every comparison parse the same shape. +function parseProcessStartTimeOutput(out) { + const trimmed = (out ?? "").trim(); + if (!trimmed) return null; + const parsed = Date.parse(trimmed); + return Number.isFinite(parsed) ? parsed : null; +} + +// The kernel's start time for a PID, in epoch ms — the identity that survives +// PID reuse. Null on platforms without `ps` (Windows) or for a PID that is +// already gone; callers must treat null as "identity unknown" and fall back +// to bare liveness rather than declaring the process dead. Synchronous — +// launcher/sweep use only; the helper's tick uses the async variant below. +// +// Windows short-circuits rather than spawning: there is no `ps` there, so the +// probe could only ever fail, and it is not called once — the launcher probes +// itself on every launch and the sweep probes up to `probeBudget` candidates. +// Paying a process spawn per probe to learn nothing is the whole cost. Windows +// therefore runs on bare liveness, and the 24h lifetime ceiling is what bounds +// a leak there. +function readProcessStartTimeMs(pid) { + if (process.platform === "win32") return null; + try { + return parseProcessStartTimeOutput( + execFileSync("ps", ["-o", "lstart=", "-p", String(pid)], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + env: { ...process.env, LC_ALL: "C" }, + timeout: 2_000, + }), + ); + } catch { + return null; + } +} + +// Async variant for the helper's status tick, which runs on the live rotation +// proxy's event loop: a wedged `ps` must stall a background probe, never an +// in-flight Responses stream. Same parse, same C locale, same null contract, +// and the same Windows short-circuit. +function readProcessStartTimeMsAsync(pid, onResult) { + if (process.platform === "win32") { + onResult(null); + return; + } + let child; + try { + child = execFile( + "ps", + ["-o", "lstart=", "-p", String(pid)], + { + encoding: "utf8", + env: { ...process.env, LC_ALL: "C" }, + timeout: 2_000, + }, + (error, stdout) => { + onResult(error ? null : parseProcessStartTimeOutput(stdout)); + }, + ); + } catch { + onResult(null); + return; + } + // The probe must not keep the helper's event loop referenced on shutdown. + child.unref?.(); +} + function isProcessAlive(pid) { try { process.kill(pid, 0); @@ -3861,8 +3997,109 @@ function isProcessAlive(pid) { } } -function isRuntimeRotationAppHelperOwnerAlive(pid) { - return isProcessAlive(pid); +// `kill(pid, 0)` answers "does *a* process hold this integer", never "is this +// still my owner". PIDs recycle, and a helper that mistakes a recycled PID for +// its owner pushes its idle deadline forward — a ratchet, because one false +// "alive" is never corrected by later true "dead"s, which is how helpers were +// observed running 33 hours past a 12-hour timeout. Identity is the owner's +// process start time, captured by the launcher at spawn: a recycled PID +// necessarily has a later start time, so the match fails and the helper +// correctly sees a dead owner. When the start time is unknown (no `ps`, or a +// pre-upgrade launcher), behavior degrades to the bare liveness check. +// +// The verdict is deliberately three-valued. "No owner PID was recorded" and +// "the owner is confirmed dead" are different facts, and collapsing them into +// one `false` makes a helper launched without an owner — invoked directly, or +// spawned by a pre-upgrade launcher that sets no owner PID — start the +// detached clock on its very first tick and reap itself while it is still +// serving. `unknown` means neither branch fires, which is exactly what the +// pre-#664 `if (ownerPid && isAlive(ownerPid))` guard did. +const OWNER_ALIVE = "alive"; +const OWNER_DEAD = "dead"; +const OWNER_UNKNOWN = "unknown"; + +function createRuntimeRotationAppHelperOwnerLivenessCheck( + ownerPid, + expectedStartTimeMs, + recheckIntervalMs = APP_RUNTIME_HELPER_OWNER_IDENTITY_RECHECK_MS, +) { + let lastIdentityCheckedAt = 0; + let lastIdentityVerdict = true; + let probeInFlight = false; + return (currentTime) => { + if (!ownerPid) { + return OWNER_UNKNOWN; + } + if (!isProcessAlive(ownerPid)) { + return OWNER_DEAD; + } + if (expectedStartTimeMs === null) { + return OWNER_ALIVE; + } + // The probe is asynchronous and single-flight: the tick runs on the live + // proxy's event loop, so it always answers from the last verdict and the + // probe updates it in the background — a stale verdict is tolerated by + // design (one recheck window against a 12h timeout), and single-flight + // means a wedged `ps` holds one child, not one per tick. + if ( + !probeInFlight && + currentTime - lastIdentityCheckedAt >= recheckIntervalMs + ) { + lastIdentityCheckedAt = currentTime; + probeInFlight = true; + readProcessStartTimeMsAsync(ownerPid, (actualStartTimeMs) => { + probeInFlight = false; + // A failed read is "identity unknown", not "owner dead": under the + // process-table pressure this fix exists for, fork itself can fail, + // and declaring a live owner dead would kill the proxy out from + // under an active session. Keep the verdict and retry next window. + if (actualStartTimeMs !== null) { + lastIdentityVerdict = actualStartTimeMs === expectedStartTimeMs; + } + }); + } + return lastIdentityVerdict ? OWNER_ALIVE : OWNER_DEAD; + }; +} + +function resolveRuntimeRotationAppHelperTickMs(idleTimeoutMs, detachedIdleMs) { + const shortestWindowMs = + detachedIdleMs > 0 ? Math.min(idleTimeoutMs, detachedIdleMs) : idleTimeoutMs; + return Math.min(1_000, Math.max(50, Math.floor(shortestWindowMs / 2))); +} + +// `lastIdentityVerdict` starts optimistic, so the first tick reports the owner +// alive while the async `ps` probe is still in flight, and a helper adopted by +// a recycled owner PID keeps refreshing its activity clock until the first +// recheck lands. Against the 12h default that is deliberate and harmless. It +// stops being harmless the moment the windows are compressed: a 60s recheck +// pinned against a 250ms idle override — how the lifecycle tests run — is +// longer than the entire window under test, so whether the verdict ever flips +// comes down to probe timing. Scaling the recheck to the shortest window that +// can fire makes the flip a property of the code rather than a race, and +// production is untouched because both defaults are hours. +function resolveRuntimeRotationAppHelperOwnerRecheckMs( + idleTimeoutMs, + detachedIdleMs, +) { + const shortestWindowMs = + detachedIdleMs > 0 ? Math.min(idleTimeoutMs, detachedIdleMs) : idleTimeoutMs; + return Math.min( + APP_RUNTIME_HELPER_OWNER_IDENTITY_RECHECK_MS, + Math.max(50, Math.floor(shortestWindowMs / 4)), + ); +} + +function resolveRuntimeRotationAppHelperDetachedIdleMs(env = process.env) { + const parsed = Number.parseInt( + env.CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS ?? "", + 10, + ); + // 0 disables the detached window explicitly, leaving a stranded helper on + // the full idle timeout — the pre-fix behavior, for anyone who depends on it. + return Number.isFinite(parsed) && parsed >= 0 + ? parsed + : DEFAULT_APP_RUNTIME_HELPER_DETACHED_IDLE_MS; } function resolveRuntimeRotationAppHelperDetachGraceMs(env = process.env) { @@ -3900,13 +4137,181 @@ function pickRuntimeRotationAppHelperEnv(env) { function writeRuntimeRotationAppHelperStatus(payload, env = process.env) { try { - const statusPath = resolveRuntimeRotationAppHelperStatusPath(env); + const statusPath = resolveRuntimeRotationAppHelperStatusPath( + env, + payload?.pid, + ); writeOwnerOnlyJsonFileAtomicSync(statusPath, payload); } catch { // Best-effort status only; the helper must not fail because telemetry is unavailable. } } +let helperMetadataCleanupBusyFailuresRemaining = resolveTestFaultInjectionCount( + "CODEX_MULTI_AUTH_TEST_HELPER_METADATA_CLEANUP_BUSY_FAILURES", +); + +function maybeThrowSimulatedHelperMetadataFileError() { + if ( + Number.isFinite(helperMetadataCleanupBusyFailuresRemaining) && + helperMetadataCleanupBusyFailuresRemaining > 0 + ) { + helperMetadataCleanupBusyFailuresRemaining -= 1; + const error = new Error("simulated EBUSY"); + error.code = "EBUSY"; + throw error; + } +} + +// Windows can hold a transient lock on a file another process just closed, so +// every metadata deletion goes through the shared retry rather than a bare +// rmSync — a swallowed EBUSY here is how stale files outlive their sweep. +function removeHelperMetadataFileWithRetry(targetPath) { + try { + withSynchronousFileOperationRetry(() => { + maybeThrowSimulatedHelperMetadataFileError(); + rmSync(targetPath, { force: true }); + }); + } catch { + // Best-effort metadata cleanup only; the next sweep retries. + } +} + +function removeRuntimeRotationAppHelperOwnerFile(env = process.env, helperPid) { + removeHelperMetadataFileWithRetry( + resolveRuntimeRotationAppHelperOwnerPath(env, helperPid), + ); +} + +// Owner and status files are written per helper PID and removed on clean +// helper exit; a killed helper leaves its files behind. This sweep runs when a +// launcher starts the next helper, mirroring the app-server shim-dir sweep: +// any per-PID metadata whose helper is no longer alive is stale, as is the +// legacy shared status file once the PID recorded inside it is dead. Terminal +// status stamps ("idle-timeout", "stopped") therefore survive until the next +// helper launch — long enough to be read, without accumulating forever. +function sweepStaleRuntimeRotationAppHelperMetadata(env = process.env) { + const multiAuthDir = + resolveOriginalMultiAuthDir(env) ?? join(resolveCodexHomeDir(env), "multi-auth"); + let entries = []; + try { + entries = readdirSync(multiAuthDir, { withFileTypes: true }); + } catch { + return; + } + // The same filename contract as `runtimeHelperPerPidPattern` in + // lib/runtime-constants.ts, re-derived here from the same two constants + // rather than imported: this wrapper has to keep working before `dist/` is + // built (see `loadRuntimeConstants`), and a sweep that silently matched + // nothing because an import failed would delete nothing and report success. + // A change to the shape there is a change here. + const perPidPattern = (baseName) => + new RegExp( + `^${baseName.replace(/\.json$/i, "").replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\.(\\d+)\\.json$`, + "i", + ); + const statusPattern = perPidPattern(APP_RUNTIME_HELPER_STATUS_FILE); + const ownerPattern = perPidPattern(APP_RUNTIME_HELPER_OWNER_FILE); + // A live kill(pid, 0) is not proof the helper is alive — the PID may have + // been recycled since a SIGKILLed helper left its files behind, and a + // recycled PID would otherwise shield the stale file from every future + // sweep. When the file records when its helper started, a current process + // whose kernel start time is meaningfully later cannot be that helper. + // Identity probes are bounded: results are memoized per PID for the sweep + // (the same PID backs both a status and an owner file), and at most a + // handful of `ps` spawns run per launch — candidates past the cap are + // treated as not-dead and the next launch finishes the work. Dead-PID + // files, the overwhelming majority after a leak, never probe at all. + const probedStartTimes = new Map(); + let probeBudget = 20; + const probeStartTime = (pid) => { + if (probedStartTimes.has(pid)) return probedStartTimes.get(pid); + if (probeBudget <= 0) return undefined; + probeBudget -= 1; + const startTime = readProcessStartTimeMs(pid); + probedStartTimes.set(pid, startTime); + return startTime; + }; + const isSweepCandidateDead = (pid, filePath) => { + if (!isProcessAlive(pid)) return true; + let recordedAt = null; + try { + const parsed = JSON.parse(readFileSync(filePath, "utf8")); + if (parsed && typeof parsed === "object") { + recordedAt = + typeof parsed.startedAt === "number" + ? parsed.startedAt + : typeof parsed.createdAt === "number" + ? parsed.createdAt + : null; + } + } catch { + return false; + } + if (recordedAt === null) return false; + const actualStartTimeMs = probeStartTime(pid); + if (actualStartTimeMs === null || actualStartTimeMs === undefined) { + return false; + } + return actualStartTimeMs > recordedAt + 60_000; + }; + // Classifying a file as stale and deleting it are two moments, and a PID + // freed between them can be handed to a helper starting right now — which + // then republishes this exact path before the delete lands, and the sweep + // erases a live helper's metadata: invisible to `rotation status`, to + // runtime account resolution, and to `unbind-app`, reapable only by its own + // timers. Deleting only a file whose mtime still matches what was + // classified closes that window; a file rewritten underneath us is by + // definition not the one judged dead. + const removeIfUnchanged = (entryPath, classifiedMtimeMs) => { + if (classifiedMtimeMs !== null) { + let currentMtimeMs = null; + try { + currentMtimeMs = statSync(entryPath).mtimeMs; + } catch { + return; + } + if (currentMtimeMs !== classifiedMtimeMs) return; + } + removeHelperMetadataFileWithRetry(entryPath); + }; + for (const entry of entries) { + if (!entry.isFile()) continue; + const match = + statusPattern.exec(entry.name) ?? ownerPattern.exec(entry.name); + if (!match) continue; + const pid = Number.parseInt(match[1], 10); + if (!Number.isInteger(pid) || pid <= 0) continue; + const entryPath = join(multiAuthDir, entry.name); + let classifiedMtimeMs = null; + try { + classifiedMtimeMs = statSync(entryPath).mtimeMs; + } catch { + continue; + } + if (!isSweepCandidateDead(pid, entryPath)) continue; + removeIfUnchanged(entryPath, classifiedMtimeMs); + } + const legacyStatusPath = join(multiAuthDir, APP_RUNTIME_HELPER_STATUS_FILE); + try { + const legacyMtimeMs = statSync(legacyStatusPath).mtimeMs; + const parsed = JSON.parse(readFileSync(legacyStatusPath, "utf8")); + const legacyPid = + parsed && typeof parsed === "object" && Number.isInteger(parsed.pid) + ? parsed.pid + : null; + if ( + legacyPid === null || + legacyPid <= 0 || + isSweepCandidateDead(legacyPid, legacyStatusPath) + ) { + removeIfUnchanged(legacyStatusPath, legacyMtimeMs); + } + } catch { + // Missing or unreadable legacy status; nothing to sweep. + } +} + function writeRuntimeRotationAppHelperOwner( identityToken, helperPid, @@ -3941,6 +4346,7 @@ function createRuntimeRotationAppHelperStatus({ identityToken, idleTimeoutMs, lastActivityAt, + idleExpiresAt, state, }) { const proxyStatus = @@ -3964,7 +4370,13 @@ function createRuntimeRotationAppHelperStatus({ updatedAt: Date.now(), baseUrl: proxyServer?.baseUrl ?? null, idleTimeoutMs, - idleExpiresAt: lastActivityAt + idleTimeoutMs, + // The reaper's real deadline, which is the detached window once the owner + // is gone. Reporting the raw idle timeout there would tell `rotation + // status` a helper has 12h left when it has minutes. + idleExpiresAt: + typeof idleExpiresAt === "number" + ? idleExpiresAt + : lastActivityAt + idleTimeoutMs, totalRequests: proxyStatus.totalRequests ?? 0, upstreamRequests: proxyStatus.upstreamRequests ?? 0, retries: proxyStatus.retries ?? 0, @@ -3985,21 +4397,97 @@ async function runRuntimeRotationAppHelper(identityToken = "") { let closing = false; const startedAt = Date.now(); const idleTimeoutMs = resolveRuntimeRotationAppHelperIdleMs(); + const maxLifetimeMs = resolveRuntimeRotationAppHelperMaxLifetimeMs(); + const detachedIdleMs = resolveRuntimeRotationAppHelperDetachedIdleMs(); const ownerPid = resolveRuntimeRotationAppHelperOwnerPid(); + const isOwnerAlive = createRuntimeRotationAppHelperOwnerLivenessCheck( + ownerPid, + resolveRuntimeRotationAppHelperOwnerStartTimeMs(), + resolveRuntimeRotationAppHelperOwnerRecheckMs(idleTimeoutMs, detachedIdleMs), + ); let lastActivityAt = startedAt; let lastRequestCount = 0; + let lastPublishedToken = null; + let lastPublishedAt = 0; + // When the owner was first confirmed dead. Null while it is alive, and reset + // to null if a later probe revives the verdict, so a transient "dead" cannot + // ratchet the detached deadline the way the old liveness check ratcheted the + // idle one. + let ownerGoneSince = null; + // A detached consumer holding a socket is evidence the handoff was real, + // so it blocks the detached reap even with no requests in flight. The + // absence of that evidence is not evidence of absence, and this fails + // open on purpose: a proxy that cannot report connections reads as zero, + // so the detached window still reaps it on activity alone. Erring the + // other way — treating "unknown" as "someone is attached" — would restore + // the leak for any shape that stopped answering. + const countOpenConnections = () => { + if (typeof proxyServer?.getOpenConnectionCount !== "function") return 0; + const open = proxyServer.getOpenConnectionCount(); + // Only a positive count of sockets is evidence of a consumer. Anything + // else — negative, fractional, NaN, Infinity — is "unknown", and unknown + // degrades exactly the way a missing method does. Comparing a garbage + // reading against 0 directly would block the reap forever and silently + // restore the leak this exists to close, which is the one direction the + // fix cannot afford to fail in. + return Number.isSafeInteger(open) && open > 0 ? open : 0; + }; + // The deadline the reaper will actually enforce, which is the earlier of the + // idle timeout and — once the owner is gone — the detached window. + const resolveIdleDeadline = () => + ownerGoneSince !== null && detachedIdleMs > 0 + ? Math.min( + lastActivityAt + idleTimeoutMs, + Math.max(lastActivityAt, ownerGoneSince) + detachedIdleMs, + ) + : lastActivityAt + idleTimeoutMs; + // Freshness readers tolerate hours of staleness, but tests run the whole + // lifecycle in milliseconds — heartbeat at least once per idle window. + // + // The detached window counts too. `publishToken` deliberately zeroes + // `idleExpiresAt` so a deadline that moves every tick is not a reason to + // rewrite the file, which means the published deadline only catches up on a + // heartbeat. Once the owner dies the real deadline collapses from the idle + // timeout to the detached window, so a heartbeat pinned to the idle window + // alone would leave `rotation status` advertising a 12-hour deadline for a + // helper that is seconds from exiting — worse under a short + // CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS override, where the helper + // can vanish before the file is ever corrected. + const statusHeartbeatMs = Math.min( + APP_RUNTIME_HELPER_STATUS_HEARTBEAT_MS, + idleTimeoutMs, + detachedIdleMs > 0 ? detachedIdleMs : Number.POSITIVE_INFINITY, + ); - const publishStatus = (state) => { - writeRuntimeRotationAppHelperStatus( - createRuntimeRotationAppHelperStatus({ - proxyServer, - startedAt, - identityToken, - idleTimeoutMs, - lastActivityAt, - state, - }), - ); + const publishStatus = (state, { force = false } = {}) => { + const payload = createRuntimeRotationAppHelperStatus({ + proxyServer, + startedAt, + identityToken, + idleTimeoutMs, + lastActivityAt, + idleExpiresAt: resolveIdleDeadline(), + state, + }); + // `updatedAt` moves every call and `idleExpiresAt` moves every tick the + // owner is alive; neither is a reason to rewrite the file. Everything + // else changing — state, traffic counters, account fields — is. + const publishToken = JSON.stringify({ + ...payload, + updatedAt: 0, + idleExpiresAt: 0, + }); + const now = Date.now(); + if ( + !force && + publishToken === lastPublishedToken && + now - lastPublishedAt < statusHeartbeatMs + ) { + return; + } + lastPublishedToken = publishToken; + lastPublishedAt = now; + writeRuntimeRotationAppHelperStatus(payload); }; const cleanup = async (state = "stopped") => { @@ -4022,7 +4510,11 @@ async function runRuntimeRotationAppHelper(identityToken = "") { try { await proxyServer?.close?.(); } finally { - publishStatus(state); + // The terminal stamp is always written; the next launcher's sweep + // removes it once this PID is dead. The owner file has no + // post-mortem value, so it goes now. + publishStatus(state, { force: true }); + removeRuntimeRotationAppHelperOwnerFile(process.env, process.pid); } } }; @@ -4095,7 +4587,10 @@ async function runRuntimeRotationAppHelper(identityToken = "") { type: "ready", pid: process.pid, baseUrl: proxyServer.baseUrl, - statusPath: resolveRuntimeRotationAppHelperStatusPath(), + statusPath: resolveRuntimeRotationAppHelperStatusPath( + process.env, + process.pid, + ), args: runtimeContext.args ?? [], env: pickRuntimeRotationAppHelperEnv(runtimeContext.env), })}\n`, @@ -4108,14 +4603,51 @@ async function runRuntimeRotationAppHelper(identityToken = "") { lastRequestCount = requestCount; lastActivityAt = currentTime; } - if (ownerPid && isRuntimeRotationAppHelperOwnerAlive(ownerPid)) { + const ownerVerdict = isOwnerAlive(currentTime); + if (ownerVerdict === OWNER_ALIVE) { lastActivityAt = currentTime; + ownerGoneSince = null; + } else if (ownerVerdict === OWNER_DEAD && ownerGoneSince === null) { + ownerGoneSince = currentTime; } publishStatus("running"); if (currentTime - lastActivityAt >= idleTimeoutMs) { exitAfterCleanup("idle-timeout", 0); + } else if ( + ownerGoneSince !== null && + detachedIdleMs > 0 && + currentTime >= resolveIdleDeadline() && + requestCount === 0 && + countOpenConnections() === 0 + ) { + // The launcher is gone, nothing is connected, nothing has been + // proxied for the detached window, and nothing has *ever* been + // proxied: this helper was stranded by a launcher that exited, not + // handed to a consumer that wants it. + // + // The never-served gate is what separates the two. An open socket + // is not a durable signal — the proxy leaves `keepAliveTimeout` at + // Node's 5s default, so a `codex app` session that is merely idle + // between turns has zero sockets within seconds, and the socket + // check alone would reap the live proxy out from under the desktop + // app after the detached window. A helper that has served even one + // request was genuinely handed off; from then on the idle timeout + // and the lifetime ceiling bound it, exactly as they did before the + // detached reap existed. + exitAfterCleanup("owner-gone", 0); + } else if ( + maxLifetimeMs > 0 && + currentTime - startedAt >= maxLifetimeMs + ) { + // The ceiling is deliberately unconditional on activity: it exists + // for exactly the case where activity accounting is wrong. + exitAfterCleanup("max-lifetime", 0); } - }, Math.min(1_000, Math.max(50, Math.floor(idleTimeoutMs / 2)))); + // Tick against the shortest window that can fire, so a short detached + // window is enforced at its own resolution rather than the idle + // timeout's. Both defaults are far above 2s, so production still ticks + // once a second. + }, resolveRuntimeRotationAppHelperTickMs(idleTimeoutMs, detachedIdleMs)); } catch (error) { process.stdout.write( `${JSON.stringify({ @@ -4203,13 +4735,25 @@ function startRuntimeRotationAppHelper(baseContext, options = {}) { let stderrBuffer = ""; let settled = false; const identityToken = randomBytes(24).toString("hex"); + // The launcher states its own identity — PID plus kernel start time — so + // the helper's owner-liveness check can tell "my launcher" from a later + // process that recycled the PID. An empty value (no `ps` on this + // platform) leaves the helper on the bare liveness check. + const launcherStartTimeMs = readProcessStartTimeMs(process.pid); const helperEnv = { ...baseContext.env, CODEX_MULTI_AUTH_DIR: resolveRuntimeRotationOriginalMultiAuthDir( realCodexHome, baseContext.env, ), + // PID and start time are two halves of one identity and must describe + // the same process: both always come from this launcher's own capture, + // never from an inherited environment value, which would marry this + // PID to another process's start time and make the helper declare its + // live owner dead at the first recheck. [APP_RUNTIME_HELPER_OWNER_PID_ENV]: String(process.pid), + [APP_RUNTIME_HELPER_OWNER_START_TIME_ENV]: + launcherStartTimeMs !== null ? String(launcherStartTimeMs) : "", [APP_RUNTIME_HELPER_REAL_CODEX_HOME_ENV]: realCodexHome, [APP_RUNTIME_HELPER_USE_CANONICAL_HOME_ENV]: options.useCanonicalHome === true ? "1" : "0", @@ -4230,6 +4774,14 @@ function startRuntimeRotationAppHelper(baseContext, options = {}) { }, ); writeRuntimeRotationAppHelperOwner(identityToken, helper.pid, helperEnv); + // Swept after the spawn, never before it. The sweep is synchronous and + // unbounded — a readdir, a readFileSync and an rmSync per candidate, plus + // bounded `ps` probes — and the state it exists to clean up (hundreds of + // stale files, a loaded process table) is exactly the state that makes it + // slow. Running it first put all of that latency in front of `codex app` + // and TUI startup; running it here lets the helper boot in parallel with + // it. Nothing about spawning depends on the sweep having finished. + sweepStaleRuntimeRotationAppHelperMetadata(helperEnv); let timeout = null; const finish = (result) => { if (settled) return; diff --git a/test/app-bind.test.ts b/test/app-bind.test.ts index 54750098..eed6fec6 100644 --- a/test/app-bind.test.ts +++ b/test/app-bind.test.ts @@ -17,10 +17,17 @@ import { stopDetachedProcess, stopRuntimeRotationAppHelperProcess, stopRuntimeRotationRouterProcess, + UNBIND_HELPER_CONCURRENCY, unbindCodexAppRuntimeRotation, } from "../lib/runtime/app-bind.js"; import { tomlStringLiteral } from "../lib/runtime/config-toml.js"; import { withFileOperationRetry } from "../lib/fs-retry.js"; +import { + withDeadPid, + withDeadPids, + withLivePid, + withLivePids, +} from "./helpers/owned-pids.js"; import { APP_RUNTIME_HELPER_OWNER_FILE, APP_RUNTIME_HELPER_STATUS_FILE, @@ -1109,25 +1116,354 @@ describe("Codex app runtime rotation bind", () => { CODEX_MULTI_AUTH_DIR: join(root, "multi-auth"), CODEX_MULTI_AUTH_APP_BIND_CODEX_HOME: join(root, "codex-home"), }; - const statusPath = await writeRuntimeHelperStatus( - { home: root, env }, - { + // A PID this test started and killed, rather than an integer above the + // platform's PID ceiling. Out-of-range PIDs classify as dead only because + // every liveness check here treats every errno but EPERM as dead — true + // today, but a property the fixture never stated and does not control + // (#668). "Has already exited" should be a fact. + await withDeadPid(async (deadPid) => { + const statusPath = await writeRuntimeHelperStatus( + { home: root, env }, + { + version: 1, + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: deadPid, + startedAt: Date.now(), + scriptPath: join(root, "runtime-helper.mjs"), + }, + ); + + await unbindCodexAppRuntimeRotation({ + platform: process.platform, + home: root, + env, + }); + + expect(existsSync(statusPath)).toBe(false); + }); + }); + + it("removes dead helpers recorded in per-PID status files on unbind", async () => { + // Helpers publish `runtime-rotation-app-helper..json`; unbind must + // walk those, not just the legacy shared path — a regression here means + // `uninstall` silently stops nothing while reporting success. + const root = await createTempRoot("codex-app-bind-helper-per-pid-"); + const env = { + CODEX_MULTI_AUTH_DIR: join(root, "multi-auth"), + CODEX_MULTI_AUTH_APP_BIND_CODEX_HOME: join(root, "codex-home"), + }; + const legacyPath = resolveRuntimeHelperStatusPath({ home: root, env }); + await withDeadPid(async (deadPid) => { + const perPidPath = legacyPath.replace(/\.json$/i, `.${deadPid}.json`); + await mkdir(dirname(perPidPath), { recursive: true }); + await writeFile( + perPidPath, + `${JSON.stringify({ + version: 1, + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: deadPid, + startedAt: Date.now(), + scriptPath: join(root, "runtime-helper.mjs"), + })}\n`, + "utf8", + ); + + await unbindCodexAppRuntimeRotation({ + platform: process.platform, + home: root, + env, + }); + + expect(existsSync(perPidPath)).toBe(false); + }); + }); + + it("removes every dead helper record — per-PID and legacy — in one unbind", async () => { + // A loop bug that processes only the first candidate would still pass the + // single-file test above; this is the actual multi-helper regression. + const root = await createTempRoot("codex-app-bind-helper-multi-"); + const env = { + CODEX_MULTI_AUTH_DIR: join(root, "multi-auth"), + CODEX_MULTI_AUTH_APP_BIND_CODEX_HOME: join(root, "codex-home"), + }; + const legacyPath = resolveRuntimeHelperStatusPath({ home: root, env }); + await mkdir(dirname(legacyPath), { recursive: true }); + const record = (pid: number) => + `${JSON.stringify({ version: 1, kind: "codex-app-runtime-rotation-helper", state: "running", - pid: 2_147_483_647, + pid, startedAt: Date.now(), scriptPath: join(root, "runtime-helper.mjs"), + })}\n`; + await withDeadPids( + 3, + async ([firstDeadPid, secondDeadPid, legacyDeadPid]) => { + const deadPids = [firstDeadPid ?? 0, secondDeadPid ?? 0]; + const perPidPaths = deadPids.map((pid) => + legacyPath.replace(/\.json$/i, `.${pid}.json`), + ); + for (const [index, path] of perPidPaths.entries()) { + await writeFile(path, record(deadPids[index] ?? 0), "utf8"); + } + await writeFile(legacyPath, record(legacyDeadPid ?? 0), "utf8"); + // An owner file beside a dead per-PID record goes with it — and its + // identity token deliberately disagrees with the status record's, + // because a dead PID means neither file describes anything that can + // still be running (#666). Gating this removal on token agreement is + // what stranded owner files forever. + const ownerPath = resolveRuntimeHelperOwnerPath( + { home: root, env }, + deadPids[0] ?? 0, + ); + await writeFile( + ownerPath, + `${JSON.stringify({ + version: 1, + kind: "codex-app-runtime-rotation-helper-owner", + identityToken: "does-not-matter-for-dead-pid", + launcherPid: 1, + createdAt: Date.now(), + })}\n`, + "utf8", + ); + + await unbindCodexAppRuntimeRotation({ + platform: process.platform, + home: root, + env, + }); + + for (const path of [...perPidPaths, legacyPath]) { + expect(existsSync(path)).toBe(false); + } + expect(existsSync(ownerPath)).toBe(false); }, ); + }); + + it("processes every helper record without exceeding the unbind concurrency bound", async () => { + // The pool exists so unbind costs roughly one stop window instead of N of + // them — on the machine from #663 there were 183 records — while not + // signalling every stale helper at once. Every other fixture here has a + // handful of records, so any width (1, 8, Infinity) behaves identically + // and the bound ships unobserved. + // + // Live PIDs with agreeing owner tokens, because only that combination + // reaches the stop path — and `verifyProcessIdentity` is the one seam on + // it, so it is where the pool's real width is visible. Returning false + // means nothing is ever signalled: these are the test's own children. + const root = await createTempRoot("codex-app-bind-helper-pool-"); + const env = { + CODEX_MULTI_AUTH_DIR: join(root, "multi-auth"), + CODEX_MULTI_AUTH_APP_BIND_CODEX_HOME: join(root, "codex-home"), + }; + const legacyPath = resolveRuntimeHelperStatusPath({ home: root, env }); + await mkdir(dirname(legacyPath), { recursive: true }); + const recordCount = UNBIND_HELPER_CONCURRENCY * 2; + await withLivePids(recordCount, async (livePids) => { + for (const pid of livePids) { + await writeFile( + legacyPath.replace(/\.json$/i, `.${pid}.json`), + `${JSON.stringify({ + version: 1, + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid, + startedAt: Date.now(), + scriptPath: join(root, "runtime-helper.mjs"), + identityToken: `token-${pid}`, + })}\n`, + "utf8", + ); + await writeFile( + resolveRuntimeHelperOwnerPath({ home: root, env }, pid), + `${JSON.stringify({ + version: 1, + kind: "codex-app-runtime-rotation-helper-owner", + identityToken: `token-${pid}`, + launcherPid: 1, + createdAt: Date.now(), + })}\n`, + "utf8", + ); + } + + let inFlight = 0; + let peakInFlight = 0; + let verified = 0; + await unbindCodexAppRuntimeRotation({ + platform: process.platform, + home: root, + env, + verifyProcessIdentity: async () => { + inFlight += 1; + verified += 1; + peakInFlight = Math.max(peakInFlight, inFlight); + await new Promise((resolve) => setTimeout(resolve, 10)); + inFlight -= 1; + return false; + }, + }); + + // Every record reached the stop path — a pool that dropped items after + // the first batch would fail here, not on the bound. + expect(verified).toBe(recordCount); + // More than one at a time, so the work really is parallel... + expect(peakInFlight).toBeGreaterThan(1); + // ...and never more than the bound, so it is really bounded. + expect(peakInFlight).toBeLessThanOrEqual(UNBIND_HELPER_CONCURRENCY); + }); + }); + + it("removes both files when a dead helper's status and owner tokens disagree", async () => { + // #666: the dead-PID branch removed the status file but gated the owner + // file on `helperOwnershipMatches`. A token mismatch therefore deleted the + // status record and kept `runtime-rotation-app-helper-owner..json` — + // and because unbind then enumerated status paths only, nothing ever + // rediscovered that owner file again. + const root = await createTempRoot("codex-app-bind-helper-mismatch-"); + const env = { + CODEX_MULTI_AUTH_DIR: join(root, "multi-auth"), + CODEX_MULTI_AUTH_APP_BIND_CODEX_HOME: join(root, "codex-home"), + }; + const legacyPath = resolveRuntimeHelperStatusPath({ home: root, env }); + await mkdir(dirname(legacyPath), { recursive: true }); + await withDeadPid(async (deadPid) => { + const perPidPath = legacyPath.replace(/\.json$/i, `.${deadPid}.json`); + await writeFile( + perPidPath, + `${JSON.stringify({ + version: 1, + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: deadPid, + startedAt: Date.now(), + scriptPath: join(root, "runtime-helper.mjs"), + identityToken: "token-from-the-status-file", + })}\n`, + "utf8", + ); + const ownerPath = resolveRuntimeHelperOwnerPath( + { home: root, env }, + deadPid, + ); + await writeFile( + ownerPath, + `${JSON.stringify({ + version: 1, + kind: "codex-app-runtime-rotation-helper-owner", + identityToken: "a-different-token-entirely", + launcherPid: 1, + createdAt: Date.now(), + })}\n`, + "utf8", + ); + + await unbindCodexAppRuntimeRotation({ + platform: process.platform, + home: root, + env, + }); + + expect(existsSync(perPidPath)).toBe(false); + expect(existsSync(ownerPath)).toBe(false); + }); + }); + + it("reclaims an orphaned owner file that has no status record left", async () => { + // #666: the accumulation this fixes. An owner file whose status file is + // already gone was unreachable — every pass walked status paths only — so + // on a machine that stopped launching helpers it stayed under the + // multi-auth root forever. Enumerating owner files is what reclaims it. + const root = await createTempRoot("codex-app-bind-helper-orphan-"); + const env = { + CODEX_MULTI_AUTH_DIR: join(root, "multi-auth"), + CODEX_MULTI_AUTH_APP_BIND_CODEX_HOME: join(root, "codex-home"), + }; + const legacyPath = resolveRuntimeHelperStatusPath({ home: root, env }); + await mkdir(dirname(legacyPath), { recursive: true }); + await withDeadPid(async (deadPid) => { + await withLivePid(async (livePid) => { + const orphanOwnerPath = resolveRuntimeHelperOwnerPath( + { home: root, env }, + deadPid, + ); + const ownerContent = (pid: number) => + `${JSON.stringify({ + version: 1, + kind: "codex-app-runtime-rotation-helper-owner", + identityToken: `token-${pid}`, + launcherPid: 1, + createdAt: Date.now(), + })}\n`; + await writeFile(orphanOwnerPath, ownerContent(deadPid), "utf8"); + // A live helper's owner file is not an orphan and must survive, even + // though it too has no status record in this fixture. + const liveOwnerPath = resolveRuntimeHelperOwnerPath( + { home: root, env }, + livePid, + ); + await writeFile(liveOwnerPath, ownerContent(livePid), "utf8"); + + await unbindCodexAppRuntimeRotation({ + platform: process.platform, + home: root, + env, + }); + + expect(existsSync(orphanOwnerPath)).toBe(false); + expect(existsSync(liveOwnerPath)).toBe(true); + }); + }); + }); + + it("preserves a running per-PID helper whose ownership cannot be verified", async () => { + // The ownership gate is what keeps unbind from signalling foreign PIDs; a + // future change that drops it must fail here, not in production. + const root = await createTempRoot("codex-app-bind-helper-foreign-"); + const env = { + CODEX_MULTI_AUTH_DIR: join(root, "multi-auth"), + CODEX_MULTI_AUTH_APP_BIND_CODEX_HOME: join(root, "codex-home"), + }; + const legacyPath = resolveRuntimeHelperStatusPath({ home: root, env }); + await mkdir(dirname(legacyPath), { recursive: true }); + // A live PID (this test process) with an identityToken and no owner file: + // ownership cannot be verified, so the record must survive with a warning. + const perPidPath = legacyPath.replace(/\.json$/i, `.${process.pid}.json`); + await writeFile( + perPidPath, + `${JSON.stringify({ + version: 1, + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: process.pid, + startedAt: Date.now(), + scriptPath: join(root, "runtime-helper.mjs"), + identityToken: "token-without-owner-file", + })}\n`, + "utf8", + ); + const logs: string[] = []; await unbindCodexAppRuntimeRotation({ platform: process.platform, home: root, env, + log: (message) => { + logs.push(message); + }, }); - expect(existsSync(statusPath)).toBe(false); + expect(existsSync(perPidPath)).toBe(true); + expect( + logs.some((message) => + message.includes("ownership metadata does not match"), + ), + ).toBe(true); }); it("fails fast when the router script cannot be resolved", async () => { diff --git a/test/app-helper-selection.test.ts b/test/app-helper-selection.test.ts new file mode 100644 index 00000000..0297673c --- /dev/null +++ b/test/app-helper-selection.test.ts @@ -0,0 +1,229 @@ +import { readFileSync } from "node:fs"; +import process from "node:process"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it } from "vitest"; +import { + RUNTIME_HELPER_CLOCK_TOLERANCE_MS, + RUNTIME_HELPER_STATUS_STALE_MS, + isLiveRuntimeHelper, + isRuntimeHelperProcessAlive, + liveRuntimeHelpers, + readRuntimeHelperPid, + selectRuntimeHelperStatus, + type RuntimeHelperSelectable, +} from "../lib/runtime/app-helper-selection.js"; +import { withDeadPid, withLivePid } from "./helpers/owned-pids.js"; + +// These four predicates decide whether a helper is reported as live, which +// helper `rotation status` names, and which account is marked `current`. They +// are exercised indirectly through two readers, where an inverted comparison or +// a reordered guard can hide behind a fixture that happens to agree. Pin them +// directly. + +const NOW = 1_700_000_000_000; + +function helper( + overrides: Partial = {}, +): RuntimeHelperSelectable { + return { + state: "running", + pid: process.pid, + startedAt: NOW - 60_000, + updatedAt: NOW - 1_000, + ...overrides, + }; +} + +describe("readRuntimeHelperPid", () => { + it("accepts only positive integers", () => { + expect(readRuntimeHelperPid(1)).toBe(1); + expect(readRuntimeHelperPid(4242)).toBe(4242); + }); + + it.each([ + ["zero", 0], + ["negative", -1234], + ["fractional", 4242.5], + ["NaN", Number.NaN], + ["Infinity", Number.POSITIVE_INFINITY], + ["null", null], + ["undefined", undefined], + ["a numeric string", "4242"], + ["an object", { pid: 4242 }], + ])("rejects %s", (_label, value) => { + expect(readRuntimeHelperPid(value)).toBeNull(); + }); +}); + +describe("isRuntimeHelperProcessAlive", () => { + it("reports a live PID as alive and a reaped one as dead", async () => { + await withLivePid((livePid) => { + expect(isRuntimeHelperProcessAlive(livePid)).toBe(true); + }); + await withDeadPid((deadPid) => { + expect(isRuntimeHelperProcessAlive(deadPid)).toBe(false); + }); + }); + + it("never probes a negative PID", () => { + // `process.kill(-1234, 0)` is a POSIX process-*group* probe and succeeds on + // any busy machine, so a record carrying a negative PID would otherwise + // report a helper that does not exist as live. The guard has to reject it + // before the syscall, not interpret the syscall's answer. + expect(isRuntimeHelperProcessAlive(-1234)).toBe(false); + expect(isRuntimeHelperProcessAlive(-1)).toBe(false); + expect(isRuntimeHelperProcessAlive(0)).toBe(false); + }); +}); + +describe("isLiveRuntimeHelper", () => { + it("accepts a running, live, freshly-published helper", () => { + expect(isLiveRuntimeHelper(helper(), NOW)).toBe(true); + }); + + it.each([ + ["stopped", "stopped"], + ["idle-timeout", "idle-timeout"], + ["max-lifetime", "max-lifetime"], + ["owner-gone", "owner-gone"], + ["error", "error"], + ["an unknown future state", "something-new"], + ["no state at all", null], + ])("rejects state %s even with a live PID", (_label, state) => { + expect(isLiveRuntimeHelper(helper({ state }), NOW)).toBe(false); + }); + + it("rejects a dead PID", async () => { + await withDeadPid((deadPid) => { + expect(isLiveRuntimeHelper(helper({ pid: deadPid }), NOW)).toBe(false); + }); + }); + + it("rejects a record older than the staleness window", () => { + const justInside = helper({ + updatedAt: NOW - RUNTIME_HELPER_STATUS_STALE_MS, + }); + const justOutside = helper({ + updatedAt: NOW - RUNTIME_HELPER_STATUS_STALE_MS - 1, + }); + expect(isLiveRuntimeHelper(justInside, NOW)).toBe(true); + expect(isLiveRuntimeHelper(justOutside, NOW)).toBe(false); + }); + + it("falls back to bare liveness when the record has no updatedAt", () => { + // Predates the heartbeat contract, so freshness is unknowable rather than + // bad. Discarding it would be stricter than the behaviour it replaced. + expect(isLiveRuntimeHelper(helper({ updatedAt: null }), NOW)).toBe(true); + }); + + it("tolerates a startedAt slightly in the future but not a bogus one", () => { + const withinSkew = helper({ + startedAt: NOW + RUNTIME_HELPER_CLOCK_TOLERANCE_MS, + }); + const beyondSkew = helper({ + startedAt: NOW + RUNTIME_HELPER_CLOCK_TOLERANCE_MS + 1, + }); + expect(isLiveRuntimeHelper(withinSkew, NOW)).toBe(true); + expect(isLiveRuntimeHelper(beyondSkew, NOW)).toBe(false); + }); + + it("ignores a missing startedAt", () => { + expect(isLiveRuntimeHelper(helper({ startedAt: null }), NOW)).toBe(true); + }); +}); + +describe("selectRuntimeHelperStatus", () => { + it("returns null for an empty set", () => { + expect(selectRuntimeHelperStatus([], NOW)).toBeNull(); + }); + + it("prefers the most recently updated live helper", () => { + const older = helper({ updatedAt: NOW - 30_000 }); + const newer = helper({ updatedAt: NOW - 1_000 }); + // Both orderings, so the result cannot come from input order. + expect(selectRuntimeHelperStatus([older, newer], NOW)).toBe(newer); + expect(selectRuntimeHelperStatus([newer, older], NOW)).toBe(newer); + }); + + it("prefers any live helper over a fresher dead one", async () => { + await withDeadPid((deadPid) => { + const live = helper({ updatedAt: NOW - 30_000 }); + const deadButFresher = helper({ pid: deadPid, updatedAt: NOW }); + expect(selectRuntimeHelperStatus([deadButFresher, live], NOW)).toBe(live); + }); + }); + + it("falls back to the freshest record when nothing is live", async () => { + await withDeadPid((deadPid) => { + const older = helper({ + pid: deadPid, + state: "idle-timeout", + updatedAt: NOW - 60_000, + }); + const newer = helper({ + pid: deadPid, + state: "stopped", + updatedAt: NOW - 10_000, + }); + expect(selectRuntimeHelperStatus([older, newer], NOW)).toBe(newer); + }); + }); + + it("does not mutate the caller's array", () => { + const older = helper({ updatedAt: NOW - 30_000 }); + const newer = helper({ updatedAt: NOW - 1_000 }); + const statuses = [older, newer]; + selectRuntimeHelperStatus(statuses, NOW); + expect(statuses[0]).toBe(older); + expect(statuses[1]).toBe(newer); + }); +}); + +describe("liveRuntimeHelpers", () => { + it("counts only the live ones and preserves input order", async () => { + await withDeadPid((deadPid) => { + const first = helper({ updatedAt: NOW - 5_000 }); + const dead = helper({ pid: deadPid }); + const stale = helper({ + updatedAt: NOW - RUNTIME_HELPER_STATUS_STALE_MS - 1, + }); + const second = helper({ updatedAt: NOW - 1_000 }); + expect(liveRuntimeHelpers([first, dead, stale, second], NOW)).toEqual([ + first, + second, + ]); + }); + }); +}); + +describe("staleness window versus the wrapper's heartbeat", () => { + it("stays well above the wrapper's publish cadence", () => { + // The staleness window only works because a live helper republishes far + // more often than it. That cadence is `APP_RUNTIME_HELPER_STATUS_HEARTBEAT_MS` + // in `scripts/codex.js`, which this module cannot import — the wrapper has + // to run before `dist/` exists, so the constant cannot be shared. Nothing + // else links the two numbers, so raising the heartbeat past a tenth of the + // staleness window would silently start declaring live helpers dead. Read + // it out of the wrapper and fail loudly here instead. + const wrapperPath = fileURLToPath( + new URL("../scripts/codex.js", import.meta.url), + ); + const wrapper = readFileSync(wrapperPath, "utf8"); + const match = /APP_RUNTIME_HELPER_STATUS_HEARTBEAT_MS\s*=\s*([0-9_]+)/.exec( + wrapper, + ); + expect(match?.[1]).toBeDefined(); + const heartbeatMs = Number.parseInt( + (match?.[1] ?? "").replace(/_/g, ""), + 10, + ); + expect(Number.isSafeInteger(heartbeatMs)).toBe(true); + expect(heartbeatMs).toBeGreaterThan(0); + // The wrapper only ever shortens this cadence (it publishes at + // `min(heartbeat, idleTimeout, detachedIdle)`), so the constant is the + // worst case and ten of them must still fit inside the staleness window. + expect(RUNTIME_HELPER_STATUS_STALE_MS).toBeGreaterThanOrEqual( + heartbeatMs * 10, + ); + }); +}); diff --git a/test/codex-bin-wrapper.test.ts b/test/codex-bin-wrapper.test.ts index 4c5b56e2..0b0b2aed 100644 --- a/test/codex-bin-wrapper.test.ts +++ b/test/codex-bin-wrapper.test.ts @@ -23,6 +23,7 @@ import { resolve, } from "node:path"; import process from "node:process"; +import { withDeadPid, withDeadPids } from "./helpers/owned-pids.js"; import { fileURLToPath, pathToFileURL } from "node:url"; import { afterEach, describe, expect, it } from "vitest"; import { @@ -332,9 +333,33 @@ function createRuntimeRotationProxyFixtureModule(fixtureRoot: string): string { " return value.length > 0 ? value : null;", "}", "", + // Opt-in: a request counter that climbs on its own for the first N ms + // and then stops, standing in for a detached consumer that keeps using + // its proxy and later goes away. Static counters cannot express + // "traffic is still arriving", which is exactly what the detached + // reaper reads. + // Garbage readings have to be expressible: a proxy that answers `-1`, + // `NaN`, or `Infinity` must degrade to "nothing attached", never to + // "someone is attached forever". + "function readProxyOpenConnections() {", + " const raw = (process.env.CODEX_MULTI_AUTH_TEST_PROXY_OPEN_CONNECTIONS ?? '').trim().toLowerCase();", + " if (raw === '') return 0;", + " if (raw === 'nan') return Number.NaN;", + " if (raw === 'infinity') return Number.POSITIVE_INFINITY;", + " const parsed = Number.parseInt(raw, 10);", + " return Number.isNaN(parsed) ? 0 : parsed;", + "}", + "", + "const proxyStartedAt = Date.now();", + "function rampedRequestCount() {", + " const rampMs = readOptionalNumberEnv('CODEX_MULTI_AUTH_TEST_PROXY_REQUEST_RAMP_MS');", + " if (rampMs === null) return null;", + " return Math.floor(Math.min(Date.now() - proxyStartedAt, rampMs) / 100);", + "}", + "", "function buildStatus() {", " return {", - " totalRequests: readOptionalNumberEnv('CODEX_MULTI_AUTH_TEST_PROXY_REQUESTS') ?? 0,", + " totalRequests: rampedRequestCount() ?? readOptionalNumberEnv('CODEX_MULTI_AUTH_TEST_PROXY_REQUESTS') ?? 0,", " upstreamRequests: 0,", " retries: 0,", " rotations: readOptionalNumberEnv('CODEX_MULTI_AUTH_TEST_PROXY_ROTATIONS') ?? 0,", @@ -397,6 +422,11 @@ function createRuntimeRotationProxyFixtureModule(fixtureRoot: string): string { " await new Promise(() => {});", " }", " },", + // Opt-in: report open client connections, which the real proxy reads off + // its live socket set. The detached reap treats a connected consumer as + // proof the handoff was real, so a test needs to be able to say "someone + // is attached" without standing up a real client. + " getOpenConnectionCount: () => readProxyOpenConnections(),", " getStatus: () => buildStatus(),", " };", "}", @@ -525,10 +555,19 @@ function createPathDiscoveredNativeCodexFixture(rootDir: string): { }; } +// The wrapper is published, so its fault injectors stay inert unless this +// switch is set alongside the counter (#668). Every injection helper below +// carries it, and `runWrapper` never sets it on its own — which is what lets +// the "production ignores the counter" test simply omit it. +const FAULT_INJECTION_ON = { + CODEX_MULTI_AUTH_TEST_FAULT_INJECTION: "1", +} as const; + function injectShadowCleanupBusyFailures( failuresBeforeSuccess = 2, ): NodeJS.ProcessEnv { return { + ...FAULT_INJECTION_ON, CODEX_MULTI_AUTH_TEST_SHADOW_CLEANUP_BUSY_FAILURES: String(failuresBeforeSuccess), }; } @@ -537,6 +576,7 @@ function injectShadowPreflightReadBusyFailures( failuresBeforeSuccess = 2, ): NodeJS.ProcessEnv { return { + ...FAULT_INJECTION_ON, CODEX_MULTI_AUTH_TEST_SHADOW_PREFLIGHT_READ_BUSY_FAILURES: String( failuresBeforeSuccess, ), @@ -547,6 +587,7 @@ function injectShadowSyncMetadataBusyFailures( failuresBeforeSuccess = 10, ): NodeJS.ProcessEnv { return { + ...FAULT_INJECTION_ON, CODEX_MULTI_AUTH_TEST_SHADOW_SYNC_METADATA_BUSY_FAILURES: String( failuresBeforeSuccess, ), @@ -555,6 +596,7 @@ function injectShadowSyncMetadataBusyFailures( function injectShadowLockRecreatedStaleCount(count = 2): NodeJS.ProcessEnv { return { + ...FAULT_INJECTION_ON, CODEX_MULTI_AUTH_TEST_SHADOW_LOCK_RECREATE_STALE_COUNT: String(count), }; } @@ -563,6 +605,7 @@ function injectShadowLockOwnerWriteFailures( failuresBeforeSuccess = 1, ): NodeJS.ProcessEnv { return { + ...FAULT_INJECTION_ON, CODEX_MULTI_AUTH_TEST_SHADOW_LOCK_OWNER_WRITE_FAILURES: String( failuresBeforeSuccess, ), @@ -689,6 +732,19 @@ function expectWrapperReturned( ).toBeUndefined(); } +// Mirrors the wrapper's own owner-identity capture: kernel start time via +// `ps -o lstart=` under the C locale, parsed to epoch ms. +function readOwnProcessStartTimeMs(): number | null { + const result = spawnSync("ps", ["-o", "lstart=", "-p", String(process.pid)], { + encoding: "utf8", + env: { ...process.env, LC_ALL: "C" }, + }); + const out = (result.stdout ?? "").trim(); + if (!out) return null; + const parsed = Date.parse(out); + return Number.isFinite(parsed) ? parsed : null; +} + function isProcessAlive(pid: number): boolean { try { process.kill(pid, 0); @@ -2776,6 +2832,7 @@ describe("codex bin wrapper", () => { CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "1000", CODEX_MULTI_AUTH_TEST_PROXY_MARKER: markerPath, CODEX_MULTI_AUTH_TEST_FORCE_APP_SERVER_SHIM_COPY: "1", + ...FAULT_INJECTION_ON, CODEX_MULTI_AUTH_TEST_APP_SERVER_SHIM_FILE_CLEANUP_BUSY_FAILURES: "2", CODEX_MULTI_AUTH_TEST_APP_SERVER_SHIM_COPY_BUSY_FAILURES: "2", CODEX_MULTI_AUTH_TEST_PROXY_LAST_ACCOUNT_INDEX: "1", @@ -2832,9 +2889,13 @@ describe("codex bin wrapper", () => { expect(readFileSync(markerPath, "utf8")).toBe( "start:http://127.0.0.1:4567\nclose\n", ); - const helperStatus = JSON.parse( - readFileSync(join(multiAuthDir, "runtime-rotation-app-helper.json"), "utf8"), - ) as { + // Status is published per helper PID; exactly one helper ran here. + const helperStatusFiles = readdirSync(multiAuthDir).filter((name) => + /^runtime-rotation-app-helper\.\d+\.json$/.test(name), + ); + expect(helperStatusFiles).toHaveLength(1); + const helperStatusPath = join(multiAuthDir, helperStatusFiles[0] ?? ""); + const helperStatus = JSON.parse(readFileSync(helperStatusPath, "utf8")) as { state: string; totalRequests: number; lastAccountIndex: number | null; @@ -2850,10 +2911,7 @@ describe("codex bin wrapper", () => { expect(helperStatus.lastAccountId).toBe("acc_second"); expect(helperStatus.lastAccountUpdatedAt).toBe(12345); if (process.platform !== "win32") { - expect( - statSync(join(multiAuthDir, "runtime-rotation-app-helper.json")).mode & - 0o777, - ).toBe(0o600); + expect(statSync(helperStatusPath).mode & 0o777).toBe(0o600); } if (shadowHomeMatch?.[1]) { expect(existsSync(shadowHomeMatch[1])).toBe(false); @@ -2981,160 +3039,1049 @@ describe("codex bin wrapper", () => { } }, 15_000); - it("sweeps stale app-server shim directories when a helper starts", async () => { + it("sweeps stale app-server shim directories when a helper starts", async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const fakeBin = createCustomFakeCodexBin(fixtureRoot, [ + "#!/usr/bin/env node", + 'const fs = require("node:fs");', + 'console.log(`STALE_SHIM_EXISTS:${fs.existsSync(process.env.CODEX_MULTI_AUTH_TEST_STALE_SHIM_DIR ?? "")}`);', + "process.exit(0);", + ]); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + const markerPath = join(fixtureRoot, "proxy-marker.txt"); + const staleShimDir = join( + multiAuthDir, + "app-server-shims", + "helper-2147483647", + ); + mkdirSync(originalHome, { recursive: true }); + mkdirSync(staleShimDir, { recursive: true }); + writeFileSync( + join(originalHome, "config.toml"), + 'model_provider = "openai"\n', + "utf8", + ); + writeFileSync( + join(staleShimDir, process.platform === "win32" ? "codex.exe" : "codex"), + "stale\n", + "utf8", + ); + + const result = runWrapper(fixtureRoot, ["app", "."], { + CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "200", + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: markerPath, + CODEX_MULTI_AUTH_TEST_STALE_SHIM_DIR: staleShimDir, + OPENAI_API_KEY: undefined, + }); + + expect(result.status).toBe(0); + expect(combinedOutput(result)).toContain("STALE_SHIM_EXISTS:false"); + expect(existsSync(staleShimDir)).toBe(false); + await waitForFileText( + markerPath, + "start:http://127.0.0.1:4567\nclose\n", + ); + }); + + // Skipped on Windows because the fixture cannot construct the state it is + // about: the owner start time comes from `ps`, which does not exist there, + // so the env var is empty, the identity branch never engages, and the test + // would silently exercise bare liveness under a name claiming otherwise. + // The Windows bare-liveness path has its own coverage below. + it.skipIf(process.platform === "win32")( + "keeps app helpers alive when owner liveness probes return EPERM", + async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + const markerPath = join(fixtureRoot, "proxy-marker.txt"); + const preloadPath = join(fixtureRoot, "owner-eperm-preload.mjs"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + writeFileSync( + preloadPath, + [ + "const originalKill = process.kill.bind(process);", + "process.kill = (pid, signal) => {", + " if (signal === 0 && String(pid) === process.env.CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID) {", + ' const error = new Error("operation not permitted");', + ' error.code = "EPERM";', + " throw error;", + " }", + " return originalKill(pid, signal);", + "};", + ].join("\n"), + "utf8", + ); + + const helper = spawn( + process.execPath, + [join(fixtureRoot, "scripts", "codex.js"), "--codex-multi-auth-runtime-app-helper"], + { + env: buildWrapperEnv({ + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "250", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + // Production launchers always pass the owner's start time, so + // EPERM tolerance must hold on the identity branch, not just the + // bare-liveness fallback — and a *matching* identity is what keeps + // a live owner's helper alive (the false-positive direction). + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS: String( + readOwnProcessStartTimeMs() ?? "", + ), + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: markerPath, + NODE_OPTIONS: `--import=${pathToFileURL(preloadPath).href}`, + }), + stdio: ["ignore", "pipe", "pipe"], + }, + ); + let stdout = ""; + let stderr = ""; + const closed = new Promise((resolve) => { + helper.once("close", () => resolve()); + }); + helper.stdout?.setEncoding("utf8"); + helper.stderr?.setEncoding("utf8"); + helper.stdout?.on("data", (chunk: string) => { + stdout += chunk; + }); + helper.stderr?.on("data", (chunk: string) => { + stderr += chunk; + }); + + try { + const ready = await new Promise<{ statusPath: string }>((resolve, reject) => { + const timeout = setTimeout(() => { + reject(new Error(`helper did not become ready\n${stdout}\n${stderr}`)); + }, 5_000); + helper.stdout?.on("data", () => { + const newlineIndex = stdout.indexOf("\n"); + if (newlineIndex < 0) return; + try { + const message = JSON.parse(stdout.slice(0, newlineIndex)) as { + type?: string; + statusPath?: string; + }; + if (message.type === "ready" && message.statusPath) { + clearTimeout(timeout); + resolve({ statusPath: message.statusPath }); + } + } catch (error) { + clearTimeout(timeout); + reject(error); + } + }); + helper.once("close", () => { + clearTimeout(timeout); + reject(new Error(`helper exited before ready\n${stdout}\n${stderr}`)); + }); + }); + + await sleep(750); + + expect(helper.pid).toBeTruthy(); + expect(isProcessAlive(helper.pid ?? -1)).toBe(true); + const status = JSON.parse(readFileSync(ready.statusPath, "utf8")) as { + state: string; + }; + expect(status.state).toBe("running"); + expect(readFileSync(markerPath, "utf8")).toBe("start:http://127.0.0.1:4567\n"); + } finally { + if (helper.pid && isProcessAlive(helper.pid)) { + helper.kill("SIGTERM"); + } + await Promise.race([closed, sleep(2_000)]); + if (helper.pid && isProcessAlive(helper.pid)) { + helper.kill("SIGKILL"); + await Promise.race([closed, sleep(2_000)]); + } + } + }, + ); + + // Spawns a helper directly (the EPERM harness above) with the given env and + // waits for its ready line; the caller owns assertions and shutdown. + async function spawnDirectAppHelper( + fixtureRoot: string, + env: Record, + ): Promise<{ + helper: ReturnType; + ready: { statusPath: string; pid: number }; + closed: Promise; + output: () => string; + }> { + const helper = spawn( + process.execPath, + [join(fixtureRoot, "scripts", "codex.js"), "--codex-multi-auth-runtime-app-helper"], + { + env: buildWrapperEnv(env), + stdio: ["ignore", "pipe", "pipe"], + }, + ); + let stdout = ""; + let stderr = ""; + const closed = new Promise((resolve) => { + helper.once("close", () => resolve()); + }); + helper.stdout?.setEncoding("utf8"); + helper.stderr?.setEncoding("utf8"); + helper.stdout?.on("data", (chunk: string) => { + stdout += chunk; + }); + helper.stderr?.on("data", (chunk: string) => { + stderr += chunk; + }); + // A rejection here throws before the caller reaches its try/finally, so + // nothing would ever call `stopDirectAppHelper` — the harness for a leak + // fix would leak a helper per failure, each holding a 1s status tick and, + // on the long-idle fixtures, a ref'd handle for a minute. Kill on the way + // out instead. (`close`-before-ready is already terminal.) + const rejectAndReap = ( + reject: (error: Error) => void, + error: Error, + ): void => { + if (helper.pid && isProcessAlive(helper.pid)) { + try { + helper.kill("SIGKILL"); + } catch { + // Best-effort: the helper may have exited between the check and here. + } + } + reject(error); + }; + const ready = await new Promise<{ statusPath: string; pid: number }>( + (resolve, reject) => { + const timeout = setTimeout(() => { + rejectAndReap( + reject, + new Error(`helper did not become ready\n${stdout}\n${stderr}`), + ); + }, 5_000); + helper.stdout?.on("data", () => { + const newlineIndex = stdout.indexOf("\n"); + if (newlineIndex < 0) return; + try { + const message = JSON.parse(stdout.slice(0, newlineIndex)) as { + type?: string; + statusPath?: string; + pid?: number; + }; + if (message.type === "ready" && message.statusPath && message.pid) { + clearTimeout(timeout); + resolve({ statusPath: message.statusPath, pid: message.pid }); + } + } catch (error) { + clearTimeout(timeout); + rejectAndReap( + reject, + error instanceof Error ? error : new Error(String(error)), + ); + } + }); + helper.once("close", () => { + clearTimeout(timeout); + reject(new Error(`helper exited before ready\n${stdout}\n${stderr}`)); + }); + }, + ); + return { helper, ready, closed, output: () => `${stdout}\n${stderr}` }; + } + + async function stopDirectAppHelper( + helper: ReturnType, + closed: Promise, + ): Promise { + if (helper.pid && isProcessAlive(helper.pid)) { + helper.kill("SIGTERM"); + } + await Promise.race([closed, sleep(2_000)]); + if (helper.pid && isProcessAlive(helper.pid)) { + helper.kill("SIGKILL"); + await Promise.race([closed, sleep(2_000)]); + } + } + + // The idle reaper's owner check is PID *plus* the owner's process start + // time. A recycled PID — a live process holding the dead launcher's integer + // — must not push the idle deadline forward: one false "alive" per window + // is a ratchet the helper never recovers from, which is how helpers were + // observed running 33 hours past a 12-hour timeout. Simulated here by + // pointing the helper at a genuinely live process (this test) with a start + // time that cannot match. Fails against the bare kill(pid, 0) check. + // POSIX-only: on Windows there is no `ps`, identity is unknowable, and the + // designed degradation is bare liveness — the companion test below. + it.skipIf(process.platform === "win32")("idles out when the owner PID is alive but its identity does not match", async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + const markerPath = join(fixtureRoot, "proxy-marker.txt"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + const { helper, ready, closed } = await spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "250", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS: "12345", + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: markerPath, + }); + try { + await Promise.race([closed, sleep(5_000)]); + expect(isProcessAlive(ready.pid)).toBe(false); + const status = JSON.parse(readFileSync(ready.statusPath, "utf8")) as { + state: string; + }; + expect(status.state).toBe("idle-timeout"); + } finally { + await stopDirectAppHelper(helper, closed); + } + }); + + // The designed Windows degradation: with no `ps`, owner identity is + // unknowable, and an unknowable identity must never kill a helper whose + // owner PID is genuinely alive — bare liveness keeps it running. + it.runIf(process.platform === "win32")( + "keeps the helper alive when owner identity is unavailable", + async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + const { helper, ready, closed } = await spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "250", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + // A start time that cannot match: with no way to read the real one, + // the check must degrade to bare liveness, not declare death. + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS: "12345", + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join(fixtureRoot, "marker.txt"), + }); + try { + await sleep(1_000); + expect(isProcessAlive(ready.pid)).toBe(true); + } finally { + await stopDirectAppHelper(helper, closed); + } + }, + ); + + // The detach grace hands a helper off to nobody whenever a launcher merely + // exits quickly — every short forwarded command strands one — and before + // this window those helpers held the full idle timeout (12h by default) + // with a dead owner, no traffic, and nothing connected. A stranded helper + // is garbage the moment the detached window elapses. Owner death is + // simulated the same way as the ratchet test: a live PID whose identity + // cannot match, which the liveness check correctly reads as dead. + it.skipIf(process.platform === "win32")( + "reaps a stranded helper on the detached window instead of the full idle timeout", + async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + const markerPath = join(fixtureRoot, "proxy-marker.txt"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + const { helper, ready, closed } = await spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + // Idle can never fire inside this test; only the detached window can, + // which is the whole point — before it existed, this helper lived on. + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "400", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS: "12345", + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: markerPath, + }); + try { + await Promise.race([closed, sleep(5_000)]); + expect(isProcessAlive(ready.pid)).toBe(false); + const status = JSON.parse(readFileSync(ready.statusPath, "utf8")) as { + state: string; + idleExpiresAt: number; + updatedAt: number; + }; + expect(status.state).toBe("owner-gone"); + // The reported deadline is the one actually enforced: `rotation + // status` must not advertise the 60s idle window to a helper the + // detached window is about to reap. + expect(status.idleExpiresAt - status.updatedAt).toBeLessThan(60_000); + } finally { + await stopDirectAppHelper(helper, closed); + } + }, + ); + + // The companion that runs everywhere, Windows included. The tests above + // simulate owner death with an unmatchable start time, which only POSIX can + // evaluate — with no `ps`, identity is unknowable and the check degrades to + // bare liveness. A *genuinely* dead owner PID is readable on every + // platform through that same bare check, so the reap itself is covered on + // win32 even though the identity flavor of it cannot be. + it("reaps a stranded helper whose owner PID is genuinely dead", async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + // A PID this test owned and then killed, so "dead" is a fact rather than a + // sentinel integer that different platforms classify differently. + // `withDeadPid` is the shared version of exactly this — it waits on the + // child's `exit` event instead of polling liveness, and re-checks the PID + // was not recycled before handing it over — so the hand-rolled copy that + // used to live here is gone. + await withDeadPid(async (deadOwnerPid) => { + const { helper, ready, closed } = await spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "400", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(deadOwnerPid), + // Left unset on purpose: this is the degraded bare-liveness path. + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS: undefined, + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join(fixtureRoot, "marker.txt"), + }); + try { + await Promise.race([closed, sleep(5_000)]); + expect(isProcessAlive(ready.pid)).toBe(false); + const status = JSON.parse(readFileSync(ready.statusPath, "utf8")) as { + state: string; + }; + expect(status.state).toBe("owner-gone"); + } finally { + await stopDirectAppHelper(helper, closed); + } + }); + }); + + // The detached window reaps strays, not handoffs. A consumer holding a + // connection is the evidence that the detach was real — `codex app` hands + // the desktop app a proxy and exits — so an attached helper keeps the full + // idle timeout no matter how long its launcher has been gone. + it.skipIf(process.platform === "win32")( + "keeps a stranded helper alive while a client connection is open", + async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + const { helper, ready, closed } = await spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "200", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS: "12345", + CODEX_MULTI_AUTH_TEST_PROXY_OPEN_CONNECTIONS: "1", + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join(fixtureRoot, "marker.txt"), + }); + try { + // Many detached windows' worth of ticks with a socket held open. + await sleep(1_500); + expect(isProcessAlive(ready.pid)).toBe(true); + } finally { + await stopDirectAppHelper(helper, closed); + } + }, + ); + + // The detached window reaps helpers that were *stranded*, never helpers that + // were handed off, and having served a request is the durable proof of a + // handoff. An open socket is not: the proxy leaves `keepAliveTimeout` at + // Node's 5s default, so a `codex app` session that is merely idle between + // turns holds no socket within seconds of its last turn. Reaping on the + // socket check alone would therefore kill the live proxy under a desktop app + // whose user simply stopped typing for the length of the detached window, + // and the next message would get ECONNREFUSED against a dead localhost port + // with nothing left to restart it. + // + // Every helper in the #663 report had `totalRequests: 0` — the leak is + // entirely a never-served phenomenon — so the narrower gate closes the leak + // without putting live sessions at risk. A served-then-abandoned helper falls + // back to the idle timeout and the lifetime ceiling, exactly as it did before + // the detached window existed. + it.skipIf(process.platform === "win32")( + "keeps a helper that has served traffic alive after its traffic stops", + async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + const { helper, ready, closed } = await spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "400", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS: "12345", + // Traffic climbs for 300ms and then freezes: one served request is + // all it takes, and the counter is frozen for many detached windows + // afterwards with no socket held. + CODEX_MULTI_AUTH_TEST_PROXY_REQUEST_RAMP_MS: "300", + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join(fixtureRoot, "marker.txt"), + }); + try { + // Several detached windows past the end of the ramp. + await sleep(2_500); + expect(isProcessAlive(ready.pid)).toBe(true); + const status = JSON.parse(readFileSync(ready.statusPath, "utf8")) as { + state: string; + }; + expect(status.state).toBe("running"); + } finally { + await stopDirectAppHelper(helper, closed); + } + }, + ); + + // The other half of the same rule: a helper whose owner is gone and which + // never served anything is a stray, and the detached window still takes it. + it.skipIf(process.platform === "win32")( + "still reaps a stranded helper that never served a request", + async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + const { helper, ready, closed } = await spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "400", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS: "12345", + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join(fixtureRoot, "marker.txt"), + }); + try { + await Promise.race([closed, sleep(5_000)]); + expect(isProcessAlive(ready.pid)).toBe(false); + const status = JSON.parse(readFileSync(ready.statusPath, "utf8")) as { + state: string; + }; + expect(status.state).toBe("owner-gone"); + } finally { + await stopDirectAppHelper(helper, closed); + } + }, + ); + + // "No owner PID was recorded" and "the owner is confirmed dead" are different + // facts. Collapsing them into one falsy verdict started the detached clock on + // the first tick for anyone invoking the helper directly — the documented + // reproduction in #663 — or running one spawned by a pre-upgrade launcher + // that sets no owner PID, and reaped it silently. + it.skipIf(process.platform === "win32")( + "does not start the detached clock for a helper launched without an owner PID", + async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + const { helper, ready, closed } = await spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "400", + // Deliberately no CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID. + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join(fixtureRoot, "marker.txt"), + }); + try { + // Many detached windows with no owner and no traffic: the idle + // timeout is the only clock that may apply, and it is 60s away. + await sleep(2_500); + expect(isProcessAlive(ready.pid)).toBe(true); + const status = JSON.parse(readFileSync(ready.statusPath, "utf8")) as { + state: string; + }; + expect(status.state).toBe("running"); + } finally { + await stopDirectAppHelper(helper, closed); + } + }, + ); + + // Only a positive socket count is evidence of a consumer. A proxy that + // answers with garbage must not be able to pin a stranded helper alive + // forever — that is the leak wearing a different hat. + for (const [label, reading] of [ + ["a negative count", "-1"], + ["NaN", "nan"], + ["Infinity", "infinity"], + ] as const) { + it.skipIf(process.platform === "win32")( + `treats ${label} from the proxy as nothing attached and still reaps`, + async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + const { helper, ready, closed } = await spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "400", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS: "12345", + CODEX_MULTI_AUTH_TEST_PROXY_OPEN_CONNECTIONS: reading, + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join(fixtureRoot, "marker.txt"), + }); + try { + await Promise.race([closed, sleep(5_000)]); + expect(isProcessAlive(ready.pid)).toBe(false); + const status = JSON.parse(readFileSync(ready.statusPath, "utf8")) as { + state: string; + }; + expect(status.state).toBe("owner-gone"); + } finally { + await stopDirectAppHelper(helper, closed); + } + }, + ); + } + + // The escape hatch is a real escape hatch: 0 restores the pre-fix behavior + // for anyone who was depending on a stranded helper outliving its launcher + // without holding a connection. + it.skipIf(process.platform === "win32")( + "keeps a stranded helper on the full idle timeout when the detached window is disabled", + async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + const { helper, ready, closed } = await spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "0", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_START_TIME_MS: "12345", + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join(fixtureRoot, "marker.txt"), + }); + try { + await sleep(1_500); + expect(isProcessAlive(ready.pid)).toBe(true); + } finally { + await stopDirectAppHelper(helper, closed); + } + }, + ); + + // The absolute lifetime ceiling is unconditional on activity: it exists for + // exactly the case where activity accounting is wrong, so a genuinely live + // owner must not extend a helper past it. + it("stops at the max-lifetime ceiling even while its owner is alive", async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + const markerPath = join(fixtureRoot, "proxy-marker.txt"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + const { helper, ready, closed } = await spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + // Idle can never fire inside this test; only the ceiling can. + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_MAX_LIFETIME_MS: "400", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: markerPath, + }); + try { + await Promise.race([closed, sleep(5_000)]); + expect(isProcessAlive(ready.pid)).toBe(false); + const status = JSON.parse(readFileSync(ready.statusPath, "utf8")) as { + state: string; + }; + expect(status.state).toBe("max-lifetime"); + } finally { + await stopDirectAppHelper(helper, closed); + } + }); + + // N helpers, N status files: each helper publishes + // `runtime-rotation-app-helper..json` and never the shared legacy + // path, so concurrent helpers stop last-writer-winning one file and every + // reader can see every helper. + it("publishes one status file per helper PID instead of one shared file", async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + const commonEnv = { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + }; + const first = await spawnDirectAppHelper(fixtureRoot, { + ...commonEnv, + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join(fixtureRoot, "marker-1.txt"), + }); + try { + const second = await spawnDirectAppHelper(fixtureRoot, { + ...commonEnv, + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join(fixtureRoot, "marker-2.txt"), + }); + try { + expect(first.ready.statusPath).not.toBe(second.ready.statusPath); + expect(first.ready.statusPath).toContain(`.${first.ready.pid}.`); + expect(second.ready.statusPath).toContain(`.${second.ready.pid}.`); + const firstStatus = JSON.parse( + readFileSync(first.ready.statusPath, "utf8"), + ) as { pid: number; state: string }; + const secondStatus = JSON.parse( + readFileSync(second.ready.statusPath, "utf8"), + ) as { pid: number; state: string }; + expect(firstStatus.pid).toBe(first.ready.pid); + expect(secondStatus.pid).toBe(second.ready.pid); + expect(firstStatus.state).toBe("running"); + expect(secondStatus.state).toBe("running"); + expect( + existsSync(join(multiAuthDir, "runtime-rotation-app-helper.json")), + ).toBe(false); + } finally { + await stopDirectAppHelper(second.helper, second.closed); + } + } finally { + await stopDirectAppHelper(first.helper, first.closed); + } + }); + + // One of the four defects was helpers rewriting status at 1 Hz; publishing + // is now change-token + heartbeat. A quiet helper's status file must not + // churn between ticks, or N helpers reintroduce the write storm silently. + it("does not rewrite an unchanged status file on every tick", async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + + const { helper, ready, closed } = await spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + // Long idle: the 1s tick keeps running, but with no traffic and a + // 60s heartbeat nothing about the payload changes between ticks. + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join(fixtureRoot, "marker.txt"), + }); + try { + const firstMtime = statSync(ready.statusPath).mtimeMs; + // Several ticks pass (tick interval is 1s at this idle timeout). + await sleep(2_600); + expect(statSync(ready.statusPath).mtimeMs).toBe(firstMtime); + } finally { + await stopDirectAppHelper(helper, closed); + } + }); + + // Windows can hold transient locks on files another process just closed; + // metadata deletions retry instead of silently leaving the stale file the + // sweep exists to remove. + it("retries transient lock failures while sweeping helper metadata", async () => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const fakeBin = createCustomFakeCodexBin(fixtureRoot, [ + "#!/usr/bin/env node", + "process.exit(0);", + ]); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + mkdirSync(multiAuthDir, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + const staleStatusPaths = [ + join(multiAuthDir, "runtime-rotation-app-helper.99999996.json"), + join(multiAuthDir, "runtime-rotation-app-helper.99999997.json"), + ]; + for (const [index, path] of staleStatusPaths.entries()) { + writeFileSync( + path, + `{"pid":9999999${6 + index},"state":"running"}\n`, + "utf8", + ); + } + + const result = runWrapper(fixtureRoot, ["app", "."], { + CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "250", + // The failure counter is process-wide: two simulated EBUSY throws land + // on the deletions in whatever order the sweep visits the two files. + // `withSynchronousFileOperationRetry` allows four attempts per call, so + // even the worst split (one file eating both failures) succeeds on that + // file's third attempt — the outcome is order-independent as long as + // the retry budget stays at three attempts or more. If that budget ever + // shrinks below three, this test fails and the sweep would silently + // leave stale metadata behind on transient Windows locks. + ...FAULT_INJECTION_ON, + CODEX_MULTI_AUTH_TEST_HELPER_METADATA_CLEANUP_BUSY_FAILURES: "2", + OPENAI_API_KEY: undefined, + }); + expect(result.status).toBe(0); + for (const path of staleStatusPaths) { + expect(existsSync(path)).toBe(false); + } + }); + + // `package.json` publishes `scripts/codex.js`, so this injector runs in every + // user's install. Two things keep it inert there: the counter does nothing + // without an explicit opt-in switch, and the value is parsed strictly — + // `Number.parseInt` reads "2abc" as 2 and "1e3" as 1, which is how a value + // that was never meant to be a count arms a fault injector (#668). Either + // leak would silently defeat the first N metadata deletions of every sweep, + // which is the exact accumulation the sweep exists to prevent. + it.each([ + ["without the opt-in switch", { CODEX_MULTI_AUTH_TEST_FAULT_INJECTION: undefined }], + ["with a non-numeric counter", { ...FAULT_INJECTION_ON }], + ] as const)( + "ignores the metadata-cleanup fault injector %s", + async (label, gateEnv) => { + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const fakeBin = createCustomFakeCodexBin(fixtureRoot, [ + "#!/usr/bin/env node", + "process.exit(0);", + ]); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + mkdirSync(multiAuthDir, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + const staleStatusPath = join( + multiAuthDir, + "runtime-rotation-app-helper.99999996.json", + ); + writeFileSync(staleStatusPath, '{"pid":99999996,"state":"running"}\n', "utf8"); + + // A counter big enough to exhaust the retry budget several times over, + // so if it were ever honoured the sweep could not recover. + const result = runWrapper(fixtureRoot, ["app", "."], { + CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "250", + ...gateEnv, + CODEX_MULTI_AUTH_TEST_HELPER_METADATA_CLEANUP_BUSY_FAILURES: + label === "with a non-numeric counter" ? "99abc" : "99", + OPENAI_API_KEY: undefined, + }); + + expect(result.status).toBe(0); + expect(existsSync(staleStatusPath)).toBe(false); + }, + ); + + // The retry budget is finite, so a file that stays locked has to be survivable + // rather than fatal: the sweep runs on the launcher's critical path, and a + // helper launch must not fail because a stale file from some other helper + // could not be deleted. The file simply waits for the next sweep. + it("leaves a permanently locked metadata file behind without failing the launch", async () => { const fixtureRoot = createWrapperFixture(); createRuntimeRotationProxyFixtureModule(fixtureRoot); const fakeBin = createCustomFakeCodexBin(fixtureRoot, [ "#!/usr/bin/env node", - 'const fs = require("node:fs");', - 'console.log(`STALE_SHIM_EXISTS:${fs.existsSync(process.env.CODEX_MULTI_AUTH_TEST_STALE_SHIM_DIR ?? "")}`);', "process.exit(0);", ]); const originalHome = join(fixtureRoot, "codex-home"); const multiAuthDir = join(fixtureRoot, "multi-auth"); - const markerPath = join(fixtureRoot, "proxy-marker.txt"); - const staleShimDir = join( - multiAuthDir, - "app-server-shims", - "helper-2147483647", - ); mkdirSync(originalHome, { recursive: true }); - mkdirSync(staleShimDir, { recursive: true }); - writeFileSync( - join(originalHome, "config.toml"), - 'model_provider = "openai"\n', - "utf8", - ); - writeFileSync( - join(staleShimDir, process.platform === "win32" ? "codex.exe" : "codex"), - "stale\n", - "utf8", + mkdirSync(multiAuthDir, { recursive: true }); + writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); + const staleStatusPath = join( + multiAuthDir, + "runtime-rotation-app-helper.99999995.json", ); + writeFileSync(staleStatusPath, '{"pid":99999995,"state":"running"}\n', "utf8"); + // Far more failures than `withSynchronousFileOperationRetry`'s budget, so + // every attempt on this file throws EBUSY and the retry never succeeds. const result = runWrapper(fixtureRoot, ["app", "."], { CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, CODEX_HOME: originalHome, CODEX_MULTI_AUTH_DIR: multiAuthDir, CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", - CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "200", - CODEX_MULTI_AUTH_TEST_PROXY_MARKER: markerPath, - CODEX_MULTI_AUTH_TEST_STALE_SHIM_DIR: staleShimDir, + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "250", + ...FAULT_INJECTION_ON, + CODEX_MULTI_AUTH_TEST_HELPER_METADATA_CLEANUP_BUSY_FAILURES: "999", OPENAI_API_KEY: undefined, }); + // The launch succeeded... expect(result.status).toBe(0); - expect(combinedOutput(result)).toContain("STALE_SHIM_EXISTS:false"); - expect(existsSync(staleShimDir)).toBe(false); - await waitForFileText( - markerPath, - "start:http://127.0.0.1:4567\nclose\n", - ); + // ...and the file it could not delete is still there for the next sweep, + // rather than the error having escaped into the launcher. + expect(existsSync(staleStatusPath)).toBe(true); }); - it("keeps app helpers alive when owner liveness probes return EPERM", async () => { + // Owner files have no post-mortem value and go with the helper; stale + // per-PID metadata from killed helpers — and a legacy shared status file + // whose recorded PID is dead — is swept when the next launcher starts a + // helper, which is what keeps 579-files-vs-183-helpers from recurring. + it("removes its owner file on exit and sweeps dead helpers' metadata on the next launch", async () => { const fixtureRoot = createWrapperFixture(); createRuntimeRotationProxyFixtureModule(fixtureRoot); + const fakeBin = createCustomFakeCodexBin(fixtureRoot, [ + "#!/usr/bin/env node", + "process.exit(0);", + ]); const originalHome = join(fixtureRoot, "codex-home"); const multiAuthDir = join(fixtureRoot, "multi-auth"); - const markerPath = join(fixtureRoot, "proxy-marker.txt"); - const preloadPath = join(fixtureRoot, "owner-eperm-preload.mjs"); mkdirSync(originalHome, { recursive: true }); + mkdirSync(multiAuthDir, { recursive: true }); writeFileSync(join(originalHome, "config.toml"), 'model_provider = "openai"\n', "utf8"); - writeFileSync( - preloadPath, - [ - "const originalKill = process.kill.bind(process);", - "process.kill = (pid, signal) => {", - " if (signal === 0 && String(pid) === process.env.CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID) {", - ' const error = new Error("operation not permitted");', - ' error.code = "EPERM";', - " throw error;", - " }", - " return originalKill(pid, signal);", - "};", - ].join("\n"), - "utf8", + // Metadata for a helper PID that cannot be alive, plus a legacy shared + // status file recording the same dead PID: all three must be swept. + const staleStatusPath = join( + multiAuthDir, + "runtime-rotation-app-helper.99999999.json", ); - - const helper = spawn( - process.execPath, - [join(fixtureRoot, "scripts", "codex.js"), "--codex-multi-auth-runtime-app-helper"], - { - env: buildWrapperEnv({ - CODEX_HOME: originalHome, - CODEX_MULTI_AUTH_DIR: multiAuthDir, - CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, - CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", - CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "250", - CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), - CODEX_MULTI_AUTH_TEST_PROXY_MARKER: markerPath, - NODE_OPTIONS: `--import=${pathToFileURL(preloadPath).href}`, - }), - stdio: ["ignore", "pipe", "pipe"], - }, + const staleOwnerPath = join( + multiAuthDir, + "runtime-rotation-app-helper-owner.99999999.json", ); - let stdout = ""; - let stderr = ""; - const closed = new Promise((resolve) => { - helper.once("close", () => resolve()); - }); - helper.stdout?.setEncoding("utf8"); - helper.stderr?.setEncoding("utf8"); - helper.stdout?.on("data", (chunk: string) => { - stdout += chunk; - }); - helper.stderr?.on("data", (chunk: string) => { - stderr += chunk; - }); - - try { - const ready = await new Promise<{ statusPath: string }>((resolve, reject) => { - const timeout = setTimeout(() => { - reject(new Error(`helper did not become ready\n${stdout}\n${stderr}`)); - }, 5_000); - helper.stdout?.on("data", () => { - const newlineIndex = stdout.indexOf("\n"); - if (newlineIndex < 0) return; - try { - const message = JSON.parse(stdout.slice(0, newlineIndex)) as { - type?: string; - statusPath?: string; - }; - if (message.type === "ready" && message.statusPath) { - clearTimeout(timeout); - resolve({ statusPath: message.statusPath }); - } - } catch (error) { - clearTimeout(timeout); - reject(error); - } - }); - helper.once("close", () => { - clearTimeout(timeout); - reject(new Error(`helper exited before ready\n${stdout}\n${stderr}`)); - }); - }); + const legacyStatusPath = join(multiAuthDir, "runtime-rotation-app-helper.json"); + writeFileSync(staleStatusPath, '{"pid":99999999,"state":"running"}\n', "utf8"); + writeFileSync(staleOwnerPath, '{"launcherPid":1,"identityToken":"x"}\n', "utf8"); + writeFileSync(legacyStatusPath, '{"pid":99999999,"state":"running"}\n', "utf8"); - await sleep(750); + const result = runWrapper(fixtureRoot, ["app", "."], { + CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "250", + OPENAI_API_KEY: undefined, + }); + expect(result.status).toBe(0); - expect(helper.pid).toBeTruthy(); - expect(isProcessAlive(helper.pid ?? -1)).toBe(true); - const status = JSON.parse(readFileSync(ready.statusPath, "utf8")) as { - state: string; - }; - expect(status.state).toBe("running"); - expect(readFileSync(markerPath, "utf8")).toBe("start:http://127.0.0.1:4567\n"); - } finally { - if (helper.pid && isProcessAlive(helper.pid)) { - helper.kill("SIGTERM"); + // The launcher swept — after spawning its own helper, so the sweep never + // sits in front of `codex app` startup, and before the launch handshake, + // so it is complete by the time the wrapper exits. + expect(existsSync(staleStatusPath)).toBe(false); + expect(existsSync(staleOwnerPath)).toBe(false); + expect(existsSync(legacyStatusPath)).toBe(false); + + // The launcher's own helper detached (grace window), then idles out with + // its owner gone; on exit it removes its owner file and leaves only its + // terminal status stamp. + const ownerPattern = /^runtime-rotation-app-helper-owner\.(\d+)\.json$/; + const statusPattern = /^runtime-rotation-app-helper\.(\d+)\.json$/; + const deadline = Date.now() + 5_000; + let ownerFiles: string[] = []; + let statusFiles: string[] = []; + let sawHelperMetadata = false; + for (;;) { + ownerFiles = readdirSync(multiAuthDir).filter((name) => + ownerPattern.test(name), + ); + statusFiles = readdirSync(multiAuthDir).filter((name) => + statusPattern.test(name), + ); + if (ownerFiles.length > 0 || statusFiles.length > 0) { + sawHelperMetadata = true; } - await Promise.race([closed, sleep(2_000)]); - if (helper.pid && isProcessAlive(helper.pid)) { - helper.kill("SIGKILL"); - await Promise.race([closed, sleep(2_000)]); + const statuses = statusFiles.map( + (name) => + JSON.parse(readFileSync(join(multiAuthDir, name), "utf8")) as { + state: string; + }, + ); + if ( + sawHelperMetadata && + ownerFiles.length === 0 && + statuses.length > 0 && + statuses.every((status) => status.state !== "running") + ) { + break; + } + if (Date.now() >= deadline) { + throw new Error( + `helper metadata did not settle: owners=${JSON.stringify(ownerFiles)} statuses=${JSON.stringify(statusFiles)}`, + ); } + await sleep(50); } - }); + expect(ownerFiles).toHaveLength(0); + expect(statusFiles).toHaveLength(1); + const finalStatus = JSON.parse( + readFileSync(join(multiAuthDir, statusFiles[0] ?? ""), "utf8"), + ) as { state: string }; + expect(finalStatus.state).toBe("idle-timeout"); + }, 15_000); it("stops failed app helpers before unsupported-model retries", async () => { const fixtureRoot = createWrapperFixture(); @@ -3275,8 +4222,11 @@ describe("codex bin wrapper", () => { const marker = readFileSync(markerPath, "utf8"); expect(marker).toContain(`real-home-env:${originalHome}\n`); + // Status is per helper PID; the shared legacy path is no longer written. expect( - existsSync(join(originalHome, "multi-auth", "runtime-rotation-app-helper.json")), + readdirSync(join(originalHome, "multi-auth")).some((name) => + /^runtime-rotation-app-helper\.\d+\.json$/.test(name), + ), ).toBe(true); const compatibilityHomeMatch = marker.match(/^codex-home-env:(.+)$/m); expect(compatibilityHomeMatch?.[1]).toBeTruthy(); @@ -6743,4 +7693,436 @@ describe("codex bin wrapper", () => { ); } }); + + // ------------------------------------------------------------------ + // Stress: the runtime behaviour, at the scale and duration #663 described. + // The tests above pin one helper at a time over a few hundred milliseconds. + // These drive many helpers, many launch cycles, and a directory already full + // of stale metadata — the state the reporting machine was actually in. + // POSIX-only for the same reason as the rest of the lifecycle suite. + // ------------------------------------------------------------------ + + function countHelperMetadata(multiAuthDir: string): { + status: number; + owner: number; + } { + let entries: string[] = []; + try { + entries = readdirSync(multiAuthDir); + } catch (error) { + // Only "the directory does not exist yet" is a legitimate zero. Any + // other readdir failure — a permissions change, a path that is not a + // directory — would otherwise be reported as a clean sweep, and the + // bounded-metadata test below asserts upper bounds that a false zero + // satisfies perfectly. + const code = + error && typeof error === "object" && "code" in error + ? String((error as { code?: unknown }).code) + : "unknown"; + if (code !== "ENOENT") throw error; + return { status: 0, owner: 0 }; + } + return { + status: entries.filter((n) => + /^runtime-rotation-app-helper\.\d+\.json$/.test(n), + ).length, + owner: entries.filter((n) => + /^runtime-rotation-app-helper-owner\.\d+\.json$/.test(n), + ).length, + }; + } + + it.skipIf(process.platform === "win32")( + "stress: metadata stays bounded across many launch/exit cycles", + async () => { + // The reported accumulation was 10-28 helpers/hour under ordinary use, + // ending at 183 live helpers and 701 owner files. One launch proves + // nothing about that; the property is that repeating the cycle does not + // grow the directory without bound. Each launch sweeps what the previous + // one left, so the count must plateau rather than climb with the cycle + // count. + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const fakeBin = createCustomFakeCodexBin(fixtureRoot, [ + "#!/usr/bin/env node", + "process.exit(0);", + ]); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + mkdirSync(multiAuthDir, { recursive: true }); + writeFileSync( + join(originalHome, "config.toml"), + 'model_provider = "openai"\n', + "utf8", + ); + + // Before measuring accumulation, prove the fixture actually produces + // helpers. Every assertion below is an upper bound, so a launch path + // that silently never publishes metadata — a wrong env gate, a proxy + // fixture that never engages, `app .` short-circuiting on the fake bin + // — would leave every count at zero and pass the whole test while + // demonstrating nothing about the leak it exists to guard. + const probe = runWrapper(fixtureRoot, ["app", "."], { + CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + // Long enough that the helper is unambiguously still alive when the + // probe looks for it. + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "30000", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "0", + OPENAI_API_KEY: undefined, + }); + expect(probe.status).toBe(0); + const probeCounts = countHelperMetadata(multiAuthDir); + expect(probeCounts.status).toBeGreaterThan(0); + expect(probeCounts.owner).toBeGreaterThan(0); + // Tear that one down before the accumulation loop starts, so it cannot + // be mistaken for a leaked helper later. + const probeEntries = readdirSync(multiAuthDir).filter((name) => + name.startsWith("runtime-rotation-app-helper"), + ); + for (const name of probeEntries) { + const match = /\.(\d+)\.json$/.exec(name); + const pid = match?.[1] ? Number.parseInt(match[1], 10) : null; + if (pid !== null && isProcessAlive(pid)) { + try { + process.kill(pid, "SIGKILL"); + } catch { + // Already gone. + } + } + rmSync(join(multiAuthDir, name), { force: true }); + } + await sleep(200); + + const cycles = 30; + const counts: number[] = []; + let sawHelperDuringLoop = false; + for (let cycle = 0; cycle < cycles; cycle += 1) { + const result = runWrapper(fixtureRoot, ["app", "."], { + CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + // Short enough that each helper is gone well before the next + // launch sweeps for it. + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "200", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "150", + OPENAI_API_KEY: undefined, + }); + expect(result.status).toBe(0); + await sleep(120); + const sample = countHelperMetadata(multiAuthDir); + if (sample.owner > 0 || sample.status > 0) sawHelperDuringLoop = true; + counts.push(sample.owner); + } + + // Give the last cycle's helper time to exit and one more launch to sweep + // after it. + await sleep(1_000); + runWrapper(fixtureRoot, ["app", "."], { + CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "200", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "150", + OPENAI_API_KEY: undefined, + }); + await sleep(1_000); + + const final = countHelperMetadata(multiAuthDir); + const peak = Math.max(...counts); + // The loop has to have observed at least one helper at some point, + // otherwise the upper bounds below are vacuous. + expect(sawHelperDuringLoop).toBe(true); + // The pre-fix behaviour was one owner file per launch, kept forever, so + // the count climbed with the cycle count. A few concurrent files are + // expected — each helper outlives the launch that spawned it by its + // idle window, so a sample can catch the previous cycle's helper still + // running — but the number must be a function of that overlap, not of + // how many times the loop ran. A ceiling well under `cycles` is what + // separates the two; `< cycles` alone would only fail at the exact + // worst case. + expect(peak).toBeLessThan(10); + expect(final.owner).toBeLessThan(5); + expect(final.status).toBeLessThan(5); + }, + 240_000, + ); + + it.skipIf(process.platform === "win32")( + "stress: concurrent helpers keep separate status files and are all discoverable", + async () => { + // Defect 3 in #663: one shared status path, N writers at 1 Hz, last + // writer wins. Per-PID files are the fix; this asserts N concurrent + // helpers really do produce N distinct records, each naming its own PID, + // with none overwriting another. + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync( + join(originalHome, "config.toml"), + 'model_provider = "openai"\n', + "utf8", + ); + + const helperCount = 10; + const spawned = await Promise.all( + Array.from({ length: helperCount }, (_unused, index) => + spawnDirectAppHelper(fixtureRoot, { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "0", + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + CODEX_MULTI_AUTH_TEST_PROXY_LAST_ACCOUNT_ID: `acc_${index}`, + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join( + fixtureRoot, + `marker-${index}.txt`, + ), + }), + ), + ); + try { + // Several publish ticks, so any trampling has had time to happen. + await sleep(1_500); + + const pids = spawned.map((s) => s.ready.pid); + expect(new Set(pids).size).toBe(helperCount); + + for (const { ready } of spawned) { + expect(existsSync(ready.statusPath)).toBe(true); + const record = JSON.parse(readFileSync(ready.statusPath, "utf8")) as { + pid: number; + state: string; + }; + // Each file describes its own helper, not whichever wrote last. + expect(record.pid).toBe(ready.pid); + expect(record.state).toBe("running"); + } + // And every one of them is still alive: nothing reaped a sibling. + for (const pid of pids) { + expect(isProcessAlive(pid)).toBe(true); + } + + const counts = countHelperMetadata(multiAuthDir); + expect(counts.status).toBe(helperCount); + } finally { + await Promise.all( + spawned.map(({ helper, closed }) => + stopDirectAppHelper(helper, closed), + ), + ); + } + }, + 180_000, + ); + + it.skipIf(process.platform === "win32")( + "stress: the reap matrix reaps exactly the stranded helpers and no others", + async () => { + // The whole lifecycle contract in one run, with every configuration live + // at the same time so a rule that fires on the wrong one is visible as a + // divergence rather than as a single red test. + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + writeFileSync( + join(originalHome, "config.toml"), + 'model_provider = "openai"\n', + "utf8", + ); + + await withDeadPid(async (deadOwnerPid) => { + const base = { + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_REAL_CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "60000", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "400", + }; + const cases: Array<{ + label: string; + survives: boolean; + env: Record; + }> = [ + { + label: "owner alive, never served", + survives: true, + env: { + ...base, + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(process.pid), + }, + }, + { + label: "owner dead, never served", + survives: false, + env: { + ...base, + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(deadOwnerPid), + }, + }, + { + label: "owner dead, served traffic", + survives: true, + env: { + ...base, + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(deadOwnerPid), + CODEX_MULTI_AUTH_TEST_PROXY_REQUEST_RAMP_MS: "200", + }, + }, + { + label: "no owner recorded, never served", + survives: true, + env: { ...base }, + }, + { + label: "owner dead, socket held", + survives: true, + env: { + ...base, + CODEX_MULTI_AUTH_APP_ROTATION_OWNER_PID: String(deadOwnerPid), + CODEX_MULTI_AUTH_TEST_PROXY_OPEN_CONNECTIONS: "1", + }, + }, + ]; + + const running = await Promise.all( + cases.map((testCase, index) => + spawnDirectAppHelper(fixtureRoot, { + ...testCase.env, + CODEX_MULTI_AUTH_TEST_PROXY_MARKER: join( + fixtureRoot, + `matrix-${index}.txt`, + ), + }), + ), + ); + try { + // Many detached windows: anything that is going to be reaped has + // been, and anything that survives this has survived on a rule. + await sleep(4_000); + + // Zipped off `running`, not indexed with a fallback: passing `0` to + // `isProcessAlive` would probe the caller's own process group on + // POSIX and answer true, so a missing helper would read as alive — + // and four of these five cases expect exactly that. + expect(running).toHaveLength(cases.length); + const actual = running.map(({ ready }, index) => { + const testCase = cases[index]; + expect(testCase).toBeDefined(); + return { + label: testCase?.label ?? `case-${index}`, + expected: testCase?.survives ?? false, + alive: isProcessAlive(ready.pid), + statusPath: ready.statusPath, + }; + }); + // Compared as a whole so a failure names every divergence at once. + expect(actual.map((a) => `${a.label}=${a.alive}`)).toEqual( + actual.map((a) => `${a.label}=${a.expected}`), + ); + + // The one that died did so for the stated reason. Looked up by + // label and asserted unconditionally: hardcoding an index meant + // reordering `cases` would silently assert `owner-gone` against a + // helper that was supposed to survive, and a guard around it would + // let the only assertion that proves *why* it died skip itself. + const reaped = actual.find( + (a) => a.label === "owner dead, never served", + ); + expect(reaped).toBeDefined(); + expect(reaped?.alive).toBe(false); + const status = JSON.parse( + readFileSync(reaped?.statusPath ?? "", "utf8"), + ) as { state: string }; + expect(status.state).toBe("owner-gone"); + } finally { + await Promise.all( + running.map(({ helper, closed }) => + stopDirectAppHelper(helper, closed), + ), + ); + } + }); + }, + 180_000, + ); + + it.skipIf(process.platform === "win32")( + "stress: a launcher sweeps a directory already holding hundreds of stale files", + async () => { + // 701 orphaned owner files was the reported end state. The sweep has a + // probe budget and a retry ladder, both of which could in principle turn + // a big directory into a slow or incomplete launch. Assert it reclaims + // the lot and the launch still succeeds. + const fixtureRoot = createWrapperFixture(); + createRuntimeRotationProxyFixtureModule(fixtureRoot); + const fakeBin = createCustomFakeCodexBin(fixtureRoot, [ + "#!/usr/bin/env node", + "process.exit(0);", + ]); + const originalHome = join(fixtureRoot, "codex-home"); + const multiAuthDir = join(fixtureRoot, "multi-auth"); + mkdirSync(originalHome, { recursive: true }); + mkdirSync(multiAuthDir, { recursive: true }); + writeFileSync( + join(originalHome, "config.toml"), + 'model_provider = "openai"\n', + "utf8", + ); + + const staleCount = 350; + await withDeadPids(staleCount, async (deadPids) => { + for (const pid of deadPids) { + writeFileSync( + join(multiAuthDir, `runtime-rotation-app-helper.${pid}.json`), + `${JSON.stringify({ pid, state: "running", startedAt: Date.now() })}\n`, + "utf8", + ); + writeFileSync( + join(multiAuthDir, `runtime-rotation-app-helper-owner.${pid}.json`), + `${JSON.stringify({ identityToken: "x", createdAt: Date.now() })}\n`, + "utf8", + ); + } + const before = countHelperMetadata(multiAuthDir); + expect(before.status).toBe(staleCount); + expect(before.owner).toBe(staleCount); + + const startedAt = Date.now(); + const result = runWrapper(fixtureRoot, ["app", "."], { + CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, + CODEX_HOME: originalHome, + CODEX_MULTI_AUTH_DIR: multiAuthDir, + CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", + CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "200", + CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "150", + OPENAI_API_KEY: undefined, + }); + const elapsedMs = Date.now() - startedAt; + + expect(result.status).toBe(0); + await sleep(600); + const after = countHelperMetadata(multiAuthDir); + // Everything stale is gone; only this launch's own helper may remain. + expect(after.status).toBeLessThan(3); + expect(after.owner).toBeLessThan(3); + // The launch handshake has a 15s bound; a sweep that pushed past it + // would fail the launch, not just be slow. + expect(elapsedMs).toBeLessThan(60_000); + }); + }, + 240_000, + ); }); diff --git a/test/codex-manager-rotation-command.test.ts b/test/codex-manager-rotation-command.test.ts index b56e6674..c3228b6a 100644 --- a/test/codex-manager-rotation-command.test.ts +++ b/test/codex-manager-rotation-command.test.ts @@ -9,6 +9,7 @@ import type { AppBindResult, AppBindStatus } from "../lib/runtime/app-bind.js"; import type { AccountStorageV3 } from "../lib/storage.js"; import type { PluginConfig } from "../lib/types.js"; import { withFileOperationRetry } from "../scripts/install-codex-auth-utils.js"; +import { withDeadPid, withLivePid } from "./helpers/owned-pids.js"; const originalRuntimeRotationProxyEnv = process.env.CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY; @@ -446,6 +447,101 @@ describe("codex-multi-auth rotation command", () => { expect(infos.join("\n")).toContain("Codex app helper: not running"); }); + it("prefers the newest live per-PID helper status and counts the others", async () => { + const root = await createTempRoot("codex-rotation-helper-per-pid-"); + process.env.CODEX_MULTI_AUTH_DIR = root; + await mkdir(root, { recursive: true }); + const now = Date.now(); + // A live per-PID helper (this test's own PID is alive), a second live + // helper record on the legacy shared path, and a dead per-PID record + // that must count for nothing. + // + // Both the second live PID and the dead PID belong to processes this test + // owns. The second one used to be `process.ppid` — the vitest pool + // process, which the test neither controls nor keeps alive, so whether + // the count read `(+1 more running)` or `(+0 more running)` depended on + // the pool implementation and on that process surviving the run (#668). + await withLivePid(async (secondLivePid) => { + await withDeadPid(async (deadPid) => { + await writeFile( + join(root, `runtime-rotation-app-helper.${process.pid}.json`), + `${JSON.stringify({ + version: 1, + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: process.pid, + totalRequests: 7, + rotations: 2, + idleExpiresAt: now + 60_000, + updatedAt: now, + })}\n`, + "utf8", + ); + await writeFile( + join(root, "runtime-rotation-app-helper.json"), + `${JSON.stringify({ + version: 1, + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: secondLivePid, + totalRequests: 1, + rotations: 0, + updatedAt: now - 5_000, + })}\n`, + "utf8", + ); + await writeFile( + join(root, `runtime-rotation-app-helper.${deadPid}.json`), + `${JSON.stringify({ + version: 1, + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: deadPid, + updatedAt: now, + })}\n`, + "utf8", + ); + const { deps, infos } = createDeps({ storage: null }); + + await expect(runRotationCommand(["status"], deps)).resolves.toBe(0); + + const output = infos.join("\n"); + // Newest live helper wins the line; the dead PID is not counted. + expect(output).toContain( + `Codex app helper: running pid=${process.pid}`, + ); + expect(output).toContain("requests=7"); + expect(output).toContain("(+1 more running)"); + }); + }); + }); + + it("treats a max-lifetime helper record as not running even when its PID is alive", async () => { + // "max-lifetime" is a terminal state the ceiling exit publishes; a live + // kill(pid, 0) on a terminal record proves nothing — the PID may be + // recycled, which is the exact gate this fix stopped trusting. + const root = await createTempRoot("codex-rotation-helper-max-lifetime-"); + process.env.CODEX_MULTI_AUTH_DIR = root; + await mkdir(root, { recursive: true }); + await writeFile( + join(root, `runtime-rotation-app-helper.${process.pid}.json`), + `${JSON.stringify({ + version: 1, + kind: "codex-app-runtime-rotation-helper", + state: "max-lifetime", + pid: process.pid, + totalRequests: 4, + updatedAt: Date.now(), + })}\n`, + "utf8", + ); + const { deps, infos } = createDeps({ storage: null }); + + await expect(runRotationCommand(["status"], deps)).resolves.toBe(0); + + expect(infos.join("\n")).toContain("Codex app helper: not running"); + }); + it("treats an array helper status file as not running", async () => { // Pins the canonical isRecord contract (lib/utils.ts): a status file // whose top-level JSON value is an array must read as "no status", not diff --git a/test/helpers/owned-pids.ts b/test/helpers/owned-pids.ts new file mode 100644 index 00000000..c8aeca34 --- /dev/null +++ b/test/helpers/owned-pids.ts @@ -0,0 +1,241 @@ +import { spawn, type ChildProcess } from "node:child_process"; +import process from "node:process"; + +/** + * PIDs the test owns, instead of sentinels the test hopes are unused. + * + * Helper-lifecycle fixtures used to stand in for "dead" with integers above the + * platform PID ceiling (`99999999`, `2_147_483_646`) and for "a second live + * process" with `process.ppid`. Neither is a fact the test controls: + * `process.kill` may raise `EINVAL` rather than `ESRCH` for an out-of-range + * PID — which happens to classify as dead only because every liveness check in + * this tree treats every errno but `EPERM` as dead — and `process.ppid` inside + * a vitest worker is the pool process, whose identity and lifetime differ + * between the `threads` and `forks` pools and which can exit mid-run (#668). + * + * Spawning a process and killing it makes "dead" a fact; keeping one alive for + * the duration of a test makes "live" a fact. + */ + +function spawnIdleChild(): ChildProcess { + // Reads stdin forever and does nothing else. stdin is a pipe the parent + // holds open, so the child stays alive until it is signalled, without a + // timer that could fire first. + return spawn(process.execPath, ["-e", "process.stdin.resume()"], { + stdio: ["pipe", "ignore", "ignore"], + }); +} + +export interface OwnedPidOptions { + /** + * Test-only seam for how a probe child is created. + * + * The failure these helpers have to survive is a child that never spawns: + * it emits `error` and never `exit`, and because a batch is awaited + * concurrently, one of them stalls every sibling. That path cannot be + * reached by spawning a working binary, and asserting on a child the test + * spawned itself only proves what Node does — it would keep passing with + * the handling here deleted. Substituting the factory is what makes the + * helpers' own behaviour observable. + */ + spawnChild?: () => ChildProcess; +} + +async function waitForExit(child: ChildProcess): Promise { + if (child.exitCode === null && child.signalCode === null) { + await new Promise((resolve) => { + let settled = false; + const finish = (): void => { + if (settled) return; + settled = true; + resolve(); + }; + child.once("exit", finish); + // A child that never spawned emits `error`, never `exit`. Waiting on + // `exit` alone leaves this promise pending forever — and the batched + // helpers below await a whole batch concurrently, so a single failed + // spawn would stall every sibling and hang the run rather than failing + // it. Either event means "this child is not running". + child.once("error", finish); + }); + } + // `exit` fires before the stdio streams are torn down, so the parent's write + // end of the stdin pipe is still open here and would linger until GC. Every + // call site opens at least one, several open three at once, and on Windows + // these are named-pipe handles — the scarcer resource. Close it explicitly + // so the lifetime is the helper's, not the collector's. + child.stdin?.destroy(); +} + +/** + * Signal a probe child, tolerating one that has nothing to signal. + * + * A child that never spawned has no process behind it, and `kill` throws + * rather than no-opping — `EINVAL` on Windows. Left unguarded that throw + * escapes the cleanup loop *before* the batch helpers can report why the + * batch was unusable, so the caller sees `kill EINVAL` instead of "failed to + * spawn", and on a partial failure the real diagnosis is masked entirely. + * `waitForExit` still settles such a child on its `error` event. + */ +function killChild(child: ChildProcess): void { + try { + child.kill("SIGKILL"); + } catch { + // Nothing to signal; the `error` event is what settles this child. + } +} + +function isPidAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + const code = + error && typeof error === "object" && "code" in error ? error.code : null; + return code === "EPERM"; + } +} + +/** + * A PID that is genuinely dead: a child this process started, signalled, and + * reaped. + * + * Deadness is re-asserted immediately before the PID is handed over, because + * "a just-exited PID is not reused" is only true where PIDs come from a + * monotonic counter. Linux and macOS qualify; Windows does not — its PIDs come + * from a pool and can be handed out again promptly. The callers that assert + * unbind *removes* a dead PID's files run on every platform, so a recycled PID + * would make unbind correctly preserve the file and the test fail — an + * intermittent Windows-only failure in a cleanup test, which looks exactly + * like the bug the test guards. The check turns that into an immediate, + * legible fixture error instead. + */ +export async function withDeadPid( + run: (pid: number) => Promise | T, + options: OwnedPidOptions = {}, +): Promise { + const child = (options.spawnChild ?? spawnIdleChild)(); + const pid = child.pid; + if (pid === undefined) { + killChild(child); + await waitForExit(child); + throw new Error( + "owned-pids: failed to spawn a probe process while building a dead PID", + ); + } + killChild(child); + await waitForExit(child); + if (isPidAlive(pid)) { + throw new Error( + `owned-pids: pid ${pid} was recycled between reaping it and using it; ` + + "this fixture needs a PID that stays dead for the length of the test", + ); + } + return await run(pid); +} + +/** + * `count` distinct dead PIDs at once, so a fixture needing several does not + * nest `withDeadPid` callbacks one inside the next. + */ +export async function withDeadPids( + count: number, + run: (pids: number[]) => Promise | T, + options: OwnedPidOptions = {}, +): Promise { + const spawnChild = options.spawnChild ?? spawnIdleChild; + // Spawned and reaped in batches rather than all at once. The stress fixtures + // ask for hundreds, and launching that many processes simultaneously can hit + // a process-table or fd limit and fail the spawn — which would surface as a + // fixture error indistinguishable from the bug under test. Batching keeps the + // instantaneous footprint small while still yielding `count` distinct PIDs. + const batchSize = 32; + const deadPids: number[] = []; + for (let offset = 0; offset < count; offset += batchSize) { + const size = Math.min(batchSize, count - offset); + const children = Array.from({ length: size }, () => spawnChild()); + const pids = children.map((child) => child.pid); + // Reap the whole batch first — including any child that failed to spawn, + // which `waitForExit` now settles on `error` — and only then decide whether + // the batch was usable. Throwing before the cleanup would leak the + // siblings that did start. + await Promise.all( + children.map(async (child) => { + killChild(child); + await waitForExit(child); + }), + ); + if (pids.some((pid) => pid === undefined)) { + throw new Error( + "owned-pids: failed to spawn a probe process while building a dead-PID batch", + ); + } + deadPids.push(...(pids as number[])); + } + const recycled = deadPids.filter((pid) => isPidAlive(pid)); + if (recycled.length > 0) { + throw new Error( + `owned-pids: pid(s) ${recycled.join(", ")} were recycled between ` + + "reaping them and using them; this fixture needs PIDs that stay dead", + ); + } + return await run(deadPids); +} + +/** + * A PID that is genuinely alive for the duration of `run`, and killed + * afterwards whether `run` throws or not. + */ +export async function withLivePid( + run: (pid: number) => Promise | T, + options: OwnedPidOptions = {}, +): Promise { + const child = (options.spawnChild ?? spawnIdleChild)(); + const pid = child.pid; + if (pid === undefined) { + killChild(child); + await waitForExit(child); + throw new Error( + "owned-pids: failed to spawn a probe process while building a live PID", + ); + } + try { + return await run(pid); + } finally { + killChild(child); + await waitForExit(child); + } +} + +/** + * `count` distinct live PIDs at once, all killed afterwards whether `run` + * throws or not. Used where a fixture needs more concurrent live helpers than + * the code under test's parallelism bound. + */ +export async function withLivePids( + count: number, + run: (pids: number[]) => Promise | T, + options: OwnedPidOptions = {}, +): Promise { + const spawnChild = options.spawnChild ?? spawnIdleChild; + const children = Array.from({ length: count }, () => spawnChild()); + try { + const pids = children.map((child) => child.pid); + if (pids.some((pid) => pid === undefined)) { + throw new Error( + "owned-pids: failed to spawn a probe process while building a live-PID set", + ); + } + return await run(pids as number[]); + } finally { + // Same contract as the dead-PID batch: a child that failed to spawn settles + // on `error` and is not signalled, so this cleanup can neither hang on it + // nor throw out of the `finally`. + await Promise.all( + children.map(async (child) => { + killChild(child); + await waitForExit(child); + }), + ); + } +} diff --git a/test/owned-pids-helper.test.ts b/test/owned-pids-helper.test.ts new file mode 100644 index 00000000..5b01e616 --- /dev/null +++ b/test/owned-pids-helper.test.ts @@ -0,0 +1,162 @@ +import { spawn } from "node:child_process"; +import process from "node:process"; +import { describe, expect, it } from "vitest"; +import { withDeadPid, withDeadPids, withLivePid, withLivePids } from "./helpers/owned-pids.js"; + +// The lifecycle fixtures depend on these helpers being facts rather than +// approximations, so the helpers themselves need coverage. The hang is the +// dangerous one: a helper that never resolves turns a test failure into a +// suite that sits there until the runner's timeout, with no useful output. + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + const code = + error && typeof error === "object" && "code" in error ? error.code : null; + return code === "EPERM"; + } +} + +describe("owned-pids", () => { + it("hands out a PID that is genuinely dead", async () => { + await withDeadPid((pid) => { + expect(Number.isInteger(pid)).toBe(true); + expect(pid).toBeGreaterThan(0); + expect(isAlive(pid)).toBe(false); + }); + }); + + it("hands out a PID that is genuinely alive, and reaps it afterwards", async () => { + let captured = 0; + await withLivePid((pid) => { + captured = pid; + expect(isAlive(pid)).toBe(true); + }); + // Killed on the way out rather than left for the OS. + expect(isAlive(captured)).toBe(false); + }); + + it("kills the live PID even when the body throws", async () => { + let captured = 0; + await expect( + withLivePid((pid) => { + captured = pid; + throw new Error("boom"); + }), + ).rejects.toThrow("boom"); + expect(isAlive(captured)).toBe(false); + }); + + it("produces distinct PIDs in batches larger than one spawn round", async () => { + // The batch size is an implementation detail; asking for more than one + // batch is what proves the loop stitches them together rather than + // returning only the last batch. + const count = 40; + await withDeadPids(count, (pids) => { + expect(pids).toHaveLength(count); + expect(new Set(pids).size).toBe(count); + for (const pid of pids) { + expect(isAlive(pid)).toBe(false); + } + }); + }, 60_000); + + it("keeps every PID in a live batch alive for the body and reaps them after", async () => { + let captured: number[] = []; + await withLivePids(5, (pids) => { + captured = [...pids]; + expect(new Set(pids).size).toBe(pids.length); + for (const pid of pids) { + expect(isAlive(pid)).toBe(true); + } + }); + for (const pid of captured) { + expect(isAlive(pid)).toBe(false); + } + }, 60_000); + + // A child that fails to spawn emits `error` and never `exit`, so waiting on + // `exit` alone left the promise pending forever — and because a batch is + // awaited concurrently, one failed spawn stalled every sibling and hung the + // run rather than failing it. + // + // These drive the helpers themselves through a substituted spawn factory. + // Asserting on a child the test spawns directly would only demonstrate what + // Node does, and would keep passing with the handling in `waitForExit` + // deleted — the failure it is supposed to catch. + describe("a child that never spawns", () => { + const spawnFailingChild = () => + spawn("definitely-not-a-real-binary-2f8c1d", ["--nope"], { + stdio: ["pipe", "ignore", "ignore"], + }); + + // Bounded well inside vitest's own timeout, so a regression reads as this + // assertion failing rather than as a suite that sits there until the + // runner gives up. + async function settlesWithin( + work: Promise, + budgetMs: number, + ): Promise { + let timer: ReturnType | undefined; + try { + return await Promise.race([ + work.then( + () => "resolved", + (error: unknown) => + `rejected: ${error instanceof Error ? error.message : String(error)}`, + ), + new Promise((resolve) => { + timer = setTimeout(() => resolve("HUNG"), budgetMs); + }), + ]); + } finally { + if (timer !== undefined) clearTimeout(timer); + } + } + + it("makes withDeadPids reject instead of hanging", async () => { + const outcome = await settlesWithin( + withDeadPids(4, () => "unreachable", { + spawnChild: spawnFailingChild, + }), + 5_000, + ); + expect(outcome).not.toBe("HUNG"); + expect(outcome).toContain("failed to spawn"); + }, 30_000); + + it("makes withLivePids reject instead of hanging", async () => { + const outcome = await settlesWithin( + withLivePids(4, () => "unreachable", { + spawnChild: spawnFailingChild, + }), + 5_000, + ); + expect(outcome).not.toBe("HUNG"); + expect(outcome).toContain("failed to spawn"); + }, 30_000); + + it("makes withDeadPid reject instead of hanging", async () => { + const outcome = await settlesWithin( + withDeadPid(() => "unreachable", { spawnChild: spawnFailingChild }), + 5_000, + ); + expect(outcome).not.toBe("HUNG"); + expect(outcome).toContain("failed to spawn"); + }, 30_000); + + it("makes withLivePid reject instead of hanging", async () => { + // The fourth entry point, and the last unexercised failed-spawn branch: + // its cleanup runs from a `finally`, where an unguarded `kill` throw + // would replace the reported reason with its own. + const outcome = await settlesWithin( + withLivePid(() => "unreachable", { spawnChild: spawnFailingChild }), + 5_000, + ); + expect(outcome).not.toBe("HUNG"); + expect(outcome).toContain("failed to spawn"); + }, 30_000); + }); +}); diff --git a/test/runtime-current-account.test.ts b/test/runtime-current-account.test.ts index 5a85efa6..20d85c3f 100644 --- a/test/runtime-current-account.test.ts +++ b/test/runtime-current-account.test.ts @@ -9,8 +9,10 @@ import { resolveRuntimeCurrentAccount, } from "../lib/runtime/runtime-current-account.js"; import { APP_RUNTIME_HELPER_STATUS_FILE } from "../lib/runtime-constants.js"; +import { RUNTIME_HELPER_STATUS_STALE_MS } from "../lib/runtime/app-helper-selection.js"; import type { AccountStorageV3 } from "../lib/storage.js"; import { removeWithRetry } from "./helpers/remove-with-retry.js"; +import { withDeadPid, withLivePid } from "./helpers/owned-pids.js"; function createStorage(): AccountStorageV3 { return { @@ -307,37 +309,72 @@ describe("resolveRuntimeCurrentAccount", () => { }); it("only turns a running live app helper status into a runtime signal", () => { + const now = Date.now(); const baseStatus = { kind: "codex-app-runtime-rotation-helper", state: "running", pid: process.pid, + startedAt: now - 60_000, lastAccountIndex: 1, lastAccountLabel: "Account 2", lastAccountEmail: null, lastAccountId: "acc_runtime", - lastAccountUpdatedAt: 10_000, - updatedAt: 10_000, + lastAccountUpdatedAt: now - 1_000, + updatedAt: now - 1_000, }; - expect(appRuntimeHelperStatusToSignal(baseStatus)).toMatchObject({ + expect(appRuntimeHelperStatusToSignal(baseStatus, now)).toMatchObject({ source: "app-helper", lastAccountIndex: 1, lastAccountId: "acc_runtime", }); expect( - appRuntimeHelperStatusToSignal({ - ...baseStatus, - state: "idle-timeout", - }), + appRuntimeHelperStatusToSignal( + { + ...baseStatus, + state: "idle-timeout", + }, + now, + ), ).toBeNull(); expect( - appRuntimeHelperStatusToSignal({ - ...baseStatus, - kind: "unrelated-process", - }), + appRuntimeHelperStatusToSignal( + { + ...baseStatus, + kind: "unrelated-process", + }, + now, + ), ).toBeNull(); }); + it("refuses to signal from a running record too stale to have a live writer", () => { + // A live helper republishes at least once per heartbeat, so a `running` + // record older than the staleness window was not written by whoever holds + // its PID now. This is the identity check the readers were missing: the + // PID here is unquestionably alive — it is this very process. + const now = Date.now(); + const stale = { + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: process.pid, + startedAt: now - RUNTIME_HELPER_STATUS_STALE_MS - 120_000, + lastAccountIndex: 1, + lastAccountLabel: "Account 2", + lastAccountEmail: null, + lastAccountId: "acc_stale", + lastAccountUpdatedAt: now - RUNTIME_HELPER_STATUS_STALE_MS - 60_000, + updatedAt: now - RUNTIME_HELPER_STATUS_STALE_MS - 60_000, + }; + expect(appRuntimeHelperStatusToSignal(stale, now)).toBeNull(); + expect( + appRuntimeHelperStatusToSignal( + { ...stale, updatedAt: now - 1_000, lastAccountUpdatedAt: now - 1_000 }, + now, + ), + ).not.toBeNull(); + }); + it("labels stored selected and runtime in-use rows separately", () => { const runtimeCurrent = { index: 1, @@ -514,6 +551,7 @@ describe("readAppRuntimeHelperStatus", () => { kind: " codex-app-runtime-rotation-helper ", state: "running", pid: 42, + startedAt: 5_000, lastAccountIndex: 1, lastAccountLabel: " ", lastAccountEmail: " user@example.com ", @@ -526,6 +564,7 @@ describe("readAppRuntimeHelperStatus", () => { kind: "codex-app-runtime-rotation-helper", state: "running", pid: 42, + startedAt: 5_000, lastAccountIndex: 1, lastAccountLabel: null, lastAccountEmail: "user@example.com", @@ -535,10 +574,161 @@ describe("readAppRuntimeHelperStatus", () => { }); }); + it("rejects a negative or fractional PID instead of probing a process group", async () => { + // `readOptionalNumber` used to accept any finite number, so `-1234` + // reached `process.kill(-1234, 0)` — a POSIX process-group probe that + // succeeds on any busy machine and reports a helper that does not exist + // as live. A PID is a positive integer or it is nothing. + for (const pid of [-1234, 4242.5, 0]) { + await writeStatusFile( + JSON.stringify({ + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid, + lastAccountId: "acc_bogus", + updatedAt: Date.now(), + }), + ); + expect(readAppRuntimeHelperStatus()?.pid).toBeNull(); + expect( + appRuntimeHelperStatusToSignal(readAppRuntimeHelperStatus()), + ).toBeNull(); + } + }); + it("rejects a JSON array status file as not-a-record", async () => { // isRecord() excludes arrays: an `[]` helper-status file is malformed // content, not an all-null status object. await writeStatusFile("[]"); expect(readAppRuntimeHelperStatus()).toBeNull(); }); + + it("prefers a live per-PID helper over a fresher record with a dead PID", async () => { + const now = Date.now(); + // Per-PID file for a live process (this test), older updatedAt. + await fs.writeFile( + join(tempDir, `runtime-rotation-app-helper.${process.pid}.json`), + JSON.stringify({ + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: process.pid, + lastAccountId: "acc_live", + updatedAt: now - 30_000, + }), + "utf8", + ); + // Legacy shared file naming a dead PID, fresher updatedAt: recency must + // not outrank liveness. The dead PID belongs to a process this test + // started and killed, so "dead" is a fact rather than a guess about the + // platform's PID ceiling. + await withDeadPid(async (deadPid) => { + await writeStatusFile( + JSON.stringify({ + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: deadPid, + lastAccountId: "acc_dead", + updatedAt: now, + }), + ); + expect(readAppRuntimeHelperStatus()?.lastAccountId).toBe("acc_live"); + }); + }); + + it("picks the most recently updated helper when several are live", async () => { + // The live branch sorts by recency before returning the first candidate. + // With only one live record that sort is dead weight — inverting or + // deleting it changes nothing — and this selector is what drives runtime + // account resolution, so the ordering needs two live candidates to be a + // claim the suite actually checks (#668). + const now = Date.now(); + await withLivePid(async (otherLivePid) => { + await fs.writeFile( + join(tempDir, `runtime-rotation-app-helper.${process.pid}.json`), + JSON.stringify({ + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: process.pid, + lastAccountId: "acc_older_live", + updatedAt: now - 30_000, + }), + "utf8", + ); + await fs.writeFile( + join(tempDir, `runtime-rotation-app-helper.${otherLivePid}.json`), + JSON.stringify({ + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: otherLivePid, + lastAccountId: "acc_newer_live", + updatedAt: now - 1_000, + }), + "utf8", + ); + expect(readAppRuntimeHelperStatus()?.lastAccountId).toBe( + "acc_newer_live", + ); + }); + }); + + it("reports no live helper for a stale legacy record whose PID was recycled", async () => { + // The #667 scenario, and the one place the two behaviours differ + // observably: a legacy shared file left behind by a SIGKILLed pre-upgrade + // helper, whose PID has since been handed to an unrelated live process. + // `kill(pid, 0)` succeeds, so bare liveness accepts the record as a + // running helper and marks its account `current` — pinning the UI to an + // account no helper is using. Recency cannot save this: there is only one + // record, so it wins selection either way. What has to change is whether + // it counts as *live*. + const now = Date.now(); + await withLivePid(async (recycledPid) => { + await writeStatusFile( + JSON.stringify({ + kind: "codex-app-runtime-rotation-helper", + state: "running", + pid: recycledPid, + lastAccountId: "acc_recycled", + updatedAt: now - RUNTIME_HELPER_STATUS_STALE_MS - 60_000, + }), + ); + // Still the record the fallback reports, so `rotation status` can show + // the last thing a helper said... + expect(readAppRuntimeHelperStatus()?.lastAccountId).toBe("acc_recycled"); + // ...but not a signal, so nothing marks that account as in use. + expect( + appRuntimeHelperStatusToSignal(readAppRuntimeHelperStatus()), + ).toBeNull(); + }); + }); + + it("falls back to the freshest terminal stamp when no helper is live", async () => { + const now = Date.now(); + await withDeadPid(async (olderDeadPid) => { + await withDeadPid(async (newerDeadPid) => { + await fs.writeFile( + join(tempDir, `runtime-rotation-app-helper.${olderDeadPid}.json`), + JSON.stringify({ + kind: "codex-app-runtime-rotation-helper", + state: "idle-timeout", + pid: olderDeadPid, + lastAccountId: "acc_older", + updatedAt: now - 60_000, + }), + "utf8", + ); + await fs.writeFile( + join(tempDir, `runtime-rotation-app-helper.${newerDeadPid}.json`), + JSON.stringify({ + kind: "codex-app-runtime-rotation-helper", + state: "stopped", + pid: newerDeadPid, + lastAccountId: "acc_newer", + updatedAt: now - 10_000, + }), + "utf8", + ); + expect(readAppRuntimeHelperStatus()?.lastAccountId).toBe("acc_newer"); + }); + }); + }); }); diff --git a/test/zz-stress-helper-lifecycle.test.ts b/test/zz-stress-helper-lifecycle.test.ts new file mode 100644 index 00000000..efbf3631 Binary files /dev/null and b/test/zz-stress-helper-lifecycle.test.ts differ