[dbezverkhnii]

Problem

IMAPAsyncSession::sessionWithMinQueue ranks pooled connections by queue length alone. A connection whose socket mailcore dropped after its idle delay therefore looks exactly as free as a warm one (operationsCount() == 0), and ties go to position in mSessions. With one warm and one cold connection free, the first asker — the most urgent one, since the queue starts it first — can be handed the cold socket and pay a full CONNECT + LOGIN, while the operation that asks second gets the warm one. A lease makes that stick for the whole operation.

Measured on Yahoo from the desktop client: a body on a warm connection answers in 0.33 s (UID FETCH … BODY.PEEK goes out on the same millisecond as the lease); the connection that has to come back up spends 2.36 s on the greeting, CAPABILITY, authentication, SELECT, COMPRESS, NAMESPACE and ID before it can ask for anything.

Change

Among equally free candidates — same queue length — prefer the one whose session is connected. The tie only: a shorter queue still wins outright, connected or not. Queue length counts commands without weighing them, so trading one unknown command against a bounded handshake would be the worse deal, and a lease keeps being handed an empty queue.

IMAPAsyncConnection::isDisconnected() exposes what IMAPSession already tracks; no other new API. With no disconnected candidate in the pool the loop is byte-for-byte what it was: the new clause can only fire when the chosen connection is disconnected, which cannot happen before the first assignment.

Tests

unittest/IMAPConnectionLeaseTests.swift — both pool positions, both acquireConnection paths (folder-matched and plain), a never-connected connection, the unpinned regular operation, and the all-reserved fallback; plus the guards for what must not change: a shorter queue wins even when it is the disconnected one, and two live idle connections keep their pool order. Five of them fail on this branch's base commit.

Whole suite: 23 in the lease file, all green; the only failure in the repository is the pre-existing testSummary (date locale).

Stand

Verified end-to-end against live Yahoo in the desktop client (mailcore pinned to this branch): the wire capture above is from that run, with the lease going to the keep-alive-warmed connection and the second, concurrent request paying the handshake. No regressions in either pool over ~25 minutes.

Worth stating plainly: the stand did not catch the old code making the wrong choice. Keep-alive warms a connection through the same acquireConnection, so the warm one is usually also the first free one in mSessions — exactly what the old rule picks. The two diverge only when the warm connection sits later in the array, a state that does occur (after an OAuth refresh recreates the pool, keep-alive held a later connection warm for ten minutes) but that no body fetch landed in during the runs.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes core async IMAP connection-pool selection and cross-thread session state reads; behavior is constrained to queue-length ties but affects latency-sensitive paths.

Overview
When two IMAP pool connections are equally idle (same queue length), connection selection now favors one that is still connected instead of breaking ties purely by pool order. That avoids handing the first lease or unpinned operation a socket that must pay a full connect/login handshake while a warm connection sits unused.

IMAPAsyncConnection::isDisconnected() exposes existing session liveness for that tie-break (appended last on the exported class to avoid shifting vtables). IMAPSession::mState is now std::atomic<int> so pool selection on another thread can read disconnect state safely while the connection thread updates it.

Shorter queues still win outright—the live preference applies only on ties. unittest/IMAPConnectionLeaseTests.swift adds coverage for leases, folder acquire, unpinned ops, exhausted-pool sharing, and guards that queue length and live-only ties behave as before.

^{Reviewed by Cursor Bugbot for commit fe7f5fa. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Copilot]

🟡 Changes recommended

Critical findings identify an unsynchronized state read and ABI-breaking virtual-method changes.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates IMAP connection-pool selection to prefer connected sessions when queue lengths tie, reducing reconnect latency.

Changes:

• Adds connection liveness accessors.
• Applies liveness as a queue-selection tie-breaker.
• Expands lease and connection-selection tests.

File summaries

File	Summary
unittest/IMAPConnectionLeaseTests.swift	Adds coverage for live versus disconnected connection selection.
src/include/MailCore/MCIMAPAsyncConnection.h	Declares the liveness API; critical ABI-breaking virtual-method insertion (1 vote).
src/async/imap/MCIMAPAsyncSession.cpp	Applies the connected-session tie-breaker.
src/async/imap/MCIMAPAsyncConnection.h	Declares the internal method; critical data-race and ABI findings (1 vote each).
src/async/imap/MCIMAPAsyncConnection.cpp	Forwards liveness state; critical unsynchronized state access/data-race finding (2 votes).

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[dbezverkhnii]

Both review findings are fixed in fe7f5fa.

Data race on the connection state. Right — the selection reads it from whatever thread starts an operation, while IMAPSession writes it on its own, so a plain int makes it a race and not the benign staleness the header claimed. mState is now std::atomic<int>: every existing read and write stays exactly as written, the field keeps its size and alignment, and it is lock-free for an int. Locking the read alone would have been theatre — the writers do not take that lock either.

Vtable layout. Also right, and free to fix: isDisconnected() now sits after every existing virtual in IMAPAsyncConnection, so operationsCount() and everything after it keep their slots. Both header copies stay byte-identical.

Lease suite is 23/23; the only failure in the repository is the pre-existing testSummary (date locale).

[dbezverkhnii]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit fe7f5fa. Configure here.}

[Copilot]

🟡 Changes recommended

Publish the corresponding Windows prebuilt artifact before merging.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Core async pool selection and cross-thread state changes warrant final human review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite