Repository navigation
fix(responses): enforce configured total sends during recovery - #4947
Conversation
|
Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
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. |
|
Merging on the macOS exception now recorded in #4956, with the Windows evidence this change needed. At its exact head all nine Windows shards are green in the dispatched Holding this fix for that defect would delay a correctness fix for a problem it does not share and cannot influence. The release candidate remains held on #4956; this merge is not a promotion and makes no claim about macOS at this SHA. |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50ecf06273
ℹ️ 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".
| if (opaqueBlobRecovery.kind === "recovered") { | ||
| upstreamResponse = opaqueBlobRecovery.response; | ||
| continue passthroughRecovery; | ||
| if (!configuredTransientSendBudgetExhausted()) { |
There was a problem hiding this comment.
Preserve the upstream response when recovery has no allowance
When transientRetryOn5xx.attempts is 4–10 and the initial transient ladder consumes all three base sends, this predicate remains true because the configured total still has headroom. A recovery-eligible final response (for example, a 502 encrypted-function-output rejection) is therefore consumed and cancelled by attemptOpaqueBlobRecovery, but rebuildAndRefetch then receives zero attempts because line 911 disables the final reserve for every configured policy; the client gets a synthetic request_send_budget_exhausted 429 instead of the original upstream response. Gate the recovery on its effective allowance before mutating the response, or permit the reserve while the configured total still has headroom.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
Summary
Restore the configured total-send invariant found by the 2.59.0 merged-tree audit. A provider configured with attempts=1 authorizes one physical upstream send total. A later rebuild previously passed zero remaining attempts to the shared recovery helper, which could still grant a final-reserve send.
Configured exhaustion now stops rebuild work before response consumption or request mutation and returns the original upstream response. The recovery helper preserves its default reserve behavior for unconfigured providers. No retry count, timeout, delay or shared request cap is increased.
Verification
Checklist