Fix assert-crash due to excessive scores length

For searches that hit multiple mailboxes (virtual mailboxes is an easy way
to force a multisearch), an assert-crash will happen if a message
matches both the main query and internal "maybe" queries.

Fix by ensuring that scores are only added once per UID.

This was broken in single mailbox search as well, but it was ignored there
as there was no assert check on scores length (the extra entries were ignored
during processing).

In order to calcuate scores correctly (now that we are only adding them once),
refactor result iteration to only run the maybe query once for all maybe
queries.  Add the main query to this query for score calculation purposes.
This is a bit of an optimization, so nice.

Fixes GitHub Issue #61

.github/common/fts-flatcurve-test.sh

@@ -203,6 +203,10 @@ run_test "Testing GitHub Issue #38" \
 	/dovecot/configs/dovecot.conf.issue-38 \
 	/dovecot/imaptest/issue-38/issue-38
 
+run_test "Testing GitHub Issue #61" \
+	/dovecot/configs/dovecot.conf.virtual \
+	/dovecot/imaptest/issue-61
+
 TESTBOX=INBOX
 echo
 echo "Testing 'doveadm fts-flatcurve remove'"

.github/common/imaptest/issue-61

@@ -0,0 +1,24 @@
+messages: 1
+
+ok search or from user body body
+* search 1
+ok search or header reply-to user body body
+* search 1
+ok search or header reply-to foo body bar
+* search
+ok search or header reply-to user or header reply-to domain body body
+* search 1
+
+ok examine Virtual/All
+
+# From header works
+ok search or from user body body
+* search 1
+
+# Panic: file fts-search.c: line 87 (level_scores_add_vuids): assertion failed: (array_count(&vuids_arr) == array_count(&br->scores))
+ok search or header reply-to user body body
+* search 1
+ok search or header reply-to foo body bar
+* search
+ok search or header reply-to user or header reply-to domain body body
+* search 1

src/fts-backend-flatcurve-xapian.cpp

@@ -196,10 +196,11 @@ struct fts_flatcurve_xapian_query_iter {
 	Xapian::Database *db;
 	Xapian::Enquire *enquire;
 	Xapian::MSetIterator i;
+	Xapian::MSet m;
 	struct fts_flatcurve_xapian_query_result *result;
-	int curr_query;
 
-	bool next_query:1;
+	bool init:1;
+	bool main_query:1;
 };
 
 static void
@@ -1687,10 +1688,8 @@ fts_flatcurve_xapian_query_iter_init(struct flatcurve_fts_query *query)
 	struct fts_flatcurve_xapian_query_iter *iter;
 
 	iter = p_new(query->pool, struct fts_flatcurve_xapian_query_iter, 1);
-	/* -1: "Master" query
-	 * >= 0: Current index of maybe_queries */
-	iter->curr_query = -1;
-	iter->next_query = TRUE;
+	iter->init = FALSE;
+	iter->main_query = TRUE;
 	iter->query = query;
 	iter->result = p_new(query->pool,
 			     struct fts_flatcurve_xapian_query_result, 1);
@@ -1701,33 +1700,36 @@ fts_flatcurve_xapian_query_iter_init(struct flatcurve_fts_query *query)
 struct fts_flatcurve_xapian_query_result *
 fts_flatcurve_xapian_query_iter_next(struct fts_flatcurve_xapian_query_iter *iter)
 {
-	Xapian::MSet m;
+	Xapian::Query maybe, *q = NULL;
 	const struct flatcurve_fts_query_xapian_maybe *mquery;
 	enum flatcurve_xapian_db_opts opts =
 		ENUM_EMPTY(flatcurve_xapian_db_opts);
-	Xapian::Query *q = NULL;
 
-	if (iter->next_query) {
-		iter->next_query = FALSE;
+	if (!iter->init) {
+		iter->init = TRUE;
 
 		/* Master query. */
-		if (iter->curr_query == -1) {
+		if (iter->main_query) {
 			if (iter->query->xapian->query == NULL)
-				++iter->curr_query;
+				iter->main_query = FALSE;
 			else
 				q = iter->query->xapian->query;
 		}
 
 		/* Maybe queries. */
-		if ((iter->curr_query >= 0) &&
-			(array_is_created(&iter->query->xapian->maybe_queries)) &&
-			(array_count(&iter->query->xapian->maybe_queries) > iter->curr_query)) {
-			mquery = array_idx(&iter->query->xapian->maybe_queries,
-							   iter->curr_query);
-			q = mquery->query;
+		if (!iter->main_query &&
+			array_is_created(&iter->query->xapian->maybe_queries)) {
+			maybe = Xapian::Query();
+			array_foreach(&iter->query->xapian->maybe_queries, mquery)
+				maybe = Xapian::Query(Xapian::Query::OP_OR, maybe, *mquery->query);
+			/* Add main query to merge Xapian scores correctly. */
+			if (iter->query->xapian->query != NULL)
+				maybe = Xapian::Query(Xapian::Query::OP_AND_MAYBE, maybe,
+					*iter->query->xapian->query);
+			q = &maybe;
 		}
 
-		if (iter->db == NULL)
+		if ((q != NULL) && (iter->db == NULL))
 		    iter->db = fts_flatcurve_xapian_read_db(iter->query->backend, opts);
 
 		if ((q == NULL) || (iter->db == NULL))
@@ -1740,7 +1742,7 @@ fts_flatcurve_xapian_query_iter_next(struct fts_flatcurve_xapian_query_iter *ite
 		iter->enquire->set_query(*q);
 
 		try {
-			m = iter->enquire->get_mset(0, iter->db->get_doccount());
+			iter->m = iter->enquire->get_mset(0, iter->db->get_doccount());
 		} catch (Xapian::DatabaseModifiedError &e) {
 			/* Per documentation, this is only thrown if more than
 			 * one change has been made to the database. To
@@ -1754,16 +1756,17 @@ fts_flatcurve_xapian_query_iter_next(struct fts_flatcurve_xapian_query_iter *ite
 			i_unreached();
 		}
 
-		iter->i = m.begin();
+		iter->i = iter->m.begin();
 	}
 
-	if (iter->i == m.end()) {
-		++iter->curr_query;
-		iter->next_query = TRUE;
+	if (iter->i == iter->m.end()) {
+		if (!iter->main_query)
+			return NULL;
+		iter->main_query = iter->init = FALSE;
 		return fts_flatcurve_xapian_query_iter_next(iter);
 	}
 
-	iter->result->maybe = (iter->curr_query >= 0);
+	iter->result->maybe = !iter->main_query;
 	iter->result->score = iter->i.get_weight();
 	/* MSet docid can be an "interleaved" docid generated by
 	 * Xapian::Database when handling multiple DBs at once. Instead, we
@@ -1799,13 +1802,18 @@ bool fts_flatcurve_xapian_run_query(struct flatcurve_fts_query *query,
 	if ((iter = fts_flatcurve_xapian_query_iter_init(query)) == NULL)
 		return FALSE;
 	while ((result = fts_flatcurve_xapian_query_iter_next(iter)) != NULL) {
-		if (result->maybe || query->xapian->maybe)
+		bool add_score = TRUE;
+		if (result->maybe || query->xapian->maybe) {
+			add_score = (!seq_range_exists(&r->uids, result->uid) &&
+				!seq_range_exists(&r->maybe_uids, result->uid));
 			seq_range_array_add(&r->maybe_uids, result->uid);
-		else
+		} else
 			seq_range_array_add(&r->uids, result->uid);
-		score = array_append_space(&r->scores);
-		score->score = (float)result->score;
-		score->uid = result->uid;
+		if (add_score) {
+			score = array_append_space(&r->scores);
+			score->score = (float)result->score;
+			score->uid = result->uid;
+		}
 	}
 	fts_flatcurve_xapian_query_iter_deinit(&iter);
 	return TRUE;