diff --git a/app/jobs/destroy_project_job.rb b/app/jobs/destroy_project_job.rb index 372e77240a..1e7fd2fe70 100644 --- a/app/jobs/destroy_project_job.rb +++ b/app/jobs/destroy_project_job.rb @@ -4,8 +4,7 @@ class DestroyProjectJob < ApplicationJob include Redmine::I18n def self.schedule(project, user: User.current) - # make the project (and any children) disappear immediately - project.self_and_descendants.update_all status: Project::STATUS_SCHEDULED_FOR_DELETION + Project.mark_for_deletion(project) perform_later project.id, user.id, user.remote_ip end diff --git a/app/jobs/destroy_projects_job.rb b/app/jobs/destroy_projects_job.rb index dba1213dcd..879bdc2363 100644 --- a/app/jobs/destroy_projects_job.rb +++ b/app/jobs/destroy_projects_job.rb @@ -4,10 +4,7 @@ class DestroyProjectsJob < ApplicationJob include Redmine::I18n def self.schedule(projects_to_delete, user: User.current) - # make the projects disappear immediately - projects_to_delete.each do |project| - project.self_and_descendants.update_all status: Project::STATUS_SCHEDULED_FOR_DELETION - end + Project.mark_for_deletion(projects_to_delete) perform_later(projects_to_delete.map(&:id), user.id, user.remote_ip) end diff --git a/app/models/project.rb b/app/models/project.rb index d15c298827..4d2422c9a8 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -410,6 +410,21 @@ class Project < ApplicationRecord self.status == STATUS_ARCHIVED end + # Atomically marks one or more projects (and all their descendants) as + # scheduled for deletion. Holds the nested-set lock and resolves the + # descendant set by id so a concurrent create/rename/move/destroy cannot + # shift lft/rgt out from under a stale cached range. + def self.mark_for_deletion(projects) + transaction do + order(:id).lock.ids + ids = Array(projects).flat_map do |p| + p.reload + p.self_and_descendants.pluck(:id) + end + where(id: ids).update_all(status: STATUS_SCHEDULED_FOR_DELETION) + end + end + def scheduled_for_deletion? self.status == STATUS_SCHEDULED_FOR_DELETION end diff --git a/test/unit/jobs/destroy_project_job_test.rb b/test/unit/jobs/destroy_project_job_test.rb index e2a4fa28d5..829af903b9 100644 --- a/test/unit/jobs/destroy_project_job_test.rb +++ b/test/unit/jobs/destroy_project_job_test.rb @@ -36,6 +36,33 @@ class DestroyProjectJobTest < ActiveJob::TestCase end end + test "schedule must not mark unrelated projects when the nested set is rebalanced between load and update_all" do + # Same race as the DestroyProjectsJob version, but on the + # single-project path: the window is just the gap between the + # controller load and the schedule call rather than spanning a loop. + target = @project # Project 1 (eCookbook) + victim = Project.find(2) # onlinestore, unrelated root + cached_lft, cached_rgt = target.lft, target.rgt + + assert_not_equal Project::STATUS_SCHEDULED_FOR_DELETION, victim.status + + # trigger nested set rebalancing while target is loaded + victim.update!(name: 'AAA renamed earlier than ecookbook') + victim.reload + assert victim.lft >= cached_lft && victim.rgt <= cached_rgt, + "test setup: victim should now sit inside target's stale range" + + # target is intentionally NOT reloaded + assert_equal cached_lft, target.lft + assert_equal cached_rgt, target.rgt + + DestroyProjectJob.schedule target, user: @user + + victim.reload + assert_not_equal Project::STATUS_SCHEDULED_FOR_DELETION, victim.status, + "Project #{victim.id} (#{victim.name}) was marked for deletion even though it was not the project passed to schedule." + end + test "schedule should enqueue job" do DestroyProjectJob.schedule @project, user: @user assert_enqueued_with( diff --git a/test/unit/jobs/destroy_projects_job_test.rb b/test/unit/jobs/destroy_projects_job_test.rb index a6b0f072b4..957716f68b 100644 --- a/test/unit/jobs/destroy_projects_job_test.rb +++ b/test/unit/jobs/destroy_projects_job_test.rb @@ -37,6 +37,42 @@ class DestroyProjectsJobTest < ActiveJob::TestCase end end + test "schedule must not mark unrelated projects when the nested set is rebalanced between load and update_all" do + # Reproduces the race condition where projects[].self_and_descendants + # uses cached in-memory lft/rgt to build a range UPDATE. When the + # nested set is rebalanced (concurrent project create/rename/move/ + # destroy) between the controller load and the update, the cached + # range can match unrelated projects, which then get status=10 even + # though their ids are never passed to perform_later -> the + # "vanished but not deleted" customer-visible symptom. + target = Project.find(1) # eCookbook, root w/ descendants + victim = Project.find(2) # onlinestore, unrelated root + cached_lft, cached_rgt = target.lft, target.rgt + + assert_not_equal Project::STATUS_SCHEDULED_FOR_DELETION, victim.status + + # Simulate a concurrent transaction renaming the unrelated root so it + # sorts before `target`. before_update :move_in_nested_set fires (see + # project_nested_set.rb:28-32) and rebalances the tree under + # lock_nested_set. After this, the database lft/rgt of every project + # has shifted, but our in-memory `target` still holds the values it + # had when it was loaded. + victim.update!(name: 'AAA renamed earlier than ecookbook') + victim.reload + assert victim.lft >= cached_lft && victim.rgt <= cached_rgt, + "test setup: victim should now sit inside target's stale range" + + # in-memory target is intentionally NOT reloaded + assert_equal cached_lft, target.lft + assert_equal cached_rgt, target.rgt + + DestroyProjectsJob.schedule [target], user: @user + + victim.reload + assert_not_equal Project::STATUS_SCHEDULED_FOR_DELETION, victim.status, + "Project #{victim.id} (#{victim.name}) was marked for deletion even though it was not in the deletion id list. " + end + test "schedule should enqueue job" do assert_enqueued_with( job: DestroyProjectsJob,