fix(deps): bound best-effort ref lookup retries

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

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

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()

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

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

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