fix(imap): always advance expected UIDNEXT to avoid skipping IDLE in a loop

Ensure the client does not busy loop
skipping IDLE if UIDNEXT of the mailbox is higher than
the last seen UID plus 1, e.g. if the message
with UID=UIDNEXT-1 was deleted before we fetched it.

We do not fallback to UIDNEXT=1 anymore
if the STATUS command cannot determine UIDNEXT.
There are no known IMAP servers with broken STATUS so far.
This allows to guarantee that select_with_uidvalidity()
sets UIDNEXT for the mailbox and use it in fetch_new_messages()
to ensure that UIDNEXT always advances even
when there are no messages to fetch.

src/imap.rs

@@ -568,9 +568,14 @@ impl Imap {
         Ok(())
     }
 
-    /// Select a folder and take care of uidvalidity changes.
+    /// Selects a folder and takes care of UIDVALIDITY changes.
-    /// Also, when selecting a folder for the first time, sets the uid_next to the current
+    ///
+    /// When selecting a folder for the first time, sets the uid_next to the current
     /// mailbox.uid_next so that no old emails are fetched.
+    ///
+    /// Makes sure that UIDNEXT is known for `selected_mailbox`
+    /// and errors out if UIDNEXT cannot be determined.
+    ///
     /// Returns Result<new_emails> (i.e. whether new emails arrived),
     /// if in doubt, returns new_emails=true so emails are fetched.
     pub(crate) async fn select_with_uidvalidity(
@@ -591,45 +596,9 @@ impl Imap {
         let new_uid_validity = mailbox
             .uid_validity
             .with_context(|| format!("No UIDVALIDITY for folder {folder}"))?;
-
+        let new_uid_next = if let Some(uid_next) = mailbox.uid_next {
-        let old_uid_validity = get_uidvalidity(context, folder)
+            uid_next
-            .await
-            .with_context(|| format!("failed to get old UID validity for folder {folder}"))?;
-        let old_uid_next = get_uid_next(context, folder)
-            .await
-            .with_context(|| format!("failed to get old UID NEXT for folder {folder}"))?;
-
-        if new_uid_validity == old_uid_validity {
-            let new_emails = if newly_selected == NewlySelected::No {
-                // The folder was not newly selected i.e. no SELECT command was run. This means that mailbox.uid_next
-                // was not updated and may contain an incorrect value. So, just return true so that
-                // the caller tries to fetch new messages (we could of course run a SELECT command now, but trying to fetch
-                // new messages is only one command, just as a SELECT command)
-                true
-            } else if let Some(uid_next) = mailbox.uid_next {
-                if uid_next < old_uid_next {
-                    warn!(
-                        context,
-                        "The server illegally decreased the uid_next of folder {folder:?} from {old_uid_next} to {uid_next} without changing validity ({new_uid_validity}), resyncing UIDs...",
-                    );
-                    set_uid_next(context, folder, uid_next).await?;
-                    context.schedule_resync().await?;
-                }
-                uid_next != old_uid_next // If uid_next changed, there are new emails
         } else {
-                true // We have no uid_next and if in doubt, return true
-            };
-            return Ok(new_emails);
-        }
-
-        // UIDVALIDITY is modified, reset highest seen MODSEQ.
-        set_modseq(context, folder, 0).await?;
-
-        // ==============  uid_validity has changed or is being set the first time.  ==============
-
-        let new_uid_next = match mailbox.uid_next {
-            Some(uid_next) => uid_next,
-            None => {
             warn!(
                 context,
                 "SELECT response for IMAP folder {folder:?} has no UIDNEXT, fall back to STATUS command."
@@ -646,23 +615,51 @@ impl Imap {
             //
             // In particular, Winmail Pro Mail Server 5.1.0616
             // never returns UIDNEXT in SELECT response,
-                // but responds to "SELECT INBOX (UIDNEXT)" command.
+            // but responds to "STATUS INBOX (UIDNEXT)" command.
             let status = session
                 .inner
                 .status(folder, "(UIDNEXT)")
                 .await
                 .context("STATUS (UIDNEXT) error for {folder:?}")?;
 
-                if let Some(uid_next) = status.uid_next {
+            status
-                    uid_next
+                .uid_next
-                } else {
+                .context("STATUS {folder} (UIDNEXT) did not return UIDNEXT")?
-                    warn!(context, "STATUS {folder} (UIDNEXT) did not return UIDNEXT");
-
-                    // Set UIDNEXT to 1 as a last resort fallback.
-                    1
-                }
-            }
         };
+        mailbox.uid_next = Some(new_uid_next);
+
+        let old_uid_validity = get_uidvalidity(context, folder)
+            .await
+            .with_context(|| format!("failed to get old UID validity for folder {folder}"))?;
+        let old_uid_next = get_uid_next(context, folder)
+            .await
+            .with_context(|| format!("failed to get old UID NEXT for folder {folder}"))?;
+
+        if new_uid_validity == old_uid_validity {
+            let new_emails = if newly_selected == NewlySelected::No {
+                // The folder was not newly selected i.e. no SELECT command was run. This means that mailbox.uid_next
+                // was not updated and may contain an incorrect value. So, just return true so that
+                // the caller tries to fetch new messages (we could of course run a SELECT command now, but trying to fetch
+                // new messages is only one command, just as a SELECT command)
+                true
+            } else {
+                if new_uid_next < old_uid_next {
+                    warn!(
+                        context,
+                        "The server illegally decreased the uid_next of folder {folder:?} from {old_uid_next} to {new_uid_next} without changing validity ({new_uid_validity}), resyncing UIDs...",
+                    );
+                    set_uid_next(context, folder, new_uid_next).await?;
+                    context.schedule_resync().await?;
+                }
+                new_uid_next != old_uid_next // If UIDNEXT changed, there are new emails
+            };
+            return Ok(new_emails);
+        }
+
+        // UIDVALIDITY is modified, reset highest seen MODSEQ.
+        set_modseq(context, folder, 0).await?;
+
+        // ==============  uid_validity has changed or is being set the first time.  ==============
 
         set_uid_next(context, folder, new_uid_next).await?;
         set_uidvalidity(context, folder, new_uid_validity).await?;
@@ -867,14 +864,28 @@ impl Imap {
             uids_fetch_in_batch.push(uid);
         }
 
-        // determine which uid_next to use to update to
+        // Advance uid_next to the maximum of the largest known UID plus 1
-        // receive_imf() returns an `Err` value only on recoverable errors, otherwise it just logs an error.
+        // and mailbox UIDNEXT.
-        // `largest_uid_processed` is the largest uid where receive_imf() did NOT return an error.
+        // Largest known UID is normally less than UIDNEXT,
-
+        // but a message may have arrived between determining UIDNEXT
-        // So: Update the uid_next to the largest uid that did NOT recoverably fail. Not perfect because if there was
+        // and executing the FETCH command.
-        // another message afterwards that succeeded, we will not retry. The upside is that we will not retry an infinite amount of times.
+        let mailbox_uid_next = self
-        let largest_uid_without_errors = max(largest_uid_fetched, largest_uid_skipped.unwrap_or(0));
+            .session
-        let new_uid_next = largest_uid_without_errors + 1;
+            .as_ref()
+            .context("No IMAP session")?
+            .selected_mailbox
+            .as_ref()
+            .with_context(|| format!("Expected {folder:?} to be selected"))?
+            .uid_next
+            .with_context(|| {
+                format!(
+                    "Expected UIDNEXT to be determined for {folder:?} by select_with_uidvalidity"
+                )
+            })?;
+        let new_uid_next = max(
+            max(largest_uid_fetched, largest_uid_skipped.unwrap_or(0)) + 1,
+            mailbox_uid_next,
+        );
 
         if new_uid_next > old_uid_next {
             set_uid_next(context, folder, new_uid_next).await?;

src/imap/idle.rs

@@ -53,7 +53,7 @@ impl Session {
             if uid_next > expected_uid_next {
                 info!(
                     context,
-                    "Skipping IDLE because UIDNEXT indicates there are new messages."
+                    "Skipping IDLE on {folder:?} because UIDNEXT {uid_next}>{expected_uid_next} indicates there are new messages."
                 );
                 return Ok((self, info));
             }