chore: Preventing flakes from TestNewSignalsForwarderMultipleUnix (#6750)

Author: yhakbar

docs/src/data/changelog/v1.1.4/command-shutdown-signal.mdx

@@ -0,0 +1,10 @@
+---
+version: "v1.1.4"
+category: "bug-fixes"
+---
+
+#### Fixed the signal sent to a running command during shutdown
+
+On Windows, when a failure rather than Ctrl+C cancelled a run, Terragrunt crashed with a nil pointer panic instead of stopping the command it had started. It now terminates the command, which is the closest thing Windows offers to an interrupt.
+
+On every platform, when a command exited on its own during the grace period after Ctrl+C, Terragrunt could still send it the signal and then log a forwarding error against a process that was already gone.

internal/os/exec/cmd.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"sync"
 	"sync/atomic"
 	"time"
 
@@ -33,6 +34,7 @@ var ErrPTYRequiresOSBackend = errors.New("PTY allocation requires an OS-backed v
 type Cmd struct {
 	vc                         vexec.Cmd
 	interruptSignal            os.Signal
+	notifier                   signal.NotifierFunc
 	filename                   string
 	dir                        string
 	forwardSignalDelay         time.Duration
@@ -56,6 +58,7 @@ func Command(ctx context.Context, v *venv.Venv, name string, args ...string) *Cm
 		vc:              vc,
 		filename:        filepath.Base(name),
 		interruptSignal: signal.InterruptSignal,
+		notifier:        signal.NotifierWithContext,
 	}
 
 	cmd.SetStdin(v.Stdin)
@@ -74,10 +77,6 @@ func Command(ctx context.Context, v *venv.Venv, name string, args ...string) *Cm
 			sig = cmd.interruptSignal
 		}
 
-		if sig == nil {
-			sig = os.Kill
-		}
-
 		if err := vc.Signal(sig); err != nil && !errors.Is(err, vexec.ErrProcessNotStarted) {
 			return err
 		}
@@ -188,12 +187,13 @@ func (cmd *Cmd) RegisterGracefullyShutdown(ctx context.Context, l log.Logger) fu
 // ForwardSignal forwards a given `sig` with a delay if cmd.forwardSignalDelay is greater than 0,
 // and if the same signal is received again, it is forwarded immediately.
 func (cmd *Cmd) ForwardSignal(ctx context.Context, l log.Logger, sig os.Signal) {
-	ctxDelay, cancelDelay := context.WithCancel(ctx)
-	defer cancelDelay()
+	escalate := make(chan struct{})
+	stopWaiting := sync.OnceFunc(func() { close(escalate) })
+
+	notifyCtx, stopNotifying := context.WithCancel(ctx)
+	defer stopNotifying()
 
-	signal.NotifierWithContext(ctx, func(_ os.Signal) {
-		cancelDelay()
-	}, sig)
+	cmd.notifier(notifyCtx, func(os.Signal) { stopWaiting() }, sig)
 
 	if cmd.forwardSignalDelay > 0 {
 		l.Debugf("%s signal will be forwarded to %s with delay %s",
@@ -203,11 +203,14 @@ func (cmd *Cmd) ForwardSignal(ctx context.Context, l log.Logger, sig os.Signal)
 		)
 	}
 
+	// escalate is a plain channel rather than a context derived from ctx. A derived one
+	// would leave two ready cases here when the caller cancels, and select would forward
+	// the signal about half the time.
 	select {
 	case <-ctx.Done():
 		return
 	case <-time.After(cmd.forwardSignalDelay):
-	case <-ctxDelay.Done():
+	case <-escalate:
 	}
 
 	cmd.SendSignal(l, sig)

internal/os/exec/cmd_unix_test.go

@@ -8,7 +8,6 @@ import (
 	"os"
 	"path/filepath"
 	"strconv"
-	"strings"
 	"testing"
 	"time"
 
@@ -40,23 +39,6 @@ func requireTrapReady(t *testing.T, readyPath string) {
 	}, 10*time.Second, 10*time.Millisecond, "child never wrote the trap-ready marker")
 }
 
-// requireInterruptCount blocks until the subprocess records that its INT trap has run
-// want times.
-func requireInterruptCount(t *testing.T, readyPath string, want int) {
-	t.Helper()
-
-	require.Eventually(t, func() bool {
-		content, err := os.ReadFile(readyPath)
-		if err != nil {
-			return false
-		}
-
-		got, err := strconv.Atoi(strings.TrimSpace(string(content)))
-
-		return err == nil && got == want
-	}, 10*time.Second, 10*time.Millisecond, "child never acknowledged interrupt %d", want)
-}
-
 func TestExitCodeUnix(t *testing.T) {
 	t.Parallel()
 
@@ -106,7 +88,7 @@ func TestNewSignalsForwarderWaitUnix(t *testing.T) {
 		readyPath,
 	)
 
-	runChannel := make(chan error)
+	runChannel := make(chan error, 1)
 
 	go func() {
 		runChannel <- cmd.Run(l)
@@ -134,46 +116,6 @@ func TestNewSignalsForwarderWaitUnix(t *testing.T) {
 	)
 }
 
-// There isn't a proper way to catch interrupts in Windows batch scripts, so this test exists only for Unix.
-func TestNewSignalsForwarderMultipleUnix(t *testing.T) {
-	t.Parallel()
-
-	expectedInterrupts := 4
-
-	l := logger.CreateLogger()
-
-	readyPath := filepath.Join(t.TempDir(), "sigint-ready")
-
-	cmd := exec.Command(
-		t.Context(), venvtest.New().WithExec(vexec.NewOSExec()),
-		"testdata/test_sigint_multiple.sh", strconv.Itoa(expectedInterrupts), readyPath,
-	)
-
-	runChannel := make(chan error)
-
-	go func() {
-		runChannel <- cmd.Run(l)
-	}()
-
-	requireInterruptCount(t, readyPath, 0)
-
-	// Bash defers its trap until the running `sleep` returns, so two signals delivered within
-	// one sleep window collapse into a single handler run. Waiting for the child to
-	// acknowledge each interrupt before sending the next keeps the count exact.
-	for interrupts := 1; interrupts <= expectedInterrupts; interrupts++ {
-		cmd.SendSignal(l, os.Interrupt)
-
-		requireInterruptCount(t, readyPath, interrupts)
-	}
-
-	err := <-runChannel
-	require.Error(t, err)
-
-	retCode, err := util.GetExitCode(err)
-	require.NoError(t, err)
-	assert.Equal(t, expectedInterrupts, retCode, "Subprocess didn't receive multiple signals")
-}
-
 // TestGracefulShutdownOnContextCancelUnix verifies that when the context is cancelled
 // without a signal cause, the Cancel callback sends SIGINT (not SIGKILL) to allow
 // processes like Terraform to gracefully shutdown their child processes.
@@ -197,7 +139,7 @@ func TestGracefulShutdownOnContextCancelUnix(t *testing.T) {
 
 	cmd.Configure(exec.WithGracefulShutdownDelay(5 * time.Second))
 
-	runChannel := make(chan error)
+	runChannel := make(chan error, 1)
 
 	go func() {
 		runChannel <- cmd.Run(l)

internal/os/exec/cmd_windows_test.go

@@ -81,7 +81,7 @@ func TestWindowsNewSignalsForwarderWait(t *testing.T) {
 		strconv.Itoa(expectedWait),
 	)
 
-	runChannel := make(chan error)
+	runChannel := make(chan error, 1)
 
 	go func() {
 		runChannel <- cmd.Run(l)

internal/os/exec/opts.go

@@ -3,6 +3,7 @@ package exec
 import (
 	"time"
 
+	"github.com/gruntwork-io/terragrunt/internal/os/signal"
 	"github.com/gruntwork-io/terragrunt/internal/venv"
 )
 
@@ -30,6 +31,14 @@ func WithForwardSignalDelay(delay time.Duration) Option {
 	}
 }
 
+// WithSignalNotifier sets the source [Cmd.ForwardSignal] watches for a repeat of the
+// signal it is holding. The default watches the OS.
+func WithSignalNotifier(notifier signal.NotifierFunc) Option {
+	return func(cmd *Cmd) {
+		cmd.notifier = notifier
+	}
+}
+
 // WithGracefulShutdownDelay sets the time to wait for a process to exit gracefully
 // after sending an interrupt signal before escalating to SIGKILL.
 // This allows processes like Terraform to clean up child processes (e.g., provider plugins).