Don't restart the sync service if the client is shutting down (#7472)

* Don't restart the sync service if the client is shutting down

This was causing issues when clearing the cache:

1. The client would ask the SDK to clear the caches.
2. The SDK would stop the sync service, clear the caches and set the `pos` value of sliding sync to 0 so the HS returns all the required state the next time the client is used.
3. However, between the sync service being stopped with the `pos=0` change and the client being destroyed, there was enough time for the `SyncOrchestrator` to decide the sync service was stopped for no good reason since there is network connectivity and the app is on foreground, so it restarted the sync, with `pos=0`.
4. This value was persisted, and the next time the new client did an 'initial sync' it had a `pos != 0`, which the HS uses to avoid returning some required state it already fetched.
5. The room list now lacks state events for lots of rooms and room names, avatars, members, etc. are now missing.

---------

Co-authored-by: Benoit Marty <benoitm@element.io>

appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt

@@ -115,7 +115,7 @@ class MatrixSessionCache(
 
     private fun onNewMatrixClient(matrixClient: MatrixClient) {
         val syncOrchestrator = syncOrchestratorFactory.create(
-            syncService = matrixClient.syncService,
+            matrixClient = matrixClient,
             sessionCoroutineScope = matrixClient.sessionCoroutineScope,
         )
         sessionIdsToMatrixSession[matrixClient.sessionId] = InMemoryMatrixSession(

appnav/src/main/kotlin/io/element/android/appnav/di/SyncOrchestrator.kt

@@ -16,7 +16,7 @@ import io.element.android.features.networkmonitor.api.NetworkMonitor
 import io.element.android.features.networkmonitor.api.NetworkStatus
 import io.element.android.libraries.core.coroutine.CoroutineDispatchers
 import io.element.android.libraries.core.coroutine.childScope
-import io.element.android.libraries.matrix.api.sync.SyncService
+import io.element.android.libraries.matrix.api.MatrixClient
 import io.element.android.libraries.matrix.api.sync.SyncState
 import io.element.android.services.analytics.api.AnalyticsService
 import io.element.android.services.analytics.api.recordTransaction
@@ -37,7 +37,7 @@ import kotlin.time.Duration.Companion.seconds
 
 @AssistedInject
 class SyncOrchestrator(
-    @Assisted private val syncService: SyncService,
+    @Assisted private val matrixClient: MatrixClient,
     @Assisted sessionCoroutineScope: CoroutineScope,
     private val appForegroundStateService: AppForegroundStateService,
     private val networkMonitor: NetworkMonitor,
@@ -47,13 +47,15 @@ class SyncOrchestrator(
     @AssistedFactory
     interface Factory {
         fun create(
-            syncService: SyncService,
+            matrixClient: MatrixClient,
             sessionCoroutineScope: CoroutineScope,
         ): SyncOrchestrator
     }
 
     private val tag = "SyncOrchestrator"
 
+    private val syncService = matrixClient.syncService
+
     private val coroutineScope = sessionCoroutineScope.childScope(dispatchers.io, tag)
 
     private val started = AtomicBoolean(false)
@@ -94,7 +96,7 @@ class SyncOrchestrator(
             appForegroundStateService.isInCall,
             appForegroundStateService.isSyncingNotificationEvent,
             appForegroundStateService.hasRingingCall,
-            appForegroundStateService.isSharingLiveLocation
+            appForegroundStateService.isSharingLiveLocation,
         )
         val isAppActiveFlow = combine(isAppActiveFlows) { actives -> actives.any { it } }
         combine(
@@ -103,6 +105,11 @@ class SyncOrchestrator(
             networkMonitor.connectivity,
             isAppActiveFlow,
         ) { syncState, networkState, isAppActive ->
+            if (matrixClient.isShuttingDown) {
+                Timber.tag(tag).d("Matrix client is shutting down, no need to start the sync service again")
+                return@combine SyncStateAction.NoOp
+            }
+
             val isNetworkAvailable = networkState == NetworkStatus.Connected
 
             Timber.tag(tag).d("isAppActive=$isAppActive, isNetworkAvailable=$isNetworkAvailable")

appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt

@@ -12,6 +12,7 @@ import io.element.android.appnav.di.SyncOrchestrator
 import io.element.android.features.networkmonitor.api.NetworkStatus
 import io.element.android.features.networkmonitor.test.FakeNetworkMonitor
 import io.element.android.libraries.matrix.api.sync.SyncState
+import io.element.android.libraries.matrix.test.FakeMatrixClient
 import io.element.android.libraries.matrix.test.sync.FakeSyncService
 import io.element.android.services.analytics.test.FakeAnalyticsService
 import io.element.android.services.appnavstate.test.FakeAppForegroundStateService
@@ -40,7 +41,7 @@ class SyncOrchestratorTest {
         }
         val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Disconnected)
         val syncOrchestrator = createSyncOrchestrator(
-            syncService = syncService,
+            matrixClient = FakeMatrixClient(syncService = syncService),
             networkMonitor = networkMonitor,
         )
 
@@ -60,7 +61,7 @@ class SyncOrchestratorTest {
         }
         val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Connected)
         val syncOrchestrator = createSyncOrchestrator(
-            syncService = syncService,
+            matrixClient = FakeMatrixClient(syncService = syncService),
             networkMonitor = networkMonitor,
         )
 
@@ -85,7 +86,7 @@ class SyncOrchestratorTest {
         val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Connected)
         val appForegroundStateService = FakeAppForegroundStateService(initialForegroundValue = true)
         val syncOrchestrator = createSyncOrchestrator(
-            syncService = syncService,
+            matrixClient = FakeMatrixClient(syncService = syncService),
             networkMonitor = networkMonitor,
             appForegroundStateService = appForegroundStateService,
         )
@@ -117,7 +118,7 @@ class SyncOrchestratorTest {
         val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Connected)
         val appForegroundStateService = FakeAppForegroundStateService(initialForegroundValue = true)
         val syncOrchestrator = createSyncOrchestrator(
-            syncService = syncService,
+            matrixClient = FakeMatrixClient(syncService = syncService),
             networkMonitor = networkMonitor,
             appForegroundStateService = appForegroundStateService,
         )
@@ -165,7 +166,7 @@ class SyncOrchestratorTest {
             initialIsSyncingNotificationEventValue = false,
         )
         val syncOrchestrator = createSyncOrchestrator(
-            syncService = syncService,
+            matrixClient = FakeMatrixClient(syncService = syncService),
             networkMonitor = networkMonitor,
             appForegroundStateService = appForegroundStateService,
         )
@@ -208,7 +209,7 @@ class SyncOrchestratorTest {
             initialIsSyncingNotificationEventValue = false,
         )
         val syncOrchestrator = createSyncOrchestrator(
-            syncService = syncService,
+            matrixClient = FakeMatrixClient(syncService = syncService),
             networkMonitor = networkMonitor,
             appForegroundStateService = appForegroundStateService,
         )
@@ -252,7 +253,7 @@ class SyncOrchestratorTest {
             initialHasRingingCall = false,
         )
         val syncOrchestrator = createSyncOrchestrator(
-            syncService = syncService,
+            matrixClient = FakeMatrixClient(syncService = syncService),
             networkMonitor = networkMonitor,
             appForegroundStateService = appForegroundStateService,
         )
@@ -296,7 +297,7 @@ class SyncOrchestratorTest {
             initialIsInCallValue = true,
         )
         val syncOrchestrator = createSyncOrchestrator(
-            syncService = syncService,
+            matrixClient = FakeMatrixClient(syncService = syncService),
             networkMonitor = networkMonitor,
             appForegroundStateService = appForegroundStateService,
         )
@@ -339,7 +340,7 @@ class SyncOrchestratorTest {
             initialIsInCallValue = false,
         )
         val syncOrchestrator = createSyncOrchestrator(
-            syncService = syncService,
+            matrixClient = FakeMatrixClient(syncService = syncService),
             networkMonitor = networkMonitor,
             appForegroundStateService = appForegroundStateService,
         )
@@ -369,7 +370,7 @@ class SyncOrchestratorTest {
         }
         val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Disconnected)
         val syncOrchestrator = createSyncOrchestrator(
-            syncService = syncService,
+            matrixClient = FakeMatrixClient(syncService = syncService),
             networkMonitor = networkMonitor,
         )
 
@@ -381,12 +382,44 @@ class SyncOrchestratorTest {
         startSyncRecorder.assertions().isNeverCalled()
     }
 
+    @Test
+    fun `when the client is shutting down, the orchestrator does nothing`() = runTest {
+        val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Disconnected)
+        val appForegroundStateService = FakeAppForegroundStateService(
+            initialForegroundValue = false,
+            initialIsSyncingNotificationEventValue = false,
+            initialIsInCallValue = false,
+        )
+
+        val startSyncRecorder = lambdaRecorder<Result<Unit>> { Result.success(Unit) }
+        val syncService = FakeSyncService(initialSyncState = SyncState.Idle).apply {
+            startSyncLambda = startSyncRecorder
+        }
+        val syncOrchestrator = createSyncOrchestrator(
+            matrixClient = FakeMatrixClient(syncService = syncService, isShuttingDownResult = { true }),
+            networkMonitor = networkMonitor,
+            appForegroundStateService = appForegroundStateService,
+        )
+
+        // We start observing
+        syncOrchestrator.observeStates()
+
+        // These should still not trigger a sync, sync the client is shutting down
+        networkMonitor.givenNetworkBlocked(false)
+        appForegroundStateService.updateIsInCallState(true)
+        appForegroundStateService.isInForeground.value = true
+
+        advanceTimeBy(10.seconds)
+
+        startSyncRecorder.assertions().isNeverCalled()
+    }
+
     private fun TestScope.createSyncOrchestrator(
-        syncService: FakeSyncService = FakeSyncService(),
+        matrixClient: FakeMatrixClient = FakeMatrixClient(),
         networkMonitor: FakeNetworkMonitor = FakeNetworkMonitor(),
         appForegroundStateService: FakeAppForegroundStateService = FakeAppForegroundStateService(),
     ) = SyncOrchestrator(
-        syncService = syncService,
+        matrixClient = matrixClient,
         sessionCoroutineScope = backgroundScope,
         networkMonitor = networkMonitor,
         appForegroundStateService = appForegroundStateService,

appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt

@@ -11,7 +11,7 @@ package io.element.android.appnav.di
 import com.bumble.appyx.core.state.MutableSavedStateMapImpl
 import com.google.common.truth.Truth.assertThat
 import io.element.android.features.networkmonitor.test.FakeNetworkMonitor
-import io.element.android.libraries.matrix.api.sync.SyncService
+import io.element.android.libraries.matrix.api.MatrixClient
 import io.element.android.libraries.matrix.test.A_SESSION_ID
 import io.element.android.libraries.matrix.test.FakeMatrixClient
 import io.element.android.libraries.matrix.test.auth.FakeMatrixAuthenticationService
@@ -138,11 +138,11 @@ class MatrixSessionCacheTest {
 
         return object : SyncOrchestrator.Factory {
             override fun create(
-                syncService: SyncService,
+                matrixClient: MatrixClient,
                 sessionCoroutineScope: CoroutineScope,
             ): SyncOrchestrator {
                 return SyncOrchestrator(
-                    syncService = syncService,
+                    matrixClient = matrixClient,
                     sessionCoroutineScope = sessionCoroutineScope,
                     appForegroundStateService = FakeAppForegroundStateService(),
                     networkMonitor = FakeNetworkMonitor(),

libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/MatrixClient.kt

@@ -136,6 +136,9 @@ interface MatrixClient : UrlContentFetcher {
     /** Scans media content before it is displayed, or `null` when no content scanner is configured for this session. */
     val contentScanner: ContentScanner?
 
+    /** Whether the session is in the process of shutting down, either because it is being logged out or its cache is being cleared. */
+    val isShuttingDown: Boolean
+
     /**
      * Returns the room with the given id if the user has joined it, along with its live [Timeline].
      * A new instance is created on each call and the caller owns it, so it must be closed once no longer needed.

libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt

@@ -135,6 +135,7 @@ import org.matrix.rustcomponents.sdk.use
 import timber.log.Timber
 import java.io.File
 import java.util.Optional
+import java.util.concurrent.atomic.AtomicBoolean
 import kotlin.jvm.optionals.getOrNull
 import kotlin.time.Duration
 import kotlin.time.Duration.Companion.seconds
@@ -304,6 +305,10 @@ class RustMatrixClient(
         .buffer(Channel.UNLIMITED)
         .stateIn(sessionCoroutineScope, started = SharingStarted.Eagerly, initialValue = persistentListOf())
 
+    private val _isShuttingDown = AtomicBoolean(false)
+    override val isShuttingDown: Boolean
+        get() = _isShuttingDown.get()
+
     init {
         // Make sure the session delegate has a reference to the client to be able to logout on auth error
         sessionDelegate.bindClient(this)
@@ -666,11 +671,13 @@ class RustMatrixClient(
     }
 
     override suspend fun clearCache() {
+        _isShuttingDown.set(true)
         innerClient.clearCaches(innerSyncService)
         destroy()
     }
 
     override suspend fun logout(userInitiated: Boolean, ignoreSdkError: Boolean) {
+        _isShuttingDown.set(true)
         sessionCoroutineScope.cancel()
         // Remove current delegate so we don't receive an auth error
         clientDelegateTaskHandle?.cancelAndDestroy()
@@ -708,6 +715,8 @@ class RustMatrixClient(
     }
 
     override suspend fun deactivateAccount(password: String, eraseData: Boolean): Result<Unit> = withContext(sessionDispatcher) {
+        _isShuttingDown.set(true)
+
         Timber.w("Deactivating account")
         // Remove current delegate so we don't receive an auth error
         clientDelegateTaskHandle?.cancelAndDestroy()

libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/FakeMatrixClient.kt

@@ -133,6 +133,7 @@ class FakeMatrixClient(
     private val resetWellKnownConfigLambda: () -> Result<Unit> = { lambdaError() },
     private val enableAutomaticCallStatusLambda: (Boolean) -> Unit = { },
     override val contentScanner: ContentScanner? = null,
+    private val isShuttingDownResult: () -> Boolean = { false },
 ) : MatrixClient {
     var setDisplayNameCalled: Boolean = false
         private set
@@ -305,6 +306,9 @@ class FakeMatrixClient(
         return knockRoomLambda(roomIdOrAlias, message, serverNames)
     }
 
+    override val isShuttingDown: Boolean
+        get() = isShuttingDownResult()
+
     // Mocks
 
     fun givenCreateRoomResult(result: Result<RoomId>) {