fix: Duplicated events are always removed

[Hywan]

When a duplicated event is found, sometimes we keep the older one and the new one is dropped, but sometimes the older one is removed and the new one is kept (backwards pagination vs. sync). So we need to be able to remove an event.

However, since #4662 and #4632, this can fail. Why? Because we can find a duplicated event in the store, and that is not loaded in the in-memory LinkedChunk. What happens in this case? We simply give up! See the error! case?

matrix-rust-sdk/crates/matrix-sdk/src/event_cache/room/events.rs

Lines 246 to 254 in bdf5fad

	pub fn remove_events_by_id(&mut self, event_ids: Vec<OwnedEventId>) {
	for event_id in event_ids {
	let Some(event_position) = self.revents().find_map(|(position, event)| {
	(event.event_id().as_ref() == Some(&event_id)).then_some(position)
	}) else {
	error!(?event_id, "Cannot find the event to remove");
	continue;
	};

Well, this is pretty bad, because it means the duplicated event will not be removed. And it means a new event with the same ID will be inserted. Kaboom 💥.

These patches fix that in simple steps:

1. EventCacheStore::filter_duplicated_events returns the event positions,
2. Deduplicator is redesigned to produce a DeduplicatorOutcome which makes the difference between in-memory vs. in-store duplicated events,
3. Use this information accordingly.

These patches also fix inconsistencies between the BloomFilterDeduplicator (used when there is no store) and StoreDeduplicator (used when there is a store), like: the new events can contain duplicates (new_events = [$ev0, $ev1, $ev0]) in theory (not in practise so far). This check was only done in the bloom filter deduplicator. Now it's done for all of them, directly in Deduplicator. Same for detecting events with no ID, it's done beforehand. See the patches one by one.

Tasks

• Do everything
• Test

---

• Address [meta] EventCache storage #3280
• Follow up of feat(sdk): Implement EventCache lazy-loading #4632

[codecov]

Codecov Report

Attention: Patch coverage is 97.11538% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.91%. Comparing base (f900db4) to head (a7ca6e5).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/event_cache/deduplicator.rs	97.72%	1 Missing ⚠️
crates/matrix-sdk/src/event_cache/room/events.rs	83.33%	1 Missing ⚠️
crates/matrix-sdk/src/event_cache/room/mod.rs	96.15%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4706   +/-   ##
=======================================
  Coverage   85.90%   85.91%           
=======================================
  Files         292      292           
  Lines       33906    33950   +44     
=======================================
+ Hits        29128    29168   +40     
- Misses       4778     4782    +4     

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

[stefanceriu]

Left some comments inline but otherwise makes sense and lgtm 👍

[stefanceriu]

This feels a bit footgun-y, can't we sort them here too, similar to what the deduplicator is doing in its sort_events_by_position_descending?

[Hywan]

You are right. I've rewritten this part. Deduplicator is no longer responsible to sort events by their position. This is done in a new RoomEventCacheState::remove_events, which is responsible to remove events in the store, and in RoomEvents.

[bnjbvr]

Nice work! just skimmed the high-level changes so I could understand it, and it makes sense to me 👍