shell: reject the ShellPromise when a state throws after the first suspension

Closed
RO
2mo ago
+225-12

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). The ShellPromise never settles.
  • Yield::run (src/runtime/shell/Yield.rs) handled Failed like Suspended. Only Interpreter::run_from_js checks the VM afterwards. Under any other driver the exception stayed pending and finish never ran.
  • As the first command the promise rejected, but the keep-alive held the process open.

Fix

  • The Failed arm calls the new Interpreter::reject_with_pending_exception. It takes the exception, rejects through the cached reject, disables the keep-alive and drops the pending-activity count. This covers all 20 .run(interp) sites.
  • It frees nothing. The finalizer's NeedsFullCleanup path frees the nodes. While a Pipeline node is live the count is kept, because a running member's callbacks still reach into the interpreter.
  • Failed producers that never threw (six WaitingWriteErr arms, Windows IOReader::start) now return Suspended, 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::run steps nodes until one suspends. Each async completion calls .run() again, with no JS frame above it.
  • Bun.$ caches the promise's resolve and reject on the ShellInterpreter wrapper. finish calls resolve. hasPendingActivity keeps 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 ReadableStream redirect panic. It was closed in favor of #40885, which rejects a ReadableStream when 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 Failed producers all throw first: the Builtin.rs JsBuf arm (7 sites), Cmd::transition_to_exec, Cmd::on_io_writer_chunk (the write of a failing-error message failed), Expansion.rs and Interpreter::spawn_expr (a dup() 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 run of a shell script) Interpreter::throw prints and exits, so Failed never reaches the trampoline there.
  • A Failed with no pending exception debug-asserts. In release builds it keeps waiting, which is what the old trampoline did, so a producer that uses Failed as 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 Cmd teardown (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 on ls * | 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 new failed scripts are collected test 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.
  • finish cannot run after a rejection: the failed node never reports to its parent, so no ancestor completes. finish debug-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). The failed flag 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/. The leak.test.ts memleak cases and two ls permission cases fail the same way on 1.4.3 in my container (slow debug build, running as root).
  • Probes under BUN_GARBAGE_COLLECTOR_LEVEL=2 and BUN_JSC_validateExceptionChecks=1: sequential, &&/||, if, subshell, non-quiet, first/last/middle pipeline member, double failure, ls -R * | failing with repeated GC, and a Worker terminated with a failed pipeline still in flight. All reject and ASAN reports nothing.
  • cargo check passes for x86_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