This repository was archived by the owner on Jul 4, 2026. It is now read-only.
fix(codex_app_server): kill the whole process group on close - #8
Merged
Conversation
``CodexAppServerClient`` spawned the codex subprocess without a dedicated session/process group. On the shipped install ``/usr/local/bin/codex`` is a node wrapper that re-execs the native ``codex`` binary as a child, so ``Popen.terminate()`` / ``Popen.kill()`` only signalled the wrapper — the native binary survived as an orphan grandchild still holding the rollout sqlite open. Every retired session (turn-level timeout, post-tool watchdog, manual ``/codex-runtime`` toggle, etc.) leaked one of these orphans. Once enough piled up, fresh codex spawns timed out at the 10s ``initialize`` deadline waiting for the sqlite lock — surfaced to the user as ``codex app-server method 'initialize' timed out after 10.0s``. Live pods accumulated 130+ orphans over ~12h before the lock contention fully wedged the runtime. This commit: - ``Popen(..., start_new_session=True)`` so the codex subprocess (and any descendant it execs) lives in its own POSIX session/process group, separated from the gateway's group. Windows ignores the flag — irrelevant because the wrapper-vs-native split is a Linux/macOS packaging artifact. - ``close()`` now signals the **process group** via ``os.killpg`` so the wrapper + native binary die together. Falls back to the old ``terminate()``/``kill()`` path when ``getpgid``/``killpg`` are unavailable (Windows) or the group is already gone (``ProcessLookupError``). The SIGTERM → wait → SIGKILL escalation is preserved. Test plan (``tests/agent/transports/test_codex_app_server_process_group.py``): - Spawn a sh wrapper that fork-execs a 60-second sleep grandchild, capture both pids, call ``client.close()``. Assert the wrapper is in its own session (``getpgid(pid) == pid``), the grandchild inherits that pgid, and both are reaped after ``close()``. Without the fix the grandchild stays alive past close. - ``close()`` is idempotent against an already-exited subprocess. - Both tests carry ``@pytest.mark.live_system_guard_bypass`` since they intentionally do real ``Popen`` + signal delivery against their own child subtree. 59/59 pass (2 new + 57 pre-existing session tests, no regressions). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kingsleydon
pushed a commit
that referenced
this pull request
Jun 3, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
Summary
`CodexAppServerClient` spawns the codex subprocess without a dedicated process group. On the shipped npm install, `/usr/local/bin/codex` is a node wrapper that re-execs the native `codex` binary as a child, so `Popen.terminate()` / `Popen.kill()` only signal the wrapper — the native binary survives as an orphan grandchild still holding the rollout sqlite.
Every retired session (turn-level timeout, post-tool watchdog, manual `/codex-runtime` toggle, etc.) leaks one of these orphans. Once enough pile up, fresh codex spawns time out at the 10 s `initialize` deadline waiting for the sqlite lock — what users see as:
A live k3s pod running this fork accumulated ~130 orphans over 12 hours before the codex runtime fully wedged. lsof on `/data/.codex/logs_2.sqlite` showed one of the orphans holding 20+ file descriptors against it.
Fix
Two-line behavioural change:
`Popen(..., start_new_session=True)` at spawn — codex subprocess (and any descendant it execs) lives in its own POSIX session/process group, separated from the gateway's group. Windows ignores the flag, irrelevant since the wrapper-vs-native split is a Linux/macOS packaging artifact.
`close()` signals the process group via `os.killpg` so wrapper + native binary die together. Falls back to the old `terminate()`/`kill()` path when `getpgid`/`killpg` are unavailable (Windows) or the group is already gone. SIGTERM → 3 s wait → SIGKILL escalation preserved.
Test plan
`tests/agent/transports/test_codex_app_server_process_group.py`:
Both tests carry `@pytest.mark.live_system_guard_bypass` since they intentionally do real `Popen` + signal delivery against their own child subtree (the existing repo guard otherwise blocks raw `os.kill`).
`pytest tests/agent/transports/test_codex_app_server_process_group.py tests/agent/transports/test_codex_app_server_session.py` → 59/59 pass (2 new + 57 pre-existing, no regressions).
Operational note
Pods already wedged by orphans need a one-time cleanup before the fix takes effect on the live process:
```
supervisorctl stop hermes-agent
pkill -KILL -f 'codex app-server'
supervisorctl start hermes-agent
```
After this PR lands and the image rebuilds, that manual cleanup stops being necessary.
🤖 Generated with Claude Code