Limit the number of changes that can be submitted together

When chaining changes together, the sequence of commits to navigate
was previously unbound, causing the potential explosion to millions
of changes.

The explosion could have also been accidental and caused by the push
of a change with a non-existent branch, which would have resulted
in the full scan of the repository for changes.

Introduce a new Gerrit configuration change.maxSubmittableAtOnce with
a safe default of 32767, which would allow any use case that would have
also worked before this change. Navigating over 32767 changes up to
potentially a huge number of commits would have generated a significant
CPU and memory overload and still not resulted in a submittable
chain of changes anyway.

Release-Notes: Limit the number of changes that can be submitted at once
Bug: Issue 16322
Change-Id: Id71aed2341f72708778395359bb6e4d4c270401c

3 files changed, 59 insertions, 6 deletions

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 9885e71e22..f5b42c6589 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -1213,6 +1213,14 @@ performance improvements by reducing the number of refs in the repo.
 +
 Default is true.
 
+[[change.maxSubmittableAtOnce]]change.maxSubmittableAtOnce::
++
+Maximum number of changes that can be chained together in the same repository
+to be submitted at once.
++
+Default is 32767.
+
+
 [[change.showAssigneeInChangesTable]]change.showAssigneeInChangesTable::
 +
 Show assignee field in changes table. If set to false, assignees will
diff --git a/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java b/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java
index 9efb976a65..60b0c5b01a 100644
--- a/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java
+++ b/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java
@@ -30,6 +30,7 @@ import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.permissions.ChangePermission;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -53,6 +54,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -65,6 +67,8 @@ import org.eclipse.jgit.revwalk.RevSort;
 public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
+  public static final int MAX_SUBMITTABLE_CHANGES_AT_ONCE_DEFAULT = 1024;
+
   public static class Module extends AbstractModule {
     @Override
     protected void configure() {
@@ -90,17 +94,22 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
   private final Map<QueryKey, List<ChangeData>> queryCache;
   private final Map<Branch.NameKey, Optional<RevCommit>> heads;
   private final ProjectCache projectCache;
+  private final int maxSubmittableChangesAtOnce;
 
   @Inject
   LocalMergeSuperSetComputation(
       PermissionBackend permissionBackend,
       Provider<InternalChangeQuery> queryProvider,
-      ProjectCache projectCache) {
+      ProjectCache projectCache,
+      @GerritServerConfig Config gerritConfig) {
     this.projectCache = projectCache;
     this.permissionBackend = permissionBackend;
     this.queryProvider = queryProvider;
     this.queryCache = new HashMap<>();
     this.heads = new HashMap<>();
+    this.maxSubmittableChangesAtOnce =
+        gerritConfig.getInt(
+            "change", "maxSubmittableAtOnce", MAX_SUBMITTABLE_CHANGES_AT_ONCE_DEFAULT);
   }
 
   @Override
@@ -146,10 +155,12 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
       }
 
       Set<String> visibleHashes =
-          walkChangesByHashes(visibleCommits, Collections.emptySet(), or, b);
+          walkChangesByHashes(
+              visibleCommits, Collections.emptySet(), or, b, maxSubmittableChangesAtOnce);
       Iterables.addAll(visibleChanges, byCommitsOnBranchNotMerged(or, db, b, visibleHashes));
 
-      Set<String> nonVisibleHashes = walkChangesByHashes(nonVisibleCommits, visibleHashes, or, b);
+      Set<String> nonVisibleHashes =
+          walkChangesByHashes(nonVisibleCommits, visibleHashes, or, b, maxSubmittableChangesAtOnce);
       Iterables.addAll(nonVisibleChanges, byCommitsOnBranchNotMerged(or, db, b, nonVisibleHashes));
     }
 
@@ -229,7 +240,11 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
   }
 
   private Set<String> walkChangesByHashes(
-      Collection<RevCommit> sourceCommits, Set<String> ignoreHashes, OpenRepo or, Branch.NameKey b)
+      Collection<RevCommit> sourceCommits,
+      Set<String> ignoreHashes,
+      OpenRepo or,
+      Branch.NameKey b,
+      int limit)
       throws IOException {
     Set<String> destHashes = new HashSet<>();
     or.rw.reset();
@@ -239,7 +254,11 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
       if (ignoreHashes.contains(name)) {
         continue;
       }
-      destHashes.add(name);
+      if (destHashes.size() < limit) {
+        destHashes.add(name);
+      } else {
+        break;
+      }
       or.rw.markStart(c);
     }
     for (RevCommit c : or.rw) {
@@ -247,7 +266,11 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
       if (ignoreHashes.contains(name)) {
         continue;
       }
-      destHashes.add(name);
+      if (destHashes.size() < limit) {
+        destHashes.add(name);
+      } else {
+        break;
+      }
     }
 
     return destHashes;
diff --git a/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java b/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java
index 304a1e48a4..0067e04cea 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java
@@ -19,7 +19,9 @@ import static com.google.gerrit.acceptance.GitUtil.pushHead;
 import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES;
 
 import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.GerritConfig;
 import com.google.gerrit.acceptance.GitUtil;
+import com.google.gerrit.acceptance.Sandboxed;
 import com.google.gerrit.acceptance.TestProjectInput;
 import com.google.gerrit.extensions.api.changes.SubmittedTogetherInfo;
 import com.google.gerrit.extensions.client.ChangeStatus;
@@ -178,6 +180,26 @@ public class SubmittedTogetherIT extends AbstractDaemonTest {
   }
 
   @Test
+  @Sandboxed
+  @GerritConfig(name = "change.maxSubmittableAtOnce", value = "2")
+  public void submittedTogetherWithMaxChangesLimit() throws Exception {
+    String targetRef = "refs/for/master";
+
+    commitBuilder().add("a.txt", "1").message("subject: 1").create();
+    pushHead(testRepo, targetRef, false);
+
+    RevCommit c2_1 = commitBuilder().add("b.txt", "2").message("subject: 2").create();
+    String id2 = getChangeId(c2_1);
+    pushHead(testRepo, targetRef, false);
+
+    RevCommit c3_1 = commitBuilder().add("b.txt", "3").message("subject: 3").create();
+    String id3 = getChangeId(c3_1);
+    pushHead(testRepo, targetRef, false);
+
+    assertSubmittedTogether(id3, id3, id2);
+  }
+
+  @Test
   public void respectTopicsOnAncestors() throws Exception {
     RevCommit initialHead = getRemoteHead();