diff --git a/src/devx/tools/install_tools.py b/src/devx/tools/install_tools.py index 71e47be..51aa39c 100644 --- a/src/devx/tools/install_tools.py +++ b/src/devx/tools/install_tools.py @@ -22,18 +22,37 @@ Usage:: from __future__ import annotations +import logging import os import platform import shutil import tarfile import tempfile +import time +import urllib.error import urllib.request from pathlib import Path import click +from tenacity import ( + Retrying, + before_sleep_log, + retry_if_exception_type, + stop_after_attempt, + wait_exponential, +) TARGET_DIR = Path.home() / ".local" / "bin" +logger = logging.getLogger(__name__) + +# Retry configuration for transient network failures during download. +# GitHub releases occasionally drops connections ("Remote end closed +# connection without response"). Retrying with backoff before falling +# through to the next fallback URL makes the build resilient to +# momentary network blips. +MAX_DOWNLOAD_RETRIES = 3 + ACTIONLINT_VERSION = "1.7.12" GIT_CLIFF_VERSION = "2.13.1" @@ -64,8 +83,30 @@ def _ensure_target_dir() -> Path: return TARGET_DIR -def _download(url: str, dest: Path) -> None: - """Download a file from ``url`` to ``dest`` with a 60s timeout.""" +def _download(url: str, dest: Path, *, _sleep=None) -> None: + """Download a file from ``url`` to ``dest`` with retry and 60s timeout. + + Retries up to ``MAX_DOWNLOAD_RETRIES`` times on transient network + errors (``URLError``, ``OSError`` from connection resets) using + exponential backoff. This handles momentary GitHub releases + connection drops that were causing CI image builds to fail. + + The ``_sleep`` kwarg is for tests to avoid real sleeping; production + code should leave it as ``None`` (uses ``time.sleep``). + """ + retrying = Retrying( + stop=stop_after_attempt(MAX_DOWNLOAD_RETRIES), + wait=wait_exponential(multiplier=2, min=2, max=10), + retry=retry_if_exception_type((urllib.error.URLError, OSError, ConnectionError)), + before_sleep=before_sleep_log(logger, logging.WARNING), + sleep=_sleep if _sleep is not None else time.sleep, + reraise=True, + ) + retrying(_do_download, url, dest) + + +def _do_download(url: str, dest: Path) -> None: + """Single download attempt — called by :func:`_download` retry wrapper.""" with urllib.request.urlopen(url, timeout=60) as resp, open(dest, "wb") as f: # nosec B310 shutil.copyfileobj(resp, f) diff --git a/tests/unit/test_install_tools.py b/tests/unit/test_install_tools.py index 8abe95f..96c7dd4 100644 --- a/tests/unit/test_install_tools.py +++ b/tests/unit/test_install_tools.py @@ -1,6 +1,7 @@ from __future__ import annotations import platform +import urllib.error from pathlib import Path from unittest.mock import patch @@ -68,6 +69,67 @@ class TestDownload: mock_urlopen.assert_called_once() assert dest.read_bytes() == b"data" + def test_download_retries_on_transient_error(self, tmp_path: Path) -> None: + """Download retries on URLError then succeeds.""" + dest = tmp_path / "file.bin" + call_count = [0] + + class _FakeResponse: + def __init__(self) -> None: + self._sent = False + + def __enter__(self) -> _FakeResponse: + return self + + def __exit__(self, *args: object) -> None: + pass + + def read(self, n: int = -1) -> bytes: + if self._sent: + return b"" + self._sent = True + return b"data" + + def _flaky_urlopen(url: str, timeout: int = 60): + call_count[0] += 1 + if call_count[0] < 2: + raise urllib.error.URLError("Remote end closed connection") + return _FakeResponse() + + with patch("urllib.request.urlopen", side_effect=_flaky_urlopen): + install_tools._download("https://example.com/file", dest, _sleep=lambda _: None) + assert call_count[0] == 2 + assert dest.read_bytes() == b"data" + + def test_download_fails_after_max_retries(self, tmp_path: Path) -> None: + """Download raises after MAX_DOWNLOAD_RETRIES attempts.""" + dest = tmp_path / "file.bin" + call_count = [0] + + def _always_fail(url: str, timeout: int = 60): + call_count[0] += 1 + raise urllib.error.URLError("Remote end closed connection") + + with patch("urllib.request.urlopen", side_effect=_always_fail): + with pytest.raises(urllib.error.URLError): + install_tools._download("https://example.com/file", dest, _sleep=lambda _: None) + assert call_count[0] == install_tools.MAX_DOWNLOAD_RETRIES + assert not dest.exists() + + def test_download_no_retry_on_non_transient_error(self, tmp_path: Path) -> None: + """Download does not retry on non-network errors (e.g. ValueError).""" + dest = tmp_path / "file.bin" + call_count = [0] + + def _fail_with_value_error(url: str, timeout: int = 60): + call_count[0] += 1 + raise ValueError("not a network error") + + with patch("urllib.request.urlopen", side_effect=_fail_with_value_error): + with pytest.raises(ValueError): + install_tools._download("https://example.com/file", dest, _sleep=lambda _: None) + assert call_count[0] == 1 + class TestDownloadBinary: def test_download(self, tmp_path: Path) -> None: