[TRTLLM-13409][feat] proxy fast-death detection + sticky EngineDeadError#15816
Merged
JunyiXu-nv merged 2 commits intoJul 10, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new ChangesEngine death fast-fail propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MPIWorker
participant GenerationExecutorProxy
participant GenerationResult
participant Caller
MPIWorker->>GenerationExecutorProxy: future completion with exception
GenerationExecutorProxy->>GenerationExecutorProxy: _handle_worker_death(error)
GenerationExecutorProxy->>GenerationExecutorProxy: _mark_engine_dead(error), set _engine_dead
GenerationExecutorProxy->>GenerationResult: enqueue EngineDeadError into pending queues
Caller->>GenerationResult: result() / aresult()
GenerationResult->>GenerationResult: _result_step/_aresult_step detects EngineDeadError
GenerationResult-->>Caller: raise EngineDeadError, mark _done
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/executor/proxy.py (1)
442-447: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle cancelled and non-exception worker exits in the done-callback.
This callback only fast-fails when
future.exception()is non-None. That misses the same exit modes_check_mpi_futures()already treats as fatal: a cancelled future will raiseCancelledErrorhere, and a done future with no exception falls back to the slow 5s monitor path instead of propagating immediately.Suggested fix
- def mpi_done_callback(future: concurrent.futures.Future): + def mpi_done_callback(future: concurrent.futures.Future) -> None: # This is called when the MPI worker is done, so future.exception() # will not block. - if future.exception() is not None: - if self_ := self_ref(): - self_._handle_worker_death(future.exception()) + if self_ := self_ref(): + exc = None if future.cancelled() else future.exception() + self_._handle_worker_death( + exc or RuntimeError("MPI worker exited unexpectedly"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/executor/proxy.py` around lines 442 - 447, The mpi_done_callback in proxy.py only handles future.exception() when it is non-None, so cancelled futures and other non-exception worker exits are not fast-failed. Update mpi_done_callback to mirror the fatal exit handling in _check_mpi_futures(), using the same logic around Future state (including cancelled/done-without-exception cases) and calling _handle_worker_death immediately for those paths. Keep the self_ref() guard, and make sure the callback does not defer these cases to the slower monitor loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/executor/result.py`:
- Around line 995-1011: Persist the terminal EngineDeadError on the result
object instead of losing it after the first raise. Update Result._result_step
and Result._aresult_step to store the caught EngineDeadError in a terminal error
field before marking _done, then make the post-completion accessors (including
result(), aresult(), and _exception()) in result.py re-raise or return that
stored _terminal_error whenever _done is set. This should keep the engine-death
failure observable on subsequent calls rather than falling back to a
successful-looking self/None state.
In `@tests/unittest/executor/test_proxy_fast_death.py`:
- Around line 123-142: Coverage is insufficient in
tests/unittest/executor/test_proxy_fast_death.py, and the final assert on
GenerationResult.result() is locking in the wrong contract after an
EngineDeadError. Update test_result_step_marks_done_before_raising so repeated
result() and aresult() calls, plus _exception(), continue to surface the same
EngineDeadError instead of returning self. Use GenerationResult, _result_step,
result, aresult, and _exception to locate the behavior and extend this file with
assertions covering the repeated-access fast-fail path.
---
Outside diff comments:
In `@tensorrt_llm/executor/proxy.py`:
- Around line 442-447: The mpi_done_callback in proxy.py only handles
future.exception() when it is non-None, so cancelled futures and other
non-exception worker exits are not fast-failed. Update mpi_done_callback to
mirror the fatal exit handling in _check_mpi_futures(), using the same logic
around Future state (including cancelled/done-without-exception cases) and
calling _handle_worker_death immediately for those paths. Keep the self_ref()
guard, and make sure the callback does not defer these cases to the slower
monitor loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d3e695ad-6f10-4ace-a7fa-14b309c94fd5
📒 Files selected for processing (6)
tensorrt_llm/executor/__init__.pytensorrt_llm/executor/proxy.pytensorrt_llm/executor/result.pytensorrt_llm/executor/utils.pytests/integration/test_lists/test-db/l0_a10.ymltests/unittest/executor/test_proxy_fast_death.py
When an executor worker dies, pending GenerationResults block forever on queue.get(): the producer is gone, so nothing is ever enqueued. Detection was also only observed on a multi-second error-monitor poll. Requests then hang until a watchdog/pod-kill. Add a sticky engine-dead path modeled on vLLM's EngineDeadError: - New EngineDeadError (tensorrt_llm.executor.EngineDeadError), engine-level (not a per-request RequestError, so it is treated as fatal). - Proxy overrides _set_fatal_error to also _mark_engine_dead: set a sticky flag and enqueue EngineDeadError onto every pending result so blocked result()/aresult() unblock and raise immediately. Uses queue.put() so the async _SyncQueue path notifies the event loop, not just the sync Queue. - result.py raises EngineDeadError when it is dequeued in _result_step / _aresult_step, and records it as a sticky terminal error (marking the result done) so subsequent result()/aresult()/_exception()/iteration keep surfacing the failure instead of re-blocking on the empty queue or looking successful. - submit() fast-fails new requests once the engine is dead, and re-checks the sticky flag after registering the result to close the submit-vs-death race where a result created just as the engine dies could be missed by the _mark_engine_dead sweep and hang forever. - Propagation is event-driven: the MPI future done-callback now calls _handle_worker_death, which broadcasts EngineDeadError to pending requests the instant a worker exits, rather than waiting for the next monitor-loop poll. The error-monitor poll stays coarse (5s) as a backstop that records the fatal error and drives pre_shutdown; it no longer gates propagation latency. Adds CPU-only unit tests for the exception, the broadcast (incl. idempotency), the event-driven _handle_worker_death path, the sync/async fast-fail steps and their done-marking, submit() fast-fail, and the submit-vs-death race re-check; registers the test in the l0_a10 CI list. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
JunyiXu-nv
force-pushed
the
dev-junyix-feat-proxy-fast-death
branch
from
July 1, 2026 09:11
d588f62 to
5fba86c
Compare
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #56902 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #56902 [ run ] completed with state
|
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #57088 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #57088 [ run ] completed with state
|
Collaborator
Author
|
/bot run |
1 similar comment
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #57519 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #57519 [ run ] completed with state
|
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #57565 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #57565 [ run ] completed with state |
QiJune
reviewed
Jul 7, 2026
… fast-death path Review follow-up: RemoteMpiCommSessionClient.submit() returns no futures, so under TLLM_SPAWN_PROXY_PROCESS=1 / trtllm-llmapi-launch the proxy's mpi_done_callback never fires and _check_mpi_futures() has nothing to watch; on the server side the exception-forwarding callback was registered only for sync tasks, while worker_main is submitted fire-and-forget. A crashed remote worker was therefore invisible to the proxy and pending GenerationResults could still block indefinitely in this deployment mode. Close the channel end to end: - mpi_session: new RemoteWorkerDeath (string-carried, since arbitrary exceptions may not pickle). RemoteMpiCommSessionServer now registers mpi_async_error_callback on fire-and-forget futures and forwards worker exceptions to the client over the existing control socket (same cross-thread ZMQ-put pattern as the pre-existing sync callback). - RemoteMpiCommSessionClient.check_worker_error(): non-blocking scan for a forwarded RemoteWorkerDeath; non-error messages encountered are buffered and served by poll() so submit_sync semantics are preserved. - proxy: _check_remote_worker_death() in the error-monitor loop feeds the forwarded exception into the same _handle_worker_death() + _set_fatal_error fast-death path, so this mode gets identical sticky-EngineDeadError behavior (bounded by the monitor poll interval; sessions without the hook are a no-op). Adds CPU-only unit tests: RemoteWorkerDeath round-trip, server callback forwards only failures (not results/cancellations), client scan + buffering, and the proxy end-to-end path incl. the no-op case for MpiPoolSession. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #58400 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #58400 [ run ] completed with state
|
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #58556 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #58556 [ run ] completed with state |
JunyiXu-nv
added a commit
to JunyiXu-nv/TensorRT-LLM
that referenced
this pull request
Jul 15, 2026
…is dead PR NVIDIA#16338 lets the proxy detect workers killed abruptly (MPI_Abort from the PyExecutor HangDetector, SIGKILL, the OOM killer) via OS process monitoring, and PR NVIDIA#15816 unblocks pending requests with a sticky EngineDeadError. But teardown still hangs: shutdown() blocks forever on f.result() for mpi4py futures that never complete after an abrupt kill, then on the result-dispatcher join (the worker that would send the shutdown sentinel is dead), then on joining the dead MPI pool. In CI the detection therefore only moves the client-side hang from generate() into LLM.__exit__, and hang-killed stages still burn pytest's full --timeout=3600 after the GPUs were freed at +300s (e.g. L0_Test-SBSA-Multi-GPU run 2744 GB300-4_GPUs-PyTorch-Post-Merge-2/3 on a post-NVIDIA#15816 main commit). Once the engine is known dead (_engine_dead, set by all detection paths): - give the futures one short collective grace instead of unbounded per-future result() waits, and skip the ones still pending - bound the result-dispatcher join (daemon thread; leaked, not awaited) - shut the owned MPI session down without waiting on the dead pool Orderly-shutdown semantics are unchanged when the engine is alive. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
JunyiXu-nv
added a commit
to JunyiXu-nv/TensorRT-LLM
that referenced
this pull request
Jul 15, 2026
…is dead PR NVIDIA#16338 lets the proxy detect workers killed abruptly (MPI_Abort from the PyExecutor HangDetector, SIGKILL, the OOM killer) via OS process monitoring, and PR NVIDIA#15816 unblocks pending requests with a sticky EngineDeadError. But teardown still hangs: shutdown() blocks forever on f.result() for mpi4py futures that never complete after an abrupt kill, then on the result-dispatcher join (the worker that would send the shutdown sentinel is dead), then on joining the dead MPI pool. In CI the detection therefore only moves the client-side hang from generate() into LLM.__exit__, and hang-killed stages still burn pytest's full --timeout=3600 after the GPUs were freed at +300s (e.g. L0_Test-SBSA-Multi-GPU run 2744 GB300-4_GPUs-PyTorch-Post-Merge-2/3 on a post-NVIDIA#15816 main commit). Once the engine is known dead (_engine_dead, set by all detection paths): - give the futures one short collective grace instead of unbounded per-future result() waits, and skip the ones still pending - bound the result-dispatcher join (daemon thread; leaked, not awaited) - shut the owned MPI session down without waiting on the dead pool Orderly-shutdown semantics are unchanged when the engine is alive. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
JunyiXu-nv
added a commit
to JunyiXu-nv/TensorRT-LLM
that referenced
this pull request
Jul 16, 2026
…is dead PR NVIDIA#16338 lets the proxy detect workers killed abruptly (MPI_Abort from the PyExecutor HangDetector, SIGKILL, the OOM killer) via OS process monitoring, and PR NVIDIA#15816 unblocks pending requests with a sticky EngineDeadError. But teardown still hangs: shutdown() blocks forever on f.result() for mpi4py futures that never complete after an abrupt kill, then on the result-dispatcher join (the worker that would send the shutdown sentinel is dead), then on joining the dead MPI pool. In CI the detection therefore only moves the client-side hang from generate() into LLM.__exit__, and hang-killed stages still burn pytest's full --timeout=3600 after the GPUs were freed at +300s (e.g. L0_Test-SBSA-Multi-GPU run 2744 GB300-4_GPUs-PyTorch-Post-Merge-2/3 on a post-NVIDIA#15816 main commit). Once the engine is known dead (_engine_dead, set by all detection paths): - give the futures one short collective grace instead of unbounded per-future result() waits, and skip the ones still pending - bound the result-dispatcher join (daemon thread; leaked, not awaited) - shut the owned MPI session down without waiting on the dead pool Orderly-shutdown semantics are unchanged when the engine is alive. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
JunyiXu-nv
added a commit
to JunyiXu-nv/TensorRT-LLM
that referenced
this pull request
Jul 16, 2026
…is dead PR NVIDIA#16338 lets the proxy detect workers killed abruptly (MPI_Abort from the PyExecutor HangDetector, SIGKILL, the OOM killer) via OS process monitoring, and PR NVIDIA#15816 unblocks pending requests with a sticky EngineDeadError. But teardown still hangs: shutdown() blocks forever on f.result() for mpi4py futures that never complete after an abrupt kill, then on the result-dispatcher join (the worker that would send the shutdown sentinel is dead), then on joining the dead MPI pool. In CI the detection therefore only moves the client-side hang from generate() into LLM.__exit__, and hang-killed stages still burn pytest's full --timeout=3600 after the GPUs were freed at +300s (e.g. L0_Test-SBSA-Multi-GPU run 2744 GB300-4_GPUs-PyTorch-Post-Merge-2/3 on a post-NVIDIA#15816 main commit). Once the engine is known dead (_engine_dead, set by all detection paths): - give the futures one short collective grace instead of unbounded per-future result() waits, and skip the ones still pending - bound the result-dispatcher join (daemon thread; leaked, not awaited) - shut the owned MPI session down without waiting on the dead pool Orderly-shutdown semantics are unchanged when the engine is alive. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
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
3 participants
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 by CodeRabbit
New Features
Bug Fixes
Tests
Description
When an executor worker dies, pending GenerationResults block forever on queue.get(): the producer is gone, so nothing is ever enqueued, and the proxy's error monitor only polled every 5s. Requests then hang until a watchdog/pod-kill.
Add a sticky engine-dead path modeled on vLLM's EngineDeadError:
Test Coverage
Adds CPU-only unit tests for the exception, the broadcast (incl. idempotency), the sync/async fast-fail steps and their done-marking, submit() fast-fail, and the submit-vs-death race re-check; registers the test in the l0_a10 CI list.
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.