fix(retry): refuse ambiguous reset replay without inviting a client retry (#4741) - #4798
Conversation
Redesigns luvs01#135 on upstream dev. Preserve total-send accounting and provider HTTP retry policy while carrying the terminal verdict through combo recovery and error formatting. Carried from #4621's sibling PR #4741 by a maintainer; original authorship preserved. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
…eserve verdict Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
An ambiguous pre-header reset is a refusal this proxy made, not something the upstream reported. It was borrowing `upstream_closed_before_response` and its 502, and the Codex client builds its policy from `retry_429: false` / `retry_5xx: true` over `DEFAULT_REQUEST_MAX_RETRIES = 4`, so the status invited up to four more sends of the exact turn the refusal exists to protect. It now carries `upstream_reset_replay_refused` and HTTP 429, following the `request_send_budget_exhausted` precedent. The distinct code keeps it separable from the WebSocket transport's post-send 502/504 verdicts, which are unchanged, and lets `formatErrorResponse` restate the status when a combo or adapter formatter re-wraps it holding an upstream-shaped 502. Because a 429 is no longer sufficient evidence of a provider rate limit, every same-target replay, key rotation, account rotation and pool-quota recorder that keys on 429 now consults `isNonReplayableResponse` first. Without that, correcting the status would have re-created the duplicate send inside native Chat and the continuation loop and written cooldowns against credentials that refused nothing. Compact records the transport outcome rather than the client-facing status, so pool health sees exactly what it saw before. `adapter-dispatch` already had the guard at the top of its recovery loop. Rewrites the owning section of structure/transports/responses.md to separate a pre-header rejection the proxy refuses to replay from an upstream reset seen mid-stream or after a terminal, and records the reclassification as the behaviour change it is. Also repairs the stale `## Upstream reset retry` section, which still described reset retries as the default and pointed at the pre-split `src/server/responses.ts`. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (24)
📝 WalkthroughWalkthroughThe change treats pre-header upstream connection resets as ambiguous. Model requests now receive a marked HTTP 429 refusal instead of automatic replay. Replay-safe image, vision, and web-search sidecars retain reset retries. Recovery and failover paths preserve the refusal. ChangesReset replay boundary
Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy as fetchWithResetRetry
participant Recovery as Responses recovery
participant Upstream
Client->>Proxy: send model request
Proxy->>Upstream: forward request
Upstream--xProxy: reset before response headers
Proxy-->>Recovery: marked 429 refusal
Recovery-->>Client: return upstream_reset_replay_refused
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ea335f11c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Preserve the terminal verdict through adapter and combo error formatting. | ||
| // This also covers a reset reached by a 401/429/413 recovery refetch. | ||
| if (isNonReplayableResponse(upstreamResponse)) { |
There was a problem hiding this comment.
The guard runs only when entering the outer recovery loop, but several inner 401/429 recovery branches assign upstreamResponse = result and remain inside their current loop. For example, if a same-target 429 retry returns the synthesized reset refusal, another configured retry or the following key-pool loop treats its 429 as a provider rate limit and sends the possibly completed turn again. Check isNonReplayableResponse after every rebuildAndRefetch result or include it in each recovery-loop condition.
Useful? React with 👍 / 👎.
| upstreamResponse.status === 429 | ||
| // Not a provider rate limit when this proxy synthesized it for a refused reset | ||
| // replay; rotating accounts on it would re-send an inference that may already | ||
| // have run and would cool down an account that refused nothing. | ||
| && !isNonReplayableResponse(upstreamResponse) |
There was a problem hiding this comment.
For a Codex forward-pool request, this guard prevents immediate account failover, but the same marked 429 subsequently reaches deliverPassthroughResponse, where recordCodexUpstreamOutcome(..., upstreamResponse.status, ...) records it as quota evidence. An ambiguous reset therefore writes a false 429 cooldown and can release affinity or route later requests away from an account that never rate-limited anything. Preserve the marker through delivery and normalize this outcome to the prior transport status, as the compact path already does.
Useful? React with 👍 / 👎.
| turn again, which is the duplicate the refusal exists to prevent. No `Retry-After` is | ||
| attached, and the proxy performs no key rotation, account failover or same-target replay on |
There was a problem hiding this comment.
On a direct native Responses request, the synthesized refusal has a non-empty JSON body, so deliverPassthroughResponse sends it through formatPassthroughUpstreamError; that formatter treats every headerless 429 as a retryable rate limit and adds the default Retry-After: 2. Thus the documented no-retry directive is false and clients that honor the header are explicitly invited to resend the possibly completed turn. Make that formatter recognize upstream_reset_replay_refused before documenting that the header is absent.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| // A 429 this proxy synthesized for a refused reset replay is not a provider rate | ||
| // limit: waiting and re-sending here is exactly the duplicate inference the refusal | ||
| // exists to stop. It kept the same shape under the old 502 only because 502 never | ||
| // matched this branch. | ||
| && !isNonReplayableResponse(response) |
There was a problem hiding this comment.
When native Chat receives the marked reset response, this guard correctly stops internal retries, but the final error path calls classifyError on status 429, producing rate_limit_exceeded; it copies upstreamCode only when the classified code is null. Consequently clients and operators receive a normal provider-rate-limit code rather than upstream_reset_replay_refused, defeating the new distinction on /v1/chat/completions. Special-case the refusal code or route it through the marker-aware formatter.
Useful? React with 👍 / 👎.
…ence and Retry-After (#4807) Release-blocker fix for 2.57.0, found by the final cross-change regression audit. Exact head has a green aggregate ci check with no failing job. The 429 reclassification that landed in #4798 guarded the call sites that read a 429 as a rate limit but not the ones that write quota evidence, synthesize Retry-After, or reclassify the status on the way out, so in two places the release as it stood invited the replay the change exists to prevent: passthrough recorded the synthetic 429 as quota evidence and attached a default Retry-After, and native Chat dropped the distinct code. Adapter recovery could also replay a refusal produced by a refetch inside an arm, which the single-retry regression could not catch. The invariant is now stated once and recorded: a refusal this proxy made never acquires a Retry-After and never becomes quota evidence. Host-owned merge decision; no local suite, typecheck, build, or install was run.
…etry (lidge-jun#4741) (lidge-jun#4798) Maintainer integration for the 2.57.0 stabilization scope. Exact head 2ea335f has a green aggregate ci check with no failing job. This carries lidge-jun#4741 and corrects the half that would have made things worse: the refusal was reported as 502, which the Codex client retries up to four times, so the proxy stopped replaying and handed the amplification to the client. It now answers 429 with upstream_reset_replay_refused, and because a 429 then stops being sufficient evidence of a provider rate limit, all ten call sites that read it that way consult isNonReplayableResponse first and record the transport outcome rather than the client-facing status, so pool health sees exactly what it saw before. The owning structure section is rewritten to separate a refusal this proxy made from an upstream reset reported mid-stream or after a terminal, and the WebSocket post-send verdicts are explicitly unchanged. Host-owned merge decision; no local suite, typecheck, build, or install was run.
…ence and Retry-After (lidge-jun#4807) Release-blocker fix for 2.57.0, found by the final cross-change regression audit. Exact head has a green aggregate ci check with no failing job. The 429 reclassification that landed in lidge-jun#4798 guarded the call sites that read a 429 as a rate limit but not the ones that write quota evidence, synthesize Retry-After, or reclassify the status on the way out, so in two places the release as it stood invited the replay the change exists to prevent: passthrough recorded the synthetic 429 as quota evidence and attached a default Retry-After, and native Chat dropped the distinct code. Adapter recovery could also replay a refusal produced by a refetch inside an arm, which the single-retry regression could not catch. The invariant is now stated once and recorded: a refusal this proxy made never acquires a Retry-After and never becomes quota evidence. Host-owned merge decision; no local suite, typecheck, build, or install was run.
2 participants
Summary
An upstream connection that drops before response headers is not evidence that the model POST was never processed, so replaying it can duplicate a paid, side-effecting turn. This carries #4741, which makes that replay opt-in, and corrects the half that would have made things worse.
The refusal was reported as HTTP 502. The Codex client builds ApiRetryConfig with retry_429 false and retry_5xx true and a default of four attempts, so a 502 there is an instruction to send the whole turn four more times: the proxy stopped replaying and handed the amplification to the client. The refusal now answers 429 with its own code, upstream_reset_replay_refused, and the bridge error formatter restates that status when a combo or adapter formatter re-wraps it holding an upstream-shaped 502.
Changing the status is not free, and that is the part worth reviewing. Once the refusal carries 429, a 429 stops being sufficient evidence of a provider rate limit, and ten call sites read it that way; nine were safe only because 502 never matched them. Left alone, the corrected status would have re-created the same duplicate send inside native Chat and the continuation loop and written cooldowns against credentials that rate-limited nothing. Each now consults isNonReplayableResponse first, in chat-native.ts, adapter-continuation.ts, passthrough-dispatch.ts and compact.ts, and records the transport outcome rather than the client-facing status so pool health sees exactly what it saw before. adapter-dispatch already had the guard from the original PR.
The worst case on the happy path is unchanged behaviour.
Co-authored work from #4741 is carried with authorship preserved; the first two commits are tree-identical to that PR head.
Verification
Checklist
Summary by CodeRabbit
New Features
upstream_reset_replay_refusedwhen a request may already have been processed, preventing duplicate submissions.Bug Fixes
Retry-Afterfor replay-refusal responses.Documentation