From 3298463867ec04ebe75fb38ed1d96044dc794467 Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Mon, 14 Sep 2026 17:08:55 +0300 Subject: [PATCH 1/5] Fix: rank a connection by the reconnect it owes, not by its socket COR-205 The tie-break asked 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 or SELECTED and raises mShouldDisconnect instead, and connectIfNeeded then pays disconnect + connect + login on the next command - strictly more than a closed socket costs. The pool therefore ranked its most expensive connection as its cheapest and, at equal queue length, handed that one to the lease every time; before the tie-break it was at least a coin flip on pool order. The state is a normal one, not an edge case: interruptCurrentCommand raises the flag deliberately and leaves the connection pooled to heal on its next command, which is exactly the connection a lease is likely to meet. needsReconnect() answers the question the selection actually has. isDisconnected() keeps its meaning - the idle timer asks it whether the socket is already gone, and a flagged connection still holds one. Co-Authored-By: Claude Opus 5 --- src/async/imap/MCIMAPAsyncConnection.cpp | 4 ++-- src/async/imap/MCIMAPAsyncConnection.h | 11 +++++------ src/async/imap/MCIMAPAsyncSession.cpp | 8 ++++---- src/core/imap/MCIMAPSession.cpp | 5 +++++ src/core/imap/MCIMAPSession.h | 7 ++++++- src/include/MailCore/MCIMAPAsyncConnection.h | 11 +++++------ src/include/MailCore/MCIMAPSession.h | 7 ++++++- 7 files changed, 33 insertions(+), 20 deletions(-) diff --git a/src/async/imap/MCIMAPAsyncConnection.cpp b/src/async/imap/MCIMAPAsyncConnection.cpp index 9bd7904b8..a3d4a5dd1 100644 --- a/src/async/imap/MCIMAPAsyncConnection.cpp +++ b/src/async/imap/MCIMAPAsyncConnection.cpp @@ -291,9 +291,9 @@ double IMAPAsyncConnection::lastLoginTime() return mSession->lastLoginTime(); } -bool IMAPAsyncConnection::isDisconnected() +bool IMAPAsyncConnection::needsReconnect() { - return mSession->isDisconnected(); + return mSession->needsReconnect(); } unsigned int IMAPAsyncConnection::operationsCount() diff --git a/src/async/imap/MCIMAPAsyncConnection.h b/src/async/imap/MCIMAPAsyncConnection.h index 66a0679af..0b0cad6e6 100644 --- a/src/async/imap/MCIMAPAsyncConnection.h +++ b/src/async/imap/MCIMAPAsyncConnection.h @@ -169,12 +169,11 @@ namespace mailcore { virtual bool isQueueRunning(); virtual void setQueueRunning(bool running); - // Whether this connection's IMAP session was torn down or never established, so that its - // next command connects and logs in first (a connection whose stream failed still reports - // connected until the next command notices). Declared last on purpose: this class is - // exported, and a virtual inserted among the existing ones would shift every vtable slot - // after it. - virtual bool isDisconnected(); + // Whether the next command on this connection has to build it again before it can run - + // never connected, torn down, or left with a stream a failed command marked for teardown + // (see IMAPSession::needsReconnect). Declared last on purpose: this class is exported, and + // a virtual inserted among the existing ones would shift every vtable slot after it. + virtual bool needsReconnect(); }; } diff --git a/src/async/imap/MCIMAPAsyncSession.cpp b/src/async/imap/MCIMAPAsyncSession.cpp index f6ff80a2c..a0b047c6b 100644 --- a/src/async/imap/MCIMAPAsyncSession.cpp +++ b/src/async/imap/MCIMAPAsyncSession.cpp @@ -411,15 +411,15 @@ IMAPAsyncConnection * IMAPAsyncSession::sessionWithMinQueue(bool filterByFolder, { IMAPAsyncConnection * chosenSession = NULL; unsigned int minOperationsCount = 0; - bool chosenSessionConnected = false; + bool chosenSessionReady = false; for (unsigned int i = 0 ; i < mSessions->count() ; i ++) { IMAPAsyncConnection * s = (IMAPAsyncConnection *) mSessions->objectAtIndex(i); // an equally free session that owes a handshake loses to one that does not unsigned int operationsCount = s->operationsCount(); - bool connected = !s->isDisconnected(); + bool ready = !s->needsReconnect(); if ((chosenSession == NULL) || (operationsCount < minOperationsCount) - || ((operationsCount == minOperationsCount) && connected && !chosenSessionConnected)) { + || ((operationsCount == minOperationsCount) && ready && !chosenSessionReady)) { bool matched = includeReserved || !s->isReserved(); if (matched && filterByFolder) { // filter by last selested folder @@ -429,7 +429,7 @@ IMAPAsyncConnection * IMAPAsyncSession::sessionWithMinQueue(bool filterByFolder, if (matched) { chosenSession = s; minOperationsCount = operationsCount; - chosenSessionConnected = connected; + chosenSessionReady = ready; } } } diff --git a/src/core/imap/MCIMAPSession.cpp b/src/core/imap/MCIMAPSession.cpp index 2eee3a795..b0469ec0c 100644 --- a/src/core/imap/MCIMAPSession.cpp +++ b/src/core/imap/MCIMAPSession.cpp @@ -4420,6 +4420,11 @@ bool IMAPSession::isDisconnected() return mState == STATE_DISCONNECTED; } +bool IMAPSession::needsReconnect() +{ + return mState == STATE_DISCONNECTED || mShouldDisconnect; +} + double IMAPSession::lastLoginTime() { LOCK(); diff --git a/src/core/imap/MCIMAPSession.h b/src/core/imap/MCIMAPSession.h index d6ca5f015..8fd268497 100644 --- a/src/core/imap/MCIMAPSession.h +++ b/src/core/imap/MCIMAPSession.h @@ -241,6 +241,11 @@ namespace mailcore { virtual void selectIfNeeded(String * folder, ErrorCode * pError); virtual bool isDisconnected(); + // Whether the next command on this session has to build the connection again - the socket + // is gone, or a failed command left a stream that connectIfNeeded tears down first. Unlike + // isDisconnected(), which answers only for the socket and is what the idle timer asks. + virtual bool needsReconnect(); + // Wall-clock moment (seconds since the epoch, sub-second resolution) of the last // successful LOGIN on this session, 0 when it has never logged in. A client that has to // know whether a pooled connection's mailbox view predates some event of its own @@ -311,7 +316,7 @@ namespace mailcore { MCB_LOCK_TYPE mConnectionLoggerLock; bool mAutomaticConfigurationEnabled; bool mAutomaticConfigurationDone; - bool mShouldDisconnect; + std::atomic mShouldDisconnect; String * mLoginResponse; String * mGmailUserDisplayName; diff --git a/src/include/MailCore/MCIMAPAsyncConnection.h b/src/include/MailCore/MCIMAPAsyncConnection.h index 66a0679af..0b0cad6e6 100644 --- a/src/include/MailCore/MCIMAPAsyncConnection.h +++ b/src/include/MailCore/MCIMAPAsyncConnection.h @@ -169,12 +169,11 @@ namespace mailcore { virtual bool isQueueRunning(); virtual void setQueueRunning(bool running); - // Whether this connection's IMAP session was torn down or never established, so that its - // next command connects and logs in first (a connection whose stream failed still reports - // connected until the next command notices). Declared last on purpose: this class is - // exported, and a virtual inserted among the existing ones would shift every vtable slot - // after it. - virtual bool isDisconnected(); + // Whether the next command on this connection has to build it again before it can run - + // never connected, torn down, or left with a stream a failed command marked for teardown + // (see IMAPSession::needsReconnect). Declared last on purpose: this class is exported, and + // a virtual inserted among the existing ones would shift every vtable slot after it. + virtual bool needsReconnect(); }; } diff --git a/src/include/MailCore/MCIMAPSession.h b/src/include/MailCore/MCIMAPSession.h index d6ca5f015..8fd268497 100644 --- a/src/include/MailCore/MCIMAPSession.h +++ b/src/include/MailCore/MCIMAPSession.h @@ -241,6 +241,11 @@ namespace mailcore { virtual void selectIfNeeded(String * folder, ErrorCode * pError); virtual bool isDisconnected(); + // Whether the next command on this session has to build the connection again - the socket + // is gone, or a failed command left a stream that connectIfNeeded tears down first. Unlike + // isDisconnected(), which answers only for the socket and is what the idle timer asks. + virtual bool needsReconnect(); + // Wall-clock moment (seconds since the epoch, sub-second resolution) of the last // successful LOGIN on this session, 0 when it has never logged in. A client that has to // know whether a pooled connection's mailbox view predates some event of its own @@ -311,7 +316,7 @@ namespace mailcore { MCB_LOCK_TYPE mConnectionLoggerLock; bool mAutomaticConfigurationEnabled; bool mAutomaticConfigurationDone; - bool mShouldDisconnect; + std::atomic mShouldDisconnect; String * mLoginResponse; String * mGmailUserDisplayName; From bcd7583f4a0e68ac5a264514f7b825deae74d144 Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Mon, 14 Sep 2026 17:08:55 +0300 Subject: [PATCH 2/5] Test: a lease skips an interrupted connection COR-205 The one case the socket state alone gets backwards, and the eight tests of the tie-break did not cover it. Co-Authored-By: Claude Opus 5 --- unittest/IMAPConnectionLeaseTests.swift | 39 +++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/unittest/IMAPConnectionLeaseTests.swift b/unittest/IMAPConnectionLeaseTests.swift index 7ac9a2ee4..f998b9b60 100644 --- a/unittest/IMAPConnectionLeaseTests.swift +++ b/unittest/IMAPConnectionLeaseTests.swift @@ -668,6 +668,45 @@ final class IMAPConnectionLeaseTests: XCTestCase { } } + /// An interrupted connection is the case the state alone gets wrong: libetpan never clears a + /// cancelled stream, so the session stays "connected" while its next command has to tear that + /// stream down and build the connection again — strictly more than a closed socket costs. It + /// must lose the tie to a connection that can answer. + func testAcquirePrefersTheLiveConnectionOverAnInterruptedOne() throws { + let endpoint = try LeaseTestTCPEndpoint(greeting: Self.bannerOnlyGreeting) + defer { endpoint.stop() } + + let session = makeSession(port: endpoint.port, maximumConnections: 2) + guard let pool = leaseTwoConnections(session) else { + return + } + + runOffMainThread(timeout: 60) { + for connection in pool { + self.runConnect(session, on: connection) + } + + // Nothing answers the NOOP, so interrupting it is what cancels the stream and leaves + // the connection pooled, connected, and owing a reconnect. + let noop = session.noopOperation() + noop.setConnection(pool[0]) + let finished = self.start(noop) + XCTAssertEqual(finished.wait(timeout: .now() + 2), .timedOut, + "The NOOP was expected to be blocked on the silent socket") + XCTAssertTrue(noop.interruptCurrentCommand()) + XCTAssertEqual(finished.wait(timeout: .now() + 10), .success) + self.releaseAll(session, pool) + + guard let acquired = session.acquireConnection(folder: nil) else { + return XCTFail("With both connections back in the pool a lease must be satisfied") + } + defer { session.releaseConnection(acquired, disconnect: false) } + + XCTAssertEqual(acquired.identity, pool[1].identity, + "A cancelled stream costs more than a closed socket, not less") + } + } + /// Two live idle connections stay interchangeable, and the pick stays the first in the pool. func testTiesAmongLiveConnectionsKeepThePoolOrder() throws { let endpoint = try LeaseTestTCPEndpoint(greeting: Self.bannerOnlyGreeting) From b2bb1e27efff1573c712027c12c9c63e3df6a0c7 Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Mon, 14 Sep 2026 17:24:23 +0300 Subject: [PATCH 3/5] Fix: raise the reconnect flag where a stream error left it unraised COR-205 needsReconnect() is only as good as the flag it reads, and two commands reported ErrorConnection without setting it: NOOP, and the delimiter LIST inside login(). Every other command in the file arms the flag on that branch. NOOP is the one that matters here - it is what the render pool's keep-alive runs, so a keep-alive that met a dead socket left the connection pooled, LOGGEDIN and looking ready to the selection. Co-Authored-By: Claude Opus 5 --- src/core/imap/MCIMAPSession.cpp | 3 +++ src/core/imap/MCIMAPSession.h | 5 +++-- src/include/MailCore/MCIMAPSession.h | 5 +++-- unittest/IMAPConnectionLeaseTests.swift | 16 +++++++++++----- 4 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/core/imap/MCIMAPSession.cpp b/src/core/imap/MCIMAPSession.cpp index b0469ec0c..71eb133ad 100644 --- a/src/core/imap/MCIMAPSession.cpp +++ b/src/core/imap/MCIMAPSession.cpp @@ -1081,6 +1081,8 @@ void IMAPSession::login(ErrorCode * pError) r = mailimap_list(mImap, "", "", &imap_folders); folders = resultsWithError(r, imap_folders, pError); + if (* pError == ErrorConnection || * pError == ErrorParse) + mShouldDisconnect = true; if (* pError != ErrorNone) return; @@ -1380,6 +1382,7 @@ void IMAPSession::noop(ErrorCode * pError) r = mailimap_noop(mImap); if (r == MAILIMAP_ERROR_STREAM) { * pError = ErrorConnection; + mShouldDisconnect = true; } if (r == MAILIMAP_ERROR_NOOP) { * pError = ErrorNoop; diff --git a/src/core/imap/MCIMAPSession.h b/src/core/imap/MCIMAPSession.h index 8fd268497..d9483853f 100644 --- a/src/core/imap/MCIMAPSession.h +++ b/src/core/imap/MCIMAPSession.h @@ -305,8 +305,8 @@ namespace mailcore { unsigned int mLastFetchedSequenceNumber; String * mCurrentFolder; MCB_LOCK_TYPE mIdleLock; - // Written on this session's own thread, read by IMAPAsyncSession's connection - // selection through IMAPAsyncConnection::isDisconnected: atomic so that read is defined. + // Written on this session's own thread, read by IMAPAsyncSession's connection selection + // through IMAPAsyncConnection::needsReconnect: atomic so that read is defined. std::atomic mState; double mLastLoginTime; mailimap * mImap; @@ -316,6 +316,7 @@ namespace mailcore { MCB_LOCK_TYPE mConnectionLoggerLock; bool mAutomaticConfigurationEnabled; bool mAutomaticConfigurationDone; + // Read cross-thread with mState, and for the same reason: see above. std::atomic mShouldDisconnect; String * mLoginResponse; diff --git a/src/include/MailCore/MCIMAPSession.h b/src/include/MailCore/MCIMAPSession.h index 8fd268497..d9483853f 100644 --- a/src/include/MailCore/MCIMAPSession.h +++ b/src/include/MailCore/MCIMAPSession.h @@ -305,8 +305,8 @@ namespace mailcore { unsigned int mLastFetchedSequenceNumber; String * mCurrentFolder; MCB_LOCK_TYPE mIdleLock; - // Written on this session's own thread, read by IMAPAsyncSession's connection - // selection through IMAPAsyncConnection::isDisconnected: atomic so that read is defined. + // Written on this session's own thread, read by IMAPAsyncSession's connection selection + // through IMAPAsyncConnection::needsReconnect: atomic so that read is defined. std::atomic mState; double mLastLoginTime; mailimap * mImap; @@ -316,6 +316,7 @@ namespace mailcore { MCB_LOCK_TYPE mConnectionLoggerLock; bool mAutomaticConfigurationEnabled; bool mAutomaticConfigurationDone; + // Read cross-thread with mState, and for the same reason: see above. std::atomic mShouldDisconnect; String * mLoginResponse; diff --git a/unittest/IMAPConnectionLeaseTests.swift b/unittest/IMAPConnectionLeaseTests.swift index f998b9b60..f4b7d2af9 100644 --- a/unittest/IMAPConnectionLeaseTests.swift +++ b/unittest/IMAPConnectionLeaseTests.swift @@ -668,12 +668,18 @@ final class IMAPConnectionLeaseTests: XCTestCase { } } - /// An interrupted connection is the case the state alone gets wrong: libetpan never clears a - /// cancelled stream, so the session stays "connected" while its next command has to tear that - /// stream down and build the connection again — strictly more than a closed socket costs. It - /// must lose the tie to a connection that can answer. + /// A connection whose stream died under a command is the case the socket state alone gets + /// wrong: libetpan never clears a cancelled stream, so the session stays "connected" while its + /// next command has to tear that stream down and build the connection again — strictly more + /// than a closed socket costs. It must lose the tie to a connection that can answer. func testAcquirePrefersTheLiveConnectionOverAnInterruptedOne() throws { - let endpoint = try LeaseTestTCPEndpoint(greeting: Self.bannerOnlyGreeting) + // LOGIN and what mailcore sends after it are answered, so the command the interrupt cuts is + // the NOOP itself - the connection is fully logged in when its stream dies, which is the + // state this is about. + let endpoint = try LeaseTestTCPEndpoint(greeting: Self.bannerOnlyGreeting, + answers: ["LOGIN": "", + "CAPABILITY": "* CAPABILITY IMAP4rev1\r\n", + "LIST": "* LIST (\\Noselect) \"/\" \"\"\r\n"]) defer { endpoint.stop() } let session = makeSession(port: endpoint.port, maximumConnections: 2) From 5ca92399dd2b23e49696337061cb0986a1bdd80d Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Mon, 14 Sep 2026 17:29:54 +0300 Subject: [PATCH 4/5] Fix: declare needsReconnect last in IMAPSession COR-205 Review of #112: between isDisconnected() and lastLoginTime() it shifted the vtable slot of every virtual after it, which an exported class does not get to do. Co-Authored-By: Claude Opus 5 --- src/core/imap/MCIMAPSession.h | 12 +++++++----- src/include/MailCore/MCIMAPSession.h | 12 +++++++----- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/core/imap/MCIMAPSession.h b/src/core/imap/MCIMAPSession.h index d9483853f..fd9890808 100644 --- a/src/core/imap/MCIMAPSession.h +++ b/src/core/imap/MCIMAPSession.h @@ -241,11 +241,6 @@ namespace mailcore { virtual void selectIfNeeded(String * folder, ErrorCode * pError); virtual bool isDisconnected(); - // Whether the next command on this session has to build the connection again - the socket - // is gone, or a failed command left a stream that connectIfNeeded tears down first. Unlike - // isDisconnected(), which answers only for the socket and is what the idle timer asks. - virtual bool needsReconnect(); - // Wall-clock moment (seconds since the epoch, sub-second resolution) of the last // successful LOGIN on this session, 0 when it has never logged in. A client that has to // know whether a pooled connection's mailbox view predates some event of its own @@ -259,6 +254,13 @@ namespace mailcore { virtual void unlockConnectionLogger(); virtual ConnectionLogger * connectionLoggerNoLock(); + // Whether the next command on this session has to build the connection again - the socket + // is gone, or a failed command left a stream that connectIfNeeded tears down first. Unlike + // isDisconnected(), which answers only for the socket and is what the idle timer asks. + // Declared last: this class is exported, and a virtual inserted among the existing ones + // would shift every vtable slot after it. + virtual bool needsReconnect(); + private: String * mHostname; unsigned int mPort; diff --git a/src/include/MailCore/MCIMAPSession.h b/src/include/MailCore/MCIMAPSession.h index d9483853f..fd9890808 100644 --- a/src/include/MailCore/MCIMAPSession.h +++ b/src/include/MailCore/MCIMAPSession.h @@ -241,11 +241,6 @@ namespace mailcore { virtual void selectIfNeeded(String * folder, ErrorCode * pError); virtual bool isDisconnected(); - // Whether the next command on this session has to build the connection again - the socket - // is gone, or a failed command left a stream that connectIfNeeded tears down first. Unlike - // isDisconnected(), which answers only for the socket and is what the idle timer asks. - virtual bool needsReconnect(); - // Wall-clock moment (seconds since the epoch, sub-second resolution) of the last // successful LOGIN on this session, 0 when it has never logged in. A client that has to // know whether a pooled connection's mailbox view predates some event of its own @@ -259,6 +254,13 @@ namespace mailcore { virtual void unlockConnectionLogger(); virtual ConnectionLogger * connectionLoggerNoLock(); + // Whether the next command on this session has to build the connection again - the socket + // is gone, or a failed command left a stream that connectIfNeeded tears down first. Unlike + // isDisconnected(), which answers only for the socket and is what the idle timer asks. + // Declared last: this class is exported, and a virtual inserted among the existing ones + // would shift every vtable slot after it. + virtual bool needsReconnect(); + private: String * mHostname; unsigned int mPort; From 86c8f1d1004e36e2f17ea5df2c41331db0cc445e Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Mon, 14 Sep 2026 17:38:24 +0300 Subject: [PATCH 5/5] Fix: a NOOP that fails to parse marks the connection for reconnect COR-205 Review of #112: mailimap_noop can also fail to parse, and that branch reported success and left the connection LOGGEDIN with a parser out of step - ready, as far as the selection could tell. Reported and flagged like every other command wrapper in the file now. Co-Authored-By: Claude Opus 5 --- src/core/imap/MCIMAPSession.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/imap/MCIMAPSession.cpp b/src/core/imap/MCIMAPSession.cpp index 71eb133ad..921840c0a 100644 --- a/src/core/imap/MCIMAPSession.cpp +++ b/src/core/imap/MCIMAPSession.cpp @@ -1384,6 +1384,10 @@ void IMAPSession::noop(ErrorCode * pError) * pError = ErrorConnection; mShouldDisconnect = true; } + if (r == MAILIMAP_ERROR_PARSE) { + * pError = ErrorParse; + mShouldDisconnect = true; + } if (r == MAILIMAP_ERROR_NOOP) { * pError = ErrorNoop; }