fix: Receiving of re-sent locally deleted messages (#7115)

Before, locally deleted messages (because of DeleteDeviceAfter set or external deletion requests)
couldn't be received if they're re-sent because tombstones prevented that forever. This led to
locally deleted webxdcs being unrecoverable.

Author: iequidoo

src/imap.rs

@@ -2352,11 +2352,11 @@ pub(crate) async fn prefetch_should_download(
     )
     .await?
     {
-        if is_trash {
-            message::prune_tombstone(context, message_id).await?;
+        let should = is_trash && !message::prune_tombstone(context, message_id).await?;
+        if !should {
+            markseen_on_imap_table(context, message_id).await?;
         }
-        markseen_on_imap_table(context, message_id).await?;
-        return Ok(false);
+        return Ok(should);
     }
 
     // We do not know the Message-ID or the Message-ID is missing (in this case, we create one in
@@ -2429,8 +2429,11 @@ pub(crate) async fn prefetch_should_download(
 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.
+    // because they are stored to the db before sending. Trashed messages also have zero
+    // timestamp_sent and mustn't make new messages "duplicates", otherwise if a webxdc message is
+    // deleted because of DeleteDeviceAfter set, it won't be recovered from a re-sent message. 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
 }
 

src/message.rs

@@ -1746,8 +1746,9 @@ pub async fn delete_msgs_ex(
 }
 
 /// Removes from the database a locally deleted message that also doesn't have a server UID.
-pub(crate) async fn prune_tombstone(context: &Context, rfc724_mid: &str) -> Result<()> {
-    context
+/// Returns whether the removal happened.
+pub(crate) async fn prune_tombstone(context: &Context, rfc724_mid: &str) -> Result<bool> {
+    Ok(context
         .sql
         .execute(
             "DELETE FROM msgs
@@ -1758,8 +1759,8 @@ pub(crate) async fn prune_tombstone(context: &Context, rfc724_mid: &str) -> Resu
             )",
             (rfc724_mid, DC_CHAT_ID_TRASH),
         )
-        .await?;
-    Ok(())
+        .await?
+        > 0)
 }
 
 /// Marks requested messages as seen.

src/receive_imf.rs

@@ -558,43 +558,53 @@ pub(crate) async fn receive_imf_inner(
     let (replace_msg_id, replace_chat_id);
     if let Some((old_msg_id, _)) = message::rfc724_mid_exists(context, rfc724_mid).await? {
         let msg = Message::load_from_db_optional(context, old_msg_id).await?;
-        if msg.is_none() {
-            message::prune_tombstone(context, rfc724_mid).await?;
+        // The tombstone being pruned means that we expected the message to appear on IMAP after
+        // deletion. NB: Not all such messages have `msgs.deleted=1`, see how external deletion
+        // requests deal with message reordering.
+        match msg.is_none() && !message::prune_tombstone(context, rfc724_mid).await? {
+            true => replace_msg_id = None,
+            false => replace_msg_id = Some(old_msg_id),
         }
-        replace_msg_id = Some(old_msg_id);
         if let Some(msg) = msg.filter(|msg| msg.download_state() != DownloadState::Done) {
             // the message was partially downloaded before and is fully downloaded now.
             info!(context, "Message already partly in DB, replacing.");
             replace_chat_id = Some(msg.chat_id);
         } else {
             replace_chat_id = None;
         }
-    } else {
-        replace_msg_id = if rfc724_mid_orig == rfc724_mid {
-            None
-        } else if let Some((old_msg_id, old_ts_sent)) =
-            message::rfc724_mid_exists(context, rfc724_mid_orig).await?
-        {
-            message::prune_tombstone(context, rfc724_mid_orig).await?;
-            if imap::is_dup_msg(
-                mime_parser.has_chat_version(),
-                mime_parser.timestamp_sent,
-                old_ts_sent,
-            ) {
-                info!(context, "Deleting duplicate message {rfc724_mid_orig}.");
-                let target = context.get_delete_msgs_target().await?;
-                context
-                    .sql
-                    .execute(
-                        "UPDATE imap SET target=? WHERE folder=? AND uidvalidity=? AND uid=?",
-                        (target, folder, uidvalidity, uid),
-                    )
-                    .await?;
-            }
-            Some(old_msg_id)
+    } else if rfc724_mid_orig == rfc724_mid {
+        replace_msg_id = None;
+        replace_chat_id = None;
+    } else if let Some((old_msg_id, old_ts_sent, is_trash)) = message::rfc724_mid_exists_ex(
+        context,
+        rfc724_mid_orig,
+        "chat_id=3", // Trash
+    )
+    .await?
+    {
+        if is_trash && !message::prune_tombstone(context, rfc724_mid_orig).await? {
+            replace_msg_id = None;
+        } else if imap::is_dup_msg(
+            mime_parser.has_chat_version(),
+            mime_parser.timestamp_sent,
+            old_ts_sent,
+        ) {
+            info!(context, "Deleting duplicate message {rfc724_mid_orig}.");
+            let target = context.get_delete_msgs_target().await?;
+            context
+                .sql
+                .execute(
+                    "UPDATE imap SET target=? WHERE folder=? AND uidvalidity=? AND uid=?",
+                    (target, folder, uidvalidity, uid),
+                )
+                .await?;
+            replace_msg_id = Some(old_msg_id);
         } else {
-            None
-        };
+            replace_msg_id = Some(old_msg_id);
+        }
+        replace_chat_id = None;
+    } else {
+        replace_msg_id = None;
         replace_chat_id = None;
     }
 

src/webxdc/webxdc_tests.rs

@@ -1853,6 +1853,14 @@ async fn test_status_update_vs_delete_device_after() -> Result<()> {
         .await?;
     let alice_chat = alice.create_chat(bob).await;
     let alice_instance = send_webxdc_instance(alice, alice_chat.id).await?;
+    // Needed to test receiving of re-sent locally deleted webxdc.
+    bob.sql
+        .execute(
+            "INSERT INTO imap (rfc724_mid, folder, uid, uidvalidity, target)
+            VALUES            (?1,         ?2,     ?3,  ?4,          ?5)",
+            (&alice_instance.rfc724_mid, "INBOX", 1, 1, "INBOX"),
+        )
+        .await?;
     let bob_instance = bob.recv_msg(&alice.pop_sent_msg().await).await;
     assert_eq!(bob.add_or_lookup_contact(alice).await.is_bot(), false);
 
@@ -1882,8 +1890,15 @@ async fn test_status_update_vs_delete_device_after() -> Result<()> {
     SystemTime::shift(Duration::from_secs(2700));
     ephemeral::delete_expired_messages(bob, tools::time()).await?;
     let bob_instance = Message::load_from_db(bob, bob_instance.id).await?;
-    assert_eq!(bob_instance.chat_id.is_trash(), false);
 
+    SystemTime::shift(Duration::from_secs(1800));
+    ephemeral::delete_expired_messages(bob, tools::time()).await?;
+    let bob_instance = Message::load_from_db_optional(bob, bob_instance.id).await?;
+    assert!(bob_instance.is_none());
+
+    // Additionally test that a re-sent instance can be received after deletion.
+    resend_msgs(alice, &[alice_instance.id]).await?;
+    bob.recv_msg(&alice.pop_sent_msg().await).await;
     Ok(())
 }