From 104afeda4ec49d9a6bf38484d2210804cd8118ce Mon Sep 17 00:00:00 2001 From: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Date: Thu, 13 Aug 2026 14:19:34 -0300 Subject: [PATCH] fix(combo): decouple failoverBeforeRetry same-model guard from the skipUpstreamRetry default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit found that DEFAULT_COMBO_CONFIG.failoverBeforeRetry has defaulted to true since before #10217 (predates #2417), and that value also feeds the independent skipUpstreamRetry mechanism (src/sse/handlers/chat.ts:859,1126). The previous commit on this branch flipped that default to false to fix the #10217 same-model retry guard, which silently disabled skipUpstreamRetry's own default-on behavior for every combo without an opt-in — a regression in the opposite direction (executor-level retries before the loop's own failover, changing latency/failure behavior). Revert the default back to true and decouple the two mechanisms instead: resolveComboConfig/resolveComboSetupConfig now also compute failoverBeforeRetryExplicit, true only when a cascade layer (combo/provider/ global) literally sets failoverBeforeRetry to true — not merely inherited from the default. The #10217 same-model retry guards in combo.ts (priority/ auto and round-robin loops) now read failoverBeforeRetryExplicit instead of config.failoverBeforeRetry, restoring opt-in-only behavior for that guard while the skipUpstreamRetry pass-through (config.failoverBeforeRetry at combo.ts:1297,2865) is untouched and keeps its historical default-on. --- open-sse/services/combo.ts | 23 +++++++++++-- open-sse/services/comboConfig.ts | 59 +++++++++++++++++++++++++------- 2 files changed, 67 insertions(+), 15 deletions(-) diff --git a/open-sse/services/combo.ts b/open-sse/services/combo.ts index 58371dc93e..df86a79375 100644 --- a/open-sse/services/combo.ts +++ b/open-sse/services/combo.ts @@ -1919,11 +1919,18 @@ export async function handleComboChat({ // 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. + // + // #10217 round-4 fix: this guard reads `failoverBeforeRetryExplicit` + // (opt-in only), NOT `config.failoverBeforeRetry` — that field + // defaults to true for the separate skipUpstreamRetry mechanism + // (see DEFAULT_COMBO_CONFIG comment in comboConfig.ts) and reading + // it here would silently skip the same-model retry for every combo, + // not just ones that explicitly opted in. if ( retry < maxRetries && isTransient && !providerExhausted && - (!config.failoverBeforeRetry || !nextTarget) + (!config.failoverBeforeRetryExplicit || !nextTarget) ) { if ( !protectedPriorityTarget && @@ -2438,7 +2445,15 @@ async function handleRoundRobinCombo({ }: HandleRoundRobinOptions): Promise { const config = settings ? resolveComboConfig(combo, settings) - : { ...getDefaultComboConfig(), ...(combo.config || {}) }; + : { + ...getDefaultComboConfig(), + ...(combo.config || {}), + // See resolveComboConfig's failoverBeforeRetryExplicit comment in + // comboConfig.ts (no `settings` here, so only the combo's own config + // can opt in). + failoverBeforeRetryExplicit: + (combo.config as Record | undefined)?.failoverBeforeRetry === true, + }; // #9158: clamp combo-level concurrency to a sane bound — a config carrying a // huge or negative value would otherwise open an unbounded semaphore and // flood targets (or deadlock at 0). @@ -3163,12 +3178,14 @@ async function handleRoundRobinCombo({ // 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. + // #10217 round-4 fix: opt-in only — read failoverBeforeRetryExplicit, + // not config.failoverBeforeRetry (see comboConfig.ts comment). const hasNextRrTarget = offset + 1 < modelCount; if ( retry < maxRetries && isTransient && !providerExhausted && - (!config.failoverBeforeRetry || !hasNextRrTarget) + (!config.failoverBeforeRetryExplicit || !hasNextRrTarget) ) { continue; } diff --git a/open-sse/services/comboConfig.ts b/open-sse/services/comboConfig.ts index dd7f220160..97e753ad92 100644 --- a/open-sse/services/comboConfig.ts +++ b/open-sse/services/comboConfig.ts @@ -126,12 +126,23 @@ const DEFAULT_COMBO_CONFIG = { resetAwareWeeklyWeight: 0.65, resetAwareTieBandPercent: 5, resetAwareExhaustionGuardPercent: 10, - // Opt-in (#2417/#10217): when unset, transient errors retry the same model - // up to maxRetries before falling over to the next target — the historical - // default. Defaulting this to true silently flipped that behavior for every - // combo that never touched the setting, breaking same-model retry semantics - // (round 4 base-red bisect: 06f41cda63 vs d2fd88dfbc). - failoverBeforeRetry: false, + // Historical default (predates #2417/#10217) — true. This value feeds TWO + // independent mechanisms and must stay true-by-default for one of them: + // 1. skipUpstreamRetry (src/sse/handlers/chat.ts:859,1126) — the + // lower-level executor retry skip. Always default-on; changing this + // default flips that mechanism's behavior for every combo, not just + // opted-in ones. + // 2. The #10217 same-model retry guard in this file's combo.ts callers + // (priority/auto + round-robin loops) — meant to be OPT-IN only. That + // guard must NOT read this field directly; it consults the sibling + // `failoverBeforeRetryExplicit` flag computed below in + // resolveComboConfig/resolveComboSetupConfig, which is true only when + // an actual cascade layer (combo/provider/global) set the flag to + // true, not merely inherited from this default. See round-4 base-red + // bisect (06f41cda63 vs d2fd88dfbc) — flipping THIS default to false + // "fixed" mechanism 2 but silently broke mechanism 1's default-on + // behavior for every combo without an explicit opt-in. + failoverBeforeRetry: true, // Feature 4985: configurable response-body validation predicate (per-combo). When set, // a 200 OK whose body fails the predicate fails over to the next target. responseValidation: undefined as ResponseValidationConfig | undefined, @@ -289,15 +300,32 @@ export function resolveComboConfig( ) ); + const cleanGlobal = clean(global); + const cleanProviderOverride = clean(providerOverride); + const cleanComboConfig = clean(comboConfig); + const merged = { ...DEFAULT_COMBO_CONFIG, - ...clean(global), - ...clean(providerOverride), - ...clean(comboConfig), + ...cleanGlobal, + ...cleanProviderOverride, + ...cleanComboConfig, }; + // #10217 round-4 fix: `failoverBeforeRetry` defaults to true (see comment on + // DEFAULT_COMBO_CONFIG above) and feeds two independent mechanisms. Callers + // that gate the OPT-IN same-model retry guard (combo.ts) must NOT read + // `merged.failoverBeforeRetry` directly — that stays true unless a layer + // explicitly disables it, which can't distinguish "inherited default" from + // "operator opted in". This flag is true only when some cascade layer + // literally set the value to true, i.e. a genuine opt-in. + const failoverBeforeRetryExplicit = + cleanComboConfig.failoverBeforeRetry === true || + cleanProviderOverride.failoverBeforeRetry === true || + cleanGlobal.failoverBeforeRetry === true; + return { ...merged, + failoverBeforeRetryExplicit, shadowRouting: { ...DEFAULT_COMBO_CONFIG.shadowRouting, ...(isRecord(global.shadowRouting) ? clean(global.shadowRouting) : {}), @@ -327,7 +355,14 @@ export function getDefaultComboConfig() { * return type is the single source of truth for ComboContext.config (combo/context.ts). */ export function resolveComboSetupConfig(combo: ComboConfigLike, settings: ComboSettingsLike) { - return settings - ? resolveComboConfig(combo, settings) - : { ...getDefaultComboConfig(), ...((combo?.config as Record) || {}) }; + if (settings) return resolveComboConfig(combo, settings); + const comboConfig = (combo?.config as Record) || {}; + return { + ...getDefaultComboConfig(), + ...comboConfig, + // See resolveComboConfig's failoverBeforeRetryExplicit comment — same + // distinction applies here (no `settings`, so only the combo's own config + // can opt in). + failoverBeforeRetryExplicit: comboConfig.failoverBeforeRetry === true, + }; }