fix(llm): stop consuming the source iterable when the stream consumer cancels - #726
Open
Matthew-Selvam wants to merge 3 commits into
Open
fix(llm): stop consuming the source iterable when the stream consumer cancels#726Matthew-Selvam wants to merge 3 commits into
Matthew-Selvam wants to merge 3 commits into
Conversation
… cancels streamFromAsyncIterable kept pulling from the SDK iterable after the consumer cancelled — generating (and billing) a full completion nobody reads — until a post-cancel enqueue threw TypeError into the generic catch and surfaced as a spurious stream error. Every aborted Gemini completion took this path (the only consumer of the helper). The pull loop now checks controller.desiredSize === null before each transform/enqueue and exits quietly; a cancel racing the loop (the enqueue/close throw) is recognized by isStreamCancelled and treated as a normal exit. Real pipeline failures still error the stream.
…ch deterministically Two CI failures in the new isStreamCancelled suite: - The message regex used 'cancell' (double-L), which misses the American 'canceled' spelling that runtimes actually emit — and that the test itself used. /cancel/ matches both. - reader.cancel() does NOT null desiredSize in Bun's runtime (it stays 0), so the closed-state branch is now reached via controller.close(), which the spec defines as null. Also reformats the probe construction to satisfy Biome.
…nly null case Per the WHATWG spec, desiredSize is null ONLY for an errored stream — closed and cancelled streams keep a numeric desiredSize (Bun's runtime confirmed: 0 after both close() and reader.cancel()). The in-loop desiredSize === null bail-out was therefore unreachable in production: this code only ever reaches the errored state through controller.error() AFTER the check. Dead code, and a 100%-coverage-gate failure. The cancel signal is the enqueue/close throw itself: a cancelled stream makes the next enqueue throw TypeError, which isStreamCancelled now recognizes. The direct unit test reaches the null branch via controller.error() — the one state where the spec makes it null — and the import line is split for Biome's line width.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cevheri
added
the
loop:needs-moderator-action
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
2 participants
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.
Summary
streamFromAsyncIterable— the helper behind every Gemini streaming completion (GeminiProvider.createStreamFromResult) — kept pulling from the SDK iterable after the consumer cancelled the stream. Two consequences:controller.enqueueon the cancelled stream threwTypeErrorinto the generic catch, surfacing the client-side abort as a spurious stream error.Fix
controller.desiredSize === null(the closed/cancelled/errored signal) before each transform/enqueue and exits quietly instead of draining the rest of the iterable.isStreamCancelledhelper recognizes that shape (nulldesiredSize, or aTypeErrornaming cancellation/closed/invalid-state — message text varies across runtimes) and treats it as a normal exit.controller.error(...)— the existinghandles error in iterabletest pins that.Test plan
stops consuming the iterable when the consumer cancels— 100-item iterable, read one chunk, cancel, assertpulled < 5isStreamCancelledsuite covering all three branches (live controller + real error → false; TypeError naming cancel/closed/invalid state → true; null desiredSize → true)Found during a broader code review of the LLM layer; no issue existed yet for it.