fix(codex): child lifecycle hardening — launch-leak remediation (stages 1a+1c)#510
Merged
Conversation
Staged plan targeting the codex "won't launch" root cause on main: per-pane codex processes pin ~/.codex/logs_2.sqlite so its WAL can never truncate, while codex's TRACE firehose grows it unbounded. - 0a: spawn-env log dampening (RUST_LOG only-if-unset, kill switch) - 0b: one-time lossless cleanup runbook (checkpoint TRUNCATE + VACUUM, verified backup + row-count oracle; never rm live data) - 1a: crash-safe teardown (sync process-group reap on exit/SIGHUP; uncaughtExceptionMonitor observe-only) - 1c: startup reaper fail-open (quarantine instead of exit(1)) Constraints honored: lossless, no pane cap, never kill attached panes, no new won't-launch paths. Follow-ups (1b, shared app-server Stage 2, monitor) referenced but out of scope. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Rewrites the 0a/0b/1a/1c remediation plan per the 2026-07-06 adversarial review (F1-F6, per-stage flaws, gaps G1-G6): - Corrects the WAL model (read-txn spanning checkpoint, not "zero connections"); firehose reframed as bounded-table churn (~22 MB/min measured; per-process ~1000-row prune bound) - New Stage 0-pre: log-dampening validation experiment RUN, negative for plain RUST_LOG=error (idle-TUI control 1 row vs treatment 1013); 0a now gated on a validated knob (scoped 'log=' directive or -c), with explicit fallback if none passes - 0a: helper consults child env only, ''-preset respected, kill switch requires restart (stated), third spawn site added (session-manager.ts codex exec), WSLENV caveat - 0b: reframed as consented maintenance window (kills live panes - I3 carve-out documented); run from outside freshell; graceful server stop over SIGSTOP; online-consistent .backup/VACUUM INTO; baselines at 0 holders; logs_2 exact row/max(id) equality; never auto-restore - 1a: renamed exception/signal-safe (honest coverage: no SIGKILL/OOM); deregister on confirmed group death / pty exit; POSIX-gated; registry-scoped assertions; uncaught-exception test replaces vacuous caught-exception test; shutdown hard-exit timeout - 1c: fail-open completed across all three formerly-fatal boot paths (readFile :811, /proc proof :800, readdir :803) + backstop at index.ts:256; quarantine only provably-dead owners; time-based backoff (shared prod+dev dir safe); 0600 preserved; boot/hourly WAL+holder observability line promoted into scope - New: deploy choreography (single declared window), honest residual risk statement, corrected traceability table Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Addresses all findings from the second adversarial review of v2 (verdict: APPROVE-WITH-CHANGES, 20/24 v1 findings closed): - #1 (blocking): S3 measurement rewritten to prune-immune metrics (isolated-holder WAL-growth diff or 1s incremental insert attribution); results table re-captioned as post-prune lower bounds (treatment 1,013 rows sits at the ~1000-row prune ceiling) - #2 (blocking): 1c quarantine narrowed to confirmed-gone groups or unparseable records only; owner-dead-group-alive (survived SIGKILL, e.g. D-state on the bloated WAL) is never quarantined; boot-time quarantine rescan promotes live-pgid records back for retry - #3: candidate 1 is now the composite directive (embedded default + log=off) since any RUST_LOG replaces the embedded filter; exact final string must be recorded - #4: backoff state homed in <record>.reaper.json (reaper-owned, no race with owner rewrites); hourly monitor homed in codex-observability.ts with unref()d timer - #5: logs_2 backup via VACUUM INTO (online .backup can livelock under ~22MB/min churn); low-churn DBs keep .backup - #6: dampening-skip emits a deduped warn (fail-open, not fail-silent) - #7-9: 1a acceptance made executable (registry snapshot before kill + environ scan incl. FRESHELL_TERMINAL_ID for ptys); structural single-caller I3 assertion replaces the universal negative; 15s hard-exit timer specified for all three signals - #10: S8/S5 ordering reconciled (backups live before servers stop); DoD reclaim figure now measured, not assumed - #11: logs.id INTEGER PRIMARY KEY schema check gates the max(id) VACUUM oracle - Anchor drift: WSL wrap :1141-1149; ~12 throwing-contract test sites; unreferenced alias deletion; re-pin note (9f3a503) Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Implements stages 1a and 1c of
docs/plans/2026-07-06-codex-launch-leak-remediation-plan.md (v2.1).
Stage 1c - startup reaper fail-open + observability:
- Per-record isolation in the reap loop; unreadable records isolated;
non-ENOENT readdir and /proc-proof failures degrade-and-continue
- Backstop try/catch at the index.ts reaper call site; boot can no
longer die in the reaper
- Quarantine narrowed: only confirmed-gone groups or unparseable
records; owner-dead-group-alive and alive-mismatched records retry
in place with time-based backoff (<record>.reaper.json, 0600);
boot-time quarantine rescan promotes live-pgid records back
- assertCodexStartupReaperSucceeded deleted (no-throw aggregator);
unreferenced alias removed; ~12 throwing-contract tests inverted
- New codex-observability.ts: boot + hourly read-only status line
(wal_bytes/holders/quarantined) with warn thresholds; never opens
the SQLite DB, never signals; hosts hourly retry + rescan
Stage 1a - exception/signal-safe teardown:
- New codex-child-registry.ts: in-memory {pid,pgid,kind} registry,
POSIX-gated, DI for hermetic tests
- App-servers register at spawn, deregister only on confirmed group
death; resume ptys register at both spawn sites (codex-only),
deregister in onExit handlers
- process.on('exit') -> reapSync (identity-verified SIGKILL of
registered groups only); SIGHUP -> graceful shutdown;
uncaughtExceptionMonitor observe-only; 15s unref'd hard-exit timer
on shutdown entry (all three signals)
- Structural I3 tests: single group-kill primitive, single caller,
referenced only from the exit binding
Verification: tsc -p tsconfig.server.json clean; vitest server suite
test/unit/server/coding-cli 44 files, 766 passed / 3 skipped.
Generated with Amplifier
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Ran the plan v2.1 S3 protocol (prune-immune incremental insert attribution, 1s max(id) polling attributed by process_uuid) against four candidates on codex-cli 0.142.5: - composite RUST_LOG (embedded default + log=off): -18% - -c feedback.enabled=false: -25% - RUST_LOG=off (entire env filter silenced): -23% - -c features.shell_snapshot=false: -10% Pass bar was >=90%. RUST_LOG=off failing proves the SQLite feedback sink does not consult the env filter at all - it is an always-on flight recorder (bounded_feedback_logs) with hardcoded TRACE capture. features list confirms the sqlite feature is stage "removed" (always-on). Per S3 candidate 3, the drop path is invoked: 0a is dropped. Protection = 0b cleanup + 1c observability (shipped) + 1a/1c lifecycle hardening (shipped); Stage 2 (shared app-server) becomes the load-bearing fix for WAL growth. Draft upstream issue text included in the results doc. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Panel: correctness/concurrency, security/safety, operations/reliability (3x stateless adversarial reviews of commit 24a6a11). All 6 majors + 9 minors fixed: MAJOR: - reapSync identity hardening: pin /proc starttime at registration; require starttime + pgrp match + codex cmdline before group kill (closes stale-entry pid-reuse kill of foreign live panes); dead-pid fallback scans the registered pgid for live codex members instead of skipping (closes the wrapper-dead/grandchildren-alive gap) - Boot reap time budget (CODEX_REAPER_BOOT_BUDGET_MS=3s): stuck records defer to the hourly tick instead of stalling pre-listen boot - Holder scan: 12-way bounded concurrency, codex-cmdline prefilter, self-skip, realpath + " (deleted)" target matching - Log escalation is wall-time (6h from firstSeen), not attempt-count (immune to tsx-watch restart storms / shared-dir double counting) - WAL warn threshold 500MB -> 2GiB with honest margin comment; wal_bytes=-1 + walStatFailed on non-ENOENT stat errors - Hard-exit timer: 30s, closeAllConnections on shutdown entry, sync stderr write on force-exit, cleared on happy path minor: no-ownership spawn-failure deregisters on child exit; per-record backoff gating on hourly passes; quarantine note written before rename + orphan-note cleanup; observability MUST-NOT-signal comment scoped; diagnostics skip .reaper.json/.note.json; pty deregister codex-gated and only on confirmed group death (ESRCH); observability stop() wired first in shutdown; stale atomic-write tmp cleanup (1h). Verification: tsc server clean; vitest test/unit/server/coding-cli 44 files, 783 passed / 3 skipped. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Round 2 verified all round-1 findings closed and found 2 new majors in the fix code plus minors. All fixed: MAJOR: - Ownership-gate the dead-pid group-kill fallback: registry entries now carry an envMarker (FRESHELL_CODEX_SIDECAR_ID for app-servers, FRESHELL_TERMINAL_ID for resume ptys); the fallback requires an exact NAME=VALUE match in a member's /proc environ before any group kill; no marker / unreadable / truncated-without-marker fail closed. Closes the recycled-pgid foreign-pane kill (I3) two reviewers found. - Budget deferral no longer counts as an attempt: deferral anchors firstSeen only, never bumps attempts/lastAttempt, and sidecar-less or lastAttempt-less records are always due - the hourly unbudgeted tick can no longer be starved by tsx-watch restart cadence. minor: unpinned-starttime entries route to the gated group scan; argv0-token codex predicate replaces substring (rejects paths merely containing "codex"); bounded 4KiB /proc reads with truncation fail-closed; single shared /proc index per reap pass; monotonic budget clock (performance.now); hourly tick sees sidecar-less dead-owner records; rename-loser note unlink guarded; anchored tmp pattern + quarantine sweep; deduped warn on sidecar write failure; closeIdleConnections + 3s deferred closeAllConnections; hourly resume-pty group re-probe drains stale entries; reap completeness caveat documented. Tests: +36 behavioral tests (env-marker gating suite, deferral due-ness, argv0 predicate, pty probe ESRCH/EPERM, spawn-failure deregister, memoized scan). tsc clean; vitest server coding-cli 44 files, 802 passed / 3 skipped. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Round 3 confirmed all round-2 closures and converged (2 independent reviewers, with on-host measurements) on one major in the round-2 fix itself: MAJOR: the envMarker ownership gate read /proc/<pid>/environ bounded at 4 KiB, but both spawn sites append the marker LAST in env-spread order and real environs on this host measure ~5,900 bytes - the marker was past the read on essentially every spawn, silently disabling the dead-pid group reap (fail-closed leak regression). Fixed: dedicated PROC_ENVIRON_READ_MAX_BYTES = 256 KiB for environ reads (4 KiB kept for cmdline); debug log distinguishes "not ours" from "marker unreachable"; env-spread order intentionally unchanged (marker-first would break override semantics when the dev server itself runs inside a freshell pane). minor: truncated environ drops its final NUL-split token before the marker match (a torn trailing entry is never proof); deferral sidecar writes use create-exclusive (wx) so a concurrent instance's attempt state is never clobbered; readProcFileBoundedSync exported and covered against real temp files (truncation, exact-size, ENOENT). Tests: +4 new / 2 re-fixtured, including a regression test pinning the marker at its real-world ~5,900-byte position (kill fires) and a >256 KiB fail-closed case. tsc clean; vitest server coding-cli 44 files, 806 passed / 3 skipped. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47e4a92054
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| const parsed: unknown = JSON.parse(await fsp.readFile(recordPath, 'utf8')) | ||
| const ownerServerPid = (parsed as { ownerServerPid?: unknown } | null)?.ownerServerPid | ||
| if (isPositiveInteger(ownerServerPid) && (await isPidAlive(ownerServerPid))) continue |
There was a problem hiding this comment.
When a sidecar-less ownership record is the only pending record, this liveness-only check prevents the hourly maintenance tick from invoking the reaper if the old ownerServerPid has been reused by any live process. The startup reaper already treats a live PID with a mismatched ownerServerIdentity as retryable, but this gate never reaches that classification, so a crashed server's leaked Codex sidecar/DB holder can remain until a restart or some unrelated due record triggers a pass. Please verify the recorded owner identity here (or otherwise let the reaper classify) instead of skipping on PID liveness alone.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Codex child-process lifecycle hardening per the remediation plan (stages 1a + 1c):
server/coding-cli/codex-app-server/codex-child-registry.ts) and observability module (codex-observability.ts)server/coding-cli/codex-app-server/runtime.tsserver/index.tsandserver/terminal-registry.tsdocs/plans/2026-07-06-codex-launch-leak-remediation-plan.mdanddocs/plans/2026-07-06-codex-launch-leak-remediation-s3-validation-results.md)Why
Remediate leaked codex child processes. The work was hardened through 4 rounds of adversarial review (round-1/2/3 findings addressed in follow-up commits on this branch).
Verification
npm run check— typecheck + 8,681 tests passed, 0 failedcodex-child-registry.test.ts(855 lines),codex-observability.test.ts, plus expandedruntime.test.tsNote
This work was originally authored on local main before the PR-only workflow could catch it; this PR moves it through the proper flow.
Generated with Amplifier