fix(worktree): bound git ref-inspection with context timeout (#685)

* fix(worktree): bound git ref-inspection with context timeout branchExists and currentBranch spawned raw exec.Command with no context and no timeout, and both were called while Manager.Create held the per-repo mutex. A hung git subprocess (credential helper, stuck filter, filesystem stall) kept the lock in Manager.repoLocks forever, blocking every subsequent task on the same repository until the backend was restarted. Route both helpers through newNonInteractiveGitCmd with a new defaultGitInspectTimeout (10s) so ctx cancellation and bounded deadlines propagate to the subprocess. Also log lock-hold duration on release for future diagnosis. * fix(worktree): address review feedback on inspect-timeout change - branchExists/currentBranch now log the ctx error when the bound fires, so a hung git surfaces in logs instead of only as a misleading "branch not found" (CodeRabbit). - inspectTimeout is a Manager field (defaults to defaultGitInspectTimeout) so tests can shrink it; added an isolation test that verifies the internal bound fires even when the caller ctx has no deadline (Claude). - Route setUpstreamIfExists through newNonInteractiveGitCmd so it also gets non-interactive env + WaitDelay while repoLock is held (Claude). - Simplify TestCreate_HangingRevParseReleasesRepoLock: drop redundant sync.WaitGroup and extract hangOnRevParseScript constant shared by all four tests (CodeRabbit).
2026-04-24 00:41:49 +01:00
parent e1d21e39a9
commit 7e938f6bdb
2 changed files with 230 additions and 27 deletions
@@ -18,9 +18,10 @@ import (
 )

 const (
-	defaultGitFetchTimeout = 90 * time.Second
-	defaultGitPullTimeout  = 60 * time.Second
-	gitNoTags              = "--no-tags"
+	defaultGitFetchTimeout   = 90 * time.Second
+	defaultGitPullTimeout    = 60 * time.Second
+	defaultGitInspectTimeout = 10 * time.Second
+	gitNoTags                = "--no-tags"
 )

 // repoLockEntry tracks a repository lock and its reference count.
@@ -46,6 +47,8 @@ type Manager struct {
 	// Timeouts for best-effort remote sync before creating a worktree.
 	fetchTimeout time.Duration
 	pullTimeout  time.Duration
+	// Bound for cheap git ref-inspection commands (branchExists, currentBranch).
+	inspectTimeout time.Duration
 }

 // ScriptMessageHandler provides script execution and message streaming.
@@ -115,13 +118,14 @@ func NewManager(cfg Config, store Store, log *logger.Logger) (*Manager, error) {
 	}

 	return &Manager{
-		config:       cfg,
-		logger:       log.WithFields(zap.String("component", "worktree-manager")),
-		store:        store,
-		worktrees:    make(map[string]*Worktree),
-		repoLocks:    make(map[string]*repoLockEntry),
-		fetchTimeout: fetchTimeout,
-		pullTimeout:  pullTimeout,
+		config:         cfg,
+		logger:         log.WithFields(zap.String("component", "worktree-manager")),
+		store:          store,
+		worktrees:      make(map[string]*Worktree),
+		repoLocks:      make(map[string]*repoLockEntry),
+		fetchTimeout:   fetchTimeout,
+		pullTimeout:    pullTimeout,
+		inspectTimeout: defaultGitInspectTimeout,
 	}, nil
 }

@@ -241,8 +245,12 @@ func (m *Manager) Create(ctx context.Context, req CreateRequest) (*Worktree, err
 	// Get repository lock for safe concurrent access
 	repoLock := m.getRepoLock(req.RepositoryPath)
 	repoLock.Lock()
+	lockAcquired := time.Now()
 	defer func() {
 		repoLock.Unlock()
+		m.logger.Debug("released worktree repo lock",
+			zap.String("repository_path", req.RepositoryPath),
+			zap.Duration("held", time.Since(lockAcquired)))
 		m.releaseRepoLock(req.RepositoryPath)
 	}()

@@ -252,7 +260,7 @@ func (m *Manager) Create(ctx context.Context, req CreateRequest) (*Worktree, err
 	}

 	// Check base branch exists
-	if !m.branchExists(req.RepositoryPath, baseRef) {
+	if !m.branchExists(ctx, req.RepositoryPath, baseRef) {
 		return nil, fmt.Errorf("%w: %s", ErrInvalidBaseBranch, baseRef)
 	}

@@ -413,14 +421,13 @@ func (m *Manager) addWorktreeForBranch(ctx context.Context, req CreateRequest, w
 // to origin/<remoteBranch> if the remote-tracking ref exists. Non-fatal on failure.
 func (m *Manager) setUpstreamIfExists(ctx context.Context, worktreePath, localBranch, remoteBranch string) {
 	upstream := "origin/" + remoteBranch
-	// Verify the remote-tracking ref exists
-	verifyCmd := exec.CommandContext(ctx, "git", "rev-parse", "--verify", upstream)
-	verifyCmd.Dir = worktreePath
+	// Verify the remote-tracking ref exists. Use the non-interactive helper so
+	// this cannot hang on a credential prompt while Create holds repoLock.
+	verifyCmd := m.newNonInteractiveGitCmd(ctx, worktreePath, "rev-parse", "--verify", upstream)
 	if err := verifyCmd.Run(); err != nil {
 		return
 	}
-	cmd := exec.CommandContext(ctx, "git", "branch", "--set-upstream-to="+upstream, localBranch)
-	cmd.Dir = worktreePath
+	cmd := m.newNonInteractiveGitCmd(ctx, worktreePath, "branch", "--set-upstream-to="+upstream, localBranch)
 	if out, err := cmd.CombinedOutput(); err != nil {
 		m.logger.Debug("failed to set upstream (non-fatal)",
 			zap.String("branch", localBranch),
@@ -473,7 +480,7 @@ func (m *Manager) fetchBranchToLocal(ctx context.Context, repoPath, branch strin
 			zap.Error(err))

 		// Fall back to local branch if it exists.
-		if !m.branchExists(repoPath, branch) {
+		if !m.branchExists(ctx, repoPath, branch) {
 			return nil, fmt.Errorf("branch %q not found locally or on remote: %s", branch, outputStr)
 		}

@@ -1227,18 +1234,37 @@ func (m *Manager) isGitRepo(path string) bool {
 }

 // branchExists checks if a branch exists in the repository.
-func (m *Manager) branchExists(repoPath, branch string) bool {
-	cmd := exec.Command("git", "rev-parse", "--verify", branch)
-	cmd.Dir = repoPath
-	err := cmd.Run()
-	return err == nil
+// Bounded by m.inspectTimeout so a hung git (credential prompt, stuck filter,
+// filesystem stall) cannot deadlock the caller while holding repoLock. When
+// the bound fires, the ctx error is logged so the root cause is visible in
+// logs rather than surfacing only as a misleading "branch not found".
+func (m *Manager) branchExists(ctx context.Context, repoPath, branch string) bool {
+	inspectCtx, cancel := context.WithTimeout(ctx, m.inspectTimeout)
+	defer cancel()
+	cmd := m.newNonInteractiveGitCmd(inspectCtx, repoPath, "rev-parse", "--verify", branch)
+	if err := cmd.Run(); err != nil {
+		if ctxErr := inspectCtx.Err(); ctxErr != nil {
+			m.logger.Warn("branchExists bounded by context",
+				zap.String("repository_path", repoPath),
+				zap.String("branch", branch),
+				zap.Error(ctxErr))
+		}
+		return false
+	}
+	return true
 }

-func (m *Manager) currentBranch(repoPath string) string {
-	cmd := exec.Command("git", "rev-parse", "--abbrev-ref", "HEAD")
-	cmd.Dir = repoPath
+func (m *Manager) currentBranch(ctx context.Context, repoPath string) string {
+	inspectCtx, cancel := context.WithTimeout(ctx, m.inspectTimeout)
+	defer cancel()
+	cmd := m.newNonInteractiveGitCmd(inspectCtx, repoPath, "rev-parse", "--abbrev-ref", "HEAD")
 	output, err := cmd.Output()
 	if err != nil {
+		if ctxErr := inspectCtx.Err(); ctxErr != nil {
+			m.logger.Warn("currentBranch bounded by context",
+				zap.String("repository_path", repoPath),
+				zap.Error(ctxErr))
+		}
 		return ""
 	}
 	return strings.TrimSpace(string(output))
@@ -1346,10 +1372,10 @@ func (m *Manager) resolveLocalBaseRef(
 	onProgress SyncProgressCallback,
 ) string {
 	remoteRef := "origin/" + localBranch
-	if m.currentBranch(repoPath) == baseBranch {
+	if m.currentBranch(ctx, repoPath) == baseBranch {
 		return m.pullCurrentBranchOrFallback(ctx, repoPath, baseBranch, remoteRef, stepName, onProgress)
 	}
-	if m.branchExists(repoPath, remoteRef) {
+	if m.branchExists(ctx, repoPath, remoteRef) {
 		m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Synced and using %s", remoteRef), "")
 		return remoteRef
 	}
@@ -0,0 +1,177 @@
+package worktree
+
+import (
+	"context"
+	"os"
+	"testing"
+	"time"
+)
+
+// hangOnRevParseScript is the shared fake-git script body used by the timeout
+// regression tests. It sleeps indefinitely on `git rev-parse` and no-ops on
+// everything else (including `fetch`), so branchExists, currentBranch, and
+// the pullBaseBranch path that calls them all exercise the hang scenario.
+const hangOnRevParseScript = `
+case "${1:-}" in
+  rev-parse)
+    sleep 30
+    exit 0
+    ;;
+  *)
+    exit 0
+    ;;
+esac
+`
+
+// TestBranchExists_RespectsContextDeadline verifies that branchExists cancels
+// the underlying git subprocess when its caller-provided context expires.
+func TestBranchExists_RespectsContextDeadline(t *testing.T) {
+	scriptDir := writeFakeGitScript(t, hangOnRevParseScript)
+	t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	cfg := newTestConfig(t)
+	mgr, err := NewManager(cfg, newMockStore(), newTestLogger())
+	if err != nil {
+		t.Fatalf("NewManager failed: %v", err)
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond)
+	defer cancel()
+
+	start := time.Now()
+	exists := mgr.branchExists(ctx, t.TempDir(), "main")
+	elapsed := time.Since(start)
+
+	if exists {
+		t.Fatalf("branchExists() = true, want false on hanging git")
+	}
+	if elapsed > 2*time.Second {
+		t.Fatalf("branchExists() took %v, want <2s (ctx not propagated to subprocess)", elapsed)
+	}
+}
+
+// TestCurrentBranch_RespectsContextDeadline verifies that currentBranch
+// cancels the git subprocess when its caller-provided context expires.
+func TestCurrentBranch_RespectsContextDeadline(t *testing.T) {
+	scriptDir := writeFakeGitScript(t, hangOnRevParseScript)
+	t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	cfg := newTestConfig(t)
+	mgr, err := NewManager(cfg, newMockStore(), newTestLogger())
+	if err != nil {
+		t.Fatalf("NewManager failed: %v", err)
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond)
+	defer cancel()
+
+	start := time.Now()
+	branch := mgr.currentBranch(ctx, t.TempDir())
+	elapsed := time.Since(start)
+
+	if branch != "" {
+		t.Fatalf("currentBranch() = %q, want empty on hanging git", branch)
+	}
+	if elapsed > 2*time.Second {
+		t.Fatalf("currentBranch() took %v, want <2s (ctx not propagated to subprocess)", elapsed)
+	}
+}
+
+// TestBranchExists_BoundedWhenCallerHasNoDeadline pins the behaviour of the
+// Manager's internal inspectTimeout. With a background (never-cancelled)
+// caller ctx, branchExists must still return within m.inspectTimeout so a
+// future refactor that drops the wrapping timeout cannot silently regress
+// the fix. We shrink inspectTimeout for the test to keep it fast.
+func TestBranchExists_BoundedWhenCallerHasNoDeadline(t *testing.T) {
+	scriptDir := writeFakeGitScript(t, hangOnRevParseScript)
+	t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	cfg := newTestConfig(t)
+	mgr, err := NewManager(cfg, newMockStore(), newTestLogger())
+	if err != nil {
+		t.Fatalf("NewManager failed: %v", err)
+	}
+	mgr.inspectTimeout = 300 * time.Millisecond
+
+	start := time.Now()
+	exists := mgr.branchExists(context.Background(), t.TempDir(), "main")
+	elapsed := time.Since(start)
+
+	if exists {
+		t.Fatalf("branchExists() = true, want false on hanging git")
+	}
+	// Budget = inspectTimeout + WaitDelay for subprocess pipe cleanup + slack.
+	if elapsed > 2*time.Second {
+		t.Fatalf("branchExists() took %v, want <2s (inspectTimeout not applied)", elapsed)
+	}
+}
+
+// TestCreate_HangingRevParseReleasesRepoLock is the core regression test for
+// the reported symptom: a hung git rev-parse during Create must not keep the
+// per-repo mutex locked indefinitely. Before the fix, branchExists and
+// currentBranch ignored ctx, so a backend restart was the only way to clear
+// the lock. After the fix, ctx cancellation propagates to the git subprocess
+// and Create returns, releasing the lock for subsequent callers.
+func TestCreate_HangingRevParseReleasesRepoLock(t *testing.T) {
+	scriptDir := writeFakeGitScript(t, hangOnRevParseScript)
+	t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	cfg := newTestConfig(t)
+	mgr, err := NewManager(cfg, newMockStore(), newTestLogger())
+	if err != nil {
+		t.Fatalf("NewManager failed: %v", err)
+	}
+	mgr.fetchTimeout = 100 * time.Millisecond
+	mgr.pullTimeout = 100 * time.Millisecond
+
+	repoPath := t.TempDir()
+	if err := os.MkdirAll(repoPath+"/.git", 0755); err != nil {
+		t.Fatalf("failed to create .git dir: %v", err)
+	}
+
+	req := CreateRequest{
+		TaskID:             "task-a",
+		SessionID:          "sess-a",
+		TaskTitle:          "hang repro",
+		RepositoryPath:     repoPath,
+		BaseBranch:         "main",
+		PullBeforeWorktree: true,
+	}
+
+	// Short caller deadline exercises ctx propagation through branchExists
+	// and currentBranch. Before the fix these ignored ctx and slept the full
+	// 30s in the fake git script, holding repoLock the entire time.
+	ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond)
+	defer cancel()
+
+	start := time.Now()
+	_, err = mgr.Create(ctx, req)
+	elapsed := time.Since(start)
+	if err == nil {
+		t.Fatalf("Create() err = nil, want error on hanging git")
+	}
+	// Budget: inspect timeout + WaitDelay per subprocess kill, bounded by a
+	// few subprocess kills and any lock bookkeeping. Pre-fix this was ~30s.
+	if elapsed > 10*time.Second {
+		t.Fatalf("Create() took %v, want <10s (lock held during hung git)", elapsed)
+	}
+
+	// Confirm the repo lock was released: a second Create call must be able
+	// to acquire it without waiting. We use a fresh context so the call can
+	// run briefly, then cancel; all we care about is that Lock() didn't
+	// block us out.
+	lockCtx, lockCancel := context.WithTimeout(context.Background(), 500*time.Millisecond)
+	defer lockCancel()
+
+	lockStart := time.Now()
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		_, _ = mgr.Create(lockCtx, req)
+	}()
+	select {
+	case <-done:
+	case <-time.After(10 * time.Second):
+		t.Fatalf("second Create() stuck on repo lock (elapsed %v)", time.Since(lockStart))
+	}
+}