fix: Don't download pre-message again if it is known already (#8488)

There was a bug in prefetch_should_download() that it made it return
true for pre-messages even when they were already downloaded. This meant
that pre-messages were downloaded from all relays, rather than just one,
wasting internet data.

The fix is in rfc724_mid_download_tried(), which is used by
prefetch_should_download() to determine whether a message was already
downloaded.

---------

Co-authored-by: l <link2xt@testrun.org>

src/imap.rs

@@ -1623,7 +1623,7 @@ pub(crate) async fn prefetch_should_download(
     message_id: &str,
     mut flags: impl Iterator<Item = Flag<'_>>,
 ) -> Result<bool> {
-    if message::rfc724_mid_download_tried(context, message_id).await? {
+    if message::rfc724_mid_fetch_tried(context, message_id).await? {
         if let Some(from) = mimeparser::get_from(headers)
             && context.is_self_addr(&from.addr).await?
         {

src/message.rs

@@ -2177,25 +2177,38 @@ pub(crate) async fn rfc724_mid_exists_ex(
     Ok(res)
 }
 
-/// Returns `true` iff there is a message
-/// with the given `rfc724_mid`
-/// and a download state other than `DownloadState::Available`,
-/// i.e. it was already tried to download the message or it's sent locally.
-pub(crate) async fn rfc724_mid_download_tried(context: &Context, rfc724_mid: &str) -> Result<bool> {
+/// Returns `true` if the given `rfc724_mid` has nothing left to fetch from a server,
+/// i.e. it was already fetched or is an outgoing message.
+///
+/// For post-messages, this returns `true` if an attempt to fetch was made or is ongoing,
+/// even if this was not successful,
+/// because we don't want to automatically try fetching these messages over and over again
+/// (this function is not called when the user manually clicked "Download").
+pub(crate) async fn rfc724_mid_fetch_tried(context: &Context, rfc724_mid: &str) -> Result<bool> {
     let rfc724_mid = rfc724_mid.trim_start_matches('<').trim_end_matches('>');
     if rfc724_mid.is_empty() {
-        warn!(
-            context,
-            "Empty rfc724_mid passed to rfc724_mid_download_tried"
-        );
+        warn!(context, "Empty rfc724_mid passed to rfc724_mid_fetch_tried");
         return Ok(false);
     }
 
+    // Explanation of the SQL statement:
+    // - For messages that were not split into pre- and post-messages,
+    //   the SQL statement is equal to `rfc724_mid=?1`
+    //   because `download_state` is always `Done` and `pre_rfc724_mid` is always an empty string.
+    // - For post-messages, we want to select them only if an attempt to fetch was made,
+    //   i.e. if `download_state!=Available`.
+    //   The Message-Id header of the post-message goes into the rfc724_mid column,
+    //   so that this is where we need to check for post-messages.
+    // - For pre-messages, the `pre_rfc724_mid` column is checked.
+    //   The pre-message is always immediately fully downloaded,
+    //   just as messages that were not split into pre- and post-messages,
+    //   so that we do not need to check the download state.
     let res = context
         .sql
         .exists(
             "SELECT COUNT(*) FROM msgs
-             WHERE rfc724_mid=? AND download_state<>?",
+             WHERE (rfc724_mid=?1 AND download_state<>?2)
+                OR pre_rfc724_mid=?1",
             (rfc724_mid, DownloadState::Available),
         )
         .await?;

src/tests/pre_messages/receiving.rs

@@ -8,12 +8,17 @@ use crate::chat::send_msg;
 use crate::config::Config;
 use crate::contact;
 use crate::download::{DownloadState, PRE_MSG_ATTACHMENT_SIZE_THRESHOLD, PostMsgMetadata};
+use crate::headerdef::HeaderDef;
+use crate::headerdef::HeaderDefMap;
+use crate::imap::prefetch_should_download;
 use crate::message::{Message, MessageState, Viewtype, delete_msgs, markseen_msgs};
 use crate::mimeparser::MimeMessage;
 use crate::param::Param;
 use crate::reaction::{get_msg_reactions, send_reaction};
 use crate::receive_imf::receive_imf;
 use crate::summary::assert_summary_texts;
+use crate::test_utils::SentMessage;
+use crate::test_utils::TestContext;
 use crate::test_utils::TestContextManager;
 use crate::tests::pre_messages::util::{
     big_webxdc_app, send_large_file_message, send_large_image_message, send_large_webxdc_message,
@@ -159,6 +164,20 @@ async fn test_receive_webxdc() -> Result<()> {
 /// for file attachment
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_receive_pre_message_and_dl_post_message() -> Result<()> {
+    async fn would_download(t: &TestContext, message: &SentMessage<'_>) -> bool {
+        let headers = mailparse::parse_mail(message.payload().as_bytes())
+            .unwrap()
+            .headers;
+        prefetch_should_download(
+            t,
+            &headers,
+            &headers.get_header_value(HeaderDef::MessageId).unwrap(),
+            std::iter::empty(),
+        )
+        .await
+        .unwrap()
+    }
+
     let mut tcm = TestContextManager::new();
     let alice = &tcm.alice().await;
     let bob = &tcm.bob().await;
@@ -168,12 +187,23 @@ async fn test_receive_pre_message_and_dl_post_message() -> Result<()> {
         send_large_file_message(alice, alice_group_id, Viewtype::File, &vec![0u8; 1_000_000])
             .await?;
 
+    assert!(would_download(bob, &pre_message).await);
+    // `prefetch_should_download` doesn't check the `Chat-Is-Post-Message` header,
+    // so that it will simply return true as long as the message is unknown:
+    assert!(would_download(bob, &post_message).await);
+
     let msg = bob.recv_msg(&pre_message).await;
     assert_eq!(msg.download_state(), DownloadState::Available);
     assert_eq!(msg.viewtype, Viewtype::Text);
     assert!(msg.param.exists(Param::PostMessageViewtype));
     assert!(msg.param.exists(Param::PostMessageFileBytes));
     assert_eq!(msg.text, "test".to_owned());
+
+    // The pre-message is known now, shouldn't be downloaded again:
+    assert_eq!(would_download(bob, &pre_message).await, false);
+    // ...But the post-message should be downloaded once it's available:
+    assert!(would_download(bob, &post_message).await);
+
     let _ = bob.recv_msg_trash(&post_message).await;
     let msg = Message::load_from_db(bob, msg.id).await?;
     assert_eq!(msg.download_state(), DownloadState::Done);
@@ -181,6 +211,11 @@ async fn test_receive_pre_message_and_dl_post_message() -> Result<()> {
     assert_eq!(msg.param.exists(Param::PostMessageViewtype), false);
     assert_eq!(msg.param.exists(Param::PostMessageFileBytes), false);
     assert_eq!(msg.text, "test".to_owned());
+
+    // Everything downloaded, if something is received again it should be ignored:
+    assert_eq!(would_download(bob, &pre_message).await, false);
+    assert_eq!(would_download(bob, &post_message).await, false);
+
     Ok(())
 }