From 9e513d3f2eb6b242728a8a68c9c0ddcb9ea26cb6 Mon Sep 17 00:00:00 2001
From: bmarjanovic <bmarjanovic@gitlab.com>
Date: Fri, 6 Dec 2024 17:15:32 +0100
Subject: [PATCH 1/4] Re-queue migration to delete orphaned groups

---
 .../delete_orphaned_groups.yml                |  2 +-
 ...1112163029_queue_delete_orphaned_groups.rb | 15 ++----
 ...45_queue_requeue_delete_orphaned_groups.rb | 31 ++++++++++++
 db/schema_migrations/20241206154945           |  1 +
 .../delete_orphaned_groups.rb                 |  4 +-
 .../delete_orphaned_groups_spec.rb            |  6 ++-
 ...63029_queue_delete_orphaned_groups_spec.rb | 38 +-------------
 ...eue_requeue_delete_orphaned_groups_spec.rb | 49 +++++++++++++++++++
 8 files changed, 95 insertions(+), 51 deletions(-)
 create mode 100644 db/post_migrate/20241206154945_queue_requeue_delete_orphaned_groups.rb
 create mode 100644 db/schema_migrations/20241206154945
 create mode 100644 spec/migrations/20241206154945_queue_requeue_delete_orphaned_groups_spec.rb

diff --git a/db/docs/batched_background_migrations/delete_orphaned_groups.yml b/db/docs/batched_background_migrations/delete_orphaned_groups.yml
index c8d84dc1c66b7b..b91d5698f15b7a 100644
--- a/db/docs/batched_background_migrations/delete_orphaned_groups.yml
+++ b/db/docs/batched_background_migrations/delete_orphaned_groups.yml
@@ -4,5 +4,5 @@ description: Deletes orhpaned groups whose parent's does not exist
 feature_category: groups_and_projects
 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/172420
 milestone: '17.7'
-queued_migration_version: 20241112163029
+queued_migration_version: 20241206154945
 finalized_by: # version of the migration that finalized this BBM
diff --git a/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb b/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb
index 4e2d7d702b40ec..1f42797685a5ea 100644
--- a/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb
+++ b/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb
@@ -10,19 +10,12 @@ class QueueDeleteOrphanedGroups < Gitlab::Database::Migration[2.2]
   SUB_BATCH_SIZE = 100
 
   def up
-    return unless Gitlab.com_except_jh? && !Gitlab.staging?
-
-    queue_batched_background_migration(
-      MIGRATION,
-      :namespaces,
-      :id,
-      job_interval: DELAY_INTERVAL,
-      batch_size: BATCH_SIZE,
-      sub_batch_size: SUB_BATCH_SIZE
-    )
+    # no-op because there was a bug in the original migration, which has been
+    # fixed by https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174995
   end
 
   def down
-    delete_batched_background_migration(MIGRATION, :namespaces, :id, [])
+    # no-op because there was a bug in the original migration, which has been
+    # fixed in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174995
   end
 end
diff --git a/db/post_migrate/20241206154945_queue_requeue_delete_orphaned_groups.rb b/db/post_migrate/20241206154945_queue_requeue_delete_orphaned_groups.rb
new file mode 100644
index 00000000000000..1646b0cf7a9053
--- /dev/null
+++ b/db/post_migrate/20241206154945_queue_requeue_delete_orphaned_groups.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+class QueueRequeueDeleteOrphanedGroups < Gitlab::Database::Migration[2.2]
+  milestone '17.7'
+  restrict_gitlab_migration gitlab_schema: :gitlab_main
+
+  MIGRATION = "DeleteOrphanedGroups"
+  DELAY_INTERVAL = 2.minutes
+  BATCH_SIZE = 1000
+  SUB_BATCH_SIZE = 100
+
+  def up
+    return unless Gitlab.com_except_jh? && !Gitlab.staging?
+
+    # Clear previous background migration execution from QueueDeleteOrphanedGroups
+    delete_batched_background_migration(MIGRATION, :namespaces, :id, [])
+
+    queue_batched_background_migration(
+      MIGRATION,
+      :namespaces,
+      :id,
+      job_interval: DELAY_INTERVAL,
+      batch_size: BATCH_SIZE,
+      sub_batch_size: SUB_BATCH_SIZE
+    )
+  end
+
+  def down
+    delete_batched_background_migration(MIGRATION, :namespaces, :id, [])
+  end
+end
diff --git a/db/schema_migrations/20241206154945 b/db/schema_migrations/20241206154945
new file mode 100644
index 00000000000000..698f5d6872e140
--- /dev/null
+++ b/db/schema_migrations/20241206154945
@@ -0,0 +1 @@
+276b46068ee45bbafbe5c51cf466cf2cee0ec0c08d273899ccba3b5e05c93f1d
\ No newline at end of file
diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb
index a032ad40180b68..abe5d94bb5216a 100644
--- a/lib/gitlab/background_migration/delete_orphaned_groups.rb
+++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb
@@ -8,13 +8,15 @@ class DeleteOrphanedGroups < BatchedMigrationJob
 
       scope_to ->(relation) { relation.where(type: 'Group').where.not(parent_id: nil) }
 
+      DELAY = 1.second
+
       def perform
         each_sub_batch do |sub_batch|
           sub_batch
           .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id")
           .where(parent: { id: nil })
           .pluck(:id).each do |orphaned_group_id|
-            ::GroupDestroyWorker.perform(orphaned_group_id, admin_bot.id)
+            ::GroupDestroyWorker.perform_in(DELAY, orphaned_group_id, admin_bot.id)
           end
         end
       end
diff --git a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb
index f94d9256352a71..6f3a938873d5c1 100644
--- a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb
+++ b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb
@@ -46,11 +46,13 @@
       parent.destroy!
 
       orphaned_groups.each do |group|
-        expect(::GroupDestroyWorker).to receive(:perform).with(group.id, ::Users::Internal.admin_bot.id)
+        expect(::GroupDestroyWorker)
+          .to receive(:perform_in).with(described_class::DELAY, group.id, ::Users::Internal.admin_bot.id)
       end
 
       groups.each do |group|
-        expect(::GroupDestroyWorker).not_to receive(:perform).with(group.id, ::Users::Internal.admin_bot.id)
+        expect(::GroupDestroyWorker)
+          .not_to receive(:perform_in).with(described_class::DELAY, group.id, ::Users::Internal.admin_bot.id)
       end
 
       background_migration
diff --git a/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb b/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb
index d4e514b53dd2d9..ce37b8a865c5f9 100644
--- a/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb
+++ b/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb
@@ -8,42 +8,8 @@
 
   it 'does not schedule a new batched migration' do
     reversible_migration do |migration|
-      migration.before -> {
-        expect(batched_migration).not_to have_scheduled_batched_migration
-      }
-
-      migration.after -> {
-        expect(batched_migration).not_to have_scheduled_batched_migration
-      }
-    end
-  end
-
-  context 'when executed on .com' do
-    before do
-      allow(Gitlab).to receive(:com_except_jh?).and_return(true)
-    end
-
-    describe '#up' do
-      it 'schedules background migration' do
-        migrate!
-
-        expect(batched_migration).to have_scheduled_batched_migration(
-          table_name: :namespaces,
-          column_name: :id,
-          interval: described_class::DELAY_INTERVAL,
-          batch_size: described_class::BATCH_SIZE,
-          sub_batch_size: described_class::SUB_BATCH_SIZE
-        )
-      end
-    end
-
-    describe '#down' do
-      it 'removes scheduled background migrations' do
-        migrate!
-        schema_migrate_down!
-
-        expect(batched_migration).not_to have_scheduled_batched_migration
-      end
+      migration.before -> { expect(batched_migration).not_to have_scheduled_batched_migration }
+      migration.after -> { expect(batched_migration).not_to have_scheduled_batched_migration }
     end
   end
 end
diff --git a/spec/migrations/20241206154945_queue_requeue_delete_orphaned_groups_spec.rb b/spec/migrations/20241206154945_queue_requeue_delete_orphaned_groups_spec.rb
new file mode 100644
index 00000000000000..47aee132a0eb5d
--- /dev/null
+++ b/spec/migrations/20241206154945_queue_requeue_delete_orphaned_groups_spec.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!
+
+RSpec.describe QueueRequeueDeleteOrphanedGroups, migration: :gitlab_main, feature_category: :groups_and_projects do
+  let!(:batched_migration) { described_class::MIGRATION }
+
+  it 'does not schedule a new batched migration' do
+    reversible_migration do |migration|
+      migration.before -> {
+        expect(batched_migration).not_to have_scheduled_batched_migration
+      }
+
+      migration.after -> {
+        expect(batched_migration).not_to have_scheduled_batched_migration
+      }
+    end
+  end
+
+  context 'when executed on .com' do
+    before do
+      allow(Gitlab).to receive(:com_except_jh?).and_return(true)
+    end
+
+    describe '#up' do
+      it 'schedules background migration' do
+        migrate!
+
+        expect(batched_migration).to have_scheduled_batched_migration(
+          table_name: :namespaces,
+          column_name: :id,
+          interval: described_class::DELAY_INTERVAL,
+          batch_size: described_class::BATCH_SIZE,
+          sub_batch_size: described_class::SUB_BATCH_SIZE
+        )
+      end
+    end
+
+    describe '#down' do
+      it 'removes scheduled background migrations' do
+        migrate!
+        schema_migrate_down!
+
+        expect(batched_migration).not_to have_scheduled_batched_migration
+      end
+    end
+  end
+end
-- 
GitLab


From ed79d2cd54932a38d848c1792fee786c694d3213 Mon Sep 17 00:00:00 2001
From: bmarjanovic <bmarjanovic@gitlab.com>
Date: Mon, 9 Dec 2024 11:35:29 +0100
Subject: [PATCH 2/4] Moves migration to ee, and updates specs

---
 app/models/namespaces/traversal/linear.rb     |  3 +-
 app/services/groups/destroy_service.rb        |  2 +-
 .../maintain_elasticsearch_on_group_update.rb |  2 +-
 ee/app/models/ee/group.rb                     |  2 +-
 .../delete_orphaned_groups.rb                 | 37 +++++++++++
 .../delete_orphaned_groups_spec.rb            |  0
 .../delete_orphaned_groups.rb                 | 24 +-------
 lib/gitlab/hook_data/subgroup_builder.rb      |  2 +
 .../delete_orphaned_groups_spec.rb            | 61 -------------------
 9 files changed, 47 insertions(+), 86 deletions(-)
 create mode 100644 ee/lib/ee/gitlab/background_migration/delete_orphaned_groups.rb
 create mode 100644 ee/spec/lib/ee/gitlab/background_migration/delete_orphaned_groups_spec.rb
 delete mode 100644 spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb

diff --git a/app/models/namespaces/traversal/linear.rb b/app/models/namespaces/traversal/linear.rb
index 394e01843fdd86..54389e314d325b 100644
--- a/app/models/namespaces/traversal/linear.rb
+++ b/app/models/namespaces/traversal/linear.rb
@@ -233,7 +233,8 @@ def set_traversal_ids
         return if is_a?(Namespaces::ProjectNamespace)
 
         self.transient_traversal_ids = if parent_id
-                                         parent.traversal_ids + [id]
+                                         # remove safe navigation and `.to_a` after https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174995
+                                         parent&.traversal_ids.to_a + [id]
                                        else
                                          [id]
                                        end
diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb
index e868d2a2fa97fb..3f03c75ec437d8 100644
--- a/app/services/groups/destroy_service.rb
+++ b/app/services/groups/destroy_service.rb
@@ -118,7 +118,7 @@ def publish_event
       event = Groups::GroupDeletedEvent.new(
         data: {
           group_id: group.id,
-          root_namespace_id: group.root_ancestor.id
+          root_namespace_id: group.root_ancestor&.id.to_i # remove safe navigation and `.to_i` after https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174995
         }
       )
 
diff --git a/ee/app/models/concerns/elastic/maintain_elasticsearch_on_group_update.rb b/ee/app/models/concerns/elastic/maintain_elasticsearch_on_group_update.rb
index 854f938bdab838..bf16b5be45f487 100644
--- a/ee/app/models/concerns/elastic/maintain_elasticsearch_on_group_update.rb
+++ b/ee/app/models/concerns/elastic/maintain_elasticsearch_on_group_update.rb
@@ -25,7 +25,7 @@ def maintain_group_associations_on_update
 
     def maintain_group_associations_on_destroy
       delete_group_wiki_in_elastic if use_elasticsearch?
-      delete_group_associations
+      delete_group_associations if root_ancestor # remove if condition after https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174995
     end
 
     def sync_group_wiki_in_elastic
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index cb2590e3495da7..32b57b3396a44a 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -1155,7 +1155,7 @@ def execute_subgroup_hooks(event)
         # When this subgroup is removed, there is no point in this subgroup's webhook itself being notified
         # that `self` was removed. Rather, we should only care about notifying its ancestors
         # and hence we need to trigger the hooks starting only from its `parent` group.
-        parent.execute_hooks(data, :subgroup_hooks)
+        parent&.execute_hooks(data, :subgroup_hooks) # remove safe navigation after https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174995
       end
     end
 
diff --git a/ee/lib/ee/gitlab/background_migration/delete_orphaned_groups.rb b/ee/lib/ee/gitlab/background_migration/delete_orphaned_groups.rb
new file mode 100644
index 00000000000000..2e4ed8c05290c7
--- /dev/null
+++ b/ee/lib/ee/gitlab/background_migration/delete_orphaned_groups.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module BackgroundMigration
+      module DeleteOrphanedGroups
+        extend ActiveSupport::Concern
+        extend ::Gitlab::Utils::Override
+
+        prepended do
+          operation_name :delete_orphaned_group_records
+          feature_category :groups_and_projects
+
+          scope_to ->(relation) { relation.where(type: 'Group').where.not(parent_id: nil) }
+        end
+
+        override :perform
+        def perform
+          each_sub_batch do |sub_batch|
+            sub_batch
+            .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id")
+            .where(parent: { id: nil })
+            .pluck(:id).each do |orphaned_group_id|
+              ::GroupDestroyWorker.new.perform(orphaned_group_id, admin_bot.id)
+            end
+          end
+        end
+
+        private
+
+        def admin_bot
+          @_admin_bot ||= ::Users::Internal.admin_bot
+        end
+      end
+    end
+  end
+end
diff --git a/ee/spec/lib/ee/gitlab/background_migration/delete_orphaned_groups_spec.rb b/ee/spec/lib/ee/gitlab/background_migration/delete_orphaned_groups_spec.rb
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb
index abe5d94bb5216a..0ffaa7a62b7b4c 100644
--- a/lib/gitlab/background_migration/delete_orphaned_groups.rb
+++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb
@@ -3,29 +3,11 @@
 module Gitlab
   module BackgroundMigration
     class DeleteOrphanedGroups < BatchedMigrationJob
-      operation_name :delete_orphaned_group_records
       feature_category :groups_and_projects
 
-      scope_to ->(relation) { relation.where(type: 'Group').where.not(parent_id: nil) }
-
-      DELAY = 1.second
-
-      def perform
-        each_sub_batch do |sub_batch|
-          sub_batch
-          .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id")
-          .where(parent: { id: nil })
-          .pluck(:id).each do |orphaned_group_id|
-            ::GroupDestroyWorker.perform_in(DELAY, orphaned_group_id, admin_bot.id)
-          end
-        end
-      end
-
-      private
-
-      def admin_bot
-        @_admin_bot ||= Users::Internal.admin_bot
-      end
+      def perform; end
     end
   end
 end
+
+Gitlab::BackgroundMigration::DeleteOrphanedGroups.prepend_mod
diff --git a/lib/gitlab/hook_data/subgroup_builder.rb b/lib/gitlab/hook_data/subgroup_builder.rb
index a620219675a4af..5ce4151f0b7da8 100644
--- a/lib/gitlab/hook_data/subgroup_builder.rb
+++ b/lib/gitlab/hook_data/subgroup_builder.rb
@@ -34,6 +34,8 @@ def event_data(event)
       def group_data
         parent = group.parent
 
+        return super unless parent
+
         super.merge(
           parent_group_id: parent.id,
           parent_name: parent.name,
diff --git a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb
deleted file mode 100644
index 6f3a938873d5c1..00000000000000
--- a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb
+++ /dev/null
@@ -1,61 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::BackgroundMigration::DeleteOrphanedGroups, feature_category: :groups_and_projects do
-  let(:organization) { table(:organizations).create!(name: 'organization', path: 'organization') }
-  let(:namespaces) { table(:namespaces) }
-  let!(:parent) { namespaces.create!(name: 'Group', type: 'Group', path: 'space1', organization_id: organization.id) }
-  let!(:group) { namespaces.create!(name: 'GitLab', type: 'Group', path: 'group1', organization_id: organization.id) }
-
-  subject(:background_migration) do
-    described_class.new(
-      start_id: namespaces.without(parent).minimum(:id),
-      end_id: namespaces.maximum(:id),
-      batch_table: :namespaces,
-      batch_column: :id,
-      sub_batch_size: 1,
-      pause_ms: 0,
-      connection: ApplicationRecord.connection
-    ).perform
-  end
-
-  describe '#perform' do
-    before do
-      # Remove constraint so we can create invalid records
-      ApplicationRecord.connection.execute("ALTER TABLE namespaces DROP CONSTRAINT fk_7f813d8c90;")
-    end
-
-    after do
-      # Re-create constraint after the test
-      ApplicationRecord.connection.execute(<<~SQL)
-        ALTER TABLE ONLY namespaces ADD CONSTRAINT fk_7f813d8c90
-        FOREIGN KEY (parent_id) REFERENCES namespaces(id) ON DELETE RESTRICT NOT VALID;
-      SQL
-    end
-
-    it 'enqueues ::GroupDestroyWorker for each group whose parent\'s do not exist' do
-      orphaned_groups = (1..4).map do |i|
-        namespaces.create!(name: "Group #{i}", path: "orphaned_group_#{i}", type: 'Group', parent_id: parent.id,
-          organization_id: organization.id)
-      end
-      groups = (1..4).map do |i|
-        namespaces.create!(name: "Group #{i}", path: "group_#{i}", type: 'Group', parent_id: group.id,
-          organization_id: organization.id)
-      end
-      parent.destroy!
-
-      orphaned_groups.each do |group|
-        expect(::GroupDestroyWorker)
-          .to receive(:perform_in).with(described_class::DELAY, group.id, ::Users::Internal.admin_bot.id)
-      end
-
-      groups.each do |group|
-        expect(::GroupDestroyWorker)
-          .not_to receive(:perform_in).with(described_class::DELAY, group.id, ::Users::Internal.admin_bot.id)
-      end
-
-      background_migration
-    end
-  end
-end
-- 
GitLab


From 51441e33b5c6d6277b825ee5013d4b43009d7361 Mon Sep 17 00:00:00 2001
From: bmarjanovic <bmarjanovic@gitlab.com>
Date: Tue, 10 Dec 2024 09:29:47 +0100
Subject: [PATCH 3/4] Apply reviewer's feedback

---
 app/models/namespaces/traversal/linear.rb     |  2 +-
 app/services/groups/destroy_service.rb        |  2 +-
 .../maintain_elasticsearch_on_group_update.rb |  3 +-
 ee/app/models/ee/group.rb                     |  2 +-
 .../delete_orphaned_groups.rb                 | 37 --------------
 .../delete_orphaned_groups_spec.rb            |  0
 .../delete_orphaned_groups.rb                 | 22 ++++++--
 .../delete_orphaned_groups_spec.rb            | 50 +++++++++++++++++++
 8 files changed, 74 insertions(+), 44 deletions(-)
 delete mode 100644 ee/lib/ee/gitlab/background_migration/delete_orphaned_groups.rb
 delete mode 100644 ee/spec/lib/ee/gitlab/background_migration/delete_orphaned_groups_spec.rb
 create mode 100644 spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb

diff --git a/app/models/namespaces/traversal/linear.rb b/app/models/namespaces/traversal/linear.rb
index 54389e314d325b..19daac030ac125 100644
--- a/app/models/namespaces/traversal/linear.rb
+++ b/app/models/namespaces/traversal/linear.rb
@@ -233,7 +233,7 @@ def set_traversal_ids
         return if is_a?(Namespaces::ProjectNamespace)
 
         self.transient_traversal_ids = if parent_id
-                                         # remove safe navigation and `.to_a` after https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174995
+                                         # remove safe navigation and `.to_a` with https://gitlab.com/gitlab-org/gitlab/-/issues/508611
                                          parent&.traversal_ids.to_a + [id]
                                        else
                                          [id]
diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb
index 3f03c75ec437d8..dc6f49eb000953 100644
--- a/app/services/groups/destroy_service.rb
+++ b/app/services/groups/destroy_service.rb
@@ -118,7 +118,7 @@ def publish_event
       event = Groups::GroupDeletedEvent.new(
         data: {
           group_id: group.id,
-          root_namespace_id: group.root_ancestor&.id.to_i # remove safe navigation and `.to_i` after https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174995
+          root_namespace_id: group.root_ancestor&.id.to_i # remove safe navigation and `.to_i` with https://gitlab.com/gitlab-org/gitlab/-/issues/508611
         }
       )
 
diff --git a/ee/app/models/concerns/elastic/maintain_elasticsearch_on_group_update.rb b/ee/app/models/concerns/elastic/maintain_elasticsearch_on_group_update.rb
index bf16b5be45f487..18015f0ccb006a 100644
--- a/ee/app/models/concerns/elastic/maintain_elasticsearch_on_group_update.rb
+++ b/ee/app/models/concerns/elastic/maintain_elasticsearch_on_group_update.rb
@@ -25,7 +25,8 @@ def maintain_group_associations_on_update
 
     def maintain_group_associations_on_destroy
       delete_group_wiki_in_elastic if use_elasticsearch?
-      delete_group_associations if root_ancestor # remove if condition after https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174995
+      # remove if condition with https://gitlab.com/gitlab-org/gitlab/-/issues/508611
+      delete_group_associations if root_ancestor
     end
 
     def sync_group_wiki_in_elastic
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index 32b57b3396a44a..da2bc6783b5732 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -1155,7 +1155,7 @@ def execute_subgroup_hooks(event)
         # When this subgroup is removed, there is no point in this subgroup's webhook itself being notified
         # that `self` was removed. Rather, we should only care about notifying its ancestors
         # and hence we need to trigger the hooks starting only from its `parent` group.
-        parent&.execute_hooks(data, :subgroup_hooks) # remove safe navigation after https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174995
+        parent&.execute_hooks(data, :subgroup_hooks) # remove safe navigation with https://gitlab.com/gitlab-org/gitlab/-/issues/508611
       end
     end
 
diff --git a/ee/lib/ee/gitlab/background_migration/delete_orphaned_groups.rb b/ee/lib/ee/gitlab/background_migration/delete_orphaned_groups.rb
deleted file mode 100644
index 2e4ed8c05290c7..00000000000000
--- a/ee/lib/ee/gitlab/background_migration/delete_orphaned_groups.rb
+++ /dev/null
@@ -1,37 +0,0 @@
-# frozen_string_literal: true
-
-module EE
-  module Gitlab
-    module BackgroundMigration
-      module DeleteOrphanedGroups
-        extend ActiveSupport::Concern
-        extend ::Gitlab::Utils::Override
-
-        prepended do
-          operation_name :delete_orphaned_group_records
-          feature_category :groups_and_projects
-
-          scope_to ->(relation) { relation.where(type: 'Group').where.not(parent_id: nil) }
-        end
-
-        override :perform
-        def perform
-          each_sub_batch do |sub_batch|
-            sub_batch
-            .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id")
-            .where(parent: { id: nil })
-            .pluck(:id).each do |orphaned_group_id|
-              ::GroupDestroyWorker.new.perform(orphaned_group_id, admin_bot.id)
-            end
-          end
-        end
-
-        private
-
-        def admin_bot
-          @_admin_bot ||= ::Users::Internal.admin_bot
-        end
-      end
-    end
-  end
-end
diff --git a/ee/spec/lib/ee/gitlab/background_migration/delete_orphaned_groups_spec.rb b/ee/spec/lib/ee/gitlab/background_migration/delete_orphaned_groups_spec.rb
deleted file mode 100644
index e69de29bb2d1d6..00000000000000
diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb
index 0ffaa7a62b7b4c..6e82f575ab9550 100644
--- a/lib/gitlab/background_migration/delete_orphaned_groups.rb
+++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb
@@ -3,11 +3,27 @@
 module Gitlab
   module BackgroundMigration
     class DeleteOrphanedGroups < BatchedMigrationJob
+      operation_name :delete_orphaned_group_records
       feature_category :groups_and_projects
 
-      def perform; end
+      scope_to ->(relation) { relation.where(type: 'Group').where.not(parent_id: nil) }
+
+      def perform
+        each_sub_batch do |sub_batch|
+          sub_batch
+          .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id")
+          .where(parent: { id: nil })
+          .pluck(:id).each do |orphaned_group_id|
+            ::GroupDestroyWorker.new.perform(orphaned_group_id, admin_bot.id)
+          end
+        end
+      end
+
+      private
+
+      def admin_bot
+        @_admin_bot ||= Users::Internal.admin_bot
+      end
     end
   end
 end
-
-Gitlab::BackgroundMigration::DeleteOrphanedGroups.prepend_mod
diff --git a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb
new file mode 100644
index 00000000000000..717ebb1546a7b3
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb
@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::DeleteOrphanedGroups, feature_category: :groups_and_projects do
+  let(:namespaces) { table(:namespaces) }
+  let!(:parent) { namespaces.create!(name: 'Group', type: 'Group', path: 'space1') }
+  let!(:group) { namespaces.create!(name: 'GitLab', type: 'Group', path: 'group1') }
+  let!(:admin_bot) { ::Users::Internal.admin_bot }
+  let!(:orphaned_groups) do
+    (1..4).map do |i|
+      namespaces.create!(name: "Group #{i}", path: "orphaned_group_#{i}", type: 'Group', parent_id: parent.id)
+    end
+  end
+
+  subject(:background_migration) do
+    described_class.new(
+      start_id: namespaces.without(parent).minimum(:id),
+      end_id: namespaces.maximum(:id),
+      batch_table: :namespaces,
+      batch_column: :id,
+      sub_batch_size: 1,
+      pause_ms: 0,
+      connection: ApplicationRecord.connection
+    ).perform
+  end
+
+  describe '#perform' do
+    before do
+      # Remove constraint so we can create invalid records
+      ApplicationRecord.connection.execute("ALTER TABLE namespaces DROP CONSTRAINT fk_7f813d8c90;")
+      (1..4).map { |i| namespaces.create!(name: "Group #{i}", path: "group_#{i}", type: 'Group', parent_id: group.id) }
+    end
+
+    after do
+      # Re-create constraint after the test
+      ApplicationRecord.connection.execute(<<~SQL)
+        ALTER TABLE ONLY namespaces ADD CONSTRAINT fk_7f813d8c90
+        FOREIGN KEY (parent_id) REFERENCES namespaces(id) ON DELETE RESTRICT NOT VALID;
+      SQL
+    end
+
+    it 'enqueues ::GroupDestroyWorker for each group whose parent\'s do not exist and destroys them', :sidekiq_inline do
+      parent.destroy!
+
+      expect { background_migration }.to change { namespaces.count }.from(10).to(6)
+        .and change { namespaces.where(id: orphaned_groups.pluck(:id)).count }.from(4).to(0)
+    end
+  end
+end
-- 
GitLab


From 0aa740f7b51c4bc498e668d2cbcb377352606928 Mon Sep 17 00:00:00 2001
From: bmarjanovic <bmarjanovic@gitlab.com>
Date: Wed, 11 Dec 2024 10:46:55 +0100
Subject: [PATCH 4/4] Use async worker

---
 lib/gitlab/background_migration/delete_orphaned_groups.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb
index 6e82f575ab9550..428867ad400257 100644
--- a/lib/gitlab/background_migration/delete_orphaned_groups.rb
+++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb
@@ -14,7 +14,7 @@ def perform
           .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id")
           .where(parent: { id: nil })
           .pluck(:id).each do |orphaned_group_id|
-            ::GroupDestroyWorker.new.perform(orphaned_group_id, admin_bot.id)
+            ::GroupDestroyWorker.perform_async(orphaned_group_id, admin_bot.id)
           end
         end
       end
-- 
GitLab