fix(classifier): a 429 about money is billing, not a rate limit - #90503
fix(classifier): a 429 about money is billing, not a rate limit#90503rodrigogs wants to merge 1 commit into
Conversation
Related: open #36280 covers the same 429 billing/quota classification family with broader permanent-usage-limit handling. This PR is the focused existing-_BILLING_PATTERNS approach; maintainers should choose whether to consolidate the overlapping paths. |
Providers reuse HTTP 429 for a depleted balance. z.ai answers one with "1113 Insufficient balance, Please recharge", and a weekly-allowance overrun with a message naming a reset days out — both status 429, neither about rate. The 429 branch fell straight through to FailoverReason.rate_limit, which earns the 429 cooldown: one hour, or 60 seconds when it is the pool's only credential (EXHAUSTED_TTL_SOLE_CREDENTIAL_SECONDS). Every per-profile pool on the install that surfaced this holds exactly one entry, so a provider blocked until a top-up or a window rollover got benched for a minute and then hammered again. The wait is days; the bench was 60s. _BILLING_PATTERNS already carries "insufficient balance" and "credits have been exhausted" — it was simply never consulted for 429. It is checked at four places in this file, all of them other status codes. Placed after the overload and OpenRouter-upstream branches, deliberately. A busy endpoint and an aggregator throttling its own upstream are both 429s that say nothing about this account's balance, and both already have the right recovery above; billing must not steal either. 4 tests, next to the existing 429-overload sibling. Two pin the fix, two are guards: an ordinary throttle stays rate_limit and stays retryable, and overload still outranks a money reading when both phrases appear. Verified by mutation — reverting agent/error_classifier.py fails exactly the two billing tests and leaves the two guards green, so they are known to pin pre-existing behaviour. 101 passed in tests/agent/test_error_classifier.py.
33212b2 to
953b83a
Compare
|
Useful pointer — I looked at #36280 to answer the consolidation question concretely They overlap in one file and diverge in scope. Both touch
So the classifier halves are two approaches to the same defect, and #36280's Two coherent ways to land them, whichever the maintainers prefer:
I am happy to do the reduction either way — say which one you want and I will |
Problem
Providers reuse HTTP 429 for a depleted balance. z.ai answers one with
1113 Insufficient balance, Please recharge, and an exhausted weekly allowance with a message naming a reset days away — both status 429, neither about rate.The 429 branch fell straight through to
FailoverReason.rate_limit, which earns the 429 cooldown:EXHAUSTED_TTL_429_SECONDSEXHAUSTED_TTL_SOLE_CREDENTIAL_SECONDScredential_pool.pycaps the bench at 60s when the pool holds a single credential and the failure is not billing. On the install that surfaced this, every per-profile pool holds exactly one entry, so a provider that could not serve a request for days was benched for a minute and then hammered again, every minute, while the fallback chain rotated onto it in turn._BILLING_PATTERNSalready carries"insufficient balance"and"credits have been exhausted". It is consulted at four places in this file — none of them the 429 branch.Fix
Check
_BILLING_PATTERNSinside the 429 branch and classify asbilling(not retryable, rotate, fall back), which is the same shape the 402 and 404 paths already use for the same condition.Placed after the overload and OpenRouter-upstream checks, deliberately: a busy endpoint and an aggregator throttling its own upstream are both 429s that say nothing about this account's balance, and both already have the correct recovery above. Billing must not steal either.
Verification
4 tests, next to the existing
test_429_with_overloaded_body_is_overloaded_not_rate_limitsibling:test_429_with_insufficient_balance_is_billing_not_rate_limit— the z.ai 1113 shapetest_429_with_an_exhausted_plan_allowance_is_billingtest_429_without_a_money_body_is_still_rate_limit— guard against over-classifying; an ordinary throttle staysrate_limitand stays retryable, so the caller still backs off instead of benchingtest_429_overload_still_outranks_a_money_reading— ordering guard, with both phrases in one bodyMutation-checked: reverting
agent/error_classifier.pyand keeping the tests fails exactly the two billing tests and leaves the two guards green, so the guards are known to pin pre-existing behaviour rather than the new branch.pytest tests/agent/test_error_classifier.py→ 101 passed.ruffclean.Posting the local run because this repo does not run checks on PRs from forks.