Don't auto-rebuild ChangeNotes from within BatchUpdate

BatchUpdate.ChangeTask#newChangeContext constructs a new ChangeNotes
from within an open transaction. If auto-rebuilding is enabled in this
path, then ChangeRebuilderImpl will call ChangeBundle#fromReviewDb,
which attempts to open another transaction. This is a somewhat
unintended use of gwtorm transactions in order to read a consistent
snapshot of the bundle. Transactions are not reentrant, so depending
on the backend this can cause a variety of failures, from failing fast
while trying to open the second transaction to
OrmConcurrencyExceptions when trying to commit the first transaction.

In an ideal world, there would be a gwtorm transaction-like thing to
do a consistent read across several entities that did not have the
same problem as a write transaction. That would be a lot of work. We
can work around the problem by simply disabling auto-rebuilding in the
BatchUpdate path.

When the change is stale, this results in the NoteDb write definitely
short-circuiting, as NoteDbUpdateManager with checkExpectedState will
refuse to apply an update if the change is out of date. This is fine:
the change is out of date to begin with, and it just stays out of date
after the next ReviewDb write. In the case of a BatchUpdate it
actually rebuilds itself nearly instantaneously during reindexing.
(This is also possibly a latency improvement if there are multiple
changes, since reindexing can happen in parallel threads.)

Change-Id: I1be7945c8917cf7522237505a38e0d4c86337650

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java

@@ -18,11 +18,15 @@ import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.TruthJUnit.assume;
 import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
 import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
+
 import static java.nio.charset.StandardCharsets.UTF_8;
+
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
+import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
 import com.google.gerrit.acceptance.PushOneCommit;
@@ -47,6 +51,8 @@ import com.google.gerrit.server.change.PostReview;
 import com.google.gerrit.server.change.Rebuild;
 import com.google.gerrit.server.change.RevisionResource;
 import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.git.BatchUpdate;
+import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
 import com.google.gerrit.server.git.RepoRefCache;
 import com.google.gerrit.server.notedb.ChangeBundle;
 import com.google.gerrit.server.notedb.ChangeNotes;
@@ -58,6 +64,7 @@ import com.google.gerrit.testutil.NoteDbChecker;
 import com.google.gerrit.testutil.NoteDbMode;
 import com.google.gerrit.testutil.TestChanges;
 import com.google.gerrit.testutil.TestTimeUtil;
+import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
@@ -106,6 +113,9 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
   @Inject
   private TestChangeRebuilderWrapper rebuilderWrapper;
 
+  @Inject
+  private BatchUpdate.Factory batchUpdateFactory;
+
   @Before
   public void setUp() {
     assume().that(NoteDbMode.readWrite()).isFalse();
@@ -366,6 +376,67 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     assertThat(actual.differencesFrom(expected)).isEmpty();
   }
 
+  @Test
+  public void rebuildAutomaticallyWithinBatchUpdate() throws Exception {
+    setNotesMigration(true, true);
+
+    PushOneCommit.Result r = createChange();
+    final Change.Id id = r.getPatchSetId().getParentKey();
+    assertChangeUpToDate(true, id);
+
+    // Make a ReviewDb change behind NoteDb's back and ensure it's detected.
+    setNotesMigration(false, false);
+    gApi.changes().id(id.get()).topic(name("a-topic"));
+    setInvalidNoteDbState(id);
+    assertChangeUpToDate(false, id);
+
+    // Next NoteDb read comes inside the transaction started by BatchUpdate. In
+    // reality this could be caused by a failed update happening between when
+    // the change is parsed by ChangesCollection and when the BatchUpdate
+    // executes. We simulate it here by using BatchUpdate directly and not going
+    // through an API handler.
+    setNotesMigration(true, true);
+    final String msg = "message from BatchUpdate";
+    try (BatchUpdate bu = batchUpdateFactory.create(db, project,
+          identifiedUserFactory.create(user.getId()), TimeUtil.nowTs())) {
+      bu.addOp(id, new BatchUpdate.Op() {
+        @Override
+        public boolean updateChange(ChangeContext ctx) throws OrmException {
+          PatchSet.Id psId = ctx.getChange().currentPatchSetId();
+          ChangeMessage cm = new ChangeMessage(
+              new ChangeMessage.Key(id, ChangeUtil.messageUUID(ctx.getDb())),
+                  ctx.getUser().getAccountId(), ctx.getWhen(), psId);
+          cm.setMessage(msg);
+          ctx.getDb().changeMessages().insert(Collections.singleton(cm));
+          ctx.getUpdate(psId).setChangeMessage(msg);
+          return true;
+        }
+      });
+      bu.execute();
+    }
+    // As an implementation detail, change wasn't actually rebuilt inside the
+    // BatchUpdate transaction, but it was rebuilt during read for the
+    // subsequent reindex. Thus it's impossible to actually observe an
+    // out-of-date state in the caller.
+    assertChangeUpToDate(true, id);
+
+    // Check that the bundles are equal.
+    ChangeNotes notes = notesFactory.create(dbProvider.get(), project, id);
+    ChangeBundle actual = ChangeBundle.fromNotes(plcUtil, notes);
+    ChangeBundle expected = ChangeBundle.fromReviewDb(unwrapDb(), id);
+    assertThat(actual.differencesFrom(expected)).isEmpty();
+    assertThat(
+            Iterables.transform(
+                notes.getChangeMessages(),
+                new Function<ChangeMessage, String>() {
+                  @Override
+                  public String apply(ChangeMessage in) {
+                    return in.getMessage();
+                  }
+                }))
+        .contains(msg);
+  }
+
   @Test
   public void rebuildIgnoresErrorIfChangeIsUpToDateAfter() throws Exception {
     setNotesMigration(true, true);

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java

@@ -186,7 +186,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     }
 
     public ChangeNotes createForBatchUpdate(Change change) throws OrmException {
-      return new ChangeNotes(args, change).load();
+      return new ChangeNotes(args, change, false, null).load();
     }
 
     // TODO(dborowitz): Remove when deleting index schemas <27.