bundler: keep BundlerPlugin filter data alive across Worker.terminate() - #36807
Open
robobun wants to merge 10 commits into
Open
bundler: keep BundlerPlugin filter data alive across Worker.terminate()#36807robobun wants to merge 10 commits into
robobun wants to merge 10 commits into
Claude / Claude Code Review
completed
Aug 3, 2026 in 17m 50s
Code review found 2 potential issues
Found 2 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/runtime/api/JSBundler.rs:1702-1705 |
Sibling Plugin::create caller in server_body.rs not audited — new +1 BundlerPlugin ref leaks |
| 🟡 Nit | src/runtime/api/JSBundler.rs:1738-1744 |
Stale trait doc: PluginJscExt::destroy no longer checks null via opaque_ref |
Annotations
Check warning on line 1705 in src/runtime/api/JSBundler.rs
claude / Claude Code Review
Sibling Plugin::create caller in server_body.rs not audited — new +1 BundlerPlugin ref leaks
There's a third `Plugin::create` caller — `ServePlugins::load_and_resolve_plugins` in `src/runtime/server/server_body.rs:1047-1049` — that wasn't included in this audit. It wraps the returned pointer in `Box<JSBundler::Plugin>` and relies on `Box` drop (a no-op for the `opaque_ffi!` ZST); `Plugin::destroy` is never called anywhere in that file, so the new +1 `BundlerPlugin` ref leaks past VM teardown. The missing-destroy itself is pre-existing and the incremental leak is tiny (one `BundlerPlugin
Check warning on line 1744 in src/runtime/api/JSBundler.rs
claude / Claude Code Review
Stale trait doc: PluginJscExt::destroy no longer checks null via opaque_ref
The trait doc on `PluginJscExt::destroy` (a few lines up, on `fn destroy(this: *mut Plugin);`) still says "non-null is checked via `Plugin::opaque_ref` (panics on null)", but 85687ea9 changed this impl to `debug_assert!(!this.is_null()); unsafe { JSBundlerPlugin__destroy(this) }` — it no longer routes through `opaque_ref`, and the null check is debug-only. All callers pass `NonNull::as_ptr()` so this is doc-only; the trait doc just needs to be updated to match ("debug-asserted non-null; caller m
Loading