Repository navigation
Stop Codex collab child watchers leaking after the parent session ends - #551
Conversation
A Codex collab child watcher had no exit path at all: it was deliberately excluded from the idle ceiling (on the assumption the parent's teardown finalizes it — that teardown is server-side records only), it gets no --parent-pid watchdog (its spawner is the parent session watcher, whose ancestry contains no codex process), and the server's StopWatcher only reaches the session watcher's connection. Observed: 16 children from one session still running two days after the parent exited, each reconnecting and resending the same tail gap forever. Three layers close the leak: - The parent session watcher now stops every child watcher it spawned (codex/gemini/opencode) as part of its own teardown, via the new WatcherManager.KillWatchers — SIGTERM, so each child runs its final drain before the parent posts session-end. - Codex child watchers join the idle ceiling as a leak backstop for a hard-killed parent: 6h default on the new KCAP_CODEX_SUBAGENT_REAP_MINUTES knob (KCAP_CODEX_SUBAGENT_IDLE_MINUTES is already the stop grace), never while a tool call is in flight. The old "an idle self-exit would end nothing server-side" rationale went stale when children gained their own live subagent-stop POST (#526); once exited, the watcher is GONE and the server sweep can finalize it. - ScanCodexSubagents re-ensures a child watcher whenever its rollout's mtime advances instead of gating on first sight only, so a reaped child whose subagent re-engages is respawned and resumes from the server frontier — without this, the ceiling would turn re-engagement into silent content loss for the rest of the parent's life. Closes #550 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR Summary by QodoStop Codex collab child watcher processes leaking after parent session exit
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
![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: 50d41382d4
ℹ️ 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".
| // skips this, which is exactly the orphan case the codex-child reap ceiling backstops. | ||
| if (spawnedChildWatcherKeys.Count > 0) { | ||
| Log($"Stopping {spawnedChildWatcherKeys.Count} spawned child watcher(s)"); | ||
| await WatcherManager.KillWatchers(spawnedChildWatcherKeys); |
There was a problem hiding this comment.
When a Codex child hits the new idle-reap ceiling, RunWatch exits without removing its .pid file. If the OS reuses that PID before the parent exits, this cleanup passes the stale key to KillWatcher, which trusts the PID file and calls Process.Kill without verifying process identity, potentially terminating an unrelated process. The child should remove its own PID state on exit, or cleanup must verify a stored process start identity before killing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1b8679c. The pid file now carries the incarnation's ProcessStartToken on line 2 (the daemon pid-file layout); KillWatcher spares on a conclusive token mismatch — "ambiguity never kills" — and sweeps the stale file instead. Watchers additionally retire their own pid file on graceful exit (RemoveOwnPidFile, guarded to this incarnation), so the reap-then-teardown window doesn't arise in the first place. Covered by KillWatcher_spares_a_recycled_pid_and_sweeps_the_stale_file (watched fail first: the bystander was killed) and RemoveOwnPidFile_removes_only_this_incarnations_file.
| // skips this, which is exactly the orphan case the codex-child reap ceiling backstops. | ||
| if (spawnedChildWatcherKeys.Count > 0) { | ||
| Log($"Stopping {spawnedChildWatcherKeys.Count} spawned child watcher(s)"); | ||
| await WatcherManager.KillWatchers(spawnedChildWatcherKeys); |
There was a problem hiding this comment.
When the parent exits through StopWatcher, SIGINT, or another path that does not run a vendor teardown with InlineDrainAsync, this call can lose the child's most recent transcript tail. KillWatchers delegates to KillWatcher, whose Process.Kill(entireProcessTree: false) forcibly terminates the process rather than sending SIGTERM, so the child's registered signal handler and final drain/spool code never execute despite the new cleanup relying on them. Send a real graceful signal first, or explicitly inline-drain each child before the force kill.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1b8679c. Confirmed: Process.Kill(entireProcessTree: false) is SIGKILL on Unix — the trap test observed exit 137 pre-fix. KillWatcher now sends SIGTERM first via a libc kill P/Invoke, keeping the existing 5s wait + force-kill fallback, so the child's registered SIGTERM handler runs its final drain + undelivered-tail spool. KillWatcher_sigterms_first_so_the_watcher_can_run_its_drain pins it (bash trap exits 42, not 137/143). Windows keeps the hard stop — no SIGTERM semantics there; recovery stays with the spool/import paths, and the docs now say so.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Two review findings were real and are now fixed with watched-red tests:
- Process.Kill is SIGKILL on Unix (the old "Send SIGTERM" comment was
wrong), so a "gracefully stopped" child never ran its final drain +
undelivered-tail spool. KillWatcher now signals SIGTERM first via a
libc kill P/Invoke (exit 143->trap proves delivery), keeping the 5s
force-kill bound. Windows keeps the hard stop; recovery stays with the
spool/import paths.
- A self-reaped watcher leaves its pid file behind, so the parent's new
teardown could kill an unrelated process through a recycled pid. The
pid file now carries the incarnation's ProcessStartToken on line 2
(daemon pid-file layout); KillWatcher spares on a conclusive token
mismatch ("ambiguity never kills") and watchers retire their own pid
file on graceful exit as the first line of defence.
Also trims the more verbose comment blocks flagged by review.
AI-1928 / #550
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Closes #550 (AI-1928)
Problem
A Codex collab child watcher had no exit path at all — observed as 16
kcap watch … --vendor codexprocesses from a single session still running two days after the parent session ended, each stuck in an infinite reconnect-and-resend loop against the server:ShouldEndOnIdledeliberately excluded codex children, assuming "the parent's session-end teardown finalizes them" — but that teardown finalizes server-side records only, never the local processes.codexprocess forGetCodingAgentPidto resolve, so every child starts with "No parent pid supplied; parent-exit watchdog disabled".KillWatcherwas never called fromWatchCommand, and the server'sStopWatchersignal only reaches the session watcher's connection.Fix (three layers)
WatcherManager.KillWatchers— SIGTERM first, so each child runs its final drain before the parent posts session-end; the existing per-child 5s force-kill bound keeps a wedged child from stalling the parent's exit.KCAP_CODEX_SUBAGENT_REAP_MINUTESknob (KCAP_CODEX_SUBAGENT_IDLE_MINUTESis already the subagent-stop grace), and never while a tool call is in flight — mirroring the Claude subagent ceiling from Reap leaked Claude subagent watchers with an idle ceiling #517. The original "an idle self-exit would end nothing server-side" rationale went stale with [AI-1861] Post live subagent-stop from Codex collab child watchers #526: children post their own live subagent-stop, and once exited the watcher is GONE, which is exactly the state the server sweep can finalize.ScanCodexSubagentsre-ensures a child watcher whenever its rollout's mtime advances instead of gating on first sight only, so a reaped child whose subagent re-engages is respawned and resumes from the server frontier. Without this, the ceiling would turn re-engagement into silent content loss for the rest of the parent's life.Testing
ChildRolloutAdvancedgate (TDD, watched red first); the codex regression guard that pinned the old ineligible behavior is superseded and updated.KillWatchers_stops_every_tracked_child_and_clears_their_pid_files— one live and one already-dead child in a batch, both swept.README documents the new knob and reaping behavior in the same PR.
🤖 Generated with Claude Code