Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat: Remove "jobs" from imap_markseen if folder doesn't exist (#5870)
Browse files Browse the repository at this point in the history
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.
iequidoo committed Dec 22, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent 3d5e442 commit 79d1cc6
Showing 6 changed files with 125 additions and 69 deletions.
3 changes: 2 additions & 1 deletion src/configure.rs
Original file line number Diff line number Diff line change
@@ -452,8 +452,9 @@ async fn configure(ctx: &Context, param: &EnteredLoginParam) -> Result<Configure
imap.configure_folders(ctx, &mut imap_session, create_mvbox)
.await?;

let create = true;
imap_session
.select_with_uidvalidity(ctx, "INBOX")
.select_with_uidvalidity(ctx, "INBOX", create)
.await
.context("could not read INBOX status")?;

7 changes: 5 additions & 2 deletions src/download.rs
Original file line number Diff line number Diff line change
@@ -3,7 +3,7 @@
use std::cmp::max;
use std::collections::BTreeMap;

use anyhow::{anyhow, bail, Result};
use anyhow::{anyhow, bail, ensure, Result};
use deltachat_derive::{FromSql, ToSql};
use serde::{Deserialize, Serialize};

@@ -201,7 +201,10 @@ impl Session {
bail!("Attempt to fetch UID 0");
}

self.select_with_uidvalidity(context, folder).await?;
ensure!(
self.select_with_uidvalidity(context, folder, false).await?,
"No folder {folder}"
);

// we are connected, and the folder is selected
info!(context, "Downloading message {}/{} fully...", folder, uid);
147 changes: 90 additions & 57 deletions src/imap.rs
Original file line number Diff line number Diff line change
@@ -13,7 +13,7 @@ use std::{
time::{Duration, UNIX_EPOCH},
};

use anyhow::{bail, format_err, Context as _, Result};
use anyhow::{bail, ensure, format_err, Context as _, Result};
use async_channel::Receiver;
use async_imap::types::{Fetch, Flag, Name, NameAttribute, UnsolicitedResponse};
use deltachat_contact_tools::ContactAddress;
@@ -540,10 +540,13 @@ impl Imap {
return Ok(false);
}

session
.select_with_uidvalidity(context, folder)
if !session
.select_with_uidvalidity(context, folder, false)
.await
.with_context(|| format!("Failed to select folder {folder:?}"))?;
.with_context(|| format!("Failed to select folder {folder:?}"))?
{
return Ok(false);
}

if !session.new_mail && !fetch_existing_msgs {
info!(context, "No new emails in folder {folder:?}.");
@@ -835,44 +838,47 @@ impl Session {
folder: &str,
folder_meaning: FolderMeaning,
) -> 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
@@ -1039,7 +1045,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() {
@@ -1133,30 +1142,40 @@ 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
.transaction(|transaction| {
let mut stmt =
transaction.prepare("DELETE FROM imap_markseen WHERE id = ?")?;
for rowid in rowid_set {
stmt.execute((rowid,))?;
}
Ok(())
})
.await
.context("Cannot remove messages marked as seen from imap_markseen table")?;
}
context
.sql
.transaction(|transaction| {
let mut stmt =
transaction.prepare("DELETE FROM imap_markseen WHERE id = ?")?;
for rowid in rowid_set {
stmt.execute((rowid,))?;
}
Ok(())
})
.await
.context("Cannot remove messages marked as seen from imap_markseen table")?;
}

Ok(())
@@ -1172,9 +1191,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
@@ -1630,7 +1653,7 @@ fn format_setmetadata(folder: &str, device_token: &str) -> String {
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<()> {
@@ -1677,7 +1700,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));
}
}
@@ -1688,7 +1715,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));
@@ -2539,10 +2569,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)
4 changes: 3 additions & 1 deletion src/imap/idle.rs
Original file line number Diff line number Diff line change
@@ -29,7 +29,9 @@ impl Session {
) -> Result<Self> {
use futures::future::FutureExt;

self.select_with_uidvalidity(context, folder).await?;
let create = true;
self.select_with_uidvalidity(context, folder, create)
.await?;

if self.drain_unsolicited_responses(context)? {
self.new_mail = true;
3 changes: 3 additions & 0 deletions src/imap/scan_folders.rs
Original file line number Diff line number Diff line change
@@ -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() {
@@ -91,6 +93,7 @@ impl Imap {
}
}

info!(context, "Found folders: {folder_names:?}.");
last_scan.replace(tools::Time::now());
Ok(true)
}
30 changes: 22 additions & 8 deletions src/imap/select_folder.rs
Original file line number Diff line number Diff line change
@@ -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<bool> {
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)
}
}

0 comments on commit 79d1cc6

Please sign in to comment.