diff --git a/open-sse/services/combo.ts b/open-sse/services/combo.ts index f0dc5efc3c..21c0ee8a19 100644 --- a/open-sse/services/combo.ts +++ b/open-sse/services/combo.ts @@ -1996,6 +1996,24 @@ export async function handleComboChat({ strategy, target: toRecordedTarget(target), }); + // LKGP (#919) mirror of the success-path set below: a just-failed target + // must not keep re-pinning itself as the "last known good" choice for the + // *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. + void (async () => { + try { + const { clearLKGP } = await import("../../src/lib/localDb"); + await Promise.all([ + clearLKGP(combo.name, target.executionKey), + clearLKGP(combo.name, combo.id || combo.name), + ]); + } catch (err) { + log.warn("COMBO", "Failed to clear Last Known Good Provider. This is non-fatal.", { + err, + }); + } + })(); recordedAttempts++; lastError = errorText || String(result.status); comboErrors.push({ @@ -3135,6 +3153,22 @@ async function handleRoundRobinCombo({ strategy: "round-robin", target: toRecordedTarget(target), }); + // 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). + void (async () => { + try { + const { clearLKGP } = await import("../../src/lib/localDb"); + await Promise.all([ + clearLKGP(combo.name, target.executionKey), + clearLKGP(combo.name, combo.id || combo.name), + ]); + } catch (err) { + log.warn("COMBO-RR", "Failed to clear Last Known Good Provider. This is non-fatal.", { + err, + }); + } + })(); recordedAttempts++; lastError = errorText || String(result.status); lastStatus = result.status; diff --git a/src/lib/db/settings.ts b/src/lib/db/settings.ts index f8231fcb65..a64d25d2f1 100644 --- a/src/lib/db/settings.ts +++ b/src/lib/db/settings.ts @@ -815,7 +815,7 @@ export { resetAllPricing, } from "./settings/pricing"; -export { type LKGPRecord, getLKGP, setLKGP, clearAllLKGP } from "./settings/lkgp"; +export { type LKGPRecord, getLKGP, setLKGP, clearAllLKGP, clearLKGP } from "./settings/lkgp"; export { type CacheTrendPoint, diff --git a/src/lib/db/settings/lkgp.ts b/src/lib/db/settings/lkgp.ts index df090f4004..dfdd03d07e 100644 --- a/src/lib/db/settings/lkgp.ts +++ b/src/lib/db/settings/lkgp.ts @@ -48,6 +48,25 @@ export function clearAllLKGP(): void { db.prepare("DELETE FROM key_value WHERE namespace = 'lkgp'").run(); } +/** + * Delete one persisted LKGP pin after its target fails. `setLKGP` is only ever + * called on success — nothing previously invalidated a pin once its provider + * started failing, so a *separate* subsequent request kept re-selecting the + * same just-failed provider via `applyStrategyOrdering.ts`'s LKGP reordering + * (live incident: 3 consecutive requests all picked the same timed-out + * opencode-zen/big-pickle target instead of failing over to another combo + * model). Circuit breaker / model lockout deliberately don't react to this + * failure class (request-scoped timeouts, see comboPredicates.ts), so nothing + * else clears the stale pin. + */ +export async function clearLKGP(comboName: string, modelId: string): Promise { + const db = getDbInstance(); + const key = `${comboName}:${modelId}`; + db.prepare("DELETE FROM key_value WHERE namespace = 'lkgp' AND key = ?").run(key); + const { invalidateCachedLKGP } = await import("../readCache"); + invalidateCachedLKGP(key); +} + /** * Delete persisted LKGP pins whose connectionId references a removed provider * connection. Provider-level pins and legacy/unparseable values are preserved. diff --git a/src/lib/localDb.ts b/src/lib/localDb.ts index 22635efee0..184fa744eb 100755 --- a/src/lib/localDb.ts +++ b/src/lib/localDb.ts @@ -144,6 +144,7 @@ export { // LKGP (Last Known Good Provider) (#919) getLKGP, setLKGP, + clearLKGP, // Pricing getPricing, diff --git a/tests/unit/combo-routing-engine.test.ts b/tests/unit/combo-routing-engine.test.ts index e3f71e8052..a610213c58 100644 --- a/tests/unit/combo-routing-engine.test.ts +++ b/tests/unit/combo-routing-engine.test.ts @@ -2540,10 +2540,59 @@ test("handleComboChat standalone lkgp strategy updates LKGP after a successful c } assert.equal(result.ok, true); - // getLKGP now returns LKGPRecord | null — source: src/lib/db/settings.ts getLKGP() assert.equal(persistedProvider?.provider, "openai"); }); +test("handleComboChat standalone lkgp strategy clears LKGP after the last-known-good target fails", async () => { + // A prior successful request pinned "openai" as the last known good provider — + // exactly the state left behind by the previous (success) test's own scenario. + await settingsDb.setLKGP("standalone-lkgp-clear", "standalone-lkgp-clear", "openai"); + + const calls: string[] = []; + const result = await handleComboChat({ + body: {}, + combo: { + id: "standalone-lkgp-clear", + name: "standalone-lkgp-clear", + strategy: "lkgp", + // maxRetries: 0 below means this single target is tried exactly once, + // then the combo loop gives up on it (and on the whole combo, since it's + // the only model) — the exact "Done retrying this model" failure path. + models: ["openai/gpt-4o-mini"], + config: { maxRetries: 0 }, + }, + handleSingleModel: async (_body: Record, modelStr: string) => { + calls.push(modelStr); + return errorResponse(504, "Stream produced no non-ping SSE event within 95000ms"); + }, + isModelAvailable: async () => true, + log: createLog(), + settings: null, + relayOptions: null, + allCombos: null, + }); + + // Give the async fire-and-forget LKGP clear a chance to execute + let persistedProvider: Awaited> = null; + for (let i = 0; i < 20; i++) { + persistedProvider = await settingsDb.getLKGP("standalone-lkgp-clear", "standalone-lkgp-clear"); + if (persistedProvider === null) { + break; + } + await new Promise((resolve) => setTimeout(resolve, 10)); + } + + assert.equal(result.ok, false, "the only target failed, so the whole combo call fails"); + assert.deepEqual(calls, ["openai/gpt-4o-mini"]); + // The bug this guards: without clearing, a *separate* subsequent request would + // keep re-selecting "openai" via LKGP reordering even though it just failed. + assert.equal( + persistedProvider, + null, + "LKGP must be cleared after its target fails, not left pointing at a just-failed provider" + ); +}); + test("handleComboChat auto strategy falls back to the full pool when tool filtering empties candidates", async () => { await settingsDb.updatePricing({ openai: { diff --git a/tests/unit/db-settings-crud.test.ts b/tests/unit/db-settings-crud.test.ts index 1cb6444510..bc5d06aa4e 100644 --- a/tests/unit/db-settings-crud.test.ts +++ b/tests/unit/db-settings-crud.test.ts @@ -234,6 +234,22 @@ test("LKGP overwrites connectionId when updated without one", async () => { assert.deepEqual(record, { provider: "openai" }); }); +test("clearLKGP deletes only the targeted combo/model key", async () => { + await settingsDb.setLKGP("combo-f", "model-f", "openai"); + await settingsDb.setLKGP("combo-f", "model-g", "anthropic"); + + await settingsDb.clearLKGP("combo-f", "model-f"); + + assert.equal(await settingsDb.getLKGP("combo-f", "model-f"), null); + // A sibling key under the same combo must survive. + assert.deepEqual(await settingsDb.getLKGP("combo-f", "model-g"), { provider: "anthropic" }); +}); + +test("clearLKGP on a key with no existing pin does not throw", async () => { + await assert.doesNotReject(() => settingsDb.clearLKGP("combo-never-set", "model-never-set")); + assert.equal(await settingsDb.getLKGP("combo-never-set", "model-never-set"), null); +}); + test("pricing helpers ignore malformed synced data and LKGP falls back to raw values", async () => { const db = core.getDbInstance(); diff --git a/tests/unit/db-settings-split.test.ts b/tests/unit/db-settings-split.test.ts index 88ca1cc203..2b71ccf7f2 100644 --- a/tests/unit/db-settings-split.test.ts +++ b/tests/unit/db-settings-split.test.ts @@ -71,6 +71,7 @@ describe("settings.ts public API surface", () => { "getLKGP", "setLKGP", "clearAllLKGP", + "clearLKGP", // Cache metrics (re-exported from ./settings/cacheMetrics) "getCacheMetrics", "updateCacheMetrics",