diff options
author | Rémy Coutable <remy@rymai.me> | 2016-11-18 13:39:06 +0000 |
---|---|---|
committer | Alejandro Rodríguez <alejorro70@gmail.com> | 2016-11-18 21:55:44 +0000 |
commit | 673e76073ea889fa10c66823f1780e745f501182 (patch) | |
tree | 4fbe8be54936ca3d0772df84182ea0dbd5d86638 | |
parent | 7835d99cf9052893069a8f4f19184278cca70459 (diff) | |
download | gitlab-ce-673e76073ea889fa10c66823f1780e745f501182.tar.gz |
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
-rw-r--r-- | app/services/destroy_group_service.rb | 10 | ||||
-rw-r--r-- | changelogs/unreleased/23223-group-deletion-race-condition.yml | 4 | ||||
-rw-r--r-- | db/migrate/20161117114805_remove_undeleted_groups.rb | 16 | ||||
-rw-r--r-- | db/schema.rb | 2 | ||||
-rw-r--r-- | spec/services/destroy_group_service_spec.rb | 40 | ||||
-rw-r--r-- | spec/support/database_connection_helpers.rb | 9 |
6 files changed, 74 insertions, 7 deletions
diff --git a/app/services/destroy_group_service.rb b/app/services/destroy_group_service.rb index 0081364b8aa..a880952e274 100644 --- a/app/services/destroy_group_service.rb +++ b/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 diff --git a/changelogs/unreleased/23223-group-deletion-race-condition.yml b/changelogs/unreleased/23223-group-deletion-race-condition.yml new file mode 100644 index 00000000000..6f22e85fb4b --- /dev/null +++ b/changelogs/unreleased/23223-group-deletion-race-condition.yml @@ -0,0 +1,4 @@ +--- +title: Fix race condition during group deletion and remove stale records present due to this bug +merge_request: 7528 +author: Timothy Andrew diff --git a/db/migrate/20161117114805_remove_undeleted_groups.rb b/db/migrate/20161117114805_remove_undeleted_groups.rb new file mode 100644 index 00000000000..ebc2d974ae0 --- /dev/null +++ b/db/migrate/20161117114805_remove_undeleted_groups.rb @@ -0,0 +1,16 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 42cf9515860..db2ce689afc 100644 --- a/db/schema.rb +++ b/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" diff --git a/spec/services/destroy_group_service_spec.rb b/spec/services/destroy_group_service_spec.rb index da724643604..538e85cdc89 100644 --- a/spec/services/destroy_group_service_spec.rb +++ b/spec/services/destroy_group_service_spec.rb @@ -1,6 +1,8 @@ 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 diff --git a/spec/support/database_connection_helpers.rb b/spec/support/database_connection_helpers.rb new file mode 100644 index 00000000000..763329499f0 --- /dev/null +++ b/spec/support/database_connection_helpers.rb @@ -0,0 +1,9 @@ +module DatabaseConnectionHelpers + def run_with_new_database_connection + pool = ActiveRecord::Base.connection_pool + conn = pool.checkout + yield conn + ensure + pool.checkin(conn) + end +end |