Merge branch '34728-fix-application-setting-created-when-redis-down' into 'master'

Prevent bad data being added to application settings when Redis is unavailable

Closes #34728

See merge request !12750

app/models/application_setting.rb

@@ -184,6 +184,9 @@ class ApplicationSetting < ActiveRecord::Base
     Rails.cache.fetch(CACHE_KEY) do
       ApplicationSetting.last
     end
+  rescue
+    # Fall back to an uncached value if there are any problems (e.g. redis down)
+    ApplicationSetting.last
   end
 
   def self.expire

changelogs/unreleased/34728-fix-application-setting-created-when-redis-down.yml

+---
+title: Prevent bad data being added to application settings when Redis is unavailable
+merge_request: 12750
+author:

lib/gitlab/current_settings.rb

@@ -33,12 +33,7 @@ module Gitlab
     def uncached_application_settings
       return fake_application_settings unless connect_to_db?
 
-      # This loads from the database into the cache, so handle Redis errors
-      begin
-        db_settings = ::ApplicationSetting.current
-      rescue ::Redis::BaseError, ::Errno::ENOENT
-        # In case Redis isn't running or the Redis UNIX socket file is not available
-      end
+      db_settings = ::ApplicationSetting.current
 
       # If there are pending migrations, it's possible there are columns that
       # need to be added to the application settings. To prevent Rake tasks

spec/lib/gitlab/current_settings_spec.rb

@@ -27,10 +27,23 @@ describe Gitlab::CurrentSettings do
       end
 
       it 'falls back to DB if Redis fails' do
+        db_settings = ApplicationSetting.create!(ApplicationSetting.defaults)
+
         expect(ApplicationSetting).to receive(:cached).and_raise(::Redis::BaseError)
-        expect(ApplicationSetting).to receive(:last).and_call_original
+        expect(Rails.cache).to receive(:fetch).with(ApplicationSetting::CACHE_KEY).and_raise(Redis::BaseError)
 
-        expect(current_application_settings).to be_a(ApplicationSetting)
+        expect(current_application_settings).to eq(db_settings)
+      end
+
+      it 'creates default ApplicationSettings if none are present' do
+        expect(ApplicationSetting).to receive(:cached).and_raise(::Redis::BaseError)
+        expect(Rails.cache).to receive(:fetch).with(ApplicationSetting::CACHE_KEY).and_raise(Redis::BaseError)
+
+        settings = current_application_settings
+
+        expect(settings).to be_a(ApplicationSetting)
+        expect(settings).to be_persisted
+        expect(settings).to have_attributes(ApplicationSetting.defaults)
       end
 
       context 'with migrations pending' do

spec/models/application_setting_spec.rb

@@ -155,6 +155,18 @@ describe ApplicationSetting, models: true do
     end
   end
 
+  describe '.current' do
+    context 'redis unavailable' do
+      it 'returns an ApplicationSetting' do
+        allow(Rails.cache).to receive(:fetch).and_call_original
+        allow(ApplicationSetting).to receive(:last).and_return(:last)
+        expect(Rails.cache).to receive(:fetch).with(ApplicationSetting::CACHE_KEY).and_raise(ArgumentError)
+
+        expect(ApplicationSetting.current).to eq(:last)
+      end
+    end
+  end
+
   context 'restricted signup domains' do
     it 'sets single domain' do
       setting.domain_whitelist_raw = 'example.com'