[opensource-elearning]

What

When a combo target is marked exhausted via applyComboTargetExhaustion — either the whole provider (providerExhausted) or the specific connection (exhaustedConnections keyed `${provider}:${connId}`) — the Last Known Good Provider (LKGP) pin was never cleared.

LKGP persists the "last good" provider per combo and re-selects it first on subsequent requests. So after a dead connection exhausts, the next request re-pins to that same dead provider/connection, fails again, and the whole combo repeatedly spends its budget on a target that can never succeed — instead of falling through to healthy targets.

Symptom

Repeated failures on a free-tier connection with no API key (e.g. opencode noauth) that 502s/stalls on every request: every request re-selects it as LKGP, exhausts it, then cascades to other targets (which may 400/429), producing multi-API error swaths and long delays.

Fix

At both exhaustion classification sites — handleComboChat and handleRoundRobinCombo — the duplicated inline clearing block is extracted into clearLKGPOnExhaustion (open-sse/services/combo/lkgpClearing.ts) and called fire-and-forget.

Clearing is now safe and gated, per review (#11914):

• the executionKey pin always names the target that just failed — always cleared;
• the combo-level pin (the only one read by applyStrategyOrdering / resolveAutoStrategy on the next request) records whichever target last succeeded, which needn't be the one that just failed. It's cleared only when it names the exhausted provider and, when both carry a connectionId, the exhausted connection. A preference pointing at a healthy target survives.
• the guard is connId ? exhaustedConnections.has(...) : providerExhausted, so no "provider:undefined" interpolation (previously load-bearing on a short-circuit).

Non-fatal on failure (logged at warn).

Validation

• New tests/unit/combo/combo-lkgp-clear.test.ts: 9/9 pass (covers provider-level, connection-level, healthy-sibling survived, healthy-provider survived, 401-connection-scoped, legacy provider-only pin, no-op and guard cases). Verified load-bearing: restoring the old unconditional-clear makes exactly 3 tests fail.
• tests/unit/combo/combo-target-exhaustion.test.ts + combo-exhausted-skip.test.ts: 45/45 pass.
• ESLint clean on both new files (combo.ts unused-var errors are pre-existing and suppressed on the base).
• Typecheck clean for the changed files (only the pre-existing executeAttempt.ts error remains on the base).

⚠️ base-red inherited: #11874

Closes #11911

[abhisheksharma2411]

Nice catch on this one — the diagnosis in #11911 is exactly right, and it's the kind of stale-state bug that's easy to stare past. setLKGP is only ever called on success, so nothing invalidated a pin once its target started failing; the exhaustion sets path was the obvious hole. Verified the mechanics against release/v3.8.51: providerExhausted is the return of applyComboTargetExhaustion and the sets mutate in place, so your check does see the state that call just wrote — no ordering trap there. The ${provider}:${connId} key matches how targetExhaustion.ts builds it at all three add() sites, and the pattern mirrors the existing 2426/3806 call sites faithfully.

Two things I'd want to talk through before this lands.

1. No test in the PR. AGENTS.md Hard Rule 18 asks for a failing-then-passing test on every bug fix, and the PR template has the matching checkbox. The 45/45 you ran are the pre-existing exhaustion tests — they pass identically with and without your change, so nothing in the suite currently fails if this fix is reverted. Worth adding one that pins the actual behaviour: seed an LKGP pin, drive a target into exhaustedConnections, assert the pin is gone. That's also the test that would protect finding #2 below, whichever way you decide it.

2. The combo-level pin gets cleared even when it names a different, healthy provider. This is the one I'd actually push on. Both read sites (applyStrategyOrdering.ts:71, resolveAutoStrategy.ts:249) only ever read the combo-level key ${combo.name}:${combo.id || combo.name} — the executionKey pin is written but never read. So the combo-level pin is the one that matters, and it holds {provider, connectionId} for whichever target last succeeded, which needn't be the one that just failed.

Concretely: combo with targets across providers A and B, pin currently naming B. B gets skipped this request (rate-limited, or filtered by apiKeyAllowedConnections), A is tried instead, A's connection 401s → markAuthLevelExhaustion → your block fires → B's pin is deleted. B was never unhealthy. Next request loses the preference and falls back to default ordering until something succeeds and re-pins.

It's a narrow window — under strategy=lkgp the pinned target normally goes first and succeeds, so it only bites when the pin is skipped rather than tried — and the blast radius is "lost a preference", not a wrong route. But since the whole point of the fix is pin hygiene, it seems worth gating the clear on the pin actually naming the exhausted provider, something like reading the record first and only clearing when record.provider === exhaustedProvider (and, when both carry one, record.connectionId === exhaustedConnId). Happy to be told this is deliberate — the existing 2426/3806 sites clear both keys unconditionally too, so you'd be consistent with them. The difference is frequency: those fire on the terminal "done retrying" path, whereas exhaustion classification fires on any 401/403/408/5xx, so a rare over-clear becomes a common one.

Two small ones, neither blocking:

• exhaustedConnections.has(\${exhaustedProvider}:${exhaustedConnId}`)interpolates the literal string"undefined"when the target has noconnectionId. It's harmless today — no-connectionId failures mark exhaustedProvidersand returntrue, so providerExhaustedshort-circuits before the.has()` is reached — but it's load-bearing on that short-circuit in a way that isn't obvious from reading the line.
• The errorResponseWithComboDiagnostics block around 3328 is a pure prettier reformat unrelated to the fix. Not a problem, just makes the diff read bigger than the change is — might be worth splitting out if you touch this again.

For what it's worth I went looking at #11911 myself and found your PR already open against it, which is why I'm here reviewing instead of duplicating you. Glad the cross-reference was there.

[opensource-elearning]

Thanks for the thorough review — both blocking points are addressed and pushed to this branch (42964d433).

1. Test added (Hard Rule 18). New file tests/unit/combo/combo-lkgp-clear.test.ts — 9 tests, all covering the clearing rules directly:

• provider-level exhaustion clears both pins;
• connection-level exhaustion clears both when they name the exact connection;
• combo-level pin naming a different, healthy provider is kept (your scenario: B skipped, A tried and exhausts → B's preference survives);
• combo-level pin naming a sibling connection is kept;
• provider-level exhaustion clears a connection-scoped pin for that provider;
• no exhaustion → nothing cleared;
• unknown provider not cleared;
• connection-scoped 401 (providerExhausted true + conn in set) clears only when the pin names that exact provider:connection;
• legacy provider-only pin cleared.

I verified the tests are load-bearing: temporarily restoring the old unconditional-clear behavior makes exactly 3 of them fail (the healthy-provider / sibling / 401-sibling cases), and restoring the fix makes 9/9 pass. They run on the real DB through setLKGP/getLKGP, so they exercise the write and read the next request sees.

2. Combo-level clear now gated on the pin naming the exhausted provider/connection. The fix does exactly the gating you sketched — read the record first, clear only when pinned.provider === exhaustedProvider && (no connId || pinned.connectionId == null || pinned.connectionId === exhaustedConnId). The executionKey pin is still cleared unconditionally, since it always names the target that just failed; the combo-level pin — the only one either read site touches — is the one that's guarded. The two inline blocks were identical, so I extracted them into open-sse/services/combo/lkgpClearing.ts::clearLKGPOnExhaustion (shared by handleComboChat + handleRoundRobinCombo), which is what made the gating unit-testable in the first place.

Along the way your first non-blocking note is fixed by construction: the guard is now connId ? exhaustedConnections.has(\${provider}:${connId}`) : providerExhausted, so there's no "provider:undefined"/"provider:null"` interpolation at all.

On the errorResponseWithComboDiagnostics prettier reformat — that landed in the base branch independently; this branch's diff doesn't touch that region, so nothing to split out here.

Same-request skip logic (applyComboTargetExhaustion) is untouched — this only clears the persisted pin for the next request, as before.

[diegosouzapw]

Obrigado pelo diagnóstico e pela correção — o root cause está certo. Fechando como subsumida: o mesmo bug (#11911) já foi corrigido e mergeado nesta sessão via #12013 ("clear persisted LKGP pin on target exhaustion and skip"), que já cobre exatamente os dois pontos de inserção desta PR.

Comparando diretamente no open-sse/services/combo.ts do tip atual:

if (
  providerExhausted ||
  exhaustedConnections.has(`${provider}:${targetWithConnection.connectionId}`) ||
  (provider && exhaustedProviders.has(provider))
) {
  clearStaleLKGP(combo.name, target.executionKey, combo.id, log, "COMBO");
}

— presente tanto em handleComboChatInner quanto em handleRoundRobinCombo, com a mesma condição de exhaustion que esta PR propõe (providerExhausted/exhaustedConnections), e o helper já existente (clearStaleLKGP) já limpa tanto o pin de executionKey quanto o pin combo-level (combo.id || combo.name) — coberto em 13 pontos de falha no total, não só os dois de exhaustion. Nenhuma ação adicional necessária.