fix: keep large-file revisions and clean failed fetches

scripts/largefiles.py

@@ -6,7 +6,9 @@ against the hash the caller declares."""
 from __future__ import annotations
 
 import contextlib
+import http.client
 import os
+import sys
 import re
 import tempfile
 import urllib.error
@@ -98,6 +100,13 @@ def asset_candidates(name: str, registered: Iterable[str]) -> list[str]:
     matches = [
         names[p] for p in sorted(names) if os.path.basename(p) == name
     ]
+    # A pinned revision is stored as .variants/<name>.<md5 prefix>: ROCKNIX
+    # pins PS3UPDAT.PUP to the variant, and asking for the bare name never
+    # reached its asset.
+    matches += [
+        names[p] for p in sorted(names)
+        if "/.variants/" in p and os.path.basename(p).startswith(name + ".")
+    ]
     return matches or [name]
 
 
@@ -150,21 +159,15 @@ def _fetch_asset(
             hashes = None
         if hashes is None:
             pass
-        elif expected_sha1 or expected_md5:
+        elif not (expected_sha1 or expected_md5) or _matches(
-            if expected_sha1 and hashes["sha1"].lower() != expected_sha1.lower():
+            hashes, expected_sha1, expected_md5
-                _drop(cached)
+        ):
-            elif expected_md5:
-                md5_list = [
-                    m.strip().lower() for m in expected_md5.split(",") if m.strip()
-                ]
-                if hashes["md5"].lower() not in md5_list:
-                    _drop(cached)
-                else:
-                    return cached
-            else:
-                return cached
-        else:
             return cached
+        else:
+            # A verified copy of this asset that answers another hash: the
+            # caller wants a different revision under the same name. Keeping
+            # it is what lets the next caller for the primary find it.
+            return None
 
     if offline:
         return None
@@ -182,42 +185,46 @@ def _fetch_asset(
     if " " in name:
         candidates.append(name.replace(" ", "."))
 
-    downloaded = False
+    try:
-    for candidate in candidates:
+        downloaded = False
-        encoded_name = urllib.parse.quote(candidate)
+        for candidate in candidates:
-        url = (
+            encoded_name = urllib.parse.quote(candidate)
-            f"https://github.com/{LARGE_FILES_REPO}/releases/download/"
+            url = (
-            f"{LARGE_FILES_RELEASE}/{encoded_name}"
+                f"https://github.com/{LARGE_FILES_REPO}/releases/download/"
-        )
+                f"{LARGE_FILES_RELEASE}/{encoded_name}"
-        try:
+            )
-            req = urllib.request.Request(url, headers={"User-Agent": "retrobios/1.0"})
+            try:
-            with urllib.request.urlopen(req, timeout=300) as resp:
+                req = urllib.request.Request(url, headers={"User-Agent": "retrobios/1.0"})
-                with open(tmp_path, "wb") as f:
+                with urllib.request.urlopen(req, timeout=300) as resp:
-                    while True:
+                    with open(tmp_path, "wb") as f:
-                        chunk = resp.read(65536)
+                        while True:
-                        if not chunk:
+                            chunk = resp.read(65536)
-                            break
+                            if not chunk:
-                        f.write(chunk)
+                                break
-            downloaded = True
+                            f.write(chunk)
-            break
+                downloaded = True
-        except (urllib.error.URLError, urllib.error.HTTPError):
+                break
-            if os.path.exists(tmp_path):
+            except (urllib.error.URLError, OSError, http.client.HTTPException) as exc:
-                os.unlink(tmp_path)
+                # A stalled or cut stream is a timeout or IncompleteRead, not
-
+                # a URLError: it escaped this handler and left its scratch.
-    if not downloaded:
+                print(f"  large file {candidate}: {exc}", file=sys.stderr)
-        if os.path.exists(tmp_path):
+        if not downloaded:
-            os.unlink(tmp_path)
-        return None
-
-    if expected_sha1 or expected_md5:
-        hashes = compute_hashes(tmp_path)
-        if expected_sha1 and hashes["sha1"].lower() != expected_sha1.lower():
-            os.unlink(tmp_path)
             return None
-        if expected_md5:
+        if (expected_sha1 or expected_md5) and not _matches(
-            md5_list = [m.strip().lower() for m in expected_md5.split(",") if m.strip()]
+            compute_hashes(tmp_path), expected_sha1, expected_md5
-            if hashes["md5"].lower() not in md5_list:
+        ):
-                os.unlink(tmp_path)
+            return None
-                return None
+        os.replace(tmp_path, cached)
-    os.replace(tmp_path, cached)
+        return cached
-    return cached
+    finally:
+        _drop(tmp_path)
+
+
+def _matches(hashes: dict, expected_sha1: str, expected_md5: str) -> bool:
+    """Whether computed hashes answer the caller's declaration."""
+    if expected_sha1 and hashes["sha1"].lower() != expected_sha1.lower():
+        return False
+    if expected_md5:
+        md5_list = [m.strip().lower() for m in expected_md5.split(",") if m.strip()]
+        return hashes["md5"].lower() in md5_list
+    return True

tests/test_large_file_cache.py

@@ -435,5 +435,68 @@ class ReleaseAssetNames(unittest.TestCase):
             self.assertTrue(Path(cache, names[self.ET]).exists())
 
 
+class _StalledResponse(io.BytesIO):
+    """Serves one chunk, then the stream stalls."""
+
+    def __init__(self):
+        super().__init__(PAYLOAD_A)
+        self._served = False
+
+    def read(self, size: int = -1) -> bytes:
+        if self._served:
+            raise TimeoutError("read timed out")
+        self._served = True
+        return super().read(4096)
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, *exc):
+        return False
+
+
+class DownloadFailuresAndRevisions(unittest.TestCase):
+    def setUp(self):
+        self.dir = tempfile.mkdtemp(dir=REPO_ROOT / "tmp")
+        self._urlopen = largefiles.urllib.request.urlopen
+        self.addCleanup(setattr, largefiles.urllib.request, "urlopen", self._urlopen)
+
+    def tearDown(self):
+        import shutil
+
+        shutil.rmtree(self.dir, ignore_errors=True)
+
+    def test_a_stalled_stream_leaves_no_scratch(self):
+        largefiles.urllib.request.urlopen = lambda *a, **k: _StalledResponse()
+        result = largefiles.fetch_large_file(
+            "big.bin", self.dir, expected_md5=hashlib.md5(PAYLOAD_A).hexdigest(),
+            registered=[],
+        )
+        self.assertIsNone(result)
+        self.assertEqual(os.listdir(self.dir), [])
+
+    def test_another_revision_is_kept_not_evicted(self):
+        cached = Path(self.dir, "PS3UPDAT.PUP")
+        cached.write_bytes(PAYLOAD_A)
+        largefiles.urllib.request.urlopen = lambda *a, **k: (_ for _ in ()).throw(
+            urllib.error.URLError("offline")
+        )
+        result = largefiles.fetch_large_file(
+            "PS3UPDAT.PUP", self.dir, expected_md5=hashlib.md5(PAYLOAD_B).hexdigest(),
+            registered=[],
+        )
+        self.assertIsNone(result)
+        self.assertTrue(cached.exists())
+
+    def test_a_variant_asset_is_a_candidate(self):
+        registered = [
+            "bios/Sony/PlayStation 3/PS3UPDAT.PUP",
+            "bios/Sony/PlayStation 3/.variants/PS3UPDAT.PUP.ed8ab192",
+        ]
+        self.assertEqual(
+            largefiles.asset_candidates("PS3UPDAT.PUP", registered),
+            ["PS3UPDAT.PUP", "PS3UPDAT.PUP.ed8ab192"],
+        )
+
 if __name__ == "__main__":
     unittest.main()