fix(download): reentra no acelerador na proxima tentativa apos queda

Achados do CodeRabbit no PR #1942.

O mais grave: com o erro classificado como transitorio, o codigo mantinha o
acelerador ligado mas caia direto no `snapshot_download` na MESMA tentativa. Se
esse download desse certo, o laco terminava e o manifesto do `.part` nunca era
reusado — exatamente o recomeco-do-zero que a correcao existe para impedir.

Agora o erro transitorio e propagado para o retry externo, cuja proxima
tentativa reentra no `_segmented_snapshot` e retoma do manifesto. A decisao
virou o helper puro `_segmented_retry_plan`, testavel direto (o laco mora dentro
de `install_model`, uma rota de ~200 linhas). A ultima tentativa fica reservada
para o caminho simples, entao o acelerador continua sem poder ser o motivo de um
install falhar de vez.

Tambem deste round de revisao:

- `Invoke-CimMethod ... Terminate` tinha o retorno descartado com `$null =`. O
  Win32_Process.Terminate reporta falha pelo ReturnValue, nao lancando: um kill
  negado por permissao era reportado como sucesso e a porta seguia presa. Agora
  o ReturnValue e validado, com exit 4 proprio e a mensagem carregando o codigo.
- O teste de concorrencia era vazio: o handler sincrono do MockTransport retorna
  antes de qualquer outra task rodar, entao `peak` nunca passava de 1 e a
  asserção `peak <= 4` passava sem exercitar o semaforo. Passou a segurar as
  requisicoes abertas com um asyncio.Event e a exigir `peak == 4` (verificado:
  com o semaforo afrouxado para 1000, o teste acusa 31).
- A doc dizia que OMNIVOICE_DOWNLOAD_MAX_WORKERS limita as faixas e que origem
  sem Range cai no snapshot_download. Nenhum dos dois: `_segmented_snapshot` nao
  passa `num_connections` (usa as 8 padrao) e origem sem Range vira stream unico
  dentro do proprio acelerador.
- Entradas de Highlights do CHANGELOG sem o `(#NNNN)` exigido.

CHANGELOG.md

@@ -11,8 +11,8 @@ the frozen-backend fallback mirror it for their toolchains.
 **Highlights**
 
 - Validate current-user Windows installers under a standard account on hosted runners (#1883)
-- Model downloads survive a flaky connection instead of restarting from zero
-- `bun run dev` recovers on Windows instead of demanding Task Manager
+- Model downloads survive a flaky connection instead of restarting from zero (#1940)
+- `bun run dev` recovers on Windows instead of demanding Task Manager (#1941)
 
 - The desktop app builds and opens from a fresh clone again (#1818) — thanks @flutterkage2k!
 - GPUs with less VRAM than the engine needs no longer get half the compute-time budget a CPU gets (#1806) — thanks @VishvakR!

backend/api/routers/setup/download.py

@@ -381,6 +381,28 @@ def _is_retryable_download_error(exc: BaseException) -> bool:
     return is_hf_connectivity_error(str(exc))
 
 
+def _segmented_retry_plan(
+    exc: BaseException, attempt: int, max_attempts: int
+) -> tuple[bool, bool]:
+    """What to do after the segmented accelerator failed on ``attempt``.
+
+    Returns ``(disable_accelerator, reraise)``.
+
+    A dropped connection is not the accelerator's fault, so the error is
+    re-raised for the outer retry: the next attempt re-enters
+    :func:`_segmented_snapshot`, which resumes from the ``.part`` manifest.
+    Falling straight through to ``snapshot_download`` instead would finish the
+    install from a separate ``.incomplete`` file and strand that manifest — the
+    restart-from-zero this exists to prevent.
+
+    The final attempt is always reserved for the plain path, so the accelerator
+    can never be the reason an install fails outright.
+    """
+    retryable = _is_retryable_download_error(exc)
+    disable = not retryable or attempt >= max_attempts - 1
+    return disable, not disable
+
+
 @router.post("/models/install")
 async def install_model(req: InstallModelRequest):
     """Download one HF repo snapshot; progress goes through the shared
@@ -574,18 +596,18 @@ async def install_model(req: InstallModelRequest):
                         except _InstallCancelled:
                             raise
                         except Exception as _seg_err:
-                            # A dropped connection is not the accelerator's
-                            # fault: keep it for the next attempt, which resumes
-                            # from the .part manifest instead of restarting at
-                            # zero. Anything else means the accelerator can't
-                            # work here — fall back for good.
-                            _segmented_off = not _is_retryable_download_error(_seg_err)
-                            logger.info(
-                                "segmented download for %s failed (%s); falling back to "
-                                "snapshot_download (accelerator %s)",
-                                req.repo_id, _seg_err,
-                                "disabled for this install" if _segmented_off else "kept for retry",
+                            _segmented_off, _seg_reraise = _segmented_retry_plan(
+                                _seg_err, _attempt, _max_attempts
                             )
+                            logger.info(
+                                "segmented download for %s failed (%s); accelerator %s",
+                                req.repo_id, _seg_err,
+                                "disabled for this install — falling back to snapshot_download"
+                                if _segmented_off
+                                else "kept for the next attempt (resumes from its manifest)",
+                            )
+                            if _seg_reraise:
+                                raise
                             _snapshot_path = None
                     if _snapshot_path is None:
                         _snapshot_path = snapshot_download(**dl_kwargs)  # nosec B615 -- immutable revision_for pin

docs/downloading-models.md

@@ -40,13 +40,17 @@ over parallel byte-ranges (IDM/uGet style), so the legacy-LFS path is no longer
 single-stream. It reports real live speed/ETA and **falls back to the normal
 download**, so it can never compromise a correct install.
 
-Ranges are capped at 16 MB each and run `OMNIVOICE_DOWNLOAD_MAX_WORKERS` at a
-time, so a completed range is committed to a resume manifest every few seconds.
-On a connection that drops mid-transfer, only the range in flight is refetched —
-the accelerator is kept for the retry and resumes from the manifest. It is
-disabled for the rest of the install only when it fails for a reason that is not
-transient network trouble (e.g. the origin refuses ranges), which is when the
-plain `snapshot_download` path takes over permanently. Adding a
+Ranges are capped at 16 MB each and eight of them are in flight at a time, so a
+completed range is committed to a resume manifest every few seconds. On a
+connection that drops mid-transfer, only the ranges in flight are refetched: the
+attempt is retried and the accelerator resumes from its manifest rather than
+starting the file over. An origin that does not serve ranges at all is handled
+inside the accelerator as a single stream, not as a failure.
+
+The accelerator is disabled for the rest of the install — handing over to the
+plain `snapshot_download` path — when it fails for a reason that is not
+transient network trouble, and on the install's final attempt, so it can never
+be the reason an install fails outright. Adding a
 free Hugging Face token (first-run setup, or Settings → Credentials) makes this
 faster still — authenticated downloads get higher rate limits and fewer stalls.
 To force the old single-stream path, set `OMNIVOICE_SEGMENTED_DOWNLOAD=0`.

scripts/clear-dev-ports.mjs

@@ -203,7 +203,10 @@ export function stopWindowsProcess(pid, _force, identity, run = spawnSync) {
     `$p = Get-CimInstance Win32_Process -Filter 'ProcessId = ${pid}'`,
     "if ($null -eq $p) { exit 0 }",
     `if ($p.CreationDate.ToUniversalTime().ToString('o') -ne '${expected}') { exit 3 }`,
-    "$null = Invoke-CimMethod -InputObject $p -MethodName Terminate",
+    // Terminate reports failure through ReturnValue, not through a thrown
+    // error: discarding it would report success on an access-denied kill.
+    "$r = Invoke-CimMethod -InputObject $p -MethodName Terminate",
+    "if ($r.ReturnValue -ne 0) { Write-Output $r.ReturnValue; exit 4 }",
   ].join("; ");
   const result = run(
     "powershell.exe",
@@ -212,7 +215,13 @@ export function stopWindowsProcess(pid, _force, identity, run = spawnSync) {
   );
   if (result.error) throw result.error;
   // exit 3 == the pid now belongs to a different process; leave it alone.
-  if (result.status !== 0 && result.status !== 3) {
+  if (result.status === 3) return;
+  if (result.status === 4) {
+    throw new Error(
+      `Could not stop process ${pid}: Terminate returned ${String(result.stdout || "").trim()}`,
+    );
+  }
+  if (result.status !== 0) {
     throw new Error(`Could not stop process ${pid}`);
   }
 }

tests/frontend/clearDevPorts.test.mjs

@@ -185,3 +185,17 @@ test("windows stop surfaces a real termination failure", () => {
     /Could not stop process 4242/,
   );
 });
+
+// Win32_Process.Terminate reports failure through ReturnValue, not by throwing:
+// discarding it would report success on an access-denied kill, and the port
+// would still be held.
+test("windows stop fails when Terminate reports a non-zero ReturnValue", () => {
+  const run = (_exe, args) => {
+    assert.match(args.at(-1), /ReturnValue -ne 0/);
+    return { status: 4, stdout: "2" };
+  };
+  assert.throws(
+    () => stopWindowsProcess(4242, false, "windows:whatever", run),
+    /Could not stop process 4242: Terminate returned 2/,
+  );
+});

tests/test_fdl_segmented_download.py

@@ -142,29 +142,57 @@ def test_small_file_still_single_segment():
 
 
 def test_concurrency_stays_at_num_connections(tmp_path, monkeypatch):
-    """Many bounded segments must not all fire at once."""
+    """Many bounded segments must not all fire at once.
+
+    The handler has to HOLD requests open: a synchronous mock returns before any
+    other task is scheduled, so nothing ever overlaps and the assertion passes
+    without exercising the semaphore at all.
+    """
     monkeypatch.setattr(sd, "_MIN_SEGMENT_BYTES", 16 * 1024)
     monkeypatch.setattr(sd, "_MAX_SEGMENT_BYTES", 32 * 1024)
-    inflight = 0
-    peak = 0
-
-    def handler(request: httpx.Request) -> httpx.Response:
-        nonlocal inflight, peak
-        if request.method == "HEAD":
-            return httpx.Response(200, headers={
-                "content-length": str(len(PAYLOAD)), "accept-ranges": "bytes"})
-        inflight += 1
-        peak = max(peak, inflight)
-        try:
-            lo, hi = request.headers["range"].replace("bytes=", "").split("-")
-            return httpx.Response(206, content=PAYLOAD[int(lo):int(hi) + 1])
-        finally:
-            inflight -= 1
-
+    connections = 4
+    assert len(_plan_segments(len(PAYLOAD), connections)) > connections, (
+        "test needs more segments than connections"
+    )
     dest = str(tmp_path / "m.bin")
-    _download(handler, dest, expected_size=len(PAYLOAD), num_connections=4)
-    assert len(_plan_segments(len(PAYLOAD), 4)) > 4, "test needs more segments than connections"
-    assert peak <= 4
+    state = {"inflight": 0, "peak": 0}
+
+    async def _run():
+        saturated = asyncio.Event()
+
+        async def handler(request: httpx.Request) -> httpx.Response:
+            if request.method == "HEAD":
+                return httpx.Response(200, headers={
+                    "content-length": str(len(PAYLOAD)), "accept-ranges": "bytes"})
+            state["inflight"] += 1
+            state["peak"] = max(state["peak"], state["inflight"])
+            try:
+                if state["inflight"] >= connections:
+                    saturated.set()
+                # Hold the range open until the pool fills, so overlap is
+                # observable without sleeping. The timeout keeps an
+                # over-restrictive semaphore a failure instead of a hang.
+                try:
+                    await asyncio.wait_for(saturated.wait(), timeout=1)
+                except asyncio.TimeoutError:
+                    pass
+                lo, hi = request.headers["range"].replace("bytes=", "").split("-")
+                return httpx.Response(206, content=PAYLOAD[int(lo):int(hi) + 1])
+            finally:
+                state["inflight"] -= 1
+
+        async with httpx.AsyncClient(
+            transport=httpx.MockTransport(handler), follow_redirects=False
+        ) as client:
+            await segmented_download(
+                "https://cdn.example.com/f.bin", dest, client=client,
+                expected_size=len(PAYLOAD), num_connections=connections,
+            )
+
+    asyncio.run(_run())
+    assert state["peak"] == connections, (
+        f"expected exactly {connections} ranges in flight, saw {state['peak']}"
+    )
     with open(dest, "rb") as f:
         assert f.read() == PAYLOAD
 

tests/test_segmented_download_default.py

@@ -28,3 +28,42 @@ def test_env_override_truthy_keeps_it_on(monkeypatch):
     for val in ("1", "true", "on", "yes"):
         monkeypatch.setenv("OMNIVOICE_SEGMENTED_DOWNLOAD", val)
         assert _segmented_enabled() is True
+
+
+# The accelerator must be re-entered on the NEXT attempt after a dropped
+# connection, so it resumes from its .part manifest. Falling straight through to
+# snapshot_download in the same attempt finishes the install from a separate
+# .incomplete file and strands the manifest — restart-from-zero all over again.
+
+import httpx  # noqa: E402
+
+from api.routers.setup.download import _segmented_retry_plan  # noqa: E402
+
+_MAX = 5
+
+
+def _dropped():
+    return httpx.RemoteProtocolError(
+        "peer closed connection without sending complete message body"
+    )
+
+
+def test_dropped_connection_reraises_so_the_next_attempt_resumes():
+    for attempt in (1, 2, 3):
+        disable, reraise = _segmented_retry_plan(_dropped(), attempt, _MAX)
+        assert reraise is True, f"attempt {attempt} must reach the outer retry"
+        assert disable is False, f"attempt {attempt} must keep the accelerator"
+
+
+def test_final_attempt_is_reserved_for_the_plain_path():
+    """The accelerator can never be the reason an install fails outright."""
+    disable, reraise = _segmented_retry_plan(_dropped(), _MAX - 1, _MAX)
+    assert (disable, reraise) == (True, False)
+    disable, reraise = _segmented_retry_plan(_dropped(), _MAX, _MAX)
+    assert (disable, reraise) == (True, False)
+
+
+def test_a_non_network_failure_disables_the_accelerator_at_once():
+    """An accelerator that cannot work here must not burn every retry."""
+    disable, reraise = _segmented_retry_plan(ValueError("sha256 mismatch"), 1, _MAX)
+    assert (disable, reraise) == (True, False)