Skip to content

Commit 877c3dd

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 8974ed8 commit 877c3dd

3 files changed

Lines changed: 133 additions & 3 deletions

File tree

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
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+
19+
def __init__(self, *, engine_dead: bool, engine_manager):
20+
self.engine_dead = engine_dead
21+
self.engine_manager = engine_manager
22+
self.cleanup = MagicMock()
23+
24+
def __call__(self):
25+
self.cleanup()
26+
27+
28+
def test_mp_client_shutdown_marks_engine_dead_before_manager_shutdown():
29+
client = object.__new__(MPClient)
30+
client._finalizer = MagicMock()
31+
client._finalizer.detach.return_value = object()
32+
33+
engine_manager = MagicMock()
34+
client.resources = DummyResources(
35+
engine_dead=False,
36+
engine_manager=engine_manager,
37+
)
38+
39+
client.shutdown(timeout=3.0)
40+
41+
assert client.resources.engine_dead is True
42+
engine_manager.shutdown.assert_called_once_with(timeout=3.0)
43+
client.resources.cleanup.assert_called_once_with()
44+
45+
46+
def test_mp_client_monitor_ignores_clean_engine_exit(monkeypatch: pytest.MonkeyPatch):
47+
client = object.__new__(MPClient)
48+
client._finalizer = SimpleNamespace(alive=True)
49+
client.resources = SimpleNamespace(
50+
engine_dead=False,
51+
engine_manager=SimpleNamespace(
52+
failed_proc_name=None,
53+
monitor_engine_liveness=lambda: None,
54+
),
55+
)
56+
client.shutdown = MagicMock()
57+
58+
thread_target = None
59+
60+
class ImmediateThread:
61+
def __init__(self, *, target, daemon, name):
62+
nonlocal thread_target
63+
thread_target = target
64+
65+
def start(self):
66+
thread_target()
67+
68+
monkeypatch.setattr(core_client_mod, "Thread", ImmediateThread)
69+
logger_error = MagicMock()
70+
monkeypatch.setattr(core_client_mod.logger, "error", logger_error)
71+
72+
client.start_engine_core_monitor()
73+
74+
assert client.resources.engine_dead is False
75+
client.shutdown.assert_not_called()
76+
logger_error.assert_not_called()
77+
78+
79+
def test_llm_engine_shutdown_cleans_up_owned_resources(
80+
monkeypatch: pytest.MonkeyPatch,
81+
):
82+
llm_engine = object.__new__(LLMEngine)
83+
renderer = MagicMock()
84+
engine_core = MagicMock()
85+
dp_group = object()
86+
llm_engine.renderer = renderer
87+
llm_engine.engine_core = engine_core
88+
llm_engine.dp_group = dp_group
89+
llm_engine.external_launcher_dp = False
90+
91+
shutdown_prometheus = MagicMock()
92+
destroy_dp_group = MagicMock()
93+
monkeypatch.setattr(llm_engine_mod, "shutdown_prometheus", shutdown_prometheus)
94+
monkeypatch.setattr(
95+
llm_engine_mod,
96+
"stateless_destroy_torch_distributed_process_group",
97+
destroy_dp_group,
98+
)
99+
100+
llm_engine.shutdown(timeout=1.5)
101+
102+
shutdown_prometheus.assert_called_once_with()
103+
renderer.shutdown.assert_called_once_with()
104+
engine_core.shutdown.assert_called_once_with(timeout=1.5)
105+
destroy_dp_group.assert_called_once_with(dp_group)
106+
assert llm_engine.renderer is None
107+
assert llm_engine.engine_core is None
108+
assert llm_engine.dp_group is None

vllm/v1/engine/core_client.py

Lines changed: 9 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,13 @@ 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+
return
701707
_self.resources.engine_dead = True
702-
logger.warning_once(
703-
"[shutdown] MPClient: engine core exited unexpectedly; starting cleanup"
708+
logger.error(
709+
"Engine core proc %s died unexpectedly, shutting down client.",
710+
failed_proc_name,
704711
)
705712
_self.shutdown()
706713
# Note: For MPClient, we don't have a failure callback mechanism

vllm/v1/engine/llm_engine.py

Lines changed: 16 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,21 @@ 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+
renderer.shutdown()
451+
self.renderer = None
452+
453+
if engine_core := getattr(self, "engine_core", None):
454+
engine_core.shutdown(timeout=timeout)
455+
self.engine_core = None
456+
446457
dp_group = getattr(self, "dp_group", None)
447458
if dp_group is not None and not self.external_launcher_dp:
448459
stateless_destroy_torch_distributed_process_group(dp_group)
460+
self.dp_group = None
461+
462+
def __del__(self):
463+
self.shutdown()

0 commit comments

Comments
 (0)