Repository navigation
[Bugfix] Keep force-killing the process tree when a child exits mid-walk - #59984
Merged
njhill merged 4 commits intoOct 14, 2026
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
6 tasks done
chessing234
reviewed
Oct 6, 2026
chessing234
left a comment
chessing234
left a comment
Contributor
There was a problem hiding this comment.
everything seems fine, ready to merge.
Contributor
|
This pull request has merge conflicts that must be resolved before it can be |
6 of 8 tasks
vllm-project#52314 started logging each child's name() before SIGKILLing it, inside suppress(ProcessLookupError). psutil raises NoSuchProcess, not ProcessLookupError, when the child was reaped after children() listed it. That race is likely on the force-kill path, where the target is still tearing down and reaping its own children. The exception escapes kill_process_tree, so the remaining children and the target itself are never killed, and callers such as shutdown() abort their loop over the remaining processes. Suppress NoSuchProcess as well: a child that is already gone needs no SIGKILL, and skipping it avoids signalling a recycled pid. Signed-off-by: Lanze Liu <lanzetech@gmail.com>
liulanze
force-pushed
the
fix/kill-process-tree-nosuchprocess
branch
from
October 9, 2026 19:39
9b42062 to
e52d119
Compare
Contributor
Author
|
Rebased onto the latest main to resolve the conflict with #59336. The conflict was only in the test file, where both PRs added imports and a test; the fix itself is unchanged. @andyxning this is a small follow-up to #52314: the new |
andyxning
reviewed
Oct 10, 2026
psutil's children() looks the target up again (Process(pid).create_time()) and raises NoSuchProcess if it has exited and been reaped since the first lookup. Treat that like the existing lookup failure and return. The tests now use real processes, so it is psutil's own name() and children() that raise NoSuchProcess. Signed-off-by: Lanze Liu <lanzetech@gmail.com>
Per review, keep this PR to the vllm-project#52314 regression: move children() back out of the try and drop the target-exit test. In the remaining test, end the child that exits mid-walk with SIGTERM instead of SIGKILL and assert it, so it is clearly told apart from the processes that kill_process_tree SIGKILLs. Signed-off-by: Lanze Liu <lanzetech@gmail.com>
Member
|
/lgtm |
Member
|
/ci run |
|
✅ @liulanze, CI is now available for this PR.
|
|
✅ Triggered Buildkite CI #94375 for commit |
Collaborator
CI selector (shadow): 90 test steps (130 jobs) instead of 57 (73 jobs)Shadow mode: this changes nothing about what CI runs. It shows what the evidence-based selector would pick for this PR, next to today's rules. How it works. Feedback welcome: reply here if it would skip a step this change needs, or runs something unrelated.
Selector would run (90)
Would skip (today's rules run them) (28)
Would add (today's rules do not run them) (61)
AMD mirrors: would skip (44)
AMD mirrors: would add (10)
2 changed files · base |
Member
|
/cc @DarkLight1337 PTAL. |
njhill
approved these changes
Oct 14, 2026
AndreasKaratzas
pushed a commit
to AndreasKaratzas/vllm
that referenced
this pull request
Oct 16, 2026
…alk (vllm-project#59984) Signed-off-by: Lanze Liu <lanzetech@gmail.com> Co-authored-by: Ning Xie <andy.xning@gmail.com> Signed-off-by: Andreas Karatzas <AndreasKaratzas@users.noreply.github.com>
4 tasks done
michael-gpu
pushed a commit
to practical-inference/vllm-ampere-extended
that referenced
this pull request
Oct 20, 2026
…alk (vllm-project#59984) Signed-off-by: Lanze Liu <lanzetech@gmail.com> Co-authored-by: Ning Xie <andy.xning@gmail.com>
5 of 6 tasks
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.
Overview
Fix a regression from #52314:
kill_process_treestops early, leaving the rest of the tree and the target alive, when a child exits betweenparent.children()andchild.name().Claims
kill_process_treenow SIGKILLs every remaining descendant and the target pid even if a child is reaped while it walks the tree.psutil.NoSuchProcessescaped the function. Callers such asvllm/v1/utils.py::shutdown()then abandoned their loop over the remaining processes, so EngineCores, API servers or workers that were meant to be force-killed survived.Validation
The new test
tests/utils_/test_system_utils.py::test_kill_process_tree_skips_child_that_exits_mid_walk:children()report three of them as the target's children;kill_process_treeruns, so psutil's ownname()raisesNoSuchProcessfor it;Results on Linux (base
04462732a4, Python 3.12):mainNoSuchProcess: process no longer exists, fromchild.name()pytest tests/utils_/test_system_utils.py: 5 passed with each psutil version. The new test also passed 30 runs in a row with each.pre-commit run --hook-stage manual(the stage CI runs) passes on the changed files, including ruff, typos and mypy.Details
Root cause. #52314 added
logger.warning(..., child.name(), child.pid)inside the existingcontextlib.suppress(ProcessLookupError). On Linux,psutil.Process.name()re-reads/proc/<pid>/staton every call; only Windows caches it. Once the child has been reaped it raisesNoSuchProcess, which is not aProcessLookupError. Before #52314 the loop only readchild.pid, which only returns the stored pid, so this path could not raise.Why the window is real. The force-kill path runs when a process did not exit within its grace period. Typically that process is still tearing down and reaping its own children, for example EngineCore joining its workers after SIGTERM, so a child listed by
children()can be gone by the timename()runs.Fix. Also suppress
NoSuchProcessfor that child. The child is already gone, so it needs no SIGKILL, and skipping it avoids signalling a pid that may have been recycled. This matches the existingpsutil.NoSuchProcessguard at the top of the same function andvllm/entrypoints/launchers/dp_supervisor.py.Affected callers.
vllm/v1/utils.py::shutdown(), used byCoreEngineProcManager,APIServerProcessManagerand the DP coordinator.vllm/v1/utils.py::_shutdown_subprocesses(), used byRustFrontendProcessManager.dp_supervisor.pyalready suppresses the exception, but before this fix it silently skipped killing the rest of the tree.Not a duplicate. I found no issue or PR that reports or fixes this. I searched
kill_process_tree,NoSuchProcessand "force killing sub process" for anything after #52314 was merged on 2026-09-30.AI assistance (Claude Code) was used to find this, write the patch and the test, and draft this description. I reviewed every changed line and ran the tests above.
Co-authored-by: Claude Opus 5.5 noreply@anthropic.com
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.