Limit retrying of submitted changes to 12 hours

Recently we discovered a change that has been retrying submit every
5 minutes since November, 2012. The change is unsubmittable due to
an unresolved dependency.

isSubmitStillPossible() is potentially broken in this case and may
be allowing changes to wait forever rather than DEPENDENCY_DELAY (15
minutes). Although the project should also fix whatever underlying
cause exists here, changes should never be allowed to automatically
retry submit for more than 6 months with no human intervention.

Be more conservative by retrying a change for 12 hours. If an
identical failure message is going to be posted, abort the submit
by setting the change status back to NEW. This requires a human to
revisit the change.

Adjust where the secondary index is invoked to ensure it gets
updated after the change was modified by MergeOp.

Conflicts:
	gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java

Change-Id: I4b2b73c2edbf61459c398e4fcc5cad7d504ba8b0
(cherry picked from commit 06f92f721c7b40037172f661592bab89f6df9671)
Reviewed-by: Ismo Haataja <ismo.haataja@digia.com>
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@theqtcompany.com>

1 files changed, 57 insertions, 40 deletions

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
index 785fbacfa5..f704434a54 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
@@ -15,7 +15,7 @@
 package com.google.gerrit.server.git;
 
 import static com.google.gerrit.server.git.MergeUtil.getSubmitter;
-import static java.util.concurrent.TimeUnit.DAYS;
+import static java.util.concurrent.TimeUnit.HOURS;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.TimeUnit.MINUTES;
 import static java.util.concurrent.TimeUnit.SECONDS;
@@ -55,7 +55,6 @@ import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
 import com.google.gerrit.server.util.RequestScopePropagator;
 import com.google.gwtorm.server.AtomicUpdate;
-import com.google.gwtorm.server.OrmConcurrencyException;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Inject;
@@ -89,6 +88,8 @@ import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
 
+import javax.annotation.Nullable;
+
 /**
  * Merges changes in submission order into a single branch.
  * <p>
@@ -117,8 +118,8 @@ public class MergeOp {
   private static final long LOCK_FAILURE_RETRY_DELAY =
       MILLISECONDS.convert(15, SECONDS);
 
-  private static final long DUPLICATE_MESSAGE_INTERVAL =
-      MILLISECONDS.convert(1, DAYS);
+  private static final long MAX_SUBMIT_WINDOW =
+      MILLISECONDS.convert(12, HOURS);
 
   private final GitRepositoryManager repoManager;
   private final SchemaFactory<ReviewDb> schemaFactory;
@@ -1025,68 +1026,84 @@ public class MergeOp {
     sendMergeFail(c, msg, true);
   }
 
-  private boolean isDuplicate(ChangeMessage msg) {
+  private enum RetryStatus {
+    UNSUBMIT, RETRY_NO_MESSAGE, RETRY_ADD_MESSAGE;
+  }
+
+  private RetryStatus getRetryStatus(
+      @Nullable PatchSetApproval submitter,
+      ChangeMessage msg) {
+    if (submitter != null
+        && System.currentTimeMillis() - submitter.getGranted().getTime()
+          > MAX_SUBMIT_WINDOW) {
+      return RetryStatus.UNSUBMIT;
+    }
+
     try {
       ChangeMessage last = Iterables.getLast(db.changeMessages().byChange(
           msg.getPatchSetId().getParentKey()), null);
       if (last != null) {
-        long lastMs = last.getWrittenOn().getTime();
-        long msgMs = msg.getWrittenOn().getTime();
         if (Objects.equal(last.getAuthor(), msg.getAuthor())
-            && Objects.equal(last.getMessage(), msg.getMessage())
-            && msgMs - lastMs < DUPLICATE_MESSAGE_INTERVAL) {
-          return true;
+            && Objects.equal(last.getMessage(), msg.getMessage())) {
+          long lastMs = last.getWrittenOn().getTime();
+          long msgMs = msg.getWrittenOn().getTime();
+          return msgMs - lastMs > MAX_SUBMIT_WINDOW
+              ? RetryStatus.UNSUBMIT
+              : RetryStatus.RETRY_NO_MESSAGE;
         }
       }
+      return RetryStatus.RETRY_ADD_MESSAGE;
     } catch (OrmException err) {
-      log.warn("Cannot check previous merge failure message", err);
+      log.warn("Cannot check previous merge failure, unsubmitting", err);
+      return RetryStatus.UNSUBMIT;
     }
-    return false;
   }
 
   private void sendMergeFail(final Change c, final ChangeMessage msg,
-      final boolean makeNew) {
-    if (makeNew) {
+      boolean makeNew) {
+    PatchSetApproval submitter = null;
+    try {
+      submitter = getSubmitter(db, c.currentPatchSetId());
+    } catch (Exception e) {
+      log.error("Cannot get submitter", e);
+    }
+
+    if (!makeNew) {
+      RetryStatus retryStatus = getRetryStatus(submitter, msg);
+      if (retryStatus == RetryStatus.RETRY_NO_MESSAGE) {
+        return;
+      } else if (retryStatus == RetryStatus.UNSUBMIT) {
+        makeNew = true;
+      }
+    }
+
+    final boolean setStatusNew = makeNew;
+    try {
+      db.changes().beginTransaction(c.getId());
       try {
-        db.changes().atomicUpdate(c.getId(), new AtomicUpdate<Change>() {
+        Change change = db.changes().atomicUpdate(
+            c.getId(),
+            new AtomicUpdate<Change>() {
           @Override
           public Change update(Change c) {
             if (c.getStatus().isOpen()) {
-              c.setStatus(Change.Status.NEW);
+              if (setStatusNew) {
+                c.setStatus(Change.Status.NEW);
+              }
               ChangeUtil.updated(c);
             }
             return c;
           }
         });
-      } catch (OrmConcurrencyException err) {
-      } catch (OrmException err) {
-        log.warn("Cannot update change status", err);
-      }
-    } else {
-      try {
-        ChangeUtil.touch(c, db);
-      } catch (OrmException err) {
-        log.warn("Cannot update change timestamp", err);
+        db.changeMessages().insert(Collections.singleton(msg));
+        db.commit();
+      } finally {
+        db.rollback();
       }
-    }
-
-    if (isDuplicate(msg)) {
-      return;
-    }
-
-    try {
-      db.changeMessages().insert(Collections.singleton(msg));
     } catch (OrmException err) {
       log.warn("Cannot record merge failure message", err);
     }
 
-    PatchSetApproval submitter = null;
-    try {
-      submitter = getSubmitter(db, c.currentPatchSetId());
-    } catch (Exception e) {
-      log.error("Cannot get submitter", e);
-    }
-
     final PatchSetApproval from = submitter;
     workQueue.getDefaultQueue()
         .submit(requestScopePropagator.wrap(new Runnable() {