cli: count a child whose output failed to read as failed
Stacked on #44031. Draft: the rule needs a design decision.
Problem
- With #44031 a failed read of a child's output closes the pipe and the command returns. A child that ignores EPIPE exits 0, and the command reports success with the output cut.
bun runprints no message for the failed read (on_reader_error,src/runtime/cli/filter_run.rs:258).
Fix
- One rule,
RunCommand::script_failure_code: a script whose output failed to read counts as failed, also with exit code 0. The exit code is the script's code, or 1. bun runprints the read error.--paralleland--sequentialhandle that script as any failed script.bun test --parallelkills the worker and fails its file.bun installfails when a lifecycle script exits 0 after the failed read.- Verified:
test/js/bun/spawn/spawn-stdio-syscall-error.test.ts(12 tests fail on the build of #44031). Self-reviewed: 14 concerns raised, 13 addressed (Notes).
Background
- The decision has two parts. Does a read that fails after the last byte fail a script that exited 0? Does an errno that can pass (ENOMEM, ENOBUFS) end the reader?
- Considered a message with no change of exit code. It keeps exit 0 for output that is not complete.
Downsides
- A script that did all its work and exited 0 fails the command when a read of its output fails.
--sequentialdoes not start the next script. bun test --parallelkills a worker in the middle of a file, so the file fails.- Release binary, linux-x64: text +3,584 B over #44031.
Notes
History. This rule was part of #44031 until a26c4e4e1c. A review before merge found that it needs a design decision and that the reader change does not, so it is a PR of its own. The code is the same as in a26c4e4e1c, plus one change: the coordinator drops the Ready and FileDone frames of a stopped worker.
The rule in each command (one read fails):
| Command | #44031 | this PR |
|---|---|---|
bun run --filter | the script's code, no message | prints error: Failed to read <script> script output from "<package>" due to error 5 EIO. Exits with the script's code, or 1 when that is 0 |
bun run --parallel, --sequential | the script's code, no message | prints <label> | Failed to read stdout due to error 5 EIO, same exit rule. The other scripts are stopped and the post script does not start. With --no-exit-on-error the next scripts run |
| status line of a script with lost output | Done | Exited with code 0 |
bun test --parallel, read fails in a file | 2 pass, exit 0 | the worker is killed, its file fails with worker killed: failed to read its output: EIO, exit 1 |
bun test --parallel, pipes fail at the start of a worker | 2 pass, exit 0 | a startup failure, the message names the cause. After two failures of each slot the files fail with no live workers |
bun install, lifecycle script that exits 0 | prints the read error, exit 0 | prints the read error, exit 1. An optional dependency is skipped |
bun test --parallel, details.
- The read error is a field of the
WorkerPipethat it ended. A slot that starts a new process makes new pipes, so an old error cannot stop the new process. Process::killdoes nothing on unix before the exit of the process is watched. SoWorker::startkills the worker again afterwatch_or_reapwhen a pipe failed before that.- A stopped worker can still deliver frames that it sent before.
Coordinator::on_framedrops itsReadyandFileDoneframes. So the file that it ran fails, it gets no other file, and a worker that was stopped before it was ready counts as a startup failure. - A worker that was told to shut down and whose last output fails to read gets a warning:
warn: failed to read the output of test worker 1: EIO.
No test covers two paths. The warning above, and the dropped frames. Both need a read that fails between the last frame of a worker and its exit. The shim cannot place a fault there.
Tests. Against #44031 this PR changes the expected result of 7 tests and adds 5: --sequential (two cases), a finished script whose output is lost (two cases), a lifecycle script that exits 0. Results: a release build of this PR passes 34 of 34. A release build of #44031 fails 12 of the 34, none of them hangs. A debug build of the same tree passed 34 of 34. Checked again with main merged at a4f1429148, which has #44086: 12 of 12 targets, cargo clippy, and 34 of 34 tests on a debug build.
Mutation checks (one clause reverted at a time, debug build, on the commits before the split):
| Clause | Test that fails |
|---|---|
| the lifecycle rule for exit code 0 | the install of the script that exits with code 0 exits 0 |
the lost-output arm of script_failure_code | three cases exit 0 |
the failure rule in the abort and dependents decision of --parallel | the --sequential case runs the next script |
| the kill of the worker | 4 pass, exit 0, no error line |
the kill after the exit is watched (Worker::start) | the worker start case never returns |
| the cause in the startup messages | the worker start case shows (SIGKILL) |
Self-review. It ran on the first version of this rule. Concerns that changed the diff: the lifecycle rule, one function for the exit rule, the worker taken out of rotation, the status line, tests with two packages. Not taken: to let the other scripts of --parallel run after a lost read. A script that counts as failed is handled as failed in each decision. A second run on the later state did not complete, so the changes after the first run have only the reviews on #44031.
Measurements (release builds, same toolchain, linux-x64): size bun text 80,853,798 with #44031 and 80,857,382 with this PR. Data and bss are equal. The stripped file grows by 4,096 B. No change to PosixBufferedReader: this PR does not touch src/io.
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/spawn/spawn-stdio-syscall-error.test.ts