From 902a1ede6754f148e21df9b10239f212d6f3e361 Mon Sep 17 00:00:00 2001 From: JUN Date: Tue, 15 Sep 2026 15:42:15 +0900 Subject: [PATCH 1/3] docs(devlog): repair the 2.56.0 release-train roadmap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pins the frozen candidate 2702911708 and enumerates all nine commits of the range, so "every commit was audited" is checkable. Restates the release sequence in the order MAINTAINERS.md and the release workflow gates actually force — the dev version pre-move comes first — and adds the preview promotion. Records the landed #4683 evidence: head d8ef6ee9b8, CI run 34935526979, squash 2702911708. --- .../260915_2560_release_train/000_roadmap.md | 68 ++++++++++++++----- .../010_land_4683.md | 59 +++++++++++----- .../020_regression_audit.md | 16 +++++ .../260915_2560_release_train/030_release.md | 60 +++++++++------- 4 files changed, 144 insertions(+), 59 deletions(-) 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..7c16962f97 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,19 @@ 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 + 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. From eb21c4416b6e9cd4d2bd574c80a4001f126a976c Mon Sep 17 00:00:00 2001 From: JUN Date: Tue, 15 Sep 2026 16:21:24 +0900 Subject: [PATCH 2/3] docs(devlog): record the 2.56.0 regression audit and its verdicts Nineteen slices over the true 59-commit range, run on gpt-5.6-sol and paired onto xai/grok-4.6 after sol began refusing parallel fan-out. Twelve god-file decompositions clean; two real regressions in the #4546 work; five risks accepted as non-regressions. Includes the per-commit coverage map and the shallow-clone lesson that corrected the range. --- .../020_regression_audit.md | 157 ++++++++++++++++++ 1 file changed, 157 insertions(+) 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 7c16962f97..a418b12e04 100644 --- a/devlog/_plan/260915_2560_release_train/020_regression_audit.md +++ b/devlog/_plan/260915_2560_release_train/020_regression_audit.md @@ -89,3 +89,160 @@ 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. From 26b3ff244434846149b560e28f7441afae529564 Mon Sep 17 00:00:00 2001 From: JUN Date: Tue, 15 Sep 2026 16:38:45 +0900 Subject: [PATCH 3/3] fix(responses,codex): stop charging a send that never happened, and let a reauthenticated account back in The 2.56.0 regression audit found two defects in the #4546 work. Neither is in any of the twelve god-file decompositions the audit spent most of its budget on. The generic-OAuth 429 ladder reserves a hop before it knows whether a rotation is possible, and the reservation is the charge. Its two explicit early-outs released the permit; its catch did not, so a throw from the snapshot fetch or from credential application spent an allowance on a send that never left the process, and a later recovery in the same request was refused because of it. adapter-dispatch now confirms with use() immediately before the rebuild that spends the permit and releases in its catch -- release() is a no-op once used, so one catch covers both halves. adapter-continuation only releases, because its replay is the next loop iteration and confirming before continue would charge a hop that never ran. run-turn-execution already had this shape. The pool refresh cooldown is learned about a credential but keyed by account id alone, so a successful reauthentication inherited the dead credential's 15-60s quarantine: selection kept excluding an account that had just been authenticated, and with a healthy sibling the thread detoured and lost its warm cache and continuation. login-flow now clears the refresh-failure record where it replaces the credential, beside the quota and needs-reauth clears already there. Generation-fenced keying stays open and is noted. The file-size ratchet also gets its six former god-files back at their current sizes. They were dropped from the cap list when they fell under the 2,000-line threshold, which left the files the decomposition programme exists to shrink as the only ones free to grow back. --- .../020_regression_audit.md | 82 +++++++++++++++++ .../040_release_decision.md | 46 ++++++++++ src/codex/account-store.ts | 7 +- src/codex/auth-api/login-flow.ts | 8 ++ src/codex/pool-refresh-backoff.ts | 25 +++++ src/server/responses/adapter-continuation.ts | 5 + src/server/responses/adapter-dispatch.ts | 32 ++++++- structure/transports/responses.md | 20 ++++ .../codex-pool-refresh-backoff.test.ts | 92 +++++++++++++++++++ tests/fixtures/file-size-baseline.json | 6 ++ tests/lib/execution-budget-permits.test.ts | 70 ++++++++++++++ 11 files changed, 390 insertions(+), 3 deletions(-) create mode 100644 devlog/_plan/260915_2560_release_train/040_release_decision.md 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 a418b12e04..1fa18c6238 100644 --- a/devlog/_plan/260915_2560_release_train/020_regression_audit.md +++ b/devlog/_plan/260915_2560_release_train/020_regression_audit.md @@ -246,3 +246,85 @@ package allowlist and the CI path filters rather than assumed. 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/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); + }); +});