diff --git a/CHANGELOG.md b/CHANGELOG.md index 441b1c4a2..ae68c2fdb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Best-effort commits API lookups now fall through promptly to Git when + rate-limited, avoiding multi-minute dependency resolution stalls -- by + @danielmeppiel (#2238). - Azure DevOps dependencies now report real latest versions in `apm outdated` and resolve correctly during bounded `apm update`, deriving transport coordinates from generic `host` and `repo_url` identity without diff --git a/src/apm_cli/deps/download_strategies.py b/src/apm_cli/deps/download_strategies.py index e09d03a07..c3e34612f 100644 --- a/src/apm_cli/deps/download_strategies.py +++ b/src/apm_cli/deps/download_strategies.py @@ -134,7 +134,7 @@ def resilient_get( url: Request URL headers: HTTP headers timeout: Request timeout in seconds - max_retries: Maximum retry attempts for transient failures + max_retries: Maximum total attempts, including the initial request stream: Whether to stream the response body instead of buffering it Returns: @@ -177,13 +177,18 @@ def resilient_get( wait = min(2**attempt, 30) * (0.5 + random.random()) # noqa: S311 else: wait = min(2**attempt, 30) * (0.5 + random.random()) # noqa: S311 - _debug( - f"Rate limited ({response.status_code}), retry in " - f"{wait:.1f}s (attempt {attempt + 1}/{max_retries})" - ) if attempt < max_retries - 1: + _debug( + f"Rate limited ({response.status_code}), retry in " + f"{wait:.1f}s (attempt {attempt + 1}/{max_retries})" + ) _close_response(response, "rate-limit retry") - time.sleep(wait) + time.sleep(wait) + else: + _debug( + f"Rate limited ({response.status_code}), no retries left " + f"(attempt {attempt + 1}/{max_retries})" + ) continue # Log rate limit proximity diff --git a/src/apm_cli/deps/git_reference_resolver.py b/src/apm_cli/deps/git_reference_resolver.py index 1745d0e46..1630575a6 100644 --- a/src/apm_cli/deps/git_reference_resolver.py +++ b/src/apm_cli/deps/git_reference_resolver.py @@ -44,6 +44,8 @@ ) if TYPE_CHECKING: + import requests + from ..core.auth import AuthResolver from .transport_selection import ProtocolPreference, TransportSelector @@ -85,7 +87,13 @@ def _build_repo_url( ) -> str: ... def _clone_with_fallback(self, *args, **kwargs): ... def _sanitize_git_error(self, error_message: str) -> str: ... - def _resilient_get(self, url: str, headers: dict, timeout: int = ...): ... + def _resilient_get( + self, + url: str, + headers: dict[str, str], + timeout: int = ..., + max_retries: int = ..., + ) -> requests.Response: ... def _parse_ls_remote_output(self, output: str) -> list[RemoteRef]: ... def _sort_remote_refs(self, refs: list[RemoteRef]) -> list[RemoteRef]: ... def _parse_artifactory_base_url(self) -> tuple | None: ... @@ -311,7 +319,8 @@ def resolve_commit_sha_for_ref(self, dep_ref: DependencyReference, ref: str) -> headers["Authorization"] = f"token {token}" try: - response = host._resilient_get(api_url, headers=headers, timeout=10) + # L2/L3 own fallback, so this optional metadata tier gets one total attempt. + response = host._resilient_get(api_url, headers=headers, timeout=10, max_retries=1) if response.status_code != 200: return None body = (response.text or "").strip() diff --git a/src/apm_cli/deps/tiered_ref_resolver.py b/src/apm_cli/deps/tiered_ref_resolver.py index 8d3edbf1e..d6cb9a0b1 100644 --- a/src/apm_cli/deps/tiered_ref_resolver.py +++ b/src/apm_cli/deps/tiered_ref_resolver.py @@ -154,9 +154,10 @@ class L1CommitsAPI: Delegates to :meth:`GitReferenceResolver.resolve_commit_sha_for_ref` -- the cheap-path helper that already dispatches through ``host_backends.build_commits_api_url`` and ``host._resilient_get``, - inheriting that helper's auth + retry behavior. Returns ``None`` for - hosts whose backend has no cheap commits endpoint (e.g. ADO today); - the caller then falls through to L2/L3. + inheriting that helper's auth behavior. The optional metadata lookup + makes one HTTP attempt so rate limiting cannot delay L2/L3 fallback. + Returns ``None`` for hosts whose backend has no cheap commits endpoint + (e.g. ADO today); the caller then falls through to L2/L3. Future: an explicit :class:`HttpCache` ETag pass could be added here for unauthenticated requests (the underlying helper does not yet diff --git a/tests/unit/deps/test_download_strategies_phase3.py b/tests/unit/deps/test_download_strategies_phase3.py index 2e7b0f08e..8feec7c5d 100644 --- a/tests/unit/deps/test_download_strategies_phase3.py +++ b/tests/unit/deps/test_download_strategies_phase3.py @@ -227,15 +227,22 @@ def test_403_without_rate_limit_header_is_returned_immediately(self) -> None: assert result is forbidden_resp assert mock_get.call_count == 1 - def test_rate_limit_exhausts_retries_returns_last_response(self) -> None: + def test_rate_limit_exhausts_retries_returns_last_response( + self, capsys: pytest.CaptureFixture[str] + ) -> None: """If every attempt is rate-limited, the last rate-limit response is returned.""" rate_resp = _fake_response(429, b"rate", headers={"Retry-After": "0.001"}) with ( patch("requests.get", return_value=rate_resp), - patch("time.sleep"), + patch("time.sleep") as mock_sleep, + patch.dict("os.environ", {"APM_DEBUG": "1"}), ): result = self.delegate.resilient_get("https://example.com", {}, max_retries=2) assert result is rate_resp + assert mock_sleep.call_count == 1 + captured = capsys.readouterr() + assert captured.err.count("retry in") == 1 + assert "no retries left (attempt 2/2)" in captured.err def test_retry_after_invalid_falls_back_to_backoff(self) -> None: """Non-numeric Retry-After header falls back to exponential back-off.""" diff --git a/tests/unit/deps/test_git_reference_resolver.py b/tests/unit/deps/test_git_reference_resolver.py index 103372800..b5855ca2c 100644 --- a/tests/unit/deps/test_git_reference_resolver.py +++ b/tests/unit/deps/test_git_reference_resolver.py @@ -14,7 +14,11 @@ import os import sys +import threading import types +from collections.abc import Iterator +from contextlib import contextmanager +from http.server import BaseHTTPRequestHandler, HTTPServer from unittest.mock import MagicMock, patch import pytest @@ -23,6 +27,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "..", "src")) from apm_cli.deps.git_reference_resolver import GitReferenceResolver +from apm_cli.deps.github_downloader import GitHubPackageDownloader from apm_cli.deps.transport_selection import ( NoOpInsteadOfResolver, ProtocolPreference, @@ -110,6 +115,41 @@ def _ctx( return host +@contextmanager +def _loopback_http_stub( + responses: list[tuple[int, dict[str, str], bytes]], +) -> Iterator[tuple[types.SimpleNamespace, str]]: + """Serve deterministic HTTP responses from loopback.""" + if not responses: + raise ValueError("responses must not be empty") + + state = types.SimpleNamespace(hits=0) + + class Handler(BaseHTTPRequestHandler): + def do_GET(self) -> None: + index = min(state.hits, len(responses) - 1) + status, headers, body = responses[index] + state.hits += 1 + self.send_response(status) + for name, value in headers.items(): + self.send_header(name, value) + self.end_headers() + self.wfile.write(body) + + def log_message(self, format: str, *args: object) -> None: + return + + server = HTTPServer(("127.0.0.1", 0), Handler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + yield state, f"http://127.0.0.1:{server.server_port}/commit/main" + finally: + server.shutdown() + thread.join(timeout=2) + server.server_close() + + # --------------------------------------------------------------------------- # resolve_commit_sha_for_ref # --------------------------------------------------------------------------- @@ -177,6 +217,59 @@ def test_no_commits_api_returns_none(self): # Use a generic-host dep -- generic backend returns None for commits API assert resolver.resolve_commit_sha_for_ref(_dep(host="git.example.com"), "main") is None + def test_best_effort_rate_limit_falls_through_without_retry_wait(self): + """The optional commits-API tier must not stall the git fallback.""" + downloader = GitHubPackageDownloader() + dep = _dep(host="github.com", reference="main") + responses = [ + ( + 403, + {"X-RateLimit-Remaining": "0", "Retry-After": "60"}, + b"rate limited", + ) + ] + + with ( + _loopback_http_stub(responses) as (server, api_url), + patch( + "apm_cli.deps.host_backends.backend_for", + return_value=types.SimpleNamespace( + build_commits_api_url=lambda dep_ref, ref: api_url + ), + ), + patch.object( + downloader.auth_resolver, + "resolve", + return_value=types.SimpleNamespace(token=None), + ), + patch("apm_cli.deps.download_strategies.time.sleep") as sleep, + ): + result = downloader._refs.resolve_commit_sha_for_ref(dep, "main") + + assert result is None + assert server.hits == 1 + sleep.assert_not_called() + + def test_primary_http_transient_rate_limit_still_retries(self): + """Primary HTTP work keeps the shared transient retry policy.""" + downloader = GitHubPackageDownloader() + sha = b"deadbeef" + (b"0" * 32) + responses = [ + (429, {"Retry-After": "0.01"}, b"rate limited"), + (200, {}, sha), + ] + + with ( + _loopback_http_stub(responses) as (server, api_url), + patch("apm_cli.deps.download_strategies.time.sleep") as sleep, + ): + response = downloader._resilient_get(api_url, {}, timeout=1) + + assert response.status_code == 200 + assert response.content == sha + assert server.hits == 2 + sleep.assert_called_once_with(0.01) + # --------------------------------------------------------------------------- # list_remote_refs