Repository navigation
Conversation
…-destroy The test slept 80 ms and then created the stdin sink. On a slow Windows runner the child was still alive at that point, so the sink wrapped a live pipe and the in-flight write failed with EPIPE when the child exited. Await child.exited instead. That is the state the test covers: a stdin sink created after the process has exited.
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 32 seconds for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 3:05 AM PT - Sep 6th, 2026
❌ @robobun, your commit 8a8f281 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 41522That installs a local version of the PR into your bun-41522 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — replaces a fixed sleep with await child.exited, which is exactly the condition the test needs.
What was reviewed:
- Confirmed the fixture (
bad-fixture.js) exits on its own without stdin data, so awaitingchild.exitedbefore writing cannot deadlock. - Verified the referenced
has_exited()branch inWritable.rsexists and is the path this test targets — the de-flake preserves what the test exercises. - Checked that the existing assertions (
out === "",exited === 1) are unchanged, so the test is not weakened.
Extended reasoning...
Overview
This PR touches a single test file, test/js/bun/spawn/spawn-stdin-destroy.test.ts, replacing await Bun.sleep(80) with await child.exited before the test first accesses child.stdin. A comment explains the Windows EPIPE failure mode that motivated the change. No production code is modified.
Security risks
None. This is a test-only change with no effect on shipped code, no new inputs, no network or filesystem behavior changes.
Level of scrutiny
Low. The change is a textbook application of the repo's own rule: await the observable condition rather than sleeping for a fixed duration. The fixture (process.stdin.read(); throw new Error("test")) exits unconditionally without needing any stdin bytes and produces no stdout, so await child.exited cannot hang on an unread pipe or a blocked write. The has_exited() → on_attached_process_exit branch in src/runtime/api/bun/subprocess/Writable.rs matches the PR's claim about which code path the test is meant to hit, and awaiting exit reaches it deterministically rather than probabilistically.
Other factors
The test's assertions (expect(out).toBe(""), expect(exited).toBe(1)) and the GC/crash-guard structure are untouched, so the de-flake does not weaken what the original test protected. The added comment is a "why" comment naming the concrete failure scenario (EPIPE on Windows when the sink wraps a live pipe), which is the kind of context REVIEW.md asks for on timing-sensitive test changes. The bug hunt exited on dry_streak with no findings and no outstanding reviewer objections in the timeline.
|
Status: closed. #45873 made the same change on main ( |
|
Closing: this is covered by #45873, which merged on 2026-10-20. On main, Build 130816 of main (38afee6) passed. Its Windows 2019 x64 lane ran this test. |
Problem
test/js/bun/spawn/spawn-stdin-destroy.test.tsfails on Windows 2019 x64 withEPIPE: broken pipe, writeatchild.stdin.write(build 110853, red on every retry).child.stdin. On a slow runner the child is still alive at that point. The sink then wraps a live pipe, the child exits while the overlapped write is in flight, and libuv reports EPIPE. The fixed sleep is a race, not a condition. The sleep dates from Fix regression from #13414 #14092.Fix
await Bun.sleep(80)withawait child.exited. The test covers a stdin sink that is created after the process has exited (Writable::to_jsinsrc/runtime/api/bun/subprocess/Writable.rs:349, thehas_exited()branch that callsFileSink::on_attached_process_exit). Waiting forexitedreaches that branch every time.FileSink::writeseesdoneand returnsDoneinstead of writing. No EPIPE is possible.bun bd test test/js/bun/spawn/spawn-stdin-destroy.test.tspasses on Linux. The same file passed 30 of 30 runs on Windows x64 with the release build.Background
Bun.spawncreates thestdinFileSinklazily on first access. If the process has exited already, the sink is markeddoneand writes are no-ops. If the process is alive, the sink owns a real pipe and a write can fail when the reader goes away.Notes
Reproduction on Windows x64 with the release build, 15 runs per value of the sleep before
child.stdin.write:So the failure needs the sink to be created in the window where the child is alive but about to exit. On the CI runner that window is at 80 ms.
The test-only change does not alter what the test exercises. The intended path (stdin created after exit) is now reached deterministically instead of most of the time.
[auto-merge] gate passed · iteration 1 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The test used a fixed 80 ms sleep before writing to the child's stdin, and on Windows the bad-fixture.js child could exit first, so the write hit a live pipe whose reader was gone and threw EPIPE before the test reached its real assertion about child.exited. The fix replaces the sleep with await child.exited, so the write deterministically lands on the has_exited() path in Writable.rs that the test was always meant to exercise. The fixture exits on its own without reading stdin, so awaiting exit cannot deadlock, and the existing assertions are unchanged.