Repository navigation
Stop concurrent requests from running in each other's mailbox - #47
Merged
Merged
Conversation
Every tool selects a mailbox and then runs its commands, and all requests for an account shared one IMAP connection. SwiftMail serializes single commands, not the select-plus-command pair, so when requests overlapped, one request's SELECT could land between another's SELECT and SEARCH: searches returned another mailbox's results or failed, UIDs were reported under the wrong mailbox, and move, trash or expunge could act on the same UIDs in the wrong mailbox. withServer now holds a per-account lock for the whole operation, so the primary connection runs one request at a time. Searches, counts and raw downloads (post search, post fetch --eml) instead lease one of two extra connections per account (SwiftMail named connections), one request per connection, so they neither wait for nor disturb the primary connection. If the server refuses an extra connection they run on the primary one. Search results now fetch header data only instead of every hit's full message. ConcurrentMailboxAccessLiveTests compares concurrent requests with the same requests run one at a time on a real account; it runs only with POST_LIVE_TEST_SERVER set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca636e3ce4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The primary-connection locks and extra-connection leases lived in the IMAPConnectionManager, which reloadConfiguration() replaces. A request holding the old manager's lock could then run on the new manager's connection while a request after the reload took the new manager's lock, so the two could interleave SELECTs on the same connection again. PostServer now owns the locks and leases, so they survive the switch. Operations still use whichever manager is current for connections; a connection the reload closes under a running operation is reconnected on the new manager while the operation keeps its lock or lease. The switch itself moved into replaceConnectionManager(configuration:), which the new tests use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every tool selects a mailbox and then runs its commands, and all requests for an account share one IMAP connection. SwiftMail's command queue serializes single commands, not the select-plus-command pair, so overlapping requests interfere: one request's SELECT lands between another's SELECT and SEARCH.
Reproduced on 1.9.0 by starting five
post searchcalls at the same time, four on one account in different mailboxes and one on a second account:The same race lets
post fetchread a UID from the wrong mailbox, and lets move, trash, junk, flag or expunge act on the same UIDs in another mailbox.Fix
withServerholds a per-accountAsyncSerialLockfor the whole operation (select, commands, reconnect retry), so the primary connection runs one request at a time.searchMessages,countMessages,downloadEmlandfetchRawMessagesrun throughwithExtraConnection, which leases one of two extra connections per account (IMAPConnectionManager.extraConnectionsPerServer, SwiftMail named connectionspost-extra-0/-1). Each serves one request at a time, and one-at-a-time use only opens the first. If the server refuses an extra connection, the request runs on the primary connection under the lock; a dropped extra connection is reconnected once.MailboxCommandRunningis a small protocol thatIMAPServerandIMAPNamedConnectionboth conform to, so the same code runs on either.fetchMessageInfosBulk, in chunks of 200) instead of downloading every hit's full message. The output is unchanged.Tests
AsyncSerialLockandConnectionSlotPool.swift test: 57 tests, 0 failures.ConcurrentMailboxAccessLiveTests(read-only; runs only withPOST_LIVE_TEST_SERVER=<server id>, optionallyPOST_LIVE_TEST_MAILBOXES): a serial baseline, then three rounds of search, count, raw download and listing on four mailboxes at once. Passed 5 of 5 runs against a real account in about 13 s each, also after rebasing onto SwiftMail 1.14. On the code before this change it fails: a concurrent search hits a server-side FETCH error.Not in this PR
countMessagesover a whole large mailbox (60k+ messages) fails withPayloadTooLargeError: the ESEARCH ALL reply exceeds SwiftMail's 1 MB response buffer. Same on main; the live test limits its counts to the last year.listMessages(post list) andfetchMessagestill download complete messages; listing could use the header-only fetch as well.🤖 Generated with Claude Code