jsc: back JsRef::Weak with a real JSC::Weak handle

[robobun]

What

JsRef::Weak was a bare JSValue copy with no GC registration. try_get() could only check for empty/undefined/null, so between the collection that determines a wrapper dead and the sweep that runs its destructor (which is what flips the ref to Finalized), every deferred reader got a dangling value back and called callbacks out of a dead cell's slots.

Four recent use-after-free fixes hit exactly this shape through different holders: fs.watchFile (#36926), UDP socket unref (#36930), Bun.serve graceful stop (#35130, open), and Bun.Terminal PTY EOF (#36962, open). Those are the per-site functional fixes; this PR fixes the primitive so the failure mode cannot be memory-unsafe anywhere, including the holders nobody has fuzzed yet.

Crash signatures of this class on debug/ASAN builds:

ASSERTION FAILED: isMarked(cell)  (JSC::Heap::addToRememberedSet)
segfaults in JSC::Integrity::auditCellFully under Bun__JSValue__call

and on release builds, type confusion where an unrelated closure runs as the callback once the cell is reused.

Fix

JsRef::Weak now holds a passive bun_jsc::Weak (a real JSC::Weak handle) registered against the value's own cell:

• try_get() reads None from the moment GC reaps the referent, before any sweep. A deferred reader skips its event instead of dispatching through a dead wrapper.
• downgrade() registers the weak handle while the Strong still roots the value, so there is no unrooted window. It keeps its no-argument signature: the handle registers against the cell's own WeakSet, no global needed.
• upgrade() of a reaped referent is a no-op instead of resurrecting a dangling JSValue into a Strong.
• is_empty() / is_not_empty() now report liveness, not just "a value was stored".

The C++ Bun::WeakRef constructor never used its VM& argument, so the globalObject parameter is dropped from Bun__WeakRef__new and from Weak::create / Weak::create_passive (three call sites), and the constructor now tolerates non-object values instead of null-dereferencing value.getObject().

The JsRef enum shape and method signatures are unchanged, so the holders (server, sockets, timers, SQL, Terminal, watchers) pick the semantics up without edits. ReadableStream's StreamSlot already used this exact Held-Strong/real-Weak pattern; this generalizes it.

One deliberate carve-out: the js_ref slot on Request/Response is only ever read synchronously while the wrapper is rooted by the caller, and wrapper cells are precise-allocated, so registering a weak handle there costs a 1 KB WeakBlock in the cell's own WeakSet on every construction. The request-clone-leak RSS test caught that regression on every release lane in the first CI run. Those two slots now use RawJsRef, a non-registering sibling type whose synchronous-reader contract is documented on the type; every deferred holder keeps the registered JsRef.

Verification

• bun bd test over the heavy JsRef holders: bun-server.test.ts, terminal.test.ts, fs.watchFile.test.ts, dgram.test.ts, fetch.stream.test.ts, setTimeout.test.js, request.test.ts, body.test.ts. Failures match the clean-main baseline exactly (3 bun-server tests and 3 setTimeout RSS-leak tests fail identically on unmodified main in this environment).
• request-clone-leak.test.ts RSS deltas match the clean-main baseline within noise after the RawJsRef carve-out (measured side by side locally; the first push regressed the release lanes by 3-6x).
• New lifecycle test in bun-server.test.ts: a keep-alive connection keeps a request in flight against a gracefully stopped server while full collections run. It asserts the server answers while the downgraded wrapper is alive, refuses (503/close) once GC collects it, never serves a 200 after the first refusal, and never crashes.

On fail-before provability

The dead-but-unswept wrapper state is not reachable from JS sequencing on a debug build: generated wrapper cells are PreciseAllocations, and Heap::finalize() sweeps those at the end of the same collection cycle that reaps them (sweepInFinalize() -> sweepPreciseAllocations()), before the event loop can dispatch anything. The remaining window is the gap between collector cycle-end and the mutator's finalize handshake, which is where the observed crashes live; it cannot be hit deterministically without instrumentation in src/.

Probe evidence: 20/20 runs of the straggler repro against the unfixed debug build complete as 200s, then one 503, then close, exit 0, no sanitizer output. The new test is therefore a canary for the crash signatures above rather than a deterministic reproduction; the fix removes the window by construction (the weak handle is cleared by the same reap that makes the cell dead, with no code path in between that can observe the difference).

Perf

A weak read is one FFI call into JSC::Weak::get() instead of an inline tag check, and each downgraded holder owns one heap-allocated Bun::WeakRef node, freed on upgrade/finalize/overwrite. fetch() already uses the same mechanism per response (WeakRefType::FetchResponse). Hot per-object paths (Request/Response construction) pay nothing: their slots use the non-registering RawJsRef.

---

no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/bun-server.test.ts

[robobun]

Status: diff ready for review.

The weak-after-close UAF class (fs.watchFile #36926, UDP unref #36930, Bun.serve stop #35130, Terminal EOF #36962) is closed at the primitive: JsRef::Weak is now a real JSC::Weak cleared at reap time, so a deferred reader can no longer observe a dead wrapper. Review follow-ups are in: Request/Response js_ref uses the non-registering RawJsRef (fixes the request-clone-leak RSS regression the first push caused), JsRef::is_dead() closes the reaped-vs-empty gap in the socket create-if-missing path, and FetchTasklet's on_body_received now neither reads nor writes wrapper slots unless the wrapper is verified live.

CI: no failures attributable to this diff across four builds. The persistent red lane is a pre-existing main failure (AsyncLocalStorage-tracking ASAN leak in node_crypto_binding.rs); build 89215 additionally hit an inotify-exhaustion watcher panic in the cron --hot test on one runner. Both are reported to main-break triage separately; everything else passed on retry. Fail-before proof is structurally unavailable for this race on a debug build (analysis and 20-run probe evidence in the PR body), so the included test is a lifecycle canary rather than a deterministic reproduction.

The open per-site PRs #35130 and #36962 remain the functional fixes and do not conflict with this change.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Weak now uses passive handles registered against object cells. JsRef uses handle liveness for weak operations. RawJsRef stores raw JS values for synchronous wrapper state. Call sites use revised APIs, and server dispatch handles reaped wrappers with new test coverage.

Changes

Passive weak-handle lifecycle

Layer / File(s)	Summary
Weak handle API and object-cell registration src/jsc/Weak.rs, src/jsc/bindings/Weak.cpp	Weak creation no longer requires a VM or JSGlobalObject. Non-object values produce empty handles.
JsRef passive storage and liveness src/jsc/JSRef.rs, src/jsc/lib.rs	JsRef::Weak stores Weak<()>. Reads, upgrades, downgrades, emptiness checks, and updates use passive-handle liveness. RawJsRef is added and re-exported.
RawJsRef call-site migration src/runtime/webcore/Request.rs, src/runtime/webcore/Response.rs, src/runtime/socket/socket_body.rs	Request and Response switch to RawJsRef for JS wrapper storage, initialization, cloning, cleanup, and finalization. Socket wrapper lookup treats dead references as finalized.
Weak API call-site migration src/runtime/webcore/Body.rs, src/runtime/webcore/ReadableStream.rs, src/runtime/webcore/fetch/FetchTasklet.rs	Readable stream, body, and fetch code use the revised weak creation and downgrade signatures. Fetch access uses the stream weak reference.
Server wrapper dispatch and GC regression src/runtime/server/mod.rs, test/js/bun/http/bun-server.test.ts	Server dispatch documentation and tests cover reaped wrappers during graceful shutdown and asynchronous GC.

Possibly related PRs

• oven-sh/bun#35130: Both changes cover server lifecycle and wrapper collection after graceful stop.
• oven-sh/bun#36790: Both changes cover Bun.serve wrapper lifecycle and weak-reference finalization.
• oven-sh/bun#36799: Both changes update weak-reference handling in related stream and body lifecycle paths.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly identifies the primary change: backing JsRef::Weak with a real JSC::Weak handle.
Description check	✅ Passed	The description explains the change, rationale, implementation, verification results, performance impact, and known test limitations.

---

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

[github-actions]

Found 4 issues this PR may fix:

1. panic(main thread): Segmentation fault at address 0x8 #22775 - Segfault at 0x8 with a stack of Bun__JSC_onAfterWait → acquireAccessSlow → handleNeedFinalize → Heap::finalize → sweepPreciseAllocations, i.e. the collect→finalize-handshake window this PR closes, in a fetch + http_server process.
2. Segfault at 0x8 in tickImmediateTasks during long Claude Code session (macOS arm64, v1.3.14) #30418 - Segfault at null+8 in tickImmediateTasks → runImmediateTask → TimerObjectInternals.run → Bun__JSTimeout__call, which is a try_get() on a downgraded this_value followed by reading the callback out of that wrapper's slots.
3. Crashed during Claude Code session #24033 - Crash at the garbage tagged address 0x3C00000003 inside JS dispatched from Bun__JSTimeout__call ← TimerObjectInternals.fire, matching the release-build type-confusion signature of reading callback slots off a reaped-but-unswept timer wrapper.
4. Segmentation fault at address 0x8 (After ~2 hours) #21002 - Faults inside the weak-handle machinery itself (Heap::runCurrentPhase → WeakSet::forEachBlock → CellContainer::vm at null+8) in a Bun.serve + Bun.sql + Bun.redis process, all of which downgrade/upgrade JsRef.

Caveat, given this PR's own note that the dead-but-unswept state was not deterministically reproducible: none of these are confirmed — each is a stack-shape match against the mechanism the PR changes, and #21002 is the weakest of the four (a WeakSet-iteration crash could plausibly be blamed on weak handles rather than fixed by them).

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #22775
Fixes #30418
Fixes #24033
Fixes #21002

🤖 Generated with Claude Code

[robobun]

On the four issues matched above (#22775, #30418, #24033, #21002): the mechanism lines up for the first three. #30418 and #24033 are queued timer dispatch calling through a wrapper's callback slots (the exact deferred-reader shape this PR hardens), and #22775 crashes in the collect-to-finalize handshake window that the weak read now covers. #21002 faults inside the WeakSet iteration machinery itself; this PR touches that machinery but there is no reason to believe it fixes it.

None of the four has a deterministic reproduction, so I am not adding auto-close lines; closing on a stack-shape match risks hiding a distinct bug. The honest check is whether these crash signatures disappear from telemetry once a release carries this change.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 src/jsc/JSRef.rs:131-136 — A reaped-but-unswept Weak is now indistinguishable from JsRef::empty(), so socket_body.rs::get_this_value() (lines 1559-1578) falls through its Finalized guard into the to_js() branch and mints a second wrapper over the same native — the exact double-finalize/double-deref its line-1564 comment guards against. Either expose the distinction on the primitive (e.g. JsRef::had_value() checking Weak.r#ref.is_some()) or have get_this_value() return UNDEFINED for any try_get()==None, matching the PR's stated "skip instead of dispatch" intent.

  Extended reasoning...

  What changed and why it matters

  Before this PR, JsRef::Weak held a bare JSValue, so try_get() returned Some(value) right up until the wrapper's destructor flipped the ref to Finalized. After this PR, JsRef::Weak holds a real JSC::Weak, and try_get() starts returning None the moment reapWeakHandles() marks the WeakImpl dead — the PR's own new doc at JSRef.rs:65-69 says exactly this: "before the lazy sweep runs the wrapper's destructor (which is what flips the ref to Finalized)". That introduces a new observable state: the enum variant is still JsRef::Weak (finalize hasn't run), but try_get() is None and is_empty() is true — indistinguishable from JsRef::empty() (never-had-a-wrapper).

  The holder that depends on the distinction

  src/runtime/socket/socket_body.rs:1559-1578 get_this_value():

  1. try_get() → Some → return it
  2. matches!(Finalized) → return UNDEFINED (comment: "Creating a new one here would result in a second finalize (and double-deref) later")
  3. otherwise → self.to_js(global) mints a fresh wrapper cell over the same native ptr, then set_strong()

  Before this PR, branch (3) was reachable only from JsRef::empty(). After this PR, a downgraded socket whose wrapper has been reaped but not yet lazily swept also lands in branch (3): variant is still Weak (so the Finalized check at (2) misses), but weak.get() is None (so (1) misses too).

  Step-by-step to double-deref

  Concrete path via Listener.rs:1549-1566 (the reuse-prev reconnect path — its own comment says "prev.this_value was downgraded to Weak by the previous close's mark_inactive()"):

  1. Socket closes → mark_inactive() (socket_body.rs:1320) downgrades this_value to Weak. Native refcount is +1, owned by the existing wrapper.
  2. JS drops its socket._handle reference; GC runs; reapWeakHandles() marks the wrapper's WeakImpl dead. Lazy sweep hasn't run yet, so this_value is still Weak, not Finalized.
  3. Reconnect calls socket_ref.get_this_value(global). try_get() → None; matches!(Finalized) → false; falls through to to_js().
  4. to_js() (socket_body.rs:449-461) hands the native pointer to a new JS cell without bumping the refcount (comment: "ownership is adopted by the C++ JSCell wrapper"). set_strong(new_wrapper) overwrites this_value.
  5. The old wrapper's lazy sweep runs finalize() (socket_body.rs:3228-3243): this_value.finalize() (drops the Strong on the new wrapper) → deref(). Native refcount +1 → 0 → freed.
  6. The new wrapper (now unrooted) is later collected → its finalize() runs → second deref() on freed memory.

  Two deref()s against one +1: exactly the UAF the line-1564 comment guards against.

  Why the existing guard doesn't catch it

  The Finalized guard at line 1563-1566 was written precisely to prevent this double-finalize. It catches "swept" but not "reaped-but-unswept" — a state that didn't exist as a distinct try_get()==None case before this PR. The PR silently renders that guard incomplete.

  Not a strict regression, but contradicts the PR's claim

  Pre-PR, the same reap→sweep window was also unsafe at this site: branch (1) returned the dead cell and the caller (data_set_cached) wrote through it. So this PR trades one UAF (dispatch-through-dead-cell) for another (deferred double-deref) at this holder. But the PR body says "this PR fixes the primitive so the failure mode cannot be memory-unsafe anywhere, including the holders nobody has fuzzed yet" and "the ~30 holders … pick the semantics up without edits" — this holder's create-if-empty pattern is a direct counterexample. Per REVIEW.md ("Fix the whole class in the same PR"), it belongs in this diff.

  Fix

  The primitive has the information — a reaped Weak has r#ref.is_some() (the WeakImpl handle exists, just reads empty), while JsRef::empty() has r#ref.is_none() — but doesn't expose it. Either:

  • add JsRef::had_value(&self) -> bool (or similar) that distinguishes reaped-Weak from empty-Weak, and make get_this_value() return UNDEFINED when it's true; or
  • simpler: change get_this_value() to return UNDEFINED for any try_get()==None on a non-Finalized ref where a wrapper previously existed, matching the PR's stated "a deferred reader skips its event instead of dispatching through a dead wrapper" intent.

  The Listener reuse-prev path already ref_()s the native and would need to keep that balanced with whatever "skip" behavior is chosen there.

[robobun]

Follow-ups from the review findings, in 5462e59:

• The reaped-but-unswept double-wrapper finding on socket get_this_value() was correct: with the weak read going dead at reap, the create-if-missing branch could no longer distinguish a dead wrapper from a never-created one and would mint a second wrapper over the same native. Added JsRef::is_dead() (registered weak handle reading dead, or Finalized) and switched the guard to it. An audit of the other try_get() readers (Terminal, subprocess, UDP, timers, Listener, server dispatch) found they all skip on None; get_this_value() was the only create-if-missing site.
• The hot-path allocation point was addressed earlier in 003d502: Request/Response js_ref is a non-registering RawJsRef, so construction pays nothing. The redundant js_ref re-set in Response::construct_error is now dropped too. SQL query wrappers keep the registered weak deliberately; their result callbacks are deferred dispatch, which is the case the registration exists for.
• Weak-path debug_asserts tightened to is_object() to match create_passive's precondition.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@test/js/bun/http/bun-server.test.ts`:
- Line 693: Name the GC stress limit used in the bun-server test loop by
introducing a dedicated constant and updating the for-loop in the same test to
use it instead of the raw 300 literal. Keep the loop behavior unchanged, and
anchor the new constant near the existing test logic in bun-server.test.ts so
the stress bound is clear and reusable.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 257ca672-1c2b-4ff1-b52e-8dd3023b4664

📥 Commits

Reviewing files that changed from the base of the PR and between ac3edcd and 5462e59.

📒 Files selected for processing (8)

• src/jsc/JSRef.rs
• src/jsc/Weak.rs
• src/jsc/lib.rs
• src/runtime/server/mod.rs
• src/runtime/socket/socket_body.rs
• src/runtime/webcore/Request.rs
• src/runtime/webcore/Response.rs
• test/js/bun/http/bun-server.test.ts

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/jsc/JSRef.rs:246-255 — The RawJsRef doc says it's for back-pointers "only read synchronously while the wrapper is rooted by the caller", but Response.js_ref has a deferred reader: FetchTasklet::on_body_received (FetchTasklet.rs:837-841) reaches the Response via native_response (a native intrusive +1, not a JS root) and calls get_body_readable_stream() → js_ref() → bare JSValue → stream_get_cached() on the wrapper cell. This is not a regression (pre-PR JsRef::Weak(JSValue) was byte-identical here), but the codebase already flags this exact hazard on the sibling path — BodyAbortListener::on_abort (Response.rs:140-145) bypasses get_body_readable_stream with the comment "its js_ref() path reads a raw JSValue to a wrapper that may be unmarked but not yet swept". Consider either applying that same bypass at FetchTasklet.rs:841 (read Locked.readable directly), or softening the RawJsRef doc to acknowledge Response's known-open deferred-reader window.

  Extended reasoning...

  What the doc contract states vs. what Response does

  The new RawJsRef doc comment (JSRef.rs:246-255) says it is for "wrapper back-pointers that are only read synchronously while the wrapper is rooted by the caller (host-fn receiver, or a value just created on the JS stack)", and that "deferred readers (event-loop callbacks, queued tasks) must use [JsRef]". The PR body justifies the Request/Response carve-out on the same basis: "the js_ref slot on Request/Response is only ever read synchronously while the wrapper is rooted by the caller".

  For Request that holds. For Response it does not: FetchTasklet::on_body_received is a network-driven event-loop callback (i.e., exactly a "deferred reader" in the doc's terms) that reads Response.js_ref through a path where the wrapper is not rooted by the caller.

  The concrete code path

  1. FetchTasklet.rs:837 — on_body_received calls self.current_response_mut().
  2. FetchTasklet.rs:583-587 — get_current_response() returns self.native_response first. native_response is set at line 1898 via Response::ref_(response) — an intrusive native +1 on the Response struct's ref_count. This keeps the native heap allocation alive; it does not root the JS wrapper cell. Only if native_response is None does the code fall back to the real JSC::Weak self.response.
  3. FetchTasklet.rs:841 — response.get_body_readable_stream(&global_this) is called on that native-ref'd Response.
  4. Body.rs:1681-1682 — get_body_readable_stream reads self.js_ref() → RawJsRef::try_get() → the bare JSValue (no liveness check, per RawJsRef's design) → Self::stream_get_cached(js_ref), which dereferences the m_stream WriteBarrier slot on the JSResponse wrapper cell.

  Step-by-step: how the doc's own hazard manifests

  1. fetch() resolves; on_resolve (line 1878-) creates the JSResponse wrapper, stores a real JSC::Weak in self.response, and stores a native +1 in self.native_response (line 1898). response.js_ref is now RawJsRef::Value(js_wrapper).
  2. User JS drops the last reference to the Response object while the body is still streaming (e.g., they fetched but never awaited/consumed the body).
  3. The concurrent collector's mark phase determines the JSResponse wrapper cell is dead. The mutator resumes. Nothing has swept yet.
  4. A body chunk arrives from the network; on_body_received fires on the JS thread.
  5. get_current_response() returns native_response — the native struct is alive (intrusive refcount ≥ 1). Response::finalize() has not run yet (the WeakHandleOwner Bun__FetchResponse_finalize fires during Heap::finalize(), which the mutator hasn't reached yet), so js_ref is still RawJsRef::Value(dead_jsvalue), not Finalized.
  6. get_body_readable_stream reads js_ref() → Some(dead_jsvalue) → stream_get_cached(dead_jsvalue) reads a slot on a dead-but-unswept cell.

  This is precisely the reap-to-sweep window the PR body describes and closes for JsRef holders. The PR's own "On fail-before provability" section explains why it isn't deterministically reproducible on debug builds (PreciseAllocation wrappers are swept in the same cycle by sweepInFinalize()), and why the remaining window is the collector-cycle-end → mutator-finalize handshake — but that window is exactly what the PR set out to close, and it remains open for Response on this one path.

  Why this isn't hypothetical: the codebase already documents it

  BodyAbortListener::on_abort (Response.rs:140-145) is another deferred callback that reaches a Response via a non-GC-rooting handle (ParentRef), and it explicitly bypasses get_body_readable_stream with this comment:

  Not get_body_readable_stream: its js_ref() path reads a raw JSValue to a wrapper that may be unmarked but not yet swept, reaching a NewSource box the source cell's (PreciseAllocation) destructor already freed. Locked.readable is a real JSC::Weak on the stream and reads None exactly when the box is gone.

  So the codebase has already identified this exact hazard on Response's js_ref path from deferred contexts, and works around it by reading Locked.readable (a real JSC::Weak) directly. FetchTasklet::on_body_received at line 841 does not apply the same bypass.

  Why this is a nit, not a blocker

  Behavior is byte-for-byte unchanged from before this PR. Pre-PR, Response.js_ref was JsRef::Weak(JSValue) — a bare JSValue with identical semantics to RawJsRef::Value(JSValue). Nothing regresses; the reap-to-sweep window on this path is pre-existing. What's new is:

  • A doc comment that states an invariant ("only read synchronously while the wrapper is rooted by the caller") that one of the type's two users (Response) violates.
  • A PR-body claim ("fixes the primitive so the failure mode cannot be memory-unsafe anywhere, including the holders nobody has fuzzed yet") that overstates scope for the Response carve-out.

  Merging as-is doesn't make anything worse than main. But since the PR is explicitly about closing this failure mode as a class, it seems worth either (a) applying the BodyAbortListener bypass pattern at FetchTasklet.rs:841 — read Locked.readable directly instead of routing through js_ref() (get_body_readable_stream already falls through to that at Body.rs:1693-1694 when js_ref() is None, so the change is small), or (b) tightening the RawJsRef doc to acknowledge that Response's deferred reader via FetchTasklet.native_response is a known-open window (matching the existing BodyAbortListener comment), so the type's contract is accurate.

[robobun]

Good catch on the FetchTasklet path: on_body_received reaches the Response through native_response (a native intrusive ref, no JS root), so its get_body_readable_stream call was exactly the deferred js_ref read the RawJsRef contract forbids, and the codebase had already documented the hazard at BodyAbortListener::on_abort. cf640f6 applies the same bypass there: read Locked.readable (a real JSC::Weak on the stream) directly, which goes None exactly when the stream is gone.

With that, both RawJsRef users satisfy the documented contract: the remaining get_body_readable_stream callers are host fns or server render paths entered with the wrapper rooted (receiver on the JS stack, or the RequestContext's response handle). Verified with fetch.stream.test.ts (the multiple-part Content-Length cases drive on_body_received chunk delivery) and body.test.ts.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 847-851: Update the response-body handling around
`ByteStream::on_data` to keep both the readable stream and `bytes` strongly
rooted while the callback may resolve JavaScript promises. Create the strong
root before invoking `on_data`, or re-acquire the weak stream and `ByteStream`
afterward before the `has_more` path calls `drop_backpressure_if_unobserved`;
preserve existing behavior otherwise.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e787718c-4bfe-42e3-9e43-7add63315166

📥 Commits

Reviewing files that changed from the base of the PR and between 5462e59 and cf640f6.

📒 Files selected for processing (1)

• src/runtime/webcore/fetch/FetchTasklet.rs

[robobun]

The detach half of that path had the same exposure: the final-chunk branch of on_body_received wrote ZERO into the wrapper's cached-stream slot through the raw js_ref (a write barrier on an unmarked cell in the reap-to-finalize window). 003d502 and cf640f6 covered the read side; 8349555 now gates the slot write on the tasklet's own weak wrapper handle (self.response) reading alive, and for a dead wrapper only releases the native Locked.readable ref, the finalizer clears the slot. With that, on_body_received neither reads nor writes wrapper slots unless the wrapper is verified live, matching the on_abort pattern, and the RawJsRef contract holds for both of its users.

[claude]

All prior findings from earlier passes have been addressed (cf640f6, 8349555, 38a9d7c, 86a6f62), and this pass found nothing new. Deferring to a human because this rewrites a core GC-safety primitive and introduces the RawJsRef carve-out — an unsafe-by-documented-contract type whose one contract violation was already found and fixed mid-review — which is a design trade-off a maintainer should sign off on.

What was reviewed:

• JsRef state transitions (downgrade/upgrade/set_weak/update) for handle leaks and unrooted windows — the Strong is held until after Weak::create_passive registers.
• Bun__WeakRef__new FFI signature change and the non-object guard in the C++ constructor.
• FetchTasklet::on_body_received after 8349555 — both the read and the final-chunk slot write are now gated on wrapper liveness.
• The is_dead() addition and its one caller in socket_body.rs (correctly distinguishes reaped-but-registered from never-set).

Extended reasoning...

Overview

The PR replaces JsRef::Weak(JSValue) — a bare, unregistered value copy — with JsRef::Weak(bun_jsc::Weak<()>), a real JSC::Weak handle registered against the referent's cell. try_get() now reads None at reap time rather than at sweep time, closing the reap-to-finalize window in which deferred callbacks could dispatch through a dead wrapper. It also drops the unused JSGlobalObject parameter from Bun__WeakRef__new/Weak::create[_passive], hardens the C++ WeakRef constructor against non-object values, adds Weak::is_registered() and JsRef::is_dead(), and introduces RawJsRef — a non-registering sibling used for Request/Response back-pointers where a per-object WeakBlock regressed the request-clone-leak RSS test. Twelve files across src/jsc/, src/runtime/webcore/, src/runtime/server/, and src/runtime/socket/ are touched, plus a 140-line lifecycle canary test in bun-server.test.ts.

Security risks

None in the classic sense (no auth/crypto/parsing of untrusted input). The risk surface is memory safety: a mistake in the new primitive or in the RawJsRef carve-out audit is a use-after-free reachable from JS. The RawJsRef contract is enforced only by documentation, and one deferred reader (FetchTasklet::on_body_received) already violated it and was fixed mid-review — that is exactly the kind of gap a human should confirm is now closed for the remaining js_ref() callers in Body.rs.

Level of scrutiny

High. This is a load-bearing GC primitive used by every long-lived native holder (server, sockets, timers, SQL clients, watchers, terminal). The change is conceptually simple, but the blast radius is the whole runtime, the failure mode is UAF/type-confusion on release builds, and the PR itself introduces a two-tier design (JsRef vs RawJsRef) whose safety depends on a per-caller audit rather than the type system.

Other factors

Every earlier review finding (the on_body_received read and write halves, the no-op PhantomData const, the stale try_swap comment) was addressed with a targeted commit and is verified fixed at tip. The new test is a scheduling-dependent canary (three subprocess runs, 120s budget, requires at least one child to reach wrapper death) rather than a deterministic reproduction — the PR body explains why deterministic reproduction is not possible without instrumenting src/, but a maintainer should decide whether that test's cost/flake profile is acceptable. The FFI signature change to Bun__WeakRef__new and the removal of the VM& argument look correct (the constructor never used it), but ABI changes to exported symbols warrant a human glance.

[robobun]

Closing: superseded by #39334 (merged as 07d38c1), which fixes the same reap-to-sweep window at the primitive by having JsRef::try_get() and upgrade() consult HeapCell::isPendingDestruction() via JSValue::is_live_cell(). That gives the same reap-time answer a registered JSC::Weak does with no per-holder allocation, so it also makes the RawJsRef carve-out here unnecessary (Request/Response keep a plain JsRef::Weak and get the liveness check for free, which covers the FetchTasklet::on_body_received path this review surfaced).

One finding from this review still applies to main's design: a reaped-but-unswept Weak reads as try_get() == None while is_finalized() is still false, so the create-if-missing path in socket get_this_value() can mint a second wrapper over the same native and double-run its finalizer. Opening a small follow-up with that guard plus the Bun.serve lifecycle canary and the dead PhantomData const from this branch.