Update comments

src/ephemeral.rs

@@ -460,7 +460,7 @@ WHERE
 /// Emits relevant `MsgsChanged` and `WebxdcInstanceDeleted` events
 /// if messages are deleted.
 ///
-/// Also see [`delete_expired_imap_messages`],
+/// Also see [`delete_tombstoned_messages_from_imap`],
 /// which marks the messages for deletion on the IMAP server.
 pub(crate) async fn delete_expired_messages(context: &Context, now: i64) -> Result<()> {
     let rows = select_expired_messages(context, now).await?;
@@ -477,8 +477,8 @@ pub(crate) async fn delete_expired_messages(context: &Context, now: i64) -> Resu
                 // and other places it references.
                 let mut del_msg_stmt = transaction.prepare(
                     "
-INSERT OR REPLACE INTO msgs (id, rfc724_mid, pre_rfc724_mid, timestamp, chat_id)
-SELECT ?1, rfc724_mid, pre_rfc724_mid, timestamp, ? FROM msgs WHERE id=?1
+INSERT OR REPLACE INTO msgs (id, rfc724_mid, pre_rfc724_mid, timestamp, chat_id, deleted)
+SELECT ?1, rfc724_mid, pre_rfc724_mid, timestamp, ?, 1 FROM msgs WHERE id=?1
                     ",
                 )?;
                 let mut del_location_stmt =
@@ -652,17 +652,20 @@ pub(crate) async fn ephemeral_loop(context: &Context, interrupt_receiver: Receiv
     }
 }
 
-/// Schedules expired IMAP messages for deletion on the server.
+/// Schedules messages for deletion on the server:
+///
+/// - messages in the trash chat with the `deleted=1` flag
+/// - In single-device mode, this additionally marks all downloaded messages for deletion
+///   (only encrypted ones for non-chatmail),
+///   because there is no other device that may need these messages.
 ///
 /// Also see [`delete_expired_messages`],
-/// which locally deletes expired messages.
-pub(crate) async fn delete_expired_imap_messages(
+/// which locally deletes expired messages, and gives them the `deleted=1` flag.
+pub(crate) async fn delete_tombstoned_messages_from_imap(
     context: &Context,
     transport_id: u32,
     is_chatmail: bool,
 ) -> Result<()> {
-    let now = time();
-
     let bcc_self = context.get_config_bool(Config::BccSelf).await?;
     if should_delete_all_downloaded_messages(bcc_self, is_chatmail) {
         // This is the only device using this relay.
@@ -681,20 +684,21 @@ pub(crate) async fn delete_expired_imap_messages(
                 WHERE transport_id=?1
                 AND rfc724_mid IN (
                     SELECT rfc724_mid FROM msgs
-                    WHERE ((ephemeral_timestamp!=0 AND ephemeral_timestamp<=?2) OR download_state=?3)
-                        AND id>9
+                    WHERE deleted=1 OR download_state=?2
                     UNION
                     SELECT pre_rfc724_mid FROM msgs
                     WHERE pre_rfc724_mid!=''
-                        AND id>9
                 )",
-                (transport_id, now, DownloadState::Done),
+                (transport_id, DownloadState::Done),
             )
             .await?;
     } else if bcc_self {
         // There may be other devices using this relay,
         // either because there is multi-device or because this is a classical email server.
-        // Only delete expired ephemeral messages.
+
+        // This uses `AND chat_id={DC_CHAT_ID_TRASH}`, so that the index on `chat_id` can be used.
+        // Only messages in the trash chat are ever marked as deleted, which makes this optimization possible.
+        // This speeds up this SQL query by a factor of ~3.
         context
             .sql
             .execute(
@@ -703,18 +707,18 @@ pub(crate) async fn delete_expired_imap_messages(
                 WHERE transport_id=?1
                 AND rfc724_mid IN (
                     SELECT rfc724_mid FROM msgs
-                    WHERE ephemeral_timestamp!=0 AND ephemeral_timestamp<=?2 AND id>9
+                    WHERE deleted=1 AND chat_id=?2
                     UNION
                     SELECT pre_rfc724_mid FROM msgs
-                    WHERE pre_rfc724_mid!=''
-                        AND ephemeral_timestamp!=0 AND ephemeral_timestamp<=?2 AND id>9
+                    WHERE pre_rfc724_mid!='' AND deleted=1 AND chat_id=?2
                 )",
-                (transport_id, now),
+                (transport_id, DC_CHAT_ID_TRASH),
             )
             .await?;
     } else {
         // Single device.
-        // Delete all expired and encrypted messages.
+        // Delete all messages that were marked for deletion,
+        // and all encrypted messages.
         context
             .sql
             .execute(
@@ -723,17 +727,14 @@ pub(crate) async fn delete_expired_imap_messages(
                  WHERE transport_id=?1
                  AND rfc724_mid IN (
                     SELECT rfc724_mid FROM msgs
-                    WHERE id>9
-                      AND ((ephemeral_timestamp!=0 AND ephemeral_timestamp<=?2) OR
-                           ((param GLOB '*\nc=1*' OR param GLOB 'c=1*') AND download_state=?3))
+                    WHERE ((param GLOB '*\nc=1*' OR param GLOB 'c=1*') AND download_state=?2) OR
+                           deleted=1
                     UNION
                     SELECT pre_rfc724_mid FROM msgs
                     WHERE pre_rfc724_mid!=''
-                      AND id>9
-                      AND ((ephemeral_timestamp!=0 AND ephemeral_timestamp<=?2) OR
-                           (param GLOB '*\nc=1*' OR param GLOB 'c=1*'))
+                      AND (param GLOB '*\nc=1*' OR param GLOB 'c=1*' OR deleted=1)
                 )",
-                (transport_id, now, DownloadState::Done),
+                (transport_id, DownloadState::Done),
             )
             .await?;
     }

src/ephemeral/ephemeral_tests.rs

@@ -441,7 +441,7 @@ async fn check_msg_is_deleted(t: &TestContext, chat: &Chat, msg_id: MsgId) {
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-async fn test_delete_expired_imap_messages() -> Result<()> {
+async fn test_delete_tombstoned_messages_from_imap() -> Result<()> {
     let t = TestContext::new_alice().await;
     let now = time();
     let transport_id: u32 = 1;
@@ -534,8 +534,8 @@ async fn test_delete_expired_imap_messages() -> Result<()> {
         t.sql
             .execute(
                 "INSERT INTO msgs \
-                 (rfc724_mid, timestamp, ephemeral_timestamp, download_state, pre_rfc724_mid, param) \
-                 VALUES (?,?,?,?,?,?)",
+                 (rfc724_mid, timestamp, ephemeral_timestamp, download_state, pre_rfc724_mid, param, chat_id) \
+                 VALUES (?,?,?,?,?,?,?)",
                 (
                     rfc724_mid,
                     now,
@@ -546,7 +546,8 @@ async fn test_delete_expired_imap_messages() -> Result<()> {
                         "c=1"
                     } else {
                         ""
-                    }
+                    },
+                    10 // Just some non-special chat id
                 ),
             )
             .await?;
@@ -586,7 +587,11 @@ async fn test_delete_expired_imap_messages() -> Result<()> {
 
         t.set_config_bool(Config::BccSelf, bcc_self).await?;
 
-        delete_expired_imap_messages(
+        // `delete_expired_messages()` is called before `delete_tombstoned_messages_from_imap()`,
+        // because this matches the behavior in production
+        // where the ephemeral loop calls `delete_expired_messages()` as soon as a message timer expired.
+        delete_expired_messages(&t, time()).await?;
+        delete_tombstoned_messages_from_imap(
             &t,
             if other_transport {
                 transport_id + 1
@@ -626,7 +631,7 @@ async fn test_delete_expired_imap_messages() -> Result<()> {
 
     // With BccSelf=true, non-expired messages are kept even if `is_chatmail` is true
     t.set_config_bool(Config::BccSelf, true).await?;
-    delete_expired_imap_messages(&t, transport_id, true).await?;
+    delete_tombstoned_messages_from_imap(&t, transport_id, true).await?;
     assert_eq!(is_deleted(&t, "expired@localhost").await?, true);
     assert_eq!(is_deleted(&t, "no_expire@localhost").await?, false);
     assert_eq!(is_deleted(&t, "done_pre@localhost").await?, false);

src/imap.rs

@@ -45,7 +45,7 @@ use crate::transport::{
 };
 use crate::{
     calls::{UnresolvedIceServer, create_fallback_ice_servers, create_ice_servers_from_metadata},
-    ephemeral::delete_expired_imap_messages,
+    ephemeral::delete_tombstoned_messages_from_imap,
 };
 
 pub(crate) mod capabilities;
@@ -486,11 +486,15 @@ impl Imap {
             context.scheduler.interrupt_ephemeral_task().await;
         }
 
-        // Mark expired messages for deletion. Note that `delete_expired_imap_messages` is
+        // Mark expired messages for deletion. Note that `delete_tombstoned_messages_from_imap` is
         // not well optimized and should not be called before fetching.
-        delete_expired_imap_messages(context, session.transport_id(), session.is_chatmail())
-            .await
-            .context("delete_expired_imap_messages")?;
+        delete_tombstoned_messages_from_imap(
+            context,
+            session.transport_id(),
+            session.is_chatmail(),
+        )
+        .await
+        .context("delete_tombstoned_messages_from_imap")?;
 
         session
             .move_delete_messages(context, watch_folder)
@@ -593,34 +597,10 @@ impl Imap {
                 .size
                 .context("imap fetch response does not contain size")?;
 
-            // Determine the target folder where the message should be moved to.
-            //
-            // We only move the messages from the INBOX and Spam folders.
-            // 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 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 delete = if let Some(message_id) = &message_id {
-                message::rfc724_mid_exists_ex(context, message_id, "deleted=1")
-                    .await?
-                    .is_some_and(|(_msg_id, deleted)| deleted)
-            } else {
-                false
-            };
-
             // Generate a fake Message-ID to identify the message in the database
             // if the message has no real Message-ID.
             let message_id = message_id.unwrap_or_else(create_message_id);
 
-            if delete {
-                info!(context, "Deleting locally deleted message {message_id}.");
-            }
-
-            let target = if delete { "" } else { folder };
-
             context
                 .sql
                 .execute(
@@ -635,21 +615,14 @@ impl Imap {
                         &folder,
                         uid,
                         uid_validity,
-                        target,
+                        &folder,
                     ),
                 )
                 .await?;
 
-            // Download only the messages which have reached their target folder if there are
-            // multiple devices. This prevents race conditions in multidevice case, where one
-            // device tries to download the message while another device moves the message at the
-            // same time. Even in single device case it is possible to fail downloading the first
-            // message, move it to the movebox and then download the second message before
-            // downloading the first one, if downloading from inbox before moving is allowed.
-            if folder == target
-                && prefetch_should_download(context, &headers, &message_id, fetch_response.flags())
-                    .await
-                    .context("prefetch_should_download")?
+            if prefetch_should_download(context, &headers, &message_id, fetch_response.flags())
+                .await
+                .context("prefetch_should_download")?
             {
                 if headers
                     .get_header_value(HeaderDef::ChatIsPostMessage)