From 1bcc5d7181bc050979700e97683614f4238bbb38 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Mon, 14 Sep 2026 11:22:19 -0400 Subject: [PATCH 1/5] feat: split Studio search into separate course and library indexes Library authoring waits synchronously on Meilisearch, and indexing cost grows with index size. On large instances the shared studio_content index is almost entirely course blocks, so creating a library component can time out (#38993). Course content is indexed asynchronously and doesn't need to share an index with libraries. Course blocks stay in studio_content so existing course documents don't need reindexing. Libraries V2 blocks, containers and collections move to a new studio_library_content index. Writes are routed by document type or key context, rebuild locks and _new temp indexes are per index, and reconcile creates/configures both. The studio search endpoint now returns course_index_name and library_index_name with one tenant token covering both indexes. index_name is kept for one release, pointing at the course index. Existing installs run `reindex_studio --libraries-only` once after migrate. It rebuilds the library index and deletes `type != "course_block"` documents from the course index without reindexing courses. Full rebuilds run the same cleanup. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01J43t1WXNsmVV5mbdzxv6iT --- openedx/core/djangoapps/content/search/api.py | 291 ++++++++++++------ ...02-separate-course-and-library-indexes.rst | 78 +++++ .../management/commands/reindex_studio.py | 26 +- .../core/djangoapps/content/search/tasks.py | 25 ++ .../content/search/tests/test_api.py | 189 ++++++++++-- .../content/search/tests/test_handlers.py | 7 + .../content/search/tests/test_reconcile.py | 90 +++++- .../content/search/tests/test_reindex_cmd.py | 51 ++- .../content/search/tests/test_views.py | 57 ++-- 9 files changed, 639 insertions(+), 175 deletions(-) create mode 100644 openedx/core/djangoapps/content/search/docs/decisions/0002-separate-course-and-library-indexes.rst diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 6d6ce6148cd2..bf7f4ade1ac3 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -21,7 +21,7 @@ from meilisearch.errors import MeilisearchApiError, MeilisearchError from meilisearch.models.task import TaskInfo from opaque_keys import OpaqueKey -from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.keys import CourseKey, LearningContextKey, UsageKey from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2 from openedx_content import api as content_api from openedx_content import models_api as content_models @@ -44,6 +44,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError from .documents import ( + DocType, Fields, meili_id_from_opaque_key, searchable_doc_collections, @@ -60,14 +61,19 @@ User = get_user_model() -STUDIO_INDEX_SUFFIX = "studio_content" +# Course content and library content live in separate indexes. Library authoring waits synchronously on +# Meilisearch, and indexing cost grows with index size, so library content gets a small index of its own. +# The course index keeps the original "studio_content" name so existing course documents don't need reindexing. +STUDIO_COURSE_INDEX_SUFFIX = "studio_content" +STUDIO_LIBRARY_INDEX_SUFFIX = "studio_library_content" Filter = str | list[str | list[str]] -if hasattr(settings, "MEILISEARCH_INDEX_PREFIX"): - STUDIO_INDEX_NAME = settings.MEILISEARCH_INDEX_PREFIX + STUDIO_INDEX_SUFFIX -else: - STUDIO_INDEX_NAME = STUDIO_INDEX_SUFFIX +_INDEX_PREFIX = getattr(settings, "MEILISEARCH_INDEX_PREFIX", "") +# Holds DocType.course_block documents (courses and legacy modulestore libraries). +STUDIO_COURSE_INDEX_NAME = _INDEX_PREFIX + STUDIO_COURSE_INDEX_SUFFIX +# Holds DocType.library_block, DocType.library_container and DocType.collection documents (Libraries V2). +STUDIO_LIBRARY_INDEX_NAME = _INDEX_PREFIX + STUDIO_LIBRARY_INDEX_SUFFIX _MEILI_CLIENT = None @@ -81,13 +87,25 @@ EXCLUDED_XBLOCK_TYPES = ["course", "course_info"] +def _index_name_for_key(key: OpaqueKey) -> str: + """ + Return the name of the index that holds the document(s) for the given key. + + Accepts a learning context key (course/library) or any key inside one (block, collection, container). + """ + context_key = key if isinstance(key, LearningContextKey) else key.context_key + if isinstance(context_key, LibraryLocatorV2): + return STUDIO_LIBRARY_INDEX_NAME + return STUDIO_COURSE_INDEX_NAME + + @contextmanager -def _index_rebuild_lock() -> Generator[str, None, None]: +def _index_rebuild_lock(index_name: str) -> Generator[str, None, None]: """ - Lock to prevent that more than one rebuild is running at the same time + Lock to prevent that more than one rebuild of the given index is running at the same time """ - lock_id = f"lock-meilisearch-index-{STUDIO_INDEX_NAME}" - new_index_name = STUDIO_INDEX_NAME + "_new" + lock_id = f"lock-meilisearch-index-{index_name}" + new_index_name = index_name + "_new" status = cache.add(lock_id, new_index_name, LOCK_EXPIRE) @@ -103,8 +121,8 @@ def _index_rebuild_lock() -> Generator[str, None, None]: cache.delete(lock_id) -def _get_running_rebuild_index_name() -> str | None: - lock_id = f"lock-meilisearch-index-{STUDIO_INDEX_NAME}" +def _get_running_rebuild_index_name(index_name: str) -> str | None: + lock_id = f"lock-meilisearch-index-{index_name}" return cache.get(lock_id) @@ -192,20 +210,24 @@ def _index_exists(index_name: str) -> bool: @contextmanager -def _using_temp_index(status_cb: Callable[[str], None] | None = None) -> Generator[str, None, None]: +def _using_temp_index( + index_name: str, + status_cb: Callable[[str], None] | None = None, +) -> Generator[str, None, None]: """ Create a new temporary Meilisearch index, populate it, then swap it to become the active index. Args: + index_name (str): The index that the temporary index will replace status_cb (Callable): A callback function to report status messages """ if status_cb is None: status_cb = log.info client = _get_meilisearch_client() - status_cb("Checking index...") - with _index_rebuild_lock() as temp_index_name: + status_cb(f"Checking index '{index_name}'...") + with _index_rebuild_lock(index_name) as temp_index_name: if _index_exists(temp_index_name): status_cb("Temporary index already exists. Deleting it...") _wait_for_meili_task(client.delete_index(temp_index_name)) @@ -216,17 +238,17 @@ def _using_temp_index(status_cb: Callable[[str], None] | None = None) -> Generat yield temp_index_name - if not _index_exists(STUDIO_INDEX_NAME): + if not _index_exists(index_name): # We have to create the "target" index before we can successfully swap the new one into it: status_cb("Preparing to swap into index (first time)...") - _wait_for_meili_task(client.create_index(STUDIO_INDEX_NAME)) + _wait_for_meili_task(client.create_index(index_name)) status_cb("Swapping index...") - client.swap_indexes([{"indexes": [temp_index_name, STUDIO_INDEX_NAME]}]) + client.swap_indexes([{"indexes": [temp_index_name, index_name]}]) # If we're using an API key that's restricted to certain index prefix(es), we won't be able to get the status # of this request unfortunately. https://github.com/meilisearch/meilisearch/issues/4103 while True: time.sleep(1) - if client.get_index(STUDIO_INDEX_NAME).created_at != new_index_created: + if client.get_index(index_name).created_at != new_index_created: status_cb("Waiting for swap completion...") else: break @@ -313,22 +335,22 @@ def _recurse_children(block, fn, status_cb: Callable[[str], None] | None = None) fn(child) -def _update_index_docs(docs) -> None: +def _update_index_docs(index_name: str, docs) -> None: """ - Helper function that updates the documents in the search index + Helper function that updates the documents in the given search index - If there is a rebuild in progress, the document will also be added to the new index. + If there is a rebuild of that index in progress, the document will also be added to the new index. """ if not docs: return client = _get_meilisearch_client() - current_rebuild_index_name = _get_running_rebuild_index_name() + current_rebuild_index_name = _get_running_rebuild_index_name(index_name) if current_rebuild_index_name: # If there is a rebuild in progress, the document will also be added to the new index. client.index(current_rebuild_index_name).update_documents(docs) - _wait_for_meili_task(client.index(STUDIO_INDEX_NAME).update_documents(docs)) + _wait_for_meili_task(client.index(index_name).update_documents(docs)) def only_if_meilisearch_enabled(f): @@ -355,15 +377,15 @@ def is_meilisearch_enabled() -> bool: return False -def reset_index(status_cb: Callable[[str], None] | None = None) -> None: +def reset_index(index_name: str, status_cb: Callable[[str], None] | None = None) -> None: """ - Reset the Meilisearch index, deleting all documents and reconfiguring it + Reset the given Meilisearch index, deleting all documents and reconfiguring it """ if status_cb is None: status_cb = log.info - status_cb("Creating new empty index...") - with _using_temp_index(status_cb) as temp_index_name: + status_cb(f"Creating new empty index '{index_name}'...") + with _using_temp_index(index_name, status_cb) as temp_index_name: _apply_index_settings(temp_index_name, wait=False) status_cb("Index recreated!") status_cb("Index reset complete.") @@ -452,9 +474,9 @@ def reconcile_index( status_cb: Callable[[str], None] | None = None, warn_cb: Callable[[str], None] | None = None ) -> None: # noqa: E501 """ - Reconcile the Meilisearch index state. + Reconcile the state of the Studio course index and library index. - Inspects the current Studio Meilisearch index and takes appropriate action based on its state: + Inspects each Studio Meilisearch index and takes appropriate action based on its state: - Creates the index if missing. - Reconfigures if empty and drifted. - Applies updated settings if populated and drifted. @@ -468,44 +490,61 @@ def reconcile_index( if warn_cb is None: warn_cb = log.warning - drift = _detect_index_drift(STUDIO_INDEX_NAME) + for index_name in (STUDIO_COURSE_INDEX_NAME, STUDIO_LIBRARY_INDEX_NAME): + _reconcile_single_index(index_name, status_cb, warn_cb) + + +def _reconcile_single_index( + index_name: str, + status_cb: Callable[[str], None], + warn_cb: Callable[[str], None], +) -> None: + """ + Reconcile the state of one Studio Meilisearch index. See reconcile_index(). + """ + if index_name == STUDIO_LIBRARY_INDEX_NAME: + populate_cmd = "./manage.py cms reindex_studio --libraries-only" + else: + populate_cmd = "./manage.py cms reindex_studio" + + drift = _detect_index_drift(index_name) # CASE: Index missing if not drift.exists: - status_cb("Studio search index not found. Creating and configuring...") - reset_index(status_cb) - status_cb("Index created. Run './manage.py cms reindex_studio' to populate.") + status_cb(f"Studio search index '{index_name}' not found. Creating and configuring...") + reset_index(index_name, status_cb) + status_cb(f"Index '{index_name}' created. Run '{populate_cmd}' to populate.") return # CASE: Primary key mismatch (must recreate regardless of population state) if not drift.primary_key_correct: if drift.is_empty: - warn_cb("Primary key mismatch on empty index. Recreating...") + warn_cb(f"Primary key mismatch on empty index '{index_name}'. Recreating...") else: warn_cb( - f"PRIMARY KEY MISMATCH on populated index '{STUDIO_INDEX_NAME}'. " + f"PRIMARY KEY MISMATCH on populated index '{index_name}'. " "Index must be recreated (data loss is unavoidable for primary key changes)." ) - warn_cb("Dropping and recreating index. Repopulate with: './manage.py cms reindex_studio'") - reset_index(status_cb) - warn_cb("Index recreated empty. Run './manage.py cms reindex_studio' to repopulate.") + warn_cb(f"Dropping and recreating index. Repopulate with: '{populate_cmd}'") + reset_index(index_name, status_cb) + warn_cb(f"Index '{index_name}' recreated empty. Run '{populate_cmd}' to repopulate.") return # CASE: Index empty if drift.is_empty: if drift.is_settings_drifted: - status_cb("Empty index has drifted settings. Reconfiguring...") - _apply_index_settings(STUDIO_INDEX_NAME, wait=True, status_cb=status_cb) - status_cb("Reconfigured. Run './manage.py cms reindex_studio' to populate.") + status_cb(f"Empty index '{index_name}' has drifted settings. Reconfiguring...") + _apply_index_settings(index_name, wait=True, status_cb=status_cb) + status_cb(f"Reconfigured. Run '{populate_cmd}' to populate.") else: status_cb( - "Index exists and is correctly configured but empty. Run './manage.py cms reindex_studio' to populate." + f"Index '{index_name}' exists and is correctly configured but empty. Run '{populate_cmd}' to populate." ) return # CASE: Index populated, attribute drifted i.e settings mismatched if drift.is_settings_drifted: - warn_cb(f"Settings drift detected on populated index '{STUDIO_INDEX_NAME}'. Applying updated settings...") + warn_cb(f"Settings drift detected on populated index '{index_name}'. Applying updated settings...") # Log per-setting mismatch details for field_name, match in ( ("distinctAttribute", drift.distinct_attribute_match), @@ -517,14 +556,14 @@ def reconcile_index( if match is False: warn_cb(f" - {field_name}: DRIFTED") - _apply_index_settings(STUDIO_INDEX_NAME, wait=True, status_cb=status_cb) + _apply_index_settings(index_name, wait=True, status_cb=status_cb) warn_cb( "Settings applied. Meilisearch will re-index documents in the background. " - "Consider running './manage.py cms reindex_studio' for a full rebuild " + f"Consider running '{populate_cmd}' for a full rebuild " "if search quality is affected." ) else: - status_cb("Index is populated and correctly configured. No action needed.") + status_cb(f"Index '{index_name}' is populated and correctly configured. No action needed.") def init_index(status_cb: Callable[[str], None] | None = None, warn_cb: Callable[[str], None] | None = None) -> None: @@ -551,7 +590,7 @@ def index_course( client = _get_meilisearch_client() docs = [] if index_name is None: - index_name = STUDIO_INDEX_NAME + index_name = STUDIO_COURSE_INDEX_NAME if status_cb is None: status_cb = log.info @@ -579,10 +618,16 @@ def add_with_children(block): def rebuild_index( # pylint: disable=too-many-statements - status_cb: Callable[[str], None] | None = None, incremental=False + status_cb: Callable[[str], None] | None = None, incremental=False, include_courses=True ) -> None: """ - Rebuild the Meilisearch index from scratch + Rebuild the Meilisearch indexes from scratch + + Libraries go into the library index, courses into the course index. Each index is swapped in + separately, so the library index is live as soon as the (fast) library pass finishes. + + With include_courses=False only the library index is rebuilt. This is how an existing install + populates the library index when upgrading from a single shared index without reindexing courses. """ if status_cb is None: status_cb = log.info @@ -605,7 +650,7 @@ def rebuild_index( # pylint: disable=too-many-statements # Get the list of courses status_cb("Counting courses...") - num_courses = CourseOverview.objects.count() + num_courses = CourseOverview.objects.count() if include_courses else 0 # Some counters so we can track our progress as indexing progresses: num_libs_skipped = len(keys_indexed) @@ -614,7 +659,11 @@ def rebuild_index( # pylint: disable=too-many-statements num_blocks_done = 0 # How many individual components/XBlocks we've indexed status_cb(f"Found {num_courses} courses, {num_libraries} libraries.") - with _using_temp_index(status_cb) if not incremental else nullcontext(STUDIO_INDEX_NAME) as index_name: + library_index_ctx = ( + nullcontext(STUDIO_LIBRARY_INDEX_NAME) if incremental + else _using_temp_index(STUDIO_LIBRARY_INDEX_NAME, status_cb) + ) + with library_index_ctx as index_name: ############## Configure the index ############## # The index settings are best changed on an empty index. @@ -741,24 +790,37 @@ def index_container_batch(batch, num_done, library_key) -> int: num_contexts_done += 1 - ############## Courses ############## - status_cb("Indexing courses...") - # To reduce memory usage on large instances, split up the CourseOverviews into pages of 1,000 courses: + # Library content used to be stored in the course index. Now that the library index has it, drop the old copies. + delete_library_docs_from_course_index(status_cb) - paginator = Paginator(CourseOverview.objects.only("id", "display_name").order_by("-created", "id"), 1000) - for p in paginator.page_range: - for course in paginator.page(p).object_list: - status_cb( - f"{num_contexts_done + 1}/{num_contexts}. Now indexing course {course.display_name} ({course.id})" - ) - if course.id in keys_indexed: + if include_courses: + course_index_ctx = ( + nullcontext(STUDIO_COURSE_INDEX_NAME) if incremental + else _using_temp_index(STUDIO_COURSE_INDEX_NAME, status_cb) + ) + with course_index_ctx as index_name: + if not incremental: + _apply_index_settings(index_name, wait=False) + + ############## Courses ############## + status_cb("Indexing courses...") + # To reduce memory usage on large instances, split up the CourseOverviews into pages of 1,000 courses: + + paginator = Paginator(CourseOverview.objects.only("id", "display_name").order_by("-created", "id"), 1000) + for p in paginator.page_range: + for course in paginator.page(p).object_list: + status_cb( + f"{num_contexts_done + 1}/{num_contexts}. " + f"Now indexing course {course.display_name} ({course.id})" + ) + if course.id in keys_indexed: + num_contexts_done += 1 + continue + course_docs = index_course(course.id, index_name, status_cb) + if incremental: + IncrementalIndexCompleted.objects.get_or_create(context_key=course.id) num_contexts_done += 1 - continue - course_docs = index_course(course.id, index_name, status_cb) - if incremental: - IncrementalIndexCompleted.objects.get_or_create(context_key=course.id) - num_contexts_done += 1 - num_blocks_done += len(course_docs) + num_blocks_done += len(course_docs) IncrementalIndexCompleted.objects.all().delete() status_cb(f"Done! {num_blocks_done} blocks indexed across {num_contexts_done} courses, collections and libraries.") @@ -790,7 +852,7 @@ def add_with_children(block): add_with_children(xblock) - _update_index_docs(docs) + _update_index_docs(STUDIO_COURSE_INDEX_NAME, docs) def delete_index_doc(key: OpaqueKey, *, delete_children: bool = False) -> None: @@ -800,55 +862,77 @@ def delete_index_doc(key: OpaqueKey, *, delete_children: bool = False) -> None: Args: key (OpaqueKey): The opaque key of the XBlock/Container to be removed from the index """ + index_name = _index_name_for_key(key) doc = searchable_doc_for_key(key) - _delete_index_doc(doc[Fields.id]) + _delete_index_doc(index_name, doc[Fields.id]) if delete_children: - _delete_documents(f'{Fields.breadcrumbs}.{Fields.usage_key} = "{key}"') + _delete_documents(index_name, f'{Fields.breadcrumbs}.{Fields.usage_key} = "{key}"') def delete_docs_with_context_key(key: OpaqueKey) -> None: """ Delete all docs for given context key """ - _delete_documents(f'{Fields.context_key} = "{key}"') + _delete_documents(_index_name_for_key(key), f'{Fields.context_key} = "{key}"') + + +def delete_library_docs_from_course_index(status_cb: Callable[[str], None] | None = None) -> None: + """ + Delete every non-course document from the course index. + + Before the course/library index split, library blocks, containers and collections were stored in the + course index. This removes those leftovers. It is a no-op once they are gone. + """ + if status_cb is None: + status_cb = log.info + + if not _index_exists(STUDIO_COURSE_INDEX_NAME): + return + status_cb(f"Removing library documents from course index '{STUDIO_COURSE_INDEX_NAME}'...") + _wait_for_meili_task( + _get_meilisearch_client().index(STUDIO_COURSE_INDEX_NAME).delete_documents( + filter=f'{Fields.type} != "{DocType.course_block}"' + ) + ) -def _delete_documents(filter_query: str) -> None: +def _delete_documents(index_name: str, filter_query: str) -> None: """ - Deletes all documents from the search index that match the given filter + Deletes all documents from the given search index that match the given filter Args: - filter (str): The query to use when filtering documents + index_name (str): The index to delete the documents from + filter_query (str): The query to use when filtering documents """ if not filter_query: return client = _get_meilisearch_client() - current_rebuild_index_name = _get_running_rebuild_index_name() + current_rebuild_index_name = _get_running_rebuild_index_name(index_name) if current_rebuild_index_name: # If there is a rebuild in progress, the document will also be removed from the new index. client.index(current_rebuild_index_name).delete_documents(filter=filter_query) - _wait_for_meili_task(client.index(STUDIO_INDEX_NAME).delete_documents(filter=filter_query)) + _wait_for_meili_task(client.index(index_name).delete_documents(filter=filter_query)) -def _delete_index_doc(doc_id) -> None: +def _delete_index_doc(index_name: str, doc_id) -> None: """ - Helper function that deletes the document with the given ID from the search index + Helper function that deletes the document with the given ID from the given search index - If there is a rebuild in progress, the document will also be removed from the new index. + If there is a rebuild of that index in progress, the document will also be removed from the new index. """ if not doc_id: return client = _get_meilisearch_client() - current_rebuild_index_name = _get_running_rebuild_index_name() + current_rebuild_index_name = _get_running_rebuild_index_name(index_name) if current_rebuild_index_name: # If there is a rebuild in progress, the document will also be removed from the new index. client.index(current_rebuild_index_name).delete_document(doc_id) - _wait_for_meili_task(client.index(STUDIO_INDEX_NAME).delete_document(doc_id)) + _wait_for_meili_task(client.index(index_name).delete_document(doc_id)) def upsert_library_block_index_doc(usage_key: UsageKey) -> None: @@ -861,10 +945,10 @@ def upsert_library_block_index_doc(usage_key: UsageKey) -> None: docs = [searchable_doc_for_library_block(library_block_metadata)] - _update_index_docs(docs) + _update_index_docs(STUDIO_LIBRARY_INDEX_NAME, docs) -def _get_document_from_index(document_id: str) -> dict: +def _get_document_from_index(index_name: str, document_id: str) -> dict: """ Returns the Document identified by the given ID, from the given index. @@ -872,7 +956,6 @@ def _get_document_from_index(document_id: str) -> dict: """ client = _get_meilisearch_client() document = None - index_name = STUDIO_INDEX_NAME try: index = client.get_index(index_name) document = index.get_document(document_id) @@ -894,11 +977,11 @@ def upsert_library_collection_index_doc(collection_key: LibraryCollectionLocator # (If the collection is soft-deleted, searchable_doc_for_collection() sets `_disabled: True`) # (If the collection is hard-deleted, searchable_doc_for_collection() leaves all fields other than ID empty) if doc.get("_disabled") or not doc.get(Fields.type): - _delete_index_doc(doc[Fields.id]) + _delete_index_doc(STUDIO_LIBRARY_INDEX_NAME, doc[Fields.id]) return # Normal case - update the collection doc. - _update_index_docs([doc]) + _update_index_docs(STUDIO_LIBRARY_INDEX_NAME, [doc]) # We do NOT update the individual entities (components/containers) in the collection here. # This event can be called if a single entity is added or removed from the collection (to update the "# of items in @@ -945,7 +1028,7 @@ def update_library_components_collections( log.info( f"Updating document.collections for library {library_key} components page {page} / {paginator.num_pages}" ) - _update_index_docs(docs) + _update_index_docs(STUDIO_LIBRARY_INDEX_NAME, docs) def update_library_containers_collections( @@ -984,7 +1067,7 @@ def update_library_containers_collections( log.info( f"Updating document.collections for library {library_key} containers page {page} / {paginator.num_pages}" ) - _update_index_docs(docs) + _update_index_docs(STUDIO_LIBRARY_INDEX_NAME, docs) def upsert_library_container_index_doc(container_key: LibraryContainerLocator) -> None: @@ -998,15 +1081,15 @@ def upsert_library_container_index_doc(container_key: LibraryContainerLocator) - # Soft-deleted/disabled containers are removed from the index # and their components updated. if doc.get("_disabled"): - _delete_index_doc(doc[Fields.id]) + _delete_index_doc(STUDIO_LIBRARY_INDEX_NAME, doc[Fields.id]) # Hard-deleted containers are also deleted from the index elif not doc.get(Fields.type): - _delete_index_doc(doc[Fields.id]) + _delete_index_doc(STUDIO_LIBRARY_INDEX_NAME, doc[Fields.id]) # Otherwise, upsert the container. else: - _update_index_docs([doc]) + _update_index_docs(STUDIO_LIBRARY_INDEX_NAME, [doc]) def upsert_content_library_index_docs(library_key: LibraryLocatorV2, full_index: bool = False) -> None: @@ -1034,7 +1117,7 @@ def upsert_content_library_index_docs(library_key: LibraryLocatorV2, full_index: doc = searchable_doc_for_collection(collection_key, collection=collection) docs.append(doc) - _update_index_docs(docs) + _update_index_docs(STUDIO_LIBRARY_INDEX_NAME, docs) def upsert_content_object_tags_index_doc(key: OpaqueKey): @@ -1043,7 +1126,7 @@ def upsert_content_object_tags_index_doc(key: OpaqueKey): """ doc = {Fields.id: meili_id_from_opaque_key(key)} doc.update(searchable_doc_tags(key)) - _update_index_docs([doc]) + _update_index_docs(_index_name_for_key(key), [doc]) def upsert_item_collections_index_docs(opaque_key: OpaqueKey): @@ -1052,7 +1135,7 @@ def upsert_item_collections_index_docs(opaque_key: OpaqueKey): """ doc = {Fields.id: meili_id_from_opaque_key(opaque_key)} doc.update(searchable_doc_collections(opaque_key)) - _update_index_docs([doc]) + _update_index_docs(_index_name_for_key(opaque_key), [doc]) def upsert_item_containers_index_docs(opaque_key: OpaqueKey, container_type: str): @@ -1061,7 +1144,7 @@ def upsert_item_containers_index_docs(opaque_key: OpaqueKey, container_type: str """ doc = {Fields.id: meili_id_from_opaque_key(opaque_key)} doc.update(searchable_doc_containers(opaque_key, container_type)) - _update_index_docs([doc]) + _update_index_docs(_index_name_for_key(opaque_key), [doc]) def _get_user_orgs(request: Request) -> list[str]: @@ -1100,8 +1183,10 @@ def generate_user_token_for_studio_search(request): """ expires_at = datetime.now(tz=timezone.utc) + timedelta(days=7) # noqa: UP017 + access_filter = _get_meili_access_filter(request) search_rules = { - STUDIO_INDEX_NAME: _get_meili_access_filter(request), + STUDIO_COURSE_INDEX_NAME: access_filter, + STUDIO_LIBRARY_INDEX_NAME: access_filter, } # Note: the following is just generating a JWT. It doesn't actually make an API call to Meilisearch. restricted_api_key = _get_meilisearch_client().generate_tenant_token( @@ -1112,7 +1197,11 @@ def generate_user_token_for_studio_search(request): return { "url": settings.MEILISEARCH_PUBLIC_URL, - "index_name": STUDIO_INDEX_NAME, + "course_index_name": STUDIO_COURSE_INDEX_NAME, + "library_index_name": STUDIO_LIBRARY_INDEX_NAME, + # Deprecated: use course_index_name / library_index_name. Kept for one release so frontends that predate + # the course/library index split keep working for course search. Remove in the following release. + "index_name": STUDIO_COURSE_INDEX_NAME, "api_key": restricted_api_key, } @@ -1158,7 +1247,8 @@ def fetch_block_types(extra_filter: Filter | None = None): extra_filter_formatted = force_array(extra_filter) client = _get_meilisearch_client() - index = client.get_index(STUDIO_INDEX_NAME) + # Only used for modulestore content (courses and legacy libraries), which lives in the course index. + index = client.get_index(STUDIO_COURSE_INDEX_NAME) response = index.search( "", @@ -1189,7 +1279,8 @@ def get_all_blocks_from_context( offset = 0 client = _get_meilisearch_client() - index = client.get_index(STUDIO_INDEX_NAME) + # Only used for modulestore content (courses and legacy libraries), which lives in the course index. + index = client.get_index(STUDIO_COURSE_INDEX_NAME) while True: response = index.search( diff --git a/openedx/core/djangoapps/content/search/docs/decisions/0002-separate-course-and-library-indexes.rst b/openedx/core/djangoapps/content/search/docs/decisions/0002-separate-course-and-library-indexes.rst new file mode 100644 index 000000000000..dcaac2408a5d --- /dev/null +++ b/openedx/core/djangoapps/content/search/docs/decisions/0002-separate-course-and-library-indexes.rst @@ -0,0 +1,78 @@ +Separate Meilisearch Indexes for Course and Library Content +############################################################ + +Status +****** + +Accepted + + +Context +******* + +`0001-meilisearch.rst`_ put all Studio content into one Meilisearch index, +``studio_content``: course blocks (``type = course_block``) plus Libraries V2 +components, containers and collections. + +Library authoring updates that index synchronously so the frontend sees the +change on its next query. Meilisearch's per-document indexing cost grows with +index size, and course blocks dominate the index on large instances. One +instance reported 1.3 million documents (11.9 GiB) of which 99.93% were course +blocks, and saw Studio time out when creating library components +(`openedx-platform#38993`_). + +Course content is indexed asynchronously by Celery tasks, so it does not need to +share an index with library content. + +.. _0001-meilisearch.rst: ./0001-meilisearch.rst +.. _openedx-platform#38993: https://github.com/openedx/openedx-platform/issues/38993 + + +Decision +******** + +Studio content is stored in two indexes: + +* ``studio_content``: course blocks + (``DocType.course_block``). This is the original index name, so upgrading does + not require reindexing course content. +* ``studio_library_content``: library blocks, library + containers and collections. + +Both indexes use the same settings. Every write is routed by document type or by +the learning context of its key. Rebuild locks and temporary ``_new`` indexes +are per index. + +``GET /api/content_search/v2/studio/`` returns ``course_index_name`` and +``library_index_name``, and one tenant token whose search rules cover both +indexes with the same access filter. The frontend uses a multi-index search for +views that span courses and libraries. ``index_name`` is still returned (equal +to ``course_index_name``) for one release, for frontends that predate the split. + + +Upgrading +********* + +On an instance that already has a populated ``studio_content`` index: + +#. ``./manage.py cms migrate``. The ``post_migrate`` reconciliation creates and + configures the empty library index. +#. ``./manage.py cms reindex_studio --libraries-only``. This enqueues a Celery + task that rebuilds the library index, then deletes every document with + ``type != "course_block"`` from the course index. Courses are not reindexed. + +Deploy a frontend that reads ``library_index_name`` at the same time. A frontend +that only knows ``index_name`` searches the course index, so it stops finding +library content once step 2 has run. + +A full ``./manage.py cms reindex_studio`` also populates both indexes and runs +the same cleanup, but it reindexes every course. + + +Consequences +************ + +* Library writes no longer pay for the size of the course index. +* Searches that span courses and libraries need a multi-index search request. +* The Meilisearch API key used by Studio must be allowed to manage both index + names (and their ``_new`` temporary indexes). diff --git a/openedx/core/djangoapps/content/search/management/commands/reindex_studio.py b/openedx/core/djangoapps/content/search/management/commands/reindex_studio.py index a1d7318f16c0..10d9b60dc61b 100644 --- a/openedx/core/djangoapps/content/search/management/commands/reindex_studio.py +++ b/openedx/core/djangoapps/content/search/management/commands/reindex_studio.py @@ -15,7 +15,7 @@ from django.core.management import BaseCommand, CommandError from ... import api -from ...tasks import rebuild_index_incremental +from ...tasks import rebuild_index_incremental, rebuild_library_index log = logging.getLogger(__name__) @@ -37,11 +37,24 @@ class Command(BaseCommand): ./manage.py cms shell -c 'IncrementalIndexCompleted.objects.all().delete()' This will delete all the IncrementalIndexCompleted records and will help in restarting the index population. + + When upgrading from the single shared Studio index to separate course and library indexes, run + `./manage.py cms reindex_studio --libraries-only` once instead. It rebuilds only the library index and + deletes library documents from the course index, without reindexing courses. """ help = "Add all course and library content to the Studio search index." def add_arguments(self, parser): + parser.add_argument( + "--libraries-only", + action="store_true", + default=False, + help=( + "Rebuild only the library index, then delete library documents from the course index. " + "Run this once when upgrading from the single shared Studio index; courses are not reindexed." + ), + ) # Removed flags — provide clear error messages for operators with old automation. parser.add_argument( "--experimental", @@ -97,6 +110,17 @@ def handle(self, *args, **options): "reindex_studio is now a stable command, so the flag is no longer necessary." ) + if options["libraries_only"]: + result = rebuild_library_index.delay() + if settings.CELERY_ALWAYS_EAGER: + self.stdout.write("Library indexing complete!") + else: + self.stdout.write( + f"Studio library index rebuild has been queued (task_id={result.id}). " + "Monitor progress in Celery worker logs." + ) + return + result = rebuild_index_incremental.delay() if settings.CELERY_ALWAYS_EAGER: diff --git a/openedx/core/djangoapps/content/search/tasks.py b/openedx/core/djangoapps/content/search/tasks.py index 2245cc85fa76..3a94bd362129 100644 --- a/openedx/core/djangoapps/content/search/tasks.py +++ b/openedx/core/djangoapps/content/search/tasks.py @@ -203,3 +203,28 @@ def rebuild_index_incremental() -> None: raise log.info("Incremental Studio search index population complete.") + + +@shared_task( + base=LoggedTask, + autoretry_for=(MeilisearchError, ConnectionError), + max_retries=3, + retry_backoff=True, +) +def rebuild_library_index() -> None: + """ + Celery task to rebuild the Studio library index and remove library documents from the course index. + + Run once when upgrading from the single shared index. Courses are not reindexed. + """ + log.info("Starting Studio library index rebuild...") + + try: + api.rebuild_index(status_cb=log.info, include_courses=False) + except RuntimeError as exc: + if "already in progress" in str(exc).lower(): + log.warning("Studio library index rebuild skipped: a rebuild is already in progress.") + return + raise + + log.info("Studio library index rebuild complete.") diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 3fd6859cafd8..4a123f857d7c 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -4,6 +4,7 @@ from __future__ import annotations import copy +from collections import defaultdict from datetime import UTC, datetime from unittest.mock import MagicMock, Mock, call, patch @@ -12,8 +13,8 @@ from django.test import override_settings from freezegun import freeze_time from meilisearch.errors import MeilisearchApiError -from opaque_keys.edx.keys import UsageKey -from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator +from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2 from openedx_content import api as content_api from openedx_content import models_api as content_models from organizations.tests.factories import OrganizationFactory @@ -337,6 +338,20 @@ def tearDown(self): content_models.Container.reset_cache() return super().tearDown() + def _mock_indexes(self, mock_meilisearch) -> defaultdict: + """ + Give each index name its own mock, so tests can assert which index a call went to. + """ + indexes = defaultdict(MagicMock) + mock_meilisearch.return_value.index.side_effect = lambda name: indexes[name] + return indexes + + def _indexes_used(self, mock_meilisearch) -> set[str]: + """ + Names of all indexes that were written to via client.index(name). + """ + return {c.args[0] for c in mock_meilisearch.return_value.index.call_args_list} + @override_settings(MEILISEARCH_ENABLED=False) def test_reindex_meilisearch_disabled(self, mock_meilisearch) -> None: with self.assertRaises(RuntimeError): # noqa: PT027 @@ -374,17 +389,51 @@ def test_reindex_meilisearch(self, mock_meilisearch) -> None: doc_section["tags"] = copy.deepcopy(EMPTY_TAGS) doc_section["collections"] = {'display_name': [], 'key': []} + indexes = self._mock_indexes(mock_meilisearch) api.rebuild_index() - assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 4 - mock_meilisearch.return_value.index.return_value.add_documents.assert_has_calls( + + # Library content goes to a temporary library index, course content to a temporary course index + library_temp_index = indexes[api.STUDIO_LIBRARY_INDEX_NAME + "_new"] + course_temp_index = indexes[api.STUDIO_COURSE_INDEX_NAME + "_new"] + assert library_temp_index.add_documents.call_count == 3 + library_temp_index.add_documents.assert_has_calls( [ - call([doc_sequential, doc_vertical]), call([doc_problem1, doc_problem2]), call([doc_collection]), call([doc_unit, doc_subsection, doc_section]), ], any_order=True, ) + course_temp_index.add_documents.assert_called_once_with([doc_sequential, doc_vertical]) + library_temp_index.update_filterable_attributes.assert_called_once_with(api.INDEX_FILTERABLE_ATTRIBUTES) + course_temp_index.update_filterable_attributes.assert_called_once_with(api.INDEX_FILTERABLE_ATTRIBUTES) + # Each temporary index is swapped into its own live index + assert mock_meilisearch.return_value.swap_indexes.call_args_list == [ + call([{"indexes": [api.STUDIO_LIBRARY_INDEX_NAME + "_new", api.STUDIO_LIBRARY_INDEX_NAME]}]), + call([{"indexes": [api.STUDIO_COURSE_INDEX_NAME + "_new", api.STUDIO_COURSE_INDEX_NAME]}]), + ] + # Library documents left over from the single shared index are removed from the course index + indexes[api.STUDIO_COURSE_INDEX_NAME].delete_documents.assert_called_once_with( + filter='type != "course_block"' + ) + indexes[api.STUDIO_LIBRARY_INDEX_NAME].delete_documents.assert_not_called() + + @override_settings(MEILISEARCH_ENABLED=True) + def test_reindex_meilisearch_libraries_only(self, mock_meilisearch) -> None: + """ + include_courses=False rebuilds the library index and cleans up the course index without reindexing courses. + """ + indexes = self._mock_indexes(mock_meilisearch) + api.rebuild_index(include_courses=False) + + assert indexes[api.STUDIO_LIBRARY_INDEX_NAME + "_new"].add_documents.call_count == 3 + mock_meilisearch.return_value.swap_indexes.assert_called_once_with( + [{"indexes": [api.STUDIO_LIBRARY_INDEX_NAME + "_new", api.STUDIO_LIBRARY_INDEX_NAME]}] + ) + assert api.STUDIO_COURSE_INDEX_NAME + "_new" not in indexes + course_index = indexes[api.STUDIO_COURSE_INDEX_NAME] + course_index.add_documents.assert_not_called() + course_index.delete_documents.assert_called_once_with(filter='type != "course_block"') @override_settings(MEILISEARCH_ENABLED=True) def test_reindex_meilisearch_incremental(self, mock_meilisearch) -> None: @@ -416,17 +465,24 @@ def test_reindex_meilisearch_incremental(self, mock_meilisearch) -> None: doc_section["tags"] = copy.deepcopy(EMPTY_TAGS) doc_section["collections"] = {'display_name': [], 'key': []} + indexes = self._mock_indexes(mock_meilisearch) + library_index = indexes[api.STUDIO_LIBRARY_INDEX_NAME] + course_index = indexes[api.STUDIO_COURSE_INDEX_NAME] + api.rebuild_index(incremental=True) - assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 4 - mock_meilisearch.return_value.index.return_value.add_documents.assert_has_calls( + # Incremental rebuilds write straight into the live indexes + mock_meilisearch.return_value.swap_indexes.assert_not_called() + assert library_index.add_documents.call_count == 3 + library_index.add_documents.assert_has_calls( [ - call([doc_sequential, doc_vertical]), call([doc_problem1, doc_problem2]), call([doc_collection]), call([doc_unit, doc_subsection, doc_section]), ], any_order=True, ) + course_index.add_documents.assert_called_once_with([doc_sequential, doc_vertical]) + course_index.delete_documents.assert_called_once_with(filter='type != "course_block"') # Now we simulate interruption by passing this function to the status_cb argument def simulated_interruption(message): @@ -437,21 +493,27 @@ def simulated_interruption(message): with pytest.raises(Exception, match="Simulated interruption"): api.rebuild_index(simulated_interruption, incremental=True) - # three more calls due to collections and containers - assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 7 + # The library was indexed again (blocks, collections and containers); no courses were + assert library_index.add_documents.call_count == 6 + assert course_index.add_documents.call_count == 1 assert IncrementalIndexCompleted.objects.all().count() == 1 api.rebuild_index(incremental=True) assert IncrementalIndexCompleted.objects.all().count() == 0 - # one missing course indexed - assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 8 + # one missing course indexed, the already-indexed library skipped + assert course_index.add_documents.call_count == 2 + assert library_index.add_documents.call_count == 6 @override_settings(MEILISEARCH_ENABLED=True) def test_reset_meilisearch_index(self, mock_meilisearch) -> None: - api.reset_index() - mock_meilisearch.return_value.swap_indexes.assert_called_once() - mock_meilisearch.return_value.create_index.assert_called_once() + api.reset_index(api.STUDIO_LIBRARY_INDEX_NAME) + mock_meilisearch.return_value.swap_indexes.assert_called_once_with( + [{"indexes": [api.STUDIO_LIBRARY_INDEX_NAME + "_new", api.STUDIO_LIBRARY_INDEX_NAME]}] + ) + mock_meilisearch.return_value.create_index.assert_called_once_with( + api.STUDIO_LIBRARY_INDEX_NAME + "_new", {"primaryKey": "id"} + ) mock_meilisearch.return_value.delete_index.call_count = 2 - api.reset_index() + api.reset_index(api.STUDIO_LIBRARY_INDEX_NAME) mock_meilisearch.return_value.delete_index.call_count = 4 @override_settings(MEILISEARCH_ENABLED=True) @@ -481,18 +543,22 @@ def test_init_meilisearch_index(self, mock_meilisearch) -> None: mock_meilisearch.return_value.create_index.assert_not_called() mock_meilisearch.return_value.delete_index.assert_not_called() - # Test index does not exist — should create it - mock_meilisearch.return_value.get_index.side_effect = [ - MeilisearchApiError("Testing reindex", Mock(text='{"code":"index_not_found"}')), - MeilisearchApiError("Testing reindex", Mock(text='{"code":"index_not_found"}')), - Mock(created_at=1), - Mock(created_at=1), - Mock(created_at=1), - ] + # Test the library index does not exist (upgrade from a single shared index) — should create only that one + created_indexes = set() + + def get_index(name): + if name == api.STUDIO_COURSE_INDEX_NAME or name in created_indexes: + return mock_index + raise MeilisearchApiError("Testing reindex", Mock(text='{"code":"index_not_found"}')) + + mock_meilisearch.return_value.get_index.side_effect = get_index + mock_meilisearch.return_value.create_index.side_effect = lambda name, *args: created_indexes.add(name) api.init_index() - mock_meilisearch.return_value.swap_indexes.assert_called_once() - mock_meilisearch.return_value.create_index.assert_called_once() - mock_meilisearch.return_value.delete_index.call_count = 2 + mock_meilisearch.return_value.swap_indexes.assert_called_once_with( + [{"indexes": [api.STUDIO_LIBRARY_INDEX_NAME + "_new", api.STUDIO_LIBRARY_INDEX_NAME]}] + ) + assert created_indexes == {api.STUDIO_LIBRARY_INDEX_NAME + "_new", api.STUDIO_LIBRARY_INDEX_NAME} + mock_meilisearch.return_value.delete_index.assert_called_once_with(api.STUDIO_LIBRARY_INDEX_NAME + "_new") @override_settings(MEILISEARCH_ENABLED=True) @patch( @@ -597,6 +663,7 @@ def test_index_xblock_metadata(self, recursive, mock_meilisearch) -> None: expected_docs = [self.doc_sequential] mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with(expected_docs) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_COURSE_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_no_index_excluded_xblocks(self, mock_meilisearch) -> None: @@ -643,6 +710,7 @@ def test_index_xblock_tags(self, mock_meilisearch) -> None: ], any_order=True, ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_COURSE_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_remove_xblock_tag_clears_index_tags(self, mock_meilisearch) -> None: @@ -693,6 +761,7 @@ def test_delete_index_xblock(self, mock_meilisearch) -> None: mock_meilisearch.return_value.index.return_value.delete_documents.assert_called_once_with( filter=f'breadcrumbs.usage_key = "{self.sequential.usage_key}"' ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_COURSE_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_index_library_block_metadata(self, mock_meilisearch) -> None: @@ -702,6 +771,7 @@ def test_index_library_block_metadata(self, mock_meilisearch) -> None: api.upsert_library_block_index_doc(self.problem1.usage_key) mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([self.doc_problem1]) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_index_library_block_tags(self, mock_meilisearch) -> None: @@ -743,6 +813,7 @@ def test_index_library_block_tags(self, mock_meilisearch) -> None: ], any_order=True, ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_index_library_block_and_collections(self, mock_meilisearch) -> None: @@ -885,6 +956,7 @@ def test_index_library_block_and_collections(self, mock_meilisearch) -> None: ], any_order=True, ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_delete_index_library_block(self, mock_meilisearch) -> None: @@ -896,17 +968,24 @@ def test_delete_index_library_block(self, mock_meilisearch) -> None: mock_meilisearch.return_value.index.return_value.delete_document.assert_called_once_with( self.doc_problem1['id'] ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_delete_docs_with_context_key(self, mock_meilisearch) -> None: """ Test deleting a all Block docs from the index using context_key. """ + indexes = self._mock_indexes(mock_meilisearch) + api.delete_docs_with_context_key(self.course.id) + api.delete_docs_with_context_key(self.library.key) - mock_meilisearch.return_value.index.return_value.delete_documents.assert_called_once_with( + indexes[api.STUDIO_COURSE_INDEX_NAME].delete_documents.assert_called_once_with( filter=f'context_key = "{self.course.id}"' ) + indexes[api.STUDIO_LIBRARY_INDEX_NAME].delete_documents.assert_called_once_with( + filter=f'context_key = "{self.library.key}"' + ) @override_settings(MEILISEARCH_ENABLED=True) def test_index_content_library_metadata(self, mock_meilisearch) -> None: @@ -918,6 +997,7 @@ def test_index_content_library_metadata(self, mock_meilisearch) -> None: mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with( [self.doc_problem1, self.doc_problem2] ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_index_tags_in_collections(self, mock_meilisearch) -> None: @@ -1083,6 +1163,7 @@ def test_delete_collection(self, mock_meilisearch) -> None: ], any_order=True, ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @ddt.data( "unit", @@ -1188,6 +1269,7 @@ def test_index_library_container_metadata(self, container_type, mock_meilisearch api.upsert_library_container_index_doc(container.container_key) mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([container_dict]) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @ddt.data( ("unit", "lctorg1libunitunit-1-e4527f7c"), @@ -1276,6 +1358,7 @@ def test_block_in_units(self, mock_meilisearch) -> None: ], any_order=True, ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_units_in_subsection(self, mock_meilisearch) -> None: @@ -1379,6 +1462,7 @@ def test_fetch_block_types(self, mock_meilisearch): mock_index = mock_meilisearch.return_value.get_index.return_value fetch_block_types('context_key = test') + mock_meilisearch.return_value.get_index.assert_called_once_with(api.STUDIO_COURSE_INDEX_NAME) mock_index.search.assert_called_once_with( "", { @@ -1436,3 +1520,52 @@ def test_get_all_blocks_from_context(self, mock_meilisearch): "attributesToRetrieve": ["usage_key", "display_name"], } ) + mock_meilisearch.return_value.get_index.assert_called_once_with(api.STUDIO_COURSE_INDEX_NAME) + + @ddt.data( + (CourseKey, "course-v1:org1+test_course+test_run", "course"), + (UsageKey, "block-v1:org1+test_course+test_run+type@sequential+block@test_sequential", "course"), + (CourseKey, "library-v1:org1+legacy_lib", "course"), + (LibraryLocatorV2, "lib:org1:lib", "library"), + (UsageKey, "lb:org1:lib:problem:p1", "library"), + (LibraryCollectionLocator, "lib-collection:org1:lib:MYCOL", "library"), + (LibraryContainerLocator, "lct:org1:lib:unit:unit-1", "library"), + ) + @ddt.unpack + def test_index_name_for_key(self, key_type, key_str, expected, mock_meilisearch) -> None: + """ + Modulestore content (courses, legacy libraries) routes to the course index, Libraries V2 content to the + library index. + """ + expected_index = {"course": api.STUDIO_COURSE_INDEX_NAME, "library": api.STUDIO_LIBRARY_INDEX_NAME}[expected] + assert api._index_name_for_key(key_type.from_string(key_str)) == expected_index # pylint: disable=protected-access + + @override_settings(MEILISEARCH_ENABLED=True) + def test_update_during_rebuild_uses_that_index_temp_index(self, mock_meilisearch) -> None: + """ + While the library index is being rebuilt, library writes also go to its temporary index. + Course writes are unaffected by the library rebuild lock. + """ + indexes = self._mock_indexes(mock_meilisearch) + + with api._index_rebuild_lock(api.STUDIO_LIBRARY_INDEX_NAME): # pylint: disable=protected-access + api.upsert_library_block_index_doc(self.problem1.usage_key) + api.upsert_xblock_index_doc(self.sequential.usage_key, recursive=False) + + indexes[api.STUDIO_LIBRARY_INDEX_NAME + "_new"].update_documents.assert_called_once_with([self.doc_problem1]) + indexes[api.STUDIO_LIBRARY_INDEX_NAME].update_documents.assert_called_once_with([self.doc_problem1]) + indexes[api.STUDIO_COURSE_INDEX_NAME].update_documents.assert_called_once_with([self.doc_sequential]) + assert api.STUDIO_COURSE_INDEX_NAME + "_new" not in indexes + + @override_settings(MEILISEARCH_ENABLED=True) + def test_delete_library_docs_from_missing_course_index(self, mock_meilisearch) -> None: + """ + Cleaning up the course index does nothing if there is no course index yet. + """ + mock_meilisearch.return_value.get_index.side_effect = MeilisearchApiError( + "Not found", Mock(text='{"code":"index_not_found"}') + ) + + api.delete_library_docs_from_course_index() + + mock_meilisearch.return_value.index.assert_not_called() diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py index e2215c7f3c56..9220acdf8d5f 100644 --- a/openedx/core/djangoapps/content/search/tests/test_handlers.py +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -82,6 +82,7 @@ def test_create_delete_xblock(self, meilisearch_client): } meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_sequential]) + meilisearch_client.return_value.index.assert_called_with(api.STUDIO_COURSE_INDEX_NAME) with freeze_time(created_date), self.captureOnCommitCallbacks(execute=True): vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") @@ -134,6 +135,9 @@ def test_create_delete_xblock(self, meilisearch_client): meilisearch_client.return_value.index.return_value.delete_document.assert_called_with( "block-v1orgatest_coursetest_runtypeverticalblocktest_vertical-011f143b" ) + assert {c.args[0] for c in meilisearch_client.return_value.index.call_args_list} == { + api.STUDIO_COURSE_INDEX_NAME + } def test_library_creation_creates_search_access(self, meilisearch_client): """ @@ -225,3 +229,6 @@ def test_create_delete_library_block(self, meilisearch_client): with self.captureOnCommitCallbacks(execute=True): library_api.restore_library_block(problem.usage_key) meilisearch_client.return_value.index.return_value.update_documents.assert_any_call([doc_problem]) + assert {c.args[0] for c in meilisearch_client.return_value.index.call_args_list} == { + api.STUDIO_LIBRARY_INDEX_NAME + } diff --git a/openedx/core/djangoapps/content/search/tests/test_reconcile.py b/openedx/core/djangoapps/content/search/tests/test_reconcile.py index 6904f555ebb4..3a0502112f9c 100644 --- a/openedx/core/djangoapps/content/search/tests/test_reconcile.py +++ b/openedx/core/djangoapps/content/search/tests/test_reconcile.py @@ -1,10 +1,11 @@ """ Tests for the Meilisearch index reconciliation logic. """ +# pylint: disable=protected-access from __future__ import annotations -from unittest.mock import MagicMock, Mock, patch +from unittest.mock import MagicMock, Mock, call, patch import pytest from django.test import TestCase, override_settings @@ -355,6 +356,49 @@ def setUp(self): super().setUp() api.clear_meilisearch_client() + @patch("openedx.core.djangoapps.content.search.api._reconcile_single_index") + def test_reconciles_both_indexes(self, mock_reconcile_single, mock_meilisearch): + """Both the course index and the library index are reconciled.""" + status_cb = Mock() + warn_cb = Mock() + + reconcile_index(status_cb=status_cb, warn_cb=warn_cb) + + assert mock_reconcile_single.call_args_list == [ + call(api.STUDIO_COURSE_INDEX_NAME, status_cb, warn_cb), + call(api.STUDIO_LIBRARY_INDEX_NAME, status_cb, warn_cb), + ] + + @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") + @patch("openedx.core.djangoapps.content.search.api.reset_index") + def test_only_library_index_missing(self, mock_reset, mock_drift, mock_meilisearch): + """ + Upgrading from a single shared index: the populated course index is left alone and the library index is + created, with a hint to populate it without reindexing courses. + """ + populated = IndexDrift( + exists=True, + is_empty=False, + primary_key_correct=True, + distinct_attribute_match=True, + filterable_attributes_match=True, + searchable_attributes_match=True, + sortable_attributes_match=True, + ranking_rules_match=True, + ) + mock_drift.side_effect = lambda name: ( + populated if name == api.STUDIO_COURSE_INDEX_NAME else IndexDrift(exists=False) + ) + status_cb = Mock() + + reconcile_index(status_cb=status_cb) + + mock_reset.assert_called_once_with(api.STUDIO_LIBRARY_INDEX_NAME, status_cb) + status_cb.assert_any_call( + f"Index '{api.STUDIO_LIBRARY_INDEX_NAME}' created. " + "Run './manage.py cms reindex_studio --libraries-only' to populate." + ) + @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") @patch("openedx.core.djangoapps.content.search.api.reset_index") def test_index_missing(self, mock_reset, mock_drift, mock_meilisearch): @@ -362,10 +406,12 @@ def test_index_missing(self, mock_reset, mock_drift, mock_meilisearch): mock_drift.return_value = IndexDrift(exists=False) status_cb = Mock() - reconcile_index(status_cb=status_cb) + api._reconcile_single_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, Mock()) - mock_reset.assert_called_once() - status_cb.assert_any_call("Studio search index not found. Creating and configuring...") + mock_reset.assert_called_once_with(api.STUDIO_COURSE_INDEX_NAME, status_cb) + status_cb.assert_any_call( + f"Studio search index '{api.STUDIO_COURSE_INDEX_NAME}' not found. Creating and configuring..." + ) @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") def test_index_empty_configured(self, mock_drift, mock_meilisearch): @@ -382,10 +428,11 @@ def test_index_empty_configured(self, mock_drift, mock_meilisearch): ) status_cb = Mock() - reconcile_index(status_cb=status_cb) + api._reconcile_single_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, Mock()) status_cb.assert_any_call( - "Index exists and is correctly configured but empty. Run './manage.py cms reindex_studio' to populate." + f"Index '{api.STUDIO_COURSE_INDEX_NAME}' exists and is correctly configured but empty. " + "Run './manage.py cms reindex_studio' to populate." ) @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") @@ -404,10 +451,12 @@ def test_index_empty_drifted_settings(self, mock_apply, mock_drift, mock_meilise ) status_cb = Mock() - reconcile_index(status_cb=status_cb) + api._reconcile_single_index(api.STUDIO_LIBRARY_INDEX_NAME, status_cb, Mock()) - mock_apply.assert_called_once_with(api.STUDIO_INDEX_NAME, wait=True, status_cb=status_cb) - status_cb.assert_any_call("Empty index has drifted settings. Reconfiguring...") + mock_apply.assert_called_once_with(api.STUDIO_LIBRARY_INDEX_NAME, wait=True, status_cb=status_cb) + status_cb.assert_any_call( + f"Empty index '{api.STUDIO_LIBRARY_INDEX_NAME}' has drifted settings. Reconfiguring..." + ) @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") @patch("openedx.core.djangoapps.content.search.api.reset_index") @@ -425,10 +474,12 @@ def test_index_empty_wrong_pk(self, mock_reset, mock_drift, mock_meilisearch): ) warn_cb = Mock() - reconcile_index(warn_cb=warn_cb) + api._reconcile_single_index(api.STUDIO_COURSE_INDEX_NAME, Mock(), warn_cb) mock_reset.assert_called_once() - warn_cb.assert_any_call("Primary key mismatch on empty index. Recreating...") + warn_cb.assert_any_call( + f"Primary key mismatch on empty index '{api.STUDIO_COURSE_INDEX_NAME}'. Recreating..." + ) @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") def test_index_populated_configured(self, mock_drift, mock_meilisearch): @@ -445,9 +496,11 @@ def test_index_populated_configured(self, mock_drift, mock_meilisearch): ) status_cb = Mock() - reconcile_index(status_cb=status_cb) + api._reconcile_single_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, Mock()) - status_cb.assert_any_call("Index is populated and correctly configured. No action needed.") + status_cb.assert_any_call( + f"Index '{api.STUDIO_COURSE_INDEX_NAME}' is populated and correctly configured. No action needed." + ) @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") @patch("openedx.core.djangoapps.content.search.api._apply_index_settings") @@ -466,9 +519,9 @@ def test_index_populated_drifted_settings(self, mock_apply, mock_drift, mock_mei status_cb = Mock() warn_cb = Mock() - reconcile_index(status_cb=status_cb, warn_cb=warn_cb) + api._reconcile_single_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, warn_cb) - mock_apply.assert_called_once_with(api.STUDIO_INDEX_NAME, wait=True, status_cb=status_cb) + mock_apply.assert_called_once_with(api.STUDIO_COURSE_INDEX_NAME, wait=True, status_cb=status_cb) # Check that drifted fields are logged warn_cb.assert_any_call(" - filterableAttributes: DRIFTED") warn_cb.assert_any_call(" - searchableAttributes: DRIFTED") @@ -492,11 +545,14 @@ def test_index_populated_wrong_pk(self, mock_reset, mock_drift, mock_meilisearch ) warn_cb = Mock() - reconcile_index(warn_cb=warn_cb) + api._reconcile_single_index(api.STUDIO_LIBRARY_INDEX_NAME, Mock(), warn_cb) mock_reset.assert_called_once() # Should warn about data loss - warn_cb.assert_any_call("Index recreated empty. Run './manage.py cms reindex_studio' to repopulate.") + warn_cb.assert_any_call( + f"Index '{api.STUDIO_LIBRARY_INDEX_NAME}' recreated empty. " + "Run './manage.py cms reindex_studio --libraries-only' to repopulate." + ) @override_settings(MEILISEARCH_ENABLED=False) def test_meilisearch_disabled(self, mock_meilisearch): diff --git a/openedx/core/djangoapps/content/search/tests/test_reindex_cmd.py b/openedx/core/djangoapps/content/search/tests/test_reindex_cmd.py index 45a89d7d9e04..30e7b8b49681 100644 --- a/openedx/core/djangoapps/content/search/tests/test_reindex_cmd.py +++ b/openedx/core/djangoapps/content/search/tests/test_reindex_cmd.py @@ -14,7 +14,7 @@ try: from .. import api - from ..tasks import rebuild_index_incremental + from ..tasks import rebuild_index_incremental, rebuild_library_index except RuntimeError: pass @@ -50,6 +50,17 @@ def test_incremental_flag_accepted_with_warning(self, mock_log, mock_delay): assert mock_log.warning.call_count == 4 mock_delay.assert_called_once_with() + @patch("openedx.core.djangoapps.content.search.tasks.rebuild_index_incremental.delay") + @patch("openedx.core.djangoapps.content.search.tasks.rebuild_library_index.delay") + def test_libraries_only(self, mock_library_delay, mock_incremental_delay): + """--libraries-only enqueues the library index rebuild and not the full incremental rebuild.""" + mock_library_delay.return_value = Mock(id="fake-task-id") + + call_command("reindex_studio", "--libraries-only") + + mock_library_delay.assert_called_once_with() + mock_incremental_delay.assert_not_called() + @skip_unless_cms @override_settings(MEILISEARCH_ENABLED=True) @@ -94,3 +105,41 @@ def test_idempotent(self, mock_rebuild, mock_meilisearch): rebuild_index_incremental() assert mock_rebuild.call_count == 2 + + +@skip_unless_cms +@override_settings(MEILISEARCH_ENABLED=True) +@patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", new=MagicMock(return_value=None)) +@patch("openedx.core.djangoapps.content.search.api.MeilisearchClient") +class TestRebuildLibraryIndexTask(TestCase): + """Tests for the rebuild_library_index Celery task.""" + + def setUp(self): + super().setUp() + api.clear_meilisearch_client() + + @patch("openedx.core.djangoapps.content.search.api.rebuild_index") + def test_rebuilds_libraries_only(self, mock_rebuild, mock_meilisearch): + """Task rebuilds the library index without reindexing courses.""" + rebuild_library_index() + + mock_rebuild.assert_called_once() + _, kwargs = mock_rebuild.call_args + assert kwargs["include_courses"] is False + assert kwargs.get("incremental", False) is False + + @patch("openedx.core.djangoapps.content.search.api.rebuild_index") + def test_rebuild_already_in_progress(self, mock_rebuild, mock_meilisearch): + """Task exits gracefully if the library index rebuild lock is already held.""" + mock_rebuild.side_effect = RuntimeError("Rebuild already in progress") + + # Should not raise + rebuild_library_index() + + @patch("openedx.core.djangoapps.content.search.api.rebuild_index") + def test_other_runtime_error_raised(self, mock_rebuild, mock_meilisearch): + """Task re-raises RuntimeError if it's not about lock contention.""" + mock_rebuild.side_effect = RuntimeError("Something else went wrong") + + with pytest.raises(RuntimeError, match="Something else went wrong"): + rebuild_library_index() diff --git a/openedx/core/djangoapps/content/search/tests/test_views.py b/openedx/core/djangoapps/content/search/tests/test_views.py index 3d056b2b4327..8ab573ff50c5 100644 --- a/openedx/core/djangoapps/content/search/tests/test_views.py +++ b/openedx/core/djangoapps/content/search/tests/test_views.py @@ -53,6 +53,16 @@ def wrapper(*args, **kwargs): return decorator +def search_rules_for_both_indexes(rule: dict) -> dict: + """ + The tenant token applies the same access rule to the course index and the library index. + """ + return { + "studio_content": rule, + "studio_library_content": rule, + } + + @ddt.ddt @skip_unless_cms @patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", new=MagicMock(return_value=None)) @@ -113,6 +123,9 @@ def test_studio_search_enabled(self, mock_search_client): mock_generate_tenant_token = self._mock_generate_tenant_token(mock_search_client) # noqa: F841 result = self.client.get(STUDIO_SEARCH_ENDPOINT_URL) assert result.status_code == 200 + assert result.data["course_index_name"] == "studio_content" + assert result.data["library_index_name"] == "studio_library_content" + # Deprecated key, kept for frontends that predate the index split assert result.data["index_name"] == "studio_content" assert result.data["url"] == "http://meilisearch.url" assert result.data["api_key"] and isinstance(result.data["api_key"], str) # noqa: PT018 @@ -129,11 +142,9 @@ def test_studio_search_student_no_access(self, mock_search_client): assert result.status_code == 200 mock_generate_tenant_token.assert_called_once_with( api_key_uid=MOCK_API_KEY_UID, - search_rules={ - "studio_content": { - "filter": "org IN [] OR access_id IN []", - } - }, + search_rules=search_rules_for_both_indexes({ + "filter": "org IN [] OR access_id IN []", + }), expires_at=ANY, ) @@ -149,9 +160,7 @@ def test_studio_search_staff(self, mock_search_client): assert result.status_code == 200 mock_generate_tenant_token.assert_called_once_with( api_key_uid=MOCK_API_KEY_UID, - search_rules={ - "studio_content": {} - }, + search_rules=search_rules_for_both_indexes({}), expires_at=ANY, ) @@ -173,11 +182,9 @@ def test_studio_search_course_staff_access(self, mock_search_client): mock_generate_tenant_token.assert_called_once_with( api_key_uid=MOCK_API_KEY_UID, - search_rules={ - "studio_content": { - "filter": f"org IN [] OR access_id IN {expected_access_ids}", - } - }, + search_rules=search_rules_for_both_indexes({ + "filter": f"org IN [] OR access_id IN {expected_access_ids}", + }), expires_at=ANY, ) @@ -197,11 +204,9 @@ def test_studio_search_org_access(self, username, mock_search_client): assert result.status_code == 200 mock_generate_tenant_token.assert_called_once_with( api_key_uid=MOCK_API_KEY_UID, - search_rules={ - "studio_content": { - "filter": "org IN ['org1'] OR access_id IN []", - } - }, + search_rules=search_rules_for_both_indexes({ + "filter": "org IN ['org1'] OR access_id IN []", + }), expires_at=ANY, ) @@ -224,11 +229,9 @@ def test_studio_search_omit_orgs(self, mock_search_client): mock_generate_tenant_token.assert_called_once_with( api_key_uid=MOCK_API_KEY_UID, - search_rules={ - "studio_content": { - "filter": f"org IN ['org1'] OR access_id IN {expected_access_ids}", - } - }, + search_rules=search_rules_for_both_indexes({ + "filter": f"org IN ['org1'] OR access_id IN {expected_access_ids}", + }), expires_at=ANY, ) @@ -259,10 +262,8 @@ def test_studio_search_limits(self, mock_search_client, mock_get_access_ids, moc mock_get_access_ids.assert_called_once() mock_generate_tenant_token.assert_called_once_with( api_key_uid=MOCK_API_KEY_UID, - search_rules={ - "studio_content": { - "filter": f"org IN {expected_user_orgs} OR access_id IN {expected_access_ids}", - } - }, + search_rules=search_rules_for_both_indexes({ + "filter": f"org IN {expected_user_orgs} OR access_id IN {expected_access_ids}", + }), expires_at=ANY, ) From 4baff25040b054607439a8eea4dcc01e927d0baf Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Mon, 14 Sep 2026 11:23:54 -0400 Subject: [PATCH 2/5] docs: drop multi-index frontend claim from index split ADR Each Studio search surface already queries course content or library content, never both, so the frontend picks one index per surface rather than issuing a multi-index search. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01J43t1WXNsmVV5mbdzxv6iT --- .../0002-separate-course-and-library-indexes.rst | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content/search/docs/decisions/0002-separate-course-and-library-indexes.rst b/openedx/core/djangoapps/content/search/docs/decisions/0002-separate-course-and-library-indexes.rst index dcaac2408a5d..1fa2b849edf2 100644 --- a/openedx/core/djangoapps/content/search/docs/decisions/0002-separate-course-and-library-indexes.rst +++ b/openedx/core/djangoapps/content/search/docs/decisions/0002-separate-course-and-library-indexes.rst @@ -43,11 +43,16 @@ Both indexes use the same settings. Every write is routed by document type or by the learning context of its key. Rebuild locks and temporary ``_new`` indexes are per index. +Every modulestore block is indexed as a ``course_block`` document, including +course blocks that link to upstream library content, so those stay in the +course index. + ``GET /api/content_search/v2/studio/`` returns ``course_index_name`` and ``library_index_name``, and one tenant token whose search rules cover both -indexes with the same access filter. The frontend uses a multi-index search for -views that span courses and libraries. ``index_name`` is still returned (equal -to ``course_index_name``) for one release, for frontends that predate the split. +indexes with the same access filter. Each Studio search surface already +searches either course content or library content, never both, so each one +picks the matching index. ``index_name`` is still returned (equal to +``course_index_name``) for one release, for frontends that predate the split. Upgrading @@ -73,6 +78,7 @@ Consequences ************ * Library writes no longer pay for the size of the course index. -* Searches that span courses and libraries need a multi-index search request. +* A future search across courses and libraries together would need a + multi-index search request. * The Meilisearch API key used by Studio must be allowed to manage both index names (and their ``_new`` temporary indexes). From b5ad5840876235580bf01ca459f408b751adb994 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Thu, 17 Sep 2026 12:18:59 -0400 Subject: [PATCH 3/5] fix: abort library index rebuild on Meilisearch write failures The library pass logged and skipped MeilisearchError from add_documents, so a failed write still swapped the partial library index in and then deleted the library documents from the course index. rebuild_library_index also reported success, so its Celery autoretry never fired. index_course already lets these errors propagate; the library pass now does the same. Per-document build errors are still logged and skipped. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JRNUbu4khegCEFfy4j9a5R --- openedx/core/djangoapps/content/search/api.py | 8 +++++--- .../djangoapps/content/search/tests/test_api.py | 16 +++++++++++++++- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index bf7f4ade1ac3..d019f85322d5 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -673,6 +673,8 @@ def rebuild_index( # pylint: disable=too-many-statements _apply_index_settings(index_name, wait=False) ############## Libraries ############## + # MeilisearchError from add_documents is not caught below: an incomplete library index must not be + # swapped in or followed by the course index cleanup, and the Celery task needs the error to retry. status_cb("Indexing libraries...") def index_library(lib_key: LibraryLocatorV2) -> list: @@ -692,7 +694,7 @@ def index_library(lib_key: LibraryLocatorV2) -> list: try: # Add all the docs in this library at once (usually faster than adding one at a time): _wait_for_meili_task(client.index(index_name).add_documents(docs)) - except (TypeError, KeyError, MeilisearchError) as err: + except (TypeError, KeyError) as err: status_cb(f"Error indexing library {lib_key}: {err}") return docs @@ -713,7 +715,7 @@ def index_collection_batch(batch, num_done, library_key) -> int: try: # Add docs in batch of 100 at once (usually faster than adding one at a time): _wait_for_meili_task(client.index(index_name).add_documents(docs)) - except (TypeError, KeyError, MeilisearchError) as err: + except (TypeError, KeyError) as err: status_cb(f"Error indexing collection batch {p}: {err}") return num_done @@ -744,7 +746,7 @@ def index_container_batch(batch, num_done, library_key) -> int: try: # Add docs in batch of 100 at once (usually faster than adding one at a time): _wait_for_meili_task(client.index(index_name).add_documents(docs)) - except (TypeError, KeyError, MeilisearchError) as err: + except (TypeError, KeyError) as err: status_cb(f"Error indexing container batch {p}: {err}") return num_done diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 4a123f857d7c..adbf1f110b3e 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -12,7 +12,7 @@ import pytest from django.test import override_settings from freezegun import freeze_time -from meilisearch.errors import MeilisearchApiError +from meilisearch.errors import MeilisearchApiError, MeilisearchError from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2 from openedx_content import api as content_api @@ -435,6 +435,20 @@ def test_reindex_meilisearch_libraries_only(self, mock_meilisearch) -> None: course_index.add_documents.assert_not_called() course_index.delete_documents.assert_called_once_with(filter='type != "course_block"') + @override_settings(MEILISEARCH_ENABLED=True) + def test_reindex_library_write_error_aborts_before_swap(self, mock_meilisearch) -> None: + """ + A failed library write must not swap in the partial library index or clean up the course index. + """ + indexes = self._mock_indexes(mock_meilisearch) + indexes[api.STUDIO_LIBRARY_INDEX_NAME + "_new"].add_documents.side_effect = MeilisearchError("write failed") + + with pytest.raises(MeilisearchError): + api.rebuild_index(include_courses=False) + + mock_meilisearch.return_value.swap_indexes.assert_not_called() + indexes[api.STUDIO_COURSE_INDEX_NAME].delete_documents.assert_not_called() + @override_settings(MEILISEARCH_ENABLED=True) def test_reindex_meilisearch_incremental(self, mock_meilisearch) -> None: From 019008fea0caf9e18415f84bc4daf517fb9be877 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Thu, 17 Sep 2026 12:19:10 -0400 Subject: [PATCH 4/5] fix: drop pre-split library checkpoints from incremental reindex state IncrementalIndexCompleted rows only record a context key. A library row left by an interrupted run from before the index split means the library was written to the course index, but an incremental rebuild would trust it and skip the library for the new library index, then delete its old copies from the course index. Tutor's init job runs exactly that incremental reindex_studio after migrate. Clearing library rows once at migration time makes those libraries reindex; library passes are cheap, so no per-index checkpoint column is needed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JRNUbu4khegCEFfy4j9a5R --- ...r_library_incremental_index_checkpoints.py | 26 +++++++++++++++++++ .../content/search/tests/test_api.py | 16 ++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 openedx/core/djangoapps/content/search/migrations/0003_clear_library_incremental_index_checkpoints.py diff --git a/openedx/core/djangoapps/content/search/migrations/0003_clear_library_incremental_index_checkpoints.py b/openedx/core/djangoapps/content/search/migrations/0003_clear_library_incremental_index_checkpoints.py new file mode 100644 index 000000000000..95e9a1e28e58 --- /dev/null +++ b/openedx/core/djangoapps/content/search/migrations/0003_clear_library_incremental_index_checkpoints.py @@ -0,0 +1,26 @@ +from django.db import migrations +from opaque_keys.edx.locator import LibraryLocatorV2 + + +def clear_library_checkpoints(apps, schema_editor): + """ + Checkpoints written before the course/library index split record libraries indexed into the course index. + An incremental rebuild would skip those libraries and never add them to the new library index. + """ + IncrementalIndexCompleted = apps.get_model("search", "IncrementalIndexCompleted") + library_ids = [ + checkpoint.id + for checkpoint in IncrementalIndexCompleted.objects.all() + if isinstance(checkpoint.context_key, LibraryLocatorV2) + ] + IncrementalIndexCompleted.objects.filter(id__in=library_ids).delete() + + +class Migration(migrations.Migration): + dependencies = [ + ("search", "0002_incrementalindexcompleted"), + ] + + operations = [ + migrations.RunPython(clear_library_checkpoints, migrations.RunPython.noop), + ] diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index adbf1f110b3e..c81a4521faff 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -4,12 +4,14 @@ from __future__ import annotations import copy +import importlib from collections import defaultdict from datetime import UTC, datetime from unittest.mock import MagicMock, Mock, call, patch import ddt import pytest +from django.apps import apps from django.test import override_settings from freezegun import freeze_time from meilisearch.errors import MeilisearchApiError, MeilisearchError @@ -449,6 +451,20 @@ def test_reindex_library_write_error_aborts_before_swap(self, mock_meilisearch) mock_meilisearch.return_value.swap_indexes.assert_not_called() indexes[api.STUDIO_COURSE_INDEX_NAME].delete_documents.assert_not_called() + def test_migration_clears_library_incremental_checkpoints(self, mock_meilisearch) -> None: + """ + Library checkpoints from before the index split are dropped so incremental rebuilds reindex those libraries. + """ + migration = importlib.import_module( + "openedx.core.djangoapps.content.search.migrations.0003_clear_library_incremental_index_checkpoints" + ) + IncrementalIndexCompleted.objects.create(context_key=self.library.key) + IncrementalIndexCompleted.objects.create(context_key=self.course.id) + + migration.clear_library_checkpoints(apps, None) + + assert list(IncrementalIndexCompleted.objects.values_list("context_key", flat=True)) == [self.course.id] + @override_settings(MEILISEARCH_ENABLED=True) def test_reindex_meilisearch_incremental(self, mock_meilisearch) -> None: From 38a25684757796a914e3eb3a746ea0abf15ac4a4 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Thu, 17 Sep 2026 12:19:10 -0400 Subject: [PATCH 5/5] refactor: rename reconcile_index to reconcile_indexes Per review: reconcile_indexes() reconciles the course and library indexes, and reconcile_index(index_name) reconciles one. reconcile_index() shipped in Verawood taking no index name, so callers of that signature must switch to reconcile_indexes(). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JRNUbu4khegCEFfy4j9a5R --- openedx/core/djangoapps/content/search/api.py | 21 +++++---- .../djangoapps/content/search/handlers.py | 4 +- .../content/search/tests/test_reconcile.py | 46 +++++++++---------- 3 files changed, 38 insertions(+), 33 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index d019f85322d5..da68dcacc297 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -470,7 +470,7 @@ def _compare_setting(key, expected): ) -def reconcile_index( +def reconcile_indexes( status_cb: Callable[[str], None] | None = None, warn_cb: Callable[[str], None] | None = None ) -> None: # noqa: E501 """ @@ -491,17 +491,22 @@ def reconcile_index( warn_cb = log.warning for index_name in (STUDIO_COURSE_INDEX_NAME, STUDIO_LIBRARY_INDEX_NAME): - _reconcile_single_index(index_name, status_cb, warn_cb) + reconcile_index(index_name, status_cb, warn_cb) -def _reconcile_single_index( +def reconcile_index( index_name: str, - status_cb: Callable[[str], None], - warn_cb: Callable[[str], None], + status_cb: Callable[[str], None] | None = None, + warn_cb: Callable[[str], None] | None = None, ) -> None: """ - Reconcile the state of one Studio Meilisearch index. See reconcile_index(). + Reconcile the state of one Studio Meilisearch index. See reconcile_indexes(). """ + if status_cb is None: + status_cb = log.info + if warn_cb is None: + warn_cb = log.warning + if index_name == STUDIO_LIBRARY_INDEX_NAME: populate_cmd = "./manage.py cms reindex_studio --libraries-only" else: @@ -572,10 +577,10 @@ def init_index(status_cb: Callable[[str], None] | None = None, warn_cb: Callable Initialize the Meilisearch index, creating it and configuring it if it doesn't exist. - This is a compatibility wrapper around reconcile_index(). + This is a compatibility wrapper around reconcile_indexes(). """ log.warning("init_index is deprecated as of Verawood and will be removed in the future release.") - reconcile_index(status_cb=status_cb, warn_cb=warn_cb) + reconcile_indexes(status_cb=status_cb, warn_cb=warn_cb) def index_course( diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 0fe93292d8fa..c60c67eb7115 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -49,7 +49,7 @@ from .api import ( is_meilisearch_enabled, only_if_meilisearch_enabled, - reconcile_index, + reconcile_indexes, upsert_content_object_tags_index_doc, upsert_item_collections_index_docs, upsert_item_containers_index_docs, @@ -86,7 +86,7 @@ def handle_post_migrate(sender, **kwargs): return try: - reconcile_index(status_cb=log.info, warn_cb=log.warning) + reconcile_indexes(status_cb=log.info, warn_cb=log.warning) except ConnectionError as exc: log.warning( "Meilisearch reconciliation skipped during post_migrate: %s. " diff --git a/openedx/core/djangoapps/content/search/tests/test_reconcile.py b/openedx/core/djangoapps/content/search/tests/test_reconcile.py index 3a0502112f9c..1b44b8c6acb0 100644 --- a/openedx/core/djangoapps/content/search/tests/test_reconcile.py +++ b/openedx/core/djangoapps/content/search/tests/test_reconcile.py @@ -19,7 +19,7 @@ IndexDrift, _apply_index_settings, _detect_index_drift, - reconcile_index, + reconcile_indexes, ) from ..apps import ContentSearchConfig from ..handlers import handle_post_migrate @@ -350,19 +350,19 @@ def test_raises_on_task_failure(self, mock_wait, mock_meilisearch): @patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", new=MagicMock(return_value=None)) @patch("openedx.core.djangoapps.content.search.api.MeilisearchClient") class TestReconcileIndex(TestCase): - """Tests for reconcile_index().""" + """Tests for reconcile_indexes().""" def setUp(self): super().setUp() api.clear_meilisearch_client() - @patch("openedx.core.djangoapps.content.search.api._reconcile_single_index") + @patch("openedx.core.djangoapps.content.search.api.reconcile_index") def test_reconciles_both_indexes(self, mock_reconcile_single, mock_meilisearch): """Both the course index and the library index are reconciled.""" status_cb = Mock() warn_cb = Mock() - reconcile_index(status_cb=status_cb, warn_cb=warn_cb) + reconcile_indexes(status_cb=status_cb, warn_cb=warn_cb) assert mock_reconcile_single.call_args_list == [ call(api.STUDIO_COURSE_INDEX_NAME, status_cb, warn_cb), @@ -391,7 +391,7 @@ def test_only_library_index_missing(self, mock_reset, mock_drift, mock_meilisear ) status_cb = Mock() - reconcile_index(status_cb=status_cb) + reconcile_indexes(status_cb=status_cb) mock_reset.assert_called_once_with(api.STUDIO_LIBRARY_INDEX_NAME, status_cb) status_cb.assert_any_call( @@ -406,7 +406,7 @@ def test_index_missing(self, mock_reset, mock_drift, mock_meilisearch): mock_drift.return_value = IndexDrift(exists=False) status_cb = Mock() - api._reconcile_single_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, Mock()) + api.reconcile_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, Mock()) mock_reset.assert_called_once_with(api.STUDIO_COURSE_INDEX_NAME, status_cb) status_cb.assert_any_call( @@ -428,7 +428,7 @@ def test_index_empty_configured(self, mock_drift, mock_meilisearch): ) status_cb = Mock() - api._reconcile_single_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, Mock()) + api.reconcile_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, Mock()) status_cb.assert_any_call( f"Index '{api.STUDIO_COURSE_INDEX_NAME}' exists and is correctly configured but empty. " @@ -451,7 +451,7 @@ def test_index_empty_drifted_settings(self, mock_apply, mock_drift, mock_meilise ) status_cb = Mock() - api._reconcile_single_index(api.STUDIO_LIBRARY_INDEX_NAME, status_cb, Mock()) + api.reconcile_index(api.STUDIO_LIBRARY_INDEX_NAME, status_cb, Mock()) mock_apply.assert_called_once_with(api.STUDIO_LIBRARY_INDEX_NAME, wait=True, status_cb=status_cb) status_cb.assert_any_call( @@ -474,7 +474,7 @@ def test_index_empty_wrong_pk(self, mock_reset, mock_drift, mock_meilisearch): ) warn_cb = Mock() - api._reconcile_single_index(api.STUDIO_COURSE_INDEX_NAME, Mock(), warn_cb) + api.reconcile_index(api.STUDIO_COURSE_INDEX_NAME, Mock(), warn_cb) mock_reset.assert_called_once() warn_cb.assert_any_call( @@ -496,7 +496,7 @@ def test_index_populated_configured(self, mock_drift, mock_meilisearch): ) status_cb = Mock() - api._reconcile_single_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, Mock()) + api.reconcile_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, Mock()) status_cb.assert_any_call( f"Index '{api.STUDIO_COURSE_INDEX_NAME}' is populated and correctly configured. No action needed." @@ -519,7 +519,7 @@ def test_index_populated_drifted_settings(self, mock_apply, mock_drift, mock_mei status_cb = Mock() warn_cb = Mock() - api._reconcile_single_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, warn_cb) + api.reconcile_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, warn_cb) mock_apply.assert_called_once_with(api.STUDIO_COURSE_INDEX_NAME, wait=True, status_cb=status_cb) # Check that drifted fields are logged @@ -545,7 +545,7 @@ def test_index_populated_wrong_pk(self, mock_reset, mock_drift, mock_meilisearch ) warn_cb = Mock() - api._reconcile_single_index(api.STUDIO_LIBRARY_INDEX_NAME, Mock(), warn_cb) + api.reconcile_index(api.STUDIO_LIBRARY_INDEX_NAME, Mock(), warn_cb) mock_reset.assert_called_once() # Should warn about data loss @@ -556,10 +556,10 @@ def test_index_populated_wrong_pk(self, mock_reset, mock_drift, mock_meilisearch @override_settings(MEILISEARCH_ENABLED=False) def test_meilisearch_disabled(self, mock_meilisearch): - """When Meilisearch is disabled, reconcile_index raises RuntimeError (from client).""" + """When Meilisearch is disabled, reconcile_indexes raises RuntimeError (from client).""" api.clear_meilisearch_client() with pytest.raises(RuntimeError): - reconcile_index() + reconcile_indexes() @skip_unless_cms @@ -573,9 +573,9 @@ def setUp(self): super().setUp() api.clear_meilisearch_client() - @patch("openedx.core.djangoapps.content.search.handlers.reconcile_index") + @patch("openedx.core.djangoapps.content.search.handlers.reconcile_indexes") def test_calls_reconcile_for_search_app(self, mock_reconcile, mock_meilisearch): - """Handler calls reconcile_index when sender is the search app.""" + """Handler calls reconcile_indexes when sender is the search app.""" sender = Mock() sender.label = ContentSearchConfig.label @@ -583,7 +583,7 @@ def test_calls_reconcile_for_search_app(self, mock_reconcile, mock_meilisearch): mock_reconcile.assert_called_once() - @patch("openedx.core.djangoapps.content.search.handlers.reconcile_index") + @patch("openedx.core.djangoapps.content.search.handlers.reconcile_indexes") def test_skips_wrong_sender(self, mock_reconcile, mock_meilisearch): """Handler does nothing when sender is a different app.""" sender = Mock() @@ -594,7 +594,7 @@ def test_skips_wrong_sender(self, mock_reconcile, mock_meilisearch): mock_reconcile.assert_not_called() @override_settings(MEILISEARCH_ENABLED=False) - @patch("openedx.core.djangoapps.content.search.handlers.reconcile_index") + @patch("openedx.core.djangoapps.content.search.handlers.reconcile_indexes") def test_skips_when_disabled(self, mock_reconcile, mock_meilisearch): """Handler does nothing when Meilisearch is disabled.""" sender = Mock() @@ -604,7 +604,7 @@ def test_skips_when_disabled(self, mock_reconcile, mock_meilisearch): mock_reconcile.assert_not_called() - @patch("openedx.core.djangoapps.content.search.handlers.reconcile_index") + @patch("openedx.core.djangoapps.content.search.handlers.reconcile_indexes") def test_catches_connection_error(self, mock_reconcile, mock_meilisearch): """Handler catches ConnectionError and logs warning.""" sender = Mock() @@ -614,7 +614,7 @@ def test_catches_connection_error(self, mock_reconcile, mock_meilisearch): # Should not raise handle_post_migrate(sender=sender) - @patch("openedx.core.djangoapps.content.search.handlers.reconcile_index") + @patch("openedx.core.djangoapps.content.search.handlers.reconcile_indexes") def test_catches_meilisearch_error(self, mock_reconcile, mock_meilisearch): """Handler catches MeilisearchError and logs warning.""" sender = Mock() @@ -624,7 +624,7 @@ def test_catches_meilisearch_error(self, mock_reconcile, mock_meilisearch): # Should not raise handle_post_migrate(sender=sender) - @patch("openedx.core.djangoapps.content.search.handlers.reconcile_index") + @patch("openedx.core.djangoapps.content.search.handlers.reconcile_indexes") def test_catches_generic_exception(self, mock_reconcile, mock_meilisearch): """Handler catches unexpected exceptions and logs warning.""" sender = Mock() @@ -656,9 +656,9 @@ def setUp(self): super().setUp() api.clear_meilisearch_client() - @patch("openedx.core.djangoapps.content.search.api.reconcile_index") + @patch("openedx.core.djangoapps.content.search.api.reconcile_indexes") def test_init_index_delegates_to_reconcile(self, mock_reconcile, mock_meilisearch): - """init_index() should delegate to reconcile_index().""" + """init_index() should delegate to reconcile_indexes().""" status_cb = Mock() warn_cb = Mock()