From 82ddda718f933928d9f83d8dae95c750f7f31491 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Fri, 25 Nov 2016 15:04:33 +0100
Subject: Fix race condition during project deletion
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/services/projects/destroy_service.rb           |  8 ++---
 ...-soft-deleted-projects-not-actually-deleted.yml |  4 +++
 spec/services/projects/destroy_service_spec.rb     | 39 ++++++++++++++++++++++
 3 files changed, 46 insertions(+), 5 deletions(-)
 create mode 100644 changelogs/unreleased/fix-soft-deleted-projects-not-actually-deleted.yml

diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index a08c6fcd94b..1a4203be730 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -7,11 +7,9 @@ module Projects
     DELETED_FLAG = '+deleted'
 
     def async_execute
-      project.transaction do
-        project.update_attribute(:pending_delete, true)
-        job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params)
-        Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.path_with_namespace} with job ID #{job_id}")
-      end
+      project.update_attribute(:pending_delete, true)
+      job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params)
+      Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.path_with_namespace} with job ID #{job_id}")
     end
 
     def execute
diff --git a/changelogs/unreleased/fix-soft-deleted-projects-not-actually-deleted.yml b/changelogs/unreleased/fix-soft-deleted-projects-not-actually-deleted.yml
new file mode 100644
index 00000000000..f6a0a3b2307
--- /dev/null
+++ b/changelogs/unreleased/fix-soft-deleted-projects-not-actually-deleted.yml
@@ -0,0 +1,4 @@
+---
+title: Fix race condition during project deletion
+merge_request: 
+author: 
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index 7dcd03496bb..bc795b41839 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -1,6 +1,7 @@
 require 'spec_helper'
 
 describe Projects::DestroyService, services: true do
+  include DatabaseConnectionHelpers
   let!(:user) { create(:user) }
   let!(:project) { create(:project, namespace: user.namespace) }
   let!(:path) { project.repository.path_to_repo }
@@ -45,6 +46,44 @@ describe Projects::DestroyService, services: true do
     end
   end
 
+  context 'potential race conditions' do
+    context "when the `ProjectDestroyWorker` task runs immediately" do
+      it "deletes the project" 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 project created in setup
+        # cannot be seen by any other connections / threads in this spec.
+        Project.connection.commit_db_transaction
+
+        project_record = run_with_new_database_connection do |conn|
+          conn.execute("SELECT * FROM projects WHERE id = #{project.id}").first
+        end
+
+        expect(project_record).not_to be_nil
+
+        # Execute the contents of `ProjectDestroyWorker` in a separate thread, to
+        # simulate data manipulation by the Sidekiq worker (different database
+        # connection / transaction).
+        expect(ProjectDestroyWorker).to receive(:perform_async).and_wrap_original do |m, project_id, user_id, params|
+          Thread.new { m[project_id, user_id, params] }.join(5)
+        end
+
+        # Kick off the initial project destroy in a new thread, so that
+        # it doesn't share this spec's database transaction.
+        Thread.new { described_class.new(project, user).async_execute }.join(5)
+
+        project_record = run_with_new_database_connection do |conn|
+          conn.execute("SELECT * FROM projects WHERE id = #{project.id}").first
+        end
+
+        expect(project_record).to be_nil
+      end
+    end
+  end
+
   context 'container registry' do
     before do
       stub_container_registry_config(enabled: true)
-- 
cgit v1.2.1