Repository navigation
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.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7d07537. Configure here.
5 of 13 tasks
This was referenced Jul 20, 2026
Member
|
superseded by #2518 |
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 #1991.
The v1 env-server broker now registers each worker's process sentinel with its existing PyZMQ poller. When a worker exits, the broker:
Requests are failed rather than transparently replayed because a rollout may have made model calls or other external changes before its worker exited.
Hard-shutdown ownership of worker subprocess trees is tracked separately in #1992. This change does not claim to reclaim descendants or remote resources after
SIGKILL.Type of Change
Testing
uv run pytestlocally.Validation performed:
uv run ruff check --fix .— passedenv server worker exited unexpectedly (exit code -9), replaced the worker, served a subsequent request, and kept the broker aliveuv run pytest tests/v1/test_configs.py tests/v1/test_taskset.py tests/test_env_server.py— 32 passeduv run pytest tests/— 956 passed, 148 skipped, 6 unrelated failures: four from the incompleteenvironments/hello_mcp_harbordirectory and two environment-load checks requiring OpenAI credentialsuv run pre-commit run --all-files— Ruff passed; the generated-guide check reported pre-existing staleenvironments/AGENTS.mdoutputs7d07537a8lost dead-worker cleanup ownership when cancellation interrupted the final-reply relay; this branch kept cleanup under_shutdownownership and restored a replacement before an interrupted failure sendorigin/main— clean, with no accepted or actionable findingsChecklist
Additional Notes
Suggested review order:
_spawn_worker()sentinel registration_drain_worker_replies()and_worker_died()lifecycle handlingrun()ordering for replies, worker exits, and new frontend requestsNote
Medium Risk
Changes broker request lifecycle and client-visible errors on worker crash; behavior is intentional but affects rollout reliability and in-flight accounting in the env-server pool.
Overview
The v1 EnvServerPool broker now watches each worker’s process sentinel on the existing ZMQ poller and recovers when a worker dies instead of leaving in-flight rollouts stuck.
On exit it drains any replies already queued on that worker’s DEALER, fails remaining requests with an explicit
BaseResponseerror (no silent replay), releases rollout-slot accounting, tears down IPC/socket/pipe state, and spawns a replacement in the same worker index. The main loop handles dealer replies before sentinel exits so a final successful reply is not treated as a failure._spawn_workeraccepts an optional index for slot-preserving restarts; workers register both dealer and sentinel with the poller.Reviewed by Cursor Bugbot for commit 7d07537. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Recover env pool after worker exit in
EnvServerPoolbroker_spawn_workerso the broker can detect when a worker process dies via poller events._drain_worker_repliesto non-blockingly flush queued replies from a dead worker's DEALER socket before failing remaining requests._worker_diedto join the dead process, drain remaining replies, sendBaseResponse(success=False)failure responses to all pending clients, and immediately respawn a replacement worker at the same pool index.Macroscope summarized 767add9.