From c9daf99e3747c1562992e9cef68c1a4d239b14fe Mon Sep 17 00:00:00 2001 From: Markus Hartung Date: Thu, 13 Aug 2026 09:02:34 +0200 Subject: [PATCH] fix(combo): clear LKGP pin when its target fails, not only set it on success (#10034) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setLKGP() was only ever called on success — nothing invalidated a "last known good provider" pin once that provider started failing, so a *separate* subsequent request kept re-selecting the same just-failed target via applyStrategyOrdering.ts's LKGP reordering. Live incident: an OpenClaw request to combo "default" (routerStrategy: lkgp) got a real reasoning + apply_patch tool call from opencode-zen/big-pickle, then 3 separate follow-up requests over the next ~2 minutes each independently re-selected the same big-pickle target and each timed out with "504 Stream produced no non-ping SSE event within 95000ms" before the client gave up — instead of failing over to any of the combo's other 12 models. Root cause confirmed via code read: circuit breaker and model lockout deliberately don't react to this failure class (isStreamReadinessFailureErrorBody exempts STREAM_READINESS_TIMEOUT/combo_target_timeout 504s from tripping the provider breaker, and REQUEST_SCOPED_UPSTREAM_ERROR_CODES suppresses model-lockout recording for the same class — both intentional, to avoid poisoning a healthy provider on request-specific timing). Nothing else in the system was clearing the stale LKGP pin, so it kept winning target-selection ordering for every new top-level request. Fix: add clearLKGP(comboName, modelId) to src/lib/db/settings/lkgp.ts, export it through settings.ts/localDb.ts, and call it (mirroring the existing setLKGP-on-success call pattern exactly, same two keys) in both combo.ts's per-target failure paths -- handleComboChat's "Done retrying this model" block and handleRoundRobinCombo's structurally identical twin -- right where a target is finally given up on and the loop moves to the next one. TDD: new regression test in tests/unit/combo-routing-engine.test.ts ("clears LKGP after the last-known-good target fails") reproduces the exact live scenario -- confirmed failing against the pre-fix code, passing after. Added direct unit coverage for clearLKGP itself in tests/unit/db-settings-crud.test.ts (deletes only the targeted key, sibling keys survive; no-op on an unset key doesn't throw) and registered the new export in db-settings-split.test.ts's public API surface characterization test. Test plan: - Full combo/LKGP-related suite (combo-routing-engine, db-settings-crud, db-settings-split, combo-strategy-fallbacks, combo-selected-connection-success, delete-provider-connection-invalidates-lkgp-8887, db-read-cache) -- 183/183 passing. - npx tsc --noEmit -- clean for all changed files (pre-existing unrelated errors elsewhere in the same test files confirmed identical against a pristine upstream/release/v3.8.50 checkout, zero diff at those lines). - npm run lint -- clean (new test's any usage properly typed, not left to inflate the file's frozen any-budget suppression). ⚠️ base-red inherited: #9985 --- open-sse/services/combo.ts | 34 +++++++++++++++++ src/lib/db/settings.ts | 2 +- src/lib/db/settings/lkgp.ts | 19 +++++++++ src/lib/localDb.ts | 1 + tests/unit/combo-routing-engine.test.ts | 51 ++++++++++++++++++++++++- tests/unit/db-settings-crud.test.ts | 16 ++++++++ tests/unit/db-settings-split.test.ts | 1 + 7 files changed, 122 insertions(+), 2 deletions(-) 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",