COR-205 - Rank a pooled connection by the reconnect it owes

[dbezverkhnii]

Follow-up to #111, from a review of the merged change.

The inversion

The tie-break added in #111 asks IMAPSession::isDisconnected(), which is only mState == STATE_DISCONNECTED. Nothing but the constructor and unsetup() ever writes that state — a command that fails or is interrupted leaves the session LOGGEDIN/SELECTED and raises mShouldDisconnect instead (61 sites), and connectIfNeeded then pays disconnect + connect + login on the next command, strictly more than the connect + login a closed socket costs.

So the pool ranked its most expensive connection as its cheapest, and at equal queue length handed exactly that one to the lease — every time, where before the tie-break it was a coin flip on pool order.

The state is a normal one on this branch: interruptCurrentCommand() raises the flag deliberately and leaves the connection pooled to heal on its next command (5f27dfd, COR-201), and any stream or parse error does the same.

The change

IMAPSession::needsReconnect() — mState == STATE_DISCONNECTED || mShouldDisconnect — is what the selection asks now. isDisconnected() keeps its meaning on purpose: IMAPAsyncConnection::tryAutomaticDisconnect() asks it whether the socket is already gone, and a flagged connection still holds one; teaching that predicate about the flag would leave those sockets open until the next command.

mShouldDisconnect is std::atomic<bool> for the same reason mState became atomic in #111: the selection reads it from the thread that starts an operation.

Tests

testAcquirePrefersTheLiveConnectionOverAnInterruptedOne — a connection whose NOOP was interrupted stays connected and owing a reconnect; the lease must go to the other one. It fails if the predicate is put back to the socket state alone. Lease suite 24/24; the only failure in the repository is the pre-existing testSummary (date locale).

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes IMAP connection-pool selection and cross-thread visibility of teardown state; wrong ranking would add extra handshakes but not auth/data corruption.

Overview
Fixes inverted connection-pool tie-breaking from the prior lease work: equal queue depth no longer favors connections that still look “connected” but owe a tear-down + reconnect after a failed or interrupted command.

Adds IMAPSession::needsReconnect() (disconnected or mShouldDisconnect) and renames IMAPAsyncConnection::isDisconnected() to needsReconnect() for pool selection in sessionWithMinQueue. isDisconnected() stays socket-only so automatic idle disconnect logic is unchanged.

Makes mShouldDisconnect std::atomic<bool> for cross-thread reads during selection, and sets the flag on login LIST and NOOP stream/parse failures so interrupted streams are ranked correctly.

Adds testAcquirePrefersTheLiveConnectionOverAnInterruptedOne to assert acquireConnection picks the healthy connection when the other owes a reconnect.

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

[dbezverkhnii]

bugbot run

[cursor]

Stale Bugbot comment from a previous run.

[Copilot]

🟡 Changes recommended

Preserve existing exported virtual API/vtable compatibility before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates IMAP connection-pool selection to prefer connections that do not require reconnection after failures or interruptions.

Changes:

• Adds reconnect-aware session state tracking.
• Uses reconnect readiness for equal-queue tie-breaking.
• Adds regression coverage for interrupted connections.
• Requires preserving existing exported virtual API/vtable compatibility.

File summaries

File	Summary
unittest/IMAPConnectionLeaseTests.swift	Adds interrupted-connection leasing coverage.
src/include/MailCore/MCIMAPSession.h	Exposes reconnect state and atomic flag.
src/include/MailCore/MCIMAPAsyncConnection.h	Updates the async connection API.
src/core/imap/MCIMAPSession.h	Mirrors session declarations.
src/core/imap/MCIMAPSession.cpp	Implements reconnect-state detection.
src/async/imap/MCIMAPAsyncSession.cpp	Applies reconnect readiness during selection.
src/async/imap/MCIMAPAsyncConnection.h	Mirrors the async API change.
src/async/imap/MCIMAPAsyncConnection.cpp	Forwards reconnect-state detection.

Review details

Suppressed comments (2)

src/core/imap/MCIMAPSession.h:319

• The concurrency comment immediately above still names IMAPAsyncConnection::isDisconnected, even though selection now calls needsReconnect() and reads this atomic flag as well. Update the comment in both header copies to document the actual cross-thread predicate.

        std::atomic<bool> mShouldDisconnect;

src/include/MailCore/MCIMAPSession.h:319

• The concurrency comment immediately above still says selection reads mState through IMAPAsyncConnection::isDisconnected, but selection now calls needsReconnect() and also reads mShouldDisconnect. Update that rationale in both header copies so it does not document the old predicate.

        std::atomic<bool> mShouldDisconnect;

• Files reviewed: 8/8 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]

Follow-up from a zero-context review of this branch, landed in b2bb1e2.

The flag had two holes. needsReconnect() is only as good as mShouldDisconnect, and two commands reported ErrorConnection without raising it: noop() and the delimiter LIST inside login(). Every other command in the file arms it on that branch. NOOP is the one that matters for this change — it is what the render pool's keep-alive runs, so a keep-alive meeting a dead socket left the connection pooled, LOGGEDIN, and looking ready to the selection. Both sites now match the rest of the file.

Test. The interrupted-connection test answers LOGIN and its follow-ups now, so the command whose stream dies is the NOOP on a fully logged-in connection rather than the LOGIN itself. It still fails if the selection is put back to asking about the socket alone.

Two findings declined, both pre-existing and outside this change:

• setup() assigns mImap without the lock that interruptCurrentCommand() takes to read it (COR-173). Real, but it is a teardown-race question of its own, not the ranking.
• A session that is STATE_CONNECTED but not logged in ranks as ready. It owes one LOGIN round-trip, not a rebuild, so the tie it wins is the cheap one — deliberate.

[dbezverkhnii]

bugbot run

[cursor]

Stale Bugbot comment from a previous run.

[Copilot]

🟡 Changes recommended

NOOP parse errors can leave a poisoned connection eligible for selection.

Get a fresh assessment by requesting another Copilot review.

Review details

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

[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 86c8f1d. Configure here.}

[Copilot]

🟢 Approval recommended

No unresolved review issues were identified.

Review details

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