From 745e3264ff8eea4a5c9d672e784b3f607b4b8abc Mon Sep 17 00:00:00 2001 From: Ryan Hill <38674843+RyanUnderhill@users.noreply.github.com> Date: Tue, 15 Sep 2026 16:56:22 -0700 Subject: [PATCH 1/3] fix(local-runtime): reject incomplete downloads and repair corrupt cache --- hermes_cli/local_runtime/binaries.py | 72 ++++++---- .../test_local_runtime_downloads.py | 135 ++++++++++++++++++ 2 files changed, 182 insertions(+), 25 deletions(-) create mode 100644 tests/hermes_cli/test_local_runtime_downloads.py diff --git a/hermes_cli/local_runtime/binaries.py b/hermes_cli/local_runtime/binaries.py index d18985646c..9b62264643 100644 --- a/hermes_cli/local_runtime/binaries.py +++ b/hermes_cli/local_runtime/binaries.py @@ -2,6 +2,7 @@ from __future__ import annotations +import gzip import hashlib import json import logging @@ -9,8 +10,10 @@ import os import platform import shutil import subprocess +import tarfile import urllib.request import zipfile +import zlib from dataclasses import dataclass, field from pathlib import Path from typing import Callable @@ -186,18 +189,26 @@ def _download(url: str, dest: Path, the server sends no Content-Length) — a several-hundred-MB archive must never look hung.""" logger.info("downloading %s", url) tmp = dest.with_suffix(dest.suffix + ".part") - with urllib.request.urlopen(url, timeout=120) as r, open(tmp, "wb") as f: - total = int(r.headers.get("Content-Length") or 0) - done = 0 - while True: - chunk = r.read(1 << 20) - if not chunk: - break - f.write(chunk) - done += len(chunk) - if progress is not None: - progress(done, total) - tmp.replace(dest) + try: + with urllib.request.urlopen(url, timeout=120) as r, open(tmp, "wb") as f: + length = r.headers.get("Content-Length") + total = int(length) if length is not None else 0 + done = 0 + while True: + chunk = r.read(1 << 20) + if not chunk: + break + f.write(chunk) + done += len(chunk) + if progress is not None: + progress(done, total) + # Chunked reads can return EOF without raising IncompleteRead. + if length is not None and done != total: + raise BinaryResolutionError( + f"incomplete download for {dest.name}: expected {total} bytes, got {done}") + tmp.replace(dest) + finally: + tmp.unlink(missing_ok=True) def _extract(archive: Path, dest: Path, @@ -207,7 +218,6 @@ def _extract(archive: Path, dest: Path, opener, list_members, size = zipfile.ZipFile, "infolist", "file_size" kwargs = {} else: - import tarfile opener, list_members, size = tarfile.open, "getmembers", "size" kwargs = {"filter": "data"} with opener(archive) as ar: @@ -289,19 +299,31 @@ def ensure_runtime_installed(tag: str, backend: str, for i, asset in enumerate(plan.assets, 1): label = f"{i}/{n_assets}" if n_assets > 1 else "" archive = downloads / asset - if not archive.exists(): - _download(RELEASE_URL.format(tag=tag, asset=asset), archive, - progress=stage_progress("download", label)) - if progress is not None: - progress("verify", 0, 0, label) - digest = _sha256(archive) - expected = (expected_sha256 or {}).get(asset) - if expected and digest != expected: - archive.unlink(missing_ok=True) - raise BinaryResolutionError( - f"sha256 mismatch for {asset}: expected {expected}, got {digest}") + # Older versions could cache truncated responses. Replace a corrupt + # cache entry once; a corrupt fresh download must fail and be evicted. + for attempt in range(2): + cached = archive.exists() + if not cached: + _download(RELEASE_URL.format(tag=tag, asset=asset), archive, + progress=stage_progress("download", label)) + if progress is not None: + progress("verify", 0, 0, label) + digest = _sha256(archive) + expected = (expected_sha256 or {}).get(asset) + if expected and digest != expected: + archive.unlink(missing_ok=True) + raise BinaryResolutionError( + f"sha256 mismatch for {asset}: expected {expected}, got {digest}") + try: + _extract(archive, install_dir, progress=stage_progress("extract", label)) + except (zipfile.BadZipFile, tarfile.ReadError, gzip.BadGzipFile, EOFError, zlib.error) as exc: + archive.unlink(missing_ok=True) + if not cached or attempt == 1: + raise BinaryResolutionError(f"invalid runtime archive {asset}: {exc}") from exc + logger.warning("discarding corrupt cached runtime archive %s: %s", asset, exc) + else: + break recorded[asset] = digest - _extract(archive, install_dir, progress=stage_progress("extract", label)) if progress is not None: progress("verify", 0, 0, "") diff --git a/tests/hermes_cli/test_local_runtime_downloads.py b/tests/hermes_cli/test_local_runtime_downloads.py new file mode 100644 index 0000000000..55233083a5 --- /dev/null +++ b/tests/hermes_cli/test_local_runtime_downloads.py @@ -0,0 +1,135 @@ +"""Runtime downloads must not turn a transient transfer failure into a poisoned cache.""" + +import hashlib +import io +import json +import sys +import tarfile +import threading +import zipfile +from http.server import BaseHTTPRequestHandler, HTTPServer +from pathlib import Path + +import pytest + +from hermes_cli.local_runtime import binaries + + +@pytest.fixture +def asset_server(): + responses = [] + requests = [] + + class Handler(BaseHTTPRequestHandler): + def do_GET(self): + requests.append(self.path) + body, length = responses.pop(0) + self.send_response(200) + if length is not None: + self.send_header("Content-Length", str(length)) + self.end_headers() + self.wfile.write(body) + self.close_connection = True + + def log_message(self, *args): + pass + + with HTTPServer(("127.0.0.1", 0), Handler) as server: + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + yield f"http://127.0.0.1:{server.server_port}/{{asset}}", responses, requests + finally: + server.shutdown() + thread.join(timeout=5) + + +@pytest.mark.parametrize("failure", ["short", "callback"]) +def test_download_only_publishes_complete_transfers(tmp_path, asset_server, failure): + url, responses, requests = asset_server + dest = tmp_path / "runtime.zip" + payload = b"runtime archive bytes" + responses.append((payload[:-1], len(payload))) + + def progress(done, total): + if failure == "callback": + raise RuntimeError("cancelled") + + error = RuntimeError if failure == "callback" else binaries.BinaryResolutionError + with pytest.raises(error): + binaries._download(url.format(asset=dest.name), dest, progress=progress) + assert not dest.exists() + assert not dest.with_suffix(".zip.part").exists() + + # A retry works, including servers that omit Content-Length. + responses.append((payload, None)) + ticks = [] + binaries._download(url.format(asset=dest.name), dest, + progress=lambda done, total: ticks.append((done, total))) + assert dest.read_bytes() == payload + assert ticks[-1] == (len(payload), 0) + assert len(requests) == 2 + + +@pytest.mark.parametrize("archive_format,corruption", [ + ("zip", "truncated"), ("zip", "crc"), ("tar.gz", "truncated"), +]) +@pytest.mark.parametrize("replacement_kind", ["valid", "invalid", "permission"]) +def test_install_recovers_corrupt_cache_once(tmp_path, monkeypatch, asset_server, + archive_format, corruption, replacement_kind): + url, responses, requests = asset_server + # Use the real cache/root resolution, HTTP transfer, digest and extraction. + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes")) + tag = f"b{sys.version_info.major}" + asset = f"runtime.{archive_format}" + plan = binaries.AssetPlan(tag, "cpu", [asset]) + monkeypatch.setattr(binaries, "resolve_assets", lambda *args: plan) + monkeypatch.setattr(binaries, "RELEASE_URL", url) + # Exercise the real --version subprocess without downloading a native runtime. + monkeypatch.setattr(binaries, "server_binary", lambda _: Path(sys.executable)) + payload = b"complete runtime payload" + stream = io.BytesIO() + if archive_format == "zip": + with zipfile.ZipFile(stream, "w") as archive: + archive.writestr("bin/runtime.dat", payload) + else: + with tarfile.open(fileobj=stream, mode="w:gz") as archive: + member = tarfile.TarInfo("bin/runtime.dat") + member.size = len(payload) + archive.addfile(member, io.BytesIO(payload)) + valid = stream.getvalue() + corrupt = (valid.replace(payload, b"!" * len(payload)) + if corruption == "crc" else valid[:10]) + cached = binaries.runtimes_root() / "downloads" / asset + cached.parent.mkdir(parents=True) + cached.write_bytes(corrupt) + replacement = valid if replacement_kind == "valid" else corrupt + responses.append((replacement, len(replacement))) + ticks = [] + + if replacement_kind == "permission": + def denied(*args, **kwargs): + raise PermissionError("destination is not writable") + + monkeypatch.setattr(binaries, "_extract", denied) + with pytest.raises(PermissionError): + binaries.ensure_runtime_installed(tag, "cpu") + assert cached.read_bytes() == corrupt + assert not requests + return + + if replacement_kind == "valid": + installed = binaries.ensure_runtime_installed(tag, "cpu", progress=lambda *p: ticks.append(p)) + assert (installed / "bin/runtime.dat").read_bytes() == payload + manifest = json.loads((installed / "manifest.json").read_text(encoding="utf-8")) + assert manifest["assets"][asset] == hashlib.sha256(valid).hexdigest() + assert manifest["verified_version"] + assert cached.read_bytes() == valid + assert {p[0] for p in ticks} == {"download", "verify", "extract"} + assert binaries.ensure_runtime_installed(tag, "cpu") == installed + else: + with pytest.raises(binaries.BinaryResolutionError, match="archive"): + binaries.ensure_runtime_installed(tag, "cpu") + assert not cached.exists() + assert not (plan.install_dir / "manifest.json").exists() + assert len(requests) == 1 From ea09e2e401572c8a058a49a6d58ce3e05b299c0b Mon Sep 17 00:00:00 2001 From: Ryan Hill <38674843+RyanUnderhill@users.noreply.github.com> Date: Wed, 16 Sep 2026 09:22:32 -0700 Subject: [PATCH 2/3] fix(local-runtime): limit archive fix to incomplete downloads --- hermes_cli/local_runtime/binaries.py | 40 ++++------ .../test_local_runtime_downloads.py | 78 +------------------ 2 files changed, 17 insertions(+), 101 deletions(-) diff --git a/hermes_cli/local_runtime/binaries.py b/hermes_cli/local_runtime/binaries.py index 9b62264643..6cc7a91929 100644 --- a/hermes_cli/local_runtime/binaries.py +++ b/hermes_cli/local_runtime/binaries.py @@ -2,7 +2,6 @@ from __future__ import annotations -import gzip import hashlib import json import logging @@ -10,10 +9,8 @@ import os import platform import shutil import subprocess -import tarfile import urllib.request import zipfile -import zlib from dataclasses import dataclass, field from pathlib import Path from typing import Callable @@ -218,6 +215,7 @@ def _extract(archive: Path, dest: Path, opener, list_members, size = zipfile.ZipFile, "infolist", "file_size" kwargs = {} else: + import tarfile opener, list_members, size = tarfile.open, "getmembers", "size" kwargs = {"filter": "data"} with opener(archive) as ar: @@ -299,31 +297,19 @@ def ensure_runtime_installed(tag: str, backend: str, for i, asset in enumerate(plan.assets, 1): label = f"{i}/{n_assets}" if n_assets > 1 else "" archive = downloads / asset - # Older versions could cache truncated responses. Replace a corrupt - # cache entry once; a corrupt fresh download must fail and be evicted. - for attempt in range(2): - cached = archive.exists() - if not cached: - _download(RELEASE_URL.format(tag=tag, asset=asset), archive, - progress=stage_progress("download", label)) - if progress is not None: - progress("verify", 0, 0, label) - digest = _sha256(archive) - expected = (expected_sha256 or {}).get(asset) - if expected and digest != expected: - archive.unlink(missing_ok=True) - raise BinaryResolutionError( - f"sha256 mismatch for {asset}: expected {expected}, got {digest}") - try: - _extract(archive, install_dir, progress=stage_progress("extract", label)) - except (zipfile.BadZipFile, tarfile.ReadError, gzip.BadGzipFile, EOFError, zlib.error) as exc: - archive.unlink(missing_ok=True) - if not cached or attempt == 1: - raise BinaryResolutionError(f"invalid runtime archive {asset}: {exc}") from exc - logger.warning("discarding corrupt cached runtime archive %s: %s", asset, exc) - else: - break + if not archive.exists(): + _download(RELEASE_URL.format(tag=tag, asset=asset), archive, + progress=stage_progress("download", label)) + if progress is not None: + progress("verify", 0, 0, label) + digest = _sha256(archive) + expected = (expected_sha256 or {}).get(asset) + if expected and digest != expected: + archive.unlink(missing_ok=True) + raise BinaryResolutionError( + f"sha256 mismatch for {asset}: expected {expected}, got {digest}") recorded[asset] = digest + _extract(archive, install_dir, progress=stage_progress("extract", label)) if progress is not None: progress("verify", 0, 0, "") diff --git a/tests/hermes_cli/test_local_runtime_downloads.py b/tests/hermes_cli/test_local_runtime_downloads.py index 55233083a5..e79d5dca06 100644 --- a/tests/hermes_cli/test_local_runtime_downloads.py +++ b/tests/hermes_cli/test_local_runtime_downloads.py @@ -1,14 +1,7 @@ """Runtime downloads must not turn a transient transfer failure into a poisoned cache.""" -import hashlib -import io -import json -import sys -import tarfile import threading -import zipfile from http.server import BaseHTTPRequestHandler, HTTPServer -from pathlib import Path import pytest @@ -45,7 +38,8 @@ def asset_server(): @pytest.mark.parametrize("failure", ["short", "callback"]) -def test_download_only_publishes_complete_transfers(tmp_path, asset_server, failure): +@pytest.mark.parametrize("known_length", [True, False]) +def test_download_only_publishes_complete_transfers(tmp_path, asset_server, failure, known_length): url, responses, requests = asset_server dest = tmp_path / "runtime.zip" payload = b"runtime archive bytes" @@ -62,74 +56,10 @@ def test_download_only_publishes_complete_transfers(tmp_path, asset_server, fail assert not dest.with_suffix(".zip.part").exists() # A retry works, including servers that omit Content-Length. - responses.append((payload, None)) + responses.append((payload, len(payload) if known_length else None)) ticks = [] binaries._download(url.format(asset=dest.name), dest, progress=lambda done, total: ticks.append((done, total))) assert dest.read_bytes() == payload - assert ticks[-1] == (len(payload), 0) + assert ticks[-1] == (len(payload), len(payload) if known_length else 0) assert len(requests) == 2 - - -@pytest.mark.parametrize("archive_format,corruption", [ - ("zip", "truncated"), ("zip", "crc"), ("tar.gz", "truncated"), -]) -@pytest.mark.parametrize("replacement_kind", ["valid", "invalid", "permission"]) -def test_install_recovers_corrupt_cache_once(tmp_path, monkeypatch, asset_server, - archive_format, corruption, replacement_kind): - url, responses, requests = asset_server - # Use the real cache/root resolution, HTTP transfer, digest and extraction. - monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes")) - tag = f"b{sys.version_info.major}" - asset = f"runtime.{archive_format}" - plan = binaries.AssetPlan(tag, "cpu", [asset]) - monkeypatch.setattr(binaries, "resolve_assets", lambda *args: plan) - monkeypatch.setattr(binaries, "RELEASE_URL", url) - # Exercise the real --version subprocess without downloading a native runtime. - monkeypatch.setattr(binaries, "server_binary", lambda _: Path(sys.executable)) - payload = b"complete runtime payload" - stream = io.BytesIO() - if archive_format == "zip": - with zipfile.ZipFile(stream, "w") as archive: - archive.writestr("bin/runtime.dat", payload) - else: - with tarfile.open(fileobj=stream, mode="w:gz") as archive: - member = tarfile.TarInfo("bin/runtime.dat") - member.size = len(payload) - archive.addfile(member, io.BytesIO(payload)) - valid = stream.getvalue() - corrupt = (valid.replace(payload, b"!" * len(payload)) - if corruption == "crc" else valid[:10]) - cached = binaries.runtimes_root() / "downloads" / asset - cached.parent.mkdir(parents=True) - cached.write_bytes(corrupt) - replacement = valid if replacement_kind == "valid" else corrupt - responses.append((replacement, len(replacement))) - ticks = [] - - if replacement_kind == "permission": - def denied(*args, **kwargs): - raise PermissionError("destination is not writable") - - monkeypatch.setattr(binaries, "_extract", denied) - with pytest.raises(PermissionError): - binaries.ensure_runtime_installed(tag, "cpu") - assert cached.read_bytes() == corrupt - assert not requests - return - - if replacement_kind == "valid": - installed = binaries.ensure_runtime_installed(tag, "cpu", progress=lambda *p: ticks.append(p)) - assert (installed / "bin/runtime.dat").read_bytes() == payload - manifest = json.loads((installed / "manifest.json").read_text(encoding="utf-8")) - assert manifest["assets"][asset] == hashlib.sha256(valid).hexdigest() - assert manifest["verified_version"] - assert cached.read_bytes() == valid - assert {p[0] for p in ticks} == {"download", "verify", "extract"} - assert binaries.ensure_runtime_installed(tag, "cpu") == installed - else: - with pytest.raises(binaries.BinaryResolutionError, match="archive"): - binaries.ensure_runtime_installed(tag, "cpu") - assert not cached.exists() - assert not (plan.install_dir / "manifest.json").exists() - assert len(requests) == 1 From 3c5659178874eddde06fcb05b89ec3a83b0563b6 Mon Sep 17 00:00:00 2001 From: emozilla Date: Wed, 16 Sep 2026 14:32:54 -0400 Subject: [PATCH 3/3] fix(local-runtime): isolate download staging files --- hermes_cli/local_runtime/binaries.py | 7 ++- .../test_local_runtime_downloads.py | 45 ++++++++++++++++--- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/hermes_cli/local_runtime/binaries.py b/hermes_cli/local_runtime/binaries.py index 6cc7a91929..c304f06ffa 100644 --- a/hermes_cli/local_runtime/binaries.py +++ b/hermes_cli/local_runtime/binaries.py @@ -9,6 +9,7 @@ import os import platform import shutil import subprocess +import tempfile import urllib.request import zipfile from dataclasses import dataclass, field @@ -185,9 +186,11 @@ def _download(url: str, dest: Path, """Stream url -> dest. ``progress(done_bytes, total_bytes)`` ticks per chunk (total 0 when the server sends no Content-Length) — a several-hundred-MB archive must never look hung.""" logger.info("downloading %s", url) - tmp = dest.with_suffix(dest.suffix + ".part") + staging = tempfile.NamedTemporaryFile( + mode="wb", dir=dest.parent, prefix=f"{dest.name}.", suffix=".part", delete=False) + tmp = Path(staging.name) try: - with urllib.request.urlopen(url, timeout=120) as r, open(tmp, "wb") as f: + with staging as f, urllib.request.urlopen(url, timeout=120) as r: length = r.headers.get("Content-Length") total = int(length) if length is not None else 0 done = 0 diff --git a/tests/hermes_cli/test_local_runtime_downloads.py b/tests/hermes_cli/test_local_runtime_downloads.py index e79d5dca06..c0ac8558e4 100644 --- a/tests/hermes_cli/test_local_runtime_downloads.py +++ b/tests/hermes_cli/test_local_runtime_downloads.py @@ -1,7 +1,9 @@ """Runtime downloads must not turn a transient transfer failure into a poisoned cache.""" import threading -from http.server import BaseHTTPRequestHandler, HTTPServer +from concurrent.futures import ThreadPoolExecutor +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from urllib.error import HTTPError import pytest @@ -16,8 +18,8 @@ def asset_server(): class Handler(BaseHTTPRequestHandler): def do_GET(self): requests.append(self.path) - body, length = responses.pop(0) - self.send_response(200) + body, length, status = responses.pop(0) + self.send_response(status) if length is not None: self.send_header("Content-Length", str(length)) self.end_headers() @@ -27,7 +29,7 @@ def asset_server(): def log_message(self, *args): pass - with HTTPServer(("127.0.0.1", 0), Handler) as server: + with ThreadingHTTPServer(("127.0.0.1", 0), Handler) as server: thread = threading.Thread(target=server.serve_forever, daemon=True) thread.start() try: @@ -43,7 +45,7 @@ def test_download_only_publishes_complete_transfers(tmp_path, asset_server, fail url, responses, requests = asset_server dest = tmp_path / "runtime.zip" payload = b"runtime archive bytes" - responses.append((payload[:-1], len(payload))) + responses.append((payload[:-1], len(payload), 200)) def progress(done, total): if failure == "callback": @@ -53,13 +55,42 @@ def test_download_only_publishes_complete_transfers(tmp_path, asset_server, fail with pytest.raises(error): binaries._download(url.format(asset=dest.name), dest, progress=progress) assert not dest.exists() - assert not dest.with_suffix(".zip.part").exists() + assert not list(tmp_path.glob("*.part")) # A retry works, including servers that omit Content-Length. - responses.append((payload, len(payload) if known_length else None)) + responses.append((payload, len(payload) if known_length else None, 200)) ticks = [] binaries._download(url.format(asset=dest.name), dest, progress=lambda done, total: ticks.append((done, total))) assert dest.read_bytes() == payload assert ticks[-1] == (len(payload), len(payload) if known_length else 0) assert len(requests) == 2 + + +def test_failed_request_preserves_another_active_download(tmp_path, asset_server): + url, responses, requests = asset_server + dest = tmp_path / "runtime.zip" + payload = b"x" * (2 << 20) + responses.extend([(payload, len(payload), 200), (b"", 0, 503)]) + started = threading.Event() + resume = threading.Event() + + def pause_download(done, total): + started.set() + assert resume.wait(10), "active download was not resumed" + + with ThreadPoolExecutor(max_workers=1) as pool: + active = pool.submit(binaries._download, url.format(asset=dest.name), dest, + progress=pause_download) + try: + assert started.wait(10), "active download did not write its first chunk" + with pytest.raises(HTTPError) as failed: + binaries._download(url.format(asset=dest.name), dest) + assert failed.value.code == 503 + finally: + resume.set() + active.result(timeout=10) + + assert dest.read_bytes() == payload + assert not list(tmp_path.glob("*.part")) + assert len(requests) == 2