From 32835e97a64505d8c551a7ffcbed32260346631d Mon Sep 17 00:00:00 2001 From: JUN Date: Tue, 15 Sep 2026 03:23:43 +0900 Subject: [PATCH] fix(responses): reserve the last two generic-OAuth hops from the shared budget (#4546) The adapter recovery loop and the continuation loop were the two arms that actually iterate the credential roster, and they were the two still running on their own cap alone. A request could re-arm the per-request bound by reaching a different loop. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify. --- src/server/responses/core.ts | 43 +++++++++++++++---- .../lib/transient-budget-scope-source.test.ts | 8 ++-- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index c926e438e8..355d5bbd2b 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -8615,13 +8615,25 @@ async function handleResponsesInner( && genericFailovers < GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST && isGenericOAuthFailoverEnabled(config, route.providerName) ) { + // Intersection with the shared request budget. This arm re-sends through + // rebuildAndRefetch, so the roster cap alone would let one request walk the roster on + // an allowance the rest of the request cannot see. A refusal ends the ladder with the + // real 429 already in hand, which is the decided exhaustion contract. + const hop = reserveCredentialHop( + "auth-recovery", + `${route.providerName}|${route.modelId}|adapter-recovery-oauth-429`, + ); + if (!hop.allowed) break; const nextAccountId = rotateGenericOAuthAccountOn429( config, route.providerName, genericFailoverAccountId, upstreamResponse.headers.get("retry-after"), ); - if (!nextAccountId) break; + if (!nextAccountId) { + hop.permit?.release(); + break; + } try { void upstreamResponse.body?.cancel().catch(() => {}); } catch { /* already consumed/closed */ } try { // The FULL snapshot, not just the bearer: Antigravity pairs an account-matched @@ -8629,7 +8641,10 @@ async function handleResponsesInner( // would mix one account's credential with another's routing data. const snapshot = await failoverAccountSnapshot(route.providerName, nextAccountId); genericFailovers += 1; - if (!await applyFailoverSnapshot(snapshot)) break; + if (!await applyFailoverSnapshot(snapshot)) { + hop.permit?.release(); + break; + } invalidateSameTargetRequest(); activeAdapter = resolveSelectionAdapter( resolveWireProtocolOverride(route.providerName, route.modelId, route.provider, inboundWire), @@ -9085,12 +9100,22 @@ async function handleResponsesInner( && genericFailovers < GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST && isGenericOAuthFailoverEnabled(config, route.providerName) ) { - const nextAccountId = rotateGenericOAuthAccountOn429( - config, - route.providerName, - genericFailoverAccountId, - response.headers.get("retry-after"), + // Intersection with the shared request budget. The continuation loop re-sends the + // turn, so without this the per-request bound could be re-armed simply by reaching a + // different loop -- which is the divergence the comment above already warns about. + const hop = reserveCredentialHop( + "auth-recovery", + `${route.providerName}|${route.modelId}|continuation-oauth-429`, ); + const nextAccountId = hop.allowed + ? rotateGenericOAuthAccountOn429( + config, + route.providerName, + genericFailoverAccountId, + response.headers.get("retry-after"), + ) + : null; + if (!nextAccountId) hop.permit?.release(); if (nextAccountId) { try { void response.body?.cancel().catch(() => {}); } catch { /* already closed */ } try { @@ -9100,7 +9125,9 @@ async function handleResponsesInner( // routing data. const snapshot = await failoverAccountSnapshot(route.providerName, nextAccountId); genericFailovers += 1; - if (await applyFailoverSnapshot(snapshot, nextParsed)) { + const applied = await applyFailoverSnapshot(snapshot, nextParsed); + if (!applied) hop.permit?.release(); + if (applied) { invalidateSameTargetRequest(); activeAdapter = resolveSelectionAdapter( resolveWireProtocolOverride(route.providerName, route.modelId, route.provider, inboundWire), diff --git a/tests/lib/transient-budget-scope-source.test.ts b/tests/lib/transient-budget-scope-source.test.ts index 0772484aba..4a192a43a3 100644 --- a/tests/lib/transient-budget-scope-source.test.ts +++ b/tests/lib/transient-budget-scope-source.test.ts @@ -145,9 +145,11 @@ describe("every dispatch path reports into the shared budget", () => { test("credential hops keep their roster cap AND reserve from the shared budget", () => { const core = source("server/responses/core.ts"); - // Four hop sites: the native passthrough 429, the shared sidecar hook's generic and - // Anthropic arms, and the runTurn preflight 429. - expect(core.match(/reserveCredentialHop\(/g)).toHaveLength(4); + // Six hop sites: the native passthrough 429, the shared sidecar hook's generic and + // Anthropic arms, the runTurn preflight 429, the adapter recovery loop, and the + // continuation loop. The last two were the arms that actually iterate the roster, so + // leaving them out meant the claim held everywhere except where it mattered most. + expect(core.match(/reserveCredentialHop\(/g)).toHaveLength(6); // The per-roster caps are NOT replaced. The effective allowance is the intersection, so // removing either half is a behaviour change that has to be argued for. expect(core).toContain("genericFailovers < GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST");