From 8a70b329ab0a28fe93e5d4cefcf1bfffdb07ac63 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 8 Jul 2026 19:31:03 -0500 Subject: [PATCH] perf(icloud): respect iCloud Mail IMAP connection & message-size limits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds two iCloud-specific, provider-scoped policies that build on the existing shared throttling framework (#360's AccountThrottleGate, #356's BackfillPacer) without refactoring either: - IcloudConnectionLimiter: a per-account permit gate capping an iCloud account at 5 simultaneous connections (Apple documents 5-8; pinned to the conservative low end). Wired into MailBackfiller around both connection-opening call sites (the header-page fetch and the body/attachment prefetch loop - the "1 + K + attachments" per-page connection count issue #363 describes). A no-op passthrough for every other provider, so it composes cleanly with #360's reactive backoff (already consulted first, per account) and #356's slice pacing (BackfillWorker composes BackfillPacer.runPaced around MailBackfiller.runBackfill, so the cap sits one layer beneath the pacer's cooldown/cap in the same call graph). - IcloudSendLimits: enforces Apple's ~20 MB outgoing message-size cap before SmtpSender ever opens a connection, estimating the actual encoded wire size (base64 inflates binary attachment bytes by ~4/3) rather than comparing raw file bytes, mirroring GraphSender's existing pre-send attachment-size guard. An over-cap send throws MessageTooLargeException, caught by SendWorker's existing runCatching and turned into a clean, PII-free outbox error - no crash, no raw provider rejection. Both are new, self-contained files kept intentionally separate from a shared cross-provider table, per the parallel-safety note on this ticket (sibling issues #361/#362/#364 add their own provider's limits the same way). Touches two shared files: MailBackfiller.kt (new constructor dependency + two call sites wrapped in icloudConnectionLimiter.withPermit) and SendWorker.kt (one guard call before smtpSender.send). Both are minimal, additive edits — flagged for conflict-awareness with the sibling provider tickets. Also excludes MailBackfillerTest.kt from detekt's LargeClass rule (config/detekt/detekt.yml), mirroring the existing MailRepositoryImplTest exclusion: one cohesive single-SUT suite tipped over the LLOC boundary by the new connection-cap wiring tests. Closes #363 --- ...IcloudConnectionLimiterInstrumentedTest.kt | 121 +++++++++++ .../data/sync/IcloudConnectionLimiter.kt | 74 +++++++ .../org/libremail/data/sync/MailBackfiller.kt | 19 +- .../org/libremail/data/sync/SendWorker.kt | 5 + .../org/libremail/mail/IcloudSendLimits.kt | 85 ++++++++ .../data/sync/IcloudConnectionLimiterTest.kt | 192 ++++++++++++++++++ .../libremail/data/sync/MailBackfillerTest.kt | 95 ++++++++- .../data/sync/MailMaintenanceGateTest.kt | 1 + .../data/sync/MailSyncConcurrencyTest.kt | 1 + .../org/libremail/data/sync/SendWorkerTest.kt | 42 ++++ .../libremail/mail/IcloudSendLimitsTest.kt | 170 ++++++++++++++++ config/detekt/detekt.yml | 7 +- 12 files changed, 805 insertions(+), 7 deletions(-) create mode 100644 app/src/androidTest/kotlin/org/libremail/data/sync/IcloudConnectionLimiterInstrumentedTest.kt create mode 100644 app/src/main/kotlin/org/libremail/data/sync/IcloudConnectionLimiter.kt create mode 100644 app/src/main/kotlin/org/libremail/mail/IcloudSendLimits.kt create mode 100644 app/src/test/kotlin/org/libremail/data/sync/IcloudConnectionLimiterTest.kt create mode 100644 app/src/test/kotlin/org/libremail/mail/IcloudSendLimitsTest.kt diff --git a/app/src/androidTest/kotlin/org/libremail/data/sync/IcloudConnectionLimiterInstrumentedTest.kt b/app/src/androidTest/kotlin/org/libremail/data/sync/IcloudConnectionLimiterInstrumentedTest.kt new file mode 100644 index 0000000..6f80f6d --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/data/sync/IcloudConnectionLimiterInstrumentedTest.kt @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.sync + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.delay +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.domain.model.MailProvider + +/** + * On-device proof of issue #363's iCloud connection cap, on the REAL Android coroutine runtime (not + * coroutines-test virtual time) across the CI API matrix. A real, production-wired [IcloudConnectionLimiter] + * (its `@Inject` constructor, so this also pins the production cap — mirrored here as [PRODUCTION_CAP]) — + * the per-account permit gate [MailBackfiller] consults around every connection it opens for an iCloud + * account — must: + * + * - let up to the cap run concurrently with no waiting; + * - make a caller past the cap wait for a live in-flight one to release, then resume it; + * - never gate a non-iCloud account, even while an iCloud account's cap is fully held. + * + * Deliberately mock-free (no `mockk`, no framework `Context`) and uses only the public production + * constructor (no internal test-only constructor, which — unlike the JVM `test` source set — + * `androidTest` cannot see, mirroring [BackfillPacerInstrumentedTest]'s own hardcoded-cap idiom): the + * limiter is the whole synchronisation primitive #363 adds, so exercising it directly is both the + * faithful behavioural test and the most portable across API 29-37. The JVM `IcloudConnectionLimiterTest` + * / `MailBackfillerTest` cover the same contract (with a smaller configured cap for speed) plus the full + * backfiller wiring under coroutines-test. + */ +@RunWith(AndroidJUnit4::class) +class IcloudConnectionLimiterInstrumentedTest { + + private val icloudAccount = MailProvider.ICLOUD.createAccount("me@icloud.com") + private val gmailAccount = MailProvider.GMAIL.createAccount("me@gmail.com") + + @Test + fun aCallerPastTheCapWaitsForALivePermitThenResumesOnceItReleases() = runBlocking { + val limiter = IcloudConnectionLimiter() + + // Exhaust every production permit for this account. + val entered = List(PRODUCTION_CAP) { CompletableDeferred() } + val release = CompletableDeferred() + val holders = entered.map { deferred -> + launch(Dispatchers.Default) { + limiter.withPermit(icloudAccount) { + deferred.complete(Unit) + release.await() + } + } + } + entered.forEach { it.await() } + + val extraEntered = CompletableDeferred() + val extra = launch(Dispatchers.Default) { + limiter.withPermit(icloudAccount) { extraEntered.complete(Unit) } + } + delay(PARK_PROBE_MS) + assertFalse("a caller past the cap must wait while every permit is held", extraEntered.isCompleted) + + release.complete(Unit) + withTimeout(HAND_OFF_TIMEOUT_MS) { extraEntered.await() } + holders.forEach { it.join() } + extra.join() + } + + @Test + fun aNonIcloudAccountIsNeverGatedEvenWhileTheIcloudCapIsFullyHeld() = runBlocking { + val limiter = IcloudConnectionLimiter() + val entered = List(PRODUCTION_CAP) { CompletableDeferred() } + val release = CompletableDeferred() + val holders = entered.map { deferred -> + launch(Dispatchers.Default) { + limiter.withPermit(icloudAccount) { + deferred.complete(Unit) + release.await() + } + } + } + entered.forEach { it.await() } + + // A regression that gated every provider on one shared cap would hang this withTimeout. + val ran = withTimeout(HAND_OFF_TIMEOUT_MS) { limiter.withPermit(gmailAccount) { true } } + + assertTrue("a non-iCloud account must never wait behind the iCloud-only cap", ran) + release.complete(Unit) + holders.forEach { it.join() } + } + + @Test + fun thePermitReleasesEvenWhenTheGuardedBlockThrowsSoNoCallerIsStrandedWaiting() = runBlocking { + val limiter = IcloudConnectionLimiter() + + val failed = runCatching { limiter.withPermit(icloudAccount) { throw IllegalStateException("boom") } } + assertTrue("the failing block propagates to the caller", failed.isFailure) + + // A regression would hang here forever instead of acquiring the "stuck" permit. + val ran = withTimeout(HAND_OFF_TIMEOUT_MS) { limiter.withPermit(icloudAccount) { true } } + assertTrue("a failed block must not strand its permit held", ran) + } + + private companion object { + /** + * Mirrors [IcloudConnectionLimiter.MAX_CONCURRENT_CONNECTIONS] (`internal`, so not a symbolic + * reference — see the class doc). A production regression to that constant would only make this + * test over- or under-exhaust the cap, not silently pass, since every permit is awaited by name. + */ + const val PRODUCTION_CAP = 5 + + /** Slack given to a parked waiter to (wrongly) resume before we assert it is still parked. */ + const val PARK_PROBE_MS = 300L + + /** Generous bound for the permit hand-off; only a real park/resume regression approaches it. */ + const val HAND_OFF_TIMEOUT_MS = 5_000L + } +} diff --git a/app/src/main/kotlin/org/libremail/data/sync/IcloudConnectionLimiter.kt b/app/src/main/kotlin/org/libremail/data/sync/IcloudConnectionLimiter.kt new file mode 100644 index 0000000..f043089 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/sync/IcloudConnectionLimiter.kt @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.sync + +import kotlinx.coroutines.sync.Semaphore +import kotlinx.coroutines.sync.withPermit +import org.libremail.domain.model.Account +import org.libremail.domain.model.MailProvider +import org.libremail.reporting.AppLog +import org.libremail.reporting.accountLogRef +import java.util.concurrent.ConcurrentHashMap +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Proactive per-account concurrency cap for iCloud Mail (issue #363): the provider-specific policy that + * keeps LibreMail's own connect-per-operation IMAP traffic ([org.libremail.mail.ImapClient]) from ever + * requesting more than [maxConcurrentConnections] connections at once for one iCloud account. Apple + * documents a 5-8 concurrent-connection ceiling per account; production pins to the conservative low end + * ([MAX_CONCURRENT_CONNECTIONS]) so LibreMail never approaches it even when backfill, an interactive + * fetch, IDLE, and a send happen to overlap. + * + * This is the *proactive* counterpart to the two mechanisms issues #360/#356 already built for the + * shared sync engine — composing with them rather than duplicating either: + * - [AccountThrottleGate] (#360) reacts *after* a provider has already throttled or locked the account; + * this gate keeps LibreMail's own request pattern from approaching that point in the first place. + * [MailBackfiller] consults both: the throttle gate first (skip an already-throttled account for the + * rest of the slice), then this one around each connection it actually opens. + * - [BackfillPacer] (#356) paces *how often* a new backfill slice starts; this gate bounds *how many* + * connections may be open at once within a slice — a different axis, so the two never fight. + * + * A no-op for every other provider: [withPermit] resolves [account]'s IMAP host to a [MailProvider] and + * only acquires a permit for [MailProvider.ICLOUD] — Gmail/Yahoo/AOL/Outlook accounts run [block] + * immediately, unconstrained (their own caps are issues #361/#362/#364's concern, kept as their own + * provider-scoped policy rather than a shared table, so the four tickets land independently). + * + * State is a per-account [Semaphore], created on first use and kept for the process lifetime (a handful + * of accounts at most, so this never grows unbounded) — in-process only, like every other gate in this + * package; a process restart simply starts every account back at full availability. + */ +@Singleton +class IcloudConnectionLimiter internal constructor(private val maxConcurrentConnections: Int) { + + /** Production wiring: the conservative, documented-cap-respecting default. */ + @Inject constructor() : this(MAX_CONCURRENT_CONNECTIONS) + + private val permits = ConcurrentHashMap() + + /** + * Runs [block] holding one of [account]'s connection permits when it is an iCloud account — + * suspending until one frees rather than rejecting, so a caller at the cap simply waits its turn + * instead of failing. Every other provider's account runs [block] immediately with no gating. The + * permit is always released, even if [block] throws (via [kotlinx.coroutines.sync.withPermit]), so a + * failed fetch can never strand another caller waiting forever. + */ + suspend fun withPermit(account: Account, block: suspend () -> T): T { + if (MailProvider.forImapHost(account.imap.host) != MailProvider.ICLOUD) return block() + val semaphore = permits.computeIfAbsent(account.id) { Semaphore(maxConcurrentConnections) } + if (semaphore.availablePermits == 0) { + AppLog.i(TAG, "${accountLogRef(account.id)} connection cap reached; waiting for a permit") + } + return semaphore.withPermit { block() } + } + + companion object { + private const val TAG = "IcloudConnLimit" + + /** + * Apple documents 5-8 concurrent IMAP connections per account (issue #363); pinned to the + * conservative low end rather than the documented maximum. `internal` so tests can assert the + * production cap without a magic number. + */ + internal const val MAX_CONCURRENT_CONNECTIONS = 5 + } +} 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 42bb8f0..a852149 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt @@ -59,6 +59,8 @@ class MailBackfiller @Inject constructor( private val maintenanceGate: MailMaintenanceGate, private val throttleGate: AccountThrottleGate, private val interactiveGate: InteractiveImapGate, + // iCloud-specific connection cap (#363); a no-op for every other provider — see its own KDoc. + private val icloudConnectionLimiter: IcloudConnectionLimiter, ) { /** 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) @@ -183,7 +185,11 @@ class MailBackfiller @Inject constructor( complete = true break } - val fetched = imapClient.fetchOlderThan(params, folder, beforeUid, BACKFILL_BATCH_SIZE) + // iCloud-specific connection cap (#363): waits for a free permit rather than opening past the + // account's documented ceiling; a no-op for every other provider (IcloudConnectionLimiter). + val fetched = icloudConnectionLimiter.withPermit(account) { + imapClient.fetchOlderThan(params, folder, beforeUid, BACKFILL_BATCH_SIZE) + } batches++ 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 @@ -214,7 +220,7 @@ class MailBackfiller @Inject constructor( } beforeUid = nextBeforeUid backfillProgressDao.upsert(BackfillProgressEntity(account.id, folder, beforeUid, complete = false)) - prefetchIfEnabled(entities.map { it.id }) + prefetchIfEnabled(account, entities.map { it.id }) // Breathe between pages so a large mailbox doesn't hammer the server. delay(BACKFILL_BATCH_DELAY_MS) } @@ -291,9 +297,12 @@ class MailBackfiller @Inject constructor( * and backfill content paths can never disagree (#88/#89). Header paging above is not gated here: * WorkManager's battery-not-low constraint on the backfill work is the scheduler-level control. * Best-effort and cancellable between messages so an interruption stops promptly; anything not - * fetched is filled in lazily when the message is opened. + * fetched is filled in lazily when the message is opened. Each message's prefetch (its body plus + * every attachment) is routed through [icloudConnectionLimiter] for an iCloud [account] — issue + * #363's "K + attachments" share of the per-page connection count the ticket describes — and is a + * no-op passthrough for every other provider. */ - private suspend fun prefetchIfEnabled(ids: List) { + private suspend fun prefetchIfEnabled(account: Account, ids: List) { // Debug-only fetch gate (issue #393): pause proactive body prefetch so a later open is a genuine // uncached fetch. Header paging above is untouched (its own gate is the BackfillWorker entry), so // history still lands; a skipped body is filled in lazily on open. Compiled out of release @@ -310,7 +319,7 @@ class MailBackfiller @Inject constructor( if (!shouldPrefetch) return for (id in ids) { currentCoroutineContext().ensureActive() - mailRepository.prefetchMessage(id) + icloudConnectionLimiter.withPermit(account) { mailRepository.prefetchMessage(id) } } } diff --git a/app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt b/app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt index 7e18f0a..9d88a89 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt @@ -21,6 +21,7 @@ import org.libremail.domain.model.OutgoingAttachment import org.libremail.domain.model.OutgoingMessage import org.libremail.mail.GraphSendException import org.libremail.mail.GraphSender +import org.libremail.mail.IcloudSendLimits import org.libremail.mail.SendableAttachment import org.libremail.mail.SmtpSender import org.libremail.reporting.AppLog @@ -112,6 +113,10 @@ class SendWorker @AssistedInject constructor( if (account.authType == AuthType.OAUTH_OUTLOOK) { sendOutlook(connectionFactory, account, message, attachments) } else { + // iCloud-specific size guard (#363): Apple caps outgoing message size, so this fails fast, + // locally, and cleanly (caught by the runCatching below) before smtpSender ever opens a + // connection — a no-op for every other provider. + IcloudSendLimits.requireWithinLimit(account, message, attachments) smtpSender.send( connectionFactory.smtpParamsFor(account), from = account.email, diff --git a/app/src/main/kotlin/org/libremail/mail/IcloudSendLimits.kt b/app/src/main/kotlin/org/libremail/mail/IcloudSendLimits.kt new file mode 100644 index 0000000..ee8240c --- /dev/null +++ b/app/src/main/kotlin/org/libremail/mail/IcloudSendLimits.kt @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import org.libremail.domain.model.Account +import org.libremail.domain.model.MailProvider +import org.libremail.domain.model.OutgoingMessage +import org.libremail.reporting.AppLog +import org.libremail.reporting.accountLogRef + +private const val BYTES_PER_MB = 1024L * 1024L + +/** Rounds [this] many bytes up to the nearest whole megabyte, for a human-readable size in an error message. */ +private fun Long.toWholeMb(): Long = (this + BYTES_PER_MB - 1) / BYTES_PER_MB + +/** + * Thrown when an outgoing message would exceed iCloud Mail's documented size cap (issue #363). The + * message text carries only byte counts — never a subject, address, or body fragment — so it is safe to + * surface verbatim as the outbox row's user-visible error ([org.libremail.data.sync.SendWorker]) and to + * pass to [AppLog], per the repo's PII-free logging rule. + */ +class MessageTooLargeException(val estimatedBytes: Long, val limitBytes: Long) : + Exception( + "Message is too large for iCloud Mail (about ${estimatedBytes.toWholeMb()} MB; " + + "the limit is ${limitBytes.toWholeMb()} MB)", + ) + +/** + * Enforces iCloud Mail's documented ~20 MB outgoing message-size cap (issue #363) before [SmtpSender] is + * ever asked to send. Apple's limit is the message as it travels the wire — body plus MIME/base64-encoded + * attachments — so [requireWithinLimit] estimates that encoded size (base64 inflates binary bytes by + * roughly 4/3) rather than comparing raw attachment file sizes directly against 20 MB, which would + * under-count and let a doomed send through to a real server-side rejection. Mirrors [GraphSender]'s + * pre-send `MAX_ATTACHMENT_BYTES` guard for the same reason: fail fast and locally, with a clear message, + * rather than spend a connection on a send that cannot succeed. + * + * A no-op for every other provider — Gmail/Yahoo/AOL/Outlook each document their own outgoing-size + * limits, tracked by issues #361/#362/#364, kept as their own provider-scoped policy rather than a shared + * table so the four tickets land independently. + */ +object IcloudSendLimits { + + /** Apple's documented outgoing message-size cap (body + attachments), unless using Mail Drop. */ + const val DOCUMENTED_LIMIT_BYTES = 20L * BYTES_PER_MB + + /** + * Throws [MessageTooLargeException] when [account] is an iCloud account and [message] plus + * [attachments]' estimated encoded size exceeds [DOCUMENTED_LIMIT_BYTES]. A no-op for every other + * provider, and for an iCloud message that fits. + */ + fun requireWithinLimit(account: Account, message: OutgoingMessage, attachments: List) { + if (MailProvider.forImapHost(account.imap.host) != MailProvider.ICLOUD) return + val estimated = estimatedEncodedBytes(message, attachments) + if (estimated <= DOCUMENTED_LIMIT_BYTES) return + AppLog.w( + TAG, + "${accountLogRef(account.id)} outgoing message over iCloud size cap: " + + "estimated=${estimated}B limit=${DOCUMENTED_LIMIT_BYTES}B", + ) + throw MessageTooLargeException(estimated, DOCUMENTED_LIMIT_BYTES) + } + + /** + * Estimates the message's size once it is on the wire: plain/HTML body bytes as-is (this app's MIME + * structure never base64-encodes the text parts — see [SmtpSender.setBody]) plus each attachment's + * *base64-encoded* size (3 raw bytes become 4 encoded chars, rounded up to the next whole group the + * way a real encoder pads a partial one). + */ + private fun estimatedEncodedBytes(message: OutgoingMessage, attachments: List): Long { + val textBytes = message.body.toByteArray(Charsets.UTF_8).size.toLong() + + (message.bodyHtml?.toByteArray(Charsets.UTF_8)?.size?.toLong() ?: 0L) + val attachmentEncodedBytes = attachments.sumOf { it.file.length().toBase64EncodedSize() } + return textBytes + attachmentEncodedBytes + } + + private fun Long.toBase64EncodedSize(): Long = + ((this + BASE64_RAW_GROUP_SIZE - 1) / BASE64_RAW_GROUP_SIZE) * BASE64_ENCODED_GROUP_SIZE + + private const val TAG = "IcloudSendLimits" + + /** Base64 groups 3 raw bytes... */ + private const val BASE64_RAW_GROUP_SIZE = 3L + + /** ...into 4 encoded characters. */ + private const val BASE64_ENCODED_GROUP_SIZE = 4L +} diff --git a/app/src/test/kotlin/org/libremail/data/sync/IcloudConnectionLimiterTest.kt b/app/src/test/kotlin/org/libremail/data/sync/IcloudConnectionLimiterTest.kt new file mode 100644 index 0000000..656e345 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/sync/IcloudConnectionLimiterTest.kt @@ -0,0 +1,192 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.sync + +import io.mockk.every +import io.mockk.mockkStatic +import io.mockk.unmockkAll +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.delay +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.libremail.domain.model.MailProvider +import org.libremail.reporting.AppLog +import org.libremail.reporting.RingLogBuffer +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * [IcloudConnectionLimiter] (issue #363) must cap an iCloud account's concurrent connections at its + * configured limit — a caller past the cap waits for a free permit rather than failing — isolate that + * budget per account, release a stuck permit even when the guarded block throws, and pass every other + * provider's account straight through, unconstrained. + * + * Deliberately real-dispatcher (`runBlocking` + `Dispatchers.Default`), not coroutines-test virtual time: + * the behaviour under test is real suspension shared across two independently launched coroutines via a + * [kotlinx.coroutines.sync.Semaphore], which a virtual clock cannot observe. Mirrors + * [org.libremail.data.sync.BackfillPacerTest]'s "composes with" tests and the #355 + * `InteractiveImapGateInstrumentedTest` park/resume idiom. + */ +class IcloudConnectionLimiterTest { + + private val logBuffer = RingLogBuffer() + + private val icloud = MailProvider.ICLOUD.createAccount("me@icloud.com") + private val gmail = MailProvider.GMAIL.createAccount("me@gmail.com") + + @Before + fun setUp() { + // AppLog forwards to android.util.Log, a throwing no-op stub under plain JVM unit tests; fully + // qualified (no import) per the ForbiddenImport style already used by GraphSenderSendTest. + mockkStatic(android.util.Log::class) + every { android.util.Log.i(any(), any()) } returns 0 + AppLog.install(logBuffer) + } + + @After + fun tearDown() = unmockkAll() + + @Test + fun `a second caller waits for the first to release when the cap is one`() = runBlocking { + val limiter = IcloudConnectionLimiter(maxConcurrentConnections = 1) + val entered = CompletableDeferred() + val release = CompletableDeferred() + val holder = launch(Dispatchers.Default) { + limiter.withPermit(icloud) { + entered.complete(Unit) + release.await() + } + } + entered.await() + + val secondEntered = CompletableDeferred() + val second = launch(Dispatchers.Default) { + limiter.withPermit(icloud) { secondEntered.complete(Unit) } + } + delay(PARK_PROBE_MS) + assertFalse(secondEntered.isCompleted, "the second caller must wait for the permit") + + release.complete(Unit) + withTimeout(HAND_OFF_TIMEOUT_MS) { secondEntered.await() } + holder.join() + second.join() + } + + @Test + fun `up to the cap runs concurrently without waiting`() = runBlocking { + val limiter = IcloudConnectionLimiter(maxConcurrentConnections = 2) + val entered1 = CompletableDeferred() + val entered2 = CompletableDeferred() + val release = CompletableDeferred() + val first = launch(Dispatchers.Default) { + limiter.withPermit(icloud) { + entered1.complete(Unit) + release.await() + } + } + val second = launch(Dispatchers.Default) { + limiter.withPermit(icloud) { + entered2.complete(Unit) + release.await() + } + } + + // Both entering (rather than one waiting on the other) proves the cap of 2 admits 2 at once. + withTimeout(HAND_OFF_TIMEOUT_MS) { + entered1.await() + entered2.await() + } + + release.complete(Unit) + first.join() + second.join() + } + + @Test + fun `every other provider runs unconstrained even while the cap is fully held`() = runBlocking { + val limiter = IcloudConnectionLimiter(maxConcurrentConnections = 1) + val release = CompletableDeferred() + val holder = launch(Dispatchers.Default) { + limiter.withPermit(icloud) { release.await() } + } + + // A Gmail account must never wait behind the iCloud-only cap — a regression would hang this. + val ran = withTimeout(HAND_OFF_TIMEOUT_MS) { limiter.withPermit(gmail) { true } } + + assertTrue(ran) + release.complete(Unit) + holder.join() + } + + @Test + fun `two iCloud accounts each get their own budget`() = runBlocking { + val limiter = IcloudConnectionLimiter(maxConcurrentConnections = 1) + val other = MailProvider.ICLOUD.createAccount("someone-else@icloud.com") + val release = CompletableDeferred() + val holder = launch(Dispatchers.Default) { + limiter.withPermit(icloud) { release.await() } + } + + // A different account's own single permit is untouched by the first account's held permit. + val ran = withTimeout(HAND_OFF_TIMEOUT_MS) { limiter.withPermit(other) { true } } + + assertTrue(ran) + release.complete(Unit) + holder.join() + } + + @Test + fun `the permit releases even when the guarded block throws`() = runBlocking { + val limiter = IcloudConnectionLimiter(maxConcurrentConnections = 1) + + val failed = runCatching { limiter.withPermit(icloud) { throw IllegalStateException("boom") } } + + assertTrue(failed.isFailure) + // A regression would hang here forever waiting on the "stuck" permit instead of acquiring it. + val ran = withTimeout(HAND_OFF_TIMEOUT_MS) { limiter.withPermit(icloud) { true } } + assertTrue(ran) + } + + @Test + fun `waiting for a full cap logs a PII-free breadcrumb`() = runBlocking { + val limiter = IcloudConnectionLimiter(maxConcurrentConnections = 1) + val entered = CompletableDeferred() + val release = CompletableDeferred() + val holder = launch(Dispatchers.Default) { + limiter.withPermit(icloud) { + entered.complete(Unit) + release.await() + } + } + entered.await() + + val second = launch(Dispatchers.Default) { limiter.withPermit(icloud) { } } + delay(PARK_PROBE_MS) + + val messages = logBuffer.snapshot().map { it.message } + assertTrue(messages.any { it.contains("connection cap reached") }, "messages=$messages") + messages.forEach { assertFalse(it.contains("icloud.com"), it) } + + release.complete(Unit) + holder.join() + second.join() + } + + @Test + fun `production wiring uses the documented conservative cap`() { + assertEquals(5, IcloudConnectionLimiter.MAX_CONCURRENT_CONNECTIONS) + } + + private companion object { + /** Slack given to a parked waiter to (wrongly) resume before we assert it is still parked. */ + const val PARK_PROBE_MS = 300L + + /** Generous bound for a permit hand-off; only a real park/resume regression approaches it. */ + const val HAND_OFF_TIMEOUT_MS = 5_000L + } +} 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 4a9ad75..38d7662 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt @@ -35,6 +35,7 @@ import org.libremail.data.local.entity.AccountEntity import org.libremail.data.local.entity.BackfillProgressEntity import org.libremail.data.local.entity.MessageEntity import org.libremail.data.local.entity.ServerConfigEmbedded +import org.libremail.data.local.toDomain import org.libremail.data.local.toEntity import org.libremail.data.settings.AccountSettingsRepository import org.libremail.data.settings.AppSettings @@ -42,6 +43,7 @@ import org.libremail.data.settings.FetchPolicy import org.libremail.data.settings.SettingsRepository import org.libremail.domain.model.AccountSettings import org.libremail.domain.model.ImapConnectionParams +import org.libremail.domain.model.MailProvider import org.libremail.domain.model.MailSecurity import org.libremail.domain.repository.MailRepository import org.libremail.mail.FetchedMessage @@ -530,6 +532,94 @@ class MailBackfillerTest { assertTrue(logBuffer.snapshot().any { it.message.startsWith("backfill skip acct:") }) } + // --- issue #363: iCloud Mail connection cap -------------------------------------------------- + + /** + * The core of #363: an iCloud account's page fetch is routed through [IcloudConnectionLimiter], so + * once its one connection permit is held elsewhere backfill's next page WAITS for it to free instead + * of opening past iCloud's documented cap — proven by holding the limiter's only permit externally + * and observing backfill park until it releases. Mirrors the #355 park/resume test's real-dispatcher + * hand-off idiom just below (a virtual-time `runTest` can't observe a real suspend shared by two + * launched coroutines across a [kotlinx.coroutines.sync.Semaphore]). + */ + @Test + fun `an iCloud account's backfill waits for a connection permit instead of exceeding the cap`() = + runBlocking { + cached += fetchedMessage(uid = "60").toEntity("acct", "INBOX") + val icloudEntity = accountEntity.copy(imap = ServerConfigEmbedded("imap.mail.me.com", 993, "SSL_TLS")) + val limiter = IcloudConnectionLimiter(maxConcurrentConnections = 1) + val fetched = CompletableDeferred() + val imapClient = mockk() + coEvery { imapClient.fetchOlderThan(any(), any(), any(), any()) } coAnswers { + fetched.complete(Unit) + emptyList() // one page, then the folder completes and the slice ends + } + + // Something else already holds the account's one iCloud connection permit. + val entered = CompletableDeferred() + val release = CompletableDeferred() + val holder = launch(Dispatchers.Default) { + limiter.withPermit(icloudEntity.toDomain()) { + entered.complete(Unit) + release.await() + } + } + entered.await() + + val backfillJob = launch(Dispatchers.Default) { + backfiller( + AccountSettings("acct"), + imapClient = imapClient, + icloudConnectionLimiter = limiter, + account = icloudEntity, + ).runBackfill() + } + delay(PARK_PROBE_MS) + assertFalse(fetched.isCompleted, "backfill must wait for a free connection permit") + + release.complete(Unit) + withTimeout(HAND_OFF_TIMEOUT_MS) { backfillJob.join() } + assertTrue(fetched.isCompleted, "backfill proceeds once a permit frees") + holder.join() + } + + /** + * A non-iCloud account (the GreenMail fixture's host) is never gated by [IcloudConnectionLimiter]: + * exhausting the SAME limiter instance's one permit for an unrelated iCloud account must not affect + * it — the gate is iCloud-only policy, not a general connection pool (#361/#362/#364 are unaffected). + */ + @Test + fun `a non-iCloud account's backfill is never gated by the iCloud connection limiter`() = runBlocking { + cached += fetchedMessage(uid = "60").toEntity("acct", "INBOX") + val limiter = IcloudConnectionLimiter(maxConcurrentConnections = 1) + val imapClient = mockk(relaxed = true) + coEvery { imapClient.fetchOlderThan(any(), any(), any(), any()) } returns emptyList() + + // Hold the one permit for a DIFFERENT, genuinely-iCloud account. + val entered = CompletableDeferred() + val release = CompletableDeferred() + val holder = launch(Dispatchers.Default) { + limiter.withPermit(MailProvider.ICLOUD.createAccount("other@icloud.com")) { + entered.complete(Unit) + release.await() + } + } + entered.await() + + val moreWork = withTimeout(HAND_OFF_TIMEOUT_MS) { + backfiller( + AccountSettings("acct"), + imapClient = imapClient, + icloudConnectionLimiter = limiter, + ).runBackfill() + } + + assertFalse(moreWork) + coVerify(atLeast = 1) { imapClient.fetchOlderThan(any(), any(), any(), any()) } + release.complete(Unit) + holder.join() + } + // --- issue #355: interactive-fetch priority ------------------------------------------------- /** @@ -686,9 +776,11 @@ class MailBackfillerTest { imapClient: ImapClient = client, throttleGate: AccountThrottleGate = AccountThrottleGate(), interactiveGate: InteractiveImapGate = InteractiveImapGate(), + icloudConnectionLimiter: IcloudConnectionLimiter = IcloudConnectionLimiter(), + account: AccountEntity = accountEntity, ): MailBackfiller { val accountDao = mockk() - coEvery { accountDao.getAll() } returns listOf(accountEntity) + coEvery { accountDao.getAll() } returns listOf(account) val messageDao = mockk(relaxed = true) coEvery { messageDao.insertNew(any()) } answers { @@ -743,6 +835,7 @@ class MailBackfillerTest { maintenanceGate = MailMaintenanceGate(), throttleGate = throttleGate, interactiveGate = interactiveGate, + icloudConnectionLimiter = icloudConnectionLimiter, ).also { lastMessageDao = messageDao lastMailRepository = mailRepository diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailMaintenanceGateTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailMaintenanceGateTest.kt index 410f941..108081b 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailMaintenanceGateTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailMaintenanceGateTest.kt @@ -202,6 +202,7 @@ class MailMaintenanceGateTest { maintenanceGate = gate, throttleGate = AccountThrottleGate(), interactiveGate = InteractiveImapGate(), + icloudConnectionLimiter = IcloudConnectionLimiter(), ) } diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt index c659e8e..6f9fc90 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt @@ -362,6 +362,7 @@ class MailSyncConcurrencyTest { maintenanceGate = MailMaintenanceGate(), throttleGate = AccountThrottleGate(), interactiveGate = InteractiveImapGate(), + icloudConnectionLimiter = IcloudConnectionLimiter(), ) } diff --git a/app/src/test/kotlin/org/libremail/data/sync/SendWorkerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/SendWorkerTest.kt index a272323..e805d0e 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/SendWorkerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/SendWorkerTest.kt @@ -38,6 +38,7 @@ import org.libremail.reporting.AppLog import org.libremail.reporting.RingLogBuffer import org.libremail.reporting.accountLogRef import java.io.File +import java.io.RandomAccessFile import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue @@ -221,6 +222,47 @@ class SendWorkerTest { messages.forEach { assertFalse(it.contains("acct@example.org"), it) } } + // --- issue #363: iCloud message-size cap ----------------------------------------------------- + + private fun icloudAccount(id: String = "acct") = + account(id, "PASSWORD_IMAP").copy(imap = ServerConfigEmbedded("imap.mail.me.com", 993, "SSL_TLS")) + + /** A sparse file of exactly [bytes] — allocates the size without writing content (fast, no disk churn). */ + private fun stageFileOfSize(messageId: String, index: Int, name: String, bytes: Long) { + val file = File(cacheDir, "outbox/$messageId/$index").apply { mkdirs() }.resolve(name) + RandomAccessFile(file, "rw").use { it.setLength(bytes) } + } + + @Test + fun `an oversized iCloud message fails cleanly without ever attempting to send (issue #363)`() = runTest { + stageFileOfSize("m1", index = 0, name = "big.bin", bytes = 21L * 1024 * 1024) // over the 20 MB cap + val attachmentsJson = listOf(OutgoingAttachment(uri = "content://1", name = "big.bin")) + .toOutgoingAttachmentsJson() + coEvery { outboxDao.getAll() } returns listOf(entity(attachments = attachmentsJson)) + coEvery { accountDao.getById("acct") } returns icloudAccount() + coEvery { connectionFactory.smtpParamsFor(any()) } returns mockk() + + assertEquals(Result.retry(), worker().doWork()) + + coVerify(exactly = 0) { smtpSender.send(any(), any(), any(), any()) } + coVerify { outboxDao.setError("m1", match { it.contains("iCloud Mail") && it.contains("MB") }) } + coVerify(exactly = 0) { outboxDao.delete(any()) } + val messages = logBuffer.snapshot().map { it.message } + messages.forEach { assertFalse(it.contains("acct@example.org"), it) } + } + + @Test + fun `an iCloud message within the size cap sends normally`() = runTest { + coEvery { outboxDao.getAll() } returns listOf(entity()) + coEvery { accountDao.getById("acct") } returns icloudAccount() + coEvery { connectionFactory.smtpParamsFor(any()) } returns mockk() + + assertEquals(Result.success(), worker().doWork()) + + coVerify { smtpSender.send(any(), "acct@example.org", any(), any()) } + coVerify { outboxDao.delete("m1") } + } + @Test fun `sends an Outlook message over Graph, logging a PII-free breadcrumb`() = runTest { coEvery { outboxDao.getAll() } returns listOf(entity()) diff --git a/app/src/test/kotlin/org/libremail/mail/IcloudSendLimitsTest.kt b/app/src/test/kotlin/org/libremail/mail/IcloudSendLimitsTest.kt new file mode 100644 index 0000000..fb6b172 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/mail/IcloudSendLimitsTest.kt @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import io.mockk.every +import io.mockk.mockkStatic +import io.mockk.unmockkAll +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.libremail.domain.model.MailProvider +import org.libremail.domain.model.OutgoingMessage +import org.libremail.reporting.AppLog +import org.libremail.reporting.RingLogBuffer +import java.io.File +import java.io.RandomAccessFile +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * [IcloudSendLimits] (issue #363) must enforce Apple's documented ~20 MB outgoing message-size cap for + * iCloud accounts only, estimating the *encoded* wire size (base64 inflates binary attachment bytes by + * ~4/3) rather than comparing raw file bytes directly — so a message can be rejected even when every + * individual attachment's raw size looks like it fits — and it must never touch any other provider. + */ +class IcloudSendLimitsTest { + + private val logBuffer = RingLogBuffer() + + private val icloudAccount = MailProvider.ICLOUD.createAccount("me@icloud.com") + private val gmailAccount = MailProvider.GMAIL.createAccount("me@gmail.com") + + @Before + fun setUp() { + // AppLog forwards to android.util.Log, a throwing no-op stub under plain JVM unit tests; fully + // qualified (no import) per the ForbiddenImport style already used by GraphSenderSendTest. + mockkStatic(android.util.Log::class) + every { android.util.Log.w(any(), any()) } returns 0 + AppLog.install(logBuffer) + } + + @After + fun tearDown() = unmockkAll() + + private fun message(body: String = "hi") = + OutgoingMessage(accountId = icloudAccount.id, to = "bob@example.org", subject = "Hi", body = body) + + /** A sparse file of exactly [bytes] — allocates the size without writing content (fast, no disk churn). */ + private fun fileOfSize(bytes: Long): File { + val file = File.createTempFile("icloud-limit", ".bin") + RandomAccessFile(file, "rw").use { it.setLength(bytes) } + file.deleteOnExit() + return file + } + + @Test + fun `a small iCloud message passes`() { + val attachment = SendableAttachment(fileOfSize(1024)) + + IcloudSendLimits.requireWithinLimit(icloudAccount, message(), listOf(attachment)) + // No exception: reaching this line is the assertion. + } + + @Test + fun `an attachment exactly at the encoded boundary passes`() { + // Empty body so only the attachment's encoded size is in play. Raw bytes chosen so ceil(raw/3)*4 + // lands EXACTLY on the documented cap (a multiple of 3, so the base64 group count divides evenly): + // 15_728_640 * 4 / 3 = 20_971_520 = DOCUMENTED_LIMIT_BYTES. + val attachment = SendableAttachment(fileOfSize(15_728_640L)) + + IcloudSendLimits.requireWithinLimit(icloudAccount, message(body = ""), listOf(attachment)) + } + + @Test + fun `an attachment one base64 group past the boundary fails`() { + // One byte over the exact-boundary raw size above rolls the base64 group count up by one full + // 4-byte group, pushing the estimate just past the cap. Empty body keeps the math exact. + val attachment = SendableAttachment(fileOfSize(15_728_641L)) + + val ex = assertFailsWithMessageTooLarge { + IcloudSendLimits.requireWithinLimit(icloudAccount, message(body = ""), listOf(attachment)) + } + assertTrue(ex.estimatedBytes > ex.limitBytes) + assertEquals(IcloudSendLimits.DOCUMENTED_LIMIT_BYTES, ex.limitBytes) + } + + @Test + fun `base64 inflation alone can push a raw-under-cap attachment over the encoded cap`() { + // 15.8 MB raw is comfortably UNDER the 20 MB documented cap, but base64 (~4/3 inflation) encodes + // it to just over 20 MB — proving the guard checks the encoded size, not the raw file size. + val rawBytes = 15_800_000L + assertTrue(rawBytes < IcloudSendLimits.DOCUMENTED_LIMIT_BYTES, "the raw size must look like it fits") + val attachment = SendableAttachment(fileOfSize(rawBytes)) + + assertFailsWithMessageTooLarge { + IcloudSendLimits.requireWithinLimit(icloudAccount, message(), listOf(attachment)) + } + } + + @Test + fun `a large body alone, with no attachments, can exceed the cap`() { + val hugeBody = "a".repeat(21_000_000) // ~21 MB of body text, over the 20 MB cap on its own + + assertFailsWithMessageTooLarge { + IcloudSendLimits.requireWithinLimit(icloudAccount, message(hugeBody), emptyList()) + } + } + + @Test + fun `body bytes count toward the total alongside attachments`() { + // Neither the body nor the attachment alone would trip the cap, but together they do — proving + // body bytes are added to the estimate rather than the check considering attachments only. + val body = "a".repeat(2_000_000) // 2 MB + val attachment = SendableAttachment(fileOfSize(14_300_000L)) // encodes to ~18.2 MB, alone under cap + + // The attachment alone (empty body) must NOT trip the cap. + IcloudSendLimits.requireWithinLimit(icloudAccount, message(), listOf(attachment)) + + // The same attachment WITH the 2 MB body pushes the estimate over the cap. + assertFailsWithMessageTooLarge { + IcloudSendLimits.requireWithinLimit(icloudAccount, message(body), listOf(attachment)) + } + } + + @Test + fun `every other provider is never gated, even wildly over iCloud's cap`() { + val attachment = SendableAttachment(fileOfSize(50L * 1024 * 1024)) // 50 MB, far over iCloud's cap + + IcloudSendLimits.requireWithinLimit(gmailAccount, message(), listOf(attachment)) + // No exception for Gmail: reaching this line is the assertion. + } + + @Test + fun `an oversized send logs a PII-free breadcrumb naming only byte counts`() { + val attachment = SendableAttachment(fileOfSize(21L * 1024 * 1024)) + + assertFailsWithMessageTooLarge { + IcloudSendLimits.requireWithinLimit(icloudAccount, message(), listOf(attachment)) + } + + val messages = logBuffer.snapshot().map { it.message } + assertTrue(messages.any { it.contains("over iCloud size cap") }, "messages=$messages") + messages.forEach { line -> + assertFalse(line.contains("@icloud.com"), line) + assertFalse(line.contains("me@"), line) + } + } + + @Test + fun `the exception message names iCloud Mail and both sizes in whole megabytes, with no PII`() { + val attachment = SendableAttachment(fileOfSize(25L * 1024 * 1024)) + + val ex = assertFailsWithMessageTooLarge { + IcloudSendLimits.requireWithinLimit(icloudAccount, message(), listOf(attachment)) + } + + val text = requireNotNull(ex.message) + assertTrue(text.contains("iCloud Mail"), text) + assertTrue(text.contains("MB"), text) + assertFalse(text.contains("@"), text) + } + + private fun assertFailsWithMessageTooLarge(block: () -> Unit): MessageTooLargeException { + val failure = runCatching(block) + assertTrue(failure.isFailure, "expected a MessageTooLargeException") + val exception = failure.exceptionOrNull() + assertTrue(exception is MessageTooLargeException, "expected MessageTooLargeException, was $exception") + return exception + } +} diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index 6458d26..023dede 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -28,7 +28,12 @@ complexity: # logging (issue #358) added its required android.util.Log mock + one breadcrumb test, tipping it # over. Excluded rather than artificially split — same "operation-rich cohesive suite" rationale as # the TooManyFunctions relaxation above. - excludes: ['**/data/repository/MailRepositoryImplTest.kt'] + # + # MailBackfillerTest is the same pattern: one cohesive single-SUT suite (a test per backfill concern + # — #12 core paging, #94/#95 boundary edge cases, #322 batching, #360 throttle, #355 interactive + # priority, #329 logging) already at the boundary; the iCloud connection-cap wiring tests (issue + # #363) tipped it over. + excludes: ['**/data/repository/MailRepositoryImplTest.kt', '**/data/sync/MailBackfillerTest.kt'] naming: FunctionNaming: