Merge pull request #1991 from rmcrackan/cursor/repair-a-search-index-missing-books-011e

fix: repair a search index that is missing books (#1989)

Source/ApplicationServices/DbContexts.cs

@@ -132,6 +132,13 @@ public static class DbContexts
 		return context.GetDeletedLibraryBooks();
 	}
 
+	/// <summary>How many books belong in the search index. A row count only; no entities are loaded.</summary>
+	public static int GetIndexableBookCount()
+	{
+		using var context = GetContext();
+		return context.GetIndexableBookCount();
+	}
+
 	/// <summary>
 	/// How many <see cref="LibraryBook"/> rows are in the trash. A row count only; no entities are loaded,
 	/// so this is cheap enough to refresh whenever the library changes.

Source/ApplicationServices/SearchEngineCommands.cs

@@ -23,6 +23,7 @@ public static class SearchEngineCommands
 		lock (IndexLock)
 		{
 			var engine = new SearchEngine();
+			repairShortIndex(engine);
 			try
 			{
 				return func(engine);
@@ -40,6 +41,59 @@ public static class SearchEngineCommands
 			}
 		}
 	}
+
+	/// <summary>Set once the index has been measured against the library, so the check costs one query per run.</summary>
+	private static bool indexCounted;
+
+	/// <summary>
+	/// Rebuilds an index that holds fewer books than the library does.
+	/// <para>
+	/// A book the index never received is not merely unsearchable. Filtering intersects the grid with the
+	/// query's hits, so a positive term such as <c>Absent</c> cannot return it, while the negated <c>-Absent</c>
+	/// resolves to "every document in the index" and therefore drops it from the grid - which looks exactly
+	/// like the negated filter working correctly. Reported as issue #1989.
+	/// </para>
+	/// <para>
+	/// Nothing else notices: the index is only ever written as a whole, and <see cref="tryUpdate"/>
+	/// deliberately swallows a rebuild that fails so a bad index cannot fail a good scan. A short index
+	/// therefore stays short until something happens to change the library again.
+	/// </para>
+	/// </summary>
+	private static void repairShortIndex(SearchEngine engine)
+	{
+		if (indexCounted)
+			return;
+
+		// before the work, not after: a check that throws must not run on every query for the rest of the session
+		indexCounted = true;
+
+		try
+		{
+			var indexed = engine.GetIndexedBookCount();
+
+			// no index yet, or one too damaged to read. Both already have their own recovery.
+			if (indexed < 0)
+				return;
+
+			var expected = DbContexts.GetIndexableBookCount();
+
+			// more documents than books is stale rather than harmful: an extra document matches no grid row.
+			if (indexed >= expected)
+				return;
+
+			Log.Warning("The search index holds {Indexed} of {Expected} books. Rebuilding it: search cannot find the rest, and a negated filter hides them.", indexed, expected);
+
+			var failures = fullReIndex(engine);
+
+			if (failures > 0)
+				Log.Error("{Failures} book(s) could not be added to the search index. Search cannot find them, and a negated filter hides them.", failures);
+		}
+		catch (Exception ex)
+		{
+			// Searching with a short index still beats not searching at all.
+			Log.Error(ex, "Could not check the search index against the library.");
+		}
+	}
 	#endregion
 
 	public static event EventHandler? SearchEngineUpdated;
@@ -92,7 +146,7 @@ public static class SearchEngineCommands
 		}
 	}
 
-	public static void FullReIndex() => performSafeCommand(fullReIndex);
+	public static void FullReIndex() => performSafeCommand(e => fullReIndex(e));
 	public static void FullReIndex(List<LibraryBook> libraryBooks)
 		=> performSafeCommand(se => fullReIndex(se, libraryBooks.WithoutParents()));
 
@@ -147,13 +201,15 @@ public static class SearchEngineCommands
 		}
 	}
 
-	private static void fullReIndex(SearchEngine engine)
+	/// <returns>How many books could not be indexed.</returns>
+	private static int fullReIndex(SearchEngine engine)
 	{
 		var library = DbContexts.GetLibrary_Flat_NoTracking();
-		fullReIndex(engine, library);
+		return fullReIndex(engine, library);
 	}
 
-	private static void fullReIndex(SearchEngine engine, IEnumerable<LibraryBook> libraryBooks)
+	/// <returns>How many books could not be indexed.</returns>
+	private static int fullReIndex(SearchEngine engine, IEnumerable<LibraryBook> libraryBooks)
 	=> engine.CreateNewIndex(libraryBooks);
 	#endregion
 }

Source/DataLayer/QueryObjects/LibraryBookQueries.cs

@@ -69,6 +69,17 @@ public static class LibraryBookQueries
 			.getLibrary()
 			.ToList();
 
+		/// <summary>
+		/// How many books a full search re-index writes: the same rows as
+		/// <see cref="GetLibrary_Flat_NoTracking"/> without the podcast parents, which are stripped before
+		/// indexing. A row count only; no entities are loaded, so this is cheap enough to check the index against.
+		/// </summary>
+		public int GetIndexableBookCount()
+			=> context
+			.LibraryBooks
+			.AsNoTracking()
+			.Count(lb => !lb.IsDeleted && lb.Book.ContentType != ContentType.Parent);
+
 		/// <summary>Counts <see cref="LibraryBook"/> rows by <see cref="LibraryBook.IsDeleted"/> (no related entities loaded).</summary>
 		public (int NotInTrash, int InTrash) GetLibraryBookCountsByTrashFlag()
 		{

Source/LibationSearchEngine/SearchEngine.cs

@@ -88,7 +88,14 @@ public class SearchEngine
 
 	#region create and update index
 	/// <summary>create new. ie: full re-index</summary>
-	public void CreateNewIndex(IEnumerable<LibraryBook> library, bool overwrite = true)
+	/// <returns>How many books could not be indexed, and so cannot be found by search or filter.</returns>
+	/// <remarks>
+	/// One book is not allowed to cost the rest their place in the index. Disposing the writer commits
+	/// whatever it already holds, so letting a single bad book escape the loop used to leave a silently
+	/// truncated index behind: every book after the failure was missing from it, and a missing book is
+	/// worse than an unsearchable one, because a negated filter drops it from the grid entirely.
+	/// </remarks>
+	public int CreateNewIndex(IEnumerable<LibraryBook> library, bool overwrite = true)
 	{
 		var libraryList = library.ToList();
 
@@ -99,10 +106,58 @@ public class SearchEngine
 		using var analyzer = new StandardAnalyzer(Version);
 		using var ixWriter = openWriterForRebuild(index, analyzer, overwrite);
 
+		var failures = 0;
+
 		foreach (var libraryBook in libraryList)
 		{
-			var doc = createBookIndexDocument(libraryBook);
-			ixWriter.AddDocument(doc);
+			try
+			{
+				var doc = createBookIndexDocument(libraryBook);
+				ixWriter.AddDocument(doc);
+			}
+			catch (Exception ex)
+			{
+				failures++;
+				Serilog.Log.Logger.Error(ex, "Could not add {Book} to the search index. It will not be found by search or filter, and a negated filter will hide it.", describeForLog(libraryBook));
+			}
+		}
+
+		return failures;
+	}
+
+	/// <summary>Name a book for a log line without risking a second exception from the book that just threw.</summary>
+	private static string describeForLog(LibraryBook libraryBook)
+	{
+		try
+		{
+			return libraryBook?.Book?.AudibleProductId ?? "a book with no product id";
+		}
+		catch
+		{
+			return "an unreadable book";
+		}
+	}
+
+	/// <summary>
+	/// How many books the index holds, or -1 when there is no index to read. Every write to the index is a
+	/// full rebuild, so this should always equal the number of books a rebuild would write; when it is short,
+	/// the missing books are invisible to search and are hidden from the grid by any negated filter.
+	/// </summary>
+	public int GetIndexedBookCount()
+	{
+		try
+		{
+			using var index = getIndex();
+			if (!IndexReader.IndexExists(index))
+				return -1;
+
+			using var reader = IndexReader.Open(index, readOnly: true);
+			return reader.NumDocs();
+		}
+		catch (Exception ex)
+		{
+			Serilog.Log.Logger.Debug(ex, "Could not count the documents in the search index");
+			return -1;
 		}
 	}
 

Source/_Tests/LibationSearchEngine.Tests/IndexCompletenessTests.cs

@@ -0,0 +1,121 @@
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using AssertionHelper;
+using DataLayer;
+using LibationSearchEngine;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Directory = System.IO.Directory;
+
+namespace SearchEngineTests;
+
+/// <summary>
+/// A book the index does not hold is worse off than a book the index gets wrong. Filtering keeps the grid rows
+/// whose product id the query returned, so a missing book cannot be found by any positive term, and every
+/// negated term - which resolves to "every document in the index" - drops it from the grid instead. Reported as
+/// issue #1989, where <c>Absent</c> found nothing while <c>-Absent</c> appeared to work perfectly.
+/// </summary>
+[TestClass]
+public class IndexCompletenessTests
+{
+	private const string PRESENT = "B0PRESENT01";
+	private const string ABSENT = "B0ABSENT001";
+	private const string THIRD = "B0THIRD0001";
+
+	private string indexDirectory = null!;
+
+	[TestInitialize]
+	public void Initialize()
+	{
+		indexDirectory = Path.Combine(Path.GetTempPath(), "LibationSearchEngineTests", Guid.NewGuid().ToString("N"));
+		Directory.CreateDirectory(indexDirectory);
+	}
+
+	[TestCleanup]
+	public void Cleanup()
+	{
+		try
+		{
+			if (Directory.Exists(indexDirectory))
+				Directory.Delete(indexDirectory, recursive: true);
+		}
+		catch (IOException)
+		{
+			// Windows refuses to delete a file Lucene still holds open, and a leftover temp directory is
+			// not worth failing a test over
+		}
+	}
+
+	private static LibraryBook book(string asin, bool absentFromLastScan = false)
+	{
+		var contributor = Contributor.GetEmpty();
+		var b = new Book(new AudibleProductId(asin), $"Title {asin}", null, null, 1, ContentType.Product, [contributor], [contributor], "us");
+		return new LibraryBook(b, new DateTime(2026, 8, 15), "account") { AbsentFromLastScan = absentFromLastScan };
+	}
+
+	/// <summary>
+	/// EF materializes entities through the private parameterless constructor, so a <see cref="LibraryBook"/>
+	/// row whose <c>BookId</c> matches nothing in the Books table arrives with a null <see cref="LibraryBook.Book"/>.
+	/// <see cref="DtoImporterService.LibraryBookImporter"/> says as much where it marks those rows absent.
+	/// Every index rule reads through that property, so such a row throws while it is being indexed.
+	/// </summary>
+	private static LibraryBook bookThatCannotBeIndexed()
+		=> (LibraryBook)Activator.CreateInstance(typeof(LibraryBook), nonPublic: true)!;
+
+	private string[] search(string query)
+		=> [.. new SearchEngine(indexDirectory).Search(query).Docs.Select(d => d.ProductId).Order()];
+
+	[TestMethod]
+	public void an_unindexable_book_does_not_cost_the_books_after_it_their_place()
+	{
+		List<LibraryBook> library = [book(PRESENT), bookThatCannotBeIndexed(), book(ABSENT, absentFromLastScan: true), book(THIRD)];
+
+		var failures = new SearchEngine(indexDirectory).CreateNewIndex(library);
+
+		failures.Should().Be(1);
+		search("*:*").Should().BeEquivalentTo([PRESENT, ABSENT, THIRD]);
+		search("Absent").Should().BeEquivalentTo([ABSENT]);
+		search("-Absent").Should().BeEquivalentTo([PRESENT, THIRD]);
+	}
+
+	[TestMethod]
+	public void a_library_that_indexes_cleanly_reports_no_failures()
+		=> new SearchEngine(indexDirectory).CreateNewIndex([book(PRESENT), book(ABSENT)]).Should().Be(0);
+
+	[TestMethod]
+	public void the_indexed_book_count_is_minus_one_before_there_is_an_index()
+		=> new SearchEngine(indexDirectory).GetIndexedBookCount().Should().Be(-1);
+
+	[TestMethod]
+	public void the_indexed_book_count_is_what_the_index_holds()
+	{
+		var engine = new SearchEngine(indexDirectory);
+		engine.CreateNewIndex([book(PRESENT), book(ABSENT), book(THIRD)]);
+
+		engine.GetIndexedBookCount().Should().Be(3);
+	}
+
+	/// <summary>
+	/// The signature from issue #1989, and the reason a short index has to be repaired rather than tolerated:
+	/// the two halves disagree, so the filter looks half-broken instead of looking like a stale index.
+	/// </summary>
+	[TestMethod]
+	public void a_book_the_index_never_received_reads_as_a_half_working_filter()
+	{
+		// the absent book is in the library but never made it into the index
+		new SearchEngine(indexDirectory).CreateNewIndex([book(PRESENT), book(THIRD)]);
+
+		// nothing to show, even though the library has an absent book
+		search("Absent").Should().BeEquivalentTo([]);
+
+		// and the negation quietly leaves it out, which is indistinguishable from working
+		search("-Absent").Should().BeEquivalentTo([PRESENT, THIRD]);
+
+		// once the index holds the whole library, both halves agree
+		new SearchEngine(indexDirectory).CreateNewIndex([book(PRESENT), book(ABSENT, absentFromLastScan: true), book(THIRD)]);
+
+		search("Absent").Should().BeEquivalentTo([ABSENT]);
+		search("-Absent").Should().BeEquivalentTo([PRESENT, THIRD]);
+	}
+}