fix: lookup_chat_by_reply(): Skip not fully downloaded and undecipherable messages (#4676)

Such a message may be assigned to a wrong chat (e.g. undecipherable group msgs often get assigned to
the 1:1 chat with the sender). Add `DownloadState::Undecipherable` so that messages referencing
undecipherable ones don't go to that wrong chat too. Also do not reply to not fully downloaded
messages. Before `Message.error` was checked for that purpose, but a message can be error for many
reasons.

deltachat-jsonrpc/src/api/types/message.rs

@@ -318,6 +318,7 @@ pub enum DownloadState {
     Done,
     Available,
     Failure,
+    Undecipherable,
     InProgress,
 }
 
@@ -327,6 +328,7 @@ impl From<download::DownloadState> for DownloadState {
             download::DownloadState::Done => DownloadState::Done,
             download::DownloadState::Available => DownloadState::Available,
             download::DownloadState::Failure => DownloadState::Failure,
+            download::DownloadState::Undecipherable => DownloadState::Undecipherable,
             download::DownloadState::InProgress => DownloadState::InProgress,
         }
     }

deltachat-repl/src/cmdline.rs

@@ -187,6 +187,7 @@ async fn log_msg(context: &Context, prefix: impl AsRef<str>, msg: &Message) {
         DownloadState::Available => " [⬇ Download available]",
         DownloadState::InProgress => " [⬇ Download in progress...]️",
         DownloadState::Failure => " [⬇ Download failed]",
+        DownloadState::Undecipherable => " [⬇ Decryption failed]",
     };
 
     let temp2 = timestamp_to_str(msg.get_timestamp());

src/chat.rs

@@ -23,6 +23,7 @@ use crate::constants::{
 use crate::contact::{Contact, ContactId, Origin, VerifiedStatus};
 use crate::context::Context;
 use crate::debug_logging::maybe_set_logging_xdc;
+use crate::download::DownloadState;
 use crate::ephemeral::Timer as EphemeralTimer;
 use crate::events::EventType;
 use crate::html::new_html_mimepart;
@@ -1038,11 +1039,14 @@ impl ChatId {
         T: Send + 'static,
     {
         let sql = &context.sql;
+        // Do not reply to not fully downloaded messages. Such a message could be a group chat
+        // message that we assigned to 1:1 chat.
         let query = format!(
             "SELECT {fields} \
-             FROM msgs WHERE chat_id=? AND state NOT IN (?, ?) AND NOT hidden \
+             FROM msgs WHERE chat_id=? AND state NOT IN (?, ?) AND NOT hidden AND download_state={} \
              ORDER BY timestamp DESC, id DESC \
-             LIMIT 1;"
+             LIMIT 1;",
+             DownloadState::Done as u32,
         );
         let row = sql
             .query_row_optional(
@@ -1067,34 +1071,17 @@ impl ChatId {
         self,
         context: &Context,
     ) -> Result<Option<(String, String, String)>> {
-        if let Some((rfc724_mid, mime_in_reply_to, mime_references, error)) = self
-            .parent_query(
-                context,
-                "rfc724_mid, mime_in_reply_to, mime_references, error",
-                |row: &rusqlite::Row| {
-                    let rfc724_mid: String = row.get(0)?;
-                    let mime_in_reply_to: String = row.get(1)?;
-                    let mime_references: String = row.get(2)?;
-                    let error: String = row.get(3)?;
-                    Ok((rfc724_mid, mime_in_reply_to, mime_references, error))
-                },
-            )
-            .await?
-        {
-            if !error.is_empty() {
-                // Do not reply to error messages.
-                //
-                // An error message could be a group chat message that we failed to decrypt and
-                // assigned to 1:1 chat. A reply to it will show up as a reply to group message
-                // on the other side. To avoid such situations, it is better not to reply to
-                // error messages at all.
-                Ok(None)
-            } else {
-                Ok(Some((rfc724_mid, mime_in_reply_to, mime_references)))
-            }
-        } else {
-            Ok(None)
-        }
+        self.parent_query(
+            context,
+            "rfc724_mid, mime_in_reply_to, mime_references",
+            |row: &rusqlite::Row| {
+                let rfc724_mid: String = row.get(0)?;
+                let mime_in_reply_to: String = row.get(1)?;
+                let mime_references: String = row.get(2)?;
+                Ok((rfc724_mid, mime_in_reply_to, mime_references))
+            },
+        )
+        .await
     }
 
     /// Returns multi-line text summary of encryption preferences of all chat contacts.

src/download.rs

@@ -59,6 +59,9 @@ pub enum DownloadState {
     /// Failed to fully download the message.
     Failure = 20,
 
+    /// Undecipherable message.
+    Undecipherable = 30,
+
     /// Full download of the message is in progress.
     InProgress = 1000,
 }
@@ -80,7 +83,9 @@ impl MsgId {
     pub async fn download_full(self, context: &Context) -> Result<()> {
         let msg = Message::load_from_db(context, self).await?;
         match msg.download_state() {
-            DownloadState::Done => return Err(anyhow!("Nothing to download.")),
+            DownloadState::Done | DownloadState::Undecipherable => {
+                return Err(anyhow!("Nothing to download."))
+            }
             DownloadState::InProgress => return Err(anyhow!("Download already in progress.")),
             DownloadState::Available | DownloadState::Failure => {
                 self.update_download_state(context, DownloadState::InProgress)

src/receive_imf.rs

@@ -1227,6 +1227,8 @@ RETURNING id
                     ephemeral_timestamp,
                     if is_partial_download.is_some() {
                         DownloadState::Available
+                    } else if mime_parser.decrypting_failed {
+                        DownloadState::Undecipherable
                     } else {
                         DownloadState::Done
                     },
@@ -1409,11 +1411,9 @@ async fn lookup_chat_by_reply(
     if let Some(parent) = parent {
         let parent_chat = Chat::load_from_db(context, parent.chat_id).await?;
 
-        if parent.error.is_some() {
-            // If the parent msg is undecipherable, then it may have been assigned to the wrong chat
-            // (undecipherable group msgs often get assigned to the 1:1 chat with the sender).
-            // We don't have any way of finding out whether a msg is undecipherable, so we check for
-            // error.is_some() instead.
+        if parent.download_state != DownloadState::Done {
+            // If the parent msg is not fully downloaded or undecipherable, it may have been
+            // assigned to the wrong chat (they often get assigned to the 1:1 chat with the sender).
             return Ok(None);
         }