FinalizationRegistry: keep the registry alive while it has registrations - #35213
FinalizationRegistry: keep the registry alive while it has registrations#35213robobun wants to merge 5 commits into
Conversation
JavaScriptCore's async bytecode generator only preserves locals that are
read after an await, so a `const fr = new FinalizationRegistry(...)`
whose last use is `fr.register(...)` becomes unreachable at the next
suspend point. The registry is then swept in the same GC that collects
its targets, before finalizeUnconditionally ever sees them, and no
cleanup callback fires. V8 preserves every async-function local across
await, so Node.js never hits this; the result is 0/K callbacks delivered
in Bun where Node delivers K/K for the same program.
Override FinalizationRegistry.prototype.{register,unregister} to add the
receiver to a per-VM Strong root set on the first successful register()
and drop it once liveCount + deadCount is zero (checked after each
cleanup-task run and after each unregister()). A registry with no
registrations is never rooted, and a drained one becomes collectable
again on the next GC, so this introduces no new leak.
|
Updated 1:55 AM PT - Jul 23rd, 2026
✅ @robobun, your commit 8e3d7306d5403720ce3552237554379a069c0be9 passed in 🧪 To try this PR locally: bunx bun-pr 35213That installs a local version of the PR into your bun-35213 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Re #33044: probably not fixed by this. That test's |
WalkthroughChangesAdds JavaScriptCore FinalizationRegistry lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ment sleep(300) - Expand the rootFinalizationRegistry comment to state that a registry holding a registration for a target that never dies is retained until VM shutdown, and why matching V8 exactly is not done here. - Call installFinalizationRegistryPrototypeHooks from NodeVMGlobalObject:: finishCreation so node:vm contexts get the same rooting. - Add a test documenting the immortal-target retention and a node:vm test. - Comment the await sleep(300) in the first test: it preserves the reported repro shape; the suspend point is what matters.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/web/finalization-registry.test.ts (1)
91-119: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun this isolated subprocess test concurrently.
run()creates independent fixture and process state, so this test can usetest.concurrentlike the surrounding GC cases.Proposed change
- test("the registry is released once every registration is drained", async () => { + test.concurrent("the registry is released once every registration is drained", async () => {🤖 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 `@test/js/web/finalization-registry.test.ts` around lines 91 - 119, Keep the FinalizationRegistry test declared with test.concurrent, matching the surrounding GC tests, so the isolated run fixture can execute concurrently. Do not change the existing cleanup, draining, assertions, or subprocess logic.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/web/finalization-registry.test.ts`:
- Around line 91-119: Keep the FinalizationRegistry test declared with
test.concurrent, matching the surrounding GC tests, so the isolated run fixture
can execute concurrently. Do not change the existing cleanup, draining,
assertions, or subprocess logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ac332e6-2680-424f-81ed-ff00de92b3d1
📒 Files selected for processing (3)
src/jsc/bindings/JSCTaskScheduler.hsrc/jsc/bindings/NodeVM.cpptest/js/web/finalization-registry.test.ts
The Strong root is released only once liveCount + deadCount == 0, i.e. after every target has died; the immortal-target clause in the same comment and the over-correction test both describe the longest-lived case.
2033827 to
438efb9
Compare
There was a problem hiding this comment.
All prior findings are addressed — the shortest → longest doc fix landed in 2033827. No new issues this pass. Deferring to a maintainer only for the retention trade-off itself: pinning a registry to its longest-lived target is now well-documented and tested, but it's a deliberate divergence from V8 in the immortal-target case that's worth a human sign-off.
What was reviewed
- Strong-root lifecycle: added on first
register(), dropped on drainedunregister()/ post-cleanup-task /markShuttingDown— every acquire has a paired release. node:vmhook inNodeVMGlobalObject::finishCreation; ruled out that forcing JSC's lazyfinalizationRegistryStructure()there is a problem (per-context, one-time).- Replacement
register/unregisterargument validation and.lengthmatch JSC's originals; error messages preserved. m_rootedFinalizationRegistriesis JS-thread-only, keyed by a cell the Strong value keeps alive, and cleared before the shutdown flag flips — no lock needed, no dangling key.
Extended reasoning...
Overview
Overrides FinalizationRegistry.prototype.{register,unregister} with Bun host functions that additionally maintain a per-VM Strong<> root set on JSCTaskScheduler, so a registry whose only JS reference is an async-function local (dropped by JSC's generatorification at the next await) survives until its registrations drain. Hooks are installed for both the main global (ZigGlobalObject::addBuiltinGlobals) and node:vm contexts (NodeVMGlobalObject::finishCreation). Eight subprocess tests cover the fix, the release path, node:vm, and the documented over-retention.
Progress since last review
Three rounds of my inline comments have all been addressed: the immortal-target over-retention is now documented in the header and covered by a dedicated test; node:vm contexts get the hooks; the sleep(300) carries a comment; and the header comment now correctly says "longest-lived" (2033827). No new findings this run; the finder's candidate about eagerly forcing the lazy structure in NodeVM.cpp was verified harmless.
Security risks
None identified. No untrusted parsing; validation of register/unregister arguments mirrors JSC's originals verbatim. The Strong root map is JS-thread-only and cleared at VM shutdown.
Level of scrutiny
High — this is C++ GC-lifetime code adding JSC::Strong<> roots and overriding built-in prototype methods. The mechanism is sound and thoroughly tested, but the semantic trade-off (a registry watching an immortal target is retained until VM shutdown, along with its callback closure and every held value) is a design decision that diverges from V8. The PR documents it clearly and locks it in with a test so a future narrowing is visible; whether that trade-off is acceptable is a maintainer call, not something I should approve unilaterally.
Other factors
Test coverage is strong (positive, negative-leak, node:vm, argument validation, documented over-correction), fails-before/passes-after was re-verified against a real unpatched release after the mechgate artifact was identified, and the change composes with adjacent PRs #30857/#34270 in the same block.
There was a problem hiding this comment.
All four rounds of findings I raised have been addressed (immortal-target retention now documented + tested, node:vm hooked, sleep(300) commented, longest-lived comment fixed). No new issues found on 438efb9. Deferring to a human because this replaces JSC's FinalizationRegistry.prototype.{register,unregister} with Bun host functions that maintain per-VM Strong<> roots — GC-lifecycle C++ with an explicit, documented over-retention trade-off vs. V8 that a maintainer should sign off on.
What was reviewed:
- Root/unroot balance across all three release paths (unregister, deferred cleanup, markShuttingDown) and that the map is JS-thread-only.
- Reimplemented
register/unregisterargument validation matches JSC's originals (canBeHeldWeakly, target≠holdings, token checks, .length, DontEnum). - Hook installation covers both Zig::GlobalObject and NodeVMGlobalObject;
runPendingWorkdowncast to JSFinalizationRegistry is safe post-task. - Test hermeticity: subprocess-per-test, bounded GC poll loops, batch-of-200 pattern to defeat conservative stack pinning.
Extended reasoning...
Overview
The PR overrides FinalizationRegistry.prototype.register and unregister on both the main global and node:vm globals with Bun host functions that additionally maintain a per-VM UncheckedKeyHashMap<JSCell*, Strong<JSObject>> on JSCTaskScheduler. The Strong root is added on the first successful register(), and dropped when liveCount + deadCount == 0 (checked after unregister() and after each runFinalizationCleanup deferred-work task), or wholesale at markShuttingDown. Files touched: JSCTaskScheduler.{cpp,h} (~130 lines new logic), one-line hook installations in ZigGlobalObject.cpp and NodeVM.cpp, and an 8-test subprocess suite.
Security risks
None identified. No untrusted-input parsing; the reimplemented register/unregister validate arguments identically to JSC's originals (canBeHeldWeakly, target-vs-holdings sameness, unregister-token type) before touching native state. The Strong-root map is JS-thread-only per its comment and all mutation sites are on the JS thread (host function, deferred-work task on the JS event loop, shutdown from the owning JS thread), so no lock is needed on it. The map key is the JSCell* and the value is a Strong<> to the same cell, so the key cannot dangle while an entry exists.
Level of scrutiny
High. This is GC-lifecycle C++ that adds JSC::Strong<> roots — the exact category REVIEW.md flags for extra justification — and it replaces spec-defined built-in prototype methods with Bun-side reimplementations. It also intentionally accepts a semantic over-correction relative to V8 (a registry whose only remaining registration watches an immortal target is retained until VM shutdown), which is now documented in the header comment, the PR description, and locked in by a dedicated test. That trade-off is a design decision a maintainer should ratify.
Other factors
Four prior review rounds from me were all addressed: (1) the immortal-target over-retention is now explicitly documented and tested rather than silent; (2) NodeVMGlobalObject::finishCreation now installs the hooks so node:vm contexts get the fix, with a covering test; (3) the sleep(300) in the first test carries a comment explaining it preserves the exact reported repro shape; (4) the header comment's "shortest-lived" → "longest-lived" inversion was corrected. The mechgate "release without fix: all passed" evidence was confirmed as a gate artifact (it ran the with-fix binary). Test coverage is thorough: fail-before/pass-after verified on ASAN debug and an actual unpatched release, drain-then-collect, unregister-drains-root, never-registered-stays-collectable, node:vm, immortal-target retention, and API-shape preservation. All eight tests are test.concurrent subprocess spawns with bounded GC poll loops.
afdd939 to
8e3d730
Compare
There was a problem hiding this comment.
All four rounds of prior feedback are addressed (immortal-target retention documented + tested, node:vm hooked, sleep(300) commented, shortest→longest fixed) and this pass found nothing new. Deferring to a maintainer for the design call: the per-VM Strong<> root set intentionally over-corrects past V8 (a registry watching an immortal target is retained until VM shutdown), which is documented and locked in by a test but is the kind of GC-lifetime trade-off a human should sign off on.
Checked this pass: the reimplemented register/unregister match JSC's argument validation and .length; the map is JS-thread-only and cleared at markShuttingDown; runPendingWork's post-task dynamicDowncast on ticket->target() is safe because the ticket holds a Strong dependency on its target.
Extended reasoning...
Overview
Roots a FinalizationRegistry in a per-VM UncheckedKeyHashMap<JSCell*, Strong<JSObject>> on JSCTaskScheduler from its first successful register() until liveCount + deadCount == 0, so a registry whose only JS reference is an async-function local that JSC's generatorification drops at the next suspend point still delivers its cleanup callbacks. Implemented by overriding FinalizationRegistry.prototype.{register,unregister} with Bun host functions installed from both Zig::GlobalObject::addBuiltinGlobals and NodeVMGlobalObject::finishCreation, plus an unroot check after each finalization deferred-work task in runPendingWork. ~130 lines of C++ across JSCTaskScheduler.{h,cpp}, two one-line hook installs, and an eight-test subprocess suite.
Security risks
None identified. No untrusted input parsing beyond the existing canBeHeldWeakly / receiver-type checks copied from JSC's own protoFuncFinalizationRegistryRegister; no new FFI surface; the map is per-VM and JS-thread-only.
Level of scrutiny
High. This is GC-lifetime code: a new JSC::Strong<> root set with three release paths, a prototype override on a Web API, and an explicit semantic widening relative to both pre-patch Bun and V8 (immortal-target retention). REVIEW.md singles out unjustified Strong refs as a top-blocked category. The author has documented the trade-off in the header comment, the PR description, and a dedicated test, and explained why a Weak<>+marking-constraint alternative needs WebKit-private access — but whether the over-correction is acceptable for Bun is a maintainer call, not a mechanical one.
Other factors
All four of my prior findings are resolved and the threads are marked as such; the only commit since the last one (438efb9) is a CI retrigger. The bug hunter surfaced no new issues. The test suite covers the positive cases (module-level, async-function, IIFE, node:vm), the release paths (unregister-drains, never-registered, drained-then-collectable), the over-correction, and argument-validation parity, all as concurrent subprocess tests with bounded GC-poll loops. The mechgate "release without fix" artifact was confirmed as a stale-binary issue by the author. Given the design-level trade-off at the centre of the change, I'm not comfortable auto-approving.
There was a problem hiding this comment.
All four concerns from earlier rounds are addressed (node:vm hook, immortal-target retention documented+tested, sleep(300) comment, longest-lived wording). No new issues found. Deferring for maintainer sign-off on the documented trade-off: Strong-rooting registries until liveCount+deadCount==0 means a registry watching an immortal target (and its callback closure + held values) is retained until VM shutdown — a semantic over-correction past V8 that the author has consciously accepted over the silent-guard failure, but it's a user-visible retention change worth a human ack.
What was reviewed:
register/unregisteroverrides mirror JSC'sprotoFuncFinalizationRegistryRegister/Unregistervalidation exactly (canBeHeldWeakly, target≠holdings, token check, .length); error messages match JSC's.- Root map is JS-thread-only, keyed by cell pointer, cleared in
markShuttingDown;unrootFinalizationRegistryIfDrainedtakes the cellLock before reading counts. runPendingWorkunroot check runs after the cleanup task, so a registry that drains mid-callback is released;dynamicDowncastonticket->target()is safe (ticket keeps target alive).- Eager
finalizationRegistryStructure()initialization inaddBuiltinGlobals/NodeVMGlobalObject::finishCreationwas examined and ruled out as a concern.
Extended reasoning...
Overview
The PR overrides FinalizationRegistry.prototype.{register,unregister} with Bun host functions that additionally maintain a per-VM UncheckedKeyHashMap<JSCell*, Strong<JSObject>> on JSCTaskScheduler. A registry is Strong-rooted on its first successful register() and released when liveCount+deadCount reaches zero (via unregister(), via the post-runFinalizationCleanup check in runPendingWork, or at VM shutdown). Hooks are installed for both Zig::GlobalObject::addBuiltinGlobals and NodeVMGlobalObject::finishCreation. Eight concurrent subprocess tests cover the repro shape, async-function variant, drain-then-collectable, unregister-drains-root, never-registered-stays-collectable, node:vm, the immortal-target over-correction, and argument validation.
Security risks
None identified. No untrusted input parsing beyond the argument validation that already existed in JSC's stock implementation, which is reproduced verbatim. The map is JS-thread-only and cleared at shutdown.
Level of scrutiny
High. This is GC-lifetime code adding JSC::Strong<> roots — the most-blocked category in REVIEW.md — and it changes user-visible semantics of a Web API (a registry that would previously be collected without firing callbacks now fires them, and conversely a registry watching an immortal target that would previously be collected is now retained). The mechanism is sound and well-tested; the open question is whether the documented over-correction is the right trade-off for Bun to ship, which is a maintainer call rather than a correctness bug.
Other factors
Four prior review rounds from me were all addressed in 9b85a1c and 438efb9: the immortal-target retention is now documented in the header comment and locked in by a dedicated test; node:vm contexts get the hook; the sleep(300) carries a comment; the "longest-lived" wording is fixed. The bug-hunting system found nothing new this run, and the one candidate it examined (eager LazyClassStructure initialization) was refuted. Test coverage is thorough and the fails-without-fix / passes-with-fix split is verified (5 fail on unpatched release, 8 pass on patched). The change is self-contained to JSCTaskScheduler plus two one-line hook installations.
|
CI status:
All are scraped as |
2 participants
What does this PR do?
A
FinalizationRegistrywhose only reference is a module-level or async-functionconstcan be collected, with all its pending registrations, before a single cleanup callback fires.Cause
JavaScriptCore's generatorification only saves locals that are read after an
await/yieldinto the suspended generator state.fris last read atfr.register(o, i), so afterawait sleep(300)nothing roots it. The GC that fires during the sleep marks neitherfrnor its targets;finalizeMarkedUnconditionalFinalizersonly walks marked cells, soJSFinalizationRegistry::finalizeUnconditionallynever runs forfrand no cleanup task is ever scheduled. The registry is swept along with its registrations. The same applies to plainfunction*and async generators.V8 preserves every async-function local across
await, so the same program keepsfrreachable in Node.js (verified: aWeakReftofrderefs non-undefined there, undefined in Bun). When the registry is truly unreachable in both engines (e.g. created inside an IIFE that returns), both deliver zero callbacks, so this is purely the generatorified-local-lifetime difference being exposed. Standalone jsc-shell repro and analysis for an upstream report are in the linked thread.The spec permits collecting an unreferenced registry without running its callbacks, but the Node.js behaviour is what every
FinalizationRegistry-based resource guard assumes.Fix
Override
FinalizationRegistry.prototype.{register,unregister}with Bun host functions that call JSC'sregisterTarget/unregisterand additionally maintain a per-VMJSC::Strong<>root set onJSCTaskScheduler:register()adds the receiver on the first successful registration.unregister()drops it whenliveCount + deadCountreaches zero.runFinalizationCleanupdeferred-work task runs, the same zero check drops the root.A registry with no registrations is never rooted; a drained one becomes collectable on the next GC. The map is cleared in
Bun__JSCTaskScheduler__markShuttingDownso noStrongoutlives the event loop's final tick. The hooks are installed for both the main global andnode:vmglobals.Retention trade-off: a registry whose only remaining registration watches a target that never dies (e.g.
globalThis) is retained until VM shutdown. V8 would collect it. Matching V8 exactly requires either walking unmarkedJSFinalizationRegistrycells during marking (private access to the target list to tell "a target is dying this cycle") or changing JSC'sBytecodeGeneratorto save every generatorified local; the silent-guard failure this fixes is judged worse than the retained registry. The comment onrootFinalizationRegistryand a dedicated test document this.How did you verify your code works?
New
test/js/web/finalization-registry.test.ts(eight concurrent subprocess tests):cleaned == 0on the unpatched build)unregister()their entries / never register at all are all collected (not leaked by the new root)node:vmcontext delivers every callbackregister/unregisterargument validation and.lengthare unchangedUSE_SYSTEM_BUN=1 bun test ...: 5 fail withcleaned == 0/alive == 0.bun bd test ...: 8 pass.Adjacent PRs #30857 and #34270 touch the same
runPendingWorkblock for different reasons (exception reporting / worker termination); this change is additive to both.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 1
evidence per changed file