fix: fall back on model support errors during auth rotation#2222
fix: fall back on model support errors during auth rotation#2222kaitranntt wants to merge 2 commits intorouter-for-me:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the authentication rotation mechanism by introducing more intelligent error handling for model support issues. Previously, certain model-related errors would halt the process; now, the system can gracefully fall back to alternative authentication methods or models. This change improves the resilience and reliability of the proxy when dealing with upstream model limitations or unavailability, ensuring a smoother user experience by attempting viable alternatives. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 effectively addresses the issue of model support errors during authentication rotation. The introduction of isModelSupportResultError and its integration into the MarkResult function correctly identifies and handles these specific errors, allowing for appropriate fallback and suspension of the failing auth-model path. The update to isRequestInvalidError ensures that model support errors are no longer treated as terminal request-shape failures, which is crucial for the intended fallback mechanism. The added regression tests provide good coverage and validate the new behavior for both bad requests and unprocessable entities, confirming that the system now falls back and suspends auths as expected. Overall, the changes are well-implemented and improve the robustness of the authentication rotation logic.
![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: 5135c22cd6
ℹ️ 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 isModelSupportResultError(result.Error) { | ||
| next := now.Add(12 * time.Hour) | ||
| state.NextRetryAfter = next | ||
| suspendReason = "not_found" | ||
| suspendReason = "model_not_supported" | ||
| shouldSuspendModel = true |
There was a problem hiding this comment.
When a requested model expands to multiple upstream candidates on the same auth, this new branch suspends result.Model, but executeMixedOnce/executeStreamWithModelPool always populate Result.Model with the alias routeModel rather than the failing upstream candidate. If the first candidate returns a model-support 400/422 and a later candidate succeeds, the success path resets that same alias state and calls ResumeClientModel, so the unsupported upstream candidate is eligible again as soon as the pool rotates back. In practice the new suspension never sticks for OpenAI-compat alias pools, which defeats the stated goal of avoiding immediate reselection of the bad path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ea3e0b7. The fallback state is now keyed to the concrete upstream candidate for pooled aliases, later requests skip suspended upstream members, and a fully blocked pool auth no longer burns retry budget before a healthy fallback auth can run. Added execute/stream/count regressions for repeated pooled requests and the retry-budget edge case.
| if isModelSupportError(err) { | ||
| return false |
There was a problem hiding this comment.
After this change, ExecuteCount will also treat model-support 400/422s as fallback-eligible, but executeCountMixedOnce still only calls m.hook.OnResult instead of MarkResult (around sdk/cliproxy/auth/conductor.go:1087-1107). In setups that run token counting before execution, the current call can fall through to another auth/model, yet the unsupported auth/model is never marked unavailable, so every later count request retries the same failing credential again. The new behavior is therefore sticky for Execute/ExecuteStream, but not for ExecuteCount.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ea3e0b7. ExecuteCount now records through MarkResult, so model-support fallback updates auth/model availability the same way as execute/stream. Added count-path regressions to cover repeated pooled requests after a model-support failure.
|
Pushed follow-up commit This addresses both Codex review findings and also fixes one related stream edge case: pre-first-byte stream bootstrap failures now continue outer auth rotation when another auth is still available, while preserving the terminal error-stream contract when nothing else can run. Verification run on this branch:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea3e0b713e
ℹ️ 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".
| continue | ||
| } | ||
| m.hook.OnResult(execCtx, result) | ||
| m.MarkResult(execCtx, result) |
There was a problem hiding this comment.
This now calls MarkResult on every successful ExecuteCount, but several CountTokens paths do not prove the auth/model is usable for real execution—for example OpenAICompatExecutor.CountTokens (internal/runtime/executor/openai_compat_executor.go:307-332) and CodexExecutor.CountTokens (internal/runtime/executor/codex_executor.go:402-435) are purely local tokenization. If a model was suspended after a real 429 or model-support failure, a later /count_tokens success will clear NextRetryAfter and ResumeClientModel anyway. In setups that count tokens before sending requests, that immediately undoes the new suspension/cooldown logic and routes traffic back to throttled or unsupported credentials.
Useful? React with 👍 / 👎.
| func executionResultModel(routeModel, upstreamModel string, pooled bool) string { | ||
| if pooled { | ||
| if resolved := strings.TrimSpace(upstreamModel); resolved != "" { | ||
| return resolved |
There was a problem hiding this comment.
In pooled alias routes, executionResultModel now records retriable failures under the concrete upstream names (qwen3.5-plus, glm-5, etc.), but shouldRetryAfterError still asks closestCooldownWait about the requested alias (req.Model). Since isAuthBlockedForModel only checks the model key it is given, once every candidate in the pool is cooling down from a 429/5xx the manager finds no blocked auth for the alias and skips the configured request_retry wait entirely. This regresses retry behavior for OpenAI-compat alias pools compared with the previous alias-scoped state.
Useful? React with 👍 / 👎.
1 participant
Closes #2221
Summary
400/422responses as fallback-eligible instead of terminal request-shape failuresValidation
go test ./sdk/cliproxy/auth/...go test ./sdk/cliproxy/...go test ./...