Repository: palantir/bulldozer (go)

Issue:
PRs left open when GitHub is too busy
We have been experiencing an intermittent issue where merge requests are rejected by the GitHub API:

```json
{
	"level": "info",
	"rid": "cf3n6dbn37kclfh117a0",
	"github_event_type": "status",
	"github_delivery_id": "3cb85cd2-96e7-11ed-9516-9f680ab3bc27",
	"github_installation_id": 1,
	"github_repository_owner": "acme",
	"github_repository_name": "anvils",
	"github_pr_num": 26725,
	"time": "2023-01-18T04:19:02.713727629Z",
	"message": "Merge rejected due to unsatisfied condition: \"Base branch was modified. Review and try the merge again.\""
}
```

A small percentage of PRs are left unmerged by bulldozer due to the above issue. At this point there are no more triggering events affecting the PR so it never retries. This seems to happen when we have a lot of PRs getting merged in quick succession and is sporadic in nature.

I am happy to contribute a fix for this, but I am not sure what the best approach might be:

1. Retry after a small delay? In the event bulldozer is restarted the PR would once again end up left open, but this would be exceedingly rare and so is acceptable.
2. Periodically scan PRs in a repo? This feels like a can of worms best not opened.

Or if you have any other ideas I would be grateful.

Pull request description:
Retry merge when github returns 405 with the base branch modified message
Resolves #369

Patch:
diff --git a/bulldozer/merge.go b/bulldozer/merge.go
index fc789e3..99d92ae 100644
--- a/bulldozer/merge.go
+++ b/bulldozer/merge.go
@@ -289,6 +289,10 @@ func attemptMerge(ctx context.Context, pullCtx pull.Context, merger Merger, meth
 
 		switch gerr.Response.StatusCode {
 		case http.StatusMethodNotAllowed:
+			if gerr.Message == "Base branch was modified. Review and try the merge again." {
+				logger.Info().Msg("Base branch was modified, retrying")
+				return false, true
+			}
 			logger.Info().Msgf("Merge rejected due to unsatisfied condition: %q", gerr.Message)
 			return false, false
 		case http.StatusConflict:


Test patch:
diff --git a/bulldozer/merge_test.go b/bulldozer/merge_test.go
index 2d8562b..e07973e 100644
--- a/bulldozer/merge_test.go
+++ b/bulldozer/merge_test.go
@@ -15,9 +15,14 @@
 package bulldozer
 
 import (
+	"bytes"
 	"context"
+	"fmt"
+	"io"
+	"net/http"
 	"testing"
 
+	"github.com/google/go-github/v47/github"
 	"github.com/palantir/bulldozer/pull"
 	"github.com/palantir/bulldozer/pull/pulltest"
 	"github.com/stretchr/testify/assert"
@@ -181,3 +186,21 @@ func TestPushRestrictionMerger(t *testing.T) {
 	assert.Equal(t, 1, normal.DeleteCount, "normal delete was incorrectly called")
 	assert.Equal(t, 1, restricted.DeleteCount, "restricted delete was not called")
 }
+
+func TestBaseBranchChangedRetry(t *testing.T) {
+	merger := &MockMerger{
+		MergeError: github.CheckResponse(
+			&http.Response{
+				StatusCode: http.StatusMethodNotAllowed,
+				Body: io.NopCloser(
+					bytes.NewReader([]byte(
+						fmt.Sprintf(`{"message": "%s"}`, "Base branch was modified. Review and try the merge again.")))),
+			},
+		),
+	}
+	ctx := context.Background()
+	pullCtx := &pulltest.MockPullContext{MergeStateValue: &pull.MergeState{Closed: false, Mergeable: boolVal(true)}}
+
+	_, retry := attemptMerge(ctx, pullCtx, merger, SquashAndMerge, CommitMessage{})
+	assert.True(t, retry, "should retry on base branch changed error")
+}