[robobun]

Problem

When a Bun.serve fetch handler proxies by returning a fetch() Response directly:

Bun.serve({
  fetch: (req) => fetch(upstreamUrl),
});

and the client disconnects mid-response, Bun keeps pulling the entire upstream body into a dead socket. A client that took 1 MB and left would still cause the proxy to pull all 512 MB from the origin.

The workarounds that already cancel correctly are fetch(url, { signal: req.signal }) and rewrapping the body via new Response(res.body, res).

Repro

import net from "node:net";
const MB = 1024 * 1024, CHUNK = 256 * 1024, N = 2048;
const st = { sent: 0, done: false, cancelled: false };
const U = net.createServer(s => {
  let closed = false;
  s.on("close", () => { closed = true; if (!st.done) st.cancelled = true; });
  s.once("data", () => {
    let n = 0;
    s.write(`HTTP/1.1 200 OK\\r\\ncontent-length: ${N * CHUNK}\\r\\n\\r\\n`);
    const pump = () => { let ok = true; while (ok && n < N) { if (closed) return; ok = s.write(new Uint8Array(CHUNK)); n++; st.sent = Math.round(n * CHUNK / MB); } if (n >= N) { st.done = true; s.end(); } };
    s.on("drain", pump); pump();
  });
});
await new Promise(r => U.listen(0, "127.0.0.1", r));
const P = Bun.serve({ port: 0, idleTimeout: 255, fetch: () => fetch(`http://127.0.0.1:${U.address().port}/`) });
const c = net.connect(P.port, "127.0.0.1");
await new Promise(r => c.on("connect", r));
c.write("GET / HTTP/1.1\\r\\nHost: x\\r\\n\\r\\n");
let got = 0;
await new Promise(r => c.on("data", d => { got += d.length; if (got > MB) r(); }));
c.destroy();
// before: upstream keeps going until st.sent == 512, st.done == true
// after:  upstream socket closes at st.sent ~= 5-20

Cause

do_render_with_body routes a fetch() Response body (Locked{task: FetchTasklet, readable: None}) through the Source::Bytes arm: it materializes the ByteStream, stores it in this.byte_stream / this.response_body_readable_stream_ref, takes a ref for the pipe, and sets the Response body to Used.

On client disconnect, on_abort tries two things:

• this.sink: only set for the JS ReadableStream (do_render_stream) path, so it's None here.
• response_weakref.get_body_readable_stream(): checks the JS-side cached stream slot (never populated for a direct fetch Response) and Value::Locked.readable (body is Used), so it returns None.

Neither this.byte_stream nor this.response_body_readable_stream_ref is consulted, so the stream's cancel_handler (FetchTasklet::on_stream_cancelled_callback) never fires and the upstream connection stays open. on_pipe keeps receiving chunks and silently discards them (it early-returns on is_aborted_or_ended()).

Fix

Handle this.byte_stream in on_abort alongside this.sink: unpipe, abort the stream held in response_body_readable_stream_ref (which fires the FetchTasklet cancel handler and closes the upstream connection), and balance the ref the pipe setup took (since on_pipe's is_done deref won't fire once the stream is cancelled).

Verification

# before (USE_SYSTEM_BUN=1)
{"mode":"return-fetch","upstreamSentMB":512,"ranToCompletion":true,"cancelledByProxy":false}

# after (bun bd)
{"mode":"return-fetch","upstreamSentMB":9,"ranToCompletion":false,"cancelledByProxy":true}

New test in test/js/bun/http/serve.test.ts fails on main (ranToCompletion: true), passes with the fix.

---

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/bun/http/serve.test.ts

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 1 minute

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: 63defb2d-cf63-4044-9fd6-f36e321e9675

📥 Commits

Reviewing files that changed from the base of the PR and between 85f22bb and 5539f7a.

📒 Files selected for processing (2)

• src/runtime/server/RequestContext.rs
• test/js/bun/http/serve.test.ts

---

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

[robobun]

^{Updated 10:16 AM PT - Jul 25th, 2026}

⏳ @robobun, your commit 5539f7a is still building in Build #80540, but has 1 failures so far (All Failures):

• step failed outside runner - exit 1 on 🪟 aarch64 - build-bun
• step failed outside runner - exit 1 on 🍎 aarch64 - build-bun

[claude]

ee6e696 addresses both prior findings — on_pipe now clears byte_stream alongside the is_done guard's deref, so the invariant (byte_stream Some ⟹ pipe ref outstanding) holds on all paths I traced, and the test now wires error/close to reject and cleans up in try/finally. No new issues found. Deferring only because this is ref-counting in RequestContext and a maintainer should confirm the balance trace; the two comment-cop flags on the new comments are also still open (though the comments read as invariant docs to me, not workaround justification).

Extended reasoning...

Overview

Two-file change: src/runtime/server/RequestContext.rs adds a new branch in on_abort to cancel the upstream ByteStream when a client disconnects from a proxied return fetch(url) response, plus a paired change in on_pipe to clear byte_stream when is_done fires. test/js/bun/http/serve.test.ts adds a regression test that pumps 256 MB from a raw-socket upstream through a Bun.serve proxy and asserts the upstream stops after the client disconnects at ~1 MB.

Prior findings and how ee6e696 resolves them

My earlier 🔴 finding was that the original on_abort branch's unconditional deref() could double-drop the pipe ref when on_pipe(is_done, backpressure) had already dropped it while leaving byte_stream set. The follow-up commit clears byte_stream (and unpipes) at the top of on_pipe's is_done path, so the invariant now holds: byte_stream is only Some while the ref_() at :3232 is outstanding. I re-traced the three sites that clear byte_stream (:1466, :1560, :3298) and the two sites that drop the pipe ref (_ref guard at :3293, deref() at :1474) — each clear pairs with exactly one drop, and finalize_without_deinit's take at :1560 is now defensive-only. The 🟡 test nit is also addressed: connect/data awaits reject on error/close, and both client and upstream are torn down in nested try/finally.

Security risks

None specific to this change. It narrows resource consumption (stops an unbounded upstream pull on client abort), which is a mild DoS-resilience improvement.

Level of scrutiny

High. RequestContext ref-counting is exactly the "reference counts provably balanced on every terminal path" category REVIEW.md calls out as most-blocked. The fix looks correct, but the correctness argument is a multi-path ref-count trace across on_abort / on_pipe / end_stream / on_writable_response_buffer, and a wrong balance here is a pooled-slot UAF in release. That's not something I should approve without a maintainer signing off on the trace.

Other factors

The comment-cop action left two open flags on the 4–5 line comments at :1461-1465 and :3295-3297. Those comments document a non-local ref-count invariant, which REVIEW.md's "comments carry only durable non-obvious content: invariants, ownership/lifetime contracts" explicitly endorses — but the bot flags are still unaddressed on the thread and a human should make the call. The test now runs the raw-socket upstream in-process with port: 0, wires all failure events, and asserts ranToCompletion === false / sentChunks < TOTAL_CHUNKS after upstreamClosed resolves, so the known regression path fails cleanly.

[claude]

Both earlier findings are addressed — the ref-count now balances on the mid-stream-abort, is_done+WantMore, and is_done+backpressure→abort paths, and the test wires error/close to reject with try/finally cleanup. Deferring only because RequestContext ref-counting is the most-blocked review category and worth a maintainer sanity-check.

What was reviewed:

• Traced ref_count through on_abort's new byte_stream branch: deref() + the line-1408 guard drop take [2]→[0], matching create+pipe refs.
• Verified ee6e696's on_pipe change restores the invariant (byte_stream Some ⇔ pipe ref live), so the is_done+backpressure→abort path no longer double-derefs.
• Confirmed readable_stream::Strong::deinit() and unpipe_without_deref are idempotent, so the follow-on finalize_without_deinit is safe.

Extended reasoning...

Overview

Two-file change: ~20 lines in src/runtime/server/RequestContext.rs adding a byte_stream branch to on_abort (unpipe → abort the held ReadableStream → deinit strong ref → deref the pipe ref) plus a matching byte_stream.take() in on_pipe when is_done fires; and a ~75-line regression test in test/js/bun/http/serve.test.ts that proxies a 256 MB upstream via return fetch(url), drops the client after 1 MB, and asserts the upstream socket closes early.

Security risks

None identified. This is resource-lifecycle plumbing on the server response path; no auth, crypto, or untrusted-input parsing is touched. The only risk vector is memory safety (ref-count imbalance / UAF on the pooled RequestContext), which is what the review focused on.

Level of scrutiny

High. RequestContext ref-counting is exactly the pooled-slot + u8-refcount pattern REVIEW.md's memory-safety section calls out as the most-blocked category, and the first draft of this PR did have an imbalance (double-deref when is_done hit backpressure before abort). That was fixed in ee6e696 by clearing byte_stream in on_pipe when is_done, so the on_abort branch's extra deref is now gated on the pipe ref actually being outstanding.

I re-traced the paths after the fix:

• Mid-stream abort (byte_stream Some, pipe ref live): entry ref_count=2 → this.deref() at 1470 → 1 → _ref guard at 1408 drops → 0 → finalize+deinit. Balanced.
• is_done + WantMore: on_pipe takes byte_stream, unpipes, end_stream(), _ref guard drops pipe ref. Later abort sees byte_stream=None, falls through to pre-existing path.
• is_done + backpressure → abort: on_pipe takes byte_stream, _ref guard drops pipe ref → ref_count=1. Abort sees byte_stream=None, falls through; _ref at 1408 drops → 0. Balanced (this is the path that was broken pre-ee6e696).
• Double-cleanup: response_body_readable_stream_ref.deinit() is called in the new branch and again in finalize_without_deinit; strong::Optional::deinit uses handle.take() so it's idempotent. unpipe_without_deref just nulls two fields. finalize_without_deinit is documented multi-call-safe.

Other factors

The test now rejects on client error/close and wraps upstream/client teardown in try/finally (ee6e696), addressing the earlier nit. The comment-cop bot's multi-line-comment complaints were trimmed in c0f9c52. The byte_stream_unpipe shim's doc-comment still says "lone caller" but there are now three call sites — cosmetic only. CI build #80254 hadn't reported a result in the timeline I fetched.

Given the change sits in pooled-request lifecycle code and already needed one ref-count correction, a maintainer pass on the terminal-path enumeration is worth the extra eyes even though I found nothing further.

[claude]

The ref-count over-deref I flagged earlier is correctly addressed — ee6e696 clears byte_stream in on_pipe when is_done fires, so the "byte_stream Some ⇒ pipe ref live" invariant now holds and on_abort's extra deref() is balanced on every path I traced. The test hardening from that round is also in. That said, this is manual ref-counting on the pooled RequestContext in the server hot path (the exact category REVIEW.md calls the most-blocked), and the first cut had a real over-deref — worth a human pass.

What was reviewed

• Ref-count trace across Source::Bytes setup (3228) → on_pipe(is_done) both WantMore/backpressure arms → on_abort new branch → _ref guard at 1408; balanced on all paths after ee6e696.
• byte_stream_unpipe from inside on_pipe: safe — ByteStream::on_data snapshots the pipe fn/ctx before dispatch and returns immediately after.
• Re-entrancy of stream.abort() at 1466: pipe already cleared at 1463, so no on_pipe re-entry; response_body_readable_stream_ref keeps the ByteStream alive across the call.
• Test: error/close now reject the awaits, upstream/client teardown is try/finally-guarded.

Extended reasoning...

Overview

The PR fixes a resource leak where Bun.serve({ fetch: () => fetch(upstream) }) keeps pulling the entire upstream body after the client disconnects. It adds a branch to RequestContext::on_abort that handles the Source::Bytes pipe path (this.byte_stream set): unpipe the ByteStream, abort the ReadableStream held in response_body_readable_stream_ref (fires FetchTasklet::on_stream_cancelled_callback, closing the upstream connection), release the strong ref, and drop the pipe ref taken in do_render_with_body's Source::Bytes arm. A companion change in on_pipe clears byte_stream and unpipes when is_done fires so that "byte_stream is Some" is a reliable proxy for "the pipe ref is still outstanding" — this was the fix for the over-deref I flagged in the first review round. A regression test in serve.test.ts sets up a raw-socket upstream that pumps 256 MB, a proxy that returns fetch(upstreamUrl), and a raw client that disconnects after 1 MB, then asserts the upstream socket closed before completion.

Verification of the ee6e696 follow-up

I re-traced the ref count for the specific scenario my earlier comment described (on_pipe(is_done=true) hits backpressure, then client aborts before on_writable_response_buffer). With the new on_pipe block at 3290-3295, byte_stream is taken and unpiped before the backpressure branch registers on_writable, and the _ref guard at 3289 drops the pipe ref on scope exit. So when on_abort subsequently runs, byte_stream is None, the new branch is skipped, and control falls through to is_dead_request() → finalize_without_deinit() → guard at 1408 drops the last ref. No double-deref. I also checked the mid-stream abort path (the actual bug being fixed): setup leaves ref_count = 2 (initial + pipe); on_abort enters the new branch, deref() at 1470 drops to 1, guard at 1408 drops to 0. Balanced.

I also confirmed that calling byte_stream_unpipe from inside the pipe callback is safe: ByteStream::on_data (ByteStream.rs:189-196) copies (pipe.ctx, pipe.on_pipe) to the stack before invoking the handler and does nothing with the ByteStream after the handler returns, so clearing pipe mid-callback can't invalidate the dispatch. And stream.abort() at 1466 can't re-enter on_pipe because the unpipe at 1463 already cleared the callback.

Security risks

None in the sense of injection/auth. The risk category here is memory safety: an unbalanced deref on a pooled RequestContext slot is a use-after-free on the next request that reuses the slot (release) or a debug-assert crash. That's exactly what the first iteration would have hit under is_done + backpressure + abort ordering, and it's fixed.

Level of scrutiny

High. This is manual ref-counting on the server request object, in on_abort — a callback that runs concurrently with (in event-loop terms) every other completion path. REVIEW.md explicitly calls memory safety the most-blocked category, and "reference counts provably balanced on every terminal path" is the bar. My previous review found a real over-deref in the first cut; the follow-up fixes it, but that history plus the hot-path location means a maintainer should sign off on the invariant rather than take my trace alone.

Other factors

The test now wires error/close to reject on both client-side awaits and wraps upstream/client teardown in try/finally, addressing the second inline comment. The comment-cop bot's paragraph-comment complaints were resolved in c0f9c52. CI shows "step failed outside runner" on several build jobs — looks like infra, but the test lane hasn't reported yet.

[robobun]

Status: ready for review; CI failures are build infrastructure, not this change.

The diff compiles and tests pass on every lane that produced a binary:

• #80290: linux x64-asan and windows x64 built; 16/16 test-bun shards on those lanes passed, 0 failed. Six other build-bun jobs exited with Sibling step <lane>-build-cpp errored [0s] while the referenced build-cpp jobs were still in scheduled (orchestration race, Rust compile itself finished cleanly).
• #80540 (retrigger): linux x64, linux aarch64-musl, linux aarch64-android built; 26/26 completed test-bun shards passed, 0 failed. Only failure is windows x64 build-bun timing out (same lane was green on #80290 and also timed out on #80194 from main's base).

Locally verified (ASAN debug build):

• new serve.test.ts test fails on main with ranToCompletion: true, passes with the fix
• full serve.test.ts, serve-body-leak.test.ts, serve-response-gc-backpressure-abort.test.ts green vs. main baseline

The ref-count balance across on_abort / on_pipe paths is traced in the review thread above; happy to walk through any path in more detail.

[robobun]

Closing in favor of #36700. It is the same fix (cancel the natively piped upstream body in RequestContext::on_abort and release the pipe's ref), rewritten for the SinkHandle structure that #36087 introduced. This branch predates #36087 and conflicts with main. #36700 also clears byte_stream in end_chunk, so the two release sites stay exclusive, and it has its own test.

[robobun]

Update: the fix landed on main through #39590 (7da5a09), not through #36700, which is closed as well. Main cancels the natively piped body in RequestContext::on_abort and test/js/web/fetch/fetch-stream-cancel-leak.test.ts covers the return fetch(url) shape.