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 cc0125f..e88017f 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt @@ -326,7 +326,7 @@ class MailRepositoryImpl @Inject constructor( val routings = messageDao.getRoutingByIds(ids) messageDao.deleteByIds(ids) // optimistic forEachAccountFolder(routings) { params, folder, group -> - group.forEach { imapClient.deleteMessage(params, folder, uidOf(it.id)) } + imapClient.deleteMessages(params, folder, group.map { uidOf(it.id) }) } } @@ -387,7 +387,7 @@ class MailRepositoryImpl @Inject constructor( when (val dest = destByAccount[group.first().accountId]) { null -> if (fallbackExpunge) { - group.forEach { imapClient.deleteMessage(params, folder, uidOf(it.id)) } + imapClient.deleteMessages(params, folder, group.map { uidOf(it.id) }) } else { error("No ${role.name.lowercase()} folder for this account") } diff --git a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt index 70a2721..43d11ea 100644 --- a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt +++ b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt @@ -348,22 +348,43 @@ class ImapClient(private val reuseConnections: Boolean) { } } - suspend fun deleteMessage(params: ImapConnectionParams, folder: String, uid: String) = withContext(Dispatchers.IO) { - withStore(params) { store -> - val mailbox = store.getFolder(folder) - mailbox.open(Folder.READ_WRITE) - try { - (mailbox as UIDFolder).getMessageByUID(uid.toLong())?.setFlag(Flags.Flag.DELETED, true) - mailbox.expunge() - } finally { - runCatching { mailbox.close(false) } + /** + * Deletes a single message by UID. Delegates to [deleteMessages] so even a lone delete uses the + * same **targeted** UID EXPUNGE — never the untargeted expunge that would also remove other + * `\Deleted`-flagged mail in the folder (issue #295). + */ + suspend fun deleteMessage(params: ImapConnectionParams, folder: String, uid: String) = + deleteMessages(params, folder, listOf(uid)) + + /** + * Permanently deletes the messages with [uids] from [folder] in a **single** IMAP session: opens + * the folder once, flags the matched messages `\Deleted`, then issues one targeted UID EXPUNGE via + * [expungeTargeted]. This is the batch, data-safe path the repository's multi-message delete/expunge + * routes through — one login + one expunge for the whole selection, instead of the N logins + N + * expunges a per-UID [deleteMessage] loop would pay, and it never touches unrelated `\Deleted` mail + * (issue #295). A no-op when [uids] is empty or none of them resolve to a message in [folder]. + */ + suspend fun deleteMessages(params: ImapConnectionParams, folder: String, uids: List) = + withContext(Dispatchers.IO) { + if (uids.isEmpty()) return@withContext + withStore(params) { store -> + val mailbox = store.getFolder(folder) + mailbox.open(Folder.READ_WRITE) + try { + val uidFolder = mailbox as UIDFolder + val messages = uids.mapNotNull { uidFolder.getMessageByUID(it.toLong()) }.toTypedArray() + if (messages.isEmpty()) return@withStore + mailbox.setFlags(messages, Flags(Flags.Flag.DELETED), true) + expungeTargeted(mailbox, messages) + } finally { + runCatching { mailbox.close(false) } + } } } - } /** * Moves messages from [sourceFolder] to [destFolder] by UID. Implemented as copy + \Deleted + - * expunge so it works on every IMAP server (no reliance on the RFC 6851 MOVE extension). + * targeted expunge so it works on every IMAP server (no reliance on the RFC 6851 MOVE extension). */ suspend fun moveMessages( params: ImapConnectionParams, @@ -381,13 +402,27 @@ class ImapClient(private val reuseConnections: Boolean) { if (messages.isEmpty()) return@withStore mailbox.copyMessages(messages, store.getFolder(destFolder)) mailbox.setFlags(messages, Flags(Flags.Flag.DELETED), true) - mailbox.expunge() + expungeTargeted(mailbox, messages) } finally { runCatching { mailbox.close(false) } } } } + /** + * Permanently removes exactly [messages] — which the caller has already flagged `\Deleted` — with a + * **targeted** UID EXPUNGE (RFC 4315, [IMAPFolder.expunge]). Deliberately never the untargeted + * [Folder.expunge], which expunges *every* `\Deleted`-flagged message in [mailbox] — including ones + * a second client, Gmail, or a partial earlier move left flagged — the data-loss bug in issue #295. + * Every folder this client opens is an [IMAPFolder], so the cast holds in production and under + * GreenMail. The server must advertise UIDPLUS (Gmail, Outlook, and GreenMail do); on one that does + * not, Angus raises "UID EXPUNGE not supported" here rather than silently falling back to the + * unrelated-mail-destroying untargeted expunge — a loud failure is the correct, safe outcome. + */ + private fun expungeTargeted(mailbox: Folder, messages: Array) { + (mailbox as IMAPFolder).expunge(messages) + } + /** * Fetches the fields needed to compose a reply/forward (recipients + body) without marking the * original \Seen (opens READ_ONLY), so building a reply doesn't change the message's read state. diff --git a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplCoverageTest.kt b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplCoverageTest.kt index 015b542..6f83549 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplCoverageTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplCoverageTest.kt @@ -576,7 +576,7 @@ class MailRepositoryImplCoverageTest { assertTrue(result.isSuccess) coVerify { messageDao.deleteByIds(listOf(id)) } // local removal still happens - coVerify(exactly = 0) { imapClient.deleteMessage(any(), any(), any()) } // nothing pushed server-side + coVerify(exactly = 0) { imapClient.deleteMessages(any(), any(), any()) } // nothing pushed server-side } // --- role-folder resolution: a same-role folder that is not selectable ---------------------- 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 c0fc895..1ef18a4 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt @@ -367,7 +367,8 @@ class MailRepositoryImplTest { val result = repository.trash(listOf(id)) assertTrue(result.isSuccess) - coVerify { imapClient.deleteMessage(any(), "INBOX", "7") } + // The trash fallback deletes the whole group in one batch call, not a per-UID loop (issue #295). + coVerify { imapClient.deleteMessages(any(), "INBOX", listOf("7")) } } @Test @@ -394,7 +395,8 @@ class MailRepositoryImplTest { repository.expunge(listOf(id)) - coVerify { imapClient.deleteMessage(any(), "Trash", "3") } + // Expunge routes the group through the batch, targeted UID EXPUNGE path (issue #295). + coVerify { imapClient.deleteMessages(any(), "Trash", listOf("3")) } } @Test diff --git a/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt index f8726cc..7fc9131 100644 --- a/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt @@ -267,6 +267,62 @@ class ImapClientTest { assertTrue(remaining.contains("Keep me"), "remaining=$remaining") } + @Test + fun `deleteMessages expunges only the given uids and spares other Deleted-flagged mail`() = runTest { + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Target", "delete this") + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Bystander", "flagged elsewhere") + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Untouched", "no flag at all") + greenMail.waitForIncomingEmail(3) + val bySubject = client.fetchRecent(params(), "INBOX", limit = 50).associateBy { it.subject } + + // Simulate a second client (or a partial earlier move): "Bystander" is flagged \Deleted but has + // NOT been expunged — setFlag closes the folder with expunge=false, so the flag persists. + client.setFlag(params(), "INBOX", bySubject.getValue("Bystander").uid, Flags.Flag.DELETED, value = true) + + // Delete only "Target". The old untargeted mailbox.expunge() would ALSO permanently drop the + // \Deleted "Bystander"; a targeted UID EXPUNGE removes just the target (issue #295). + client.deleteMessages(params(), "INBOX", listOf(bySubject.getValue("Target").uid)) + + val remaining = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject } + assertFalse(remaining.contains("Target"), "the targeted message must be expunged, remaining=$remaining") + assertTrue(remaining.contains("Bystander"), "an unrelated \\Deleted message must survive, remaining=$remaining") + assertTrue(remaining.contains("Untouched"), "an unflagged message must survive, remaining=$remaining") + } + + @Test + fun `deleteMessages removes every uid in the batch`() = runTest { + listOf("A", "B", "C", "D").forEach { + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", it, "Body $it") + } + greenMail.waitForIncomingEmail(4) + val bySubject = client.fetchRecent(params(), "INBOX", limit = 50).associateBy { it.subject } + + client.deleteMessages(params(), "INBOX", listOf(bySubject.getValue("A").uid, bySubject.getValue("C").uid)) + + val remaining = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject }.toSet() + assertEquals(setOf("B", "D"), remaining, "remaining=$remaining") + } + + @Test + fun `moveMessages expunges only the moved uids and spares other Deleted-flagged mail`() = runTest { + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Move me", "relocate this") + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Bystander", "flagged elsewhere") + greenMail.waitForIncomingEmail(2) + appendMessage("Archive", "carol@example.org", "Seed", "Creates the Archive folder") + val bySubject = client.fetchRecent(params(), "INBOX", limit = 50).associateBy { it.subject } + // "Bystander" is flagged \Deleted (by another client) but left in the source folder, un-expunged. + client.setFlag(params(), "INBOX", bySubject.getValue("Bystander").uid, Flags.Flag.DELETED, value = true) + + client.moveMessages(params(), "INBOX", listOf(bySubject.getValue("Move me").uid), "Archive") + + val inbox = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject } + assertFalse(inbox.contains("Move me"), "the moved message must leave the source, inbox=$inbox") + // The move's expunge must be targeted: an unrelated \Deleted message must NOT be dropped (#295). + assertTrue(inbox.contains("Bystander"), "an unrelated \\Deleted message must survive the move, inbox=$inbox") + val archive = client.fetchRecent(params(), "Archive", limit = 50).map { it.subject } + assertTrue(archive.contains("Move me"), "archive=$archive") + } + @Test fun `fetchRecent returns empty for an empty folder`() = runTest { createFolder("Empty") diff --git a/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt index 77a2cd1..e957a11 100644 --- a/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt @@ -112,6 +112,24 @@ class ImapFolderOpenLatencyTest { assertEquals(1, proxy.commandCount("LOGOUT"), "one LOGOUT — the connection is torn down, not kept alive") } + @Test + fun `a batch delete opens one connection and pays one LOGIN for the whole selection`() = runTest { + seedInbox(3) + val uids = client.fetchRecent(params(), "INBOX", limit = 50).map { it.uid } // the fetch: connection 1 + proxy.awaitClientStreamsSettled() + val connectionsBefore = proxy.connectionCount + val loginsBefore = proxy.authCommandCount() + + client.deleteMessages(params(), "INBOX", uids) + proxy.awaitClientStreamsSettled() + + // Deleting all THREE messages is a single CONNECT + LOGIN + SELECT + STORE + UID EXPUNGE + LOGOUT + // — one connection and one LOGIN for the whole batch, not the three connects / three LOGINs a + // per-UID deleteMessage() loop would pay (the #295 N-login inefficiency). + assertEquals(1, proxy.connectionCount - connectionsBefore, "batch delete must open exactly one connection") + assertEquals(1, proxy.authCommandCount() - loginsBefore, "batch delete must pay exactly one LOGIN") + } + @Test fun `opening a folder then reading a message uses two separate connections (compounding cost)`() = runTest { seedInbox(1)