diff --git a/devlog/_plan/260917_l3_retry_budget_admission/000_roadmap.md b/devlog/_plan/260917_l3_retry_budget_admission/000_roadmap.md new file mode 100644 index 0000000000..ea07668f05 --- /dev/null +++ b/devlog/_plan/260917_l3_retry_budget_admission/000_roadmap.md @@ -0,0 +1,75 @@ +# L3 — retry, admission and combo-recovery integration + +## Why this unit exists + +Four open pull requests all widen what this proxy is willing to send again. Read one at a time +each looks reasonable; read together they are the same question asked four ways, and the question +is not "does the retry work". It is whether a retry that works still has a bound on what it costs. + +The integrating principle for this unit is therefore **not** "retry better". It is: a retry must +leave cost, waiting and duplicate execution bounded. Concretely, three properties have to survive +every change here. + +1. **One physical upstream send is charged exactly once.** Not one adapter entry, not one logical + attempt. The nested ladder that #4546 measured stayed invisible precisely because an adapter + that sent three times reported one. +2. **Waiting is finite and cancellable.** A recovery path that sleeps must have a ceiling that + does not depend on what upstream chooses to report, and it must observe the client's abort. +3. **Nothing is replayed after it became observable.** Once a tool call has executed upstream or a + response has been committed to the client, no path may quietly send the same turn somewhere + else. A path that can do that is a blocker for this unit regardless of its other merits. + +## The four pull requests + +| PR | Head at survey | Area | What it widens | +| --- | --- | --- | --- | +| #4865 | `22130fbed5` | `src/adapters/` | Adapter-owned retry ladders admit through the request send budget | +| #4800 | `af985d3d13` | `src/providers/key-failover.ts` | Opt-in transient-5xx replay reaches `openai-responses` key-auth providers | +| #4817 | `7b0d51bafe` | `src/server/responses/combo-stream-preflight.ts` | A zero-output bare SSE `error` may advance a combo | +| #4824 | `2744efb6be` | `src/server/responses/core-combo.ts` | A single-target combo may retry its one target after its cooldown | + +### They are not a stack + +The obvious reading is that #4817 and #4824 collide, because both are described as "combo +failover". They do not. #4817 edits `combo-stream-preflight.ts` — how a streamed attempt is +*classified* before any output is committed. #4824 edits `core-combo.ts` — what the target loop +*does* once a failure has already been classified. The two files are disjoint, and the merge bases +confirm it: the change sets share no path. + +So this unit verifies each PR independently and does not serialise them into one chain. A stack +would buy nothing and would make three PRs wait on the slowest one. + +## Order of work + +**wp1 — #4865 to completion.** It is first because it is the one that installs the bound the other +three spend. It is also the narrowest: it is not a resubmission of the closed #4621, whose budget +core (`adapterDispatchBudget`, `pendingHopPermit`, `permit.assumeCharge()`) is already on `dev`. +What is left is the three ladders that still issued bare fetches — mimo-free's 401 JWT replay, +command-code's reasoning-effort repair, and the shared google-http transient loop. + +**wp2 — #4800, #4817, #4824 in parallel.** Independent verification, each against its own question: + +- #4800: does the widened replay stay inside key-auth `openai-responses`, or can it reach another + auth mode or another transport? +- #4817: is the first-committed-output / error / terminal verdict stable across SSE chunk + boundaries, or can a split frame change the decision? +- #4824: do wait time, cancellation and retry count all terminate? + +**wp3 — evidence.** Each PR ends open, rebased on the current `dev`, with Cross-platform CI +evidence at its exact head. This lane does not merge, does not push to `dev`, and does not rebase +anything outside these four heads. + +## Constraints this lane accepted + +Verification is static plus hosted CI only. No local suite, typecheck, build, install or `ocx` +invocation is used to reach a conclusion here, so every claim below has to be either a source +reading with a cited path or a hosted run at a named SHA. + +All four heads live in forks with maintainer-edit enabled, and Cross-platform CI on a fork pull +request lands in `action_required` until a maintainer approves the run. That approval is the +mechanism by which exact-head evidence exists at all; without it these PRs carry hygiene and +labeller checks and no test evidence. + +Flakiness is not a lever. No timeout widening, no added retry, no platform skip, no masking is +used to turn a red run green. The Windows leg is dispatch-only, so a change that reaches Windows +is reported rather than dispatched from inside this lane. diff --git a/devlog/_plan/260917_l3_retry_budget_admission/010_admission_audit.md b/devlog/_plan/260917_l3_retry_budget_admission/010_admission_audit.md new file mode 100644 index 0000000000..2208126651 --- /dev/null +++ b/devlog/_plan/260917_l3_retry_budget_admission/010_admission_audit.md @@ -0,0 +1,77 @@ +# wp1 — #4865, adapter-owned sends and the request budget + +## The question + +Three adapter retry ladders reached upstream without asking the request's send budget. The PR +routes them through `ctx.sendBudget`. The question for this lane is not whether that is a good +idea; it is whether the resulting accounting is exact in all three directions: one physical send +charged once, a send that never happens charged never, and a refusal that stays visible. + +## What the helper actually guarantees + +`createAdapterPhysicalSend` (`src/adapters/physical-send.ts`) reserves once per call and hands the +adapter an executor that can be used at most once. + +**One send, one charge.** The reservation happens before anything else, and the inner executor +carries a `dispatched` latch alongside `permit.use()`. A second call into the same executor throws +`SendBudgetExhaustedError` instead of quietly sending twice on one reservation. The charge itself +is not deferred to `use()` — `reserveDispatch` in `src/lib/request-execution-budget.ts` books the +spend at reservation time on purpose, because deciding and charging separately let two legs read +the same remainder and both dispatch. + +**No charge for a send that did not happen.** `permit.release()` in the `finally` returns the +booking whenever the permit was never used, and `release()` is a no-op once settled. Every exit +before dispatch — an aborted signal at entry, an abort observed after pacing, an abort observed +after `beforeDispatch`, or a throw from `beforeDispatch` itself — therefore refunds. + +**The order of operations is the load-bearing part.** Admission precedes the executor's pacing +slot, the backoff sleep, the JWT refresh and the cancellation of a superseded response, all of +which the PR moved behind `beforeDispatch`. A refused retry consequently pays neither a pacing +queue slot nor a backoff wait. + +**A refusal is not swallowed.** Each ladder catches `SendBudgetExhaustedError` and returns the +last real upstream response, with its status, `Retry-After` and quota body intact. That is the +established exhaustion contract, not a silent success: a refusal that never reached upstream at +all propagates, and `src/server/responses/adapter-dispatch.ts` answers it as `429` with +`SEND_BUDGET_EXHAUSTED_CODE` rather than mislabelling it `502` — which matters because the Codex +client retries `5xx` and does not retry `429`. + +**No double counting.** `onPhysicalSend` is observation only. `noteAdapterPhysicalSend` in +`src/server/responses/request-send-budget.ts` ignores ordinal 1 and records an attempt send for +the rest; it never touches the counter. + +## One defect found + +In `src/adapters/mimo-free.ts` the 401 replay drains the first response *after* refreshing the +JWT: + +``` +resetMimoJwtCache(); +const freshJwt = await getMimoJwt(ctx?.abortSignal); +retryHeaders = { ... }; +try { void response.body?.cancel().catch(() => {}); } catch { /* already consumed */ } +``` + +`getMimoJwt` performs its own network call and can throw. When it does, the error leaves +`fetchResponse` and the 401 response body is never drained — a leak the pre-change code did not +have, because it cancelled first and refreshed second. + +The fix is to restore that ordering inside `beforeDispatch` rather than outside it. The drain has +to stay behind admission: if the budget refuses the replay, the ladder returns that same 401 +response to its caller and its body must still be readable. Cancelling first *within* +`beforeDispatch` satisfies both, because `beforeDispatch` only ever runs after admission. + +## Two things that look like defects and are not + +**The google-http 429 peek now always clones.** It reads +`const peekTarget = res.clone()` where it used to read `res` directly unless `returnRawErrors` was +set. This is required: `pendingResponse` may have to be returned later, so the original body has +to survive the peek. It is also observationally identical on the quota-exhausted path, because +`formatMessage` already falls back with `payloadText || peek`. Before the change +`normalizeUpstreamHttpErrorResponse` re-read an exhausted body and got `""`, then used `peek`; +after it, `payloadText` is the same text `peek` holds. + +**The final `throw lastError ?? new Error(...)` cannot strand a `pendingResponse`.** Every +retryable-status path returns a normalised response on the last attempt, and every retry drains +the previous response in `beforeDispatch` before dispatching. The remaining exit is an abort, +which is already a discarded request. diff --git a/devlog/_plan/260917_l3_retry_budget_admission/020_independent_review.md b/devlog/_plan/260917_l3_retry_budget_admission/020_independent_review.md new file mode 100644 index 0000000000..a8de1c0940 --- /dev/null +++ b/devlog/_plan/260917_l3_retry_budget_admission/020_independent_review.md @@ -0,0 +1,73 @@ +# wp2 — the three recovery-widening pull requests + +Each is reviewed against one question, and each ends with its own exact-head evidence. They are +not chained. + +## #4800 — transient 5xx replay for key-auth `openai-responses` + +The change is two lines in `src/providers/key-failover.ts`: `transientRetryPolicyFor` stops +refusing the `openai-responses` adapter. + +The question is containment. `transientRetryOn5xx` is opt-in and absent by default, and the +auth-mode gate that follows the adapter gate is the fail-closed half — explicit `key` or the +documented omitted default, never OAuth, forward or local. What still has to be established is +that the `openai-responses` transport replays the same bytes it sent, that nothing in it carries +per-attempt server state, and that a stream which already emitted bytes is not a replay candidate. + +The user-facing documentation for `transientRetryOn5xx` names the eligible adapter. Eight locales +carry it under `docs-site/src/content/docs/**/reference/configuration/providers.md`, and the PR as +surveyed updates none of them. A behaviour widening whose documentation still says the old scope +is a docs-sync gap, not a nit. + +## #4817 — zero-output bare SSE errors may advance a combo + +The change classifies a top-level `{"type":"error"}` frame arriving before any output as terminal +evidence, and lets unknown, rate-limit and server-class failures advance to the next declared +target while explicit client errors stay committed. + +The question is boundary stability. A combo may only move while *nothing* has been committed to +the client, so the verdict has to be a function of the decoded event stream and not of how the +bytes were split. Three things decide it: that `createSseInspector` reassembles frames before the +payload callback sees them, that the new early return in `onParsedPayload` latches the first +verdict rather than letting a later frame overwrite it, and that `outputCommitted` is still set by +anything that reached the client. + +The failure mode to rule out is the one this unit calls a blocker: a frame arriving after a tool +call has already executed upstream, or after output was committed, being reclassified as +retryable and replayed against a different provider. + +## #4824 — a single-target combo may retry after its cooldown + +The change lets `executeComboResponses` re-pick when the combo declares exactly one target and +`waitForCooldownMs` is positive, by repeating `pickWithWait` without the `exclude` set that made +the first call return nothing. + +**Termination is settled.** Three independent bounds hold, and they are not restatements of one +another. + +- *Locally*, the new branch requires `comboTargetsDispatched <= 1`, and the dispatch that follows + makes it `2`. The branch cannot fire twice. +- *By budget*, `comboExecutionBudgetPolicy(1)` yields `maxAlternateTargetSends: 1`. The retry is + the first non-initial dispatch, so it is admitted; a second would be refused by + `reserveDispatch` in `src/lib/request-execution-budget.ts`. The per-target clamp is unaffected: + `combo.targets.length - 1 - comboTargetsDispatched` goes to `-1` and `comboTargetSendBudget` + clamps it with `Math.max(0, ...)`. +- *By wait*, `pickComboTargetWithWait` returns `null` outright when the earliest expiry exceeds + `waitForCooldownMs`, so the sleep is never longer than the configured ceiling, which + `src/combos/types.ts` validates to at most `600000`. + +**Cancellation is observed.** The wrapper passes `options.abortSignal` through, and +`pickComboTargetWithWait` returns `null` both when the sleep rejects and when the signal is already +aborted on wake; the caller then answers with the client-cancelled response. + +**Default behaviour does not move.** `COMBO_DEFAULT_WAIT_FOR_COOLDOWN_MS` is `0`, so a combo that +never configured a wait keeps failing on its first failure exactly as before. That is what the +third test in the PR pins. + +**No committed-output replay.** The branch sits on the failure path, which is only reached after a +non-2xx that the combo classifier already decided to hop on. A streamed attempt that committed +output returns before this point. + +The residual question is narrower than termination: whether repeating the same target is the right +answer for every status the classifier calls a hop, given that the retry re-sends the identical +turn to the identical provider.