Merge branch 'move-traversal-id-update-to-before-commit' into 'master'

Move traversal_ids update to the end of transaction

See merge request gitlab-org/gitlab!79964

app/models/namespaces/traversal/linear.rb

@@ -43,14 +43,23 @@ module Namespaces
 
       included do
         before_update :lock_both_roots, if: -> { sync_traversal_ids? && parent_id_changed? }
-        after_create :sync_traversal_ids, if: -> { sync_traversal_ids? }
         after_update :sync_traversal_ids, if: -> { sync_traversal_ids? && saved_change_to_parent_id? }
+        # sync traversal_ids on namespace create, which can happen quite early within a transaction, thus keeping the lock on root namespace record
+        # for a relatively long time, e.g. creating the project namespace when a project is being created.
+        after_create :sync_traversal_ids, if: -> { sync_traversal_ids? && !sync_traversal_ids_before_commit? }
+        # This uses rails internal before_commit API to sync traversal_ids on namespace create, right before transaction is committed.
+        # This helps reduce the time during which the root namespace record is locked to ensure updated traversal_ids are valid
+        before_commit :sync_traversal_ids, on: [:create], if: -> { sync_traversal_ids? && sync_traversal_ids_before_commit? }
       end
 
       def sync_traversal_ids?
         Feature.enabled?(:sync_traversal_ids, root_ancestor, default_enabled: :yaml)
       end
 
+      def sync_traversal_ids_before_commit?
+        Feature.enabled?(:sync_traversal_ids_before_commit, root_ancestor, default_enabled: :yaml)
+      end
+
       def use_traversal_ids?
         return false unless Feature.enabled?(:use_traversal_ids, default_enabled: :yaml)
 

config/feature_flags/development/sync_traversal_ids_before_commit.yml

+---
+name: sync_traversal_ids_before_commit
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/79964
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/352499
+group: group::workspace
+type: development
+default_enabled: false
+milestone: '14.8'

spec/models/namespace_spec.rb

@@ -425,7 +425,7 @@ RSpec.describe Namespace do
   end
 
   context 'traversal_ids on create' do
-    context 'default traversal_ids' do
+    shared_examples 'default traversal_ids' do
       let(:namespace) { build(:namespace) }
 
       before do
@@ -435,6 +435,18 @@ RSpec.describe Namespace do
 
       it { expect(namespace.traversal_ids).to eq [namespace.id] }
     end
+
+    context 'with before_commit callback' do
+      it_behaves_like 'default traversal_ids'
+    end
+
+    context 'with after_create callback' do
+      before do
+        stub_feature_flags(sync_traversal_ids_before_commit: false)
+      end
+
+      it_behaves_like 'default traversal_ids'
+    end
   end
 
   describe "after_commit :expire_child_caches" do

spec/models/project_spec.rb

@@ -236,27 +236,42 @@ RSpec.describe Project, factory_default: :keep do
       end
 
       context 'with project namespaces' do
-        it 'automatically creates a project namespace' do
-          project = build(:project, path: 'hopefully-valid-path1')
-          project.save!
+        shared_examples 'creates project namespace' do
+          it 'automatically creates a project namespace' do
+            project = build(:project, path: 'hopefully-valid-path1')
+            project.save!
 
-          expect(project).to be_persisted
-          expect(project.project_namespace).to be_persisted
-          expect(project.project_namespace).to be_in_sync_with_project(project)
-        end
+            expect(project).to be_persisted
+            expect(project.project_namespace).to be_persisted
+            expect(project.project_namespace).to be_in_sync_with_project(project)
+            expect(project.reload.project_namespace.traversal_ids).to eq([project.namespace.traversal_ids, project.project_namespace.id].flatten.compact)
+          end
 
-        context 'with FF disabled' do
-          before do
-            stub_feature_flags(create_project_namespace_on_project_create: false)
+          context 'with FF disabled' do
+            before do
+              stub_feature_flags(create_project_namespace_on_project_create: false)
+            end
+
+            it 'does not create a project namespace' do
+              project = build(:project, path: 'hopefully-valid-path2')
+              project.save!
+
+              expect(project).to be_persisted
+              expect(project.project_namespace).to be_nil
+            end
           end
+        end
 
-          it 'does not create a project namespace' do
-            project = build(:project, path: 'hopefully-valid-path2')
-            project.save!
+        context 'sync-ing traversal_ids in before_commit callback' do
+          it_behaves_like 'creates project namespace'
+        end
 
-            expect(project).to be_persisted
-            expect(project.project_namespace).to be_nil
+        context 'sync-ing traversal_ids in after_create callback' do
+          before do
+            stub_feature_flags(sync_traversal_ids_before_commit: false)
           end
+
+          it_behaves_like 'creates project namespace'
         end
       end
     end

spec/services/groups/create_service_spec.rb

@@ -8,6 +8,10 @@ RSpec.describe Groups::CreateService, '#execute' do
 
   subject { service.execute }
 
+  shared_examples 'has sync-ed traversal_ids' do
+    specify { expect(subject.reload.traversal_ids).to eq([subject.parent&.traversal_ids, subject.id].flatten.compact) }
+  end
+
   describe 'visibility level restrictions' do
     let!(:service) { described_class.new(user, group_params) }
 
@@ -77,6 +81,18 @@ RSpec.describe Groups::CreateService, '#execute' do
       it 'adds an onboarding progress record' do
         expect { subject }.to change(OnboardingProgress, :count).from(0).to(1)
       end
+
+      context 'with before_commit callback' do
+        it_behaves_like 'has sync-ed traversal_ids'
+      end
+
+      context 'with after_create callback' do
+        before do
+          stub_feature_flags(sync_traversal_ids_before_commit: false)
+        end
+
+        it_behaves_like 'has sync-ed traversal_ids'
+      end
     end
 
     context 'when user can not create a group' do
@@ -102,6 +118,18 @@ RSpec.describe Groups::CreateService, '#execute' do
       it 'does not add an onboarding progress record' do
         expect { subject }.not_to change(OnboardingProgress, :count).from(0)
       end
+
+      context 'with before_commit callback' do
+        it_behaves_like 'has sync-ed traversal_ids'
+      end
+
+      context 'with after_create callback' do
+        before do
+          stub_feature_flags(sync_traversal_ids_before_commit: false)
+        end
+
+        it_behaves_like 'has sync-ed traversal_ids'
+      end
     end
 
     context 'as guest' do

spec/services/projects/create_service_spec.rb

@@ -190,9 +190,13 @@ RSpec.describe Projects::CreateService, '#execute' do
       user.refresh_authorized_projects # Ensure cache is warm
     end
 
-    it do
-      project = create_project(user, opts.merge!(namespace_id: group.id))
+    subject(:project) { create_project(user, opts.merge!(namespace_id: group.id)) }
 
+    shared_examples 'has sync-ed traversal_ids' do
+      specify { expect(project.reload.project_namespace.traversal_ids).to eq([project.namespace.traversal_ids, project.project_namespace.id].flatten.compact) }
+    end
+
+    it 'creates the project' do
       expect(project).to be_valid
       expect(project.owner).to eq(group)
       expect(project.namespace).to eq(group)
@@ -200,6 +204,18 @@ RSpec.describe Projects::CreateService, '#execute' do
       expect(user.authorized_projects).to include(project)
       expect(project.project_namespace).to be_in_sync_with_project(project)
     end
+
+    context 'with before_commit callback' do
+      it_behaves_like 'has sync-ed traversal_ids'
+    end
+
+    context 'with after_create callback' do
+      before do
+        stub_feature_flags(sync_traversal_ids_before_commit: false)
+      end
+
+      it_behaves_like 'has sync-ed traversal_ids'
+    end
   end
 
   context 'group sharing', :sidekiq_inline do