Merge branch 'refresh-authorizations-fork-join' into 'master'

Fix race conditions for AuthorizedProjectsWorker

Closes #26194 and #26310

See merge request !8701

4 files changed, 20 insertions, 12 deletions

diff --git a/app/models/member.rb b/app/models/member.rb
index c585e0b450e..26a6054e00d 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -68,9 +68,9 @@ class Member < ActiveRecord::Base
   after_create :send_request, if: :request?, unless: :importing?
   after_create :create_notification_setting, unless: [:pending?, :importing?]
   after_create :post_create_hook, unless: [:pending?, :importing?]
-  after_create :refresh_member_authorized_projects, if: :importing?
   after_update :post_update_hook, unless: [:pending?, :importing?]
   after_destroy :post_destroy_hook, unless: :pending?
+  after_commit :refresh_member_authorized_projects
 
   delegate :name, :username, :email, to: :user, prefix: true
 
@@ -147,8 +147,6 @@ class Member < ActiveRecord::Base
         member.save
       end
 
-      UserProjectAccessChangedService.new(user.id).execute if user.is_a?(User)
-
       member
     end
 
@@ -275,23 +273,27 @@ class Member < ActiveRecord::Base
   end
 
   def post_create_hook
-    UserProjectAccessChangedService.new(user.id).execute
     system_hook_service.execute_hooks_for(self, :create)
   end
 
   def post_update_hook
-    UserProjectAccessChangedService.new(user.id).execute if access_level_changed?
+    # override in sub class
   end
 
   def post_destroy_hook
-    refresh_member_authorized_projects
     system_hook_service.execute_hooks_for(self, :destroy)
   end
 
+  # Refreshes authorizations of the current member.
+  #
+  # This method schedules a job using Sidekiq and as such **must not** be called
+  # in a transaction. Doing so can lead to the job running before the
+  # transaction has been committed, resulting in the job either throwing an
+  # error or not doing any meaningful work.
   def refresh_member_authorized_projects
-    # If user/source is being destroyed, project access are gonna be destroyed eventually
-    # because of DB foreign keys, so we shouldn't bother with refreshing after each
-    # member is destroyed through association
+    # If user/source is being destroyed, project access are going to be
+    # destroyed eventually because of DB foreign keys, so we shouldn't bother
+    # with refreshing after each member is destroyed through association
     return if destroyed_by_association.present?
 
     UserProjectAccessChangedService.new(user_id).execute
diff --git a/app/models/project_group_link.rb b/app/models/project_group_link.rb
index 6149c35cc61..5cb6b0c527d 100644
--- a/app/models/project_group_link.rb
+++ b/app/models/project_group_link.rb
@@ -16,8 +16,7 @@ class ProjectGroupLink < ActiveRecord::Base
   validates :group_access, inclusion: { in: Gitlab::Access.values }, presence: true
   validate :different_group
 
-  after_create :refresh_group_members_authorized_projects
-  after_destroy :refresh_group_members_authorized_projects
+  after_commit :refresh_group_members_authorized_projects
 
   def self.access_options
     Gitlab::Access.options
diff --git a/app/services/user_project_access_changed_service.rb b/app/services/user_project_access_changed_service.rb
index 2469b4f0d7c..d7a6804ee88 100644
--- a/app/services/user_project_access_changed_service.rb
+++ b/app/services/user_project_access_changed_service.rb
@@ -4,6 +4,6 @@ class UserProjectAccessChangedService
   end
 
   def execute
-    AuthorizedProjectsWorker.bulk_perform_async(@user_ids.map { |id| [id] })
+    AuthorizedProjectsWorker.bulk_perform_and_wait(@user_ids.map { |id| [id] })
   end
 end
diff --git a/app/workers/authorized_projects_worker.rb b/app/workers/authorized_projects_worker.rb
index 2badd0680fb..6abbb5a5250 100644
--- a/app/workers/authorized_projects_worker.rb
+++ b/app/workers/authorized_projects_worker.rb
@@ -2,6 +2,13 @@ class AuthorizedProjectsWorker
   include Sidekiq::Worker
   include DedicatedSidekiqQueue
 
+  # Schedules multiple jobs and waits for them to be completed.
+  def self.bulk_perform_and_wait(args_list)
+    job_ids = bulk_perform_async(args_list)
+
+    Gitlab::JobWaiter.new(job_ids).wait
+  end
+
   def self.bulk_perform_async(args_list)
     Sidekiq::Client.push_bulk('class' => self, 'args' => args_list)
   end