Mitigate stale cache across processes (related to #735)

lib/split.rb

@@ -7,6 +7,7 @@
 require "split/algorithms/weighted_sample"
 require "split/algorithms/whiplash"
 require "split/alternative"
+require "split/cache_invalidator"
 require "split/cache"
 require "split/configuration"
 require "split/encapsulated_helper"

lib/split/cache.rb

@@ -4,23 +4,36 @@ module Split
   class Cache
     def self.clear
       @cache = nil
+      CacheInvalidator.reset
     end
 
     def self.fetch(namespace, key)
       return yield unless Split.configuration.cache
 
-      @cache ||= {}
-      @cache[namespace] ||= {}
+      # Check global invalidation
+      CacheInvalidator.check_and_clear_if_needed(self)
 
-      value = @cache[namespace][key]
+      # Take a local snapshot so a concurrent `clear` (which replaces @cache)
+      # cannot make us dereference nil mid-fetch. Worst case we write into a
+      # detached hash, which is simply an extra cache miss on the next fetch.
+      cache = (@cache ||= {})
+      namespace_cache = (cache[namespace] ||= {})
+
+      value = namespace_cache[key]
       return value if value
 
-      @cache[namespace][key] = yield
+      namespace_cache[key] = yield
     end
 
     def self.clear_key(key)
-      @cache&.keys&.each do |namespace|
-        @cache[namespace]&.delete(key)
+      # Invalidate globally for all processes
+      CacheInvalidator.invalidate
+
+      # Clear from local cache immediately. Snapshot @cache and its values
+      # so a concurrent `clear` cannot nil it out mid-iteration.
+      cache = @cache
+      cache&.values&.each do |namespace_cache|
+        namespace_cache.delete(key)
       end
     end
   end

lib/split/cache_invalidator.rb

@@ -0,0 +1,75 @@
+# frozen_string_literal: true
+
+module Split
+  # Manages global cache invalidation across multiple processes using Redis timestamp.
+  # The mechanism is opt-in: it activates only when
+  # Split.configuration.cache_global_ts_check_interval is set.
+  class CacheInvalidator
+    class << self
+      # Check global timestamp and clear cache if it has been updated.
+      # No-op when cache_global_ts_check_interval is not configured.
+      # @param cache [Split::Cache] the cache instance to potentially clear
+      def check_and_clear_if_needed(cache)
+        return unless check_interval
+
+        now = Time.now
+
+        # Skip if within check interval
+        return if within_check_interval?(now)
+
+        # Get global timestamp from Redis
+        current_global_ts = fetch_global_timestamp
+
+        # Clear cache if timestamp was updated
+        cache.clear if timestamp_updated?(current_global_ts)
+
+        # Update local timestamp and check time
+        update_local_state(current_global_ts, now)
+      end
+
+      # Invalidate cache globally by updating the global timestamp.
+      # No-op when cache_global_ts_check_interval is not configured.
+      def invalidate
+        return unless check_interval
+
+        Split.redis.set(global_timestamp_key, Time.now.to_f)
+      end
+
+      # Reset local state (used when cache is cleared)
+      def reset
+        @global_cache_ts = nil
+        @last_global_ts_check = nil
+      end
+
+      private
+        def within_check_interval?(now)
+          # Snapshot the ivar so a concurrent `reset` cannot nil it between reads
+          last_check = @last_global_ts_check
+          last_check && (now.to_f - last_check.to_f) < check_interval
+        end
+
+        def fetch_global_timestamp
+          Split.redis.get(global_timestamp_key).to_f
+        end
+
+        def timestamp_updated?(current_global_ts)
+          # Snapshot the ivar so a concurrent `reset` cannot nil it between reads
+          known_ts = @global_cache_ts
+          known_ts && current_global_ts > known_ts
+        end
+
+        def update_local_state(current_global_ts, now)
+          @global_cache_ts = current_global_ts
+          @last_global_ts_check = now
+        end
+
+        def global_timestamp_key
+          "split:cache:global_ts"
+        end
+
+        def check_interval
+          Split.configuration.cache_global_ts_check_interval
+        end
+    end
+  end
+end

lib/split/configuration.rb

@@ -30,6 +30,7 @@ class Configuration
     attr_accessor :redis
     attr_accessor :dashboard_pagination_default_per_page
     attr_accessor :cache
+    attr_accessor :cache_global_ts_check_interval
 
     attr_reader :experiments
 

lib/split/experiment.rb

@@ -153,6 +153,7 @@ def has_winner?
     def winner=(winner_name)
       redis.hset(:experiment_winner, name, winner_name.to_s)
       @has_winner = true
+      Split::Cache.clear_key(@name)
       Split.configuration.on_experiment_winner_choose.call(self)
     end
 

spec/cache_spec.rb

@@ -10,9 +10,15 @@
   before { allow(Time).to receive(:now).and_return(now) }
 
   describe "clear" do
-    before { Split.configuration.cache = true }
+    before do
+      Split.configuration.cache = true
+      # Disable global invalidation check for this test
+      allow(Split::CacheInvalidator).to receive(:check_and_clear_if_needed)
+    end
 
     it "clears the cache" do
+      # First fetch: yield calls Time.now (x1)
+      # Second fetch: yield calls Time.now (x1)
       expect(Time).to receive(:now).and_return(now).exactly(2).times
       Split::Cache.fetch(namespace, key) { Time.now }
       Split::Cache.clear
@@ -21,9 +27,19 @@
   end
 
   describe "clear_key" do
-    before { Split.configuration.cache = true }
+    before do
+      Split.configuration.cache = true
+      # Disable global invalidation check for this test
+      allow(Split::CacheInvalidator).to receive(:check_and_clear_if_needed)
+    end
 
     it "clears the cache" do
+      # fetch :key1 (miss): yield (x1)
+      # fetch :key2 (miss): yield (x1)
+      # clear_key(:key1): no Time.now (invalidate is no-op without interval)
+      # fetch :key1 (miss again, cleared): yield (x1)
+      # fetch :key2 (cache hit): 0
+      # Total: 3
       expect(Time).to receive(:now).and_return(now).exactly(3).times
       Split::Cache.fetch(namespace, :key1) { Time.now }
       Split::Cache.fetch(namespace, :key2) { Time.now }
@@ -32,6 +48,17 @@
       Split::Cache.fetch(namespace, :key1) { Time.now }
       Split::Cache.fetch(namespace, :key2) { Time.now }
     end
+
+    it "updates global timestamp in Redis when invalidation is enabled" do
+      Split.configuration.cache = true
+      Split.configuration.cache_global_ts_check_interval = 5
+      current_time = Time.now.to_f
+
+      expect(Split.redis).to receive(:set).with("split:cache:global_ts", current_time)
+      allow(Time).to receive(:now).and_return(Time.at(current_time))
+
+      Split::Cache.clear_key(:some_key)
+    end
   end
 
   describe "fetch" do
@@ -52,14 +79,20 @@
     end
 
     context "when cache enabled" do
-      before { Split.configuration.cache = true }
+      before do
+        Split.configuration.cache = true
+        # Disable global invalidation check for this test
+        allow(Split::CacheInvalidator).to receive(:check_and_clear_if_needed)
+      end
 
       it "returns the yield" do
         expect(subject).to eql(now)
       end
 
       it "yields once" do
-        expect(Time).to receive(:now).and_return(now).once
+        # First fetch: yield calls Time.now (x1)
+        # Second fetch: cache hit, no yield (0)
+        expect(Time).to receive(:now).and_return(now).exactly(1).times
         Split::Cache.fetch(namespace, key) { Time.now }
         Split::Cache.fetch(namespace, key) { Time.now }
       end
@@ -81,4 +114,85 @@
       end
     end
   end
+
+  describe "thread safety" do
+    before do
+      Split.configuration.cache = true
+      # Disable global invalidation check for this test
+      allow(Split::CacheInvalidator).to receive(:check_and_clear_if_needed)
+    end
+
+    it "tolerates the cache being cleared while fetch is computing a value" do
+      # Simulates `clear` (from another thread, or triggered by global
+      # invalidation) landing between fetch taking its snapshot and writing
+      # the computed value back.
+      result = nil
+      expect {
+        result = Split::Cache.fetch(namespace, key) do
+          Split::Cache.clear
+          :computed
+        end
+      }.not_to raise_error
+      expect(result).to eq(:computed)
+    end
+  end
+
+  describe "global timestamp invalidation" do
+    before do
+      Split.configuration.cache = true
+      Split.configuration.cache_global_ts_check_interval = 5
+      Split::Cache.clear
+      Split::CacheInvalidator.reset
+    end
+
+    it "clears cache when global timestamp is updated" do
+      # Mock Redis to return initial timestamp
+      allow(Split.redis).to receive(:get).with("split:cache:global_ts").and_return("1000.0")
+
+      # First fetch - should cache the value
+      result1 = Split::Cache.fetch(namespace, key) { "value1" }
+      expect(result1).to eq("value1")
+
+      # Simulate another process updating the global timestamp
+      allow(Split.redis).to receive(:get).with("split:cache:global_ts").and_return("2000.0")
+
+      # Move time forward to bypass check interval
+      allow(Time).to receive(:now).and_return(Time.at(now + 10))
+
+      # Second fetch - should get new value because cache was cleared
+      result2 = Split::Cache.fetch(namespace, key) { "value2" }
+      expect(result2).to eq("value2")
+    end
+
+    it "does not check Redis within check interval" do
+      allow(Split.redis).to receive(:get).with("split:cache:global_ts").and_return("1000.0")
+
+      # First fetch - checks Redis
+      Split::Cache.fetch(namespace, key) { "value1" }
+
+      # Move time forward by 3 seconds (within interval)
+      allow(Time).to receive(:now).and_return(Time.at(now + 3))
+
+      # Second fetch - should NOT check Redis
+      expect(Split.redis).not_to receive(:get).with("split:cache:global_ts")
+      Split::Cache.fetch(namespace, key) { "value1" }
+    end
+
+    context "when cache_global_ts_check_interval is not configured" do
+      before do
+        Split.configuration.cache_global_ts_check_interval = nil
+        Split::Cache.clear
+        Split::CacheInvalidator.reset
+      end
+
+      it "does not read or write the global timestamp in Redis" do
+        expect(Split.redis).not_to receive(:get).with("split:cache:global_ts")
+        expect(Split.redis).not_to receive(:set).with("split:cache:global_ts", anything)
+
+        Split::Cache.fetch(namespace, key) { "value1" }
+        Split::Cache.clear_key(:some_key)
+        Split::Cache.fetch(namespace, key) { "value2" }
+      end
+    end
+  end
 end

spec/experiment_spec.rb

@@ -261,6 +261,11 @@ def alternative(color)
       experiment.winner = "green"
     end
 
+    it "should clear the cache for the experiment" do
+      expect(Split::Cache).to receive(:clear_key).with(experiment.name)
+      experiment.winner = "red"
+    end
+
     context "when has_winner state is memoized" do
       before { expect(experiment).to_not have_winner }