feat(python/robomp): retried transient 5xx errors for idempotent requests
- Add automatic retries for transient 5xx status codes on idempotent request methods. - Ensure non-idempotent methods like POST fail immediately without retry.
This commit is contained in:
@@ -230,7 +230,16 @@ class GitHubClient:
|
||||
return resp.json()
|
||||
|
||||
_TRANSIENT_RETRY_DELAYS = (1.0, 3.0, 10.0)
|
||||
"""Backoff schedule for transient connection/timeout errors."""
|
||||
"""Backoff schedule for transient connection/timeout/5xx errors."""
|
||||
|
||||
_TRANSIENT_STATUSES = frozenset({500, 502, 503, 504})
|
||||
"""Upstream statuses treated as transient — retried for idempotent methods only."""
|
||||
|
||||
_IDEMPOTENT_METHODS = frozenset({"GET", "HEAD"})
|
||||
"""Methods safe to replay: a lost response cannot have caused a visible write."""
|
||||
|
||||
def _transient_5xx(self, method: str, exc: GitHubError) -> bool:
|
||||
return method.upper() in self._IDEMPOTENT_METHODS and exc.status in self._TRANSIENT_STATUSES
|
||||
|
||||
def request_sync(
|
||||
self, method: str, path: str, *, json: Mapping[str, Any] | None = None, params: Mapping[str, Any] | None = None
|
||||
@@ -250,6 +259,15 @@ class GitHubClient:
|
||||
extra={"method": method, "path": path, "attempt": attempt + 1, "delay": delay, "error": str(exc)},
|
||||
)
|
||||
time.sleep(delay)
|
||||
except GitHubError as exc:
|
||||
if delay is None or not self._transient_5xx(method, exc):
|
||||
raise
|
||||
last_exc = exc
|
||||
log.warning(
|
||||
"transient github 5xx, retrying",
|
||||
extra={"method": method, "path": path, "attempt": attempt + 1, "delay": delay, "status": exc.status},
|
||||
)
|
||||
time.sleep(delay)
|
||||
raise last_exc # type: ignore[misc]
|
||||
|
||||
async def request(
|
||||
@@ -270,6 +288,15 @@ class GitHubClient:
|
||||
extra={"method": method, "path": path, "attempt": attempt + 1, "delay": delay, "error": str(exc)},
|
||||
)
|
||||
await asyncio.sleep(delay)
|
||||
except GitHubError as exc:
|
||||
if delay is None or not self._transient_5xx(method, exc):
|
||||
raise
|
||||
last_exc = exc
|
||||
log.warning(
|
||||
"transient github 5xx, retrying",
|
||||
extra={"method": method, "path": path, "attempt": attempt + 1, "delay": delay, "status": exc.status},
|
||||
)
|
||||
await asyncio.sleep(delay)
|
||||
raise last_exc # type: ignore[misc]
|
||||
|
||||
# ---- repos / issues / comments / PRs ----
|
||||
|
||||
@@ -62,6 +62,34 @@ def test_redirect_without_follow_raises_github_error() -> None:
|
||||
assert exc.value.status in (301, 410)
|
||||
|
||||
|
||||
def test_transient_5xx_retries_get_but_not_post(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""A transient upstream 500 must be replayed for idempotent GETs (the
|
||||
manual-triage fetch path) and surfaced immediately for non-idempotent
|
||||
POSTs, where a blind replay could double-apply a write."""
|
||||
monkeypatch.setattr(GitHubClient, "_TRANSIENT_RETRY_DELAYS", (0.01, 0.01))
|
||||
get_calls = 0
|
||||
post_calls = 0
|
||||
|
||||
def handler(request: httpx.Request) -> httpx.Response:
|
||||
nonlocal get_calls, post_calls
|
||||
if request.method == "POST":
|
||||
post_calls += 1
|
||||
return httpx.Response(500, json={"message": "boom"})
|
||||
get_calls += 1
|
||||
if get_calls == 1:
|
||||
return httpx.Response(500, json={"message": "boom"})
|
||||
return httpx.Response(200, json={"ok": True})
|
||||
|
||||
client = GitHubClient("tok", transport=httpx.MockTransport(handler))
|
||||
assert _run_async(client.request("GET", "/x")) == {"ok": True}
|
||||
assert get_calls == 2
|
||||
|
||||
with pytest.raises(GitHubError) as exc:
|
||||
_run_async(client.request("POST", "/x", json={}))
|
||||
assert exc.value.status == 500
|
||||
assert post_calls == 1
|
||||
|
||||
|
||||
def test_redirect_target_succeeds_when_followable() -> None:
|
||||
"""A 301 → 200 chain should resolve to the followed payload."""
|
||||
|
||||
|
||||
Reference in New Issue
Block a user