shell: reject the ShellPromise when a state throws after the first suspension
Closed
RO
2mo ago+225+225+012345678901234567890123456789-12-12-01234567890123456789
Problem
- A state of the shell can throw a JS exception and return
Yield::Failed, for example> ${new Response("r")}on a command. If the command is not the first one, the process dies:panic: a task returned Ok with a JS exception pending(debug),panic(main thread): abort() called(release). TheShellPromisenever settles. Yield::run(src/runtime/shell/Yield.rs) handledFailedlikeSuspended. OnlyInterpreter::run_from_jschecks the VM afterwards. Under any other driver the exception stayed pending andfinishnever ran.- As the first command the promise rejected, but the keep-alive held the process open.
Fix
- The
Failedarm calls the newInterpreter::reject_with_pending_exception. It takes the exception, rejects through the cachedreject, disables the keep-alive and drops the pending-activity count. This covers all 20.run(interp)sites. - It frees nothing. The finalizer's
NeedsFullCleanuppath frees the nodes. While aPipelinenode is live the count is kept, because a running member's callbacks still reach into the interpreter. Failedproducers that never threw (sixWaitingWriteErrarms, WindowsIOReader::start) now returnSuspended, which the old trampoline treated the same.- Verified:
test/js/bun/shell/yield.test.ts(8 new cases, all time out on 1.4.3). Notes lists the other suites.
Background
- State functions return a
Yield.Yield::runsteps nodes until one suspends. Each async completion calls.run()again, with no JS frame above it. Bun.$caches the promise'sresolveandrejecton theShellInterpreterwrapper.finishcallsresolve.hasPendingActivitykeeps the wrapper alive until then.- A JS value in redirect position is inspected when its command reaches exec (
Builtin::init_redirections,Stdio::extract_blob).
Notes
Repro (1.4.3: abort() called at the timer, or a silent hang without it):
setTimeout(() => { console.log("2s: still pending"); process.exit(0); }, 2000);
const p = $`/usr/bin/touch marker; /usr/bin/printf B > ${new Response("r")}`.quiet().nothrow();
p.then(r => console.log("settled", r.exitCode), e => console.log("rejected:", e.message));
- History: #33996 first wrote this rejection path for the
ReadableStreamredirect panic. It was closed in favor of #40885, which rejects aReadableStreamwhen the template is parsed. Both note that a throw from redirect init after an async resume is stranded with no JS frame. This PR is the fix for that gap. It does not depend on #40885, and #35314 and #35318 add more exec-time throws that need it. - Stack of the throw:
VM::throw_error <- Stdio::extract_blob <- Cmd::init_subproc_redirections <- Cmd::transition_to_exec <- Cmd::next <- Interpreter::next_node <- Yield::run <- ShellSubprocess::on_process_exit <- Process::on_exit <- on_wait_pid_from_event_loop_task. There is no host-function boundary above it. - The 11 remaining
Failedproducers all throw first: theBuiltin.rsJsBuf arm (7 sites),Cmd::transition_to_exec,Cmd::on_io_writer_chunk(the write of a failing-error message failed),Expansion.rsandInterpreter::spawn_expr(adup()for a subshell or command substitution failed). The last three are syscall failures that need no misuse of the API:$`cmd1; missing-cmd`with a full stderr never settles on 1.4.3. On the mini event loop (bun runof a shell script)Interpreter::throwprints and exits, soFailednever reaches the trampoline there. - A
Failedwith no pending exception debug-asserts. In release builds it keeps waiting, which is what the old trampoline did, so a producer that usesFailedas a wait state cannot cause a spurious rejection. - Why the count is kept for pipelines: the finalizer's in-flight teardown is written for VM shutdown. On Windows it skips the
Cmdteardown (leak over use-after-free), and a builtin's thread-pool task holds a raw pointer back to the interpreter (#33996 hit a use-after-free onls * | cmd < ${stream}with a narrower predicate). A sequential script has nothing in flight when a later command fails, so it is safe to collect, and the newfailed scripts are collectedtest checks that it is. - Known limit, same as before this PR: a failed pipeline's interpreter lives until the VM exits, its started members keep running, and the members after the failed one are not started. The pipe ends of the failed member stay open, so a producer that writes more than the pipe buffer blocks, and the process does not exit on its own in that shape. Closing those pipe ends and stopping the started members is a separate change, tracked in #42282.
finishcannot run after a rejection: the failed node never reports to its parent, so no ancestor completes.finishdebug-asserts this.- A second failure in one interpreter is possible (
echo *.txt > ${a} | cat > ${b}: the glob suspends the first member, the second fails first). Thefailedflag makes the second one take and drop its exception. .nothrow()does not apply to these rejections. They are JS errors, not exit codes. The first-command case already behaved this way.- Suites run on the ASAN debug build:
bunshell,yield,throw,lazy,leak,shell-hang,pipeline_stack,epipe,shell-worker-terminate-leak,shelloutput,bunshell-instance,file-io,exec,shell-write-fault,shell-pipe-read-fault,commands/. Theleak.test.tsmemleak cases and twolspermission cases fail the same way on 1.4.3 in my container (slow debug build, running as root). - Probes under
BUN_GARBAGE_COLLECTOR_LEVEL=2andBUN_JSC_validateExceptionChecks=1: sequential,&&/||,if, subshell, non-quiet, first/last/middle pipeline member, double failure,ls -R * | failingwith repeated GC, and a Worker terminated with a failed pipeline still in flight. All reject and ASAN reports nothing. cargo checkpasses forx86_64-pc-windows-msvc.
no test proof · iteration 1 · 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