[TRTLLM-13409][feat] anti-zombie worker cleanup (PR_SET_PDEATHSIG + tree-kill) - #16404
[TRTLLM-13409][feat] anti-zombie worker cleanup (PR_SET_PDEATHSIG + tree-kill)#16404JunyiXu-nv wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughAdds Linux process lifecycle utilities for parent-death signaling and recursive process-tree termination, integrates them into worker startup and fatal proxy shutdown, and adds subprocess-based Linux tests registered in the A10 test list. ChangesAnti-zombie process lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant ParentDeathSignal
participant libc
Worker->>ParentDeathSignal: configure default SIGKILL
ParentDeathSignal->>libc: call prctl(PR_SET_PDEATHSIG)
libc-->>Worker: return success or OSError
sequenceDiagram
participant GenerationExecutorProxy
participant kill_process_tree
participant psutil
participant Descendants
GenerationExecutorProxy->>kill_process_tree: clean descendants after fatal error
kill_process_tree->>psutil: enumerate recursive children
kill_process_tree->>Descendants: kill processes
kill_process_tree->>psutil: wait for process exit
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/unittest/_utils/test_anti_zombie.py (2)
57-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit
Nonereturn annotations.All three
test_*functions are non-returning and should declare-> None.As per coding guidelines, “Annotate every function, use
Nonefor non-returning functions.”🤖 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 `@tests/unittest/_utils/test_anti_zombie.py` around lines 57 - 141, Add an explicit -> None return annotation to each of the three test functions: test_prctl_kills_child_when_parent_dies, test_kill_process_tree_reaps_grandchildren, and test_set_parent_death_signal_idempotent. Do not alter their behavior or surrounding test logic.Source: Coding guidelines
98-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the proxy’s
include_parent=Falsecleanup path.This test only exercises
include_parent=True, whiletensorrt_llm/executor/proxy.pyLines 629-633 relies onFalse. Add a case that verifies descendants exit whileTOP_PIDremains alive, then explicitly reap the top process. Coverage is otherwise insufficient for the integration branch.🤖 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 `@tests/unittest/_utils/test_anti_zombie.py` around lines 98 - 115, The test coverage in test_kill_process_tree_reaps_grandchildren only exercises include_parent=True; add a case using kill_process_tree with include_parent=False that verifies CHILD_PID and GRANDCHILD_PID are reaped while TOP_PID remains alive, then explicitly terminate and reap TOP_PID to avoid leaking the process.Source: Path instructions
tensorrt_llm/executor/proxy.py (1)
629-637: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the cleanup exception boundary.
except Exceptionalso hides programming errors in the new fatal-shutdown path. Handle expected process races (psutil.Errorand, if applicable,OSError) explicitly; let unrelated defects remain visible.As per coding guidelines, “Catch the narrowest possible exceptions.”
🤖 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 629 - 637, Update the cleanup exception handler around kill_process_tree in the fatal-error shutdown path to catch only expected process-race exceptions, specifically psutil.Error and OSError where applicable. Preserve the existing debug logging for those failures, while allowing unrelated programming errors to propagate.Source: Coding guidelines
tensorrt_llm/_utils.py (1)
407-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the public helper contracts.
Annotate
sig(for example,int | None) and add Google-styleArgs:documentation for both exported helpers.🤖 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/_utils.py` around lines 407 - 440, Complete the public contracts for set_parent_death_signal and kill_process_tree by adding an explicit type annotation for set_parent_death_signal’s sig parameter, allowing None, and adding Google-style Args: sections documenting each parameter in both helpers, including defaults and behavior.Source: Coding guidelines
🤖 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/_utils.py`:
- Around line 425-428: Update the parent-death setup helper around libc.prctl to
accept the expected parent PID, arm PR_SET_PDEATHSIG, then compare the current
getppid() with that expected PID and terminate the worker if it changed.
Preserve the existing prctl error handling, and add a regression test covering
the launcher exiting during this startup window.
In `@tests/unittest/_utils/test_anti_zombie.py`:
- Around line 57-75: Update test_prctl_kills_child_when_parent_dies to
initialize child_pid before the try block and, in finally, best-effort terminate
and reap the child when it was created but remains alive, while preserving the
existing parent cleanup.
---
Nitpick comments:
In `@tensorrt_llm/_utils.py`:
- Around line 407-440: Complete the public contracts for set_parent_death_signal
and kill_process_tree by adding an explicit type annotation for
set_parent_death_signal’s sig parameter, allowing None, and adding Google-style
Args: sections documenting each parameter in both helpers, including defaults
and behavior.
In `@tensorrt_llm/executor/proxy.py`:
- Around line 629-637: Update the cleanup exception handler around
kill_process_tree in the fatal-error shutdown path to catch only expected
process-race exceptions, specifically psutil.Error and OSError where applicable.
Preserve the existing debug logging for those failures, while allowing unrelated
programming errors to propagate.
In `@tests/unittest/_utils/test_anti_zombie.py`:
- Around line 57-141: Add an explicit -> None return annotation to each of the
three test functions: test_prctl_kills_child_when_parent_dies,
test_kill_process_tree_reaps_grandchildren, and
test_set_parent_death_signal_idempotent. Do not alter their behavior or
surrounding test logic.
- Around line 98-115: The test coverage in
test_kill_process_tree_reaps_grandchildren only exercises include_parent=True;
add a case using kill_process_tree with include_parent=False that verifies
CHILD_PID and GRANDCHILD_PID are reaped while TOP_PID remains alive, then
explicitly terminate and reap TOP_PID to avoid leaking the process.
🪄 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: a79c6740-f1a2-4db5-90e6-196bfaab4213
📒 Files selected for processing (5)
tensorrt_llm/_utils.pytensorrt_llm/executor/proxy.pytensorrt_llm/executor/worker.pytests/integration/test_lists/test-db/l0_a10.ymltests/unittest/_utils/test_anti_zombie.py
2fa3dca to
a0190df
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #59422 [ run ] triggered by Bot. Commit: |
There was a problem hiding this comment.
expected_parent_pid is tested but never passed by a caller — worker_main (worker.py:191) calls bare set_parent_death_signal(), so the arming-race self-kill never runs on a real path. Worth wiring the proxy PID for the spawn case, or noting it's deferred so it doesn't read as dead code.
|
PR_Github #59422 [ run ] completed with state
|
200ee4b to
43d6470
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #59723 [ run ] triggered by Bot. Commit: |
|
PR_Github #59723 [ run ] completed with state
|
|
PR_Github #60867 [ run ] completed with state
|
|
/bot run |
Resolve conflicts: - tensorrt_llm/executor/proxy.py: keep both the anti-zombie kill_process_tree call at the end of pre_shutdown() and the new _get_next_client_id / _cleanup_multi_frontend_ipc_dir methods added on main. - tests/integration/test_lists/test-db/l0_cpu_x86.yml: keep both the new test_anti_zombie.py entry and the new test_multi_frontend_routing.py entry. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
Addresses review nit from @brnguyen2: signal, ctypes, time (stdlib) and psutil (third-party) were lazily imported inside set_parent_death_signal and kill_process_tree. Move them to the module-level imports for consistency with the rest of _utils.py, which already imports numpy, nvtx, mpi4py unconditionally. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
a0bc207 to
457a7d7
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #61179 [ run ] triggered by Bot. Commit: |
|
PR_Github #61179 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61478 [ run ] triggered by Bot. Commit: |
|
PR_Github #61478 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61686 [ run ] triggered by Bot. Commit: |
|
PR_Github #61686 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61694 [ run ] triggered by Bot. Commit: |
|
PR_Github #61694 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61782 [ run ] triggered by Bot. Commit: |
|
PR_Github #61782 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62105 [ run ] triggered by Bot. Commit: |
|
PR_Github #62105 [ run ] completed with state
|
|
Pipeline #50288 failed on one stage,
Root exception in both: Why this is not the PR:
Re-running CI. |
|
/bot run |
|
PR_Github #62131 [ run ] triggered by Bot. Commit: |
|
PR_Github #62131 [ run ] completed with state |
YihuiLu512
left a comment
There was a problem hiding this comment.
Could this mechanism fail to trigger because an MpiPoolSession worker is parented by the MPI daemon rather than the proxy, and the proxy exiting does not mean the daemon exits?
| # Anti-zombie: if our parent (proxy / MPI launcher) dies abruptly, have the | ||
| # kernel SIGKILL this worker so it can't orphan and leak GPU memory. | ||
| try: | ||
| set_parent_death_signal() |
There was a problem hiding this comment.
the registration scope of prctl(PR_SET_PDEATHSIG) is thread-level, but its kill effect is process-level.
In the case of MPICommExecutor:
- (a) the host process of
MpiCommSessionmay be launched from a worker thread by an external script (such as a test framework), which could cause unintended termination; - (b)
worker_mainfor rank 0 runs in the host process’s thread pool, which could result in the entire process being killed.
These two factors may amplify each other’s impact. Please evaluate whether this behavior is expected.
There was a problem hiding this comment.
Similarly for MpiPoolSession, although tasks are currently executed on the main thread, this relies on mpi4py's internal implementation details. If task dispatch is ever moved to a worker thread, the protection would silently disappear when that thread exits after worker_main returns.
Should we add an assertion to ensure that it must be registered in a callback of the main thread?
| # not the proxy's children and are covered by PR_SET_PDEATHSIG instead. | ||
| if self._fatal_error is not None: | ||
| try: | ||
| kill_process_tree(os.getpid(), |
There was a problem hiding this comment.
Would directly killing the process tree be overly aggressive?
For example:
- (a) the shadow pool prefetched in
session_prefetcherfor the next test - (b) multiple
LLMinstances within the same process
There was a problem hiding this comment.
#16770 session_prefetcher was merged yesterday
Closing — the motivating premise does not survive measurementThis PR adds two mechanisms against orphaned workers holding GPU memory. All three parts have now been measured on real hardware, and none of them justifies merging. Closing rather than reworking, with the evidence recorded here so the next person does not re-derive it. 1.
|
| Arm | Runs | Ranks die | GPU back to 0 MiB |
|---|---|---|---|
main, no PDEATHSIG |
3/3 | +1.25 s | ≤ +5.4 s |
| this PR, PDEATHSIG armed | 2/2 | +1.25 s | ≤ +5.4 s |
Survivors after a 360 s watch: none, in either arm.
Why the spin does not protect the rank. orted signals the ranks' process groups from outside — strace shows SIGCONT at +0.14 s, SIGTERM at +1.14 s, SIGKILL at +1.58 s — and a default-disposition SIGTERM kills a process regardless of what its main thread is doing in userspace. Separately, when orted is killed too, the ranks still self-exit with code 205 via OpenMPI's daemon-loss abort, which runs on the PMIx/OOB progress thread rather than the wedged main thread.
And a structural point that settles it independently of timing: PR_SET_PDEATHSIG armed in worker_main keys off orted's death, not the proxy's, because orted is the workers' direct parent. In the one documented orphan case (2026-07-22, ~35 GB held), orted survived. So the mechanism in this PR could not have fired in the very scenario that motivated it.
What remains open, honestly
The 2026-07-22 orphan case is still unexplained. Process topology was identical and it could not be reproduced, so the differentiator is something else — plausibly the broken-PCIe-P2P node state wedging orted itself. That remains an unexplained outlier rather than a general property, and if it is real, the mechanism that would address it is an out-of-band per-node reaper (ST-5), which does not depend on orted — not this PR.
Not measured
Single node, TP=2, one model, inside the devel container. Multi-node, Ray, and disaggregated topologies were not tested. Deep-tree PDEATHSIG was not tested — only the documented worker-under-orted case.
Full evidence — watcher timelines, evidence.json, strace traces, the harness and the applied patch — is preserved at tmp/trtllm-serve-ai-dev/data/public/multi_gpu_error_handling/impl/busy-spin-orphan-2026-07-31/, and the analysis is written up in design-round2-2026-07-30.html §2.1–2.2 and subtask-designs-2026-07-30.html §7.
Thanks to the reviewers who spent time on this — the thread-scope and blast-radius questions raised here were what prompted the measurements that settled it.
Summary by CodeRabbit
Bug Fixes
Tests
Description
When the proxy / MPI launcher dies abruptly (e.g. a watchdog hard-kill or a pod-kill), mpi4py worker processes can be left orphaned, keeping their CUDA context and holding GPU memory until the next run OOMs at model load and blames the wrong PR.
Add two anti-zombie helpers to tensorrt_llm._utils and wire them in:
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.