Repository navigation
streams: deliver the source error to native sinks on controller.close(error) - #40669
Conversation
…(error) When a ReadableStream pumped into a native sink fails, readStreamIntoSink calls `controller.close(error)` on the sink controller before it rejects the pump promise. The generated `__close` never read that argument and `JSSink::js_close` always called `end(None)`, so every native sink saw a clean end: - S3 (`NetworkSink::end`) committed the buffered bytes as a complete single-file PUT while the caller's promise rejected. - HTMLRewriter (`RewriterPipe::end_from_stream(None)`) ended the rewrite with the partial output; for an input that was already errored when `transform()` ran, the Rejected arm found the pipe done and `.text()` resolved to "". The controller's `close(reason)` now passes the reason through to a new `JsSinkType::close_with_error(this, global, reason)`. Its default is the old clean end, which is right for the sinks whose owner handles the pump rejection (ArrayBufferSink, FetchRequestBodySink, the HTTP response sinks, FileSink). NetworkSink fails the upload and stashes the error for the caller's promise; RewriterPipe fails the rewrite with it. The sink's own `close()` method keeps passing no reason.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughGenerated sink bindings now propagate JavaScript close reasons. Runtime sinks route those reasons through failure handlers. S3 and HTML rewriter tests cover explicit, undefined, synchronous, and asynchronous stream errors. ChangesStream close reason propagation
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains at the current head; the supplied evidence does not identify a concrete product, runtime, data, security, or availability issue. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, affected sinks, behavior changes, and verification results. It does not use the template headings exactly, but it provides the required information and remains focused on the pull request. Comment |
|
Updated 3:00 PM PT - Aug 27th, 2026
⏳ @robobun, your commit 89a2beb is still building in
|
StatusReady for review at 3af8c5a. Reproduced with a loopback S3 stub: a CI on build 107008: every lane that runs the tests this PR adds or changes is green on all platforms. The red lanes are Not changed here: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/api/html_rewriter.rs— Stale doc comment onjs_pump_reaction_pendingstill claims "the generated${controller}__closedrops its error argument" — this PR makes that false (the argument is now forwarded), and the PR updated the parallel wording at ~L1481 and in html-rewriter.test.js but missed this oneExtended reasoning...
A future reader of
js_pump_reaction_pendingis told the close argument is dropped and that the deferral exists because the error is unavailable here — the opposite of what the code now does — and may remove or reorder the guard on that basis; base branch's comment was accurate, so this PR introduces the inaccuracy. Fix: reword to match the updated comment at end_from_stream (the pump calls close(...) before the promise settles; the reaction remains the terminal authority).Verification: nit — /home/claude/bun/src/runtime/api/html_rewriter.rs:732-734 still reads: "The generated
${controller}__closedrops its error argument, soend_from_streamdefers terminal work to the reaction (which carries the real error) while this is set." This PR makes that statement false: generate-jssink.ts now emits `${name}__close(lexicalGlobalObject, ptr,… | nit — The field doc comment at…
controller.error() with no argument errors the stream with undefined, and the pump then calls close(undefined). Keying the sink's decision on the reason's nullishness read that as a clean close. The generated controller close now passes the empty value when close() was called with no argument, so any argument, nullish or not, fails the sink. NetworkSink only stashes a non-nullish reason for the caller's promise; the generic S3 error covers the rest. Also rewords the js_pump_reaction_pending field comment, which still said the close argument was dropped.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The nullish-reason gap from my earlier comment is closed by 8d99f00 — js_close now branches on reason.is_empty() (argument presence via argumentCount() > 0, not the value's truthiness), and both s3.test.ts and html-rewriter.test.js gained a controller.error()-with-no-reason case that fails on stock bun.
Extended reasoning...
This run followed up on the commit pushed after the prior review. The earlier concern — that js_close gated close_with_error on !reason.is_empty_or_undefined_or_null(), so a source that errored with undefined/null still hit the clean-end path and committed truncated output — is now addressed: the generated ${controller}__close encodes "no argument" as the empty JSValue via callFrame->argumentCount() > 0, js_close checks only reason.is_empty(), and the new S3 (error() without a reason) and HTMLRewriter (undefined reason) test rows exercise exactly that path. The remaining inline finding this run is separate from that and speaks for itself.
| // close(reason): the pump's abrupt path passes the source's error, which | ||
| // can itself be undefined (controller.error() with no argument); a clean | ||
| // close passes no argument. "No argument" is encoded as the empty value so | ||
| // the sink can tell the two apart. | ||
| ${name}__close(lexicalGlobalObject, ptr, | ||
| callFrame->argumentCount() > 0 ? JSC::JSValue::encode(callFrame->argument(0)) : JSC::JSValue::encode(JSC::JSValue())); |
There was a problem hiding this comment.
🔴 ${controller}__close is the JS-visible close method on the sink-controller prototype (line 1228), and a type: 'direct' ReadableStream hands that controller straight to the user's pull(controller) (readDirectStream, BunStreamSource.cpp:857-859). Keying the error path on callFrame->argumentCount() > 0 therefore lets user code that calls controller.close(anything) — including controller.close(undefined) — trip close_with_error on NetworkSink and RewriterPipe, aborting an upload / rejecting a rewrite that on base would have committed cleanly. …
Extended reasoning...
…The pump is not the only caller of this method; the abrupt-close signal should be carried on a channel user code can't reach (e.g. a distinct internal entry point invoked by rsisSinkClose, or a private slot on the controller set by rsisAbrupt and read here), not on the public method's argument count.
const rs = new ReadableStream({
type: 'direct',
async pull(c) { c.write(new Uint8Array(3)); c.close(undefined); },
});
await new Bun.S3Client({endpoint:`http://127.0.0.1:${port}`,bucket:'b',accessKeyId:'x',secretAccessKey:'x'}).write('k', rs);On base: c.close(undefined) → end(None) → the 3-byte PUT is sent and write() resolves. After this PR: argumentCount()>0, so NetworkSink__close receives a non-empty reason, js_close routes to close_with_error → fail_from_js_pump → task.fail() aborts the upload and write() rejects with S3Error: ReadableStream ended with an error even though the source closed cleanly. The same shape (new HTMLRewriter().transform(new Response(rs)).text()) now rejects with undefined where base resolved with the…
Verification: normal — ${controller}__close is the public close method on the sink-controller prototype (generate-jssink.ts:1228: close ${controller}__close ReadOnly|DontDelete|Function 0), and readDirectStream hands that controller directly to the user's pull(controller) (BunStreamSource.cpp:857-859: pullArgs.append(sinkController); … call(globalObject, pull, …)). assignToStream… | normal —…
) ### Problem - A `type: "direct"` ReadableStream whose `pull()` ends with `controller.close(undefined)` (or `null`, `false`, `""`) is treated as a failed source. `S3Client.write` sends no PutObject and rejects with `ReadableStream ended with an error`. `HTMLRewriter.transform(...).text()` rejects with `undefined`. 1.4.0 commits the PUT and resolves the text. `close()` is unaffected. - Cause: #40669 made the generated `${controller}__close` (`src/codegen/generate-jssink.ts`) forward `argument(0)` whenever `argumentCount() > 0`, so `JSSink::js_close` (`src/runtime/webcore/Sink.rs`) routed `undefined` to `close_with_error`. The pump (`rsisAbrupt`, `BunStreamSource.cpp`) reports a failed source through the same JS method, and its error can itself be `undefined`. ### Fix - The JS-facing `close(error?)` treats a falsy argument as a clean close, the truthiness rule `readDirectStreamCloseImpl` already uses for the same value. The host function body moves to a shared `${controller}__closeWithReason`, where an empty `reason` is a clean close. - The pump gets its own entry, `WebCore::closeSinkControllerWithError`, which dispatches on the controller class and always delivers the failure, `undefined` included. `rsisSinkClose` uses it for native sink controllers. - Correct because the two callers mean different things by the same value: user code passes an optional error, the pump passes "the source failed, with this reason". The Rust contract in `js_close` is unchanged: empty means clean, anything else means failed. - Verified: `test/js/bun/s3/s3.test.ts` and `test/js/workerd/html-rewriter.test.js` (six new rows each, four per file fail on main). Also the serve direct stream, streams, spawn stdin, fetch body and backpressure suites. ### Background - A native sink (`JsSinkType` in `Sink.rs`) is a Rust writer behind a JS controller object. For a `type: "direct"` stream, `pull(controller)` receives that sink controller, so the user's `controller.close()` is the generated host function. - `readStreamIntoSink` is the pump for non-direct streams. When the source errors, `rsisAbrupt` closes the sink with the error, then rejects the pump promise. - `JsSinkType::close_with_error` is the #40669 hook: `NetworkSink` (S3) aborts the upload, `RewriterPipe` fails the rewrite, every other sink keeps the clean end. <details><summary>Notes</summary> Repro on main (debug build, same results with `null`, `false`, `""`): ```js const mk = () => new ReadableStream({ type: "direct", pull(c) { c.write("<b>hello</b>"); c.close(undefined); } }); await new HTMLRewriter().transform(new Response(mk())).text(); // rejects with undefined await s3.write("k", new Response(mk())); // rejects, no PUT on the wire ``` `close(new Error("boom"))` on a direct stream keeps the #40669 behavior: no PUT, the promise rejects with `boom`, the HTMLRewriter body rejects with the error. Both test files pin that row too. The `error() without a reason` rows from #40669 still pass: that path is the pump, which now calls `closeSinkControllerWithError(controller, undefined)` and reaches `close_with_error` as before. `readableStreamCancel` on a stream bound to a native sink also calls the JS `close(reason)`. `cancel()` with no reason is a clean close again (as in 1.4.0), `cancel(error)` fails the sink. Suites run: `serve-direct-readable-stream.test.ts`, `serve-stream-body-error.test.ts`, `streams.test.js`, `spawn-stdin-readable-stream*.test.ts`, `body.test.ts`, `body-stream.test.ts`, `fetch.stream.test.ts`, `fetch-backpressure.test.ts`. In the last one, the four `h3 receive backpressure` tests time out in this container when the whole file runs and pass in isolation. They fail the same way on an unmodified build of main here. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/workerd/html-rewriter.test.js <!-- robobun:evidence:end -->
Problem
ReadableStreampumped into a native sink fails, and the sink treats it as a clean end.S3Client.write(key, stream)commits the buffered bytes as a complete single-file PUT (the object exists, truncated) while the returned promise rejects with the source error.new HTMLRewriter().transform(new Response(stream)).text()resolves to""when the stream is already errored attransform()time.readStreamIntoSinkcallscontroller.close(error)on the sink controller before it rejects the pump promise (BunStreamSource.cpp,rsisAbrupt→rsisSinkClose). The generated${controller}__close(src/codegen/generate-jssink.ts) never read the argument, andJSSink::js_close(src/runtime/webcore/Sink.rs) always calledend(None). Every native sink saw EOF.Fix
close(reason)now reaches the sink: the generated controller method passescallFrame->argument(0)to${name}__close, andjs_closeroutes a non-empty reason to a newJsSinkType::close_with_error(this, global, reason). Its default is the old clean end, soArrayBufferSink,FetchRequestBodySink, the HTTP response sinks andFileSinkbehave as before (their owners already handle the pump rejection).NetworkSink(S3) fails the upload instead of committing:task.failbefore any PUT is scheduled, with the source error stashed so the caller's promise rejects with it.RewriterPipefails the rewrite with the error, so the output body rejects.test/js/bun/s3/s3.test.ts(two new rows, stock bun commits a 2048 byte PUT) andtest/js/workerd/html-rewriter.test.js(one new test, stock bun resolves""). Alsofilesink.test.ts, thespawn-stdin-*files,streams.test.js,serve-stream-body-error.test.ts,serve-direct-readable-stream.test.ts,body.test.ts,body-stream.test.ts,fetch.stream.test.ts,fetch-backpressure.test.ts,html-rewriter-doctype.test.ts.Background
JsSinkTypeinSink.rs) is a Rust writer behind a JS controller object.JSSink::assign_to_streamstartsreadStreamIntoSink, the JS pump: it reads the stream and calls the controller'swrite,end, orclose(error). The pump also returns a promise the sink's owner reacts to.close_with_errortakes*mut Self, likefinalize:NetworkSink::failfires the S3 task's completion callback synchronously, and that callback borrows the sink again.close()method (${name}__doClose) still passes no reason.Notes
Wire before the fix, S3 stub on loopback, stream enqueues two 1 KB chunks then
controller.error(new Error("boom")): the stub receivesPUT /my_bucket/objwithContent-Length: 2048and a complete body;client.writerejects withboom. After: no PUT, the same rejection. A stream that closes cleanly still produces the PUT.HTMLRewriter before the fix: a stream that errors after the first chunk already rejected (the pump's reject reaction is the terminal step on the Pending arm). Only the already-errored input resolved to
"":readManythrows synchronously, soclose(error)and the rejection both happen insidetransform(), the pipe had already runend_rewritefrom the clean close, and the Rejected arm'send_from_stream(Some(err))found it done.FileSink (spawn stdin,
Bun.write(path, stream)) keeps the default. Its pump reactions already handle the failure (Bun.writerejects; the spawn path aborts the stream). Closing without flushing would drop the bytes the source produced before it failed depending on whether the auto-flusher had run yet, andspawn-stdin-readable-stream-edge-cases.test.ts("ReadableStream with exception in pull") pins that those bytes reach the child. A pipe has no incomplete-message signal beyond EOF, so there is nothing else for the sink to do there; what a JS observer for a failed spawn stdin stream should look like is a separate question.Related: #40596 fixes the same family on the
Bun.serveresponse side.no test proof · iteration 7 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/workerd/html-rewriter.test.js