test(worker_threads): keep worker_destruction.test.ts under the default timeout on debug builds - #37460
test(worker_threads): keep worker_destruction.test.ts under the default timeout on debug builds#37460robobun wants to merge 2 commits into
Conversation
…orkload on debug builds worker_thread_check.ts now takes RUN_COUNT and CONCURRENCY from the environment (defaulting to the previous 5 x 10), and the test passes 1 x 3 on debug builds, where a worker_threads Worker takes ~1.7s to come online and the full workload took ~30s per case against the 5s default timeout. Release builds, which is what CI runs, keep 5 x 10. The test also asserts one "Spawned N workers" line per round so the counts the fixture actually ran are checked.
|
Warning Review limit reached
Next review available in: 10 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 (2)
Comment |
|
Status: reproduced on a debug+ASAN build of main (54d6d16): |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only workload reduction gated on isDebug, release/CI paths unchanged.
What was reviewed:
bunRunenv merge and stdout.trim()— the new stdout assertion splits a trimmed string, so the line count matchesRUN_COUNTexactly with no trailing empty element.- Fixture's
console.logoutput shape vs. the/ RSS \\d+ MB$/strip — matches, including RSS = 0. bunEnv/bunExeimports are still live (used by the fourth test).
Extended reasoning...
Overview
Two test-only files. worker_thread_check.ts now reads RUN_COUNT and CONCURRENCY from env with defaults matching the previous hardcoded 5×10. worker_destruction.test.ts passes 1×3 when isDebug is set and 5×10 otherwise, and adds a stdout assertion that the fixture printed exactly RUN_COUNT "Spawned N workers" lines. No production code is touched; release builds (all CI lanes) run identical work to before.
Security risks
None. This is a test fixture parametrization with no auth, network, crypto, or user-facing surface.
Level of scrutiny
Low. It follows the established isDebug-gated repetition-count pattern used elsewhere in the suite (e.g. the sibling worker_heap_snapshot_gc.test.ts), shrinks the workload rather than raising the timeout per REVIEW.md guidance, and does not weaken coverage — the new stdout assertion is strictly stronger than the prior exit-code-only check and guards against a misconfigured no-op run.
Other factors
I confirmed against test/harness.ts that bunRun's second argument is spread over bunEnv (so the two env vars reach the child), that result.stdout is pre-trimmed (so split("\\n") yields exactly RUN_COUNT entries), and that toSpawn() already asserts exit 0 + empty stderr. The fixture's console.log output is space-joined args, so the / RSS \\d+ MB$/ regex correctly strips the suffix. The PR description documents verification on both debug (previously timing out, now ~3s/case) and release (unchanged 5×10 passing through the new assertion), and robobun independently reproduced both. The bug-hunting pass found nothing.
|
Updated 7:28 AM PT - Aug 11th, 2026
✅ @robobun, your commit 910faac4258c9ff28ab62b572495b94a341b2134 passed in 🧪 To try this PR locally: bunx bun-pr 37460That installs a local version of the PR into your bun-37460 --bun |
One round of three workers per case is ~29s of CPU for the file, which still times out on 4 and 6 core machines; one worker per case is ~15s and passes on 4 cores with over a second to spare.
|
Pushed 910faac, which drops the debug workload from one round of 3 workers per case to one round of 1, and updated the description to match. Timing the first version pinned to fewer cores showed the file is CPU bound rather than latency bound: one worker_threads spawn + terminate is ~2.4s of CPU on a debug+ASAN build, so 3 per case across the three concurrent cases is ~29s of CPU per file, which still timed out 4 of 4 on 4 cores and 1 to 3 tests per run on 6 cores (1 x 2 is ~22s and still fails on 4 cores). One per case is ~15s of CPU and passes pinned to 4 cores at 3.2 to 4.0s per test, 2.7 to 3.1s unpinned. The fix this fixture was added with (#11494) is per-VM shutdown guards, so taking each of the three teardown paths once, in three concurrent processes, is the debug-build functional check; the repetition stays on release, which is what CI runs. The comment in the test now states that cost model so the counts are not bumped back up later. Release builds are unaffected by this commit. |
There was a problem hiding this comment.
LGTM — test-only workload reduction gated on isDebug, release/CI paths unchanged.
What was reviewed
bunRunsignature and return: second arg spreads overbunEnv,stdoutis.trim()ed, sosplit("\n")yields exactlyRUN_COUNTlines and the newtoEqualassertion holds.- Fixture
console.logoutput format vs. the/ RSS \d+ MB$/strip — matches, and the env override always wins over the fixture defaults since the test passes both explicitly. bunEnv/bunExeimports still used by the fourth test; no dead imports. Follows the existingisDebugrepetition-count pattern (e.g.worker_heap_snapshot_gc.test.ts).
Minor: the PR description still says 1 x 3 on debug, but the code (and its comment) is 1 x 1 per commit 910faac — not a code issue.
Extended reasoning...
Overview
Two test files touched. worker_thread_check.ts now reads RUN_COUNT/CONCURRENCY from env with the previous 5/10 as defaults. worker_destruction.test.ts passes 1/1 when isDebug (else 5/10), and adds a stdout assertion that the fixture printed exactly one Spawned N workers line per round after stripping the RSS suffix. No production code touched.
Security risks
None. Test fixture only; env vars are set by the test itself and read via Number(process.env.X ?? default) in a spawned child.
Level of scrutiny
Low. This is the established pattern for shrinking repetition counts on debug builds (isDebug ? small : full) rather than raising timeouts, per REVIEW.md. Release and ASAN CI lanes see identical behavior to before (explicit 5/10 passed). The debug path still exercises each of the three teardown methods (Bun.connect / Bun.listen / fetch) once, and the three test.concurrent cases still run in parallel across processes.
Other factors
- Verified
bunRuntrims stdout, so the trailing newline fromconsole.logdoes not produce an empty tail element insplit("\n"). - The new stdout assertion is a strict improvement: previously only
.toSpawn()(exit code + empty stderr) was checked, so a fixture that ran zero rounds would have passed silently. - The code comment and commit 910faac explain the drop to
1 x 1(1 x 3 still timed out on 4–6 core boxes); the PR body text lags one commit behind but the code is internally consistent. - No CODEOWNERS entry for this path. No prior human review comments to address.
|
The description has been updated for 910faac: the debug configuration is 1 x 1 throughout, and 1 x 3 now only appears in the core-count table as the configuration that was measured and rejected. No code change since that commit. |
1 participant
What
bun bd test test/js/node/worker_threads/worker_destruction.test.tsfails three of its four tests on a debug build purely on time:With
--timeout 180000they pass in ~29.5s each (19s for one case run on its own), so they are slow rather than hung. CI does not see this: it runs release and release+ASAN binaries, where the whole file takes 1.1s / 2.5s (test/expected-durations.json), under a 90s per-test timeout. It only affects running the file againstbun bd.Why
Each case runs
worker_thread_check.ts, which does 5 rounds of 10node:worker_threadsWorkers, each terminated shortly after it comes online, and the three cases aretest.concurrentwith each other and with the fourth test, so the file starts 150 workers at once. On a debug+ASAN build one worker_threads spawn + terminate costs ~2.4s of CPU and ~1.7s of latency (both under 30ms on release; a plain Web Worker comes up in ~130ms on the same build, the rest is the node:worker_threads bootstrap running in the worker at debug speed, the same ~65x factorrequire("worker_threads")shows on the main thread: 7ms release, ~460ms debug). The file is CPU bound: it needs roughly3 x CONCURRENCY x 2.4s + ~8sof CPU (three fixture processes, the fourth test and the runner itself), spread over however many cores the machine has, inside the 5s default timeout.Measured with the real test file on this debug+ASAN build, pinned with
tasksetto vary the core count (per-test times, two or three runs each):The fourth test (one worker plus a child process) is ~2.9s on its own on this build, all of it worker boot; at HEAD it reaches 4.2 to 4.5s (7.2s was seen on a busier box) only because it shares the CPU with the other 150 workers, and it is unchanged. With one worker per case it sets the floor for the file: the four tests finish within ~0.3s of each other at every core count above.
Change
Test-only.
worker_thread_check.tsreadsRUN_COUNTandCONCURRENCYfrom the environment, defaulting to the previous 5 x 10, andworker_destruction.test.tspasses1 x 1whenisDebugand5 x 10otherwise, following the existingisDebugconvention for repetition counts (worker_heap_snapshot_gc.test.tsin the same directory,fs.test.ts, #37374). Release builds, and therefore every CI lane including ASAN, run exactly what they ran before. A debug build takes each of the three teardown paths (Bun.connect, Bun.listen, fetch in a terminating worker) once, in three concurrent processes, which is the functional check; the repetition that makes it a stress test stays on release. The fix this fixture shipped with (#11494) is per-VM shutdown guards in the socket and server code, so nothing it guards needs more than one worker per process; a first version of this PR used 3 per case on the strength of "a few at once", and the table above is why it was dropped to 1: 3 is ~29s of CPU and still times out on 4 and 6 core machines, while 1 passes on 4 cores with over a second to spare. Per REVIEW.md the workload is shrunk rather than the timeout raised; with one worker per case no debug-only timeout is needed. More rounds are not an option either: each round is ~2s of latency on top of ~0.75s of fixture startup, so 2 rounds is at the limit even with one worker.The test now also asserts that stdout is exactly one
Spawned N workersline per round (the RSS suffix stripped), so the counts are checked against what the fixture actually ran. Previously only the exit code and empty stderr were checked, so a fixture that ran zero rounds would have passed. Checked by running the new test file against the unmodified fixture: it fails with 5 xSpawned 10 workersreceived where a single line was expected.Verification
bun bd test test/js/node/worker_threads/worker_destruction.test.ts: 3 of 4 fail on time before (above); after, 4/4 on every run at the 12 core quota (2.7 to 3.1s per test, file ~5.3s, was ~33s with a raised timeout), and 4/4 in five runs pinned to 4 cores and four pinned to 6 (table above).USE_SYSTEM_BUN=1 bun test ...(release, runs the unchanged 5 x 10 through the new assertion): 4/4, ~0.5s per case, 8 expect() calls.[stamp-90s] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file