feat: protect the Date header

We do not try to delete resent messages
anymore. Previously resent messages
were distinguised by having duplicate Message-ID,
but future Date, but now we need to download
the message before we even see the Date.
We now move the message to the destination folder
but do not fetch it.

It may not be a good idea to delete
the duplicate in multi-device setups anyway,
because the device which has a message
may delete the duplicate of a message
the other device missed.

To avoid triggering IMAP move loop
described in the comments
we now only move the messages
from INBOX and Spam folders.

Author: link2xt

python/tests/test_1_online.py

@@ -333,7 +333,7 @@ def test_move_works(acfactory):
 
 
 def test_move_avoids_loop(acfactory):
-    """Test that the message is only moved once.
+    """Test that the message is only moved from INBOX to DeltaChat.
 
     This is to avoid busy loop if moved message reappears in the Inbox
     or some scanned folder later.
@@ -344,27 +344,43 @@ def test_move_avoids_loop(acfactory):
     ac1 = acfactory.new_online_configuring_account()
     ac2 = acfactory.new_online_configuring_account(mvbox_move=True)
     acfactory.bring_accounts_online()
+
+    # Create INBOX.DeltaChat folder and make sure
+    # it is detected by full folder scan.
+    ac2.direct_imap.create_folder("INBOX.DeltaChat")
+    ac2.stop_io()
+    ac2.start_io()
+    ac2._evtracker.get_info_contains("Found folders:")  #  Wait until the end of folder scan.
+
     ac1_chat = acfactory.get_accepted_chat(ac1, ac2)
     ac1_chat.send_text("Message 1")
 
     # Message is moved to the DeltaChat folder and downloaded.
     ac2_msg1 = ac2._evtracker.wait_next_incoming_message()
     assert ac2_msg1.text == "Message 1"
 
-    # Move the message to the INBOX again.
+    # Move the message to the INBOX.DeltaChat again.
+    # We assume that test server uses "." as the delimiter.
     ac2.direct_imap.select_folder("DeltaChat")
-    ac2.direct_imap.conn.move(["*"], "INBOX")
+    ac2.direct_imap.conn.move(["*"], "INBOX.DeltaChat")
 
     ac1_chat.send_text("Message 2")
     ac2_msg2 = ac2._evtracker.wait_next_incoming_message()
     assert ac2_msg2.text == "Message 2"
 
-    # Check that Message 1 is still in the INBOX folder
+    # Stop and start I/O to trigger folder scan.
+    ac2.stop_io()
+    ac2.start_io()
+    ac2._evtracker.get_info_contains("Found folders:")  #  Wait until the end of folder scan.
+
+    # Check that Message 1 is still in the INBOX.DeltaChat folder
     # and Message 2 is in the DeltaChat folder.
     ac2.direct_imap.select_folder("INBOX")
-    assert len(ac2.direct_imap.get_all_messages()) == 1
+    assert len(ac2.direct_imap.get_all_messages()) == 0
     ac2.direct_imap.select_folder("DeltaChat")
     assert len(ac2.direct_imap.get_all_messages()) == 1
+    ac2.direct_imap.select_folder("INBOX.DeltaChat")
+    assert len(ac2.direct_imap.get_all_messages()) == 1
 
 
 def test_move_works_on_self_sent(acfactory):
@@ -471,14 +487,19 @@ def test_resend_message(acfactory, lp):
     lp.sec("ac2: receive message")
     msg_in = ac2._evtracker.wait_next_incoming_message()
     assert msg_in.text == "message"
-    chat2 = msg_in.chat
-    chat2_msg_cnt = len(chat2.get_messages())
 
     lp.sec("ac1: resend message")
     ac1.resend_messages([msg_in])
 
-    lp.sec("ac2: check that message is deleted")
-    ac2._evtracker.get_matching("DC_EVENT_IMAP_MESSAGE_DELETED")
+    lp.sec("ac1: send another message")
+    chat1.send_text("another message")
+
+    lp.sec("ac2: receive another message")
+    msg_in = ac2._evtracker.wait_next_incoming_message()
+    assert msg_in.text == "another message"
+    chat2 = msg_in.chat
+    chat2_msg_cnt = len(chat2.get_messages())
+
     assert len(chat2.get_messages()) == chat2_msg_cnt
 
 
@@ -1882,8 +1903,8 @@ def test_group_quote(acfactory, lp):
         (
             "xyz",
             True,
-            "DeltaChat",
-        ),  # ...emails are found in a random folder and moved to DeltaChat
+            "xyz",
+        ),  # ...emails are found in a random folder and downloaded without moving
         (
             "Spam",
             False,

src/config.rs

@@ -735,9 +735,6 @@ impl Context {
             _ => Default::default(),
         };
         self.set_config_internal(key, value).await?;
-        if key == Config::SentboxWatch {
-            self.last_full_folder_scan.lock().await.take();
-        }
         Ok(())
     }
 

src/context.rs

@@ -263,8 +263,6 @@ pub struct InnerContext {
     /// IMAP METADATA.
     pub(crate) metadata: RwLock<Option<ServerMetadata>>,
 
-    pub(crate) last_full_folder_scan: Mutex<Option<tools::Time>>,
-
     /// ID for this `Context` in the current process.
     ///
     /// This allows for multiple `Context`s open in a single process where each context can
@@ -445,7 +443,6 @@ impl Context {
             server_id: RwLock::new(None),
             metadata: RwLock::new(None),
             creation_time: tools::Time::now(),
-            last_full_folder_scan: Mutex::new(None),
             last_error: parking_lot::RwLock::new("".to_string()),
             debug_logging: std::sync::RwLock::new(None),
             push_subscriber,

src/download.rs

@@ -161,25 +161,18 @@ pub(crate) async fn download_msg(
             |row| {
                 let server_uid: u32 = row.get(0)?;
                 let server_folder: String = row.get(1)?;
-                let uidvalidity: u32 = row.get(2)?;
-                Ok((server_uid, server_folder, uidvalidity))
+                Ok((server_uid, server_folder))
             },
         )
         .await?;
 
-    let Some((server_uid, server_folder, uidvalidity)) = row else {
+    let Some((server_uid, server_folder)) = row else {
         // No IMAP record found, we don't know the UID and folder.
         return Err(anyhow!("Call download_full() again to try over."));
     };
 
     session
-        .fetch_single_msg(
-            context,
-            &server_folder,
-            uidvalidity,
-            server_uid,
-            msg.rfc724_mid.clone(),
-        )
+        .fetch_single_msg(context, &server_folder, server_uid, msg.rfc724_mid.clone())
         .await?;
     Ok(())
 }
@@ -193,7 +186,6 @@ impl Session {
         &mut self,
         context: &Context,
         folder: &str,
-        uidvalidity: u32,
         uid: u32,
         rfc724_mid: String,
     ) -> Result<()> {
@@ -213,14 +205,7 @@ impl Session {
         let mut uid_message_ids: BTreeMap<u32, String> = BTreeMap::new();
         uid_message_ids.insert(uid, rfc724_mid);
         let (last_uid, _received) = self
-            .fetch_many_msgs(
-                context,
-                folder,
-                uidvalidity,
-                vec![uid],
-                &uid_message_ids,
-                false,
-            )
+            .fetch_many_msgs(context, folder, vec![uid], &uid_message_ids, false)
             .await?;
         if last_uid.is_none() {
             bail!("Failed to fetch UID {uid}");

src/imap.rs

@@ -581,52 +581,26 @@ impl Imap {
 
             // Determine the target folder where the message should be moved to.
             //
-            // If we have seen the message on the IMAP server before, do not move it.
+            // We only move the messages from the INBOX folder.
             // This is required to avoid infinite MOVE loop on IMAP servers
             // that alias `DeltaChat` folder to other names.
             // For example, some Dovecot servers alias `DeltaChat` folder to `INBOX.DeltaChat`.
-            // In this case Delta Chat configured with `DeltaChat` as the destination folder
-            // would detect messages in the `INBOX.DeltaChat` folder
-            // and try to move them to the `DeltaChat` folder.
-            // Such move to the same folder results in the messages
-            // getting a new UID, so the messages will be detected as new
+            // In this case moving from `INBOX.DeltaChat` to `DeltaChat`
+            // results in the messages getting a new UID,
+            // so the messages will be detected as new
             // in the `INBOX.DeltaChat` folder again.
             let _target;
             let target = if let Some(message_id) = &message_id {
                 let msg_info =
                     message::rfc724_mid_exists_ex(context, message_id, "deleted=1").await?;
-                let delete = if let Some((_, _, true)) = msg_info {
+                let delete = if let Some((_, true)) = msg_info {
                     info!(context, "Deleting locally deleted message {message_id}.");
                     true
-                } else if let Some((_, ts_sent_old, _)) = msg_info {
-                    let is_chat_msg = headers.get_header_value(HeaderDef::ChatVersion).is_some();
-                    let ts_sent = headers
-                        .get_header_value(HeaderDef::Date)
-                        .and_then(|v| mailparse::dateparse(&v).ok())
-                        .unwrap_or_default();
-                    let is_dup = is_dup_msg(is_chat_msg, ts_sent, ts_sent_old);
-                    if is_dup {
-                        info!(context, "Deleting duplicate message {message_id}.");
-                    }
-                    is_dup
                 } else {
                     false
                 };
                 if delete {
                     &delete_target
-                } else if context
-                    .sql
-                    .exists(
-                        "SELECT COUNT (*) FROM imap WHERE rfc724_mid=?",
-                        (message_id,),
-                    )
-                    .await?
-                {
-                    info!(
-                        context,
-                        "Not moving the message {} that we have seen before.", &message_id
-                    );
-                    folder
                 } else {
                     _target = target_folder(context, folder, folder_meaning, &headers).await?;
                     &_target
@@ -707,7 +681,6 @@ impl Imap {
                     .fetch_many_msgs(
                         context,
                         folder,
-                        uid_validity,
                         uids_fetch_in_batch.split_off(0),
                         &uid_message_ids,
                         fetch_partially,
@@ -1305,7 +1278,6 @@ impl Session {
         &mut self,
         context: &Context,
         folder: &str,
-        uidvalidity: u32,
         request_uids: Vec<u32>,
         uid_message_ids: &BTreeMap<u32, String>,
         fetch_partially: bool,
@@ -1433,18 +1405,7 @@ impl Session {
                     context,
                     "Passing message UID {} to receive_imf().", request_uid
                 );
-                match receive_imf_inner(
-                    context,
-                    folder,
-                    uidvalidity,
-                    request_uid,
-                    rfc724_mid,
-                    body,
-                    is_seen,
-                    partial,
-                )
-                .await
-                {
+                match receive_imf_inner(context, rfc724_mid, body, is_seen, partial).await {
                     Ok(received_msg) => {
                         if let Some(m) = received_msg {
                             received_msgs.push(m);
@@ -1952,7 +1913,9 @@ pub async fn target_folder_cfg(
 
     if folder_meaning == FolderMeaning::Spam {
         spam_target_folder_cfg(context, headers).await
-    } else if needs_move_to_mvbox(context, headers).await? {
+    } else if folder_meaning == FolderMeaning::Inbox
+        && needs_move_to_mvbox(context, headers).await?
+    {
         Ok(Some(Config::ConfiguredMvboxFolder))
     } else {
         Ok(None)
@@ -2121,7 +2084,9 @@ fn get_folder_meaning_by_name(folder_name: &str) -> FolderMeaning {
     ];
     let lower = folder_name.to_lowercase();
 
-    if SENT_NAMES.iter().any(|s| s.to_lowercase() == lower) {
+    if lower == "inbox" {
+        FolderMeaning::Inbox
+    } else if SENT_NAMES.iter().any(|s| s.to_lowercase() == lower) {
         FolderMeaning::Sent
     } else if SPAM_NAMES.iter().any(|s| s.to_lowercase() == lower) {
         FolderMeaning::Spam
@@ -2280,15 +2245,6 @@ pub(crate) async fn prefetch_should_download(
     Ok(should_download)
 }
 
-/// Returns whether a message is a duplicate (resent message).
-pub(crate) fn is_dup_msg(is_chat_msg: bool, ts_sent: i64, ts_sent_old: i64) -> bool {
-    // If the existing message has timestamp_sent == 0, that means we don't know its actual sent
-    // timestamp, so don't delete the new message. E.g. outgoing messages have zero timestamp_sent
-    // because they are stored to the db before sending. Also consider as duplicates only messages
-    // with greater timestamp to avoid deleting both messages in a multi-device setting.
-    is_chat_msg && ts_sent_old != 0 && ts_sent > ts_sent_old
-}
-
 /// Marks messages in `msgs` table as seen, searching for them by UID.
 ///
 /// Returns updated chat ID if any message was marked as seen.

src/imap/imap_tests.rs

@@ -180,7 +180,7 @@ const COMBINATIONS_ACCEPTED_CHAT: &[(&str, bool, bool, &str)] = &[
     ("Sent", false, false, "Sent"),
     ("Sent", false, true, "Sent"),
     ("Sent", true, false, "Sent"),
-    ("Sent", true, true, "DeltaChat"),
+    ("Sent", true, true, "Sent"),
     ("Spam", false, false, "INBOX"), // Move classical emails in accepted chats from Spam to Inbox, not 100% sure on this, we could also just never move non-chat-msgs
     ("Spam", false, true, "INBOX"),
     ("Spam", true, false, "INBOX"), // Move classical emails in accepted chats from Spam to Inbox, not 100% sure on this, we could also just never move non-chat-msgs
@@ -196,7 +196,7 @@ const COMBINATIONS_REQUEST: &[(&str, bool, bool, &str)] = &[
     ("Sent", false, false, "Sent"),
     ("Sent", false, true, "Sent"),
     ("Sent", true, false, "Sent"),
-    ("Sent", true, true, "DeltaChat"),
+    ("Sent", true, true, "Sent"),
     ("Spam", false, false, "Spam"),
     ("Spam", false, true, "INBOX"),
     ("Spam", true, false, "Spam"),

src/imap/scan_folders.rs

@@ -18,7 +18,7 @@ impl Imap {
     ) -> Result<bool> {
         // First of all, debounce to once per minute:
         {
-            let mut last_scan = context.last_full_folder_scan.lock().await;
+            let mut last_scan = session.last_full_folder_scan.lock().await;
             if let Some(last_scan) = *last_scan {
                 let elapsed_secs = time_elapsed(&last_scan).as_secs();
                 let debounce_secs = context

src/imap/session.rs

@@ -5,9 +5,11 @@ use anyhow::{Context as _, Result};
 use async_imap::types::Mailbox;
 use async_imap::Session as ImapSession;
 use futures::TryStreamExt;
+use tokio::sync::Mutex;
 
 use crate::imap::capabilities::Capabilities;
 use crate::net::session::SessionStream;
+use crate::tools;
 
 /// Prefetch:
 /// - Message-ID to check if we already have the message.
@@ -40,6 +42,8 @@ pub(crate) struct Session {
 
     pub selected_folder_needs_expunge: bool,
 
+    pub(crate) last_full_folder_scan: Mutex<Option<tools::Time>>,
+
     /// True if currently selected folder has new messages.
     ///
     /// Should be false if no folder is currently selected.
@@ -71,6 +75,7 @@ impl Session {
             selected_folder: None,
             selected_mailbox: None,
             selected_folder_needs_expunge: false,
+            last_full_folder_scan: Mutex::new(None),
             new_mail: false,
         }
     }