fix(v1): reap env pool worker subprocesses#1995
Open
morluto wants to merge 5 commits into
Open
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7d281db. Configure here.
6 tasks
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
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.

Description
Fixes #1992.
Env-server pool workers now lead a POSIX session that owns their local subprocesses.
SubprocessRuntimestill gives each rollout its own process group, but those groups remain inside the worker session. Pool shutdown keeps the existing gracefulSIGTERMwindow, then freezes and kills any processes left in the worker session.Shutdown waits on multiprocessing sentinels without joining first. This keeps an exited worker's PID and session ID reserved until session cleanup finishes, avoiding stale descendant-PID snapshots and PID-reuse races.
Provider-side sandbox and tunnel registrations are outside the OS session and remain tracked in #1994.
Type of Change
Testing
uv run pytestlocally.Validation performed:
SIGTERMand when the worker exited during the grace perioduv run pytest tests/test_env_server.py— 15 passeduv run ruff check --fix .— passeduv run pre-commit run --all-files— Ruff passed; the generated-guide check reported pre-existing staleenvironments/AGENTS.mdoutputsuv run ty check verifiers/utils/process_utils.py verifiers/v1/serve/pool.py— passedThe repository asks contributors not to add unit tests; the regression proof was run as a temporary process-level script.
Checklist
Additional Notes
Suggested review order:
verifiers/v1/serve/pool.py: worker session creation and sentinel-based shutdown orderingverifiers/v1/runtimes/subprocess.py: per-rollout process groups remain inside the worker sessionverifiers/utils/process_utils.py: freeze-and-kill session cleanupPR #1993 also changes
pool.pyfor worker-death recovery. The changes are independent but may require a small rebase after either PR merges.Note
Medium Risk
Changes POSIX signal handling and process-tree teardown during pool shutdown; mistakes could affect unrelated processes if session discovery misidentifies leaders, though the code guards non-leader workers by killing only the worker PID.
Overview
Fixes leaked rollout subprocesses when env-server pool workers shut down by treating each worker as a POSIX session leader and reaping the whole session instead of killing only the worker PID.
Pool workers now enter via
_serve_pool_worker, which callsos.setsid()beforeserve_env, so the worker PID doubles as the session ID for cleanup.EnvServerPool._shutdownstill sends gracefulSIGTERM, then waits on multiprocessing sentinels (without joining first, so the zombie keeps the session identity stable), and escalates withkill_process_sessionrather than a bareprocess.kill(). Shutdown temporarily ignoresSIGINT/SIGTERMon the broker while tearing workers down.kill_process_session(new inprocess_utils) discovers session members via/procwith apsfallback, freezes live PIDs withSIGSTOPuntil membership stabilizes (late forks), then iterativelySIGKILLs session members and joins the worker.SubprocessRuntimeswitches fromstart_new_session=Truetoprocess_group=0, so each rollout still gets its own process group forkillpgon cancel/cleanup, but those groups stay inside the worker session for pool-level reaping.Reviewed by Cursor Bugbot for commit 55e7b71. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Reap env pool worker subprocesses by killing entire POSIX sessions on shutdown
os.setsid()via a new_serve_pool_workerwrapper in pool.py, making it a session leader so all its descendants share a session ID.EnvServerPool._shutdownwaits on worker sentinels (10s timeout), then calls a newkill_process_sessionutility to freeze and SIGKILL every process in the worker's session, ensuring no orphaned subprocesses remain.kill_process_sessionin process_utils.py discovers session members via/proc(orps -Aas fallback), SIGSTOPs live members to prevent forking, then iteratively SIGKILLs until the session is drained or a timeout is reached.process_group=0instead ofstart_new_session=True, keeping them inside the worker's session so they are caught by session-level cleanup.Macroscope summarized 55e7b71.