Revert "--wip-- [skip ci]"

This reverts commit 1355112e5a4d657404f8499e3cfbc61f45371048.

src/ephemeral.rs

@@ -73,11 +73,9 @@ use serde::{Deserialize, Serialize};
 use tokio::time::timeout;
 
 use crate::chat::{ChatId, ChatIdBlocked, send_msg};
-use crate::config::Config;
 use crate::constants::{DC_CHAT_ID_LAST_SPECIAL, DC_CHAT_ID_TRASH};
 use crate::contact::ContactId;
 use crate::context::Context;
-use crate::download::DownloadState;
 use crate::events::EventType;
 use crate::log::{LogExt, warn};
 use crate::message::{Message, MessageState, MsgId, Viewtype};
@@ -650,115 +648,20 @@ pub(crate) async fn ephemeral_loop(context: &Context, interrupt_receiver: Receiv
 }
 
 /// Schedules expired IMAP messages for deletion.
-pub(crate) async fn delete_expired_imap_messages(
-    context: &Context,
-    transport_id: u32,
-    is_chatmail: bool,
-) -> Result<()> {
+pub(crate) async fn delete_expired_imap_messages(context: &Context) -> Result<()> {
     let now = time();
-
-    if !context.get_config_bool(Config::BccSelf).await? && is_chatmail {
-        info!(
-            context,
-            "dbg marking all as deleted 1 - rfc724_mids: {:?}",
-            context
-                .sql
-                .query_map_vec(
-                    "SELECT rfc724_mid FROM msgs
-                    WHERE (ephemeral_timestamp!=0 AND ephemeral_timestamp<=?)
-                    OR download_state=?",
-                    (now, DownloadState::Done),
-                    |row| Ok(row.get::<_, String>(0)?)
-                )
-                .await
-        );
-        info!(
-            context,
-            "dbg marking all as deleted 1 - pre_rfc724_mids: {:?}",
-            context
-                .sql
-                .query_map_vec(
-                    "SELECT pre_rfc724_mid FROM msgs
-                    WHERE pre_rfc724_mid!=''",
-                    (),
-                    |row| Ok(row.get::<_, String>(0)?)
-                )
-                .await
-        );
-        // This the only device using this relay.
-        // Mark all downloaded messages for deletion, because they are not needed anymore.
-        //
-        // For pre- and post-messages, `rfc724_mid` contains the post-message's Message-Id.
-        // The pre-message's Message-Id is in pre_rfc724_mid, if it exists.
-        //
-        // Pre-messages can be deleted even if the message wasn't fully downloaded yet,
-        // because it's only the post-message that hasn't been downloaded.
-        context
-            .sql
-            .execute(
-                "UPDATE imap
-                SET target=''
-                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
-                    UNION
-                    SELECT pre_rfc724_mid FROM msgs
-                    WHERE pre_rfc724_mid!=''
-                )",
-                (transport_id, now, DownloadState::Done),
-            )
-            .await?;
-    } else {
-        info!(
-            context,
-            "dbg marking ephemeral as deleted 1 - rfc724_mids: {:?}",
-            context
-                .sql
-                .query_map_vec(
-                    "SELECT rfc724_mid FROM msgs
-                    WHERE (ephemeral_timestamp!=0 AND ephemeral_timestamp<=?2)",
-                    (transport_id, now),
-                    |row| Ok(row.get::<_, String>(0)?)
-                )
-                .await
-        );
-        info!(
-            context,
-            "dbg marking ephemeral as deleted 1 - pre_rfc724_mids: {:?}",
-            context
-                .sql
-                .query_map_vec(
-                    "SELECT pre_rfc724_mid FROM msgs
-                    WHERE pre_rfc724_mid!=''
-                    AND (ephemeral_timestamp!=0 AND ephemeral_timestamp<=?)",
-                    (now,),
-                    |row| Ok(row.get::<_, String>(0)?)
-                )
-                .await
-        );
-        // There may be other devices using this relay,
-        // either because there is multi-relay or because this is a classical email server.
-        // Only delete expired ephemeral messages.
-        context
-            .sql
-            .execute(
-                "UPDATE imap
-                SET target=''
-                WHERE transport_id=?1
-                AND rfc724_mid IN (
-                    SELECT rfc724_mid FROM msgs
-                    WHERE (ephemeral_timestamp!=0 AND ephemeral_timestamp<=?2)
-                    UNION
-                    SELECT pre_rfc724_mid FROM msgs
-                    WHERE pre_rfc724_mid!=''
-                    AND (ephemeral_timestamp!=0 AND ephemeral_timestamp<=?2)
-                )",
-                (transport_id, now),
-            )
-            .await?;
-    }
+    context
+        .sql
+        .execute(
+            "UPDATE imap
+             SET target=''
+             WHERE rfc724_mid IN (
+               SELECT rfc724_mid FROM msgs
+               WHERE ephemeral_timestamp != 0 AND ephemeral_timestamp <= ?
+             )",
+            (now,),
+        )
+        .await?;
 
     Ok(())
 }

src/ephemeral/ephemeral_tests.rs

@@ -451,178 +451,77 @@ async fn test_delete_expired_imap_messages() -> Result<()> {
     let t = TestContext::new_alice().await;
     const HOUR: i64 = 60 * 60;
     let now = time();
-    let transport_id: u32 = 1;
-    let other_transport_id: u32 = 2;
-    let uidvalidity = 12345u32;
-
-    async fn is_deleted(context: &Context, mid: &str) -> Result<bool> {
-        Ok(context
-            .sql
-            .count(
-                "SELECT COUNT(*) FROM imap WHERE target='' AND rfc724_mid=?",
-                (mid,),
+    let transport_id = 1;
+    let uidvalidity = 12345;
+    for (id, timestamp, ephemeral_timestamp) in &[
+        (900, now - 2 * HOUR, 0),
+        (1010, now - 23 * HOUR, 0),
+        (1020, now - 21 * HOUR, 0),
+        (1030, now - 19 * HOUR, 0),
+        (2000, now - 18 * HOUR, now - HOUR),
+        (2020, now - 17 * HOUR, now + HOUR),
+        (3000, now + HOUR, 0),
+    ] {
+        let message_id = id.to_string();
+        t.sql
+               .execute(
+                   "INSERT INTO msgs (id, rfc724_mid, timestamp, ephemeral_timestamp) VALUES (?,?,?,?);",
+                   (id, &message_id, timestamp, ephemeral_timestamp),
+               )
+               .await?;
+        t.sql
+            .execute(
+                "INSERT INTO imap (transport_id, rfc724_mid, folder, uid, target, uidvalidity) VALUES (?, ?,'INBOX',?, 'INBOX', ?);",
+                (transport_id, &message_id, id, uidvalidity),
             )
-            .await?
-            == 1)
+            .await?;
     }
 
-    async fn reset_targets(context: &Context) {
+    async fn test_marked_for_deletion(context: &Context, id: u32) -> Result<()> {
+        assert_eq!(
+            context
+                .sql
+                .count(
+                    "SELECT COUNT(*) FROM imap WHERE target='' AND rfc724_mid=?",
+                    (id.to_string(),),
+                )
+                .await?,
+            1
+        );
+        Ok(())
+    }
+
+    async fn remove_uid(context: &Context, id: u32) -> Result<()> {
         context
             .sql
-            .execute("UPDATE imap SET target='INBOX'", ())
-            .await
-            .unwrap();
-    }
-
-    // ── Test messages ────────────────────────────────────────────────────────
-    //
-    // (id, rfc724_mid, ephemeral_timestamp, download_state, pre_rfc724_mid)
-    //
-    //  "expired"          – expired ephemeral, no pre-msg
-    //  "no_expire"        – ephemeral_timestamp=0, not Done  → never deleted
-    //  "future"           – future ephemeral, not Done       → never deleted
-    //  "done"             – Done, no ephemeral               → branch 1 only
-    //  "pre_no_expire_*"  – has pre-msg, but no expiry/Done
-    //  "pre_expired_*"    – has pre-msg, expired ephemeral
-    //  "pre_future_*"     – has pre-msg, future ephemeral
-    //  "wrong_tid"        – expired+Done, but wrong transport_id in imap
-    let msgs: &[(&str, i64, DownloadState, &str)] = &[
-        ("expired", now - HOUR, DownloadState::Available, ""),
-        ("no_expire", 0, DownloadState::Available, ""),
-        ("future", now + HOUR, DownloadState::Available, ""),
-        ("done", 0, DownloadState::Done, ""),
-        (
-            "pre_no_expire_post",
-            0,
-            DownloadState::Available,
-            "pre_no_expire_pre",
-        ),
-        (
-            "pre_expired_post",
-            now - HOUR,
-            DownloadState::Available,
-            "pre_expired_pre",
-        ),
-        (
-            "pre_future_post",
-            now + HOUR,
-            DownloadState::Available,
-            "pre_future_pre",
-        ),
-        ("wrong_tid", now - HOUR, DownloadState::Done, ""),
-    ];
-    for (mid, eph_ts, dl_state, pre_mid) in msgs {
-        t.sql
-            .execute(
-                "INSERT INTO msgs \
-                 (rfc724_mid, timestamp, ephemeral_timestamp, download_state, pre_rfc724_mid) \
-                 VALUES (?,?,0,?,?,?)",
-                (*mid, *eph_ts, *dl_state, *pre_mid),
-            )
+            .execute("DELETE FROM imap WHERE rfc724_mid=?", (id.to_string(),))
             .await?;
+        Ok(())
     }
 
-    // One imap row per mid (including separate rows for pre-messages),
-    // plus "wrong_tid" on a different transport_id.
-    let imap_rows: &[(&str, u32)] = &[
-        ("expired", transport_id),
-        ("no_expire", transport_id),
-        ("future", transport_id),
-        ("done", transport_id),
-        ("pre_no_expire_post", transport_id),
-        ("pre_no_expire_pre", transport_id), // the pre-message's own imap row
-        ("pre_expired_post", transport_id),
-        ("pre_expired_pre", transport_id),
-        ("pre_future_post", transport_id),
-        ("pre_future_pre", transport_id),
-        ("wrong_tid", other_transport_id), // transport_id filter test
-    ];
-    for (i, (mid, tid)) in imap_rows.iter().enumerate() {
+    // This should mark message 2000 for deletion.
+    delete_expired_imap_messages(&t).await?;
+    test_marked_for_deletion(&t, 2000).await?;
+    remove_uid(&t, 2000).await?;
+    // No other messages are marked for deletion.
+    assert_eq!(
         t.sql
-            .execute(
-                "INSERT INTO imap \
-                 (transport_id, rfc724_mid, folder, uid, target, uidvalidity) \
-                 VALUES (?,?,'INBOX',?,'INBOX',?)",
-                (*tid, *mid, (i + 1) as u32, uidvalidity),
-            )
-            .await?;
-    }
+            .count("SELECT COUNT(*) FROM imap WHERE target=''", ())
+            .await?,
+        0
+    );
 
-    // ── Branch 1: is_chatmail=true, BccSelf=false (default) ─────────────────
-    //
-    // SQL deletes: (ephemeral_timestamp!=0 AND <=now) OR download_state=Done
-    // Pre-messages: ALL with pre_rfc724_mid!='' unconditionally.
-    delete_expired_imap_messages(&t, transport_id, true).await?;
-
-    // Tests (ephemeral_timestamp!=0 AND ephemeral_timestamp<=now) path.
-    assert!(is_deleted(&t, "expired").await?);
-    // Tests the ephemeral_timestamp!=0 guard: timestamp=0 satisfies <=now but must not match.
-    assert!(!is_deleted(&t, "no_expire").await?);
-    // Tests the ephemeral_timestamp<=now guard.
-    assert!(!is_deleted(&t, "future").await?);
-    // Tests the OR download_state=Done clause.
-    assert!(is_deleted(&t, "done").await?);
-    // Post-message: no expiry, not Done → not deleted.
-    assert!(!is_deleted(&t, "pre_no_expire_post").await?);
-    // Pre-message: deleted unconditionally (tests UNION SELECT pre_rfc724_mid ... WHERE pre_rfc724_mid!='').
-    assert!(is_deleted(&t, "pre_no_expire_pre").await?);
-    // Post-message with expired ephemeral → deleted.
-    assert!(is_deleted(&t, "pre_expired_post").await?);
-    // Pre-message of expired post → deleted (unconditional pre path).
-    assert!(is_deleted(&t, "pre_expired_pre").await?);
-    // Post-message with future ephemeral → not deleted.
-    assert!(!is_deleted(&t, "pre_future_post").await?);
-    // Pre-message of future post → still deleted (branch 1 pre path has NO ephemeral condition).
-    // If the pre UNION clause gains an ephemeral condition, this would wrongly not be deleted.
-    assert!(is_deleted(&t, "pre_future_pre").await?);
-    // Tests transport_id=?1: expired+Done but on wrong transport_id → not deleted.
-    assert!(!is_deleted(&t, "wrong_tid").await?);
-
-    reset_targets(&t).await;
-
-    // ── Branch 2: is_chatmail=false ──────────────────────────────────────────
-    //
-    // SQL deletes: ephemeral_timestamp!=0 AND <=now only (no Done).
-    // Pre-messages: only when the post also satisfies the ephemeral condition.
-    delete_expired_imap_messages(&t, transport_id, false).await?;
-
-    // Expired ephemeral → deleted.
-    assert!(is_deleted(&t, "expired").await?);
-    // ephemeral_timestamp=0 → not deleted (tests !=0 guard in branch 2).
-    assert!(!is_deleted(&t, "no_expire").await?);
-    // Future ephemeral → not deleted (tests <=now guard in branch 2).
-    assert!(!is_deleted(&t, "future").await?);
-    // Done without expired ephemeral → NOT deleted (key branch 1 vs 2 difference).
-    // If download_state=Done were added to branch 2, this would wrongly be deleted.
-    assert!(!is_deleted(&t, "done").await?);
-    // Post-message: no expiry → not deleted.
-    assert!(!is_deleted(&t, "pre_no_expire_post").await?);
-    // Pre-message of non-expiring post → NOT deleted
-    // (tests ephemeral_timestamp!=0 in branch 2's pre subquery).
-    assert!(!is_deleted(&t, "pre_no_expire_pre").await?);
-    // Post-message with expired ephemeral → deleted.
-    assert!(is_deleted(&t, "pre_expired_post").await?);
-    // Pre-message of expired post → deleted (tests full ephemeral condition in pre subquery).
-    assert!(is_deleted(&t, "pre_expired_pre").await?);
-    // Post-message with future ephemeral → not deleted.
-    assert!(!is_deleted(&t, "pre_future_post").await?);
-    // Pre-message of future post → NOT deleted
-    // (tests ephemeral_timestamp<=now in branch 2's pre subquery).
-    // If the <=now guard were removed there, this would wrongly be deleted.
-    assert!(!is_deleted(&t, "pre_future_pre").await?);
-    // Wrong transport_id → not deleted.
-    assert!(!is_deleted(&t, "wrong_tid").await?);
-
-    reset_targets(&t).await;
-
-    // ── BccSelf=true forces branch 2 even when is_chatmail=true ─────────────
-    //
-    // Tests the `!BccSelf` part of the Rust condition.
-    // If `!BccSelf` were dropped, Done would be deleted here (branch 1 behaviour).
-    t.set_config(Config::BccSelf, Some("1")).await?;
-    delete_expired_imap_messages(&t, transport_id, true).await?;
-    assert!(!is_deleted(&t, "done").await?); // must stay on branch 2
-    assert!(is_deleted(&t, "expired").await?); // branch 2 still runs normally
+    MsgId::new(1010)
+        .update_download_state(&t, DownloadState::Available)
+        .await?;
+    delete_expired_imap_messages(&t).await?;
+    // Keep downloadable for now.
+    assert_eq!(
+        t.sql
+            .count("SELECT COUNT(*) FROM imap WHERE target=''", ())
+            .await?,
+        0
+    );
 
     Ok(())
 }

src/imap.rs

@@ -21,6 +21,9 @@ use futures_lite::FutureExt;
 use ratelimit::Ratelimit;
 use url::Url;
 
+use crate::calls::{
+    UnresolvedIceServer, create_fallback_ice_servers, create_ice_servers_from_metadata,
+};
 use crate::chat::{self, ChatId, ChatIdBlocked, add_device_msg};
 use crate::chatlist_events;
 use crate::config::Config;
@@ -46,10 +49,6 @@ use crate::tools::{self, create_id, duration_to_str, time};
 use crate::transport::{
     ConfiguredLoginParam, ConfiguredServerLoginParam, prioritize_server_login_params,
 };
-use crate::{
-    calls::{UnresolvedIceServer, create_fallback_ice_servers, create_ice_servers_from_metadata},
-    ephemeral::delete_expired_imap_messages,
-};
 
 pub(crate) mod capabilities;
 mod client;
@@ -526,12 +525,6 @@ impl Imap {
             context.scheduler.interrupt_ephemeral_task().await;
         }
 
-        // Mark expired messages for deletion. Note that `delete_expired_imap_messages` is not
-        // 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")?;
-
         session
             .move_delete_messages(context, watch_folder)
             .await
@@ -1288,6 +1281,7 @@ impl Session {
         if request_uids.is_empty() {
             return Ok(());
         }
+        let is_chatmail = self.is_chatmail();
 
         for (request_uids, set) in build_sequence_sets(&request_uids)? {
             info!(context, "Starting UID FETCH of message set \"{}\".", set);
@@ -1387,18 +1381,17 @@ impl Session {
                 );
                 let res = receive_imf_inner(context, rfc724_mid, body, is_seen).await;
 
-                // TODO I don't think this code is needed anymore:
-                // // If the message is not needed anymore on the server, mark it for deletion:
-                // if !context.get_config_bool(Config::BccSelf).await? && is_chatmail {
-                //     context
-                //         .sql
-                //         .execute(
-                //             "UPDATE imap SET target='' WHERE rfc724_mid=?",
-                //             (rfc724_mid,),
-                //         )
-                //         .await?;
-                //     context.scheduler.interrupt_inbox().await;
-                // }
+                // If the message is not needed anymore on the server, mark it for deletion:
+                if !context.get_config_bool(Config::BccSelf).await? && is_chatmail {
+                    context
+                        .sql
+                        .execute(
+                            "UPDATE imap SET target='' WHERE rfc724_mid=?",
+                            (rfc724_mid,),
+                        )
+                        .await?;
+                    context.scheduler.interrupt_inbox().await;
+                }
 
                 // If there was an error receiving the message, show a device message:
                 let received_msg = match res {

src/scheduler.rs

@@ -15,7 +15,7 @@ use crate::config::Config;
 use crate::contact::{ContactId, RecentlySeenLoop};
 use crate::context::Context;
 use crate::download::{download_known_post_messages_without_pre_message, download_msgs};
-use crate::ephemeral;
+use crate::ephemeral::{self, delete_expired_imap_messages};
 use crate::events::EventType;
 use crate::imap::{Imap, session::Session};
 use crate::location;
@@ -484,6 +484,14 @@ async fn fetch_idle(ctx: &Context, connection: &mut Imap, mut session: Session)
         .await
         .context("fetch_move_delete")?;
 
+    // Mark expired messages for deletion. Marked messages will be deleted from the server
+    // on the next iteration of `fetch_move_delete`. `delete_expired_imap_messages` is not
+    // called right before `fetch_move_delete` because it is not well optimized and would
+    // otherwise slow down message fetching.
+    delete_expired_imap_messages(ctx)
+        .await
+        .context("delete_expired_imap_messages")?;
+
     download_known_post_messages_without_pre_message(ctx, &mut session).await?;
     download_msgs(ctx, &mut session)
         .await