From 809e1c72d224b538e9618a7770da6b2725fdc7a1 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 13:53:50 +0900 Subject: [PATCH] fix(combos): preserve declared targets under the send budget (#4656) A long failover combo could exhaust the request allowance after a few providers and return the last 429/502 while later declared targets were never attempted at all. The combo policy and the per-target holdback were already correct. What was missing is that a derived scope never actually observed the request's spend. Aliasing the public used property shared only what callers read from outside: remainingBaseSends, the total check and the reserve test all consult the factory's own private counter, which an overridden property cannot reach. So every derived scope admitted dispatches as though the request had spent nothing, and comboTargetSendBudget's holdback -- expressed against maxTotalModelSends -- had nothing to hold back from. Move the physical-send ledger out of the closure and let a derived scope bind to the parent's exact one. deriveRequestExecutionBudget applies its own policy and keeps its own recovery ledgers while spending the shared ledger, so the holdback that reserves one dispatch for each still-declared target becomes enforceable. Three things travel on that ledger and have to travel together. The spend and the pending externally-counted bookings, because a pending booking is a send already counted in the total and waiting for its reporter, so sharing one without the other would either charge that send twice or never charge it. And the durable-spend observer, which books by watching this counter move: a derived scope that spent the counter without carrying the observer would move it without booking, and a combo child's sends would go missing from the spend ledger entirely. assumeCharge, which an adapter that owns its transport uses to take over a booking, closes it on that same shared ledger, so the adapter handoff and the combo derivation agree. What stays per-scope is deliberate: the reserve, alternate-target and transition ledgers are each target's own recovery decision, while the physical-send total is what binds every target together. A parent that did not come from this factory bridges onto its public used accessor rather than throwing. isRequestExecutionBudget is a shape test, so a stub can reach the derivation, and turning that into a thrown error would convert a routing request into a 500 to report a condition production never produces. The three-target row is asserted as the invariant the layer promises -- every declared target reached, the first target keeping a whole ladder, the total inside the declared policy total -- rather than as an exact per-target vector. A vector also pins how far this harness's adapter climbs inside each allowance, and the local suite is not run on this branch, so a number guessed from reading is a number nobody checked. A thirteen-target row covers the reported shape directly. This changes nothing about when a combo may advance. Another target is selected only after a child failure has been converted to a non-OK response, which the stream preflight does only for a terminal that committed no output. Closes #4656 Co-authored-by: RHODIZ IT --- src/lib/request-execution-budget.ts | 117 ++++++++++--- src/server/responses/core-combo.ts | 32 ++-- structure/transports/responses.md | 21 +++ tests/lib/execution-budget-permits.test.ts | 159 ++++++++++++++++++ .../responses-send-budget-counts.test.ts | 61 ++++--- 5 files changed, 326 insertions(+), 64 deletions(-) diff --git a/src/lib/request-execution-budget.ts b/src/lib/request-execution-budget.ts index dc6f2cede93..df27a156138 100644 --- a/src/lib/request-execution-budget.ts +++ b/src/lib/request-execution-budget.ts @@ -168,35 +168,53 @@ const RESERVE_FUNDED_CLASSES: ReadonlySet = new Set([ let logicalRequestSeq = 0; -export function createRequestExecutionBudget( - policy: RequestExecutionBudgetPolicy = CODEX_TEXT_GUARDED_BUDGET_POLICY, - logicalRequestId?: string, - observer?: RequestSendObserver, +/** + * One request's physical-send ledger, held apart from the budget object so a derived policy + * scope can share the exact same one. + * + * `spent` and `pendingExternalSends` belong together: a pending booking is a send that is + * already counted in `spent` and awaiting its reporter, so a scope that shared one without the + * other would either charge that send twice or never charge it at all. + * + * The durable-spend observer belongs here for the same reason. It books one entry per physical + * send by watching this counter move, so a derived scope that spent the counter without + * carrying the observer would move it without booking, and a combo child's sends would go + * missing from the ledger (#4707). + */ +interface SharedSendLedger { + spent: number; + pendingExternalSends: number; + readonly observer?: RequestSendObserver; +} + +const sharedSendLedgers = new WeakMap(); + +function createRequestExecutionBudgetWithLedger( + policy: RequestExecutionBudgetPolicy, + logicalRequestId: string | undefined, + counter: SharedSendLedger, ): RequestExecutionBudget { - let spent = 0; - // Reservations whose physical send is reported by a retry helper rather than by the permit. - // They are already charged; the reporter's first send settles one instead of charging again. - let pendingExternalSends = 0; + const observer = counter.observer; let reserveSpent = false; let alternateTargetSends = 0; let targetTransitions = 0; let lastTargetKey: string | undefined; const budget: RequestExecutionBudget = { - get used(): number { return spent; }, + get used(): number { return counter.spent; }, set used(next: number) { // The retry helpers report their real send count by assigning through this field. A // reservation taken with `countedExternally` has already booked one of those sends, so // the report settles the pending booking first and only the surplus is charged. - const delta = next - spent; + const delta = next - counter.spent; if (delta <= 0) { - spent = Math.max(0, next); + counter.spent = Math.max(0, next); return; } - const settled = Math.min(delta, pendingExternalSends); - pendingExternalSends -= settled; + const settled = Math.min(delta, counter.pendingExternalSends); + counter.pendingExternalSends -= settled; const charged = delta - settled; - spent += charged; + counter.spent += charged; // These sends have already left. The ledger records them even past a ceiling it would // have refused, because refusing after the fact only hides spend that was really // incurred -- the refusal has to happen at the reservation below, or not at all. @@ -211,11 +229,11 @@ export function createRequestExecutionBudget( get lastTargetKey() { return lastTargetKey; }, remainingBaseSends(cap: number): number { const capped = Number.isFinite(cap) ? Math.trunc(cap) : 0; - return Math.max(0, Math.min(capped, policy.baseSendAllowance - spent)); + return Math.max(0, Math.min(capped, policy.baseSendAllowance - counter.spent)); }, reserveDispatch(intent: DispatchIntent): DispatchDecision { if (intent.replaySafe === false) return { allowed: false, reason: "not-replay-safe" }; - if (spent >= policy.maxTotalModelSends) return { allowed: false, reason: "total-exhausted" }; + if (counter.spent >= policy.maxTotalModelSends) return { allowed: false, reason: "total-exhausted" }; const changesTarget = lastTargetKey !== undefined && lastTargetKey !== intent.targetKey; const isAlternateTarget = changesTarget || intent.sendClass === "account-failover" @@ -230,7 +248,7 @@ export function createRequestExecutionBudget( // The base allowance is spent first. Only once it is gone does a recovery class reach // for the single shared reserve -- an account move and a validated rebuild cannot each // take one. - const drawsReserve = policy.baseSendAllowance - spent <= 0; + const drawsReserve = policy.baseSendAllowance - counter.spent <= 0; if (drawsReserve) { if (!RESERVE_FUNDED_CLASSES.has(intent.sendClass)) { return { allowed: false, reason: "base-allowance-exhausted" }; @@ -250,8 +268,8 @@ export function createRequestExecutionBudget( // one remaining send admitted two physical sends, which is the per-request multiplication // this budget exists to stop. Everything is booked now; `release()` is the way back. const previousTargetKey = lastTargetKey; - spent += 1; - if (intent.countedExternally === true) pendingExternalSends += 1; + counter.spent += 1; + if (intent.countedExternally === true) counter.pendingExternalSends += 1; if (drawsReserve) reserveSpent = true; if (isAlternateTarget) alternateTargetSends += 1; if (changesTarget) targetTransitions += 1; @@ -273,8 +291,8 @@ export function createRequestExecutionBudget( // The booking this reservation made for an external reporter is now owned by the // caller. Leaving it pending is not harmless: the next `used` report of this request // would settle against it and one real send would go uncharged. - if (intent.countedExternally === true && pendingExternalSends > 0) { - pendingExternalSends -= 1; + if (intent.countedExternally === true && counter.pendingExternalSends > 0) { + counter.pendingExternalSends -= 1; } return true; }, @@ -284,10 +302,10 @@ export function createRequestExecutionBudget( // An externally counted reservation the reporter already settled paid for a send // that physically happened. Refunding it would hand the request a free send back. if (intent.countedExternally === true) { - if (pendingExternalSends === 0) return; - pendingExternalSends -= 1; + if (counter.pendingExternalSends === 0) return; + counter.pendingExternalSends -= 1; } - spent -= 1; + counter.spent -= 1; observer?.refund(); if (drawsReserve) reserveSpent = false; if (isAlternateTarget) alternateTargetSends -= 1; @@ -298,9 +316,60 @@ export function createRequestExecutionBudget( }; }, }; + sharedSendLedgers.set(budget, counter); return budget; } +export function createRequestExecutionBudget( + policy: RequestExecutionBudgetPolicy = CODEX_TEXT_GUARDED_BUDGET_POLICY, + logicalRequestId?: string, + observer?: RequestSendObserver, +): RequestExecutionBudget { + return createRequestExecutionBudgetWithLedger(policy, logicalRequestId, { + spent: 0, + pendingExternalSends: 0, + ...(observer ? { observer } : {}), + }); +} + +/** + * A budget that applies its own policy and keeps its own recovery ledgers while spending the + * parent's exact physical-send ledger. + * + * Aliasing the public `used` property was not enough, and that is the whole defect. The factory + * reads its own private counter back in `remainingBaseSends`, in the total check, and in the + * reserve test, so an aliased scope answered every admission question from a counter that only + * ever saw its own reservations. A combo's per-target holdback is computed from + * `maxTotalModelSends` and is therefore unenforceable unless the scope actually observes what + * the request has already spent. + */ +export function deriveRequestExecutionBudget( + parent: RequestExecutionBudget, + policy: RequestExecutionBudgetPolicy, +): RequestExecutionBudget { + return createRequestExecutionBudgetWithLedger(policy, parent.logicalRequestId, ledgerFor(parent)); +} + +/** + * A budget that did not come from this factory still honors the public `used` contract, so + * bridge onto it rather than failing the request. `isRequestExecutionBudget` is a shape test, + * so a stub can reach here; turning that into a thrown error would convert a routing request + * into a 500 to report a condition production never produces. Only a factory-backed parent can + * share pending external bookings and a durable-spend observer, which are private by + * construction; a bridged scope keeps the parent's spend accurate and books nothing of its own. + */ +function ledgerFor(parent: RequestExecutionBudget): SharedSendLedger { + const existing = sharedSendLedgers.get(parent); + if (existing) return existing; + let pendingExternalSends = 0; + return { + get spent(): number { return parent.used; }, + set spent(next: number) { parent.used = next; }, + get pendingExternalSends(): number { return pendingExternalSends; }, + set pendingExternalSends(next: number) { pendingExternalSends = next; }, + }; +} + export function isRequestExecutionBudget( value: TransientSendBudget | undefined, ): value is RequestExecutionBudget { diff --git a/src/server/responses/core-combo.ts b/src/server/responses/core-combo.ts index 6a573e4907b..31cbc4f6e73 100644 --- a/src/server/responses/core-combo.ts +++ b/src/server/responses/core-combo.ts @@ -2,7 +2,7 @@ import { isDeclaredReasoningEffort } from "../../reasoning-effort"; import { recordAttemptRequestedEffort } from "../request-log"; import { CODEX_TEXT_GUARDED_BUDGET_POLICY, - createRequestExecutionBudget, + deriveRequestExecutionBudget, isRequestExecutionBudget, } from "../../lib/request-execution-budget"; import type { @@ -107,25 +107,27 @@ export function comboExecutionBudgetPolicy(declaredTargets: number): RequestExec /** * A budget scope that keeps its own recovery ledgers but spends the SAME request-wide counter. * - * `used` is redefined as an accessor onto the parent because the factory reads it back off this - * object -- `remainingBaseSends` and the total check both do -- so a copied number would let a - * combo target run its ladder against a stale total, which is precisely the per-layer counting - * this work exists to remove. The reserve, alternate-target and transition ledgers stay - * per-scope on purpose: a combo target's account failover is its own recovery decision, while - * the request total still bounds every target together. + * The sharing has to happen inside the factory. Redefining `used` as an accessor onto the parent + * only shared what callers read from the outside: `remainingBaseSends`, the total check and the + * reserve test all consult the factory's own private counter, which an overridden property + * cannot reach. Each derived scope therefore admitted dispatches as though the request had spent + * nothing, and the per-target holdback below -- expressed against `maxTotalModelSends` -- had + * nothing to hold back from. + * + * `deriveRequestExecutionBudget` binds the scope to the parent's real ledger, including pending + * externally-counted bookings and the durable-spend observer, all of which must travel together. + * A pending booking is a send already counted in the total and waiting for its reporter, and the + * observer books by watching that same counter move (#4707) -- so a scope that spent the counter + * without carrying the observer would move it without booking, and this combo's child sends + * would go missing from the spend ledger. The reserve, alternate-target and transition ledgers + * stay per-scope on purpose: a combo target's account failover is its own recovery decision, + * while the request total still bounds every target together. */ export function deriveSendBudgetScope( parent: RequestExecutionBudget, policy: RequestExecutionBudgetPolicy, ): RequestExecutionBudget { - const scope = createRequestExecutionBudget(policy, parent.logicalRequestId); - Object.defineProperty(scope, "used", { - get: () => parent.used, - set: (value: number) => { parent.used = value; }, - enumerable: true, - configurable: true, - }); - return scope; + return deriveRequestExecutionBudget(parent, policy); } diff --git a/structure/transports/responses.md b/structure/transports/responses.md index f2f96927c90..da218f05e34 100644 --- a/structure/transports/responses.md +++ b/structure/transports/responses.md @@ -779,6 +779,27 @@ later recovery in the same request then cannot have. `tests/lib/execution-budget pins the settlement rule and every ladder shape against exactly that, and `tests/responses/responses-core-modules.test.ts` pins the adapter view's live delegation. +A combo derives a policy scope per target, and that derivation has to happen inside the budget +factory. Overriding the public `used` property shares only what callers read from outside: +`remainingBaseSends`, the total check and the reserve test all consult the factory's own private +counter, which an overridden property cannot reach. Each derived scope therefore admitted +dispatches as though the request had spent nothing, and the per-target holdback in +`comboTargetSendBudget` — expressed against `maxTotalModelSends` — had nothing to hold back from, +so a long failover combo could exhaust the allowance before its later declared targets were ever +attempted. `deriveRequestExecutionBudget` binds the scope to the parent's real ledger instead. + +Three things travel on that shared ledger and have to travel together. The spend and the pending +externally-counted bookings, because a pending booking is a send already counted in the total and +waiting for its reporter, so sharing one without the other would either charge that send twice or +never charge it. And the durable-spend observer below, because it books by watching this counter +move: a derived scope that spent the counter without carrying the observer would move it without +booking, and a combo child's sends would go missing from the ledger. `permit.assumeCharge()` +closes its booking on the same shared ledger, so the adapter handoff above and the combo +derivation agree rather than each settling against a counter the other cannot see. + +What stays per-scope is deliberate: the reserve, alternate-target and transition ledgers are each +target's own recovery decision, while the physical-send total is what binds every target together. + ## Durable spend reservations The request's send budget bounds how many times it may reach upstream; the spend ledger bounds diff --git a/tests/lib/execution-budget-permits.test.ts b/tests/lib/execution-budget-permits.test.ts index 687597e2416..f9163860bcc 100644 --- a/tests/lib/execution-budget-permits.test.ts +++ b/tests/lib/execution-budget-permits.test.ts @@ -3,6 +3,7 @@ import { readFileSync } from "node:fs"; import { CODEX_TEXT_GUARDED_BUDGET_POLICY, createRequestExecutionBudget, + deriveRequestExecutionBudget, type RequestExecutionBudgetPolicy, } from "../../src/lib/request-execution-budget"; @@ -348,3 +349,161 @@ describe("a credential hop is settled by whichever layer dispatches its replay", } }); }); + +describe("derived policy scopes", () => { + const wide: RequestExecutionBudgetPolicy = { + maxTotalModelSends: 8, baseSendAllowance: 7, finalRecoveryAllowance: 1, + maxAlternateTargetSends: 7, maxTargetTransitions: 7, + }; + + test("a derived scope admits against what the REQUEST has spent, not its own history", () => { + // The defect this closes. Aliasing the public `used` property shared only what callers read + // from outside; `remainingBaseSends`, the total check and the reserve test all consulted the + // factory's own private counter, so each derived scope believed the request had spent + // nothing and a per-target holdback had nothing to hold back from. + const parent = createRequestExecutionBudget(wide); + const first = deriveRequestExecutionBudget(parent, { ...wide, maxTotalModelSends: 2 }); + expect(first.reserveDispatch({ sendClass: "initial", targetKey: "a/m" }).allowed).toBe(true); + expect(first.reserveDispatch({ sendClass: "transient", targetKey: "a/m" }).allowed).toBe(true); + expect(parent.used).toBe(2); + + const second = deriveRequestExecutionBudget(parent, { ...wide, maxTotalModelSends: 2 }); + expect(second.used).toBe(2); + expect(second.remainingBaseSends(99)).toBe(5); + expect(second.reserveDispatch({ sendClass: "combo-failover", targetKey: "b/m" })) + .toEqual({ allowed: false, reason: "total-exhausted" }); + }); + + test("recovery ledgers stay per-scope while the send ledger is shared", () => { + // A later target's account failover is its own recovery decision; only the physical-send + // total binds the targets together. + const parent = createRequestExecutionBudget(wide); + const a = deriveRequestExecutionBudget(parent, { ...wide, maxAlternateTargetSends: 1, maxTargetTransitions: 1 }); + const b = deriveRequestExecutionBudget(parent, { ...wide, maxAlternateTargetSends: 1, maxTargetTransitions: 1 }); + expect(a.reserveDispatch({ sendClass: "account-failover", targetKey: "a/m" }).allowed).toBe(true); + expect(a.alternateTargetSends).toBe(1); + expect(b.alternateTargetSends).toBe(0); + expect(b.reserveDispatch({ sendClass: "account-failover", targetKey: "b/m" }).allowed).toBe(true); + expect(parent.used).toBe(2); + }); + + test("a pending external booking travels with the shared ledger", () => { + // A pending booking is a send already counted in the total and waiting for its reporter, so + // sharing the spend without it would charge that send twice. + const parent = createRequestExecutionBudget(wide); + const scope = deriveRequestExecutionBudget(parent, wide); + const hop = scope.reserveDispatch({ sendClass: "initial", targetKey: "a/m", countedExternally: true }); + expect(hop.allowed).toBe(true); + expect(parent.used).toBe(1); + + const target = deriveRequestExecutionBudget(scope, wide); + // The reporter names the send that the booking above already paid for. + target.used += 1; + expect(parent.used).toBe(1); + // Anything beyond it is a genuinely new send. + target.used += 2; + expect(parent.used).toBe(3); + }); + + test("assumeCharge on a derived scope closes the booking on the shared ledger", () => { + // bl1's adapter handoff and this shared ledger have to agree: an adapter that takes over a + // counted-externally reservation must close the booking the whole request can see, or the + // next report would settle against it and one real send would go uncharged. + const parent = createRequestExecutionBudget(wide); + const scope = deriveRequestExecutionBudget(parent, wide); + const hop = scope.reserveDispatch({ sendClass: "auth-recovery", targetKey: "a/m", countedExternally: true }); + expect(hop.allowed).toBe(true); + expect(hop.allowed && hop.permit.assumeCharge()).toBe(true); + expect(parent.used).toBe(1); + parent.used += 1; + expect(parent.used).toBe(2); + }); + + test("a scope derived from a foreign budget bridges instead of throwing", () => { + // `isRequestExecutionBudget` is a shape test, so a stub can reach the derivation. Turning + // that into a thrown error would convert a routing request into a 500 to report a condition + // production never produces. + let used = 4; + const foreign = { + get used() { return used; }, + set used(next: number) { used = next; }, + logicalRequestId: "foreign", + policyVersion: "guarded-v1", + policy: wide, + reserveSpent: false, + alternateTargetSends: 0, + targetTransitions: 0, + lastTargetKey: undefined, + remainingBaseSends: () => 0, + reserveDispatch: () => ({ allowed: false, reason: "total-exhausted" }), + } as unknown as Parameters[0]; + const scope = deriveRequestExecutionBudget(foreign, wide); + expect(scope.used).toBe(4); + expect(scope.reserveDispatch({ sendClass: "initial", targetKey: "a/m" }).allowed).toBe(true); + expect(used).toBe(5); + }); +}); + +describe("derived scopes and the durable spend observer", () => { + const wide: RequestExecutionBudgetPolicy = { + maxTotalModelSends: 8, baseSendAllowance: 7, finalRecoveryAllowance: 1, + maxAlternateTargetSends: 7, maxTargetTransitions: 7, + }; + const recordingObserver = () => { + const events: string[] = []; + let allow = true; + return { + events, + deny: () => { allow = false; }, + observer: { + charge: () => { events.push(allow ? "charge" : "refused"); return allow; }, + refund: () => { events.push("refund"); }, + }, + }; + }; + + test("a derived scope books its sends on the parent's ledger", () => { + // The observer books by watching the send counter move. A derived scope that spent the + // shared counter without carrying the observer would move it without booking, and every + // combo child send would be missing from the durable ledger. + const spy = recordingObserver(); + const parent = createRequestExecutionBudget(wide, "lr-observer", spy.observer); + const scope = deriveRequestExecutionBudget(parent, wide); + expect(scope.reserveDispatch({ sendClass: "combo-failover", targetKey: "b/m" }).allowed).toBe(true); + expect(spy.events).toEqual(["charge"]); + expect(parent.used).toBe(1); + }); + + test("one physical send is booked exactly once across the derivation", () => { + // A combo hop reserves with countedExternally and the child reports the same send. The + // pending booking settles that report, so the ledger must see one entry, not two. + const spy = recordingObserver(); + const parent = createRequestExecutionBudget(wide, "lr-once", spy.observer); + const scope = deriveRequestExecutionBudget(parent, wide); + expect(scope.reserveDispatch({ sendClass: "initial", targetKey: "a/m", countedExternally: true }).allowed).toBe(true); + deriveRequestExecutionBudget(scope, wide).used += 1; + expect(spy.events).toEqual(["charge"]); + expect(parent.used).toBe(1); + }); + + test("a released derivation refunds on the parent's ledger", () => { + const spy = recordingObserver(); + const parent = createRequestExecutionBudget(wide, "lr-refund", spy.observer); + const scope = deriveRequestExecutionBudget(parent, wide); + const leg = scope.reserveDispatch({ sendClass: "auth-recovery", targetKey: "a/m" }); + expect(leg.allowed).toBe(true); + if (leg.allowed) leg.permit.release(); + expect(spy.events).toEqual(["charge", "refund"]); + expect(parent.used).toBe(0); + }); + + test("a ledger ceiling refuses a derived dispatch rather than describing it afterwards", () => { + const spy = recordingObserver(); + const parent = createRequestExecutionBudget(wide, "lr-ceiling", spy.observer); + const scope = deriveRequestExecutionBudget(parent, wide); + spy.deny(); + expect(scope.reserveDispatch({ sendClass: "combo-failover", targetKey: "b/m" })) + .toEqual({ allowed: false, reason: "spend-exhausted" }); + expect(parent.used).toBe(0); + }); +}); diff --git a/tests/responses/responses-send-budget-counts.test.ts b/tests/responses/responses-send-budget-counts.test.ts index 78f5a428562..1fd7635e866 100644 --- a/tests/responses/responses-send-budget-counts.test.ts +++ b/tests/responses/responses-send-budget-counts.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { clearComboSelectionState, clearComboTargetCooldowns } from "../../src/combos"; import { clearKeyCooldowns } from "../../src/providers/key-failover"; import { handleResponses } from "../../src/server/responses/core"; +import { COMBO_TARGET_BASE_SENDS, comboExecutionBudgetPolicy } from "../../src/server/responses/core-combo"; import type { RequestLogContext } from "../../src/server/request-log"; import type { OcxConfig } from "../../src/types"; @@ -120,7 +121,7 @@ describe("upstream sends per logical request", () => { expect(sendCounts(logCtx)).toEqual([3]); }); - test("a three-target combo fan-out gives every declared target a send and totals six", async () => { + test("a three-target combo fan-out gives every declared target a send and stays bounded", async () => { const upstream = alwaysFailing(502, "upstream busy"); const logCtx: RequestLogContext = { model: "", provider: "" }; @@ -128,34 +129,44 @@ describe("upstream sends per logical request", () => { expect(response.status).toBe(502); await response.text(); - // The measured shape in #4546 was twelve: four sends per target, because each child took a - // fresh full allowance. Sharing one counter alone was not the answer either -- it starved - // the later targets to zero. The first target runs its own ladder, each later target draws - // what is left, and the clamp holds back one send for every target still declared, so the - // last target is still reached. - // Asserted as the INVARIANT the derived policy guarantees rather than as a fixture count. - // An exact per-target vector pins how this harness happens to distribute the ladder, which - // is not what the layer promises and not something this branch can observe: the local suite - // is not run here, so a number guessed from reading is a number nobody checked. + // Asserted as the INVARIANT the derived policy guarantees, not as a fixture vector. An exact + // per-target count also pins how far this harness's adapter happens to climb its own ladder + // inside each allowance, which is not what this layer promises; and the local suite is not + // run on this branch, so a vector guessed from reading is a vector nobody checked. const bearers = upstream.authorizations; - // Every declared target is still reached. Starving the last target is the failure mode that - // sharing one counter WITHOUT a per-target policy produces. + // Every declared target is reached. Starving the last one is the failure mode that sharing a + // counter WITHOUT a per-target policy produces, and #4546 measured the opposite failure -- + // twelve sends, four per target, because each child drew a fresh full allowance. expect(new Set(bearers).size).toBe(3); + expect(bearers[0]).toBe("Bearer sk-t0"); expect(bearers).toContain("Bearer sk-t2"); - // The first target keeps its full ladder, so the first sends are all its own. + // The first target keeps a whole ladder to itself. + expect(sendCounts(logCtx)[0]).toBe(COMBO_TARGET_BASE_SENDS); + // And the request total is the declared policy total, which is what the derived scope can + // now actually enforce: before the shared ledger, each scope admitted against a counter that + // had only ever seen its own reservations. + expect(totalSends(logCtx)).toBeLessThanOrEqual(comboExecutionBudgetPolicy(3).maxTotalModelSends); + expect(totalSends(logCtx)).toBe(bearers.length); + }); + + test("a thirteen-target combo still reaches every declared fallback", async () => { + // The reported shape: a long failover combo exhausted the allowance after a few providers + // and returned the last 502 while later declared targets were never attempted at all. + const upstream = alwaysFailing(502, "upstream busy"); + const logCtx: RequestLogContext = { model: "", provider: "" }; + + const response = await handleResponses(responsesRequest("combo/fan"), comboOverTargets(13), logCtx); + + expect(response.status).toBe(502); + await response.text(); + const bearers = upstream.authorizations; + expect(new Set(bearers).size).toBe(13); + for (let index = 0; index < 13; index += 1) { + expect(bearers).toContain(`Bearer sk-t${index}`); + } expect(bearers[0]).toBe("Bearer sk-t0"); - // Bounded by the derived total: the first target's ladder, one send per further declared - // target, and the single shared final-recovery reserve. The measured regression in #4546 was - // twelve, four per target, because each child drew a fresh full allowance. - // The measured bound is NINE, and saying six here would be describing an intention rather - // than the code. #4546 measured twelve -- four sends per target, each child drawing a fresh - // full allowance -- so sharing one counter removes the per-target reserve and takes it to - // nine. The clamp that was meant to hold back one send for every target still declared is - // NOT yet effective; that is stated in the pull request as the open item rather than hidden - // behind an assertion that passes for the wrong reason. - expect(bearers.length).toBeLessThanOrEqual(9); - expect(bearers.length).toBeLessThan(12); - expect(bearers.length).toBeGreaterThanOrEqual(3); + expect(sendCounts(logCtx)[0]).toBe(COMBO_TARGET_BASE_SENDS); + expect(totalSends(logCtx)).toBeLessThanOrEqual(comboExecutionBudgetPolicy(13).maxTotalModelSends); }); // REMOVED: "a 401 before the 5xx streak spends one of the same three sends".