FIX: Theme site settings not reloading across processes (#34242)

martin-brennan · yuriyaran · commit 427e8285489e · 2025-08-21T15:32:51.000+03:00
This commit fixes an issue in multi-process setups where the following would happen: 1. Admin changes a theme site setting. The new value is stored in memory in the process that made the change. For this example, changing `enable_welcome_banner` from `true` to `false` 2. Another user visits the site and hits another process, which has the old value of `enable_welcome_banner` still set to `true` in memory 3. The other user's value for `enable_welcome_banner` is cached in the cache for theme site settings, which is shared across all processes The issue is fixed by doing the same thing as site settings: when the new value is saved, we send a MessageBus message to all processes via `SiteSettingExtension` which calls `refresh!` and reloads the in-memory cache of theme site settings from the DB. c.f. https://meta.discourse.org/t/difficulty-understanding-the-enable-welcome-banner-setting/377321
diff --git a/app/models/remote_theme.rb b/app/models/remote_theme.rb
@@ -484,7 +484,7 @@ def create_theme_site_settings(theme, theme_site_settings)
           value: value,
         },
         guardian: Discourse.system_user.guardian,
-      ) { |result| }
+      )
     end
 
     SiteSetting.refresh!(refresh_site_settings: false, refresh_theme_site_settings: true)
diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb
@@ -177,7 +177,7 @@ def theme_site_settings_json(theme_id)
   end
 
   def setting_metadata_hash(setting)
-    setting_hash = {
+    {
       setting:,
       default: SiteSetting.defaults[setting],
       description: SiteSetting.description(setting),
@@ -433,6 +433,7 @@ def refresh!(refresh_site_settings: true, refresh_theme_site_settings: true)
 
         theme_site_setting_changes, theme_site_setting_deletions =
           diff_hash(new_theme_site_settings, theme_site_settings)
+
         theme_site_setting_changes.each do |theme_id, settings|
           theme_site_settings[theme_id] ||= {}
           theme_site_settings[theme_id].merge!(settings)
@@ -599,6 +600,7 @@ def change_themeable_site_setting(theme_id, name, val)
     theme_site_settings[theme_id][name] = val
 
     notify_clients!(name, theme_id: theme_id) if client_settings.include?(name)
+    notify_changed!
 
     clear_cache!(expire_theme_site_setting_cache: true)
 
diff --git a/lib/site_settings/local_process_provider.rb b/lib/site_settings/local_process_provider.rb
@@ -21,6 +21,8 @@ def initialize(name, data_type)
     end
   end
 
+  attr_accessor :model
+
   def settings
     @settings[current_site] ||= {}
   end
@@ -46,6 +48,11 @@ def save(name, value, data_type)
       settings[name] = setting
     end
     setting.value = value.to_s
+
+    # So we can simulate the MessageBus notifications
+    # done by the DbProvider
+    @model.notify_changed! if @model
+
     setting
   end
 
diff --git a/spec/lib/site_setting_extension_spec.rb b/spec/lib/site_setting_extension_spec.rb
@@ -41,7 +41,13 @@
     new_settings(provider_local)
   end
 
-  it "Does not leak state cause changes are not linked" do
+  after do
+    settings.provider.model = nil
+    settings.listen_for_changes = false
+    settings2.listen_for_changes = false
+  end
+
+  it "does not leak state cause changes are not linked" do
     t1 =
       Thread.new do
         5.times do
@@ -89,7 +95,7 @@
       expect(settings.hello).to eq(99)
     end
 
-    it "publishes changes cross sites" do
+    it "picks up changes from provider on refresh across processes" do
       settings.setting(:hello, 1)
       settings2.setting(:hello, 1)
 
@@ -104,6 +110,30 @@
       expect(settings2.hello).to eq(99)
     end
 
+    it "publishes changes across processes" do
+      MessageBus.on
+
+      # Only need to do this once, since both settings will use the same provider. Doing it twice leads to confusion with MessageBus
+      settings.provider.model = settings
+
+      settings.listen_for_changes = true
+      settings2.listen_for_changes = true
+
+      settings.refresh!
+      settings2.refresh!
+
+      settings.setting(:hello, 1)
+      settings2.setting(:hello, 1)
+
+      settings.hello = 100
+
+      expect(settings2.hello).to eq(100)
+
+      settings.hello = 99
+
+      expect(settings2.hello).to eq(99)
+    end
+
     it "does not override types in the type supervisor" do
       settings.setting(:foo, "bar")
       settings.provider.save(:foo, "bar", SiteSetting.types[:enum])
@@ -1048,6 +1078,38 @@ def self.translate_names?
       expect(SiteSetting.search_experience(theme_id: theme_2.id)).to eq("search_field")
     end
 
+    it "publishes changes across processes" do
+      MessageBus.on
+
+      settings_tss_instance_1 = new_settings(provider_local)
+      settings_tss_instance_2 = new_settings(provider_local)
+
+      settings_tss_instance_1.listen_for_changes = true
+      settings_tss_instance_2.listen_for_changes = true
+
+      settings_tss_instance_1.load_settings(File.join(Rails.root, "config", "site_settings.yml"))
+      settings_tss_instance_2.load_settings(File.join(Rails.root, "config", "site_settings.yml"))
+
+      settings_tss_instance_1.refresh!
+      settings_tss_instance_2.refresh!
+
+      expect(settings_tss_instance_1.enable_welcome_banner(theme_id: theme_1.id)).to eq(false)
+      expect(settings_tss_instance_2.enable_welcome_banner(theme_id: theme_1.id)).to eq(false)
+
+      tss_1.update!(value: true)
+      settings_tss_instance_1.change_themeable_site_setting(
+        theme_1.id,
+        :enable_welcome_banner,
+        true,
+      )
+
+      # Get through the MessageBus queue
+      try_until_success(frequency: 0.5) do
+        expect(settings_tss_instance_1.enable_welcome_banner(theme_id: theme_1.id)).to eq(true)
+        expect(settings_tss_instance_2.enable_welcome_banner(theme_id: theme_1.id)).to eq(true)
+      end
+    end
+
     describe ".theme_site_settings_json_uncached" do
       it "returns the correct JSON" do
         SiteSetting.refresh!
diff --git a/spec/support/helpers.rb b/spec/support/helpers.rb
@@ -318,6 +318,17 @@ def enable_current_plugin
     SiteSetting.public_send("#{plugin.enabled_site_setting}=", true)
   end
 
+  def try_until_success(timeout: 3, frequency: 0.01)
+    start ||= Time.zone.now
+    backoff ||= frequency
+    yield
+  rescue RSpec::Expectations::ExpectationNotMetError
+    raise if Time.zone.now >= start + timeout.seconds
+    sleep backoff
+    backoff += frequency
+    retry
+  end
+
   private
 
   def directory_from_caller
