fix(streaming): surface server error frames and settle streams past throwing listeners - #2502
fix(streaming): surface server error frames and settle streams past throwing listeners#2502morgan-coded wants to merge 3 commits into
Conversation
…hrowing listeners
There was a problem hiding this comment.
💡 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".
| } | ||
| this.#rejectConnectedPromise(error); | ||
| this.#rejectEndPromise(error); | ||
| this._emit('end'); |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Requesting changes because a throwing terminal listener can silently convert a failed or aborted stream into successful async-iterator completion.
| } | ||
| this.#rejectConnectedPromise(error); | ||
| this.#rejectEndPromise(error); | ||
| this._emit('end'); |
There was a problem hiding this comment.
[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.
…fore emitting end
There was a problem hiding this comment.
💡 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".
| if ('error' in item && typeof item.error === 'object' && item.error !== null) { | ||
| throw new APIError(undefined, item.error, undefined, undefined); |
There was a problem hiding this comment.
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 👍 / 👎.
|
Recorded the terminal error as Added three regression tests for your scenarios — a pending reader, a buffered drain-then-reject, and the abort variant — each failing on One behavior change reaches past the throwing-listener case: an iterator or |
ChatCompletionStream.fromReadableStream()turned a server error frame intoTypeError: chunk.choices is not iterable, whileAssistantStreamreturned a bareErrorand a throwingerrororabortlistener could leavedone()pending forever. This converts object-valued error frames in those helper paths toAPIErrorwith their server payload intact and settles terminal state before rethrowing the first listener failure; later listeners remain skipped, secondaryendlistener failures are suppressed, and the caller's catch runs afterendlisteners.ResponseStreamalready carries this contract, and #2047 is the existing cross-helper streaming precedent. I checked the red cases against the base, then ranpnpm 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-valuederrorkey now fails withAPIError; string error bodies remain unchanged, the check follows the function's existing property-detection style, and I can move the conversion into genericStream.fromReadableStreamif you prefer that broader central contract.