diff --git a/apps/backend/internal/worktree/manager_git.go b/apps/backend/internal/worktree/manager_git.go index 02eb6f68b2..7892bb486d 100644 --- a/apps/backend/internal/worktree/manager_git.go +++ b/apps/backend/internal/worktree/manager_git.go @@ -168,12 +168,6 @@ func (m *Manager) BranchRecoveryStatus(ctx context.Context, repoPath, branch str // wait, timeout, missing ref) returns false, and callers must treat that as // "cannot rule out loss": being slightly stale is recoverable, silently // dropping the user's unpushed commits is not. -// -// NOTE: PR #2654 adds an equivalent `isAncestor` helper on the -// pullCurrentBranchOrFallback path, with the arguments in the opposite order. -// The two are deliberately named differently so both can land in either order -// without a duplicate-symbol build failure; whichever merges second should -// collapse them into one helper. func (m *Manager) refContains(ctx context.Context, repoPath, container, contained string) bool { // Same Acquire-then-build-execCtx ordering as branchExists. release, err := subproc.AcquireGit(ctx, subproc.GitLifecycle) @@ -338,14 +332,27 @@ func (m *Manager) pullCurrentBranchOrFallback( output, err, execCtxErr := m.runGitCombinedAfterAcquire(ctx, m.pullTimeout, repoPath, "pull", "--ff-only", "origin", baseBranch) if err != nil { reason := classifyGitFallbackReason(err, string(output), execCtxErr) - m.logger.Warn("git pull failed before worktree creation; continuing with remote ref", + // The preceding fetch succeeded, so origin/ holds the latest + // remote state and is normally the better start point. It is only + // better when it is *ahead* of the local branch, though: if the local + // branch carries commits origin does not have, starting from the + // remote ref silently drops them from the new worktree, and creation + // still succeeds so nothing surfaces the loss. Keep the local branch + // in that case, matching handleFetchFallback and the non-current-branch + // path in resolveLocalBaseRef, which both resolve to it on failure. + fallbackRef := remoteRef + if !m.refContains(ctx, repoPath, remoteRef, baseBranch) { + fallbackRef = baseBranch + } + m.logger.Warn("git pull failed before worktree creation; continuing with fallback ref", zap.String("branch", baseBranch), zap.String("reason", reason), zap.String("remote_ref", remoteRef), + zap.String("fallback_ref", fallbackRef), zap.String("output", string(output)), zap.Error(err)) - m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Pull %s; using %s", reason, remoteRef), string(output)) - return remoteRef + m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Pull %s; using %s", reason, fallbackRef), string(output)) + return fallbackRef } m.reportSyncCompleted(stepName, onProgress, fmt.Sprintf("Synced and using %s", baseBranch), "") return baseBranch diff --git a/apps/backend/internal/worktree/manager_pull_fallback_test.go b/apps/backend/internal/worktree/manager_pull_fallback_test.go new file mode 100644 index 0000000000..2eef5dec43 --- /dev/null +++ b/apps/backend/internal/worktree/manager_pull_fallback_test.go @@ -0,0 +1,163 @@ +package worktree + +import ( + "context" + "path/filepath" + "strings" + "testing" +) + +// initGitRepoWithOriginAheadLocal builds the shape that matters here: a normal +// repository sitting on `main`, wired to a reachable bare `origin`, whose local +// `main` carries a commit that was never pushed. origin/main is therefore a +// strict ancestor of local main. +// +// Returns the working repository path and the local-only commit SHA. +func initGitRepoWithOriginAheadLocal(t *testing.T) (string, string) { + t.Helper() + + base := t.TempDir() + originPath := filepath.Join(base, "origin.git") + repoPath := filepath.Join(base, "repo") + + runGit(t, base, "init", "--bare", "-b", "main", originPath) + runGit(t, base, "init", "-b", "main", repoPath) + runGit(t, repoPath, "config", "user.email", "test@example.com") + runGit(t, repoPath, "config", "user.name", "Test User") + runGit(t, repoPath, "config", "commit.gpgsign", "false") + + writeRepoFile(t, repoPath, "README.md", "initial\n") + runGit(t, repoPath, "add", "README.md") + runGit(t, repoPath, "commit", "-m", "initial commit") + runGit(t, repoPath, "remote", "add", "origin", originPath) + runGit(t, repoPath, "push", "origin", "main") + + // The commit that only exists locally — the equivalent of a user's unpushed + // work on their base branch. + writeRepoFile(t, repoPath, "local-only.txt", "unpushed\n") + runGit(t, repoPath, "add", "local-only.txt") + runGit(t, repoPath, "commit", "-m", "local-only commit") + + localHead := strings.TrimSpace(runGit(t, repoPath, "rev-parse", "main")) + return repoPath, localHead +} + +// captureSyncProgress records every SyncProgressEvent so a test can assert +// which branch of the sync actually ran, not merely what it returned. +func captureSyncProgress(events *[]SyncProgressEvent) SyncProgressCallback { + return func(event SyncProgressEvent) { *events = append(*events, event) } +} + +// syncOutputs joins the captured progress output for assertion messages. +func syncOutputs(events []SyncProgressEvent) string { + parts := make([]string, 0, len(events)) + for _, e := range events { + parts = append(parts, e.Output) + } + return strings.Join(parts, " | ") +} + +// TestPullBaseBranch_PullFailureKeepsLocalCommits pins the behaviour that a +// transient `git pull` failure must not silently rebase the worktree's starting +// point onto origin/. +// +// When the pull fails for an environmental reason (throttle contention, a +// timeout, a stale index.lock) origin/ is not fresher than the local +// branch — here it is a strict ancestor of it. Returning the remote ref +// therefore discards the user's unpushed commits from the new worktree, and it +// does so silently: creation still succeeds, so nothing surfaces the loss. +// +// The two sibling fallbacks in manager_git.go (handleFetchFallback, and the +// non-current-branch path in resolveLocalBaseRef) already resolve to the local +// branch on failure. This one must agree with them. +func TestPullBaseBranch_PullFailureKeepsLocalCommits(t *testing.T) { + repoPath, localHead := initGitRepoWithOriginAheadLocal(t) + + mgr, err := NewManager(newTestConfig(t), newMockStore(), newTestLogger()) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + // Let the fetch succeed (so we reach the pull) but force the pull to fail + // the way contention makes it fail in a loaded run. A zero timeout yields + // an already-expired context, so the pull cannot accidentally succeed. + mgr.pullTimeout = 0 + + var events []SyncProgressEvent + resolved := mgr.pullBaseBranch(context.Background(), repoPath, "main", captureSyncProgress(&events)) + if resolved == "" { + t.Fatal("pullBaseBranch returned an empty ref") + } + + // Prove the failure path actually ran. Both the success and failure paths + // resolve to `main` here, so the SHA assertion below cannot distinguish + // them on its own — without this the test would pass vacuously if the pull + // ever succeeded. The failure path reports "Pull ; using ", + // the success path "Synced and using ". + outputs := syncOutputs(events) + if !strings.Contains(outputs, "Pull ") { + t.Fatalf( + "pull was expected to fail and take the fallback path, but sync progress was %q;\n"+ + "the assertion below would pass without exercising the fallback", + outputs, + ) + } + + // Assert on the commit the ref actually points at, not on the ref's name: + // the defect is that the worktree starts from the wrong commit, and a + // name-only assertion would still pass if the mapping changed. + resolvedSHA := strings.TrimSpace(runGit(t, repoPath, "rev-parse", resolved)) + if resolvedSHA != localHead { + t.Fatalf( + "pullBaseBranch resolved %q to %s, which drops the unpushed local commit %s;\n"+ + "a failed pull must not move the worktree base backwards onto origin/main", + resolved, resolvedSHA, localHead, + ) + } +} + +// TestPullBaseBranch_SuccessfulPullStillAdoptsRemoteCommits guards the happy +// path the fix must not regress: when the pull succeeds, the base branch is +// fast-forwarded and the worktree starts from the updated local branch, which +// now contains the remote commit. +func TestPullBaseBranch_SuccessfulPullStillAdoptsRemoteCommits(t *testing.T) { + base := t.TempDir() + originPath := filepath.Join(base, "origin.git") + repoPath := filepath.Join(base, "repo") + otherPath := filepath.Join(base, "other") + + runGit(t, base, "init", "--bare", "-b", "main", originPath) + runGit(t, base, "init", "-b", "main", repoPath) + runGit(t, repoPath, "config", "user.email", "test@example.com") + runGit(t, repoPath, "config", "user.name", "Test User") + runGit(t, repoPath, "config", "commit.gpgsign", "false") + writeRepoFile(t, repoPath, "README.md", "initial\n") + runGit(t, repoPath, "add", "README.md") + runGit(t, repoPath, "commit", "-m", "initial commit") + runGit(t, repoPath, "remote", "add", "origin", originPath) + runGit(t, repoPath, "push", "origin", "main") + + // A second clone pushes a commit the working repo has not seen yet. + runGit(t, base, "clone", originPath, otherPath) + runGit(t, otherPath, "config", "user.email", "other@example.com") + runGit(t, otherPath, "config", "user.name", "Other User") + runGit(t, otherPath, "config", "commit.gpgsign", "false") + writeRepoFile(t, otherPath, "remote-only.txt", "pushed\n") + runGit(t, otherPath, "add", "remote-only.txt") + runGit(t, otherPath, "commit", "-m", "remote-only commit") + runGit(t, otherPath, "push", "origin", "main") + remoteHead := strings.TrimSpace(runGit(t, otherPath, "rev-parse", "HEAD")) + + mgr, err := NewManager(newTestConfig(t), newMockStore(), newTestLogger()) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + + resolved := mgr.pullBaseBranch(context.Background(), repoPath, "main", nil) + resolvedSHA := strings.TrimSpace(runGit(t, repoPath, "rev-parse", resolved)) + if resolvedSHA != remoteHead { + t.Fatalf( + "pullBaseBranch resolved %q to %s, want the pulled remote commit %s", + resolved, resolvedSHA, remoteHead, + ) + } +}