feat(gateway): add provider retry fallback - #1651
Conversation
When a provider fails with 5xx, 429, or network errors and no specific provider was requested, automatically retry with the next best provider (up to 2 retries). Failed attempts are logged in response metadata and DB. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a provider-aware retry-and-fallback system to the gateway chat: per-attempt provider context resolution, provider selection/heuristics, per-attempt failure routing metadata, streaming-aware retries, tests, and schema/UI updates to expose failed attempts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant ProviderA as Provider (attempt 1)
participant ProviderB as Provider (attempt 2)
participant DB
Client->>Gateway: chat request
Gateway->>Gateway: resolveProviderContext (attempt 1)
Gateway->>ProviderA: send request (attempt 1)
ProviderA-->>Gateway: error / timeout (FailedAttempt)
Gateway->>DB: record failed attempt (routing metadata)
Gateway->>Gateway: selectNextProvider, resolveProviderContext (attempt 2)
Gateway->>ProviderB: send request (attempt 2)
ProviderB-->>Gateway: success (stream/response)
Gateway->>DB: log final used provider + routing array
Gateway-->>Client: OpenAI-style response (metadata.routing includes FailedAttempt[])
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Pull request overview
Adds automatic provider fallback retries in the gateway when an upstream provider fails with retryable errors, while tracking failed attempts in response metadata and persisted logs.
Changes:
- Introduces retry decision + provider selection utilities (
retry-with-fallback.ts) with unit tests. - Adds provider-context re-resolution helper (
resolveProviderContext) to support retry iterations. - Plumbs failed-attempt routing metadata through OpenAI-shaped responses and database logs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/actions/src/get-cheapest-from-available-providers.ts | Extends RoutingMetadata to optionally include failed-attempt routing data. |
| apps/gateway/src/chat/tools/transform-response-to-openai.ts | Adds optional routing metadata into non-streaming OpenAI-formatted responses. |
| apps/gateway/src/chat/tools/retry-with-fallback.ts | Implements retryability checks, retry gatekeeping, next-provider selection, and error-type mapping. |
| apps/gateway/src/chat/tools/retry-with-fallback.spec.ts | Unit tests for retry/fallback helper logic. |
| apps/gateway/src/chat/tools/resolve-provider-context.ts | New helper to rebuild provider-specific request context for retry attempts. |
| apps/gateway/src/chat/chat.ts | Implements streaming + non-streaming retry loops, logs failed attempts, and emits routing metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| frequency_penalty = ctx.frequency_penalty; | ||
| presence_penalty = ctx.presence_penalty; | ||
| } catch { | ||
| failedProviderIds.add(nextProvider.providerId); |
There was a problem hiding this comment.
In the non-streaming retry loop, a failure inside resolveProviderContext (catch { ... continue; }) still increments retryAttempt, potentially exhausting retries without making any upstream call for that iteration. If context resolution fails (e.g., no key for that provider), consider skipping the retry counter and moving to the next candidate provider so you still get up to MAX_RETRIES real attempts.
| failedProviderIds.add(nextProvider.providerId); | |
| failedProviderIds.add(nextProvider.providerId); | |
| // Context resolution failed before making an upstream call; | |
| // do not consume a retry attempt for this iteration. | |
| if (retryAttempt > 0) { | |
| retryAttempt -= 1; | |
| } |
| options.hasExistingToolCalls, | ||
| providerKey?.options || undefined, | ||
| configIndex, | ||
| false, // isImageGeneration — retry doesn't change image model status | ||
| ); |
There was a problem hiding this comment.
getProviderEndpoint(..., false) hard-codes imageGenerations to false. For image generation models this can produce the wrong base URL/endpoint (e.g., providers like Alibaba/ZAI switch URLs when imageGenerations is true), breaking retries for image models. Compute isImageGeneration before calling getProviderEndpoint and pass it through (and/or reuse providerMappingInfo.imageGenerations).
| const supportsReasoning = modelInfo.providers.some( | ||
| (provider) => (provider as ProviderModelMapping).reasoning === true, | ||
| ); |
There was a problem hiding this comment.
supportsReasoning is derived from modelInfo.providers.some(...), which can be true even if the selected provider mapping has reasoning: false (some models have mixed reasoning support across providers). This can cause prepareRequestBody to send provider-specific reasoning/thinking fields to a provider that doesn't support them. Derive supportsReasoning from the selected providerMappingInfo.reasoning === true instead of .some(...).
| const supportsReasoning = modelInfo.providers.some( | |
| (provider) => (provider as ProviderModelMapping).reasoning === true, | |
| ); | |
| const supportsReasoning = | |
| (providerMappingInfo as ProviderModelMapping | undefined)?.reasoning === | |
| true; |
| let supportsReasoning = modelInfo.providers.some( | ||
| (provider) => (provider as ProviderModelMapping).reasoning === true, | ||
| ); |
There was a problem hiding this comment.
supportsReasoning is computed with modelInfo.providers.some(...), which can be true even when the chosen provider mapping has reasoning: false (models can have different reasoning support per provider). This can lead to invalid request bodies (e.g., sending Anthropic/ZAI thinking fields) when fallback switches providers. Prefer computing this from the specific mapping for usedProvider+usedModel (e.g., providerMapping.reasoning === true).
| frequency_penalty = ctx.frequency_penalty; | ||
| presence_penalty = ctx.presence_penalty; | ||
| } catch { | ||
| failedProviderIds.add(nextProvider.providerId); |
There was a problem hiding this comment.
If resolveProviderContext throws, the catch { ... continue; } still advances the for (retryAttempt...) counter. This can consume a retry without making an upstream attempt (e.g., missing API key for the next provider), reducing the number of actual fallback attempts below the intended MAX_RETRIES. Consider not incrementing the retry count when context resolution fails (e.g., loop until you either successfully build context or exhaust candidate providers).
| failedProviderIds.add(nextProvider.providerId); | |
| failedProviderIds.add(nextProvider.providerId); | |
| retryAttempt--; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/gateway/src/chat/chat.ts`:
- Around line 2164-2188: The retry logic can pick ineligible or intentionally
skipped providers because it passes routingMetadata?.providerScores directly
into selectNextProvider; fix by computing a retry-eligible provider list once
from the initial filtered providers (e.g., using modelInfo.providers or whatever
initial filter produced the "allowed" providers), then before each call to
selectNextProvider produce a filtered scores array that: 1) only contains
entries whose providerId exists in that retry-eligible set, 2) excludes scores <
0 (or other markers for excluded providers), and 3) omits providers in
failedProviderIds; if the filtered scores array is empty, synthesize a fallback
score list from the retry-eligible providers (respecting failedProviderIds) and
pass that sanitized list to selectNextProvider instead of
routingMetadata?.providerScores so retries never target excluded or non‑eligible
providers.
In `@apps/gateway/src/chat/tools/resolve-provider-context.ts`:
- Around line 147-169: resolveProviderContext currently falls back to env tokens
(via getProviderEnv) in credits/hybrid mode without performing the same
credit/plan validation used in the main provider path, allowing retries to
consume credits when balance is zero; extract the credit-availability check from
chat.ts into a shared helper (e.g., ensureSufficientCreditsOrPlan) and call it
inside resolveProviderContext before assigning usedToken from getProviderEnv in
both the credits branch and the hybrid fallback branch (the code paths around
findCustomProviderKey/findProviderKey and getProviderEnv) so env-token usage is
gated by the same validation.
- Around line 181-212: Compute the actual isImageGeneration flag before calling
getProviderEndpoint by inspecting modelInfo.providers for a mapping matching
usedProvider and usedModel (use the existing imageGenProviderMapping /
ProviderModelMapping logic to set isImageGeneration), then pass that computed
isImageGeneration into getProviderEndpoint instead of the hard-coded false;
update the block where url is created to use the precomputed isImageGeneration
so retries and endpoint selection use the correct image-generation status.
| // --- Retry loop for provider fallback --- | ||
| const failedAttempts: FailedAttempt[] = []; | ||
| const failedProviderIds = new Set<string>(); | ||
| let res: Response | undefined; | ||
| for (let retryAttempt = 0; retryAttempt <= MAX_RETRIES; retryAttempt++) { | ||
| // Type guard: narrow variables that TypeScript widens due to loop reassignment | ||
| if ( | ||
| usedProvider === "anthropic" && | ||
| response_format?.type === "json_schema" | ||
| !usedProvider || | ||
| !usedToken || | ||
| !url || | ||
| !usedModelFormatted || | ||
| !usedModelMapping | ||
| ) { | ||
| const currentBeta = headers["anthropic-beta"]; | ||
| headers["anthropic-beta"] = currentBeta | ||
| ? `${currentBeta},structured-outputs-2025-11-13` | ||
| : "structured-outputs-2025-11-13"; | ||
| throw new Error("Provider context not initialized"); | ||
| } | ||
|
|
||
| // Create a combined signal for both timeout and cancellation | ||
| const fetchSignal = createStreamingCombinedSignal( | ||
| requestCanBeCanceled ? controller : undefined, | ||
| ); | ||
| if (retryAttempt > 0) { | ||
| // Re-add abort listener (catch block removes it on error) | ||
| c.req.raw.signal.addEventListener("abort", onAbort); | ||
|
|
||
| res = await fetch(url, { | ||
| method: "POST", | ||
| headers, | ||
| body: JSON.stringify(requestBody), | ||
| signal: fetchSignal, | ||
| }); | ||
| } catch (error) { | ||
| // Clean up the event listeners | ||
| c.req.raw.signal.removeEventListener("abort", onAbort); | ||
| const nextProvider = selectNextProvider( | ||
| routingMetadata?.providerScores ?? [], | ||
| failedProviderIds, | ||
| modelInfo.providers, | ||
| ); |
There was a problem hiding this comment.
Retry selection can target ineligible or intentionally skipped providers.
selectNextProvider is fed routingMetadata.providerScores. In low‑uptime fallback, the original provider is stored with a negative score; in direct-provider routing, providerScores can include providers without keys or capability filtering; and random exploration can leave providerScores empty. This can cause retries to bounce to excluded providers or skip viable fallbacks. Consider keeping a retry‑eligible provider list from the initial filtered providers and filter providerScores (e.g., exclude score < 0 / non‑eligible IDs) before calling selectNextProvider.
Also applies to: 4642-4650
🤖 Prompt for AI Agents
In `@apps/gateway/src/chat/chat.ts` around lines 2164 - 2188, The retry logic can
pick ineligible or intentionally skipped providers because it passes
routingMetadata?.providerScores directly into selectNextProvider; fix by
computing a retry-eligible provider list once from the initial filtered
providers (e.g., using modelInfo.providers or whatever initial filter produced
the "allowed" providers), then before each call to selectNextProvider produce a
filtered scores array that: 1) only contains entries whose providerId exists in
that retry-eligible set, 2) excludes scores < 0 (or other markers for excluded
providers), and 3) omits providers in failedProviderIds; if the filtered scores
array is empty, synthesize a fallback score list from the retry-eligible
providers (respecting failedProviderIds) and pass that sanitized list to
selectNextProvider instead of routingMetadata?.providerScores so retries never
target excluded or non‑eligible providers.
| } else if (project.mode === "credits") { | ||
| const envResult = getProviderEnv(usedProvider as Provider); | ||
| usedToken = envResult.token; | ||
| configIndex = envResult.configIndex; | ||
| envVarName = envResult.envVarName; | ||
| } else if (project.mode === "hybrid") { | ||
| if (usedProvider === "custom" && options.customProviderName) { | ||
| providerKey = await findCustomProviderKey( | ||
| project.organizationId, | ||
| options.customProviderName, | ||
| ); | ||
| } else { | ||
| providerKey = await findProviderKey(project.organizationId, usedProvider); | ||
| } | ||
|
|
||
| if (providerKey) { | ||
| usedToken = providerKey.token; | ||
| } else { | ||
| const envResult = getProviderEnv(usedProvider as Provider); | ||
| usedToken = envResult.token; | ||
| configIndex = envResult.configIndex; | ||
| envVarName = envResult.envVarName; | ||
| } |
There was a problem hiding this comment.
Credits validation is skipped when falling back to env tokens.
In credits/hybrid mode, if no provider key exists, resolveProviderContext falls back to env tokens without the credit/dev-plan checks that the main flow enforces. Retries can therefore consume credits even when the balance is zero. Consider factoring the credit-availability check from chat.ts into a shared helper and applying it before using env tokens.
🤖 Prompt for AI Agents
In `@apps/gateway/src/chat/tools/resolve-provider-context.ts` around lines 147 -
169, resolveProviderContext currently falls back to env tokens (via
getProviderEnv) in credits/hybrid mode without performing the same credit/plan
validation used in the main provider path, allowing retries to consume credits
when balance is zero; extract the credit-availability check from chat.ts into a
shared helper (e.g., ensureSufficientCreditsOrPlan) and call it inside
resolveProviderContext before assigning usedToken from getProviderEnv in both
the credits branch and the hybrid fallback branch (the code paths around
findCustomProviderKey/findProviderKey and getProviderEnv) so env-token usage is
gated by the same validation.
Add unit tests with the mock server to verify error classification for specific HTTP status codes (500, 429, 404, 401, 403, 503), metadata in responses, routing metadata and error details in DB log entries, streaming error handling, and X-No-Fallback header behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/gateway/src/fallback.spec.ts`:
- Around line 856-897: The test "streaming: retries on 500 and delivers response
on fallback provider" conditionally guards routing assertions behind if
(logWithRouting), letting the test pass without verifying retry metadata; change
this to assert that a log with routing exists (e.g.,
expect(logWithRouting).toBeDefined()) after obtaining logs via waitForLogs and
then unconditionally check routingMetadata.routing contents
(routing[0].status_code === 500) so the test fails when routing info is missing;
update references: the test name, variables logWithRouting, logs, and
waitForLogs.
- Around line 729-783: The test "non-streaming: retries on 500 and succeeds on
fallback provider with failed_attempts in metadata" currently wraps key
assertions in conditionals (if (json.metadata.routing), if (successLog), if
(successLog.routingMetadata?.routing)) which lets the test pass without
verifying retry behavior; update the test to assert unconditionally that
json.metadata.routing exists and has length >= 1 and contains an entry with
provider, status_code: 500, and error_type, and assert unconditionally that a
successLog exists (fail the test if not) and that
successLog.routingMetadata.routing exists and contains an entry with status_code
500, using the existing symbols json.metadata.routing, successLog, and
successLog.routingMetadata to locate and modify the assertions.
- Around line 370-371: Update the incorrect property path string passed to
toHaveProperty by removing the dot before the bracket (e.g. change
"choices.[0].message.content" to "choices[0].message.content" or
"choices.0.message.content"); search for other occurrences (the comment notes a
second instance around the later assertion) and fix them similarly so
Jest/Vitest recognizes the array index correctly in the expectation checks.
🧹 Nitpick comments (3)
apps/gateway/src/test-utils/mock-openai-server.ts (2)
130-136: Module-level mutable state is acceptable here but fragile across test files.
failOnceCounteris shared module state. This works because Vitest runs tests within a file sequentially, and the spec resets it inbeforeEach. However, if another test file imports from this module and triggersTRIGGER_FAIL_ONCE, the counter state could leak. Consider documenting this constraint or scoping the counter per-request-content (e.g., aSetof request IDs) for more robust isolation.
214-214:as anycast on status code is justified here.Hono's
c.status()expects narrow literal types, so casting a dynamicnumberrequires this. Since this is test utility code, the trade-off is acceptable. A comment explaining why would help future readers.Optional: add clarifying comment
- c.status(statusTrigger.statusCode as any); + // Hono expects literal status types; cast needed for dynamic status codes + c.status(statusTrigger.statusCode as any);As per coding guidelines, "Never use
anyoras anytype assertions in TypeScript code unless absolutely necessary".apps/gateway/src/fallback.spec.ts (1)
767-768: Minor:(l: any)in.find()callbacks.These could be typed using the log table's inferred type to avoid
any.As per coding guidelines, "Never use
anyoras anytype assertions in TypeScript code unless absolutely necessary".Also applies to: 887-887
Test fixes: - Fix toHaveProperty path syntax (use array notation for nested paths) - Make conditional assertions unconditional in retry tests - Type log callbacks with Log type instead of any - Add clarifying comment for Hono status code cast - Document failOnceCounter module-level state constraint Feature fixes: - Don't consume retry slots for context-resolution failures - Derive supportsReasoning from selected provider, not .some() across all - Compute isImageGeneration from provider mapping instead of hardcoding false - Add routing/noFallback/originalProvider fields to DB schema type Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/src/chat/chat.ts (1)
2706-2834:⚠️ Potential issue | 🟡 MinorAbort listener not removed before retry on HTTP error (streaming path).
When the fetch succeeds but
!res.ok, the code logs and potentially retries viacontinue(line 2833). However, unlike thecatchblock (line 2297) which callsremoveEventListener("abort", onAbort), this path does not remove the listener before continuing. On the next iteration, line 2184 adds a new listener, resulting in duplicate listeners.While
AbortController.abort()is idempotent, duplicate handlers cause redundant work and thecanceled = trueassignment runs multiple times unnecessarily. Add a cleanup call beforecontinue.Proposed fix
if ( shouldRetryRequest({ ... }) ) { failedAttempts.push({...}); failedProviderIds.add(usedProvider); + c.req.raw.signal.removeEventListener("abort", onAbort); continue; }Apply at line 2833 and also at line 2406 (timeout retry in streaming).
🤖 Fix all issues with AI agents
In `@apps/gateway/src/chat/chat.ts`:
- Around line 2251-2256: The retry loop that calls selectNextProvider and
resolveProviderContext can spin excessively if resolveProviderContext repeatedly
throws because failedProviderIds is updated but retryAttempt is decremented so
the retry slot isn't consumed; add a secondary hard cap (e.g., totalIterations
or maxContextFailures) inside the loop to stop after a fixed number of
iterations and break with an error if exceeded. Concretely, in the blocks that
call selectNextProvider, resolveProviderContext and manipulate failedProviderIds
and retryAttempt (see selectNextProvider, resolveProviderContext,
failedProviderIds, retryAttempt, MAX_RETRIES), introduce a local counter
incremented each loop and check it against a constant like
MAX_ITERATIONS_PER_REQUEST (choose a reasonable ceiling) and abort the loop if
reached; apply the same change to both the streaming retry loop and the
non-streaming retry loop so both paths have the secondary cap.
🧹 Nitpick comments (6)
apps/gateway/src/test-utils/mock-openai-server.ts (1)
214-220:as anycast onc.status()— acceptable for test utility but noted.Hono's
c.status()expects a narrowStatusCodeunion, and dynamic status codes require a cast here. The comment on line 217 explains the rationale. While the coding guidelines say to avoidas any, this is a test-only file where the alternative would be an unwieldy union type assertion. Consider using a more specific cast likeas StatusCodeif the Hono type is importable.As per coding guidelines, "Never use
anyoras anytype assertions in TypeScript code unless absolutely necessary."♻️ Optional: use Hono's StatusCode type instead of `any`
+import type { StatusCode } from "hono/utils/http-status"; ... - c.status(statusTrigger.statusCode as any); + c.status(statusTrigger.statusCode as StatusCode);apps/gateway/src/fallback.spec.ts (1)
459-515: Deactivated provider fallback test uses a date-stamped model name.The model
"gemini-2.5-flash-preview-09-2025"(line 487) is date-stamped and may become stale as models are updated. Consider using a model name that's less likely to be removed from the models registry, or adding a comment explaining that this particular model is chosen because itsgoogle-vertexmapping is deactivated whilegoogle-ai-studiois active.apps/gateway/src/chat/tools/resolve-provider-context.ts (3)
110-122:providerMapping.providerIdis cast toProviderwithout validation.Line 118 does
const usedProvider = providerMapping.providerId as Provider. IfselectNextProvider(the retry logic) always returns valid provider IDs from the model definition, this is safe. However, there's no runtime guard. If an invalid provider ID reaches this function, it would silently propagate and fail later ingetProviderEndpointorgetProviderHeaders.
257-315: Doublemax_tokenscap: before and afterprepareRequestBody.Lines 258–268 cap
max_tokenson the local variable before passing it toprepareRequestBody, and then lines 302–315 cap it again on the returnedrequestBody.max_tokens. The post-validation is needed becauseprepareRequestBodymay set its ownmax_tokens(e.g., Anthropic defaults). The pre-validation (lines 258–268) is therefore redundant for correctness but acts as a belt-and-suspenders safeguard.♻️ Consider removing the pre-validation to reduce duplication
If
prepareRequestBodyalways respects the inputmax_tokens, the pre-cap at lines 258–268 is redundant with the post-cap at lines 302–315. Removing the pre-cap would simplify the code while the post-cap remains as the definitive guard.- // --- max_tokens validation --- - if (max_tokens !== undefined && providerMappingForSelected) { - if ( - "maxOutput" in providerMappingForSelected && - providerMappingForSelected.maxOutput !== undefined - ) { - if (max_tokens > providerMappingForSelected.maxOutput) { - // Silently cap to max output instead of throwing on retry - max_tokens = providerMappingForSelected.maxOutput; - } - } - } - // --- requestCanBeCanceled ---
176-188: Remove redundantas ProviderModelMappingcasts.
modelInfo.providersis typed asProviderModelMapping[], so.find()returnsProviderModelMapping | undefined. The casts on lines 183 and 187 are unnecessary since the optional chaining operator?.already handles theundefinedcase. Removing them improves clarity.♻️ Simplify type casts
- const supportsReasoning = - (providerMappingForSelected as ProviderModelMapping)?.reasoning === true; + const supportsReasoning = providerMappingForSelected?.reasoning === true; - const isImageGeneration = - (providerMappingForSelected as ProviderModelMapping)?.imageGenerations === - true; + const isImageGeneration = + providerMappingForSelected?.imageGenerations === true;apps/gateway/src/chat/chat.ts (1)
2251-2256: Consider logging context-resolution failures for debuggability.The bare
catchdiscards the error entirely. IfresolveProviderContextfails (e.g., missing token, invalid config), there's no log trail to explain why a provider was skipped during retry. Same applies to the non-streaming path at line 4715.Proposed fix
- } catch { + } catch (err) { + logger.warn("Failed to resolve provider context for retry", { + providerId: nextProvider.providerId, + error: err instanceof Error ? err.message : String(err), + }); failedProviderIds.add(nextProvider.providerId);
| } catch { | ||
| failedProviderIds.add(nextProvider.providerId); | ||
| // Don't consume a retry slot for context-resolution failures | ||
| retryAttempt--; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Unbounded loop if resolveProviderContext consistently throws for many providers.
When resolveProviderContext throws, the code adds the provider to failedProviderIds and decrements retryAttempt so the retry slot isn't consumed. The loop is bounded by modelInfo.providers length (since selectNextProvider returns null when all are exhausted), but if the model has many providers and context resolution keeps failing, this could iterate far more than MAX_RETRIES times — potentially dozens of iterations, each doing async work (DB lookups, token resolution).
Consider adding a secondary cap (e.g., total iteration count) to prevent excessive looping on repeated context-resolution failures.
Proposed fix
+ const MAX_TOTAL_ITERATIONS = MAX_RETRIES + modelInfo.providers.length;
+ let totalIterations = 0;
for (let retryAttempt = 0; retryAttempt <= MAX_RETRIES; retryAttempt++) {
+ if (++totalIterations > MAX_TOTAL_ITERATIONS) {
+ break;
+ }Apply to both streaming (line 2170) and non-streaming (line 4634) retry loops.
🤖 Prompt for AI Agents
In `@apps/gateway/src/chat/chat.ts` around lines 2251 - 2256, The retry loop that
calls selectNextProvider and resolveProviderContext can spin excessively if
resolveProviderContext repeatedly throws because failedProviderIds is updated
but retryAttempt is decremented so the retry slot isn't consumed; add a
secondary hard cap (e.g., totalIterations or maxContextFailures) inside the loop
to stop after a fixed number of iterations and break with an error if exceeded.
Concretely, in the blocks that call selectNextProvider, resolveProviderContext
and manipulate failedProviderIds and retryAttempt (see selectNextProvider,
resolveProviderContext, failedProviderIds, retryAttempt, MAX_RETRIES), introduce
a local counter incremented each loop and check it against a constant like
MAX_ITERATIONS_PER_REQUEST (choose a reasonable ceiling) and abort the loop if
reached; apply the same change to both the streaming retry loop and the
non-streaming retry loop so both paths have the secondary cap.
Incorporates content_filter handling, ratio-based scoring, and Google sign-in changes from main into the retry-with-fallback branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/gateway/src/chat/chat.ts`:
- Around line 2386-2407: The remainingProviders count can undercount because it
only uses routingMetadata?.providerScores.length; update the calculation used in
the shouldRetryRequest call (and the same logic at the other occurrences) to
base remainingProviders on the broader provider set (union of
routingMetadata.providerScores providers and modelInfo.providers) minus
failedProviderIds and the current usedProvider so retries consider all possible
fallbacks; locate the calculation around shouldRetryRequest(...) (references:
shouldRetryRequest, routingMetadata, providerScores, modelInfo.providers,
failedProviderIds, remainingProviders) and replace the single-array length math
with a computed set-size of all available providers minus already failed/used
providers, and apply the same change at the other two occurrences noted.
| if ( | ||
| shouldRetryRequest({ | ||
| requestedProvider, | ||
| noFallback, | ||
| statusCode: 0, | ||
| retryCount: retryAttempt, | ||
| remainingProviders: | ||
| (routingMetadata?.providerScores.length ?? 0) - | ||
| failedProviderIds.size - | ||
| 1, | ||
| usedProvider, | ||
| }) | ||
| ) { | ||
| failedAttempts.push({ | ||
| provider: usedProvider, | ||
| model: usedModel, | ||
| status_code: 0, | ||
| error_type: getErrorType(0), | ||
| }); | ||
| failedProviderIds.add(usedProvider); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
remainingProviders calculation may undercount available fallback targets.
The formula (routingMetadata?.providerScores.length ?? 0) - failedProviderIds.size - 1 uses providerScores length, but selectNextProvider also falls back to modelInfo.providers. If providerScores is empty or smaller than the actual available provider set (e.g., random exploration, direct-provider routing), remainingProviders could be 0 or negative, preventing retries even when viable providers exist in modelInfo.providers.
Consider using the broader provider set for this count:
- remainingProviders:
- (routingMetadata?.providerScores.length ?? 0) -
- failedProviderIds.size -
- 1,
+ remainingProviders:
+ Math.max(
+ (routingMetadata?.providerScores.length ?? 0),
+ modelInfo.providers.length,
+ ) -
+ failedProviderIds.size -
+ 1,Also applies to: 2658-2680, 2817-2839
🤖 Prompt for AI Agents
In `@apps/gateway/src/chat/chat.ts` around lines 2386 - 2407, The
remainingProviders count can undercount because it only uses
routingMetadata?.providerScores.length; update the calculation used in the
shouldRetryRequest call (and the same logic at the other occurrences) to base
remainingProviders on the broader provider set (union of
routingMetadata.providerScores providers and modelInfo.providers) minus
failedProviderIds and the current usedProvider so retries consider all possible
fallbacks; locate the calculation around shouldRetryRequest(...) (references:
shouldRetryRequest, routingMetadata, providerScores, modelInfo.providers,
failedProviderIds, remainingProviders) and replace the single-array length math
with a computed set-size of all available providers minus already failed/used
providers, and apply the same change at the other two occurrences noted.
Enrich providerScores metadata with failure info (failed, status_code, error_type) from the retry loop's failedAttempts array. Display failure indicators inline next to provider scores and add a "Failed Attempts" section in both log-card and log-detail-client UI components. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/gateway/src/chat/chat.ts`:
- Line 4867: The failed-attempt log uses duration = Date.now() - startTime which
measures total elapsed time since the first attempt; change the retry loop to
record a per-attempt start timestamp at the top of each iteration (e.g.,
perAttemptStartTime = Date.now()), compute perAttemptDuration = Date.now() -
perAttemptStartTime and totalDuration = Date.now() - startTime, and update the
failed-attempt log (the place that currently sets duration) to include
perAttemptDuration (and optionally totalDuration) instead of the cumulative
duration variable; keep the existing startTime and duration variables but ensure
the per-attempt values are computed and logged inside the retry loop where
failed attempts are emitted.
🧹 Nitpick comments (3)
packages/db/src/schema.ts (1)
473-498: Consider sharing theRoutingMetadatatype between schema and actions package.The
routingMetadataJSON type here duplicates theRoutingMetadatainterface frompackages/actions/src/get-cheapest-from-available-providers.ts. This was pre-existing, but the surface area is growing with each new field (failed,status_code,error_type,routing,noFallback,originalProvider,originalProviderUptime). If they drift, the DB type and the runtime type will silently disagree.Consider importing
RoutingMetadataand using.$type<RoutingMetadata>()to keep a single source of truth.apps/ui/src/components/dashboard/log-card.tsx (1)
346-419: Duplicated routing failure UI betweenlog-card.tsxandlog-detail-client.tsx.The provider-score failure indicator (Lines 348-360) and the "Failed Attempts" block (Lines 396-419) are nearly identical to Lines 420-477 in
log-detail-client.tsx. This duplication predates this PR but is growing with each new feature. Consider extracting shared components (e.g.,ProviderScoreRow,FailedAttemptsList) to reduce the maintenance surface.apps/gateway/src/chat/chat.ts (1)
2166-2947: Significant code duplication between streaming and non-streaming retry paths.The retry-loop structure —
selectNextProvider,resolveProviderContext, error handling withshouldRetryRequest,failedAttemptstracking, androutingMetadataenrichment — is duplicated almost identically between the streaming path (~780 lines) and non-streaming path (~650 lines). This significantly increases the maintenance burden and risk of the two paths drifting out of sync.Consider extracting the common retry orchestration (provider selection, context resolution, fetch + error classification, retry decision) into a shared helper, parameterized by the response-handling strategy (streaming vs. non-streaming).
Also applies to: 4707-5356
| isTimeoutFetchError ? 504 : 502, | ||
| ); | ||
| } | ||
| duration = Date.now() - startTime; |
There was a problem hiding this comment.
Per-attempt duration in failed-attempt logs is cumulative, not per-attempt.
duration is computed as Date.now() - startTime where startTime is set once before the retry loop (line 2089). Each failed-attempt log entry inside the loop records the total elapsed time since the original request began, not the duration of that specific attempt. This reduces the usefulness of per-attempt latency data for diagnostics.
Consider capturing a per-attempt start time at the top of each loop iteration, or logging both the per-attempt and total durations.
Suggested approach
for (let retryAttempt = 0; retryAttempt <= MAX_RETRIES; retryAttempt++) {
+ const attemptStartTime = Date.now();
// ...
- duration = Date.now() - startTime;
+ duration = Date.now() - startTime; // total duration
+ const attemptDuration = Date.now() - attemptStartTime; // per-attempt
// In insertLog calls for failed attempts, use attemptDuration🤖 Prompt for AI Agents
In `@apps/gateway/src/chat/chat.ts` at line 4867, The failed-attempt log uses
duration = Date.now() - startTime which measures total elapsed time since the
first attempt; change the retry loop to record a per-attempt start timestamp at
the top of each iteration (e.g., perAttemptStartTime = Date.now()), compute
perAttemptDuration = Date.now() - perAttemptStartTime and totalDuration =
Date.now() - startTime, and update the failed-attempt log (the place that
currently sets duration) to include perAttemptDuration (and optionally
totalDuration) instead of the cumulative duration variable; keep the existing
startTime and duration variables but ensure the per-attempt values are computed
and logged inside the retry loop where failed attempts are emitted.
…outing array Rename FailedAttempt to RoutingAttempt with a `succeeded` boolean field. The routing array now always includes the successful final attempt alongside any failed ones. Response metadata and SSE chunks always include routing info for consistency. UI labels updated from "Failed Attempts" to "Request Attempts" with green/red styling per attempt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Integrates reasoning.effort/reasoning.max_tokens unified configuration from #1555 and Alibaba endpoint URL fix into retry-fallback branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add `retried` boolean and `retriedByLogId` fields to log schema - Pre-generate final log ID before retry loop - Check shouldRetryRequest before insertLog to set retried fields - Failed logs that trigger a retry get retried=true and retriedByLogId pointing to the final successful log's pre-generated ID - UI shows "Retried" badge on failed logs with link to successful request - Update API log schema with routing array and retried fields - Tests verify retried flag and retriedByLogId linkage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reset migrations to origin/main state, merged latest, and regenerated migration for retried/retriedByLogId log columns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…/llmgateway into gateway-retry-fallback
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add changelog entry announcing automatic retry & fallback feature - Update routing docs with new "Automatic Retry & Fallback" section covering retry behavior, triggers, routing transparency, retried log tracking, and impact on provider health - Add retry-fallback.jpg changelog image Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Images automagically compressed by Calibre's image-actions ✨ Compression reduced images by 8.4%, saving 6.2 KB.
|
Each retry iteration now records its own start timestamp so that failed-attempt logs reflect the duration of that single attempt rather than the cumulative elapsed time since the first attempt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
routingarray) and persisted to database logsresolveProviderContext()for retry iterationsTest plan
retry-with-fallback.spec.ts— 25 tests)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests