fix(infra): reap orphaned workers and survive partial teardown - #1585
fix(infra): reap orphaned workers and survive partial teardown#1585Le8r0nJames wants to merge 1 commit into
Conversation
Three failure paths left processes behind. The SGLang launcher died on SIGTERM without running its finally block, so cancelling a job orphaned the whole sglang child tree - the multiprocessing scheduler and detokenizer workers kept the GPUs busy until someone noticed. Turn SIGTERM and SIGINT into SystemExit so the existing kill_process_tree runs. Trainer close() assumed a fully constructed trainer and stopped at the first exception, so a failure in __init__ or in one engine's destroy() left the remaining workers alive. Look every component up defensively and keep going. Save and load block a worker for tens of seconds, during which its RPC server can briefly refuse connections; the default three-retry budget then declared a busy-but-alive worker dead. Widen the budget for those two ops only. Also consume AREAL_APPTAINER_STAGGER_SECONDS, which launch scripts had been exporting with nothing reading it: concurrent singularity mounts on one node race for the kernel's loop devices and intermittently fail to find one.
|
|
||
| try: | ||
| launch_sglang_server(argv) | ||
| except SystemExit: |
There was a problem hiding this comment.
except Exception cannot catch SystemExit, this is meaningless.
| # sglang child tree (its multiprocessing scheduler and detokenizer workers) | ||
| # and leaving the node occupied. Convert SIGTERM/SIGINT into SystemExit so | ||
| # the finally-block kill_process_tree below reaps the whole tree. | ||
| def _term_handler(signum, _frame): |
There was a problem hiding this comment.
kill_process_tree(graceful=True) will block in psutil.wait_procs for up to 5 seconds. A second SIGTERM within this window will re-enter _term_handler, raise SystemExit from finally, causing the cleanup to terminate prematurely. Use an idempotent flag + SIG_IGN for duplicate signals. Do not reset to SIG_DFL — that would kill the process while cleanup is in progress.
| if self.critic is not None: | ||
| self.critic.destroy() | ||
| self.actor.destroy() | ||
| # Must tolerate a partially-constructed trainer (called from |
There was a problem hiding this comment.
There is no call to self.close() in PPOTrainer.init. The only caller is exit, what is the triggering condition for the issue being fixed here?
| # __init__'s failure path), and one engine's destroy() failure must | ||
| # not keep the remaining workers alive. | ||
| saver = getattr(self, "saver", None) | ||
| if saver is not None: |
There was a problem hiding this comment.
All component failures—including saver.finalize()—are downgraded to logger.warning. The current task exits with 0, leaving only a warning.
|
|
||
|
|
||
| def main(argv): | ||
| # SIGTERM terminates Python without running finally blocks, orphaning the |
There was a problem hiding this comment.
The main thread is blocked within the list(server_iterator) inside with ThreadPoolExecutor throughout the model loading window. At this point, SIGTERM raises SystemExit in the main thread, triggering stack unwinding to Executor.__exit__ → shutdown(wait=True) → join all worker threads. These threads are stuck in wait_for_server, with timeout=None causing an infinite loop on RequestException. CPython 3.12 does not handle SystemExit specially: Executor.__exit__ and _python_exit(registered via threading._register_atexit) cannot abandon a running task, and the queue sentinel can only prevent new tasks from starting.
| # after the (larger) retry budget is exhausted, so this does not mask | ||
| # real crashes. | ||
| long_op = method in ("save", "load") | ||
| retry_kw = dict(max_retries=8, retry_delay=2.0) if long_op else {} |
There was a problem hiding this comment.
The _call_workers function distributes the save operation to all workers using asyncio.gather, while the engine's save internally performs an all-gather TP operation—a collective communication. The scheduler's retry loop will resend the same POST on TimeoutError, ClientConnectionError, HTTP 503, and HTTP 500s containing "timeout." Only "connection refused" indicates that the request wasn't delivered; the others don't. The Engine's save/load lacks request IDs or deduplication mechanisms. A replay causes a worker to enter all-gather twice, while its peer rank enters only once → mismatched collective communication → NCCL deadlock, plus repeated writes to the same checkpoint path.
2 participants
Description
Three failure paths left processes behind.
The SGLang launcher died on SIGTERM without running its
finallyblock, socancelling a job orphaned the whole sglang child tree - the multiprocessing
scheduler and detokenizer workers kept the GPUs busy until someone noticed.
SIGTERM and SIGINT now become
SystemExitso the existingkill_process_treeruns.
Trainer
close()assumed a fully constructed trainer and stopped at the firstexception, so a failure in
__init__or in one engine'sdestroy()left theremaining workers alive. Every component is now looked up defensively and one
failure no longer skips the rest.
saveandloadblock a worker for tens of seconds, during which its RPC servercan briefly refuse connections; the default three-retry budget then declared a
busy-but-alive worker dead. The budget is widened for those two ops only, so a
genuinely dead worker still fails after the larger budget is exhausted.
Also consumes
AREAL_APPTAINER_STAGGER_SECONDS, which launch scripts had beenexporting with nothing reading it: concurrent singularity mounts on one node race
for the kernel's loop devices and intermittently fail to find one.
Related Issue
N/A
Type of Change
Checklist
Additional Context
tests/test_worker_teardown_robustness.pysends a real SIGTERM to the launcherand asserts both the exit code and that the reaper ran, and checks that
close()on a bare trainer does not raise and that a failing component does not skip the
others. 4 tests.