fix: cancel prompt and session-abort requests on stream close; wire doGenerate abort signal#31
Merged
Merged
Conversation
…oGenerate abort signal Follow-up to #30, which fixed the leaked GET /event SSE connection that kept the Node event loop alive after streaming finished. This extends the same per-stream AbortController to the remaining unanchored requests: - session.prompt and session.abort in doStream now receive the stream's abort signal, so a prompt the server never completes (e.g. after an abort race) cannot pin the event loop, and dispose() tears these requests down along with the SSE subscription. Prompt results that arrive after an intentional close are ignored instead of being mis-reported as empty-response errors (the SDK resolves with { error } rather than rejecting on fetch abort). - doGenerate now forwards options.abortSignal to the prompt request, so aborting generateText cancels the underlying HTTP request instead of leaving it pending. A caller-initiated abort surfaces as AbortError (previously a generic empty-response error) and best-effort aborts the server-side session to stop generation. Adds the regression tests #30 lacked: SSE signal cancellation on stream close (including abort-before-iterator.return ordering), prompt-fetch cancellation on the abort and completion paths, doGenerate mid-flight abort semantics, and manager dispose() aborting registered controllers. Also adds the missing CHANGELOG entries for #30 and this change.
![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: b468520ca7
ℹ️ 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".
…ate abort Codex review on #31 flagged that clients configured with the supported clientOptions.throwOnError passthrough reject the session.prompt await on fetch abort instead of resolving a fields-style { error } result, so the best-effort session.abort in the empty-data path never ran and the server kept generating after generateText was cancelled. Hoist the server-side abort into a helper and issue it from both the rejection path and the { error } path before rethrowing.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Merged
ben-vargas
added a commit
that referenced
this pull request
Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #30, which fixed the leaked
GET /eventSSE connection that kept the Node event loop alive after streaming finished. This PR extends the same per-streamAbortControllerto the remaining unanchored requests and adds the regression coverage #30 shipped without.Changes
doStream: prompt and session-abort requests cancelled on stream closeThe per-stream abort signal from #30 is now also passed to
session.promptandsession.abort, so a prompt the server never completes (e.g. after an abort race) cannot pin the event loop, anddispose()tears these requests down along with the SSE subscription. The local controller is renamedeventAbortController→requestAbortControllerto reflect its broader scope; the manager API from #30 (registerEventSubscription) is unchanged.Prompt results that arrive after an intentional close are now ignored. Without that guard, cancelling the prompt fetch would be mis-reported as an empty-response error stream part, because the SDK resolves with
{ error }rather than rejecting on fetch abort.doGenerate: abort signal actually wiredOn main,
options.abortSignalnever reaches the prompt request — abortinggenerateTextleaves the HTTP request pending and the server generating. The signal is now forwarded to the fetch; a caller-initiated abort surfaces asAbortError(previously a generic empty-response error) and best-effort aborts the server-side session to stop generation.Regression tests (5 new, none existed for #30's behavior)
iterator.return()ordering fix: abort SSE event subscription on stream close so process can exit #30 relies on (return() can block on a pending read otherwise)doGeneratemid-flight abort throwsAbortErrorand callssession.abortdispose()aborts registered controllers; unregistered ones are untouchedCHANGELOG
Adds the missing Unreleased entries for both #30 and this change.
Verification
npm run cigreen: 376/376 tests, typecheck, lint; build clean127.0.0.1:4096: repeated runs ofexamples/abort-signal.tsall exit within ~4s of printing "Done." with no leftover ESTABLISHED connectionsContext
Live debugging of the original hang (Node inspector attached to a hung process) showed the leaked request was the SSE
GET /event— fixed by #30 — while the prompt POST completes oncesession.abortis processed. The prompt cancellation here is therefore defense-in-depth rather than a reproduced hang; thedoGeneratesignal wiring is an independent real gap. This also retires the "fix flaky process hang after stream abort" follow-up: its hypothesized prompt-fetch leak has no reproduction independent of the SSE leak, and the hygiene it asked for is implemented and tested here.