fix: honor Retry-After minima and request cancellation — SDK-235 - #977
fix: honor Retry-After minima and request cancellation — SDK-235#977markstuart-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. 55 mixed files remain; 0 existing customizations changed. Compared 55 existing customizations unchanged
15 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 33944619859 --repo openai/openai-java \
--name castiron-custom-code-33944619859-1 --dir /tmp/castiron-custom-code-33944619859-1
git apply --stat /tmp/castiron-custom-code-33944619859-1/custom-code.patch
cat /tmp/castiron-custom-code-33944619859-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 198a7bdf339d3180c11db70a7fd6ee0c96c79376 689c242319d90fea47f895210ad280f615b4f8bf
python3 scripts/castiron/custom_code_report.py report \
--base 198a7bdf339d3180c11db70a7fd6ee0c96c79376 \
--head 689c242319d90fea47f895210ad280f615b4f8bf --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-689c242319d9
cat /tmp/castiron-custom-code-689c242319d9/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: 9a651b6ac6
ℹ️ 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.
Reviewed exact head 9a651b6 and confirmed three blocking cancellation/retry issues already called out inline: (1) CancellableFuture does not override SDK-used stage methods such as thenAccept and exceptionally, so cancellation is disconnected on public void operations and built-in managed-identity provider chains; (2) WorkloadIdentityHttpClient closes a 401 response before capturing headers, so close-sensitive transports can lose Retry-After or replace the intended retryable error; and (3) canceling DefaultSleeper TimerTask instances does not purge them from Timer’s queue, retaining long-delay tasks/futures until their original deadlines. Please address those exact in-scope findings and add concrete path regressions. Current completed CI is green; build/Jackson compatibility remains in progress.
|
Addressed all three findings:
Public regressions reproduce each defect before the fix, including void-response requests, actual provider chains, post-close header access and a cancelled long-delay wait. The final focused, full native test/build, formatting and lint checks pass. Pushed commit 8cd6c16 to this PR branch and verified it on GitHub. No package was published to a registry, and no release or merge was performed. Final validation: 110 focused tests; 6,794 native tests (22 existing skips); 5,935 Jackson compatibility tests; native build, format and lint; trusted-current-main budget 1,906/2,000. Two consecutive independent adversarial review rounds are clean. All three inline findings above are answered and resolved; please re-review the updated implementation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cd6c16b04
ℹ️ 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".
markstuart-oai
changed the title
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The current head addresses the earlier cancellation ownership gaps in a coherent way: CancellableFuture now wraps SDK-used stages such as thenAccept/exceptionally, the X.509 path captures safe retry metadata before response cleanup, and DefaultSleeper uses a ScheduledThreadPoolExecutor with remove-on-cancel rather than retaining cancelled long-delay tasks. Exact decimal retry parsing/upward rounding also avoids shortening server minima. The added public-path cancellation and long-sleep retention regressions are the important part here. I don’t see a remaining blocker in these fixes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: decf8fad88
ℹ️ 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: 7ac1417593
ℹ️ 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: cb5d417df7
ℹ️ 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 23 changed files at cb5d417 against 198a7bd, including the public void/model paths, GCP/Azure metadata chains, shared refresh ownership, X.509 adapter, decimal retry parsing, timer rounding, and all review discussions.
The three earlier requested fixes are present: SDK-used thenAccept/exceptionally stages propagate cancellation, 401 retry headers are captured before close, and canceled waits use scheduler removal. The subsequent retry-metadata and background-generation fixes are also reflected in the code and regression tests. The X.509 cache-owned issuer refresh policy is unchanged from the base.
Two existing findings remain actionable on this head:
- Source cancellation exceptions: CancellableFuture.cancel invokes the source callback without containment after establishing cancellation, so a throwing custom transport cancel escapes to the public caller.
- Active handle-stage cleanup: LoggingHttpClient can be inside response status/header access after its parent completes; canceling that completed parent does not close the active response to unblock the handler.
I found no additional distinct issue to add inline. These outstanding cancellation cases prevent approval. Static review only; I did not execute tests or independently validate the reported test results.
There was a problem hiding this comment.
💡 Codex Review
Complete the response lease when cancellation cleanup throws
When cancellation wins just as an X.509 response arrives and that response's close() throws, this callback exits before completing responseLeaseFuture. The response worker remains blocked in get() indefinitely; repeated occurrences can consume every thread in the fixed response executor and make later token exchanges unavailable. Ensure lease publication occurs even if cancellation cleanup fails.
AGENTS.md reference: AGENTS.md:L55-L56
ℹ️ 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: 4283d481aa
ℹ️ 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: 2eccc96cc3
ℹ️ 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: 61c44746c9
ℹ️ 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".
4 participants
Valid server retry minima can be lost through decimal rounding or credential refresh, and cancelling an asynchronous request can leave its transport or retry wait running.
Preserve retry minima with exact decimal parsing and upward timer rounding, retain response headers before closing authentication failures, and return the original error when the delay cannot be represented. Carry cancellation through the concrete stages used by public requests and metadata providers, isolate callers sharing a credential refresh, and remove cancelled waits promptly. The X.509 adapter forwards cancellation to active API I/O, handles cancellation during transport enrollment, and closes responses discarded during those races. Issuer body-read failures retain safe timing metadata while preserving the original I/O cause and sanitized diagnostics.
Validation on the published bytes: 110 focused tests; full native tests (6,794 tests, 22 existing skips); 5,935 Jackson compatibility tests; native build, formatting and lint. Public regressions reproduce the defects before the fixes. Two consecutive independent adversarial review rounds are clean. Trusted-current-main custom-code budget passes at 1,906/2,000.