diff --git a/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/push/FetchPushForegroundServiceManager.kt b/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/push/FetchPushForegroundServiceManager.kt index be178bbe6c..3510242e60 100644 --- a/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/push/FetchPushForegroundServiceManager.kt +++ b/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/push/FetchPushForegroundServiceManager.kt @@ -8,7 +8,8 @@ package io.element.android.libraries.push.api.push /** - * A helper to manage the foreground service used to keep the device awake while we schedule and wait for the work to fetch the notification content to run. + * A helper to manage the foreground service used to keep the device awake, and the process out of background execution limits, from the moment + * a push is received until the fetched notification content has been handed over for display. */ interface FetchPushForegroundServiceManager { /** diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/FetchPendingNotificationsWorker.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/FetchPendingNotificationsWorker.kt index cefbd31515..d800a7efc2 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/FetchPendingNotificationsWorker.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/FetchPendingNotificationsWorker.kt @@ -9,6 +9,7 @@ package io.element.android.libraries.push.impl.workmanager import android.content.Context +import android.os.Build import androidx.work.CoroutineWorker import androidx.work.WorkerParameters import dev.zacsweers.metro.AppScope @@ -39,8 +40,11 @@ import io.element.android.services.analytics.api.AnalyticsService import io.element.android.services.analytics.api.finishLongRunningTransaction import io.element.android.services.analytics.api.recordTransaction import io.element.android.services.analyticsproviders.api.AnalyticsTransaction +import io.element.android.services.toolbox.api.sdk.BuildVersionSdkIntProvider import io.element.android.services.toolbox.api.systemclock.SystemClock +import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.flow.first +import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeoutOrNull import timber.log.Timber import kotlin.time.Duration.Companion.days @@ -59,16 +63,33 @@ class FetchPendingNotificationsWorker( private val analyticsService: AnalyticsService, private val systemClock: SystemClock, private val fetchPushForegroundServiceManager: FetchPushForegroundServiceManager, + private val buildVersionSdkIntProvider: BuildVersionSdkIntProvider, ) : CoroutineWorker(context, params) { + private var foregroundServiceReleased = false + override suspend fun doWork(): Result { Timber.d("FetchNotificationsWorker started") + return try { + doWorkInternal() + } finally { + // Make sure the foreground service is released on every exit path, including when the system cancels the worker. + withContext(NonCancellable) { + releaseForegroundService() + } + } + } + + private suspend fun doWorkInternal(): Result { // RunCatching for test in debug mode val sessionId = runCatchingExceptions { inputData.getString(SyncPendingNotificationsRequestBuilder.SESSION_ID)?.let(::SessionId) }.getOrNull() ?: return Result.failure() - // We can stop the foreground service and unlock the wakelock, since the work is now running and the device should be kept awake - fetchPushForegroundServiceManager.stop() + if (!buildVersionSdkIntProvider.isAtLeast(Build.VERSION_CODES.S)) { + // Before Android 12 the foreground service notification is displayed immediately, so release the service as soon as the work is + // running: the wakelock was only there to make sure the scheduled work starts. See [releaseForegroundService] for newer versions. + releaseForegroundService() + } // Fetch pending requests in the last 24 hours val fetchSince = Instant.fromEpochMilliseconds(systemClock.epochMillis()).minus(1.days) @@ -132,6 +153,10 @@ class FetchPendingNotificationsWorker( pushHistoryService.insertOrUpdatePushRequests(updatedRequests) + // The results have been handed over, so release the foreground service before the opportunistic sync, which deliberately takes a while + // in the background. This keeps the service short-lived in the common case, so its notification is normally never displayed. + releaseForegroundService() + analyticsService.recordTransaction("Opportunistic sync", "opportunistic_sync") { performOpportunisticSyncIfNeeded(mapOf(sessionId to requests)) } @@ -139,6 +164,21 @@ class FetchPendingNotificationsWorker( return if (updatedRequests.any { it.status == PushRequestStatus.PENDING.value }) Result.retry() else Result.success() } + /** + * Release the foreground service, and the wakelock it holds, that was started when the push was received. Only the first call has an effect. + * + * From Android 12 on, the service is kept while the events are resolved: this needs several network round trips (sliding sync, room key + * retrieval, decryption) and, for ringing calls, waiting for the room state to be synced. While the service is running the process is in the + * foreground service state, so the expedited job quota and Doze restrictions don't cut this work short. The system defers the notification + * of a foreground service by 10 seconds on these versions, so it only becomes visible when the fetch is slow, which is exactly when the + * extra time is needed. + */ + private suspend fun releaseForegroundService() { + if (foregroundServiceReleased) return + foregroundServiceReleased = true + fetchPushForegroundServiceManager.stop() + } + private suspend fun performOpportunisticSyncIfNeeded( groupedRequests: Map>, ) { diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncPendingNotificationsRequestBuilder.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncPendingNotificationsRequestBuilder.kt index 4c1da42660..b7788fcc04 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncPendingNotificationsRequestBuilder.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncPendingNotificationsRequestBuilder.kt @@ -91,10 +91,11 @@ class DefaultSyncPendingNotificationsRequestBuilder( val request = OneTimeWorkRequestBuilder() .setInputData(workDataOf(SESSION_ID to sessionId.value)) .apply { - // Expedited workers aren't needed on Android 12 or lower: - // They force displaying a foreground sync notification for no good reason, since they sync almost immediately anyway + // Before Android 12 (API 31), WorkManager runs expedited work as a foreground service with a visible notification, which we + // don't want and don't support (no `getForegroundInfo` implementation). From API 31 on, expedited work is a native JobScheduler + // expedited job: it starts sooner and is less affected by Doze and App Standby, so use it wherever it is available. // See https://developer.android.com/develop/background-work/background-tasks/persistent/getting-started/define-work#backwards-compat - if (buildVersionSdkIntProvider.isAtLeast(Build.VERSION_CODES.TIRAMISU)) { + if (buildVersionSdkIntProvider.isAtLeast(Build.VERSION_CODES.S)) { setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) } } diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/DefaultSyncPendingNotificationsRequestBuilderTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/DefaultSyncPendingNotificationsRequestBuilderTest.kt index 49d0d12ec8..43926c1d13 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/DefaultSyncPendingNotificationsRequestBuilderTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/DefaultSyncPendingNotificationsRequestBuilderTest.kt @@ -28,10 +28,10 @@ import org.robolectric.annotation.Config @Config(sdk = [33]) class DefaultSyncPendingNotificationsRequestBuilderTest : RobolectricTest() { @Test - fun `build - success API 33`() = runTest { + fun `build - success API 31`() = runTest { val request = createSyncPendingNotificationsRequestBuilder( sessionId = A_SESSION_ID, - sdkVersion = 33, + sdkVersion = 31, ) val results = request.build() @@ -42,7 +42,7 @@ class DefaultSyncPendingNotificationsRequestBuilderTest : RobolectricTest() { assertThat(this).isInstanceOf(OneTimeWorkRequest::class.java) assertThat(workSpec.input.hasKeyWithValueOfType(SyncPendingNotificationsRequestBuilder.SESSION_ID)).isTrue() assertThat(workSpec.hasConstraints()).isTrue() - // True in API 33+ + // True in API 31+ assertThat(workSpec.expedited).isTrue() assertThat(workSpec.traceTag).isEqualTo(workManagerTag(A_SESSION_ID, WorkManagerRequestType.NOTIFICATION_SYNC)) } @@ -50,10 +50,10 @@ class DefaultSyncPendingNotificationsRequestBuilderTest : RobolectricTest() { } @Test - fun `build - success API 32 and lower`() = runTest { + fun `build - success API 30 and lower`() = runTest { val request = createSyncPendingNotificationsRequestBuilder( sessionId = A_SESSION_ID, - sdkVersion = 32, + sdkVersion = 30, ) val results = request.build() @@ -65,7 +65,7 @@ class DefaultSyncPendingNotificationsRequestBuilderTest : RobolectricTest() { assertThat(this).isInstanceOf(OneTimeWorkRequest::class.java) assertThat(workSpec.input.hasKeyWithValueOfType(SyncPendingNotificationsRequestBuilder.SESSION_ID)).isTrue() assertThat(workSpec.hasConstraints()).isTrue() - // False before API 33 + // False before API 31 assertThat(workSpec.expedited).isFalse() assertThat(workSpec.traceTag).isEqualTo(workManagerTag(A_SESSION_ID, WorkManagerRequestType.NOTIFICATION_SYNC)) } diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/FetchPendingNotificationWorkerTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/FetchPendingNotificationWorkerTest.kt index 889fb878d2..77082774dc 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/FetchPendingNotificationWorkerTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/FetchPendingNotificationWorkerTest.kt @@ -31,6 +31,7 @@ import io.element.android.libraries.push.test.push.FakeFetchPushForegroundServic import io.element.android.libraries.workmanager.api.WorkManagerRequestBuilder import io.element.android.libraries.workmanager.api.di.MetroWorkerFactory import io.element.android.services.analytics.test.FakeAnalyticsService +import io.element.android.services.toolbox.test.sdk.FakeBuildVersionSdkIntProvider import io.element.android.services.toolbox.test.systemclock.FakeSystemClock import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.robolectric.RobolectricTest @@ -50,10 +51,18 @@ import kotlin.time.Instant class FetchPendingNotificationWorkerTest : RobolectricTest() { @Test fun `test - success`() = runTest { + val callOrder = mutableListOf() var synced = false - val syncOnNotifiableEventLambda = SyncOnNotifiableEvent { synced = true } - val emitResultLambda = lambdaRecorder>, Unit> {} + val syncOnNotifiableEventLambda = SyncOnNotifiableEvent { + synced = true + callOrder += "sync" + } + val emitResultLambda = lambdaRecorder>, Unit> { callOrder += "emit" } val processor = FakeNotificationResultProcessor(emit = emitResultLambda) + val stopForegroundServiceLambda = lambdaRecorder { + callOrder += "stop" + true + } val getPendingResultsLambda = lambdaRecorder>> { _, _ -> Result.success(listOf(aPushRequest())) } val replacePushRequestsLambda = lambdaRecorder, Result> { Result.success(Unit) } @@ -69,6 +78,7 @@ class FetchPendingNotificationWorkerTest : RobolectricTest() { pushHistoryService = pushHistoryService, resultProcessor = processor, syncOnNotifiableEvent = syncOnNotifiableEventLambda, + pushHandlingWakeLock = FakeFetchPushForegroundServiceManager(unlock = stopForegroundServiceLambda), ) val result = worker.doWork() @@ -86,16 +96,61 @@ class FetchPendingNotificationWorkerTest : RobolectricTest() { // An opportunistic sync was triggered assertThat(synced).isTrue() + + // The foreground service is released once, after the results were handed over and before the opportunistic sync + stopForegroundServiceLambda.assertions().isCalledOnce() + assertThat(callOrder).containsExactly("emit", "stop", "sync").inOrder() + } + + @Test + fun `test - before Android 12 the foreground service is released as soon as the work runs`() = runTest { + val callOrder = mutableListOf() + val syncOnNotifiableEventLambda = SyncOnNotifiableEvent { callOrder += "sync" } + val emitResultLambda = lambdaRecorder>, Unit> { callOrder += "emit" } + val processor = FakeNotificationResultProcessor(emit = emitResultLambda) + val stopForegroundServiceLambda = lambdaRecorder { + callOrder += "stop" + true + } + val pushHistoryService = FakePushHistoryService( + getPendingPushRequests = { _, _ -> Result.success(listOf(aPushRequest())) }, + replacePushRequests = { Result.success(Unit) }, + removeOldPushRequests = { Result.success(Unit) }, + ) + + val worker = createWorker( + input = "@alice:matrix.org", + pushHistoryService = pushHistoryService, + resultProcessor = processor, + syncOnNotifiableEvent = syncOnNotifiableEventLambda, + pushHandlingWakeLock = FakeFetchPushForegroundServiceManager(unlock = stopForegroundServiceLambda), + buildVersionSdkIntProvider = FakeBuildVersionSdkIntProvider(30), + ) + + val result = worker.doWork() + + assertThat(result).isEqualTo(ListenableWorker.Result.success()) + + // The foreground service is released once, before the events are resolved, since its notification would be displayed immediately + stopForegroundServiceLambda.assertions().isCalledOnce() + assertThat(callOrder).containsExactly("stop", "emit", "sync").inOrder() } @Test fun `test - invalid input fails the work`() = runTest { - val worker = createWorker(input = "!alice:matrix.org") + val stopForegroundServiceLambda = lambdaRecorder { true } + val worker = createWorker( + input = "!alice:matrix.org", + pushHandlingWakeLock = FakeFetchPushForegroundServiceManager(unlock = stopForegroundServiceLambda), + ) val result = worker.doWork() // The process failed assertThat(result).isEqualTo(ListenableWorker.Result.failure()) + + // The foreground service is still stopped + stopForegroundServiceLambda.assertions().isCalledOnce() } @Test @@ -108,11 +163,13 @@ class FetchPendingNotificationWorkerTest : RobolectricTest() { replacePushRequests = { Result.success(Unit) }, removeOldPushRequests = { Result.success(Unit) }, ) + val stopForegroundServiceLambda = lambdaRecorder { true } val worker = createWorker( input = "@alice:matrix.org", networkMonitor = networkMonitor, resultProcessor = processor, pushHistoryService = pushHistoryService, + pushHandlingWakeLock = FakeFetchPushForegroundServiceManager(unlock = stopForegroundServiceLambda), ) val result = worker.doWork() @@ -121,6 +178,9 @@ class FetchPendingNotificationWorkerTest : RobolectricTest() { // The process failed due to a timeout in getting the network connectivity, a retry is scheduled assertThat(result).isEqualTo(ListenableWorker.Result.retry()) + + // The foreground service is still stopped + stopForegroundServiceLambda.assertions().isCalledOnce() } @Test @@ -238,6 +298,7 @@ class FetchPendingNotificationWorkerTest : RobolectricTest() { resultProcessor: FakeNotificationResultProcessor = FakeNotificationResultProcessor(), systemClock: FakeSystemClock = FakeSystemClock(), pushHandlingWakeLock: FakeFetchPushForegroundServiceManager = FakeFetchPushForegroundServiceManager(), + buildVersionSdkIntProvider: FakeBuildVersionSdkIntProvider = FakeBuildVersionSdkIntProvider(33), ) = FetchPendingNotificationsWorker( params = createWorkerParams(workDataOf("session_id" to input)), context = InstrumentationRegistry.getInstrumentation().context, @@ -249,6 +310,7 @@ class FetchPendingNotificationWorkerTest : RobolectricTest() { resultProcessor = resultProcessor, systemClock = systemClock, fetchPushForegroundServiceManager = pushHandlingWakeLock, + buildVersionSdkIntProvider = buildVersionSdkIntProvider, ) private fun TestScope.createWorkerParams(