test(spawn): await stdin.write() so its EPIPE rejection is handled - #35342
test(spawn): await stdin.write() so its EPIPE rejection is handled#35342robobun wants to merge 1 commit into
Conversation
#35278 made FileSink.write()'s pending promise reject with EPIPE when the deferred auto-flush hits a broken pipe. The 'stdin.end() rejects with EPIPE' test (added in #35209) called write(16MB) without awaiting the result: on release Linux x64 the child exits fast enough that end()'s synchronous flush() sees EPIPE and throws, leaving write()'s separate pending promise to be rejected later by on_attached_process_exit as an unhandled rejection. Depending on timing that surfaced either as the EPIPE test itself failing or as the following 'pipe > hello > before exit' test failing, and always tripped the BUN_FEATURE_FLAG_FORCE_WAITER_THREAD re-run. Move the write() into the try/catch and await it so whichever of write()/end() observes EPIPE first is the one the assertion checks and neither becomes an unhandled rejection.
|
Warning Review limit reached
Next review available in: 11 minutes 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)
Comment |
There was a problem hiding this comment.
LGTM — test-only fix for a CI failure currently red on main.
What was reviewed:
- Confirmed the assertion (
caught?.code === "EPIPE") is unchanged; the test still enforces the same invariant, just with thewrite()promise now inside the try so it can't surface as an unhandled rejection. - Checked that if
await wroterejects andend()is never reached,await using procstill disposes the subprocess cleanly. - The
instanceof Promiseguard correctly handleswrite()'s number-or-Promise return.
Extended reasoning...
Overview
This PR touches a single test in test/js/bun/spawn/spawn.test.ts. The test was added in #35209 and started failing on release linux-x64 lanes after #35278 changed FileSink to reject (rather than resolve) its pending promise when the attached process exits with unwritten data. The test previously called proc.stdin.write(16MB) without awaiting the result, then awaited end() in a try/catch. When end() threw synchronously (because flush() saw EPIPE), the still-armed write() promise was later rejected by on_attached_process_exit and surfaced as an unhandled rejection outside the try block.
The fix moves write() into the try block and awaits it when it returns a Promise, so whichever of write()/end() observes EPIPE first is the one caught and asserted on. The test title and comment are updated to reflect that either call may be the one that rejects.
Security risks
None. Test-only change; no runtime, native, or user-facing code touched.
Level of scrutiny
Low. This is a ~10-line adjustment to a single existing test to account for a runtime behavior change that already landed. The PR description traces the exact mechanism to the source line (FileSink.rs:1135), explains why debug builds masked it, and shows 30/30 before/after verification on a release build plus the waiter-thread and ASAN variants. The core assertion is preserved verbatim.
Other factors
- The change follows the repo's "await conditions, wire failures to reject" test guidance directly — it stops dropping a rejectable promise on the floor.
FileSink.write()returnsnumber | Promise<number>per its API, so theinstanceof Promisebranch is the correct way to handle both.- If
await wroterejects first,end()is never called, butawait using prochandles teardown so nothing leaks. - No existing assertion is weakened or removed; the test title rename is more accurate than the original.
|
Closing in favor of #35344, which fixes this at the right layer: |
Fixes the
spawn.test.tsfailure currently red on every linux-x64 lane on main (ubuntu 25.04, debian 13, alpine 3.23) since 773be9d (#35278).Failure
e.g. https://buildkite.com/bun/bun/builds/78977#019f9132-5d06-4f91-a829-49b25f370c19
Cause
The test (added in #35209) fires
proc.stdin.write(16MB)without awaiting the return value, then awaitsproc.stdin.end()inside a try/catch.FileSink.write()andend()normally share the sink's single pending promise, so awaitingend()covers both. But when the child has already closed the pipe by the timeend_from_jscallsflush(),end()throws synchronously (FileSink.rs:1135) instead of returning that promise, leavingwrite()'s promise still armed.on_attached_process_exitthen rejects it duringawait proc.exited, and since the test dropped thewrite()result it surfaces as an unhandled rejection.Before #35278 that stranded promise resolved with the buffered byte count (the very bug #35278 fixed), so nothing surfaced. After #35278 it rejects with
EPIPE. On release linux-x64 the child reads one byte and exits in well under the ~16ms test duration, so this hits deterministically; debug builds are slow enough to mask it.Fix
Move
write()into the try/catch and await it when it returns a promise. Whichever ofwrite()/end()observesEPIPEfirst is the one the assertion checks, and neither path becomes an unhandled rejection.Verification
Release build on linux-x64, current main (b57c7ae):
Also green under
BUN_FEATURE_FLAG_FORCE_WAITER_THREAD=1 BUN_GARBAGE_COLLECTOR_LEVEL=1and on the debug+ASAN build.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.