diff --git a/app/schemas/org.libremail.data.local.LibreMailDatabase/13.json b/app/schemas/org.libremail.data.local.LibreMailDatabase/13.json index fd2565d..a66d542 100644 --- a/app/schemas/org.libremail.data.local.LibreMailDatabase/13.json +++ b/app/schemas/org.libremail.data.local.LibreMailDatabase/13.json @@ -2,7 +2,7 @@ "formatVersion": 1, "database": { "version": 13, - "identityHash": "c064a4da054e0f98b4688ea2d04ecaec", + "identityHash": "e4f7ef1e0d780324d6eec3efced768ae", "entities": [ { "tableName": "accounts", @@ -256,6 +256,17 @@ ], "orders": [], "createSql": "CREATE INDEX IF NOT EXISTS `index_messages_timestampMillis` ON `${TABLE_NAME}` (`timestampMillis`)" + }, + { + "name": "index_messages_accountId_folder_uid", + "unique": false, + "columnNames": [ + "accountId", + "folder", + "uid" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_messages_accountId_folder_uid` ON `${TABLE_NAME}` (`accountId`, `folder`, `uid`)" } ] }, @@ -654,7 +665,7 @@ ], "setupQueries": [ "CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)", - "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, 'c064a4da054e0f98b4688ea2d04ecaec')" + "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, 'e4f7ef1e0d780324d6eec3efced768ae')" ] } } \ No newline at end of file diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt index 61ce90d..909467e 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt @@ -169,8 +169,9 @@ class LibreMailDatabaseTest { assertEquals(setOf("acct:INBOX:1", "acct:INBOX:2"), messageDao.getSyncedIds("acct", "INBOX").toSet()) assertEquals(listOf("acct:Archive:1"), messageDao.getSyncedIds("acct", "Archive")) - // Reconciling the inbox must not touch other folders' rows. - messageDao.deleteSyncedNotIn("acct", "INBOX", listOf("acct:INBOX:1")) + // Reconciling the inbox must not touch other folders' rows (windowed reconcile; whole-inbox + // window since these rows have uid 0). + messageDao.deleteSyncedInWindowNotIn("acct", "INBOX", minWindowUid = 0, keepIds = listOf("acct:INBOX:1")) assertEquals( setOf("acct:INBOX:1", "acct:Archive:1"), messageDao.observeSummaries().first().map { it.id }.toSet(), diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoRetentionTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoRetentionTest.kt index 9e4e2e5..bb78a66 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoRetentionTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoRetentionTest.kt @@ -68,28 +68,30 @@ class MessageDaoRetentionTest { ) /** - * The count-based prune boundary keeps the newest [keep] by recency and returns the REST for - * deletion, breaking timestamp ties by the higher UID. This pins the ordering *direction* (a - * flipped `DESC` would keep the OLDEST rows — i.e. locally delete the user's most recent mail) and - * the tie-break, neither of which the mocked-DAO unit tests can catch. + * The count-based prune boundary keeps the newest [keep] by ARRIVAL (server UID) and returns the + * REST for deletion. Keeping by UID — not by the Date header — matches the newest-by-UID recent + * window foreground sync re-fetches, so a high-UID/old-Date message isn't re-downloaded every sync + * and re-pruned every cycle. This pins the ordering column (a Date-ordered keep would evict the + * high-UID/old-Date row) and its direction (a flipped DESC would keep the OLDEST arrivals). */ @Test - fun syncedIdsBeyondCountInFolderKeepsNewestByTimestampThenUid() = runBlocking { + fun syncedIdsBeyondCountInFolderKeepsNewestByArrivalUid() = runBlocking { dao.insertNew( listOf( - message("A", uid = 30, timestampMillis = 300), // newest - message("B", uid = 25, timestampMillis = 200), // ties C on timestamp; higher uid => newer - message("C", uid = 20, timestampMillis = 200), - message("D", uid = 10, timestampMillis = 100), // oldest + message("recent-old-date", uid = 100, timestampMillis = 50), // newest arrival, oldest Date + message("A", uid = 30, timestampMillis = 300), + message("B", uid = 25, timestampMillis = 200), + message("D", uid = 10, timestampMillis = 100), // Scoping decoys: a search-only row and another folder must never enter the ranking. message("SR", uid = 99, timestampMillis = 999, inInbox = false), message("AR", uid = 5, timestampMillis = 50, folder = "Archive"), ), ) - // Keep the newest 2 (A, B); the rest are prunable. B is kept over C purely by the uid tie-break. + // Keep the newest 2 by UID (recent-old-date, A); the rest are prunable. A Date-ordered keep would + // wrongly evict recent-old-date (oldest Date) and keep B. assertEquals( - setOf("C", "D"), + setOf("B", "D"), dao.syncedIdsBeyondCountInFolder("acct", "INBOX", keep = 2).toSet(), ) // Keeping at least as many as exist prunes nothing. diff --git a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt index c73df14..c4dade3 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt @@ -58,6 +58,8 @@ class FakeAccountRepository( override suspend fun deleteAccount(id: String) { accountsFlow.value = accountsFlow.value.filterNot { it.id == id } } + + override suspend fun resetBackfillProgress(accountId: String?) = Unit } /** diff --git a/app/src/main/kotlin/org/libremail/data/AttachmentCache.kt b/app/src/main/kotlin/org/libremail/data/AttachmentCache.kt new file mode 100644 index 0000000..2b88008 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/AttachmentCache.kt @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data + +import java.io.File + +/** + * The per-message on-disk attachment cache directory, keyed by a filesystem-safe form of the message + * id. Shared by the writer ([org.libremail.data.repository.MailRepositoryImpl]) and the retention + * pruner ([org.libremail.data.sync.MailPruner]) so the two can never disagree on where a message's + * attachments live — a divergence would silently leak orphaned files that the pruner no longer finds. + */ +internal fun attachmentCacheDir(cacheDir: File, messageId: String): File { + val safeId = messageId.replace(Regex("[^A-Za-z0-9._-]"), "_") + return File(cacheDir, "attachments/$safeId") +} diff --git a/app/src/main/kotlin/org/libremail/data/local/Migrations.kt b/app/src/main/kotlin/org/libremail/data/local/Migrations.kt index b8f5cc4..5a6a32b 100644 --- a/app/src/main/kotlin/org/libremail/data/local/Migrations.kt +++ b/app/src/main/kotlin/org/libremail/data/local/Migrations.kt @@ -259,5 +259,11 @@ val MIGRATION_12_13 = object : Migration(12, 13) { "`nextBeforeUid` INTEGER NOT NULL, `complete` INTEGER NOT NULL, " + "PRIMARY KEY(`accountId`, `folder`))", ) + + // Index the folder-scoped UID probes the backfill/reconcile hot paths run on every page/sync. + db.execSQL( + "CREATE INDEX IF NOT EXISTS `index_messages_accountId_folder_uid` " + + "ON `messages` (`accountId`, `folder`, `uid`)", + ) } } diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/BackfillProgressDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/BackfillProgressDao.kt index b2c62f6..7e9bc1e 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/BackfillProgressDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/BackfillProgressDao.kt @@ -12,12 +12,13 @@ interface BackfillProgressDao { @Query("SELECT * FROM backfill_progress WHERE accountId = :accountId AND folder = :folder LIMIT 1") suspend fun get(accountId: String, folder: String): BackfillProgressEntity? - @Query("SELECT * FROM backfill_progress WHERE accountId = :accountId") - suspend fun getForAccount(accountId: String): List - @Insert(onConflict = OnConflictStrategy.REPLACE) suspend fun upsert(progress: BackfillProgressEntity) @Query("DELETE FROM backfill_progress WHERE accountId = :accountId") suspend fun deleteForAccount(accountId: String) + + /** Clears all backfill progress (e.g. when the global retention default changes) so it re-evaluates. */ + @Query("DELETE FROM backfill_progress") + suspend fun deleteAll() } diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt index 12fe62a..51e3704 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt @@ -41,6 +41,10 @@ interface MessageDao { @Insert(onConflict = OnConflictStrategy.IGNORE) suspend fun insertNew(messages: List) + /** Of the given [ids], those that already have a row — lets a caller refresh only pre-existing rows. */ + @Query("SELECT id FROM messages WHERE id IN (:ids)") + suspend fun existingIds(ids: List): List + /** * Refreshes the display fields (and the materialized [MessageEntity.uid], keeping it fresh for * rows migrated before the column existed) from the server without touching the cached body, the @@ -87,18 +91,12 @@ interface MessageDao { @Query("DELETE FROM messages WHERE accountId = :accountId AND folder = :folder AND inInbox = 1") suspend fun deleteSyncedByAccountFolder(accountId: String, folder: String) - /** Drops synced rows in [folder] for an account that are no longer present on the server. */ - @Query( - "DELETE FROM messages WHERE accountId = :accountId AND folder = :folder AND inInbox = 1 " + - "AND id NOT IN (:keepIds)", - ) - suspend fun deleteSyncedNotIn(accountId: String, folder: String, keepIds: List) - /** * Windowed deletion reconcile for full-history sync (issue #12): within [folder], delete synced * rows whose UID falls inside the freshly-fetched recent window (`uid >= minWindowUid`) but which * the server no longer returns ([keepIds]). Rows below the window — older history fetched by the - * background backfill — are deliberately left intact, unlike [deleteSyncedNotIn]. + * background backfill — are deliberately left intact, unlike a whole-folder "not in the recent + * set" reconcile, which would wipe that backfilled history. */ @Query( "DELETE FROM messages WHERE accountId = :accountId AND folder = :folder AND inInbox = 1 " + @@ -130,14 +128,16 @@ interface MessageDao { suspend fun syncedIdsOlderThan(accountId: String, cutoffMillis: Long): List /** - * Ids of an account's synced rows in [folder] beyond the newest [keep] by recency (count-based - * prune candidates). Ties broken by UID so the boundary is deterministic. + * Ids of an account's synced rows in [folder] beyond the newest [keep] by ARRIVAL (server UID — + * count-based prune candidates). Ordering by UID (not by the Date header) matches the newest-by-UID + * recent window [org.libremail.mail.ImapClient.fetchRecent] keeps fresh, so a message with a high + * UID but an old Date isn't re-fetched by every sync and re-pruned by every cycle. */ @Query( "SELECT id FROM messages WHERE accountId = :accountId AND folder = :folder AND inInbox = 1 " + "AND id NOT IN (" + "SELECT id FROM messages WHERE accountId = :accountId AND folder = :folder AND inInbox = 1 " + - "ORDER BY timestampMillis DESC, uid DESC LIMIT :keep)", + "ORDER BY uid DESC LIMIT :keep)", ) suspend fun syncedIdsBeyondCountInFolder(accountId: String, folder: String, keep: Int): List diff --git a/app/src/main/kotlin/org/libremail/data/local/entity/MessageEntity.kt b/app/src/main/kotlin/org/libremail/data/local/entity/MessageEntity.kt index 24170d7..0fb4b08 100644 --- a/app/src/main/kotlin/org/libremail/data/local/entity/MessageEntity.kt +++ b/app/src/main/kotlin/org/libremail/data/local/entity/MessageEntity.kt @@ -8,7 +8,10 @@ import androidx.room.PrimaryKey @Entity( tableName = "messages", - indices = [Index("accountId"), Index("timestampMillis")], + // The (accountId, folder, uid) index serves the folder-scoped UID probes the backfill/reconcile + // hot paths run on every page/sync: MIN(uid) (lowestSyncedUid) and the uid >= window bound + // (deleteSyncedInWindowNotIn / syncedIdsBeyondCountInFolder). + indices = [Index("accountId"), Index("timestampMillis"), Index("accountId", "folder", "uid")], ) data class MessageEntity( @PrimaryKey val id: String, diff --git a/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt b/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt index d123f78..f682538 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt @@ -79,4 +79,9 @@ class AccountRepositoryImpl @Inject constructor( folderDao.deleteForAccount(id) backfillProgressDao.deleteForAccount(id) } + + override suspend fun resetBackfillProgress(accountId: String?) { + if (accountId != null) backfillProgressDao.deleteForAccount(accountId) else backfillProgressDao.deleteAll() + syncScheduler.backfillNow() + } } diff --git a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt index 1f7d1ac..be3a569 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt @@ -9,6 +9,7 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.map import org.libremail.data.ReplyBuilder import org.libremail.data.SignatureBlock +import org.libremail.data.attachmentCacheDir import org.libremail.data.local.dao.AccountDao import org.libremail.data.local.dao.AttachmentDao import org.libremail.data.local.dao.DraftDao @@ -357,9 +358,8 @@ class MailRepositoryImpl @Inject constructor( * and avoids filename collisions between messages. */ private fun attachmentFile(messageId: String, partIndex: Int, filename: String): File { - val safeId = messageId.replace(Regex("[^A-Za-z0-9._-]"), "_") val safeName = filename.substringAfterLast('/').substringAfterLast('\\').ifBlank { "attachment" } - return File(context.cacheDir, "attachments/$safeId/$partIndex/$safeName") + return File(attachmentCacheDir(context.cacheDir, messageId), "$partIndex/$safeName") } } diff --git a/app/src/main/kotlin/org/libremail/data/settings/RetentionPolicy.kt b/app/src/main/kotlin/org/libremail/data/settings/RetentionPolicy.kt index ffe63ab..3e1e637 100644 --- a/app/src/main/kotlin/org/libremail/data/settings/RetentionPolicy.kt +++ b/app/src/main/kotlin/org/libremail/data/settings/RetentionPolicy.kt @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.data.settings +import kotlinx.coroutines.flow.first import java.time.Instant import java.time.ZoneOffset @@ -49,3 +50,25 @@ data class RetentionPolicy(val count: Int, val months: Int) { ) } } + +/** + * The effective retention policy for [accountId], resolving its per-account overrides against the + * global default. Read through the same [AccountSettingsRepository] / [SettingsRepository] every + * enforcement site uses — the foreground fetch window ([org.libremail.data.sync.MailSyncer]), the + * backfill floor ([org.libremail.data.sync.MailBackfiller]), and the pruner + * ([org.libremail.data.sync.MailPruner]) — so none of them can resolve a different floor, the + * divergence that would otherwise let backfill and prune fight over the same rows. + */ +internal suspend fun AccountSettingsRepository.effectiveRetention( + settings: SettingsRepository, + accountId: String, +): RetentionPolicy { + val account = get(accountId) + val global = settings.settings.first() + return RetentionPolicy.resolve( + accountCount = account.retentionCount, + accountMonths = account.retentionMonths, + defaultCount = global.retentionCount, + defaultMonths = global.retentionMonths, + ) +} diff --git a/app/src/main/kotlin/org/libremail/data/sync/BackfillWorker.kt b/app/src/main/kotlin/org/libremail/data/sync/BackfillWorker.kt index 68211d2..6d44e14 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/BackfillWorker.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/BackfillWorker.kt @@ -7,6 +7,7 @@ import androidx.work.CoroutineWorker import androidx.work.WorkerParameters import dagger.assisted.Assisted import dagger.assisted.AssistedInject +import kotlinx.coroutines.CancellationException /** * Runs one bounded slice of the full-history backfill (issue #12). Cancellable (WorkManager stops it @@ -21,8 +22,13 @@ class BackfillWorker @AssistedInject constructor( private val backfiller: MailBackfiller, ) : CoroutineWorker(appContext, workerParams) { - override suspend fun doWork(): Result = runCatching { backfiller.runBackfill() }.fold( + override suspend fun doWork(): Result = runCatching { + // Chain bounded slices back-to-back while history remains, so a large mailbox isn't limited to + // one slice per periodic run. runBackfill() returns true while any folder still has pages left; + // isStopped lets WorkManager end a long run gracefully (the periodic schedule resumes it). + while (backfiller.runBackfill() && !isStopped) { /* page the next slice */ } + }.fold( onSuccess = { Result.success() }, - onFailure = { Result.retry() }, + onFailure = { error -> if (error is CancellationException) throw error else Result.retry() }, ) } diff --git a/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt b/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt index b8653e2..2a200bd 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt @@ -2,14 +2,11 @@ package org.libremail.data.sync import android.content.Context -import android.net.ConnectivityManager -import android.net.NetworkCapabilities import dagger.hilt.android.qualifiers.ApplicationContext import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.delay import kotlinx.coroutines.ensureActive -import kotlinx.coroutines.flow.first import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import org.libremail.data.local.dao.AccountDao @@ -23,6 +20,7 @@ import org.libremail.data.settings.AccountSettingsRepository import org.libremail.data.settings.FetchPolicy import org.libremail.data.settings.RetentionPolicy import org.libremail.data.settings.SettingsRepository +import org.libremail.data.settings.effectiveRetention import org.libremail.domain.model.Account import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.repository.MailRepository @@ -67,7 +65,7 @@ class MailBackfiller @Inject constructor( var moreWork = false for (account in accountDao.getAll().map { it.toDomain() }) { val params = runCatching { connectionFactory.imapParamsFor(account) }.getOrNull() ?: continue - val policy = effectivePolicy(account.id) + val policy = accountSettingsRepository.effectiveRetention(settingsRepository, account.id) for (folder in messageDao.syncedFolders(account.id)) { if (remaining <= 0) return@withLock true // Per-folder failures (e.g. a transient server error) must not abort the whole slice. @@ -87,23 +85,29 @@ class MailBackfiller @Inject constructor( policy: RetentionPolicy, maxBatches: Int, ): FolderResult { - if (backfillProgressDao.get(account.id, folder)?.complete == true) { + val progress = backfillProgressDao.get(account.id, folder) + if (progress?.complete == true) { return FolderResult(batches = 0, complete = true) } - // Always page strictly below the LOWEST currently-cached UID. Deriving the boundary from the - // cache (rather than a stored cursor) keeps backfill gap-free even after the pruner raised the - // floor, and lets a later loosening of retention resume filling automatically. The mutex in - // runBackfill keeps the pruner from moving this boundary mid-run. - var beforeUid = messageDao.lowestSyncedUid(account.id, folder) ?: Long.MAX_VALUE + // Resume from the persisted low-water mark so paging is monotonic: it never re-descends into a + // region an earlier run already reached, even after the pruner deletes rows above it. Falls back + // to the lowest currently-cached UID on the very first run, before any progress is persisted. + var beforeUid = progress?.nextBeforeUid + ?: messageDao.lowestSyncedUid(account.id, folder) + ?: Long.MAX_VALUE var batches = 0 while (batches < maxBatches) { currentCoroutineContext().ensureActive() - // Retention floor (#13 precedence): pause — but do NOT mark complete — once the device-only - // limit is reached, so backfill and the pruner never contend for the same messages and a - // later loosening of the limit resumes paging from where it stopped. + // Retention floor (#13 precedence): once the folder holds everything retention keeps, mark it + // complete and stop. Marking complete (rather than pausing) is what keeps backfill and the + // pruner from fighting: otherwise the pruner deleting aged-out rows would raise the oldest + // cached timestamp back above the age cutoff and re-open paging on the next run, forever. A + // retention change resets progress (AccountRepository.resetBackfillProgress) so loosening + // still resumes paging. if (reachedRetentionFloor(account.id, folder, policy)) { - return FolderResult(batches, complete = false) + markComplete(account.id, folder, beforeUid) + return FolderResult(batches, complete = true) } val fetched = imapClient.fetchOlderThan(params, folder, beforeUid, BACKFILL_BATCH_SIZE) batches++ @@ -138,18 +142,25 @@ class MailBackfiller @Inject constructor( /** Inserts backfilled headers; never deletes. Uncancellable so a persisted boundary always has its rows. */ private suspend fun persistBatch(entities: List) = withContext(NonCancellable) { - messageDao.insertNew(entities) val ids = entities.map { it.id } - messageDao.markSynced(ids) - entities.forEach { - messageDao.updateHeaderContent( - id = it.id, - sender = it.sender, - senderEmail = it.senderEmail, - subject = it.subject, - timestampMillis = it.timestampMillis, - uid = it.uid, - ) + // insertNew (IGNORE) writes brand-new rows in full — headers, uid, and inInbox = 1 — so only rows + // that ALREADY existed (e.g. a former search-only row) need their membership/header refreshed. + // Limiting the updates to those avoids a redundant per-row UPDATE for every freshly-inserted row. + val preexisting = messageDao.existingIds(ids).toHashSet() + messageDao.insertNew(entities) + val toRefresh = entities.filter { it.id in preexisting } + if (toRefresh.isNotEmpty()) { + messageDao.markSynced(toRefresh.map { it.id }) + toRefresh.forEach { + messageDao.updateHeaderContent( + id = it.id, + sender = it.sender, + senderEmail = it.senderEmail, + subject = it.subject, + timestampMillis = it.timestampMillis, + uid = it.uid, + ) + } } } @@ -165,7 +176,7 @@ class MailBackfiller @Inject constructor( private suspend fun prefetchIfEnabled(ids: List) { val shouldPrefetch = when (settingsRepository.fetchPolicy()) { FetchPolicy.ALWAYS -> true - FetchPolicy.WIFI_ONLY -> isUnmetered() + FetchPolicy.WIFI_ONLY -> context.isActiveNetworkUnmetered() FetchPolicy.ON_DEMAND -> false } if (!shouldPrefetch) return @@ -175,23 +186,6 @@ class MailBackfiller @Inject constructor( } } - private suspend fun effectivePolicy(accountId: String): RetentionPolicy { - val account = accountSettingsRepository.get(accountId) - val global = settingsRepository.settings.first() - return RetentionPolicy.resolve( - accountCount = account.retentionCount, - accountMonths = account.retentionMonths, - defaultCount = global.retentionCount, - defaultMonths = global.retentionMonths, - ) - } - - private fun isUnmetered(): Boolean { - val manager = context.getSystemService(ConnectivityManager::class.java) ?: return false - val capabilities = manager.getNetworkCapabilities(manager.activeNetwork) ?: return false - return capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_METERED) - } - private companion object { /** Headers fetched per server page. */ const val BACKFILL_BATCH_SIZE = 50 diff --git a/app/src/main/kotlin/org/libremail/data/sync/MailPruner.kt b/app/src/main/kotlin/org/libremail/data/sync/MailPruner.kt index 562fff6..b922dcf 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailPruner.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailPruner.kt @@ -5,14 +5,14 @@ import android.content.Context import dagger.hilt.android.qualifiers.ApplicationContext import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.ensureActive -import kotlinx.coroutines.flow.first import kotlinx.coroutines.sync.withLock +import org.libremail.data.attachmentCacheDir import org.libremail.data.local.dao.AccountDao import org.libremail.data.local.dao.MessageDao import org.libremail.data.settings.AccountSettingsRepository import org.libremail.data.settings.RetentionPolicy import org.libremail.data.settings.SettingsRepository -import java.io.File +import org.libremail.data.settings.effectiveRetention import javax.inject.Inject import javax.inject.Singleton @@ -37,17 +37,10 @@ class MailPruner @Inject constructor( ) { /** Prunes every account to its effective retention policy. Returns the number of messages removed. */ suspend fun prune(nowMillis: Long = System.currentTimeMillis()): Int = maintenanceGate.mutex.withLock { - val global = settingsRepository.settings.first() var removed = 0 for (account in accountDao.getAll()) { currentCoroutineContext().ensureActive() - val settings = accountSettingsRepository.get(account.id) - val policy = RetentionPolicy.resolve( - accountCount = settings.retentionCount, - accountMonths = settings.retentionMonths, - defaultCount = global.retentionCount, - defaultMonths = global.retentionMonths, - ) + val policy = accountSettingsRepository.effectiveRetention(settingsRepository, account.id) if (policy.isUnlimited) continue removed += pruneAccount(account.id, policy, nowMillis) } @@ -80,8 +73,7 @@ class MailPruner @Inject constructor( /** Removes the per-message on-disk attachment cache (keyed the same way MailRepositoryImpl writes it). */ private fun deleteCacheFiles(messageId: String) { - val safeId = messageId.replace(Regex("[^A-Za-z0-9._-]"), "_") - runCatching { File(context.cacheDir, "attachments/$safeId").deleteRecursively() } + runCatching { attachmentCacheDir(context.cacheDir, messageId).deleteRecursively() } } private companion object { diff --git a/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt b/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt index b6a80b2..425a996 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt @@ -2,13 +2,10 @@ package org.libremail.data.sync import android.content.Context -import android.net.ConnectivityManager -import android.net.NetworkCapabilities import dagger.hilt.android.qualifiers.ApplicationContext import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.ensureActive -import kotlinx.coroutines.flow.first import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext @@ -18,8 +15,8 @@ import org.libremail.data.local.toDomain import org.libremail.data.local.toEntity import org.libremail.data.settings.AccountSettingsRepository import org.libremail.data.settings.FetchPolicy -import org.libremail.data.settings.RetentionPolicy import org.libremail.data.settings.SettingsRepository +import org.libremail.data.settings.effectiveRetention import org.libremail.domain.model.Account import org.libremail.domain.repository.MailRepository import org.libremail.mail.ImapClient @@ -43,7 +40,7 @@ class MailSyncer @Inject constructor( // Serializes all syncing: syncAll/syncAccount/syncFolder are invoked concurrently by the periodic // worker, pull-to-refresh, one-shot syncs, folder opens, and one IDLE watcher per account. Without // this, two runs can both compute the same message as "new" (double-notify) or let a stale - // deleteSyncedNotIn snapshot delete a row another run just inserted. + // deleteSyncedInWindowNotIn snapshot delete a row another run just inserted. private val syncMutex = Mutex() /** Syncs every account's inbox. Succeeds if at least one account synced (or there are none). */ @@ -128,9 +125,14 @@ class MailSyncer @Inject constructor( } // Reconcile server-side deletions ONLY within the fetched recent-UID window, so older // history paged in by the background backfill (issue #12) survives each foreground sync - // instead of being wiped by a whole-folder "not in the recent 50" delete. - val minWindowUid = entities.minOf { it.uid } - messageDao.deleteSyncedInWindowNotIn(account.id, folder, minWindowUid, ids) + // instead of being wiped by a whole-folder "not in the recent 50" delete. Bound the + // window by the lowest POSITIVE fetched UID: a message whose UID couldn't be resolved + // (UIDFolder.getUID returns -1) must not collapse the bound to <= 0 and turn this into a + // whole-folder delete that wipes the backfilled history below the window. + val minWindowUid = entities.mapNotNull { entity -> entity.uid.takeIf { it > 0L } }.minOrNull() + if (minWindowUid != null) { + messageDao.deleteSyncedInWindowNotIn(account.id, folder, minWindowUid, ids) + } } val shouldNotify = notify && @@ -150,14 +152,7 @@ class MailSyncer @Inject constructor( * would immediately trim. Age-only or unlimited retention leaves the full window in place. */ private suspend fun recentWindowFor(account: Account): Int { - val accountSettings = accountSettingsRepository.get(account.id) - val global = settingsRepository.settings.first() - val policy = RetentionPolicy.resolve( - accountCount = accountSettings.retentionCount, - accountMonths = accountSettings.retentionMonths, - defaultCount = global.retentionCount, - defaultMonths = global.retentionMonths, - ) + val policy = accountSettingsRepository.effectiveRetention(settingsRepository, account.id) return policy.countLimit?.let { minOf(FETCH_LIMIT, it) } ?: FETCH_LIMIT } @@ -169,7 +164,7 @@ class MailSyncer @Inject constructor( private suspend fun prefetchIfEnabled(account: Account, folder: String) { val shouldPrefetch = when (settingsRepository.fetchPolicy()) { FetchPolicy.ALWAYS -> true - FetchPolicy.WIFI_ONLY -> isUnmetered() + FetchPolicy.WIFI_ONLY -> context.isActiveNetworkUnmetered() FetchPolicy.ON_DEMAND -> false } if (!shouldPrefetch) return @@ -179,13 +174,6 @@ class MailSyncer @Inject constructor( } } - /** True when the active network is unmetered (e.g. Wi-Fi), used by [FetchPolicy.WIFI_ONLY]. */ - private fun isUnmetered(): Boolean { - val manager = context.getSystemService(ConnectivityManager::class.java) ?: return false - val capabilities = manager.getNetworkCapabilities(manager.activeNetwork) ?: return false - return capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_METERED) - } - private companion object { const val INBOX = "INBOX" diff --git a/app/src/main/kotlin/org/libremail/data/sync/NetworkStatus.kt b/app/src/main/kotlin/org/libremail/data/sync/NetworkStatus.kt new file mode 100644 index 0000000..49565bd --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/sync/NetworkStatus.kt @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.sync + +import android.content.Context +import android.net.ConnectivityManager +import android.net.NetworkCapabilities + +/** + * True when the device's active network is unmetered (e.g. Wi-Fi). Shared by [MailSyncer] and + * [MailBackfiller] so both background jobs agree on what `Wi-Fi only` prefetch means; a divergent + * copy would let one job download on cellular while the other doesn't. + */ +internal fun Context.isActiveNetworkUnmetered(): Boolean { + val manager = getSystemService(ConnectivityManager::class.java) ?: return false + val capabilities = manager.getNetworkCapabilities(manager.activeNetwork) ?: return false + return capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_METERED) +} diff --git a/app/src/main/kotlin/org/libremail/domain/repository/AccountRepository.kt b/app/src/main/kotlin/org/libremail/domain/repository/AccountRepository.kt index a571b2c..f25c0be 100644 --- a/app/src/main/kotlin/org/libremail/domain/repository/AccountRepository.kt +++ b/app/src/main/kotlin/org/libremail/domain/repository/AccountRepository.kt @@ -19,4 +19,11 @@ interface AccountRepository { suspend fun addOutlookAccount(email: String, accessToken: String, authStateJson: String): Result> suspend fun deleteAccount(id: String) + + /** + * Discards full-history backfill progress so it re-evaluates against the current retention floor + * after a retention change: tightening re-hits the (tighter) floor cheaply, loosening resumes + * paging older history. [accountId] null clears every account (a global-default change). + */ + suspend fun resetBackfillProgress(accountId: String?) } diff --git a/app/src/main/kotlin/org/libremail/ui/settings/AccountSettingsViewModel.kt b/app/src/main/kotlin/org/libremail/ui/settings/AccountSettingsViewModel.kt index f999bcd..dabf463 100644 --- a/app/src/main/kotlin/org/libremail/ui/settings/AccountSettingsViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/settings/AccountSettingsViewModel.kt @@ -66,6 +66,7 @@ class AccountSettingsViewModel @Inject constructor( viewModelScope.launch { accountSettingsRepository.setRetentionCount(accountId, value) syncScheduler.pruneNow() + accountRepository.resetBackfillProgress(accountId) } } @@ -73,6 +74,7 @@ class AccountSettingsViewModel @Inject constructor( viewModelScope.launch { accountSettingsRepository.setRetentionMonths(accountId, value) syncScheduler.pruneNow() + accountRepository.resetBackfillProgress(accountId) } } diff --git a/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt b/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt index 95094b7..1356f8f 100644 --- a/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt @@ -66,11 +66,13 @@ class SettingsViewModel @Inject constructor( fun setRetentionCount(value: Int) = update { settingsRepository.setRetentionCount(value) syncScheduler.pruneNow() + accountRepository.resetBackfillProgress(null) } fun setRetentionMonths(value: Int) = update { settingsRepository.setRetentionMonths(value) syncScheduler.pruneNow() + accountRepository.resetBackfillProgress(null) } private inline fun update(crossinline action: suspend () -> Unit) { diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt index a50feaf..d325e9d 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt @@ -35,6 +35,7 @@ import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.model.MailSecurity import org.libremail.domain.repository.MailRepository import org.libremail.mail.ImapClient +import java.util.Date import java.util.Properties import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -142,14 +143,52 @@ class MailBackfillerTest { assertTrue(afterFirst >= 60, "must fetch at least up to the retention floor") assertTrue(afterFirst < TOTAL, "must NOT page the entire 120-message history") - // Paused at the floor (NOT marked complete, so a later loosening could resume), and stable: - // running again fetches nothing more. - assertFalse(progress["acct" to "INBOX"]!!.complete) + // Marked complete at the floor so the pruner deleting aged-out rows can't re-open paging (a + // retention change resets progress to resume); stable — running again fetches nothing more. + assertTrue(progress["acct" to "INBOX"]!!.complete) backfiller.runBackfill() assertEquals(afterFirst, cached.size, "at the floor, further runs must not fetch more") assertNoDeletes() } + /** + * Regression for the #12/#13 AGE-retention contention: reaching the age floor marks the folder + * complete, so the pruner deleting aged-out rows — which raises the oldest cached timestamp back + * above the cutoff — can't re-open paging. Before the fix, the next run re-fetched exactly the rows + * the pruner had just deleted, an endless re-download/re-prune loop. (The count floor was already a + * fixpoint, so only an age-based case exercises this.) + */ + @Test + fun `reaching the age floor is sticky across a prune, so backfill never re-fetches`() = runTest { + val now = System.currentTimeMillis() + // UID order follows append order: the first 60 are ~8 months old (beyond the 6-month cutoff), + // the last 60 are recent (within retention). + val old = (1..60).map { now - 8 * MONTH_MILLIS - it * DAY_MILLIS } + val recent = (1..60).map { now - it * DAY_MILLIS } + appendMessages(old + recent) + seedForegroundWindow() + + val backfiller = backfiller(AccountSettings("acct", retentionMonths = 6)) + backfiller.runBackfill() + + assertEquals( + true, + progress["acct" to "INBOX"]!!.complete, + "backfill marks the folder complete at the age floor", + ) + val offeredBeforePrune = totalOffered + + // Simulate the pruner: drop every cached row older than the 6-month cutoff. This raises the + // oldest cached timestamp back above the cutoff — the state that used to re-open paging. + val cutoff = now - 6 * MONTH_MILLIS + cached.removeAll { it.timestampMillis < cutoff } + + backfiller.runBackfill() + + assertEquals(offeredBeforePrune, totalOffered, "a floored folder must not re-fetch after a prune") + assertNoDeletes() + } + /** Builds a backfiller wired to GreenMail with the in-memory fakes and the given account settings. */ private fun backfiller(accountSettings: AccountSettings): MailBackfiller { val accountDao = mockk() @@ -222,12 +261,17 @@ class MailBackfillerTest { private fun assertNoDeletes() { val dao = lastMessageDao ?: return coVerify(exactly = 0) { dao.deleteByIds(any()) } - coVerify(exactly = 0) { dao.deleteSyncedNotIn(any(), any(), any()) } coVerify(exactly = 0) { dao.deleteSyncedInWindowNotIn(any(), any(), any(), any()) } coVerify(exactly = 0) { dao.deleteSyncedByAccountFolder(any(), any()) } } - private fun appendMessages(count: Int) { + private fun appendMessages(count: Int) = appendMessages(List(count) { null }) + + /** + * Appends messages to INBOX with the given per-message sent dates (null = server default, ~now). + * UID order follows list order, so earlier entries get lower UIDs. + */ + private fun appendMessages(sentDates: List) { val props = Properties().apply { put("mail.store.protocol", "imap") put("mail.imap.host", "127.0.0.1") @@ -239,12 +283,13 @@ class MailBackfillerTest { try { val inbox = store.getFolder("INBOX") inbox.open(Folder.READ_WRITE) - val messages = (1..count).map { i -> + val messages = sentDates.mapIndexed { i, millis -> MimeMessage(session).apply { setFrom(InternetAddress("sender$i@example.org")) setRecipient(Message.RecipientType.TO, InternetAddress("alice@example.org")) subject = "Message $i" setText("Body of message $i") + if (millis != null) sentDate = Date(millis) } }.toTypedArray() inbox.appendMessages(messages) @@ -257,5 +302,7 @@ class MailBackfillerTest { private companion object { const val TOTAL = 120 const val WINDOW = 50 + private const val DAY_MILLIS = 24L * 60 * 60 * 1000 + private const val MONTH_MILLIS = 30L * DAY_MILLIS } }