Bun.serve: send the buffered tail when a direct stream controller is close()d by robobun · Pull Request #37696 · oven-sh/bun

robobun · 2026-08-12T01:06:40Z

Problem

A Bun.serve response from a direct ReadableStream whose pull() writes and then calls controller.close() synchronously loses whatever is still buffered: the client gets a well formed 200 with Content-Length: 0, or after a mid-stream flush() a chunked body missing its tail. controller.end() in the same spot sends everything. Bun 1.4.0 and main.
Over HTTP/3 the same happens from an async pull() too (c.write("hey"); c.close() returns an empty body).
Cause: close() with buffered bytes only marked the sink as ending and left the send to a deferred auto-flush task, but a synchronous close has its sink torn down as soon as pull() returns, before that task runs.
The deferred path also parked nothing for the request to wait on when the transport could not take the tail (common on QUIC right after HEADERS), so the sink was torn down mid-send even from an async pull().

Fix

close() with buffered bytes now sends the tail at once, the way end() already did: hand it to uWS, and if uWS cannot drain it, park the pending-flush promise the request already waits on.
Correct because close() and end() now finish through one path, and that path already survives a synchronous pull() and backpressure.
The auto-flusher's finish-the-response branch becomes unreachable, so it is deleted and replaced with a debug assertion. Empty-buffer close() is unchanged; close(error) for a body that errors mid-stream also takes the new path and is observably the same on HTTP/1 as before.
Verification: new tests for sync and async close() and end(), with and without a mid-stream flush(), run over both plain HTTP and TLS (the TLS sink is a separate instantiation of the same code), plus two HTTP/3 backpressure cases. Eight fail on the unfixed build (three sync close() cases on each of HTTP and HTTPS, both HTTP/3 cases); all pass with the fix, and the existing serve and stream suites still pass.
The same matrix also pins down the two close() shapes that have nothing buffered (no write at all, and a single write at the high-water mark that went straight to the socket), which this change leaves alone; they pass before and after. These and the TLS runs are the cases Bun.serve: flush buffered bytes when a direct stream's sync pull() calls controller.close() #34334 covered that were missing here.
Supersedes Bun.serve: flush buffered bytes when a direct stream's sync pull() calls controller.close() #34334, which fixed the same bug in the generated controller code and has conflicted with main.

Background

Direct stream (new ReadableStream({ type: "direct", pull })): a Bun extension where pull() writes bytes straight into a sink via controller.write() / flush() / close() / end() instead of enqueueing chunks. As a Response body, that sink is the HTTP response itself.
Sink buffering: writes below the high-water mark sit in memory. A stream that finishes with nothing flushed goes out whole with Content-Length; once a flush() has gone out, the response is chunked and the rest follows as more chunks.
Auto-flusher: a deferred task the sink registers to push its buffer to the socket later. It only helps if the sink is still alive when the event loop gets to it.
Rendering a stream body: if pull() returns nothing to wait on and no flush promise is parked, the request treats the body as done and finalizes the sink immediately; a parked promise makes it wait.
uWS try_end: the HTTP layer's finish call reports whether it managed to write the tail. If not (backpressure), the sink must retry from its writable callback, so something has to keep the sink alive until then.

Original description

Repro

const mk = (fin, flushMid) =>
  new ReadableStream({
    type: "direct",
    pull(c) { c.write("hello"); if (flushMid) c.flush(); c.write("world"); c[fin](); },
  });
const srv = Bun.serve({
  port: 0,
  fetch(req) {
    const u = new URL(req.url);
    return new Response(mk(u.searchParams.get("fin"), u.searchParams.has("flush")));
  },
});
for (const q of ["fin=close", "fin=end", "fin=close&flush", "fin=end&flush"]) {
  const r = await fetch(`${srv.url}?${q}`);
  console.log(q, r.headers.get("content-length"), r.headers.get("transfer-encoding"), JSON.stringify(await r.text()));
}

Bun 1.4.0 and current main:

fin=close        0     null     ""            <- whole body lost
fin=end          10    null     "helloworld"
fin=close&flush  null  chunked  "hello"       <- "world" lost
fin=end&flush    null  chunked  "helloworld"

Every byte still buffered in the sink when a synchronous pull() calls controller.close() is dropped, and the response is a well formed 200 with Content-Length: 0 (or a chunked body missing its tail). controller.end() in the same position sends everything. This is what you get by combining the sync pull() from the direct stream docs with controller.close().

Over HTTP/3 the same thing happens from an async pull() too (c.write("hey"); c.close() returns an empty body).

Cause

controller.close() reaches HTTPServerWritable::end() in src/runtime/webcore/streams.rs. With bytes buffered it only set requested_end / end_len and returned, leaving the send to the auto-flusher (a deferred task), and on_auto_flush() had a requested_end branch to finish the response afterwards.

That never runs for a synchronous close: readDirectStream returns undefined because the stream is already closed, and do_render_stream takes its no-promise path, which calls mark_done() (unregistering the flusher) before finalize(), destroys the sink with the bytes still in its buffer, and ends the response through render_missing().

The deferred design also had no answer for transport backpressure: when the flusher's try_end could not drain (common on QUIC right after HEADERS), nothing was parked for handle_resolve_stream to wait on, so the sink was torn down and the body truncated even from an async pull().

end_from_js() (the end() controller method) has neither problem: it sends the tail immediately and parks a pending_flush promise on backpressure, which do_render_stream and handle_resolve_stream already wait for.

Fix

HTTPServerWritable::end() now sends the tail the same way end_from_js() does: send_readable(0) (uWS try_end / end), then the same post-end bookkeeping, or park_pending_flush() when uWS could not drain it, so on_writable resends the tail and settles the promise the request is waiting on. The shared pieces are factored into park_pending_flush() (also used by flush_from_js(), which had the same block) and handle_ended_response().

With that, the requested_end branch in on_auto_flush() is unreachable (end() / end_from_js() unregister the flusher on every path and writes after requested_end are rejected), so it is deleted and replaced by a debug assertion. uWS markDone() already drops onWritable on both HTTP/1 and HTTP/3, which is what end_from_js() was relying on, so the clear_on_writable() from that branch is not needed.

The empty-buffer close() path is unchanged. close(error) from readStreamIntoSink (a default ReadableStream body that errors while bytes are still buffered in the sink) also goes through end(); on HTTP/1 those bytes were already being delivered by the auto-flusher before this change, so the observable behaviour there is the same, just no longer dependent on a deferred task running first. The existing mid-stream error tests in serve-stream-body-error.test.ts still pass.

Supersedes #34334, which fixed the same bug by routing close() to endWithSink in the generated controller code based on the argument count; it has conflicted with main since #36006 / #36943.

Verification

New cases in test/js/bun/http/serve-direct-readable-stream.test.ts:

buffered bytes are sent when the controller finishes: sync close(), sync end(), both with and without a mid-stream flush(), async close() siblings, and a single-write close(). Each asserts the body plus the framing (Content-Length: 10 for the unflushed cases, Transfer-Encoding: chunked for the flushed ones).
Two close() siblings in the existing h3 backpressure block (sync and async pull()).

On the unfixed build 5 of them fail (3 HTTP/1 sync close() cases, both h3 cases); with the fix all 23 tests in the file pass, including the #36940 single-terminator test, whose close() now goes through the new path. serve.test.ts, bun-server.test.ts, serve-http3.test.ts, the other serve-*stream* / *-leak files and test/js/web/streams pass as well (the remaining failures in serve.test.ts / bun-server.test.ts are the localhost / IPv6 / root-port ones that fail identically with the stock binary in this environment).

…close()d HTTPServerWritable::end() (the controller close() path) left bytes buffered below the high-water mark for the auto-flusher. A direct stream whose pull() closes synchronously is torn down by do_render_stream as soon as pull() returns, before any deferred task runs, so the tail was dropped and the client received Content-Length: 0, or a chunked body missing its tail after a mid-stream flush(). Over HTTP/3 the same happened from an async pull(): the auto-flusher's try_end hit backpressure with no pending flush parked for the request to wait on. end() now sends the tail itself exactly like end_from_js(): try_end / end through uWS, or park a pending_flush promise on backpressure so do_render_stream and handle_resolve_stream wait for on_writable to drain it. The shared pieces (parking the flush promise, the post-end bookkeeping) are factored into park_pending_flush() and handle_ended_response(). The requested_end branch of on_auto_flush() only served the deferred end and is removed; a debug assertion documents that the flusher never sees an ended sink.

coderabbitai · 2026-08-12T01:06:48Z

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 18 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: aa5e9b9e-48b9-4263-8747-db595664def5

📥 Commits

Reviewing files that changed from the base of the PR and between e7abdf7 and 34631f2.

📒 Files selected for processing (2)

src/runtime/webcore/streams.rs
test/js/bun/http/serve-direct-readable-stream.test.ts

_{Comment @coderabbitai help to get the list of available commands.}

robobun · 2026-08-12T01:24:22Z

^{Updated 2:45 AM PT - Aug 13th, 2026}

✅ @robobun, your commit 34631f2e4d0ee6ea07def9394bc8ca345cb56a83 passed in Build #94476! 🎉

🧪 To try this PR locally:

bunx bun-pr 37696

That installs a local version of the PR into your bun-37696 executable, so you can run:

bun-37696 --bun

claude

I reviewed this PR and didn't find any bugs. Because it reworks control flow in HTTPServerWritable::end() — a lifetime-sensitive teardown path in the HTTP server that already has several ASAN regression guards in this file — a human look would still be worthwhile.

What was reviewed:

global_this is set at sink construction in do_render_stream (RequestContext.rs:2021), so self.global_this() in the new end() backpressure branch cannot panic.
send_readable() unconditionally unregisters the auto-flusher, so the new debug_assert!(!self.requested_end) in on_auto_flush holds on both the drained and parked branches of end()/end_from_js().
The protect() in park_pending_flush() is balanced by unprotect() in flush_promise(); discarding the return value in end() is fine because do_render_stream/handle_resolve_stream pick the promise up from sink.pending_flush.
handle_ended_response is a byte-for-byte extraction of the block it replaces in end_from_js, plus threading err through to source.close (matching the empty-buffer paths in end()).

Extended reasoning...

Overview

This PR fixes data loss in Bun.serve when a direct ReadableStream controller's close() is called while bytes are still buffered in the sink. The fix makes HTTPServerWritable::end() (in src/runtime/webcore/streams.rs) send its buffered tail immediately via send_readable(0) — mirroring what end_from_js() already does — and park a pending_flush promise on transport backpressure so do_render_stream / handle_resolve_stream wait on it. Two shared blocks are extracted into park_pending_flush() and handle_ended_response(), and the now-unreachable requested_end branch in on_auto_flush() is replaced with a debug assertion. New tests cover sync/async close() with and without a mid-stream flush(), plus HTTP/3 backpressure siblings.

Security risks

None identified. The change is confined to server-side response body draining; no parsing of untrusted input, no auth/permission surface, no new allocation shapes. The protect()/unprotect() pairing on the parked promise is unchanged from the pre-existing flush_from_js/end_from_js pattern.

Level of scrutiny

High. HTTPServerWritable teardown is production-critical and has a documented history of UAF bugs (three ASAN-gated tests in this very file guard sink/socket lifetime around end()). This change alters when mark_done()/finalize() run relative to uWS try_end, removes a code path based on a reachability argument, and introduces a new backpressure branch in end() that leaves the sink alive with a parked promise. The reasoning in the PR description is sound and I traced each claim (auto-flusher unregistered on every end path via send_readable → unregister_auto_flusher; global_this installed at construction; do_render_stream picks up pending_flush when assignToStream returns undefined), but a maintainer familiar with the h3 markDone/onAborted interaction and the #36940 double-terminator history should confirm the removed clear_on_writable() and the new eager-send in end() don't reintroduce either.

Other factors

Test coverage is thorough (7-case matrix over sync/async × close/end × flushed/unflushed asserting body + framing headers, plus two h3 backpressure siblings), and the PR description confirms the wider serve* / streams suites still pass. The refactor is behavior-preserving for end_from_js and flush_from_js (byte-identical extraction). One candidate issue — the debug_assert firing when end() parks under backpressure — was raised by a finder and refuted: send_readable() unregisters the flusher before returning false. Given the sensitivity of this code path and the removal of a defensive branch, deferring to human review.

robobun · 2026-08-12T01:31:48Z

For whoever takes the human look, the two points above are covered like this:

Removed clear_on_writable(): the drained branch of end() only runs after uWS end() / a completed try_end(), and both transports drop the drain callback in markDone() themselves (packages/bun-uws/src/HttpResponseData.h:47-51 for HTTP/1, packages/bun-uws/src/Http3Response.h:279-280 for HTTP/3). end_from_js() has been relying on exactly that; the parked branch keeps its callback armed on purpose, since on_writable is what resends the tail.
Bun.serve writes the chunked terminator twice when a direct ReadableStream is closed with unflushed data #36940: the eager send sets ended_response through handle_ended_response() before finalize() runs, so finalize() skips end_stream() the same way it does for end_from_js(). The single-terminator test from Bun.serve: write the chunked terminator once when a direct stream closes with unflushed data #36943 (close() with unflushed data writes the chunked terminator exactly once) now exercises this new path and passes.

robobun · 2026-08-12T01:35:20Z

CI status: build 92696 is green on 189 of 190 jobs. The one red job is Windows 2019 x64, where test/bake/deinitialization.test.ts segfaults during dev server teardown; that failure also shows up in the final builds of recently merged PRs, so it is independent of this change and has been reported separately. The remaining annotations are retry-passed flakes on unrelated tests.

… with nothing buffered

github-actions · 2026-08-13T09:27:47Z

+    // JSC_BORROW: process-lifetime VM global, installed by
+    // `RequestContext::do_render_stream` at construction. Safe `Deref` via `BackRef`.
