feat(sdk): Do not send a room subscription that has already been sent

crates/matrix-sdk-ui/tests/integration/room_list_service.rs

@@ -2195,16 +2195,6 @@ async fn test_room_subscription() -> Result<(), Error> {
                 },
             },
             "room_subscriptions": {
-                room_id_1: {
-                    "required_state": [
-                        ["m.room.name", ""],
-                        ["m.room.topic", ""],
-                        ["m.room.avatar", ""],
-                        ["m.room.canonical_alias", ""],
-                        ["m.room.create", ""],
-                    ],
-                    "timeline_limit": 30,
-                },
                 room_id_2: {
                     "required_state": [
                         ["m.room.name", ""],

crates/matrix-sdk/src/sliding_sync/mod.rs

@@ -158,7 +158,10 @@ impl SlidingSync {
                 room.mark_members_missing();
             }
 
-            room_subscriptions.insert((*room_id).to_owned(), settings.clone());
+            room_subscriptions.insert(
+                (*room_id).to_owned(),
+                (RoomSubscriptionState::default(), settings.clone()),
+            );
         }
 
         self.inner.internal_channel_send_if_possible(
@@ -893,13 +896,30 @@ pub struct UpdateSummary {
     pub rooms: Vec<OwnedRoomId>,
 }
 
+/// A very basic bool-ish enum to represent the state of a
+/// [`http::request::RoomSubscription`]. A `RoomSubscription` that has been sent
+/// once should ideally not being sent again, to mostly save bandwidth.
+#[derive(Debug, Default)]
+enum RoomSubscriptionState {
+    /// The `RoomSubscription` has not been sent or received correctly from the
+    /// server, i.e. the `RoomSubscription` —which is part of the sticky
+    /// parameters— has not been committed.
+    #[default]
+    Pending,
+
+    /// The `RoomSubscription` has been sent and received correctly by the
+    /// server.
+    Applied,
+}
+
 /// The set of sticky parameters owned by the `SlidingSyncInner` instance, and
 /// sent in the request.
 #[derive(Debug)]
 pub(super) struct SlidingSyncStickyParameters {
     /// Room subscriptions, i.e. rooms that may be out-of-scope of all lists
     /// but one wants to receive updates.
-    room_subscriptions: BTreeMap<OwnedRoomId, http::request::RoomSubscription>,
+    room_subscriptions:
+        BTreeMap<OwnedRoomId, (RoomSubscriptionState, http::request::RoomSubscription)>,
 
     /// The intended state of the extensions being supplied to sliding /sync
     /// calls.
@@ -912,17 +932,41 @@ impl SlidingSyncStickyParameters {
         room_subscriptions: BTreeMap<OwnedRoomId, http::request::RoomSubscription>,
         extensions: http::request::Extensions,
     ) -> Self {
-        Self { room_subscriptions, extensions }
+        Self {
+            room_subscriptions: room_subscriptions
+                .into_iter()
+                .map(|(room_id, room_subscription)| {
+                    (room_id, (RoomSubscriptionState::Pending, room_subscription))
+                })
+                .collect(),
+            extensions,
+        }
     }
 }
 
 impl StickyData for SlidingSyncStickyParameters {
     type Request = http::Request;
 
     fn apply(&self, request: &mut Self::Request) {
-        request.room_subscriptions = self.room_subscriptions.clone();
+        request.room_subscriptions = self
+            .room_subscriptions
+            .iter()
+            .filter_map(|(room_id, (state, room_subscription))| {
+                matches!(state, RoomSubscriptionState::Pending)
+                    .then(|| (room_id.clone(), room_subscription.clone()))
+            })
+            .collect();
         request.extensions = self.extensions.clone();
     }
+
+    fn on_commit(&mut self) {
+        // All room subscriptions are marked as `Applied`.
+        for (state, _room_subscription) in self.room_subscriptions.values_mut() {
+            if matches!(state, RoomSubscriptionState::Pending) {
+                *state = RoomSubscriptionState::Applied;
+            }
+        }
+    }
 }
 
 /// As of 2023-07-13, the sliding sync proxy doesn't provide us with `limited`
@@ -1193,7 +1237,8 @@ mod tests {
 
     #[test]
     fn test_sticky_parameters_api_invalidated_flow() {
-        let r0 = room_id!("!room:example.org");
+        let r0 = room_id!("!r0.matrix.org");
+        let r1 = room_id!("!r1:matrix.org");
 
         let mut room_subscriptions = BTreeMap::new();
         room_subscriptions.insert(r0.to_owned(), Default::default());
@@ -1226,7 +1271,7 @@ mod tests {
         sticky
             .data_mut()
             .room_subscriptions
-            .insert(room_id!("!r1:bar.org").to_owned(), Default::default());
+            .insert(r1.to_owned(), (Default::default(), Default::default()));
         assert!(sticky.is_invalidated());
 
         // Committing with the wrong transaction id will keep it invalidated.
@@ -1240,15 +1285,22 @@ mod tests {
         sticky.maybe_apply(&mut request1, &mut LazyTransactionId::from_owned(txn_id1.to_owned()));
 
         assert!(sticky.is_invalidated());
-        assert_eq!(request1.room_subscriptions.len(), 2);
+        // The first room subscription has been applied to `request`, so it's not
+        // reapplied here. It's a particular logic of `room_subscriptions`, it's not
+        // related to the sticky design.
+        assert_eq!(request1.room_subscriptions.len(), 1);
+        assert!(request1.room_subscriptions.contains_key(r1));
 
         let txn_id2: &TransactionId = "tid789".into();
         let mut request2 = http::Request::default();
         request2.txn_id = Some(txn_id2.to_string());
 
         sticky.maybe_apply(&mut request2, &mut LazyTransactionId::from_owned(txn_id2.to_owned()));
         assert!(sticky.is_invalidated());
-        assert_eq!(request2.room_subscriptions.len(), 2);
+        // `request2` contains `r1` because the sticky parameters have not been
+        // committed, so it's still marked as pending.
+        assert_eq!(request2.room_subscriptions.len(), 1);
+        assert!(request2.room_subscriptions.contains_key(r1));
 
         // Here we commit with the not most-recent TID, so it keeps the invalidated
         // status.
@@ -1260,6 +1312,101 @@ mod tests {
         assert!(!sticky.is_invalidated());
     }
 
+    #[test]
+    fn test_room_subscriptions_are_sticky() {
+        let r0 = room_id!("!r0.matrix.org");
+        let r1 = room_id!("!r1:matrix.org");
+
+        let mut sticky = SlidingSyncStickyManager::new(SlidingSyncStickyParameters::new(
+            BTreeMap::new(),
+            Default::default(),
+        ));
+
+        // A room subscription is added, applied, and committed.
+        {
+            // Insert `r0`.
+            sticky
+                .data_mut()
+                .room_subscriptions
+                .insert(r0.to_owned(), (Default::default(), Default::default()));
+
+            // Then the sticky parameters are applied.
+            let txn_id: &TransactionId = "tid0".into();
+            let mut request = http::Request::default();
+            request.txn_id = Some(txn_id.to_string());
+
+            sticky.maybe_apply(&mut request, &mut LazyTransactionId::from_owned(txn_id.to_owned()));
+
+            assert!(request.txn_id.is_some());
+            assert_eq!(request.room_subscriptions.len(), 1);
+            assert!(request.room_subscriptions.contains_key(r0));
+
+            // Then the sticky parameters are committed.
+            let tid = request.txn_id.unwrap();
+
+            sticky.maybe_commit(tid.as_str().into());
+        }
+
+        // A room subscription is added, applied, but NOT committed.
+        {
+            // Insert `r1`.
+            sticky
+                .data_mut()
+                .room_subscriptions
+                .insert(r1.to_owned(), (Default::default(), Default::default()));
+
+            // Then the sticky parameters are applied.
+            let txn_id: &TransactionId = "tid1".into();
+            let mut request = http::Request::default();
+            request.txn_id = Some(txn_id.to_string());
+
+            sticky.maybe_apply(&mut request, &mut LazyTransactionId::from_owned(txn_id.to_owned()));
+
+            assert!(request.txn_id.is_some());
+            assert_eq!(request.room_subscriptions.len(), 1);
+            // `r0` is not present, it's only `r1`.
+            assert!(request.room_subscriptions.contains_key(r1));
+
+            // Then the sticky parameters are NOT committed.
+            // It can happen if the request has failed to be sent for example,
+            // or if the response didn't match.
+        }
+
+        // A previously added room subscription is re-added, applied, and committed.
+        {
+            // Then the sticky parameters are applied.
+            let txn_id: &TransactionId = "tid2".into();
+            let mut request = http::Request::default();
+            request.txn_id = Some(txn_id.to_string());
+
+            sticky.maybe_apply(&mut request, &mut LazyTransactionId::from_owned(txn_id.to_owned()));
+
+            assert!(request.txn_id.is_some());
+            assert_eq!(request.room_subscriptions.len(), 1);
+            // `r0` is not present, it's only `r1`.
+            assert!(request.room_subscriptions.contains_key(r1));
+
+            // Then the sticky parameters are committed.
+            let tid = request.txn_id.unwrap();
+
+            sticky.maybe_commit(tid.as_str().into());
+        }
+
+        // All room subscriptions have been committed.
+        {
+            // Then the sticky parameters are applied.
+            let txn_id: &TransactionId = "tid3".into();
+            let mut request = http::Request::default();
+            request.txn_id = Some(txn_id.to_string());
+
+            sticky.maybe_apply(&mut request, &mut LazyTransactionId::from_owned(txn_id.to_owned()));
+
+            assert!(request.txn_id.is_some());
+            // All room subscriptions have been sent.
+            assert!(request.room_subscriptions.is_empty());
+        }
+    }
+
     #[test]
     fn test_extensions_are_sticky() {
         let mut extensions = http::request::Extensions::default();

crates/matrix-sdk/src/sliding_sync/sticky_parameters.rs

@@ -49,6 +49,12 @@ pub trait StickyData {
 
     /// Apply the current data onto the request.
     fn apply(&self, request: &mut Self::Request);
+
+    /// When the current are committed, i.e. when the request has been validated
+    /// by a response.
+    fn on_commit(&mut self) {
+        // noop
+    }
 }
 
 /// Helper data structure to manage sticky parameters, for any kind of data.
@@ -122,6 +128,7 @@ impl<D: StickyData> SlidingSyncStickyManager<D> {
     pub fn maybe_commit(&mut self, txn_id: &TransactionId) {
         if self.invalidated && self.txn_id.as_deref() == Some(txn_id) {
             self.invalidated = false;
+            self.data.on_commit();
         }
     }
 
@@ -135,7 +142,7 @@ impl<D: StickyData> SlidingSyncStickyManager<D> {
 mod tests {
     use super::{LazyTransactionId, SlidingSyncStickyManager, StickyData};
 
-    struct EmptyStickyData;
+    struct EmptyStickyData(u8);
 
     impl StickyData for EmptyStickyData {
         type Request = bool;
@@ -144,11 +151,15 @@ mod tests {
             // Mark that applied has had an effect.
             *req = true;
         }
+
+        fn on_commit(&mut self) {
+            self.0 += 1;
+        }
     }
 
     #[test]
     fn test_sticky_parameters_api_non_invalidated_no_effect() {
-        let mut sticky = SlidingSyncStickyManager::new(EmptyStickyData);
+        let mut sticky = SlidingSyncStickyManager::new(EmptyStickyData(0));
 
         // At first, it's always invalidated.
         assert!(sticky.is_invalidated());
@@ -162,10 +173,12 @@ mod tests {
 
         // Committing with the wrong transaction id won't commit.
         sticky.maybe_commit("tid456".into());
+        assert_eq!(sticky.data.0, 0);
         assert!(sticky.is_invalidated());
 
         // Providing the correct transaction id will commit.
         sticky.maybe_commit(txn_id.get().unwrap());
+        assert_eq!(sticky.data.0, 1);
         assert!(!sticky.is_invalidated());
 
         // Applying without being invalidated won't do anything, and not generate a