node:http: release NodeHTTPResponse refs through its pointer instead of freeing under &self

[robobun]

Problem

• Nothing is observable today; this is latent. NodeHTTPResponse released its refcount through a &self method, and on the last release that method freed the allocation through a pointer taken from &self.
• A reference argument stays live and protected until the call returns, so freeing what it points to is undefined behaviour under both Stacked Borrows and Tree Borrows (what bun run rust:miri checks), whether or not anything reads it afterwards. blob: delete Blob::deinit, which freed the allocation through &mut self #37672 has a standalone reduction of this shape.
• The pointer came from as_ctx_ptr(), whose own doc says it has shared provenance and exists to fill C-shaped ctx slots, so it is not a pointer the allocation can be freed through.
• Same shape as Blob::deinit (blob: delete Blob::deinit, which freed the allocation through &mut self #37672) and ReadBytesHandler::on_read_bytes (blob: hand the read handler's pointer to on_read_bytes instead of &mut self #37681); a tree-wide grep finds this as the only remaining site.

Fix

• The type now uses the shared CellRefCounted derive, like CronJob and PostgresSQLConnection, so release is deref(*mut Self) and deinit takes the raw pointer. Teardown runs inside a scoped borrow that ends before the free.
• Every release goes through a pointer. Sites that already hold one use it, keep-alive ref/deref brackets become a ScopedRef guard, and sites with only &self call Self::deref(self.as_ctx_ptr()), so that shortcut is visible at each site instead of hidden inside a safe deref(&self).
• Property to check: refcount arithmetic is unchanged, each ref_() keeps the same single release on the same paths. The one ordering change is the auto-flush release, now made when the trampoline's guard drops, right after on_auto_flush returns.
• Verification: no behavioural reproducer exists, the bug is the shape of the free. The source lint from blob: hand the read handler's pointer to on_read_bytes instead of &mut self #37681 now counts self.as_ctx_ptr() as a spelling of the receiver: on main it reports exactly this one line, with the fix nothing. The node:http suites listed in the original pass on the debug ASAN build (one failure is pre-existing, dns: add the loopback family AI_ADDRCONFIG filters out of localhost lookups #37442).

Background

• Intrusive refcount: the count is a field of the heap object and whoever drops it to zero frees the object. #[derive(bun_ptr::CellRefCounted)] generates ref_(), unsafe fn deref(*mut Self) and the bridge ScopedRef and finalize_js_box need, and calls the named destroy function at zero.
• Classes declared in .classes.ts are a JS wrapper cell holding a raw pointer (m_ctx) to the Rust object; the wrapper owns one ref and gives it up in finalize when GC collects it. value.as_::<T>() returns that raw pointer, as_class_ref a &T to the same object.
• as_ctx_ptr() is a blanket helper returning self's address as *mut Self, meant for handing the object to C callbacks that take a void* ctx.
• bun_ptr::ScopedRef is a guard holding one ref and releasing it on drop; new takes a fresh ref, adopt takes over a ref someone else already took.
• Under Rust's aliasing models (Stacked Borrows and Tree Borrows, both run by Miri) a &self argument is protected for the whole call, so a free is only allowed from a frame that holds the object as a raw pointer.

Original description

Problem

NodeHTTPResponse (src/runtime/server/NodeHTTPResponse.rs) hand-rolled its intrusive refcount as deref(&self), which on zero called deinit(&self), which ended in

unsafe { drop(bun_core::heap::take(self.as_ctx_ptr())) };

That frees the allocation through a pointer derived from &self, while &self is still a live argument of both the deinit and the deref frame. Two things are wrong with it, independent of whether anything reads self afterwards (nothing does today, so this is latent):

• as_ctx_ptr() is bun_ptr::AsCtxPtr's "address of self as *mut" helper; its own doc says the result has shared provenance and exists to fill C-shaped ctx slots. Deallocating through it is not something a &self-derived pointer can do.
• A reference argument is protected for the duration of the call, and deallocating protected memory is rejected by both Stacked Borrows and Tree Borrows (the model bun run rust:miri uses). blob: delete Blob::deinit, which freed the allocation through &mut self #37672 has a standalone reduction of exactly this shape.

The comment on the hand-written AnyRefCounted impl already described converting the callers to a pointer-taking deref as a separate sweep; this is that sweep. The same shape in Blob::deinit is #37672, and ReadBytesHandler::on_read_bytes was #37681. The tree-wide grep for a reclaim through as_ctx_ptr() finds only this site.

Fix

• The struct becomes #[derive(bun_ptr::CellRefCounted)] with #[ref_count(destroy = Self::deinit)], the same arrangement as CronJob, NativeZlib, PostgresSQLConnection and JSMySQLConnection. The derive supplies ref_(), unsafe fn deref(this: *mut Self) and the AnyRefCounted bridge, so the hand-written bridge and the ref_/deref pair are deleted. deinit now takes this: *mut Self: the teardown runs through a scoped shared borrow, and heap::take(this) runs after that borrow ends, through the pointer the count was released on.
• Every release site now goes through a pointer rather than a &self method call:
  • the synchronous dispatch tail in on_node_http_request* (src/runtime/server/mod.rs) releases the server-handler ref through the *mut NodeHTTPResponse out-param it already holds, and Bun__NodeHTTPRequest__onResolve / onReject through the wrapper's m_ctx pointer (as_::<NodeHTTPResponse>() instead of as_class_ref);
  • on_auto_flush_trampoline adopts the task's ref as a ScopedRef on the ctx pointer the deferred-task queue hands it, so the release happens after on_auto_flush(&self) has returned; on_auto_flush no longer releases anything itself;
  • the ref_()/deref() keep-alive brackets in cork, write_head_and_end and on_drain_corked become a ScopedRef guard (the last of these had three exits, each with its own deref());
  • the sites that only have &self (mark_request_as_done, handle_abort_or_timeout, on_data_or_aborted, unregister_auto_flush) call Self::deref(self.as_ctx_ptr()) explicitly, as PostgresSQLConnection and websocket_client do. Those methods are reached from &self host functions and uws callbacks, so they have no better pointer to offer; the change here is that the frame that frees is deref/deinit holding the raw pointer, not a &self method, and the shortcut is visible at each site instead of hidden inside a safe deref(&self).
• finalize uses finalize_js_box for its pre-release work (clearing armed_this_value), as the other derive users do.

Refcount arithmetic is unchanged at every site: each ref_() still has the same single release on the same paths, and the order of the teardown steps in deinit is the same. The only ordering change is the auto-flush release, which now happens when the trampoline's guard drops, immediately after on_auto_flush returns, instead of as its last statement.

Test

test/internal/source-lints/self-receiver-reclaim.test.ts (the lint from #37681) now treats self.as_ctx_ptr() and as_ctx_ptr(self) as spellings of the receiver, with positive and negative examples (self.field.as_ctx_ptr(), handing the pointer to on_data, Self::deref(self.as_ctx_ptr()) and a ScopedRef over it stay allowed). With this test change and main's src/, the lint reports exactly

src/runtime/server/NodeHTTPResponse.rs:2643: heap::take(self.as_ctx_ptr()

and with the fix the tree is at zero. Per-type as_ptr(&self) -> *mut Self helpers (for example FileResponseStream's) are deliberately not added: heap::take(x.as_ptr()) is also how smart-pointer newtypes free their pointee, and none of them reclaims the receiver today.

There is no behavioural reproducer; the bug is the shape of the free, not something observable before the allocator happens to reuse the memory.

Verification

On the debug (ASAN) build: test/js/node/http/{node-http,node-http-uaf,node-http-server-abort-events,node-http-nested-cork,node-http-pinned-write,node-http-backpressure,node-http-backpressure-max,node-http-server-timeouts,node-http-ondata-reregister-leak,node-http-req-socket-pause,node-http-server-socket-end-drain,node-http-connect,node-http-with-ws,node-http-transfer-encoding,node-http-res-settimeout-unref,node-http.compress.leak,node-http-parser,node-http-maxHeaderSize,node-http-syscall-fault,client-timeout-error,early-hints-crlf-injection,numeric-header}.test.ts, test/js/bun/http/{node-http-halfclose-midupload,request-smuggling}.test.ts, and 44 of the ported Node suites under test/js/node/test/parallel (test-http-abort*, test-http-flush*, test-http-pipeline*, test-http-response-{close,cork,readable}, test-http-server-close*, test-http-server-request-timeout*, test-http-upgrade*, test-http-set-timeout*, and a few others) all pass. These cover the abort, timeout, flushHeaders (auto-flush), onwritable drain, sync and async handler, upgrade, CONNECT and pipelining release paths. The one failure seen, request via http proxy, issue#4295 in node-http.test.ts, fails identically on the unmodified release build in this container (listen(0, "localhost") binds ::1 while the client connects to 127.0.0.1; the class #37442 describes) and is unrelated. Several of the node:http child-process tests need more than the default 5 s timeout on this debug build because importing node:http alone takes about 2.7 s here; they pass with --timeout 60000.

bun test test/internal/source-lints/ passes; cargo clippy -p bun_runtime --no-deps and cargo fmt -p bun_runtime -- --check are clean.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 2 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: c806d283-1af1-47af-8845-c399e5e02a9e

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and b434675.

📒 Files selected for processing (3)

• src/runtime/server/NodeHTTPResponse.rs
• src/runtime/server/mod.rs
• test/internal/source-lints/self-receiver-reclaim.test.ts

---

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

[robobun]

Status

Reproduced structurally: with this PR's test change applied to main, bun test test/internal/source-lints/self-receiver-reclaim.test.ts reports src/runtime/server/NodeHTTPResponse.rs:2643: heap::take(self.as_ctx_ptr(); with the src/ change it reports nothing. The node:http suites listed in the description pass on the debug (ASAN) build.

Fix: this PR (#37875). Self-review in progress; will update here if it turns anything up.

[claude]

I reviewed this and didn't find any bugs — the refcount arithmetic looks balanced at every converted site and the patterns match PostgresSQLConnection / websocket_client. Because it reworks the intrusive-refcount release path across ~10 sites in node:http's server response lifecycle, a human pass on the ref/deref pairing would still be worthwhile.

What was reviewed:

• ScopedRef::new vs ::adopt semantics against each converted site (cork, write_head_and_end, on_drain_corked bump-and-release; on_auto_flush_trampoline adopts the task's existing ref) — counts match the removed ref_()/deref() pairs.
• _keep_alive guards are named bindings (not let _ =), so they drop at end of scope after the last use of self/this.
• finalize_js_box vs the old set + finalize_js_box_noop — same ordering (before-closure runs, then one deref).
• deinit teardown steps and order unchanged; the shared borrow is scoped to end before heap::take(this).

Extended reasoning...

Overview

This PR converts NodeHTTPResponse from a hand-rolled ref_()/deref(&self) intrusive refcount to #[derive(bun_ptr::CellRefCounted)] with #[ref_count(destroy = Self::deinit)], changing deinit from &self to unsafe fn(this: *mut Self) so the allocation is freed through the raw pointer rather than under a live protected &self. Roughly ten release sites are updated: three keep-alive brackets become ScopedRef guards, the auto-flush trampoline adopts the task ref via ScopedRef::adopt, the promise resolve/reject callbacks and the sync dispatch tail in mod.rs release through the m_ctx / out-param pointer, and the remaining &self sites call Self::deref(self.as_ctx_ptr()) explicitly (the same pattern PostgresSQLConnection and websocket_client use). The source-lint test is extended to recognise self.as_ctx_ptr() as a receiver spelling.

Security risks

None in the traditional sense. The risk here is memory safety: an unbalanced ref at any converted site is a UAF or leak in the node:http server request path. I traced each site and the arithmetic matches the old code (each removed deref() has exactly one replacement release on the same paths; on_drain_corked's three-exit manual deref becomes one guard that drops on every exit). The only ordering change — the auto-flush ref now releases when the trampoline's guard drops rather than as on_auto_flush's last statement — is benign (the guard drops immediately after the call returns).

Level of scrutiny

High. This is native memory-safety code in a hot production path (node:http server response lifecycle), squarely in REVIEW.md's most-blocked category. The change is a mechanical sweep following an established derive pattern, and the PR description documents extensive test coverage across abort/timeout/drain/upgrade/pipelining paths on the ASAN build, but the consequence of a miscounted ref here is severe enough that a maintainer familiar with the NodeHTTPResponse lifecycle should confirm the pairing.

Other factors

• The pattern is well-established: CellRefCounted + unsafe fn deinit(this: *mut Self) + Self::deref(self.as_ctx_ptr()) at &self sites appears identically in PostgresSQLConnection.rs and http_jsc/websocket_client.rs.
• I verified ScopedRef::new bumps on construction and derefs on drop, while ::adopt only derefs — the trampoline correctly uses adopt (the ref was taken in register_auto_flush), and the three keep-alive brackets correctly use new.
• finalize now uses finalize_js_box(self, |this| ...) in place of manual set + finalize_js_box_noop; checked src/ptr/ref_count.rs:174 — same net effect (run closure on &T, then one rc_deref).
• The _keep_alive guards are named bindings, so they live to end of scope (a bare let _ = would drop immediately and defeat the purpose).
• No behavioural test is added because the fix is for a latent Tree-Borrows/provenance violation, not an observable bug; the source-lint extension does catch the old shape on unmodified main.

[robobun]

For whoever does the ref/deref pass, here is every count on a NodeHTTPResponse and where it is released after this change (line numbers are for b434675; the acquire side of each pair is untouched by this PR, only the release side moved):

Count	Taken	Released
uws response / IS_REQUEST_PENDING (1 of the initial 3, createForJS L2759)	at creation	mark_request_as_done L770 (unconditional, was the unconditional self.deref())
JS wrapper (1 of 3)	at creation	finalize -> finalize_js_box L2645, one rc_deref on the Box::into_raw pointer, same as finalize_js_box_noop before
server handler (1 of 3)	at creation	exactly one of: sync dispatch tail, mod.rs L1525 (!is_async); onResolve L1562 / onReject L1616 when they find the promise slot still set; or mark_request_as_done L768 when it empties the slot first (had_async_promise). The slot is the token, as before
abort/timeout keep-alive, handle_abort_or_timeout L1321	self.ref_() (unchanged)	L1367, the single tail exit; the only return in the function is above the ref_()
last-chunk keep-alive, on_data_or_aborted L1729 (if last)	self.ref_() (unchanged)	L1781, inside the same if last; no early returns in between
auto-flush task, register_auto_flush L2425	self.ref_() (unchanged)	either the trampoline's ScopedRef::adopt L342 when the task runs (it returns false, so it runs at most once), or unregister_auto_flush L2451, which removes the task from the queue before releasing; both are gated on registered
cork L2612, write_head_and_end L1140, on_drain_corked L1878	ScopedRef::new (+1), replacing the ref_() at the same point	guard drop at function exit, replacing one tail deref() in the first two and the three per-exit deref()s in on_drain_corked

body_read_ref and poll_ref are event-loop keep-alives, not counts on this object, and are untouched. Nothing else in the tree calls ref_()/deref() on this type (the mod.rs site above was the only external caller).

[robobun]

^{Updated 2:05 PM PT - Aug 12th, 2026}

❌ @robobun, your commit b434675 has some failures in Build #93358 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 37875

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

bun-37875 --bun