JSPropertyIterator: drop Ref<VM>; free on trap-in-window so worker ~VM runs

[robobun]

What

Terminating a worker while it is driving http2 requests sometimes leaks every native-backed cell that worker allocated: H2FrameParser (the lshpack state + HashMap<u32, Stream> backing), ImmediateObject, Blob, FileInternalReadableStreamSource. Seen in #34424's build 74745; #34563 works around the h2-specific allocations with a thread-exit sweep. The handoff that prompted this described it as MarkedSpace::lastChanceToFinalize / BlockDirectory::forEachBlock skipping IsoSubspace cells.

Cause

It isn't a sweep miss: for the one worker that leaks, ~VM never runs at all. Instrumenting WebWorker__teardownJSCVM showed that worker reaches the final vm.derefSuppressingSaferCPPChecking() with vm.refCount() == 2, so the deref leaves it at 1 and heap.lastChanceToFinalize() is never reached. Every other worker in the same run reached it with refCount() == 1 and cleanly destroyed.

The stray ref is a leaked JSPropertyIterator. Its C++ side held a Ref<JSC::VM>:

class JSPropertyIterator {
    RefPtr<JSC::PropertyNameArray> properties;
    Ref<JSC::VM> vm;
    ...
};

and the Rust wrapper obtained the raw pointer through from_js_host_call_generic:

let raw = from_js_host_call_generic(global_object, || {
    Bun__JSPropertyIterator__create(...)
})?;

Bun__JSPropertyIterator__create checks RETURN_IF_EXCEPTION after getPropertyNames, then allocates the iterator and returns. from_js_host_call_generic's own post-call check is scope.exception_including_traps(), which also handles traps. A parent's terminate() (VMTraps::fireTrap(NeedTermination)) landing in the handful of instructions between those two checks makes the Rust side observe a termination exception after create has already succeeded; ? propagates, the raw pointer is dropped (a no-op), and the heap-allocated JSPropertyIterator leaks its +1 VM ref.

http2's request() walks the headers object via JSPropertyIterator on every call, so a setImmediate-driven loop of requests re-enters that window every tick.

Per-run trace of the leaking worker:

spin-pre-loadEntry vmRefCount=1
shutdown-ENTRY     vmRefCount=2   propIterLive=1
pre-deref          vmRefCount=2
teardownJSCVM END  clientDataDtorDelta=0      (~JSVMClientData never reached)

Fix

• JSPropertyIterator stores JSC::VM& instead of Ref<JSC::VM>. The Rust owner is stack-scoped with a lifetime tied to its &JSGlobalObject, so the VM always outlives it; a strong ref serves no purpose and, when leaked, keeps a terminated worker's VM alive past teardown.
• JSPropertyIteratorImpl::init hoists the returned pointer out of the closure and frees it if the post-call check reports an exception, so the iterator itself no longer leaks either.

Verification

New ASAN-only test terminate() during native property iteration still runs the worker VM's finalizers in worker_threads.test.ts runs 6 rounds of 42 workers each terminated mid-request() under detect_leaks=1. Fail-before 3/3 runs (first failing iteration at 28s/49s/111s), pass-after 1/1 plus 0/30 LSan hits on the raw fixture. worker_threads.test.ts 92 pass; node-http2.test.js 305 pass / 6 skip.

The trap window is a few instructions wide and the test is probabilistic; 3/3 fail-before on this container does not guarantee the gate always fires, so the instrumentation trace above is the deterministic evidence.

Supersedes the workaround in #34563; related to #34448 (same "refcount > 1 at final deref" failure mode on the main-thread destructOnExit path).

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/worker_threads/worker_threads.test.ts

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b2a64775-eb01-4898-bc4b-1c8fcd322ea7

📥 Commits

Reviewing files that changed from the base of the PR and between ca67b8a and 3a716f6.

📒 Files selected for processing (1)

• test/js/node/worker_threads/worker_threads.test.ts

---

Walkthrough

Changes

The property iterator now cleans up native allocations when host-call exception handling fails, uses a non-owning VM reference, and adds an ASAN regression test that terminates workers during HTTP/2 property iteration.

Property Iterator Termination

Layer / File(s)	Summary
Iterator allocation and VM lifetime src/jsc/JSPropertyIterator.rs, src/jsc/bindings/JSPropertyIterator.cpp	Native iterator allocations are explicitly deinitialized on host-call errors, and JSPropertyIterator stores a non-owning VM reference used by VM inquiry logic.
Worker termination regression coverage test/js/node/worker_threads/*fixture.js, test/js/node/worker_threads/worker_threads.test.ts	HTTP/2 worker fixtures trigger termination during property iteration, while an ASAN-only test repeats the scenario and checks for leak signatures and successful exits.

Suggested reviewers: cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main fix: removing VM ownership from JSPropertyIterator and freeing the iterator on the trap window leak path.
Description check	✅ Passed	The description covers what changed, why it was needed, and how it was verified, matching the template's required intent.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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

[robobun]

^{Updated 1:46 AM PT - Jul 18th, 2026}

❌ @robobun, your commit 3a716f6 has 1 failures in Build #75181 (All Failures):

• test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js - SIGABRT on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 34574

That installs a local version of the PR into your bun-34574 executable, so you can run:

bun-34574 --bun

[github-actions]

This PR may be a duplicate of:

1. destructOnExit: release nested JSLockHolder VM refs so ~VM runs #34448 - Also fixes strong VM refs preventing ~VM from running during worker teardown (via JSLockHolder rather than JSPropertyIterator); same root cause and failure mode
2. node:http2: reclaim H2FrameParser native allocations at worker thread exit #34563 - Explicitly superseded by this PR; both address leaked native allocations when workers are terminated mid-dispatch

🤖 Generated with Claude Code

[claude]

I didn't find any bugs, but this changes VM refcounting semantics on a worker-teardown path, so it's worth a human look.

What was reviewed:

• Ref<JSC::VM> → JSC::VM&: confirmed the Rust JSPropertyIterator<'a> is lifetime-bound to &'a JSGlobalObject and freed in Drop, so the VM outlives every C++ iterator instance; auto& vm = iter->vm and &vm at the PropertySlot site both still bind correctly.
• JSPropertyIteratorImpl::init: the hoisted raw + post-call free matches call_check_slow's return_if_exception() → exception_including_traps() semantics; no double-free path (only freed on Err, otherwise ownership passes to the caller's Drop).
• Test fixture: ../../../leaksan.supp resolves to test/leaksan.supp (exists); pipes drained concurrently; bunEnv spread preserved.

Extended reasoning...

Overview

The PR fixes a leaked JSPropertyIterator (and through it a leaked Ref<JSC::VM>) that prevents ~VM from running when a worker's terminate() trap lands between the C++ RETURN_IF_EXCEPTION in Bun__JSPropertyIterator__create and the Rust caller's post-call trap check. Two independent fixes: (1) drop the strong Ref<JSC::VM> on the C++ JSPropertyIterator in favor of a raw JSC::VM&, and (2) hoist the returned raw pointer out of the from_js_host_call_generic closure so it can be freed if the post-call check reports an exception. Adds an ASAN-only probabilistic leak test driving 6×42 workers through http2 request loops under detect_leaks=1.

Security risks

None identified. No user-controlled input reaches new code paths; the change narrows a resource leak.

Level of scrutiny

High. This is JSC-binding memory-lifetime code on the worker VM teardown path — exactly the "most-blocked category" per REVIEW.md. Dropping a Ref<VM> for a raw reference is a lifetime-semantics change: I verified the Rust wrapper is stack-scoped with a 'a lifetime bound to &JSGlobalObject and that the C++ object is only ever created/destroyed through the Rust FFI (Bun__JSPropertyIterator__create/deinit), so the VM provably outlives every instance. But someone who owns the worker teardown / derefSuppressingSaferCPPChecking path should confirm this doesn't interact with any other VM-refcount assumption, and weigh in on whether this fully supersedes #34563's h2 thread-exit sweep.

Other factors

• The root-cause trace in the PR description is convincing and matches what I read in call_check_slow_at / return_if_exception → exception_including_traps.
• ~25 Rust files construct JSPropertyIterator; all go through the same init / Drop pair, so the ownership change is centralized.
• The new test is probabilistic with a 180s timeout and is ASAN-gated; the PR is upfront that fail-before is 3/3 but not guaranteed. That's acceptable for a narrow race, but a maintainer should confirm the timeout budget is OK for CI.
• No prior human reviews on the timeline.

[robobun]

Self-review addressed: the parent fixture now fails loudly on worker error or a non-terminate exit (instead of swallowing and passing vacuously), and the 6-spawn loop has a comment explaining it exists for fail-fast LSan on regression.

CI on build 75181 is complete. The new test passed on the x64-asan lane (the only one it runs on). All failures are either [flaky] (passed on retry: complex-workspace, cpu-prof, no-orphans, require-cache, proxy-stress-errors, node-net, test-http2-connect-method-extended-cant-turn-off, test-repl-close, valkey, html-rewriter-leak, napi, 20144) or [pre-existing] (test-worker-message-port-transfer-terminate.js, the known termination-during-preload JSC assertion, also red on main). None touch this diff.

The sibling URL::from_js / FetchHeaders::create_from_js trap-window leaks noted in review are intentionally excluded (no Ref<VM>, no finalizer cascade, not on any path the fixture exercises) and handed off separately; see the resolved thread on JSPropertyIterator.rs.

Ready for a maintainer.

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@test/js/node/worker_threads/worker_threads.test.ts`:
- Line 1786: Update the stderr assertion in the worker-thread leak test to
reject any LeakSanitizer report by checking for the generic “LeakSanitizer”
marker, rather than relying on the symbol-name regex. Preserve the existing
negative assertion structure while making the sanitizer marker the primary
invariant.

🪄 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: 9f281d47-fe79-43f5-80e0-f16ad6e6f7b7

📥 Commits

Reviewing files that changed from the base of the PR and between f5b73a0 and ca67b8a.

📒 Files selected for processing (5)

• src/jsc/JSPropertyIterator.rs
• src/jsc/bindings/JSPropertyIterator.cpp
• test/js/node/worker_threads/worker-terminate-propiter-parent-fixture.js
• test/js/node/worker_threads/worker-terminate-propiter-worker-fixture.js
• test/js/node/worker_threads/worker_threads.test.ts