From 04a57c958b00d2d2e52e2aac5480d71d33fac705 Mon Sep 17 00:00:00 2001
From: can1357 <me@can.ac>
Date: Thu, 23 Jul 2026 20:41:36 +0200
Subject: [PATCH] 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.
---
 python/robomp/src/github_client.py        | 29 ++++++++++++++++++++++-
 python/robomp/tests/test_github_client.py | 28 ++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/python/robomp/src/github_client.py b/python/robomp/src/github_client.py
index f6b03d088..b03b73297 100644
--- a/python/robomp/src/github_client.py
+++ b/python/robomp/src/github_client.py
@@ -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 ----
diff --git a/python/robomp/tests/test_github_client.py b/python/robomp/tests/test_github_client.py
index c055eee72..0ed67681c 100644
--- a/python/robomp/tests/test_github_client.py
+++ b/python/robomp/tests/test_github_client.py
@@ -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."""