Key import race causing a worse key to be saved in place of a better version.

[BillCarsonFr]

As seen in this rageshake https://rageshakes.element.io/api/listing/2024-11-06/130841-YOSPUGGB/console.2024-11-06-12.log.gz

There is slightly late room key, so there is an UTD and the sdk tries to query the key from key storage.
And just after the room key arrives down the sync.

We then have two concurent actions trying to import the key

• From key storage (using import_room_keys)
• And down the sync (using handle_key)

The race is that they both try to see if there is an existing key in order to see if the new one is better or worse

matrix-rust-sdk/crates/matrix-sdk-crypto/src/store/mod.rs

Lines 1748 to 1756 in 8d07f36

	let old_session = self
	.inner
	.store
	.get_inbound_group_session(session.room_id(), session.session_id())
	.await?;
	// Only import the session if we didn't have this session or
	// if it's a better version of the same session.
	if new_session_better(&session, old_session).await {

and

matrix-rust-sdk/crates/matrix-sdk-crypto/src/machine/mod.rs

Lines 891 to 906 in 8d07f36

	match self.store().compare_group_session(&session).await? {
	SessionOrdering::Better => {
	info!("Received a new megolm room key");
	Ok(Some(session))
	}
	comparison_result => {
	warn!(
	?comparison_result,
	"Received a megolm room key that we already have a better version \
	of, discarding"
	);
	Ok(None)
	}
	}

But they both think they are the first and that there is no matching key in store.

Therefore the first one to commit to the store wins. By-pasing the better/worse check.
The late key is better than the key from key storage, because it is safe (no gray shield)

[BillCarsonFr]

One source of element-hq/element-x-ios#3352

[BillCarsonFr]

We might want to have some import room key lock.

[andybalaam]

@poljar : we think adding an in-memory lock would fix this, but we need to try it out.