From db3bb7597355bf5a6b3ba9d7d5c288abaad2bbe1 Mon Sep 17 00:00:00 2001 From: Jonatan Dahl Date: Wed, 12 Aug 2026 15:50:46 -0400 Subject: [PATCH] fix: bound sync network operations Co-Authored-By: Codex --- cmd/prune.go | 7 ++--- cmd/status.go | 5 +++- cmd/sync.go | 47 ++++++++++++++++++++-------------- cmd/worktree.go | 5 ++-- decision-log.md | 8 ++++++ internal/git/git.go | 20 +++++++++++++-- internal/git/git_test.go | 20 ++++++++++++++- internal/github/github.go | 38 ++++++++++++++++++++++----- internal/github/github_test.go | 34 +++++++++++++++++++++++- internal/github/interface.go | 3 +-- internal/testutil/mocks.go | 8 ++++-- 11 files changed, 156 insertions(+), 39 deletions(-) diff --git a/cmd/prune.go b/cmd/prune.go index 2fe4f39..bd89b80 100644 --- a/cmd/prune.go +++ b/cmd/prune.go @@ -111,10 +111,11 @@ func runPrune(gitClient git.GitClient, githubClient github.GitHubClient) error { // Fetch PRs for the branches we need to check (parallel individual fetches) var prCache map[string]*github.PRInfo if err := spinner.WrapWithSuccess("Fetching PRs...", "Fetched PRs", func() error { - prCache = githubClient.GetPRsForBranches(branchNames) - return nil - }); err != nil { + var err error + prCache, err = githubClient.GetPRsForBranches(branchNames) return err + }); err != nil { + return fmt.Errorf("failed to fetch PRs: %w", err) } // Find branches with merged PRs diff --git a/cmd/status.go b/cmd/status.go index 3a51c92..3251914 100644 --- a/cmd/status.go +++ b/cmd/status.go @@ -115,7 +115,10 @@ func runStatus(gitClient git.GitClient, githubClient github.GitHubClient) error // Fetch PRs for stack branches only (parallel individual fetches) if !noPR { - prCache = githubClient.GetPRsForBranches(allTreeBranches) + prCache, err = githubClient.GetPRsForBranches(allTreeBranches) + if err != nil { + return fmt.Errorf("failed to load PRs: %w", err) + } } return nil diff --git a/cmd/sync.go b/cmd/sync.go index 7afcea5..e5fdf45 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -23,21 +23,21 @@ import ( var errAlreadyPrinted = errors.New("") var ( - syncForce bool - syncResume bool - syncAbort bool - syncCherryPick bool - syncCrossWorktree bool - syncAll bool + syncForce bool + syncResume bool + syncAbort bool + syncCherryPick bool + syncCrossWorktree bool + syncAll bool // stdinReader allows tests to inject mock input for prompts stdinReader io.Reader = os.Stdin ) // Git config keys for sync state persistence const ( - configSyncStashed = "stack.sync.stashed" - configSyncOriginalBranch = "stack.sync.originalBranch" - configSyncConflictWorktree = "stack.sync.conflictWorktreePath" + configSyncStashed = "stack.sync.stashed" + configSyncOriginalBranch = "stack.sync.originalBranch" + configSyncConflictWorktree = "stack.sync.conflictWorktreePath" ) var syncCmd = &cobra.Command{ @@ -510,24 +510,33 @@ func runSync(gitClient git.GitClient, githubClient github.GitHubClient, syncRemo prBranches = append(prBranches, b) } - // Wait for git fetch and fetch PRs in parallel for stack branches only + // Wait for the bounded git fetch before loading PRs. Keeping these as separate + // progress steps makes it clear which network dependency is slow or failed. var prCache map[string]*github.PRInfo - fetchMsg := fmt.Sprintf("Fetching from %s and loading PRs...", syncRemote) - fetchDoneMsg := fmt.Sprintf("Fetched from %s and loaded PRs", syncRemote) + fetchMsg := fmt.Sprintf("Fetching from %s...", syncRemote) + fetchDoneMsg := fmt.Sprintf("Fetched from %s", syncRemote) if err := spinner.WrapWithSuccess(fetchMsg, fetchDoneMsg, func() error { wg.Wait() - prCache = githubClient.GetPRsForBranches(prBranches) + if fetchErr != nil { + return fmt.Errorf("failed to fetch from %s: %w", syncRemote, fetchErr) + } + if originFetchErr != nil { + return fmt.Errorf("failed to fetch from origin: %w", originFetchErr) + } return nil }); err != nil { return err } - // Check for fetch errors - if fetchErr != nil { - return fmt.Errorf("failed to fetch from %s: %w", syncRemote, fetchErr) - } - if originFetchErr != nil { - return fmt.Errorf("failed to fetch from origin: %w", originFetchErr) + if err := spinner.WrapWithSuccess("Loading PRs...", "Loaded PRs", func() error { + var err error + prCache, err = githubClient.GetPRsForBranches(prBranches) + if err != nil { + return fmt.Errorf("failed to load PRs: %w", err) + } + return nil + }); err != nil { + return err } // Get all remote branches in one call (more efficient than checking each branch individually) diff --git a/cmd/worktree.go b/cmd/worktree.go index 806aa1c..4d14f39 100644 --- a/cmd/worktree.go +++ b/cmd/worktree.go @@ -438,8 +438,9 @@ func runWorktreePrune(gitClient git.GitClient, githubClient github.GitHubClient) } var prCache map[string]*github.PRInfo if err := spinner.WrapWithSuccess("Fetching PRs...", "Fetched PRs", func() error { - prCache = githubClient.GetPRsForBranches(wtBranches) - return nil + var err error + prCache, err = githubClient.GetPRsForBranches(wtBranches) + return err }); err != nil { return fmt.Errorf("failed to fetch PRs: %w", err) } diff --git a/decision-log.md b/decision-log.md index 1a573ad..f5ab587 100644 --- a/decision-log.md +++ b/decision-log.md @@ -2,6 +2,14 @@ Architectural and design decisions for Stackinator. +## 2026-08-12 — Bound and separate sync network operations + +**Decision**: Give git fetches a five-minute timeout and GitHub CLI operations a 30-second timeout. Display fetch and PR loading as separate sync progress steps, and propagate PR lookup failures. + +**Context**: `stack sync` grouped an unbounded background `git fetch` and unbounded parallel `gh pr view` calls under one spinner. A stalled remote, credential helper, or GHE request could therefore hang forever at `Fetching from origin and loading PRs...`, and GitHub errors were silently interpreted as missing PRs. + +**Resolution**: Run fetch and GitHub subprocesses with context deadlines and a bounded pipe wait. Wait for fetch and PR loading in distinct spinner steps so the active dependency is visible. Return PR lookup failures to callers while preserving the normal no-PR result. + ## 2026-08-04 — Distribute the Codex skill through a plugin marketplace **Decision**: Publish the existing Stackinator skill as a Codex plugin from the Stackinator repository. diff --git a/internal/git/git.go b/internal/git/git.go index d75c6f7..a1c483d 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -2,10 +2,12 @@ package git import ( "bytes" + "context" "fmt" "os/exec" "path/filepath" "strings" + "time" ) // Verbose controls whether to print executed commands @@ -14,6 +16,10 @@ var Verbose = false // DryRun controls whether to actually execute mutation commands var DryRun = false +// fetchTimeout prevents a stalled remote or credential helper from blocking a +// sync forever. It is a variable so timeout behavior can be tested quickly. +var fetchTimeout = 5 * time.Minute + // gitClient implements the GitClient interface using exec.Command type gitClient struct { dir string @@ -40,17 +46,25 @@ func (c *gitClient) gitArgs(args ...string) []string { // runCmd executes a git command and returns stdout func (c *gitClient) runCmd(args ...string) (string, error) { + return c.runCmdWithContext(context.Background(), args...) +} + +func (c *gitClient) runCmdWithContext(ctx context.Context, args ...string) (string, error) { args = c.gitArgs(args...) if Verbose { fmt.Printf(" [git] %s\n", strings.Join(args, " ")) } - cmd := exec.Command("git", args...) + cmd := exec.CommandContext(ctx, "git", args...) + cmd.WaitDelay = 2 * time.Second var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr err := cmd.Run() if err != nil { + if ctx.Err() != nil { + return "", fmt.Errorf("git %s timed out: %w", strings.Join(args, " "), ctx.Err()) + } return "", fmt.Errorf("git %s failed: %s", strings.Join(args, " "), stderr.String()) } @@ -308,7 +322,9 @@ func (c *gitClient) FetchRemote(remote string) error { fmt.Printf(" [DRY RUN] git fetch %s\n", remote) return nil } - _, err := c.runCmd("fetch", remote) + ctx, cancel := context.WithTimeout(context.Background(), fetchTimeout) + defer cancel() + _, err := c.runCmdWithContext(ctx, "fetch", remote) return err } diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 7f6cfe9..7222c3a 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -1,7 +1,11 @@ package git import ( + "os" + "path/filepath" + "strings" "testing" + "time" "github.com/stretchr/testify/assert" ) @@ -16,7 +20,21 @@ func TestGitClientInterface(t *testing.T) { var _ GitClient = &gitClient{} } +func TestFetchRemoteTimesOut(t *testing.T) { + tempDir := t.TempDir() + gitPath := filepath.Join(tempDir, "git") + assert.NoError(t, os.WriteFile(gitPath, []byte("#!/bin/sh\nexec sleep 10\n"), 0o755)) + t.Setenv("PATH", tempDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + previousTimeout := fetchTimeout + fetchTimeout = 20 * time.Millisecond + t.Cleanup(func() { fetchTimeout = previousTimeout }) + + err := NewGitClient().FetchRemote("origin") + assert.Error(t, err) + assert.True(t, strings.Contains(err.Error(), "git fetch origin timed out"), err.Error()) +} + // Note: More comprehensive tests would require mocking exec.Command or running actual git commands // For unit tests focused on critical path, we rely on integration tests or testutil mocks // The real value is in testing the stack package and command packages with mocked clients - diff --git a/internal/github/github.go b/internal/github/github.go index 1bd39c7..093f610 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -2,12 +2,15 @@ package github import ( "bytes" + "context" "encoding/json" + "errors" "fmt" "os/exec" "strconv" "strings" "sync" + "time" ) // Verbose controls whether to print executed commands @@ -16,6 +19,10 @@ var Verbose = false // DryRun controls whether to actually execute mutation commands var DryRun = false +// commandTimeout prevents an unresponsive GitHub host or credential helper +// from blocking stack operations forever. It is a variable for fast tests. +var commandTimeout = 30 * time.Second + // PRInfo contains information about a Pull Request type PRInfo struct { Number int @@ -98,13 +105,19 @@ func (c *githubClient) runGH(args ...string) (string, error) { if Verbose { fmt.Printf(" [gh] %s\n", strings.Join(args, " ")) } - cmd := exec.Command("gh", args...) + ctx, cancel := context.WithTimeout(context.Background(), commandTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, "gh", args...) + cmd.WaitDelay = 2 * time.Second var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr err := cmd.Run() if err != nil { + if ctx.Err() != nil { + return "", fmt.Errorf("gh %s timed out after %s: %w", strings.Join(args, " "), commandTimeout, ctx.Err()) + } return "", fmt.Errorf("gh %s failed: %s", strings.Join(args, " "), stderr.String()) } @@ -115,8 +128,13 @@ func (c *githubClient) runGH(args ...string) (string, error) { func (c *githubClient) GetPRForBranch(branch string) (*PRInfo, error) { output, err := c.runGH("pr", "view", branch, "--json", "number,state,baseRefName,title,url,mergeStateStatus") if err != nil { - // No PR exists for this branch - return nil, nil + // gh uses a non-zero exit status when no PR exists. Preserve that behavior, + // but surface timeouts and operational failures instead of silently treating + // them as an absent PR. + if strings.Contains(strings.ToLower(err.Error()), "no pull requests found") { + return nil, nil + } + return nil, err } var data struct { @@ -145,16 +163,24 @@ func (c *githubClient) GetPRForBranch(branch string) (*PRInfo, error) { // GetPRsForBranches fetches PR info for specific branches in parallel. // This is much faster than bulk-fetching all PRs on large repos (500+ PRs), // where `gh pr list --limit 500` can time out with 502 Bad Gateway. -func (c *githubClient) GetPRsForBranches(branches []string) map[string]*PRInfo { +func (c *githubClient) GetPRsForBranches(branches []string) (map[string]*PRInfo, error) { result := make(map[string]*PRInfo) var mu sync.Mutex var wg sync.WaitGroup + var errs []error for _, branch := range branches { wg.Add(1) go func(b string) { defer wg.Done() - if pr, err := c.GetPRForBranch(b); err == nil && pr != nil { + pr, err := c.GetPRForBranch(b) + if err != nil { + mu.Lock() + errs = append(errs, fmt.Errorf("failed to load PR for %s: %w", b, err)) + mu.Unlock() + return + } + if pr != nil { mu.Lock() result[b] = pr mu.Unlock() @@ -163,7 +189,7 @@ func (c *githubClient) GetPRsForBranches(branches []string) map[string]*PRInfo { } wg.Wait() - return result + return result, errors.Join(errs...) } // UpdatePRBase updates the base branch of a PR diff --git a/internal/github/github_test.go b/internal/github/github_test.go index d7aa7e3..006ec09 100644 --- a/internal/github/github_test.go +++ b/internal/github/github_test.go @@ -1,7 +1,11 @@ package github import ( + "os" + "path/filepath" + "strings" "testing" + "time" "github.com/stretchr/testify/assert" ) @@ -76,6 +80,34 @@ func TestParseRepoFromURL(t *testing.T) { } } +func TestGetPRsForBranchesTimesOut(t *testing.T) { + tempDir := t.TempDir() + ghPath := filepath.Join(tempDir, "gh") + assert.NoError(t, os.WriteFile(ghPath, []byte("#!/bin/sh\nexec sleep 10\n"), 0o755)) + t.Setenv("PATH", tempDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + previousTimeout := commandTimeout + commandTimeout = 20 * time.Millisecond + t.Cleanup(func() { commandTimeout = previousTimeout }) + + prs, err := NewGitHubClient("ghe.spotify.net/org/repo").GetPRsForBranches([]string{"feature"}) + assert.Empty(t, prs) + assert.Error(t, err) + assert.True(t, strings.Contains(err.Error(), "failed to load PR for feature"), err.Error()) + assert.True(t, strings.Contains(err.Error(), "timed out after 20ms"), err.Error()) +} + +func TestGetPRForBranchTreatsMissingPRAsAbsent(t *testing.T) { + tempDir := t.TempDir() + ghPath := filepath.Join(tempDir, "gh") + script := "#!/bin/sh\necho 'no pull requests found for branch feature' >&2\nexit 1\n" + assert.NoError(t, os.WriteFile(ghPath, []byte(script), 0o755)) + t.Setenv("PATH", tempDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + pr, err := NewGitHubClient("owner/repo").GetPRForBranch("feature") + assert.NoError(t, err) + assert.Nil(t, pr) +} + // Note: More comprehensive tests would require mocking exec.Command or running actual gh CLI commands // For unit tests focused on critical path, we rely on integration tests or testutil mocks - diff --git a/internal/github/interface.go b/internal/github/interface.go index 97e441f..0a84a28 100644 --- a/internal/github/interface.go +++ b/internal/github/interface.go @@ -3,8 +3,7 @@ package github // GitHubClient defines the interface for all GitHub operations type GitHubClient interface { GetPRForBranch(branch string) (*PRInfo, error) - GetPRsForBranches(branches []string) map[string]*PRInfo + GetPRsForBranches(branches []string) (map[string]*PRInfo, error) UpdatePRBase(prNumber int, newBase string) error IsPRMerged(prNumber int) (bool, error) } - diff --git a/internal/testutil/mocks.go b/internal/testutil/mocks.go index 5ff1329..9f54191 100644 --- a/internal/testutil/mocks.go +++ b/internal/testutil/mocks.go @@ -300,9 +300,13 @@ func (m *MockGitHubClient) GetPRForBranch(branch string) (*github.PRInfo, error) return args.Get(0).(*github.PRInfo), args.Error(1) } -func (m *MockGitHubClient) GetPRsForBranches(branches []string) map[string]*github.PRInfo { +func (m *MockGitHubClient) GetPRsForBranches(branches []string) (map[string]*github.PRInfo, error) { args := m.Called(branches) - return args.Get(0).(map[string]*github.PRInfo) + var err error + if len(args) > 1 { + err = args.Error(1) + } + return args.Get(0).(map[string]*github.PRInfo), err } func (m *MockGitHubClient) UpdatePRBase(prNumber int, newBase string) error {