bugfix: ensure the SessionStore is cleared when regenerating the OlmMachine

[kegsay]

This fixes #3110

[andybalaam]

@poljar I approve this code but can't review as I am a co-author

[poljar]

All of this looks good, but surely we can add some tests for this.

[Hywan]

Thanks for the PR!

The code looks good. How hard is it to add a test to ensure the caches are cleared as expected?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.60%. Comparing base (f7329c7) to head (381c02d).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3338      +/-   ##
==========================================
+ Coverage   83.58%   83.60%   +0.01%     
==========================================
  Files         240      240              
  Lines       24841    24849       +8     
==========================================
+ Hits        20764    20774      +10     
+ Misses       4077     4075       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kegsay]

The current test for this is https://github.com/matrix-org/complement-crypto/pull/52/files#diff-fd8835211753c17a95b8c01292df184e7a252c2544f0535baf79ea9db1d56d7cR243 which is what we used to verify the fix. We can probably write a unit test though for clear_caches?

[andybalaam]

I am working on a couple of tests.

[andybalaam]

Testing the clear_caches() implementation on the sqlite and indexeddb stores is difficult since the cache is not observable via external interfaces, and we need a real DB to run this code. I don't want to add to the CryptoStore interface just to test this.

I've added a test that regenerate_olm() calls clear_caches().

A real test will bring up two store instances against the same backing store and check that they keep up to date with each other.

I HEREBY PINKIE PROMISE TO WRITE A GENUINE REGRESSION TEST FOR #3110 OR FAIL TRYING.

But that will be a later PR.

[poljar]

I mean this is a test, so you got me there...

Sorry but can we have a test that puts a session into the store and cache and then when we call the method we check that the cache is empty?

As we agreed, porting the complement test isn't necessary but please at least something that doesn't just check that a method gets called.

[poljar]

Testing the clear_caches() implementation on the sqlite and indexeddb stores is difficult since the cache is not observable via external interfaces, and we need a real DB to run this code. I don't want to add to the CryptoStore interface just to test this.

To help you out, you don't need a new interface, the field is available inside the store specific tests:

diff --git a/crates/matrix-sdk-sqlite/src/crypto_store.rs b/crates/matrix-sdk-sqlite/src/crypto_store.rs
index 612165662..80a5fec40 100644
--- a/crates/matrix-sdk-sqlite/src/crypto_store.rs
+++ b/crates/matrix-sdk-sqlite/src/crypto_store.rs
@@ -1304,7 +1304,11 @@ mod tests {
 
 #[cfg(test)]
 mod encrypted_tests {
-    use matrix_sdk_crypto::{cryptostore_integration_tests, cryptostore_integration_tests_time};
+    use matrix_sdk_crypto::{
+        cryptostore_integration_tests, cryptostore_integration_tests_time,
+        store::{Changes, CryptoStore as _, PendingChanges},
+    };
+    use matrix_sdk_test::async_test;
     use once_cell::sync::Lazy;
     use tempfile::{tempdir, TempDir};
 
@@ -1321,6 +1325,26 @@ mod encrypted_tests {
             .expect("Can't create a passphrase protected store")
     }
 
+    #[async_test]
+    async fn cache_cleared() {
+        let store = get_store("cache_cleaerd", None).await;
+        // Given we created a session and saved it in the store
+        let (account, session) = cryptostore_integration_tests::get_account_and_session().await;
+        let sender_key = session.sender_key.to_base64();
+
+        store
+            .save_pending_changes(PendingChanges { account: Some(account.deep_clone()) })
+            .await
+            .expect("Can't save account");
+
+        let changes = Changes { sessions: vec![session.clone()], ..Default::default() };
+        store.save_changes(changes).await.unwrap();
+
+        store.session_cache.get(&sender_key).expect("We should have a session");
+
+        // TODO: Clear the cache
+    }
+
     cryptostore_integration_tests!();
     cryptostore_integration_tests_time!();
 }

[andybalaam]

Thanks @poljar , working on it.

[BillCarsonFr]

LGTM

[poljar]

I find the other test a bit pointless, but ok if we manage to port the complement test, then we can remove it.

[bnjbvr]

Not sure this test is really useful? This is using a fake crypto store impl, and just checking that a method has been called, which is better tested in the other test. How do y'all feel about removing this test instead and all the bootstrapping that it requires, which is a bit noisey to me?

[bnjbvr]

#3342

[poljar]

I agreed as much in my final review, it is a bit useful since it's the only place which checks that the method gets called.

Porting the complement crypto test is still in the pinkie promise so that might get rid of this noisy code.

[bnjbvr]

E_TOO_SLOW