From 7370344318130d82a395a1aaf5f440e8a6305293 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Wed, 5 Aug 2026 16:22:39 -0400 Subject: [PATCH 01/12] Harden GH Actions supply chain: add zizmor static analysis + 7-day dependency cool-down (#3712) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Harden GitHub Actions supply-chain security with zizmor and delayed dependency updates Recent supply-chain attacks have exploited both malicious/misconfigured GitHub Actions workflows and newly-published malicious package versions slipping into CI before the ecosystem catches them. This adds two independent mitigations: - Run zizmor (static analysis for GitHub Actions workflows) both in CI, on any push/PR that touches .github/workflows/**, and as a local pre-commit hook, so workflow misconfigurations (e.g. injectable expressions, overly broad permissions, unpinned actions) are caught before merge. - Set uv's exclude-newer to a rolling "7d" window in pyproject.toml, so `uv lock` won't resolve a dependency version until it has been published for at least 7 days. This gives the community time to flag newly-introduced malicious or broken releases before this repo picks them up. Confirmed via `uv lock` that this is stored as a self-updating exclude-newer-span (P7D) in uv.lock rather than a fixed cutoff date. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KDdYm3kAV35FottKdRFXo2 * Exempt in-house packages from exclude-newer cool-down The exclude-newer = "7d" setting added in a prior commit guards against newly-published third-party malicious/vulnerable packages by delaying resolution to versions at least 7 days old. That protection isn't needed for packages MIT ODL owns and iterates on rapidly (ol-concourse, django-aqueduct, open-edx-plugins, ol-django) — the 7-day delay only slows down consuming our own releases without adding any security benefit. Add [tool.uv.exclude-newer-package] with "0d" overrides for each in-house package so uv always resolves them to the latest available version while the global 7-day cool-down still applies to everything else. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KDdYm3kAV35FottKdRFXo2 * Fix zizmor high-severity findings and align pre-commit gate with CI The zizmor CI check was failing on 3 high-severity unpinned-uses findings (astral-sh/setup-uv@v7, openapi-generators/openapitools-generator-action@v1 x2). Ran `zizmor --fix=all` to pin those refs to SHAs with version comments, plus incidental artipacked fixes (persist-credentials: false) and a ref-version-mismatch correction the same pass caught. Also: - Add required-version = ">=0.9.17" to [tool.uv] in pyproject.toml. That's the uv release that introduced relative-duration exclude-newer support (used by this table's exclude-newer = "7d"); older uv now fails loudly with a version-mismatch error instead of silently mishandling the setting. `uv lock` re-run confirms this is a no-op for the lockfile. - Add args to the pre-commit zizmor hook to match the CI workflow's own --min-severity=high --min-confidence=medium gate, so pre-commit.ci and CI agree on what blocks a PR instead of pre-commit.ci enforcing a stricter, unfiltered threshold. --no-progress is kept since args: overrides all default args. Remaining lower-severity zizmor findings (10 medium excessive-permissions, below the CI gate) and an unrelated uv.lock jinxed/ansicon marker regression are tracked in mitodl/mit-learn#3715 rather than bundled here. * fix(ci): drop redundant pull_request trigger from zizmor workflow Both push and pull_request fired on every PR commit for the same path-scoped check, running zizmor twice per push. push alone still covers PR branch commits. Co-Authored-By: Claude Sonnet 5 * fix(ci): add back pull_request trigger for zizmor workflow, scope push to main Bot reviewers (Copilot, Sentry) correctly flagged that a push-only trigger misses fork-based PRs and can't act as a required merge-gate status check. Scoping push to the default branch avoids the original double-run problem (push firing on every commit to a same-repo PR branch, redundant with pull_request) while restoring PR-gate coverage. Co-Authored-By: Claude Sonnet 5 * chore: trim exclude-newer-package allowlist to this repo's actual dependencies The allowlist exempting in-house MIT ODL packages from the 7-day uv dependency cool-down was copy-pasted org-wide, unpruned. Reviewers on two separate PRs independently flagged the same thing: most of the ~44 entries (mostly Open edX plugins) aren't dependencies of this repo at all. Trimmed to the intersection with this repo's own dependency closure (uv.lock's locked package set, or pyproject.toml's declared deps where no lockfile exists). Co-Authored-By: Claude Sonnet 5 * fix(deps): regenerate uv.lock to match the trimmed exclude-newer-package allowlist The previous commit edited pyproject.toml's allowlist without regenerating uv.lock, so 'uv sync --locked' correctly rejected the mismatch in CI. Co-Authored-By: Claude Sonnet 5 * fix(ci): lower zizmor gate to min-severity=medium, fix resulting findings - ci.yml: add workflow-level `permissions: contents: read`. All six jobs (python-tests, javascript-tests, build-nextjs-container, build-storybook, openapi-generated-client-check-v0/v1) only checkout, build, or test — none push, publish, or comment — so a single read-only default resolves both the workflow-level and per-job excessive-permissions findings. - openapi-diff.yml: add job-level `permissions: contents: read, pull-requests: write` to the openapi-diff job. It posts/updates a PR comment via peter-evans/find-comment and peter-evans/create-or-update-comment using secrets.GITHUB_TOKEN, which requires pull-requests: write. - publish-pages.yml: add workflow-level `permissions: contents: read`. The build job only checks out and builds Storybook, so it picks up the read-only default; the deploy job already declares its own narrower permissions (pages: write, id-token: write) which continue to override. - actions-static-analysis.yml: lower zizmor gate from min-severity=high to min-severity=medium (min-confidence unchanged at medium). - .pre-commit-config.yaml: lower the local zizmor hook's --min-severity from high to medium to match CI. Verified with `zizmor --min-severity=medium --min-confidence=medium .github/workflows/`: no findings to report. Co-Authored-By: Claude Sonnet 5 --------- Co-authored-by: Claude Sonnet 5 --- .github/workflows/actions-static-analysis.yml | 34 ++++++++++++++ .github/workflows/ci.yml | 45 +++++++++++++------ .github/workflows/openapi-diff.yml | 9 +++- .github/workflows/publish-pages.yml | 9 +++- .pre-commit-config.yaml | 5 +++ pyproject.toml | 8 ++++ uv.lock | 12 ++++- 7 files changed, 103 insertions(+), 19 deletions(-) create mode 100644 .github/workflows/actions-static-analysis.yml diff --git a/.github/workflows/actions-static-analysis.yml b/.github/workflows/actions-static-analysis.yml new file mode 100644 index 0000000000..bb72567963 --- /dev/null +++ b/.github/workflows/actions-static-analysis.yml @@ -0,0 +1,34 @@ +name: GitHub Actions Static Analysis + +on: + push: + branches: + - "main" + paths: + - ".github/workflows/**" + pull_request: + paths: + - ".github/workflows/**" + +permissions: {} + +jobs: + zizmor: + name: Run zizmor + runs-on: ubuntu-latest + permissions: + contents: read + actions: read + steps: + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Run zizmor 🌈 + uses: zizmorcore/zizmor-action@3dc1ecc9bcb9e94e9b2c709687979e1298497054 # v0.6.2 + with: + inputs: ".github/workflows/" + min-severity: medium + min-confidence: medium + advanced-security: false diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 827325367f..7ae468c785 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,5 +1,9 @@ name: CI on: [push] + +permissions: + contents: read + jobs: python-tests: runs-on: ubuntu-24.04 @@ -28,7 +32,9 @@ jobs: - 6379:6379 steps: - - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 + - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false - name: update apt run: sudo apt-get update -y @@ -37,7 +43,7 @@ jobs: run: cat Aptfile | sudo xargs apt-get install - name: Install uv - uses: astral-sh/setup-uv@v7 + uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7.6.0 with: enable-cache: true @@ -84,8 +90,10 @@ jobs: javascript-tests: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 - - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 + - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false + - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0 with: node-version: "^24.0.0" cache: yarn @@ -117,10 +125,11 @@ jobs: uses: SimenB/github-actions-cpu-cores@97ba232459a8e02ff6121db9362b09661c875ab8 # v2 - name: Tests - run: yarn test --max-workers ${{ steps.cpu-cores.outputs.count }} + run: yarn test --max-workers ${STEPS_CPU_CORES_OUTPUTS_COUNT} env: CODECOV: true NODE_ENV: test + STEPS_CPU_CORES_OUTPUTS_COUNT: ${{ steps.cpu-cores.outputs.count }} - name: Upload coverage to CodeCov uses: codecov/codecov-action@671740ac38dd9b0130fbe1cec585b89eea48d3de # v5.5.2 @@ -132,7 +141,9 @@ jobs: runs-on: ubuntu-24.04 steps: - name: Checkout - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 + uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false - name: Build the Docker image env: @@ -147,9 +158,11 @@ jobs: runs-on: ubuntu-24.04 steps: - name: Checkout - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 + uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false - - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 + - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0 with: node-version: "^24.0.0" cache: yarn @@ -171,8 +184,10 @@ jobs: GENERATOR_OUTPUT_DIR_VC: ./frontends/api/src/generated/v0 runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 - - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 + - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false + - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0 with: node-version: "^24.0.0" cache: yarn @@ -182,7 +197,7 @@ jobs: run: yarn install --immutable - name: Generate Fresh API Client - uses: openapi-generators/openapitools-generator-action@v1 + uses: openapi-generators/openapitools-generator-action@515e8d70646f72de54d61c7f54b9447b7a1176f0 # v1 with: generator: typescript-axios openapi-file: $OPENAPI_SCHEMA @@ -210,8 +225,10 @@ jobs: GENERATOR_OUTPUT_DIR_VC: ./frontends/api/src/generated/v1 runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 - - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 + - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false + - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0 with: node-version: "^24.0.0" cache: yarn @@ -221,7 +238,7 @@ jobs: run: yarn install --immutable - name: Generate Fresh API Client - uses: openapi-generators/openapitools-generator-action@v1 + uses: openapi-generators/openapitools-generator-action@515e8d70646f72de54d61c7f54b9447b7a1176f0 # v1 with: generator: typescript-axios openapi-file: $OPENAPI_SCHEMA diff --git a/.github/workflows/openapi-diff.yml b/.github/workflows/openapi-diff.yml index cbb7770a22..b0c571520d 100644 --- a/.github/workflows/openapi-diff.yml +++ b/.github/workflows/openapi-diff.yml @@ -3,17 +3,22 @@ on: [pull_request] jobs: openapi-diff: runs-on: ubuntu-24.04 + permissions: + contents: read + pull-requests: write steps: - name: Checkout HEAD - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 + uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 with: ref: ${{ github.head_ref }} path: head + persist-credentials: false - name: Checkout BASE - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 + uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 with: ref: ${{ github.base_ref }} path: base + persist-credentials: false - name: Generate oasdiff summary id: oasdif_changelog run: | diff --git a/.github/workflows/publish-pages.yml b/.github/workflows/publish-pages.yml index aa123c6ca2..fb86ab697a 100644 --- a/.github/workflows/publish-pages.yml +++ b/.github/workflows/publish-pages.yml @@ -8,14 +8,19 @@ on: # Allows you to run this workflow manually from the Actions tab workflow_dispatch: +permissions: + contents: read + jobs: build: runs-on: ubuntu-24.04 steps: - name: Checkout - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 + uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + persist-credentials: false - - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 + - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0 with: node-version: "^24.0.0" cache: yarn diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index e538e2bd3a..edaf1ca459 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -122,6 +122,11 @@ repos: hooks: - id: shellcheck args: ["--severity=warning"] + - repo: https://github.com/zizmorcore/zizmor-pre-commit + rev: v1.29.0 + hooks: + - id: zizmor + args: [--no-progress, --min-severity=medium, --min-confidence=medium] - repo: local hooks: - id: drf-serializer-orm-check diff --git a/pyproject.toml b/pyproject.toml index bbf3d7178b..49991ffd08 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -163,6 +163,14 @@ dev = [ package = false default-groups = "all" override-dependencies = ["setuptools<80"] +exclude-newer = "7d" +required-version = ">=0.9.17" + +[tool.uv.exclude-newer-package] +mitol-django-common = "0d" +mitol-django-observability = "0d" +mitol-django-scim = "0d" +mitol-drf-lint = "0d" [tool.uv.build-backend] module-root = "" diff --git a/uv.lock b/uv.lock index 8f99f9d1a9..c1b80213dc 100644 --- a/uv.lock +++ b/uv.lock @@ -2,6 +2,16 @@ version = 1 revision = 3 requires-python = "==3.12.*" +[options] +exclude-newer = "0001-01-01T00:00:00Z" # This has no effect and is included for backwards compatibility when using relative exclude-newer values. +exclude-newer-span = "P7D" + +[options.exclude-newer-package] +mitol-drf-lint = { timestamp = "0001-01-01T00:00:00Z", span = "PT0S" } +mitol-django-observability = { timestamp = "0001-01-01T00:00:00Z", span = "PT0S" } +mitol-django-scim = { timestamp = "0001-01-01T00:00:00Z", span = "PT0S" } +mitol-django-common = { timestamp = "0001-01-01T00:00:00Z", span = "PT0S" } + [manifest] overrides = [{ name = "setuptools", specifier = "<80" }] @@ -1861,7 +1871,7 @@ name = "jinxed" version = "1.3.0" source = { registry = "https://pypi.org/simple" } dependencies = [ - { name = "ansicon", marker = "sys_platform == 'win32'" }, + { name = "ansicon" }, ] sdist = { url = "https://files.pythonhosted.org/packages/20/d0/59b2b80e7a52d255f9e0ad040d2e826342d05580c4b1d7d7747cfb8db731/jinxed-1.3.0.tar.gz", hash = "sha256:1593124b18a41b7a3da3b078471442e51dbad3d77b4d4f2b0c26ab6f7d660dbf", size = 80981, upload-time = "2024-07-31T22:39:18.854Z" } wheels = [ From d9ed422976d4beec2f7a0c376cb01583c08032c9 Mon Sep 17 00:00:00 2001 From: Zaman Afzal Date: Thu, 6 Aug 2026 15:35:52 +0500 Subject: [PATCH 02/12] Use CMS Certificate Title for program LinkedIn "Add to Profile" (#3518) * feat: use CMS Certificate Title for program LinkedIn "Add to Profile" The LinkedIn "Add to Profile" link for program certificates now uses the CMS "Certificate Title" (product_name) as the credential name, matching the title shown on the certificate, and falls back to the program title when product_name is unset. Course certificates are unchanged. --- .../main/src/common/certificateUtils.test.ts | 44 +++++++++++++++++++ frontends/main/src/common/certificateUtils.ts | 15 +++++-- 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/frontends/main/src/common/certificateUtils.test.ts b/frontends/main/src/common/certificateUtils.test.ts index a0e3d0b0d7..1a358a2db2 100644 --- a/frontends/main/src/common/certificateUtils.test.ts +++ b/frontends/main/src/common/certificateUtils.test.ts @@ -1,9 +1,12 @@ import { + CertificateType, getCertificateBadgeLines, getCertificateBadgeTypography, getCertificateInfo, + getCertificateLinkedInUrl, getCertificateTitle, } from "./certificateUtils" +import { factories } from "api/test-utils" describe("getCertificateInfo", () => { it("returns default certificate label when no program type is provided", () => { @@ -148,3 +151,44 @@ describe("getCertificateBadgeTypography", () => { ) }) }) + +describe("getCertificateLinkedInUrl", () => { + const pageUrl = "https://example.com/certificate/program/abc" + + it("uses the CMS product name for program certificates", () => { + const certificate = factories.mitxonline.programCertificate() + certificate.certificate_page.product_name = "Universal AI" + certificate.program.title = "Fundamentals of Programming and ML" + + const url = new URL( + getCertificateLinkedInUrl(CertificateType.Program, certificate, pageUrl), + ) + + expect(url.searchParams.get("name")).toBe("Universal AI") + }) + + it("falls back to the program title when product name is empty", () => { + const certificate = factories.mitxonline.programCertificate() + certificate.certificate_page.product_name = "" + certificate.program.title = "Fundamentals of Programming and ML" + + const url = new URL( + getCertificateLinkedInUrl(CertificateType.Program, certificate, pageUrl), + ) + + expect(url.searchParams.get("name")).toBe( + "Fundamentals of Programming and ML", + ) + }) + + it("uses the course title for course certificates", () => { + const certificate = factories.mitxonline.courseCertificate() + certificate.course_run.course.title = "Intro to Python" + + const url = new URL( + getCertificateLinkedInUrl(CertificateType.Course, certificate, pageUrl), + ) + + expect(url.searchParams.get("name")).toBe("Intro to Python") + }) +}) diff --git a/frontends/main/src/common/certificateUtils.ts b/frontends/main/src/common/certificateUtils.ts index d40fdacb74..218985f106 100644 --- a/frontends/main/src/common/certificateUtils.ts +++ b/frontends/main/src/common/certificateUtils.ts @@ -253,10 +253,17 @@ export const getCertificateLinkedInUrl = ( certificateData: V2ProgramCertificate | V2CourseRunCertificate, pageUrl: string, ): string => { - const credentialName = - certificateType === CertificateType.Course - ? (certificateData as V2CourseRunCertificate).course_run.course.title - : (certificateData as V2ProgramCertificate).program.title + let credentialName: string + if (certificateType === CertificateType.Course) { + credentialName = (certificateData as V2CourseRunCertificate).course_run + .course.title + } else { + const programCert = certificateData as V2ProgramCertificate + credentialName = getCertificateTitle( + programCert.certificate_page?.product_name, + programCert.program.title, + ) + } const certId = certificateData.uuid const linkedinUrl = new URL(LINKEDIN_ADD_TO_PROFILE_BASE_URL) linkedinUrl.searchParams.set("startTask", "CERTIFICATION_NAME") From ab8bb2b6e879dd66826bb43cd04a996e1db29f83 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Thu, 6 Aug 2026 09:11:39 -0400 Subject: [PATCH 03/12] Fix PostHog view-event ETL crash from duplicate view events (#3714) --- learning_resources/etl/posthog.py | 64 +++++++++++++-- learning_resources/etl/posthog_test.py | 79 +++++++++++++++++++ .../0119_unique_resource_view_event.py | 28 +++++++ .../0120_view_event_uuid_unique_index.py | 46 +++++++++++ learning_resources/models.py | 19 +++++ 5 files changed, 229 insertions(+), 7 deletions(-) create mode 100644 learning_resources/migrations/0119_unique_resource_view_event.py create mode 100644 learning_resources/migrations/0120_view_event_uuid_unique_index.py diff --git a/learning_resources/etl/posthog.py b/learning_resources/etl/posthog.py index b5b9ea4dd0..2d0f63184f 100644 --- a/learning_resources/etl/posthog.py +++ b/learning_resources/etl/posthog.py @@ -10,6 +10,7 @@ import boto3 import pandas as pd from django.conf import settings +from django.db import IntegrityError, transaction from learning_resources.models import LearningResource, LearningResourceViewEvent from learning_resources.utils import resource_upserted_actions @@ -28,6 +29,7 @@ class PostHogLearningResourceViewEvent: resource_id: int event_date: datetime + event_uuid: str def posthog_extract_lrd_view_events() -> Generator[dict, None, None]: @@ -94,11 +96,23 @@ def posthog_transform_lrd_view_events( # The PostHog data files contain other kinds of events, for example llm calls. # We only want to the resource views - if resource_id: - yield PostHogLearningResourceViewEvent( - resource_id=resource_id, - event_date=event.get("timestamp"), + if not resource_id: + continue + + event_uuid = event.get("uuid") + if not event_uuid: + # PostHog assigns every event a uuid, so this indicates + # malformed source data + log.warning( + "Skipping lrd_view event without a uuid for resource %s", resource_id ) + continue + + yield PostHogLearningResourceViewEvent( + resource_id=resource_id, + event_date=event.get("timestamp"), + event_uuid=event_uuid, + ) def load_posthog_lrd_view_event( @@ -136,9 +150,45 @@ def load_posthog_lrd_view_event( log.warning(skip_warning) return None - lr_event, _ = LearningResourceViewEvent.objects.update_or_create( - learning_resource=learning_resource, - event_date=event.event_date, + # The newest S3 file is re-read on every run (its last_modified is always + # later than the events it holds), so most events arrive already stored. + # Return early: stamping this uuid onto another legacy duplicate below + # would violate the unique index. + existing = LearningResourceViewEvent.objects.filter( + event_uuid=event.event_uuid + ).first() + if existing: + return existing + + # Adopt a matching legacy row (loaded before event_uuid existed) so re-read + # S3 files don't duplicate it. Stamp only the oldest one; the legacy tail + # can hold duplicate (resource, event_date) rows. + legacy_row = ( + LearningResourceViewEvent.objects.filter( + learning_resource=learning_resource, + event_date=event.event_date, + event_uuid__isnull=True, + ) + .order_by("id") + .first() + ) + if legacy_row: + try: + # Savepoint: the check above is not a lock, so a concurrent run can + # adopt a different duplicate for this uuid first. get_or_create + # below then finds that row. + with transaction.atomic(): + LearningResourceViewEvent.objects.filter(pk=legacy_row.pk).update( + event_uuid=event.event_uuid + ) + except IntegrityError: + log.info( + "Legacy view event row for resource %s was adopted concurrently", + event.resource_id, + ) + + lr_event, _ = LearningResourceViewEvent.objects.get_or_create( + event_uuid=event.event_uuid, defaults={ "learning_resource": learning_resource, "event_date": event.event_date, diff --git a/learning_resources/etl/posthog_test.py b/learning_resources/etl/posthog_test.py index 3fe0cc2bd9..75ece8f240 100644 --- a/learning_resources/etl/posthog_test.py +++ b/learning_resources/etl/posthog_test.py @@ -4,6 +4,7 @@ from pathlib import Path import pytest +from django.db import IntegrityError from freezegun import freeze_time from learning_resources.etl import posthog @@ -137,3 +138,81 @@ def test_load_posthog_lrd_view_events( else: assert LearningResourceViewEvent.objects.count() == 0 assert len([event for event in loaded_events if event is not None]) == 0 + + +@pytest.mark.django_db +@pytest.mark.parametrize("runs", [1, 2]) +def test_load_posthog_lrd_view_events_duplicate_legacy_rows( + mocker, mock_posthog_event_bucket, settings, runs +): + """Duplicate legacy rows are tolerated, and re-running the ETL stays idempotent""" + LearningResourceViewEvent.objects.all().delete() + bucket = mock_posthog_event_bucket.bucket + settings.POSTHOG_EVENT_S3_BUCKET = bucket.name + settings.POSTHOG_EVENT_S3_PREFIX = "events/" + with Path.open(Path("test_json/posthog/test_data.parquet.zst"), "rb") as infile: + bucket.put_object( + Key="events/file1.parquet.zst", + Body=infile.read(), + ACL="public-read", + ) + + resource = LearningResourceFactory.create(id=3235) + mocker.patch( + "learning_resources.etl.posthog.resource_upserted_actions", + autospec=True, + ) + + event_date = datetime(2025, 8, 28, 15, 20, 13, 620000, tzinfo=UTC) + legacy_rows = LearningResourceViewEventFactory.create_batch( + 2, learning_resource=resource, event_date=event_date + ) + + for _ in range(runs): + posthog.load_posthog_lrd_view_events( + posthog.posthog_transform_lrd_view_events( + posthog.posthog_extract_lrd_view_events() + ) + ) + + stamped = [ + row + for row in LearningResourceViewEvent.objects.filter( + pk__in=[r.pk for r in legacy_rows] + ) + if row.event_uuid is not None + ] + assert len(stamped) == 1 + # The unstamped duplicate survives rather than being deleted + assert ( + LearningResourceViewEvent.objects.filter( + learning_resource=resource, event_date=event_date, event_uuid__isnull=True + ).count() + == 1 + ) + + +@pytest.mark.django_db +def test_load_posthog_lrd_view_event_adoption_lost_to_concurrent_run(mocker): + """Losing the adoption race to a concurrent run doesn't crash the loader""" + resource = LearningResourceFactory.create() + event_date = now_in_utc() + LearningResourceViewEventFactory.create( + learning_resource=resource, event_date=event_date + ) + event_uuid = "0198f143-cf8c-79b6-bab8-9c9063659a54" + # Simulates another worker stamping a different duplicate with this uuid + # between our existence check and the adoption update + mocker.patch( + "django.db.models.QuerySet.update", + side_effect=IntegrityError("duplicate key value violates unique constraint"), + ) + + lr_event = posthog.load_posthog_lrd_view_event( + posthog.PostHogLearningResourceViewEvent( + resource_id=resource.id, event_date=event_date, event_uuid=event_uuid + ) + ) + + assert lr_event is not None + assert LearningResourceViewEvent.objects.filter(event_uuid=event_uuid).count() == 1 diff --git a/learning_resources/migrations/0119_unique_resource_view_event.py b/learning_resources/migrations/0119_unique_resource_view_event.py new file mode 100644 index 0000000000..58c5fe3b96 --- /dev/null +++ b/learning_resources/migrations/0119_unique_resource_view_event.py @@ -0,0 +1,28 @@ +"""Add the PostHog event UUID column to LearningResourceViewEvent""" + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("learning_resources", "0118_delete_micromasters_resources"), + ] + + operations = [ + # Nullable column with no default: metadata-only, no table rewrite. + # The unique index is built CONCURRENTLY in the next migration. + # Pre-existing duplicate rows are left alone - they all have a NULL + # event_uuid, which never conflicts in a unique index. + migrations.AddField( + model_name="learningresourceviewevent", + name="event_uuid", + field=models.UUIDField( + editable=False, + help_text=( + "The PostHog event UUID. Null only for rows loaded" + " before this field existed." + ), + null=True, + ), + ), + ] diff --git a/learning_resources/migrations/0120_view_event_uuid_unique_index.py b/learning_resources/migrations/0120_view_event_uuid_unique_index.py new file mode 100644 index 0000000000..e555527d01 --- /dev/null +++ b/learning_resources/migrations/0120_view_event_uuid_unique_index.py @@ -0,0 +1,46 @@ +"""Build the unique index on event_uuid concurrently to avoid blocking reads""" + +from django.db import migrations, models +from django.db.models import Q + +INDEX_NAME = "learning_resources_lrviewevent_event_uuid_uniq" +TABLE_NAME = "learning_resources_learningresourceviewevent" + + +class Migration(migrations.Migration): + # CREATE INDEX CONCURRENTLY cannot run inside a transaction + atomic = False + + dependencies = [ + ("learning_resources", "0119_unique_resource_view_event"), + ] + + operations = [ + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.AddConstraint( + model_name="learningresourceviewevent", + constraint=models.UniqueConstraint( + condition=Q(event_uuid__isnull=False), + fields=("event_uuid",), + name=INDEX_NAME, + ), + ), + ], + database_operations=[ + migrations.RunSQL( + [ + # A failed CONCURRENTLY build leaves an INVALID index + # that IF NOT EXISTS would silently accept; drop any + # leftover so a rerun rebuilds and enforces uniqueness + f"DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME}", + # Partial: the legacy NULL rows need no index entries + f"CREATE UNIQUE INDEX CONCURRENTLY {INDEX_NAME}" + f" ON {TABLE_NAME} (event_uuid)" + f" WHERE event_uuid IS NOT NULL", + ], + reverse_sql=f"DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME}", + ), + ], + ), + ] diff --git a/learning_resources/models.py b/learning_resources/models.py index 488fbf812e..9b0f2ac71a 100644 --- a/learning_resources/models.py +++ b/learning_resources/models.py @@ -1566,6 +1566,25 @@ class LearningResourceViewEvent(TimestampedModel): editable=False, help_text="The date of the lrd_view event, as collected by PostHog.", ) + event_uuid = models.UUIDField( + null=True, + editable=False, + help_text=( + "The PostHog event UUID. Null only for rows loaded" + " before this field existed." + ), + ) + + class Meta: + constraints = [ + # Conditional so the index skips the legacy NULL rows, which would + # otherwise cost ~105MB of index for entries nothing ever probes. + models.UniqueConstraint( + fields=["event_uuid"], + condition=Q(event_uuid__isnull=False), + name="learning_resources_lrviewevent_event_uuid_uniq", + ) + ] def __str__(self): """Return a string representation of the event.""" From d4e2760ef1122b67e6d36faf28f7c4a3a1a9edbc Mon Sep 17 00:00:00 2001 From: Anastasia Beglova Date: Thu, 6 Aug 2026 09:50:34 -0400 Subject: [PATCH 04/12] Make recreate index resilient to pod culling (#3716) --- learning_resources_search/constants.py | 15 + learning_resources_search/indexing_api.py | 17 + .../management/commands/recreate_index.py | 121 ++- .../commands/recreate_index_test.py | 195 ++++ learning_resources_search/tasks.py | 609 +++++++++---- learning_resources_search/tasks_test.py | 854 ++++++++++++++---- main/admin.py | 37 + main/factories.py | 25 + main/migrations/0001_taskjob_taskbatch.py | 102 +++ main/migrations/__init__.py | 0 main/models.py | 68 ++ main/settings.py | 6 + main/settings_celery.py | 4 + main/tasks.py | 45 + main/tasks_test.py | 55 ++ 15 files changed, 1774 insertions(+), 379 deletions(-) create mode 100644 learning_resources_search/management/commands/recreate_index_test.py create mode 100644 main/admin.py create mode 100644 main/migrations/0001_taskjob_taskbatch.py create mode 100644 main/migrations/__init__.py create mode 100644 main/tasks.py create mode 100644 main/tasks_test.py diff --git a/learning_resources_search/constants.py b/learning_resources_search/constants.py index dd6a1a535e..9603d0aa0e 100644 --- a/learning_resources_search/constants.py +++ b/learning_resources_search/constants.py @@ -56,6 +56,21 @@ class IndexestoUpdate(Enum): all_indexes = "all_indexes" +# TaskJob.task_name for recreate_index jobs +REINDEX_TASK_NAME = "recreate_index" + + +class ReindexBatchKind(Enum): + """ + Enum for the kinds of TaskBatch used by recreate_index jobs + """ + + learning_resources = "learning_resources" + content_files = "content_files" + percolate = "percolate" + dispatch_content_files = "dispatch_content_files" + + LEARNING_RESOURCE_TYPES = ( COURSE_TYPE, PROGRAM_TYPE, diff --git a/learning_resources_search/indexing_api.py b/learning_resources_search/indexing_api.py index 7e23248e5e..042b3ee982 100644 --- a/learning_resources_search/indexing_api.py +++ b/learning_resources_search/indexing_api.py @@ -640,6 +640,23 @@ def switch_indices(backing_index, object_type): log.warning("Reindex alias not found for %s", object_type) +def is_default_backing_index(backing_index, object_type): + """ + Check whether the default alias already points at the given backing index + + Args: + backing_index (str): The backing index to check + object_type (str): The object type for the index + + Returns: + bool: True if the default alias already points at backing_index + """ + conn = get_conn() + return conn.indices.exists_alias( + index=backing_index, name=get_default_alias_name(object_type) + ) + + def delete_orphaned_indexes(obj_types, delete_reindexing_tags): """ Delete any indices without aliases diff --git a/learning_resources_search/management/commands/recreate_index.py b/learning_resources_search/management/commands/recreate_index.py index 420ad3127e..9e22d47c60 100644 --- a/learning_resources_search/management/commands/recreate_index.py +++ b/learning_resources_search/management/commands/recreate_index.py @@ -1,11 +1,16 @@ """Management command to index content""" -from django.core.management.base import BaseCommand, CommandError +from django.core.management.base import BaseCommand +from django.db.models import Count -from learning_resources_search.constants import ALL_INDEX_TYPES, HYBRID_COMBINED_INDEX +from learning_resources_search.constants import ( + ALL_INDEX_TYPES, + HYBRID_COMBINED_INDEX, + REINDEX_TASK_NAME, +) from learning_resources_search.indexing_api import get_existing_reindexing_indexes from learning_resources_search.tasks import start_recreate_index -from main.utils import now_in_utc +from main.models import TaskJob class Command(BaseCommand): @@ -15,10 +20,13 @@ class Command(BaseCommand): def add_arguments(self, parser): parser.add_argument( - "--remove_existing_reindexing_tags", - dest="remove_existing_reindexing_tags", + "--restart", + dest="restart", action="store_true", - help="Overwrite any existing reindexing tags and remove those indexes", + help=( + "Discard any in-progress reindex (existing reindexing indexes and" + " active jobs) and start over" + ), ) parser.add_argument( @@ -32,6 +40,19 @@ def add_arguments(self, parser): help="Recreate combined index for hybrid search", ) + parser.add_argument( + "--status", + dest="status", + nargs="?", + type=int, + const=0, + default=None, + help=( + "Print the status of a reindex job instead of starting one " + "(defaults to the most recent job)" + ), + ) + for object_type in sorted(ALL_INDEX_TYPES): if object_type != HYBRID_COMBINED_INDEX: parser.add_argument( @@ -42,11 +63,40 @@ def add_arguments(self, parser): ) super().add_arguments(parser) + def print_job_status(self, job_id): + """Print the status of a reindex job""" + jobs = TaskJob.objects.filter(task_name=REINDEX_TASK_NAME) + job = jobs.filter(id=job_id).first() if job_id else jobs.order_by("-id").first() + if not job: + self.stdout.write("No reindex job found") + return + self.stdout.write(f"Reindex job {job.id}: {job.status}") + self.stdout.write(f" indexes: {job.params.get('indexes')}") + self.stdout.write(f" backing indexes: {job.params.get('backing_indexes')}") + self.stdout.write(" batches:") + batch_counts = job.batches.values("kind", "status").annotate(count=Count("id")) + kind_counts = {} + for row in batch_counts: + kind_counts.setdefault(row["kind"], {})[row["status"]] = row["count"] + for kind, counts in sorted(kind_counts.items()): + self.stdout.write(f" {kind}: {counts}") + if job.error: + self.stdout.write(f" error: {job.error}") + def handle(self, *args, **options): # noqa: ARG002 """Index all LEARNING_RESOURCE_TYPES""" - remove_existing_reindexing_tags = options["remove_existing_reindexing_tags"] + if options["status"] is not None: + self.print_job_status(options["status"]) + return + + restart = options["restart"] if options["all"]: - indexes_to_update = list(ALL_INDEX_TYPES) + # the combined hybrid index is still experimental and expensive to + # build; keep it out of --all and require the explicit + # --combined_hybrid flag to reindex it + indexes_to_update = [ + index for index in ALL_INDEX_TYPES if index != HYBRID_COMBINED_INDEX + ] else: indexes_to_update = list( filter( @@ -61,7 +111,15 @@ def handle(self, *args, **options): # noqa: ARG002 for object_type in sorted(ALL_INDEX_TYPES): self.stdout.write(f" --{object_type}s") return - if not remove_existing_reindexing_tags: + + active_jobs = [ + job + for job in TaskJob.objects.filter( + task_name=REINDEX_TASK_NAME, status__in=TaskJob.ACTIVE_STATUSES + ) + if set(job.params.get("indexes", [])) & set(indexes_to_update) + ] + if not restart: existing_reindexing_indexes = get_existing_reindexing_indexes( indexes_to_update ) @@ -69,24 +127,39 @@ def handle(self, *args, **options): # noqa: ARG002 self.stdout.write( f"Reindexing in progress. Reindexing indexes already exist:" f" {', '.join(existing_reindexing_indexes)}" - f"\nUse --remove_existing_reindexing_tags if you want to continue" + f"\nUse --restart if you want to continue" + ) + return + if active_jobs: + self.stdout.write( + f"Reindexing in progress. Active reindex jobs already exist:" + f" {', '.join(str(job.id) for job in active_jobs)}" + f"\nUse --restart if you want to continue" ) return - task = start_recreate_index.delay( - indexes_to_update, remove_existing_reindexing_tags - ) - self.stdout.write( - f"Started celery task {task} to index content for the following" - f" indexes: {indexes_to_update}" + job = TaskJob.objects.create( + task_name=REINDEX_TASK_NAME, + params={ + "indexes": indexes_to_update, + "restart": restart, + }, ) - self.stdout.write("Waiting on task...") - start = now_in_utc() - error = task.get() - if error: - msg = f"Recreate index errored: {error}" - raise CommandError(msg) + if restart: + for active_job in active_jobs: + TaskJob.objects.filter( + id=active_job.id, status__in=TaskJob.ACTIVE_STATUSES + ).update( + status=TaskJob.Status.FAILED, + error=f"superseded by reindex job {job.id}", + ) - total_seconds = (now_in_utc() - start).total_seconds() - self.stdout.write(f"Recreate index finished, took {total_seconds} seconds") + start_recreate_index.delay(job.id) + self.stdout.write( + f"Started reindex job {job.id} for the following indexes:" + f" {indexes_to_update}" + ) + self.stdout.write( + f"Check progress with: ./manage.py recreate_index --status {job.id}" + ) diff --git a/learning_resources_search/management/commands/recreate_index_test.py b/learning_resources_search/management/commands/recreate_index_test.py new file mode 100644 index 0000000000..58b3d4bd6b --- /dev/null +++ b/learning_resources_search/management/commands/recreate_index_test.py @@ -0,0 +1,195 @@ +"""Tests for the recreate_index management command""" + +from io import StringIO + +import pytest +from django.core.management import call_command + +from learning_resources_search.constants import ( + ALL_INDEX_TYPES, + HYBRID_COMBINED_INDEX, + REINDEX_TASK_NAME, +) +from main.factories import TaskBatchFactory, TaskJobFactory +from main.models import TaskBatch, TaskJob + +pytestmark = pytest.mark.django_db + + +@pytest.fixture +def start_recreate_index_mock(mocker): + """Mock the start_recreate_index task""" + return mocker.patch( + "learning_resources_search.management.commands.recreate_index.start_recreate_index", + autospec=True, + ) + + +@pytest.fixture +def _no_existing_reindexing_indexes(mocker): + """Mock get_existing_reindexing_indexes to return nothing""" + mocker.patch( + "learning_resources_search.management.commands.recreate_index.get_existing_reindexing_indexes", + autospec=True, + return_value=[], + ) + + +@pytest.mark.usefixtures("_no_existing_reindexing_indexes") +def test_recreate_index_starts_job(start_recreate_index_mock): + """The command should create a job, enqueue the start task and exit""" + stdout = StringIO() + call_command("recreate_index", "--programs", stdout=stdout) + + job = TaskJob.objects.get() + assert job.task_name == REINDEX_TASK_NAME + assert job.params == { + "indexes": ["program"], + "restart": False, + } + assert job.status == TaskJob.Status.QUEUED + start_recreate_index_mock.delay.assert_called_once_with(job.id) + output = stdout.getvalue() + assert f"Started reindex job {job.id}" in output + assert f"--status {job.id}" in output + + +@pytest.mark.usefixtures("_no_existing_reindexing_indexes") +def test_recreate_index_all_excludes_combined_hybrid(start_recreate_index_mock): + """--all should reindex every type except the experimental hybrid index""" + call_command("recreate_index", "--all", stdout=StringIO()) + + job = TaskJob.objects.get() + assert HYBRID_COMBINED_INDEX not in job.params["indexes"] + assert set(job.params["indexes"]) == { + index for index in ALL_INDEX_TYPES if index != HYBRID_COMBINED_INDEX + } + + +@pytest.mark.usefixtures("_no_existing_reindexing_indexes") +def test_recreate_index_combined_hybrid_flag(start_recreate_index_mock): + """--combined_hybrid should still reindex the hybrid index explicitly""" + call_command("recreate_index", "--combined_hybrid", stdout=StringIO()) + + job = TaskJob.objects.get() + assert job.params["indexes"] == [HYBRID_COMBINED_INDEX] + + +@pytest.mark.usefixtures("_no_existing_reindexing_indexes") +def test_recreate_index_blocks_on_active_job(start_recreate_index_mock): + """The command should not start a job when an overlapping job is active""" + active_job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["program"]}, + status=TaskJob.Status.RUNNING, + ) + + stdout = StringIO() + call_command("recreate_index", "--programs", stdout=stdout) + + assert TaskJob.objects.count() == 1 + start_recreate_index_mock.delay.assert_not_called() + assert str(active_job.id) in stdout.getvalue() + + +@pytest.mark.usefixtures("_no_existing_reindexing_indexes") +def test_recreate_index_supersedes_active_job(start_recreate_index_mock): + """--restart should fail overlapping active jobs""" + active_job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["program"]}, + status=TaskJob.Status.RUNNING, + ) + + stdout = StringIO() + call_command( + "recreate_index", + "--programs", + "--restart", + stdout=stdout, + ) + + active_job.refresh_from_db() + new_job = TaskJob.objects.exclude(id=active_job.id).get() + assert active_job.status == TaskJob.Status.FAILED + assert f"superseded by reindex job {new_job.id}" == active_job.error + assert new_job.params["restart"] is True + start_recreate_index_mock.delay.assert_called_once_with(new_job.id) + + +def test_recreate_index_blocks_on_existing_reindexing_indexes( + mocker, start_recreate_index_mock +): + """The command should not start a job when reindexing indexes exist""" + mocker.patch( + "learning_resources_search.management.commands.recreate_index.get_existing_reindexing_indexes", + autospec=True, + return_value=["some_reindexing_index"], + ) + + stdout = StringIO() + call_command("recreate_index", "--programs", stdout=stdout) + + assert TaskJob.objects.count() == 0 + start_recreate_index_mock.delay.assert_not_called() + assert "some_reindexing_index" in stdout.getvalue() + + +def test_recreate_index_requires_index_selection(start_recreate_index_mock): + """The command should print valid options when no index is selected""" + stdout = StringIO() + call_command("recreate_index", stdout=stdout) + + assert TaskJob.objects.count() == 0 + start_recreate_index_mock.delay.assert_not_called() + assert "Must select at least one index to update" in stdout.getvalue() + + +def test_recreate_index_status(start_recreate_index_mock): + """--status should print the job status without starting anything""" + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["course"], "backing_indexes": {"course": "backing"}}, + status=TaskJob.Status.RUNNING, + ) + TaskBatchFactory.create( + job=job, kind="learning_resources", status=TaskBatch.Status.SUCCEEDED + ) + TaskBatchFactory.create( + job=job, kind="learning_resources", status=TaskBatch.Status.QUEUED + ) + + stdout = StringIO() + call_command("recreate_index", "--status", str(job.id), stdout=stdout) + + output = stdout.getvalue() + assert f"Reindex job {job.id}: running" in output + assert "learning_resources" in output + assert "'succeeded': 1" in output + assert "'queued': 1" in output + start_recreate_index_mock.delay.assert_not_called() + + +def test_recreate_index_status_latest(start_recreate_index_mock): + """--status with no id should print the most recent job""" + TaskJobFactory.create(task_name=REINDEX_TASK_NAME, params={"indexes": ["course"]}) + latest_job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["program"]}, + status=TaskJob.Status.SUCCEEDED, + ) + + stdout = StringIO() + call_command("recreate_index", "--status", stdout=stdout) + + assert f"Reindex job {latest_job.id}: succeeded" in stdout.getvalue() + start_recreate_index_mock.delay.assert_not_called() + + +def test_recreate_index_status_no_jobs(start_recreate_index_mock): + """--status should handle there being no jobs at all""" + stdout = StringIO() + call_command("recreate_index", "--status", stdout=stdout) + + assert "No reindex job found" in stdout.getvalue() + start_recreate_index_mock.delay.assert_not_called() diff --git a/learning_resources_search/tasks.py b/learning_resources_search/tasks.py index eb817d2bdd..21086c922a 100644 --- a/learning_resources_search/tasks.py +++ b/learning_resources_search/tasks.py @@ -41,8 +41,10 @@ LEARNING_RESOURCE_TYPES, PERCOLATE_INDEX_TYPE, PROGRAM_TYPE, + REINDEX_TASK_NAME, SEARCH_CONN_EXCEPTIONS, IndexestoUpdate, + ReindexBatchKind, ) from learning_resources_search.exceptions import ReindexError, RetryError from learning_resources_search.models import PercolateQuery @@ -53,11 +55,12 @@ serialize_percolate_query_for_update, ) from main.celery import app +from main.models import TaskBatch, TaskJob +from main.tasks import maybe_finish_task_job from main.utils import ( chunks, clear_views_cache, frontend_absolute_url, - merge_strings, now_in_utc, ) from profiles.utils import send_template_email @@ -568,200 +571,419 @@ def wrap_retry_exception(*exception_classes): raise -@app.task(bind=True) -def start_recreate_index(self, indexes, remove_existing_reindexing_tags): # noqa: C901 - """ - Wipe and recreate index and mapping, and index all items. +def _build_reindex_batches(job): # noqa: C901, PLR0912 """ - try: - if not remove_existing_reindexing_tags: - existing_reindexing_indexes = api.get_existing_reindexing_indexes(indexes) + Build the TaskBatch rows for a reindex job using fast id-only queries. - if existing_reindexing_indexes: - error = ( - f"Reindexing in progress. Reindexing indexes already exist: " - f"{', '.join(existing_reindexing_indexes)}" - ) - log.exception(error) - return error + Content files are not enumerated here; dispatch batches defer the slow + per-resource ContentFile queries to run_reindex_batch workers. - api.delete_orphaned_indexes( - indexes, delete_reindexing_tags=remove_existing_reindexing_tags - ) + Args: + job (TaskJob): the reindex job - new_backing_indices = { - obj_type: api.create_backing_index(obj_type) for obj_type in indexes - } + Returns: + list of TaskBatch: unsaved batch rows + """ + indexes = job.params["indexes"] + batches = [] - # Do the indexing on the temp index - log.info("starting to index %s objects...", ", ".join(indexes)) + def add_batch(kind, batch_key, params): + batches.append( + TaskBatch(job=job, kind=kind.value, batch_key=batch_key, params=params) + ) - index_tasks = [] + if PERCOLATE_INDEX_TYPE in indexes: + for chunk, ids in enumerate( + chunks( + PercolateQuery.objects.order_by("id").values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, + ) + ): + add_batch(ReindexBatchKind.percolate, f"percolate:{chunk}", {"ids": ids}) - if PERCOLATE_INDEX_TYPE in indexes: - index_tasks = index_tasks + [ - bulk_index_percolate_queries.si( - percolate_ids, IndexestoUpdate.reindexing_index.value - ) - for percolate_ids in chunks( - PercolateQuery.objects.order_by("id").values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, - ) - ] + if COURSE_TYPE in indexes or HYBRID_COMBINED_INDEX in indexes: + blocklisted_ids = load_course_blocklist() - if COURSE_TYPE in indexes: - blocklisted_ids = load_course_blocklist() - index_tasks = index_tasks + [ - index_learning_resources.si( - ids, - COURSE_TYPE, - index_types=IndexestoUpdate.reindexing_index.value, - ) - for ids in chunks( - Course.objects.filter(learning_resource__published=True) - .exclude(learning_resource__readable_id__in=blocklisted_ids) - .order_by("learning_resource_id") - .values_list("learning_resource_id", flat=True), - chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, - ) - ] + if COURSE_TYPE in indexes: + for chunk, ids in enumerate( + chunks( + Course.objects.filter(learning_resource__published=True) + .exclude(learning_resource__readable_id__in=blocklisted_ids) + .order_by("learning_resource_id") + .values_list("learning_resource_id", flat=True), + chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, + ) + ): + add_batch( + ReindexBatchKind.learning_resources, + f"{COURSE_TYPE}:resources:{chunk}", + {"ids": ids, "index_name": COURSE_TYPE}, + ) - for course in ( + for chunk, resource_ids in enumerate( + chunks( Course.objects.filter(learning_resource__published=True) .filter(learning_resource__etl_source__in=RESOURCE_FILE_ETL_SOURCES) .exclude(learning_resource__readable_id__in=blocklisted_ids) .order_by("learning_resource_id") - ): - index_tasks = ( - index_tasks - + [ - index_content_files.si( - ids, - course.learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - ) - for ids in chunks( - ContentFile.objects.filter( - run__learning_resource_id=course.learning_resource_id, - published=True, - run__published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ] - + [ - index_content_files.si( - ids, - course.learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - ) - for ids in chunks( - ContentFile.objects.filter( - learning_resource_id=course.learning_resource_id, - published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ] - ) + .values_list("learning_resource_id", flat=True), + chunk_size=settings.OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE, + ) + ): + add_batch( + ReindexBatchKind.dispatch_content_files, + f"dispatch:{COURSE_TYPE}:{chunk}", + { + "learning_resource_ids": resource_ids, + "resource_type": COURSE_TYPE, + }, + ) - if HYBRID_COMBINED_INDEX in indexes: - blocklisted_ids = load_course_blocklist() + if HYBRID_COMBINED_INDEX in indexes: + for chunk, ids in enumerate( + chunks( + LearningResource.objects.filter(published=True) + .exclude(readable_id__in=blocklisted_ids) + .order_by("id") + .values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, + ) + ): + add_batch( + ReindexBatchKind.learning_resources, + f"{HYBRID_COMBINED_INDEX}:resources:{chunk}", + {"ids": ids, "index_name": HYBRID_COMBINED_INDEX}, + ) - index_tasks = index_tasks + [ - index_learning_resources.si( - ids, - HYBRID_COMBINED_INDEX, - index_types=IndexestoUpdate.reindexing_index.value, - ) - for ids in chunks( + for resource_type in set(LEARNING_RESOURCE_TYPES) - {COURSE_TYPE}: + if resource_type in indexes: + for chunk, ids in enumerate( + chunks( LearningResource.objects.filter( published=True, + resource_type=resource_type, ) - .exclude(readable_id__in=blocklisted_ids) .order_by("id") .values_list("id", flat=True), chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, ) - ] + ): + add_batch( + ReindexBatchKind.learning_resources, + f"{resource_type}:resources:{chunk}", + {"ids": ids, "index_name": resource_type}, + ) - for resource_type in set(LEARNING_RESOURCE_TYPES) - {COURSE_TYPE}: - if resource_type in indexes: - index_tasks = index_tasks + [ - index_learning_resources.si( - ids, - resource_type, - index_types=IndexestoUpdate.reindexing_index.value, - ) - for ids in chunks( - LearningResource.objects.filter( - published=True, - resource_type=resource_type, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_INDEXING_CHUNK_SIZE, - ) - ] + if PROGRAM_TYPE in indexes: + for chunk, resource_ids in enumerate( + chunks( + LearningResource.objects.filter( + published=True, resource_type=PROGRAM_TYPE + ) + .order_by("id") + .values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE, + ) + ): + add_batch( + ReindexBatchKind.dispatch_content_files, + f"dispatch:{PROGRAM_TYPE}:{chunk}", + { + "learning_resource_ids": resource_ids, + "resource_type": PROGRAM_TYPE, + }, + ) - if PROGRAM_TYPE in indexes: - for program_resource in LearningResource.objects.filter( - published=True, resource_type=PROGRAM_TYPE - ).order_by("id"): - index_tasks = ( - index_tasks - + [ - index_content_files.si( - ids, - program_resource.id, - index_types=IndexestoUpdate.reindexing_index.value, - resource_type=PROGRAM_TYPE, - ) - for ids in chunks( - ContentFile.objects.filter( - run__learning_resource_id=program_resource.id, - published=True, - run__published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) + return batches + + +def _dispatch_content_file_batches(batch): + """ + Create and enqueue the content file batches for a dispatch batch. + + Child rows are created (idempotently, via the unique batch_key) before the + dispatch batch itself is marked complete, so the job can never appear + finished while content file fan-out is still pending. + + Args: + batch (TaskBatch): a dispatch_content_files batch + """ + resource_type = batch.params["resource_type"] + children = [] + for resource_id in batch.params["learning_resource_ids"]: + for chunk, ids in enumerate( + chunks( + ContentFile.objects.filter( + run__learning_resource_id=resource_id, + published=True, + run__published=True, + ) + .order_by("id") + .values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, + ) + ): + children.append( + TaskBatch( + job=batch.job, + kind=ReindexBatchKind.content_files.value, + batch_key=f"content_files:{resource_id}:run:{chunk}", + params={ + "ids": ids, + "learning_resource_id": resource_id, + "resource_type": resource_type, + }, + ) + ) + for chunk, ids in enumerate( + chunks( + ContentFile.objects.filter( + learning_resource_id=resource_id, + published=True, + ) + .order_by("id") + .values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, + ) + ): + children.append( + TaskBatch( + job=batch.job, + kind=ReindexBatchKind.content_files.value, + batch_key=f"content_files:{resource_id}:direct:{chunk}", + params={ + "ids": ids, + "learning_resource_id": resource_id, + "resource_type": resource_type, + }, + ) + ) + TaskBatch.objects.bulk_create(children, ignore_conflicts=True) + child_ids = batch.job.batches.filter( + batch_key__in=[child.batch_key for child in children], + status=TaskBatch.Status.QUEUED, + ).values_list("id", flat=True) + for child_id in child_ids: + run_reindex_batch.delay(child_id) + + +def _execute_reindex_batch(batch): + """ + Run the indexing work for a single reindex batch + + Args: + batch (TaskBatch): the batch to execute + """ + params = batch.params + if batch.kind == ReindexBatchKind.learning_resources.value: + api.index_learning_resources( + params["ids"], + params["index_name"], + IndexestoUpdate.reindexing_index.value, + ) + elif batch.kind == ReindexBatchKind.content_files.value: + api.index_content_files( + params["ids"], + params["learning_resource_id"], + index_types=IndexestoUpdate.reindexing_index.value, + resource_type=params["resource_type"], + ) + elif batch.kind == ReindexBatchKind.percolate.value: + api.index_items( + serialize_bulk_percolators(params["ids"]), + PERCOLATE_INDEX_TYPE, + IndexestoUpdate.reindexing_index.value, + ) + elif batch.kind == ReindexBatchKind.dispatch_content_files.value: + _dispatch_content_file_batches(batch) + + +def _maybe_finish_reindex_job(job_id): + """ + Claim and enqueue finish_reindex_job if every batch of the job is done + + Args: + job_id (int): TaskJob id + """ + maybe_finish_task_job(job_id, finish_reindex_job) + + +class _RunReindexBatchTask(app.Task): + """ + Base task that fails the batch if run_reindex_batch gives up. + + When autoretries are exhausted the task raises without having marked the + batch terminal, which would leave it RUNNING and hang the job. on_failure + fires once, on the final give-up, so we mark the batch FAILED and nudge + completion. (A worker killed mid-run does not trigger this — that message + is redelivered by acks_late instead.) + """ + + def on_failure(self, exc, task_id, args, kwargs, einfo): # noqa: ARG002 + batch_id = args[0] if args else kwargs.get("batch_id") + batch = TaskBatch.objects.filter(id=batch_id).first() + if batch is None: + return + TaskBatch.objects.filter( + id=batch_id, status__in=TaskBatch.NON_TERMINAL_STATUSES + ).update( + status=TaskBatch.Status.FAILED, + error=f"run_reindex_batch gave up: {exc}", + ) + _maybe_finish_reindex_job(batch.job_id) + + +@app.task( + base=_RunReindexBatchTask, + acks_late=True, + reject_on_worker_lost=True, + autoretry_for=(RetryError,), + retry_backoff=True, + rate_limit=settings.CELERY_SEARCH_RATE_LIMIT, +) +def run_reindex_batch(batch_id): + """ + Execute one reindex batch and record its completion in the database + + Args: + batch_id (int): TaskBatch id + """ + batch = TaskBatch.objects.select_related("job").get(id=batch_id) + if ( + batch.status not in TaskBatch.NON_TERMINAL_STATUSES + or batch.job.status not in TaskJob.ACTIVE_STATUSES + ): + log.info( + "Skipping reindex batch %s (batch status=%s, job status=%s)", + batch.batch_key, + batch.status, + batch.job.status, + ) + # a redelivery of an already-finished batch still nudges completion, so + # the job can't hang if the last batch's worker was culled right after + # committing its status but before the finish step was enqueued + _maybe_finish_reindex_job(batch.job_id) + return + # mark the batch running; a redelivered message may find the batch already + # running, which is fine — execution is idempotent + TaskBatch.objects.filter( + id=batch_id, status__in=TaskBatch.NON_TERMINAL_STATUSES + ).update(status=TaskBatch.Status.RUNNING) + try: + with wrap_retry_exception(*SEARCH_CONN_EXCEPTIONS): + _execute_reindex_batch(batch) + except (RetryError, Ignore): + raise + except SystemExit as err: + raise RetryError(SystemExit.__name__) from err + except Exception as ex: + error = f"run_reindex_batch threw an error: {type(ex).__name__}: {ex}" + log.exception("Reindex batch %s failed", batch.batch_key) + TaskBatch.objects.filter( + id=batch_id, status__in=TaskBatch.NON_TERMINAL_STATUSES + ).update(status=TaskBatch.Status.FAILED, error=error) + else: + TaskBatch.objects.filter( + id=batch_id, status__in=TaskBatch.NON_TERMINAL_STATUSES + ).update(status=TaskBatch.Status.SUCCEEDED) + _maybe_finish_reindex_job(batch.job_id) + + +@app.task(acks_late=True, reject_on_worker_lost=True) +def start_recreate_index(job_id): + """ + Create backing indexes for a reindex job and fan out indexing batches. + + All indexing writes go only to the new (reindexing) backing indexes; + search keeps using the current default indexes until finish_reindex_job + switches the aliases after every batch has succeeded. + + Args: + job_id (int): TaskJob id + """ + job = TaskJob.objects.get(id=job_id) + # QUEUED: fresh job, do one-time setup. RUNNING: a redelivery (the worker + # died partway through the enqueue loop) — skip setup and just re-enqueue + # whatever batches are still waiting. Anything else is already done. + if job.status not in (TaskJob.Status.QUEUED, TaskJob.Status.RUNNING): + log.info("Reindex job %s not startable (status=%s)", job_id, job.status) + return + indexes = job.params["indexes"] + restart = job.params.get("restart", False) + + if job.status == TaskJob.Status.QUEUED: + try: + error = None + if not restart: + existing_reindexing_indexes = api.get_existing_reindexing_indexes( + indexes + ) + if existing_reindexing_indexes: + error = ( + f"Reindexing in progress. Reindexing indexes already exist: " + f"{', '.join(existing_reindexing_indexes)}" + ) + else: + other_active_jobs = [ + other_job + for other_job in TaskJob.objects.filter( + task_name=REINDEX_TASK_NAME, + status__in=TaskJob.ACTIVE_STATUSES, + ).exclude(id=job_id) + if set(other_job.params.get("indexes", [])) & set(indexes) ] - + [ - index_content_files.si( - ids, - program_resource.id, - index_types=IndexestoUpdate.reindexing_index.value, - resource_type=PROGRAM_TYPE, - ) - for ids in chunks( - ContentFile.objects.filter( - learning_resource_id=program_resource.id, - published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, + if other_active_jobs: + error = ( + f"Reindexing in progress. Active reindex jobs already" + f" exist: " + f"{', '.join(str(other.id) for other in other_active_jobs)}" ) - ] + if error: + log.error(error) + TaskJob.objects.filter(id=job_id).update( + status=TaskJob.Status.FAILED, error=error ) + return - index_tasks = celery.group(index_tasks) - except: # noqa: E722 - error = "start_recreate_index threw an error" - log.exception(error) - return error + api.delete_orphaned_indexes(indexes, delete_reindexing_tags=restart) - # Use self.replace so that code waiting on this task will also wait on the indexing - # and finish tasks - return self.replace( - celery.chain(index_tasks, finish_recreate_index.s(new_backing_indices)) - ) + job.params["backing_indexes"] = { + obj_type: api.create_backing_index(obj_type) for obj_type in indexes + } + job.save() + + log.info("starting to index %s objects...", ", ".join(indexes)) + + TaskBatch.objects.bulk_create( + _build_reindex_batches(job), ignore_conflicts=True + ) + # flip to RUNNING before enqueuing so a redelivery after this point + # resumes the enqueue loop rather than redoing setup + TaskJob.objects.filter(id=job_id, status=TaskJob.Status.QUEUED).update( + status=TaskJob.Status.RUNNING + ) + except Exception: + error = "start_recreate_index threw an error" + log.exception(error) + TaskJob.objects.filter(id=job_id).update( + status=TaskJob.Status.FAILED, error=error + ) + try: + api.delete_orphaned_indexes(indexes, delete_reindexing_tags=True) + except Exception: + log.exception( + "Failed to clean up reindexing indexes for job %s", job_id + ) + return + + # (re)enqueue every batch still waiting; idempotent under redelivery, so a + # worker death mid-loop can't strand batches in QUEUED + for batch_id in job.batches.filter(status=TaskBatch.Status.QUEUED).values_list( + "id", flat=True + ): + run_reindex_batch.delay(batch_id) + # handles the edge case of a job with no batches at all + _maybe_finish_reindex_job(job_id) @app.task( @@ -1111,40 +1333,81 @@ def get_update_learning_resource_tasks(resource_type): ] +class _FinishReindexJobTask(app.Task): + """ + Base task that fails the job if finish_reindex_job gives up. + + If the alias switch keeps erroring until autoretries are exhausted, the job + would otherwise hang in FINISHING. on_failure marks it FAILED so it doesn't + stay active forever; the old index keeps serving and an operator can re-run. + """ + + def on_failure(self, exc, task_id, args, kwargs, einfo): # noqa: ARG002 + job_id = args[0] if args else kwargs.get("job_id") + TaskJob.objects.filter(id=job_id, status=TaskJob.Status.FINISHING).update( + status=TaskJob.Status.FAILED, + error=f"finish_reindex_job gave up: {exc}", + ) + + @app.task( + base=_FinishReindexJobTask, acks_late=True, reject_on_worker_lost=True, autoretry_for=(RetryError, SystemExit), retry_backoff=True, rate_limit=settings.CELERY_SEARCH_RATE_LIMIT, ) -def finish_recreate_index(results, backing_indices): +def finish_reindex_job(job_id): """ - Swap reindex backing index with default backing index + Swap the reindex backing indexes with the default backing indexes once + every batch of the job has succeeded, or clean up if any batch failed. + + Safe to re-run: already-switched object types are skipped, so a redelivery + can never delete the newly promoted backing index. Args: - results (list or bool): Results saying whether the error exists - backing_indices (dict): The backing OpenSearch indices keyed by object type + job_id (int): TaskJob id """ - errors = merge_strings(results) + job = TaskJob.objects.get(id=job_id) + if job.status != TaskJob.Status.FINISHING: + log.info("Skipping finish for reindex job %s (status=%s)", job_id, job.status) + return + + backing_indexes = job.params.get("backing_indexes", {}) + errors = [ + f"{batch_key}: {error}" + for batch_key, error in job.batches.filter( + status=TaskBatch.Status.FAILED + ).values_list("batch_key", "error") + ] if errors: try: api.delete_orphaned_indexes( - list(backing_indices.keys()), delete_reindexing_tags=True + list(backing_indexes.keys()), delete_reindexing_tags=True ) except RequestError as ex: raise RetryError(str(ex)) from ex msg = f"Errors occurred during recreate_index: {errors}" + TaskJob.objects.filter(id=job_id, status=TaskJob.Status.FINISHING).update( + status=TaskJob.Status.FAILED, error=msg + ) raise ReindexError(msg) log.info( "Done with temporary index. Pointing default aliases to newly created backing indexes..." # noqa: E501 ) - for obj_type, backing_index in backing_indices.items(): + for obj_type, backing_index in backing_indexes.items(): try: + if api.is_default_backing_index(backing_index, obj_type): + # already switched by a previous delivery of this task + continue api.switch_indices(backing_index, obj_type) except RequestError as ex: raise RetryError(str(ex)) from ex + TaskJob.objects.filter(id=job_id, status=TaskJob.Status.FINISHING).update( + status=TaskJob.Status.SUCCEEDED + ) log.info("recreate_index has finished successfully!") clear_views_cache() diff --git a/learning_resources_search/tasks_test.py b/learning_resources_search/tasks_test.py index e8f3a67730..654c7672c7 100644 --- a/learning_resources_search/tasks_test.py +++ b/learning_resources_search/tasks_test.py @@ -27,8 +27,11 @@ COURSE_TYPE, HYBRID_COMBINED_INDEX, LEARNING_RESOURCE_TYPES, + PERCOLATE_INDEX_TYPE, PROGRAM_TYPE, + REINDEX_TASK_NAME, IndexestoUpdate, + ReindexBatchKind, ) from learning_resources_search.exceptions import ReindexError, RetryError from learning_resources_search.factories import PercolateQueryFactory @@ -42,12 +45,14 @@ _get_percolated_rows, _group_percolated_rows, _infer_percolate_group, + _maybe_finish_reindex_job, bulk_deindex_learning_resources, deindex_document, deindex_run_content_files, - finish_recreate_index, + finish_reindex_job, index_learning_resources, index_run_content_files, + run_reindex_batch, send_subscription_emails, start_recreate_index, start_update_index, @@ -56,7 +61,8 @@ upsert_learning_resource, wrap_retry_exception, ) -from main.factories import UserFactory +from main.factories import TaskBatchFactory, TaskJobFactory, UserFactory +from main.models import TaskBatch, TaskJob from main.test_utils import assert_not_raises pytestmark = pytest.mark.django_db @@ -140,12 +146,13 @@ def test_system_exit_retry(mocker): ["combined_hybrid"], ], ) -def test_start_recreate_index(mocker, mocked_celery, user, indexes): # noqa: C901, PLR0915 +def test_start_recreate_index(mocker, indexes): # noqa: C901, PLR0912, PLR0915 """ - recreate_index should recreate the OpenSearch index and reindex all data with it + recreate_index should create backing indexes and batch rows for all data """ settings.OPENSEARCH_INDEXING_CHUNK_SIZE = 2 settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE = 2 + settings.OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE = 2 mock_blocklist = mocker.patch( "learning_resources_search.tasks.load_course_blocklist", return_value=[] @@ -159,12 +166,6 @@ def test_start_recreate_index(mocker, mocked_celery, user, indexes): # noqa: C9 for course in ocw_courses: ContentFileFactory.create_batch(3, run=course.learning_resource.runs.first()) - # A resource-level (marketing page) content file attached directly to the - # learning resource rather than a run. - course_marketing_file = ContentFileFactory.create( - learning_resource=ocw_courses[0].learning_resource - ) - oll_courses = CourseFactory.create_batch(2, etl_source=ETLSource.ocw.value) courses = sorted( @@ -180,21 +181,8 @@ def test_start_recreate_index(mocker, mocked_celery, user, indexes): # noqa: C9 key=lambda program: program.learning_resource_id, ) - # Attach both a run-level and a resource-level (marketing page) content file - # to one program to exercise program content file indexing. - program_with_files = programs[0] - program_run_file = ContentFileFactory.create( - run=program_with_files.learning_resource.runs.first() - ) - program_marketing_file = ContentFileFactory.create( - learning_resource=program_with_files.learning_resource - ) - - index_learning_resources_mock = mocker.patch( - "learning_resources_search.tasks.index_learning_resources", autospec=True - ) - index_files_mock = mocker.patch( - "learning_resources_search.tasks.index_content_files", autospec=True + run_reindex_batch_mock = mocker.patch( + "learning_resources_search.tasks.run_reindex_batch", autospec=True ) backing_index = "backing" @@ -211,123 +199,98 @@ def test_start_recreate_index(mocker, mocked_celery, user, indexes): # noqa: C9 delete_orphaned_indexes_mock = mocker.patch( "learning_resources_search.indexing_api.delete_orphaned_indexes", autospec=True ) - finish_recreate_index_mock = mocker.patch( - "learning_resources_search.tasks.finish_recreate_index", autospec=True + + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, params={"indexes": indexes} ) + start_recreate_index.delay(job.id) - finish_recreate_index_dict = {} - - with pytest.raises(mocked_celery.replace_exception_class): - start_recreate_index.delay(indexes, remove_existing_reindexing_tags=False) + job.refresh_from_db() + assert job.status == TaskJob.Status.RUNNING + assert job.error == "" for doctype in [COURSE_TYPE, PROGRAM_TYPE, HYBRID_COMBINED_INDEX]: if doctype in indexes: - finish_recreate_index_dict[doctype] = backing_index + assert job.params["backing_indexes"][doctype] == backing_index create_backing_index_mock.assert_any_call(doctype) - finish_recreate_index_mock.s.assert_called_once_with(finish_recreate_index_dict) - delete_orphaned_indexes_mock.assert_called_once_with( indexes, delete_reindexing_tags=False ) - assert mocked_celery.group.call_count == 1 + resource_batches = job.batches.filter( + kind=ReindexBatchKind.learning_resources.value + ).order_by("id") + dispatch_batches = job.batches.filter( + kind=ReindexBatchKind.dispatch_content_files.value + ).order_by("id") - # Celery's 'group' function takes a generator as an argument. In order to make assertions about the items - # in that generator, 'list' is being called to force iteration through all of those items. - list(mocked_celery.group.call_args[0][0]) + # no content file batches until dispatch batches run + assert not job.batches.filter(kind=ReindexBatchKind.content_files.value).exists() if COURSE_TYPE in indexes: - assert index_learning_resources_mock.si.call_count == 3 + assert resource_batches.count() == 3 if PROGRAM_TYPE in indexes: - assert index_learning_resources_mock.si.call_count == 2 + assert resource_batches.count() == 2 if HYBRID_COMBINED_INDEX in indexes: - assert index_learning_resources_mock.si.call_count == 5 + assert resource_batches.count() == 5 + + resource_id_chunks = [batch.params["ids"] for batch in resource_batches] + for batch in resource_batches: + assert batch.params["index_name"] == indexes[0] if COURSE_TYPE in indexes or HYBRID_COMBINED_INDEX in indexes: mock_blocklist.assert_called_once() - index_type = indexes[0] - index_learning_resources_mock.si.assert_any_call( + for chunk in ( [courses[0].learning_resource_id, courses[1].learning_resource_id], - index_type, - index_types=IndexestoUpdate.reindexing_index.value, - ) - index_learning_resources_mock.si.assert_any_call( [courses[2].learning_resource_id, courses[3].learning_resource_id], - index_type, - index_types=IndexestoUpdate.reindexing_index.value, - ) - index_learning_resources_mock.si.assert_any_call( [courses[4].learning_resource_id, courses[5].learning_resource_id], - index_type, - index_types=IndexestoUpdate.reindexing_index.value, - ) - - if COURSE_TYPE in indexes: - for course in ocw_courses: - content_file_ids = ( - course.learning_resource.runs.first() - .content_files.order_by("id") - .values_list("id", flat=True) - ) - index_files_mock.si.assert_any_call( - [content_file_ids[0], content_file_ids[1]], - course.learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - ) - - index_files_mock.si.assert_any_call( - [content_file_ids[2]], - course.learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - ) - - # resource-level (marketing page) content file attached directly to the - # learning resource - index_files_mock.si.assert_any_call( - [course_marketing_file.id], - ocw_courses[0].learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - ) - - if PROGRAM_TYPE in indexes: - # Program content files are indexed with resource_type=PROGRAM_TYPE, for - # both run-level and resource-level (marketing page) content files. - index_files_mock.si.assert_any_call( - [program_run_file.id], - program_with_files.learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - resource_type=PROGRAM_TYPE, - ) - index_files_mock.si.assert_any_call( - [program_marketing_file.id], - program_with_files.learning_resource_id, - index_types=IndexestoUpdate.reindexing_index.value, - resource_type=PROGRAM_TYPE, - ) + ): + assert chunk in resource_id_chunks if PROGRAM_TYPE in indexes or HYBRID_COMBINED_INDEX in indexes: - index_type = indexes[0] - index_learning_resources_mock.si.assert_any_call( + for chunk in ( [programs[0].learning_resource_id, programs[1].learning_resource_id], - index_type, - index_types=IndexestoUpdate.reindexing_index.value, - ) - index_learning_resources_mock.si.assert_any_call( [programs[2].learning_resource_id, programs[3].learning_resource_id], - index_type, - index_types=IndexestoUpdate.reindexing_index.value, - ) - assert mocked_celery.replace.call_count == 1 - assert mocked_celery.replace.call_args[0][1] == mocked_celery.chain.return_value + ): + assert chunk in resource_id_chunks + + if COURSE_TYPE in indexes: + # all 6 courses are resource-file (ocw) courses, dispatched in chunks of 2 + assert dispatch_batches.count() == 3 + dispatched_ids = [ + resource_id + for batch in dispatch_batches + for resource_id in batch.params["learning_resource_ids"] + ] + assert dispatched_ids == [course.learning_resource_id for course in courses] + for batch in dispatch_batches: + assert batch.params["resource_type"] == COURSE_TYPE + elif PROGRAM_TYPE in indexes: + assert dispatch_batches.count() == 2 + dispatched_ids = [ + resource_id + for batch in dispatch_batches + for resource_id in batch.params["learning_resource_ids"] + ] + assert dispatched_ids == [program.learning_resource_id for program in programs] + for batch in dispatch_batches: + assert batch.params["resource_type"] == PROGRAM_TYPE + else: + assert dispatch_batches.count() == 0 + + # every batch was enqueued + assert run_reindex_batch_mock.delay.call_count == job.batches.count() + enqueued_ids = { + call.args[0] for call in run_reindex_batch_mock.delay.call_args_list + } + assert enqueued_ids == set(job.batches.values_list("id", flat=True)) @pytest.mark.parametrize("indexes", [["course"], ["combined_hybrid"]]) -def test_start_recreate_index_excludes_blocklisted_courses( - mocker, mocked_celery, indexes -): +def test_start_recreate_index_excludes_blocklisted_courses(mocker, indexes): """start_recreate_index should not index courses whose readable_id is blocklisted""" courses = CourseFactory.create_batch(3, etl_source=ETLSource.ocw.value) blocked = courses[0] @@ -337,54 +300,115 @@ def test_start_recreate_index_excludes_blocklisted_courses( "learning_resources_search.tasks.load_course_blocklist", return_value=[blocked.learning_resource.readable_id], ) - index_learning_resources_mock = mocker.patch( - "learning_resources_search.tasks.index_learning_resources", autospec=True - ) - index_files_mock = mocker.patch( - "learning_resources_search.tasks.index_content_files", autospec=True - ) + mocker.patch("learning_resources_search.tasks.run_reindex_batch", autospec=True) mocker.patch( "learning_resources_search.indexing_api.get_existing_reindexing_indexes", autospec=True, return_value=[], ) # ponytail: no assertions on these; they just keep the task off OpenSearch - for target in ( + # (create_backing_index must return a string so it can be stored in the + # job's JSON params) + mocker.patch( "learning_resources_search.indexing_api.create_backing_index", + autospec=True, + return_value="backing", + ) + mocker.patch( "learning_resources_search.indexing_api.delete_orphaned_indexes", - "learning_resources_search.tasks.finish_recreate_index", - ): - mocker.patch(target, autospec=True) + autospec=True, + ) - with pytest.raises(mocked_celery.replace_exception_class): - start_recreate_index.delay(indexes, remove_existing_reindexing_tags=False) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, params={"indexes": indexes} + ) + start_recreate_index.delay(job.id) + + job.refresh_from_db() + assert job.status == TaskJob.Status.RUNNING indexed_resource_ids = { resource_id - for call in index_learning_resources_mock.si.call_args_list - for resource_id in call.args[0] + for batch in job.batches.filter(kind=ReindexBatchKind.learning_resources.value) + for resource_id in batch.params["ids"] } assert indexed_resource_ids == { course.learning_resource_id for course in courses[1:] } if COURSE_TYPE in indexes: - file_course_ids = {call.args[1] for call in index_files_mock.si.call_args_list} - assert file_course_ids == { - course.learning_resource_id for course in courses[1:] + dispatched_ids = { + resource_id + for batch in job.batches.filter( + kind=ReindexBatchKind.dispatch_content_files.value + ) + for resource_id in batch.params["learning_resource_ids"] } + assert dispatched_ids == {course.learning_resource_id for course in courses[1:]} + + +def test_start_recreate_index_percolate(mocker): + """start_recreate_index should chunk and enqueue percolate query batches""" + settings.OPENSEARCH_INDEXING_CHUNK_SIZE = 2 + + PercolateQuery.objects.bulk_create(PercolateQueryFactory.build_batch(5)) + percolate_queries = list(PercolateQuery.objects.order_by("id")) + + run_reindex_batch_mock = mocker.patch( + "learning_resources_search.tasks.run_reindex_batch", autospec=True + ) + mocker.patch( + "learning_resources_search.indexing_api.create_backing_index", + autospec=True, + return_value="backing", + ) + mocker.patch( + "learning_resources_search.indexing_api.get_existing_reindexing_indexes", + autospec=True, + return_value=[], + ) + mocker.patch( + "learning_resources_search.indexing_api.delete_orphaned_indexes", autospec=True + ) + + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, params={"indexes": [PERCOLATE_INDEX_TYPE]} + ) + start_recreate_index.delay(job.id) + + job.refresh_from_db() + assert job.status == TaskJob.Status.RUNNING + + percolate_batches = job.batches.filter( + kind=ReindexBatchKind.percolate.value + ).order_by("batch_key") + # 5 queries in chunks of 2 -> 3 batches, and only percolate batches + assert percolate_batches.count() == 3 + assert job.batches.count() == 3 + + id_chunks = [batch.params["ids"] for batch in percolate_batches] + assert id_chunks == [ + [percolate_queries[0].id, percolate_queries[1].id], + [percolate_queries[2].id, percolate_queries[3].id], + [percolate_queries[4].id], + ] + + # every percolate batch was enqueued + enqueued_ids = { + call.args[0] for call in run_reindex_batch_mock.delay.call_args_list + } + assert enqueued_ids == set(percolate_batches.values_list("id", flat=True)) @pytest.mark.parametrize( - "remove_existing_reindexing_tags", + "restart", [True, False], ) -def test_start_recreate_index_existing_reindexing_index( - mocker, mocked_celery, user, remove_existing_reindexing_tags -): +def test_start_recreate_index_existing_reindexing_index(mocker, restart): """start_recreate_index should stop when reindexing indexes already exist.""" settings.OPENSEARCH_INDEXING_CHUNK_SIZE = 2 settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE = 2 + settings.OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE = 2 indexes = ["program"] programs = sorted( @@ -392,8 +416,8 @@ def test_start_recreate_index_existing_reindexing_index( key=lambda program: program.learning_resource_id, ) - index_learning_resources_mock = mocker.patch( - "learning_resources_search.tasks.index_learning_resources", autospec=True + run_reindex_batch_mock = mocker.patch( + "learning_resources_search.tasks.run_reindex_batch", autospec=True ) backing_index = "backing" @@ -405,9 +429,6 @@ def test_start_recreate_index_existing_reindexing_index( delete_orphaned_indexes_mock = mocker.patch( "learning_resources_search.indexing_api.delete_orphaned_indexes", autospec=True ) - finish_recreate_index_mock = mocker.patch( - "learning_resources_search.tasks.finish_recreate_index", autospec=True - ) mocker.patch( "learning_resources_search.indexing_api.get_existing_reindexing_indexes", @@ -415,81 +436,222 @@ def test_start_recreate_index_existing_reindexing_index( return_value=["another_reindexing_index"], ) - if remove_existing_reindexing_tags: - with pytest.raises(mocked_celery.replace_exception_class): - start_recreate_index.delay( - indexes, remove_existing_reindexing_tags=remove_existing_reindexing_tags - ) - else: - start_recreate_index.delay( - indexes, remove_existing_reindexing_tags=remove_existing_reindexing_tags - ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={ + "indexes": indexes, + "restart": restart, + }, + ) + start_recreate_index.delay(job.id) - finish_recreate_index_dict = {"program": "backing"} + job.refresh_from_db() - if remove_existing_reindexing_tags: + if restart: + assert job.status == TaskJob.Status.RUNNING delete_orphaned_indexes_mock.assert_called_once_with( indexes, delete_reindexing_tags=True ) + assert job.params["backing_indexes"] == {"program": "backing"} - finish_recreate_index_mock.s.assert_called_once_with(finish_recreate_index_dict) - assert mocked_celery.group.call_count == 1 - - # Celery's 'group' function takes a generator as an argument. In order to make assertions about the items - # in that generator, 'list' is being called to force iteration through all of those items. - list(mocked_celery.group.call_args[0][0]) - assert index_learning_resources_mock.si.call_count == 2 - index_learning_resources_mock.si.assert_any_call( - [programs[0].learning_resource_id, programs[1].learning_resource_id], - PROGRAM_TYPE, - index_types=IndexestoUpdate.reindexing_index.value, + resource_batches = job.batches.filter( + kind=ReindexBatchKind.learning_resources.value ) - index_learning_resources_mock.si.assert_any_call( + assert resource_batches.count() == 2 + resource_id_chunks = [batch.params["ids"] for batch in resource_batches] + for chunk in ( + [programs[0].learning_resource_id, programs[1].learning_resource_id], [programs[2].learning_resource_id, programs[3].learning_resource_id], - PROGRAM_TYPE, - index_types=IndexestoUpdate.reindexing_index.value, - ) - - assert mocked_celery.replace.call_count == 1 - assert mocked_celery.replace.call_args[0][1] == mocked_celery.chain.return_value + ): + assert chunk in resource_id_chunks + assert run_reindex_batch_mock.delay.call_count == job.batches.count() else: - assert index_learning_resources_mock.si.call_count == 0 - assert mocked_celery.replace.call_count == 0 + assert job.status == TaskJob.Status.FAILED + assert "another_reindexing_index" in job.error + assert job.batches.count() == 0 + assert run_reindex_batch_mock.delay.call_count == 0 + delete_orphaned_indexes_mock.assert_not_called() + + +def test_start_recreate_index_existing_active_job(mocker): + """start_recreate_index should stop when another active reindex job overlaps""" + mocker.patch( + "learning_resources_search.indexing_api.get_existing_reindexing_indexes", + autospec=True, + return_value=[], + ) + run_reindex_batch_mock = mocker.patch( + "learning_resources_search.tasks.run_reindex_batch", autospec=True + ) + other_job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["program"]}, + status=TaskJob.Status.RUNNING, + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, params={"indexes": ["program"]} + ) + + start_recreate_index.delay(job.id) + + job.refresh_from_db() + assert job.status == TaskJob.Status.FAILED + assert str(other_job.id) in job.error + assert run_reindex_batch_mock.delay.call_count == 0 + + +def test_start_recreate_index_resumes_running_job(mocker): + """ + A redelivery of start_recreate_index for a RUNNING job (worker died mid + enqueue loop) should re-enqueue still-queued batches without redoing setup + """ + create_backing_index_mock = mocker.patch( + "learning_resources_search.indexing_api.create_backing_index", autospec=True + ) + delete_orphaned_indexes_mock = mocker.patch( + "learning_resources_search.indexing_api.delete_orphaned_indexes", autospec=True + ) + run_reindex_batch_mock = mocker.patch( + "learning_resources_search.tasks.run_reindex_batch", autospec=True + ) + finish_mock = mocker.patch( + "learning_resources_search.tasks.finish_reindex_job", autospec=True + ) + + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["course"], "backing_indexes": {"course": "backing"}}, + status=TaskJob.Status.RUNNING, + ) + # one batch already finished in the first attempt, one never got enqueued + done_batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + status=TaskBatch.Status.SUCCEEDED, + ) + stranded_batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + status=TaskBatch.Status.QUEUED, + ) + + start_recreate_index.delay(job.id) + + # setup must NOT be redone (recreating backing indexes would orphan the + # documents the finished batch already wrote) + create_backing_index_mock.assert_not_called() + delete_orphaned_indexes_mock.assert_not_called() + # only the still-queued batch is (re)enqueued + run_reindex_batch_mock.delay.assert_called_once_with(stranded_batch.id) + # no new batch rows created + assert job.batches.count() == 2 + assert done_batch.job_id == job.id + # job is not complete yet (a queued batch remains), so finish isn't claimed + finish_mock.delay.assert_not_called() @pytest.mark.parametrize("with_error", [True, False]) -def test_finish_recreate_index(mocker, with_error): +def test_finish_reindex_job(mocker, with_error): """ - finish_recreate_index should attach the backing index to the default alias + finish_reindex_job should attach the backing index to the default alias """ - backing_indices = {"course": "backing", "program": "backing"} - results = ["error"] if with_error else [] + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={ + "indexes": ["course", "program"], + "backing_indexes": {"course": "backing", "program": "backing"}, + }, + status=TaskJob.Status.FINISHING, + ) + TaskBatchFactory.create(job=job, status=TaskBatch.Status.SUCCEEDED) + if with_error: + TaskBatchFactory.create(job=job, status=TaskBatch.Status.FAILED, error="error") switch_indices_mock = mocker.patch( "learning_resources_search.indexing_api.switch_indices", autospec=True ) mock_delete_orphans = mocker.patch( "learning_resources_search.indexing_api.delete_orphaned_indexes" ) + mocker.patch( + "learning_resources_search.indexing_api.is_default_backing_index", + autospec=True, + return_value=False, + ) if with_error: with pytest.raises(ReindexError): - finish_recreate_index.delay(results, backing_indices) + finish_reindex_job.delay(job.id) switch_indices_mock.assert_not_called() mock_delete_orphans.assert_called_once() + job.refresh_from_db() + assert job.status == TaskJob.Status.FAILED + assert "error" in job.error else: - finish_recreate_index.delay(results, backing_indices) + finish_reindex_job.delay(job.id) switch_indices_mock.assert_any_call("backing", COURSE_TYPE) switch_indices_mock.assert_any_call("backing", PROGRAM_TYPE) mock_delete_orphans.assert_not_called() + job.refresh_from_db() + assert job.status == TaskJob.Status.SUCCEEDED + + +def test_finish_reindex_job_skips_already_switched(mocker): + """ + A redelivered finish_reindex_job should not re-switch (and thereby delete) + a backing index that is already the default + """ + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["course"], "backing_indexes": {"course": "backing"}}, + status=TaskJob.Status.FINISHING, + ) + switch_indices_mock = mocker.patch( + "learning_resources_search.indexing_api.switch_indices", autospec=True + ) + mocker.patch( + "learning_resources_search.indexing_api.is_default_backing_index", + autospec=True, + return_value=True, + ) + + finish_reindex_job.delay(job.id) + + switch_indices_mock.assert_not_called() + job.refresh_from_db() + assert job.status == TaskJob.Status.SUCCEEDED + + +def test_finish_reindex_job_noop_when_not_finishing(mocker): + """finish_reindex_job should do nothing unless the job is in finishing state""" + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["course"], "backing_indexes": {"course": "backing"}}, + status=TaskJob.Status.SUCCEEDED, + ) + switch_indices_mock = mocker.patch( + "learning_resources_search.indexing_api.switch_indices", autospec=True + ) + + finish_reindex_job.delay(job.id) + + switch_indices_mock.assert_not_called() @pytest.mark.parametrize("with_error", [True, False]) -def test_finish_recreate_index_retry_exceptions(mocker, with_error): +def test_finish_reindex_job_retry_exceptions(mocker, with_error): """ - finish_recreate_index should be retried on RequestErrors + finish_reindex_job should be retried on RequestErrors """ - backing_indices = {"course": "backing", "program": "backing"} - results = ["error"] if with_error else [] + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={ + "indexes": ["course", "program"], + "backing_indexes": {"course": "backing", "program": "backing"}, + }, + status=TaskJob.Status.FINISHING, + ) + if with_error: + TaskBatchFactory.create(job=job, status=TaskBatch.Status.FAILED, error="error") mock_error = RequestError(429, "oops", {}) switch_indices_mock = mocker.patch( "learning_resources_search.indexing_api.switch_indices", @@ -500,9 +662,17 @@ def test_finish_recreate_index_retry_exceptions(mocker, with_error): "learning_resources_search.indexing_api.delete_orphaned_indexes", side_effect=[mock_error, None], ) + mocker.patch( + "learning_resources_search.indexing_api.is_default_backing_index", + autospec=True, + return_value=False, + ) with pytest.raises(Retry): - finish_recreate_index.delay(results, backing_indices) + finish_reindex_job.delay(job.id) + job.refresh_from_db() + # the job stays in finishing state so the retry can pick it back up + assert job.status == TaskJob.Status.FINISHING if with_error: switch_indices_mock.assert_not_called() mock_delete_orphans.assert_called_once() @@ -511,6 +681,315 @@ def test_finish_recreate_index_retry_exceptions(mocker, with_error): switch_indices_mock.assert_called_once() +def test_run_reindex_batch_learning_resources(mocker, mocked_api): + """run_reindex_batch should index learning resources for that batch kind""" + finish_mock = mocker.patch( + "learning_resources_search.tasks.finish_reindex_job", autospec=True + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + params={"ids": [1, 2], "index_name": COURSE_TYPE}, + ) + + run_reindex_batch.delay(batch.id) + + mocked_api.index_learning_resources.assert_called_once_with( + [1, 2], COURSE_TYPE, IndexestoUpdate.reindexing_index.value + ) + batch.refresh_from_db() + assert batch.status == TaskBatch.Status.SUCCEEDED + # the last batch of the job claims the finish step + job.refresh_from_db() + assert job.status == TaskJob.Status.FINISHING + finish_mock.delay.assert_called_once_with(job.id) + + +def test_run_reindex_batch_content_files(mocker, mocked_api): + """run_reindex_batch should index content files for that batch kind""" + mocker.patch("learning_resources_search.tasks.finish_reindex_job", autospec=True) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.content_files.value, + params={ + "ids": [3, 4], + "learning_resource_id": 7, + "resource_type": PROGRAM_TYPE, + }, + ) + + run_reindex_batch.delay(batch.id) + + mocked_api.index_content_files.assert_called_once_with( + [3, 4], + 7, + index_types=IndexestoUpdate.reindexing_index.value, + resource_type=PROGRAM_TYPE, + ) + batch.refresh_from_db() + assert batch.status == TaskBatch.Status.SUCCEEDED + + +def test_run_reindex_batch_percolate(mocker, mocked_api): + """run_reindex_batch should index percolate queries for that batch kind""" + mocker.patch("learning_resources_search.tasks.finish_reindex_job", autospec=True) + serialize_mock = mocker.patch( + "learning_resources_search.tasks.serialize_bulk_percolators", + return_value=["serialized"], + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.percolate.value, + params={"ids": [5, 6]}, + ) + + run_reindex_batch.delay(batch.id) + + serialize_mock.assert_called_once_with([5, 6]) + mocked_api.index_items.assert_called_once_with( + ["serialized"], + PERCOLATE_INDEX_TYPE, + IndexestoUpdate.reindexing_index.value, + ) + batch.refresh_from_db() + assert batch.status == TaskBatch.Status.SUCCEEDED + + +def test_run_reindex_batch_dispatch_content_files(mocker, mocked_api): + """ + A dispatch batch should create and enqueue content file batches, idempotently + """ + settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE = 2 + course = CourseFactory.create(etl_source=ETLSource.ocw.value) + run = course.learning_resource.runs.first() + run_files = sorted( + ContentFileFactory.create_batch(3, run=run), key=lambda file: file.id + ) + marketing_file = ContentFileFactory.create( + learning_resource=course.learning_resource + ) + delay_mock = mocker.patch.object(run_reindex_batch, "delay") + + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.dispatch_content_files.value, + params={ + "learning_resource_ids": [course.learning_resource_id], + "resource_type": COURSE_TYPE, + }, + ) + + run_reindex_batch(batch.id) + + children = job.batches.filter(kind=ReindexBatchKind.content_files.value).order_by( + "batch_key" + ) + assert children.count() == 3 + child_params = [child.params for child in children] + assert { + "ids": [run_files[0].id, run_files[1].id], + "learning_resource_id": course.learning_resource_id, + "resource_type": COURSE_TYPE, + } in child_params + assert { + "ids": [run_files[2].id], + "learning_resource_id": course.learning_resource_id, + "resource_type": COURSE_TYPE, + } in child_params + assert { + "ids": [marketing_file.id], + "learning_resource_id": course.learning_resource_id, + "resource_type": COURSE_TYPE, + } in child_params + + batch.refresh_from_db() + assert batch.status == TaskBatch.Status.SUCCEEDED + assert delay_mock.call_count == 3 + + # re-running the dispatch (e.g. on redelivery) must not duplicate children + TaskBatch.objects.filter(id=batch.id).update(status=TaskBatch.Status.QUEUED) + run_reindex_batch(batch.id) + assert job.batches.filter(kind=ReindexBatchKind.content_files.value).count() == 3 + + +def test_run_reindex_batch_error(mocker, mocked_api): + """run_reindex_batch should mark the batch failed on a non-retryable error""" + finish_mock = mocker.patch( + "learning_resources_search.tasks.finish_reindex_job", autospec=True + ) + mocked_api.index_learning_resources.side_effect = ValueError("boom") + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + params={"ids": [1], "index_name": COURSE_TYPE}, + ) + + run_reindex_batch.delay(batch.id) + + batch.refresh_from_db() + assert batch.status == TaskBatch.Status.FAILED + assert "boom" in batch.error + # failed batches still count toward completion so the job can finish/clean up + job.refresh_from_db() + assert job.status == TaskJob.Status.FINISHING + finish_mock.delay.assert_called_once_with(job.id) + + +def test_run_reindex_batch_retry(mocker, mocked_api): + """run_reindex_batch should retry on search connection errors""" + mocker.patch("learning_resources_search.tasks.finish_reindex_job", autospec=True) + mocked_api.index_learning_resources.side_effect = ESConnectionError( + "err", "err", "err" + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + params={"ids": [1], "index_name": COURSE_TYPE}, + ) + + with pytest.raises(Retry): + run_reindex_batch.delay(batch.id) + + batch.refresh_from_db() + # the batch is left non-terminal so the celery retry re-runs it + assert batch.status == TaskBatch.Status.RUNNING + + +def test_run_reindex_batch_on_failure_fails_batch(mocker): + """ + When run_reindex_batch gives up (retries exhausted) its on_failure handler + should mark the batch FAILED and nudge the job toward completion + """ + finish_mock = mocker.patch( + "learning_resources_search.tasks.finish_reindex_job", autospec=True + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + status=TaskBatch.Status.RUNNING, + ) + + run_reindex_batch.on_failure(RetryError("boom"), "task-id", [batch.id], {}, None) + + batch.refresh_from_db() + assert batch.status == TaskBatch.Status.FAILED + assert "boom" in batch.error + # it was the only batch, so the job is now claimable for finishing + job.refresh_from_db() + assert job.status == TaskJob.Status.FINISHING + finish_mock.delay.assert_called_once_with(job.id) + + +def test_finish_reindex_job_on_failure_fails_job(): + """ + When finish_reindex_job gives up its on_failure handler should mark the + FINISHING job FAILED so it doesn't hang active forever + """ + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={"indexes": ["course"], "backing_indexes": {"course": "backing"}}, + status=TaskJob.Status.FINISHING, + ) + + finish_reindex_job.on_failure( + RetryError("switch failed"), "task-id", [job.id], {}, None + ) + + job.refresh_from_db() + assert job.status == TaskJob.Status.FAILED + assert "switch failed" in job.error + + +@pytest.mark.parametrize( + ("batch_status", "job_status"), + [ + (TaskBatch.Status.SUCCEEDED, TaskJob.Status.RUNNING), + (TaskBatch.Status.QUEUED, TaskJob.Status.FAILED), + ], +) +def test_run_reindex_batch_noop(mocker, mocked_api, batch_status, job_status): + """run_reindex_batch should do nothing if the batch or job is terminal""" + mocker.patch("learning_resources_search.tasks.finish_reindex_job", autospec=True) + job = TaskJobFactory.create(task_name=REINDEX_TASK_NAME, status=job_status) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.learning_resources.value, + status=batch_status, + params={"ids": [1], "index_name": COURSE_TYPE}, + ) + + run_reindex_batch.delay(batch.id) + + mocked_api.index_learning_resources.assert_not_called() + batch.refresh_from_db() + assert batch.status == batch_status + + +def test_maybe_finish_reindex_job(mocker): + """_maybe_finish_reindex_job should claim the finish exactly once""" + finish_mock = mocker.patch( + "learning_resources_search.tasks.finish_reindex_job", autospec=True + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + done_batch = TaskBatchFactory.create(job=job, status=TaskBatch.Status.SUCCEEDED) + pending_batch = TaskBatchFactory.create(job=job, status=TaskBatch.Status.QUEUED) + + _maybe_finish_reindex_job(job.id) + job.refresh_from_db() + assert job.status == TaskJob.Status.RUNNING + finish_mock.delay.assert_not_called() + + TaskBatch.objects.filter(id=pending_batch.id).update(status=TaskBatch.Status.FAILED) + _maybe_finish_reindex_job(job.id) + job.refresh_from_db() + assert job.status == TaskJob.Status.FINISHING + finish_mock.delay.assert_called_once_with(job.id) + + # a second caller cannot claim the finish again + _maybe_finish_reindex_job(job.id) + assert finish_mock.delay.call_count == 1 + assert done_batch.job_id == job.id + + +def test_maybe_finish_reindex_job_no_batches(mocker): + """A job with no batches at all should finish immediately""" + finish_mock = mocker.patch( + "learning_resources_search.tasks.finish_reindex_job", autospec=True + ) + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + + _maybe_finish_reindex_job(job.id) + + job.refresh_from_db() + assert job.status == TaskJob.Status.FINISHING + finish_mock.delay.assert_called_once_with(job.id) + + @pytest.mark.usefixtures("_wrap_retry_mock") @pytest.mark.parametrize("with_error", [True, False]) @pytest.mark.parametrize( @@ -1379,11 +1858,22 @@ def test_cache_is_cleared_after_reindex(mocker): "learning_resources_search.tasks.clear_views_cache" ) - backing_indices = {"course": "backing", "program": "backing"} - results = [] + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, + params={ + "indexes": ["course", "program"], + "backing_indexes": {"course": "backing", "program": "backing"}, + }, + status=TaskJob.Status.FINISHING, + ) mocker.patch("learning_resources_search.indexing_api.switch_indices", autospec=True) mocker.patch("learning_resources_search.indexing_api.delete_orphaned_indexes") - finish_recreate_index.delay(results, backing_indices) + mocker.patch( + "learning_resources_search.indexing_api.is_default_backing_index", + autospec=True, + return_value=False, + ) + finish_reindex_job.delay(job.id) assert mocked_clear_views_cache.call_count == 1 diff --git a/main/admin.py b/main/admin.py new file mode 100644 index 0000000000..81c7460d42 --- /dev/null +++ b/main/admin.py @@ -0,0 +1,37 @@ +"""admin for main""" + +from django.contrib import admin + +from main import models + + +class TaskJobAdmin(admin.ModelAdmin): + """TaskJob Admin""" + + model = models.TaskJob + list_display = ("id", "task_name", "status", "created_on", "updated_on") + list_filter = ("status", "task_name") + readonly_fields = ("created_on", "updated_on") + + +admin.site.register(models.TaskJob, TaskJobAdmin) + + +class TaskBatchAdmin(admin.ModelAdmin): + """TaskBatch Admin""" + + model = models.TaskBatch + list_display = ( + "id", + "job", + "batch_key", + "kind", + "status", + "updated_on", + ) + list_filter = ("status", "kind") + search_fields = ("batch_key",) + readonly_fields = ("created_on", "updated_on") + + +admin.site.register(models.TaskBatch, TaskBatchAdmin) diff --git a/main/factories.py b/main/factories.py index 02ab8b93a2..e9a8a7552f 100644 --- a/main/factories.py +++ b/main/factories.py @@ -11,6 +11,7 @@ LazyFunction, RelatedFactory, SelfAttribute, + Sequence, SubFactory, Trait, ) @@ -18,6 +19,8 @@ from factory.fuzzy import FuzzyText from social_django.models import UserSocialAuth +from main.models import TaskBatch, TaskJob + class UserFactory(DjangoModelFactory): """Factory for Users""" @@ -62,3 +65,25 @@ class UserSocialAuthFactory(DjangoModelFactory): class Meta: model = UserSocialAuth + + +class TaskJobFactory(DjangoModelFactory): + """Factory for TaskJobs""" + + task_name = FuzzyText() + params = LazyFunction(dict) + + class Meta: + model = TaskJob + + +class TaskBatchFactory(DjangoModelFactory): + """Factory for TaskBatches""" + + job = SubFactory(TaskJobFactory) + batch_key = Sequence(lambda n: f"batch:{n}") + kind = FuzzyText() + params = LazyFunction(dict) + + class Meta: + model = TaskBatch diff --git a/main/migrations/0001_taskjob_taskbatch.py b/main/migrations/0001_taskjob_taskbatch.py new file mode 100644 index 0000000000..d17f4f38e9 --- /dev/null +++ b/main/migrations/0001_taskjob_taskbatch.py @@ -0,0 +1,102 @@ +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + initial = True + + dependencies = [] + + operations = [ + migrations.CreateModel( + name="TaskJob", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("created_on", models.DateTimeField(auto_now_add=True, db_index=True)), + ("updated_on", models.DateTimeField(auto_now=True)), + ("task_name", models.CharField(db_index=True, max_length=255)), + ("params", models.JSONField(default=dict)), + ( + "status", + models.CharField( + choices=[ + ("queued", "Queued"), + ("running", "Running"), + ("finishing", "Finishing"), + ("succeeded", "Succeeded"), + ("failed", "Failed"), + ], + db_index=True, + default="queued", + max_length=20, + ), + ), + ("error", models.TextField(blank=True)), + ], + options={ + "abstract": False, + }, + ), + migrations.CreateModel( + name="TaskBatch", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("created_on", models.DateTimeField(auto_now_add=True, db_index=True)), + ("updated_on", models.DateTimeField(auto_now=True)), + ("batch_key", models.CharField(max_length=255)), + ("kind", models.CharField(max_length=64)), + ("params", models.JSONField(default=dict)), + ( + "status", + models.CharField( + choices=[ + ("queued", "Queued"), + ("running", "Running"), + ("succeeded", "Succeeded"), + ("failed", "Failed"), + ], + db_index=True, + default="queued", + max_length=20, + ), + ), + ("error", models.TextField(blank=True)), + ( + "job", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="batches", + to="main.taskjob", + ), + ), + ], + ), + migrations.AddIndex( + model_name="taskbatch", + index=models.Index( + fields=["job", "status"], name="taskbatch_job_status_idx" + ), + ), + migrations.AddConstraint( + model_name="taskbatch", + constraint=models.UniqueConstraint( + fields=("job", "batch_key"), name="unique_task_batch_key" + ), + ), + ] diff --git a/main/migrations/__init__.py b/main/migrations/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/main/models.py b/main/models.py index 4bafc925ff..431fe9ee5f 100644 --- a/main/models.py +++ b/main/models.py @@ -2,6 +2,7 @@ Classes related to models for main """ +from django.db import models from django.db.models import DateTimeField, Model from django.db.models.query import QuerySet @@ -52,3 +53,70 @@ class NoDefaultTimestampedModel(TimestampedModel): class Meta: abstract = True + + +class TaskJob(TimestampedModel): + """ + Tracks a long-running celery task that has been decomposed into batches + (TaskBatch rows), so its progress and completion live in the database + rather than in any single worker process + """ + + class Status(models.TextChoices): + QUEUED = "queued" # created; the start task has not completed yet + RUNNING = "running" # batches created and executing + FINISHING = "finishing" # all batches done; finish step claimed + SUCCEEDED = "succeeded" + FAILED = "failed" + + ACTIVE_STATUSES = (Status.QUEUED, Status.RUNNING, Status.FINISHING) + + task_name = models.CharField(max_length=255, db_index=True) + params = models.JSONField(default=dict) + status = models.CharField( + max_length=20, + choices=Status.choices, + default=Status.QUEUED, + db_index=True, + ) + error = models.TextField(blank=True) + + def __str__(self): + return f"Task job {self.id} {self.task_name} ({self.status})" + + +class TaskBatch(TimestampedModel): + """A unit of work for a TaskJob""" + + class Status(models.TextChoices): + QUEUED = "queued" # waiting for a worker to pick it up + RUNNING = "running" # a worker has started executing it + SUCCEEDED = "succeeded" + FAILED = "failed" + + NON_TERMINAL_STATUSES = (Status.QUEUED, Status.RUNNING) + + job = models.ForeignKey(TaskJob, on_delete=models.CASCADE, related_name="batches") + batch_key = models.CharField(max_length=255) + kind = models.CharField(max_length=64) + params = models.JSONField(default=dict) + status = models.CharField( + max_length=20, + choices=Status.choices, + default=Status.QUEUED, + db_index=True, + ) + error = models.TextField(blank=True) + + class Meta: + constraints = [ + models.UniqueConstraint( + fields=["job", "batch_key"], name="unique_task_batch_key" + ) + ] + indexes = [ + models.Index(fields=["job", "status"], name="taskbatch_job_status_idx") + ] + + def __str__(self): + return f"Task batch {self.batch_key} ({self.status}) for job {self.job_id}" diff --git a/main/settings.py b/main/settings.py index 621d1c45ef..c791de962f 100644 --- a/main/settings.py +++ b/main/settings.py @@ -545,6 +545,12 @@ "OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE", get_int("OPENSEARCH_INDEXING_CHUNK_SIZE", 100), ) +# learning resources per dispatch_content_files reindex batch +OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE = get_int( + "OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE", 100 +) +# how long finished TaskJobs (and their batches) are kept before cleanup +TASK_JOB_RETENTION_DAYS = get_int("TASK_JOB_RETENTION_DAYS", 7) OPENSEARCH_MIN_QUERY_SIZE = get_int("OPENSEARCH_MIN_QUERY_SIZE", 2) OPENSEARCH_MAX_SUGGEST_HITS = get_int("OPENSEARCH_MAX_SUGGEST_HITS", 1) OPENSEARCH_MAX_SUGGEST_RESULTS = get_int("OPENSEARCH_MAX_SUGGEST_RESULTS", 1) diff --git a/main/settings_celery.py b/main/settings_celery.py index 62f11fce73..70452ff21e 100644 --- a/main/settings_celery.py +++ b/main/settings_celery.py @@ -164,6 +164,10 @@ "task": "learning_resources_search.tasks.update_featured_rank", "schedule": crontab(minute=30, hour=7), # 3:30am EST }, + "delete-old-task-jobs-every-1-days": { + "task": "main.tasks.delete_old_task_jobs", + "schedule": crontab(minute=0, hour=8), # 4:00am EST + }, "scrape-marketing-pages-every-1-days": { "task": "learning_resources.tasks.scrape_marketing_pages", "schedule": get_int( diff --git a/main/tasks.py b/main/tasks.py new file mode 100644 index 0000000000..90183524fc --- /dev/null +++ b/main/tasks.py @@ -0,0 +1,45 @@ +"""Helpers for batched task jobs (TaskJob / TaskBatch)""" + +import datetime +import logging + +from django.conf import settings + +from main.celery import app +from main.models import TaskBatch, TaskJob +from main.utils import now_in_utc + +log = logging.getLogger(__name__) + + +def maybe_finish_task_job(job_id, finish_task): + """ + Claim and enqueue the finish step if every batch of the job is done. + + The conditional UPDATE guarantees exactly one caller claims the finish; + each batch commits its terminal status before calling this, so the last + batch to finish is guaranteed to see all batches terminal. + + Args: + job_id (int): TaskJob id + finish_task (celery task): task taking a TaskJob id that finalizes it + """ + claimed = ( + TaskJob.objects.filter(id=job_id, status=TaskJob.Status.RUNNING) + .exclude(batches__status__in=TaskBatch.NON_TERMINAL_STATUSES) + .update(status=TaskJob.Status.FINISHING) + ) + if claimed: + finish_task.delay(job_id) + + +@app.task +def delete_old_task_jobs(): + """ + Delete TaskJobs (and, by cascade, their batches) with no activity in the + retention window. A job untouched for that long is treated as inactive, + whatever its status. + """ + threshold = now_in_utc() - datetime.timedelta(days=settings.TASK_JOB_RETENTION_DAYS) + deleted, _ = TaskJob.objects.filter(updated_on__lt=threshold).delete() + log.info("Deleted %d old task job/batch rows", deleted) diff --git a/main/tasks_test.py b/main/tasks_test.py new file mode 100644 index 0000000000..340d75cc8b --- /dev/null +++ b/main/tasks_test.py @@ -0,0 +1,55 @@ +"""Tests for batched task job helpers""" + +import datetime + +import pytest +from django.conf import settings + +from main.factories import TaskBatchFactory, TaskJobFactory +from main.models import TaskBatch, TaskJob +from main.tasks import delete_old_task_jobs, maybe_finish_task_job +from main.utils import now_in_utc + +pytestmark = pytest.mark.django_db + + +def test_maybe_finish_task_job(mocker): + """maybe_finish_task_job should claim the finish once all batches terminal""" + finish_task = mocker.Mock() + job = TaskJobFactory.create(status=TaskJob.Status.RUNNING) + TaskBatchFactory.create(job=job, status=TaskBatch.Status.SUCCEEDED) + running_batch = TaskBatchFactory.create(job=job, status=TaskBatch.Status.RUNNING) + + # a running (non-terminal) batch must block completion + maybe_finish_task_job(job.id, finish_task) + job.refresh_from_db() + assert job.status == TaskJob.Status.RUNNING + finish_task.delay.assert_not_called() + + TaskBatch.objects.filter(id=running_batch.id).update(status=TaskBatch.Status.FAILED) + maybe_finish_task_job(job.id, finish_task) + job.refresh_from_db() + assert job.status == TaskJob.Status.FINISHING + finish_task.delay.assert_called_once_with(job.id) + + # a second caller cannot claim the finish again + maybe_finish_task_job(job.id, finish_task) + assert finish_task.delay.call_count == 1 + + +def test_delete_old_task_jobs(): + """Jobs untouched past the retention window are deleted with their batches""" + old_job = TaskJobFactory.create(status=TaskJob.Status.SUCCEEDED) + old_batch = TaskBatchFactory.create(job=old_job) + recent_job = TaskJobFactory.create(status=TaskJob.Status.RUNNING) + recent_batch = TaskBatchFactory.create(job=recent_job) + + stale = now_in_utc() - datetime.timedelta(days=settings.TASK_JOB_RETENTION_DAYS + 1) + TaskJob.objects.filter(id=old_job.id).update(updated_on=stale) + + delete_old_task_jobs.delay() + + assert not TaskJob.objects.filter(id=old_job.id).exists() + assert not TaskBatch.objects.filter(id=old_batch.id).exists() # cascaded + assert TaskJob.objects.filter(id=recent_job.id).exists() + assert TaskBatch.objects.filter(id=recent_batch.id).exists() From 8d0e5fe33a0245cd37e18a3d5780065db631bce3 Mon Sep 17 00:00:00 2001 From: Danielle Frappier Date: Thu, 6 Aug 2026 10:21:52 -0400 Subject: [PATCH 05/12] Fix: HTML leaking into meta description tags (#3727) --- .../main/src/common/htmlToPlainText.test.ts | 43 +++++++++++++++++++ frontends/main/src/common/htmlToPlainText.ts | 33 ++++++++++++++ frontends/main/src/common/metadata.test.ts | 12 ++++++ frontends/main/src/common/metadata.ts | 4 +- 4 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 frontends/main/src/common/htmlToPlainText.test.ts create mode 100644 frontends/main/src/common/htmlToPlainText.ts diff --git a/frontends/main/src/common/htmlToPlainText.test.ts b/frontends/main/src/common/htmlToPlainText.test.ts new file mode 100644 index 0000000000..f1762ced8c --- /dev/null +++ b/frontends/main/src/common/htmlToPlainText.test.ts @@ -0,0 +1,43 @@ +import { htmlToPlainText } from "./htmlToPlainText" + +describe("htmlToPlainText", () => { + it("strips tags and decodes entities", () => { + expect(htmlToPlainText("

Daryl Morey & Jessica Gelman

")).toBe( + "Daryl Morey & Jessica Gelman", + ) + }) + + it("keeps a space between adjacent block-level elements", () => { + expect(htmlToPlainText("

First

Second

")).toBe("First Second") + }) + + it("keeps a space where a
separates lines", () => { + expect(htmlToPlainText("Line one
Line two")).toBe("Line one Line two") + }) + + it("keeps a space between adjacent table cells", () => { + expect( + htmlToPlainText("
AB
"), + ).toBe("A B") + }) + + it("keeps a space after a blockquote or pre block", () => { + expect( + htmlToPlainText("
Quote
Code
Text"), + ).toBe("Quote Code Text") + }) + + it("strips links but keeps their text", () => { + expect( + htmlToPlainText('

OCW resources

'), + ).toBe("OCW resources") + }) + + it("leaves plain text unchanged", () => { + expect(htmlToPlainText("Just plain text")).toBe("Just plain text") + }) + + it("returns an empty string for empty input", () => { + expect(htmlToPlainText("")).toBe("") + }) +}) diff --git a/frontends/main/src/common/htmlToPlainText.ts b/frontends/main/src/common/htmlToPlainText.ts new file mode 100644 index 0000000000..d2f540e882 --- /dev/null +++ b/frontends/main/src/common/htmlToPlainText.ts @@ -0,0 +1,33 @@ +import DOMPurify from "isomorphic-dompurify" +import { collapseWhitespace } from "@/common/utils" + +const BLOCK_BOUNDARY_TAGS = + /<\/(?:p|div|li|h[1-6]|td|th|tr|blockquote|pre)>|/gi + +/** + * Converts a sanitized-HTML string (e.g. a resource `description`) to plain + * text suitable for contexts that must not contain markup, like , og:description, and twitter:description. Strips all + * tags and decodes entities (& -> &); a space is inserted at block-level + * boundaries first so adjacent paragraphs/list items don't get mashed + * together once their tags are removed. + * + * Kept out of common/utils.ts and imported only by server-only code (e.g. + * metadata.ts): isomorphic-dompurify has no `sideEffects: false`, so any + * client component importing anything from utils.ts would otherwise pull + * DOMPurify into its bundle even when htmlToPlainText itself is unused. + */ +const htmlToPlainText = (html: string): string => { + if (!html) return "" + const withBreaks = html.replace(BLOCK_BOUNDARY_TAGS, (match) => `${match} `) + // RETURN_DOM_FRAGMENT gives back real DOM nodes rather than a serialized + // HTML string, so reading .textContent decodes entities for free (a + // serialized-string result stays HTML-escaped, e.g. "&", since it's + // meant to be re-inserted as HTML). + const fragment = DOMPurify.sanitize(withBreaks, { + RETURN_DOM_FRAGMENT: true, + }) + return collapseWhitespace(fragment.textContent ?? "") +} + +export { htmlToPlainText } diff --git a/frontends/main/src/common/metadata.test.ts b/frontends/main/src/common/metadata.test.ts index 32c935b42e..7abefca8c6 100644 --- a/frontends/main/src/common/metadata.test.ts +++ b/frontends/main/src/common/metadata.test.ts @@ -68,6 +68,18 @@ describe("safeGenerateMetadata", () => { }) }) +describe("standardizeMetadata", () => { + test("converts an HTML description to plain text in all description fields", async () => { + const meta = await standardizeMetadata({ + description: "

Daryl Morey & Jessica Gelman

", + }) + + expect(meta.description).toBe("Daryl Morey & Jessica Gelman") + expect(meta.openGraph?.description).toBe("Daryl Morey & Jessica Gelman") + expect(meta.twitter?.description).toBe("Daryl Morey & Jessica Gelman") + }) +}) + describe("getMetadataAsync drawer canonical", () => { test("emits a slugged separate-param canonical for a valid ?resource=", async () => { const resource = factories.learningResources.course() diff --git a/frontends/main/src/common/metadata.ts b/frontends/main/src/common/metadata.ts index 3cae33da60..53aa8e9111 100644 --- a/frontends/main/src/common/metadata.ts +++ b/frontends/main/src/common/metadata.ts @@ -4,6 +4,7 @@ import { RESOURCE_DRAWER_PARAMS, } from "@/common/urls" import { parseResourceId } from "@/common/slugs" +import { htmlToPlainText } from "@/common/htmlToPlainText" import type { AxiosError } from "axios" import type { Metadata } from "next" import * as Sentry from "@sentry/nextjs" @@ -86,7 +87,7 @@ export const getMetadataAsync = async ({ learningResourceQueries.detail(learningResourceId), ) title = data?.title - description = data?.description?.replace(/<\/[^>]+(>|$)/g, "") ?? "" + description = data?.description ?? "" image = data?.image?.url || image imageAlt = image === data?.image?.url ? imageAlt : data?.image?.alt || "" alts.canonical = canonicalResourceDrawerUrl(learningResourceId, data?.title) @@ -118,6 +119,7 @@ export const standardizeMetadata = ({ ...otherMeta }: MetadataProps = {}): Metadata => { title = `${title} | ${env("NEXT_PUBLIC_SITE_NAME")}` + description = htmlToPlainText(description) const socialMetadata = social ? { openGraph: { From b907280bbd6f0858a6b7a3236784d928ab7980cf Mon Sep 17 00:00:00 2001 From: Anastasia Beglova Date: Thu, 6 Aug 2026 12:59:27 -0400 Subject: [PATCH 06/12] remove GITHUB_ACCESS_TOKEN (#3730) --- learning_resources/etl/podcast.py | 5 +---- learning_resources/etl/podcast_test.py | 4 +--- main/settings_course_etl.py | 3 --- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/learning_resources/etl/podcast.py b/learning_resources/etl/podcast.py index 43838ca212..4eb78c2208 100644 --- a/learning_resources/etl/podcast.py +++ b/learning_resources/etl/podcast.py @@ -40,10 +40,7 @@ def github_podcast_config_files(): A list of pyGithub contentFile objects """ # noqa: D401 - if settings.GITHUB_ACCESS_TOKEN: - github_client = github.Github(settings.GITHUB_ACCESS_TOKEN) - else: - github_client = github.Github() + github_client = github.Github() repo = github_client.get_repo(CONFIG_FILE_REPO) diff --git a/learning_resources/etl/podcast_test.py b/learning_resources/etl/podcast_test.py index 1025c188a5..64d57abddf 100644 --- a/learning_resources/etl/podcast_test.py +++ b/learning_resources/etl/podcast_test.py @@ -326,10 +326,8 @@ def test_generate_aggregate_podcast_rss(): assert result == bs(expected_rss, "xml").prettify() -@pytest.mark.parametrize("github_token", [None, "token"]) -def test_github_podcast_config_files(settings, mock_github_client, github_token): +def test_github_podcast_config_files(settings, mock_github_client): """Test the logic for retrieving podcast config files from github""" - settings.GITHUB_ACCESS_TOKEN = github_token mock_github_client.return_value.get_repo.return_value.get_contents.return_value = [ mock_podcast_file(), mock_podcast_file(), diff --git a/main/settings_course_etl.py b/main/settings_course_etl.py index c4211b5661..fe27d826ce 100644 --- a/main/settings_course_etl.py +++ b/main/settings_course_etl.py @@ -14,9 +14,6 @@ "EDX_COURSE_BUCKET_PREFIX", "edxorg-raw-data/edxorg/raw_data/course_xml/" ) -# Authentication for the github api -GITHUB_ACCESS_TOKEN = get_string("GITHUB_ACCESS_TOKEN", None) - # OCW settings OCW_LIVE_BUCKET = get_string("OCW_LIVE_BUCKET", None) OCW_ITERATOR_CHUNK_SIZE = get_int("OCW_ITERATOR_CHUNK_SIZE", 1000) From 39c0d3f3bd4f338fba3c3a91262fa279f24c1902 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Fri, 7 Aug 2026 08:00:43 -0400 Subject: [PATCH 07/12] Skip unchanged edX course archives before downloading from S3 (#3722) --- learning_resources/etl/edx_shared.py | 48 ++++- learning_resources/etl/edx_shared_test.py | 197 +++++++++++++++++- learning_resources/etl/loaders_test.py | 7 +- .../0121_learningresourcerun_archive_key.py | 17 ++ learning_resources/models.py | 2 + learning_resources/serializers.py | 8 +- 6 files changed, 258 insertions(+), 21 deletions(-) create mode 100644 learning_resources/migrations/0121_learningresourcerun_archive_key.py diff --git a/learning_resources/etl/edx_shared.py b/learning_resources/etl/edx_shared.py index 365210123d..e75c93bde5 100644 --- a/learning_resources/etl/edx_shared.py +++ b/learning_resources/etl/edx_shared.py @@ -1,6 +1,7 @@ """Shared functions for EdX sites""" import logging +from itertools import chain from pathlib import Path from tarfile import ReadError from tempfile import TemporaryDirectory @@ -114,9 +115,10 @@ def build_run_lookup( def process_course_archive( bucket, key: str, run: LearningResourceRun, *, overwrite: bool = False -) -> None: +) -> bool: """ - Download and process a course archive from S3. + Download and process a course archive from S3, skipping the download + entirely when run.archive_key already matches the content-addressed key. Args: bucket: S3 bucket object @@ -125,8 +127,11 @@ def process_course_archive( overwrite(bool): Whether to overwrite existing content files Returns: - bool: True if successfully processed, False if skipped due to matching checksum + bool: False if skipped via matching archive_key, True otherwise """ + if run.archive_key == key and not overwrite: + log.debug("Archive key unchanged for %s, skipping download", key) + return False with TemporaryDirectory() as export_tempdir: course_tarpath = Path(export_tempdir, key.rsplit("/", maxsplit=1)[-1]) log.info("course tarpath for run %s is %s", run.run_id, course_tarpath) @@ -135,20 +140,35 @@ def process_course_archive( checksum = calc_checksum(course_tarpath) except ReadError: log.exception("Error reading tar file %s, skipping", course_tarpath) - return False + return True if run.checksum == checksum and not overwrite: + # unchanged content under a new key: record it to skip future downloads + run.archive_key = key + run.save(update_fields=["archive_key"]) log.info("Checksums match for %s, skipping load", key) - return False + return True try: + content_files_data = iter( + transform_content_files(course_tarpath, run, overwrite=overwrite) + ) + first = next(content_files_data, None) + if first is None: + # empty archive: stop re-downloading it + run.archive_key = key + run.save(update_fields=["archive_key"]) + return True content_files_ids = load_content_files( - run, - transform_content_files(course_tarpath, run, overwrite=overwrite), + run, chain([first], content_files_data) ) if content_files_ids: run.checksum = checksum - run.save(update_fields=["checksum"]) + run.archive_key = key + run.save(update_fields=["checksum", "archive_key"]) + # else: files yielded but none loaded — save nothing so the next + # sync retries except: # noqa: E722 log.exception("Error ingesting OLX content data for %s", key) + return True def get_most_recent_course_archives(etl_source: str) -> list[str]: @@ -315,6 +335,7 @@ def sync_edx_course_files( bucket = get_bucket_by_name(settings.COURSE_ARCHIVE_BUCKET_NAME) run_lookup = build_run_lookup(etl_source, ids) + skipped = processed = 0 for key in keys: normalized_key_id = extract_run_id_from_key(etl_source, key) matching_runs = run_lookup.get(normalized_key_id) @@ -327,4 +348,13 @@ def sync_edx_course_files( log.warning("There are %d runs for %s", len(matching_runs), key) run = matching_runs[0] - process_course_archive(bucket, key, run, overwrite=overwrite) + if process_course_archive(bucket, key, run, overwrite=overwrite): + processed += 1 + else: + skipped += 1 + log.info( + "%s content file sync: %d unchanged archives skipped, %d processed", + etl_source, + skipped, + processed, + ) diff --git a/learning_resources/etl/edx_shared_test.py b/learning_resources/etl/edx_shared_test.py index d702f84861..31a0070fa2 100644 --- a/learning_resources/etl/edx_shared_test.py +++ b/learning_resources/etl/edx_shared_test.py @@ -161,6 +161,39 @@ def test_sync_edx_course_files_matching_checksum(mocker, mock_course_archive_buc mock_load.assert_not_called() mock_index.assert_not_called() + run.refresh_from_db() + assert run.archive_key == key + + +def test_sync_edx_course_files_skips_unchanged_archive_keys(mocker): + """Keys matching a run's archive_key skip without downloading; summary logged""" + run = LearningResourceFactory.create( + is_course=True, create_runs=True, etl_source=ETLSource.mitxonline.name + ).best_run + key = ( + f"{get_s3_prefix_for_source(ETLSource.mitxonline.name)}/{run.run_id}/foo.tar.gz" + ) + run.archive_key = key + run.save() + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.get_bucket_by_name", + return_value=bucket, + ) + mock_load = mocker.patch("learning_resources.etl.edx_shared.load_content_files") + mock_log = mocker.patch("learning_resources.etl.edx_shared.log.info") + + sync_edx_course_files("mitxonline", [run.learning_resource.id], [key]) + + bucket.download_file.assert_not_called() + mock_load.assert_not_called() + mock_log.assert_any_call( + "%s content file sync: %d unchanged archives skipped, %d processed", + "mitxonline", + 1, + 0, + ) + @pytest.mark.parametrize("source", [ETLSource.mitxonline.value, ETLSource.xpro.value]) def test_sync_edx_course_files_invalid_tarfile( @@ -257,7 +290,10 @@ def test_sync_edx_course_files_error(mock_course_archive_bucket, mocker, source) sync_edx_course_files(source, [run.learning_resource.id], [key]) assert mock_transform.call_count == 1 assert str(mock_transform.call_args[0][0]).endswith("foo.tar.gz") is True - mock_load_content_files.assert_called_once_with(run, fake_data) + mock_load_content_files.assert_called_once() + called_run, called_data = mock_load_content_files.call_args[0] + assert called_run == run + assert list(called_data) == list(fake_data) assert mock_log.call_args[0][0].startswith("Error ingesting OLX content data for ") @@ -358,8 +394,13 @@ def test_sync_edx_course_files_test_mode_all_runs_processed( assert mock_load_content_files.call_count == 3 # Verify each run was processed + called_runs = [call.args[0] for call in mock_load_content_files.call_args_list] + called_data = [ + list(call.args[1]) for call in mock_load_content_files.call_args_list + ] for run in runs: - mock_load_content_files.assert_any_call(run, fake_data) + assert run in called_runs + assert called_data == [list(fake_data)] * 3 @pytest.mark.parametrize("source", [ETLSource.mit_edx.value, ETLSource.xpro.value]) @@ -681,7 +722,10 @@ def test_sync_edx_archive_success( sync_edx_archive(etl_source, s3_key, overwrite=False) mock_transform.assert_called_once() - mock_load.assert_called_once_with(run, '{"key": "data"}') + mock_load.assert_called_once() + called_run, called_data = mock_load.call_args[0] + assert called_run == run + assert list(called_data) == list('{"key": "data"}') run.refresh_from_db() assert run.checksum is not None @@ -868,7 +912,10 @@ def test_sync_edx_archive_test_mode_all_runs( sync_edx_archive(etl_source, s3_key, overwrite=False) mock_transform.assert_called_once() - mock_load.assert_called_once_with(old_run, '{"key": "data"}') + mock_load.assert_called_once() + called_run, called_data = mock_load.call_args[0] + assert called_run == old_run + assert list(called_data) == list('{"key": "data"}') @pytest.mark.parametrize("etl_source", [ETLSource.mitxonline.name, ETLSource.xpro.name]) @@ -1315,8 +1362,8 @@ def test_build_run_lookup_cross_format_prefix_match(): assert lookup[normalized_key][0].id == run.id -def test_process_course_archive_does_not_set_checksum_on_empty_ingest(mocker): - """process_course_archive should not update run.checksum if load_content_files returns empty list""" +def test_process_course_archive_saves_nothing_when_all_files_fail_ingest(mocker): + """process_course_archive should not update run.checksum or run.archive_key if all files fail to load""" run = LearningResourceRunFactory.create(published=True, checksum=None) bucket = mocker.MagicMock() key = "mitxonline/courses/course-v1:Test+Course+R1/archive.tar.gz" @@ -1327,17 +1374,22 @@ def test_process_course_archive_does_not_set_checksum_on_empty_ingest(mocker): ) mocker.patch( "learning_resources.etl.edx_shared.transform_content_files", - return_value=iter([]), + return_value=iter([{"key": "content.txt"}]), ) + + def fake_load(run_arg, data, **kwargs): + list(data) # consume the generator like the real loader + return [] + mocker.patch( - "learning_resources.etl.edx_shared.load_content_files", - return_value=[], + "learning_resources.etl.edx_shared.load_content_files", side_effect=fake_load ) process_course_archive(bucket, key, run) run.refresh_from_db() assert run.checksum is None + assert run.archive_key is None def test_process_course_archive_sets_checksum_on_successful_ingest(mocker): @@ -1352,7 +1404,7 @@ def test_process_course_archive_sets_checksum_on_successful_ingest(mocker): ) mocker.patch( "learning_resources.etl.edx_shared.transform_content_files", - return_value=iter([]), + return_value=iter([{"key": "content.txt"}]), ) mocker.patch( "learning_resources.etl.edx_shared.load_content_files", @@ -1388,3 +1440,128 @@ def test_process_course_archive_does_not_set_checksum_on_exception(mocker): run.refresh_from_db() assert run.checksum == "oldchecksum" + + +def test_process_course_archive_skips_download_when_key_matches(mocker): + """A stored archive_key equal to the S3 key should skip without downloading""" + key = "mitxonline/openedx/raw_data/course_xml/course-v1:Test+Course+R1/abc123.xml.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=key, checksum="oldchecksum" + ) + bucket = mocker.MagicMock() + mock_load = mocker.patch("learning_resources.etl.edx_shared.load_content_files") + + process_course_archive(bucket, key, run) + + bucket.download_file.assert_not_called() + mock_load.assert_not_called() + run.refresh_from_db() + assert run.archive_key == key + assert run.checksum == "oldchecksum" + + +def test_process_course_archive_stamps_key_on_checksum_match(mocker): + """A matching checksum with a stale archive_key should stamp the key, no load""" + key = "mitxonline/openedx/raw_data/course_xml/course-v1:Test+Course+R1/abc123.xml.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=None, checksum="samechecksum" + ) + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.calc_checksum", return_value="samechecksum" + ) + mock_load = mocker.patch("learning_resources.etl.edx_shared.load_content_files") + + process_course_archive(bucket, key, run) + + bucket.download_file.assert_called_once() + mock_load.assert_not_called() + run.refresh_from_db() + assert run.archive_key == key + assert run.checksum == "samechecksum" + + +def test_process_course_archive_saves_both_fields_on_load(mocker): + """A changed archive should load and save both checksum and archive_key""" + key = "mitxonline/openedx/raw_data/course_xml/course-v1:Test+Course+R1/abc123.xml.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=None, checksum=None + ) + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.calc_checksum", return_value="newchecksum" + ) + mocker.patch( + "learning_resources.etl.edx_shared.transform_content_files", + return_value=iter([{"key": "content.txt"}]), + ) + + def fake_load(run_arg, data, **kwargs): + list(data) # consume the generator like the real loader + return [1, 2, 3] + + mocker.patch( + "learning_resources.etl.edx_shared.load_content_files", side_effect=fake_load + ) + + process_course_archive(bucket, key, run) + + run.refresh_from_db() + assert run.archive_key == key + assert run.checksum == "newchecksum" + + +def test_process_course_archive_stamps_key_for_empty_archive(mocker): + """An archive whose transform yields no payloads should stamp archive_key only""" + key = "mitxonline/openedx/raw_data/course_xml/course-v1:Test+Course+R1/abc123.xml.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=None, checksum=None + ) + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.calc_checksum", return_value="newchecksum" + ) + mocker.patch( + "learning_resources.etl.edx_shared.transform_content_files", + return_value=iter([]), + ) + mock_load = mocker.patch("learning_resources.etl.edx_shared.load_content_files") + + process_course_archive(bucket, key, run) + + mock_load.assert_not_called() + run.refresh_from_db() + assert run.archive_key == key + assert run.checksum is None + + +def test_process_course_archive_overwrite_bypasses_key_gate(mocker): + """overwrite=True should download and load even when the stored key matches""" + key = "mitxonline/openedx/raw_data/course_xml/course-v1:Test+Course+R1/abc123.xml.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=key, checksum=None + ) + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.calc_checksum", return_value="newchecksum" + ) + mocker.patch( + "learning_resources.etl.edx_shared.transform_content_files", + return_value=iter([{"key": "content.txt"}]), + ) + + def fake_load(run_arg, data, **kwargs): + list(data) + return [1] + + mock_load = mocker.patch( + "learning_resources.etl.edx_shared.load_content_files", side_effect=fake_load + ) + + process_course_archive(bucket, key, run, overwrite=True) + + bucket.download_file.assert_called_once() + mock_load.assert_called_once() + run.refresh_from_db() + assert run.archive_key == key + assert run.checksum == "newchecksum" diff --git a/learning_resources/etl/loaders_test.py b/learning_resources/etl/loaders_test.py index fa8c962b4a..f6e254c280 100644 --- a/learning_resources/etl/loaders_test.py +++ b/learning_resources/etl/loaders_test.py @@ -2019,6 +2019,10 @@ def test_load_test_mode_resource_content_files( autospec=True, return_value=[], ) + mocker.patch( + "learning_resources.etl.edx_shared.transform_content_files", + return_value=iter(content_data), + ) mocker.patch( "learning_resources_search.plugins.tasks.deindex_run_content_files", autospec=True, @@ -2045,7 +2049,8 @@ def test_load_test_mode_resource_content_files( ) if test_mode: - assert len(mock_load_content_files.mock_calls[0].args) == len(content_data) + _, data_arg = mock_load_content_files.mock_calls[0].args + assert list(data_arg) == content_data else: assert mock_load_content_files.call_count == 0 diff --git a/learning_resources/migrations/0121_learningresourcerun_archive_key.py b/learning_resources/migrations/0121_learningresourcerun_archive_key.py new file mode 100644 index 0000000000..169cbf537c --- /dev/null +++ b/learning_resources/migrations/0121_learningresourcerun_archive_key.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.30 on 2026-07-23 14:14 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("learning_resources", "0120_view_event_uuid_unique_index"), + ] + + operations = [ + migrations.AddField( + model_name="learningresourcerun", + name="archive_key", + field=models.CharField(blank=True, max_length=512, null=True), + ), + ] diff --git a/learning_resources/models.py b/learning_resources/models.py index 9b0f2ac71a..f7d54aa81a 100644 --- a/learning_resources/models.py +++ b/learning_resources/models.py @@ -828,6 +828,8 @@ class LearningResourceRun(TimestampedModel): ) resource_prices = models.ManyToManyField(LearningResourcePrice, blank=True) checksum = models.CharField(max_length=32, null=True, blank=True) # noqa: DJ001 + # S3 key of the last-processed course archive (content-addressed) + archive_key = models.CharField(max_length=512, null=True, blank=True) # noqa: DJ001 delivery = ArrayField( models.CharField( max_length=24, db_index=True, choices=LearningResourceDelivery.as_tuple() diff --git a/learning_resources/serializers.py b/learning_resources/serializers.py index 836dbf55cc..53408e7aa4 100644 --- a/learning_resources/serializers.py +++ b/learning_resources/serializers.py @@ -390,7 +390,13 @@ class LearningResourceRunSerializer(serializers.ModelSerializer): class Meta: model = models.LearningResourceRun - exclude = ["learning_resource", "is_b2b", "is_variant", *COMMON_IGNORED_FIELDS] + exclude = [ + "learning_resource", + "is_b2b", + "is_variant", + "archive_key", + *COMMON_IGNORED_FIELDS, + ] class ResourceListMixin(serializers.Serializer): From 7a6eda79bcc46ebb007efdd2ba75ba68a835e475 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Fri, 7 Aug 2026 16:51:24 -0400 Subject: [PATCH 08/12] Fix Canvas archive change detection: deterministic checksum, save after load (#3728) --- learning_resources/etl/canvas.py | 32 +-- learning_resources/etl/canvas_test.py | 305 ++++++++++++++++++++++++- learning_resources/etl/canvas_utils.py | 39 ++++ learning_resources/etl/utils.py | 12 +- 4 files changed, 357 insertions(+), 31 deletions(-) diff --git a/learning_resources/etl/canvas.py b/learning_resources/etl/canvas.py index 24337d749c..4a71c35d7e 100644 --- a/learning_resources/etl/canvas.py +++ b/learning_resources/etl/canvas.py @@ -15,6 +15,7 @@ PlatformType, ) from learning_resources.etl.canvas_utils import ( + canvas_course_checksum, canvas_course_url, canvas_url_config, get_published_items, @@ -22,7 +23,6 @@ ) from learning_resources.etl.constants import ETLSource from learning_resources.etl.utils import ( - calc_checksum, get_edx_module_id, process_olx_path, ) @@ -52,38 +52,46 @@ def sync_canvas_archive(bucket, key: str, overwrite): course_archive_path = Path(export_tempdir, key.rsplit("/", maxsplit=1)[-1]) bucket.download_file(key, course_archive_path) url_config = canvas_url_config(bucket, export_tempdir, url_config_file) + checksum = canvas_course_checksum(course_archive_path, url_config) resource_readable_id, run = run_for_canvas_archive( - course_archive_path, course_folder=course_folder, overwrite=overwrite + course_archive_path, + course_folder=course_folder, + checksum=checksum, + overwrite=overwrite, ) - checksum = calc_checksum(course_archive_path) if run: canvas_content_files = list( transform_canvas_content_files( course_archive_path, run, url_config=url_config, overwrite=overwrite ) ) - load_content_files( + content_files_ids = load_content_files( run, canvas_content_files, ) - load_problem_files( - run, + canvas_problem_files = list( transform_canvas_problem_files( course_archive_path, run, overwrite=overwrite - ), + ) ) - run.checksum = checksum - run.save() + problem_files_ids = load_problem_files(run, canvas_problem_files) + content_loaded = content_files_ids or not canvas_content_files + # load_problem_file swallows per-file errors and returns None + problems_loaded = any(problem_files_ids) or not canvas_problem_files + if content_loaded and problems_loaded: + # a failed or empty load must be retried on the next sync, so + # only mark processed once everything loaded (or was unpublished) + run.checksum = checksum + run.save(update_fields=["checksum"]) return resource_readable_id -def run_for_canvas_archive(course_archive_path, course_folder, overwrite): +def run_for_canvas_archive(course_archive_path, course_folder, checksum, overwrite): """ Generate and return a LearningResourceRun for a Canvas course """ - checksum = calc_checksum(course_archive_path) course_info = parse_canvas_settings(course_archive_path) course_title = course_info.get("title", f"canvas course {course_folder}") url = canvas_course_url(course_archive_path) @@ -130,8 +138,6 @@ def run_for_canvas_archive(course_archive_path, course_folder, overwrite): if run.checksum == checksum and not overwrite: log.debug("Checksums match for %s, skipping load", readable_id) return resource_readable_id, None - run.checksum = checksum - run.save() return resource_readable_id, run diff --git a/learning_resources/etl/canvas_test.py b/learning_resources/etl/canvas_test.py index 2e1517a308..e182d6febd 100644 --- a/learning_resources/etl/canvas_test.py +++ b/learning_resources/etl/canvas_test.py @@ -3,19 +3,23 @@ import zipfile from datetime import timedelta from pathlib import Path +from types import SimpleNamespace from unittest.mock import MagicMock import pytest from defusedxml import ElementTree +from freezegun import freeze_time from learning_resources.constants import LearningResourceType, PlatformType from learning_resources.etl.canvas import ( run_for_canvas_archive, + sync_canvas_archive, transform_canvas_content_files, transform_canvas_problem_files, ) from learning_resources.etl.canvas_utils import ( _compact_element, + canvas_course_checksum, get_published_items, is_file_published, parse_canvas_files, @@ -179,12 +183,11 @@ def test_run_for_canvas_archive_creates_resource_and_run(tmp_path, mocker): return_value={"course_id": "123", "canvas_domain": "mit.edu"}, ) - mocker.patch("learning_resources.etl.canvas.calc_checksum", return_value="abc123") # No resource exists yet zip_path = tmp_path / "archive.zip" _, run = run_for_canvas_archive( - zip_path, course_folder=course_folder, overwrite=True + zip_path, course_folder=course_folder, checksum="abc123", overwrite=True ) resource = LearningResource.objects.get(readable_id=f"{course_folder}-TEST101") assert resource.title == "Test Course" @@ -193,7 +196,9 @@ def test_run_for_canvas_archive_creates_resource_and_run(tmp_path, mocker): assert resource.platform.code == PlatformType.canvas.name assert run is not None assert run.learning_resource == resource - assert run.checksum == "abc123" + # checksum is only saved after a successful content load in + # sync_canvas_archive, never by run_for_canvas_archive + assert run.checksum is None @pytest.mark.django_db @@ -210,9 +215,6 @@ def test_run_for_canvas_archive_creates_run_if_none_exists(tmp_path, mocker): "learning_resources.etl.canvas_utils.parse_context_xml", return_value={"course_id": "123", "canvas_domain": "mit.edu"}, ) - mocker.patch( - "learning_resources.etl.canvas.calc_checksum", return_value="checksum104" - ) # Create resource with no runs resource = LearningResourceFactory.create( readable_id=f"{course_folder}-TEST104", @@ -226,11 +228,14 @@ def test_run_for_canvas_archive_creates_run_if_none_exists(tmp_path, mocker): course_archive_path = tmp_path / "archive4.zip" course_archive_path.write_text("dummy") _, run = run_for_canvas_archive( - course_archive_path, course_folder=course_folder, overwrite=True + course_archive_path, + course_folder=course_folder, + checksum="checksum104", + overwrite=True, ) assert run is not None assert run.learning_resource == resource - assert run.checksum == "checksum104" + assert run.checksum is None def make_canvas_zip( @@ -1994,7 +1999,9 @@ def test_ingestion_finishes_with_missing_xml_files( "learning_resources.etl.utils.extract_text_metadata", return_value={"content": "test"}, ) - _, run = run_for_canvas_archive(zip_path, tmp_path, overwrite=True) + _, run = run_for_canvas_archive( + zip_path, tmp_path, checksum="abc123", overwrite=True + ) content_results = list( transform_canvas_content_files( Path(zip_path), run, url_config={}, overwrite=True @@ -2088,3 +2095,283 @@ def test_empty_pdf_is_skipped(tmp_path): ) ) assert result == [] + + +LOCK_FILES_XML_TEMPLATE = """ + + + + {display_name} + {unlock_at} + uncategorized + + + +""" + +LOCK_MANIFEST_XML = b""" + + + + + + + +""" + + +def make_timed_lock_zip( # noqa: PLR0913 + tmp_path, + unlock_at, + name="lock_course.zip", + settings_xml=DEFAULT_SETTINGS_XML, + manifest_xml=LOCK_MANIFEST_XML, + display_name="file3", + content=b"one", +): + """Course archive with one file whose visibility is gated by unlock_at""" + zip_path = tmp_path / name + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr("course_settings/course_settings.xml", settings_xml) + zf.writestr( + "course_settings/files_meta.xml", + LOCK_FILES_XML_TEMPLATE.format( + unlock_at=unlock_at, display_name=display_name + ).encode(), + ) + zf.writestr("imsmanifest.xml", manifest_xml) + zf.writestr("web_resources/file3.html", content) + return zip_path + + +UNLOCKED = "2001-01-01T00:00:00" + + +def test_canvas_course_checksum_ignores_export_noise(tmp_path): + """ + Archives differing only in imsmanifest.xml bytes and course_settings/* + bytes AND size must produce the same digest — Canvas rewrites these on + every no-op export (verified across 13 prod course pairs). + """ + a = make_timed_lock_zip(tmp_path, UNLOCKED, name="a.zip") + b = make_timed_lock_zip( + tmp_path, + UNLOCKED, + name="b.zip", + settings_xml=DEFAULT_SETTINGS_XML + b"", + manifest_xml=LOCK_MANIFEST_XML.replace(b"", b""), + ) + assert canvas_course_checksum(a, {}) == canvas_course_checksum(b, {}) + + +def test_canvas_course_checksum_detects_same_size_content_edit(tmp_path): + """ + A content member with identical name and size but different bytes must + change the digest — parity with the per-file archive_checksum gate that + the archive-level skip would otherwise shadow. + """ + a = make_timed_lock_zip( + tmp_path, UNLOCKED, name="a.zip", content=b"one" + ) + b = make_timed_lock_zip( + tmp_path, UNLOCKED, name="b.zip", content=b"two" + ) + assert canvas_course_checksum(a, {}) != canvas_course_checksum(b, {}) + + +def test_canvas_course_checksum_independent_of_member_order(tmp_path): + """Zip directory order must not affect the digest""" + members = [ + ("web_resources/x.html", b"xx"), + ("web_resources/y.html", b"yy"), + ("course_settings/course_settings.xml", DEFAULT_SETTINGS_XML), + ] + digests = [] + for name, order in (("a.zip", members), ("b.zip", members[::-1])): + with zipfile.ZipFile(tmp_path / name, "w") as zf: + for member, content in order: + zf.writestr(member, content) + digests.append(canvas_course_checksum(tmp_path / name, {})) + assert digests[0] == digests[1] + + +def test_canvas_course_checksum_sensitive_to_url_config(tmp_path): + """ + url_config (from the .metadata.json sidecar) feeds ContentFile urls; a + changed url must change the digest even when the archive is unchanged. + """ + zip_path = make_timed_lock_zip(tmp_path, UNLOCKED) + with_url = {"/file3.html": {"url": "https://mit.edu/f/1"}} + reminted = {"/file3.html": {"url": "https://mit.edu/f/2"}} + assert canvas_course_checksum(zip_path, with_url) != canvas_course_checksum( + zip_path, reminted + ) + + +def test_canvas_course_checksum_sensitive_to_display_name(tmp_path): + """ + A same-length display_name change lives in the excluded files_meta.xml; + the publish-set component must still catch it (it feeds content_title). + """ + a = make_timed_lock_zip(tmp_path, UNLOCKED, name="a.zip", display_name="fileA") + b = make_timed_lock_zip(tmp_path, UNLOCKED, name="b.zip", display_name="fileB") + assert canvas_course_checksum(a, {}) != canvas_course_checksum(b, {}) + + +def test_canvas_course_checksum_changes_when_lock_boundary_passes(tmp_path): + """ + The same archive must produce a different checksum once a timed lock + boundary passes — publish status depends on wall-clock time, and the gate + must reprocess when it flips. + """ + zip_path = make_timed_lock_zip(tmp_path, "2026-09-01T00:00:00") + with freeze_time("2026-08-15"): + while_locked = canvas_course_checksum(zip_path, {}) + with freeze_time("2026-09-15"): + after_unlock = canvas_course_checksum(zip_path, {}) + assert while_locked != after_unlock + + +def test_canvas_course_checksum_is_cwd_independent(tmp_path, monkeypatch): + """Checksum must not embed the worker process's working directory""" + zip_path = make_timed_lock_zip(tmp_path, "2001-01-01T00:00:00") + digest = canvas_course_checksum(zip_path, {}) + other_cwd = tmp_path / "elsewhere" + other_cwd.mkdir() + monkeypatch.chdir(other_cwd) + assert canvas_course_checksum(zip_path, {}) == digest + + +@pytest.fixture +def sync_mocks(mocker, tmp_path): + """Bucket/url_config/loader mocks for sync_canvas_archive""" + zip_path = make_timed_lock_zip(tmp_path, "2001-01-01T00:00:00") + + def fake_download(key, dest): + Path(dest).write_bytes(zip_path.read_bytes()) + + bucket = MagicMock() + bucket.download_file.side_effect = fake_download + mocker.patch("learning_resources.etl.canvas.canvas_url_config", return_value={}) + mocker.patch( + "learning_resources.etl.utils.extract_text_metadata", + return_value={"content": "TEXT"}, + ) + mocker.patch( + "learning_resources.etl.canvas_utils.parse_context_xml", + return_value={"course_id": "123", "canvas_domain": "mit.edu"}, + ) + load_content = mocker.patch("learning_resources.etl.loaders.load_content_files") + load_problems = mocker.patch("learning_resources.etl.loaders.load_problem_files") + return SimpleNamespace( + bucket=bucket, load_content=load_content, load_problems=load_problems + ) + + +def _canvas_run(readable_id): + return LearningResource.objects.get(readable_id=readable_id).runs.first() + + +def test_sync_canvas_archive_skips_unchanged_archive(sync_mocks): + """A second sync of an unchanged archive must not reload content""" + readable_id = sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert sync_mocks.load_content.call_count == 1 + first_checksum = _canvas_run(readable_id).checksum + assert first_checksum + + sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert sync_mocks.load_content.call_count == 1 + assert _canvas_run(readable_id).checksum == first_checksum + + +def test_sync_canvas_archive_saves_checksum_only_after_successful_load(sync_mocks): + """ + A failed load must leave the checksum unset so the next sync retries + instead of silently skipping content that was never ingested. + """ + sync_mocks.load_content.side_effect = Exception("load failed") + with pytest.raises(Exception, match="load failed"): + sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + resource = LearningResource.objects.get(etl_source=ETLSource.canvas.name) + assert resource.runs.first().checksum is None + + sync_mocks.load_content.side_effect = None + sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert sync_mocks.load_content.call_count == 2 + assert resource.runs.first().checksum + + +def test_sync_canvas_archive_retries_when_nothing_loads(sync_mocks): + """ + load_content_files returning [] (all files failed without raising) must + NOT save the checksum — the next sync retries instead of skipping content + that was never ingested. + """ + sync_mocks.load_content.return_value = [] + readable_id = sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert _canvas_run(readable_id).checksum is None + + sync_mocks.load_content.return_value = [1] + sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert sync_mocks.load_content.call_count == 2 + assert _canvas_run(readable_id).checksum + + +def test_sync_canvas_archive_retries_when_problem_files_fail(mocker, sync_mocks): + """ + Tutor problem files yielded but none loaded (load_problem_file swallows + per-file errors and returns None) must NOT save the checksum, so the next + sync retries. + """ + mocker.patch( + "learning_resources.etl.canvas.transform_canvas_problem_files", + side_effect=lambda *_args, **_kwargs: iter( + [{"source_path": "tutorbot/p1/problem.pdf"}] + ), + ) + sync_mocks.load_problems.return_value = [None] + readable_id = sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert _canvas_run(readable_id).checksum is None + + sync_mocks.load_problems.return_value = [7] + sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert _canvas_run(readable_id).checksum + + +def test_sync_canvas_archive_saves_checksum_for_legitimately_empty_course( + mocker, sync_mocks, tmp_path +): + """ + A course with nothing published yields no payloads and loads nothing; + that's not a failure — save the checksum so it isn't reprocessed weekly. + (The publish-set digest component unfreezes it if anything is published.) + """ + locked_zip = make_timed_lock_zip( + tmp_path, "2099-01-01T00:00:00", name="all_locked.zip" + ) + + def fake_download(key, dest): + Path(dest).write_bytes(locked_zip.read_bytes()) + + sync_mocks.bucket.download_file.side_effect = fake_download + sync_mocks.load_content.return_value = [] + readable_id = sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + assert _canvas_run(readable_id).checksum diff --git a/learning_resources/etl/canvas_utils.py b/learning_resources/etl/canvas_utils.py index 5686bd3d3f..4132011053 100644 --- a/learning_resources/etl/canvas_utils.py +++ b/learning_resources/etl/canvas_utils.py @@ -1,9 +1,11 @@ import json import logging +import os import sys import zipfile from collections import defaultdict from datetime import UTC +from hashlib import md5 from pathlib import Path from urllib.parse import unquote, unquote_plus from zoneinfo import ZoneInfo @@ -706,3 +708,40 @@ def get_published_items(zipfile_path, url_config): published_items[embedded_path] = embedded return published_items + + +# Export-noise carriers: Canvas rewrites these on every no-op export +# (imsmanifest.xml embeds the export timestamp and reshuffles resources; +# course_settings/* change bytes AND size — verified across 13 prod course +# pairs). They are excluded from the byte digest; their semantic content +# (publish state, titles) is covered by the publish-set component below. +# Nothing under these paths is ever ingested as a content file (IGNORE_FILES). +CHECKSUM_EXCLUDED = ("imsmanifest.xml", "course_settings/") + + +def canvas_course_checksum(course_archive_path, url_config: dict) -> str: + """ + Digest of archive content + effective publish state + url metadata. + + Changes when a content member's bytes change (name/size/CRC, excluding + the export-noise carriers), when the published set or a display title + changes (publish toggles, timed lock/unlock boundaries passing), or when + the .metadata.json url config changes — every way a course's ingested + content can change. Stable across Canvas's no-op scheduled exports. + """ + hasher = md5() # noqa: S324 - non-cryptographic change detection + with zipfile.ZipFile(course_archive_path, "r") as course_archive: + for info in sorted(course_archive.infolist(), key=lambda i: i.filename): + if info.filename.startswith(CHECKSUM_EXCLUDED): + continue + hasher.update(f"{info.filename}\0{info.file_size}\0{info.CRC}\0".encode()) + published = get_published_items(course_archive_path, url_config) + # relpath: get_published_items keys are resolve()'d against the process + # cwd, which must not leak into the digest + for path, title in sorted( + (os.path.relpath(path), item.get("title") or "") + for path, item in published.items() + ): + hasher.update(f"{path}\0{title}\0".encode()) + hasher.update(json.dumps(url_config, sort_keys=True, default=str).encode()) + return hasher.hexdigest() diff --git a/learning_resources/etl/utils.py b/learning_resources/etl/utils.py index b32a8a4265..d06c1628ad 100644 --- a/learning_resources/etl/utils.py +++ b/learning_resources/etl/utils.py @@ -10,7 +10,6 @@ import re import tarfile import uuid -import zipfile from collections import Counter from collections.abc import Generator from datetime import UTC, datetime @@ -899,18 +898,13 @@ def get_bucket_by_name(bucket_name: str) -> object: def calc_checksum(filename) -> str: """ - Return the md5 checksum of the specified filepath + Return a checksum for the specified tar archive Args: - filename(str): The path to the file to checksum + filename(str): The path to the tar archive to checksum Returns: - str: The md5 checksum of the file + str: checksum derived from the archive members' header checksums """ - if zipfile.is_zipfile(filename): - with zipfile.ZipFile(filename, "r") as zip_file: - return str( - hash(tuple(f"{zp.filename}:{zp.file_size}" for zp in zip_file.filelist)) - ) with tarfile.open(filename, "r") as tgz_file: return str(hash(tuple(ti.chksum for ti in tgz_file.getmembers()))) From 1a571251d9a15e938448e854eda404ff6a2ee2b6 Mon Sep 17 00:00:00 2001 From: Zaman Afzal Date: Mon, 10 Aug 2026 18:01:46 +0500 Subject: [PATCH 09/12] feat(content_feedback): allow anonymous submissions (#3738) * feat(content_feedback): allow anonymous submissions --- content_feedback/views.py | 12 ++++++++---- content_feedback/views_test.py | 24 ++++++++++++++++++++---- openapi/specs/v0.yaml | 2 ++ 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/content_feedback/views.py b/content_feedback/views.py index f91f49fc8b..ff4c0708d8 100644 --- a/content_feedback/views.py +++ b/content_feedback/views.py @@ -2,7 +2,7 @@ from drf_spectacular.utils import OpenApiResponse, extend_schema from rest_framework.generics import CreateAPIView -from rest_framework.permissions import IsAuthenticated +from rest_framework.permissions import AllowAny from content_feedback.models import ContentFeedback from content_feedback.serializers import ContentFeedbackSerializer @@ -22,10 +22,14 @@ class ContentFeedbackView(CreateAPIView): queryset = ContentFeedback.objects.all() serializer_class = ContentFeedbackSerializer - permission_classes = (IsAuthenticated,) + # AllowAny: courseware-only learners have no mit-learn/APISIX session, so + # requiring auth would 403 nearly all of them. Authenticated rows record the + # user; anonymous rows store null (mirrors learn-ai/AskTIM). + permission_classes = (AllowAny,) throttle_classes = (RedisScopedRateThrottle,) throttle_scope = "content_feedback" def perform_create(self, serializer): - """Attribute the feedback to the authenticated user (server-side).""" - serializer.save(user=self.request.user) + """Attribute to the request user when authenticated, else store null.""" + user = self.request.user if self.request.user.is_authenticated else None + serializer.save(user=user) diff --git a/content_feedback/views_test.py b/content_feedback/views_test.py index c3b998f09c..a647a06166 100644 --- a/content_feedback/views_test.py +++ b/content_feedback/views_test.py @@ -2,6 +2,7 @@ import pytest from django.urls import reverse +from rest_framework.test import APIClient from content_feedback.factories import ContentFeedbackFactory from content_feedback.models import ContentFeedback @@ -26,10 +27,24 @@ def _payload(**overrides): return payload -def test_submit_requires_authentication(client): - """Anonymous users cannot submit feedback.""" +def test_submit_allows_anonymous(): + """Anonymous users can submit without a CSRF token; the record has no user.""" + # enforce_csrf_checks=True mirrors production SessionAuthentication: CSRF is + # only enforced for session-authenticated callers, so an anonymous POST with + # no token still succeeds. + client = APIClient(enforce_csrf_checks=True) response = client.post(reverse("content_feedback:v0:content_feedback"), _payload()) - assert response.status_code in (401, 403) + assert response.status_code == 201 + assert ContentFeedback.objects.count() == 1 + assert ContentFeedback.objects.get().user is None + + +def test_authenticated_without_csrf_token_rejected(user): + """A session-authenticated POST without a CSRF token is rejected (403).""" + client = APIClient(enforce_csrf_checks=True) + client.force_login(user) + response = client.post(reverse("content_feedback:v0:content_feedback"), _payload()) + assert response.status_code == 403 assert ContentFeedback.objects.count() == 0 @@ -119,7 +134,6 @@ def test_factory_builds_valid_record(): def test_submit_rate_limited(user_client, mocker): """Exceeding the per-user rate returns 429; a different user is unaffected.""" from django.core.cache.backends.locmem import LocMemCache - from rest_framework.test import APIClient from main.factories import UserFactory from main.throttles import RedisScopedRateThrottle @@ -141,6 +155,8 @@ def test_submit_rate_limited(user_client, mocker): # The limit is keyed per authenticated user: a different user still gets # through even after the first user is throttled. (The user_client fixture # shares one APIClient, so build a distinct client for the second user.) + # NB: anonymous requests instead key on client IP, which is spoofable via + # X-Forwarded-For -- tracked as a follow-up (mitodl/hq#12775). other_client = APIClient() other_client.force_login(UserFactory.create()) assert other_client.post(url, _payload()).status_code == 201 diff --git a/openapi/specs/v0.yaml b/openapi/specs/v0.yaml index dfba96cec4..384a27716b 100644 --- a/openapi/specs/v0.yaml +++ b/openapi/specs/v0.yaml @@ -147,6 +147,8 @@ paths: schema: $ref: '#/components/schemas/ContentFeedbackRequest' required: true + security: + - {} responses: '201': content: From 3900e4a8e514791ade6a0a6dc236503ee0c8406f Mon Sep 17 00:00:00 2001 From: Danielle Frappier Date: Mon, 10 Aug 2026 09:59:43 -0400 Subject: [PATCH 10/12] Fix: render podcast show descriptions as HTML instead of raw markup (#3721) --- .../PodcastPage/PodcastDetailPage.test.tsx | 40 +++++++++++ .../PodcastPage/PodcastDetailPage.tsx | 66 ++++++++++++++----- .../PodcastSection.test.tsx | 41 ++++++++++++ .../PodcastsListingPage/PodcastSection.tsx | 30 +++++++-- 4 files changed, 155 insertions(+), 22 deletions(-) diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.test.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.test.tsx index cce80ce425..2a43638a36 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.test.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.test.tsx @@ -45,12 +45,15 @@ const makePodcastEpisodes = (count: number): PodcastEpisodeResource[] => const setupApis = ({ episodesPage1, episodesPage2, + podcastOverrides = {}, }: { episodesPage1: LearningResource[] episodesPage2?: LearningResource[] + podcastOverrides?: Partial }) => { const podcast = factories.learningResources.resource({ resource_type: ResourceTypeEnum.Podcast, + ...podcastOverrides, }) // Episodes of this podcast reference it as their parent, as they would in @@ -202,6 +205,43 @@ describe("PodcastDetailPage", () => { await screen.findByText(episodes[0].title!) }) + test("renders a formatted show description", async () => { + const episodes = makePodcastEpisodes(1) + const { podcast } = setupApis({ + episodesPage1: episodes, + podcastOverrides: { + description: "

Daryl Morey & Jessica Gelman

", + }, + }) + + renderWithProviders() + + expect( + await screen.findByText("Daryl Morey & Jessica Gelman"), + ).toBeInTheDocument() + }) + + test("opens external links in the show description in a new tab", async () => { + const episodes = makePodcastEpisodes(1) + const { podcast } = setupApis({ + episodesPage1: episodes, + podcastOverrides: { + // rel="noopener noreferrer" mirrors real backend output: nh3 adds it + // to every during ETL sanitization, regardless of destination. + description: + 'Relevant Resources: OCW and Search.', + }, + }) + + renderWithProviders() + + const externalLink = await screen.findByRole("link", { name: "OCW" }) + expect(externalLink).toHaveAttribute("target", "_blank") + + const internalLink = screen.getByRole("link", { name: "Search" }) + expect(internalLink).not.toHaveAttribute("target") + }) + test("shows an error when the podcast fails to load", async () => { const podcast = factories.learningResources.resource({ resource_type: ResourceTypeEnum.Podcast, diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx index edc1606ace..d4d0e9c2ee 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx @@ -1,7 +1,7 @@ "use client" -import React from "react" -import { Typography, Skeleton, styled } from "ol-components" +import React, { useMemo } from "react" +import { Typography, Skeleton, styled, TypographyProps } from "ol-components" import { Button } from "@mitodl/smoot-design" import { RiPlayFill, RiPauseFill } from "@remixicon/react" import { @@ -12,6 +12,7 @@ import { ResourceTypeEnum } from "api/v1" import type { LearningResource } from "api/v1" import { formatDate } from "ol-utilities" import { HOME, podcastEpisodePageView } from "@/common/urls" +import { addExternalLinkTargets } from "@/common/utils" import PodcastContainer from "./PodcastContainer" import PodcastBreadcrumbs from "./PodcastBreadcrumbs" import { usePodcastPage } from "./usePodcastPage" @@ -66,18 +67,28 @@ const MetaLine = styled(Typography)(({ theme }) => ({ }, })) -const Description = styled(Typography)(({ theme }) => ({ - color: theme.custom.colors.darkGray2, - display: "block", - marginBottom: "16px", - ...theme.typography.body1, - lineHeight: "26px", - [theme.breakpoints.down("sm")]: { - marginBottom: "8px", - ...theme.typography.body2, - lineHeight: "22px", - }, -})) +const Description = styled(Typography)>( + ({ theme }) => ({ + color: theme.custom.colors.darkGray2, + display: "block", + marginBottom: "16px", + ...theme.typography.body1, + lineHeight: "26px", + a: { + textDecoration: "underline", + color: theme.custom.colors.darkGray2, + fontWeight: theme.typography.fontWeightMedium, + }, + "a:hover": { + textDecoration: "none", + }, + [theme.breakpoints.down("sm")]: { + marginBottom: "8px", + ...theme.typography.body2, + lineHeight: "22px", + }, + }), +) const LatestEpisodeLine = styled(Typography)(({ theme }) => ({ color: theme.custom.colors.silverGrayDark, @@ -274,6 +285,21 @@ export const PodcastDetailPage: React.FC = ({ const handlePlayClick = (episode: LearningResource) => toggle(episode, id) + // Podcast descriptions are sanitized on the backend with nh3 during ETL + // (only is allowed), so the HTML is safe to render verbatim + // — the same trust model as podcast episode descriptions. Rendering it + // directly keeps server and client output identical, avoiding a hydration + // mismatch; target="_blank" is added via addExternalLinkTargets so it's + // part of the HTML fed to dangerouslySetInnerHTML on both server and + // client, keeping SSR output byte-identical to the client's first render. + const description = useMemo( + () => + resource?.description + ? addExternalLinkTargets(resource.description) + : null, + [resource?.description], + ) + return ( <> @@ -315,10 +341,14 @@ export const PodcastDetailPage: React.FC = ({ )} - {resource?.description && ( - - {resource.description} - + {description && ( + )} {latestEpisode && ( diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.test.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.test.tsx index f67ae94f00..51317c7f6f 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.test.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.test.tsx @@ -71,6 +71,47 @@ describe("PodcastSection", () => { ) }) + it("renders a sanitized, formatted summary for featured series", () => { + const series = makeSeries({ + description: + '

Teaching & learning at MIT

', + }) + renderWithProviders( + , + ) + expect(screen.getByText("Teaching & learning at MIT")).toBeInTheDocument() + expect(document.querySelector("script")).not.toBeInTheDocument() + }) + + it("strips links from the featured summary, keeping their text, since the card is itself a link", () => { + const series = makeSeries({ + description: + 'Relevant Resources: OCW and Search.', + }) + renderWithProviders( + , + ) + expect( + screen.getByText("Relevant Resources: OCW and Search.", { + exact: false, + }), + ).toBeInTheDocument() + // Only the card's own outer link should be present — no nested . + expect(screen.getAllByRole("link", { name: /Chalk Radio/ })).toHaveLength(1) + }) + it("renders 'More Podcasts' rows with title and offered_by", () => { const series = makeSeries({ title: "The Aggregate", diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.tsx index 2cbbe0eae1..c5c3d5e4a9 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.tsx @@ -4,9 +4,11 @@ import { Typography, Skeleton, styled } from "ol-components" import type { TypographyProps } from "ol-components" import { ButtonLink } from "@mitodl/smoot-design" import { RiArrowRightLine, RiArrowRightSLine } from "@remixicon/react" +import DOMPurify from "isomorphic-dompurify" import { formatDate } from "ol-utilities" import type { LearningResource } from "api/v1" import { SEARCH_PODCASTS, podcastPageView } from "@/common/urls" +import { stripAnchorTags } from "@/common/utils" import { Section, SectionHeader, @@ -109,7 +111,9 @@ const FeaturedPodcastTitle = styled(Typography)< marginBottom: "8px", })) -const FeaturedPodcastSummary = styled(Typography)(({ theme }) => ({ +const FeaturedPodcastSummary = styled(Typography)< + Pick +>(({ theme }) => ({ color: theme.custom.colors.silverGrayDark, lineHeight: "24px", marginBottom: "16px", @@ -117,6 +121,18 @@ const FeaturedPodcastSummary = styled(Typography)(({ theme }) => ({ WebkitBoxOrient: "vertical", WebkitLineClamp: 2, overflow: "hidden", + maxWidth: "100%", + overflowWrap: "break-word", + wordBreak: "break-word", + "& p": { + display: "inline", + margin: 0, + maxWidth: "100%", + overflowWrap: "break-word", + wordBreak: "break-word", + whiteSpace: "normal", + textWrap: "wrap", + }, })) const FeaturedPodcastMeta = styled(Typography)(({ theme }) => ({ @@ -324,9 +340,15 @@ const PodcastSection: React.FC = ({ {item.title} {item.description && ( - - {item.description} - + )} {[ From 4ddedb9e71f9a12863019b4c676db0376b0e97ab Mon Sep 17 00:00:00 2001 From: Shankar Ambady Date: Mon, 10 Aug 2026 13:09:55 -0400 Subject: [PATCH 11/12] fix broken images in subscription emails (#3737) * adding check for image reachability * use fallback image url * adding tests and celery task for cleaning resources with bad images * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Potential fix for pull request finding 'CodeQL / Full server-side request forgery' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * Revert "Potential fix for pull request finding 'CodeQL / Full server-side request forgery'" This reverts commit ac761b9fc0e03c3bdb3f797688a8d5ce8cdb86f6. * more efficient processing for image pruning task * follow through for 301 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * removing all the overkill (pruning tasks etc) features. keeping simple check during percolate/email send process * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- learning_resources_search/tasks.py | 50 ++++++++++++++- learning_resources_search/tasks_test.py | 85 +++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 3 deletions(-) diff --git a/learning_resources_search/tasks.py b/learning_resources_search/tasks.py index 21086c922a..fa9cd2a2c2 100644 --- a/learning_resources_search/tasks.py +++ b/learning_resources_search/tasks.py @@ -5,11 +5,13 @@ import logging from collections import OrderedDict from contextlib import contextmanager +from http import HTTPStatus from itertools import groupby from random import random from urllib.parse import urlencode import celery +import requests from celery.exceptions import Ignore from django.conf import settings from django.contrib.auth import get_user_model @@ -68,6 +70,9 @@ User = get_user_model() log = logging.getLogger(__name__) +# Timeout for the digest email's image liveness check +IMAGE_CHECK_TIMEOUT_SECONDS = 5 + # For our tasks that attempt to partially update a document, there's a chance that # the document has not yet been created. When we get an error that indicates that the @@ -220,6 +225,46 @@ def key_func(x): return grouped_data +def _image_url_is_reachable(url): + """ + Check whether an image URL responds successfully. + + Uses a short timeout: this runs while building a digest email, so a slow + or hanging image host should not hold up the send. + """ + try: + response = requests.head( + url, timeout=IMAGE_CHECK_TIMEOUT_SECONDS, allow_redirects=True + ) + if response.status_code in ( + HTTPStatus.METHOD_NOT_ALLOWED, + HTTPStatus.NOT_IMPLEMENTED, + ): + # some servers reject HEAD; retry without downloading the body + response = requests.get( + url, timeout=IMAGE_CHECK_TIMEOUT_SECONDS, stream=True + ) + response.close() + except requests.RequestException: + return False + return HTTPStatus.OK <= response.status_code < HTTPStatus.MULTIPLE_CHOICES + + +def _validated_resource_image_url(resource): + """ + Return the resource's image URL if it is reachable, otherwise the default + resource image. Email clients can't fall back on their own, so a dead URL + would render as a broken image icon. + """ + if ( + resource.image + and resource.image.url + and _image_url_is_reachable(resource.image.url) + ): + return resource.image.url + return frontend_absolute_url("/images/default_resource.jpg") + + def _get_percolated_rows(resources, subscription_type): """ Get percolated rows for a list of learning resources and subscription type @@ -232,6 +277,7 @@ def _get_percolated_rows(resources, subscription_type): source_type=subscription_type ) if percolated.count() > 0: + resource_image_url = _validated_resource_image_url(resource) percolated_users = set(percolated.values_list("users", flat=True)) all_users.update(percolated_users) for user in percolated_users: @@ -251,9 +297,7 @@ def _get_percolated_rows(resources, subscription_type): { "resource_url": resource_url, "resource_title": resource.title, - "resource_image_url": resource.image.url - if resource.image - else frontend_absolute_url("/images/default_resource.jpg"), + "resource_image_url": resource_image_url, "resource_type": LearningResourceType[ resource.resource_type ].value, diff --git a/learning_resources_search/tasks_test.py b/learning_resources_search/tasks_test.py index 654c7672c7..b63575de7a 100644 --- a/learning_resources_search/tasks_test.py +++ b/learning_resources_search/tasks_test.py @@ -3,6 +3,8 @@ from collections import OrderedDict import pytest +import requests +import responses from celery.exceptions import Ignore, Retry from django.conf import settings from django.contrib.auth import get_user_model @@ -44,8 +46,10 @@ _generate_subscription_digest_subject, _get_percolated_rows, _group_percolated_rows, + _image_url_is_reachable, _infer_percolate_group, _maybe_finish_reindex_job, + _validated_resource_image_url, bulk_deindex_learning_resources, deindex_document, deindex_run_content_files, @@ -64,6 +68,7 @@ from main.factories import TaskBatchFactory, TaskJobFactory, UserFactory from main.models import TaskBatch, TaskJob from main.test_utils import assert_not_raises +from main.utils import frontend_absolute_url pytestmark = pytest.mark.django_db User = get_user_model() @@ -86,6 +91,19 @@ def mocked_api(mocker): return mocker.patch("learning_resources_search.tasks.api") +@pytest.fixture(autouse=True) +def mock_image_url_is_reachable(mocker, request): + """ + Stub the digest email's image reachability check so tests don't make real + requests. Tests that use mocked_responses exercise the real function. + """ + if "mocked_responses" in request.fixturenames: + return None + return mocker.patch( + "learning_resources_search.tasks._image_url_is_reachable", return_value=True + ) + + def test_upsert_learning_resource(mocked_api): """Test that upsert_learning_resourc will serialize the learning resource data and upsert it to the OS index""" resource = LearningResourceFactory.create() @@ -1724,6 +1742,73 @@ def get_percolator(res): assert topic in template_data +@pytest.mark.parametrize( + ("status", "expected"), + [(200, True), (404, False), (403, False), (500, False)], +) +def test_image_url_is_reachable(mocked_responses, status, expected): + """_image_url_is_reachable should be True only for successful responses""" + url = "http://example.com/image.jpg" + mocked_responses.add(responses.HEAD, url, status=status) + assert _image_url_is_reachable(url) is expected + + +@pytest.mark.parametrize(("final_status", "expected"), [(200, True), (404, False)]) +def test_image_url_is_reachable_follows_redirect( + mocked_responses, final_status, expected +): + """A redirect is followed through to the destination's status""" + url = "http://example.com/image.jpg" + redirected_to = "http://example.com/moved.jpg" + mocked_responses.add( + responses.HEAD, url, status=301, headers={"Location": redirected_to} + ) + mocked_responses.add(responses.HEAD, redirected_to, status=final_status) + assert _image_url_is_reachable(url) is expected + + +def test_image_url_is_reachable_head_not_allowed(mocked_responses): + """Servers that reject HEAD should be retried with GET""" + url = "http://example.com/image.jpg" + mocked_responses.add(responses.HEAD, url, status=405) + mocked_responses.add(responses.GET, url, status=200) + assert _image_url_is_reachable(url) is True + + +def test_image_url_is_reachable_connection_error(mocked_responses): + """A request that errors out counts as unreachable""" + url = "http://example.com/image.jpg" + mocked_responses.add( + responses.HEAD, url, body=requests.exceptions.ConnectionError() + ) + assert _image_url_is_reachable(url) is False + + +@pytest.mark.parametrize("reachable", [True, False]) +def test_validated_resource_image_url(mock_image_url_is_reachable, reachable): + """ + The digest email should use the resource image only if its URL is + reachable, and the default image otherwise + """ + mock_image_url_is_reachable.return_value = reachable + resource = LearningResourceFactory.create(is_course=True) + validated_url = _validated_resource_image_url(resource) + if reachable: + assert validated_url == resource.image.url + else: + assert validated_url == frontend_absolute_url("/images/default_resource.jpg") + mock_image_url_is_reachable.assert_called_once_with(resource.image.url) + + +def test_validated_resource_image_url_no_image(mock_image_url_is_reachable): + """The digest email should use the default image if the resource has none""" + resource = LearningResourceFactory.create(is_course=True, no_image=True) + assert _validated_resource_image_url(resource) == frontend_absolute_url( + "/images/default_resource.jpg" + ) + mock_image_url_is_reachable.assert_not_called() + + def test_subscription_digest_subject(): """ Test that email generates a dynamic subject based From d7e3380c424f6ed35ed7f5e72426dd1a06b4460e Mon Sep 17 00:00:00 2001 From: Doof Date: Mon, 10 Aug 2026 18:17:24 +0000 Subject: [PATCH 12/12] Release 0.77.3 --- RELEASE.rst | 15 +++++++++++++++ main/settings.py | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/RELEASE.rst b/RELEASE.rst index 20d5e75bd0..4c9a90e832 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,21 @@ Release Notes ============= +Version 0.77.3 +-------------- + +- fix broken images in subscription emails (#3737) +- Fix: render podcast show descriptions as HTML instead of raw markup (#3721) +- feat(content_feedback): allow anonymous submissions (#3738) +- Fix Canvas archive change detection: deterministic checksum, save after load (#3728) +- Skip unchanged edX course archives before downloading from S3 (#3722) +- remove GITHUB_ACCESS_TOKEN (#3730) +- Fix: HTML leaking into meta description tags (#3727) +- Make recreate index resilient to pod culling (#3716) +- Fix PostHog view-event ETL crash from duplicate view events (#3714) +- Use CMS Certificate Title for program LinkedIn "Add to Profile" (#3518) +- Harden GH Actions supply chain: add zizmor static analysis + 7-day dependency cool-down (#3712) + Version 0.77.2 (Released August 10, 2026) -------------- diff --git a/main/settings.py b/main/settings.py index 2d16eab3e6..42db269aa2 100644 --- a/main/settings.py +++ b/main/settings.py @@ -36,7 +36,7 @@ from main.settings_pluggy import * # noqa: F403 from openapi.settings_spectacular import open_spectacular_settings -VERSION = "0.77.2" +VERSION = "0.77.3" log = logging.getLogger()