fix(combo): fallback to next model on all-accounts-rate-limited 503 (… - #1523
Conversation
…1398) Integrated into release/v3.6.9
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logic to detect when all accounts for a provider are rate-limited by checking for 503 responses with specific error patterns, allowing the system to fall back to the next model in both priority and round-robin strategies. Feedback was provided to simplify the regex patterns by removing redundant entries and to improve the robustness of the content-type check using case-insensitive comparison.
| const ALL_ACCOUNTS_RATE_LIMITED_PATTERNS = [/unavailable/i, /service temporarily unavailable/i]; | ||
|
|
||
| function isAllAccountsRateLimitedResponse(status: number, contentType: string | null, errorText: string): boolean { | ||
| if (status !== 503) return false; | ||
| if (!contentType?.includes("application/json")) return false; | ||
| return ALL_ACCOUNTS_RATE_LIMITED_PATTERNS.some((p) => p.test(errorText)); |
There was a problem hiding this comment.
There are a couple of minor improvements for this utility function:
- Redundant Regex: In
ALL_ACCOUNTS_RATE_LIMITED_PATTERNS, the pattern/unavailable/iis a subset of/service temporarily unavailable/i. Any string matching the latter will already match the former, so the second pattern can be removed. - Case Sensitivity: While header names are case-insensitive, header values might not be normalized depending on the environment. It is safer to use
.toLowerCase()when checking thecontent-typevalue.
const ALL_ACCOUNTS_RATE_LIMITED_PATTERNS = [/unavailable/i];
function isAllAccountsRateLimitedResponse(status: number, contentType: string | null, errorText: string): boolean {
if (status !== 503) return false;
if (!contentType?.toLowerCase().includes("application/json")) return false;
return ALL_ACCOUNTS_RATE_LIMITED_PATTERNS.some((p) => p.test(errorText));
}| const ALL_ACCOUNTS_RATE_LIMITED_PATTERNS = [/unavailable/i, /service temporarily unavailable/i]; | |
| function isAllAccountsRateLimitedResponse(status: number, contentType: string | null, errorText: string): boolean { | |
| if (status !== 503) return false; | |
| if (!contentType?.includes("application/json")) return false; | |
| return ALL_ACCOUNTS_RATE_LIMITED_PATTERNS.some((p) => p.test(errorText)); | |
| const ALL_ACCOUNTS_RATE_LIMITED_PATTERNS = [/unavailable/i]; | |
| function isAllAccountsRateLimitedResponse(status: number, contentType: string | null, errorText: string): boolean { | |
| if (status !== 503) return false; | |
| if (!contentType?.toLowerCase().includes("application/json")) return false; | |
| return ALL_ACCOUNTS_RATE_LIMITED_PATTERNS.some((p) => p.test(errorText)); | |
| } |
![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: 3bc97b3e37
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| handleSingleModel: async (_body, modelStr) => { | ||
| calls.push(modelStr); | ||
| // Simulate handleNoCredentials returning a 503 with "unavailable" message | ||
| // This is the signal emitted when getProviderCredentialsWithQuotaPreflight exhausts all accounts | ||
| return new Response( | ||
| JSON.stringify({ error: { message: `[provider/model] Service temporarily unavailable` } }), | ||
| { | ||
| status: 503, | ||
| headers: { "content-type": "application/json" }, | ||
| } | ||
| ); |
There was a problem hiding this comment.
This test’s handleSingleModel stub always returns a 503 for every model, so the combo can never produce the asserted result.ok === true / "ok" payload. As written, it will fail instead of validating the new fallback behavior; you need to make at least one fallback target (e.g., model-b) return a 200 response. The same pattern appears in the adjacent round-robin fallback test.
Useful? React with 👍 / 👎.
|
Thanks @diegosouzapw for this great contribution! 🎉 This resilience improvement was already integrated into our pipeline during the v3.6.9 patch, but we are officially merging this PR now to ensure it's tracked properly and included in the release/v3.7.0 history. We appreciate your effort! |
|
Thanks @diegosouzapw for this great contribution! 🎉 This resilience improvement was already partially integrated into our pipeline during the v3.6.9 patch, but we are officially merging this PR now to ensure it's tracked properly and included in the release/v3.7.0 history. We appreciate your effort! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6389c4f82
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const supportsPromptCaching = | ||
| provider === "claude" || provider?.startsWith?.("anthropic-compatible-"); |
There was a problem hiding this comment.
prepareClaudeRequest now gates all cache marker insertion on supportsPromptCaching, but that predicate only matches "claude" and anthropic-compatible-*. The primary API-key Anthropic provider ID is "anthropic" (see open-sse/config/providerRegistry.ts), so Anthropic-bound requests will have cache_control stripped and never re-added, silently disabling prompt caching and increasing latency/cost relative to previous behavior. Please include provider === "anthropic" in this check.
Useful? React with 👍 / 👎.
diegosouzapw#1523) Integrated into release/v3.7.0
diegosouzapw#1523) Integrated into release/v3.7.0
2 participants
…#1398)
Integrated into release/v3.6.9
Summary
Related Issues
Validation
npm run lintnpm run test:unitnpm run test:coverage>= 60%for statements, lines, functions, and branchesTests Added Or Updated
Coverage Notes
src/,open-sse/,electron/, orbin/, explain which tests cover the change.Reviewer Notes