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 23a222311795..5c08e91da471 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. @@ -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"; @@ -24,9 +26,14 @@ async function makeWorker() { return w; } +const iters = Number(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(); -const iters = Number(process.env.ITERS); +let completed = 0; +let firstPayloadChecked = false; for (let i = 0; i < iters; i++) { let stream; try { @@ -34,10 +41,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(); @@ -46,14 +52,31 @@ 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++; } 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 de939901550d..b7d1cb89827b 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,47 @@ 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, 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 +// 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,15 +52,21 @@ 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, }); } }, - 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, );