diff --git a/apps/backend/internal/worktree/manager.go b/apps/backend/internal/worktree/manager.go index a7aff481f2..0964b8f0cf 100644 --- a/apps/backend/internal/worktree/manager.go +++ b/apps/backend/internal/worktree/manager.go @@ -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/ 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 } diff --git a/apps/backend/internal/worktree/manager_timeout_test.go b/apps/backend/internal/worktree/manager_timeout_test.go new file mode 100644 index 0000000000..a646dce11a --- /dev/null +++ b/apps/backend/internal/worktree/manager_timeout_test.go @@ -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)) + } +}