From 5b2203c9686b203c203b47734c4b2c4f41853cbe Mon Sep 17 00:00:00 2001 From: Jacob Persson <7156+typfel@users.noreply.github.com> Date: Fri, 15 Sep 2023 15:39:34 +0200 Subject: [PATCH 1/2] feat: limit re-trying commit to 2 retry attemps --- .../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 7870eda7812..cd7ad5e21db 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 @@ -111,10 +111,15 @@ private enum class CommitStrategy { } private fun CoreFailure.getStrategy( + retryCount: Int, retryOnClientMismatch: Boolean = true, retryOnStaleMessage: Boolean = true ): CommitStrategy { - return if (this is NetworkFailure.ServerMiscommunication && this.kaliumException is KaliumException.InvalidRequestError) { + return if ( + retryCount > 0 && + this is NetworkFailure.ServerMiscommunication && + kaliumException is KaliumException.InvalidRequestError + ) { if (this.kaliumException.isMlsClientMismatch() && retryOnClientMismatch) { CommitStrategy.DISCARD_AND_RETRY } else if ( @@ -458,18 +463,27 @@ internal class MLSConversationDataSource( ) = operation() .flatMapLeft { - handleCommitFailure(it, groupID, retryOnClientMismatch, retryOnStaleMessage, operation) + handleCommitFailure( + failure = it, + groupID = groupID, + retryCount = 2, + retryOnClientMismatch = retryOnClientMismatch, + retryOnStaleMessage = retryOnStaleMessage, + retryOperation = operation + ) } private suspend fun handleCommitFailure( failure: CoreFailure, groupID: GroupID, + retryCount: Int, retryOnClientMismatch: Boolean, retryOnStaleMessage: Boolean, retryOperation: suspend () -> Either ): Either { return when ( failure.getStrategy( + retryCount = retryCount, retryOnClientMismatch = retryOnClientMismatch, retryOnStaleMessage = retryOnStaleMessage ) @@ -478,7 +492,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, + retryCount = retryCount - 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 ea45ed73d68..7e7b09b2873 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 @@ -406,6 +406,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() @@ -552,6 +573,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() @@ -637,6 +676,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() @@ -874,6 +934,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 From 8e4a7ef535c92cfeb950ea80683ddd3f95b9880c Mon Sep 17 00:00:00 2001 From: Jacob Persson <7156+typfel@users.noreply.github.com> Date: Mon, 18 Sep 2023 16:47:51 +0200 Subject: [PATCH 2/2] refactor: rename retryCount to remainingAttempts for better readability --- .../data/conversation/MLSConversationRepository.kt | 12 ++++++------ 1 file changed, 6 insertions(+), 6 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 cd7ad5e21db..68c098b31a7 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 @@ -111,12 +111,12 @@ private enum class CommitStrategy { } private fun CoreFailure.getStrategy( - retryCount: Int, + remainingAttempts: Int, retryOnClientMismatch: Boolean = true, retryOnStaleMessage: Boolean = true ): CommitStrategy { return if ( - retryCount > 0 && + remainingAttempts > 0 && this is NetworkFailure.ServerMiscommunication && kaliumException is KaliumException.InvalidRequestError ) { @@ -466,7 +466,7 @@ internal class MLSConversationDataSource( handleCommitFailure( failure = it, groupID = groupID, - retryCount = 2, + remainingAttempts = 2, retryOnClientMismatch = retryOnClientMismatch, retryOnStaleMessage = retryOnStaleMessage, retryOperation = operation @@ -476,14 +476,14 @@ internal class MLSConversationDataSource( private suspend fun handleCommitFailure( failure: CoreFailure, groupID: GroupID, - retryCount: Int, + remainingAttempts: Int, retryOnClientMismatch: Boolean, retryOnStaleMessage: Boolean, retryOperation: suspend () -> Either ): Either { return when ( failure.getStrategy( - retryCount = retryCount, + remainingAttempts = remainingAttempts, retryOnClientMismatch = retryOnClientMismatch, retryOnStaleMessage = retryOnStaleMessage ) @@ -495,7 +495,7 @@ internal class MLSConversationDataSource( handleCommitFailure( failure = it, groupID = groupID, - retryCount = retryCount - 1, + remainingAttempts = remainingAttempts - 1, retryOnClientMismatch = retryOnClientMismatch, retryOnStaleMessage = retryOnStaleMessage, retryOperation = retryOperation