fetch: protect stream error JSValue stored in ByteStream pending result

[robobun]

Problem

ByteStream.append / onData stored stream.err directly into pending.result when there was no pending pull and no buffer action. When the error is a StreamError.JSValue, that raw JSValue was never protect()'d, even though every consumer (Result.deinit, fulfillPromise, toBufferedValue) already treats that variant as strong and unprotects it.

The only GC root for the error in that window is the Strong held in the owning Response's body.value.Error. If user code keeps the ReadableStream but drops the Response, the Response is collected along with that Strong, and pending.result.err.JSValue is left pointing at a freed JSCell. The next Bun.readableStreamToText(body) → toBufferedValue would reject a promise with that collected cell.

This addresses the TODO at streams.zig:225 (// TODO: use an explicit jsc.Strong.Optional here.) at the storage site rather than changing the union type.

Repro

const res = await fetch(urlThatResetsAfterHeaders);
const body = res.body;            // no read(), no buffer action
/* server terminates connection → ByteStream.append stores raw .JSValue */
res = undefined; Bun.gc(true);     // Response + its Strong collected
await Bun.readableStreamToText(body); // reads freed JSCell

Debug build before fix:

ASSERTION FAILED: decontaminate()
.../JavaScriptCore/StructureID.h(94) : Structure *JSC::StructureID::decode() const

Fix

Add ByteStream.setPendingError which protect()s the .JSValue variant before storing it in pending.result, and deinit()s the previous result first (the error is delivered twice on this path — once from FetchTasklet.onBodyReceived directly, once from Body.toErrorInstance — so the old protected value must be released). All existing consumers already unprotect on consumption, so the balance is maintained.

Verification

New test in test/js/web/fetch/fetch.stream.test.ts spawns a subprocess that reproduces the exact sequence (headers → terminate → drop Response → GC → readableStreamToText):

• before: subprocess aborts with the StructureID assertion, stdout empty → test fails
• after: subprocess rejects cleanly with ECONNRESET, prints PASS ECONNRESET → test passes

[robobun]

^{Updated 3:30 AM PT - May 4th, 2026}

@robobun, your commit 9cf788d is building: #51065

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c546d0b9-9ee8-4932-b089-7ef6fb2b4091

📥 Commits

Reviewing files that changed from the base of the PR and between b5bf0e3 and 9e26f14.

📒 Files selected for processing (1)

• test/js/web/fetch/fetch.stream.test.ts

---

Walkthrough

Centralizes stream error storage into pending.result via a new setPendingError, updates stream lifecycle paths to clear/deinit/reset pending.result promptly, and adds a subprocess-isolated regression test exercising fetch body-stream errors under GC pressure.

Changes

Cohort / File(s)	Summary
ByteStream changes src/bun.js/webcore/ByteStream.zig	Introduce setPendingError to store StreamError into pending.result (protect raw JSValue, deinit previous value). Replace direct .err assignments in onData/append; ensure pending_buffer is cleared and pending.result is deinitialized/reset immediately in onCancel; extend deinit and toBufferedValue to reset/deinit pending.result where appropriate.
Regression test test/js/web/fetch/fetch.stream.test.ts	Add a subprocess-based regression test that creates a chunked response, drops the Response to force the body stream into an error state, runs Bun.gc(true) with allocation churn, then verifies Bun.readableStreamToText(body) throws an Error whose message is a string; parent process asserts subprocess stdout begins with PASS and no unexpected stderr.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and specifically describes the main change: adding protection for a JSValue in ByteStream's pending error handling.
Description check	✅ Passed	The PR description comprehensively covers Problem, Repro, Fix, and Verification sections, going beyond the minimal template with excellent context and rationale.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/bun.js/webcore/ByteStream.zig (1)

445-452: 🧹 Nitpick | 🔵 Trivial

Consider adding ensureStillAlive() for consistency with fulfillPromise.

The error is extracted via toJSWeak, then deinit() unprotects it, followed by use on line 451. Comparing with the fulfillPromise pattern in streams.zig (lines 528-535), that code calls ensureStillAlive() before unprotecting:

const js_err, const was_strong = err.toJSWeak(globalThis);
js_err.ensureStillAlive();  // <-- called before unprotect
if (was_strong == .Strong)
    js_err.unprotect();

While this is likely safe in practice (no GC-triggering operations between deinit and use, and the local variable keeps the value alive), adding ensureStillAlive() would make the pattern consistent:

🛡️ Suggested defensive improvement

 if (this.pending.result == .err) {
     const err, _ = this.pending.result.err.toJSWeak(globalThis);
+    err.ensureStillAlive();
     this.pending.result.deinit();
     this.pending.result = .{ .done = {} };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/webcore/ByteStream.zig` around lines 445 - 452, The error-handling
path in the ByteStream code extracts the JS error via
this.pending.result.err.toJSWeak(globalThis) then calls
this.pending.result.deinit() and returns the error without calling
ensureStillAlive(), which differs from the fulfillPromise pattern; update the
error branch to call ensureStillAlive() on the js error returned by toJSWeak
(similar to how streams.zig's fulfillPromise does) before unprotecting/deiniting
the pending result so the JS value is defensively kept alive (reference:
this.pending.result, toJSWeak, ensureStillAlive, deinit, fulfillPromise).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/bun.js/webcore/ByteStream.zig`:
- Around line 445-452: The error-handling path in the ByteStream code extracts
the JS error via this.pending.result.err.toJSWeak(globalThis) then calls
this.pending.result.deinit() and returns the error without calling
ensureStillAlive(), which differs from the fulfillPromise pattern; update the
error branch to call ensureStillAlive() on the js error returned by toJSWeak
(similar to how streams.zig's fulfillPromise does) before unprotecting/deiniting
the pending result so the JS value is defensively kept alive (reference:
this.pending.result, toJSWeak, ensureStillAlive, deinit, fulfillPromise).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ab35a805-a58d-44c8-a230-791b86e07eda

📥 Commits

Reviewing files that changed from the base of the PR and between 18b5c03 and b5bf0e3.

📒 Files selected for processing (1)

• src/bun.js/webcore/ByteStream.zig

[robobun]

Independently reproduced and arrived at the same root cause from a separate report. For reference, branch farm/d9a60f05/protect-stream-error-jsvalue takes the protect-at-producer approach (FetchTasklet, s3/client, Body.toStreamError) and additionally balances the unprotect on the synchronous-consume/discard paths (BufferAction.reject, ByteStream.onData done early-return, RequestContext.onPipe, Body.onStreamPipe) so those don't leak once producers start protecting. It also keeps the append() previous-result deinit() that this PR adds.

See also #29822 which covers the s3 producer and several of the same consumers — these two PRs together cover essentially the same ground.

[claude]

No further issues found, but given this is JSC protect/unprotect balancing on a hot path (and the open question of reconciling this storage-site approach with the producer-side approach in #29822 / the referenced branch), I'd like a human to sign off on the overall direction.

Extended reasoning...

Overview

This PR adds ByteStream.setPendingError which protect()s a StreamError.JSValue before storing it in pending.result, fixing a use-after-free where the error's only GC root (the owning Response's body Strong) could be collected while the ByteStream still held a raw pointer. To keep the protect/unprotect ledger balanced it also: hoists pending.result.deinit() out of the view != .zero guard in onCancel, adds an else branch in deinit() to release a stored error when done was already set, and resets pending.result = .done after deinit() in toBufferedValue. A subprocess-isolated regression test reproduces the original GC crash.

Security risks

None in the conventional sense (no auth/injection/data-exposure surface). The risk class here is memory safety: an unbalanced protect leaks; an unbalanced unprotect can free an unrelated cell at a reused address, i.e. a deferred UAF. Two such imbalances were caught and fixed during this review cycle, which is precisely why I don't think this should be auto-approved.

Level of scrutiny

High. ByteStream is the core fetch response body stream; every streamed HTTP response flows through it. JSC protect/unprotect bookkeeping is notoriously easy to get subtly wrong, and the consequences (heap corruption, intermittent crashes) are hard to diagnose after the fact.

Other factors

All three of my earlier inline comments (cancel-path leak, toBufferedValue double-unprotect, stderr surfacing in the test) have been addressed and resolved. The current bug-hunting pass found nothing. What tips this toward human review is the author's own 2026-05-03 note: an alternative branch takes the protect-at-producer approach (FetchTasklet, s3/client, Body.toStreamError) and additionally balances several synchronous-consume/discard paths, and #29822 overlaps substantially. Choosing storage-site vs. producer-site protection — and deciding how this PR and #29822 should land relative to each other — is a design call best made by a maintainer.

[robobun]

Superseded by #31963, which fixes this bug class in the Rust implementation that ships (this PR patches the .zig reference files, which are no longer compiled). #31963 covers the fetch producer site along with the other producers and the consumer-side release paths. Closing.