Make sure to always compare the latest local ref
When validating the ref operations against the shared ref-db,
make sure that values are always compared to the latest ones on
the repository and not the ones read and cached at the very beginning
of the operation.
Pushes can be transparently retried multiple times when there are
race conditions, the value read at the very beginning could not be
actual anymore when the ref-update is actually executed.
Bug: Issue 11081
Change-Id: I7ac82f6f5437f60f3998d0ab3807434dcf95279a
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/BatchRefUpdateValidator.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/BatchRefUpdateValidator.java
index 59b5753..33be190 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/BatchRefUpdateValidator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/BatchRefUpdateValidator.java
@@ -21,6 +21,7 @@
import com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb.SharedRefEnforcement;
import com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb.SharedRefEnforcement.EnforcePolicy;
import java.io.IOException;
+import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;
@@ -92,7 +93,7 @@
}
try (CloseableSet<AutoCloseable> locks = new CloseableSet<>()) {
- checkIfLocalRefIsUpToDateWithSharedRefDb(refsToUpdate, locks);
+ refsToUpdate = compareAndGetLatestLocalRefs(refsToUpdate, locks);
delegateUpdate.invoke();
updateSharedRefDb(batchRefUpdate.getCommands().stream(), refsToUpdate);
}
@@ -143,10 +144,12 @@
return command.getNewId();
}
- private void checkIfLocalRefIsUpToDateWithSharedRefDb(
+ private List<RefPair> compareAndGetLatestLocalRefs(
List<RefPair> refsToUpdate, CloseableSet<AutoCloseable> locks) throws IOException {
+ List<RefPair> latestRefsToUpdate = new ArrayList<>();
for (RefPair refPair : refsToUpdate) {
- checkIfLocalRefIsUpToDateWithSharedRefDb(refPair, locks);
+ latestRefsToUpdate.add(compareAndGetLatestLocalRef(refPair, locks));
}
+ return latestRefsToUpdate;
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidator.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidator.java
index 18f8654..dec6ae4 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidator.java
@@ -14,6 +14,8 @@
package com.googlesource.gerrit.plugins.multisite.validation;
+import static com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb.SharedRefDatabase.nullRef;
+
import com.google.common.base.MoreObjects;
import com.google.common.flogger.FluentLogger;
import com.google.inject.Inject;
@@ -26,6 +28,7 @@
import com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb.SharedRefEnforcement.EnforcePolicy;
import java.io.IOException;
import java.util.HashMap;
+import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefDatabase;
import org.eclipse.jgit.lib.RefUpdate;
@@ -113,7 +116,7 @@
throws IOException {
try (CloseableSet<AutoCloseable> locks = new CloseableSet<>()) {
RefPair refPairForUpdate = newRefPairFrom(refUpdate);
- checkIfLocalRefIsUpToDateWithSharedRefDb(refPairForUpdate, locks);
+ compareAndGetLatestLocalRef(refPairForUpdate, locks);
RefUpdate.Result result = refUpdateFunction.invoke();
if (isSuccessful(result)) {
updateSharedDbOrThrowExceptionFor(refPairForUpdate);
@@ -146,32 +149,39 @@
}
}
- protected void checkIfLocalRefIsUpToDateWithSharedRefDb(
+ protected RefPair compareAndGetLatestLocalRef(
RefPair refPair, CloseableSet<AutoCloseable> locks)
throws SharedLockException, OutOfSyncException, IOException {
String refName = refPair.getName();
EnforcePolicy refEnforcementPolicy = refEnforcement.getPolicy(projectName, refName);
if (refEnforcementPolicy == EnforcePolicy.IGNORED) {
- return;
+ return refPair;
}
- Ref localRef = refPair.compareRef;
-
locks.addResourceIfNotExist(
String.format("%s-%s", projectName, refName),
() -> sharedRefDb.lockRef(projectName, refName));
+ RefPair latestRefPair = getLatestLocalRef(refPair);
boolean isInSync =
- (localRef != null)
- ? sharedRefDb.isUpToDate(projectName, localRef)
- : !sharedRefDb.exists(projectName, refName);
+ (latestRefPair.compareRef.getObjectId().equals(ObjectId.zeroId()))
+ ? !sharedRefDb.exists(projectName, refName)
+ : sharedRefDb.isUpToDate(projectName, latestRefPair.compareRef);
if (!isInSync) {
validationMetrics.incrementSplitBrainPrevention();
softFailBasedOnEnforcement(
- new OutOfSyncException(projectName, localRef), refEnforcementPolicy);
+ new OutOfSyncException(projectName, latestRefPair.compareRef), refEnforcementPolicy);
}
+
+ return latestRefPair;
+ }
+
+ private RefPair getLatestLocalRef(RefPair refPair) throws IOException {
+ Ref latestRef = refDb.exactRef(refPair.getName());
+ return new RefPair(
+ latestRef == null ? nullRef(refPair.getName()) : latestRef, refPair.putValue);
}
protected boolean isSuccessful(RefUpdate.Result result) {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/MultiSiteBatchRefUpdateTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/MultiSiteBatchRefUpdateTest.java
index 7e0dc6e..9b8b562 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/MultiSiteBatchRefUpdateTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/MultiSiteBatchRefUpdateTest.java
@@ -98,6 +98,7 @@
.thenReturn(asList(successReceiveCommandAfterExecution));
doReturn(oldRef).when(refDatabase).getRef(A_TEST_REF_NAME);
+ doReturn(oldRef).when(refDatabase).exactRef(A_TEST_REF_NAME);
multiSiteRefUpdate = getMultiSiteBatchRefUpdateWithDefaultPolicyEnforcement();
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidatorTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidatorTest.java
index e7624bd..d95860c 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidatorTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidatorTest.java
@@ -67,6 +67,7 @@
localRef = newRef(refName, AN_OBJECT_ID_3);
doReturn(localRef).when(localRefDb).getRef(refName);
+ doReturn(localRef).when(localRefDb).exactRef(refName);
doReturn(oldUpdateRef).when(refUpdate).getRef();
doReturn(newUpdateRef.getObjectId()).when(refUpdate).getNewObjectId();
doReturn(refName).when(refUpdate).getName();
