Keep the push foreground service alive while notifications are fetched (#7639)

* Keep the push foreground service alive while notifications are fetched

* Release the push foreground service as soon as results are handed over

---------

Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>

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 {
     /**

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
+        if (!buildVersionSdkIntProvider.isAtLeast(Build.VERSION_CODES.S)) {
-        fetchPushForegroundServiceManager.stop()
+            // 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<SessionId, List<PushRequest>>,
     ) {

libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncPendingNotificationsRequestBuilder.kt

@@ -91,10 +91,11 @@ class DefaultSyncPendingNotificationsRequestBuilder(
         val request = OneTimeWorkRequestBuilder<FetchPendingNotificationsWorker>()
             .setInputData(workDataOf(SESSION_ID to sessionId.value))
             .apply {
-                // Expedited workers aren't needed on Android 12 or lower:
+                // Before Android 12 (API 31), WorkManager runs expedited work as a foreground service with a visible notification, which we
-                // They force displaying a foreground sync notification for no good reason, since they sync almost immediately anyway
+                // 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)
                 }
             }

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<String>(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<String>(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))
             }

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<String>()
         var synced = false
-        val syncOnNotifiableEventLambda = SyncOnNotifiableEvent { synced = true }
+        val syncOnNotifiableEventLambda = SyncOnNotifiableEvent {
-        val emitResultLambda = lambdaRecorder<Map<PushRequest, Result<ResolvedPushEvent>>, Unit> {}
+            synced = true
+            callOrder += "sync"
+        }
+        val emitResultLambda = lambdaRecorder<Map<PushRequest, Result<ResolvedPushEvent>>, Unit> { callOrder += "emit" }
         val processor = FakeNotificationResultProcessor(emit = emitResultLambda)
+        val stopForegroundServiceLambda = lambdaRecorder<Boolean> {
+            callOrder += "stop"
+            true
+        }
 
         val getPendingResultsLambda = lambdaRecorder<SessionId, Instant?, Result<List<PushRequest>>> { _, _ -> Result.success(listOf(aPushRequest())) }
         val replacePushRequestsLambda = lambdaRecorder<List<PushRequest>, Result<Unit>> { 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<String>()
+        val syncOnNotifiableEventLambda = SyncOnNotifiableEvent { callOrder += "sync" }
+        val emitResultLambda = lambdaRecorder<Map<PushRequest, Result<ResolvedPushEvent>>, Unit> { callOrder += "emit" }
+        val processor = FakeNotificationResultProcessor(emit = emitResultLambda)
+        val stopForegroundServiceLambda = lambdaRecorder<Boolean> {
+            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<Boolean> { 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<Boolean> { 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(