fix: add tenacity retry to install_tools._download for transient network failures
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>
This commit is contained in:
co-authored by
Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
parent
fc5e4634dd
commit
dd603d3e91
@@ -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)
|
||||
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user