From 79a6a605c91ec1c9eae30b80bf026843d839bd2e Mon Sep 17 00:00:00 2001 From: shankar ambady Date: Wed, 5 Aug 2026 15:14:30 -0400 Subject: [PATCH 1/9] adding check for image reachability --- learning_resources/utils.py | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/learning_resources/utils.py b/learning_resources/utils.py index 3823b3fd27..7e75efe4c5 100644 --- a/learning_resources/utils.py +++ b/learning_resources/utils.py @@ -4,6 +4,8 @@ import re from collections import defaultdict from functools import cache +from hashlib import md5 +from http import HTTPStatus from shutil import which from typing import TYPE_CHECKING @@ -45,6 +47,7 @@ log = logging.getLogger() BLOCKLIST_CACHE_TIMEOUT = 60 * 60 * 24 +IMAGE_URL_REACHABLE_CACHE_TIMEOUT = 60 * 60 * 24 # edX block types that never have content @@ -227,6 +230,40 @@ def load_course_blocklist(): return blocklist +def image_url_is_reachable(url: str) -> bool: + """ + Check whether an image URL responds successfully, caching the result. + + Args: + url (str): the image URL to check + + Returns: + bool: True if the URL responds successfully + """ + cache_key = f"image_url_reachable:{md5(url.encode('utf-8')).hexdigest()}" # noqa: S324 + redis_cache = caches["redis"] + reachable = redis_cache.get(cache_key) + if reachable is None: + try: + response = requests.head( + url, timeout=settings.REQUESTS_TIMEOUT, allow_redirects=True + ) + if response.status_code in ( + HTTPStatus.METHOD_NOT_ALLOWED, + HTTPStatus.NOT_IMPLEMENTED, + ): + # Some servers reject HEAD requests; retry without the body + response = requests.get( + url, timeout=settings.REQUESTS_TIMEOUT, stream=True + ) + response.close() + reachable = response.ok + except requests.RequestException: + reachable = False + redis_cache.set(cache_key, reachable, timeout=IMAGE_URL_REACHABLE_CACHE_TIMEOUT) + return reachable + + def load_course_duplicates(etl_source: str) -> list: """ Get a list of blocklisted course ids for an ETL pipeline source From ed6b25272f7e688c0083570864d50a423931c4ea Mon Sep 17 00:00:00 2001 From: shankar ambady Date: Wed, 5 Aug 2026 15:15:20 -0400 Subject: [PATCH 2/9] use fallback image url --- learning_resources_search/tasks.py | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/learning_resources_search/tasks.py b/learning_resources_search/tasks.py index eb817d2bdd..5bb42677d9 100644 --- a/learning_resources_search/tasks.py +++ b/learning_resources_search/tasks.py @@ -27,7 +27,7 @@ LearningResourceDepartment, LearningResourceOfferor, ) -from learning_resources.utils import load_course_blocklist +from learning_resources.utils import image_url_is_reachable, load_course_blocklist from learning_resources.views import FeaturedViewSet from learning_resources_search import indexing_api as api from learning_resources_search.api import ( @@ -217,6 +217,21 @@ def key_func(x): return grouped_data +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 @@ -229,6 +244,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: @@ -248,9 +264,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, From 5b7a2321b2611773e9cfbbd0cfcfda8736516651 Mon Sep 17 00:00:00 2001 From: shankar ambady Date: Thu, 6 Aug 2026 18:08:36 -0400 Subject: [PATCH 3/9] adding tests and celery task for cleaning resources with bad images --- learning_resources/tasks.py | 39 ++++++++++++++++- learning_resources/tasks_test.py | 56 +++++++++++++++++++++++++ learning_resources/utils_test.py | 47 ++++++++++++++++++++- learning_resources_search/tasks_test.py | 35 ++++++++++++++++ main/settings_celery.py | 6 +++ 5 files changed, 181 insertions(+), 2 deletions(-) diff --git a/learning_resources/tasks.py b/learning_resources/tasks.py index 6bd4b0a37e..e9ed822bdb 100644 --- a/learning_resources/tasks.py +++ b/learning_resources/tasks.py @@ -38,11 +38,16 @@ get_bucket_by_name, get_s3_prefix_for_source, ) -from learning_resources.models import ContentFile, LearningResource +from learning_resources.models import ( + ContentFile, + LearningResource, + LearningResourceImage, +) from learning_resources.site_scrapers.utils import scraper_for_site from learning_resources.utils import ( build_program_children_content_bulk, html_to_markdown, + image_url_is_reachable, load_course_blocklist, programs_needing_children_heal, resource_unpublished_actions, @@ -847,3 +852,35 @@ def cleanup_deleted_content_files(): error = "cleanup_deleted_content_files threw an error" log.exception(error) return error + + +@app.task(acks_late=True, reject_on_worker_lost=True) +def prune_unreachable_resource_images(): + """ + Delete LearningResourceImage records whose URLs no longer resolve, so that + resources fall back to the default image everywhere (API, search, emails) + instead of rendering a broken image. + """ + image_ids = ( + LearningResource.objects.filter(published=True, image__isnull=False) + .values_list("image_id", flat=True) + .distinct() + ) + removed = 0 + for image in LearningResourceImage.objects.filter(id__in=image_ids).iterator(): + if image_url_is_reachable(image.url): + continue + resource_ids = list(image.learningresource_set.values_list("id", flat=True)) + log.info( + "Pruning unreachable image %s from resources %s", image.url, resource_ids + ) + # FK is SET_NULL, so deleting the record clears it on referencing resources + image.delete() + removed += 1 + for resource in LearningResource.objects.filter( + id__in=resource_ids, published=True + ): + resource_upserted_actions( + resource, percolate=False, generate_embeddings=False + ) + return removed diff --git a/learning_resources/tasks_test.py b/learning_resources/tasks_test.py index bfd9d591a0..c4cf941439 100644 --- a/learning_resources/tasks_test.py +++ b/learning_resources/tasks_test.py @@ -26,6 +26,7 @@ get_youtube_data, get_youtube_transcripts, marketing_page_for_resources, + prune_unreachable_resource_images, scrape_marketing_pages, sync_canvas_courses, update_next_start_date_and_prices, @@ -1187,3 +1188,58 @@ def test_cleanup_deleted_content_files_returns_error_on_unexpected_exception(moc result = cleanup_deleted_content_files() assert result == "cleanup_deleted_content_files threw an error" + + +def test_prune_unreachable_resource_images(mocker): + """ + prune_unreachable_resource_images should delete image records with dead + URLs and reindex the affected published resources + """ + good_resource = LearningResourceFactory.create(is_course=True) + bad_resource = LearningResourceFactory.create(is_course=True) + unpublished_resource = LearningResourceFactory.create( + is_course=True, published=False + ) + reachable_mock = mocker.patch( + "learning_resources.tasks.image_url_is_reachable", + side_effect=lambda url: url == good_resource.image.url, + ) + upserted_mock = mocker.patch("learning_resources.tasks.resource_upserted_actions") + + removed = prune_unreachable_resource_images() + + assert removed == 1 + good_resource.refresh_from_db() + bad_resource.refresh_from_db() + unpublished_resource.refresh_from_db() + assert good_resource.image is not None + assert bad_resource.image is None + # unpublished resources should not be checked at all + assert unpublished_resource.image is not None + checked_urls = [call.args[0] for call in reachable_mock.call_args_list] + assert unpublished_resource.image.url not in checked_urls + upserted_mock.assert_called_once_with( + bad_resource, percolate=False, generate_embeddings=False + ) + + +def test_prune_unreachable_resource_images_shared_image(mocker): + """ + Images are deduplicated by URL, so a single dead image record can be shared + by many resources. All published referencing resources should be reindexed. + """ + shared_image = factories.LearningResourceImageFactory.create( + url="http://example.com/dead.jpg" + ) + shared = LearningResourceFactory.create_batch(2, is_course=True, image=shared_image) + mocker.patch("learning_resources.tasks.image_url_is_reachable", return_value=False) + upserted_mock = mocker.patch("learning_resources.tasks.resource_upserted_actions") + + # a shared image is only checked (and deleted) once + assert prune_unreachable_resource_images() == 1 + assert not models.LearningResourceImage.objects.filter(id=shared_image.id).exists() + for resource in shared: + resource.refresh_from_db() + assert resource.image is None + reindexed = {call.args[0].id for call in upserted_mock.call_args_list} + assert reindexed == {resource.id for resource in shared} diff --git a/learning_resources/utils_test.py b/learning_resources/utils_test.py index f8b1f3124c..776842c11a 100644 --- a/learning_resources/utils_test.py +++ b/learning_resources/utils_test.py @@ -6,11 +6,13 @@ import random from pathlib import Path -import markdown import pytest +import requests +import responses import yaml from faker import Faker +import markdown from data_fixtures import utils as data_utils from learning_resources import utils from learning_resources.constants import ( @@ -45,6 +47,7 @@ build_program_children_content, build_program_children_content_bulk, filter_valid_edx_module_ids, + image_url_is_reachable, is_loggable_missing_content_id, is_valid_edx_module_id, log_missing_content_file, @@ -1304,3 +1307,45 @@ def test_sanitize_llm_text(text, expected): assert result == expected # The result must always be storable: strict UTF-8 encoding cannot raise result.encode("utf-8") + + +@pytest.mark.parametrize( + ("status", "expected"), + [(200, True), (301, 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 + + +def test_image_url_is_reachable_head_not_allowed(mocked_responses): + """image_url_is_reachable should fall back to GET if HEAD is rejected""" + 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): + """image_url_is_reachable should be False if the request errors out""" + url = "http://example.com/image.jpg" + mocked_responses.add( + responses.HEAD, url, body=requests.exceptions.ConnectionError() + ) + assert image_url_is_reachable(url) is False + + +def test_image_url_is_reachable_caches_result(mocked_responses, settings): + """image_url_is_reachable should cache results per URL""" + settings.CACHES = { + **settings.CACHES, + "redis": {"BACKEND": "django.core.cache.backends.locmem.LocMemCache"}, + } + url = "http://example.com/image.jpg" + mocked_responses.add(responses.HEAD, url, status=404) + assert image_url_is_reachable(url) is False + # second call hits the cache; RequestsMock would fail on a second request + assert image_url_is_reachable(url) is False + assert len(mocked_responses.calls) == 1 diff --git a/learning_resources_search/tasks_test.py b/learning_resources_search/tasks_test.py index e8f3a67730..c43a58404a 100644 --- a/learning_resources_search/tasks_test.py +++ b/learning_resources_search/tasks_test.py @@ -42,6 +42,7 @@ _get_percolated_rows, _group_percolated_rows, _infer_percolate_group, + _validated_resource_image_url, bulk_deindex_learning_resources, deindex_document, deindex_run_content_files, @@ -58,6 +59,7 @@ ) from main.factories import UserFactory from main.test_utils import assert_not_raises +from main.utils import frontend_absolute_url pytestmark = pytest.mark.django_db User = get_user_model() @@ -80,6 +82,14 @@ def mocked_api(mocker): return mocker.patch("learning_resources_search.tasks.api") +@pytest.fixture(autouse=True) +def mock_image_url_is_reachable(mocker): + """Mock the image URL reachability check to avoid network requests""" + 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() @@ -1245,6 +1255,31 @@ def get_percolator(res): assert topic in template_data +@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 diff --git a/main/settings_celery.py b/main/settings_celery.py index 62f11fce73..e5661ebf1a 100644 --- a/main/settings_celery.py +++ b/main/settings_celery.py @@ -197,6 +197,12 @@ minute=0, hour=6, day_of_week=6 ), # 2:00am EST on Friday }, + "weekly-prune-unreachable-resource-images": { + "task": "learning_resources.tasks.prune_unreachable_resource_images", + "schedule": crontab( + minute=0, hour=10, day_of_week=0 + ), # 6:00am EST on Sunday + }, "cleanup-deleted-content-files-every-1-days": { "task": "learning_resources.tasks.cleanup_deleted_content_files", "schedule": crontab( From a3c19f6587792f272ef5e58e05f09a9b342b7e3e Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 6 Aug 2026 22:36:34 +0000 Subject: [PATCH 4/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- learning_resources/utils_test.py | 2 +- learning_resources_search/tasks_test.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/learning_resources/utils_test.py b/learning_resources/utils_test.py index 776842c11a..e3dbb76d46 100644 --- a/learning_resources/utils_test.py +++ b/learning_resources/utils_test.py @@ -6,13 +6,13 @@ import random from pathlib import Path +import markdown import pytest import requests import responses import yaml from faker import Faker -import markdown from data_fixtures import utils as data_utils from learning_resources import utils from learning_resources.constants import ( diff --git a/learning_resources_search/tasks_test.py b/learning_resources_search/tasks_test.py index f211d84323..8bbc789795 100644 --- a/learning_resources_search/tasks_test.py +++ b/learning_resources_search/tasks_test.py @@ -45,8 +45,8 @@ _get_percolated_rows, _group_percolated_rows, _infer_percolate_group, - _validated_resource_image_url, _maybe_finish_reindex_job, + _validated_resource_image_url, bulk_deindex_learning_resources, deindex_document, deindex_run_content_files, From ac761b9fc0e03c3bdb3f797688a8d5ce8cdb86f6 Mon Sep 17 00:00:00 2001 From: Shankar Ambady Date: Sat, 8 Aug 2026 19:52:58 -0400 Subject: [PATCH 5/9] 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> --- learning_resources/utils.py | 70 +++++++++++++++++++++++++++++-------- 1 file changed, 55 insertions(+), 15 deletions(-) diff --git a/learning_resources/utils.py b/learning_resources/utils.py index 7e75efe4c5..3a1bde561e 100644 --- a/learning_resources/utils.py +++ b/learning_resources/utils.py @@ -1,13 +1,16 @@ """Utils for learning resources""" +import ipaddress import logging import re +import socket from collections import defaultdict from functools import cache from hashlib import md5 from http import HTTPStatus from shutil import which from typing import TYPE_CHECKING +from urllib.parse import urlparse import html2text import rapidjson @@ -230,6 +233,40 @@ def load_course_blocklist(): return blocklist +def _is_safe_public_http_url(url: str) -> bool: + """ + Return True if URL is http(s), has a hostname, and resolves only to public IPs. + """ + try: + parsed = urlparse(url) + except ValueError: + return False + + if parsed.scheme not in ("http", "https"): + return False + if not parsed.hostname: + return False + + try: + addrinfo = socket.getaddrinfo(parsed.hostname, None) + except socket.gaierror: + return False + + for entry in addrinfo: + ip_str = entry[4][0] + ip_obj = ipaddress.ip_address(ip_str) + if ( + ip_obj.is_private + or ip_obj.is_loopback + or ip_obj.is_link_local + or ip_obj.is_multicast + or ip_obj.is_reserved + or ip_obj.is_unspecified + ): + return False + return True + + def image_url_is_reachable(url: str) -> bool: """ Check whether an image URL responds successfully, caching the result. @@ -244,22 +281,25 @@ def image_url_is_reachable(url: str) -> bool: redis_cache = caches["redis"] reachable = redis_cache.get(cache_key) if reachable is None: - try: - response = requests.head( - url, timeout=settings.REQUESTS_TIMEOUT, allow_redirects=True - ) - if response.status_code in ( - HTTPStatus.METHOD_NOT_ALLOWED, - HTTPStatus.NOT_IMPLEMENTED, - ): - # Some servers reject HEAD requests; retry without the body - response = requests.get( - url, timeout=settings.REQUESTS_TIMEOUT, stream=True - ) - response.close() - reachable = response.ok - except requests.RequestException: + if not _is_safe_public_http_url(url): reachable = False + else: + try: + response = requests.head( + url, timeout=settings.REQUESTS_TIMEOUT, allow_redirects=True + ) + if response.status_code in ( + HTTPStatus.METHOD_NOT_ALLOWED, + HTTPStatus.NOT_IMPLEMENTED, + ): + # Some servers reject HEAD requests; retry without the body + response = requests.get( + url, timeout=settings.REQUESTS_TIMEOUT, stream=True + ) + response.close() + reachable = response.ok + except requests.RequestException: + reachable = False redis_cache.set(cache_key, reachable, timeout=IMAGE_URL_REACHABLE_CACHE_TIMEOUT) return reachable From 63912ba3bfdf65a783255ee0719c565336c529ca Mon Sep 17 00:00:00 2001 From: shankar ambady Date: Sun, 9 Aug 2026 15:52:15 -0400 Subject: [PATCH 6/9] Revert "Potential fix for pull request finding 'CodeQL / Full server-side request forgery'" This reverts commit ac761b9fc0e03c3bdb3f797688a8d5ce8cdb86f6. --- learning_resources/utils.py | 70 ++++++++----------------------------- 1 file changed, 15 insertions(+), 55 deletions(-) diff --git a/learning_resources/utils.py b/learning_resources/utils.py index 3a1bde561e..7e75efe4c5 100644 --- a/learning_resources/utils.py +++ b/learning_resources/utils.py @@ -1,16 +1,13 @@ """Utils for learning resources""" -import ipaddress import logging import re -import socket from collections import defaultdict from functools import cache from hashlib import md5 from http import HTTPStatus from shutil import which from typing import TYPE_CHECKING -from urllib.parse import urlparse import html2text import rapidjson @@ -233,40 +230,6 @@ def load_course_blocklist(): return blocklist -def _is_safe_public_http_url(url: str) -> bool: - """ - Return True if URL is http(s), has a hostname, and resolves only to public IPs. - """ - try: - parsed = urlparse(url) - except ValueError: - return False - - if parsed.scheme not in ("http", "https"): - return False - if not parsed.hostname: - return False - - try: - addrinfo = socket.getaddrinfo(parsed.hostname, None) - except socket.gaierror: - return False - - for entry in addrinfo: - ip_str = entry[4][0] - ip_obj = ipaddress.ip_address(ip_str) - if ( - ip_obj.is_private - or ip_obj.is_loopback - or ip_obj.is_link_local - or ip_obj.is_multicast - or ip_obj.is_reserved - or ip_obj.is_unspecified - ): - return False - return True - - def image_url_is_reachable(url: str) -> bool: """ Check whether an image URL responds successfully, caching the result. @@ -281,25 +244,22 @@ def image_url_is_reachable(url: str) -> bool: redis_cache = caches["redis"] reachable = redis_cache.get(cache_key) if reachable is None: - if not _is_safe_public_http_url(url): - reachable = False - else: - try: - response = requests.head( - url, timeout=settings.REQUESTS_TIMEOUT, allow_redirects=True + try: + response = requests.head( + url, timeout=settings.REQUESTS_TIMEOUT, allow_redirects=True + ) + if response.status_code in ( + HTTPStatus.METHOD_NOT_ALLOWED, + HTTPStatus.NOT_IMPLEMENTED, + ): + # Some servers reject HEAD requests; retry without the body + response = requests.get( + url, timeout=settings.REQUESTS_TIMEOUT, stream=True ) - if response.status_code in ( - HTTPStatus.METHOD_NOT_ALLOWED, - HTTPStatus.NOT_IMPLEMENTED, - ): - # Some servers reject HEAD requests; retry without the body - response = requests.get( - url, timeout=settings.REQUESTS_TIMEOUT, stream=True - ) - response.close() - reachable = response.ok - except requests.RequestException: - reachable = False + response.close() + reachable = response.ok + except requests.RequestException: + reachable = False redis_cache.set(cache_key, reachable, timeout=IMAGE_URL_REACHABLE_CACHE_TIMEOUT) return reachable From dc7154aa338bea1844fe23b7ce0497735900bcb4 Mon Sep 17 00:00:00 2001 From: shankar ambady Date: Sun, 9 Aug 2026 19:59:24 -0400 Subject: [PATCH 7/9] more efficient processing for image pruning task --- learning_resources/tasks.py | 31 ++++++++++++++++++-- learning_resources/tasks_test.py | 49 ++++++++++++++++++++++++++++---- main/settings.py | 1 + 3 files changed, 73 insertions(+), 8 deletions(-) diff --git a/learning_resources/tasks.py b/learning_resources/tasks.py index e9ed822bdb..2afbebbf62 100644 --- a/learning_resources/tasks.py +++ b/learning_resources/tasks.py @@ -854,18 +854,43 @@ def cleanup_deleted_content_files(): return error -@app.task(acks_late=True, reject_on_worker_lost=True) -def prune_unreachable_resource_images(): +@app.task(bind=True, acks_late=True) +def prune_unreachable_resource_images(self, *, chunk_size=None): """ Delete LearningResourceImage records whose URLs no longer resolve, so that resources fall back to the default image everywhere (API, search, emails) instead of rendering a broken image. """ - image_ids = ( + if chunk_size is None: + chunk_size = settings.IMAGE_PRUNE_CHUNK_SIZE + + image_ids = list( LearningResource.objects.filter(published=True, image__isnull=False) + .order_by("image_id") .values_list("image_id", flat=True) .distinct() ) + if not image_ids: + return None + + tasks = celery.group( + [ + check_and_prune_image_batch.si(ids) + for ids in chunks(image_ids, chunk_size=chunk_size) + ] + ) + return self.replace(tasks) + + +@app.task( + acks_late=True, + reject_on_worker_lost=True, + rate_limit=settings.CELERY_RATE_LIMIT, +) +def check_and_prune_image_batch(image_ids: list[int]) -> int: + """ + Check image reachability for a batch of image IDs and prune unreachable images. + """ removed = 0 for image in LearningResourceImage.objects.filter(id__in=image_ids).iterator(): if image_url_is_reachable(image.url): diff --git a/learning_resources/tasks_test.py b/learning_resources/tasks_test.py index c4cf941439..0245fc1457 100644 --- a/learning_resources/tasks_test.py +++ b/learning_resources/tasks_test.py @@ -21,6 +21,7 @@ ) from learning_resources.models import ContentFile, LearningResource from learning_resources.tasks import ( + check_and_prune_image_batch, cleanup_deleted_content_files, get_ocw_data, get_youtube_data, @@ -1190,9 +1191,45 @@ def test_cleanup_deleted_content_files_returns_error_on_unexpected_exception(moc assert result == "cleanup_deleted_content_files threw an error" -def test_prune_unreachable_resource_images(mocker): +def test_prune_unreachable_resource_images_dispatcher(mocker, settings, mocked_celery): """ - prune_unreachable_resource_images should delete image records with dead + prune_unreachable_resource_images should query distinct image IDs for published + resources, chunk them, and dispatch check_and_prune_image_batch subtasks via group + """ + settings.IMAGE_PRUNE_CHUNK_SIZE = 2 + published_resources = LearningResourceFactory.create_batch(3, is_course=True) + LearningResourceFactory.create(is_course=True, published=False) + + si_mock = mocker.patch("learning_resources.tasks.check_and_prune_image_batch.si") + si_mock.side_effect = lambda ids: ("si", tuple(ids)) + + with pytest.raises(mocked_celery.replace_exception_class): + prune_unreachable_resource_images.delay() + + published_image_ids = sorted( + {res.image.id for res in published_resources if res.image} + ) + assert len(published_image_ids) == 3 + assert si_mock.call_count == 2 + assert si_mock.call_args_list[0].args[0] == published_image_ids[:2] + assert si_mock.call_args_list[1].args[0] == published_image_ids[2:] + mocked_celery.group.assert_called_once() + assert mocked_celery.replace.call_count == 1 + + +def test_prune_unreachable_resource_images_dispatcher_no_images(mocked_celery): + """ + prune_unreachable_resource_images should return None if no published resources have images + """ + LearningResourceFactory.create(is_course=True, image=None, published=True) + assert prune_unreachable_resource_images.delay().result is None + assert mocked_celery.group.call_count == 0 + assert mocked_celery.replace.call_count == 0 + + +def test_check_and_prune_image_batch(mocker): + """ + check_and_prune_image_batch should delete image records with dead URLs and reindex the affected published resources """ good_resource = LearningResourceFactory.create(is_course=True) @@ -1206,7 +1243,9 @@ def test_prune_unreachable_resource_images(mocker): ) upserted_mock = mocker.patch("learning_resources.tasks.resource_upserted_actions") - removed = prune_unreachable_resource_images() + removed = check_and_prune_image_batch( + [good_resource.image.id, bad_resource.image.id] + ) assert removed == 1 good_resource.refresh_from_db() @@ -1223,7 +1262,7 @@ def test_prune_unreachable_resource_images(mocker): ) -def test_prune_unreachable_resource_images_shared_image(mocker): +def test_check_and_prune_image_batch_shared_image(mocker): """ Images are deduplicated by URL, so a single dead image record can be shared by many resources. All published referencing resources should be reindexed. @@ -1236,7 +1275,7 @@ def test_prune_unreachable_resource_images_shared_image(mocker): upserted_mock = mocker.patch("learning_resources.tasks.resource_upserted_actions") # a shared image is only checked (and deleted) once - assert prune_unreachable_resource_images() == 1 + assert check_and_prune_image_batch([shared_image.id]) == 1 assert not models.LearningResourceImage.objects.filter(id=shared_image.id).exists() for resource in shared: resource.refresh_from_db() diff --git a/main/settings.py b/main/settings.py index c791de962f..cd953a4480 100644 --- a/main/settings.py +++ b/main/settings.py @@ -560,6 +560,7 @@ INDEXING_ERROR_RETRIES = get_int("INDEXING_ERROR_RETRIES", 1) CONTENT_FILE_RETENTION_DAYS = get_int("CONTENT_FILE_RETENTION_DAYS", 14) CONTENT_FILE_CLEANUP_CHUNK_SIZE = get_int("CONTENT_FILE_CLEANUP_CHUNK_SIZE", 1000) +IMAGE_PRUNE_CHUNK_SIZE = get_int("IMAGE_PRUNE_CHUNK_SIZE", 100) # JWT authentication settings MITOL_JWT_SECRET = get_string( From 00e1708dc91ce1070bc2d76c646274dafcc84ab4 Mon Sep 17 00:00:00 2001 From: shankar ambady Date: Sun, 9 Aug 2026 22:06:43 -0400 Subject: [PATCH 8/9] follow through for 301 --- learning_resources/utils.py | 4 +++- learning_resources/utils_test.py | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/learning_resources/utils.py b/learning_resources/utils.py index 7e75efe4c5..9b489f4e58 100644 --- a/learning_resources/utils.py +++ b/learning_resources/utils.py @@ -257,7 +257,9 @@ def image_url_is_reachable(url: str) -> bool: url, timeout=settings.REQUESTS_TIMEOUT, stream=True ) response.close() - reachable = response.ok + reachable = ( + HTTPStatus.OK <= response.status_code < HTTPStatus.MULTIPLE_CHOICES + ) except requests.RequestException: reachable = False redis_cache.set(cache_key, reachable, timeout=IMAGE_URL_REACHABLE_CACHE_TIMEOUT) diff --git a/learning_resources/utils_test.py b/learning_resources/utils_test.py index e3dbb76d46..a0851adf90 100644 --- a/learning_resources/utils_test.py +++ b/learning_resources/utils_test.py @@ -6,13 +6,13 @@ import random from pathlib import Path -import markdown import pytest import requests import responses import yaml from faker import Faker +import markdown from data_fixtures import utils as data_utils from learning_resources import utils from learning_resources.constants import ( @@ -1311,7 +1311,7 @@ def test_sanitize_llm_text(text, expected): @pytest.mark.parametrize( ("status", "expected"), - [(200, True), (301, True), (404, False), (403, False), (500, False)], + [(200, True), (301, False), (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""" From a048d37b793e4d138c9866716f91dfec68f1f5ab Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 10 Aug 2026 02:14:21 +0000 Subject: [PATCH 9/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- learning_resources/utils_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/learning_resources/utils_test.py b/learning_resources/utils_test.py index a0851adf90..b41f0f47cf 100644 --- a/learning_resources/utils_test.py +++ b/learning_resources/utils_test.py @@ -6,13 +6,13 @@ import random from pathlib import Path +import markdown import pytest import requests import responses import yaml from faker import Faker -import markdown from data_fixtures import utils as data_utils from learning_resources import utils from learning_resources.constants import (