fix: Prune tombstone when a message isn't expected to appear on IMAP anymore (#7115)

This allows to receive deleted messages again if they're re-sent:
- After a manual deletion,
- If both DeleteServerAfter and DeleteDeviceAfter are set,

w/o waiting for 2 days when stale tombstones are GC-ed. This is particularly useful for webxdc. If
only DeleteDeviceAfter is set though, this changes nothing and maybe a separate fix is needed.

This may also greately reduce the db size for some bots which receive and immediately delete many
messages.

Also insert a tombstone to the db if a deletion request (incl. sync messages) can't find
`rfc724_mid`. This may happen in case of message reordering and the deleted message mustn't appear
when it finally arrives.

Author: iequidoo

src/imap.rs

@@ -595,6 +595,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() {
+                    message::prune_tombstone(context, message_id).await?;
+                }
                 let delete = if let Some((_, _, true)) = msg_info {
                     info!(context, "Deleting locally deleted message {message_id}.");
                     true
@@ -2252,6 +2255,7 @@ pub(crate) async fn prefetch_should_download(
         .await?
         .is_some()
     {
+        message::prune_tombstone(context, message_id).await?;
         markseen_on_imap_table(context, message_id).await?;
         return Ok(false);
     }

src/message.rs

@@ -114,18 +114,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(
@@ -134,7 +136,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?;
 
@@ -1668,11 +1670,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
@@ -1740,6 +1746,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?;
@@ -1757,18 +1764,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?;
 
@@ -1797,9 +1810,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?;
 
@@ -1809,6 +1822,23 @@ pub async fn delete_msgs_ex(
     Ok(())
 }
 
+/// 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
+        .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?;
+    Ok(())
+}
+
 /// 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};
 
@@ -559,24 +559,28 @@ pub(crate) async fn receive_imf_inner(
             );
             return Ok(None);
         }
-        let msg = Message::load_from_db(context, old_msg_id).await?;
+        let msg = Message::load_from_db_optional(context, old_msg_id).await?;
+        if msg.is_none() {
+            message::prune_tombstone(context, rfc724_mid).await?;
+        }
         replace_msg_id = Some(old_msg_id);
-        replace_chat_id = if msg.download_state() != DownloadState::Done {
+        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 by full message."
             );
-            Some(msg.chat_id)
+            replace_chat_id = Some(msg.chat_id);
         } else {
-            None
-        };
+            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,
@@ -2194,10 +2198,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?;
     }
 
@@ -2305,7 +2306,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 {
@@ -2316,6 +2318,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;
@@ -310,14 +310,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?;