Skip to content

fix(streaming): surface server error frames and settle streams past throwing listeners - #2502

Open
morgan-coded wants to merge 3 commits into
openai:mainfrom
morgan-coded:fix/stream-terminal-failure-integrity
Open

fix(streaming): surface server error frames and settle streams past throwing listeners#2502
morgan-coded wants to merge 3 commits into
openai:mainfrom
morgan-coded:fix/stream-terminal-failure-integrity

Conversation

@morgan-coded

Copy link
Copy Markdown
Contributor

ChatCompletionStream.fromReadableStream() turned a server error frame into TypeError: chunk.choices is not iterable, while AssistantStream returned a bare Error and a throwing error or abort listener could leave done() pending forever. This converts object-valued error frames in those helper paths to APIError with their server payload intact and settles terminal state before rethrowing the first listener failure; later listeners remain skipped, secondary end listener failures are suppressed, and the caller's catch runs after end listeners. ResponseStream already carries this contract, and #2047 is the existing cross-helper streaming precedent. I checked the red cases against the base, then ran pnpm exec tsc, lint, the three focused stream files with 391 tests passing, and the full suite with 6,999 handwritten and 556 generated tests passing. One otherwise-valid chunk with an extra top-level object-valued error key now fails with APIError; string error bodies remain unchanged, the check follows the function's existing property-detection style, and I can move the conversion into generic Stream.fromReadableStream if you prefer that broader central contract.

@morgan-coded
morgan-coded requested a review from a team as a code owner August 27, 2026 13:13

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d9b575ce32

ℹ️ 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".

Comment thread src/lib/EventStream.ts
}
this.#rejectConnectedPromise(error);
this.#rejectEndPromise(error);
this._emit('end');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve terminal failures for pending event iterators

When a user registers a throwing error listener before creating an events() iterator, that listener throws before the iterator's internal onFailure listener runs. This new end emission then invokes onEnd while its failure is still unset, so a pending next() resolves with {done: true} and consumers silently treat a failed stream as clean EOF. Ensure the terminal error reaches iterator state before emitting end, or continue dispatching later listeners while retaining the first listener exception.

AGENTS.md reference: AGENTS.md:L119-L123

Useful? React with 👍 / 👎.

@HAYDEN-OAI HAYDEN-OAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes because a throwing terminal listener can silently convert a failed or aborted stream into successful async-iterator completion.

Comment thread src/lib/EventStream.ts
}
this.#rejectConnectedPromise(error);
this.#rejectEndPromise(error);
this._emit('end');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve terminal failures before emitting end. When an error listener registered before an async iterator throws, listener dispatch stops before the iterator's internal onFailure listener runs. This newly guaranteed end emission then resolves a pending next() with { done: true }; buffered iterators likewise drain and complete without surfacing the original error. The same issue exists for abort at line 1911 and affects Chat Completions, Assistants, and Responses. Please record the terminal error in iterator-visible state before emitting end, or guarantee internal failure listeners run even when user listeners throw.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b7b8188d3f

ℹ️ 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".

Comment thread src/lib/ChatCompletionStream.ts Outdated
Comment on lines +1775 to +1776
if ('error' in item && typeof item.error === 'object' && item.error !== null) {
throw new APIError(undefined, item.error, undefined, undefined);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Require an own error property before rejecting chunks

When Object.prototype or another inherited prototype defines an object-valued error property, every valid decoded completion chunk satisfies this in check and the stream rejects with an APIError built from the inherited value. Since fromReadableStream() parses NDJSON into ordinary objects, prototype pollution elsewhere in the application is sufficient to break unrelated completion streams; use the already imported hasOwn helper so only an error field actually present in the frame is treated as a server error.

AGENTS.md reference: AGENTS.md:L98-L103

Useful? React with 👍 / 👎.

@morgan-coded

Copy link
Copy Markdown
Contributor Author

Recorded the terminal error as #terminalFailure in #settleTerminalEvent before _emit('end'), on both the error branch and the abort branch you named. _createIterator adopts it in onEnd and in a next() made after the stream ended, gated by each iterator's rejectOnError/rejectOnAbort so events('error')-style iterators never reject the events they yield, and return() marks the failure delivered so a consumer-returned iterator stays silently done.

Added three regression tests for your scenarios — a pending reader, a buffered drain-then-reject, and the abort variant — each failing on d9b575ce and passing on the revision. Checked the same probe, a throwing terminal listener registered before the iterator with one event delivered before the failure, against Chat Completions, Assistants, and Responses: main hangs, d9b575ce completes without surfacing the error, and the revision rejects with that first event still delivered.

One behavior change reaches past the throwing-listener case: an iterator or toReadableStream() created after the stream already failed now rejects once instead of completing as a silently truncated zero-chunk transfer, while streams that end without a failure, gated error/abort event iterators, and already-returned iterators are unchanged. Left two pre-existing behaviors alone: concurrently parked next() calls still deliver the failure only to the first, as on d9b575ce, and a throwing end listener still starves onEnd, though a fresh next() now surfaces the error where the prior head hung. b7b8188d sits on top of the reviewed head, so the re-review diff is only this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants