Skip to content

bake: deref html_bundle in DevServer Drop #32213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alii wants to merge 1 commit into from
Closed

bake: deref html_bundle in DevServer Drop #32213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/runtime/bake/DevServer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1247,6 +1247,10 @@
// SAFETY: stored ref from `init_from_any_blob`; no live borrow.
unsafe { StaticRoute::deref_(cached.as_ptr()) };
}
// 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) };

Check warning on line 1253 in src/runtime/bake/DevServer.rs

View check run for this annotation

Claude / Claude Code Review

Leak fix lands without a regression test 

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 th
Comment on lines +1250 to +1253

@coderabbitai coderabbitai Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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

Comment on lines +1250 to +1253

@claude claude Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

  1. HTMLBundleRoute is declared with #[ref_count(debug_name = "HTMLBundleRoute")] (HTMLBundle.rs:149), which registers it with the intrusive-refcount tracker by name.
  2. A test can record the baseline live count for "HTMLBundleRoute", construct a DevServer with N HTML routes (which calls get_or_put_route_bundle → ref_ N times), then drop the DevServer.
  3. On the unfixed build, the tracked count after drop would sit at baseline + N because DevServer::Drop never released html.html_bundle. On the fixed build it returns to baseline.
  4. 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.

}
}

Expand Down Expand Up @@ -5376,8 +5380,8 @@
let file = &mut self.client_graph.bundled_files.values_mut()
[incremental_graph_index.get() as usize];
file.html_route_bundle_index = Some(bundle_index);
// Bump the intrusive refcount; matched by
// `RouteBundle::deinit`'s deref of `html_bundle`.
// Bump the intrusive refcount; matched by the
// `html_bundle` deref in `DevServer`'s `Drop`.
// SAFETY: `html` is a live IntrusiveRc-managed allocation.
unsafe { bun_ptr::RefCount::<HTMLBundleRoute>::ref_(html) };
break 'brk route_bundle::Data::Html(route_bundle::Html {
Expand Down
11 changes: 6 additions & 5 deletions src/runtime/bake/dev_server/route_bundle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,9 @@
}
}

// `deinit` is fully subsumed by Drop:
// - client_bundle / cached_response: Option<Arc<StaticRoute>> drop = .deref()
// - Framework: StrongOptional fields drop = .deinit()
// - Html: bundled_html_text Box<[u8]> drop = allocator.free()
// html_bundle RefPtr drop = .deref()
// 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.

Check failure on line 172 in src/runtime/bake/dev_server/route_bundle.rs

View check run for this annotation

Claude / Claude Code Review

cached_response leaks at two sites contradicting new BackRef contract 

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. **

Check warning on line 172 in src/runtime/bake/dev_server/route_bundle.rs

View check run for this annotation

Claude / Claude Code Review

Stale 'does not yet impl RefCounted' comment on html_bundle field 

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 clean
Comment on lines +167 to +172

@claude claude Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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)

  1. User loads an HTML route. on_html_request calls init_from_any_blob → route_ptr with refcount=1, stored into html.cached_response = Some(BackRef(route_ptr)) (DevServer.rs:2835-2848).
  2. User edits a file; hot-reload runs with had_adjusted_edges = true.
  3. DevServer.rs:4579 executes html.cached_response.take() → returns Some(BackRef(route_ptr)), slot now None.
  4. let _ = blob; drops a Copy value — no destructor runs, refcount still 1.
  5. Next request repopulates cached_response with a new StaticRoute. The old one is unreachable with refcount=1 → leaked.
  6. Repeat per hot-reload. DevServer::Drop only 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.

Comment on lines +167 to +172

@claude claude Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 HTMLBundleRoute does not yet impl bun_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:

  1. HTMLBundleRoute is Route. src/runtime/server/HTMLBundle.rs:137: pub(crate) type HTMLBundleRoute = Route;.
  2. Route derives RefCounted. 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".
  3. RefCount<T> requires T: RefCounted. src/ptr/ref_count.rs:248: pub struct RefCount<T: RefCounted>; :258: impl<T: RefCounted> RefCount<T>.
  4. This PR (and the existing code) call RefCount::<HTMLBundleRoute> methods. Pre-existing DevServer.rs:5386 calls RefCount::<HTMLBundleRoute>::ref_(html); this PR adds DevServer.rs:1253 calling RefCount::<HTMLBundleRoute>::deref(html.html_bundle). By (3), neither would type-check unless HTMLBundleRoute: 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.

Loading