diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java index e8cddb6f3..ddd980715 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -558,11 +558,11 @@ public class AccountController { @DELETE @Path("/me") - public CompletableFuture deleteAccount(@Auth AuthenticatedDevice auth) { + public void deleteAccount(@Auth AuthenticatedDevice auth) { final Account account = accounts.getByAccountIdentifier(auth.accountIdentifier()) .orElseThrow(() -> new WebApplicationException(Status.UNAUTHORIZED)); - return accounts.delete(account, AccountsManager.DeletionReason.USER_REQUEST).thenApply(Util.ASYNC_EMPTY_RESPONSE); + accounts.delete(account, AccountsManager.DeletionReason.USER_REQUEST); } private void clearUsernameLink(final Account account) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/grpc/AccountsGrpcService.java b/service/src/main/java/org/whispersystems/textsecuregcm/grpc/AccountsGrpcService.java index f894f703f..7be76b73c 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/grpc/AccountsGrpcService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/grpc/AccountsGrpcService.java @@ -96,8 +96,7 @@ public class AccountsGrpcService extends SimpleAccountsGrpc.AccountsImplBase { @Override public DeleteAccountResponse deleteAccount(final DeleteAccountRequest request) { accountsManager.delete(getAuthenticatedAccount(AuthenticationUtil.requireAuthenticatedPrimaryDevice()), - AccountsManager.DeletionReason.USER_REQUEST) - .join(); + AccountsManager.DeletionReason.USER_REQUEST); return DeleteAccountResponse.getDefaultInstance(); } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java index 1d8642b93..70906cb76 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java @@ -1176,30 +1176,39 @@ public class Accounts { .map(UUID::fromString); } - public CompletableFuture delete(final UUID uuid, final List additionalWriteItems) { + public void delete(final UUID uuid, final List additionalWriteItems) { final Timer.Sample sample = Timer.start(); - return getByAccountIdentifierAsync(uuid) - .thenCompose(maybeAccount -> maybeAccount.map(account -> { - final List transactWriteItems = new ArrayList<>(List.of( - buildDelete(phoneNumberConstraintTableName, ATTR_ACCOUNT_E164, account.getNumber()), - buildDelete(accountsTableName, KEY_ACCOUNT_UUID, uuid), - buildDelete(phoneNumberIdentifierConstraintTableName, ATTR_PNI_UUID, account.getPhoneNumberIdentifier()), - buildPutDeletedAccount(uuid, account.getPhoneNumberIdentifier()) - )); + try { + final Account account; + { + final Optional maybeAccount = getByAccountIdentifier(uuid); - account.getUsernameHash().ifPresent(usernameHash -> transactWriteItems.add( - buildDelete(usernamesConstraintTableName, UsernameTable.KEY_USERNAME_HASH, usernameHash))); + if (maybeAccount.isEmpty()) { + return; + } - transactWriteItems.addAll(additionalWriteItems); + account = maybeAccount.get(); + } - return dynamoDbAsyncClient.transactWriteItems(TransactWriteItemsRequest.builder() - .transactItems(transactWriteItems) - .build()) - .thenRun(Util.NOOP); - }) - .orElseGet(() -> CompletableFuture.completedFuture(null))) - .thenRun(() -> sample.stop(DELETE_TIMER)); + final List transactWriteItems = new ArrayList<>(List.of( + buildDelete(phoneNumberConstraintTableName, ATTR_ACCOUNT_E164, account.getNumber()), + buildDelete(accountsTableName, KEY_ACCOUNT_UUID, uuid), + buildDelete(phoneNumberIdentifierConstraintTableName, ATTR_PNI_UUID, account.getPhoneNumberIdentifier()), + buildPutDeletedAccount(uuid, account.getPhoneNumberIdentifier()) + )); + + account.getUsernameHash().ifPresent(usernameHash -> transactWriteItems.add( + buildDelete(usernamesConstraintTableName, UsernameTable.KEY_USERNAME_HASH, usernameHash))); + + transactWriteItems.addAll(additionalWriteItems); + + dynamoDbClient.transactWriteItems(TransactWriteItemsRequest.builder() + .transactItems(transactWriteItems) + .build()); + } finally { + sample.stop(DELETE_TIMER); + } } Flux getAll(final int segments, final Scheduler scheduler) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java index c8ad6fbfe..b47ad35a8 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -747,7 +747,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen if (maybeExistingAccount.get().getIdentifier(IdentityType.ACI).equals(account.getIdentifier(IdentityType.ACI))) { maybeDisplacedUuid = Optional.empty(); } else { - delete(maybeExistingAccount.get()).join(); + delete(maybeExistingAccount.get()); maybeDisplacedUuid = maybeExistingAccount.map(Account::getUuid); } } else { @@ -1133,36 +1133,42 @@ public class AccountsManager extends RedisPubSubAdapter implemen return accounts.getAll(segments, scheduler); } - public CompletableFuture delete(final Account account, final DeletionReason deletionReason) { + public void delete(final Account account, final DeletionReason deletionReason) { final Timer.Sample sample = Timer.start(); - return accountLockManager.withLockAsync(Set.of(account.getPhoneNumberIdentifier()), () -> delete(account), - accountLockExecutor) - .whenComplete((ignored, throwable) -> { - sample.stop(deleteTimer); + try { + accountLockManager.withLock(Set.of(account.getPhoneNumberIdentifier()), () -> { + delete(account); + return null; + }, accountLockExecutor); - if (throwable == null) { - Metrics.counter(DELETE_COUNTER_NAME, - COUNTRY_CODE_TAG_NAME, Util.getCountryCode(account.getNumber()), - DELETION_REASON_TAG_NAME, deletionReason.tagValue) - .increment(); - } else { - logger.warn("Failed to delete account", throwable); - } - }); + Metrics.counter(DELETE_COUNTER_NAME, + COUNTRY_CODE_TAG_NAME, Util.getCountryCode(account.getNumber()), + DELETION_REASON_TAG_NAME, deletionReason.tagValue) + .increment(); + } catch (final Exception e) { + logger.warn("Failed to delete account", e); + + if (e instanceof RuntimeException runtimeException) { + throw runtimeException; + } + + throw new RuntimeException(e); + } finally { + sample.stop(deleteTimer); + } } - private CompletableFuture delete(final Account account) { - final List additionalWriteItems = - account.getDevices().stream() - .flatMap(device -> keysManager.buildWriteItemsForRemovedDevice( - account.getIdentifier(IdentityType.ACI), - account.getIdentifier(IdentityType.PNI), - device.getId()) - .stream()) - .toList(); + private void delete(final Account account) { + final List additionalWriteItems = account.getDevices().stream() + .flatMap(device -> keysManager.buildWriteItemsForRemovedDevice( + account.getIdentifier(IdentityType.ACI), + account.getIdentifier(IdentityType.PNI), + device.getId()) + .stream()) + .toList(); - return CompletableFuture.allOf( + CompletableFuture.allOf( secureStorageClient.deleteStoredData(account.getUuid()), secureValueRecovery2Client.removeData(account.getUuid()), keysManager.deleteSingleUsePreKeys(account.getUuid()), @@ -1170,9 +1176,12 @@ public class AccountsManager extends RedisPubSubAdapter implemen messagesManager.clear(account.getUuid()), profilesManager.deleteAll(account.getUuid(), true), registrationRecoveryPasswordsManager.remove(account.getIdentifier(IdentityType.PNI))) - .thenCompose(ignored -> accounts.delete(account.getUuid(), additionalWriteItems)) - .thenCompose(ignored -> redisDeleteAsync(account)) - .thenRun(() -> disconnectionRequestManager.requestDisconnection(account)); + .join(); + + accounts.delete(account.getUuid(), additionalWriteItems); + redisDelete(account); + + disconnectionRequestManager.requestDisconnection(account); } private String getAccountMapKey(String key) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java index 199d2680a..d464f7e01 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java @@ -51,7 +51,7 @@ public class DeleteUserCommand extends AbstractCommandWithDependencies { Optional account = accountsManager.getByE164(user); if (account.isPresent()) { - accountsManager.delete(account.get(), DeletionReason.ADMIN_DELETED).join(); + accountsManager.delete(account.get(), DeletionReason.ADMIN_DELETED); logger.warn("Removed " + account.get().getNumber()); } else { logger.warn("Account not found"); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/RemoveExpiredAccountsCommand.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/RemoveExpiredAccountsCommand.java index 3c4e134e8..2e59594cb 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/RemoveExpiredAccountsCommand.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/RemoveExpiredAccountsCommand.java @@ -20,6 +20,7 @@ import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountsManager; import reactor.core.publisher.Mono; import reactor.core.publisher.Flux; +import reactor.core.scheduler.Schedulers; import reactor.util.retry.Retry; public class RemoveExpiredAccountsCommand extends AbstractSinglePassCrawlAccountsCommand { @@ -66,7 +67,9 @@ public class RemoveExpiredAccountsCommand extends AbstractSinglePassCrawlAccount .flatMap(expiredAccount -> { final Mono deleteAccountMono = isDryRun ? Mono.empty() - : Mono.fromFuture(() -> getCommandDependencies().accountsManager().delete(expiredAccount, AccountsManager.DeletionReason.EXPIRED)); + : Mono.fromRunnable(() -> getCommandDependencies().accountsManager().delete(expiredAccount, AccountsManager.DeletionReason.EXPIRED)) + .subscribeOn(Schedulers.boundedElastic()) + .then(); return deleteAccountMono .doOnSuccess(ignored -> deletedAccountCounter.increment()) diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java index 092c1a241..b9aeb8b65 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java @@ -92,7 +92,6 @@ import org.whispersystems.textsecuregcm.storage.UsernameHashNotAvailableExceptio import org.whispersystems.textsecuregcm.storage.UsernameReservationNotFoundException; import org.whispersystems.textsecuregcm.tests.util.AccountsHelper; import org.whispersystems.textsecuregcm.tests.util.AuthHelper; -import org.whispersystems.textsecuregcm.util.CompletableFutureTestUtil; import org.whispersystems.textsecuregcm.util.MockUtils; import org.whispersystems.textsecuregcm.util.SystemMapper; import org.whispersystems.textsecuregcm.util.TestRandomUtil; @@ -846,8 +845,6 @@ class AccountControllerTest { @Test void testDeleteAccount() { - when(accountsManager.delete(any(), any())).thenReturn(CompletableFutureTestUtil.almostCompletedFuture(null)); - try (final Response response = resources.getJerseyTest() .target("/v1/accounts/me") .request() @@ -861,7 +858,8 @@ class AccountControllerTest { @Test void testDeleteAccountException() { - when(accountsManager.delete(any(), any())).thenReturn(CompletableFuture.failedFuture(new RuntimeException("OH NO"))); + doThrow(new RuntimeException("OH NO")) + .when(accountsManager).delete(any(), any()); try (final Response response = resources.getJerseyTest() .target("/v1/accounts/me") diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/grpc/AccountsGrpcServiceTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/grpc/AccountsGrpcServiceTest.java index 01bbf941c..91f58bea2 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/grpc/AccountsGrpcServiceTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/grpc/AccountsGrpcServiceTest.java @@ -154,9 +154,6 @@ class AccountsGrpcServiceTest extends SimpleBaseGrpcTest) invocation -> { final Account account = invocation.getArgument(0, Account.class); final String number = invocation.getArgument(1, String.class); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java index 1d03efb29..132425395 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -214,7 +214,7 @@ class AccountsTest { assertPhoneNumberConstraintExists("+14151112222", account.getUuid()); assertPhoneNumberIdentifierConstraintExists(account.getPhoneNumberIdentifier(), account.getUuid()); - accounts.delete(originalUuid, Collections.emptyList()).join(); + accounts.delete(originalUuid, Collections.emptyList()); assertThat(accounts.findRecentlyDeletedAccountIdentifier(account.getPhoneNumberIdentifier())).hasValue(originalUuid); freshUser = createAccount(account); @@ -773,7 +773,7 @@ class AccountsTest { assertThat(accounts.getByAccountIdentifier(deletedAccount.getUuid())).isPresent(); assertThat(accounts.getByAccountIdentifier(retainedAccount.getUuid())).isPresent(); - accounts.delete(deletedAccount.getUuid(), Collections.emptyList()).join(); + accounts.delete(deletedAccount.getUuid(), Collections.emptyList()); assertThat(accounts.getByAccountIdentifier(deletedAccount.getUuid())).isNotPresent(); assertThat(accounts.findRecentlyDeletedAccountIdentifier(deletedAccount.getPhoneNumberIdentifier())).hasValue(deletedAccount.getUuid()); @@ -1813,7 +1813,7 @@ class AccountsTest { .build()) .items()); - accounts.delete(account.getIdentifier(IdentityType.ACI), Collections.emptyList()).join(); + accounts.delete(account.getIdentifier(IdentityType.ACI), Collections.emptyList()); writeAccountRecordWithoutConstraints(account); accounts.regenerateConstraints(account).join(); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/workers/RemoveExpiredAccountsCommandTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/workers/RemoveExpiredAccountsCommandTest.java index 8a4532e3c..bc2943412 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/workers/RemoveExpiredAccountsCommandTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/workers/RemoveExpiredAccountsCommandTest.java @@ -16,7 +16,6 @@ import static org.mockito.Mockito.when; import java.time.Clock; import java.time.Instant; import java.time.ZoneId; -import java.util.concurrent.CompletableFuture; import java.util.stream.Stream; import net.sourceforge.argparse4j.inf.Namespace; import org.junit.jupiter.params.ParameterizedTest; @@ -61,7 +60,6 @@ class RemoveExpiredAccountsCommandTest { final Clock clock = Clock.fixed(Instant.now(), ZoneId.systemDefault()); final AccountsManager accountsManager = mock(AccountsManager.class); - when(accountsManager.delete(any(), any())).thenReturn(CompletableFuture.completedFuture(null)); final RemoveExpiredAccountsCommand removeExpiredAccountsCommand = new TestRemoveExpiredAccountsCommand(clock, accountsManager, isDryRun);