Retry account updates on LockFailureException

If an account update fails with LockFailureException (because the same
account was concurrently updated by another thread) we now retry the
account update.

In future AccountsUpdate should allow to update an account and its
external IDs atomically. For external ID updates there is a higher
chance of lock failures since the external IDs of all users are stored
in a single notes branch. Hence when AccountsUpdates takes to also
update external IDs it gets important to retry account updates that
failed due to lock failure. So this change is a preparation to support
updating accounts and external IDs atomically in future.

Change-Id: I2bc6b4cceb111855f48d02a68690003992b3dc53
Signed-off-by: Edwin Kempin <ekempin@google.com>

java/com/google/gerrit/server/account/AccountsUpdate.java

@@ -16,7 +16,9 @@ package com.google.gerrit.server.account;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.Lists;
+import com.google.common.util.concurrent.Runnables;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Project;
@@ -29,7 +31,9 @@ import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.index.change.ReindexAfterRefUpdate;
 import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
+import com.google.gerrit.server.update.RetryHelper;
 import com.google.gwtorm.server.OrmDuplicateKeyException;
+import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
@@ -109,6 +113,7 @@ public class AccountsUpdate {
     private final OutgoingEmailValidator emailValidator;
     private final Provider<PersonIdent> serverIdent;
     private final Provider<MetaDataUpdate.Server> metaDataUpdateServerFactory;
+    private final RetryHelper retryHelper;
 
     @Inject
     public Server(
@@ -117,13 +122,15 @@ public class AccountsUpdate {
         AllUsersName allUsersName,
         OutgoingEmailValidator emailValidator,
         @GerritPersonIdent Provider<PersonIdent> serverIdent,
-        Provider<MetaDataUpdate.Server> metaDataUpdateServerFactory) {
+        Provider<MetaDataUpdate.Server> metaDataUpdateServerFactory,
+        RetryHelper retryHelper) {
       this.repoManager = repoManager;
       this.gitRefUpdated = gitRefUpdated;
       this.allUsersName = allUsersName;
       this.emailValidator = emailValidator;
       this.serverIdent = serverIdent;
       this.metaDataUpdateServerFactory = metaDataUpdateServerFactory;
+      this.retryHelper = retryHelper;
     }
 
     public AccountsUpdate create() {
@@ -135,7 +142,8 @@ public class AccountsUpdate {
           allUsersName,
           emailValidator,
           i,
-          () -> metaDataUpdateServerFactory.get().create(allUsersName));
+          () -> metaDataUpdateServerFactory.get().create(allUsersName),
+          retryHelper);
     }
   }
 
@@ -154,6 +162,7 @@ public class AccountsUpdate {
     private final Provider<PersonIdent> serverIdent;
     private final Provider<IdentifiedUser> identifiedUser;
     private final Provider<MetaDataUpdate.User> metaDataUpdateUserFactory;
+    private final RetryHelper retryHelper;
 
     @Inject
     public User(
@@ -163,7 +172,8 @@ public class AccountsUpdate {
         OutgoingEmailValidator emailValidator,
         @GerritPersonIdent Provider<PersonIdent> serverIdent,
         Provider<IdentifiedUser> identifiedUser,
-        Provider<MetaDataUpdate.User> metaDataUpdateUserFactory) {
+        Provider<MetaDataUpdate.User> metaDataUpdateUserFactory,
+        RetryHelper retryHelper) {
       this.repoManager = repoManager;
       this.gitRefUpdated = gitRefUpdated;
       this.allUsersName = allUsersName;
@@ -171,6 +181,7 @@ public class AccountsUpdate {
       this.emailValidator = emailValidator;
       this.identifiedUser = identifiedUser;
       this.metaDataUpdateUserFactory = metaDataUpdateUserFactory;
+      this.retryHelper = retryHelper;
     }
 
     public AccountsUpdate create() {
@@ -183,7 +194,8 @@ public class AccountsUpdate {
           allUsersName,
           emailValidator,
           createPersonIdent(i, user),
-          () -> metaDataUpdateUserFactory.get().create(allUsersName));
+          () -> metaDataUpdateUserFactory.get().create(allUsersName),
+          retryHelper);
     }
 
     private PersonIdent createPersonIdent(PersonIdent ident, IdentifiedUser user) {
@@ -198,6 +210,8 @@ public class AccountsUpdate {
   private final OutgoingEmailValidator emailValidator;
   private final PersonIdent committerIdent;
   private final MetaDataUpdateFactory metaDataUpdateFactory;
+  private final RetryHelper retryHelper;
+  private final Runnable afterReadRevision;
 
   private AccountsUpdate(
       GitRepositoryManager repoManager,
@@ -206,7 +220,31 @@ public class AccountsUpdate {
       AllUsersName allUsersName,
       OutgoingEmailValidator emailValidator,
       PersonIdent committerIdent,
-      MetaDataUpdateFactory metaDataUpdateFactory) {
+      MetaDataUpdateFactory metaDataUpdateFactory,
+      RetryHelper retryHelper) {
+    this(
+        repoManager,
+        gitRefUpdated,
+        currentUser,
+        allUsersName,
+        emailValidator,
+        committerIdent,
+        metaDataUpdateFactory,
+        retryHelper,
+        Runnables.doNothing());
+  }
+
+  @VisibleForTesting
+  public AccountsUpdate(
+      GitRepositoryManager repoManager,
+      GitReferenceUpdated gitRefUpdated,
+      @Nullable IdentifiedUser currentUser,
+      AllUsersName allUsersName,
+      OutgoingEmailValidator emailValidator,
+      PersonIdent committerIdent,
+      MetaDataUpdateFactory metaDataUpdateFactory,
+      RetryHelper retryHelper,
+      Runnable afterReadRevision) {
     this.repoManager = checkNotNull(repoManager, "repoManager");
     this.gitRefUpdated = checkNotNull(gitRefUpdated, "gitRefUpdated");
     this.currentUser = currentUser;
@@ -214,6 +252,8 @@ public class AccountsUpdate {
     this.emailValidator = checkNotNull(emailValidator, "emailValidator");
     this.committerIdent = checkNotNull(committerIdent, "committerIdent");
     this.metaDataUpdateFactory = checkNotNull(metaDataUpdateFactory, "metaDataUpdateFactory");
+    this.retryHelper = checkNotNull(retryHelper, "retryHelper");
+    this.afterReadRevision = afterReadRevision;
   }
 
   /**
@@ -223,11 +263,12 @@ public class AccountsUpdate {
    * @param init consumer to populate the new account
    * @return the newly created account
    * @throws OrmDuplicateKeyException if the account already exists
-   * @throws IOException if updating the user branch fails
+   * @throws IOException if creating the user branch fails due to an IO error
+   * @throws OrmException if creating the user branch fails
    * @throws ConfigInvalidException if any of the account fields has an invalid value
    */
   public Account insert(Account.Id accountId, Consumer<InternalAccountUpdate.Builder> init)
-      throws OrmDuplicateKeyException, IOException, ConfigInvalidException {
+      throws OrmException, IOException, ConfigInvalidException {
     return insert(accountId, AccountUpdater.fromConsumer(init));
   }
 
@@ -238,18 +279,24 @@ public class AccountsUpdate {
    * @param updater updater to populate the new account
    * @return the newly created account
    * @throws OrmDuplicateKeyException if the account already exists
-   * @throws IOException if updating the user branch fails
+   * @throws IOException if creating the user branch fails due to an IO error
+   * @throws OrmException if creating the user branch fails
    * @throws ConfigInvalidException if any of the account fields has an invalid value
    */
   public Account insert(Account.Id accountId, AccountUpdater updater)
-      throws OrmDuplicateKeyException, IOException, ConfigInvalidException {
+      throws OrmException, IOException, ConfigInvalidException {
+    return updateAccount(
+        () -> {
           AccountConfig accountConfig = read(accountId);
           Account account = accountConfig.getNewAccount();
           InternalAccountUpdate.Builder updateBuilder = InternalAccountUpdate.builder();
           updater.update(account, updateBuilder);
           accountConfig.setAccountUpdate(updateBuilder.build());
-    commitNew(accountConfig);
-    return accountConfig.getLoadedAccount().get();
+
+          UpdatedAccount updatedAccounts = new UpdatedAccount(accountConfig);
+          updatedAccounts.setCreated(true);
+          return updatedAccounts;
+        });
   }
 
   /**
@@ -260,11 +307,12 @@ public class AccountsUpdate {
    * @param accountId ID of the account
    * @param update consumer to update the account, only invoked if the account exists
    * @return the updated account, {@code null} if the account doesn't exist
-   * @throws IOException if updating the user branch fails
+   * @throws IOException if updating the user branch fails due to an IO error
+   * @throws OrmException if updating the user branch fails
    * @throws ConfigInvalidException if any of the account fields has an invalid value
    */
   public Account update(Account.Id accountId, Consumer<InternalAccountUpdate.Builder> update)
-      throws IOException, ConfigInvalidException {
+      throws OrmException, IOException, ConfigInvalidException {
     return update(accountId, AccountUpdater.fromConsumer(update));
   }
 
@@ -276,12 +324,15 @@ public class AccountsUpdate {
    * @param accountId ID of the account
    * @param updater updater to update the account, only invoked if the account exists
    * @return the updated account, {@code null} if the account doesn't exist
-   * @throws IOException if updating the user branch fails
+   * @throws IOException if updating the user branch fails due to an IO error
+   * @throws OrmException if updating the user branch fails
    * @throws ConfigInvalidException if any of the account fields has an invalid value
    */
   @Nullable
   public Account update(Account.Id accountId, AccountUpdater updater)
-      throws IOException, ConfigInvalidException {
+      throws OrmException, IOException, ConfigInvalidException {
+    return updateAccount(
+        () -> {
           AccountConfig accountConfig = read(accountId);
           Optional<Account> account = accountConfig.getLoadedAccount();
           if (!account.isPresent()) {
@@ -291,17 +342,20 @@ public class AccountsUpdate {
           InternalAccountUpdate.Builder updateBuilder = InternalAccountUpdate.builder();
           updater.update(account.get(), updateBuilder);
           accountConfig.setAccountUpdate(updateBuilder.build());
-    commit(accountConfig);
-    return accountConfig.getLoadedAccount().orElse(null);
+          UpdatedAccount updatedAccounts = new UpdatedAccount(accountConfig);
+          return updatedAccounts;
+        });
   }
 
   /**
    * Deletes the account.
    *
    * @param account the account that should be deleted
-   * @throws IOException if updating the user branch fails
+   * @throws IOException if deleting the user branch fails due to an IO error
+   * @throws OrmException if deleting the user branch fails
+   * @throws ConfigInvalidException
    */
-  public void delete(Account account) throws IOException {
+  public void delete(Account account) throws IOException, OrmException, ConfigInvalidException {
     deleteByKey(account.getId());
   }
 
@@ -309,10 +363,22 @@ public class AccountsUpdate {
    * Deletes the account.
    *
    * @param accountId the ID of the account that should be deleted
-   * @throws IOException if updating the user branch fails
+   * @throws IOException if deleting the user branch fails due to an IO error
+   * @throws OrmException if deleting the user branch fails
+   * @throws ConfigInvalidException
    */
-  public void deleteByKey(Account.Id accountId) throws IOException {
+  public void deleteByKey(Account.Id accountId)
+      throws IOException, OrmException, ConfigInvalidException {
+    deleteAccount(accountId);
+  }
+
+  private Account deleteAccount(Account.Id accountId)
+      throws IOException, OrmException, ConfigInvalidException {
+    return retryHelper.execute(
+        () -> {
           deleteUserBranch(accountId);
+          return null;
+        });
   }
 
   private void deleteUserBranch(Account.Id accountId) throws IOException {
@@ -352,10 +418,35 @@ public class AccountsUpdate {
     try (Repository repo = repoManager.openRepository(allUsersName)) {
       AccountConfig accountConfig = new AccountConfig(emailValidator, accountId);
       accountConfig.load(repo);
+
+      afterReadRevision.run();
+
       return accountConfig;
     }
   }
 
+  private Account updateAccount(AccountUpdate accountUpdate)
+      throws IOException, ConfigInvalidException, OrmException {
+    return retryHelper.execute(
+        () -> {
+          UpdatedAccount updatedAccount = accountUpdate.update();
+          if (updatedAccount == null) {
+            return null;
+          }
+
+          commit(updatedAccount);
+          return updatedAccount.getAccount();
+        });
+  }
+
+  private void commit(UpdatedAccount updatedAccount) throws IOException {
+    if (updatedAccount.isCreated()) {
+      commitNew(updatedAccount.getAccountConfig());
+    } else {
+      commit(updatedAccount.getAccountConfig());
+    }
+  }
+
   private void commitNew(AccountConfig accountConfig) throws IOException {
     // When creating a new account we must allow empty commits so that the user branch gets created
     // with an empty commit when no account properties are set and hence no 'account.config' file
@@ -374,8 +465,39 @@ public class AccountsUpdate {
     }
   }
 
+  @VisibleForTesting
   @FunctionalInterface
-  private static interface MetaDataUpdateFactory {
+  public static interface MetaDataUpdateFactory {
     MetaDataUpdate create() throws IOException;
   }
+
+  @FunctionalInterface
+  private static interface AccountUpdate {
+    UpdatedAccount update() throws IOException, ConfigInvalidException, OrmException;
+  }
+
+  private static class UpdatedAccount {
+    private final AccountConfig accountConfig;
+    private boolean created;
+
+    private UpdatedAccount(AccountConfig accountConfig) {
+      this.accountConfig = checkNotNull(accountConfig);
+    }
+
+    public void setCreated(boolean created) {
+      this.created = created;
+    }
+
+    public boolean isCreated() {
+      return created;
+    }
+
+    public AccountConfig getAccountConfig() {
+      return accountConfig;
+    }
+
+    public Account getAccount() {
+      return accountConfig.getLoadedAccount().get();
+    }
+  }
 }

java/com/google/gerrit/server/account/PutName.java

@@ -66,7 +66,7 @@ public class PutName implements RestModifyView<AccountResource, NameInput> {
 
   public Response<String> apply(IdentifiedUser user, NameInput input)
       throws MethodNotAllowedException, ResourceNotFoundException, IOException,
-          ConfigInvalidException {
+          ConfigInvalidException, OrmException {
     if (input == null) {
       input = new NameInput();
     }

java/com/google/gerrit/server/account/PutPreferred.java

@@ -61,7 +61,7 @@ public class PutPreferred implements RestModifyView<AccountResource.Email, Input
   }
 
   public Response<String> apply(IdentifiedUser user, String email)
-      throws ResourceNotFoundException, IOException, ConfigInvalidException {
+      throws ResourceNotFoundException, IOException, ConfigInvalidException, OrmException {
     AtomicBoolean alreadyPreferred = new AtomicBoolean(false);
     Account account =
         accountsUpdate

java/com/google/gerrit/server/account/PutStatus.java

@@ -60,7 +60,7 @@ public class PutStatus implements RestModifyView<AccountResource, StatusInput> {
   }
 
   public Response<String> apply(IdentifiedUser user, StatusInput input)
-      throws ResourceNotFoundException, IOException, ConfigInvalidException {
+      throws ResourceNotFoundException, IOException, ConfigInvalidException, OrmException {
     if (input == null) {
       input = new StatusInput();
     }

java/com/google/gerrit/server/account/SetInactiveFlag.java

@@ -19,6 +19,7 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.Account;
+import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.io.IOException;
@@ -36,7 +37,7 @@ public class SetInactiveFlag {
   }
 
   public Response<?> deactivate(Account.Id accountId)
-      throws RestApiException, IOException, ConfigInvalidException {
+      throws RestApiException, IOException, ConfigInvalidException, OrmException {
     AtomicBoolean alreadyInactive = new AtomicBoolean(false);
     Account account =
         accountsUpdate
@@ -60,7 +61,7 @@ public class SetInactiveFlag {
   }
 
   public Response<String> activate(Account.Id accountId)
-      throws ResourceNotFoundException, IOException, ConfigInvalidException {
+      throws ResourceNotFoundException, IOException, ConfigInvalidException, OrmException {
     AtomicBoolean alreadyActive = new AtomicBoolean(false);
     Account account =
         accountsUpdate

java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -2941,7 +2941,7 @@ class ReceiveCommits {
       if (account != null) {
         user.getAccount().setFullName(account.getFullName());
       }
-    } catch (IOException | ConfigInvalidException e) {
+    } catch (OrmException | IOException | ConfigInvalidException e) {
       logWarn("Failed to update full name of caller", e);
     }
   }

javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java

@@ -35,6 +35,7 @@ import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toSet;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
+import com.github.rholder.retry.StopStrategies;
 import com.google.common.cache.LoadingCache;
 import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
@@ -92,16 +93,21 @@ import com.google.gerrit.server.account.WatchConfig.NotifyType;
 import com.google.gerrit.server.account.externalids.ExternalId;
 import com.google.gerrit.server.account.externalids.ExternalIds;
 import com.google.gerrit.server.account.externalids.ExternalIdsUpdate;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.LockFailureException;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.index.account.AccountIndexer;
 import com.google.gerrit.server.index.account.StalenessChecker;
 import com.google.gerrit.server.mail.Address;
+import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
 import com.google.gerrit.server.notedb.rebuild.ChangeRebuilderImpl;
 import com.google.gerrit.server.project.RefPattern;
 import com.google.gerrit.server.query.account.InternalAccountQuery;
+import com.google.gerrit.server.update.RetryHelper;
 import com.google.gerrit.server.util.MagicBranch;
 import com.google.gerrit.testing.ConfigSuite;
 import com.google.gerrit.testing.FakeEmailSender.Message;
+import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.name.Named;
@@ -119,10 +125,12 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 import org.bouncycastle.bcpg.ArmoredOutputStream;
 import org.bouncycastle.openpgp.PGPPublicKey;
 import org.bouncycastle.openpgp.PGPPublicKeyRing;
 import org.eclipse.jgit.api.errors.TransportException;
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.CommitBuilder;
@@ -181,6 +189,12 @@ public class AccountIT extends AbstractDaemonTest {
 
   @Inject private AccountIndexer accountIndexer;
 
+  @Inject private OutgoingEmailValidator emailValidator;
+
+  @Inject private GitReferenceUpdated gitReferenceUpdated;
+
+  @Inject private RetryHelper.Metrics retryMetrics;
+
   @Inject
   @Named("accounts")
   private LoadingCache<Account.Id, Optional<AccountState>> accountsCache;
@@ -1880,6 +1894,106 @@ public class AccountIT extends AbstractDaemonTest {
         Permission.CREATE);
   }
 
+  @Test
+  public void retryOnLockFailure() throws Exception {
+    String status = "happy";
+    String fullName = "Foo";
+    AtomicBoolean doneBgUpdate = new AtomicBoolean(false);
+    AccountsUpdate update =
+        new AccountsUpdate(
+            repoManager,
+            gitReferenceUpdated,
+            null,
+            allUsers,
+            emailValidator,
+            serverIdent.get(),
+            () -> metaDataUpdateFactory.create(allUsers),
+            new RetryHelper(
+                cfg,
+                retryMetrics,
+                null,
+                null,
+                null,
+                r -> r.withBlockStrategy(noSleepBlockStrategy)),
+            () -> {
+              if (!doneBgUpdate.getAndSet(true)) {
+                try {
+                  accountsUpdate.create().update(admin.id, u -> u.setStatus(status));
+                } catch (IOException | ConfigInvalidException | OrmException e) {
+                  // Ignore, the successful update of the account is asserted later
+                }
+              }
+            });
+    assertThat(doneBgUpdate.get()).isFalse();
+    AccountInfo accountInfo = gApi.accounts().id(admin.id.get()).get();
+    assertThat(accountInfo.status).isNull();
+    assertThat(accountInfo.name).isNotEqualTo(fullName);
+
+    Account updatedAccount = update.update(admin.id, u -> u.setFullName(fullName));
+    assertThat(doneBgUpdate.get()).isTrue();
+
+    assertThat(updatedAccount.getStatus()).isEqualTo(status);
+    assertThat(updatedAccount.getFullName()).isEqualTo(fullName);
+
+    accountInfo = gApi.accounts().id(admin.id.get()).get();
+    assertThat(accountInfo.status).isEqualTo(status);
+    assertThat(accountInfo.name).isEqualTo(fullName);
+  }
+
+  @Test
+  public void failAfterRetryerGivesUp() throws Exception {
+    List<String> status = ImmutableList.of("foo", "bar", "baz");
+    String fullName = "Foo";
+    AtomicInteger bgCounter = new AtomicInteger(0);
+    AccountsUpdate update =
+        new AccountsUpdate(
+            repoManager,
+            gitReferenceUpdated,
+            null,
+            allUsers,
+            emailValidator,
+            serverIdent.get(),
+            () -> metaDataUpdateFactory.create(allUsers),
+            new RetryHelper(
+                cfg,
+                retryMetrics,
+                null,
+                null,
+                null,
+                r ->
+                    r.withStopStrategy(StopStrategies.stopAfterAttempt(status.size()))
+                        .withBlockStrategy(noSleepBlockStrategy)),
+            () -> {
+              try {
+                accountsUpdate
+                    .create()
+                    .update(admin.id, u -> u.setStatus(status.get(bgCounter.getAndAdd(1))));
+              } catch (IOException | ConfigInvalidException | OrmException e) {
+                // Ignore, the expected exception is asserted later
+              }
+            });
+    assertThat(bgCounter.get()).isEqualTo(0);
+    AccountInfo accountInfo = gApi.accounts().id(admin.id.get()).get();
+    assertThat(accountInfo.status).isNull();
+    assertThat(accountInfo.name).isNotEqualTo(fullName);
+
+    try {
+      update.update(admin.id, u -> u.setFullName(fullName));
+      fail("expected LockFailureException");
+    } catch (LockFailureException e) {
+      // Ignore, expected
+    }
+    assertThat(bgCounter.get()).isEqualTo(status.size());
+
+    Account updatedAccount = accounts.get(admin.id);
+    assertThat(updatedAccount.getStatus()).isEqualTo(Iterables.getLast(status));
+    assertThat(updatedAccount.getFullName()).isEqualTo(admin.fullName);
+
+    accountInfo = gApi.accounts().id(admin.id.get()).get();
+    assertThat(accountInfo.status).isEqualTo(Iterables.getLast(status));
+    assertThat(accountInfo.name).isEqualTo(admin.fullName);
+  }
+
   @Test
   public void stalenessChecker() throws Exception {
     // Newly created account is not stale.