fix: wait for cooldown expiry instead of failing when all profiles are rate-limited#24157
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (const candidate of candidates) { | ||
| const profileIds = resolveAuthProfileOrder({ | ||
| cfg: params.cfg, | ||
| store: authStore, | ||
| provider: candidate.provider, | ||
| }); | ||
| if (!profileIds.some((id) => !isProfileInCooldown(authStore, id))) { | ||
| continue; | ||
| } | ||
| try { | ||
| return { | ||
| result: await params.run(candidate.provider, candidate.model), | ||
| provider: candidate.provider, | ||
| model: candidate.model, | ||
| attempts, | ||
| }; | ||
| } catch (retryErr) { | ||
| if (shouldRethrowAbort(retryErr)) { | ||
| throw retryErr; | ||
| } | ||
| lastError = retryErr; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Retry loop breaks on first error, skipping remaining candidates
The retry for loop iterates over all candidates but breaks on the first failure (line 474). This means if the first available candidate fails on retry, other candidates that may have recovered from cooldown are never tried. This is arguably intentional ("retry once"), but the loop structure suggests a full pass over candidates. If only one retry attempt is intended, consider adding a comment explaining this is by design, or replacing the loop with a targeted single-candidate retry to make the intent clearer.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/model-fallback.ts
Line: 453-476
Comment:
**Retry loop breaks on first error, skipping remaining candidates**
The retry `for` loop iterates over all candidates but `break`s on the first failure (line 474). This means if the first available candidate fails on retry, other candidates that may have recovered from cooldown are never tried. This is arguably intentional ("retry once"), but the loop structure suggests a full pass over candidates. If only one retry attempt is intended, consider adding a comment explaining this is by design, or replacing the loop with a targeted single-candidate retry to make the intent clearer.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
The "does NOT probe non-primary candidates during cooldown" test sets The fix applied to the "single candidate exhausts candidates" test (changing to Prompt To Fix With AIThis is a comment left during a code review.
Path: src/agents/model-fallback.probe.test.ts
Line: 183-184
Comment:
**Test will block for ~30s on real timers**
The "does NOT probe non-primary candidates during cooldown" test sets `almostExpired = NOW + 30 * 1000` (30s), which is under the new 90s `MAX_COOLDOWN_WAIT_MS` threshold. After the primary probe fails with 429 and the other candidates are skipped as cooldown, `allCooldownSkips` evaluates to `true` and the wait-and-retry path triggers a real `setTimeout(resolve, 30500)`. Since the test doesn't use `vi.useFakeTimers()`, this will block for ~30 real seconds before the retry loop skips all candidates (because `isProfileInCooldown` still returns `true`) and finally throws.
The fix applied to the "single candidate exhausts candidates" test (changing to `NOW + 120 * 1000`) should also be applied here, or the test should use fake timers to advance past the wait.
How can I resolve this? If you propose a fix, please make it concise. |
|
I agree with greptile here. But in general, I do like the idea. We have no "retry" button, so user-facing aborts are always annoying. 90 seconds can feel like an eternity, though. Maybe make this configurable? |
|
Thanks for the reviews @greptile-apps and @HenryLoenwind! Addressed both points in 8bb0591:
|
|
Looking at your updates in 8bb0591, both changes look good! The test fix and configurable wait are implemented correctly. A few observations: Test fix ✅ Config implementation ✅
One minor suggestion: In This makes it immediately obvious how long the actual wait will be vs the configured max. Otherwise, the implementation is solid and ready. The config gives users an escape hatch (set to 0 to disable) while keeping a sensible default that prevents most transient rate-limit errors from surfacing. |
|
This pull request has been automatically marked as stale due to inactivity. |
|
Still relevant and working as intended — this fix prevents transient rate limit windows from surfacing as hard errors when a short wait would resolve them. The 90s configurable ceiling keeps it from hanging indefinitely. Happy to address any remaining review feedback or rebase if needed. |
|
This pull request has been automatically marked as stale due to inactivity. |
openclaw-barnacle
Bot
added
stale
…e rate-limited When all auth profiles for every model candidate are in cooldown, runWithModelFallback now waits for the soonest cooldown to expire (up to 90 seconds) and retries once, rather than immediately throwing "All models failed". This prevents transient rate limits from surfacing as hard user-facing errors when the recovery window is short. If the wait would exceed 90s or the retry also fails, the original error is thrown as before. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Address PR review feedback: - Add auth.cooldowns.rateLimitWaitSeconds config option (default: 90, 0 to disable) - Fix second test that had 30s cooldown within the 90s wait window (now 120s) - Add type, zod schema, help text, and label for the new config option Co-Authored-By: Claude Opus 4.6 <[email protected]>
8bb0591 to
a380831
Compare
|
Rebased onto current main — conflicts resolved and all prior review feedback is still addressed. Quick recap of what this PR does:
This prevents transient rate limits from surfacing as hard user-facing errors when a short wait would resolve them. Ready for another look! |
The previous implementation capped the cooldown wait at 90 seconds, which meant that longer but still reasonable rate-limit windows would fail immediately even though the system knew exactly when the cooldown expires. Now the default behavior waits for the actual cooldown expiry with no cap, matching how Claude Code handles rate limits. Users can still set auth.cooldowns.rateLimitWaitSeconds to a custom cap or 0 to disable. Adds user-visible log.warn messages during the wait and on retry. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Pushed an additional improvement in 26d3693: Changed the default from a 90s cap to waiting for the actual cooldown expiry. The system already tracks exact reset timestamps for every profile, so there's no reason to impose an arbitrary ceiling — if the cooldown resets in 5 minutes, we should wait 5 minutes instead of throwing an error the user can't recover from. This matches how Claude Code handles rate limits: it knows when the limit resets and just waits. The config still gives users full control:
Also added
All 20 probe tests pass. |
|
This pull request has been automatically marked as stale due to inactivity. |
|
Closing due to inactivity. |
2 participants
Summary
runWithModelFallbacknow waits for the soonest cooldown to expire (up to 90 seconds) and retries once, rather than immediately throwing "All models failed"Fixes #24158
Motivation
When using Claude Max subscription tokens via OAuth, hitting rate limits causes all auth profiles to enter cooldown simultaneously. With the current code, this immediately fails the request even if the cooldown is only seconds away from expiring. This is especially disruptive for messaging channel users (Discord, Telegram) who see "All models failed" errors for what is a brief, transient rate limit.
Changes
src/agents/model-fallback.ts: After exhausting all candidates, detect if all failures are cooldown-related. If the soonest cooldown expires within 90s, sleep until it expires, clear expired cooldowns, and retry once.src/agents/model-fallback.probe.test.ts: AddedclearExpiredCooldownsto the auth-profiles mock. Adjusted the "single candidate exhausts candidates" test to use a cooldown beyond the 90s wait threshold so it continues to test the immediate-failure path.Test plan
pnpm tsgo)🤖 Generated with Claude Code
Greptile Summary
This PR adds a cooldown-wait-retry mechanism to
runWithModelFallbackso that when all model candidates are rate-limited, the function waits for the soonest cooldown to expire (up to 90s) and retries once, instead of immediately failing. This is a targeted improvement for Claude Max / OAuth users who hit rate limits across all profiles simultaneously.setTimeout, causing the test to block for ~30 real secondsConfidence Score: 3/5
Last reviewed commit: 0f58ff4
(2/5) Greptile learns from your feedback when you react with thumbs up/down!