From ced5af18d096b4adac8044dbe5e4dc114b5f7bc3 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 13 Nov 2025 23:12:57 +0100 Subject: [PATCH] Do not attempt to restore the pusher after 2 removal in a short time. --- .../NotificationSettingsPresenterTest.kt | 2 +- .../libraries/push/test/FakePushService.kt | 2 +- .../UnifiedPushRemovedGatewayHandler.kt | 45 +++++++-- ...ultUnifiedPushRemovedGatewayHandlerTest.kt | 98 ++++++++++++++++++- 4 files changed, 136 insertions(+), 11 deletions(-) diff --git a/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenterTest.kt b/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenterTest.kt index 33eddc3926..9b36c477a4 100644 --- a/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenterTest.kt +++ b/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenterTest.kt @@ -333,7 +333,7 @@ class NotificationSettingsPresenterTest { } private fun createFakePushService( - registerWithLambda: suspend (MatrixClient, PushProvider, Distributor) -> Result = { _, _, _ -> + registerWithLambda: (MatrixClient, PushProvider, Distributor) -> Result = { _, _, _ -> Result.success(Unit) } ): PushService { diff --git a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt index d39c5eb274..604ff88728 100644 --- a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt +++ b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt @@ -23,7 +23,7 @@ import kotlinx.coroutines.flow.MutableStateFlow class FakePushService( private val testPushBlock: suspend (SessionId) -> Boolean = { true }, private val availablePushProviders: List = emptyList(), - private val registerWithLambda: suspend (MatrixClient, PushProvider, Distributor) -> Result = { _, _, _ -> + private val registerWithLambda: (MatrixClient, PushProvider, Distributor) -> Result = { _, _, _ -> Result.success(Unit) }, private val currentPushProvider: (SessionId) -> PushProvider? = { availablePushProviders.firstOrNull() }, diff --git a/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushRemovedGatewayHandler.kt b/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushRemovedGatewayHandler.kt index f8ddb8385a..2b30209245 100644 --- a/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushRemovedGatewayHandler.kt +++ b/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushRemovedGatewayHandler.kt @@ -9,11 +9,16 @@ package io.element.android.libraries.pushproviders.unifiedpush import dev.zacsweers.metro.AppScope import dev.zacsweers.metro.ContributesBinding +import dev.zacsweers.metro.Inject +import dev.zacsweers.metro.SingleIn +import io.element.android.libraries.androidutils.throttler.FirstThrottler import io.element.android.libraries.core.extensions.flatMap import io.element.android.libraries.core.log.logger.LoggerTag +import io.element.android.libraries.di.annotations.AppCoroutineScope import io.element.android.libraries.matrix.api.MatrixClientProvider import io.element.android.libraries.push.api.PushService import io.element.android.libraries.pushstore.api.clientsecret.PushClientSecret +import kotlinx.coroutines.CoroutineScope import timber.log.Timber private val loggerTag = LoggerTag("UnifiedPushRemovedGatewayHandler", LoggerTag.PushLoggerTag) @@ -25,12 +30,29 @@ fun interface UnifiedPushRemovedGatewayHandler { suspend fun handle(clientSecret: String): Result } +@Inject +@SingleIn(AppScope::class) +class UnifiedPushRemovedGatewayThrottler( + @AppCoroutineScope + private val appCoroutineScope: CoroutineScope, +) { + private val firstThrottler = FirstThrottler( + minimumInterval = 60_000, + coroutineScope = appCoroutineScope, + ) + + fun canRegisterAgain(): Boolean { + return firstThrottler.canHandle() + } +} + @ContributesBinding(AppScope::class) class DefaultUnifiedPushRemovedGatewayHandler( private val unregisterUnifiedPushUseCase: UnregisterUnifiedPushUseCase, private val pushClientSecret: PushClientSecret, private val matrixClientProvider: MatrixClientProvider, private val pushService: PushService, + private val unifiedPushRemovedGatewayThrottler: UnifiedPushRemovedGatewayThrottler, ) : UnifiedPushRemovedGatewayHandler { override suspend fun handle(clientSecret: String): Result { // Unregister the pusher for the session with this client secret. @@ -58,14 +80,21 @@ class DefaultUnifiedPushRemovedGatewayHandler( val distributor = pushProvider?.getCurrentDistributor(userId) // Attempt to register again if (pushProvider != null && distributor != null) { - pushService.registerWith( - client, - pushProvider, - distributor, - ) - .onFailure { - Timber.tag(loggerTag.value).w(it, "Unable to register with current data") - } + if (unifiedPushRemovedGatewayThrottler.canRegisterAgain()) { + pushService.registerWith( + client, + pushProvider, + distributor, + ) + .onFailure { + Timber.tag(loggerTag.value).w(it, "Unable to register with current data") + } + } else { + // Let the user know + Timber.tag(loggerTag.value).w("Second removal in less than 1 minute, do not register again") + pushService.onServiceUnregistered(userId) + Result.success(Unit) + } } else { Result.failure(IllegalStateException("Unable to register again")) } diff --git a/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/DefaultUnifiedPushRemovedGatewayHandlerTest.kt b/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/DefaultUnifiedPushRemovedGatewayHandlerTest.kt index 04c2bf0323..452013e18d 100644 --- a/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/DefaultUnifiedPushRemovedGatewayHandlerTest.kt +++ b/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/DefaultUnifiedPushRemovedGatewayHandlerTest.kt @@ -19,14 +19,19 @@ import io.element.android.libraries.matrix.test.FakeMatrixClientProvider import io.element.android.libraries.push.api.PushService import io.element.android.libraries.push.test.FakePushService import io.element.android.libraries.pushproviders.api.Distributor +import io.element.android.libraries.pushproviders.api.PushProvider import io.element.android.libraries.pushproviders.test.FakePushProvider import io.element.android.libraries.pushstore.api.clientsecret.PushClientSecret import io.element.android.libraries.pushstore.test.userpushstore.clientsecret.FakePushClientSecret import io.element.android.tests.testutils.lambda.any import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.lambda.value +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.runTest import org.junit.Test +import kotlin.time.Duration.Companion.seconds class DefaultUnifiedPushRemovedGatewayHandlerTest { @Test @@ -188,7 +193,95 @@ class DefaultUnifiedPushRemovedGatewayHandlerTest { onServiceUnregisteredResult.assertions().isNeverCalled() } - private fun createDefaultUnifiedPushRemovedGatewayHandler( + @Test + fun `handle returns success if can register again, but after 2 removals user is notified`() = runTest { + val onServiceUnregisteredResult = lambdaRecorder { } + val unregisterLambda = lambdaRecorder> { _, _, _ -> Result.success(Unit) } + val registerWithLambda = lambdaRecorder> { _, _, _ -> Result.success(Unit) } + val sut = createDefaultUnifiedPushRemovedGatewayHandler( + pushClientSecret = FakePushClientSecret( + getUserIdFromSecretResult = { A_SESSION_ID }, + ), + matrixClientProvider = FakeMatrixClientProvider( + getClient = { Result.success(FakeMatrixClient()) }, + ), + unregisterUnifiedPushUseCase = FakeUnregisterUnifiedPushUseCase( + unregisterLambda = unregisterLambda, + ), + pushService = FakePushService( + currentPushProvider = { + FakePushProvider( + currentDistributor = { Distributor("aValue", "aName") }, + ) + }, + registerWithLambda = registerWithLambda, + onServiceUnregisteredResult = onServiceUnregisteredResult, + ), + ) + val result = sut.handle(A_SECRET) + assertThat(result.isSuccess).isTrue() + unregisterLambda.assertions().isCalledOnce().with( + any(), + value(A_SECRET), + value(false), + ) + registerWithLambda.assertions().isCalledOnce() + onServiceUnregisteredResult.assertions().isNeverCalled() + // Second attempt in less than 1 minute + val result2 = sut.handle(A_SECRET) + assertThat(result2.isSuccess).isTrue() + unregisterLambda.assertions().isCalledExactly(2) + // Registration is not called twice + registerWithLambda.assertions().isCalledOnce() + onServiceUnregisteredResult.assertions().isCalledOnce().with(value(A_SESSION_ID)) + } + + @OptIn(ExperimentalCoroutinesApi::class) + @Test + fun `handle returns success if can register again, but after 2 distant removals user is not notified`() = runTest { + val onServiceUnregisteredResult = lambdaRecorder { } + val unregisterLambda = lambdaRecorder> { _, _, _ -> Result.success(Unit) } + val registerWithLambda = lambdaRecorder> { _, _, _ -> Result.success(Unit) } + val sut = createDefaultUnifiedPushRemovedGatewayHandler( + pushClientSecret = FakePushClientSecret( + getUserIdFromSecretResult = { A_SESSION_ID }, + ), + matrixClientProvider = FakeMatrixClientProvider( + getClient = { Result.success(FakeMatrixClient()) }, + ), + unregisterUnifiedPushUseCase = FakeUnregisterUnifiedPushUseCase( + unregisterLambda = unregisterLambda, + ), + pushService = FakePushService( + currentPushProvider = { + FakePushProvider( + currentDistributor = { Distributor("aValue", "aName") }, + ) + }, + registerWithLambda = registerWithLambda, + onServiceUnregisteredResult = onServiceUnregisteredResult, + ), + ) + val result = sut.handle(A_SECRET) + assertThat(result.isSuccess).isTrue() + unregisterLambda.assertions().isCalledOnce().with( + any(), + value(A_SECRET), + value(false), + ) + registerWithLambda.assertions().isCalledOnce() + onServiceUnregisteredResult.assertions().isNeverCalled() + // Second attempt in more than 1 minute + advanceTimeBy(61.seconds) + val result2 = sut.handle(A_SECRET) + assertThat(result2.isSuccess).isTrue() + unregisterLambda.assertions().isCalledExactly(2) + // Registration is not called twice + registerWithLambda.assertions().isCalledExactly(2) + onServiceUnregisteredResult.assertions().isNeverCalled() + } + + private fun TestScope.createDefaultUnifiedPushRemovedGatewayHandler( unregisterUnifiedPushUseCase: UnregisterUnifiedPushUseCase = FakeUnregisterUnifiedPushUseCase(), pushClientSecret: PushClientSecret = FakePushClientSecret(), matrixClientProvider: MatrixClientProvider = FakeMatrixClientProvider(), @@ -198,5 +291,8 @@ class DefaultUnifiedPushRemovedGatewayHandlerTest { pushClientSecret = pushClientSecret, matrixClientProvider = matrixClientProvider, pushService = pushService, + unifiedPushRemovedGatewayThrottler = UnifiedPushRemovedGatewayThrottler( + appCoroutineScope = backgroundScope, + ), ) }