From 7481d3714044bf901588a1f5288b6e2137ddef0e Mon Sep 17 00:00:00 2001 From: Jacob Persson <7156+typfel@users.noreply.github.com> Date: Mon, 18 Sep 2023 17:24:58 +0200 Subject: [PATCH] feat: limit re-trying commit to 2 retry attemps (#2068) * feat: limit re-trying commit to 2 retry attemps * refactor: rename retryCount to remainingAttempts for better readability --- .../conversation/MLSConversationRepository.kt | 27 ++++++- .../MLSConversationRepositoryTest.kt | 79 +++++++++++++++++++ 2 files changed, 103 insertions(+), 3 deletions(-) diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepository.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepository.kt index ca2691bd9c0..a8d47d7cd48 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepository.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepository.kt @@ -117,10 +117,15 @@ private enum class CommitStrategy { } private fun CoreFailure.getStrategy( + remainingAttempts: Int, retryOnClientMismatch: Boolean = true, retryOnStaleMessage: Boolean = true ): CommitStrategy { - return if (this is NetworkFailure.ServerMiscommunication && this.kaliumException is KaliumException.InvalidRequestError) { + return if ( + remainingAttempts > 0 && + this is NetworkFailure.ServerMiscommunication && + kaliumException is KaliumException.InvalidRequestError + ) { if (this.kaliumException.isMlsClientMismatch() && retryOnClientMismatch) { CommitStrategy.DISCARD_AND_RETRY } else if ( @@ -520,18 +525,27 @@ internal class MLSConversationDataSource( ) = operation() .flatMapLeft { - handleCommitFailure(it, groupID, retryOnClientMismatch, retryOnStaleMessage, operation) + handleCommitFailure( + failure = it, + groupID = groupID, + remainingAttempts = 2, + retryOnClientMismatch = retryOnClientMismatch, + retryOnStaleMessage = retryOnStaleMessage, + retryOperation = operation + ) } private suspend fun handleCommitFailure( failure: CoreFailure, groupID: GroupID, + remainingAttempts: Int, retryOnClientMismatch: Boolean, retryOnStaleMessage: Boolean, retryOperation: suspend () -> Either ): Either { return when ( failure.getStrategy( + remainingAttempts = remainingAttempts, retryOnClientMismatch = retryOnClientMismatch, retryOnStaleMessage = retryOnStaleMessage ) @@ -540,7 +554,14 @@ internal class MLSConversationDataSource( CommitStrategy.DISCARD_AND_RETRY -> discardCommitAndRetry(groupID, retryOperation) CommitStrategy.ABORT -> return discardCommit(groupID).flatMap { Either.Left(failure) } }.flatMapLeft { - handleCommitFailure(it, groupID, retryOnClientMismatch, retryOnStaleMessage, retryOperation) + handleCommitFailure( + failure = it, + groupID = groupID, + remainingAttempts = remainingAttempts - 1, + retryOnClientMismatch = retryOnClientMismatch, + retryOnStaleMessage = retryOnStaleMessage, + retryOperation = retryOperation + ) } } diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepositoryTest.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepositoryTest.kt index f764cb0fdf8..e90ebd5581a 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepositoryTest.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepositoryTest.kt @@ -423,6 +423,27 @@ class MLSConversationRepositoryTest { .wasInvoked(once) } + @Test + fun givenRetryLimitIsReached_whenCallingAddMemberToMLSGroup_thenClearCommitAndFail() = runTest { + val (arrangement, mlsConversationRepository) = Arrangement() + .withClaimKeyPackagesSuccessful() + .withGetMLSClientSuccessful() + .withAddMLSMemberSuccessful() + .withSendCommitBundleFailing(Arrangement.MLS_STALE_MESSAGE_ERROR, times = Int.MAX_VALUE) + .withCommitPendingProposalsSuccessful() + .withClearProposalTimerSuccessful() + .withWaitUntilLiveSuccessful() + .arrange() + + val result = mlsConversationRepository.addMemberToMLSGroup(Arrangement.GROUP_ID, listOf(TestConversation.USER_ID1)) + result.shouldFail() + + verify(arrangement.mlsClient) + .function(arrangement.mlsClient::clearPendingCommit) + .with(eq(Arrangement.RAW_GROUP_ID)) + .wasInvoked(once) + } + @Test fun givenSuccessfulResponses_whenCallingRequestToJoinGroup_ThenGroupStateIsUpdated() = runTest { val (arrangement, mlsConversationRepository) = Arrangement() @@ -570,6 +591,24 @@ class MLSConversationRepositoryTest { .wasInvoked(once) } + @Test + fun givenRetryLimitIsReached_whenCallingCommitPendingProposals_thenClearCommitAndFail() = runTest { + val (arrangement, mlsConversationRepository) = Arrangement() + .withGetMLSClientSuccessful() + .withCommitPendingProposalsSuccessful() + .withSendCommitBundleFailing(Arrangement.MLS_STALE_MESSAGE_ERROR, times = Int.MAX_VALUE) + .withWaitUntilLiveSuccessful() + .arrange() + + val result = mlsConversationRepository.commitPendingProposals(Arrangement.GROUP_ID) + result.shouldFail() + + verify(arrangement.mlsClient) + .function(arrangement.mlsClient::clearPendingCommit) + .with(eq(Arrangement.RAW_GROUP_ID)) + .wasInvoked(once) + } + @Test fun givenSuccessfulResponses_whenCallingRemoveMemberFromGroup_thenCommitBundleIsSentAndAccepted() = runTest { val (arrangement, mlsConversationRepository) = Arrangement() @@ -655,6 +694,27 @@ class MLSConversationRepositoryTest { .wasInvoked(once) } + @Test + fun givenRetryLimitIsReached_whenCallingRemoveMemberFromGroup_thenClearCommitAndFail() = runTest { + val (arrangement, mlsConversationRepository) = Arrangement() + .withCommitPendingProposalsSuccessful() + .withGetMLSClientSuccessful() + .withFetchClientsOfUsersSuccessful() + .withRemoveMemberSuccessful() + .withSendCommitBundleFailing(Arrangement.MLS_STALE_MESSAGE_ERROR, times = Int.MAX_VALUE) + .withWaitUntilLiveSuccessful() + .arrange() + + val users = listOf(TestUser.USER_ID) + val result = mlsConversationRepository.removeMembersFromMLSGroup(Arrangement.GROUP_ID, users) + result.shouldFail() + + verify(arrangement.mlsClient) + .function(arrangement.mlsClient::clearPendingCommit) + .with(eq(Arrangement.RAW_GROUP_ID)) + .wasInvoked(once) + } + @Test fun givenClientMismatchError_whenCallingRemoveMemberFromGroup_thenClearCommitAndRetry() = runTest { val (arrangement, mlsConversationRepository) = Arrangement() @@ -892,6 +952,25 @@ class MLSConversationRepositoryTest { .wasInvoked(once) } + @Test + fun givenRetryLimitIsReached_whenCallingUpdateKeyMaterial_clearCommitAndFail() = runTest { + val (arrangement, mlsConversationRepository) = Arrangement() + .withGetMLSClientSuccessful() + .withUpdateKeyingMaterialSuccessful() + .withCommitPendingProposalsSuccessful() + .withSendCommitBundleFailing(Arrangement.MLS_STALE_MESSAGE_ERROR, times = Int.MAX_VALUE) + .withWaitUntilLiveSuccessful() + .arrange() + + val result = mlsConversationRepository.updateKeyingMaterial(Arrangement.GROUP_ID) + result.shouldFail() + + verify(arrangement.mlsClient) + .function(arrangement.mlsClient::clearPendingCommit) + .with(eq(Arrangement.RAW_GROUP_ID)) + .wasInvoked(once) + } + @Test fun givenConversationWithOutdatedEpoch_whenCallingIsGroupOutOfSync_returnsTrue() = runTest { val returnEpoch = 10UL