fix: retry a bad data download, keep the site step

The buildbot answered once with something that was not ScummVM.zip. refresh_data_dirs gave up at once, Generate the site was skipped, and 228 site tests failed in its place. A failed or unreadable download is now tried three times, with waits of 5 and 20 s, and an empty archive still fails at once. The site step runs after a failed fetch, which stays red on its own step.

.github/workflows/validate.yml

@@ -276,8 +276,11 @@ jobs:
 
       # The pages are gitignored, and the tests that read them fail
       # in CI without it. The suite runs whatever this step returned, so a
-      # generator that stops still leaves every other test reporting.
+      # generator that stops still leaves every other test reporting. It runs
+      # after a failed data fetch too: one bad mirror stays red on its own
+      # step instead of skipping the site and failing 228 site tests with it.
       - name: Generate the site
+        if: ${{ !cancelled() }}
         run: python scripts/generate_site.py
 
       - name: Run test suite

scripts/refresh_data_dirs.py

@@ -20,6 +20,7 @@ import os
 import shutil
 import tarfile
 import tempfile
+import time
 import urllib.error
 import urllib.request
 import zipfile
@@ -40,6 +41,11 @@ VERSIONS_FILE = "data/.versions.json"
 USER_AGENT = "retrobios/1.0"
 REQUEST_TIMEOUT = 30
 DOWNLOAD_TIMEOUT = 300
+# A mirror sometimes answers with a page that is not the archive: the
+# buildbot's ScummVM.zip failed CI once as "File is not a zip file" and
+# downloaded fine minutes later. Seconds to wait before each retry.
+DOWNLOAD_RETRY_WAITS = (5, 20)
+_TRANSIENT = (urllib.error.URLError, OSError, tarfile.TarError, zipfile.BadZipFile, EOFError)
 
 
 def load_registry(registry_path: str = DEFAULT_REGISTRY) -> dict[str, dict]:
@@ -492,26 +498,29 @@ def _refresh_entry(
         else:
             remote_tag = get_remote_sha(entry["source_url"], version)
 
-    try:
+    def fetch() -> int:
         if source_type == "zip":
-            strip = entry.get("strip_components", 0)
-            file_count = _download_and_extract_zip(
-                source_url, local_cache, exclude, strip
+            return _download_and_extract_zip(
+                source_url, local_cache, exclude, entry.get("strip_components", 0)
             )
-        else:
-            source_path = entry["source_path"].format(version=version)
-            file_count = _download_and_extract(
-                source_url, source_path, local_cache, exclude
-            )
-    except (
-        urllib.error.URLError,
-        OSError,
-        tarfile.TarError,
-        zipfile.BadZipFile,
-        NothingExtractedError,
-    ) as exc:
-        log.warning("[%s] download failed: %s", key, exc)
-        return None
+        source_path = entry["source_path"].format(version=version)
+        return _download_and_extract(source_url, source_path, local_cache, exclude)
+
+    # An archive that reads but holds nothing under the path is the same on
+    # every try: only a failed or unreadable download is tried again.
+    for attempt, wait in enumerate((*DOWNLOAD_RETRY_WAITS, None), start=1):
+        try:
+            file_count = fetch()
+            break
+        except NothingExtractedError as exc:
+            log.warning("[%s] download failed: %s", key, exc)
+            return None
+        except _TRANSIENT as exc:
+            if wait is None:
+                log.warning("[%s] download failed after %d tries: %s", key, attempt, exc)
+                return None
+            log.warning("[%s] download failed (%s), trying again in %d s", key, exc, wait)
+            time.sleep(wait)
 
     _record_version(key, {"sha": remote_tag or "", "version": version}, versions_path)
 

tests/test_refresh_failures.py

@@ -15,6 +15,7 @@ import subprocess
 import sys
 import tempfile
 import unittest
+import zipfile
 from pathlib import Path
 from unittest import mock
 
@@ -215,6 +216,50 @@ class TheVersionIsReadBeforeTheDownload(unittest.TestCase):
         self.assertEqual(json.loads(versions.read_text())["sys"]["sha"], "build-1")
 
 
+class ABadResponseIsTriedAgain(unittest.TestCase):
+    """The buildbot once served something that was not ScummVM.zip. CI
+    failed the whole data step on it, and the next download was fine."""
+
+    def setUp(self) -> None:
+        import refresh_data_dirs as rdd
+
+        self.rdd = rdd
+        root = Path(tempfile.mkdtemp(dir=REPO_ROOT / "tmp"))
+        self.addCleanup(shutil.rmtree, root, True)
+        self.versions = str(root / "versions.json")
+        self.entry = {"source_type": "zip", "source_url": "https://example.invalid/S.zip",
+                      "local_cache": str(root / "cache")}
+        self.waits: list[int] = []
+
+    def run_with(self, outcomes: list) -> bool | None:
+        calls = iter(outcomes)
+
+        def download(*_args, **_kwargs):
+            outcome = next(calls)
+            if isinstance(outcome, Exception):
+                raise outcome
+            return outcome
+
+        with mock.patch.object(self.rdd, "_get_remote_etag", lambda _url: "tag"), \
+                mock.patch.object(self.rdd, "_download_and_extract_zip", download), \
+                mock.patch.object(self.rdd.time, "sleep", self.waits.append):
+            return self.rdd._refresh_entry("s", self.entry, True, False, self.versions)
+
+    def test_a_bad_archive_then_a_good_one_refreshes(self):
+        self.assertTrue(self.run_with([zipfile.BadZipFile("File is not a zip file"), 12]))
+        self.assertEqual(self.waits, [self.rdd.DOWNLOAD_RETRY_WAITS[0]])
+
+    def test_a_source_bad_on_every_try_fails_after_them(self):
+        bad = zipfile.BadZipFile("File is not a zip file")
+        tries = len(self.rdd.DOWNLOAD_RETRY_WAITS) + 1
+        self.assertIsNone(self.run_with([bad] * tries))
+        self.assertEqual(self.waits, list(self.rdd.DOWNLOAD_RETRY_WAITS))
+
+    def test_an_empty_archive_is_not_tried_again(self):
+        self.assertIsNone(self.run_with([self.rdd.NothingExtractedError("nothing")]))
+        self.assertEqual(self.waits, [])
+
+
 class AKeyMustServeThePlatform(unittest.TestCase):
     """--platform narrowed only a full refresh: under --key it was ignored and
     dolphin-sys was refreshed for RomM, which never reads it."""

tests/test_workflow_paths.py

@@ -143,6 +143,9 @@ class ThePullRequestBuildsTheSite(unittest.TestCase):
         suite = next(i for i, run in enumerate(runs) if "unittest discover" in run)
         self.assertLess(generate, suite)
         self.assertEqual(steps[suite].get("if"), "${{ !cancelled() }}")
+        # A failed data fetch skipped the generator, and all 228 site tests
+        # then failed under it: the generator runs whatever came before.
+        self.assertEqual(steps[generate].get("if"), "${{ !cancelled() }}")
 
     def test_every_site_input_triggers_validation(self):
         deploy = self._workflow("deploy-site.yml")