COR-217 - Reconnect a cut connection at the next operation boundary

[dbezverkhnii]

Follow-up to #113. There the interrupt stopped raising the reconnect flag itself and left that to the command failing on the cancelled stream. A cut that meets no read in flight — between two commands of one operation, or after its last command — fails nothing: needsReconnect() reports the connection ready, sessionWithMinQueue may rank it above a live one, and its next command pays one ErrorConnection before the reconnect.

Change. interruptCurrentCommand() remembers every cut in an atomic mStreamCancelled (cleared in unsetup() with the stream). needsReconnect() reports it, so the pool ranks a cut connection as needing a rebuild at once. IMAPOperation::beforeMain() — which runs synchronously before main() while the connection's queue thread waits, so nothing of that connection is on the wire — turns it into mShouldDisconnect; the operation's outermost connectIfNeeded() then rebuilds before the first command. The flag is still never raised at a point a nested connectIfNeeded() inside a running login() could consume it. login()'s postcondition also fails on a cut between its own commands.

Why not read mStreamCancelled in connectIfNeeded() directly: same result for the target case, but a cut between two of login()'s commands would then rebuild mid-login and be reported by the postcondition one socket later.

Test. testCutWithNothingOnTheWireReconnectsBeforeTheNextOperation: connect, an answered NOOP, cancelStream() (new connection-level hook — the operation-level interrupt can only cut a command in flight), a second NOOP; asserts it succeeds and the endpoint saw a second client. Without the beforeMain conversion it fails with ErrorConnection on one client. 31 IMAP tests green, 29 under ASan.

scheduleReconnectIfInterrupted() and cancelStream() are appended last in their exported classes; the C++ change needs a new Windows prebuilt before the check goes green.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes cross-thread IMAP reconnect signaling and operation startup timing in core connection pooling; logic is narrow but mistakes could cause spurious reconnects or mid-login teardown.

Overview
Fixes a gap where interrupting an idle IMAP connection (stream cut with no read in flight) left the session looking healthy until the next command failed with ErrorConnection.

interruptCurrentCommand() now records every cut in atomic mStreamCancelled (set before mailstream_cancel(), cleared in unsetup()). needsReconnect() treats that flag like a pending teardown so the pool does not prefer a silently broken connection.

At the next operation boundary, IMAPOperation::beforeMain() calls scheduleReconnectIfInterrupted(), which promotes mStreamCancelled to mShouldDisconnect so the operation’s outer connectIfNeeded() rebuilds before the first command—without raising disconnect during a multi-step login() (the reason reconnect is not set directly inside the interrupt path).

Adds connection-level cancelStream() (C/Swift) for tests that need a cut while nothing is running on the wire; docs for interruptCurrentCommand() are updated to match. New lease test testCutWithNothingOnTheWireReconnectsBeforeTheNextOperation covers connect → NOOP → cancelStream() → NOOP succeeding with a second TCP accept.

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

Resolve the operation-entry race, add deterministic login regression coverage, and publish the required Windows prebuilt.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR makes IMAP stream cancellations trigger reconnection at the next operation boundary and strengthens login validation.

Changes:

• Tracks cancelled streams and schedules reconnects before subsequent operations.
• Adds login postcondition checks for interrupted streams.
• Adds C/Swift cancellation hooks, documentation updates, and regression coverage.

File summaries

File	Summary
unittest/IMAPConnectionLeaseTests.swift	Adds idle-cut reconnect regression coverage.
src/swift/imap/IMAPBaseOperation.swift	Updates interruption documentation.
src/swift/imap/IMAPAsyncConnection.swift	Adds the Swift cancellation hook.
src/include/MailCore/MCIMAPSession.h	Declares reconnect tracking state and APIs.
src/include/MailCore/MCIMAPOperation.h	Updates interruption documentation.
src/include/MailCore/MCIMAPAsyncConnection.h	Declares the cancellation API.
src/include/MailCore/CIMAPAsyncConnection.h	Exposes the C cancellation function.
src/core/imap/MCIMAPSession.h	Adds internal reconnect state declarations.
src/core/imap/MCIMAPSession.cpp	Tracks cancellations and validates login state.
src/c/imap/CIMAPAsyncConnection.h	Mirrors the C API declaration.
src/c/imap/CIMAPAsyncConnection.cpp	Implements the C forwarding function.
src/async/imap/MCIMAPOperation.h	Updates operation interruption documentation.
src/async/imap/MCIMAPOperation.cpp	Schedules reconnects before operation execution.
src/async/imap/MCIMAPAsyncConnection.h	Declares the C++ cancellation hook.
src/async/imap/MCIMAPAsyncConnection.cpp	Forwards cancellation to the IMAP session.

Review details

Suppressed comments (4)

src/async/imap/MCIMAPAsyncConnection.h:182

• interruptCurrentCommand() only verifies that the operation is still published as mRunningOperation; it can race just after a command completes, as the queue's existing contract documents. It is therefore not limited to an in-flight read, so this explanation should distinguish the deterministic no-operation idle cut provided by cancelStream().

        // Cuts this connection's stream whatever is running on it - IMAPSession::interruptCurrentCommand()
        // without the queue's check that an operation is running. For tests, which need a cut that
        // lands with nothing on the wire. Declared last, like needsReconnect().

src/core/imap/MCIMAPSession.cpp:430

• These src changes alter the content digest used for the Windows prebuilt, so the mailcore2 - Windows prebuilt check will remain red until mailcore2-windows-<digest>.zip is built and uploaded from this committed revision. Please publish the matching archive before merging.

    mStreamCancelled = false;

src/include/MailCore/MCIMAPAsyncConnection.h:182

• interruptCurrentCommand() is not limited to a command blocked in a read: OperationQueue::interruptRunningOperation() only checks mRunningOperation, and its existing contract allows the interrupt to race just after the command completed. This description is therefore misleading; describe cancelStream() as the hook that cuts without requiring a particular operation to be running, which is the deterministic idle case this test needs.

        // Cuts this connection's stream whatever is running on it - IMAPSession::interruptCurrentCommand()
        // without the queue's check that an operation is running. For tests, which need a cut that
        // lands with nothing on the wire. Declared last, like needsReconnect().

src/swift/imap/IMAPAsyncConnection.swift:67

• The operation-level interrupt is not strictly limited to an in-flight read: its queue check only requires the operation to be mRunningOperation, so it can race just after a command completes. Please describe this hook as the deterministic way to cut an idle connection without a particular operation, rather than claiming the other API cannot produce a no-read cut.

    /// Cuts this connection's stream whatever is running on it. For tests: the interrupt that lands
    /// with nothing on the wire, which MCOIMAPBaseOperation.interruptCurrentCommand() cannot
    /// produce - it only cuts a command that is in flight.

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

🔵 Needs a closer look

A moderate cancellation-state publication race remains unresolved; the interruption documentation also needs a minor correction.

Review details

Suppressed comments (2)

src/core/imap/MCIMAPSession.cpp:3767

• Publish mStreamCancelled before calling mailstream_cancel. With the current order, an idle connection can be canceled and then observed by sessionWithMinQueue/a newly queued operation during the gap before the atomic store at line 3767; beforeMain() can then miss the marker and the operation uses the canceled stream, paying the ErrorConnection this change is meant to avoid. Storing the marker first is safe because the connection lock prevents teardown from racing the cancellation call.

        // would rebuild the connection under a caller that ignores the result. Every cut is
        // remembered in mStreamCancelled: one that met no read in flight failed no command, and
        // gets its reconnect at the next operation boundary (scheduleReconnectIfInterrupted).
        mStreamCancelled = true;

src/swift/imap/IMAPAsyncConnection.swift:67

• interruptCurrentCommand() is not limited to a command that is in flight: OperationQueue only checks mRunningOperation, which remains set for the whole main() and can span gaps between its internal commands. This hook is specifically needed for the idle gap after the operation has completed; please narrow the comment so it does not describe the operation-level interrupt's behavior incorrectly.

    /// Cuts this connection's stream whatever is running on it. For tests: the interrupt that lands
    /// with nothing on the wire, which MCOIMAPBaseOperation.interruptCurrentCommand() cannot
    /// produce - it only cuts a command that is in flight.

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• 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 239dbfd. Configure here.}

[Copilot]

🔵 Needs a closer look

The regression test does not verify live-connection selection, and the cancellation documentation needs correction.

Review details

Suppressed comments (5)

src/async/imap/MCIMAPOperation.h:56

• The revised contract is too broad for the queue timing: an interrupt can still arrive after beforeMain() and before the first command, so mStreamCancelled is remembered but not promoted until a later operation and that first command can return ErrorConnection. The text currently says a cut with no read in flight “fails nothing”; qualify that statement to cover this accepted boundary race.

         Teardown of this connection only - a command in flight fails with a connection error, a
         cut that met none fails nothing, and either way the connection is rebuilt before the next
         operation's first command - 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/core/imap/MCIMAPSession.cpp:656

• unsetup() now clears mStreamCancelled, so the public interruptCurrentCommand() contract that says the cancelled state is “never reset” and the session remains unusable is no longer true. This can mislead callers about whether the connection is automatically reusable after the new reconnect boundary; update the corresponding API documentation to describe the reset during teardown (in both public/core headers).

    mStreamCancelled = false;

src/core/imap/MCIMAPSession.cpp:4446

• The new mStreamCancelled readiness path is the part intended to affect sessionWithMinQueue, but the added regression pins the next operation to the cut connection and therefore never exercises that selection. The existing liveness test covers mShouldDisconnect after an interrupted command, not an idle cut with mShouldDisconnect == false; please add an unpinned/acquire scenario with a live peer that asserts the live connection is selected.

    return mState == STATE_DISCONNECTED || mShouldDisconnect || mStreamCancelled;

src/include/MailCore/MCIMAPOperation.h:56

• The revised contract is too broad for the queue timing: an interrupt can still arrive after beforeMain() and before the first command, so mStreamCancelled is remembered but not promoted until a later operation and that first command can return ErrorConnection. The text currently says a cut with no read in flight “fails nothing”; qualify that statement to cover this accepted boundary race.

         Teardown of this connection only - a command in flight fails with a connection error, a
         cut that met none fails nothing, and either way the connection is rebuilt before the next
         operation's first command - 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:40

• The revised contract is too broad for the queue timing: an interrupt can still arrive after beforeMain() and before the first command, so mStreamCancelled is remembered but not promoted until a later operation and that first command can return ErrorConnection. The text currently says a cut with no read in flight “fails nothing”; qualify that statement (and the duplicated C++ documentation) to cover this accepted boundary race.

     reaches the command already in flight. It costs the connection: a command in flight fails with
     a connection error, a cut that met none fails nothing, and either way the connection is rebuilt
     before the next operation's first command - so call it for a command being abandoned, never to
     hurry up one whose result still matters.

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