From 1ac1ba0c8fe59fae7a636412609d1df838c490b0 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:03:02 +0900 Subject: [PATCH 1/2] fix(usage): verify OAuth rotation before withheld attribution --- src/oauth/generic-account-failover.ts | 11 +++++++++++ src/server/responses/adapter-continuation.ts | 9 ++++++--- src/server/responses/passthrough-dispatch.ts | 10 ++++++---- src/server/responses/run-turn-execution.ts | 11 ++++++----- structure/transports/responses.md | 2 ++ tests/oauth/generic-oauth-failover.test.ts | 11 +++++++++++ 6 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/oauth/generic-account-failover.ts b/src/oauth/generic-account-failover.ts index db2764d34c0..6086d80e96c 100644 --- a/src/oauth/generic-account-failover.ts +++ b/src/oauth/generic-account-failover.ts @@ -186,6 +186,17 @@ export function eligibleFailoverAccounts(providerName: string, now = Date.now(), .map(account => account.id); } +/** Whether reactive rotation has an alternate account it could select without mutating health. */ +export function hasEligibleGenericOAuthFailoverTarget( + providerName: string, + failedAccountId: string, + now = Date.now(), + requestedModelId?: string | null, +): boolean { + const family = classifyModelFamilyForQuota(providerName, requestedModelId); + return eligibleFailoverAccounts(providerName, now, family).some(id => id !== failedAccountId); +} + /** Generic pool strategies the kernel can actually run. `quota` IS the pre-kernel path. */ type ActiveGenericStrategy = "round-robin" | "fill-first"; diff --git a/src/server/responses/adapter-continuation.ts b/src/server/responses/adapter-continuation.ts index 858c76f538a..fd76d867d7a 100644 --- a/src/server/responses/adapter-continuation.ts +++ b/src/server/responses/adapter-continuation.ts @@ -40,6 +40,7 @@ import { } from "../../oauth/anthropic-routing"; import { GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST, + hasEligibleGenericOAuthFailoverTarget, isGenericOAuthFailoverEnabled, rotateGenericOAuthAccountOn429, failoverAccountSnapshot, @@ -431,9 +432,11 @@ export function createAdapterContinuations( route.modelId, ) : null; - // Eligible and refused by the shared budget, as opposed to eligible and finding no next - // account: the two produce the same response and need different follow-ups (#5044). - if (!hop.allowed) noteAttemptRecoveryWithheld(logCtx.activeAttempt, "rotation-send-budget"); + // A roster quorum ignores cooldowns, so only attribute a budget refusal when the + // non-mutating selector confirms that an alternate account could serve this model now. + if (!hop.allowed && hasEligibleGenericOAuthFailoverTarget( + route.providerName, transportState.genericFailoverAccountId, Date.now(), route.modelId, + )) noteAttemptRecoveryWithheld(logCtx.activeAttempt, "rotation-send-budget"); if (!nextAccountId) hop.permit?.release(); if (nextAccountId) { try { void response.body?.cancel().catch(() => {}); } catch { /* already closed */ } diff --git a/src/server/responses/passthrough-dispatch.ts b/src/server/responses/passthrough-dispatch.ts index 7bd9b64253c..6d8ce7965e5 100644 --- a/src/server/responses/passthrough-dispatch.ts +++ b/src/server/responses/passthrough-dispatch.ts @@ -136,6 +136,7 @@ import { publicOAuthAuthenticationErrorMessage } from "../../oauth"; import { resolveCopilotApiBaseUrl } from "../../oauth/github-copilot"; import { GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST, + hasEligibleGenericOAuthFailoverTarget, isGenericOAuthFailoverEnabled, rotateGenericOAuthAccountOn429, failoverAccountSnapshot, @@ -1303,10 +1304,11 @@ export async function preparePassthroughExchange( // No credential moved, so the reservation costs nothing. hop.permit?.release(); } else { - // Rotation was available -- the roster cap above admitted it -- and the shared request - // budget refused. Recorded so a one-send log is not read as "nothing was eligible", - // which is the ambiguity this attribution exists to remove (#5044). - noteAttemptRecoveryWithheld(logCtx.activeAttempt, "rotation-send-budget"); + // The activation quorum ignores cooldowns; prove that the selector has a live alternate + // before describing this as a recovery that only the shared request budget withheld. + if (hasEligibleGenericOAuthFailoverTarget( + route.providerName, transportState.genericFailoverAccountId, Date.now(), route.modelId, + )) noteAttemptRecoveryWithheld(logCtx.activeAttempt, "rotation-send-budget"); } } diff --git a/src/server/responses/run-turn-execution.ts b/src/server/responses/run-turn-execution.ts index 02f7b7e567c..04d61848f9e 100644 --- a/src/server/responses/run-turn-execution.ts +++ b/src/server/responses/run-turn-execution.ts @@ -23,6 +23,7 @@ import { adapterFailureFromMessage, SEND_BUDGET_EXHAUSTED_CODE } from "../../lib import { SendBudgetExhaustedError } from "../../lib/upstream-retry"; import { GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST, + hasEligibleGenericOAuthFailoverTarget, isGenericOAuthFailoverEnabled, rotateGenericOAuthAccountOn429, failoverAccountSnapshot, @@ -253,11 +254,11 @@ export async function executeResponsesRunTurn( `${route.providerName}|${route.modelId}|runturn-oauth-429`, ); if (!hop.allowed) { - // The roster bound above already said this credential set may rotate again; the shared - // request budget is what refused. Returning false lets the preflight 429 reach the - // client unchanged, which is right, but it used to leave a log indistinguishable from - // a request where no rotation was ever available (#5044). - noteAttemptRecoveryWithheld(logCtx.activeAttempt, "rotation-send-budget"); + // The activation quorum deliberately ignores cooldowns. Attribute a withheld recovery + // only when the non-mutating selector proves a usable alternate exists right now. + if (hasEligibleGenericOAuthFailoverTarget( + route.providerName, transportState.genericFailoverAccountId, Date.now(), route.modelId, + )) noteAttemptRecoveryWithheld(logCtx.activeAttempt, "rotation-send-budget"); return false; } const nextAccountId = rotateGenericOAuthAccountOn429( diff --git a/structure/transports/responses.md b/structure/transports/responses.md index a82e8f8cb08..e9043701011 100644 --- a/structure/transports/responses.md +++ b/structure/transports/responses.md @@ -427,6 +427,8 @@ Native Responses participates in the same pre-stream OAuth HTTP-429 account rota bridge. It uses the existing account quorum, cooldown and three-rotation request cap, refreshes the complete credential/transport/replay identity, and attributes usage to the serving account. Single-account installs do not retry; a missing alternate credential preserves the original error. +Send-budget refusal is attributed as a withheld rotation only when a non-mutating, model-family-aware +eligibility check confirms that an alternate account is not currently cooled. `shouldRetryCodexPoolAccountQuota` withholds that rotation when the 429 or 402 body names an organization- or project-scoped exhaustion (`codexScopedExhaustionCode` in diff --git a/tests/oauth/generic-oauth-failover.test.ts b/tests/oauth/generic-oauth-failover.test.ts index cc3f6f2e900..8a40371bd8c 100644 --- a/tests/oauth/generic-oauth-failover.test.ts +++ b/tests/oauth/generic-oauth-failover.test.ts @@ -7,6 +7,7 @@ import { clearGenericFailoverHealth, eligibleFailoverAccounts, genericFailoverRetryAfterSeconds, + hasEligibleGenericOAuthFailoverTarget, hasFailoverAccountQuorum, isGenericFailoverProvider, isGenericOAuthFailoverEnabled, @@ -241,6 +242,10 @@ describe("#2568 generic OAuth account failover", () => { const ids = await seed(2); const cfg = config(); expect(rotateGenericOAuthAccountOn429(cfg, "xai", ids[0]!, "120")).toBe(ids[1]); + // The durable roster quorum remains active, but the only alternate is cooled. A denied + // request budget must not describe this state as an otherwise available rotation. + expect(isGenericOAuthFailoverEnabled(cfg, "xai")).toBe(true); + expect(hasEligibleGenericOAuthFailoverTarget("xai", ids[1]!)).toBe(false); expect(rotateGenericOAuthAccountOn429(cfg, "xai", ids[1]!, "30")).toBeNull(); const retryAfter = genericFailoverRetryAfterSeconds("xai"); // The earliest window wins: a client must not be told to wait for the longest cooldown. @@ -282,6 +287,12 @@ describe("#2568 generic OAuth account failover", () => { describe("sidecar on429 wiring", () => { const coreSource = readResponsesCoreSource(); + test("budget-withheld attribution proves a cooldown-eligible generic OAuth target", () => { + // Continuation, native passthrough and run-turn each have their own budget-denial branch. + // A durable two-account quorum is insufficient because it intentionally ignores cooldowns. + expect(coreSource.match(/hasEligibleGenericOAuthFailoverTarget\(/g)).toHaveLength(3); + }); + test("both sidecar loops receive the SAME hook, so neither can drift key-pool-only", () => { const hooks = coreSource.match(/^\s*on429: (\w+),$/gm)?.map(line => line.trim()) ?? []; // Two injection sites — the image bridge and the web-search loop — and one shared hook. From f6778bfb7074d5ca3efd52a83fde1727fcbea908 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Tue, 22 Sep 2026 06:52:36 +0900 Subject: [PATCH 2/2] test(oauth): cover the eligible-target positive case and gate assertion --- tests/oauth/generic-oauth-failover.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/oauth/generic-oauth-failover.test.ts b/tests/oauth/generic-oauth-failover.test.ts index 8a40371bd8c..63b47f4585f 100644 --- a/tests/oauth/generic-oauth-failover.test.ts +++ b/tests/oauth/generic-oauth-failover.test.ts @@ -253,6 +253,15 @@ describe("#2568 generic OAuth account failover", () => { expect(retryAfter!).toBeLessThanOrEqual(30); }); + test("an uncooled alternate reports an eligible target", async () => { + const ids = await seed(2); + // The negative case above proves cooled accounts are excluded; without this positive + // side an always-false implementation would also pass, silently deleting the + // rotation-send-budget attribution for the normal case it exists to describe. + expect(hasEligibleGenericOAuthFailoverTarget("xai", ids[0]!)).toBe(true); + expect(hasEligibleGenericOAuthFailoverTarget("xai", ids[1]!)).toBe(true); + }); + test("Retry-After drives the cooldown length", async () => { const ids = await seed(2); rotateGenericOAuthAccountOn429(config(), "xai", ids[0]!, "600"); @@ -291,6 +300,9 @@ describe("sidecar on429 wiring", () => { // Continuation, native passthrough and run-turn each have their own budget-denial branch. // A durable two-account quorum is insufficient because it intentionally ignores cooldowns. expect(coreSource.match(/hasEligibleGenericOAuthFailoverTarget\(/g)).toHaveLength(3); + // The check must GATE the log, not merely run beside it: every call site wraps + // noteAttemptRecoveryWithheld in the eligibility condition. + expect(coreSource.match(/hasEligibleGenericOAuthFailoverTarget\([\s\S]*?\)\s*\)\s*noteAttemptRecoveryWithheld/g)).toHaveLength(3); }); test("both sidecar loops receive the SAME hook, so neither can drift key-pool-only", () => {