Block user outside transaction when deleting

When migrating user records to the ghost user, we block the user outside
the transaction so that the user cannot create any new records.

This also avoids having a subtransaction.

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
+      # 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
-        @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 { 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
-
-          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(user.reload).not_to be_blocked
-        end
-
-        it "doesn't unblock an 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