fix(resilience): clear the combo LKGP pin only when it names the failed target (#12235)

* fix(resilience): clear the combo LKGP pin only when it names the failed target

Re-applied onto current release/v3.8.51. The branch was 217 commits behind
and dispatchWithCooldownRetry / handleRoundRobinCombo have since moved out
of combo.ts, so this is a re-application, not a rebase: the definition is
still in combo.ts, and the 14 call sites now live in
combo/executeTargetAttempt.ts (4), combo/executeTargetGates.ts (5) and
combo/roundRobinCombo.ts (5). clearStaleLKGP is dependency-injected via
attemptLoopTypes.ts, so that signature takes the new parameter too.

Unchanged in substance. The target-scoped pin is still always cleared; the
combo-level pin is cleared only when it actually names the failed target's
provider, because it records whichever provider last *succeeded* and that
need not be the one failing now. Under `auto` the pin is a scoring input
rather than a hoist, so clearing unconditionally discarded a preference for
a healthy provider every time an unrelated target was skipped. Omitting
`failed` keeps the old behaviour for callers with no target in scope.

  4 regression tests pass, including "a pin naming a healthy provider
  survives another target being skipped"

  mutation: clear the combo pin unconditionally (the pre-fix behaviour)
    -> only that test fails, 3 pass

  248 tests pass across tests/unit/lkgp*, tests/unit/combo/*
  eslint clean on all five changed files (exit 0, no suppressions flag)

* docs(changelog): add fragment for the LKGP pin scope fix

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

---------

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Co-authored-by: abhisheksharma2411 <abhisheksharma2411@users.noreply.github.com>

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)

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<void>) | undefined,
+    failed?: { provider?: string | null; connectionId?: string | null } | null
   ) => void;
   /**
    * Closed-over setup values from handleComboChatInner. Optional so Task 2

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({

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,

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;

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<void>;
 
+/** 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<void> {
   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<void>[] = 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<void> {
-  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,
+      });
+    }
+  );
 }

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"
+  );
+});