Move records to the ghost user in a transaction.

- While deleting a user, some of the user's associated records are moved to the
  ghost user so they aren't deleted. The user is blocked before these records
  are moved, to prevent the user from creating new records while the migration
  is happening, and so preventing a data race.

- Previously, if the migration failed, the user would _remain_ blocked, which is
  not the expected behavior. On the other hand, we can't just stick the block +
  migration into a transaction, because we want the block to be committed before
  the migration starts (for the data race reason mentioned above).

- One solution (implemented in this commit) is to block the user in a parent
  transaction, migrate the associated records in a nested sub-transaction, and
  then unblock the user in the parent transaction if the sub-transaction fails.

3 files changed, 93 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/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