Starting fish background job in an event/variable handler borks commandline for duration of job

[IlanCosman]

fish, version 3.2.1-5-gd79f86407
Tested on MacOS and Arch

not-highlighting.mp4

function on_foo --on-event foo
    fish --command "sleep 3" &
end

Syntax highlighting will be disabled for the full duration of the background job, i.e if you replace sleep 3 with sleep 10 it'll take 10 seconds to come back instead.

This also happens with --on-variable, and I presume all other handlers though I haven't tested them.

Interestingly this bug doesn't appear when running other commands such as sleep or kill, it only seems to work with fish.

Not sure if this is a regression.

[IlanCosman]

@faho Is there anything I can do right now with fish script to remove this problem? Fish 3.3 might be a while and this is an annoying bug in my project 😄

[faho]

@IlanCosman I'll leave you in the capable hands of the others, I'm taking a break.

[IlanCosman]

@ridiculousfish 😅 Sorry to bother, but would you happen to know a fancy Fish trick to get around this?

[krobelus]

@IlanCosman on Linux you can use setsid to launch the background fish in a new process group.
Since it doesn't own the terminal 013a563 does the job.

function on_foo --on-event foo
    setsid fish --command "sleep 3" &
end

I'm not sure about which portable solution is best. Apparently nohup works as well instead of setsid.

[IlanCosman]

Huh, it seems simply setting status job-control full fixes this. Sick!

[faho]

Huh, it seems simply setting status job-control full fixes this. Sick!

@IlanCosman That is emphatically not a knob you want to touch (to be honest it should not exist to begin with, but we're working on that).

Instead simply set the background process's stdin to /dev/null:

    fish --command "sleep 3" </dev/null &

Since the issue is that fish only checks if the group controlling the stdin terminal is its group, just make it so that stdin isn't a terminal.

[IlanCosman]

Thanks a ton @faho! Yah, setting the job-control was a bit sketchy, I noticed it was setting the user's last_pid outside of Tide but decided that minimal risk was an ok tradeoff.

This is why you can't take a break Fabian, I need you too much ❤️😂

[zanchey]

This reproduces for me in 3.2.1, so I don't think it's a regression.

[faho]

It's a regression from 3.2.0, caused by 013a563

[zanchey]

OK, I'd prefer to keep the regression tag for "things that aren't in a release" as it means I can skip over them for the release notes.