Merge branch '338949-remove-subtransactions-in-user' into 'master'

Block user outside transaction when deleting

See merge request gitlab-org/gitlab!68690

app/services/users/migrate_to_ghost_user_service.rb

@@ -14,23 +14,30 @@ module Users
 
     def initialize(user)
       @user = user
+      @ghost_user = User.ghost
     end
 
     def execute
       transition = user.block_transition
 
-      user.transaction do
       # Block the user before moving records to prevent a data race.
       # For example, if the user creates an issue after `migrate_issues`
       # runs and before the user is destroyed, the destroy will fail with
       # an exception.
       user.block
 
+      begin
+        user.transaction do
+          migrate_records
+        end
+      rescue Exception # rubocop:disable Lint/RescueException
         # Reverse the user block if record migration fails
-        if !migrate_records_in_transaction && transition
+        if transition
           transition.rollback
           user.save!
         end
+
+        raise
       end
 
       user.reset
@@ -38,14 +45,6 @@ module Users
 
     private
 
-    def migrate_records_in_transaction
-      user.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
-        @ghost_user = User.ghost
-
-        migrate_records
-      end
-    end
-
     def migrate_records
       migrate_issues
       migrate_merge_requests

spec/services/users/migrate_to_ghost_user_service_spec.rb

@@ -92,23 +92,5 @@ RSpec.describe Users::MigrateToGhostUserService do
         let(:created_record) { create(:review, author: user) }
       end
     end
-
-    context "when record migration fails with a rollback exception" do
-      before do
-        expect_any_instance_of(ActiveRecord::Associations::CollectionProxy)
-          .to receive(:update_all).and_raise(ActiveRecord::Rollback)
-      end
-
-      context "for records that were already migrated" do
-        let!(:issue) { create(:issue, project: project, author: user) }
-        let!(:merge_request) { create(:merge_request, source_project: project, author: user, target_branch: "first") }
-
-        it "reverses the migration" do
-          service.execute
-
-          expect(issue.reload.author).to eq(user)
-        end
-      end
-    end
   end
 end

spec/support/services/migrate_to_ghost_user_service_shared_examples.rb

@@ -32,7 +32,7 @@ RSpec.shared_examples "migrating a deleted user's associated records to the ghos
       expect(user).to be_blocked
     end
 
-    it 'migrates all associated fields to te "Ghost user"' do
+    it 'migrates all associated fields to the "Ghost user"' do
       service.execute
 
       migrated_record = record_class.find_by_id(record.id)
@@ -46,40 +46,19 @@ RSpec.shared_examples "migrating a deleted user's associated records to the ghos
       context "when #{record_class_name} migration fails and is rolled back" do
         before do
           expect_any_instance_of(ActiveRecord::Associations::CollectionProxy)
-            .to receive(:update_all).and_raise(ActiveRecord::Rollback)
+            .to receive(:update_all).and_raise(ActiveRecord::StatementTimeout)
         end
 
         it 'rolls back the user block' do
-          service.execute
-
-          expect(user.reload).not_to be_blocked
-        end
-
-        it "doesn't unblock an previously-blocked user" do
-          user.block
-
-          service.execute
-
-          expect(user.reload).to be_blocked
-        end
-      end
-
-      context "when #{record_class_name} migration fails with a non-rollback exception" do
-        before do
-          expect_any_instance_of(ActiveRecord::Associations::CollectionProxy)
-            .to receive(:update_all).and_raise(ArgumentError)
-        end
-
-        it 'rolls back the user block' do
-          service.execute rescue nil
+          expect { service.execute }.to raise_error(ActiveRecord::StatementTimeout)
 
           expect(user.reload).not_to be_blocked
         end
 
-        it "doesn't unblock an previously-blocked user" do
+        it "doesn't unblock a previously-blocked user" do
           user.block
 
-          service.execute rescue nil
+          expect { service.execute }.to raise_error(ActiveRecord::StatementTimeout)
 
           expect(user.reload).to be_blocked
         end