Skip to content

fix: dirty-check edX sync and ProductPage saves to cut Fastly purges - #3806

Open
Anas12091101 wants to merge 2 commits into
mainfrom
anas/reduce-redundant-fastly-purges
Open

fix: dirty-check edX sync and ProductPage saves to cut Fastly purges#3806
Anas12091101 wants to merge 2 commits into
mainfrom
anas/reduce-redundant-fastly-purges

Conversation

@Anas12091101

Copy link
Copy Markdown
Contributor

What are the relevant tickets?

https://github.com/mitodl/hq/issues/12614

Description (What does it do?)

Fastly surrogate-key purges were firing hundreds of times a day for changes that never happened. Root cause: two code paths call .save() unconditionally, and every CourseRun/Course save triggers a post_save purge signal.

This PR adds dirty checks so those saves (and their purges) only happen when something actually changed:

  • sync_course_runs (hourly edX sync): compares incoming edX values against the stored run and skips save() when nothing differs. This eliminates the bulk of the purges (a course with 8 live runs was emitting ~192 identical purges/day, none reflecting a change) and a pointless full-column UPDATE.
  • ProductPage.save(): only pushes the title down to the linked Course/Program (and saves it) when the title actually changed, removing the redundant purges emitted on every page save.

Screenshots (if appropriate):

How can this be tested?

The goal is to confirm that a purge is only scheduled when data actually changes. We do this in a Django shell by watching the purge task and running the sync twice — the second run should schedule zero purges.

  1. Make sure your local stack is up and migrations are applied:
    docker compose up -d
    docker compose run --rm web python manage.py migrate
  2. Open a Django shell:
    docker compose run --rm web python manage.py shell
  3. Paste the following into the shell. It creates a throwaway course run, feeds it fake edX data, and runs the sync twice. Everything is rolled back at the end, so nothing is saved to your DB:
    import uuid
    from unittest.mock import patch
    from django.db import transaction
    from edx_api.course_detail import CourseDetail
    from courses.factories import CourseRunFactory
    from courses.api import sync_course_runs
    
    suffix = uuid.uuid4().hex[:8]
    cid = f"course-v1:Demo+Purge+{suffix}"
    
    with patch("courses.signals.transaction.on_commit", side_effect=lambda cb: cb()), \
         patch("cms.tasks.queue_fastly_surrogate_key_purge.delay") as purge, \
         patch("courses.api.get_edx_api_course_list_client") as client:
        try:
            with transaction.atomic():
                run = CourseRunFactory.create(
                    courseware_id=cid,
                    course__readable_id=f"course-v1:Demo+Purge{suffix}",
                )
                client.return_value.get_courses.return_value = [CourseDetail({
                    "id": cid, "start": "2015-09-15T05:00:00Z", "end": "2015-12-31T05:00:00Z",
                    "enrollment_start": "2015-09-01T00:00:00Z", "enrollment_end": None,
                    "name": "Demo Course", "pacing": "instructor",
                })]
    
                purge.reset_mock()
                s, f = sync_course_runs([run])
                print(f"1st sync (data changed):   saved={s}  purges={purge.call_count}  -> expect saved=1 purges=1")
    
                purge.reset_mock()
                s, f = sync_course_runs([run])
                print(f"2nd sync (nothing changed): saved={s}  purges={purge.call_count}  -> expect saved=0 purges=0")
    
                raise RuntimeError("rollback")  # discard the throwaway data
        except RuntimeError:
            pass
  4. Expected output:
    1st sync (data changed):   saved=1  purges=1  -> expect saved=1 purges=1
    2nd sync (nothing changed): saved=0  purges=0  -> expect saved=0 purges=0
    
    The second line is the fix in action: identical data → no save → no Fastly purge. Before this PR, the second line would show purges=1 for a change that didn't happen.

New regression tests covering both fixes are included and run automatically in CI, so no extra steps are needed for those.

Additional Context

  • Kept .save() in the sync path (didn't switch to .update()) so CourseRun.clean() still runs — the expiration_date logic depends on it.
  • The dirty check stops bumping CourseRun.updated_on (auto_now). Nothing reads it (no admin list_display/serializer), so this is safe to lose.
  • Surrogate-key dedup and narrowing the receiver to MIT-Learn-rendered fields were considered but left out — they're largely redundant once the unnecessary saves are stopped at the source (dedup also carries a rollback footgun since Django has no on_rollback).

@github-actions

Copy link
Copy Markdown

OpenAPI Changes

Show/hide changes
## Changes for v0.yaml:
No changes detected

## Changes for v1.yaml:
No changes detected

## Changes for v2.yaml:
No changes detected

Unexpected changes? Ensure your branch is up-to-date with main (consider rebasing).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Reduces unnecessary Fastly surrogate-key purges in MITx Online by avoiding unconditional model saves in two high-frequency paths: the hourly Open edX course run sync and Wagtail ProductPage saves that mirror titles onto Course/Program records.

Changes:

  • Added a “dirty check” path for edX CourseRun sync so save() only happens when incoming values differ.
  • Updated ProductPage.save() to only save the linked Course/Program when the title actually changed.
  • Added regression tests to ensure unchanged data does not trigger additional saves/purges.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
courses/api.py Introduces _sync_course_run_from_edx and skips CourseRun.save() when edX data is unchanged.
courses/api_test.py Adds a test to verify the sync does not enqueue additional purges on a second identical pass.
cms/models.py Avoids redundant Course/Program saves from ProductPage.save() when title is unchanged.
cms/models_test.py Adds a test ensuring courseware save() is not called when a ProductPage title hasn’t changed.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread courses/api_test.py Outdated
Comment on lines +1262 to +1269
mock_course_list = mocker.patch("courses.api.get_edx_api_course_list_client")
mock_course_list.return_value.get_courses.return_value = [course_detail]

# First pass writes the edX values and enqueues one purge.
success_count, failure_count = sync_course_runs([course_run])
assert (success_count, failure_count) == (1, 0)
purge_calls_after_first = mock_purge_delay.call_count
assert purge_calls_after_first >= 1
Comment thread courses/api.py
Comment on lines 716 to 718
def sync_course_runs(runs):
"""
Sync course run dates and title from Open edX using course list API

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants