-
Notifications
You must be signed in to change notification settings - Fork 1
fix(gateway): honor Retry-After and wait out a 429 rate-limit storm #1179
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
f29f529
fix(gateway): honor Retry-After and wait out a 429 rate-limit storm
seonghobae a6a8a0a
fix(gateway): extend rate-limit-aware admission to route_once/conduct
seonghobae 2493d81
fix(gateway): assume a cooldown when a 429 states none at all
seonghobae 529c5f6
fix(gateway): admit a rate-limit wait by selector kind, not candidate…
seonghobae 5e81e67
Merge origin/main into feat/rate-limit-aware-admission
seonghobae 5b2857b
test(gateway): stabilize HTTP storm wait with injectable sleep seam
seonghobae a6ff937
Merge remote-tracking branch 'origin/main' into feat/rate-limit-aware…
seonghobae File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| Under a provider 429 storm (org CI review lanes hit this on `orchestrator/free`: | ||
| noema run 34758641142, strix run 34758679736 -- every candidate returned 429 | ||
| within ~50ms; see `ContextualWisdomLab/.github#2148`, `#2165`), the gateway no | ||
| longer fails the request immediately. It now parses `Retry-After` (delta-seconds | ||
| or an HTTP-date) and, when absent, a numeric `x-ratelimit-reset*` header, tracks | ||
| a per-agent quota cooldown separate from the health circuit breaker (a 429 is | ||
| quota exhaustion, not a model health failure, and no longer trips the breaker), | ||
| skips a currently cooled-down candidate by default in `_failover_candidates` | ||
| (shared by every caller), and waits out the earliest cooldown -- one shared | ||
| `_await_rate_limit_recovery` implementation -- when it fits the request's | ||
| administrator-owned model deadline (issue #1053) or the new | ||
| `rate_limit_wait_seconds` caller-contract default (30s), retrying once the | ||
| wait elapses. Both real request paths reach it: `proxy_completion`'s | ||
| passthrough failover loop, and `route_once`/`conduct` (every step, including | ||
| the worker step) via `_invoke_with_rate_limit_recovery`, which is what | ||
| `orchestrator/free` actually runs over `/v1/chat/completions`. When waiting is | ||
| impossible, the gateway now returns an honest `429` with error code | ||
| `provider_rate_limited` and a `Retry-After` header (or the equivalent terminal | ||
| SSE error frame when streaming) instead of misclassifying quota exhaustion as | ||
| a `502` connection failure. `provider_readiness_report` | ||
| (`/api/v1/provider_readiness/latest`) now also reports `rate_limited_until` and | ||
| `earliest_ready_seconds` so an external preflight/readiness sidecar can wait | ||
| instead of exiting. | ||
|
|
||
| A 429 that states no cooldown at all (RFC 9110 permits omitting | ||
| `Retry-After`/`x-ratelimit-reset*`, and NIM/OpenRouter routinely do) now | ||
| records an assumed cooldown -- the new `rate_limit_unknown_cooldown_seconds` | ||
| default (5s) -- instead of nothing, so an all-omitted-header storm can no | ||
| longer look identical to "nothing is rate-limited" and fail as if this | ||
| feature did not exist; every cooldown surface labels itself | ||
| `cooldown_source: "provider"` or `"assumed"` accordingly, and a provider-stated | ||
| cooldown is never shortened or relabeled by a later assumed one. This | ||
| assumption applies to 429 only (a 503 with no header keeps requiring a real | ||
| provider-stated duration) -- scoped narrowly after concrete pre-existing-test | ||
| regression evidence, not by design intent alone. | ||
|
|
||
| The wait admission decision no longer turns on candidate count. An earlier | ||
| version of this guard returned immediately whenever fewer than two | ||
| candidates were eligible, which misclassified a virtual selector's pool | ||
| wiped down to exactly one eligible candidate by a 429 -- a real production | ||
| shape (noema-review run 34772771262 on `contextual-orchestrator#1177`, | ||
| preflight `ready_count: 1`, failing after 562s; `ContextualWisdomLab/.github#2148` | ||
| documents a three-route OpenRouter `:free` ZDR pool that a single 429 can | ||
| wipe to one route) -- identically to a genuinely pinned concrete model, and | ||
| failed the request immediately instead of waiting. The discriminator is now | ||
| whether the caller delegated model selection at all: | ||
| `_await_rate_limit_recovery` and `_invoke_with_rate_limit_recovery` take a | ||
| `virtual_selector` flag (computed once by each caller from the same | ||
| `GATEWAY_DEFAULT_MODEL`/`AUTO_MODEL`/`FREE_MODEL` constants used elsewhere in | ||
| the file); a virtual selector waits out a storm even with a single eligible | ||
| candidate, while an explicit concrete model id keeps failing fast | ||
| unconditionally, regardless of how many failover candidates exist -- | ||
| preserving the `tests/test_provider_error_taxonomy.py` single-candidate, | ||
| no-header 429 contract that must never wait. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
503관련 도움말을 수정하세요.PR 계약상 헤더가 없는
503응답에는 assumed cooldown을 적용하지 않습니다. 그러나 이 도움말은--rate-limit-unknown-cooldown-seconds가 헤더 없는429/503모두에 적용된다고 설명합니다. 운영자가503대기 동작을 잘못 구성할 수 있습니다.429만 명시하도록 문구를 변경하세요.수정 예시
PR objective에 따르면 헤더 없는
503에는 가정 cooldown을 적용하지 않습니다.📝 Committable suggestion
🤖 Prompt for AI Agents