From c2599451e82a1bebe39f4e7e4c45b6ce7acdc366 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 8 Jan 2025 19:21:35 +0100 Subject: [PATCH 1/3] fix(sdk): Ensure a gap has been inserted before removing it. This patch fixes a bug where the code assumes a gap has been inserted, and thus, is always present. But this isn't the case. If `prev_batch` is `None`, a gap is not inserted, and so we cannot remove it. This patch checks that `prev_batch` is `Some(_)`, which means the invariant is correct, and the code can remove the gap. --- crates/matrix-sdk/src/event_cache/room/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk/src/event_cache/room/mod.rs b/crates/matrix-sdk/src/event_cache/room/mod.rs index 7ad4a6afe2b..103d6863c44 100644 --- a/crates/matrix-sdk/src/event_cache/room/mod.rs +++ b/crates/matrix-sdk/src/event_cache/room/mod.rs @@ -431,7 +431,7 @@ impl RoomEventCacheInner { let added_unique_events = room_events.push_events(sync_timeline_events.clone()); - if !added_unique_events { + if !added_unique_events && prev_batch.is_some() { debug!( "not storing previous batch token, because we deduplicated all new sync events" ); From 40fb11914f10433aa945bff5ad4e9c6d851f2473 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 9 Jan 2025 11:22:56 +0100 Subject: [PATCH 2/3] refactor(event cache): use a more fine-grained check for the gap removal --- crates/matrix-sdk/src/event_cache/room/mod.rs | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/crates/matrix-sdk/src/event_cache/room/mod.rs b/crates/matrix-sdk/src/event_cache/room/mod.rs index 103d6863c44..fca7f9b0f14 100644 --- a/crates/matrix-sdk/src/event_cache/room/mod.rs +++ b/crates/matrix-sdk/src/event_cache/room/mod.rs @@ -431,19 +431,30 @@ impl RoomEventCacheInner { let added_unique_events = room_events.push_events(sync_timeline_events.clone()); - if !added_unique_events && prev_batch.is_some() { + if !added_unique_events { debug!( "not storing previous batch token, because we deduplicated all new sync events" ); - // Remove the gap we just inserted. - let prev_gap_id = room_events - .rchunks() - .find_map(|c| c.is_gap().then_some(c.identifier())) - .expect("we just inserted the gap beforehand"); - room_events - .replace_gap_at([], prev_gap_id) - .expect("we obtained the valid position beforehand"); + if let Some(prev_token) = &prev_batch { + // Note: there can't be any race with another task touching the linked + // chunk at this point, because we're using `with_events_mut` which + // guards access to the data. + trace!("removing gap we just inserted"); + + // Find the gap that had the previous-batch token we inserted above. + let prev_gap_id = room_events + .rchunks() + .find_map(|c| { + let gap = as_variant::as_variant!(c.content(), ChunkContent::Gap)?; + (gap.prev_token == *prev_token).then_some(c.identifier()) + }) + .expect("we just inserted the gap beforehand"); + + room_events + .replace_gap_at([], prev_gap_id) + .expect("we obtained the valid position beforehand"); + } } }) .await?; From e0c9b7a3d03d6bd34647fa0acd0e1575a2e1b1ce Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 9 Jan 2025 11:28:50 +0100 Subject: [PATCH 3/3] test(event cache): add a regression test for not deleting a gap that wasn't inserted --- .../tests/integration/event_cache.rs | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/crates/matrix-sdk/tests/integration/event_cache.rs b/crates/matrix-sdk/tests/integration/event_cache.rs index 08189f34404..602c3732838 100644 --- a/crates/matrix-sdk/tests/integration/event_cache.rs +++ b/crates/matrix-sdk/tests/integration/event_cache.rs @@ -1378,3 +1378,67 @@ async fn test_no_gap_stored_after_deduplicated_backpagination() { assert!(stream.is_empty()); } + +#[async_test] +async fn test_dont_delete_gap_that_wasnt_inserted() { + let server = MatrixMockServer::new().await; + let client = server.client_builder().build().await; + + let event_cache = client.event_cache(); + + // Immediately subscribe the event cache to sync updates. + event_cache.subscribe().unwrap(); + event_cache.enable_storage().unwrap(); + + let room_id = room_id!("!omelette:fromage.fr"); + + let f = EventFactory::new().room(room_id).sender(user_id!("@a:b.c")); + + // Start with a room with a single event, limited timeline and prev-batch token. + let room = server + .sync_room( + &client, + JoinedRoomBuilder::new(room_id) + .add_timeline_event(f.text_msg("sup").event_id(event_id!("$3")).into_raw_sync()) + .set_timeline_limited() + .set_timeline_prev_batch("prev-batch".to_owned()), + ) + .await; + + let (room_event_cache, _drop_handles) = room.event_cache().await.unwrap(); + + let (events, mut stream) = room_event_cache.subscribe().await.unwrap(); + if events.is_empty() { + assert_let_timeout!(Ok(RoomEventCacheUpdate::UpdateTimelineEvents { .. }) = stream.recv()); + } + drop(events); + + // Back-paginate to consume the existing gap. + // Say the back-pagination doesn't return anything. + server + .mock_room_messages() + .from("prev-batch") + .ok("start-token-unused".to_owned(), None, Vec::>::new(), Vec::new()) + .mock_once() + .mount() + .await; + room_event_cache.pagination().run_backwards(20, once).await.unwrap(); + + // This doesn't cause an update, because nothing changed. + assert!(stream.is_empty()); + + // After a restart, a sync with the same sliding sync window may return the same + // events, but no prev-batch token this time. + server + .sync_room( + &client, + JoinedRoomBuilder::new(room_id).add_timeline_bulk(vec![f + .text_msg("sup") + .event_id(event_id!("$3")) + .into_raw_sync()]), + ) + .await; + + // This doesn't cause an update, because nothing changed. + assert!(stream.is_empty()); +}