Serialize IMAP recent-message claims

contrib/testing/upstream-imaptest-check.py

@@ -456,6 +456,10 @@ def stress(args: argparse.Namespace) -> None:
                 raise UpstreamTestError(
                     "stress run operated on a missing fixture mailbox"
                 )
+            if re.search(r"(?m)^Error:", output):
+                raise UpstreamTestError(
+                    "Dovecot ImapTest reported a state-tracking error"
+                )
         finally:
             time.sleep(0.1)
             cleanup_scripted_mailboxes(args, args.mailbox)

src/agents/imap/event.c

@@ -515,6 +515,7 @@ SendRememberedEvents(ImapSession *session, unsigned long typesAllowed)
                 
     if ((events->remembered & STORE_EVENT_NEW) && (typesAllowed & STORE_EVENT_NEW)) {
         uint32_t recentUid;
+        BOOL recentLocked = !selectedFolder->readOnly;
 
         sequenceChanged = TRUE;
         /*
@@ -523,22 +524,34 @@ SendRememberedEvents(ImapSession *session, unsigned long typesAllowed)
          * \Recent flags and advances the shared threshold; later sessions
          * therefore cannot report the same messages as recent.
          */
+        if (recentLocked) FolderRecentLock();
         ccode = FolderGetRecentUid(storeConn, selectedFolder->info,
                                    &recentUid);
-        if (ccode != STATUS_CONTINUE) return ccode;
+        if (ccode != STATUS_CONTINUE) {
+            if (recentLocked) FolderRecentUnlock();
+            return ccode;
+        }
         selectedFolder->info->uidRecent = recentUid;
         ccode = MessageListAddNewMessages(storeConn, selectedFolder,
                                           events->new, events->newCount);
-        if (ccode != STATUS_CONTINUE) return ccode;
+        if (ccode != STATUS_CONTINUE) {
+            if (recentLocked) FolderRecentUnlock();
+            return ccode;
+        }
         /*
          * A newly delivered message can already carry keywords.  Announce
          * the expanded mailbox FLAGS set before any later FETCH can expose
          * such a keyword to this selected client.
          */
         ccode = SendMailboxFlags(clientConn, selectedFolder);
-        if (ccode != STATUS_CONTINUE) return ccode;
+        if (ccode != STATUS_CONTINUE) {
+            if (recentLocked) FolderRecentUnlock();
+            return ccode;
+        }
         if (!selectedFolder->readOnly) {
             ccode = FolderSetRecentUid(storeConn, selectedFolder->info);
+            FolderRecentUnlock();
+            recentLocked = FALSE;
             if (ccode != STATUS_CONTINUE) return ccode;
         }
         events->newCount = 0;

src/agents/imap/imapd.c

@@ -333,6 +333,18 @@ FolderSetRecentUid(Connection *storeConn, FolderInformation *folder)
     return(CheckForNMAPCommError(ccode));
 }
 
+void
+FolderRecentLock(void)
+{
+    XplMutexLock(Imap.recentLock);
+}
+
+void
+FolderRecentUnlock(void)
+{
+    XplMutexUnlock(Imap.recentLock);
+}
+
 __inline static long
 FolderWatchAdd(Connection *storeConn, FolderInformation *folder, const char *eventString)
 {
@@ -368,9 +380,12 @@ FolderDeselect(ImapSession *session)
         ccode = FolderWatchRemove(session->store.conn, selectedFolder->info);
         EventsFree(&(selectedFolder->events));
 
-        if ((ccode == STATUS_CONTINUE) && !(selectedFolder->readOnly)) {
-            ccode = FolderSetRecentUid(session->store.conn, selectedFolder->info);
-        }
+        /*
+         * A read-write SELECT and every NEW-event batch claim \Recent when
+         * they load it.  Rewriting the threshold here from a stale
+         * session-local uidNext can move it backwards and let another
+         * session claim the same messages.
+         */
     }
 
     FolderClose(selectedFolder);
@@ -542,7 +557,10 @@ FolderSelect(ImapSession *session, char *folderName, BOOL readOnly)
 
     if ((ccode = FolderGetByName(session, folderName, &folder)) == STATUS_CONTINUE) {
         if ((ccode = FolderWatchAdd(session->store.conn, folder, "FLAGS MODIFIED NEW DELETED")) == STATUS_CONTINUE) {
-            if ((ccode = FolderOpen(session->store.conn, &session->folder.selected, folder, readOnly)) == STATUS_CONTINUE) {
+            if (!readOnly) FolderRecentLock();
+            ccode = FolderOpen(session->store.conn,
+                               &session->folder.selected, folder, readOnly);
+            if (ccode == STATUS_CONTINUE) {
                 /*
                  * A read-write SELECT claims the current \Recent set for this
                  * session.  Keep the session-local flags loaded by FolderOpen,
@@ -552,15 +570,18 @@ FolderSelect(ImapSession *session, char *folderName, BOOL readOnly)
                 if (!readOnly &&
                     (ccode = FolderSetRecentUid(session->store.conn, folder)) !=
                         STATUS_CONTINUE) {
+                    FolderRecentUnlock();
                     FolderClose(&session->folder.selected);
                     FolderWatchRemove(session->store.conn, folder);
                     EventsFree(&session->folder.selected.events);
                     return ccode;
                 }
+                if (!readOnly) FolderRecentUnlock();
                 session->client.state = STATE_SELECTED;
                 ImapSearchResultsReset(session);
                 return(STATUS_CONTINUE);
             }
+            if (!readOnly) FolderRecentUnlock();
             FolderWatchRemove(session->store.conn, folder);
             EventsFree(&(session->folder.selected.events));
         }
@@ -4465,6 +4486,7 @@ InitializeImapGlobals()
     /* Initialize the Busy List and Semaphore */
     Imap.list_Busy = NULL;
     XplOpenLocalSemaphore(Imap.sem_Busy, 1);
+    XplMutexInit(Imap.recentLock);
 
     /* Global allocations */
     BongoKeywordIndexCreateFromTable(Imap.command.index, ImapProtocolCommands, .name, TRUE);
@@ -4502,6 +4524,7 @@ FreeImapGlobals()
     CommandFetchCleanup();
     FreeReturnValueIndex(Imap.command.returnValueIndex);
     BongoKeywordIndexFree(Imap.command.index);
+    XplMutexDestroy(Imap.recentLock);
     return(TRUE);
 }
 

src/agents/imap/imapd.h

@@ -442,6 +442,7 @@ typedef struct {
 
     BongoList *list_Busy;      /* Singly linked list of sessions that we should update every 10 seconds */
     XplSemaphore sem_Busy;      /* Semaphore protecting the busy list */
+    XplMutex recentLock;        /* serializes the shared \Recent claim */
     
     void *logHandle;
     XplAtomic exiting;
@@ -463,6 +464,8 @@ long GetMessageRange(MessageInformation *message, unsigned long messageCount, ch
 
 long FolderOpen(Connection *storeConn, OpenedFolder *openFolder, FolderInformation *folder, BOOL readOnly);
 long FolderListLoad(ImapSession *session);
+void FolderRecentLock(void);
+void FolderRecentUnlock(void);
 long FolderListInitialize(ImapSession *session);
 long MessageListLoad(Connection *conn, OpenedFolder *selected);