Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/matrix-sdk-search/changelog.d/6731.fixed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
`RoomIndex::bulk_execute` now waits for the index writers merge threads before dropping it, avoiding spurious "couldn't find segment in SegmentManager" warnings and index fragmentation
35 changes: 35 additions & 0 deletions crates/matrix-sdk-search/src/index/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ impl RoomIndex {
}

self.commit_and_reload(&mut writer)?;
writer.wait_merging_threads()?;

Ok(())
}
Expand Down Expand Up @@ -450,6 +451,40 @@ mod tests {
Ok(())
}

#[test]
fn test_bulk_execute_indexes_all_events() -> Result<(), Box<dyn Error>> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run this test with and without your previous commit and nothing changes. I quote your description of the problem:

RoomIndex::bulk_execute oepns a tantivy IndexWriter, commits, and then lets it drop without calling wait_merging_threads(). The writers background merge threads can outlive it and still be running when the next bulk_execute opens a new writer on the same index, so the two race over the same segments

So I understand we need at least 2 bulk_execute calls. We have a single one here.

let room_id = room_id!("!room_id:localhost");
let mut index = RoomIndexBuilder::new_in_memory(room_id).build();

let event_id_1 = event_id!("$event_id_1:localhost");
let event_id_2 = event_id!("$event_id_2:localhost");
let user_id = user_id!("@user_id:localhost");
let f = EventFactory::new().room(room_id).sender(user_id);

let ops = vec![
RoomIndexOperation::Add(
f.text_msg("This is a sentence")
.event_id(event_id_1)
.into_original_sync_room_message_event(),
),
RoomIndexOperation::Add(
f.text_msg("Another sentence")
.event_id(event_id_2)
.into_original_sync_room_message_event(),
),
];

index.bulk_execute(ops)?;

let result: HashSet<_> =
index.search("sentence", 10, None)?.into_iter().map(|(_score, id)| id).collect();
let expected: HashSet<_> =
[event_id_1.to_owned(), event_id_2.to_owned()].into_iter().collect();
assert_eq!(result, expected, "bulk_execute did not index all events: {result:?}");

Ok(())
}

#[test]
fn test_search_empty_index() -> Result<(), Box<dyn Error>> {
let room_id = room_id!("!room_id:localhost");
Expand Down
12 changes: 8 additions & 4 deletions crates/matrix-sdk-search/src/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,26 @@ pub(crate) struct SearchIndexWriter {
}

impl SearchIndexWriter {
pub(crate) fn new(writer: IndexWriter, schema: RoomMessageSchema) -> Self {
pub fn new(writer: IndexWriter, schema: RoomMessageSchema) -> Self {
Self { last_commit_opstamp: writer.commit_opstamp(), inner: writer, schema }
}

pub(crate) fn add(&self, document: TantivyDocument) -> Result<OpStamp, IndexError> {
pub fn add(&self, document: TantivyDocument) -> Result<OpStamp, IndexError> {
Ok(self.inner.add_document(document)?) // TODO: This is blocking. Handle
// it.
}

pub(crate) fn remove(&self, event_id: &EventId) {
pub fn remove(&self, event_id: &EventId) {
self.inner
.delete_term(Term::from_field_text(self.schema.deletion_key(), event_id.as_str()));
}

pub(crate) fn commit(&mut self) -> Result<OpStamp, TantivyError> {
pub fn commit(&mut self) -> Result<OpStamp, TantivyError> {
self.last_commit_opstamp = self.inner.commit()?; // TODO: This is blocking. Handle it.
Ok(self.last_commit_opstamp)
}

pub fn wait_merging_threads(self) -> Result<(), TantivyError> {
self.inner.wait_merging_threads()
}
}
Loading