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

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