Fixes #5134 - Settings cache is applied for 24 hours after the last change only

app/models/setting.rb

@@ -1,5 +1,8 @@
 # Copyright (C) 2012-2024 Zammad Foundation, https://zammad-foundation.org/
 
+# Class variables are used here as performance optimization.
+# Technically it is not thread-safe, but it never caused issues.
+# rubocop:disable Style/ClassVars
 class Setting < ApplicationModel
   store         :options
   store         :state_current
@@ -7,20 +10,19 @@ class Setting < ApplicationModel
   store         :preferences
   before_validation :state_check
   before_create :set_initial
-  after_create  :reset_change_id
-  after_update  :reset_change_id
-  after_commit  :reset_cache, :broadcast_frontend, :check_refresh
+  after_save    :reset_class_cache_key
+  after_commit  :reset_other_caches, :broadcast_frontend, :check_refresh
 
   validates_with Setting::Validator
 
   attr_accessor :state
 
-  @@current         = {} # rubocop:disable Style/ClassVars
-  @@raw             = {} # rubocop:disable Style/ClassVars
-  @@change_id       = nil # rubocop:disable Style/ClassVars
-  @@last_changed_at = nil # rubocop:disable Style/ClassVars
-  @@lookup_at       = nil # rubocop:disable Style/ClassVars
-  @@lookup_timeout  = if ENV['ZAMMAD_SETTING_TTL'] # rubocop:disable Style/ClassVars
+  @@current         = {}
+  @@raw             = {}
+  @@query_cache_key = nil
+  @@last_changed_at = nil
+  @@lookup_at       = nil
+  @@lookup_timeout  = if ENV['ZAMMAD_SETTING_TTL']
                         ENV['ZAMMAD_SETTING_TTL'].to_i.seconds
                       else
                         15.seconds
@@ -91,10 +93,27 @@ reload config settings
 =end
 
   def self.reload
-    @@last_changed_at = nil # rubocop:disable Style/ClassVars
+    @@last_changed_at = nil
     load(true)
   end
 
+  # check if cache is still valid
+  def self.cache_valid?
+    # Check if last last lookup was recent enough
+    if @@lookup_at && @@lookup_at > @@lookup_timeout.ago
+      # logger.debug "Setting.cache_valid?: cache_id has been set within last #{@@lookup_timeout} seconds"
+      return true
+    end
+
+    if @@query_cache_key && Setting.reorder(:id).cache_key_with_version == @@query_cache_key
+      @@lookup_at = Time.current
+
+      return true
+    end
+
+    false
+  end
+
   private
 
   # load values and cache them
@@ -104,52 +123,58 @@ reload config settings
     return false if !force && @@current.present? && cache_valid?
 
     # read all or only changed since last read
-    latest = Setting.reorder(updated_at: :desc).limit(1).pluck(:updated_at)
-    settings = if @@last_changed_at && @@current.present?
-                 Setting.where('updated_at >= ?', @@last_changed_at).reorder(:id).pluck(:name, :state_current)
-               else
-                 Setting.reorder(:id).pluck(:name, :state_current)
-               end
-
-    if latest && latest[0]
-      @@last_changed_at = [Time.current, latest[0]].min # rubocop:disable Style/ClassVars
-    end
+    latest = Setting.maximum(:updated_at)
+
+    base_query = Setting.reorder(:id)
+
+    settings_query = if @@last_changed_at && @@current.present?
+                       base_query.where('updated_at >= ?', @@last_changed_at)
+                     else
+                       base_query
+                     end
+
+    settings = settings_query.pluck(:name, :state_current)
+
+    @@last_changed_at = [Time.current, latest].min if latest
 
     if settings.present?
       settings.each do |setting|
         @@raw[setting[0]] = setting[1]['value']
       end
+
       @@raw.each do |key, value|
-        if value.class != String
-          @@current[key] = value
-          next
-        end
-        @@current[key] = value.gsub(%r{\#\{config\.(.+?)\}}) do
-          @@raw[$1].to_s
-        end
+        @@current[key] = interpolate_value value
       end
     end
 
-    @@change_id = Rails.cache.read('Setting::ChangeId') # rubocop:disable Style/ClassVars
-    @@lookup_at = Time.now.to_i # rubocop:disable Style/ClassVars
+    @@query_cache_key = base_query.cache_key_with_version
+    @@lookup_at = Time.current
+
     true
   end
   private_class_method :load
 
+  def self.interpolate_value(input)
+    return input if !input.is_a? String
+
+    input.gsub(%r{\#\{config\.(.+?)\}}) do
+      @@raw[$1].to_s
+    end
+  end
+  private_class_method :interpolate_value
+
   # set initial value in state_initial
   def set_initial
     self.state_initial = state_current
   end
 
-  def reset_change_id
-    change_id = SecureRandom.uuid
-    logger.debug { "Setting.reset_change_id: set new cache, #{change_id}" }
-    Rails.cache.write('Setting::ChangeId', change_id, { expires_in: 24.hours })
-    @@lookup_at = nil # rubocop:disable Style/ClassVars
-    true
+  def reset_class_cache_key
+    @@lookup_at = nil
+    @@query_cache_key = nil
   end
 
-  def reset_cache
+  # Resets caches related to the setting in question.
+  def reset_other_caches
     return if preferences[:cache].blank?
 
     Array(preferences[:cache]).each do |key|
@@ -157,25 +182,7 @@ reload config settings
     end
   end
 
-  # check if cache is still valid
-  def self.cache_valid?
-    if @@lookup_at && @@lookup_at > Time.now.to_i - @@lookup_timeout
-      # logger.debug "Setting.cache_valid?: cache_id has been set within last #{@@lookup_timeout} seconds"
-      return true
-    end
-
-    change_id = Rails.cache.read('Setting::ChangeId')
-    if @@change_id && change_id == @@change_id
-      @@lookup_at = Time.now.to_i # rubocop:disable Style/ClassVars
-      # logger.debug "Setting.cache_valid?: cache still valid, #{@@change_id}/#{change_id}"
-      return true
-    end
-    # logger.debug "Setting.cache_valid?: cache has changed, #{@@change_id}/#{change_id}"
-    false
-  end
-  private_class_method :cache_valid?
-
-  # convert state into hash to be able to store it as store
+  # Convert state into hash to be able to store it as store.
   def state_check
     return if state.nil? # allow false value
     return if state.try(:key?, :value)
@@ -212,3 +219,4 @@ reload config settings
     AppVersion.set(true, AppVersion::MSG_CONFIG_CHANGED)
   end
 end
+# rubocop:enable Style/ClassVars

spec/models/setting_spec.rb

@@ -58,6 +58,60 @@ RSpec.describe Setting, type: :model do
     end
   end
 
+  describe '.cache_valid?' do
+    context 'when loading first time' do
+      before do
+        # ensure no cache checks are set
+        described_class.class_variable_set(:@@lookup_at, nil) # rubocop:disable Style/ClassVars
+        described_class.class_variable_set(:@@query_cache_key, nil) # rubocop:disable Style/ClassVars
+      end
+
+      it 'cache is not valid' do
+        expect(described_class).not_to be_cache_valid
+      end
+    end
+
+    context 'when cache is valid' do
+      before do
+        # ensure cache is warm
+        described_class.send(:load)
+
+        # ensure cache is not touched by broadcasting the new value
+        allow_any_instance_of(described_class).to receive(:broadcast_frontend)
+      end
+
+      it 'cache is valid' do
+        expect(described_class).to be_cache_valid
+      end
+
+      it 'cache is still valid after some time' do
+        travel 1.minute
+        expect(described_class).to be_cache_valid
+      end
+
+      context 'when Setting is updated in the same process' do
+        before { described_class.set('maintenance_login', 'sample message') }
+
+        it 'cache is not valid' do
+          expect(described_class).not_to be_cache_valid
+        end
+      end
+
+      context 'when Setting updated outside of the process and class variables were not touched' do
+        before { described_class.all.sample.touch }
+
+        it 'cache is seen as valid' do
+          expect(described_class).to be_cache_valid
+        end
+
+        it 'cache is seen as invalid after some time' do
+          travel 1.minute
+          expect(described_class).not_to be_cache_valid
+        end
+      end
+    end
+  end
+
   describe 'attributes' do
     describe '#state_initial' do
       subject(:setting) { build(:setting, state: 'foo') }