fix(quota-share): release the winner's reserved in-flight slot (#11371)

[oyi77]

Summary

Fixes #11371.

selectQuotaShareTarget reserves an in-flight slot for its winner and returns an idempotent release callback — but no production caller ever invoked it, so counters only grew: P2C degenerated from "least loaded" into "fewest lifetime dispatches" (overriding operator DRR weights) and the FASE 2.1 max_concurrent gate fail-opened permanently past its cap.

This threads the release out of the ordering path (shape 1 from the issue):

• applyStrategyOrdering now returns { orderedTargets, quotaShareRelease } — non-null only when the quota-share strategy ran
• resolveComboTargetPipeline releases it itself when a post-selection hard filter produces an earlyResponse, otherwise hands it to the host
• handleComboChatInner invokes it in the dispatch finally (beside quotaShareConcurrencyRelease) and on the two early returns between resolution and the try block

The callback is idempotent, so exactly-once is enforced at the source and defensive double-calls are harmless.

Tests

• New case the issue called out as missing: ordering through the real applyStrategyOrdering("quota-share", …) path reserves one slot and leaves the counter back at zero after release; double-release floors at 0; non-quota-share strategies return a null release (tests/unit/quota-share-strategy.test.ts)
• Updated the split-guard tests for the new return shape (combo-apply-strategy-ordering-split.test.ts, prompt-cache-affinity.test.ts)

Verification

quota-share-strategy + apply-strategy-ordering-split + prompt-cache-affinity: 45/45 pass
combo-strategy-fallbacks + router-strategies:                                41/41 pass
combo-target-resolution-split:                                                6/6 pass
tsc --noEmit: no errors in changed files

Base: release/v3.8.51 (3192eb88d). No behavior change for non-quota-share strategies.

[abhisheksharma2411]

The diagnosis in #11371 is exactly right and the fix shape is the correct one — threading the release out of applyStrategyOrdering rather than releasing inside it, making it idempotent, and firing it from finally plus both early exits. The interface doc explaining why dropping the callback degenerates P2C into "fewest lifetime dispatches" is the part that will stop this regressing again.

But there is an accidental deletion in this diff, and I think it is a real regression.

recordComboFailure is removed from the no-executable-targets path

   if (orderedTargets.length === 0) {
     // Surface a recovery hint + auto-clear the session pin after enough consecutive
     // no-target failures (silent-stop fix). Threshold of 3 prevents a one-off account
     // wipe from destroying the prompt-cache pin benefit on the next request.
-    recordComboFailure(effectiveSessionId, combo.name);
+    // #11371: same early-exit release as the pinned-turn path above.
+    targetResolution.quotaShareRelease?.();

The release call is correct and belongs there. It looks like it replaced the line instead of being added above it — the comment describing the auto-clear is still sitting on top of code that no longer does it.

Counting call sites in combo.ts:

base:     34 (import), 931, 1415, 2626, 2723, 2755   → 5 calls
#11408:   34 (import),      1419, 2630, 2727, 2759   → 4 calls

The four survivors are all on dispatch-failure paths. Nothing else records a failure for the "Combo has no executable targets" early return.

Why it matters: recordComboFailure is what increments the per-(session, combo) counter and clears the session's model pin once it reaches COMBO_FAILURE_THRESHOLD. With this call gone, a session that keeps hitting "no executable targets" never accumulates a streak, so the pin is never auto-cleared — which is the silent-stop the comment above it was added to fix. A user whose pinned target has gone away stays stuck, and the recovery hint in the 404 becomes the only way out.

And nothing catches it. I ran the combo unit suite on this branch: tests/unit/combo/*.test.ts → 162 tests, 162 pass, 0 fail, same as the base branch. There is a pin-recovery.test.ts, but it covers buildRecoveryHint and payload shape, not the recording call on this path. So the behaviour change is silent in CI as well as in the diff.

The fix is presumably just to keep both:

recordComboFailure(effectiveSessionId, combo.name);
// #11371: same early-exit release as the pinned-turn path above.
targetResolution.quotaShareRelease?.();

If it was deliberate — say you judged that a no-target combo shouldn't count toward the pin-clear streak — then it's a separate behavioural change that deserves its own line in the description and its own test, because it is not implied by #11371.

On the rest

The finally placement looks right: quotaShareConcurrencyRelease?.() and the new quotaShareRelease?.() are different reservations (admission vs the P2C in-flight slot), and calling both is correct rather than redundant. Firing the release again from the early exits is safe because release latches on released, so the double-call from an early-exit path that later also unwinds through finally is a no-op — worth keeping that released flag prominent, since the whole design leans on it.

One question: selectQuotaShareTarget reserves the slot at selection time using the selection-time clock, but the request may sit in cooldown-retry for a while before dispatch. Is the lease long enough that a slow request's slot is never reaped as expired before its own release runs, or is the idempotent release the only thing keeping the count honest there? Not a defect I can point at — I ask because "reserve early, release in finally" is exactly where lease expiry and explicit release tend to disagree under load.

CI on this head currently shows Unit Tests fast-path (2/4) and (4/4) red plus No new ESLint warnings — I did not chase those, since the combo suite is green locally and they may be unrelated to this change.

[oyi77]

CI triage (all four red checks investigated)

Check	Verdict	Evidence
Unit Tests fast-path (2/4)	Flake, not this diff	8529/8533 pass; sole failure ollama-local-embedding-2824.test.ts is an rmSync ENOTEMPTY race in the test's own after() tmp-cleanup hook — a file untouched by this PR. Passes locally.
Unit Tests fast-path (4/4)	Flake, not this diff	8275/8280; sole failure token-health-check-kimi.test.ts (jitter-window assertion) — file untouched by this PR. Passes locally.
No new ESLint warnings	Pre-existing on the base branch	run-eslint-json --max-warnings 0 exits 2 with "suppressions left that do not occur anymore" — reproduced verbatim on a stashed clean release/v3.8.51 checkout. Note the currently-green PRs (#11428/#11429/#11434) all target release/v3.8.50, whose suppression state differs.
dast-smoke	Does not reproduce from this diff	The reported 500s are DELETE /api/keys/{id}, OPTIONS /api/keys, OPTIONS /api/auth/logout — probed against this branch running locally: 204 / 401 / 204 (all documented statuses). The fuzz 500s are unrelated to combo dispatch and appear environmental to the CI harness run.

No code changes required for this PR; happy to follow up if maintainers see otherwise.

[diegosouzapw]

Obrigado pela contribuição — o fix de vazamento do contador in-flight do quota-share (#11371) é real e valioso, mas encontramos um problema antes de mergear.

No caminho if (orderedTargets.length === 0) que retorna 404 "Combo has no executable targets", o diff remove a chamada recordComboFailure(effectiveSessionId, combo.name); e a substitui pela chamada targetResolution.quotaShareRelease?.();, em vez de manter as duas. O comentário logo acima ("Surface a recovery hint + auto-clear the session pin after enough consecutive no-target failures...") permanece no código, mas passa a descrever um comportamento que não acontece mais nesse ponto — o auto-clear do session pin depois de 3 falhas consecutivas deixaria de contar essa falha específica.

Poderia ajustar para manter as duas chamadas (recordComboFailure(...) seguido de targetResolution.quotaShareRelease?.()) nesse callsite? Os outros dois pontos onde a release foi adicionada (o early-return de native-turn-pin e o finally bloco) não têm esse problema — são estritamente aditivos.

[diegosouzapw]

Nice work — the reservation-leak mechanism matches the idempotent decrementInflight contract in quotaShareStrategy and the new ordering tests are exactly what #11371 asked for (45/45 green locally). One real regression to fix before merge: in handleComboChatInner's orderedTargets.length === 0 early exit, the edit REPLACED recordComboFailure(effectiveSessionId, combo.name) with the quotaShareRelease call instead of adding beside it — the silent-stop pin auto-clear (#5923 Finding) no longer counts failures on this path, and no test caught it because none covers that path today. Please restore the recordComboFailure call alongside quotaShareRelease?.() and add a guard test asserting the combo-failure counter increments on an empty-pool early exit. Also note combo.ts picked up +56 lines on the current release tip since you branched, so a rebase onto origin/release/v3.8.51 is needed. Everything else looks merge-ready.