webcore: revoke a Worker's blob URLs when the worker exits by robobun · Pull Request #35828 · oven-sh/bun

robobun · 2026-07-25T23:05:26Z

Problem

URL.createObjectURL inside a Worker registers the blob in the process-global ObjectURLRegistry with no record of which realm created it. When the worker exits, the entry (and its payload) stays resolvable from any thread for the life of the process.

import { Worker } from "node:worker_threads";
const urls = [];
for (let i = 0; i < 5; i++) {
  const w = new Worker(`
    const { parentPort } = require("node:worker_threads");
    const u8 = new Uint8Array(16 << 20); u8.fill(0x30 + ${i});
    parentPort.postMessage(URL.createObjectURL(new Blob([u8])));
    setTimeout(() => process.exit(0), 10);`, { eval: true });
  urls.push(await new Promise(r => w.once("message", r)));
  await new Promise(r => w.once("exit", r));
}
let servedMB = 0;
for (const u of urls) { try { servedMB += (await (await fetch(u)).arrayBuffer()).byteLength >> 20; } catch {} }
console.log({ servedMBAfterAllWorkersExited: servedMB });
// node: 0   bun (before): 80

The File API spec scopes a blob URL store entry to the environment that created it and removes the entry when that environment is discarded. Node follows this (the parent's fetch rejects after the worker exits).

Fix

ObjectURLRegistry::Entry now carries the creating ScriptExecutionContext id, recorded in register() from vm.initial_script_execution_context_identifier.
New ObjectURLRegistry::revoke_all_for_context(context_id) removes every entry whose context matches.
New RuntimeHooks::revoke_object_urls_for_context slot (the registry lives in bun_runtime, worker teardown in bun_jsc).
WebWorker::shutdown calls it right after vm.on_exit() and before dispatchExit, so by the time the parent observes the exit event the worker's URLs are gone. Entries from other contexts are untouched.

Verification

With the fix the repro above prints { servedMBAfterAllWorkersExited: 0 }. New tests in test/js/web/workers/worker_blob.test.ts check that a worker's blob URL stops resolving (via resolveObjectURL and fetch) once the worker has exited, and that worker exit leaves the parent's own blob URLs alone.

URL.createObjectURL inside a Worker registered the blob in the process-global ObjectURLRegistry with no owner, so when the worker exited the entry (and its payload) stayed resolvable from any thread for the life of the process. Five workers that each mint a 16 MB blob URL and then exit left 80 MB served byte-exact after every owning thread was dead; Node rejects the same fetch because the File API scopes a blob URL store entry to the environment that created it and removes the entry when that environment is discarded. Tag each registry entry with the creating ScriptExecutionContext id and add revoke_all_for_context, wired through RuntimeHooks so WebWorker::shutdown can call it right after the worker's on_exit handlers run and before dispatchExit posts the close task to the parent. By the time the parent observes the exit event, the worker's blob URLs are gone; entries created by other contexts are untouched.

coderabbitai · 2026-07-25T23:05:32Z

Warning

Review limit reached

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

Next review available in: 17 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: 0d39596e-85b4-4bcc-864b-aa52dd15d5c9

📥 Commits

Reviewing files that changed from the base of the PR and between 916492f and 32a417c.

📒 Files selected for processing (5)

src/jsc/VirtualMachine.rs
src/jsc/web_worker.rs
src/runtime/jsc_hooks.rs
src/runtime/webcore/ObjectURLRegistry.rs
test/js/web/workers/worker_blob.test.ts

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

github-actions · 2026-07-25T23:09:27Z

Found 2 issues this PR may fix:

Bun leaks memory in Workers #5709 - Worker memory leak: unreleased blob URLs are one contributing cause of RSS growing monotonically when spawning workers
macOS Apple Silicon: memory invisible to RSS — bmalloc slabs, worker cleanup gaps, GC safety bugs #28318 - Incomplete worker cleanup: blob URL revocation on worker exit addresses one of the documented cleanup gaps

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

Fixes #5709
Fixes #28318

🤖 Generated with Claude Code

github-actions · 2026-07-25T23:10:17Z

+    /// `ObjectURLRegistry::revoke_all_for_context` — drops every blob URL a
+    /// worker registered, called from `WebWorker::shutdown`. Registry lives in
+    /// `bun_runtime::webcore`.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-07-25T23:10:18Z

+                // Before `dispatchExit` so the parent never observes `exit`
+                // with this worker's blob URLs still live.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-07-25T23:10:19Z

This PR may be a duplicate of:

webcore: sweep worker-owned blob URLs on Worker shutdown #35827 - Same fix: tags ObjectURLRegistry entries with an owner context ID and sweeps them during WebWorker::shutdown via RuntimeHooks, touching the same four production files

🤖 Generated with Claude Code

robobun · 2026-07-25T23:11:06Z

Duplicate of #35827, which landed the same approach a few minutes earlier. Closing this one; left a note there with the alternate test in case it's useful.

robobun · 2026-07-25T23:11:44Z

^{Updated 4:06 PM PT - Jul 25th, 2026}

@robobun, your commit cdb759e is building: #81694

claude

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

🟡 src/runtime/webcore/ObjectURLRegistry.rs:36 — The comment just below Entry still says "its sole field is Blob", but this PR adds a second field context_id: i32. The assert_send::<Entry>() still holds (i32 is trivially Send), so this is just a stale comment — drop "sole" or reword to cover both fields.
Extended reasoning...

What

The static-assertion comment immediately below the Entry struct (lines 39–40) reads:
```
// `Entry` is auto-`Send`: its sole field is `Blob`, which already asserts
// `Send + Sync` (see `webcore_types::Blob`). No `unsafe impl` needed.
```
This PR adds a second field to Entry:
```
pub struct Entry {
    blob: Blob,
    context_id: i32,   // ← new in this PR
}
```
so the "sole field" phrasing is now factually wrong.

Step-by-step
1. Before this PR, Entry had exactly one field (blob: Blob), and the comment accurately explained why the compile-time assert_send::<Entry>() passes without an unsafe impl Send.
2. This PR adds context_id: i32 at line 36 to track the creating ScriptExecutionContext.
3. The comment at line 39 is left unchanged and still claims Blob is the sole field.
4. The assertion itself is unaffected: i32 is Send + Sync by auto-trait rules, so Entry is still auto-Send and assert_send::<Entry>() compiles. There is no runtime or type-checking impact.
Why it's worth mentioning

Per REVIEW.md, comments carry "only durable non-obvious content: invariants, ownership/lifetime contracts, SAFETY justifications". This comment is exactly that kind of invariant justification — it explains why no unsafe impl is needed. A comment stating an invariant that no longer describes the struct it justifies is the sort of drift REVIEW.md asks to keep accurate. Someone later auditing thread-safety would read "sole field is Blob", see two fields, and have to re-derive whether the assertion is still meaningful.

Impact

Documentation-only. Zero effect on generated code, tests, or behavior. Not worth blocking merge over.

Fix

A one-word edit, e.g.:
```
// `Entry` is auto-`Send`: `Blob` already asserts `Send + Sync` (see
// `webcore_types::Blob`) and `i32` is trivially `Send`. No `unsafe impl` needed.
```
or simply drop "sole" → "its Blob field already asserts Send + Sync…".

claude · 2026-07-25T23:16:34Z

+  await new Promise(resolve => worker.once("exit", resolve));
+
+  expect(resolveObjectURL(url)).toBeUndefined();
+  await expect(fetch(url)).rejects.toThrow();


🟡 Bare .rejects.toThrow() passes on any rejection, so this line doesn't actually pin the revoked-URL behavior — REVIEW.md asks for a specific error class/code/message (never bare toThrow()). Consider asserting the actual error, e.g. .rejects.toThrow(TypeError) or matching /Failed to resolve blob:/ (which is what fetch on a missing blob URL produces). Nit only — line 140's resolveObjectURL(url) → undefined already proves revocation.

Extended reasoning...

What the issue is

REVIEW.md, under Every assertion must be able to fail, and assert the strongest invariant, states: "specific error class/code/message (never bare toThrow())". Line 141 asserts await expect(fetch(url)).rejects.toThrow(); with no error class or message argument. Any rejection — including one from an unrelated regression in fetch(blob:...) (a parse failure, a network-layer bug, an OOM) — satisfies this assertion, so the line does not actually pin the reason the fetch fails.

Code path

When fetch() is given a blob: URL whose UUID is not in ObjectURLRegistry, src/runtime/webcore/fetch.rs (around the ObjectURLRegistry::singleton().resolve_and_dupe(...) call) rejects with a TypeError (ErrorCode INVALID_ARG_VALUE) whose message is Failed to resolve blob:<uuid>. That is the specific, stable error this test should assert.

Why the existing check on line 140 doesn't fully cover it

Line 140 (expect(resolveObjectURL(url)).toBeUndefined()) already proves the registry entry is gone via a completely separate code path (node:buffer's resolveObjectURL), so the test as a whole is not vacuous — the primary property (revocation on worker exit) is pinned. The fetch assertion on line 141 is a secondary check that the registry removal is also observable through fetch. But in its bare form it would still pass if, say, a future refactor made fetch(blob:...) reject before ever consulting the registry, or reject for a different reason on this URL shape — masking a regression in the very path being exercised.

Step-by-step proof

Worker exits → revoke_all_for_context removes the entry.

resolveObjectURL(url) returns undefined → line 140 passes (correct).

Suppose a regression makes fetch on any blob: URL throw RangeError: invalid URL before the registry lookup. fetch(url) rejects → bare .rejects.toThrow() on line 141 still passes.

The test is green even though fetch no longer proves anything about revocation. With .rejects.toThrow(/Failed to resolve blob:/) the test would fail and catch the regression.

Impact

Low. Nothing breaks if this merges as-is: line 140 already asserts the load-bearing invariant, and the runtime change is fully covered. This is a test-quality tightening per the repo's stated review rule, not a correctness bug.

How to fix

await expect(fetch(url)).rejects.toThrow(TypeError); // or, more precisely: await expect(fetch(url)).rejects.toThrow(/Failed to resolve blob:/);

Addressing the refutation

One verifier refuted this as a duplicate of another report on the same line. That objection is about deduplication bookkeeping, not validity — it explicitly does not dispute the finding itself. Synthesis has consolidated the reports into this single entry, so the duplication concern is moot; only one comment will be posted.

claude · 2026-07-25T23:16:35Z

+  const url = await new Promise<string>(resolve => worker.once("message", resolve));
+  await new Promise(resolve => worker.once("exit", resolve));


🟡 nit: both new tests await new Promise(resolve => worker.once("message", resolve)) without wiring worker.once("error", reject). If a regression breaks Worker eval startup or URL.createObjectURL inside the worker, message never fires and the test hangs to the file timeout instead of failing fast with the actual error. Same shape at line 152. REVIEW.md: "Wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise."

Extended reasoning...

What the issue is

Both new tests added in this PR wait for the worker's first message with a resolve-only promise:

// line 137 const url = await new Promise<string>(resolve => worker.once("message", resolve)); // line 152 const workerUrl = await new Promise<string>(resolve => worker.once("message", resolve));

Neither promise is wired to reject on the worker's error event. REVIEW.md's Tests reviewers reject section is explicit on this shape:

Wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise — never throw inside event callbacks.

How it manifests

The eval scripts here are trivial and pass today, so nothing breaks on merge. The concern is diagnosability under a future regression: if new NodeWorker(src, { eval: true }) startup regresses, or URL.createObjectURL / Blob construction inside the worker starts throwing, the worker emits error (and then exit) but never message. The awaited promise on line 137 never settles, and the test sits idle until the file-level timeout fires — surfacing as a generic "timed out" instead of the actual worker error.

The exit awaits on lines 138/153 are fine on their own (a node:worker_threads Worker always emits exit, even on error), but execution never reaches them because it's blocked on the preceding message await.

Why existing code doesn't prevent it

There is no .on("error", ...) handler on either NodeWorker instance. With node:worker_threads, an unhandled error event is re-thrown on the parent's next tick, which in Bun's test runner would crash the test process rather than silently pass — so the test wouldn't go green on a broken build. But the failure mode is a process-level uncaught exception (or a hang, depending on ordering), not a clean test failure pointing at the worker's error. The REVIEW.md rule exists precisely so the awaited promise is the thing that fails, carrying the real error into the test's failure output.

Pre-existing tests in this file (e.g. the Web Worker tests at lines 19–27, 109–125) share the resolve-only shape, but per the guidelines local precedent doesn't weaken the rule for new code.

Step-by-step proof

Suppose a regression makes URL.createObjectURL throw inside a worker (or the eval-mode entry fails to parse).

The worker never reaches parentPort.postMessage(...), so "message" never fires.

The NodeWorker emits "error" with the thrown value, then "exit".

On line 137, worker.once("message", resolve) was the only handler registered; resolve is never called and the promise stays pending.

await on line 137 blocks; line 138's exit await is never reached.

The test hangs until the runner's timeout, reporting "timed out" with no mention of the actual worker error.

Fix

Wire error to reject on the message await (both tests):

const url = await new Promise<string>((resolve, reject) => { worker.once("message", resolve); worker.once("error", reject); });

The subsequent exit await can stay as-is (it always fires), or be folded into the same pattern for symmetry.

Severity

nit — this is test-diagnosability, not a correctness bug in the shipped runtime code. The happy path works and a regression would still fail (just less cleanly and more slowly).

robobun assigned Jarred-Sumner Jul 25, 2026

github-actions Bot added the claude label Jul 25, 2026

github-actions Bot mentioned this pull request Jul 25, 2026

webcore: sweep worker-owned blob URLs on Worker shutdown #35827

Open

[autofix.ci] apply automated fixes

fea8f09