Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/runtime/api/bun/subprocess.rs
Original file line number Diff line number Diff line change
Expand Up @@ -705,9 +705,10 @@
sp.on_max_buffer(kind);
}

/// Close any still-open stdout/stderr pipe readers so the sync wait loop
/// stops waiting for EOF after timeout/maxBuffer. Matches Node.js
/// `SyncProcessRunner::Kill()`. Called outside any reader callback.
/// Close any still-open stdout/stderr pipe readers so we stop waiting for
/// EOF after a timeout/maxBuffer kill. A grandchild may still hold the
/// pipe's write end. Matches Node.js `SyncProcessRunner::Kill()`. Called
/// outside any reader callback.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
pub fn close_readable_pipes(&self) {
if matches!(self.stdout.get(), Readable::Pipe(_)) {
self.stdout.with_mut(|s| s.close());
Expand Down Expand Up @@ -1033,6 +1034,18 @@
}
}

// When Bun itself killed the child (timeout/maxBuffer), stop waiting on
// pipe EOF: a grandchild may still hold the write end and the caller
// has already opted into a bounded wait. The drain above buffers
// whatever was readable at exit; closing now delivers that buffer
// instead of blocking `proc.stdout` until the grandchild exits. Same
// semantics as the spawnSync wait loop's `close_readable_pipes()` call.
if self.event_loop_timer.get().state == EventLoopTimerState::FIRED
|| self.exited_due_to_maxbuf.get().is_some()
{
self.close_readable_pipes();
}

Check failure on line 1047 in src/runtime/api/bun/subprocess.rs

View check run for this annotation

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
Comment thread
robobun marked this conversation as resolved.

if let Some(pipe_ptr) = stdin {
self.weak_file_sink_stdin_ptr.set(None);
self.update_flags(|f| f.insert(Flags::HAS_STDIN_DESTRUCTOR_CALLED));
Expand Down
28 changes: 28 additions & 0 deletions test/js/bun/spawn/spawn-maxbuf.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,34 @@
expect(stderr).toBe("");
});

// When Bun kills the child on timeout, a grandchild that inherited the pipe
// may still hold the write end. Reading stdout/stderr after `proc.exited`
// must not wait for that grandchild to exit; Bun closes its read end and
// delivers whatever was buffered (same as `spawnSync`).
test.skipIf(isWindows)(
"Bun.spawn stdout does not hang when a grandchild outlives the timeout",
async () => {
// `sh` starts fast enough that the background `sleep` is running before
// the timeout fires even under a debug build; the signal is delivered
// to `sh` alone, so `sleep` survives holding the pipe's write end.
await using proc = Bun.spawn({
cmd: ["sh", "-c", "echo from-child; sleep 60 & read _"],
env: bunEnv,
timeout: 200,
killSignal: "SIGTERM",
stdio: ["pipe", "pipe", "pipe"],
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});
await proc.exited;
const [stdout, stderr] = await Promise.all([proc.stdout.text(), proc.stderr.text()]);
expect({ stdout, stderr, exitCode: proc.exitCode, signalCode: proc.signalCode }).toEqual({
stdout: "from-child\n",
stderr: "",
exitCode: null,
signalCode: "SIGTERM",
});

Check warning on line 214 in test/js/bun/spawn/spawn-maxbuf.test.ts

View check run for this annotation

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
Comment thread
robobun marked this conversation as resolved.
},
);

test("Bun.spawnSync", () => {
const timeStart = Date.now();
const proc = Bun.spawnSync([bunExe(), "exec", "sleep 5"], {
Expand Down
Loading