Public Access
DEVX-156: Add retry with backoff to install_tools download for transient network failures #255
@@ -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