fix(llm): stop consuming the source iterable when the stream consumer cancels - #726
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
cevheri
left a comment
cevheri
left a comment
There was a problem hiding this comment.
I measured this one before replying, on Bun 1.4.2 and Node 24.14, and I do not think the defect is there.
After a consumer cancels, the loop already stops: the next enqueue throws, that throw ends the for await, and the generator is returned. With a 200-item iterable, pulled reaches 3 on main and 3 with this patch. Through a Response aborted mid-read, which is the production shape, it is the same 3 on both, with zero unhandled rejections. controller.error() on an already-cancelled stream is a spec no-op, so no spurious error reaches anyone either. The new "stops consuming" test passes unchanged against main, so it is not pinning the change.
The patch does change one thing, and it goes the wrong way. isStreamCancelled matches any TypeError whose message contains "cancel", "closed" or "invalid state". A genuine one, say Cannot read properties of undefined (reading 'closed'), now returns early and leaves the controller neither closed nor errored, so the reader never settles. Main rejects with the real error; with this patch that read hangs. Measured on both runtimes.
There is a real drain, though, and it is worth fixing. When transform returns null for every item, which is Gemini's safety-blocked path, nothing is enqueued, nothing throws, and the loop pulls all 200 items after cancel, on main and with this patch alike. The desiredSize === null check your description says the loop makes before each transform is not in the diff. That check, inside the loop, is the fix. I would take that on its own, without the catch guard.
|
@Matthew-Selvam friendly ping, are you still planning to work on this? No rush, just want to know whether to keep it in the review queue. |
|
any update |
1 similar comment
|
any update |
|
closing |
|
forgot about this, sorry,do u want me to work on it? |
|
forgot about this let me know if u still want me to work on it
Best Regards,
Matthew Selvam
…On Thu, Sep 17, 2026 at 1:19 PM Mehmet Cevheri BOZOGLAN < ***@***.***> wrote:
*cevheri* left a comment (libredb/libredb-studio#726)
<#726 (comment)>
closing
—
Reply to this email directly, view it on GitHub
<#726?email_source=notifications&email_token=AVJ6GVME2TGRX5UTYD7UZLD5POJO3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNZRGA4DSNRQGE3KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5710896016>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVJ6GVOQZ4HCCTQLED2RV4D5POJO3AVCNFSNUABGKJSXA33TNF2G64TZHMYTCMRRHE4DSMBZGM5US43TOVSTWNJTHE2TINRYGEZTTILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AVJ6GVJDTD3KVTYCWOJGWWT5POJO3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNZRGA4DSNRQGE3KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/AVJ6GVJOO6KR7IFA4HDMVLD5POJO3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNZRGA4DSNRQGE3KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
it is yours |
cevheri
removed
the
loop:needs-moderator-action
Per review of libredb#726: the enqueue-throw path already stops the loop after cancellation on main, and the isStreamCancelled guard was harmful — its message-sniffing regex could swallow a genuine TypeError (e.g. reading 'closed') and leave the reader hung, while controller.error() on a cancelled stream is a spec no-op anyway. The real defect is the filter path: when transform returns null for every item (Gemini's safety-blocked shape) nothing is ever enqueued, so the loop drains the entire SDK iterable after the consumer cancels — a full completion generated and billed for nobody. Fix it the way the review asked: check for cancellation inside the loop, without the catch guard. - streamFromAsyncIterable flips a flag from the stream's cancel() callback and returns before each transform once cancelled - drop isStreamCancelled entirely; real errors keep propagating through controller.error - make the cancel test deterministic (suspend the generator instead of sleeping 50ms) and pin that genuine 'closed' TypeErrors still reach the reader
Per review of libredb#726: the enqueue-throw path already stops the loop after cancellation on main, and the isStreamCancelled guard was harmful — its message-sniffing regex could swallow a genuine TypeError (e.g. reading 'closed') and leave the reader hung, while controller.error() on a cancelled stream is a spec no-op anyway. The real defect is the filter path: when transform returns null for every item (Gemini's safety-blocked shape) nothing is ever enqueued, so the loop drains the entire SDK iterable after the consumer cancels — a full completion generated and billed for nobody. Fix it the way the review asked: check for cancellation inside the loop, without the catch guard. - streamFromAsyncIterable flips a flag from the stream's cancel() callback and returns before each transform once cancelled - drop isStreamCancelled entirely; real errors keep propagating through controller.error - make the cancel test deterministic (suspend the generator instead of sleeping 50ms) and pin that genuine 'closed' TypeErrors still reach the reader
ce467a3 to
309825f
Compare
|
@cevheri Sorry for the silence — back on it now, and the branch is reworked per your review (309825f).
Local gates: format/lint/typecheck/knip/drift guards/build/build:lib/attw green; streaming suite 30/30. ( Ready for another look whenever you have time. |
|
Heads-up on the red ❌: Cross-platform Tests (windows-latest) failed on two timeouts in tests that don't exist on this branch — they come from
Both pass locally on a pristine |
|
thanks, |
2 participants
Description
Per the review on this PR:
main— the claimed general defect was not there, and theisStreamCancelledguard was strictly harmful: a genuineTypeErrorwhose message happens to mention "closed" gets swallowed, leaving the reader hung forever.transformreturnsnullfor every item (Gemini's safety-blocked shape), nothing is enqueued, nothing throws, and the loop drains the entire SDK iterable after the consumer cancels — a full completion generated and billed for nobody.Type of Change
Related Issue
Found during a code review of the LLM layer; no issue existed yet for it.
Changes Made
streamFromAsyncIterablerecords cancellation via the stream'scancel()callback and checks it at the top of every loop iteration, before transform/enqueue — the fix as requested in review, without the catch guardisStreamCancelledentirely; real errors keep flowing throughcontroller.error()pulled === 2for both the null-transform and enqueueing paths — it fails onmain(pulled = 200); a regression test pins that genuine "closed"TypeErrors still reach the readerTesting
format,lint,typecheck,knip,chart:check,channels:showcase:check,readme:check,security:check,build,build:lib+attw, streaming suite 30/30helm,7zandnode:sqliteare unavailable in this environment, so 181 unrelated tests (helm/packaging/sqlite-driver) fail identically with and without the patch — verified by running the suite on pristineHEADbefore re-applying the change. Required checks run in CI.