diff --git a/src/imap.rs b/src/imap.rs index ea6d907362..2a4273df9e 100644 --- a/src/imap.rs +++ b/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_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 @@ -2248,12 +2251,18 @@ pub(crate) async fn prefetch_should_download( message_id: &str, mut flags: impl Iterator>, ) -> Result { - 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 @@ -2326,8 +2335,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 } diff --git a/src/message.rs b/src/message.rs index 04fbad610a..6cdf0edb63 100644 --- a/src/message.rs +++ b/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,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 { + 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) -> Result<()> { if msg_ids.is_empty() { diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 03375862f4..bdebf67c4c 100644 --- a/src/receive_imf.rs +++ b/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,43 +559,57 @@ pub(crate) async fn receive_imf_inner( ); return Ok(None); } - let msg = Message::load_from_db(context, old_msg_id).await?; - replace_msg_id = Some(old_msg_id); - replace_chat_id = if 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 by full message." ); - Some(msg.chat_id) + replace_chat_id = Some(msg.chat_id); } else { - 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; } @@ -2194,10 +2208,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 +2316,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 +2328,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?; diff --git a/src/receive_imf/receive_imf_tests.rs b/src/receive_imf/receive_imf_tests.rs index 7f777742a8..da9cd85196 100644 --- a/src/receive_imf/receive_imf_tests.rs +++ b/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")) diff --git a/src/sync.rs b/src/sync.rs index 90e302f06b..ae4e3f5e78 100644 --- a/src/sync.rs +++ b/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?; diff --git a/src/webxdc/webxdc_tests.rs b/src/webxdc/webxdc_tests.rs index 35039459ed..63fdd90e56 100644 --- a/src/webxdc/webxdc_tests.rs +++ b/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(()) }