Merge branch '355015-forbid-application-settings-recursion' into 'master'

Handle recursion when creating ApplicationSettings

See merge request gitlab-org/gitlab!82930

app/models/application_setting.rb

@@ -657,7 +657,17 @@ class ApplicationSetting < ApplicationRecord
     users_count >= INSTANCE_REVIEW_MIN_USERS
   end
 
+  Recursion = Class.new(RuntimeError)
+
   def self.create_from_defaults
+    # this is posssible if calls to create the record depend on application
+    # settings themselves. This was seen in the case of a feature flag called by
+    # `transaction` that ended up requiring application settings to determine metrics behavior.
+    # If something like that happens, we break the loop here, and let the caller decide how to manage it.
+    raise Recursion if Thread.current[:application_setting_create_from_defaults]
+
+    Thread.current[:application_setting_create_from_defaults] = true
+
     check_schema!
 
     transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
@@ -666,6 +676,8 @@ class ApplicationSetting < ApplicationRecord
   rescue ActiveRecord::RecordNotUnique
     # We already have an ApplicationSetting record, so just return it.
     current_without_cache
+  ensure
+    Thread.current[:application_setting_create_from_defaults] = nil
   end
 
   def self.find_or_create_without_cache

lib/gitlab/current_settings.rb

@@ -70,6 +70,8 @@ module Gitlab
         else
           ::ApplicationSetting.create_from_defaults
         end
+      rescue ::ApplicationSetting::Recursion
+        in_memory_application_settings
       end
 
       def fake_application_settings(attributes = {})

spec/lib/gitlab/current_settings_spec.rb

@@ -179,6 +179,21 @@ RSpec.describe Gitlab::CurrentSettings do
           expect(settings).to have_attributes(settings_from_defaults)
         end
 
+        context 'when we hit a recursive loop' do
+          before do
+            expect(ApplicationSetting).to receive(:create_from_defaults) do
+              raise ApplicationSetting::Recursion
+            end
+          end
+
+          it 'recovers and returns in-memory settings' do
+            settings = described_class.current_application_settings
+
+            expect(settings).to be_a(ApplicationSetting)
+            expect(settings).not_to be_persisted
+          end
+        end
+
         context 'when ApplicationSettings does not have a primary key' do
           before do
             allow(ApplicationSetting.connection).to receive(:primary_key).with('application_settings').and_return(nil)