shell(IOReader): drain readers instead of iterating a snapshot of the list

Closed
RO
19d ago
+39-21

What

IOReader.on_reader_done_cb and on_reader_error iterated a clone of readers while the loop body runs the Yield trampoline. The trampoline drives shell state machines that can call add_reader() on the same IOReader (the root stdin reader is shared across every command that inherits stdin). Two problems:

  • readers appended mid-loop are never notified, because the snapshot was taken before they existed
  • already-notified entries stay in the list, so add_reader's contains() dedup can match a freed-then-reused NodeId and silently skip registering the next reader

Both loops are replaced by drain_readers(), which pops each entry via swap_remove(0) before dispatching, so neither hazard applies and readers appended mid-drain are still picked up. A draining re-entrancy flag makes nested calls (reached from inside the trampoline) no-ops so the outermost loop owns the drain and Yield::run nesting stays bounded.

Windows

The new test exposed a separate pre-existing Windows crash: the BufferedReader nulls its source before invoking on_reader_done_cb, so when a reader's done-handler starts the next cat and calls start() from inside that callback, start_with_current_pipe() unwraps a None source and panics. echo | (cat && cat) had never been tested, so this never fired.

start() on Windows now checks reader().is_done() and drains the late-registered reader(s) instead of restarting the finished pipe. Because of the re-entrancy guard, this covers both call sites: reached from inside a drain (sync stdout write) it's a no-op and the outer loop handles the new reader; reached after the drain returned (async stdout, on_io_writer_chunk fires later) it starts a fresh drain. Without the fresh-drain path the test hung for 90s on Windows 11 aarch64 in CI.

Test

Added echo hi | (cat && cat && … ×12) to the pipeline-stack tests in bunshell.test.ts. Every cat shares the subshell's stdin IOReader; each done-handler appends the next reader and calls start() on the already-done reader. Crashes on Windows without this change; passes with it on all three Windows lanes plus debian-asan.

Rebase note

Earlier revisions of this PR changed both src/runtime/shell/IOReader.zig and src/runtime/shell/IOReader.rs. The Zig shell source has since been deleted from main (the Zig→Rust port completed), so the .zig half of the change was dropped during the rebase and the PR now touches only IOReader.rs and the test. The .rs logic is unchanged from what reviewers already looked at.