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.

src/imap.rs

@@ -634,6 +634,9 @@ impl Imap {
             let target = if let Some(message_id) = &message_id {
                 let msg_info =
                     message::rfc724_mid_exists_ex(context, message_id, "deleted=1").await?;
+                if msg_info.is_some_and(|(_, ts_sent, _)| ts_sent == 0) {
+                    message::prune_tombstone(context, message_id).await?;
+                }
                 let delete = if let Some((_, _, true)) = msg_info {
                     info!(context, "Deleting locally deleted message {message_id}.");
                     true
@@ -2342,12 +2345,18 @@ pub(crate) async fn prefetch_should_download(
     message_id: &str,
     mut flags: impl Iterator<Item = Flag<'_>>,
 ) -> Result<bool> {
-    if message::rfc724_mid_exists(context, message_id)
-        .await?
-        .is_some()
+    if let Some((_, _, is_trash)) = message::rfc724_mid_exists_ex(
+        context,
+        message_id,
+        "chat_id=3", // Trash
+    )
+    .await?
     {
-        markseen_on_imap_table(context, message_id).await?;
-        return Ok(false);
+        let should = is_trash && !message::prune_tombstone(context, message_id).await?;
+        if !should {
+            markseen_on_imap_table(context, message_id).await?;
+        }
+        return Ok(should);
     }
 
     // We do not know the Message-ID or the Message-ID is missing (in this case, we create one in
@@ -2420,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

@@ -112,18 +112,20 @@ impl MsgId {
             .unwrap_or_default())
     }
 
-    /// Put message into trash chat and delete message text.
+    /// Put message into trash chat or delete it.
     ///
     /// It means the message is deleted locally, but not on the server.
-    /// We keep some infos to
-    /// 1. not download the same message again
-    /// 2. be able to delete the message on the server if we want to
     ///
-    /// * `on_server`: Delete the message on the server also if it is seen on IMAP later, but only
-    ///   if all parts of the message are trashed with this flag. `true` if the user explicitly
-    ///   deletes the message. As for trashing a partially downloaded message when replacing it with
-    ///   a fully downloaded one, see `receive_imf::add_parts()`.
+    /// * `on_server`: Keep some info to delete the message on the server also if it is seen on IMAP
+    ///   later.
     pub(crate) async fn trash(self, context: &Context, on_server: bool) -> Result<()> {
+        if !on_server {
+            context
+                .sql
+                .execute("DELETE FROM msgs WHERE id=?1", (self,))
+                .await?;
+            return Ok(());
+        }
         context
             .sql
             .execute(
@@ -132,7 +134,7 @@ impl MsgId {
                 // still adds to the db if chat_id is TRASH.
                 "INSERT OR REPLACE INTO msgs (id, rfc724_mid, timestamp, chat_id, deleted)
                  SELECT ?1, rfc724_mid, timestamp, ?, ? FROM msgs WHERE id=?1",
-                (self, DC_CHAT_ID_TRASH, on_server),
+                (self, DC_CHAT_ID_TRASH, true),
             )
             .await?;
 
@@ -1590,11 +1592,15 @@ pub(crate) async fn get_mime_headers(context: &Context, msg_id: MsgId) -> Result
 
 /// Delete a single message from the database, including references in other tables.
 /// This may be called in batches; the final events are emitted in delete_msgs_locally_done() then.
-pub(crate) async fn delete_msg_locally(context: &Context, msg: &Message) -> Result<()> {
+pub(crate) async fn delete_msg_locally(
+    context: &Context,
+    msg: &Message,
+    keep_tombstone: bool,
+) -> Result<()> {
     if msg.location_id > 0 {
         delete_poi_location(context, msg.location_id).await?;
     }
-    let on_server = true;
+    let on_server = keep_tombstone;
     msg.id
         .trash(context, on_server)
         .await
@@ -1662,6 +1668,7 @@ pub async fn delete_msgs_ex(
     let mut modified_chat_ids = HashSet::new();
     let mut deleted_rfc724_mid = Vec::new();
     let mut res = Ok(());
+    let mut msgs = Vec::with_capacity(msg_ids.len());
 
     for &msg_id in msg_ids {
         let msg = Message::load_from_db(context, msg_id).await?;
@@ -1679,18 +1686,24 @@ pub async fn delete_msgs_ex(
 
         let target = context.get_delete_msgs_target().await?;
         let update_db = |trans: &mut rusqlite::Transaction| {
-            trans.execute(
+            // NB: If the message isn't sent yet, keeping its tombstone is unnecessary, but safe.
+            let keep_tombstone = trans.execute(
                 "UPDATE imap SET target=? WHERE rfc724_mid=?",
-                (target, msg.rfc724_mid),
-            )?;
+                (&target, msg.rfc724_mid),
+            )? == 0
+                || !target.is_empty();
             trans.execute("DELETE FROM smtp WHERE msg_id=?", (msg_id,))?;
-            Ok(())
+            Ok(keep_tombstone)
         };
-        if let Err(e) = context.sql.transaction(update_db).await {
-            error!(context, "delete_msgs: failed to update db: {e:#}.");
-            res = Err(e);
-            continue;
-        }
+        let keep_tombstone = match context.sql.transaction(update_db).await {
+            Ok(v) => v,
+            Err(e) => {
+                error!(context, "delete_msgs: failed to update db: {e:#}.");
+                res = Err(e);
+                continue;
+            }
+        };
+        msgs.push((msg_id, keep_tombstone));
     }
     res?;
 
@@ -1719,9 +1732,9 @@ pub async fn delete_msgs_ex(
             .await?;
     }
 
-    for &msg_id in msg_ids {
+    for (msg_id, keep_tombstone) in msgs {
         let msg = Message::load_from_db(context, msg_id).await?;
-        delete_msg_locally(context, &msg).await?;
+        delete_msg_locally(context, &msg, keep_tombstone).await?;
     }
     delete_msgs_locally_done(context, msg_ids, modified_chat_ids).await?;
 
@@ -1731,6 +1744,24 @@ pub async fn delete_msgs_ex(
     Ok(())
 }
 
+/// Removes from the database a locally deleted message that also doesn't have a server UID.
+/// 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
+            WHERE rfc724_mid=?
+            AND chat_id=?
+            AND NOT EXISTS (
+                SELECT * FROM imap WHERE msgs.rfc724_mid=rfc724_mid AND target!=''
+            )",
+            (rfc724_mid, DC_CHAT_ID_TRASH),
+        )
+        .await?
+        > 0)
+}
+
 /// Marks requested messages as seen.
 pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()> {
     if msg_ids.is_empty() {

src/receive_imf.rs

@@ -44,7 +44,7 @@ use crate::securejoin::{self, handle_securejoin_handshake, observe_securejoin_on
 use crate::simplify;
 use crate::stock_str;
 use crate::sync::Sync::*;
-use crate::tools::{self, buf_compress, remove_subject_prefix};
+use crate::tools::{self, buf_compress, remove_subject_prefix, time};
 use crate::{chatlist_events, ensure_and_debug_assert, ensure_and_debug_assert_eq, location};
 use crate::{contact, imap};
 
@@ -557,44 +557,56 @@ pub(crate) async fn receive_imf_inner(
     // make sure, this check is done eg. before securejoin-processing.
     let (replace_msg_id, replace_chat_id);
     if let Some((old_msg_id, _)) = message::rfc724_mid_exists(context, rfc724_mid).await? {
-        replace_msg_id = Some(old_msg_id);
-        replace_chat_id = if let Some(msg) = Message::load_from_db_optional(context, old_msg_id)
-            .await?
-            .filter(|msg| msg.download_state() != DownloadState::Done)
-        {
+        let msg = Message::load_from_db_optional(context, old_msg_id).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),
+        }
+        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.");
-            Some(msg.chat_id)
+            replace_chat_id = Some(msg.chat_id);
         } else {
             // The message was already fully downloaded
             // or cannot be loaded because it is deleted.
-            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?
-        {
-            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)
+            replace_chat_id = None;
+        }
+    } 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;
     }
 
@@ -2200,10 +2212,7 @@ RETURNING id
     }
 
     if let Some(replace_msg_id) = replace_msg_id {
-        // Trash the "replace" placeholder with a message that has no parts. If it has the original
-        // "Message-ID", mark the placeholder for server-side deletion so as if the user deletes the
-        // fully downloaded message later, the server-side deletion is issued.
-        let on_server = rfc724_mid == rfc724_mid_orig;
+        let on_server = false;
         replace_msg_id.trash(context, on_server).await?;
     }
 
@@ -2311,7 +2320,8 @@ async fn handle_edit_delete(
                     {
                         if let Some(msg) = Message::load_from_db_optional(context, msg_id).await? {
                             if msg.from_id == from_id {
-                                message::delete_msg_locally(context, &msg).await?;
+                                let keep_tombstone = false;
+                                message::delete_msg_locally(context, &msg, keep_tombstone).await?;
                                 msg_ids.push(msg.id);
                                 modified_chat_ids.insert(msg.chat_id);
                             } else {
@@ -2322,6 +2332,14 @@ async fn handle_edit_delete(
                         }
                     } else {
                         warn!(context, "Delete message: {rfc724_mid:?} not found.");
+                        context
+                            .sql
+                            .execute(
+                                "INSERT INTO msgs (rfc724_mid, timestamp, chat_id)
+                                VALUES (?, ?, ?)",
+                                (rfc724_mid, time(), DC_CHAT_ID_TRASH),
+                            )
+                            .await?;
                     }
                 }
                 message::delete_msgs_locally_done(context, &msg_ids, modified_chat_ids).await?;

src/receive_imf/receive_imf_tests.rs

@@ -815,6 +815,31 @@ async fn test_concat_multiple_ndns() -> Result<()> {
     Ok(())
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_receive_resent_deleted_msg() -> Result<()> {
+    let alice = &TestContext::new_alice().await;
+    let bob = &TestContext::new_bob().await;
+    let alice_bob_id = alice.add_or_lookup_contact_id(bob).await;
+    let alice_chat_id = ChatId::create_for_contact(alice, alice_bob_id).await?;
+    let sent = alice.send_text(alice_chat_id, "hi").await;
+    let alice_msg = Message::load_from_db(alice, sent.sender_msg_id).await?;
+    bob.sql
+        .execute(
+            "INSERT INTO imap (rfc724_mid, folder, uid, uidvalidity, target)
+            VALUES            (?1,         ?2,     ?3,  ?4,          ?5)",
+            (&alice_msg.rfc724_mid, "INBOX", 1, 1, "INBOX"),
+        )
+        .await?;
+    let bob_msg = bob.recv_msg(&sent).await;
+    let bob_chat_id = bob_msg.chat_id;
+    message::delete_msgs(bob, &[bob_msg.id]).await?;
+    chat::resend_msgs(alice, &[alice_msg.id]).await?;
+    let bob_msg = bob.recv_msg(&alice.pop_sent_msg().await).await;
+    assert_eq!(bob_msg.chat_id, bob_chat_id);
+    assert_eq!(bob_msg.text, "hi");
+    Ok(())
+}
+
 async fn load_imf_email(context: &Context, imf_raw: &[u8]) -> Message {
     context
         .set_config(Config::ShowEmails, Some("2"))

src/sync.rs

@@ -6,7 +6,7 @@ use serde::{Deserialize, Serialize};
 
 use crate::chat::{self, ChatId};
 use crate::config::Config;
-use crate::constants::Blocked;
+use crate::constants::{self, Blocked};
 use crate::contact::ContactId;
 use crate::context::Context;
 use crate::log::LogExt;
@@ -317,14 +317,21 @@ impl Context {
         for rfc724_mid in msgs {
             if let Some((msg_id, _)) = message::rfc724_mid_exists(self, rfc724_mid).await? {
                 if let Some(msg) = Message::load_from_db_optional(self, msg_id).await? {
-                    message::delete_msg_locally(self, &msg).await?;
+                    let keep_tombstone = false;
+                    message::delete_msg_locally(self, &msg, keep_tombstone).await?;
                     msg_ids.push(msg.id);
                     modified_chat_ids.insert(msg.chat_id);
                 } else {
                     warn!(self, "Sync message delete: Database entry does not exist.");
                 }
             } else {
-                warn!(self, "Sync message delete: {rfc724_mid:?} not found.");
+                info!(self, "sync_message_deletion: {rfc724_mid:?} not found.");
+                self.sql
+                    .execute(
+                        "INSERT INTO msgs (rfc724_mid, timestamp, chat_id) VALUES (?, ?, ?)",
+                        (rfc724_mid, time(), constants::DC_CHAT_ID_TRASH),
+                    )
+                    .await?;
             }
         }
         message::delete_msgs_locally_done(self, &msg_ids, modified_chat_ids).await?;

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(())
 }