COR-211, COR-212, COR-213, COR-216 - Fix IMAP login and connection lifetime crashes

[dbezverkhnii]

Spark iOS 3.19.29 (first build on 2.1.55) got three new crash groups, each on the connection's queue thread at libetpan's first dereference of a NULL mailimap*: mailimap_expunge, mailimap_namespace (inside IMAPSession::login), search_modseq. A fourth, older group spiked with it: IMAPOperationQueueCallback::queueStartRunning() on a freed IMAPAsyncSession.

Why it happened. login() had no postcondition. identity() is the one step whose error it ignores, and identity() begins with connectIfNeeded(): a mShouldDisconnect raised meanwhile makes it tear the connection down and rebuild it (or fail to), and login() still reported success; selectIfNeeded() then skips SELECT for a session that is CONNECTED or DISCONNECTED and the command runs on a NULL mailimap. 5f27dfd (2.1.55) made interruptCurrentCommand() raise that flag asynchronously from the interrupting thread, which turned a rare conjunction (an ignored PARSE error in COMPRESS, no NAMESPACE, a failed reconnect) into a per-interrupt race against the client's 2-second grace interrupt.

What changes, one commit each:

• disconnectOperation() retains the owner like every other operation — the queue's own retain of the owner is released in queueStoppedRunning() right before startThread() restarts for a queued operation.
• The flag raise in interruptCurrentCommand() is removed. It is redundant since COR-205 — every command wrapper raises the flag on the stream error the cut command fails with — and it broke the invariant that the flag is written by the session's thread together with an error its caller checks. Cost: an interrupt landing with no command on the wire leaves the flag down, so the next command fails once before the reconnect.
• login() requires STATE_LOGGEDIN before reporting success; otherwise ErrorConnection, which the client retries. This closes the pre-2.1.55 route too.
• MCOIMAPAsyncConnection.deinit returns its lease on the session's dispatch queue instead of inline: a release with disconnect starts an operation, and that was the only start not coming from the session's queue.

Tests. IMAPConnectionOwnerLifetimeTests reproduces the owner use-after-free deterministically (ASan without the retain). IMAPLoginTests raises the flag while NAMESPACE is on the wire through a new scheduleReconnect() hook — the flag without the stream cut — and asserts ErrorConnection (without the postcondition: ErrorExpunge on an unauthenticated connection). Both were run failing-then-passing. xcrun swift test --sanitize=address over the IMAP suites is green; the 43 failures in the legacy unittest are pre-existing.

Alternative considered. Keeping the flag raise and skipping identity() when it is set would preserve needsReconnect() for an interrupt landing after an operation's last command, at the price of keeping an asynchronous writer of the flag. The postcondition stays as the backstop either way; the raise can be reintroduced on the operation boundary later if that window matters in practice.

🤖 Generated with Claude Code

---

Note

High Risk
Changes core IMAP login, interrupt/reconnect, and async session lifetime semantics that previously caused NULL mailimap and freed-owner crashes in production.

Overview
Fixes IMAP crashes from reporting login success on an unauthenticated or torn-down connection, use-after-free when a queued disconnect outlives the session, and unsafe lease release off the session queue.

login() now fails with ErrorConnection unless the session ends in STATE_LOGGEDIN without mShouldDisconnect—covering cases where identity()’s ignored connectIfNeeded() rebuilt the socket mid-login.

interruptCurrentCommand() no longer sets mShouldDisconnect from the interrupt thread (avoids racing login()’s multi-step flow); reconnect is driven by the cut command’s stream error, with docs noting the extra fail-then-rebuild edge when nothing is on the wire. scheduleReconnect() is exposed (C/Swift/tests) to raise the flag between commands without cancelling the stream.

disconnectOperation() retains the async session owner like other operations so a queued disconnect still runs after the pool drops the last session reference. MCOIMAPAsyncConnection.deinit asyncs lease release onto the session dispatch queue. Tests add IMAPConnectionOwnerLifetimeTests, IMAPLoginTests, and LeaseTestTCPEndpoint.beforeAnswering.

^{Reviewed by Cursor Bugbot for commit 35710cc. 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

The owner-lifetime test is missing its required Darwin import, and three documentation comments need clarification.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes IMAP login false-success crashes and asynchronous connection owner lifetime issues.

Changes:

• Validates login state and removes interrupt-thread reconnect flag writes.
• Retains owners for queued disconnects and queues lease cleanup.
• Adds reconnect hooks and regression tests.

File summaries

File	Summary
unittest/LeaseTestTCPEndpoint.swift	Adds command synchronization hooks.
unittest/IMAPLoginTests.swift	Tests login failure after reconnect.
unittest/IMAPConnectionOwnerLifetimeTests.swift	Tests queued disconnect owner lifetime; requires an explicit Darwin import for usleep.
unittest/IMAPConnectionLeaseTests.swift	Adjusts asynchronous lease expectations.
src/swift/imap/IMAPSession.swift	Adds internal lease-release support.
src/swift/imap/IMAPBaseOperation.swift	Updates interrupt behavior documentation; should qualify the no-command-on-wire exception.
src/swift/imap/IMAPAsyncConnection.swift	Queues deinitialization cleanup and adds reconnect support.
src/include/MailCore/MCIMAPSession.h	Declares reconnect scheduling.
src/include/MailCore/MCIMAPOperation.h	Updates interrupt documentation; should qualify the reconnect exception.
src/include/MailCore/MCIMAPAsyncConnection.h	Declares connection reconnect support.
src/include/MailCore/CIMAPAsyncConnection.h	Exposes reconnect scheduling through C bindings.
src/core/imap/MCIMAPSession.h	Adds reconnect API and state documentation.
src/core/imap/MCIMAPSession.cpp	Validates login state and changes interrupt handling.
src/c/imap/CIMAPAsyncConnection.h	Updates C wrapper declarations.
src/c/imap/CIMAPAsyncConnection.cpp	Implements the C reconnect wrapper.
src/async/imap/MCIMAPOperation.h	Updates interrupt documentation; should qualify the reconnect exception.
src/async/imap/MCIMAPAsyncConnection.h	Declares asynchronous reconnect support.
src/async/imap/MCIMAPAsyncConnection.cpp	Retains owners for disconnect operations.
Package.swift	Registers the new tests.

Review details

Suppressed comments (3)

src/async/imap/MCIMAPOperation.h:56

• The implementation intentionally leaves mShouldDisconnect unset when the interrupt lands after the command has finished; in that case the cancelled stream is used by the next command, which fails once before a later reconnect. This new header wording promises an unconditional rebuild before the next command and contradicts the documented race/result case below; qualify it with that exception.

         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.

src/include/MailCore/MCIMAPOperation.h:56

• The implementation intentionally leaves mShouldDisconnect unset when the interrupt lands after the command has finished; in that case the cancelled stream is used by the next command, which fails once before a later reconnect. This new public-header wording promises an unconditional rebuild before the next command and contradicts the documented race/result case below; qualify it with that exception.

         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.

src/swift/imap/IMAPBaseOperation.swift:39

• The implementation intentionally leaves mShouldDisconnect unset when the interrupt lands after the command has finished; in that case the cancelled stream is used by the next command, which fails once before a later reconnect. This wording promises an unconditional rebuild before the next command and contradicts the documented race/result case below; qualify it with that exception.

     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.

• Files reviewed: 19/19 changed files
• Comments generated: 1
• 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]

bugbot run

[cursor]

Stale Bugbot comment from a previous run.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings include a missing Swift import, a required Windows artifact, and reconnect/teardown race concerns.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/core/imap/MCIMAPSession.cpp:4445

• scheduleReconnect() is documented and used from another thread, but connectIfNeeded() clears mShouldDisconnect only after the potentially long disconnect(). If another schedule arrives during that teardown, the later unconditional clear erases it, so the following command can run without honoring the new request. Clear the flag with an atomic exchange(false) before teardown (or otherwise preserve concurrent writes) so a schedule during disconnect remains pending.

    mShouldDisconnect = true;

src/swift/imap/IMAPAsyncConnection.swift:52

• This teardown is queued on the session's current dispatch queue, but existing connections copy the session queue only when they are created (src/async/imap/MCIMAPAsyncSession.cpp:298-300); changing MCOIMAPSession.dispatchQueue later does not update the connection's operation queue (src/async/imap/MCIMAPAsyncSession.cpp:1008-1017). If the queue is changed while this lease is alive, deinit can start the disconnect from a different queue than the connection's queued callbacks/timer, racing another start/stop. Capture the connection's queue for this lease (or update existing connections when the session queue changes) before hopping the release.

        (session.dispatchQueue ?? DispatchQueue.main).async {
            session.releaseConnection(connection, leaseGeneration: leaseGeneration, disconnect: true)

• Files reviewed: 19/19 changed files
• Comments generated: 2
• Review effort level: Lite

[dbezverkhnii]

bugbot run

[cursor]

Stale Bugbot comment from a previous run.

[Copilot]

🔵 Needs a closer look

Unresolved findings remain in login correctness, Swift 5.4 test compatibility, and queue serialization across high-risk IMAP threading and lifetime changes.

Review details

Suppressed comments (2)

src/include/MailCore/MCIMAPSession.h:265

• The new contract says scheduleReconnect() is “between operations only”, but the implementation and IMAPLoginTests intentionally call it while NAMESPACE is on the wire and rely on the next connectIfNeeded() in that same login operation. Please clarify this public/source-duplicate documentation to state whether cross-thread calls during an operation are supported; as written, callers cannot reconcile the documented restriction with the tested behavior.

        // 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.

src/swift/imap/IMAPAsyncConnection.swift:55

• This hops to the session's current dispatch queue, but existing connections keep the queue they were created with (IMAPAsyncSession::session() copies mDispatchQueue into the connection and later session queue changes do not update them). If dispatchQueue is changed after this lease is acquired, deinit starts releaseConnection on a different queue from the connection's operation queue, violating the acquire/release serialization contract and racing queue state. Capture/use the connection's queue (or explicitly make the session queue immutable after connections are created).

        (session.dispatchQueue ?? DispatchQueue.main).async {
            session.releaseConnection(connection, leaseGeneration: leaseGeneration, disconnect: true)
            connection.release()
        }

• Files reviewed: 19/19 changed files
• Comments generated: 3
• 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 35710cc. Configure here.}

[Copilot]

🟡 Changes recommended

Two critical issues remain around login failure propagation and queue-correct lease cleanup.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 19/19 changed files
• Comments generated: 2
• Review effort level: Lite