Merge pull request #368 from JMR-dev/feat-125-imap-connection-reuse

perf(mail): enable IMAP connection reuse by default with a hardened cache
2026-07-05 22:18:11 -05:00
parent a069188b65 e36afc8ade
commit b8d67557a0
11 changed files with 424 additions and 98 deletions
@@ -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
@@ -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<FetchedFolder> = withContext(Dispatchers.IO) {
@@ -632,7 +639,7 @@ class ImapClient(private val reuseConnections: Boolean) {
    private suspend fun <T> 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
    }
 }

@@ -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<String, Entry>()
+    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 <T> 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 <T> 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 <T> 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 <T> 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,42 @@ 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
-        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.
+        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 {
+        const val TAG = "ImapReuse"
+        const val PERF_TAG = "ImapPerf"
+        const val NANOS_PER_MS = 1_000_000L
    }
 }
@@ -112,6 +112,10 @@ 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() }
    }

    /**
@@ -134,6 +138,20 @@ class IdleService : Service() {
        degradeToPeriodicSync()
    }

+    /**
+     * 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
@@ -185,6 +203,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")
        }
@@ -310,6 +332,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
@@ -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",
@@ -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<Thread>())

+    /** Accepted client-side sockets, so a test can force-drop them to simulate a server/NAT disconnect. */
+    private val acceptedSockets = Collections.synchronizedList(mutableListOf<Socket>())
+
    @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)
@@ -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() {
@@ -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() {
@@ -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<String>(), any<String>()) } returns 0
+        every { android.util.Log.w(any<String>(), any<String>(), 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<IOException> { cache.withStore(params) { throw IOException("still down") } }
+        assertFailsWith<IOException> { 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<IllegalStateException> {
-            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<MessagingException> {
-            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<Store>(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<Store>(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
+    }
 }
@@ -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<String>(), any<String>()) } returns 0
+        every { android.util.Log.w(any<String>(), any<String>(), 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<Exception> { 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
    }
@@ -1,6 +1,17 @@
 <!-- SPDX-License-Identifier: GPL-3.0-or-later -->
 # 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