Repository navigation
Conversation
Reduce rounds from 16 to 4 on debug/ASAN builds (96 spawns instead of 384) and bundle to stdout instead of --outdir so the test can assert on the actual bundle contents rather than only on exit code and wall time. Since #34009 lowered the idle-futex timeout from 10s to 100ms, a skipped shutdown wake now costs at most ~100ms, which is below the 24-way scheduling noise floor. The 16-round version no longer distinguishes a reverted Event::wake fix either, so cutting rounds does not weaken the timing guard; it remains a coarse check for multi-second stalls plus a concurrent-build smoke test with real output assertions.
|
Warning Review limit reached
Next review available in: 8 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only speedup with strictly stronger assertions.
What was reviewed:
- Round reduction gated on
isASAN || isDebug(both exist in harness); PR description shows the 16-round variant already can't distinguish a reverted fix post-#34009, so nothing is lost. - Switch to stdout bundling: pipes are drained concurrently via
Promise.all, bundle is small (40 tiny modules), no deadlock risk;rootstill used forcwd. - New per-build assertions check
stderr, chain head/tail, and entry-specific line beforeexitCode— matches repo convention of asserting output before exit code.
Extended reasoning...
Overview
Single-file change to test/regression/issue/32492.test.ts, a regression test for a worker-pool shutdown stall. Three modifications: (1) ROUNDS drops 16→4 on debug/ASAN via isASAN || isDebug, (2) bun build writes to stdout instead of --outdir/--sourcemap=external, dropping the now-unused round parameter and per-build output dirs, (3) the exit-code-only check becomes five per-build assertions on stderr, bundle head/tail markers, the entry-specific console.log line, and exit code.
Security risks
None. Test-only; spawns bunExe() with bunEnv in a tempDir, no network, no external inputs.
Level of scrutiny
Low. No production code touched. The core question is whether coverage is weakened, and the PR description addresses this directly with measurements: after #34009 lowered the idle-futex timeout to 100ms, even the original 16-round test passes with the #32494 fix reverted, so the timing guard was already only a coarse multi-second-stall detector. Reducing rounds on the slow lanes preserves that while cutting ~75% of runtime. The removed --sourcemap=external/--outdir path is orthogonal to the pool-shutdown code path being guarded.
Other factors
- Harness conventions followed:
Promise.all([stdout.text(), stderr.text(), exited])for concurrent pipe drain, output asserted beforeexitCode,isASAN/isDebugbranching for load reduction. - The new
toContainassertions on printer output (var v40 = 40;,var v1 = 1 + v2;) tie the test to bundler output shape, but that's normal for a bundler smoke test and makes it strictly stronger than the previous exit-code-only check. - The unused
rootvariable is still used (cwd: root); the removedroundparameter was only feeding the deleted--outdirpath. - PR notes overlap with #33704's 32492 hunk; this is scoped to one file and adds assertions on top.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Only the first futex wait in Event::wait has the 100ms timeout; later waits are indefinite (see ThreadPool.rs:1412-1416). The round reduction is justified by the empirical revert result, not by a timeout bound, so say that instead.
There was a problem hiding this comment.
LGTM — test-only speedup with stronger assertions.
What was reviewed:
isASAN/isDebugare exported fromtest/harness.tsand this branching pattern is used in other regression tests.- The timing guard (
slowestMs < STALL_MS) and per-build exit-code check are both retained; new stdout/stderr assertions are added on top, so the test is strictly stronger per build. - Pipes are still drained concurrently via
Promise.all, and output is asserted beforeexitCodeper repo convention. - Checked that dropping
--outdir/--sourcemap=externaldoesn't remove the property under test — the PR justifies this (shutdown path is sink-independent) and the round reduction is backed by an empirical revert experiment showing the 16-round version already can't distinguish the reverted fix on current main.
Extended reasoning...
Overview
Single-file change to test/regression/issue/32492.test.ts, a regression test for a ~10s worker-pool shutdown stall. The PR reduces ROUNDS from 16 to 4 on debug/ASAN builds (release stays at 16), switches from --outdir/--sourcemap=external to stdout bundling, and replaces the throw-on-nonzero-exit check with per-build expect() assertions on stderr, three bundle-content substrings, and exitCode. The unused round parameter to buildOnce is dropped since per-round outdirs are gone.
Security risks
None. Test-only change; no production code, no untrusted input handling, no network.
Level of scrutiny
Low-to-moderate. It's a test speedup, but REVIEW.md flags "never silently weaken an existing test" as a hard block, so the key question is whether the round reduction and dropped --sourcemap=external lose signal. The PR description addresses this head-on with an empirical revert of the #32494 fix showing the original 16-round test already passes on current main with the fix reverted (because #34009 lowered the first-wait timeout to 100ms). Given that, cutting to 4 rounds on debug/ASAN loses no fail-before signal, and the new bundle-content assertions add value as a concurrent-build smoke test. The 9000ms coarse guard against multi-second stalls is unchanged.
Other factors
isASANandisDebugverified as exports fromtest/harness.ts:27,54; the same pattern appears in othertest/regression/issue/files.- Subprocess pipes are still drained concurrently (
Promise.all([stdout.text(), stderr.text(), exited])), avoiding the 64KB-pipe deadlock — relevant now that stdout carries the full bundle. - Assertions are ordered output-before-exit-code per CLAUDE.md/REVIEW.md guidance.
- No CODEOWNERS entry covers this path.
- The PR notes overlap with #33704 (which only reduces rounds); this PR is a superset of that hunk and additionally strengthens assertions, so there's no conflict in intent.
- Author reports local
bun bd testpasses (484 expect calls, ~5s test time), which also confirms thevar vN = N;bundle-output shape and empty stderr on the stdout path.
|
Updated 8:09 AM PT - Jul 23rd, 2026
❌ @robobun, your commit 8a451d4 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35266That installs a local version of the PR into your bun-35266 --bun |
|
CI build #78631: 193 passed, 1 failed. The one hard failure is Ready to merge from my side. |
|
Closing this PR because it has been inactive for more than 90 days. |
What
test/regression/issue/32492.test.tswas the slowest file on the debian 13 x64-asan lane at 36s (build #78397). This brings it down roughly 4x and replaces the exit-code-only check with real assertions on the bundled output.Changes
--outdir/--sourcemap=external, which drops 384 per-build output directories and lets the test assert on the bundle text. The regression lived in thread-pool shutdown, which runs identically regardless of where the bundle is written.stderr == "", that the bundle contains the head and tail of the 40-module chain (var v40 = 40;,var v1 = 1 + v2;), and the entry-specificconsole.log(...)line, before checkingexitCode. 16 expect() calls -> 484.Why cutting rounds does not weaken the timing guard
The original regression was a ~10s stall because the first idle-futex wait in
Event::waithad a 10 second timeout. #34009 lowered that first-wait timeout to 100ms (for themi_on_thread_idlesweep). I reverted the|| wake_threads == u32::MAXguard from #32494 on currentmainand re-ran both versions of the test:The ~80-100ms delta is real and consistent with workers hitting the 100ms first-wait timeout, but it sits inside 24-way scheduling noise and well under the 9000ms threshold. The existing 16-round test already cannot distinguish a reverted
Event::wakefix on currentmain, so reducing rounds on debug/ASAN loses no fail-before signal there. The timing assertion remains as a coarse guard against multi-second shutdown stalls, and the new output assertions make it a useful concurrent-build smoke test.(Note: only the first wait in
Event::waitis bounded at 100ms; subsequent waits after the idle sweep are indefinite perThreadPool.rs. The indefinite-hang case did not trigger across 1152 reverted builds in this workload, but the round reduction is justified by the empirical result above, not by a timeout bound.)Verification
Local
bun bd test test/regression/issue/32492.test.ts(debug+ASAN, 16 cores):Overlaps with the 32492 hunk of #33704 (which only reduces rounds); this PR is scoped to the one file and also adds the output assertions.
[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