Repository navigation
Conversation
get_or_put_route_bundle takes an intrusive ref when it creates a RouteBundle, but the raw html_bundle pointer has no Drop and DevServer Drop never released it. One Route per HTML route per dev-server leaked. Zig RouteBundle.deinit:112 does the deref; the Rust port missed it.
|
Updated 1:48 PM PT - Jun 12th, 2026
❌ @alii, your commit 6728c23 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32213That installs a local version of the PR into your bun-32213 --bun |
WalkthroughThis PR refactors how HTML cached bundle resources are released during ChangesHTML Bundle Resource Deinitialization
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/bake/DevServer.rs`:
- Around line 1250-1253: Clear HTMLBundleRoute.dev_server_id before releasing
the intrusive reference: in the Drop/teardown path where you call unsafe {
bun_ptr::RefCount::<HTMLBundleRoute>::deref(html.html_bundle) } (the same
location referenced by get_or_put_route_bundle), set
html.html_bundle.dev_server_id = None (or the equivalent sentinel) immediately
prior to the deref so any external cache that calls get_or_put_route_bundle
cannot observe a stale bundle index; ensure the mutation happens on the
HTMLBundleRoute instance owned by html before the deref and mirror the same
change at the other referenced tear-down site (lines ~5383-5386).
🪄 Autofix (Beta)
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: e6f1a8a8-60dd-4cf6-8a0c-e4a40ed808aa
📒 Files selected for processing (2)
src/runtime/bake/DevServer.rssrc/runtime/bake/dev_server/route_bundle.rs
| // SAFETY: paired with the `ref_` taken in | ||
| // `get_or_put_route_bundle` when this bundle was created; the | ||
| // raw `html_bundle` field has no Drop, so release it here. | ||
| unsafe { bun_ptr::RefCount::<HTMLBundleRoute>::deref(html.html_bundle) }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear HTMLBundleRoute.dev_server_id before dropping the intrusive ref.
get_or_put_route_bundle() trusts HTMLBundleRoute.dev_server_id before it touches self.route_bundles, but Drop only decrements the intrusive ref. If the HTMLBundleRoute survives this DevServer, that external cache can still point at a stale bundle index. Reset the back-reference before deref() so teardown leaves the surviving route object in a reusable state.
Suggested fix
if let route_bundle::Data::Html(html) = &mut rb.data {
if let Some(cached) = html.cached_response.take() {
// SAFETY: stored ref from `init_from_any_blob`; no live borrow.
unsafe { StaticRoute::deref_(cached.as_ptr()) };
}
+ // SAFETY: `html_bundle` is still alive under this route bundle's intrusive ref.
+ unsafe { (*html.html_bundle).dev_server_id.set(None) };
// SAFETY: paired with the `ref_` taken in
// `get_or_put_route_bundle` when this bundle was created; the
// raw `html_bundle` field has no Drop, so release it here.
unsafe { bun_ptr::RefCount::<HTMLBundleRoute>::deref(html.html_bundle) };
}As per coding guidelines, "Store state on the object whose lifetime matches it" and "Update every lifecycle method (reset/init/drop/clone) when adding mutable state."
Also applies to: 5383-5386
🤖 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/runtime/bake/DevServer.rs` around lines 1250 - 1253, Clear
HTMLBundleRoute.dev_server_id before releasing the intrusive reference: in the
Drop/teardown path where you call unsafe {
bun_ptr::RefCount::<HTMLBundleRoute>::deref(html.html_bundle) } (the same
location referenced by get_or_put_route_bundle), set
html.html_bundle.dev_server_id = None (or the equivalent sentinel) immediately
prior to the deref so any external cache that calls get_or_put_route_bundle
cannot observe a stale bundle index; ensure the mutation happens on the
HTMLBundleRoute instance owned by html before the deref and mirror the same
change at the other referenced tear-down site (lines ~5383-5386).
Source: Coding guidelines
|
Folded into #32215. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Zig `RouteBundle.deinit` equivalent, split across two mechanisms: | ||
| // - Drop: `Framework` StrongOptional fields (= .deinit()) and | ||
| // `Html.bundled_html_text` Box<[u8]> (= allocator.free()). | ||
| // - `DevServer`'s `Drop` (explicit): `client_bundle`, `Html.cached_response` | ||
| // (BackRef, no Drop) and `Html.html_bundle` (raw ptr, no Drop) each hold | ||
| // an intrusive ref that is deref'd there. |
There was a problem hiding this comment.
🔴 This trailer now correctly documents that cached_response is (BackRef, no Drop) and must be explicitly deref'd — but two sites in DevServer.rs still rely on the old Arc-drop assumption and leak the StaticRoute ref on every hot reload. Site A (DevServer.rs:4578-4582, had_adjusted_edges path): if let Some(blob) = html.cached_response.take() { let _ = blob; } — let _ = blob on a Copy BackRef is a no-op, and the inline comment is the verbatim stale text this PR just deleted. Site B (DevServer.rs:4226-4231, HTML chunk re-emit): .cached_response.take().is_some() discards the BackRef, then invalidate_client_bundle sees None and skips its own deref branch. Both should call unsafe { StaticRoute::deref_(cached.as_ptr()) } like DevServer.rs:1248 / route_bundle.rs:113 — same bug class as the html_bundle leak this PR fixes.
Extended reasoning...
What the bug is
This PR rewrites the route_bundle.rs trailer comment to assert that Html.cached_response is a BackRef with no Drop, holding an intrusive ref that must be released explicitly in DevServer::Drop. That assertion is correct: BackRef<T> is Copy (src/ptr/lib.rs:179) and Copy types cannot impl Drop, so dropping a BackRef<StaticRoute> releases nothing. But two existing call sites in DevServer.rs were written under the old "Arc<StaticRoute> drop = .deref()" assumption that this PR just deleted, and they leak one StaticRoute per HTML route per hot-reload.
The two leak sites
Site A — DevServer.rs:4578-4582 (had_adjusted_edges hot-reload path):
route_bundle::Data::Html(html) => {
if let Some(blob) = html.cached_response.take() {
// Arc<StaticRoute> drop = .deref()
let _ = blob;
}
}blob is BackRef<StaticRoute> (Copy); let _ = blob; is a no-op. Note the inline comment is verbatim the stale text this PR removes from route_bundle.rs:168 — a smoking gun that this site was written under the old Arc assumption and was missed in the sweep.
Site B — DevServer.rs:4226-4235 (HTML chunk re-emit):
if route_bundle.data.html_mut().cached_response.take().is_some() {
route_bundle.invalidate_client_bundle(unsafe { &mut (*dev_ptr).source_maps });
}.take().is_some() empties the slot and discards the BackRef. invalidate_client_bundle then re-checks html.cached_response.take() (route_bundle.rs:111) — now None — and skips its own StaticRoute::deref_ branch. The condition should be .is_some() without .take(), letting invalidate_client_bundle do the take+deref, or it should deref the taken value explicitly.
Why existing code doesn't prevent it
The slot is ref-owning. StaticRoute::init_from_any_blob "returns a fresh ref_count=1 box" (DevServer.rs:1825), and DevServer.rs:2846 stores that pointer directly into cached_response via BackRef::from(NonNull) — the +1 from init is the slot's ref. The two correct release sites confirm the contract: DevServer::Drop (line 1248) and invalidate_client_bundle (route_bundle.rs:113) both call unsafe { StaticRoute::deref_(cached.as_ptr()) }. Sites A and B bypass both: they .take() the value out of the slot (so Drop/invalidate_client_bundle find None) and then drop the Copy handle without calling deref_.
Step-by-step proof (Site A)
- User loads an HTML route.
on_html_requestcallsinit_from_any_blob→route_ptrwith refcount=1, stored intohtml.cached_response = Some(BackRef(route_ptr))(DevServer.rs:2835-2848). - User edits a file; hot-reload runs with
had_adjusted_edges = true. - DevServer.rs:4579 executes
html.cached_response.take()→ returnsSome(BackRef(route_ptr)), slot nowNone. let _ = blob;drops aCopyvalue — no destructor runs, refcount still 1.- Next request repopulates
cached_responsewith a newStaticRoute. The old one is unreachable with refcount=1 → leaked. - Repeat per hot-reload.
DevServer::Droponly derefs whatever is currently in the slot, not the N previously-leaked routes.
Site B is identical except step 3 happens at DevServer.rs:4226-4231 and step 4 is invalidate_client_bundle finding cached_response == None at route_bundle.rs:111.
Impact and fix
One StaticRoute (boxed HTML payload + headers) leaked per HTML route per hot-reload — exactly the same bug class (missed intrusive deref in the Zig→Rust port) as the html_bundle leak this PR fixes. Fix both sites with the same pattern already used at DevServer.rs:1248 and route_bundle.rs:113:
// Site A
if let Some(blob) = html.cached_response.take() {
// SAFETY: stored ref from `init_from_any_blob`; no live borrow.
unsafe { StaticRoute::deref_(blob.as_ptr()) };
}
// Site B — drop the .take(), let invalidate_client_bundle handle it
if route_bundle.data.html().cached_response.is_some() {
route_bundle.invalidate_client_bundle(unsafe { &mut (*dev_ptr).source_maps });
}These leaks pre-date this PR, but the PR is explicitly an intrusive-ref-leak sweep on RouteBundle, rewrites the doc comment to assert the exact invariant these sites violate, and leaves Site A's inline comment as the literal stale text it just removed elsewhere — so they belong in this PR.
| // SAFETY: paired with the `ref_` taken in | ||
| // `get_or_put_route_bundle` when this bundle was created; the | ||
| // raw `html_bundle` field has no Drop, so release it here. | ||
| unsafe { bun_ptr::RefCount::<HTMLBundleRoute>::deref(html.html_bundle) }; |
There was a problem hiding this comment.
🟡 nit: per CLAUDE.md ("Landing PRs" → tests), leak fixes are expected to ship with a regression test in the same PR — either an ASan repro that fails on the unfixed build or a refcount-baseline check. HTMLBundleRoute already has #[ref_count(debug_name = "HTMLBundleRoute")], so a test that constructs a DevServer with HTML routes, drops it, and asserts the tracked count returns to baseline should be feasible. Maintainer call on whether the harness cost is worth it for a one-line Drop fix, but the convention is explicit.
Extended reasoning...
What this is
This is a process/convention finding, not a code defect. The deref added in DevServer::Drop is correct and properly paired with the ref_ taken in get_or_put_route_bundle at DevServer.rs:5386. The issue is that the PR ships only the two .rs source files and no accompanying test.
Why it applies
CLAUDE.md:259 states verbatim:
Every behavioral change ships an automated test in the same PR... UAF/leak fixes need a repro that consistently fails under ASan on the unfixed build, or a leak regression test (Bun.gc(true) + heapStats objectTypeCounts filtered to the exact leaked type; RSS thresholds need ~2x headroom).
The section header at CLAUDE.md:247-249 notes "everything here has blocked merges". This PR is squarely in the named category — it fixes an intrusive-refcount leak (one HTMLBundleRoute per HTML route per dev-server lifetime, as the PR description itself states) — and the changed-files list contains only src/runtime/bake/DevServer.rs and src/runtime/bake/dev_server/route_bundle.rs.
Step-by-step: why a test is feasible here
HTMLBundleRouteis declared with#[ref_count(debug_name = "HTMLBundleRoute")](HTMLBundle.rs:149), which registers it with the intrusive-refcount tracker by name.- A test can record the baseline live count for
"HTMLBundleRoute", construct a DevServer with N HTML routes (which callsget_or_put_route_bundle→ref_N times), then drop the DevServer. - On the unfixed build, the tracked count after drop would sit at
baseline + NbecauseDevServer::Dropnever releasedhtml.html_bundle. On the fixed build it returns tobaseline. - Alternatively, an ASan run of the same construct/drop sequence would report the leaked allocations on the unfixed build.
Note that HTMLBundleRoute is a native intrusive-refcounted type rather than a JSC heap object, so the literal heapStats objectTypeCounts path may not apply directly — the debug_name refcount tracker or the ASan-repro option from the same CLAUDE.md sentence is the more applicable mechanism.
Why this might reasonably be waived
One verifier objected that this is generic "missing test" review commentary rather than a code bug, and that standing up a DevServer-with-HTML-routes harness from JS test code is a significant undertaking relative to a one-line Drop fix. That's a fair point — the fix is obviously correct by inspection (the ref_/deref pair is now symmetric, and the updated comment at route_bundle.rs:167-172 documents the split-ownership model). Whether the harness cost is justified is a maintainer judgment call.
Suggested action
Either add a small regression test asserting the HTMLBundleRoute refcount returns to baseline after constructing+dropping a DevServer with HTML routes (or an ASan repro), or note in the PR why the test is being skipped for this change. Filed as nit since the fix itself is correct and the convention is process-level.
| // Zig `RouteBundle.deinit` equivalent, split across two mechanisms: | ||
| // - Drop: `Framework` StrongOptional fields (= .deinit()) and | ||
| // `Html.bundled_html_text` Box<[u8]> (= allocator.free()). | ||
| // - `DevServer`'s `Drop` (explicit): `client_bundle`, `Html.cached_response` | ||
| // (BackRef, no Drop) and `Html.html_bundle` (raw ptr, no Drop) each hold | ||
| // an intrusive ref that is deref'd there. |
There was a problem hiding this comment.
🟡 While you're here updating the html_bundle lifecycle docs: the field comment at route_bundle.rs:39-41 still says HTMLBundleRoute "does not yet impl bun_ptr::RefCounted (gated server-side)" with a TODO to switch to RefPtr once it does. That's stale — Route has #[derive(bun_ptr::RefCounted)] (HTMLBundle.rs:148, no cfg gate), and the RefCount::<HTMLBundleRoute>::ref_/deref calls at DevServer.rs:5386/:1253 wouldn't compile otherwise. Pre-existing, but worth fixing alongside the cleanup-mechanism comment this PR rewrites 130 lines down for the same field.
Extended reasoning...
What's stale
route_bundle.rs:39-41 documents the Html.html_bundle field as:
Stored as raw ptr because
HTMLBundleRoutedoes not yet implbun_ptr::RefCounted(gated server-side).
// TODO: switch to bun_ptr::RefPtr once the RefCounted impl is real.
Both the stated reason and the TODO predicate are false today.
Proof that HTMLBundleRoute: RefCounted holds
Step through the types:
HTMLBundleRouteisRoute.src/runtime/server/HTMLBundle.rs:137:pub(crate) type HTMLBundleRoute = Route;.RoutederivesRefCounted.src/runtime/server/HTMLBundle.rs:148-149:#[derive(bun_ptr::RefCounted)] #[ref_count(debug_name = "HTMLBundleRoute")] pub struct Route { ... }. There is no#[cfg(...)]on the derive — it is not "gated server-side".RefCount<T>requiresT: RefCounted.src/ptr/ref_count.rs:248:pub struct RefCount<T: RefCounted>;:258:impl<T: RefCounted> RefCount<T>.- This PR (and the existing code) call
RefCount::<HTMLBundleRoute>methods. Pre-existingDevServer.rs:5386callsRefCount::<HTMLBundleRoute>::ref_(html); this PR addsDevServer.rs:1253callingRefCount::<HTMLBundleRoute>::deref(html.html_bundle). By (3), neither would type-check unlessHTMLBundleRoute: RefCounted.
So the comment's claim — that the impl "does not yet" exist / is gated — is provably contradicted by code that compiles in the same crate.
Why it's in scope for this PR
The staleness pre-dates this PR (the ref_ at :5386 already required the trait). But this PR is specifically a doc-correctness pass for html_bundle's lifecycle: it rewrites the cleanup-mechanism comment block at route_bundle.rs:167-172 to describe where html_bundle's ref is released, and updates the pairing comment at DevServer.rs:5380-5381 to point at the new Drop. The contradictory field doc sits 130 lines above the block this PR just rewrote, in the same file, describing the same field. Per the doc-sweep guidance (CLAUDE.md — "sweep the same PR for everything describing the old state... a stale refcount comment invites a future maintainer to restore unref()"), this is the natural place to fix it.
Impact
Doc-only. A future reader of :39-41 would conclude (a) HTMLBundleRoute lacks RefCounted and (b) the raw pointer is a temporary stopgap pending that impl — both wrong. Worst case, someone "completes" the TODO by swapping to RefPtr without checking whether RefPtr actually has a Drop (per ref_count.rs it does not), or assumes the ref/deref pairing here is provisional. The new :167-172 block already contradicts it ("raw ptr, no Drop ... deref'd there"), so the file is internally inconsistent after this PR.
Suggested fix
Replace the stale rationale with the real one, e.g.:
/// Stored as a raw ptr (not `RefPtr`) because `RefPtr` has no `Drop` impl
/// anyway; the intrusive ref taken in `get_or_put_route_bundle` is released
/// explicitly in `DevServer`'s `Drop` — see the cleanup note at the bottom
/// of this file.
pub html_bundle: *mut HTMLBundleRoute,and drop the TODO.
2 participants
get_or_put_route_bundletakes an intrusive ref onRoutewhen it creates aRouteBundle; the rawhtml_bundlefield has noDropandDevServer::Dropnever released it. ZigRouteBundle.deinit:112does the deref; the Rust port missed it. OneRouteper HTML route per dev-server leaked.