Merge branch 'mk/expire-count-service-cache-on-secondary' into 'master'

Geo: Secondaries react to MR and issue count changes [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!62428

app/services/base_count_service.rb

@@ -27,7 +27,7 @@ class BaseCountService
   end
 
   def delete_cache
-    Rails.cache.delete(cache_key)
+    ::Gitlab::Cache.delete(cache_key)
   end
 
   def raw?
@@ -49,4 +49,4 @@ class BaseCountService
   end
 end
 
-BaseCountService.prepend_mod_with('BaseCountService')
+BaseCountService.prepend_mod

ee/app/services/ee/base_count_service.rb

@@ -4,12 +4,12 @@ module EE
   module BaseCountService
     extend ::Gitlab::Utils::Override
 
-    # geo secondary cache should expire quicker than primary, otherwise various counts
-    # could be incorrect for 2 weeks.
-    override :cache_options
-    def cache_options
-      super.tap do |options|
-        options[:expires_in] = 20.minutes if ::Gitlab::Geo.secondary?
+    # When updating a cached count on a Geo primary, also invalidate the key on
+    # Geo secondaries.
+    override :update_cache_for_key
+    def update_cache_for_key(key, &block)
+      super.tap do
+        ::Gitlab::Cache.delete_on_geo_secondaries(key)
       end
     end
   end

ee/app/services/geo/cache_invalidation_event_store.rb

@@ -7,7 +7,8 @@ module Geo
     attr_reader :key
 
     def initialize(key)
-      @key = key
+      # Rails cache keys are often not `String`, see https://guides.rubyonrails.org/caching_with_rails.html#cache-keys
+      @key = ::ActiveSupport::Cache.expand_cache_key(key)
     end
 
     private

ee/lib/ee/gitlab/cache.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Cache
+      extend ActiveSupport::Concern
+
+      class_methods do
+        extend ::Gitlab::Utils::Override
+
+        # Utility to `Rails.cache.delete` *and* propagate to Geo secondaries.
+        override :delete
+        def delete(key)
+          super.tap do
+            delete_on_geo_secondaries(key)
+          end
+        end
+
+        def delete_on_geo_secondaries(key)
+          Geo::CacheInvalidationEventStore.new(key).create!
+        end
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/cache_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Cache do
+  include ::EE::GeoHelpers
+
+  describe '.delete' do
+    let(:key) { %w{a cache key} }
+
+    subject(:delete) { described_class.delete(key) }
+
+    it 'calls Rails.cache.delete' do
+      expect(Rails.cache).to receive(:delete).with(key)
+
+      delete
+    end
+
+    it 'calls .delete_on_geo_secondaries' do
+      expect(described_class).to receive(:delete_on_geo_secondaries).with(key)
+
+      delete
+    end
+  end
+
+  describe '.delete_on_geo_secondaries' do
+    let(:key) { %w{a cache key} }
+
+    subject(:delete_on_geo_secondaries) { described_class.delete_on_geo_secondaries(key) }
+
+    context 'without Geo' do
+      it 'does not create a Geo::CacheInvalidationEvent' do
+        expect do
+          delete_on_geo_secondaries
+        end.not_to change { ::Geo::CacheInvalidationEvent.count }
+      end
+    end
+
+    context 'for a Geo primary site' do
+      before do
+        stub_primary_node
+      end
+
+      context 'when there is at least one Geo secondary site' do
+        before do
+          allow(::Gitlab::Geo).to receive(:secondary_nodes).and_return(double(any?: true))
+        end
+
+        it 'creates a Geo::CacheInvalidationEvent' do
+          expect do
+            delete_on_geo_secondaries
+          end.to change { ::Geo::CacheInvalidationEvent.count }.by(1)
+        end
+      end
+
+      context 'when there are no Geo secondary sites' do
+        before do
+          allow(::Gitlab::Geo).to receive(:secondary_nodes).and_return(double(any?: false))
+        end
+
+        it 'does not create a Geo::CacheInvalidationEvent' do
+          expect do
+            delete_on_geo_secondaries
+          end.not_to change { ::Geo::CacheInvalidationEvent.count }
+        end
+      end
+    end
+
+    context 'for a Geo secondary site' do
+      before do
+        stub_secondary_node
+      end
+
+      context 'when there is at least one Geo secondary site' do
+        before do
+          allow(::Gitlab::Geo).to receive(:secondary_nodes).and_return(double(any?: true))
+        end
+
+        it 'does not create a Geo::CacheInvalidationEvent' do
+          expect do
+            delete_on_geo_secondaries
+          end.not_to change { ::Geo::CacheInvalidationEvent.count }
+        end
+      end
+    end
+  end
+end

ee/spec/services/base_count_service_spec.rb

@@ -5,28 +5,13 @@ require 'spec_helper'
 RSpec.describe BaseCountService do
   include ::EE::GeoHelpers
 
-  describe '#cache_options' do
-    subject { described_class.new.cache_options }
+  describe '#update_cache_for_key' do
+    let(:key) { %w{a cache key} }
 
-    it 'returns the default' do
-      stub_current_geo_node(nil)
+    it 'calls Gitlab::Cache.delete_on_geo_secondaries' do
+      expect(::Gitlab::Cache).to receive(:delete_on_geo_secondaries).with(key)
 
-      is_expected.to include(:raw)
-      is_expected.not_to include(:expires_in)
-    end
-
-    it 'returns default on a Geo primary' do
-      stub_current_geo_node(create(:geo_node, :primary))
-
-      is_expected.to include(:raw)
-      is_expected.not_to include(:expires_in)
-    end
-
-    it 'returns cache of 20 mins on a Geo secondary' do
-      stub_current_geo_node(create(:geo_node))
-
-      is_expected.to include(:raw)
-      is_expected.to include(expires_in: 20.minutes)
+      described_class.new.update_cache_for_key(key) { 123 }
     end
   end
 end

ee/spec/services/geo/cache_invalidation_event_store_spec.rb

@@ -11,6 +11,22 @@ RSpec.describe Geo::CacheInvalidationEventStore do
 
   subject { described_class.new(cache_key) }
 
+  describe '#initialize' do
+    context 'when the key is a String' do
+      it 'does not modify the key' do
+        expect(subject.key).to eq(cache_key)
+      end
+    end
+
+    context 'when the key is an Array' do
+      let(:cache_key) { %w{a cache key} }
+
+      it 'expands the key' do
+        expect(subject.key).to eq('a/cache/key')
+      end
+    end
+  end
+
   describe '#create' do
     it_behaves_like 'a Geo event store', Geo::CacheInvalidationEvent
 

lib/gitlab/cache.rb

@@ -13,6 +13,13 @@ module Gitlab
           end
         end
       end
+
+      # Hook for EE
+      def delete(key)
+        Rails.cache.delete(key)
+      end
     end
   end
 end
+
+Gitlab::Cache.prepend_mod

spec/lib/gitlab/cache_spec.rb

@@ -26,4 +26,16 @@ RSpec.describe Gitlab::Cache, :request_store do
       expect(subject.call).to eq("return value")
     end
   end
+
+  describe '.delete' do
+    let(:key) { %w{a cache key} }
+
+    subject(:delete) { described_class.delete(key) }
+
+    it 'calls Rails.cache.delete' do
+      expect(Rails.cache).to receive(:delete).with(key)
+
+      delete
+    end
+  end
 end

spec/services/issues/close_service_spec.rb

@@ -222,7 +222,7 @@ RSpec.describe Issues::CloseService do
 
       it 'verifies the number of queries' do
         recorded = ActiveRecord::QueryRecorder.new { close_issue }
-        expected_queries = 23
+        expected_queries = 24
 
         expect(recorded.count).to be <= expected_queries
         expect(recorded.cached_count).to eq(0)