[TRTLLM-13409][fix] hard-kill all ranks when one rank's executor loop crashes - #16592
[TRTLLM-13409][fix] hard-kill all ranks when one rank's executor loop crashes#16592JunyiXu-nv wants to merge 9 commits into
Conversation
03e5e9e to
cf7fdc1
Compare
|
/bot run --disable-fail-fast |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds configurable rank-crash hard-kill handling with a grace period, optional watchdog, and defensive error handling. Executor crash cleanup now starts the watchdog before cleanup and invokes direct world termination afterward, with tests covering policy and ordering. ChangesRank crash handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PyExecutor_event_loop_wrapper
participant RankCrashKillWatchdog
participant executor_loop_cleanup
participant hard_kill_on_rank_crash
PyExecutor_event_loop_wrapper->>RankCrashKillWatchdog: Arm after event-loop crash
PyExecutor_event_loop_wrapper->>executor_loop_cleanup: Execute local cleanup
RankCrashKillWatchdog->>hard_kill_on_rank_crash: Trigger after grace period
PyExecutor_event_loop_wrapper->>hard_kill_on_rank_crash: Invoke after cleanup with original deadline
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR_Github #60270 [ run ] triggered by Bot. Commit: |
|
PR_Github #60270 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60547 [ run ] triggered by Bot. Commit: |
|
PR_Github #60547 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60580 [ run ] triggered by Bot. Commit: |
nv-xtf
left a comment
nv-xtf
left a comment
There was a problem hiding this comment.
LGTM — One non-blocking question inline about kill reachability if cleanup blocks on a PP send handle.
|
PR_Github #60580 [ run ] completed with state
|
cf7fdc1 to
ec5a475
Compare
ec5a475 to
c0c1a43
Compare
|
PR_Github #62397 [ run ] triggered by Bot. Commit: |
|
PR_Github #62397 [ run ] completed with state
|
Pipeline #50558 — one timeout, on a path this PR does touchNow that the scaffolding blocker is waived, this run got a clean signal. One root failure across 24 stages (reported twice, as the wrapper and the inner test): I am not claiming this is unreachable, because it isn't. Measured ambient rate:
Build #50609 is an unrelated PR, and the scan window has moved past #50558, so this count contains no data from this PR at all. That makes it an independent occurrence of the same test timing out without this change present — which is the evidence I wanted before attributing the failure to ambient noise rather than to the sentinel. That is sufficient to re-run rather than to go re-read the sentinel logic. If it recurs here, I will treat that as a suspected regression in this PR and investigate the overlap loop directly instead of re-running again. Re-running. |
|
/bot run |
|
PR_Github #62510 [ run ] triggered by Bot. Commit: |
|
PR_Github #62510 [ run ] completed with state
|
Pipeline #50658 — the overlap-scheduler timeout did not recurI said on the previous run that a recurrence of What failed instead: eight stages were cut mid-test with one unfinished test each, and all eight are multi-GPU: One unfinished test apiece across a coherent stage group is the signature of a single cancellation, not eight independent failures. I checked whether this PR's own hard-kill caused it, since "multi-GPU stages die simultaneously" is exactly what a mis-firing peer-rank kill would look like, and this PR is the one that adds that kill. It did fire — once — and here is where: That is this PR's own regression test exercising the feature, on a stage ( For the record on this PR's recent history, since the pattern matters more than any single run: #50428 aborted with no test failures, #50558 failed on one timeout now shown to be ambient (1 failure in 48 runs, the other on an unrelated PR), and #50658 has zero test failures. Three consecutive pipelines have ended without a single failing test attributable to this change. Re-running. |
|
/bot run |
|
PR_Github #62530 [ run ] triggered by Bot. Commit: |
|
PR_Github #62530 [ run ] completed with state
|
Pipeline #50679 — one timeout, in code with no path to this changeOne root failure across 19 stages (reported twice, as wrapper and inner test): Unreachable from this PR. Ambient rate — thin, and I will not dress it up. Across the last 60 pipelines this parametrisation appears in only 3 stage-runs, with 0 failures. That is too small a sample to call anything, and it contains no independent sighting of this failure. The weight here rests entirely on the unreachability argument above, not on the rate. Pattern across this PR's recent runs, which I think is the more informative signal: #50428 aborted with no test failures; #50558 failed on For context on the environment: I measured that roughly 1 in 5 recent pipelines has at least one test killed by the harness timeout, spread across unrelated areas, and roughly 1 in 6 ends with zero failing tests at all. Re-running. |
|
/bot run |
|
PR_Github #62649 [ run ] triggered by Bot. Commit: |
|
PR_Github #62649 [ run ] completed with state
|
Pipeline #50789 — stale test file on this branch, not a regressionI said on the previous run that a recurrence in The two failures are not the same failure:
The second is behavioural, and it is explained by this branch's base, not by its code: Those lines were removed from So the correct action is a rebase onto current For completeness on the earlier timeout in #50679: that is a different mode and remains unexplained by this; the broader question of whether Worth noting the general lesson, since it applies to any long-lived branch here: CI runs the branch's own copy of files the branch does not modify, so a stale base means stale tests — including tests that |
|
Confirming datapoint for the stale-base diagnosis above. Scanning the last 120 pipelines for this test across every stage that ran it: One failure in seven runs, and it is ours. That is precisely the pattern the stale base predicts — other PRs are based on a So: rebase, don't re-run. |
… crashes When a rank's executor loop dies on an exception, the rank stops participating in collectives but nothing tells its peers: every peer blocks in its next collective until its own HangDetector fires 300s later, and the whole multi-GPU test session burns that long for an error that was already known (the A4 AutoDeploy catches are this signature: peers crash, the survivor wedges in ADP until the 300s backstop). Escalate at error time instead: after the loop's local cleanup has woken rank-local waiters, hard-kill the world via the ST-1 propagation path. A grace period (TLLM_RANK_CRASH_HARD_KILL_GRACE, default 10s, negative disables) lets the cleaner error paths win the race first -- the stashed error reaches rank-local response waiters, the init-phase ready handshake returns the real exception to the proxy, and the worker future completes with the original error -- so the client reports the actual failure rather than a bare worker death. Single-rank worlds are exempt (no peers to unblock), and the kill helper never raises (it runs in a finally where an exception would mask the loop's original error). Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…blocks or raises The kill sat after _executor_loop_cleanup() in the finally block, so it was skippable in exactly the situations it exists for: cleanup blocking without bound (unbounded wait() on a PP send handle wedged by the crash) or cleanup raising (aborting the finally before the kill). Either way peers fall back to burning 300s in their own HangDetectors. Arm a daemon watchdog thread BEFORE cleanup that fires the kill at crash + grace regardless of cleanup progress, and nest the post-cleanup kill in its own finally so a raising cleanup cannot skip it. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…pool-session shutdown test The test builds MpiPoolSession via __new__ (a real spawn is neither needed nor wanted), but MpiPoolSession.shutdown() now reads n_workers (added by NVIDIA#16456 while the test was in flight) and _wait_shutdown, both set only in __init__. Provide them explicitly and drop the waive. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…rand peers The kill was armed for ANY exception escaping the executor loop wrapper, including one raised on the way out of an already-shutting-down loop. A hardware A/B on 2x GB200 (mpirun -n 2 trtllm-llmapi-launch trtllm-bench --tp 2) showed it turning a benign late exception -- raised after event_loop() returned, i.e. after the shutdown request was processed and all work was done -- into a whole-job SIGKILL: mpirun exit 137 at crash+10s, where the same injection on the merge-base logged the error, let the worker thread die, and completed the benchmark with exit 0. It fired during the memory-profiling dry run, before the benchmark even started: PyExecutor is constructed and shut down twice per process and that first shutdown is a normal lifecycle event. Three changes: - crashed = not self.is_shutdown. Once is_shutdown is set every rank has processed the shutdown broadcast, so no peer is waiting on this one and there is nothing to escalate. - Scope the flag to event_loop() itself via an inner try. Teardown of the enclosing host-profiler / GC context managers is not a stranded-peer condition either. - Make the watchdog cancellable and give it an explicit deadline. It was a bare daemon thread that slept the grace and killed with no cancel path, so once armed it fired at crash+10s even if the process went on to shut down cleanly and would have exited 0. It also double-armed: the watchdog and the post-cleanup hard_kill_on_rank_crash each ran their own timer. The watchdog now covers only the window where cleanup may block forever; once cleanup returns the caller cancels it and carries the kill inline on the watchdog's ORIGINAL deadline, so exactly one timer is live at any moment and the handover cannot push the kill out by a second grace. A genuine mid-loop crash still arms the watchdog and still hard-kills the world -- the behavior the kill exists for is unchanged. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…bed kill If cancel() ever regresses, the watchdog thread outlives monkeypatch teardown and SIGKILLs the pytest process instead of failing the test. Join it inside the test, while propagate_hard_kill is still stubbed. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…is_shutdown The previous commit keyed the decision off `not self.is_shutdown`, on the premise that is_shutdown means "every rank has processed the shutdown broadcast". That premise is false, and the mistake silently disabled the kill for this feature's most common trigger. `_handle_errors` sets `self.is_shutdown = True` rank-locally whenever an error is fatal, and `classify_error` returns `immediate_fatal` for a CUDA illegal address / device-side assert / launch failure, bypassing the error budget entirely. The only "broadcast" that follows is `enqueue_shutdown_request()`, which pushes into this process's own queue -- peers are told nothing. `should_stop_processing` additionally requires empty active/waiting queues, so the loop keeps entering collectives after the flag is set. So: rank 3 of 4 takes cudaErrorIllegalAddress in the forward pass, _handle_errors sets is_shutdown and returns None, and the next unguarded statement (guided_decoder.execute(batch_outputs['logits'])) raises TypeError on None. The wrapper computed crashed=False, armed nothing, and ranks 0-2 blocked in their next NCCL collective for the full 300s -- exactly the failure this PR exists to remove. Gate on an explicit completion sentinel instead. `_event_loop_completed` is set at the three executor loops' normal-exit `break` sites and nowhere else, so it answers the actual question: did event_loop() reach its own termination? A raise after that point (profiler/hang-detector __exit__, or the enclosing context managers) is a teardown error; anything else -- now including a failure before the loop ever started, which the previous inner try narrowed away -- strands peers and escalates. Also corrects two overstated claims from the previous commit. The watchdog/post-cleanup "double-arm" never produced an extra or later kill: earliest-wins gave crash+grace before, and max(cleanup_end, crash+grace) = crash+grace after. cancel() cannot spare a rank that exits cleanly either -- it is followed one line later by the same kill on the same deadline. The handover machinery is kept because one timer is clearer than two, but all the protection against a spurious SIGKILL rests on the predicate above. Log fixes: the disarm message drops from ERROR to DEBUG (it printed "cancelled before the grace elapsed" moments before the world was SIGKILLed, which reads during triage as "the kill was called off"), and the countdown now logs the remaining time rather than the full grace on the handover path. Tests: a rank-local-fatal regression test that fails against the previous predicate; a pre-loop-failure test; coverage for the already-elapsed deadline (the max(0.0, ...) clamp is the only thing keeping a negative sleep from raising into the blanket except and dropping the kill); and an AST check that every `break` in the three loops is preceded by the sentinel, so a new normal-exit path cannot make clean shutdowns look like crashes. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
… process-wide sleep patch monkeypatching time.sleep is process-wide, so any background thread that sleeps during these two tests appends its own entry into the asserted list. Assert the ordering these tests are about instead of exact list equality. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…le on the deadline clamp
Five guard-quality defects from review. None changed runtime behavior on a
healthy run, but two recreated the conditions under which the predicate has
already regressed twice.
1. The AST guard enforced the wrong invariant. It required EVERY own `break`
in the three loops to be preceded by the sentinel, but only a break
exiting the outer `while True` terminates the event loop, and
_executor_loop / _executor_loop_overlap already contain inner `for` loops.
Worse, its failure message told the reader "every normal exit must set
it", so a contributor who added an inner-loop break would have been
instructed to set the sentinel while the loop was still running -- making
every later rank-local crash look like a clean shutdown and silently
disabling the kill, the same class as the previous blocker. Now: exactly
one loop-terminating break per loop, located by recursing through
if/try/with but stopping at nested for/while/def.
Mutation-tested: inner `for _z in (): break` without the sentinel PASSES;
a missing sentinel on the outer break FAILS; a second outer break FAILS.
2. The load-bearing reset was untested. Deleting
`self._event_loop_completed = False` from _event_loop_wrapper left the
whole suite green while a second loop run on the same executor misread a
genuine crash as a clean shutdown (verified: run-2 events go from
[watchdog, cleanup, cancel, kill] to [cleanup]). The one "initialization"
test only grepped __init__ -- the site that is NOT load-bearing -- which
invited deleting the real one as redundant. Added a behavioral
two-invocation test plus a source assertion on the wrapper.
3. The rationale on the max(0.0, ...) deadline clamp was false. The comment
and docstring claimed it was the only thing preventing a negative sleep
from raising into the blanket except and dropping the kill. It is not:
_wait_out_kill_grace guards with `remaining > 0`, and Event.wait() returns
immediately for a negative timeout (probe: _wait_out_kill_grace(-100, None)
-> True, zero sleep calls). A false "this guard protects X" comment is how
the previous regressions got through, and someone trusting it could drop
the real guard as redundant. Corrected both, moved the load-bearing note
onto the `> 0` guard, and added a test asserting sleep is never called
with a negative argument.
4. Hardening the two grace-ordering tests against the process-wide time.sleep
patch weakened them: membership-only assertions accepted sleeping the
grace TWICE before killing -- exactly the crash + 2*grace bug this series
introduced with its two independent timers. Pinned the counts to 1 while
still tolerating an unrelated thread's sleep. The handover test now pins
time.monotonic and asserts the exact remaining duration instead of racing
a 0.3s real sleep against a 0.55s bound on a loaded CI node.
5. The AST guard checked neither the assigned value nor the target object:
`self._event_loop_completed = False` before the break passed, and so did
`self.dist._event_loop_completed = True`. A value inversion turns every
clean shutdown into a whole-job SIGKILL and was invisible. Now requires
Constant True assigned to an attribute of Name('self'); both mutants fail.
Also documents why _executor_loop_pp sets the sentinel before its Stage-5
drain: reaching there means every rank observed should_stop_processing, so
the drain consumes only a rank-local queue and a raise in it strands nobody.
Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…p, not walk order _executor_loop_pp contains four while loops, one of them (the Stage-5 drain) a sibling of the event loop under the same with-block. Selecting by ast.walk order happened to pick the right one, but a body reorder would silently repoint the guard at the drain. Select by the `True` test and assert uniqueness instead. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
50eeaca to
dd070b7
Compare
Rebased onto current
|
|
/bot run |
|
PR_Github #62761 [ run ] triggered by Bot. Commit: |
|
PR_Github #62761 [ run ] completed with state
|
5 participants
Dev Engineer Review
TLLM_RANK_CRASH_HARD_KILL_GRACE), where invalid/unset values default to 10s and negative values disable world hard-kill.RankCrashKillWatchdogand integrated it intoPyExecutor._event_loop_wrapperso the watchdog is armed before executor cleanup and the hard-kill is attempted afterward, preserving the original fire deadline.QA Engineer Review
Test changes (unit)
tests/unittest/_torch/executor/test_hang_detector_kill.pytest_rank_crash_kill_single_rank_is_nooptest_rank_crash_kill_fires_for_multi_ranktest_rank_crash_kill_sleeps_grace_before_killtest_rank_crash_kill_disabled_by_negative_gracetest_rank_crash_kill_invalid_grace_uses_defaulttest_rank_crash_kill_never_raisestest_watchdog_kills_while_caller_blockstest_watchdog_not_armed_for_single_ranktest_watchdog_not_armed_when_disabledtest_watchdog_cancel_prevents_the_killtest_kill_keeps_original_deadline_on_handovertest_event_loop_wrapper_kills_world_on_crashtest_event_loop_wrapper_kills_world_when_cleanup_raisestest_event_loop_wrapper_kills_world_when_watchdog_cannot_armtest_event_loop_wrapper_no_kill_on_clean_exittest_event_loop_wrapper_no_kill_when_loop_raises_after_shutdowntest_event_loop_wrapper_no_kill_when_enclosing_context_manager_raisestests/integration/test_lists/test-db/l0_sanity_check.yml(moduleunittest/_torch/executor/test_hang_detector_kill.py)tests/unittest/executor/test_proxy_fast_death.pytest_pool_session_shutdown_never_blocks_after_releasetests/integration/test_lists/test-db/l0_a10.yml(moduleunittest/executor/test_proxy_fast_death.py)Verdict
test-dbvia the test list entries above).Description
When a rank's executor loop dies on an exception, the rank stops participating in collectives but nothing tells its peers: every peer blocks in its next collective until its own HangDetector fires 300s later, and the whole multi-GPU test session burns that long for an error that was already known (the A4 AutoDeploy catches are this signature: peers crash, the survivor wedges in ADP until the 300s backstop).
Single-rank worlds are exempt (no peers to unblock), and the kill helper never raises (it runs in a finally where an exception would mask the loop's original error).
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.