From 850ae8e192c2e6502fa9a63f9054cf0607ee1d8d Mon Sep 17 00:00:00 2001 From: Palash Debnath Date: Thu, 30 Jul 2026 01:30:06 -0700 Subject: [PATCH] test: reset model-manager shutdown state between backend tests (#1320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 * 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 --------- Co-authored-by: Claude Opus 5 --- backend/tests/conftest.py | 39 ++++++++++++ .../tests/test_shutdown_state_isolation.py | 59 +++++++++++++++++++ tests/test_torch_compile_path_gate.py | 14 ++++- 3 files changed, 109 insertions(+), 3 deletions(-) create mode 100644 backend/tests/test_shutdown_state_isolation.py diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 0f583a4a..504ec728 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -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() diff --git a/backend/tests/test_shutdown_state_isolation.py b/backend/tests/test_shutdown_state_isolation.py new file mode 100644 index 00000000..cb736222 --- /dev/null +++ b/backend/tests/test_shutdown_state_isolation.py @@ -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 + ) diff --git a/tests/test_torch_compile_path_gate.py b/tests/test_torch_compile_path_gate.py index c5ccf2cb..b920ecab 100644 --- a/tests/test_torch_compile_path_gate.py +++ b/tests/test_torch_compile_path_gate.py @@ -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, "")