From 02358ee210116fa0e7d85457d548fcd56a0a9298 Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Tue, 15 Sep 2026 19:47:15 +0300 Subject: [PATCH 1/9] Fix: a connection-scoped disconnect retains the pool that owns it COR-216 IMAPAsyncConnection::disconnectOperation() retained the connection but not its owner, unlike every operation the owner's factories create. The queue thread's retain of the owner is released in queueStoppedRunning() before stoppedOnMainThread() restarts the thread for an operation queued while it was quitting, so a queued disconnect could restart it under a freed IMAPAsyncSession - the queueStartRunning() crash group (43 events in iOS 3.19.26, 9 in 3.19.29). The test reproduces it under AddressSanitizer without the retain. Co-Authored-By: Claude Fable 5.1 --- Package.swift | 2 +- src/async/imap/MCIMAPAsyncConnection.cpp | 4 + .../IMAPConnectionOwnerLifetimeTests.swift | 90 +++++++++++++++++++ 3 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 unittest/IMAPConnectionOwnerLifetimeTests.swift diff --git a/Package.swift b/Package.swift index 03dec004b..5bf6d2d20 100644 --- a/Package.swift +++ b/Package.swift @@ -265,7 +265,7 @@ var targets: [Target] = [ "unittest.cpp", "unittest.mm" ], - sources: ["CertificateUtilsTests.swift", "IMAPConnectionLeaseTests.swift", "IMAPIdleCancellationTests.swift", "IMAPInterruptCurrentCommandTests.swift", "LeaseTestTCPEndpoint.swift", "LibetpanHelperTests.swift", "unittest.swift"], + sources: ["CertificateUtilsTests.swift", "IMAPConnectionLeaseTests.swift", "IMAPConnectionOwnerLifetimeTests.swift", "IMAPIdleCancellationTests.swift", "IMAPInterruptCurrentCommandTests.swift", "LeaseTestTCPEndpoint.swift", "LibetpanHelperTests.swift", "unittest.swift"], resources: [ .copy("data") ] diff --git a/src/async/imap/MCIMAPAsyncConnection.cpp b/src/async/imap/MCIMAPAsyncConnection.cpp index a3d4a5dd1..8104c024a 100644 --- a/src/async/imap/MCIMAPAsyncConnection.cpp +++ b/src/async/imap/MCIMAPAsyncConnection.cpp @@ -277,6 +277,10 @@ IMAPOperation * IMAPAsyncConnection::disconnectOperation() { IMAPDisconnectOperation * op = new IMAPDisconnectOperation(); op->setSession(this); + // Retains the owner like every operation the owner creates: this connection reaches it through + // a raw pointer on each queue start and stop, and the queue's own retain is released before + // a queued operation restarts it. + op->setMainSession(mOwner); op->autorelease(); return op; } diff --git a/unittest/IMAPConnectionOwnerLifetimeTests.swift b/unittest/IMAPConnectionOwnerLifetimeTests.swift new file mode 100644 index 000000000..9327d36c4 --- /dev/null +++ b/unittest/IMAPConnectionOwnerLifetimeTests.swift @@ -0,0 +1,90 @@ +// +// IMAPConnectionOwnerLifetimeTests.swift +// mailcore2 +// +// A pooled connection must not outlive its IMAPAsyncSession while it still has work: the +// connection reaches its owner through a raw pointer on every queue start and stop. +// + +#if canImport(Darwin) + +import Dispatch +import Foundation +import XCTest + +#if SWIFT_PACKAGE +import CMailCore +#endif + +@testable import MailCore + +final class IMAPConnectionOwnerLifetimeTests: XCTestCase { + + private func makeSession() -> MCOIMAPSession { + let session = MCOIMAPSession() + session.hostname = "127.0.0.1" + session.port = 1 // never connected: a disconnect on a fresh connection is a no-op + session.connectionType = ConnectionTypeClear + session.username = "user" + session.password = "password" + session.maximumConnections = 1 + return session + } + + private func runOffMainThread(timeout: TimeInterval, _ body: @escaping () -> Void) { + let finished = expectation(description: "test body") + DispatchQueue.global(qos: .userInitiated).async { + body() + finished.fulfill() + } + waitForExpectations(timeout: timeout) + } + + private func start(_ operation: MCOIMAPOperation) -> DispatchSemaphore { + let finished = DispatchSemaphore(value: 0) + operation.start { _ in + finished.signal() + } + return finished + } + + /// A connection-scoped disconnect is the one operation that does not retain the session it + /// belongs to. Queue one, let every other reference to the session go, wait for the + /// connection's queue thread to stop - its stop releases the session's last retain - and then + /// start the queued operation: the queue restarts and reports to an owner that no longer exists. + func testQueuedDisconnectKeepsTheOwnerAlive() { + var session: MCOIMAPSession? = makeSession() + var handle = session!.acquireConnection(folder: nil) + XCTAssertNotNil(handle) + + // Retains the connection (through the operation), not the session. + let later = handle!.disconnectOperation() + + runOffMainThread(timeout: 30) { + // Wakes the connection's queue thread; when it stops it releases its retain of the + // session, which is the last one once the Swift wrappers below are gone. + XCTAssertEqual(self.start(handle!.disconnectOperation()).wait(timeout: .now() + 5), .success) + + session!.releaseConnection(handle!, disconnect: false) + + // The queue thread lingers for about a second after its last operation and releases + // its retain of the session when it stops; only then are the wrappers below the last + // holders. + let deadline = Date(timeIntervalSinceNow: 10) + while session!.isOperationQueueRunning && Date() < deadline { + usleep(20_000) + } + XCTAssertFalse(session!.isOperationQueueRunning, "the connection's queue was expected to stop") + handle = nil + // The dropped handle returns its lease on the session's queue (the main queue here) + // and holds the session until then; let that run before the session is dropped. + DispatchQueue.main.sync {} + session = nil + + XCTAssertEqual(self.start(later).wait(timeout: .now() + 5), .success, + "The queued disconnect must still run once the pool has been dropped") + } + } +} + +#endif From 5113816ed5c4855dd371e6ebbc90c4e2335b156f Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Tue, 15 Sep 2026 19:47:16 +0300 Subject: [PATCH 2/9] Fix: let the cut command raise the reconnect flag itself COR-211 interruptCurrentCommand() raised mShouldDisconnect from the interrupting thread (5f27dfd2). Raised while login() is between two of its commands, the flag is met by the connectIfNeeded() inside identity(), whose result login() ignores: the connection is torn down and rebuilt - or not - under a caller that then reports success. That is how iOS 3.19.29 got its mailimap_expunge, mailimap_namespace and search_modseq crashes on a NULL mailimap (COR-211, COR-212, COR-213). The line is redundant since COR-205: every command wrapper raises the flag on the stream error the cut command fails with, at a point its caller checks. Leaving the flag down when an interrupt lands with no command on the wire costs one failed command before the reconnect. Co-Authored-By: Claude Fable 5.1 --- src/async/imap/MCIMAPOperation.h | 7 ++++--- src/core/imap/MCIMAPSession.cpp | 9 ++++----- src/include/MailCore/MCIMAPOperation.h | 7 ++++--- src/swift/imap/IMAPBaseOperation.swift | 6 +++--- unittest/IMAPConnectionLeaseTests.swift | 4 ++-- 5 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/async/imap/MCIMAPOperation.h b/src/async/imap/MCIMAPOperation.h index a62dee913..42f7c5c74 100644 --- a/src/async/imap/MCIMAPOperation.h +++ b/src/async/imap/MCIMAPOperation.h @@ -50,9 +50,10 @@ namespace mailcore { the operations queued behind it (a disconnect, most importantly) run immediately. Does nothing when the operation is not the one running. - Teardown of this connection only - it is left unusable and reconnects on next use, so call - it for a command that is being abandoned (cancelled, or given up on), never to hurry up a - command whose result still matters. + Teardown of this connection only - the cut command fails with a connection error and the + connection is rebuilt before the one after - so call it for a command that is being + abandoned (cancelled, or given up on), never to hurry up a command whose result still + matters. Returns whether this operation was the one the queue was running at that moment. That may include a command that finished just as the interrupt landed: its result is intact, but the stream is cancelled all the same. */ diff --git a/src/core/imap/MCIMAPSession.cpp b/src/core/imap/MCIMAPSession.cpp index 921840c0a..b0b290a27 100644 --- a/src/core/imap/MCIMAPSession.cpp +++ b/src/core/imap/MCIMAPSession.cpp @@ -3747,11 +3747,10 @@ void IMAPSession::interruptCurrentCommand() LOCK(); if (mImap != NULL && mImap->imap_stream != NULL) { mailstream_cancel(mImap->imap_stream); - // libetpan never clears a stream's cancelled state: every read and write on it fails from - // here on. The next command must reconnect, and connectIfNeeded does that for this flag, - // so the connection stays pooled and heals on its own instead of relying on the caller - // to tear it down. - mShouldDisconnect = true; + // Deliberately not raising mShouldDisconnect here: the command this cuts fails with a + // stream error and raises it itself, at a point its caller checks. Raised from this thread + // it can land between two commands of one login(), where the nested connectIfNeeded() + // would rebuild the connection under a caller that ignores the result. } UNLOCK(); } diff --git a/src/include/MailCore/MCIMAPOperation.h b/src/include/MailCore/MCIMAPOperation.h index a62dee913..42f7c5c74 100644 --- a/src/include/MailCore/MCIMAPOperation.h +++ b/src/include/MailCore/MCIMAPOperation.h @@ -50,9 +50,10 @@ namespace mailcore { the operations queued behind it (a disconnect, most importantly) run immediately. Does nothing when the operation is not the one running. - Teardown of this connection only - it is left unusable and reconnects on next use, so call - it for a command that is being abandoned (cancelled, or given up on), never to hurry up a - command whose result still matters. + Teardown of this connection only - the cut command fails with a connection error and the + connection is rebuilt before the one after - so call it for a command that is being + abandoned (cancelled, or given up on), never to hurry up a command whose result still + matters. Returns whether this operation was the one the queue was running at that moment. That may include a command that finished just as the interrupt landed: its result is intact, but the stream is cancelled all the same. */ diff --git a/src/swift/imap/IMAPBaseOperation.swift b/src/swift/imap/IMAPBaseOperation.swift index d8905829a..e0afac414 100644 --- a/src/swift/imap/IMAPBaseOperation.swift +++ b/src/swift/imap/IMAPBaseOperation.swift @@ -34,9 +34,9 @@ public class MCOIMAPBaseOperation : MCOOperation { the one running. Unlike cancel(), which only raises a flag mailcore checks before starting an operation, this - reaches the command already in flight. It costs the connection: the stream stays cancelled and - is rebuilt on next use, so call it for a command being abandoned, never to hurry up one whose - result still matters. + reaches the command already in flight. It costs the connection: the cut command fails with a + connection error and the connection is rebuilt before the one after, so call it for a command + being abandoned, never to hurry up one whose result still matters. - Returns: whether this operation was the one the queue was running at that moment. That may include a command that finished just as the interrupt landed: its result is intact, but the diff --git a/unittest/IMAPConnectionLeaseTests.swift b/unittest/IMAPConnectionLeaseTests.swift index f4b7d2af9..b6caaff03 100644 --- a/unittest/IMAPConnectionLeaseTests.swift +++ b/unittest/IMAPConnectionLeaseTests.swift @@ -136,8 +136,8 @@ final class IMAPConnectionLeaseTests: XCTestCase { func testInterruptedConnectionReconnectsOnItsNextCommand() throws { // LOGIN and what mailcore sends right after it (CAPABILITY, the delimiter LIST) are - // answered, so that the command the interrupt cuts is the NOOP itself: a stream error - // inside any of those already schedules a reconnect on its own, one inside NOOP does not. + // answered, so that the command the interrupt cuts is the NOOP itself - a plain command + // outside login(), whose stream error is what has to schedule the reconnect. let endpoint = try LeaseTestTCPEndpoint(greeting: "* OK [CAPABILITY IMAP4rev1] LeaseTestTCPEndpoint ready\r\n", answers: ["LOGIN": "", "CAPABILITY": "* CAPABILITY IMAP4rev1\r\n", From f198be6a6c110b6f9406678618f11409f615d788 Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Tue, 15 Sep 2026 19:47:17 +0300 Subject: [PATCH 3/9] Fix: fail a login that identity() left not logged in COR-211 login() ignores identity()'s error, and identity() starts with connectIfNeeded(): with mShouldDisconnect raised meanwhile it tears the connection down and rebuilds it, and login() still returned ErrorNone - with the session CONNECTED but not authenticated, or with no mailimap at all when the rebuild failed. selectIfNeeded() then skips SELECT for such a state and the command runs on a NULL mailimap (COR-211, COR-212, COR-213). login() now requires STATE_LOGGEDIN before reporting success. The test raises the flag while NAMESPACE is on the wire, through a scheduleReconnect() hook on the connection that sets the flag without cutting the stream; declared last in the exported classes to keep their vtable layout. Co-Authored-By: Claude Fable 5.1 --- Package.swift | 2 +- src/async/imap/MCIMAPAsyncConnection.cpp | 5 ++ src/async/imap/MCIMAPAsyncConnection.h | 3 + src/c/imap/CIMAPAsyncConnection.cpp | 1 + src/c/imap/CIMAPAsyncConnection.h | 1 + src/core/imap/MCIMAPSession.cpp | 14 +++ src/core/imap/MCIMAPSession.h | 6 ++ src/include/MailCore/CIMAPAsyncConnection.h | 1 + src/include/MailCore/MCIMAPAsyncConnection.h | 3 + src/include/MailCore/MCIMAPSession.h | 6 ++ src/swift/imap/IMAPAsyncConnection.swift | 7 ++ unittest/IMAPLoginTests.swift | 93 ++++++++++++++++++++ unittest/LeaseTestTCPEndpoint.swift | 11 ++- 13 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 unittest/IMAPLoginTests.swift diff --git a/Package.swift b/Package.swift index 5bf6d2d20..f107c34c8 100644 --- a/Package.swift +++ b/Package.swift @@ -265,7 +265,7 @@ var targets: [Target] = [ "unittest.cpp", "unittest.mm" ], - sources: ["CertificateUtilsTests.swift", "IMAPConnectionLeaseTests.swift", "IMAPConnectionOwnerLifetimeTests.swift", "IMAPIdleCancellationTests.swift", "IMAPInterruptCurrentCommandTests.swift", "LeaseTestTCPEndpoint.swift", "LibetpanHelperTests.swift", "unittest.swift"], + sources: ["CertificateUtilsTests.swift", "IMAPConnectionLeaseTests.swift", "IMAPConnectionOwnerLifetimeTests.swift", "IMAPIdleCancellationTests.swift", "IMAPInterruptCurrentCommandTests.swift", "IMAPLoginTests.swift", "LeaseTestTCPEndpoint.swift", "LibetpanHelperTests.swift", "unittest.swift"], resources: [ .copy("data") ] diff --git a/src/async/imap/MCIMAPAsyncConnection.cpp b/src/async/imap/MCIMAPAsyncConnection.cpp index 8104c024a..d415d63cb 100644 --- a/src/async/imap/MCIMAPAsyncConnection.cpp +++ b/src/async/imap/MCIMAPAsyncConnection.cpp @@ -300,6 +300,11 @@ bool IMAPAsyncConnection::needsReconnect() return mSession->needsReconnect(); } +void IMAPAsyncConnection::scheduleReconnect() +{ + mSession->scheduleReconnect(); +} + unsigned int IMAPAsyncConnection::operationsCount() { return mQueue->count(); diff --git a/src/async/imap/MCIMAPAsyncConnection.h b/src/async/imap/MCIMAPAsyncConnection.h index 0b0cad6e6..9822bc421 100644 --- a/src/async/imap/MCIMAPAsyncConnection.h +++ b/src/async/imap/MCIMAPAsyncConnection.h @@ -174,6 +174,9 @@ namespace mailcore { // (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(); + // Makes the next command on this connection rebuild it first; same contract as + // IMAPSession::scheduleReconnect(). Declared last, like needsReconnect(). + virtual void scheduleReconnect(); }; } diff --git a/src/c/imap/CIMAPAsyncConnection.cpp b/src/c/imap/CIMAPAsyncConnection.cpp index 39c3a554c..9761fa55a 100644 --- a/src/c/imap/CIMAPAsyncConnection.cpp +++ b/src/c/imap/CIMAPAsyncConnection.cpp @@ -16,3 +16,4 @@ C_SYNTHESIZE_FUNC_WITH_SCALAR(unsigned int, leaseGeneration) C_SYNTHESIZE_FUNC_WITH_SCALAR(unsigned int, operationsCount) C_SYNTHESIZE_FUNC_WITH_SCALAR(double, lastLoginTime) C_SYNTHESIZE_FUNC_WITH_OBJ(CIMAPBaseOperation, disconnectOperation) +C_SYNTHESIZE_FUNC_WITH_VOID(scheduleReconnect) diff --git a/src/c/imap/CIMAPAsyncConnection.h b/src/c/imap/CIMAPAsyncConnection.h index d253fb482..1381e0e4f 100644 --- a/src/c/imap/CIMAPAsyncConnection.h +++ b/src/c/imap/CIMAPAsyncConnection.h @@ -25,6 +25,7 @@ extern "C" { C_SYNTHESIZE_READONLY_PROPERTY_DEFINITION(CIMAPAsyncConnection, double, lastLoginTime) C_SYNTHESIZE_FUNC_DEFINITION(CIMAPAsyncConnection, CIMAPBaseOperation, disconnectOperation) + C_SYNTHESIZE_FUNC_DEFINITION(CIMAPAsyncConnection, void, scheduleReconnect) #ifdef __cplusplus } diff --git a/src/core/imap/MCIMAPSession.cpp b/src/core/imap/MCIMAPSession.cpp index b0b290a27..07e2d13cd 100644 --- a/src/core/imap/MCIMAPSession.cpp +++ b/src/core/imap/MCIMAPSession.cpp @@ -1116,6 +1116,15 @@ void IMAPSession::login(ErrorCode * pError) else { // TODO: namespace should be shared with other sessions for non automatic namespace. } + + // identity() starts with connectIfNeeded(): with mShouldDisconnect raised meanwhile it tears the + // connection down and rebuilds it, and its result is ignored above. A rebuilt connection is not + // logged in - and a rebuild that failed has no mImap at all - so this must not read as a + // successful login. + if (mState != STATE_LOGGEDIN) { + * pError = ErrorConnection; + return; + } mAutomaticConfigurationDone = true; @@ -4431,6 +4440,11 @@ bool IMAPSession::needsReconnect() return mState == STATE_DISCONNECTED || mShouldDisconnect; } +void IMAPSession::scheduleReconnect() +{ + mShouldDisconnect = true; +} + double IMAPSession::lastLoginTime() { LOCK(); diff --git a/src/core/imap/MCIMAPSession.h b/src/core/imap/MCIMAPSession.h index fd9890808..4d90adc4b 100644 --- a/src/core/imap/MCIMAPSession.h +++ b/src/core/imap/MCIMAPSession.h @@ -260,6 +260,11 @@ namespace mailcore { // Declared last: this class is exported, and a virtual inserted among the existing ones // would shift every vtable slot after it. virtual bool needsReconnect(); + // Makes the next command rebuild the connection first, as a failed command would have. + // Any thread, but between operations only: raised while one runs, it is met by the next + // connectIfNeeded() inside that operation - login() then fails with ErrorConnection. + // Declared last, for the same reason as needsReconnect(). + virtual void scheduleReconnect(); private: String * mHostname; @@ -309,6 +314,7 @@ namespace mailcore { MCB_LOCK_TYPE mIdleLock; // Written on this session's own thread, read by IMAPAsyncSession's connection selection // through IMAPAsyncConnection::needsReconnect: atomic so that read is defined. + // mShouldDisconnect has one more writer, scheduleReconnect(), on any thread. std::atomic mState; double mLastLoginTime; mailimap * mImap; diff --git a/src/include/MailCore/CIMAPAsyncConnection.h b/src/include/MailCore/CIMAPAsyncConnection.h index d253fb482..1381e0e4f 100644 --- a/src/include/MailCore/CIMAPAsyncConnection.h +++ b/src/include/MailCore/CIMAPAsyncConnection.h @@ -25,6 +25,7 @@ extern "C" { C_SYNTHESIZE_READONLY_PROPERTY_DEFINITION(CIMAPAsyncConnection, double, lastLoginTime) C_SYNTHESIZE_FUNC_DEFINITION(CIMAPAsyncConnection, CIMAPBaseOperation, disconnectOperation) + C_SYNTHESIZE_FUNC_DEFINITION(CIMAPAsyncConnection, void, scheduleReconnect) #ifdef __cplusplus } diff --git a/src/include/MailCore/MCIMAPAsyncConnection.h b/src/include/MailCore/MCIMAPAsyncConnection.h index 0b0cad6e6..9822bc421 100644 --- a/src/include/MailCore/MCIMAPAsyncConnection.h +++ b/src/include/MailCore/MCIMAPAsyncConnection.h @@ -174,6 +174,9 @@ namespace mailcore { // (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(); + // Makes the next command on this connection rebuild it first; same contract as + // IMAPSession::scheduleReconnect(). Declared last, like needsReconnect(). + virtual void scheduleReconnect(); }; } diff --git a/src/include/MailCore/MCIMAPSession.h b/src/include/MailCore/MCIMAPSession.h index fd9890808..4d90adc4b 100644 --- a/src/include/MailCore/MCIMAPSession.h +++ b/src/include/MailCore/MCIMAPSession.h @@ -260,6 +260,11 @@ namespace mailcore { // Declared last: this class is exported, and a virtual inserted among the existing ones // would shift every vtable slot after it. virtual bool needsReconnect(); + // Makes the next command rebuild the connection first, as a failed command would have. + // Any thread, but between operations only: raised while one runs, it is met by the next + // connectIfNeeded() inside that operation - login() then fails with ErrorConnection. + // Declared last, for the same reason as needsReconnect(). + virtual void scheduleReconnect(); private: String * mHostname; @@ -309,6 +314,7 @@ namespace mailcore { MCB_LOCK_TYPE mIdleLock; // Written on this session's own thread, read by IMAPAsyncSession's connection selection // through IMAPAsyncConnection::needsReconnect: atomic so that read is defined. + // mShouldDisconnect has one more writer, scheduleReconnect(), on any thread. std::atomic mState; double mLastLoginTime; mailimap * mImap; diff --git a/src/swift/imap/IMAPAsyncConnection.swift b/src/swift/imap/IMAPAsyncConnection.swift index 72aede8bf..4dbe7b178 100644 --- a/src/swift/imap/IMAPAsyncConnection.swift +++ b/src/swift/imap/IMAPAsyncConnection.swift @@ -46,6 +46,13 @@ public class MCOIMAPAsyncConnection: NSObjectCompat { connection.release() } + /// Makes the next command on this connection rebuild it first, the way a failed command would + /// have. For tests: raises the flag from outside the connection's thread without cutting the + /// stream, so a command in flight completes and the flag is met by the next one. + internal func scheduleReconnect() { + connection.scheduleReconnect() + } + internal var isReserved: Bool { return connection.isReserved } diff --git a/unittest/IMAPLoginTests.swift b/unittest/IMAPLoginTests.swift new file mode 100644 index 000000000..17861cc12 --- /dev/null +++ b/unittest/IMAPLoginTests.swift @@ -0,0 +1,93 @@ +// +// IMAPLoginTests.swift +// mailcore2 +// +// login() must not report success on a session that is not logged in. +// + +#if canImport(Darwin) + +import Dispatch +import Foundation +import XCTest + +#if SWIFT_PACKAGE +import CMailCore +#endif + +@testable import MailCore + +final class IMAPLoginTests: XCTestCase { + + private final class ConnectionSlot: @unchecked Sendable { + private let lock = NSLock() + private var value: MCOIMAPAsyncConnection? + var connection: MCOIMAPAsyncConnection? { + get { lock.withLock { value } } + set { lock.withLock { value = newValue } } + } + } + + private func runOffMainThread(timeout: TimeInterval, _ body: @escaping () -> Void) { + let finished = expectation(description: "test body") + DispatchQueue.global(qos: .userInitiated).async { + body() + finished.fulfill() + } + waitForExpectations(timeout: timeout) + } + + /// The reconnect flag raised while login() is between its commands - an interrupt from + /// another thread does that - is met by identity(), the one step of login() whose result is + /// ignored. It tears the connection down and rebuilds it, and the rebuilt connection is not + /// logged in. login() must report that instead of success: a caller told "logged in" runs its + /// command on a session that is not, or - when the rebuild failed - on no mailimap at all. + func testLoginFailsWhenItsIdentityStepRebuildsTheConnection() throws { + // The endpoint's reader thread raises the flag on a connection the test thread leases + // after the endpoint exists; the slot makes that hand-over defined. + let slot = ConnectionSlot() + // The flag lands while NAMESPACE is on the wire: after fetchNamespace()'s own + // connectIfNeeded() has run, before identity()'s does. + let endpoint = try LeaseTestTCPEndpoint(greeting: "* OK [CAPABILITY IMAP4rev1 ID NAMESPACE] LeaseTestTCPEndpoint ready\r\n", + answers: ["LOGIN": "", + "CAPABILITY": "* CAPABILITY IMAP4rev1 ID NAMESPACE\r\n", + "NAMESPACE": "* NAMESPACE ((\"\" \"/\")) NIL NIL\r\n", + "ID": "* ID NIL\r\n"], + beforeAnswering: ["NAMESPACE": { slot.connection?.scheduleReconnect() }]) + defer { endpoint.stop() } + + let session = MCOIMAPSession() + session.hostname = "127.0.0.1" + session.port = UInt32(endpoint.port) + session.connectionType = ConnectionTypeClear + session.username = "user" + session.password = "password" + session.timeout = 60 + session.maximumConnections = 1 + + guard let leased = session.acquireConnection(folder: nil) else { + return XCTFail("An empty pool with room for 1 connection must satisfy the lease") + } + slot.connection = leased + + runOffMainThread(timeout: 30) { + let expunge = session.expungeOperation(folder: "INBOX") + expunge.setConnection(leased) + var error: Error? + let finished = DispatchSemaphore(value: 0) + expunge.start { opError in + error = opError + finished.signal() + } + XCTAssertEqual(finished.wait(timeout: .now() + 10), .success) + + XCTAssertEqual(endpoint.acceptedClientCount, 2, "identity() was expected to rebuild the connection") + XCTAssertEqual((error as NSError?)?.code, Int(MailCoreError.errorConnection.rawValue), + "A login that ends on a rebuilt, unauthenticated connection must fail as a connection error, got \(String(describing: error))") + + session.releaseConnection(leased, disconnect: false) + } + } +} + +#endif diff --git a/unittest/LeaseTestTCPEndpoint.swift b/unittest/LeaseTestTCPEndpoint.swift index 5d6dda444..cbf96a26c 100644 --- a/unittest/LeaseTestTCPEndpoint.swift +++ b/unittest/LeaseTestTCPEndpoint.swift @@ -26,6 +26,7 @@ final class LeaseTestTCPEndpoint { private let listeningSocket: Int32 private let greeting: String? private let answers: [String: String] + private let beforeAnswering: [String: () -> Void] private let acceptQueue = DispatchQueue(label: "LeaseTestTCPEndpoint.accept") private let lock = NSLock() private var acceptedSockets: [Int32] = [] @@ -38,10 +39,14 @@ final class LeaseTestTCPEndpoint { /// Pass an IMAP banner (e.g. "* OK [CAPABILITY IMAP4rev1] ready\r\n") to let connects finish; /// pass nil to stay silent so that every command blocks. `answers` maps a command name /// (LOGIN, LIST, ...) to the untagged lines to send before its tagged OK, so a test can walk - /// the client to a chosen state; any command not listed still blocks. - init(greeting: String? = nil, answers: [String: String] = [:]) throws { + /// the client to a chosen state; any command not listed still blocks. `beforeAnswering` runs + /// a block once a listed command has arrived and before it is answered - while the client is + /// blocked in that command's read, which is the one moment a test can act on it from another + /// thread with a known position in the client's command sequence. + init(greeting: String? = nil, answers: [String: String] = [:], beforeAnswering: [String: () -> Void] = [:]) throws { self.greeting = greeting self.answers = answers + self.beforeAnswering = beforeAnswering // Everything below works on a local descriptor: a closure that touched `listeningSocket` // would capture self before `port` is initialized. @@ -131,6 +136,7 @@ final class LeaseTestTCPEndpoint { // descriptor; stop() only shuts the socket down, which wakes recv(), and the close // happens here. let answers = self.answers + let beforeAnswering = self.beforeAnswering DispatchQueue.global().async { [weak self] in var buffer = [UInt8](repeating: 0, count: 1024) var pending = "" @@ -150,6 +156,7 @@ final class LeaseTestTCPEndpoint { guard words.count >= 2, let untagged = answers[words[1].uppercased()] else { continue } + beforeAnswering[words[1].uppercased()]?() let reply = Array((untagged + "\(words[0]) OK \(words[1]) completed\r\n").utf8) _ = reply.withUnsafeBufferPointer { send(accepted, $0.baseAddress!, $0.count, 0) } } From 97288a7317bc025829d085aa16355a87c8974bc5 Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Tue, 15 Sep 2026 19:47:17 +0300 Subject: [PATCH 4/9] Fix: return a dropped lease on the session's queue COR-216 MCOIMAPAsyncConnection.deinit released its lease inline, on whatever thread dropped the handle, and a release with disconnect starts an operation. Every operation start on a session has to come from its dispatch queue: OperationQueue::startThread() and IMAPAsyncConnection::runOperation() guard their state by that queue alone, and a start from elsewhere can run two queue threads on one connection or over-release the owner. The release now hops to the session's queue; the lease is back in the pool once that queue has run. Co-Authored-By: Claude Fable 5.1 --- src/swift/imap/IMAPAsyncConnection.swift | 18 +++++++++++++----- src/swift/imap/IMAPSession.swift | 6 +++++- unittest/IMAPConnectionLeaseTests.swift | 10 +++++++++- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/swift/imap/IMAPAsyncConnection.swift b/src/swift/imap/IMAPAsyncConnection.swift index 4dbe7b178..017bcf2d9 100644 --- a/src/swift/imap/IMAPAsyncConnection.swift +++ b/src/swift/imap/IMAPAsyncConnection.swift @@ -38,12 +38,20 @@ public class MCOIMAPAsyncConnection: NSObjectCompat { /// Returns the lease if its holder never did. A leaked lease is permanent otherwise — nothing /// in the pool clears a reservation on its own — and the connection would be lost to the pool /// for the life of the session. Torn down rather than pooled: a holder that lost track of its - /// lease cannot have left the connection in a state anybody should inherit. Best effort: - /// deinit runs on whatever thread drops the last reference, outside the serialisation the - /// release contract asks for; a holder that releases explicitly never gets here. + /// lease cannot have left the connection in a state anybody should inherit. deinit runs on + /// whatever thread drops the last reference, so the release hops to the session's queue: a + /// release starts the teardown operation, and every operation start on a session must come + /// from that queue (see MCOIMAPSession.acquireConnection). The lease is therefore back in the + /// pool only once that queue has run, not the instant the handle dies. A holder that releases + /// explicitly never gets here. deinit { - session.releaseConnection(self, disconnect: true) - connection.release() + let session = self.session + let connection = self.connection + let leaseGeneration = self.leaseGeneration + (session.dispatchQueue ?? DispatchQueue.main).async { + session.releaseConnection(connection, leaseGeneration: leaseGeneration, disconnect: true) + connection.release() + } } /// Makes the next command on this connection rebuild it first, the way a failed command would diff --git a/src/swift/imap/IMAPSession.swift b/src/swift/imap/IMAPSession.swift index eaf77fe6f..bc24c8085 100644 --- a/src/swift/imap/IMAPSession.swift +++ b/src/swift/imap/IMAPSession.swift @@ -194,8 +194,12 @@ public class MCOIMAPSession: NSObjectCompat { contract as acquireConnection(folder:). */ public func releaseConnection(_ connection: MCOIMAPAsyncConnection, disconnect: Bool) { + releaseConnection(connection.connection, leaseGeneration: connection.leaseGeneration, disconnect: disconnect) + } + + internal func releaseConnection(_ connection: CIMAPAsyncConnection, leaseGeneration: UInt32, disconnect: Bool) { mailCoreAutoreleasePool { - session.releaseConnection(connection.connection, connection.leaseGeneration, disconnect) + session.releaseConnection(connection, leaseGeneration, disconnect) } } diff --git a/unittest/IMAPConnectionLeaseTests.swift b/unittest/IMAPConnectionLeaseTests.swift index b6caaff03..8254bab20 100644 --- a/unittest/IMAPConnectionLeaseTests.swift +++ b/unittest/IMAPConnectionLeaseTests.swift @@ -434,7 +434,15 @@ final class IMAPConnectionLeaseTests: XCTestCase { XCTAssertNil(session.acquireConnection(folder: nil), "the only connection is leased") } - guard let reacquired = session.acquireConnection(folder: nil) else { + // The dropped handle returns its lease on the session's queue - the main queue here - + // so the pool frees up once that queue has run. + var reacquired: MCOIMAPAsyncConnection? + let deadline = Date(timeIntervalSinceNow: 5) + while reacquired == nil && Date() < deadline { + RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.02)) + reacquired = session.acquireConnection(folder: nil) + } + guard let reacquired = reacquired else { return XCTFail("A dropped handle must have returned its lease to the pool") } session.releaseConnection(reacquired, disconnect: false) From eecee81a4bc90128917810a0a4148399636643a5 Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Tue, 15 Sep 2026 19:59:35 +0300 Subject: [PATCH 5/9] Test: import Darwin explicitly for usleep COR-216 Foundation re-exports it on Apple platforms, so the file compiled, but the sibling tests spell the dependency out and this one now does too. Co-Authored-By: Claude Fable 5.1 --- unittest/IMAPConnectionOwnerLifetimeTests.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/unittest/IMAPConnectionOwnerLifetimeTests.swift b/unittest/IMAPConnectionOwnerLifetimeTests.swift index 9327d36c4..378b440e3 100644 --- a/unittest/IMAPConnectionOwnerLifetimeTests.swift +++ b/unittest/IMAPConnectionOwnerLifetimeTests.swift @@ -8,6 +8,7 @@ #if canImport(Darwin) +import Darwin import Dispatch import Foundation import XCTest From 2d1de87018d82918d5cc45b71b02ce8989308ce2 Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Tue, 15 Sep 2026 20:21:36 +0300 Subject: [PATCH 6/9] Dev: qualify the interrupt docs for a cut with no command in flight COR-211 The wording promised a rebuild before the next command unconditionally; with the reconnect flag raised by the failing command rather than by the interrupt, an interrupt that lands between commands costs that next command first. Co-Authored-By: Claude Fable 5.1 --- src/async/imap/MCIMAPOperation.h | 5 +++-- src/include/MailCore/MCIMAPOperation.h | 5 +++-- src/swift/imap/IMAPBaseOperation.swift | 5 +++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/async/imap/MCIMAPOperation.h b/src/async/imap/MCIMAPOperation.h index 42f7c5c74..1cb416a43 100644 --- a/src/async/imap/MCIMAPOperation.h +++ b/src/async/imap/MCIMAPOperation.h @@ -51,8 +51,9 @@ namespace mailcore { Does nothing when the operation is not the one running. Teardown of this connection only - the cut command fails with a connection error and the - connection is rebuilt before the one after - so call it for a command that is being - abandoned (cancelled, or given up on), never to hurry up a command whose result still + connection is rebuilt before the one after; landing with no command on the wire, it is the + next command that fails and the one after that rebuilds - so call it for a command that is + being abandoned (cancelled, or given up on), never to hurry up a command whose result still matters. Returns whether this operation was the one the queue was running at that moment. That may include a command that finished just as the interrupt landed: its result is intact, but diff --git a/src/include/MailCore/MCIMAPOperation.h b/src/include/MailCore/MCIMAPOperation.h index 42f7c5c74..1cb416a43 100644 --- a/src/include/MailCore/MCIMAPOperation.h +++ b/src/include/MailCore/MCIMAPOperation.h @@ -51,8 +51,9 @@ namespace mailcore { Does nothing when the operation is not the one running. Teardown of this connection only - the cut command fails with a connection error and the - connection is rebuilt before the one after - so call it for a command that is being - abandoned (cancelled, or given up on), never to hurry up a command whose result still + connection is rebuilt before the one after; landing with no command on the wire, it is the + next command that fails and the one after that rebuilds - so call it for a command that is + being abandoned (cancelled, or given up on), never to hurry up a command whose result still matters. Returns whether this operation was the one the queue was running at that moment. That may include a command that finished just as the interrupt landed: its result is intact, but diff --git a/src/swift/imap/IMAPBaseOperation.swift b/src/swift/imap/IMAPBaseOperation.swift index e0afac414..cdfa760d5 100644 --- a/src/swift/imap/IMAPBaseOperation.swift +++ b/src/swift/imap/IMAPBaseOperation.swift @@ -35,8 +35,9 @@ public class MCOIMAPBaseOperation : MCOOperation { Unlike cancel(), which only raises a flag mailcore checks before starting an operation, this reaches the command already in flight. It costs the connection: the cut command fails with a - connection error and the connection is rebuilt before the one after, so call it for a command - being abandoned, never to hurry up one whose result still matters. + connection error and the connection is rebuilt before the one after (landing with no command on + the wire, it is the next command that fails and the one after that rebuilds), so call it for a + command being abandoned, never to hurry up one whose result still matters. - Returns: whether this operation was the one the queue was running at that moment. That may include a command that finished just as the interrupt landed: its result is intact, but the From a2eb1dca26010f47bc21ff54ed2d5d046869594a Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Tue, 15 Sep 2026 20:32:52 +0300 Subject: [PATCH 7/9] Dev: import Dispatch where deinit now dispatches COR-216 Co-Authored-By: Claude Fable 5.1 --- src/swift/imap/IMAPAsyncConnection.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/src/swift/imap/IMAPAsyncConnection.swift b/src/swift/imap/IMAPAsyncConnection.swift index 017bcf2d9..c0d1cbb7f 100644 --- a/src/swift/imap/IMAPAsyncConnection.swift +++ b/src/swift/imap/IMAPAsyncConnection.swift @@ -1,3 +1,4 @@ +import Dispatch import Foundation import CMailCore From 5ea366abc7260a082282f2638b54911d9128e615 Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Tue, 15 Sep 2026 20:51:14 +0300 Subject: [PATCH 8/9] Fix: fail a login whose ignored ID left the stream marked for teardown COR-211 A stream or parse error inside identity() keeps the state at LOGGEDIN but raises mShouldDisconnect; the next command would fail on it anyway, so login() reports the connection error itself instead of a success on a stream that has to be rebuilt. Co-Authored-By: Claude Fable 5.1 --- src/core/imap/MCIMAPSession.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/imap/MCIMAPSession.cpp b/src/core/imap/MCIMAPSession.cpp index 07e2d13cd..95c056fd0 100644 --- a/src/core/imap/MCIMAPSession.cpp +++ b/src/core/imap/MCIMAPSession.cpp @@ -1120,8 +1120,8 @@ void IMAPSession::login(ErrorCode * pError) // identity() starts with connectIfNeeded(): with mShouldDisconnect raised meanwhile it tears the // connection down and rebuilds it, and its result is ignored above. A rebuilt connection is not // logged in - and a rebuild that failed has no mImap at all - so this must not read as a - // successful login. - if (mState != STATE_LOGGEDIN) { + // successful login; nor may one whose ignored ID left the stream marked for teardown. + if (mState != STATE_LOGGEDIN || mShouldDisconnect) { * pError = ErrorConnection; return; } From 35710cca2c12ca7f1ac5774cc03dc478f4c3e1c6 Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Tue, 15 Sep 2026 20:51:15 +0300 Subject: [PATCH 9/9] Test: keep IMAPLoginTests within the package's Swift baseline COR-211 Explicit lock()/unlock() instead of withLock, and no Sendable conformance: the slot is only ever handed to a non-Sendable closure. Co-Authored-By: Claude Fable 5.1 --- unittest/IMAPLoginTests.swift | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/unittest/IMAPLoginTests.swift b/unittest/IMAPLoginTests.swift index 17861cc12..638d24a30 100644 --- a/unittest/IMAPLoginTests.swift +++ b/unittest/IMAPLoginTests.swift @@ -19,12 +19,20 @@ import CMailCore final class IMAPLoginTests: XCTestCase { - private final class ConnectionSlot: @unchecked Sendable { + private final class ConnectionSlot { private let lock = NSLock() private var value: MCOIMAPAsyncConnection? var connection: MCOIMAPAsyncConnection? { - get { lock.withLock { value } } - set { lock.withLock { value = newValue } } + get { + lock.lock() + defer { lock.unlock() } + return value + } + set { + lock.lock() + value = newValue + lock.unlock() + } } }