Merge branch 'sidekiq-transaction' into 'master'

Forbid Sidekiq scheduling in transactions

Closes #27233

See merge request !9376

9 files changed, 91 insertions, 21 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 3bcc8be5cae..effee3f48b1 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -47,8 +47,8 @@ module Ci
     before_destroy { unscoped_project }
 
     after_create :execute_hooks
-    after_save :update_project_statistics, if: :artifacts_size_changed?
-    after_destroy :update_project_statistics
+    after_commit :update_project_statistics_after_save, on: [:create, :update]
+    after_commit :update_project_statistics, on: :destroy
 
     class << self
       # This is needed for url_for to work,
@@ -512,5 +512,11 @@ module Ci
 
       ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size])
     end
+
+    def update_project_statistics_after_save
+      if previous_changes.include?('artifacts_size')
+        update_project_statistics
+      end
+    end
   end
 end
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index fe63728ea23..ce507f7774b 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -18,7 +18,7 @@ class CommitStatus < ActiveRecord::Base
   validates :name, presence: true
 
   alias_attribute :author, :user
-  
+
   scope :failed_but_allowed, -> do
     where(allow_failure: true, status: [:failed, :canceled])
   end
@@ -83,14 +83,15 @@ class CommitStatus < ActiveRecord::Base
       next if transition.loopback?
 
       commit_status.run_after_commit do
-        pipeline.try do |pipeline|
+        if pipeline
           if complete? || manual?
             PipelineProcessWorker.perform_async(pipeline.id)
           else
             PipelineUpdateWorker.perform_async(pipeline.id)
           end
-          ExpireJobCacheWorker.perform_async(commit_status.id)
         end
+
+        ExpireJobCacheWorker.perform_async(commit_status.id)
       end
     end
 
diff --git a/app/models/key.rb b/app/models/key.rb
index b7956052c3f..cb8f10f6d55 100644
--- a/app/models/key.rb
+++ b/app/models/key.rb
@@ -1,7 +1,6 @@
 require 'digest/md5'
 
 class Key < ActiveRecord::Base
-  include AfterCommitQueue
   include Sortable
 
   LAST_USED_AT_REFRESH_TIME = 1.day.to_i
@@ -25,10 +24,10 @@ class Key < ActiveRecord::Base
 
   delegate :name, :email, to: :user, prefix: true
 
-  after_create :add_to_shell
-  after_create :notify_user
+  after_commit :add_to_shell, on: :create
+  after_commit :notify_user, on: :create
   after_create :post_create_hook
-  after_destroy :remove_from_shell
+  after_commit :remove_from_shell, on: :destroy
   after_destroy :post_destroy_hook
 
   def key=(value)
@@ -93,6 +92,6 @@ class Key < ActiveRecord::Base
   end
 
   def notify_user
-    run_after_commit { NotificationService.new.new_key(self) }
+    NotificationService.new.new_key(self)
   end
 end
diff --git a/app/models/lfs_objects_project.rb b/app/models/lfs_objects_project.rb
index 007eed5600a..b0625c52b62 100644
--- a/app/models/lfs_objects_project.rb
+++ b/app/models/lfs_objects_project.rb
@@ -6,8 +6,7 @@ class LfsObjectsProject < ActiveRecord::Base
   validates :lfs_object_id, uniqueness: { scope: [:project_id], message: "already exists in project" }
   validates :project_id, presence: true
 
-  after_create :update_project_statistics
-  after_destroy :update_project_statistics
+  after_commit :update_project_statistics, on: [:create, :destroy]
 
   private
 
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index aebee06d560..b48d73dcae7 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -6,6 +6,7 @@ class Namespace < ActiveRecord::Base
   include Gitlab::ShellAdapter
   include Gitlab::CurrentSettings
   include Routable
+  include AfterCommitQueue
 
   # Prevent users from creating unreasonably deep level of nesting.
   # The number 20 was taken based on maximum nesting level of
@@ -242,7 +243,9 @@ class Namespace < ActiveRecord::Base
 
         # Remove namespace directroy async with delay so
         # GitLab has time to remove all projects first
-        GitlabShellWorker.perform_in(5.minutes, :rm_namespace, repository_storage_path, new_path)
+        run_after_commit do
+          GitlabShellWorker.perform_in(5.minutes, :rm_namespace, repository_storage_path, new_path)
+        end
       end
     end
 
diff --git a/app/models/project.rb b/app/models/project.rb
index 446329557d5..f16d1cab6c4 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -471,7 +471,9 @@ class Project < ActiveRecord::Base
   end
 
   def reset_cache_and_import_attrs
-    ProjectCacheWorker.perform_async(self.id)
+    run_after_commit do
+      ProjectCacheWorker.perform_async(self.id)
+    end
 
     self.import_data&.destroy
   end
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 06d8d143231..e2b2660ea71 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -7,11 +7,9 @@ module Projects
     DELETED_FLAG = '+deleted'.freeze
 
     def async_execute
-      project.transaction do
-        project.update_attribute(:pending_delete, true)
-        job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params)
-        Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.path_with_namespace} with job ID #{job_id}")
-      end
+      project.update_attribute(:pending_delete, true)
+      job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params)
+      Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.path_with_namespace} with job ID #{job_id}")
     end
 
     def execute
@@ -62,7 +60,11 @@ module Projects
 
       if gitlab_shell.mv_repository(project.repository_storage_path, path, new_path)
         log_info("Repository \"#{path}\" moved to \"#{new_path}\"")
-        GitlabShellWorker.perform_in(5.minutes, :remove_repository, project.repository_storage_path, new_path)
+
+        project.run_after_commit do
+          # self is now project
+          GitlabShellWorker.perform_in(5.minutes, :remove_repository, self.repository_storage_path, new_path)
+        end
       else
         false
       end
diff --git a/config/initializers/forbid_sidekiq_in_transactions.rb b/config/initializers/forbid_sidekiq_in_transactions.rb
new file mode 100644
index 00000000000..a78711fe599
--- /dev/null
+++ b/config/initializers/forbid_sidekiq_in_transactions.rb
@@ -0,0 +1,49 @@
+module Sidekiq
+  module Worker
+    mattr_accessor :skip_transaction_check
+    self.skip_transaction_check = false
+
+    def self.skipping_transaction_check(&block)
+      skip_transaction_check = self.skip_transaction_check
+      self.skip_transaction_check = true
+      yield
+    ensure
+      self.skip_transaction_check = skip_transaction_check
+    end
+
+    module ClassMethods
+      module NoSchedulingFromTransactions
+        NESTING = ::Rails.env.test? ? 1 : 0
+
+        %i(perform_async perform_at perform_in).each do |name|
+          define_method(name) do |*args|
+            return super(*args) if Sidekiq::Worker.skip_transaction_check
+            return super(*args) unless ActiveRecord::Base.connection.open_transactions > NESTING
+
+            raise <<-MSG.strip_heredoc
+              `#{self}.#{name}` cannot be called inside a transaction as this can lead to
+              race conditions when the worker runs before the transaction is committed and
+              tries to access a model that has not been saved yet.
+
+              Use an `after_commit` hook, or include `AfterCommitQueue` and use a `run_after_commit` block instead.
+            MSG
+          end
+        end
+      end
+
+      prepend NoSchedulingFromTransactions
+    end
+  end
+end
+
+module ActiveRecord
+  class Base
+    module SkipTransactionCheckAfterCommit
+      def committed!(*)
+        Sidekiq::Worker.skipping_transaction_check { super }
+      end
+    end
+
+    prepend SkipTransactionCheckAfterCommit
+  end
+end
diff --git a/db/fixtures/development/11_keys.rb b/db/fixtures/development/11_keys.rb
index 51e22137d6f..c405ecfdaf3 100644
--- a/db/fixtures/development/11_keys.rb
+++ b/db/fixtures/development/11_keys.rb
@@ -1,17 +1,26 @@
 require './spec/support/sidekiq'
 
+
 # Creating keys runs a gitlab-shell worker. Since we may not have the right
 # gitlab-shell path set (yet) we need to disable this for these fixtures.
 Sidekiq::Testing.disable! do
   Gitlab::Seeder.quiet do
+    # We want to run `add_to_shell` immediately instead of after the commit, so
+    # that it falls under `Sidekiq::Testing.disable!`.
+    Key.skip_callback(:commit, :after, :add_to_shell)
+
     User.first(10).each do |user|
       key = "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt#{user.id + 100}6k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0="
 
-      user.keys.create(
+      key = user.keys.create(
         title: "Sample key #{user.id}",
         key: key
       )
 
+      Sidekiq::Worker.skipping_transaction_check do
+        key.add_to_shell
+      end
+
       print '.'
     end
   end