From c429734d8bafb19e8cbea7e01b21ba654f94948a Mon Sep 17 00:00:00 2001 From: Hammad Majid Date: Sat, 5 Sep 2026 13:03:46 +0500 Subject: [PATCH] fix(cmd): force-exit on a second SIGINT and stop skipping signal cleanup signal.NotifyContext's stop func is what calls signal.Stop and restores the default signal disposition, and it was only deferred until after the run had finished. The handler therefore stayed installed for the whole run, so the runtime consumed and discarded every SIGINT after the first: a user whose grg was wedged in a phase that cannot unwind (a blocking write to a stalled consumer, an uncancellable walk) had no way out but SIGKILL. A watcher goroutine now calls stop() as soon as the first signal cancels ctx, restoring the default disposition so a second Ctrl-C terminates the process. The goroutine cannot leak: defer stop() cancels ctx on the normal path too, so <-ctx.Done() always returns. os.Exit runs no deferred functions, so exiting from inside main skipped that cleanup on every non-zero exit - and exit code 1 (no match found) is the common case, leaving NotifyContext's relay goroutine and its signal registration behind. The body moves to realMain, which returns an exit code that main hands to os.Exit, so every deferred cleanup runs first. Also deletes the statically dead executor dispatch. searchPipeline is a concrete *search.Pipeline whose ExecuteContext returns channels, so the chanExecutor assertion always succeeded; the sliceExecutor branch and the bare Execute fallback were unreachable, and no single type can satisfy both interfaces because the method names collide with different signatures. Worse, the dead fallback called Execute, which hardcodes context.Background(), so it modelled an uncancellable path and invited a future fix to the wrong branch. runContext now calls ExecuteContext directly and keeps the drain-then-read- error sequence. Removing the fallbacks also made the generic `if err != nil` epilogue that followed the dispatch provably unreachable, since the error read from errCh already returns, so that block goes too; the live ctx.Err() check after it stays. The new integration test pins grg in a state where signal handling actually has to do the work: stdout is a pipe nobody reads and the workload emits far more than a pipe buffer holds, so the process blocks in write(2) with matches pending, where no context check can rescue it. The first SIGINT is absorbed by the handler and changes nothing; the second must kill the process. It fails against the previous code with "second SIGINT was swallowed" and passes now. The three pre-existing signal tests are documented as weak - their 25-commit, 10-file workload finishes in milliseconds and they all accept exit code 0 - so it is clear where the real assertion lives. Closes #16 --- cmd/grg/main.go | 63 +++++++-------- test/integration/signal_test.go | 137 ++++++++++++++++++++++++++++++++ 2 files changed, 166 insertions(+), 34 deletions(-) diff --git a/cmd/grg/main.go b/cmd/grg/main.go index ac627d2..f789af0 100644 --- a/cmd/grg/main.go +++ b/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} } diff --git a/test/integration/signal_test.go b/test/integration/signal_test.go index 0a27d0b..3bd9f27 100644 --- a/test/integration/signal_test.go +++ b/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) + } +}