Retry HTTP 400s that wrap a transient upstream error code#391
Conversation
OpenRouter is a passthrough for providers it does not control, and some transient provider failures (502/503/529/...) arrive as HTTP 400 with the real status in the JSON error.code. Retry only those; ordinary 400 invalid-request responses (including inner error.code == 400) stay non-retryable. Reuses the existing TemporaryError/backoff. Note: this edits generated code because retry classification here is HTTP-status only (x-speakeasy-retries) and SDK hooks run outside the retry loop, so a 400 body cannot be inspected via an overlay or hook. Preserving it across regen needs a genignore or upstream Speakeasy support -- flagging for maintainers. Co-Authored-By: Claudypoo <288921227+claudypoo@users.noreply.github.com>
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK’s retry behavior to treat certain HTTP 400 responses as transient when the response JSON body indicates an upstream transient failure via error.code (e.g. an outer 400 with inner 502/503), and adds tests validating the new classification and retry behavior.
Changes:
- Add
_wraps_transient_inner_code()and a transient inner-code allowlist to detect “400 wrapping upstream transient” responses. - Extend both
retry()andretry_async()backoff paths to retry HTTP 400 when the innererror.codeis transient. - Add unit + integration tests covering the classifier and confirming sync/async retry behavior for wrapped vs plain 400s.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/openrouter/utils/retries.py |
Adds helper + logic to retry outer-400 responses when JSON error.code is a transient upstream status. |
tests/test_retries.py |
Adds classifier tests and retry integration tests (sync + async) for wrapped-transient vs plain 400. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Code generated by Speakeasy (https://speakeasy.com). DO NOT EDIT.""" | ||
|
|
||
| import asyncio | ||
| import json |
There was a problem hiding this comment.
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
| backoff = BackoffStrategy( | ||
| initial_interval=1, max_interval=1, exponent=1.0, max_elapsed_time=20, jitter_ms=0 | ||
| ) |
There was a problem hiding this comment.
Fixed in 0a691a1: the tests now return a transient 400 twice then a 200, so the retry count is exact and not timing-dependent. -- Claude
Addresses Copilot review: the timing-based calls>1 assertion could be flaky on slow CI. Now the stub returns a transient 400 twice then a 200, so the retry count is exact regardless of scheduling. Co-Authored-By: Claudypoo <288921227+claudypoo@users.noreply.github.com>
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
Perry's Review
Retries an HTTP 400 when its JSON body wraps a transient upstream status in error.code, restoring recovery for OpenRouter's passthrough 400-wrapping behavior without loosening retries on genuine invalid-request 400s.
Verdict: 💬 Comments / questions
Details
Risk: 🟡 Medium — behavioral change to the shared retry path used by every SDK call, but blast radius is per-request/client-side and easily reverted via SDK version pin; well tested.
CI: none configured on this repo — informational only, not a gate.
Findings (see inline comment for full context):
- 🟡
retries.py:163— malformed/non-UTF8 400 body raises an uncaughtUnicodeDecodeErrorinstead of being classified non-retryable
Research: n/a (small tier — deep spec research skipped per tier routing)
Security: no concerns — no credentials, auth, or tenant-isolation code touched; diff-scoped secret grep clean
Test coverage: good — 8 new unit tests (classifier cases + sync/async retry-then-succeed integration) all pass locally against this branch; the one gap is the malformed-body edge case flagged inline
Unresolved threads: 2 existing threads from copilot-pull-request-reviewer (generated-file DO NOT EDIT concern, addressed in the PR body's "Heads-up for maintainers" section; flaky-timing concern, fixed in 0a691a1 per commit history verification) — both appear satisfactorily addressed by the author, left for the author/maintainers to resolve
Scope: first review (full)
Review: tier=small · model=claude-sonnet-latest · score=2.0
| 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.
[suggestion] retries.py:163 — malformed 400 body (invalid encoding) can raise UnicodeDecodeError instead of being treated as non-retryable
Details
Why: _wraps_transient_inner_code() only catches json.JSONDecodeError and httpx.ResponseNotRead around response.json(). But httpx.Response.json() calls json.loads(self.content, ...), and CPython's json.loads first attempts to auto-detect a UTF-16/UTF-32 BOM before falling back to UTF-8 — a garbled/non-UTF8 400 body (plausible on a passthrough middleman receiving a truncated or malformed upstream response) can trigger a UnicodeDecodeError there, which is not one of the two caught exception types. Verified locally against this branch:
>>> httpx.Response(400, content=b"\xff\xfe\x00\x01\x02", request=req).json()
UnicodeDecodeError: 'utf-16-le' codec can't decode byte 0x02 in position 4: truncated data
That exception escapes _wraps_transient_inner_code, propagates through retry()'s do_request(), gets wrapped in PermanentError, and retry_with_backoff re-raises exception.inner — so callers see a raw UnicodeDecodeError instead of the normal BadRequestResponseError (or a legitimate retry). This is a behavior regression versus pre-PR: previously every 400 fell straight through to the existing error-unmarshalling path, which reads .text (never raises) rather than .json().
Fix:
try:
body = response.json()
except (json.JSONDecodeError, UnicodeDecodeError, httpx.ResponseNotRead):
return FalseRef: json.loads encoding detection — bytes input is auto-detected for BOM/encoding before UTF-8 fallback, which is where the malformed input raises.
Prompt for agents
In src/openrouter/utils/retries.py:163, _wraps_transient_inner_code() catches (json.JSONDecodeError, httpx.ResponseNotRead) around response.json(), but a malformed/non-UTF8 400 body can make json.loads raise UnicodeDecodeError during its BOM/encoding sniffing before it ever attempts to parse JSON — that exception type is not caught. Widen the except tuple at retries.py:163 to also include UnicodeDecodeError (or replace both JSONDecodeError and UnicodeDecodeError with their common base ValueError), so a garbled 400 body is classified non-retryable instead of leaking a raw UnicodeDecodeError out of retry()/retry_async() to the caller. Add a regression test in tests/test_retries.py asserting _wraps_transient_inner_code(response(400, content=b"\xff\xfe\x00\x01\x02")) returns False without raising.
Reviewed at 0a691a1
What
Retry an HTTP 400 when its JSON body carries a transient upstream status in
error.code, e.g.{"error": {"message": "Provider returned error", "code": 502}}. Ordinary 400 invalid-request responses (including innererror.code == 400) stay non-retryable.Why
OpenRouter is a passthrough for upstream providers it doesn't control. Some transient provider failures (502/503/524/529/...) arrive as an HTTP 400 with the real status preserved in
error.code. Retrying all 400s would hide caller bugs; retrying only those with a transient inner code lets the SDK recover from provider blips without changing behavior for malformed requests. This wrapping is undocumented.How (minimal)
One branch added to each retry loop (sync + async), plus a small helper and a code set. Reuses the existing
TemporaryError+ backoff path. Net +31 lines inretries.py.Heads-up for maintainers
This edits generated code, which I know you avoid. There is no clean alternative surface:
x-speakeasy-retriesmatches HTTP status only, and SDK hooks (after_success/after_error) run outsideutils.retry, so they cannot inspect a 400 body and trigger a retry. To survive regeneration this needs either a.genignoreentry or upstream support in Speakeasy's retry template. Happy to adjust to whatever you prefer, or file it with Speakeasy.Tests
tests/test_retries.py(8 tests): the classifier per case, plus integration tests assertingretry()/retry_async()actually retry a 400-inner-502/503 and do not retry a plain 400.-- Claude (wassname's assistant)