Merge branch 'stable-3.5' into stable-3.6 * stable-3.5: Do not notify failures on collisions Fixed erroneous increment of the rescheduled metric Ensure failures are only incremented in RunnableWithMetrics Record replication metrics for retrying tasks Report deletions as succeeded in the fetch-ref-replicated events NOOP fetches are to be considered succeded Properly propagate failure when ref deletion fails Accept remotes without `fetch` option on primary Pull-replication plugin should warn about inconsistent timeouts Remove events from beforeStartupEventsQueue after fire() Fix ordering of parameters for apply objects log Respect `remote.mirror` option in DeleteRefCommand Remove events from beforeStartupEventsQueue in-memory queue Reverse the order of storing commit, tree and blobs Do not honor gerrit.replicateOnStartup on primary side Do not record fetch replication metrics when all refs are excluded from fetch task Don't require remote.<name>.url on primary side Use GJF 1.7 for code-style verification Change-Id: I0cb49e9761e5783a024899db00078ca75338e77a

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchOne.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchOne.java
index 560f8ee..df39d5a 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchOne.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchOne.java

@@ -88,6 +88,7 @@
   private final Set<TransportException> fetchFailures = Sets.newHashSetWithExpectedSize(4);
   private boolean fetchAllRefs;
   private Repository git;
+  private boolean isCollision;
   private boolean retrying;
   private int retryCount;
   private final int maxRetries;
@@ -259,7 +260,7 @@
   }
 
   private void statesCleanUp() {
-    if (!stateMap.isEmpty() && !isRetrying()) {
+    if (!stateMap.isEmpty() && !isRetrying() && !isCollision) {
       for (Map.Entry<String, ReplicationState> entry : stateMap.entries()) {
         entry
             .getValue()
@@ -315,6 +316,7 @@
     // we start replication (instead a new instance, with the same URI, is
     // created and scheduled for a future point in time.)
     //
+    isCollision = false;
     if (replicationType == ReplicationType.ASYNC && !pool.requestRunway(this)) {
       if (!canceled) {
         repLog.info(
@@ -323,6 +325,7 @@
             uri,
             pool.getInFlight(getURI()).map(FetchOne::getTaskIdHex).orElse("<unknown>"));
         pool.reschedule(this, Source.RetryReason.COLLISION);
+        isCollision = true;
       }
       return;
     }
@@ -356,8 +359,9 @@
                 .flatMap(metrics -> metrics.stop(config.getName()))
                 .map(NANOSECONDS::toMillis);
         repLog.info(
-            "[{}] Replication from {} completed in {}ms, {}ms delay, {} retries{}",
+            "[{}] {} replication from {} completed in {}ms, {}ms delay, {} retries{}",
             taskIdHex,
+            replicationType,
             uri,
             elapsed,
             delay,
@@ -501,6 +505,11 @@
     boolean anyRefFailed = false;
     RefUpdate.Result lastRefUpdateResult = RefUpdate.Result.NO_CHANGE;
 
+    // NOOP fetches are considered as successes
+    if (refUpdates.isEmpty()) {
+      succeeded = true;
+    }
+
     for (RefUpdateState u : refUpdates) {
       ReplicationState.RefFetchResult fetchStatus = ReplicationState.RefFetchResult.SUCCEEDED;
       Set<ReplicationState> logStates = new HashSet<>();

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchRefReplicatedEvent.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchRefReplicatedEvent.java
index 0eabf42..2c18983 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchRefReplicatedEvent.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchRefReplicatedEvent.java

@@ -14,6 +14,7 @@
 
 package com.googlesource.gerrit.plugins.replication.pull;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.gerrit.entities.Project;
 import com.googlesource.gerrit.plugins.replication.events.RemoteRefReplicationEvent;
 import java.util.Objects;
@@ -74,4 +75,9 @@
   public String getRefName() {
     return ref;
   }
+
+  @VisibleForTesting
+  public RefUpdate.Result getRefUpdateResult() {
+    return refUpdateResult;
+  }
 }

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java
index ef95596..ef1c2fd 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java

@@ -547,13 +547,14 @@
   private void fireBeforeStartupEvents() {
     Set<String> eventsReplayed = new HashSet<>();
     ReferenceUpdatedEvent event;
-    while ((event = beforeStartupEventsQueue.poll()) != null) {
+    while ((event = beforeStartupEventsQueue.peek()) != null) {
       String eventKey = String.format("%s:%s", event.projectName(), event.refName());
       if (!eventsReplayed.contains(eventKey)) {
         repLog.info("Firing pending task {}", event);
         fire(event);
         eventsReplayed.add(eventKey);
       }
+      beforeStartupEventsQueue.remove(event);
     }
   }
 

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/Source.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/Source.java
index 012a046..cc33ce9 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/Source.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/Source.java

@@ -628,8 +628,6 @@
                   fetchOp.getTaskIdHex(), fetchOp.getURI(), pendingFetchOp.getTaskIdHex()),
               fetchOp.getStatesAsArray());
 
-          queueMetrics.incrementTaskRescheduled(this);
-
         } else {
           // The one pending is one that is NOT retrying, it was just
           // scheduled believing no problem would happen. The one pending
@@ -654,9 +652,8 @@
                   "[%s] Merging the pending fetch from [%s] with task [%s] and rescheduling",
                   pendingFetchOp.getTaskIdHex(), pendingFetchOp.getURI(), fetchOp.getTaskIdHex()),
               pendingFetchOp.getStatesAsArray());
-
-          queueMetrics.incrementTaskMerged(this);
         }
+        queueMetrics.incrementTaskMerged(this);
       }
 
       if (pendingFetchOp == null || !pendingFetchOp.isRetrying()) {
@@ -677,11 +674,13 @@
                     ? RefUpdate.Result.NOT_ATTEMPTED
                     : RefUpdate.Result.REJECTED_OTHER_REASON;
             postReplicationFailedEvent(fetchOp, trackingRefUpdate);
-            queueMetrics.incrementTaskFailed(this);
 
             if (fetchOp.setToRetry()) {
               postReplicationScheduledEvent(fetchOp);
-              pool.schedule(fetchOp, config.getRetryDelay(), TimeUnit.MINUTES);
+              pool.schedule(
+                  queueMetrics.runWithMetrics(this, fetchOp),
+                  config.getRetryDelay(),
+                  TimeUnit.MINUTES);
               queueMetrics.incrementTaskRetrying(this);
             } else {
               fetchOp.canceledByReplication();
@@ -906,6 +905,10 @@
     return config.replicateProjectDeletions();
   }
 
+  public boolean isMirror() {
+    return config.getRemoteConfig().isMirror();
+  }
+
   void scheduleUpdateHead(String apiUrl, Project.NameKey project, String newHead) {
     try {
       URIish apiURI = new URIish(apiUrl);

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/SourceConfigParser.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/SourceConfigParser.java
index d7ae063..67ac6f1 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/SourceConfigParser.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/SourceConfigParser.java

@@ -79,14 +79,14 @@
     return sourceConfigs.build();
   }
 
-  private static List<RemoteConfig> allFetchRemotes(Config cfg) throws ConfigInvalidException {
+  private List<RemoteConfig> allFetchRemotes(Config cfg) throws ConfigInvalidException {
 
     Set<String> names = cfg.getSubsections("remote");
     List<RemoteConfig> result = Lists.newArrayListWithCapacity(names.size());
     for (String name : names) {
       try {
         final RemoteConfig remoteConfig = new RemoteConfig(cfg, name);
-        if (!remoteConfig.getFetchRefSpecs().isEmpty()) {
+        if (!isReplica || !remoteConfig.getFetchRefSpecs().isEmpty()) {
           result.add(remoteConfig);
         } else {
           logger.atFine().log(

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/SourceConfiguration.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/SourceConfiguration.java
index 0e840bd..b6647bd 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/SourceConfiguration.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/SourceConfiguration.java

@@ -16,6 +16,7 @@
 
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableList;
+import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.server.config.ConfigUtil;
 import com.googlesource.gerrit.plugins.replication.RemoteConfiguration;
 import java.util.concurrent.TimeUnit;
@@ -23,6 +24,7 @@
 import org.eclipse.jgit.transport.RemoteConfig;
 
 public class SourceConfiguration implements RemoteConfiguration {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
   static final int DEFAULT_REPLICATION_DELAY = 4;
   static final int DEFAULT_RESCHEDULE_DELAY = 3;
   static final int DEFAULT_SLOW_LATENCY_THRESHOLD_SECS = 900;
@@ -64,6 +66,13 @@
     apis = ImmutableList.copyOf(cfg.getStringList("remote", name, "apiUrl"));
     connectionTimeout =
         cfg.getInt("remote", name, "connectionTimeout", DEFAULT_CONNECTION_TIMEOUT_MS);
+    int connectionTimeoutInSec = connectionTimeout / 1000;
+    if (connectionTimeoutInSec < getRemoteConfig().getTimeout()) {
+      logger.atWarning().log(
+          "The connection timeout is currently set to %s sec, which is less than the timeout value of %s sec. "
+              + "To avoid potential issues, consider increasing the connection timeout to exceed the timeout value.",
+          connectionTimeoutInSec, getRemoteConfig().getTimeout());
+    }
     idleTimeout = cfg.getInt("remote", name, "idleTimeout", DEFAULT_MAX_CONNECTION_INACTIVITY_MS);
     maxConnectionsPerRoute =
         cfg.getInt("replication", "maxConnectionsPerRoute", DEFAULT_CONNECTIONS_PER_ROUTE);

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommand.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommand.java
index e49c8b6..5192351 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommand.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommand.java

@@ -35,6 +35,7 @@
 import com.googlesource.gerrit.plugins.replication.pull.ReplicationState;
 import com.googlesource.gerrit.plugins.replication.pull.Source;
 import com.googlesource.gerrit.plugins.replication.pull.SourcesCollection;
+import com.googlesource.gerrit.plugins.replication.pull.api.exception.DeleteRefException;
 import com.googlesource.gerrit.plugins.replication.pull.fetch.RefUpdateState;
 import java.io.IOException;
 import java.util.Optional;
@@ -72,6 +73,18 @@
 
   public void deleteRef(Project.NameKey name, String refName, String sourceLabel)
       throws IOException, RestApiException {
+    Source source =
+        sourcesCollection
+            .getByRemoteName(sourceLabel)
+            .orElseThrow(
+                () ->
+                    new IllegalStateException(
+                        String.format("Could not find URI for %s remote", sourceLabel)));
+    if (!source.isMirror()) {
+      repLog.info(
+          "Ignoring ref {} deletion from project {}, as mirror option is false", refName, name);
+      return;
+    }
     try {
       repLog.info("Delete ref from {} for project {}, ref name {}", sourceLabel, name, refName);
       Optional<ProjectState> projectState = projectCache.get(name);
@@ -85,19 +98,12 @@
         return;
       }
 
-      Source source =
-          sourcesCollection
-              .getByRemoteName(sourceLabel)
-              .orElseThrow(
-                  () ->
-                      new IllegalStateException(
-                          String.format("Could not find URI for %s remote", sourceLabel)));
       URIish sourceUri = source.getURI(name);
 
       try {
 
         Context.setLocalEvent(true);
-        deleteRef(name, ref.get());
+        RefUpdate.Result successResult = ensureSuccess(deleteRef(name, ref.get()));
 
         eventDispatcher
             .get()
@@ -107,13 +113,17 @@
                     refName,
                     sourceUri,
                     ReplicationState.RefFetchResult.SUCCEEDED,
-                    RefUpdate.Result.FORCED));
+                    successResult));
       } catch (PermissionBackendException e) {
         logger.atSevere().withCause(e).log(
             "Unexpected error while trying to delete ref '%s' on project %s and notifying it",
             refName, name);
         throw RestApiException.wrap(e.getMessage(), e);
       } catch (IOException e) {
+        RefUpdate.Result refUpdateResult =
+            e instanceof DeleteRefException
+                ? ((DeleteRefException) e).getResult()
+                : RefUpdate.Result.LOCK_FAILURE;
         eventDispatcher
             .get()
             .postEvent(
@@ -122,7 +132,7 @@
                     refName,
                     sourceUri,
                     ReplicationState.RefFetchResult.FAILED,
-                    RefUpdate.Result.LOCK_FAILURE));
+                    refUpdateResult));
         String message =
             String.format(
                 "RefUpdate lock failure for: sourceLabel=%s, project=%s, refName=%s",
@@ -158,7 +168,22 @@
       u.setForceUpdate(true);
 
       result = u.delete();
-      return new RefUpdateState(ref.getName(), result);
+      return new RefUpdateState(":" + ref.getName(), result);
     }
   }
+
+  private static RefUpdate.Result ensureSuccess(RefUpdateState refUpdateState)
+      throws DeleteRefException {
+    switch (refUpdateState.getResult()) {
+      case NOT_ATTEMPTED:
+      case REJECTED:
+      case REJECTED_CURRENT_BRANCH:
+      case REJECTED_MISSING_OBJECT:
+      case LOCK_FAILURE:
+      case IO_FAILURE:
+      case REJECTED_OTHER_REASON:
+        throw new DeleteRefException("Failed ref deletion", refUpdateState.getResult());
+    }
+    return refUpdateState.getResult();
+  }
 }

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/exception/DeleteRefException.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/exception/DeleteRefException.java
new file mode 100644
index 0000000..a1d6876
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/exception/DeleteRefException.java

@@ -0,0 +1,33 @@
+// Copyright (C) 2024 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.googlesource.gerrit.plugins.replication.pull.api.exception;
+
+import java.io.IOException;
+import org.eclipse.jgit.lib.RefUpdate;
+
+public class DeleteRefException extends IOException {
+
+  private static final long serialVersionUID = 1L;
+  private final RefUpdate.Result result;
+
+  public DeleteRefException(String msg, RefUpdate.Result result) {
+    super(msg);
+    this.result = result;
+  }
+
+  public RefUpdate.Result getResult() {
+    return result;
+  }
+}

diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md
index 61d62b4..bffca58 100644
--- a/src/main/resources/Documentation/config.md
+++ b/src/main/resources/Documentation/config.md

@@ -439,6 +439,10 @@
 	when `replicatePermissions` is true, even if the push refspec
 	is 'all refs'.
 
+>  NOTE: When using the pull-replication and replication plugins together,
+>  **NOT having** a `fetch` configuration for a remote, will also enable
+>  the standard _push_ replication for that remote.
+
 [2]: #example_file
 
 remote.NAME.timeout

diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommandTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommandTest.java
index fc1b02c..e238528 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommandTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommandTest.java

@@ -15,10 +15,12 @@
 package com.googlesource.gerrit.plugins.replication.pull.api;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.anyString;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;
 
 import com.google.gerrit.entities.Project;
@@ -36,9 +38,11 @@
 import com.googlesource.gerrit.plugins.replication.pull.FetchRefReplicatedEvent;
 import com.googlesource.gerrit.plugins.replication.pull.LocalGitRepositoryManagerProvider;
 import com.googlesource.gerrit.plugins.replication.pull.PullReplicationStateLogger;
+import com.googlesource.gerrit.plugins.replication.pull.ReplicationState;
 import com.googlesource.gerrit.plugins.replication.pull.Source;
 import com.googlesource.gerrit.plugins.replication.pull.SourcesCollection;
 import com.googlesource.gerrit.plugins.replication.pull.fetch.ApplyObject;
+import java.io.IOException;
 import java.util.Optional;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefDatabase;
@@ -109,22 +113,53 @@
 
   @Test
   public void shouldSendEventWhenDeletingRef() throws Exception {
+    when(source.isMirror()).thenReturn(true);
+
     objectUnderTest.deleteRef(TEST_PROJECT_NAME, TEST_REF_NAME, TEST_SOURCE_LABEL);
 
+    assertFetchReplicatedEvent(ReplicationState.RefFetchResult.SUCCEEDED, Result.FORCED);
+  }
+
+  @Test
+  public void shouldNotSendNotSendEventWhenMirroringIsDisabled() throws Exception {
+    when(source.isMirror()).thenReturn(false);
+
+    objectUnderTest.deleteRef(TEST_PROJECT_NAME, TEST_REF_NAME, TEST_SOURCE_LABEL);
+
+    verifyNoInteractions(eventDispatcher);
+  }
+
+  @Test
+  public void shouldHandleNonExistingRef() throws Exception {
+    when(source.isMirror()).thenReturn(true);
+    when(refDb.exactRef(anyString())).thenReturn(null);
+
+    objectUnderTest.deleteRef(TEST_PROJECT_NAME, NON_EXISTING_REF_NAME, TEST_SOURCE_LABEL);
+
+    verify(eventDispatcher, never()).postEvent(any());
+  }
+
+  @Test
+  public void shouldThrowWhenRefDeletionFails() throws Exception {
+    when(source.isMirror()).thenReturn(true);
+    when(refUpdate.delete()).thenReturn(Result.LOCK_FAILURE);
+
+    assertThrows(
+        IOException.class,
+        () -> objectUnderTest.deleteRef(TEST_PROJECT_NAME, TEST_REF_NAME, TEST_SOURCE_LABEL));
+
+    assertFetchReplicatedEvent(ReplicationState.RefFetchResult.FAILED, Result.LOCK_FAILURE);
+  }
+
+  private void assertFetchReplicatedEvent(
+      ReplicationState.RefFetchResult refFetchResult, RefUpdate.Result result) throws Exception {
     verify(eventDispatcher).postEvent(eventCaptor.capture());
     Event sentEvent = eventCaptor.getValue();
     assertThat(sentEvent).isInstanceOf(FetchRefReplicatedEvent.class);
     FetchRefReplicatedEvent fetchEvent = (FetchRefReplicatedEvent) sentEvent;
     assertThat(fetchEvent.getProjectNameKey()).isEqualTo(TEST_PROJECT_NAME);
     assertThat(fetchEvent.getRefName()).isEqualTo(TEST_REF_NAME);
-  }
-
-  @Test
-  public void shouldHandleNonExistingRef() throws Exception {
-    when(refDb.exactRef(anyString())).thenReturn(null);
-
-    objectUnderTest.deleteRef(TEST_PROJECT_NAME, NON_EXISTING_REF_NAME, TEST_SOURCE_LABEL);
-
-    verify(eventDispatcher, never()).postEvent(any());
+    assertThat(fetchEvent.getStatus()).isEqualTo(refFetchResult.toString());
+    assertThat(fetchEvent.getRefUpdateResult()).isEqualTo(result);
   }
 }