fix(backend): shutdown wait bound 3s→20s — post-merge review finding on #1002; absorb #1015's design-path test (#1020)
Greptile's review of the merged #1002 flagged a real residual gap: a cold transformers import alone can exceed the 3s shutdown wait, and cancelling the asyncio task doesn't stop the underlying OS thread — so quitting during an unusually slow preload could still let shutdown report "done" while that thread was alive, the exact #1000 class with lower odds. Python cannot forcibly kill a running thread, so no finite bound eliminates this outright; 20s shrinks the window from "any preload" to "an unusually slow cold-import," the practical ceiling before a long shutdown becomes its own complaint. New source-level contract test pins the production bound at ≥15s so a future edit can't quietly shrink it back without deliberate consideration. Also absorbs the one test case from community PR #1015 (superseded by the earlier-merged #1017, which duplicated it — my fault for not checking the PR queue) that the merged version lacked: the design/instruct path with no ref kwargs at all stays untouched by the ref_text forwarding fix. Co-authored-by: mergetest <test@local> Co-authored-by: MahdiHedhli <noreply@github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
mergetest
MahdiHedhli
Claude Fable 5
parent
93a3cb260a
commit
69ce697ee5
+13
-1
@@ -688,8 +688,20 @@ async def lifespan(app: FastAPI):
|
||||
# (still importing, not yet mid weight-download) finish cleanly before we
|
||||
# report done; a load that's genuinely deep into a multi-GB download still
|
||||
# times out — _reset_gpu_pool() below abandons it either way.
|
||||
#
|
||||
# 20s, not the original 3s (code-review finding post-merge): a cold
|
||||
# transformers import alone can take longer than 3s on a slow disk or a
|
||||
# first-ever launch, so the original bound left a real residual window —
|
||||
# cancellation detaches the asyncio task, but the underlying OS thread
|
||||
# keeps running past it, and shutdown could still report "done" while
|
||||
# that thread was alive. Python cannot forcibly kill a running thread, so
|
||||
# no finite bound eliminates this outright — 20s just shrinks the window
|
||||
# from "any preload" to "an unusually slow cold-import," which is the
|
||||
# practical ceiling before a longer shutdown itself becomes the
|
||||
# complaint. A thread that's still running past 20s was never going to
|
||||
# finish in a shutdown-appropriate timeframe regardless.
|
||||
await _cancel_and_await_tasks(
|
||||
idle_task, worker_task, preload_task, capture_preload_task, timeout=3.0,
|
||||
idle_task, worker_task, preload_task, capture_preload_task, timeout=20.0,
|
||||
)
|
||||
# Unload the model and free GPU memory
|
||||
try:
|
||||
|
||||
@@ -400,6 +400,27 @@ def test_mlx_audio_generate_omits_ref_text_without_ref_audio():
|
||||
assert "ref_text" not in captured
|
||||
|
||||
|
||||
def test_mlx_audio_generate_design_path_unaffected_without_any_ref():
|
||||
# Absorbed from community PR #1015 (MahdiHedhli) — the design/instruct
|
||||
# path (no ref_audio, no ref_text at all) must stay untouched by the
|
||||
# ref_text forwarding fix; neither kwarg may leak into the model call.
|
||||
pytest.importorskip("mlx_audio", reason="mlx-audio is Apple-Silicon-only")
|
||||
backend = tts_backend.MLXAudioBackend()
|
||||
backend._ensure_loaded = lambda: None
|
||||
|
||||
captured = {}
|
||||
|
||||
def _fake_generate(**kw):
|
||||
captured.update(kw)
|
||||
return iter([types.SimpleNamespace(audio=__import__("numpy").zeros(4))])
|
||||
|
||||
backend._model = types.SimpleNamespace(generate=_fake_generate)
|
||||
backend.generate("hello")
|
||||
|
||||
assert "ref_text" not in captured
|
||||
assert "ref_audio" not in captured
|
||||
|
||||
|
||||
def test_mlx_audio_generate_auto_language_skips_lang_code_entirely():
|
||||
# Matches the "Auto" convention other engines in this file use
|
||||
# (OmniVoiceBackend.generate(), _run_backend_inference) — never resolved,
|
||||
|
||||
@@ -110,3 +110,34 @@ def test_multiple_tasks_are_all_cancelled_before_any_await():
|
||||
|
||||
_run(_scenario())
|
||||
assert set(cancelled_order) == {"slow", "fast"}
|
||||
|
||||
|
||||
def test_production_shutdown_wait_is_generous_enough_for_a_cold_import():
|
||||
"""Post-merge code-review finding (Greptile, PR #1002): the original 3s
|
||||
bound left a real residual window — cancelling the asyncio task doesn't
|
||||
stop the underlying OS thread, so a cold transformers import taking
|
||||
longer than the bound could still let shutdown report "done" while that
|
||||
thread was alive, the exact #1000 class again just with lower odds.
|
||||
Python can't forcibly kill a running thread, so no finite bound
|
||||
eliminates this outright — this pins the production call site to a
|
||||
materially more generous wait (20s, not 3s) rather than letting a future
|
||||
edit quietly shrink it back down without deliberate consideration.
|
||||
|
||||
Source-level guard, not a live-timing test: driving an actual >3s cold
|
||||
import through this suite would make it slow and environment-dependent
|
||||
for no real benefit.
|
||||
"""
|
||||
import re
|
||||
|
||||
src = open(os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file__))),
|
||||
"backend", "main.py")).read()
|
||||
call = re.search(
|
||||
r"await _cancel_and_await_tasks\(\s*idle_task,\s*worker_task,\s*preload_task,"
|
||||
r"\s*capture_preload_task,\s*timeout=([\d.]+),?\s*\)",
|
||||
src,
|
||||
)
|
||||
assert call, "production shutdown call site not found in main.py"
|
||||
assert float(call.group(1)) >= 15.0, (
|
||||
f"shutdown wait bound regressed to {call.group(1)}s — see PR #1002 review history "
|
||||
"before shrinking this"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user