diff --git a/xmodule/modulestore/split_mongo/mongo_connection.py b/xmodule/modulestore/split_mongo/mongo_connection.py index a818c04ede62..e52de5488231 100644 --- a/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/xmodule/modulestore/split_mongo/mongo_connection.py @@ -24,7 +24,7 @@ from edx_django_utils.cache import RequestCache # Import this just to export it -from pymongo.errors import DuplicateKeyError # pylint: disable=unused-import # noqa: F401 +from pymongo.errors import DuplicateKeyError from common.djangoapps.split_modulestore_django.models import SplitModulestoreCourseIndex from openedx.core.lib.cache_utils import request_cached @@ -741,7 +741,28 @@ def insert_course_index(self, course_index, course_context=None): # pylint: dis # Also write to MongoDB, so we can switch back to using it if this new MySQL version doesn't work well. # NOTE: This is REQUIRED for pruning (structures.py) to run safely. Don't remove this write until # pruning is modified to read from SplitModulestoreCourseIndex to get active versions. - super().insert_course_index(course_index, course_context) + try: + super().insert_course_index(course_index, course_context) + except DuplicateKeyError: + # A MongoDB doc already exists for this org/course/run, but MySQL is the source of truth for + # which courses exist and new_index.save() above has already succeeded, so we know no MySQL row + # existed for this key. The MongoDB doc is therefore stale: it is left behind whenever an + # earlier request got this far and then had its MySQL transaction rolled back, since MongoDB + # writes are not covered by ATOMIC_REQUESTS. Drop it and retry, otherwise this course could + # never be created again -- every attempt would fail here identically, and the rollback leaves + # no MySQL trace to explain why. + stale_docs = self.course_index.delete_many({ + 'org': course_index['org'], + 'course': course_index['course'], + 'run': course_index['run'], + }).deleted_count + log.warning( + "Removed %d stale MongoDB active_versions doc(s) for %s/%s/%s which had no " + "SplitModulestoreCourseIndex row. This usually means an earlier attempt to create this " + "course wrote to MongoDB and then had its MySQL transaction rolled back.", + stale_docs, course_index['org'], course_index['course'], course_index['run'], + ) + super().insert_course_index(course_index, course_context) def update_course_index(self, course_index, from_index=None, course_context=None): # pylint: disable=arguments-differ """ diff --git a/xmodule/modulestore/tests/test_split_modulestore.py b/xmodule/modulestore/tests/test_split_modulestore.py index 1d1c54859b4e..18131459a912 100644 --- a/xmodule/modulestore/tests/test_split_modulestore.py +++ b/xmodule/modulestore/tests/test_split_modulestore.py @@ -18,6 +18,7 @@ from opaque_keys.edx.locator import BlockUsageLocator, CourseKey, CourseLocator, LocalId from xblock.fields import Date, Reference, ReferenceList, ReferenceValueDict, Timedelta +from common.djangoapps.split_modulestore_django.models import SplitModulestoreCourseIndex from openedx.core.djangolib.testing.utils import CacheIsolationMixin from openedx.core.lib import tempdir from openedx.core.lib.tests import attr @@ -1643,6 +1644,52 @@ def test_simple_creation(self): assert len(new_course.grading_policy['GRADER']) == 4 self.assertDictEqual(new_course.grade_cutoffs, {"Pass": 0.5}) # noqa: PT009 + def test_creation_recovers_from_orphaned_mongo_index(self): + """ + Creating a course must succeed even if MongoDB's active_versions still holds a doc for that + org/course/run while MySQL has no SplitModulestoreCourseIndex row. + + This state arises whenever an earlier attempt to create the course wrote the MongoDB doc and then + had its MySQL transaction rolled back -- MongoDB writes are not covered by ATOMIC_REQUESTS. MySQL is + the source of truth for which courses exist, so `has_course()` reports the course as absent and + creation is attempted again. Before this was fixed, the follower write to MongoDB used insert_one() + and raised DuplicateKeyError against the UNIQUE(org, course, run) index on active_versions, so the + course could never be created again -- every retry failed identically, and the rollback left no + MySQL trace to explain why. + """ + store = modulestore() + # Provision the UNIQUE(org, course, run) index on active_versions that deployments get from the + # `ensure_indexes` management command. Without it MongoDB silently accepts a second doc for the + # same course instead of raising, which is not the behaviour we need to guard against here. + store.ensure_indexes() + org, course, run = 'orphan_org', 'orphan_course', 'orphan_run' + created = store.create_course(org, course, run, TEST_USER_ID, BRANCH_NAME_DRAFT) + course_key = created.location.course_key + + # Simulate the rolled-back request: drop only the MySQL row, leaving the MongoDB doc orphaned. + # Deliberately bypasses delete_course_index(), which would clear both stores. + SplitModulestoreCourseIndex.objects.filter( + course_id=course_key.for_branch(None).version_agnostic() + ).delete() + db_connection = store.db_connection + mongo_query = {'org': org, 'course': course, 'run': run} + assert db_connection.course_index.count_documents(mongo_query) == 1, "expected an orphaned Mongo doc" + assert store.has_course(CourseLocator(org, course, run)) is None, "MySQL should report absence" + + # Creating the course again must now succeed rather than raising DuplicateKeyError. + recreated = store.create_course(org, course, run, TEST_USER_ID, BRANCH_NAME_DRAFT) + recreated_key = recreated.location.course_key + + # The stale doc is replaced, not duplicated, and the two stores agree again. + assert db_connection.course_index.count_documents(mongo_query) == 1 + mongo_doc = db_connection.course_index.find_one(mongo_query) + mysql_row = SplitModulestoreCourseIndex.objects.get( + course_id=recreated_key.for_branch(None).version_agnostic() + ) + assert str(mongo_doc['_id']) == mysql_row.objectid + assert mongo_doc['versions'][BRANCH_NAME_DRAFT] == \ + store.get_course_index_info(recreated_key)['versions'][BRANCH_NAME_DRAFT] + def test_cloned_course(self): """ Test making a course which points to an existing draft and published but not making any changes to either.