Skip to content

fix(llm): retry rate-limited (429) batches instead of dropping them - #403

Closed
keshprad wants to merge 18 commits into
mainfrom
keshavp/codex/llm-429-retries
Closed

keshprad wants to merge 18 commits into
mainfrom
keshavp/codex/llm-429-retries

Conversation

@keshprad

Copy link
Copy Markdown
Contributor

Summary

  • retry provider rate-limit failures in both synchronous and asynchronous LLM batch loops with a bounded 5s/15s/30s/60s/60s schedule
  • honor numeric Retry-After values up to 120 seconds, including for models with native provider retries
  • record exhausted rate-limit retries as llm_rate_limit_retries_exhausted in the inspection ledger

Validation

  • make lint — passed
  • make format-check — passed
  • pytest -q tests/nodes/test_llm_analyzer_base.py tests/test_inspection_ledger.py — 171 passed
  • unit suite — 2213 passed, 13 skipped, 4 xfailed; seven SSRF hostname tests fail because GitHub/GitLab resolve to private/internal addresses in this environment. The exact same seven tests fail on the untouched 29b0dc8 base.

Attribution

Applied from the attached signed-off patch while preserving Devin AI as the commit author and the original Signed-off-by/Co-Authored-By trailers.

devin-ai-integration Bot and others added 18 commits August 20, 2026 00:40
Add a bounded rate-limit retry policy (5s, 15s, 30s, 60s, 60s, honoring a numeric Retry-After up to 120s) to both the sync and async LLM batch retry loops, applied even for models with native provider retries. Exhausted retries record LLM_RATE_LIMIT_RETRIES_EXHAUSTED in the inspection ledger.

Signed-off-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Keshav Pradeep <keshavp@nvidia.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Requesting changes because supported provider exception shapes still bypass the intended rate-limit behavior. Anthropic retries but ignores the server's Retry-After, while Bedrock throttles are not classified as retryable and the batch is dropped immediately.

the OpenAI-compatible endpoints reached through ``ChatOpenAI``); raw HTTP
clients surface it as a 429 status error instead.
"""
if type(exc).__name__ == "RateLimitError":

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P1] Handle the concrete rate-limit shapes from every supported provider. An actual anthropic.RateLimitError passes this class-name check, but _rate_limit_retry_after_seconds rejects it because it is not OpenAI's APIStatusError; Retry-After: 42 therefore becomes the fixed 5-second delay. Bedrock surfaces exhausted throttles as botocore.exceptions.ClientError with Error.Code=ThrottlingException and HTTP 429, which returns false here and drops the batch. Add provider-neutral status/header extraction (including botocore response metadata/code) and tests built from the real SDK exception types.

@mohgupta-ship-it

mohgupta-ship-it commented Aug 25, 2026 •

Copy link
Copy Markdown
Member

Powered by Codex: PR council review result.

This is a triage signal, not a maintainer approval.

  • Rating: critical fix
  • Confidence: high
  • Status read: Draft, changes requested, no checks, conflicting
  • Review method: fresh GitHub metadata/body/files/reviews/checks plus selected diffs; council lenses were spec fit, dead-code/reachability, YAGNI/scope, design/coupling, and code standards/tests.
  • Council assessment: Rate-limit retry fix is high-value, but supported provider exception shapes still bypass intended behavior; Anthropic Retry-After and Bedrock throttling remain suspect.
  • Recommended action: Do not merge; add provider-specific 429/Retry-After tests, resolve conflicts, and run CI.

@rng1995
rng1995 marked this pull request as ready for review August 26, 2026 19:04

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SkillSpector Review]

Changes are still required at ac93c9f. The retry classifier only recognizes an exception named RateLimitError or OpenAI/httpx status errors. As the existing unresolved inline thread documents, an Anthropic rate-limit exception is retried but its Retry-After header is ignored, while Bedrock's botocore.exceptions.ClientError throttling shape is not classified as retryable and the batch is dropped. Add provider-neutral status/header extraction, including Bedrock throttling metadata, and regressions built from the real supported SDK exception types. Hosted checks pass, but the branch currently conflicts with main.

@rng1995

rng1995 commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

[SkillSpector Review]

Hi @keshprad, thank you for your contribution and for raising the problem of rate-limited (429) LLM batches being dropped.

This is now covered on main by #555 ("fix(llm): retry transient provider failures", merged 2026-09-21). It adds a bounded, provider-neutral retry policy that treats HTTP 408/409/425/429, RateLimitError and Bedrock ThrottlingException as retryable (see _RETRYABLE_PROVIDER_STATUS_CODES in llm_analyzer_base.py), which addresses the open review items on this PR. Since this PR now conflicts with that implementation, I'm closing it as superseded.

If we've missed something — for example, a case that still reproduces on current main — please comment here or open an issue with a minimal repro and we'll gladly reopen. Thanks again for helping make SkillSpector better!

@rng1995 rng1995 closed this Sep 28, 2026
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.

3 participants