From 8632500cf6a1432daf42bbe66b87318227b29c3b Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 7 Jul 2026 23:42:58 -0500 Subject: [PATCH] perf(data): route MailBackfiller.persistBatch through batched updateHeaderContents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit persistBatch refreshed each pre-existing backfilled header with a per-row updateHeaderContent in its own implicit transaction — the same N-commits-per-page anti-pattern #310 fixed in MailSyncer. Route the whole pre-existing subset through the batched MessageDao.updateHeaderContents(List) @Transaction so a page costs one commit instead of one fsync per message (amplified on the encrypted cache). Semantics are unchanged: updateHeaderContents applies updateHeaderContent to each row in list order, so the same rows get the same values (and the same casefold columns); the isNotEmpty guard still skips an empty refresh batch; brand-new rows stay insert-only. No schema change. Adds a PII-free, counts-only AppLog breadcrumb at the persist point. Tests: - MailBackfillerTest: the refresh routes through the batched update and never the per-row one; a partial page refreshes only its pre-existing subset in one batched call; an all-new page skips the batch entirely (empty boundary); breadcrumb counts. - MessageDaoTest (real Room): the batch writes byte-for-byte the same row as the per-row path; an empty batch is a no-op. Closes #322 --- .../libremail/data/local/MessageDaoTest.kt | 56 ++++++++++++ .../org/libremail/data/sync/MailBackfiller.kt | 18 ++-- .../libremail/data/sync/MailBackfillerTest.kt | 85 ++++++++++++++++++- 3 files changed, 146 insertions(+), 13 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt index fdb9956..35d0169 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt @@ -320,6 +320,62 @@ class MessageDaoTest { assertEquals("cached-2", two.body) } + /** + * The batched [MessageDao.updateHeaderContents] must write byte-for-byte the same row as the per-row + * [MessageDao.updateHeaderContent] it replaces on the backfill path (issue #322): the same refreshed + * fields and casefold columns, and the same untouched flags/body/membership. Two rows seeded + * identically and refreshed by the two paths with the same values must end up identical. + */ + @Test + fun updateHeaderContentsWritesTheSameResultAsThePerRowUpdate() = runBlocking { + dao.insertNew( + listOf( + message("perRow", isStarred = true, body = "cached"), + message("batch", isStarred = true, body = "cached"), + ), + ) + + // Old path: the single-row update. New path: the batch, carrying identical field values. + dao.updateHeaderContent( + id = "perRow", + sender = "Refreshed", + senderEmail = "refreshed@example.org", + subject = "Fresh", + timestampMillis = 9_000L, + uid = 7L, + ) + dao.updateHeaderContents( + listOf( + message( + "batch", + sender = "Refreshed", + senderEmail = "refreshed@example.org", + subject = "Fresh", + timestampMillis = 9_000L, + uid = 7L, + ), + ), + ) + + // Every stored column — refreshed and preserved alike — matches between the two paths. + val perRow = requireNotNull(dao.getById("perRow")) + val batch = requireNotNull(dao.getById("batch")) + assertEquals(perRow.copy(id = "id"), batch.copy(id = "id")) + } + + /** An empty batch is a no-op — issue #322's empty-batch boundary at the DAO transaction level. */ + @Test + fun updateHeaderContentsWithAnEmptyBatchWritesNothing() = runBlocking { + dao.insertNew(listOf(message("acct:1", subject = "Original", isRead = true, uid = 5))) + + dao.updateHeaderContents(emptyList()) + + val row = requireNotNull(dao.getById("acct:1")) + assertEquals("Original", row.subject) + assertEquals(5L, row.uid) + assertTrue(row.isRead) + } + @Test fun markSyncedPromotesSearchOnlyRowsIntoTheFolder() = runBlocking { dao.insertNew( 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 d0f12b2..48b2a90 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt @@ -198,17 +198,15 @@ class MailBackfiller @Inject constructor( 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, - ) - } + // Refresh every pre-existing row's header in ONE transaction (issue #322) rather than a per-row + // UPDATE each in its own implicit transaction — the same batched path foreground sync uses + // (issue #310). N per-row commits fsync the journal once per message (amplified on the + // encrypted cache); routing the whole batch through updateHeaderContents collapses them into a + // single commit per page. Semantically identical: it applies updateHeaderContent to each row in + // list order, so the same rows get the same values (and the same casefold columns). + messageDao.updateHeaderContents(toRefresh) } + AppLog.d(TAG, "backfill persist: fetched=${entities.size} refreshed=${toRefresh.size}") } private suspend fun markComplete(accountId: String, folder: String, nextBeforeUid: Long) { 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 0c1ef44..670080d 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt @@ -378,10 +378,12 @@ class MailBackfillerTest { /** * A backfilled page whose ids already exist (e.g. former search-only rows) must be *refreshed* * (markSynced + header update), not just IGNORE-inserted — this covers persistBatch's - * pre-existing-row branch, which the all-brand-new happy paths above never hit. + * pre-existing-row branch, which the all-brand-new happy paths above never hit. The refresh is + * routed through the BATCHED [MessageDao.updateHeaderContents] (issue #322), never the per-row + * [MessageDao.updateHeaderContent]. */ @Test - fun `re-inserting a pre-existing header refreshes it rather than only inserting`() = runTest { + fun `re-inserting a pre-existing header refreshes it through the batched update`() = runTest { appendMessages(60) seedForegroundWindow() val backfiller = backfiller(AccountSettings("acct")) @@ -391,11 +393,88 @@ class MailBackfillerTest { backfiller.runBackfill() coVerify(atLeast = 1) { lastMessageDao!!.markSynced(any()) } - coVerify(atLeast = 1) { + // The whole batch is refreshed in one transaction — the per-row path is never taken (issue #322). + coVerify(atLeast = 1) { lastMessageDao!!.updateHeaderContents(any()) } + coVerify(exactly = 0) { lastMessageDao!!.updateHeaderContent(any(), any(), any(), any(), any(), any()) } } + // --- issue #322: batched persist ------------------------------------------------------------ + + /** + * persistBatch routes its pre-existing-row refreshes through the BATCHED + * [MessageDao.updateHeaderContents] — one transaction per page (issue #322) — and refreshes ONLY + * the rows that already existed: brand-new rows are written whole by insertNew, so re-updating them + * would be redundant. A partial page (some ids already present, some brand-new) exercises exactly + * that split, in one batched call, never the per-row [MessageDao.updateHeaderContent]. + */ + @Test + fun `a partial page refreshes only its pre-existing rows through one batched update`() = runTest { + // One cached window row makes INBOX a backfill target with boundary UID 20. + cached += fetchedMessage(uid = "20").toEntity("acct", "INBOX") + // A single older page (UIDs 10..19); the next fetch (boundary 10) returns empty → folder complete. + val page = (10..19).map { fetchedMessage(uid = it.toString()) } + val imapClient = mockk() + coEvery { imapClient.fetchOlderThan(any(), any(), any(), any()) } answers { + if (thirdArg() > 10L) page else emptyList() + } + val backfiller = backfiller(AccountSettings("acct"), imapClient = imapClient) + // Only UIDs 10..13 are reported as already present — the partial pre-existing subset. + val preexistingIds = (10..13).map { "acct:INBOX:$it" }.toSet() + coEvery { lastMessageDao!!.existingIds(any()) } answers { + firstArg>().filter { it in preexistingIds } + } + val refreshedBatches = mutableListOf>() + coEvery { lastMessageDao!!.updateHeaderContents(any()) } answers { + refreshedBatches += firstArg>() + } + + backfiller.runBackfill() + + // Exactly one batched call for the page, carrying exactly the pre-existing subset (not the 6 new rows). + assertEquals(1, refreshedBatches.size, "one batched update per page") + assertEquals(preexistingIds, refreshedBatches.single().mapTo(HashSet()) { it.id }) + // markSynced promotes exactly that subset; the per-row update path is never taken (issue #322). + coVerify(exactly = 1) { lastMessageDao!!.markSynced(match { it.toSet() == preexistingIds }) } + coVerify(exactly = 0) { + lastMessageDao!!.updateHeaderContent(any(), any(), any(), any(), any(), any()) + } + // Counts-only persist breadcrumb (PII-free): 10 fetched, 4 refreshed. + assertTrue( + logBuffer.snapshot().any { it.message == "backfill persist: fetched=10 refreshed=4" }, + "the persist breadcrumb logs page + refresh counts only", + ) + } + + /** + * The empty-refresh boundary: a page whose rows are ALL brand-new must skip the header-refresh + * transaction entirely — insertNew writes them whole, so neither markSynced nor the batched + * updateHeaderContents runs (issue #322 preserves persistBatch's isNotEmpty guard). + */ + @Test + fun `an all-new page skips the batched header update entirely`() = runTest { + cached += fetchedMessage(uid = "20").toEntity("acct", "INBOX") + val page = (10..19).map { fetchedMessage(uid = it.toString()) } + val imapClient = mockk() + coEvery { imapClient.fetchOlderThan(any(), any(), any(), any()) } answers { + if (thirdArg() > 10L) page else emptyList() + } + // existingIds stays at the relaxed default (empty) → every fetched row is brand-new. + backfiller(AccountSettings("acct"), imapClient = imapClient).runBackfill() + + coVerify(exactly = 0) { lastMessageDao!!.updateHeaderContents(any()) } + coVerify(exactly = 0) { lastMessageDao!!.markSynced(any()) } + coVerify(exactly = 0) { + lastMessageDao!!.updateHeaderContent(any(), any(), any(), any(), any(), any()) + } + // The persist breadcrumb still records the page, with zero refreshed. + assertTrue( + logBuffer.snapshot().any { it.message == "backfill persist: fetched=10 refreshed=0" }, + "an all-new page logs refreshed=0", + ) + } + // --- issue #329: AppLog breadcrumbs --------------------------------------------------------- @Test