Try to implement pipefail feature by WindSoilder · Pull Request #16449 · nushell/nushell

WindSoilder · 2025-08-16T13:52:16Z

The core requirement here is to have a way to track all exit status in a pipeline. To achieve this without affecting command itself.

I created a new struct in pipeline_data.rs:

pub struct PipelineExecutionData {
    pub body: PipelineData,
    /// This field is using to track all exit status in a pipeline.
    pub exit: Vec<Option<(Arc<Mutex<ExitStatusFuture>>, Span)>>,
}

Then I changed registers in EvalContext from &'a mut [PipelineData] to &'a mut [PipelineExecutionData]. So the eval engine itself can track exit status.
Then during evaluation, when nushell is going to execute Instruction::Call. It takes input.exit first, then attach this into results.exit. This is similar to the way nushell implements backtrace feature.

After all of this, when nushell executes Instruction::Drain, or printing pipeline. It iterates through exit_futures reversely. To see if a command exits with non-zero status.

Release notes summary - What our users need to know

This release will be add an experimental option for pipefail. This can be set using nu --experimental-options=['pipefail']

If set, the LAST_EXIT_CODE will be set to any command which exit with a non-zero status, or zero if all commands in the pipeline exit successfully.

> ^false | print aa
aa
> $env.LAST_EXIT_CODE
1

It'd be more interesting if using it with $env.config.display_errors.exit_code = true. Show users are able to see which command is failed.

> $env.config.display_errors.exit_code = true
> ^ls | ^false | print aa
aa
Error: nu::shell::non_zero_exit_code

  × External command had a non-zero exit code
   ╭─[entry #5:1:8]
 1 │ ^ls | ^false | print aa
   ·        ──┬──
   ·          ╰── exited with code 1
   ╰────

Tasks after submitting

Update the documentation

github-actions · 2025-08-16T13:52:22Z

Hey, just a bot checking in! You edited files related to the configuration.
If you changed any of the default values or added a new config option, don't forget to update the doc_config.nu which documents the options for our users including the defaults provided by the Rust implementation.
If you didn't make a change here, you can just ignore me.

WindSoilder · 2025-08-16T14:44:02Z

~~Sadly it failed to compile in wasm, I need to find a way to make PipelineExecutionData.exit field only available under feature = "os"~~

…uture`

132ikl

Looks good, after further review I think this PipelineExecutionData is definitely a good approach. I have some worries about the code structure here and how this might make the code more confusing for both newcomers and for trying to debug issues.

crates/nu-cli/src/eval_cmds.rs

crates/nu-protocol/src/pipeline/pipeline_data.rs

crates/nu-engine/src/eval.rs

crates/nu-experimental/src/options/pipefail.rs

tests/shell/pipeline/commands/external.rs

crates/nu-protocol/src/pipeline/pipeline_data.rs

132ikl · 2025-08-18T21:25:38Z

crates/nu-protocol/src/pipeline/pipeline_data.rs

+            PipelineData::Empty | PipelineData::Value(..) | PipelineData::ListStream(..) => None,
+            PipelineData::ByteStream(stream, ..) => match stream.source() {
+                ByteStreamSource::Read(..) | ByteStreamSource::File(..) => None,
+                ByteStreamSource::Child(c) => Some(c.clone_exit_status_future()),


This is clever, and also a little bit strange. I think this is probably acceptable, it just feels a little weird that ByteStreamSource is load-bearing for pipefail (and the evaluation engine more generally). I don't know where else we would get that info from though, so this is probably fine?

We should definitely have a comment on the ByteStreamSource::Child variant noting that we are using it for pipefail, and call out this function in that comment

I added a comment to the method, not sure is it ok enough?

Co-authored-by: rose <132@ikl.sh>

crates/nu-experimental/src/options/pipefail.rs

crates/nu-protocol/src/process/child.rs

cptpiepmatz · 2025-08-30T08:55:40Z

crates/nu-protocol/src/process/child.rs

    fmt::Debug,
    io::{self, Read},
    sync::mpsc::{self, Receiver, RecvError, TryRecvError},
+    sync::{Arc, Mutex},


Maybe instead of the std::sync::Mutex, we could use a parking_lot::Mutex. This way you don't need to expect lock results. We already have parking_lot in our dependency graph, so adding it should be fine.

okay, after some research i'm convinced now about parking_lot::Mutex. i'd support that if @WindSoilder wanted to add that for this change in this PR but i'm also fine with moving forward without it

I read the documentation of parking_lot:

This library provides implementations of Mutex, RwLock, Condvar and Once that are smaller, faster and more flexible than those in the Rust standard library. It also provides a ReentrantMutex type.

But after I ran benchmark, the result doesn't support it:

load_standard_lib [ 2.8 ms ... 2.8 ms ] +1.28% load_use_standard_lib [ 22.9 ms ... 22.6 ms ] -1.35% record_create_1 [ 105.0 us ... 105.7 us ] +0.64% record_create_10 [ 121.8 us ... 124.9 us ] +2.56%* record_create_100 [ 334.8 us ... 334.3 us ] -0.15% record_create_1000 [ 3.6 ms ... 3.8 ms ] +5.45%* record_flat_access_1 [ 103.7 us ... 105.7 us ] +1.93% record_flat_access_10 [ 121.1 us ... 120.9 us ] -0.15% record_flat_access_100 [ 113.7 us ... 114.3 us ] +0.53% record_flat_access_1000 [ 253.7 us ... 257.2 us ] +1.40% record_nested_access_1 [ 119.1 us ... 122.7 us ] +3.07% record_nested_access_2 [ 159.4 us ... 160.8 us ] +0.88% record_nested_access_4 [ 134.4 us ... 136.2 us ] +1.32% record_nested_access_8 [ 124.5 us ... 123.9 us ] -0.50% record_nested_access_16 [ 124.5 us ... 123.9 us ] -0.43% record_nested_access_32 [ 135.7 us ... 141.0 us ] +3.88% record_nested_access_64 [ 210.8 us ... 213.8 us ] +1.41% record_nested_access_128 [ 213.5 us ... 215.1 us ] +0.76% record_insert_1_1 [ 219.0 us ... 219.5 us ] +0.22% record_insert_10_1 [ 178.3 us ... 175.9 us ] -1.34% record_insert_100_1 [ 249.6 us ... 250.4 us ] +0.33% record_insert_1000_1 [ 776.4 us ... 793.8 us ] +2.24% record_insert_1_10 [ 196.9 us ... 201.0 us ] +2.09% record_insert_10_10 [ 198.5 us ... 200.7 us ] +1.13% record_insert_100_10 [ 206.6 us ... 214.9 us ] +4.03%* record_insert_1000_10 [ 812.6 us ... 849.6 us ] +4.55% table_create_1 [ 136.2 us ... 140.2 us ] +2.90% table_create_10 [ 145.7 us ... 147.2 us ] +1.06% table_create_100 [ 456.8 us ... 459.0 us ] +0.49% table_create_1000 [ 3.3 ms ... 3.3 ms ] -0.64%* table_get_1 [ 107.6 us ... 111.4 us ] +3.55%* table_get_10 [ 110.4 us ... 112.2 us ] +1.63%* table_get_100 [ 122.6 us ... 122.2 us ] -0.30% table_get_1000 [ 219.5 us ... 225.7 us ] +2.81%* table_select_1 [ 105.2 us ... 106.8 us ] +1.48% table_select_10 [ 106.5 us ... 107.6 us ] +1.04% table_select_100 [ 126.5 us ... 130.0 us ] +2.84%* table_select_1000 [ 308.9 us ... 315.6 us ] +2.20%* table_insert_row_1_1 [ 111.5 us ... 113.4 us ] +1.69% table_insert_row_10_1 [ 110.7 us ... 113.5 us ] +2.55%* table_insert_row_100_1 [ 117.1 us ... 119.8 us ] +2.33%* table_insert_row_1000_1 [ 160.6 us ... 163.2 us ] +1.65% table_insert_row_1_10 [ 205.3 us ... 208.2 us ] +1.44%* table_insert_row_10_10 [ 206.5 us ... 206.5 us ] -0.02% table_insert_row_100_10 [ 218.1 us ... 216.2 us ] -0.84% table_insert_row_1000_10 [ 282.8 us ... 281.3 us ] -0.54% table_insert_col_1_1 [ 100.6 us ... 101.5 us ] +0.85% table_insert_col_10_1 [ 106.5 us ... 106.7 us ] +0.22% table_insert_col_100_1 [ 138.8 us ... 140.9 us ] +1.56% table_insert_col_1000_1 [ 422.0 us ... 419.9 us ] -0.49% table_insert_col_1_10 [ 142.2 us ... 141.4 us ] -0.57% table_insert_col_10_10 [ 157.7 us ... 156.9 us ] -0.54% table_insert_col_100_10 [ 289.6 us ... 292.0 us ] +0.83% table_insert_col_1000_10 [ 1.6 ms ... 1.6 ms ] +2.89%* eval_interleave_100 [ 2.2 ms ... 2.3 ms ] +2.05% eval_interleave_1000 [ 18.8 ms ... 17.4 ms ] -7.31%* eval_interleave_10000 [ 134.7 ms ... 136.8 ms ] +1.61% eval_interleave_with_interrupt_100 [ 2.2 ms ... 2.2 ms ] -1.02% eval_interleave_with_interrupt_1000 [ 18.3 ms ... 17.5 ms ] -4.48% eval_interleave_with_interrupt_10000 [ 137.2 ms ... 137.2 ms ] +0.05% eval_for_1 [ 113.0 us ... 114.4 us ] +1.24% eval_for_10 [ 120.5 us ... 122.5 us ] +1.64%* eval_for_100 [ 137.9 us ... 140.1 us ] +1.55% eval_for_1000 [ 273.3 us ... 275.3 us ] +0.73% eval_for_10000 [ 1.6 ms ... 1.6 ms ] +0.37% eval_each_1 [ 132.6 us ... 135.0 us ] +1.86%* eval_each_10 [ 133.9 us ... 138.3 us ] +3.23%* eval_each_100 [ 160.7 us ... 164.5 us ] +2.34%* eval_each_1000 [ 391.1 us ... 403.8 us ] +3.27%* eval_each_10000 [ 2.6 ms ... 2.8 ms ] +5.00%* eval_par_each_1 [ 293.0 us ... 293.9 us ] +0.33% eval_par_each_10 [ 296.7 us ... 295.4 us ] -0.43% eval_par_each_100 [ 323.0 us ... 334.3 us ] +3.49%* eval_par_each_1000 [ 545.1 us ... 551.2 us ] +1.12% eval_par_each_10000 [ 2.9 ms ... 3.0 ms ] +1.97% eval_default_config [ 338.3 us ... 337.0 us ] -0.40% eval_default_env [ 413.7 us ... 412.2 us ] -0.38% encode_json_100_5 [ 101.3 us ... 106.6 us ] +5.27%* encode_json_10000_15 [ 26.9 ms ... 28.4 ms ] +5.70%* encode_msgpack_100_5 [ 61.4 us ... 66.0 us ] +7.60%* encode_msgpack_10000_15 [ 16.0 ms ... 17.3 ms ] +8.55%* decode_json_100_5 [ 410.0 us ... 409.7 us ] -0.05% decode_json_10000_15 [ 114.5 ms ... 114.7 ms ] +0.23% decode_msgpack_100_5 [ 115.1 us ... 125.7 us ] +9.21%* decode_msgpack_10000_15 [ 33.8 ms ... 36.5 ms ] +7.77%*

I don't much better results running toolkit benchmark-compare main main, so I'm not really sure about that. Using the parking_lot::Mutex is just easier imo. But we can keep the std one.

cptpiepmatz · 2025-09-05T23:05:59Z

@WindSoilder this has some conflicts but looks fine otherwise.

@132ikl Take a look if this PR is ready yet, I think we could merge it after the conflicts are fixed.

Hofer-Julian · 2025-09-08T05:22:57Z

Thank you for working on this @WindSoilder 🥳

WindSoilder added 20 commits August 15, 2025 21:48

WIP

1117248

rename from TmpPipelineData to PipelineDataWithExit

6224d4b

finish debugger changes

cfd4604

use Arc<Mutex<ExitStatusFuture>>

d6c96a6

finish changes on child

a2d8aa5

clone exit_status future

621f4f7

WIP

ad540ac

WIP

efb91b1

finish collecting

55bee2e

handle for drain

861521b

WIP

8b4d699

WIP

c852841

WIP

31ca587

make some rename

76b737f

track span on exit status future

42b839f

impl check_exit_stauts_future_ok

6aacfc7

add a test for this

40af386

add doc_config

588370d

fix clippy

92f2865

fix

39843a8

WindSoilder added 3 commits August 16, 2025 21:53

fix typo

0f29f91

no unwrap

2312244

try guard os feature, and fix clippy

423ffa0

WindSoilder added 2 commits August 16, 2025 23:05

try to make it compile on wasm, so I introduce a `check_exit_status_f…

f43e8fe

…uture`

wasm: more try

0c4656a

WindSoilder force-pushed the pipefail branch from 7891af5 to 0c4656a Compare August 16, 2025 15:09

WindSoilder added 2 commits August 16, 2025 23:13

wasm: more try

27faa5a

wasm: more try

ca12382

132ikl requested changes Aug 18, 2025

View reviewed changes

WindSoilder and others added 4 commits August 19, 2025 16:29

Update crates/nu-protocol/src/pipeline/pipeline_data.rs

6c4acf8

Co-authored-by: rose <132@ikl.sh>

Update crates/nu-protocol/src/pipeline/pipeline_data.rs

dd7c87a

Co-authored-by: rose <132@ikl.sh>

add some test cases

67f4c21

remove some print

7da0fae

WindSoilder marked this pull request as draft August 20, 2025 01:50

WindSoilder added 2 commits August 20, 2025 17:08

ignore one more test

9cd75c6

change eval_block/eval_block_with_early_return's prototype

b78a615

github-actions bot added the deprecated:pr-plugins Deprecated: use the A:plugins label instead label Aug 22, 2025

WindSoilder added 2 commits August 22, 2025 17:46

change pipefail to failed with rightmost exit status

27c140d

adjust test

e45fc92

WindSoilder marked this pull request as ready for review August 22, 2025 12:18

Merge branch 'main' into pipefail

38496c1

cptpiepmatz self-requested a review August 27, 2025 20:51

Merge branch 'main' into pipefail

b80808e

cptpiepmatz reviewed Aug 30, 2025

View reviewed changes

WindSoilder added 3 commits September 1, 2025 15:02

return early, and make a better description

9f35eef

simplify code

5fd6fd2

fix clippy

4a05780

cptpiepmatz approved these changes Sep 1, 2025

View reviewed changes

merge from main

9ae903c

132ikl approved these changes Sep 8, 2025

View reviewed changes

132ikl merged commit f712c37 into nushell:main Sep 8, 2025
17 checks passed

github-actions bot added this to the v0.108.0 milestone Sep 8, 2025

This was referenced Sep 24, 2025

Pipefail: fix if there is a variable or simple expression inside a pipeline. #16748

Closed

Experimental Option Tracking: pipefail #16760

Open

cptpiepmatz added notes:ready The "Release notes summary" section of this PR is ready to be included in our release notes. notes:additions Include the release notes summary in the "Additions" section labels Oct 12, 2025

Conversation

WindSoilder commented Aug 16, 2025 • edited by Bahex Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes summary - What our users need to know

Tasks after submitting

Uh oh!

github-actions bot commented Aug 16, 2025

Uh oh!

WindSoilder commented Aug 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

132ikl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

132ikl Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

WindSoilder Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cptpiepmatz Aug 30, 2025

Choose a reason for hiding this comment

Uh oh!

132ikl Aug 31, 2025

Choose a reason for hiding this comment

Uh oh!

WindSoilder Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

cptpiepmatz Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

cptpiepmatz commented Sep 5, 2025

Uh oh!

Uh oh!

Hofer-Julian commented Sep 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WindSoilder commented Aug 16, 2025 •

edited by Bahex

Loading

WindSoilder commented Aug 16, 2025 •

edited

Loading