From b2533d3833f62ee9126449ef8dd569d642a59e82 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Sun, 2 Aug 2026 01:01:14 -0400 Subject: [PATCH] fix: survive short network outages when downloading (#2953) * fix: use exponential backoff when a download fails The retry used a flat 3 second wait over 3 attempts, which covers only about 6 seconds of downtime. A CI runner lost DNS for approximately a minute and the PyPy download failed. Wait 3, 6, 12, 24, then 48 seconds over 6 attempts. Assisted-by: ClaudeCode:claude-opus-5 * ci: raise the uv HTTP retry count uv stops after 3 retries, which was too few when a runner lost DNS for approximately a minute during a test run. Assisted-by: ClaudeCode:claude-opus-5 * fix: do not retry a download after a client error A bad URL will not fix itself, and the longer backoff made the wait before the report about 93 seconds. Report 4xx responses at once, and keep retrying 5xx. Assisted-by: ClaudeCode:claude-opus-5 --- .github/workflows/test.yml | 4 ++ cibuildwheel/util/file.py | 13 +++- unit_test/download_test.py | 135 +++++++++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ff6b445d..5118b922 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -34,6 +34,10 @@ concurrency: group: test-${{ github.ref }} cancel-in-progress: true +env: + # runners see short DNS/network outages; uv's default of 3 retries is too few + UV_HTTP_RETRIES: "6" + jobs: lint: name: Linters (mypy, ruff, etc.) diff --git a/cibuildwheel/util/file.py b/cibuildwheel/util/file.py index 32b0a4fc..951fdf3f 100644 --- a/cibuildwheel/util/file.py +++ b/cibuildwheel/util/file.py @@ -9,6 +9,7 @@ __lazy_modules__ = { "tarfile", "typing", "urllib", + "urllib.error", "urllib.request", "zipfile", } @@ -20,6 +21,7 @@ import shutil import ssl import tarfile import time +import urllib.error import urllib.request from contextlib import contextmanager from pathlib import Path, PurePath @@ -86,17 +88,24 @@ def download(url: str, dest: Path, *, sha256: str | None = None) -> None: # so we use certifi (this sounds odd but requests also does this by default) cafile = os.environ.get("SSL_CERT_FILE", certifi.where()) context = ssl.create_default_context(cafile=cafile) - repeat_num = 3 + # exponential backoff, so that a network outage of about a minute is survivable + repeat_num = 6 for i in range(repeat_num): try: with urllib.request.urlopen(url, context=context) as response: dest.write_bytes(response.read()) break + except urllib.error.HTTPError as error: + # a client error, such as a bad URL, will not fix itself + if i == repeat_num - 1 or 400 <= error.code < 500: + raise + time.sleep(3 * 2**i) + except OSError: if i == repeat_num - 1: raise - time.sleep(3) + time.sleep(3 * 2**i) if sha256: with dest.open("rb") as f: diff --git a/unit_test/download_test.py b/unit_test/download_test.py index 0508b5f2..7a8826ac 100644 --- a/unit_test/download_test.py +++ b/unit_test/download_test.py @@ -1,6 +1,10 @@ from __future__ import annotations +import io import ssl +import time +import urllib.error +import urllib.request import certifi import pytest @@ -9,9 +13,12 @@ from cibuildwheel.util.file import download TYPE_CHECKING = False if TYPE_CHECKING: + from collections.abc import Callable, Iterator from pathlib import Path + from typing import Self DOWNLOAD_URL = "https://cdn.jsdelivr.net/gh/pypa/cibuildwheel@v1.6.3/requirements-dev.txt" +PAYLOAD = b"payload" def test_download(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: @@ -35,3 +42,131 @@ def test_download_bad_ssl_cert_file(monkeypatch: pytest.MonkeyPatch, tmp_path: P dest = tmp_path / "file.txt" with pytest.raises(ssl.SSLError): download(DOWNLOAD_URL, dest) + + +def _no_sleep(seconds: float) -> None: + pass + + +@pytest.fixture +def http_error() -> Iterator[Callable[[str, int], urllib.error.HTTPError]]: + """Build HTTPErrors, and close them, as each one holds a temporary file.""" + errors = [] + + def make(url: str, code: int) -> urllib.error.HTTPError: + error = urllib.error.HTTPError(url, code, "error", {}, io.BytesIO(b"")) # type: ignore[arg-type] + errors.append(error) + return error + + yield make + + for error in errors: + error.close() + + +class FakeResponse: + def __enter__(self) -> Self: + return self + + def __exit__(self, *args: object) -> None: + return None + + def read(self) -> bytes: + return PAYLOAD + + +@pytest.fixture +def fake_network( + monkeypatch: pytest.MonkeyPatch, +) -> Callable[[int], tuple[list[str], list[float]]]: + """Fail the first ``failures`` downloads, recording attempts and sleeps.""" + + def setup(failures: int) -> tuple[list[str], list[float]]: + attempts: list[str] = [] + sleeps: list[float] = [] + + def fake_urlopen(url: str, context: object = None) -> FakeResponse: # noqa: ARG001 + attempts.append(url) + if len(attempts) <= failures: + msg = "temporary DNS failure" + raise OSError(msg) + return FakeResponse() + + monkeypatch.setattr(urllib.request, "urlopen", fake_urlopen) + monkeypatch.setattr(time, "sleep", sleeps.append) + return attempts, sleeps + + return setup + + +def test_download_retries_transient_failures( + fake_network: Callable[[int], tuple[list[str], list[float]]], tmp_path: Path +) -> None: + attempts, sleeps = fake_network(3) + dest = tmp_path / "file.txt" + + download(DOWNLOAD_URL, dest) + + assert dest.read_bytes() == PAYLOAD + assert len(attempts) == 4 + # the wait must grow, so that a long outage is survivable + assert sleeps == sorted(sleeps) + assert sleeps[-1] > sleeps[0] + + +def test_download_backoff_covers_a_minute_outage( + fake_network: Callable[[int], tuple[list[str], list[float]]], tmp_path: Path +) -> None: + _, sleeps = fake_network(99) + dest = tmp_path / "file.txt" + + with pytest.raises(OSError, match="temporary DNS failure"): + download(DOWNLOAD_URL, dest) + + assert sum(sleeps) >= 60 + + +@pytest.mark.parametrize("code", [400, 404, 410]) +def test_download_does_not_retry_client_errors( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + code: int, + http_error: Callable[[str, int], urllib.error.HTTPError], +) -> None: + """A bad URL is not going to fix itself, so report it at once.""" + attempts: list[str] = [] + + def fake_urlopen(url: str, context: object = None) -> FakeResponse: # noqa: ARG001 + attempts.append(url) + raise http_error(url, code) + + monkeypatch.setattr(urllib.request, "urlopen", fake_urlopen) + monkeypatch.setattr(time, "sleep", _no_sleep) + + with pytest.raises(urllib.error.HTTPError): + download(DOWNLOAD_URL, tmp_path / "file.txt") + + assert len(attempts) == 1 + + +@pytest.mark.parametrize("code", [500, 503]) +def test_download_retries_server_errors( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + code: int, + http_error: Callable[[str, int], urllib.error.HTTPError], +) -> None: + attempts: list[str] = [] + + def fake_urlopen(url: str, context: object = None) -> FakeResponse: # noqa: ARG001 + attempts.append(url) + if len(attempts) == 1: + raise http_error(url, code) + return FakeResponse() + + monkeypatch.setattr(urllib.request, "urlopen", fake_urlopen) + monkeypatch.setattr(time, "sleep", _no_sleep) + + download(DOWNLOAD_URL, tmp_path / "file.txt") + + assert len(attempts) == 2