fix(client): make retry backoff abortable via AbortSignal - #2653
Conversation
Pass request AbortSignals through sleep during Retry-After backoff so abort settles promptly instead of waiting out the full delay. Add unit tests for mid-backoff abort. Fixes openai#2580.
![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: 224f8feab5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } else { | ||
| await sleep(timeoutMillis); | ||
| const retrySignals = | ||
| requestSignal === options.signal ? [requestSignal] : [requestSignal, options.signal]; |
There was a problem hiding this comment.
When a protected/provider hook replaces req.signal with a signal that is synchronously aborted in response to the original options.signal, both signals are aborted before this catch resumes. Because retrySignals puts the replacement first, find() selects it and _makeUserAbortError exposes its reason rather than the caller's reason, unlike the caller-first precedence used for active requests at lines 1255-1257 and 1278-1279. Put options.signal first (while still listening to both) so abort-cause preservation remains consistent during backoff.
AGENTS.md reference: AGENTS.md:L119-L123
Useful? React with 👍 / 👎.
| settle(() => reject(error)); | ||
| } | ||
| } | ||
| if (activeSignals.some((signal) => signal.aborted)) { |
There was a problem hiding this comment.
If a structural or polyfilled signal's aborted getter succeeds before listener registration but throws during this final race-closing probe, the Promise constructor converts that throw directly into a rejection without calling settle(). The retry timer and all installed abort listeners consequently remain live until the backoff expires (up to 60 seconds in this client), retaining request state after the operation has already failed. Catch failures from this probe and reject through settle() so the timer and listeners are cleaned up.
AGENTS.md reference: AGENTS.md:L119-L123
Useful? React with 👍 / 👎.
| break; | ||
| } | ||
| try { | ||
| signal.addEventListener('abort', abort, { once: true }); |
There was a problem hiding this comment.
If a structural or polyfilled signal invokes abort synchronously from addEventListener() and then finishes installing that listener, settle() runs its cleanup before the listener has actually been registered. The loop then observes settled and exits without removing the newly installed listener; since the signal has already aborted, { once: true } may never clean it up, permanently retaining the retry closure. Check settled immediately after registration and explicitly remove the just-added listener, as the repository's other abortable wait implementations do.
AGENTS.md reference: AGENTS.md:L119-L123
Useful? React with 👍 / 👎.
Prefer the caller AbortSignal reason when both signals abort during backoff, and harden sleep cleanup for sync registration and probe failures so timers/listeners do not leak.
|
Addressed the 3 P2 Codex review notes in 42067d7:
|
1 participant
Summary
sleepaccept optionalAbortSignals and clear its timer on abort.retryRequestbackoff so abort duringRetry-After/ default backoff settles promptly instead of waiting the full delay.causeviaAPIUserAbortError.Fixes #2580.
Test plan
vitest run --config vitest.config.mts tests/internal/utils.test.ts tests/client-behavior.test.ts(74 passed)