Skip to content

fix: correctly handle pipe errors in builtins#953

Merged
reubeno merged 1 commit into
mainfrom
sigpipe
Jan 23, 2026
Merged

fix: correctly handle pipe errors in builtins#953
reubeno merged 1 commit into
mainfrom
sigpipe

Conversation

@reubeno
Copy link
Copy Markdown
Owner

@reubeno reubeno commented Jan 23, 2026

This should, among other things, resolve some of the flaky tests we've seen very recently. Those were cases where a pipe error was sometimes getting returned by the printf builtin.

@github-actions
Copy link
Copy Markdown

Test Results

    3 files     32 suites   11m 4s ⏱️
1 605 tests 1 605 ✅ 0 💤 0 ❌
4 790 runs  4 790 ✅ 0 💤 0 ❌

Results for commit 5793756.

@github-actions
Copy link
Copy Markdown

Public API changes for crate: brush-core

Added items

+pub fn brush_core::error::Error::as_io_error(&self) -> core::option::Option<&std::io::error::Error>
+pub fn brush_core::error::Error::as_io_error(&self) -> core::option::Option<&std::io::error::Error>
+pub fn brush_core::error::Error::as_io_error(&self) -> core::option::Option<&std::io::error::Error>
+pub fn brush_core::error::Error::as_io_error(&self) -> core::option::Option<&std::io::error::Error>
+pub fn brush_core::error::Error::as_io_error(&self) -> core::option::Option<&std::io::error::Error>
+pub fn brush_core::error::Error::as_io_error(&self) -> core::option::Option<&std::io::error::Error>
+pub fn brush_core::error::BuiltinError::as_io_error(&self) -> core::option::Option<&std::io::error::Error>
+pub brush_core::results::ExecutionExitCode::BrokenPipe
+impl core::convert::From<&std::io::error::Error> for brush_core::results::ExecutionExitCode
+impl core::convert::From<&std::io::error::Error> for brush_core::results::ExecutionExitCode
+pub fn brush_core::results::ExecutionExitCode::from(io_err: &std::io::error::Error) -> Self
+pub fn brush_core::results::ExecutionExitCode::from(io_err: &std::io::error::Error) -> Self
+pub brush_core::ExecutionExitCode::BrokenPipe
+pub fn brush_core::BuiltinError::as_io_error(&self) -> core::option::Option<&std::io::error::Error>

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 16.79 μs 16.78 μs -0.01 μs ⚪ Unchanged
eval_arithmetic 0.15 μs 0.15 μs -0.00 μs ⚪ Unchanged
expand_one_string 1.59 μs 1.61 μs 0.02 μs ⚪ Unchanged
for_loop 23.04 μs 22.90 μs -0.14 μs ⚪ Unchanged
function_call 2.28 μs 2.13 μs -0.15 μs ⚪ Unchanged
instantiate_shell 50.89 μs 51.80 μs 0.91 μs 🟠 +1.78%
instantiate_shell_with_init_scripts 25738.07 μs 24824.92 μs -913.15 μs 🟢 -3.55%
parse_bash_completion 2054.83 μs 2020.02 μs -34.81 μs 🟢 -1.69%
parse_sample_script 1.94 μs 1.94 μs -0.01 μs ⚪ Unchanged
run_echo_builtin_command 15.60 μs 15.74 μs 0.14 μs ⚪ Unchanged
tokenize_sample_script 3.46 μs 3.50 μs 0.04 μs ⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-builtins/src/alias.rs 🟠 73.91% 🟢 77.27% 🟢 3.36%
brush-builtins/src/bind.rs 🟢 81.76% 🟢 84.41% 🟢 2.65%
brush-builtins/src/complete.rs 🟢 83.38% 🟢 83.33% 🔴 -0.05%
brush-builtins/src/declare.rs 🟢 86.13% 🟢 85.98% 🔴 -0.15%
brush-builtins/src/hash.rs 🟢 92.73% 🟢 92.45% 🔴 -0.28%
brush-builtins/src/help.rs 🟢 89.66% 🟢 89.53% 🔴 -0.13%
brush-builtins/src/history.rs 🟢 89.15% 🟢 89.06% 🔴 -0.09%
brush-builtins/src/kill.rs 🟠 51.69% 🟠 54.76% 🟢 3.07%
brush-builtins/src/printf.rs 🟢 92.16% 🟢 93.2% 🟢 1.04%
brush-builtins/src/read.rs 🟢 91.53% 🟢 91.5% 🔴 -0.03%
brush-builtins/src/return_.rs 🟢 94.12% 🟢 93.75% 🔴 -0.37%
brush-builtins/src/set.rs 🟢 77.63% 🟢 77.53% 🔴 -0.1%
brush-builtins/src/shopt.rs 🟢 75.9% 🟢 77.22% 🟢 1.32%
brush-builtins/src/times.rs 🟢 90.48% 🟢 89.47% 🔴 -1.01%
brush-builtins/src/trap.rs 🟢 83.82% 🟢 83.58% 🔴 -0.24%
brush-builtins/src/type_.rs 🟢 83.76% 🟢 83.48% 🔴 -0.28%
brush-builtins/src/ulimit.rs 🟠 66.45% 🟠 66.67% 🟢 0.22%
brush-builtins/src/unalias.rs 🟢 83.33% 🟢 82.35% 🔴 -0.98%
brush-core/src/commands.rs 🟢 91.48% 🟢 91.79% 🟢 0.31%
brush-core/src/error.rs 🟢 88.14% 🟢 90.91% 🟢 2.77%
brush-core/src/expansion.rs 🟢 94.31% 🟢 94.23% 🔴 -0.08%
brush-core/src/extendedtests.rs 🟠 73.99% 🟠 74.75% 🟢 0.76%
brush-core/src/jobs.rs 🟠 51.38% 🟠 51.61% 🟢 0.23%
brush-core/src/results.rs 🟢 79.84% 🟢 79.37% 🔴 -0.47%
brush-core/src/wellknownvars.rs 🟢 83.94% 🟢 83.9% 🔴 -0.04%
brush-interactive/src/basic/term_line_reader.rs 🔴 38.14% 🔴 38.74% 🟢 0.6%
brush-interactive/src/interactive_shell.rs 🟠 63.4% 🟠 63.95% 🟢 0.55%
brush-parser/src/ast.rs 🔴 44.99% 🔴 45.09% 🟢 0.1%
brush-parser/src/parser/peg.rs 🟢 95.89% 🟢 96.16% 🟢 0.27%
brush-parser/src/test_command.rs 🟢 83.58% 🟢 85.07% 🟢 1.49%
brush-parser/src/tokenizer.rs 🟢 93.24% 🟢 93.46% 🟢 0.22%
brush-test-harness/src/execution.rs 🟢 80.75% 🟢 79.14% 🔴 -1.61%
brush-test-harness/src/reporting.rs 🔴 10.71% 🔴 11.62% 🟢 0.91%
brush-test-harness/src/runner.rs 🟢 82.51% 🟢 82.41% 🔴 -0.1%
xtask/src/common.rs 🟠 51.11% 🟠 52.27% 🟢 1.16%
Overall Coverage 🟢 74.66% 🟢 74.88% 🟢 0.22%

Minimum allowed coverage is 70%, this run produced 74.88%

Test Summary: bash-completion test suite

Outcome Count Percentage
✅ Pass 1558 73.87
❗️ Error 18 0.85
❌ Fail 179 8.49
⏩ Skip 339 16.07
❎ Expected Fail 13 0.62
✔️ Unexpected Pass 2 0.09
📊 Total 2109 100.00

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR standardizes how broken pipe (SIGPIPE) conditions from builtins are handled, ensuring they translate into the expected exit status (141) and do not surface as fatal or noisy errors in pipelines.

Changes:

  • Add a dedicated ExecutionExitCode::BrokenPipe variant and wire it into exit-code conversions, including from std::io::Error.
  • Extend the builtin error infrastructure (BuiltinError and Error) to expose underlying std::io::Errors so broken pipe causes can be detected centrally in execute_builtin_command.
  • Update printf to propagate I/O errors directly and add compat test cases to validate broken pipe behavior in pipelines and PIPESTATUS.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
brush-shell/tests/cases/compat/pipeline.yaml Unskips a previously TODO’d broken-pipe case and adds a new printf pipeline test to assert exit code 141 and correct PIPESTATUS.
brush-core/src/results.rs Introduces ExecutionExitCode::BrokenPipe and maps it to/from numeric code 141, integrating SIGPIPE into the unified exit-code model.
brush-core/src/error.rs Enhances the error model with BuiltinError::as_io_error, Error::as_io_error, and an impl From<&std::io::Error> for ExecutionExitCode, so I/O errors (notably broken pipes) can be converted into appropriate exit codes.
brush-core/src/commands.rs Adjusts builtin execution to intercept broken-pipe I/O errors, turning them into a normal ExecutionResult with exit code 141 instead of propagating a fatal or noisy error.
brush-builtins/src/printf.rs Changes printf to propagate underlying I/O errors as brush_core::Error (rather than wrapping them as usage errors), allowing broken pipes from printf to be recognized and mapped to 141.

@reubeno reubeno merged commit 516ba65 into main Jan 23, 2026
50 checks passed
@reubeno reubeno deleted the sigpipe branch January 23, 2026 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants