From 407f363f265cb0176e0f894b6d2f42041dbbfe26 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Mon, 29 Sep 2025 14:51:58 -0300 Subject: [PATCH 1/2] fix: receive_imf: Report replaced message id in `MsgsChanged` if chat is the same --- src/receive_imf.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 86ae562d2a..f53458e363 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -1007,7 +1007,10 @@ pub(crate) async fn receive_imf_inner( } else if received_msg.hidden { // No need to emit an event about the changed message } else if let Some(replace_chat_id) = replace_chat_id { - context.emit_msgs_changed_without_msg_id(replace_chat_id); + match replace_chat_id == chat_id { + false => context.emit_msgs_changed_without_msg_id(replace_chat_id), + true => context.emit_msgs_changed(chat_id, replace_msg_id.unwrap_or_default()), + } } else if !chat_id.is_trash() { let fresh = received_msg.state == MessageState::InFresh && mime_parser.is_system_message != SystemMessage::CallAccepted From 44261c3f04bbb1d0b2e4364187dad4c7bf5534a8 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Mon, 29 Sep 2025 14:55:57 -0300 Subject: [PATCH 2/2] feat: Don't ignore receive_imf_inner() errors, try adding partially downloaded message instead (#7196) Ignoring `receive_imf_inner()` errors, i.e. silently skipping messages on failures, leads to bugs never fixed. As for temporary I/O errors, ignoring them leads to lost messages, in this case it's better to bubble up the error and get the IMAP loop stuck. However if there's some logic error, it's better to show it to the user so that it's more likely reported, and continue receiving messages. To distinguish these cases, on error, try adding the message as partially downloaded with the error set to `msgs.error`, this way the user also can retry downloading the message to finally see it if the problem is fixed. --- deltachat-rpc-client/tests/test_something.py | 46 ++++++++++++++++++++ src/config.rs | 3 ++ src/context.rs | 7 +++ src/download.rs | 11 ++++- src/imap.rs | 35 ++++++++++----- src/mimeparser.rs | 14 +++--- src/receive_imf.rs | 33 ++++++-------- 7 files changed, 109 insertions(+), 40 deletions(-) diff --git a/deltachat-rpc-client/tests/test_something.py b/deltachat-rpc-client/tests/test_something.py index 8e1301c770..f7c144a4a4 100644 --- a/deltachat-rpc-client/tests/test_something.py +++ b/deltachat-rpc-client/tests/test_something.py @@ -331,6 +331,52 @@ def test_message(acfactory) -> None: assert reactions == snapshot.reactions +def test_receive_imf_failure(acfactory) -> None: + alice, bob = acfactory.get_online_accounts(2) + alice_contact_bob = alice.create_contact(bob, "Bob") + alice_chat_bob = alice_contact_bob.create_chat() + + bob.set_config("fail_on_receiving_full_msg", "1") + alice_chat_bob.send_text("Hello!") + event = bob.wait_for_incoming_msg_event() + chat_id = event.chat_id + msg_id = event.msg_id + message = bob.get_message_by_id(msg_id) + snapshot = message.get_snapshot() + assert snapshot.chat_id == chat_id + assert snapshot.download_state == DownloadState.AVAILABLE + assert snapshot.error is not None + assert snapshot.show_padlock + + # The failed message doesn't break the IMAP loop. + bob.set_config("fail_on_receiving_full_msg", "0") + alice_chat_bob.send_text("Hello again!") + event = bob.wait_for_incoming_msg_event() + assert event.chat_id == chat_id + msg_id = event.msg_id + message1 = bob.get_message_by_id(msg_id) + snapshot = message1.get_snapshot() + assert snapshot.chat_id == chat_id + assert snapshot.download_state == DownloadState.DONE + assert snapshot.error is None + + # The failed message can be re-downloaded later. + bob._rpc.download_full_message(bob.id, message.id) + event = bob.wait_for_event(EventType.MSGS_CHANGED) + message = bob.get_message_by_id(event.msg_id) + snapshot = message.get_snapshot() + assert snapshot.download_state == DownloadState.IN_PROGRESS + event = bob.wait_for_event(EventType.MSGS_CHANGED) + assert event.chat_id == chat_id + msg_id = event.msg_id + message = bob.get_message_by_id(msg_id) + snapshot = message.get_snapshot() + assert snapshot.chat_id == chat_id + assert snapshot.download_state == DownloadState.DONE + assert snapshot.error is None + assert snapshot.text == "Hello!" + + def test_selfavatar_sync(acfactory, data, log) -> None: alice = acfactory.get_online_account() diff --git a/src/config.rs b/src/config.rs index 02ad57374f..fa5acc0bd8 100644 --- a/src/config.rs +++ b/src/config.rs @@ -450,6 +450,9 @@ pub enum Config { /// to avoid encrypting it differently and /// storing the same token multiple times on the server. EncryptedDeviceToken, + + /// Return an error from `receive_imf_inner()` for a fully downloaded message. For tests. + FailOnReceivingFullMsg, } impl Config { diff --git a/src/context.rs b/src/context.rs index c94780a8dd..e44f3a46b0 100644 --- a/src/context.rs +++ b/src/context.rs @@ -1079,6 +1079,13 @@ impl Context { .await? .unwrap_or_default(), ); + res.insert( + "fail_on_receiving_full_msg", + self.sql + .get_raw_config("fail_on_receiving_full_msg") + .await? + .unwrap_or_default(), + ); let elapsed = time_elapsed(&self.creation_time); res.insert("uptime", duration_to_str(elapsed)); diff --git a/src/download.rs b/src/download.rs index 57086a43dc..3f10263279 100644 --- a/src/download.rs +++ b/src/download.rs @@ -238,14 +238,20 @@ impl MimeMessage { /// the mime-structure itself is not available. /// /// The placeholder part currently contains a text with size and availability of the message; + /// `error` is set as the part error; /// in the future, we may do more advanced things as previews here. pub(crate) async fn create_stub_from_partial_download( &mut self, context: &Context, org_bytes: u32, + error: Option, ) -> Result<()> { + let prefix = match error { + None => "", + Some(_) => "[❗] ", + }; let mut text = format!( - "[{}]", + "{prefix}[{}]", stock_str::partial_download_msg_body(context, org_bytes).await ); if let Some(delete_server_after) = context.get_config_delete_server_after().await? { @@ -259,9 +265,10 @@ impl MimeMessage { info!(context, "Partial download: {}", text); - self.parts.push(Part { + self.do_add_single_part(Part { typ: Viewtype::Text, msg: text, + error, ..Default::default() }); diff --git a/src/imap.rs b/src/imap.rs index b29422ac94..e10be9e4ae 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -1480,7 +1480,7 @@ impl Session { context, "Passing message UID {} to receive_imf().", request_uid ); - match receive_imf_inner( + let res = receive_imf_inner( context, folder, uidvalidity, @@ -1488,20 +1488,31 @@ impl Session { rfc724_mid, body, is_seen, - partial, + partial.map(|msg_size| (msg_size, None)), ) - .await - { - Ok(received_msg) => { - received_msgs_channel - .send((request_uid, received_msg)) - .await?; - } - Err(err) => { - warn!(context, "receive_imf error: {:#}.", err); - received_msgs_channel.send((request_uid, None)).await?; + .await; + let received_msg = if let Err(err) = res { + warn!(context, "receive_imf error: {:#}.", err); + if partial.is_some() { + return Err(err); } + receive_imf_inner( + context, + folder, + uidvalidity, + request_uid, + rfc724_mid, + body, + is_seen, + Some((body.len().try_into()?, Some(format!("{err:#}")))), + ) + .await? + } else { + res? }; + received_msgs_channel + .send((request_uid, received_msg)) + .await?; } // If we don't process the whole response, IMAP client is left in a broken state where diff --git a/src/mimeparser.rs b/src/mimeparser.rs index ce4d104598..573e0c5994 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -240,12 +240,12 @@ const MIME_AC_SETUP_FILE: &str = "application/autocrypt-setup"; impl MimeMessage { /// Parse a mime message. /// - /// If `partial` is set, it contains the full message size in bytes - /// and `body` contains the header only. + /// If `partial` is set, it contains the full message size in bytes and an optional error text + /// for the partially downloaded message, and `body` contains the HEADER only. pub(crate) async fn from_bytes( context: &Context, body: &[u8], - partial: Option, + partial: Option<(u32, Option)>, ) -> Result { let mail = mailparse::parse_mail(body)?; @@ -612,9 +612,9 @@ impl MimeMessage { }; match partial { - Some(org_bytes) => { + Some((org_bytes, err)) => { parser - .create_stub_from_partial_download(context, org_bytes) + .create_stub_from_partial_download(context, org_bytes, err) .await?; } None => match mail { @@ -634,7 +634,7 @@ impl MimeMessage { error: Some(format!("Decrypting failed: {err:#}")), ..Default::default() }; - parser.parts.push(part); + parser.do_add_single_part(part); } }, }; @@ -1543,7 +1543,7 @@ impl MimeMessage { Ok(true) } - fn do_add_single_part(&mut self, mut part: Part) { + pub(crate) fn do_add_single_part(&mut self, mut part: Part) { if self.was_encrypted() { part.param.set_int(Param::GuaranteeE2ee, 1); } diff --git a/src/receive_imf.rs b/src/receive_imf.rs index f53458e363..3e9e3d762d 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -196,7 +196,7 @@ pub(crate) async fn receive_imf_from_inbox( rfc724_mid, imf_raw, seen, - is_partial_download, + is_partial_download.map(|msg_size| (msg_size, None)), ) .await } @@ -494,9 +494,8 @@ async fn get_to_and_past_contact_ids( /// If the message is so wrong that we didn't even create a database entry, /// returns `Ok(None)`. /// -/// If `is_partial_download` is set, it contains the full message size in bytes. -/// Do not confuse that with `replace_msg_id` that will be set when the full message is loaded -/// later. +/// If `partial` is set, it contains the full message size in bytes and an optional error text for +/// the partially downloaded message. #[expect(clippy::too_many_arguments)] pub(crate) async fn receive_imf_inner( context: &Context, @@ -506,7 +505,7 @@ pub(crate) async fn receive_imf_inner( rfc724_mid: &str, imf_raw: &[u8], seen: bool, - is_partial_download: Option, + partial: Option<(u32, Option)>, ) -> Result> { if std::env::var(crate::DCC_MIME_DEBUG).is_ok() { info!( @@ -515,9 +514,16 @@ pub(crate) async fn receive_imf_inner( String::from_utf8_lossy(imf_raw), ); } + if partial.is_none() { + ensure!( + !context + .get_config_bool(Config::FailOnReceivingFullMsg) + .await? + ); + } - let mut mime_parser = match MimeMessage::from_bytes(context, imf_raw, is_partial_download).await - { + let is_partial_download = partial.as_ref().map(|(msg_size, _err)| *msg_size); + let mut mime_parser = match MimeMessage::from_bytes(context, imf_raw, partial).await { Err(err) => { warn!(context, "receive_imf: can't parse MIME: {err:#}."); if rfc724_mid.starts_with(GENERATED_PREFIX) { @@ -551,22 +557,11 @@ pub(crate) async fn receive_imf_inner( // make sure, this check is done eg. before securejoin-processing. let (replace_msg_id, replace_chat_id); if let Some((old_msg_id, _)) = message::rfc724_mid_exists(context, rfc724_mid).await? { - if is_partial_download.is_some() { - // Should never happen, see imap::prefetch_should_download(), but still. - info!( - context, - "Got a partial download and message is already in DB." - ); - 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 { // the message was partially downloaded before and is fully downloaded now. - info!( - context, - "Message already partly in DB, replacing by full message." - ); + info!(context, "Message already partly in DB, replacing."); Some(msg.chat_id) } else { None