Squiddy-merge the PR, not the branch

We've recently started seeing occasional weird merges with squiddy,
where the branch itself is merged, but github considers the PR "closed"
and having zero commits. (e.g.
https://github.com/futurelearn/futurelearn/pull/14124)

This action performs the following steps:

* we first rebase the PR branch using cirrus-actions/rebase
* we then merge the branch (NB: not the PR!) using the github API
* we delete the branch unless the repo is configured to do this
  automatically

None of these operations are directly on the PR, meaning that Github has
to detect any relevant changes to the PR and update its state. This
happens asynchronously, and is therefore prone to race conditions.

This commit tries to eliminate the possibility of race conditions by:

* Waiting for the PR head to match the branch head (in other words, it
  gives Github a chance to catch up following the rebase)
* Issuing the merge command on the PR itself, not the branch

Note that previously we were using the `merge_method: 'rebase' param in
the #merge call; this was a mistake, as we actually want to create a
merge commit, but as it happens calling merge on a branch doesn't
support the merge_method option, so we got a merge commit anyway.

We're now using the #merge_pull_request method, which *does* support
that option, and we want to create a merge commit, so the strategy is
set to 'merge', not 'rebase'.

actions/bubble_merge/git_client.rb

@@ -3,7 +3,7 @@
 
 module Squiddy
   class GitClient
-    attr_reader :client, :event, :pr, :repo
+    attr_reader :client, :event, :pr
 
     CHECK_STATUSES = %w[queued in_progress completed].freeze
     CHECK_CONCLUSIONS = %w[success failure neutral cancelled skipped timed_out action_required stale].freeze
@@ -16,25 +16,56 @@ def initialize
       @client = Octokit::Client.new(access_token: ENV["GITHUB_TOKEN"])
       @event = git_event
       @pr = pull_request
-      @repo = repository
     end
 
     def bubble_merge
       if already_merged?
         client.add_comment(repo_name, pr_number, 'This PR is already merged.')
       else
-        merge_and_close_pr
+        wait_for_pr_to_match_branch do |merge_sha|
+          merge_and_close_pr(merge_sha)
+        end
       end
     end
 
     private
 
+    def wait_for_pr_to_match_branch
+      retry_sleeps = [1, 2, 3, 5, 8]
+
+      loop do
+        expected = branch_head_sha
+        actual = pull_request_head_sha
+
+        if actual == expected
+          puts "PR HEAD #{actual[0..10]} matches #{branch} HEAD #{expected[0..10]}; merging..."
+          yield expected
+          return
+        elsif s = retry_sleeps.shift
+          puts "PR HEAD #{actual[0..10]} did not match #{branch} HEAD #{expected[0..10]}; waiting for #{s}s..."
+          sleep s
+          next
+        else
+          raise "PR HEAD #{actual[0..10]} did not match #{branch} HEAD #{expected[0..10]}; exiting"
+        end
+      end
+    end
+
     def already_merged?
       client.pull_merged?(repo_name, pr_number)
     end
 
-    def merge_and_close_pr
-      merge
+    def merge_and_close_pr(merge_sha)
+      puts "PR ##{pr_number}: merging #{branch}@#{merge_sha} into #{base_branch}"
+      client.merge_pull_request(
+        repo_name,
+        pr_number,
+        commit_message,
+        {
+          merge_method: 'merge',
+          sha: merge_sha
+        }
+      )
       delete_branch
     rescue StandardError => e
       message = <<~MESSAGE
@@ -46,29 +77,13 @@ def merge_and_close_pr
       client.add_comment(repo_name, pr_number, message)
     end
 
-    def merge
-      client.merge(
-        repo_name,
-        base_branch,
-        branch,
-        {
-          merge_method: 'rebase',
-          commit_message: commit_message,
-          sha: main_sha
-        }
-      )
-    end
-
     def delete_branch
-      return if repo.delete_branch_on_merge
+      return if repository.delete_branch_on_merge
 
+      puts "PR ##{pr_number}: deleting #{branch}"
       client.delete_branch(repo_name, branch)
     end
 
-    def main_sha
-      client.list_commits(repo_name).last.sha
-    end
-
     def branch
       pr.head.ref
     end
@@ -81,8 +96,12 @@ def pr_number
       event.dig('issue', 'number')
     end
 
-    def head_commit_sha
-      client.pull_request_commits(repo_name, pr_number).last.sha
+    def branch_head_sha
+      client.branch(repo_name, branch).commit.sha
+    end
+
+    def pull_request_head_sha
+      client.pull_request(repo_name, pr_number).head.sha
     end
 
     def comment_author

spec/actions/bubble_merge/git_client_spec.rb

@@ -24,30 +24,21 @@
   let(:pull_request) {
     Hashie::Mash.new({
       base: { ref: 'test-base-branch' },
-      head: { ref: 'test-branch' }
+      head: { ref: 'test-branch', sha: '1234' }
     })
   }
-  let(:list_commits) {
-    [
-      Hashie::Mash.new({ sha: 'abcd' }),
-      Hashie::Mash.new({ sha: '1234' })
-    ]
-  }
-  let(:pull_request_commits) {
-    [
-      Hashie::Mash.new({ sha: 'febe' }),
-      Hashie::Mash.new({ sha: '5678' })
-    ]
+  let(:branch) {
+    Hashie::Mash.new({
+      commit: { sha: '1234' },
+    })
   }
   let(:pull_merged?) { false }
   let(:octokit_client) {
     instance_double(
       'Octokit::Client',
-      merge: nil,
+      merge_pull_request: nil,
       delete_branch: nil,
       add_comment: nil,
-      list_commits: list_commits,
-      pull_request_commits: pull_request_commits,
       pull_merged?: pull_merged?
     )
   }
@@ -57,10 +48,20 @@
     stub_const('ENV', 'GITHUB_EVENT' => event, 'GITHUB_TOKEN' => 'token')
     allow(Octokit::Client).to receive(:new).and_return(octokit_client)
     allow(octokit_client).to receive(:pull_request).with('test-user/test-repo', 'test-pr-number').and_return(pull_request)
-    allow(octokit_client).to receive(:pull_merged?).with('test-user/test-repo', 'test-pr-number').and_return(false)
+    allow(octokit_client).to receive(:branch).with('test-user/test-repo', 'test-branch').and_return(branch)
     allow(octokit_client).to receive(:repository).with('test-user/test-repo').and_return(repository)
   end
 
+  around do |example|
+    begin
+      old_stdout = $stdout
+      $stdout = StringIO.new(String.new, 'w')
+      example.call
+    ensure
+      $stdout = old_stdout
+    end
+  end
+
   context '#bubble_merge' do
     let(:commit_message) {
       <<~MESSAGE
@@ -78,13 +79,12 @@
     }
 
     it 'merges successfully' do
-      expect(octokit_client).to receive(:merge).with(
+      expect(octokit_client).to receive(:merge_pull_request).with(
         'test-user/test-repo',
-        'test-base-branch',
-        'test-branch',
+        'test-pr-number',
+        commit_message,
         {
-          commit_message: commit_message,
-          merge_method: 'rebase',
+          merge_method: 'merge',
           sha: '1234'
         }
       )
@@ -107,7 +107,7 @@
       }
 
       before do
-        allow(octokit_client).to receive(:merge).and_raise(Octokit::Conflict.new)
+        allow(octokit_client).to receive(:merge_pull_request).and_raise(Octokit::Conflict.new)
       end
 
       it 'prints an error message' do
@@ -127,7 +127,7 @@
       end
 
       it 'does not attempt to merge again' do
-        expect(octokit_client).not_to receive(:merge)
+        expect(octokit_client).not_to receive(:merge_pull_request)
         subject.bubble_merge
       end
 
@@ -137,6 +137,42 @@
       end
     end
 
+    context 'when the PR branch and PR are out of sync' do
+      let(:pull_request_head) { double(:pull_request_head, ref: 'test-branch') }
+      let(:pull_request) {
+        Hashie::Mash.new({
+          base: { ref: 'test-base-branch' },
+          head: pull_request_head,
+        })
+      }
+      let(:branch) {
+        Hashie::Mash.new({
+          commit: { sha: '1234' },
+        })
+      }
+
+      before do
+        allow(subject).to receive(:sleep)
+      end
+
+      it 'waits 5 times and then fails' do
+        allow(pull_request_head).to receive(:sha).and_return('5678')
+
+        expect {
+          subject.bubble_merge
+        }.to raise_error(/PR HEAD 5678 did not match test-branch HEAD 1234/)
+        expect(subject).to have_received(:sleep).exactly(5).times
+      end
+
+      it 'recovers if the PR updates to match the branch' do
+        allow(pull_request_head).to receive(:sha).and_return('5678', '1234')
+
+        subject.bubble_merge
+
+        expect(subject).to have_received(:sleep).once
+      end
+    end
+
     context 'when the repository auto deletes branches on merge' do
       let(:repository) { double(delete_branch_on_merge: true) }