fix(gateway): enable retry/fallback for auto-routed model requests - #1732
Conversation
After auto routing selects a real model (e.g. gpt-5-nano), modelInfo still pointed to the "auto" model definition which only has "llmgateway" as a provider. This caused selectNextProvider() in the retry loop to never find matching providers, silently disabling retries. Update modelInfo to the selected model's definition after auto routing so the retry loop can discover alternative providers on failure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThe change modifies Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/gateway/src/chat/chat.ts (1)
891-898:selectedProviders: any[]propagates intomodelInfo.providers, losing type safety downstream.
selectedProvidersis declared asany[](line 721) even though it is always a filtered subset ofmodelDef.providers. Assigning it tomodelInfo.providershere makes all subsequent accesses tomodelInfo.providersuntyped — including theselectNextProvidercalls and theresolveProviderContextcalls in both retry loops.🛡️ Suggested fix — type `selectedProviders` correctly
- let selectedProviders: any[] = []; + let selectedProviders: ModelDefinition["providers"] = [];This preserves the existing runtime behaviour while restoring type-checking on every provider property access (
reasoning,webSearch,providerId, etc.).As per coding guidelines, "Never use
anyoras anytype assertions in TypeScript code unless absolutely necessary."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 891 - 898, selectedProviders is typed as any[] which propagates into modelInfo.providers and breaks downstream type safety (used by selectNextProvider and resolveProviderContext). Change the declaration of selectedProviders to the proper provider element type (the same type as elements of modelDef.providers — e.g., Provider / ModelProvider / the existing provider interface used elsewhere) instead of any[], and ensure modelInfo.providers is assigned that typed array; update any intermediate filters to preserve that type (use typed predicates or type guards) so selectNextProvider and resolveProviderContext continue to have full typings without runtime changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 899-905: The fallback path uses models.find((m) => m.id ===
"gpt-5-nano") to set fallbackModelDef but silently does nothing if that returns
undefined, leaving modelInfo still pointing at the "auto" definition; change the
block so that if fallbackModelDef is undefined you emit a warning (use the
existing logger/processLogger) including the missing id and maybe list available
model ids, and explicitly clear or set modelInfo to undefined/null instead of
leaving the previous value so callers can handle the missing model rather than
silently continuing; update the models.find/fallbackModelDef branch to log the
warning and set modelInfo accordingly.
---
Nitpick comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 891-898: selectedProviders is typed as any[] which propagates into
modelInfo.providers and breaks downstream type safety (used by
selectNextProvider and resolveProviderContext). Change the declaration of
selectedProviders to the proper provider element type (the same type as elements
of modelDef.providers — e.g., Provider / ModelProvider / the existing provider
interface used elsewhere) instead of any[], and ensure modelInfo.providers is
assigned that typed array; update any intermediate filters to preserve that type
(use typed predicates or type guards) so selectNextProvider and
resolveProviderContext continue to have full typings without runtime changes.
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where retry/fallback behavior was silently disabled for auto-routed model requests. When auto-routing selected a real model (e.g., gpt-5-nano), the modelInfo variable still pointed to the auto model definition which only has llmgateway as a provider. This caused selectNextProvider() in both streaming and non-streaming retry loops to never find matching providers since it searches through modelInfo.providers.
Changes:
- Change
modelInfofromconsttoletto allow reassignment after auto-routing - After auto-routing completes, update
modelInfoto the selected model's definition with filtered providers - Handle fallback case where no suitable model is found by looking up the gpt-5-nano model definition
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Fallback case: look up the default model definition | ||
| const fallbackModelDef = models.find((m) => m.id === "gpt-5-nano"); | ||
| if (fallbackModelDef) { | ||
| modelInfo = fallbackModelDef; |
There was a problem hiding this comment.
In the fallback case where no suitable model is found during auto-routing, the code looks up the full gpt-5-nano model definition which includes ALL providers without any filtering. This differs from the main path (lines 894-898) which uses selectedProviders that are pre-filtered for availability, IAM rules, and capabilities (lines 752-828).
While the retry logic will eventually filter out unusable providers through failed resolveProviderContext calls, this causes unnecessary retry attempts with providers that are known to be unavailable or unsuitable. Consider applying similar filtering to the fallback path, or extracting the filtering logic (similar to lines 1112-1149) to ensure only suitable providers are included in modelInfo.providers.
This is a minor inefficiency rather than a critical bug, as failed retries will be handled gracefully, but it could lead to suboptimal user experience with extra latency from failed retry attempts.
| modelInfo = fallbackModelDef; | |
| // Filter providers to only those with configured environment tokens to | |
| // avoid retrying against known-unavailable providers. | |
| const availableProviders = Array.isArray(fallbackModelDef.providers) | |
| ? fallbackModelDef.providers.filter((pm) => { | |
| const providerConfig = | |
| providers[ | |
| pm.providerId as keyof typeof providers | |
| ]; | |
| return ( | |
| !!providerConfig && | |
| hasProviderEnvironmentToken(providerConfig) | |
| ); | |
| }) | |
| : fallbackModelDef.providers; | |
| modelInfo = { | |
| ...fallbackModelDef, | |
| providers: availableProviders, | |
| }; |
| // Fallback case: look up the default model definition | ||
| const fallbackModelDef = models.find((m) => m.id === "gpt-5-nano"); | ||
| if (fallbackModelDef) { | ||
| modelInfo = fallbackModelDef; |
There was a problem hiding this comment.
If the fallback model definition for "gpt-5-nano" is not found, modelInfo remains unchanged and still points to the "auto" model definition. This means the retry/fallback logic would still fail to find matching providers, which is the exact issue this PR is trying to fix.
Consider adding an else clause or throwing an error if fallbackModelDef is not found, or at minimum add a comment explaining that gpt-5-nano is expected to always exist. This would make the code more defensive and prevent silent failures in unexpected scenarios.
| // Fallback case: look up the default model definition | |
| const fallbackModelDef = models.find((m) => m.id === "gpt-5-nano"); | |
| if (fallbackModelDef) { | |
| modelInfo = fallbackModelDef; | |
| // Fallback case: look up the default model definition. | |
| // This model is expected to always exist; if it does not, treat it as a configuration error. | |
| const fallbackModelDef = models.find((m) => m.id === "gpt-5-nano"); | |
| if (fallbackModelDef) { | |
| modelInfo = fallbackModelDef; | |
| } else { | |
| logger.error( | |
| "Default fallback model 'gpt-5-nano' not found in models list during auto routing fallback.", | |
| ); | |
| throw new HTTPException(500, { | |
| message: | |
| "Internal configuration error: default fallback model 'gpt-5-nano' is not available.", | |
| }); |
Summary
gpt-5-nano),modelInfostill pointed to theautomodel definition which only hasllmgatewayas a providerselectNextProvider()in both the streaming and non-streaming retry loops to never find matching providers, silently disabling all retry/fallback behavior for auto-routed requestsmodelInfoto the selected model's definition (with filtered providers) after auto routing completesTest plan
gpt-4owithout provider prefix) still retry correctly (regression check)openai/gpt-4o) still skip retries as expectedapps/gateway/src/chat/tools/)🤖 Generated with Claude Code
Summary by CodeRabbit