fix(client): stop retries when server delay exceeds maximum — SDK-235 - #609
fix(client): stop retries when server delay exceeds maximum — SDK-235#609markstuart-oai wants to merge 15 commits into
Conversation
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. |
Castiron custom code✅ No new custom-code files detected. 47 mixed files remain; 1 existing customization changed. Compared
46 existing customizations unchanged
6 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 33847219377 --repo openai/openai-ruby \
--name castiron-custom-code-33847219377-1 --dir /tmp/castiron-custom-code-33847219377-1
git apply --stat /tmp/castiron-custom-code-33847219377-1/custom-code.patch
cat /tmp/castiron-custom-code-33847219377-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 1560da01073061cc0a0dadc853f7dbc6e688a4d4 3b65e696ff7cc0dbadc8c54eb0bb67d9696bf3eb
python3 scripts/castiron/custom_code_report.py report \
--base 1560da01073061cc0a0dadc853f7dbc6e688a4d4 \
--head 3b65e696ff7cc0dbadc8c54eb0bb67d9696bf3eb --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-3b65e696ff7c
cat /tmp/castiron-custom-code-3b65e696ff7c/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
![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: c978ac9982
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61b0327a79
ℹ️ 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".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Blocking: lib/openai/internal/transport/base_client.rb:518 still treats positive numeric overflow as an invalid hint. Ruby parses both Retry-After: 1e999 and a standards-valid 400-digit delay-seconds value as Infinity; the finite? filter drops them and falls back to short jittered backoff, so the client retries earlier than the server minimum. Preserve positive numeric overflow as an excessive delay (while keeping literal infinity and negative overflow invalid), and cover both Retry-After and Retry-After-Ms plus the authentication replay path. Reviewed exact head 3b65e69.
|
Fixed positive numeric overflow for both The same pass closes two related authentication paths: finite 401 minima are honored before replay, and issuer minima survive cached-token fallback through subsequent API retries. Timing is captured before response cleanup and remains local to the logical request. The public regressions fail before the fixes and pass afterward, including concurrent callers whose shared issuer response body fails. Published in |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The current head closes the important overflow case rather than merely clamping it: positive numeric overflow remains an excessive server minimum, while literal/negative invalid values still fall back through the normal policy. Capturing issuer retry timing before response cleanup is also necessary for workload-identity replay, otherwise authentication can silently shorten the same minimum. The deadline and cached-token precedence look appropriately preserved.
markstuart-oai
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6449ab3db7
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22b67c6ee2
ℹ️ 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".
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Reviewed all 18 changed files at b45cbc1 against 06e2c91, including the prior review discussions. Positive numeric overflow now remains an excessive minimum for both retry headers; invalid literal and negative values retain fallback behavior. Authentication replay, issuer minima across cached/shared refresh, deadline precedence, retry notifications, and RBI/RBS signatures are consistent with the added regressions. No new actionable findings. Static review only; Ruby is unavailable in this environment, so I did not run tests.
4 participants
Valid server retry delays above
max_retry_delaywere shortened, allowing requests before the server’s minimum. Authentication replay could also bypass finite API or issuer hints.Preserve the server minimum across normal retries, credential refresh, and cached-token fallback. Return the original error when that minimum exceeds the cap, including numeric overflow. Shared-refresh participants retain their strongest observed issuer minimum without delaying independent callers. A short request deadline takes precedence over refusing an excessive 401 replay, and delayed authentication replays emit the standard retry callback and log event.
Validation on the final bytes: full suite 1,659 tests / 16,025 assertions (one skip); Bedrock 44 tests / 390 assertions (one opt-in live-test skip); focused authentication regressions, formatting, RuboCop, Sorbet, and RBS passed. Two consecutive independent adversarial-review rounds were clean. Signed commit
22b67c6ee236a4f21921941a0b6f2032b6dbad3dpasses the trusted-current-main custom-code budget at 3,434 / 4,000 lines.The shipped RBI/RBS signatures include the optional retry_state keyword. Retained stronger refusals take effect before another notification or wait; delayed 401 callbacks report the reachable attempt limit. Early test setup failures preserve their original errors during cleanup. The focused suite passes 81 tests / 1,647 assertions, and all 1,279 RBS checks pass.