Repository navigation
shell: remove unsafe from the interpreter, state nodes, IOWriter/IOReader and Builtin - #40228
Open
Jarred-Sumner wants to merge 6 commits into
Open
Jarred-Sumner wants to merge 6 commits into
Jarred-Sumner wants to merge 6 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Collaborator
|
Updated 12:42 PM PT - Sep 6th, 2026
⏳ @autofix-ci[bot], your commit 9d93c46 is still building in |
Contributor
There was a problem hiding this comment.
The EPIPE hang I flagged in the previous run is fixed — OutputTask::write now parks the task on OutputQueue.parked (and bumps waiting) before calling enqueue_with, and the fill closure re-finds it by seq; the two new ls/mkdir closed-pipe tests cover exactly that scenario. I re-traced the dead-writer path through on_chunk → waiting -= 1 and it now balances.
Extended reasoning...
Re-checked the reworked OutputQueue/OutputTask in interpreter.rs against the failure mode from the previous review: the park now happens unconditionally before Builtin::write_out_with, so handle_dead_writer's early-return Yield::OnIoWriterChunk still routes to OutputTask::on_chunk(seq), which finds the parked task, decrements waiting, and continues to write_output/finish. The drained() gate in each builtin's next() therefore no longer stalls. Commit 42d6a27 and the new regression tests in ls.test.ts / mkdir.test.ts cover this. The remaining inline finding is a Windows-only test-assertion issue, not a runtime defect.
Jarred-Sumner
force-pushed
the
claude/subprocess-zero-unsafe
branch
from
August 24, 2026 22:39
f82faaf to
2d9fb62
Compare
Jarred-Sumner
force-pushed
the
claude/shell-zero-unsafe
branch
2 times, most recently
from
August 24, 2026 23:38
80264a4 to
a09c68f
Compare
Jarred-Sumner
force-pushed
the
claude/subprocess-zero-unsafe
branch
from
August 27, 2026 07:43
30e9a6e to
5f83101
Compare
Jarred-Sumner
force-pushed
the
claude/shell-zero-unsafe
branch
from
August 27, 2026 08:38
a09c68f to
393343a
Compare
Jarred-Sumner
force-pushed
the
claude/subprocess-zero-unsafe
branch
from
August 29, 2026 07:07
5f83101 to
c74f011
Compare
Jarred-Sumner
force-pushed
the
claude/shell-zero-unsafe
branch
from
August 29, 2026 07:50
393343a to
550a539
Compare
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 30, 2026
### 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 -->
Jarred-Sumner
force-pushed
the
claude/subprocess-zero-unsafe
branch
from
August 31, 2026 05:06
e3d78da to
005bf58
Compare
Jarred-Sumner
force-pushed
the
claude/shell-zero-unsafe
branch
from
August 31, 2026 05:43
0ffcbbd to
2dead19
Compare
Jarred-Sumner
force-pushed
the
claude/shell-zero-unsafe
branch
from
August 31, 2026 06:19
2dead19 to
2288ef8
Compare
Jarred-Sumner
force-pushed
the
claude/subprocess-zero-unsafe
branch
from
September 6, 2026 17:23
69765fa to
46f59f1
Compare
Jarred-Sumner
force-pushed
the
claude/shell-zero-unsafe
branch
from
September 6, 2026 19:26
2288ef8 to
c8ea1ef
Compare
Jarred-Sumner
force-pushed
the
claude/subprocess-zero-unsafe
branch
from
September 8, 2026 16:22
23dd8f5 to
b9ae552
Compare
…ader and Builtin State nodes live in a chunked RefCell arena addressed by NodeId; the shell env and captured output buffers are shared through Rc handles; IOWriter and IOReader are intrusively refcounted (RefPtr/ThisPtr) with cell fields; the parsed AST is owned by bun_shell_parser::ParsedScript and reached through BackRef; thread-pool tasks are boxed and bounce back through the node they embed (EventLoopTask::arm_boxed), so the round trip allocates nothing. Builtins no longer hold their Cmd node borrowed while an IOWriter runs (Builtin::write_out*), Stdio::Capture carries no buffer pointer, and Body::Value::with_request_or_response scopes the body borrow.
…n a dead pipe Node slots, the shell env and captured buffers use JsRefCell (RefCell semantics, checked in debug builds); the arena keeps its first chunk inline and builds further chunks in place; Stmt reuses its parent's IO; task and pool bounces stay allocation-free. Instruction counts vs the base branch (release, perf stat, 7 runs): 1000 builtins -0.15%, 500x stat/mkdir/touch/ glob +0.15%, 3000x JS $`true` +0.35%. ls/mkdir/touch/cp park each OutputTask under its own chunk sequence number (ChildPtr.seq) so every queued chunk is called back exactly once, including when the writer fails with EPIPE; rm numbers its verbose chunks the same way. Previously a builtin with several chunks queued when the pipe closed was called back once and never finished.
…dir path without building a regex from it
Jarred-Sumner
force-pushed
the
claude/shell-zero-unsafe
branch
from
September 8, 2026 16:41
9d93c46 to
c014292
Compare
This was referenced Sep 16, 2026
Collaborator
|
Note for a rebase: #43055 rewrites |
This was referenced Sep 17, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Same programme as #40055 … #40221. Stacked on #40204 (base branch
claude/subprocess-zero-unsafe; retarget to main once that lands). Every target insrc/runtime/shell/goes to 0:interpreter.rs53 → 0,states/{Cmd,Expansion,CondExpr,Async,Base,Pipeline,Script,Stmt,Subshell}.rs→ 0,IOWriter.rs7 → 0,IOReader.rs7 → 0,Builtin.rs10 → 0,IO.rs4 → 0,dispatch_tasks.rs5 → 0; builtins touched on the way:mv.rs8 → 0,touch.rs6 → 0,export.rs1 → 0,mkdir.rs6 → 1,ls.rs11 → 2,cp.rs17 → 3 (rm.rs's DirTask tree is its own raw-pointer design and is left for a follow-up).Interpreteraddressed by id; parent/child links are ids,child_donere-enters through the interpreter, and noRef/RefMuton a node, env, or captured buffer is held across a re-entrant call (builtins never hold their node borrowed acrossIOWriter::enqueue).ShellExecEnvhasDrop+clear();Bufio::Borrowed→Rc<RefCell<Vec>>.bun_shell_parser::ParsedScriptowns arena + root;root(&self) -> &Script<'_>is covariant/safe and the erased'staticform is only available asroot_backref() -> BackRef<Script<'static>>(one lifetime-only transmute with SAFETY in the parser crate).IOWriterRef/IOReaderRefRAII newtypes overRefPtr);impl_buffered_writer_parent!gets aborrow = thisentry whoseref_/derefhooks are the parent's intrusive count; pending-writer entries are typed; sync completions are deferred throughYield::OnIoWriterChunk.bun_event_loop::EventLoopTask::arm_boxed<T, R>(Box<T>, node)(JS arm = manual-deinitConcurrentTaskwithT::TAG, mini arm = trampoline toR::run_from_loop_thread(Box<T>)),AnyTaskWithExtraContext::{arm_boxed, load}(also fixes a pre-existing free-under-&mutin the mini loop — it now copies the node out before running),boxed_taskable!,crate::shell_task!(Ty);ShellTask.interpis anInterpRefopenable only on the owning thread. Bounces use the embedded nodes (no allocation per bounce; miniAsyncis 0 allocs vs 1 on main).bun_ptr::SelfRoot/RefPtr::new_cycliccherry-picked from bun_ptr: RefPtr::new_cyclic; a BackRef<_, Root> can no longer be dangling #40210;owned_task!/intrusive_work_task!/intrusive_field!accept a nested field path;EventLoopHandle::with_env;Body::Value::with_request_or_response(value, |v| ..);Stdio::Captureis a unit variant.Behaviour notes: a parser-internal error with no diagnostics surfaces as
ParseFailure::Other(thrown/returned) instead of an empty message; a pipeline/subshell child's duped env closes when its slot is freed (afterCmd::deinitrather than just before). Windows: a synchronous uv submission failure still re-entersIOWriter::on_errorfrom insidewrite()exactly as on main (io-layer follow-up).Perf (release,
perf stat -e instructions:u,taskset-pinned, interleaved, 7 runs, medians; base = #40204):bun run --shell=bunscript of 1000×true−0.15 %; 500× ([ -f ]+mkdir -p+touch+ glob) +0.15 %; JS loop 3000×$`true`+0.35 %. (Node state uses abun_ptr::JsRefCell— RefCell API/panics in debug,JsCellcost in release; node chunks are built in place with the first chunk inline.) No allocation per thread-pool or Async bounce (embedded nodes; miniAsyncis 0 allocs vs 1 on main).Pre-existing bug fixed in passing:
IOWriter::fail_pending_writerscalled each child back once while every output chunk of a builtin shared oneChildPtr, somkdir -v a b c | head -1/ls dir1 dir2 … | head -1could hang after EPIPE; each parkedOutputTasknow has its own sequence number and the four per-builtin output vtables are one genericOutputTask::{start, write, on_chunk}(tests added).Testing
Debug+ASAN: bunshell, commands/*, exec, throw, yield, lazy, leak, lex/parse/brace/glob/escape/env, file-io, epipe, pipeline_stack, worker-terminate-leak, seq-condexpr, sentinel, cmdsub-crash, blocking-pipe, read/write-fault, leak-args, shelloutput, bunshell-file/instance/default, run-shell, spawn-shell-signal, shell regression tests — 945 pass / 3 fail (root-user cases, identical on base) / 33 skip; new test
rm error with stderr redirected to a file. Hand-driven: pipelines mixing builtins and subprocesses,${Buffer}/${Response}redirects,&&/||/subshell/cmdsubst, brace+glob,.text/.lines/.quiet/.nothrow/.env/.cwd, throwing interpolation, Worker terminated mid-pipeline, 2000×echo hithen GC (ShellInterpretercount back to baseline) — exit codes correct, no ASAN output. clippy clean; source lints pass;rust-check-allwindows-msvc + apple-darwin pass.