COR-218 - A NOOP on a cold connection connects first

src/core/imap/MCIMAPSession.cpp

@@ -654,6 +654,9 @@ void IMAPSession::unsetup()
     mImap = NULL;
     mIdleEnabled = false;
     mStreamCancelled = false;
+    // Published with mImap, not after the stream is closed and freed: needsReconnect() reads the
+    // state from another thread and would otherwise call this session logged in meanwhile.
+    mState = STATE_DISCONNECTED;
     UNLOCK();
 
     if (imap != NULL) {
@@ -664,8 +667,6 @@ void IMAPSession::unsetup()
         mailimap_free(imap);
         imap = NULL;
     }
-
-    mState = STATE_DISCONNECTED;
 }
 
 void IMAPSession::connect(ErrorCode * pError)
@@ -1381,9 +1382,6 @@ void IMAPSession::noop(ErrorCode * pError)
 {
     int r;
 
-    if (mImap == NULL)
-        return;
-
     MCLog("connect");
     loginIfNeeded(pError);
     if (* pError != ErrorNone) {

unittest/IMAPConnectionLeaseTests.swift

@@ -180,6 +180,51 @@ final class IMAPConnectionLeaseTests: XCTestCase {
         }
     }
 
+    /// A NOOP is the probe the client sends to learn whether a connection is up and its
+    /// credentials still good, and the command its keep-alive uses to hold a pooled connection
+    /// warm. Both are pointless on a connection that reports success without connecting - never
+    /// connected, or torn down behind the keep-alive.
+    func testNoopConnectsAColdConnection() throws {
+        let endpoint = try LeaseTestTCPEndpoint(greeting: "* OK [CAPABILITY IMAP4rev1] LeaseTestTCPEndpoint ready\r\n",
+                                                answers: ["LOGIN": "",
+                                                          "CAPABILITY": "* CAPABILITY IMAP4rev1\r\n",
+                                                          "LIST": "* LIST (\\Noselect) \"/\" \"\"\r\n",
+                                                          "NOOP": ""])
+        defer { endpoint.stop() }
+
+        let session = makeSession(port: endpoint.port, maximumConnections: 1)
+
+        guard let leased = session.acquireConnection(folder: nil) else {
+            return XCTFail("An empty pool with room for 1 connection must satisfy the lease")
+        }
+
+        func noop() -> Error? {
+            let noop = session.noopOperation()
+            noop.setConnection(leased)
+            var error: Error?
+            let finished = DispatchSemaphore(value: 0)
+            noop.start { opError in
+                error = opError
+                finished.signal()
+            }
+            XCTAssertEqual(finished.wait(timeout: .now() + 5), .success)
+            return error
+        }
+
+        runOffMainThread(timeout: 30) {
+            XCTAssertNil(noop())
+            XCTAssertEqual(endpoint.acceptedClientCount, 1, "A NOOP on a never-connected connection must connect and log in")
+
+            XCTAssertEqual(self.start(leased.disconnectOperation()).wait(timeout: .now() + 5), .success)
+            XCTAssertTrue(endpoint.waitForClientDisconnect(timeout: 5))
+
+            XCTAssertNil(noop())
+            XCTAssertEqual(endpoint.acceptedClientCount, 2, "A NOOP on a torn-down connection must reconnect rather than report success")
+
+            session.releaseConnection(leased, disconnect: false)
+        }
+    }
+
     func testCutWithNothingOnTheWireReconnectsBeforeTheNextOperation() throws {
         // Every command is answered, so the NOOPs complete and the cut below meets no read in
         // flight - the case an interrupt of a running command cannot produce.
@@ -197,10 +242,6 @@ final class IMAPConnectionLeaseTests: XCTestCase {
         }
 
         runOffMainThread(timeout: 30) {
-            // A NOOP on a never-connected connection is a no-op, so connect first.
-            let connect = session.connectOperation()
-            connect.setConnection(leased)
-            XCTAssertEqual(self.start(connect).wait(timeout: .now() + 5), .success)
             let first = session.noopOperation()
             first.setConnection(leased)
             XCTAssertEqual(self.start(first).wait(timeout: .now() + 5), .success)

unittest/IMAPInterruptCurrentCommandTests.swift

@@ -237,8 +237,16 @@ final class IMAPInterruptCurrentCommandTests: XCTestCase {
 
             XCTAssertEqual(runningFinished.wait(timeout: .now() + 10), .success,
                            "interruptCurrentCommand() did not unblock the running command")
+
+            // The queued NOOP now runs and, its connection torn down by the interrupt, reconnects to
+            // the silent endpoint and blocks in turn - so it is the running operation now, and the
+            // same interrupt frees it.
+            XCTAssertEqual(queuedFinished.wait(timeout: .now() + 2), .timedOut,
+                           "The queued NOOP was expected to reconnect and block on the silent socket")
+            XCTAssertTrue(queued.interruptCurrentCommand(),
+                          "The queued operation was expected to be the running one by now")
             XCTAssertEqual(queuedFinished.wait(timeout: .now() + 10), .success,
-                           "The queued operation was expected to finish once the connection was freed")
+                           "interruptCurrentCommand() did not unblock the queued operation")
         }
     }
 }