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",