Repository navigation
feat(cli): cancel in-flight fetch on SIGINT/SIGTERM (PR B) - #25
Open
bschellenberger2600 wants to merge 9 commits into
Open
bschellenberger2600 wants to merge 9 commits into
bschellenberger2600 wants to merge 9 commits into
Conversation
Wire signal.NotifyContext into default streaming and post-TUI runs so Ctrl-C kills network git via RunNetworkGit, prints a partial summary, and exits non-zero. Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-971d' into cursor/run-cancel-signals-971d Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Interrupt deadlocks streaming fetch pipeline
- Workers now continue draining repoChan after cancel instead of breaking, so the upsert sender cannot block forever on send and <-upsertDone returns.
Or push these changes by commenting:
@cursor push 8892d3a80f
Preview (8892d3a80f)
diff --git a/cmd/default_stream_stall_test.go b/cmd/default_stream_stall_test.go
--- a/cmd/default_stream_stall_test.go
+++ b/cmd/default_stream_stall_test.go
@@ -2,6 +2,7 @@
import (
"bytes"
+ "context"
"fmt"
"io"
"net"
@@ -10,10 +11,10 @@
"testing"
"time"
+ testutil "github.com/git-fire/git-testkit"
"github.com/git-rain/git-rain/internal/config"
"github.com/git-rain/git-rain/internal/git"
"github.com/git-rain/git-rain/internal/registry"
- testutil "github.com/git-fire/git-testkit"
)
func TestRunRainDefaultStream_StallRemoteDoesNotBlockSummary(t *testing.T) {
@@ -79,6 +80,72 @@
}
}
+func TestRunRainDefaultStream_CancelDoesNotDeadlock(t *testing.T) {
+ baseURL, cleanup := startStallTCPListener(t)
+ defer cleanup()
+
+ // More repos than repoChan buffer + fetch workers so upsert blocks on send
+ // once workers stop consuming after cancel.
+ const nRepos = 12
+ known := make(map[string]bool, nRepos)
+ for i := 0; i < nRepos; i++ {
+ path := testutil.CreateTestRepo(t, testutil.RepoOptions{
+ Name: fmt.Sprintf("cancel-stall-%d", i),
+ Remotes: map[string]string{
+ "origin": fmt.Sprintf("%s/cancel-stall-%d.git", baseURL, i),
+ },
+ })
+ known[path] = true
+ }
+
+ cfg := config.DefaultConfig()
+ cfg.Global.FetchWorkers = 1
+ cfg.Global.FetchTimeoutSec = 60
+ cfg.Global.DisableScan = true
+
+ opts := git.DefaultScanOptions()
+ opts.DisableScan = true
+ opts.KnownPaths = known
+ opts.Workers = 2
+
+ ctx, cancel := context.WithCancel(context.Background())
+ rainOpts := git.RainOptions{
+ FetchTimeoutSec: 60,
+ RunCtx: ctx,
+ }
+
+ oldStdout := os.Stdout
+ r, w, _ := os.Pipe()
+ os.Stdout = w
+
+ done := make(chan error, 1)
+ go func() {
+ done <- runRainDefaultStream(&cfg, ®istry.Registry{}, "", opts, rainOpts, false)
+ }()
+
+ // Let the pipeline fill (workers busy on stalled fetches, upsert blocked).
+ time.Sleep(500 * time.Millisecond)
+ cancel()
+
+ var err error
+ select {
+ case err = <-done:
+ case <-time.After(8 * time.Second):
+ os.Stdout = oldStdout
+ _ = w.Close()
+ _, _ = io.Copy(io.Discard, r)
+ t.Fatal("runRainDefaultStream deadlocked after cancel")
+ }
+
+ os.Stdout = oldStdout
+ _ = w.Close()
+ _, _ = io.Copy(io.Discard, r)
+
+ if err == nil || err.Error() != "interrupted" {
+ t.Fatalf("expected interrupted error, got %v", err)
+ }
+}
+
func startStallTCPListener(t *testing.T) (baseURL string, cleanup func()) {
t.Helper()
ln, err := net.Listen("tcp", "127.0.0.1:0")
diff --git a/cmd/root.go b/cmd/root.go
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -21,10 +21,10 @@
"github.com/mattn/go-runewidth"
"github.com/spf13/cobra"
+ "github.com/git-fire/git-harness/safety"
"github.com/git-rain/git-rain/internal/config"
"github.com/git-rain/git-rain/internal/git"
"github.com/git-rain/git-rain/internal/registry"
- "github.com/git-fire/git-harness/safety"
"github.com/git-rain/git-rain/internal/sessionlog"
"github.com/git-rain/git-rain/internal/ui"
)
@@ -807,8 +807,10 @@
go func() {
defer workersWG.Done()
for repo := range repoChan {
+ // After cancel, keep draining so upsert cannot block forever
+ // on repoChan send (deadlocking on <-upsertDone).
if ctx.Err() != nil {
- break
+ continue
}
current := int(atomic.AddInt64(&seq, 1))
total := int(atomic.LoadInt64(&totalFound))You can send follow-ups to the cloud agent here.
Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
…frozen Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Interrupted sync looks empty
- RainRepository now returns context.Canceled on interrupt, and runRainOnRepo reports it as skipped instead of "no local branches".
- ✅ Fixed: Canceled fetches omitted from totals
- The fetch-only cancel path now increments delta.skipped so the summary matches the per-repo skipped lines.
Or push these changes by commenting:
@cursor push 9be3ee9c4e
Preview (9be3ee9c4e)
diff --git a/cmd/root.go b/cmd/root.go
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -21,10 +21,10 @@
"github.com/mattn/go-runewidth"
"github.com/spf13/cobra"
+ "github.com/git-fire/git-harness/safety"
"github.com/git-rain/git-rain/internal/config"
"github.com/git-rain/git-rain/internal/git"
"github.com/git-rain/git-rain/internal/registry"
- "github.com/git-fire/git-harness/safety"
"github.com/git-rain/git-rain/internal/sessionlog"
"github.com/git-rain/git-rain/internal/ui"
)
@@ -392,8 +392,9 @@
if !fullSync && !rainFetchMainline {
if fetchErr := fetchOnly(repoOpts.RunCtx, repo.Path, repoOpts); fetchErr != nil {
if errors.Is(fetchErr, context.Canceled) {
- _, _ = fmt.Fprintln(out, " ~ skipped\n")
+ _, _ = fmt.Fprintln(out, " ~ skipped")
_, _ = fmt.Fprintln(out)
+ delta.skipped++
return delta
}
_, _ = fmt.Fprintf(out, " ❄ (fetch --all): %s\n\n",
@@ -430,6 +431,12 @@
res, rainErr := git.RainRepository(repo.Path, repoOpts)
if rainErr != nil {
+ if errors.Is(rainErr, context.Canceled) {
+ _, _ = fmt.Fprintln(out, " ~ skipped")
+ _, _ = fmt.Fprintln(out)
+ delta.skipped++
+ return delta
+ }
_, _ = fmt.Fprintf(out, " ✗ failed: %s\n\n", safety.SanitizeText(rainErr.Error()))
delta.failed++
return delta
diff --git a/internal/git/rain.go b/internal/git/rain.go
--- a/internal/git/rain.go
+++ b/internal/git/rain.go
@@ -223,7 +223,8 @@
output, fetchErr := RunNetworkGit(runCtx, cmd, opts.FetchTimeoutSec)
if fetchErr != nil {
if errors.Is(fetchErr, context.Canceled) {
- return result, nil
+ // Propagate cancel so callers do not treat this as "no local branches".
+ return result, fetchErr
}
// Freeze gracefully — could not reach remote (auth, network, etc.).
// This is not a hard failure; the repo is untouched. Try again later.You can send follow-ups to the cloud agent here.
Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
…gnals-971d Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Default path skips interrupt summary
- Removed the early Aborted. return so canceled default runs reach printRainSummary/JSON emit and return interrupted.
- ✅ Fixed: JSON rain path ignores interrupt
- runRainOnRepos now returns interrupted after JSON emit when RunCtx is canceled, matching the default stream path.
Or push these changes by commenting:
@cursor push ec4eba24db
Preview (ec4eba24db)
diff --git a/cmd/default_stream_stall_test.go b/cmd/default_stream_stall_test.go
--- a/cmd/default_stream_stall_test.go
+++ b/cmd/default_stream_stall_test.go
@@ -139,8 +139,8 @@
select {
case err := <-done:
- if err != nil {
- t.Fatalf("runRainDefaultStream: %v", err)
+ if err == nil || !strings.Contains(err.Error(), "interrupted") {
+ t.Fatalf("runRainDefaultStream: want interrupted error, got %v", err)
}
case <-time.After(5 * time.Second):
t.Fatal("runRainDefaultStream did not return after cancel")
@@ -152,12 +152,15 @@
_, _ = io.Copy(&out, r)
body := out.String()
- if !strings.Contains(body, "Aborted.") {
- t.Fatalf("expected Aborted. after cancel, got:\n%s", body)
+ if !strings.Contains(body, "rain interrupted") {
+ t.Fatalf("expected rain interrupted summary after cancel, got:\n%s", body)
}
if strings.Contains(body, "rain delivered") {
t.Fatalf("did not expect completed summary after cancel, got:\n%s", body)
}
+ if strings.Contains(body, "Aborted.") {
+ t.Fatalf("did not expect Aborted. after cancel, got:\n%s", body)
+ }
// With one worker and three stalled remotes, cancel must not fetch all three
// to completion (each would otherwise hang until the 30s timeout).
if elapsed > 5*time.Second {
diff --git a/cmd/root.go b/cmd/root.go
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -971,15 +971,12 @@
printMu.Lock()
defer printMu.Unlock()
- if ctx.Err() != nil {
- fmt.Println("Aborted.")
- return nil
- }
-
if atomic.LoadInt64(&seq) == 0 {
if rainOpts.RunCtx != nil && rainOpts.RunCtx.Err() != nil {
if rainJSON {
- return emitJSONRun(buildJSONRunMeta(rainOpts, fullSync), rainTotals{}, nil)
+ if err := emitJSONRun(buildJSONRunMeta(rainOpts, fullSync), rainTotals{}, nil); err != nil {
+ return err
+ }
}
return fmt.Errorf("interrupted")
}
@@ -1041,6 +1038,9 @@
if err := emitJSONRun(buildJSONRunMeta(opts, fullSync), totals, jsonReports); err != nil {
return err
}
+ if opts.RunCtx != nil && opts.RunCtx.Err() != nil {
+ return fmt.Errorf("interrupted")
+ }
if totals.failed > 0 {
return fmt.Errorf("%d branch(es) failed — see --json output", totals.failed)
}You can send follow-ups to the cloud agent here.
|
Bugbot Autofix prepared fixes for both issues found in the latest run.
Or push these changes by commenting: Preview (ec4eba24db)diff --git a/cmd/default_stream_stall_test.go b/cmd/default_stream_stall_test.go
--- a/cmd/default_stream_stall_test.go
+++ b/cmd/default_stream_stall_test.go
@@ -139,8 +139,8 @@
select {
case err := <-done:
- if err != nil {
- t.Fatalf("runRainDefaultStream: %v", err)
+ if err == nil || !strings.Contains(err.Error(), "interrupted") {
+ t.Fatalf("runRainDefaultStream: want interrupted error, got %v", err)
}
case <-time.After(5 * time.Second):
t.Fatal("runRainDefaultStream did not return after cancel")
@@ -152,12 +152,15 @@
_, _ = io.Copy(&out, r)
body := out.String()
- if !strings.Contains(body, "Aborted.") {
- t.Fatalf("expected Aborted. after cancel, got:\n%s", body)
+ if !strings.Contains(body, "rain interrupted") {
+ t.Fatalf("expected rain interrupted summary after cancel, got:\n%s", body)
}
if strings.Contains(body, "rain delivered") {
t.Fatalf("did not expect completed summary after cancel, got:\n%s", body)
}
+ if strings.Contains(body, "Aborted.") {
+ t.Fatalf("did not expect Aborted. after cancel, got:\n%s", body)
+ }
// With one worker and three stalled remotes, cancel must not fetch all three
// to completion (each would otherwise hang until the 30s timeout).
if elapsed > 5*time.Second {
diff --git a/cmd/root.go b/cmd/root.go
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -971,15 +971,12 @@
printMu.Lock()
defer printMu.Unlock()
- if ctx.Err() != nil {
- fmt.Println("Aborted.")
- return nil
- }
-
if atomic.LoadInt64(&seq) == 0 {
if rainOpts.RunCtx != nil && rainOpts.RunCtx.Err() != nil {
if rainJSON {
- return emitJSONRun(buildJSONRunMeta(rainOpts, fullSync), rainTotals{}, nil)
+ if err := emitJSONRun(buildJSONRunMeta(rainOpts, fullSync), rainTotals{}, nil); err != nil {
+ return err
+ }
}
return fmt.Errorf("interrupted")
}
@@ -1041,6 +1038,9 @@
if err := emitJSONRun(buildJSONRunMeta(opts, fullSync), totals, jsonReports); err != nil {
return err
}
+ if opts.RunCtx != nil && opts.RunCtx.Err() != nil {
+ return fmt.Errorf("interrupted")
+ }
if totals.failed > 0 {
return fmt.Errorf("%d branch(es) failed — see --json output", totals.failed)
}You can send follow-ups to the cloud agent here. |
…gnals-971d Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Canceled fetch leaves outcome unset
- Canceled fetch --all now increments delta.skipped and sets summary_outcome to "skipped" so totals and JSON match the human skip line.
Or push these changes by commenting:
@cursor push c5fbebaedb
Preview (c5fbebaedb)
diff --git a/cmd/root.go b/cmd/root.go
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -451,6 +451,8 @@
_, _ = fmt.Fprintln(out, " ~ skipped")
_, _ = fmt.Fprintln(out)
}
+ delta.skipped++
+ report.SummaryOutcome = "skipped"
return delta, report
}
msg := safety.SanitizeText(fetchFailureMessage(fetchErr.Error()))You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 44ff171. Configure here.
Canceled fetches already printed "~ skipped" but left delta.skipped and summary_outcome unset, so interrupted repos disappeared from totals and JSON reports. Set both to match the human output.
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
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.
Stacked stall-review fix 2/3. Base:
cursor/network-git-fetch-timeout-971d(PR A). PR C stacks on this branch.Summary
signal.NotifyContext(SIGINT/SIGTERM) into live runs for the default streaming path and the post---rainfetch path.RainOptions.RunCtxsoRunNetworkGitcancels in-flightgit fetch.rain interrupted — …) and exit non-zero.Tests
TestRunNetworkGit_CancelStopsInFlightFetch— cancel during stall fetch completes in <5s.Verification
go vet ./...— passgo test -race -count=1 ./...— pass except known pre-existingTestNetworkFetch_UnauthenticatedHTTPSFailsWithoutPrompt(PR test: make network auth fetch regression test deterministic #21)