Skip to content

fix: cancel the run context on SIGINT/SIGTERM - #3

Merged
funkyshu merged 3 commits into
mainfrom
fix/signal-handling-stash-cleanup
Aug 14, 2026
Merged

funkyshu merged 3 commits into
mainfrom
fix/signal-handling-stash-cleanup

Conversation

@funkyshu

Copy link
Copy Markdown
Member

Summary

  • An external kill of the prenup process (a CI/editor timeout, a git GUI's "cancel" button, etc.) previously terminated the process via the OS default signal action, skipping every deferred cleanup in runRun/runner.Run — including endStash's git stash pop that restores the clean_worktree stash created by beginStash. That left an orphaned stash the user would only discover later as a confusing conflict.
  • Wire SIGINT/SIGTERM to context cancellation via signal.NotifyContext instead. BashExecutor.Run already reacts to context cancellation by sending the in-flight task SIGTERM (falling back to SIGKILL after the configured grace period), so canceling the top-level context now gives runner.Run a chance to return normally and run its deferred stash pop before the process exits. A second SIGINT/SIGTERM still forces immediate termination, per signal.NotifyContext's documented behavior.

Fixes #2.

Test plan

  • go build ./...
  • go test ./...
  • golangci-lint run --max-same-issues 0 ./...
  • New unit tests (TestNewRunContext_CanceledOnSIGTERM, TestNewRunContext_CanceledOnSIGINT) send a real signal to the test process and assert the returned context is canceled.
  • Ran prenup run (self-hosted) locally to validate the changelog entry and full task suite pass.

Made with Cursor

An external kill of the prenup process (a CI/editor timeout, a git GUI's
"cancel" button, etc.) previously terminated the process via the OS default
signal action, skipping every deferred cleanup in runRun/runner.Run -
including endStash's `git stash pop` that restores the clean-worktree
stash created by beginStash. That left an orphaned stash the user would
only discover later as a surprise conflict.

Wire SIGINT/SIGTERM to context cancellation via signal.NotifyContext
instead. BashExecutor.Run already reacts to context cancellation by
sending the in-flight task SIGTERM (falling back to SIGKILL after the
configured grace period), so canceling the top-level context now gives
runner.Run a chance to return normally and run its deferred stash pop
before the process exits. A second SIGINT/SIGTERM still forces immediate
termination, per signal.NotifyContext's documented behavior.

Fixes #2.
@c2fo-cibot c2fo-cibot Bot added the size/M Denotes a PR that changes 30-99 lines label Aug 12, 2026
Follow-up to a381320 (SIGINT/SIGTERM -> context cancellation). runner.Run's
task loop didn't check ctx between tasks, so once a signal canceled the
context, every remaining selected task was still attempted and failed
almost instantly with a generic "context canceled" error - turning one
interruption into a wall of misleading per-task failures, though the
deferred stash pop still ran correctly either way.

Run now checks ctx.Err() before starting each task that hasn't begun yet
and, once it's non-nil, reports the rest as a single "run interrupted"
skip instead of attempting (and failing) each one. A new Result.Interrupted
field lets callers distinguish "cut short by cancellation" from "every
attempted task genuinely failed", and the run still exits non-zero even
if no task technically failed.

Also adds an end-to-end regression test that pins the actual bug fixed by
a381320: a real git repo with an unstaged change, clean_worktree enabled,
and a fake Executor that cancels the context mid-run. Asserts the
not-yet-started task is skipped (never invoked) and, most importantly,
that the autostash is popped and the unstaged edit is restored once Run
returns - the behavior the previous unit tests (which only exercised
newRunContext's signal-to-cancellation plumbing) didn't cover.
@c2fo-cibot c2fo-cibot Bot added size/L Denotes a PR that changes 100-499 lines and removed size/M Denotes a PR that changes 30-99 lines labels Aug 13, 2026
@funkyshu

Copy link
Copy Markdown
Member Author

Addressed the code-review findings from the earlier round:

  • runner.Run now checks ctx.Err() between tasks and stops attempting anything that hasn't started once the context is canceled, instead of letting every remaining task fail near-instantly with a generic context canceled error. Added Result.Interrupted so callers can tell "cut short by cancellation" apart from "every attempted task genuinely failed"; the run still exits non-zero either way.
  • Added an end-to-end regression test (TestContextCancellationSkipsRemainingTasksAndPopsStash) that exercises the actual bug from Autostash safety net isn't signal-safe outside the interactive TUI #2 directly: a real git repo with an unstaged edit, clean_worktree enabled, and a fake Executor that cancels the context mid-run. It asserts the not-yet-started task is never invoked and that the autostash is popped and the unstaged edit is restored once Run returns — the previous unit tests only covered newRunContext's signal-to-cancellation plumbing, not the stash-pop behavior itself.
  • Documented the Windows/SIGTERM caveat inline (synthetic value there; os.Interrupt is the one signal os/signal guarantees everywhere) so it doesn't look like an oversight later.

Left as-is (noted but out of scope for this PR): Ctrl-C during the interactive human/TUI selector is handled by Bubble Tea's raw-mode keystroke capture, not OS signal delivery, so this change mainly benefits non-interactive runs (CI/editor-triggered SIGTERM, or Ctrl-C when no TUI has claimed the terminal) — which is the scenario that actually caused the original orphaned-stash report.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes prenup run handle external SIGINT/SIGTERM gracefully by canceling the run context (instead of letting the OS terminate the process), allowing deferred cleanup in runner.Run—notably the clean_worktree stash pop—to reliably execute and avoid orphaned stashes.

Changes:

  • Wire SIGINT/SIGTERM into a cancelable run context via signal.NotifyContext in the CLI entrypoint.
  • Update runner.Run to stop starting new tasks after cancellation and report remaining tasks as “run interrupted” skips (plus a new Result.Interrupted flag).
  • Add unit tests covering signal-driven context cancellation and stash restoration on cancellation, and document the change in CHANGELOG.md.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
internal/cli/run.go Creates a signal-cancelable context for prenup run via newRunContext().
internal/cli/run_internal_test.go Adds tests asserting newRunContext() is canceled by SIGTERM/SIGINT.
internal/runner/runner.go Adds Result.Interrupted and skips not-yet-started tasks when the run context is canceled.
internal/runner/runner_test.go Adds a regression test ensuring cancellation skips remaining tasks and still pops the autostash.
CHANGELOG.md Documents the SIGINT/SIGTERM cancellation behavior and improved interruption reporting.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/runner/runner.go Resolved
Comment thread internal/cli/run.go Resolved
- internal/runner/runner.go: correct a misleading doc comment. The
  in-flight task when cancellation happens isn't guaranteed to fail —
  it observes ctx via its own exec.CommandContext call and runs to
  whatever outcome that produces, which can be a success if the task
  finishes right as the signal arrives (exactly what the new regression
  test exercises). Only not-yet-started tasks are unconditionally
  skipped by this loop.
- internal/cli/run.go: drop the Windows/SIGTERM tangent from
  newRunContext's doc comment. Per README.md, Windows support is
  explicitly out of scope for prenup, so the caveat about SIGTERM being
  synthetic there was irrelevant noise rather than a real concern for
  this codebase's supported platforms (Linux/macOS).
@funkyshu

Copy link
Copy Markdown
Member Author

Addressed both Copilot review comments (commit 1495640):

  • internal/runner/runner.go: corrected the doc comment — the in-flight task at cancellation time isn't guaranteed to fail; it observes ctx via its own exec.CommandContext call and can still finish successfully if the task completes right as the signal arrives (exactly what the regression test does). Only not-yet-started tasks are unconditionally skipped by the loop.
  • internal/cli/run.go: dropped the Windows/SIGTERM tangent from newRunContext's comment, since the README already states Windows is explicitly out of scope for prenup — that caveat was irrelevant noise here.

All CI checks are green.

@funkyshu
funkyshu merged commit 7c0d059 into main Aug 14, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Autostash safety net isn't signal-safe outside the interactive TUI

2 participants