From 7a52ba06176fe6d779cb0e5b7f1efe379a806c94 Mon Sep 17 00:00:00 2001 From: Chen Zhang Date: Thu, 16 Oct 2025 16:43:25 -0400 Subject: [PATCH 1/4] Add Sidekiq worker for group transfer to organization Creates a new Sidekiq worker to handle asynchronous group transfers to organizations. The worker calls Organizations::Groups::TransferService to perform the actual transfer operation. Key features: - Idempotent worker that can safely retry on failure - Low urgency with database health signal deferral - Sticky data consistency for efficient database load balancing - Gracefully handles missing records (group, organization, or user) The worker is located in CE (app/workers) as Organizations is a Community Edition feature. https://gitlab.com/gitlab-org/gitlab/-/issues/431557 Changelog: added --- .../organizations/groups/transfer_worker.rb | 34 +++++++ .../groups/transfer_worker_spec.rb | 90 +++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 app/workers/organizations/groups/transfer_worker.rb create mode 100644 spec/workers/organizations/groups/transfer_worker_spec.rb diff --git a/app/workers/organizations/groups/transfer_worker.rb b/app/workers/organizations/groups/transfer_worker.rb new file mode 100644 index 00000000000000..30395494ca6c3a --- /dev/null +++ b/app/workers/organizations/groups/transfer_worker.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module Organizations + module Groups + class TransferWorker + include ApplicationWorker + + data_consistency :sticky + idempotent! + + feature_category :organization + urgency :low + + defer_on_database_health_signal :gitlab_main, [:groups], 1.minute + + def perform(group_id, organization_id, current_user_id) + group = Group.find_by_id(group_id) + return unless group + + organization = Organization.find_by_id(organization_id) + return unless organization + + current_user = User.find_by_id(current_user_id) + return unless current_user + + Organizations::Groups::TransferService.new( + group: group, + new_organization: organization, + current_user: current_user + ).execute + end + end + end +end diff --git a/spec/workers/organizations/groups/transfer_worker_spec.rb b/spec/workers/organizations/groups/transfer_worker_spec.rb new file mode 100644 index 00000000000000..79b64d996f4c59 --- /dev/null +++ b/spec/workers/organizations/groups/transfer_worker_spec.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Organizations::Groups::TransferWorker, feature_category: :organization do + let_it_be(:group) { create(:group) } + let_it_be(:organization) { create(:organization) } + let_it_be(:user) { create(:user) } + + let(:worker) { described_class.new } + + describe '#perform' do + subject(:perform) { worker.perform(group.id, organization.id, user.id) } + + context 'when all records exist' do + it 'calls the transfer service' do + expect_next_instance_of( + Organizations::Groups::TransferService, + group: group, + new_organization: organization, + current_user: user + ) do |service| + expect(service).to receive(:execute) + end + + perform + end + end + + context 'when group does not exist' do + subject(:perform) { worker.perform(non_existing_record_id, organization.id, user.id) } + + it 'does not call the transfer service' do + expect(Organizations::Groups::TransferService).not_to receive(:new) + + perform + end + + it 'does not raise an error' do + expect { perform }.not_to raise_error + end + end + + context 'when organization does not exist' do + subject(:perform) { worker.perform(group.id, non_existing_record_id, user.id) } + + it 'does not call the transfer service' do + expect(Organizations::Groups::TransferService).not_to receive(:new) + + perform + end + + it 'does not raise an error' do + expect { perform }.not_to raise_error + end + end + + context 'when user does not exist' do + subject(:perform) { worker.perform(group.id, organization.id, non_existing_record_id) } + + it 'does not call the transfer service' do + expect(Organizations::Groups::TransferService).not_to receive(:new) + + perform + end + + it 'does not raise an error' do + expect { perform }.not_to raise_error + end + end + end + + describe 'worker attributes' do + it 'is idempotent' do + expect(described_class).to be_idempotent + end + + it 'has the correct feature category' do + expect(described_class.get_feature_category).to eq(:organization) + end + + it 'has low urgency' do + expect(described_class.get_urgency).to eq(:low) + end + end + + it_behaves_like 'an idempotent worker' do + let(:job_args) { [group.id, organization.id, user.id] } + end +end -- GitLab From a341c8dff296e8cb4f341cc50dac2d58071c02d4 Mon Sep 17 00:00:00 2001 From: Chen Zhang Date: Sun, 19 Oct 2025 23:08:04 -0400 Subject: [PATCH 2/4] Update worker queues --- app/workers/all_queues.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 88cb09d9fa6c3c..27dda6f9b9e189 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -4564,6 +4564,16 @@ :idempotent: true :tags: [] :queue_namespace: +- :name: organizations_groups_transfer + :worker_name: Organizations::Groups::TransferWorker + :feature_category: :organization + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] + :queue_namespace: - :name: pages :worker_name: PagesWorker :feature_category: :pages -- GitLab From 49d5e0c9285a1226627640cab48a1e4a7b25b87b Mon Sep 17 00:00:00 2001 From: Chen Zhang Date: Sun, 19 Oct 2025 23:10:49 -0400 Subject: [PATCH 3/4] Update sidekiq queues --- config/sidekiq_queues.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 3e764c38432da9..c578c061a1e364 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -731,6 +731,8 @@ - 1 - - onboarding_progress_tracking - 1 +- - organizations_groups_transfer + - 1 - - package_cleanup - 1 - - package_metadata_global_advisory_scan -- GitLab From 646518b6cdf98dd16ff4f72e215565f9bb217e78 Mon Sep 17 00:00:00 2001 From: Chen Zhang Date: Mon, 20 Oct 2025 21:19:11 -0400 Subject: [PATCH 4/4] Refactor TransferWorker to use hash-based arguments Changes the worker signature from positional arguments to a hash-based approach for better forward compatibility and flexibility. Using a hash allows us to add optional parameters in the future without requiring a multi-release backward compatibility process. This follows GitLab's recommendation for new workers as documented in the Sidekiq compatibility guidelines. Benefits: - Future parameter additions are backward compatible - More self-documenting code with named keys - No need for default values or version checks when extending - Easier to maintain as requirements evolve https://gitlab.com/gitlab-org/gitlab/-/issues/431557 --- .../organizations/groups/transfer_worker.rb | 6 +++- .../groups/transfer_worker_spec.rb | 35 ++++++++++++++++--- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/app/workers/organizations/groups/transfer_worker.rb b/app/workers/organizations/groups/transfer_worker.rb index 30395494ca6c3a..ed3694a771b9ef 100644 --- a/app/workers/organizations/groups/transfer_worker.rb +++ b/app/workers/organizations/groups/transfer_worker.rb @@ -13,7 +13,11 @@ class TransferWorker defer_on_database_health_signal :gitlab_main, [:groups], 1.minute - def perform(group_id, organization_id, current_user_id) + def perform(args) + group_id = args['group_id'] + organization_id = args['organization_id'] + current_user_id = args['current_user_id'] + group = Group.find_by_id(group_id) return unless group diff --git a/spec/workers/organizations/groups/transfer_worker_spec.rb b/spec/workers/organizations/groups/transfer_worker_spec.rb index 79b64d996f4c59..3cf91065d241ea 100644 --- a/spec/workers/organizations/groups/transfer_worker_spec.rb +++ b/spec/workers/organizations/groups/transfer_worker_spec.rb @@ -8,9 +8,16 @@ let_it_be(:user) { create(:user) } let(:worker) { described_class.new } + let(:args) do + { + 'group_id' => group.id, + 'organization_id' => organization.id, + 'current_user_id' => user.id + } + end describe '#perform' do - subject(:perform) { worker.perform(group.id, organization.id, user.id) } + subject(:perform) { worker.perform(args) } context 'when all records exist' do it 'calls the transfer service' do @@ -28,7 +35,13 @@ end context 'when group does not exist' do - subject(:perform) { worker.perform(non_existing_record_id, organization.id, user.id) } + let(:args) do + { + 'group_id' => non_existing_record_id, + 'organization_id' => organization.id, + 'current_user_id' => user.id + } + end it 'does not call the transfer service' do expect(Organizations::Groups::TransferService).not_to receive(:new) @@ -42,7 +55,13 @@ end context 'when organization does not exist' do - subject(:perform) { worker.perform(group.id, non_existing_record_id, user.id) } + let(:args) do + { + 'group_id' => group.id, + 'organization_id' => non_existing_record_id, + 'current_user_id' => user.id + } + end it 'does not call the transfer service' do expect(Organizations::Groups::TransferService).not_to receive(:new) @@ -56,7 +75,13 @@ end context 'when user does not exist' do - subject(:perform) { worker.perform(group.id, organization.id, non_existing_record_id) } + let(:args) do + { + 'group_id' => group.id, + 'organization_id' => organization.id, + 'current_user_id' => non_existing_record_id + } + end it 'does not call the transfer service' do expect(Organizations::Groups::TransferService).not_to receive(:new) @@ -85,6 +110,6 @@ end it_behaves_like 'an idempotent worker' do - let(:job_args) { [group.id, organization.id, user.id] } + let(:job_args) { [args] } end end -- GitLab