tee: replace the pipe a failed output used by abendrothj · Pull Request #15132 · uutils/coreutils

abendrothj · 2026-10-06T07:28:35Z

On Linux, tee copies its input to the outputs through two pipes. When splicing to one output failed part-way (EFBIG, ENOSPC), the bytes it didn't take stayed in the pipe. The other outputs would get them in front of their next chunk, and with the 2nd pipe left part-full tee panicked on its 2nd pipe should have enough spare assertion.

After a failed output, the old pipe is now closed and replaced with an empty one. A new pipe gets the larger size only if the 2nd pipe got it at startup, so the 2nd is never smaller than the 1st. The old pipe is closed first, so this also works at the open file limit. If no new pipe can be made, tee fails with the error. test_tee_failed_output_does_not_spill_into_others and test_tee_failed_output_at_open_file_limit fail on main and pass here.

Split out of #15086.

Closes #15119

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

abendrothj · 2026-10-06T08:10:38Z

Sign in to view

+                    splice_or_detach!(
+                        pipe2_read,
+                        pipe2_write,
+                        pipe::<true>(),


No, pipe::<true>() is a macro argument and only runs inside the drain_pipe error branch, so a new pipe is made once when an output fails, and that output is removed right after. The success path doesn't change.

Done in ff0fcb9. The macro now makes the new pipe itself, enlarged only if the 2nd pipe was enlarged at startup.

abendrothj · 2026-10-06T08:10:40Z

Sign in to view

+                    .is_err();
                    $writer.name.clear(); //mark as exited
+                    // the failed write can leave bytes in the pipe: replace it with an empty one.
+                    // Free it first, so that the new one does not need more file descriptors.


Done in 6997600. At the open file limit tee now can't make the new pipe and continues with read/write, which test_tee_failed_output_at_open_file_limit still covers.

I missed your edit, sorry. The drops are back in ff0fcb9.

One of an annoying fact is lack of kernel API to make pipe empty. splice to /dev/null is theorically faster if /dev/null is cached, but it assumes /dev is mounted.

Agreed, that's why it makes a new pipe instead.

Previously, cat, tee, etc... was fallbacking from all splice error (instead of 1st splice only) at cat, tee by reading everything of pipe to userspace RAM. So the bug was not existing previously. It has simpler code base. However, we cannot recover the behaviour because GnuTests wants to catch EIO of 2nd splice...

I am still not sure if we should close & open pipes, or read everything to RAM...

I think recreating pipes is better for other utils becuase we can safely fallback from fast-path when EMFILE happened, but about tee, it might not.

I'd keep recreating, as you suggested for the pipe cache in #15086. the old pipe is closed first, so the fd limit doesn't block it (test_tee_failed_output_at_open_file_limit runs with 10 fds). if pipe() still fails, tee reports it and stops; no test reaches that.

I think we can still cause EMFILE by system's limit. This looks race. But this PR is OK at a monent.

oech3 · 2026-10-06T07:41:53Z

                    return Ok(());
                }
-                let Some((last, others)) = self.writers.split_last_mut() else {
+                let Some(last) = self.writers.len().checked_sub(1) else {


Please revert this. Loop by index is difficult to understand.

Reverted to split_last_mut in 6997600. The fallback still needs the index for drain(..=i), so it's others.iter_mut().enumerate().

Why do we need done array?

Outputs up to i already have this chunk, and write_flush writes to all of self.writers. So they're taken out while the rest gets the leftover input, then put back in front to keep the order. I can add a write_flush that starts at an index instead, if you prefer.

Failed writers can be marked by empty names. Is it not enough? Previous code did not have Vec allocation.

Yes, done in 6e1c349: the fallback writes to the outputs after the failed one in place and marks failures by clearing the name, so the done Vec is gone.

remove index too.

Removed the fallback: if no new pipe can be made, tee now fails with the error.

Yes. drop minimizes risk of failure (e.g. EMFILE) and we ahready know that pipe(2) is supported on the system at here. So catching error of pipe(2) should be fine.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

sylvestre · 2026-10-06T08:29:29Z

Sign in to view

                    let tee_res = uucore::pipes::tee(&pipe_read, &pipe2_write, s);
                    assert_eq!(tee_res, Ok(s), "2nd pipe should have enough spare");
-                    splice_or_detach!(&pipe2_read, other, s);
+                    splice_or_detach!(pipe2_read, pipe2_write, *other, s, {


since the drops are back, no test reaches this fallback anymore, no?
could we keep it simpler or please add a test for it

Right, with the drops back no test reaches it: once the old pipe is freed, pipe() there only fails for reasons a test can't set up reliably (system-wide file or memory limits, or the per-user pipe buffer limit refusing the resize). I made the fallback smaller in 6e1c349. If you'd rather not keep it untested, the other option is removing the drops again, which lets the open-file-limit test reach it. (This is about the block that runs when no new pipe can be made, not the assert_eq!.)

it's simpler now: if no new pipe can be made, tee reports the error and stops. still no test reaches it.

sylvestre · 2026-10-06T08:29:35Z

Sign in to view

                }
                // last one consumes input
-                splice_or_detach!(&pipe_read, last, s);
+                splice_or_detach!(pipe_read, pipe_write, *last, s, {


this is the same retain + aborted check as just below, could be dedup?

Done in 6e1c349: both fallbacks now break out of the loop, and the retain and aborted check is one remove_exited(), called in the loop and after it.

done when the fallback went, failed writers are removed in one place again.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

abendrothj · 2026-10-06T09:32:28Z

Sign in to view

                let _ = fcntl_setpipe_size(&pipe_read, MAX_ROOTLESS_PIPE_SIZE);
                let _ = fcntl_setpipe_size(&self.writers[0], MAX_ROOTLESS_PIPE_SIZE); // stdout
            }
+            macro_rules! splice_or_detach {


Moved it back and squashed.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

sylvestre · 2026-10-06T14:15:04Z

Seems that it makes coverage sad:


    running 1 test
    bin: "/home/runner/work/coreutils/coreutils/target/debug/coreutils"
    write(default): /tmp/.tmpDxGTOX/in
    run: /home/runner/work/coreutils/coreutils/target/debug/coreutils tee capped /dev/null capped2
    test test_tee::linux_only::test_tee_failed_output_does_not_spill_into_others ... FAILED

    failures:

    ---- test_tee::linux_only::test_tee_failed_output_does_not_spill_into_others stdout ----
    ---- test_tee::linux_only::test_tee_failed_output_does_not_spill_into_others stderr ----


    failures:
        test_tee::linux_only::test_tee_failed_output_does_not_spill_into_others

    test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 6075 filtered out; finished in 0.30s
    
  stderr ───

    thread 'main' (80690) panicked at tests/by-util/test_tee.rs:730:14:
    assertion failed: `(left == right)`

    Diff < left / right > :
     tee: capped: File too large
     tee: capped2: File too large
    <LLVM Profile Error: Failed to write file "/home/runner/work/coreutils/coreutils/coverage/traces/coverage-15187755631959443666_2.profraw": File too large

tee duplicates its input through two pipes on Linux. When splicing from one of them to an output failed part-way, for example with ENOSPC or EFBIG, the bytes the output did not take stayed in the pipe, and the remaining outputs got them in front of their next chunk. Replace the pipe with an empty one after a failed output. The old pipe is closed first, so that this also works at the open file limit. A new pipe gets the larger size only if the 2nd pipe got it at the start, so that the 2nd is never smaller than the 1st. If no new pipe can be made, tee fails with the error.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

abendrothj · 2026-10-06T23:24:04Z

thanks, that's the coverage build writing its profile under the same size limit. both tests now check exit code 1 and use stderr_contains, like the dd/cp size-limit tests. with -Cinstrument-coverage on Linux they failed before and pass now; extra stderr lines no longer fail them, a panic still does.

oech3 · 2026-10-07T02:43:08Z

+                    drop($pipe_read);
+                    drop($pipe_write);
+                    // same size as the 2nd pipe got, so the 2nd is never smaller than the 1st
+                    match if $sized { pipe::<true>() } else { io::pipe() } {


Same with pipe::<$sized>()?

Oops, it is const generics... this PR is OK as now.

sylvestre · 2026-10-07T06:22:23Z

Thanks for your PR

Copilot AI balanced review requested due to automatic review settings October 6, 2026 07:28

Copilot AI reviewed Oct 6, 2026

abendrothj mentioned this pull request Oct 6, 2026

uucore: drop the pipe after a failed splice write #15086

Open

oech3 reviewed Oct 6, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 6, 2026 08:10

Copilot AI reviewed Oct 6, 2026

Copilot AI balanced review requested due to automatic review settings October 6, 2026 08:18

Copilot AI reviewed Oct 6, 2026

sylvestre reviewed Oct 6, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 6, 2026 08:45

Copilot AI reviewed Oct 6, 2026

Copilot AI balanced review requested due to automatic review settings October 6, 2026 09:08

Copilot AI reviewed Oct 6, 2026

oech3 reviewed Oct 6, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 6, 2026 09:32

abendrothj force-pushed the tee-replace-failed-pipe branch from fa83021 to 95228ac Compare October 6, 2026 09:32

Copilot AI reviewed Oct 6, 2026

Copilot AI balanced review requested due to automatic review settings October 6, 2026 23:23

abendrothj force-pushed the tee-replace-failed-pipe branch from 95228ac to b2c51e5 Compare October 6, 2026 23:23

Copilot AI reviewed Oct 6, 2026

oech3 reviewed Oct 7, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

sylvestre merged commit 2c545f6 into uutils:main Oct 7, 2026
99 checks passed

This was referenced Oct 7, 2026

tee: cleanup pipe by read when splice failed instead of dropping #15149

Closed

tee: remove size variable from macro #15152

Open

Conversation

abendrothj commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oech3 Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oech3 Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as low quality.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

abendrothj commented Oct 6, 2026 •

edited

Loading

oech3 Oct 6, 2026 •

edited

Loading

oech3 Oct 6, 2026 •

edited

Loading