Fix external ID cache

The purpose of the external ID cache is to provide external IDs by account without need to reload all external IDs when one external ID is changed. To achieve this ExternalIdsUpdate informs the cache when an update to the external IDs is done so that the cache can apply the delta to the current value. The sequence in ExternalIdsUpdate is: 1. update external ID in NoteDb 2. inform the cache about the update The cache needs to know the old cache value in order to apply the delta. Instead of getting the old cache value it was wrongly reading the external IDs from the current revision of the NoteDb branch. Fix this by passing the old revision of the NoteDb branch to the cache and let the cache use this revision to get the old cache value. External IDs for the old revision are normally already loaded so that reloading the external IDs from NoteDb does not happen. Change-Id: Icb7277e726f77182939980d3365ffa92a93dcef4 Signed-off-by: Edwin Kempin <ekempin@google.com>
2017-03-29 12:41:50 +02:00
parent 384229ee9c
commit 560d3188f2
6 changed files with 165 additions and 69 deletions
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
@@ -39,6 +39,7 @@ import com.google.gerrit.server.account.externalids.ExternalId;
 import com.google.gerrit.server.account.externalids.ExternalIdReader;
 import com.google.gerrit.server.account.externalids.ExternalIds;
 import com.google.gerrit.server.account.externalids.ExternalIdsUpdate;
+import com.google.gerrit.server.account.externalids.ExternalIdsUpdate.RefsMetaExternalIdsUpdate;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.git.LockFailureException;
 import com.google.gson.reflect.TypeToken;
@@ -183,7 +184,7 @@ public class ExternalIdIT extends AbstractDaemonTest {

  @Test
  public void retryOnLockFailure() throws Exception {
-    Retryer<ObjectId> retryer =
+    Retryer<RefsMetaExternalIdsUpdate> retryer =
        ExternalIdsUpdate.retryerBuilder()
            .withBlockStrategy(
                new BlockStrategy() {
@@ -249,7 +250,7 @@ public class ExternalIdIT extends AbstractDaemonTest {
                // Ignore, the successful insertion of the external ID is asserted later
              }
            },
-            RetryerBuilder.<ObjectId>newBuilder()
+            RetryerBuilder.<RefsMetaExternalIdsUpdate>newBuilder()
                .retryIfException(e -> e instanceof LockFailureException)
                .withStopStrategy(StopStrategies.stopAfterAttempt(extIdsKeys.length))
                .build());
@@ -275,6 +276,31 @@ public class ExternalIdIT extends AbstractDaemonTest {
    assertThat(extId.accountId()).isEqualTo(accountId);
  }

+  @Test
+  @GerritConfig(name = "user.readExternalIdsFromGit", value = "true")
+  public void checkNoReloadAfterUpdate() throws Exception {
+    Set<ExternalId> expectedExtIds = new HashSet<>(externalIds.byAccount(db, admin.id));
+    externalIdReader.setFailOnLoad(true);
+
+    // insert external ID
+    ExternalId extId = ExternalId.create("foo", "bar", admin.id);
+    extIdsUpdate.create().insert(db, extId);
+    expectedExtIds.add(extId);
+    assertThat(externalIds.byAccount(db, admin.id)).containsExactlyElementsIn(expectedExtIds);
+
+    // update external ID
+    expectedExtIds.remove(extId);
+    extId = ExternalId.createWithEmail("foo", "bar", admin.id, "foo.bar@example.com");
+    extIdsUpdate.create().upsert(db, extId);
+    expectedExtIds.add(extId);
+    assertThat(externalIds.byAccount(db, admin.id)).containsExactlyElementsIn(expectedExtIds);
+
+    // delete external ID
+    extIdsUpdate.create().delete(db, extId);
+    expectedExtIds.remove(extId);
+    assertThat(externalIds.byAccount(db, admin.id)).containsExactlyElementsIn(expectedExtIds);
+  }
+
  @Test
  @GerritConfig(name = "user.readExternalIdsFromGit", value = "true")
  public void byAccountFailIfReadingExternalIdsFails() throws Exception {
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/DisabledExternalIdCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/DisabledExternalIdCache.java
@@ -34,13 +34,14 @@ public class DisabledExternalIdCache implements ExternalIdCache {
  }

  @Override
-  public void onCreate(ObjectId newNotesRev, Collection<ExternalId> extId) {}
+  public void onCreate(ObjectId oldNotesRev, ObjectId newNotesRev, Collection<ExternalId> extId) {}

  @Override
-  public void onUpdate(ObjectId newNotesRev, Collection<ExternalId> extId) {}
+  public void onUpdate(ObjectId oldNotesRev, ObjectId newNotesRev, Collection<ExternalId> extId) {}

  @Override
  public void onReplace(
+      ObjectId oldNotesRev,
      ObjectId newNotesRev,
      Account.Id accountId,
      Collection<ExternalId> toRemove,
@@ -48,6 +49,7 @@ public class DisabledExternalIdCache implements ExternalIdCache {

  @Override
  public void onReplaceByKeys(
+      ObjectId oldNotesRev,
      ObjectId newNotesRev,
      Account.Id accountId,
      Collection<ExternalId.Key> toRemove,
@@ -55,21 +57,31 @@ public class DisabledExternalIdCache implements ExternalIdCache {

  @Override
  public void onReplaceByKeys(
-      ObjectId newNotesRev, Collection<ExternalId.Key> toRemove, Collection<ExternalId> toAdd) {}
+      ObjectId oldNotesRev,
+      ObjectId newNotesRev,
+      Collection<ExternalId.Key> toRemove,
+      Collection<ExternalId> toAdd) {}

  @Override
  public void onReplace(
-      ObjectId newNotesRev, Collection<ExternalId> toRemove, Collection<ExternalId> toAdd) {}
+      ObjectId oldNotesRev,
+      ObjectId newNotesRev,
+      Collection<ExternalId> toRemove,
+      Collection<ExternalId> toAdd) {}

  @Override
-  public void onRemove(ObjectId newNotesRev, Collection<ExternalId> extId) {}
+  public void onRemove(ObjectId oldNotesRev, ObjectId newNotesRev, Collection<ExternalId> extId) {}

  @Override
  public void onRemoveByKeys(
-      ObjectId newNotesRev, Account.Id accountId, Collection<ExternalId.Key> extIdKeys) {}
+      ObjectId oldNotesRev,
+      ObjectId newNotesRev,
+      Account.Id accountId,
+      Collection<ExternalId.Key> extIdKeys) {}

  @Override
-  public void onRemoveByKeys(ObjectId newNotesRev, Collection<ExternalId.Key> extIdKeys) {}
+  public void onRemoveByKeys(
+      ObjectId oldNotesRev, ObjectId newNotesRev, Collection<ExternalId.Key> extIdKeys) {}

  @Override
  public Set<ExternalId> byAccount(Account.Id accountId) {
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
@@ -23,11 +23,14 @@ import org.eclipse.jgit.lib.ObjectId;

 /** Caches external IDs of all accounts */
 interface ExternalIdCache {
-  void onCreate(ObjectId newNotesRev, Collection<ExternalId> extId) throws IOException;
+  void onCreate(ObjectId oldNotesRev, ObjectId newNotesRev, Collection<ExternalId> extId)
+      throws IOException;

-  void onUpdate(ObjectId newNotesRev, Collection<ExternalId> extId) throws IOException;
+  void onUpdate(ObjectId oldNotesRev, ObjectId newNotesRev, Collection<ExternalId> extId)
+      throws IOException;

  void onReplace(
+      ObjectId oldNotesRev,
      ObjectId newNotesRev,
      Account.Id accountId,
      Collection<ExternalId> toRemove,
@@ -35,6 +38,7 @@ interface ExternalIdCache {
      throws IOException;

  void onReplaceByKeys(
+      ObjectId oldNotesRev,
      ObjectId newNotesRev,
      Account.Id accountId,
      Collection<ExternalId.Key> toRemove,
@@ -42,40 +46,55 @@ interface ExternalIdCache {
      throws IOException;

  void onReplaceByKeys(
-      ObjectId newNotesRev, Collection<ExternalId.Key> toRemove, Collection<ExternalId> toAdd)
+      ObjectId oldNotesRev,
+      ObjectId newNotesRev,
+      Collection<ExternalId.Key> toRemove,
+      Collection<ExternalId> toAdd)
      throws IOException;

  void onReplace(
-      ObjectId newNotesRev, Collection<ExternalId> toRemove, Collection<ExternalId> toAdd)
+      ObjectId oldNotesRev,
+      ObjectId newNotesRev,
+      Collection<ExternalId> toRemove,
+      Collection<ExternalId> toAdd)
      throws IOException;

-  void onRemove(ObjectId newNotesRev, Collection<ExternalId> extId) throws IOException;
+  void onRemove(ObjectId oldNotesRev, ObjectId newNotesRev, Collection<ExternalId> extId)
+      throws IOException;

  void onRemoveByKeys(
-      ObjectId newNotesRev, Account.Id accountId, Collection<ExternalId.Key> extIdKeys)
+      ObjectId oldNotesRev,
+      ObjectId newNotesRev,
+      Account.Id accountId,
+      Collection<ExternalId.Key> extIdKeys)
      throws IOException;

-  void onRemoveByKeys(ObjectId newNotesRev, Collection<ExternalId.Key> extIdKeys)
+  void onRemoveByKeys(
+      ObjectId oldNotesRev, ObjectId newNotesRev, Collection<ExternalId.Key> extIdKeys)
      throws IOException;

  Set<ExternalId> byAccount(Account.Id accountId) throws IOException;

  Set<ExternalId> byEmail(String email) throws IOException;

-  default void onCreate(ObjectId newNotesRev, ExternalId extId) throws IOException {
-    onCreate(newNotesRev, Collections.singleton(extId));
-  }
-
-  default void onRemove(ObjectId newNotesRev, ExternalId extId) throws IOException {
-    onRemove(newNotesRev, Collections.singleton(extId));
-  }
-
-  default void onRemoveByKey(ObjectId newNotesRev, Account.Id accountId, ExternalId.Key extIdKey)
+  default void onCreate(ObjectId oldNotesRev, ObjectId newNotesRev, ExternalId extId)
      throws IOException {
-    onRemoveByKeys(newNotesRev, accountId, Collections.singleton(extIdKey));
+    onCreate(oldNotesRev, newNotesRev, Collections.singleton(extId));
  }

-  default void onUpdate(ObjectId newNotesRev, ExternalId updatedExtId) throws IOException {
-    onUpdate(newNotesRev, Collections.singleton(updatedExtId));
+  default void onRemove(ObjectId oldNotesRev, ObjectId newNotesRev, ExternalId extId)
+      throws IOException {
+    onRemove(oldNotesRev, newNotesRev, Collections.singleton(extId));
+  }
+
+  default void onRemoveByKey(
+      ObjectId oldNotesRev, ObjectId newNotesRev, Account.Id accountId, ExternalId.Key extIdKey)
+      throws IOException {
+    onRemoveByKeys(oldNotesRev, newNotesRev, accountId, Collections.singleton(extIdKey));
+  }
+
+  default void onUpdate(ObjectId oldNotesRev, ObjectId newNotesRev, ExternalId updatedExtId)
+      throws IOException {
+    onUpdate(oldNotesRev, newNotesRev, Collections.singleton(updatedExtId));
  }
 }
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java
@@ -62,8 +62,10 @@ class ExternalIdCacheImpl implements ExternalIdCache {
  }

  @Override
-  public void onCreate(ObjectId newNotesRev, Collection<ExternalId> extIds) throws IOException {
+  public void onCreate(ObjectId oldNotesRev, ObjectId newNotesRev, Collection<ExternalId> extIds)
+      throws IOException {
    updateCache(
+        oldNotesRev,
        newNotesRev,
        m -> {
          for (ExternalId extId : extIds) {
@@ -73,8 +75,10 @@ class ExternalIdCacheImpl implements ExternalIdCache {
  }

  @Override
-  public void onRemove(ObjectId newNotesRev, Collection<ExternalId> extIds) throws IOException {
+  public void onRemove(ObjectId oldNotesRev, ObjectId newNotesRev, Collection<ExternalId> extIds)
+      throws IOException {
    updateCache(
+        oldNotesRev,
        newNotesRev,
        m -> {
          for (ExternalId extId : extIds) {
@@ -85,21 +89,27 @@ class ExternalIdCacheImpl implements ExternalIdCache {

  @Override
  public void onRemoveByKeys(
-      ObjectId newNotesRev, Account.Id accountId, Collection<ExternalId.Key> extIdKeys)
+      ObjectId oldNotesRev,
+      ObjectId newNotesRev,
+      Account.Id accountId,
+      Collection<ExternalId.Key> extIdKeys)
      throws IOException {
-    updateCache(newNotesRev, m -> removeKeys(m.get(accountId), extIdKeys));
+    updateCache(oldNotesRev, newNotesRev, m -> removeKeys(m.get(accountId), extIdKeys));
  }

  @Override
-  public void onRemoveByKeys(ObjectId newNotesRev, Collection<ExternalId.Key> extIdKeys)
+  public void onRemoveByKeys(
+      ObjectId oldNotesRev, ObjectId newNotesRev, Collection<ExternalId.Key> extIdKeys)
      throws IOException {
-    updateCache(newNotesRev, m -> removeKeys(m.values(), extIdKeys));
+    updateCache(oldNotesRev, newNotesRev, m -> removeKeys(m.values(), extIdKeys));
  }

  @Override
-  public void onUpdate(ObjectId newNotesRev, Collection<ExternalId> updatedExtIds)
+  public void onUpdate(
+      ObjectId oldNotesRev, ObjectId newNotesRev, Collection<ExternalId> updatedExtIds)
      throws IOException {
    updateCache(
+        oldNotesRev,
        newNotesRev,
        m -> {
          removeKeys(m.values(), updatedExtIds.stream().map(e -> e.key()).collect(toSet()));
@@ -111,6 +121,7 @@ class ExternalIdCacheImpl implements ExternalIdCache {

  @Override
  public void onReplace(
+      ObjectId oldNotesRev,
      ObjectId newNotesRev,
      Account.Id accountId,
      Collection<ExternalId> toRemove,
@@ -119,6 +130,7 @@ class ExternalIdCacheImpl implements ExternalIdCache {
    ExternalIdsUpdate.checkSameAccount(Iterables.concat(toRemove, toAdd), accountId);

    updateCache(
+        oldNotesRev,
        newNotesRev,
        m -> {
          for (ExternalId extId : toRemove) {
@@ -132,6 +144,7 @@ class ExternalIdCacheImpl implements ExternalIdCache {

  @Override
  public void onReplaceByKeys(
+      ObjectId oldNotesRev,
      ObjectId newNotesRev,
      Account.Id accountId,
      Collection<ExternalId.Key> toRemove,
@@ -140,6 +153,7 @@ class ExternalIdCacheImpl implements ExternalIdCache {
    ExternalIdsUpdate.checkSameAccount(toAdd, accountId);

    updateCache(
+        oldNotesRev,
        newNotesRev,
        m -> {
          removeKeys(m.get(accountId), toRemove);
@@ -151,9 +165,13 @@ class ExternalIdCacheImpl implements ExternalIdCache {

  @Override
  public void onReplaceByKeys(
-      ObjectId newNotesRev, Collection<ExternalId.Key> toRemove, Collection<ExternalId> toAdd)
+      ObjectId oldNotesRev,
+      ObjectId newNotesRev,
+      Collection<ExternalId.Key> toRemove,
+      Collection<ExternalId> toAdd)
      throws IOException {
    updateCache(
+        oldNotesRev,
        newNotesRev,
        m -> {
          removeKeys(m.values(), toRemove);
@@ -165,9 +183,13 @@ class ExternalIdCacheImpl implements ExternalIdCache {

  @Override
  public void onReplace(
-      ObjectId newNotesRev, Collection<ExternalId> toRemove, Collection<ExternalId> toAdd)
+      ObjectId oldNotesRev,
+      ObjectId newNotesRev,
+      Collection<ExternalId> toRemove,
+      Collection<ExternalId> toAdd)
      throws IOException {
    updateCache(
+        oldNotesRev,
        newNotesRev,
        m -> {
          for (ExternalId extId : toRemove) {
@@ -202,14 +224,18 @@ class ExternalIdCacheImpl implements ExternalIdCache {
    }
  }

-  private void updateCache(ObjectId newNotesRev, Consumer<Multimap<Account.Id, ExternalId>> update)
-      throws IOException {
+  private void updateCache(
+      ObjectId oldNotesRev,
+      ObjectId newNotesRev,
+      Consumer<Multimap<Account.Id, ExternalId>> update) {
    lock.lock();
    try {
-      ListMultimap<Account.Id, ExternalId> m =
-          MultimapBuilder.hashKeys()
-              .arrayListValues()
-              .build(extIdsByAccount.get(externalIdReader.readRevision()));
+      ListMultimap<Account.Id, ExternalId> m;
+      if (!ObjectId.zeroId().equals(oldNotesRev)) {
+        m = MultimapBuilder.hashKeys().arrayListValues().build(extIdsByAccount.get(oldNotesRev));
+      } else {
+        m = MultimapBuilder.hashKeys().arrayListValues().build();
+      }
      update.accept(m);
      extIdsByAccount.put(newNotesRev, ImmutableSetMultimap.copyOf(m));
    } catch (ExecutionException e) {
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdsBatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdsBatchUpdate.java
@@ -112,7 +112,7 @@ public class ExternalIdsBatchUpdate {
      ObjectId newRev =
          ExternalIdsUpdate.commit(
              repo, rw, ins, rev, noteMap, commitMessage, serverIdent, serverIdent);
-      externalIdCache.onReplace(newRev, toDelete, toAdd);
+      externalIdCache.onReplace(rev, newRev, toDelete, toAdd);
    }

    toAdd.clear();
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdsUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdsUpdate.java
@@ -170,8 +170,8 @@ public class ExternalIdsUpdate {
  }

  @VisibleForTesting
-  public static RetryerBuilder<ObjectId> retryerBuilder() {
-    return RetryerBuilder.<ObjectId>newBuilder()
+  public static RetryerBuilder<RefsMetaExternalIdsUpdate> retryerBuilder() {
+    return RetryerBuilder.<RefsMetaExternalIdsUpdate>newBuilder()
        .retryIfException(e -> e instanceof LockFailureException)
        .withWaitStrategy(
            WaitStrategies.join(
@@ -180,7 +180,7 @@ public class ExternalIdsUpdate {
        .withStopStrategy(StopStrategies.stopAfterDelay(10, TimeUnit.SECONDS));
  }

-  private static final Retryer<ObjectId> RETRYER = retryerBuilder().build();
+  private static final Retryer<RefsMetaExternalIdsUpdate> RETRYER = retryerBuilder().build();

  private final GitRepositoryManager repoManager;
  private final AllUsersName allUsersName;
@@ -189,7 +189,7 @@ public class ExternalIdsUpdate {
  private final PersonIdent committerIdent;
  private final PersonIdent authorIdent;
  private final Runnable afterReadRevision;
-  private final Retryer<ObjectId> retryer;
+  private final Retryer<RefsMetaExternalIdsUpdate> retryer;

  private ExternalIdsUpdate(
      GitRepositoryManager repoManager,
@@ -218,7 +218,7 @@ public class ExternalIdsUpdate {
      PersonIdent committerIdent,
      PersonIdent authorIdent,
      Runnable afterReadRevision,
-      Retryer<ObjectId> retryer) {
+      Retryer<RefsMetaExternalIdsUpdate> retryer) {
    this.repoManager = checkNotNull(repoManager, "repoManager");
    this.allUsersName = checkNotNull(allUsersName, "allUsersName");
    this.committerIdent = checkNotNull(committerIdent, "committerIdent");
@@ -249,14 +249,14 @@ public class ExternalIdsUpdate {
      throws IOException, ConfigInvalidException, OrmException {
    db.accountExternalIds().insert(toAccountExternalIds(extIds));

-    ObjectId newRev =
+    RefsMetaExternalIdsUpdate u =
        updateNoteMap(
            o -> {
              for (ExternalId extId : extIds) {
                insert(o.rw(), o.ins(), o.noteMap(), extId);
              }
            });
-    externalIdCache.onCreate(newRev, extIds);
+    externalIdCache.onCreate(u.oldRev(), u.newRev(), extIds);
  }

  /**
@@ -278,14 +278,14 @@ public class ExternalIdsUpdate {
      throws IOException, ConfigInvalidException, OrmException {
    db.accountExternalIds().upsert(toAccountExternalIds(extIds));

-    ObjectId newRev =
+    RefsMetaExternalIdsUpdate u =
        updateNoteMap(
            o -> {
              for (ExternalId extId : extIds) {
                upsert(o.rw(), o.ins(), o.noteMap(), extId);
              }
            });
-    externalIdCache.onUpdate(newRev, extIds);
+    externalIdCache.onUpdate(u.oldRev(), u.newRev(), extIds);
  }

  /**
@@ -310,14 +310,14 @@ public class ExternalIdsUpdate {
      throws IOException, ConfigInvalidException, OrmException {
    db.accountExternalIds().delete(toAccountExternalIds(extIds));

-    ObjectId newRev =
+    RefsMetaExternalIdsUpdate u =
        updateNoteMap(
            o -> {
              for (ExternalId extId : extIds) {
                remove(o.rw(), o.noteMap(), extId);
              }
            });
-    externalIdCache.onRemove(newRev, extIds);
+    externalIdCache.onRemove(u.oldRev(), u.newRev(), extIds);
  }

  /**
@@ -341,14 +341,14 @@ public class ExternalIdsUpdate {
      throws IOException, ConfigInvalidException, OrmException {
    db.accountExternalIds().deleteKeys(toAccountExternalIdKeys(extIdKeys));

-    ObjectId newRev =
+    RefsMetaExternalIdsUpdate u =
        updateNoteMap(
            o -> {
              for (ExternalId.Key extIdKey : extIdKeys) {
                remove(o.rw(), o.noteMap(), extIdKey, accountId);
              }
            });
-    externalIdCache.onRemoveByKeys(newRev, accountId, extIdKeys);
+    externalIdCache.onRemoveByKeys(u.oldRev(), u.newRev(), accountId, extIdKeys);
  }

  /**
@@ -360,14 +360,14 @@ public class ExternalIdsUpdate {
      throws IOException, ConfigInvalidException, OrmException {
    db.accountExternalIds().deleteKeys(toAccountExternalIdKeys(extIdKeys));

-    ObjectId newRev =
+    RefsMetaExternalIdsUpdate u =
        updateNoteMap(
            o -> {
              for (ExternalId.Key extIdKey : extIdKeys) {
                remove(o.rw(), o.noteMap(), extIdKey, null);
              }
            });
-    externalIdCache.onRemoveByKeys(newRev, extIdKeys);
+    externalIdCache.onRemoveByKeys(u.oldRev(), u.newRev(), extIdKeys);
  }

  /** Deletes all external IDs of the specified account. */
@@ -398,7 +398,7 @@ public class ExternalIdsUpdate {
    db.accountExternalIds().deleteKeys(toAccountExternalIdKeys(toDelete));
    db.accountExternalIds().insert(toAccountExternalIds(toAdd));

-    ObjectId newRev =
+    RefsMetaExternalIdsUpdate u =
        updateNoteMap(
            o -> {
              for (ExternalId.Key extIdKey : toDelete) {
@@ -409,7 +409,7 @@ public class ExternalIdsUpdate {
                insert(o.rw(), o.ins(), o.noteMap(), extId);
              }
            });
-    externalIdCache.onReplaceByKeys(newRev, accountId, toDelete, toAdd);
+    externalIdCache.onReplaceByKeys(u.oldRev(), u.newRev(), accountId, toDelete, toAdd);
  }

  /**
@@ -428,7 +428,7 @@ public class ExternalIdsUpdate {
    db.accountExternalIds().deleteKeys(toAccountExternalIdKeys(toDelete));
    db.accountExternalIds().insert(toAccountExternalIds(toAdd));

-    ObjectId newRev =
+    RefsMetaExternalIdsUpdate u =
        updateNoteMap(
            o -> {
              for (ExternalId.Key extIdKey : toDelete) {
@@ -439,7 +439,7 @@ public class ExternalIdsUpdate {
                insert(o.rw(), o.ins(), o.noteMap(), extId);
              }
            });
-    externalIdCache.onReplaceByKeys(newRev, toDelete, toAdd);
+    externalIdCache.onReplaceByKeys(u.oldRev(), u.newRev(), toDelete, toAdd);
  }

  /**
@@ -599,7 +599,7 @@ public class ExternalIdsUpdate {
    noteMap.remove(noteId);
  }

-  private ObjectId updateNoteMap(MyConsumer<OpenRepo> update)
+  private RefsMetaExternalIdsUpdate updateNoteMap(MyConsumer<OpenRepo> update)
      throws IOException, ConfigInvalidException, OrmException {
    try (Repository repo = repoManager.openRepository(allUsersName);
        RevWalk rw = new RevWalk(repo);
@@ -615,10 +615,11 @@ public class ExternalIdsUpdate {
    }
  }

-  private ObjectId commit(
+  private RefsMetaExternalIdsUpdate commit(
      Repository repo, RevWalk rw, ObjectInserter ins, ObjectId rev, NoteMap noteMap)
      throws IOException {
-    return commit(repo, rw, ins, rev, noteMap, COMMIT_MSG, committerIdent, authorIdent);
+    ObjectId newRev = commit(repo, rw, ins, rev, noteMap, COMMIT_MSG, committerIdent, authorIdent);
+    return RefsMetaExternalIdsUpdate.create(rev, newRev);
  }

  /** Commits updates to the external IDs. */
@@ -675,7 +676,7 @@ public class ExternalIdsUpdate {
      default:
        throw new IOException("Updating external IDs failed with " + res);
    }
-    return commitId;
+    return rw.parseCommit(commitId);
  }

  private static ObjectId emptyTree(ObjectInserter ins) throws IOException {
@@ -702,7 +703,19 @@ public class ExternalIdsUpdate {
    abstract NoteMap noteMap();
  }

-  private class TryNoteMapUpdate implements Callable<ObjectId> {
+  @VisibleForTesting
+  @AutoValue
+  public abstract static class RefsMetaExternalIdsUpdate {
+    static RefsMetaExternalIdsUpdate create(ObjectId oldRev, ObjectId newRev) {
+      return new AutoValue_ExternalIdsUpdate_RefsMetaExternalIdsUpdate(oldRev, newRev);
+    }
+
+    abstract ObjectId oldRev();
+
+    abstract ObjectId newRev();
+  }
+
+  private class TryNoteMapUpdate implements Callable<RefsMetaExternalIdsUpdate> {
    private final Repository repo;
    private final RevWalk rw;
    private final ObjectInserter ins;
@@ -717,7 +730,7 @@ public class ExternalIdsUpdate {
    }

    @Override
-    public ObjectId call() throws Exception {
+    public RefsMetaExternalIdsUpdate call() throws Exception {
      ObjectId rev = readRevision(repo);

      afterReadRevision.run();