feat: controller-rollout robustness + explicit StopCommand shutdown + UCXX teardown#696
Merged
lfengad merged 1 commit intoJun 13, 2026
Conversation
d607140 to
852edc1
Compare
23d5103 to
b8c93c2
Compare
6c4243c to
56fc881
Compare
lfengad
approved these changes
Jun 12, 2026
lfengad
left a comment
Collaborator
There was a problem hiding this comment.
overall LGTM. Seems also among PRs some contents are shared and may conflict? We might merge them carefully with that order?
2 tasks
Collaborator
Author
|
After #695 is merged, I will rebase this one. |
6 tasks
Separate training completion from shutdown so rollouts can drain cleanly. Defer TrainingComplete until policy replicas finish in-flight work, bounded-drain P2R/teardown streams, and kill full subprocess trees in GPU integration tests so controller/torchrun zombies do not poison later CI cases.
56fc881 to
5a59825
Compare
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.
Summary
Unifies multi-rank rollout shutdown and hardens teardown across the controller,
policy, and rollout paths. The root cause was a design coupling, not a missing
guard: the controller forced a final P2R→R2R weight sync purely as an "ending
signal", held open by a hardcoded
time.sleep(30)in the policy main loop —which raced rollouts that had already drained and NCCL-aborted, wedging
ncclCommAborton the P2R recipient. This branch also adds the Slurm full-CItooling and reworks the UCXX payload transport for correctness.
What changed
Controller / rollout shutdown (
fix: harden controller-rollout shutdown in CI)is_endpath:_get_batched_prompt_implforcesis_end=Trueoncetraining_finished()(non-validation), routing step-boundedruns through the same NCCL-free path as data-bounded runs. Starvation-safe:
every needed rollout is already buffered by training-finish.
pure
need_weight_synchelper; validation keeps it (final validation is drivenby that R2R broadcast).
time.sleep(30)(gated onvalidation.enable).StopCommandbroadcast once every policy replicaunregisters — reaches every rollout via
consume_command→handle_stopregardless of fetch state, unblocking a rank wedged on the
version_failgate;broadcast_object_cpugives race-free lockstep exit forworld_size>1.async_r2r_syncbarrier fixes: authoritative per-round participant count(shrinks as replicas finish, killing the 120s barrier timeout) + abortable
barrier wait (checks
_stopeach iteration).JobPhase(RUNNING → DRAINING) + zero-workTrainingCompleteCommandack so completion is separated from teardown._maybe_finalizelogs an error if thecontroller finalizes while validation is still active (covered by
test_finalize_logs_error_if_validation_is_still_active).bounded_drain_or_abort(WST + inferencestreams); rollout-aware controller checkout gate; UCXX teardown
drain→stop→reset ordering —
_drain_inflight_requestscancels in-flight UCPrequests with the progress thread alive so callbacks fire under a healthy
interpreter, fixing a GC-time use-after-free SIGSEGV.
and demoted high-volume counters to DEBUG.
Test plan
tests/test_multirank_shutdown.py— 58 tests (StopCommand round-trip/handler,weight-sync
status.endedexclusion,nccl_abort_all,bounded_drain_or_abort,rollout-checkout gate, R2R barrier world-size,
_maybe_finalizevalidation guard)tests/test_ucxx_transport.py::TestResetUCXXContext(drain→stop→reset order,cancel-API fallback)
tests/test_process_flow.py,test_custom_class.py,test_colocated.py,test_teacher_model.py(import-layout fix verified)test_deepep.pyCUDA(
layout.cu:138 'named symbol not found') infra failure