Merge branch 'main' into fix-359-sqlcipher-16kb

2026-07-06 12:50:29 -05:00
parent 921681812c b8d67557a0
commit 5c715e1676
14 changed files with 782 additions and 121 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
@@ -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<Context>()
+
+    @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)
+    }
+}
@@ -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
    }
 }
@@ -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
+    }
+}
@@ -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,72 @@ class IdleService : Service() {
    /** Active IDLE watcher per account id, so we can start/stop them as accounts change. */
    private val watchers = mutableMapOf<String, Job>()

-    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()
+        }
+        // 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() }
+    }
+
+    /**
+     * 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()
+    }
+
+    /**
+     * 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) }
        }
-        return START_STICKY
    }

    /**
@@ -152,24 +203,43 @@ 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")
        }
    }

    /**
-     * 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 +257,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
@@ -243,8 +332,24 @@ 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
+
+        // 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
    }
 }
@@ -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
    }
@@ -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)
+    }
+}
@@ -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