fix: do not miss new messages while expunging the folder

src/download.rs

@@ -184,7 +184,7 @@ impl Session {
             bail!("Attempt to fetch UID 0");
         }
 
-        self.select_folder(context, folder).await?;
+        self.select_with_uidvalidity(context, folder).await?;
 
         // we are connected, and the folder is selected
         info!(context, "Downloading message {}/{} fully...", folder, uid);

src/imap.rs

@@ -543,15 +543,16 @@ impl Imap {
             return Ok(false);
         }
 
-        let new_emails = session
+        session
             .select_with_uidvalidity(context, folder)
             .await
             .with_context(|| format!("Failed to select folder {folder:?}"))?;
 
-        if !new_emails && !fetch_existing_msgs {
+        if !session.new_mail && !fetch_existing_msgs {
             info!(context, "No new emails in folder {folder:?}.");
             return Ok(false);
         }
+        session.new_mail = false;
 
         let uid_validity = get_uidvalidity(context, folder).await?;
         let old_uid_next = get_uid_next(context, folder).await?;
@@ -838,7 +839,7 @@ impl Session {
         // Collect pairs of UID and Message-ID.
         let mut msgs = BTreeMap::new();
 
-        self.select_folder(context, folder).await?;
+        self.select_with_uidvalidity(context, folder).await?;
 
         let mut list = self
             .uid_fetch("1:*", RFC724MID_UID)
@@ -1039,7 +1040,7 @@ impl Session {
             // MOVE/DELETE operations. This does not result in multiple SELECT commands
             // being sent because `select_folder()` does nothing if the folder is already
             // selected.
-            self.select_folder(context, folder).await?;
+            self.select_with_uidvalidity(context, folder).await?;
 
             // Empty target folder name means messages should be deleted.
             if target.is_empty() {
@@ -1087,7 +1088,7 @@ impl Session {
             .await?;
 
         for (folder, rowid_set, uid_set) in UidGrouper::from(rows) {
-            self.select_folder(context, &folder)
+            self.select_with_uidvalidity(context, &folder)
                 .await
                 .context("failed to select folder")?;
 
@@ -1131,7 +1132,7 @@ impl Session {
             return Ok(());
         }
 
-        self.select_folder(context, folder)
+        self.select_with_uidvalidity(context, folder)
             .await
             .context("failed to select folder")?;
 

src/imap/idle.rs

@@ -29,9 +29,13 @@ impl Session {
     ) -> Result<Self> {
         use futures::future::FutureExt;
 
-        self.select_folder(context, folder).await?;
+        self.select_with_uidvalidity(context, folder).await?;
 
         if self.server_sent_unsolicited_exists(context)? {
+            self.new_mail = true;
+        }
+
+        if self.new_mail {
             return Ok(self);
         }
 
@@ -92,6 +96,9 @@ impl Session {
         session.as_mut().set_read_timeout(Some(IMAP_TIMEOUT));
         self.inner = session;
 
+        // Fetch mail once we exit IDLE.
+        self.new_mail = true;
+
         Ok(self)
     }
 }

src/imap/select_folder.rs

@@ -10,12 +10,6 @@ type Result<T> = std::result::Result<T, Error>;
 
 #[derive(Debug, thiserror::Error)]
 pub enum Error {
-    #[error("IMAP Connection Lost or no connection established")]
-    ConnectionLost,
-
-    #[error("IMAP Folder name invalid: {0}")]
-    BadFolderName(String),
-
     #[error("Got a NO response when trying to select {0}, usually this means that it doesn't exist: {1}")]
     NoFolder(String, String),
 
@@ -33,7 +27,8 @@ impl ImapSession {
     /// Issues a CLOSE command if selected folder needs expunge,
     /// i.e. if Delta Chat marked a message there as deleted previously.
     ///
-    /// CLOSE is considerably faster than an EXPUNGE, see
+    /// CLOSE is considerably faster than an EXPUNGE
+    /// because no EXPUNGE responses are sent, see
     /// <https://tools.ietf.org/html/rfc3501#section-6.4.2>
     pub(super) async fn maybe_close_folder(&mut self, context: &Context) -> anyhow::Result<()> {
         if let Some(folder) = &self.selected_folder {
@@ -44,6 +39,7 @@ impl ImapSession {
                 info!(context, "close/expunge succeeded");
                 self.selected_folder = None;
                 self.selected_folder_needs_expunge = false;
+                self.new_mail = false;
             }
         }
         Ok(())
@@ -52,11 +48,7 @@ impl ImapSession {
     /// Selects a folder, possibly updating uid_validity and, if needed,
     /// expunging the folder to remove delete-marked messages.
     /// Returns whether a new folder was selected.
-    pub(crate) async fn select_folder(
-        &mut self,
-        context: &Context,
-        folder: &str,
-    ) -> Result<NewlySelected> {
+    async fn select_folder(&mut self, context: &Context, folder: &str) -> Result<NewlySelected> {
         // if there is a new folder and the new folder is equal to the selected one, there's nothing to do.
         // if there is _no_ new folder, we continue as we might want to expunge below.
         if let Some(selected_folder) = &self.selected_folder {
@@ -85,10 +77,6 @@ impl ImapSession {
                 self.selected_mailbox = Some(mailbox);
                 Ok(NewlySelected::Yes)
             }
-            Err(async_imap::error::Error::ConnectionLost) => Err(Error::ConnectionLost),
-            Err(async_imap::error::Error::Validate(_)) => {
-                Err(Error::BadFolderName(folder.to_string()))
-            }
             Err(async_imap::error::Error::No(response)) => {
                 Err(Error::NoFolder(folder.to_string(), response))
             }
@@ -128,13 +116,14 @@ impl ImapSession {
     /// 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.
     ///
-    /// Returns Result<new_emails> (i.e. whether new emails arrived),
-    /// if in doubt, returns new_emails=true so emails are fetched.
+    /// Updates `self.new_mail` if folder was previously unselected
+    /// and new mails are detected after selecting,
+    /// i.e. UIDNEXT advanced while the folder was closed.
     pub(crate) async fn select_with_uidvalidity(
         &mut self,
         context: &Context,
         folder: &str,
-    ) -> Result<bool> {
+    ) -> Result<()> {
         let newly_selected = self
             .select_or_create_folder(context, folder)
             .await
@@ -191,28 +180,26 @@ impl ImapSession {
         mailbox.uid_next = new_uid_next;
 
         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(new_uid_next) = new_uid_next {
-                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?;
+            if newly_selected == NewlySelected::Yes {
+                if let Some(new_uid_next) = new_uid_next {
+                    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?;
+                    }
+
+                    // If UIDNEXT changed, there are new emails.
+                    self.new_mail |= new_uid_next != old_uid_next;
+                } else {
+                    warn!(context, "Folder {folder} was just selected but we failed to determine UIDNEXT, assume that it has new mail.");
+                    self.new_mail = true;
                 }
-                new_uid_next != old_uid_next // If UIDNEXT changed, there are new emails
-            } else {
-                // We have no UIDNEXT and if in doubt, return true.
-                true
-            };
+            }
 
-            return Ok(new_emails);
+            return Ok(());
         }
 
         // UIDVALIDITY is modified, reset highest seen MODSEQ.
@@ -223,6 +210,7 @@ impl ImapSession {
         let new_uid_next = new_uid_next.unwrap_or_default();
         set_uid_next(context, folder, new_uid_next).await?;
         set_uidvalidity(context, folder, new_uid_validity).await?;
+        self.new_mail = true;
 
         // Collect garbage entries in `imap` table.
         context
@@ -245,7 +233,7 @@ impl ImapSession {
             old_uid_next,
             old_uid_validity,
         );
-        Ok(false)
+        Ok(())
     }
 }
 

src/imap/session.rs

@@ -40,6 +40,11 @@ pub(crate) struct Session {
     pub selected_mailbox: Option<Mailbox>,
 
     pub selected_folder_needs_expunge: bool,
+
+    /// True if currently selected folder has new messages.
+    ///
+    /// Should be false if no folder is currently selected.
+    pub new_mail: bool,
 }
 
 impl Deref for Session {
@@ -67,6 +72,7 @@ impl Session {
             selected_folder: None,
             selected_mailbox: None,
             selected_folder_needs_expunge: false,
+            new_mail: false,
         }
     }