test: make common/gc.js onGC() honour upstream's setImmediate contract - #34176
cirospaciari wants to merge 4 commits into
Conversation
…gistry timing test-net-connect-memleak.js and test-tls-connect-memleak.js fail intermittently on the alpine/musl x64 lanes. Both are verbatim upstream Node tests that call globalThis.gc() and then assert, inside the very next setImmediate(), that their onGC() listener has fired. Upstream can rely on that because its onGC() is built on an async_hooks destroy hook, and it documents the resulting contract: "A full setImmediate() invocation passes between a global.gc() call and the listener being invoked." Bun's common/gc.js is an adapted harness, not a verbatim port, and it implements onGC() with a FinalizationRegistry because Bun's async_hooks.createHook only emits init events. FinalizationRegistry cannot honour that contract: JSC schedules cleanup callbacks through DeferredWorkTimer, which Bun drains via the event loop's concurrent task queue, while setImmediate() has its own separate queues. Neither queue is ordered against the other, so the callback can land after the observing setImmediate(). Nothing leaks in these tests -- the listener is removed and the object is collected; only the notification arrives too late. Deliver listeners off WeakRef instead, which gc() clears synchronously, so a sweep run right after an explicit gc() sees the collection with no queue in between. The FinalizationRegistry stays for the two callers that rely on a natural GC and never call gc() at all (test-http-client-leaky-with-double-response, test-http-server-keepalive-req-gc); a pending-set guard makes each listener fire exactly once no matter which path gets there first. Also drops the now-obsolete LINUX-X64-MUSL quarantine of test-tls-connect-memleak.js, whose stated cause was exactly this ordering. Verified with the debug build: all 10 files that load common/gc.js pass 20/20 each in CI configuration. To show the delivery no longer depends on the deferred-work queue, the gc()-driven tests were also run with FinalizationRegistry stubbed out so its callbacks never fire: they pass on this change and fail on main.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
| var finalizationRegistry = new FinalizationRegistry(heldValue => { | ||
| heldValue.ongc(); | ||
| }) | ||
| // Upstream implements onGC() with an async_hooks destroy hook, which gives it |
There was a problem hiding this comment.
Node's onGC() (test/common/gc.js @ v26.3.0) is built on async_hooks:
const gcTrackerMap = new WeakMap();
const gcTrackerTag = 'NODE_TEST_COMMON_GC_TRACKER';
function onGC(obj, gcListener) {
const async_hooks = require('async_hooks');
const onGcAsyncHook = async_hooks.createHook({
init: common.mustCallAtLeast(function(id, type) {
if (this.trackedId === undefined) {
assert.strictEqual(type, gcTrackerTag);
this.trackedId = id;
}
}),
destroy(id) {
if (id === this.trackedId) {
this.gcListener.ongc();
onGcAsyncHook.disable();
}
},
}).enable();
onGcAsyncHook.gcListener = gcListener;
gcTrackerMap.set(obj, new async_hooks.AsyncResource(gcTrackerTag));
obj = null;
}It creates an AsyncResource held only by a WeakMap keyed on obj. When V8 collects obj the AsyncResource becomes unreachable, V8 collects it, and its native finalizer calls EmitDestroy, which Node drains through env->SetImmediate on the next loop iteration. That's where the documented guarantee in their JSDoc comes from: "A full setImmediate() invocation passes between a global.gc() call and the listener being invoked."
Bun can't use that path: async_hooks.createHook here only delivers init (for TickObject) and warns that destroy is never called (src/js/node/async_hooks.ts:362). So there's no destroy event to hook. The WeakRef sweep after an explicit gc() is the closest mechanism that gives the same "observable by the next setImmediate" ordering, and the FinalizationRegistry stays for the two callers that never call gc() at all.
There was a problem hiding this comment.
@robobun check cirospaciari async hooks PR to see if it would solve this
There was a problem hiding this comment.
Checked #31825. It wouldn't solve this on its own.
That PR's src/js/node/async_hooks.ts diff covers AsyncLocalStorage (run/disable restore semantics, defaultValue, name, withScope) and AsyncResource.bind() parity, but it doesn't touch createHook at all. The destroy hook still never fires; the PR's own vendored test-eventemitter-asyncresource.js adds a comment saying exactly that ("Bun does not implement createHook event [delivery]").
Node's onGC() needs two things that remain missing after #31825:
createHook({ destroy })actually deliveringdestroyevents.AsyncResourcehaving a native finalizer that callsemitDestroywhen GC collects it. In Node theAsyncResourceis a nativeAsyncWrap; V8 runs its C++ destructor, which queuesEmitDestroy. In BunAsyncResourceis a plain JS class with no JSC finalizer, so nothing fires on collection even if (1) were wired up.
So even with #31825 merged, common/gc.js still needs a non-async_hooks mechanism, and the WeakRef sweep here is that mechanism. If/when Bun grows a native-backed AsyncResource with GC-driven destroy, this file could switch to upstream's verbatim implementation and drop the WeakRef path.
WalkthroughChangesGC listener timing and test expectations
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/test/common/gc.js`:
- Around line 159-167: Update flushGCListeners to isolate each
fireGCListener(entry) invocation so an exception from one ongc callback does not
abort the loop. Catch the per-entry exception, continue processing all remaining
collected entries from the snapshot, and preserve the existing pending-listener
cleanup behavior handled by fireGCListener.
🪄 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: 5b835af0-7395-4d80-a657-e05c6ceadd36
📒 Files selected for processing (2)
test/expectations.txttest/js/node/test/common/gc.js
💤 Files with no reviewable changes (1)
- test/expectations.txt
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues with the implementation, but since the author explicitly flags the alpine 3.23 x64 / x64-baseline lanes as the real verification (and the fix un-quarantines test-tls-connect-memleak.js on musl), a human should confirm those lanes are green before merging.
What was reviewed:
- Exactly-once delivery guard via
pendingListeners.delete()return value — both the WeakRef sweep and the FinalizationRegistry callback route throughfireGCListener, andunregisterprevents a late FR callback. entryholds only aWeakRefto the tracked value, sopendingListenersdoesn't keep targets alive;flushGCListenersiterates a snapshot so mutation during iteration is safe.- All 9
onGCcall sites intest/js/node/test/parallel/— the two natural-GC callers still get delivery via the retained FinalizationRegistry path; thegc()wrapper preserves args/return and only installs whengcexists.
Extended reasoning...
Overview
This PR modifies only test infrastructure: test/js/node/test/common/gc.js (Bun's adapted — not verbatim — Node test harness) and removes one [ LINUX-X64-MUSL ] quarantine entry from test/expectations.txt. No runtime code is touched. The change makes onGC() deliver notifications via a WeakRef sweep triggered synchronously after every explicit gc() call, in addition to the existing FinalizationRegistry path, so that vendored Node tests asserting collected === true inside the very next setImmediate() after gc() no longer depend on JSC's DeferredWorkTimer / concurrent-task-queue ordering.
Security risks
None. This is test-harness JavaScript that only runs inside vendored Node compatibility tests. It wraps globalThis.gc at module load, but only for the ~10 test files that require('../common/gc'), each of which runs as its own process.
Level of scrutiny
Medium. The logic is small and self-contained, and the exactly-once guard (Set.delete returning false short-circuits the second delivery path, plus finalizationRegistry.unregister(entry)) is straightforward to verify. I traced all 9 onGC callers and confirmed the entry object never strongly references the tracked value, flushGCListeners iterates a snapshot copy, and the gc wrapper is a transparent pass-through. The mustNotCall case in test-common-gc.js (onGC(process, ...)) is handled correctly since deref() stays non-undefined for a live object.
Other factors
The reason I'm deferring rather than approving is the PR description's own caveat: the flake reproduces only on musl, the author could not reproduce it locally on glibc, and states that "the alpine 3.23 x64 + x64-baseline lanes on this PR are the real verification." The PR also removes the musl quarantine for test-tls-connect-memleak.js. Given the build was still in progress at review time and the correctness claim explicitly hinges on those specific CI lanes, a human should eyeball the alpine results before merging — if they're green, this looks ready to go.
…reason CI (build 73000) shows the alpine x64 and x64-baseline failure survives the onGC() change, so the FinalizationRegistry delivery ordering was not its cause. Put the quarantine back. The run is still informative. onGC() now fires listeners synchronously from inside gc(), so a late cleanup callback can no longer be the explanation: collected === false means the object is genuinely still reachable when gc() returns on musl x64. Rewrite the comment to say that instead of the FinalizationRegistry theory it used to assert, and record what rules that theory out.
cirospaciari
changed the title
There was a problem hiding this comment.
LGTM — test-harness-only hardening with no runtime impact.
Checked that the pendingListeners Set + fireGCListener guard delivers each listener exactly once whichever path (WeakRef sweep or FinalizationRegistry) arrives first, and that entries hold only a WeakRef so tracked values remain collectable.
Checked the globalThis.gc wrapper is scoped to processes that load common/gc.js, is idempotent via the module cache, and that gcUntil in the same file picking up the wrapped gc() is harmless.
Confirmed the expectations.txt change is comment-only — the [ LINUX-X64-MUSL ] … [ FLAKY ] entry is preserved, so runner behaviour is unchanged.
The stale "Returns false" comment in fireGCListener was noted and treated as a nit.
Extended reasoning...
Overview
Two files touched, both test infrastructure:
test/js/node/test/common/gc.js— reimplementsonGC()(Bun's adapted, non-verbatim harness helper) to add aWeakRef-based synchronous sweep inside a wrappedglobalThis.gc(), alongside the existingFinalizationRegistrypath. ApendingListenersSet provides exactly-once delivery. ~70 lines, mostly explanatory comment.test/expectations.txt— rewrites the comment block for the existingtest-tls-connect-memleak.jsmusl quarantine to reflect what CI build 73000 proved (object genuinely uncollected on musl, not FR-callback timing). The quarantine entry itself — modifier, path, expectation — is unchanged.
No runtime, native, or bundled-JS code is touched. Blast radius is the ~11 vendored Node tests that require('../common/gc').
Security risks
None. Test-harness JavaScript only; no user-facing surface, no untrusted input, no auth/crypto/permissions paths.
Level of scrutiny
Low-to-moderate. This is shared test infrastructure, so a mistake could quietly break or false-pass ~11 vendored Node tests, but it cannot affect the shipped binary. The mechanism was traced end-to-end:
entry = { ref: new WeakRef(value), ongc }— no strong reference to the tracked value;pendingListenerskeeps only the entry (WeakRef + callback) alive.finalizationRegistry.register(value, entry, entry)usesentryas both held-value and unregister token, sounregister(entry)infireGCListenercleanly removes the FR registration when the WeakRef path wins.pendingListeners.delete(entry)returningfalseis the exactly-once guard for whichever path arrives second — correct for both orderings.flushGCListenerssnapshots[...pendingListeners]before iterating, so the in-loopdeleteis safe.- The
globalThis.gcwrapper is installed once at module load (module cache), only whengcalready exists (i.e.,--expose-gc), forwardsthis/args and return value, and itsflushGCListeners()is a no-op when nothing is registered — so othergc()callers in the same process (includinggcUntilin this file) are unaffected beyond the intended flush.
The PR description documents 200/200 passes across all consumers under the CI config, plus a differential test with FR callbacks stubbed out showing the WeakRef path is now sufficient for explicit-gc() callers.
Other factors
- The one raised concern (CodeRabbit: a throwing
ongc()aborts the flush loop) was rebutted and resolved — cleanup happens before the call, upstream's contract is thatongcmust not throw, and no current caller does. - The reviewer question ("what node.js do here?") was answered in-thread; no outstanding requests.
- The stale "Returns false" comment on
fireGCListeneris a cosmetic nit already surfaced this run; not blocking. expectations.txtchange is documentation-only for the runner (only the trailing#comment differs on the entry line), so no coverage is added or removed.
|
Build #73002: alpine 3.23 x64 and x64-baseline are green (the stated verification for this harness change). All The one red is |
…ss-race # Conflicts: # test/expectations.txt # test/js/node/test/common/gc.js
|
This PR is now a no-op ( #32630 landed on main at cc40d78 with a more complete version of the same Nothing left to land here; can be closed. |
|
Closing: superseded by #32630 (see comment above). The branch has no diff against main after cc40d78 landed the more complete |
2 participants
What this does
Reimplements
onGC()in the vendored-Node test harness (test/js/node/test/common/gc.js) so it honours the contract the upstream tests are written against, and corrects the recorded root cause of thetest-tls-connect-memleak.jsmusl quarantine, which was wrong.No runtime code changes. No vendored test file is touched. No expectations entry is added or removed.
The contract
test-net-connect-memleak.jsandtest-tls-connect-memleak.jsare verbatim upstream Node v26.3.0 files. They callglobalThis.gc()and then assert, inside the very nextsetImmediate(), that theironGC()listener has already fired.Upstream can rely on that because its
onGC()uses an async_hooksdestroyhook, and documents the result: "A full setImmediate() invocation passes between a global.gc() call and the listener being invoked."Bun's
common/gc.jsis an adapted harness, not a verbatim port, and implementsonGC()with a FinalizationRegistry — because Bun'sasync_hooks.createHookonly emitsinit(src/js/node/async_hooks.ts:362: "hooks can still be created but will never be called"). Upstream's mechanism simply isn't available.FinalizationRegistry cannot honour that contract:
DeferredWorkTimer.Bun__queueJSCDeferredWorkTaskConcurrently(src/jsc/bindings/JSCTaskScheduler.cpp:54) — the event loop's concurrent task queue.setImmediate()has its own two separate queues (src/jsc/event_loop.rs:55).Nothing orders those against each other, so a cleanup callback is free to land after the
setImmediate()observing it. That is a real latent hazard for all 9onGC()callers, independent of the bug I set out to fix.What changed
onGC()now delivers offWeakRef, whichgc()clears synchronously (verified:deref()returnsundefinedimmediately aftergc()). A sweep run right after an explicitgc()observes the collection with no queue in between.The FinalizationRegistry is kept, because two callers rely on a natural GC and never call
gc()at all (test-http-client-leaky-with-double-response,test-http-server-keepalive-req-gc). Whichever path arrives first wins; a pending-set guard fires each listener exactly once — required, as these tests usemustCall().Rejected alternatives, after reading the implementations: JSC has no
FinalizationRegistry.prototype.cleanupSome;bun:jsc'sreleaseWeakRefsis not an FR drain (bindings.cpp:4541— it'svm->finalizeSynchronousJSExecution(), which only bumps the weak-ref version);drainMicrotasksdoesn't touch the concurrent queue.What CI proved (and disproved)
I originally claimed this fixed the alpine flake and dropped the quarantine. Build 73000 failed on
alpine 3.23 x64andx64-baselinewithtest-tls-connect-memleak.jsstill assertingcollected === false. So I restored the quarantine (second commit).That result is worth more than the original hypothesis. Because
onGC()now fires synchronously insidegc()— verified live on the CI path, not just locally — a late cleanup callback can no longer be an explanation.collected === falsenow means one thing only: the object is genuinely still reachable whengc()returns on musl x64.So main's existing comment ("JSC FinalizationRegistry callback delivery vs setImmediate timing on musl x64") is wrong, and this PR replaces it with what's now provable, including what rules the old theory out. Best remaining explanation for the actual failure: JSC's conservative stack scanning retaining the connect closure under musl's stack layout — which neither the harness nor the test can control. Marked unconfirmed in the comment; it has never reproduced off musl.
Notably the net twin passes on alpine while the TLS one fails, despite being structurally identical.
How we know the harness change is sound
Debug build throughout.
All 10 files that load
common/gc.jspass 20/20 each (200/200) in CI configuration (bun test --config=bunfig.node-test.toml <abs path>), including both natural-GC callers — the FinalizationRegistry path still works.The ordering dependency is gone. With
FinalizationRegistrystubbed so its callbacks never fire:net-connect-memleaktls-connect-memleakcommon-gcOn main the FR is the only delivery path. Here it is no longer load-bearing for any test that calls
gc().Wrapper confirmed installed under the real runner path, not just under a bare
--expose-gc.test-v8-serialize-leakfails 10/10 on this branch and 10/10 on main — pre-existing, usesgcUntil/RSS, never callsonGC(). Untouched.Developed on glibc; the flake is musl-only and I could not reproduce it locally (30/30 clean on main, 40/40 under 8x CPU load, no musl container available).
After merge
No flake is fixed. What lands:
onGC()stops depending on an undefined queue ordering for all 9 callers, and the quarantine explains the failure correctly instead of pointing at a mechanism that has now been ruled out — so the next person to look attest-tls-connect-memleak.jsstarts from the object not being collected, not from FinalizationRegistry timing.