Implement consistency checker for NoteDb

Change the public interface of ConsistencyChecker to only take a
ChangeControl, since this always includes a pre-loaded ChangeNotes.
Not all callers have a ChangeData available, and it's wasteful to load
a Change just to turn around and load a ChangeNotes from it again.

This simplifies the interface and implementation, but does mean that
certain types of corruption, such as complete garbage in a NoteDb
commit message, will result in an error in the caller rather than a
nicely-formatted message. However, when rewriting the tests, I found
that there were not really any situations we cared about that are
really and truly corrupt data.

Most issues were due to the database and the underlying repo getting
out of sync. From the perspective of NoteDb, it's pretty easy to
generate these corrupt states. The only place we had to go behind the
back of normal change operations during tests was when pointing a
patch set at a nonexisting object, since otherwise PatchSetUtil will
try to parse the object to get the subject. It was easy enough to
manually construct a commit message there.

The most complicated fix was for deleting patch sets pointing to
nonexistent objects. I think this actually turned out nicer, since we
were able to decompose the problem into a set of independent deletion
operations that then get packaged together into a single BatchUpdate.

The one and only test that really cannot be replicated in NoteDb is
when the change destination repository is missing, since that's where
the NoteDb data would be stored. This one it's ok to skip.

Since I was rewriting the tests pretty much completely anyway, I
simplified some code by implementing ProblemInfo#equals so we can use
assertThat(problems).containsExactly(...) for more compact assertions.

Change-Id: I1d77f9202c053127089b4926383abf02aa24466b

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.extensions.common;
 
+import java.util.Objects;
+
 public class ProblemInfo {
   public enum Status {
     FIXED, FIX_FAILED
@@ -23,6 +25,22 @@ public class ProblemInfo {
   public Status status;
   public String outcome;
 
+  @Override
+  public int hashCode() {
+    return Objects.hash(message, status, outcome);
+  }
+
+  @Override
+  public boolean equals(Object o) {
+    if (!(o instanceof ProblemInfo)) {
+      return false;
+    }
+    ProblemInfo p = (ProblemInfo) o;
+    return Objects.equals(message, p.message)
+        && Objects.equals(status, p.status)
+        && Objects.equals(outcome, p.outcome);
+  }
+
   @Override
   public String toString() {
     StringBuilder sb = new StringBuilder(getClass().getSimpleName())

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -355,7 +355,21 @@ public class ChangeJson {
   }
 
   private ChangeInfo checkOnly(ChangeData cd) {
-    ConsistencyChecker.Result result = checkerProvider.get().check(cd, fix);
+    ChangeControl ctl;
+    try {
+      ctl = cd.changeControl().forUser(userProvider.get());
+    } catch (OrmException e) {
+      String msg = "Error loading change";
+      log.warn(msg + " " + cd.getId(), e);
+      ChangeInfo info = new ChangeInfo();
+      info._number = cd.getId().get();
+      ProblemInfo p = new ProblemInfo();
+      p.message = msg;
+      info.problems = Lists.newArrayList(p);
+      return info;
+    }
+
+    ConsistencyChecker.Result result = checkerProvider.get().check(ctl, fix);
     ChangeInfo info;
     Change c = result.change();
     if (c != null) {
@@ -384,9 +398,11 @@ public class ChangeJson {
       Optional<PatchSet.Id> limitToPsId) throws PatchListNotAvailableException,
       GpgException, OrmException, IOException {
     ChangeInfo out = new ChangeInfo();
+    CurrentUser user = userProvider.get();
+    ChangeControl ctl = cd.changeControl().forUser(user);
 
     if (has(CHECK)) {
-      out.problems = checkerProvider.get().check(cd.change(), fix).problems();
+      out.problems = checkerProvider.get().check(ctl, fix).problems();
       // If any problems were fixed, the ChangeData needs to be reloaded.
       for (ProblemInfo p : out.problems) {
         if (p.status == ProblemInfo.Status.FIXED) {
@@ -397,8 +413,6 @@ public class ChangeJson {
     }
 
     Change in = cd.change();
-    CurrentUser user = userProvider.get();
-    ChangeControl ctl = cd.changeControl().forUser(user);
     out.project = in.getProject().get();
     out.branch = in.getDest().getShortName();
     out.topic = in.getTopic();

gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java

@@ -18,12 +18,10 @@ import com.google.gerrit.extensions.api.changes.FixInput;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.extensions.restapi.NotImplementedException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.extensions.restapi.RestReadView;
-import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -32,27 +30,22 @@ import java.util.EnumSet;
 
 public class Check implements RestReadView<ChangeResource>,
     RestModifyView<ChangeResource, FixInput> {
-  private final NotesMigration notesMigration;
   private final ChangeJson.Factory jsonFactory;
 
   @Inject
-  Check(NotesMigration notesMigration,
+  Check(ChangeJson.Factory json) {
-      ChangeJson.Factory json) {
-    this.notesMigration = notesMigration;
     this.jsonFactory = json;
   }
 
   @Override
   public Response<ChangeInfo> apply(ChangeResource rsrc)
       throws RestApiException, OrmException {
-    checkEnabled();
     return Response.withMustRevalidate(newChangeJson().format(rsrc));
   }
 
   @Override
   public Response<ChangeInfo> apply(ChangeResource rsrc, FixInput input)
       throws RestApiException, OrmException {
-    checkEnabled();
     ChangeControl ctl = rsrc.getControl();
     if (!ctl.isOwner()
         && !ctl.getProjectControl().isOwner()
@@ -65,10 +58,4 @@ public class Check implements RestReadView<ChangeResource>,
   private ChangeJson newChangeJson() {
     return jsonFactory.create(EnumSet.of(ListChangesOption.CHECK));
   }
-
-  private void checkEnabled() throws NotImplementedException {
-    if (notesMigration.readChanges()) {
-      throw new NotImplementedException("check not implemented for NoteDb");
-    }
-  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java

@@ -14,7 +14,8 @@
 
 package com.google.gerrit.server.change;
 
-import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES;
 import static com.google.gerrit.reviewdb.server.ReviewDbUtil.intKeyOrdering;
 import static com.google.gerrit.server.ChangeUtil.PS_ID_ORDER;
@@ -39,23 +40,22 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.git.BatchUpdate;
+import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.git.validators.CommitValidators;
-import com.google.gerrit.server.index.change.ChangeIndexer;
 import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.PatchSetState;
-import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gwtorm.server.AtomicUpdate;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -78,9 +78,10 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
+import java.util.Set;
 
 /**
  * Checks changes for various kinds of inconsistency and corruption.
@@ -94,12 +95,10 @@ public class ConsistencyChecker {
 
   @AutoValue
   public abstract static class Result {
-    private static Result create(Change.Id id, List<ProblemInfo> problems) {
+    private static Result create(ChangeControl ctl,
-      return new AutoValue_ConsistencyChecker_Result(id, null, problems);
+        List<ProblemInfo> problems) {
-    }
+      return new AutoValue_ConsistencyChecker_Result(
-
+          ctl.getId(), ctl.getChange(), problems);
-    private static Result create(Change c, List<ProblemInfo> problems) {
-      return new AutoValue_ConsistencyChecker_Result(c.getId(), c, problems);
     }
 
     public abstract Change.Id id();
@@ -110,22 +109,20 @@ public class ConsistencyChecker {
     public abstract List<ProblemInfo> problems();
   }
 
-  private final Provider<ReviewDb> db;
-  private final GitRepositoryManager repoManager;
-  private final NotesMigration notesMigration;
-  private final Provider<CurrentUser> user;
-  private final Provider<PersonIdent> serverIdent;
-  private final PatchSetInfoFactory patchSetInfoFactory;
-  private final PatchSetInserter.Factory patchSetInserterFactory;
   private final BatchUpdate.Factory updateFactory;
-  private final ChangeIndexer indexer;
   private final ChangeControl.GenericFactory changeControlFactory;
   private final ChangeNotes.Factory notesFactory;
-  private final ChangeUpdate.Factory changeUpdateFactory;
   private final DynamicItem<AccountPatchReviewStore> accountPatchReviewStore;
+  private final GitRepositoryManager repoManager;
+  private final PatchSetInfoFactory patchSetInfoFactory;
+  private final PatchSetInserter.Factory patchSetInserterFactory;
+  private final PatchSetUtil psUtil;
+  private final Provider<CurrentUser> user;
+  private final Provider<PersonIdent> serverIdent;
+  private final Provider<ReviewDb> db;
 
   private FixInput fix;
-  private Change change;
+  private ChangeControl ctl;
   private Repository repo;
   private RevWalk rw;
 
@@ -137,67 +134,51 @@ public class ConsistencyChecker {
   private List<ProblemInfo> problems;
 
   @Inject
-  ConsistencyChecker(Provider<ReviewDb> db,
+  ConsistencyChecker(
-      GitRepositoryManager repoManager,
-      NotesMigration notesMigration,
-      Provider<CurrentUser> user,
       @GerritPersonIdent Provider<PersonIdent> serverIdent,
-      PatchSetInfoFactory patchSetInfoFactory,
-      PatchSetInserter.Factory patchSetInserterFactory,
       BatchUpdate.Factory updateFactory,
-      ChangeIndexer indexer,
       ChangeControl.GenericFactory changeControlFactory,
       ChangeNotes.Factory notesFactory,
-      ChangeUpdate.Factory changeUpdateFactory,
+      DynamicItem<AccountPatchReviewStore> accountPatchReviewStore,
-      DynamicItem<AccountPatchReviewStore> accountPatchReviewStore) {
+      GitRepositoryManager repoManager,
+      PatchSetInfoFactory patchSetInfoFactory,
+      PatchSetInserter.Factory patchSetInserterFactory,
+      PatchSetUtil psUtil,
+      Provider<CurrentUser> user,
+      Provider<ReviewDb> db) {
+    this.accountPatchReviewStore = accountPatchReviewStore;
+    this.changeControlFactory = changeControlFactory;
     this.db = db;
-    this.notesMigration = notesMigration;
+    this.notesFactory = notesFactory;
-    this.repoManager = repoManager;
-    this.user = user;
-    this.serverIdent = serverIdent;
     this.patchSetInfoFactory = patchSetInfoFactory;
     this.patchSetInserterFactory = patchSetInserterFactory;
+    this.psUtil = psUtil;
+    this.repoManager = repoManager;
+    this.serverIdent = serverIdent;
     this.updateFactory = updateFactory;
-    this.indexer = indexer;
+    this.user = user;
-    this.changeControlFactory = changeControlFactory;
-    this.notesFactory = notesFactory;
-    this.changeUpdateFactory = changeUpdateFactory;
-    this.accountPatchReviewStore = accountPatchReviewStore;
     reset();
   }
 
   private void reset() {
-    change = null;
+    ctl = null;
     repo = null;
     rw = null;
     problems = new ArrayList<>();
   }
 
-  public Result check(ChangeData cd) {
+  private Change change() {
-    return check(cd, null);
+    return ctl.getChange();
   }
 
-  public Result check(ChangeData cd, @Nullable FixInput f) {
+  public Result check(ChangeControl cc, @Nullable FixInput f) {
-    reset();
+    checkNotNull(cc);
-    try {
-      return check(cd.change(), f);
-    } catch (OrmException e) {
-      error("Error looking up change", e);
-      return Result.create(cd.getId(), problems);
-    }
-  }
-
-  public Result check(Change c) {
-    return check(c, null);
-  }
-
-  public Result check(Change c, @Nullable FixInput f) {
-    reset();
-    fix = f;
-    change = c;
     try {
+      reset();
+      ctl = cc;
+      fix = f;
       checkImpl();
-      return Result.create(c, problems);
+      return result();
     } finally {
       if (rw != null) {
         rw.close();
@@ -209,8 +190,6 @@ public class ConsistencyChecker {
   }
 
   private void checkImpl() {
-    checkState(!notesMigration.readChanges(),
-        "ConsistencyChecker for NoteDb not yet implemented");
     checkOwner();
     checkCurrentPatchSetEntity();
 
@@ -226,8 +205,8 @@ public class ConsistencyChecker {
 
   private void checkOwner() {
     try {
-      if (db.get().accounts().get(change.getOwner()) == null) {
+      if (db.get().accounts().get(change().getOwner()) == null) {
-        problem("Missing change owner: " + change.getOwner());
+        problem("Missing change owner: " + change().getOwner());
       }
     } catch (OrmException e) {
       error("Failed to look up owner", e);
@@ -236,10 +215,10 @@ public class ConsistencyChecker {
 
   private void checkCurrentPatchSetEntity() {
     try {
-      PatchSet.Id psId = change.currentPatchSetId();
+      currPs = psUtil.current(db.get(), ctl.getNotes());
-      currPs = db.get().patchSets().get(psId);
       if (currPs == null) {
-        problem(String.format("Current patch set %d not found", psId.get()));
+        problem(String.format("Current patch set %d not found",
+              change().currentPatchSetId().get()));
       }
     } catch (OrmException e) {
       error("Failed to look up current patch set", e);
@@ -247,7 +226,7 @@ public class ConsistencyChecker {
   }
 
   private boolean openRepo() {
-    Project.NameKey project = change.getDest().getParentKey();
+    Project.NameKey project = change().getDest().getParentKey();
     try {
       repo = repoManager.openRepository(project);
       rw = new RevWalk(repo);
@@ -262,13 +241,11 @@ public class ConsistencyChecker {
   private boolean checkPatchSets() {
     List<PatchSet> all;
     try {
-      all = Lists.newArrayList(db.get().patchSets().byChange(change.getId()));
+      // Iterate in descending order.
+      all = PS_ID_ORDER.sortedCopy(psUtil.byChange(db.get(), ctl.getNotes()));
     } catch (OrmException e) {
       return error("Failed to look up patch sets", e);
     }
-    // Iterate in descending order so deletePatchSet can assume the latest patch
-    // set exists.
-    Collections.sort(all, PS_ID_ORDER.reverse());
     patchSetsBySha = MultimapBuilder.hashKeys(all.size())
         .treeSetValues(PS_ID_ORDER)
         .build();
@@ -287,6 +264,7 @@ public class ConsistencyChecker {
       refs = Collections.emptyMap();
     }
 
+    List<DeletePatchSetFromDbOp> deletePatchSetOps = new ArrayList<>();
     for (PatchSet ps : all) {
       // Check revision format.
       int psNum = ps.getId().get();
@@ -317,17 +295,21 @@ public class ConsistencyChecker {
           objId, String.format("patch set %d", psNum));
       if (psCommit == null) {
         if (fix != null && fix.deletePatchSetIfCommitMissing) {
-          deletePatchSet(lastProblem(), change.getProject(), ps.getId());
+          deletePatchSetOps.add(
+              new DeletePatchSetFromDbOp(lastProblem(), ps.getId()));
         }
         continue;
       } else if (refProblem != null && fix != null) {
         fixPatchSetRef(refProblem, ps);
       }
-      if (ps.getId().equals(change.currentPatchSetId())) {
+      if (ps.getId().equals(change().currentPatchSetId())) {
         currPsCommit = psCommit;
       }
     }
 
+    // Delete any bad patch sets found above, in a single update.
+    deletePatchSets(deletePatchSetOps);
+
     // Check for duplicates.
     for (Map.Entry<ObjectId, Collection<PatchSet>> e
         : patchSetsBySha.asMap().entrySet()) {
@@ -342,7 +324,7 @@ public class ConsistencyChecker {
   }
 
   private void checkMerged() {
-    String refName = change.getDest().get();
+    String refName = change().getDest().get();
     Ref dest;
     try {
       dest = repo.getRefDatabase().exactRef(refName);
@@ -375,22 +357,27 @@ public class ConsistencyChecker {
     }
   }
 
+  private ProblemInfo wrongChangeStatus(PatchSet.Id psId, RevCommit commit) {
+    String refName = change().getDest().get();
+    return problem(String.format(
+        "Patch set %d (%s) is merged into destination ref %s (%s), but change"
+        + " status is %s", psId.get(), commit.name(),
+        refName, tip.name(), change().getStatus()));
+  }
+
   private void checkMergedBitMatchesStatus(PatchSet.Id psId, RevCommit commit,
       boolean merged) {
-    String refName = change.getDest().get();
+    String refName = change().getDest().get();
-    if (merged && change.getStatus() != Change.Status.MERGED) {
+    if (merged && change().getStatus() != Change.Status.MERGED) {
-      ProblemInfo p = problem(String.format(
+      ProblemInfo p = wrongChangeStatus(psId, commit);
-          "Patch set %d (%s) is merged into destination ref %s (%s), but change"
-          + " status is %s", psId.get(), commit.name(),
-          refName, tip.name(), change.getStatus()));
       if (fix != null) {
         fixMerged(p);
       }
-    } else if (!merged && change.getStatus() == Change.Status.MERGED) {
+    } else if (!merged && change().getStatus() == Change.Status.MERGED) {
       problem(String.format("Patch set %d (%s) is not merged into"
             + " destination ref %s (%s), but change status is %s",
             currPs.getId().get(), commit.name(), refName, tip.name(),
-            change.getStatus()));
+            change().getStatus()));
     }
   }
 
@@ -401,16 +388,12 @@ public class ConsistencyChecker {
     if (commit == null) {
       return;
     }
-    if (Objects.equals(commit, currPsCommit)) {
-      // Caller gave us latest patch set SHA-1; verified in checkPatchSets.
-      return;
-    }
 
     try {
       if (!rw.isMergedInto(commit, tip)) {
         problem(String.format("Expected merged commit %s is not merged into"
               + " destination ref %s (%s)",
-              commit.name(), change.getDest().get(), tip.name()));
+              commit.name(), change().getDest().get(), tip.name()));
         return;
       }
 
@@ -425,8 +408,8 @@ public class ConsistencyChecker {
         }
         try {
           Change c = notesFactory.createChecked(
-              db.get(), change.getProject(), psId.getParentKey()).getChange();
+              db.get(), change().getProject(), psId.getParentKey()).getChange();
-          if (!c.getDest().equals(change.getDest())) {
+          if (!c.getDest().equals(change().getDest())) {
             continue;
           }
         } catch (OrmException | NoSuchChangeException e) {
@@ -442,16 +425,17 @@ public class ConsistencyChecker {
           String changeId = Iterables.getFirst(
               commit.getFooterLines(FooterConstants.CHANGE_ID), null);
           // Missing Change-Id footer is ok, but mismatched is not.
-          if (changeId != null && !changeId.equals(change.getKey().get())) {
+          if (changeId != null && !changeId.equals(change().getKey().get())) {
             problem(String.format("Expected merged commit %s has Change-Id: %s,"
                   + " but expected %s",
-                  commit.name(), changeId, change.getKey().get()));
+                  commit.name(), changeId, change().getKey().get()));
             return;
           }
-          PatchSet.Id psId = insertPatchSet(commit);
+          // TODO(dborowitz): Combine into one op.
-          if (psId != null) {
+          insertPatchSet(commit);
-            checkMergedBitMatchesStatus(psId, commit, true);
+          fixMerged(problem(String.format(
-          }
+              "Expected merged commit %s has no associated patch set",
+              commit.name())));
           break;
 
         case 1:
@@ -460,10 +444,12 @@ public class ConsistencyChecker {
           // TODO(dborowitz): This could be fixed if it's an older patch set of
           // the current change.
           PatchSet.Id id = psIds.get(0);
-          if (!id.equals(change.currentPatchSetId())) {
+          if (id.equals(change().currentPatchSetId())) {
+            fixMerged(wrongChangeStatus(id, commit));
+          } else {
             problem(String.format("Expected merged commit %s corresponds to"
                   + " patch set %s, which is not the current patch set %s",
-                  commit.name(), id, change.currentPatchSetId()));
+                  commit.name(), id, change().currentPatchSetId()));
           }
           break;
 
@@ -490,17 +476,14 @@ public class ConsistencyChecker {
     }
 
     try {
-      ChangeControl ctl = changeControlFactory
-          .controlFor(db.get(), change, user.get());
       PatchSet.Id psId =
-          ChangeUtil.nextPatchSetId(repo, change.currentPatchSetId());
+          ChangeUtil.nextPatchSetId(repo, change().currentPatchSetId());
       PatchSetInserter inserter =
           patchSetInserterFactory.create(ctl, psId, commit);
-      try (BatchUpdate bu = updateFactory.create(
+      try (BatchUpdate bu = newBatchUpdate();
-            db.get(), change.getProject(), ctl.getUser(), TimeUtil.nowTs());
           ObjectInserter oi = repo.newObjectInserter()) {
         bu.setRepository(repo, rw, oi);
-        bu.addOp(change.getId(), inserter
+        bu.addOp(ctl.getId(), inserter
             .setValidatePolicy(CommitValidators.Policy.NONE)
             .setFireRevisionCreated(false)
             .setSendMail(false)
@@ -509,7 +492,8 @@ public class ConsistencyChecker {
                 "Patch set for merged commit inserted by consistency checker"));
         bu.execute();
       }
-      change = inserter.getChange();
+      ctl = changeControlFactory.controlFor(
+          db.get(), inserter.getChange(), ctl.getUser());
       p.status = Status.FIXED;
       p.outcome = "Inserted as patch set " + psId.get();
       return psId;
@@ -523,30 +507,33 @@ public class ConsistencyChecker {
   }
 
   private void fixMerged(ProblemInfo p) {
-    try {
+    try (BatchUpdate bu = newBatchUpdate();
-      change = db.get().changes().atomicUpdate(change.getId(),
+        ObjectInserter oi = repo.newObjectInserter()) {
-          new AtomicUpdate<Change>() {
+      bu.setRepository(repo, rw, oi);
-            @Override
+      bu.addOp(ctl.getId(), new BatchUpdate.Op() {
-            public Change update(Change c) {
+        @Override
-              c.setStatus(Change.Status.MERGED);
+        public boolean updateChange(ChangeContext ctx) throws OrmException {
-              return c;
+          ctx.getChange().setStatus(Change.Status.MERGED);
-            }
+          ctx.getUpdate(ctx.getChange().currentPatchSetId())
-          });
+            .fixStatus(Change.Status.MERGED);
-      ChangeUpdate changeUpdate =
+          return true;
-          changeUpdateFactory.create(
+        }
-              changeControlFactory.controlFor(db.get(), change, user.get()));
+      });
-      changeUpdate.fixStatus(Change.Status.MERGED);
+      bu.execute();
-      changeUpdate.commit();
-      indexer.index(db.get(), change);
       p.status = Status.FIXED;
       p.outcome = "Marked change as merged";
-    } catch (OrmException | IOException | NoSuchChangeException e) {
+    } catch (UpdateException | RestApiException e) {
-      log.warn("Error marking " + change.getId() + "as merged", e);
+      log.warn("Error marking " + ctl.getId() + "as merged", e);
       p.status = Status.FIX_FAILED;
       p.outcome = "Error updating status to merged";
     }
   }
 
+  private BatchUpdate newBatchUpdate() {
+    return updateFactory.create(
+        db.get(), change().getProject(), ctl.getUser(), TimeUtil.nowTs());
+  }
+
   private void fixPatchSetRef(ProblemInfo p, PatchSet ps) {
     try {
       RefUpdate ru = repo.updateRef(ps.getId().toRefName());
@@ -582,59 +569,108 @@ public class ConsistencyChecker {
     }
   }
 
-  private void deletePatchSet(ProblemInfo p, Project.NameKey project,
+  private void deletePatchSets(List<DeletePatchSetFromDbOp> ops) {
-      PatchSet.Id psId) {
+    try (BatchUpdate bu = newBatchUpdate();
-    ReviewDb db = this.db.get();
+        ObjectInserter oi = repo.newObjectInserter()) {
-    Change.Id cid = psId.getParentKey();
+      bu.setRepository(repo, rw, oi);
-    try {
+      for (DeletePatchSetFromDbOp op : ops) {
-      db.changes().beginTransaction(cid);
+        checkArgument(op.psId.getParentKey().equals(ctl.getId()));
-      try {
+        bu.addOp(ctl.getId(), op);
-        ChangeNotes notes = notesFactory.createChecked(db, project, cid);
-        Change c = notes.getChange();
-        if (psId.equals(c.currentPatchSetId())) {
-          List<PatchSet> all = Lists.newArrayList(db.patchSets().byChange(cid));
-          if (all.size() == 1 && all.get(0).getId().equals(psId)) {
-            p.status = Status.FIX_FAILED;
-            p.outcome = "Cannot delete patch set; no patch sets would remain";
-            return;
-          }
-          // If there were multiple missing patch sets, assumes deletePatchSet
-          // has been called in decreasing order, so the max remaining PatchSet
-          // is the effective current patch set.
-          Collections.sort(all, PS_ID_ORDER.reverse());
-          PatchSet.Id latest = null;
-          for (PatchSet ps : all) {
-            latest = ps.getId();
-            if (!ps.getId().equals(psId)) {
-              break;
-            }
-          }
-          c.setCurrentPatchSet(patchSetInfoFactory.get(db, notes, latest));
-          db.changes().update(Collections.singleton(c));
-        }
-
-        // Delete dangling primary key references. Don't delete ChangeMessages,
-        // which don't use patch sets as a primary key, and may provide useful
-        // historical information.
-        accountPatchReviewStore.get().clearReviewed(psId);
-        db.patchSetApprovals().delete(
-            db.patchSetApprovals().byPatchSet(psId));
-        db.patchComments().delete(
-            db.patchComments().byPatchSet(psId));
-        db.patchSets().deleteKeys(Collections.singleton(psId));
-        db.commit();
-
-        p.status = Status.FIXED;
-        p.outcome = "Deleted patch set";
-      } finally {
-        db.rollback();
       }
-    } catch (PatchSetInfoNotAvailableException | OrmException
+      bu.addOp(ctl.getId(), new UpdateCurrentPatchSetOp(ops));
-        | NoSuchChangeException e) {
+      bu.execute();
+    } catch (NoPatchSetsWouldRemainException e) {
+      for (DeletePatchSetFromDbOp op : ops) {
+        op.p.status = Status.FIX_FAILED;
+        op.p.outcome = e.getMessage();
+      }
+    } catch (UpdateException | RestApiException e) {
       String msg = "Error deleting patch set";
-      log.warn(msg + ' ' + psId, e);
+      log.warn(msg + " of change " + ops.get(0).psId.getParentKey(), e);
-      p.status = Status.FIX_FAILED;
+      for (DeletePatchSetFromDbOp op : ops) {
-      p.outcome = msg;
+        // Overwrite existing statuses that were set before the transaction was
+        // rolled back.
+        op.p.status = Status.FIX_FAILED;
+        op.p.outcome = msg;
+      }
+    }
+  }
+
+  private class DeletePatchSetFromDbOp extends BatchUpdate.Op {
+    private final ProblemInfo p;
+    private final PatchSet.Id psId;
+
+    private DeletePatchSetFromDbOp(ProblemInfo p, PatchSet.Id psId) {
+      this.p = p;
+      this.psId = psId;
+    }
+
+    @Override
+    public boolean updateChange(ChangeContext ctx)
+        throws OrmException, PatchSetInfoNotAvailableException {
+      // Delete dangling key references.
+      ReviewDb db = DeleteDraftChangeOp.unwrap(ctx.getDb());
+      accountPatchReviewStore.get().clearReviewed(psId);
+      db.changeMessages().delete(
+          db.changeMessages().byChange(psId.getParentKey()));
+      db.patchSetApprovals().delete(
+          db.patchSetApprovals().byPatchSet(psId));
+      db.patchComments().delete(
+          db.patchComments().byPatchSet(psId));
+      db.patchSets().deleteKeys(Collections.singleton(psId));
+
+      // NoteDb requires no additional fiddling; setting the state to deleted is
+      // sufficient to filter everything else out.
+      ctx.getUpdate(psId).setPatchSetState(PatchSetState.DELETED);
+
+      p.status = Status.FIXED;
+      p.outcome = "Deleted patch set";
+      return true;
+    }
+  }
+
+  private static class NoPatchSetsWouldRemainException
+      extends RestApiException {
+    private static final long serialVersionUID = 1L;
+
+    private NoPatchSetsWouldRemainException() {
+      super("Cannot delete patch set; no patch sets would remain");
+    }
+  }
+
+  private class UpdateCurrentPatchSetOp extends BatchUpdate.Op {
+    private final Set<PatchSet.Id> toDelete;
+
+    private UpdateCurrentPatchSetOp(List<DeletePatchSetFromDbOp> deleteOps) {
+      toDelete = new HashSet<>();
+      for (DeletePatchSetFromDbOp op : deleteOps) {
+        toDelete.add(op.psId);
+      }
+    }
+
+    @Override
+    public boolean updateChange(ChangeContext ctx)
+        throws OrmException, PatchSetInfoNotAvailableException,
+        NoPatchSetsWouldRemainException {
+      if (!toDelete.contains(ctx.getChange().currentPatchSetId())) {
+        return false;
+      }
+      Set<PatchSet.Id> all = new HashSet<>();
+      // Doesn't make any assumptions about the order in which deletes happen
+      // and whether they are seen by this op; we are already given the full set
+      // of patch sets that will eventually be deleted in this update.
+      for (PatchSet ps : psUtil.byChange(ctx.getDb(), ctx.getNotes())) {
+        if (!toDelete.contains(ps.getId())) {
+          all.add(ps.getId());
+        }
+      }
+      if (all.isEmpty()) {
+        throw new NoPatchSetsWouldRemainException();
+      }
+      PatchSet.Id latest = ReviewDbUtil.intKeyOrdering().max(all);
+      ctx.getChange().setCurrentPatchSet(
+          patchSetInfoFactory.get(ctx.getDb(), ctx.getNotes(), latest));
+      return true;
     }
   }
 
@@ -687,6 +723,10 @@ public class ConsistencyChecker {
   }
 
   private void warn(Throwable t) {
-    log.warn("Error in consistency check of change " + change.getId(), t);
+    log.warn("Error in consistency check of change " + ctl.getId(), t);
+  }
+
+  private Result result() {
+    return Result.create(ctl, problems);
   }
 }