diff --git a/open-sse/services/combo.ts b/open-sse/services/combo.ts index afd4ca63ad..58371dc93e 100644 --- a/open-sse/services/combo.ts +++ b/open-sse/services/combo.ts @@ -1908,7 +1908,23 @@ export async function handleComboChat({ !isTokenLimitBreach && !scopedFailure && [408, 429, 500, 502, 503, 504].includes(result.status); - if (retry < maxRetries && isTransient && !providerExhausted) { + // failoverBeforeRetry means what it says: prefer the next sibling + // target over hammering this one again. Without this check, a + // transient error always re-hit the SAME model up to maxRetries + // times regardless of the setting — config.failoverBeforeRetry was + // threaded through to skipUpstreamRetry (a different, lower-level + // retry mechanism) but never consulted here, so a rate-limited + // model got maxRetries+1 back-to-back attempts on itself before + // this loop's own fallback-to-next-target ever ran (#2417). Only + // skip the same-model retry when `nextTarget` (computed above) + // actually gives us somewhere to fail over to — with no sibling + // left, skipping just burns the last attempt for nothing. + if ( + retry < maxRetries && + isTransient && + !providerExhausted && + (!config.failoverBeforeRetry || !nextTarget) + ) { if ( !protectedPriorityTarget && provider && @@ -3142,7 +3158,18 @@ async function handleRoundRobinCombo({ !isTokenLimitBreach && !scopedFailure && [408, 429, 500, 502, 503, 504].includes(result.status); - if (retry < maxRetries && isTransient && !providerExhausted) { + // See the same guard's comment in the "auto" strategy loop above — + // failoverBeforeRetry must prevent this same-model retry too, not + // just the lower-level skipUpstreamRetry mechanism. Only skip when + // `offset + 1 < modelCount` means a sibling target is actually left + // in this rotation; with none left, skipping just wastes the attempt. + const hasNextRrTarget = offset + 1 < modelCount; + if ( + retry < maxRetries && + isTransient && + !providerExhausted && + (!config.failoverBeforeRetry || !hasNextRrTarget) + ) { continue; } diff --git a/tests/unit/combo-routing-engine.test.ts b/tests/unit/combo-routing-engine.test.ts index a610213c58..5997fc83a5 100644 --- a/tests/unit/combo-routing-engine.test.ts +++ b/tests/unit/combo-routing-engine.test.ts @@ -2956,6 +2956,69 @@ test("handleComboChat round-robin retries a transient failure on the same model assert.deepEqual(calls, ["model-a", "model-a"]); }); +test("handleComboChat round-robin: failoverBeforeRetry skips the same-model retry and goes straight to the sibling", async () => { + // #2417's whole point: failoverBeforeRetry should prefer a sibling model + // over hammering a rate-limited one again. Same shape as the test above + // (maxRetries: 1, a transient 429 on the first call) but with a second + // model available and failoverBeforeRetry set — calls must show a single + // model-a attempt followed directly by model-b, never a same-model retry. + const calls = []; + + const result = await handleComboChat({ + body: {}, + combo: { + name: "rr-failover-before-retry", + strategy: "round-robin", + models: ["model-a", "model-b"], + config: { + maxRetries: 1, + retryDelayMs: 1, + failoverBeforeRetry: true, + concurrencyPerModel: 1, + queueTimeoutMs: 5, + }, + }, + handleSingleModel: async (_body, modelStr) => { + calls.push(modelStr); + if (modelStr === "model-a") return errorResponse(429, "rate limited"); + return okResponse(); + }, + isModelAvailable: async () => true, + log: createLog(), + settings: null, + relayOptions: null, + allCombos: null, + }); + + assert.equal(result.ok, true); + assert.deepEqual(calls, ["model-a", "model-b"]); +}); + +test("handleComboChat priority strategy: failoverBeforeRetry skips the same-model retry and goes straight to the sibling", async () => { + const calls = []; + + const result = await handleComboChat({ + body: {}, + combo: { + name: "priority-failover-before-retry", + models: ["model-a", "model-b"], + config: { maxRetries: 1, retryDelayMs: 1, failoverBeforeRetry: true }, + }, + handleSingleModel: async (_body, modelStr) => { + calls.push(modelStr); + if (modelStr === "model-a") return errorResponse(429, "rate limited"); + return okResponse(); + }, + isModelAvailable: async () => true, + log: createLog(), + settings: null, + allCombos: null, + }); + + assert.equal(result.ok, true); + assert.deepEqual(calls, ["model-a", "model-b"]); +}); + test("handleComboChat round-robin recovers from 400s when a later model succeeds", async () => { const calls: any[] = [];