From 8bfc31f17c2993efa9d91f07833c6e2ef082f8ef Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 09:29:55 -0500 Subject: [PATCH] spike(imap): prototype flag-gated connection reuse for folder-open Prototype the per-account connection reuse the #125 investigation recommended and deferred, behind an OFF-by-default flag so it cannot destabilize `main`. - ImapConnectionCache: keeps one authenticated Store alive per account, guarded by a per-account mutex, keyed by connection identity (not the rotating secret), with lazy catch-and-retry-once stale handling. No eviction policy yet beyond an explicit closeReusedConnections() hook. - ImapClient gains a `reuseConnections` flag (default false via the @Inject no-arg constructor). With it off, withStore is byte-for-byte the previous connect + LOGOUT-per-call; with it on, calls borrow the kept-alive Store. - ImapFolderOpenLatencyTest flips the flag on: the same real-IMAP operations that cost N connections / N LOGINs collapse to 1 connection / 1 LOGIN, with the necessary per-open EXAMINE unchanged (proven via CountingImapProxy + GreenMail; localhost is ~0 RTT so this proves structure, not wall-clock). - docs/perf/issue-125-connection-reuse-spike.md: prototype design, the flag-off-vs-on proof, per-decision trade-offs, and the refined real-device validation plan. References #125; does not close it (needs device validation). Co-Authored-By: Claude Fable 5 --- .../kotlin/org/libremail/mail/ImapClient.kt | 104 +++++++++---- .../org/libremail/mail/ImapConnectionCache.kt | 111 +++++++++++++ .../mail/ImapFolderOpenLatencyTest.kt | 47 ++++++ docs/perf/issue-125-connection-reuse-spike.md | 147 ++++++++++++++++++ docs/perf/issue-125-imap-folder-open.md | 5 + 5 files changed, 387 insertions(+), 27 deletions(-) create mode 100644 app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt create mode 100644 docs/perf/issue-125-connection-reuse-spike.md diff --git a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt index 001409d..3ce17cd 100644 --- a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt +++ b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt @@ -86,7 +86,23 @@ data class ReplyContext( /** Thin IMAP client over Jakarta/Angus Mail. Supports password and XOAUTH2 auth. */ @Singleton -class ImapClient @Inject constructor() { +class ImapClient(private val reuseConnections: Boolean) { + + /** + * 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. + */ + @Inject constructor() : this(reuseConnections = false) + + /** + * 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. + */ + private val connectionCache: ImapConnectionCache? = + if (reuseConnections) ImapConnectionCache(::openConnectedStore) else null /** Connects and returns the account's folders with their SPECIAL-USE attributes. Throws on failure. */ suspend fun listFolders(params: ImapConnectionParams): List = withContext(Dispatchers.IO) { @@ -530,37 +546,71 @@ class ImapClient @Inject constructor() { ) } - private inline fun withStore(params: ImapConnectionParams, block: (Store) -> T): T { - val protocol = if (params.security == MailSecurity.SSL_TLS) "imaps" else "imap" - val store = Session.getInstance(buildProps(protocol, params)).getStore(protocol) - store.connect(params.host, params.port, params.username, params.secret) - return try { - block(store) - } finally { - runCatching { store.close() } + /** + * Runs [block] against a connected [Store]. With the reuse flag OFF (default) this is the original + * behaviour: a fresh, authenticated connection per call, torn down in `finally`. With it ON, the + * call borrows a kept-alive per-account connection from [connectionCache] (established once, reused + * across folder-opens) instead — see issue #125. + */ + private suspend fun withStore(params: ImapConnectionParams, block: (Store) -> T): T { + val cache = connectionCache + return if (cache != null) { + cache.withStore(params, block) + } else { + val store = openConnectedStore(params) + try { + block(store) + } finally { + runCatching { store.close() } + } } } - private fun buildProps(protocol: String, params: ImapConnectionParams): Properties = Properties().apply { - put("mail.store.protocol", protocol) - put("mail.$protocol.host", params.host) - put("mail.$protocol.port", params.port.toString()) - put("mail.$protocol.connectiontimeout", TIMEOUT_MS) - put("mail.$protocol.timeout", TIMEOUT_MS) - put("mail.$protocol.writetimeout", TIMEOUT_MS) - if (params.security == MailSecurity.STARTTLS) { - put("mail.$protocol.starttls.enable", "true") - put("mail.$protocol.starttls.required", params.strictStartTls.toString()) - } - // Verify the server certificate matches the host whenever TLS is used. Angus already - // defaults this to true; set it explicitly so a future library-default change can't - // silently disable hostname checking and expose us to MITM. (No-op for MailSecurity.NONE.) - put("mail.$protocol.ssl.checkserveridentity", "true") - if (params.useXoauth2) { - put("mail.$protocol.auth.mechanisms", "XOAUTH2") - } + /** Builds and authenticates a fresh [Store] (`CONNECT + TLS + LOGIN`); the caller owns closing it. */ + private fun openConnectedStore(params: ImapConnectionParams): Store { + val protocol = if (params.security == MailSecurity.SSL_TLS) "imaps" else "imap" + val store = Session.getInstance(buildProps(protocol, params, reuse = reuseConnections)).getStore(protocol) + store.connect(params.host, params.port, params.username, params.secret) + return store } + /** + * 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). + */ + suspend fun closeReusedConnections() { + connectionCache?.closeAll() + } + + private fun buildProps(protocol: String, params: ImapConnectionParams, reuse: Boolean = false): Properties = + Properties().apply { + put("mail.store.protocol", protocol) + put("mail.$protocol.host", params.host) + put("mail.$protocol.port", params.port.toString()) + put("mail.$protocol.connectiontimeout", TIMEOUT_MS) + put("mail.$protocol.timeout", TIMEOUT_MS) + put("mail.$protocol.writetimeout", TIMEOUT_MS) + if (params.security == MailSecurity.STARTTLS) { + put("mail.$protocol.starttls.enable", "true") + put("mail.$protocol.starttls.required", params.strictStartTls.toString()) + } + // Verify the server certificate matches the host whenever TLS is used. Angus already + // defaults this to true; set it explicitly so a future library-default change can't + // silently disable hostname checking and expose us to MITM. (No-op for MailSecurity.NONE.) + put("mail.$protocol.ssl.checkserveridentity", "true") + if (params.useXoauth2) { + put("mail.$protocol.auth.mechanisms", "XOAUTH2") + } + if (reuse) { + // SPIKE (issue #125): pin a reused Store to exactly one authenticated socket. The + // per-account mutex already serializes access, so one pooled connection suffices — and + // capping it here makes "1 connection for N opens" the literal, provable invariant. + put("mail.$protocol.connectionpoolsize", "1") + put("mail.$protocol.separatestoreconnection", "false") + } + } + private companion object { const val TIMEOUT_MS = "15000" const val TAG = "LibreMailIdle" diff --git a/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt b/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt new file mode 100644 index 0000000..899f3b2 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/mail/ImapConnectionCache.kt @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import jakarta.mail.FolderClosedException +import jakarta.mail.MessagingException +import jakarta.mail.Store +import jakarta.mail.StoreClosedException +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock +import org.libremail.domain.model.ImapConnectionParams +import java.io.IOException +import java.util.concurrent.ConcurrentHashMap + +/** + * 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`. + * + * Prototype stance — deliberately the simplest thing that *proves reuse*, leaving the tuning knobs to a + * measured follow-up: + * - **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. + * - **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]. + * + * 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. + * + * @param connect builds and authenticates a fresh [Store] for the given params (blocking network I/O). + */ +internal class ImapConnectionCache(private val connect: (ImapConnectionParams) -> Store) { + + private class Entry { + val mutex = Mutex() + var store: Store? = null + } + + private val entries = ConcurrentHashMap() + + /** + * 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. + */ + suspend fun withStore(params: ImapConnectionParams, block: (Store) -> T): T { + val entry = entries.computeIfAbsent(key(params)) { Entry() } + return entry.mutex.withLock { + val store = entry.store ?: connect(params).also { entry.store = it } + 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() } + entry.store = null + throw retry + } + } + } + } + + /** Closes and forgets every cached connection (`LOGOUT` + socket teardown). The only eviction today. */ + suspend fun closeAll() { + for ((_, entry) in entries) { + entry.mutex.withLock { + entry.store?.let { store -> runCatching { store.close() } } + entry.store = null + } + } + entries.clear() + } + + /** + * Account identity for reuse — everything that pins a distinct authenticated socket EXCEPT the + * secret, so a rotated OAuth token reuses the same live connection instead of orphaning it. + */ + private fun key(params: ImapConnectionParams): String = + "${params.host}|${params.port}|${params.security}|${params.username}|${params.useXoauth2}" + + /** + * 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. + */ + private fun isConnectionDrop(error: Throwable): Boolean = when (error) { + is FolderClosedException, is StoreClosedException, is IOException -> true + is MessagingException -> error.cause is IOException + else -> false + } +} diff --git a/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt index 62c5dd1..77a2cd1 100644 --- a/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt @@ -4,6 +4,7 @@ package org.libremail.mail import com.icegreen.greenmail.util.GreenMail import com.icegreen.greenmail.util.GreenMailUtil import com.icegreen.greenmail.util.ServerSetupTest +import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.runTest import org.junit.After import org.junit.Before @@ -33,8 +34,13 @@ 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() + /** Flag ON (the spike prototype, issue #125): one kept-alive connection reused across operations. */ + private val reuseClient = ImapClient(reuseConnections = true) + @Before fun setUp() { greenMail = GreenMail(ServerSetupTest.SMTP_IMAP) @@ -46,6 +52,7 @@ class ImapFolderOpenLatencyTest { @After fun tearDown() { + runBlocking { reuseClient.closeReusedConnections() } // release any kept-alive socket before the server stops proxy.close() greenMail.stop() } @@ -119,6 +126,46 @@ 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). --- + // 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. + + @Test + fun `with reuse on, N folder-opens share one connection and one LOGIN`() = runTest { + seedInbox(2) + + repeat(OPENS) { reuseClient.fetchRecent(params(), "INBOX", limit = 50) } + + // The win: one socket accepted for all OPENS opens (vs. OPENS sockets with the flag off). + // connectionCount is incremented synchronously on accept, so it needs no stream settling. + assertEquals(1, proxy.connectionCount, "reuse: a single TCP connection serves every folder-open") + + reuseClient.closeReusedConnections() // evict -> LOGOUT + close, so the proxy's command stream settles + proxy.awaitClientStreamsSettled() + + assertEquals(1, proxy.authCommandCount(), "reuse: LOGIN paid once, then reused (vs. one per open)") + assertEquals(OPENS, proxy.commandCount("EXAMINE"), "reuse keeps the necessary one EXAMINE per open") + assertEquals(1, proxy.commandCount("LOGOUT"), "reuse: one LOGOUT at eviction, not one per open") + } + + @Test + fun `with reuse on, opening a folder then reading a message reuses the one connection`() = runTest { + seedInbox(1) + + val uid = reuseClient.fetchRecent(params(), "INBOX", limit = 50).first().uid // open folder + reuseClient.fetchBodyMarkingSeen(params(), "INBOX", uid) // read a message in it + + // Contrast with `opening a folder then reading a message uses two separate connections`: the + // same list-then-read here shares the one kept-alive connection instead of paying a second setup. + assertEquals(1, proxy.connectionCount, "reuse: list + read share the one connection") + + reuseClient.closeReusedConnections() + proxy.awaitClientStreamsSettled() + + assertEquals(1, proxy.authCommandCount(), "reuse: one LOGIN covers both the list and the read") + } + private companion object { const val OPENS = 3 } diff --git a/docs/perf/issue-125-connection-reuse-spike.md b/docs/perf/issue-125-connection-reuse-spike.md new file mode 100644 index 0000000..4759f88 --- /dev/null +++ b/docs/perf/issue-125-connection-reuse-spike.md @@ -0,0 +1,147 @@ + +# IMAP connection-reuse spike (issue #125) + +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 +cost?* — not to ship a finished feature. The prototype is **flag-gated and OFF by default**, so it +cannot change `main`'s behaviour, and the win is proven structurally with the existing GreenMail +harness. + +> **Still no wall-clock numbers.** As in the investigation, everything here counts *protocol +> round-trips* (deterministic in-process) and TCP connections. Localhost GreenMail is ~0 RTT, so this +> spike proves the connection is **reused** (structure), not how many milliseconds that saves (that is +> the real-device work in the last section). No latency figure is fabricated. + +## What the spike delivers + +1. A flag-gated per-account keep-alive **prototype** — `ImapConnectionCache` + an OFF-by-default + `reuseConnections` flag on `ImapClient`. +2. **Deterministic proof it reuses the connection** — two new `ImapFolderOpenLatencyTest` cases that + flip the flag on and assert the connection/LOGIN counts collapse, run against real in-process IMAP. +3. This design note: the prototype's stance on each real design decision, and the refined real-device + validation plan. + +## The prototype + +### The flag (default OFF, cannot destabilize `main`) + +`ImapClient`'s production constructor is unchanged in behaviour: + +```kotlin +class ImapClient(private val reuseConnections: Boolean) { + @Inject constructor() : this(reuseConnections = false) // production: reuse OFF + ... +} +``` + +Hilt still calls the no-arg `@Inject` constructor, so every production/`ImapClient()` call site gets +`reuseConnections = false`. With the flag off, `withStore` is byte-for-byte the previous +connect-per-call + `LOGOUT`-per-call code, the reuse cache is **never allocated**, and no new state or +code path is reachable. Only the harness opts in, via `ImapClient(reuseConnections = true)`. When +real-device validation confirms the win, this flag is what gets wired to a setting / `BuildConfig`. + +### The reused connection — `ImapConnectionCache` + +`ImapConnectionCache` keeps one authenticated `jakarta.mail.Store` alive per account and lends it out: + +- **One connection per account, mutex-guarded.** Each account key owns a single `Store` behind its own + coroutine `Mutex`; `withStore` locks it, ensures the `Store` is connected (creating it on first use), + runs the operation, and returns **without closing it**. Angus's `IMAPStore` internally pools the + authenticated connection across folder `open()`/`close()`, so a kept-alive `Store` reuses one socket; + the reused store is pinned to `connectionpoolsize=1` + `separatestoreconnection=false` so it is + provably a single socket. +- **Keyed by connection identity, not the secret.** The key is + `host|port|security|username|useXoauth2` — deliberately **excluding** `secret`, so a rotated OAuth + access token reuses the same live, already-authenticated socket instead of orphaning it. The current + `params` (with the fresh secret) is always passed to `connect`, so a genuine reconnect uses the new + token. +- **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 throws a + dropped-connection signal (`FolderClosedException`, `StoreClosedException`, or a `MessagingException` + caused by `IOException`), the socket is rebuilt once and the op retried. A non-connection error + (e.g. "message not found") is never retried. +- **`closeReusedConnections()`** evicts everything (`LOGOUT` + teardown). Today it is the *only* + eviction, driven by the harness; a shipped feature would also drive it from an idle timer and the + low-battery push teardown. + +IDLE is untouched: `ImapClient.idle` still opens its own dedicated long-lived `Store` (it is *not* in +the cache), so the reuse connection is strictly **additional** to the IDLE connection — which is +exactly why the per-account connection budget below is a first-class concern. + +## Deterministic proof (the harness, flag off vs on) + +`ImapFolderOpenLatencyTest` routes `ImapClient` through `CountingImapProxy` (a localhost TCP proxy in +front of GreenMail that counts TCP connections and parses IMAP command words). The existing cases pin +the flag-**off** behaviour; the two new cases flip the flag **on** over the *same* real IMAP +operations. For `N = OPENS = 3` folder-opens: + +| Scenario | TCP connections | LOGIN | EXAMINE (per open) | LOGOUT | +|----------|-----------------|-------|--------------------|--------| +| **Flag OFF** — `N` folder-opens | `N` (=3) | `N` (=3) | `N` (=3) | `N` (=3) | +| **Flag ON** — `N` folder-opens | **1** | **1** | `N` (=3) | **1** (at eviction) | +| **Flag OFF** — open folder + read a message | 2 | 2 | (1 EXAMINE + 1 SELECT) | 2 | +| **Flag ON** — open folder + read a message | **1** | **1** | (1 EXAMINE + 1 SELECT) | **1** | + +The avoidable setup — `CONNECT + TLS + LOGIN` and the trailing `LOGOUT` — drops from *once per +operation* to *once per account, ever*, while the intrinsic per-folder `EXAMINE` is unchanged. That +divergence (operations ≫ connections/LOGINs) **is** connection reuse, proven against a real IMAP +server. These flag-on assertions are also the regression guard the investigation asked for: they fail +if reuse ever silently regresses to connect-per-call. + +All six cases pass on the JVM fast gate (`:app:testDebugUnitTest`); no emulator needed. + +## Real design decisions — the prototype's stance and the trade-offs + +The spike takes the **simplest defensible** position on each knob and leaves the tuning to +measurement. Each is a genuine latency/battery/complexity trade-off that localhost cannot settle. + +| Decision | Prototype's stance | Trade-off / what's left open | +|----------|-------------------|------------------------------| +| **Single connection vs. bounded pool** | Single mutex-guarded connection per account. | Simplest and provably one socket, but **head-of-line blocking**: a quick flag toggle can queue behind a slow body download — a regression of today's connect-per-call concurrency. A bounded pool (N sockets + a size cap) restores parallelism at the cost of more sockets and eviction bookkeeping. Which wins needs real throughput/latency measurement. | +| **Idle-eviction timeout** | None yet; a connection lives until `closeReusedConnections()`. | A kept-alive socket has a battery cost (below). The right idle timeout is a battery-vs-latency trade-off; the hook exists (`closeReusedConnections`) but no timer drives it. | +| **Stale-connection detection** | Lazy catch-and-retry-once on a dropped-connection signal; no `NOOP` probe. | Retry avoids a per-op probe RTT but means one operation *fails then recovers* when a stale socket is first used; a `NOOP` pre-check trades that for a guaranteed extra RTT on every op. For a **mutating** op, an automatic retry after a mid-flight drop is at-least-once — safe for the read-only folder-open target, but a real-server correctness item for flags/move/expunge. | +| **IDLE per-account budget (#90)** | Reuse connection is **additional** to the IDLE connection (IDLE stays separate). | So an account holding IDLE **and** a reuse connection uses ≥2 persistent sockets; a bounded pool would use even more. Must stay under the server's per-account limit (Gmail ~15; many servers 3–5). A shipped version should treat IDLE + reuse (+ pool) as one budget. | +| **Concurrency (prefetch outside `syncMutex`, unserialized UI ops)** | The per-account mutex serializes *all* reuse traffic for an account. | Correct and thread-safe under the current design (concurrent UI ops + prefetch can hit the same account), but it serializes work that today runs concurrently on separate throwaway sockets — the head-of-line cost again. A pool would relax this. | +| **Low-battery posture (#88/#89/#90)** | None yet — no battery signal wired in. | A kept-alive socket has idle cost; #90 already tears IDLE down at low battery. Reuse should mirror that (evict + stop reusing at low battery). The eviction hook exists; the policy wiring is deferred. | + +**Deliberately left open** (out of this spike's scope): the eviction timer, the battery-signal wiring, +the bounded-pool variant, unifying the IDLE + reuse connection budget, and the mutation-retry +idempotency review. Each needs the real-device measurement below to tune, not a guess. + +## Feasibility verdict + recommendation + +**Feasible, and mechanically small.** The reuse path is one ~90-line class plus a flag; the existing +concurrency model already hands us the seam (a single `withStore` chokepoint every operation flows +through), and Angus's own connection pooling does the socket reuse once we stop discarding the `Store`. +The structural win is real and now proven: setup collapses from per-operation to per-account. + +**Recommendation:** keep the flag **OFF** and land this as a spike (harness + prototype + this note). +Before flipping the default on, do the real-device validation below and decide the two knobs that +localhost cannot: **single connection vs. bounded pool** (measure the head-of-line cost against real +concurrent UI-op + prefetch traffic) and the **idle-eviction timeout** (measure the kept-alive +socket's battery cost). Ship the mutation-retry idempotency review and the IDLE-budget unification +alongside. If the pool is chosen, the mutex-per-account seam generalizes to a bounded semaphore with +minimal churn. + +## Real-device / real-account validation that remains + +Refines the investigation's six-step plan against what *this prototype* needs: + +1. **A/B the flag on real accounts/networks.** Flip `reuseConnections` on (wire it to a debug setting) + and measure folder-switch (open A → open B → back to A) and list-then-open-message latency, cold vs. + warm-reuse, on Gmail + Outlook over Wi-Fi and cellular. Expect warm opens to fall by the + connection-setup share; quantify it. +2. **Attribute the wall-clock.** Instrument `store.connect` / `open` / `fetch` / `close` (or Angus + `mail.imap` debug) and confirm setup dominates the cold open and is what reuse removes. +3. **Decide single vs. pool.** Under real concurrent traffic (UI op + prefetch on one account), + measure the single-connection head-of-line delay; if material, prototype the bounded pool and + re-measure. +4. **Tune idle-eviction against battery.** Measure the kept-alive socket's idle drain across candidate + timeouts; pick one that beats the #88/#89/#90 posture, and wire `closeReusedConnections()` to that + timer and to the low-battery teardown. +5. **Resilience + connection budget.** Force server idle-timeout and network transitions; confirm the + catch-and-retry-once reconnect is transparent (and review mutation idempotency), and that IDLE + + reuse (+ pool) stay under the per-account connection limit. +6. **Lock it in.** The flag-on `ImapFolderOpenLatencyTest` cases are already the deterministic + regression guard; once the default flips on, they assert reuse can't silently regress. diff --git a/docs/perf/issue-125-imap-folder-open.md b/docs/perf/issue-125-imap-folder-open.md index aebc139..10d2486 100644 --- a/docs/perf/issue-125-imap-folder-open.md +++ b/docs/perf/issue-125-imap-folder-open.md @@ -153,6 +153,11 @@ real device — which this environment cannot provide — forcing an implementat Per #125's "investigation/spike first" guidance, this change ships the measurement harness + analysis and **defers the pool to a measured follow-up**. +> **Follow-up spike.** A flag-gated (default OFF) prototype of this reuse now exists, with the harness +> flipped to prove it collapses `N` opens to one connection / one LOGIN. See +> [`issue-125-connection-reuse-spike.md`](issue-125-connection-reuse-spike.md) for the prototype +> design, the flag-off-vs-on proof, and the per-decision trade-offs. + **Already correct — do not redo.** Optimistic render-from-cache is already the architecture (`selectFolder` renders cached rows instantly; the network sync is a background refresh). #125's "optimistic render while the network catches up" is satisfied; only connection reuse remains.