Merge branch 'mk/asymmetric-exists-cache' into 'master'

Resolve "Geo: Does not mark repositories as missing on primary due to stale cache"

Closes #50211 and #44044

See merge request gitlab-org/gitlab-ce!21789

app/models/repository.rb

@@ -510,7 +510,7 @@ class Repository
 
     raw_repository.exists?
   end
-  cache_method :exists?
+  cache_method_asymmetrically :exists?
 
   # We don't need to cache the output of this method because both exists? and
   # has_visible_content? are already memoized and cached. There's no guarantee
@@ -612,7 +612,7 @@ class Repository
 
     Licensee::License.new(license_key)
   end
-  cache_method :license, memoize_only: true
+  memoize_method :license
 
   def gitignore
     file_on_head(:gitignore)
@@ -1029,6 +1029,10 @@ class Repository
     @cache ||= Gitlab::RepositoryCache.new(self)
   end
 
+  def request_store_cache
+    @request_store_cache ||= Gitlab::RepositoryCache.new(self, backend: Gitlab::SafeRequestStore)
+  end
+
   def tags_sorted_by_committed_date
     tags.sort_by do |tag|
       # Annotated tags can point to any object (e.g. a blob), but generally

changelogs/unreleased/mk-asymmetric-exists-cache.yml

@@ -0,0 +1,6 @@
+---
+title: 'Resolve "Geo: Does not mark repositories as missing on primary due to stale
+  cache"'
+merge_request: 21789
+author:
+type: fixed

lib/gitlab/repository_cache.rb

@@ -29,5 +29,21 @@ module Gitlab
     def read(key)
       backend.read(cache_key(key))
     end
+
+    def write(key, value)
+      backend.write(cache_key(key), value)
+    end
+
+    def fetch_without_caching_false(key, &block)
+      value = read(key)
+      return value if value
+
+      value = yield
+
+      # Don't cache false values
+      write(key, value) if value
+
+      value
+    end
   end
 end

lib/gitlab/repository_cache_adapter.rb

@@ -1,23 +1,80 @@
 module Gitlab
   module RepositoryCacheAdapter
     extend ActiveSupport::Concern
+    include Gitlab::Utils::StrongMemoize
 
     class_methods do
-      # Wraps around the given method and caches its output in Redis and an instance
-      # variable.
+      # Caches and strongly memoizes the method.
       #
       # This only works for methods that do not take any arguments.
-      def cache_method(name, fallback: nil, memoize_only: false)
-        original = :"_uncached_#{name}"
-
-        alias_method(original, name)
+      #
+      # name     - The name of the method to be cached.
+      # fallback - A value to fall back to if the repository does not exist, or
+      #            in case of a Git error. Defaults to nil.
+      def cache_method(name, fallback: nil)
+        uncached_name = alias_uncached_method(name)
 
         define_method(name) do
-          cache_method_output(name, fallback: fallback, memoize_only: memoize_only) do
-            __send__(original) # rubocop:disable GitlabSecurity/PublicSend
+          cache_method_output(name, fallback: fallback) do
+            __send__(uncached_name) # rubocop:disable GitlabSecurity/PublicSend
           end
         end
       end
+
+      # Caches truthy values from the method. All values are strongly memoized,
+      # and cached in RequestStore.
+      #
+      # Currently only used to cache `exists?` since stale false values are
+      # particularly troublesome. This can occur, for example, when an NFS mount
+      # is temporarily down.
+      #
+      # This only works for methods that do not take any arguments.
+      #
+      # name - The name of the method to be cached.
+      def cache_method_asymmetrically(name)
+        uncached_name = alias_uncached_method(name)
+
+        define_method(name) do
+          cache_method_output_asymmetrically(name) do
+            __send__(uncached_name) # rubocop:disable GitlabSecurity/PublicSend
+          end
+        end
+      end
+
+      # Strongly memoizes the method.
+      #
+      # This only works for methods that do not take any arguments.
+      #
+      # name     - The name of the method to be memoized.
+      # fallback - A value to fall back to if the repository does not exist, or
+      #            in case of a Git error. Defaults to nil. The fallback value
+      #            is not memoized.
+      def memoize_method(name, fallback: nil)
+        uncached_name = alias_uncached_method(name)
+
+        define_method(name) do
+          memoize_method_output(name, fallback: fallback) do
+            __send__(uncached_name) # rubocop:disable GitlabSecurity/PublicSend
+          end
+        end
+      end
+
+      # Prepends "_uncached_" to the target method name
+      #
+      # Returns the uncached method name
+      def alias_uncached_method(name)
+        uncached_name = :"_uncached_#{name}"
+
+        alias_method(uncached_name, name)
+
+        uncached_name
+      end
+    end
+
+    # RequestStore-backed RepositoryCache to be used. Should be overridden by
+    # the including class
+    def request_store_cache
+      raise NotImplementedError
     end
 
     # RepositoryCache to be used. Should be overridden by the including class
@@ -30,65 +87,93 @@ module Gitlab
       raise NotImplementedError
     end
 
-    # Caches the supplied block both in a cache and in an instance variable.
+    # Caches and strongly memoizes the supplied block.
     #
-    # The cache key and instance variable are named the same way as the value of
-    # the `key` argument.
-    #
-    # This method will return `nil` if the corresponding instance variable is also
-    # set to `nil`. This ensures we don't keep yielding the block when it returns
-    # `nil`.
-    #
-    # key - The name of the key to cache the data in.
-    # fallback - A value to fall back to in the event of a Git error.
-    def cache_method_output(key, fallback: nil, memoize_only: false, &block)
-      ivar = cache_instance_variable_name(key)
+    # name     - The name of the method to be cached.
+    # fallback - A value to fall back to if the repository does not exist, or
+    #            in case of a Git error. Defaults to nil.
+    def cache_method_output(name, fallback: nil, &block)
+      memoize_method_output(name, fallback: fallback) do
+        cache.fetch(name, &block)
+      end
+    end
 
-      if instance_variable_defined?(ivar)
-        instance_variable_get(ivar)
-      else
-        # If the repository doesn't exist and a fallback was specified we return
-        # that value inmediately. This saves us Rugged/gRPC invocations.
-        return fallback unless fallback.nil? || cache.repository.exists?
+    # Caches truthy values from the supplied block. All values are strongly
+    # memoized, and cached in RequestStore.
+    #
+    # Currently only used to cache `exists?` since stale false values are
+    # particularly troublesome. This can occur, for example, when an NFS mount
+    # is temporarily down.
+    #
+    # name - The name of the method to be cached.
+    def cache_method_output_asymmetrically(name, &block)
+      memoize_method_output(name) do
+        request_store_cache.fetch(name) do
+          cache.fetch_without_caching_false(name, &block)
+        end
+      end
+    end
+
+    # Strongly memoizes the supplied block.
+    #
+    # name     - The name of the method to be memoized.
+    # fallback - A value to fall back to if the repository does not exist, or
+    #            in case of a Git error. Defaults to nil. The fallback value is
+    #            not memoized.
+    def memoize_method_output(name, fallback: nil, &block)
+      no_repository_fallback(name, fallback: fallback) do
+        strong_memoize(memoizable_name(name), &block)
+      end
+    end
+
+    # Returns the fallback value if the repository does not exist
+    def no_repository_fallback(name, fallback: nil, &block)
+      # Avoid unnecessary gRPC invocations
+      return fallback if fallback && fallback_early?(name)
 
-        begin
-          value =
-            if memoize_only
       yield
-            else
-              cache.fetch(key, &block)
-            end
-
-          instance_variable_set(ivar, value)
     rescue Gitlab::Git::Repository::NoRepository
-          # Even if the above `#exists?` check passes these errors might still
-          # occur (for example because of a non-existing HEAD). We want to
-          # gracefully handle this and not cache anything
+      # Even if the `#exists?` check in `fallback_early?` passes, these errors
+      # might still occur (for example because of a non-existing HEAD). We
+      # want to gracefully handle this and not memoize anything.
       fallback
     end
-      end
-    end
 
     # Expires the caches of a specific set of methods
     def expire_method_caches(methods)
-      methods.each do |key|
-        unless cached_methods.include?(key.to_sym)
-          Rails.logger.error "Requested to expire non-existent method '#{key}' for Repository"
+      methods.each do |name|
+        unless cached_methods.include?(name.to_sym)
+          Rails.logger.error "Requested to expire non-existent method '#{name}' for Repository"
           next
         end
 
-        cache.expire(key)
+        cache.expire(name)
 
-        ivar = cache_instance_variable_name(key)
-
-        remove_instance_variable(ivar) if instance_variable_defined?(ivar)
+        clear_memoization(memoizable_name(name))
       end
+
+      expire_request_store_method_caches(methods)
     end
 
     private
 
-    def cache_instance_variable_name(key)
-      :"@#{key.to_s.tr('?!', '')}"
+    def memoizable_name(name)
+      "#{name.to_s.tr('?!', '')}"
+    end
+
+    def expire_request_store_method_caches(methods)
+      methods.each do |name|
+        request_store_cache.expire(name)
+      end
+    end
+
+    # All cached repository methods depend on the existence of a Git repository,
+    # so if the repository doesn't exist, we already know not to call it.
+    def fallback_early?(method_name)
+      # Avoid infinite loop
+      return false if method_name == :exists?
+
+      !exists?
     end
   end
 end

spec/controllers/projects/pipelines_controller_spec.rb

@@ -90,6 +90,11 @@ describe Projects::PipelinesController do
 
     context 'when performing gitaly calls', :request_store do
       it 'limits the Gitaly requests' do
+        # Isolate from test preparation (Repository#exists? is also cached in RequestStore)
+        RequestStore.end!
+        RequestStore.clear!
+        RequestStore.begin!
+
         expect { get_pipelines_index_json }
           .to change { Gitlab::GitalyClient.get_request_count }.by(2)
       end

spec/lib/gitlab/repository_cache_adapter_spec.rb

@@ -65,6 +65,144 @@ describe Gitlab::RepositoryCacheAdapter do
     end
   end
 
+  describe '#cache_method_output_asymmetrically', :use_clean_rails_memory_store_caching, :request_store do
+    let(:request_store_cache) { repository.send(:request_store_cache) }
+
+    context 'with a non-existing repository' do
+      let(:project) { create(:project) } # No repository
+      let(:object) { double }
+
+      subject do
+        repository.cache_method_output_asymmetrically(:cats) do
+          object.cats_call_stub
+        end
+      end
+
+      it 'returns the output of the original method' do
+        expect(object).to receive(:cats_call_stub).and_return('output')
+
+        expect(subject).to eq('output')
+      end
+    end
+
+    context 'with a method throwing a non-existing-repository error' do
+      subject do
+        repository.cache_method_output_asymmetrically(:cats) do
+          raise Gitlab::Git::Repository::NoRepository
+        end
+      end
+
+      it 'returns nil' do
+        expect(subject).to eq(nil)
+      end
+
+      it 'does not cache the data' do
+        subject
+
+        expect(repository.instance_variable_defined?(:@cats)).to eq(false)
+        expect(cache.exist?(:cats)).to eq(false)
+      end
+    end
+
+    context 'with an existing repository' do
+      let(:object) { double }
+
+      context 'when it returns truthy' do
+        before do
+          expect(object).to receive(:cats).once.and_return('truthy output')
+        end
+
+        it 'caches the output in RequestStore' do
+          expect do
+            repository.cache_method_output_asymmetrically(:cats) { object.cats }
+          end.to change { request_store_cache.read(:cats) }.from(nil).to('truthy output')
+        end
+
+        it 'caches the output in RepositoryCache' do
+          expect do
+            repository.cache_method_output_asymmetrically(:cats) { object.cats }
+          end.to change { cache.read(:cats) }.from(nil).to('truthy output')
+        end
+      end
+
+      context 'when it returns false' do
+        before do
+          expect(object).to receive(:cats).once.and_return(false)
+        end
+
+        it 'caches the output in RequestStore' do
+          expect do
+            repository.cache_method_output_asymmetrically(:cats) { object.cats }
+          end.to change { request_store_cache.read(:cats) }.from(nil).to(false)
+        end
+
+        it 'does NOT cache the output in RepositoryCache' do
+          expect do
+            repository.cache_method_output_asymmetrically(:cats) { object.cats }
+          end.not_to change { cache.read(:cats) }.from(nil)
+        end
+      end
+    end
+  end
+
+  describe '#memoize_method_output' do
+    let(:fallback) { 10 }
+
+    context 'with a non-existing repository' do
+      let(:project) { create(:project) } # No repository
+
+      subject do
+        repository.memoize_method_output(:cats, fallback: fallback) do
+          repository.cats_call_stub
+        end
+      end
+
+      it 'returns the fallback value' do
+        expect(subject).to eq(fallback)
+      end
+
+      it 'avoids calling the original method' do
+        expect(repository).not_to receive(:cats_call_stub)
+
+        subject
+      end
+
+      it 'does not set the instance variable' do
+        subject
+
+        expect(repository.instance_variable_defined?(:@cats)).to eq(false)
+      end
+    end
+
+    context 'with a method throwing a non-existing-repository error' do
+      subject do
+        repository.memoize_method_output(:cats, fallback: fallback) do
+          raise Gitlab::Git::Repository::NoRepository
+        end
+      end
+
+      it 'returns the fallback value' do
+        expect(subject).to eq(fallback)
+      end
+
+      it 'does not set the instance variable' do
+        subject
+
+        expect(repository.instance_variable_defined?(:@cats)).to eq(false)
+      end
+    end
+
+    context 'with an existing repository' do
+      it 'sets the instance variable' do
+        repository.memoize_method_output(:cats, fallback: fallback) do
+          'block output'
+        end
+
+        expect(repository.instance_variable_get(:@cats)).to eq('block output')
+      end
+    end
+  end
+
   describe '#expire_method_caches' do
     it 'expires the caches of the given methods' do
       expect(cache).to receive(:expire).with(:rendered_readme)

spec/lib/gitlab/repository_cache_spec.rb

@@ -47,4 +47,89 @@ describe Gitlab::RepositoryCache do
       expect(backend).to have_received(:fetch).with("baz:#{namespace}", &p)
     end
   end
+
+  describe '#fetch_without_caching_false', :use_clean_rails_memory_store_caching do
+    let(:key) { :foo }
+    let(:backend) { Rails.cache }
+
+    it 'requires a block' do
+      expect do
+        cache.fetch_without_caching_false(key)
+      end.to raise_error(LocalJumpError)
+    end
+
+    context 'when the key does not exist in the cache' do
+      context 'when the result of the block is truthy' do
+        it 'returns the result of the block' do
+          result = cache.fetch_without_caching_false(key) { true }
+
+          expect(result).to be true
+        end
+
+        it 'caches the value' do
+          expect(backend).to receive(:write).with("#{key}:#{namespace}", true)
+
+          cache.fetch_without_caching_false(key) { true }
+        end
+      end
+
+      context 'when the result of the block is falsey' do
+        let(:p) { -> { false } }
+
+        it 'returns the result of the block' do
+          result = cache.fetch_without_caching_false(key, &p)
+
+          expect(result).to be false
+        end
+
+        it 'does not cache the value' do
+          expect(backend).not_to receive(:write).with("#{key}:#{namespace}", true)
+
+          cache.fetch_without_caching_false(key, &p)
+        end
+      end
+    end
+
+    context 'when the cached value is truthy' do
+      before do
+        backend.write("#{key}:#{namespace}", true)
+      end
+
+      it 'returns the cached value' do
+        result = cache.fetch_without_caching_false(key) { 'block result' }
+
+        expect(result).to be true
+      end
+
+      it 'does not execute the block' do
+        expect do |b|
+          cache.fetch_without_caching_false(key, &b)
+        end.not_to yield_control
+      end
+
+      it 'does not write to the cache' do
+        expect(backend).not_to receive(:write)
+
+        cache.fetch_without_caching_false(key) { 'block result' }
+      end
+    end
+
+    context 'when the cached value is falsey' do
+      before do
+        backend.write("#{key}:#{namespace}", false)
+      end
+
+      it 'returns the result of the block' do
+        result = cache.fetch_without_caching_false(key) { 'block result' }
+
+        expect(result).to eq 'block result'
+      end
+
+      it 'writes the truthy value to the cache' do
+        expect(backend).to receive(:write).with("#{key}:#{namespace}", 'block result')
+
+        cache.fetch_without_caching_false(key) { 'block result' }
+      end
+    end
+  end
 end

spec/models/repository_spec.rb

@@ -1044,6 +1044,47 @@ describe Repository do
         expect_to_raise_storage_error { broken_repository.exists? }
       end
     end
+
+    context 'asymmetric caching', :use_clean_rails_memory_store_caching, :request_store do
+      let(:cache) { repository.send(:cache) }
+      let(:request_store_cache) { repository.send(:request_store_cache) }
+
+      context 'when it returns true' do
+        before do
+          expect(repository.raw_repository).to receive(:exists?).once.and_return(true)
+        end
+
+        it 'caches the output in RequestStore' do
+          expect do
+            repository.exists?
+          end.to change { request_store_cache.read(:exists?) }.from(nil).to(true)
+        end
+
+        it 'caches the output in RepositoryCache' do
+          expect do
+            repository.exists?
+          end.to change { cache.read(:exists?) }.from(nil).to(true)
+        end
+      end
+
+      context 'when it returns false' do
+        before do
+          expect(repository.raw_repository).to receive(:exists?).once.and_return(false)
+        end
+
+        it 'caches the output in RequestStore' do
+          expect do
+            repository.exists?
+          end.to change { request_store_cache.read(:exists?) }.from(nil).to(false)
+        end
+
+        it 'does NOT cache the output in RepositoryCache' do
+          expect do
+            repository.exists?
+          end.not_to change { cache.read(:exists?) }.from(nil)
+        end
+      end
+    end
   end
 
   describe '#has_visible_content?' do
@@ -1716,12 +1757,19 @@ describe Repository do
 
   describe '#expire_exists_cache' do
     let(:cache) { repository.send(:cache) }
+    let(:request_store_cache) { repository.send(:request_store_cache) }
 
     it 'expires the cache' do
       expect(cache).to receive(:expire).with(:exists?)
 
       repository.expire_exists_cache
     end
+
+    it 'expires the request store cache', :request_store do
+      expect(request_store_cache).to receive(:expire).with(:exists?)
+
+      repository.expire_exists_cache
+    end
   end
 
   describe '#xcode_project?' do
@@ -1892,7 +1940,7 @@ describe Repository do
         match[1].to_sym if match
       end.compact
 
-      expect(methods).to match_array(Repository::CACHED_METHODS + Repository::MEMOIZED_CACHED_METHODS)
+      expect(Repository::CACHED_METHODS + Repository::MEMOIZED_CACHED_METHODS).to include(*methods)
     end
   end