Conversation
…lls controller.close() A type: "direct" ReadableStream whose sync pull() writes and calls controller.close() delivered a 200 with an empty body: close() reached HTTPServerWritable via js_close -> end(None), which set requested_end and returned without touching the buffer when readable_len > 0. The no-promise fallback in do_render_stream then mark_done()'d before finalize(), so the buffered bytes were freed unsent and render_missing ended the response empty. Since #32140 a sync pull() that returns without closing keeps the response open waiting for end() (react-dom/server.bun depends on this), so the docs/runtime/streams.mdx example pull(controller) { controller.write("hello"); controller.write("world"); } now pins the connection until idleTimeout and the client sees ECONNRESET. Adding the close() the standard controller API expects then hit the bug above and still delivered zero bytes. end(None) now delegates to end_from_js: the buffer is flushed via send_readable (parking a pending_flush on backpressure, which do_render_stream already waits for) and the response terminates, matching controller.end(). The err=Some branch keeps the abort-path semantics (no partial flush). The docs example is updated to call controller.close() and notes that returning without closing leaves the destination open for captured-controller writes. The no-promise fallback in RequestContext.rs is not deleted: it remains the residual catch-all for a sync pull() whose close()/end() neither responded nor parked a flush, and for a direct stream with no pull.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Verified locally: 699fceb: routing at the controller's CI 73832: 284/286 jobs passed. The two red lanes are |
|
Updated 5:09 AM PT - Jul 16th, 2026
❌ @robobun, your commit 699fceb has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34334That installs a local version of the PR into your bun-34334 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…-flush abort path The previous commit made HTTPServerWritable::end(None) delegate to end_from_js, but js_close reaches end(None) for BOTH a user's controller.close() (normal completion) and readStreamIntoSink's abrupt path (rsisSinkClose(op, error) -> sink.close(error) -> the 0-arg controller close slot). That turned a mid-stream error on a default ReadableStream body into a cleanly-terminated 200, breaking serve-stream-body-error.test.ts and bake's react-response.test.ts. Route at the controller's close() instead: no argument -> endWithSink (flush + terminate, matching end()); an error argument -> the existing __close (abort, no flush). HTTPServerWritable::end stays the abort-only path. rsisSinkClose passes the error, a direct stream's controller.close() passes nothing, so both callers get the behaviour they need.
controller.error() (or throw undefined, or a rejection with undefined) reaches rsisSinkClose with reason undefined, which it still forwards as exactly one argument. The isUndefinedOrNull() check treated that as a normal close and cleanly terminated the response. Gate on argumentCount() == 0 instead: rsisSinkClose and the NativeSink cancel in ReadableStreamOperations always append one argument, a direct stream's controller.close() passes none. Adds a mid-stream-nullish-error variant to the serve-stream-body-error fixture asserting no clean chunked terminator.
There was a problem hiding this comment.
The two earlier concerns are addressed — 699fceb gates on callFrame->argumentCount() == 0, and I verified both internal callers (rsisSinkClose at BunStreamSource.cpp:968-971 and the NativeSink cancel at ReadableStreamOperations.cpp:420-423) unconditionally append exactly one argument, so the abort path is preserved even for nullish reasons. Deferring for a human look because the routing change fans out to all six generated sink controllers (ArrayBufferSink/FileSink/NetworkSink included) and shifts controller.close() from end(None) to end_from_js for each — worth a maintainer confirming that's the intended contract across the non-HTTP sinks.
Checked: the argumentCount() == 0 gate against every C++ call site of the controller's close; ${controller}__close still discards endWithSink's return value so close()'s JS return stays undefined; the new mid-stream-nullish-error fixture pins the RFC 9112 §7 force-close for controller.error()-with-no-arg.
Extended reasoning...
Overview
The PR fixes a regression where a direct ReadableStream whose synchronous pull() calls controller.write(...) then controller.close() delivers an empty body over Bun.serve. The fix lives in src/codegen/generate-jssink.ts: the generated ${controller}__close host fn now branches on callFrame->argumentCount(), routing zero-arg close() to ${name}__endWithSink (flush buffered bytes + clean terminator) and any-arg close(reason) to the existing ${name}__close abort path. src/runtime/webcore/streams.rs only gains a doc comment; docs/runtime/streams.mdx is updated to teach controller.close(); three test files add coverage for both the flush fix and the nullish-error abort case.
Review history on this PR
I flagged two issues on earlier revisions, both fixed:
- The initial
end(None)→end_from_jsdelegation applied to the readStreamIntoSink error path too, defeating RFC 9112 §7 force-close. Fixed in 3cbdbea by moving the routing to the generated controllerclose(). - The
isUndefinedOrNull()gate in 3cbdbea couldn't distinguishclose()fromclose(undefined), so a stream erroring with a nullish reason still hit the flush path. Fixed in 699fceb by gating onargumentCount() == 0, with a newmid-stream-nullish-errorregression test.
For 699fceb I verified the invariant the gate depends on: rsisSinkClose (BunStreamSource.cpp:968-971) and the ControllerKind::NativeSink cancel arm (ReadableStreamOperations.cpp:420-423) both do args.append(...) unconditionally before invoking .close, so argumentCount() is always ≥1 on every internal abort/cancel route regardless of the reason value. A grep for other .close invocations on the sink controller found none. The JSDirectStreamController.cpp:392 .close(error) call targets the sink prototype (${name}__doClose), not the controller, so it's unaffected.
Security risks
The RFC 9112 §7 behavior (force-close without a chunked terminator on mid-stream failure) is correctness-adjacent to security: a client that receives a cleanly-terminated truncated body can't detect the truncation. Both prior review rounds were about preserving exactly this. The final gate does, and the new test asserts it for the nullish-reason edge.
Level of scrutiny
High. This is generated code that fans out to all six sink types (ArrayBufferSink, FileSink, HTTP{,S,3}ResponseSink, NetworkSink) on the hot HTTP-response-streaming path, and it took three revisions to get the abort-path preservation right. Not a mechanical change.
Other factors / why defer
- The new tests cover the HTTP/HTTPS sinks and the readStreamIntoSink abort path well. They do not exercise
controller.close()onFileSink/NetworkSink/ArrayBufferSinkcontrollers, which now also route throughend_from_jsinstead ofend(None). ForNetworkSinkthat meansend_from_jsmay now armend_promisewhereend(None)didn't; the return value is discarded so JS-visible behavior is likely unchanged, but a maintainer should confirm the contract. - User code that spells
controller.close(undefined)still hits the no-flush path (argumentCount 1). That's not a regression — it was already broken — and matches neither the docs example nor the standard controller signature, so it seems acceptable to leave. - Two rounds of substantive fixes on a ~12-line generated-code change is enough churn that human sign-off is warranted before merge.
|
On the fan-out to non-HTTP sinks: the generated controller is only ever constructed via
So the only live non-HTTP controller is FileSink, and its |
|
This PR now conflicts with main (the surrounding The same bug came up again, and there is an alternative fix on the branch Only one of the two should land; leaving the choice to a maintainer rather than opening a second PR. |
1 participant
Repro
Since #32140, a
type: "direct"ReadableStreamwhose synchronouspull()returns without closing keeps the response open untilcontroller.end()(react-dom/server.bun'srenderToReadableStreamdepends on this to stream Suspense boundaries afterpull()returns). That made the example atdocs/runtime/streams.mdx:62-68, which writes and returns without callingclose(), pin the connection untilidleTimeoutand surfaceECONNRESETto the client. Adding thecontroller.close()the standardReadableStreamcontroller API expects then hit the bug above and still delivered zero bytes for writes below the auto-flush threshold, so there was no working shape for the documented synchronous case.Cause
controller.close()on the sink controller dispatches via the generated${controller}__close, which always called${name}__close->JSSink::js_close->HTTPServerWritable::end(None). With buffered bytes, that path setrequested_end = trueand returned without flushing; thereadable_len > 0branch was a no-op. Back inreadDirectStreamthe stream is alreadyClosed, so it returnsjsUndefined(), anddo_render_stream's no-promise fallback runsmark_done()immediately beforefinalize(), sofinalize'sflush_no_wait()branch is skipped,destroy_sinkfrees the buffer with the bytes still in it, andrender_missing()ends the response empty.controller.end()took the sibling path (${name}__endWithSink->end_from_js), which flushes viasend_readable(0)and parks apending_flushon backpressure, so it always delivered the full body.Fix
The generated
${controller}__closenow routes on its argument:close()with no argument is normal completion and calls${name}__endWithSink(flush buffered bytes, terminate, park apending_flushon backpressure fordo_render_streamto wait on), matchingcontroller.end().close(error)is thereadStreamIntoSinkabrupt path (rsisSinkClose(op, error)invokes.close(error)when a defaultReadableStreambody errors) and keeps the existing${name}__closecall so the sink is torn down without a clean terminator andhandle_reject_streamforce-closes the connection per RFC 9112 section 7.The docs example is updated to call
controller.close()and notes that returning without closing leaves the destination open for captured-controller writes (the React SSR pattern). Reverting to the 1.3.x auto-close on synchronous return would re-break #32137, so the #32140 contract is kept.The no-promise fallback in
RequestContext.rsis left in place: it is still the residual catch-all whenreadDirectStreamreturnsjsUndefined()and is not dead after #32140.Tests
New cases in
test/js/bun/http/serve-direct-readable-stream.test.ts:sync pull() that writes N bytes and calls close()for N in{1, 11, 2048, 200000}over HTTP and HTTPS. The 1- and 11-byte cases deliver""on the unfixed build; 2048 and 200000 happened to work becausewrite()auto-flushed pasthighWaterMark, and stay as sibling coverage.sync pull() that calls close() with nothing writtenends the response.The existing React-pattern tests (
sync pull() that ends later ...,cancel() fires ..., the h3 backpressure cases, and the AsyncLocalStorage leak test) all still pass.serve-stream-body-error.test.tscovers theclose(error)abort path (mid-stream error on a defaultReadableStreambody must not send a clean chunked terminator), andbake/dev/react-response.test.tsanddirect-readable-stream.test.tsxexercise react-dom'sserver.bunbuild end to end.[review] gate passed · iteration 0 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file