From dd603d3e91f093041abf514533e298310a8f9b71 Mon Sep 17 00:00:00 2001 From: emil Date: Wed, 12 Aug 2026 19:49:20 +0200 Subject: [PATCH] fix: add tenacity retry to install_tools._download for transient network failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The build-images CI job failed because GitHub releases dropped connections ("Remote end closed connection without response") for actionlint, vale, and hadolint simultaneously. The previous code only fell through to the next fallback URL — there was no retry on the same URL for transient connection resets. Now uses tenacity.Retrying with exponential backoff (2-10s, 3 attempts) on URLError/OSError/ConnectionError, matching the pattern already used in devx.utils.network. The _sleep kwarg allows tests to skip real sleeping. 3 new tests cover: retry-then-success, exhaustion-after-max-retries, and no-retry-on-non-transient-errors. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- src/devx/tools/install_tools.py | 45 +++++++++++++++++++++-- tests/unit/test_install_tools.py | 62 ++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 2 deletions(-) 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: -- 2.54.0