fix(packages): bound archive downloads and gate them in offline mode

CHANGELOG.md

@@ -22,6 +22,23 @@
   `cjson` (v1.7.18), `nanopb` (v0.4.9.1), `lvgl` (v9.2.2), `tinyusb` (v0.18.0), and `unity` (v2.6.1).
 
 ### Fixed
+- **Package archive downloads can no longer hang or poison the cache.**
+  `PackageFetcher` downloaded with `urlretrieve()`, which has no timeout: one
+  unresponsive mirror left `ebuild build` hanging until the user killed it.
+  Downloads are now streamed through `urllib.request.urlopen(..., timeout=...)`
+  (30 s default, configurable via `PackageFetcher(..., timeout=...)`), refuse
+  anything larger than 512 MB while streaming (so a server that lies about or
+  omits `Content-Length` still cannot fill the disk), and are written to a
+  `.part` file renamed into place only when complete — a connection that dies
+  mid-body no longer leaves a truncated archive in the download cache to
+  satisfy every later fetch (`ebuild/packages/fetcher.py`).
+- **Archive fetching is now gated in offline mode.** `EBUILD_OFFLINE=1` and
+  `ebuild update-index --offline` already governed index synchronization;
+  package archive fetching was exempt and hit the network anyway. A fetch
+  whose archive is not already in the download cache now fails with a message
+  naming the missing archive; a cached archive still extracts offline, so
+  air-gapped rebuilds work from a warmed cache
+  (`ebuild/packages/fetcher.py`).
 - **`ebuild test` now finds Windows test binaries.** The Ninja edge for a
   native `type: test` target already carried the platform suffix
   (`_exe_suffix()` names it `<name>.exe` on Windows), but `ebuild test`

TASKS.md

@@ -15,6 +15,7 @@ Status is one of: `todo`, `in-progress`, `blocked`, `review`, `done`.
 | T-003 | `ebuild package` looks for the unsuffixed binary on Windows (`_build/app` rather than `_build/app.exe`) | backend | Maintenance | review | none |
 | T-004 | `_report_footprint` (the flash/RAM report `ebuild build` prints) looks for the unsuffixed binary on Windows, and fails silently rather than logging why | backend | Maintenance | review | none |
 | T-005 | Move `executable_output_path()` out of the Ninja-specific backend into a backend-neutral module (`ebuild/build/layout.py`), re-exported from `ninja_backend` for compatibility | backend | Maintenance | todo | none |
+| T-006 | `PackageRecipe.to_dict()` was deleted in #112; `ebuild update-index` fails with `AttributeError` on every recipe it caches (`index_sync.py:354` calls it); 9 tests in `tests/unit/test_index_sync.py` fail on master, and mypy reports the call site | backend | Maintenance | todo | none |
 
 ### Evidence (self-reported by implementer; pending independent review per `.ai/reviewer.md` — "if you implemented it, you do not approve it")
 

ebuild/packages/fetcher.py

@@ -5,19 +5,41 @@
 
 Handles HTTP/HTTPS downloads, SHA-256 checksum verification, and
 tar/zip extraction into the package cache directory.
+
+Downloads are bounded and atomic: a socket timeout keeps a stalled mirror
+from hanging the build forever, an overall size cap keeps a runaway
+response from filling the disk, and the archive is streamed to a ``.part``
+file that is renamed into place only once complete, so a crashed download
+never leaves a truncated archive behind to satisfy the cache.
 """
 
 from __future__ import annotations
 
 import hashlib
+import os
 import tarfile
+import urllib.error
+import urllib.request
 import zipfile
 from pathlib import Path
 from typing import Optional
-from urllib.request import urlretrieve
 
+from ebuild.packages.index_sync import is_offline
 from ebuild.packages.recipe import PackageRecipe
 
+# Seconds a download may stall before it is abandoned. urlretrieve() carried
+# no timeout at all, so one unresponsive mirror hung `ebuild build` until the
+# user killed it.
+DEFAULT_DOWNLOAD_TIMEOUT_SECONDS = 30
+
+# Upper bound on a single downloaded archive (512 MB). Enforced while
+# streaming, so a server that lies about (or omits) Content-Length still
+# cannot fill the disk.
+MAX_ARCHIVE_SIZE_BYTES = 512 * 1024 * 1024
+
+# Chunk size for streaming a download to disk.
+DOWNLOAD_CHUNK_SIZE_BYTES = 256 * 1024
+
 
 class FetchError(Exception):
     """Raised when a package cannot be fetched or verified."""
@@ -30,8 +52,13 @@ class PackageFetcher:
     Checksums are verified to ensure integrity.
     """
 
-    def __init__(self, download_dir: str | Path) -> None:
+    def __init__(
+        self,
+        download_dir: str | Path,
+        timeout: int = DEFAULT_DOWNLOAD_TIMEOUT_SECONDS,
+    ) -> None:
         self.download_dir = Path(download_dir)
+        self.timeout = timeout
         self.download_dir.mkdir(parents=True, exist_ok=True)
 
     def fetch(self, recipe: PackageRecipe, extract_to: str | Path) -> Path:
@@ -45,8 +72,21 @@ def fetch(self, recipe: PackageRecipe, extract_to: str | Path) -> Path:
             Path to the extracted source directory.
 
         Raises:
-            FetchError: If download or verification fails.
+            FetchError: If download or verification fails, or if offline
+                mode is active and the archive is not already cached.
         """
+        # Offline mode must gate archive fetching the same way it gates index
+        # synchronization: a fetch that is not already in the download cache
+        # requires the network, and in offline mode there is no network.
+        if is_offline() and not self.is_downloaded(recipe):
+            archive_path = self._archive_path(recipe)
+            where = f" ({archive_path})" if archive_path else ""
+            raise FetchError(
+                f"Offline mode (EBUILD_OFFLINE=1): package {recipe.name} "
+                f"v{recipe.version} is not in the download cache{where}. "
+                f"Re-run without offline mode to download it."
+            )
+
         # PackageRecipe.validate() rejects a recipe without a checksum, but
         # fetch() is reachable with a hand-built recipe too, so refuse here as
         # well rather than falling through to an unverified extract.
@@ -90,14 +130,42 @@ def _download(self, recipe: PackageRecipe) -> Path:
             return archive_path
 
         archive_path.parent.mkdir(parents=True, exist_ok=True)
+
+        # Stream to a .part sibling and rename only once complete. A download
+        # that dies halfway must not leave a truncated file at archive_path:
+        # _download() short-circuits on existence, so a partial archive would
+        # be served by every later fetch as if it were the real thing.
+        partial_path = archive_path.with_name(archive_path.name + ".part")
         try:
-            urlretrieve(recipe.url, str(archive_path))
-        except Exception as e:
-            archive_path.unlink(missing_ok=True)
-            raise FetchError(
-                f"Failed to download {recipe.name} v{recipe.version} "
-                f"from {recipe.url}: {e}"
-            )
+            try:
+                request = urllib.request.Request(
+                    recipe.url,
+                    headers={"User-Agent": "ebuild-package-manager/3.0"},
+                )
+                with urllib.request.urlopen(
+                    request, timeout=self.timeout
+                ) as response:
+                    received = 0
+                    with open(partial_path, "wb") as f:
+                        while True:
+                            chunk = response.read(DOWNLOAD_CHUNK_SIZE_BYTES)
+                            if not chunk:
+                                break
+                            received += len(chunk)
+                            if received > MAX_ARCHIVE_SIZE_BYTES:
+                                raise FetchError(
+                                    f"Archive from {recipe.url} exceeds the "
+                                    f"maximum size of {MAX_ARCHIVE_SIZE_BYTES} bytes"
+                                )
+                            f.write(chunk)
+                os.replace(partial_path, archive_path)
+            except (urllib.error.URLError, OSError) as e:
+                raise FetchError(
+                    f"Failed to download {recipe.name} v{recipe.version} "
+                    f"from {recipe.url}: {e}"
+                ) from e
+        finally:
+            partial_path.unlink(missing_ok=True)
 
         return archive_path
 

tests/ebuild/test_package_fetcher.py

@@ -3,7 +3,7 @@
 
 """Tests for ebuild.packages.fetcher.PackageFetcher.
 
-No test here touches the network: ``urlretrieve`` is replaced with a stub
+No test here touches the network: ``urlopen`` is replaced with a stub
 that synthesises a small tarball whose contents identify the URL it was
 asked for. That makes cross-package contamination observable — a package
 extracted from the wrong archive carries the wrong marker.
@@ -99,12 +99,21 @@ def fake_download(monkeypatch):
     """
     calls = []
 
-    def _urlretrieve(url, filename):
+    class _FakeResponse(io.BytesIO):
+        """Duck-typed urlopen() response: BytesIO plus a context manager."""
+
+        def __enter__(self):
+            return self
+
+        def __exit__(self, *exc_info):
+            return False
+
+    def _urlopen(request, timeout=None):
+        url = request.full_url if hasattr(request, "full_url") else request
         calls.append(url)
-        with open(filename, "wb") as f:
-            f.write(targz_bytes(url))
+        return _FakeResponse(targz_bytes(url))
 
-    monkeypatch.setattr("ebuild.packages.fetcher.urlretrieve", _urlretrieve)
+    monkeypatch.setattr("ebuild.packages.fetcher.urllib.request.urlopen", _urlopen)
     return calls
 
 
@@ -252,12 +261,19 @@ def test_fetch_extracts_into_the_requested_directory(tmp_path, fake_download):
 
 
 def test_unsupported_archive_format_is_rejected(tmp_path, monkeypatch):
+    class _Response(io.BytesIO):
+        def __enter__(self):
+            return self
+
+        def __exit__(self, *exc_info):
+            return False
+
     monkeypatch.setattr(
-        "ebuild.packages.fetcher.urlretrieve",
-        lambda url, filename: open(filename, "wb").write(b"not an archive"),
+        "ebuild.packages.fetcher.urllib.request.urlopen",
+        lambda request, timeout=None: _Response(b"not an archive"),
     )
     fetcher = PackageFetcher(tmp_path / "dl")
-    # Checksum of the bytes the patched urlretrieve writes, so the fetch gets
+    # Checksum of the bytes the patched urlopen serves, so the fetch gets
     # past verification and reaches the format check this test is about.
     recipe = make_recipe(
         "littlefs",
@@ -287,16 +303,155 @@ def test_non_http_url_schemes_are_rejected(tmp_path, url):
 
 
 def test_failed_download_leaves_no_partial_archive(tmp_path, monkeypatch):
-    def _boom(url, filename):
-        with open(filename, "wb") as f:
-            f.write(b"partial")
-        raise OSError("connection reset")
+    class _Response(io.BytesIO):
+        def __enter__(self):
+            return self
+
+        def __exit__(self, *exc_info):
+            return False
+
+    def _boom(request, timeout=None):
+        return _Response(b"partial")
+
+    monkeypatch.setattr(
+        "ebuild.packages.fetcher.urllib.request.urlopen", _boom
+    )
+    monkeypatch.setattr(
+        "ebuild.packages.fetcher.MAX_ARCHIVE_SIZE_BYTES", 4
+    )
+    fetcher = PackageFetcher(tmp_path / "dl")
+    recipe = make_recipe("littlefs")
+
+    with pytest.raises(FetchError, match="maximum size"):
+        fetcher.fetch(recipe, tmp_path / "src")
+
+    assert fetcher.is_downloaded(recipe) is False
+
+
+# ── Download hardening (timeout, size cap, atomicity) ────────
+
+
+def test_download_passes_the_configured_timeout(tmp_path, monkeypatch):
+    """urlretrieve() carried no timeout, so a stalled mirror hung the build.
+
+    The timeout must actually reach urlopen — it is the whole point.
+    """
+    seen = {}
+
+    def _urlopen(request, timeout=None):
+        seen["timeout"] = timeout
+        return io.BytesIO(b"")
+
+    monkeypatch.setattr(
+        "ebuild.packages.fetcher.urllib.request.urlopen", _urlopen
+    )
+    fetcher = PackageFetcher(tmp_path / "dl", timeout=7)
+    # A mismatching checksum lets the download run to completion before the
+    # verification failure ends the fetch.
+    recipe = make_recipe("littlefs", checksum="sha256:" + "0" * 64)
+    with pytest.raises(FetchError):
+        fetcher.fetch(recipe, tmp_path / "src")
+
+    assert seen["timeout"] == 7
+
+
+def test_default_fetcher_gets_the_default_timeout(tmp_path, fake_download):
+    """A fetcher constructed with no explicit timeout still times out."""
+    from ebuild.packages.fetcher import DEFAULT_DOWNLOAD_TIMEOUT_SECONDS
+
+    fetcher = PackageFetcher(tmp_path / "dl")
+    assert fetcher.timeout == DEFAULT_DOWNLOAD_TIMEOUT_SECONDS
+
+
+def test_oversized_download_is_rejected(tmp_path, monkeypatch):
+    """A response larger than the cap must be refused, not written to disk."""
+    class _Response(io.BytesIO):
+        def __enter__(self):
+            return self
+
+        def __exit__(self, *exc_info):
+            return False
+
+    monkeypatch.setattr(
+        "ebuild.packages.fetcher.urllib.request.urlopen",
+        lambda request, timeout=None: _Response(b"x" * 16),
+    )
+    monkeypatch.setattr(
+        "ebuild.packages.fetcher.MAX_ARCHIVE_SIZE_BYTES", 8
+    )
+    fetcher = PackageFetcher(tmp_path / "dl")
+    recipe = make_recipe("littlefs", checksum="sha256:" + "0" * 64)
+
+    with pytest.raises(FetchError, match="maximum size"):
+        fetcher.fetch(recipe, tmp_path / "src")
+
+    assert fetcher.is_downloaded(recipe) is False
+    # Only the (empty) package directory may remain — no archive, no .part.
+    assert all(p.is_dir() for p in (tmp_path / "dl").rglob("*"))
+
 
-    monkeypatch.setattr("ebuild.packages.fetcher.urlretrieve", _boom)
+def test_truncated_download_leaves_no_cache_entry(tmp_path, monkeypatch):
+    """A connection that dies mid-body must not satisfy the cache.
+
+    _download() short-circuits on existence, so a truncated archive left at
+    the cache path would be extracted (or checksum-failed) forever after.
+    The streamed .part file must never be renamed into place on failure.
+    """
+    class _TruncatedResponse(io.BytesIO):
+        def read(self, size=-1):
+            raise OSError("connection reset by peer")
+
+        def __enter__(self):
+            return self
+
+        def __exit__(self, *exc_info):
+            return False
+
+    monkeypatch.setattr(
+        "ebuild.packages.fetcher.urllib.request.urlopen",
+        lambda request, timeout=None: _TruncatedResponse(b""),
+    )
     fetcher = PackageFetcher(tmp_path / "dl")
     recipe = make_recipe("littlefs")
 
     with pytest.raises(FetchError, match="Failed to download"):
         fetcher.fetch(recipe, tmp_path / "src")
 
     assert fetcher.is_downloaded(recipe) is False
+    # No .part litter either.
+    assert not list((tmp_path / "dl").rglob("*.part"))
+
+
+# ── Offline mode ─────────────────────────────────────────────
+
+
+def test_offline_mode_refuses_an_uncached_package(tmp_path, monkeypatch):
+    """Offline mode must gate archive fetching like it gates index sync."""
+    monkeypatch.setattr(
+        "ebuild.packages.fetcher.is_offline", lambda offline_flag=False: True
+    )
+    fetcher = PackageFetcher(tmp_path / "dl")
+    recipe = make_recipe("littlefs")
+
+    with pytest.raises(FetchError, match="Offline mode"):
+        fetcher.fetch(recipe, tmp_path / "src")
+
+    # Nothing was downloaded or extracted.
+    assert fetcher.is_downloaded(recipe) is False
+    assert not (tmp_path / "src").exists()
+
+
+def test_offline_mode_uses_the_cached_archive(tmp_path, monkeypatch, fake_download):
+    """A package already in the download cache stays buildable offline."""
+    fetcher = PackageFetcher(tmp_path / "dl")
+    recipe = make_recipe("littlefs")
+    fetcher.fetch(recipe, tmp_path / "src-warm")
+    assert fake_download == [LITTLEFS_URL]
+
+    monkeypatch.setattr(
+        "ebuild.packages.fetcher.is_offline", lambda offline_flag=False: True
+    )
+    fetcher.fetch(recipe, tmp_path / "src-offline")
+
+    assert fake_download == [LITTLEFS_URL]
+    assert marker_in(tmp_path / "src-offline") == LITTLEFS_URL