Repository navigation
Conversation
…erence#3200) When a combo has multiple targets from the same provider and that provider fails (404, 429, 400, 502, 504), the combo would keep retrying ALL targets from the same provider before moving to a different one — creating the appearance of an infinite loop consulting the same dead provider. Root causes: 1. No consecutive failure tracking per provider across combo targets 2. !sameProviderNext guard prevented global circuit breaker from tripping for same-provider targets, even when the provider was clearly failing Fix (applied to both priority and round-robin handlers): - Add consecutiveProviderFailures Map to track per-provider failure count - After N consecutive failures (default 2, configurable via providerFailureThreshold), mark provider as exhausted so remaining same-provider targets are skipped - Modify !sameProviderNext guard to trip global breaker when threshold exceeded — the provider is proven unreliable across multiple attempts - Reset failure counter on provider success - Count failures in cheaper-inference#2101 400 guard break path (previously dead code)
![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 a mechanism to track consecutive failures per provider across targets in both handleComboChat and handleRoundRobinCombo to prevent infinite loops when a provider fails repeatedly. The review feedback highlights a critical issue where a break statement fails to exit the outer target loop on a body-specific error, and suggests returning an error response instead. Additionally, the feedback recommends robust parsing of providerFailureThreshold to handle negative numbers safely and suggests optimizing Map lookups using nullish coalescing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (provider && provider !== "unknown") { | ||
| const prev = consecutiveProviderFailures.get(provider) ?? 0; | ||
| const next = prev + 1; | ||
| consecutiveProviderFailures.set(provider, next); | ||
| if (next >= providerFailureThreshold && !exhaustedProviders.has(provider)) { | ||
| exhaustedProviders.add(provider); | ||
| log.info("COMBO", `Provider ${provider} consecutive failure threshold reached (${next}/${providerFailureThreshold}) — skipping remaining targets (#3200)`); | ||
| } | ||
| } | ||
| break; |
There was a problem hiding this comment.
The break statement on line 3858 only breaks out of the inner retry loop of executeTarget, not the outer target loop of handleComboChat. As a result, executeTarget still returns null, and the combo continues to try other models, completely defeating the purpose of the 400 Bad Request guard ("stopping combo" and "skipping fallback to other targets to prevent infinite loop"). To correctly stop the combo and return the error response immediately, we should return { ok: false, response: result }. This also makes the consecutiveProviderFailures tracking inside this block unnecessary since the entire combo execution is aborted.
return { ok: false, response: result };| const providerFailureThreshold = | ||
| (config as Record<string, unknown>).providerFailureThreshold | ||
| ? Number((config as Record<string, unknown>).providerFailureThreshold) || 2 | ||
| : 2; |
There was a problem hiding this comment.
If providerFailureThreshold is configured as a negative number (e.g., -1), Number(-1) is -1, which is truthy and bypasses the || 2 fallback. This will set the threshold to -1, causing immediate skips of all targets for that provider (since 0 >= -1 is true). We should safely parse and validate the threshold to ensure it is a positive integer >= 1.
const parsedThreshold = Number((config as Record<string, unknown>).providerFailureThreshold);
const providerFailureThreshold = Number.isInteger(parsedThreshold) && parsedThreshold > 0 ? parsedThreshold : 2;| const providerFailureThreshold = | ||
| (config as Record<string, unknown>).providerFailureThreshold | ||
| ? Number((config as Record<string, unknown>).providerFailureThreshold) || 2 | ||
| : 2; |
There was a problem hiding this comment.
Similar to the parsing in handleComboChat, if providerFailureThreshold is configured as a negative number, it will bypass the fallback and cause immediate skips of all targets for that provider in round-robin mode. We should safely parse and validate the threshold to ensure it is a positive integer >= 1.
const parsedThreshold = Number((config as Record<string, unknown>).providerFailureThreshold);
const providerFailureThreshold = Number.isInteger(parsedThreshold) && parsedThreshold > 0 ? parsedThreshold : 2;| if (provider && consecutiveProviderFailures.has(provider) && consecutiveProviderFailures.get(provider)! >= providerFailureThreshold) { | ||
| log.info("COMBO", `Skipping ${modelStr} — provider ${provider} hit consecutive failure threshold (#3200)`); | ||
| if (i > 0) fallbackCount++; | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Using consecutiveProviderFailures.has(provider) followed by consecutiveProviderFailures.get(provider)! performs redundant Map lookups. We can simplify this by using nullish coalescing consecutiveProviderFailures.get(provider) ?? 0, which is cleaner and more efficient.
| if (provider && consecutiveProviderFailures.has(provider) && consecutiveProviderFailures.get(provider)! >= providerFailureThreshold) { | |
| log.info("COMBO", `Skipping ${modelStr} — provider ${provider} hit consecutive failure threshold (#3200)`); | |
| if (i > 0) fallbackCount++; | |
| return null; | |
| } | |
| if (provider && (consecutiveProviderFailures.get(provider) ?? 0) >= providerFailureThreshold) { | |
| log.info("COMBO", "Skipping " + modelStr + " — provider " + provider + " hit consecutive failure threshold (#3200)"); | |
| if (i > 0) fallbackCount++; | |
| return null; | |
| } |
- Critical: cheaper-inference#2101 400 guard now returns {ok:false, response} to abort combo immediately instead of break (which only exits retry loop) - High: providerFailureThreshold parsing now validates positive integer >= 1 to prevent negative values causing immediate skips - Medium: removed redundant Map .has() + .get()!, using ?? 0 instead
|
Thanks for this @herjarsa — but I'm holding it for now, because I couldn't reproduce the loop it targets, and I want to avoid landing a guard that may change routing behavior without a failing repro. What I found while validating against the real combo pipeline (
Could you share a minimal failing case (ideally a test against the combo harness, or the exact combo config + upstream responses) where the same-provider loop actually happens despite connection cooldown? Once I can see it fail, I'll merge the fix (with that test as the regression guard). Really appreciate the contribution — just want it backed by a repro per our bug-fix policy. |
|
Thanks for digging into this @herjarsa — the same-provider cascade is a real concern. Before merging I ran a TDD evaluation against What I tested (real chat-pipeline harness, not an isolated unit): a Result on the clean baseline (no PR applied): the provider is dispatched to exactly once. After the first failure With this PR applied the count is the same for 404/5xx (the connection cooldown fires first), and the suite stays green (97/97) — so it doesn't regress anything, but in the common case it doesn't change behavior either, while adding a second tracking mechanism to the hot combo path. I think the counter would only help a narrow case: a provider whose error yields per-model lockout instead of whole-connection cooldown, and multiple same-provider targets. Could you share a concrete repro (provider + error code + combo shape) where the current connection-cooldown does not already short-circuit the cascade? That would let us scope the fix precisely (and add a failing test for exactly that case). I've opened #3194 with the baseline guard test so we have a shared reference. Leaving this open in the meantime — appreciate the contribution. |
Code Review SummaryStatus: 2 Previous Issues Fixed | 4 Active Issues Remain (carried forward, not in this diff) | Recommendation: Address active concerns before merge Overview
Issue Details (click to expand)Active Inline Issues (unchanged lines)
Other Observations (not in diff)1 active observation (medium, no specific line):
2 previous warnings fixed in this update:
Files Reviewed (2 files)
Reviewed by step-3.7-flash-20260528 · 238,252 tokens |
|
The 2 issues from the bot review have been addressed in commit 63b0f80:
Tests pass: ✅ combo-consecutive-failures-3145 (2/2), combo-provider-exhaustion (2/2), pipeline-combo (10/10), combo-routing-e2e (9/9), account-fallback-service (57/57) |
|
@diegosouzapw The 2 bot review issues are fixed in commit 63b0f80 — quality failures now increment consecutiveProviderFailures (both handleComboChat + handleRoundRobinCombo) and RR success resets the counter. Requesting re-review. |
|
Thanks for tackling the #3200 cascade, @herjarsa — and for including an integration test. I did a careful re-audit before merging and hit a blocker: Your own test passes against current Separately, loosening the provider-circuit-breaker trip from Leaving the PR open for your reply — if you can show a concrete scenario the connection cooldown doesn't already cover (e.g. same single account, immediate retry with no cooldown set), I'll happily revisit. 🙏 |
Closing — analysis summary (the fix is redundant with an already-shipped mechanism)Thanks again for the effort here, @herjarsa. After a careful re-audit I'm closing this, and I want to leave the full reasoning on record so it's clear why (and so you can reopen with a counter-case if I'm wrong): 1. The fix's own test passes without the fixI copied your 2. Why it already works: connection cooldownThe same-provider 429 cascade is already short-circuited by the per-account connection cooldown ( 3. The breaker change is a latent riskLoosening the provider-circuit-breaker trip from If I'm wrongIf you have a concrete repro the connection cooldown does not cover — e.g. a single account that returns 429 with no |
|
Closing as redundant with the existing connection-cooldown mechanism (full analysis above). Reopen with a failing-on-release repro if a scenario isn't covered. |
|
El mecanismo de enfriamiento de conexión existente maneja el rate limiting a nivel de credenciales (misma cuenta, mismo modelo), pero NO previene el bucle infinito cuando TODAS las cuentas de un mismo provider están fallando consecutivamente. Escenario reproducible:
Confirmado en producción con múltiples providers:
El connection cooldown NO detecta que es el mismo provider fallando consecutivamente — solo detecta que la misma cuenta+modelo está rate limited. Cuando todas las cuentas del provider están rate limited, el combo runner entra en bucle infinito. La variable El test en |
Problem
When a combo has multiple targets from the same provider and that provider fails (404, 429, 400, 502, 504), the combo keeps retrying ALL targets from the same provider before moving to a different one — creating the appearance of an infinite loop.
Users reported: combos stay stuck on 2 models from the same provider, never falling back to other available options until exhausting all retries.
Root Causes
!sameProviderNextguard onrecordProviderFailure: Prevented the global circuit breaker from ever tripping when multiple targets shared the same providerFix
Applied to both the priority handler (
handleComboChat) and round-robin handler (handleRoundRobinCombo):consecutiveProviderFailuresMap to track per-provider failure count across targetsproviderFailureThreshold), the provider is marked as exhausted and remaining same-provider targets are skipped!sameProviderNextguard to trip the global circuit breaker when the threshold is exceededbreak)Error codes covered
404, 429, 400, 500, 502, 503, 504 — any non-success response increments the counter.
Verification
lsp_diagnostics: cleand1a74c2