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

4 files changed, 50 insertions, 46 deletions

diff --git a/app/models/concerns/uniquify.rb b/app/models/concerns/uniquify.rb
index 6734472bc6d..a7fe5951b6e 100644
--- a/app/models/concerns/uniquify.rb
+++ b/app/models/concerns/uniquify.rb
@@ -1,15 +1,15 @@
 class Uniquify
   # Return a version of the given 'base' string that is unique
   # by appending a counter to it. Uniqueness is determined by
-  # repeated calls to `exists_fn`.
+  # repeated calls to the passed block.
   #
   # If `base` is a function/proc, we expect that calling it with a
   # candidate counter returns a string to test/return.
-  def string(base, exists_fn)
+  def string(base)
     @base = base
     @counter = nil
 
-    increment_counter! while exists_fn[base_string]
+    increment_counter! while yield(base_string)
     base_string
   end
 
@@ -24,6 +24,7 @@ class Uniquify
   end
 
   def increment_counter!
-    @counter = @counter ? @counter.next : 1
+    @counter ||= 0
+    @counter += 1
   end
 end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 8cc3c1473e2..3137dd32f93 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -99,7 +99,7 @@ class Namespace < ActiveRecord::Base
       path = "blank" if path.blank?
 
       uniquify = Uniquify.new
-      uniquify.string(path, -> (s) { Namespace.find_by_path_or_name(s) })
+      uniquify.string(path) { |s| Namespace.find_by_path_or_name(s) }
     end
   end
 
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
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
index bf1e49734cc..523279944ae 100644
--- a/app/services/users/destroy_service.rb
+++ b/app/services/users/destroy_service.rb
@@ -37,13 +37,18 @@ module Users
     end
 
     private
-    
+
     def move_issues_to_ghost_user(user)
+      # Block the user before moving issues to prevent a data race.
+      # If the user creates an issue after `move_issues_to_ghost_user`
+      # runs and before the user is destroyed, the destroy will fail with
+      # an exception. We block the user so that issues can't be created
+      # after `move_issues_to_ghost_user` runs and before the destroy happens.
+      user.block
+
       ghost_user = User.ghost
 
-      Issue.transaction do
-        user.issues.update_all(author_id: ghost_user.id)
-      end
+      user.issues.update_all(author_id: ghost_user.id)
 
       user.reload
     end