diff --git a/changelog.d/fixes/12235-lkgp-clear-scope.md b/changelog.d/fixes/12235-lkgp-clear-scope.md new file mode 100644 index 0000000000..60c8d17f42 --- /dev/null +++ b/changelog.d/fixes/12235-lkgp-clear-scope.md @@ -0,0 +1 @@ +- fix(resilience): only clear the combo-level LKGP pin when it names the target that actually failed, so an unrelated target skip under `auto`/`round-robin` no longer discards a valid pin for a healthy provider (#12235) diff --git a/open-sse/services/combo/attemptLoopTypes.ts b/open-sse/services/combo/attemptLoopTypes.ts index 184e357c99..c60fa50c5e 100644 --- a/open-sse/services/combo/attemptLoopTypes.ts +++ b/open-sse/services/combo/attemptLoopTypes.ts @@ -92,7 +92,10 @@ export type AttemptLoopDeps = { executionKey: string | undefined, comboId: string | undefined, log: ComboLogger, - tag: string + tag: string, + /** Test seam, unused on the routing path; see staleLkgpClear.ts. */ + clearLKGP?: ((comboName: string, modelKey: string) => Promise) | undefined, + failed?: { provider?: string | null; connectionId?: string | null } | null ) => void; /** * Closed-over setup values from handleComboChatInner. Optional so Task 2 diff --git a/open-sse/services/combo/executeTargetAttempt.ts b/open-sse/services/combo/executeTargetAttempt.ts index debe92d2d7..2c0a261421 100644 --- a/open-sse/services/combo/executeTargetAttempt.ts +++ b/open-sse/services/combo/executeTargetAttempt.ts @@ -126,7 +126,15 @@ export async function executeTargetAttempt(opts: { const stopProtectedPriorityTarget = (message: string, cause?: ProtectedPriorityStopCause) => { state.observeFailure(false, target.executionKey); - deps.clearStaleLKGP(deps.combo.name, target.executionKey, deps.combo.id, deps.log, "COMBO"); + deps.clearStaleLKGP( + deps.combo.name, + target.executionKey, + deps.combo.id, + deps.log, + "COMBO", + undefined, + target + ); return protectedPriorityTarget ? { ok: false as const, response: errorResponse(protectedPriorityStopStatus(cause), message) } : null; @@ -906,7 +914,15 @@ export async function executeTargetAttempt(opts: { state.exhaustedConnections.has(`${provider}:${targetWithConnection.connectionId}`) || (provider && state.exhaustedProviders.has(provider)) ) { - deps.clearStaleLKGP(deps.combo.name, target.executionKey, deps.combo.id, deps.log, "COMBO"); + deps.clearStaleLKGP( + deps.combo.name, + target.executionKey, + deps.combo.id, + deps.log, + "COMBO", + undefined, + target + ); } // #2101: Prevent infinite fallback loops with 400 Bad Request errors that are genuinely @@ -948,7 +964,15 @@ export async function executeTargetAttempt(opts: { state.lastStatus = result.status; if (i > 0) state.fallbackCount++; deps.log.warn("COMBO", `Model ${modelStr} failed with body-specific error, stopping combo`); - deps.clearStaleLKGP(deps.combo.name, target.executionKey, deps.combo.id, deps.log, "COMBO"); + deps.clearStaleLKGP( + deps.combo.name, + target.executionKey, + deps.combo.id, + deps.log, + "COMBO", + undefined, + target + ); // #4279: surface the 400 via the {ok,response} contract so the OUTER // target loop resolves the combo and stops. A bare `break` here only // exits the inner retry loop; executeTarget then returns null, which @@ -1137,7 +1161,15 @@ export async function executeTargetAttempt(opts: { // *next* separate request. Circuit breaker / model lockout deliberately // don't react to request-scoped failure classes (see scopedFailure below), // so nothing else clears this stale pin. - deps.clearStaleLKGP(deps.combo.name, target.executionKey, deps.combo.id, deps.log, "COMBO"); + deps.clearStaleLKGP( + deps.combo.name, + target.executionKey, + deps.combo.id, + deps.log, + "COMBO", + undefined, + target + ); state.recordedAttempts++; state.lastError = errorText || String(result.status); state.comboErrors.push({ diff --git a/open-sse/services/combo/executeTargetGates.ts b/open-sse/services/combo/executeTargetGates.ts index 4bf24601bb..ccacd0387f 100644 --- a/open-sse/services/combo/executeTargetGates.ts +++ b/open-sse/services/combo/executeTargetGates.ts @@ -62,7 +62,15 @@ export async function evaluateExecuteTargetGates(opts: { const stopProtectedPriorityTarget = (message: string, cause?: ProtectedPriorityStopCause) => { state.observeFailure(false, target.executionKey); - deps.clearStaleLKGP(deps.combo.name, target.executionKey, deps.combo.id, deps.log, "COMBO"); + deps.clearStaleLKGP( + deps.combo.name, + target.executionKey, + deps.combo.id, + deps.log, + "COMBO", + undefined, + target + ); return protectedPriorityTarget ? { ok: false as const, response: errorResponse(protectedPriorityStopStatus(cause), message) } : null; @@ -156,7 +164,15 @@ export async function evaluateExecuteTargetGates(opts: { decision: "skipped_before_dispatch", reason: "persisted_cooldown", }); - deps.clearStaleLKGP(deps.combo.name, target.executionKey, deps.combo.id, deps.log, "COMBO"); + deps.clearStaleLKGP( + deps.combo.name, + target.executionKey, + deps.combo.id, + deps.log, + "COMBO", + undefined, + target + ); bumpFallback(); return { kind: "skip", result: null }; } @@ -212,7 +228,15 @@ export async function evaluateExecuteTargetGates(opts: { "COMBO", `Skipping ${modelStr} — quota exhaustion cutoff (${quotaCutoff.reason || "quota_exhausted"})` ); - deps.clearStaleLKGP(deps.combo.name, target.executionKey, deps.combo.id, deps.log, "COMBO"); + deps.clearStaleLKGP( + deps.combo.name, + target.executionKey, + deps.combo.id, + deps.log, + "COMBO", + undefined, + target + ); recordComboDecision(deps.traceInvocationId, { step: target.executionKey, target: modelStr, @@ -249,7 +273,15 @@ export async function evaluateExecuteTargetGates(opts: { "COMBO", `Skipping ${modelStr} — quota budget ${quotaDecision.reason} (remaining ${quotaDecision.tokensRemaining ?? 0}, cost ${quotaDecision.estimatedCost ?? 0})` ); - deps.clearStaleLKGP(deps.combo.name, target.executionKey, deps.combo.id, deps.log, "COMBO"); + deps.clearStaleLKGP( + deps.combo.name, + target.executionKey, + deps.combo.id, + deps.log, + "COMBO", + undefined, + target + ); bumpFallback(); return { kind: "skip", result: null }; } @@ -262,7 +294,15 @@ export async function evaluateExecuteTargetGates(opts: { "COMBO", `Skipping ${modelStr} — no credentials available or model excluded` ); - deps.clearStaleLKGP(deps.combo.name, target.executionKey, deps.combo.id, deps.log, "COMBO"); + deps.clearStaleLKGP( + deps.combo.name, + target.executionKey, + deps.combo.id, + deps.log, + "COMBO", + undefined, + target + ); recordComboDecision(deps.traceInvocationId, { step: target.executionKey, target: modelStr, diff --git a/open-sse/services/combo/roundRobinCombo.ts b/open-sse/services/combo/roundRobinCombo.ts index 3b728188a9..6f9219fb28 100644 --- a/open-sse/services/combo/roundRobinCombo.ts +++ b/open-sse/services/combo/roundRobinCombo.ts @@ -503,7 +503,15 @@ export async function handleRoundRobinCombo({ "COMBO-RR", `Skipping ${modelStr} — no credentials available or model excluded` ); - clearStaleLKGP(combo.name, target.executionKey, combo.id, log, "COMBO-RR"); + clearStaleLKGP( + combo.name, + target.executionKey, + combo.id, + log, + "COMBO-RR", + undefined, + target + ); if (offset > 0) fallbackCount++; continue; } @@ -519,7 +527,15 @@ export async function handleRoundRobinCombo({ ) ) { log.info("COMBO-RR", `Skipping ${modelStr} — provider ${provider} in global cooldown`); - clearStaleLKGP(combo.name, target.executionKey, combo.id, log, "COMBO-RR"); + clearStaleLKGP( + combo.name, + target.executionKey, + combo.id, + log, + "COMBO-RR", + undefined, + target + ); if (offset > 0) fallbackCount++; continue; } @@ -532,7 +548,15 @@ export async function handleRoundRobinCombo({ ); if (exhaustedSkip) { log.info("COMBO-RR", exhaustedSkip); - clearStaleLKGP(combo.name, target.executionKey, combo.id, log, "COMBO-RR"); + clearStaleLKGP( + combo.name, + target.executionKey, + combo.id, + log, + "COMBO-RR", + undefined, + target + ); if (offset > 0) fallbackCount++; continue; } @@ -983,7 +1007,15 @@ export async function handleRoundRobinCombo({ exhaustedConnections.has(`${provider}:${targetWithConnection.connectionId}`) || (provider && exhaustedProviders.has(provider)) ) { - clearStaleLKGP(combo.name, target.executionKey, combo.id, log, "COMBO-RR"); + clearStaleLKGP( + combo.name, + target.executionKey, + combo.id, + log, + "COMBO-RR", + undefined, + target + ); } // Transient errors → mark in semaphore so round-robin stops stampeding this target. @@ -1041,7 +1073,15 @@ export async function handleRoundRobinCombo({ // LKGP (#919) mirror of handleComboChat's failure-path clear above — see // that comment for why this must happen (nothing else clears a pin left // by a request-scoped failure class like a stream-readiness timeout). - clearStaleLKGP(combo.name, target.executionKey, combo.id, log, "COMBO-RR"); + clearStaleLKGP( + combo.name, + target.executionKey, + combo.id, + log, + "COMBO-RR", + undefined, + target + ); recordedAttempts++; lastError = errorText || String(result.status); lastStatus = result.status; diff --git a/open-sse/services/combo/staleLkgpClear.ts b/open-sse/services/combo/staleLkgpClear.ts index 5d9824b4a9..cfdcf5c97d 100644 --- a/open-sse/services/combo/staleLkgpClear.ts +++ b/open-sse/services/combo/staleLkgpClear.ts @@ -1,6 +1,7 @@ /** * Clear persisted LKGP pins when a combo target fails or is skipped for exhaustion, - * cooldown or unavailability (#11911 #919). + * cooldown or unavailability (#11911 #919), scoped to the pin that names the + * failed target when the caller knows it (#12235). * * Non-blocking by design: the fallback loop never waits on these SQLite writes. A * failed clear is not silent — it logs a warning carrying the combo and the @@ -13,15 +14,37 @@ type WarnLogger = { warn?: (tag: string, msg: string, data?: unknown) => void } | null; type ClearLkgp = (comboName: string, modelKey: string) => Promise; +/** The target whose failure triggered the clear, when the caller has one in scope. */ +type FailedTarget = { provider?: string | null; connectionId?: string | null } | null; + async function clearPins( comboName: string, executionKey: string | null | undefined, comboId: string | null | undefined, - clearLKGP: ClearLkgp | undefined + clearLKGP: ClearLkgp | undefined, + failed: FailedTarget ): Promise { const clear = clearLKGP ?? (await import("@/lib/db/settings")).clearLKGP; - const keys = [comboId || comboName, ...(executionKey ? [executionKey] : [])]; - await Promise.all(keys.map((key) => clear(comboName, key))); + const comboKey = comboId || comboName; + + // The target-scoped pin is unambiguously about the target that just failed. + const pending: Promise[] = executionKey ? [clear(comboName, executionKey)] : []; + + if (!failed?.provider) { + // No target in scope: previous unconditional behaviour. + pending.push(clear(comboName, comboKey)); + } else { + const { getLKGP } = await import("@/lib/db/settings"); + const pin = await getLKGP(comboName, comboKey); + // Same provider, and — when both sides carry one — the same connection. + // A sibling connection failing does not make the pinned one stale. + const namesFailedTarget = + pin?.provider === failed.provider && + (!pin?.connectionId || !failed.connectionId || pin.connectionId === failed.connectionId); + if (namesFailedTarget) pending.push(clear(comboName, comboKey)); + } + + await Promise.all(pending); } export function clearStaleLKGP( @@ -31,14 +54,27 @@ export function clearStaleLKGP( log?: WarnLogger, tag: string = "COMBO", /** Test seam; the routing path always resolves clearLKGP from @/lib/db/settings. */ - clearLKGP?: ClearLkgp + clearLKGP?: ClearLkgp, + /** + * The failed target, when the caller has one. Scopes the COMBO-LEVEL pin so it + * is cleared only when it actually names that target's provider: the pin + * records whichever provider last SUCCEEDED, which need not be the one failing + * now. Under `auto` the pin is a scoring input rather than a hoist + * (`resolveAutoStrategy` reads it into `lastKnownGoodProvider`), so the pinned + * provider is not necessarily tried first, and clearing unconditionally + * discarded a preference for a healthy provider every time an unrelated target + * was skipped. Omitted keeps the previous unconditional behaviour (#12235). + */ + failed?: FailedTarget ): Promise { - return clearPins(comboName, executionKey, comboId, clearLKGP).catch((err: unknown) => { - log?.warn?.(tag, "Failed to clear Last Known Good Provider. This is non-fatal.", { - combo: comboName, - comboId: comboId ?? null, - executionKey: executionKey ?? null, - err, - }); - }); + return clearPins(comboName, executionKey, comboId, clearLKGP, failed ?? null).catch( + (err: unknown) => { + log?.warn?.(tag, "Failed to clear Last Known Good Provider. This is non-fatal.", { + combo: comboName, + comboId: comboId ?? null, + executionKey: executionKey ?? null, + err, + }); + } + ); } diff --git a/tests/unit/lkgp-stale-pin-exhaustion-11911.test.ts b/tests/unit/lkgp-stale-pin-exhaustion-11911.test.ts index 25a2e1fcb5..9e40e94e4e 100644 --- a/tests/unit/lkgp-stale-pin-exhaustion-11911.test.ts +++ b/tests/unit/lkgp-stale-pin-exhaustion-11911.test.ts @@ -16,11 +16,13 @@ const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-lkgp-stal process.env.DATA_DIR = TEST_DATA_DIR; const { handleComboChat } = await import("../../open-sse/services/combo.ts"); +const { clearStaleLKGP } = await import("../../open-sse/services/combo.ts"); const settingsDb = await import("../../src/lib/db/settings.ts"); const core = await import("../../src/lib/db/core.ts"); const { resetAllComboMetrics } = await import("../../open-sse/services/comboMetrics.ts"); const { resetAllCircuitBreakers } = await import("../../src/shared/utils/circuitBreaker.ts"); -const { resetAll: resetAllSemaphores } = await import("../../open-sse/services/rateLimitSemaphore.ts"); +const { resetAll: resetAllSemaphores } = + await import("../../open-sse/services/rateLimitSemaphore.ts"); after(() => { core.resetDbInstance(); @@ -163,3 +165,70 @@ test("#11911: handleComboChat (round-robin) clears LKGP pin when target is skipp const pinAfter = await settingsDb.getLKGP(comboName, comboName); assert.equal(pinAfter, null, "stale LKGP pin in round-robin must be cleared on unavailable skip"); }); + +test("#11911 follow-up: a pin naming a healthy provider survives another target being skipped", async () => { + // The #11911 fix clears the combo-level pin from 12 call sites, none of which look at + // which provider the pin actually names. Under `auto` the pin is a scoring input rather + // than a hoist (resolveAutoStrategy reads it into lastKnownGoodProvider), so the pinned + // provider is not necessarily tried first — and skipping an unrelated target destroys a + // preference for a provider that never failed. + const comboName = "auto-cross-provider-pin"; + await settingsDb.setLKGP(comboName, comboName, "felo", undefined); + + const result = await handleComboChat({ + body: { messages: [{ role: "user", content: "hi" }] }, + combo: { + name: comboName, + strategy: "auto", + models: ["opencode/deepseek-free", "felo/felo-flash"], + config: { maxRetries: 0 }, + }, + handleSingleModel: async (_body, targetModel) => + targetModel.includes("felo") + ? jsonResponse(200, { ok: true }) + : jsonResponse(502, { error: { message: "opencode down" } }), + isModelAvailable: async (modelStr) => !modelStr.includes("opencode"), + log: createLog(), + settings: null, + relayOptions: null, + allCombos: null, + }); + + assert.equal(result.status, 200); + const pinAfter = await settingsDb.getLKGP(comboName, comboName); + assert.deepEqual( + pinAfter, + { provider: "felo" }, + "skipping opencode must not clear a pin naming healthy felo" + ); +}); + +test("#12235: a sibling connection failing does not clear a pin naming the same provider", async () => { + // The combo pin carries a connectionId as well as a provider. Two connections + // of the SAME provider are independent targets: one going down says nothing + // about the other, so matching on provider alone would throw away a pin for a + // connection that never failed. + const comboName = "sibling-connection-pin"; + await settingsDb.setLKGP(comboName, comboName, "felo", "conn-A"); + + await clearStaleLKGP(comboName, null, comboName, null, "COMBO", undefined, { + provider: "felo", + connectionId: "conn-B", + }); + assert.deepEqual( + await settingsDb.getLKGP(comboName, comboName), + { provider: "felo", connectionId: "conn-A" }, + "conn-B failing must not clear a pin naming conn-A" + ); + + // ...and the pin IS cleared when the failure names that same connection. + await clearStaleLKGP(comboName, null, comboName, null, "COMBO", undefined, { + provider: "felo", + connectionId: "conn-A", + }); + assert.equal( + await settingsDb.getLKGP(comboName, comboName), + null, + "conn-A failing must clear the pin that names it" + ); +});