-
Notifications
You must be signed in to change notification settings - Fork 32
Retry HTTP 400s that wrap a transient upstream error code #391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| """Code generated by Speakeasy (https://speakeasy.com). DO NOT EDIT.""" | ||
|
|
||
| import asyncio | ||
| import json | ||
| import random | ||
| import time | ||
| from datetime import datetime | ||
|
|
@@ -150,6 +151,24 @@ def _parse_retry_after_ms_header(response: httpx.Response) -> Optional[int]: | |
| return None | ||
|
|
||
|
|
||
| TRANSIENT_INNER_ERROR_CODES = {408, 429, 500, 502, 503, 504, 524, 529} | ||
|
|
||
|
|
||
| def _wraps_transient_inner_code(response: httpx.Response) -> bool: | ||
| """OpenRouter (a middleman) sometimes wraps a transient upstream failure in an HTTP | ||
| 400 whose JSON error.code holds the real status; retry those. Ordinary 400 | ||
| invalid-request responses (including inner error.code == 400) stay non-retryable.""" | ||
| try: | ||
| body = response.json() | ||
| except (json.JSONDecodeError, httpx.ResponseNotRead): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] DetailsWhy: That exception escapes Fix: try:
body = response.json()
except (json.JSONDecodeError, UnicodeDecodeError, httpx.ResponseNotRead):
return FalseRef: Prompt for agentsReviewed at |
||
| return False | ||
| error = body.get("error") if isinstance(body, dict) else None | ||
| code = error.get("code") if isinstance(error, dict) else None | ||
| if isinstance(code, str) and code.isdigit(): | ||
| code = int(code) | ||
| return code in TRANSIENT_INNER_ERROR_CODES | ||
|
|
||
|
|
||
| def _get_sleep_interval( | ||
| exception: Exception, | ||
| initial_interval: int, | ||
|
|
@@ -207,6 +226,12 @@ def do_request() -> httpx.Response: | |
|
|
||
| if res.status_code == parsed_code: | ||
| raise TemporaryError(res) | ||
|
|
||
| if res.status_code == 400: | ||
| if not res.is_closed: | ||
| res.read() | ||
| if _wraps_transient_inner_code(res): | ||
| raise TemporaryError(res) | ||
| except (httpx.NetworkError, httpx.TimeoutException) as exception: | ||
| if retries.config.retry_connection_errors: | ||
| raise | ||
|
|
@@ -252,6 +277,12 @@ async def do_request() -> httpx.Response: | |
|
|
||
| if res.status_code == parsed_code: | ||
| raise TemporaryError(res) | ||
|
|
||
| if res.status_code == 400: | ||
| if not res.is_closed: | ||
| await res.aread() | ||
| if _wraps_transient_inner_code(res): | ||
| raise TemporaryError(res) | ||
| except (httpx.NetworkError, httpx.TimeoutException) as exception: | ||
| if retries.config.retry_connection_errors: | ||
| raise | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| import asyncio | ||
| import unittest | ||
|
|
||
| import httpx | ||
|
|
||
| from openrouter.utils.retries import ( | ||
| BackoffStrategy, | ||
| RetryConfig, | ||
| Retries, | ||
| _wraps_transient_inner_code, | ||
| retry, | ||
| retry_async, | ||
| ) | ||
|
|
||
|
|
||
| def response(status_code, json_body=None): | ||
| request = httpx.Request("POST", "https://openrouter.ai/api/v1/chat/completions") | ||
| if json_body is None: | ||
| return httpx.Response(status_code, request=request) | ||
| return httpx.Response(status_code, json=json_body, request=request) | ||
|
|
||
|
|
||
| def fast_retries(): | ||
| # Jitter-free, 1ms intervals. Budget is generous so these tests terminate on | ||
| # success / non-retryable status rather than on the elapsed-time timeout, which | ||
| # keeps `calls` counts deterministic regardless of CI scheduling overhead. | ||
| backoff = BackoffStrategy( | ||
| initial_interval=1, max_interval=1, exponent=1.0, max_elapsed_time=10000, jitter_ms=0 | ||
| ) | ||
| return Retries(RetryConfig("backoff", backoff, retry_connection_errors=True), ["5XX"]) | ||
|
|
||
|
|
||
| class WrapsTransientInnerCodeTests(unittest.TestCase): | ||
| def test_400_with_transient_inner_code(self): | ||
| self.assertTrue(_wraps_transient_inner_code(response(400, {"error": {"code": 502}}))) | ||
|
|
||
| def test_400_with_string_inner_code(self): | ||
| self.assertTrue(_wraps_transient_inner_code(response(400, {"error": {"code": "529"}}))) | ||
|
|
||
| def test_400_invalid_request_inner_400_not_retried(self): | ||
| self.assertFalse(_wraps_transient_inner_code(response(400, {"error": {"code": 400}}))) | ||
|
|
||
| def test_400_without_inner_code(self): | ||
| self.assertFalse(_wraps_transient_inner_code(response(400, {"error": {"message": "bad"}}))) | ||
|
|
||
| def test_400_non_json_body(self): | ||
| self.assertFalse(_wraps_transient_inner_code(response(400))) | ||
|
|
||
|
|
||
| class RetryIntegrationTests(unittest.TestCase): | ||
| def test_retries_400_wrapping_transient_code_then_succeeds(self): | ||
| calls = 0 | ||
|
|
||
| def func(): | ||
| nonlocal calls | ||
| calls += 1 | ||
| if calls < 3: | ||
| return response(400, {"error": {"message": "Provider returned error", "code": 502}}) | ||
| return response(200, {"ok": True}) | ||
|
|
||
| result = retry(func, fast_retries()) | ||
| self.assertEqual(result.status_code, 200) | ||
| self.assertEqual(calls, 3) # retried twice, then succeeded | ||
|
|
||
| def test_does_not_retry_plain_400(self): | ||
| calls = 0 | ||
|
|
||
| def func(): | ||
| nonlocal calls | ||
| calls += 1 | ||
| return response(400, {"error": {"message": "logprobs must be 0-5", "code": 400}}) | ||
|
|
||
| result = retry(func, fast_retries()) | ||
| self.assertEqual(result.status_code, 400) | ||
| self.assertEqual(calls, 1) # not retried | ||
|
|
||
| def test_async_retries_400_wrapping_transient_code_then_succeeds(self): | ||
| calls = 0 | ||
|
|
||
| async def func(): | ||
| nonlocal calls | ||
| calls += 1 | ||
| if calls < 3: | ||
| return response(400, {"error": {"code": 503}}) | ||
| return response(200, {"ok": True}) | ||
|
|
||
| result = asyncio.run(retry_async(func, fast_retries())) | ||
| self.assertEqual(result.status_code, 200) | ||
| self.assertEqual(calls, 3) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deliberate, not an oversight, see the "Heads-up for maintainers" in the PR body. No overlay/hook can do body-based retries here (retry config is HTTP-status only; hooks run outside the retry loop), so persisting it across regen is a maintainer call: genignore or Speakeasy template support. -- Claude