Merge pull request #1417 from Shopify/retry-github-sync-job

Retry GithubSyncJob if new commit sha is missing in Github response on push from target branch

Author: Shinomix

app/jobs/shipit/github_sync_job.rb

@@ -7,6 +7,8 @@ class GithubSyncJob < BackgroundJob
     attr_reader :stack
 
     MAX_FETCHED_COMMITS = 25
+    MAX_RETRY_ATTEMPTS = 5
+    RETRY_DELAY = 5.seconds
     queue_as :default
     on_duplicate :drop
 
@@ -15,10 +17,19 @@ class GithubSyncJob < BackgroundJob
 
     def perform(params)
       @stack = Stack.find(params[:stack_id])
+      expected_head_sha = params[:expected_head_sha]
+      retry_count = params[:retry_count] || 0
 
       handle_github_errors do
         new_commits, shared_parent = fetch_missing_commits { stack.github_commits }
 
+        # Retry on Github eventual consistency: webhook indicated new commits but we found none
+        if expected_head_sha && new_commits.empty? && !commit_exists?(expected_head_sha) &&
+           retry_count < MAX_RETRY_ATTEMPTS
+          GithubSyncJob.set(wait: RETRY_DELAY * retry_count).perform_later(params.merge(retry_count: retry_count + 1))
+          return
+        end
+
         stack.transaction do
           shared_parent&.detach_children!
           appended_commits = new_commits.map do |gh_commit|
@@ -63,5 +74,9 @@ def handle_github_errors
     def lookup_commit(sha)
       stack.commits.find_by(sha:)
     end
+
+    def commit_exists?(sha)
+      stack.commits.exists?(sha:)
+    end
   end
 end

app/models/shipit/stack.rb

@@ -602,8 +602,8 @@ def recent_deploys_durations
       tasks.where(type: 'Shipit::Deploy').success.order(id: :desc).limit(100).durations
     end
 
-    def sync_github
-      GithubSyncJob.perform_later(stack_id: id)
+    def sync_github(expected_head_sha: nil)
+      GithubSyncJob.perform_later(stack_id: id, expected_head_sha:)
     end
 
     def links

app/models/shipit/webhooks/handlers/push_handler.rb

@@ -6,12 +6,14 @@ module Handlers
       class PushHandler < Handler
         params do
           requires :ref
+          requires :after
         end
+
         def process
           stacks
             .not_archived
             .where(branch:)
-            .find_each(&:sync_github)
+            .find_each { |stack| stack.sync_github(expected_head_sha: params.after) }
         end
 
         private

test/controllers/webhooks_controller_test.rb

@@ -23,9 +23,11 @@ class WebhooksControllerTest < ActionController::TestCase
     test ":push with the target branch queues a GithubSyncJob" do
       request.headers['X-Github-Event'] = 'push'
 
-      body = JSON.parse(payload(:push_master)).to_json
-      assert_enqueued_with(job: GithubSyncJob, args: [stack_id: @stack.id]) do
-        post :create, body:, as: :json
+      parsed_body = JSON.parse(payload(:push_master))
+      expected_head_sha = parsed_body["after"]
+
+      assert_enqueued_with(job: GithubSyncJob, args: [stack_id: @stack.id, expected_head_sha:]) do
+        post :create, body: parsed_body.to_json, as: :json
       end
     end
 

test/jobs/github_sync_job_test.rb

@@ -118,5 +118,50 @@ class GithubSyncJobTest < ActiveSupport::TestCase
 
       assert_equal true, @stack.reload.inaccessible_since?
     end
+
+    test "#perform retries when expected_head_sha is not found in the github response" do
+      expected_sha = "abcd1234"
+      Stack.any_instance.expects(:github_commits).returns(@github_commits)
+      @job.expects(:fetch_missing_commits).yields.returns([[], nil])
+      @job.expects(:commit_exists?).with(expected_sha).returns(false)
+
+      assert_enqueued_with(job: GithubSyncJob, args: [stack_id: @stack.id, expected_head_sha: expected_sha, retry_count: 1]) do
+        @job.perform(stack_id: @stack.id, expected_head_sha: expected_sha)
+      end
+    end
+
+    test "#perform stops retrying after MAX_RETRY_ATTEMPTS" do
+      expected_sha = "abcd1234"
+      Stack.any_instance.expects(:github_commits).returns(@github_commits)
+      @job.expects(:fetch_missing_commits).yields.returns([[], nil])
+      @job.expects(:commit_exists?).with(expected_sha).returns(false)
+
+      assert_no_enqueued_jobs(only: GithubSyncJob) do
+        @job.perform(stack_id: @stack.id, expected_head_sha: expected_sha, retry_count: GithubSyncJob::MAX_RETRY_ATTEMPTS)
+      end
+    end
+
+    test "#perform processes normally when expected_head_sha exists" do
+      expected_sha = "abcd1234"
+      Stack.any_instance.expects(:github_commits).returns(@github_commits)
+      @job.expects(:fetch_missing_commits).yields.returns([[], nil])
+      @job.expects(:commit_exists?).with(expected_sha).returns(true)
+
+      assert_enqueued_with(job: CacheDeploySpecJob, args: [@stack]) do
+        @job.perform(stack_id: @stack.id, expected_head_sha: expected_sha)
+      end
+    end
+
+    test "#perform processes normally when new commits are found" do
+      expected_sha = "abcd1234"
+      new_commit = stub(sha: expected_sha)
+      Stack.any_instance.expects(:github_commits).returns(@github_commits)
+      @job.expects(:fetch_missing_commits).yields.returns([[new_commit], nil])
+      @job.expects(:append_commit).with(new_commit).returns(stub(revert?: false))
+
+      assert_enqueued_with(job: CacheDeploySpecJob, args: [@stack]) do
+        @job.perform(stack_id: @stack.id, expected_head_sha: expected_sha)
+      end
+    end
   end
 end

test/models/shipit/review_stack_test.rb

@@ -70,7 +70,7 @@ class ReviewStackTest < ActiveSupport::TestCase
         stack.archive!(shipit_users(:codertocat))
       end
 
-      assert_enqueued_with(job: GithubSyncJob, args: [stack_id: stack.id]) do
+      assert_enqueued_with(job: GithubSyncJob, args: [stack_id: stack.id, expected_head_sha: nil]) do
         stack.unarchive!
       end
     end

test/models/shipit/stack_test.rb

@@ -997,13 +997,13 @@ def self.deliver(event, stack, payload)
         @stack.archive!(shipit_users(:codertocat))
       end
 
-      assert_enqueued_with(job: GithubSyncJob, args: [stack_id: @stack.id]) do
+      assert_enqueued_with(job: GithubSyncJob, args: [stack_id: @stack.id, expected_head_sha: nil]) do
         @stack.unarchive!
       end
     end
 
     test "#update that changes the branch name triggers a GithubSync job" do
-      assert_enqueued_with(job: GithubSyncJob, args: [stack_id: @stack.id]) do
+      assert_enqueued_with(job: GithubSyncJob, args: [stack_id: @stack.id, expected_head_sha: nil]) do
         @stack.update!(branch: 'test')
       end
     end

test/models/shipit/webhooks/handlers/pull_request/review_stack_adapter_test.rb

@@ -26,7 +26,7 @@ class ReviewStackAdapterTest < ActiveSupport::TestCase
               scope: stack.repository.stacks
             )
 
-            assert_enqueued_with(job: GithubSyncJob, args: [stack_id: stack.id]) do
+            assert_enqueued_with(job: GithubSyncJob, args: [stack_id: stack.id, expected_head_sha: nil]) do
               review_stack.unarchive!
             end
           end