COR-221 - Wait out a running IDLE before freeing its stream

[dbezverkhnii]

Cherry-pick of upstream 03a19472 ("Fix IMAP IDLE teardown races", by the mailcore maintainer), adapted to this fork. One of the 39 upstream commits we do not have; taking it now is also a down payment on the pending large upstream merge.

What was wrong

Two defects, both the shape we closed elsewhere in COR-211/212/213 and COR-216, just in the IDLE path:

1. setupIdle(), interruptIdle() and unsetupIdle() dereferenced mImap->imap_stream guarded only by mIdleEnabled. unsetup() does clear mIdleEnabled in the same critical section that nils mImap, so mIdleEnabled == true did imply mImap != NULL — but not that imap_stream != NULL, which libetpan nils on logout and on some stream errors.
2. idle() read mImap->imap_stream under no lock and then blocked in mailstream_wait_idle() on it, while a teardown on another thread closed and mailimap_freed it. Use-after-free.

What the port does

mIdleInProgress plus a condition variable. idle() takes a local mailimap * under the lock and raises the flag; unsetup() and unsetupIdle() interrupt the running idle and then wait for the flag to drop before freeing anything; the three main-thread entry points get their NULL checks. IMAPIdleOperation gains a cancel() override that calls interruptIdle(), and its mSetupSuccess becomes lock-protected.

cancel() is an override of the already-virtual Operation::cancel(), so no vtable slot is added and interruptIdle()'s index is unchanged.

Fork adaptations

• This fork locks through the MCB_* shim rather than raw pthread calls, so upstream's pthread_cond_* arrives as new MCB_COND_* macros in MCBasicLock.h: CONDITION_VARIABLE + SleepConditionVariableSRW(c, l, INFINITE, 0) on _MSC_VER (flags 0 = the SRWLOCK is held exclusively, which is how MCB_LOCK takes it), pthread_cond_* elsewhere. MCB_COND_DESTROY is empty on Windows, mirroring the existing empty MCB_LOCK_DESTROY.
• The merge preserves this fork's own unsetup() changes from COR-217/COR-218 unchanged: mStreamCancelled cleared and mState = STATE_DISCONNECTED published inside the lock, close and free after the unlock. The new wait sits ahead of all of it.
• Upstream's companion stress harness ab53363b is a 634-line C++ file in tests/, which exists because upstream has no Swift tests; our CMake tests executable is not what CI runs, so it was not copied. Its third goal — "does not crash or hang under repetition, especially under ASan" — is covered by a Swift test instead.
• interruptCurrentCommand()'s comment claimed holding mIdleLock is a bounded wait. That is no longer true, so the comment now says so.

Tests

testCancelAllOperationsWakesRunningIdle was carrying an XCTExpectFailure waiting for exactly this commit; the expectation is gone and the assertion stands (and is strictly stronger — waitForDoneOrClose was previously only reached conditionally).

New testRepeatedCancelDuringIdleDoesNotHangOrCrash runs the connect/idle/cancel/tear-down cycle six times. It gates on the fake server signalling + idling rather than on a sleep, so a cancel that arrives before IDLE is established — which the operation queue simply drops, proving nothing — cannot make it pass vacuously.

Verified: 6/6 iterations fail against the pre-fix sources (the queue never stops, 60s of hung iterations) and pass in 6.3s with the fix. All 33 IMAP tests pass on macOS, and again under --sanitize=address with no sanitizer reports.

Two waitForIdleEntered preconditions went 5s → 20s. The iOS job failed once on that budget already (connect + CAPABILITY + LOGIN + LIST + SELECT + IDLE inside 5s on a loaded runner); the waits that carry the property under test are untouched at 1s and 5s.

Known gaps, deliberately not addressed here

• Defect 2 has no test. Upstream's Test B — disconnect racing a running idle() — needs the synchronous IMAPSession driven from two threads, which the Swift layer does not expose. Everything shipping here exercises defect 1. Reaching the new mIdleCond machinery from a test would mean adding another test hook to the production surface; not worth it in this change.
• unsetupIdle() issues the interrupt but not mailstream_cancel, unlike unsetup(). If it ever ran with an idle in flight blocked inside mailimap_idle(), it would spin until the socket timeout — on a method documented "main thread". Unreachable today: its only caller is IMAPIdleOperation::unprepare, which runs after idle() returned. Left as upstream wrote it to keep the merge clean.
• IMAPIdleOperation::prepare() drops mLock between reading mInterrupted and writing mSetupSuccess, so a cancel() landing in that window issues no interrupt and main() never re-checks. Present identically upstream; out of scope for a port.

🤖 Generated with Claude Code

---

Note

High Risk
Changes cross-thread IMAP teardown and blocking waits on the idle lock, which can affect disconnect/cancel timing and previously had use-after-free risk on the live stream.

Overview
Fixes IMAP IDLE teardown races by tracking when idle() is blocked on the socket and making disconnect/unsetup wait for that work to finish before closing the stream.

IMAPSession adds mIdleInProgress and a condition variable (MCB_COND_* in MCBasicLock.h). idle() sets the flag under mIdleLock, runs IDLE on a local mailimap *, then clears the flag and broadcasts in a shared cleanup path. unsetup() and unsetupIdle() interrupt any in-flight IDLE and MCB_COND_WAIT until the flag drops; setupIdle(), interruptIdle(), and related paths gain mImap / imap_stream null checks and refuse overlapping IDLE setup.

IMAPIdleOperation overrides cancel() to call interruptIdle() so cancelAllOperations() can wake a running IDLE; mSetupSuccess is read/written under its lock, and interruptIdle() only calls into the session when setup succeeded and session() is non-null.

Swift tests drop the XCTExpectFailure on cancel-waking-IDLE, add a six-iteration cancel-during-IDLE stress case, and relax some setup timeouts on slow CI.

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

[Copilot]

🟡 Changes recommended

Address the setup state, Windows artifact, and test timeout issues.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Ports the upstream IMAP IDLE teardown-race fix, coordinating stream lifetime, teardown, and cancellation.

Changes:

• Adds condition-variable synchronization for in-progress IDLE operations.
• Interrupts active IDLE operations on cancellation.
• Expands Swift cancellation and stress-test coverage.

File summaries

File	Reviewed change	Final review note
unittest/IMAPIdleCancellationTests.swift	Updates IDLE cancellation tests and wait budgets.	Increase the outer timeout. Moderate, 3 votes.
src/include/MailCore/MCIMAPSession.h	Declares IDLE synchronization state.	No final comment.
src/include/MailCore/MCIMAPIdleOperation.h	Declares cancellation support.	No final comment.
src/include/MailCore/MCBasicLock.h	Adds condition-variable macros.	Publish the matching Windows prebuilt archive. Moderate, 1 vote.
src/core/imap/MCIMAPSession.h	Adds internal synchronization declarations.	No final comment.
src/core/imap/MCIMAPSession.cpp	Coordinates IDLE execution and teardown.	No final comment.
src/core/basetypes/MCBasicLock.h	Adds internal condition-variable macros.	Publish the matching Windows prebuilt archive. Moderate, 1 vote.
src/async/imap/MCIMAPIdleOperation.h	Updates async operation declarations.	No final comment.
src/async/imap/MCIMAPIdleOperation.cpp	Implements cancellation interruption and locking.	Clear mSetupSuccess during unprepare(). Moderate, 1 vote.

Review details

Suppressed comments (4)

src/async/imap/MCIMAPIdleOperation.cpp:51

• mSetupSuccess remains true after unprepare() completes, so cancelling a retained, already-finished idle operation still enters interruptIdle() and can interrupt a later IDLE on the same connection. Clear the setup-success state as part of unprepare (before calling unsetupIdle()) so cancellation only targets the operation's active setup.

    if (setupSuccess()) {

src/core/basetypes/MCBasicLock.h:20

• These C/C++ changes are included in the Windows source digest, so they require a matching mailcore2-windows-<digest>.zip on the permanent windows-prebuilt release. Without publishing that archive before merge, the Windows prebuilt check and Spark's Windows build will fail even though the source compiles elsewhere.

#define MCB_COND_TYPE CONDITION_VARIABLE
#define MCB_COND_INIT(c) InitializeConditionVariable(c)
#define MCB_COND_DESTROY(c)
/* 0 = the lock is held exclusively, which is how MCB_LOCK takes it. */
#define MCB_COND_WAIT(c, l) SleepConditionVariableSRW(c, l, INFINITE, 0)

unittest/IMAPIdleCancellationTests.swift:347

• This 20-second gate plus the later 1s, 5s, and 5s waits can take 31 seconds, but the enclosing runOffMainThread(timeout: 30) still expires at 30 seconds. A slow but successful cancellation test can therefore fail due only to the wrapper timeout; increase that outer budget.

            XCTAssertTrue(server.waitForIdleEntered(timeout: 20), "The session was expected to enter IDLE. Client sent:\n\(server.transcript)")

unittest/IMAPIdleCancellationTests.swift:376

• The new gate can consume 20 seconds and the queue-stop wait another 10 seconds, while this body is still wrapped in runOffMainThread(timeout: 30). That leaves no scheduling margin and can make a valid iteration time out; use a larger outer timeout for this stress case.

                XCTAssertTrue(server.waitForIdleEntered(timeout: 20),

• Files reviewed: 9/9 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]

Thanks — one accepted, one declined, one already planned.

Test timeout arithmetic — fixed in 8b0bf9f. Correct arithmetic and my bug, not upstream's: raising waitForIdleEntered to 20s while leaving runOffMainThread(timeout: 30) alone would have traded the flake I was fixing for a wrapper-timeout flake. testInterruptIdleEndsRunningIdle sums to 36s (20+1+5+5+5), the cancel case to 31s, one stress iteration to 30s. All three bodies now get 60s, with a note on runOffMainThread that the budget has to clear the sum of the waits so a slow-but-correct body fails on its own assertion. Re-ran green.

Clearing mSetupSuccess in unprepare() — declining here. The stale-true window is real, but it is upstream's code and it predates this PR: interruptIdle() already read mSetupSuccess with exactly these semantics before the cherry-pick, and this change only moves that read under mLock. Nothing here widens it.

It is also not reachable from this repo's consumer. Spark's RSMIdleIMAPSession clears its idleOperation slot in the completion callback behind an identity guard, and interruptIdleOperation() nils the slot before interrupting its local copy and then drops the reference — so a finished operation is never retained and interrupted later. The one residual window is an operation completing between that read and the interruptIdle() call, which would interrupt a newer IDLE on the same connection; that window is unchanged by this PR, and Spark's own comment shows the reporting side of the same race is already handled.

This fork is 572 commits ahead of its upstream merge base with a large merge pending, so a deviation from 03a19472 has to earn its keep. This one does not, in this change. Worth its own ticket rather than a silent divergence in a port — flagging it.

Windows prebuilt — already on the plan. Correct, and the red mailcore2 - Windows prebuilt check is exactly that question being asked early. MCBasicLock.h is under src, so the digest changed and the archive gets published from a Windows machine at the final revision before merge. Worth stressing the other half of it: no CI job compiles the _MSC_VER branch at all, so that publish run is the first time the new MCB_COND_* macros meet a compiler.

[Copilot]

🔵 Needs a closer look

Cancellation can be lost during setup, allowing a canceled operation to enter blocking IDLE.

Review details

Suppressed comments (1)

src/async/imap/MCIMAPIdleOperation.cpp:46

• Cancellation can still be lost in this handoff: isInterrupted() releases mLock before setupIdle() completes and setSetupSuccess() records the result. If cancel() lands in that window, it observes mSetupSuccess == false and sends no interrupt; main() then observes setup success and enters the blocking idle() despite the cancellation. Make the cancellation/setup transition atomic, or re-check cancellation after prepare and undo the setup before returning.

    setSetupSuccess(session()->session()->setupIdle());

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

[dbezverkhnii]

Declining this one too, for the same reason as the unprepare() finding — and in this case the port makes the behaviour strictly better, not worse.

The window is real and correctly described: prepare() drops mLock between isInterrupted() and setSetupSuccess(), so a cancel() landing there sends no interrupt and main() goes on to block in idle(). But it is upstream's code unchanged — before this cherry-pick prepare() was

if (isInterrupted()) { mSetupSuccess = false; return; }
mSetupSuccess = session()->session()->setupIdle();

with interruptIdle() reading mSetupSuccess unsynchronised. The port narrows that: every access now goes through mLock. The handoff itself is untouched.

Worth comparing the two sides for the case you describe, a cancel arriving around setup:

• before: cancelAllOperations() did not call interruptIdle() at all, so a running IDLE was always left blocked until MAX_IDLE_DELAY or the socket timeout. That is the defect this PR exists to fix, and it is what testCancelAllOperationsWakesRunningIdle was carrying an XCTExpectFailure for.
• after: the IDLE is woken, except when the cancel lands inside this sub-millisecond handoff.

So the port takes it from "always" to "in a narrow race". Closing the race as well means either holding mLock across setupIdle() — which calls into IMAPSession and takes mIdleLock, i.e. a new lock-order edge in the layer this change is trying to make safer — or re-checking cancellation after prepare() and undoing the setup. Both are real design choices, not a port's business: this fork is 572 commits ahead of its merge base with a large upstream merge pending, and a silent divergence inside a cherry-pick is exactly what makes that merge expensive.

It is already listed under "known gaps" in the description, and it is now tracked with the unprepare() finding in COR-226 — same field, same two methods, one fix to design.

CI is green on 8b0bf9fd: Android, macOS, iOS, Secrets, Bugbot, and the Windows prebuilt check now that the archive for this digest is published. The earlier iOS red was the runner, not the code — xcodebuild found no iOS simulator at all on that machine ({ OS:26.2, name:iPhone 17 } unmatched, only macOS destinations listed); it passed on re-run.