Fix self-upgrade interrupt handling - #425
Merged
Merged
Conversation
Contributor
Reviewer's GuideRefactors CLI signal handling into a reusable helper that wires OS signal notifications into a cancelable context, ensures signal notifications are stopped after the first handled interrupt or parent cancellation, and adds regression tests plus a changelog entry documenting the fix for Ctrl-C behavior during self-upgrade. Sequence diagram for updated CLI signalContext handlingsequenceDiagram
participant Main
participant getContext
participant signalContext
participant signalNotify as signal.Notify
participant signalStop as signal.Stop
participant Goroutine
participant ParentCtx as parent.Done
participant Ch as chan_os_Signal
Main->>getContext: getContext()
getContext->>signalNotify: signal.Notify(Ch, os.Interrupt, SIGTERM)
getContext->>signalContext: signalContext(context.Background(), Ch, stop)
signalContext->>signalContext: context.WithCancel(parent)
signalContext-->>getContext: ctx
signalContext->>Goroutine: start goroutine
loop wait_for_signal_or_parent_cancel
alt OS sends SIGINT or SIGTERM
OS-->>Ch: os.Signal
Ch-->>Goroutine: os.Signal
Goroutine->>Goroutine: log.Printf(signal received)
Goroutine->>signalContext: cancel()
Goroutine->>signalStop: stop()
else parent context canceled
ParentCtx-->>Goroutine: parent.Done()
Goroutine->>signalContext: cancel()
Goroutine->>signalStop: stop()
end
end
Main-->>ctxUser: ctx.Done() (cancellation propagated)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
kindermax
marked this pull request as ready for review
July 29, 2026 12:30
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="internal/cli/cli_test.go" line_range="97-86" />
<code_context>
+ }
+ })
+
+ t.Run("cancels when parent context is canceled", func(t *testing.T) {
+ parent, cancel := context.WithCancel(context.Background())
+ ctx := signalContext(parent, make(chan os.Signal), func() {})
+
+ cancel()
+
+ select {
+ case <-ctx.Done():
+ case <-time.After(time.Second):
+ t.Fatal("expected signal context to be canceled by parent")
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that signal notification is stopped when the parent context is canceled
This subtest only checks that the child context is canceled when the parent is canceled; it doesn’t verify that the `stop` callback is invoked in this path. Since `signalContext` always defers `stop()`, consider mirroring the first subtest by passing a `stopped` channel and asserting it is closed when the parent is canceled, so the signal notification cleanup is covered for both signal- and parent-driven cancellation.
</issue_to_address>
### Comment 2
<location path="internal/cli/cli_test.go" line_range="74-72" />
<code_context>
}
}
+func TestSignalContext(t *testing.T) {
+ t.Run("cancels and stops signal notification after first signal", func(t *testing.T) {
+ signals := make(chan os.Signal, 1)
+ stopped := make(chan struct{})
+ ctx := signalContext(context.Background(), signals, func() {
+ close(stopped)
+ })
+
+ signals <- os.Interrupt
+
+ select {
+ case <-ctx.Done():
+ case <-time.After(time.Second):
+ t.Fatal("expected signal context to be canceled")
+ }
+
+ select {
+ case <-stopped:
+ case <-time.After(time.Second):
+ t.Fatal("expected signal notification to stop")
+ }
+ })
+
+ t.Run("cancels when parent context is canceled", func(t *testing.T) {
+ parent, cancel := context.WithCancel(context.Background())
+ ctx := signalContext(parent, make(chan os.Signal), func() {})
+
+ cancel()
+
+ select {
+ case <-ctx.Done():
+ case <-time.After(time.Second):
+ t.Fatal("expected signal context to be canceled by parent")
+ }
+ })
+}
+
func TestShouldCheckForUpdate(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test case where the parent context is already canceled before creating the signal context
The existing subtests cover (1) cancellation on first signal and (2) cancellation when a live parent is later canceled. Please add a subtest for a parent context that is already canceled before `signalContext` is called, to verify the returned context is immediately done and the `stop` callback is invoked in that scenario.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| case <-time.After(time.Second): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add an assertion that signal notification is stopped when the parent context is canceled
This subtest only checks that the child context is canceled when the parent is canceled; it doesn’t verify that the stop callback is invoked in this path. Since signalContext always defers stop(), consider mirroring the first subtest by passing a stopped channel and asserting it is closed when the parent is canceled, so the signal notification cleanup is covered for both signal- and parent-driven cancellation.
| @@ -68,6 +71,43 @@ func TestFailOnConfigError(t *testing.T) { | |||
| } | |||
| } | |||
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider a test case where the parent context is already canceled before creating the signal context
The existing subtests cover (1) cancellation on first signal and (2) cancellation when a live parent is later canceled. Please add a subtest for a parent context that is already canceled before signalContext is called, to verify the returned context is immediately done and the stop callback is invoked in that scenario.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix
lets self upgradeinterrupt handling so an in-progress download responds to Ctrl-C by canceling the shared CLI context and then restoring normal signal behavior for any later interrupt.Root Cause
The CLI signal handler kept SIGINT/SIGTERM registered after the first signal. If the self-upgrade path was still blocked while cancellation propagated, later Ctrl-C presses were still intercepted instead of falling back to the process default behavior.
Changes
Validation
go test ./internal/cli -run TestSignalContext -count=1go test ./...Summary by Sourcery
Improve CLI signal handling to ensure self-upgrade downloads respond correctly to interrupts and restore default behavior after the first signal.
Bug Fixes:
lets self upgradedownloads are canceled on the first Ctrl-C and stop custom signal handling afterward.Enhancements:
Documentation:
lets self upgradeinterrupt handling in the Unreleased changelog.Tests: