fix(interp): run function pipeline stages as background tasks, not inline

[vivo75]

Summary

Fixes a real deadlock: a shell function used as a non-last stage of a pipeline blocks forever once it writes more to its stdout than the OS pipe buffer holds (~64KiB on Linux), because the pipeline-spawning loop doesn't move on to the next stage (the one that would drain that pipe) until the function has fully returned.

Minimal repro, no external commands involved:

f() {
  for i in $(seq 1 5000); do echo "line $i"; done
}
f | cat > out.txt   # hangs forever

Real bash and every other shell run every pipeline stage concurrently, so this case never arises there.

Root cause

spawn_pipeline_processes() (brush-core/src/interp.rs) spawns each pipeline stage in a loop, .awaiting execute_in_pipeline() before moving on to the next stage:

let spawn_result = command
    .execute_in_pipeline(pipeline_context, cmd_params)
    .await?;

For external processes, and for builtins run in an owned (non-last-stage) shell, that .await resolves as soon as the work is spawned, not when it finishes — execute_via_builtin_in_owned_shell() already wraps the builtin in tokio::task::spawn_blocking, returning ExecutionSpawnResult::StartedTask immediately.

execute_via_function() had no such wrapping: it ran the function body inline (invoke_shell_function(...).await), so the loop genuinely blocked on the entire function body completing before the next pipeline stage was even spawned. If that function writes enough to stdout to fill the kernel pipe buffer before returning, its write() blocks — and since nothing is reading the other end yet (the next stage hasn't been spawned), it blocks forever.

This isn't just a synthetic case: any script piping the output of a function that produces more than ~64KiB (e.g. some_function | grep foo, some_function | wc -l, or shells that dump their own state through a function-based filter) hits this.

Fix

Split execute_via_function the same way execute_via_builtin already is:

• Owned-shell path (used for every pipeline stage except the last): spawns the function body as a background task via tokio::task::spawn_blocking + rt.block_on(...), mirroring execute_via_builtin_in_owned_shell exactly, and returns ExecutionSpawnResult::StartedTask immediately so the pipeline loop can proceed to spawn the next stage right away.
• Parent-shell path (used only for a pipeline's own last stage, which by definition never needs to unblock a downstream reader): unchanged, still awaits inline.

post_execute is intentionally not invoked in the new owned-shell path, matching execute_via_builtin_in_owned_shell's existing behavior — the owned shell is a throwaway clone discarded after the pipeline stage completes, so running post_execute against it has no observable effect on the parent shell.

Testing

• New compat case (brush-shell/tests/cases/compat/pipeline.yaml, "Function stage writing more than a pipe buffer before the next stage is spawned") reproduces the original hang under the compat suite's own 15s per-test timeout, so a regression here fails the test rather than hanging CI.

• Full brush-compat-tests suite passes identically before and after this change, confirmed via a clean (cargo clean) rebuild on both sides of the fix rather than trusting a cached binary:

  before: 2174 test case(s) ran: 1795 succeeded, 0 failed, 379 known to fail, 28 skipped.
  after:  2175 test case(s) ran: 1796 succeeded, 0 failed, 379 known to fail, 28 skipped.
  

  (+1/+1 is exactly the new test case; no other test's outcome changed.)

How this was found

Found while embedding brush_core::Shell as the bash-execution backend for a Gentoo Portage ebuild-phase-execution prototype. Real bin/phase-functions.sh's post-phase step pipes __save_ebuild_env | __filter_readonly_variables — both sides are shell functions, and __save_ebuild_env dumps every function and variable currently in scope via declare -f/declare -p. Small scripts stayed under the pipe-buffer threshold and worked fine; anything that had sourced a nontrivial number of shell functions into scope (as few as a couple hundred short functions) reliably deadlocked. Bisected down to execute_via_function's lack of concurrent spawning with the standalone repro above, independent of any of the ebuild-specific code.

[hartsock]

Hi @vivo75 — I have an open PR, #1242, that fixes the same class of deadlock at a different dispatch site, so I checked whether they collide. They don't: they're orthogonal. I cherry-picked your commit onto current main (30a3bce) locally to measure it, rather than guess — your branch untouched.

Both repros, timeout 15, exit 124 = hung:

	big | wc -l (function stage)	while …; done | wc -l (compound stage)
main 30a3bce	hangs (124)	hangs (124)
#1276 on main	5000, exit 0	hangs (124)
#1242 on main	hangs (124)	4096, exit 0

Your change is in SimpleCommand::execute_via_function (commands.rs); mine is in the ast::Command::Compound arm of ExecuteInPipeline (interp.rs). Neither reaches the other's path, so both are needed. Your commit also cherry-picks onto current main cleanly.

One thing worth fixing, though — the new case lands in the middle of the previous one. On main, pipeline.yaml ends:

  - name: "printf broken pipe returns 141 in PIPESTATUS"
    stdin: |
      printf '%s\n' {0..10000} | x=1
      echo "Last: $?, PIPESTATUS: ${PIPESTATUS[*]}"

and the diff inserts the new case before that last echo, so on the branch it reads:

  - name: "printf broken pipe returns 141 in PIPESTATUS"
    stdin: |
      printf '%s\n' {0..10000} | x=1

  - name: "Function stage writing more than a pipe buffer before the next stage is spawned"
    ...
      big | wc -l
      echo "Last: $?, PIPESTATUS: ${PIPESTATUS[*]}"

The PIPESTATUS case no longer prints PIPESTATUS, so it stops testing the thing it's named for — and it still passes, because oracle and test now both produce nothing. Would you mind putting that echo back on the printf case? Your own case reads fine either way.

Heads-up that we both append to the tail of pipeline.yaml, so whichever lands second takes a small conflict there — happy to be the one who rebases. I'm glad to review yours if a second pair of eyes is useful, and happy to fold both into one PR under your authorship if the maintainer would rather review a single change; your fix is the one that needs the more careful look, since it also has to collapse the nested ExecutionSpawnResult.