Memoize Repository#empty? instead of double caching the value

We saw that in a customer instance, `empty?` was cached to be `true` even though `has_visible_content?` and `exists?` were `true`. This double caching can run into edge cases because there's no guarantee that the inner values will properly expire the outer one, especially if there is Redis replication lag. Consider this scenario: 1. `exists?` and `has_visible_content?` are false 2. `empty?` is expired 3. A subsequent call to `empty?` returns `true` because `exists?` is false even though `empty` is true 4. `exists?` and `has_visible_content?` are then expired 5. `exists?` and `has_visible_content?` are set to true 6. `empty?` is still stuck in the wrong value as `true` Closes #43882

Memoize Repository#empty? instead of double caching the value
611929eb · Stan Hu · ff00cfe4 · 611929eb · 611929eb · 611929eb
Commit 611929eb authored Mar 06, 2018 by Stan Hu
Showing with 10 additions and 5 deletions

repository.rb app/models/repository.rb +5 -3

sh-remove-double-caching-repo-empty.yml ...gelogs/unreleased/sh-remove-double-caching-repo-empty.yml +5 -0

repository_spec.rb spec/models/repository_spec.rb +0 -2

No files found.
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -35,7 +35,7 @@ class Repository
  CACHED_METHODS = %i(size commit_count rendered_readme contribution_guide
                      changelog license_blob license_key gitignore koding_yml
                      gitlab_ci_yml branch_names tag_names branch_count
-                      tag_count avatar exists? empty? root_ref has_visible_content?
+                      tag_count avatar exists? root_ref has_visible_content?
                      issue_template_names merge_request_template_names).freeze

  # Methods that use cache_method but only memoize the value
@@ -360,7 +360,7 @@ class Repository
  def expire_emptiness_caches
    return unless empty?

-    expire_method_caches(%i(empty? has_visible_content?))
+    expire_method_caches(%i(has_visible_content?))
  end

  def lookup_cache
@@ -506,12 +506,14 @@ class Repository
  end
  cache_method :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
+  # that the values are expired and loaded atomically.
  def empty?
    return true unless exists?

    !has_visible_content?
  end
-  cache_method :empty?

  # The size of this repository in megabytes.
  def size

--- a/changelogs/unreleased/sh-remove-double-caching-repo-empty.yml
+++ b/changelogs/unreleased/sh-remove-double-caching-repo-empty.yml
+---
+title: Remove double caching of Repository#empty?
+merge_request:
+author:
+type: fixed
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1447,7 +1447,6 @@ describe Repository do
    it 'expires the caches for an empty repository' do
      allow(repository).to receive(:empty?).and_return(true)

-      expect(cache).to receive(:expire).with(:empty?)
      expect(cache).to receive(:expire).with(:has_visible_content?)

      repository.expire_emptiness_caches
@@ -1456,7 +1455,6 @@ describe Repository do
    it 'does not expire the cache for a non-empty repository' do
      allow(repository).to receive(:empty?).and_return(false)

-      expect(cache).not_to receive(:expire).with(:empty?)
      expect(cache).not_to receive(:expire).with(:has_visible_content?)

      repository.expire_emptiness_caches