streams: do not swallow an ERR_INVALID_THIS error from an async-iterable body - #43758
Conversation
…ble body asyncIterFinishWithError treated every error with the ERR_INVALID_THIS code as a consumer that is gone: it called iterator.return() and resolved the pull. The check dates from when a detached sink threw that code on write(). Since #15234 a detached sink throws a plain Error, and a sink whose destination closed returns 0, so the check matched only errors from the iterator itself, for example a wrong-receiver call in the generator. The body then ended as if it were complete, or its reader never settled. The error now takes the same path as every other error. errorCodeIs() has no caller left and goes with it.
|
Warning Review limit reached
This review includes 4 billable files and costs up to $1.00.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 5 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: reproduced with the script in the Notes of the PR body, on Bun 1.4.3-canary (367d939) and on a debug build of main (4ada08b). When the generator throws an error with PR: #43758 |
|
Updated 11:08 PM PT - Sep 21st, 2026
✅ @robobun, your commit 56f2c5b0ae4d1e9d873ccd5be28c5ebe4d888ee6 passed in 🧪 To try this PR locally: bunx bun-pr 43758That installs a local version of the PR into your bun-43758 --bun |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings on the test, I also checked the PR's claim that no native sink still throws an error with this code into the pump: in src/runtime/webcore/Sink.rs, get_this throws ERR_INVALID_THIS only for CAST_FAILED (wrong receiver), while a detached sink throws a plain Error with no code or returns 0 bytes for CONTROLLER_DETACHED, so the deleted branch in asyncIterFinishWithError has no in-tree producer left. errorCodeIs has no remaining callers outside the removed site.
Extended reasoning...
The change deletes the ERR_INVALID_THIS special case from the async-iterable ReadableStream source in src/jsc/bindings/webcore/streams/BunAsyncIterableSource.cpp, removes the now-unused errorCodeIs helper, and generalizes the existing coded-error test into a test.each with an added upload path; it touches no security-sensitive surface. The two inline findings are about assertion strength in the test, not the C++ fix, and the ruled-out check confirms no native sink path routes this code to the pump.
The generator now throws once its request is at the server, and the test records whether the server read a complete body or an aborted one.
There was a problem hiding this comment.
LGTM — a small, self-contained deletion of a by-code special case in the async-iterable stream source, with the test extended to cover both codes and the upload path.
What was reviewed:
- Confirmed from
src/runtime/webcore/Sink.rs(get_this) that a detached sink now throws a plainErroror returns 0 bytes, andERR_INVALID_THISis only thrown for a wrong receiver — so the removed early return no longer guarded any real sink path. - Checked that
errorCodeIshas no remaining callers insrc/after its removal fromWebStreamsMisc.cpp/WebStreamsInternals.h. - Read the remaining error tail:
iterator.throw()is still followed byRETURN_IF_EXCEPTION, and the cancelled check still resolves instead of rejecting. - The server-side
request body: abortedassertion added in the second commit addresses the earlier gap where the test could not distinguish an accepted truncated upload from a rejected one.
Extended reasoning...
The change removes a special case in asyncIterFinishWithError (src/jsc/bindings/webcore/streams/BunAsyncIterableSource.cpp) that swallowed iterator errors carrying code ERR_INVALID_THIS, deletes the now-unused errorCodeIs helper, and generalizes the existing ERR_INVALID_STATE test into a test.each over both codes plus a fetch upload with a server-side assertion. It touches no auth, crypto, or input-parsing surface; the user-visible effect is that such errors now fail the body like every other error, matching Node and the precedent set by #43714 for ERR_INVALID_STATE. Approval is based on the change being a pure deletion whose justification I verified from Sink.rs, no remaining callers of the deleted helper, no CODEOWNERS entry covering the changed files, and the prior review's substantive point being addressed in a follow-up commit. I did not run the test locally (no debug build was present in this checkout).
Problem
new Response(asyncGen()), afetchupload, aBun.serveresponse) swallows each iterator error withcode: "ERR_INVALID_THIS", for example fromReadableStream.prototype.tee.call({}).text()resolves truncated, a server accepts the upload as complete, and aread()loop never settles.asyncIterFinishWithError(src/jsc/bindings/webcore/streams/BunAsyncIterableSource.cpp:180). streams: add the ERR_INVALID_STATE code to the locked-stream errors of cancel, pipeTo and pipeThrough #43714 removed the same check forERR_INVALID_STATE.Fix
errorCodeIs(), which has no caller left. The error now rejects the pull like every other error.write()when Support async generator functions inResponseandRequestfor bodies #8941 added the check. Since elaborate on error "Expected Sink" #15234 it throws anErrorwith no code, or returns 0.test/js/bun/http/async-iterator-stream.test.ts. TheERR_INVALID_STATEtest of streams: add the ERR_INVALID_STATE code to the locked-stream errors of cancel, pipeTo and pipeThrough #43714 is now atest.eachover both codes, with an upload whose server must read an aborted body. Main fails theERR_INVALID_THIScase. Alsobody-async-iterator,serve-direct-readable-stream,streams,body-stream. Self-reviewed: 3 concerns raised, 3 addressed.Background
BunAsyncIterableSource.cppfeeds aReadableStreamfrom the iterator. Its pump callsiterator.next()and writes each value to the consumer's controller.Downsides
ERR_INVALID_THISerror now fails the body. The repo has no such use.Bun.serveresponse. Before, the client got a complete 200.Notes
Repro (runs under Node.js and Bun):
tee.call({})text()"first;"tee.call({})lockedgetter ofReadableStreamorWritableStreamon{}So the swallow is older than the C++ streams rewrite (#33193) for an error that already carried the code. It is new in 1.4.0 for the two
lockedgetters, which got the code with the rewrite.Wider probes, all on an ASAN debug build with
Malloc=1, no sanitizer report:yield, before it, after the last one, in afinallyat the normal end, a custom iterator whosenext()rejects or throws, an async generator function as the body) x 8 consumers (text(),arrayBuffer(), reader loop,for await,pipeTo(),Bun.write(),fetchupload,Bun.serveresponse). The 1.4.3 canary swallows 336 of 336 cells with the code. This PR: each cell has the outcome of the plainTypeErrorcontrol, none hangs, and there is no unhandled rejection.request.text(),Readable.fromWeb(), a node:http response fed throughpipeline) after a 6 byte and an 8 MiB prefix: the 1.4.3 canary swallows 22 of 22 cells, this PR 0.reader.cancel()with and without a reason, afor awaitthat breaks, a client that aborts aBun.servestream, a raw socket destroyed in the middle of the response, an upload whose server closes early, an upload aborted by its signal. In each case the generator ends quietly, throws a plainErrorfrom itsfinally, or throws an error with the code from itsfinally. The 21 outcomes are the same before and after this change, with no unhandled rejection. These cases do not reach the error tail: thecancel()orclose()hook of the source setsm_cancelledfirst.Producers of the code. Under
src/jsc/bindings/webcore/streamsonly the brand checks of the spec classes throw it (ReadableStream,WritableStream,TransformStream, their readers, writers and controllers, the queuing strategies, the compression and text streams). The pump calls none of them.JSDirectStreamController.cpp,BunStreamSource.cpp,BunStreamConsumers.cppand the generatedJSSink.cppdo not throw it. On the sink side onlyJSSink::get_this(src/runtime/webcore/Sink.rs:473) throws it, forCAST_FAILED.${name}__fromJS(src/codegen/generate-jssink.ts) returns that value only whenthisis neither the sink nor its controller, and the pump callswrite,flushandendwith the controller asthis. The five methods of the JS-facingJSDirectStreamControllerare bound functions and are no-ops once the source ended.The test: the generator of the upload throws once its request is at the server, so the server side is deterministic. With the
src/of main (4ada08b) theERR_INVALID_THIScase fails:text()and the upload resolve, the server readscomplete, 6 bytes, andread()never settles. TheERR_INVALID_STATEcase passes there. With this change both pass, 20 of 20 runs on a Linux ASAN build and 20 of 20 on a Windows x64 debug build. An upload that never settles ends in the timeout of the test runner. The test has no timer of its own.Self-review, three concerns, all addressed. (1) Can a sink or controller still send the code to the pump when the consumer is gone? No, see the list of producers above. (2) The first version of the test repeated the #43714 test. The two are now one
test.each. (3) The upload raced the throw against the connect. The generator now throws once its request is at the server, and the test asserts what the server read. The review bot raised two optional points on the test. The server-side assertion covers the first. For the second (an upload that never settles has no diagnostics) I changed the comment and did not add a timer, because the test rules of this repo do not allow one.Suites run with the debug build:
test/js/bun/http/async-iterator-stream.test.ts(97 pass),test/js/web/fetch/body-async-iterator.test.ts,test/js/bun/http/serve-direct-readable-stream.test.ts(169 pass),test/js/bun/http/serve-error-handler-stream.test.ts,test/js/web/fetch/body-clone.test.ts,test/js/web/streams/streams.test.js(614 pass),test/js/web/fetch/body.test.ts,test/js/web/fetch/body-stream.test.ts(9086 pass),test/js/bun/http/bun-server.test.ts. The two coded-error tests also pass withBUN_JSC_validateExceptionChecks=1.