Merge branch '23223-group-deletion-race-condition' into 'master'

Remove race condition while deleting groups

## What does this MR do?

The intended flow during a group deletion is:

```
  Soft-delete group (sync) -> Delete group projects (async) -> Hard-delete group (async)
```

The soft-delete was run in a transaction, which was committed only after
the async job (for hard-deletion) was kicked off. There was a race
condition here - the soft-delete transaction could complete _after_ the
hard delete completed, leaving a soft-deleted record in the database.

This MR removes this race condition. There is no need to run the
soft-delete in a transaction. The soft-delete completes before the async
job is kicked off.

This MR also adds a migration to delete all existing (soft-deleted) groups left in an inconsistent state
due to this bug.

- Closes #23223 
- EE merge request: gitlab-org/gitlab-ee!886

See merge request !7528

app/services/destroy_group_service.rb

@@ -6,12 +6,10 @@ class DestroyGroupService
   end
 
   def async_execute
-    group.transaction do
-      # Soft delete via paranoia gem
-      group.destroy
-      job_id = GroupDestroyWorker.perform_async(group.id, current_user.id)
-      Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}")
-    end
+    # Soft delete via paranoia gem
+    group.destroy
+    job_id = GroupDestroyWorker.perform_async(group.id, current_user.id)
+    Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}")
   end
 
   def execute

changelogs/unreleased/23223-group-deletion-race-condition.yml

+---
+title: Fix race condition during group deletion and remove stale records present due to this bug
+merge_request: 7528
+author: Timothy Andrew

db/migrate/20161117114805_remove_undeleted_groups.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveUndeletedGroups < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def up
+    execute "DELETE FROM namespaces WHERE deleted_at IS NOT NULL;"
+  end
+
+  def down
+    # This is an irreversible migration;
+    # If someone is trying to rollback for other reasons, we should not throw an Exception.
+    # raise ActiveRecord::IrreversibleMigration
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20161113184239) do
+ActiveRecord::Schema.define(version: 20161117114805) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"

spec/services/destroy_group_service_spec.rb

 require 'spec_helper'
 
 describe DestroyGroupService, services: true do
+  include DatabaseConnectionHelpers
+
   let!(:user) { create(:user) }
   let!(:group) { create(:group) }
   let!(:project) { create(:project, namespace: group) }
@@ -50,6 +52,44 @@ describe DestroyGroupService, services: true do
 
   describe 'asynchronous delete' do
     it_behaves_like 'group destruction', true
+
+    context 'potential race conditions' do
+      context "when the `GroupDestroyWorker` task runs immediately" do
+        it "deletes the group" do
+          # Commit the contents of this spec's transaction so far
+          # so subsequent db connections can see it.
+          #
+          # DO NOT REMOVE THIS LINE, even if you see a WARNING with "No
+          # transaction is currently in progress". Without this, this
+          # spec will always be green, since the group created in setup
+          # cannot be seen by any other connections / threads in this spec.
+          Group.connection.commit_db_transaction
+
+          group_record = run_with_new_database_connection do |conn|
+            conn.execute("SELECT * FROM namespaces WHERE id = #{group.id}").first
+          end
+
+          expect(group_record).not_to be_nil
+
+          # Execute the contents of `GroupDestroyWorker` in a separate thread, to
+          # simulate data manipulation by the Sidekiq worker (different database
+          # connection / transaction).
+          expect(GroupDestroyWorker).to receive(:perform_async).and_wrap_original do |m, group_id, user_id|
+            Thread.new { m[group_id, user_id] }.join(5)
+          end
+
+          # Kick off the initial group destroy in a new thread, so that
+          # it doesn't share this spec's database transaction.
+          Thread.new { DestroyGroupService.new(group, user).async_execute }.join(5)
+
+          group_record = run_with_new_database_connection do |conn|
+            conn.execute("SELECT * FROM namespaces WHERE id = #{group.id}").first
+          end
+
+          expect(group_record).to be_nil
+        end
+      end
+    end
   end
 
   describe 'synchronous delete' do

spec/support/database_connection_helpers.rb

+module DatabaseConnectionHelpers
+  def run_with_new_database_connection
+    pool = ActiveRecord::Base.connection_pool
+    conn = pool.checkout
+    yield conn
+  ensure
+    pool.checkin(conn)
+  end
+end