test(bundler): run bundler_compile_prelinked.test.ts three links at a time and assert stderr - #42358
Conversation
|
Status: ready for review. The diff is green. Both CI builds are red only for a test that this PR does not touch (see the end). How the slow run was reproduced: With this branch the same machine takes 26 to 35 s (7 runs). The file passes with Time of this file in CI: main (build 114078) against the two builds of this PR. The file passed on every lane that ran it, 27 pass each time.
The Linux and darwin aarch64 columns are not like for like. A modified file runs first in its shard, next to the start of the shard's docker services and the dependency install. On main the file runs later, in the parallel bucket. The last build of #42002 (113552) is the like for like case, because the file was new there and also ran first: 25.1 s on debian 13 x64 and 18.2 s on debian 13 aarch64, against 4.2 to 10.0 s and 7.8 to 8.8 s here. Facts for the three choices a reviewer may want to check:
The red tests: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesThe prelinked bundler tests now run all loader modes, support platform-aware executable launching, limit concurrent links, require empty stderr, and validate exact generated-graph hook counts. ChangesPrelinked bundler test execution
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No confirmed merge-blocking risk remains in these test changes. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. A human look is still worthwhile: the hand-rolled beforeEach/afterEach semaphore with an Infinity hook timeout is a new pattern in the suite, and the pinned loader-hook counts (252/5/3/63/1) plus the cmd.exe /d /c launcher are cross-platform choices worth a maintainer sign-off.
What was reviewed:
- Semaphore correctness — acquire/release balanced on every path;
afterEachruns after throw/timeout so no slot leaks;waiting.shift()is fine at ~26 entries. bunArgsplacement inexpectBundled.ts— forcompile: truethe command becomes[...bunArgs, file, ...args], so["cmd.exe","/d","/c", exe]is a valid argv[0] launcher.stderr: undefinedinsplitRuncorrectly suppresses the newstderr: ""default (expectBundled filtersv !== undefined), and the line-by-line hookCalls parse turns any non-Loader [...]line into a failing key.- Removing
hasPrelinkOptionsstrengthens rather than weakens coverage — all three loader modes now always run.
Extended reasoning...
Overview
This PR reworks a single test file, test/bundler/bundler_compile_prelinked.test.ts, for concurrency and stricter assertions. It switches to describe.concurrent gated by a hand-rolled 3-slot (1 under ASAN/debug) semaphore in beforeEach/afterEach, adds a Windows-only cmd.exe /d /c launcher via bunArgs to move Smart App Control's first-CreateProcess stall off the JS thread, adds stderr: "" to every graphCase run, drops the hasPrelinkOptions probe so all three loader modes always run, and replaces the loose count("evaluate")===63 / greater-than / less-than-or-equal checks in GeneratedGraph+splitting with an exact .toEqual on a parsed hook→count map.
Security risks
None. Test-only change; no production code, no network, no credentials, no untrusted input parsing beyond stderr from a locally-built executable.
Level of scrutiny
Medium. It's test-only and additive in coverage, but it introduces a concurrency pattern (beforeEach(..., Infinity) semaphore) that appears nowhere else in the suite and sits at the edge of test/CLAUDE.md's "do not set a timeout on tests" rule — the Infinity is on a hook, not a test, and is load-bearing (queued beforeEach waits can exceed the 5 s default), but a maintainer should confirm this pattern is acceptable versus, say, a harness helper. The 3-slot cap references prior measurement (#39649) on Linux lanes; the PR verified Windows and Linux but a maintainer familiar with all CI lanes' memory headroom should confirm. The exact loader-hook counts were verified on four platforms per the PR notes and follow REVIEW.md's "assert the strongest invariant," though they will need updating if JSC's dumpModuleLoadingState output ever changes.
Other factors
I traced the semaphore through failure/timeout paths and it releases correctly. I confirmed in expectBundled.ts that bunArgs prepends to the compiled executable path when compile: true, so the cmd.exe launcher works as described, and that run.stderr === undefined skips the stderr equality check (so splitRun's stderr: undefined correctly overrides the new default). Removing the probe does not weaken coverage — an older bun without the JSC options now hard-fails on invalid JSC environment variable instead of silently running only one mode. The cmd.exe /d /c launcher's known limitation (paths containing &, (, ), ^, %VAR%) is acknowledged in the PR notes and is unlikely to bite CI temp dirs. Bug-hunt exit reason was dry_streak with no findings.
|
Updated 1:24 PM PT - Sep 11th, 2026
❌ @robobun, your commit e7bcca2 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 42358That installs a local version of the PR into your bun-42358 --bun |
…e and pin stderr The 26 compile cases ran one after the other. They now run under describe.concurrent, with a beforeEach/afterEach pair that lets three links be in flight on release builds and one on ASAN and debug builds. On Windows the executables start through cmd.exe. The first CreateProcess of a new executable blocks its caller while Windows looks up the file's reputation, and Bun.spawn makes that call on the JS thread, so without this the cases cannot overlap that wait. Every run now also asserts an empty stderr. GeneratedGraph+splitting asserts the exact count of each loader hook call. The module-load probe for the JSC options is gone: every bun that can run this file has them.
0bdaed0 to
e7bcca2
Compare
There was a problem hiding this comment.
LGTM — test-only change that tightens assertions and bounds link concurrency.
What was reviewed:
- Semaphore in
beforeEach/afterEach: acquire/release balanced on all paths,Infinityis a validHookOptionspertest.d.ts, slot count branches onisASAN/isDebug. bunArgslauncher:expectBundled.tsbuilds[...bunArgs, file, ...args]whencompileis true, socmd.exe /d /c <exe>is argv[0] as intended; non-Windows gets[].stderr: ""added to every mode;splitRuncorrectly overrides toundefinedvia spread order so the loader-logvalidatesees the output; the histogram.toEqualstrengthens the prior loose bounds.- Dead
hasPrelinkOptionsprobe and its unused imports removed alongside the unconditional mode list.
Extended reasoning...
Overview
This PR touches a single test file, test/bundler/bundler_compile_prelinked.test.ts. It converts the suite to describe.concurrent with a hand-rolled 3-slot (1 under ASAN/debug) semaphore in beforeEach/afterEach, wraps compiled-executable launches in cmd.exe /d /c on Windows to move the Smart App Control first-run stall off the JS thread, adds stderr: "" to every loader-mode run, replaces the loose loader-log bounds with an exact per-hook .toEqual histogram, and deletes the hasPrelinkOptions spawnSync probe together with its now-unused bunEnv/bunExe imports. No production code changes.
Security risks
None. The change is confined to a test file; it spawns only the locally built executable (via expectBundled) and cmd.exe on Windows CI runners. No network, credentials, or untrusted-input parsing is introduced.
Level of scrutiny
Low-to-moderate. This is test infrastructure with no runtime surface. The parts worth checking were mechanical: the semaphore's acquire/release pairing (correct — afterEach wakes one waiter or increments freeSlots, never both), the beforeEach(fn, number) overload (HookOptions = number | { timeout?: number } in packages/bun-types/test.d.ts), and that expectBundled places bunArgs ahead of the compiled file when compile is set (it does, line ~1799). The spread order in graphCase puts splitRun's stderr: undefined after the default stderr: "", so the loader-log case is exempted from the empty-stderr check and validated line-by-line instead.
Other factors
The change follows the repo's stated test conventions closely: describe.concurrent for independent subprocess-spawning suites, resource bounds branched on isASAN/isDebug, exact .toEqual on a whole object over loose comparisons, dead code removed in the same change, and a why-comment on the Windows-only branch. A prior COMMENTED review under this app's identity was followed by a force-push; no CHANGES_REQUESTED review or unresolved third-party objection is outstanding. The PR description's timing measurements are consistent with the shape of the change but are not load-bearing for correctness.
2 participants
Problem
test/bundler/bundler_compile_prelinked.test.tstakes 81 s on the Windows 11 aarch64 lane (build 114078). It links 26 executables one after the other and runs each one 3 times.CreateProcessof a new executable blocks about 2.4 s.Bun.spawnmakes that call on the JS thread, sodescribe.concurrentalone cannot overlap it (74.5 s to 69 s).Fix
describe.concurrent. AbeforeEach/afterEachpair lets 3 cases link at once on release builds and 1 on ASAN and debug builds (the split test(bundler): overlap bundler_compile.test.ts links on release builds and pin more outputs #39649 measured).cmd.exe /d /c. The wait happens incmd.exe, and the cases overlap it.GeneratedGraph+splittingpins each loader hook count:resolveis 5 with the graph, 252 without (was<= 6,> 63). The module-loadBun.spawnSyncprobe is gone.bun bd testis unchanged (1 slot). All 27 tests and 3 loader modes still run.Background
Notes
Where the time goes. Probe on a Windows 11 arm64 VM with Smart App Control in evaluation mode (
Get-MpComputerStatusreportsSmartAppControlState: Eval, the same as the CI image):bun build --compile --bytecode260 to 330 ms, first launch of each new executable 2.4 to 2.7 s, second and third launch 16 ms. TheBun.spawn()call itself returns after 2.3 to 4.0 s for a new executable and after 2 to 3 ms for a known one. Eight cases started at once take 22.0 s with a direct spawn (the same as serial) and 5.8 s throughcmd.exe /d /c, where theBun.spawn()call returns in 7 ms. During one first launchMsMpEnguses about 1.7 s of CPU. Pinning the test process tree to 4 or to 2 cores does not change the file's time (28.0 s and 27.2 s), so the 4 vCPU runners should see a similar gain.Whole file,
bun testwith a release build, main to this branch.cmd.exelauncherbun bd test(debug + ASAN, 1 slot)Slot count on the Windows 11 VM: 2 slots 38.3 s, 3 slots 27 s, 4 slots 22.9 s, 6 slots 17.0 s. The file uses 3 on every release build, the number #39649 checked on the 4 vCPU, 8 GB Linux lanes.
Per lane in build 114078 (all in the parallel bucket): windows 11 aarch64 80.55 s, darwin x64 44.52 s, debian x64-asan 37.19 s, windows 2019 x64 17.27 s, debian x64 9.68 s, ubuntu x64 9.60 s, ubuntu aarch64 7.20 s, alpine x64 7.18 s, debian aarch64 7.08 s, alpine aarch64 5.00 s, darwin aarch64 4.31 s. In that batch of 189 files (91 s) this file was the critical path on Windows 11.
ASAN and debug stay serial. Locally the link is 2.5 to 2.9 s of a 3.5 s case with the 812 MB debug binary, and each run is about 0.3 s. #39649 tried 3 links at once on the x64-asan lane: the file got slower there (170 s against 107 s), because the link is three passes over the binary and the lane is bound by writeback. One slot under
describe.concurrentbehaves like today.The three runs of one executable do not overlap. After the first launch a run takes 5 ms on Linux and 16 ms on Windows, so there is nothing to gain on release builds. On debug builds it would save about 0.6 s per case, and it needs a change to the run loop in
expectBundled.ts, which every bundler test file shares.bunArgs.expectBundledbuilds the command as[...bunArgs, file, ...args]for a compiled executable, so the launcher needs no change to the helper.cmd.exe /d /c <path>passes stdio and the exit code through. It works for paths with spaces and+(probed). It does not work for a temp directory whose path contains&,(,),^or%VAR%.Loader counts. With
BUN_JSC_dumpModuleLoadingState=1JSC prints oneLoader [hook] keyline per host hook call. Linux x64 release, Linux x64 debug, Windows 11 arm64 and Windows Server 2019 x64 all print the same table:resolve5 (bun:main twice, the entry, m24 twice) with the graph, 252 without it,fetch3,evaluate63,import1, and no other line on stderr.The probe.
hasPrelinkOptionslet an older bun fail only in the loader log check. That was for the fail-before run of #42002. A bun without the options now fails the second and third run of every case withinvalid JSC environment variable, which is also a clear failure.Slots. 45 concurrent tests with 3 slots, one test that throws and one that times out: 45 ran, at most 3 held a slot, no slot leaked.
afterEachruns after a failed or timed out test. The per-test timeout starts when the test callback starts (on_entry_startedinsrc/runtime/test_runner/Execution.rs), so the wait inbeforeEachdoes not count against it. The reported duration of a test does include the wait.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bundler_compile_prelinked.test.ts