diff --git a/hermes_cli/local_runtime/binaries.py b/hermes_cli/local_runtime/binaries.py index 11844a6dab..a7b98c9296 100644 --- a/hermes_cli/local_runtime/binaries.py +++ b/hermes_cli/local_runtime/binaries.py @@ -10,8 +10,10 @@ import platform import shutil import subprocess import tempfile +import time import urllib.request import zipfile +from contextlib import suppress from dataclasses import dataclass, field from pathlib import Path from typing import Callable @@ -182,6 +184,36 @@ def _sha256(path: Path) -> str: return h.hexdigest() +# How long a finished download may wait for another process to let go of it before the +# download is reported as failed. +_RELEASE_WAIT_SECONDS = 60.0 + + +def replace_when_released(tmp: Path, dest: Path, *, timeout: float = _RELEASE_WAIT_SECONDS) -> None: + """Rename a finished download into place, waiting out a transient hold on the file. + + On Windows a file whose last write handle just closed is often still open to an antivirus + or indexing scan, and renaming it fails with a permission error until the scan lets go — + for a multi-gigabyte model that can take many seconds. ``os.replace`` is retried through + that window; it never falls back to copying (``shutil.move`` does, which duplicates the whole + file and then reports the leftover's failed delete as the download's failure). A hold that + outlasts the window raises a plain-language error. + """ + deadline = time.monotonic() + timeout + delay = 0.1 + while True: + try: + os.replace(tmp, dest) + return + except PermissionError as exc: + if time.monotonic() >= deadline: + raise RuntimeError( + f"The download finished, but another program (usually an antivirus scan) kept " + f"{tmp.name} open and it could not be renamed into place. Please try again.") from exc + time.sleep(delay) + delay = min(delay * 2, 2.0) + + def _download(url: str, dest: Path, progress: "Callable[[int, int], None] | None" = None) -> None: """Stream url -> dest. ``progress(done_bytes, total_bytes)`` ticks per chunk (total 0 when @@ -207,9 +239,11 @@ def _download(url: str, dest: Path, if length is not None and done != total: raise BinaryResolutionError( f"incomplete download for {dest.name}: expected {total} bytes, got {done}") - tmp.replace(dest) + replace_when_released(tmp, dest) finally: - tmp.unlink(missing_ok=True) + # Best effort: a leftover that cannot be removed must not hide the error that left it. + with suppress(OSError): + tmp.unlink(missing_ok=True) def _extract(archive: Path, dest: Path, diff --git a/hermes_cli/web_routers/local_models.py b/hermes_cli/web_routers/local_models.py index 907123778a..c52a43a66f 100644 --- a/hermes_cli/web_routers/local_models.py +++ b/hermes_cli/web_routers/local_models.py @@ -380,9 +380,12 @@ def download_file(url: str, dest: Path, job: Dict[str, Any], *, base_done: int = if length and file_done[0] != length: raise RuntimeError(f"Download ended at {file_done[0]:,} bytes but the server " f"said {length:,} — connection dropped? Removed; try again") - shutil.move(str(tmp), str(dest)) + job["detail"] = "Finishing" + binaries.replace_when_released(tmp, dest) except Exception: - tmp.unlink(missing_ok=True) + # Best effort: a leftover that cannot be removed must not hide the error that left it. + with contextlib.suppress(OSError): + tmp.unlink(missing_ok=True) raise diff --git a/tests/hermes_cli/test_local_models_routes.py b/tests/hermes_cli/test_local_models_routes.py index 0ed8891aad..7ff868db92 100644 --- a/tests/hermes_cli/test_local_models_routes.py +++ b/tests/hermes_cli/test_local_models_routes.py @@ -8,6 +8,7 @@ from __future__ import annotations import io import json +import os import time from pathlib import Path @@ -358,3 +359,97 @@ def test_download_tolerates_stale_catalog_size(client, monkeypatch): break time.sleep(0.05) assert status is not None and status["status"] == "done", status.get("error") + + +def test_download_survives_a_held_finished_file(client, monkeypatch): + """The finished .part is often still open to an antivirus scan when the rename runs (Windows), + which refuses it with a permission error. The job must wait the hold out and land the file — + not copy it, and not report a complete download as failed.""" + + body = b"x" * 48 + + class FakeResponse(io.BytesIO): + headers = {"Content-Length": str(len(body))} + + def __enter__(self): + return self + + def __exit__(self, *a): + return False + + monkeypatch.setattr("urllib.request.urlopen", lambda *a, **k: FakeResponse(body)) + + real_replace = os.replace + refusals = [] + + def held_at_first(src, dst): + if str(src).endswith(".part") and len(refusals) < 2: + refusals.append(src) + raise PermissionError(13, "Access is denied") + real_replace(src, dst) + + monkeypatch.setattr(os, "replace", held_at_first) + + from hermes_cli.local_runtime.estimator import HardwareBudget + + budget = HardwareBudget(usable_vram_bytes=64 << 30, total_device_bytes=64 << 30, + ram_available_bytes=64 << 30) + monkeypatch.setattr("hermes_cli.local_runtime.hardware.probe_budget", lambda **kw: budget) + monkeypatch.setattr("hermes_cli.local_runtime.bootstrap.refresh_local_runtime", lambda: False) + + from hermes_cli.local_runtime.bootstrap import models_dir + from hermes_cli.local_runtime.catalog import CATALOG + + entry_id = CATALOG[0].id + r = client.post("/api/local-models/download", json={"model_id": entry_id}) + assert r.status_code == 200 + job_id = r.json()["job_id"] + + deadline = time.time() + 10 + status = None + while time.time() < deadline: + status = client.get(f"/api/local-models/jobs/{job_id}").json() + if status["status"] in ("done", "error"): + break + time.sleep(0.05) + assert status is not None and status["status"] == "done", status.get("error") + assert len(refusals) == 2 + assert not list(models_dir().glob("*.part")) + assert any(p.read_bytes() == body for p in models_dir().glob("*.gguf")) + + +def test_download_failure_is_not_masked_by_a_stuck_leftover(tmp_path, monkeypatch): + """When the rename gives up, the user must see that message — not the error from the + cleanup that could not remove the still-held .part either.""" + from hermes_cli.web_routers import local_models + + body = b"x" * 48 + + class FakeResponse(io.BytesIO): + headers = {"Content-Length": str(len(body))} + + def __enter__(self): + return self + + def __exit__(self, *a): + return False + + monkeypatch.setattr("urllib.request.urlopen", lambda *a, **k: FakeResponse(body)) + + def still_held(tmp, dest, **kw): + raise RuntimeError("could not be renamed into place") + + monkeypatch.setattr("hermes_cli.local_runtime.binaries.replace_when_released", still_held) + real_unlink = Path.unlink + + def stuck_part(self, missing_ok=False): + if self.suffix == ".part": + raise PermissionError(13, "Access is denied") + return real_unlink(self, missing_ok=missing_ok) + + monkeypatch.setattr(Path, "unlink", stuck_part) + + dest = tmp_path / "models" / "model.gguf" + with pytest.raises(RuntimeError, match="could not be renamed"): + local_models.download_file("http://example.invalid/model.gguf", dest, {}) + assert not dest.exists() diff --git a/tests/hermes_cli/test_local_runtime_downloads.py b/tests/hermes_cli/test_local_runtime_downloads.py index c0ac8558e4..60f741ddaa 100644 --- a/tests/hermes_cli/test_local_runtime_downloads.py +++ b/tests/hermes_cli/test_local_runtime_downloads.py @@ -1,5 +1,6 @@ """Runtime downloads must not turn a transient transfer failure into a poisoned cache.""" +import os import threading from concurrent.futures import ThreadPoolExecutor from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer @@ -94,3 +95,43 @@ def test_failed_request_preserves_another_active_download(tmp_path, asset_server assert dest.read_bytes() == payload assert not list(tmp_path.glob("*.part")) assert len(requests) == 2 + + +def test_replace_waits_out_a_transient_hold(tmp_path, monkeypatch): + """A rename refused while another process still holds the finished file (antivirus and + indexing scans on Windows) must be retried, not degraded to a copy or reported as a + failed download.""" + tmp = tmp_path / "model.part" + dest = tmp_path / "model.gguf" + tmp.write_bytes(b"weights") + real_replace = os.replace + refusals = [] + + def held_twice(src, dst): + if len(refusals) < 2: + refusals.append(src) + raise PermissionError(13, "Access is denied") + real_replace(src, dst) + + monkeypatch.setattr(os, "replace", held_twice) + binaries.replace_when_released(tmp, dest, timeout=5) + assert len(refusals) == 2 + assert dest.read_bytes() == b"weights" + assert not tmp.exists() + + +def test_replace_gives_up_with_a_plain_language_error(tmp_path, monkeypatch): + tmp = tmp_path / "model.part" + dest = tmp_path / "model.gguf" + tmp.write_bytes(b"weights") + + def always_held(src, dst): + raise PermissionError(13, "Access is denied") + + monkeypatch.setattr(os, "replace", always_held) + with pytest.raises(RuntimeError) as failed: + binaries.replace_when_released(tmp, dest, timeout=0.3) + assert "model.part" in str(failed.value) + assert "try again" in str(failed.value).lower() + assert isinstance(failed.value.__cause__, PermissionError) + assert not dest.exists()