From 89c9f688a16fdb49f135ab2607746d79afab061b Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 16:58:56 -0500 Subject: [PATCH] fix(reader): propagate SEEN flag off the message-open critical path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit openMessage() was running a live imapClient.setFlag(SEEN) IMAP round trip (connection + STORE) before returning whenever a message's body was already cached but unread — purely to mark it read on the server. That network call sat on the reader's critical path even though nothing needed for rendering (body/attachments) required it, making "open an already-downloaded message" feel slow (#148). The local isRead flag is now set immediately (optimistic, local-only) and openMessage returns without awaiting the SEEN push. The push itself runs on a new application-lifetime backgroundScope (same CoroutineScope(SupervisorJob() + Dispatchers.IO) pattern already used by LibreMailApplication.appScope and IdleService.scope), with a bounded retry (3 attempts, short backoff) since today's folder sync deliberately never overwrites local read/star flags with server state (see MessageDao.updateHeaderContent) and therefore would not otherwise re-drive a push that never reached the server. Closes #148 Co-Authored-By: Claude Opus 4.8 --- .../data/repository/MailRepositoryImpl.kt | 46 ++++++++++++- .../data/repository/MailRepositoryImplTest.kt | 66 +++++++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) 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 d785cff..35f5813 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt @@ -9,8 +9,13 @@ import androidx.paging.PagingData import androidx.paging.map import dagger.hilt.android.qualifiers.ApplicationContext import jakarta.mail.Flags +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.map +import kotlinx.coroutines.launch import org.libremail.data.ReplyBuilder import org.libremail.data.SignatureBlock import org.libremail.data.Snippet @@ -65,6 +70,12 @@ class MailRepositoryImpl @Inject constructor( private val signatureRepository: SignatureRepository, ) : MailRepository { + // Application-lifetime scope for fire-and-forget server pushes that must outlive the caller — e.g. + // openMessage() returning to the reader screen before the SEEN flag reaches the server (#148). Same + // pattern as LibreMailApplication.appScope / IdleService.scope: this class is @Singleton (bound to + // Hilt's SingletonComponent), so the scope's lifetime is the process's, not any one caller's coroutine. + private val backgroundScope = CoroutineScope(SupervisorJob() + Dispatchers.IO) + override fun observeFolderMessages(accountId: String, folder: String): Flow> = messageDao.observeFolderSummaries(accountId, folder).map { rows -> rows.map { it.toDomain() } } @@ -114,13 +125,40 @@ class MailRepositoryImpl @Inject constructor( attachmentDao.replaceForMessage(id, content.attachments.map { it.toEntity(id) }) messageDao.setRead(id, true) } else if (!entity.isRead) { - runCatching { imapClient.setFlag(params, entity.folder, uidOf(id), Flags.Flag.SEEN, true) } + // Optimistic, local-only: the reader can render as soon as this returns. The SEEN flag + // still needs to reach the server, but that IMAP round trip (connection + STORE) must not + // sit on this path (#148) — the body/attachments are already fully local, so nothing about + // rendering the screen needs it. Pushed on backgroundScope, which outlives this call. messageDao.setRead(id, true) + pushSeenFlagInBackground(params, entity.folder, id) } } messageDao.getById(id)?.toDomain() ?: error("Message not found") } + /** + * Best-effort, fire-and-forget propagation of the SEEN flag to the server, off the message-open + * critical path (#148). Retries a few times with a short backoff, then gives up silently: local state + * is already correct (the caller set it before launching this), so a permanent failure here just means + * the server's copy stays "unread" until something else touches the flag — e.g. the message is opened + * from another client, or a future sync gains upward read-state reconciliation. Today's folder sync + * does NOT do that: it deliberately leaves cached read/star flags alone when refreshing headers from + * the server (see `MessageDao.updateHeaderContent`), so it protects an optimistic local flag from + * being clobbered by stale server state, but it does not re-drive a push that never reached the server + * either. This retry is in-memory only and does not survive process death mid-backoff. + */ + private fun pushSeenFlagInBackground(params: ImapConnectionParams, folder: String, id: String) { + backgroundScope.launch { + var attempt = 0 + while (true) { + attempt++ + val result = runCatching { imapClient.setFlag(params, folder, uidOf(id), Flags.Flag.SEEN, true) } + if (result.isSuccess || attempt >= SEEN_FLAG_PUSH_MAX_ATTEMPTS) return@launch + delay(SEEN_FLAG_RETRY_BACKOFF_MS * attempt) + } + } + } + override fun observeAttachments(messageId: String): Flow> = attachmentDao.observeForMessage(messageId).map { rows -> rows.map { it.toDomain() } @@ -410,5 +448,11 @@ private const val SEARCH_LIMIT = 50 /** Rows per page for the unified inbox (issue #124) — a page is a few screenfuls of message rows. */ private const val MAILBOX_PAGE_SIZE = 40 +/** Attempts for the background best-effort SEEN-flag push before giving up silently (issue #148). */ +private const val SEEN_FLAG_PUSH_MAX_ATTEMPTS = 3 + +/** Base backoff between SEEN-flag push retries, scaled by attempt number (2s, then 4s). */ +private const val SEEN_FLAG_RETRY_BACKOFF_MS = 2_000L + /** Message id is ":"; the uid is the trailing segment. */ private fun uidOf(id: String): String = id.substringAfterLast(':') diff --git a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt index 7aaeb95..cbb561b 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt @@ -13,8 +13,12 @@ import io.mockk.every import io.mockk.just import io.mockk.mockk import io.mockk.slot +import jakarta.mail.Flags +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.withTimeout import org.junit.Test import org.libremail.data.local.dao.AccountDao import org.libremail.data.local.dao.AttachmentDao @@ -207,6 +211,62 @@ class MailRepositoryImplTest { assertEquals("Tom & Jerry say \"hi\"", snippet.captured) } + /** + * The #148 fix: when the body is already cached and the message is unread, openMessage() must not + * await the SEEN-flag IMAP round trip before returning. Uses `runBlocking` (not `runTest`) and a + * [CompletableDeferred] gate — same idiom as `MailMaintenanceGateTest` — because the behavior under + * test is genuine concurrency between the caller's coroutine and the repository's own background + * scope, not something a virtual-time test dispatcher can observe. + */ + @Test + fun `openMessage returns without awaiting the background SEEN-flag push`() { + // Block body (not `= runBlocking { ... }`): the last statement below returns Boolean + // (CompletableDeferred.complete), and JUnit4 requires @Test methods to return void/Unit. + runBlocking { + val id = "acct:INBOX:40" + coEvery { messageDao.getById(id) } returns + messageEntity(id, "INBOX", bodyFetched = true) // isRead = false + coEvery { accountDao.getById("acct") } returns accountEntity() + coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() + coEvery { messageDao.setRead(id, true) } just Runs + val flagPushStarted = CompletableDeferred() + val releaseFlagPush = CompletableDeferred() + coEvery { imapClient.setFlag(any(), "INBOX", "40", Flags.Flag.SEEN, true) } coAnswers { + flagPushStarted.complete(Unit) + releaseFlagPush.await() // stays "in flight" until this test explicitly releases it + } + + val result = repository.openMessage(id) + + // openMessage already completed successfully even though the mocked setFlag call above is + // still parked on releaseFlagPush — proof it is not on the awaited path. + assertTrue(result.isSuccess) + coVerify { messageDao.setRead(id, true) } // the optimistic local write happens synchronously + // The push must still actually happen, just off this path — bounded wait, not indefinite. + withTimeout(FLAG_PUSH_AWAIT_TIMEOUT_MS) { flagPushStarted.await() } + releaseFlagPush.complete(Unit) // let the background coroutine finish cleanly before the test ends + } + } + + /** Best-effort means retried, not "one attempt and silently give up" (#148 design point 2). */ + @Test + fun `a failed SEEN-flag push is retried in the background`() = runBlocking { + val id = "acct:INBOX:41" + coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX", bodyFetched = true) + coEvery { accountDao.getById("acct") } returns accountEntity() + coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() + coEvery { messageDao.setRead(id, true) } just Runs + coEvery { imapClient.setFlag(any(), "INBOX", "41", Flags.Flag.SEEN, true) } throws RuntimeException("boom") + + repository.openMessage(id) + + // The first attempt fails immediately; observing a second proves this is a retry loop rather than + // a single best-effort attempt that gives up silently after one failure. + coVerify(timeout = FLAG_PUSH_RETRY_VERIFY_TIMEOUT_MS, atLeast = 2) { + imapClient.setFlag(any(), "INBOX", "41", Flags.Flag.SEEN, true) + } + } + @Test fun `prefetchMessage leaves a plain-text body's literal angle brackets in the snippet`() = runTest { val cache = Files.createTempDirectory("attach").toFile() @@ -656,3 +716,9 @@ class MailRepositoryImplTest { LoadResult.Page(data = rows, prevKey = null, nextKey = null) } } + +/** Bounded real-time wait for the background SEEN-flag push to have started (see `pushSeenFlagInBackground`). */ +private const val FLAG_PUSH_AWAIT_TIMEOUT_MS = 2_000L + +/** Bounded real-time wait for a second (retried) SEEN-flag push attempt to land. */ +private const val FLAG_PUSH_RETRY_VERIFY_TIMEOUT_MS = 5_000L