Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe shell interpreter now stops in-flight commands and builtin work after interruption, execution failure, or module-graph disposal. Stop tasks coordinate completion and cleanup. Tests cover interruption, failed pipelines, and disposed contexts. ChangesShell stop handling
Suggested reviewers: Priority: ⬆️ High Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the primary change: stopping running shell builtins and their pool work when a script stops. |
| Description check | |
The description provides detailed problem, fix, verification, limitations, performance data, and test results. It does not use the template headings exactly, but it contains the required information a… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description provides detailed problem, fix, verification, limitations, performance data, and test results. It does not use the template headings exactly, but it contains the required information and is complete.
✨ Finishing Touches: 💡 2
-
⚔️ Resolve merge conflicts: 💡
-
- Resolve merge conflict in branch
robobun/34fd9349/shell-stop-builtins-on-failure
- Resolve merge conflict in branch
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Updated 9:11 PM PT - Oct 21st, 2026
❌ @robobun, your commit edc0bfd has 2 failures in
🧪 To try this PR locally: bunx bun-pr 46168That installs a local version of the PR into your bun-46168 --bun |
|
Status for #46168. How the problems were reproduced on main (Linux x64, release build):
A fourth case was a The tests for these are in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the finish hold around the new stop task (queue_stop_task increments async_commands_executing, run_stop_task releases it after the reject callback, and release_unrun only occurs on VM teardown where the finalizer frees the interpreter) and the Cat::stop reader removal for a chunk lost to a sibling cat on a shared reader — neither turned up a defect.
Extended reasoning...
The change is a 21-file, +1466/-150 rework of the Rust shell interpreter's stop path (new ShellStopTask event-loop task, Builtin::stop, AtomicBool stop flags carried into pool tasks, deferred promise rejection via Strong pairs) plus three test files; it touches no auth, crypto or injection surface but is heavily lifetime- and thread-affinity-sensitive. Inline findings already flag behavioral regressions on Ctrl+C for background builtins and glob-expanding members, so a human look is needed; this note only records what else was examined.
Findings marked 🟡 are optional suggestions and need no follow-up push.
…6270) ### What does this PR do? Hardening follow-up to #46021. It fixes regressions from #46021 and #44618 that were never released, includes more of the open pull requests that fix a crash, and fixes a few crashes that had no pull request. #### Regressions fixed (none was released) - `spyOn(Bun, "$")`, and the same for `"sql"`, `"postgres"` and `"SQL"`, threw "does not support accessor properties yet". - Async `fs.read`, `fs.write` and `zlib.brotliCompress` on a view over a `WebAssembly.Memory` could lose data or copy far more than they needed. - `node:vm` with `vm.constants.DONT_CONTEXTIFY`: `this` in host accessors, prototype handling, `util.types.isProxy`, `Bun.deepEquals`, `assert.deepEqual` and `console.log` of a context. - `zlib` `stream.flush()` threw for arguments it used to accept, and streams had a new enumerable property. - `Bun.Transpiler` `exports.replace` with the name `"default"`. - `process.env.X = 3000` could store the number once the function was JIT-compiled. - `node:sqlite` and `bun:sqlite` could lose rows at exit. - Dev server: stylesheets shared between pages after a build error. - Shell `cp` of a file onto itself exited 1. - `error.stack`: the frame of `require()` reads `at anonymous (unknown)` again. - `import * as process from "process"` followed by `process.on(...)` threw at load. The EventEmitter methods that the module exports now act on `process`. - `delete res.statusCode; res.end()` in a `node:http` handler threw. It sends 200, as in Node. (from #46128) - `node:tls`, `node:https` and `node:http2`: connection teardown and write callbacks behave as in 1.4.3 and Node again. #### Crash fixes Bundler, parser and CSS: #42286, #46092, #46097, #46177, #46239, #36334, #43024, #40769. `bun test`, `--hot` and `--watch`: #30776, #38206, #38974. Shell: #43183, #37743, #46168, #46207, #34900. `node:http`, `node:http2`, `Bun.serve`, streams: #43449, #45380, #44457, #42197, #42485, #38082, #38124, #46074, #45999, #46045. `node:vm`, native addons, workers: #46129, #46198, #45615, #33412, #38031. Dev server: #42790, #42850, #39488. Other: #35991, #38030, #38586, #40526, #41830, #46029, #46078, #46114, and the bug of #42215. #### Behavior changes - `crypto.randomFill(view, cb)` reports `ERR_CRYPTO_OPERATION_FAILED` if it could not fill the view. It used to report success. - A function registered twice on `process` runs twice, as in Node. (#41830) - Under `--hot`, a reload after an uncaught exception cancels the timers that the failed run created. The new run's timers fire, and so do timers and `fs.watchFile` watchers that earlier runs kept on `globalThis`. The reload also resets the exit code to 0. (#38206) - The parent of a Worker no longer gets an `'error'` event for a failed `Bun.serve` request in that Worker. The error is printed, as on the main thread. - `hash.write()` after `digest()` drops the chunk, as in Node. (#35991) - `http2` over TLS with `server.emit("connection", socket)`: `session.socket` is a `TLSSocket` and a `'connection'` listener fires, as in Node. (#38124) - A `slice()` with no type of a body `Blob` has `type === ""`, as in Node. (#46114) - `util.types.isProxy(globalThis)` is false, as in Node. - A write on a `node:net` or `node:tls` socket that is still pending when the connection closes calls back with `ECANCELED`, as in Node. - `HTMLRewriter.on()` throws for a selector that is nested too deeply. - `Bun.Image`: `maxPixels` also bounds PNG metadata, `metadata()` of a GIF reports the size that gets decoded, and a path with a NUL byte throws. (#40526) - `bun build --no-bundle` with the `base64` or `dataurl` loader prints an error. (#36334) - Bundled output for a default-exported function or class expression changes to `var file_default = function name() {}`, as in esbuild. `.name` is unchanged. (#46097) - A Proxy in the prototype chain of a `node:vm` global is allowed. jsdom 30 with `runScripts` needs this. - `bun:sqlite` commits a write that `iterate()` left open when the main thread exits. An explicit `BEGIN` is still rolled back. - When a Worker stops while a native addon's async work is still running, the addon's finalizers run after that work is done, as in Node. - Shell: `mv -n` and `cp -n` don't overwrite, and `cp` reads `R`, `v` and `n` anywhere in a flag group. #### Performance - Ordinary `node:vm` contexts run at the same speed. With `DONT_CONTEXTIFY`, stores to globals and reads of inherited names in a hot loop cost 10 to 20 % more than on `main`. - A `process.env` write costs 20 to 35 % less than after #46021. - An async read into a `WebAssembly.Memory` view looks through the live memories: about 1.5 µs more with one memory. #### Not included - Taken in part, so these should stay open for the rest: #36334, #38974, #39488, #40526, #40769, #42485, #45615, #45999, #46029, #46092, #46114, #46168, #46207, #46239. - Needs a WebKit change: #34655 #39204 #40170 #40253 #41587 #41900 #42699 #42897 #42903 #43084 #43813 #44518 #44949 #45770 #45804 #45827. - #44723 (covered by #46097), #41675, #42753, #38762, #41475, #46016. ### How did you verify your code works? - Every item was reproduced first on release builds of `main`, of its parent and of 1.4.3, and compared with Node v26 where Node has the API. - Every fix has a test that fails on `main` and passes on a debug + ASAN build of this branch, except where that isn't practical (huge inputs, code that only runs on Windows or macOS). - The regressions were checked on grids of calls against the build before #46021 and against 1.4.3. - Bun's own test suites and Node's `test-vm-*`, `test-http2-*` and `test-zlib-*` files for the areas touched.
…pped A script is stopped from outside its commands when a member throws a JS error, when a Ctrl+C kills a foreground child, or when the Bun.ModuleGraph that started it is disposed. Each of these ended only the subprocesses. A builtin runs in the shell's own process, so it ran on: `child | yes` outlived the Ctrl+C that killed the child, a disposed graph's `yes` held the process, and a `cat` on an idle stdin held a rejected script. Interpreter::stop_in_flight is the one step for the three stops. It kills subprocesses as before and calls Builtin::stop for a running builtin: the chunks it queued are cancelled, `cat` leaves its reader, and a flag is set that the builtin's pool tasks read through ShellTask. A pool task that starts after the stop does nothing, and a walk or copy in progress ends at its next entry. rm's and mv's own stop signals are gone. Interpreter::fail no longer runs JS or reports the failed node under the trampoline that raised the failure. It stops what runs, parks the node and queues a task. On its turn the task reports the stopped builtins and the failed node, rejects the promise, and lets finish run. A catch block that turned the event loop (expect().resolves, Bun.build with an async plugin) used to run the exit of the killed member under frames that still named its nodes: `panic: expected Node::Pipeline at Node#2, got Free`. A script that a disposed graph starts is halted from its first command. A command substitution that a Ctrl+C cut short ends its command, so the command does not start with what the substitution had printed.
… the stop tests hold on macOS - `Cmd::stop` skips a builtin under an `&` command when a Ctrl+C is the cause (`in_background`), the rule a subprocess has. - yield.test.ts: one child for each place where a stopped `cat` runs, so a script that never settles names its place. The three places that need the `cat` of a regular file to end inside its own start run on Linux only: epoll refuses the file, kqueue takes it. - run_command.test.ts: the child waits for the process of the command substitution before it raises the Ctrl+C. A process that `bun run` spawns after the Ctrl+C never gets it. - The LeakSanitizer test for queued `ls` output moves next to the other leak tests in shell-worker-terminate-leak.test.ts. - Comments: why the stop flags use `Relaxed`, and which function holds `finish` for the stop task.
314f611 to
edc0bfd
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The follow-up commit (edc0bfd) addresses the four points from the earlier round: Cmd::stop now leaves an & builtin alone on a Ctrl+C via in_background, the Relaxed stores carry a comment pointing at ShellTask::stopped, the dispatch SAFETY comment names queue_stop_task, and the per-test timeout is gone from yield.test.ts. Beyond the new inline finding, I also checked that the ShellStopTask tag has arms in both run_task and release_task_unrun with the COUNT assert bumped to 87, so a stop task queued and never run is released rather than leaked.
Extended reasoning...
The change rewires how the Bun shell interpreter stops in-flight builtins and pool tasks across 18 source files plus three test files, with no security-sensitive surface. One confirmed finding remains (a debug-only assert in CondExpr reachable from an interrupted [[ $(...) ]]), and the size and lifetime-sensitive nature of the change mean a human look is still worthwhile.
| if interp.interrupted(this) { | ||
| // The script is halted or a Ctrl+C cut the substitution short: the rest of the word is not expanded. | ||
| { | ||
| let me = interp.as_expansion_mut(this); | ||
| me.state = ExpansionState::Done; |
There was a problem hiding this comment.
🟡 (optional) Debug builds now panic when a Ctrl+C or a Bun.ModuleGraph#dispose() cuts short a command substitution inside [[ ]]. Expansion.rs:613 ends the word on interp.interrupted(this) and reports exit 1 at Expansion.rs:622, but CondExpr::child_done (CondExpr.rs:238) asserts interp.failed() on that path, which only a JS error sets. Fix: let CondExpr::child_done accept an interrupted or halted substitution (gate the assertion on interp.interrupted(this) / halted(), or drop it) and finish the [[ ]] with exit 1 as Cmd::child_done does; release builds already take that path. The base gated the early return on interp.failed(), so the assertion held there.
Why this was flagged
Trigger: a Bun.$ script in a Bun.ModuleGraph runs [[ $(sleep 100) == x ]] and the graph is disposed; or, under bun exec, a pipeline such as sleep 100 | [[ $(sleep 50) == x ]] gets a Ctrl+C. Dispose runs the interpreter's on_abort (interpreter.rs:340-343), which calls halt and SIGKILLs the inner sleep; a Ctrl+C sets base.interrupted on the dying Cmd and propagate_interrupt carries it up. Either way the exit flows to the Expansion, where the new check at Expansion.rs:613 is true: the state is set to Done and child_done(parent, this, 1) runs at Expansion.rs:622 with the CondExpr as parent. Expansion::take_err returns None because the state is Done, not Err, and debug_assert!(interp.failed(), "Expansion child failed without an error") at CondExpr.rs:238 fails: failed() is false. On the base branch Expansion.rs gated this early return on interp.failed() alone, so the assertion's precondition always held. Release builds skip the assert and report exit 1, so the panic is debug-only.
Verification: nit (debug-only panic). Expansion.rs:613-622 sets state Done and returns child_done(parent, this, 1) on interp.interrupted(this), replacing the base's if interp.failed(). At CondExpr.rs:229-240 Expansion::take_err returns None, then debug_assert!(interp.failed(), "Expansion child failed without an error") fires because failed() is false.
This branch has not been deployed
1 participant
Refs #11868
Problem
Bun.$script: a JS error in a member, a Ctrl+C that kills a child,Bun.ModuleGraph#dispose(). Each ended only subprocesses (shell: reject the promise and stop the pipeline when a member throws a JS error #42288). A builtin ran on. Since 1.4.1 (bun run: leave Ctrl+C to the script and report its exit (Windows CTRL_C_EVENT, --shell=bun SIGINT) #39729),child | yesoutlives the Ctrl+C.Interpreter::fail(src/runtime/shell/interpreter.rs:1373) rejected under the shell's own frames. Acatchblock that turns the event loop ends inpanic: expected Node::Pipeline at Node#2, got Free.Fix
Interpreter::stop_in_flightserves the three stops.Builtin::stopcancels queued writes and sets a flag that its pool tasks read.failonly stops and queues a task. On the next event loop turn it reports the stopped builtins and the failed node, then rejects.test/js/bun/shell/yield.test.ts(19 of 20 new tests fail without the fix),test/cli/run/run_command.test.ts,shell-worker-terminate-leak.test.ts. Self-reviewed: 40 concerns raised, 37 addressed (Notes).Background
yes,cat,rm) runs inside the bun process. It has no process to kill, only a reader listener, queued writer chunks and pool tasks.Yield::run) names nodes by index. A frame reads a node again after a callee freed it, sofailreports nothing.yesinto a buffer.Downsides
$,Interpreter+48 B (same 640 B mimalloc bin), each pool task +8 B,.text+9.1 KB.Notes
Reach. Nobody reported this. It was found by an audit after #42288.
yeswith its output away from the broken pipe (yes 1>&2 | <member that throws>,child | yesunder Ctrl+C, a graph'syes).catandcpare builtins by default on Windows only. On POSIX they needBUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1.child | yes. main needs a second Ctrl+C.stop_in_flightis the engine such an API would call.What a stop does to each kind of work.
cat: it leaves the reader of its stdin, or of the file or FIFO operand it reads.ls,rm,mv,cp,mkdirandtouch: a task that starts after the stop does nothing. A walk or a copy in progress ends at its next entry.rmandmvuse this in place of their own signals.yesinto a buffer: its event loop task reports it on its next turn.The panic.
failcalled the promise'srejectunderevent_loop.entered(). When the failure came from a poll callback, the microtasks ran insidefail, and with them thecatchblock.expect().resolvesandBun.buildwith an async plugin turn the event loop from there. That turn ran the exit of the killed member, which freed the pipeline that the outer frames still named. main has this panic for a subprocess member (sleep 0.05; sleep 10 | echo hi > ${new Blob(["x"])}in atry, thenawait expect(Bun.sleep(50)).resolves). The failed node now stays parked until the task reports it, sofailneeds no exception for the node that failed.Ctrl+C and a command that still expands. In
child | echo x $(yes) > out.txt, the substitution is cut short.Expansion::child_donenow ends the word when its script was interrupted, soechodoes not start andout.txtis not made (as bash 5.2 and bun 1.4.0). main makes the file when the substitution runs a process. A command whose substitution child handles the signal and exits by itself still runs, as in bash.Not in this PR.
yes 1>&2 | server). A process that the script spawns between the two does not get the signal, as on main.cpfrom a FIFO) holds the shell until the call returns.[[ ]]that could not start, and the relayed output of a killed child, still drain.write(2).yes > filenever returns to the event loop (shell: let the event loop run whileyeswrites to a file or /dev/null #42817).ls a b | truedoes not settle with no failure involved (shell: fail every queued chunk when an IOWriter dies, not one per child #43183).cancel_chunkson a chunk whoseuv_writeis already out, with another writer queued behind it, has wrong accounting inIOWriter(a code trace, not a run). main reaches the same state fromcatand from the capture of a subprocess. It is not changed here because it cannot be run here.worker.terminate()), abun testtimeout and the--isolatefile swap (test --isolate: restore signal dispositions and kill Bun.$ children at the file swap #41878) are not stops.child | echo dir/** > out.txtmakes the file when the walk outlasts the child, as on main.&command is left alone by a Ctrl+C stop (in_background, the rule a subprocess has).&is a parse error today, so no test reaches it.true | cat filepanics withexpected Node::Pipeline at Node#2, got Freeon main and here. It needs no failure and no stop.mvhas no test that stops it while it moves files.Order against other PRs.
cpwalk is a work list, so the stop is read for each directory and each entry.yeswrites to a file or /dev/null #42817 rewrites theyesloop. Its bounce has to settask_queued.Measurements. main
6f58f6b894against this branch, release builds, Linux x64. They were taken before the rebase onto #46021.caton stdin 3007 -> 3 ms,cat < fifonever -> 3 ms,cat fifonever -> 3 ms,yes 1>&2never -> 3 ms,yesquiet never -> 8 ms,seq 1 100000 1>&2undrained 3017 -> 3 ms,ls d1 d2 d3 d4never in 9 of 10 -> 6 ms, dispose 3006 -> 3 ms.bun exec 'cat | sleep 100'with the flag: main is alive after 3000 ms in all 10. The branch exits 3 ms (median) after the Ctrl+C.child | yes: 1147 ms and a second Ctrl+C on main, 3.5 ms on the branch./proc/self/iosyscr, syscw. A control with no builtin is 2, 1): maincat4, 1. mainyes2, 2923 to 4099 (24 to 34 MB written). mainseq2, 3. The branch: 2, 1 for all three.cp -Rof 10,050 entries next to a member that throws 20 ms later: 1, 4 and 159 entries copied at exit in 3 runs.$`echo hi`.quiet()(gdbstepi, 2 runs each):createShellInterpreter4326 -> 4329,runFromJS25296 and 25321 -> 25294 and 25294,Cmd::deinit274 -> 274.Interpreter::next_node,start_node,Cmd::deinit,IOWriter::enqueueandIOWriter::bumphave 0 changed instructions after field offsets are normalized (llvm-objdump).child_doneis laid out again (1803 -> 1704 instructions).lspool task: 874 -> 897 instructions. One event loop turn ofyesinto a buffer (32 KB): 2222 -> 2281.Interpreter576 -> 624 B (allocation size inInterpreter::init). Debugsizeof:ShellSubprocess128 -> 80 B,ShellTask104 -> 112 B,ShellLsTask240 -> 248 B,ShellRmTask224 -> 232 B, rmExecState88 -> 96 B,Builtin328 B,Node176 B andCmd176 B unchanged.ShellStopTaskis 32 B and exists only after a stop..text: 107,124,483 -> 107,133,540 B (llvm-size).dataandbssunchanged. One more task tag.Suites (debug and ASAN build, Linux x64, after the rebase onto #46021).
yield.test.ts44 pass, the Ctrl+C tests ofrun_command.test.ts13 pass,shell-worker-terminate-leak.test.ts10 pass.1.4.3-canary.1build,USE_SYSTEM_BUN=1): 19 of the 20 new tests ofyield.test.tsfail (mkdir, touch, cp and rm -vin 3 of 5 runs, the others in every run), and 6 of the 7 new Ctrl+C tests fail. The two that pass guard this change: acatstopped while its reader reports, and a child that handles the Ctrl+C.rmand pool tests park the thread pool of their child, so they do not depend on timing. With the stop check ofrmdisabled in a build, thermtest fails on its file operand. With the call that frees queued output skipped in a debugger, LeakSanitizer reports 976 leakedOutputTask<Ls>in the queuedlstest.cat, and it timed out there. Three of the places read a regular file with the builtincatin a command substitution. On Linux thatcatends inside its own start (epoll refuses the file,catexits 1), which is the case those places are for. kqueue takes the file. Each place now has its own test, and those three run on Linux only.bun runhad not spawned the process of the substitution yet, that process never got the signal and the test hung (2 of 25 local runs). The child now waits for it: 0 of 65 runs.test/js/bun/shell/andtest/js/bun/shell/commands/: no failure that is not a timeout of a test that starts several debugbunprocesses, or that needs the registry.leak.test.ts,shell-hang.test.ts,shell-leak-args.test.tsandshell-load.test.tstime out on the debug build on this machine. They did so on main at the earlier base too.bun run rust:check-allforx86_64-pc-windows-msvcandaarch64-apple-darwin: both pass.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/yield.test.ts, test/js/bun/shell/shell-worker-terminate-leak.test.ts, test/cli/run/run_command.test.ts