feat(send_queue): Persist failed to send errors (#4137)

Modify the SendQueue in order to persist the error that cause the event
to fail to send as a `QueueWedgeError`. The `QueueWedgeError` is not a
1:1 mapping for all kinds of errors, but holds variant and information
that the client can react to in order to propose "quick fixes"/solution
before retrying to send.

Fixes #3973 
Also fixes element-hq/element-x-ios#3287
because when a timeline reset occurs the fail to send reason is also
lost.

This PR starts with a refactoring commit
e769600
to introduce the new `QueueWedgedError` and move the logic that was in
the ffi layer to convert api errors to SendState error variant. This
`QueueWedgedError` can be directly use in the `SendingFailed` variant
and expose to ffi.

Second commit
109c133
adds the persistence, `QueuedEvent` now have an optional error field
instead of a `is_weged` boolean. Same for LocalEchoContent::Event.
Adds also Migration for sqlite and indexeddb

Co-authored-by: Benjamin Bouvier <benjamin@bouvier.cc>

Changelog:  We now persist the error that caused an event to fail to send. The error `QueueWedgeError` contains info that client can use to try to resolve the problem when the error is not automatically retry-able. Some breaking changes occurred in the FFI layer for `timeline::EventSendState`, `SendingFailed` now directly contains the wedge reason enum; use it in place of the removed variant of `EventSendState`.

Author: BillCarsonFr

bindings/matrix-sdk-ffi/src/error.rs

@@ -1,13 +1,13 @@
-use std::fmt::Display;
+use std::{collections::HashMap, fmt, fmt::Display};
 
 use matrix_sdk::{
     encryption::CryptoStoreError, event_cache::EventCacheError, oidc::OidcError, reqwest,
     room::edit::EditError, send_queue::RoomSendQueueError, HttpError, IdParseError,
-    NotificationSettingsError as SdkNotificationSettingsError, StoreError,
+    NotificationSettingsError as SdkNotificationSettingsError,
+    QueueWedgeError as SdkQueueWedgeError, StoreError,
 };
 use matrix_sdk_ui::{encryption_sync_service, notification_client, sync_service, timeline};
 use uniffi::UnexpectedUniFFICallbackError;
-
 #[derive(Debug, thiserror::Error)]
 pub enum ClientError {
     #[error("client error: {msg}")]
@@ -146,6 +146,79 @@ impl From<RoomSendQueueError> for ClientError {
     }
 }
 
+/// Bindings version of the sdk type replacing OwnedUserId/DeviceIds with simple
+/// String.
+///
+/// Represent a failed to send unrecoverable error of an event sent via the
+/// send_queue. It is a serializable representation of a client error, see
+/// `From` implementation for more details. These errors can not be
+/// automatically retried, but yet some manual action can be taken before retry
+/// sending. If not the only solution is to delete the local event.
+#[derive(Debug, Clone, uniffi::Enum)]
+pub enum QueueWedgeError {
+    /// This error occurs when there are some insecure devices in the room, and
+    /// the current encryption setting prohibit sharing with them.
+    InsecureDevices {
+        /// The insecure devices as a Map of userID to deviceID.
+        user_device_map: HashMap<String, Vec<String>>,
+    },
+
+    /// This error occurs when a previously verified user is not anymore, and
+    /// the current encryption setting prohibit sharing when it happens.
+    IdentityViolations {
+        /// The users that are expected to be verified but are not.
+        users: Vec<String>,
+    },
+
+    /// It is required to set up cross-signing and properly erify the current
+    /// session before sending.
+    CrossVerificationRequired,
+
+    /// Other errors.
+    GenericApiError { msg: String },
+}
+
+/// Simple display implementation that strips out userIds/DeviceIds to avoid
+/// accidental logging.
+impl Display for QueueWedgeError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            QueueWedgeError::InsecureDevices { .. } => {
+                f.write_str("There are insecure devices in the room")
+            }
+            QueueWedgeError::IdentityViolations { .. } => {
+                f.write_str("Some users that were previously verified are not anymore")
+            }
+            QueueWedgeError::CrossVerificationRequired => {
+                f.write_str("Own verification is required")
+            }
+            QueueWedgeError::GenericApiError { msg } => f.write_str(msg),
+        }
+    }
+}
+impl From<SdkQueueWedgeError> for QueueWedgeError {
+    fn from(value: SdkQueueWedgeError) -> Self {
+        match value {
+            SdkQueueWedgeError::InsecureDevices { user_device_map } => Self::InsecureDevices {
+                user_device_map: user_device_map
+                    .iter()
+                    .map(|(user_id, devices)| {
+                        (
+                            user_id.to_string(),
+                            devices.iter().map(|device_id| device_id.to_string()).collect(),
+                        )
+                    })
+                    .collect(),
+            },
+            SdkQueueWedgeError::IdentityViolations { users } => Self::IdentityViolations {
+                users: users.iter().map(ruma::OwnedUserId::to_string).collect(),
+            },
+            SdkQueueWedgeError::CrossVerificationRequired => Self::CrossVerificationRequired,
+            SdkQueueWedgeError::GenericApiError { msg } => Self::GenericApiError { msg },
+        }
+    }
+}
+
 #[derive(Debug, thiserror::Error, uniffi::Error)]
 #[uniffi(flat_error)]
 pub enum RoomError {

bindings/matrix-sdk-ffi/src/timeline/mod.rs

@@ -82,6 +82,8 @@ mod content;
 
 pub use content::MessageContent;
 
+use crate::error::QueueWedgeError;
+
 #[derive(uniffi::Object)]
 #[repr(transparent)]
 pub struct Timeline {
@@ -951,49 +953,20 @@ pub enum EventSendState {
     /// The local event has not been sent yet.
     NotSentYet,
 
-    /// One or more verified users in the room has an unsigned device.
-    ///
-    /// Happens only when the room key recipient strategy (as set by
-    /// [`ClientBuilder::room_key_recipient_strategy`]) has
-    /// [`error_on_verified_user_problem`](CollectStrategy::DeviceBasedStrategy::error_on_verified_user_problem) set.
-    VerifiedUserHasUnsignedDevice {
-        /// The unsigned devices belonging to verified users. A map from user ID
-        /// to a list of device IDs.
-        devices: HashMap<String, Vec<String>>,
-    },
-
-    /// One or more verified users in the room has changed identity since they
-    /// were verified.
-    ///
-    /// Happens only when the room key recipient strategy (as set by
-    /// [`ClientBuilder::room_key_recipient_strategy`]) has
-    /// [`error_on_verified_user_problem`](CollectStrategy::DeviceBasedStrategy::error_on_verified_user_problem)
-    /// set, or when using [`CollectStrategy::IdentityBasedStrategy`].
-    VerifiedUserChangedIdentity {
-        /// The users that were previously verified, but are no longer
-        users: Vec<String>,
-    },
-
-    /// The user does not have cross-signing set up, but
-    /// [`CollectStrategy::IdentityBasedStrategy`] was used.
-    CrossSigningNotSetup,
-
-    /// The current device is not verified, but
-    /// [`CollectStrategy::IdentityBasedStrategy`] was used.
-    SendingFromUnverifiedDevice,
-
     /// The local event has been sent to the server, but unsuccessfully: The
     /// sending has failed.
     SendingFailed {
-        /// Stringified error message.
-        error: String,
+        /// The error reason, with information for the user.
+        error: QueueWedgeError,
+
         /// Whether the error is considered recoverable or not.
         ///
         /// An error that's recoverable will disable the room's send queue,
         /// while an unrecoverable error will be parked, until the user
         /// decides to cancel sending it.
         is_recoverable: bool,
     },
+
     /// The local event has been sent successfully to the server.
     Sent { event_id: String },
 }
@@ -1005,46 +978,17 @@ impl From<&matrix_sdk_ui::timeline::EventSendState> for EventSendState {
         match value {
             NotSentYet => Self::NotSentYet,
             SendingFailed { error, is_recoverable } => {
-                event_send_state_from_sending_failed(error, *is_recoverable)
+                let as_queue_wedge_error: matrix_sdk::QueueWedgeError = (&**error).into();
+                Self::SendingFailed {
+                    is_recoverable: *is_recoverable,
+                    error: as_queue_wedge_error.into(),
+                }
             }
             Sent { event_id } => Self::Sent { event_id: event_id.to_string() },
         }
     }
 }
 
-fn event_send_state_from_sending_failed(error: &Error, is_recoverable: bool) -> EventSendState {
-    use matrix_sdk::crypto::{OlmError, SessionRecipientCollectionError::*};
-
-    match error {
-        // Special-case the SessionRecipientCollectionErrors, to pass the information they contain
-        // back to the application.
-        Error::OlmError(OlmError::SessionRecipientCollectionError(error)) => match error {
-            VerifiedUserHasUnsignedDevice(devices) => {
-                let devices = devices
-                    .iter()
-                    .map(|(user_id, devices)| {
-                        (
-                            user_id.to_string(),
-                            devices.iter().map(|device_id| device_id.to_string()).collect(),
-                        )
-                    })
-                    .collect();
-                EventSendState::VerifiedUserHasUnsignedDevice { devices }
-            }
-
-            VerifiedUserChangedIdentity(bad_users) => EventSendState::VerifiedUserChangedIdentity {
-                users: bad_users.iter().map(|user_id| user_id.to_string()).collect(),
-            },
-
-            CrossSigningNotSetup => EventSendState::CrossSigningNotSetup,
-
-            SendingFromUnverifiedDevice => EventSendState::SendingFromUnverifiedDevice,
-        },
-
-        _ => EventSendState::SendingFailed { error: error.to_string(), is_recoverable },
-    }
-}
-
 /// Recommended decorations for decrypted messages, representing the message's
 /// authenticity properties.
 #[derive(uniffi::Enum, Clone)]

crates/matrix-sdk-base/src/lib.rs

@@ -61,7 +61,7 @@ pub use rooms::{
     RoomStateFilter,
 };
 pub use store::{
-    ComposerDraft, ComposerDraftType, StateChanges, StateStore, StateStoreDataKey,
+    ComposerDraft, ComposerDraftType, QueueWedgeError, StateChanges, StateStore, StateStoreDataKey,
     StateStoreDataValue, StoreError,
 };
 pub use utils::{

crates/matrix-sdk-base/src/store/integration_tests.rs

@@ -36,7 +36,10 @@ use serde_json::{json, value::Value as JsonValue};
 use super::{DependentQueuedEventKind, DynStateStore, ServerCapabilities};
 use crate::{
     deserialized_responses::MemberEvent,
-    store::{traits::ChildTransactionId, Result, SerializableEventContent, StateStoreExt},
+    store::{
+        traits::ChildTransactionId, QueueWedgeError, Result, SerializableEventContent,
+        StateStoreExt,
+    },
     RoomInfo, RoomMemberships, RoomState, StateChanges, StateStoreDataKey, StateStoreDataValue,
 };
 
@@ -1223,7 +1226,7 @@ impl StateStoreIntegrationTests for DynStateStore {
             assert_let!(AnyMessageLikeEventContent::RoomMessage(content) = deserialized);
             assert_eq!(content.body(), "msg0");
 
-            assert!(!pending[0].is_wedged);
+            assert!(!pending[0].is_wedged());
         }
 
         // Saving another three things should work.
@@ -1249,12 +1252,18 @@ impl StateStoreIntegrationTests for DynStateStore {
             let deserialized = pending[i].event.deserialize().unwrap();
             assert_let!(AnyMessageLikeEventContent::RoomMessage(content) = deserialized);
             assert_eq!(content.body(), format!("msg{i}"));
-            assert!(!pending[i].is_wedged);
+            assert!(!pending[i].is_wedged());
         }
 
         // Marking an event as wedged works.
         let txn2 = &pending[2].transaction_id;
-        self.update_send_queue_event_status(room_id, txn2, true).await.unwrap();
+        self.update_send_queue_event_status(
+            room_id,
+            txn2,
+            Some(QueueWedgeError::GenericApiError { msg: "Oops".to_owned() }),
+        )
+        .await
+        .unwrap();
 
         // And it is reflected.
         let pending = self.load_send_queue_events(room_id).await.unwrap();
@@ -1263,10 +1272,13 @@ impl StateStoreIntegrationTests for DynStateStore {
         assert_eq!(pending.len(), 4);
         assert_eq!(pending[0].transaction_id, txn0);
         assert_eq!(pending[2].transaction_id, *txn2);
-        assert!(pending[2].is_wedged);
+        assert!(pending[2].is_wedged());
+        let error = pending[2].clone().error.unwrap();
+        let generic_error = assert_matches!(error, QueueWedgeError::GenericApiError { msg } => msg);
+        assert_eq!(generic_error, "Oops");
         for i in 0..4 {
             if i != 2 {
-                assert!(!pending[i].is_wedged);
+                assert!(!pending[i].is_wedged());
             }
         }
 
@@ -1288,15 +1300,15 @@ impl StateStoreIntegrationTests for DynStateStore {
             assert_let!(AnyMessageLikeEventContent::RoomMessage(content) = deserialized);
             assert_eq!(content.body(), "wow that's a cool test");
 
-            assert!(!pending[2].is_wedged);
+            assert!(!pending[2].is_wedged());
 
             for i in 0..4 {
                 if i != 2 {
                     let deserialized = pending[i].event.deserialize().unwrap();
                     assert_let!(AnyMessageLikeEventContent::RoomMessage(content) = deserialized);
                     assert_eq!(content.body(), format!("msg{i}"));
 
-                    assert!(!pending[i].is_wedged);
+                    assert!(!pending[i].is_wedged());
                 }
             }
         }

crates/matrix-sdk-base/src/store/memory_store.rs

@@ -44,8 +44,8 @@ use super::{
     StoreError,
 };
 use crate::{
-    deserialized_responses::RawAnySyncOrStrippedState, MinimalRoomMemberEvent, RoomMemberships,
-    StateStoreDataKey, StateStoreDataValue,
+    deserialized_responses::RawAnySyncOrStrippedState, store::QueueWedgeError,
+    MinimalRoomMemberEvent, RoomMemberships, StateStoreDataKey, StateStoreDataValue,
 };
 
 /// In-memory, non-persistent implementation of the `StateStore`.
@@ -815,7 +815,7 @@ impl StateStore for MemoryStore {
             .unwrap()
             .entry(room_id.to_owned())
             .or_default()
-            .push(QueuedEvent { event, transaction_id, is_wedged: false });
+            .push(QueuedEvent { event, transaction_id, error: None });
         Ok(())
     }
 
@@ -835,7 +835,7 @@ impl StateStore for MemoryStore {
             .find(|item| item.transaction_id == transaction_id)
         {
             entry.event = content;
-            entry.is_wedged = false;
+            entry.error = None;
             Ok(true)
         } else {
             Ok(false)
@@ -876,7 +876,7 @@ impl StateStore for MemoryStore {
         &self,
         room_id: &RoomId,
         transaction_id: &TransactionId,
-        wedged: bool,
+        error: Option<QueueWedgeError>,
     ) -> Result<(), Self::Error> {
         if let Some(entry) = self
             .send_queue_events
@@ -887,7 +887,7 @@ impl StateStore for MemoryStore {
             .iter_mut()
             .find(|item| item.transaction_id == transaction_id)
         {
-            entry.is_wedged = wedged;
+            entry.error = error;
         }
         Ok(())
     }

crates/matrix-sdk-base/src/store/mod.rs

@@ -75,7 +75,7 @@ pub use self::{
     memory_store::MemoryStore,
     traits::{
         ChildTransactionId, ComposerDraft, ComposerDraftType, DependentQueuedEvent,
-        DependentQueuedEventKind, DynStateStore, IntoStateStore, QueuedEvent,
+        DependentQueuedEventKind, DynStateStore, IntoStateStore, QueueWedgeError, QueuedEvent,
         SerializableEventContent, ServerCapabilities, StateStore, StateStoreDataKey,
         StateStoreDataValue, StateStoreExt,
     },