fix(sync): make backfill age floor robust to out-of-order dates and guard lowestSyncedUid

Two code-review-derived backfill-correctness bugs (from the PR #46
review). Both govern where MailBackfiller stops and resumes paging a
folder, so they are fixed together.

(MIN(timestampMillis)), but paging descends by UID. One high-UID
message with an old Date header (moved/imported mail) dragged the
cached minimum below the cutoff and marked the folder complete while
lower-UID within-retention messages were still unfetched — a silent,
permanent gap (completion is sticky). The age floor is now decided from
each page actually fetched: only a page ENTIRELY older than the cutoff
(or folder exhaustion) ends paging, and such a prune-fodder page is not
persisted. The count floor keeps its cheap cache check — it orders by
UID like paging, so inversions can't bite it. oldestSyncedTimestamp had
no remaining caller and is removed.

migrated before the uid column existed, or a UIDFolder.getUID -1
fetch); fetchOlderThan treats beforeUid <= 1 as "nothing older", so the
folder was falsely marked fully backfilled. lowestSyncedUid now ignores
uid <= 0 rows (matching MailSyncer's minWindowUid guard), a stale
persisted boundary <= 0 is discarded on resume, and the per-page
descent takes min over positive UIDs only. A page of entirely
unresolved UIDs stalls the folder — it stays incomplete (a future
scheduled run retries) but reports no immediate more-work, so
BackfillWorker's slice-chaining loop can't busy-spin on it.

Together: #95 guarantees paging always descends with a real positive
UID boundary, and #94 makes the stop decision independent of cached
aggregates, so a placeholder or old-Dated row can no longer end
backfill early through either path. Completion stays sticky and is
declared only on positive evidence, preserving the #12/#13
backfill/pruner non-interference.

Tests (JVM, GreenMail + the existing in-memory DAO-fake harness; all
four fail against the pre-fix code): a high-UID/old-Date message must
not gap within-retention history (#94); an entirely-old page ends
paging without persisting prune-fodder (#94); a uid=0 row must not
poison the boundary (#95); a page of unresolvable UIDs stalls instead
of falsely completing (#95). MessageDaoRetentionTest pins the uid > 0
SQL guard against real SQLite and drops the removed oldestSyncedTimestamp
probe.

Closes #94
Closes #95

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
2026-07-02 02:20:31 -05:00
co-authored by Claude Fable 5
parent 45edb792a9
commit bce82452d0
4 changed files with 223 additions and 59 deletions
@@ -21,12 +21,12 @@ import org.libremail.data.local.entity.MessageEntity
*
* These queries *define* the device-only retention floor — the pruner deletes below it
* ([MessageDao.syncedIdsBeyondCountInFolder] / [MessageDao.syncedIdsOlderThan]) and the backfiller
* stops above it ([MessageDao.lowestSyncedUid] / [MessageDao.countSynced] /
* [MessageDao.oldestSyncedTimestamp]) — so the whole "backfill and prune never fight over the same
* rows" guarantee rests on their SQL. The [org.libremail.data.sync.MailPruner] /
* [org.libremail.data.sync.MailBackfiller] unit tests mock the DAO, so the `ORDER BY … DESC LIMIT`
* newest-N selection, the strict age cutoff, and the windowed reconcile that spares backfilled history
* are exercised here against a real database instead.
* pages and stops around it ([MessageDao.lowestSyncedUid] / [MessageDao.countSynced]) — so the whole
* "backfill and prune never fight over the same rows" guarantee rests on their SQL. The
* [org.libremail.data.sync.MailPruner] / [org.libremail.data.sync.MailBackfiller] unit tests mock
* the DAO, so the `ORDER BY … DESC LIMIT` newest-N selection, the strict age cutoff, and the
* windowed reconcile that spares backfilled history are exercised here against a real database
* instead.
*/
@RunWith(AndroidJUnit4::class)
class MessageDaoRetentionTest {
@@ -153,6 +153,9 @@ class MessageDaoRetentionTest {
/**
* The backfiller's floor probes reflect only an account's synced rows in the given folder, and are
* null/zero for a folder with nothing cached (so the backfiller then starts from `Long.MAX_VALUE`).
* The paging boundary additionally skips `uid <= 0` placeholder rows (#95): a row migrated before
* the `uid` column existed (backfilled to 0) or one whose UID the server failed to resolve (-1)
* must not collapse MIN(uid) to a bound the backfiller treats as "folder fully paged".
*/
@Test
fun floorProbesReflectOnlySyncedRowsInTheFolder() = runBlocking {
@@ -160,18 +163,22 @@ class MessageDaoRetentionTest {
listOf(
message("a", uid = 30, timestampMillis = 300),
message("d", uid = 10, timestampMillis = 100),
message("legacy", uid = 0, timestampMillis = 40), // pre-uid-column migration row (#95)
message("unresolved", uid = -1, timestampMillis = 30), // UIDFolder.getUID failure (#95)
message("search", uid = 1, timestampMillis = 1, inInbox = false), // excluded
message("archive", uid = 5, timestampMillis = 50, folder = "Archive"), // different folder
),
)
assertEquals(10L, dao.lowestSyncedUid("acct", "INBOX"))
assertEquals(2, dao.countSynced("acct", "INBOX"))
assertEquals(100L, dao.oldestSyncedTimestamp("acct", "INBOX"))
assertEquals(4, dao.countSynced("acct", "INBOX"))
assertNull(dao.lowestSyncedUid("acct", "Nonexistent"))
assertEquals(0, dao.countSynced("acct", "Nonexistent"))
assertNull(dao.oldestSyncedTimestamp("acct", "Nonexistent"))
// A folder holding ONLY placeholder rows has no usable boundary: null (start from the top).
dao.insertNew(listOf(message("only-legacy", uid = 0, folder = "Imported")))
assertNull(dao.lowestSyncedUid("acct", "Imported"))
}
/** Backfill / prune enumerate their targets via [syncedFolders]: distinct synced folders, per account. */
@@ -104,21 +104,25 @@ interface MessageDao {
)
suspend fun deleteSyncedInWindowNotIn(accountId: String, folder: String, minWindowUid: Long, keepIds: List<String>)
/** Lowest cached UID among an account's synced rows in [folder] — the backfill boundary. Null if none. */
@Query("SELECT MIN(uid) FROM messages WHERE accountId = :accountId AND folder = :folder AND inInbox = 1")
/**
* Lowest cached *resolved* UID among an account's synced rows in [folder] — the backfill
* boundary. Placeholder rows with `uid <= 0` (a row migrated before the `uid` column existed, or
* a fetch where the server failed to resolve the UID) are excluded: letting one collapse
* MIN(uid) to `<= 0` would make the backfiller page below a bound `fetchOlderThan` treats as
* "nothing older", falsely marking the folder fully backfilled (#95, matching the
* `minWindowUid` guard in MailSyncer). Null when no resolved-UID row exists, in which case
* backfill starts over from the newest message.
*/
@Query(
"SELECT MIN(uid) FROM messages WHERE accountId = :accountId AND folder = :folder " +
"AND inInbox = 1 AND uid > 0",
)
suspend fun lowestSyncedUid(accountId: String, folder: String): Long?
/** Number of an account's synced rows in [folder] (count-based retention floor / prune sizing). */
@Query("SELECT COUNT(*) FROM messages WHERE accountId = :accountId AND folder = :folder AND inInbox = 1")
suspend fun countSynced(accountId: String, folder: String): Int
/** Oldest cached timestamp among an account's synced rows in [folder] (age-based retention floor). Null if none. */
@Query(
"SELECT MIN(timestampMillis) FROM messages " +
"WHERE accountId = :accountId AND folder = :folder AND inInbox = 1",
)
suspend fun oldestSyncedTimestamp(accountId: String, folder: String): Long?
/** Distinct folders that have at least one synced row for [accountId] (backfill/prune targets). */
@Query("SELECT DISTINCT folder FROM messages WHERE accountId = :accountId AND inInbox = 1")
suspend fun syncedFolders(accountId: String): List<String>
@@ -54,12 +54,15 @@ class MailBackfiller @Inject constructor(
private val mailRepository: MailRepository,
private val maintenanceGate: MailMaintenanceGate,
) {
private data class FolderResult(val batches: Int, val complete: Boolean)
/** One folder's slice outcome: pages fetched, and whether an immediate follow-up slice has work to do. */
private data class FolderResult(val batches: Int, val moreWork: Boolean)
/**
* Runs one bounded slice of backfill across all accounts and their synced folders. Does at most
* [maxBatches] server pages total, persisting progress after each, then returns whether any
* folder still has history left to fetch (so the caller may schedule another run sooner).
* [maxBatches] server pages total, persisting progress after each, then returns whether an
* immediate follow-up slice has more work to do (so the caller may chain another run). A folder
* that stalled (see the unresolved-UID guard in [backfillFolder]) stays incomplete but does not
* count as more work — it is retried on a future scheduled run instead of spun on back-to-back.
*/
suspend fun runBackfill(maxBatches: Int = DEFAULT_MAX_BATCHES): Boolean = maintenanceGate.mutex.withLock {
var remaining = maxBatches
@@ -71,9 +74,9 @@ class MailBackfiller @Inject constructor(
if (remaining <= 0) return@withLock true
// Per-folder failures (e.g. a transient server error) must not abort the whole slice.
val result = runCatching { backfillFolder(account, params, folder, policy, remaining) }
.getOrElse { FolderResult(batches = 0, complete = false) }
.getOrElse { FolderResult(batches = 0, moreWork = true) }
remaining -= result.batches
if (!result.complete) moreWork = true
if (result.moreWork) moreWork = true
}
}
moreWork
@@ -88,57 +91,88 @@ class MailBackfiller @Inject constructor(
): FolderResult {
val progress = backfillProgressDao.get(account.id, folder)
if (progress?.complete == true) {
return FolderResult(batches = 0, complete = true)
return FolderResult(batches = 0, moreWork = false)
}
// 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
// Both sources are guarded against unresolved-UID placeholders (#95): lowestSyncedUid excludes
// `uid <= 0` rows at the SQL level, and a stale persisted boundary `<= 0` is discarded rather
// than trusted — fetchOlderThan treats such a bound as "nothing older", which would falsely
// mark the folder fully backfilled.
var beforeUid = progress?.nextBeforeUid?.takeIf { it > 0L }
?: messageDao.lowestSyncedUid(account.id, folder)
?: Long.MAX_VALUE
var batches = 0
var complete = false
var stalled = false
while (batches < maxBatches) {
currentCoroutineContext().ensureActive()
// 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)) {
markComplete(account.id, folder, beforeUid)
return FolderResult(batches, complete = true)
// Count floor (#13 precedence): once the folder holds as many messages as retention keeps,
// stop before fetching another page. Unlike the age floor below, this can be decided from
// the cache alone: the count pruner keeps the newest-N by UID — exactly the order paging
// descends in — so a Date/UID inversion cannot make it stop early.
if (reachedCountFloor(account.id, folder, policy)) {
complete = true
break
}
val fetched = imapClient.fetchOlderThan(params, folder, beforeUid, BACKFILL_BATCH_SIZE)
batches++
if (fetched.isEmpty()) {
// Genuine end of the folder — mark complete so it is skipped on future runs.
markComplete(account.id, folder, beforeUid)
return FolderResult(batches, complete = true)
}
val entities = fetched.map { it.toEntity(account.id, folder) }
// Stop at the genuine end of the folder, or at the age floor (#13): a page ENTIRELY older
// than the Date cutoff. The age decision is made from the page actually fetched, NOT from
// the oldest cached timestamp — paging is by UID (arrival order) while the floor cuts by
// the Date header, so a single high-UID message with an old Date (moved/imported mail)
// would drag the cached minimum below the cutoff and end paging while within-retention
// history is still unfetched (#94). An entirely-old page is pure prune-fodder, so it is
// not persisted either. Both cases mark the folder complete (rather than pausing): the
// pruner deleting aged-out rows must never re-open paging on the next run, forever
// re-downloading what was just pruned. A retention change resets progress
// (AccountRepository.resetBackfillProgress) so loosening still resumes paging.
if (fetched.isEmpty() || entirelyBeyondAgeFloor(entities, policy)) {
complete = true
break
}
persistBatch(entities)
beforeUid = entities.minOf { it.uid }
// Derive the next boundary only from resolved UIDs (#95): a row whose UID the server
// failed to resolve (UIDFolder.getUID returns -1) must not collapse the boundary to <= 0,
// where fetchOlderThan reads "nothing older" and the folder would be FALSELY marked fully
// backfilled. If a whole page came back unresolved, stall the folder: not complete (so a
// later run retries once the server behaves) but claiming no more work either — an
// immediate follow-up slice would just spin on the same page.
val nextBeforeUid = entities.mapNotNull { entity -> entity.uid.takeIf { it > 0L } }.minOrNull()
if (nextBeforeUid == null) {
stalled = true
break
}
beforeUid = nextBeforeUid
backfillProgressDao.upsert(BackfillProgressEntity(account.id, folder, beforeUid, complete = false))
prefetchIfEnabled(entities.map { it.id })
// Breathe between pages so a large mailbox doesn't hammer the server.
delay(BACKFILL_BATCH_DELAY_MS)
}
return FolderResult(batches, complete = false)
if (complete) markComplete(account.id, folder, beforeUid)
return FolderResult(batches, moreWork = !complete && !stalled)
}
/** True once the folder already holds as much as the retention policy would keep (or more). */
private suspend fun reachedRetentionFloor(accountId: String, folder: String, policy: RetentionPolicy): Boolean {
if (policy.isUnlimited) return false
policy.countLimit?.let { limit ->
if (messageDao.countSynced(accountId, folder) >= limit) return true
}
policy.ageCutoffMillis(System.currentTimeMillis())?.let { cutoff ->
val oldest = messageDao.oldestSyncedTimestamp(accountId, folder)
if (oldest != null && oldest < cutoff) return true
}
return false
/** True once the folder already holds as many messages as the count retention keeps (or more). */
private suspend fun reachedCountFloor(accountId: String, folder: String, policy: RetentionPolicy): Boolean {
val limit = policy.countLimit ?: return false
return messageDao.countSynced(accountId, folder) >= limit
}
/**
* True when a fetched page sits entirely below the age retention floor — every message on it is
* older than the policy's Date cutoff. Deciding per page (rather than from the single oldest
* cached timestamp) makes the floor robust to Date/UID inversions (#94): one old-Dated high-UID
* message ends paging only if a whole page around it is old too. The trade is deliberate — a
* pathologically interleaved mailbox may over-fetch (the pruner reclaims the excess), but
* backfill never silently gaps within-retention history.
*/
private fun entirelyBeyondAgeFloor(page: List<MessageEntity>, policy: RetentionPolicy): Boolean {
val cutoff = policy.ageCutoffMillis(System.currentTimeMillis()) ?: return false
return page.isNotEmpty() && page.all { it.timestampMillis < cutoff }
}
/** Inserts backfilled headers; never deletes. Uncancellable so a persisted boundary always has its rows. */
@@ -34,6 +34,7 @@ import org.libremail.domain.model.AccountSettings
import org.libremail.domain.model.ImapConnectionParams
import org.libremail.domain.model.MailSecurity
import org.libremail.domain.repository.MailRepository
import org.libremail.mail.FetchedMessage
import org.libremail.mail.ImapClient
import org.libremail.power.BatteryStatus
import org.libremail.power.BatteryStatusProvider
@@ -41,6 +42,7 @@ import java.util.Date
import java.util.Properties
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertNotEquals
import kotlin.test.assertNotNull
import kotlin.test.assertTrue
@@ -221,11 +223,131 @@ class MailBackfillerTest {
coVerify(atLeast = 1) { requireNotNull(lastMailRepository).prefetchMessage(any()) }
}
/** Builds a backfiller wired to GreenMail with the in-memory fakes and the given account settings. */
/**
* Regression for #94: backfill pages by UID (arrival order) while the age floor cuts by the Date
* header. A single message with a HIGH UID but an OLD Date (mail moved/imported into the folder)
* used to drag the oldest cached timestamp below the cutoff and stop paging after zero pages,
* silently gapping every older-UID message whose Date is still within retention. The floor must
* instead be decided from the pages actually fetched.
*/
@Test
fun `a high-UID old-Date message must not stop the age floor while history is unfetched`() = runTest {
val now = System.currentTimeMillis()
// UIDs 1..20: genuinely old (~8 months). UIDs 21..99: within retention, Dates ascending with
// UID. UID 100: a recent arrival whose Date header is 8 months old — the Date/UID inversion —
// which lands inside the seeded foreground window.
val old = (1..20).map { now - 8 * MONTH_MILLIS - it * DAY_MILLIS }
val recent = (1..79).map { now - (80 - it) * DAY_MILLIS }
val inverted = listOf(now - 8 * MONTH_MILLIS)
appendMessages(old + recent + inverted)
seedForegroundWindow()
val backfiller = backfiller(AccountSettings("acct", retentionMonths = 6))
var guard = 0
while (backfiller.runBackfill() && guard++ < 10) { /* drive to completion */ }
val cutoff = now - 6 * MONTH_MILLIS
assertEquals(
recent.size,
cached.count { it.timestampMillis >= cutoff },
"every within-retention message must be cached — no silent history gap",
)
assertEquals(true, progress["acct" to "INBOX"]?.complete, "paging still terminates at the floor")
assertNoDeletes()
}
/**
* The flip side of the page-based age floor (#94): a page ENTIRELY older than the cutoff ends
* paging — the folder is marked complete without caching that page, which is pure prune-fodder
* (persisting it would just make the pruner delete it again, the #12/#13 churn the sticky floor
* exists to prevent).
*/
@Test
fun `an entirely-old page ends age paging without caching beyond the floor`() = runTest {
val now = System.currentTimeMillis()
// UIDs 1..50 are ~8 months old; UIDs 51..100 are within retention and fill the whole window.
val old = (1..50).map { now - 8 * MONTH_MILLIS - (51 - it) * DAY_MILLIS }
val recent = (1..50).map { now - (51 - it) * DAY_MILLIS }
appendMessages(old + recent)
seedForegroundWindow()
backfiller(AccountSettings("acct", retentionMonths = 6)).runBackfill()
assertEquals(true, progress["acct" to "INBOX"]?.complete, "one entirely-old page ends the folder")
assertEquals(0, totalOffered, "the beyond-the-floor page must not be cached")
val cutoff = now - 6 * MONTH_MILLIS
assertTrue(cached.none { it.timestampMillis < cutoff }, "nothing older than the cutoff is cached")
assertNoDeletes()
}
/**
* Regression for #95: a cached row with `uid = 0` — a row migrated before the `uid` column
* existed (MIGRATION_12_13 backfills a non-numeric id tail to 0) — used to collapse MIN(uid) to
* 0, and `fetchOlderThan` treats a bound `<= 1` as "nothing older": the folder was marked fully
* backfilled after caching NOTHING. The boundary must come from resolved (positive) UIDs only.
*/
@Test
fun `a uid 0 placeholder row must not poison the backfill boundary`() = runTest {
appendMessages(TOTAL)
seedForegroundWindow()
// A pre-uid-column row, exactly as MIGRATION_12_13 leaves one whose id tail isn't numeric.
cached += cached.first().copy(id = "acct:INBOX:legacy", uid = 0L)
val backfiller = backfiller(AccountSettings("acct"))
var guard = 0
while (backfiller.runBackfill() && guard++ < 10) { /* drive to completion */ }
assertEquals(
TOTAL,
cached.mapTo(HashSet()) { it.uid }.count { it > 0L },
"the placeholder row must not stop backfill from paging the full history",
)
assertEquals(TOTAL - WINDOW, totalOffered, "each backfilled message fetched exactly once")
assertEquals(true, progress["acct" to "INBOX"]?.complete)
assertNoDeletes()
}
/**
* Regression for #95 (server variant): when every UID on a fetched page is unresolvable
* (UIDFolder.getUID returned -1), the boundary must not descend to -1 — the next fetch would come
* back empty and the folder would be FALSELY marked fully backfilled. The folder must instead
* stall: stay incomplete (retryable on a future run) without claiming immediate more-work (which
* would make the worker's slice-chaining loop spin on the same page).
*/
@Test
fun `a page of unresolvable UIDs stalls the folder instead of falsely completing it`() = runTest {
// One cached window row makes INBOX a backfill target with boundary UID 60.
cached += fetchedMessage(uid = "60").toEntity("acct", "INBOX")
val imapClient = mockk<ImapClient>()
coEvery { imapClient.fetchOlderThan(any(), any(), any(), any()) } answers {
// The real client's contract: a collapsed boundary (<= 1) means "nothing older".
if (thirdArg<Long>() <= 1L) emptyList() else listOf(fetchedMessage(uid = "-1"))
}
val moreWork = backfiller(AccountSettings("acct"), imapClient = imapClient).runBackfill()
assertNotEquals(true, progress["acct" to "INBOX"]?.complete, "the folder must stay retryable")
assertFalse(moreWork, "a stalled folder must not spin the worker's slice-chaining loop")
coVerify(exactly = 0) { imapClient.fetchOlderThan(any(), any(), match { it <= 1L }, any()) }
}
private fun fetchedMessage(uid: String) = FetchedMessage(
uid = uid,
sender = "Sender",
senderEmail = "sender@example.org",
subject = "Message $uid",
timestampMillis = System.currentTimeMillis(),
isRead = false,
isFlagged = false,
)
/** Builds a backfiller wired to [imapClient] (GreenMail-backed by default) with the in-memory fakes. */
private fun backfiller(
accountSettings: AccountSettings,
fetchPolicy: FetchPolicy = FetchPolicy.ON_DEMAND,
battery: BatteryStatus = BatteryStatus(percent = 100, isCharging = false),
imapClient: ImapClient = client,
): MailBackfiller {
val accountDao = mockk<AccountDao>()
coEvery { accountDao.getAll() } returns listOf(accountEntity)
@@ -241,16 +363,13 @@ class MailBackfillerTest {
}
coEvery { messageDao.lowestSyncedUid("acct", any()) } answers {
val folder = secondArg<String>()
cached.filter { it.inInbox && it.folder == folder }.minOfOrNull { it.uid }
// Mirrors the real query's `uid > 0` guard (#95): placeholder rows never drive the boundary.
cached.filter { it.inInbox && it.folder == folder && it.uid > 0L }.minOfOrNull { it.uid }
}
coEvery { messageDao.countSynced("acct", any()) } answers {
val folder = secondArg<String>()
cached.count { it.inInbox && it.folder == folder }
}
coEvery { messageDao.oldestSyncedTimestamp("acct", any()) } answers {
val folder = secondArg<String>()
cached.filter { it.inInbox && it.folder == folder }.minOfOrNull { it.timestampMillis }
}
val backfillProgressDao = mockk<BackfillProgressDao>(relaxed = true)
coEvery { backfillProgressDao.get("acct", any()) } answers { progress["acct" to secondArg<String>()] }
@@ -277,7 +396,7 @@ class MailBackfillerTest {
accountDao = accountDao,
messageDao = messageDao,
backfillProgressDao = backfillProgressDao,
imapClient = client,
imapClient = imapClient,
connectionFactory = connectionFactory,
settingsRepository = settingsRepository,
accountSettingsRepository = accountSettingsRepository,