From 9615f19c43666d4c2f799cb0c380f04806cd6f7d Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 10 Aug 2026 16:24:27 -0400 Subject: [PATCH 01/12] Fall back to tika when OCR extraction raises --- learning_resources/etl/utils.py | 8 +++++++- learning_resources/etl/utils_test.py | 26 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/learning_resources/etl/utils.py b/learning_resources/etl/utils.py index d06c1628ad..a02d2ffa90 100644 --- a/learning_resources/etl/utils.py +++ b/learning_resources/etl/utils.py @@ -653,7 +653,13 @@ def _extract_content( # noqa: PLR0913 if _should_use_ocr( file_extension=file_extension, file_path=file_path, use_ocr=use_ocr ): - content_dict = _extract_content_with_ocr(file_path, is_tutor_problem) + try: + content_dict = _extract_content_with_ocr(file_path, is_tutor_problem) + except Exception: + log.exception( + "OCR extraction failed for %s, falling back to tika", file_path + ) + content_dict = None if content_dict: return content_dict diff --git a/learning_resources/etl/utils_test.py b/learning_resources/etl/utils_test.py index 58704fd55d..9eca11cb7e 100644 --- a/learning_resources/etl/utils_test.py +++ b/learning_resources/etl/utils_test.py @@ -1150,3 +1150,29 @@ def test_extract_content_ocr_fallback_to_tika(mocker, settings, tmp_path): ) assert result == {"content": "tika content", "content_title": "Tika Title"} + + +def test_extract_content_ocr_failure_falls_back_to_tika(mocker, settings, tmp_path): + """An OCR crash (e.g. missing converter output JSON) should fall through to tika""" + settings.SKIP_TIKA = False + mocker.patch("learning_resources.etl.utils._should_use_ocr", return_value=True) + mocker.patch( + "learning_resources.etl.utils._extract_content_with_ocr", + side_effect=FileNotFoundError("no converter output json"), + ) + mocker.patch( + "learning_resources.etl.utils.extract_text_metadata", + return_value={"content": "tika text", "metadata": {"title": "doc title"}}, + ) + result = utils._extract_content( # noqa: SLF001 + b"pdf bytes", + { + "source_path": "web_resources/7.06_Fall2025_Exam1_answers.pdf", + "file_extension": ".pdf", + "mime_type": "application/pdf", + }, + str(tmp_path), + "test-key", + use_ocr=True, + ) + assert result["content"] == "tika text" From a63be45436ee560def1a877323ea2e9859dc0f06 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 10 Aug 2026 16:30:40 -0400 Subject: [PATCH 02/12] Skip and record files whose extraction raises in process_olx_path Co-Authored-By: Claude Fable 5 --- learning_resources/etl/utils.py | 45 +++++++++++++++++----------- learning_resources/etl/utils_test.py | 30 +++++++++++++++++++ 2 files changed, 58 insertions(+), 17 deletions(-) diff --git a/learning_resources/etl/utils.py b/learning_resources/etl/utils.py index a02d2ffa90..a5213ee356 100644 --- a/learning_resources/etl/utils.py +++ b/learning_resources/etl/utils.py @@ -804,6 +804,7 @@ def process_olx_path( # noqa: PLR0913 valid_file_types=VALID_TEXT_FILE_TYPES, is_tutor_problem_file_import=False, use_ocr=False, + failed_source_paths: list | None = None, ) -> Generator[dict, None, None]: """Process OLX path and yield content dictionaries.""" video_srt_metadata = get_video_metadata(olx_path, run) @@ -814,25 +815,35 @@ def process_olx_path( # noqa: PLR0913 source_path = metadata.get("source_path") key = get_edx_module_id(source_path, run) - existing_record = _get_existing_record( - source_path, key, run, is_tutor_problem_file_import - ) - - if _should_reprocess(existing_record, metadata, overwrite): - content_dict = _extract_content( - document, - metadata, - olx_path, - key, - use_ocr=use_ocr, - is_tutor_problem=is_tutor_problem_file_import, + try: + existing_record = _get_existing_record( + source_path, key, run, is_tutor_problem_file_import ) - if content_dict is None: - continue - else: - content_dict = _get_cached_content( - existing_record, is_tutor_problem_file_import + + if _should_reprocess(existing_record, metadata, overwrite): + content_dict = _extract_content( + document, + metadata, + olx_path, + key, + use_ocr=use_ocr, + is_tutor_problem=is_tutor_problem_file_import, + ) + if content_dict is None: + continue + else: + content_dict = _get_cached_content( + existing_record, is_tutor_problem_file_import + ) + except Exception: + log.exception( + "Extraction failed for %s in run %s, skipping file", + source_path, + run.id, ) + if failed_source_paths is not None: + failed_source_paths.append(source_path) + continue yield _build_result( olx_path, metadata, key, run, video_srt_metadata, content_dict diff --git a/learning_resources/etl/utils_test.py b/learning_resources/etl/utils_test.py index 9eca11cb7e..1304da9aea 100644 --- a/learning_resources/etl/utils_test.py +++ b/learning_resources/etl/utils_test.py @@ -1176,3 +1176,33 @@ def test_extract_content_ocr_failure_falls_back_to_tika(mocker, settings, tmp_pa use_ocr=True, ) assert result["content"] == "tika text" + + +@pytest.mark.django_db +def test_process_olx_path_skips_failed_files(mocker, tmp_path): + """A file whose extraction raises is skipped and recorded; other files still yield""" + run = LearningResourceRunFactory.create() + static_dir = tmp_path / "static" + static_dir.mkdir() + (static_dir / "good.html").write_text("

good

") + (static_dir / "bad.html").write_text("

bad

") + + def fake_extract(document, metadata, olx_path, key, **kwargs): + if "bad.html" in metadata["source_path"]: + msg = "converter output missing" + raise FileNotFoundError(msg) + return {"content": "text", "content_title": ""} + + mocker.patch( + "learning_resources.etl.utils._extract_content", side_effect=fake_extract + ) + failed = [] + results = list( + utils.process_olx_path( + str(tmp_path), run, overwrite=True, failed_source_paths=failed + ) + ) + assert len(results) == 1 + assert "good.html" in results[0]["source_path"] + assert len(failed) == 1 + assert "bad.html" in failed[0] From 4baf07706cb71af9b06d593a607009c08a724ddb Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 10 Aug 2026 16:38:44 -0400 Subject: [PATCH 03/12] Retain contentfiles for files whose extraction failed during canvas sync --- learning_resources/etl/canvas.py | 12 +++- learning_resources/etl/canvas_test.py | 87 ++++++++++++++++++++++++++- 2 files changed, 97 insertions(+), 2 deletions(-) diff --git a/learning_resources/etl/canvas.py b/learning_resources/etl/canvas.py index 4a71c35d7e..306ed67260 100644 --- a/learning_resources/etl/canvas.py +++ b/learning_resources/etl/canvas.py @@ -151,6 +151,8 @@ def transform_canvas_content_files( zipfile_path = course_zipfile.absolute() published_items = get_published_items(zipfile_path, url_config) + failed_source_paths = [] + def _generate_content(): """Inner generator for yielding content data""" with ( @@ -166,7 +168,11 @@ def _generate_content(): log.debug("skipping unpublished file %s", member.filename) for content_data in process_olx_path( - olx_path, run, overwrite=overwrite, use_ocr=True + olx_path, + run, + overwrite=overwrite, + use_ocr=True, + failed_source_paths=failed_source_paths, ): url_path = content_data["source_path"].lstrip( content_data["source_path"].split("/")[0] @@ -189,6 +195,10 @@ def _generate_content(): full_path = Path(basedir) / Path(content_data["source_path"]) published_keys.append(get_edx_module_id(str(full_path), run)) yield content_data + # files whose extraction failed are retained, not treated as unpublished + for source_path in failed_source_paths: + full_path = Path(basedir) / Path(source_path) + published_keys.append(get_edx_module_id(str(full_path), run)) unpublished_content = run.content_files.exclude(key__in=published_keys) # remove unpublished contentfiles bulk_resources_unpublished_actions( diff --git a/learning_resources/etl/canvas_test.py b/learning_resources/etl/canvas_test.py index e182d6febd..cff4b15d88 100644 --- a/learning_resources/etl/canvas_test.py +++ b/learning_resources/etl/canvas_test.py @@ -37,7 +37,7 @@ LearningResourceRunFactory, TutorProblemFileFactory, ) -from learning_resources.models import LearningResource +from learning_resources.models import ContentFile, LearningResource from learning_resources_search.constants import CONTENT_FILE_TYPE from main.utils import now_in_utc @@ -452,6 +452,91 @@ def test_transform_canvas_content_files_removes_unpublished_content(mocker, tmp_ bulk_unpub.assert_called_once_with([unpublished_cf.id], CONTENT_FILE_TYPE) +def test_transform_canvas_content_files_retains_failed_files(mocker, tmp_path): + """A file whose extraction raises must not be deleted; true orphans still are""" + resource = LearningResourceFactory.create(etl_source=ETLSource.canvas.name) + run = LearningResourceRunFactory.create(learning_resource=resource) + + published_path = "/test/published/file1.html" + unpublished_path = "/test/unpublished/file2.html" + failing_cf = ContentFileFactory.create( + run=run, published=True, key=get_edx_module_id(published_path, run) + ) + unpublished_cf = ContentFileFactory.create( + run=run, published=True, key=get_edx_module_id(unpublished_path, run) + ) + module_xml = b""" + + + Module 1 + + + active + Item 1 + RES1 + resource + + + unpublished + Item 2 + RES2 + resource + + + + + """ + manifest_xml = bytes( + f""" + + + + + + + + + + + + + Item 1 + + + Item 2 + + + + + """, + "utf-8", + ) + zip_path = tmp_path / "canvas_course.zip" + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr("course_settings/module_meta.xml", module_xml) + zf.writestr("imsmanifest.xml", manifest_xml) + zf.writestr(published_path, "content") + zf.writestr(unpublished_path, "content") + + mocker.patch( + "learning_resources.etl.utils._extract_content", + side_effect=FileNotFoundError("ocr output missing"), + ) + bulk_unpub = mocker.patch( + "learning_resources.etl.canvas.bulk_resources_unpublished_actions" + ) + + list( + transform_canvas_content_files( + Path(zip_path), run, url_config={}, overwrite=True + ) + ) + + assert ContentFile.objects.filter(id=failing_cf.id).exists() + assert not ContentFile.objects.filter(id=unpublished_cf.id).exists() + bulk_unpub.assert_called_once_with([unpublished_cf.id], CONTENT_FILE_TYPE) + + @pytest.mark.parametrize("overwrite", [True, False]) @pytest.mark.parametrize("existing_file", [True, False]) def test_transform_canvas_problem_files_pdf_calls_pdf_to_markdown( # noqa: PLR0913 From 4f4bfad2571bddc0938254aea6ce5f9a6f1a8c4e Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 10 Aug 2026 16:57:06 -0400 Subject: [PATCH 04/12] Retain tutor problem files whose extraction failed --- learning_resources/etl/canvas.py | 19 ++++++++++++++++--- learning_resources/etl/loaders.py | 14 +++++++++----- learning_resources/etl/loaders_test.py | 16 ++++++++++++++++ 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/learning_resources/etl/canvas.py b/learning_resources/etl/canvas.py index 306ed67260..dd3c164427 100644 --- a/learning_resources/etl/canvas.py +++ b/learning_resources/etl/canvas.py @@ -70,12 +70,20 @@ def sync_canvas_archive(bucket, key: str, overwrite): canvas_content_files, ) + failed_problem_paths = [] canvas_problem_files = list( transform_canvas_problem_files( - course_archive_path, run, overwrite=overwrite + course_archive_path, + run, + overwrite=overwrite, + failed_source_paths=failed_problem_paths, ) ) - problem_files_ids = load_problem_files(run, canvas_problem_files) + problem_files_ids = load_problem_files( + run, + canvas_problem_files, + failed_source_paths=failed_problem_paths, + ) 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 @@ -208,7 +216,11 @@ def _generate_content(): def transform_canvas_problem_files( - course_zipfile: Path, run: LearningResourceRun, *, overwrite + course_zipfile: Path, + run: LearningResourceRun, + *, + overwrite, + failed_source_paths: list | None = None, ) -> Generator[dict, None, None]: """ Transform problem files from a Canvas course zipfile @@ -229,6 +241,7 @@ def transform_canvas_problem_files( valid_file_types=VALID_TUTOR_PROBLEM_FILE_TYPES, is_tutor_problem_file_import=True, use_ocr=True, + failed_source_paths=failed_source_paths, ): keys_to_keep = [ "run", diff --git a/learning_resources/etl/loaders.py b/learning_resources/etl/loaders.py index 0460777b29..6bd815ec40 100644 --- a/learning_resources/etl/loaders.py +++ b/learning_resources/etl/loaders.py @@ -1198,6 +1198,7 @@ def load_problem_file( def load_problem_files( course_run: LearningResourceRun, problem_files_data: list[dict], + failed_source_paths: list | None = None, ) -> list[int]: """ Sync all problem files for canvas course @@ -1205,6 +1206,8 @@ def load_problem_files( Args: course_run (LearningResourceRun): a course run problem_files_data (list or generator): Details about the problem files + failed_source_paths (list): source paths whose extraction failed; these + are retained rather than deleted as orphans Returns: list of int: Ids of the TutorProblemFile objects that were created/updated @@ -1214,11 +1217,12 @@ def load_problem_files( load_problem_file(course_run, problem_file) for problem_file in problem_files_data ] - for file in ( - TutorProblemFile.objects.filter(run=course_run) - .exclude(id__in=problem_files_ids) - .all() - ): + deletable = TutorProblemFile.objects.filter(run=course_run).exclude( + id__in=problem_files_ids + ) + if failed_source_paths: + deletable = deletable.exclude(source_path__in=failed_source_paths) + for file in deletable.all(): file.delete() return problem_files_ids diff --git a/learning_resources/etl/loaders_test.py b/learning_resources/etl/loaders_test.py index f6e254c280..cc2cf2d30f 100644 --- a/learning_resources/etl/loaders_test.py +++ b/learning_resources/etl/loaders_test.py @@ -2140,6 +2140,22 @@ def test_load_problem_files(mocker): ).exists() +def test_load_problem_files_retains_failed_source_paths(): + """Problem files whose extraction failed are not deleted as orphans""" + run = LearningResourceRunFactory.create() + failed = TutorProblemFileFactory.create( + run=run, source_path="web_resources/ai/tutor/p1/failed.pdf" + ) + orphan = TutorProblemFileFactory.create( + run=run, source_path="web_resources/ai/tutor/p2/orphan.pdf" + ) + + load_problem_files(run, [], failed_source_paths=[failed.source_path]) + + assert TutorProblemFile.objects.filter(id=failed.id).exists() + assert not TutorProblemFile.objects.filter(id=orphan.id).exists() + + def test_load_image(): """Test that image resources are uniquely created or retrieved based on parameters""" resource_url = "https://mit.edu" From 358c44eccd2de12876965826069a4fc4fa3e0507 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 10 Aug 2026 17:32:38 -0400 Subject: [PATCH 05/12] Keep failed-extraction contentfiles published through the staleness pass --- learning_resources/etl/canvas.py | 26 +++++++++++++++-- learning_resources/etl/loaders.py | 5 ++++ learning_resources/etl/loaders_test.py | 40 ++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 3 deletions(-) diff --git a/learning_resources/etl/canvas.py b/learning_resources/etl/canvas.py index dd3c164427..f24fc30bb9 100644 --- a/learning_resources/etl/canvas.py +++ b/learning_resources/etl/canvas.py @@ -60,14 +60,20 @@ def sync_canvas_archive(bucket, key: str, overwrite): overwrite=overwrite, ) if run: + failed_content_keys = [] canvas_content_files = list( transform_canvas_content_files( - course_archive_path, run, url_config=url_config, overwrite=overwrite + course_archive_path, + run, + url_config=url_config, + overwrite=overwrite, + failed_keys=failed_content_keys, ) ) content_files_ids = load_content_files( run, canvas_content_files, + failed_keys=failed_content_keys, ) failed_problem_paths = [] @@ -150,10 +156,18 @@ def run_for_canvas_archive(course_archive_path, course_folder, checksum, overwri def transform_canvas_content_files( - course_zipfile: Path, run: LearningResourceRun, url_config: dict, *, overwrite + course_zipfile: Path, + run: LearningResourceRun, + url_config: dict, + *, + overwrite, + failed_keys: list | None = None, ) -> Generator[dict, None, None]: """ Transform published content files from a Canvas course zipfile + + Files whose extraction fails are skipped and their existing records + are retained (not deleted/unpublished). """ basedir = course_zipfile.name.split(".")[0] zipfile_path = course_zipfile.absolute() @@ -206,7 +220,10 @@ def _generate_content(): # files whose extraction failed are retained, not treated as unpublished for source_path in failed_source_paths: full_path = Path(basedir) / Path(source_path) - published_keys.append(get_edx_module_id(str(full_path), run)) + failed_key = get_edx_module_id(str(full_path), run) + published_keys.append(failed_key) + if failed_keys is not None: + failed_keys.append(failed_key) unpublished_content = run.content_files.exclude(key__in=published_keys) # remove unpublished contentfiles bulk_resources_unpublished_actions( @@ -224,6 +241,9 @@ def transform_canvas_problem_files( ) -> Generator[dict, None, None]: """ Transform problem files from a Canvas course zipfile + + Files whose extraction fails are skipped and their existing records + are retained (not deleted/unpublished). """ basedir = course_zipfile.name.split(".")[0] with ( diff --git a/learning_resources/etl/loaders.py b/learning_resources/etl/loaders.py index 6bd815ec40..cac9297328 100644 --- a/learning_resources/etl/loaders.py +++ b/learning_resources/etl/loaders.py @@ -1014,6 +1014,7 @@ def load_content_files( content_files_data: list[dict], *, calc_completeness: bool = False, + failed_keys: list | None = None, ) -> list[int]: """ Sync all content files for a course run to database and S3 if not present in DB @@ -1022,6 +1023,8 @@ def load_content_files( course_run (LearningResourceRun): a course run content_files_data (list or generator): Details about the content files calc_completeness: bool: Whether to calculate the completeness score + failed_keys: list: Keys of content files whose extraction failed and + should be exempted from the stale/unpublish pass Returns: list of int: Ids of the ContentFile objects that were created/updated @@ -1047,6 +1050,8 @@ def load_content_files( stale_published_files = ContentFile.objects.filter( run=course_run, published=True ).exclude(id__in=content_files_ids) + if failed_keys: + stale_published_files = stale_published_files.exclude(key__in=failed_keys) stale_direct_resource_ids = list( stale_published_files.filter(direct_learning_resource__isnull=False) .values_list("direct_learning_resource_id", flat=True) diff --git a/learning_resources/etl/loaders_test.py b/learning_resources/etl/loaders_test.py index cc2cf2d30f..caaa4bee71 100644 --- a/learning_resources/etl/loaders_test.py +++ b/learning_resources/etl/loaders_test.py @@ -1993,6 +1993,46 @@ def test_load_content_files_does_not_update_already_unpublished_stale_files(mock assert stale_unpublished_file.updated_on == old_timestamp +def test_load_content_files_failed_keys_stay_published(mocker): + """Contentfiles whose extraction failed are not unpublished as stale""" + course = LearningResourceFactory.create(is_course=True, create_runs=False) + course_run = LearningResourceRunFactory.create( + published=True, learning_resource=course + ) + loaded_cf = ContentFileFactory.create( + run=course_run, published=True, key="loaded-key" + ) + failed_cf = ContentFileFactory.create( + run=course_run, published=True, key="failed-key" + ) + stale_cf = ContentFileFactory.create( + run=course_run, published=True, key="stale-key" + ) + + mocker.patch( + "learning_resources.etl.loaders.load_content_file", + return_value=loaded_cf.id, + autospec=True, + ) + mocker.patch( + "learning_resources.etl.loaders.content_files_loaded_actions", + autospec=True, + ) + + result = load_content_files( + course_run, + [{"key": "loaded-key", "content": "text"}], + failed_keys=["failed-key"], + ) + + assert result == [loaded_cf.id] + + failed_cf.refresh_from_db() + stale_cf.refresh_from_db() + assert failed_cf.published is True + assert stale_cf.published is False + + @pytest.mark.parametrize("test_mode", [True, False]) def test_load_test_mode_resource_content_files( mocker, mock_course_archive_bucket, test_mode From 3f85471e750d46eb2d6cc564cfcd765e371129c3 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 10 Aug 2026 21:38:30 -0400 Subject: [PATCH 06/12] Log OCR-to-tika fallback at warning level Co-Authored-By: Claude Fable 5 --- learning_resources/etl/utils.py | 6 ++---- learning_resources/etl/utils_test.py | 7 ++++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/learning_resources/etl/utils.py b/learning_resources/etl/utils.py index a5213ee356..30cb311abe 100644 --- a/learning_resources/etl/utils.py +++ b/learning_resources/etl/utils.py @@ -655,10 +655,8 @@ def _extract_content( # noqa: PLR0913 ): try: content_dict = _extract_content_with_ocr(file_path, is_tutor_problem) - except Exception: - log.exception( - "OCR extraction failed for %s, falling back to tika", file_path - ) + except Exception: # noqa: BLE001 + log.warning("OCR extraction failed for %s, falling back to tika", file_path) content_dict = None if content_dict: return content_dict diff --git a/learning_resources/etl/utils_test.py b/learning_resources/etl/utils_test.py index 1304da9aea..1b08ec91af 100644 --- a/learning_resources/etl/utils_test.py +++ b/learning_resources/etl/utils_test.py @@ -1152,7 +1152,9 @@ def test_extract_content_ocr_fallback_to_tika(mocker, settings, tmp_path): assert result == {"content": "tika content", "content_title": "Tika Title"} -def test_extract_content_ocr_failure_falls_back_to_tika(mocker, settings, tmp_path): +def test_extract_content_ocr_failure_falls_back_to_tika( + mocker, settings, tmp_path, caplog +): """An OCR crash (e.g. missing converter output JSON) should fall through to tika""" settings.SKIP_TIKA = False mocker.patch("learning_resources.etl.utils._should_use_ocr", return_value=True) @@ -1176,6 +1178,9 @@ def test_extract_content_ocr_failure_falls_back_to_tika(mocker, settings, tmp_pa use_ocr=True, ) assert result["content"] == "tika text" + ocr_records = [r for r in caplog.records if "OCR extraction failed" in r.message] + assert ocr_records + assert all(r.levelname == "WARNING" for r in ocr_records) @pytest.mark.django_db From 83d6d1df17ead35b4c2d65c81b0c6c7d44e750b6 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 10 Aug 2026 21:45:14 -0400 Subject: [PATCH 07/12] Treat pypdf-invalid PDFs as extraction failures Co-Authored-By: Claude Fable 5 --- learning_resources/etl/utils.py | 8 ++++++-- learning_resources/etl/utils_test.py | 30 ++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/learning_resources/etl/utils.py b/learning_resources/etl/utils.py index 30cb311abe..f40cea43d0 100644 --- a/learning_resources/etl/utils.py +++ b/learning_resources/etl/utils.py @@ -66,6 +66,10 @@ log = logging.getLogger(__name__) +class InvalidPDFError(Exception): + """Raised when a PDF fails pypdf validation before extraction.""" + + def load_offeror_topic_map(offeror_code: str): """ Load the topic mappings from the database. @@ -648,8 +652,8 @@ def _extract_content( # noqa: PLR0913 file_extension = metadata.get("file_extension") file_path = Path(olx_path) / Path(source_path) if file_extension == ".pdf" and file_path.is_file() and not pdf_is_valid(file_path): - log.warning("Skipping invalid pdf %s", file_path) - return None + msg = f"Invalid PDF {file_path}" + raise InvalidPDFError(msg) if _should_use_ocr( file_extension=file_extension, file_path=file_path, use_ocr=use_ocr ): diff --git a/learning_resources/etl/utils_test.py b/learning_resources/etl/utils_test.py index 1b08ec91af..9dba4f2f3b 100644 --- a/learning_resources/etl/utils_test.py +++ b/learning_resources/etl/utils_test.py @@ -949,7 +949,7 @@ def test_process_olx_path_malformed_sjson(mocker, settings): def test_process_olx_path_encrypted_pdf(mocker, settings, tmp_path): """ - Test that process_olx_path logs an error and skips encrypted PDFs + Test that process_olx_path records encrypted PDFs as failures and skips them """ settings.OCR_MODEL = "test_model" settings.SKIP_TIKA = False @@ -998,17 +998,22 @@ def test_process_olx_path_encrypted_pdf(mocker, settings, tmp_path): side_effect=pypdf.errors.FileNotDecryptedError, ) + failed = [] results = list( utils.process_olx_path( str(olx_path), run, overwrite=True, use_ocr=True, + failed_source_paths=failed, ) ) assert len(results) == 0 - mock_log.warning.assert_called_with("Skipping invalid pdf %s", full_path) + assert failed == [source_rel_path] + mock_log.exception.assert_called_with( + "Extraction failed for %s in run %s, skipping file", source_rel_path, run.id + ) tika_from_buffer_mock.assert_not_called() @@ -1211,3 +1216,24 @@ def fake_extract(document, metadata, olx_path, key, **kwargs): assert "good.html" in results[0]["source_path"] assert len(failed) == 1 assert "bad.html" in failed[0] + + +def test_extract_content_invalid_pdf_raises(mocker, settings, tmp_path): + """A PDF failing pdf_is_valid raises, so process_olx_path records a failure + instead of the old silent drop + """ + settings.SKIP_TIKA = False # _extract_content short-circuits before pdf_is_valid + (tmp_path / "bad.pdf").write_bytes(b"%PDF- not really") + mocker.patch("learning_resources.etl.utils.pdf_is_valid", return_value=False) + + with pytest.raises(utils.InvalidPDFError): + utils._extract_content( # noqa: SLF001 + b"%PDF- not really", + { + "source_path": "bad.pdf", + "file_extension": ".pdf", + "mime_type": "application/pdf", + }, + str(tmp_path), + "test-key", + ) From 062324e82d7e5fdfed60a23d86b4cebecdd32f3a Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 10 Aug 2026 21:49:13 -0400 Subject: [PATCH 08/12] Retain edX contentfiles whose extraction failed; don't mark all-failed archives empty Co-Authored-By: Claude Fable 5 --- learning_resources/etl/edx_shared.py | 10 +++- learning_resources/etl/edx_shared_test.py | 72 ++++++++++++++++++++++- learning_resources/etl/utils.py | 18 +++++- 3 files changed, 95 insertions(+), 5 deletions(-) diff --git a/learning_resources/etl/edx_shared.py b/learning_resources/etl/edx_shared.py index e75c93bde5..b5666ff9d1 100644 --- a/learning_resources/etl/edx_shared.py +++ b/learning_resources/etl/edx_shared.py @@ -148,17 +148,23 @@ def process_course_archive( log.info("Checksums match for %s, skipping load", key) return True try: + failed_keys = [] content_files_data = iter( - transform_content_files(course_tarpath, run, overwrite=overwrite) + transform_content_files( + course_tarpath, run, overwrite=overwrite, failed_keys=failed_keys + ) ) first = next(content_files_data, None) if first is None: + if failed_keys: + # every file failed: retry next sync, don't mark as empty + return True # 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, chain([first], content_files_data) + run, chain([first], content_files_data), failed_keys=failed_keys ) if content_files_ids: run.checksum = checksum diff --git a/learning_resources/etl/edx_shared_test.py b/learning_resources/etl/edx_shared_test.py index 31a0070fa2..ecdba18df2 100644 --- a/learning_resources/etl/edx_shared_test.py +++ b/learning_resources/etl/edx_shared_test.py @@ -1,5 +1,7 @@ """ETL utils test""" +import shutil +import tarfile from datetime import datetime from pathlib import Path from zoneinfo import ZoneInfo @@ -16,13 +18,15 @@ process_course_archive, sync_edx_course_files, ) -from learning_resources.etl.utils import get_s3_prefix_for_source +from learning_resources.etl.utils import get_edx_module_id, get_s3_prefix_for_source from learning_resources.factories import ( + ContentFileFactory, CourseFactory, LearningResourceFactory, LearningResourcePlatformFactory, LearningResourceRunFactory, ) +from learning_resources.models import ContentFile pytestmark = pytest.mark.django_db @@ -1565,3 +1569,69 @@ def fake_load(run_arg, data, **kwargs): run.refresh_from_db() assert run.archive_key == key assert run.checksum == "newchecksum" + + +def _make_olx_tarball(tmp_path): + """Build a minimal OLX tarball with one good and one bad static file""" + course_dir = tmp_path / "course" + static_dir = course_dir / "static" + static_dir.mkdir(parents=True) + (static_dir / "good.html").write_text("

good

") + (static_dir / "bad.html").write_text("

bad

") + tarball = tmp_path / "course.tar.gz" + with tarfile.open(tarball, "w:gz") as tar: + tar.add(course_dir, arcname="course") + return tarball + + +def test_process_course_archive_retains_failed_file(mocker, tmp_path): + """One raising file: others load, its existing ContentFile stays published, + checksum and archive_key are stamped + """ + run = LearningResourceRunFactory.create(archive_key=None, checksum=None) + tarball = _make_olx_tarball(tmp_path) + bucket = mocker.MagicMock() + bucket.download_file.side_effect = lambda _key, dest: shutil.copy(tarball, dest) + + failing_key = get_edx_module_id("course/static/bad.html", run) + existing = ContentFileFactory.create(run=run, key=failing_key, published=True) + + def fake_extract(document, metadata, olx_path, key, **kwargs): + if "bad.html" in metadata["source_path"]: + msg = "converter output missing" + raise FileNotFoundError(msg) + return {"content": "text", "content_title": ""} + + mocker.patch( + "learning_resources.etl.utils._extract_content", side_effect=fake_extract + ) + + key = "20240101/courses/course.tar.gz" + process_course_archive(bucket, key, run) + + good_key = get_edx_module_id("course/static/good.html", run) + assert ContentFile.objects.filter(run=run, key=good_key).exists() + existing.refresh_from_db() + assert existing.published is True + run.refresh_from_db() + assert run.checksum + assert run.archive_key == key + + +def test_process_course_archive_all_failures_not_marked_empty(mocker, tmp_path): + """Every file raising: archive_key is NOT stamped, so the archive retries""" + run = LearningResourceRunFactory.create(archive_key=None, checksum=None) + tarball = _make_olx_tarball(tmp_path) + bucket = mocker.MagicMock() + bucket.download_file.side_effect = lambda _key, dest: shutil.copy(tarball, dest) + + mocker.patch( + "learning_resources.etl.utils._extract_content", + side_effect=FileNotFoundError("converter output missing"), + ) + + process_course_archive(bucket, "20240101/courses/course.tar.gz", run) + + run.refresh_from_db() + assert run.archive_key is None + assert run.checksum is None diff --git a/learning_resources/etl/utils.py b/learning_resources/etl/utils.py index f40cea43d0..f1834f8954 100644 --- a/learning_resources/etl/utils.py +++ b/learning_resources/etl/utils.py @@ -853,7 +853,11 @@ def process_olx_path( # noqa: PLR0913 def transform_content_files( - course_tarpath: Path, run: LearningResourceRun, *, overwrite: bool + course_tarpath: Path, + run: LearningResourceRun, + *, + overwrite: bool, + failed_keys: list | None = None, ) -> Generator[dict, None, None]: """ Pass content to tika, then return JSON document with transformed content inside it @@ -861,15 +865,25 @@ def transform_content_files( Args: course_tarpath (str): The path to the tarball which contains the OLX run (LearningResourceRun): The run associated witb the content files + failed_keys (list): caller-owned list extended in place with the + ContentFile keys of files whose extraction failed; valid only once + the generator is fully exhausted Yields: dict: content from file """ basedir = course_tarpath.name.split(".")[0] + failed_source_paths = [] with TemporaryDirectory(prefix=basedir) as inner_tempdir: check_call(["tar", "xf", course_tarpath], cwd=inner_tempdir) # noqa: S603,S607 olx_path = glob.glob(inner_tempdir + "/*")[0] # noqa: PTH207 - yield from process_olx_path(olx_path, run, overwrite=overwrite) + yield from process_olx_path( + olx_path, run, overwrite=overwrite, failed_source_paths=failed_source_paths + ) + if failed_keys is not None: + failed_keys.extend( + get_edx_module_id(source_path, run) for source_path in failed_source_paths + ) def get_s3_prefix_for_source(etl_source: str) -> str: From cc8d4c674f34370b7f3084e84502a132df96174b Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 10 Aug 2026 21:55:19 -0400 Subject: [PATCH 09/12] Retry canvas archives where every file failed; add end-to-end failure tests Co-Authored-By: Claude Fable 5 --- learning_resources/etl/canvas.py | 8 ++- learning_resources/etl/canvas_test.py | 75 +++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/learning_resources/etl/canvas.py b/learning_resources/etl/canvas.py index f24fc30bb9..a6e3b0e5cd 100644 --- a/learning_resources/etl/canvas.py +++ b/learning_resources/etl/canvas.py @@ -90,9 +90,13 @@ def sync_canvas_archive(bucket, key: str, overwrite): canvas_problem_files, failed_source_paths=failed_problem_paths, ) - content_loaded = content_files_ids or not canvas_content_files + content_loaded = content_files_ids or ( + not canvas_content_files and not failed_content_keys + ) # load_problem_file swallows per-file errors and returns None - problems_loaded = any(problem_files_ids) or not canvas_problem_files + problems_loaded = any(problem_files_ids) or ( + not canvas_problem_files and not failed_problem_paths + ) 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) diff --git a/learning_resources/etl/canvas_test.py b/learning_resources/etl/canvas_test.py index cff4b15d88..d62685d4e4 100644 --- a/learning_resources/etl/canvas_test.py +++ b/learning_resources/etl/canvas_test.py @@ -2460,3 +2460,78 @@ def fake_download(key, dest): sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False ) assert _canvas_run(readable_id).checksum + + +TWO_FILE_MANIFEST_XML = b""" + + + + + + + + + + +""" + + +def test_sync_canvas_archive_partial_failure_retains_and_stamps( + mocker, tmp_path, sync_mocks +): + """One raising file: others yield, failed record survives the transform's + delete pass, failed_keys reaches load_content_files, checksum is stamped + """ + two_file_zip = make_timed_lock_zip( + tmp_path, + "2001-01-01T00:00:00", + name="two_files.zip", + manifest_xml=TWO_FILE_MANIFEST_XML, + ) + with zipfile.ZipFile(two_file_zip, "a") as zf: + zf.writestr("web_resources/file4.pdf", b"%PDF-fake") + + def fake_download(_key, dest): + Path(dest).write_bytes(two_file_zip.read_bytes()) + + sync_mocks.bucket.download_file.side_effect = fake_download + + # first sync materializes the resource/run (loaders are mocked, so no rows) + key = "canvas/course_content/1/abc.imscc" + readable_id = sync_canvas_archive(sync_mocks.bucket, key, overwrite=False) + run = _canvas_run(readable_id) + failing_key = get_edx_module_id(str(Path("abc") / "web_resources/file4.pdf"), run) + existing = ContentFileFactory.create(run=run, key=failing_key, published=True) + + def fake_extract(document, metadata, olx_path, key, **kwargs): + if "file4.pdf" in metadata["source_path"]: + msg = "converter output missing" + raise FileNotFoundError(msg) + return {"content": "TEXT", "content_title": ""} + + mocker.patch( + "learning_resources.etl.utils._extract_content", side_effect=fake_extract + ) + + sync_canvas_archive(sync_mocks.bucket, key, overwrite=True) + + assert ContentFile.objects.filter(id=existing.id).exists() + assert sync_mocks.load_content.call_args.kwargs["failed_keys"] == [failing_key] + assert _canvas_run(readable_id).checksum + + +def test_sync_canvas_archive_total_failure_does_not_stamp_checksum(mocker, sync_mocks): + """Every file raising: checksum NOT stamped, so next sync retries""" + # a bare MagicMock return is truthy and would satisfy content_loaded via + # content_files_ids, bypassing the gate under test + sync_mocks.load_content.return_value = [] + mocker.patch( + "learning_resources.etl.utils._extract_content", + side_effect=FileNotFoundError("converter output missing"), + ) + + readable_id = sync_canvas_archive( + sync_mocks.bucket, "canvas/course_content/1/abc.imscc", overwrite=False + ) + + assert _canvas_run(readable_id).checksum is None From d6131b9018d74b056d24b74fa1de4e2894f01fa5 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 10 Aug 2026 21:59:04 -0400 Subject: [PATCH 10/12] Make failure accumulator params keyword-only and document the contract Co-Authored-By: Claude Fable 5 --- learning_resources/etl/loaders.py | 11 +++++++---- learning_resources/etl/utils.py | 8 +++++++- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/learning_resources/etl/loaders.py b/learning_resources/etl/loaders.py index cac9297328..1c1f045fba 100644 --- a/learning_resources/etl/loaders.py +++ b/learning_resources/etl/loaders.py @@ -1023,8 +1023,9 @@ def load_content_files( course_run (LearningResourceRun): a course run content_files_data (list or generator): Details about the content files calc_completeness: bool: Whether to calculate the completeness score - failed_keys: list: Keys of content files whose extraction failed and - should be exempted from the stale/unpublish pass + failed_keys: list: caller-owned list of ContentFile keys whose + extraction failed, mutated in place while content_files_data is + consumed; those records are exempted from the stale/unpublish pass Returns: list of int: Ids of the ContentFile objects that were created/updated @@ -1203,6 +1204,7 @@ def load_problem_file( def load_problem_files( course_run: LearningResourceRun, problem_files_data: list[dict], + *, failed_source_paths: list | None = None, ) -> list[int]: """ @@ -1211,8 +1213,9 @@ def load_problem_files( Args: course_run (LearningResourceRun): a course run problem_files_data (list or generator): Details about the problem files - failed_source_paths (list): source paths whose extraction failed; these - are retained rather than deleted as orphans + failed_source_paths (list): caller-owned list of source paths whose + extraction failed, mutated in place while problem_files_data is + consumed; those records are retained rather than deleted as orphans Returns: list of int: Ids of the TutorProblemFile objects that were created/updated diff --git a/learning_resources/etl/utils.py b/learning_resources/etl/utils.py index f1834f8954..0292b5018e 100644 --- a/learning_resources/etl/utils.py +++ b/learning_resources/etl/utils.py @@ -808,7 +808,13 @@ def process_olx_path( # noqa: PLR0913 use_ocr=False, failed_source_paths: list | None = None, ) -> Generator[dict, None, None]: - """Process OLX path and yield content dictionaries.""" + """ + Process OLX path and yield content dictionaries. + + failed_source_paths is a caller-owned list appended to in place with the + source_path of each file whose processing raises; it is only complete once + the generator is fully exhausted. + """ video_srt_metadata = get_video_metadata(olx_path, run) for document, metadata in documents_from_olx( From a152bbc07fa0a0b977b1081de36dbd2fedec2d89 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 10 Aug 2026 22:11:45 -0400 Subject: [PATCH 11/12] Pin course resource type in edX retention tests load_content_files no-ops for non-course resources, so the randomized factory type made the test order-dependent. Co-Authored-By: Claude Fable 5 --- learning_resources/etl/edx_shared_test.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/learning_resources/etl/edx_shared_test.py b/learning_resources/etl/edx_shared_test.py index ecdba18df2..5cdb30586b 100644 --- a/learning_resources/etl/edx_shared_test.py +++ b/learning_resources/etl/edx_shared_test.py @@ -1588,7 +1588,12 @@ def test_process_course_archive_retains_failed_file(mocker, tmp_path): """One raising file: others load, its existing ContentFile stays published, checksum and archive_key are stamped """ - run = LearningResourceRunFactory.create(archive_key=None, checksum=None) + # load_content_files no-ops for non-course resources; pin the random factory + run = LearningResourceRunFactory.create( + archive_key=None, + checksum=None, + learning_resource=LearningResourceFactory.create(is_course=True), + ) tarball = _make_olx_tarball(tmp_path) bucket = mocker.MagicMock() bucket.download_file.side_effect = lambda _key, dest: shutil.copy(tarball, dest) @@ -1620,7 +1625,11 @@ def fake_extract(document, metadata, olx_path, key, **kwargs): def test_process_course_archive_all_failures_not_marked_empty(mocker, tmp_path): """Every file raising: archive_key is NOT stamped, so the archive retries""" - run = LearningResourceRunFactory.create(archive_key=None, checksum=None) + run = LearningResourceRunFactory.create( + archive_key=None, + checksum=None, + learning_resource=LearningResourceFactory.create(is_course=True), + ) tarball = _make_olx_tarball(tmp_path) bucket = mocker.MagicMock() bucket.download_file.side_effect = lambda _key, dest: shutil.copy(tarball, dest) From 97cd4e7a7c3fc41b8fb36364363033ede2e1b322 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Tue, 11 Aug 2026 07:02:41 -0400 Subject: [PATCH 12/12] Mock search-index plugin hook in edX retention test content_files_loaded_actions fires search indexing gated by randomized resource fields; when the draw enables it, the eager task hits the nonexistent test opensearch and the resulting Retry is swallowed by process_course_archive's bare except, leaving the checksum unstamped. Co-Authored-By: Claude Fable 5 --- learning_resources/etl/edx_shared_test.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/learning_resources/etl/edx_shared_test.py b/learning_resources/etl/edx_shared_test.py index 5cdb30586b..e0a8bc8a5c 100644 --- a/learning_resources/etl/edx_shared_test.py +++ b/learning_resources/etl/edx_shared_test.py @@ -1610,6 +1610,9 @@ def fake_extract(document, metadata, olx_path, key, **kwargs): mocker.patch( "learning_resources.etl.utils._extract_content", side_effect=fake_extract ) + # search-index plugin hook, not under test; depends on randomized resource + # fields and hits a nonexistent opensearch in CI + mocker.patch("learning_resources.etl.loaders.content_files_loaded_actions") key = "20240101/courses/course.tar.gz" process_course_archive(bucket, key, run)