From 4346ab0149c9094c24e0bb925398cd72bb0e2516 Mon Sep 17 00:00:00 2001 From: Paul Sokolik Date: Mon, 23 Feb 2026 10:56:41 -0500 Subject: [PATCH 1/2] Fix process-global memory leak in folded_declaration_cache In long-lived processes (e.g., Sidekiq workers rendering emails via Premailer), the process-global `@folded_declaration_cache` on `CssParser::Parser` accumulates entries from every CSS parse operation and is never cleared, causing unbounded memory growth. The class-level `@folded_declaration_cache` (and its `attr_reader`) on `CssParser::Parser` was defined as a class instance variable that persists for the lifetime of the process. While `reset!` (called in `initialize`) creates a per-instance `@folded_declaration_cache`, the class-level variable and accessor remained, leaking the global reference. Similarly, `CssParser.merge` set a module-level `@folded_declaration_cache = {}` on every call, which also persisted at the module level. This commit: - Removes the class-level `@folded_declaration_cache` and its `class << self; attr_reader` from `CssParser::Parser` - Removes the module-level `@folded_declaration_cache` assignment in `CssParser.merge` - The per-instance cache (set in `reset!`) is already properly scoped and gets garbage collected with the `Parser` instance - Adds tests verifying the cache is per-instance and not class-level Amp-Thread-ID: https://ampcode.com/threads/T-019c8b1f-829d-70cd-83c5-3409a635e616 Co-authored-by: Amp --- lib/css_parser.rb | 2 - lib/css_parser/parser.rb | 6 --- ...est_css_parser_folded_declaration_cache.rb | 42 +++++++++++++++++++ 3 files changed, 42 insertions(+), 8 deletions(-) create mode 100644 test/test_css_parser_folded_declaration_cache.rb diff --git a/lib/css_parser.rb b/lib/css_parser.rb index f62e180..0cbe40d 100644 --- a/lib/css_parser.rb +++ b/lib/css_parser.rb @@ -52,8 +52,6 @@ module CssParser # TODO: declaration_hashes should be able to contain a RuleSet # this should be a Class method def self.merge(*rule_sets) - @folded_declaration_cache = {} - # in case called like CssParser.merge([rule_set, rule_set]) rule_sets.flatten! if rule_sets[0].is_a?(Array) diff --git a/lib/css_parser/parser.rb b/lib/css_parser/parser.rb index 57d3a5e..0dddb98 100644 --- a/lib/css_parser/parser.rb +++ b/lib/css_parser/parser.rb @@ -31,12 +31,6 @@ class Parser # Array of CSS files that have been loaded. attr_reader :loaded_uris - #-- - # Class variable? see http://www.oreillynet.com/ruby/blog/2007/01/nubygems_dont_use_class_variab_1.html - #++ - @folded_declaration_cache = {} - class << self; attr_reader :folded_declaration_cache; end - def initialize(options = {}) @options = { absolute_paths: false, diff --git a/test/test_css_parser_folded_declaration_cache.rb b/test/test_css_parser_folded_declaration_cache.rb new file mode 100644 index 0000000..8e811d1 --- /dev/null +++ b/test/test_css_parser_folded_declaration_cache.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require_relative 'test_helper' + +class CssParserFoldedDeclarationCacheTest < Minitest::Test + include CssParser + + def test_folded_declaration_cache_is_per_instance + cp1 = Parser.new + cp2 = Parser.new + + cp1.add_block!('p { color: red; }') + cp2.add_block!('p { color: blue; }') + + cache1 = cp1.send(:instance_variable_get, :@folded_declaration_cache) + cache2 = cp2.send(:instance_variable_get, :@folded_declaration_cache) + + refute_same cache1, cache2 + end + + def test_folded_declaration_cache_does_not_persist_across_instances + cp1 = Parser.new + cp1.add_block!('p { color: red; }') + + cp2 = Parser.new + cache = cp2.send(:instance_variable_get, :@folded_declaration_cache) + + assert_empty cache + end + + def test_no_class_level_folded_declaration_cache + refute Parser.respond_to?(:folded_declaration_cache), + 'Parser should not have a class-level folded_declaration_cache accessor' + end + + def test_cache_is_reset_on_new_instance + cp = Parser.new + cache = cp.send(:instance_variable_get, :@folded_declaration_cache) + assert_instance_of Hash, cache + assert_empty cache + end +end From 69408221cb4be34e99e27bee9d26f64949a71468 Mon Sep 17 00:00:00 2001 From: Paul Sokolik Date: Mon, 23 Feb 2026 14:38:53 -0500 Subject: [PATCH 2/2] Remove tests for deleted class-level cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tests were asserting the absence of removed code — unnecessary. Amp-Thread-ID: https://ampcode.com/threads/T-019c8b1f-829d-70cd-83c5-3409a635e616 Co-authored-by: Amp --- ...est_css_parser_folded_declaration_cache.rb | 42 ------------------- 1 file changed, 42 deletions(-) delete mode 100644 test/test_css_parser_folded_declaration_cache.rb diff --git a/test/test_css_parser_folded_declaration_cache.rb b/test/test_css_parser_folded_declaration_cache.rb deleted file mode 100644 index 8e811d1..0000000 --- a/test/test_css_parser_folded_declaration_cache.rb +++ /dev/null @@ -1,42 +0,0 @@ -# frozen_string_literal: true - -require_relative 'test_helper' - -class CssParserFoldedDeclarationCacheTest < Minitest::Test - include CssParser - - def test_folded_declaration_cache_is_per_instance - cp1 = Parser.new - cp2 = Parser.new - - cp1.add_block!('p { color: red; }') - cp2.add_block!('p { color: blue; }') - - cache1 = cp1.send(:instance_variable_get, :@folded_declaration_cache) - cache2 = cp2.send(:instance_variable_get, :@folded_declaration_cache) - - refute_same cache1, cache2 - end - - def test_folded_declaration_cache_does_not_persist_across_instances - cp1 = Parser.new - cp1.add_block!('p { color: red; }') - - cp2 = Parser.new - cache = cp2.send(:instance_variable_get, :@folded_declaration_cache) - - assert_empty cache - end - - def test_no_class_level_folded_declaration_cache - refute Parser.respond_to?(:folded_declaration_cache), - 'Parser should not have a class-level folded_declaration_cache accessor' - end - - def test_cache_is_reset_on_new_instance - cp = Parser.new - cache = cp.send(:instance_variable_get, :@folded_declaration_cache) - assert_instance_of Hash, cache - assert_empty cache - end -end