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

6 files changed, 62 insertions, 61 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
diff --git a/spec/models/concerns/uniquify_spec.rb b/spec/models/concerns/uniquify_spec.rb
index 4e06a1f4c31..83187d732e4 100644
--- a/spec/models/concerns/uniquify_spec.rb
+++ b/spec/models/concerns/uniquify_spec.rb
@@ -1,38 +1,31 @@
 require 'spec_helper'
 
 describe Uniquify, models: true do
+  let(:uniquify) { described_class.new }
+
   describe "#string" do
     it 'returns the given string if it does not exist' do
-      uniquify = Uniquify.new
-
-      result = uniquify.string('test_string', -> (s) { false })
+      result = uniquify.string('test_string') { |s| false }
 
       expect(result).to eq('test_string')
     end
 
     it 'returns the given string with a counter attached if the string exists' do
-      uniquify = Uniquify.new
-
-      result = uniquify.string('test_string', -> (s) { true if s == 'test_string' })
+      result = uniquify.string('test_string') { |s| s == 'test_string' }
 
       expect(result).to eq('test_string1')
     end
 
     it 'increments the counter for each candidate string that also exists' do
-      uniquify = Uniquify.new
-
-      result = uniquify.string('test_string', -> (s) { true if s == 'test_string' || s == 'test_string1' })
+      result = uniquify.string('test_string') { |s| s == 'test_string' || s == 'test_string1' }
 
       expect(result).to eq('test_string2')
     end
 
     it 'allows passing in a base function that defines the location of the counter' do
-      uniquify = Uniquify.new
-
-      result = uniquify.string(
-        -> (counter) { "test_#{counter}_string" },
-        -> (s) { true if s == 'test__string' }
-      )
+      result = uniquify.string(-> (counter) { "test_#{counter}_string" }) do |s|
+        s == 'test__string'
+      end
 
       expect(result).to eq('test_1_string')
     end
diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_spec.rb
index 4f0834064e5..922e82445d0 100644
--- a/spec/services/users/destroy_spec.rb
+++ b/spec/services/users/destroy_spec.rb
@@ -47,6 +47,10 @@ describe Users::DestroyService, services: true do
 
           expect(migrated_issue.author).to eq(User.ghost)
         end
+
+        it 'blocks the user before migrating issues to the "Ghost User' do
+          expect(user).to be_blocked
+        end
       end
 
       context "for an issue the user was assigned to" do