diff --git a/devlog/_plan/260915_2560_release_train/000_roadmap.md b/devlog/_plan/260915_2560_release_train/000_roadmap.md index bae9b980e6..2a965e86d0 100644 --- a/devlog/_plan/260915_2560_release_train/000_roadmap.md +++ b/devlog/_plan/260915_2560_release_train/000_roadmap.md @@ -1,15 +1,29 @@ # 2.56.0 release train — roadmap -Status: open. Opened 2026-09-15. +Status: open. Opened 2026-09-15. Roadmap repaired 2026-09-15 after a reviewer round rejected the +first version; what changed is recorded under "Repairs" at the end. -## What this unit covers +## The frozen range -Everything between the `v2.55.0` tip on `main` (`1cc89cf88c`) and the `dev` tip that becomes -2.56.0, plus the release promotion itself. The range is small in commit count and large in blast -radius: three of the seven commits are facade splits of the hottest files in the project -(`bridge.ts` #4672, `server/index.ts` #4675, `server/responses/core.ts` #4677), each landed as a +The release candidate is **`2702911708`** and the baseline is **`1cc89cf88c`** (`v2.55.0`, the +current `main` tip). Nine commits, named here so "every commit was audited" is a checkable claim +rather than a feeling: + +| Commit | PR | What it is | +| --- | --- | --- | +| `369be813c4` | #4673 | reasoning input items get the summary the upstream requires | +| `11f1119718` | #4672 | `bridge.ts` split behind a facade | +| `3ea88f3db8` | #4674 | lab synchronous-activation guard extended to callees | +| `a63a47363f` | #4675 | `server/index.ts` split behind a facade | +| `89bc67353c` | #4681 | a quota test stops deleting the real OpenCodex home | +| `485a525aa9` | #4677 | `server/responses/core.ts` split behind a facade | +| `4bef58bf82` | #4684 | devlog only | +| `2046e684ed` | #4685 | devlog only — this plan unit | +| `2702911708` | #4683 | continuation replay misses refuse instead of truncating | + +Three of the nine are facade splits of the hottest files in the project, each landed as a behaviour-preserving refactor. A refactor that claims to change nothing is exactly the change a -release audit should not take on faith. +release audit should not take on faith, and it is why the audit spends most of its budget there. ## Constraint that shapes the whole unit @@ -21,18 +35,36 @@ instruments. Every claim below therefore names either a CI run at a SHA or a spe | Phase | Doc | Outcome | | --- | --- | --- | -| wp1 | this file | Roadmap locked; implementation starts in wp2. | -| wp2 | `10_land_4683.md` | #4683 rebased onto the dev tip, CI green at its exact head, squash-merged. | -| wp3 | `20_regression_audit.md` | Every commit in the range audited by a dispatched subagent; findings triaged. | -| wp4 | `30_release.md` | 2.56.0 promoted to `main`, release workflow green, publish verified. | - -wp2 and wp3 are independent and run concurrently: the audit reads committed objects, the landing -work touches the working tree. wp4 depends on both. +| wp1 | this file | Roadmap locked and repaired; implementation starts in wp2. | +| wp2 | `010_land_4683.md` | #4683 landed on `dev` with CI green at its exact head. **Done.** | +| wp3 | `020_regression_audit.md` | Every commit in the frozen range audited; findings triaged. | +| wp4 | `030_release.md` | 2.56.0 on `main` and `preview`, publish verified. | ## Completion criteria 1. #4683 squash-merged into `dev` with Cross-platform CI success at its exact head SHA. -2. Every commit in `v2.55.0..` the post-merge `dev` tip audited, with each REGRESSION or RISK - finding fixed or explicitly accepted with a stated reason. -3. 2.56.0 on `main` with hosted CI green at the promotion head and a successful publish. -4. No local full suite, typecheck or build was run anywhere in this unit. + **Met:** head `d8ef6ee9b889e51e5d3e547d60a537b8fbecfb85`, run `34935526979` success, squashed + as `2702911708`. +2. Each of the nine commits enumerated above has a recorded subagent verdict, and the final tree at + `2702911708` is audited for the invariants the three facade splits could break together. Every + REGRESSION or RISK is fixed on `dev` or accepted here in writing with a stated reason. +3. An explicit go/no-go decision is recorded against that audit before any promotion merge. +4. 2.56.0 reaches `main` and `preview`, each with hosted CI success at its exact promotion head, + and the release workflow reports a successful publish dispatched with `expected-sha` equal to + the `main` release commit. That commit is not the frozen candidate itself — a promotion merge + creates a new commit — so what must match the candidate is its tree, not its SHA, and + `release.yml` refuses any dispatch whose `expected-sha` differs from the commit it checks out. +5. No local full suite, typecheck or build was run anywhere in this unit. Every pass claim in these + documents cites either a hosted CI run at a SHA or a named focused test file. + +## Repairs + +The first roadmap was reviewed and rejected. Three blockers, all now discharged: + +- **The release order contradicted `MAINTAINERS.md`.** It promoted first and moved `dev` after. + `MAINTAINERS.md` lines 84-91 require the `dev` version move first. `030_release.md` now states + the order the policy and the workflow gates actually force. +- **The audit range had no frozen endpoint**, so "every commit" could not be checked. The table + above pins it, including the two devlog commits the first slice list omitted. +- **The landed evidence for #4683 was stale**, naming an intermediate head. Criterion 1 now carries + the exact head, the CI run and the squash commit. diff --git a/devlog/_plan/260915_2560_release_train/010_land_4683.md b/devlog/_plan/260915_2560_release_train/010_land_4683.md index 9567f8f12c..23bfaa8bdc 100644 --- a/devlog/_plan/260915_2560_release_train/010_land_4683.md +++ b/devlog/_plan/260915_2560_release_train/010_land_4683.md @@ -1,27 +1,52 @@ -# wp2 — land #4683 +# wp2 (round 1) — land #4683 + +Closed. The change is on `dev` as `2702911708` and is the last commit of the frozen 2.56.0 +candidate. ## The change A Codex client chained by `previous_response_id` sends only the newest turn. When local replay -state was gone, a destination on a translated wire received that delta alone under a normal 200: -the conversation was replaced by the one line the user had just typed. Only the canonical ChatGPT -forward route and stateless Responses destinations failed closed. The fix refuses with -`previous_response_not_found` for every destination that cannot see the omitted prefix, and raises -`RESPONSE_TTL_MS` from 1 hour to 24 hours so an ordinary idle gap resumes by expansion instead. +state was gone, a destination that cannot see the omitted prefix received that delta alone under a +normal 200: the conversation was replaced by the one line the user had just typed, with nothing in +the response saying so. Only the canonical ChatGPT forward route and stateless Responses +destinations failed closed. + +The fix refuses with `previous_response_not_found` for every destination except the native +Responses passthrough, which forwards the id to a backend that stored the chain. The three wires +that look stateful do not qualify, and `src/responses/continuation-ownership.ts` records why: devin +re-sends the whole conversation each turn, cursor reads its `checkpointRef` out of the same expired +store and otherwise falls back to `full-replay`, and kiro rebuilds `conversationState.history` from +the turns it was handed. Kiro's former `invalid_request_error` is removed with them, because that +code ended the task instead of triggering the recovery Codex performs on the structured one. -## Rebase note +`RESPONSE_TTL_MS` moves from 1 hour to 24 hours so an ordinary idle gap resumes by local expansion +instead of a replay round trip, and `WEBSOCKET_IDLE_TIMEOUT_SECONDS` is documented as coupled to it +with a test holding the pair together. + +## Two things this cycle got wrong first The branch was opened against `49dcdbf535`, before #4677 split `core.ts`. The gate had moved to -`src/server/responses/request-prepare.ts`, so the branch was rebuilt on the current `dev` tip and -the gate ported there rather than rebased through a conflicting delete/split. One rebase, then CI, -then squash merge. +`src/server/responses/request-prepare.ts`, so the branch was rebuilt on the `dev` tip and the gate +ported there rather than rebased through a conflicting split. One rebase, then CI, then squash. + +The first allowlist let kiro, cursor and devin through. A dispatched audit disputed it and was +right; all three were then verified in source to rebuild the conversation from the request they are +handed, and the exported set is now empty. ## Evidence -- `bun test tests/codex-integration/issue-702-expired-replay-state.test.ts` — 16 pass / 0 fail on - the rebased base. The new case was driven red first: with the gate stashed, the expired - continuation returned 200 carrying the delta only. -- `bun test tests/responses/responses-core-modules.test.ts` — 9 pass, so the owner-module - inventory and line ceiling still hold after the port. -- `bun run structure:check` — passed. -- Cross-platform CI at the exact head SHA — recorded in the PR. +- Exact head `d8ef6ee9b889e51e5d3e547d60a537b8fbecfb85`. Cross-platform CI run `34935526979`: + success on Linux, Windows and macOS. Squash-merged to `dev` as `2702911708`. +- Two CI-found failures were fixed rather than worked around: the file-size ratchet caught + `tests/responses/responses-state.test.ts` growing past its cap, and the three added lines were + removed instead of raising the baseline; `tests/oauth/state-store-sweeper.test.ts` swept at + `+1h`, which no longer expires a continuation row under 24-hour retention. +- Focused local files, each passing on the final tree: + `tests/codex-integration/issue-702-expired-replay-state.test.ts` (20), + `tests/responses/responses-state.test.ts` (145), `tests/responses/ws-endpoint.test.ts` (27), + `tests/responses/responses-core-modules.test.ts` (9), + `tests/oauth/state-store-sweeper.test.ts` (19), + `tests/ci-workflows/file-size-ratchet.test.ts` (6). +- The new refusal case was driven red first: with the gate reverted, the expired continuation + returned 200 carrying the delta only. +- `bun run structure:check` — passed. No local full suite was run. diff --git a/devlog/_plan/260915_2560_release_train/020_regression_audit.md b/devlog/_plan/260915_2560_release_train/020_regression_audit.md index 009d90c408..1fa18c6238 100644 --- a/devlog/_plan/260915_2560_release_train/020_regression_audit.md +++ b/devlog/_plan/260915_2560_release_train/020_regression_audit.md @@ -73,3 +73,258 @@ live sideband relay; and it would not help HTTP clients, a restarted proxy, or a early by the byte caps. The refusal path covers all of those uniformly, so the timeout stays 0 and the coupling is recorded where the constant lives, with `tests/responses/ws-endpoint.test.ts` holding the pair together. + +## Round 2 — the frozen range, audited from this worktree + +Round 1 ran before #4683 landed and against a range that had no frozen endpoint. Round 2 audits +the nine commits enumerated in `000_roadmap.md` against the candidate `2702911708`, from a managed +worktree so the auditors read a tree nobody is editing underneath them. Same instrument as round 1: +parallel `gpt-5.6-sol` subagents at medium reasoning effort, reading committed objects, running no +tests. + +The weight is deliberately on the three facade splits and on the final tree they produce together. +Each split was landed as behaviour-preserving, and each was reviewed alone; what no single review +covered is the tree that results from all three plus the new module #4683 added. That is the slice +that exists because a per-commit-clean range can still end in a broken tree. + +### Round 2 findings + +#### Round 2 slices + +| Slice | Target | Why it exists | +| --- | --- | --- | +| S1 | `11f1119718` bridge split | SSE assembly, usage accounting, shared watchdog state, export surface. | +| S2 | `a63a47363f` server/index split | Synchronous `startServer`, `labActivationRequired` gate, slot registration order. | +| S3 | `485a525aa9` core.ts split | The largest split, on the hottest request path; moved guards and module state. | +| S4 | `369be813c4`, `3ea88f3db8`, `89bc67353c` | The three small commits: in-place mutation, a possibly vacuous guard, a destructive test path. | +| S5 | `4bef58bf82`, `2046e684ed` | Devlog-only claim, checked against the packaging and CI path filters. | +| S6 | `2702911708` as landed | The squash equals the reviewed head, and the change re-attacked on the landed tree. | +| S7 | final tree | The invariants all three splits could break TOGETHER: lab-boundary import graph, the synchronous activation window, cycles, duplicated module state. | +| S8 | release surface | Packaging allowlist, workflow permissions, action refs, and test integrity — deleted, skipped or weakened tests and regenerated baselines across the range. | + +S7 is the slice this round exists for. Each split was reviewed alone and each looked clean alone; +nothing has yet read the tree they produce together, which is the tree being released. + +#### The range was wrong, and why that matters + +Round 2 opened against a nine-commit range. A reviewer round on the audit plan rejected it: the +merge-base between `main` and the candidate did not exist and `369be813c4` appeared to be a +parentless root commit. Both were artifacts of a **shallow clone** — `git rev-parse +--is-shallow-repository` returned `true` and `.git/shallow` held the graft list. The nine commits +were simply the ones that survived the graft. + +After `git fetch --unshallow`, the real release delta is **59 commits, 290 files, +66,064 / +-43,948**, with merge-base `62f02223a0`. The nine-commit table in `000_roadmap.md` described the +tail of the range, not the range. + +This is worth recording beyond this release. Every claim of the form "we audited every commit from +main" is only as good as the clone it was computed in, and a shallow clone answers that question +wrongly without erroring. The check is one command and it now belongs at the front of any release +audit. + +The corrected range is dominated by god-file decompositions across three rounds — `config.ts`, +`openai-responses.ts`, the `openai-chat` adapter, `provider-fetch`, the codex auth management API, +the provider registry table, state and shim, routing and quota, inject and catalog sync, then +`bridge.ts`, `server/index.ts` and `responses/core.ts` — plus the #4546 send-budget, spend-ledger +and identity/lineage work. Several splits are followed by their own repair commits +(`ce51b3eb07`, `48abcfbff5`, `e874436065`, `e443f58e8a`), which is the pattern a release audit +should be least willing to take on trust: a repair that silenced the symptom is not evidence that +the split dropped nothing else. + +#### Round 2, wave 2 slices + +| Slice | Target | +| --- | --- | +| W1 | `9b711073ab` openai-responses.ts split | +| W2 | `90aeffa702` openai-chat split, `47b1879af9` provider-fetch split | +| W3 | `0c745bd825` codex auth API split, `ee9f4df7b1` provider registry table split | +| W4 | `d2d35e02e2` config.ts split and its import-depth repair | +| W5 | `913e0d071f`, `ce51b3eb07`, `c63e9ea676`, `e874436065` state/shim/inject/catalog-sync and repairs | +| W6 | `35969857f2`, `48abcfbff5` routing/quota split and repair | +| W7 | #4546 send-budget and spend-ledger family, eight commits | +| W8 | #4546 identity, lineage and continuation-ownership family, four commits | +| W9 | the guards themselves: ratchet, import-resolution, version line | +| W10 | release surface over the true range, including the packaging allowlist for every new leaf | +| W11 | cross-facade behavioural wiring at the final tree, four traced end-to-end paths | + +W10 carries a failure mode nothing else would catch: a facade that imports a leaf which the +published package does not ship passes every test in CI and breaks every install. + +### Round 2 verdicts + +Nineteen slices returned, run on `gpt-5.6-sol` and, after sol began refusing parallel fan-out with +429s, paired 1:1 onto `xai/grok-4.6`. Coverage is every commit in the frozen range plus four +whole-tree slices. + +**The twelve god-file decompositions are clean.** That is the headline, and it is the claim this +round existed to disprove. + +| Slice | Target | Verdict | +| --- | --- | --- | +| S1 | `bridge.ts` split | CLEAN — six exports preserved, SSE/JSON/error bodies byte-identical, watchdog timeout a single live binding. | +| S2 | `server/index.ts` split | CLEAN — `startServer` still synchronous, Lab still behind `labActivationRequired`, 55 exports identical, registration in the same turn as `Bun.serve`. | +| S3 | `responses/core.ts` split | CLEAN — 31 exports identical, all 13 module-level state declarations have exactly one owner, 1,246 modules walked with no new cycle touching the split. | +| C1 | `openai-responses.ts`, `openai-chat`, `provider-fetch` splits | CLEAN — declaration parity 83/83, 73/73, 104/104; catalog timeout, abort and retry preserved; dedupe and memo maps single-owned. | +| G-W3 | codex auth API, provider registry table | CLEAN — 39 facade exports and all 24 route pairs survive; tokens stay inside `withResetCreditAuth`; 93 registry rows with matching flag checksums. | +| G-W4 | `config.ts` split | CLEAN — export surface, lock and atomic-write semantics, and all five schema defaults unchanged; the one wrong import depth was `routing/active-account` and nothing else in `src/`. | +| G-W5 | state, shim, inject, catalog-sync and their repairs | CLEAN — the splits did drop bindings; the repairs restored the complete set. Eight wrong-module or missing symbols enumerated and confirmed restored. | +| G-W6 | routing and quota split and its repair | CLEAN — 118/134 and 136/139 function bodies byte-identical, the rest accessor-wrapped; every cooldown, affinity and quota table has one owner. | +| G-S7 | final-tree state duplication | CLEAN — full owner/mutator inventory across every facade in the range; no binding with two declaration sites, no re-export copying a value instead of the live binding. | +| S5 | the two devlog commits | CLEAN — devlog only, excluded from the package allowlist and the CI path filters. | +| S6 | #4683 as landed | CLEAN — the interdiff against the reviewed head is only this plan unit. | + +**Two real regressions, both in the #4546 work rather than in any split.** + +1. `ce0ac617da` leaks a charged send permit on a pre-dispatch failure. `reserveCredentialHop()` + charges immediately; the generic-OAuth 429 ladder releases it on its two explicit early-outs but + its `catch` does not, so a throw from `failoverAccountSnapshot()` or snapshot application + consumes an allowance for a send that never happened, and a later recovery in the same request + can be refused because of it. Both loops have it: + `src/server/responses/adapter-dispatch.ts` and `src/server/responses/adapter-continuation.ts`. + The fix is not a blanket release in the `catch`: the dispatch loop's `try` also wraps + `rebuildAndRefetch`, which really does send, so the pre-dispatch part has to be separated. +2. `c3106e3eed` lets a successful reauthentication inherit the failed credential's cooldown. + `src/codex/pool-refresh-backoff.ts` keys cooldowns by account id with no credential generation, + and `login-flow.ts` clears quota and reauth state but not the refresh-failure record, so a + freshly authenticated account stays excluded from selection for 15-60 seconds. With a healthy + sibling the thread detours and loses its warm cache and continuation. This worked immediately + before that commit. + +**Risks recorded and accepted, none of them a runtime regression.** + +- The file-size ratchet dropped six former god-files from its cap list when they fell under the + 2,000-line threshold, so `src/codex/routing.ts` can grow 373 lines and `src/responses/state.ts` + 628 before the gate says anything — while facades that were lowered in place cannot. The same + baseline also raised caps for three test files that grew, and eleven of the twelve + `GENERATED_PATHS` exemptions are hand-written files, including the `en.ts` i18n catalogue that + calls itself the source of truth. +- The lab synchrony guard stops one hop after `startServer`, and the destructive-home guard matches + only single-line `rmSync(getConfigDir())` forms. Both would stay green on a future reintroduction. +- The durable spend ledger has no production caller: `admitWorkflowTurn()` is invoked without the + `spend` argument, so no reservation reaches the journal and the ceilings remain process-local. + The feature is incomplete rather than broken. +- Adapter and runTurn paths report send-budget exhaustion as `502 upstream_error` while the + passthrough path returns `429 request_send_budget_exhausted`, and the continuation 429 loop does + not consult `sendBudgetExhausted()`. Both predate this range. +- An account change scrubs `previous_response_id` and `conversation` but not uploaded `file_id` + references, although the same module classifies those as non-portable. Also pre-existing. + +**What this audit cannot discharge.** Source reading cannot prove the candidate typechecks, builds, +or behaves under real streaming, cancellation, replay and concurrency. That residual is carried by +hosted CI at the exact release SHA, and by the focused guard files run locally on the candidate: +the lab-boundary import graph, every relative import under `src` and `gui/src` resolving, the +responses core-module inventory, the test layout, structure SSOT and the ratchet — 138 assertions, +all passing. + +### Coverage: every commit in the frozen range, and the slice that read it + +Criterion 2 says each commit in `1cc89cf88c..2702911708` carries a recorded verdict. This is that +mapping, so the claim can be checked rather than believed. Merge commits are covered by the slice +that owns the lane they merged; devlog and plan commits are covered by S5's rule that a devlog-only +diff touches nothing in the build, test, packaging or workflow path, which was verified against the +package allowlist and the CI path filters rather than assumed. + +| Commits | Slice | +| --- | --- | +| `2702911708` | S6 | +| `2046e684ed`, `4bef58bf82`, `ca00b7e33e`, `8301dcb900`, `d97f740f73`, `db6b9f2ed3`, `f2dd9dd622`, `4f788f916e`, `7b7648e17a` | S5 (devlog/plan only) | +| `485a525aa9` | S3 | +| `a63a47363f` | S2 | +| `11f1119718` | S1 | +| `9b711073ab`, `90aeffa702`, `47b1879af9` | C1 | +| `369be813c4`, `3ea88f3db8`, `89bc67353c` | S4 | +| `d2d35e02e2`, `e443f58e8a` | G-W4 | +| `913e0d071f`, `ce51b3eb07`, `c63e9ea676`, `e874436065` | G-W5 | +| `35969857f2`, `48abcfbff5` | G-W6 | +| `0c745bd825`, `ee9f4df7b1` | G-W3 | +| `d5585a021a`, `8caf0a5126`, `a223a25d3b`, `00f1762d03`, `627274b8f5`, `ce0ac617da`, `836511b9c4`, `49dcdbf535` | W7 | +| `68951a16c1`, `38a2d9fb84`, `2b43c14c03`, `c3106e3eed` | W8 | +| `45fca0ad62`, `f5a8a44094`, `0eab3851a5`, `626b0f932c` | G-W9 | +| `aa91958e3b`, `09067c586a`, `9eb6290367`, `a90a99a521`, `16869805d6`, `90e7c23175`, `55cd467401`, `571cbe2d0e`, `f9e2ee077c`, `ccb7454a2d`, `60d935f888`, `cf1099577a`, `a6c6e29018`, `a6eb03b82e` | merges into the lanes their slices own; `571cbe2d0e` additionally read by G-W9 for the baseline reseed and by G-W6 for the issuer map | +| whole tree at `2702911708` | S7/G-S7 (state duplication), G-W11 (behavioural wiring), S8 (release surface, test integrity) | + +The release-surface slice adds one result worth stating separately, because it is the failure mode +that no test would catch: all 125 source files this range adds are covered by the `src` entry in the +package allowlist, so no facade imports a leaf the published package would omit. + +### What the audit changed on dev + +Two regressions fixed, and one of the accepted risks closed because it was cheap to close. + +- The generic-OAuth 429 ladder now hands its reservation back when nothing was sent. + `src/server/responses/adapter-dispatch.ts` confirms the permit immediately before the rebuild + that spends it and releases in its `catch`; since `release()` is a no-op once used, that one + catch covers both a pre-dispatch throw and a throw from the send itself. + `src/server/responses/adapter-continuation.ts` only releases, because its replay happens on the + next loop iteration and confirming before `continue` would charge a hop that never ran. This is + the shape `run-turn-execution.ts` already had. +- A replacement credential no longer inherits the dead one's quarantine: + `src/codex/auth-api/login-flow.ts` clears the refresh-failure record where it replaces the + credential, beside the quota and needs-reauth clears that were already there. The store already + cleared on a successful refresh and on deletion; replacement was the missing case. Keying the + cooldown by account id alone stays latent — a stale in-flight refresh of the old generation can + still record a failure after the clear — and is left for a generation-fencing change rather than + widened here. +- The file-size ratchet gets its six former god-files back at their current sizes + (`src/codex/routing.ts` 1626, `src/responses/state.ts` 1371, `src/codex/shim.ts` 1246, + `src/codex/inject.ts` 987, `src/providers/quota.ts` 558, `src/codex/catalog/sync.ts` 52). They + had been dropped from the cap list when they fell under the 2,000-line threshold, so the files + this whole decomposition programme exists to shrink were the only ones free to grow back. + +The remaining accepted risks are unchanged: two guards with false-negative shapes, eleven +hand-written files exempted as "generated", the unwired spend ledger, the adapter path reporting +budget exhaustion as a 502, and the file-only account-change scrub. None is a regression in this +range, and each is written down here rather than carried silently into the release. + +### The fix itself needed a second round + +The release-decision review caught that the first permit fix moved the leak rather than closing it. +Confirming the hop with `use()` immediately before `rebuildAndRefetch` looked right, but that +function returns `{ failed }` when `buildRequest` throws — a request-shaping failure that never +reaches the wire — and the outer `catch` never sees it, so the charge stayed for a send that never +happened. + +The hop is now confirmed by a callback the rebuild invokes at its own dispatch boundary, after the +request is shaped and immediately before `noteAttemptSend`, and the `{ failed }` arm releases: +a no-op when the boundary was reached, a refund when the rebuild died before it. That boundary is +also the honest place to name, because it is the line where "we are about to send" becomes true. + +Two residuals stay recorded rather than closed. The permit guards are source oracles: they pin the +control flow at the boundary, not the budget arithmetic under an injected failure, because +exercising that path needs a rotation fixture with a throwing snapshot fetch. And the cooldown fix +has a source oracle for the caller plus a unit case for the store, where an integration test +through the existing mock OAuth harness could assert eligibility directly after a reauthentication. + +### Closing the cooldown race rather than accepting it + +The release-decision review also pointed out that clearing on replacement is mitigation, not +elimination: a refresh flight already in the air when the reauthentication lands still fails +afterwards, and its late report would re-quarantine the credential that replaced the one it was +about. Relative to 2.55.0, which had no cooldown at all, that is a new user-visible exclusion, so +it is fixed rather than written down. + +`clearCodexPoolRefreshFailure` now bumps a per-account fence, a refresh flight captures that fence +before it settles, and a failure reporting a stale fence is dropped. A failure of the NEW credential +still counts, so the bound the cooldown exists to enforce is unchanged. `clearAllCodexPoolRefreshFailures` +deliberately does not bump: it is the coarse reset the routing layer performs when it discards +per-account state, and a later genuine failure should still count against the account. + +### Third round on the same fix + +An interdiff audit of the shipping tree — not the tree the audit started from — found the boundary +was still one step too early. `onDispatch` fired before `waitForProviderRequestSlot`, and that wait +rejects for an abort, a saturated queue, an expired slot or a removed provider without ever calling +the adapter. Since `release()` is a no-op once used, neither the `{ failed }` arm nor the catch +could refund that no-send case. + +The hop is now confirmed at the two places that actually reach the wire: after the pacing wait and +immediately before `fetchResponse`, and inside the retry thunk immediately before +`fetchWithHeaderTimeout`. The guard pins both orderings rather than the single textual placement it +pinned before, which is what let the earlier version pass. + +The same audit recorded one High finding that is **not** from this change and is accepted with the +others: the hop reservation and the adapter's own budget can both charge one physical replay, +because the hop is not handed down through `pendingHopPermit` the way the passthrough ladder does +it, and Kiro reserves again immediately before its send. That is the same #4546 accounting +incompleteness already listed above, it predates this range's fix, and closing it means threading +the permit through the adapter boundary rather than widening this patch. diff --git a/devlog/_plan/260915_2560_release_train/030_release.md b/devlog/_plan/260915_2560_release_train/030_release.md index 44062b9b40..9fc8005a4a 100644 --- a/devlog/_plan/260915_2560_release_train/030_release.md +++ b/devlog/_plan/260915_2560_release_train/030_release.md @@ -2,36 +2,48 @@ ## Preconditions -- wp2 closed: #4683 on `dev` with Cross-platform CI green at its exact head. -- wp3 closed: no open REGRESSION finding. -- `dev` carries 2.56.0 (`dev-version-bump` owns that line). +- wp2 closed: #4683 on `dev`, Cross-platform CI green at its exact head. +- wp3 closed: no open REGRESSION finding, and a recorded go decision. +- The release candidate SHA is frozen: `2702911708`, which reads 2.56.0 in `package.json`. ## Sequence -The order is forced by two gates in `.github/workflows/release.yml`, not by preference. - -1. Record the `dev` tip and its Cross-platform CI conclusion at that exact SHA. -2. Cut the promotion branch from that `dev` commit — it still reads 2.56.0 — and open its PR to - `main`. Merge it. That merge commit is the release SHA `M1`. -3. Confirm Cross-platform CI succeeded for `M1` on `main`. `release.yml` requires a successful - run for the dispatched commit (`Require successful Cross-platform CI for this commit`), and - `Service lifecycle` too when service files changed in the range. -4. Dispatch `dev-version-bump.yml` with `intended-version: 2.56.0`, mode `pre-move`. It opens a - PR moving `dev` to the next line; merge it. This is not optional: `release.yml` ends with - `Require dev to be ready for this release`, which runs +The order is forced by `MAINTAINERS.md` lines 84-91 and by three gates in +`.github/workflows/release.yml`. It is written here because the first version of this document +had it backwards. + +1. **Freeze the candidate.** `2702911708`. Everything below publishes that tree and nothing else. +2. **Move `dev`'s version line first.** Dispatch `dev-version-bump.yml` with + `intended-version: 2.56.0`, mode `pre-move`, and merge the pull request it opens. `release.yml` + ends with `Require dev to be ready for this release`, which runs `version-line.ts assert-ahead ` and refuses to publish while - `dev` still equals 2.56.0. -5. Dispatch `release.yml` with `version: 2.56.0` and `expected-sha: M1`. The workflow refuses any - dispatch whose `GITHUB_SHA` differs from `expected-sha`, so the branch must not move between - step 3 and here. -6. Verify the publish from the workflow's own conclusion. Registry metadata can lag a successful - publish; a lagging read is not a reason to publish again. + `dev` still reads 2.56.0. Doing this after publication is what left `dev` and every open pull + request carrying a failure contributors could not fix from their own diff, ten times. +3. **Promote the frozen candidate to `main`.** The promotion branch is cut from `2702911708`, not + from the post-bump `dev` tip, so `main` receives 2.56.0 rather than the next line. Its + `enforce-target` check fails with "wrong base (main)" — that gate exists for feature pull + requests and every promotion carries the same red mark; the 2.55.0 promotion #4619 merged in + exactly that state. +4. **Prove the release SHA.** `release.yml` requires a successful Cross-platform CI run for the + dispatched commit, and a successful Service lifecycle run for it as well whenever + `src/service.ts`, `src/cli.ts`, `src/cli/index.ts`, `src/lib/bun-runtime.ts`, `package.json`, + `bun.lock` or either of those two workflow files changed since the previous tag. `package.json` + always changes across a release, so Service lifecycle is always required here. +5. **Dispatch `release.yml`** with `version: 2.56.0`, `tag: latest`, `dry-run: false` and + `expected-sha` set to the `main` release commit. The workflow refuses any dispatch whose + `GITHUB_SHA` differs, so the branch must not move between step 4 and here. +6. **Promote to `preview`.** `preview` currently carries `2.55.0-preview.20260914`; bringing it + onto the released tree keeps the prerelease train from restating a shipped stable. +7. **Verify the publish from the workflow's own conclusion.** Registry metadata can lag a + successful publish; a lagging read is not a reason to publish again. ## Evidence Recorded as each step completes: SHA, run id, conclusion. -- wp2 head under CI: `4e548b693c` (previous heads `27c61e2dfb`, `9dffc3f06f`, `35ad194ec2` - superseded; `35ad194ec2` failed the file-size ratchet on - `tests/responses/responses-state.test.ts` and was fixed by removing the three added lines rather - than raising the cap). +- Candidate `2702911708`. #4683 landed at head `d8ef6ee9b889e51e5d3e547d60a537b8fbecfb85` with + Cross-platform CI run `34935526979` success; two earlier heads were superseded, and the last of + them failed the file-size ratchet on `tests/responses/responses-state.test.ts`, which was fixed + by removing the three added lines rather than by raising the baseline. +- Pre-move pull request: #4686 (`dev` to 2.57.0). +- Promotion pull request to `main`: #4687, cut from the frozen candidate. diff --git a/devlog/_plan/260915_2560_release_train/040_release_decision.md b/devlog/_plan/260915_2560_release_train/040_release_decision.md new file mode 100644 index 0000000000..971af86c6c --- /dev/null +++ b/devlog/_plan/260915_2560_release_train/040_release_decision.md @@ -0,0 +1,46 @@ +# wp4 — the release decision, and then the release + +## Decision: GO, on the post-fix candidate + +The audit did not clear 2702911708. It cleared the tree that carries the two fixes it produced, so +the release candidate moved: whatever commit lands #4690 on `dev` is what gets promoted, and +2702911708 is now only the commit the audit started from. The promotion opened earlier from +2702911708 (#4687) is stale for the same reason and has to be re-cut. + +What the decision rests on, and what it does not: + +- Twelve god-file decompositions audited and clean, each with a mechanical argument rather than an + impression — declaration parity counts, single-owner state inventories, restore-completeness + enumerations, and four traced end-to-end paths. Two independent models were run 1:1 on the + highest-risk slices and agreed. +- Two real regressions found, fixed, guarded and reviewed. Both were in the #4546 work; neither was + in a split. +- Six risks recorded and accepted in writing, none of them a regression in this range. +- The residual that no amount of source reading discharges: whether the candidate typechecks, + builds, and behaves under real streaming, cancellation, replay and concurrency. That is carried + by hosted CI at the exact release SHA, and it is the reason no step below accepts a green from a + different commit. + +## Sequence + +1. Land #4690 on `dev` with Cross-platform CI green at its exact head. That merge commit is the + release candidate. +2. Merge the `dev` version pre-move (#4686) so `dev` outranks 2.56.0 — `release.yml` refuses to + publish otherwise, and doing this after publication is what left `dev` and every open pull + request carrying a version-line failure ten times before. +3. Re-cut the promotion branch from the new candidate and open it against `main`. Its + `enforce-target` check fails with "wrong base (main)"; every promotion carries that mark. +4. Require Cross-platform CI success for the `main` release commit, and Service lifecycle for it + too — `package.json` always changes across a release, so that gate always applies here. +5. Dispatch `release.yml` with `version: 2.56.0`, `tag: latest`, `dry-run: false`, and + `expected-sha` equal to the `main` release commit. The workflow refuses any dispatch whose + `GITHUB_SHA` differs, so nothing may move between step 4 and here. +6. Promote the released tree to `preview`, which currently carries `2.55.0-preview.20260914`. +7. Verify the publish from the workflow's own conclusion. Registry metadata can lag a successful + publish; a lagging read is not a reason to publish twice. + +## Evidence + +Recorded as each step completes. + +- #4690 head `0026b14e83`, the post-fix candidate. diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index b3c6313d83..1e8885ae11 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -20,6 +20,7 @@ import { CODEX_REFRESH_FLIGHT_CEILING_MS } from "./quota-recovery-timing"; import { CodexPoolRefreshCooldownError, clearCodexPoolRefreshFailure, + codexPoolRefreshFence, isCodexPoolRefreshCooling, noteCodexPoolRefreshFailure, } from "./pool-refresh-backoff"; @@ -851,6 +852,10 @@ export async function forceRefreshCodexPoolToken( // the credential, not for whoever happened to be waiting. undefined, ); + // Captured before the flight settles, spent only if it fails. A reauthentication that lands + // while this is in the air replaces the grant and clears its failures; this fence is how the + // late failure knows it is talking about a credential that no longer exists. + const refreshFence = codexPoolRefreshFence(id); completion.then( resolved => { clearCodexPoolRefreshFailure(id); @@ -865,7 +870,7 @@ export async function forceRefreshCodexPoolToken( if (isTerminalCodexPoolRefreshFailure(error) || isOperationalCodexPoolRefreshFailure(error)) { if (isTerminalCodexPoolRefreshFailure(error)) clearCodexPoolRefreshFailure(id); } else { - noteCodexPoolRefreshFailure(id, classifyCodexPoolRefreshFailureReason(error)); + noteCodexPoolRefreshFailure(id, classifyCodexPoolRefreshFailureReason(error), undefined, refreshFence); } settle({ kind: "failed", error }); }, diff --git a/src/codex/auth-api/login-flow.ts b/src/codex/auth-api/login-flow.ts index 663d68e68f..ad384e9539 100644 --- a/src/codex/auth-api/login-flow.ts +++ b/src/codex/auth-api/login-flow.ts @@ -7,6 +7,7 @@ import { appendDefaultCodexAccountNamespace, codexAccountPickerEnabled } from ". import { catalogRefreshIsPending, normalizeCatalogDisposition } from "../catalog-refresh-status"; import { checkAccountIdCollision } from "../auth-collision"; import { clearAccountNeedsReauth, isAccountNeedsReauth, markAccountNeedsReauth } from "../account-runtime-state"; +import { clearCodexPoolRefreshFailure } from "../pool-refresh-backoff"; import { reconcileLiveStateStores } from "../../lib/state-store-registrations"; import { emailMaskingEnabled, projectEmail } from "../../lib/privacy"; import { codexWarmupFailureReason, isCodexWarmupProvisioningFailure, warmCodexAccount } from "../warmup"; @@ -365,6 +366,13 @@ export async function handleCodexAuthLoginStart(req: Request, config: OcxConfig, // A successful reauthentication replaces the credential generation. Do not let a // failed optional WHAM probe make the replacement inherit quota from the old record. if (reauth) clearAccountQuota(accountId); + // The refresh cooldown is learned about a CREDENTIAL, not about an account, and it + // is keyed by account id alone. A replacement generation therefore inherits the + // dead one's 15-60s quarantine: selection keeps excluding an account that was just + // authenticated, and with a healthy sibling the thread detours and loses its warm + // cache and continuation. A successful save is the proof the old failures were + // about a credential that no longer exists. + clearCodexPoolRefreshFailure(accountId); if (warmup.validatedAt !== undefined) markCodexAccountValidated(accountId, warmup.validatedAt, generation); clearAccountNeedsReauth(accountId); if (quota) setAccountQuotaFromParsed(accountId, quota); diff --git a/src/codex/pool-refresh-backoff.ts b/src/codex/pool-refresh-backoff.ts index 410d11600a..d9474ceb21 100644 --- a/src/codex/pool-refresh-backoff.ts +++ b/src/codex/pool-refresh-backoff.ts @@ -36,6 +36,13 @@ type RefreshFailureBackoff = { }; const backoffByAccount = new Map(); +/** + * Bumped whenever an account's failures are cleared because something proved them obsolete — a + * successful refresh, or a replacement credential written by login/reauth. A refresh flight that + * started before that moment is reporting on a grant that no longer exists, and its late failure + * must not re-quarantine the credential that replaced it. + */ +const fenceByAccount = new Map(); let nowOverride: number | undefined; export function setCodexPoolRefreshFailureNowForTests(now?: number): void { @@ -44,11 +51,18 @@ export function setCodexPoolRefreshFailureNowForTests(now?: number): void { export function resetCodexPoolRefreshFailureBackoffForTests(): void { backoffByAccount.clear(); + fenceByAccount.clear(); nowOverride = undefined; } +/** The value a refresh flight captures before it starts, to be handed back on failure. */ +export function codexPoolRefreshFence(accountId: string): number { + return fenceByAccount.get(accountId) ?? 0; +} + export function clearCodexPoolRefreshFailure(accountId: string): void { backoffByAccount.delete(accountId); + fenceByAccount.set(accountId, (fenceByAccount.get(accountId) ?? 0) + 1); } /** @@ -101,8 +115,19 @@ export function noteCodexPoolRefreshFailure( accountId: string, reason: string, now = currentNow(), + fence?: number, ): { consecutiveFailures: number; cooldownUntil: number; openedWindow: boolean } { const existing = backoffByAccount.get(accountId); + // A flight that started before the account's failures were cleared is speaking for a grant + // that has since been replaced or proven healthy. Recording it would put the new credential + // back in the quarantine its predecessor earned. + if (fence !== undefined && fence !== codexPoolRefreshFence(accountId)) { + return { + consecutiveFailures: existing?.consecutiveFailures ?? 0, + cooldownUntil: existing?.cooldownUntil ?? 0, + openedWindow: false, + }; + } // The "do not grow inside an open window" rule applies only once the window is actually // WITHHOLDING. Below the threshold no refresh is being withheld, so every failure is a real // attempt that really failed and must count -- otherwise a client retrying the 503 once a diff --git a/src/server/responses/adapter-continuation.ts b/src/server/responses/adapter-continuation.ts index 5201af2d54..a1db9398d5 100644 --- a/src/server/responses/adapter-continuation.ts +++ b/src/server/responses/adapter-continuation.ts @@ -421,6 +421,11 @@ export function createAdapterContinuations( continue; } } catch { + // Everything in this try runs before the replay: the send happens on the next + // iteration, after `continue`. A throw here therefore leaves a reservation that + // never dispatched, and holding it would refuse a later recovery in this same + // request for a send that never left the process. + hop.permit?.release(); // fall through to emit continuation error below } } diff --git a/src/server/responses/adapter-dispatch.ts b/src/server/responses/adapter-dispatch.ts index e1757186cf..57eadf6a28 100644 --- a/src/server/responses/adapter-dispatch.ts +++ b/src/server/responses/adapter-dispatch.ts @@ -369,6 +369,13 @@ export async function prepareAdapterExchange( */ const rebuildAndRefetch = async ( recovery: AttemptRecoveryKind, + /** + * Called at the dispatch boundary — after the request is rebuilt and shaped, immediately + * before the send. A caller holding a reserved hop confirms it here rather than before the + * rebuild, because a build failure returns `{ failed }` without ever reaching the wire and + * a permit confirmed earlier would keep the charge for a send that never happened. + */ + onDispatch?: () => void, ): Promise => { let retryRequest: AdapterRequest; if (transportState.sameTargetRequest !== undefined && transportState.sameTargetParsed === parsed && transportState.sameTargetToken === transportState.transportToken) { @@ -410,6 +417,11 @@ export async function prepareAdapterExchange( try { if (transportState.activeAdapter.fetchResponse) { await waitForProviderRequestSlot(route.providerName, route.provider, route.modelId, upstream.signal); + // The dispatch boundary is HERE, not before the pacing wait: that wait can reject for + // an abort, a saturated queue, an expired slot or a removed provider, and none of + // those reach the wire. Confirming earlier would hold the charge for a send that the + // pacer refused. + onDispatch?.(); return await transportState.activeAdapter.fetchResponse(retryRequest, { abortSignal: upstream.signal, timeoutMs: connectMs, @@ -449,6 +461,9 @@ export async function prepareAdapterExchange( if (refetchAllowance?.permit && !refetchAllowance.permit.use()) { throw new SendBudgetExhaustedError(safeHostLabel(retryRequest.url)); } + // Same boundary on the helper path: the thunk is what reaches the wire, and it + // can be refused above before it does. use() past the first attempt is a no-op. + onDispatch?.(); return fetchWithHeaderTimeout(retryRequest.url, applyUpstreamRecoveryInit({ method: retryRequest.method, headers: retryRequest.headers, body: retryRequest.body, @@ -740,10 +755,23 @@ export async function prepareAdapterExchange( ); sealRequestAttemptIdentity(logCtx.activeAttempt, logCtx.provider, transportState.activeAdapter.name, logCtx.accountLogLabel); recordAttemptCredentialSource(logCtx.activeAttempt, route.providerName, route.provider, transportState.activeAdapter.name); - const result = await rebuildAndRefetch("oauth-account-429"); - if ("failed" in result) return result.failed; + // Confirm at the dispatch boundary, not here: a rebuild can fail while shaping the + // request and return `{ failed }` without reaching the wire, and a permit confirmed + // before that would hold the charge for a send that never happened. + const result = await rebuildAndRefetch("oauth-account-429", () => { hop.permit?.use(); }); + if ("failed" in result) { + // A no-op if the boundary was reached; a refund if the rebuild died before it. + hop.permit?.release(); + return result.failed; + } upstreamResponse = result; } catch { + // A throw before the send — snapshot fetch, credential application, adapter + // resolution — must hand the reservation back. Without this the ladder charges the + // request for a send it never made, and a later recovery in the same request is + // refused on an allowance nothing spent. release() is idempotent and a no-op once + // used, so a throw from the rebuild keeps its charge. + hop.permit?.release(); break; } } diff --git a/structure/transports/responses.md b/structure/transports/responses.md index 6f0276696b..99156b4402 100644 --- a/structure/transports/responses.md +++ b/structure/transports/responses.md @@ -673,3 +673,23 @@ acyclic dependencies, recursive dispatch, lease-transfer wiring, capture-name hy send-holder/permit behavior. Cross-owner source assertions read the actual implementations via `tests/helpers/responses-core-source.ts`; focused passthrough and subagent assertions read their specific delivery/preparation owner. Existing runtime Lab-boundary tests still start at `core.ts`. + +## Credential-hop reservations + +A credential rotation inside one provider's roster reserves a hop from the request's shared send +budget before it knows whether a rotation is even possible, because the reservation is the charge: +`reserveDispatch` spends, `permit.use()` only confirms which leg sent, and `permit.release()` is +idempotent and a no-op once used. Every ladder therefore owes the budget an answer on every exit. + +Two shapes are correct and both are in the tree. Where the ladder dispatches inside its own `try` +— `adapter-dispatch.ts`, `run-turn-execution.ts` — it confirms with `use()` immediately before the +send and releases in its `catch`, so one catch covers a pre-dispatch throw and a throw from the +send alike. Where the replay happens after the loop continues — `adapter-continuation.ts` — it must +not confirm, because the send has not happened yet; it only releases. The passthrough ladder is a +third shape: it reserves with `countedExternally: true` and hands the permit to the rebuild through +`pendingHopPermit`, because there the retry helper reports the same physical send. + +What must not happen is a ladder that charges and then returns through a path that neither confirms +nor releases. That is not a lost send; it is a send the request never made, spending an allowance a +later recovery in the same request then cannot have. `tests/lib/execution-budget-permits.test.ts` +pins both ladder shapes against exactly that. diff --git a/tests/codex-integration/codex-pool-refresh-backoff.test.ts b/tests/codex-integration/codex-pool-refresh-backoff.test.ts index eca22b97a8..1b13db292b 100644 --- a/tests/codex-integration/codex-pool-refresh-backoff.test.ts +++ b/tests/codex-integration/codex-pool-refresh-backoff.test.ts @@ -1,9 +1,11 @@ import { describe, expect, test, beforeEach } from "bun:test"; +import { readFileSync } from "node:fs"; import { CODEX_POOL_REFRESH_COOLDOWN_AFTER_FAILURES, CODEX_POOL_REFRESH_FAILURE_BACKOFF_MS, CodexPoolRefreshCooldownError, clearCodexPoolRefreshFailure, + codexPoolRefreshFence, getCodexPoolRefreshCooldownUntil, isCodexPoolRefreshCooling, noteCodexPoolRefreshFailure, @@ -128,3 +130,93 @@ describe("terminal has one definition", () => { }); }); + +/** + * The cooldown is learned about a CREDENTIAL and keyed by account id alone, so a replacement + * generation inherited the dead one's quarantine: an account that had just been reauthenticated + * stayed out of selection for up to a minute, and with a healthy sibling the thread detoured and + * lost its warm cache and continuation. Clearing on a successful refresh was already there + * (`account-store`); clearing on a successful credential REPLACEMENT was not. + * + * The behaviour is asserted at the unit below; the oracle is what pins the caller, because a + * store-level test cannot see a login path that forgets to call it. + */ +describe("a replacement credential does not inherit the failed one's cooldown", () => { + test("clearing after the cooldown opened restores eligibility immediately", () => { + const now = 2_000_000; + setCodexPoolRefreshFailureNowForTests(now); + for (let attempt = 0; attempt < CODEX_POOL_REFRESH_COOLDOWN_AFTER_FAILURES; attempt += 1) { + noteCodexPoolRefreshFailure("acct-reauth", "unknown"); + } + expect(isCodexPoolRefreshCooling("acct-reauth")).toBe(true); + clearCodexPoolRefreshFailure("acct-reauth"); + expect(isCodexPoolRefreshCooling("acct-reauth")).toBe(false); + expect(getCodexPoolRefreshCooldownUntil("acct-reauth")).toBeNull(); + setCodexPoolRefreshFailureNowForTests(undefined); + }); + + test("the login path clears it where it replaces the credential", () => { + const source = readFileSync( + new URL("../../src/codex/auth-api/login-flow.ts", import.meta.url), + "utf8", + ); + const save = source.indexOf("saveCodexAccountCredential(accountId, credential"); + const settled = source.indexOf("clearAccountNeedsReauth(accountId)", save); + expect(save).toBeGreaterThan(-1); + expect(settled).toBeGreaterThan(save); + // Same block that already drops the stale quota and the needs-reauth flag: the refresh + // cooldown belongs with them, because the credential those failures were about is gone. + expect(source.slice(save, settled)).toContain("clearCodexPoolRefreshFailure(accountId)"); + }); +}); + +/** + * Clearing on replacement is only half the fix. A refresh flight that started before the + * reauthentication is still in the air, and its late failure would have re-quarantined the + * credential that replaced the one it was actually about — the same 15-60s exclusion, arriving + * a moment after the account was let back in. + */ +describe("a late failure from the replaced credential cannot re-cool the new one", () => { + test("a stale fence is ignored and a current one still counts", () => { + const now = 3_000_000; + setCodexPoolRefreshFailureNowForTests(now); + const staleFence = codexPoolRefreshFence("acct-fenced"); + for (let attempt = 0; attempt < CODEX_POOL_REFRESH_COOLDOWN_AFTER_FAILURES; attempt += 1) { + noteCodexPoolRefreshFailure("acct-fenced", "unknown", undefined, staleFence); + } + expect(isCodexPoolRefreshCooling("acct-fenced")).toBe(true); + + // The reauthentication lands: failures cleared, fence moved. + clearCodexPoolRefreshFailure("acct-fenced"); + expect(isCodexPoolRefreshCooling("acct-fenced")).toBe(false); + const freshFence = codexPoolRefreshFence("acct-fenced"); + expect(freshFence).not.toBe(staleFence); + + // The old flight finally fails. It is speaking for a grant that no longer exists. + for (let attempt = 0; attempt < CODEX_POOL_REFRESH_COOLDOWN_AFTER_FAILURES; attempt += 1) { + noteCodexPoolRefreshFailure("acct-fenced", "unknown", undefined, staleFence); + } + expect(isCodexPoolRefreshCooling("acct-fenced")).toBe(false); + + // A failure of the NEW credential still counts, so the bound is not weakened. + for (let attempt = 0; attempt < CODEX_POOL_REFRESH_COOLDOWN_AFTER_FAILURES; attempt += 1) { + noteCodexPoolRefreshFailure("acct-fenced", "unknown", undefined, freshFence); + } + expect(isCodexPoolRefreshCooling("acct-fenced")).toBe(true); + setCodexPoolRefreshFailureNowForTests(undefined); + }); + + test("the refresh flight captures the fence before it settles", () => { + const source = readFileSync( + new URL("../../src/codex/account-store.ts", import.meta.url), + "utf8", + ); + const captured = source.indexOf("codexPoolRefreshFence(id)"); + const reported = source.indexOf("noteCodexPoolRefreshFailure(id,"); + expect(captured).toBeGreaterThan(-1); + // Captured before the settlement that spends it, not read at failure time — reading it late + // would return the post-reauthentication value and defeat the fence. + expect(reported).toBeGreaterThan(captured); + expect(source.slice(reported, reported + 200)).toContain("refreshFence"); + }); +}); diff --git a/tests/fixtures/file-size-baseline.json b/tests/fixtures/file-size-baseline.json index 56835250c9..462abdc1f5 100644 --- a/tests/fixtures/file-size-baseline.json +++ b/tests/fixtures/file-size-baseline.json @@ -22,8 +22,14 @@ "src/bridge.ts": 7, "src/codex/auth-api.ts": 43, "src/codex/catalog/provider-fetch.ts": 54, + "src/codex/catalog/sync.ts": 52, + "src/codex/inject.ts": 987, + "src/codex/routing.ts": 1626, + "src/codex/shim.ts": 1246, "src/config.ts": 460, + "src/providers/quota.ts": 558, "src/providers/registry.ts": 232, + "src/responses/state.ts": 1371, "src/server/index.ts": 893, "src/server/responses/core.ts": 210, "tests/ci-workflows/ci-workflows.test.ts": 5628, diff --git a/tests/lib/execution-budget-permits.test.ts b/tests/lib/execution-budget-permits.test.ts index 2276c921ae..6aff76df69 100644 --- a/tests/lib/execution-budget-permits.test.ts +++ b/tests/lib/execution-budget-permits.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; import { CODEX_TEXT_GUARDED_BUDGET_POLICY, createRequestExecutionBudget, @@ -196,3 +197,72 @@ describe("layer caps intersect the shared budget", () => { expect(budget.reserveSpent).toBe(false); }); }); + +/** + * The refund property above is only worth something if every caller actually uses it. + * + * The generic-OAuth 429 ladder reserves a hop before it knows whether a rotation is possible. + * Two of its three exits released correctly and the `catch` did not, so a throw from the + * snapshot fetch or from credential application charged the request for a send that never left + * the process — and a later recovery in the same request was then refused on an allowance + * nothing had spent. The passthrough and runTurn ladders already had it right; these two did not. + * + * This is a source oracle because the defect lives in the caller's control flow, not in the + * budget: a unit test of the budget cannot see a caller that forgets to hand the permit back. + */ +describe("generic-OAuth hop reservations are handed back when no send happens", () => { + // Bounded to each ladder's own span and matched on the catch that opens it. An earlier version + // of this test searched from the first following "catch {" and found the inline body-cancel + // catch instead, so it passed while the defect was still present. + const ladder = (relativePath: string, fromMarker: string, toMarker: string): string => { + const source = readFileSync(new URL("../../" + relativePath, import.meta.url), "utf8"); + const from = source.indexOf(fromMarker); + const to = source.indexOf(toMarker, from); + expect(from).toBeGreaterThan(-1); + expect(to).toBeGreaterThan(from); + return source.slice(from, to); + }; + const refundsOnThrow = /catch \{[^}]*hop\.permit\?\.release\(\)/; + + test("the adapter dispatch ladder confirms at the dispatch boundary and refunds otherwise", () => { + const source = readFileSync(new URL("../../src/server/responses/adapter-dispatch.ts", import.meta.url), "utf8"); + // Confirming before the rebuild is not enough: buildRequest failures return { failed } + // without reaching the wire, so the hop is confirmed by the callback the rebuild invokes at + // its dispatch boundary, and the { failed } arm refunds whatever that callback did not spend. + expect(source).toContain("onDispatch?.()"); + // Confirmed at the wire, not before the pacer: waitForProviderRequestSlot can reject for an + // abort, a saturated queue, an expired slot or a removed provider without ever calling the + // adapter, and release() is a no-op once used, so an early confirm could never be refunded. + const slotWait = source.indexOf("await waitForProviderRequestSlot("); + const confirmAfterWait = source.indexOf("onDispatch?.()", slotWait); + const adapterSend = source.indexOf("transportState.activeAdapter.fetchResponse(retryRequest", confirmAfterWait); + expect(slotWait).toBeGreaterThan(-1); + expect(confirmAfterWait).toBeGreaterThan(slotWait); + expect(adapterSend).toBeGreaterThan(confirmAfterWait); + // The helper path has the same boundary inside the thunk that reaches the wire. + const thunkConfirm = source.indexOf("onDispatch?.()", adapterSend); + const headerTimeout = source.indexOf("fetchWithHeaderTimeout(retryRequest.url", thunkConfirm); + expect(thunkConfirm).toBeGreaterThan(adapterSend); + expect(headerTimeout).toBeGreaterThan(thunkConfirm); + const block = ladder( + "src/server/responses/adapter-dispatch.ts", + "adapter-recovery-oauth-429", + "attemptOpaqueBlobRecovery", + ); + expect(block).toContain('rebuildAndRefetch("oauth-account-429", () => { hop.permit?.use(); })'); + expect(block).toMatch(/if \("failed" in result\) \{[^}]*hop\.permit\?\.release\(\)/); + expect(block).toMatch(refundsOnThrow); + }); + + test("the continuation ladder refunds, because its send happens after the loop continues", () => { + const block = ladder( + "src/server/responses/adapter-continuation.ts", + "continuation-oauth-429", + "shouldAttemptImageTierRetry", + ); + // Nothing in that try dispatches: the replay is the next iteration, so a throw must return + // the reservation rather than confirm it. + expect(block).not.toContain("hop.permit?.use()"); + expect(block).toMatch(refundsOnThrow); + }); +});