Evict ref from the ref cache even when ref update fails If for some reason there is a miss-match between a persisted ref value and the cached value of that ref, it is likely to result in a ref-update failure. There are many places in Gerrit where code is retried on ref-update failures with the hopes that the failure can be recovered from. To make this recovery possible, it is essential to ensure that the outdated cached ref value no longer be used, and evicting the cached value even on failures makes this possible. Some reasons where cached ref values could be outdated are: 3rd party actors modifying the repos outside of Gerrit, multi-primary setups. For example, consider a multi-primary Gerrit setup with 2 primaries pointing to the repositories on NFS. When one primary receives a review command and at the same time another primary updates the same change with a new patchset and succeeds, then the review command would fail with Lock failure. In this case, review command would retry the operation, but it fails again because the cached ref value is still outdated. In order for a retry operation to succeed, we need to invalidate the ref from the cache when ref update fails. Change-Id: I724d5ba3d756e6a22a0b8c64be7ebd4f82d69d14
diff --git a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/BatchRefUpdateWithCacheUpdate.java b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/BatchRefUpdateWithCacheUpdate.java index c3b8537..cf1340f 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/BatchRefUpdateWithCacheUpdate.java +++ b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/BatchRefUpdateWithCacheUpdate.java
@@ -160,14 +160,20 @@ @Override public void execute(RevWalk walk, ProgressMonitor monitor, List<String> options) throws IOException { - delegate.execute(walk, monitor, options); - evictCache(); + try { + delegate.execute(walk, monitor, options); + } finally { + evictCache(); + } } @Override public void execute(RevWalk walk, ProgressMonitor monitor) throws IOException { - delegate.execute(walk, monitor); - evictCache(); + try { + delegate.execute(walk, monitor); + } finally { + evictCache(); + } } private void evictCache() { @@ -175,9 +181,7 @@ .getCommands() .forEach( cmd -> { - if (cmd.getResult() == ReceiveCommand.Result.OK) { - refsCache.evict(repo.getProjectName(), cmd.getRefName()); - } + refsCache.evict(repo.getProjectName(), cmd.getRefName()); }); } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefRenameWithCacheUpdate.java b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefRenameWithCacheUpdate.java index f6debce..ed99ff3 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefRenameWithCacheUpdate.java +++ b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefRenameWithCacheUpdate.java
@@ -17,7 +17,6 @@ import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; import java.io.IOException; -import java.util.EnumSet; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.RefRename; import org.eclipse.jgit.lib.RefUpdate; @@ -33,8 +32,6 @@ } private static final String NOT_SUPPORTED_MSG = "Should never be called"; - private static final EnumSet<Result> SUCCESSFUL_RENAMES = - EnumSet.of(Result.NEW, Result.FORCED, Result.FAST_FORWARD, Result.RENAMED); private final RefByNameCacheWrapper refsCache; private final CachedRefRepository repo; @@ -92,11 +89,11 @@ @Override public Result rename() throws IOException { - Result r = delegate.rename(); - if (SUCCESSFUL_RENAMES.contains(r)) { + try { + return delegate.rename(); + } finally { refsCache.evict(repo.getProjectName(), src.getName()); } - return r; } @Override
diff --git a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefUpdateWithCacheUpdate.java b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefUpdateWithCacheUpdate.java index 76579ce..99e3dc7 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefUpdateWithCacheUpdate.java +++ b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefUpdateWithCacheUpdate.java
@@ -3,7 +3,6 @@ import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; import java.io.IOException; -import java.util.EnumSet; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; @@ -21,8 +20,6 @@ } private static final String NOT_SUPPORTED_MSG = "Should never be called"; - private static final EnumSet<Result> SUCCESSFUL_UPDATES = - EnumSet.of(Result.NEW, Result.FORCED, Result.FAST_FORWARD, Result.RENAMED); private final RefByNameCacheWrapper refsCache; private final RefDatabase refDb; @@ -139,32 +136,56 @@ @Override public Result forceUpdate() throws IOException { - return evictCache(delegate.forceUpdate()); + try { + return delegate.forceUpdate(); + } finally { + evictCache(); + } } @Override public Result update() throws IOException { - return evictCache(delegate.update()); + try { + return delegate.update(); + } finally { + evictCache(); + } } @Override public Result update(RevWalk walk) throws IOException { - return evictCache(delegate.update(walk)); + try { + return delegate.update(walk); + } finally { + evictCache(); + } } @Override public Result delete() throws IOException { - return evictCache(delegate.delete()); + try { + return delegate.delete(); + } finally { + evictCache(); + } } @Override public Result delete(RevWalk walk) throws IOException { - return evictCache(delegate.delete(walk)); + try { + return delegate.delete(walk); + } finally { + evictCache(); + } } @Override public Result link(String target) throws IOException { - return evictCache(delegate.link(target)); + try { + return delegate.link(target); + } finally { + evictCache(); + } } @Override @@ -207,10 +228,7 @@ throw new UnsupportedOperationException(NOT_SUPPORTED_MSG); } - private Result evictCache(Result r) { - if (SUCCESSFUL_UPDATES.contains(r)) { - refsCache.evict(repo.getProjectName(), getName()); - } - return r; + private void evictCache() { + refsCache.evict(repo.getProjectName(), getName()); } }