feat(crypto) Provide a method to check whether server backup exists without hitting the server every time

crates/matrix-sdk/CHANGELOG.md

@@ -17,6 +17,7 @@ All notable changes to this project will be documented in this file.
 - Do not use the encrypted original file's content type as the encrypted
   thumbnail's content type.
   ([#ecf4434](https://github.com/matrix-org/matrix-rust-sdk/commit/ecf44348cf6a872b843fb7d7af1a88f724c58c3e))
+
 ### Features
 
 - Enable persistent storage for the `EventCache`. This allows events received
@@ -28,6 +29,11 @@ All notable changes to this project will be documented in this file.
 - [**breaking**] Make all fields of Thumbnail required
   ([#4324](https://github.com/matrix-org/matrix-rust-sdk/pull/4324))
 
+- `Backups::exists_on_server`, which always fetches up-to-date information from the
+  server about whether a key storage backup exists, was renamed to
+  `fetch_exists_on_the_server`, and a new implementation of `exists_on_server`
+  which caches the most recent answer is now provided.
+
 ## [0.8.0] - 2024-11-19
 
 ### Bug Fixes

crates/matrix-sdk/src/encryption/backups/mod.rs

@@ -90,6 +90,7 @@ impl Backups {
     /// # anyhow::Ok(()) };
     /// ```
     pub async fn create(&self) -> Result<(), Error> {
+        self.client.inner.e2ee.backup_state.clear_backup_exists_on_server();
         let _guard = self.client.locks().backup_modify_lock.lock().await;
 
         self.set_state(BackupState::Creating);
@@ -387,7 +388,30 @@ impl Backups {
     /// This method will request info about the current backup from the
     /// homeserver and if a backup exits return `true`, otherwise `false`.
     pub async fn exists_on_server(&self) -> Result<bool, Error> {
-        Ok(self.get_current_version().await?.is_some())
+        let exists_on_server = self.get_current_version().await?.is_some();
+        self.client.inner.e2ee.backup_state.set_backup_exists_on_server(exists_on_server);
+        Ok(exists_on_server)
+    }
+
+    /// Does a backup exist on the server?
+    ///
+    /// This method is identical to [`Self::exists_on_server`] except that we
+    /// cache the latest answer in memory and only empty the cache if the local
+    /// device adds or deletes a backup itself.
+    ///
+    /// Do not use this method if you need an accurate answer about whether a
+    /// backup exists - instead use [`Self::exists_on_server`]. This method is
+    /// useful when performance is more important than guaranteed accuracy,
+    /// such as when classifying UTDs.
+    pub async fn fast_exists_on_server(&self) -> Result<bool, Error> {
+        // If we have an answer cached, return it immediately
+        if let Some(cached_value) = self.client.inner.e2ee.backup_state.backup_exists_on_server() {
+            return Ok(cached_value);
+        }
+
+        // Otherwise, delegate to exists_on_server. (It will update the cached value for
+        // us.)
+        self.exists_on_server().await
     }
 
     /// Subscribe to a stream that notifies when a room key for the specified
@@ -621,7 +645,7 @@ impl Backups {
     async fn delete_backup_from_server(&self, version: String) -> Result<(), Error> {
         let request = ruma::api::client::backup::delete_backup_version::v3::Request::new(version);
 
-        match self.client.send(request, Default::default()).await {
+        let ret = match self.client.send(request, Default::default()).await {
             Ok(_) => Ok(()),
             Err(e) => {
                 if let Some(kind) = e.client_api_error_kind() {
@@ -634,7 +658,11 @@ impl Backups {
                     Err(e.into())
                 }
             }
-        }
+        };
+
+        self.client.inner.e2ee.backup_state.clear_backup_exists_on_server();
+
+        ret
     }
 
     #[instrument(skip(self, olm_machine, request))]
@@ -1135,6 +1163,23 @@ mod test {
         assert!(exists, "We should deduce that a backup exists on the server");
     }
 
+    #[async_test]
+    async fn test_repeated_calls_to_exists_on_server_makes_repeated_requests() {
+        let server = MatrixMockServer::new().await;
+        let client = server.client_builder().build().await;
+
+        // Expect 2 requests to the server
+        server.mock_room_keys_version().exists().expect(2).mount().await;
+
+        let backups = client.encryption().backups();
+
+        // Call exists_on_server twice
+        backups.exists_on_server().await.unwrap();
+        let exists = backups.exists_on_server().await.unwrap();
+
+        assert!(exists, "We should deduce that a backup exists on the server");
+    }
+
     #[async_test]
     async fn test_when_no_backup_exists_then_exists_on_server_returns_false() {
         let server = MatrixMockServer::new().await;
@@ -1177,20 +1222,148 @@ mod test {
     }
 
     #[async_test]
-    async fn test_waiting_for_steady_state_resets_the_delay() {
-        let server = MockServer::start().await;
-        let client = logged_in_client(Some(server.uri())).await;
+    async fn test_when_a_backup_exists_then_fast_exists_on_server_returns_true() {
+        let server = MatrixMockServer::new().await;
+        let client = server.client_builder().build().await;
 
-        Mock::given(method("POST"))
-            .and(path("_matrix/client/unstable/room_keys/version"))
-            .and(header("authorization", "Bearer 1234"))
-            .respond_with(ResponseTemplate::new(200).set_body_json(json!({
-              "version": "1"
-            })))
-            .expect(1)
-            .named("POST for the backup creation")
-            .mount(&server)
-            .await;
+        server.mock_room_keys_version().exists().expect(1).mount().await;
+
+        let exists = client
+            .encryption()
+            .backups()
+            .fast_exists_on_server()
+            .await
+            .expect("We should be able to check if backups exist on the server");
+
+        assert!(exists, "We should deduce that a backup exists on the server");
+    }
+
+    #[async_test]
+    async fn test_when_no_backup_exists_then_fast_exists_on_server_returns_false() {
+        let server = MatrixMockServer::new().await;
+        let client = server.client_builder().build().await;
+
+        server.mock_room_keys_version().none().expect(1).mount().await;
+
+        let exists = client
+            .encryption()
+            .backups()
+            .fast_exists_on_server()
+            .await
+            .expect("We should be able to check if backups exist on the server");
+
+        assert!(!exists, "We should deduce that no backup exists on the server");
+    }
+
+    #[async_test]
+    async fn test_when_server_returns_an_error_then_fast_exists_on_server_returns_an_error() {
+        let server = MatrixMockServer::new().await;
+        let client = server.client_builder().build().await;
+
+        {
+            let _scope =
+                server.mock_room_keys_version().error429().expect(1).mount_as_scoped().await;
+
+            client.encryption().backups().fast_exists_on_server().await.expect_err(
+                "If the /version endpoint returns a non 404 error we should throw an error",
+            );
+        }
+
+        {
+            let _scope =
+                server.mock_room_keys_version().error404().expect(1).mount_as_scoped().await;
+
+            client.encryption().backups().fast_exists_on_server().await.expect_err(
+                "If the /version endpoint returns a non-Matrix 404 error we should throw an error",
+            );
+        }
+    }
+
+    #[async_test]
+    async fn test_repeated_calls_to_fast_exists_on_server_do_not_make_additional_requests() {
+        let server = MatrixMockServer::new().await;
+        let client = server.client_builder().build().await;
+
+        // Create a mock stating that the request should only be made once
+        server.mock_room_keys_version().exists().expect(1).mount().await;
+
+        let backups = client.encryption().backups();
+
+        // Call fast_exists_on_server several times
+        backups.fast_exists_on_server().await.unwrap();
+        backups.fast_exists_on_server().await.unwrap();
+        backups.fast_exists_on_server().await.unwrap();
+
+        let exists = backups
+            .fast_exists_on_server()
+            .await
+            .expect("We should be able to check if backups exist on the server");
+
+        assert!(exists, "We should deduce that a backup exists on the server");
+
+        // We check expectations here, confirming that only one call was made
+    }
+
+    #[async_test]
+    async fn test_adding_a_backup_invalidates_fast_exists_on_server_cache() {
+        let server = MatrixMockServer::new().await;
+        let client = server.client_builder().build().await;
+        let backups = client.encryption().backups();
+
+        {
+            let _scope = server.mock_room_keys_version().none().expect(1).mount_as_scoped().await;
+
+            // Call fast_exists_on_server to fill the cache
+            let exists = backups.fast_exists_on_server().await.unwrap();
+            assert!(!exists, "No backup exists at this point");
+        }
+
+        // Create a new backup. Should invalidate the cache
+        server.mock_add_room_keys_version().ok().expect(1).mount().await;
+        backups.create().await.expect("Failed to create a backup");
+
+        server.mock_room_keys_version().exists().expect(1).mount().await;
+        let exists = backups
+            .fast_exists_on_server()
+            .await
+            .expect("We should be able to check if backups exist on the server");
+
+        assert!(exists, "But now a backup does exist");
+    }
+
+    #[async_test]
+    async fn test_removing_a_backup_invalidates_fast_exists_on_server_cache() {
+        let server = MatrixMockServer::new().await;
+        let client = server.client_builder().build().await;
+        let backups = client.encryption().backups();
+
+        {
+            let _scope = server.mock_room_keys_version().exists().expect(1).mount_as_scoped().await;
+
+            // Call fast_exists_on_server to fill the cache
+            let exists = backups.fast_exists_on_server().await.unwrap();
+            assert!(exists, "A backup exists at this point");
+        }
+
+        // Delete the backup. Should invalidate the cache
+        server.mock_delete_room_keys_version().ok().expect(1).mount().await;
+        backups.delete_backup_from_server("1".to_owned()).await.expect("Failed to delete a backup");
+
+        server.mock_room_keys_version().none().expect(1).mount().await;
+        let exists = backups
+            .fast_exists_on_server()
+            .await
+            .expect("We should be able to check if backups exist on the server");
+
+        assert!(!exists, "But now there is no backup");
+    }
+
+    #[async_test]
+    async fn test_waiting_for_steady_state_resets_the_delay() {
+        let server = MatrixMockServer::new().await;
+        let client = server.client_builder().build().await;
+
+        server.mock_add_room_keys_version().ok().expect(1).mount().await;
 
         client
             .encryption()
@@ -1247,7 +1420,5 @@ mod test {
             { client.inner.e2ee.backup_state.upload_delay.read().unwrap().to_owned() };
 
         assert_eq!(old_duration, current_duration);
-
-        server.verify().await;
     }
 }

crates/matrix-sdk/src/encryption/backups/types.rs

@@ -53,6 +53,37 @@ pub(crate) struct BackupClientState {
     pub(crate) upload_progress: ChannelObservable<UploadState>,
     pub(super) global_state: ChannelObservable<BackupState>,
     pub(super) room_keys_broadcaster: broadcast::Sender<RoomKeyImportResult>,
+
+    /// Whether a key storage backup exists on the server, as far as we know.
+    ///
+    /// This is `None` if we have not asked the server yet, and `Some`
+    /// otherwise. This value is not always up-to-date: if the backup status
+    /// on the server was changed by some other client, we will have a old
+    /// value.
+    pub(super) backup_exists_on_server: RwLock<Option<bool>>,
+}
+
+impl BackupClientState {
+    /// Update the cached value indicating whether a key storage backup exists
+    /// on the server
+    pub(crate) fn set_backup_exists_on_server(&self, exists_on_server: bool) {
+        *self.backup_exists_on_server.write().unwrap() = Some(exists_on_server);
+    }
+
+    /// Ask whether the key storage backup exists on the server. Returns `None`
+    /// if we haven't checked. Note that this value will be out-of-date if
+    /// some other client changed the state since the last time we checked.
+    pub(crate) fn backup_exists_on_server(&self) -> Option<bool> {
+        *self.backup_exists_on_server.read().unwrap()
+    }
+
+    /// Clear out the cached value indicating whether a key storage backup
+    /// exists on the server, meaning that the code in
+    /// [`super::Backups`] will repopulate it when needed
+    /// with an up-to-date value.
+    pub(crate) fn clear_backup_exists_on_server(&self) {
+        *self.backup_exists_on_server.write().unwrap() = None;
+    }
 }
 
 const DEFAULT_BACKUP_UPLOAD_DELAY: Duration = Duration::from_millis(100);
@@ -64,6 +95,7 @@ impl Default for BackupClientState {
             upload_progress: ChannelObservable::new(UploadState::Idle),
             global_state: Default::default(),
             room_keys_broadcaster: broadcast::Sender::new(100),
+            backup_exists_on_server: RwLock::new(None),
         }
     }
 }

crates/matrix-sdk/src/test_utils/mocks.rs

@@ -591,6 +591,22 @@ impl MatrixMockServer {
             .and(header("authorization", "Bearer 1234"));
         MockEndpoint { mock, server: &self.server, endpoint: RoomKeysVersionEndpoint }
     }
+
+    /// Create a prebuilt mock for adding key storage backups via POST
+    pub fn mock_add_room_keys_version(&self) -> MockEndpoint<'_, AddRoomKeysVersionEndpoint> {
+        let mock = Mock::given(method("POST"))
+            .and(path_regex(r"_matrix/client/v3/room_keys/version"))
+            .and(header("authorization", "Bearer 1234"));
+        MockEndpoint { mock, server: &self.server, endpoint: AddRoomKeysVersionEndpoint }
+    }
+
+    /// Create a prebuilt mock for adding key storage backups via POST
+    pub fn mock_delete_room_keys_version(&self) -> MockEndpoint<'_, DeleteRoomKeysVersionEndpoint> {
+        let mock = Mock::given(method("DELETE"))
+            .and(path_regex(r"_matrix/client/v3/room_keys/version/[^/]*"))
+            .and(header("authorization", "Bearer 1234"));
+        MockEndpoint { mock, server: &self.server, endpoint: DeleteRoomKeysVersionEndpoint }
+    }
 }
 
 /// Parameter to [`MatrixMockServer::sync_room`].
@@ -1669,7 +1685,8 @@ impl<'a> MockEndpoint<'a, PublicRoomsEndpoint> {
     }
 }
 
-/// A prebuilt mock for `room_keys/version`: storage ("backup") of room keys.
+/// A prebuilt mock for `GET room_keys/version`: storage ("backup") of room
+/// keys.
 pub struct RoomKeysVersionEndpoint;
 
 impl<'a> MockEndpoint<'a, RoomKeysVersionEndpoint> {
@@ -1713,3 +1730,34 @@ impl<'a> MockEndpoint<'a, RoomKeysVersionEndpoint> {
         MatrixMock { server: self.server, mock }
     }
 }
+
+/// A prebuilt mock for `POST room_keys/version`: adding room key backups.
+pub struct AddRoomKeysVersionEndpoint;
+
+impl<'a> MockEndpoint<'a, AddRoomKeysVersionEndpoint> {
+    /// Returns an endpoint that may be used to add room key backups
+    pub fn ok(self) -> MatrixMock<'a> {
+        let mock = self
+            .mock
+            .respond_with(ResponseTemplate::new(200).set_body_json(json!({
+              "version": "1"
+            })))
+            .named("POST for the backup creation");
+        MatrixMock { server: self.server, mock }
+    }
+}
+
+/// A prebuilt mock for `DELETE room_keys/version/xxx`: deleting room key
+/// backups.
+pub struct DeleteRoomKeysVersionEndpoint;
+
+impl<'a> MockEndpoint<'a, DeleteRoomKeysVersionEndpoint> {
+    /// Returns an endpoint that allows deleting room key backups
+    pub fn ok(self) -> MatrixMock<'a> {
+        let mock = self
+            .mock
+            .respond_with(ResponseTemplate::new(200).set_body_json(json!({})))
+            .named("DELETE for the backup deletion");
+        MatrixMock { server: self.server, mock }
+    }
+}