Implement final review comments from @DouweM and @rymai

- Have `Uniquify` take a block instead of a Proc/function. This is more
  idiomatic than passing around a function in Ruby.

- Block a user before moving their issues to the ghost user. This avoids a data
  race where an issue is created after the issues are migrated to the ghost user,
  and before the destroy takes place.

- No need to migrate issues (to the ghost user) in a transaction, because
  we're using `update_all`

- Other minor changes

1 files changed, 35 insertions, 37 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index 7207cdc4581..4bb9dc0c59f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -346,43 +346,7 @@ class User < ActiveRecord::Base
     # Return (create if necessary) the ghost user. The ghost user
     # owns records previously belonging to deleted users.
     def ghost
-      ghost_user = User.find_by_ghost(true)
-
-      ghost_user ||
-        begin
-          # Since we only want a single ghost user in an instance, we use an
-          # exclusive lease to ensure than this block is never run concurrently.
-          lease_key = "ghost_user_creation"
-          lease = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.minute.to_i)
-
-          until uuid = lease.try_obtain
-            # Keep trying until we obtain the lease. To prevent hammering Redis too
-            # much we'll wait for a bit between retries.
-            sleep(1)
-          end
-
-          # Recheck if a ghost user is already present (one might have been)
-          # added between the time we last checked (first line of this method)
-          # and the time we acquired the lock.
-          ghost_user = User.find_by_ghost(true)
-          return ghost_user if ghost_user.present?
-
-          uniquify = Uniquify.new
-
-          username = uniquify.string("ghost", -> (s) { User.find_by_username(s) })
-
-          email = uniquify.string(
-            -> (n) { "ghost#{n}@example.com" },
-            -> (s) { User.find_by_email(s) }
-          )
-
-          User.create(
-            username: username, password: Devise.friendly_token,
-            email: email, name: "Ghost User", state: :blocked, ghost: true
-          )
-        ensure
-          Gitlab::ExclusiveLease.cancel(lease_key, uuid)
-        end
+      User.find_by_ghost(true) || create_ghost_user
     end
   end
 
@@ -1052,4 +1016,38 @@ class User < ActiveRecord::Base
       super
     end
   end
+
+  def self.create_ghost_user
+    # Since we only want a single ghost user in an instance, we use an
+    # exclusive lease to ensure than this block is never run concurrently.
+    lease_key = "ghost_user_creation"
+    lease = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.minute.to_i)
+
+    until uuid = lease.try_obtain
+      # Keep trying until we obtain the lease. To prevent hammering Redis too
+      # much we'll wait for a bit between retries.
+      sleep(1)
+    end
+
+    # Recheck if a ghost user is already present. One might have been
+    # added between the time we last checked (first line of this method)
+    # and the time we acquired the lock.
+    ghost_user = User.find_by_ghost(true)
+    return ghost_user if ghost_user.present?
+
+    uniquify = Uniquify.new
+
+    username = uniquify.string("ghost") { |s| User.find_by_username(s) }
+
+    email = uniquify.string(-> (n) { "ghost#{n}@example.com" }) do |s|
+      User.find_by_email(s)
+    end
+
+    User.create(
+      username: username, password: Devise.friendly_token,
+      email: email, name: "Ghost User", state: :blocked, ghost: true
+    )
+  ensure
+    Gitlab::ExclusiveLease.cancel(lease_key, uuid)
+  end
 end