fix(cmd): force-exit on a second SIGINT and stop skipping signal cleanup

cmd/grg/main.go

@@ -25,18 +25,36 @@ import (
 )
 
 func main() {
-	ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
-	defer cancel()
+	os.Exit(realMain())
+}
+
+// realMain owns the whole run and returns the process exit code. main does
+// nothing but hand that code to os.Exit, which runs no deferred functions:
+// exiting from inside this function would skip signal cleanup on every
+// non-zero exit, and exit code 1 (no match found) is the common case.
+func realMain() int {
+	ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
+	defer stop()
+
+	// Release the signal handler as soon as the first signal cancels ctx. That
+	// restores the default signal disposition, so a second Ctrl-C terminates
+	// the process immediately instead of being swallowed by a handler that
+	// would otherwise stay installed for the whole run. This goroutine cannot
+	// leak: defer stop() cancels ctx on the normal path too, so <-ctx.Done()
+	// always returns.
+	go func() {
+		<-ctx.Done()
+		stop()
+	}()
 
 	if err := runContext(ctx, os.Args[1:], os.Stdout, os.Stderr); err != nil {
 		ec := exitCodeForError(err)
-		if ec != 1 {
-			if !isQuietError(err) {
-				fmt.Fprintf(os.Stderr, "grg: %v\n", err)
-			}
+		if ec != 1 && !isQuietError(err) {
+			fmt.Fprintf(os.Stderr, "grg: %v\n", err)
 		}
-		os.Exit(ec)
+		return ec
 	}
+	return 0
 }
 
 func run(args []string) error {
@@ -129,35 +147,12 @@ func runContext(ctx context.Context, args []string, stdout, stderr io.Writer) er
 
 	searchPipeline := search.NewPipeline(reader, matcher, cfg)
 
-	type chanExecutor interface {
-		ExecuteContext(ctx context.Context, occurrences []model.BlobOccurrence) (<-chan *search.BlobResult, <-chan error)
-	}
-	type sliceExecutor interface {
-		ExecuteContext(ctx context.Context, occurrences []model.BlobOccurrence) ([]*search.BlobResult, error)
-	}
-
 	var results []*search.BlobResult
-	if che, ok := any(searchPipeline).(chanExecutor); ok {
-		resultsCh, errCh := che.ExecuteContext(ctx, occurrences)
-		for res := range resultsCh {
-			results = append(results, res)
-		}
-		if err = <-errCh; err != nil {
-			if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) || ctx.Err() != nil {
-				return cancelError{err: err, quiet: cfg.Quiet}
-			}
-			return err
-		}
-	} else if se, ok := any(searchPipeline).(sliceExecutor); ok {
-		results, err = se.ExecuteContext(ctx, occurrences)
-	} else {
-		if ctxErr := ctx.Err(); ctxErr != nil {
-			return cancelError{err: ctxErr, quiet: cfg.Quiet}
-		}
-		results, err = searchPipeline.Execute(occurrences)
+	resultsCh, errCh := searchPipeline.ExecuteContext(ctx, occurrences)
+	for res := range resultsCh {
+		results = append(results, res)
 	}
-
-	if err != nil {
+	if err = <-errCh; err != nil {
 		if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) || ctx.Err() != nil {
 			return cancelError{err: err, quiet: cfg.Quiet}
 		}

test/integration/signal_test.go

@@ -2,13 +2,23 @@ package integration
 
 import (
 	"fmt"
+	"os"
 	"os/exec"
+	"strings"
 	"syscall"
 	"testing"
 	"time"
 )
 
 // setupWorkloadRepo creates a repository with enough commit history to test process interruption.
+//
+// Note that this workload is weak on purpose-preserving history: 25 commits of
+// 10 tiny files are searched in a few milliseconds, so the three tests below
+// almost certainly finish before their signal is delivered, and all of them
+// accept exit code 0 as valid. They only prove that a signal does not wedge
+// the process. TestSignal_SecondSIGINT_ForceExits carries the real assertion:
+// it pins the process in a state where the signal has to do the work, and
+// accepts nothing but a non-zero exit.
 func setupWorkloadRepo(t *testing.T) *TestRepo {
 	t.Helper()
 	repo := NewTestRepo(t)
@@ -158,3 +168,130 @@ func TestSignal_ImmediateInterrupt(t *testing.T) {
 		// Succeeded in exiting within timeout
 	}
 }
+
+// setupOutputHeavyRepo creates a repository whose search emits far more output
+// than a pipe buffer holds (64 KiB on Linux), so a grg process whose stdout is
+// never drained is guaranteed to fill the pipe and block in write(2) with
+// matches still pending.
+func setupOutputHeavyRepo(t *testing.T) *TestRepo {
+	t.Helper()
+	repo := NewTestRepo(t)
+
+	for i := 1; i <= 30; i++ {
+		files := make(map[string]string)
+		for j := 1; j <= 12; j++ {
+			var b strings.Builder
+			for k := 1; k <= 8; k++ {
+				fmt.Fprintf(&b, "commit %d file %d line %d TOKEN_WORKLOAD_MARKER padding padding padding padding\n", i, j, k)
+			}
+			files[fmt.Sprintf("pkg%d/file%d.txt", j, j)] = b.String()
+		}
+		repo.Commit(fmt.Sprintf("Commit batch %d", i), files)
+	}
+
+	return repo
+}
+
+// TestSignal_SecondSIGINT_ForceExits verifies that a second SIGINT terminates
+// grg even when the run cannot unwind on its own.
+//
+// The process is pinned in an uninterruptible write: its stdout is a pipe that
+// nobody reads, so once the pipe buffer is full every remaining match line
+// blocks in write(2). No context check can rescue that goroutine, which is
+// exactly the situation a user hits when grg is piped into a stalled consumer.
+// The first SIGINT is therefore necessarily absorbed by the installed handler
+// and changes nothing observable. Only releasing that handler once cancellation
+// has fired - restoring the default disposition - lets the second SIGINT end
+// the process. Without that release, package signal keeps relaying and then
+// silently dropping every later signal and the process survives until SIGKILL.
+func TestSignal_SecondSIGINT_ForceExits(t *testing.T) {
+	repo := setupOutputHeavyRepo(t)
+	bin := getGRGBinary(t)
+
+	// The parent closes its copy of the write end so the child owns the only
+	// writer, and holds the read end open without ever reading it so the child
+	// blocks instead of receiving EPIPE.
+	pr, pw, err := os.Pipe()
+	if err != nil {
+		t.Fatalf("failed creating pipe: %v", err)
+	}
+	defer pr.Close()
+
+	cmd := exec.Command(bin, "--color=never", "TOKEN_WORKLOAD_MARKER")
+	cmd.Dir = repo.Dir
+	cmd.Stdout = pw
+
+	if err := cmd.Start(); err != nil {
+		_ = pw.Close()
+		t.Fatalf("failed starting grg subprocess: %v", err)
+	}
+	_ = pw.Close()
+
+	done := make(chan error, 1)
+	go func() {
+		done <- cmd.Wait()
+	}()
+
+	// Premise check: the search itself is quick, but the output cannot drain, so
+	// the process must still be alive and stuck. Had it exited, the workload
+	// would fit in the pipe buffer and the rest of this test would prove nothing.
+	select {
+	case err := <-done:
+		t.Fatalf("grg exited (%v) before filling the stdout pipe; workload is too small for this test to mean anything", err)
+	case <-time.After(1 * time.Second):
+	}
+
+	if err := cmd.Process.Signal(syscall.SIGINT); err != nil {
+		t.Fatalf("failed sending first SIGINT: %v", err)
+	}
+
+	// Give cancellation time to propagate and release the signal handler.
+	select {
+	case err := <-done:
+		// Not expected while blocked in write, but a prompt non-zero exit is a
+		// perfectly good outcome for an interrupted run.
+		assertInterruptedExit(t, err)
+		return
+	case <-time.After(250 * time.Millisecond):
+	}
+
+	if err := cmd.Process.Signal(syscall.SIGINT); err != nil {
+		t.Fatalf("failed sending second SIGINT: %v", err)
+	}
+
+	select {
+	case <-time.After(2 * time.Second):
+		_ = cmd.Process.Kill()
+		t.Fatal("second SIGINT was swallowed: grg was still alive 2s later and only SIGKILL ended it")
+	case err := <-done:
+		assertInterruptedExit(t, err)
+	}
+}
+
+// assertInterruptedExit requires that grg died because it was interrupted:
+// killed outright by SIGINT once the default disposition was restored, or
+// exited non-zero after handling the cancellation itself. Exit code 0 would
+// claim the search completed successfully, which it did not.
+func assertInterruptedExit(t *testing.T, err error) {
+	t.Helper()
+
+	if err == nil {
+		t.Fatal("grg exited 0 after being interrupted; an interrupted run must not report success")
+	}
+
+	exitErr, ok := err.(*exec.ExitError)
+	if !ok {
+		t.Fatalf("waiting on grg failed: %v", err)
+	}
+
+	if ws, ok := exitErr.Sys().(syscall.WaitStatus); ok && ws.Signaled() {
+		if ws.Signal() != syscall.SIGINT {
+			t.Fatalf("expected termination by SIGINT, got %v", ws.Signal())
+		}
+		return
+	}
+
+	if code := exitErr.ExitCode(); code == 0 {
+		t.Fatalf("expected a non-zero exit code after interruption, got %d", code)
+	}
+}