From e90d0aeb28e584e7c46aca9e612f4514359c19b1 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 11:30:50 +0900 Subject: [PATCH 1/5] fix(routing): give a withheld recovery a retry time that is actually later (#4546) [skip ci] The transient-hold resolver and the pool-wide recovery limiter added in #4626 still have no production caller, so #4701 is not closed here. Wiring them turned up a defect in the thing being wired, and that has to be fixed first. A withheld dispatch promises the caller a retry time. It was computed from the probe pacing alone. When the RATIO limiter is what refused, the account usually has no probe state at all -- nothing was ever granted for it -- so nextProbeAt returned now, and the refusal told the caller to try again immediately. A withheld dispatch that busy-loops puts the same load on an already-failing pool as the dispatch it refused, which is the opposite of what the limiter is for. It also violates the Retry-After half of #4701's completion criteria directly. The limiter is the only thing that knows when its own window moves, so it now says: nextRecoveryAt returns now while the allowance is unspent, and otherwise the moment the oldest bucket still inside the window falls out. Every such bucket started after now - windowMs, so the answer is always strictly in the future, and it is a real change point rather than a guessed delay. The withheld result takes the later of that and the probe pacing. The existing zero-allowance test asserted only that the result was withheld, which is why the defect survived the unit suite that was written to cover this module. It now asserts the time as well. Refs #4701 --- src/routing/probe-lease.ts | 35 ++++++++++++++++++++++++++++++- tests/routing/probe-lease.test.ts | 30 ++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/src/routing/probe-lease.ts b/src/routing/probe-lease.ts index fb63061e01c..2182bab3eb2 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/tests/routing/probe-lease.test.ts b/tests/routing/probe-lease.test.ts index e3cf4ece838..1139d3140f3 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); }); }); From 6317c41ee1990251f6cb0e78e71712e302d8a30f Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 11:32:21 +0900 Subject: [PATCH 2/5] test(responses): pin the #4546 incident as one system, not five fixes (#4546) Each layer of this lane closes one seam of the #4546 amplification: the credential hop that was charged twice, the spend ledger with no caller, the refusal reported as a provider fault, the usage attributed to the wrong key, the withheld recovery that said "retry now". What none of them checks is whether the seams agree with each other. This composes the real primitives -- the request execution budget, the durable spend ledger with its request-scoped caller, the pool recovery limiter -- and asserts that the numbers describe the same events: physical sends, budget consumption, ledger reservation and settlement, and the refusal the caller is given. The scenarios are the incident's own: a request whose every layer tries to recover, concurrent requests contending for one process-wide recovery allowance, a caller that keeps its detour instead of adding a second trial to a failing account, a fan-out child spending the parent's allowance rather than a fresh one, and a restart that must neither reset a ceiling nor settle the same send twice. A fixture that only counted sends would have passed throughout the incident, which is why every case ties a send count to the spend the ledger recorded for it. --- scripts/test-layout/layout.json | 1 + tests/fixtures/test-layout-expected.json | 1 + ...responses-4546-incident-regression.test.ts | 155 ++++++++++++++++++ 3 files changed, 157 insertions(+) create mode 100644 tests/responses/responses-4546-incident-regression.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 1a1234c0682..17fc012dc30 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/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 5d6ede1f232..6994165c610 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/responses/responses-4546-incident-regression.test.ts b/tests/responses/responses-4546-incident-regression.test.ts new file mode 100644 index 00000000000..2e6436732b5 --- /dev/null +++ b/tests/responses/responses-4546-incident-regression.test.ts @@ -0,0 +1,155 @@ +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"; + +/** + * 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); + // ...but an account move cannot ALSO have one. This is the intersection the incident lacked: + // each layer used to hold its own allowance, so a spent request still funded every one. + 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("final-recovery-spent"); + + 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); + expect(limiter.state(now).refusedTotal).toBeGreaterThan(0); + }); + + 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); + // The send ids are still known, so a replayed request cannot authorise another dispatch. + expect(after.settle("lr-restart", { inputTokens: 1, outputTokens: 1 })).toBe(false); + }); +}); From 94db101206f8aa107e173df5cb2ba5ea18345b89 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 11:33:31 +0900 Subject: [PATCH 3/5] test(responses): pin the account-change half of the incident (#4546) The account-change scenario the incident needs, written against current behaviour because the #4710 refusal is owned by another lane and is not in this stack yet. What it pins now: continuation state is dropped and the turn continues, an uploaded file reference is classified non-portable and is NOT removed by the scrub, and the carriers must be read directly because the portability verdict reports only the first reason it finds -- a body carrying both a response id and a file reports the response id. What it documents: once the refusal lands, that body must be declined before dispatch and the refusal must win over the response id. The two properties above are what the change has to preserve, so they are asserted now. Also pins the accounting invariant that refusal owes: a decision made before dispatch spends no send and books no ledger entry. A refusal counted as a send would appear as provider load that never happened and would push a healthy account toward a cooldown. --- ...responses-4546-incident-regression.test.ts | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/tests/responses/responses-4546-incident-regression.test.ts b/tests/responses/responses-4546-incident-regression.test.ts index 2e6436732b5..ea57ae4f7e0 100644 --- a/tests/responses/responses-4546-incident-regression.test.ts +++ b/tests/responses/responses-4546-incident-regression.test.ts @@ -10,6 +10,15 @@ import { 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. @@ -152,4 +161,62 @@ describe("#4546 cost guard, end to end", () => { // The send ids are still known, so a replayed request cannot authorise another dispatch. expect(after.settle("lr-restart", { inputTokens: 1, outputTokens: 1 })).toBe(false); }); + + 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); + }); }); From 78800ec9d3149a52291ea06e725e248b9e469e95 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 12:13:33 +0900 Subject: [PATCH 4/5] fix(responses): keep core.ts at its cap and stop a degraded ledger refusing sends (#4546) Four fixes, batched into one push so the queue only pays once. 1. src/server/responses/core.ts was 214 lines against a 210-line cap in tests/fixtures/file-size-baseline.json. The spend-observer wiring added four lines of comment and continuation. The comment is now one line and the expression one line, and the file is back at its cap. The ratchet only ever lowers caps, so growing past one is a hard failure rather than a nudge. 2. The spend tracker refused a dispatch on ANY ledger denial. Only an operator's configured ceiling should: capacity, durability and a journal this process could not prove complete all mean the ledger cannot ACCOUNT for the 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. 3. The shared ledger is now resolved on the first charge rather than when the request is built. It opens a journal under the OpenCodex home, and a request that never dispatches has no business creating one; this also means the home in effect at dispatch is the one written to, instead of whichever home was current when the first request of the process happened to be constructed. 4. Three assertions in the new tests claimed states the code never reaches. The concurrent-probe case asserted a limiter refusal, but the second caller short-circuits on the lease before it reaches the limiter and costs no allowance; the shared bound is now proved by asking the limiter directly. The exhausted-ceiling case asserted final-recovery-spent where the total ceiling refuses first, so it asserts total-exhausted and checks reserveSpent separately for the point it was making. The unstructured-error control asserted an exact 502 where the property that matters is that the identity is gone, so it asserts that instead. Tests are not typechecked -- tsconfig includes only src -- so a test that asserts the opposite of what it claims passes silently. These were found by reading, not by running. --- src/server/responses/core.ts | 8 ++---- src/server/responses/request-spend.ts | 27 +++++++++++++------ ...responses-4546-incident-regression.test.ts | 21 +++++++++++---- .../responses-send-budget-errors.test.ts | 6 ++++- 4 files changed, 42 insertions(+), 20 deletions(-) diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index d09bfafb8c1..89bf34dad3a 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 0f7c088c056..b238c43395c 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; @@ -61,12 +67,12 @@ export function createRequestSpendTracker( * than unresolved, for at most one send per request. */ 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/tests/responses/responses-4546-incident-regression.test.ts b/tests/responses/responses-4546-incident-regression.test.ts index ea57ae4f7e0..348d9ac898c 100644 --- a/tests/responses/responses-4546-incident-regression.test.ts +++ b/tests/responses/responses-4546-incident-regression.test.ts @@ -65,12 +65,15 @@ describe("#4546 cost guard, end to end", () => { // 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); - // ...but an account move cannot ALSO have one. This is the intersection the incident lacked: - // each layer used to hold its own allowance, so a spent request still funded every one. + // ...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("final-recovery-spent"); + 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. @@ -99,7 +102,13 @@ describe("#4546 cost guard, end to end", () => { } // Separate request objects cannot mint private allowances: the limiter is process-wide. expect(limiter.state(now).recoveryDispatches).toBe(1); - expect(limiter.state(now).refusedTotal).toBeGreaterThan(0); + // 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", () => { @@ -158,8 +167,10 @@ describe("#4546 cost guard, end to end", () => { expect(settledAfter?.settled).toBe(150); expect(settledAfter?.unresolved).toBe(500); expect(settledAfter?.reserved).toBe(0); - // The send ids are still known, so a replayed request cannot authorise another dispatch. + // 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", () => { diff --git a/tests/responses/responses-send-budget-errors.test.ts b/tests/responses/responses-send-budget-errors.test.ts index 638bb1a2f4d..6e1306980b8 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", () => { From d48e3d2749daa81f847fea7da8960a6c174923ff Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 12:41:30 +0900 Subject: [PATCH 5/5] fix(lib): keep an exhausted ceiling exhausted across a restart (#4546) Two source-of-truth failures from the previous tip run, both mine. tests/lib/transient-budget-scope-source.test.ts pinned the exact core.ts line that mints the request's send budget, and bl2 changed it to install the spend observer. The oracle now matches the new shape and additionally asserts the observer is attached at the same place, which is the property that actually matters: a combo child inherits the parent's holder and must not open a second set of ledger entries for the same physical sends. tests/lib/spend-reservation-ledger.test.ts caught a real defect in the replay reconciliation, not a stale expectation. An exhausted scope must still be exhausted after a restart -- that is the whole reason the ledger is on disk -- and abandoning a replayed undispatched reservation handed its tokens back and reset the ceiling. The distinction I drew was wrong. "Open" does not prove nothing was sent: the torn-tail rule immediately above says the journal may be missing its last record, so a send can dispatch and die before its dispatch record lands. Both live states now resolve to unresolved spend, which is the conservative answer and the one that preserves the ceiling. The bl2 wiring test asserted the old split and is updated to the new figures, along with the structure contract and the tracker's own comment. --- src/lib/spend-reservation-ledger.ts | 25 +++++++++---------- src/server/responses/request-spend.ts | 6 ++--- structure/transports/responses.md | 16 ++++++------ .../lib/transient-budget-scope-source.test.ts | 5 +++- .../responses-spend-ledger-wiring.test.ts | 9 ++++--- 5 files changed, 33 insertions(+), 28 deletions(-) diff --git a/src/lib/spend-reservation-ledger.ts b/src/lib/spend-reservation-ledger.ts index 1aeb3893334..de7dba4e05b 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/server/responses/request-spend.ts b/src/server/responses/request-spend.ts index b238c43395c..3d9a9fc2e3f 100644 --- a/src/server/responses/request-spend.ts +++ b/src/server/responses/request-spend.ts @@ -62,9 +62,9 @@ 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); diff --git a/structure/transports/responses.md b/structure/transports/responses.md index 907abf05cb0..87e6a0dc561 100644 --- a/structure/transports/responses.md +++ b/structure/transports/responses.md @@ -741,19 +741,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/lib/transient-budget-scope-source.test.ts b/tests/lib/transient-budget-scope-source.test.ts index 8bc9c8d0ea9..a26a18bfa8b 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-spend-ledger-wiring.test.ts b/tests/responses/responses-spend-ledger-wiring.test.ts index fb7a724d368..fec30f94e6f 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); }); });