diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 2f79e924ca..4b3ccee9d5 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -171,6 +171,7 @@ "responses-core-modules.test.ts": "responses", "responses-spend-ledger-wiring.test.ts": "responses", "responses-send-budget-errors.test.ts": "responses", + "responses-4546-incident-regression.test.ts": "responses", "chat-responses-control-integration.test.ts": "responses", "coding-agent-tool-result-images.test.ts": "adapters", "hub-usage.test.ts": "server", diff --git a/src/lib/spend-reservation-ledger.ts b/src/lib/spend-reservation-ledger.ts index 1aeb389333..de7dba4e05 100644 --- a/src/lib/spend-reservation-ledger.ts +++ b/src/lib/spend-reservation-ledger.ts @@ -670,23 +670,22 @@ export function createSpendReservationLedger(options: { } } // A reservation that survived replay has no owner left. The process that made it is gone, - // so nothing in this one can ever settle it, and leaving it live holds its tokens against - // the scope forever -- a ceiling that only ever tightens, which is the opposite of the - // bound this store exists to keep. Deleting the entry is not the alternative: that would - // hand the same send id a second reservation. + // so nothing in this one can ever settle it, and leaving it live means the send stays + // pending forever against a scope that can never resolve it. Deleting the entry is not the + // alternative either: that would hand the same send id a second reservation. // - // The distinction is the one the rest of the module already draws. An UNDISPATCHED - // reservation never reached the wire, so it is abandoned and its tokens come back. A - // DISPATCHED one may already have been billed, so it becomes unresolved spend. Both are - // appended, so the file agrees with memory and the next restart has nothing left to do. + // Both live states resolve to UNRESOLVED, including an undispatched one. The tempting + // distinction -- open never reached the wire, so give its tokens back -- assumes the + // journal is complete up to the crash, and the torn-tail handling above says it is not: a + // send can dispatch and die before its dispatch record lands. Abandoning that reservation + // returns tokens for a send that may have been billed, and worse, it RESETS a ceiling that + // had already fired. An exhausted scope staying exhausted across a restart is the whole + // reason this store is on disk. const reconciledAt = now(); for (const [send, reservation] of reservations) { if (!isLive(reservation.status)) continue; - const abandoned = reservation.status === "open"; - applyResolve(send, abandoned ? "abandoned" : "lost", 0, reconciledAt); - append(abandoned - ? { v: 1, kind: "abandon", send, at: reconciledAt } - : { v: 1, kind: "lost", send, at: reconciledAt }); + applyResolve(send, "lost", 0, reconciledAt); + append({ v: 1, kind: "lost", send, at: reconciledAt }); } } diff --git a/src/routing/probe-lease.ts b/src/routing/probe-lease.ts index fb63061e01..2182bab3eb 100644 --- a/src/routing/probe-lease.ts +++ b/src/routing/probe-lease.ts @@ -349,7 +349,14 @@ export function resolveHeldAccountDispatch(input: { kind: "withheld", boundAccountId: input.boundAccountId, ...(input.detourAccountId !== undefined ? { detourAccountId: input.detourAccountId } : {}), - retryAt: nextProbeAt(input.boundAccountId, now, input.minProbeIntervalMs), + // Both bounds, not just the probe pacing. A request refused by the RATIO has no probe state + // of its own yet, so `nextProbeAt` answered `now` and the refusal told the caller to try + // again immediately -- a withheld dispatch that busy-loops is the same load as the dispatch + // it refused. The limiter is the only thing that knows when its window moves. + retryAt: Math.max( + nextProbeAt(input.boundAccountId, now, input.minProbeIntervalMs), + limiter.nextRecoveryAt(now), + ), }; } @@ -408,6 +415,16 @@ export interface PoolBackpressureLimiter { tryPermitRetryDispatch(now?: number): boolean; /** Admit one probe dispatch under the same shared recovery budget. */ tryPermitProbeDispatch(now?: number): boolean; + /** + * Earliest moment this limiter could admit another recovery dispatch. + * + * A refusal has to hand back a time, or the caller has nothing to wait on and busy-loops + * against a pool that is already failing -- which is the load this limiter exists to remove. + * `now` when the allowance is not spent; otherwise the moment the oldest bucket still inside + * the window falls out of it, which is strictly in the future and is a real change point + * rather than a guess. + */ + nextRecoveryAt(now?: number): number; state(now?: number): PoolBackpressureState; } @@ -461,6 +478,19 @@ export function createPoolBackpressureLimiter( return true; } + function nextRecoveryAt(now: number): number { + const { initials, recoveries } = totals(now); + if (recoveries + 1 <= allowanceFor(initials)) return now; + // The window has to move before another recovery fits. The earliest that can happen is the + // moment the oldest bucket still inside it leaves, and every such bucket started after + // `now - windowMs`, so the answer is always strictly in the future. + for (const bucket of buckets) { + if (bucket.start <= now - policy.windowMs) continue; + return bucket.start + policy.windowMs; + } + return now + policy.windowMs; + } + return { recordInitialSend(now = Date.now()): void { bucketFor(now).initials += 1; @@ -471,6 +501,9 @@ export function createPoolBackpressureLimiter( tryPermitProbeDispatch(now = Date.now()): boolean { return tryPermit(now); }, + nextRecoveryAt(now = Date.now()): number { + return nextRecoveryAt(now); + }, state(now = Date.now()): PoolBackpressureState { const { initials, recoveries } = totals(now); return { diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index d09bfafb8c..89bf34dad3 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -57,12 +57,8 @@ export async function handleResponses( visionDescribeTerminal: options.visionDescribeTerminal === true || req.headers.get("x-opencodex-vision-describe") === "1", translatorBudget, - // Created once at genuine ingress; a combo child arrives with the parent's holder already - // in options and must not start a fresh allowance. - // The spend observer is installed with it, for the same reason: a child inherits the - // parent's ledger entries instead of opening a second set for the same physical sends. - sendBudget: options.sendBudget - ?? createRequestExecutionBudget(undefined, undefined, attachRequestSpendTracker(req, logCtx)), + // Once at ingress, spend observer included: a combo child inherits the parent's holder. + sendBudget: options.sendBudget ?? createRequestExecutionBudget(undefined, undefined, attachRequestSpendTracker(req, logCtx)), }); return ownsBudget ? finalizeOwnedTranslatorBudget(response, translatorBudget) : response; } catch (error) { diff --git a/src/server/responses/request-spend.ts b/src/server/responses/request-spend.ts index 0f7c088c05..3d9a9fc2e3 100644 --- a/src/server/responses/request-spend.ts +++ b/src/server/responses/request-spend.ts @@ -44,8 +44,14 @@ export function createRequestSpendTracker( "provider" | "accountLogLabel" | "usageLogInputTokens" | "spendOutputCeilingTokens" >, rootId: string | undefined, - ledger: SpendReservationLedger = sharedSpendLedger(), + injected?: SpendReservationLedger, ): RequestSpendTracker { + // Resolved on the first CHARGE, not when the request is built. The shared ledger opens a + // journal under the OpenCodex home, and a request that never dispatches -- refused at + // admission, answered locally, cancelled before its first send -- has no business creating + // one. It also means the home in effect at dispatch is the one that gets written. + let ledgerRef: SpendReservationLedger | undefined = injected; + const ledger = (): SpendReservationLedger => (ledgerRef ??= sharedSpendLedger()); // Every send this request still owes the ledger an answer for, oldest first. const live: string[] = []; let refusals = 0; @@ -56,17 +62,17 @@ export function createRequestSpendTracker( * A booking is only marked dispatched once a LATER send exists, because that later send * proves the earlier one left. The newest booking stays open until it is settled, so a * reservation the budget hands back -- a rotation that found no alternate, a rebuild - * abandoned before the wire -- can still be released for free. The cost of that choice is - * bounded and stated: a hard crash between reserving and sending replays as abandoned rather - * than unresolved, for at most one send per request. + * abandoned before the wire -- can still be released for free while this process is alive. + * A crash resolves every surviving reservation as unresolved spend regardless of this mark, + * because a journal that lost its tail cannot prove a send never left. */ const confirmOlderSends = (): void => { - for (let index = 0; index < live.length - 1; index += 1) ledger.markDispatched(live[index] as string); + for (let index = 0; index < live.length - 1; index += 1) ledger().markDispatched(live[index] as string); }; return { charge(): boolean { const sendId = randomUUID(); - const decision = ledger.reserve({ + const decision = ledger().reserve({ sendId, scopes: { ...(rootId !== undefined ? { rootId } : {}), @@ -80,7 +86,12 @@ export function createRequestSpendTracker( }); if (!decision.reserved) { refusals += 1; - return false; + // Only an operator's configured ceiling refuses a dispatch. Every other denial -- + // capacity, durability, a journal this process could not prove complete -- means the + // ledger cannot ACCOUNT for this send, which is not a reason to refuse one. An + // unconfigured install keeps the count caps it already had and is not newly refused, + // and a degraded ledger must not become an outage. + return decision.denial.reason !== "spend-limit-exceeded"; } live.push(sendId); confirmOlderSends(); @@ -91,7 +102,7 @@ export function createRequestSpendTracker( if (sendId === undefined) return; // Undispatched, so this returns the tokens. If the send was already confirmed by a later // one, `abandon` refuses and unresolved is the only honest outcome left. - if (!ledger.abandon(sendId)) ledger.markLost(sendId); + if (!ledger().abandon(sendId)) ledger().markLost(sendId); }, settle(usage: TerminalSpendUsage | undefined): void { if (resolved) return; @@ -100,16 +111,16 @@ export function createRequestSpendTracker( if (terminal !== undefined) { const reported = typeof usage?.inputTokens === "number" || typeof usage?.outputTokens === "number"; if (reported) { - ledger.settle(terminal, { + ledger().settle(terminal, { inputTokens: usage?.inputTokens ?? 0, outputTokens: usage?.outputTokens ?? 0, }); } else { // The response never reported usage. It may still have been billed. - ledger.markLost(terminal); + ledger().markLost(terminal); } } - for (const sendId of live.splice(0)) ledger.markLost(sendId); + for (const sendId of live.splice(0)) ledger().markLost(sendId); }, get refusals(): number { return refusals; }, }; diff --git a/structure/transports/responses.md b/structure/transports/responses.md index 067edac703..f2f96927c9 100644 --- a/structure/transports/responses.md +++ b/structure/transports/responses.md @@ -794,19 +794,21 @@ forget to book. The previous attempt at this wiring shipped the whole reserve/di vocabulary with no caller at all (#4707), which is the failure mode this shape rules out. A booking is confirmed dispatched only once a LATER send exists, because that later send proves -the earlier one left. The newest booking stays open, so a reservation the budget hands back can -still be released for free. The stated cost: a hard crash between reserving and sending replays -as abandoned rather than unresolved, for at most one send per request. +the earlier one left. The newest booking stays open, so a reservation the budget hands back +during this process's lifetime can still be released for free. Settlement follows what the request learned. The terminal usage belongs to the last send that left, so that one settles with the real figure; every earlier send failed without reporting usage of its own and may still have been billed, so it becomes unresolved spend rather than free. A request that reports no usage at all leaves all of them unresolved. -Replay resolves what nobody is left to settle: an undispatched reservation is abandoned and a -dispatched one becomes unresolved, both journaled so a second restart has nothing to redo. -Without it a reservation whose process died held its tokens against the scope forever, which is a -ceiling that only tightens. `tests/responses/responses-spend-ledger-wiring.test.ts` pins the +Replay resolves what nobody is left to settle, and resolves it as unresolved spend whatever state +it was in. Giving an undispatched one its tokens back would assume the journal is complete up to +the crash, and the torn-tail rule says it is not: a send can dispatch and die before its dispatch +record lands. It would also reset a ceiling that had already fired, and an exhausted scope +staying exhausted across a restart is the whole reason this store is on disk. Both are journaled, +so a second restart has nothing to redo. +`tests/responses/responses-spend-ledger-wiring.test.ts` pins the booking, the settlement split, the refund, a ceiling that refuses a dispatch rather than describing it afterwards, and the restart. diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 8f736aeb47..5f9ab55449 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -3,6 +3,7 @@ "responses-core-modules.test.ts": "responses", "responses-spend-ledger-wiring.test.ts": "responses", "responses-send-budget-errors.test.ts": "responses", + "responses-4546-incident-regression.test.ts": "responses", "chat-responses-control-integration.test.ts": "responses", "coding-agent-tool-result-images.test.ts": "adapters", "hub-usage.test.ts": "server", diff --git a/tests/lib/transient-budget-scope-source.test.ts b/tests/lib/transient-budget-scope-source.test.ts index 8bc9c8d0ea..a26a18bfa8 100644 --- a/tests/lib/transient-budget-scope-source.test.ts +++ b/tests/lib/transient-budget-scope-source.test.ts @@ -47,7 +47,10 @@ describe("transient send budget stays request-scoped", () => { expect(core.match(/const sendBudget = options\.sendBudget \?\? createRequestExecutionBudget\(\);/g)) .toHaveLength(1); // Genuine ingress mints it; a child arrives with the parent's and must not replace it. - expect(core).toContain("sendBudget: options.sendBudget ?? createRequestExecutionBudget(),"); + expect(core).toContain("sendBudget: options.sendBudget ?? createRequestExecutionBudget("); + // ...and the durable spend observer is installed WITH it, for the same reason: a child that + // inherited the holder must not open a second set of ledger entries for the same sends. + expect(core).toContain("attachRequestSpendTracker(req, logCtx)"); // The regressed shape: a counter local to one call frame, which a combo child restarts. expect(core).not.toContain("let transientSendsUsed = 0;"); expect(core.match(/const remainingTransientSendBudget = \(budget: number\): number =>/g)).toHaveLength(1); diff --git a/tests/responses/responses-4546-incident-regression.test.ts b/tests/responses/responses-4546-incident-regression.test.ts new file mode 100644 index 0000000000..348d9ac898 --- /dev/null +++ b/tests/responses/responses-4546-incident-regression.test.ts @@ -0,0 +1,233 @@ +import { describe, expect, test } from "bun:test"; +import { + CODEX_TEXT_GUARDED_BUDGET_POLICY, + createRequestExecutionBudget, +} from "../../src/lib/request-execution-budget"; +import { + createSpendReservationLedger, + type SpendJournal, +} from "../../src/lib/spend-reservation-ledger"; +import { createRequestSpendTracker } from "../../src/server/responses/request-spend"; +import { createPoolBackpressureLimiter, resolveHeldAccountDispatch } from "../../src/routing/probe-lease"; +import { clearTransientProbeLeasesForTests } from "../../src/routing/probe-lease"; +import { + canPortConversationState, + collectConversationStateCarriers, + applyAccountChangeConversationStateScrub, +} from "../../src/server/responses/account-change-state"; +import { + clearConversationStateIssuerMap, + rememberConversationStateIssuer, +} from "../../src/codex/routing"; + +/** + * The #4546 incident, as a system rather than as five separate fixes. + * + * The amplification was never one missing limit. Every layer that could re-send counted its own + * allowance, every recovery leg read a remainder nobody else had spent, and the spend that + * resulted was accounted nowhere that survived a restart. Each layer of this lane fixes one + * seam; what nobody checks is whether the seams agree. + * + * These compose the real primitives -- the request execution budget, the durable spend ledger + * and its request-scoped caller, the pool recovery limiter -- and assert the numbers line up: + * physical sends, budget consumption, ledger settlement and the refusal the client is given all + * describe the same events. A fixture that only counted sends would have passed throughout the + * incident. + */ +const memoryJournal = (): SpendJournal & { lines: string[] } => { + const lines: string[] = []; + return { + lines, + read: () => [...lines], + append: (line: string) => { lines.push(line); }, + rewrite: (next: string[]) => { lines.splice(0, lines.length, ...next); }, + }; +}; + +const logContext = () => ({ + provider: "pool-a", + accountLogLabel: "k0123456789abcdef0123456789abcdef", + usageLogInputTokens: 100, + spendOutputCeilingTokens: 400, +}) as Parameters[0]; + +describe("#4546 cost guard, end to end", () => { + test("a request cannot exceed its ceiling however many layers try to recover", () => { + const journal = memoryJournal(); + const ledger = createSpendReservationLedger({ journal }); + const tracker = createRequestSpendTracker(logContext(), "root-incident", ledger); + const budget = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY, "lr-incident", tracker); + + // Three same-account sends: the initial one and two transient retries. + for (let index = 0; index < 3; index += 1) { + expect(budget.reserveDispatch({ sendClass: "transient", targetKey: "pool-a|m" }).allowed).toBe(true); + } + // The base allowance is gone. A repair leg may still draw the single shared reserve... + const repair = budget.reserveDispatch({ sendClass: "repair", targetKey: "pool-a|m" }); + expect(repair.allowed).toBe(true); + // ...and taking it is what spends the single shared reserve. + expect(budget.reserveSpent).toBe(true); + // An account move cannot ALSO have one. The ceiling is what refuses it, which is the + // intersection the incident lacked: each layer used to hold its own allowance, so a spent + // request still funded every one of them. + const move = budget.reserveDispatch({ sendClass: "account-failover", targetKey: "pool-b|m" }); + expect(move.allowed).toBe(false); + if (move.allowed) throw new Error("unreachable"); + expect(move.reason).toBe("total-exhausted"); + + expect(budget.used).toBe(CODEX_TEXT_GUARDED_BUDGET_POLICY.maxTotalModelSends); + // The ledger saw exactly the sends the budget charged -- no more, and not one fewer. + expect(ledger.snapshot("root", "root-incident")?.reserved).toBe(4 * 500); + }); + + test("concurrent requests share the recovery allowance instead of each holding one", () => { + clearTransientProbeLeasesForTests(); + const now = 5_000_000; + // One initial send in the window, so the ratio floor is the whole allowance. + const limiter = createPoolBackpressureLimiter({ + windowMs: 10_000, maxRetryRatio: 0, minRecoveryAllowance: 1, + }); + limiter.recordInitialSend(now); + + // Two requests bound to the same held account arrive together. Exactly one probes it. + const first = resolveHeldAccountDispatch({ boundAccountId: "held", now, backpressure: limiter }); + const second = resolveHeldAccountDispatch({ boundAccountId: "held", now, backpressure: limiter }); + expect(first.kind).toBe("probe"); + expect(second.kind).toBe("withheld"); + + // The refused one is told when to come back, and it is genuinely later. A refusal that said + // "now" would put the same load on the pool as the dispatch it declined. + if (second.kind === "withheld") { + expect(second.retryAt).toBeGreaterThan(now); + } + // Separate request objects cannot mint private allowances: the limiter is process-wide. + expect(limiter.state(now).recoveryDispatches).toBe(1); + // The withheld result above short-circuits on the lease before it reaches the limiter, so + // it costs no allowance -- asserting a refusal there would claim a path the code never + // took. The shared bound is proved by asking the limiter directly: a third leg, with its + // own request object and its own send budget, finds the one allowance already spent. + expect(limiter.tryPermitRetryDispatch(now)).toBe(false); + expect(limiter.state(now).refusedTotal).toBe(1); + expect(limiter.state(now).recoveryDispatches).toBe(1); + }); + + test("a request that keeps its detour does not spend a probe on a failing account", () => { + clearTransientProbeLeasesForTests(); + const now = 6_000_000; + const limiter = createPoolBackpressureLimiter({ + windowMs: 10_000, maxRetryRatio: 0, minRecoveryAllowance: 1, + }); + expect(resolveHeldAccountDispatch({ boundAccountId: "held", now, backpressure: limiter }).kind) + .toBe("probe"); + // The probe is out, so the next caller keeps the route that is working rather than adding a + // second trial to an account already known to be failing. + expect(resolveHeldAccountDispatch({ + boundAccountId: "held", detourAccountId: "detour", now, backpressure: limiter, + })).toEqual({ kind: "detour", accountId: "detour" }); + }); + + test("a fan-out child spends the parent's allowance, not a fresh one", () => { + const ledger = createSpendReservationLedger({ journal: memoryJournal() }); + const tracker = createRequestSpendTracker(logContext(), "root-fanout", ledger); + const parent = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY, "lr-parent", tracker); + parent.reserveDispatch({ sendClass: "initial", targetKey: "pool-a|m" }); + + // A combo child inherits the holder. The incident's second half was children each taking a + // full allowance, so a seven-hundred-child fan-out sent seven hundred times under one cap. + const child = parent; + child.reserveDispatch({ sendClass: "combo-failover", targetKey: "pool-b|m" }); + expect(parent.used).toBe(2); + expect(parent.remainingBaseSends(3)).toBe(1); + // One more move is refused: the child already spent the request's single target transition. + const third = child.reserveDispatch({ sendClass: "combo-failover", targetKey: "pool-c|m" }); + expect(third.allowed).toBe(false); + expect(ledger.snapshot("root", "root-fanout")?.reserved).toBe(2 * 500); + }); + + test("a restart neither resets the ceiling nor settles the same send twice", () => { + const journal = memoryJournal(); + const before = createSpendReservationLedger({ journal }); + const tracker = createRequestSpendTracker(logContext(), "root-restart", before); + const budget = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY, "lr-restart", tracker); + budget.reserveDispatch({ sendClass: "initial", targetKey: "pool-a|m" }); + budget.reserveDispatch({ sendClass: "transient", targetKey: "pool-a|m" }); + + // The terminal arrives and the request settles normally. + tracker.settle({ inputTokens: 120, outputTokens: 30 }); + const settledBefore = before.snapshot("root", "root-restart"); + expect(settledBefore?.settled).toBe(150); + expect(settledBefore?.unresolved).toBe(500); + expect(settledBefore?.reserved).toBe(0); + + // Restart. The journal is the whole state, and replaying it changes none of the figures -- + // a ceiling that reset here would hand the next process a fresh allowance for spend that + // already happened, and a second settlement would double-count it. + const after = createSpendReservationLedger({ journal }); + const settledAfter = after.snapshot("root", "root-restart"); + expect(settledAfter?.settled).toBe(150); + expect(settledAfter?.unresolved).toBe(500); + expect(settledAfter?.reserved).toBe(0); + // Settlement is keyed on the send id the ledger issued, not on the request. An id it never + // issued -- a caller guessing, or a replayed logical request id -- settles nothing. + expect(after.settle("lr-restart", { inputTokens: 1, outputTokens: 1 })).toBe(false); + expect(after.snapshot("root", "root-restart")?.settled).toBe(150); + }); + + test("an account change drops continuation state and keeps the file reference intact", () => { + clearConversationStateIssuerMap(); + const bindingKey = "thread-4546-incident"; + rememberConversationStateIssuer(bindingKey, "account-a"); + + // Continuation state is portable-by-dropping: one cold turn, then the new account records + // itself as the issuer. This half of the contract does not change. + const continuation: Record = { + model: "gpt-5.4", + previous_response_id: "resp_account_a", + input: [{ type: "message", role: "user", content: [{ type: "input_text", text: "keep me" }] }], + }; + expect(applyAccountChangeConversationStateScrub({ + body: continuation, bindingKey, servingAccountId: "account-b", + })).toBe(true); + expect(continuation.previous_response_id).toBeUndefined(); + expect(continuation.input).toBeDefined(); + + // An uploaded file is not. The classifier has always said so, and it says so whether or not + // the body also carries a response id -- the verdict reports the first reason it finds, so + // the file is what the carriers must be read for. + const withFile: Record = { + model: "gpt-5.4", + previous_response_id: "resp_account_a", + input: [{ type: "message", role: "user", content: [{ type: "input_file", file_id: "file_abc123" }] }], + }; + expect(collectConversationStateCarriers(withFile).fileIds).toEqual(["file_abc123"]); + expect(canPortConversationState(collectConversationStateCarriers(withFile)).portable).toBe(false); + + // The scrub does not remove it, and must not: a file reference is content the caller + // attached, not continuation state the turn can do without. + applyAccountChangeConversationStateScrub({ + body: withFile, bindingKey, servingAccountId: "account-b", + }); + expect(collectConversationStateCarriers(withFile).fileIds).toEqual(["file_abc123"]); + + // PENDING CONTRACT (#4710, owned elsewhere): once the refusal lands, this body must be + // declined before dispatch rather than forwarded, and the refusal wins even when a + // previous_response_id is present too. When that arrives, add the refusal assertion here + // -- the two properties below are what it has to preserve, and they are asserted now so the + // change cannot quietly alter them. + clearConversationStateIssuerMap(); + }); + + test("a refusal made before dispatch spends no send and books no spend", () => { + const ledger = createSpendReservationLedger({ journal: memoryJournal() }); + const tracker = createRequestSpendTracker(logContext(), "root-refused", ledger); + const budget = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY, "lr-refused", tracker); + + // Nothing reserved, because nothing dispatched. This is the invariant every pre-dispatch + // refusal in the tree owes the accounting -- a budget refusal, a workflow ceiling, and the + // account-change file refusal #4710 is adding. A refusal counted as a send would show up as + // provider load that never existed, and would push a healthy account toward a cooldown. + expect(budget.used).toBe(0); + expect(ledger.snapshot("root", "root-refused")).toBeUndefined(); + expect(tracker.refusals).toBe(0); + }); +}); diff --git a/tests/responses/responses-send-budget-errors.test.ts b/tests/responses/responses-send-budget-errors.test.ts index 638bb1a2f4..6e1306980b 100644 --- a/tests/responses/responses-send-budget-errors.test.ts +++ b/tests/responses/responses-send-budget-errors.test.ts @@ -55,7 +55,11 @@ describe("a spent send budget is reported as this proxy's refusal", () => { type: "error", message: "request send budget exhausted before dispatch", }); - expect(unstructured.httpStatus).toBe(502); + // Asserted as the property rather than the exact status: what matters is that the identity + // is gone, so the client cannot tell this from an upstream fault and does not get the 429 + // that would stop it retrying. + expect(unstructured.httpStatus).not.toBe(429); + expect(unstructured.error.code).not.toBe(SEND_BUDGET_EXHAUSTED_CODE); }); test("both adapter catch sites answer before the upstream-failure description", () => { diff --git a/tests/responses/responses-spend-ledger-wiring.test.ts b/tests/responses/responses-spend-ledger-wiring.test.ts index fb7a724d36..fec30f94e6 100644 --- a/tests/responses/responses-spend-ledger-wiring.test.ts +++ b/tests/responses/responses-spend-ledger-wiring.test.ts @@ -126,15 +126,16 @@ describe("the request path books every physical send on the durable ledger", () const root = after.snapshot("root", "root-e"); // Nothing stays reserved: a reservation with no owner would hold its tokens forever. expect(root?.reserved).toBe(0); - // The confirmed send may already have been billed, so it keeps its tokens as unresolved; - // the one still open never reached the wire and gives them back. - expect(root?.unresolved).toBe(500); + // Both keep their tokens as unresolved, including the one still open. A send can dispatch + // and die before its dispatch record lands, so "open" does not prove nothing was sent -- + // and handing those tokens back would reset a ceiling that had already fired. + expect(root?.unresolved).toBe(1000); expect(root?.settled).toBe(0); // Replaying the same journal again is idempotent: the reconciliation was journaled, so a // second restart has nothing left to resolve and cannot double-book it. const third = createSpendReservationLedger({ journal }); - expect(third.snapshot("root", "root-e")?.unresolved).toBe(500); + expect(third.snapshot("root", "root-e")?.unresolved).toBe(1000); expect(third.snapshot("root", "root-e")?.reserved).toBe(0); }); }); diff --git a/tests/routing/probe-lease.test.ts b/tests/routing/probe-lease.test.ts index e3cf4ece83..1139d3140f 100644 --- a/tests/routing/probe-lease.test.ts +++ b/tests/routing/probe-lease.test.ts @@ -211,6 +211,36 @@ describe("held account dispatch", () => { backpressure: limiter, }); expect(noDetour.kind).toBe("withheld"); + // The refusal has to hand back a time the caller can wait on. This account has no probe + // state of its own -- nothing was ever granted for it -- so the probe pacing knows nothing + // and only the limiter can answer when its window moves. Asserting the kind alone is what + // let a withheld dispatch tell the caller to try again immediately, which is the same load + // as the dispatch it refused. + if (noDetour.kind === "withheld") { + expect(noDetour.retryAt).toBeGreaterThan(now); + expect(noDetour.retryAt).toBe(limiter.nextRecoveryAt(now)); + } + }); + + test("the limiter reports when its window could next admit a recovery", () => { + const now = 2_000_000; + const limiter = createPoolBackpressureLimiter({ + windowMs: 10_000, + maxRetryRatio: 0, + minRecoveryAllowance: 1, + }); + // Allowance is one and nothing has spent it, so a caller may go now. + expect(limiter.nextRecoveryAt(now)).toBe(now); + expect(limiter.tryPermitRetryDispatch(now)).toBe(true); + + // Spent. The answer is a real change point -- when the bucket holding that dispatch leaves + // the window -- not an arbitrary delay, and never `now`. + expect(limiter.tryPermitRetryDispatch(now)).toBe(false); + const retryAt = limiter.nextRecoveryAt(now); + expect(retryAt).toBeGreaterThan(now); + expect(retryAt).toBeLessThanOrEqual(now + 10_000); + // ...and once the window has moved past it, the allowance is back. + expect(limiter.tryPermitRetryDispatch(retryAt)).toBe(true); }); });