feat: add InsufficientQuotaError for insufficient_quota 429 responses - #3507
feat: add InsufficientQuotaError for insufficient_quota 429 responses#3507pctablet505 wants to merge 4 commits into
Conversation
Adds InsufficientQuotaError as a subclass of RateLimitError and maps 429 responses with code='insufficient_quota' to it so callers can distinguish quota exhaustion from retryable rate limits. Fixes openai#1671
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e77b297e8f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if response.status_code == 429: | ||
| if is_mapping(data) and data.get("code") == "insufficient_quota": | ||
| return _exceptions.InsufficientQuotaError(err_msg, response=response, body=data) |
There was a problem hiding this comment.
With the default max_retries, _base_client calls _should_retry() and retries every 429 before _make_status_error() is reached, so this new branch only runs after the retry budget is exhausted; the async path has the same flow. For deterministic code="insufficient_quota" responses this still sends multiple doomed requests and delays surfacing the non-retryable quota error, so the quota code needs to be classified before retrying or handled in _should_retry.
Useful? React with 👍 / 👎.
_should_retry treated every 429 as retryable, so a deterministic insufficient_quota error still burned the whole retry budget before _make_status_error got a chance to raise InsufficientQuotaError, delaying the new error type behind unnecessary retries and backoff sleeps. Check the error code in _should_retry and skip retrying when it's insufficient_quota, mirroring the extraction already done in _make_status_error.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94462847c6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # quota; retrying will not help, so don't burn the retry budget | ||
| # on a request that is guaranteed to fail again. | ||
| try: | ||
| body = response.json() |
There was a problem hiding this comment.
Fresh evidence after the retry fix: for stream=True/.with_streaming_response calls, _base_client.request() sends the request with streaming enabled, so the 429 body has not been read when this line calls response.json(); httpx raises ResponseNotRead, the broad except turns the body into None, and the code falls through to retry. Streaming chat/responses requests that receive code="insufficient_quota" will therefore still burn max_retries before raising, unlike the non-streaming path covered by the new tests.
Useful? React with 👍 / 👎.
rkfshakti
left a comment
rkfshakti
left a comment
There was a problem hiding this comment.
Good addition. The insufficient_quota error code is a well-known OpenAI API response, and treating it as non-retryable is the correct behavior — retrying a quota-exceeded request just burns retry budget and delays the user getting the real error.
The implementation is clean:
InsufficientQuotaErrorextendsRateLimitError(same HTTP status, different semantics) — callers catchingRateLimitErrorwill still catch this, which is backward compatible.- The retry guard in
_should_retryparses the response body only when status is 429, so there's no performance overhead on normal responses. - Both sync and async
_make_status_errorpaths are covered.
One minor thing: the try/except Exception around response.json() could be narrowed to json.JSONDecodeError to avoid swallowing unrelated errors. But since this is in a retry-decision path where the consequence of a false negative is just an extra retry, the broad catch is acceptable.
LGTM.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Two blockers:
-
The non-retry behavior does not work for streamed responses.
_should_retry()callsresponse.json()before the error response has been consumed; forstream=Trueand.with_streaming_response, httpx raisesResponseNotRead, the broadexceptturns that intobody = None, and the 429 falls through to the normal retry path. I reproduced this with a streaming 429 body andmax_retries=3: it made 4 requests before finally raisingInsufficientQuotaError. Please make body-based retry classification work after the error body is available in both sync and async paths, and add streaming regressions for both APIs. -
The repository lint gate is currently red.
./scripts/lintreports unsorted import blocks insrc/openai/__init__.pyandtests/test_client.py;ruff format --checkalso reports thattests/test_client.pyneeds formatting. Please run the repo formatter/linter and commit the result.
_should_retry() calls response.json() to check for insufficient_quota, but for stream=True requests and .with_streaming_response the body is not read automatically. httpx raises ResponseNotRead in that case, which the broad except in _should_retry swallowed, silently falling back to the normal retry path and burning the whole retry budget on a 429 that could never succeed. Read the response body in both the sync and async request loops before calling _should_retry, so the insufficient_quota check works the same way for streaming and non-streaming responses. Also run the repo's ruff import sort and formatter to fix the unsorted import blocks in __init__.py and test_client.py.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d683d77fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # `stream=True` requests the body is not read automatically and accessing | ||
| # it before this point raises `httpx.ResponseNotRead`. | ||
| if not err.response.is_closed: | ||
| err.response.read() |
There was a problem hiding this comment.
For stream=True/.with_streaming_response calls, a retriable 429 that is not insufficient_quota or any 5xx can have its status headers available while the error body stalls or fails; this unconditional read() then raises (for example httpx.ReadTimeout) before _should_retry() runs, so the default retry path is skipped and a raw transport error leaks. Since only the quota classification needs the body, gate or catch this read so other retriable streamed status errors can still retry based on status/headers; the async path has the same issue.
Useful? React with 👍 / 👎.
3 participants
Fixes #1671
Adds
InsufficientQuotaError, a subclass ofRateLimitError, and maps 429 responses withcode="insufficient_quota"to it so callers can distinguish quota exhaustion from retryable rate limits.