webcore: sweep worker-owned blob URLs on Worker shutdown - #35827
Conversation
ObjectURLRegistry is a process-global map keyed by UUID. register() receives the creating VM but records no owner on the entry, and the only removal path is URL.revokeObjectURL. A Worker that calls URL.createObjectURL and exits without revoking therefore pins the duped blob store for the lifetime of the process: roughly 1 MB retained per MB minted per dead worker, linearly. Record the registering context's initial_script_execution_context_identifier on each Entry and add ObjectURLRegistry::sweep_owner, reached from WebWorker::shutdown via a new RuntimeHooks slot (the registry lives in bun_runtime, the shutdown path in bun_jsc). The sweep runs in step 2 of shutdown, after vm.on_exit() and alongside rare.release_js_handles(), so user exit handlers can still revoke and the JSC VM is still live when the entries drop. The process-global map is unchanged, so cross-thread fetch/revoke of a worker's blob URL while the worker is alive keeps working. After the worker exits, fetching one of its blob URLs fails the same way as a revoked URL. Node's per-Environment registry drops worker entries at env teardown as well; Bun continues to allow cross-thread resolution while the owning context is alive.
|
Reproduced locally: 40 workers x 4 MB grows RSS by ~161 MB on release bun and the dead worker's URL still serves its payload. With the sweep, growth is ~11 MB and the URL throws. The existing five CI status: the new The |
WalkthroughChangesWorker object URL cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:07 PM PT - Jul 25th, 2026
❌ @robobun, your commit 0376626 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35827That installs a local version of the PR into your bun-35827 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
#35828 landed five minutes after this one with the same fix shape (owner id on
Either fix closes the leak; happy to fold the in-process |
|
I opened #35828 independently with the same core fix (context-id tag on Branch for reference: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline test nits: I checked that ScriptExecutionContextIdentifier is a process-global monotonic counter (++lastUniqueIdentifier in ScriptExecutionContext.cpp) and never recycled, so sweep_owner cannot hit a live context's entries; and that Entry::drop under the map lock only runs blob.deinit() (refcount decrement on a Send + Sync store — no JS, no registry re-entry). The hook plumbing mirrors close_dns_for_terminate just above it. Deferring to a human because this sits in worker-shutdown ordering and introduces a user-visible semantic change (dead-worker blob URLs stop resolving).
Extended reasoning...
Verified the owner key is safe: context identifiers come from a process-global atomic increment and are never reused, so sweeping a dead worker's id cannot collide with the main context or another worker. Also confirmed the sweep runs after vm.on_exit() (user exit handlers see their URLs intact) and before JSC teardown, and that dropping Box<Entry> while holding the Guarded lock only decrements a blob-store refcount — no path back into JS or the registry. The change itself is small and patterned on the adjacent close_dns_for_terminate hook, but worker teardown is a high-scrutiny path and the PR deliberately changes observable behavior, so a human should sign off.
… reject The deadUrlServed / parentUrlStillResolves checks observe registry state via fetch() and are platform-independent; only the RSS bound is skipped on Windows. Also give runWorker() a rejection path so a worker that errors before postMessage surfaces the error instead of hanging the subprocess until the outer timeout.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Both of my earlier notes (Windows gating only the RSS assertion, wiring w.onerror to reject) are addressed in b33aaab, and the doc-comment trims in c65c1d5 look fine. I didn't find anything further, but this touches WebWorker::shutdown teardown ordering and introduces a user-visible semantic change (dead-worker blob URLs now fail to resolve), so it's worth a human sign-off.
What was reviewed:
sweep_ownerholds theGuardedlock across collect+remove;Box<Entry>drop runsblob.deinit(), which is already thread-agnostic (same path asrevoke()).- Sweep placement is after
on_exit/close_all_socket_groups/release_js_handles(nothing left to re-register) and beforeteardownJSCVM, so worker VM state is still live. - Owner key is
initial_script_execution_context_identifier— set toworker.execution_context_id()ininit_worker, distinct from the main context; test'sparentUrlStillResolvesconfirms no over-deletion.
Extended reasoning...
Overview
The PR tags each ObjectURLRegistry entry with the registering VM's initial_script_execution_context_identifier, adds sweep_owner(id) to drop all entries with that owner, and calls it from WebWorker::shutdown step 2 via a new RuntimeHooks slot. Five files touched: the registry (Entry gains a field, register records it, new sweep_owner), RuntimeHooks struct, the hook impl in jsc_hooks.rs, the call site in web_worker.rs, and a subprocess-based leak+semantics test.
Security risks
None identified. No untrusted input parsing; the owner id is a VM-internal integer. The sweep runs under the existing Guarded mutex, so it composes with concurrent register/revoke/resolve_and_dupe from other threads.
Level of scrutiny
High. WebWorker::shutdown is a memory-safety-critical teardown sequence with documented step-by-step ordering constraints (VM unpublish, exit handlers, JSC teardown, dispatchExit, arena free). Inserting a new call requires confidence it doesn't touch state freed earlier or run JS after termination is armed. The placement here — after release_js_handles(), before exit_code snapshot — is correct: JSC is still live, no JS runs (sweep_owner is pure Rust map manipulation + Blob::deinit), and it follows the exact wiring pattern of close_dns_for_terminate directly above. But this is the kind of ordering a maintainer should confirm.
Other factors
- Semantic change: blob URLs minted inside a worker now fail like revoked URLs after that worker exits/terminates. The PR argues this matches Node's per-Environment teardown and no in-tree test relied on cross-worker survival, but it's still a behavioral change a human should ratify.
- The hook indirection (
bun_jsc→bun_runtime) mirrors every otherRuntimeHooksslot; nothing novel there. sweep_owneruses collect-keys-then-remove rather thanretain; correct if slightly less direct, and the map is small.- Test now runs on all platforms with only the RSS check gated on
!isWindows;runWorkerwiresonerrorto reject. Both of my prior comments are resolved. - A duplicate PR (#35828) with the same shape was closed in favor of this one.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/webcore/ObjectURLRegistry.rs`:
- Around line 105-120: Update ObjectURLRegistry::sweep_owner to use the backing
map’s retain method, removing entries whose owner matches the supplied owner.
First verify that the map type supports retain with the expected key/value
predicate signature; preserve the existing lock and leave entries belonging to
other owners untouched.
In `@test/js/web/workers/worker_blob.test.ts`:
- Around line 134-216: Update the test declaration for “blob URLs created inside
a Worker are released when the Worker exits” to use test.concurrent, preserving
its existing subprocess logic, assertions, and timeout.
🪄 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: 380aa898-79e9-4878-b34a-2f2d0ac32bbc
📒 Files selected for processing (5)
src/jsc/VirtualMachine.rssrc/jsc/web_worker.rssrc/runtime/jsc_hooks.rssrc/runtime/webcore/ObjectURLRegistry.rstest/js/web/workers/worker_blob.test.ts
There was a problem hiding this comment.
No new issues found — both prior nits (Windows gating, onerror wiring) are addressed in b33aaab, and c65c1d5 trimmed the flagged doc comments. Deferring to a maintainer for the user-visible semantic change (blob URLs minted inside a worker now fail to resolve after the worker exits) and the placement in WebWorker::shutdown's teardown ordering.
What was reviewed:
sweep_ownerholds the registry lock across collect+remove;Entry::drop→Blob::deinit→detach()is a thread-safe refcount decrement with no re-entry into the registry.- Owner id sourced from
initial_script_execution_context_identifier(i32), set per-worker ininit_workerfromexecution_context_id(); main-context entries carry a distinct id and are never swept. - Sweep placement after
release_js_handles()— every earlier step that can run JS (on_exit, socketon_close, DNS destruction) has already run, so nothing re-registers post-sweep.
Extended reasoning...
Overview
The PR tags each ObjectURLRegistry entry with the registering VM's initial_script_execution_context_identifier, adds sweep_owner(id) to remove all entries for a given context, and wires it into WebWorker::shutdown via a new RuntimeHooks::sweep_object_urls_for_owner slot. Five files touched: the registry (~25 lines), the hook declaration/impl (~10 lines), the shutdown call site (~6 lines), and a subprocess-spawning leak+semantics test.
Security risks
None identified. The registry is process-global and mutex-guarded; the new sweep only removes entries and does so under the existing lock. No new user-controlled input reaches native code.
Level of scrutiny
Medium-high. WebWorker::shutdown is a teardown-ordering-critical path with a documented step barrier (each numbered step is a barrier for the next), and the file header enumerates several UAF hazards around it. The new call is placed inside step 2, after every hook that can re-enter JS and before teardownJSCVM, which is the correct window — but shutdown ordering in this file has historically been delicate (see the surrounding comments on markTerminating, close_dns_for_terminate, release_queued_tasks_for_shutdown). The semantic change — a dead worker's blob URL now fails to resolve rather than serving its payload — is user-observable and worth a maintainer's explicit ack, even though it aligns with Node's per-Environment registry behavior.
Other factors
- The RuntimeHooks pattern is copied exactly from the adjacent
close_dns_for_terminate/cancel_all_timersslots; no novel abstraction. Entryremains auto-Send(i32+Blob); the compile-timeassert_sendstill covers it.- The test now runs on all platforms with only the RSS threshold gated on
!isWindows, andrunWorker()wiresonerrorto reject — both prior review points are resolved. - A duplicate PR (#35828) with the same fix shape was closed in favor of this one; the author of that PR noted the sweep placement here (after
release_js_handles()) is preferable. - No bugs were surfaced by the bug-hunting pass on the current revision.
2 participants
Reproduction
200 workers each mint 4x1 MB blob URLs and exit without revoking; post-GC RSS grows ~806 MB (~1 MB retained per MB minted per dead worker). The first dead worker's URL still serves its payload.
Cause
ObjectURLRegistry(src/runtime/webcore/ObjectURLRegistry.rs) is a process-globalGuarded<HashMap<[u8;16], Box<Entry>>>.register()receives the creatingVirtualMachinebut records no owner on theEntry; the only removal path isrevoke()fromURL.revokeObjectURL.WebWorker::shutdown,RareData::deinit, andglobal_exitnever touch the map, so a worker's entries (each a strongdupe_with_content_typeof the blob store) outlive the worker indefinitely.Fix
Entrygainsowner: i32= the registering VM'sinitial_script_execution_context_identifier.ObjectURLRegistry::sweep_owner(id)removes every entry with that owner.RuntimeHooks::sweep_object_urls_for_ownerslot letsbun_jscreach the registry inbun_runtime.WebWorker::shutdowncalls the sweep in step 2, aftervm.on_exit()and alongsiderare.release_js_handles(), beforeWebWorker__teardownJSCVM.The process-global map stays, so cross-thread
fetch/revokeObjectURLof a worker's URL while that worker is alive keeps working (a Bun extension over Node's per-Environment registry). Main-context entries are unaffected.Semantic change
After a worker exits or is terminated,
fetch()of a blob URL minted inside that worker fails like a revoked URL instead of serving the payload. No in-tree test relied on cross-worker blob-URL survival; every worker test mints the URL on the parent. Node drops worker blob URLs at environment teardown as well.Verification
test/js/web/workers/worker_blob.test.tsspawns a subprocess that cycles 30 workers (each minting 4x1 MB), then asserts post-GC RSS growth < 40 MB (unfixed: ~120 MB), the dead worker's URL no longer resolves, and the parent-minted worker-source URL still does.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/workers/worker_blob.test.ts