From 8dea1588ced5e8ce0738f7184264753de9a16c18 Mon Sep 17 00:00:00 2001 From: link2xt Date: Tue, 4 Jun 2024 18:03:24 +0000 Subject: [PATCH 1/3] refactor: remove unused select_folder::Error variants --- src/imap/select_folder.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/imap/select_folder.rs b/src/imap/select_folder.rs index b51196a8e6..baaae3d0bd 100644 --- a/src/imap/select_folder.rs +++ b/src/imap/select_folder.rs @@ -10,12 +10,6 @@ type Result = std::result::Result; #[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), @@ -85,10 +79,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)) } From feb3cff973fbc468b7ead1c6bd34a288f67c5a62 Mon Sep 17 00:00:00 2001 From: link2xt Date: Tue, 4 Jun 2024 21:26:59 +0000 Subject: [PATCH 2/3] docs(imap): document why CLOSE is faster than EXPUNGE --- src/imap/select_folder.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/imap/select_folder.rs b/src/imap/select_folder.rs index baaae3d0bd..c712325b5c 100644 --- a/src/imap/select_folder.rs +++ b/src/imap/select_folder.rs @@ -27,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 /// pub(super) async fn maybe_close_folder(&mut self, context: &Context) -> anyhow::Result<()> { if let Some(folder) = &self.selected_folder { From e598748e6d47aea0ebff16d3d332649807830cc8 Mon Sep 17 00:00:00 2001 From: link2xt Date: Tue, 4 Jun 2024 15:41:39 +0000 Subject: [PATCH 3/3] fix: do not miss new messages while expunging the folder This should fix flaky `test_verified_group_vs_delete_server_after`. --- src/download.rs | 2 +- src/imap.rs | 13 ++++----- src/imap/idle.rs | 9 ++++++- src/imap/select_folder.rs | 55 ++++++++++++++++++--------------------- src/imap/session.rs | 6 +++++ 5 files changed, 48 insertions(+), 37 deletions(-) diff --git a/src/download.rs b/src/download.rs index a90844d7c6..eb763eed84 100644 --- a/src/download.rs +++ b/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); diff --git a/src/imap.rs b/src/imap.rs index 5839a6097e..8341506879 100644 --- a/src/imap.rs +++ b/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")?; diff --git a/src/imap/idle.rs b/src/imap/idle.rs index b43cc441f7..0bb3afe770 100644 --- a/src/imap/idle.rs +++ b/src/imap/idle.rs @@ -29,9 +29,13 @@ impl Session { ) -> Result { 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) } } diff --git a/src/imap/select_folder.rs b/src/imap/select_folder.rs index c712325b5c..1c2c64d3ab 100644 --- a/src/imap/select_folder.rs +++ b/src/imap/select_folder.rs @@ -39,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(()) @@ -47,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 { + async fn select_folder(&mut self, context: &Context, folder: &str) -> Result { // 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 { @@ -119,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 (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 { + ) -> Result<()> { let newly_selected = self .select_or_create_folder(context, folder) .await @@ -182,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. @@ -214,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 @@ -236,7 +233,7 @@ impl ImapSession { old_uid_next, old_uid_validity, ); - Ok(false) + Ok(()) } } diff --git a/src/imap/session.rs b/src/imap/session.rs index b3f09af27f..f085bc7272 100644 --- a/src/imap/session.rs +++ b/src/imap/session.rs @@ -40,6 +40,11 @@ pub(crate) struct Session { pub selected_mailbox: Option, 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, } }