diff --git a/CHANGELOG.md b/CHANGELOG.md index 537a031a8..cbb39f0c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,6 @@ # Unreleased +* (bugfix) Update existing check runs without first attempting a duplicate insert. Concurrent + creations still update the existing run, and a savepoint keeps enclosing transactions usable. # 0.45.4 * (performance) Preload commit statuses and check runs when serializing deploys. `DeploySerializer` diff --git a/app/models/shipit/check_run.rb b/app/models/shipit/check_run.rb index c2b91db75..a7f8d97b8 100644 --- a/app/models/shipit/check_run.rb +++ b/app/models/shipit/check_run.rb @@ -19,9 +19,16 @@ class CheckRun < ApplicationRecord class << self def create_or_update_by!(selector:, attributes: {}) - create!(selector.merge(attributes)) - rescue ActiveRecord::RecordNotUnique - record = find_by!(selector) + record = find_by(selector) + unless record + begin + return transaction(requires_new: true) { create!(selector.merge(attributes)) } + rescue ActiveRecord::RecordNotUnique + # A concurrent insert may have created the run after the initial lookup. + # Locking also makes this a current read on MySQL within an enclosing transaction. + record = lock.find_by!(selector) + end + end # Checkruns can jump between states and conclusions, and the github timestamps are low precision and unreliable. # Since there's a conflict and the webhook seems older, enqueue a refresh. diff --git a/test/models/shipit/check_run_test.rb b/test/models/shipit/check_run_test.rb index 138c3c182..276285a98 100644 --- a/test/models/shipit/check_run_test.rb +++ b/test/models/shipit/check_run_test.rb @@ -73,6 +73,74 @@ class CheckRunTest < ActiveSupport::TestCase end end + test ".create_or_update_from_github! updates existing runs without SQL errors" do + completed_at = Time.now + check_run = @commit.check_runs.create_or_update_from_github!( + @stack.id, + github_check_run(conclusion: nil, completed_at:) + ) + sql_errors = [] + subscriber = lambda do |event| + sql_errors << event.payload[:exception] if event.payload[:exception] + end + + ActiveSupport::Notifications.subscribed(subscriber, 'sql.active_record') do + assert_no_difference -> { @commit.check_runs.count } do + @commit.check_runs.create_or_update_from_github!( + @stack.id, + github_check_run(conclusion: 'success', completed_at: completed_at + 1.minute) + ) + end + end + + assert_equal 'success', check_run.reload.conclusion + assert_empty sql_errors + end + + test ".create_or_update_from_github! scopes matching GitHub IDs to the commit" do + check_run = nil + assert_difference -> { CheckRun.where(github_id: @check_run.github_id).count }, +1 do + check_run = @commit.check_runs.create_or_update_from_github!( + @stack.id, + github_check_run(conclusion: 'failure') + ) + end + + @commit.check_runs.create_or_update_from_github!( + @stack.id, + github_check_run(conclusion: 'cancelled') + ) + + assert_equal @commit.id, check_run.commit_id + assert_equal 'cancelled', check_run.reload.conclusion + assert_equal 'success', @check_run.reload.conclusion + end + + test ".create_or_update_from_github! handles a concurrent insert inside a transaction" do + completed_at = Time.now + check_run = @commit.check_runs.create_or_update_from_github!( + @stack.id, + github_check_run(conclusion: 'success', completed_at:) + ) + CheckRun.expects(:find_by).with({ github_id: check_run.github_id }).returns(nil) + + result = nil + CheckRun.transaction(requires_new: true) do + assert_no_difference -> { @commit.check_runs.count } do + assert_enqueued_with(job: RefreshCheckRunsJob, args: [{ commit_id: @commit.id }]) do + result = @commit.check_runs.create_or_update_from_github!( + @stack.id, + github_check_run(conclusion: 'failure', completed_at: completed_at - 1.minute) + ) + end + end + end + + assert_equal check_run.id, result.id + assert_equal 'failure', check_run.reload.conclusion + assert_equal 'success', @check_run.reload.conclusion + end + test ".create_or_update_from_github! enqueues refresh and updates record when new statuses have stale timestamps" do completed_at = Time.now assert_difference -> { @commit.check_runs.count }, +1 do