diff --git a/vinca/distro.py b/vinca/distro.py index cdb3420..727f906 100644 --- a/vinca/distro.py +++ b/vinca/distro.py @@ -2,6 +2,7 @@ import os import posixpath import tarfile +import time import urllib.parse import zipfile from concurrent.futures import ThreadPoolExecutor @@ -12,6 +13,10 @@ from rosdistro.dependency_walker import DependencyWalker from rosdistro.manifest_provider import get_release_tag +# Retries for fetching package.xml files (raw.githubusercontent.com is flaky in CI). +_FETCH_ATTEMPTS = 5 +_RETRY_STATUS = {429, 500, 502, 503, 504} + # Source archive suffixes an additional recipe may point at instead of a git repository. # Such a package is fetched by URL (checksummed with sha256) rather than cloned. # Zstandard is left out on purpose: tarfile only reads it from Python 3.14 on, while @@ -477,8 +482,26 @@ def _get(self, url): requests also drops the Authorization header when a redirect leaves the original host, which matters because an Artifactory server usually redirects a download to a presigned URL on a separate storage host. + + Transient failures (connection errors, timeouts, 429 and 5xx responses, e.g. + raw.githubusercontent.com's "503 first byte timeout") are retried with + exponential backoff before giving up. """ - response = requests.get(url, headers=self._get_auth_headers(url), timeout=60) + headers = self._get_auth_headers(url) + for attempt in range(1, _FETCH_ATTEMPTS): + try: + response = requests.get(url, headers=headers, timeout=60) + status = getattr(response, "status_code", 200) + if status not in _RETRY_STATUS: + response.raise_for_status() + return response + error = f"{status} for url: {url}" + except (requests.ConnectionError, requests.Timeout) as exc: + error = exc + delay = 2**attempt + print(f"Fetching {url} failed ({error}); retrying in {delay}s") + time.sleep(delay) + response = requests.get(url, headers=headers, timeout=60) response.raise_for_status() return response diff --git a/vinca/test_fetch_retry.py b/vinca/test_fetch_retry.py new file mode 100644 index 0000000..ca62093 --- /dev/null +++ b/vinca/test_fetch_retry.py @@ -0,0 +1,58 @@ +from types import SimpleNamespace +from unittest.mock import Mock + +import pytest +import requests + +from vinca import distro as distro_module +from vinca.distro import Distro + + +def fake_distro(): + return SimpleNamespace(_get_auth_headers=lambda url: {}) + + +def response(status): + r = Mock(status_code=status) + if status >= 400: + r.raise_for_status.side_effect = requests.HTTPError(f"{status}", response=r) + return r + + +def test_retries_transient_failures(monkeypatch): + calls = [requests.ConnectionError("reset"), response(503), response(200)] + get = Mock( + side_effect=lambda *a, **k: (_ for _ in ()).throw(c) + if isinstance(c := calls.pop(0), Exception) + else c + ) + monkeypatch.setattr(distro_module.requests, "get", get) + sleep = Mock() + monkeypatch.setattr(distro_module.time, "sleep", sleep) + + assert ( + Distro._get(fake_distro(), "https://raw.githubusercontent.com/x").status_code + == 200 + ) + assert get.call_count == 3 + assert [c.args[0] for c in sleep.call_args_list] == [2, 4] + + +def test_does_not_retry_client_errors(monkeypatch): + get = Mock(return_value=response(404)) + monkeypatch.setattr(distro_module.requests, "get", get) + monkeypatch.setattr(distro_module.time, "sleep", Mock()) + + with pytest.raises(requests.HTTPError): + Distro._get(fake_distro(), "https://raw.githubusercontent.com/x") + assert get.call_count == 1 + + +def test_gives_up_after_the_last_attempt(monkeypatch): + get = Mock(return_value=response(503)) + monkeypatch.setattr(distro_module.requests, "get", get) + monkeypatch.setattr(distro_module.time, "sleep", Mock()) + + with pytest.raises(requests.HTTPError): + Distro._get(fake_distro(), "https://raw.githubusercontent.com/x") + assert get.call_count == distro_module._FETCH_ATTEMPTS