Don’t schedule workers from inside transactions

7 files changed, 32 insertions, 20 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 58dfdd87652..2c2140c53b0 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,
@@ -491,5 +491,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