From 5ef69bde5eae7f0db27fc0008136f56242ecc8a0 Mon Sep 17 00:00:00 2001 From: Sneha Date: Thu, 20 Aug 2026 11:46:40 +1000 Subject: [PATCH] fix: Clarify error when GitHub PR merge refspec fetch fails refs/pull/N/merge only exists when GitHub can compute the speculative merge. When there's a real merge conflict the ref is never created and the fetch fails with a generic git error, which looks like transient flake rather than something the PR author needs to resolve. A-1711, A-1722 --- internal/job/checkout_fetch.go | 12 +++++++-- internal/job/checkout_mirror.go | 12 ++++++--- internal/job/checkout_mirror_remote_test.go | 30 +++++++++++++++++++++ internal/job/checkout_test.go | 5 ++++ 4 files changed, 53 insertions(+), 6 deletions(-) diff --git a/internal/job/checkout_fetch.go b/internal/job/checkout_fetch.go index ad55121a6d..523a4ec132 100644 --- a/internal/job/checkout_fetch.go +++ b/internal/job/checkout_fetch.go @@ -146,7 +146,7 @@ func (e *Executor) fetchSource(ctx context.Context, addBloblessFilter bool, atte Retry: kind == refspecGithubPRHead, RefSpecs: refspecs, }); err != nil { - return fmt.Errorf("fetching PR refspec %q: %w", refspecs, err) + return fmt.Errorf("fetching PR refspec %q: %w%s", refspecs, err, prMergeRefspecHint(kind == refspecGithubPRMerge)) } if kind == refspecGithubPRMerge && e.PullRequestHeadCommit != "" { if err := e.validateGithubPRMergeHead(ctx); err != nil { @@ -172,7 +172,7 @@ func (e *Executor) fetchSource(ctx context.Context, addBloblessFilter bool, atte refspecs = append(refspecs, e.Commit) // We aim to eliminate network round-trip as much as possible so we use a single git fetch here. if err := gitFetchWithFallback(ctx, e.shell, gitFetchFlags, refspecs...); err != nil { - return fmt.Errorf("fetching PR refspec %q: %w", refspecs, err) + return fmt.Errorf("fetching PR refspec %q: %w%s", refspecs, err, prMergeRefspecHint(kind == refspecGithubPRMerge)) } } @@ -257,6 +257,14 @@ func commitSecondParent(commit string) (string, bool) { return "", false } +// prMergeRefspecHint returns a clear suffix for refs/pull/N/merge fetch failures. +func prMergeRefspecHint(isMergeRefspec bool) string { + if !isMergeRefspec { + return "" + } + return "\nThis is possibly due to a merge conflict, or GitHub being unable to create the merge ref automatically" +} + func isExistingCheckoutRemoteMirrorAttempt(attempt *remoteMirrorAttempt) bool { return attempt != nil && attempt.site == remoteMirrorSiteExistingCheckout && diff --git a/internal/job/checkout_mirror.go b/internal/job/checkout_mirror.go index 4b2e6fb253..2233cf1548 100644 --- a/internal/job/checkout_mirror.go +++ b/internal/job/checkout_mirror.go @@ -323,7 +323,7 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem if isMainRepository && !commitAlreadyPresent && !remoteMirrorHit { var refspecs []string - var retry bool + var retry, isMergeRefspec bool switch { case e.RefSpec != "": @@ -331,15 +331,19 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem e.shell.Commentf("Fetching and mirroring custom refspec %s", e.RefSpec) refspecs = []string{e.RefSpec} case e.PullRequest != "false" && strings.Contains(e.PipelineProvider, "github"): - e.shell.Commentf("Fetching and mirroring pull request head from GitHub. This will be retried if it fails, as the pull request head might not be available yet — GitHub creates them asynchronously") var refspec string if e.PullRequestUsingMergeRefspec { + // As in fetchSource: a missing merge ref usually means a real + // merge conflict, so fail fast rather than retrying for ~2m. + e.shell.Commentf("Fetching and mirroring pull request merge commit from GitHub") refspec = fmt.Sprintf("refs/pull/%s/merge", e.PullRequest) + isMergeRefspec = true } else { + e.shell.Commentf("Fetching and mirroring pull request head from GitHub. This will be retried if it fails, as the pull request head might not be available yet — GitHub creates them asynchronously") refspec = fmt.Sprintf("refs/pull/%s/head", e.PullRequest) + retry = true } refspecs = []string{refspec} - retry = true default: // Fetch the build branch from the upstream repository into the mirror. refspecs = []string{e.Branch} @@ -355,7 +359,7 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem Retry: retry, }) }); err != nil { - return "", err + return "", fmt.Errorf("%w%s", err, prMergeRefspecHint(isMergeRefspec)) } } if !isMainRepository { diff --git a/internal/job/checkout_mirror_remote_test.go b/internal/job/checkout_mirror_remote_test.go index d2a380f2c2..61ef762fb0 100644 --- a/internal/job/checkout_mirror_remote_test.go +++ b/internal/job/checkout_mirror_remote_test.go @@ -538,6 +538,36 @@ func TestUpdateGitMirrorPullRequestHeadMissFallsBackToCanonical(t *testing.T) { } } +// TestUpdateGitMirrorMergeRefspecMissingHintsMergeConflict checks that a +// missing refs/pull/N/merge ref hits the same clarifying hint on the +// --git-mirrors-path update fetch as it does on the canonical checkout fetch. +func TestUpdateGitMirrorMergeRefspecMissingHintsMergeConflict(t *testing.T) { + canonical := newOnHostMirrorHTTPRepo(t, "canonical") + if _, _, err := canonical.PushBranch("canonical", "feature-branch"); err != nil { + t.Fatal(err) + } + + // A commit that doesn't exist anywhere, standing in for the speculative + // merge commit GitHub never created because of a conflict. + const missingMergeCommit = "0000000000000000000000000000000000000f" + e := newOnHostMirrorExecutor(t, canonical.RepoURL("canonical"), missingMergeCommit) + cloneOnHostMirrorToPath(t, e.Repository, expectedOnHostMirrorDir(e)) + e.PullRequest = "999" + e.PipelineProvider = "github" + e.PullRequestUsingMergeRefspec = true + + // refs/pull/999/merge is never created, so the mirror update fetch fails. + _, err := e.updateGitMirror(t.Context(), e.Repository, nil) + if err == nil { + t.Fatal("updateGitMirror() error = nil, want non-nil (missing merge ref)") + } + + const wantHint = "This is possibly due to a merge conflict, or GitHub being unable to create the merge ref automatically" + if !strings.Contains(err.Error(), wantHint) { + t.Fatalf("updateGitMirror() error = %q, want it to contain %q", err.Error(), wantHint) + } +} + func TestGetOrUpdateMirrorDirCloneLockTimeoutFallsBackWithoutMirror(t *testing.T) { canonical := newOnHostMirrorHTTPRepo(t, "canonical") commit, _, err := canonical.PushBranch("canonical", "feature-branch") diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index e225733f51..6c5d65733d 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -467,6 +467,11 @@ func TestDefaultCheckoutPhase_MergeRefspecFailsFast(t *testing.T) { if elapsed >= maxDuration { t.Fatalf("executor.defaultCheckoutPhase(ctx, 1) took %s, want < %s — merge refspec should not be retried", elapsed, maxDuration) } + + const wantHint = "This is possibly due to a merge conflict, or GitHub being unable to create the merge ref automatically" + if !strings.Contains(err.Error(), wantHint) { + t.Fatalf("executor.defaultCheckoutPhase(ctx, 1) error = %q, want it to contain %q", err.Error(), wantHint) + } } func TestSkipCheckout(t *testing.T) {