test: give the worker_threads tests in message-port-pipe.test.ts a debug/ASAN budget - #38055
test: give the worker_threads tests in message-port-pipe.test.ts a debug/ASAN budget#38055robobun wants to merge 2 commits into
Conversation
…bug/ASAN budget A node:worker_threads Worker takes ~2.7s to boot on a debug build (it compiles node:worker_threads, node:stream and node:console before the worker's code runs), so the four tests here that spawn one sit at or over the 5s default on debug builds no matter how little work they do; three of them time out on an unmodified debug build. Give those four tests a 60s timeout on debug/ASAN builds (release keeps the default) and run them concurrently. Restructure the concurrent-creation test so the workers wait until all of them have booted before churning, which is when the churn actually overlaps, and have each worker report its iteration count instead of a bare "done".
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 7:04 AM PT - Aug 13th, 2026
❌ @robobun, your commit c23bec6 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 38055That installs a local version of the PR into your bun-38055 --bun |
|
Status: reproduced and fixed, waiting on CI.
|
There was a problem hiding this comment.
LGTM — test-only timeout/concurrency adjustment following existing patterns.
What was reviewed:
test.concurrent.skipIfchaining and theisDebug || isASAN ? 60_000 : undefinedpattern are both established elsewhere intest/.- Race-test rewrite: the ready/go handshake attaches
once("message")listeners before posting on both sides, so no message is lost; each worker's reported count is asserted, so the halved iteration count can't silently no-op. - Assertions are unchanged or strengthened; release builds keep the default timeout via
undefined.
Extended reasoning...
Overview
This PR touches a single test file, test/js/web/workers/message-port-pipe.test.ts. It introduces a workerTimeout constant (60s on debug/ASAN, undefined on release), converts the four subprocess-spawning worker_threads tests to test.concurrent with that timeout, and rewrites the concurrent-MessageChannel race fixture so all five threads start churning together after a ready/go handshake instead of the main thread finishing before workers boot. No production code is touched.
Security risks
None. Test-only change; no new inputs, no network, no filesystem writes beyond what the existing tests already did.
Level of scrutiny
Low-to-medium. It is test infrastructure, but the repo's review guidance flags "don't raise per-test timeouts to make a slow test pass" and "never silently weaken an existing test", so I checked both. The timeout raise is justified by a fixed per-process floor (~4.3s of debug worker_threads bootstrap) that no iteration shrink can remove — the PR description documents this with measurements, and the same 60s debug/ASAN budget appears in worker-terminate-lifetime.test.ts, worker_heap_snapshot_gc.test.ts, broadcast-channel-worker-gc.test.ts, and string-decoder.test.js. The race test is strengthened, not weakened: it now asserts each worker churned exactly N channels (previously it only awaited a "done" string), and the synchronized start means the main thread's churn actually overlaps the workers' — the property the test exists to exercise.
Other factors
test.concurrent.skipIf(...)chaining is used in ~10 other test files, so the API composes as written.- Passing
undefinedas the timeout arg to keep the default is an existing idiom (e.g.string-decoder.test.js:382). - The handshake ordering is correct: workers register
parentPort.once("message")before posting "ready"; the main thread registers the secondnextMessagelistener for the count before posting "go";countsis created before the main-thread churn and awaited after. The leftoveronce("error")listeners from the firstnextMessagecall are harmless (8 total across 4 workers). test.concurrentis appropriate here since each test is an independentBun.spawnsubprocess with no shared state, matching the harness guidance for independent subprocess suites.
A worker that has returned from its entry script runs a full GC before it can receive a message, so starting the workers with postMessage staggered them by more than the few milliseconds the churn takes on a release build. Block each worker in Atomics.wait() inside its entry script instead and release all of them with one Atomics.notify() right before the main thread churns.
There was a problem hiding this comment.
LGTM — test-only timeout budget and test.concurrent for the four worker_threads subprocess tests, matching the established isDebug || isASAN ? 60_000 : undefined pattern.
What was reviewed:
- The Atomics barrier in the race test: workers block in
Atomics.wait(start, 0, 0)until the main thread stores+notifies after attaching the secondonce("message")listeners, so the count message can't race the listener. test.concurrent.skipIf(...)chaining and the third-argundefinedtimeout are both used elsewhere in the suite.- The race test's assertion is stronger than before (each worker must report N=1000; previously just "done"), so lowering the per-thread count from 2000 doesn't weaken coverage given all five threads now churn in the same window.
- The other three tests are unchanged aside from indentation,
test.concurrent, and the timeout arg.
Extended reasoning...
Overview
Test-only change to test/js/web/workers/message-port-pipe.test.ts. Four tests that spawn a subprocess containing a node:worker_threads Worker get (a) a 60s timeout on debug/ASAN builds via workerTimeout (undefined on release, so the default stays), (b) test.concurrent since each is an independent subprocess, and (c) the race test is restructured to boot all four workers, block them in Atomics.wait on a shared Int32Array, and release them together so the main thread and all workers churn MessageChannels in the same window. The race test now asserts each worker reports its full iteration count instead of a bare "done".
Security risks
None. No production code touched; no new inputs; subprocess fixtures are self-contained -e scripts with no filesystem or network access.
Level of scrutiny
Low-to-medium. It's a test infrastructure fix for local debug-build timeouts (CI already passes --timeout 90/270, so this only affects bun bd test). The one non-mechanical part — the Atomics barrier — is small and I verified the ordering: the second nextMessage listeners are attached before Atomics.store/Atomics.notify, so the worker's count message cannot arrive before the listener, and SharedArrayBuffer in workerData shares the backing store across the structured clone.
Other factors
test.concurrent.skipIfis an established chain (used in ~10 other test files).- The
isDebug || isASAN ? 60_000 : undefinedtimeout pattern already appears instring-decoder.test.jsand matchesworker_heap_snapshot_gc.test.ts/broadcast-channel-worker-gc.test.tscited in the PR. - Assertions in the three non-race tests are byte-identical to before (only re-indented under the new call form). The race test's assertion is strictly stronger (verifies
churned.every(n => n === N)), so the 2000→1000 reduction combined with synchronized start does not weaken the property under test — it improves the overlap the test exists to exercise. - No prior human review comments to address; CI build in progress.
Problem
bun bd test test/js/web/workers/message-port-pipe.test.tsfails withthis test timed out after 5000mson three tests, all unmodified on main: "concurrent MessageChannel creation across workers is race-free", "burst of postMessage across threads delivers every message in order" and "round-trip burst delivers in order with microtasks between each". The fourth worker test, "messages sent before worker online are delivered once it starts", passes at 4.6-4.7s.node:worker_threadsWorker, and on a debug build a worker_threads Worker takes ~2.7s from construction until its code runs (a webWorkertakes ~0.2s). The difference is the per-worker bootstrap from node:worker_threads: +48 Node.js tests passing — MessagePort, stdio, SHARE_ENV, exit codes, transfer semantics, postMessageToThread + inspector #31216 (src/js/node/worker_threads.ts:preload: ["node:worker_threads"]plussetupWorkerStdio, which requiresnode:consoleand constructs aConsole); compiling those builtins costs ~2.2s on a debug build. With ~0.4s of debug process startup and 1-2s of worker VM teardown at exit, a subprocess that spawns a single worker and does nothing else takes ~4.3s, so the 5s default leaves these tests no room for any work or any load on the machine. The tests were written in Replace MessagePort/BroadcastChannel registries with MessagePortPipe primitive #29937 before that bootstrap existed.test/expected-durations.json), and the CI runner passes--timeoutof 90s/270s anyway; the 5s default applies to localbun bd testruns, which is where this was hit.Fix
test/js/web/workers/message-port-pipe.test.ts: the four tests that spawn a worker_threads Worker get a 60s timeout on debug/ASAN builds (workerTimeout;undefinedon release, so release keeps whatever default is in effect) and run withtest.concurrent, since each one is an independent subprocess. 60s matchesworker-terminate-lifetime.test.tsandworker_heap_snapshot_gc.test.ts, which document the same debug/ASAN worker startup cost.Atomics.wait()on anInt32Arrayover aSharedArrayBufferpassed inworkerData, still inside its entry script; once all four have reported in, the main thread does oneAtomics.store+Atomics.notifyand churns itself, so all five threads create and close channels in the same window. It churns 1000 channels per thread instead of 2000, which is plenty once the windows actually coincide.src/jsc/web_worker.rsruns a full GC (run_gcafterworkerGlobalScopeStarted) before the first tick that can deliver a message, and on a release build that GC takes longer than the whole churn (measured 2-33ms of skew per worker against 3-6ms of churn), so releasing the workers withpostMessagestaggers them. Measured on a release build with a shared active-thread counter (not part of the test): the fixture on main never had all five threads churning at once in 40 runs; releasing viapostMessagegot there in about half the runs and in 4 of 20 some worker churned entirely alone; theAtomicsbarrier got there in 20 of 20. The test does not assert the overlap itself, since on a small CI runner the threads can legitimately be time-sliced apart.workerDataplumbing cannot silently turn the test into a no-op (verified by breaking it on purpose:expected every worker to churn 1000 channels, got [ 0, 0, 0, 0 ], exit 1).bun bd test test/js/web/workers/message-port-pipe.test.tson an unmodified debug build at bdb7382: 10 pass, 3 time out. With this change: 13 pass across 13 runs; the race test takes 5-13s depending on host load, so it would still time out at the default, and the file went from ~28s to ~14-17s.USE_SYSTEM_BUN=1 bun test(release): 10 pass, 3 skipped, as before. The barrier fixture was also run standalone on both builds:Atomics.waitis allowed in a worker, and the worker's "ready" message is delivered while it is blocked.Background
isDebug/isASAN(test/harness.ts) identify debug and sanitizer builds. Three of the four tests are already gated on them because the race they cover only shows up under a sanitizer; those tests never run on a release build.bun testapplies a 5s timeout to any test that does not pass its own; the third argument totest()sets one per test, and passingundefinedleaves the default in place. CI (scripts/runner.node.mjs) raises the default with--timeout, local runs do not.test.concurrentruns a test in parallel with the other concurrent tests of the file instead of one at a time.Atomics.wait(array, index, expected)blocks the calling thread until another thread callsAtomics.notifyon the same element (returning immediately if the value is no longerexpected), which makes aSharedArrayBufferelement a one-shot start gate: the waiting threads all wake from a singlenotify, with no event loop involved on either side.Timings from a debug build (linux-x64, 12 CPU cgroup)
Superseded first version
The first push released the workers with a
postMessage("go")fan-out after collecting "ready" messages. Review pointed out that on release-class builds the post-entry GC inweb_worker.rsstaggers delivery of that message by more than the churn takes, which the active-counter measurement above confirmed (5-way overlap in 10 of 20 runs, a lone thread in 4 of 20), so it was replaced with theAtomicsbarrier. The timeout,test.concurrentand count-reporting parts are unchanged from that version.[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file