From e7696003e846b64c41761109f326fd37c4506040 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 15 Oct 2024 16:49:03 +0200 Subject: [PATCH] refactor(send_queue): Introduce QueueWedgeError, move logic out of ffi --- bindings/matrix-sdk-ffi/src/timeline/mod.rs | 71 ++----------------- .../src/deserialized_responses.rs | 49 ++++++++++++- .../src/timeline/controller/mod.rs | 12 ++-- .../src/timeline/event_item/local.rs | 7 +- .../matrix-sdk-ui/src/timeline/tests/echo.rs | 12 ++-- crates/matrix-sdk/src/send_queue.rs | 52 +++++++++++--- .../tests/integration/send_queue.rs | 4 +- 7 files changed, 111 insertions(+), 96 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/timeline/mod.rs b/bindings/matrix-sdk-ffi/src/timeline/mod.rs index 7d3117c3b8c..c7c926fa32b 100644 --- a/bindings/matrix-sdk-ffi/src/timeline/mod.rs +++ b/bindings/matrix-sdk-ffi/src/timeline/mod.rs @@ -80,6 +80,7 @@ use crate::{ mod content; pub use content::MessageContent; +use matrix_sdk::deserialized_responses::QueueWedgeError; #[derive(uniffi::Object)] #[repr(transparent)] @@ -908,42 +909,11 @@ 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>, - }, - - /// 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, - }, - - /// 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, @@ -962,46 +932,13 @@ 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) + Self::SendingFailed { is_recoverable: *is_recoverable, error: error.clone() } } 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)] diff --git a/crates/matrix-sdk-common/src/deserialized_responses.rs b/crates/matrix-sdk-common/src/deserialized_responses.rs index 6ef33dbfd20..37282158546 100644 --- a/crates/matrix-sdk-common/src/deserialized_responses.rs +++ b/crates/matrix-sdk-common/src/deserialized_responses.rs @@ -12,7 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::{collections::BTreeMap, fmt}; +use std::{ + collections::{BTreeMap, HashMap}, + fmt, +}; use ruma::{ events::{AnyMessageLikeEvent, AnySyncTimelineEvent, AnyTimelineEvent}, @@ -703,6 +706,50 @@ impl From for SyncTimelineEvent { } } +/// Represent a failed to send error of an event sent via the send_queue. +/// These errors are not automatically retrievable, but yet some manual action +/// can be taken before retry sending. +#[derive(Clone, Debug, Serialize, Deserialize)] +#[cfg_attr(feature = "uniffi", derive(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>, + }, + /// 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, + }, + /// It is required to set up cross-signing and properly verify the current + /// session before sending. + CrossVerificationRequired, + /// Other errors. + GenericApiError { msg: String }, +} + +/// Simple display implementation that strips out userIds/DeviceIds to avoid +/// accidental logging. +impl fmt::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), + } + } +} + #[cfg(test)] mod tests { use assert_matches::assert_matches; diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index ba23ab6e5bb..2478a4c86cd 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -29,6 +29,7 @@ use matrix_sdk::{ }, Result, Room, }; +use matrix_sdk_base::deserialized_responses::QueueWedgeError; use ruma::{ api::client::receipt::create_receipt::v3::ReceiptType as SendReceiptType, events::{ @@ -1262,9 +1263,9 @@ impl TimelineController

{ EventSendState::SendingFailed { // Put a dummy error in this case, since we're not persisting the errors // that occurred in previous sessions. - error: Arc::new(matrix_sdk::Error::UnknownError(Box::new( - MissingLocalEchoFailError, - ))), + error: QueueWedgeError::GenericApiError { + msg: MISSING_LOCAL_ECHO_FAIL_ERROR.into(), + }, is_recoverable: false, }, ) @@ -1521,9 +1522,8 @@ impl TimelineController { } } -#[derive(Debug, Error)] -#[error("local echo failed to send in a previous session")] -struct MissingLocalEchoFailError; +const MISSING_LOCAL_ECHO_FAIL_ERROR: &'static str = + "local echo failed to send in a previous session"; #[derive(Debug, Default)] pub(super) struct HandleManyEventsResult { diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/local.rs b/crates/matrix-sdk-ui/src/timeline/event_item/local.rs index 2890b6eab8e..f8de35f3adc 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/local.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_item/local.rs @@ -12,10 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::sync::Arc; - use as_variant::as_variant; -use matrix_sdk::{send_queue::SendHandle, Error}; +use matrix_sdk::send_queue::SendHandle; +use matrix_sdk_base::deserialized_responses::QueueWedgeError; use ruma::{EventId, OwnedEventId, OwnedTransactionId}; use super::TimelineEventItemId; @@ -70,7 +69,7 @@ pub enum EventSendState { /// sending has failed. SendingFailed { /// Details about how sending the event failed. - error: Arc, + error: QueueWedgeError, /// Whether the error is considered recoverable or not. /// /// An error that's recoverable will disable the room's send queue, diff --git a/crates/matrix-sdk-ui/src/timeline/tests/echo.rs b/crates/matrix-sdk-ui/src/timeline/tests/echo.rs index d8d8b56f6d8..8e268a3c72e 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/echo.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/echo.rs @@ -12,14 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::{io, sync::Arc}; - use assert_matches::assert_matches; use eyeball_im::VectorDiff; use matrix_sdk::{ assert_next_matches_with_timeout, send_queue::RoomSendQueueUpdate, - test_utils::events::EventFactory, Error, + test_utils::events::EventFactory, }; +use matrix_sdk_base::deserialized_responses::QueueWedgeError; use matrix_sdk_test::{async_test, ALICE, BOB}; use ruma::{ event_id, @@ -66,15 +65,12 @@ async fn test_remote_echo_full_trip() { // Scenario 2: The local event has not been sent to the server successfully, it // has failed. In this case, there is no event ID. { - let some_io_error = Error::Io(io::Error::new(io::ErrorKind::Other, "this is a test")); + let some_io_error = QueueWedgeError::GenericApiError { msg: "this is a test".to_owned() }; timeline .controller .update_event_send_state( &txn_id, - EventSendState::SendingFailed { - error: Arc::new(some_io_error), - is_recoverable: true, - }, + EventSendState::SendingFailed { error: some_io_error, is_recoverable: true }, ) .await; diff --git a/crates/matrix-sdk/src/send_queue.rs b/crates/matrix-sdk/src/send_queue.rs index 82753295ae5..2d2488022e3 100644 --- a/crates/matrix-sdk/src/send_queue.rs +++ b/crates/matrix-sdk/src/send_queue.rs @@ -58,18 +58,23 @@ use matrix_sdk_base::{ }, RoomState, StoreError, }; -use matrix_sdk_common::executor::{spawn, JoinHandle}; +use matrix_sdk_common::{ + deserialized_responses::QueueWedgeError, + executor::{spawn, JoinHandle}, +}; use ruma::{ events::{ reaction::ReactionEventContent, relation::Annotation, AnyMessageLikeEventContent, EventContent as _, }, serde::Raw, - EventId, OwnedEventId, OwnedRoomId, OwnedTransactionId, TransactionId, + EventId, OwnedEventId, OwnedRoomId, OwnedTransactionId, OwnedUserId, TransactionId, }; use tokio::sync::{broadcast, Notify, RwLock}; use tracing::{debug, error, info, instrument, trace, warn}; +#[cfg(feature = "e2e-encryption")] +use crate::crypto::{OlmError, SessionRecipientCollectionError}; use crate::{ client::WeakClient, config::RequestConfig, @@ -187,7 +192,7 @@ pub struct SendQueueRoomError { pub room_id: OwnedRoomId, /// The error the room has ran into, when trying to send an event. - pub error: Arc, + pub error: QueueWedgeError, /// Whether the error is considered recoverable or not. /// @@ -494,6 +499,8 @@ impl RoomSendQueue { _ => false, }; + let wedged_err = convert_error_to_wedged_reason(&err); + if is_recoverable { warn!(txn_id = %queued_event.transaction_id, error = ?err, "Recoverable error when sending event: {err}, disabling send queue"); @@ -520,17 +527,15 @@ impl RoomSendQueue { } } - let error = Arc::new(err); - let _ = global_error_reporter.send(SendQueueRoomError { room_id: room.room_id().to_owned(), - error: error.clone(), + error: wedged_err.clone(), is_recoverable, }); let _ = updates.send(RoomSendQueueUpdate::SendError { transaction_id: queued_event.transaction_id, - error, + error: wedged_err, is_recoverable, }); } @@ -577,6 +582,37 @@ impl RoomSendQueue { } } +fn convert_error_to_wedged_reason(value: &crate::Error) -> QueueWedgeError { + match value { + #[cfg(feature = "e2e-encryption")] + crate::Error::OlmError(OlmError::SessionRecipientCollectionError(error)) => match error { + SessionRecipientCollectionError::VerifiedUserHasUnsignedDevice(user_map) => { + QueueWedgeError::InsecureDevices { + user_device_map: user_map + .iter() + .map(|(user_id, devices)| { + ( + user_id.to_string(), + devices.iter().map(|device_id| device_id.to_string()).collect(), + ) + }) + .collect(), + } + } + SessionRecipientCollectionError::VerifiedUserChangedIdentity(users) => { + QueueWedgeError::IdentityViolations { + users: users.iter().map(OwnedUserId::to_string).collect(), + } + } + SessionRecipientCollectionError::CrossSigningNotSetup + | SessionRecipientCollectionError::SendingFromUnverifiedDevice => { + QueueWedgeError::CrossVerificationRequired + } + }, + _ => QueueWedgeError::GenericApiError { msg: value.to_string() }, + } +} + struct RoomSendQueueInner { /// The room which this send queue relates to. room: WeakRoom, @@ -1193,7 +1229,7 @@ pub enum RoomSendQueueUpdate { /// Transaction id used to identify this event. transaction_id: OwnedTransactionId, /// Error received while sending the event. - error: Arc, + error: QueueWedgeError, /// Whether the error is considered recoverable or not. /// /// An error that's recoverable will disable the room's send queue, diff --git a/crates/matrix-sdk/tests/integration/send_queue.rs b/crates/matrix-sdk/tests/integration/send_queue.rs index 8e6c6d1c29b..36e86e0d7d4 100644 --- a/crates/matrix-sdk/tests/integration/send_queue.rs +++ b/crates/matrix-sdk/tests/integration/send_queue.rs @@ -10,6 +10,7 @@ use std::{ use assert_matches2::{assert_let, assert_matches}; use matrix_sdk::{ config::{RequestConfig, StoreConfig}, + deserialized_responses::QueueWedgeError, send_queue::{LocalEcho, LocalEchoContent, RoomSendQueueError, RoomSendQueueUpdate}, test_utils::{ events::EventFactory, logged_in_client, logged_in_client_with_server, set_client_session, @@ -471,8 +472,7 @@ async fn test_error_then_locally_reenabling() { // seconds). // It's the same transaction id that's used to signal the send error. let error = assert_update!(watch => error { recoverable=true, txn=txn1 }); - let error = error.as_client_api_error().unwrap(); - assert_eq!(error.status_code, 500); + assert_matches!(error, QueueWedgeError::GenericApiError { .. }); sleep(Duration::from_millis(50)).await;