fix: Return MessageState::OutMdnRcvd for 1:1 failed-to-deliver messages with MDNs (#8241)

The docs for `OutFailed` say that it's for unrecoverable errors, but if a message failed to be
delivered to one relay of the contact, but was delivered to another relay and an MDN was received,
it's rather an expected condition and there's no much sense in displaying the message as
failed. MDNs are enabled by default, so this should work for 1:1 chats for most users.

For group chats this isn't easy to fix however, a message may fail to be delivered for one member,
but there may be MDNs from other members.

src/message.rs

@@ -84,15 +84,19 @@ impl MsgId {
         let result = context
             .sql
             .query_row_optional(
-                "SELECT m.state, mdns.msg_id
-                  FROM msgs m LEFT JOIN msgs_mdns mdns ON mdns.msg_id=m.id
-                  WHERE id=?
-                  LIMIT 1",
+                "
+SELECT m.state, mdns.msg_id, c.type
+FROM msgs m
+LEFT JOIN msgs_mdns mdns ON mdns.msg_id=m.id
+LEFT JOIN chats c ON c.id=m.chat_id
+WHERE m.id=?
+LIMIT 1",
                 (self,),
                 |row| {
                     let state: MessageState = row.get(0)?;
                     let mdn_msg_id: Option<MsgId> = row.get(1)?;
-                    Ok(state.with_mdns(mdn_msg_id.is_some()))
+                    let chat_type: Option<Chattype> = row.get(2)?;
+                    Ok(state.with_mdns(mdn_msg_id.is_some(), chat_type))
                 },
             )
             .await?
@@ -536,6 +540,7 @@ impl Message {
                     m.param AS param,
                     m.hidden AS hidden,
                     m.location_id AS location,
+                    c.type AS chat_type,
                     c.archived AS visibility,
                     c.blocked AS blocked
                  FROM msgs m
@@ -547,6 +552,7 @@ impl Message {
                 |row| {
                     let state: MessageState = row.get("state")?;
                     let mdn_msg_id: Option<MsgId> = row.get("mdn_msg_id")?;
+                    let chat_type: Option<Chattype> = row.get("chat_type")?;
                     let text = match row.get_ref("txt")? {
                         rusqlite::types::ValueRef::Text(buf) => {
                             match String::from_utf8(buf.to_vec()) {
@@ -582,7 +588,7 @@ impl Message {
                         ephemeral_timer: row.get("ephemeral_timer")?,
                         ephemeral_timestamp: row.get("ephemeral_timestamp")?,
                         viewtype: row.get("type").unwrap_or_default(),
-                        state: state.with_mdns(mdn_msg_id.is_some()),
+                        state: state.with_mdns(mdn_msg_id.is_some(), chat_type),
                         download_state: row.get("download_state")?,
                         error: Some(row.get::<_, String>("error")?)
                             .filter(|error| !error.is_empty()),
@@ -1485,11 +1491,17 @@ impl MessageState {
     }
 
     /// Returns adjusted message state if the message has MDNs.
-    pub(crate) fn with_mdns(self, has_mdns: bool) -> Self {
-        if self == MessageState::OutDelivered && has_mdns {
-            return MessageState::OutMdnRcvd;
+    pub(crate) fn with_mdns(self, has_mdns: bool, chat_type: Option<Chattype>) -> Self {
+        if !has_mdns {
+            return self;
+        }
+        match self {
+            MessageState::OutFailed if chat_type == Some(Chattype::Single) => {
+                MessageState::OutMdnRcvd
+            }
+            MessageState::OutDelivered => MessageState::OutMdnRcvd,
+            _ => self,
         }
-        self
     }
 }
 

src/message/message_tests.rs

@@ -450,6 +450,12 @@ async fn test_get_state() -> Result<()> {
     markseen_msgs(&bob, vec![bob_msg.id]).await?;
     assert_state(&bob, bob_msg.id, MessageState::InSeen).await;
 
+    let sent = bob.get_sent_mdn().await;
+    alice.recv_msg_trash(&sent).await;
+    let alice_msg = Message::load_from_db(&alice, alice_msg.id).await?;
+    assert_eq!(alice_msg.get_state(), MessageState::OutMdnRcvd);
+    assert_eq!(alice_msg.error().unwrap(), "badly failed");
+
     Ok(())
 }
 

src/smtp.rs

@@ -492,7 +492,35 @@ async fn send_mdns(context: &Context, connection: &mut Smtp) -> Result<()> {
             return Ok(());
         }
 
-        let more_mdns = send_mdn(context, connection).await?;
+        let more_mdns = send_mdn(context, async |rfc724_mid, body, recipients| {
+            let recipients: Vec<_> = recipients
+                .into_iter()
+                .filter_map(|addr| {
+                    async_smtp::EmailAddress::new(addr.clone())
+                        .with_context(|| format!("Invalid recipient: {addr}"))
+                        .log_err(context)
+                        .ok()
+                })
+                .collect();
+
+            match smtp_send(context, &recipients, &body, connection, None).await {
+                SendResult::Success => {
+                    if !recipients.is_empty() {
+                        info!(context, "Successfully sent MDN for {rfc724_mid}.");
+                    }
+                    Ok(true)
+                }
+                SendResult::Retry => {
+                    info!(
+                        context,
+                        "Temporary SMTP failure while sending an MDN for {rfc724_mid}."
+                    );
+                    Ok(false)
+                }
+                SendResult::Failure(err) => Err(err),
+            }
+        })
+        .await?;
         if !more_mdns {
             // No more MDNs to send or one of them failed.
             return Ok(());
@@ -550,7 +578,7 @@ async fn send_mdn_rfc724_mid(
     context: &Context,
     rfc724_mid: &str,
     contact_id: ContactId,
-    smtp: &mut Smtp,
+    send_fn: impl AsyncFnOnce(&str, String, Vec<String>) -> Result<bool>,
 ) -> Result<bool> {
     let contact = Contact::get_by_id(context, contact_id).await?;
     if contact.is_blocked() {
@@ -590,48 +618,29 @@ async fn send_mdn_rfc724_mid(
     if context.get_config_bool(Config::BccSelf).await? {
         add_self_recipients(context, &mut recipients, encrypted).await?;
     }
-    let recipients: Vec<_> = recipients
-        .into_iter()
-        .filter_map(|addr| {
-            async_smtp::EmailAddress::new(addr.clone())
-                .with_context(|| format!("Invalid recipient: {addr}"))
-                .log_err(context)
-                .ok()
-        })
-        .collect();
     message::insert_tombstone(context, &rendered_msg.rfc724_mid).await?;
-    match smtp_send(context, &recipients, &body, smtp, None).await {
-        SendResult::Success => {
-            if !recipients.is_empty() {
-                info!(context, "Successfully sent MDN for {rfc724_mid}.");
-            }
-            context
-                .sql
-                .transaction(|transaction| {
-                    let mut stmt =
-                        transaction.prepare("DELETE FROM smtp_mdns WHERE rfc724_mid = ?")?;
-                    stmt.execute((rfc724_mid,))?;
-                    for additional_rfc724_mid in additional_rfc724_mids {
-                        stmt.execute((additional_rfc724_mid,))?;
-                    }
-                    Ok(())
-                })
-                .await?;
-            Ok(true)
-        }
-        SendResult::Retry => {
-            info!(
-                context,
-                "Temporary SMTP failure while sending an MDN for {rfc724_mid}."
-            );
-            Ok(false)
-        }
-        SendResult::Failure(err) => Err(err),
+    let sent = send_fn(rfc724_mid, body, recipients).await?;
+    if sent {
+        context
+            .sql
+            .transaction(|transaction| {
+                let mut stmt = transaction.prepare("DELETE FROM smtp_mdns WHERE rfc724_mid = ?")?;
+                stmt.execute((rfc724_mid,))?;
+                for additional_rfc724_mid in additional_rfc724_mids {
+                    stmt.execute((additional_rfc724_mid,))?;
+                }
+                Ok(())
+            })
+            .await?;
     }
+    Ok(sent)
 }
 
 /// Tries to send a single MDN. Returns true if more MDNs should be sent.
-async fn send_mdn(context: &Context, smtp: &mut Smtp) -> Result<bool> {
+pub(crate) async fn send_mdn(
+    context: &Context,
+    send_fn: impl AsyncFnOnce(&str, String, Vec<String>) -> Result<bool>,
+) -> Result<bool> {
     if !context.should_send_mdns().await? {
         context.sql.execute("DELETE FROM smtp_mdns", []).await?;
         return Ok(false);
@@ -668,7 +677,7 @@ async fn send_mdn(context: &Context, smtp: &mut Smtp) -> Result<bool> {
         .await
         .context("Failed to update MDN retries count")?;
 
-    match send_mdn_rfc724_mid(context, &rfc724_mid, contact_id, smtp).await {
+    match send_mdn_rfc724_mid(context, &rfc724_mid, contact_id, send_fn).await {
         Err(err) => {
             // If there is an error, for example there is no message corresponding to the msg_id in the
             // database, do not try to send this MDN again.

src/test_utils.rs

@@ -43,7 +43,7 @@ use crate::mimeparser::{MimeMessage, SystemMessage};
 use crate::pgp::SeipdVersion;
 use crate::receive_imf::{ReceivedMsg, receive_imf};
 use crate::securejoin::{get_securejoin_qr, join_securejoin};
-use crate::smtp::msg_has_pending_smtp_job;
+use crate::smtp::{self, msg_has_pending_smtp_job};
 use crate::stock_str::StockStrings;
 use crate::tools::time;
 
@@ -1080,6 +1080,22 @@ ORDER BY id"
         res
     }
 
+    pub async fn get_sent_mdn(&self) -> SentMessage<'_> {
+        let mut sent_mdn = None;
+        smtp::send_mdn(self, async |_rfc724_mid, body, recipients| {
+            sent_mdn = Some(SentMessage {
+                payload: body,
+                sender_msg_id: MsgId::new(u32::MAX),
+                sender_context: &self.ctx,
+                recipients: recipients.join(" "),
+            });
+            Ok(true)
+        })
+        .await
+        .expect("smtp::send_mdn");
+        sent_mdn.unwrap()
+    }
+
     pub async fn golden_test_chat(&self, chat_id: ChatId, filename: &str) {
         let filename = Path::new("test-data/golden/").join(filename);