Merge changes from topic 'rebuild-fixes'

* changes:
  Don't auto-rebuild ChangeNotes from within BatchUpdate
  NoteDbUpdateManager: Default checkExpectedState to true
  Rename ChangeNotes.Factory#createForNew to createForBatchUpdate
  Parse draft notes from staged results if auto-rebuilding fails

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

@@ -18,10 +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;
@@ -46,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;
@@ -57,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;
 
@@ -105,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();
@@ -365,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);
@@ -431,7 +503,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
   }
 
   @Test
-  public void rebuildReturnsCorrectDraftResultEvenIfSavingToNoteDbFailed()
+  public void rebuildReturnsDraftResultWhenRebuildingInChangeNotesFails()
       throws Exception {
     setNotesMigration(true, true);
 
@@ -451,7 +523,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     assertThat(getMetaRef(allUsers, refsDraftComments(id, user.getId())))
         .isEqualTo(oldMetaId);
 
-    // Force the next rebuild attempt to fail.
+    // Force the next rebuild attempt to fail (in ChangeNotes).
     rebuilderWrapper.failNextUpdate();
     setNotesMigration(true, true);
     ChangeNotes notes = notesFactory.create(dbProvider.get(), project, id);
@@ -473,6 +545,62 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
         .isNotEqualTo(oldMetaId);
   }
 
+  @Test
+  public void rebuildReturnsDraftResultWhenRebuildingInDraftCommentNotesFails()
+      throws Exception {
+    setNotesMigration(true, true);
+
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getPatchSetId().getParentKey();
+    putDraft(user, id, 1, "comment by user");
+    assertChangeUpToDate(true, id);
+
+    ObjectId oldMetaId =
+        getMetaRef(allUsers, refsDraftComments(id, user.getId()));
+
+    // Add a draft behind NoteDb's back.
+    setNotesMigration(false, false);
+    putDraft(user, id, 1, "second comment by user");
+
+    ReviewDb db = unwrapDb();
+    Change c = db.changes().get(id);
+    // Leave change meta ID alone so DraftCommentNotes does the rebuild.
+    NoteDbChangeState bogusState = new NoteDbChangeState(
+        id, NoteDbChangeState.parse(c).getChangeMetaId(),
+        ImmutableMap.<Account.Id, ObjectId>of(
+            user.getId(),
+            ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")));
+    c.setNoteDbState(bogusState.toString());
+    db.changes().update(Collections.singleton(c));
+
+    assertDraftsUpToDate(false, id, user);
+    assertThat(getMetaRef(allUsers, refsDraftComments(id, user.getId())))
+        .isEqualTo(oldMetaId);
+
+    // Force the next rebuild attempt to fail (in DraftCommentNotes).
+    rebuilderWrapper.failNextUpdate();
+    setNotesMigration(true, true);
+    ChangeNotes notes = notesFactory.create(dbProvider.get(), project, id);
+    notes.getDraftComments(user.getId());
+    assertThat(getMetaRef(allUsers, refsDraftComments(id, user.getId())))
+        .isEqualTo(oldMetaId);
+
+    // Not up to date, but the actual returned state matches anyway.
+    assertChangeUpToDate(true, id);
+    assertDraftsUpToDate(false, id, user);
+    ChangeBundle actual = ChangeBundle.fromNotes(plcUtil, notes);
+    ChangeBundle expected = ChangeBundle.fromReviewDb(unwrapDb(), id);
+    assertThat(actual.differencesFrom(expected)).isEmpty();
+
+    // Another rebuild attempt succeeds
+    notesFactory.create(dbProvider.get(), project, id)
+        .getDraftComments(user.getId());
+    assertChangeUpToDate(true, id);
+    assertDraftsUpToDate(true, id, user);
+    assertThat(getMetaRef(allUsers, refsDraftComments(id, user.getId())))
+        .isNotEqualTo(oldMetaId);
+  }
+
   @Test
   public void rebuildAutomaticallyWhenDraftsOutOfDate() throws Exception {
     setNotesMigration(true, true);

gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java

@@ -855,7 +855,7 @@ public class BatchUpdate implements AutoCloseable {
       // Pass in preloaded change to controlFor, to avoid:
       //  - reading from a db that does not belong to this update
       //  - attempting to read a change that doesn't exist yet
-      ChangeNotes notes = changeNotesFactory.createForNew(c);
+      ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c);
       ChangeControl ctl = changeControlFactory.controlFor(notes, user);
       return new ChangeContext(ctl, new BatchUpdateReviewDb(db), repo, rw);
     }

gerrit-server/src/main/java/com/google/gerrit/server/git/ChainedReceiveCommands.java

@@ -49,6 +49,10 @@ public class ChainedReceiveCommands implements RefCache {
     this.refCache = checkNotNull(refCache);
   }
 
+  public RepoRefCache getRepoRefCache() {
+    return refCache;
+  }
+
   public boolean isEmpty() {
     return commands.isEmpty();
   }

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

@@ -185,8 +185,8 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       return new ChangeNotes(args, change);
     }
 
-    public ChangeNotes createForNew(Change change) throws OrmException {
+    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.

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

@@ -15,17 +15,20 @@
 package com.google.gerrit.server.notedb;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.Multimap;
+import com.google.gerrit.metrics.Timer1;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.git.RepoRefCache;
 import com.google.gerrit.server.notedb.NoteDbUpdateManager.StagedResult;
 import com.google.gerrit.server.project.NoSuchChangeException;
@@ -181,22 +184,42 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
   }
 
   private LoadHandle rebuildAndOpen(Repository repo) throws IOException {
-    try {
+    try (Timer1.Context timer =
-      NoteDbUpdateManager.Result r =
+        args.metrics.autoRebuildLatency.start(CHANGES)) {
-          args.rebuilder.get().rebuild(args.db.get(), getChangeId());
+      Change.Id cid = getChangeId();
-      if (r == null) {
+      ReviewDb db = args.db.get();
+      ChangeRebuilder rebuilder = args.rebuilder.get();
+      NoteDbUpdateManager manager = rebuilder.stage(db, cid);
+      if (manager == null) {
         return super.openHandle(repo); // May be null in tests.
       }
-      ObjectId draftsId = r.newState().getDraftIds().get(author);
+      NoteDbUpdateManager.Result r = manager.stageAndApplyDelta(change);
-      repo.scanForRepoChanges();
+      try {
-      return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), draftsId);
+        rebuilder.execute(db, cid, manager);
+        repo.scanForRepoChanges();
+      } catch (OrmException | IOException e) {
+        // See ChangeNotes#rebuildAndOpen.
+        args.metrics.autoRebuildFailureCount.increment(CHANGES);
+        checkNotNull(r.staged());
+        return LoadHandle.create(
+            ChangeNotesCommit.newStagedRevWalk(
+                repo, r.staged().allUsersObjects()),
+            draftsId(r));
+      }
+      return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), draftsId(r));
     } catch (NoSuchChangeException e) {
       return super.openHandle(repo);
-    } catch (OrmException | ConfigInvalidException e) {
+    } catch (OrmException e) {
       throw new IOException(e);
     }
   }
 
+  private ObjectId draftsId(NoteDbUpdateManager.Result r) {
+    checkNotNull(r);
+    checkNotNull(r.newState());
+    return r.newState().getDraftIds().get(author);
+  }
+
   @VisibleForTesting
   NoteMap getNoteMap() {
     return revisionNoteMap != null ? revisionNoteMap.noteMap : null;

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

@@ -209,7 +209,8 @@ public class NoteDbChangeState {
     return changeId;
   }
 
-  ObjectId getChangeMetaId() {
+  @VisibleForTesting
+  public ObjectId getChangeMetaId() {
     return changeMetaId;
   }
 

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

@@ -180,7 +180,7 @@ public class NoteDbUpdateManager {
   private OpenRepo changeRepo;
   private OpenRepo allUsersRepo;
   private Map<Change.Id, StagedResult> staged;
-  private boolean checkExpectedState;
+  private boolean checkExpectedState = true;
 
   @AssistedInject
   NoteDbUpdateManager(GitRepositoryManager repoManager,
@@ -486,7 +486,7 @@ public class NoteDbUpdateManager {
         continue;
       }
 
-      if (!expectedState.isChangeUpToDate(changeRepo.cmds)) {
+      if (!expectedState.isChangeUpToDate(changeRepo.cmds.getRepoRefCache())) {
         throw new OrmConcurrencyException(String.format(
             "cannot apply NoteDb updates for change %s;"
             + " change meta ref does not match %s",
@@ -504,7 +504,7 @@ public class NoteDbUpdateManager {
 
       Account.Id accountId = u.getAccountId();
       if (!expectedState.areDraftsUpToDate(
-          allUsersRepo.cmds, accountId)) {
+          allUsersRepo.cmds.getRepoRefCache(), accountId)) {
         throw new OrmConcurrencyException(String.format(
             "cannot apply NoteDb updates for change %s;"
             + " draft ref for account %s does not match %s",