[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 |
| only covers the direct parent. Deeper / forked trees are handled by | ||
| ``kill_process_tree``. | ||
| """ | ||
| import signal |
There was a problem hiding this comment.
Nit: the lazy imports in set_parent_death_signal (signal, ctypes) and kill_process_tree (time, psutil) could be moved to the top of the file. signal, ctypes, and time are stdlib; ctypes is already partially imported at the top (from ctypes import byref). psutil is the only third-party dep here, but _utils.py already imports numpy, nvtx, and mpi4py unconditionally so the bar for lazy-importing it seems inconsistent.
There was a problem hiding this comment.
Thanks for the suggestion! Updated.
|
PR_Github #59723 [ run ] triggered by Bot. Commit: |
|
PR_Github #59723 [ run ] completed with state
|
|
PR_Github #60256 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60557 [ run ] triggered by Bot. Commit: |
|
PR_Github #60557 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60713 [ run ] triggered by Bot. Commit: |
|
PR_Github #60713 [ run ] completed with state
|
The CPU-Generic-arm CI stage invokes pytest with -m 'cpu_only and not disabled', so tests without the cpu_only marker are deselected and the stage exits with pytest code 5, which the outer runner reports as 'AssertionError: failure reported in unittests'. Add pytest.mark.cpu_only alongside the existing linux-only skipif so the four tests in tests/unittest/_utils/test_anti_zombie.py are actually selected on the CPU stages. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run |
|
PR_Github #60725 [ run ] triggered by Bot. Commit: |
|
PR_Github #60725 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60867 [ run ] triggered by Bot. Commit: |
|
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
|
6 participants
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.