COR-223, COR-224 - Report a cut that did not happen, and name the lease queue

[dbezverkhnii]

Two independent small fixes in the same layer, one commit each.

COR-223 — the cancel fallback lies when it cuts nothing

IMAPSession::interruptCurrentCommand() cancels the stream only when mImap != NULL && mImap->imap_stream != NULL. libetpan assigns imap_stream at the top of mailimap_connect, so everything before that — DNS, mail_tcp_connect_timeout, and on an implicit-TLS session the handshake — runs with no stream this call can reach. OperationQueue::interruptRunningOperation() nevertheless returned true, because it raised its flag purely on mRunningOperation == op.

Measured on a throwaway probe against 192.0.2.1 (TEST-NET-1, SYNs dropped), before the change:

COR223-DIAG interruptCurrentCommand: mImap=0x85f4ec000 imap_stream=0x0 -> NO-OP
COR223-DIAG after 2s: still blocked
COR223-DIAG interruptCurrentCommand() returned true
COR223-DIAG 8s after the interrupt: STILL BLOCKED (total 10.0s)

After it, the same probe reports false and the operation is — unchanged — still blocked.

interrupt() now returns whether it broke anything and the queue passes that up. Operation::interrupt(), IMAPOperation::interrupt() and IMAPSession::interruptCurrentCommand() change from void to bool; no virtual is added, removed or reordered, so no vtable slot moves. The whole interrupt chain is a fork addition — upstream/master has none of these three methods — so this costs nothing at the pending upstream merge. IMAPOperation is the only override of interrupt() in the repository, and IMAPAsyncConnection::interruptCurrentCommand() is the only caller of interruptRunningOperation().

This does not shorten the hang. A connect blocked in connect(2) is still not interruptible; making it so means changing mail_tcp_connect_timeout in the libetpan fork, which is out of scope here. The operation still runs to the session timeout — the caller is simply no longer told otherwise. Downstream, Spark reads the result only to decide whether to write a trace line, so behaviour there is unchanged and the log stops lying.

No test is added. Every existing assertion passes against the old code too: LeaseTestTCPEndpoint and SilentTCPEndpoint both accept the TCP connection, so imap_stream is always assigned before the client blocks. Covering the new false needs a black-holed destination, which is environment-dependent and slow; the probe above was thrown away rather than committed. Declining that deliberately rather than silently.

One thing I did not verify: once the stream exists, whether libetpan's TLS handshake inside mailimap_socket_starttls() actually observes mailstream_cancel(). If it does not, a true there is the same shape of dishonesty one phase later — same libetpan work to fix, not addressed here.

COR-224 — documentation only

The header and IMAPSession.swift told callers to serialize acquireConnection / releaseConnection with every start() "on a queue of your choosing". That is wrong: acquireConnection appends to mSessions on the caller's thread while operationRunningStateChanged() walks it on the session's dispatch queue, with no lock. Confirmed by printing pthread_self() at both sites — two distinct threads, interleaved, with an append landing between two walks.

MCOIMAPAsyncConnection.deinit already documented the strict rule, hops to that queue, and cross-referenced the text that contradicted it. No executable change.

Verification

• 33 IMAP tests green on macOS, including testRepeatedCancelDuringIdleDoesNotHangOrCrash from COR-221.
• Full macOS suite: the only failures are the 43 pre-existing unittest.testSummary locale/date-format mismatches, which are green in CI.
• Mirrored headers under src/include/MailCore/ regenerated with configure-headers.sh and checked byte-identical to their sources.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changing virtual interrupt() / interruptCurrentCommand() return types is an ABI/API break for any external subclass, though the tree only overrides IMAPOperation::interrupt(); interrupt logic touches connection teardown and queue locking.

Overview
Interrupt reporting (COR-223): The operation interrupt path now returns bool instead of void, so callers can tell when nothing was actually aborted. IMAPSession::interruptCurrentCommand() returns false when there is no libetpan stream yet (connect blocked in DNS/TCP/TLS) or after teardown; OperationQueue::interruptRunningOperation() propagates that instead of reporting success whenever the target operation was merely the running one. Docs/Swift comments for interruptCurrentCommand() are aligned with that semantics. This does not make pre-stream connects interruptible—only the reported outcome changes.

Lease threading docs (COR-224): acquireConnection / releaseConnection documentation now requires serializing with every start() on the session’s own dispatch queue, because pool bookkeeping (e.g. operationRunningStateChanged()) runs there—not on an arbitrary caller queue. Documentation-only; no runtime behavior change.

^{Reviewed by Cursor Bugbot for commit d8b121c. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Copilot]

🟡 Changes recommended

Resolve the public virtual API compatibility breaks, serial-queue requirement, and Windows archive update.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR improves IMAP interruption reporting and clarifies lease-operation threading requirements.

Changes:

• Propagates whether interruption actually cancelled a stream.
• Updates Swift and C++ interruption documentation.
• Clarifies lease serialization on the session queue.
• Regenerates mirrored headers.

File summaries

File	Reviewed changes
src/swift/imap/IMAPSession.swift	Clarifies lease queue requirements.
src/swift/imap/IMAPBaseOperation.swift	Documents boolean interruption results.
src/include/MailCore/MCOperationQueue.h	Updates the interruption contract.
src/include/MailCore/MCOperation.h	Changes the interrupt return type.
src/include/MailCore/MCIMAPSession.h	Documents stream cancellation results.
src/include/MailCore/MCIMAPOperation.h	Updates the IMAP interruption API.
src/include/MailCore/MCIMAPAsyncSession.h	Clarifies lease threading.
src/core/imap/MCIMAPSession.h	Mirrors session API changes.
src/core/imap/MCIMAPSession.cpp	Reports whether a stream was cancelled.
src/core/basetypes/MCOperationQueue.h	Mirrors the queue contract.
src/core/basetypes/MCOperationQueue.cpp	Forwards interruption results.
src/core/basetypes/MCOperation.h	Mirrors the interrupt return type.
src/core/basetypes/MCOperation.cpp	Returns false by default.
src/async/imap/MCIMAPOperation.h	Updates the override declaration.
src/async/imap/MCIMAPOperation.cpp	Returns the session cancellation result.
src/async/imap/MCIMAPAsyncSession.h	Mirrors lease-threading documentation.

Review details

Suppressed comments (3)

src/async/imap/MCIMAPAsyncSession.h:128

• This contract also requires a serial queue. setDispatchQueue() accepts any dispatch queue, including concurrent/global queues; placing acquire/release/start on one does not serialize them, so the mSessions append/walk race can remain. Please state or enforce the serial-queue requirement here and regenerate the public mirror.

         start() on this session, and do it on the session's own dispatch queue: the pool's
         bookkeeping is touched from that queue as well - operationRunningStateChanged() walks
         the connection list that a new connection is appended to - so any other queue
         serializes the callers against each other and against nothing else. */

src/core/imap/MCIMAPSession.cpp:3783

• These C++ changes are under src, which is part of the content hashed for the Windows prebuilt archive. The new digest-named mailcore2-windows-<digest>.zip must be built and published for this revision; otherwise the Windows prebuilt check/consumers will fail even though the macOS verification passes.

bool IMAPSession::interruptCurrentCommand()

src/swift/imap/IMAPSession.swift:177

• This contract also requires a serial queue. dispatchQueue is caller-configurable, and a concurrent/global queue does not serialize acquire/release/start, so the append/walk race COR-224 is still possible. Please state or enforce that the session queue must be serial in this API and the mirrored C++ documentation.

     acquireConnection and releaseConnection with every start() on this session, and do it on the
     session's own dispatch queue: the pool's bookkeeping is touched from that queue as well, so
     any other queue serializes the callers against each other and against nothing else.

• Files reviewed: 16/16 changed files
• Comments generated: 3
• 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

[Copilot]

🟡 Changes recommended

Publish the Windows prebuilt artifact for this revision so the Windows check can pass.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 16/16 changed files
• Comments generated: 1
• Review effort level: Lite

[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 d8b121c. Configure here.}