From 52503c000c149b5e2a11ddd4f080570aed0f69d3 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sun, 5 Jul 2026 19:17:19 -0500 Subject: [PATCH 1/4] fix(push): stop IdleService dataSync FGS crash-loop on exhausted 24h cap Root cause: after #302's runtime-cap fallBackToPeriodicSync() stops the dataSync foreground service, IdleService was restarted (START_STICKY null-intent redelivery + explicit startForegroundService) and onStartCommand unconditionally called startForeground(DATA_SYNC) while the rolling-24h budget was still exhausted. The platform rejected the start with ForegroundServiceStartNotAllowedException; it was uncaught, the process crashed, and START_STICKY restarted straight back into the same rejection -- a crash loop until the 24h window freed budget (#354). Fix (IdleService.kt): - onStartCommand now returns START_NOT_STICKY. Push is app-managed (LibreMailApplication.ensurePushStarted deterministically restarts it), so the sticky null-intent auto-restart was redundant and fired exactly when a dataSync FGS start is illegal. - Guard the foreground start via a new JVM-testable IdleForegroundStarter seam: a ForegroundServiceStartNotAllowedException (caught via its IllegalStateException supertype, so no minSdk-29 class load) degrades like the cap handler -- schedulePeriodicSync(), keep the degraded POLLING notification, stopSelf() promptly (avoids the "did not call startForeground in time" ANR) -- instead of propagating. - Record the cap event (elapsedRealtime); while still inside the cap window, onStartCommand skips the now-guaranteed-illegal foreground start entirely. - onTimeout stop path kept fast so ForegroundServiceDidNotStopInTimeException stays mitigated. PII-free AppLog.w/i on the degrade paths. Tests: - Unit (IdleForegroundStarterTest): onStartCommand returns START_NOT_STICKY; a rejected start is caught and routed to degrade without propagating; the cap window skips the attempt; a non-ISE propagates. - Instrumented (IdleServiceForegroundStartInstrumentedTest): the degrade path on a real Context -- rejection caught, periodic-sync fallback scheduled, degraded "instant delivery paused" notification built, watching skipped. Co-Authored-By: Claude Opus 4.8 --- ...eServiceForegroundStartInstrumentedTest.kt | 87 ++++++++++++ .../libremail/push/IdleForegroundStarter.kt | 65 +++++++++ .../kotlin/org/libremail/push/IdleService.kt | 125 ++++++++++++++---- .../push/IdleForegroundStarterTest.kt | 104 +++++++++++++++ 4 files changed, 358 insertions(+), 23 deletions(-) create mode 100644 app/src/androidTest/kotlin/org/libremail/push/IdleServiceForegroundStartInstrumentedTest.kt create mode 100644 app/src/main/kotlin/org/libremail/push/IdleForegroundStarter.kt create mode 100644 app/src/test/kotlin/org/libremail/push/IdleForegroundStarterTest.kt diff --git a/app/src/androidTest/kotlin/org/libremail/push/IdleServiceForegroundStartInstrumentedTest.kt b/app/src/androidTest/kotlin/org/libremail/push/IdleServiceForegroundStartInstrumentedTest.kt new file mode 100644 index 0000000..447ce4f --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/push/IdleServiceForegroundStartInstrumentedTest.kt @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.push + +import android.app.Notification +import android.app.Service +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.R +import org.libremail.data.sync.PushMode + +/** + * On-device coverage of the #354 dataSync-FGS degrade path that [IdleService.onStartCommand] routes + * through [IdleForegroundStarter]. When a foreground start is rejected — the runtime-cap + * `ForegroundServiceStartNotAllowedException`, surfaced as its [IllegalStateException] supertype — the + * seam must catch it, skip IDLE watching, and degrade to periodic sync plus the degraded + * ("instant delivery paused") notification, never propagating. This drives the same decision seam the + * service uses and builds the real degraded notification with a real application `Context` (a + * `ContextWrapper`, never a mocked `Context`), mirroring `PushStatusNotificationInstrumentedTest`; it + * stands up no foreground service, Hilt graph, or network, so it is deterministic — and unlike a JVM + * unit test it exercises the real `Notification` build (the unit-test `android.jar`'s + * `NotificationCompat` is a no-op stub). + */ +@RunWith(AndroidJUnit4::class) +class IdleServiceForegroundStartInstrumentedTest { + + private val context = ApplicationProvider.getApplicationContext() + + @Test + fun rejectedForegroundStart_degradesToPeriodicSyncWithPausedNotification_andSkipsWatching() { + val rejection = IllegalStateException( + "Time limit already exhausted for foreground service type dataSync", + ) + var watchingStarted = false + var periodicSyncScheduled = false + var degradedNotification: Notification? = null + + val result = IdleForegroundStarter.startForegroundOrDegrade( + capActive = false, + enterForeground = { throw rejection }, + onStarted = { watchingStarted = true }, + onDegraded = { cause -> + assertSame("the runtime-cap rejection must reach the degrade path", rejection, cause) + // Mirror IdleService.degradeToPeriodicSync on a real Context: (re)assert periodic sync and + // build the degraded status notification the service would post. + periodicSyncScheduled = true + PushStatusNotification.ensureChannel(context) + degradedNotification = PushStatusNotification.build(context, PushMode.POLLING, timedOut = true) + }, + ) + + assertEquals(Service.START_NOT_STICKY, result) + assertFalse("a rejected dataSync FGS start must not begin IDLE watching", watchingStarted) + assertTrue("the degrade path must (re)assert the 15-minute periodic sync fallback", periodicSyncScheduled) + val notification = requireNotNull(degradedNotification) { "the degrade path must build a status notification" } + assertEquals( + "the degraded notification must show the instant-delivery-paused text", + context.getString(R.string.notif_push_status_text_timed_out), + notification.extras.getCharSequence(Notification.EXTRA_TEXT).toString(), + ) + } + + @Test + fun activeCapWindow_skipsForegroundStartAttempt_andStillDegrades() { + var enterForegroundAttempted = false + var watchingStarted = false + var degraded = false + + val result = IdleForegroundStarter.startForegroundOrDegrade( + capActive = true, + enterForeground = { enterForegroundAttempted = true }, + onStarted = { watchingStarted = true }, + onDegraded = { degraded = true }, + ) + + assertEquals(Service.START_NOT_STICKY, result) + assertFalse("must not attempt a dataSync FGS start while still inside the cap window", enterForegroundAttempted) + assertFalse(watchingStarted) + assertTrue("must fall back to periodic sync while capped", degraded) + } +} diff --git a/app/src/main/kotlin/org/libremail/push/IdleForegroundStarter.kt b/app/src/main/kotlin/org/libremail/push/IdleForegroundStarter.kt new file mode 100644 index 0000000..6dde100 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/push/IdleForegroundStarter.kt @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.push + +import android.app.Service + +/** + * The dataSync foreground-start decision for [IdleService.onStartCommand] (#354), pulled out of the + * Android [Service] so it is JVM-unit-testable without Robolectric (which this repo does not use). + * + * After the Android 14+ dataSync FGS runtime cap fires (#302) and [IdleService] self-stops, the + * service is (re)started — the app deterministically restarts it via + * `LibreMailApplication.ensurePushStarted()`, and previously the platform also auto-restarted it via + * `START_STICKY` with a null intent. Each restart re-entered `onStartCommand` and unconditionally + * called `startForeground(..., dataSync)` while the rolling-24h budget was still exhausted, so the + * platform rejected it with `ForegroundServiceStartNotAllowedException` (uncaught → crash → sticky + * restart → loop). This seam encodes the fix: skip the start outright while still inside the cap + * window, otherwise attempt it and route the rejection — a `ForegroundServiceStartNotAllowedException` + * (API 31+), caught here via its [IllegalStateException] supertype so no `minSdk`-29 class load is + * needed — into a clean degrade instead of letting it propagate. Any other throwable propagates. + */ +internal object IdleForegroundStarter { + + /** + * Enters dataSync foreground state, or degrades to the 15-minute periodic-sync fallback when that + * start is — or would be — illegal. + * + * @param capActive true while still within the runtime-cap window recorded at the last cap event; + * the start is then skipped without attempting it (and without a cause). + * @param enterForeground the real `ServiceCompat.startForeground(..., dataSync)` call; may throw + * `ForegroundServiceStartNotAllowedException` (an [IllegalStateException]) when the runtime cap is + * exhausted or the start raced into the background. + * @param onStarted run after a successful foreground start (proceed with IDLE watchers). + * @param onDegraded run when the start was skipped ([capActive]) or rejected; receives the + * rejection cause, or `null` for the cap-window skip. Must schedule periodic sync and stop the + * service (it was started via `startForegroundService`, so it must stop promptly to avoid the + * "did not call startForeground in time" ANR). + * @return the value `onStartCommand` should return — always [Service.START_NOT_STICKY]: push is + * app-managed, so the platform's sticky null-intent auto-restart is redundant and fires exactly + * in the states that cannot legally start a dataSync FGS. + */ + fun startForegroundOrDegrade( + capActive: Boolean, + enterForeground: () -> Unit, + onStarted: () -> Unit, + onDegraded: (cause: Throwable?) -> Unit, + ): Int { + when { + capActive -> onDegraded(null) + else -> { + val entered = try { + enterForeground() + true + } catch (rejected: IllegalStateException) { + // ForegroundServiceStartNotAllowedException (API 31+) extends IllegalStateException; + // catching the supertype funnels the runtime-cap/background rejection into the + // degrade path without a version gate, instead of crash-looping (#354). + onDegraded(rejected) + false + } + if (entered) onStarted() + } + } + return Service.START_NOT_STICKY + } +} diff --git a/app/src/main/kotlin/org/libremail/push/IdleService.kt b/app/src/main/kotlin/org/libremail/push/IdleService.kt index f41e1ef..2ce39d3 100644 --- a/app/src/main/kotlin/org/libremail/push/IdleService.kt +++ b/app/src/main/kotlin/org/libremail/push/IdleService.kt @@ -8,6 +8,7 @@ import android.content.Intent import android.content.pm.PackageManager import android.content.pm.ServiceInfo import android.os.IBinder +import android.os.SystemClock import androidx.core.app.NotificationManagerCompat import androidx.core.app.ServiceCompat import androidx.core.content.ContextCompat @@ -83,22 +84,54 @@ class IdleService : Service() { /** Active IDLE watcher per account id, so we can start/stop them as accounts change. */ private val watchers = mutableMapOf() - override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { - startAsForeground(shownMode) - if (!watching) { - watching = true - scope.launch { - // Can't open the encrypted DB without the user present. Defer (stop) and let the app - // restart push after the next unlock, rather than block the service and ANR. - if (cacheGuard.isCacheLocked()) { - AppLog.i(TAG, "encrypted cache locked; deferring IDLE push until the app is unlocked") - stopSelf() - return@launch - } - reconcileWatchers() + override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int = + // Always START_NOT_STICKY (never START_STICKY): push is app-managed — LibreMailApplication's + // settings/account collector and ensurePushStarted() deterministically (re)start the service + // whenever it should run — so the platform's sticky null-intent auto-restart is redundant AND, + // after the dataSync FGS runtime cap (#302), re-enters here exactly when a dataSync foreground + // start is illegal, which was the #354 crash loop. The start/degrade decision lives in the + // JVM-testable IdleForegroundStarter seam. + IdleForegroundStarter.startForegroundOrDegrade( + capActive = capWindowActive(), + enterForeground = { startAsForeground(shownMode) }, + onStarted = ::startWatchingIfNeeded, + onDegraded = ::degradeAfterBlockedForegroundStart, + ) + + /** After a successful foreground start, begin watching accounts for IDLE (once per service life). */ + private fun startWatchingIfNeeded() { + if (watching) return + watching = true + scope.launch { + // Can't open the encrypted DB without the user present. Defer (stop) and let the app + // restart push after the next unlock, rather than block the service and ANR. + if (cacheGuard.isCacheLocked()) { + AppLog.i(TAG, "encrypted cache locked; deferring IDLE push until the app is unlocked") + stopSelf() + return@launch } + reconcileWatchers() } - return START_STICKY + } + + /** + * A dataSync foreground start was skipped (still inside the runtime-cap window, [cause] null) or + * rejected by the platform ([cause] is the `ForegroundServiceStartNotAllowedException`). Either way + * degrade like the cap handler instead of crashing (#354): log PII-free and fall back to periodic + * sync. On an actual rejection, also (re)arm the cap window so the next restart skips the attempt. + */ + private fun degradeAfterBlockedForegroundStart(cause: Throwable?) { + if (cause == null) { + AppLog.i(TAG, "dataSync FGS cap still active; skipping foreground start, periodic sync covers mail") + } else { + AppLog.w( + TAG, + "dataSync FGS start rejected (runtime cap or background); staying on 15-minute periodic sync", + cause, + ) + markCapReached() + } + degradeToPeriodicSync() } /** @@ -158,18 +191,33 @@ class IdleService : Service() { } /** - * Clean shutdown for the dataSync FGS runtime-cap timeout (issue #302): re-assert the periodic - * sync fallback, swap the persistent notification to the degraded text and DETACH it so it stays - * posted after we leave foreground state, then stop the service. Stopping foreground state is not - * optional here — a `dataSync` service that is still foreground when its timeout elapses is the - * exact condition the platform force-stops (and throws) on, so we must not keep running as an FGS. - * [stopSelf] then tears down [scope] in [onDestroy], closing the IDLE connections; mail arrives via - * the 15-minute periodic sync until push is started again (next app foreground / cap reset). + * The dataSync FGS runtime-cap timeout (issue #302): record the cap event so the next (re)start + * skips its now-illegal foreground start (#354), then degrade to the periodic-sync fallback. Kept + * fast/synchronous so a `dataSync` service that is still foreground when its timeout elapses — the + * exact condition the platform force-stops (and throws `ForegroundServiceDidNotStopInTimeException`) + * on — leaves foreground state within the grace window. + */ + private fun fallBackToPeriodicSync() { + AppLog.i(TAG, "dataSync FGS runtime cap reached: pausing IMAP IDLE; mail arrives via 15-minute periodic sync") + markCapReached() + degradeToPeriodicSync() + } + + /** + * Shared degrade to the 15-minute periodic-sync fallback, used by the runtime-cap timeout + * ([fallBackToPeriodicSync]) and by [onStartCommand] when a dataSync foreground start is skipped or + * rejected (#354): re-assert the periodic sync, swap the persistent notification to the degraded + * text and DETACH it so it stays posted after we leave foreground state, then stop the service. + * Leaving foreground state is safe on the [onStartCommand] paths too (never-foregrounded there, so + * `stopForeground` is a no-op), and [stopSelf] must run promptly because that start arrived via + * `startForegroundService` — otherwise the platform raises the "did not call startForeground in + * time" ANR. [stopSelf] then tears down [scope] in [onDestroy], closing the IDLE connections; mail + * arrives via the 15-minute periodic sync until push is started again (next app foreground / cap + * reset). */ // Permission is checked via hasNotificationPermission() below; lint can't trace the indirect guard. @SuppressLint("MissingPermission") - private fun fallBackToPeriodicSync() { - AppLog.i(TAG, "dataSync FGS runtime cap reached: pausing IMAP IDLE; mail arrives via 15-minute periodic sync") + private fun degradeToPeriodicSync() { // Already scheduled at every app start (UPDATE, so a no-op here) — re-asserted so the fallback // provably exists now that push is paused, mirroring the low-battery path in onPushModeChanged. syncScheduler.schedulePeriodicSync() @@ -187,6 +235,25 @@ class IdleService : Service() { stopSelf() } + /** Records the wall-independent time of the last dataSync cap event, arming [capWindowActive]. */ + private fun markCapReached() { + capReachedElapsedMs = SystemClock.elapsedRealtime() + } + + /** + * True while still within [CAP_WINDOW_MS] of the last cap event ([markCapReached]) — a burst of + * restarts in that window is certainly still capped, so [onStartCommand] skips the foreground start + * (and its now-guaranteed rejection) entirely. The window is anchored to the last real cap event + * and never refreshed by the skip itself, so it expires and lets a later restart re-probe; that + * probe is safe because a still-capped rejection is caught. Uses [SystemClock.elapsedRealtime] (not + * wall-clock) so it is immune to clock changes, and the companion field survives service + * re-creation within the process (which is where the restart storm happens). + */ + private fun capWindowActive(): Boolean { + val reachedAt = capReachedElapsedMs + return reachedAt != 0L && SystemClock.elapsedRealtime() - reachedAt < CAP_WINDOW_MS + } + private fun hasNotificationPermission(): Boolean = ContextCompat.checkSelfPermission(this, Manifest.permission.POST_NOTIFICATIONS) == PackageManager.PERMISSION_GRANTED @@ -246,5 +313,17 @@ class IdleService : Service() { // Re-establish IDLE on this cadence — under RFC 2177's 29-minute ceiling and short enough // to beat typical NAT/firewall idle-socket timeouts. const val IDLE_RENEWAL_MS = 9 * 60_000L + + // How long after a dataSync cap event onStartCommand skips the (still-illegal) foreground start + // outright (#354). The true rolling-24h budget reset is unknowable client-side, so this is a + // restart-storm damper, not a precise predictor: it matches the periodic-sync interval — the + // fallback already covering mail — so at most one foreground-start probe happens per cycle, and + // re-probing after it is safe because a still-capped rejection is caught, not fatal. + const val CAP_WINDOW_MS = 15 * 60_000L + + // elapsedRealtime() of the last dataSync cap event; 0 = none this process. Companion-scoped so + // it survives IdleService re-creation within the process, where the restart storm happens. + @Volatile + private var capReachedElapsedMs = 0L } } diff --git a/app/src/test/kotlin/org/libremail/push/IdleForegroundStarterTest.kt b/app/src/test/kotlin/org/libremail/push/IdleForegroundStarterTest.kt new file mode 100644 index 0000000..cda84ed --- /dev/null +++ b/app/src/test/kotlin/org/libremail/push/IdleForegroundStarterTest.kt @@ -0,0 +1,104 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.push + +import android.app.Service +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * JVM coverage of [IdleForegroundStarter] — the dataSync foreground-start decision + * `IdleService.onStartCommand` delegates to (#354). Extracted out of the Android `Service` precisely + * so this branchy logic — skip while capped, catch the runtime-cap + * `ForegroundServiceStartNotAllowedException` (surfaced via its [IllegalStateException] supertype) and + * degrade, otherwise proceed — is unit-testable with no emulator and no Robolectric (this repo has + * neither). It reads only the `Service.START_NOT_STICKY` constant; no `Service` is instantiated. + */ +class IdleForegroundStarterTest { + + @Test + fun `a successful foreground start proceeds to onStarted and returns START_NOT_STICKY`() { + var started = false + var degradeCalled = false + + val result = IdleForegroundStarter.startForegroundOrDegrade( + capActive = false, + enterForeground = { /* startForeground succeeds */ }, + onStarted = { started = true }, + onDegraded = { degradeCalled = true }, + ) + + assertEquals(Service.START_NOT_STICKY, result) + assertTrue("onStarted must run after a successful foreground start", started) + assertFalse("degrade must not run when the start succeeds", degradeCalled) + } + + @Test + fun `a runtime-cap rejection is caught, routed to degrade with the cause, and never propagates`() { + // The exact platform rejection is a ForegroundServiceStartNotAllowedException (API 31+); here we + // throw its IllegalStateException supertype, which is what the seam catches (and what the stub + // android.jar lets us construct on the JVM). + val rejection = IllegalStateException("Time limit already exhausted for foreground service type dataSync") + var started = false + var degradedWith: Throwable? = null + + // No exception escapes — that is the whole point of the fix (was an uncaught crash → restart loop). + val result = IdleForegroundStarter.startForegroundOrDegrade( + capActive = false, + enterForeground = { throw rejection }, + onStarted = { started = true }, + onDegraded = { cause -> degradedWith = cause }, + ) + + assertEquals(Service.START_NOT_STICKY, result) + assertFalse("a rejected foreground start must not begin IDLE watching", started) + assertSame("the rejection cause must reach the degrade path", rejection, degradedWith) + } + + @Test + fun `an active cap window skips the foreground start entirely and degrades with no cause`() { + var enterForegroundAttempted = false + var started = false + var degradeCalled = false + var degradedWith: Throwable? = null + + val result = IdleForegroundStarter.startForegroundOrDegrade( + capActive = true, + enterForeground = { enterForegroundAttempted = true }, + onStarted = { started = true }, + onDegraded = { cause -> + degradeCalled = true + degradedWith = cause + }, + ) + + assertEquals(Service.START_NOT_STICKY, result) + assertFalse("must not attempt a dataSync FGS start while still inside the cap window", enterForegroundAttempted) + assertFalse(started) + assertTrue("must still degrade to periodic sync while capped", degradeCalled) + assertNull("the cap-window skip carries no throwable cause", degradedWith) + } + + @Test + fun `a non-IllegalStateException from the foreground start propagates unchanged`() { + // Only the runtime-cap/background ISE is a safe-degrade condition; anything else (e.g. a + // SecurityException) is a genuine bug we must not swallow. + val boom = SecurityException("not an FGS runtime-cap rejection") + var degradeCalled = false + + val thrown = runCatching { + IdleForegroundStarter.startForegroundOrDegrade( + capActive = false, + enterForeground = { throw boom }, + onStarted = {}, + onDegraded = { degradeCalled = true }, + ) + }.exceptionOrNull() + + assertSame("an unrelated failure must propagate, not degrade", boom, thrown) + assertFalse("degrade must not run for a non-ISE failure", degradeCalled) + } +} From cc067408b8cff97c99775367b49da3bedf8417f3 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sun, 5 Jul 2026 19:47:01 -0500 Subject: [PATCH 2/4] perf(mail): enable IMAP connection reuse by default with a hardened cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An on-device drilldown proved Gmail server-side throttles LibreMail's connect-per-operation IMAP: every op was a fresh CONNECT+TLS+LOGIN, and full-history backfill's body+attachment prefetch generated ~601 connections in ~22 min, tripping (and sustaining) Gmail's per-account rate/bandwidth clamp (body download collapsed to ~4 KB/s). The `live` gauge peaked at only 5 (Gmail allows ~15), so it is connection *volume*, not count. Outlook IMAP on the same device opened in 2-3 s. Reusing one warm socket per account (~601 -> ~1) removes the throttle's trigger. This wires the reuse path the #125 spike built and left OFF (issue #357 Part 2 — connection reuse only; prefetch is a separate PR). How it is enabled (with a safety switch): - New `BuildConfig.IMAP_CONNECTION_REUSE` (default true) drives the production `ImapClient` no-arg `@Inject` constructor. To disable if a server misbehaves, flip it to "false" in app/build.gradle.kts — a build-config change, no Kotlin edit. The internal `ImapClient(reuseConnections, reuseIdleTimeoutMillis)` constructor stays the test/harness seam. - Universal: applies to all providers (incl. Outlook). No per-provider caps or throttling here — that is a separate effort (#356/#360-#364). Hardening `ImapConnectionCache` for production (was a spike): - Transparent stale recovery: broadened drop detection to Angus's own `iap.ConnectionException` (and a MessagingException caused by one) — the real signal `folder.open()` throws on a server-dropped idle socket, which the IOException-only check missed, so the reconnect now actually fires. A dropped reused socket is rebuilt once and the op retried, so callers see no spurious error; a genuine app error (e.g. message-not-found) is never retried. - Idle eviction: `evictIdle()` closes a connection unused past the reuse idle timeout (default 5 min), swept every 2 min by `IdleService`; skips any in-use connection. - Teardown: `IdleService` also tears down reused connections on the low-battery push-teardown path (#88/#89/#90), mirroring the IDLE connection teardown. - Concurrency: one connection per account behind a per-account mutex; the eviction sweep takes the lock non-blockingly so it never stalls or interrupts an in-flight op. Coexists with IMAP IDLE (its own separate connection). - PII-free AppLog on the lifecycle (open / reuse-hit / reconnect-stale / evict / teardown) keyed by an opaque per-cache ordinal, plus the #358 ImapPerf breadcrumb (connect~=0ms on a reuse hit). Tests (all via the fast gate, no emulator): - ImapConnectionCacheTest: reuse, retry-once stale recovery, narrow drop detection, deterministic idle eviction (injected clock), teardown. - ImapFolderOpenLatencyTest (GreenMail + counting proxy): N ops share one connection/LOGIN; a force-dropped socket is transparently reconnected; an app error does not reconnect; idle eviction LOGS-OUT and the next op reconnects. - Correctness suites (ImapClientTest/ImapClientBackfillTest/MailBackfillerTest) pinned to reuse-off to keep their connect-per-op assertions unchanged. Fast gate green: assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin, lintDebug, ktlintCheck, detekt. Co-Authored-By: Claude Opus 4.8 --- app/build.gradle.kts | 6 + .../kotlin/org/libremail/mail/ImapClient.kt | 50 +++-- .../org/libremail/mail/ImapConnectionCache.kt | 204 ++++++++++++++---- .../kotlin/org/libremail/push/IdleService.kt | 26 +++ .../libremail/data/sync/MailBackfillerTest.kt | 4 +- .../org/libremail/mail/CountingImapProxy.kt | 15 ++ .../libremail/mail/ImapClientBackfillTest.kt | 4 +- .../org/libremail/mail/ImapClientTest.kt | 5 +- .../libremail/mail/ImapConnectionCacheTest.kt | 92 ++++++-- .../mail/ImapFolderOpenLatencyTest.kt | 95 ++++++-- docs/perf/issue-125-connection-reuse-spike.md | 11 + 11 files changed, 416 insertions(+), 96 deletions(-) diff --git a/app/build.gradle.kts b/app/build.gradle.kts index c63c990..ef0c692 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -65,6 +65,12 @@ android { buildConfigField("String", "OUTLOOK_OAUTH_CLIENT_ID", "\"$outlookOAuthClientId\"") buildConfigField("String", "OUTLOOK_OAUTH_REDIRECT_URI", "\"$outlookRedirectScheme://oauth2redirect\"") buildConfigField("String", "DEBUG_REPORT_ENDPOINT", "\"$debugReportEndpoint\"") + // IMAP connection reuse (issue #357 Part 2, wiring the #125 spike): keep one authenticated + // IMAP connection warm per account instead of paying a cold CONNECT+TLS+LOGIN on every + // operation — the fix for Gmail throttling LibreMail's connect-per-operation traffic. ON by + // default; this is the safety switch: flip to "false" here (a build-config change, no Kotlin + // edit) to fall back to connect-per-operation if a server misbehaves with a kept-alive socket. + buildConfigField("Boolean", "IMAP_CONNECTION_REUSE", "true") // AppAuth's bundled manifest requires this placeholder; it registers the redirect scheme on // RedirectUriReceiverActivity so the Outlook sign-in redirect returns to the app. manifestPlaceholders["appAuthRedirectScheme"] = outlookRedirectScheme diff --git a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt index c10948e..2ab1570 100644 --- a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt +++ b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt @@ -30,6 +30,7 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import org.eclipse.angus.mail.imap.IMAPFolder import org.eclipse.angus.mail.imap.IMAPMessage +import org.libremail.BuildConfig import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.model.MailSecurity import org.libremail.reporting.AppLog @@ -99,23 +100,29 @@ data class ReplyContext( /** Thin IMAP client over Jakarta/Angus Mail. Supports password and XOAUTH2 auth. */ @Singleton -class ImapClient(private val reuseConnections: Boolean) { +class ImapClient internal constructor( + private val reuseConnections: Boolean, + private val reuseIdleTimeoutMillis: Long = DEFAULT_REUSE_IDLE_TIMEOUT_MS, +) { /** - * Production entry point. Connection reuse is a SPIKE flag (issue #125), **OFF by default** so it - * cannot destabilize the connect-per-operation behaviour on `main`: with it off, [withStore] is - * byte-for-byte today's connect + LOGOUT-per-call. Once real-device validation (see - * `docs/perf/issue-125-connection-reuse-spike.md`) confirms the win, wire this to a setting or - * `BuildConfig`; today only the reuse harness flips it on via the primary constructor. + * Production entry point. Connection reuse (issue #357 Part 2, wiring the #125 spike) is **ON by + * default**, driven by [BuildConfig.IMAP_CONNECTION_REUSE]: instead of a cold `CONNECT + TLS + + * LOGIN` per operation, each account keeps one authenticated connection warm (see + * [ImapConnectionCache]), which is the fix for Gmail throttling LibreMail's connect-per-operation + * traffic (`docs/perf/issue-125-*`). The `BuildConfig` field is the safety switch: flipping it to + * `false` (a build-config change, no code edit) restores connect-per-operation if a server + * misbehaves with a kept-alive socket. The internal constructor is the test/harness seam. */ - @Inject constructor() : this(reuseConnections = false) + @Inject constructor() : this(reuseConnections = BuildConfig.IMAP_CONNECTION_REUSE) /** - * Per-account keep-alive cache; allocated only when the spike flag is on, so the default build - * carries neither the state nor the reuse code path. + * Per-account keep-alive cache; allocated only when reuse is enabled, so a reuse-disabled build + * carries neither the state nor the reuse code path (and [withStore] stays byte-for-byte the old + * connect + LOGOUT-per-call). */ private val connectionCache: ImapConnectionCache? = - if (reuseConnections) ImapConnectionCache(::openConnectedStore) else null + if (reuseConnections) ImapConnectionCache(::openConnectedStore, reuseIdleTimeoutMillis) else null /** Connects and returns the account's folders with their SPECIAL-USE attributes. Throws on failure. */ suspend fun listFolders(params: ImapConnectionParams): List = withContext(Dispatchers.IO) { @@ -632,7 +639,7 @@ class ImapClient(private val reuseConnections: Boolean) { private suspend fun withStore(params: ImapConnectionParams, op: String = "imap", block: (Store) -> T): T { val cache = connectionCache return if (cache != null) { - cache.withStore(params, block) + cache.withStore(params, op, block) } else { // Time CONNECT + TLS + LOGIN separately from the op's own work, and record how many // connect-per-op sockets are live at once, so a slow op can be attributed and the provider @@ -662,14 +669,24 @@ class ImapClient(private val reuseConnections: Boolean) { } /** - * SPIKE hook (issue #125): closes any kept-alive reused connections (`LOGOUT` + teardown), a no-op - * when the reuse flag is OFF. The reuse harness calls this to force settlement; a shipped feature - * would also drive it from an idle-eviction timer and the low-battery push teardown (#88/#89/#90). + * Tears down every kept-alive reused connection (`LOGOUT` + teardown); a no-op when reuse is + * disabled. `IdleService` drives this on the low-battery push-teardown path (#88/#89/#90), mirroring + * the IDLE connection teardown, and the reuse tests call it to force settlement. */ suspend fun closeReusedConnections() { connectionCache?.closeAll() } + /** + * Closes any reused connection that has sat unused past the reuse idle timeout (issue #357 Part 2); + * a no-op when reuse is disabled or nothing is idle. `IdleService` calls this on a periodic sweep so + * a socket kept warm for latency doesn't linger and drain battery; a connection currently in use is + * skipped. + */ + suspend fun evictIdleReusedConnections() { + connectionCache?.evictIdle() + } + private fun buildProps(protocol: String, params: ImapConnectionParams, reuse: Boolean = false): Properties = Properties().apply { put("mail.store.protocol", protocol) @@ -703,6 +720,11 @@ class ImapClient(private val reuseConnections: Boolean) { const val TAG = "LibreMailIdle" const val PERF_TAG = "ImapPerf" const val NANOS_PER_MS = 1_000_000L + + // A reused connection unused for this long is idle-evicted (issue #357 Part 2): long enough to + // stay warm across an active reading session, well under typical server idle timeouts (Gmail + // ~30 min) so eviction, not a server drop, is what usually closes it. + const val DEFAULT_REUSE_IDLE_TIMEOUT_MS = 5 * 60_000L } } diff --git a/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt b/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt index 899f3b2..40e1c8b 100644 --- a/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt +++ b/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt @@ -7,83 +7,172 @@ import jakarta.mail.Store import jakarta.mail.StoreClosedException import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock +import org.eclipse.angus.mail.iap.ConnectionException import org.libremail.domain.model.ImapConnectionParams +import org.libremail.reporting.AppLog import java.io.IOException import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.atomic.AtomicInteger /** - * SPIKE (issue #125): a per-account keep-alive cache of authenticated IMAP [Store]s, so folder-opens - * and message operations reuse one already-connected session instead of re-paying - * `CONNECT + TLS + LOGIN` on every call. See `docs/perf/issue-125-connection-reuse-spike.md`. + * A per-account keep-alive cache of authenticated IMAP [Store]s, so folder-opens and message + * operations reuse one already-connected session instead of re-paying `CONNECT + TLS + LOGIN` on + * every call. Wiring the reuse path proven by the #125 spike; the production default is ON (see + * `BuildConfig.IMAP_CONNECTION_REUSE`). This is the fix for the on-device finding that Gmail throttles + * LibreMail's connect-per-operation IMAP traffic — collapsing ~one socket per operation to ~one warm + * socket per account removes the throttle's trigger (issue #357 Part 2, `docs/perf/issue-125-*`). * - * Prototype stance — deliberately the simplest thing that *proves reuse*, leaving the tuning knobs to a - * measured follow-up: + * Design: * - **One connection per account, mutex-guarded.** Each account key holds a single [Store] behind its - * own [Mutex]; every operation on that account serializes through it. This is the simplest safe - * design and the one the investigation named as the starting point. Its known cost is head-of-line - * blocking — a quick flag toggle can queue behind a slow body download. A bounded pool would trade - * that for more sockets (and a size cap + eviction); not prototyped here. - * - **Lazy, catch-and-retry-once stale handling.** No periodic `NOOP` probe (that would add a - * round-trip to every reused op, partly defeating the point). An operation runs optimistically; if - * it fails with a dropped-connection signal, the socket is rebuilt once and the operation retried. + * own [Mutex]; every operation on that account serializes through it, so the single socket is only + * ever touched by one caller at a time (IMAP is serial per connection). Its known cost is + * head-of-line blocking — a quick flag toggle can queue behind a slow body download. A bounded pool + * would trade that for more sockets; that (and per-provider connection caps) is a separate effort + * (#356/#360-#364), deliberately NOT in scope here. + * - **Transparent stale-connection recovery.** No periodic `NOOP` probe (that would add a round-trip + * to every reused op). An operation runs optimistically; if it fails with a dropped-connection + * signal (server idle-timeout, NAT rebind, network change) the socket is rebuilt once and the + * operation retried, so the caller never sees a spurious error. A second failure clears the slot so + * the next call reconnects. A non-connection error (e.g. "message not found") is never retried, so a + * working socket is never needlessly torn down and a mutation is never re-issued over a live socket. + * - **Idle eviction.** [evictIdle] closes any connection unused for longer than [idleTimeoutMillis] + * (driven by a periodic sweep in `IdleService`), so a socket kept warm for latency doesn't linger + * and drain battery once the user goes idle. It skips any connection currently in use. + * - **Teardown.** [closeAll] evicts everything (`LOGOUT` + socket teardown); `IdleService` drives it + * on the low-battery push-teardown path (#88/#89/#90), mirroring the IDLE connection teardown. * - **Keyed by connection identity, not the secret.** The OAuth access token * ([ImapConnectionParams.secret]) rotates; keying on host/port/user/security/mechanism keeps a token * refresh from orphaning a live, already-authenticated socket. A refreshed secret only matters when * we actually reconnect, and [connect] is always handed the current [params]. * + * Coexists with IMAP IDLE: `ImapClient.idle` holds its own dedicated long-lived [Store] (not in this + * cache), so reuse adds at most one more persistent socket per account — well under provider limits + * (Gmail ~15). + * * Thread-safety: [ImapClient]'s UI operations are not otherwise serialized and prefetch runs outside - * the syncer's mutex, so [withStore] must be safe under concurrent callers for the same account — the - * per-key mutex provides that. Not wired to any lifecycle/battery signal yet: [closeAll] is the only - * eviction and is driven by the harness today; an idle-eviction timer and low-battery teardown - * (#88/#89/#90) are follow-ups. + * the syncer's mutex, so every entry point here is safe under concurrent callers for the same account — + * the per-key [Mutex] provides that, and [evictIdle] takes it non-blockingly so a sweep never stalls + * behind (or interrupts) an in-flight operation. * * @param connect builds and authenticates a fresh [Store] for the given params (blocking network I/O). + * @param idleTimeoutMillis how long a cached connection may sit unused before [evictIdle] closes it. + * @param nowNanos monotonic clock source (injected for deterministic idle-eviction tests). */ -internal class ImapConnectionCache(private val connect: (ImapConnectionParams) -> Store) { +internal class ImapConnectionCache( + private val connect: (ImapConnectionParams) -> Store, + private val idleTimeoutMillis: Long, + private val nowNanos: () -> Long = System::nanoTime, +) { - private class Entry { + /** + * One account's reused connection. [id] is an opaque per-cache ordinal used only for PII-free log + * correlation — it is NOT derived from the host/username/secret, so a log line can attribute an + * event to an account without ever naming it. + */ + private class Entry(val id: Int) { val mutex = Mutex() + + @Volatile var store: Store? = null + + @Volatile + var lastUsedAtNanos: Long = 0L } private val entries = ConcurrentHashMap() + private val nextId = AtomicInteger(0) /** - * Runs [block] against a reused, authenticated [Store] for [params]'s account: it is established on - * first use and kept open afterwards, so only the first call pays connection setup. Serialized per - * account by the key's [Mutex]. If the operation hits a dropped connection the socket is rebuilt - * once and the operation retried; a second failure clears the slot so the next call reconnects. + * Runs [block] against a reused, authenticated [Store] for [params]'s account: established on first + * use and kept open afterwards, so only the first call pays connection setup. Serialized per account + * by the key's [Mutex]. Transparently reconnects once if the cached socket has been dropped. [op] is + * a short, PII-free intent label (`body-fetch`, `backfill-page`, …) for the perf breadcrumb. */ - suspend fun withStore(params: ImapConnectionParams, block: (Store) -> T): T { - val entry = entries.computeIfAbsent(key(params)) { Entry() } - return entry.mutex.withLock { - val store = entry.store ?: connect(params).also { entry.store = it } + suspend fun withStore(params: ImapConnectionParams, op: String, block: (Store) -> T): T { + val entry = entries.computeIfAbsent(key(params)) { Entry(nextId.incrementAndGet()) } + return entry.mutex.withLock { runReusing(entry, params, op, block) } + } + + /** Establishes-or-reuses the account's [Store], runs [block], and reconnects once on a dropped socket. */ + private fun runReusing(entry: Entry, params: ImapConnectionParams, op: String, block: (Store) -> T): T { + val connectMs = ensureConnected(entry, params) // 0ms when the live connection is reused + entry.lastUsedAtNanos = nowNanos() + val workStart = nowNanos() + return try { + block(requireNotNull(entry.store)) + } catch (e: Throwable) { + if (!isConnectionDrop(e)) throw e + // Stale socket: rebuild once and retry so the caller never sees the drop. + AppLog.d(TAG, "reuse stale acct=${entry.id}; reconnecting", e) + reconnectAndRetry(entry, params, block) + } finally { + entry.lastUsedAtNanos = nowNanos() + AppLog.d(PERF_TAG, "$op connect=${connectMs}ms work=${elapsedMs(workStart)}ms live=${liveCount()}") + } + } + + /** Reuses the live [Store] (0ms) or connects a fresh one, returning the connect cost in ms. */ + private fun ensureConnected(entry: Entry, params: ImapConnectionParams): Long { + if (entry.store != null) { + AppLog.d(TAG, "reuse hit acct=${entry.id}") + return 0L + } + val start = nowNanos() + entry.store = connect(params) + val ms = elapsedMs(start) + AppLog.d(TAG, "reuse open acct=${entry.id} connect=${ms}ms live=${liveCount()}") + return ms + } + + /** Closes the dropped socket, reconnects once, and retries [block]; a second failure clears the slot. */ + private fun reconnectAndRetry(entry: Entry, params: ImapConnectionParams, block: (Store) -> T): T { + runCatching { entry.store?.close() } + entry.store = null + entry.store = connect(params) + entry.lastUsedAtNanos = nowNanos() + AppLog.d(TAG, "reuse reconnected acct=${entry.id} live=${liveCount()}") + return try { + block(requireNotNull(entry.store)) + } catch (retry: Throwable) { + runCatching { entry.store?.close() } + entry.store = null + AppLog.w(TAG, "reuse reconnect failed acct=${entry.id}", retry) + throw retry + } + } + + /** + * Closes and forgets every connection whose last use is older than [idleTimeoutMillis] (`LOGOUT` + + * teardown). Takes each account's lock non-blockingly, so a connection currently in use is left + * untouched and the sweep never stalls behind a slow operation. No-op when nothing is cached. + */ + suspend fun evictIdle() { + if (entries.isEmpty()) return + val cutoffNanos = idleTimeoutMillis * NANOS_PER_MS + for ((_, entry) in entries) { + if (!entry.mutex.tryLock()) continue // in use — skip, don't interrupt or wait try { - block(store) - } catch (e: Throwable) { - if (!isConnectionDrop(e)) throw e - // Stale socket (server idle-timeout, NAT rebind, network change): rebuild once and retry. - runCatching { store.close() } - // Forget the dead socket before reconnecting, so a failed connect leaves a clean slot. - entry.store = null - val fresh = connect(params) - entry.store = fresh - try { - block(fresh) - } catch (retry: Throwable) { - runCatching { fresh.close() } + val store = entry.store + if (store != null && nowNanos() - entry.lastUsedAtNanos >= cutoffNanos) { + runCatching { store.close() } entry.store = null - throw retry + AppLog.d(TAG, "reuse evict idle acct=${entry.id} live=${liveCount()}") } + } finally { + entry.mutex.unlock() } } } - /** Closes and forgets every cached connection (`LOGOUT` + socket teardown). The only eviction today. */ + /** Closes and forgets every cached connection (`LOGOUT` + socket teardown). No-op when empty. */ suspend fun closeAll() { + if (entries.isEmpty()) return for ((_, entry) in entries) { entry.mutex.withLock { - entry.store?.let { store -> runCatching { store.close() } } + entry.store?.let { store -> + runCatching { store.close() } + AppLog.d(TAG, "reuse teardown acct=${entry.id}") + } entry.store = null } } @@ -97,15 +186,36 @@ internal class ImapConnectionCache(private val connect: (ImapConnectionParams) - private fun key(params: ImapConnectionParams): String = "${params.host}|${params.port}|${params.security}|${params.username}|${params.useXoauth2}" + /** Count of currently-held reused sockets, for the PII-free log breadcrumb (approximate under races). */ + private fun liveCount(): Int = entries.values.count { it.store != null } + + private fun elapsedMs(startNanos: Long): Long = (nowNanos() - startNanos) / NANOS_PER_MS + /** * Whether [error] signals a dropped connection (retry on a fresh socket) rather than a genuine - * protocol/application error (propagate as-is). Deliberately narrow: a plain [MessagingException] - * for a real server error whose connection is still live is NOT retried, so we never re-issue a - * mutation over a working connection. + * protocol/application error (propagate as-is). A server idle-timeout / NAT rebind / network change + * surfaces as a [FolderClosedException], a [StoreClosedException], a raw [IOException] or Angus's own + * [ConnectionException] — or, most commonly for `folder.open()` on a server-dropped socket, a plain + * [MessagingException] *caused by* one of those ("Connection dropped by server?"). Deliberately + * still narrow: a [MessagingException] caused by anything else (a `CommandFailedException` / + * `BadCommandException` — a real server NO on a live connection) is NOT retried, so a working socket + * is never needlessly torn down. + * + * NB (residual, tracked as the deferred mutation-idempotency review — see the #125 spike doc): the + * retry re-runs the whole operation, so a *mutation* (flag/move/expunge) dropped mid-flight is + * at-least-once. Flag sets are idempotent; the dominant real case — a socket the server dropped + * while idle, detected on the next op's first command before any mutation is issued — is safe. A + * copy-then-expunge move interrupted between its two halves is the rare exception left to that review. */ private fun isConnectionDrop(error: Throwable): Boolean = when (error) { - is FolderClosedException, is StoreClosedException, is IOException -> true - is MessagingException -> error.cause is IOException + is FolderClosedException, is StoreClosedException, is IOException, is ConnectionException -> true + is MessagingException -> error.cause is IOException || error.cause is ConnectionException else -> false } + + private companion object { + const val TAG = "ImapReuse" + const val PERF_TAG = "ImapPerf" + const val NANOS_PER_MS = 1_000_000L + } } diff --git a/app/src/main/kotlin/org/libremail/push/IdleService.kt b/app/src/main/kotlin/org/libremail/push/IdleService.kt index f41e1ef..621eb48 100644 --- a/app/src/main/kotlin/org/libremail/push/IdleService.kt +++ b/app/src/main/kotlin/org/libremail/push/IdleService.kt @@ -97,10 +97,28 @@ class IdleService : Service() { } reconcileWatchers() } + // The reuse cache (issue #357 Part 2) keeps interactive/sync IMAP connections warm; sweep + // them so a socket that has gone idle past the reuse timeout is closed rather than left + // draining battery. Independent of push mode — it runs while the service lives. + scope.launch { evictIdleReuseConnectionsLoop() } } return START_STICKY } + /** + * Periodically evicts IMAP connections the reuse cache kept warm once they go idle past the reuse + * idle timeout (issue #357 Part 2). A no-op when reuse is disabled or nothing is idle; + * [ImapClient.evictIdleReusedConnections] skips any connection currently in use, so a sweep never + * disturbs an in-flight sync or interactive fetch. + */ + private suspend fun evictIdleReuseConnectionsLoop() { + while (scope.isActive) { + delay(REUSE_EVICTION_SWEEP_MS) + runCatching { imapClient.evictIdleReusedConnections() } + .onFailure { AppLog.w(TAG, "reuse idle-eviction sweep failed", it) } + } + } + /** * Android 14+'s runtime cap on a `dataSync` foreground service (~6h per rolling 24h window) fires * this callback and then force-stops the service — throwing a system FGS-timeout exception — if we @@ -152,6 +170,10 @@ class IdleService : Service() { // here is effectively a no-op — done anyway so the fallback provably exists whenever push is // paused, without disturbing the running period. syncScheduler.schedulePeriodicSync() + // Mirror the IDLE teardown for the reuse cache (issue #357 Part 2): drop any warm + // interactive/sync connections so we hold no kept-alive IMAP sockets while conserving + // battery. They re-establish on the next sync/interactive op once battery recovers. + scope.launch { imapClient.closeReusedConnections() } } else { AppLog.i(TAG, "Battery recovered: resuming IMAP IDLE push") } @@ -243,6 +265,10 @@ class IdleService : Service() { const val INITIAL_BACKOFF_MS = 5_000L const val MAX_BACKOFF_MS = 5 * 60_000L + // Cadence of the reuse-cache idle-eviction sweep (issue #357 Part 2). Tighter than the reuse + // idle timeout so an idle socket is closed shortly after it crosses it. + const val REUSE_EVICTION_SWEEP_MS = 2 * 60_000L + // Re-establish IDLE on this cadence — under RFC 2177's 29-minute ceiling and short enough // to beat typical NAT/firewall idle-socket timeouts. const val IDLE_RENEWAL_MS = 9 * 60_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 7c1af2d..695d9c5 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt @@ -60,7 +60,9 @@ import kotlin.test.assertTrue class MailBackfillerTest { private lateinit var greenMail: GreenMail - private val client = ImapClient() + + // Reuse off: this suite pins the connect-per-operation backfill behaviour it was written against. + private val client = ImapClient(reuseConnections = false) private val accountEntity = AccountEntity( id = "acct", diff --git a/app/src/test/kotlin/org/libremail/mail/CountingImapProxy.kt b/app/src/test/kotlin/org/libremail/mail/CountingImapProxy.kt index 145754e..9dc04ee 100644 --- a/app/src/test/kotlin/org/libremail/mail/CountingImapProxy.kt +++ b/app/src/test/kotlin/org/libremail/mail/CountingImapProxy.kt @@ -36,6 +36,9 @@ class CountingImapProxy(private val backendHost: String, private val backendPort /** Client → server pump threads, tracked so tests can wait for the parsed command stream to settle. */ private val clientPumps = Collections.synchronizedList(mutableListOf()) + /** Accepted client-side sockets, so a test can force-drop them to simulate a server/NAT disconnect. */ + private val acceptedSockets = Collections.synchronizedList(mutableListOf()) + @Volatile private var running = true /** The local port to point [ImapClient] at; it forwards to the backend. */ @@ -69,6 +72,17 @@ class CountingImapProxy(private val backendHost: String, private val backendPort } } + /** + * Force-closes every currently-accepted client socket, simulating a server idle-timeout / NAT + * rebind / network drop of the kept-alive reused connection. The client's next use of that socket + * then fails with an I/O error, which the connection-reuse cache should transparently reconnect + * from. [connectionCount] keeps counting, so a subsequent reconnect makes it rise. + */ + fun dropAcceptedConnections() { + val snapshot = synchronized(acceptedSockets) { acceptedSockets.toList().also { acceptedSockets.clear() } } + snapshot.forEach { runCatching { it.close() } } + } + override fun close() { running = false runCatching { server.close() } @@ -88,6 +102,7 @@ class CountingImapProxy(private val backendHost: String, private val backendPort runCatching { client.close() } continue } + acceptedSockets.add(client) val upstream = Thread({ pumpCountingCommands(client, backend) }, "imap-proxy-up").apply { isDaemon = true } val downstream = Thread({ pump(backend, client) }, "imap-proxy-down").apply { isDaemon = true } clientPumps.add(upstream) diff --git a/app/src/test/kotlin/org/libremail/mail/ImapClientBackfillTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapClientBackfillTest.kt index f73d5cb..5019b23 100644 --- a/app/src/test/kotlin/org/libremail/mail/ImapClientBackfillTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/ImapClientBackfillTest.kt @@ -30,7 +30,9 @@ import kotlin.test.assertTrue class ImapClientBackfillTest { private lateinit var greenMail: GreenMail - private val client = ImapClient() + + // Reuse off: this suite pins the connect-per-operation paging behaviour it was written against. + private val client = ImapClient(reuseConnections = false) @Before fun setUp() { diff --git a/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt index c776003..5c23358 100644 --- a/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt @@ -42,7 +42,10 @@ import kotlin.test.assertTrue class ImapClientTest { private lateinit var greenMail: GreenMail - private val client = ImapClient() + + // Reuse off: this suite pins the connect-per-operation IMAP behaviour it was written against. The + // connection-reuse path (production default) has its own coverage in ImapFolderOpenLatencyTest. + private val client = ImapClient(reuseConnections = false) @Before fun setUp() { diff --git a/app/src/test/kotlin/org/libremail/mail/ImapConnectionCacheTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapConnectionCacheTest.kt index 4e02d7e..c70d853 100644 --- a/app/src/test/kotlin/org/libremail/mail/ImapConnectionCacheTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/ImapConnectionCacheTest.kt @@ -1,7 +1,10 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.mail +import io.mockk.every import io.mockk.mockk +import io.mockk.mockkStatic +import io.mockk.unmockkAll import io.mockk.verify import jakarta.mail.Folder import jakarta.mail.FolderClosedException @@ -9,6 +12,8 @@ import jakarta.mail.MessagingException import jakarta.mail.Store import jakarta.mail.StoreClosedException import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Before import org.junit.Test import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.model.MailSecurity @@ -17,10 +22,11 @@ import kotlin.test.assertEquals import kotlin.test.assertFailsWith /** - * The connection-reuse cache (issue #125 spike): one authenticated [Store] per account behind a - * mutex, established lazily and kept open. These tests pin the reuse guarantee and the lazy - * catch-and-retry-once stale handling — a dropped connection is rebuilt and the op retried, a second - * failure clears the slot, and a genuine protocol error is propagated without ever reconnecting. + * The connection-reuse cache (issue #357 Part 2, wiring the #125 spike): one authenticated [Store] per + * account behind a mutex, established lazily and kept open. These tests pin the reuse guarantee, the + * lazy catch-and-retry-once stale handling — a dropped connection is rebuilt and the op retried, a + * second failure clears the slot, and a genuine protocol error is propagated without ever reconnecting + * — plus idle eviction (a connection unused past the timeout is closed) and teardown. */ class ImapConnectionCacheTest { @@ -35,18 +41,41 @@ class ImapConnectionCacheTest { private var connects = 0 - /** A cache whose connect step counts calls and returns [supply] (a fresh relaxed [Store] by default). */ - private fun cache(supply: () -> Store = { mockk(relaxed = true) }) = ImapConnectionCache { - connects++ - supply() + /** Injected monotonic clock (nanos) so idle eviction is deterministic; advanced by the test. */ + private var nowNanos = 0L + + @Before + fun setUp() { + // The cache breadcrumbs through AppLog, which forwards to android.util.Log — a throwing no-op + // stub under plain JVM tests. Mock it class-wide (fully qualified, so this file never imports + // android.util.Log) so no test crashes on the unmocked method. + mockkStatic(android.util.Log::class) + every { android.util.Log.d(any(), any()) } returns 0 + every { android.util.Log.d(any(), any(), any()) } returns 0 + every { android.util.Log.w(any(), any()) } returns 0 + every { android.util.Log.w(any(), any(), any()) } returns 0 } + @After + fun tearDown() = unmockkAll() + + /** A cache whose connect step counts calls and returns [supply] (a fresh relaxed [Store] by default). */ + private fun cache(idleTimeoutMillis: Long = IDLE_TIMEOUT_MS, supply: () -> Store = { mockk(relaxed = true) }) = + ImapConnectionCache( + connect = { + connects++ + supply() + }, + idleTimeoutMillis = idleTimeoutMillis, + nowNanos = { nowNanos }, + ) + @Test fun `establishes one connection and reuses it across calls`() = runTest { val cache = cache() - assertEquals("a", cache.withStore(params) { "a" }) - assertEquals("b", cache.withStore(params) { "b" }) + assertEquals("a", cache.withStore(params, op = "test") { "a" }) + assertEquals("b", cache.withStore(params, op = "test") { "b" }) assertEquals(1, connects, "the second op reuses the first connection") } @@ -59,7 +88,7 @@ class ImapConnectionCacheTest { } var attempts = 0 - val result = cache.withStore(params) { + val result = cache.withStore(params, op = "test") { attempts++ if (attempts == 1) throw IOException("dropped") else "recovered" } @@ -73,10 +102,10 @@ class ImapConnectionCacheTest { fun `a second failure after reconnect clears the slot so the next call reconnects`() = runTest { val cache = cache() - assertFailsWith { cache.withStore(params) { throw IOException("still down") } } + assertFailsWith { cache.withStore(params, op = "test") { throw IOException("still down") } } assertEquals(2, connects, "initial connect plus one rebuild") - cache.withStore(params) { "ok" } + cache.withStore(params, op = "test") { "ok" } assertEquals(3, connects, "the cleared slot forces a fresh connect") } @@ -85,7 +114,7 @@ class ImapConnectionCacheTest { val cache = cache() assertFailsWith { - cache.withStore(params) { throw IllegalStateException("bad login") } + cache.withStore(params, op = "test") { throw IllegalStateException("bad login") } } assertEquals(1, connects, "a non-drop error must not trigger a reconnect") @@ -104,22 +133,44 @@ class ImapConnectionCacheTest { val cache = cache() assertFailsWith { - cache.withStore(params) { throw MessagingException("server said no") } + cache.withStore(params, op = "test") { throw MessagingException("server said no") } } assertEquals(1, connects) } + @Test + fun `evictIdle closes a connection idle past the timeout but keeps a fresh one`() = runTest { + val store = mockk(relaxed = true) + val cache = cache(idleTimeoutMillis = IDLE_TIMEOUT_MS) { store } + + nowNanos = 0L + cache.withStore(params, op = "test") { "a" } // establish; lastUsed = 0 + + // Still within the timeout: not yet idle -> kept. + nowNanos = (IDLE_TIMEOUT_MS - 1) * NANOS_PER_MS + cache.evictIdle() + verify(exactly = 0) { store.close() } + + // Idle past the timeout -> evicted, and the next op reconnects. + nowNanos = IDLE_TIMEOUT_MS * NANOS_PER_MS + cache.evictIdle() + verify(exactly = 1) { store.close() } + + cache.withStore(params, op = "test") { "b" } + assertEquals(2, connects, "after idle eviction the next op reconnects") + } + @Test fun `closeAll tears down and forgets every cached connection`() = runTest { val store = mockk(relaxed = true) val cache = cache { store } - cache.withStore(params) { "a" } + cache.withStore(params, op = "test") { "a" } cache.closeAll() verify { store.close() } - cache.withStore(params) { "b" } + cache.withStore(params, op = "test") { "b" } assertEquals(2, connects, "after closeAll the next op reconnects") } @@ -129,7 +180,7 @@ class ImapConnectionCacheTest { val cache = cache() var attempts = 0 - val result = cache.withStore(params) { + val result = cache.withStore(params, op = "test") { attempts++ if (attempts == 1) throw error else "ok" } @@ -137,4 +188,9 @@ class ImapConnectionCacheTest { assertEquals("ok", result) assertEquals(2, connects, "${error.javaClass.simpleName} should have been retried on a fresh socket") } + + private companion object { + const val IDLE_TIMEOUT_MS = 60_000L + const val NANOS_PER_MS = 1_000_000L + } } diff --git a/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt index e95e584..569865f 100644 --- a/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt @@ -15,6 +15,7 @@ import org.junit.Test import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.model.MailSecurity import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertTrue /** @@ -22,28 +23,36 @@ import kotlin.test.assertTrue * deterministically and without a real network, by routing [ImapClient] through a [CountingImapProxy] * that counts the TCP connections and IMAP commands it establishes. * - * The finding these tests pin down: [ImapClient] wraps every operation in its own short-lived - * [jakarta.mail.Store] (`withStore`), so **each folder-open pays a fresh CONNECT + LOGIN + SELECT + - * FETCH + LOGOUT** — nothing is reused between operations. On a real network the CONNECT + TLS + LOGIN - * group is several RTTs of user-perceived latency that a pooled/kept-alive connection would pay only - * once. See `docs/perf/issue-125-imap-folder-open.md`. + * Two contrasting behaviours are pinned. With reuse **off**, [ImapClient] wraps every operation in its + * own short-lived [jakarta.mail.Store] (`withStore`), so **each folder-open pays a fresh CONNECT + + * LOGIN + SELECT + FETCH + LOGOUT** — nothing is reused. With reuse **on** (the production default, + * issue #357 Part 2), the same real IMAP operations collapse onto **one** kept-alive connection / one + * LOGIN, with the necessary per-folder EXAMINE unchanged — and a dropped socket is transparently + * reconnected, an application error is not mistaken for a drop, and an idle socket is evicted. On a + * real network the CONNECT + TLS + LOGIN group is several RTTs of user-perceived latency (and, on + * Gmail, the connect *volume* that trips server-side throttling) that reuse pays only once. See + * `docs/perf/issue-125-imap-folder-open.md`. * - * These assertions encode the *current* (no-reuse) behaviour. They are also the harness to validate a - * future connection-reuse fix: when the client reuses one authenticated connection across folder - * switches, the connection/auth counts here drop below the operation count — flip the expectations to - * assert reuse and the tests confirm the win against a real IMAP server. + * The reuse-on assertions are also the regression guard: they fail if reuse ever silently regresses to + * connect-per-operation. */ class ImapFolderOpenLatencyTest { private lateinit var greenMail: GreenMail private lateinit var proxy: CountingImapProxy - /** Flag OFF (production default): a fresh connect + LOGOUT per operation. */ - private val client = ImapClient() + /** Reuse OFF: a fresh connect + LOGOUT per operation — the baseline these counts contrast against. */ + private val client = ImapClient(reuseConnections = false) - /** Flag ON (the spike prototype, issue #125): one kept-alive connection reused across operations. */ + /** Reuse ON (production default, issue #357 Part 2): one kept-alive connection reused across ops. */ private val reuseClient = ImapClient(reuseConnections = true) + /** + * Reuse ON with a zero idle timeout, so `evictIdleReusedConnections()` closes the kept-alive socket + * immediately — lets the idle-eviction test assert the teardown deterministically without a clock. + */ + private val evictClient = ImapClient(reuseConnections = true, reuseIdleTimeoutMillis = 0L) + @Before fun setUp() { greenMail = GreenMail(ServerSetupTest.SMTP_IMAP) @@ -57,13 +66,19 @@ class ImapFolderOpenLatencyTest { // this file still never imports android.util.Log — so no test crashes on the unmocked method. mockkStatic(android.util.Log::class) every { android.util.Log.d(any(), any()) } returns 0 + every { android.util.Log.d(any(), any(), any()) } returns 0 // reuse-stale reconnect logs with a throwable every { android.util.Log.i(any(), any()) } returns 0 every { android.util.Log.w(any(), any()) } returns 0 + every { android.util.Log.w(any(), any(), any()) } returns 0 } @After fun tearDown() { - runBlocking { reuseClient.closeReusedConnections() } // release any kept-alive socket before the server stops + // Release any kept-alive socket before the server stops (a no-op for a client that never reused). + runBlocking { + reuseClient.closeReusedConnections() + evictClient.closeReusedConnections() + } proxy.close() greenMail.stop() unmockkAll() @@ -156,7 +171,7 @@ class ImapFolderOpenLatencyTest { assertEquals(2, proxy.authCommandCount(), "list + read each pay a full LOGIN") } - // --- Flag ON: the spike prototype reuses one connection across operations (issue #125). --- + // --- Reuse ON (production default): one connection is reused across operations (issue #357 Part 2). --- // These are the deterministic proof that reuse works: the SAME real-IMAP operations that cost N // connections / N LOGINs above collapse to ONE connection / ONE LOGIN here, with the necessary // per-open EXAMINE unchanged. Localhost is ~0 RTT so this proves the STRUCTURE, not wall-clock. @@ -196,6 +211,58 @@ class ImapFolderOpenLatencyTest { assertEquals(1, proxy.authCommandCount(), "reuse: one LOGIN covers both the list and the read") } + // --- Hardening: transparent stale recovery, narrow drop detection, and idle eviction. --- + + @Test + fun `with reuse on, a dropped connection is transparently reconnected on the next op`() = runTest { + seedInbox(1) + + reuseClient.fetchRecent(params(), "INBOX", limit = 50) // establish the kept-alive connection + assertEquals(1, proxy.connectionCount, "one connection is established and kept alive") + + // The server (or NAT / a network change) silently drops the idle socket. + proxy.dropAcceptedConnections() + + // The next op must NOT surface an error: the cache detects the dead socket, reconnects once, + // and completes the operation, returning its real result. + val messages = reuseClient.fetchRecent(params(), "INBOX", limit = 50) + + assertEquals(1, messages.size, "the op still returns its result after a transparent reconnect") + assertEquals(2, proxy.connectionCount, "a dropped reused socket is transparently reconnected (1 -> 2)") + } + + @Test + fun `with reuse on, an application error reuses the live connection rather than reconnecting`() = runTest { + seedInbox(1) + + reuseClient.fetchRecent(params(), "INBOX", limit = 50) // establish the kept-alive connection + assertEquals(1, proxy.connectionCount) + + // A non-connection error (the UID doesn't exist) must propagate as-is, NOT be mistaken for a + // dropped socket — so the live connection is neither torn down nor needlessly reconnected, and a + // mutation would never be silently re-issued over a working socket. + assertFailsWith { reuseClient.fetchBodyMarkingSeen(params(), "INBOX", "999999") } + + assertEquals(1, proxy.connectionCount, "an application error keeps reusing the one live connection") + } + + @Test + fun `with reuse on, idle eviction closes the socket and the next op reconnects`() = runTest { + seedInbox(1) + + evictClient.fetchRecent(params(), "INBOX", limit = 50) // establish the kept-alive connection + assertEquals(1, proxy.connectionCount) + + // Idle timeout is zero for evictClient, so the sweep closes the just-used socket now. + evictClient.evictIdleReusedConnections() + proxy.awaitClientStreamsSettled() // let the LOGOUT + close flush through the proxy + + assertEquals(1, proxy.commandCount("LOGOUT"), "idle eviction tears the socket down with a LOGOUT") + + evictClient.fetchRecent(params(), "INBOX", limit = 50) // must reconnect, the socket is gone + assertEquals(2, proxy.connectionCount, "the next op after idle eviction reconnects (1 -> 2)") + } + private companion object { const val OPENS = 3 } diff --git a/docs/perf/issue-125-connection-reuse-spike.md b/docs/perf/issue-125-connection-reuse-spike.md index 4759f88..949b698 100644 --- a/docs/perf/issue-125-connection-reuse-spike.md +++ b/docs/perf/issue-125-connection-reuse-spike.md @@ -1,6 +1,17 @@ # IMAP connection-reuse spike (issue #125) +> **Update — shipped (issue #357 Part 2).** The real-device validation this spike deferred has since +> run: an on-device drilldown proved Gmail server-side throttles LibreMail's connect-per-operation IMAP +> (full-history backfill generated ~601 connections in ~22 min, tripping and sustaining a per-account +> rate/bandwidth clamp; `live` peaked at only 5, so it is connection *volume*, not count). Connection +> reuse is therefore now **ON by default**, gated by `BuildConfig.IMAP_CONNECTION_REUSE` as a safety +> switch, with the cache hardened for production: transparent stale-connection recovery, idle eviction +> (`ImapConnectionCache.evictIdle`, swept by `IdleService`), low-battery teardown, and per-account +> mutex concurrency. The single-connection-vs-pool and per-provider-cap knobs below remain a separate +> effort (#356/#360-#364); this change is connection reuse only. The sections below are the original +> spike design, kept for context. + A time-boxed spike that **prototypes** the connection reuse the investigation (`issue-125-imap-folder-open.md`) recommended and defers. It exists to reduce uncertainty — *is per-account keep-alive feasible in this codebase, and does it actually collapse the per-open setup From 81a3b7ea347b681c1fdb2a9e63dd3553a95035b3 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sun, 5 Jul 2026 20:36:38 -0500 Subject: [PATCH 3/4] refactor(mail): early-return guard in ImapConnectionCache (#357 review) Restructure isConnectionDrop as leading guard clauses (definite-drop types, then a not-MessagingException early return) instead of a when expression, per maintainer review feedback on PR #368. Behavior is unchanged; verified by the existing ImapConnectionCacheTest suite (all 8 cases still pass), including the FolderClosedException / StoreClosedException cases that depend on the check running before the MessagingException .cause guard. Co-Authored-By: Claude Opus 4.8 --- .../kotlin/org/libremail/mail/ImapConnectionCache.kt | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt b/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt index 40e1c8b..3b2446a 100644 --- a/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt +++ b/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt @@ -207,10 +207,14 @@ internal class ImapConnectionCache( * while idle, detected on the next op's first command before any mutation is issued — is safe. A * copy-then-expunge move interrupted between its two halves is the rare exception left to that review. */ - private fun isConnectionDrop(error: Throwable): Boolean = when (error) { - is FolderClosedException, is StoreClosedException, is IOException, is ConnectionException -> true - is MessagingException -> error.cause is IOException || error.cause is ConnectionException - else -> false + private fun isConnectionDrop(error: Throwable): Boolean { + // FolderClosedException/StoreClosedException are themselves MessagingException subtypes, so these + // definite-drop checks must run before the MessagingException guard below — otherwise they'd fall + // into it and get gated on a `.cause` they don't carry, instead of the unconditional `true` below. + if (error is FolderClosedException || error is StoreClosedException) return true + if (error is IOException || error is ConnectionException) return true + if (error !is MessagingException) return false + return error.cause is IOException || error.cause is ConnectionException } private companion object { From e36afc8ade9ad7d4c11c44041b26623171783736 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sun, 5 Jul 2026 21:18:49 -0500 Subject: [PATCH 4/4] changed conditional style to easier to read/maintain when (like switch) statement --- .../kotlin/org/libremail/mail/ImapConnectionCache.kt | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt b/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt index 3b2446a..3dddea0 100644 --- a/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt +++ b/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt @@ -211,10 +211,12 @@ internal class ImapConnectionCache( // FolderClosedException/StoreClosedException are themselves MessagingException subtypes, so these // definite-drop checks must run before the MessagingException guard below — otherwise they'd fall // into it and get gated on a `.cause` they don't carry, instead of the unconditional `true` below. - if (error is FolderClosedException || error is StoreClosedException) return true - if (error is IOException || error is ConnectionException) return true - if (error !is MessagingException) return false - return error.cause is IOException || error.cause is ConnectionException + when { + error is FolderClosedException || error is StoreClosedException -> return true + error is IOException || error is ConnectionException -> return true + error !is MessagingException -> return false + else -> return error.cause is IOException || error.cause is ConnectionException + } } private companion object {