From e823f9b17e4bc2980affbd6c38d383ae3dc7be3a Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Fri, 3 Jul 2026 13:17:24 -0500 Subject: [PATCH] fix(sync): bound the foreground fetch window by the age cutoff in age retention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In age-based retention, MailSyncer fetched the newest-N headers but only capped that window by the retention COUNT, not the age cutoff. On a low-traffic mailbox whose newest-N span older than the cutoff, each sync re-inserted messages the age pruner had just deleted, and the next prune deleted them again — a churn loop of wasted DB writes + prune deletes (issue #193). Sync now drops fetched messages older than policy.ageCutoffMillis before persisting (the same cutoff the pruner uses), so sync and prune keep exactly the same set in both retention modes. Count/unlimited modes have a null cutoff and are unchanged. The empty-folder wipe is keyed on the raw fetch (server truth), so a folder holding only past-cutoff mail is left to the pruner rather than wiped. MailPruner's KDoc now documents the sync alignment for both modes. Closes #193 Co-Authored-By: Claude Opus 4.8 --- .../org/libremail/data/sync/MailPruner.kt | 5 +++ .../org/libremail/data/sync/MailSyncer.kt | 26 ++++++++-------- .../org/libremail/data/sync/MailSyncerTest.kt | 31 ++++++++++++++++++- 3 files changed, 48 insertions(+), 14 deletions(-) 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 b922dcf..07a9d8b 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailPruner.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailPruner.kt @@ -25,6 +25,11 @@ import javax.inject.Singleton * Precedence with the #12 backfill is guaranteed two ways: backfill stops paging at the same * retention floor this pruner deletes below (their working sets are disjoint), and both jobs share * [MailMaintenanceGate] so they never run at once. + * + * Foreground sync ([MailSyncer]) is aligned the same way in BOTH retention modes so it never + * re-inserts what this pruner deletes: it caps its fetch window to the retention count AND drops + * anything older than the age cutoff before persisting (#193). The two limits are independent, so + * both bounds apply together. */ @Singleton class MailPruner @Inject constructor( 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 b059125..e701ae9 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt @@ -87,11 +87,19 @@ class MailSyncer @Inject constructor( private suspend fun syncFolderHeaders(account: Account, folder: String, notify: Boolean): Result = runCatching { val params = connectionFactory.imapParamsFor(account) + val policy = accountSettingsRepository.effectiveRetention(settingsRepository, account.id) // Never fetch more of the recent window than device-only retention (#13) would keep. Without // this, a count limit BELOW the window would make foreground sync re-download the same rows // the pruner just trimmed, on every sync — an endless re-download/re-prune fight. - val fetched = imapClient.fetchRecent(params, folder, recentWindowFor(account)) // cancellable network I/O + val window = policy.countLimit?.let { minOf(FETCH_LIMIT, it) } ?: FETCH_LIMIT + val fetched = imapClient.fetchRecent(params, folder, window) // cancellable network I/O + // Age-based retention (#193): drop anything older than the age cutoff before persisting. On a + // low-traffic mailbox the newest-N can extend PAST the cutoff, so without this a sync re-inserts + // rows the age pruner just deleted and the next prune deletes them again — a churn loop. Count/ + // unlimited modes have a null cutoff and keep the full window, so their behavior is unchanged. + val cutoff = policy.ageCutoffMillis(System.currentTimeMillis()) val entities = fetched.map { it.toEntity(account.id, folder) } + .let { mapped -> if (cutoff == null) mapped else mapped.filter { it.timestampMillis >= cutoff } } // Persist and notify atomically with respect to cancellation: an IDLE renewal that cancels // mid-sync must not drop a notification (the rows would then look "already seen" next time). @@ -104,9 +112,11 @@ class MailSyncer @Inject constructor( entities.filter { it.id !in existingIds && !it.isRead } } - if (entities.isEmpty()) { + if (fetched.isEmpty()) { // An empty recent window means the server folder itself is empty, so nothing (not - // even backfilled history) should remain cached for it. + // even backfilled history) should remain cached for it. Keyed on the raw fetch, not the + // age-filtered set: a folder holding only mail older than the age cutoff is NOT empty on + // the server, so its stale local rows are left to the pruner rather than wiped here. messageDao.deleteSyncedByAccountFolder(account.id, folder) } else { val ids = entities.map { it.id } @@ -147,16 +157,6 @@ class MailSyncer @Inject constructor( fetched.size } - /** - * The number of recent headers to fetch: the standard [FETCH_LIMIT], but capped by the account's - * effective device-only retention count so foreground sync never re-downloads rows the pruner - * would immediately trim. Age-only or unlimited retention leaves the full window in place. - */ - private suspend fun recentWindowFor(account: Account): Int { - val policy = accountSettingsRepository.effectiveRetention(settingsRepository, account.id) - return policy.countLimit?.let { minOf(FETCH_LIMIT, it) } ?: FETCH_LIMIT - } - /** * Aggressively pre-caches each not-yet-fetched message's full content (body + attachments) per the * user's fetch policy, pausing at low battery regardless of policy — see diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt index 1574f54..af745fb 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt @@ -44,6 +44,9 @@ class MailSyncerTest { /** The IMAP client of the most recently built [syncer], for verifying the fetch window size. */ private lateinit var lastImapClient: ImapClient + /** The MessageDao of the most recently built [syncer], for verifying what got persisted. */ + private lateinit var lastMessageDao: MessageDao + /** A syncer whose header sync is a no-op (no server messages) so tests focus on the prefetch step. */ private fun syncer( policy: FetchPolicy, @@ -52,14 +55,16 @@ class MailSyncerTest { accountSettings: AccountSettings = AccountSettings("acct"), globalSettings: AppSettings = AppSettings(), battery: BatteryStatus = BatteryStatus(percent = 100, isCharging = false), + fetched: List = emptyList(), ): MailSyncer { val accountDao = mockk() coEvery { accountDao.getById("acct") } returns account val messageDao = mockk(relaxed = true) coEvery { messageDao.getSyncedIds(any(), any()) } returns emptyList() coEvery { messageDao.getUnfetchedIds("acct", "INBOX") } returns listOf("acct:INBOX:1") + lastMessageDao = messageDao val imapClient = mockk() - coEvery { imapClient.fetchRecent(any(), any(), any()) } returns emptyList() + coEvery { imapClient.fetchRecent(any(), any(), any()) } returns fetched lastImapClient = imapClient val connectionFactory = mockk() coEvery { connectionFactory.imapParamsFor(any()) } returns mockk() @@ -135,6 +140,30 @@ class MailSyncerTest { coVerify { lastImapClient.fetchRecent(any(), "INBOX", 50) } } + @Test + fun `age retention does not re-insert fetched messages older than the cutoff`() = runTest { + val repo = mockk(relaxed = true) + val now = System.currentTimeMillis() + val day = 24L * 60 * 60 * 1000 + val recentTs = now - 10 * day // well within a 6-month window + val oldTs = now - 400 * day // well past a 6-month window — the pruner would delete it + val fetched = listOf( + FetchedMessage("2", "New", "new@example.org", "recent", recentTs, isRead = true, isFlagged = false), + FetchedMessage("1", "Old", "old@example.org", "stale", oldTs, isRead = true, isFlagged = false), + ) + + syncer( + FetchPolicy.ON_DEMAND, + repo, + accountSettings = AccountSettings("acct", retentionMonths = 6), + fetched = fetched, + ).syncFolder("acct", "INBOX") + + // Only the in-window message is persisted; the past-cutoff one is never re-inserted, so the age + // pruner won't just delete it again next cycle (#193 — no re-download/re-prune churn loop). + coVerify { lastMessageDao.insertNew(match { batch -> batch.map { it.timestampMillis } == listOf(recentTs) }) } + } + @Test fun `WIFI_ONLY prefetches on an unmetered network`() = runTest { val repo = mockk() -- 2.47.3