diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 75fb53dbd..fc6d1fc69 100644 --- a/.github/workflows/validate.yml +++ b/.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 diff --git a/scripts/refresh_data_dirs.py b/scripts/refresh_data_dirs.py index 5a8ba1140..a7059e666 100644 --- a/scripts/refresh_data_dirs.py +++ b/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) diff --git a/tests/test_refresh_failures.py b/tests/test_refresh_failures.py index 524d354b9..2225ccfe5 100644 --- a/tests/test_refresh_failures.py +++ b/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.""" diff --git a/tests/test_workflow_paths.py b/tests/test_workflow_paths.py index e9b714746..f1a791a8d 100644 --- a/tests/test_workflow_paths.py +++ b/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")