fix: preserve chat runner abort reasons - #2607
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. |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Reason propagation is now consistent across both cancellation boundaries: external signals transfer their original reason into the runner controller, and tool checkpoints wrap that same reason in APIUserAbortError.cause. Covering already-aborted signals is important because that path bypasses listener delivery entirely.
…#2591) ## Summary Cancelling a tool runner during its final `afterCompletion` callback could be reported as success: `done()` resolved and successful final events were emitted even though the runner controller was aborted. This PR observes callback cancellation at the existing post-callback boundary and again at successful final settlement: - Continue awaiting the configured callback at all three exit sites: a final response, a forced function call, and the completion limit. - Retain the early cancellation check before another request, and use one final-settlement check to cover later fulfillment reactions on a callback's returned promise. - Preserve the runner signal's reason by identity on `APIUserAbortError.cause`. - Preserve completed tool results and the original error when the callback itself rejects. The final-settlement check is gated by a private flag set only when `afterCompletion` is actually invoked. Omitted/null callbacks, configured-but-never-invoked callbacks, and unrelated `ChatCompletionStream` behavior remain unchanged. ## Scope Only handwritten `src/lib/AbstractChatCompletionRunner.ts`, its existing cancellation test, and `docs/helpers.md` change. No generated files, dependencies, exported types, or general `EventStream` machinery change. [openai#2607](openai#2607) owns the broader external-signal reason forwarding and five preexisting tool checkpoints. Those changes are not duplicated here. This PR preserves the reason already present on the runner's signal at the new callback boundaries and composes with that separate forwarding fix. ## Verification - Fail-before regressions cover callback/external cancellation, cause identity, and the late-promise-reaction race across JSON/SSE and every exit route. The final settlement addition made eight previously failing late-reaction cases pass while preserving 34 controls. - **118 related tests pass** on Node 22.22.2 and 24.19.0, including callback-error-precedence and no-callback controls. - **474 final built CJS/ESM checks pass** across exact Node 22.0.0, 24.19.0, and 26.7.0: a 324-case runner matrix plus 150 independent compatibility checks. These verify cause identity/descriptors, arbitrary/falsy reasons, late reactions, one request, retained history, lifecycle counts, omitted/null callbacks, never-invoked callbacks, and bare streams. - `tsc --noEmit`, canonical `./scripts/build`, pinned Oxfmt 0.62.0, available cached Oxlint 1.76.0, and `git diff --check` pass. Emitted CJS/ESM runner declarations remain byte-identical. - Full handwritten unit suite on the final revision: **7,450 passed, 1 failed**. The sole `oxlint-config` package-command dependency-verification failure reproduces identically on the clean `dde19c5c` base. - Repeated independent adversarial review covered security, compatibility, callback/error precedence, final-settlement ordering, and regression gaps. No blocking implementation findings remain within this callback scope. ## Review follow-ups - [`718da1c6`](openai@718da1c) adds cause preservation at the new checkpoint. - [`4ae4ac34`](openai@4ae4ac3) closes the late-promise-reaction gap with the callback-scoped settlement guard. ## Local tooling limitation Fresh frozen installation remains blocked by missing publication-time metadata for `qs@6.16.0` in the configured registry. Local checks use available cached Vitest 4.1.10 and Oxlint 1.76.0 rather than pinned 4.1.11 and 1.79.0; TypeScript 6.0.3 and Oxfmt 0.62.0 match their pins. Fresh CI validates the new head with the pinned toolchain. No lockfile, registry configuration, or supply-chain safeguards were changed.
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Requesting changes for a cancellation classification regression in sequential tool callbacks. Forwarding the caller's reason also requires normalizing callback rejections that match that reason into APIUserAbortError, preserving cause.
| } | ||
|
|
||
| const listener = () => this.controller.abort(); | ||
| const listener = () => this.controller.abort(signal.reason); |
There was a problem hiding this comment.
[P2] Preserve cancellation classification for tool callbacks
With parallel_tool_calls: false or a named tool_choice, a tool callback that calls runner.controller.signal.throwIfAborted() after the caller cancels with abort(new Error('stop')) now throws that custom error. The callback rejection propagates through await runToolCall(...), bypassing the new abort checkpoint, and EventStream.#handleError classifies it as an ordinary error because its name is not AbortError. Consequently, runner.done() rejects with OpenAIError, the runner emits error instead of abort, and runner.aborted remains false. Before this change, the runner controller's default AbortError was correctly converted into APIUserAbortError. Please normalize rejections matching the aborted runner signal's reason into APIUserAbortError, preserving cause.
be65087 to
69e2710
Compare
|
Addressed the requested cancellation-classification case. Sequential tool callbacks now normalize a rejection that is exactly the aborted runner signal reason into APIUserAbortError while preserving that reason as cause. Added a focused parallel_tool_calls:false regression where the callback observes the caller abort via throwIfAborted(); it now emits the abort path and the focused test passes. |
![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: 69e2710cce
ℹ️ 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".
| } | ||
|
|
||
| const listener = () => this.controller.abort(); | ||
| const listener = () => this.controller.abort(signal.reason); |
There was a problem hiding this comment.
When the caller aborts after a streaming response has begun, this listener stores the reason on the runner controller, but ChatCompletionStream._createChatCompletion() subsequently ends iteration and throws a fresh cause-less APIUserAbortError at line 1757; ResponseStream and AssistantStream have the same pattern. Consequently, runTools({ stream: true }).done() still loses the caller's reason during in-body cancellation, so construct those terminal errors from the controller reason and cover the actual streaming boundary.
AGENTS.md reference: AGENTS.md:L119-L123
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| const listener = () => this.controller.abort(); | ||
| const listener = () => this.controller.abort(signal.reason); |
There was a problem hiding this comment.
For a structural or polyfilled AbortSignal whose reason getter throws, this abort-event callback fails before this.controller.abort() runs. With native EventTarget dispatch that can surface as an uncaught listener exception while the SDK request and runner.done() remain pending, rather than settling cancellation; guard the reason read and ensure the internal controller or terminal promise is settled with the resulting failure.
AGENTS.md reference: AGENTS.md:L98-L105
Useful? React with 👍 / 👎.
| try { | ||
| rawContent = await fn.function(parsed, runner, toolContext); | ||
| } catch (error) { | ||
| if (this.controller.signal.aborted && error === this.controller.signal.reason) { |
There was a problem hiding this comment.
If a caller uses controller.abort(Number.NaN) and the tool callback observes cancellation through activeRunner.controller.signal.throwIfAborted(), the callback throws that exact reason but error === this.controller.signal.reason is false for NaN. The runner therefore converts the cancellation into an ordinary OpenAIError('NaN'), reports aborted as false, and loses the cause; use Object.is for this abort-reason comparison in both callback branches.
AGENTS.md reference: AGENTS.md:L119-L123
Useful? React with 👍 / 👎.
|
Addressed the new cancellation findings. Abort propagation now tolerates structural signals whose |
2 participants
Changes being requested
Preserve caller-provided abort reasons across chat completion convenience runners.
The direct request layer already attaches
signal.reasontoAPIUserAbortError.cause, but chat completion runners could lose or misclassify that reason at internal cancellation boundaries. This change:signal.reasonwhenEventStreambridges an external signal into its runner-owned controller;APIUserAbortError, preserving that reason ascause;Fixes #2606.
Review follow-up
The current head addresses the sequential-callback classification issue raised in review. Both parsed and unparsed tool callback paths now catch a rejection equal to
runner.controller.signal.reasonafter cancellation and translate it through the sameuserAbortError()helper. The regression exercisesparallel_tool_calls: false, cancels with a customError, has the active callback callthrowIfAborted(), and verifiesrunner.done()rejects withAPIUserAbortError, preserves the custom cause, emits the abort path, and leavesrunner.aborted === true.Fresh code/security review is running on commit
69e2710cce37947e2f51476d8b57ab3f05c836fb.Validation
Previous-head validation:
pnpm test tests/lib/ChatCompletionRunFunctions.test.ts tests/lib/runner-abort-listeners.test.ts— 48 passedpnpm exec tsc --noEmit— passedpnpm build— passedgit diff --check— passedPushed-head CI/review is required for the new regression.
Additional context & links
The change does not alter cancellation timing or add a new API; it preserves and consistently classifies the same abort-cause information already exposed by direct requests.