Repository navigation
shell: finish a pipeline whose pipe setup fails instead of throwing - #40740
Conversation
When `socketpair()` or the per-command `dup()` of the cwd fd fails in `Pipeline::next_starting` (for example with EMFILE), the pipeline set its state to `WaitingWriteErr`, threw a JS error and returned `Yield::failed()`. Nothing was enqueued for that state and no `child_done` reached the parent, so the interpreter never called `finish()` and the `$` promise never settled. The process also stayed alive because the pending-activity count was never dropped. Write `bun: <message>\n` to the pipeline's stderr the way `Cmd` and `CondExpr` already do, and finish the pipeline with exit code 1 through the `Done` state. A stderr fd goes through the IOWriter and a new `WriterTag::Pipeline` completion. A captured stderr is appended to synchronously. Both paths return `Next(this)` so the trampoline drops the pipeline from `pipeline_stack` before the parent frees it.
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 8 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: reproduced and fixed. Repro on the released bun (1.4.1): ulimit -n 16
bun -e 'import { $ } from "bun"; await $`echo a; echo hi | cat | cat | cat | cat | cat | cat | cat | cat`; console.log("done")'Prints With this branch the pipeline prints |
|
Updated 12:11 AM PT - Aug 28th, 2026
⏳ @robobun, your commit 85c9385 is still building 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 and didn't find any bugs. Because it changes control flow in the shell interpreter's trampoline-driven state machine — in particular the dupe_for_subshell failure at cmd_idx > 0, where finish completes the pipeline and Pipeline::deinit tears down already-started builtin children — a human look from someone familiar with the shell rewrite would still be worthwhile.
What was reviewed:
- Traced
finish→Yield::Next(this)throughYield::runfor all four entry shapes (initialNextpush,drain_pipelinesre-entry, syncOnIoWriterChunkfromenqueue, and async IOWriter callback) — the.position()removal handles both on-stack and already-popped cases, andWaitingWriteErrcorrectly makesdrain_pipelinespop-without-next. - Compared
write_failing_errorline-by-line againstCondExpr::write_failing_errorandBuiltin::cmd_write_failing_error— same shape, withfinishin place ofchild_done(parent, ..)for the pipeline-stack invariant. - Checked the fd-close accounting in the
dupe_for_subshellerror arm atcmd_idx == 0(IOWriter inchild_ioownspipes[0][1], loop closes the rest). - Looked at whether
firstFailure > 0in the second test could go false under debug+ASAN's higher baseline fd count — a 2-command pipeline needs one socketpair plus one dup, which fits comfortably under 32.
Extended reasoning...
Overview
This PR fixes a hang in Bun.$ when pipeline setup fails (EMFILE from socketpair or the per-command env dup). The old code called interp.throw(..) + Yield::failed() from inside Pipeline::next_starting, which raised a JS exception no caller catches and never reported completion to the parent — the promise hung forever. The fix adds Pipeline::write_failing_error (mirroring CondExpr::write_failing_error and Builtin::cmd_write_failing_error), a WriterTag::Pipeline dispatch arm in IOWriter, Pipeline::on_io_writer_chunk as the async-write completion, and a finish helper that sets Done { exit_code } and returns Next(this) so the trampoline pops the node from pipeline_stack before next() frees it via child_done. Two POSIX-only tests exercise captured stderr, fd stderr, script continuation, .throw() mode, and a length sweep that crosses from success into the dup-failure and then the socketpair-failure regime.
Security risks
None. The change is entirely internal to the shell interpreter's error-path control flow — no parsing of untrusted input, no auth/crypto/permissions surface, no new syscalls. The only user-visible effect is that a previously-hanging promise now settles with exit code 1 and an error message on stderr.
Level of scrutiny
Medium-high. The code itself is small and closely follows two existing in-tree templates, but it lives in the shell interpreter's arena-backed state machine where node lifetime, pipeline_stack membership, and re-entrancy through the Yield::run trampoline are all load-bearing. I traced every path finish's Next(this) can take through Yield::run (initial push, drain_pipelines peek-and-return, sync OnIoWriterChunk, async callback into a fresh trampoline) and confirmed the .position()-based removal is a no-op when the node was already popped and correct when it wasn't. The WaitingWriteErr state makes is_starting_cmds false and is_done false, so drain_pipelines pops it without stepping — matching the pre-existing behavior for that state. The fd-close accounting in both error arms was already present and is unchanged in substance.
Other factors
The one path I'd want a human to confirm is dupe_for_subshell failing at cmd_idx > 0: finish completes the pipeline and the parent's child_done calls Pipeline::deinit, which walks cmds[] and deinit_nodes already-started children (builtin cats with live IOReader/IOWriter registrations). The PR notes state Cmd::deinit handles this and that the second test reaches it under ASAN across several cmd_idx values, and the old code on this path was strictly worse (same live children, plus a permanent hang) — but this is exactly the kind of teardown-while-registered path REVIEW.md flags for careful memory-safety review. The tests are well-designed (subprocess-spawned, await using, drain pipes concurrently, assert output before exit code, bunEnv spread correctly, describe.skipIf(isWindows)) and the PR description lists a broad set of related shell tests as passing. The firstFailure > 0 assertion in the sweep test was examined for debug+ASAN fragility and looks safe: a 2-command pipeline under ulimit -n 32 with builtin cat needs one socketpair and one dup on top of Bun's baseline, well within budget.
### Problem - A pipeline whose per-command `dup` fails at command `i > 0` crashes later. Repro: `(echo hi) | cat | cat | ... ` under `ulimit -n 28`. The pipeline prints `bun: Too many open files` and exits 1, then the process panics with `internal error: entered unreachable code: child_done on freed Node#3` (`interpreter.rs:794`) on the next event loop turn. With a different slot layout it reads a freed node instead (ASAN SEGV in `Stmt::next`). - The cause is in `Pipeline::next_starting` (`src/runtime/shell/states/Pipeline.rs`). It inits and starts one child per call, so at command `i` the commands `0..i` already run. `write_failing_error` finishes the pipeline, the parent frees it, and `Pipeline::deinit` frees those children. It does not reach the subtrees they own: a Subshell's Script, a Cmd's Expansion. The orphaned subtree still writes into the pipe. When the pipe closes under it, it reports to the freed node. ### Fix - `Pipeline::setup_commands` creates the pipes, dupes the env and inits every child before any child starts. A failed pipe or dup leaves only children that never ran, so `Pipeline::deinit` frees them cleanly and their IOReader/IOWriter drops close the pipe ends. This is the order the Zig interpreter used (`setupCommands` before `start`). - Correct because the children are independent until they start. Each gets a dupe of the parent env, which no child can change, and no pipe end is read or written before its owner starts. The `pipes` field goes away: the fd numbers only matter inside setup. - Pipeline is the only state with children that run at the same time, so it was the only place a parent could be freed with a running child underneath. Every other `deinit` runs from the parent's `child_done`, after the child finished. - Verified: `test/js/bun/shell/bunshell.test.ts`, new test `reports EMFILE from the per-command env dup when the head is a subshell` (panics on the released bun). Also `bunshell.test.ts`, `pipeline_stack.test.ts`, `yield.test.ts`, `assignments-in-pipeline.test.ts`, `epipe.test.ts`, `throw.test.ts`, `exec.test.ts`, `shell-hang.test.ts`, `shell-write-fault.test.ts`, `shell-pipe-read-fault.test.ts` and the rest of `test/js/bun/shell/`. ### Background - The shell interpreter is a state machine over an arena of nodes (`Script`, `Stmt`, `Pipeline`, `Cmd`, `Subshell`, ...). Each step returns a `Yield`. `Yield::run` is the trampoline that drives it. A node reports completion with `interp.child_done(parent, this, exit_code)`. The parent then frees the child with `deinit_node`. - A node's `deinit` frees what the node owns, not its running children. `Subshell::deinit` frees the duped env only. `Cmd::deinit` tears down a builtin or subprocess, but a Cmd still expanding its arguments has an `Expansion` child node instead. - `IOWriter` is the shared async writer for an fd. `enqueue` queues bytes for a `ChildPtr` and later calls that child's `on_io_writer_chunk`. The orphaned echo in the repro holds its own clone of the pipe's IOWriter, which is why its chunk outlives the pipeline. <details><summary>Notes</summary> - Found by code review on #32302 (closed) and handed over after #40740 merged. #40740 made the pipeline finish with exit 1 instead of throwing. Its second test uses `echo hi | cat | ...` with the builtin `cat`, where every child is a plain Cmd. Those have no subtree, and their IOWriter dies with the node, so the pending chunk never fires. A Subshell head has a Script subtree that holds its own clone of the IO, so the chunk outlives the pipeline. - Trace of the panic on main (`BUN_DEBUG_SHELL=1`, `ulimit -n 28`, `(echo a) | cat x6`): the subshell (Node#3) starts, its echo enqueues `a\n` on the socketpair writer, children 1..5 start, the dup for child 6 fails, `Pipeline Node#2 deinit`, `Subshell Node#3 deinit`. On the next tick the writer gets EPIPE (the read end closed with child 1), `Cmd Node#6 execDone exit=65504`, `Stmt Node#5 childDone`, `Script::finish` calls `child_done(Node#3)`: panic. - The test runs `n = 2..16` under `ulimit -n 32` and awaits `Bun.sleep(0)` after each pipeline. That is one event loop turn, not a timed wait: the orphaned echo's EPIPE completion is delivered then, while the old interpreter's slot is still free, so the unfixed build panics deterministically instead of dispatching into a reused slot. The window where the dup fails at `i > 0` spans about four values of `n` for any fd budget, so the sweep hits it regardless of the base fd count of the machine. - With the fix, the failing sweep no longer leaks fds: on main each orphan kept its socketpair end open until the interpreter was finalized. Checked with 20 identical failing pipelines followed by one that must still fit. - `write_failing_error` now clones the stderr `Arc<IOWriter>` out of the node before `enqueue` instead of holding a borrow of the node across the call. Behavior is unchanged. - Not changed here: on Windows, `WindowsBufferedWriter::write` can call `IOWriter::on_error` from under `enqueue` when uv rejects the write synchronously (`start_with_current_pipe` returns `Ok` unconditionally). That runs the completion on a nested trampoline while the caller's trampoline still has the pipeline on its `pipeline_stack`. On POSIX the same failure is returned as a `Yield` through `on_sync_error`. An earlier revision of this PR carried a Pipeline-local guard for it (an `EnqueuingWriteErr` state). The self-review found that every `enqueue` caller has the same exposure and that the contract `IOWriter::write` documents (failures are returned, never dispatched) is what Windows breaks, so the guard was dropped in favor of a fix at the IOWriter layer. The open shell refactor #40228 defers to the same io-layer follow-up. Windows behavior is unchanged from main. - Out of scope, already noted in #40740: with subprocess children under fd exhaustion, a spawn whose `pidfd_open` fails with EMFILE blocks in `wait4`. A sweep with `(sleep 0.3; echo a) | cat | ...` (the real `cat`) hangs at some budgets on main and on this branch alike. - `ls` and `rm` tests under `test/js/bun/shell/commands/` fail in this container for environment reasons (root user for the permission denied cases, no registry for the `node_modules` cases). They do not involve this change. - `cargo check -p bun_runtime --target x86_64-pc-windows-msvc` and `cargo clippy -p bun_runtime --no-deps` are clean. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/shell/bunshell.test.ts <!-- robobun:evidence:end -->
3 participants
Problem
$promise never settles and the process never exits. Repro:ulimit -n 16; bun -e 'await Bun.$\echo a; echo hi | cat | cat | cat | cat | cat | cat | cat | cat`'printsa, reportsEMFILE: Too many open files (socketpair)`, then hangs.Pipeline::next_starting(src/runtime/shell/states/Pipeline.rs, thesocketpair()loop and thedupe_for_subshellcall). They set the state toWaitingWriteErr, callinterp.throw(..)and returnYield::failed(). No writer is enqueued for that state and nochild_donereaches the parent, so the interpreter never callsfinish(). The pending-activity count is never dropped either, so the process stays alive.Fix
Pipeline::write_failing_error, which writesbun: <message>\nto the pipeline's stderr and finishes with exit code 1. This is the shapeBuiltin::cmd_write_failing_errorandCondExpr::write_failing_erroralready use.WriterTag::Pipelineand parks inWaitingWriteErr.Pipeline::on_io_writer_chunkfinishes the pipeline when the write completes. A captured stderr is appended to synchronously.Pipeline::finish, which setsDone { exit_code }and returnsNext(this). The trampoline then removes the pipeline frompipeline_stackbeforenext()reports to the parent, which frees the node. The empty-pipeline path uses the same helper.test/js/bun/shell/bunshell.test.ts, two new tests underpipeline that fails to create its pipes(both fail on the released bun with an uncaught EMFILE error). Alsobunshell.test.ts,pipeline_stack.test.ts,yield.test.ts,shell-hang.test.ts,epipe.test.ts,assignments-in-pipeline.test.ts,throw.test.ts,exec.test.ts,shell-write-fault.test.ts,shell-pipe-read-fault.test.ts.Background
Script,Stmt,Pipeline,Cmd, ...). Each step returns aYield.Yield::runis the trampoline that drives it. A node reports completion to its parent withinterp.child_done(parent, this, exit_code), and the root completion callsInterpreter::finish, which settles the JS promise.Yield::failed()only means "a JS exception was raised". It does not complete the node. Outsiderun_from_js, nothing catches that exception, so the parent waits forever.pipeline_stackof pipelines that are still starting children. A pipeline must not callchild_doneon its parent while it is on that stack: the parent frees the node and the stack then points at a freed slot. ReturningNext(this)in theDonestate lets the trampoline pop it first.IOWriteris the shared async writer for an fd. A state enqueues bytes with aChildPtr(node id plusWriterTag) and getson_io_writer_chunkwhen the chunk is written. The tag selects which state's callback runs.Notes
writeFailingError("bun: {f}\n", system_err.message)thenchildDone(this, 1)fromonIOWriterChunk). The Rust port replaced it withthrow+failed().to_shell_system_error().messagewith no path, so it readsbun: Too many open files. TheShellErrDisplayimpl would add a trailing:becausesocketpairanddupcarry no path.on_io_writer_chunkfinishes with exit 1 even when the stderr write itself failed. Throwing there would hang the same way, and the parent always needs a completion.dupe_for_subshellpath atcmd_idx > 0, earlier children are already running. Finishing the pipeline makes the parent free it, andPipeline::deinittears down those children (Cmd::deinitkills a live subprocess, drops a builtin).ProcessHandledetaches the exit handler before the subprocess box is freed. The second test reaches this path with live builtin children at severalcmd_idxvalues under the ASAN debug build.bun exec(mini event loop) used to abort the whole script witherror: Failed due to error: bunsh: Too many open files:and exit 1. It now printsbun: Too many open filesand continues, like bash does afterpipe error./bin/sh -c 'ulimit -n N && exec bun -e <script>'lowers both the soft and hard fd limits so bun cannot raise them at startup. The first test uses 24cats under a limit of 32, so the pipes can never fit, and covers captured stderr, stderr on an fd, a script that continues after the failed pipeline, and the thrownShellError. The second test grows the pipeline from 2 to 16 commands under a limit of 32 with the builtincat, so the runs cross from success into thedupfailure and then into thesocketpairfailure without any subprocess fds in the accounting.cmd_idx > 0path throwing.posix_spawnfollowed bypidfd_openfailing with EMFILE makesspawn_process.rsblock inwait4on a live child (the pipeline hangs at the firstcatspawn for someulimitvalues). That is a separate bug insrc/spawn_sys/spawn_process.rswith its own PRs.leak.test.tswas also run. Itsmemleak_*and#11816 > externalcases time out in this debug build environment for tests that do not involve pipelines too (memleak_redirect_file,memleak_change_cwd), so that is the environment, not this change.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/shell/bunshell.test.ts