From fd8effb8d5c97b5d29b507da13d85329ded15b98 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Wed, 14 Oct 2026 16:48:17 +0000 Subject: [PATCH] fix(dependencies): Retry transient GitHub errors when downloading dependencies GitHub's zipball API intermittently returns 5xx, which devservices never retried, failing devservices up (and sentry CI) on the first blip. Retry 5xx and 429 with exponential backoff, and include the underlying cause in the DependencyError message so failures are diagnosable. Co-Authored-By: Joshua Li <14209781+joshuarli@users.noreply.github.com> --- devservices/exceptions.py | 7 +++- devservices/utils/dependencies.py | 13 +++++- devservices/utils/retry.py | 6 ++- tests/utils/test_dependencies.py | 69 ++++++++++++++++++++++++++++++- 4 files changed, 89 insertions(+), 6 deletions(-) diff --git a/devservices/exceptions.py b/devservices/exceptions.py index 8487d41..ec98f80 100644 --- a/devservices/exceptions.py +++ b/devservices/exceptions.py @@ -99,7 +99,12 @@ def __init__( self.stderr = stderr def __str__(self) -> str: - return f"DependencyError: {self.repo_name} ({self.repo_link}) on {self.branch}" + message = ( + f"DependencyError: {self.repo_name} ({self.repo_link}) on {self.branch}" + ) + if self.__cause__ is not None: + message += f" ({type(self.__cause__).__name__}: {self.__cause__})" + return message class InvalidDependencyConfigError(DependencyError): diff --git a/devservices/utils/dependencies.py b/devservices/utils/dependencies.py index 53ca090..2ffe39f 100644 --- a/devservices/utils/dependencies.py +++ b/devservices/utils/dependencies.py @@ -408,6 +408,14 @@ def install_dependency(dependency: RemoteConfig) -> set[InstalledRemoteDependenc return installed_dependencies +def _is_retryable_download_error(e: BaseException) -> bool: + # GitHub's API intermittently returns 5xx (and 429 when throttled). Those + # are worth retrying; other HTTP errors (404, 401, 403) won't fix themselves. + if isinstance(e, urllib.error.HTTPError): + return e.code >= 500 or e.code == 429 + return True + + def _fetch_dependency( dependency: RemoteConfig, dependency_repo_dir: str, @@ -440,8 +448,11 @@ def _download() -> bytes: zip_data = io.BytesIO( retry( _download, + retries=5, + delay=1.0, + backoff=2.0, exceptions=(OSError,), - should_retry=lambda e: not isinstance(e, urllib.error.HTTPError), + should_retry=_is_retryable_download_error, ) ) except (OSError, ValueError) as e: diff --git a/devservices/utils/retry.py b/devservices/utils/retry.py index 3b72bfd..4137c87 100644 --- a/devservices/utils/retry.py +++ b/devservices/utils/retry.py @@ -12,6 +12,7 @@ def retry( *, retries: int = 3, delay: float = 1.0, + backoff: float = 1.0, exceptions: tuple[type[BaseException], ...] = (Exception,), on_retry: Callable[[BaseException, int], None] | None = None, should_retry: Callable[[BaseException], bool] | None = None, @@ -23,7 +24,8 @@ def retry( Args: fn: Callable to invoke. retries: Total number of attempts (must be >= 1). - delay: Seconds to sleep between attempts. + delay: Seconds to sleep before the first retry. + backoff: Multiplier applied to the delay after each retry. exceptions: Exception types that trigger a retry. on_retry: Optional callback(exc, attempts_remaining) invoked before each sleep. should_retry: Optional predicate; if it returns False the exception is re-raised @@ -41,5 +43,5 @@ def retry( raise if on_retry is not None: on_retry(e, retries - attempt - 1) - time.sleep(delay) + time.sleep(delay * backoff**attempt) raise AssertionError("unreachable") diff --git a/tests/utils/test_dependencies.py b/tests/utils/test_dependencies.py index fa14719..59b5530 100644 --- a/tests/utils/test_dependencies.py +++ b/tests/utils/test_dependencies.py @@ -106,7 +106,7 @@ def test_fetch_dependency_network_error(tmp_path: Path) -> None: _fetch_dependency(dep, str(tmp_path / "dest")) -def test_fetch_dependency_http_error_not_retried(tmp_path: Path) -> None: +def test_fetch_dependency_client_error_not_retried(tmp_path: Path) -> None: dep = RemoteConfig( repo_name="test-repo", branch="main", @@ -125,10 +125,75 @@ def test_fetch_dependency_http_error_not_retried(tmp_path: Path) -> None: ) with urlopen_mock as mock_urlopen, pytest.raises(DependencyError): _fetch_dependency(dep, str(tmp_path / "dest")) - # HTTP errors must not be retried, so urlopen is only ever called once. + # Client errors won't fix themselves, so urlopen is only ever called once. mock_urlopen.assert_called_once() +def _http_error(code: int, msg: str) -> urllib.error.HTTPError: + return urllib.error.HTTPError( + url="https://api.github.com/repos/getsentry/test-repo/zipball/main", + code=code, + msg=msg, + hdrs=None, # type: ignore[arg-type] + fp=None, + ) + + +@pytest.mark.parametrize( + ("code", "msg"), + [(500, "Internal Server Error"), (503, "Service Unavailable"), (429, "Too Many")], +) +def test_fetch_dependency_retries_transient_http_error( + tmp_path: Path, code: int, msg: str +) -> None: + dep = RemoteConfig( + repo_name="test-repo", + branch="main", + repo_link="https://github.com/getsentry/test-repo", + ) + zip_bytes = _make_zip_bytes(BASIC_SERVICE_CONFIG) + dest = str(tmp_path / "dest") + with ( + mock.patch("devservices.utils.retry.time.sleep") as mock_sleep, + mock.patch( + "devservices.utils.dependencies.urllib.request.urlopen", + side_effect=[ + _http_error(code, msg), + _http_error(code, msg), + _make_urlopen_response(zip_bytes), + ], + ) as mock_urlopen, + ): + _fetch_dependency(dep, dest) + assert mock_urlopen.call_count == 3 + assert [c.args[0] for c in mock_sleep.call_args_list] == [1.0, 2.0] + assert (Path(dest) / DEVSERVICES_DIR_NAME / CONFIG_FILE_NAME).exists() + + +def test_fetch_dependency_server_error_exhausts_retries(tmp_path: Path) -> None: + dep = RemoteConfig( + repo_name="test-repo", + branch="main", + repo_link="https://github.com/getsentry/test-repo", + ) + with ( + mock.patch("devservices.utils.retry.time.sleep") as mock_sleep, + mock.patch( + "devservices.utils.dependencies.urllib.request.urlopen", + side_effect=_http_error(500, "Internal Server Error"), + ) as mock_urlopen, + pytest.raises(DependencyError) as exc_info, + ): + _fetch_dependency(dep, str(tmp_path / "dest")) + assert mock_urlopen.call_count == 5 + assert [c.args[0] for c in mock_sleep.call_args_list] == [1.0, 2.0, 4.0, 8.0] + # The underlying cause is surfaced so CI logs show what actually failed. + assert str(exc_info.value) == ( + "DependencyError: test-repo (https://github.com/getsentry/test-repo) on main " + "(HTTPError: HTTP Error 500: Internal Server Error)" + ) + + def test_fetch_dependency_read_error(tmp_path: Path) -> None: dep = RemoteConfig( repo_name="test-repo",