Overview

This PR changes how Bun.serve handles a type: "direct" ReadableStream whose pull() returns synchronously without ending the sink — the shape react-dom/server.bun's renderToReadableStream uses for Suspense streaming. Three coordinated changes:

• ReadableStreamInternals.ts: readDirectStream now returns a close-promise capability when sync pull() leaves the stream readable, so native consumers wait for end() instead of finalizing immediately.
• RequestContext.rs (handle_resolve_stream): if endFromJS() parked a pending_flush (h3 backpressure right after HEADERS), defer sink teardown — ref_(), wrap the flush in a NativePromiseContext, attach ON_RESOLVE_STREAM/ON_REJECT_STREAM, and re-enter when it settles.
• generate-jssink.ts (detach()): invoke m_onClose via AsyncContextFrame::call so the close callback fires when the stream was constructed under AsyncLocalStorage.run() (previously getCallData reported the wrapped frame as not callable and silently skipped it, leaking the request).

Tests cover progressive streaming, empty-shell-then-write, client-disconnect cancel, h3 backpressure (async pull ending sync, sync pull ending from a microtask), an ALS heapStats leak regression, and the existing 10k-request sink-leak fixture retargeted at the new path. body-stream.test.ts was tightened from toBeOneOf(["hey", ""]) to toBe("hey").

Prior review

In the previous run I flagged two issues, both now resolved (commit 4342087):

1. ALS-wrapped streams leaked because detach() couldn't call an AsyncContextFrame — fixed by routing through AsyncContextFrame::call, with a heapStats regression test.
2. serve-response-stream-sink-leak-fixture.ts relied on the old truncating fallback and would hang — fixture now ends each stream from the client side and still asserts flat memory.

The bug-hunting system found nothing new this round.

Security risks

None apparent. No auth, crypto, parsing of untrusted input, or permission surfaces are touched. The change is internal stream-completion plumbing. The main risk class is correctness/availability: a mis-balanced ref_() or a never-settling close promise would leak/hang requests, and a premature teardown on the new pending_flush re-entry path could truncate h3 responses. Those are reliability concerns, not security exposures.

Level of scrutiny

High. This is core HTTP server request lifecycle:

• Adds a req.ref_() in handle_resolve_stream that must be balanced by the re-entered resolve/reject handler in every code path (including abort racing the parked flush).
• Changes when the heap ResponseStreamJSSink is destroyed.
• Alters generated C++ that runs for every sink controller (all six sink classes).
• Changes the contract of readDirectStream's return value, which other native consumers (e.g. FileSink) also depend on.

The interactions between abort, h3 backpressure, ALS context wrapping, and the new close-promise are subtle enough that I'm not comfortable approving without a human pass.

Other factors

• CI: robobun reports failures in Build #61975 on the latest commit (bdc63ca, the autofix-applied version of 4342087). Worth checking whether those are related before merge.
• Test coverage is good and targeted; each of the three fixes has a test that fails without it.
• The body-stream.test.ts change is inside a describe.each over http/1.1 and http/3 with a shared pulled/controller capture; the test only fetches once per row so the module-scope capture is fine, but it's the kind of thing a reviewer should glance at.
• No CODEOWNERS file in the repo, so no ownership gate.

Overview

This PR changes how Bun.serve handles a type: "direct" ReadableStream whose pull() returns synchronously without ending the sink. It spans three layers: readDirectStream in ReadableStreamInternals.ts now returns a close-promise capability instead of undefined; handle_resolve_stream in RequestContext.rs defers sink teardown while a pending_flush is parked (with a new ref_() and re-entry through ON_RESOLVE_STREAM/ON_REJECT_STREAM); and the codegen'd controller detach() now invokes m_onClose via AsyncContextFrame::call so an ALS-wrapped callback actually fires. Tests cover the React-shaped scenario, client-disconnect cancel, h3 backpressure, an ALS leak regression, and the existing 10k-request sink-leak fixture was retargeted to the new path.

Security risks

None apparent — no auth, parsing, or untrusted-input handling is touched. The risk profile here is correctness/leaks (unbalanced ref_(), never-settling promises, sinks not destroyed) rather than security.

Level of scrutiny

High. This is a behavior change on the hot HTTP response path, touching request-context refcounting and sink lifetime across JS builtins, Rust, and generated C++. Two real bugs (the ALS detach() skip and the broken sink-leak fixture) were already found and fixed during review, which is a good sign for test coverage but also confirms the change is subtle. The new pending_flush re-entry in handle_resolve_stream adds another ref/promise edge to the teardown graph and deserves eyes from someone who owns that code.

Other factors

My two prior inline findings are resolved with regression tests in 4342087. The bug-hunting pass on the current revision found nothing. However, robobun reports failures in Build #61975 for the latest commit; the author attributes them to an unrelated stale-workspace issue, but that should be confirmed green before merge. The semantic shift (sync pull() without end() now holds the response open instead of truncating) is intentional and well-argued, but it's exactly the kind of contract change a maintainer should sign off on.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/codegen/generate-jssink.ts (2)

470-476: 🩺 Stability & Availability | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Inspect existing JSC exception-preservation patterns before patching this generated path.
# Expectation: find a local pattern that roots/protects exceptions or JSValues before clearing a ThrowScope
# and running code that can execute JS.
rg -n -C4 'scope\.exception\(\)|tryClearException\(\)|Strong<.*Exception|throwException\(.*pending|throwException\(.*exception' src

Repository: oven-sh/bun

Length of output: 50368

---

🏁 Script executed:

#!/bin/bash
# Get the file size first to check if we can read it
wc -l src/codegen/generate-jssink.ts

# Read the relevant sections to understand the code structure
echo "=== Lines 460-480 ==="
sed -n '460,480p' src/codegen/generate-jssink.ts

echo "=== Lines 510-530 ==="
sed -n '510,530p' src/codegen/generate-jssink.ts

Repository: oven-sh/bun

Length of output: 1776

---

🏁 Script executed:

#!/bin/bash
# Search for how Strong<> is used with Exception in the JSC bindings
rg -n 'Strong.*Exception|protect.*exception|exception.*protect' src/jsc --max-count 20

# Also look for how exceptions are preserved across JS-executing calls
echo "=== Looking for exception preservation patterns ==="
rg -n -B3 -A3 'exception.*Strong|Strong.*exception' src/jsc/bindings

Repository: oven-sh/bun

Length of output: 2001

---

🏁 Script executed:

#!/bin/bash
# Search for how Strong<> is created and used in generated bindings
rg -n 'Strong<.*>\(' src/jsc/bindings --max-count 10

# Look for JSC::Strong declaration and usage patterns
rg -n 'JSC::Strong' src/jsc --max-count 5

# Search for exception rooting in similar detach/cleanup patterns
rg -n -B5 -A5 'controller.*detach|detach.*exception' src/runtime

Repository: oven-sh/bun

Length of output: 17097

---

🏁 Script executed:

#!/bin/bash
# Look at how exceptions are held and rethrown in similar patterns
rg -n -B2 -A8 'auto.*exception.*scope\.exception|Exception.*scope\.exception' src/jsc/bindings | head -100

# Check if Strong<Exception> is used anywhere
rg -n 'Strong.*Exception\*|Strong<.*Exception' src --max-count 20

Repository: oven-sh/bun

Length of output: 8822

---

🏁 Script executed:

#!/bin/bash
# Check if Exception::value() needs rooting or if Strong can protect the exception cell directly
rg -n -B2 -A2 'exception.*value\(\)|Exception.*value()' src/jsc --max-count 20

# Look at what type Exception is and how it's stored
rg -n 'class Exception|struct Exception' src/jsc --max-count 5

# See how Strong is used with exception values specifically
rg -n -B3 -A3 'Strong.*exception\->value\(\)|Strong.*exception->value' src/jsc/bindings

Repository: oven-sh/bun

Length of output: 50367

---

🏁 Script executed:

#!/bin/bash
# Check if this is a generated file
head -20 src/codegen/generate-jssink.ts | grep -i 'generated\|autogen\|generated file'

# Look at the actual code pattern used for exception rooting in similar contexts
rg -n -B5 -A5 'Strong.*vm.*exception|Strong.*exception.*value\(\)' src/jsc/bindings | head -50

Repository: oven-sh/bun

Length of output: 149

---

🏁 Script executed:

#!/bin/bash
# Look for how to handle exceptions that need to survive JS-executing calls
# Check if there are patterns of protecting exception values specifically
rg -n -B3 -A8 'exception.*value\(\).*Strong|Strong.*exception.*value' src/jsc/bindings | head -60

# Also check what protect/ensure_still_alive does for exceptions
rg -n 'ensure_still_alive.*exception|protection.*exception' src

Repository: oven-sh/bun

Length of output: 149

---

🏁 Script executed:

#!/bin/bash
# Look at what the generated code looks like and understand the full context
sed -n '450,490p' src/codegen/generate-jssink.ts

# Check what controller->detach does and if it can run JS
rg -n 'void.*detach\(' src/runtime/webcore --max-count 5

Repository: oven-sh/bun

Length of output: 1855

---

Protect the pending exception value before detach() runs user JS.

The exception is extracted as a raw JSC::Exception*, then scope.tryClearException() removes the VM's exception root. At that point, controller->detach() can invoke m_onClose (user code) and trigger GC, leaving the raw pointer dangling.

Create a JSC::Strong<JSC::Unknown> to hold the exception value before clearing the scope, then rethrow the protected value after detach() completes:

if (JSC::Exception* pending = scope.exception()) [[unlikely]] {
    JSC::Strong<JSC::Unknown> protectedValue(vm, pending->value());
    if (!scope.tryClearException()) {
        return {};
    }
    controller->detach();
    (void)scope.tryClearException();
    scope.throwException(lexicalGlobalObject, protectedValue.get());
    return {};
}

Same fix applies to the identical pattern at lines 517–523 in the __end function.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/codegen/generate-jssink.ts` around lines 470 - 476, Protect the pending
exception value before scope clearing and detach() to prevent dangling pointer
issues. In the exception handling block where JSC::Exception* pending is
captured, create a JSC::Strong<JSC::Unknown> to protect pending->value() before
calling scope.tryClearException(). After controller->detach() completes, rethrow
the exception using the protected Strong reference instead of the raw pointer.
Apply this same fix to the identical exception handling pattern in the __end
function (around lines 517-523).

Source: Coding guidelines

---

716-728: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Clear m_onClose before invoking it.

AsyncContextFrame::call runs user JS, and the callback can re-enter detach() while m_onClose is still set, causing duplicate close/cancel callbacks. Copy the callback, clear the stored one-shot state, then call it.

🔁 Proposed fix

-    auto readableStream = m_weakReadableStream.get();
-    auto onClose = m_onClose.get();
+    auto readableStream = m_weakReadableStream.get();
+    JSC::JSValue onClose = m_onClose.get();
+    m_onClose.clear();
+    m_weakReadableStream.clear();
 
     if (readableStream && onClose) {
         JSC::JSGlobalObject *globalObject = this->globalObject();
         JSC::MarkedArgumentBuffer arguments;
         arguments.append(readableStream);
@@
         // unwraps the frame and also handles plain callables.
         AsyncContextFrame::call(globalObject, onClose, JSC::jsUndefined(), arguments);
     }
-
-    m_onClose.clear();
-    m_weakReadableStream.clear();

As per coding guidelines, stored one-shot callbacks should be copied and nulled before invoking user JS.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/codegen/generate-jssink.ts` around lines 716 - 728, In the block where
readableStream and onClose are checked before calling AsyncContextFrame::call,
clear the m_onClose member variable before invoking the callback. After
obtaining the onClose value via m_onClose.get(), reset m_onClose to null (or use
whatever method clears this member) before calling AsyncContextFrame::call with
the onClose parameter. This prevents re-entrancy issues where the callback can
re-enter detach() and cause duplicate close/cancel callbacks, following the
one-shot callback pattern of copying the value, clearing the stored state, then
invoking it.

Source: Coding guidelines

src/js/builtins/ReadableStreamInternals.ts (2)

951-952: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Avoid coercing Promise write results in the backpressure check.

The comments say FileSink may return a Promise; sink.write(...) < 0 / controller.write(...) < 0 coerces non-number results, which can run user-overridable coercion hooks and turn the intended “Promise falls through” path into userland execution. Cache the result and compare only numbers.

🛡️ Proposed fix

-      if (sink.write(values[i]) < 0) {
+      const written = sink.write(values[i]);
+      if (typeof written === "number" && written < 0) {
         await sink.flush(true);
         // The sink's close path resolves the same promise; stop writing into a
         // dead sink.
         if (state.didClose) break;
@@
-      if (sink.write(value) < 0) {
+      const written = sink.write(value);
+      if (typeof written === "number" && written < 0) {
         await sink.flush(true);
         if (state.didClose) return sink.end();
       }
@@
-          if (controller.write(value) < 0) {
+          const written = controller.write(value);
+          if (typeof written === "number" && written < 0) {
             clearImmediate(immediateTask);
             immediateTask = undefined;
             await controller.flush(true);

As per coding guidelines, builtin JS modules must avoid user-overridable machinery on hostile/user-facing paths.

Also applies to: 973-974, 1912-1915

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/js/builtins/ReadableStreamInternals.ts` around lines 951 - 952, The
backpressure check at lines 951-952 and similar locations coerces the result of
sink.write() to a number using the comparison operator, which can trigger
user-overridable coercion hooks when the result is a Promise. Store the return
value of sink.write(values[i]) and controller.write() calls in a variable, then
check if it is a number before performing the less-than comparison with 0,
avoiding implicit coercion. Apply this fix to all affected backpressure checks
at the mentioned line ranges: 951-952, 973-974, and 1912-1915.

Source: Coding guidelines

---

1043-1048: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Wrap the underlyingSource.close lookup too.

underlyingSource.close can be a user-defined getter; reading it before the try lets a thrown getter skip pending-read rejection, sink.end(), and stream cleanup. Put the property lookup and call in the same guarded block.

🛡️ Proposed fix

-  const underlyingSource = this.$underlyingSource;
-  const underlyingClose = underlyingSource.close;
-  if (typeof underlyingClose === "function") {
-    try {
+  try {
+    const underlyingSource = this.$underlyingSource;
+    const underlyingClose = underlyingSource?.close;
+    if ($isCallable(underlyingClose)) {
       underlyingClose.$call(underlyingSource, e);
-    } catch {}
-  }
+    }
+  } catch {}
@@
-  const underlyingSource = this.$underlyingSource;
-  const underlyingClose = underlyingSource.close;
-  if (typeof underlyingClose === "function") {
-    try {
+  try {
+    const underlyingSource = this.$underlyingSource;
+    const underlyingClose = underlyingSource?.close;
+    if ($isCallable(underlyingClose)) {
       underlyingClose.$call(underlyingSource, reason);
-    } catch {}
-  }
+    }
+  } catch {}

As per coding guidelines, anything that can run user JS, including getters, must be handled before relying on subsequent cleanup.

Also applies to: 1202-1207

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/js/builtins/ReadableStreamInternals.ts` around lines 1043 - 1048, The
property lookup for underlyingSource.close is occurring outside the try-catch
block, but since close can be a user-defined getter that may throw, this allows
errors to bypass the exception handler and skip necessary cleanup operations.
Move the underlyingSource.close property lookup inside the try block so that any
getter execution is caught and handled properly. Apply this same fix to both
occurrences in the file (the initial location and the secondary location around
lines 1202-1207).

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes