Skip to content

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

Open
muhammadali286 wants to merge 1 commit into
develop-ulmofrom
muhammadali286/EDLYPRODUCT-8393-rerun-course-cleanup-fix
Open

fix: prevent rerun_course from deleting an already-succeeded course [EDLYPRODUCT-8393]#755
muhammadali286 wants to merge 1 commit into
develop-ulmofrom
muhammadali286/EDLYPRODUCT-8393-rerun-course-cleanup-fix

Conversation

@muhammadali286

Copy link
Copy Markdown

Summary

Hardens rerun_course (cms/djangoapps/contentstore/tasks.py) so it never deletes a destination course that has already been fully cloned and marked succeeded, and makes the auxiliary post-succeeded steps (video copy, RestrictedCourse/CountryAccessRule clone, organization linking) individually non-fatal.

Motivation (EDLYPRODUCT-8393): a client reran a course with an org change. The rerun completed, the course was confirmed usable (a Course Team member was successfully added) -- then it silently disappeared and Studio Home showed "Configuration error". The only code path that deletes a rerun's destination course is the catch-all exception handler in rerun_course, so something in the post-succeeded() tail of that function (video copy / RestrictedCourse clone / org linking / event dispatch) must have raised.

We have not identified the exact operation that threw, and this PR does not claim to. QA could not reproduce the deletion/"Configuration error" on staging (only a transient, unrelated "Not found" on reload for a larger course, which cleared itself). Given that, the fix is deliberately defensive: it protects the course regardless of which post-succeeded step fails, rather than patching one specific suspected cause.

This is stock upstream edx-platform behavior (confirmed identical on openedx/edx-platform master) with no existing upstream fix for this bug. (Note: PR/issue openedx#38840/openedx#38826 upstream is a different rerun bug, about the authz.enable_course_authoring flag failing before any course exists -- not related.) Edly should consider upstreaming this hardening.

What changed

  • Added a rerun_succeeded guard, set immediately after CourseRerunState.objects.succeeded(...) is called. The catch-all except Exception handler now only calls modulestore().delete_course(...) when this is still False (i.e. the clone never completed / never got marked succeeded -- genuine partial garbage). Once the guard is True, a later failure is logged but the course is preserved.
  • Wrapped each of the three auxiliary/best-effort post-succeeded steps -- copy_course_videos, the RestrictedCourse/CountryAccessRule clone, and ensure_organization/add_organization_course -- in its own try/except with a distinct diagnostic log message. A failure in one no longer prevents the others from running and no longer flips the rerun to failed or deletes the course.
  • DuplicateCourseError handling and pre-/mid-clone failure cleanup (delete the partial course, mark failed) are unchanged.

Behavior change to be aware of

Because the course is no longer deleted after a post-succeeded auxiliary failure, re-running with the exact same destination org+number+run a second time will now hit DuplicateCourseError (reported as "duplicate course") instead of silently proceeding against a course that no longer exists. This is the correct/intended outcome -- the first course is real and still there -- but it's a visible behavior change from today, where a failed auxiliary step would previously vanish the course and let a retry recreate it from scratch under the same key.

Tests

  • cms/djangoapps/contentstore/tests/test_clone_course.py:
    • test_rerun_course_auxiliary_step_failure_does_not_delete_course -- forces add_organization_course to raise; asserts the task returns "succeeded", CourseRerunState is SUCCEEDED, and the course is retrievable.
    • test_rerun_course_post_succeeded_failure_preserves_course -- forces a failure in COURSE_RERUN_COMPLETED.send_event (after succeeded(), outside the individually-wrapped steps); asserts the task reports the failure and the state is FAILED, but the course is not deleted.
    • Pre-existing test_rerun_course (failure during clone_course -> course deleted, state FAILED) is unchanged and still covers the genuine-partial-clone cleanup path.
  • cms/djangoapps/contentstore/tests/test_tasks.py: test_auxiliary_step_failure_does_not_delete_course -- focused unit test forcing copy_course_videos to raise, same assertions.

Local verification caveat

This local Tutor dev environment has a pre-existing, unrelated issue: any test that invokes a @shared_task (confirmed on both rerun_course and the unrelated export_olx task, and reproducible on unmodified develop-ulmo) currently hits InvalidCacheBackendError: The connection 'general' doesn't exist from Django's cache framework during/after task execution. This is independent of this change (verified on the pre-rebase baseline too) and blocks a fully green local pytest run for these suites in this container specifically. Across all runs, zero AssertionErrors were raised -- only this cache error -- and captured logs confirm the new guard/logging fires exactly as designed (e.g. "...already succeeded before this error was raised; preserving the course instead of deleting it."). CI is the real gate for this PR; recommend treating a green CI run here as authoritative over the local repro.

Out-of-scope follow-up observed

While investigating, noticed handle_reindex_on_signal in openedx/core/djangoapps/content/search/handlers.py (the receiver for COURSE_RERUN_COMPLETED/COURSE_IMPORT_COMPLETED) is missing the @only_if_meilisearch_enabled guard that every sibling handler in that file has, so it will raise if Meilisearch is disabled/unreachable when a rerun completes. This fires synchronously off the same COURSE_RERUN_COMPLETED.send_event() call inside rerun_course, so a Meilisearch hiccup there would land in exactly the failure window this PR now protects -- plausibly consistent with QA's observation that a larger course showed transient issues, though this is speculative and not confirmed. Filing separately rather than expanding this PR's scope.

…EDLYPRODUCT-8393]

rerun_course() marks the destination course's CourseRerunState as
succeeded well before all of its work is done (video copy, restricted
course/country rules clone, organization linking). If any of those
auxiliary, best-effort steps raised, the catch-all exception handler
would flip the state back to failed and delete the destination course
-- even though it was already a complete, usable course.

- Track whether the rerun has already been marked succeeded. The
  catch-all handler now only deletes the destination course when
  that hasn't happened yet (a genuinely partial/garbage clone);
  once succeeded, the course is preserved and the failure is logged
  instead.
- Wrap video copying, RestrictedCourse/CountryAccessRule cloning, and
  organization linking each in their own try/except so a failure in
  one no longer prevents the others from running or endangers the
  course, with a distinct diagnostic log message per step.

Motivated by an incident where a course rerun was confirmed usable and
then silently deleted, leaving Studio Home showing a "Configuration
error". The exact step that raised in that incident was not
conclusively identified; this hardens rerun_course against the whole
class of post-succeeded failures regardless of which step throws. This
is stock upstream edx-platform behavior with no existing upstream fix;
worth upstreaming.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant