Repository navigation
fix(core): isolate server signals and drain agent sessions on shutdown - #5141
Conversation
Signed-off-by: RyaliNvidia <ryali@nvidia.com>
Signed-off-by: RyaliNvidia <ryali@nvidia.com>
yaoyu-33
left a comment
There was a problem hiding this comment.
LGTM on the reviewed implementation; 104 focused tests passed. Please resolve the main conflict and run CI before merging.
Signed-off-by: RyaliNvidia <ryali@nvidia.com>
|
/ok to test af02e78 |
Signed-off-by: RyaliNvidia <ryali@nvidia.com>
|
/ok to test 8327eae |
|
🌿 Preview your docs: https://nvidia-preview-pr-5141-247b02f8662c.docs.buildwithfern.com/nemo/gym |
Signed-off-by: RyaliNvidia <ryali@nvidia.com>
|
/ok to test 1071096 |
Signed-off-by: RyaliNvidia <ryali@nvidia.com>
yaoyu-33
left a comment
There was a problem hiding this comment.
Reapproved after checking the handled-cancellation fix and main integration. 187 focused session, activation, process-shutdown, and setup tests passed, including all six cancellation regressions. No new blocking issue found. Latest-head required CI is still outstanding; Nooa integration was not rerun locally.
|
/ok to test 247b02f |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/41517538815 |
Brings in main up to 94545e7 (#5141, #5384, #5144, #4615, #4954, #5001, #5175). Textual conflict: nemo_gym/cli/env.py, imports only. This branch imports PIPE from subprocess (the server start error capture); main imports SIGKILL from signal (#5141, the shutdown drain). Both are kept: "from signal import SIGINT, SIGKILL" and "from subprocess import PIPE, Popen, TimeoutExpired". Semantic conflict: #5141 starts each server in its own session (run_command(..., start_new_session=True)) so shutdown can signal its process group, while this branch starts servers through RunHelper._spawn_server, which keeps the tail of the error stream. The merged call passed start_new_session to _spawn_server, which did not accept it (11 test_cli failures). _spawn_server now takes start_new_session and forwards it to run_command, so both behaviours hold. The dry-run path keeps this branch's process.wait(): the tail thread of _spawn_server owns the piped stream, so main's communicate() would compete with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Adam Rajfer <arajfer@nvidia.com>
…-from-saved-evidence Brings #5072's merge of main up to 94545e7 (#5141 and six more), item 97 and the Pool agent test adjustment into this branch. Conflicts, all additive (each side added its own setting next to the other's): - nemo_gym/cli/env.py, RunHelper.__init__: this branch reads the head server spin-up timeout, main (#5141) reads the server shutdown timeout. Both lines are kept. - nemo_gym/config/global_config.py, the constants: this branch's DEFAULT_SERVER_SPINUP_TIMEOUT_SECONDS and main's SERVER_SHUTDOWN_TIMEOUT_SECONDS_KEY_NAME are both kept. - nemo_gym/config/global_config.py, the setdefaults: the spin-up default keeps this branch's named constant (main wrote the same 600 as a literal), and main's shutdown default of 1 s is added. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Adam Rajfer <arajfer@nvidia.com>
The base branch moved from 7f474e1 to b8a89a0: it took the recovery stack's head 2b (edc0fb0: main through #5141, the judge-only settle compatibility fix, recovery and export changes). This merge brings it in; the author's commits are untouched. No conflicts. Refreshed by Adam's agent on the owner's order. Signed-off-by: Adam Rajfer <arajfer@nvidia.com> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…the worker that launches it Ray blocks SIGINT in every thread of its worker processes, and a process starts with the signal mask of the thread that launched it. Under NeMo RL, whose Gym actor runs RunHelper, every Gym server therefore starts with SIGINT blocked, through its launch shell and log tee. The interrupt that RunHelper sends each server's process group at shutdown (#5141) stays pending, uvicorn's handler never runs, and no server runs its own shutdown. In a NeMo RL training run that ended normally (Ray 2.56.1), no Gym server logged uvicorn's shutdown line, and a resources server went on serving after the interrupt. Each server now unblocks SIGINT and SIGTERM first thing in run_webserver. Tests that fail on the parent: run_webserver starts uvicorn with SIGINT and SIGTERM unblocked when launched from a thread that blocks SIGINT; and a stand-in server launched from such a thread, through RunHelper's launch shell and log tee, starts with SIGINT blocked yet runs its own shutdown on RunHelper's interrupt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Daniele Molinari <dmolinari@nvidia.com>
What does this PR do?
Isolate managed servers in their own process sessions so an interrupt reaches Uvicorn once and permits lifespan cleanup. Keep the logging tee alive, wait within one shared configurable grace period, signal all surviving process groups with SIGKILL, then reap within a separate shared bound. Descendants remain owned even after their wrapper exits.
Drain agent sessions concurrently on FastAPI shutdown through the same close-by-ID operation as HTTP close. Preserve partial canceled seeds, failed-seed cleanup, shielded continuation closes, checkpoint state and retry receipts. Unconfirmed cleanup retains handles and reports session IDs.
Incorporates main
3bf99705a905f9aefb18b03875a003189ff1edbf, retaining its capability/stop routes, cancellation behavior, MCP timeout settings and rollout-claim ownership. The extracted close operation releases main's rollout claim while preserving new-versus-cached receipt cookie behavior.Part of #3160; complements #3735. This does not close the broader lifecycle issue.
Configuration and compatibility
server_shutdown_timeout_seconds: shared finite nonnegative server grace, default 1 second; zero requests immediate escalation. Reaping has a separate shared bound. Telemetry, sidecar and head-server teardown remain outside this grace.session_close_timeout_seconds: base agent close timeout and total shutdown-drain budget, default 60 seconds. Existing Hermes/Stirrup 30-second overrides remain.Cleanup entered after a caller has already caught cancellation now completes normally. Each close/release records the task's cancellation count at entry and rejects only a newly requested cancellation. This preserves bounded shutdown and cancellation-suppression protection while allowing Nooa's caller-cancellation cleanup to close both Agent and Resources sessions. No Nooa test expectations were changed.
Validation
051472343211914222e24ce36d8752f4e86bbe43. A same-commit archive was used after Git fetching stalled; temporary setup constraints allowed Nooa's declared Rich minimum while preserving the model SDK pins, including OpenAI 2.44.0. No repository dependency policy or Nooa test assertions changed.pre-commit run --all-files, Ruff/format and whitespace checks passed. The private guardwords scan was skipped because its catalog was unavailable.Live model/sandbox rollout: N/A for this lifecycle correction; checks use synthetic fixtures and local subprocesses. No live provider or remote sandbox qualification is claimed. No dependency pins, timeout settings, shields, or conformance expectations were changed. No
uncancel()calls were added. Commits include DCO sign-off. No comment replies or trust-approval comments were posted.Proposed maintainer labels:
bug,area:orchestration,needs-review.