Skip to content

fix(streaming): surface mid-stream errors when iterating buffered streams - #2047

Open
pouyashahrdami wants to merge 2 commits into
openai:mainfrom
pouyashahrdami:fix/stream-iterator-swallows-errors
Open

fix(streaming): surface mid-stream errors when iterating buffered streams#2047
pouyashahrdami wants to merge 2 commits into
openai:mainfrom
pouyashahrdami:fix/stream-iterator-swallows-errors

Conversation

@pouyashahrdami

Copy link
Copy Markdown
  • I understand that this repository is auto-generated and my pull request may not be merged

Changes being requested

Fixes #2046.

ChatCompletionStream and ResponseStream can silently swallow a mid-stream error when consumed with for await. Both [Symbol.asyncIterator]() implementations buffer incoming events in an internal pushQueue when the producer runs ahead of the consumer (which happens whenever the loop body does any await). The error/abort handlers only rejected readers that were waiting at that exact moment and never stored the error — so if the error fired while events were buffered with no reader waiting, it rejected nobody and just set done = true. The consumer then drained the buffer and next() returned { done: true } without throwing, so the loop finished as if the response had completed successfully.

This change captures the terminal error and re-throws it once the buffer drains, so for await surfaces it instead of ending silently. The behavior when a reader is already waiting is unchanged (a failureDelivered guard prevents a double-throw). This mirrors the pattern already used by the SDK's own EventStream.events().

A regression test is added for each stream; each fails on main and passes with the fix.

Note on the auto-generated disclaimer: both changed files live under src/lib/, which CONTRIBUTING.md states the generator will never modify (the generator will never modify the contents of the src/lib/ and examples/ directories). So this patch is in handwritten, human-maintained code and persists across regenerations.

Additional context & links

Validation

  • ./node_modules/.bin/vitest run --config vitest.config.mts tests/lib/ChatCompletionStream.test.ts tests/lib/ResponseStream.test.ts — 17/17 pass (incl. the two new tests); both new tests fail on main without the fix.
  • ./node_modules/.bin/prettier --check and ./node_modules/.bin/eslint on all four files — clean.
  • ./node_modules/typescript/bin/tsc --noEmit — clean.

…eams

The `[Symbol.asyncIterator]()` implementations in ChatCompletionStream and
ResponseStream buffer events in `pushQueue` when the producer is ahead of the
consumer. The error/abort handlers only rejected readers waiting at that moment
and never stored the error, so an error that fired while events were buffered
(no reader waiting) was lost: the consumer drained the buffer and `next()`
returned done without throwing.

Capture the terminal error and re-throw it once the buffer drains, mirroring the
existing EventStream.events() pattern. Behavior when a reader is already waiting
is unchanged. Adds a regression test to each stream.

Closes openai#2046

@jbeckwith-oai jbeckwith-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.

Blocking: the same buffered-terminal-error loss remains in two public handwritten streaming paths. ChatCompletionStreamingRunner.toReadableStream() overrides the fixed base conversion with its own pushQueue/readQueue/error handlers, and AssistantStreamSymbol.asyncIterator has the same old pattern. I reproduced both on this exact head: enqueue two items with no reader waiting, emit OpenAIError('boom'), then consume; each returned both items and ended normally with thrown=null. This means tool-runner readable-stream bridges and Assistants iteration can still report a failed response as successful. Please apply the retained-failure semantics (preferably via a shared/EventStream.events-based adapter so these copies cannot drift), including listener/queue cleanup on return/end, to every buffered adapter, and add regressions for these two paths. The two paths changed here otherwise behave correctly: their 17 targeted tests, TypeScript, ESLint, Prettier, build, and diff checks pass. Please also make the new tests deterministic by waiting for the stream's terminal error/done signal instead of a fixed 10 ms sleep, and assert the exact error/message rather than any Error.

…s survive in every stream bridge

Address review: the retained-failure fix previously covered only
ChatCompletionStream and ResponseStream, while
ChatCompletionStreamingRunner.toReadableStream() and
AssistantStream[Symbol.asyncIterator]() kept their own copies of the
buffering logic and still dropped terminal errors once events were
buffered with no reader waiting.

- extract EventStream#_createIterator, a shared buffered async-iterator
  adapter (the events() logic, generalized); events() now delegates to it
- rebuild all four duplicated adapters on the shared helper: chunk/event
  iterators keep their break-aborts semantics via onReturn, the runner
  bridge keeps eager listener registration, AssistantStream keeps
  structuredClone-at-push
- remove listeners and drain queues on end/return (previously leaked);
  mark the terminal promise handled when the consumer explicitly ends
  iteration so the self-inflicted abort no longer surfaces as an
  unhandled rejection
- add regression tests for the runner bridge and AssistantStream paths,
  plus edge-case tests for abort retention, exactly-once failure
  delivery, break-aborts, post-end iteration, and events('error')
- make the buffered-error tests deterministic by awaiting the stream's
  terminal signal and asserting the exact error class and message
@pouyashahrdami

Copy link
Copy Markdown
Author

@jbeckwith-oai Thanks for the thorough review — all points addressed in 5f98601.

Shared adapter: Went with your suggestion: extracted the events() logic into a shared EventStream#_createIterator and rebuilt every buffered adapter on it — both Symbol.asyncIterator implementations, AssistantStream's iterator, and ChatCompletionStreamingRunner.toReadableStream(). events() itself now delegates to it too, so there's exactly one copy of the queue/retained-failure logic left. Per-path semantics are preserved: break-out-of-loop still aborts, the runner bridge still registers listeners eagerly (pre-pull message deltas aren't dropped), and AssistantStream still clones at push time.

Cleanup: Listeners are removed and queues drained on end/return. This surfaced one subtlety worth flagging: the old leaked listeners incidentally suppressed the intentional unhandled-rejection warning when a consumer breaks out of iteration (the self-triggered abort found no listeners after cleanup). return() now marks the terminal promise handled before invoking the abort hook, so existing consumer behavior is unchanged; events() doesn't use that hook, so its "don't suppress after cleanup" contract still holds (covered by the existing test).

Tests: Added regressions for both paths you flagged (both fail on the previous head), plus edge cases: abort retention through a full buffer, exactly-once failure delivery with a pending reader, break-aborts, and post-end iteration returning done instead of hanging. The buffered-error tests now await the stream's terminal done() signal instead of sleeping, and assert the exact error class and message.

Full suite, type check, lint, format, and build are green locally. Ready for another look.

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.

ChatCompletionStream/ResponseStream: mid-stream errors are silently swallowed when iterating with for await

2 participants