cc @maggie-lou @bduffany

👍 I'll take a look tomorrow

It looks like this issue might only happen when running sh_binary targets. When running with bb the process tree looks like this:

        - 154116 bash (pgid: 154116, sid: 154116, tty: 34833) bash 
          - 580762 sh (pgid: 580762, sid: 154116, tty: 34833) /bin/sh /home/b/.cache/bazel/_bazel_b/edd33241c517dda06a232361ced2493d/execroot/_main/bazel-out/k8-fastbuild/bin/sleeper 
            - 581175 sleep (pgid: 580762, sid: 154116, tty: 34833) sleep 10 

But when running with bazel the process tree looks like this:

        - 154116 bash (pgid: 154116, sid: 154116, tty: 34833) bash 
          - 581554 bazel (pgid: 581554, sid: 154116, tty: 34833) bazel run :sleeper 
            - 581559 bash (pgid: 581554, sid: 154116, tty: 34833) /bin/bash -c /home/b/.cache/bazel/_bazel_b/edd33241c517dda06a232361ced2493d/execroot/_main/bazel-out/k8-fastbuild/bin/sleeper 
              - 581893 sleep (pgid: 581554, sid: 154116, tty: 34833) sleep 10 

When running under bazel, the sh_binary runs under bash, but for some reason, when running under bb, it runs under sh.

In the past, I've run into issues with signal handling in sh - it doesn't handle signals as nicely as bash does.

I wonder if there's some env variable that determines which shell gets used, which we're dropping, or something like that. Or maybe this is a bug with Bazel's --script_path flag.

eh, nevermind - probably not related to sh vs bash. When using --script_path with vanilla bazel, it runs under sh instead of bash like it would under bazel run, but Ctrl+C still works.

I didn't include this in the issue description because I didn't want to bias towards a particular solution, but I did narrow down the cause of this behavior.

First, note that upon exec, the signal disposition of the exec'ed process will depend on that of the process before the exec: (Excerpt from POSIX.1-2024 standard)

Signals set to the default action (SIG_DFL) in the calling process image shall be set to the default action in the new process image. Except for SIGCHLD, signals set to be ignored (SIG_IGN) by the calling process image shall be set to be ignored by the new process image. Signals set to be caught by the calling process image shall be set to the default action in the new process image (see <signal.h>).

So: If the process before exec has set the signal to be ignored, it will be ignored in the process after exec as well. If the signal is set to either default, or has a non-default handler, then it will be set to default in the process after exec.

Case 1: Bazel is directly invoked. In this case, Bazel installs some signal handlers:

https://github.com/bazelbuild/bazel/blob/master/src/main/cpp/blaze_util_posix.cc#L209-L212

These are used to forward cancelations to the server process. However, how they're used doesn't actually matter. What matters is that they are set at all. This means that by the time the client process receives the ExecRequest and execs here:

https://github.com/bazelbuild/bazel/blob/5b4973534f6adf439ca6d61b7627e0fdc8d9dfe8/src/main/cpp/blaze_util_posix.cc#L328

The signals will be SIG_DFL in the exec'ed process, because the process before exec had a handler set.

Case 2: Bazelisk is invoked, and Bazelisk runs Bazel. In this case, Bazelisk runs this signal.Ignore line:

https://github.com/bazelbuild/bazelisk/blob/1f9a1aca958cdb50b4adb84b15cdda55a600ed31/core/core.go#L702

This ignores those signals in the Bazelisk process. However, it's called after the Bazel process was spawned, so the Bazel process still inherits reasonable (non-ignored) signal handlers. Even if the signal.Ignore ran first, Bazel installed its own signal handler anyway, so it works out fine. It's not a problem that the parent process has signals ignored -- when a user interrupts, the signal is sent to all processes in the process group, so even if a parent process ignores the signal, the child process will still receive it too.

Case 3: BuildBuddy CLI is run. In this case, the BuildBuddy CLI first runs a build, and then it runs the resulting --script_path. Digging into each of those steps:

1. The build step calls directly into Bazelisk logic, when dispatching out to Bazel to do the build. This ends up running signal.Ignore on those three signals, and this applies globally to the entire process. From this point onwards, bb will ignore signals.
2. Later, bb execs into the --script_path script. Because the signals are still set to SIG_IGN, the resulting exec'ed process will also have SIG_IGN for those signals.

Case 4: Bazelisk is invoked and runs BuildBuddy CLI. Basically the same as case 3, except with an extra parent process in the way ignoring signals (but again, that's not a problem because everything in the process group will receive the signal -- it's up to the child to figure out what to do with it though).

Basically the problem is that during the build step, when the BuildBuddy CLI called into Bazelisk as a library, Bazelisk contaminated the BuildBuddy CLI's signal disposition, so that when the BuildBuddy CLI goes then to exec the --script_path script, all the signals have already been set to SIG_IGN and that setting gets inherited through the exec.

There's many different ways this could be handled, but some options:

1. Bazelisk could be patched to not run signal.Ignore -- a safer option could be to set a signal.Notify while running the cmd.Wait() and then running signal.Stop afterwards, which would restore the signal disposition to what it was before.
2. Rather than using Bazelisk as a library, the BuildBuddy CLI could run it in a subprocess. Then the scope of the signal.Ignore is limited.
3. Before exec'ing the --script_path, BuildBuddy could set a dummy signal.Notify on all of SIGINT, SIGTERM, and SIGQUIT. The channel passed to signal.Notify doesn't otherwise even have to be used at all -- the mere fact that signal.Notify now set a handler on these signals now means that the exec'ed process will receive SIG_DFL signal dispositions.

Option 3 has the downside that any steps that run between the bazel run --script_path and the exec, will still be uninterruptible. In particular this means that post-Bazel plugins won't be able to be interrupted. But at least interrupts for the executable actually getting run would go through.

Thanks for the great analysis!

I sent bazelbuild/bazelisk#657 to realize Option 1. We can patch it in even before the upstream PR has been merged.

Thanks for the great analysis!

I sent bazelbuild/bazelisk#657 to realize Option 1. We can patch it in even before the upstream PR has been merged.

From my experiments, signal.Ignore is not actually reset by signal.Reset. signal.Ignore is somewhat permanent and doesn't have an easy "undo". Example:

package main

import (
	"fmt"
	"os/signal"
	"syscall"
)

func main() {
	fmt.Println(signal.Ignored(syscall.SIGINT))
	signal.Ignore(syscall.SIGINT)
	fmt.Println(signal.Ignored(syscall.SIGINT))
	signal.Reset(syscall.SIGINT)
	fmt.Println(signal.Ignored(syscall.SIGINT))
}

Running it:

% go run example.go 
false
true
true

signal.Ignored remains true after the signal.Reset. This is why my option 1 suggestion was to avoid using Ignore at all, not to try to restore after Ignore (which from my experiments, is hard-to-impossible, due to limitations in the Go standard library).

Thank you for sharing your knowledge about signals! Confirmed that the signal.Reset patch doesn't seem to fix the issue, but using signal.Notify/signal.Stop does. @fmeum here's bazelbuild/bazelisk#658 with the patch