diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb
index 1b083c70bba85a13ce8bfda2d780200952b654a1..da289cdbb3b2c9956b9b815f797327aebd68e1da 100644
--- a/app/mailers/previews/notify_preview.rb
+++ b/app/mailers/previews/notify_preview.rb
@@ -241,11 +241,17 @@ def new_email_address_added_email
end
def service_desk_new_note_email
- cleanup do
- note = create_note(noteable_type: 'Issue', noteable_id: issue.id, note: 'Issue note content')
- participant = IssueEmailParticipant.create!(issue: issue, email: 'user@example.com')
-
- Notify.service_desk_new_note_email(issue.id, note.id, participant).message
+ # TODO: create support_bot outside of the transaction
+ # The exclusive lease is obtained when creating support_bot in app/mailers/emails/service_desk.rb
+ # The `cleanup` method wraps the email-generating-block in a transaction.
+ # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/441523
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ cleanup do
+ note = create_note(noteable_type: 'Issue', noteable_id: issue.id, note: 'Issue note content')
+ participant = IssueEmailParticipant.create!(issue: issue, email: 'user@example.com')
+
+ Notify.service_desk_new_note_email(issue.id, note.id, participant).message
+ end
end
end
@@ -254,10 +260,16 @@ def service_desk_thank_you_email
end
def service_desk_custom_email_verification_email
- cleanup do
- setup_service_desk_custom_email_objects
-
- Notify.service_desk_custom_email_verification_email(service_desk_setting).message
+ # TODO: create support_bot outside of the transaction
+ # The exclusive lease is obtained when creating support_bot in app/mailers/emails/service_desk.rb
+ # The `cleanup` method wraps the email-generating-block in a transaction.
+ # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/441523
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ cleanup do
+ setup_service_desk_custom_email_objects
+
+ Notify.service_desk_custom_email_verification_email(service_desk_setting).message
+ end
end
end
@@ -440,10 +452,16 @@ def create_note(params)
end
def note_email(method)
- cleanup do
- note = yield
-
- Notify.public_send(method, user.id, note) # rubocop:disable GitlabSecurity/PublicSend
+ # TODO: create support_bot outside of the transaction
+ # The exclusive lease is obtained when creating support_bot in app/mailers/emails/service_desk.rb
+ # The `cleanup` method wraps the email-generating-block in a transaction.
+ # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/441523
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ cleanup do
+ note = yield
+
+ Notify.public_send(method, user.id, note) # rubocop:disable GitlabSecurity/PublicSend
+ end
end
end
diff --git a/app/models/projects/build_artifacts_size_refresh.rb b/app/models/projects/build_artifacts_size_refresh.rb
index b791cb1254c3c58c23aa86b83ecd9f37ad5b78f4..66f2c533b515cfd3a69965728178f4c73e852044 100644
--- a/app/models/projects/build_artifacts_size_refresh.rb
+++ b/app/models/projects/build_artifacts_size_refresh.rb
@@ -106,7 +106,11 @@ def self.process_next_refresh!
end
def reset_project_statistics!
- project.statistics.initiate_refresh!(COUNTER_ATTRIBUTE_NAME)
+ # This method is called in the `before_transition` block which is in a transaction.
+ # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/441524
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ project.statistics.initiate_refresh!(COUNTER_ATTRIBUTE_NAME)
+ end
end
def next_batch(limit:)
diff --git a/app/services/ci/retry_job_service.rb b/app/services/ci/retry_job_service.rb
index a8ea5ac6df02300205ccb1b0c711e941aa246ab8..8d9dc3a9cb89e155217e034e29a381a7e880deb2 100644
--- a/app/services/ci/retry_job_service.rb
+++ b/app/services/ci/retry_job_service.rb
@@ -44,9 +44,15 @@ def clone!(job, variables: [], enqueue_if_actionable: false, start_pipeline: fal
.close(new_job)
end
- ::Ci::Pipelines::AddJobService.new(job.pipeline).execute!(new_job) do |processable|
- BulkInsertableAssociations.with_bulk_insert do
- processable.save!
+ # This method is called on the `drop!` state transition for Ci::Build which runs the retry in the
+ # `after_transition` block within a transaction.
+ # Ci::Pipelines::AddJobService then obtains the exclusive lease inside the same transaction.
+ # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/441525
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ ::Ci::Pipelines::AddJobService.new(job.pipeline).execute!(new_job) do |processable|
+ BulkInsertableAssociations.with_bulk_insert do
+ processable.save!
+ end
end
end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 0706e921a9d9a444744916c70bf1b2174006291a..e508499a92b65f64bf76cfa826b1124e4bcdaf59 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -341,7 +341,12 @@ def update(issuable)
issuable.updated_by = current_user if should_touch
- transaction_update(issuable, { save_with_touch: should_touch })
+ # `issuable` could create a ghost user when updating `last_edited_by`.
+ # Users::Internal.ghost will obtain an ExclusiveLease within this transaction.
+ # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/441526
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ transaction_update(issuable, { save_with_touch: should_touch })
+ end
end
if issuable_saved
diff --git a/app/services/users/activity_service.rb b/app/services/users/activity_service.rb
index b490df6a134f620d4d6f28befcd7700c61c2e868..87597d2a59ab69b114e52bc707c9d3f339c6de5c 100644
--- a/app/services/users/activity_service.rb
+++ b/app/services/users/activity_service.rb
@@ -33,7 +33,9 @@ def record_activity
return if user.last_activity_on == today
lease = Gitlab::ExclusiveLease.new("activity_service:#{user.id}", timeout: LEASE_TIMEOUT)
- return unless lease.try_obtain
+ # Skip transaction checks for exclusive lease as it is breaking system specs.
+ # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/441536
+ return unless Gitlab::ExclusiveLease.skipping_transaction_check { lease.try_obtain }
user.update_attribute(:last_activity_on, today)
diff --git a/config/initializers/forbid_sidekiq_in_transactions.rb b/config/initializers/forbid_sidekiq_in_transactions.rb
index 6dd12c285151cf0f0f25409665a4ef8d15176517..003884a067b2cfc49d4f8f15d92ef039c96177d4 100644
--- a/config/initializers/forbid_sidekiq_in_transactions.rb
+++ b/config/initializers/forbid_sidekiq_in_transactions.rb
@@ -98,15 +98,3 @@ class TestAdapter
end
end
end
-
-module ActiveRecord
- class Base
- module SkipTransactionCheckAfterCommit
- def committed!(*args, **kwargs)
- Sidekiq::Worker.skipping_transaction_check { super }
- end
- end
-
- prepend SkipTransactionCheckAfterCommit
- end
-end
diff --git a/config/initializers/skip_transaction_checks.rb b/config/initializers/skip_transaction_checks.rb
new file mode 100644
index 0000000000000000000000000000000000000000..1298d0e398f3c16f263f75ff73682146c179fc52
--- /dev/null
+++ b/config/initializers/skip_transaction_checks.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+module ActiveRecord
+ class Base
+ module SkipTransactionCheckAfterCommit
+ def committed!(*args, **kwargs)
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ Sidekiq::Worker.skipping_transaction_check { super }
+ end
+ end
+ end
+
+ prepend SkipTransactionCheckAfterCommit
+ end
+end
diff --git a/db/fixtures/development/03_project.rb b/db/fixtures/development/03_project.rb
index 9366316c7675fdc7603684d44216596b9bbab37a..e00c3a12704f75a9f53c1b79d3c8e8ab3794ed6e 100644
--- a/db/fixtures/development/03_project.rb
+++ b/db/fixtures/development/03_project.rb
@@ -184,19 +184,21 @@ def create_real_project!(url, force_latest_storage: false)
project = nil
- Sidekiq::Worker.skipping_transaction_check do
- project = ::Projects::CreateService.new(User.first, params).execute
-
- # Seed-Fu runs this entire fixture in a transaction, so the `after_commit`
- # hook won't run until after the fixture is loaded. That is too late
- # since the Sidekiq::Testing block has already exited. Force clearing
- # the `after_commit` queue to ensure the job is run now.
- project.send(:_run_after_commit_queue)
- project.import_state.send(:_run_after_commit_queue)
-
- # Expire repository cache after import to ensure
- # valid_repo? call below returns a correct answer
- project.repository.expire_all_method_caches
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ Sidekiq::Worker.skipping_transaction_check do
+ project = ::Projects::CreateService.new(User.first, params).execute
+
+ # Seed-Fu runs this entire fixture in a transaction, so the `after_commit`
+ # hook won't run until after the fixture is loaded. That is too late
+ # since the Sidekiq::Testing block has already exited. Force clearing
+ # the `after_commit` queue to ensure the job is run now.
+ project.send(:_run_after_commit_queue)
+ project.import_state.send(:_run_after_commit_queue)
+
+ # Expire repository cache after import to ensure
+ # valid_repo? call below returns a correct answer
+ project.repository.expire_all_method_caches
+ end
end
if project.valid? && project.valid_repo?
diff --git a/db/fixtures/development/13_comments.rb b/db/fixtures/development/13_comments.rb
index 8f63ca302c69f523fef8e1e5b5bfc6c5ec658dbb..f19beaefcb7a8f6f1f6b6f087bb417531415715f 100644
--- a/db/fixtures/development/13_comments.rb
+++ b/db/fixtures/development/13_comments.rb
@@ -11,7 +11,9 @@
note: FFaker::Lorem.sentence,
}
- Notes::CreateService.new(project, user, note_params).execute
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ Notes::CreateService.new(project, user, note_params).execute
+ end
print '.'
end
end
@@ -25,8 +27,9 @@
noteable_id: mr.id,
note: FFaker::Lorem.sentence,
}
-
- Notes::CreateService.new(project, user, note_params).execute
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ Notes::CreateService.new(project, user, note_params).execute
+ end
print '.'
end
end
diff --git a/db/fixtures/development/14_pipelines.rb b/db/fixtures/development/14_pipelines.rb
index 05916c688f3f7ec2887e605144986bd7c0878cd5..e02b55786ad4d317a1b150930884ced99f62884f 100644
--- a/db/fixtures/development/14_pipelines.rb
+++ b/db/fixtures/development/14_pipelines.rb
@@ -108,7 +108,9 @@ def seed!
pipeline.update_duration
- ::Ci::ProcessPipelineService.new(pipeline).execute
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ ::Ci::ProcessPipelineService.new(pipeline).execute
+ end
end
::Gitlab::Seeders::Ci::DailyBuildGroupReportResult.new(@project).seed if @project.last_pipeline
@@ -242,7 +244,9 @@ def setup_test_reports(build)
def setup_build_log(build)
return unless %w[running success failed].include?(build.status)
- build.trace.set(FFaker::Lorem.paragraphs(6).join("\n\n"))
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ build.trace.set(FFaker::Lorem.paragraphs(6).join("\n\n"))
+ end
end
def generic_commit_status_create!(pipeline, stage, opts = {})
diff --git a/db/fixtures/development/24_forks.rb b/db/fixtures/development/24_forks.rb
index f702192d978bf62d982033def5381aef49b24dc3..fc86afc2a9d4af56c1b54cb4a3fcd5a36a8a0b56 100644
--- a/db/fixtures/development/24_forks.rb
+++ b/db/fixtures/development/24_forks.rb
@@ -29,8 +29,11 @@
# hook won't run until after the fixture is loaded. That is too late
# since the Sidekiq::Testing block has already exited. Force clearing
# the `after_commit` queue to ensure the job is run now.
- fork_project.send(:_run_after_commit_queue)
- fork_project.import_state.send(:_run_after_commit_queue)
+
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ fork_project.send(:_run_after_commit_queue)
+ fork_project.import_state.send(:_run_after_commit_queue)
+ end
# We should also force-run the project authorizations refresh job for the created project.
AuthorizedProjectUpdate::ProjectRecalculateService.new(fork_project).execute
diff --git a/db/fixtures/development/26_packages.rb b/db/fixtures/development/26_packages.rb
index c09d3f1fef974c0471bdcb4af9eecc86dc504442..0c244c992ffe68d1420829ef782f38b0b78ebf7d 100644
--- a/db/fixtures/development/26_packages.rb
+++ b/db/fixtures/development/26_packages.rb
@@ -21,7 +21,9 @@ def seed_npm_packages
.gsub('1.0.1', version))
.with_indifferent_access
- ::Packages::Npm::CreatePackageService.new(project, project.creator, params).execute
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ ::Packages::Npm::CreatePackageService.new(project, project.creator, params).execute
+ end
print '.'
end
diff --git a/db/fixtures/development/34_uploads.rb b/db/fixtures/development/34_uploads.rb
index fc45c51e136eebb035417d1ca534a9f2df214c46..5238ac26b687a01127777582418f2f0b913ab5c4 100644
--- a/db/fixtures/development/34_uploads.rb
+++ b/db/fixtures/development/34_uploads.rb
@@ -1,4 +1,4 @@
-# frozen_string_literal: true
+# frozen_string_literal: true
# This seeder seeds comments as well, because uploads are not relevant by
# themselves
@@ -24,7 +24,9 @@
note: "Seeded upload: #{uploader.to_h[:markdown]}",
}
- Notes::CreateService.new(project, user, note_params).execute
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ Notes::CreateService.new(project, user, note_params).execute
+ end
print '.'
end
end
@@ -47,7 +49,9 @@
note: "Seeded upload: #{uploader.to_h[:markdown]}",
}
- Notes::CreateService.new(project, user, note_params).execute
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ Notes::CreateService.new(project, user, note_params).execute
+ end
print '.'
end
end
diff --git a/doc/development/performance.md b/doc/development/performance.md
index 9f63c5509f3c38032cd77e3f58df6b83842d7d2e..204c887e67f45f0e8f1de3090e5707e3649a1e85 100644
--- a/doc/development/performance.md
+++ b/doc/development/performance.md
@@ -956,3 +956,19 @@ Assuming you are working with ActiveRecord models, you might also find these lin
### Examples
You may find some useful examples in [this snippet](https://gitlab.com/gitlab-org/gitlab-foss/snippets/33946).
+
+## ExclusiveLease
+
+`Gitlab::ExclusiveLease` is a Redis-based locking mechanism that lets developers achieve mutual exclusion across distributed servers. There are several wrappers available for developers to make use of:
+
+1. The `Gitlab::ExclusiveLeaseHelpers` module provides a helper method to block the process or thread until the lease can be expired.
+1. The `ExclusiveLease::Guard` module helps get an exclusive lease for a running block of code.
+
+You should not use `ExclusiveLease` in a database transaction because any slow Redis I/O could increase idle transaction duration. The `.try_obtain` method checks if the lease attempt is within any database transactions, and tracks an exception in Sentry and the `log/exceptions_json.log`.
+
+In a test or development environment, any lease attempts in database transactions will raise a `Gitlab::ExclusiveLease::LeaseWithinTransactionError` unless performed within a `Gitlab::ExclusiveLease.skipping_transaction_check` block. You should only use the skip functionality in specs where possible, and placed as close to the lease as possible for ease of understanding. To keep the specs DRY, there are two parts of the codebase where the transaction check skips are re-used:
+
+1. `Users::Internal` is patched to skip transaction checks for bot creation in `let_it_be`.
+1. `FactoryBot` factory for `:deploy_key` skips transaction during the `DeployKey` model creation.
+
+Any use of `Gitlab::ExclusiveLease.skipping_transaction_check` in non-spec or non-fixture files should include links to an infradev issue for plans to remove it.
diff --git a/ee/app/workers/work_items/update_parent_objectives_progress_worker.rb b/ee/app/workers/work_items/update_parent_objectives_progress_worker.rb
index 475ab11bb9ddf2e3626957ae8c297aaf82a816c1..b50793c48d93ab14623e73f22c9ca5b1360462de 100644
--- a/ee/app/workers/work_items/update_parent_objectives_progress_worker.rb
+++ b/ee/app/workers/work_items/update_parent_objectives_progress_worker.rb
@@ -32,7 +32,11 @@ def update_parent_progress(parent)
return unless parent_progress.progress_changed?
parent_progress.save!
- ::SystemNoteService.change_progress_note(parent, Users::Internal.automation_bot)
+ # TODO: creating automation_bot obtains an exclusive lease within the transaction in `.perform`
+ # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/441527
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ ::SystemNoteService.change_progress_note(parent, Users::Internal.automation_bot)
+ end
::GraphqlTriggers.work_item_updated(parent)
end
end
diff --git a/ee/db/fixtures/development/90_productivity_analytics.rb b/ee/db/fixtures/development/90_productivity_analytics.rb
index 41cd3dd5eafd8154c1d29f196dd6392fa8188ef0..ff1b3ce4b233592da6f4762d2fc52ce3b8d2d998 100644
--- a/ee/db/fixtures/development/90_productivity_analytics.rb
+++ b/ee/db/fixtures/development/90_productivity_analytics.rb
@@ -48,23 +48,25 @@ def seed!
Sidekiq::Worker.skipping_transaction_check do
Sidekiq::Testing.inline! do
- create_maintainers!
- print '.'
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ create_maintainers!
+ print '.'
- issues = create_issues
- print '.'
+ issues = create_issues
+ print '.'
- add_milestones_and_list_labels(issues)
- print '.'
+ add_milestones_and_list_labels(issues)
+ print '.'
- branches = mention_in_commits(issues)
- print '.'
+ branches = mention_in_commits(issues)
+ print '.'
- merge_requests = create_merge_requests_closing_issues(issues, branches)
- print '.'
+ merge_requests = create_merge_requests_closing_issues(issues, branches)
+ print '.'
- create_notes(merge_requests)
- print '.'
+ create_notes(merge_requests)
+ print '.'
+ end
end
end
@@ -219,19 +221,21 @@ def create_notes(merge_requests)
end
def merge_merge_requests
- Sidekiq::Worker.skipping_transaction_check do
- project.merge_requests.take(7).each do |merge_request| # leaves some MRs opened for code review analytics chart
- date = get_date_after(merge_request.created_at)
- user = maintainers.sample
- travel_to(date) do
- MergeRequests::MergeService.new(
- project: merge_request.project,
- current_user: user,
- params: { sha: merge_request.diff_head_sha }
- ).execute(merge_request.reset)
-
- issue = merge_request.visible_closing_issues_for(user).first
- MergeRequests::CloseIssueWorker.new.perform(project.id, user.id, issue.id, merge_request.id) if issue
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ Sidekiq::Worker.skipping_transaction_check do
+ project.merge_requests.take(7).each do |merge_request| # leaves some MRs opened for code review analytics chart
+ date = get_date_after(merge_request.created_at)
+ user = maintainers.sample
+ travel_to(date) do
+ MergeRequests::MergeService.new(
+ project: merge_request.project,
+ current_user: user,
+ params: { sha: merge_request.diff_head_sha }
+ ).execute(merge_request.reset)
+
+ issue = merge_request.visible_closing_issues_for(user).first
+ MergeRequests::CloseIssueWorker.new.perform(project.id, user.id, issue.id, merge_request.id) if issue
+ end
end
end
end
diff --git a/lib/gitlab/counters/buffered_counter.rb b/lib/gitlab/counters/buffered_counter.rb
index 9d704f5613c96f2e1aaee665153cd9bc30461ea7..17c79b53e3dbbf54cb0a5c0c5b0764283aaea5ae 100644
--- a/lib/gitlab/counters/buffered_counter.rb
+++ b/lib/gitlab/counters/buffered_counter.rb
@@ -148,7 +148,12 @@ def commit_increment!
next if flush_amount == 0
counter_record.transaction do
- counter_record.update_counters_with_lease({ attribute => flush_amount })
+ # Exclusive lease is obtained within `counter_record.transaction` which could lead to idle transactions
+ # if exclusive lease Redis I/O is slow. We skip the transaction check for now.
+ # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/441530
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ counter_record.update_counters_with_lease({ attribute => flush_amount })
+ end
remove_flushed_key
end
diff --git a/lib/gitlab/exclusive_lease.rb b/lib/gitlab/exclusive_lease.rb
index 0b18a33770701353fb385d29408e9eb22f7560c3..d470fb503cbe266022cf8d3f04e60f47d151226d 100644
--- a/lib/gitlab/exclusive_lease.rb
+++ b/lib/gitlab/exclusive_lease.rb
@@ -12,6 +12,8 @@ module Gitlab
# ExclusiveLease.
#
class ExclusiveLease
+ LeaseWithinTransactionError = Class.new(StandardError)
+
PREFIX = 'gitlab:exclusive_lease'
NoKey = Class.new(ArgumentError)
@@ -86,6 +88,23 @@ def self.reset_all!(scope = '*')
end
end
+ def self.set_skip_transaction_check_flag(flag = nil)
+ Thread.current[:skip_transaction_check_for_exclusive_lease] = flag
+ end
+
+ def self.skip_transaction_check?
+ Thread.current[:skip_transaction_check_for_exclusive_lease]
+ end
+
+ def self.skipping_transaction_check
+ previous_skip_transaction_check = skip_transaction_check?
+ set_skip_transaction_check_flag(true)
+
+ yield
+ ensure
+ set_skip_transaction_check_flag(previous_skip_transaction_check)
+ end
+
def initialize(key, uuid: nil, timeout:)
@redis_shared_state_key = self.class.redis_shared_state_key(key)
@timeout = timeout
@@ -95,12 +114,25 @@ def initialize(key, uuid: nil, timeout:)
# Try to obtain the lease. Return lease UUID on success,
# false if the lease is already taken.
def try_obtain
+ report_lock_attempt_inside_transaction unless self.class.skip_transaction_check?
+
# Performing a single SET is atomic
Gitlab::Redis::SharedState.with do |redis|
redis.set(@redis_shared_state_key, @uuid, nx: true, ex: @timeout) && @uuid
end
end
+ def report_lock_attempt_inside_transaction
+ return unless ::ApplicationRecord.inside_transaction? || ::Ci::ApplicationRecord.inside_transaction?
+
+ raise LeaseWithinTransactionError,
+ "Exclusive lease cannot be obtained within a transaction as it could lead to idle transactions."
+ rescue LeaseWithinTransactionError => e
+ Gitlab::ErrorTracking.track_and_raise_for_dev_exception(
+ e, issue_url: "https://gitlab.com/gitlab-org/gitlab/-/issues/440368"
+ )
+ end
+
# This lease is waiting to obtain
def waiting?
!try_obtain
diff --git a/lib/gitlab/performance_bar/redis_adapter_when_peek_enabled.rb b/lib/gitlab/performance_bar/redis_adapter_when_peek_enabled.rb
index 79c00a48336e98b00a741ab21598f75748afc344..4eeed8825ff1a385c641f747397e55a8ccedd350 100644
--- a/lib/gitlab/performance_bar/redis_adapter_when_peek_enabled.rb
+++ b/lib/gitlab/performance_bar/redis_adapter_when_peek_enabled.rb
@@ -21,10 +21,17 @@ def enqueue_stats_job(request_id)
@client.sadd?(GitlabPerformanceBarStatsWorker::STATS_KEY, request_id)
- return unless uuid = Gitlab::ExclusiveLease.new(
- GitlabPerformanceBarStatsWorker::LEASE_KEY,
- timeout: GitlabPerformanceBarStatsWorker::LEASE_TIMEOUT
- ).try_obtain
+ # We skip transaction check as the transaction check fails the system spec for
+ # spec/features/user_can_display_performance_bar_spec.rb.
+ # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/441535
+ uuid = Gitlab::ExclusiveLease.skipping_transaction_check do
+ Gitlab::ExclusiveLease.new(
+ GitlabPerformanceBarStatsWorker::LEASE_KEY,
+ timeout: GitlabPerformanceBarStatsWorker::LEASE_TIMEOUT
+ ).try_obtain
+ end
+
+ return unless uuid
# stats key should be periodically processed and deleted by
# GitlabPerformanceBarStatsWorker but if it doesn't happen for
diff --git a/lib/users/internal.rb b/lib/users/internal.rb
index 4b6df4ed9280dc8d97dfcd2da59871b2aa055a5d..82c8049bed69af47f58c7589639077a68e72627d 100644
--- a/lib/users/internal.rb
+++ b/lib/users/internal.rb
@@ -101,6 +101,8 @@ def bot_avatar(image:)
Rails.root.join('lib', 'assets', 'images', 'bot_avatars', image).open
end
+ # NOTE: This method is patched in spec/spec_helper.rb to allow use of exclusive lease in RSpec's
+ # :before_all scope to keep the specs DRY.
def unique_internal(scope, username, email_pattern, &block)
scope.first || create_unique_internal(scope, username, email_pattern, &block)
end
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index 6662e83b56474226b01fb611fba24d9ef090ad93..3bfd15b56e02f9ded1c88204e31778014a48d625 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -231,14 +231,16 @@
end
after(:create) do |build, evaluator|
- build.trace.set("Coverage #{evaluator.trace_coverage}%")
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ build.trace.set("Coverage #{evaluator.trace_coverage}%")
+ end
build.trace.archive! if build.complete?
end
end
trait :trace_live do
after(:create) do |build, evaluator|
- build.trace.set('BUILD TRACE')
+ Gitlab::ExclusiveLease.skipping_transaction_check { build.trace.set('BUILD TRACE') }
end
end
@@ -260,7 +262,7 @@
File.expand_path(
Rails.root.join('spec/fixtures/trace/trace_with_duplicate_sections')))
- build.trace.set(trace)
+ Gitlab::ExclusiveLease.skipping_transaction_check { build.trace.set(trace) }
end
end
@@ -270,7 +272,7 @@
File.expand_path(
Rails.root.join('spec/fixtures/trace/trace_with_sections')))
- build.trace.set(trace)
+ Gitlab::ExclusiveLease.skipping_transaction_check { build.trace.set(trace) }
end
end
diff --git a/spec/factories/keys.rb b/spec/factories/keys.rb
index 2cfe13722006c4db0755049ae1b5acac73f1526d..056cea47177789b80f53eec3654476f081284321 100644
--- a/spec/factories/keys.rb
+++ b/spec/factories/keys.rb
@@ -31,6 +31,9 @@
end
factory :deploy_key, class: 'DeployKey' do
+ after(:build) { Gitlab::ExclusiveLease.set_skip_transaction_check_flag(true) }
+ after(:create) { Gitlab::ExclusiveLease.set_skip_transaction_check_flag(nil) }
+
trait :private do
public { false }
end
@@ -55,6 +58,9 @@
end
factory :another_key do
+ after(:build) { Gitlab::ExclusiveLease.set_skip_transaction_check_flag(true) }
+ after(:create) { Gitlab::ExclusiveLease.set_skip_transaction_check_flag(nil) }
+
factory :another_deploy_key, class: 'DeployKey'
end
@@ -70,7 +76,10 @@
KEY
end
- factory :rsa_deploy_key_2048, class: 'DeployKey'
+ factory :rsa_deploy_key_2048, class: 'DeployKey' do
+ after(:build) { Gitlab::ExclusiveLease.set_skip_transaction_check_flag(true) }
+ after(:create) { Gitlab::ExclusiveLease.set_skip_transaction_check_flag(nil) }
+ end
end
factory :rsa_key_4096 do
@@ -110,7 +119,10 @@
KEY
end
- factory :rsa_deploy_key_5120, class: 'DeployKey'
+ factory :rsa_deploy_key_5120, class: 'DeployKey' do
+ after(:build) { Gitlab::ExclusiveLease.set_skip_transaction_check_flag(true) }
+ after(:create) { Gitlab::ExclusiveLease.set_skip_transaction_check_flag(nil) }
+ end
end
factory :rsa_key_8192 do
diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb
index e751a0d740320dd943187153acfbaf4a157c75e2..0dc3ca278256481a6ca1d5fa1bb36a378d86d8a8 100644
--- a/spec/factories/projects.rb
+++ b/spec/factories/projects.rb
@@ -113,7 +113,7 @@
# user have access to the project. Our specs don't use said service class,
# thus we must manually refresh things here.
unless project.group || project.pending_delete
- project.add_owner(project.first_owner)
+ Gitlab::ExclusiveLease.skipping_transaction_check { project.add_owner(project.first_owner) }
end
if project.group
diff --git a/spec/factories/users.rb b/spec/factories/users.rb
index f51b087e494ab871044468d2835b57e4180c12f7..b23c5046eb9b3d678eaa944861d5003ee866826b 100644
--- a/spec/factories/users.rb
+++ b/spec/factories/users.rb
@@ -52,7 +52,9 @@
end
trait :locked do
- after(:build) { |user, _| user.lock_access! }
+ after(:build) do |user, _|
+ Gitlab::ExclusiveLease.skipping_transaction_check { user.lock_access! }
+ end
end
trait :disallowed_password do
diff --git a/spec/lib/gitlab/exclusive_lease_spec.rb b/spec/lib/gitlab/exclusive_lease_spec.rb
index a02e2625c5eb66411c973a3dec76e217c10b53fb..33870c042cfca787778d5137cd9e9981c99f128f 100644
--- a/spec/lib/gitlab/exclusive_lease_spec.rb
+++ b/spec/lib/gitlab/exclusive_lease_spec.rb
@@ -20,6 +20,78 @@
sleep(2 * timeout) # lease should have expired now
expect(lease.try_obtain).to be_present
end
+
+ context 'when lease attempt within pg transaction' do
+ let(:lease) { described_class.new(unique_key, timeout: 1) }
+
+ subject(:lease_attempt) { lease.try_obtain }
+
+ context 'in development/test environment' do
+ it 'raises error within ci db' do
+ expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).and_call_original
+
+ Ci::Pipeline.transaction do
+ expect { lease_attempt }.to raise_error(Gitlab::ExclusiveLease::LeaseWithinTransactionError)
+ end
+ end
+
+ it 'raises error within main db' do
+ expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).and_call_original
+
+ ApplicationRecord.transaction do
+ expect { lease_attempt }.to raise_error(Gitlab::ExclusiveLease::LeaseWithinTransactionError)
+ end
+ end
+ end
+
+ context 'in production environment' do
+ before do
+ stub_rails_env('production')
+ end
+
+ it 'logs error within ci db' do
+ expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).and_call_original
+
+ Ci::Pipeline.transaction { lease_attempt }
+ end
+
+ it 'logs error within main db' do
+ expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).and_call_original
+
+ ApplicationRecord.transaction { lease_attempt }
+ end
+ end
+ end
+
+ context 'when allowed to attempt within pg transaction' do
+ shared_examples 'no error tracking performed' do
+ it 'does not raise error within ci db' do
+ expect(Gitlab::ErrorTracking).not_to receive(:track_and_raise_for_dev_exception).and_call_original
+
+ Ci::Pipeline.transaction { allowed_lease_attempt }
+ end
+
+ it 'does not raise error within main db' do
+ expect(Gitlab::ErrorTracking).not_to receive(:track_and_raise_for_dev_exception).and_call_original
+
+ ApplicationRecord.transaction { allowed_lease_attempt }
+ end
+ end
+
+ let(:lease) { described_class.new(unique_key, timeout: 1) }
+
+ subject(:allowed_lease_attempt) { described_class.skipping_transaction_check { lease.try_obtain } }
+
+ it_behaves_like 'no error tracking performed'
+
+ context 'in production environment' do
+ before do
+ stub_rails_env('production')
+ end
+
+ it_behaves_like 'no error tracking performed'
+ end
+ end
end
describe '.redis_shared_state_key' do
diff --git a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb
index d565f3f3150f2f56ad5a4bba4a3d2fe09ed5682a..521a661b75455586afd3db5c8e683d2bac55cbb3 100644
--- a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb
@@ -39,7 +39,9 @@ def match_mr1_note(content_regex)
project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project)
- @restored_project_json = project_tree_restorer.restore
+ @restored_project_json = Gitlab::ExclusiveLease.skipping_transaction_check do
+ project_tree_restorer.restore
+ end
end
end
diff --git a/spec/lib/gitlab/sidekiq_middleware/pause_control/strategies/click_house_migration_spec.rb b/spec/lib/gitlab/sidekiq_middleware/pause_control/strategies/click_house_migration_spec.rb
index 470c860fb605acc63445abaa76357a1f0193863d..0bf23734b582c74478f4c9b6461079901aeb669d 100644
--- a/spec/lib/gitlab/sidekiq_middleware/pause_control/strategies/click_house_migration_spec.rb
+++ b/spec/lib/gitlab/sidekiq_middleware/pause_control/strategies/click_house_migration_spec.rb
@@ -41,8 +41,10 @@ def perform(*); end
include ExclusiveLeaseHelpers
around do |example|
- ClickHouse::MigrationSupport::ExclusiveLock.execute_migration do
- example.run
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ ClickHouse::MigrationSupport::ExclusiveLock.execute_migration do
+ example.run
+ end
end
end
diff --git a/spec/services/ci/create_commit_status_service_spec.rb b/spec/services/ci/create_commit_status_service_spec.rb
index 3c7346e3e2e7a25585be013a9b1486eae1fb5c37..cbdb681afe31b5a4f85caef70f7e820e25085119 100644
--- a/spec/services/ci/create_commit_status_service_spec.rb
+++ b/spec/services/ci/create_commit_status_service_spec.rb
@@ -437,7 +437,7 @@
expect do
snyk_params_list.map do |snyk_params|
Thread.new do
- response = execute_service(snyk_params)
+ response = Gitlab::ExclusiveLease.skipping_transaction_check { execute_service(snyk_params) }
expect(response).to be_success
end
end.each(&:join)
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 058208fdf98f673ad2a5c50500ca15e5f94e8fe0..c3e9f5a178e9123441a0a76cd164fe02df19acca 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -556,3 +556,23 @@ def process(commands, &block)
end
Redis::Client.prepend(RedisCommands::Instrumentation)
+
+module UsersInternalAllowExclusiveLease
+ extend ActiveSupport::Concern
+
+ class_methods do
+ def unique_internal(scope, username, email_pattern, &block)
+ # this lets skip transaction checks when Users::Internal bots are created in
+ # let_it_be blocks during test set-up.
+ #
+ # Users::Internal bot creation within examples are still checked since the RSPec.current_scope is :example
+ if ::RSpec.respond_to?(:current_scope) && ::RSpec.current_scope == :before_all
+ Gitlab::ExclusiveLease.skipping_transaction_check { super }
+ else
+ super
+ end
+ end
+ end
+end
+
+Users::Internal.prepend(UsersInternalAllowExclusiveLease)
diff --git a/spec/support/database/click_house/hooks.rb b/spec/support/database/click_house/hooks.rb
index 8c8afdbc689e8a4171a149826b419d3347da4484..31c603a42ac47711b6023802065d20e71475ad5c 100644
--- a/spec/support/database/click_house/hooks.rb
+++ b/spec/support/database/click_house/hooks.rb
@@ -32,7 +32,7 @@ def ensure_schema
schema_migration.ensure_table
migration_context = ClickHouse::MigrationSupport::MigrationContext.new(connection,
migrations_paths, schema_migration)
- migrate(migration_context, nil)
+ Gitlab::ExclusiveLease.skipping_transaction_check { migrate(migration_context, nil) }
@ensure_schema = true
end
diff --git a/spec/support/helpers/stubbed_member.rb b/spec/support/helpers/stubbed_member.rb
index d61cdea5354a8982a018673f713e7fd847da9c98..7e7d7c10f00b829ff6960e9e42e9e29f4aaec416 100644
--- a/spec/support/helpers/stubbed_member.rb
+++ b/spec/support/helpers/stubbed_member.rb
@@ -12,7 +12,11 @@ module Member
private
def refresh_member_authorized_projects
- AuthorizedProjectsWorker.new.perform(user_id)
+ # In stubbed_member the original methods stubbed would call .perform_async
+ # so the affected workers would not be in a transaction in a non-test environment.
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ AuthorizedProjectsWorker.new.perform(user_id)
+ end
end
end
@@ -20,7 +24,11 @@ module ProjectMember
private
def execute_project_authorizations_refresh
- AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker.new.perform(project.id, user.id)
+ # In stubbed_member the original methods stubbed would call .perform_async
+ # so the affected workers would not be in a transaction in a non-test environment.
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker.new.perform(project.id, user.id)
+ end
end
end
end