Merge branch 'sh-fix-destroy-user-race' into 'master'

Fix race condition where a namespace would be deleted before a project was deleted

Closes #30334 and #30306

See merge request !10389

app/controllers/registrations_controller.rb

@@ -25,12 +25,12 @@ class RegistrationsController < Devise::RegistrationsController
   end
 
   def destroy
-    Users::DestroyService.new(current_user).execute(current_user)
+    DeleteUserWorker.perform_async(current_user.id, current_user.id)
 
     respond_to do |format|
       format.html do
         session.try(:destroy)
-        redirect_to new_user_session_path, notice: "Account successfully removed."
+        redirect_to new_user_session_path, notice: "Account scheduled for removal."
       end
     end
   end

app/services/users/destroy_service.rb

@@ -20,10 +20,10 @@ module Users
         Groups::DestroyService.new(group, current_user).execute
       end
 
-      user.personal_projects.each do |project|
+      user.personal_projects.with_deleted.each do |project|
         # Skip repository removal because we remove directory with namespace
         # that contain all this repositories
-        ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
+        ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
       end
 
       move_issues_to_ghost_user(user)

changelogs/unreleased/sh-fix-destroy-user-race.yml

+---
+title: Fix race condition where a namespace would be deleted before a project was
+  deleted
+merge_request:
+author:

lib/api/users.rb

@@ -293,7 +293,7 @@ module API
         user = User.find_by(id: params[:id])
         not_found!('User') unless user
 
-        ::Users::DestroyService.new(current_user).execute(user)
+        DeleteUserWorker.perform_async(current_user.id, user.id)
       end
 
       desc 'Block a user. Available only for admins.'

spec/controllers/registrations_controller_spec.rb

@@ -68,4 +68,20 @@ describe RegistrationsController do
       end
     end
   end
+
+  describe '#destroy' do
+    let(:user) { create(:user) }
+
+    before do
+      sign_in(user)
+    end
+
+    it 'schedules the user for destruction' do
+      expect(DeleteUserWorker).to receive(:perform_async).with(user.id, user.id)
+
+      post(:destroy)
+
+      expect(response.status).to eq(302)
+    end
+  end
 end

spec/requests/api/users_spec.rb

@@ -676,7 +676,7 @@ describe API::Users, api: true  do
     before { admin }
 
     it "deletes user" do
-      delete api("/users/#{user.id}", admin)
+      Sidekiq::Testing.inline! { delete api("/users/#{user.id}", admin) }
 
       expect(response).to have_http_status(204)
       expect { User.find(user.id) }.to raise_error ActiveRecord::RecordNotFound
@@ -684,23 +684,23 @@ describe API::Users, api: true  do
     end
 
     it "does not delete for unauthenticated user" do
-      delete api("/users/#{user.id}")
+      Sidekiq::Testing.inline! { delete api("/users/#{user.id}") }
       expect(response).to have_http_status(401)
     end
 
     it "is not available for non admin users" do
-      delete api("/users/#{user.id}", user)
+      Sidekiq::Testing.inline! { delete api("/users/#{user.id}", user) }
       expect(response).to have_http_status(403)
     end
 
     it "returns 404 for non-existing user" do
-      delete api("/users/999999", admin)
+      Sidekiq::Testing.inline! { delete api("/users/999999", admin) }
       expect(response).to have_http_status(404)
       expect(json_response['message']).to eq('404 User Not Found')
     end
 
     it "returns a 404 for invalid ID" do
-      delete api("/users/ASDF", admin)
+      Sidekiq::Testing.inline! { delete api("/users/ASDF", admin) }
 
       expect(response).to have_http_status(404)
     end

spec/services/users/destroy_spec.rb

@@ -17,13 +17,28 @@ describe Users::DestroyService, services: true do
         expect { Namespace.with_deleted.find(user.namespace.id) }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
-      it 'will delete the project in the near future' do
-        expect_any_instance_of(Projects::DestroyService).to receive(:async_execute).once
+      it 'will delete the project' do
+        expect_any_instance_of(Projects::DestroyService).to receive(:execute).once
 
         service.execute(user)
       end
     end
 
+    context 'projects in pending_delete' do
+      before do
+        project.pending_delete = true
+        project.save
+      end
+
+      it 'destroys a project in pending_delete' do
+        expect_any_instance_of(Projects::DestroyService).to receive(:execute).once
+
+        service.execute(user)
+
+        expect { Project.find(project.id) }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+
     context "a deleted user's issues" do
       let(:project) { create(:project) }