fix(event cache): don't try to remove a previous gap if it's the only chunk in memory

A linked chunk never wants to be empty. However, after a limited gap
that doesn't contain events, it may be shrunk to the latest chunk that's
a gap.

If later we decide to remove the gap (because it's been resolved with no
events), then we would try to remove the last chunk, which is not
correct.

Ideally, we'd keep an events chunk around; but if we have an events
chunk *before* a gap, that may look like missing events to the user, at
least until the gap has been resolved.

The fix to this problem is to *not* optimize / remove the gap, if it's
the only chunk kept in memory. This was only a memory optimization, but
it's not absolutely required per se.

Author: bnjbvr

crates/matrix-sdk/src/event_cache/pagination.rs

@@ -367,17 +367,15 @@ impl RoomPagination {
             let insert_new_gap_pos = if let Some(gap_id) = prev_gap_id {
                 // There is a prior gap, let's replace it by new events!
                 if all_duplicates {
-                    // All the events were duplicated; don't act upon them, and only remove the
-                    // prior gap that we just filled.
-                    trace!("removing previous gap, as all events have been deduplicated");
-                    room_events.remove_empty_chunk_at(gap_id).expect("gap identifier is a valid gap chunk id we read previously")
-                } else {
-                    trace!("replacing previous gap with the back-paginated events");
-
-                    // Replace the gap with the events we just deduplicated.
-                    room_events.replace_gap_at(reversed_events.clone(), gap_id)
-                        .expect("gap_identifier is a valid chunk id we read previously")
+                    assert!(reversed_events.is_empty());
                 }
+
+                trace!("replacing previous gap with the back-paginated events");
+
+                // Replace the gap with the events we just deduplicated. This might get rid of the
+                // underlying gap, if the conditions are favorable to us.
+                room_events.replace_gap_at(reversed_events.clone(), gap_id)
+                    .expect("gap_identifier is a valid chunk id we read previously")
             } else if let Some(pos) = first_event_pos {
                 // No prior gap, but we had some events: assume we need to prepend events
                 // before those.

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

@@ -126,7 +126,8 @@ impl RoomEvents {
 
     /// Remove an empty chunk at the given position.
     ///
-    /// Note: the chunk must either be a gap, or an empty items chunk.
+    /// Note: the chunk must either be a gap, or an empty items chunk, and it
+    /// must NOT be the last one.
     ///
     /// Returns the next insert position, if any, left after the chunk that has
     /// just been removed.
@@ -149,7 +150,24 @@ impl RoomEvents {
         events: Vec<Event>,
         gap_identifier: ChunkIdentifier,
     ) -> Result<Option<Position>, Error> {
-        let next_pos = if events.is_empty() {
+        // As an optimization, we'll remove the empty chunk if it's a gap.
+        //
+        // However, our linked chunk requires that it includes at least one chunk in the
+        // in-memory representation. We could tweak this invariant, but in the
+        // meanwhile, don't remove the gap chunk if it's the only one we know
+        // about.
+        let has_only_one_chunk = {
+            let mut it = self.chunks.chunks();
+
+            // If there's no chunks at all, then we won't be able to find the gap chunk.
+            let _ =
+                it.next().ok_or(Error::InvalidChunkIdentifier { identifier: gap_identifier })?;
+
+            // If there's no next chunk, we can conclude there's only one.
+            it.next().is_none()
+        };
+
+        let next_pos = if events.is_empty() && !has_only_one_chunk {
             // There are no new events, so there's no need to create a new empty items
             // chunk; instead, remove the gap.
             self.chunks.remove_empty_chunk_at(gap_identifier)?

crates/matrix-sdk/tests/integration/event_cache.rs

@@ -2507,3 +2507,36 @@ async fn test_timeline_then_empty_timeline_then_deduplication_with_storage() {
     // That's all, folks!
     assert!(subscriber.is_empty());
 }
+
+#[async_test]
+async fn test_dont_remove_only_gap() {
+    let server = MatrixMockServer::new().await;
+    let client = server.client_builder().build().await;
+
+    client.event_cache().subscribe().unwrap();
+    client.event_cache().enable_storage().unwrap();
+
+    let room_id = room_id!("!galette:saucisse.bzh");
+    let room = server
+        .sync_room(
+            &client,
+            JoinedRoomBuilder::new(room_id)
+                .set_timeline_limited()
+                .set_timeline_prev_batch("brillat-savarin"),
+        )
+        .await;
+
+    let (room_event_cache, _drop_handles) = room.event_cache().await.unwrap();
+
+    server
+        .mock_room_messages()
+        .match_from("brillat-savarin")
+        .ok(RoomMessagesResponseTemplate::default())
+        .named("room/messages")
+        .mount()
+        .await;
+
+    // Back-paginate with the given token.
+    let outcome = room_event_cache.pagination().run_backwards_once(16).await.unwrap();
+    assert!(outcome.reached_start);
+}