IMAP: Bugs found by Claude

[benbucksch]

IMAP Code Review — app/logic/Mail/IMAP/

I went through IMAPAccount.ts, IMAPFolder.ts, IMAPEMail.ts, plus the relevant imapflow
library code. Findings ordered roughly by severity.

1. Infinite loop in downloadMessages when downloads are already in flight

IMAPFolder.ts:311-351

while (needMsgs.hasItems) {
let downloadingMsgs = needMsgs.getIndexRange(needMsgs.length - kMaxCount, kMaxCount);
downloadingMsgs = downloadingMsgs.filter((msg) => !msg.downloadRunOnce.running);
needMsgs.removeAll(downloadingMsgs);
let uids = downloadingMsgs.map(msg => msg.uid).join(",");
await this.runCommand(...);
}

getIndexRange returns the slice without removing it. We then filter to only-not-running, and
only removeAll(downloadingMsgs) the filtered subset. If the entire batch of 50 happens to
have downloadRunOnce.running (e.g., the user opens an email while a bulk download is
happening), downloadingMsgs becomes [], removeAll([]) is a no-op, uids is "", and the loop
iterates forever with the same state. conn.fetch({uid: ""}, ...) will also misbehave on most
servers (Cyrus/Dovecot will respond BAD to an empty UID set).

Fix: always remove the entire batch from needMsgs, regardless of how many we actually fetch.

2. UIDVALIDITY is stored but never validated

IMAPFolder.ts:21 has uidvalidity: number = 0; and it's persisted in
fromExtraJSON/toExtraJSON, but I can't find anywhere it's compared to the server's
UIDVALIDITY when reopening the mailbox. If a server renumbers UIDs (Exchange and Stalwart
bridges can do this; even Dovecot does it after some recovery scenarios), every stored UID
becomes wrong. We'd silently send STORE/COPY/FETCH against UIDs that now refer to different
messages. Worst case: flag changes / deletes / spam markers applied to the wrong messages.

Fix: read UIDVALIDITY from getMailboxLock/imapflow's mailbox object, compare against stored
value; if it differs, drop the local cache and re-fetch from scratch.

3. messageDeletedNotification silently drops EXPUNGEs at seq 1 and at the end

IMAPFolder.ts:512-520

if (seq == 1) {
return; // TODO Handle seq == 1
}
...
if (remainingUIDs.length != 2) {
this.account.log(this, connection, "newest message deleted", "TODO handle this");
return;
}

Two acknowledged TODOs. Real deletions are not propagated locally. Over hours/days, the
local view diverges from server: ghost messages remain in the list, attempts to open them
fail with "Invalid uidset". Particularly affects users who keep small inboxes (newest msg =
lowest seq #) or who have automated rules that prune from the top.

Fix for seq==1: query {seq: "1:2"}, the returned single UID is the new seq-1; anything
before it in local cache is gone.
Fix for end: query {seq: (seq-1) + ":*"} — if you get back ≤1 UID, every local UID greater
than it has been deleted.

4. \Recent is sent on APPEND

IMAPEMail.ts:132-134

if (message.isNewArrived) {
flags.push("\Recent");
}

\Recent is a server-managed flag (RFC 3501 §2.3.2) and is removed entirely in IMAP4rev2 (RFC
9051). Clients are forbidden from setting it. Behavior across servers:

• Dovecot / Cyrus: silently ignored (mostly).
• Gmail: rejected with BAD.
• Exchange (IMAP gateway): seen to reject the whole APPEND.
• Stalwart: parsed strictly per RFC 9051; will reject.

Fix: never push \Recent from a client.

5. Custom tags only ever get added, never removed

IMAPEMail.ts:118-124

for (let customTag of flags) {
if (customTag.startsWith("\") || customTag.startsWith("$") || ...) continue;
this.tags.add(getTagByName(customTag));
}

System flags (\Seen, \Flagged, etc.) are assigned with = so they correctly track add/remove.
But for keyword/tag flags, this only adds, never deletes. When a tag is removed (by another
client, a sieve rule, or our own removeTagOnServer echoed back), the local set still
contains it. Tags grow monotonically until cache reset.

Fix: compute the set difference — remove from this.tags any tag that was previously set but
is no longer in flags.

6. addMessage discards the new UID returned by APPENDUID

IMAPFolder.ts:540-545

async addMessage(message: EMail) {
message.mime ??= await CreateMIME.getMIME(message);
await this.runCommand(async (conn) => {
await conn.append(this.path, Buffer.from(message.mime),
IMAPEMail.getIMAPFlags(message), message.received);
});
}

imapflow's append() returns an AppendResponseObject with uid/uidValidity when the server
supports UIDPLUS. We throw it away. So after saveSent() or any other path that calls
addMessage, the message has no pID/uid. Subsequent markRead, setFlagServer,
deleteMessageOnServer will run messageFlagsAdd(null, ...) which is a broken command.

Fix: capture the response, set message.uid = response.uid, and only fall back to "fetch by
Message-ID" if APPENDUID isn't supported.

7. countChanged log shows the wrong "our old"

IMAPFolder.ts:473-479

async countChanged(newCount: number, oldCount: number): Promise {
let hasChanged = newCount != oldCount || newCount != this.countTotal;
this.countTotal = newCount; // <-- overwrite here
if (hasChanged) {
this.account.log(..., "our old:", this.countTotal); // <-- already the new value

this.countTotal was just overwritten with newCount. Diagnostic log is permanently
misleading. Capture before overwriting, or move the log above the assignment.

8. setFlagServer log uses wrong operator precedence

IMAPEMail.ts:189

this.folder.account.log(this.folder, conn, set ? "set" : "remove" + " email flag", ...

Parses as set ? "set" : ("remove" + " email flag"). When the call is adding a flag, the log
says just "set" — the " email flag" suffix is lost. Wrap with parens: (set ? "set" :
"remove") + " email flag".

9. logout() triggers the close-handler's reconnect cascade

IMAPAccount.ts:373-388 + attachListeners() at :167-184

logout() calls await conn.logout(), which closes the socket, which fires the close event,
whose handler calls this.reconnect(connection). By then we've already removed the connection
from this.connections, so:

• getKeyForValue(connection) returns undefined
• the purpose param wasn't passed
• assert(purpose, ...) fails
• the assertion's exception sets this.fatalError = new ConnectError(...)

So every clean logout pollutes fatalError with a confusing reconnect-failed message.
verifyLogin() also hits this (connect → logout immediately).

Fix: detach the listeners before calling conn.logout(), OR set a this.isClosing flag that
the close handler respects.

10. connection() can return a dead socket

IMAPAccount.ts:79-84

let conn = this.connections.get(purpose);
if (conn) {
return conn;
}

If the socket has dropped but the close/error handler hasn't yet cleared the entry (race
between socket layer and event loop), callers get a dead connection. The first command will
throw NoConnection, which only runCommand knows how to retry — many of the direct callers
(getNamespaces, getSharedPersons, addPermission, removePermission, listFolders) don't have
that retry.

Fix: check conn.usable && conn.authenticated (and !conn.socket?.destroyed) before returning;
otherwise trigger reconnect.

---

Concurrency / multi-connection bugs

11. messageFlagsChanged falls back to seq lookup when uid is known but unknown locally

IMAPFolder.ts:489-491

let query = uid && this.getEMailByUID(uid)
? { uid: uid }
: { seq: seq };

If the server gave us a uid but we don't have that email cached, we fall through to {seq}.
Two problems:

• fetchFlags will then getEMailByUID(msgInfo.uid) and skip if not found anyway — the seq
  lookup gains nothing useful.
• Worse, seq is volatile. Between event emission and our fetch, an EXPUNGE on the same
  connection can shift seq. We'd then read flags for a different message and (if its uid
  matches some local email) write those flags onto the wrong local message.

Fix: prefer uid whenever it's present, full stop.

12. runCommand doesn't retry if the connection drops mid-operation

IMAPFolder.ts:59-97

The try/catch (NoConnection) block only wraps mailbox-open. The actual imapFunc(conn) runs
outside:

try {
...
lockMailbox = await conn.getMailboxLock(this.path);
} catch (ex) {
if (ex.code == "NoConnection") {
...reconnect & re-open mailbox...
} else {
throw ex;
}
}
return await imapFunc(conn); // no retry here

If the socket dies during the FETCH/STORE itself (very common on flaky networks, mobile
tethering, VPN drops), the user sees a hard error. Especially nasty for the IDLE-on-Main
connection which holds a long-lived socket.

Fix: move the retry to wrap imapFunc(conn) too, or use a wrapper that retries any
NoConnection once.

13. The close and error handlers can race when their captured connection is stale

IMAPAccount.ts:167-184

The closure captures connection. If we reconnect, replace the entry, and then the old socket
emits close (sockets can be slow), the handler calls this.reconnect(staleConn). Inside
reconnect:
purpose = this.connections.getKeyForValue(connection) ?? purpose;
getKeyForValue(staleConn) returns undefined; no purpose arg from event handlers; assert
fires; fatalError set. Same root cause as bug #9.

Fix: when reconnecting, replace the entry first, then detach handlers from the old conn. Or
have the close handler bail if connections.getKeyForValue(connection) is undefined.

14. reconnect calls conn.close() which fires the close handler, which calls reconnect

IMAPAccount.ts:229-257 + :167

return await this.reconnectRunOnce.get(purpose).runOnce(async () => {
try {
await connection.close(); // ← fires 'close' event → handler → reconnect()
} catch (ex) {...}

The runOnce guard saves us from infinite recursion, but the re-entrant call from the close
handler still triggers logging, "fatalError set" if anything goes wrong, etc. Cleaner to
remove listeners before close().

15. getMailboxLock may run on a different mailbox than the one that fired the event

IMAPFolder.runCommand always calls getMailboxLock(this.path). For event handlers (flags /
expunge), they pass the originating connection, which might be in a different mailbox right
now. getMailboxLock will queue and re-SELECT it. While that's happening, more events for the
original mailbox keep arriving on this connection, and their seq numbers refer to "back
when it was selected" — those notifications get processed with stale seq.

Symptom: under heavy notification load while user is browsing other folders, occasional flag
updates/expunges applied to the wrong message.

Fix: when the event-firing connection is currently elsewhere, prefer to do the fetch on the
Fetch connection by UID (and ignore the seq number entirely), instead of dragging the
originating connection back.

16. flagsChanging suppresses real concurrent server-side changes

IMAPEMail.ts:107 + setFlagServer

When we're in the middle of setFlagServer("\Seen", true), any incoming FLAGS notification
for this same message is ignored (if (this.flagsChanging) return). The intent is to suppress
our own echo, but it also suppresses a genuine concurrent change from another IMAP client
(e.g., a phone marking the same email starred). Window is short but real — and if the user
toggles flags rapidly, the flag stays "true" for the duration of the runCommand.

Fix: compare what we asked for vs. what came back. Only suppress if it matches our own
request.

---

Wrong commands / response handling

17. EXPUNGE handler doesn't recognize vanished: true payloads

IMAPAccount.ts:211-221

assert(typeof (info.seq) == "number", "seq must be a number");
await folder.messageDeletedNotification(info.seq, connection);

imapflow's expunge event has two shapes:

• {path, seq, vanished: false} — classic EXPUNGE
• {path, uid, vanished: true, earlier: bool} — QRESYNC VANISHED

QRESYNC isn't currently enabled (no qresync: true in connect options), so this is dormant.
But should QRESYNC ever be turned on (it would be a huge win for fast resync), this
assertion fires for every deletion. Should check info.vanished first; on true go through a
UID-based delete path.

18. fetchMessageList's modseq tracking saves "last seen", not max

IMAPFolder.ts:224-240

let modseq: bigint;
for await (let msgInfo of msgsAsyncIterator) {
...
modseq = msgInfo.modseq
}
await this.updateModSeq(modseq);

Iteration order isn't guaranteed (imapflow returns in seq order, but modseq doesn't strictly
correlate). updateModSeq is monotonic so it's not corrupting — but it means we save a
smaller modseq than what we processed, and the next CONDSTORE sync refetches changes we
already have. Wasteful, especially for large folders.

Fix: track max.

19. fetchMessageList ignores updatedMessages when called from listChangedMessages

IMAPFolder.ts:179-188

let { newMessages } = await this.fetchMessageList({ all: true }, {
uid: true,
changedSince: this.lastModSeq,
});
this.messages.addAll(newMessages);

We destructure only newMessages. updatedMessages are the ones whose flags/threadId/envelope
changed since lastModSeq. We don't persist those updates locally. Result: flag changes from
other clients during offline periods are lost on CONDSTORE-aware servers (Dovecot, Cyrus,
Stalwart all support CONDSTORE). The "fresh" copy from periodic restarts/full-resync covers
it, but in between, the user sees stale flags.

Fix: also saveMsgUpdates(updatedMessages).

20. listNewMessages query refetches the highest known UID

IMAPFolder.ts:199-200

let fromUID = this.getHighestUID() ?? 1;
let { newMessages } = await this.fetchMessageList({ uid: fromUID + ":*" }, {});

fromUID:* is inclusive on both ends, so we always refetch the message at fromUID. That's
fine for catching flag changes on the newest known msg, but if that's the only reason, just
use (fromUID+1):* and call updateNewFlags separately. Currently updateNewFlags is called on
a different path (listAllUnknownMessages) and not from listNewMessages — so there's actually
a partial dependency on this overlap.

21. Error matching by message string is fragile

IMAPEMail.ts:46

if (ex.message == "IMAP UID FETCH: Invalid uidset") {
await this.disappeared();
return;
}

This matches imapflow's exact wording. Any upstream change ("Invalid UID set", lowercase,
locale, etc.) silently breaks the disappeared-message path — the email re-shows in the UI
and the user can never get rid of it. Use error code if available, or match the response
text from the IMAP server (NO/BAD responses for non-existent UIDs are more stable).

22. msgInfo from fetchOne can be null but we deref it

IMAPEMail.ts:53

if (!msgInfo.envelope || this.folder.deletions.has(msgInfo.uid)) {

If fetchOne returns null (message not found, no error thrown), msgInfo.envelope is a
TypeError, not a graceful "disappeared". Add a !msgInfo || guard.

23. getNamespaces doesn't lock the connection

IMAPAccount.ts:272-279

let conn = await this.connection();
await conn.run("NAMESPACE");
await conn.__refresh();
return conn.namespaces;

Unlike most other operations, this doesn't acquire connectionLock. If another caller is in
the middle of a long FETCH on the Main connection, calling run("NAMESPACE") may be safe
(imapflow internally serializes) but the surrounding __refresh() reads connection state
without the lock — and on a slow server the namespace command can block. Since it's only
called during startup, low-impact, but inconsistent with how the rest of the code locks.

24. getSharedPersons etc. use conn.exec directly without runCommand

IMAPFolder.ts:636-681

getSharedPersons, addPermission, removePermission go around runCommand and use the raw
conn.exec / lock pattern. They don't call getMailboxLock(this.path), so they execute against
whatever mailbox is currently selected — for ACL commands this is fine since the path is in
the command args. But they also don't reconnect on NoConnection. So an ACL-management UI
action right after a network blip will fail rather than reconnecting.

---

Performance / hidden cost

25. getEMailByUID is O(n) and called O(n) times per fetch

IMAPFolder.ts:362-364 + uses in fetchMessageList, fetchFlags, downloadMessages.

getEMailByUID(uid: number): IMAPEMail {
return this.messages.find(m => m.uid == uid);
}

For a folder of 10k messages, processing a batch of 200 new fetches does 200×10000
comparisons. Big inbox? Several seconds blocking the renderer.

Fix: maintain a Map<uid, IMAPEMail> synced with messages.addAll/removeAll.

26. listAllUnknownMessages does O(N×M) UID set diff

IMAPFolder.ts:134

let deletedMsgs = this.messages.filterOnce(msg => !allUIDs.includes(msg.uid));

For a 30k-message folder, 30k×30k = 900M comparisons. Convert allUIDs to a Set once.

27. Throttle runs on every connection() call, including the hot path

IMAPAccount.ts:80

async connection(...): Promise {
await this.throttle.throttle();
let conn = this.connections.get(purpose);
if (conn) {
return conn;
}

50/sec is plenty, but the throttle bookkeeping (shift+push to a 50-slot array) runs on every
call, even when we just return a cached healthy connection. With many small downloads,
that's a lot of churn. Move the throttle inside the if (!conn) branch — only throttle actual
connection creation.

---

Smaller issues

28. Duplicate flagsChanging = true — IMAPEMail.ts:186, 188. Set outside and inside the
    callback; harmless dup.

29. runCommand's finally always logs "released lock" — even when nothing was locked.
    Cosmetic.

30. getHighestUID() returns 1 for an empty folder — gives "1:*" semantics that fetch all,
    which is OK by accident; document or rename to getHighestUIDOrDefault().

31. listFolders calls await this.storage.readFolderHierarchy(this) again on every refresh —
    login() already did this immediately before; arguably wasted I/O.

32. reconnect doesn't reset pollIntervalMinutes or restart polling for non-inbox folders —
    comment says "Do not stop polling", but polling only runs on the inbox in the first place
    (startup() only starts polling on inbox). If reconnect happens during a long network outage,
    the existing setInterval still fires while disconnected and accumulates throttled requests.

33. addSharedPerson calls addPermission per folder, sequentially with await — for an account
    with hundreds of folders this can take a long time without progress feedback. Worth
    parallelizing with a small concurrency cap.

34. createSubFolder and createToplevelFolder set the path before the server's response —
    newFolder.path = this.path + "/" + name then overwritten by created.path. If the server uses
    a different delimiter (. vs /), the intermediate path persists briefly in any reactive
    observers. Cosmetic, but Cyrus typically uses . and not /.

35. setSpecialUse matches path.toLowerCase() == "sent" but Exchange returns "Sent Items",
    Gmail returns "[Gmail]/Sent Mail". Only literal "sent" matches. The XLIST/SPECIAL-USE
    flag-based branch above should cover those, but Exchange's IMAP gateway is notorious for not
    setting \Sent.

36. Variable shadowing in readFolders — subFoldersInfo is the param name and a let inside
    two of the branches. Hard to follow; rename the inner ones.

37. The connection.on("log", ...) listener has no off-switch — if logLibrary is toggled at
    runtime, listeners aren't detached.

---

Suggested next steps (in priority order)

1. Fix the downloadMessages infinite loop (Bump acorn from 6.4.0 to 6.4.1 #1) — this can hang the UI.
2. Add UIDVALIDITY check (Bump websocket-extensions from 0.1.3 to 0.1.4 #2) — silent data corruption risk.
3. Capture APPENDUID (Bump node-fetch from 2.6.0 to 2.6.1 #6) — sent emails are currently un-flaggable.
4. Fix tag-removal sync (Bump http-proxy from 1.18.0 to 1.18.1 #5).
5. Stop sending \Recent on APPEND (Bump elliptic from 6.5.2 to 6.5.3 #4).
6. Suppress reconnect on intentional logout (Bump semver from 6.3.0 to 6.3.1 in /react-electron #9, Runner: Need Dev Tools #13, Runner: Add Firefox's DevToolsStartup file, to allow -jsdebugger to work #14).
7. Handle EXPUNGE at seq 1 and at end (Bump electron from 7.1.6 to 7.2.4 #3).
8. Replace getEMailByUID linear scan with a Map (Fix probable logic error #25).

Bug #1 (infinite loop) and #6 (lost APPENDUID) are the ones I'd patch first — both are quick
wins with high user impact.

[NeilRashbrook]

6. addMessage discards the new UID returned by APPENDUID

I'm not convinced we actually care about this one - the message passed in never makes it into a message list (or might be a message from another server) while the copy we're making of this message on the server will be picked up when we next refresh the message list.

23. getNamespaces doesn't lock the connection

It's only called once, during startup, but I'm not sufficiently familiar with the IMAP connection locking to quickly resolve this.

24. getSharedPersons etc. use conn.exec directly without runCommand

Note that runCommand is a method on IMAPFolder and tries to SELECT it which isn't strictly needed.

33. addSharedPerson calls addPermission per folder, sequentially with await

Unless the connection code is refactored to allow multiple connections (e.g. connection pool of five connections, each tracking the current selected folder, when a connection for a given folder is requested, if it already exists bump its lru otherwise take the oldest connection and change its folder, then for folder-less connections just use any free connection), all of the calls are going to go down the same connection, so they won't parallelise.