fix: Order of messages if outgoing reply is received earlier (#7169)

This fixes a scenario when an outgoing reply is received earlier than an incoming message for some
reason like device B having `MvboxMove` enabled, sending the reply and going offline immediately, so
the reply is in Inbox and it's processed by device A earlier because e.g. `MvboxMove` is disabled on
it. Also if we add multi-transport later, this scenario will be just normal.

This allows received outgoing messages to mingle with fresh incoming ones, i.e. sorts them together
purely by timestamp by assigning `InFresh` state to received outgoing messages, but still returning
`OutDelivered` by all APIs for compatibility. NB: We already do such a trick for `OutMdnRcvd`.

As for messages sent locally, there's no need to make them more noticeable even if they are newer,
so received outgoing messages are always added after them.

src/chat.rs

@@ -1218,37 +1218,34 @@ impl ChatId {
                 )
                 .await?
         } else if received {
-            // Received messages shouldn't mingle with just sent ones and appear somewhere in the
-            // middle of the chat, so we go after the newest non fresh message.
-            //
-            // But if a received outgoing message is older than some seen message, better sort the
-            // received message purely by timestamp. We could place it just before that seen
-            // message, but anyway the user may not notice it.
+            // Received incoming messages shouldn't mingle with just sent ones and appear somewhere
+            // in the middle of the chat, so we go after the newest non fresh message. Received
+            // outgoing messages are allowed to mingle with seen messages though to avoid seen
+            // replies appearing before messages sent from another device (cases like the user
+            // sharing the account with others or bots are rare, so let them break sometimes).
             //
             // NB: Received outgoing messages may break sorting of fresh incoming ones, but this
             // shouldn't happen frequently. Seen incoming messages don't really break sorting of
             // fresh ones, they rather mean that older incoming messages are actually seen as well.
+            let states = match incoming {
+                true => "13, 16, 18, 20, 24, 26", // `> MessageState::InFresh`
+                false => "18, 20, 24, 26",        // `> MessageState::InSeen`
+            };
             context
                 .sql
                 .query_row_optional(
-                    "SELECT MAX(timestamp), MAX(IIF(state=?,timestamp_sent,0))
-                     FROM msgs
-                     WHERE chat_id=? AND hidden=0 AND state>?
-                     HAVING COUNT(*) > 0",
-                    (MessageState::InSeen, self, MessageState::InFresh),
+                    &format!(
+                        "SELECT MAX(timestamp) FROM msgs
+                        WHERE state IN ({states}) AND hidden=0 AND chat_id=?
+                        HAVING COUNT(*) > 0"
+                    ),
+                    (self,),
                     |row| {
                         let ts: i64 = row.get(0)?;
-                        let ts_sent_seen: i64 = row.get(1)?;
-                        Ok((ts, ts_sent_seen))
+                        Ok(ts)
                     },
                 )
                 .await?
-                .and_then(|(ts, ts_sent_seen)| {
-                    match incoming || ts_sent_seen <= message_timestamp {
-                        true => Some(ts),
-                        false => None,
-                    }
-                })
         } else {
             None
         };

src/events/payload.rs

@@ -36,7 +36,7 @@ pub enum EventType {
     /// Emitted when an IMAP message has been moved
     ImapMessageMoved(String),
 
-    /// Emitted before going into IDLE on the Inbox folder.
+    /// Emitted before going into IDLE on any folder.
     ImapInboxIdle,
 
     /// Emitted when an new file in the $BLOBDIR was created

src/message.rs

@@ -85,7 +85,7 @@ impl MsgId {
             .sql
             .query_row_optional(
                 concat!(
-                    "SELECT m.state, mdns.msg_id",
+                    "SELECT m.state, m.from_id, mdns.msg_id",
                     " FROM msgs m LEFT JOIN msgs_mdns mdns ON mdns.msg_id=m.id",
                     " WHERE id=?",
                     " LIMIT 1",
@@ -93,8 +93,9 @@ impl MsgId {
                 (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 from_id: ContactId = row.get(1)?;
+                    let mdn_msg_id: Option<MsgId> = row.get(2)?;
+                    Ok(state.with(from_id, mdn_msg_id.is_some()))
                 },
             )
             .await?
@@ -551,6 +552,7 @@ impl Message {
                         }
                         _ => String::new(),
                     };
+                    let from_id = row.get("from_id")?;
                     let msg = Message {
                         id: row.get("id")?,
                         rfc724_mid: row.get::<_, String>("rfc724mid")?,
@@ -558,7 +560,7 @@ impl Message {
                             .get::<_, Option<String>>("mime_in_reply_to")?
                             .and_then(|in_reply_to| parse_message_id(&in_reply_to).ok()),
                         chat_id: row.get("chat_id")?,
-                        from_id: row.get("from_id")?,
+                        from_id,
                         to_id: row.get("to_id")?,
                         timestamp_sort: row.get("timestamp")?,
                         timestamp_sent: row.get("timestamp_sent")?,
@@ -566,7 +568,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(from_id, mdn_msg_id.is_some()),
                         download_state: row.get("download_state")?,
                         error: Some(row.get::<_, String>("error")?)
                             .filter(|error| !error.is_empty()),
@@ -1366,7 +1368,7 @@ pub enum MessageState {
     OutDelivered = 26,
 
     /// Outgoing message read by the recipient (two checkmarks; this
-    /// requires goodwill on the receiver's side). Not used in the db for new messages.
+    /// requires goodwill on the receiver's side). API-only, not used in the db.
     OutMdnRcvd = 28,
 }
 
@@ -1410,10 +1412,16 @@ impl MessageState {
         )
     }
 
-    /// Returns adjusted message state if the message has MDNs.
-    pub(crate) fn with_mdns(self, has_mdns: bool) -> Self {
+    /// Returns adjusted message state.
+    pub(crate) fn with(mut self, from_id: ContactId, has_mdns: bool) -> Self {
+        if MessageState::InFresh <= self
+            && self <= MessageState::InSeen
+            && from_id == ContactId::SELF
+        {
+            self = MessageState::OutDelivered;
+        }
         if self == MessageState::OutDelivered && has_mdns {
-            return MessageState::OutMdnRcvd;
+            self = MessageState::OutMdnRcvd;
         }
         self
     }

src/receive_imf.rs

@@ -1694,7 +1694,7 @@ async fn add_parts(
     };
 
     let state = if !mime_parser.incoming {
-        MessageState::OutDelivered
+        MessageState::InFresh
     } else if seen || is_mdn || chat_id_blocked == Blocked::Yes || group_changes.silent
     // No check for `hidden` because only reactions are such and they should be `InFresh`.
     {
@@ -1702,7 +1702,6 @@ async fn add_parts(
     } else {
         MessageState::InFresh
     };
-    let in_fresh = state == MessageState::InFresh;
 
     let sort_to_bottom = false;
     let received = true;
@@ -1999,7 +1998,7 @@ async fn add_parts(
         save_mime_modified |= mime_parser.is_mime_modified && !part_is_empty && !hidden;
         let save_mime_modified = save_mime_modified && parts.peek().is_none();
 
-        let ephemeral_timestamp = if in_fresh {
+        let ephemeral_timestamp = if state == MessageState::InFresh {
             0
         } else {
             match ephemeral_timer {
@@ -2111,6 +2110,8 @@ RETURNING id
         ensure_and_debug_assert!(!row_id.is_special(), "Rowid {row_id} is special");
         created_db_entries.push(row_id);
     }
+    let has_mdns = false;
+    let state = state.with(from_id, has_mdns);
 
     // Maybe set logging xdc and add gossip topics for webxdcs.
     for (part, msg_id) in mime_parser.parts.iter().zip(&created_db_entries) {

src/sql/migrations.rs

@@ -1271,6 +1271,18 @@ CREATE INDEX gossip_timestamp_index ON gossip_timestamp (chat_id, fingerprint);
         .await?;
     }
 
+    inc_and_check(&mut migration_version, 135)?;
+    if dbversion < migration_version {
+        // Tweak the index for `chat::calc_sort_timestamp()`.
+        sql.execute_migration(
+            "UPDATE msgs SET state=26 WHERE state=28;
+            DROP INDEX IF EXISTS msgs_index7;
+            CREATE INDEX msgs_index7 ON msgs (state, hidden, chat_id, timestamp);",
+            migration_version,
+        )
+        .await?;
+    }
+
     let new_version = sql
         .get_raw_config_int(VERSION_CFG)
         .await?

src/tests/verified_chats.rs

@@ -245,9 +245,9 @@ async fn test_old_message_4() -> Result<()> {
     Ok(())
 }
 
-/// Alice is offline for some time.
-/// When they come online, first their sentbox is synced and then their inbox.
-/// This test tests that the messages are still in the right order.
+/// Alice's device#0 is offline for some time.
+/// When it comes online, it sees a message from another device and an incoming message. Messages
+/// may come from different folders.
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_old_message_5() -> Result<()> {
     let alice = TestContext::new_alice().await;
@@ -277,7 +277,11 @@ async fn test_old_message_5() -> Result<()> {
     .await?
     .unwrap();
 
-    assert!(msg_sent.sort_timestamp == msg_incoming.sort_timestamp);
+    // If the messages come from the same folder and `msg_sent` is sent by Alice, it's better to
+    // sort `msg_incoming` after it so that it's more visible. Messages coming from different
+    // folders are a rare case now, but if Alice shares her account with someone else or has some
+    // auto-reply bot, messages should be sorted just by "Date".
+    assert!(msg_incoming.sort_timestamp < msg_sent.sort_timestamp);
     alice
         .golden_test_chat(msg_sent.chat_id, "test_old_message_5")
         .await;

test-data/golden/test_old_message_5

@@ -1,5 +1,5 @@
 Single#Chat#10: Bob [bob@example.net] Icon: 4138c52e5bc1c576cda7dd44d088c07.png
 --------------------------------------------------------------------------------
-Msg#10: Me (Contact#Contact#Self): Happy birthday, Bob!  √
 Msg#11:  (Contact#Contact#10): Happy birthday to me, Alice! [FRESH]
+Msg#10: Me (Contact#Contact#Self): Happy birthday, Bob!  √
 --------------------------------------------------------------------------------

test-data/golden/test_outgoing_encrypted_msg

@@ -1,5 +1,5 @@
 Single#Chat#10: bob@example.net [KEY bob@example.net]
 --------------------------------------------------------------------------------
-Msg#10: info (Contact#Contact#Info): Messages are end-to-end encrypted. [NOTICED][INFO]
 Msg#11🔒: Me (Contact#Contact#Self): Test – This is encrypted, signed, and has an Autocrypt Header without prefer-encrypt=mutual.  √
+Msg#10: info (Contact#Contact#Info): Messages are end-to-end encrypted. [NOTICED][INFO]
 --------------------------------------------------------------------------------