From 98d38b4b27a342dc7ee8b1a95d27731cb2a51d4b Mon Sep 17 00:00:00 2001 From: iequidoo Date: Sun, 18 Aug 2024 17:49:29 -0300 Subject: [PATCH] feat: Remove "jobs" from imap_markseen if folder doesn't exist (#5870) Add a `create` param to `select_with_uidvalidity()` instead of always trying to create the folder and return `Ok(false)` from it if the folder doesn't exist and shouldn't be created, and handle this in `store_seen_flags_on_imap()` by just removing "jobs" from the `imap_markseen` table. Also don't create the folder in other code paths where it's not necessary. --- src/configure.rs | 3 +- src/download.rs | 7 +- src/imap.rs | 145 +++++++++++++++++++++++--------------- src/imap/idle.rs | 4 +- src/imap/scan_folders.rs | 3 + src/imap/select_folder.rs | 30 +++++--- 6 files changed, 124 insertions(+), 68 deletions(-) diff --git a/src/configure.rs b/src/configure.rs index d836a11eb7..fb4c3596ef 100644 --- a/src/configure.rs +++ b/src/configure.rs @@ -459,8 +459,9 @@ async fn configure(ctx: &Context, param: &EnteredLoginParam) -> Result Result<()> { + let uid_validity; // Collect pairs of UID and Message-ID. let mut msgs = BTreeMap::new(); - self.select_with_uidvalidity(context, folder).await?; + if self.select_with_uidvalidity(context, folder, false).await? { + let mut list = self + .uid_fetch("1:*", RFC724MID_UID) + .await + .with_context(|| format!("can't resync folder {folder}"))?; + while let Some(fetch) = list.try_next().await? { + let headers = match get_fetch_headers(&fetch) { + Ok(headers) => headers, + Err(err) => { + warn!(context, "Failed to parse FETCH headers: {}", err); + continue; + } + }; + let message_id = prefetch_get_message_id(&headers); - let mut list = self - .uid_fetch("1:*", RFC724MID_UID) - .await - .with_context(|| format!("can't resync folder {folder}"))?; - while let Some(fetch) = list.try_next().await? { - let headers = match get_fetch_headers(&fetch) { - Ok(headers) => headers, - Err(err) => { - warn!(context, "Failed to parse FETCH headers: {}", err); - continue; + if let (Some(uid), Some(rfc724_mid)) = (fetch.uid, message_id) { + msgs.insert( + uid, + ( + rfc724_mid, + target_folder(context, folder, folder_meaning, &headers).await?, + ), + ); } - }; - let message_id = prefetch_get_message_id(&headers); - - if let (Some(uid), Some(rfc724_mid)) = (fetch.uid, message_id) { - msgs.insert( - uid, - ( - rfc724_mid, - target_folder(context, folder, folder_meaning, &headers).await?, - ), - ); } - } - info!( - context, - "Resync: collected {} message IDs in folder {}", - msgs.len(), - folder, - ); + info!( + context, + "resync_folder_uids: Collected {} message IDs in {folder}.", + msgs.len(), + ); - let uid_validity = get_uidvalidity(context, folder).await?; + uid_validity = get_uidvalidity(context, folder).await?; + } else { + warn!(context, "resync_folder_uids: No folder {folder}."); + uid_validity = 0; + } // Write collected UIDs to SQLite database. context @@ -1016,7 +1022,10 @@ 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_with_uidvalidity(context, folder).await?; + ensure!( + self.select_with_uidvalidity(context, folder, false).await?, + "No folder {folder}" + ); // Empty target folder name means messages should be deleted. if target.is_empty() { @@ -1110,29 +1119,39 @@ impl Session { .await?; for (folder, rowid_set, uid_set) in UidGrouper::from(rows) { - if let Err(err) = self.select_with_uidvalidity(context, &folder).await { - warn!(context, "store_seen_flags_on_imap: Failed to select {folder}, will retry later: {err:#}."); + let is_ok = match self.select_with_uidvalidity(context, &folder, false).await { + Err(err) => { + warn!( + context, + "store_seen_flags_on_imap: Failed to select {folder}, will retry later: {err:#}."); + continue; + } + Ok(is_ok) => is_ok, + }; + if !is_ok { + warn!(context, "store_seen_flags_on_imap: No folder {folder}."); } else if let Err(err) = self.add_flag_finalized_with_set(&uid_set, "\\Seen").await { warn!( context, "Cannot mark messages {uid_set} in {folder} as seen, will retry later: {err:#}."); + continue; } else { info!( context, "Marked messages {} in folder {} as seen.", uid_set, folder ); - context - .sql - .execute( - &format!( - "DELETE FROM imap_markseen WHERE id IN ({})", - sql::repeat_vars(rowid_set.len()) - ), - rusqlite::params_from_iter(rowid_set), - ) - .await - .context("cannot remove messages marked as seen from imap_markseen table")?; } + context + .sql + .execute( + &format!( + "DELETE FROM imap_markseen WHERE id IN ({})", + sql::repeat_vars(rowid_set.len()) + ), + rusqlite::params_from_iter(rowid_set), + ) + .await + .context("cannot remove messages marked as seen from imap_markseen table")?; } Ok(()) @@ -1148,9 +1167,13 @@ impl Session { return Ok(()); } - self.select_with_uidvalidity(context, folder) + if !self + .select_with_uidvalidity(context, folder, false) .await - .context("failed to select folder")?; + .context("failed to select folder")? + { + return Ok(()); + } let mailbox = self .selected_mailbox @@ -1549,7 +1572,7 @@ impl Session { impl Session { /// Returns success if we successfully set the flag or we otherwise /// think add_flag should not be retried: Disconnection during setting - /// the flag, or other imap-errors, returns true as well. + /// the flag, or other imap-errors, returns Ok as well. /// /// Returning error means that the operation can be retried. async fn add_flag_finalized_with_set(&mut self, uid_set: &str, flag: &str) -> Result<()> { @@ -1596,7 +1619,11 @@ impl Session { self.close().await?; // Before moving emails to the mvbox we need to remember its UIDVALIDITY, otherwise // emails moved before that wouldn't be fetched but considered "old" instead. - self.select_with_uidvalidity(context, folder).await?; + ensure!( + self.select_with_uidvalidity(context, folder, false).await?, + "No MVBOX folder {:?}??", + &folder + ); return Ok(Some(folder)); } } @@ -1607,7 +1634,10 @@ impl Session { // Some servers require namespace-style folder names like "INBOX.DeltaChat", so we try all // the variants here. for folder in folders { - match self.select_with_uidvalidity(context, folder).await { + match self + .select_with_uidvalidity(context, folder, create_mvbox) + .await + { Ok(_) => { info!(context, "MVBOX-folder {} created.", folder); return Ok(Some(folder)); @@ -2444,10 +2474,13 @@ async fn add_all_recipients_as_contacts( ); return Ok(()); }; - session - .select_with_uidvalidity(context, &mailbox) + if !session + .select_with_uidvalidity(context, &mailbox, false) .await - .with_context(|| format!("could not select {mailbox}"))?; + .with_context(|| format!("could not select {mailbox}"))? + { + return Ok(()); + } let recipients = session .get_all_recipients(context) diff --git a/src/imap/idle.rs b/src/imap/idle.rs index 892e8757b8..81e7b14ffa 100644 --- a/src/imap/idle.rs +++ b/src/imap/idle.rs @@ -30,7 +30,9 @@ impl Session { ) -> Result { use futures::future::FutureExt; - self.select_with_uidvalidity(context, folder).await?; + let create = true; + self.select_with_uidvalidity(context, folder, create) + .await?; if self.server_sent_unsolicited_exists(context)? { self.new_mail = true; diff --git a/src/imap/scan_folders.rs b/src/imap/scan_folders.rs index a4e17d04d6..ab17c0950c 100644 --- a/src/imap/scan_folders.rs +++ b/src/imap/scan_folders.rs @@ -34,6 +34,7 @@ impl Imap { let watched_folders = get_watched_folders(context).await?; let mut folder_configs = BTreeMap::new(); + let mut folder_names = Vec::new(); for folder in folders { let folder_meaning = get_folder_meaning_by_attrs(folder.attributes()); @@ -44,6 +45,7 @@ impl Imap { // already been moved and left it in the inbox. continue; } + folder_names.push(folder.name().to_string()); let folder_name_meaning = get_folder_meaning_by_name(folder.name()); if let Some(config) = folder_meaning.to_config() { @@ -101,6 +103,7 @@ impl Imap { } } + info!(context, "Found folders: {folder_names:?}."); last_scan.replace(tools::Time::now()); Ok(true) } diff --git a/src/imap/select_folder.rs b/src/imap/select_folder.rs index 1c2c64d3ab..c4849ada12 100644 --- a/src/imap/select_folder.rs +++ b/src/imap/select_folder.rs @@ -111,7 +111,8 @@ impl ImapSession { } } - /// Selects a folder and takes care of UIDVALIDITY changes. + /// Selects a folder optionally creating it and takes care of UIDVALIDITY changes. Returns false + /// iff `folder` doesn't exist. /// /// 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. @@ -123,11 +124,24 @@ impl ImapSession { &mut self, context: &Context, folder: &str, - ) -> Result<()> { - let newly_selected = self - .select_or_create_folder(context, folder) - .await - .with_context(|| format!("failed to select or create folder {folder}"))?; + create: bool, + ) -> Result { + let newly_selected = if create { + self.select_or_create_folder(context, folder) + .await + .with_context(|| format!("failed to select or create folder {folder}"))? + } else { + match self.select_folder(context, folder).await { + Ok(newly_selected) => newly_selected, + Err(err) => match err { + Error::NoFolder(..) => return Ok(false), + _ => { + return Err(err) + .with_context(|| format!("failed to select folder {folder}"))? + } + }, + } + }; let mailbox = self .selected_mailbox .as_mut() @@ -199,7 +213,7 @@ impl ImapSession { } } - return Ok(()); + return Ok(true); } // UIDVALIDITY is modified, reset highest seen MODSEQ. @@ -233,7 +247,7 @@ impl ImapSession { old_uid_next, old_uid_validity, ); - Ok(()) + Ok(true) } }