fix: prevent rerun_course from deleting an already-succeeded course [EDLYPRODUCT-8393]

cms/djangoapps/contentstore/tasks.py

@@ -174,6 +174,11 @@ def rerun_course(source_course_key_string, destination_course_key_string, user_i
 
     source_course_key = CourseKey.from_string(source_course_key_string)
     destination_course_key = CourseKey.from_string(destination_course_key_string)
+    # Tracks whether the rerun has already been marked succeeded, i.e. the destination course is a
+    # complete, usable course. Once this is True, the catch-all handler below must never delete the
+    # course out from under the user again -- any failure past this point is in auxiliary,
+    # non-essential post-processing (see the individually-guarded steps further down).
+    rerun_succeeded = False
     try:
         # deserialize the payload
         fields = deserialize_fields(fields) if fields else None
@@ -191,27 +196,55 @@ def rerun_course(source_course_key_string, destination_course_key_string, user_i
 
         # update state: Succeeded
         CourseRerunState.objects.succeeded(course_key=destination_course_key)
+        rerun_succeeded = True
 
         COURSE_RERUN_COMPLETED.send_event(
             time=datetime.now(timezone.utc),
             course=CourseData(
                 course_key=destination_course_key
             )
         )
+
+        # The steps below are auxiliary/best-effort: the rerun has already succeeded and the course
+        # is live and usable at this point, so a failure here must not flip the state back to failed
+        # or delete the course. Each is wrapped and logged independently so a failure in one doesn't
+        # prevent the others from running.
+
         # call edxval to attach videos to the rerun
-        copy_course_videos(source_course_key, destination_course_key)
+        try:
+            copy_course_videos(source_course_key, destination_course_key)
+        except Exception:  # pylint: disable=broad-except
+            LOGGER.exception(
+                'Course Rerun: failed to copy videos from %s to %s. The rerun course itself is unaffected.',
+                source_course_key, destination_course_key,
+            )
 
         # Copy RestrictedCourse
-        restricted_course = RestrictedCourse.objects.filter(course_key=source_course_key).first()
+        try:
+            restricted_course = RestrictedCourse.objects.filter(course_key=source_course_key).first()
 
-        if restricted_course:
-            country_access_rules = CountryAccessRule.objects.filter(restricted_course=restricted_course)
-            new_restricted_course = clone_instance(restricted_course, {'course_key': destination_course_key})
-            for country_access_rule in country_access_rules:
-                clone_instance(country_access_rule, {'restricted_course': new_restricted_course})
+            if restricted_course:
+                country_access_rules = CountryAccessRule.objects.filter(restricted_course=restricted_course)
+                new_restricted_course = clone_instance(restricted_course, {'course_key': destination_course_key})
+                for country_access_rule in country_access_rules:
+                    clone_instance(country_access_rule, {'restricted_course': new_restricted_course})
+        except Exception:  # pylint: disable=broad-except
+            LOGGER.exception(
+                'Course Rerun: failed to clone RestrictedCourse/CountryAccessRule from %s to %s. '
+                'The rerun course itself is unaffected.',
+                source_course_key, destination_course_key,
+            )
+
+        try:
+            org_data = ensure_organization(destination_course_key.org)
+            add_organization_course(org_data, destination_course_key)
+        except Exception:  # pylint: disable=broad-except
+            LOGGER.exception(
+                'Course Rerun: failed to link destination course %s to organization %s. The course exists '
+                'but may not be linked to its organization -- needs investigation.',
+                destination_course_key, destination_course_key.org,
+            )
 
-        org_data = ensure_organization(destination_course_key.org)
-        add_organization_course(org_data, destination_course_key)
         return "succeeded"
 
     except DuplicateCourseError:
@@ -226,12 +259,21 @@ def rerun_course(source_course_key_string, destination_course_key_string, user_i
         CourseRerunState.objects.failed(course_key=destination_course_key)
         LOGGER.exception('Course Rerun Error')
 
-        try:
-            # cleanup any remnants of the course
-            modulestore().delete_course(destination_course_key, user_id)
-        except ItemNotFoundError:
-            # it's possible there was an error even before the course block was created
-            pass
+        if not rerun_succeeded:
+            try:
+                # cleanup any remnants of the course
+                modulestore().delete_course(destination_course_key, user_id)
+            except ItemNotFoundError:
+                # it's possible there was an error even before the course block was created
+                pass
+        else:
+            # The course was already fully cloned and marked succeeded before this exception was
+            # raised -- it's a valid, usable course. Do not delete it; just log for investigation.
+            LOGGER.exception(
+                'Course Rerun: destination course %s already succeeded before this error was raised; '
+                'preserving the course instead of deleting it.',
+                destination_course_key,
+            )
 
         return "exception: " + str(exc)
 

cms/djangoapps/contentstore/tests/test_clone_course.py

@@ -141,3 +141,91 @@ def test_rerun_course(self):
                 course_key=split_course4_id,
                 state=CourseRerunUIStateManager.State.FAILED
             )
+
+    def test_rerun_course_auxiliary_step_failure_does_not_delete_course(self):
+        """
+        Regression test for EDLYPRODUCT-8393: if an auxiliary, post-succeeded step (e.g. linking the
+        destination course to its organization) raises, the rerun must still be reported as
+        "succeeded" and the already-cloned course must NOT be deleted.
+        """
+        org = 'edX'
+        course_number = 'CS101'
+        course_run = '2015_Q1'
+        display_name = 'rerun'
+
+        split_course = CourseFactory.create(
+            org=org,
+            number=course_number,
+            run=course_run,
+            display_name=display_name,
+            default_store=ModuleStoreEnum.Type.split
+        )
+
+        rerun_course_id = CourseLocator(org=org, course=course_number, run="rerun_aux_failure")
+        fields = {'display_name': 'rerun'}
+        CourseRerunState.objects.initiated(split_course.id, rerun_course_id, self.user, fields['display_name'])
+
+        with patch(
+            'cms.djangoapps.contentstore.tasks.add_organization_course',
+            Mock(side_effect=Exception('org link boom!')),
+        ):
+            result = rerun_course.delay(
+                str(split_course.id), str(rerun_course_id), self.user.id,
+                json.dumps(fields, cls=EdxJSONEncoder)
+            )
+
+        # The failure in the auxiliary org-linking step must not surface as a task failure...
+        self.assertEqual(result.get(), "succeeded")
+        # ...the rerun state must remain SUCCEEDED...
+        rerun_state = CourseRerunState.objects.find_first(course_key=rerun_course_id)
+        self.assertEqual(rerun_state.state, CourseRerunUIStateManager.State.SUCCEEDED)
+        # ...and the course itself must still exist and be usable.
+        self.assertIsNotNone(self.store.get_course(rerun_course_id), "Course was deleted after an auxiliary failure")
+
+    def test_rerun_course_post_succeeded_failure_preserves_course(self):
+        """
+        Regression test for EDLYPRODUCT-8393: even a failure that isn't in one of the individually
+        wrapped auxiliary steps -- as long as it happens after the rerun has already been marked
+        succeeded -- must not delete the already-cloned, already-usable course. The rerun is still
+        reported/marked as failed (something genuinely went wrong and should be surfaced), but the
+        course itself is preserved rather than destroyed.
+        """
+        org = 'edX'
+        course_number = 'CS101'
+        course_run = '2015_Q1'
+        display_name = 'rerun'
+
+        split_course = CourseFactory.create(
+            org=org,
+            number=course_number,
+            run=course_run,
+            display_name=display_name,
+            default_store=ModuleStoreEnum.Type.split
+        )
+
+        rerun_course_id = CourseLocator(org=org, course=course_number, run="rerun_post_succeeded_failure")
+        fields = {'display_name': 'rerun'}
+        CourseRerunState.objects.initiated(split_course.id, rerun_course_id, self.user, fields['display_name'])
+
+        with patch(
+            'cms.djangoapps.contentstore.tasks.COURSE_RERUN_COMPLETED.send_event',
+            Mock(side_effect=Exception('event bus boom!')),
+        ):
+            result = rerun_course.delay(
+                str(split_course.id), str(rerun_course_id), self.user.id,
+                json.dumps(fields, cls=EdxJSONEncoder)
+            )
+
+        # The task itself surfaces the failure...
+        self.assertIn("exception: ", result.get())
+        # ...and the rerun state is (correctly) marked failed, since something genuinely broke...
+        CourseRerunState.objects.find_first(
+            course_key=rerun_course_id,
+            state=CourseRerunUIStateManager.State.FAILED
+        )
+        # ...but the course was already fully cloned and marked succeeded before the failure, so it
+        # must NOT be deleted.
+        self.assertIsNotNone(
+            self.store.get_course(rerun_course_id),
+            "Course was deleted even though it had already succeeded before the failure"
+        )

cms/djangoapps/contentstore/tests/test_tasks.py

@@ -22,6 +22,7 @@
 
 from cms.djangoapps.contentstore.tests.test_libraries import LibraryTestCase
 from cms.djangoapps.contentstore.tests.utils import CourseTestCase
+from common.djangoapps.course_action_state.managers import CourseRerunUIStateManager
 from common.djangoapps.course_action_state.models import CourseRerunState
 from common.djangoapps.student.tests.factories import UserFactory
 from openedx.core.djangoapps.course_apps.toggles import EXAMS_IDA
@@ -209,6 +210,34 @@ def test_success_different_org(self):
         self.assertEqual(OrganizationCourse.objects.count(), 2)
         OrganizationCourse.objects.get(course_id=new_course_id, organization__short_name='neworg')
 
+    def test_auxiliary_step_failure_does_not_delete_course(self):
+        """
+        Regression test for EDLYPRODUCT-8393.
+
+        If an auxiliary/best-effort post-succeeded step (video copy, RestrictedCourse clone, or
+        organization linking) raises, the task must still report "succeeded", the CourseRerunState
+        must remain SUCCEEDED, and the already-cloned course must remain retrievable from the
+        modulestore -- it must NOT be deleted.
+        """
+        old_course_key = self.course.id
+        new_course_key = CourseLocator(org=old_course_key.org, course=old_course_key.course, run='rerun')
+
+        CourseRerunState.objects.initiated(old_course_key, new_course_key, self.user, 'Test Re-run')
+
+        with mock.patch(
+            'edxval.api.copy_course_videos',
+            side_effect=Exception('edxval boom!'),
+        ):
+            result = rerun_course(str(old_course_key), str(new_course_key), self.user.id)
+
+        self.assertEqual(result, "succeeded")
+        rerun_state = CourseRerunState.objects.find_first(course_key=new_course_key)
+        self.assertEqual(rerun_state.state, CourseRerunUIStateManager.State.SUCCEEDED)
+        self.assertIsNotNone(
+            modulestore().get_course(new_course_key),
+            "Course was deleted after an auxiliary (video copy) failure",
+        )
+
 
 @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE)
 class RegisterExamsTaskTestCase(CourseTestCase):  # pylint: disable=missing-class-docstring