Repository navigation
ci: run the fast, non-flaky test files as one bun test --parallel batch per shard - #36175
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds a generated parallel-test allowlist, updates test execution with parallel buckets and failure attribution, coordinates CI output groups, wires allowlist regeneration into CI, and removes explicit CPU metadata from EC2 test agents. ChangesParallel test execution
CI agent configuration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Collaborator
|
Updated 7:39 AM PT - Jul 28th, 2026
❌ @Jarred-Sumner, your commit 90e8118 has some failures in 🧪 To try this PR locally: bunx bun-pr 36175That installs a local version of the PR into your bun-36175 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.buildkite/ci.mjs (1)
368-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the EC2 descriptor contract.
These changes remove
cpu-countandthreads-per-corefrom every Windows/Linux test-agent path. Add or confirm tests asserting that"instance-type"remains present while both CPU-limit fields are omitted across all branches.Also applies to: 425-429, 443-457
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.buildkite/ci.mjs around lines 368 - 370, Add regression coverage for the EC2 descriptor branches that construct test-agent configurations, including the paths around the existing "instance-type" assignments and the additional Windows/Linux branches. Assert that "instance-type" is present while "cpu-count" and "threads-per-core" are omitted in every branch, preserving the current descriptor values and behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/runner.node.mjs`:
- Around line 971-1029: Add a runner-level regression fixture covering the
attribution logic around byPath, group fallback, ::error file annotations,
worker-crash lines, and unattributed failures. Assert the exact files rerun
individually for each scenario, including the all-files fallback when no file is
attributed, and verify flaky Buildkite annotations are emitted only for blamed
files that pass alone. Keep the existing JSON-shape test unchanged and exercise
the behavior through the runner’s public test flow.
- Around line 953-967: Update runParallelBucket() so its spawnBun test arguments
include --reporter=junit and --reporter-outfile pointing to the bucket’s
expected JUnit output file when --junit is enabled. Preserve the existing
arguments and ensure each bucket writes a distinct XML file consumed by the
existing upload loop.
In `@scripts/update-parallel-allowlist.mjs`:
- Around line 116-120: Update the build annotation retrieval around the
api(`builds/${build}/annotations?per_page=100`) call to fetch and combine every
pagination page before evaluating annotations. If any selected build’s
annotation request fails, stop allowlist generation instead of logging and
continuing, ensuring incomplete annotation history cannot produce output. Add
regression coverage for multi-page results and fetch failures.
In `@src/runtime/cli/test/parallel/Coordinator.rs`:
- Around line 310-320: Update Coordinator::ensure_header() to emit a matching
GitHub Actions ::endgroup:: before opening the next file’s ::group::, and ensure
the final group is closed when reporting completes. Preserve non-GitHub output
and existing file-header behavior.
---
Outside diff comments:
In @.buildkite/ci.mjs:
- Around line 368-370: Add regression coverage for the EC2 descriptor branches
that construct test-agent configurations, including the paths around the
existing "instance-type" assignments and the additional Windows/Linux branches.
Assert that "instance-type" is present while "cpu-count" and "threads-per-core"
are omitted in every branch, preserving the current descriptor values and
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ea80093-fdb4-496e-8ced-61bf8afa238e
📒 Files selected for processing (8)
.buildkite/ci.mjs.buildkite/update-test-durations.ymlpackage.jsonscripts/runner.node.mjsscripts/update-parallel-allowlist.mjssrc/runtime/cli/test/parallel/Coordinator.rstest/internal/parallel-allowlist.test.tstest/parallel-allowlist.json
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/cli/test/parallel/Coordinator.rs (1)
148-178: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose any open
::group::before printing the interrupted/not-started summary.
abort_allnever callsself.end_group(). If a file's group is still open when SIGINT/SIGTERM fires, the new "Interrupted while still running" / "N file(s) had not started" listing (lines 170-175) prints inside that group, so it renders folded/collapsed in the GitHub Actions log viewer — exactly the diagnostic text a hung/timed-out CI run needs to be visible.🔧 Suggested fix
fn abort_all(&mut self) -> ! { abort_handler::uninstall(); + // Surface the interrupted-run summary at top level, not folded + // inside whatever file's group happened to still be open. + self.end_group(); // Name what was still running so a stalled run is diagnosable from🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/cli/test/parallel/Coordinator.rs` around lines 148 - 178, Update abort_all to close any active output group by calling self.end_group() before printing the interrupted or not-started summaries. Keep the existing running-worker and pending-file diagnostics unchanged, ensuring they render outside the group and remain visible in CI logs.scripts/runner.node.mjs (1)
642-645: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
validationAppliesinstead of re-deriving the condition.The exact same expression (
basename(execPath).includes("asan") || !isCI) is already inlined at lines 865 and 869. Now thatvalidationAppliesexists, those two sites should reference it to avoid the conditions silently diverging later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/runner.node.mjs` around lines 642 - 645, Replace the duplicated basename(execPath).includes("asan") || !isCI checks at the sites around lines 865 and 869 with the existing validationApplies variable. Keep the surrounding logic unchanged so both sites use the same validation condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/runner.node.mjs`:
- Around line 642-645: Replace the duplicated
basename(execPath).includes("asan") || !isCI checks at the sites around lines
865 and 869 with the existing validationApplies variable. Keep the surrounding
logic unchanged so both sites use the same validation condition.
In `@src/runtime/cli/test/parallel/Coordinator.rs`:
- Around line 148-178: Update abort_all to close any active output group by
calling self.end_group() before printing the interrupted or not-started
summaries. Keep the existing running-worker and pending-file diagnostics
unchanged, ensuring they render outside the group and remain visible in CI logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40777ea2-daa4-416a-8015-2bab881d0d0b
📒 Files selected for processing (2)
scripts/runner.node.mjssrc/runtime/cli/test/parallel/Coordinator.rs
Comment on lines
+953
to
+968
| const { ok, error, stdout, crashes } = await startGroup(`${range} ${label}`, () => | ||
| spawnBun(execPath, { | ||
| args: [ | ||
| "test", | ||
| `--parallel=${width}`, | ||
| `--timeout=${perTestTimeout}`, | ||
| "--dots", | ||
| ...bucketFiles.map(t => join(testsPath, t)), | ||
| ], | ||
| cwd, | ||
| timeout: Math.max(10 * 60_000, bucketFiles.length * 5_000), | ||
| gracefulTimeout: true, | ||
| env, | ||
| stdout: chunk => pipeTestStdout(process.stdout, chunk), | ||
| stderr: chunk => pipeTestStdout(process.stderr, chunk), | ||
| }), |
Contributor
There was a problem hiding this comment.
🟡 The bucket coordinator gets one mkdtempSync dir from spawnBun (line 1611) as TMPDIR/BUN_INSTALL_CACHE_DIR, and every worker inherits it — so all ~80 bucketed files now share one tmpdir and one install cache, whereas the per-file path this replaces gave each file a private mkdtempSync that was rmSync'd afterward. cli/install and cli/install/migration are in the allowlist, so concurrent bun install runs share BUN_INSTALL_CACHE_DIR — a parallel-only condition the allowlist's per-file zero-flake history could not have observed. It's fail-safe (a flake re-runs solo and gets the flaky annotation), but since run_as_coordinator already builds a per-worker envp for JEST_WORKER_ID, appending a per-worker TMPDIR/BUN_INSTALL_CACHE_DIR subdir there would preserve the isolation the history was measured against.
Extended reasoning...
What changes
runParallelBucket calls spawnBun() once for the whole bucket. spawnBun (scripts/runner.node.mjs:1611-1631) creates a single mkdtempSync(join(tmpdir(), 'buntmp-')) directory and sets it as TMPDIR, BUN_TMPDIR, TEST_TMPDIR, and BUN_INSTALL_CACHE_DIR on the coordinator process. The coordinator then spawns N workers whose envp is env.map.create_null_delimited_env_map() (src/runtime/cli/test/parallel/runner.rs:140-147) — a serialization of the coordinator's process environment with only JEST_WORKER_ID/BUN_TEST_WORKER_ID varied per slot. So every worker, and every test file the bucket runs, inherits the same os.tmpdir() and the same BUN_INSTALL_CACHE_DIR.
The one-process-per-file path this replaces (spawnBunTest → spawnBun) creates a fresh mkdtempSync per file and rmSync's it in finally (line 1751). Each file's os.tmpdir() and install cache were private, and were cleaned between files. That per-file isolation is what the allowlist's zero-flake history (300 builds × per-file runs) was measured against; per-bucket sharing is a condition that history cannot have observed.
Step-by-step
- A shard's bucket has ~80 allowlisted files across 4 workers.
runParallelBucketcallsspawnBun(execPath, {args: ['test', '--parallel=4', ...]}). spawnBunat line 1611:tmpdirPath = mkdtempSync(join(tmpdir(), 'buntmp-'))→ e.g./tmp/buntmp-abc123. SetsTMPDIR=BUN_TMPDIR=TEST_TMPDIR=BUN_INSTALL_CACHE_DIR=/tmp/buntmp-abc123on the coordinator.run_as_coordinator(runner.rs) builds each worker's envp: for each of 4 slots,env.map.put(b"JEST_WORKER_ID", id)thenenv.map.create_null_delimited_env_map().TMPDIRandBUN_INSTALL_CACHE_DIRpass through unchanged → all 4 workers see/tmp/buntmp-abc123.cli/installandcli/install/migrationare inparallel-allowlist.json'sdirs. Files not inexcludeFiles(e.g.npmrc.test.ts,bun-pm.test.ts,bun-install-patch.test.ts,migration/*.test.tsother than the two excluded) runbun installwithBUN_INSTALL_CACHE_DIR=/tmp/buntmp-abc123concurrently across workers.harnesstempDir()/tmpdirSync()(test/harness.ts) dofs.mkdtempSync(join(realpathSync.native(os.tmpdir()), ...)), so every file's temp trees root under the same/tmp/buntmp-abc123for the whole bucket lifetime, and nothing isrmSync'd between files.
Why this is a nit, not blocking
- Fail-safe by design. If a bucketed file flakes because of shared-cache contention, it fails, is re-run solo through
runOneTest(which callsspawnBunand gets a fresh private tmpdir), and — if it then passes — is annotatedflakyon Buildkite. That annotation feeds straight into the nextupdate-parallel-allowlist.mjsregen, which drops the file intoexcludeFiles. Nothing is silently marked green. mkdtempSyncstill gives unique subdirs.tempDir()/tmpdirSync()create a unique directory under the shared base per call, so working-directory paths never collide; only tests that globos.tmpdir()itself, or that race on the sharedBUN_INSTALL_CACHE_DIR, are affected.- Shared install cache is the user-facing default. Concurrent
bun installinvocations sharing~/.bun/install/cacheis how bun runs on a dev machine; the cache is designed for it. The heaviest install tests that stress this (bun-install.test.ts,bun-add,bun-install-registry,bun-workspaces,hoist,isolated-install,bunx,migration/complex-workspace,migration/pnpm-migration) are already inexcludeFiles.
So merging without this doesn't break anything observable — at worst it produces a few extra flaky annotations that the existing feedback loop absorbs.
Why it's still worth noting
The allowlist's inclusion criterion is "zero flaky/failed annotations across 300 builds", and that history was gathered under strictly stronger isolation than the bucket provides. Changing the isolation model out from under a history-derived allowlist means the first few bucket runs are also the first observation of the new condition. The fix is nearly free: run_as_coordinator already builds a distinct envp per worker slot to append JEST_WORKER_ID; appending TMPDIR/BUN_INSTALL_CACHE_DIR = <coordinator-tmpdir>/w<i> (and make_path'ing it) alongside would restore per-worker isolation with no runner.node.mjs change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/runner.node.mjs`:
- Around line 990-999: Update the JUnit parsing loop around the suites Map and
matchAll call to parse only file-level testsuite elements by requiring the
suite’s name attribute to equal its file attribute, since both are set from the
same name for file suites. Continue recording the file-normalized path and
aggregate failures only for those entries, preventing nested suites from
overwriting file results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8af73ae9-fd22-442d-b320-cf0e5aeebf25
📒 Files selected for processing (2)
scripts/runner.node.mjssrc/runtime/cli/test_command.rs
Jarred-Sumner
changed the title
bun test --parallel bucketsbun test --parallel batch per shard
Adds a parallel-bucket phase to the CI runner. Each shard now runs the PR's modified test files first, then every remaining file one process at a time as before, then the allowlisted fast/non-flaky bun test files as up to four sequential `bun test --parallel` invocations, and finally the existing node parallel-safe phase. The two wide phases run last so no serial test overlaps them. The allowlist (test/parallel-allowlist.json) is a set of test/-relative directories plus per-file carve-outs, generated by scripts/update-parallel-allowlist.mjs from the checked-in duration table and the flaky/failed annotations of the last 300 finished PR builds: a directory qualifies when at least 2/3 of its files are <=15s median with zero flake or failure annotations in that window. Docker-service and node-style directories are never bucketed. Per-file attribution comes from the junit report each bucket writes: any file whose testsuite is missing (load error, top-level throw, dead worker) or reports failures is re-run individually through the normal retry/annotation path; a file that fails in a bucket but passes alone is annotated as flaky. Files opted out of exception/leaksan validation stay serial when validation applies, since a bucket shares one env. Also drops the cpu-count/threads-per-core agent tags from the EC2 test agents, which had the launcher clip 4- and 8-vCPU instances to 2 cores; without the tags each box comes up with its full core count. The scheduled durations job now regenerates the allowlist artifact too. No-Verification-Needed: user requested shipping without further checks
The `--parallel` coordinator already balances files across its workers, so batching the allowlisted set into four sequential invocations only inserted barriers; each shard now makes one `bun test --parallel` call for the whole set, with `--dots` kept for a compact console. Failure attribution no longer reads a junit report back. It parses the bucket's own GITHUB_ACTIONS output: `::error file=<path>` annotations, the coordinator's `worker crashed` lines, and a fileless `::error` charged to the `::group::` block it printed under. Those files re-run individually (retries + annotation as before); a failure that names no bucket file re-runs the batch quietly instead of annotating every solo pass as flaky — the first CI run showed a single worker crash fanning out into hundreds of bogus flaky entries via that fallback. The parallel coordinator printed a file's failure output with no header under --dots and without the `::group::` prefix under GITHUB Actions; the header is now emitted lazily on a file's first non-dot line, matching the serial reporter. No-Verification-Needed: user requested shipping without further checks
In the first CI run every >10-minute shard was a parallel bucket that went silent until the runner's timeout: setTimeout.test.js (debian and ubuntu), tty-reopen-after-stdin-eof.test.ts (both alpine) and mmap.test.js (windows 2019) each hung their worker without printing. Keep them on the one-process-per-file path. No-Verification-Needed: user requested shipping without further checks
`bun test --parallel --dots` printed a failing file's output with no header, and the coordinator's header lacked the `::group::` prefix under GITHUB_ACTIONS; the worker also printed its own `::endgroup::` per file, chopping dot runs into one line per file. The coordinator now prints the header lazily on a file's first non-dot line (matching serial --dots), writes it in one syscall so the `::group::` line reaches the log intact, and owns group open/close; the worker emits no group markers. parallel.test.ts's dots case asserted "no file headers", a rule serial --dots never followed; it now asserts the serial rule. On SIGTERM/SIGINT the coordinator names each file still running and how many never started, so a stalled batch is diagnosable from the log. The CI runner's bucket timeout is SIGTERM-first (SIGKILL 15s later) to let that print, labels the run a timeout from its own flag, and re-runs the whole batch quietly whenever the coordinator didn't exit on its own instead of trusting partial per-file output. Stress-named tests are excluded from the parallel allowlist by both the generator and the runner.
The interrupt report from `bun test --parallel` now lists the files that were still running and, by name, the files that never started. The CI runner uses that report plus the per-failure `::error`/worker-crash lines to re-run only files that failed, hung, or never ran; files the batch passed are not touched. A bucket-fail-then-solo-pass is annotated as a parallel-mode flake only for files the batch actually ran and blamed. The whole batch re-runs solely when the coordinator dies without any report to trust. The runner's bucket timeout is a single SIGTERM (Node's own timeout kill disabled) so the coordinator is not signaled twice mid-abort, and the run is labeled a timeout from the runner's own timer rather than from exit-code timing. No-Verification-Needed: user requested shipping without further checks
Each failed at least once inside a bun test --parallel batch on the last CI run and then passed when re-run alone. No-Verification-Needed: allowlist data only, guard test passes
…start `bun test`'s junit reporter opened a file's <testsuite> on its first test result and closed it when the next file's first result arrived, and its `time=` was the sum of test bodies in integer ms — so a file's time landed on its predecessor, sub-ms files read as 0, and load/hook time was uncounted. The file loop now stamps a monotonic u64 ns at each file's real start and end, and a file-level suite's `time` is that end minus start (nested describe suites keep the summed metric). Same code path serves serial and --parallel worker fragments. The CI runner's parallel bucket adds `--reporter=junit` (with --dots for the console) and reads the per-file outcome from it: failures count and wall clock per file, no suite meaning the file didn't finish. That report only exists once the coordinator completes, so when it's absent the runner falls back to what streamed (`::error file=…`, worker-crash lines, the interrupt report's still-running / not-started lists); a batch with neither re-runs whole. A file the batch passed is never re-run.
A deliberately-failing test used to exercise the parallel-bucket retry path was left in the file by mistake. No-Verification-Needed: reverts a stray debugging edit
A `--parallel` worker on Windows crashed with `preload not found "./test/preload.ts"`: an earlier file in that worker had process.chdir'd away and the cwd leaked into the next file, so the relative bunfig preload no longer resolved. `process.chdir()` under `--isolate` now records the file's original cwd in a per-file `TestIsolationState` (reachable through `VirtualMachine::test_isolation_scope`, which is a no-op outside isolated runs), and the global-object swap chdirs back before the next file. The chdir + resolver-cwd sync moves into `VirtualMachine::set_process_cwd` so both directions share it. A worker that died mid-run also discarded the junit suites of every file it had already finished (fragments were written only at exit), which read as dozens of "unfinished" files. Each worker now rewrites its fragment after every file, and the coordinator merges a dead worker's fragment by its deterministic path. Runner side: the parallel bucket prints each file's junit wall clock into the log (and the durations updater reads those lines, since bucketed files no longer get a timed header of their own); a bucket-fail-then- solo-pass annotation carries that file's output block; and the two files found wedging their buckets (ctrl-c, bun-run-bunfig) join the exclusions.
Each node-napi-tests do.test.ts compiled its own addon on demand, which on Windows meant one multi-second node-gyp/MSBuild bootstrap per file — several at once under bun test --parallel. The runner now hands only the addon directories this shard will run to prebuild.ts, which builds them in one go: the harness's direct-compile fast path as a bounded clang fan on posix, and on Windows a single node-gyp run over one merged binding.gyp (targets namespaced by directory, artifacts copied back under the names their tests expect). The prebuild starts right after the installs without blocking anything; only the first do.test.ts to run awaits it. harness build() short-circuits when build/Debug/<target>.node already exists, and a node-gyp failure throws instead of exiting the process so one bad addon can't end the batch — its own test rebuilds it and reports the error as before.
… files as regular groups `bun test --parallel` gathered junit and LCOV by having each worker write fragment files into a shared temp dir and announce their paths over the IPC channel, with the coordinator reading them back to merge — and my attempt to keep a crashed worker's finished files (rewriting the fragment after every file) called the reporter's one-shot finalizer repeatedly, so the merged report grew a closing tag per file and the merged-junit test failed on every lane. Replace the file dance with the channel that was carrying the paths anyway: a worker sends each file's completed <testsuite> as a JunitChunk frame right after the file and its LCOV as a CoverageChunk at exit; the coordinator concatenates chunks in memory and is the only process that writes a report. Worker reporters are elements-only (no XML prolog / <testsuites> header — the coordinator owns the document), a crashed worker loses nothing that finished, and the worker temp dir, fragment files, and merge-time re-parsing are gone. The CI runner drops --dots for the parallel bucket and instead renders each file's streamed `::group::<path>` as the runner's own `[i/N] <path>` group, so a bucketed file reads like any other test in the log and the existing durations job times it header-to-header; the ad-hoc timings group and its scraper branch are removed. The junit still drives which files failed or never finished.
Collaborator
|
Heads up: after this widened the parallel-safe phase from 1 to 3, |
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jul 31, 2026
…ng (#36478) Fixes `test/js/node/test/parallel/test-fs-read-stream-pos.js` going red on main (seen on builds 84352, 84676, 85400, 85630, 85664, 85692, 85720-85866 across win2019, alpine/ubuntu/debian aarch64, and debian x64-asan). ### Cause The test's exit path is a pure timing race: a `setInterval(append, 1)` writer must land a write between two consecutive `ReadStream` preads within a single stream instance (partial chunk → another `'data'` before `'end'`). Upstream ships it with a 90-second safety timer for exactly that reason. In addition, each stream's `'data'` handler is wrapped in `common.mustCallAtLeast(1)`, so any stream that observes zero bytes fails the process on exit. Two recent PRs together pushed both failure modes over the runner's 20 s ceiling: * #34834 raised Windows event-loop timer resolution from ~15 ms to ~1 ms. With the old resolution the writer and reader both fired on the same ~15 ms tick and each stream covered ~1 write (so the race hit in ≤25 cycles); with 1 ms resolution each stream covers ~10 writes, the partial chunk is always last, and the per-stream hit rate drops to ~1/500. 30 solo runs on a Windows box: 1 s–40 s, max 40 s, 5 runs >20 s. * #36175 removed the `cpuCount: 2` clamp on the linux/windows test agents, taking the parallel-safe phase from width 1 to width 3. Running alongside `test-fs-read-stream-fd-leak.js` (50× `createReadStream` at 2 ms) starves the 1 ms appender long enough for a later stream to be created with `start == EOF`, producing zero `'data'` events and failing `mustCallAtLeast` with "Mismatched function calls. Expected at least 1, actual 0." (e.g. build 85400 debian aarch64, attempts 1–2). Neither changes what the test is actually checking (ReadStream position tracking while a file is being appended, nodejs/node#33940), which has never failed here; all observed reds are the race not being reached inside the runner's 20 s window, or the per-stream `mustCallAtLeast` tripping under I/O contention. ### Change In `scripts/runner.node.mjs`: * `isParallelSafeTest` now returns `false` for this file, so it runs in the serial phase. With no I/O neighbours the 1 ms writer keeps its cadence and every stream sees at least one write, so the `mustCallAtLeast` failure cannot occur. * `getNodeParallelTestTimeout` gives it 120 s, so on the occasions the race does not hit within 90 s the test's own safety timer fires and the process exits 0 (Node's python runner also allows 120 s per `test/parallel` file). Same assertions and code path; only the runner's scheduling of this file changes. ### Verification * Windows x64 solo, 30 consecutive runs with the release canary: 30/30 pass, times 1 s–40 s (median ~6 s), none reached the 90 s safety timer, none exited non-zero. * `node scripts/runner.node.mjs --include js/node/test/parallel/test-fs-read-stream-pos.js` on linux: file is scheduled in the serial phase (not under "Running N parallel-safe tests"), passes. * `bun test test/internal/parallel-allowlist.test.ts`: 2/2 pass. The event-loop ordering difference (Bun drains queued thread-pool completions before yielding to due timers, Node yields to the timer phase between poll iterations) is a pre-existing behaviour not introduced by either of the two PRs above; changing it is out of scope for unbreaking this test. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · build/CI scripts only; test-proof not applicable <!-- robobun:evidence:end -->
robobun
added a commit
that referenced
this pull request
Jul 31, 2026
…ibling parsers Since #36175, runner.node.mjs also emits: --- napi prebuild: N addon(s), Xs --- [A-B/M] K files in parallel (Nx) [N/M] <path> (X.XXs) (bucket per-file summary) none of which match '^--- [N/M] <path>'. Close the open span on any '--- ' group header so the test preceding the bucket is not charged the whole bucket run, and read the inline '(X.XXs)' timing from bucket summary lines. buildkite-slow-tests.js and update-test-durations.mjs share the same header contract and get the same fix.
This was referenced Jul 31, 2026
Closed
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jul 31, 2026
## Problem The parallel-batch allowlist (`test/parallel-allowlist.json`) was generated from builds 82146-83745, which predate #36175 (the PR that introduced the parallel batch). The generator's rule is "zero flaky/failed annotations in the scanned builds", but at that point no test had ever run inside a shared `bun test --parallel` process, so the allowlist couldn't know which tests are incompatible with in-process parallelism. The result: tests that measure process-wide state (`heapStats().objectTypeCounts`, `rss()`, `expectMaxObjectTypeCount`) or have tight wall-clock budgets were included, and they now flake on nearly every build. Across the last 400 completed PR builds, the top "failed in the parallel batch; passed alone" annotations by distinct build count: | test | builds (of 400) | | --- | --- | | `test/js/web/streams/streams-leak.test.ts` | 348 | | `test/js/bun/shell/leak.test.ts` | 340 | | `test/js/bun/net/socket-retention.test.ts` | 334 | | `test/js/web/fetch/fetch-tls-abortsignal-timeout.test.ts` | 282 | | `test/js/bun/spawn/spawn-streaming-stdout.test.ts` | 267 | | `test/regression/issue/08965/08965.test.ts` | 261 | | `test/regression/issue/20875.test.ts` | 151 | | `test/js/node/http/node-http-pinned-write.test.ts` | 110 | | `test/regression/issue/27272.test.ts` | 103 | | `test/js/third_party/esbuild/esbuild-child_process.test.ts` | 103 | Each of these generates a yellow annotation + serial retry on most builds. ## Fix Re-ran `scripts/update-parallel-allowlist.mjs` (300 PR builds, 85133-86444). 609 distinct files flaked or failed in that window; the generator drops them per its existing rule. - 282 dirs -> 215 - 1642 files -> 1296 eligible for the batch - 153 -> 160 explicit excludes All six of the top offenders above are now excluded (either via `excludeFiles` or their dir dropped). ## Verification `streams-leak.test.ts` asserts `rss() < 250MB`; in the batch it sees 390MB-2.6GB from neighbouring tests. `shell/leak.test.ts`'s `#11816` tests assert `heapStats().objectTypeCounts.ShellInterpreter <= 3` in-process; any concurrent test using `$` contributes. `socket-retention.test.ts` and `spawn-streaming-stdout.test.ts` use `expectMaxObjectTypeCount` similarly. These are correct tests in an isolated process and cannot be made parallel-safe without moving the assertions into a subprocess; excluding them from the batch is what the generator already does once it has data. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · docs-only change; test-proof not applicable <!-- robobun:evidence:end -->
This was referenced Aug 2, 2026
bun test: --timings for duration-balanced --shard/--parallel, docs for --parallel & --isolate
#36814
Merged
This was referenced Aug 4, 2026
This was referenced Aug 6, 2026
Closed
alii
added a commit
that referenced
this pull request
Aug 16, 2026
…ctory (#39236) Since #36175 each shard runs the files in `test/parallel-allowlist.json` as one `bun test --parallel` batch and everything else one at a time. On a macOS shard that one-at-a-time phase is **9.6 of 13.5 minutes** (389 files). Classifying those 389 against the allowlist rules: 241 of them (2.3 min) are fast and never flaky on their own and were kept serial only because the generator listed a **directory** just when two thirds of it qualified, so one slow or flaky neighbour held back whole directories. **`scripts/update-parallel-allowlist.mjs`** - Qualifies each file by itself: same test as before (fast, zero flaky/failed annotations in the scanned builds, not docker-service or stress). Every directory with a qualifying file is listed and the rest of that directory goes to `excludeFiles`, so the JSON shape and `runner.node.mjs` are unchanged. - "Fast" now means the **slowest lane** in `expected-durations.json`, not only the Linux median, so a file that is quick on Linux but slow on Windows or ASAN does not hold a bucket slot for its whole run. **`test/parallel-allowlist.json`** regenerated from the last 300 finished PR builds (#97649..#98676): **1744 files eligible (was 1296), 286 dirs (was 215), 242 excluded (was 160)**. Dirs +84 −13; the 13 dropped are single-file napi/comlink dirs whose only file flaked in the fresh window (same rule, newer data). Expected effect: ~2 min less serial time per macOS shard (~17%), similar on the Linux lanes, no test skipped. Safety net is unchanged: a file that fails in the batch is re-run alone, and if it then passes it gets the `flaky` annotation ("in the parallel batch … passed alone"), which demotes it at the next regen. What stays serial on that macOS shard after this, for context: 33 files >15s on some lane (4.6 min: `js/web/fetch/body-stream`, `fetch.test`, `bake/dev/*`, `expo`, `cron`, …), 41 recently-flaky files (2.0 min), 42 node-test/stress/docker files (0.4 min). Those need per-file work (splitting the slow ones, deflaking), not allowlisting.
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 26, 2026
…it report (#37483) ### Problem `scripts/runner.node.mjs` runs the parallel-safe bucket as one `bun test --parallel --reporter=junit` (#36175) and reads the report to decide which files failed, what to re-run alone, and the `(X.XXs)` it prints per file. It collected the suites like this: ```js for (const [, attrs] of xml.matchAll(/<testsuite\b([^>]*)>/g)) { const file = /\bfile="([^"]+)"/.exec(attrs)?.[1]; ... if (file) suites.set(file, { failures, seconds, cases: [] }); } ``` bun's reporter writes one `<testsuite name="<path>" file="<path>">` per file and, nested inside it, one `<testsuite name="<describe>" file="<path>">` per describe block (`begin_test_suite_with_line` in `src/runtime/cli/test_command.rs` puts `file=` on both kinds). Every suite of a file therefore overwrote the previous one, and a file with describe blocks ended up represented by whichever describe suite came last in the report. Report written by the released bun for a file with a failing top-level test, a failing `describe("first")` and a passing `describe("second")`, plus a file with a `describe.concurrent` of three 100ms tests: ```xml <testsuite name="test/a.test.ts" file="test/a.test.ts" tests="4" failures="2" time="0.002066623" ...> ... <testsuite name="first" file="test/a.test.ts" tests="1" failures="1" time="0" ...> <testsuite name="second" file="test/a.test.ts" tests="2" failures="0" time="0" ...> </testsuite> <testsuite name="test/b.test.ts" file="test/b.test.ts" tests="3" failures="0" time="0.100953121" ...> <testsuite name="conc" file="test/b.test.ts" tests="3" failures="0" time="0.3" ...> </testsuite> ``` The runner's loop turned that into `a.test.ts: { failures: 0, seconds: 0 }` and `b.test.ts: { seconds: 0.3 }`: * `a.test.ts` is not added to `failed`, and because the report exists (`evidence` is true) nothing is re-run: the file is printed as passed and pushed to `okResults`, so the bucket's non-zero exit is swallowed and the job goes green. Any bucket file whose failures are at top level or in a describe block other than the last one is affected. * The per-file time is a describe block's sum of test durations instead of the file suite's wall clock (`end_test_suite` times file suites from `file_start_ns` to `file_end_ns` and describe suites by summing their tests), so `describe.concurrent` files over-report and files with several describes under-report. These lines feed `scripts/ci-slowest-tests.ts` and `scripts/update-test-durations.mjs`. * The failing cases are attached to that same entry, and since the file is not classified as failed they are never printed in the "failing in the parallel batch" group or the flaky annotation. ### Fix The parsing moves to `parseJunitFileSuites()` in `scripts/utils.mjs` (the runner's existing helper module; `runner.node.mjs` itself runs on import, so it cannot be unit tested) and only keeps suites whose `name` is their `file`, which is how the reporter writes file suites. That is the right suite to read on its own terms: `end_test_suite` adds each closed suite's metrics to its parent, so the file suite's `failures` includes every nested describe block, and its `time` is the only wall clock measurement in the report. The synthetic suite the `--parallel` coordinator writes for a file whose worker crashed or was interrupted (`merge_junit_fragments` in `src/runtime/cli/test/parallel/aggregate.rs`) has the same `name == file` shape, so crashed and hung files are still classified as failed. The `<testcase>` loop is unchanged: it keys on each case's own `file` attribute. The runner's classification, printing and annotation code is unchanged apart from calling the helper. #36218 (in flight) keeps the same file-suite shape, so it is unaffected. The earlier draft of the batch runner, #29654, applied this same `name === file` rule in its `parsePassedFilesFromJunit()`; the rule did not make it into the version that landed in #36175. #29654 is a rewrite of the whole batch mechanism and conflicts with main, so this PR only restores the rule in the code that shipped. ### Tests `test/internal/runner-junit.test.ts`: * a hand-written report in the reporter's shape (top-level failure, a failing describe, a passing describe with a nested describe last, a second plain file): the file is reported with `failures: 2`, the file suite's `time`, and both failing cases with their names and messages unescaped; * Windows-style backslash paths and escaped characters are keyed the way the runner looks files up; * the coordinator's crashed-file suite is reported as one failure with no cases; * a real `bun test --parallel=2 --reporter=junit` run over a file with describe blocks, a plain file and a file that calls `process.exit()`: the report contains four suites carrying the describe file's path, and the parsed result has the file suite's counts, its `time`, the two failing cases, and the crash marker. With the previous behaviour (every suite with a `file` attribute) the first and last of these fail (`failures: 0` and the last describe block's time for the file with describe blocks); with this change all five pass under `bun bd test` and with the released bun. `node scripts/runner.node.mjs --exec-path ./build/debug/bun-debug js/web/encoding` still forms the 8-file bucket, prints the file suites' times, and re-ran the one file the interrupted batch marked as failed.
6 of 8 tasks
This was referenced Sep 6, 2026
This was referenced Oct 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Each test-bun shard now runs its fast, historically non-flaky
bun testfiles as onebun test --parallelbatch instead of a process per file, gives the EC2 test agents their full core count, and fixes the--parallelrunner bugs the extra usage surfaced.Per-shard phases:
bun install→ the PR's modified test files → every other file one at a time (docker-backed last) → allowlisted files as a singlebun test --parallel=<width>batch → the existing node parallel-safe phase. Both wide phases run only after all serial files finish.Allowlist (
test/parallel-allowlist.json, regenerated byscripts/update-parallel-allowlist.mjsand the weekly durations job): a directory is batched when ≥⅔ of its files are ≤15 s median with zero flaky/failed annotations across the last 300 finished PR builds; stragglers, stress-named tests, and docker-service dirs are excluded (~1.5k files eligible, ~150 carve-outs).Batch reporting: the batch's junit is the record — per-file wall clock, which files failed / never finished. Passed files print as
[i/N] <path> (T.TTs)label lines (the durations job reads the time off the title; the junit is uploaded as the shard's artifact), a file that failed in the batch gets a group with its junit failures and re-runs alone (a batch-fail-then-solo-pass is annotated as flaky, with the failure in the title), and only a coordinator that dies with no report re-runs the whole batch. Failure titles name the leak site / unchecked-exception location / first failing assertion instead of the exit signal.bun test --parallelfixes (found by running it at scale):process.chdir()leaked into the next file its worker ran (Windows workers then diedpreload not found "./test/preload.ts")VirtualMachine::test_isolation_scope)<testsuite>(and their LCOV) over the existing IPC channel; only the coordinator writes reports; merged output is deterministic (file order)--dotsprinted a failing file's output with no header; group markers unpairedAlso: node-napi-tests addons for a shard are built once, up front, in one process (bounded clang fan on posix; one merged node-gyp/MSBuild run on Windows) instead of per test file; and the
cpu-count/threads-per-coreagent tags that clipped the linux/asan test agents to 2 usable cores are removed.Measured
Per-lane test-bun shard time, current
main(build #83936) vs this branch (build #84092):The asan row's baseline is the last
mainbuild that ran that lane (#83936 had none). The larger maxes on a few linux lanes are one honest ~6-minute batch, not a stall: no shard on any lane hit the batch cap. The lanes moved once the test agents booted with their full core count (they had been launching withCpuOptionsclipping a 4-vCPU box to 2), which is also what removed the 10-minute wedge tail earlier builds showed.Flaky-annotation volume per build dropped from ~250 phantom entries (early fallback fan-out) to ~15 real ones.
How did you verify your code works?
Drove
scripts/runner.node.mjslocally over allowlisted directories with injected failures (batch → junit attribution → solo retry, wedge → interrupt report → named files), addedtest/cli/test/parallel.test.tscases for the crash-mid-run junit (fails on the old fragment-file design) and a multi-MB single-file report over the channel, ran the file against the debug build (26/27; the pre-existing worker-id scheduling case fails locally onmaintoo), and read the per-lane numbers off the branch's own CI builds.