fix(transports): answer an ambiguous resend the same way on every HTTP surface - #5377
Conversation
…P surface The pre-header replay refusal was correct where it was written and absent one wrapper away. The translated Chat surface preserved only the cyber-policy code and model_not_found, took the upstream code only when classifyError had produced none, and then attached the default Retry-After for a retryable 429, so a refusal to resend reached the client as an ordinary rate limit with an instruction to send the turn again. Carry the verdict as a property of the response every surface shares rather than something each one re-derives: retainReplayRefusal and carryReplayRefusal restate the in-process marker at each re-wrap, including the deferred-logging wrapper that silently dropped it. All four formatters now read the marker, or the code a body kept through an intermediate formatter, and never the status -- 429 is exactly what a refusal and a real rate limit have in common. Dropping Retry-After is necessary and not sufficient. The Stainless-generated clients (openai and anthropic, Python and Node) retry 408, 409, 429 and every 5xx from their own table and compute their own backoff when no wait is named, so the status stays 429 (Codex stops there, a 5xx invites four more sends) and every surface also emits x-should-retry: false, which those clients read first.
The acceptance unit for this refusal is not the shape of one response: it is how many times the turn physically reaches upstream when a client with retries enabled is the one deciding. A single fetch cannot see that, because the proxy can answer correctly and the duplicate inference still happens. The case runs the proxy over a socket, counts sends at the upstream boundary and drives all three surfaces with a client that implements the published SDK rule (an explicit x-should-retry first, then 408/409/429/5xx). Its header literals are deliberate: the double stands in for the third party and has to keep believing what those clients believe. A rate-limit control shows the same client resending, so "one send" is a property of the answer rather than of the double.
Both documents stated the refusal carries no Retry-After and stopped there, which is the half of the policy that does not survive contact with a retrying client. Name the fourth writer, the property that now travels with the response, and the suppression header, and point the invariant at the count that holds it.
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. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change prevents replay-refusal responses from triggering SDK retries. It adds shared refusal headers and provenance handling, applies them across native and translated HTTP surfaces, preserves them during response rewrites, and adds parity tests. ChangesReplay refusal handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant Upstream
Client->>Proxy: Send request
Proxy->>Upstream: Forward request
Upstream-->>Proxy: Pre-header ECONNRESET
Proxy-->>Client: Replay refusal with x-should-retry: false
Client->>Client: Do not resend request
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 64.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 7 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
![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: a235bcfbd1
ℹ️ 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".
| const surfaces = [ | ||
| { name: "native Chat", path: "/v1/chat/completions", body: { model: "native/model", ...CHAT_TURN } }, | ||
| { name: "translated Chat", path: "/v1/chat/completions", body: { model: "bridged/model", ...CHAT_TURN } }, | ||
| { name: "Responses", path: "/v1/responses", body: { model: "bridged/model", ...RESPONSES_TURN } }, | ||
| ]; |
There was a problem hiding this comment.
The claimed parity omits /v1/messages. For an Anthropic Messages request whose Responses-backed upstream hits an ambiguous pre-header reset, handleResponses returns the marked 429, but src/server/claude-messages.ts:981-1023 consumes it, ignores its code and marker, synthesizes Retry-After: 2, and rebuilds the response without x-should-retry: false; the Anthropic SDK therefore resends a turn that may already be executing. Preserve the refusal through that wrapper and add Messages to this surface table.
AGENTS.md reference: AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
리뷰 · 우선순위 74 / 80이 PR은 “업스트림이 헤더도 받기 전에 끊겼는데, 그 턴이 이미 돌아갔을 수도 있다”는 상황에서 프록시가 다시 보내지 않기로 한 답( src/server/chat-completions.ts 번역 Chat 경로 - 예전에 거절을 레이트리밋+Retry-After로 바꾸던 구멍은 이번 분기와 헤더로 막혔습니다. 다만 이 헤더를 모르는 raw tests/server/replay-refusal-parity.test.ts - 수락 단위를 “업스트림 전송 횟수”로 잡은 방향은 버그 성격과 잘 맞습니다. 로컬 스위트는 안 돌렸고(작성자 명시), 이 리뷰 시점 hosted CI의 test shard·macos 일부는 아직 pending입니다. 머지 전에 그 초록을 확인하는 게 안전합니다. 메인테이너의 판단이 필요한 지점 거절을 계속 429로 두고 너의 추천 핵심 수리(표시를 래핑마다 이어 주기 + 번역 Chat이 거절 코드를 살림 + 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
upstream_reset_replay_refused: the turn may already have executed, so this proxy refuses to send it again. Two of the three HTTP surfaces said that; the translated Chat wrapper insrc/server/chat-completions.tsdid not. It preserved only the cyber-policy code andmodel_not_found, assigned the upstream code only whenclassifyErrorhad produced none, and then attached the defaultRetry-After: 2. A client on/v1/chat/completionsagainst a Responses-wire provider was therefore told the provider had throttled it, and told when to send the turn again — the exact duplicate the refusal exists to prevent.retainReplayRefusalandcarryReplayRefusal(src/lib/upstream-retry.ts) restate the in-process marker at every re-wrap, including the deferred-logging wrapper insrc/server/relay.tsthat dropped it silently. All four formatters — native Chat, translated Chat, the Responses passthrough formatter and the shared bridge formatter — read the marker, or the code a body kept through an intermediate formatter, and never the status. 429 is precisely what a refusal and a real rate limit have in common, which is why reading the status is what produced this defect.upstream_error.The status decision, and why the status alone is not the fix
The status stays 429. Codex builds its retry policy from
retry_429: false, retry_5xx: trueover four attempts, so a 5xx here multiplies the duplicate send; 429 is where that client stops.That is not sufficient for everyone else. The Stainless-generated clients —
openaiandanthropic, Python and Node — decide from a status table that retries 408, 409, 429 and every 5xx, and they compute their own backoff when no wait is named. RemovingRetry-Aftertherefore does not stop a resend by itself; it only removes the schedule. Those clients do read one explicit signal before that table, so every surface now also emitsx-should-retry: falsealongside the absentRetry-After. Keeping 429 and adding the header covers both client families without inventing a status that describes neither failure.Scope is deliberately narrow: the two WebSocket post-send verdicts (
upstream_no_response,upstream_closed_before_response) are genuine upstream observations and keep their existing 502/504 contract.Verification
tests/server/replay-refusal-parity.test.tsstarts the proxy on a real socket, counts physical upstream sends at the boundary, and drives all three surfaces with a client that implements the published SDK retry rule (an explicitx-should-retryfirst, then the 408/409/429/5xx table). Each logical request must produce exactly one upstream send and one client attempt, with statusREPLAY_REFUSED_STATUS, codeupstream_reset_replay_refused, noRetry-Afterand the suppression header — all derived from the source constants rather than restated as literals.scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json; no file at its size cap grew.tests/lib/upstream-retry.test.ts,tests/server/retry-after-429.test.ts,tests/providers/upstream-transient-retry.test.ts,tests/responses/responses-send-budget-counts.test.ts,tests/responses/responses-account-label.test.ts,tests/codex-integration/reserve-dispatch.test.ts,tests/usage/request-log.test.ts): each remaining assertion holds, since ordinary and cyber-policy formatting is untouched and only a marked or allowlisted refusal enters the new branches.Checklist
structure/transports/responses.mdrecords the property and the client contract;docs-sitestates the wire behaviour users see.Summary by CodeRabbit
Bug Fixes
Documentation
Tests