Skip to content

COR-237 - Close the connection after an SMTP account check - #118

Merged
dbezverkhnii merged 1 commit into
spark2from
fix/COR-237-smtp-check-disconnects
Oct 7, 2026
Merged

dbezverkhnii merged 1 commit into
spark2from
fix/COR-237-smtp-check-disconnects

Conversation

@dbezverkhnii

@dbezverkhnii dbezverkhnii commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

SMTPCheckAccountOperation left the SMTP session connected after the check. The socket stayed open until SMTPAsyncSession::tryAutomaticDisconnect, 30 s later, and the delayed call retains the session, so dropping the Swift object does not close it either.

Why it matters

Some servers cap concurrent SMTP connections per client IP. Measured on a GoDaddy cPanel account (Exim):

  • exactly one connection is allowed, with or without AUTH, and ports 465 and 587 share the counter;
  • the second connection gets 421 Too many concurrent SMTP connections in plaintext before TLS;
  • on 465 mailcore only reports ErrorConnection.

Spark's add-account flow checks the same credentials twice about 5 s apart (Verify, then Login). The second check therefore fails while the first one's socket is still open.

Change

  • SMTPCheckAccountOperation::main calls disconnect() after checkAccount, whatever the outcome:
    • on a connect failure mSmtp is already NULL and the call does nothing;
    • otherwise it sends QUIT, which mailsmtp_quit does not wait on, and unsets the session;
    • the later automatic disconnect then finds the session disconnected and does nothing.
  • The only caller of checkAccountOperation in Spark core is discovery's IMAPChecker. The send path does not use it.
  • A consumer that checks and then sends on the same session now pays one extra connect.

Tests

  • SMTPCheckAccountTests plays an SMTP server on loopback (LeaseTestTCPEndpoint gains a replies mode for untagged line protocols). It asserts that the client closes its socket within 5 s after a successful check and after a rejected sender.
  • Both tests fail without the fix (the socket stays open) and pass with it.
  • swift test --filter 'IMAP|SMTP|Libetpan|Certificate': 52 tests, 0 failures. The IMAP tests that use the endpoint are unchanged.

Not done here

  • Windows prebuilt. The C++ source changes, so the Windows prebuilt has to be republished from a Windows machine (Publish-Mailcore2Prebuilt.ps1). The mailcore2 - Windows prebuilt check will stay red until then.
  • Tag and core bump. After merge the fix still needs a tag and a bump in spark-core-mono. The core side of COR-237 (retrying refused discovery permutations) is a separate PR.

🤖 Generated with Claude Code


Note

Medium Risk
Changes SMTP session lifecycle for all check-account callers; safe for discovery-style one-shot checks but adds an extra connect if the same session is reused for sending.

Overview
SMTP account checks now tear down the session immediately instead of leaving the socket open until the ~30s automatic disconnect. SMTPCheckAccountOperation::main calls disconnect() after checkAccount completes, whether the check succeeds or fails, so per-IP connection limits no longer block a follow-up check (e.g. back-to-back verify/login).

Test support: LeaseTestTCPEndpoint gains an optional replies closure for line-oriented protocols like SMTP. New SMTPCheckAccountTests assert the client closes within 5s after a successful check and after a rejected sender. Package.swift registers the new test file.

Trade-off: Reusing the same MCOSMTPSession for a check and then send now requires an extra connect.

Reviewed by Cursor Bugbot for commit f209e2f. Bugbot is set up for automated code reviews on this repo. Configure here.

@dbezverkhnii

Copy link
Copy Markdown
Collaborator Author

bugbot run

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

`SMTPCheckAccountOperation` left the session connected, so the socket
stayed open until the automatic disconnect 30 s later. A server that
caps concurrent SMTP connections per client IP (GoDaddy's Exim allows
one) then refuses the next check with `421` before TLS, which reaches
the caller as `ErrorConnection`: Spark's account discovery checks the
same credentials twice a few seconds apart and fails on the second.

`LeaseTestTCPEndpoint` learns to answer untagged line protocols so the
test can play an SMTP server.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dbezverkhnii
dbezverkhnii force-pushed the fix/COR-237-smtp-check-disconnects branch from bdb7bb7 to f209e2f Compare October 7, 2026 18:24
@dbezverkhnii

Copy link
Copy Markdown
Collaborator Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 f209e2f. Configure here.

@dbezverkhnii
dbezverkhnii marked this pull request as ready for review October 7, 2026 20:03
@dbezverkhnii
dbezverkhnii merged commit 8b0b265 into spark2 Oct 7, 2026
9 of 10 checks passed
@dbezverkhnii
dbezverkhnii deleted the fix/COR-237-smtp-check-disconnects branch October 7, 2026 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant