opendev/gerrit
Code Issues Proposed changes

ConsistencyChecker: Handle dangling PS ref with expect_merged_as

Browse Source 
If a direct push to a branch fails, it's possible to both create a new
patch set ref and update the branch, but fail the database write that
creates the new patch set record and updates the change's current
patch set. ConsistencyChecker was not able to fix up this case,
because the patch set ID it found in the refs did not match the
current patch set ID of the change. There was no reason for this other
than I didn't implement it. But we've seen these in the wild, so now
is the time to fix it.

This doesn't handle the general problem of dangling patch set refs
outside of the expect_merged_as path; fixing those is still possible,
but is not the proximate corruption issue we're trying to fix here.

Change-Id: I0c2082e45bd6090e0b2b9e024c2d32a7df603a66
This commit is contained in:
Dave Borowitz
2016-08-12 09:47:55 -04:00
parent 666ddc185f
commit 641cceb20c
2 changed files with 178 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

116
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
View File

@@ -512,11 +512,11 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
ctl, fix,
problem(
"No patch set found for merged commit " + mergedAs.name(),
FIXED, "Inserted as patch set 2"),
FIXED, "Marked change as merged"),
problem(
"Expected merged commit " + mergedAs.name()
+ " has no associated patch set",
FIXED, "Marked change as merged"));
FIXED, "Inserted as patch set 2"));
ctl = reload(ctl);
PatchSet.Id psId2 = new PatchSet.Id(ctl.getId(), 2);
@@ -553,11 +553,11 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
ctl, fix,
problem(
"No patch set found for merged commit " + mergedAs.name(),
FIXED, "Inserted as patch set 2"),
FIXED, "Marked change as merged"),
problem(
"Expected merged commit " + mergedAs.name()
+ " has no associated patch set",
FIXED, "Marked change as merged"));
FIXED, "Inserted as patch set 2"));
ctl = reload(ctl);
PatchSet.Id psId2 = new PatchSet.Id(ctl.getId(), 2);
@@ -584,9 +584,106 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
assertProblems(
ctl, fix,
problem(
"Expected merged commit " + rev1 + " corresponds to patch set "
+ ps1.getId() + ", which is not the current patch set "
+ ps2.getId()));
"No patch set found for merged commit " + rev1,
FIXED, "Marked change as merged"),
problem(
"Expected merge commit " + rev1 + " corresponds to patch set 1,"
+ " not the current patch set 2",
FIXED, "Deleted patch set"),
problem(
"Expected merge commit " + rev1 + " corresponds to patch set 1,"
+ " not the current patch set 2",
FIXED, "Inserted as patch set 3"));
ctl = reload(ctl);
PatchSet.Id psId3 = new PatchSet.Id(ctl.getId(), 3);
assertThat(ctl.getChange().currentPatchSetId()).isEqualTo(psId3);
assertThat(ctl.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
assertThat(psUtil.byChangeAsMap(db, ctl.getNotes()).keySet())
.containsExactly(ps2.getId(), psId3);
assertThat(psUtil.get(db, ctl.getNotes(), psId3).getRevision().get())
.isEqualTo(rev1);
}
@Test
public void expectedMergedCommitIsDanglingPatchSetOlderThanCurrent()
throws Exception {
ChangeControl ctl = insertChange();
PatchSet ps1 = psUtil.current(db, ctl.getNotes());
// Create dangling ref so next ID in the database becomes 3.
PatchSet.Id psId2 = new PatchSet.Id(ctl.getId(), 2);
RevCommit commit2 = patchSetCommit(psId2);
String rev2 = commit2.name();
testRepo.branch(psId2.toRefName()).update(commit2);
ctl = incrementPatchSet(ctl);
PatchSet ps3 = psUtil.current(db, ctl.getNotes());
assertThat(ps3.getId().get()).isEqualTo(3);
testRepo.branch(ctl.getChange().getDest().get())
.update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev2)));
FixInput fix = new FixInput();
fix.expectMergedAs = rev2;
assertProblems(
ctl, fix,
problem(
"No patch set found for merged commit " + rev2,
FIXED, "Marked change as merged"),
problem(
"Expected merge commit " + rev2 + " corresponds to patch set 2,"
+ " not the current patch set 3",
FIXED, "Deleted patch set"),
problem(
"Expected merge commit " + rev2 + " corresponds to patch set 2,"
+ " not the current patch set 3",
FIXED, "Inserted as patch set 4"));
ctl = reload(ctl);
PatchSet.Id psId4 = new PatchSet.Id(ctl.getId(), 4);
assertThat(ctl.getChange().currentPatchSetId()).isEqualTo(psId4);
assertThat(ctl.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
assertThat(psUtil.byChangeAsMap(db, ctl.getNotes()).keySet())
.containsExactly(ps1.getId(), ps3.getId(), psId4);
assertThat(psUtil.get(db, ctl.getNotes(), psId4).getRevision().get())
.isEqualTo(rev2);
}
@Test
public void expectedMergedCommitIsDanglingPatchSetNewerThanCurrent()
throws Exception {
ChangeControl ctl = insertChange();
PatchSet ps1 = psUtil.current(db, ctl.getNotes());
// Create dangling ref with no patch set.
PatchSet.Id psId2 = new PatchSet.Id(ctl.getId(), 2);
RevCommit commit2 = patchSetCommit(psId2);
String rev2 = commit2.name();
testRepo.branch(psId2.toRefName()).update(commit2);
testRepo.branch(ctl.getChange().getDest().get())
.update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev2)));
FixInput fix = new FixInput();
fix.expectMergedAs = rev2;
assertProblems(
ctl, fix,
problem(
"No patch set found for merged commit " + rev2,
FIXED, "Marked change as merged"),
problem(
"Expected merge commit " + rev2 + " corresponds to patch set 2,"
+ " not the current patch set 1",
FIXED, "Inserted as patch set 2"));
ctl = reload(ctl);
assertThat(ctl.getChange().currentPatchSetId()).isEqualTo(psId2);
assertThat(ctl.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
assertThat(psUtil.byChangeAsMap(db, ctl.getNotes()).keySet())
.containsExactly(ps1.getId(), psId2);
assertThat(psUtil.get(db, ctl.getNotes(), psId2).getRevision().get())
.isEqualTo(rev2);
}
@Test
@@ -683,8 +780,9 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
db, ins.getChange(), userFactory.create(adminId));
}
private PatchSet.Id nextPatchSetId(ChangeControl ctl) {
return ChangeUtil.nextPatchSetId(ctl.getChange().currentPatchSetId());
private PatchSet.Id nextPatchSetId(ChangeControl ctl) throws Exception {
return ChangeUtil.nextPatchSetId(
testRepo.getRepository(), ctl.getChange().currentPatchSetId());
}
private ChangeControl incrementPatchSet(ChangeControl ctl) throws Exception {

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

@@ -24,7 +24,6 @@ import static com.google.gerrit.server.ChangeUtil.TO_PS_ID;
import com.google.auto.value.AutoValue;
import com.google.common.base.Function;
import com.google.common.collect.Collections2;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Multimap;
@@ -48,6 +47,7 @@ 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.BatchUpdate.RepoContext;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.git.validators.CommitValidators;
@@ -72,6 +72,7 @@ import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -398,7 +399,7 @@ public class ConsistencyChecker {
return;
}
List<PatchSet.Id> psIds = new ArrayList<>();
List<PatchSet.Id> thisCommitPsIds = new ArrayList<>();
for (Ref ref : repo.getRefDatabase().getRefs(REFS_CHANGES).values()) {
if (!ref.getObjectId().equals(commit)) {
continue;
@@ -417,9 +418,9 @@ public class ConsistencyChecker {
warn(e);
// Include this patch set; should cause an error below, which is good.
}
psIds.add(psId);
thisCommitPsIds.add(psId);
}
switch (psIds.size()) {
switch (thisCommitPsIds.size()) {
case 0:
// No patch set for this commit; insert one.
rw.parseBody(commit);
@@ -432,28 +433,30 @@ public class ConsistencyChecker {
commit.name(), changeId, change().getKey().get()));
return;
}
insertMergedPatchSet(commit);
insertMergedPatchSet(commit, null, false);
break;
case 1:
// Existing patch set of this commit; check that it is the current
// patch set.
// TODO(dborowitz): This could be fixed if it's an older patch set of
// the current change.
PatchSet.Id id = psIds.get(0);
// Existing patch set ref pointing to this commit.
PatchSet.Id id = thisCommitPsIds.get(0);
if (id.equals(change().currentPatchSetId())) {
// If it's the current patch set, we can just fix the status.
fixMerged(wrongChangeStatus(id, commit));
} else if (id.get() > change().currentPatchSetId().get()) {
// If it's newer than the current patch set, reuse this patch set
// ID when inserting a new merged patch set.
insertMergedPatchSet(commit, id, true);
} 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()));
// If it's older than the current patch set, just delete the old
// ref, and use a new ID when inserting a new merged patch set.
insertMergedPatchSet(commit, id, false);
}
break;
default:
problem(String.format(
"Multiple patch sets for expected merged commit %s: %s",
commit.name(), intKeyOrdering().sortedCopy(psIds)));
commit.name(), intKeyOrdering().sortedCopy(thisCommitPsIds)));
break;
}
} catch (IOException e) {
@@ -462,27 +465,67 @@ public class ConsistencyChecker {
}
}
private void insertMergedPatchSet(RevCommit commit) {
ProblemInfo p =
private void insertMergedPatchSet(final RevCommit commit,
final @Nullable PatchSet.Id psIdToDelete, boolean reuseOldPsId) {
ProblemInfo notFound =
problem("No patch set found for merged commit " + commit.name());
if (!user.get().isIdentifiedUser()) {
p.status = Status.FIX_FAILED;
p.outcome =
notFound.status = Status.FIX_FAILED;
notFound.outcome =
"Must be called by an identified user to insert new patch set";
return;
}
ProblemInfo psp = problem(String.format(
"Expected merged commit %s has no associated patch set",
commit.name()));
ProblemInfo insertPatchSetProblem, deleteOldPatchSetProblem;
if (psIdToDelete == null) {
insertPatchSetProblem = problem(String.format(
"Expected merged commit %s has no associated patch set",
commit.name()));
deleteOldPatchSetProblem = null;
} else {
String msg = String.format(
"Expected merge commit %s corresponds to patch set %s,"
+ " not the current patch set %s",
commit.name(), psIdToDelete.get(),
change().currentPatchSetId().get());
// Maybe an identical problem, but different fix.
deleteOldPatchSetProblem = reuseOldPsId ? null : problem(msg);
insertPatchSetProblem = problem(msg);
}
List<ProblemInfo> currProblems = new ArrayList<>(3);
currProblems.add(notFound);
if (deleteOldPatchSetProblem != null) {
currProblems.add(insertPatchSetProblem);
}
currProblems.add(insertPatchSetProblem);
try {
PatchSet.Id psId =
ChangeUtil.nextPatchSetId(repo, change().currentPatchSetId());
PatchSet.Id psId = (psIdToDelete != null && reuseOldPsId)
? psIdToDelete
: ChangeUtil.nextPatchSetId(repo, change().currentPatchSetId());
PatchSetInserter inserter =
patchSetInserterFactory.create(ctl, psId, commit);
try (BatchUpdate bu = newBatchUpdate();
ObjectInserter oi = repo.newObjectInserter()) {
bu.setRepository(repo, rw, oi);
if (psIdToDelete != null) {
// Delete the given patch set ref. If reuseOldPsId is true,
// PatchSetInserter will reinsert the same ref, making it a no-op.
bu.addOp(ctl.getId(), new BatchUpdate.Op() {
@Override
public void updateRepo(RepoContext ctx) throws IOException {
ctx.addRefUpdate(new ReceiveCommand(
commit, ObjectId.zeroId(), psIdToDelete.toRefName()));
}
});
if (!reuseOldPsId) {
bu.addOp(ctl.getId(), new DeletePatchSetFromDbOp(
checkNotNull(deleteOldPatchSetProblem), psIdToDelete));
}
}
bu.addOp(ctl.getId(), inserter
.setValidatePolicy(CommitValidators.Policy.NONE)
.setFireRevisionCreated(false)
@@ -490,17 +533,17 @@ public class ConsistencyChecker {
.setAllowClosed(true)
.setMessage(
"Patch set for merged commit inserted by consistency checker"));
bu.addOp(ctl.getId(), new FixMergedOp(psp));
bu.addOp(ctl.getId(), new FixMergedOp(notFound));
bu.execute();
}
ctl = changeControlFactory.controlFor(
db.get(), inserter.getChange(), ctl.getUser());
p.status = Status.FIXED;
p.outcome = "Inserted as patch set " + psId.get();
insertPatchSetProblem.status = Status.FIXED;
insertPatchSetProblem.outcome = "Inserted as patch set " + psId.get();
} catch (OrmException | IOException | NoSuchChangeException
| UpdateException | RestApiException e) {
warn(e);
for (ProblemInfo pi : ImmutableList.of(p, psp)) {
for (ProblemInfo pi : currProblems) {
pi.status = Status.FIX_FAILED;
pi.outcome = "Error inserting merged patch set";
}
@@ -716,7 +759,7 @@ public class ConsistencyChecker {
private ProblemInfo problem(String msg) {
ProblemInfo p = new ProblemInfo();
p.message = msg;
p.message = checkNotNull(msg);
problems.add(p);
return p;
}