test(worker): keep the terminate() race tests under the default per-test timeout on debug builds - #37374
test(worker): keep the terminate() race tests under the default per-test timeout on debug builds#37374robobun wants to merge 3 commits into
Conversation
…est timeout on debug builds Three tests in the "terminate() races and lifecycle edges" block took 5.5s, 15s and 4-5s on a debug+ASAN build, against bun test's 5s default: - preload with an un-awaited import(): big.js goes from 4000 to 1000 exported functions. Its transpile is still tens of times side.js's, which is what the test relies on; the rest of the size only cost debug builds time. - terminate() while fs.readFile completions keep arriving: 10 workers (one per 0-9ms terminate offset, in two batches) instead of 48, and the worker require()s node:fs instead of building its ESM namespace, which loads the stream classes and nearly doubled each worker's start on debug builds. - terminate() while a node:vm script is running: the six workers run concurrently instead of one after another; loading node:vm dominates each one's start on debug builds. Debug build: 1.8s, 2.0s and 0.8s; the file as a whole goes from 53s to 34s. Release: 20ms, 50ms and 15ms.
|
Warning Review limit reached
Next review available in: 4 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 |
|
Status: reproduced on a debug+ASAN build of main (23d233b): |
![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 that follows the "shrink the workload, don't raise the timeout" rule.
What was reviewed:
- The vm-timeout test's sequential→concurrent change: the race is per-worker (terminate() vs. that worker's watchdog), so parallelizing boot doesn't change what's exercised.
- The readFile-churn reduction (48→10 workers, import→require): terminate offsets 0-9ms are still all covered via
r * 5 + i, two batches preserve pool-reuse, and the PR names the sibling tests covering the deterministic paths. - The
big.jsshrink (4000→1000): PR description verified fail-for-right-reason by rebuilding with thenoteModuleEvaluationguard removed — 20/20 detection at 1000.
Extended reasoning...
Overview
Test-only change to test/js/web/workers/worker.test.ts reducing the workload of three terminate() race tests so they complete under the default 5s per-test timeout on debug+ASAN builds (they were passing in CI only because the CI runner passes --timeout=90s). Three edits: (1) the node:vm timeout test runs its 6 workers concurrently instead of sequentially, (2) the preload dynamic-import test's big.js fixture shrinks from 4000 to 1000 exported functions, (3) the fs.readFile churn test drops from 48 workers to 10 and switches the worker's import to require to skip node:fs's ESM namespace build. No production code touched.
Security risks
None. Test fixture and iteration-count changes only.
Level of scrutiny
Low-to-medium. The diff is small and mechanical, but the review guidance explicitly flags "never silently weaken an existing test" and "when de-flaking, keep asserting the property the original assertion protected", so the question is whether coverage is preserved. The PR description addresses this head-on for each test — most notably for the preload test, where the author rebuilt a debug binary with the noteModuleEvaluation root-record check removed and confirmed the shrunk fixture still detects the regression 20/20 times (vs. 18/20 at the original size). For the readFile test, the terminate-offset space (0-9ms) is fully preserved by r * 5 + i over r∈{0,1}, i∈{0..4}, the two-batch structure keeps the pool-reuse-after-discard phase, and the deterministic completion-refusal paths are named as covered by worker-refused-completion.test.ts and worker-terminate-funnels. The vm test's parallelization is safe because each worker is an independent VM and the race being tested is entirely within one worker.
Other factors
This is exactly what REVIEW.md prescribes: "Don't raise per-test timeouts to make a slow test pass; shrink the workload." The assertions themselves are unchanged (expect(stdout).toBe("PASS\n"), expect(ev.data).toEqual([...])). The import→require switch in the readFile worker doesn't affect what's being raced (the fs completions, not module loading) and matches the fixtures in the two sibling test files the PR references. The added comments explain the sizing choices per the repo's comment guidance (why-not-what). No prior reviewer comments to address; the only timeline entry is a CodeRabbit rate-limit notice.
…adFile churn to its first completion Release builds (and so CI) keep the original repetition counts: big.js stays at 4000 functions, the entry-resolution and natural-exit tests keep 12 and 3 rounds, and the readFile churn keeps ~50 workers. Only a debug build, where each of these workers costs 0.2-0.5s to start, runs fewer. The readFile worker now posts busy from its first completion instead of right after issuing the reads. On a debug build the worker's first loop turn takes 40-60ms, longer than the whole 0-9ms range of terminate offsets, so with busy posted synchronously every terminate() landed before any completion had been dispatched; anchored to a completion, every offset lands while completions are being dispatched and re-issued on both builds.
|
Pushed af4c0c7, which changes the approach after self-review; the description is updated to match.
Debug build: the five tests take 0.8 to 2.3s each, the file 33s (was 53s); release file 1.7s, 36/36. |
There was a problem hiding this comment.
LGTM — test-only workload reduction gated on isDebug, following the established harness pattern. The one inline nit is about the PR description drifting after the second commit; worth a quick re-sync before merge but not blocking.
Extended reasoning...
Overview
This PR touches a single file, test/js/web/workers/worker.test.ts, and only the terminate() races and lifecycle edges describe block within it. No runtime code changes. The changes reduce wall-clock on debug+ASAN builds so the tests fit under the default 5s per-test timeout when run locally with bun bd test:
- Gate repetition counts on
isDebugfor four tests (entry-resolution rounds 12→3, natural-exit rounds 3→1, readFile-churn batches, big.js size 4000→1000). - Run the six node:vm workers concurrently instead of sequentially (same per-worker race, fewer sequential boots).
- In the readFile-churn worker, switch
import→require("node:fs")and postbusyfrom the first completion callback so the terminate offset window lands mid-churn on slow builds.
Security risks
None. Test-only; no untrusted input handling, no runtime surface.
Level of scrutiny
Moderate. Per REVIEW.md, reducing iteration counts is a form of test weakening that needs justification. The author addressed this directly: for the preload test they rebuilt with the noteModuleEvaluation root-record check removed and confirmed 20/20 detection at 1000 functions (vs 18/20 at 4000), so the smaller fixture is at least as sensitive; for the readFile test they note the deterministic phases are covered by worker-refused-completion.test.ts and worker-terminate-funnels, and the reduced count is fewer samples of the same stochastic race rather than a dropped phase. All reductions are isDebug-gated, so release/CI coverage is unchanged (or increased: readFile goes 48→50 workers on release). This is the "shrink the workload, don't raise the timeout" and "branch per-platform rather than dropping precision" guidance applied correctly.
Other factors
isDebuggating of iteration counts is an established pattern across the suite (fetch, worker_threads, tls, vm leak tests all do it).- The concurrent-vm rewrite has no shared state between the six workers; each independently awaits its own
messageandclose. - The
busyflag in the readFile worker is set on the worker's single JS thread, so the once-only guard is race-free. - The interpolated
${rounds}/${rounds * 8}in the entry-resolution test keep the assertion in lockstep with the loop bound. - The inline nit (PR description stale after commit af4c0c7) is real per
.claude/docs/landing-prs.md's "re-sync title/description" rule, but it's metadata, not code — fine to fix at merge time.
…ng worker boot Same change as #37355, carried here so that the whole file passes on a debug build; it drops out on rebase once that lands.
|
Pushed 9d60ce7: the message flood test in the same block fails on every debug build too (it is #37355's subject), and without it the file cannot pass as a whole on a debug build, so that PR's diff is carried here unchanged as the last commit. If #37355 lands first it drops out on rebase; if this lands first, #37355 becomes a no-op. With it, |
There was a problem hiding this comment.
LGTM — test-only timing fix that gates repetition counts on isDebug so release/CI coverage is unchanged, following the existing convention in this test directory.
What was reviewed:
- Confirmed release-build paths keep their original counts (12 rounds, 3 rounds, 4000 fns, 5×10 workers); only debug reduces.
- Checked
drainBatchLimit = 1024in WorkerMessagingProxy.cpp — the flood test's 1500 backlog andtoBeLessThan(total)assertion line up with it. - The readFile churn's
busy-from-first-completion anchor andrequireswap tighten the sampled window rather than weaken it; release goes 48→50 workers. - Prior nit about the stale PR description was addressed (thread resolved).
Extended reasoning...
Overview
Single-file, test-only change to test/js/web/workers/worker.test.ts. Five terminate() race tests that were timing out at the 5s default on debug+ASAN builds now gate their repetition counts on isDebug (imported from harness), so release builds — the only thing CI runs — execute exactly what they did before. Two changes apply to all builds: the node:vm test starts its 6 workers concurrently instead of serially, and the readFile churn worker posts busy from its first completion (and requires node:fs instead of importing it) so every terminate offset lands mid-churn rather than before any callback. The message-flood test rewrite is carried unchanged from #37355 to make the file pass as a whole on debug.
Security risks
None. No runtime code touched; test fixtures spawn subprocesses under bunEnv in temp dirs with no network or credential handling.
Level of scrutiny
Low-to-medium. Test-only, release CI coverage is provably unchanged (all reductions are behind isDebug), and the pattern matches the repo's own de-flaking guidance (branch per build type rather than drop precision). The two all-builds changes — concurrent vm workers and the readFile busy anchor — are reasoned from measurements in the description and, per the author's SharedArrayBuffer instrumentation, make the release-build test sample the intended window more reliably than before (offset 0 previously saw 0 callbacks; now 11+).
Other factors
- REVIEW.md's "never silently weaken a test" concern is directly addressed: the description shows the 1000-function
big.jsstill catches the #37075 regression 20/20 on a deliberately-broken debug build, and release keeps 4000. The readFile race's deterministic coverage lives inworker-refused-completion.test.tsandworker-terminate-funnels, cited in the description. - I spot-checked
drainBatchLimit(1024) insrc/jsc/bindings/webcore/WorkerMessagingProxy.cpp— the flood test'stotal = 1500and its comment referencing that constant are accurate. - The message-flood rewrite (Atomics.wait to build the backlog up front, then assert one drain batch < total) is more deterministic than the previous 3×10ms timer window and is #37355's diff verbatim; it has its own review track and was verified 20/20 on release here.
- My earlier inline nit (stale description) was resolved — the description now matches the final diff.
- Author reports 6/6 debug and 20/20 release passes for the five changed tests, plus full-file 36/36 on both. Any residual ordering assumption in the flood test would surface immediately in CI, not ship to users.
1 participant
What
bun bd test test/js/web/workers/worker.test.tson a debug+ASAN build fails two of the tests added in #37075 purely on time, and the next few sit close to the limit:With
--timeout 120000they pass in 5.5s and 14.9s, so they are slow, not hung.terminate() while a node:vm script with a timeout is runningtook 4.0s in isolation and timed out at 5.0s in a full-file run while the host was busier;terminate() while the entry point fails to resolve(96 workers) andmessages posted right before a natural exit(3 x 5000 messages) reached 4.4s in the same conditions. CI never sees any of this: it only runs release and release+ASAN binaries and the runner passes--timeout=90s(x3 on ASAN). It only affects running the file locally against a debug build, where a worker takes ~150ms to start (~2ms release), loading node:fs or node:vm in it another ~0.4s, and the 4000-functionbig.js~1.7s per iteration. On a release build the whole file takes under 2s.Change
Repetition counts are gated on
isDebug; release builds, and therefore CI, run exactly what they ran before. This is the existing convention for this (worker-terminate-funnels.test.tsbranches its budget onisDebug,fs.test.tsitsiterCount,worker_threads.test.tsits default timeout), and it means the ASAN lanes, which are the ones that turn a bad release path into a failure, lose no samples. On a debug build:big.jshas 1000 functions instead of 4000 (3 iterations as before). What the test relies on isbig.jstranspiling on the pool long enough thatside.js, requested earlier by the preload, evaluates in a tick before the entry graph arrives; at 1000 functions that is ~107ms vs ~1.5ms on debug. To check it still catches what it was added for (the Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 review thread:entry_evaluation_startedset from anymoduleLoaderEvaluate), I built a debug binary with the root-record check removed fromnoteModuleEvaluationand ran the scenario 20 times per size: the parent's messages were dropped 20/20 at 1000 (18/20 at 4000, 0/20 with an emptybig.js, so the size matters but 1000 is well past where it does); the real build delivered 5/5. Debug builds do not restore from the runtime transpiler cache and CI disables it (BUN_RUNTIME_TRANSPILER_CACHE_PATH=0in the runner and inbunEnv), so the one configuration in which a smaller cachedbig.jswould weaken the test, a local release run with a warm cache, is the one that keeps 4000.worker-refused-completion.test.tshas anfs.readFilerow for the refusal path andworker-terminate-funnelsterminates at fixed points aroundfs.readFile.Carried from #37355: the message flood test in the same block also fails on every debug build (it asserted on a fixed 30ms window that a debug worker's boot alone exceeds). Its fix is #37355, open separately; the last commit here is that PR's diff, unchanged, so that this file passes as a whole on a debug build. It drops out on rebase if #37355 lands first, and makes #37355 a no-op if this lands first.
Two changes apply to every build:
The vm test starts its 6 workers concurrently instead of one after another. The race (terminate() landing in a worker while a script with a timeout watchdog is running in it) is per worker and unchanged; six concurrent starts cost about one sequential one.
The readFile worker
require()s node:fs (importing it also builds the ESM namespace, whoseReadStreamgetter loads the stream classes) and postsbusyfrom inside its first completion instead of right after issuing the reads. The second part matters for what the test samples. I had the worker's callbacks count themselves into a SharedArrayBuffer and recorded, per terminate offset, how many had run when terminate() was called / by the time the worker stopped:import, busy posted synchronously)require, busy posted synchronouslyrequire, busy posted from the first completionOn a debug build the worker's first loop turn takes 40-60ms, longer than the whole 0-9ms offset range, so with
busyposted synchronously every terminate() landed before a single completion had been dispatched, and the original head only reached the dispatch phase at the larger offsets. Anchored to a completion, every offset lands while completions are being dispatched and re-issued on both builds, and some samples now catch completions landing between terminate() and the stop (e.g. 92 run at terminate(), 94 before the stop), which is the race the test is about.Verification
Debug build (
bun bd test, 16-core box under load), before -> after:The five tests passed 6/6 debug runs plus 5 more of the readFile test alone, and 20/20 release runs plus 15 more of the readFile test. Full file on debug: 36 pass in 31s (53s before); on release: 36 pass in 1.7s.
test/js/node/worker_threads/worker_destruction.test.tshas the same problem in a bigger way (three tests of 50worker_threadsworkers each take 30 to 45s on a debug build); it is a different file and is being looked at separately.[stamp-90s] gate passed · iteration 1 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file