Skip to content

Retry request timeouts and rate limits (408/429)#381

Open
wassname wants to merge 6 commits into
OpenRouterTeam:mainfrom
wassname:fix/chat-transient-retries
Open

Retry request timeouts and rate limits (408/429)#381
wassname wants to merge 6 commits into
OpenRouterTeam:mainfrom
wassname:fix/chat-transient-retries

Conversation

@wassname

@wassname wassname commented Jun 30, 2026

Copy link
Copy Markdown

What

Retry HTTP 408 (request timeout) and 429 (rate limit), alongside the existing 5XX.

How

A new overlay (.speakeasy/overlays/retry-transient-status-codes.overlay.yaml) registered in workflow.yaml. No generated code is edited, so it survives regeneration (per CONTRIBUTING). Net diff is two files.

Verification

speakeasy overlay apply on the source spec yields statusCodes: [5XX, 408, 429] (no duplicates).

History (for reviewers)

Earlier commits on this branch edited the generated utils/retries.py (and tried a .genignore workaround) to also retry a 400 whose JSON error.code is a transient upstream code. That was reverted: it has no clean surface in the generated SDK, and is handled client-side instead. This PR is now only the overlay. Perry's earlier review comments about the "DO NOT EDIT" banner refer to that reverted approach.

Copilot AI review requested due to automatic review settings June 30, 2026 06:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors retry response classification into centralized helpers and expands what the SDK considers “transient” so chat completion requests can retry in additional real-world failure modes (notably OpenRouter’s occasional HTTP 400 wrapper around upstream 5xx provider errors).

Changes:

  • Centralized retryability checks into _is_retryable_response() / _is_retryable_response_async() and reused them in both sync and async retry loops.
  • Added default retryability for HTTP 408 (timeout) and HTTP 429 (rate limit), independent of configured status code patterns.
  • Added narrow handling for HTTP 400 responses that carry a transient upstream/provider error code inside the JSON error body, with new unit tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/openrouter/utils/retries.py Centralizes retry response classification and adds transient 408/429 + 400-with-transient-inner-code handling for sync/async retry.
tests/test_retries.py Adds unit tests covering retryability for 408/429, configured 5XX patterns, and the new 400 inner error-code behavior (sync + async).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/openrouter/utils/retries.py Outdated
@wassname

Copy link
Copy Markdown
Author

Downstream examples that motivated looking inside OpenRouter 400 bodies:

{"error":{"message":"Provider returned error","code":400,"metadata":{"raw":"{\"error\":{\"object\":\"error\",\"type\":\"invalid_request_error\",\"message\":\"logprobs must be between 0 and 5: 20\"}}","provider_name":"Fireworks"}}}
{"error":{"message":"Provider returned error","code":400,"metadata":{"raw":"{\"code\":\"Client specified an invalid argument\",\"error\":\"top_logprobs must be less equal than 8 but top_logprobs = 20\"}","provider_name":"xAI"}}}

These particular examples are non-retryable, and the added tests keep inner code: 400 non-retryable. The useful bit for this PR is that OpenRouter can expose provider failures inside a 400 envelope, so the retry classifier should inspect the structured inner error code instead of using only the HTTP status.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
perry-the-pr-reviewer[bot]

This comment was marked as outdated.

perry-the-pr-reviewer[bot]

This comment was marked as outdated.

@wassname

wassname commented Jul 5, 2026

Copy link
Copy Markdown
Author

note these are ported from my repo I've been using for months https://github.com/wassname/openrouter_wrapper. I'll fix that speakeasy thing now...

The retry response classification (408/429/5XX + transient inner error.code
on 400s) has no overlay or hook surface in Speakeasy, so retries.py must be
hand-maintained. It is tracked in gen.lock, so without a genignore entry the
next `speakeasy generate` overwrites this PR's changes. pylintrc is already
genignored the same way as precedent.

Co-Authored-By: Claudypoo <288921227+claudypoo@users.noreply.github.com>
@cursor

cursor Bot commented Jul 5, 2026

Copy link
Copy Markdown

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.

@wassname

wassname commented Jul 5, 2026

Copy link
Copy Markdown
Author

BLUF: reworked so it no longer edits generated code.

This PR now just adds 408 and 429 to the retry status codes via an overlay, so it survives regeneration (per CONTRIBUTING, generated files aren't hand-edited). Verified offline with speakeasy overlay apply: statusCodes becomes [5XX, 408, 429].

Dropped from here: retrying a 400 whose JSON error.code is a transient upstream code. The SDK can't express that (retry config is HTTP-status only, and hooks run outside the retry loop), so I handle it client-side instead.

-- Claude

perry-the-pr-reviewer[bot]

This comment was marked as outdated.

…retries

# Conflicts:
#	.genignore
#	src/openrouter/utils/retries.py
@cursor

cursor Bot commented Jul 5, 2026

Copy link
Copy Markdown

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.

perry-the-pr-reviewer[bot]

This comment was marked as outdated.

…rated code

Reverts the hand-edits to the generated utils/retries.py, its .genignore entry, and
the test. Adds an overlay so 408/429 join 5XX in the retry config, regenerating cleanly
per CONTRIBUTING (generated code is not edited directly). Verified with
`speakeasy overlay apply`: statusCodes -> [5XX, 408, 429].

The 400-with-transient-inner-error.code case is not portable to the SDK (status-only
retry config, hooks run outside the retry loop) and is handled client-side instead.

Co-Authored-By: Claudypoo <288921227+claudypoo@users.noreply.github.com>
@cursor

cursor Bot commented Jul 5, 2026

Copy link
Copy Markdown

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.

Completes the prior commit: 408/429 join 5XX via x-speakeasy-retries overlay.
Verified offline with `speakeasy overlay apply`: statusCodes -> [5XX, 408, 429].

Co-Authored-By: Claudypoo <288921227+claudypoo@users.noreply.github.com>
@cursor

cursor Bot commented Jul 5, 2026

Copy link
Copy Markdown

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.

@wassname

wassname commented Jul 5, 2026

Copy link
Copy Markdown
Author

Ready to review.
This adds retries for request timeouts and rate limits (408/429), on top of the existing server-error (5XX) retries. It changes config, not the auto-generated code, so it survives when the SDK is rebuilt. Checked locally it produces the right retry list.
The trickier case, retrying a 400 that wraps a transient upstream error code, is coming as a separate PR.
-- Claude

@wassname wassname changed the title Retry transient chat completion errors Retry request timeouts and rate limits (408/429) Jul 5, 2026

@perry-the-pr-reviewer perry-the-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perry's Review

Retries 408/429 via a new Speakeasy overlay instead of hand-editing generated code, replacing the earlier approach this PR previously took.

Verdict: 💬 Comments / questions

Details

Risk: 🟢 Low

CI: no checks reported on this branch — nothing to validate against

Findings (see inline comment for full context):

  • 🟡 overlay file:6 — the array-append behavior that keeps 5XX retryable alongside the new 408/429 codes is unverified by any CI step; it currently rests on manual verification noted in the PR description

Codex: skipped (trivial tier)

Research: skipped (trivial tier)

Security: no concerns — config-only change, no provider-key/streaming/logging/auth/tenant-isolation surface touched

Test coverage: n/a — repo has no dedicated test suite for generated SDK output; the test file from the earlier reverted approach was correctly dropped along with the hand-edit

Unresolved threads: 2 of my own prior threads (about the reverted hand-edited generated retry module) are now moot — the code they targeted no longer exists in this diff. I could not reply into or resolve them: the maintainer app installation on this org lacks write access to pull request threads, so both mutations returned 403. They are safe to disregard/close manually.

Scope: first review (full)
Review: tier=trivial · model=claude-sonnet-latest · score=0.0

info:
title: Retry request-timeout and rate-limit responses
version: 0.0.0
actions:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] No CI step regenerates the spec or applies this overlay to confirm the update action appends 408/429 rather than replacing the existing statusCodes array.

Details

Why: The PR description states speakeasy overlay apply was run manually and produced statusCodes: [5XX, 408, 429], but this repo's CI has no check that regenerates the spec from this workflow. If the overlay's array semantics ever behaved as a wholesale replace instead of an append (or a future Speakeasy version changed that behavior), 5XX would silently drop out of the retry config with no signal — every other overlay in this directory only ever sets a scalar or object field, so there's no existing regression test covering array-update semantics either.

Fix: Add a lightweight CI step (or a local pytest-style check) that runs the overlay against the base spec and asserts the resulting statusCodes list contains 5XX, 408, and 429 — turning the author's one-time manual verification into a repeatable regression guard.

Ref: OpenAPI Overlay Specification — array update semantics

Prompt for agents
In .speakeasy/workflow.yaml and .speakeasy/overlays/retry-transient-status-codes.overlay.yaml, add a CI check (e.g. a new step in an existing GitHub Actions workflow, or a small script under a test/ or scripts/ directory) that runs `speakeasy overlay apply` (or the equivalent library call) against `.speakeasy/in.openapi.yaml` with this overlay and asserts the resulting `x-speakeasy-retries.statusCodes` array contains all of "5XX", "408", and "429". This turns the manual verification described in the PR body into an automated regression guard against future Speakeasy version changes to array-update semantics.

Reviewed at 4e39d32

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, no CI covers it. Verified locally with speakeasy overlay apply: statusCodes becomes [5XX, 408, 429] (append keeps 5XX). -- Claude

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants