test(spawn): speed up spawn-noread-leak.test.ts (27s -> 2s on Windows arm64) - #35427
test(spawn): speed up spawn-noread-leak.test.ts (27s -> 2s on Windows arm64)#35427robobun wants to merge 3 commits into
Conversation
… arm64) - Reduce total spawns 3000 -> 750 (3 warmup + 12 measure batches of 50). - Spawn native `cmd /c echo x` on Windows instead of msys2 `cat` (~2.5 ms vs ~9 ms per spawn), and `echo x` instead of `cat` on POSIX. - Pipe stdout with 2 bytes written instead of an empty stderr pipe. Since the PosixBufferedReader stack-buffer fast path landed, an empty pipe hits EOF in the shared stack buffer and never allocates the per-reader Vec, so the previous form was not exercising the reserve(16 KB) path on POSIX. Writing a byte makes it fall through to the allocating loop. - Assert on post-warmup RSS delta with a per-build-type bound (5 MB release, 30 MB debug/ASAN) instead of the baseline-relative ratio. Tighter than the old `before * 3` check (~8.5 KB/spawn vs ~12 KB/spawn headroom) and stable under ASAN quarantine noise. - Check child exit codes so a missing `cmd`/`echo` fails loudly. - Drop the `0` (infinite) per-test timeout; the rewrite runs in ~2 s on the slowest lane so the default suffices.
|
Warning Review limit reached
Next review available in: 1 minute 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)
WalkthroughThe spawn leak test now repeatedly launches children with unread piped stdout, performs GC-assisted RSS measurements, and verifies successful exits against an ASAN-adjusted memory delta limit. ChangesSpawn pipe regression coverage
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Verified locally: CI build #79609: spawn-noread-leak passed on every test lane; 194 jobs passed, the only annotations are unrelated flakes that retried green, and the one red job is |
![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 that tightens the leak bound and follows the harness leak-test pattern (warmup, Bun.gc(true), RSS delta branched on isASAN/isDebug). The inline nit about the ~10 MB vs ~15 MB comment is worth fixing but doesn't affect the 30 MB limit's correctness.
Extended reasoning...
Overview
This PR rewrites a single leak-regression test file (test/js/bun/spawn/spawn-noread-leak.test.ts) to cut Windows arm64 wall time from ~27 s to ~2 s. It reduces spawn count from 3000 to 750, swaps msys2 cat for native cmd /c echo x / echo x, pipes stdout (with bytes written) instead of an always-empty stderr so the POSIX per-reader reserve() path is actually allocated, and replaces the loose before + after < before * 3 ratio with a warmup-then-delta assertion (5 MB release / 30 MB debug+ASAN). No production code is touched.
Security risks
None. Test-only change spawning echo/cmd /c echo x locally with no network, no filesystem writes, no untrusted input.
Level of scrutiny
Moderate — it rewrites an existing regression test's workload, which REVIEW.md flags for "keep asserting the property the original assertion protected". The PR does exactly that: it still bounds RSS growth over many unread-pipe spawns, and the author traces through PosixBufferedReader::read_with_fn to show the previous form had gone vacuous on POSIX (empty stderr never reached the reserve() allocation). The new delta assertion is strictly tighter than the old ratio on release lanes, and the ASAN/debug branch matches the CLAUDE.md leak-test guidance (branched threshold, well below the unfixed leak, measured after warmup). Measured RSS deltas across four lanes give ~3-10x headroom to the release limit and ~2x to the ASAN limit.
Other factors
- Harness helpers (
isASAN,isDebug,isWindows) andprocess.memoryUsage.rss()are all established patterns in the test suite. - Exit-code fold ensures a missing
cmd/echofails loudly rather than silently changing the workload. - The infinite per-test timeout is dropped now that the slowest lane finishes in ~2 s.
- The one finding is a comment-accuracy nit ("~10 MB" vs the author's own measured ~15 MB for ASAN); the 30 MB limit is unaffected either way. This is a one-character-class fix that shouldn't block merge.
|
Updated 8:58 AM PT - Jul 24th, 2026
❌ @robobun, your commit 540c8ed has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35427That installs a local version of the PR into your bun-35427 --bun |
…ct it Review feedback: - The previous revision's 2-byte write only touched one page of the ~16 KB reserved buffer, so a regressed retention showed ~4 MB over 600 spawns on Linux (overcommit) and slipped under the 5 MB bound. The child now writes 16 KB via `cat`/`cmd /c type` of a temp file so the whole buffer is faulted in; the regressed delta is ~10 MB (verified by replacing the into_boxed_slice() with mem::forget in on_close_io locally: +10.0 MB x3, test fails as intended). - Drop `|| isDebug` from the bound selector: non-ASAN debug builds (native Windows/macOS-x64 `bun bd`, `--asan=off`) should use the tight bound so they can detect the leak locally. Matches the `isASAN ? ...` pattern in the other spawn leak tests. - Fix the buffer-size comment: libuv's pipe alloc_cb passes 65536 on Windows, 16 KB is the POSIX hardcode. - Bump the ASAN bound to 100 MB (observed ~50-52 MB with the 16 KB payload going through quarantine).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/js/bun/spawn/spawn-noread-leak.test.ts`:
- Around line 68-77: In the spawn leak test, move the expect(badExit).toBe(0)
assertion to after the deltaMB RSS-bound assertion, keeping the existing limitMB
calculation and memory assertion unchanged. Ensure the subprocess exit code is
asserted last.
🪄 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: e05d8b9d-56e7-4e71-8f41-434d8a8726af
📒 Files selected for processing (1)
test/js/bun/spawn/spawn-noread-leak.test.ts
There was a problem hiding this comment.
LGTM — the follow-up commit addressed the comment-accuracy nit (ASAN delta now reads ~50 MB against the 100 MB bound, matching the updated measurements), and the byte-count remark is moot with the 16 KB payload.
What was reviewed:
- Leak assertion is strictly tighter than before (5 MB delta vs the old
before*3ratio that permitted ~1.8 GB on debug baselines), with fail-before verified viamem::forgetinjection. - RSS bound branches on
isASANper REVIEW.md; release headroom (~1-2 MB observed vs 5 MB limit) and ASAN headroom (~51 MB vs 100 MB) both look safe against the 5+ run tables. - Windows
cmd /c type <tempDir path>— exit codes are folded and asserted, so a path/quoting failure would surface as a test failure rather than a silently-different workload. tempDir+using,Buffer.alloc(n, fill), dropped infinite timeout — all match harness conventions.
Extended reasoning...
Overview
Test-only rewrite of test/js/bun/spawn/spawn-noread-leak.test.ts (regression coverage for #18265 / PR #20102). No runtime code touched. The change reduces spawn count 3000→750, switches Windows to native cmd /c type instead of msys2 cat, writes a 16 KB payload to the piped stdout so the per-reader buffer is actually allocated and its pages faulted in, replaces the loose before + after < before * 3 ratio with a warmup+delta assertion (5 MB release / 100 MB ASAN), adds an exit-code fold so a missing child binary fails loudly, and drops the , 0 infinite timeout.
Security risks
None. Test-only; spawns cat/cmd /c type on a temp file the test itself creates, no network, no external input.
Level of scrutiny
Low-to-medium. This is a leak-regression test rewrite, and the main risks for this class of change are (a) weakening the property being asserted and (b) introducing flakiness via too-tight RSS bounds. Both are well-covered: the author demonstrated fail-before by injecting mem::forget(taken) in Subprocess::on_close_io on a release build (new test fails at +10 MB, old test passes), and provided 5-8 runs per lane showing release deltas of 0-2.4 MB against a 5 MB limit and ASAN deltas of 50-52 MB against a 100 MB limit. The old ratio check was effectively vacuous on high-baseline builds, so this is a strict tightening.
Other factors
- My prior review flagged a comment/measurement mismatch on the ASAN bound; e43ba3e reworked the payload to 16 KB and rewrote the comment to match fresh measurements (~50 MB ASAN, 100 MB bound). The current diff and PR description are now consistent.
- Harness conventions followed:
tempDirwithusingfor cleanup,Buffer.alloc(16*1024, "x").toString()(not.repeat),isASAN/isWindowsfrom harness, no hardcoded sleeps. - The deliberate choice to gate the wide bound on
isASANonly (notisASAN || isDebug) is explained in the description — non-ASAN debug lanes should be able to detect the leak with the tight bound. If a non-ASAN debug lane turns out noisier than expected, CI will surface it and the bound can be widened; that's the right direction to fail in. - The Windows
cmd /c type <path>invocation passes an unquoted temp path; CI temp dirs don't contain spaces and the author verified on both Windows lanes, and theexpect(badExit).toBe(0)guard means any future path issue fails the test rather than silently skipping the workload.
There was a problem hiding this comment.
LGTM — test-only speedup with fail-before proof; all prior review threads (comment/measurement mismatch, assertion order) are addressed.
What was reviewed:
- The new assertion is strictly tighter than the old
before + after < before * 3ratio, and the stderr→stdout switch is justified (old form no longer allocated the per-reader buffer on POSIX). - RSS bounds vs. measured data: release 5 MB limit sits between healthy (~0-2.4 MB) and regressed (~10 MB); ASAN 100 MB vs. ~51 MB observed.
isASAN-only gating (droppingisDebug) is intentional and stated. - Harness conventions:
tempDir,Buffer.alloc(n, fill), exit-code asserted last, default timeout restored.
Extended reasoning...
Overview
Single-file rewrite of test/js/bun/spawn/spawn-noread-leak.test.ts, the regression guard for issue #18265 (unread "pipe" stdio retaining the PipeReader buffer past child exit). Changes: 3000→750 spawns, native cmd /c type on Windows instead of msys2 cat, 16 KB payload on piped stdout so a retained buffer actually faults pages in, warmup + delta-based RSS assertion replacing a loose ratio, exit-code fold, and default timeout instead of , 0). No production code touched.
Security risks
None. Test-only; spawns local cat/cmd against a temp file it creates.
Level of scrutiny
Moderate for a test rewrite: RSS-threshold leak tests are the class REVIEW.md singles out for headroom/flake scrutiny. The PR body supplies 5+ RSS-delta samples per lane (release max +2.4 MB vs. 5 MB limit; ASAN ~51 MB vs. 100 MB limit — both ~2x headroom) plus a fail-before demonstration (mem::forget on the buffer → +10 MB → assertion trips). The old assertion permitted ~1.8 GB of growth on debug baselines and passed against the same regressed build, so this is a tightening, not a weakening.
Other factors
- All three prior review threads are resolved: my earlier comment/measurement-mismatch nit was mooted by the 16 KB payload rewrite in e43ba3e (new numbers now match the code comment), and CodeRabbit's assertion-order request landed in 540c8ed.
- The stderr→stdout pipe change is explained (POSIX stack-buffer fast path means an empty stderr pipe never hits
reserve()), so the test now exercises the allocation it claims to guard rather than being vacuous. isASAN-only gating (dropping|| isDebug) is a deliberate choice with a stated reason (non-ASAN debug lanes should still detect the leak); if a non-ASAN debug CI lane exceeds 5 MB it will surface immediately.- Harness idioms are followed:
tempDir,Buffer.alloc(n, fill).toString(), exit code asserted last, no hardcoded sleep/timeout.
1 participant
What
Rewrite
test/js/bun/spawn/spawn-noread-leak.test.tsso the Windows arm64 lane runs it in ~2 s instead of ~27 s, with a leak bound the release lanes can actually trip.Why
This file was one of the slowest single-test files on Windows arm64 (build #79247: 29 s wall time). Almost all of that was process-creation cost: 3000 spawns of msys2
cat.exeat ~9 ms each. The RSS check at the end was a loosebefore + after < before * 3ratio that on a debug/ASAN baseline of ~360 MB permitted ~1.8 GB of growth.While tracing the allocation path I also found the previous form no longer exercised the
reserve(16 * 1024)path on POSIX: it piped stderr,catwrites nothing to stderr, andPosixBufferedReader::read_with_fnnow reads the first chunk into the shared event-loop stack buffer and returns on EOF without ever allocating_buffer. InstrumentingSubprocess::on_close_ioconfirmslen=0 cap=0for the stderr pipe there.Changes
["cmd", "/c", "type", payload]on Windows (~2.5 ms/spawn) and["cat", payload]on POSIX, replacing the msys2catlookup on Windows (~9 ms/spawn).cmd/catfails the test instead of silently changing the workload.isASANonly for the wide bound (notisASAN || isDebug): non-ASAN debug builds should use the tight bound so localbun bdon Windows/macOS x64 can detect the leak.Verification
Fail-before: replacing
taken.into_boxed_slice()withmem::forget(taken)inSubprocess::on_close_io(simulating the buffer being retained past child exit) and rebuilding release:The previous form of the test passes against the same build (
before 37.4 MB, after 40.4 MB, well insidebefore * 3).Pass-after timings (
bun bd test/ releasebun test):RSS deltas (5+ runs each)
no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.