From e52d1190c9eefdfe99a33f52189c7aae87d5dfe0 Mon Sep 17 00:00:00 2001 From: Lanze Liu Date: Sat, 3 Oct 2026 15:11:41 -0700 Subject: [PATCH 1/3] [Bugfix] Keep force-killing the process tree when a child exits mid-walk #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 --- tests/utils_/test_system_utils.py | 26 ++++++++++++++++++++++++++ vllm/utils/system_utils.py | 2 +- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/tests/utils_/test_system_utils.py b/tests/utils_/test_system_utils.py index 55ed35eeb56d..c8a97993e03d 100644 --- a/tests/utils_/test_system_utils.py +++ b/tests/utils_/test_system_utils.py @@ -7,7 +7,11 @@ import sys import tempfile from pathlib import Path +from types import SimpleNamespace +import psutil + +from vllm.utils import system_utils from vllm.utils.system_utils import ( _maybe_force_spawn, suppress_stdout, @@ -48,3 +52,25 @@ def test_suppress_stdout_keeps_stderr_when_sys_stdout_is_stderr(capfd, monkeypat out, err = capfd.readouterr() assert "c library output" not in out assert "error message" in err + + +def test_kill_process_tree_skips_child_that_exits_mid_walk(monkeypatch): + class Child: + def __init__(self, pid: int, exited: bool = False): + self.pid = pid + self.exited = exited + + def name(self) -> str: + if self.exited: + raise psutil.NoSuchProcess(self.pid) + return "VLLM::Worker" + + children = [Child(101), Child(102, exited=True), Child(103)] + parent = SimpleNamespace(children=lambda recursive: children) + killed: list[int] = [] + monkeypatch.setattr(system_utils.psutil, "Process", lambda pid: parent) + monkeypatch.setattr(system_utils.os, "kill", lambda pid, sig: killed.append(pid)) + + system_utils.kill_process_tree(100) + + assert killed == [101, 103, 100] diff --git a/vllm/utils/system_utils.py b/vllm/utils/system_utils.py index a15dbf3c6cdf..71947609bf37 100644 --- a/vllm/utils/system_utils.py +++ b/vllm/utils/system_utils.py @@ -221,7 +221,7 @@ def kill_process_tree(pid: int): # Send SIGKILL to all children first for child in children: - with contextlib.suppress(ProcessLookupError): + with contextlib.suppress(ProcessLookupError, psutil.NoSuchProcess): logger.warning( "[shutdown] force killing sub process %s pid %d", child.name(), From d0deff924f01621a9738111f85aecc2575c222ad Mon Sep 17 00:00:00 2001 From: Lanze Liu Date: Sat, 10 Oct 2026 14:18:26 -0700 Subject: [PATCH 2/3] Return early when the target exits before its children are listed 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 --- tests/utils_/test_system_utils.py | 62 +++++++++++++++++++++---------- vllm/utils/system_utils.py | 5 +-- 2 files changed, 45 insertions(+), 22 deletions(-) diff --git a/tests/utils_/test_system_utils.py b/tests/utils_/test_system_utils.py index c8a97993e03d..71e796925601 100644 --- a/tests/utils_/test_system_utils.py +++ b/tests/utils_/test_system_utils.py @@ -4,10 +4,11 @@ import contextlib import io import os +import signal +import subprocess import sys import tempfile from pathlib import Path -from types import SimpleNamespace import psutil @@ -54,23 +55,46 @@ def test_suppress_stdout_keeps_stderr_when_sys_stdout_is_stderr(capfd, monkeypat assert "error message" in err -def test_kill_process_tree_skips_child_that_exits_mid_walk(monkeypatch): - class Child: - def __init__(self, pid: int, exited: bool = False): - self.pid = pid - self.exited = exited - - def name(self) -> str: - if self.exited: - raise psutil.NoSuchProcess(self.pid) - return "VLLM::Worker" +def _sleeper() -> subprocess.Popen: + return subprocess.Popen([sys.executable, "-c", "import time; time.sleep(60)"]) - children = [Child(101), Child(102, exited=True), Child(103)] - parent = SimpleNamespace(children=lambda recursive: children) - killed: list[int] = [] - monkeypatch.setattr(system_utils.psutil, "Process", lambda pid: parent) - monkeypatch.setattr(system_utils.os, "kill", lambda pid, sig: killed.append(pid)) - system_utils.kill_process_tree(100) - - assert killed == [101, 103, 100] +def test_kill_process_tree_skips_child_that_exits_mid_walk(monkeypatch): + target, *children = procs = [_sleeper() for _ in range(4)] + try: + # List the children while they are alive, then let one exit and be + # reaped before the walk reaches it, so psutil's name() raises. + listed = [psutil.Process(p.pid) for p in children] + children[1].kill() + children[1].wait() + monkeypatch.setattr( + psutil.Process, "children", lambda self, recursive=False: listed + ) + + system_utils.kill_process_tree(target.pid) + + for p in (children[0], children[2], target): + assert p.wait(timeout=10) == -signal.SIGKILL + finally: + for p in procs: + p.kill() + p.wait() + + +def test_kill_process_tree_returns_when_target_exits_before_children(monkeypatch): + target = _sleeper() + list_children = psutil.Process.children + + def exit_then_list(self, recursive=False): + # The target exits and is reaped after the lookup, so psutil raises + # NoSuchProcess from inside children(). + target.kill() + target.wait() + return list_children(self, recursive=recursive) + + monkeypatch.setattr(psutil.Process, "children", exit_then_list) + try: + system_utils.kill_process_tree(target.pid) + finally: + target.kill() + target.wait() diff --git a/vllm/utils/system_utils.py b/vllm/utils/system_utils.py index 71947609bf37..8bd08fe5576d 100644 --- a/vllm/utils/system_utils.py +++ b/vllm/utils/system_utils.py @@ -213,12 +213,11 @@ def kill_process_tree(pid: int): """ try: parent = psutil.Process(pid) + # Get all children recursively + children = parent.children(recursive=True) except psutil.NoSuchProcess: return - # Get all children recursively - children = parent.children(recursive=True) - # Send SIGKILL to all children first for child in children: with contextlib.suppress(ProcessLookupError, psutil.NoSuchProcess): From 0b5d79250a051fa527dfaaa4ef9c83cf62d8ec4a Mon Sep 17 00:00:00 2001 From: Lanze Liu Date: Mon, 12 Oct 2026 16:02:37 -0700 Subject: [PATCH 3/3] Revert the children() change and SIGTERM the exited child in the test Per review, keep this PR to the #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 --- tests/utils_/test_system_utils.py | 32 ++++++------------------------- vllm/utils/system_utils.py | 5 +++-- 2 files changed, 9 insertions(+), 28 deletions(-) diff --git a/tests/utils_/test_system_utils.py b/tests/utils_/test_system_utils.py index 71e796925601..ef95bd7c368a 100644 --- a/tests/utils_/test_system_utils.py +++ b/tests/utils_/test_system_utils.py @@ -55,46 +55,26 @@ def test_suppress_stdout_keeps_stderr_when_sys_stdout_is_stderr(capfd, monkeypat assert "error message" in err -def _sleeper() -> subprocess.Popen: - return subprocess.Popen([sys.executable, "-c", "import time; time.sleep(60)"]) - - def test_kill_process_tree_skips_child_that_exits_mid_walk(monkeypatch): - target, *children = procs = [_sleeper() for _ in range(4)] + sleep = [sys.executable, "-c", "import time; time.sleep(60)"] + target, *children = procs = [subprocess.Popen(sleep) for _ in range(4)] try: # List the children while they are alive, then let one exit and be # reaped before the walk reaches it, so psutil's name() raises. listed = [psutil.Process(p.pid) for p in children] - children[1].kill() - children[1].wait() + children[1].terminate() + children[1].wait(timeout=10) monkeypatch.setattr( psutil.Process, "children", lambda self, recursive=False: listed ) system_utils.kill_process_tree(target.pid) + # The exited child died from our SIGTERM; the rest were SIGKILLed. + assert children[1].returncode == -signal.SIGTERM for p in (children[0], children[2], target): assert p.wait(timeout=10) == -signal.SIGKILL finally: for p in procs: p.kill() p.wait() - - -def test_kill_process_tree_returns_when_target_exits_before_children(monkeypatch): - target = _sleeper() - list_children = psutil.Process.children - - def exit_then_list(self, recursive=False): - # The target exits and is reaped after the lookup, so psutil raises - # NoSuchProcess from inside children(). - target.kill() - target.wait() - return list_children(self, recursive=recursive) - - monkeypatch.setattr(psutil.Process, "children", exit_then_list) - try: - system_utils.kill_process_tree(target.pid) - finally: - target.kill() - target.wait() diff --git a/vllm/utils/system_utils.py b/vllm/utils/system_utils.py index 8bd08fe5576d..71947609bf37 100644 --- a/vllm/utils/system_utils.py +++ b/vllm/utils/system_utils.py @@ -213,11 +213,12 @@ def kill_process_tree(pid: int): """ try: parent = psutil.Process(pid) - # Get all children recursively - children = parent.children(recursive=True) except psutil.NoSuchProcess: return + # Get all children recursively + children = parent.children(recursive=True) + # Send SIGKILL to all children first for child in children: with contextlib.suppress(ProcessLookupError, psutil.NoSuchProcess):