From 9b9772392b1b2d16d3f3a99abf8e86b34dae4388 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Mon, 20 Apr 2026 13:10:12 +0200 Subject: [PATCH 1/2] refactor(sdk): move `load_linked_chunk_metadata` to a file where it can be reused for other linked chunks Makes sense to me to have it in the persistence file, since it's reloading all the chunks quickly to set the initial ordering. --- .../src/event_cache/caches/room/state.rs | 128 +---------------- .../matrix-sdk/src/event_cache/persistence.rs | 130 +++++++++++++++++- 2 files changed, 130 insertions(+), 128 deletions(-) diff --git a/crates/matrix-sdk/src/event_cache/caches/room/state.rs b/crates/matrix-sdk/src/event_cache/caches/room/state.rs index addcfe736c1..a240c7fbe41 100644 --- a/crates/matrix-sdk/src/event_cache/caches/room/state.rs +++ b/crates/matrix-sdk/src/event_cache/caches/room/state.rs @@ -31,8 +31,7 @@ use matrix_sdk_base::{ store::{EventCacheStoreLock, EventCacheStoreLockGuard, EventCacheStoreLockState}, }, linked_chunk::{ - ChunkIdentifierGenerator, ChunkMetadata, LinkedChunkId, OwnedLinkedChunkId, Position, - Update, lazy_loader, + ChunkIdentifierGenerator, LinkedChunkId, OwnedLinkedChunkId, Position, Update, lazy_loader, }, serde_helpers::{extract_edit_target, extract_thread_root}, sync::Timeline, @@ -76,6 +75,7 @@ use crate::{ Room, event_cache::{ automatic_pagination::AutomaticPagination, caches::pagination::SharedPaginationStatus, + persistence::load_linked_chunk_metadata, }, room::WeakRoom, }; @@ -1430,127 +1430,3 @@ fn get_event_focused_cache( let key = EventFocusedCacheKey { focused: event_id, thread_mode }; state.event_focused_caches.get(&key).cloned() } - -/// Load a linked chunk's full metadata, making sure the chunks are -/// according to their their links. -/// -/// Returns `None` if there's no such linked chunk in the store, or an -/// error if the linked chunk is malformed. -async fn load_linked_chunk_metadata( - store_guard: &EventCacheStoreLockGuard, - linked_chunk_id: LinkedChunkId<'_>, -) -> Result>, EventCacheError> { - let mut all_chunks = store_guard - .load_all_chunks_metadata(linked_chunk_id) - .await - .map_err(EventCacheError::from)?; - - if all_chunks.is_empty() { - // There are no chunks, so there's nothing to do. - return Ok(None); - } - - // Transform the vector into a hashmap, for quick lookup of the predecessors. - let chunk_map: HashMap<_, _> = all_chunks.iter().map(|meta| (meta.identifier, meta)).collect(); - - // Find a last chunk. - let mut iter = all_chunks.iter().filter(|meta| meta.next.is_none()); - let Some(last) = iter.next() else { - return Err(EventCacheError::InvalidLinkedChunkMetadata { - details: "no last chunk found".to_owned(), - }); - }; - - // There must at most one last chunk. - if let Some(other_last) = iter.next() { - return Err(EventCacheError::InvalidLinkedChunkMetadata { - details: format!( - "chunks {} and {} both claim to be last chunks", - last.identifier.index(), - other_last.identifier.index() - ), - }); - } - - // Rewind the chain back to the first chunk, and do some checks at the same - // time. - let mut seen = HashSet::new(); - let mut current = last; - loop { - // If we've already seen this chunk, there's a cycle somewhere. - if !seen.insert(current.identifier) { - return Err(EventCacheError::InvalidLinkedChunkMetadata { - details: format!( - "cycle detected in linked chunk at {}", - current.identifier.index() - ), - }); - } - - let Some(prev_id) = current.previous else { - // If there's no previous chunk, we're done. - if seen.len() != all_chunks.len() { - return Err(EventCacheError::InvalidLinkedChunkMetadata { - details: format!( - "linked chunk likely has multiple components: {} chunks seen through the chain of predecessors, but {} expected", - seen.len(), - all_chunks.len() - ), - }); - } - break; - }; - - // If the previous chunk is not in the map, then it's unknown - // and missing. - let Some(pred_meta) = chunk_map.get(&prev_id) else { - return Err(EventCacheError::InvalidLinkedChunkMetadata { - details: format!( - "missing predecessor {} chunk for {}", - prev_id.index(), - current.identifier.index() - ), - }); - }; - - // If the previous chunk isn't connected to the next, then the link is invalid. - if pred_meta.next != Some(current.identifier) { - return Err(EventCacheError::InvalidLinkedChunkMetadata { - details: format!( - "chunk {}'s next ({:?}) doesn't match the current chunk ({})", - pred_meta.identifier.index(), - pred_meta.next.map(|chunk_id| chunk_id.index()), - current.identifier.index() - ), - }); - } - - current = *pred_meta; - } - - // At this point, `current` is the identifier of the first chunk. - // - // Reorder the resulting vector, by going through the chain of `next` links, and - // swapping items into their final position. - // - // Invariant in this loop: all items in [0..i[ are in their final, correct - // position. - let mut current = current.identifier; - for i in 0..all_chunks.len() { - // Find the target metadata. - let j = all_chunks - .iter() - .rev() - .position(|meta| meta.identifier == current) - .map(|j| all_chunks.len() - 1 - j) - .expect("the target chunk must be present in the metadata"); - if i != j { - all_chunks.swap(i, j); - } - if let Some(next) = all_chunks[i].next { - current = next; - } - } - - Ok(Some(all_chunks)) -} diff --git a/crates/matrix-sdk/src/event_cache/persistence.rs b/crates/matrix-sdk/src/event_cache/persistence.rs index 6b6580362fd..e08535387f7 100644 --- a/crates/matrix-sdk/src/event_cache/persistence.rs +++ b/crates/matrix-sdk/src/event_cache/persistence.rs @@ -12,17 +12,19 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::collections::{HashMap, HashSet}; + use matrix_sdk_base::{ deserialized_responses::TimelineEventKind, event_cache::{Event, Gap, store::EventCacheStoreLockGuard}, executor::spawn, - linked_chunk::{OwnedLinkedChunkId, Update}, + linked_chunk::{ChunkMetadata, LinkedChunkId, OwnedLinkedChunkId, Update}, }; use ruma::serde::Raw; use tokio::sync::broadcast::Sender; use tracing::trace; -use crate::event_cache::{Result, caches::room::RoomEventCacheLinkedChunkUpdate}; +use crate::event_cache::{EventCacheError, Result, caches::room::RoomEventCacheLinkedChunkUpdate}; /// Propagate linked chunk updates to the store and to the linked chunk update /// observers. @@ -136,3 +138,127 @@ fn strip_relations_if_present(event: &mut Raw) { }; let _ = closure(); } + +/// Load a linked chunk's full metadata, making sure the chunks are +/// according to their their links. +/// +/// Returns `None` if there's no such linked chunk in the store, or an +/// error if the linked chunk is malformed. +pub(super) async fn load_linked_chunk_metadata( + store_guard: &EventCacheStoreLockGuard, + linked_chunk_id: LinkedChunkId<'_>, +) -> Result>> { + let mut all_chunks = store_guard + .load_all_chunks_metadata(linked_chunk_id) + .await + .map_err(EventCacheError::from)?; + + if all_chunks.is_empty() { + // There are no chunks, so there's nothing to do. + return Ok(None); + } + + // Transform the vector into a hashmap, for quick lookup of the predecessors. + let chunk_map: HashMap<_, _> = all_chunks.iter().map(|meta| (meta.identifier, meta)).collect(); + + // Find a last chunk. + let mut iter = all_chunks.iter().filter(|meta| meta.next.is_none()); + let Some(last) = iter.next() else { + return Err(EventCacheError::InvalidLinkedChunkMetadata { + details: "no last chunk found".to_owned(), + }); + }; + + // There must at most one last chunk. + if let Some(other_last) = iter.next() { + return Err(EventCacheError::InvalidLinkedChunkMetadata { + details: format!( + "chunks {} and {} both claim to be last chunks", + last.identifier.index(), + other_last.identifier.index() + ), + }); + } + + // Rewind the chain back to the first chunk, and do some checks at the same + // time. + let mut seen = HashSet::new(); + let mut current = last; + loop { + // If we've already seen this chunk, there's a cycle somewhere. + if !seen.insert(current.identifier) { + return Err(EventCacheError::InvalidLinkedChunkMetadata { + details: format!( + "cycle detected in linked chunk at {}", + current.identifier.index() + ), + }); + } + + let Some(prev_id) = current.previous else { + // If there's no previous chunk, we're done. + if seen.len() != all_chunks.len() { + return Err(EventCacheError::InvalidLinkedChunkMetadata { + details: format!( + "linked chunk likely has multiple components: {} chunks seen through the chain of predecessors, but {} expected", + seen.len(), + all_chunks.len() + ), + }); + } + break; + }; + + // If the previous chunk is not in the map, then it's unknown + // and missing. + let Some(pred_meta) = chunk_map.get(&prev_id) else { + return Err(EventCacheError::InvalidLinkedChunkMetadata { + details: format!( + "missing predecessor {} chunk for {}", + prev_id.index(), + current.identifier.index() + ), + }); + }; + + // If the previous chunk isn't connected to the next, then the link is invalid. + if pred_meta.next != Some(current.identifier) { + return Err(EventCacheError::InvalidLinkedChunkMetadata { + details: format!( + "chunk {}'s next ({:?}) doesn't match the current chunk ({})", + pred_meta.identifier.index(), + pred_meta.next.map(|chunk_id| chunk_id.index()), + current.identifier.index() + ), + }); + } + + current = *pred_meta; + } + + // At this point, `current` is the identifier of the first chunk. + // + // Reorder the resulting vector, by going through the chain of `next` links, and + // swapping items into their final position. + // + // Invariant in this loop: all items in [0..i[ are in their final, correct + // position. + let mut current = current.identifier; + for i in 0..all_chunks.len() { + // Find the target metadata. + let j = all_chunks + .iter() + .rev() + .position(|meta| meta.identifier == current) + .map(|j| all_chunks.len() - 1 - j) + .expect("the target chunk must be present in the metadata"); + if i != j { + all_chunks.swap(i, j); + } + if let Some(next) = all_chunks[i].next { + current = next; + } + } + + Ok(Some(all_chunks)) +} From 4892c81c69334748a1f41b140c9d5070a3737838 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Mon, 20 Apr 2026 13:13:29 +0200 Subject: [PATCH 2/2] fix(pinned events): initialize events ordering before reloading from disk This fixes a subtle bug, found by re-enabling the assertion disabled in github.com/matrix-org/matrix-rust-sdk/pull/5332. Since the events ordering was never properly initialized, events couldn't be ordered against each other even after being added etc. This could result in the incorrect latest edit being shown in a pinned events timeline (maybe? it's hard to make a standalone test case for this, so I'll really on people re-enabling the ordering assertion back in the future). --- .../src/event_cache/caches/pinned_events/mod.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk/src/event_cache/caches/pinned_events/mod.rs b/crates/matrix-sdk/src/event_cache/caches/pinned_events/mod.rs index 4d77ae2b881..94702c9963d 100644 --- a/crates/matrix-sdk/src/event_cache/caches/pinned_events/mod.rs +++ b/crates/matrix-sdk/src/event_cache/caches/pinned_events/mod.rs @@ -41,7 +41,10 @@ use super::{ room::RoomEventCacheLinkedChunkUpdate, }; use crate::{ - Room, client::WeakClient, config::RequestConfig, event_cache::TimelineVectorDiffs, + Room, + client::WeakClient, + config::RequestConfig, + event_cache::{TimelineVectorDiffs, persistence::load_linked_chunk_metadata}, room::WeakRoom, }; @@ -124,6 +127,12 @@ impl<'a> PinnedEventCacheStateLockWriteGuard<'a> { return Ok(()); }; + let linked_chunk_metadata = + load_linked_chunk_metadata(&self.store, linked_chunk_id).await?; + + // Reload the metadata first. + self.state.chunk = EventLinkedChunk::with_initial_linked_chunk(None, linked_chunk_metadata); + { let mut current_chunk_identifier = last_chunk.identifier; self.state.chunk.replace_with(Some(last_chunk), chunk_id_gen)?;