Two code-review-derived correctness bugs in the full-history backfill (from the PR #46 review), fixed together because both govern the same decision: where MailBackfiller stops and resumes paging a folder.
#94 — age floor stopped early on out-of-order Date/UID
Root cause. Backfill pages a folder newest-to-oldest by UID (arrival order), but the age retention floor was decided from MIN(timestampMillis) of the cached rows — the Date header. One message with a high UID but an old Date (mail moved/imported into the folder) dragged that minimum below the cutoff, and reachedRetentionFloor marked the folder complete while lower-UID messages with within-retention Dates were still unfetched: a silent, permanent history gap (completion is deliberately sticky).
Fix. The age floor is now decided from the page actually fetched, not from cached aggregates: paging ends only when a fetched page is entirely older than the cutoff (or the folder is exhausted). A single inverted message can no longer end paging; only a whole old page — strong evidence the descent has left the retention window — can. Such a terminal page is pure prune-fodder, so it is not persisted (no insert-then-prune churn with MailPruner). The trade is deliberate and documented: a pathologically interleaved mailbox may over-fetch (the pruner reclaims the excess) but backfill never silently gaps. The count floor keeps its cheap pre-fetch cache check — it orders by UID, exactly like paging, so inversions can't bite it. MessageDao.oldestSyncedTimestamp lost its last caller and is removed.
Root cause.lowestSyncedUid was a bare MIN(uid). Rows with uid <= 0 exist by design: MIGRATION_12_13 backfills a non-numeric id tail to 0, and a fetch where UIDFolder.getUID returns -1 maps to -1. One such row collapsed the resume boundary to <= 0, ImapClient.fetchOlderThan treats beforeUid <= 1 as "nothing older", and the folder was falsely marked fully backfilled after fetching nothing — sticky completion again made the under-fetch permanent.
Fix. Positive-UID guards at every boundary derivation, mirroring the minWindowUid guard PR #46 already added to MailSyncer:
lowestSyncedUid now excludes uid <= 0 at the SQL level (AND uid > 0; null → backfill starts from the newest message and heals the placeholder via persistBatch's header refresh);
a stale persisted nextBeforeUid <= 0 is discarded on resume rather than trusted;
the per-page descent takes min over resolved (positive) UIDs only. If a whole page comes back unresolved, the folder stalls: it stays incomplete (a future scheduled run retries) but reports no immediate more-work — otherwise BackfillWorker's slice-chaining loop (while (runBackfill()) …) would spin on the same page.
Why no data fix / migration: a migration cannot restore real UIDs (they live on the server). The existing design already heals placeholders — updateHeaderContent refreshes uid on the next sync/backfill pass — so the schema is untouched (no version bump, no app/schemas change); the DAO guard just stops placeholders from steering paging in the meantime.
How the two fixes interact
Both bugs were "backfill quietly stops too early" with sticky completion sealing the gap; the fixes compose in backfillFolder's single loop:
#95 guarantees paging always descends with a real, positive UID boundary (resume + per-page), so #94's page-based floor always evaluates real pages.
#94 makes the stop decision independent of cached aggregates, which also kills the compound case: a migrated uid = 0 row with an old Date could previously end backfill through either path (boundary collapse or premature age floor). Now it can do neither.
Completion stays sticky and is only ever declared on positive evidence (count floor, folder exhaustion, or an entirely-old page), so the #12/#13 backfill/pruner non-interference (shared MailMaintenanceGate, disjoint working sets, no re-open after prune) is preserved — the existing regression tests for it still pass unchanged.
Tests
JVM unit tests (GreenMail real in-process IMAP + the existing in-memory DAO-fake harness in MailBackfillerTest); all four fail against the pre-fix code (verified by reverting the fix locally):
#94 repro: a high-UID/old-Date message inside the seeded window with retentionMonths = 6 — asserts every within-retention message gets cached (no gap) and paging still terminates.
#94 floor semantics: an entirely-old page ends paging without persisting prune-fodder and without paging the whole folder.
#95 repro (migration variant): a cached uid = 0 row (as MIGRATION_12_13 leaves for a non-numeric id tail) — asserts the full history is still paged and each message is fetched exactly once.
#95 repro (server variant): a page of getUID = -1 messages — asserts the folder is not falsely marked complete, the boundary never collapses to <= 1, and the worker loop is not spun.
androidTest: MessageDaoRetentionTest now pins the uid > 0 SQL guard against real SQLite (placeholder 0/-1 rows are ignored; a folder holding only placeholders probes as null), and drops the removed oldestSyncedTimestamp probe.
The MailBackfillerTest DAO fake mirrors the fixed lowestSyncedUid semantics (commented as such); the SQL itself is pinned by the androidTest.
Two code-review-derived correctness bugs in the full-history backfill (from the PR #46 review), fixed together because both govern the same decision: where `MailBackfiller` stops and resumes paging a folder.
## #94 — age floor stopped early on out-of-order Date/UID
**Root cause.** Backfill pages a folder newest-to-oldest **by UID** (arrival order), but the age retention floor was decided from `MIN(timestampMillis)` of the *cached* rows — the **Date header**. One message with a high UID but an old Date (mail moved/imported into the folder) dragged that minimum below the cutoff, and `reachedRetentionFloor` marked the folder complete while lower-UID messages with within-retention Dates were still unfetched: a silent, permanent history gap (completion is deliberately sticky).
**Fix.** The age floor is now decided from **the page actually fetched**, not from cached aggregates: paging ends only when a fetched page is *entirely* older than the cutoff (or the folder is exhausted). A single inverted message can no longer end paging; only a whole old page — strong evidence the descent has left the retention window — can. Such a terminal page is pure prune-fodder, so it is not persisted (no insert-then-prune churn with `MailPruner`). The trade is deliberate and documented: a pathologically interleaved mailbox may over-fetch (the pruner reclaims the excess) but backfill never silently gaps. The count floor keeps its cheap pre-fetch cache check — it orders by UID, exactly like paging, so inversions can't bite it. `MessageDao.oldestSyncedTimestamp` lost its last caller and is removed.
## #95 — `uid <= 0` row poisoned `MessageDao.lowestSyncedUid`
**Root cause.** `lowestSyncedUid` was a bare `MIN(uid)`. Rows with `uid <= 0` exist by design: MIGRATION_12_13 backfills a non-numeric id tail to `0`, and a fetch where `UIDFolder.getUID` returns `-1` maps to `-1`. One such row collapsed the resume boundary to `<= 0`, `ImapClient.fetchOlderThan` treats `beforeUid <= 1` as "nothing older", and the folder was **falsely marked fully backfilled** after fetching nothing — sticky completion again made the under-fetch permanent.
**Fix.** Positive-UID guards at every boundary derivation, mirroring the `minWindowUid` guard PR #46 already added to `MailSyncer`:
- `lowestSyncedUid` now excludes `uid <= 0` at the SQL level (`AND uid > 0`; null → backfill starts from the newest message and heals the placeholder via `persistBatch`'s header refresh);
- a stale persisted `nextBeforeUid <= 0` is discarded on resume rather than trusted;
- the per-page descent takes `min` over resolved (positive) UIDs only. If a whole page comes back unresolved, the folder **stalls**: it stays incomplete (a future scheduled run retries) but reports no immediate more-work — otherwise `BackfillWorker`'s slice-chaining loop (`while (runBackfill()) …`) would spin on the same page.
**Why no data fix / migration:** a migration cannot restore real UIDs (they live on the server). The existing design already heals placeholders — `updateHeaderContent` refreshes `uid` on the next sync/backfill pass — so the schema is untouched (no version bump, no `app/schemas` change); the DAO guard just stops placeholders from steering paging in the meantime.
## How the two fixes interact
Both bugs were "backfill quietly stops too early" with sticky completion sealing the gap; the fixes compose in `backfillFolder`'s single loop:
- #95 guarantees paging always *descends with a real, positive UID boundary* (resume + per-page), so #94's page-based floor always evaluates real pages.
- #94 makes the *stop decision* independent of cached aggregates, which also kills the compound case: a migrated `uid = 0` row with an old Date could previously end backfill through **either** path (boundary collapse or premature age floor). Now it can do neither.
- Completion stays sticky and is only ever declared on positive evidence (count floor, folder exhaustion, or an entirely-old page), so the #12/#13 backfill/pruner non-interference (shared `MailMaintenanceGate`, disjoint working sets, no re-open after prune) is preserved — the existing regression tests for it still pass unchanged.
## Tests
JVM unit tests (GreenMail real in-process IMAP + the existing in-memory DAO-fake harness in `MailBackfillerTest`); all four fail against the pre-fix code (verified by reverting the fix locally):
- **#94 repro:** a high-UID/old-Date message inside the seeded window with `retentionMonths = 6` — asserts every within-retention message gets cached (no gap) and paging still terminates.
- **#94 floor semantics:** an entirely-old page ends paging without persisting prune-fodder and without paging the whole folder.
- **#95 repro (migration variant):** a cached `uid = 0` row (as MIGRATION_12_13 leaves for a non-numeric id tail) — asserts the full history is still paged and each message is fetched exactly once.
- **#95 repro (server variant):** a page of `getUID = -1` messages — asserts the folder is *not* falsely marked complete, the boundary never collapses to `<= 1`, and the worker loop is not spun.
androidTest: `MessageDaoRetentionTest` now pins the `uid > 0` SQL guard against real SQLite (placeholder `0`/`-1` rows are ignored; a folder holding only placeholders probes as null), and drops the removed `oldestSyncedTimestamp` probe.
The `MailBackfillerTest` DAO fake mirrors the fixed `lowestSyncedUid` semantics (commented as such); the SQL itself is pinned by the androidTest.
Closes #94
Closes #95
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Two code-review-derived correctness bugs in the full-history backfill (from the PR #46 review), fixed together because both govern the same decision: where
MailBackfillerstops and resumes paging a folder.#94 — age floor stopped early on out-of-order Date/UID
Root cause. Backfill pages a folder newest-to-oldest by UID (arrival order), but the age retention floor was decided from
MIN(timestampMillis)of the cached rows — the Date header. One message with a high UID but an old Date (mail moved/imported into the folder) dragged that minimum below the cutoff, andreachedRetentionFloormarked the folder complete while lower-UID messages with within-retention Dates were still unfetched: a silent, permanent history gap (completion is deliberately sticky).Fix. The age floor is now decided from the page actually fetched, not from cached aggregates: paging ends only when a fetched page is entirely older than the cutoff (or the folder is exhausted). A single inverted message can no longer end paging; only a whole old page — strong evidence the descent has left the retention window — can. Such a terminal page is pure prune-fodder, so it is not persisted (no insert-then-prune churn with
MailPruner). The trade is deliberate and documented: a pathologically interleaved mailbox may over-fetch (the pruner reclaims the excess) but backfill never silently gaps. The count floor keeps its cheap pre-fetch cache check — it orders by UID, exactly like paging, so inversions can't bite it.MessageDao.oldestSyncedTimestamplost its last caller and is removed.#95 —
uid <= 0row poisonedMessageDao.lowestSyncedUidRoot cause.
lowestSyncedUidwas a bareMIN(uid). Rows withuid <= 0exist by design: MIGRATION_12_13 backfills a non-numeric id tail to0, and a fetch whereUIDFolder.getUIDreturns-1maps to-1. One such row collapsed the resume boundary to<= 0,ImapClient.fetchOlderThantreatsbeforeUid <= 1as "nothing older", and the folder was falsely marked fully backfilled after fetching nothing — sticky completion again made the under-fetch permanent.Fix. Positive-UID guards at every boundary derivation, mirroring the
minWindowUidguard PR #46 already added toMailSyncer:lowestSyncedUidnow excludesuid <= 0at the SQL level (AND uid > 0; null → backfill starts from the newest message and heals the placeholder viapersistBatch's header refresh);nextBeforeUid <= 0is discarded on resume rather than trusted;minover resolved (positive) UIDs only. If a whole page comes back unresolved, the folder stalls: it stays incomplete (a future scheduled run retries) but reports no immediate more-work — otherwiseBackfillWorker's slice-chaining loop (while (runBackfill()) …) would spin on the same page.Why no data fix / migration: a migration cannot restore real UIDs (they live on the server). The existing design already heals placeholders —
updateHeaderContentrefreshesuidon the next sync/backfill pass — so the schema is untouched (no version bump, noapp/schemaschange); the DAO guard just stops placeholders from steering paging in the meantime.How the two fixes interact
Both bugs were "backfill quietly stops too early" with sticky completion sealing the gap; the fixes compose in
backfillFolder's single loop:uid = 0row with an old Date could previously end backfill through either path (boundary collapse or premature age floor). Now it can do neither.MailMaintenanceGate, disjoint working sets, no re-open after prune) is preserved — the existing regression tests for it still pass unchanged.Tests
JVM unit tests (GreenMail real in-process IMAP + the existing in-memory DAO-fake harness in
MailBackfillerTest); all four fail against the pre-fix code (verified by reverting the fix locally):retentionMonths = 6— asserts every within-retention message gets cached (no gap) and paging still terminates.uid = 0row (as MIGRATION_12_13 leaves for a non-numeric id tail) — asserts the full history is still paged and each message is fetched exactly once.getUID = -1messages — asserts the folder is not falsely marked complete, the boundary never collapses to<= 1, and the worker loop is not spun.androidTest:
MessageDaoRetentionTestnow pins theuid > 0SQL guard against real SQLite (placeholder0/-1rows are ignored; a folder holding only placeholders probes as null), and drops the removedoldestSyncedTimestampprobe.The
MailBackfillerTestDAO fake mirrors the fixedlowestSyncedUidsemantics (commented as such); the SQL itself is pinned by the androidTest.Closes #94
Closes #95
🤖 Generated with Claude Code