Merge branch 'cleanup_locking_fixes_feature_flags' into 'master'

Cleanup development feature flags for locking namespaces fix

See merge request gitlab-org/gitlab!81805

app/models/namespace/traversal_hierarchy.rb

@@ -40,12 +40,7 @@ class Namespace
       SQL
 
       Namespace.transaction do
-        if Feature.enabled?(:for_no_key_update_lock, default_enabled: :yaml)
-          @root.lock!("FOR NO KEY UPDATE")
-        else
-          @root.lock!
-        end
-
+        @root.lock!("FOR NO KEY UPDATE")
         Namespace.connection.exec_query(sql)
       end
     rescue ActiveRecord::Deadlocked

app/models/namespaces/traversal/linear.rb

@@ -44,22 +44,15 @@ module Namespaces
       included do
         before_update :lock_both_roots, if: -> { sync_traversal_ids? && parent_id_changed? }
         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? }
+        before_commit :sync_traversal_ids, on: [:create], if: -> { sync_traversal_ids? }
       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/for_no_key_update_lock.yml

----
-name: for_no_key_update_lock
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/81239
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/353619
-milestone: '14.9'
-type: development
-group: group::workspaces
-default_enabled: false

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/group_spec.rb

@@ -390,26 +390,8 @@ RSpec.describe Group do
         let!(:old_parent) { create(:group, parent: root) }
         let!(:new_parent) { create(:group, parent: root) }
 
-        context 'with FOR UPDATE lock' do
-          before do
-            stub_feature_flags(for_no_key_update_lock: false)
-            subject
-            reload_models(old_parent, new_parent, group)
-          end
-
-          it 'updates traversal_ids' do
-            expect(group.traversal_ids).to eq [root.id, new_parent.id, group.id]
-          end
-
-          it_behaves_like 'hierarchy with traversal_ids'
-          it_behaves_like 'locked row', 'FOR UPDATE' do
-            let(:row) { root }
-          end
-        end
-
         context 'with FOR NO KEY UPDATE lock' do
           before do
-            stub_feature_flags(for_no_key_update_lock: true)
             subject
             reload_models(old_parent, new_parent, group)
           end
@@ -419,7 +401,7 @@ RSpec.describe Group do
           end
 
           it_behaves_like 'hierarchy with traversal_ids'
-          it_behaves_like 'locked row', 'FOR NO KEY UPDATE' do
+          it_behaves_like 'locked row' do
             let(:row) { root }
           end
         end

spec/models/namespace/traversal_hierarchy_spec.rb

@@ -68,24 +68,11 @@ RSpec.describe Namespace::TraversalHierarchy, type: :model do
       end
     end
 
-    it_behaves_like 'locked row', 'FOR UPDATE' do
+    it_behaves_like 'locked row' do
       let(:recorded_queries) { ActiveRecord::QueryRecorder.new }
       let(:row) { root }
 
       before do
-        stub_feature_flags(for_no_key_update_lock: false)
-
-        recorded_queries.record { subject }
-      end
-    end
-
-    it_behaves_like 'locked row', 'FOR NO KEY UPDATE' do
-      let(:recorded_queries) { ActiveRecord::QueryRecorder.new }
-      let(:row) { root }
-
-      before do
-        stub_feature_flags(for_no_key_update_lock: true)
-
         recorded_queries.record { subject }
       end
     end

spec/models/namespace_spec.rb

@@ -436,17 +436,7 @@ 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
+    it_behaves_like 'default traversal_ids'
   end
 
   describe "after_commit :expire_child_caches" do

spec/models/project_spec.rb

@@ -262,17 +262,7 @@ RSpec.describe Project, factory_default: :keep do
           end
         end
 
-        context 'sync-ing traversal_ids in before_commit callback' do
-          it_behaves_like 'creates project namespace'
-        end
-
-        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
+        it_behaves_like 'creates project namespace'
       end
     end
 

spec/services/groups/create_service_spec.rb

@@ -85,14 +85,6 @@ RSpec.describe Groups::CreateService, '#execute' do
       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
@@ -119,17 +111,7 @@ RSpec.describe Groups::CreateService, '#execute' 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
+      it_behaves_like 'has sync-ed traversal_ids'
     end
 
     context 'as guest' do

spec/services/projects/create_service_spec.rb

@@ -225,17 +225,7 @@ RSpec.describe Projects::CreateService, '#execute' do
       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
+    it_behaves_like 'has sync-ed traversal_ids'
   end
 
   context 'group sharing', :sidekiq_inline do

spec/support/shared_examples/row_lock_shared_examples.rb

@@ -4,10 +4,10 @@
 # Ensure a transaction also occurred.
 # Be careful! This form of spec is not foolproof, but better than nothing.
 
-RSpec.shared_examples 'locked row' do |lock_type|
+RSpec.shared_examples 'locked row' do
   it "has locked row" do
     table_name = row.class.table_name
-    ids_regex = /SELECT.*FROM.*#{table_name}.*"#{table_name}"."id" = #{row.id}.+#{lock_type}/m
+    ids_regex = /SELECT.*FROM.*#{table_name}.*"#{table_name}"."id" = #{row.id}.+FOR NO KEY UPDATE/m
 
     expect(recorded_queries.log).to include a_string_matching 'SAVEPOINT'
     expect(recorded_queries.log).to include a_string_matching ids_regex