less is suspended/backgrounded when using two pipes with grep

[malthejorgensen]

I have a minor issue where less gets suspended/backgrounded unexpectedly when using two pipes and the last one is a grep (with some caveats, see below). It's easy to work around, so not a big priority but I wanted to raise it if other people stumble upon it.

How to reproduce the issue

# Make a file called "minimal_example.txt" that contains 50 empty lines
# (it has to be more lines than can fit in your terminal vertically)
> string repeat -n 50 \n > minimal_example.txt
> cat minimal_example.txt | grep '.*' | less
:fish: Job 1, 'cat minimal_example.txt | grep…' has stopped
# less is suspended/backgrounded immediately as the command is run

I can reliably reproduce the issue both using fish --no-config and sh -c 'env HOME=$(mktemp -d) fish'.

Interestingly, replacing cat minimal_example.txt with string repeat -n 50 \n or replacing grep '.*' with cat "fixes" the issue. Also the pipeline must have two pipes, e.g. grep '.*' minimal_example.txt | less works perfectly fine.
Furthermore, it seems related to terminal height. If I resize the terminal window to be tall enough for all the lines to fit the window the issue also does not occur.

The issue happens both when fish is run in Terminal.app and iTerm2. It does not occur in bash and zsh (not that fish should emulate any of those -- but this feels like bug).

In this asciinema video of the issue, the issue only happens on the second invocation, so it might be some kind race-condition. I currently hit the bug 95% of the time I run the test command, but it's been a bit lower previously.

fish version

> fish --version
fish, version 3.3.1
> echo $version
3.3.1
> echo $TERM
xterm-256color
> uname -a
Darwin Malthes-MBP.lan 21.3.0 Darwin Kernel Version 21.3.0: Wed Jan  5 21:37:58 PST 2022; root:xnu-8019.80.24~20/RELEASE_ARM64_T6000 arm64

I'm on a Macbook Pro (Apple M1 Pro) with macOS 12.2.

[ridiculousfish]

Great find and amazing repro steps! I am sure it was tedious to reduce this, thank you for taking the time.

This is definitely a bug in fish. I am pretty sure the sequence is:

1. fish launches cat and less, and begins executing the grep function, all in parallel. These all live in a new pgroup whose leader is cat.
2. fish knows it has to transfer ownership of the tty to the pgroup. This is an essential race; here the race winner is the /usr/bin/grep command within the grep function. So grep has transferred ownership!
3. grep finishes and so fish reclaims the tty. This is the bug: fish should NOT reclaim the tty until the outermost job is finished.
4. less tries to manipulate the tty or output something and so it gets SIGTTOU, stopping it.

The right fix here is to only call tcsetpgrp at "top level," when running an outermost job - never when running a nested job like a function. Unfortunately a proper fix will have to wait to post 3.4.0, but a workaround is to replace the fish function with a real command; for example this:

 cat minimal_example.txt | command grep '.*' | less

won't trigger it because there's no fish functions running.