From dc267fe1fb5c7f1154755cc9a263f6581dde1f57 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Sat, 1 Aug 2026 12:48:24 +0200 Subject: [PATCH] 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 --- src/imap.rs | 2 +- src/message.rs | 33 ++++++++++++++++++--------- src/tests/pre_messages/receiving.rs | 35 +++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/src/imap.rs b/src/imap.rs index 08ba89e88..6378137b1 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -1623,7 +1623,7 @@ pub(crate) async fn prefetch_should_download( message_id: &str, mut flags: impl Iterator>, ) -> Result { - 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? { diff --git a/src/message.rs b/src/message.rs index 4a80b5744..550e0ad5e 100644 --- a/src/message.rs +++ b/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 { +/// 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 { 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?; diff --git a/src/tests/pre_messages/receiving.rs b/src/tests/pre_messages/receiving.rs index 7a2586e2c..ee1be4298 100644 --- a/src/tests/pre_messages/receiving.rs +++ b/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(()) }