Bun.spawn: close stdout/stderr pipes after a timeout kill - #35012
Merged
Claude / Claude Code Review
completed
Jul 21, 2026 in 19m 11s
Code review found 1 important issue
Found 3 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/api/bun/subprocess.rs:1043-1047 |
close_readable_pipes gate misses sibling cases: getter-before-exit and AbortSignal kill |
| 🟡 Nit | test/js/bun/spawn/spawn-maxbuf.test.ts:200-214 |
Test leaks a sleep 60 grandchild process on every run |
Annotations
Check failure on line 1047 in src/runtime/api/bun/subprocess.rs
claude / Claude Code Review
close_readable_pipes gate misses sibling cases: getter-before-exit and AbortSignal kill
This gate misses two direct siblings of the case it fixes. (1) If the user reads `proc.stdout` *before* the timeout fires — the idiomatic `Promise.all([proc.stdout.text(), proc.exited])` pattern REVIEW.md itself recommends — `Readable::to_js` has already replaced `self.stdout` with `Readable::Closed` and handed the fd to a `FileReader`, so `close_readable_pipes()` matches nothing and the read still hangs on the grandchild's EOF. (2) An AbortSignal-triggered kill (`handle_abort_signal` → `try_kil
Check warning on line 214 in test/js/bun/spawn/spawn-maxbuf.test.ts
claude / Claude Code Review
Test leaks a sleep 60 grandchild process on every run
The backgrounded `sleep 60` grandchild is orphaned to init when `sh` receives SIGTERM and runs for ~60s after the test completes — nothing reaps it (closing the read end doesn't SIGPIPE it since `sleep` never writes), which REVIEW.md's "tests must be hermetic and leave nothing behind" flags. Consider having sh also `echo $!` and `try { process.kill(pid) } catch {}` in a `finally` after the assertions; simply shortening to `sleep 5` would match the sibling tests but (per this PR's own description
Loading