RepoRefCache: Hold a reference to the refDatabase with ref counting

The RepoRefCache was holding a reference to a refDatabase associated
to a Repository object that was created externally and could have been
already closed once the RepoRefCache.get was called.

Increment and decrement the reference counting for the Repository
object held in the RepoRefCache, making sure that all accesses to
the get() and other methods are always using an active and open
repository.

The problem existed for years but it was never noticed because
of JGit being very flexible on using repositories even when
their reference counting is zero and they are effectively closed.
However, using a closed repository is inconsistent with its interface
and may lead to further issues in the past, because of JGit not
being aware of the repository is actually used by an in-memory
cache holding a reference to it.

Add one extra relevant tests associated with this corner case for
showing the issue, using a TestRepositoryWithRefCounting wrapper
that fails when accessing a refDatabase of a closed repository.

Release-Notes: skip
Change-Id: I4b2c43ea430a0506c73101bc4f6bc62926d5a094

5 files changed, 320 insertions, 4 deletions

diff --git a/java/com/google/gerrit/server/git/RefCache.java b/java/com/google/gerrit/server/git/RefCache.java
index 5a5cae9332..2dee427bf7 100644
--- a/java/com/google/gerrit/server/git/RefCache.java
+++ b/java/com/google/gerrit/server/git/RefCache.java
@@ -37,4 +37,7 @@ public interface RefCache {
    *     present with a value of {@link ObjectId#zeroId()}.
    */
   Optional<ObjectId> get(String refName) throws IOException;
+
+  /** Closes this cache, releasing the references to any underlying resources. */
+  void close();
 }
diff --git a/java/com/google/gerrit/server/git/RepoRefCache.java b/java/com/google/gerrit/server/git/RepoRefCache.java
index 9086da7fb5..1fe905e5e3 100644
--- a/java/com/google/gerrit/server/git/RepoRefCache.java
+++ b/java/com/google/gerrit/server/git/RepoRefCache.java
@@ -29,6 +29,7 @@ import org.eclipse.jgit.lib.Repository;
 public class RepoRefCache implements RefCache {
   private final RefDatabase refdb;
   private final Map<String, Optional<ObjectId>> ids;
+  private final Repository repo;
 
   public static Optional<RefCache> getOptional(Repository repo) {
     PerThreadCache cache = PerThreadCache.get();
@@ -42,6 +43,8 @@ public class RepoRefCache implements RefCache {
   }
 
   public RepoRefCache(Repository repo) {
+    repo.incrementOpen();
+    this.repo = repo;
     this.refdb = repo.getRefDatabase();
     this.ids = new HashMap<>();
   }
@@ -62,4 +65,9 @@ public class RepoRefCache implements RefCache {
   public Map<String, Optional<ObjectId>> getCachedRefs() {
     return Collections.unmodifiableMap(ids);
   }
+
+  @Override
+  public void close() {
+    repo.close();
+  }
 }
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index e2d34facfe..3bceeeecda 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -752,9 +752,25 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
         // ReviewDb claims NoteDb state exists, but meta ref isn't present: fall through and
         // auto-rebuild if necessary.
       }
-      RefCache refs = RepoRefCache.getOptional(repo).orElse(new RepoRefCache(repo));
-      if (!NoteDbChangeState.isChangeUpToDate(state, refs, getChangeId())) {
-        return rebuildAndOpen(repo, id);
+      RefCache refs;
+      RepoRefCache newRefCache = null;
+      Optional<RefCache> cachedRefCache = RepoRefCache.getOptional(repo);
+
+      if (cachedRefCache.isPresent()) {
+        refs = cachedRefCache.get();
+      } else {
+        newRefCache = new RepoRefCache(repo);
+        refs = newRefCache;
+      }
+
+      try {
+        if (!NoteDbChangeState.isChangeUpToDate(state, refs, getChangeId())) {
+          return rebuildAndOpen(repo, id);
+        }
+      } finally {
+        if (newRefCache != null) {
+          newRefCache.close();
+        }
       }
     }
     return super.openHandle(repo);
diff --git a/java/com/google/gerrit/server/update/ChainedReceiveCommands.java b/java/com/google/gerrit/server/update/ChainedReceiveCommands.java
index c223aec637..9a395759bf 100644
--- a/java/com/google/gerrit/server/update/ChainedReceiveCommands.java
+++ b/java/com/google/gerrit/server/update/ChainedReceiveCommands.java
@@ -39,13 +39,19 @@ import org.eclipse.jgit.transport.ReceiveCommand;
 public class ChainedReceiveCommands implements RefCache {
   private final Map<String, ReceiveCommand> commands = new LinkedHashMap<>();
   private final RepoRefCache refCache;
+  private final boolean closeRefCache;
 
   public ChainedReceiveCommands(Repository repo) {
-    this(new RepoRefCache(repo));
+    this(new RepoRefCache(repo), true);
   }
 
   public ChainedReceiveCommands(RepoRefCache refCache) {
+    this(refCache, false);
+  }
+
+  private ChainedReceiveCommands(RepoRefCache refCache, boolean closeRefCache) {
     this.refCache = requireNonNull(refCache);
+    this.closeRefCache = closeRefCache;
   }
 
   public RepoRefCache getRepoRefCache() {
@@ -122,4 +128,11 @@ public class ChainedReceiveCommands implements RefCache {
   public Map<String, ReceiveCommand> getCommands() {
     return Collections.unmodifiableMap(commands);
   }
+
+  @Override
+  public void close() {
+    if (closeRefCache) {
+      refCache.close();
+    }
+  }
 }
diff --git a/javatests/com/google/gerrit/server/git/RepoRefCacheTest.java b/javatests/com/google/gerrit/server/git/RepoRefCacheTest.java
new file mode 100644
index 0000000000..1f422c1fe1
--- /dev/null
+++ b/javatests/com/google/gerrit/server/git/RepoRefCacheTest.java
@@ -0,0 +1,276 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.git;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import org.eclipse.jgit.attributes.AttributesNodeProvider;
+import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository.Builder;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.BatchRefUpdate;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectDatabase;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefDatabase;
+import org.eclipse.jgit.lib.RefRename;
+import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.lib.ReflogReader;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.lib.StoredConfig;
+import org.eclipse.jgit.util.FS;
+import org.junit.Test;
+
+public class RepoRefCacheTest {
+  private static final String TEST_BRANCH = "main";
+
+  @Test
+  @SuppressWarnings("resource")
+  public void repositoryUseShouldBeTrackedByRepoRefCache() throws Exception {
+    RefCache cache;
+    TestRepositoryWithRefCounting repoWithRefCounting;
+
+    try (TestRepositoryWithRefCounting repo =
+        TestRepositoryWithRefCounting.createWithBranch(TEST_BRANCH)) {
+      assertThat(repo.refCounter()).isEqualTo(1);
+      repoWithRefCounting = repo;
+      cache = new RepoRefCache(repo);
+    }
+
+    assertThat(repoWithRefCounting.refCounter()).isEqualTo(1);
+    assertThat(cache.get(Constants.R_HEADS + TEST_BRANCH)).isNotNull();
+  }
+
+  private static class TestRepositoryWithRefCounting extends Repository {
+    private int refCounter;
+
+    static TestRepositoryWithRefCounting createWithBranch(String branchName) throws Exception {
+      Builder builder =
+          new InMemoryRepository.Builder()
+              .setRepositoryDescription(new DfsRepositoryDescription(""))
+              .setFS(FS.detect().setUserHome(null));
+      TestRepositoryWithRefCounting testRepo = new TestRepositoryWithRefCounting(builder);
+      new TestRepository<>(testRepo).branch(branchName).commit().message("").create();
+      return testRepo;
+    }
+
+    private final Repository repo;
+
+    private TestRepositoryWithRefCounting(InMemoryRepository.Builder builder) throws IOException {
+      super(builder);
+
+      repo = builder.build();
+      refCounter = 1;
+    }
+
+    public int refCounter() {
+      return refCounter;
+    }
+
+    @Override
+    public void incrementOpen() {
+      repo.incrementOpen();
+      refCounter++;
+    }
+
+    @Override
+    public void close() {
+      repo.close();
+      refCounter--;
+    }
+
+    @Override
+    public void create(boolean bare) throws IOException {}
+
+    @Override
+    public ObjectDatabase getObjectDatabase() {
+      checkIsOpen();
+      return repo.getObjectDatabase();
+    }
+
+    @Override
+    public RefDatabase getRefDatabase() {
+      RefDatabase refDatabase = repo.getRefDatabase();
+      return new RefDatabase() {
+
+        @Override
+        public int hashCode() {
+          return refDatabase.hashCode();
+        }
+
+        @Override
+        public void create() throws IOException {
+          refDatabase.create();
+        }
+
+        @Override
+        public void close() {
+          checkIsOpen();
+          refDatabase.close();
+        }
+
+        @Override
+        public boolean isNameConflicting(String name) throws IOException {
+          checkIsOpen();
+          return refDatabase.isNameConflicting(name);
+        }
+
+        @Override
+        public boolean equals(Object obj) {
+          return refDatabase.equals(obj);
+        }
+
+        @Override
+        public Collection<String> getConflictingNames(String name) throws IOException {
+          checkIsOpen();
+          return refDatabase.getConflictingNames(name);
+        }
+
+        @Override
+        public RefUpdate newUpdate(String name, boolean detach) throws IOException {
+          checkIsOpen();
+          return refDatabase.newUpdate(name, detach);
+        }
+
+        @Override
+        public RefRename newRename(String fromName, String toName) throws IOException {
+          checkIsOpen();
+          return refDatabase.newRename(fromName, toName);
+        }
+
+        @Override
+        public BatchRefUpdate newBatchUpdate() {
+          checkIsOpen();
+          return refDatabase.newBatchUpdate();
+        }
+
+        @Override
+        public boolean performsAtomicTransactions() {
+          checkIsOpen();
+          return refDatabase.performsAtomicTransactions();
+        }
+
+        @Override
+        public Ref getRef(String name) throws IOException {
+          checkIsOpen();
+          return refDatabase.getRef(name);
+        }
+
+        @Override
+        public Ref exactRef(String name) throws IOException {
+          checkIsOpen();
+          return refDatabase.exactRef(name);
+        }
+
+        @Override
+        public String toString() {
+          return refDatabase.toString();
+        }
+
+        @Override
+        public Map<String, Ref> exactRef(String... refs) throws IOException {
+          checkIsOpen();
+          return refDatabase.exactRef(refs);
+        }
+
+        @Override
+        public Ref firstExactRef(String... refs) throws IOException {
+          checkIsOpen();
+          return refDatabase.firstExactRef(refs);
+        }
+
+        @Override
+        public List<Ref> getRefs() throws IOException {
+          checkIsOpen();
+          return refDatabase.getRefs();
+        }
+
+        @Override
+        public Map<String, Ref> getRefs(String prefix) throws IOException {
+          checkIsOpen();
+          return refDatabase.getRefs(prefix);
+        }
+
+        @Override
+        public List<Ref> getRefsByPrefix(String prefix) throws IOException {
+          checkIsOpen();
+          return refDatabase.getRefsByPrefix(prefix);
+        }
+
+        @Override
+        public boolean hasRefs() throws IOException {
+          checkIsOpen();
+          return refDatabase.hasRefs();
+        }
+
+        @Override
+        public List<Ref> getAdditionalRefs() throws IOException {
+          checkIsOpen();
+          return refDatabase.getAdditionalRefs();
+        }
+
+        @Override
+        public Ref peel(Ref ref) throws IOException {
+          checkIsOpen();
+          return refDatabase.peel(ref);
+        }
+
+        @Override
+        public void refresh() {
+          checkIsOpen();
+          refDatabase.refresh();
+        }
+      };
+    }
+
+    @Override
+    public StoredConfig getConfig() {
+      return repo.getConfig();
+    }
+
+    @Override
+    public AttributesNodeProvider createAttributesNodeProvider() {
+      checkIsOpen();
+      return repo.createAttributesNodeProvider();
+    }
+
+    @Override
+    public void scanForRepoChanges() throws IOException {
+      checkIsOpen();
+    }
+
+    @Override
+    public void notifyIndexChanged(boolean internal) {
+      checkIsOpen();
+    }
+
+    @Override
+    public ReflogReader getReflogReader(String refName) throws IOException {
+      checkIsOpen();
+      return repo.getReflogReader(refName);
+    }
+
+    private void checkIsOpen() {
+      if (refCounter <= 0) {
+        throw new IllegalStateException("Repository is not open (refCounter=" + refCounter + ")");
+      }
+    }
+  }
+}