diff --git a/app/jobs/shipit/github_sync_job.rb b/app/jobs/shipit/github_sync_job.rb index 6dd7ecc5a..8897e28d9 100644 --- a/app/jobs/shipit/github_sync_job.rb +++ b/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 diff --git a/app/models/shipit/stack.rb b/app/models/shipit/stack.rb index 6731d8775..938bb60aa 100644 --- a/app/models/shipit/stack.rb +++ b/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 diff --git a/app/models/shipit/webhooks/handlers/push_handler.rb b/app/models/shipit/webhooks/handlers/push_handler.rb index 7464dbff1..1cb3a3694 100644 --- a/app/models/shipit/webhooks/handlers/push_handler.rb +++ b/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 diff --git a/test/controllers/webhooks_controller_test.rb b/test/controllers/webhooks_controller_test.rb index 6d8598152..2e566cbb8 100644 --- a/test/controllers/webhooks_controller_test.rb +++ b/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 diff --git a/test/jobs/github_sync_job_test.rb b/test/jobs/github_sync_job_test.rb index 6b6af349c..e60ee16b3 100644 --- a/test/jobs/github_sync_job_test.rb +++ b/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 diff --git a/test/models/shipit/review_stack_test.rb b/test/models/shipit/review_stack_test.rb index 29a9d3238..2aca96259 100644 --- a/test/models/shipit/review_stack_test.rb +++ b/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 diff --git a/test/models/shipit/stack_test.rb b/test/models/shipit/stack_test.rb index 693720676..74038d35f 100644 --- a/test/models/shipit/stack_test.rb +++ b/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 diff --git a/test/models/shipit/webhooks/handlers/pull_request/review_stack_adapter_test.rb b/test/models/shipit/webhooks/handlers/pull_request/review_stack_adapter_test.rb index 63217449d..8215851a3 100644 --- a/test/models/shipit/webhooks/handlers/pull_request/review_stack_adapter_test.rb +++ b/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