Merge branch '30306-transaction-while-moving-issues-to-ghost-user' into 'master'

Add a transaction around `move_issues_to_ghost_user`

See merge request !10465

4 files changed, 97 insertions, 11 deletions

diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb
index 1e1ed1791ec..4628c4c6f6e 100644
--- a/app/services/users/migrate_to_ghost_user_service.rb
+++ b/app/services/users/migrate_to_ghost_user_service.rb
@@ -15,27 +15,39 @@ module Users
     end
 
     def execute
-      # 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
+      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
+
+        # Reverse the user block if record migration fails
+        if !migrate_records && transition
+          transition.rollback
+          user.save!
+        end
+      end
+
+      user.reload
+    end
+
+    private
+
+    def migrate_records
+      user.transaction(requires_new: true) do
         @ghost_user = User.ghost
 
         migrate_issues
         migrate_merge_requests
         migrate_notes
         migrate_abuse_reports
-        migrate_award_emoji
+        migrate_award_emojis
       end
-
-      user.reload
     end
 
-    private
-
     def migrate_issues
       user.issues.update_all(author_id: ghost_user.id)
     end
@@ -52,7 +64,7 @@ module Users
       user.reported_abuse_reports.update_all(reporter_id: ghost_user.id)
     end
 
-    def migrate_award_emoji
+    def migrate_award_emojis
       user.award_emoji.update_all(user_id: ghost_user.id)
     end
   end
diff --git a/changelogs/unreleased/30306-transaction-while-moving-issues-to-ghost-user.yml b/changelogs/unreleased/30306-transaction-while-moving-issues-to-ghost-user.yml
new file mode 100644
index 00000000000..5fc57e44be6
--- /dev/null
+++ b/changelogs/unreleased/30306-transaction-while-moving-issues-to-ghost-user.yml
@@ -0,0 +1,4 @@
+---
+title: Add a transaction around move_issues_to_ghost_user
+merge_request: 10465
+author:
diff --git a/spec/services/users/migrate_to_ghost_user_service_spec.rb b/spec/services/users/migrate_to_ghost_user_service_spec.rb
index 8c5b7e41c15..9e1edf1ac30 100644
--- a/spec/services/users/migrate_to_ghost_user_service_spec.rb
+++ b/spec/services/users/migrate_to_ghost_user_service_spec.rb
@@ -60,5 +60,23 @@ describe Users::MigrateToGhostUserService, services: true do
         end
       end
     end
+
+    context "when record migration fails with a rollback exception" do
+      before do
+        expect_any_instance_of(MergeRequest::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
diff --git a/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb
index 0eac587e973..dcc562c684b 100644
--- a/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb
+++ b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb
@@ -35,5 +35,57 @@ shared_examples "migrating a deleted user's associated records to the ghost user
 
       expect(user).to be_blocked
     end
+
+    context "race conditions" do
+      context "when #{record_class_name} migration fails and is rolled back" do
+        before do
+          expect_any_instance_of(record_class::ActiveRecord_Associations_CollectionProxy)
+            .to receive(:update_all).and_raise(ActiveRecord::Rollback)
+        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(record_class::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(user.reload).to be_blocked
+        end
+      end
+
+      it "blocks the user before #{record_class_name} migration begins" do
+        expect(service).to receive("migrate_#{record_class_name.parameterize('_')}s".to_sym) do
+          expect(user.reload).to be_blocked
+        end
+
+        service.execute
+      end
+    end
   end
 end