Repository navigation
Conversation
… concurrently The two tests that ask the executable of src/main.ts and its five other entry points share one build in each mode. The file starts 12 builds where it started 14. All 11 tests run concurrently. Each has folders of its own, and the two that share an executable only read it. Each run of an executable compares stdout, stderr, the exit code and the signal with exact values. Each build that must fail compares the message on stderr, exit code 1, and that it wrote no executable. The duplicate output error must name the shared output, sub/typed.js.
|
Status
CI on the head (
Open, for a maintainer
|
|
Updated 10:00 AM PT - Oct 30th, 2026
❌ @robobun, your commit 852c22d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 47463That installs a local version of the PR into your bun-47463 --bun |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Shared executable test setup test/aot/entry-point-names.test.ts |
The tests run concurrently, share build promises and a temporary directory, and clean up the directory after the suite. |
Process and build failure assertions test/aot/entry-point-names.test.ts |
The tests check complete process results, including trailing newlines, stderr, exit codes, and signals. Build-failure checks match specific errors and verify that no matching output file was created. |
Possibly related PRs
- oven-sh/bun#47468: Both PRs share AOT builds across related tests and run test cases concurrently.
- oven-sh/bun#47482: Both PRs use concurrent AOT test execution and compare complete process results.
- oven-sh/bun#47474: Both PRs share AOT builds and compare complete process results.
Suggested reviewers: jarred-sumner
Priority:
Merge Risk: 
852c2
-
The shared builds and concurrent tests have no established blocking defect. The change is mergeable subject to normal checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main test optimization: sharing one build and running the entry-point tests concurrently. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description provides a detailed explanation of the problem, fix, behavior, performance impact, testing method, CI evidence, and validation results. It does not use the template headings exactly, b… Full details: Description check
-
Explanation
The description provides a detailed explanation of the problem, fix, behavior, performance impact, testing method, CI evidence, and validation results. It does not use the template headings exactly, but it includes the required information.
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked that the shared build's folder outlives both consumers (it is only disposed in afterAll, and the two tests that use it just spawn or read the file), that spawned() returns exactly the four keys { stdout, stderr, exitCode, signal } so the new toEqual shapes are exact rather than vacuous, and that the rebuilt expected string matches the old value plus a trailing newline.
Extended reasoning...
The change is confined to test/aot/entry-point-names.test.ts: it switches the file to test.concurrent, memoizes one build per mode into a module-level shared tempDir disposed in afterAll, and tightens every run assertion to the full spawned() result. It touches no security-sensitive surface. The two inline findings (unbounded concurrent builds with no slot gating, and afterAll disposing shared folders without try/finally) are what keep this from an approve; the ruled-out checks above are recorded so a human reviewer need not repeat them.
| function withEveryEntryPoint(): Promise<string> { | ||
| if (!once) { | ||
| const dir = tempDir("aot-entry-point-names", files); | ||
| shared.push(dir); | ||
| once = build(String(dir), [...flags], ["src/main.ts", ...additional], title); |
There was a problem hiding this comment.
🟡 (optional) Developers running this file on a debug build with few cores get spurious failures: a --aot build killed at 90 s. The file now starts up to 12 builds at once (test/aot/entry-point-names.test.ts:127 and every build() call) with no slot() bound, unlike in-place-executables.test.ts:15-20 which gates each command on a processor. spawned() kills a child at limitOfARun = 90 s (test/aot/harness/commands.ts:58) when not ASAN. Fix: wrap each build() in slot() from ./harness/commands so concurrent builds never exceed availableParallelism, matching the sibling file.
Why this was flagged
Trigger: bun bd test test/aot/entry-point-names.test.ts on a debug, non-ASAN Bun with 2-4 cores, or any lane where the runner's --max-concurrency cap (scripts/runner.node.ts:1425-1427) does not apply because the file is run directly. bun test's default max-concurrency is 20, so all 11 tests start together and test/aot/entry-point-names.test.ts:107 spawns a build for each; the PR says each --aot debug build alone takes 10 to 24 s. With 5 to 6 --aot builds sharing 2 to 4 cores, one build's wall clock exceeds limitOfARun = 90 s (test/aot/harness/commands.ts:58, chosen at commands.ts:73-79 since isASAN is false and moreBuildFlags is empty); Bun.spawn kills it with SIGKILL (commands.ts:89) and built() at commands.ts:170-175 fails on exitCode. On the base branch the tests ran in series, one build at a time, so no build competed for cores. The author measured only release builds on 2 cores (2.9 s each) and ASAN debug on 12 threads, which never exercise the 90 s non-ASAN limit under contention. Remedy: gate every build() through slot() as in-place-executables.test.ts does.
Verification: Trigger: a direct run on a debug Bun without ASAN on a machine with few cores. test/aot/entry-point-names.test.ts:15 makes 11 tests start together. Each build calls spawned(...) at line 107 with no slot() wrapper. test/aot/harness/commands.ts:58 limitOfARun = 90_000; built() at commands.ts:170-175 fails on the nonzero exit code. The base ran these builds serially, so this risk is new.
There was a problem hiding this comment.
Done in 1c05c67. Each command now goes through slot(), as in in-place-executables.test.ts.
I checked the case with a debug build on 2 cores and 20 concurrent tests. Without the slots, the tests with an --aot build took 112 to 122 s each, nearly all of it in the build. With the slots, the longest command takes 49 s, and the file takes 113 s where it took 128 s.
There was a problem hiding this comment.
Correction to my reply above: I removed the slots again in 852c22d. This note is not addressed.
The slots help in the case you describe (2 cores, debug build: longest command 49 s, against 112 to 122 s without them). In CI they cost time. On ubuntu x64, ubuntu aarch64 and debian aarch64 the file went from 2.5 to 3.2 s (build 135747) to 5.8 to 6.6 s (build 135759), while the eight files after it on the same shards stayed within about 5%. A machine with four processors has four slots and the file has five --aot builds, so the fifth starts when the first ends, and each run of an executable waits behind the four builds.
That is one run for each lane, and debian x64 went the other way (5.5 s to 4.0 s). The next CI run shows whether the three lanes return to about 3 s.
So a debug build without a sanitizer on few processors can still reach the 90 s limit of spawned() with this file.
There was a problem hiding this comment.
The CI run I pointed to is in: 4.2 s, 3.1 s and 4.7 s on the three lanes without the slots (they were 6.3 s, 5.8 s and 6.6 s with them, and 3.2 s, 2.5 s and 3.0 s in the first build). The lanes came down, but two did not return to about 3 s. The details are in my reply on the newer thread at line 15.
…cessors, and dispose of each shared folder Each command waits for a slot of the harness, so no more commands run at once than the machine has processors. With a debug build on 2 cores the longest command takes 49 s. Without the slots the builds with --aot took about 115 s each, and spawned() ends a command at 90 s where Bun has no sanitizer. The folders of the two shared builds are held in a DisposableStack. When one of them cannot be removed, the other is still removed.
There was a problem hiding this comment.
LGTM — the second commit addresses both earlier inline notes (commands now go through slot(), shared folders live on a DisposableStack), and the rest is a test-only tightening.
What was reviewed:
slot()wrapping:withEveryEntryPoint()is awaited before the outerspawned, so no slot is held while waiting for another — no deadlock againstavailableParallelism().- The
sub[\\/]typed\.jsregex against both producers:src/bundler/aot/options.rs:567emits...output path: sub/typed.jsandgenerateChunksInParallel.rs:444-453emits the path on its ownsub/typed.js:line, so both[]and--aotmatch. - The
readdirSync"nothing left behind" checks: neither fixture dir has a file starting withnone/equal, so they fail only if a build wrote one. DisposableStack.dispose()runs every registeredtempDirdispose and aggregates throws, so a Windows EBUSY on one folder no longer skips the other.
Extended reasoning...
The diff touches a single test file, test/aot/entry-point-names.test.ts, with no production code and no security-sensitive surface. It switches the file to test.concurrent, gates every spawned command on a processor slot via the existing slot() helper from test/aot/harness/commands.ts, shares one build between two tests per variant, and tightens assertions to whole {stdout, stderr, exitCode, signal} objects — the same shape the sibling in-place-executables.test.ts already uses. The two concerns raised on the first push were both fixed in commit 1c05c67, the path is not covered by CODEOWNERS, and the hunt ran dry, which decided approve.
…t a slot The file has five builds with --aot and a slot for each processor. On a machine with four processors the fifth build starts when the first one ends, and each run of an executable waits behind the four builds. With the slots the file took 5.8 s to 6.6 s on three CI lanes with four processors. Without them it took 2.5 s to 3.2 s there. The files beside it took the same time in both builds. The folders of the shared builds stay in the DisposableStack.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the shared-build plumbing: withEveryEntryPoint() assigns once synchronously before any await, so the two concurrent consumers cannot start two builds, and a failed build rejects both tests rather than hanging; shared.dispose() in afterAll now disposes the second folder even when the first rmSync throws; and spawned() returns exactly the four keys (stdout, stderr, exitCode, signal) that the new toEqual calls compare, so the stricter assertions are not vacuous or over-constrained. lastIndexOf(...) !== -1 is equivalent to includes for the listed-names check.
Extended reasoning...
The change is confined to one test file, test/aot/entry-point-names.test.ts, with no production code and no security-sensitive surface. It makes all tests concurrent, memoizes one build per mode in a module-level DisposableStack, and tightens assertions to exact stdout/stderr/exitCode/signal. The posted inline finding about the dropped slot() wrapper is the open item; the memoization, disposal ordering, and assertion shapes were checked against test/aot/harness/commands.ts and test/aot/supported.ts and found consistent.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
| // Each test has folders of its own. The two that ask one executable only read it. | ||
| const test = oneAtATime.concurrent; |
There was a problem hiding this comment.
🟡 (optional) The issue filed at test/aot/entry-point-names.test.ts:124 is back: commit 852c22d removed the slot() wrapper, so developers on a debug Bun without ASAN and few cores again get --aot builds killed at 90 s. With test.concurrent at line 15 all 11 tests start together and build() at line 107 spawns 12 builds with no slot; spawned() kills a build at limitOfARun = 90_000 (test/aot/harness/commands.ts:58) when isASAN is false. Fix: keep every build in this file behind slot() (or another bound on concurrent builds) so no single --aot debug build waits past 90 s on 2-4 cores, while keeping the runs of executables unbounded if the CI timing matters.
Why this was flagged
Trigger: bun bd --asan=off test test/aot/entry-point-names.test.ts on a machine with 2 to 4 cores. test/aot/entry-point-names.test.ts:15 makes every test concurrent and bun test's default max-concurrency is 20, so all tests start at once and build() at line 107 calls spawned() for each of 12 builds, 5 of them with --aot, with no slot() gate. In test/aot/harness/commands.ts:73-79 the timeout is limitOfARun = 90_000 (line 58) because isASAN is false and moreBuildFlags is empty; Bun.spawn kills the child with SIGKILL at line 89 and built() at commands.ts:170-175 fails on the exit code. The PR's own notes measured that without slot() five --aot debug builds on 2 cores take 112 to 122 s each, over that limit. On the base branch the tests ran in series, one build at a time, so no build competed for cores. Commit 1c05c67 had applied the remedy the resolved entry at line 124 asked for; commit 852c22d removed it for CI speed on 4-core release-profile lanes, which does not cover the non-ASAN debug case.
Verification: 852c22d deletes const spawned = (...command) => slot(() => spawnedAtOnce(...command)); build() at test/aot/entry-point-names.test.ts:107 calls it with no gate. test/aot/harness/commands.ts:58 limitOfARun = 90_000, chosen at lines 73-79 when isASAN is false; built() at 170-175 fails on the nonzero exit code. On the base the file used the serial test, so this is new.
There was a problem hiding this comment.
This is correct, and it is open. I have not pushed a change for it yet, for two reasons.
-
The reason I gave for removing the slots is not confirmed. With the slots, three CI lanes with four processors took about twice as long (one run for each lane). My explanation is a local trace: four slots, five
--aotbuilds, so the fifth build starts when the first ends. Build 135810 runs the head without the slots. If those lanes do not return to about 3 s, the explanation is wrong and I put the slots back. -
If the explanation holds, slots for the builds alone keep the cost. The command that waits in the trace is the fifth
--aotbuild, not a run. A bound equal to the number of processors protects a machine with 2 processors and costs a build length on a machine with 4. A larger bound removes the cost and, on 2 processors, lets four--aotbuilds run at once, which I have not measured.
One limit of my own numbers: the 112 to 122 s are from a debug build with ASAN and --max-concurrency=20, used in place of a build without a sanitizer. I have not run a debug build without ASAN.
The candidate I see is a gate that applies only to a debug build without a sanitizer, since that is the one configuration with the 90 s limit and slow builds. I will not push it before I can run such a build.
There was a problem hiding this comment.
Build 135810 has the three lanes. Same shard in each build, one run each:
| lane | no slots (135747) | slots (135759) | head, no slots (135810) |
|---|---|---|---|
| ubuntu 25.04 x64 | 3.2 s | 6.3 s | 4.2 s |
| ubuntu 25.04 aarch64 | 2.5 s | 5.8 s | 3.1 s |
| debian 13 aarch64 | 3.0 s | 6.6 s | 4.7 s |
I wrote that I put the slots back if these lanes do not return to about 3 s. Two of them did not. As I worded it, the condition is not met.
I keep the slots out all the same, for this reason: the file got slower on all three lanes when the slots went in and faster on all three when they came out, by about 2 s or more each time. The two builds without slots differ by 0.6 to 1.7 s on the same lane, so the direction is clearer than the size. I cannot say how much the slots cost. "Twice as long" was too strong.
This note is still not addressed. A debug build without a sanitizer on few processors can reach the 90 s limit with this file.
Behaviour change: none
Problem
test/aot/entry-point-names.test.tstakes 68 s on the darwin x64 lane (build #135385). The other lanes take 3 to 23 s.Fix
bun bd test test/aot/entry-point-names.test.tstakes 144 s before and 48 s after (medians of 5 and 8 runs). A release build takes 4.4 s before and 1.2 s after. The Notes have the CI lanes.Background
bun build --compile, with and without--aot, and starts it. A build writes a copy of Bun.setDefaultTimeout(240_000)stays. It equals the limit thatspawned()gives a command under ASAN. The default of the harness reaches only the first file of a process.beforeAllfor the shared build: it delays the other tests. Considered fewer executables: each of the 8 starts at another first entry point.Notes
Timings on one machine (Linux x64, 12 threads). The after column is the second commit. The head differs from it only by the slots, which do not bind on 12 threads. On 4 cores the head takes 1.34 s to 1.73 s (50 runs).
bun bd(debug, ASAN)da867c60b), 12 threads, 10 runstaskset -c 0-3), 10 runstaskset -c 0-1), 10 runs--aottakes 10 to 24 s. The disk and the five--aotbuilds set the time, and the runs vary with the state of the disk. Two pairs that ran one after the other: 147.2 s then 44.7 s, 144.0 s then 38.7 s.bun test --parallel=2 --max-concurrency=2withextensionless-entry-points.test.tsandexit-hooks-of-an-executable.test.ts.MemAvailabledrops by about 0.55 GB at the peak of the concurrent file. It dropped by 0.17 GB before.Timings in CI
DisposableStack. Build #135810 ran it, and no lane reports a failure of this file. I read its time on four lanes: the three in the next section, and darwin x64. For the other lanes I know only that the file is not in the list of failures.bundler_compile.test.tsfinishes one test every 1.85 s on that lane. This file starts 8 new executables. Concurrency lets the builds overlap and puts the 8 starts in the queue together. It does not remove a start.exit-hooks-of-an-executable.test.tstakes 5.4 s there and 0.30 s on a macOS 15 agent.test/aot/in-place-executables.test.tstimed out at 120 s, and the file passed on the retry. This PR does not touch that file. Main builds #135281, #135376 and #135526 also list it as failed once and passed on a retry on darwin x64, so it is not from this PR. I did not read which of its tests failed on main. It has the same pattern as this file (concurrent tests, new executables) and the 120 s limit of the harness. This file has a limit of 240 s.A slot for each command was tried and removed
slot()of the harness, after a review note about a debug build on few processors. In that case it helps: on 2 cores with a debug build (with ASAN, and 20 concurrent tests forced) the longest command takes 49 s with the slots. Without them the tests with an--aotbuild took 112 to 122 s each, andspawned()ends a command at 90 s where Bun has no sanitizer.test/cli/bun.test.ts: 118 to 230 ms on ubuntu x64, 178 to 441 ms on debian aarch64). The only code difference between those two builds is theDisposableStack. I have no sign that it costs time (the disposal takes 0.02 s in a local trace), and these runs cannot rule it out.--aotbuilds. The fifth starts when the first ends, and each run of an executable waits behind the four builds. On one Linux machine I could not measure a difference on 4 cores: both variants take 1.4 to 1.7 s there.The shared folders
DisposableStack.dispose()removes each folder, also when the removal of another one throws, and then throws that error.The time limit
spawned()ends a command at 90 s, and at 240 s under ASAN or withAOT_BUILD_FLAGS. A test limit below that would end the test beforespawned()ends the build and says what the build was doing.bun test --timeout=300 test/aot/entry-point-names.test.ts test/aot/exit-hooks-of-an-executable.test.ts: the second file has no limit of its own, and its test times out after 300 ms.Assertions
stdout.trim()and the exit code. After: the exact stdout,stderr: "",exitCode: 0,signal: null, in onetoEqualon whatspawned()returns.No output files to bundle. Before: an exit code that is not 0, then the text in stdout or stderr. After: the text on stderr, exit code 1, and no filenone*in the folder.Multiple files share the same output path. Before: an exit code that is not 0, then the text on stderr. After: the text on stderr followed by the shared outputsub/typed.js, exit code 1, and no fileequal*in the folder.expect()calls go from 28 to 26. The two that are gone are thebuilt()checks of the two builds that no longer run.lastIndexOfin place ofincludes. The answer is the same. The names are about 55 KB before the end of a file of 71 to 140 MB. In the debug build eachincludesread 1.6 GB and took about 3 s on the thread of the test runner.Also run
test/aot/test-files-self-skip.test.tsandtest/aot/every-test-runs-in-ci.test.tspass.prettier --check,oxlintandtscreport nothing for the file.no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/aot/entry-point-names.test.ts