Repository navigation
fix: auto-decay backoffLevel when rate limit window has passed - #657
diegosouzapw merged 1 commit into
Conversation
High backoffLevel (up to 15) persisted permanently in the DB after a burst of 429s. The account health score dropped to zero (100 - 15*10 = -50), causing the account selector to never pick the account again. Only a successful request could reset backoffLevel via clearAccountError, but the account was never selected — creating a deadlock. Now, during account selection, any non-terminal connection whose rateLimitedUntil has passed gets its backoffLevel reset to 0 and testStatus restored to active. The DB update is fire-and-forget to avoid blocking the hot path. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Code Review
This pull request introduces an auto-decay mechanism for account backoff levels in src/sse/services/auth.ts. When a rate limit window expires, the code now resets the backoff level and clears error-related fields to ensure accounts can be re-selected for requests. Feedback suggests refactoring the update logic to avoid duplication with existing functions, explicitly clearing the rateLimitedUntil field, and implementing error logging for the background update operation instead of swallowing exceptions.
| updateProviderConnection(c.id, { | ||
| backoffLevel: 0, | ||
| testStatus: "active", | ||
| lastError: null, | ||
| lastErrorAt: null, | ||
| lastErrorType: null, | ||
| lastErrorSource: null, | ||
| errorCode: null, | ||
| }).catch(() => {}); |
There was a problem hiding this comment.
This update object is very similar to the one used in clearAccountError (lines 859-868), creating code duplication. This could become a maintenance concern if new error-related fields are added in the future. It would be beneficial to refactor this to a shared helper or constant.
I have a couple of suggestions for this block:
- For consistency with
clearAccountError, consider addingrateLimitedUntil: nullto the update payload, since the rate limit has passed. - Swallowing errors with an empty
.catch(() => {})can hide underlying problems. While this is a fire-and-forget operation, logging the error would be valuable for debugging without blocking the request.
updateProviderConnection(c.id, {
backoffLevel: 0,
testStatus: "active",
lastError: null,
lastErrorAt: null,
lastErrorType: null,
lastErrorSource: null,
errorCode: null,
rateLimitedUntil: null,
}).catch((err) =>
log.warn("AUTH", `Failed to auto-decay backoffLevel for ${c.id.slice(0, 8)}`, err)
);
Code Review SummaryStatus: Issues Addressed | Recommendation: Address inline feedback before merge OverviewAn existing review from gemini-code-assist[bot] has already identified the key issues with this PR. The inline comment on line 390 is comprehensive and covers:
RecommendationAddress the suggestions in the existing inline comment before merging:
Files Reviewed (1 file)
Additional Notes
|
|
Thanks @brendandebeasi for this excellent bug fix! 🎉 The backoff deadlock was a real issue — accounts would get permanently deprioritized after a rate limit even when the window expired. Your auto-decay logic is clean and well-placed. This will be part of v3.1.5. We really appreciate this contribution! 🚀 |
Changes: - fix: auto-decay backoffLevel when rate limit window expires (PR #657 by @brendandebeasi) - i18n: comprehensive Chinese translation rewrite (PR #658 by @only4copilot) - docs: update CHANGELOG and OpenAPI spec
…off-level-auto-decay Merged! Great catch on the backoff deadlock.
Changes: - fix: auto-decay backoffLevel when rate limit window expires (PR cheaper-inference#657 by @brendandebeasi) - i18n: comprehensive Chinese translation rewrite (PR cheaper-inference#658 by @only4copilot) - docs: update CHANGELOG and OpenAPI spec
…off-level-auto-decay Merged! Great catch on the backoff deadlock.
Changes: - fix: auto-decay backoffLevel when rate limit window expires (PR cheaper-inference#657 by @brendandebeasi) - i18n: comprehensive Chinese translation rewrite (PR cheaper-inference#658 by @only4copilot) - docs: update CHANGELOG and OpenAPI spec
Summary
backoffLevel(up to 15) after a burst of 429s were permanently deprioritized by the account selector, even after the rate limit window expiredbackoffLevelto 0 for any non-terminal connection whoserateLimitedUntilhas passedProblem
When a provider returns repeated 429s,
backoffLevelincrements up tomaxLevel: 15andtestStatusis set tounavailable. OncerateLimitedUntilexpires (max 2 min), the account becomes eligible for selection again — butbackoffLevel: 15persists permanently in the DB.The account health score is calculated as
100 - (backoffLevel * 10), so a level-15 account scores -50. The account selector always picks healthier accounts first, meaning this account is never selected. The only way to resetbackoffLevelis viaclearAccountError, which requires a successful request through that account — creating a deadlock:This is what causes the "Quota exceeded" error in clients even when the upstream provider has plenty of quota available.
Solution
During account selection in
getProviderCredentials, before filtering available connections, check each connection:backoffLevel > 0AND not a terminal status ANDrateLimitedUntilhas passed (or is null)backoffLevel: 0,testStatus: "active", clear error fieldsThe DB update is fire-and-forget (
.catch(() => {})) to avoid blocking the hot path. The in-memorybackoffLevelis also set to 0 immediately so the current request benefits.Verification