test: reset model-manager shutdown state between backend tests (#1320)
* test: reset model-manager shutdown state between backend tests Two leaks, one of them mine. 1. `model_manager._shutting_down` is a module-global Event and the GPU pool is a module-global executor. Any test that runs the app lifespan flips both on the way out (begin_shutdown + _reset_gpu_pool) and nothing puts them back — correct in production, where the process is ending; wrong across a combined session. A test arriving with the flag set finds a shut-down executor, so its first run_in_executor raises "cannot schedule new futures after shutdown", which the preload path classifies as benign and swallows. The symptom is a load that silently never starts. Reset before AND after: before so an inherited flag cannot decide the test, after so a test that legitimately shuts down does not hand it on. 2. tests/test_torch_compile_path_gate.py assigned services.settings_store into sys.modules directly instead of via monkeypatch.setitem. That leaks process-wide out of collection and breaks every later import of the real module. backend/tests/test_no_module_stubs.py exists to catch exactly that, and caught it — I introduced it two commits ago while isolating the Settings gate for a review finding. #1269 stays open for its last failure, which is a different root cause: test_lifespan_shutdown_mid_load fails because a reload fixture in tests/ replaces services.model_manager, so the test patches one module object while main's lifespan uses another (verified: `same=False`). That is the duplicate- module class, not a state leak, and needs its own fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: let the shutdown-state reset fail loudly CodeRabbit Major: the broad try/except meant a reset that raised left the next test with stale shutdown or executor state — precisely the order-dependent failure the fixture exists to remove, while looking like it had worked. That is the same silent-fail-open shape as the watermark and ffmpeg bugs fixed earlier in this cycle. If reset_shutdown_flag() or _reset_gpu_pool() can raise, that is a real problem in model_manager and it should be loud. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: assert the shutdown-state reset fixture actually resets (CodeRabbit) Ordered pair: one test leaves the module globals exactly as the lifespan leaves them, the next asserts it arrived clean — delete the fixture and the second fails. Plus a mechanical guard that the reset stays un-swallowed, so a future try/except cannot make the fixture look like it worked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
26bcd95088
commit
850ae8e192
@@ -109,3 +109,42 @@ def _clear_asr_installed_memo(request):
|
||||
_clear_all()
|
||||
yield
|
||||
_clear_all()
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _clean_model_manager_shutdown_state():
|
||||
"""Start every test with the model manager NOT in shutdown mode (#1269).
|
||||
|
||||
``model_manager._shutting_down`` is a module-global Event and the GPU pool is
|
||||
a module-global executor. Any test that runs the app lifespan flips both on
|
||||
the way out — ``begin_shutdown()`` plus ``_reset_gpu_pool()`` — and nothing
|
||||
puts them back, because in production that state is correct: the process is
|
||||
ending.
|
||||
|
||||
Across a combined ``pytest tests/ backend/tests/`` session it is not
|
||||
correct, and it is not a cosmetic leak. A test that arrives with the flag set
|
||||
finds a shut-down executor, so its very first ``run_in_executor`` raises
|
||||
"cannot schedule new futures after shutdown" — which the preload path
|
||||
classifies as a benign shutdown and swallows. The symptom is a load that
|
||||
silently never starts: ``test_lifespan_shutdown_mid_load_is_clean_and_clears
|
||||
_sentinel`` failed on ``assert started.is_set()`` for exactly this reason,
|
||||
while passing alone.
|
||||
|
||||
Reset before AND after: before so an inherited flag cannot decide this test,
|
||||
after so a test that legitimately shuts down does not hand the state on.
|
||||
"""
|
||||
import services.model_manager as _mm
|
||||
|
||||
def _clean():
|
||||
# Deliberately NOT wrapped in try/except. A reset that fails silently
|
||||
# leaves the next test with stale shutdown or executor state, which is
|
||||
# precisely the order-dependent failure this fixture exists to remove —
|
||||
# swallowing the error would defeat the fixture while looking like it
|
||||
# worked (CodeRabbit). If either of these can raise, that is a real
|
||||
# problem in model_manager and it should be loud.
|
||||
_mm.reset_shutdown_flag()
|
||||
_mm._reset_gpu_pool()
|
||||
|
||||
_clean()
|
||||
yield
|
||||
_clean()
|
||||
|
||||
@@ -0,0 +1,59 @@
|
||||
"""The autouse shutdown-state reset must actually reset, and fail loudly.
|
||||
|
||||
``conftest._clean_model_manager_shutdown_state`` is the fix for #1269: a test
|
||||
that runs the app lifespan leaves ``model_manager._shutting_down`` set and the
|
||||
GPU pool torn down, and the next test then finds every ``run_in_executor``
|
||||
raising "cannot schedule new futures after shutdown" — swallowed by the preload
|
||||
path as a benign shutdown, so the symptom is a load that silently never starts.
|
||||
|
||||
A fixture with nothing asserting it is a fixture nobody notices breaking. The
|
||||
pair below is deliberately order-dependent (pytest runs tests in definition
|
||||
order within a file): the first test dirties exactly the state the lifespan
|
||||
dirties, the second asserts it arrived clean. Delete the fixture and the second
|
||||
test fails; that is the fail-before/pass-after this file exists to provide.
|
||||
"""
|
||||
|
||||
import os
|
||||
|
||||
import services.model_manager as mm
|
||||
|
||||
_CONFTEST = os.path.join(os.path.dirname(os.path.abspath(__file__)), "conftest.py")
|
||||
|
||||
|
||||
def test_dirty_the_shutdown_state():
|
||||
"""Stand in for any lifespan-running test: leave the module globals in the
|
||||
exact state graceful shutdown leaves them."""
|
||||
mm.begin_shutdown()
|
||||
mm._reset_gpu_pool()
|
||||
assert mm.is_shutting_down()
|
||||
|
||||
|
||||
def test_next_test_starts_clean():
|
||||
"""Runs immediately after the test above and must not inherit its state."""
|
||||
assert not mm.is_shutting_down(), (
|
||||
"the shutdown flag leaked from the previous test — the autouse fixture "
|
||||
"in backend/tests/conftest.py is not resetting it, and every executor "
|
||||
"submit in this test will now raise 'cannot schedule new futures after "
|
||||
"shutdown' and be misread as a benign cancellation (#1269)"
|
||||
)
|
||||
|
||||
|
||||
def test_reset_failures_are_not_swallowed():
|
||||
"""A reset wrapped in ``except: pass`` hands the next test stale state while
|
||||
reporting success — the fixture would look like it worked and #1269 would
|
||||
come back with the evidence removed. Mechanical, so it stays true."""
|
||||
with open(_CONFTEST, encoding="utf-8") as fh:
|
||||
src = fh.read()
|
||||
marker = "def _clean_model_manager_shutdown_state("
|
||||
assert marker in src, f"fixture renamed or removed from {_CONFTEST}"
|
||||
body = src.split(marker, 1)[1].split("\n@", 1)[0]
|
||||
# Comments and the docstring explain *why* there is no try/except; only the
|
||||
# code is evidence of whether there is one.
|
||||
code = "\n".join(
|
||||
line for line in body.split('"""')[-1].splitlines()
|
||||
if not line.lstrip().startswith("#")
|
||||
)
|
||||
assert "except" not in code, (
|
||||
"the shutdown-state reset catches exceptions; a failed reset must fail "
|
||||
"the test that caused it, not leak into the next one:\n" + code
|
||||
)
|
||||
@@ -35,9 +35,17 @@ def _env(monkeypatch, *, torch_file, triton=True):
|
||||
monkeypatch.setattr(
|
||||
engine_env, "settings_store", None, raising=False
|
||||
)
|
||||
import sys as _sys, types as _types
|
||||
_sys.modules["services.settings_store"] = _types.SimpleNamespace(
|
||||
get_text=lambda *a, **k: "0"
|
||||
# monkeypatch.setitem, never a raw assignment: a bare object dropped into
|
||||
# sys.modules leaks process-wide out of collection and breaks every later
|
||||
# import of the real module in a mixed run. backend/tests/
|
||||
# test_no_module_stubs.py exists to catch exactly that, and caught this.
|
||||
import sys as _sys
|
||||
import types as _types
|
||||
|
||||
monkeypatch.setitem(
|
||||
_sys.modules,
|
||||
"services.settings_store",
|
||||
_types.SimpleNamespace(get_text=lambda *a, **k: "0"),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
engine_env, "_cuda_arch_supported_for_compile", lambda: (True, "")
|
||||
|
||||
Reference in New Issue
Block a user