Check for tty with tty? instead of comparing pgid

[timreinke]

Hi,

I think there's a problem with how the SIGINT forwarding works. Checking Process.getpgid(child_pid) != Process.pid can be wrong in both directions:

• if executed through a task runner like rake, the check will pass and a second interrupt will send
  • I put together a small repro here
• worse (for me at least 😄) - if the tests are executed outside a tty, it's possible for the check to fail and the signal swallowed.

In my case, I want to perform cleanup (release selenium grid handles) when a github workflow is cancelled. If I define my step as:

# use exec so signals are forwarded
run: exec bundle exec parallel_cucumber ...

Then the parallel_test is group leader and Process.getpgid(child_pid) == Process.pid, so the signal is swallowed.

One workaround is:

run: |
   handle_signal() {
     kill -INT $child_pid
   }
   trap handle_signal SIGINT
   bundle exec parallel_cucumber .. &
   child_pid=$?

Now the runner shell remains the group leader and the check passes -- it's kind of a bother though :). There's a lot of discussion about how github could do better, but here we are.

This changes the check to use tty? - which itself can be wrong (redirects in shell or command runners). This feels like the sort of thing where someone will always be unhappy, unfortunately. Could add an envvar as an escape hatch?

Checklist

• [X] Feature branch is up-to-date with master (if not - rebase it).
• [X] Added tests.
  • Reusing existing SIGINT test for detached processes
  • Maybe the PTY repro could be adopted to test the tty case? Not sure if you'd want something like that in here
• [ ] Added an entry to the Changelog if the new code introduces user-observable changes.
  • It might?
• [X] ~Update Readme.md when cli options are changed~

[grosser]

don't really want to mess with that logic since it has a history of having weird edge-cases and it was pretty stable since the last change, so would prefer an env-var escape hatch instead

[timreinke]

Yeah, I don't think I'd want to step into that one either :)

I switched to using an envvar, and can update the PR title/desc + add a small note in the README if you think this is worth getting in.

On my end, I've ended up finding it's better to install an interrupt handler in the top-level shell process, and send SIGINT directly to the test runners. Trying to get everything between to propagate and behave nicely turned out a bit of a fool's errand. So I'm just as happy to close this.

[grosser]

thx for digging into this mess, let's close it until someone else comes around that needs it then :)