Skip to content

fix(pipeline): run compound command stages concurrently - #1242

Open
hartsock wants to merge 5 commits into
reubeno:mainfrom
hartsock:fix/compound-pipeline-stage-deadlock
Open

hartsock wants to merge 5 commits into
reubeno:mainfrom
hartsock:fix/compound-pipeline-stage-deadlock

Conversation

@hartsock

@hartsock hartsock commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

brush runs a compound command in a non-final pipeline stage inline to completion, so the stage that would drain its output never starts. Once the compound writes past one pipe buffer (64 KiB on Linux/macOS), it blocks forever. bash completes both of these.

# completes (62 KiB)
i=0; while [ $i -lt 2000 ]; do printf '%s\n' 0123456789012345678901234567890; i=$((i+1)); done | cat
# hangs (65 KiB)
i=0; while [ $i -lt 2100 ]; do printf '%s\n' 0123456789012345678901234567890; i=$((i+1)); done | cat

The common shape is ... | while read x; do ...; done | sort | head.

Fix. Spawn the stage when it owns its shell — the shape execute_via_builtin_in_owned_shell already uses for builtins. An owned shell is a throwaway clone whose mutations are discarded, so only when the stage runs changes, not which shell it mutates. Parent-shell stages — a single-command pipeline, or the last stage under lastpipe — still run inline and keep their side effects.

Validation. cargo xtask ci pre-commit clean; compat gains exactly the one added case (2301 ran: 1845 succeeded, 0 failed, 456 known to fail, against main's 2300/1844). Applied alone to unfixed main that case fails: the shell hangs and the harness kills it, so a regression fails the suite rather than hanging CI. Full numbers. Merged with current main (812336d).

Two things a reviewer may expect to be wrong: [[ ... ]], and overlap with #1276

[[ ... ]] since #1274. It is now CompoundCommand::ExtendedTest, so it reaches this arm. No exception is warranted: a bare [[ $x =~ $re ]] is a single-command pipeline and stays on the parent-shell path, so BASH_REMATCH is unaffected; as a non-final stage it was already evaluating against a discarded clone. Adding an exception would hand-rebuild the special case #1274 removed.

#1276 is complementary — measured, not assumed. Each repro still hangs under the other's fix (timeout 15, exit 124 = hung):

big | wc -l while …; done | wc -l
main 30a3bce hangs hangs
#1276 5000 hangs
this PR hangs 4096

That PR fixes execute_via_function in commands.rs; this one fixes the Compound arm in interp.rs. We both append to pipeline.yaml, so whoever lands second takes a trivial conflict — happy to be the one who rebases.

@hartsock

Copy link
Copy Markdown
Contributor Author

Found alongside #1184 and #1244 during practical testing of brush embedded as a shell library.

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 17.38 μs 17.82 μs 0.45 μs 🟠 +2.56%
eval_arithmetic 0.15 μs 0.16 μs 0.00 μs ⚪ Unchanged
expand_one_string 1.73 μs 1.80 μs 0.07 μs ⚪ Unchanged
for_loop 30.98 μs 31.21 μs 0.23 μs ⚪ Unchanged
full_peg_complex 58.94 μs 59.34 μs 0.39 μs ⚪ Unchanged
full_peg_for_loop 6.27 μs 6.30 μs 0.04 μs ⚪ Unchanged
full_peg_nested_expansions 16.32 μs 16.27 μs -0.06 μs ⚪ Unchanged
full_peg_pipeline 4.40 μs 4.36 μs -0.04 μs ⚪ Unchanged
full_peg_simple 1.84 μs 1.81 μs -0.03 μs 🟢 -1.36%
function_call 3.39 μs 3.35 μs -0.04 μs ⚪ Unchanged
instantiate_shell 54.66 μs 54.89 μs 0.22 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 25517.35 μs 26025.71 μs 508.36 μs 🟠 +1.99%
parse_peg_bash_completion 2113.71 μs 2121.23 μs 7.51 μs ⚪ Unchanged
parse_peg_complex 19.55 μs 19.57 μs 0.02 μs ⚪ Unchanged
parse_peg_for_loop 2.04 μs 2.03 μs -0.00 μs ⚪ Unchanged
parse_peg_pipeline 2.24 μs 2.27 μs 0.02 μs ⚪ Unchanged
parse_peg_simple 1.16 μs 1.15 μs -0.01 μs 🟢 -0.61%
run_echo_builtin_command 16.18 μs 16.02 μs -0.16 μs ⚪ Unchanged
tokenize_sample_script 3.54 μs 3.44 μs -0.10 μs 🟢 -2.94%

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
Overall Coverage 🟢 28.38% 🟢 28.37% 🔴 -0.01%

Minimum allowed coverage is 20%, this run produced 28.37%
Maximum allowed coverage difference is -5%, this run produced -0.01%

Test Summary: bash-completion test suite

Outcome Count Percentage
✅ Pass 1591 75.44
❗️ Error 13 0.62
❌ Fail 151 7.16
⏩ Skip 339 16.07
❎ Expected Fail 13 0.62
✔️ Unexpected Pass 2 0.09
📊 Total 2109 100.00

@hartsock
hartsock force-pushed the fix/compound-pipeline-stage-deadlock branch 3 times, most recently from 0934b05 to 91cc76a Compare July 31, 2026 03:24
@hartsock
hartsock force-pushed the fix/compound-pipeline-stage-deadlock branch 2 times, most recently from 1beace1 to fd4e0be Compare August 9, 2026 03:24
@hartsock
hartsock force-pushed the fix/compound-pipeline-stage-deadlock branch from fd4e0be to fa9fdb3 Compare August 11, 2026 03:24
@hartsock
hartsock force-pushed the fix/compound-pipeline-stage-deadlock branch from fa9fdb3 to 5eb515f Compare August 28, 2026 14:15
@hartsock

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (30a3bce) and revalidated. Head is now 5eb515f.

The rebase was not mechanical: #1274 moved [[ ... ]] from its own ast::Command::ExtendedTest arm into ast::CompoundCommand::ExtendedTest, which means extended tests now reach the exact arm this PR rewrites. The original description claimed they didn't — that was true when written and is not any more, so I checked the behavior rather than re-asserting it.

Result: [[ ... ]] should be spawned here, and no special case is warranted. A bare [[ $x =~ $re ]] is a single-command pipeline, so it still takes the ParentShell path and still publishes BASH_REMATCH; as a non-final stage it was already evaluating against a discarded shell.clone(), so spawning it cannot lose a side effect inline execution would have kept; and bash runs it in its own process too. Adding an explicit exception would hand-rebuild the special case #1274 removed. Description updated to say this.

Validation at 5eb515f against base 30a3bce:

  • cargo xtask ci pre-commit — fmt, build, clippy (--workspace --all-features --all-targets), schemas all clean; unit 378/378; integration (nextest --workspace) 2698 run, 2698 passed, 40 skipped.
  • cargo test --test brush-compat-tests at head — 2301 ran: 1845 succeeded, 0 failed, 456 known to fail, 39 skipped.
  • Same, on clean main 30a3bce2300 ran: 1844 succeeded, 0 failed, 456 known to fail. Exactly +1/+1, the added case; known-failures unchanged.
  • Before/after on the regression itself: applying only the new YAML case to unfixed main fails it — status mismatch: exit status: 0 from oracle vs. signal: 9 (SIGKILL) from test, i.e. brush hangs and the harness timeout kills it. It passes on this branch. So a regression here fails the suite rather than hanging CI.

(Linux/x86_64 locally; other platforms via CI.)

On #1276: complementary, not overlapping. Same invariant, two different dispatch sites — that PR fixes SimpleCommand::execute_via_function in commands.rs (a function call as a stage); this one fixes the ast::Command::Compound arm in interp.rs. f | cat is untouched by this PR and while ...; done | cat is untouched by #1276. The only interaction is textual: we both append to the end of pipeline.yaml, so whichever lands second takes a trivial conflict. Happy to rebase around it if you'd rather take #1276 first.

This is refreshed and ready for review — could it go into the needs-review queue?

@hartsock

Copy link
Copy Markdown
Contributor Author

The six red Source code checks jobs on this PR are inherited from main, not caused by it — flagging so the red doesn't read as "not ready".

All six fail at the Deny check step, and the identical six fail on #1184, which shares only the base. Reproduced in a clean worktree checked out at 30a3bce with nothing applied:

$ cargo deny --all-features check advisories
error[yanked]: detected yanked crate (try `cargo update -p chacha20`)
  chacha20 v0.10.1
warning[advisory-not-detected]: advisory was not encountered
  deny.toml:48  { id = "RUSTSEC-2026-0253", … }   # no crate matches this ignore any more
advisories FAILED, bans ok, licenses ok, sources ok

chacha20 0.10.1 was yanked from crates.io after the lockfile was pinned, so this started failing on its own. This PR changes two files — brush-core/src/interp.rs and brush-shell/tests/cases/compat/pipeline.yaml — and no Cargo.lock, so it cannot be the cause. The earlier run on this branch was 52/52 green on 2026-07-25, before the yank.

cargo update -p chacha20 --dry-run resolves cleanly to 0.10.2, so it looks like a one-line lockfile bump. The open dependabot branch still carries 0.10.1, so it isn't already in flight. I didn't want to smuggle a lockfile change into a behavioural PR — happy to open that as its own one-line PR if it's useful, or leave it to you.

@hartsock
hartsock marked this pull request as draft August 29, 2026 18:12
A compound command used as a non-final pipeline stage ran inline to
completion. Stages are set up in order, so the stage that would drain its
output did not exist yet; once the compound command wrote more than one
pipe buffer (64 KiB on Linux and macOS) it blocked forever.

Spawn it onto a blocking task when it owns its shell, the same shape
execute_via_builtin_in_owned_shell already uses for builtin stages. The
parent-shell case -- a single-command pipeline, or the last stage under
lastpipe -- still runs inline, preserving its side effects. That path
already executed against a shell clone, so only *when* the stage runs
changes, not which shell it mutates.

Since reubeno#1274, [[ ... ]] is CompoundCommand::ExtendedTest and so reaches this
arm. That needs no exception: a bare [[ ... ]] is a single-command pipeline
and stays on the parent-shell path, so BASH_REMATCH is unaffected, and as a
non-final stage it was already evaluating against a discarded clone.

Assisted-by: Claude Code:claude-opus-4-8
Assisted-by: Claude Code:claude-opus-5
@hartsock
hartsock force-pushed the fix/compound-pipeline-stage-deadlock branch from 5eb515f to c6c8ac1 Compare September 1, 2026 12:05
@hartsock
hartsock marked this pull request as ready for review September 7, 2026 18:40
@hartsock
hartsock marked this pull request as draft September 7, 2026 18:40
@hartsock

hartsock commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Out of draft.

Since the last pass: merged with current main (812336d), which also clears the Deny check red I flagged above — the lockfile now carries chacha20 0.10.2. And I reworded the comment on the spawn arm: an owned shell does not imply a non-final stage, since without lastpipe the last stage owns one too. What makes spawning safe is that the clone's mutations are discarded either way.

23 lines across interp.rs and pipeline.yaml. CI is re-running on eccc38a; the merge head under it was 52/52 green.

The comment said an owned shell means a non-final stage. That is not
always true: with lastpipe off (or job control on) the final stage gets an
owned shell too. What actually makes spawning safe is that the owned shell
is a throwaway clone whose mutations are discarded; say that instead.

Comment only, no behavior change.

Assisted-by: Claude Code:claude-opus-5
@hartsock
hartsock marked this pull request as ready for review September 10, 2026 18:49
@hartsock

Copy link
Copy Markdown
Contributor Author

I did several passes on this before allowing the agent to flip this out of "draft" it should be fairly concise now.

The Fedora OS-target job failed on the compat case
"Process substitution: input + output" (cp <(echo hi) >(cat)).

That case is unrelated to this PR: it is a pre-existing race in
setup_process_substitution, which fires the subshell with a bare
tokio::spawn and never tracks it, while entry.rs calls
std::process::exit as soon as the main future returns. If the outer
command finishes before a worker picks up the task, the substitution
never runs and its output is lost. Same root cause as issue reubeno#1140,
which reproduces deterministically:

    brush -c 'echo ok 1> >(tr o O)'   # prints nothing; bash prints Ok

This PR touches only the Compound arm of pipeline execution; the
failing case is a simple command and never reaches it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@avih

avih commented Sep 10, 2026

Copy link
Copy Markdown

Hi, and thanks for this fix.

Have you seen #1359 ?

According to my tests, on windows, a pipe hangs if it passes more than 4096 chars, but the latest CI build of this PR (of commit de64416) does not fix it on windows.

To me it looks like the same or similar issue as this PR tries to fix, but I don't know that for a fact.

Is this PR supposed to fix #1359 too?

If yes, could you please try the test script at the issue, and check whether it's fixed on windows or not?

For me it's not yet fixed.

Thanks.

@avih

avih commented Sep 13, 2026

Copy link
Copy Markdown

@hartsock ? (previous comment)

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