From 2988fea17d082026a68cde920e9f10e72c5a63c6 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 5 Aug 2026 10:24:54 +0000 Subject: [PATCH 1/3] test: shrink worker_heap_snapshot_gc release workload, assert fixture completion count The 15x300 release workload was sized as a probabilistic crash guard for the #30185 cross-thread HandleSet race. That detection relied on the GC controller collecting every ~16ms of event-loop activity, which made the parent's strong-handle scans overlap the worker's task teardown. #35356 removed those per-tick collections, and the reintroduced bug now survives the full 15x300 workload with zero detections in 18000 iterations (it was caught at ~60% per process before). The loop is kept as a functional check of the cross-VM round-trip at 15x25 in release; the ASAN lane keeps 100 iterations for memory-bug coverage. The fixture now validates ITERS and reports the completed iteration count, and the test asserts the exact count so a fixture that silently exits early fails instead of passing. --- .../heap-snapshot-gc-race-fixture.js | 16 ++++-- .../worker_heap_snapshot_gc.test.ts | 55 +++++++++++-------- 2 files changed, 44 insertions(+), 27 deletions(-) diff --git a/test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js b/test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js index 23a22231179..d4b66c0887d 100644 --- a/test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js +++ b/test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js @@ -1,4 +1,4 @@ -// Stress getHeapSnapshot() against a parent-thread full GC. +// Exercise getHeapSnapshot() round-trips against parent-thread full GCs. // // Each getHeapSnapshot() round-trip used to capture a parent-VM // Strong by value in a lambda that ran on the worker thread. @@ -27,6 +27,9 @@ async function makeWorker() { let worker = await makeWorker(); const iters = Number(process.env.ITERS); +if (!Number.isInteger(iters) || iters <= 0) throw new Error(`invalid ITERS: ${JSON.stringify(process.env.ITERS)}`); + +let completed = 0; for (let i = 0; i < iters; i++) { let stream; try { @@ -34,10 +37,9 @@ for (let i = 0; i < iters; i++) { } catch (e) { // On some CI platforms the worker has been observed to exit on its own // after a few hundred heap snapshots — that surfaces here as a clean - // ERR_WORKER_NOT_RUNNING rejection, not the process-level segfault this + // ERR_WORKER_NOT_RUNNING rejection, not the process-level corruption this // fixture is looking for. Recreate the worker and keep going so the - // overall round-trip count (and thus the number of race opportunities - // against the parent VM's HandleSet) is preserved. + // overall round-trip count is preserved. if (e?.code === "ERR_WORKER_NOT_RUNNING") { await worker.terminate().catch(() => {}); worker = await makeWorker(); @@ -53,7 +55,11 @@ for (let i = 0; i < iters; i++) { Bun.gc(true); stream.on("data", () => {}); await new Promise(resolve => stream.once("end", resolve)); + completed++; } await worker.terminate(); -console.log("ok"); +// The completed count lets the test reject a fixture that silently exited +// early (e.g. ITERS lost in env plumbing would otherwise skip the loop and +// still print a bare "ok"). +console.log(`ok ${completed}`); diff --git a/test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts b/test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts index de939901550..c0ef7aad1c6 100644 --- a/test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts +++ b/test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts @@ -3,37 +3,46 @@ import { bunEnv, bunExe, isASAN, isDebug, isIntelMacOS, isWindows } from "harnes import { join } from "node:path"; // The getHeapSnapshot() round-trip must never let the worker thread touch -// the parent VM's HandleSet. Before the fix this crashed with a segfault at -// 0x10 inside the "Sh" (Strong Handles) marking constraint — a parent-VM -// Strong was captured by value in a lambda that ran on the worker -// thread, and Strong's copy/dtor mutated HandleSet::m_strongList without -// the parent VM's lock while the collector was iterating it. +// the parent VM's HandleSet. Before the fix (#30185) this corrupted +// HandleSet::m_strongList — a parent-VM Strong was captured by +// value in a lambda that ran on the worker thread, and Strong's copy/dtor +// mutated the list without the parent VM's lock while the collector was +// iterating it, faulting at 0x10 inside the "Sh" marking constraint (or +// livelocking on the torn list). // -// The race window is a handful of instructions after each snapshot -// completes, so no single run is guaranteed to hit it; we run the fixture -// repeatedly in release and fail if any attempt crashes. Debug and ASAN -// builds are several times slower per heap snapshot, so they get a reduced -// workload as a functional check — plain release CI is where this guards -// against regressions. -// Skipped on Windows and Intel (x64) macOS: this branch's always-on per-worker -// stdio path adds per-spawn overhead that a 15x300-snapshot stress exceeds on -// those builders. The race it guards is platform-agnostic and still covered on -// Linux and Apple-Silicon macOS. +// This test used to run 15x300 iterations in release as a probabilistic +// crash guard. That sizing dated from when the GC controller ran a +// collection every ~16ms of event-loop activity, which is what made the +// parent's strong-handle scans overlap the worker's task teardown; #35356 +// removed those per-tick collections, and with them the overlap: the +// original bug, reintroduced, survives 15x300 with no crashes (0 detections +// in 18k iterations, vs ~60% per process before #35356). With no scheduling +// coincidence left to amplify, the loop is kept as a functional check of the +// cross-VM round-trip under concurrent processes and explicit parent GCs: +// promises settle, streams drain intact, workers terminate cleanly. +// +// The ASAN lane keeps a larger iteration count: ASAN can catch ordinary +// memory bugs in the round-trip/stream machinery that the plain-release +// lanes cannot. +// +// Skipped on Windows and Intel (x64) macOS: the always-on per-worker stdio +// path adds per-spawn overhead that this stress exceeds on those builders. +// The code path is platform-agnostic and still covered on Linux and +// Apple-Silicon macOS. test.skipIf(isWindows || isIntelMacOS)( "worker.getHeapSnapshot() does not race the parent VM's Strong Handles list under GC", async () => { - const slow = isDebug || isASAN; - const attempts = slow ? 1 : 15; - const iters = isDebug ? "5" : slow ? "100" : "300"; + const attempts = isDebug || isASAN ? 1 : 15; + const iters = isDebug ? 5 : isASAN ? 100 : 25; const fixture = join(import.meta.dir, "heap-snapshot-gc-race-fixture.js"); // The attempts are independent processes with no shared state, so run them - // all concurrently; the race being guarded is intra-process. + // all concurrently; the behavior being exercised is intra-process. const results = await Promise.all( Array.from({ length: attempts }, async (_, i) => { await using proc = Bun.spawn({ cmd: [bunExe(), fixture], - env: { ...bunEnv, ITERS: iters }, + env: { ...bunEnv, ITERS: String(iters) }, stdout: "pipe", stderr: "pipe", }); @@ -42,10 +51,12 @@ test.skipIf(isWindows || isIntelMacOS)( }), ); for (const result of results) { - // One assertion per attempt so a crash shows stdout/stderr/signal together. + // One assertion per attempt so a failure shows stdout/stderr/signal + // together. The "ok " stdout proves the fixture ran every + // iteration rather than exiting early. expect(result).toEqual({ attempt: result.attempt, - stdout: "ok\n", + stdout: `ok ${iters}\n`, stderr: "", exitCode: 0, signalCode: null, From 2ca44ba919c4157ac734532eebce9684bf474f45 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 5 Aug 2026 11:17:30 +0000 Subject: [PATCH 2/3] test: finish the comment truth-pass, verify stream payloads, drop the stale 120s timeout Review follow-ups: the fixture header now describes the current reqId-map design (#31216) instead of the superseded raw-pointer fix, the mid-loop comment no longer claims a GC/teardown overlap this PR measured to be gone, every iteration fails on an empty snapshot stream (one payload per process is parsed as JSON), and the 120s release timeout arm sized for the old 15x300 workload collapses into a single 60s ceiling. --- .../heap-snapshot-gc-race-fixture.js | 30 ++++++++++++++----- .../worker_heap_snapshot_gc.test.ts | 9 ++++-- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js b/test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js index d4b66c0887d..5e562fa77ce 100644 --- a/test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js +++ b/test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js @@ -10,8 +10,10 @@ // SentinelLinkedList node and fault reading HandleNode::m_value at // (nullptr + 0x10). // -// The fix heap-allocates the Strong once on the parent thread and passes -// only the raw pointer across, so the worker thread never touches the +// The original fix (#30185) heap-allocated the Strong on the parent thread +// and passed only a raw pointer across. Since #31216 the promise is held in +// a parent-side map keyed by reqId (Worker::m_pendingCrossVMRequests) and +// only the id crosses threads, so the worker thread never touches the // parent VM's HandleSet. import { Worker } from "node:worker_threads"; @@ -30,6 +32,7 @@ const iters = Number(process.env.ITERS); if (!Number.isInteger(iters) || iters <= 0) throw new Error(`invalid ITERS: ${JSON.stringify(process.env.ITERS)}`); let completed = 0; +let firstPayloadChecked = false; for (let i = 0; i < iters; i++) { let stream; try { @@ -48,13 +51,26 @@ for (let i = 0; i < iters; i++) { } throw e; } - // Right now the worker thread has posted the result (resolving the await - // above) but may still be destroying its outer EventLoopTask. Force a - // synchronous full GC so the "Sh" constraint walks m_strongList while - // the worker would have been removing a node from it. + // Kept from the original repro shape: a synchronous full GC right after + // the round-trip resolves, where the pre-fix worker thread might still be + // tearing down its task. Post-#35356 GC cadence this almost never + // coincides with the worker-side teardown (see the test header), so it is + // an interleaving exercise, not a reliable race trigger. Bun.gc(true); - stream.on("data", () => {}); + let bytes = 0; + const chunks = firstPayloadChecked ? null : []; + stream.on("data", chunk => { + bytes += chunk.length; + chunks?.push(chunk); + }); await new Promise(resolve => stream.once("end", resolve)); + if (bytes === 0) throw new Error(`empty heap snapshot stream on iteration ${i}`); + if (chunks) { + // Parse one payload per process to prove the round-trip carries a real + // snapshot; later iterations only need the cheap non-empty check. + JSON.parse(Buffer.concat(chunks).toString()); + firstPayloadChecked = true; + } completed++; } diff --git a/test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts b/test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts index c0ef7aad1c6..b7d1cb89827 100644 --- a/test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts +++ b/test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts @@ -19,7 +19,8 @@ import { join } from "node:path"; // in 18k iterations, vs ~60% per process before #35356). With no scheduling // coincidence left to amplify, the loop is kept as a functional check of the // cross-VM round-trip under concurrent processes and explicit parent GCs: -// promises settle, streams drain intact, workers terminate cleanly. +// promises settle, every stream delivers a non-empty payload (parsed as JSON +// once per process), workers terminate cleanly. // // The ASAN lane keeps a larger iteration count: ASAN can catch ordinary // memory bugs in the round-trip/stream machinery that the plain-release @@ -63,5 +64,9 @@ test.skipIf(isWindows || isIntelMacOS)( }); } }, - isDebug || isASAN ? 60_000 : 120_000, + // One explicit ceiling for every lane: the debug/ASAN run needs more than + // the local 5s default (~20s), and a regression of the guarded race can + // present as a livelock, so the timeout is the time-to-red for hangs. The + // old 120s release arm was sized for the 15x300 workload. + 60_000, ); From 3f8663f82f55a9a8f0b97c980ee9d4ddebc2d732 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 5 Aug 2026 11:25:55 +0000 Subject: [PATCH 3/3] test: reject unsafe-integer ITERS before creating the worker --- .../node/worker_threads/heap-snapshot-gc-race-fixture.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js b/test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js index 5e562fa77ce..5c08e91da47 100644 --- a/test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js +++ b/test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js @@ -26,10 +26,11 @@ async function makeWorker() { return w; } -let worker = await makeWorker(); - const iters = Number(process.env.ITERS); -if (!Number.isInteger(iters) || iters <= 0) throw new Error(`invalid ITERS: ${JSON.stringify(process.env.ITERS)}`); +if (!Number.isSafeInteger(iters) || iters <= 0) + throw new Error(`invalid ITERS (expected a positive integer): ${JSON.stringify(process.env.ITERS)}`); + +let worker = await makeWorker(); let completed = 0; let firstPayloadChecked = false;