Repository navigation
fix(logs): attribute proxy replay refusals as transport-ambiguous - #6887
Conversation
A synthetic 429 upstream_reset_replay_refused was attributed like a provider throttle (rate-limit/permitted) because attribution treated every 429 as a rate limit. Read the in-process replay-refusal marker in the deferred logger and carry a synthetic transport-ambiguous cause hint into final request and attempt attribution. Client-visible refusal behavior is unchanged. Closes #6658.
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. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 44 minutes. View limit details
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: e396cf5102
ℹ️ 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".
| A proxy replay refusal is one such fact: the deferred logger in `src/server/relay.ts` reads the | ||
| in-process `isReplayRefusalResponse` marker and passes a synthetic `transport-ambiguous` cause | ||
| hint, so its 429 keeps the `upstream_reset_replay_refused` code but is never attributed as a | ||
| provider rate limit; refusal-like provider text alone cannot establish it. | ||
| Enforced by `tests/lib/failure-attribution.test.ts`. |
There was a problem hiding this comment.
The expanded invariant claims that responseWithDeferredRequestLog recognizes the in-process replay-refusal marker, but its Enforced by target remains tests/lib/failure-attribution.test.ts, which only tests the generic attribution functions and never exercises that logger or marker. The new end-to-end assertions are instead in tests/responses/responses-translated-reset.test.ts without the invariant ID, so the documented binding and structure gate would remain green if the actual marker wiring regressed or those assertions disappeared. Split/rebind the invariant so the exercising test carries the ID, while retaining an honest binding for the generic attribution contract.
AGENTS.md reference: structure/AGENTS.md:L100-L108
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
Summary
The request log no longer records a proxy replay refusal as a provider rate limit. When an ambiguous pre-header reset cannot be replayed, OpenCodex answers with a synthetic HTTP 429
upstream_reset_replay_refused(by design, so clients do not resend a turn the upstream may already be running). The final request and attempt attribution treated every 429 asfailureCause: rate-limitwithresendPermission: permitted. That is the contradictory pair in #6658, where one Anthropic send producedupstream_reset_replay_refusedalongsiderate-limit/permitted, and it sent triage looking for an Anthropic quota problem that the trace did not show.responseWithDeferredRequestLognow reads the in-processisReplayRefusalResponsemarker before logging and carriesterminalSource: "synthetic"plus a closedtransport-ambiguouscause hint intoattributeFinalRequest. The logical row and final attempt both showtransport-ambiguous/refused-ambiguous.requestLogErrorCodeprefers that provenance and keeps the message match only as a fallback.The client-visible refusal (status, body, no
Retry-After) is unchanged, a genuine provider 429 still attributes asrate-limit/permitted, and provider text that merely resembles the refusal wording cannot set the hint. Routing and health decisions read statuses and transport outcomes, not the persisted cause, so only the log projection changes.The "exceeded retry limit" wording in #6658 comes from the Codex client's handling of non-quota 429 bodies, as noted in the issue triage; this PR does not change what the client receives.
Closes #6658
Verification
bun test tests/responses/responses-translated-reset.test.ts tests/usage/request-log.test.ts tests/usage/request-log-nonstream.test.ts tests/lib/failure-attribution.test.ts: 243 pass, 0 fail. Eight new cases inresponses-translated-reset.test.tscover the logical row and final attempt, empty and bodyless marked refusals, unmarked refusal-like provider text, a genuine 429, and cancellation precedence.deferred replay refusal attributioncases give 4 pass, 4 fail (marked refusals attributedrate-limit).bun run typecheck,bun run structure:check,bun scripts/file-size-ratchet.ts: pass.src/server/request-log.tsis 1999 lines (threshold 2000), so two existing comments were shortened to fit.Checklist