diff --git a/apps/backend/internal/worktree/manager.go b/apps/backend/internal/worktree/manager.go index ffa952c21e..e80ad636bc 100644 --- a/apps/backend/internal/worktree/manager.go +++ b/apps/backend/internal/worktree/manager.go @@ -2,6 +2,7 @@ package worktree import ( "context" + "errors" "fmt" "os" "os/exec" @@ -16,6 +17,11 @@ import ( "github.com/kandev/kandev/internal/common/logger" ) +const ( + defaultGitFetchTimeout = 8 * time.Second + defaultGitPullTimeout = 8 * time.Second +) + // repoLockEntry tracks a repository lock and its reference count. type repoLockEntry struct { mu *sync.Mutex @@ -35,6 +41,10 @@ type Manager struct { // Optional dependencies for script execution repoProvider RepositoryProvider scriptMsgHandler ScriptMessageHandler + + // Timeouts for best-effort remote sync before creating a worktree. + fetchTimeout time.Duration + pullTimeout time.Duration } // ScriptMessageHandler provides script execution and message streaming. @@ -83,11 +93,13 @@ 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), + config: cfg, + logger: log.WithFields(zap.String("component", "worktree-manager")), + store: store, + worktrees: make(map[string]*Worktree), + repoLocks: make(map[string]*repoLockEntry), + fetchTimeout: defaultGitFetchTimeout, + pullTimeout: defaultGitPullTimeout, }, nil } @@ -724,6 +736,40 @@ func (m *Manager) currentBranch(repoPath string) string { return strings.TrimSpace(string(output)) } +func (m *Manager) newNonInteractiveGitCmd(ctx context.Context, repoPath string, args ...string) *exec.Cmd { + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = repoPath + cmd.Env = append(os.Environ(), + "GIT_TERMINAL_PROMPT=0", + "GCM_INTERACTIVE=Never", + "GIT_ASKPASS=echo", + "SSH_ASKPASS=/bin/false", + "GIT_SSH_COMMAND=ssh -oBatchMode=yes", + ) + // After the context cancels and the process is killed, child processes + // (e.g. credential helpers) may still hold stdout/stderr pipes open. + // WaitDelay bounds how long CombinedOutput waits for those pipes to close. + cmd.WaitDelay = 500 * time.Millisecond + return cmd +} + +func classifyGitFallbackReason(cmdErr error, cmdOutput string, ctxErr error) string { + if errors.Is(ctxErr, context.DeadlineExceeded) || errors.Is(cmdErr, context.DeadlineExceeded) { + return "timeout" + } + + out := strings.ToLower(cmdOutput) + if strings.Contains(out, "authentication failed") || + strings.Contains(out, "terminal prompts disabled") || + strings.Contains(out, "could not read username") || + strings.Contains(out, "username for 'https://") || + strings.Contains(out, "askpass") { + return "non_interactive_auth_failed" + } + + return "git_command_failed" +} + // pullBaseBranch fetches the latest changes from origin and returns the best ref to use // for creating a new worktree. The function handles three scenarios: // @@ -737,19 +783,20 @@ func (m *Manager) pullBaseBranch(repoPath, baseBranch string) string { localBranch := strings.TrimPrefix(baseBranch, "origin/") isRemoteRef := localBranch != baseBranch - // Fetch the branch from origin (30s timeout for fetch + potential pull) - fetchCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() + // Fetch branch from origin in non-interactive mode. + fetchCtx, cancelFetch := context.WithTimeout(context.Background(), m.fetchTimeout) + defer cancelFetch() fetchArgs := []string{"fetch", "origin"} if localBranch != "" { fetchArgs = append(fetchArgs, localBranch) } - fetchCmd := exec.CommandContext(fetchCtx, "git", fetchArgs...) - fetchCmd.Dir = repoPath + fetchCmd := m.newNonInteractiveGitCmd(fetchCtx, repoPath, fetchArgs...) if output, err := fetchCmd.CombinedOutput(); err != nil { - m.logger.Error("git fetch failed before worktree creation", + m.logger.Warn("git fetch failed before worktree creation; continuing with fallback ref", zap.String("branch", baseBranch), + zap.String("reason", classifyGitFallbackReason(err, string(output), fetchCtx.Err())), + zap.String("fallback_ref", baseBranch), zap.String("output", string(output)), zap.Error(err)) return baseBranch @@ -766,11 +813,14 @@ func (m *Manager) pullBaseBranch(repoPath, baseBranch string) string { if currentBranch == baseBranch { // We're on the target branch - try to pull (fast-forward only) - pullCmd := exec.CommandContext(fetchCtx, "git", "pull", "--ff-only", "origin", baseBranch) - pullCmd.Dir = repoPath + pullCtx, cancelPull := context.WithTimeout(context.Background(), m.pullTimeout) + defer cancelPull() + + pullCmd := m.newNonInteractiveGitCmd(pullCtx, repoPath, "pull", "--ff-only", "origin", baseBranch) if output, err := pullCmd.CombinedOutput(); err != nil { - m.logger.Warn("git pull failed before worktree creation, falling back to remote ref", + m.logger.Warn("git pull failed before worktree creation; continuing with remote ref", zap.String("branch", baseBranch), + zap.String("reason", classifyGitFallbackReason(err, string(output), pullCtx.Err())), zap.String("remote_ref", remoteRef), zap.String("output", string(output)), zap.Error(err)) diff --git a/apps/backend/internal/worktree/manager_test.go b/apps/backend/internal/worktree/manager_test.go index 10ef70c23a..8e19bf0c85 100644 --- a/apps/backend/internal/worktree/manager_test.go +++ b/apps/backend/internal/worktree/manager_test.go @@ -5,7 +5,9 @@ import ( "os" "path/filepath" "regexp" + "strings" "testing" + "time" "github.com/kandev/kandev/internal/common/logger" ) @@ -596,3 +598,172 @@ func TestRepoLocks_ReleaseNonexistent(t *testing.T) { t.Errorf("expected 0 locks in map, got %d", count) } } + +func writeFakeGitScript(t *testing.T, scriptBody string) string { + t.Helper() + + scriptDir := t.TempDir() + scriptPath := filepath.Join(scriptDir, "git") + content := "#!/bin/sh\nset -eu\n\n" + scriptBody + "\n" + if err := os.WriteFile(scriptPath, []byte(content), 0755); err != nil { + t.Fatalf("failed to write fake git script: %v", err) + } + return scriptDir +} + +func TestPullBaseBranch_UsesNonInteractiveGitEnv(t *testing.T) { + logPath := filepath.Join(t.TempDir(), "git-env.log") + scriptDir := writeFakeGitScript(t, ` +case "${1:-}" in + fetch) + printf "%s|%s|%s|%s|%s" \ + "${GIT_TERMINAL_PROMPT:-}" \ + "${GCM_INTERACTIVE:-}" \ + "${GIT_ASKPASS:-}" \ + "${SSH_ASKPASS:-}" \ + "${GIT_SSH_COMMAND:-}" > "${KD_GIT_ENV_LOG:?}" + exit 0 + ;; + rev-parse) + if [ "${2:-}" = "--abbrev-ref" ]; then + echo "master" + fi + exit 0 + ;; + *) + exit 0 + ;; +esac +`) + + t.Setenv("KD_GIT_ENV_LOG", logPath) + t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + cfg := newTestConfig(t) + log := newTestLogger() + store := newMockStore() + mgr, err := NewManager(cfg, store, log) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + + repoPath := t.TempDir() + ref := mgr.pullBaseBranch(repoPath, "origin/master") + if ref != "origin/master" { + t.Fatalf("pullBaseBranch() ref = %q, want %q", ref, "origin/master") + } + + envBytes, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("failed reading fake git env log: %v", err) + } + + got := string(envBytes) + want := "0|Never|echo|/bin/false|ssh -oBatchMode=yes" + if got != want { + t.Fatalf("fake git env = %q, want %q", got, want) + } +} + +func TestPullBaseBranch_FetchTimeoutFallsBackQuickly(t *testing.T) { + scriptDir := writeFakeGitScript(t, ` +case "${1:-}" in + fetch) + sleep 2 + exit 0 + ;; + rev-parse) + if [ "${2:-}" = "--abbrev-ref" ]; then + echo "master" + fi + exit 0 + ;; + *) + exit 0 + ;; +esac +`) + + t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + cfg := newTestConfig(t) + log := newTestLogger() + store := newMockStore() + mgr, err := NewManager(cfg, store, log) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + mgr.fetchTimeout = 100 * time.Millisecond + + repoPath := t.TempDir() + start := time.Now() + ref := mgr.pullBaseBranch(repoPath, "master") + elapsed := time.Since(start) + + if ref != "master" { + t.Fatalf("pullBaseBranch() ref = %q, want %q", ref, "master") + } + // Allow CI scheduling variance while still asserting we timed out + // well before the fake 2s fetch command completes. + if elapsed > 1500*time.Millisecond { + t.Fatalf("pullBaseBranch() took too long: %v", elapsed) + } +} + +func TestPullBaseBranch_PullFailureFallsBackToRemoteRef(t *testing.T) { + scriptDir := writeFakeGitScript(t, ` +case "${1:-}" in + fetch) + exit 0 + ;; + pull) + echo "Authentication failed" 1>&2 + exit 1 + ;; + rev-parse) + if [ "${2:-}" = "--abbrev-ref" ]; then + echo "master" + exit 0 + fi + if [ "${2:-}" = "--verify" ]; then + exit 0 + fi + exit 0 + ;; + *) + exit 0 + ;; +esac +`) + + t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + cfg := newTestConfig(t) + log := newTestLogger() + store := newMockStore() + mgr, err := NewManager(cfg, store, log) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + mgr.pullTimeout = 300 * time.Millisecond + + repoPath := t.TempDir() + ref := mgr.pullBaseBranch(repoPath, "master") + if ref != "origin/master" { + t.Fatalf("pullBaseBranch() ref = %q, want %q", ref, "origin/master") + } +} + +func TestClassifyGitFallbackReason_AuthPrompt(t *testing.T) { + reason := classifyGitFallbackReason(nil, "fatal: could not read Username for 'https://github.com'", nil) + if reason != "non_interactive_auth_failed" { + t.Fatalf("classifyGitFallbackReason() = %q, want %q", reason, "non_interactive_auth_failed") + } +} + +func TestClassifyGitFallbackReason_Timeout(t *testing.T) { + reason := classifyGitFallbackReason(context.DeadlineExceeded, "", context.DeadlineExceeded) + if !strings.EqualFold(reason, "timeout") { + t.Fatalf("classifyGitFallbackReason() = %q, want %q", reason, "timeout") + } +} diff --git a/apps/web/components/diff/diff-viewer.tsx b/apps/web/components/diff/diff-viewer.tsx index 5535246b96..7bc91b2bac 100644 --- a/apps/web/components/diff/diff-viewer.tsx +++ b/apps/web/components/diff/diff-viewer.tsx @@ -114,7 +114,7 @@ export const DiffViewer = memo(function DiffViewer({ return (