From c140351cd6e57fe3d82fe292301ce77e4bc4bea2 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 18 Jun 2026 12:29:31 -0400 Subject: [PATCH] fix(#2432): retry merge on 409 after updating PR branch When MergeChangeProposal gets a 409 "Head branch is out of date", call GitHub's update-branch endpoint to sync the PR branch with the base, wait briefly, then retry the merge. Up to 3 attempts before giving up. Non-409 errors are still returned immediately. Closes #2432 Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/forge/github/github.go | 38 ++++++- internal/forge/github/github_merge_test.go | 121 +++++++++++++++++++++ 2 files changed, 154 insertions(+), 5 deletions(-) create mode 100644 internal/forge/github/github_merge_test.go diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index a01889f40..f90151197 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -2057,13 +2057,41 @@ func (c *LiveClient) DismissPullRequestReview(ctx context.Context, owner, repo s } // MergeChangeProposal squash-merges a pull request by number. +// If the merge fails with a 409 (head branch out of date), it updates the PR +// branch and retries up to 3 times with a short delay between attempts. func (c *LiveClient) MergeChangeProposal(ctx context.Context, owner, repo string, number int) error { - resp, err := c.put(ctx, fmt.Sprintf("/repos/%s/%s/pulls/%d/merge", owner, repo, number), map[string]string{"merge_method": "squash"}) - if err != nil { - return fmt.Errorf("merge pull request #%d: %w", number, err) + const maxAttempts = 3 + mergePath := fmt.Sprintf("/repos/%s/%s/pulls/%d/merge", owner, repo, number) + updatePath := fmt.Sprintf("/repos/%s/%s/pulls/%d/update-branch", owner, repo, number) + + for attempt := range maxAttempts { + resp, err := c.put(ctx, mergePath, map[string]string{"merge_method": "squash"}) + if err == nil { + resp.Body.Close() + return nil + } + + var apiErr *APIError + if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusConflict { + return fmt.Errorf("merge pull request #%d: %w", number, err) + } + + // Update the PR branch to incorporate base branch changes. + updateResp, updateErr := c.do(ctx, http.MethodPut, updatePath, map[string]string{}) + if updateErr == nil { + updateResp.Body.Close() + } + + if attempt < maxAttempts-1 { + select { + case <-time.After(3 * time.Second): + case <-ctx.Done(): + return ctx.Err() + } + } } - resp.Body.Close() - return nil + + return fmt.Errorf("merge pull request #%d: branch remained out of date after %d update-and-retry attempts", number, maxAttempts) } // UpdatePullRequestBranch updates a PR's head branch by merging the base diff --git a/internal/forge/github/github_merge_test.go b/internal/forge/github/github_merge_test.go new file mode 100644 index 000000000..cf4b6ea4e --- /dev/null +++ b/internal/forge/github/github_merge_test.go @@ -0,0 +1,121 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMergeChangeProposal_Success(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, http.MethodPut, r.Method) + assert.Equal(t, "/repos/org/repo/pulls/42/merge", r.URL.Path) + + var body map[string]string + json.NewDecoder(r.Body).Decode(&body) + assert.Equal(t, "squash", body["merge_method"]) + + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]string{"sha": "abc123"}) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.MergeChangeProposal(context.Background(), "org", "repo", 42) + require.NoError(t, err) +} + +func TestMergeChangeProposal_409UpdatesBranchAndRetries(t *testing.T) { + var mergeAttempts atomic.Int32 + var updateCalls atomic.Int32 + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPut && r.URL.Path == "/repos/org/repo/pulls/7/merge": + attempt := mergeAttempts.Add(1) + if attempt == 1 { + // First merge attempt: 409 conflict. + w.WriteHeader(http.StatusConflict) + json.NewEncoder(w).Encode(map[string]string{ + "message": "Head branch is out of date", + }) + return + } + // Second merge attempt: success. + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]string{"sha": "def456"}) + + case r.Method == http.MethodPut && r.URL.Path == "/repos/org/repo/pulls/7/update-branch": + updateCalls.Add(1) + w.WriteHeader(http.StatusAccepted) + json.NewEncoder(w).Encode(map[string]string{"message": "Updating pull request branch."}) + + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.MergeChangeProposal(context.Background(), "org", "repo", 7) + require.NoError(t, err) + assert.Equal(t, int32(2), mergeAttempts.Load(), "should have attempted merge twice") + assert.Equal(t, int32(1), updateCalls.Load(), "should have called update-branch once") +} + +func TestMergeChangeProposal_NonConflictErrorNotRetried(t *testing.T) { + var mergeAttempts atomic.Int32 + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mergeAttempts.Add(1) + w.WriteHeader(http.StatusUnprocessableEntity) + json.NewEncoder(w).Encode(map[string]string{ + "message": "Pull Request is not mergeable", + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.MergeChangeProposal(context.Background(), "org", "repo", 7) + require.Error(t, err) + assert.Contains(t, err.Error(), "not mergeable") + assert.Equal(t, int32(1), mergeAttempts.Load(), "should not retry non-409 errors") +} + +func TestMergeChangeProposal_409PersistsAfterRetries(t *testing.T) { + var mergeAttempts atomic.Int32 + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPut && r.URL.Path == "/repos/org/repo/pulls/7/merge": + mergeAttempts.Add(1) + w.WriteHeader(http.StatusConflict) + json.NewEncoder(w).Encode(map[string]string{ + "message": "Head branch is out of date", + }) + + case r.Method == http.MethodPut && r.URL.Path == "/repos/org/repo/pulls/7/update-branch": + w.WriteHeader(http.StatusAccepted) + json.NewEncoder(w).Encode(map[string]string{"message": "Updating pull request branch."}) + + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.MergeChangeProposal(context.Background(), "org", "repo", 7) + require.Error(t, err) + assert.Contains(t, err.Error(), "merge pull request #7") + // Should have tried multiple times before giving up. + assert.Greater(t, mergeAttempts.Load(), int32(1), "should have retried merge") +}