PERF: Make stylesheet hashes consistent between deploys (#18909)

Previously the stylesheet cachebusting hash was based on the maximum mtime of files. This works well in development and during in-container updates (e.g. via docker_manager). However, when a fresh docker image is created for each deploy, the file mtimes will change even if the contents has not.

This commit changes the production logic to calculate the cachebuster from the filenames and contents of the relevant assets. This should be consistent across deploys, thereby improving cache hits and improving page load times.

lib/stylesheet/manager.rb

@@ -6,10 +6,10 @@ require 'stylesheet/compiler'
 module Stylesheet; end
 
 class Stylesheet::Manager
+  BASE_COMPILER_VERSION = 1
 
   CACHE_PATH ||= 'tmp/stylesheet-cache'
   MANIFEST_DIR ||= "#{Rails.root}/tmp/cache/assets/#{Rails.env}"
-  MANIFEST_FULL_PATH ||= "#{MANIFEST_DIR}/stylesheet-manifest"
   THEME_REGEX ||= /_theme$/
   COLOR_SCHEME_STYLESHEET ||= "color_definitions"
 
@@ -105,34 +105,65 @@ class Stylesheet::Manager
     nil
   end
 
-  def self.last_file_updated
+  def self.fs_asset_cachebuster
-    if Rails.env.production?
+    if use_file_hash_for_cachebuster?
-      @last_file_updated ||= if File.exist?(MANIFEST_FULL_PATH)
+      @cachebuster ||= if File.exist?(manifest_full_path)
-        File.readlines(MANIFEST_FULL_PATH, 'r')[0]
+        File.readlines(manifest_full_path, 'r')[0]
       else
-        mtime = max_file_mtime
+        cachebuster = "#{BASE_COMPILER_VERSION}:#{fs_assets_hash}"
         FileUtils.mkdir_p(MANIFEST_DIR)
-        File.open(MANIFEST_FULL_PATH, "w") { |f| f.print(mtime) }
+        File.open(manifest_full_path, "w") { |f| f.print(cachebuster) }
-        mtime
+        cachebuster
       end
     else
-      max_file_mtime
+      "#{BASE_COMPILER_VERSION}:#{max_file_mtime}"
     end
   end
 
-  def self.max_file_mtime
+  def self.recalculate_fs_asset_cachebuster!
-    globs = ["#{Rails.root}/app/assets/stylesheets/**/*.*css",
+    File.delete(manifest_full_path) if File.exist?(manifest_full_path)
-             "#{Rails.root}/app/assets/images/**/*.*"]
+    @cachebuster = nil
+    fs_asset_cachebuster
+  end
 
-    Discourse.plugins.map { |plugin| File.dirname(plugin.path) }.each do |path|
+  def self.manifest_full_path
+    path = "#{MANIFEST_DIR}/stylesheet-manifest"
+    return path if !Rails.env.test?
+    "#{path}-test_#{ENV['TEST_ENV_NUMBER'].presence || '0'}"
+  end
+  private_class_method :manifest_full_path
+
+  def self.use_file_hash_for_cachebuster?
+    Rails.env.production?
+  end
+  private_class_method :use_file_hash_for_cachebuster?
+
+  def self.list_files
+    globs = [
+      "#{Rails.root}/app/assets/stylesheets/**/*.*css",
+      "#{Rails.root}/app/assets/images/**/*.*"
+    ]
+
+    Discourse.plugins.each do |plugin|
+      path = File.dirname(plugin.path)
       globs << "#{path}/plugin.rb"
       globs << "#{path}/assets/stylesheets/**/*.*css"
     end
 
-    globs.map do |pattern|
+    globs.flat_map { |g| Dir.glob(g) }.compact
-      Dir.glob(pattern).map { |x| File.mtime(x) }.max
-    end.compact.max.to_i
   end
+  private_class_method :list_files
+
+  def self.max_file_mtime
+    list_files.map { |x| File.mtime(x) }.compact.max.to_i
+  end
+  private_class_method :max_file_mtime
+
+  def self.fs_assets_hash
+    hashes = list_files.sort.map { |x| Digest::SHA1.hexdigest("#{x}: #{File.read(x)}") }
+    Digest::SHA1.hexdigest(hashes.join("|"))
+  end
+  private_class_method :fs_assets_hash
 
   def self.cache_fullpath
     path = "#{Rails.root}/#{CACHE_PATH}"

lib/stylesheet/manager/builder.rb

@@ -13,7 +13,7 @@ class Stylesheet::Manager::Builder
   def compile(opts = {})
     if !opts[:force]
       if File.exist?(stylesheet_fullpath)
-        unless StylesheetCache.where(target: qualified_target, digest: digest).exists?
+        if !StylesheetCache.where(target: qualified_target, digest: digest).exists?
           begin
             source_map = begin
               File.read(source_map_fullpath)
@@ -229,7 +229,7 @@ class Stylesheet::Manager::Builder
   end
 
   def default_digest
-    Digest::SHA1.hexdigest "default-#{Stylesheet::Manager.last_file_updated}-#{plugins_digest}-#{current_hostname}"
+    Digest::SHA1.hexdigest "default-#{Stylesheet::Manager.fs_asset_cachebuster}-#{plugins_digest}-#{current_hostname}"
   end
 
   def color_scheme_digest
@@ -248,9 +248,9 @@ class Stylesheet::Manager::Builder
     digest_string = "#{current_hostname}-"
     if cs || categories_updated > 0
       theme_color_defs = resolve_baked_field(:common, :color_definitions)
-      digest_string += "#{RailsMultisite::ConnectionManagement.current_db}-#{cs&.id}-#{cs&.version}-#{theme_color_defs}-#{Stylesheet::Manager.last_file_updated}-#{categories_updated}-#{fonts}"
+      digest_string += "#{RailsMultisite::ConnectionManagement.current_db}-#{cs&.id}-#{cs&.version}-#{theme_color_defs}-#{Stylesheet::Manager.fs_asset_cachebuster}-#{categories_updated}-#{fonts}"
     else
-      digest_string += "defaults-#{Stylesheet::Manager.last_file_updated}-#{fonts}"
+      digest_string += "defaults-#{Stylesheet::Manager.fs_asset_cachebuster}-#{fonts}"
 
       if cdn_url = GlobalSetting.cdn_url
         digest_string += "-#{cdn_url}"

lib/tasks/assets.rake

@@ -64,6 +64,7 @@ task 'assets:precompile:css' => 'environment' do
         STDERR.puts "-------------"
         STDERR.puts "Compiling CSS for #{db} #{Time.zone.now}"
         begin
+          Stylesheet::Manager.recalculate_fs_asset_cachebuster!
           Stylesheet::Manager.precompile_css if db == "default"
           Stylesheet::Manager.precompile_theme_css
         rescue PG::UndefinedColumn, ActiveModel::MissingAttributeError, NoMethodError => e

spec/lib/stylesheet/manager_spec.rb

@@ -873,4 +873,44 @@ RSpec.describe Stylesheet::Manager do
       expect(content).to match(/# sourceMappingURL=[^\/]+\.css\.map\?__ws=test\.localhost/)
     end
   end
+
+  describe ".fs_asset_cachebuster" do
+    it "returns a number in test/development mode" do
+      expect(Stylesheet::Manager.fs_asset_cachebuster).to match(/\A[0-9]+:[0-9]+\z/)
+    end
+
+    context "with production mode enabled" do
+      before do
+        Stylesheet::Manager.stubs(:use_file_hash_for_cachebuster?).returns(true)
+      end
+
+      after do
+        path = Stylesheet::Manager.send(:manifest_full_path)
+        File.delete(path) if File.exists?(path)
+      end
+
+      it "returns a hash" do
+        cachebuster = Stylesheet::Manager.fs_asset_cachebuster
+        expect(cachebuster).to match(/\A[0-9]+:[0-9a-f]{40}\z/)
+      end
+
+      it "caches the value on the filesystem" do
+        initial_cachebuster = Stylesheet::Manager.recalculate_fs_asset_cachebuster!
+        Stylesheet::Manager.stubs(:list_files).never
+        expect(Stylesheet::Manager.fs_asset_cachebuster).to eq(initial_cachebuster)
+        expect(File.read(Stylesheet::Manager.send(:manifest_full_path))).to eq(initial_cachebuster)
+      end
+
+      it "updates the hash when a file changes" do
+        original_files = Stylesheet::Manager.send(:list_files)
+        initial_cachebuster = Stylesheet::Manager.recalculate_fs_asset_cachebuster!
+
+        additional_file_path = "#{Rails.root}/spec/fixtures/plugins/scss_plugin/assets/stylesheets/colors.scss"
+        Stylesheet::Manager.stubs(:list_files).returns(original_files + [additional_file_path])
+
+        new_cachebuster = Stylesheet::Manager.recalculate_fs_asset_cachebuster!
+        expect(new_cachebuster).not_to eq(initial_cachebuster)
+      end
+    end
+  end
 end