Skip to content

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

Merged
hammadmajid merged 1 commit into
fix/15-aggregator-drainfrom
fix/16-signal-force-exit
Sep 5, 2026
Merged

hammadmajid merged 1 commit into
fix/15-aggregator-drainfrom
fix/16-signal-force-exit

Conversation

@hammadmajid

@hammadmajid hammadmajid commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Description

A user who hit Ctrl-C during a long operation saw nothing happen, hit it again, and still saw nothing happen. Only kill -9 worked.

Closes #16

Type of Change

  • Bug fix (non-breaking change fixing an issue)
  • New feature (non-breaking change adding functionality)
  • Performance improvement
  • Refactoring or code cleanup
  • Documentation update
  • CI/CD or build workflow change

Key Changes

A second SIGINT now force-exits. signal.NotifyContext's stop func is what calls signal.Stop and restores default signal disposition, but it was only deferred — so the handler stayed installed for the entire run and the Go runtime silently swallowed every subsequent signal. A watcher goroutine now releases the handler as soon as the first signal cancels the context:

ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
defer stop()
go func() { <-ctx.Done(); stop() }()

That goroutine cannot leak: defer stop() always cancels ctx on the normal path. The comment says so.

os.Exit no longer skips cleanup. os.Exit runs no deferred functions, so on every non-zero exit — and noMatchError is exit 1, the common case — signal.Stop never ran and NotifyContext's relay goroutine was never released. Split into func main() { os.Exit(realMain()) } with all deferred cleanup inside realMain.

Deleted the statically dead executor dispatch. searchPipeline is a concrete *search.Pipeline whose ExecuteContext returns channels, so the chanExecutor assertion always succeeded; sliceExecutor and the bare-Execute fallback were unreachable, and a single type can never satisfy both interfaces because the method names collide with different signatures. Worse, the dead fallback called Execute, which hardcodes context.Background() — it modelled an uncancellable path and invited a future fix to the wrong branch. The generic error epilogue below it became provably unreachable once the fallbacks were gone and is deleted too; the live ctx.Err() check after it is untouched.

Verification & Testing

  • Ran go test -v -count=1 ./...
  • Ran go test -race -shuffle=on -count=1 ./...
  • Ran go vet ./...
  • Added or updated unit/integration tests
  • Tested manually against sample Git repository histories
gofmt -l cmd/grg test/integration                          empty
go build ./... && go vet ./...                             clean
go test -race -count=1 ./cmd/... ./test/integration/...    ok 1.29s / 7.37s

New test TestSignal_SecondSIGINT_ForceExits. The child's stdout is an os.Pipe the parent never reads, so with a ~260 KB match workload against a 64 KiB pipe buffer grg provably blocks in write(2) with matches pending — a state no ctx check can unwind, which is what makes the second signal the only escape. The test asserts the premise (still alive after 1 s), sends SIGINT, waits 250 ms, sends a second, then requires death within 2 s with either WaitStatus.Signaled() == SIGINT or a non-zero exit code. Exit 0 is a hard failure.

Discrimination proof: against a copy of the tree with cmd/grg/main.go restored from HEAD, it fails after 4.77 s with second SIGINT was swallowed: grg was still alive 2s later and only SIGKILL ended it. Passes on the fixed tree in 2.10 s.

The three pre-existing signal tests all accept exit code 0 as valid against a 25-commit × 10-file repo that finishes in milliseconds, so they only ever proved non-wedging. A doc comment on setupWorkloadRepo now records that.

Exit-code smoke on a real repository: match=0, no-match=1 (silent), unknown flag=2 (message on stderr), --version=0, quiet+match=0, no args=2.

Note

Stacked on #23.

Checklist

  • gofmt clean
  • Every new test mutation-checked: reverting the fix makes it fail
  • This layer builds and passes the full race suite on its own, not just at the top of the stack

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
@hammadmajid
hammadmajid force-pushed the fix/16-signal-force-exit branch from dc1f407 to c429734 Compare September 5, 2026 14:16
@hammadmajid
hammadmajid marked this pull request as ready for review September 5, 2026 14:19
@hammadmajid
hammadmajid merged commit 22a2c9f into main Sep 5, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cmd/grg: second Ctrl-C is swallowed, os.Exit skips signal cleanup, dead executor dispatch

1 participant