From b9ca7f422b4e4cd6abe1274489e96f763fa80c5e Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 21 Sep 2026 17:01:12 +0200 Subject: [PATCH 01/11] refactor(conversations): make the room list sync awaitable getRooms launches into the repository's own scope and returns straight away, which is what the conversation list wants and what a background worker cannot use: WorkManager tears the process down the moment the worker returns, and the sync would be cut off mid-request. Add a suspend entry point that completes when the sync and the message catch-up it triggers are done. The catch-up is launched detached on the existing path and awaited on this one, so a worker cannot report success for messages it never fetched. It deliberately leaves the observed account alone. That selects what the conversation list screen shows, and a worker walking several accounts in one run must not move it. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../data/OfflineConversationsRepository.kt | 18 ++++++++++++++++++ .../OfflineFirstConversationsRepository.kt | 18 ++++++++++++++++-- .../OfflineFirstConversationsRepositoryTest.kt | 11 +++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt index 5ac4e201eae..4ce6497c19b 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt @@ -50,6 +50,18 @@ interface OfflineConversationsRepository { @Deprecated("use observeConversation") fun getRooms(user: User, forceFullSync: Boolean = false): Job + /** + * Synchronizes [user]'s conversations with the server and returns once that sync and the + * message catch-up it triggers are done, reporting whether it worked. + * + * [getRooms] launches into the repository's own scope and returns immediately, which is what + * the conversation list wants and what a background worker cannot use: WorkManager tears the + * process down once the worker returns, mid-request. This does not select the observed account + * either - that is what the conversation list screen shows, and a worker walking several + * accounts must not move it. + */ + suspend fun syncRooms(user: User, forceFullSync: Boolean = false): Boolean + /** * Called once onStart to emit a conversation to [conversationFlow] * to be handled asynchronously. @@ -76,5 +88,11 @@ interface OfflineConversationsRepository { */ fun requireFullSync(accountId: Long) + /** + * When the conversation list of the account with the internal id [accountId] was last fetched + * in full, or null when it never was. + */ + fun lastFullSyncAt(accountId: Long): Long? + fun observeConversation(accountId: Long, roomToken: String): Flow } diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt index 00a47446938..680692952c3 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt @@ -105,6 +105,9 @@ class OfflineFirstConversationsRepository @Inject constructor( } } + override suspend fun syncRooms(user: User, forceFullSync: Boolean): Boolean = + getRoomsFromServer(user, forceFullSync = forceFullSync, awaitCatchUp = true) != null + @Suppress("Detekt.TooGenericExceptionCaught") override fun getRoom(user: User, roomToken: String): Job = scope.launch { @@ -152,7 +155,11 @@ class OfflineFirstConversationsRepository @Inject constructor( } @Suppress("Detekt.TooGenericExceptionCaught") - private suspend fun getRoomsFromServer(user: User, forceFullSync: Boolean = false): List? { + private suspend fun getRoomsFromServer( + user: User, + forceFullSync: Boolean = false, + awaitCatchUp: Boolean = false + ): List? { var conversationsFromSync: List? = null if (!networkMonitor.isOnline.value) { @@ -206,12 +213,17 @@ class OfflineFirstConversationsRepository @Inject constructor( rememberSyncedState(accountId, roomList) val roomsWithNewMessages = getRoomsWithNewMessages(conversationsFromSync, previousConversations) - scope.launch { catchUpRoomsWithNewMessages(user, roomsWithNewMessages) } + if (awaitCatchUp) { + catchUpRoomsWithNewMessages(user, roomsWithNewMessages) + } else { + scope.launch { catchUpRoomsWithNewMessages(user, roomsWithNewMessages) } + } } catch (e: CancellationException) { throw e } catch (e: Exception) { Log.e(TAG, "Something went wrong when fetching conversations", e) storeTimestamp(accountId, KEY_MODIFIED_SINCE, null) + conversationsFromSync = null val hasCachedConversations = dao.getConversationsForUser(accountId).first().isNotEmpty() if (!hasCachedConversations) { _syncErrorFlow.emit(e) @@ -224,6 +236,8 @@ class OfflineFirstConversationsRepository @Inject constructor( storeTimestamp(accountId, KEY_LAST_FULL_SYNC_AT, null) } + override fun lastFullSyncAt(accountId: Long): Long? = readTimestamp(accountId, KEY_LAST_FULL_SYNC_AT) + /** * The value to send as `modifiedSince`, or null when this sync has to be a full one. * diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt index 9e00d419a64..9ac4ac25601 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt @@ -145,6 +145,17 @@ class OfflineFirstConversationsRepositoryTest { assertEquals(ROOM_TOKEN, entityCaptor.firstValue.first().token) } + @Test + fun `syncRooms reports a sync whose database write failed as failed`() = + runBlocking { + val room = conversation(token = ROOM_TOKEN, lastActivity = 5, unreadMessages = 0) + whenever(network.getRooms(any(), any(), any(), anyOrNull())).thenReturn(roomList(listOf(room))) + wheneverBlocking { dao.syncConversationsForUser(any(), any(), any()) } + .thenThrow(IllegalStateException("database is gone")) + + assertEquals(false, repository.syncRooms(user())) + } + @Test fun `getRooms skips deleting local conversations when the server returns an empty list`() = runBlocking { From f481d169811f5dab114ad8318aaaeff3b1959f03 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 21 Sep 2026 17:03:47 +0200 Subject: [PATCH 02/11] feat(conversations): sync conversations periodically in background A backgrounded app learns nothing about its conversations until a push arrives. Where push is disabled, unavailable or simply does not turn up, that means the list, the unread counts and the cached messages stay as they were until the user opens the app. Add a periodic worker that runs the conversation list sync and its message catch-up for every configured account. Accounts are synced one after another rather than at once, because each fans out into its own bounded set of message requests and running them together multiplies that into a burst on a single wake-up. It stands down in battery saver and while the app is in the foreground, where the list refreshes itself, and leaves reachability to the work request's network constraint - the connectivity flow is frozen in a process with no UI collector, and pre-checking it there is what silently disabled the message prefetch before. WorkManager's period floor is 15 minutes and Doze defers it further, so this bounds staleness rather than delivering immediacy. Scheduling keeps an existing run instead of replacing it, so an app opened often still reaches one. The battery saver check would have been copied a third time here, so it moves to a Context extension the repository and both workers share. The worker's decision is kept apart from doWork, which reaches for the application singleton to inject itself and can therefore not be driven from a unit test. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../application/NextcloudTalkApplication.kt | 3 + .../talk/extensions/ContextExtensions.kt | 13 ++ .../talk/jobs/ChatMessageCatchUpWorker.kt | 9 +- .../talk/jobs/ConversationsSyncWorker.kt | 161 ++++++++++++++++++ 4 files changed, 179 insertions(+), 7 deletions(-) create mode 100644 app/src/main/java/com/nextcloud/talk/extensions/ContextExtensions.kt create mode 100644 app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt diff --git a/app/src/main/java/com/nextcloud/talk/application/NextcloudTalkApplication.kt b/app/src/main/java/com/nextcloud/talk/application/NextcloudTalkApplication.kt index cec49766a69..2b592961139 100644 --- a/app/src/main/java/com/nextcloud/talk/application/NextcloudTalkApplication.kt +++ b/app/src/main/java/com/nextcloud/talk/application/NextcloudTalkApplication.kt @@ -51,6 +51,7 @@ import com.nextcloud.talk.dagger.modules.UtilsModule import com.nextcloud.talk.dagger.modules.ViewModelModule import com.nextcloud.talk.filebrowser.webdav.DavUtils import com.nextcloud.talk.jobs.AccountRemovalWorker +import com.nextcloud.talk.jobs.ConversationsSyncWorker import com.nextcloud.talk.jobs.CapabilitiesSyncWorker import com.nextcloud.talk.jobs.SignalingSettingsWorker import com.nextcloud.talk.jobs.WebsocketConnectionsWorker @@ -271,6 +272,8 @@ class NextcloudTalkApplication : ExistingPeriodicWorkPolicy.REPLACE, periodicCapabilitiesUpdateWork ) + + ConversationsSyncWorker.schedule(applicationContext) } override fun onTerminate() { diff --git a/app/src/main/java/com/nextcloud/talk/extensions/ContextExtensions.kt b/app/src/main/java/com/nextcloud/talk/extensions/ContextExtensions.kt new file mode 100644 index 00000000000..7cda514f0d6 --- /dev/null +++ b/app/src/main/java/com/nextcloud/talk/extensions/ContextExtensions.kt @@ -0,0 +1,13 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Andy Scherzinger + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.extensions + +import android.content.Context +import android.os.PowerManager + +/** Whether the device is currently in battery saver mode. */ +fun Context.isPowerSaveMode(): Boolean = (getSystemService(Context.POWER_SERVICE) as PowerManager).isPowerSaveMode diff --git a/app/src/main/java/com/nextcloud/talk/jobs/ChatMessageCatchUpWorker.kt b/app/src/main/java/com/nextcloud/talk/jobs/ChatMessageCatchUpWorker.kt index cb57651cfa5..a3acb4fc61a 100644 --- a/app/src/main/java/com/nextcloud/talk/jobs/ChatMessageCatchUpWorker.kt +++ b/app/src/main/java/com/nextcloud/talk/jobs/ChatMessageCatchUpWorker.kt @@ -7,7 +7,6 @@ package com.nextcloud.talk.jobs import android.content.Context -import android.os.PowerManager import android.util.Log import androidx.work.BackoffPolicy import androidx.work.Constraints @@ -22,6 +21,7 @@ import androidx.work.WorkerParameters import autodagger.AutoInjector import com.nextcloud.talk.application.NextcloudTalkApplication import com.nextcloud.talk.application.NextcloudTalkApplication.Companion.sharedApplication +import com.nextcloud.talk.extensions.isPowerSaveMode import com.nextcloud.talk.chat.data.network.ChatMessageSyncer import com.nextcloud.talk.data.database.dao.ConversationsDao import com.nextcloud.talk.users.UserManager @@ -71,7 +71,7 @@ class ChatMessageCatchUpWorker(context: Context, workerParams: WorkerParameters) Result.failure() } - isPowerSaveMode() -> { + applicationContext.isPowerSaveMode() -> { Log.d(TAG, "Battery saver is active, skipping message catch-up for room $roomToken") Result.success() } @@ -127,11 +127,6 @@ class ChatMessageCatchUpWorker(context: Context, workerParams: WorkerParameters) private fun retryOrFail(): Result = if (runAttemptCount < MAX_RUN_ATTEMPTS - 1) Result.retry() else Result.failure() - private fun isPowerSaveMode(): Boolean { - val powerManager = applicationContext.getSystemService(Context.POWER_SERVICE) as PowerManager - return powerManager.isPowerSaveMode - } - companion object { private val TAG: String = ChatMessageCatchUpWorker::class.java.simpleName private const val CHAT_API_VERSION = 1 diff --git a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt new file mode 100644 index 00000000000..c0b1a8f32f3 --- /dev/null +++ b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt @@ -0,0 +1,161 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Andy Scherzinger + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.jobs + +import android.content.Context +import android.util.Log +import androidx.annotation.VisibleForTesting +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.ProcessLifecycleOwner +import androidx.work.Constraints +import androidx.work.CoroutineWorker +import androidx.work.ExistingPeriodicWorkPolicy +import androidx.work.NetworkType +import androidx.work.PeriodicWorkRequest +import androidx.work.WorkManager +import androidx.work.WorkerParameters +import autodagger.AutoInjector +import com.nextcloud.talk.application.NextcloudTalkApplication +import com.nextcloud.talk.application.NextcloudTalkApplication.Companion.sharedApplication +import com.nextcloud.talk.conversationlist.data.OfflineConversationsRepository +import com.nextcloud.talk.data.user.model.User +import com.nextcloud.talk.extensions.isPowerSaveMode +import com.nextcloud.talk.users.UserManager +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.TimeoutCancellationException +import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeout +import java.util.concurrent.TimeUnit +import javax.inject.Inject + +/** + * Periodic worker that syncs the conversation list, and the messages that sync prefetches, for + * every configured account. + * + * Accounts are synced one after another. The run is skipped in battery saver mode and while the app + * is in the foreground, and a run in which any account failed is retried up to [MAX_RUN_ATTEMPTS] + * times. Network availability is enforced by the [NetworkType.CONNECTED] constraint on the request + * rather than checked here. + */ +@AutoInjector(NextcloudTalkApplication::class) +class ConversationsSyncWorker(context: Context, workerParams: WorkerParameters) : + CoroutineWorker(context, workerParams) { + + @Inject + lateinit var userManager: UserManager + + @Inject + lateinit var conversationsRepository: OfflineConversationsRepository + + override suspend fun doWork(): Result { + sharedApplication!!.componentApplication.inject(this) + return sync() + } + + /** Runs the sync, or reports success without syncing when a guard stands the run down. */ + @VisibleForTesting + internal suspend fun sync(): Result = + when { + applicationContext.isPowerSaveMode() -> { + Log.d(TAG, "Battery saver is active, skipping the background conversation sync") + Result.success() + } + + isAppInForeground() -> { + Log.d(TAG, "App is in the foreground, skipping the background conversation sync") + Result.success() + } + + else -> syncAccounts() + } + + private suspend fun syncAccounts(): Result { + val accounts = runCatching { userManager.getUsers() }.getOrElse { throwable -> + if (throwable is CancellationException) throw throwable + Log.e(TAG, "Could not read the accounts to sync", throwable) + return retryOrFail() + } + + if (accounts.isEmpty()) { + Log.d(TAG, "No account to sync") + return Result.success() + } + + // WorkManager stops a run after ten minutes, so a fixed order would let slow accounts at + // the front keep the ones behind them from ever being reached + val longestWaitingFirst = accounts.sortedBy { conversationsRepository.lastFullSyncAt(it.id!!) ?: 0L } + + val failed = longestWaitingFirst.count { !syncAccount(it) } + + return if (failed == 0) { + Result.success() + } else { + Log.w(TAG, "$failed of ${accounts.size} accounts did not sync (attempt ${runAttemptCount + 1})") + retryOrFail() + } + } + + /** + * Syncs a single account, returning whether it succeeded. Failures are logged; the run being + * cancelled propagates. + * + * The sync is given at most [ACCOUNT_TIMEOUT_MINUTES] minutes. An account that cannot finish + * would otherwise be able to spend the whole execution window WorkManager grants the run, and + * since its stored timestamp never advances it would keep sorting first and keep the accounts + * behind it from ever being reached. + */ + private suspend fun syncAccount(user: User): Boolean = + runCatching { + withTimeout(TimeUnit.MINUTES.toMillis(ACCOUNT_TIMEOUT_MINUTES)) { + conversationsRepository.syncRooms(user) + } + }.getOrElse { throwable -> + if (throwable is CancellationException && throwable !is TimeoutCancellationException) throw throwable + if (throwable is TimeoutCancellationException) { + Log.w(TAG, "Background conversation sync for account ${user.id} ran out of its time budget") + } else { + Log.e(TAG, "Background conversation sync failed for account ${user.id}", throwable) + } + false + } + + private fun retryOrFail(): Result = if (runAttemptCount < MAX_RUN_ATTEMPTS - 1) Result.retry() else Result.failure() + + private suspend fun isAppInForeground(): Boolean = + withContext(Dispatchers.Main.immediate) { + ProcessLifecycleOwner.get().lifecycle.currentState.isAtLeast(Lifecycle.State.STARTED) + } + + companion object { + private val TAG: String = ConversationsSyncWorker::class.java.simpleName + private const val MAX_RUN_ATTEMPTS = 3 + private const val ACCOUNT_TIMEOUT_MINUTES = 2L + private const val REPEAT_INTERVAL_MINUTES = 15L + const val UNIQUE_WORK_NAME = "PeriodicConversationsSync" + + /** + * Schedules the worker to run every [REPEAT_INTERVAL_MINUTES] minutes while a network is + * available, leaving an already scheduled run in place. + */ + fun schedule(context: Context) { + val work = PeriodicWorkRequest.Builder( + ConversationsSyncWorker::class.java, + REPEAT_INTERVAL_MINUTES, + TimeUnit.MINUTES + ).setConstraints( + Constraints.Builder().setRequiredNetworkType(NetworkType.CONNECTED).build() + ).build() + + WorkManager.getInstance(context).enqueueUniquePeriodicWork( + UNIQUE_WORK_NAME, + ExistingPeriodicWorkPolicy.KEEP, + work + ) + } + } +} From 31c8d72171df76d563918ea0099ccd0eaad789bb Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 21 Sep 2026 17:15:49 +0200 Subject: [PATCH 03/11] test(conversations): cover the periodic conversation sync worker What is worth pinning down about a background worker is when it stands down and how many accounts it touches when it does not: a guard that quietly stops working is invisible until it turns up as battery drain or request volume, long after the change that broke it. Covers battery saver standing the run down without reading a single account, every configured account being synced rather than only the current one, a failed account asking for another attempt while its neighbours still sync, the attempt cap ending the retries, and an account lookup that throws being survivable. Unit tests gain the WorkManager testing artifact, which was available to instrumented tests only. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- app/build.gradle.kts | 1 + .../talk/jobs/ConversationsSyncWorkerTest.kt | 198 ++++++++++++++++++ 2 files changed, 199 insertions(+) create mode 100644 app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt diff --git a/app/build.gradle.kts b/app/build.gradle.kts index bf70a909531..031159c1ec6 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -373,6 +373,7 @@ dependencies { androidTestImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:1.11.0") androidTestImplementation("androidx.test:core-ktx:1.7.0") androidTestImplementation("org.mockito:mockito-android:5.22.0") + testImplementation("androidx.work:work-testing:$workVersion") androidTestImplementation("androidx.work:work-testing:$workVersion") androidTestImplementation("androidx.test.espresso:espresso-core:$espressoVersion") { exclude(group = "com.android.support", module = "support-annotations") diff --git a/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt b/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt new file mode 100644 index 00000000000..7f8507038b9 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt @@ -0,0 +1,198 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Andy Scherzinger + * SPDX-License-Identifier: GPL-3.0-or-later + */ + +package com.nextcloud.talk.jobs + +import android.app.Application +import android.content.Context +import android.os.PowerManager +import androidx.test.core.app.ApplicationProvider +import androidx.work.ListenableWorker +import androidx.work.testing.TestListenableWorkerBuilder +import com.nextcloud.talk.conversationlist.data.OfflineConversationsRepository +import com.nextcloud.talk.data.user.model.User +import com.nextcloud.talk.users.UserManager +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.awaitCancellation +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertThrows +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.kotlin.any +import org.mockito.kotlin.argThat +import org.mockito.kotlin.argumentCaptor +import org.mockito.kotlin.doReturn +import org.mockito.kotlin.doSuspendableAnswer +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.stub +import org.mockito.kotlin.times +import org.mockito.kotlin.verifyBlocking +import org.mockito.kotlin.whenever +import org.mockito.kotlin.wheneverBlocking +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config + +/** + * Tests for [ConversationsSyncWorker]: which accounts a run syncs, when a run stands down without + * syncing, and how a failed run reports itself. + */ +@RunWith(RobolectricTestRunner::class) +@Config(application = Application::class, sdk = [33]) +class ConversationsSyncWorkerTest { + + private val userManager: UserManager = mock() + private val repository: OfflineConversationsRepository = mock() + + @Test + fun `every account is synced, not just the current one`() { + val worker = worker() + wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2), user(3))) + wheneverBlocking { repository.syncRooms(any(), any()) }.thenReturn(true) + + val result = runBlocking { worker.sync() } + + assertEquals(ListenableWorker.Result.success(), result) + verifyBlocking(repository) { syncRooms(user(1), false) } + verifyBlocking(repository) { syncRooms(user(2), false) } + verifyBlocking(repository) { syncRooms(user(3), false) } + } + + @Test + fun `battery saver stands the sync down entirely`() { + val worker = worker() + shadowOf(applicationContext().getSystemService(Context.POWER_SERVICE) as PowerManager) + .setIsPowerSaveMode(true) + + val result = runBlocking { worker.sync() } + + assertEquals(ListenableWorker.Result.success(), result) + verifyBlocking(userManager, never()) { getUsers() } + verifyBlocking(repository, never()) { syncRooms(any(), any()) } + } + + @Test + fun `an account with nothing to sync is not an error`() { + val worker = worker() + wheneverBlocking { userManager.getUsers() }.thenReturn(emptyList()) + + val result = runBlocking { worker.sync() } + + assertEquals(ListenableWorker.Result.success(), result) + verifyBlocking(repository, never()) { syncRooms(any(), any()) } + } + + @Test + fun `a failed account asks for another attempt`() { + val worker = worker(runAttempt = 0) + wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2))) + wheneverBlocking { repository.syncRooms(user(1), false) }.thenReturn(true) + wheneverBlocking { repository.syncRooms(user(2), false) }.thenReturn(false) + + val result = runBlocking { worker.sync() } + + assertEquals(ListenableWorker.Result.retry(), result) + verifyBlocking(repository) { syncRooms(user(1), false) } + } + + @Test + fun `a sync that keeps failing gives up instead of retrying for ever`() { + val worker = worker(runAttempt = 2) + wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1))) + wheneverBlocking { repository.syncRooms(any(), any()) }.thenReturn(false) + + val result = runBlocking { worker.sync() } + + assertEquals(ListenableWorker.Result.failure(), result) + } + + @Test + fun `the account that waited longest is synced first`() { + val worker = worker() + wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2), user(3))) + whenever(repository.lastFullSyncAt(1)).thenReturn(300) + whenever(repository.lastFullSyncAt(2)).thenReturn(null) + whenever(repository.lastFullSyncAt(3)).thenReturn(100) + wheneverBlocking { repository.syncRooms(any(), any()) }.thenReturn(true) + + runBlocking { worker.sync() } + + val synced = argumentCaptor() + verifyBlocking(repository, times(3)) { syncRooms(synced.capture(), any()) } + assertEquals(listOf(2L, 3L, 1L), synced.allValues.map { it.id }) + } + + @OptIn(ExperimentalCoroutinesApi::class) + @Test + fun `an account that never finishes does not keep the others from syncing`() = + runTest { + val worker = worker() + wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2))) + repository.stub { + onBlocking { syncRooms(argThat { id == 1L }, any()) } doSuspendableAnswer { awaitCancellation() } + onBlocking { syncRooms(argThat { id == 2L }, any()) } doReturn true + } + + val result = worker.sync() + + verifyBlocking(repository) { syncRooms(argThat { id == 2L }, any()) } + assertEquals(ListenableWorker.Result.retry(), result) + } + + @Test + fun `a failed account lookup is retried rather than reported as a sync`() { + val worker = worker() + wheneverBlocking { userManager.getUsers() }.thenThrow(IllegalStateException("database is gone")) + + val result = runBlocking { worker.sync() } + + assertEquals(ListenableWorker.Result.retry(), result) + verifyBlocking(repository, never()) { syncRooms(any(), any()) } + } + + @Test + fun `a last attempt whose account lookup fails reports failure`() { + val worker = worker(runAttempt = 2) + wheneverBlocking { userManager.getUsers() }.thenThrow(IllegalStateException("database is gone")) + + val result = runBlocking { worker.sync() } + + assertEquals(ListenableWorker.Result.failure(), result) + } + + @Test + fun `a cancelled run stops instead of syncing the remaining accounts`() { + val worker = worker() + wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2), user(3))) + wheneverBlocking { repository.syncRooms(any(), any()) }.thenThrow(CancellationException("run stopped")) + + assertThrows(CancellationException::class.java) { runBlocking { worker.sync() } } + + verifyBlocking(repository, times(1)) { syncRooms(any(), any()) } + } + + private fun worker(runAttempt: Int = 0): ConversationsSyncWorker = + TestListenableWorkerBuilder(applicationContext()) + .setRunAttemptCount(runAttempt) + .build() + .also { + it.userManager = userManager + it.conversationsRepository = repository + } + + private fun applicationContext(): Context = ApplicationProvider.getApplicationContext() + + private fun user(id: Long): User = User(id = id, userId = "user$id", username = "user$id", baseUrl = BASE_URL) + + companion object { + private const val BASE_URL = "https://server.example.com" + } +} From 5ddb0a8f8156c145e53450956efb7bcf36f7ab19 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Tue, 6 Oct 2026 11:36:29 +0200 Subject: [PATCH 04/11] fix(conversations): sync every room list before catching up messages The background worker gave each account two minutes for its whole sync, the message catch-up included. On a slow connection the catch-up alone can take longer than that - up to twenty rooms, each needing requests that may run into the 45 second HTTP timeouts - so an account whose room list was already stored got reported as failed, and the run was retried for every account. Timing only the room list fixes that, but leaves the catch-up running inline between accounts: one account's slow prefetch then delays the room lists of every account behind it, and can use up the ten minutes WorkManager grants a run before those are reached at all. The room list is what matters - unread counts, new and left conversations - while missed messages load anyway once a chat is opened. So the worker now works in two passes: it syncs the room list of every account first, under the per-account time budget, and only then catches up messages with the time the run has left. A catch-up failure is logged and no longer fails the run, since the list it follows is already stored. To allow that, syncRooms() no longer catches up itself: it returns the rooms with new messages, or null when the sync failed, and the new catchUpRooms() prefetches them. getRooms() keeps launching the catch-up in the background, so the conversation list screen behaves as before. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Marcel Hibbe --- .../data/OfflineConversationsRepository.kt | 24 +++- .../OfflineFirstConversationsRepository.kt | 133 ++++++++++-------- .../talk/jobs/ConversationsSyncWorker.kt | 60 +++++--- ...OfflineFirstConversationsRepositoryTest.kt | 49 ++++++- .../talk/jobs/ConversationsSyncWorkerTest.kt | 95 ++++++++----- 5 files changed, 238 insertions(+), 123 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt index 4ce6497c19b..36aeb398d15 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt @@ -8,6 +8,7 @@ package com.nextcloud.talk.conversationlist.data import com.nextcloud.talk.conversationlist.data.network.OfflineFirstConversationsRepository.ConversationResult +import com.nextcloud.talk.data.database.model.ConversationEntity import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.models.domain.ConversationModel import kotlinx.coroutines.Job @@ -51,16 +52,31 @@ interface OfflineConversationsRepository { fun getRooms(user: User, forceFullSync: Boolean = false): Job /** - * Synchronizes [user]'s conversations with the server and returns once that sync and the - * message catch-up it triggers are done, reporting whether it worked. + * Synchronizes [user]'s conversations with the server and returns once they are stored, + * handing back the rooms whose messages should be caught up, or null when the sync failed. + * The catch-up itself is left to the caller, through [catchUpRooms]. * * [getRooms] launches into the repository's own scope and returns immediately, which is what * the conversation list wants and what a background worker cannot use: WorkManager tears the * process down once the worker returns, mid-request. This does not select the observed account * either - that is what the conversation list screen shows, and a worker walking several - * accounts must not move it. + * accounts must not move it. Leaving the catch-up to the caller lets a worker sync the room + * lists of all accounts before it spends time on any account's messages. + * + * With [roomListTimeoutMillis] set, the sync is given at most that long and reports failure + * when it runs out. + */ + suspend fun syncRooms( + user: User, + forceFullSync: Boolean = false, + roomListTimeoutMillis: Long? = null + ): List? + + /** + * Prefetches the messages of [rooms], as returned by [syncRooms], into the local database and + * returns once that is done. Failures for single rooms are logged, not thrown. */ - suspend fun syncRooms(user: User, forceFullSync: Boolean = false): Boolean + suspend fun catchUpRooms(user: User, rooms: List) /** * Called once onStart to emit a conversation to [conversationFlow] diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt index 680692952c3..054ccc2a557 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt @@ -35,6 +35,7 @@ import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job +import kotlinx.coroutines.TimeoutCancellationException import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.distinctUntilChanged @@ -44,10 +45,12 @@ import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Semaphore import kotlinx.coroutines.sync.withPermit +import kotlinx.coroutines.withTimeout import javax.inject.Inject import kotlin.collections.map +import kotlin.time.Duration.Companion.milliseconds -@Suppress("LongParameterList") +@Suppress("LongParameterList", "TooManyFunctions") class OfflineFirstConversationsRepository @Inject constructor( private val dao: ConversationsDao, private val network: ConversationsNetworkDataSource, @@ -101,12 +104,19 @@ class OfflineFirstConversationsRepository @Inject constructor( override fun getRooms(user: User, forceFullSync: Boolean): Job = scope.launch { if (networkMonitor.isOnline.value) { - getRoomsFromServer(user, forceFullSync = forceFullSync) + val roomsWithNewMessages = getRoomsFromServer(user, forceFullSync = forceFullSync) + ?: return@launch + // a launch of its own, so the returned job completes without waiting for the catch-up + scope.launch { catchUpRooms(user, roomsWithNewMessages) } } } - override suspend fun syncRooms(user: User, forceFullSync: Boolean): Boolean = - getRoomsFromServer(user, forceFullSync = forceFullSync, awaitCatchUp = true) != null + override suspend fun syncRooms( + user: User, + forceFullSync: Boolean, + roomListTimeoutMillis: Long? + ): List? = + getRoomsFromServer(user, forceFullSync = forceFullSync, roomListTimeoutMillis = roomListTimeoutMillis) @Suppress("Detekt.TooGenericExceptionCaught") override fun getRoom(user: User, roomToken: String): Job = @@ -158,10 +168,8 @@ class OfflineFirstConversationsRepository @Inject constructor( private suspend fun getRoomsFromServer( user: User, forceFullSync: Boolean = false, - awaitCatchUp: Boolean = false + roomListTimeoutMillis: Long? = null ): List? { - var conversationsFromSync: List? = null - if (!networkMonitor.isOnline.value) { Log.d(TAG, "Device is offline, can't load conversations from server") return null @@ -170,66 +178,77 @@ class OfflineFirstConversationsRepository @Inject constructor( val accountId = user.id!! val modifiedSince = modifiedSinceFor(user, forceFullSync) - val includeStatus = modifiedSince == null && isUserStatusAvailable(user) - - try { - val roomList = withRetry( - retries = NETWORK_FETCH_RETRIES, - initialDelayMillis = NETWORK_FETCH_RETRY_INITIAL_DELAY_MS, - maxDelayMillis = NETWORK_FETCH_RETRY_MAX_DELAY_MS - ) { - network.getRooms(user, user.baseUrl!!, includeStatus, modifiedSince) - .subscribeOn(Schedulers.io()) - .observeOn(AndroidSchedulers.mainThread()) - .blockingSingle() - } - - conversationsFromSync = roomList.conversations.map { - it.asEntity(accountId) - } - - val previousConversations = dao.getConversationsForUser(accountId).first() - .associateBy { it.internalId } - - val serverItems = if (includeStatus) { - conversationsFromSync - } else { - keepCachedStatus(conversationsFromSync, previousConversations) - } - - dao.syncConversationsForUser( - accountId = accountId, - serverItems = conversationListUpdater.preservePendingLocalState( - previousConversations, - serverItems - ), - conversationIdsToDelete = if (roomList.wasDelta) { - emptyList() - } else { - determineLeftConversationIds(previousConversations, conversationsFromSync) - } - ) - - rememberSyncedState(accountId, roomList) - - val roomsWithNewMessages = getRoomsWithNewMessages(conversationsFromSync, previousConversations) - if (awaitCatchUp) { - catchUpRoomsWithNewMessages(user, roomsWithNewMessages) + return try { + if (roomListTimeoutMillis == null) { + syncRoomList(user, modifiedSince) } else { - scope.launch { catchUpRoomsWithNewMessages(user, roomsWithNewMessages) } + withTimeout(roomListTimeoutMillis.milliseconds) { syncRoomList(user, modifiedSince) } } + } catch (e: TimeoutCancellationException) { + // the caller's run goes on, so this is a failed sync rather than a cancellation + Log.w(TAG, "Room list sync for account $accountId ran out of its time budget") + null } catch (e: CancellationException) { throw e } catch (e: Exception) { Log.e(TAG, "Something went wrong when fetching conversations", e) storeTimestamp(accountId, KEY_MODIFIED_SINCE, null) - conversationsFromSync = null val hasCachedConversations = dao.getConversationsForUser(accountId).first().isNotEmpty() if (!hasCachedConversations) { _syncErrorFlow.emit(e) } + null } - return conversationsFromSync + } + + /** + * Fetches [user]'s room list, filtered by [modifiedSince] when set, and stores it, returning the + * rooms whose messages should be caught up. + */ + private suspend fun syncRoomList(user: User, modifiedSince: Long?): List { + val accountId = user.id!! + val includeStatus = modifiedSince == null && isUserStatusAvailable(user) + + val roomList = withRetry( + retries = NETWORK_FETCH_RETRIES, + initialDelayMillis = NETWORK_FETCH_RETRY_INITIAL_DELAY_MS, + maxDelayMillis = NETWORK_FETCH_RETRY_MAX_DELAY_MS + ) { + network.getRooms(user, user.baseUrl!!, includeStatus, modifiedSince) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .blockingSingle() + } + + val conversationsFromSync = roomList.conversations.map { + it.asEntity(accountId) + } + + val previousConversations = dao.getConversationsForUser(accountId).first() + .associateBy { it.internalId } + + val serverItems = if (includeStatus) { + conversationsFromSync + } else { + keepCachedStatus(conversationsFromSync, previousConversations) + } + + dao.syncConversationsForUser( + accountId = accountId, + serverItems = conversationListUpdater.preservePendingLocalState( + previousConversations, + serverItems + ), + conversationIdsToDelete = if (roomList.wasDelta) { + emptyList() + } else { + determineLeftConversationIds(previousConversations, conversationsFromSync) + } + ) + + rememberSyncedState(accountId, roomList) + + return getRoomsWithNewMessages(conversationsFromSync, previousConversations) } override fun requireFullSync(accountId: Long) { @@ -303,14 +322,14 @@ class OfflineFirstConversationsRepository @Inject constructor( /** * Prefetches the messages of [rooms] into the local database so they are instantly visible * when a chat is opened. Runs after the room list sync; failures are logged and never affect - * the conversation list itself. + * the conversation list itself, and cancellation propagates. * * The catch-up is skipped in battery saver mode and when background data is restricted on a * metered network (mirroring the Low Power Mode guard on iOS), and is bounded to the * [MAX_ROOMS_TO_CATCH_UP] most recently active rooms with [MAX_CONCURRENT_CATCH_UPS] parallel * requests, so a fresh install with many rooms cannot cause an unbounded request burst. */ - private suspend fun catchUpRoomsWithNewMessages(user: User, rooms: List) { + override suspend fun catchUpRooms(user: User, rooms: List) { if (rooms.isEmpty() || !isCatchUpAllowed(user)) { return } diff --git a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt index c0b1a8f32f3..47123b33a81 100644 --- a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt +++ b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt @@ -22,14 +22,13 @@ import autodagger.AutoInjector import com.nextcloud.talk.application.NextcloudTalkApplication import com.nextcloud.talk.application.NextcloudTalkApplication.Companion.sharedApplication import com.nextcloud.talk.conversationlist.data.OfflineConversationsRepository +import com.nextcloud.talk.data.database.model.ConversationEntity import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.extensions.isPowerSaveMode import com.nextcloud.talk.users.UserManager import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.TimeoutCancellationException import kotlinx.coroutines.withContext -import kotlinx.coroutines.withTimeout import java.util.concurrent.TimeUnit import javax.inject.Inject @@ -37,7 +36,9 @@ import javax.inject.Inject * Periodic worker that syncs the conversation list, and the messages that sync prefetches, for * every configured account. * - * Accounts are synced one after another. The run is skipped in battery saver mode and while the app + * Accounts are synced one after another, in two passes: first the room list of every account, + * then the messages of each. A slow message prefetch therefore cannot keep another account's + * room list from being synced. The run is skipped in battery saver mode and while the app * is in the foreground, and a run in which any account failed is retried up to [MAX_RUN_ATTEMPTS] * times. Network availability is enforced by the [NetworkType.CONNECTED] constraint on the request * rather than checked here. @@ -90,7 +91,14 @@ class ConversationsSyncWorker(context: Context, workerParams: WorkerParameters) // the front keep the ones behind them from ever being reached val longestWaitingFirst = accounts.sortedBy { conversationsRepository.lastFullSyncAt(it.id!!) ?: 0L } - val failed = longestWaitingFirst.count { !syncAccount(it) } + // room lists first, for every account: they are what the conversation list shows. Messages + // are prefetched only after that, with the time the run has left + val syncedAccounts = longestWaitingFirst.map { it to syncAccount(it) } + syncedAccounts.forEach { (user, roomsWithNewMessages) -> + roomsWithNewMessages?.let { catchUpAccount(user, it) } + } + + val failed = syncedAccounts.count { (_, roomsWithNewMessages) -> roomsWithNewMessages == null } return if (failed == 0) { Result.success() @@ -101,29 +109,37 @@ class ConversationsSyncWorker(context: Context, workerParams: WorkerParameters) } /** - * Syncs a single account, returning whether it succeeded. Failures are logged; the run being - * cancelled propagates. + * Syncs a single account's room list, returning the rooms whose messages should be caught up, + * or null when it failed. Failures are logged; the run being cancelled propagates. * - * The sync is given at most [ACCOUNT_TIMEOUT_MINUTES] minutes. An account that cannot finish - * would otherwise be able to spend the whole execution window WorkManager grants the run, and - * since its stored timestamp never advances it would keep sorting first and keep the accounts - * behind it from ever being reached. + * The sync is given at most [ROOM_LIST_TIMEOUT_MINUTES] minutes. An account whose list cannot be + * synced would otherwise be able to spend the whole execution window WorkManager grants the run, + * and since its stored timestamp never advances it would keep sorting first and keep the + * accounts behind it from ever being reached. */ - private suspend fun syncAccount(user: User): Boolean = + private suspend fun syncAccount(user: User): List? = runCatching { - withTimeout(TimeUnit.MINUTES.toMillis(ACCOUNT_TIMEOUT_MINUTES)) { - conversationsRepository.syncRooms(user) - } + conversationsRepository.syncRooms( + user, + roomListTimeoutMillis = TimeUnit.MINUTES.toMillis(ROOM_LIST_TIMEOUT_MINUTES) + ) }.getOrElse { throwable -> - if (throwable is CancellationException && throwable !is TimeoutCancellationException) throw throwable - if (throwable is TimeoutCancellationException) { - Log.w(TAG, "Background conversation sync for account ${user.id} ran out of its time budget") - } else { - Log.e(TAG, "Background conversation sync failed for account ${user.id}", throwable) - } - false + if (throwable is CancellationException) throw throwable + Log.e(TAG, "Background conversation sync failed for account ${user.id}", throwable) + null } + /** + * Prefetches the messages of an account's [rooms]. A failure is logged and does not fail the + * run: the room list is stored already, and the messages load when the chat is opened. + */ + private suspend fun catchUpAccount(user: User, rooms: List) { + runCatching { conversationsRepository.catchUpRooms(user, rooms) }.onFailure { throwable -> + if (throwable is CancellationException) throw throwable + Log.e(TAG, "Background message catch-up failed for account ${user.id}", throwable) + } + } + private fun retryOrFail(): Result = if (runAttemptCount < MAX_RUN_ATTEMPTS - 1) Result.retry() else Result.failure() private suspend fun isAppInForeground(): Boolean = @@ -134,7 +150,7 @@ class ConversationsSyncWorker(context: Context, workerParams: WorkerParameters) companion object { private val TAG: String = ConversationsSyncWorker::class.java.simpleName private const val MAX_RUN_ATTEMPTS = 3 - private const val ACCOUNT_TIMEOUT_MINUTES = 2L + private const val ROOM_LIST_TIMEOUT_MINUTES = 2L private const val REPEAT_INTERVAL_MINUTES = 15L const val UNIQUE_WORK_NAME = "PeriodicConversationsSync" diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt index 9ac4ac25601..4230c7e8587 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt @@ -29,6 +29,7 @@ import io.reactivex.Observable import io.reactivex.android.plugins.RxAndroidPlugins import io.reactivex.schedulers.Schedulers import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.awaitCancellation import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.first @@ -47,9 +48,11 @@ import org.mockito.Mockito.timeout import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argumentCaptor +import org.mockito.kotlin.doSuspendableAnswer import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never +import org.mockito.kotlin.stub import org.mockito.kotlin.verify import org.mockito.kotlin.verifyBlocking import org.mockito.kotlin.wheneverBlocking @@ -153,7 +156,50 @@ class OfflineFirstConversationsRepositoryTest { wheneverBlocking { dao.syncConversationsForUser(any(), any(), any()) } .thenThrow(IllegalStateException("database is gone")) - assertEquals(false, repository.syncRooms(user())) + assertNull(repository.syncRooms(user())) + } + + @Test + fun `syncRooms reports a room list sync that runs out of its time budget as failed`() = + runTest { + val room = conversation(token = ROOM_TOKEN, lastActivity = 5, unreadMessages = 2) + whenever(network.getRooms(any(), any(), any(), anyOrNull())).thenReturn(roomList(listOf(room))) + dao.stub { + onBlocking { syncConversationsForUser(any(), any(), any()) } doSuspendableAnswer { awaitCancellation() } + } + + assertNull(repository.syncRooms(user(), roomListTimeoutMillis = ROOM_LIST_TIMEOUT_MILLIS)) + verifyBlocking(chatMessageSyncer, never()) { catchUpRoom(any(), any(), anyOrNull(), any()) } + } + + @Test + fun `syncRooms hands the rooms with new messages back instead of catching them up`() = + runBlocking { + stubCatchUpRoom() + val room = conversation(token = ROOM_TOKEN, lastActivity = 5, unreadMessages = 2) + whenever(network.getRooms(any(), any(), any(), anyOrNull())).thenReturn(roomList(listOf(room))) + + val roomsWithNewMessages = repository.syncRooms(user()) + + assertEquals(listOf(ROOM_TOKEN), roomsWithNewMessages?.map { it.token }) + verifyBlocking(chatMessageSyncer, after(AFTER_DELAY_MILLIS).never()) { + catchUpRoom(any(), any(), anyOrNull(), any()) + } + } + + @Test + fun `catchUpRooms prefetches the messages of the rooms syncRooms handed back`() = + runBlocking { + stubCatchUpRoom() + val room = conversation(token = ROOM_TOKEN, lastActivity = 5, unreadMessages = 2) + whenever(network.getRooms(any(), any(), any(), anyOrNull())).thenReturn(roomList(listOf(room))) + val roomsWithNewMessages = repository.syncRooms(user())!! + + repository.catchUpRooms(user(), roomsWithNewMessages) + + val targetCaptor = argumentCaptor() + verifyBlocking(chatMessageSyncer) { catchUpRoom(targetCaptor.capture(), any(), anyOrNull(), any()) } + assertEquals(ROOM_TOKEN, targetCaptor.firstValue.roomToken) } @Test @@ -470,6 +516,7 @@ class OfflineFirstConversationsRepositoryTest { private const val AWAIT_TIMEOUT_MILLIS = 2000L private const val POLL_INTERVAL_MILLIS = 50L private const val COLLECTOR_STARTUP_MILLIS = 100L + private const val ROOM_LIST_TIMEOUT_MILLIS = 1000L } } diff --git a/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt b/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt index 7f8507038b9..bf5ac2678a4 100644 --- a/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt +++ b/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt @@ -14,25 +14,22 @@ import androidx.test.core.app.ApplicationProvider import androidx.work.ListenableWorker import androidx.work.testing.TestListenableWorkerBuilder import com.nextcloud.talk.conversationlist.data.OfflineConversationsRepository +import com.nextcloud.talk.data.database.model.ConversationEntity import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.users.UserManager import kotlinx.coroutines.CancellationException -import kotlinx.coroutines.awaitCancellation -import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.runBlocking -import kotlinx.coroutines.test.runTest import org.junit.Assert.assertEquals import org.junit.Assert.assertThrows import org.junit.Test import org.junit.runner.RunWith import org.mockito.kotlin.any -import org.mockito.kotlin.argThat +import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argumentCaptor -import org.mockito.kotlin.doReturn -import org.mockito.kotlin.doSuspendableAnswer +import org.mockito.kotlin.eq +import org.mockito.kotlin.inOrder import org.mockito.kotlin.mock import org.mockito.kotlin.never -import org.mockito.kotlin.stub import org.mockito.kotlin.times import org.mockito.kotlin.verifyBlocking import org.mockito.kotlin.whenever @@ -40,6 +37,7 @@ import org.mockito.kotlin.wheneverBlocking import org.robolectric.RobolectricTestRunner import org.robolectric.Shadows.shadowOf import org.robolectric.annotation.Config +import java.util.concurrent.TimeUnit /** * Tests for [ConversationsSyncWorker]: which accounts a run syncs, when a run stands down without @@ -56,14 +54,14 @@ class ConversationsSyncWorkerTest { fun `every account is synced, not just the current one`() { val worker = worker() wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2), user(3))) - wheneverBlocking { repository.syncRooms(any(), any()) }.thenReturn(true) + wheneverBlocking { repository.syncRooms(any(), any(), anyOrNull()) }.thenReturn(NO_ROOMS) val result = runBlocking { worker.sync() } assertEquals(ListenableWorker.Result.success(), result) - verifyBlocking(repository) { syncRooms(user(1), false) } - verifyBlocking(repository) { syncRooms(user(2), false) } - verifyBlocking(repository) { syncRooms(user(3), false) } + verifyBlocking(repository) { syncRooms(user(1), false, ROOM_LIST_TIMEOUT_MILLIS) } + verifyBlocking(repository) { syncRooms(user(2), false, ROOM_LIST_TIMEOUT_MILLIS) } + verifyBlocking(repository) { syncRooms(user(3), false, ROOM_LIST_TIMEOUT_MILLIS) } } @Test @@ -76,7 +74,7 @@ class ConversationsSyncWorkerTest { assertEquals(ListenableWorker.Result.success(), result) verifyBlocking(userManager, never()) { getUsers() } - verifyBlocking(repository, never()) { syncRooms(any(), any()) } + verifyBlocking(repository, never()) { syncRooms(any(), any(), anyOrNull()) } } @Test @@ -87,27 +85,60 @@ class ConversationsSyncWorkerTest { val result = runBlocking { worker.sync() } assertEquals(ListenableWorker.Result.success(), result) - verifyBlocking(repository, never()) { syncRooms(any(), any()) } + verifyBlocking(repository, never()) { syncRooms(any(), any(), anyOrNull()) } } @Test fun `a failed account asks for another attempt`() { val worker = worker(runAttempt = 0) wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2))) - wheneverBlocking { repository.syncRooms(user(1), false) }.thenReturn(true) - wheneverBlocking { repository.syncRooms(user(2), false) }.thenReturn(false) + wheneverBlocking { repository.syncRooms(user(1), false, ROOM_LIST_TIMEOUT_MILLIS) }.thenReturn(NO_ROOMS) + wheneverBlocking { repository.syncRooms(user(2), false, ROOM_LIST_TIMEOUT_MILLIS) }.thenReturn(null) val result = runBlocking { worker.sync() } assertEquals(ListenableWorker.Result.retry(), result) - verifyBlocking(repository) { syncRooms(user(1), false) } + verifyBlocking(repository) { syncRooms(user(1), false, ROOM_LIST_TIMEOUT_MILLIS) } + verifyBlocking(repository) { catchUpRooms(user(1), NO_ROOMS) } + verifyBlocking(repository, never()) { catchUpRooms(eq(user(2)), any()) } + } + + @Test + fun `every room list is synced before any messages are caught up`() { + val worker = worker() + wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2))) + wheneverBlocking { repository.syncRooms(any(), any(), anyOrNull()) }.thenReturn(NO_ROOMS) + + runBlocking { worker.sync() } + + val order = inOrder(repository) + runBlocking { + order.verify(repository).syncRooms(eq(user(1)), any(), anyOrNull()) + order.verify(repository).syncRooms(eq(user(2)), any(), anyOrNull()) + order.verify(repository).catchUpRooms(user(1), NO_ROOMS) + order.verify(repository).catchUpRooms(user(2), NO_ROOMS) + } + } + + @Test + fun `a failed message catch-up does not fail the run`() { + val worker = worker() + wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2))) + wheneverBlocking { repository.syncRooms(any(), any(), anyOrNull()) }.thenReturn(NO_ROOMS) + wheneverBlocking { repository.catchUpRooms(eq(user(1)), any()) } + .thenThrow(IllegalStateException("database is gone")) + + val result = runBlocking { worker.sync() } + + assertEquals(ListenableWorker.Result.success(), result) + verifyBlocking(repository) { catchUpRooms(user(2), NO_ROOMS) } } @Test fun `a sync that keeps failing gives up instead of retrying for ever`() { val worker = worker(runAttempt = 2) wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1))) - wheneverBlocking { repository.syncRooms(any(), any()) }.thenReturn(false) + wheneverBlocking { repository.syncRooms(any(), any(), anyOrNull()) }.thenReturn(null) val result = runBlocking { worker.sync() } @@ -121,32 +152,15 @@ class ConversationsSyncWorkerTest { whenever(repository.lastFullSyncAt(1)).thenReturn(300) whenever(repository.lastFullSyncAt(2)).thenReturn(null) whenever(repository.lastFullSyncAt(3)).thenReturn(100) - wheneverBlocking { repository.syncRooms(any(), any()) }.thenReturn(true) + wheneverBlocking { repository.syncRooms(any(), any(), anyOrNull()) }.thenReturn(NO_ROOMS) runBlocking { worker.sync() } val synced = argumentCaptor() - verifyBlocking(repository, times(3)) { syncRooms(synced.capture(), any()) } + verifyBlocking(repository, times(3)) { syncRooms(synced.capture(), any(), anyOrNull()) } assertEquals(listOf(2L, 3L, 1L), synced.allValues.map { it.id }) } - @OptIn(ExperimentalCoroutinesApi::class) - @Test - fun `an account that never finishes does not keep the others from syncing`() = - runTest { - val worker = worker() - wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2))) - repository.stub { - onBlocking { syncRooms(argThat { id == 1L }, any()) } doSuspendableAnswer { awaitCancellation() } - onBlocking { syncRooms(argThat { id == 2L }, any()) } doReturn true - } - - val result = worker.sync() - - verifyBlocking(repository) { syncRooms(argThat { id == 2L }, any()) } - assertEquals(ListenableWorker.Result.retry(), result) - } - @Test fun `a failed account lookup is retried rather than reported as a sync`() { val worker = worker() @@ -155,7 +169,7 @@ class ConversationsSyncWorkerTest { val result = runBlocking { worker.sync() } assertEquals(ListenableWorker.Result.retry(), result) - verifyBlocking(repository, never()) { syncRooms(any(), any()) } + verifyBlocking(repository, never()) { syncRooms(any(), any(), anyOrNull()) } } @Test @@ -172,11 +186,12 @@ class ConversationsSyncWorkerTest { fun `a cancelled run stops instead of syncing the remaining accounts`() { val worker = worker() wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2), user(3))) - wheneverBlocking { repository.syncRooms(any(), any()) }.thenThrow(CancellationException("run stopped")) + wheneverBlocking { repository.syncRooms(any(), any(), anyOrNull()) } + .thenThrow(CancellationException("run stopped")) assertThrows(CancellationException::class.java) { runBlocking { worker.sync() } } - verifyBlocking(repository, times(1)) { syncRooms(any(), any()) } + verifyBlocking(repository, times(1)) { syncRooms(any(), any(), anyOrNull()) } } private fun worker(runAttempt: Int = 0): ConversationsSyncWorker = @@ -194,5 +209,7 @@ class ConversationsSyncWorkerTest { companion object { private const val BASE_URL = "https://server.example.com" + private val ROOM_LIST_TIMEOUT_MILLIS = TimeUnit.MINUTES.toMillis(2) + private val NO_ROOMS = emptyList() } } From c35f9658ca0f81a95ba8073703374cb9b5aae348 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Tue, 6 Oct 2026 11:42:52 +0200 Subject: [PATCH 05/11] fix(conversations): leave reachability of the background sync to WorkManager The room list sync returned early whenever the network monitor did not report the device as online. The conversation list screen checks that before it syncs anyway, so the early return only ever affected the background worker - the one caller that is meant to rely on its work request's network constraint instead. The two disagree on a network that is connected but not yet validated, or sits behind a captive portal: WorkManager starts the worker, while the monitor, which waits for validation, reports offline. Every account then counted as failed and the run burned its retries without sending a single request. Send the request and let it fail like any other when the network really is unusable, as ChatMessageSyncer already does for the same reason. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Marcel Hibbe --- .../data/OfflineConversationsRepository.kt | 3 +++ .../network/OfflineFirstConversationsRepository.kt | 5 ----- .../OfflineFirstConversationsRepositoryTest.kt | 12 ++++++++++++ 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt index 36aeb398d15..a7bb6cfb651 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt @@ -65,6 +65,9 @@ interface OfflineConversationsRepository { * * With [roomListTimeoutMillis] set, the sync is given at most that long and reports failure * when it runs out. + * + * Connectivity is not pre-checked: the request is sent and a failure is reported like any + * other, so a network that is connected but not yet validated still gets its chance. */ suspend fun syncRooms( user: User, diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt index 054ccc2a557..18f28a67e9e 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt @@ -170,11 +170,6 @@ class OfflineFirstConversationsRepository @Inject constructor( forceFullSync: Boolean = false, roomListTimeoutMillis: Long? = null ): List? { - if (!networkMonitor.isOnline.value) { - Log.d(TAG, "Device is offline, can't load conversations from server") - return null - } - val accountId = user.id!! val modifiedSince = modifiedSinceFor(user, forceFullSync) diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt index 4230c7e8587..2b19b998853 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt @@ -134,6 +134,18 @@ class OfflineFirstConversationsRepositoryTest { verifyBlocking(network, never()) { getRooms(any(), any(), any(), anyOrNull()) } } + @Test + fun `syncRooms tries the server even when the device is not known to be online`() = + runBlocking { + whenever(networkMonitor.isOnline).thenReturn(MutableStateFlow(false)) + val room = conversation(token = ROOM_TOKEN, lastActivity = 5, unreadMessages = 0) + whenever(network.getRooms(any(), any(), any(), anyOrNull())).thenReturn(roomList(listOf(room))) + + val roomsWithNewMessages = repository.syncRooms(user()) + + assertEquals(listOf(ROOM_TOKEN), roomsWithNewMessages?.map { it.token }) + } + @Test fun `getRooms fetches conversations from the server and syncs them locally when online`() = runBlocking { From accb072eb29af6888000af2c59612fc573feb33d Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Tue, 6 Oct 2026 13:28:29 +0200 Subject: [PATCH 06/11] fix(conversations): stop syncing room lists once the app comes to the foreground The worker checked whether the app was in the foreground only when a run started. A run can take up to the ten minutes WorkManager grants it, and opening the app in that time had the conversation list sync the account it shows while the worker went on syncing the same account. The two write the same rows, and a slower, older response landing last put stale unread counts back until the next sync. Check again before each account's room list, and leave the remaining room lists to the next run once the app is in the foreground - the same stand-down a run that starts there gets. The message catch-up is not cut short with them. The run stored those rooms' latest activity with their room list already, so the conversation list's own sync no longer sees new messages in them and would not catch them up in the run's place. It can safely run alongside the screen: its update to a conversation is skipped when the stored state is newer, and overlapping catch-ups of a room are coalesced. A run that stops early leaves accounts untried, so its warning counts failures against the accounts it tried rather than all of them. The foreground guard at the start of a run was untested, while the battery saver one next to it was covered. Both it and the app coming to the foreground mid-run are now covered, driving the real process lifecycle. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Marcel Hibbe --- .../talk/jobs/ConversationsSyncWorker.kt | 50 +++++++++++++++---- .../talk/jobs/ConversationsSyncWorkerTest.kt | 43 ++++++++++++++++ 2 files changed, 84 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt index 47123b33a81..b33ee156864 100644 --- a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt +++ b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt @@ -38,10 +38,10 @@ import javax.inject.Inject * * Accounts are synced one after another, in two passes: first the room list of every account, * then the messages of each. A slow message prefetch therefore cannot keep another account's - * room list from being synced. The run is skipped in battery saver mode and while the app - * is in the foreground, and a run in which any account failed is retried up to [MAX_RUN_ATTEMPTS] - * times. Network availability is enforced by the [NetworkType.CONNECTED] constraint on the request - * rather than checked here. + * room list from being synced. The run is skipped in battery saver mode and while the app is in + * the foreground, stops syncing room lists once the app comes to the foreground, and a run in which + * any account failed is retried up to [MAX_RUN_ATTEMPTS] times. Network availability is enforced by the + * [NetworkType.CONNECTED] constraint on the request rather than checked here. */ @AutoInjector(NextcloudTalkApplication::class) class ConversationsSyncWorker(context: Context, workerParams: WorkerParameters) : @@ -93,21 +93,53 @@ class ConversationsSyncWorker(context: Context, workerParams: WorkerParameters) // room lists first, for every account: they are what the conversation list shows. Messages // are prefetched only after that, with the time the run has left - val syncedAccounts = longestWaitingFirst.map { it to syncAccount(it) } - syncedAccounts.forEach { (user, roomsWithNewMessages) -> - roomsWithNewMessages?.let { catchUpAccount(user, it) } - } + val syncedAccounts = syncRoomLists(longestWaitingFirst) + catchUpMessages(syncedAccounts) val failed = syncedAccounts.count { (_, roomsWithNewMessages) -> roomsWithNewMessages == null } return if (failed == 0) { Result.success() } else { - Log.w(TAG, "$failed of ${accounts.size} accounts did not sync (attempt ${runAttemptCount + 1})") + Log.w(TAG, "$failed of ${syncedAccounts.size} tried accounts did not sync (attempt ${runAttemptCount + 1})") retryOrFail() } } + /** + * Syncs the room list of each of [accounts] in turn, pairing each account with the rooms to + * catch up, or null when its sync failed. Accounts the run did not get to are left out. + */ + private suspend fun syncRoomLists(accounts: List): List?>> { + val syncedAccounts = mutableListOf?>>() + for (user in accounts) { + if (cameToForeground()) break + syncedAccounts += user to syncAccount(user) + } + return syncedAccounts + } + + /** + * Catches up the messages of every account whose room list was synced, even once the app is in + * the foreground: the list stored those rooms' latest activity already, so its own sync no + * longer counts them as having new messages and would not catch them up in the run's place. + */ + private suspend fun catchUpMessages(syncedAccounts: List?>>) { + for ((user, roomsWithNewMessages) in syncedAccounts) { + roomsWithNewMessages?.let { catchUpAccount(user, it) } + } + } + + /** + * Whether the app came to the foreground since the run started. The conversation list then + * syncs on its own, and a run going on alongside it would race it for the same rows, so the + * remaining room lists are left to the next run. + */ + private suspend fun cameToForeground(): Boolean = + isAppInForeground().also { inForeground -> + if (inForeground) Log.d(TAG, "App came to the foreground, leaving the remaining room lists to the next run") + } + /** * Syncs a single account's room list, returning the rooms whose messages should be caught up, * or null when it failed. Failures are logged; the run being cancelled propagates. diff --git a/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt b/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt index bf5ac2678a4..66187643d05 100644 --- a/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt +++ b/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt @@ -10,6 +10,9 @@ package com.nextcloud.talk.jobs import android.app.Application import android.content.Context import android.os.PowerManager +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.LifecycleRegistry +import androidx.lifecycle.ProcessLifecycleOwner import androidx.test.core.app.ApplicationProvider import androidx.work.ListenableWorker import androidx.work.testing.TestListenableWorkerBuilder @@ -19,6 +22,7 @@ import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.users.UserManager import kotlinx.coroutines.CancellationException import kotlinx.coroutines.runBlocking +import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertThrows import org.junit.Test @@ -43,6 +47,7 @@ import java.util.concurrent.TimeUnit * Tests for [ConversationsSyncWorker]: which accounts a run syncs, when a run stands down without * syncing, and how a failed run reports itself. */ +@Suppress("TooManyFunctions") @RunWith(RobolectricTestRunner::class) @Config(application = Application::class, sdk = [33]) class ConversationsSyncWorkerTest { @@ -77,6 +82,18 @@ class ConversationsSyncWorkerTest { verifyBlocking(repository, never()) { syncRooms(any(), any(), anyOrNull()) } } + @Test + fun `an app in the foreground stands the sync down entirely`() { + val worker = worker() + moveAppTo(Lifecycle.State.STARTED) + + val result = runBlocking { worker.sync() } + + assertEquals(ListenableWorker.Result.success(), result) + verifyBlocking(userManager, never()) { getUsers() } + verifyBlocking(repository, never()) { syncRooms(any(), any(), anyOrNull()) } + } + @Test fun `an account with nothing to sync is not an error`() { val worker = worker() @@ -161,6 +178,22 @@ class ConversationsSyncWorkerTest { assertEquals(listOf(2L, 3L, 1L), synced.allValues.map { it.id }) } + @Test + fun `the app coming to the foreground stops the room lists but not the messages already due`() { + val worker = worker() + wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2))) + wheneverBlocking { repository.syncRooms(eq(user(1)), any(), anyOrNull()) }.thenAnswer { + moveAppTo(Lifecycle.State.STARTED) + NO_ROOMS + } + + val result = runBlocking { worker.sync() } + + assertEquals(ListenableWorker.Result.success(), result) + verifyBlocking(repository, never()) { syncRooms(eq(user(2)), any(), anyOrNull()) } + verifyBlocking(repository) { catchUpRooms(user(1), NO_ROOMS) } + } + @Test fun `a failed account lookup is retried rather than reported as a sync`() { val worker = worker() @@ -194,6 +227,16 @@ class ConversationsSyncWorkerTest { verifyBlocking(repository, times(1)) { syncRooms(any(), any(), anyOrNull()) } } + @After + fun tearDown() { + moveAppTo(Lifecycle.State.CREATED) + } + + /** Moves the process lifecycle, which outlives a single test, to [state]. */ + private fun moveAppTo(state: Lifecycle.State) { + (ProcessLifecycleOwner.get().lifecycle as LifecycleRegistry).currentState = state + } + private fun worker(runAttempt: Int = 0): ConversationsSyncWorker = TestListenableWorkerBuilder(applicationContext()) .setRunAttemptCount(runAttempt) From 3c3a7bd15d2ae90be6961d66cfc44606032ccdcc Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Tue, 6 Oct 2026 12:19:56 +0200 Subject: [PATCH 07/11] fix(conversations): keep background sync failures off the conversation list A failed room list sync of an account without cached conversations is reported on syncErrorFlow, so the conversation list can tell the user why it stays empty. The flow does not say which account failed, and the list's view model collects it for as long as it lives - in the background too - and turns whatever arrives into an error dialog. The background worker runs that same sync for every account. A 401 for an account whose password was changed, or a server that is down, then put an unauthorized or error dialog on the list of whichever account the user had open when they came back. Only the sync the conversation list starts for the account it shows reports there now, as the flow's documentation already promises. The worker learns of a failure through syncRooms' return value and logs it. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Marcel Hibbe --- .../data/OfflineConversationsRepository.kt | 3 ++ .../OfflineFirstConversationsRepository.kt | 28 ++++++++++++---- ...OfflineFirstConversationsRepositoryTest.kt | 32 +++++++++++++++++++ 3 files changed, 57 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt index a7bb6cfb651..ad8a2f38e6b 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt @@ -68,6 +68,9 @@ interface OfflineConversationsRepository { * * Connectivity is not pre-checked: the request is sent and a failure is reported like any * other, so a network that is connected but not yet validated still gets its chance. + * + * A failure is reported only through the return value, never on [syncErrorFlow]: that flow + * feeds the conversation list, which must not show an error of an account it does not show. */ suspend fun syncRooms( user: User, diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt index 18f28a67e9e..e4ceb1e0536 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt @@ -104,8 +104,11 @@ class OfflineFirstConversationsRepository @Inject constructor( override fun getRooms(user: User, forceFullSync: Boolean): Job = scope.launch { if (networkMonitor.isOnline.value) { - val roomsWithNewMessages = getRoomsFromServer(user, forceFullSync = forceFullSync) - ?: return@launch + val roomsWithNewMessages = getRoomsFromServer( + user, + forceFullSync = forceFullSync, + reportSyncError = true + ) ?: return@launch // a launch of its own, so the returned job completes without waiting for the catch-up scope.launch { catchUpRooms(user, roomsWithNewMessages) } } @@ -116,7 +119,12 @@ class OfflineFirstConversationsRepository @Inject constructor( forceFullSync: Boolean, roomListTimeoutMillis: Long? ): List? = - getRoomsFromServer(user, forceFullSync = forceFullSync, roomListTimeoutMillis = roomListTimeoutMillis) + getRoomsFromServer( + user, + forceFullSync = forceFullSync, + roomListTimeoutMillis = roomListTimeoutMillis, + reportSyncError = false + ) @Suppress("Detekt.TooGenericExceptionCaught") override fun getRoom(user: User, roomToken: String): Job = @@ -164,11 +172,20 @@ class OfflineFirstConversationsRepository @Inject constructor( return getConversation(id, roomToken) } + /** + * Syncs [user]'s room list, returning the rooms whose messages should be caught up, or null + * when the sync failed. + * + * [reportSyncError] lets a failure reach [syncErrorFlow]. That flow does not say which account + * failed and the conversation list shows whatever arrives there, so only a sync of the account + * the list shows may report to it. + */ @Suppress("Detekt.TooGenericExceptionCaught") private suspend fun getRoomsFromServer( user: User, forceFullSync: Boolean = false, - roomListTimeoutMillis: Long? = null + roomListTimeoutMillis: Long? = null, + reportSyncError: Boolean ): List? { val accountId = user.id!! val modifiedSince = modifiedSinceFor(user, forceFullSync) @@ -188,8 +205,7 @@ class OfflineFirstConversationsRepository @Inject constructor( } catch (e: Exception) { Log.e(TAG, "Something went wrong when fetching conversations", e) storeTimestamp(accountId, KEY_MODIFIED_SINCE, null) - val hasCachedConversations = dao.getConversationsForUser(accountId).first().isNotEmpty() - if (!hasCachedConversations) { + if (reportSyncError && dao.getConversationsForUser(accountId).first().isEmpty()) { _syncErrorFlow.emit(e) } null diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt index 2b19b998853..f92b4966227 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt @@ -171,6 +171,38 @@ class OfflineFirstConversationsRepositoryTest { assertNull(repository.syncRooms(user())) } + @Test + fun `getRooms reports a failed sync of an account without cached conversations`() = + runBlocking { + val room = conversation(token = ROOM_TOKEN, lastActivity = 5, unreadMessages = 0) + whenever(network.getRooms(any(), any(), any(), anyOrNull())).thenReturn(roomList(listOf(room))) + wheneverBlocking { dao.syncConversationsForUser(any(), any(), any()) } + .thenThrow(IllegalStateException("database is gone")) + val errors = mutableListOf() + val collector = launch(Dispatchers.Unconfined) { repository.syncErrorFlow.collect { errors += it } } + + repository.getRooms(user()).join() + + awaitUntil { errors.isNotEmpty() } + collector.cancel() + } + + @Test + fun `syncRooms keeps a failed sync off the conversation list's error flow`() = + runBlocking { + val room = conversation(token = ROOM_TOKEN, lastActivity = 5, unreadMessages = 0) + whenever(network.getRooms(any(), any(), any(), anyOrNull())).thenReturn(roomList(listOf(room))) + wheneverBlocking { dao.syncConversationsForUser(any(), any(), any()) } + .thenThrow(IllegalStateException("database is gone")) + val errors = mutableListOf() + val collector = launch(Dispatchers.Unconfined) { repository.syncErrorFlow.collect { errors += it } } + + repository.syncRooms(user()) + + collector.cancel() + assertEquals(emptyList(), errors) + } + @Test fun `syncRooms reports a room list sync that runs out of its time budget as failed`() = runTest { From 9b68932efc478e4a26ed99d1c7fa07d0d0aeb538 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Tue, 6 Oct 2026 13:29:08 +0200 Subject: [PATCH 08/11] fix(conversations): keep the background sync's blocking work off the caller's thread syncRooms blocks the thread it runs on: the request is awaited with blockingSingle, retried up to three times with each attempt bounded only by the 45 second HTTP timeouts, and the stored sync state is read with blockingGet. getRooms never showed it, since it runs in the repository's own IO scope, but syncRooms runs on its caller's thread - for the background worker one of the few threads of Dispatchers.Default, sized to the CPU cores and shared with the rest of the app's coroutines, which a slow server then held for minutes. Called from the main thread it would not even get that far: the request delivers its result on the main thread that blockingSingle is blocking. The worker's ordering of the accounts did the same on a smaller scale. It sorted with sortedBy on the stored time of each account's last full sync, and sortedBy calls its selector for every comparison rather than once per account - each call a blocking database query. Run syncRooms and lastFullSyncAt on the IO dispatcher, as ConversationListUpdater does around its blocking calls, so both are safe to call from any thread, and read each account's last full sync once before sorting on the values. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Marcel Hibbe --- .../data/OfflineConversationsRepository.kt | 6 ++++-- .../OfflineFirstConversationsRepository.kt | 19 ++++++++++++------- .../talk/jobs/ConversationsSyncWorker.kt | 5 ++++- ...OfflineFirstConversationsRepositoryTest.kt | 15 +++++++++++++++ .../talk/jobs/ConversationsSyncWorkerTest.kt | 10 ++++++---- 5 files changed, 41 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt index ad8a2f38e6b..d5aa33dfe38 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt @@ -71,6 +71,8 @@ interface OfflineConversationsRepository { * * A failure is reported only through the return value, never on [syncErrorFlow]: that flow * feeds the conversation list, which must not show an error of an account it does not show. + * + * Safe to call from any thread: the blocking work runs on the IO dispatcher. */ suspend fun syncRooms( user: User, @@ -112,9 +114,9 @@ interface OfflineConversationsRepository { /** * When the conversation list of the account with the internal id [accountId] was last fetched - * in full, or null when it never was. + * in full, or null when it never was. Safe to call from any thread. */ - fun lastFullSyncAt(accountId: Long): Long? + suspend fun lastFullSyncAt(accountId: Long): Long? fun observeConversation(accountId: Long, roomToken: String): Flow } diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt index e4ceb1e0536..28261526d7d 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt @@ -45,6 +45,7 @@ import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Semaphore import kotlinx.coroutines.sync.withPermit +import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeout import javax.inject.Inject import kotlin.collections.map @@ -119,12 +120,15 @@ class OfflineFirstConversationsRepository @Inject constructor( forceFullSync: Boolean, roomListTimeoutMillis: Long? ): List? = - getRoomsFromServer( - user, - forceFullSync = forceFullSync, - roomListTimeoutMillis = roomListTimeoutMillis, - reportSyncError = false - ) + // the sync blocks its thread on the request and on stored state, so it must not run on the caller's + withContext(Dispatchers.IO) { + getRoomsFromServer( + user, + forceFullSync = forceFullSync, + roomListTimeoutMillis = roomListTimeoutMillis, + reportSyncError = false + ) + } @Suppress("Detekt.TooGenericExceptionCaught") override fun getRoom(user: User, roomToken: String): Job = @@ -266,7 +270,8 @@ class OfflineFirstConversationsRepository @Inject constructor( storeTimestamp(accountId, KEY_LAST_FULL_SYNC_AT, null) } - override fun lastFullSyncAt(accountId: Long): Long? = readTimestamp(accountId, KEY_LAST_FULL_SYNC_AT) + override suspend fun lastFullSyncAt(accountId: Long): Long? = + withContext(Dispatchers.IO) { readTimestamp(accountId, KEY_LAST_FULL_SYNC_AT) } /** * The value to send as `modifiedSince`, or null when this sync has to be a full one. diff --git a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt index b33ee156864..0ab1150adc0 100644 --- a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt +++ b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt @@ -89,7 +89,10 @@ class ConversationsSyncWorker(context: Context, workerParams: WorkerParameters) // WorkManager stops a run after ten minutes, so a fixed order would let slow accounts at // the front keep the ones behind them from ever being reached - val longestWaitingFirst = accounts.sortedBy { conversationsRepository.lastFullSyncAt(it.id!!) ?: 0L } + val longestWaitingFirst = accounts + .map { it to (conversationsRepository.lastFullSyncAt(it.id!!) ?: 0L) } + .sortedBy { (_, lastFullSyncAt) -> lastFullSyncAt } + .map { (user, _) -> user } // room lists first, for every account: they are what the conversation list shows. Messages // are prefetched only after that, with the time the run has left diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt index f92b4966227..5c1a9717bd9 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt @@ -171,6 +171,21 @@ class OfflineFirstConversationsRepositoryTest { assertNull(repository.syncRooms(user())) } + @Test + fun `syncRooms does its blocking work off the caller's thread`() = + runBlocking { + val room = conversation(token = ROOM_TOKEN, lastActivity = 5, unreadMessages = 0) + var requestThread: Thread? = null + whenever(network.getRooms(any(), any(), any(), anyOrNull())).thenAnswer { + requestThread = Thread.currentThread() + roomList(listOf(room)) + } + + repository.syncRooms(user()) + + assertTrue(requestThread != null && requestThread != Thread.currentThread()) + } + @Test fun `getRooms reports a failed sync of an account without cached conversations`() = runBlocking { diff --git a/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt b/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt index 66187643d05..5f8db8e9e70 100644 --- a/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt +++ b/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt @@ -36,7 +36,6 @@ import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.times import org.mockito.kotlin.verifyBlocking -import org.mockito.kotlin.whenever import org.mockito.kotlin.wheneverBlocking import org.robolectric.RobolectricTestRunner import org.robolectric.Shadows.shadowOf @@ -166,9 +165,9 @@ class ConversationsSyncWorkerTest { fun `the account that waited longest is synced first`() { val worker = worker() wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2), user(3))) - whenever(repository.lastFullSyncAt(1)).thenReturn(300) - whenever(repository.lastFullSyncAt(2)).thenReturn(null) - whenever(repository.lastFullSyncAt(3)).thenReturn(100) + wheneverBlocking { repository.lastFullSyncAt(1) }.thenReturn(300) + wheneverBlocking { repository.lastFullSyncAt(2) }.thenReturn(null) + wheneverBlocking { repository.lastFullSyncAt(3) }.thenReturn(100) wheneverBlocking { repository.syncRooms(any(), any(), anyOrNull()) }.thenReturn(NO_ROOMS) runBlocking { worker.sync() } @@ -176,6 +175,9 @@ class ConversationsSyncWorkerTest { val synced = argumentCaptor() verifyBlocking(repository, times(3)) { syncRooms(synced.capture(), any(), anyOrNull()) } assertEquals(listOf(2L, 3L, 1L), synced.allValues.map { it.id }) + verifyBlocking(repository, times(1)) { lastFullSyncAt(1) } + verifyBlocking(repository, times(1)) { lastFullSyncAt(2) } + verifyBlocking(repository, times(1)) { lastFullSyncAt(3) } } @Test From 810d7eb585fdfb296f5687920fee89a52c5c5cf3 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Tue, 6 Oct 2026 13:14:00 +0200 Subject: [PATCH 09/11] fix(conversations): update the scheduled conversation sync instead of keeping it The periodic conversation sync was scheduled with KEEP, so that an app started often does not restart the period every time and so never reaches a run. KEEP also ignores the request it is handed, though: once a version of the app has scheduled the worker, a later version that changes its interval or constraints never gets them applied - existing installs keep running the old request until the app data is cleared. UPDATE keeps what KEEP was chosen for - the scheduled run and its timing stay in place - but applies the new request to it, so changes to the schedule reach existing installs. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Marcel Hibbe --- .../talk/jobs/ConversationsSyncWorker.kt | 7 +++++-- .../talk/jobs/ConversationsSyncWorkerTest.kt | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt index 0ab1150adc0..0570ea202e3 100644 --- a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt +++ b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt @@ -191,7 +191,10 @@ class ConversationsSyncWorker(context: Context, workerParams: WorkerParameters) /** * Schedules the worker to run every [REPEAT_INTERVAL_MINUTES] minutes while a network is - * available, leaving an already scheduled run in place. + * available. An already scheduled run is updated to this request rather than replaced, so + * a changed interval or constraint reaches existing installs while the period keeps its + * timing - replacing it would restart the period on every app start, and an app opened + * often would never reach a run. */ fun schedule(context: Context) { val work = PeriodicWorkRequest.Builder( @@ -204,7 +207,7 @@ class ConversationsSyncWorker(context: Context, workerParams: WorkerParameters) WorkManager.getInstance(context).enqueueUniquePeriodicWork( UNIQUE_WORK_NAME, - ExistingPeriodicWorkPolicy.KEEP, + ExistingPeriodicWorkPolicy.UPDATE, work ) } diff --git a/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt b/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt index 5f8db8e9e70..71e6d300673 100644 --- a/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt +++ b/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt @@ -15,7 +15,10 @@ import androidx.lifecycle.LifecycleRegistry import androidx.lifecycle.ProcessLifecycleOwner import androidx.test.core.app.ApplicationProvider import androidx.work.ListenableWorker +import androidx.work.WorkInfo +import androidx.work.WorkManager import androidx.work.testing.TestListenableWorkerBuilder +import androidx.work.testing.WorkManagerTestInitHelper import com.nextcloud.talk.conversationlist.data.OfflineConversationsRepository import com.nextcloud.talk.data.database.model.ConversationEntity import com.nextcloud.talk.data.user.model.User @@ -229,6 +232,21 @@ class ConversationsSyncWorkerTest { verifyBlocking(repository, times(1)) { syncRooms(any(), any(), anyOrNull()) } } + @Test + fun `scheduling again updates the scheduled run instead of keeping or replacing it`() { + WorkManagerTestInitHelper.initializeTestWorkManager(applicationContext()) + val workManager = WorkManager.getInstance(applicationContext()) + + ConversationsSyncWorker.schedule(applicationContext()) + val first = workManager.getWorkInfosForUniqueWork(ConversationsSyncWorker.UNIQUE_WORK_NAME).get().single() + ConversationsSyncWorker.schedule(applicationContext()) + val second = workManager.getWorkInfosForUniqueWork(ConversationsSyncWorker.UNIQUE_WORK_NAME).get().single() + + assertEquals(first.id, second.id) + assertEquals(first.generation + 1, second.generation) + assertEquals(WorkInfo.State.ENQUEUED, second.state) + } + @After fun tearDown() { moveAppTo(Lifecycle.State.CREATED) From 2f899db55c4b94dfcff7c2be04fb69c9fca3b2ef Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Tue, 6 Oct 2026 14:25:08 +0200 Subject: [PATCH 10/11] fix(conversations): sync every account even when its sync time cannot be read The worker orders the accounts by the time of their last full sync, read from the database with a blocking query. Nothing caught a failure of that read, so it escaped doWork and failed the run outright: no account was synced and no retry was scheduled, all for an order that only exists to keep slow accounts from starving the others. Fall back to the accounts' own order when the read fails, and let cancellation through as before. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Marcel Hibbe --- .../talk/jobs/ConversationsSyncWorker.kt | 15 +++++++++++---- .../talk/jobs/ConversationsSyncWorkerTest.kt | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt index 0570ea202e3..316e740660a 100644 --- a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt +++ b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt @@ -89,10 +89,17 @@ class ConversationsSyncWorker(context: Context, workerParams: WorkerParameters) // WorkManager stops a run after ten minutes, so a fixed order would let slow accounts at // the front keep the ones behind them from ever being reached - val longestWaitingFirst = accounts - .map { it to (conversationsRepository.lastFullSyncAt(it.id!!) ?: 0L) } - .sortedBy { (_, lastFullSyncAt) -> lastFullSyncAt } - .map { (user, _) -> user } + // the order is only an optimisation, so a failed read must not keep any account from syncing + val longestWaitingFirst = runCatching { + accounts + .map { it to (conversationsRepository.lastFullSyncAt(it.id!!) ?: 0L) } + .sortedBy { (_, lastFullSyncAt) -> lastFullSyncAt } + .map { (user, _) -> user } + }.getOrElse { throwable -> + if (throwable is CancellationException) throw throwable + Log.w(TAG, "Could not read the sync timestamps to order the accounts", throwable) + accounts + } // room lists first, for every account: they are what the conversation list shows. Messages // are prefetched only after that, with the time the run has left diff --git a/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt b/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt index 71e6d300673..62b30490feb 100644 --- a/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt +++ b/app/src/test/java/com/nextcloud/talk/jobs/ConversationsSyncWorkerTest.kt @@ -183,6 +183,21 @@ class ConversationsSyncWorkerTest { verifyBlocking(repository, times(1)) { lastFullSyncAt(3) } } + @Test + fun `a failed read of the sync times keeps the account order and still syncs every account`() { + val worker = worker() + wheneverBlocking { userManager.getUsers() }.thenReturn(listOf(user(1), user(2), user(3))) + wheneverBlocking { repository.lastFullSyncAt(any()) }.thenThrow(IllegalStateException("database is gone")) + wheneverBlocking { repository.syncRooms(any(), any(), anyOrNull()) }.thenReturn(NO_ROOMS) + + val result = runBlocking { worker.sync() } + + assertEquals(ListenableWorker.Result.success(), result) + val synced = argumentCaptor() + verifyBlocking(repository, times(3)) { syncRooms(synced.capture(), any(), anyOrNull()) } + assertEquals(listOf(1L, 2L, 3L), synced.allValues.map { it.id }) + } + @Test fun `the app coming to the foreground stops the room lists but not the messages already due`() { val worker = worker() From 298813b641262110435e92631e8222fe44034d73 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Tue, 6 Oct 2026 14:31:20 +0200 Subject: [PATCH 11/11] refactor(conversations): split the background sync's account handling syncAccounts read the accounts, ordered them and synced them in one function with three returns, over detekt's ReturnCount limit of two. Move reading the accounts, ordering them and syncing them into functions of their own, leaving syncAccounts a single when over the outcome of the read. The behaviour is unchanged. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Marcel Hibbe --- .../talk/jobs/ConversationsSyncWorker.kt | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt index 316e740660a..f72068d5b6b 100644 --- a/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt +++ b/app/src/main/java/com/nextcloud/talk/jobs/ConversationsSyncWorker.kt @@ -76,21 +76,35 @@ class ConversationsSyncWorker(context: Context, workerParams: WorkerParameters) } private suspend fun syncAccounts(): Result { - val accounts = runCatching { userManager.getUsers() }.getOrElse { throwable -> - if (throwable is CancellationException) throw throwable - Log.e(TAG, "Could not read the accounts to sync", throwable) - return retryOrFail() + val accounts = readAccounts() + return when { + accounts == null -> retryOrFail() + + accounts.isEmpty() -> { + Log.d(TAG, "No account to sync") + Result.success() + } + + else -> syncAccounts(longestWaitingFirst(accounts)) } + } - if (accounts.isEmpty()) { - Log.d(TAG, "No account to sync") - return Result.success() + /** The accounts to sync, or null when they could not be read. */ + private suspend fun readAccounts(): List? = + runCatching { userManager.getUsers() }.getOrElse { throwable -> + if (throwable is CancellationException) throw throwable + Log.e(TAG, "Could not read the accounts to sync", throwable) + null } - // WorkManager stops a run after ten minutes, so a fixed order would let slow accounts at - // the front keep the ones behind them from ever being reached - // the order is only an optimisation, so a failed read must not keep any account from syncing - val longestWaitingFirst = runCatching { + /** + * [accounts] ordered by the time of their last full sync, oldest first. WorkManager stops a run + * after ten minutes, so a fixed order would let slow accounts at the front keep the ones behind + * them from ever being reached. The order is only an optimisation, so when the sync times + * cannot be read the accounts keep their own order. + */ + private suspend fun longestWaitingFirst(accounts: List): List = + runCatching { accounts .map { it to (conversationsRepository.lastFullSyncAt(it.id!!) ?: 0L) } .sortedBy { (_, lastFullSyncAt) -> lastFullSyncAt } @@ -101,9 +115,10 @@ class ConversationsSyncWorker(context: Context, workerParams: WorkerParameters) accounts } + private suspend fun syncAccounts(accounts: List): Result { // room lists first, for every account: they are what the conversation list shows. Messages // are prefetched only after that, with the time the run has left - val syncedAccounts = syncRoomLists(longestWaitingFirst) + val syncedAccounts = syncRoomLists(accounts) catchUpMessages(syncedAccounts) val failed = syncedAccounts.count { (_, roomsWithNewMessages) -> roomsWithNewMessages == null }