Implement review comments from @DouweM and @nick.thomas.

1. Use an advisory lock to guarantee the absence of concurrency in `User.ghost`,
to prevent data races from creating more than one ghost, or preventing the
creation of ghost users by causing validation errors.

2. Use `update_all` instead of updating issues one-by-one.

3 files changed, 85 insertions, 2 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index 13529c9997a..e4adcd6cde9 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -351,6 +351,17 @@ class User < ActiveRecord::Base
 
       ghost_user ||
         begin
+          # Since we only want a single ghost user in an instance, we use an
+          # advisory lock to ensure than this block is never run concurrently.
+          advisory_lock = Gitlab::Database::AdvisoryLocking.new(:ghost_user)
+          advisory_lock.lock
+
+          # 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.with_state(:ghost).first
+          return ghost_user if ghost_user.present?
+
           uniquify = Uniquify.new
 
           username = uniquify.string("ghost", -> (s) { User.find_by_username(s) })
@@ -364,6 +375,8 @@ class User < ActiveRecord::Base
             username: username, password: Devise.friendly_token,
             email: email, name: "Ghost User", state: :ghost
           )
+        ensure
+          advisory_lock.unlock
         end
     end
   end
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
index d88b81c04e2..bf1e49734cc 100644
--- a/app/services/users/destroy_service.rb
+++ b/app/services/users/destroy_service.rb
@@ -37,12 +37,12 @@ module Users
     end
 
     private
-
+    
     def move_issues_to_ghost_user(user)
       ghost_user = User.ghost
 
       Issue.transaction do
-        user.issues.each { |issue| issue.update!(author: ghost_user) }
+        user.issues.update_all(author_id: ghost_user.id)
       end
 
       user.reload
diff --git a/lib/gitlab/database/advisory_locking.rb b/lib/gitlab/database/advisory_locking.rb
new file mode 100644
index 00000000000..950898288be
--- /dev/null
+++ b/lib/gitlab/database/advisory_locking.rb
@@ -0,0 +1,70 @@
+# An advisory lock is an application-level database lock which isn't tied
+# to a specific table or row.
+#
+# Postgres names its advisory locks with integers, while MySQL uses strings.
+# We support both here by using a `LOCK_TYPES` map of symbols to integers.
+# The symbol (stringified) is used for MySQL, and the corresponding integer
+# is used for Postgres.
+module Gitlab
+  module Database
+    class AdvisoryLocking
+      LOCK_TYPES = {
+        ghost_user: 1
+      }
+
+      def initialize(lock_type)
+        @lock_type = lock_type
+      end
+
+      def lock
+        ensure_valid_lock_type!
+
+        query =
+          if Gitlab::Database.postgresql?
+            Arel::SelectManager.new(ActiveRecord::Base).project(
+              Arel::Nodes::NamedFunction.new("pg_advisory_lock", [LOCK_TYPES[@lock_type]])
+            )
+          elsif Gitlab::Database.mysql?
+            Arel::SelectManager.new(ActiveRecord::Base).project(
+              Arel::Nodes::NamedFunction.new("get_lock", [Arel.sql("'#{@lock_type}'"), -1])
+            )
+          end
+
+        run_query(query)
+      end
+
+      def unlock
+        ensure_valid_lock_type!
+
+        query =
+          if Gitlab::Database.postgresql?
+            Arel::SelectManager.new(ActiveRecord::Base).project(
+              Arel::Nodes::NamedFunction.new("pg_advisory_unlock", [LOCK_TYPES[@lock_type]])
+            )
+          elsif Gitlab::Database.mysql?
+            Arel::SelectManager.new(ActiveRecord::Base).project(
+              Arel::Nodes::NamedFunction.new("release_lock", [Arel.sql("'#{@lock_type}'")])
+            )
+          end
+
+        run_query(query)
+      end
+
+      private
+
+      def ensure_valid_lock_type!
+        unless valid_lock_type?
+          raise RuntimeError, "Trying to use an advisory lock with an invalid lock type, #{@lock_type}."
+        end
+      end
+
+      def valid_lock_type?
+        LOCK_TYPES.keys.include?(@lock_type)
+      end
+
+      def run_query(arel_query)
+        ActiveRecord::Base.connection.execute(arel_query.to_sql)
+      end
+    end
+  end
+end