Repository navigation
Conversation
The two 4 GiB "Buffer length limit" cases took 11 of the file's 14 s in CI. Each one pumps 4 GiB through a pipe while spawnSync's read buffer doubles to 8 GiB, so most of the time is page faults in the child. They now run at the same time on machines with at least 32 GiB of memory (the ASAN CI machines have 64 GiB), and one at a time below that. The rest of the file drops the bun children where the assertion did not need bun: the timeout 0 cases run /bin/sh instead of bun -e, the two counter fixtures merge into one run, the drain cases cover stdout and stderr in one spawn with shorter pauses, and the grandchild timeout case waits 300 ms instead of 500 ms. Every case now compares the whole result shape (exitCode, signalCode, success, exitedDueToTimeout, exitedDueToMaxBuffer, stdout, stderr). The timeout 0 cases lose their elapsed-time assertion and instead prove that maxBuffer 0 sets no limit. The counter fixture reports exact deltas instead of throwing on a missing increment. Locally with the debug build: 33.1 s and 30.7 s before, 19.3 s, 18.4 s and 16.1 s after.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughThe PR expands ChangesspawnSync test coverage
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:26 PM PT - Aug 24th, 2026
✅ @robobun, your commit 4c820aa2dba5be528c32c5f5ac4835d647d8f5c6 passed in 🧪 To try this PR locally: bunx bun-pr 40320That installs a local version of the PR into your bun-40320 --bun |
|
Status: ready for review. Measured with the debug build (
Before the change the two 4 GiB cases took 11.8 s and 12.7 s. They now overlap and take 11 to 14 s together. The remaining cases went from about 5 s to about 3 s, most of which is the The release build passes the file in 10.2 s. Test count goes from 18 to 19: the memfd and counters fixtures share one run, and the drain block covers a third stdio layout (both fds piped) next to the two single-pipe layouts. Review follow-ups: the single-pipe drain layouts and the 500 ms grandchild timeout are back (9f51408), the concurrency switch uses the cgroup-aware memory budget and each 4 GiB child has its own 100 s timeout (cc1ac5c), and the counter deltas for a buffer stdin are pinned off Linux too (4c820aa). CI on the first push failed only in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it consolidates test cases (4 drain cases → 2, dropping the single-pipe-only variants) and shortens several timing thresholds (500→300 ms timeout, 300→100 ms drain pauses), a human look at whether those coverage and flakiness trade-offs are acceptable across all CI lanes would still be worthwhile.
What was reviewed:
describe.concurrent.skipIfcomposes correctly (returnsDescribe<T>; same pattern used ins3.test.ts).- Merged counters fixture preserves and tightens the old memfd/blocking assertions — exact deltas replace
>checks; the non-LinuxbufferStdinbranch was never asserted before either. - Drain assertions on final content ("ABC"/"abc"), not read boundaries, so the shorter sleeps can't cause false failures from coalesced reads.
printf ''for theinheritcase and["true"]in the fixture are portable across POSIX.
Extended reasoning...
Overview
This PR is a test-only refactor of test/js/bun/spawn/spawnSync.test.ts plus its fixtures, aimed at cutting CI wall time (~14 s → faster) while tightening assertions. It: introduces a summary() helper that compares the full result shape; merges two fixture subprocess runs into one that reports exact counter deltas; consolidates four drain cases into two that pipe stdout and stderr together; swaps bun children for /bin/sh where bun is not required; shortens sleep/timeout durations; and gates the two 4 GiB cases on describe.concurrent when the machine has ≥32 GiB. No runtime code is touched.
Security risks
None. Test-only changes spawning /bin/sh, true, head, and the debug bun binary in isolated child processes. No network, no untrusted input.
Level of scrutiny
Moderate. It's test-only, but REVIEW.md is explicit that consolidating or deleting tests requires a stated reason and must preserve the property the original assertions protected. The PR description states each consolidation, and I verified that the merged assertions are at least as strong as before (exact counter deltas vs. >; full result shape vs. two fields; error name+code vs. message-only snapshot). The one arguable coverage reduction is that the drain block no longer exercises "only stdout piped, stderr ignored" and "only stderr piped, stdout ignored" separately — it now pipes both. The drain loop reads whatever pipes exist, so this is likely fine, but it's a judgment call a human should confirm.
Other factors
Timing thresholds were reduced (grandchild timeout 500→300 ms with the holder sleep 5→2, drain pauses 300/100→100/50 ms, timeout-0 child 5→50 ms via /bin/sh). The author stress-tested 120/120 passes at load average 30, and the drain assertions are on final buffer content rather than read timing, so coalesced reads under load still pass. The 32 GiB concurrency threshold gives ~15 GiB headroom over the stated 17 GiB peak for the pair; below that it falls back to serial. describe.concurrent.skipIf is a valid composition (used elsewhere in the suite). These all look reasonable, but the flakiness margin on slower CI lanes (macOS, aarch64, Windows-ASAN-free) is the kind of thing a maintainer with CI history should sign off on rather than an automated approval.
…ld timeout The drain block now covers three stdio layouts (stdout piped, stderr piped, both piped) for each maxBuffer value, so the two single-pipe layouts from before stay covered. The grandchild timeout case keeps its 500 ms timeout and 5 s holder, since the 200 ms saved is not worth a smaller margin on slow CI machines.
|
Thanks for the look. Both points are addressed in 9f51408:
Full file with the debug build after this change: 18.8 s (before the PR: 33.1 s and 30.7 s). |
There was a problem hiding this comment.
LGTM — test-only speedup that tightens assertions rather than loosening them.
Checked that every rewritten case preserves or strengthens what it asserted before: the summary() helper now compares the whole result shape (including signalCode/exitedDueToTimeout/exitedDueToMaxBuffer), the counters fixture asserts exact deltas instead of >, and the 4 GiB case gains a lastByte check. Verified describe.concurrent.skipIf chains correctly per the Describe<T> type and existing usage in the suite. The reduced drain-test pauses (0.3→0.1 s) were stress-tested 120× under load per the description, and the dropped elapsed-time bound in the timeout=0 cases is replaced by stricter structural checks that would fail the same way if timeout: 0 were misapplied.
Extended reasoning...
Overview
Test-only change to test/js/bun/spawn/spawnSync.test.ts plus its two fixture files. Merges the memfd and counters fixtures into one run, swaps bun subprocesses for /bin/sh where bun isn't needed, gates the two 4 GiB cases behind describe.concurrent on ≥32 GiB machines (serial otherwise), and introduces a summary() helper so every case asserts the full SyncSubprocess result shape. No runtime code is touched.
Security risks
None. This is test infrastructure only; the only external processes invoked are /bin/sh, printf, sleep, true, and head on POSIX, all with fixed literal arguments.
Level of scrutiny
Medium — the REVIEW.md guidance on "never silently weaken an existing test" is the main lens. I walked each rewritten case against its predecessor:
- RangeError: snapshot → name+code+message (stronger).
- timeout=0: dropped a vacuous
< 1000mselapsed check, added proof thatmaxBuffer: 0sets no limit and that no signal/timeout flags appear (stronger; would still fail iftimeout: 0killed the child). - counters/memfd: two fixtures → one;
> before→ exact1/0deltas on Linux, and the non-Linux buffer-stdin arm only checksexitCodebecause the old memfd fixture was Linux-only (no lost coverage). - drain: 4 → 6 cases (adds the both-piped layout), full-shape assertion, pauses reduced but stress-tested under load.
- 4 GiB: adds
lastByte: 0; concurrency gated on 32 GiB total memory vs. ~17 GiB peak for the pair.
Other factors
Confirmed describe.concurrent returns a Describe<T> so .skipIf(...) chains, and it's already used elsewhere in test/js/. SyncSubprocess declares signalCode/exitedDueToTimeout/exitedDueToMaxBuffer as optional, matching the undefined expectations in summary(). The second commit restored the single-pipe drain layouts and the 500 ms grandchild timeout, so the PR body's "four cases become two" note is slightly stale but the code is more conservative than described. No outstanding reviewer comments; bug hunter found nothing.
…ildren os.totalmem() reports the host's memory, so inside a container with a cgroup memory limit the concurrency switch could pick the 17 GiB concurrent run on a 16 GiB cap. The budget is now the smaller of os.totalmem() and process.constrainedMemory(), which is the cgroup limit on Linux and the total memory elsewhere. The test runner does not kill dangling children when a concurrent test times out, so each 4 GiB child now carries its own 100 s timeout with SIGKILL. The counters test parses an empty fixture stdout as null so a crashed fixture reports its stderr and exit code instead of a JSON parse error.
There was a problem hiding this comment.
LGTM — test-only speedup that tightens assertions without dropping coverage.
What was reviewed:
- Verified no existing assertion is weakened: the dropped
end - start < 1000timing check is replaced by a stronger proof (child completes with output undertimeout: 0/maxBuffer: 0); the merged counters fixture now asserts exact deltas instead of> before; the drain block keeps both single-pipe layouts and adds a third. - Checked
process.constrainedMemory()in Bun returnsWTF::ramSize()(never 0), soMath.min(totalmem(), constrainedMemory())won't accidentally skip the 4 GiB cases on unconstrained hosts. - Confirmed
describe.concurrentchains with.skipIf(returnsDescribe<T>pertest.d.ts), and the child-sidetimeout: 100_000/SIGKILLbounds the concurrent 4 GiB pair as described.
Extended reasoning...
Overview
This PR touches three test files only: it rewrites test/js/bun/spawn/spawnSync.test.ts to run faster (33 s → ~19 s in debug), merges spawnSync-memfd-fixture.ts into spawnSync-counters-fixture.ts, and deletes the now-redundant memfd fixture. No runtime code is touched.
Security risks
None. All changes are confined to test/, spawn only local shells and the debug bun binary, and contact no network.
Level of scrutiny
Medium — test refactors carry the risk of silently weakening coverage (a REVIEW.md most-blocked category). I traced each removed or changed assertion:
toThrowErrorMatchingInlineSnapshot→toThrow(objectContaining({name, code, message})): strictly stronger.timeout: 0tests dropped the< 1000mselapsed check but now assert the child ran to completion with output; a wrongly-applied 0 ms timeout or 0-byte maxBuffer would kill it, so this is a stronger invariant than the old timing bound (which would also pass if the child were killed instantly).- Counter fixtures: exact
spawnSync_blocking: 1/spawn_memfd: 1deltas replaceafter > before. The non-LinuxbufferStdincase only checksexitCode, but the old memfd fixture was Linux-only so nothing is lost. - Drain tests: sleeps shortened (0.3→0.1, 0.1→0.05). The PR correctly notes these only affect regression detection sensitivity, not test correctness — assertions compare final content, so a slow machine cannot false-fail. Single-pipe layouts were restored after earlier review feedback and a both-piped layout added, so coverage is a strict superset.
- 4 GiB pair: gains a
lastBytecheck and a 100 s child-side SIGKILL to reclaim memory if the concurrent test times out (the runner's dangling-process kill is disabled underdescribe.concurrent, per the PR notes).
Other factors
I verified describe.concurrent.skipIf is a valid chain (concurrent returns Describe<T> in packages/bun-types/test.d.ts:246), and that Bun's process.constrainedMemory() is WTF::ramSize() (BunProcess.cpp:3786), which always returns a positive value — so the Math.min gate doesn't accidentally evaluate to 0 and skip the 4 GiB cases on unconstrained machines. The author stress-tested the timing-sensitive cases 120/120 under load, addressed prior review feedback (restored the 500 ms grandchild timeout and single-pipe layouts), and kept the uid/gid block untouched to avoid conflicting with #39232. The PR description is unusually thorough about every trade-off.
Off Linux and Android the buffer stdin stays a pipe, so neither the
blocking nor the memfd counter moves. The fixture returns exactly
{ exitCode: 0, spawnSync_blocking: 0, spawn_memfd: 0 } on that path,
which BUN_FEATURE_FLAG_DISABLE_MEMFD=1 reproduces on Linux. The test
now expects that value instead of only checking the exit code.
There was a problem hiding this comment.
LGTM — test-only speedup that tightens assertions without weakening coverage.
What was reviewed:
- Confirmed
isAndroidexists in harness anddescribe.concurrent.skipIfchains correctly (used elsewhere in the suite). - Checked
process.constrainedMemory()in Bun returnsWTF::ramSize()(always positive), soMath.min(totalmem(), constrainedMemory())cannot produce 0 and wrongly skip the 4 GiB block. - Verified no existing assertion is weakened: single-pipe drain layouts and the 500 ms grandchild timeout are retained, and the new
summary()shape checks are strictly stronger than the old field-by-field checks. - Confirmed
bunRunreturn shape matches the new destructuring, and the merged counters fixture pins exact deltas on both the Linux and non-Linux paths.
Extended reasoning...
Overview
This PR speeds up test/js/bun/spawn/spawnSync.test.ts from ~31 s to ~19 s in debug builds and tightens its assertions. It touches three test-only files: the test itself, one fixture that is rewritten to report exact counter deltas as JSON, and a second fixture that is deleted (folded into the first). No production code is modified.
The main mechanisms: the two 4 GiB cases run under describe.concurrent when the cgroup-aware memory budget is ≥ 32 GiB (with a 100 s SIGKILL bound on each child so an overrun doesn't leave 8.5 GiB processes behind); children that don't need bun run /bin/sh; drain-case pauses shrink from 300/100 ms to 100/50 ms; and every case now compares the full result shape via a summary() helper instead of spot-checking one or two fields.
Security risks
None. Test-only changes that spawn local /bin/sh, true, head, and the debug bun binary. No new external inputs, no network, no auth or crypto surface.
Level of scrutiny
Medium — the concern with test speedups is silently weakening coverage or introducing flakiness, both of which the repo review guidance calls out. I checked each rewritten case against its predecessor: the RangeError check now also asserts name and code; the timeout=0 cases now prove maxBuffer: 0 sets no limit and assert the whole result shape (the removed end - start < 1000 timing check was redundant with exitCode: 0 + exitedDueToTimeout: undefined); the counters test pins exact deltas on every platform instead of > before; the drain block adds a third both-piped layout while keeping both single-pipe layouts; the 4 GiB block adds a lastByte check. Nothing is weaker. Prior review feedback about restoring the single-pipe layouts and the 500 ms grandchild margin was already addressed in 9f51408.
On flakiness: the shortened drain pauses (100/50 ms) only affect whether a regression is caught in a given run — the assertions are on final content, so a slow machine can't false-fail. The PR includes a stress run (--rerun-each=20 under 24 busy loops, 120/120 pass). The concurrent 4 GiB pair is gated on ≥ 32 GiB and each child self-terminates via timeout: 100_000, killSignal: SIGKILL, which the PR explains is needed because describe.concurrent skips the runner's dangling-process kill.
Other factors
I verified the harness/runtime pieces the diff relies on: isAndroid is exported from test/harness.ts; describe.concurrent.skipIf is an established chain (s3.test.ts); bunRun returns { stdout, stderr, exitCode, signalCode } matching the new destructuring; Bun's process.constrainedMemory() is WTF::ramSize() and never 0/undefined, so the Math.min guard is sound. The summary() helper reads fields explicitly instead of spreading, which the PR notes avoids the debug-build hasInlineStorage() assert (#36545). CI build #104944 passed on cc1ac5c per the author's status comment.
Problem
test/js/bun/spawn/spawnSync.test.tstakes 14 s in CI (debian 13 x64-asan, build #104578) and 31 to 33 s locally (debug build).Fix
Math.min(os.totalmem(), process.constrainedMemory())is at least 32 GiB (ASAN CI machines have 64 GiB), one at a time below that. Each child gets a 100 s SIGKILL timeout: the runner does not kill the children of a concurrent test that times out./bin/sh, the two counter fixtures merge into one run, and the drain pauses shrink from 300 and 100 ms to 100 and 50 ms.summary()helper. The timeout 0 cases prove thatmaxBuffer: 0sets no limit. The counter fixture reports exact deltas on every platform.bun bd test test/js/bun/spawn/spawnSync.test.tspasses in 18.4 to 20.2 s (before: 33.1 s and 30.7 s).Background
Bun.spawnSyncsetssignalCode,exitedDueToTimeoutandexitedDueToMaxBufferonly when they applied, andstdout/stderronly when piped.timeout: 0andmaxBuffer: 0mean "no limit".waitpidinstead of running its event loop (spawnSync_blocking). On Linux a buffer stdin becomes a memfd (spawn_memfd).bun testdisables that fast path in its own process, hence the fixture.Notes
Per-test timings with the debug build before the change: RangeError 12 ms, timeout 0 cases 3 x 300 ms, memfd fixture 1342 ms, counters fixture 1292 ms, drain cases 4 x 410 ms, grandchild timeout 508 ms, 2^32+1 case 11816 ms, 2^32 case 12722 ms, uid/gid cases 11 ms, 8 ms and 369 ms. The CI split comes from the Buildkite log timestamps of build #104578: 5.8 s and 5.4 s for the two 4 GiB cases, about 2.4 s for everything else.
After: timeout 0 cases 3 x 60 ms, counters fixture 1350 to 2040 ms, drain cases 6 x 156 ms, grandchild timeout 508 ms, the 4 GiB pair 11.2 to 14.4 s wall (they overlap). Each child peaks near 8.5 GiB of RSS, so the pair needs about 17 GiB.
The 4 GiB child:
head -c N /dev/zero | cat > /dev/nulltakes 1.8 s here. spawnSync on the same output takes 11.2 s in the debug build and 6.5 to 8.4 s in the release build, 7 to 10 s of it sys time.PosixBufferedReader::fill_bufferreserves 16 KiB at a time into aVec, so the buffer doubles up to 8 GiB and each doubling copies into fresh pages: about 12 GiB of page faults per case. A chunk list ormremapgrowth would cut this, but that is a runtime change and out of scope here.The counters fixture costs 1.3 s in the debug build, 1 s of it in the
bun:internal-for-testingimport:exposedInternalseagerly requiresinternal/assert/myers_diff,internal/webstreams_adaptersand through themnode:streamandfs.WriteStream. The release build imports it in 30 ms. 127 test files import that module, so making those entries lazy would help the whole ASAN lane. Out of scope here.process.constrainedMemory()isWTF::ramSize(), which on Linux returnsmin(uv_get_constrained_memory(), sysinfo.totalram)and on macOS and Windows the total memory. On the container this was developed in,os.totalmem()reports 247.8 GiB andprocess.constrainedMemory()32 GiB.With
describe.concurrent,kill_dangling_processes_on_timeoutinsrc/runtime/test_runner/Execution.rsskips the kill (it only runs for a single sequence), so a child that outlived the 120 s test timeout would keep its 8.5 GiB until it finished on its own. The child-sidetimeout: 100_000withkillSignal: "SIGKILL"bounds that, and an overrun shows up assignalCode: "SIGKILL"in thetoEqualdiff.Off Linux and Android a buffer stdin stays
Stdio::ArrayBuffer, whichis_piped()treats as a pipe, so the blocking fast path is not taken and neither counter moves. The fixture returns{ exitCode: 0, spawnSync_blocking: 0, spawn_memfd: 0 }there.BUN_FEATURE_FLAG_DISABLE_MEMFD=1reproduces that path on Linux and prints the same value. The darwin lanes verify the pin.The first version of this PR merged the drain cases into two that piped both fds and cut the grandchild timeout to 300 ms. Review asked whether the single-pipe layouts and the timing margin were worth keeping; they are cheap (6 x 156 ms, and 200 ms), so both are back.
The result object is never spread in the test: spreading a
spawnSyncresult tripsASSERT(hasInlineStorage())in debug builds (#36545 fixes it).The uid/gid block is untouched so that #39232 (harness
isRoot) merges with at most a one-line conflict: both PRs add a name to theharnessimport (isAndroidhere,isRootthere).Stress check:
bun bd test test/js/bun/spawn/spawnSync.test.ts -t "drains|should not set" --rerun-each=20with 24 busy loops on 16 cores: 120 pass, 0 fail.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/spawn/spawnSync.test.ts