fix(download): #1224 review — replace the source-grep tests with real ones

CodeRabbit's Major finding was fair and the most useful of the set: three of
my tests asserted that a NAME appeared in a module's source. The installer one
passed merely because the module imports the symbol — it would not have
noticed the retry loop ignoring the classifier entirely. Tests that can't fail
for the reason they exist are worse than no tests.

Replaced with behaviour:

- the installer's retry decision is now a named helper, tested against a REAL
  httpx.RemoteProtocolError instance (plus a cancel, a bad repo id, a 401, and
  the original type-based cases so widening didn't drop them);
- VoxCPM2's loader is driven through a fake voxcpm module that truncates twice
  then succeeds, asserting three calls — and one that fails fast on a real
  error.

The stream-path check stays structural (reaching it needs a live WebSocket)
but now also resolves the symbol it names, so a rename on either side fails.

Two P1s fixed as well:

- the closed-client reset incremented the same counter as the download
  retries, leaving a resumable multi-GB download one attempt short of its
  configured budget. The two budgets are now genuinely independent.
- OMNIVOICE_MODEL_LOAD_BACKOFF_S=inf parsed fine and made sleep(inf) raise
  OverflowError, replacing a retryable failure with an unrelated crash that
  hid the original error. Non-finite values fall back to the default.

CHANGELOG entries shortened (CodeRabbit).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

CHANGELOG.md

@@ -14,9 +14,9 @@ The bundled TTS model package (`pyproject.toml`) is versioned independently.
 
 ### Fixed
 
-- A truncated model download (`peer closed connection without sending complete message body`) is now retried and resumed; it arrives as a transport error that isn't an `OSError`, so it escaped the installer's retry loop entirely and aborted multi-GB installs near the end (#1224) — thanks @Reaksa-Cambodia!
+- A model download truncated near the end is now retried and resumed instead of aborting the whole install — thanks @Reaksa-Cambodia! (#1224)
 - Engine first-use downloads (VoxCPM2, MOSS-TTS-Nano) retry transient network failures instead of failing the load outright (#1224)
-- The streaming synth path now logs the low-memory advisory before loading a model, so a backend killed by the OS leaves a trail in the crash report — only the non-streaming path did (#1224)
+- A backend killed by the OS mid-stream now leaves a low-memory trail in the crash report (#1224)
 
 ## [0.4.0] — 2026-07-21
 

backend/api/routers/setup/download.py

@@ -321,6 +321,31 @@ class InstallModelRequest(BaseModel):
     repo_id: str
 
 
+
+def _is_retryable_download_error(exc: BaseException) -> bool:
+    """Whether a failed download attempt is worth retrying.
+
+    Decides by CLASSIFICATION, not by exception type. The type-based tuple this
+    replaced — ``(HfHubHTTPError, LocalEntryNotFoundError, OSError)`` — silently
+    excluded ``httpx.RemoteProtocolError``, which inherits ``Exception``: a
+    4.6 GB model truncated at 4.0 GB escaped all five attempts and aborted the
+    install (#1224). Any future transport error with a novel base class would
+    have reopened the same hole.
+
+    A user cancel is never retryable, and neither is anything
+    ``is_hf_connectivity_error`` does not recognise.
+    """
+    # Imported here, not at module scope, for the same reason the worker does:
+    # huggingface_hub is heavy and this module is on the setup import path.
+    from huggingface_hub.utils import HfHubHTTPError, LocalEntryNotFoundError
+
+    if isinstance(exc, _InstallCancelled):
+        return False
+    if isinstance(exc, (HfHubHTTPError, LocalEntryNotFoundError, OSError)):
+        return True
+    return is_hf_connectivity_error(str(exc))
+
+
 @router.post("/models/install")
 async def install_model(req: InstallModelRequest):
     """Download one HF repo snapshot; progress goes through the shared
@@ -506,12 +531,9 @@ async def install_model(req: InstallModelRequest):
                     # truncation signatures. Anything unrecognised (a cancel, a
                     # validation failure, a bug) propagates untouched, exactly
                     # as before.
-                    if isinstance(net_err, _InstallCancelled):
-                        raise
-                    _retryable = isinstance(
-                        net_err, (HfHubHTTPError, LocalEntryNotFoundError, OSError)
-                    ) or is_hf_connectivity_error(str(net_err))
-                    if _attempt >= _max_attempts or not _retryable:
+                    if _attempt >= _max_attempts or not _is_retryable_download_error(
+                        net_err
+                    ):
                         raise
                     _backoff = min(30, 2 ** _attempt)
                     logger.info(

backend/services/tts_backend.py

@@ -132,7 +132,6 @@ def _retry_once_with_fresh_hf_client(loader, what: str):
     client_reset_used = False
     attempt = 0
     while True:
-        attempt += 1
         try:
             return loader()
         except Exception as e:
@@ -152,7 +151,12 @@ def _retry_once_with_fresh_hf_client(loader, what: str):
                         "%s: couldn't reset the HF Hub client; retrying anyway.",
                         what,
                     )
+                # Deliberately does NOT consume a download attempt: the two
+                # budgets are independent, and letting the session reset eat
+                # one left a resumable multi-GB download a retry short of its
+                # configured budget (#1224 review).
                 continue  # immediate — nothing to back off from
+            attempt += 1
             if not is_hf_connectivity_error(str(e)) or attempt >= attempts:
                 raise
             logger.warning(
@@ -174,9 +178,15 @@ def _int_env(name: str, default: int) -> int:
 
 def _float_env(name: str, default: float) -> float:
     try:
-        return float(os.environ.get(name, default))
+        value = float(os.environ.get(name, default))
     except (TypeError, ValueError):
         return default
+    # inf/nan parse fine and then poison the caller: `sleep(inf)` raises
+    # OverflowError, turning a retryable download failure into an unrelated
+    # crash that hides the original error (#1224 review).
+    if value != value or value in (float("inf"), float("-inf")):
+        return default
+    return value
 
 
 # ── Protocol ────────────────────────────────────────────────────────────────

tests/test_truncated_download_retry.py

@@ -143,32 +143,111 @@ def test_the_closed_client_path_stays_single_shot(monkeypatch):
     assert len(calls) == 2, "closed-client must stay single-shot regardless of the budget"
 
 
-def test_voxcpm2_load_goes_through_the_retry_wrapper():
-    """The #1224 call site itself — a direct from_pretrained here would mean
-    the fix above never runs for the engine the reporter was using."""
-    import inspect
+def test_voxcpm2_load_actually_retries_a_truncated_download(monkeypatch):
+    """The #1224 call site itself, exercised — not grepped.
 
-    src = inspect.getsource(tts_backend.VoxCPM2Backend._ensure_loaded)
-    assert "_retry_once_with_fresh_hf_client" in src
-    assert "VoxCPM.from_pretrained(checkpoint" not in src.replace(
-        "lambda: VoxCPM.from_pretrained(checkpoint", ""
+    An earlier version of this test asserted the wrapper's NAME appeared in the
+    method source, which would have passed even if the wrapper were called with
+    the wrong argument or its result discarded (#1224 review)."""
+    import sys
+    import types
+
+    calls = []
+
+    class _FakeVoxCPM:
+        @staticmethod
+        def from_pretrained(checkpoint, **kw):
+            calls.append(checkpoint)
+            if len(calls) < 3:
+                raise RuntimeError(
+                    "peer closed connection without sending complete message body"
+                )
+            return f"model:{checkpoint}"
+
+    monkeypatch.setitem(
+        sys.modules, "voxcpm", types.SimpleNamespace(VoxCPM=_FakeVoxCPM)
     )
+    monkeypatch.setenv("OMNIVOICE_VOXCPM_MODEL", "openbmb/VoxCPM2")
+    monkeypatch.setattr(
+        tts_backend.VoxCPM2Backend, "is_available", classmethod(lambda cls: (True, ""))
+    )
+    monkeypatch.setattr(tts_backend, "_voxcpm_upgrade_hint", lambda: None)
+
+    backend = tts_backend.VoxCPM2Backend()
+    backend._ensure_loaded()
+
+    assert backend._model == "model:openbmb/VoxCPM2"
+    assert len(calls) == 3, "the load must retry, not fail on the first truncation"
+
+
+def test_voxcpm2_load_still_fails_fast_on_a_real_error(monkeypatch):
+    import sys
+    import types
+
+    calls = []
+
+    class _FakeVoxCPM:
+        @staticmethod
+        def from_pretrained(checkpoint, **kw):
+            calls.append(1)
+            raise ValueError("checkpoint has no config.json")
+
+    monkeypatch.setitem(
+        sys.modules, "voxcpm", types.SimpleNamespace(VoxCPM=_FakeVoxCPM)
+    )
+    monkeypatch.setattr(
+        tts_backend.VoxCPM2Backend, "is_available", classmethod(lambda cls: (True, ""))
+    )
+    monkeypatch.setattr(tts_backend, "_voxcpm_upgrade_hint", lambda: None)
+
+    with pytest.raises(ValueError):
+        tts_backend.VoxCPM2Backend()._ensure_loaded()
+    assert len(calls) == 1
 
 
 # ── the installer's retry loop catches it ────────────────────────────────
 
 
-def test_installer_retry_catches_non_oserror_transport_failures():
-    """RemoteProtocolError is not an OSError — the loop must decide by
-    classification, not by exception type."""
-    import inspect
+def test_installer_retries_a_real_remoteprotocolerror():
+    """The #1224 root cause, against a REAL httpx exception instance.
 
-    from api.routers.setup import download
+    An earlier version asserted `"is_hf_connectivity_error" in getsource(...)`,
+    which passed merely because the module imports it — it would not have
+    noticed the loop ignoring the classifier entirely (#1224 review)."""
+    import httpx
 
-    src = inspect.getsource(download)
-    assert "is_hf_connectivity_error" in src, (
-        "the install retry loop must classify failures, not match OSError only"
+    from api.routers.setup.download import _is_retryable_download_error
+
+    truncated = httpx.RemoteProtocolError(
+        "peer closed connection without sending complete message body "
+        "(received 4084175097 bytes, expected 4580080592)"
     )
+    assert not isinstance(truncated, OSError), (
+        "if this ever becomes an OSError the original bug is gone, but the "
+        "classification path must still hold"
+    )
+    assert _is_retryable_download_error(truncated)
+
+
+def test_installer_does_not_retry_a_cancel_or_a_real_error():
+    from api.routers.setup.download import (
+        _InstallCancelled,
+        _is_retryable_download_error,
+    )
+
+    assert not _is_retryable_download_error(_InstallCancelled())
+    assert not _is_retryable_download_error(ValueError("no such repo"))
+    assert not _is_retryable_download_error(RuntimeError("401 Unauthorized"))
+
+
+def test_installer_still_retries_the_original_type_based_cases():
+    """Widening to classification must not drop what the old tuple caught."""
+    from huggingface_hub.utils import LocalEntryNotFoundError
+
+    from api.routers.setup.download import _is_retryable_download_error
+
+    assert _is_retryable_download_error(OSError("connection reset by peer"))
+    assert _is_retryable_download_error(LocalEntryNotFoundError("offline"))
 
 
 # ── the streaming path leaves an OOM breadcrumb ──────────────────────────
@@ -178,11 +257,52 @@ def test_stream_path_checks_memory_before_loading():
     """The reporter was SIGKILLed on a 16 GB Mac. /generate has logged a
     low-memory advisory since the earlier reports of that class, but the
     STREAMING path — which the desktop UI tries first — did not, so the load
-    most likely to tip the machine over was the one with no trail in the
-    captured stderr tail."""
+    most likely to tip the machine over left no trail in the captured stderr
+    tail a SIGKILL report has to go on.
+
+    Structural rather than behavioural: reaching the call needs a live
+    WebSocket session. Kept honest by also resolving the symbol it names, so a
+    rename or removal on either side fails here (#1224 review).
+    """
     import inspect
 
     from api.routers import tts_stream
+    from services.memory_budget import log_if_low
 
+    assert callable(log_if_low)
     src = inspect.getsource(tts_stream)
-    assert "log_if_low" in src
+    assert "from services.memory_budget import log_if_low" in src
+    assert "log_if_low(f\"TTS stream load" in src
+
+
+def test_the_two_budgets_do_not_share_a_counter(monkeypatch):
+    """Review finding (#1224): the closed-client reset incremented the same
+    counter as the download retries, so a session reset followed by transient
+    failures left a resumable multi-GB download one attempt short of its
+    configured budget."""
+    monkeypatch.setenv("OMNIVOICE_MODEL_LOAD_RETRIES", "3")
+    import huggingface_hub.utils as hub_utils
+
+    monkeypatch.setattr(hub_utils, "close_session", lambda: None, raising=False)
+
+    calls = []
+
+    def loader():
+        calls.append(1)
+        if len(calls) == 1:
+            raise RuntimeError("Cannot send a request, as the client has been closed.")
+        raise RuntimeError("peer closed connection without sending complete message body")
+
+    with pytest.raises(RuntimeError):
+        tts_backend._retry_once_with_fresh_hf_client(loader, "VoxCPM2")
+    # 1 closed-client + a full budget of 3 download attempts.
+    assert len(calls) == 4
+
+
+@pytest.mark.parametrize("bad", ["inf", "-inf", "nan"])
+def test_a_non_finite_backoff_falls_back_to_the_default(monkeypatch, bad):
+    """Review finding (#1224): `float("inf")` parses fine and then makes
+    `sleep(inf)` raise OverflowError, replacing a retryable download failure
+    with an unrelated crash that hides the original error."""
+    monkeypatch.setenv("OMNIVOICE_MODEL_LOAD_BACKOFF_S", bad)
+    assert tts_backend._float_env("OMNIVOICE_MODEL_LOAD_BACKOFF_S", 2.0) == 2.0