Skip to content

Commit fca2867

Browse files
fix(v1): avoid false shutdown failures on clean exit
- mark MPClient shutdown as intentional before engine teardown\n- ignore clean monitor exits without failed_proc_name noise\n- add an explicit LLMEngine.shutdown path with focused unit tests\n\nCo-authored-by: GitHub Copilot <copilot@github.com> Signed-off-by: shuhao zhang <shuhao_zhang@hust.edu.cn>
1 parent 0ba2aa3 commit fca2867

3 files changed

Lines changed: 153 additions & 3 deletions

File tree

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
# SPDX-License-Identifier: Apache-2.0
2+
# SPDX-FileCopyrightText: Copyright contributors to the vLLM project
3+
4+
from types import SimpleNamespace
5+
from unittest.mock import MagicMock
6+
7+
import pytest
8+
9+
import vllm.v1.engine.core_client as core_client_mod
10+
import vllm.v1.engine.llm_engine as llm_engine_mod
11+
from vllm.v1.engine.core_client import MPClient
12+
from vllm.v1.engine.llm_engine import LLMEngine
13+
14+
pytestmark = pytest.mark.skip_global_cleanup
15+
16+
17+
class DummyResources:
18+
def __init__(self, *, engine_dead: bool, engine_manager):
19+
self.engine_dead = engine_dead
20+
self.engine_manager = engine_manager
21+
self.cleanup = MagicMock()
22+
23+
def __call__(self):
24+
self.cleanup()
25+
26+
27+
def test_mp_client_shutdown_marks_engine_dead_before_manager_shutdown():
28+
client = object.__new__(MPClient)
29+
client._finalizer = MagicMock()
30+
client._finalizer.detach.return_value = object()
31+
32+
engine_manager = MagicMock()
33+
client.resources = DummyResources(
34+
engine_dead=False,
35+
engine_manager=engine_manager,
36+
)
37+
38+
client.shutdown(timeout=3.0)
39+
40+
assert client.resources.engine_dead is True
41+
engine_manager.shutdown.assert_called_once_with(timeout=3.0)
42+
client.resources.cleanup.assert_called_once_with()
43+
44+
45+
def test_mp_client_monitor_cleans_up_after_clean_engine_exit(
46+
monkeypatch: pytest.MonkeyPatch,
47+
):
48+
client = object.__new__(MPClient)
49+
client._finalizer = SimpleNamespace(alive=True)
50+
client.resources = SimpleNamespace(
51+
engine_dead=False,
52+
engine_manager=SimpleNamespace(
53+
failed_proc_name=None,
54+
monitor_engine_liveness=lambda: None,
55+
),
56+
)
57+
client.shutdown = MagicMock()
58+
59+
thread_target = None
60+
61+
class ImmediateThread:
62+
def __init__(self, *, target, daemon, name):
63+
nonlocal thread_target
64+
thread_target = target
65+
66+
def start(self):
67+
thread_target()
68+
69+
monkeypatch.setattr(core_client_mod, "Thread", ImmediateThread)
70+
logger_error = MagicMock()
71+
monkeypatch.setattr(core_client_mod.logger, "error", logger_error)
72+
73+
client.start_engine_core_monitor()
74+
75+
assert client.resources.engine_dead is False
76+
client.shutdown.assert_called_once_with()
77+
logger_error.assert_not_called()
78+
79+
80+
def test_llm_engine_shutdown_cleans_up_owned_resources(
81+
monkeypatch: pytest.MonkeyPatch,
82+
):
83+
llm_engine = object.__new__(LLMEngine)
84+
renderer = MagicMock()
85+
engine_core = MagicMock()
86+
dp_group = object()
87+
llm_engine.renderer = renderer
88+
llm_engine.engine_core = engine_core
89+
llm_engine.dp_group = dp_group
90+
llm_engine.external_launcher_dp = False
91+
92+
shutdown_prometheus = MagicMock()
93+
destroy_dp_group = MagicMock()
94+
monkeypatch.setattr(llm_engine_mod, "shutdown_prometheus", shutdown_prometheus)
95+
monkeypatch.setattr(
96+
llm_engine_mod,
97+
"stateless_destroy_torch_distributed_process_group",
98+
destroy_dp_group,
99+
)
100+
101+
llm_engine.shutdown(timeout=1.5)
102+
103+
shutdown_prometheus.assert_called_once_with()
104+
renderer.shutdown.assert_called_once_with()
105+
engine_core.shutdown.assert_called_once_with(timeout=1.5)
106+
destroy_dp_group.assert_called_once_with(dp_group)
107+
assert llm_engine.renderer is None
108+
assert llm_engine.engine_core is None
109+
assert llm_engine.dp_group is None
110+
111+
112+
def test_llm_engine_shutdown_tolerates_renderer_without_shutdown(
113+
monkeypatch: pytest.MonkeyPatch,
114+
):
115+
llm_engine = object.__new__(LLMEngine)
116+
llm_engine.renderer = object()
117+
llm_engine.engine_core = None
118+
llm_engine.dp_group = None
119+
llm_engine.external_launcher_dp = False
120+
monkeypatch.setattr(llm_engine_mod, "shutdown_prometheus", MagicMock())
121+
122+
llm_engine.shutdown()
123+
124+
assert llm_engine.renderer is None

vllm/v1/engine/core_client.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -653,6 +653,9 @@ def shutdown(self, timeout: float | None = None) -> None:
653653
if self._finalizer.detach() is not None:
654654
timeout_str = "default" if timeout is None else f"{timeout}s"
655655
logger.info("[shutdown] MPClient: start timeout=%s", timeout_str)
656+
# Mark shutdown as intentional before tearing down child processes
657+
# so the monitor thread can distinguish it from a real crash.
658+
self.resources.engine_dead = True
656659
if self.resources.engine_manager is not None:
657660
logger.info_once("[shutdown] MPClient: stopping engine manager")
658661
self.resources.engine_manager.shutdown(timeout=timeout)
@@ -698,9 +701,16 @@ def monitor_engine_cores():
698701
_self = self_ref()
699702
if not _self or not _self._finalizer.alive or _self.resources.engine_dead:
700703
return
704+
failed_proc_name = getattr(engine_manager, "failed_proc_name", None)
705+
if failed_proc_name is None:
706+
# The manager exited cleanly, but the client still owns sockets,
707+
# tasks, and other background resources that must be released.
708+
_self.shutdown()
709+
return
701710
_self.resources.engine_dead = True
702-
logger.warning_once(
703-
"[shutdown] MPClient: engine core exited unexpectedly; starting cleanup"
711+
logger.error(
712+
"Engine core proc %s died unexpectedly, shutting down client.",
713+
failed_proc_name,
704714
)
705715
_self.shutdown()
706716
# Note: For MPClient, we don't have a failure callback mechanism

vllm/v1/engine/llm_engine.py

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
from vllm.v1.engine.parallel_sampling import ParentRequest
3636
from vllm.v1.executor import Executor
3737
from vllm.v1.metrics.loggers import StatLoggerFactory, StatLoggerManager
38+
from vllm.v1.metrics.prometheus import shutdown_prometheus
3839
from vllm.v1.metrics.reader import Metric, get_metrics_snapshot
3940
from vllm.v1.metrics.stats import IterationStats
4041
from vllm.v1.utils import record_function_or_nullcontext
@@ -442,7 +443,22 @@ def _cleanup_instance_caches(model) -> None:
442443
if isinstance(module, TorchCompileWithNoGuardsWrapper):
443444
module.cleanup()
444445

445-
def __del__(self):
446+
def shutdown(self, timeout: float | None = None) -> None:
447+
shutdown_prometheus()
448+
449+
if renderer := getattr(self, "renderer", None):
450+
if shutdown := getattr(renderer, "shutdown", None):
451+
shutdown()
452+
self.renderer = None
453+
454+
if engine_core := getattr(self, "engine_core", None):
455+
engine_core.shutdown(timeout=timeout)
456+
self.engine_core = None
457+
446458
dp_group = getattr(self, "dp_group", None)
447459
if dp_group is not None and not self.external_launcher_dp:
448460
stateless_destroy_torch_distributed_process_group(dp_group)
461+
self.dp_group = None
462+
463+
def __del__(self):
464+
self.shutdown()

0 commit comments

Comments
 (0)