Merge branch 'refresh-authorizations-tighter-lease' into 'master'

app/services/users/refresh_authorized_projects_service.rb

@@ -26,8 +26,26 @@ module Users
       user.reload
     end
 
-    # This method returns the updated User object.
     def execute
+      lease_key = "refresh_authorized_projects:#{user.id}"
+      lease = Gitlab::ExclusiveLease.new(lease_key, timeout: LEASE_TIMEOUT)
+
+      until uuid = lease.try_obtain
+        # Keep trying until we obtain the lease. If we don't do so we may end up
+        # not updating the list of authorized projects properly. To prevent
+        # hammering Redis too much we'll wait for a bit between retries.
+        sleep(1)
+      end
+
+      begin
+        execute_without_lease
+      ensure
+        Gitlab::ExclusiveLease.cancel(lease_key, uuid)
+      end
+    end
+
+    # This method returns the updated User object.
+    def execute_without_lease
       current = current_authorizations_per_project
       fresh = fresh_access_levels_per_project
 
@@ -47,26 +65,7 @@ module Users
         end
       end
 
-      update_with_lease(remove, add)
-    end
-
-    # Updates the list of authorizations using an exclusive lease.
-    def update_with_lease(remove = [], add = [])
-      lease_key = "refresh_authorized_projects:#{user.id}"
-      lease = Gitlab::ExclusiveLease.new(lease_key, timeout: LEASE_TIMEOUT)
-
-      until uuid = lease.try_obtain
-        # Keep trying until we obtain the lease. If we don't do so we may end up
-        # not updating the list of authorized projects properly. To prevent
-        # hammering Redis too much we'll wait for a bit between retries.
-        sleep(1)
-      end
-
-      begin
       update_authorizations(remove, add)
-      ensure
-        Gitlab::ExclusiveLease.cancel(lease_key, uuid)
-      end
     end
 
     # Updates the list of authorizations for the current user.

changelogs/unreleased/refresh-authorizations-tighter-lease.yml

+---
+title: Synchronize all project authorization refreshing work to prevent race conditions
+merge_request:
+author:

spec/services/users/refresh_authorized_projects_service_spec.rb

@@ -10,7 +10,21 @@ describe Users::RefreshAuthorizedProjectsService do
       create!(project: project, user: user, access_level: access_level)
   end
 
-  describe '#execute' do
+  describe '#execute', :redis do
+    it 'refreshes the authorizations using a lease' do
+      expect_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).
+        and_return('foo')
+
+      expect(Gitlab::ExclusiveLease).to receive(:cancel).
+        with(an_instance_of(String), 'foo')
+
+      expect(service).to receive(:execute_without_lease)
+
+      service.execute
+    end
+  end
+
+  describe '#execute_without_lease' do
     before do
       user.project_authorizations.delete_all
     end
@@ -19,37 +33,23 @@ describe Users::RefreshAuthorizedProjectsService do
       project2 = create(:empty_project)
       to_remove = create_authorization(project2, user)
 
-      expect(service).to receive(:update_with_lease).
+      expect(service).to receive(:update_authorizations).
         with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MASTER]])
 
-      service.execute
+      service.execute_without_lease
     end
 
     it 'sets the access level of a project to the highest available level' do
       to_remove = create_authorization(project, user, Gitlab::Access::DEVELOPER)
 
-      expect(service).to receive(:update_with_lease).
+      expect(service).to receive(:update_authorizations).
         with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MASTER]])
 
-      service.execute
+      service.execute_without_lease
     end
 
     it 'returns a User' do
-      expect(service.execute).to be_an_instance_of(User)
-    end
-  end
-
-  describe '#update_with_lease', :redis do
-    it 'refreshes the authorizations using a lease' do
-      expect_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).
-        and_return('foo')
-
-      expect(Gitlab::ExclusiveLease).to receive(:cancel).
-        with(an_instance_of(String), 'foo')
-
-      expect(service).to receive(:update_authorizations).with([1], [])
-
-      service.update_with_lease([1])
+      expect(service.execute_without_lease).to be_an_instance_of(User)
     end
   end