Properly propagate failure when ref deletion fails
Refs deletions are implemented via the `DeleteCommand.deleteRef()`
method which attempts to delete refs locally after checking for
permissions and relevant configuration settings (i.e. `isMirror` flag).
Exceptions thrown during the deletion are then propagated to the caller,
which is correct, in order for the caller to take appropriate actions.
The problem however, is that not every ref deletion failure causes an
exception: failures are also represented as a result status of the
delete operation.
Take for example the existence of a ref lock on the ref being deleted:
this will cause the delete to return `LOCK_FAILURE`, rather than
throwing an exception.
However, because failure results are completely ignored, every
ref-update deletion that is not causing an exception is erroneously
treated as successful.
This behaviour impacts, as an example, the
`/pull-replication~batch-apply-object` and the
`/pull-replication-apply-object` HTTP endpoints, which invoke
`DeleteCommand.deleteRef()` when handling the deletion of a ref.
In case of an exception the endpoints will return the relevant HTTP
status code, informing the client of the failure, in all other cases
(even upon failing result statuses), the endpoint will erroneously
report success, misleading the client into believing that the deletion
was successful.
Stop ignoring the result statuses of ref-update deletions so that
callers can be made aware on the actual result of the failure and take
appropriate action.
Bug: Issue 288965464
Change-Id: Ib49247a1bb167c0b265151a1b1f5a6be15006012
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/api/DeleteRefCommand.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommand.java
index 3150fe1..3a7d0ff 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
@@ -34,6 +34,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;
@@ -98,7 +99,7 @@
try {
Context.setLocalEvent(true);
- deleteRef(name, ref.get());
+ RefUpdate.Result successResult = ensureSuccess(deleteRef(name, ref.get()));
eventDispatcher
.get()
@@ -108,13 +109,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(
@@ -123,7 +128,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",
@@ -162,4 +167,19 @@
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/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 40912ac..5ede40b 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,6 +15,7 @@
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;
@@ -36,9 +37,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.RefUpdate;
@@ -108,12 +111,7 @@
objectUnderTest.deleteRef(TEST_PROJECT_NAME, TEST_REF_NAME, TEST_SOURCE_LABEL);
- 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);
+ assertFetchReplicatedEvent(ReplicationState.RefFetchResult.SUCCEEDED, Result.FORCED);
}
@Test
@@ -133,4 +131,28 @@
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);
+ assertThat(fetchEvent.getStatus()).isEqualTo(refFetchResult.toString());
+ assertThat(fetchEvent.getRefUpdateResult()).isEqualTo(result);
+ }
}
