diff --git a/.profile b/.profile new file mode 100644 index 00000000..7a612214 --- /dev/null +++ b/.profile @@ -0,0 +1,14 @@ +# Sourced by Heroku before the dyno's command runs, for every process type — +# web, worker, the release phase, and one-off `heroku run` dynos. +# +# glibc hands each thread its own malloc arena, up to 8 x nproc, and a dyno +# reports the host's core count rather than its own share. Arenas are never +# returned to the OS, so on a process with real thread churn — the publisher +# builds a fresh ThreadPoolExecutor every 15s, and boto3's managed transfer +# adds ten threads per download — RSS ratchets upward and never comes back. +# That is what put the 512MB worker at 110% of quota with 3081 R14s in a day. +# +# Set as a DEFAULT, not an override: .profile is sourced after config vars are +# injected, so a bare assignment would silently stomp a value set from the +# dashboard and make this untunable without a deploy. +export MALLOC_ARENA_MAX="${MALLOC_ARENA_MAX:-2}" diff --git a/Procfile b/Procfile index bb9b0fa8..358efbb4 100644 --- a/Procfile +++ b/Procfile @@ -1,3 +1,3 @@ release: python manage.py migrate web: gunicorn config.wsgi:application --bind 0.0.0.0:$PORT --workers 1 --threads 4 -worker: python manage.py process_tasks +worker: python manage.py process_tasks --duration 3600 diff --git a/README.md b/README.md index dab69c64..dc993951 100644 --- a/README.md +++ b/README.md @@ -311,6 +311,10 @@ All platforms with ephemeral filesystems require `STORAGE_BACKEND=s3` - see `.en **Memory sizing.** Importing the app costs roughly 100 MB before it serves a request, and a warmed Gunicorn worker settles near 200 MB - so the shipped command runs a single threaded worker (`--workers 1 --threads 4`), which fits a 512 MB dyno with room for media handling. Raise `--workers` only when you raise the memory to match; a rule of thumb is 250 MB per worker. Do **not** add `--max-requests`: gthread stops heartbeating at the start of the request that trips the counter, so the arbiter kills the worker mid-request once `--timeout` (30s) passes - which drops whichever upload happened to be that request, and uploads here can be up to 1 GB. The `worker` process needs the same headroom: it downloads videos to disk and streams them to the platform, and if it is killed mid-publish (Heroku's R15, an OOM kill, a deploy) the affected post is failed by the confirmation sweep rather than left in limbo. +**Why the worker runs with `--duration 3600`.** `process_tasks` is a single non-forking process that runs every task in the same heap and never restarts, so a peak allocation raises RSS permanently - CPython and glibc keep the freed pages in their own arenas. Left alone it ratchets: on a 512 MB Basic dyno it climbed from ~280 MB after a deploy to 566 MB over 15 hours and sat at 110% of quota. `--duration` is checked at the *top* of the run loop, so a task in flight always finishes; the process then exits 0 between tasks and the platform restarts it at its floor. Nothing is lost - a recycle cannot interrupt a publish, and `confirm_pending_publishes` settles anything in flight regardless. Don't go below ~1800s, where Heroku's crash cool-off starts to engage. Other deploy targets keep the plain command: `docker-compose.yml` has no `restart:` policy on the worker, so there a clean exit would simply stop it. + +**`MALLOC_ARENA_MAX`.** glibc gives each thread its own arena (up to 64 MB) capped at `8 x nproc`, and containers report the host's core count, so the cap is effectively unbounded. This app has real thread churn - the publisher builds a fresh pool every 15s and boto3's managed transfer adds ten threads per download - and those arenas are never returned to the OS. On Heroku this is already set to 2 by the repo's `.profile`, which applies to web, worker, the release phase and one-off `heroku run` dynos alike; it is written as a default rather than an override, so a config var still wins if you want to tune it. Deploy targets that build from the `Dockerfile` (Render, Railway, docker-compose) do not read `.profile` - set it in their own environment config if the host is memory-tight. + See `architecture.md` for detailed per-platform instructions and cost breakdowns. ## Project Structure diff --git a/apps/accounts/views.py b/apps/accounts/views.py index 487d0ff5..d87edeea 100644 --- a/apps/accounts/views.py +++ b/apps/accounts/views.py @@ -5,7 +5,6 @@ from django.shortcuts import redirect, render from django.utils import timezone from django.views.decorators.http import require_http_methods -from PIL import Image def health_check(request): @@ -110,9 +109,17 @@ def _handle_photo_update(request, user): return # Validate minimum dimensions (180x180) + # + # PIL is imported here rather than at module scope because config.urls + # imports this module for ``health_check``, so a top-level import loaded + # Pillow's _imaging extension into every web AND worker process at boot, + # whether or not an avatar was ever touched. Matches how the rest of the + # codebase imports PIL (apps.media_library.services, apps.intelligence.views). + from PIL import Image + try: - img = Image.open(avatar) - width, height = img.size + with Image.open(avatar) as img: + width, height = img.size if width < 180 or height < 180: messages.error(request, "Photo must be at least 180×180 pixels.") return diff --git a/apps/analytics/services.py b/apps/analytics/services.py index 9bb3483f..c508898e 100644 --- a/apps/analytics/services.py +++ b/apps/analytics/services.py @@ -14,6 +14,7 @@ from datetime import timedelta from typing import Any, NamedTuple +from django.db import connections from django.utils import timezone from apps.composer.models import PlatformPost @@ -293,25 +294,27 @@ def account_analytics_bundle(account: SocialAccount, days: int) -> dict[str, Any start = end - timedelta(days=2 * days - 1) platform_metrics = PLATFORM_METRICS.get(account.platform, []) - rows = list( - AccountInsightsSnapshot.objects.filter( - social_account=account, - metric_key__in=platform_metrics, - date__gte=start, - date__lte=end, - ) - ) + # Four columns, not model instances: AccountInsightsSnapshot carries ``raw`` + # and ``errors`` JSONFields that Django decodes on hydration and that + # nothing below reads. See ``_latest_post_stats`` for the same reasoning at + # the scale where it actually hurt. + rows = AccountInsightsSnapshot.objects.filter( + social_account=account, + metric_key__in=platform_metrics, + date__gte=start, + date__lte=end, + ).values_list("metric_key", "date", "value", "captured_at") by_metric: dict[str, dict[dt_date, float]] = defaultdict(dict) captured_by_metric: dict[str, Any] = {} max_captured: Any = None metrics_with_account_data: set[str] = set() - for r in rows: - by_metric[r.metric_key][r.date] = r.value - metrics_with_account_data.add(r.metric_key) - if r.metric_key not in captured_by_metric or r.captured_at > captured_by_metric[r.metric_key]: - captured_by_metric[r.metric_key] = r.captured_at - if max_captured is None or r.captured_at > max_captured: - max_captured = r.captured_at + for metric_key, day, value, captured_at in rows: + by_metric[metric_key][day] = value + metrics_with_account_data.add(metric_key) + if metric_key not in captured_by_metric or captured_at > captured_by_metric[metric_key]: + captured_by_metric[metric_key] = captured_at + if max_captured is None or captured_at > max_captured: + max_captured = captured_at # Hybrid fallback: for content-attribution metrics, derive a daily series # by summing per-post deltas so platforms without ``get_account_metrics`` @@ -566,7 +569,7 @@ def all_posts_for( posts: list[PlatformPost] = list(qs) metrics = post_metrics_for(account.platform) - stats_by_post = _latest_post_stats(posts, metrics) + stats_by_post = _latest_post_stats([p.id for p in posts], metrics) rows: list[dict[str, Any]] = [] for p in posts: @@ -633,7 +636,7 @@ def post_detail(post: PlatformPost) -> dict[str, Any]: """ account = post.social_account metrics = post_metrics_for(account.platform) - stats = _latest_post_stats([post], metrics).get(post.id, {}) + stats = _latest_post_stats([post.id], metrics).get(post.id, {}) sparklines_by_metric, max_captured = _post_sparklines_with_freshness(post, metrics) return { "post": post, @@ -667,46 +670,75 @@ def _label(metric_key: str) -> str: return METRICS.get(metric_key, {}).get("label", metric_key.replace("_", " ").title()) -def _latest_post_stats(posts: Iterable[PlatformPost], metrics: list[str]) -> dict[Any, dict[str, float]]: - """For each post, return ``{metric_key: latest value}``.""" - post_ids = [p.id for p in posts] +def _latest_post_stats(post_ids: Iterable[Any], metrics: list[str]) -> dict[Any, dict[str, float]]: + """For each post id, return ``{metric_key: latest value}``. + + Three columns, never model instances. ``PostInsightsSnapshot`` is one row + per (post, metric, day) and carries two JSONFields — ``raw`` is the entire + provider response — and Django decodes both eagerly while hydrating a row. + Pulling these as models therefore ran a ``json.loads`` over every payload + in the table for the account, twice per row, to read three numbers none of + which are in the JSON. On an account with 300 posts and 90 days of history + that is 216k instances and 432k needless decodes, in a web process with + ~60 MB of headroom. + + Where the backend supports it, ``DISTINCT ON`` also does the dedup in + Postgres rather than in Python, so the query returns one row per + (post, metric) instead of one per day. The ``order_by`` prefix it requires + is the ordering this needs anyway. + + That is an optimization, not a requirement: README documents SQLite for + local development and small deployments, and SQLite inherits Django's base + ``distinct_sql``, which raises ``NotSupportedError`` the moment any field is + passed. So the clause is applied only when the backend advertises it, and + everything else dedups the same rows in Python. The ``values_list`` above + is where nearly all of the saving comes from and it works everywhere. + """ + post_ids = list(post_ids) if not post_ids: return {} - rows = PostInsightsSnapshot.objects.filter(platform_post_id__in=post_ids, metric_key__in=metrics).order_by( - "platform_post_id", "metric_key", "-date" + rows = ( + PostInsightsSnapshot.objects.filter(platform_post_id__in=post_ids, metric_key__in=metrics) + .order_by("platform_post_id", "metric_key", "-date") + .values_list("platform_post_id", "metric_key", "value") ) + out: dict[Any, dict[str, float]] = defaultdict(dict) + if connections[rows.db].features.can_distinct_on_fields: + for post_id, metric_key, value in rows.distinct("platform_post_id", "metric_key"): + out[post_id][metric_key] = value + return out + + # Same ordering, so the first row for each (post, metric) is still the + # newest; ``iterator`` keeps the untrimmed result set from being cached. seen: set[tuple[Any, str]] = set() - for r in rows: - key = (r.platform_post_id, r.metric_key) + for post_id, metric_key, value in rows.iterator(chunk_size=2000): + key = (post_id, metric_key) if key in seen: continue seen.add(key) - out[r.platform_post_id][r.metric_key] = r.value + out[post_id][metric_key] = value return out -def _post_sparklines(post: PlatformPost, metrics: list[str]) -> dict[str, list[float]]: - """Daily history per metric since publish — for the detail-drawer sparkline.""" - return _post_sparklines_with_freshness(post, metrics)[0] - - def _post_sparklines_with_freshness(post: PlatformPost, metrics: list[str]) -> tuple[dict[str, list[float]], Any]: - """Same as :func:`_post_sparklines` but also returns the max ``captured_at``. + """Daily history per metric since publish, plus the max ``captured_at``. - Used by :func:`post_detail` so the freshness side-channel - (:func:`apps.analytics.freshness.post_freshness`) doesn't need its own - ``Max("captured_at")`` aggregate against the same rows. + Feeds the detail-drawer sparkline. The freshness value rides along so the + side-channel (:func:`apps.analytics.freshness.post_freshness`) doesn't need + its own ``Max("captured_at")`` aggregate against the same rows. """ - rows = PostInsightsSnapshot.objects.filter(platform_post=post, metric_key__in=metrics).order_by( - "metric_key", "date" + rows = ( + PostInsightsSnapshot.objects.filter(platform_post=post, metric_key__in=metrics) + .order_by("metric_key", "date") + .values_list("metric_key", "value", "captured_at") ) out: dict[str, list[float]] = defaultdict(list) max_captured: Any = None - for r in rows: - out[r.metric_key].append(r.value) - if max_captured is None or r.captured_at > max_captured: - max_captured = r.captured_at + for metric_key, value, captured_at in rows: + out[metric_key].append(value) + if max_captured is None or captured_at > max_captured: + max_captured = captured_at return dict(out), max_captured diff --git a/apps/analytics/tests/test_services.py b/apps/analytics/tests/test_services.py index 6a2d6161..cc24dc0f 100644 --- a/apps/analytics/tests/test_services.py +++ b/apps/analytics/tests/test_services.py @@ -99,3 +99,157 @@ def test_account_bundle_keeps_account_reach_instead_of_summing_post_reach(facebo series = account_analytics_bundle(facebook_account, 7)["series_map"]["reach"] assert series[-1] == 10 + + +@pytest.mark.django_db +def test_latest_post_stats_takes_the_newest_row_per_metric(facebook_account): + """The dedup moved from a Python ``seen`` set into Postgres ``DISTINCT ON``. + + The ordering prefix is what makes that correct — ``date DESC`` inside the + ORDER BY is the only reason the newest row survives — so pin it here rather + than trusting the clause to keep meaning what it means. + """ + from apps.analytics.models import PostInsightsSnapshot + from apps.analytics.services import _latest_post_stats + + post = _published_platform_post(facebook_account) + today = timezone.now().date() + for offset, value in ((2, 10.0), (1, 20.0), (0, 30.0)): + PostInsightsSnapshot.objects.create( + platform_post=post, + metric_key="likes", + date=today - timedelta(days=offset), + value=value, + ) + + assert _latest_post_stats([post.id], ["likes"]) == {post.id: {"likes": 30.0}} + + +@pytest.mark.django_db +def test_latest_post_stats_keeps_metrics_and_posts_separate(facebook_account): + from apps.analytics.models import PostInsightsSnapshot + from apps.analytics.services import _latest_post_stats + + first = _published_platform_post(facebook_account) + second = _published_platform_post(facebook_account) + today = timezone.now().date() + for post, metric, value in ( + (first, "likes", 1.0), + (first, "comments", 2.0), + (second, "likes", 3.0), + ): + PostInsightsSnapshot.objects.create(platform_post=post, metric_key=metric, date=today, value=value) + + assert _latest_post_stats([first.id, second.id], ["likes", "comments"]) == { + first.id: {"likes": 1.0, "comments": 2.0}, + second.id: {"likes": 3.0}, + } + + +@pytest.mark.django_db +def test_latest_post_stats_ignores_metrics_not_asked_for(facebook_account): + from apps.analytics.models import PostInsightsSnapshot + from apps.analytics.services import _latest_post_stats + + post = _published_platform_post(facebook_account) + today = timezone.now().date() + PostInsightsSnapshot.objects.create(platform_post=post, metric_key="likes", date=today, value=1.0) + PostInsightsSnapshot.objects.create(platform_post=post, metric_key="shares", date=today, value=9.0) + + assert _latest_post_stats([post.id], ["likes"]) == {post.id: {"likes": 1.0}} + + +@pytest.mark.django_db +def test_latest_post_stats_short_circuits_on_no_posts(django_assert_num_queries): + from apps.analytics.services import _latest_post_stats + + with django_assert_num_queries(0): + assert _latest_post_stats([], ["likes"]) == {} + + +@pytest.mark.django_db +def test_latest_post_stats_runs_one_query_regardless_of_history(facebook_account, django_assert_num_queries): + """No JSON is decoded and no model is hydrated: three columns, one query. + + ``PostInsightsSnapshot`` carries two JSONFields — ``raw`` is the whole + provider response — that Django decodes eagerly on hydration. Reading these + as models ran a ``json.loads`` per payload per row to fetch three numbers + that are not in the JSON, which is what pushed the web dyno to 88% of a + 512 MB quota. + """ + from apps.analytics.models import PostInsightsSnapshot + from apps.analytics.services import _latest_post_stats + + post = _published_platform_post(facebook_account) + today = timezone.now().date() + bulky = {"payload": "x" * 2000} + for offset in range(30): + PostInsightsSnapshot.objects.create( + platform_post=post, + metric_key="likes", + date=today - timedelta(days=offset), + value=float(offset), + raw=bulky, + ) + + with django_assert_num_queries(1): + result = _latest_post_stats([post.id], ["likes"]) + assert result == {post.id: {"likes": 0.0}} + + +@pytest.mark.django_db +def test_latest_post_stats_dedups_without_distinct_on(facebook_account, monkeypatch): + """SQLite has no DISTINCT ON, and the README supports SQLite deployments. + + Django's base ``distinct_sql`` raises ``NotSupportedError`` as soon as a + field is passed, so the whole analytics surface 500s on SQLite if the + clause is applied unconditionally. Force the fallback and assert it picks + the same rows. + """ + from django.db import connections + + from apps.analytics.models import PostInsightsSnapshot + from apps.analytics.services import _latest_post_stats + + post = _published_platform_post(facebook_account) + today = timezone.now().date() + for offset, value in ((2, 10.0), (1, 20.0), (0, 30.0)): + PostInsightsSnapshot.objects.create( + platform_post=post, + metric_key="likes", + date=today - timedelta(days=offset), + value=value, + ) + PostInsightsSnapshot.objects.create(platform_post=post, metric_key="comments", date=today, value=7.0) + + monkeypatch.setattr(connections["default"].features, "can_distinct_on_fields", False, raising=False) + + assert _latest_post_stats([post.id], ["likes", "comments"]) == {post.id: {"likes": 30.0, "comments": 7.0}} + + +@pytest.mark.django_db +def test_latest_post_stats_agrees_across_both_dedup_paths(facebook_account, monkeypatch): + """The two branches must be interchangeable, not merely both plausible.""" + from django.db import connections + + from apps.analytics.models import PostInsightsSnapshot + from apps.analytics.services import _latest_post_stats + + first = _published_platform_post(facebook_account) + second = _published_platform_post(facebook_account) + today = timezone.now().date() + for post in (first, second): + for offset, metric in ((0, "likes"), (3, "likes"), (1, "comments")): + PostInsightsSnapshot.objects.create( + platform_post=post, + metric_key=metric, + date=today - timedelta(days=offset), + value=float(offset), + ) + + ids = [first.id, second.id] + with_distinct_on = _latest_post_stats(ids, ["likes", "comments"]) + monkeypatch.setattr(connections["default"].features, "can_distinct_on_fields", False, raising=False) + without_distinct_on = _latest_post_stats(ids, ["likes", "comments"]) + + assert with_distinct_on == without_distinct_on diff --git a/apps/media_library/services.py b/apps/media_library/services.py index 9412d071..75e420d0 100644 --- a/apps/media_library/services.py +++ b/apps/media_library/services.py @@ -1,5 +1,6 @@ """Business logic for media library operations.""" +import contextlib import io import logging import os @@ -365,17 +366,93 @@ def _check_post_references(asset): return [{"id": str(ref.post_id), "caption": (ref.post.caption or "")[:80]} for ref in scheduled_refs] -def extract_image_metadata(file_path_or_file): - """Extract dimensions from an image file using Pillow.""" +class ImageTooLargeError(Exception): + """An image would cost more memory to decode than we are willing to spend.""" + + +@contextlib.contextmanager +def open_image(file_path_or_file, *, draft_size=None, enforce_limit=True): + """Open an image, bound its decode cost, and close it afterwards. + + Every Pillow entry point in this module goes through here, because on a + 512 MB dyno the decode — not the file — is what kills the process. A 20 MB + upload (``MEDIA_LIBRARY_MAX_IMAGE_SIZE``) says nothing about pixel count, + and Pillow's own bomb check only *warns* between 1x and 2x its + ``MAX_IMAGE_PIXELS``, so its default leaves a window that decodes to over + 500 MB. + + ``Image.open`` reads the header only, so both the draft and the guard run + before a single pixel is decoded. + + ``draft_size`` is the size the caller ultimately wants, and asks the JPEG + decoder to downscale during the read (DCT scaling at 1/2, 1/4, 1/8). An + 8000x6000 JPEG then decodes at roughly 1000x750, and the guard sees that + reduced size — the honest measure of what the file costs us. We draft to + twice the requested size, which is what ``Image.thumbnail`` does with its + default ``reducing_gap=2.0``, so the later resample has the same source + data to work with and output quality is unchanged. (``thumbnail`` calls + ``draft`` itself; the second call is a documented no-op, guarded by + ``if self.decoderconfig: return None``.) + + ``draft`` is defined on JpegImageFile only and documented as a no-op + elsewhere, so PNG/WebP/GIF fall through to a full-size check — which is + right, because those genuinely do decode full-size. + + ``enforce_limit=False`` opens without the ceiling, for callers that only + read header attributes. ``Image.open`` decodes nothing, so reading + ``img.size`` off a 500-megapixel file is free — refusing to answer would + just mean storing 0x0 for an image whose dimensions we are holding. + + Raises ``ImageTooLargeError`` rather than returning None: the caller has to + tell "this file is too big" from "Pillow could not read this", and the + media asset needs a real reason to show the user. + """ + from PIL import Image + + max_pixels = getattr(settings, "MEDIA_LIBRARY_MAX_IMAGE_PIXELS", 30_000_000) + # Deliberately NOT assigning Image.MAX_IMAGE_PIXELS from that number. + # Pillow evaluates its own ceiling inside Image.open(), before draft() has + # had a chance to reduce anything, so pinning it here would reject large + # JPEGs on their header dimensions — exactly the files draft() makes cheap. + # Pillow's default stays as the outer backstop; the check below is the + # operative limit, and it runs on the drafted size. + + if hasattr(file_path_or_file, "seek"): + file_path_or_file.seek(0) + try: - from PIL import Image + opened = Image.open(file_path_or_file) + except Image.DecompressionBombError as exc: + # Pillow's own backstop fired first (above 2x MAX_IMAGE_PIXELS). Speak + # with one voice so callers only have to handle our exception. + raise ImageTooLargeError(str(exc)) from exc + + with opened as img: + if draft_size is not None: + img.draft("RGB", (draft_size[0] * 2, draft_size[1] * 2)) + pixels = img.width * img.height + if enforce_limit and pixels > max_pixels: + raise ImageTooLargeError( + f"Image is {img.width}x{img.height} ({pixels / 1_000_000:.1f} megapixels); " + f"the limit is {max_pixels / 1_000_000:g} megapixels." + ) + yield img - if hasattr(file_path_or_file, "read"): - file_path_or_file.seek(0) - img = Image.open(file_path_or_file) - else: - img = Image.open(file_path_or_file) - width, height = img.size + +def extract_image_metadata(file_path_or_file): + """Extract dimensions from an image file using Pillow. + + Deliberately exempt from the pixel ceiling, and not drafted. Reading + ``img.size`` decodes nothing, so there is no memory to save by refusing — + and refusing would be actively wrong, since the width and height we would + withhold are the very numbers the guard just read to make its decision. + Enforcing here stored 0x0 on assets whose thumbnails generated fine, because + the thumbnail path drafts the image down and this one does not, so the two + judged the same file against different pixel counts. + """ + try: + with open_image(file_path_or_file, enforce_limit=False) as img: + width, height = img.size return {"width": width, "height": height} except Exception: logger.exception("Failed to extract image metadata") @@ -383,34 +460,49 @@ def extract_image_metadata(file_path_or_file): def generate_image_thumbnail(file_path_or_file): - """Generate a thumbnail from an image file using Pillow.""" + """Generate a thumbnail from an image file using Pillow. + + Order matters here, and it used to be wrong. Flattening alpha or converting + a colour space BEFORE the resize does that work at full resolution — a + white background allocated at the source's size, then pasted onto, all to + produce a 400x400 JPEG. Shrinking first makes both operations free. + Measured on a 6000x4000 RGBA PNG: peak RSS +196 MB before, +104 MB after. + + It also keeps ``thumbnail()``'s own internal ``draft()`` call effective. + Converting first replaces the JpegImageFile with a plain Image, and the DCT + downscale is lost with it — which is what made CMYK JPEGs so expensive. + """ try: from PIL import Image thumb_size = getattr(settings, "MEDIA_LIBRARY_THUMBNAIL_SIZE", (400, 400)) - if hasattr(file_path_or_file, "read"): - file_path_or_file.seek(0) - img = Image.open(file_path_or_file) - else: - img = Image.open(file_path_or_file) - - # Convert to RGB if necessary (e.g., RGBA PNGs, CMYK) - if img.mode in ("RGBA", "LA", "P"): - background = Image.new("RGB", img.size, (255, 255, 255)) - if img.mode == "P": - img = img.convert("RGBA") - background.paste(img, mask=img.split()[-1] if "A" in img.mode else None) - img = background - elif img.mode != "RGB": - img = img.convert("RGB") - - img.thumbnail(thumb_size, Image.LANCZOS) - - buffer = io.BytesIO() - img.save(buffer, format="JPEG", quality=85) - buffer.seek(0) - return ContentFile(buffer.read(), name="thumbnail.jpg") + with open_image(file_path_or_file, draft_size=thumb_size) as src: + # Palette images resample badly — Pillow forces NEAREST on mode "P" + # — so promote to RGBA first. That is the one conversion worth + # doing at full size, and it is 4 bytes/px against the 8 the old + # order cost. + img = src.convert("RGBA") if src.mode == "P" else src + + img.thumbnail(thumb_size, Image.LANCZOS) + + # Now at thumbnail size, so flattening and converting are free. + if img.mode in ("RGBA", "LA"): + background = Image.new("RGB", img.size, (255, 255, 255)) + background.paste(img, mask=img.split()[-1]) + img = background + elif img.mode != "RGB": + img = img.convert("RGB") + + buffer = io.BytesIO() + img.save(buffer, format="JPEG", quality=85) + return ContentFile(buffer.getvalue(), name="thumbnail.jpg") + except ImageTooLargeError: + # Propagates on purpose, unlike every other failure here. "Too large" is + # determinate and worth telling the user about, so the caller can fail + # the asset; returning None would mark it COMPLETED with no thumbnail, + # no dimensions and nothing on screen to explain either. + raise except Exception: logger.exception("Failed to generate image thumbnail") return None @@ -563,52 +655,55 @@ def apply_image_edits(file_path_or_file, operations): """ from PIL import Image - if hasattr(file_path_or_file, "read"): - file_path_or_file.seek(0) - img = Image.open(file_path_or_file) - else: - img = Image.open(file_path_or_file) - - # Apply crop - crop = operations.get("crop") - if crop: - left = int(crop["x"]) - top = int(crop["y"]) - right = left + int(crop["width"]) - bottom = top + int(crop["height"]) - img = img.crop((left, top, right, bottom)) - - # Apply rotation - rotate = operations.get("rotate") - if rotate: - img = img.rotate(-int(rotate), expand=True) - - # Apply flip - flip = operations.get("flip") - if flip == "horizontal": - img = img.transpose(Image.FLIP_LEFT_RIGHT) - elif flip == "vertical": - img = img.transpose(Image.FLIP_TOP_BOTTOM) - - # Apply resize - resize = operations.get("resize") - if resize: - img = img.resize((int(resize["width"]), int(resize["height"])), Image.LANCZOS) - - # Save to buffer - if img.mode in ("RGBA", "LA", "P"): - format_str = "PNG" - ext = "png" - else: - if img.mode != "RGB": - img = img.convert("RGB") - format_str = "JPEG" - ext = "jpg" - - buffer = io.BytesIO() - img.save(buffer, format=format_str, quality=90) - buffer.seek(0) - return ContentFile(buffer.read(), name=f"edited.{ext}"), img.size + # No draft here: crop and rotate operate on the original pixels, so this + # path genuinely needs full resolution and the guard's full-size check is + # the right one. ``ImageTooLargeError`` propagates to the caller, which + # fails the version rather than taking the dyno down with it. + with open_image(file_path_or_file) as src: + # Crop first when we have one — every later step then works on a + # smaller buffer. + crop = operations.get("crop") + if crop: + left = int(crop["x"]) + top = int(crop["y"]) + right = left + int(crop["width"]) + bottom = top + int(crop["height"]) + img = src.crop((left, top, right, bottom)) + else: + img = src + + # Apply rotation + rotate = operations.get("rotate") + if rotate: + img = img.rotate(-int(rotate), expand=True) + + # Apply flip + flip = operations.get("flip") + if flip == "horizontal": + img = img.transpose(Image.FLIP_LEFT_RIGHT) + elif flip == "vertical": + img = img.transpose(Image.FLIP_TOP_BOTTOM) + + # Apply resize + resize = operations.get("resize") + if resize: + img = img.resize((int(resize["width"]), int(resize["height"])), Image.LANCZOS) + + # Save to buffer. Inside the ``with`` on purpose: when ``operations`` is + # empty, ``img`` IS the opened image, and leaving the block closes its + # file pointer before it has ever been loaded. + if img.mode in ("RGBA", "LA", "P"): + format_str = "PNG" + ext = "png" + else: + if img.mode != "RGB": + img = img.convert("RGB") + format_str = "JPEG" + ext = "jpg" + + buffer = io.BytesIO() + img.save(buffer, format=format_str, quality=90) + return ContentFile(buffer.getvalue(), name=f"edited.{ext}"), img.size def trim_video(input_path, output_path, start_seconds, end_seconds): diff --git a/apps/media_library/storage.py b/apps/media_library/storage.py index 6777d9e1..5990ee65 100644 --- a/apps/media_library/storage.py +++ b/apps/media_library/storage.py @@ -11,6 +11,7 @@ from __future__ import annotations import shutil +import threading import uuid from django.conf import settings @@ -23,6 +24,10 @@ # match boto3's own transfer chunk so both branches behave alike. _COPY_CHUNK_SIZE = 1024 * 1024 +# One boto3 client for the whole process. See ``_client_and_bucket``. +_client_lock = threading.Lock() +_cached_client = None + def is_s3_backend() -> bool: """True when ``default_storage`` is the S3/R2 backend (presigning works). @@ -67,8 +72,56 @@ def supports_presigned_post() -> bool: def _client_and_bucket(): - """Return ``(boto3_client, bucket_name)`` for the configured S3/R2 bucket.""" - return default_storage.connection.meta.client, default_storage.bucket_name + """Return ``(boto3_client, bucket_name)`` for the configured S3/R2 bucket. + + The client is memoized for the life of the process, which is the whole + point of this function. ``default_storage.connection`` is backed by a + ``threading.local()`` in django-storages, so reaching through it builds a + fresh ``boto3.Session()`` and ``session.resource()`` on *every thread* that + touches storage — and a fresh session means botocore re-parsing the S3 + service model and endpoint ruleset, several MB of Python dicts each time. + The publisher creates a new ThreadPoolExecutor every 15-second cycle + (``apps.publisher.engine``) and boto3's own managed transfer adds ten more + threads per download, so the worker was building and discarding those + clients all day into an allocator that never hands the pages back. + + Safe to share: boto3 *clients* are documented thread-safe (resources are + not), and every caller here uses client methods only. Credential refresh is + handled inside the client. + """ + global _cached_client + + bucket = default_storage.bucket_name + client = _cached_client + if client is None: + with _client_lock: + # Re-check under the lock: two threads can race the None test. + if _cached_client is None: + _cached_client = default_storage.connection.meta.client + client = _cached_client + return client, bucket + + +def reset_cached_client() -> None: + """Drop the memoized client so the next call rebuilds it. + + For tests: without it the first one to touch S3 pins its client for the + rest of the run and every later ``override_settings`` on a bucket, endpoint + or credential is silently ignored. An autouse fixture in ``conftest.py`` + calls this between tests. + + Deliberately not wired to the ``setting_changed`` signal. That put a + global receiver in every web and worker process to serve a concern that + only exists under ``override_settings``, matched setting names by string + prefix (so a new storage setting would silently stop resetting), and could + fire *during* a call — ``_client_and_bucket`` reads the cache before taking + the lock, so a concurrent reset could be missed and a stale client + returned. Called explicitly between tests, none of that applies. + """ + global _cached_client + + with _client_lock: + _cached_client = None def _normalize(storage_key: str) -> str: diff --git a/apps/media_library/tasks.py b/apps/media_library/tasks.py index 9938188f..d320c50c 100644 --- a/apps/media_library/tasks.py +++ b/apps/media_library/tasks.py @@ -8,6 +8,7 @@ from .models import MediaAsset, MediaAssetVersion from .services import ( + ImageTooLargeError, apply_image_edits, extract_image_metadata, extract_video_metadata, @@ -39,6 +40,14 @@ def process_media_asset(asset_id): _process_video(asset) asset.processing_status = MediaAsset.ProcessingStatus.COMPLETED asset.save(update_fields=["processing_status", "width", "height", "duration", "thumbnail", "updated_at"]) + except ImageTooLargeError as exc: + # Determinate, and the user can act on it, so say so at WARNING with the + # dimensions rather than dumping a traceback. Still FAILED — an asset we + # cannot thumbnail is not a processed asset. Keep the dimensions we did + # read, so the library can show what was wrong with it. + logger.warning("Media asset %s rejected: %s", asset_id, exc) + asset.processing_status = MediaAsset.ProcessingStatus.FAILED + asset.save(update_fields=["processing_status", "width", "height", "updated_at"]) except Exception: logger.exception("Failed to process media asset %s", asset_id) asset.processing_status = MediaAsset.ProcessingStatus.FAILED @@ -46,7 +55,13 @@ def process_media_asset(asset_id): def _process_image(asset): - """Extract metadata and generate thumbnail for an image.""" + """Extract metadata and generate thumbnail for an image. + + ``ImageTooLargeError`` is allowed to propagate to ``process_media_asset``, + which marks the asset FAILED. Swallowing it left the asset COMPLETED with + 0x0 dimensions and no thumbnail, which reads as "this worked" everywhere + in the UI. + """ metadata = extract_image_metadata(asset.file) asset.width = metadata.get("width", 0) asset.height = metadata.get("height", 0) @@ -87,6 +102,13 @@ def process_image_edit(version_id, operations): logger.warning("MediaAssetVersion %s not found", version_id) return + # ``create_version`` seeds the row by assigning the asset's own FieldFile, + # which copies the NAME rather than the bytes — until the edit is written, + # version.file and asset.file are the same stored object. Remember it so the + # cleanup below can tell "a file this task generated" from "the shared + # source", and never delete the latter out from under the asset. + source_name = version.file.name + try: edited_file, (width, height) = apply_image_edits(version.media_asset.file, operations) @@ -107,6 +129,34 @@ def process_image_edit(version_id, operations): asset.thumbnail = version.thumbnail asset.save(update_fields=["width", "height", "thumbnail", "updated_at"]) + except ImageTooLargeError as exc: + # The version row exists only to hold the edit result — ``create_version`` + # seeds it with a copy of the source file — so a deterministic failure + # would otherwise leave a version that looks like an unchanged duplicate + # and will never become anything else. Retrying cannot help: the image is + # the size it is. + # + # ``create_version`` also pointed the asset at this row, and the FK is + # SET_NULL, so rewind to the version it superseded first. Deleting + # without that would leave an edited asset with no current version at + # all, which is a worse state than the one we are cleaning up. + logger.warning("Image edit for version %s rejected: %s", version_id, exc) + asset = version.media_asset + + # Django does not delete FileField objects when a row goes, so dropping + # the version without this strands whatever was already written. It is + # reachable: ``apply_image_edits`` checks the SOURCE size, so an + # upscaling resize can succeed and then produce output too large to + # thumbnail, by which point version.file is already saved. + if version.thumbnail: + version.thumbnail.delete(save=False) + if version.file and version.file.name != source_name: + version.file.delete(save=False) + + previous = asset.versions.exclude(pk=version.pk).order_by("-version_number").first() + version.delete() + asset.current_version = previous + asset.save(update_fields=["current_version", "updated_at"]) except Exception: logger.exception("Failed to process image edit for version %s", version_id) diff --git a/apps/media_library/tests/test_asset_processing.py b/apps/media_library/tests/test_asset_processing.py new file mode 100644 index 00000000..99b638a8 --- /dev/null +++ b/apps/media_library/tests/test_asset_processing.py @@ -0,0 +1,240 @@ +"""End-to-end coverage for ``process_media_asset``. + +The unit tests around the Pillow helpers pass happily while the task built on +them does the wrong thing: an over-limit image used to be saved COMPLETED with +0x0 dimensions and no thumbnail, because both helpers swallowed the failure. +Nothing composed the two calls the way ``_process_image`` does, so nothing +caught it. These tests assert on the asset row, which is what the UI reads. +""" + +import io + +import pytest +from django.core.files.base import ContentFile +from django.test import override_settings +from PIL import Image + +from apps.media_library.models import MediaAsset, MediaAssetVersion +from apps.media_library.tasks import process_media_asset + + +def _stored_names_containing(fragment): + """Every stored file under the media library whose name contains ``fragment``. + + Walks rather than guessing a path: the versions directory is date-stamped + and the storage backend may add a uniqueness suffix. + """ + from django.core.files.storage import default_storage + + found = [] + pending = ["media_library"] + while pending: + current = pending.pop() + try: + dirs, files = default_storage.listdir(current) + except (FileNotFoundError, OSError): + continue + pending.extend(f"{current}/{d}" for d in dirs) + found.extend(f for f in files if fragment in f) + return found + + +def _png_bytes(width, height, mode="RGBA"): + buf = io.BytesIO() + Image.new(mode, (width, height), (10, 200, 90, 128)[: 4 if mode == "RGBA" else 3]).save(buf, format="PNG") + return buf.getvalue() + + +def _jpeg_bytes(width, height): + buf = io.BytesIO() + Image.new("RGB", (width, height), (100, 50, 25)).save(buf, format="JPEG") + return buf.getvalue() + + +@pytest.fixture +def asset(db, organization, user): + from apps.workspaces.models import Workspace + + workspace = Workspace.objects.create(name="Media WS", organization=organization) + + def _make(data, filename="image.png"): + media = MediaAsset.objects.create( + organization=organization, + workspace=workspace, + uploaded_by=user, + filename=filename, + media_type=MediaAsset.MediaType.IMAGE, + file_size=len(data), + ) + media.file.save(filename, ContentFile(data), save=True) + return media + + return _make + + +@pytest.mark.django_db +def test_normal_image_completes_with_dimensions_and_thumbnail(asset): + media = asset(_png_bytes(600, 400)) + + process_media_asset.now(str(media.id)) + + media.refresh_from_db() + assert media.processing_status == MediaAsset.ProcessingStatus.COMPLETED + assert (media.width, media.height) == (600, 400) + assert media.thumbnail + + +@pytest.mark.django_db +def test_over_limit_image_is_marked_failed_not_completed(asset): + """The bug: this used to land in COMPLETED with nothing in it.""" + media = asset(_png_bytes(600, 400)) + + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000): + process_media_asset.now(str(media.id)) + + media.refresh_from_db() + assert media.processing_status == MediaAsset.ProcessingStatus.FAILED + assert not media.thumbnail + + +@pytest.mark.django_db +def test_over_limit_image_still_records_the_dimensions_that_caused_it(asset): + """Keeping these is what lets the library explain the rejection.""" + media = asset(_png_bytes(600, 400)) + + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000): + process_media_asset.now(str(media.id)) + + media.refresh_from_db() + assert (media.width, media.height) == (600, 400) + + +@pytest.mark.django_db +def test_large_jpeg_completes_with_real_dimensions_and_a_thumbnail(asset): + """A 40MP JPEG drafts down cheaply, so it must not be treated as over-limit. + + Regression: metadata judged the header size while the thumbnail judged the + drafted size, so this combination produced a working thumbnail alongside + width=0, height=0. + """ + media = asset(_jpeg_bytes(8000, 5000), filename="big.jpg") + + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=30_000_000): + process_media_asset.now(str(media.id)) + + media.refresh_from_db() + assert media.processing_status == MediaAsset.ProcessingStatus.COMPLETED + assert (media.width, media.height) == (8000, 5000) + assert media.thumbnail + + +@pytest.mark.django_db +def test_unreadable_file_completes_without_a_thumbnail(asset): + """Unreadable stays a soft failure, unlike over-limit. + + Only "too large" propagates. "Pillow could not read this" keeps the + pre-existing best-effort behaviour: the asset completes with no thumbnail + and zero dimensions. Pinned here so the distinction stays deliberate. + """ + media = asset(b"this is not an image", filename="broken.png") + + process_media_asset.now(str(media.id)) + + media.refresh_from_db() + assert media.processing_status == MediaAsset.ProcessingStatus.COMPLETED + assert not media.thumbnail + assert (media.width, media.height) == (0, 0) + + +@pytest.mark.django_db +def test_rejected_edit_removes_the_version_and_rewinds_current(asset, user): + """A failed edit must not leave a version that will never materialize. + + ``create_version`` seeds the row with a copy of the source and points the + asset at it, so swallowing the failure left what looks like an unchanged + duplicate version. The asset's ``current_version`` FK is SET_NULL, so the + rewind matters: deleting without it strands the asset with no current + version at all. + """ + from apps.media_library.models import MediaAssetVersion + from apps.media_library.services import create_version + from apps.media_library.tasks import process_image_edit + + media = asset(_png_bytes(600, 400)) + first = create_version(asset=media, file=media.file, change_description="v1", created_by=user) + second = create_version(asset=media, file=media.file, change_description="v2", created_by=user) + + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000): + process_image_edit.now(str(second.id), {"rotate": 90}) + + media.refresh_from_db() + assert not MediaAssetVersion.objects.filter(pk=second.pk).exists() + assert media.current_version_id == first.pk + + +@pytest.mark.django_db +def test_rejected_edit_on_a_first_version_leaves_no_current(asset, user): + """Nothing to rewind to is a legitimate state, not an error.""" + from apps.media_library.models import MediaAssetVersion + from apps.media_library.services import create_version + from apps.media_library.tasks import process_image_edit + + media = asset(_png_bytes(600, 400)) + only = create_version(asset=media, file=media.file, change_description="v1", created_by=user) + + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000): + process_image_edit.now(str(only.id), {"rotate": 90}) + + media.refresh_from_db() + assert not MediaAssetVersion.objects.filter(pk=only.pk).exists() + assert media.current_version_id is None + + +@pytest.mark.django_db +def test_rejected_edit_deletes_the_file_it_generated(asset, user): + """Django does not delete FileField objects when a row goes. + + Reachable: ``apply_image_edits`` checks the SOURCE size, so an upscaling + resize succeeds and then produces output too large to thumbnail — by which + point ``version.file`` has already been written to storage. + """ + + from apps.media_library.services import create_version + from apps.media_library.tasks import process_image_edit + + media = asset(_png_bytes(400, 400)) + version = create_version(asset=media, file=media.file, change_description="v1", created_by=user) + version_id = str(version.id) + + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=30_000_000): + # Upscales past the ceiling: the edit itself succeeds and is written, + # then thumbnailing the result raises. + process_image_edit.now(version_id, {"resize": {"width": 7000, "height": 7000}}) + + assert not MediaAssetVersion.objects.filter(pk=version.pk).exists() + assert not _stored_names_containing(version_id), "edited file left behind in storage" + + +@pytest.mark.django_db +def test_rejected_edit_preserves_the_shared_source_file(asset, user): + """``create_version`` copies the asset's file NAME, not its bytes. + + Until the edit is written the version and the asset point at the same + stored object, so a naive cleanup would delete the asset's own file. + """ + from django.core.files.storage import default_storage + + from apps.media_library.services import create_version + from apps.media_library.tasks import process_image_edit + + media = asset(_png_bytes(600, 400)) + version = create_version(asset=media, file=media.file, change_description="v1", created_by=user) + assert version.file.name == media.file.name + + # Fails inside apply_image_edits, before anything is written. + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000): + process_image_edit.now(str(version.id), {"rotate": 90}) + + media.refresh_from_db() + assert media.file.name + assert default_storage.exists(media.file.name) diff --git a/apps/media_library/tests/test_image_processing.py b/apps/media_library/tests/test_image_processing.py new file mode 100644 index 00000000..2c218250 --- /dev/null +++ b/apps/media_library/tests/test_image_processing.py @@ -0,0 +1,288 @@ +"""Tests for the Pillow seam in ``apps.media_library.services``. + +These paths had no coverage at all, which is uncomfortable given what they do: +they are the only place the worker decodes attacker-supplied pixel data, and an +unbounded decode is what put the Heroku worker over its 512 MB quota. The cases +below pin the two properties that matter — the decode is bounded, and the +thumbnail still looks right for every format we accept. +""" + +import io + +from django.core.files.uploadedfile import SimpleUploadedFile +from django.test import SimpleTestCase, override_settings +from PIL import Image + +from apps.media_library.services import ( + ImageTooLargeError, + apply_image_edits, + extract_image_metadata, + generate_image_thumbnail, + open_image, +) +from apps.media_library.validators import validate_file + + +def _encode(img, fmt): + buf = io.BytesIO() + img.save(buf, format=fmt) + buf.seek(0) + return buf + + +def _jpeg(width=1200, height=800, mode="RGB"): + return _encode(Image.new(mode, (width, height), (120, 30, 200)), "JPEG") + + +def _alpha_png(width=600, height=400): + return _encode(Image.new("RGBA", (width, height), (10, 200, 90, 128)), "PNG") + + +def _palette_png(width=600, height=400): + return _encode(Image.new("RGB", (width, height), (200, 40, 40)).convert("P"), "PNG") + + +class OpenImageGuardTest(SimpleTestCase): + def test_rejects_an_image_over_the_pixel_limit(self): + # 600x400 = 240_000 px, so a limit just under it must reject. + with ( + override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000), + self.assertRaises(ImageTooLargeError), + open_image(_alpha_png()), + ): + pass + + def test_error_names_the_dimensions_and_the_limit(self): + with ( + override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000), + self.assertRaises(ImageTooLargeError) as ctx, + open_image(_alpha_png()), + ): + pass + message = str(ctx.exception) + assert "600x400" in message + assert "megapixels" in message + + def test_allows_an_image_under_the_limit(self): + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=1_000_000), open_image(_alpha_png()) as img: + assert img.size == (600, 400) + + def test_draft_lets_a_large_jpeg_through_on_its_reduced_size(self): + """A JPEG's header dimensions are not what it costs us to decode. + + The decoder downscales during the read, so judging a JPEG on its full + size would refuse a file that never allocates that much. 2400x1600 is + 3.84M px, but drafted for a 400x400 thumbnail it decodes far smaller. + """ + with ( + override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=1_000_000), + open_image(_jpeg(2400, 1600), draft_size=(400, 400)) as img, + ): + assert img.width * img.height <= 1_000_000 + + def test_a_png_of_the_same_size_is_rejected(self): + """PNG has no draft support, so it really does decode full-size.""" + with ( + override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=1_000_000), + self.assertRaises(ImageTooLargeError), + open_image(_alpha_png(2400, 1600), draft_size=(400, 400)), + ): + pass + + def test_does_not_close_a_caller_supplied_file_object(self): + """``_process_image`` opens the same FieldFile twice, in sequence.""" + handle = _alpha_png() + with open_image(handle): + pass + assert not handle.closed + with open_image(handle) as img: + assert img.size == (600, 400) + + +class GenerateImageThumbnailTest(SimpleTestCase): + def test_rgb_jpeg(self): + thumb = generate_image_thumbnail(_jpeg()) + assert thumb is not None + with Image.open(io.BytesIO(thumb.read())) as out: + assert out.format == "JPEG" + assert out.mode == "RGB" + assert max(out.size) <= 400 + + def test_alpha_png_is_flattened_onto_white(self): + thumb = generate_image_thumbnail(_alpha_png()) + assert thumb is not None + with Image.open(io.BytesIO(thumb.read())) as out: + assert out.mode == "RGB" + assert max(out.size) <= 400 + + def test_palette_png(self): + """Mode "P" is promoted to RGBA before the resize. + + Pillow forces NEAREST resampling on palette images, so skipping the + promotion visibly degrades the thumbnail. + """ + thumb = generate_image_thumbnail(_palette_png()) + assert thumb is not None + with Image.open(io.BytesIO(thumb.read())) as out: + assert out.mode == "RGB" + assert max(out.size) <= 400 + + def test_cmyk_jpeg(self): + source = _encode(Image.new("CMYK", (1200, 800), (10, 20, 30, 40)), "JPEG") + thumb = generate_image_thumbnail(source) + assert thumb is not None + with Image.open(io.BytesIO(thumb.read())) as out: + assert out.mode == "RGB" + + def test_preserves_aspect_ratio(self): + thumb = generate_image_thumbnail(_jpeg(1200, 400)) + with Image.open(io.BytesIO(thumb.read())) as out: + assert out.size == (400, 133) + + def test_raises_over_the_limit_rather_than_returning_none(self): + """Returning None marked the asset COMPLETED with nothing in it. + + "Too large" is determinate and the user can act on it, so it has to + reach ``process_media_asset``. Only "Pillow could not read this" stays + a soft None. + """ + with ( + override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000), + self.assertRaises(ImageTooLargeError), + ): + generate_image_thumbnail(_alpha_png()) + + def test_returns_none_on_a_file_that_is_not_an_image(self): + assert generate_image_thumbnail(io.BytesIO(b"not an image at all")) is None + + +class ExtractImageMetadataTest(SimpleTestCase): + def test_reports_real_dimensions_not_drafted_ones(self): + assert extract_image_metadata(_jpeg(2400, 1600)) == {"width": 2400, "height": 1600} + + def test_reports_dimensions_even_over_the_limit(self): + """Reading the header decodes nothing, so there is nothing to protect. + + Withholding these stored 0x0 on assets whose thumbnails generated + perfectly well, because the thumbnail path drafts and this one does not. + """ + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000): + assert extract_image_metadata(_alpha_png()) == {"width": 600, "height": 400} + + def test_agrees_with_the_thumbnail_path_on_a_large_jpeg(self): + """The two must not judge the same file against different pixel counts. + + A 40MP JPEG drafts down to something cheap, so the thumbnail succeeds; + metadata must not meanwhile decide the file is unusable and report 0x0. + """ + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=30_000_000): + assert extract_image_metadata(_jpeg(8000, 5000)) == {"width": 8000, "height": 5000} + assert generate_image_thumbnail(_jpeg(8000, 5000)) is not None + + def test_returns_empty_on_unreadable_input(self): + assert extract_image_metadata(io.BytesIO(b"nope")) == {} + + +class ApplyImageEditsTest(SimpleTestCase): + def test_no_operations_still_returns_a_file(self): + """Regression: the save used to sit outside the open context. + + With no operations the working image IS the opened one, so leaving the + context first closed the file pointer out from under ``save()``. + """ + edited, size = apply_image_edits(_jpeg(800, 600), {}) + assert size == (800, 600) + assert edited.size > 0 + + def test_crop(self): + edited, size = apply_image_edits(_jpeg(800, 600), {"crop": {"x": 10, "y": 20, "width": 100, "height": 50}}) + assert size == (100, 50) + with Image.open(io.BytesIO(edited.read())) as out: + assert out.size == (100, 50) + + def test_rotate_expands(self): + _, size = apply_image_edits(_jpeg(800, 600), {"rotate": 90}) + assert size == (600, 800) + + def test_resize(self): + _, size = apply_image_edits(_jpeg(800, 600), {"resize": {"width": 320, "height": 240}}) + assert size == (320, 240) + + def test_alpha_source_is_written_as_png(self): + edited, _ = apply_image_edits(_alpha_png(), {"rotate": 180}) + assert edited.name.endswith(".png") + + def test_raises_over_the_limit(self): + """Unlike the thumbnail path, this propagates: the edit has failed.""" + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000), self.assertRaises(ImageTooLargeError): + apply_image_edits(_alpha_png(), {"rotate": 90}) + + +class UploadPixelValidationTest(SimpleTestCase): + """The ceiling is enforced synchronously too, where the path allows it. + + Presigned direct-to-storage uploads never reach ``validate_file``, so the + worker still has to enforce it — but for a normal upload, telling the user + at submit time beats accepting the file and failing it minutes later with + nothing on screen to explain why. + """ + + def _upload(self, data, name="image.png"): + return SimpleUploadedFile(name, data, content_type="image/png") + + def test_rejects_an_image_over_the_pixel_limit(self): + upload = self._upload(_alpha_png().getvalue()) + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000): + _, errors = validate_file(upload) + assert any("megapixels" in e for e in errors) + + def test_the_message_names_the_dimensions_and_the_limit(self): + upload = self._upload(_alpha_png().getvalue()) + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000): + _, errors = validate_file(upload) + message = next(e for e in errors if "megapixels" in e) + assert "600x400" in message + assert "0.2 megapixels" in message + assert "limit is 0.2" in message + + def test_accepts_a_large_jpeg_the_worker_can_handle(self): + """Upload and worker must judge the same file the same way. + + Measuring raw header dimensions here rejected a 40MP JPEG that + ``generate_image_thumbnail`` handles without trouble, because the JPEG + decoder downscales during the read. That also made a REST upload behave + differently from a presigned one, which never reaches this validator. + """ + upload = SimpleUploadedFile("big.jpg", _jpeg(8000, 5000).getvalue(), content_type="image/jpeg") + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=30_000_000): + file_type, errors = validate_file(upload) + assert generate_image_thumbnail(_jpeg(8000, 5000)) is not None + assert file_type == "image" + assert errors == [] + + def test_still_rejects_a_png_of_the_same_size(self): + """PNG has no draft support, so it really would decode full-size.""" + upload = SimpleUploadedFile("big.png", _alpha_png(8000, 5000).getvalue(), content_type="image/png") + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=30_000_000): + _, errors = validate_file(upload) + assert any("megapixels" in e for e in errors) + + def test_accepts_an_image_under_the_limit(self): + upload = self._upload(_alpha_png().getvalue()) + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=1_000_000): + file_type, errors = validate_file(upload) + assert file_type == "image" + assert errors == [] + + def test_leaves_the_read_position_at_zero_for_the_caller(self): + """The caller stores this file next; a consumed handle writes nothing.""" + upload = self._upload(_alpha_png().getvalue()) + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=1_000_000): + validate_file(upload) + assert upload.tell() == 0 + + def test_a_non_image_is_unaffected(self): + upload = SimpleUploadedFile("clip.mp4", b"\x00\x00\x00\x20ftypmp42", content_type="video/mp4") + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=1): + _, errors = validate_file(upload) + assert not any("megapixels" in e for e in errors) diff --git a/apps/media_library/validators.py b/apps/media_library/validators.py index 279ab5ad..ff4fb1b8 100644 --- a/apps/media_library/validators.py +++ b/apps/media_library/validators.py @@ -1,5 +1,6 @@ """File validation for media library uploads.""" +import contextlib from pathlib import Path from django.conf import settings @@ -181,9 +182,54 @@ def validate_file(uploaded_file): max_mb = max_size / (1024 * 1024) errors.append(f"File too large. Maximum size for {file_type} files is {max_mb:.0f}MB.") + if file_type in ("image", "gif"): + errors.extend(_image_pixel_errors(uploaded_file)) + return file_type, errors +def _image_pixel_errors(uploaded_file) -> list[str]: + """Reject images whose decoded size would blow the worker's memory budget. + + File size does not bound this: a highly compressible image can be tiny on + disk and enormous decoded, and decoded cost is what the worker pays. The + worker has to enforce the ceiling itself because presigned + direct-to-storage uploads never pass through here, but checking it on the + synchronous path means the common case is told at upload time instead of + silently landing in FAILED minutes later. + + Goes through ``open_image`` with the same ``draft_size`` the thumbnail path + uses, rather than measuring the header here, so the two agree by + construction. Measuring raw header dimensions would reject a 40MP JPEG that + the worker thumbnails without trouble — the JPEG decoder downscales during + the read — and would make a REST upload behave differently from a presigned + one for the same file. + + Imported lazily because ``services`` imports this module; at module level it + would be circular. + + Anything unreadable is left alone: ``sniff_mime`` has already vouched for + the magic bytes, and a Pillow failure here is not this function's to report. + """ + from django.conf import settings + + from .services import ImageTooLargeError, open_image + + thumb_size = getattr(settings, "MEDIA_LIBRARY_THUMBNAIL_SIZE", (400, 400)) + + try: + with open_image(uploaded_file, draft_size=thumb_size): + return [] + except ImageTooLargeError as exc: + return [str(exc)] + except Exception: + return [] + finally: + # The caller stores this file next; a consumed handle writes nothing. + with contextlib.suppress(OSError, ValueError): + uploaded_file.seek(0) + + def get_accepted_file_types(): """Return a comma-separated string of accepted MIME types for HTML file input.""" return ",".join(sorted(ALL_ALLOWED_MIMES)) diff --git a/config/settings/base.py b/config/settings/base.py index de5d30a2..33c7e47e 100644 --- a/config/settings/base.py +++ b/config/settings/base.py @@ -378,6 +378,20 @@ # Media Library MEDIA_LIBRARY_MAX_IMAGE_SIZE = 20 * 1024 * 1024 # 20MB +# The ceiling that actually bounds memory. A 20 MB file says nothing about how +# many pixels it expands to, and a palette PNG being converted for resampling +# peaks near 5 bytes per pixel, so 30M px is ~150 MB — the spike budget a +# 512 MB worker has left once the app itself is resident. +# +# Pillow's own decompression-bomb check does NOT cover this: it only *warns* +# between 1x and 2x MAX_IMAGE_PIXELS and raises above 2x, leaving a window that +# decodes to over 500 MB. ``apps.media_library.services`` checks this itself, +# and does it AFTER ``Image.draft()`` — a 61 MP JPEG is cheap because the JPEG +# decoder downscales during the read, so rejecting it on its header dimensions +# would refuse a file that never costs us the memory. What this really bounds +# is the formats that have no draft support (PNG, WebP, GIF) and the edit path, +# which needs full resolution by definition. +MEDIA_LIBRARY_MAX_IMAGE_PIXELS = 30_000_000 MEDIA_LIBRARY_MAX_VIDEO_SIZE = 1024 * 1024 * 1024 # 1GB MEDIA_LIBRARY_MAX_BULK_UPLOAD = 50 MEDIA_LIBRARY_THUMBNAIL_SIZE = (400, 400) diff --git a/conftest.py b/conftest.py index bb21d427..f2b9ad3d 100644 --- a/conftest.py +++ b/conftest.py @@ -2,6 +2,7 @@ from django.utils import timezone from apps.accounts.models import User +from apps.media_library.storage import reset_cached_client from apps.members.models import OrgMembership from apps.organizations.models import Organization @@ -22,3 +23,18 @@ def organization(db): def org_owner(db, user, organization): OrgMembership.objects.create(user=user, organization=organization, org_role=OrgMembership.OrgRole.OWNER) return user + + +@pytest.fixture(autouse=True) +def _fresh_storage_client(): + """Keep the memoized boto3 client from leaking across tests. + + ``apps.media_library.storage`` caches one client for the life of the + process — the point is to stop django-storages rebuilding a boto3 Session + per thread. Without this, the first test to touch S3 would pin its client + and every later ``override_settings`` on a bucket or endpoint would be + silently ignored. + """ + reset_cached_client() + yield + reset_cached_client() diff --git a/providers/bluesky.py b/providers/bluesky.py index db2b9903..a463f263 100644 --- a/providers/bluesky.py +++ b/providers/bluesky.py @@ -5,6 +5,7 @@ import base64 import json import logging +import os import re import time from datetime import UTC, datetime @@ -335,22 +336,32 @@ def _parse_facets(self, text: str, access_token: str) -> list[dict]: # ------------------------------------------------------------------ def _upload_blob(self, access_token: str, media_path: str) -> dict: - """Upload a blob to the PDS and return the blob reference.""" + """Upload a blob to the PDS and return the blob reference. + + The file object is handed to httpx as-is so it is read in chunks off + disk. Reading it into a bytes object first put the whole file in RSS, + and this is reached for VIDEO too (see :meth:`_build_embed`) where + ``MEDIA_LIBRARY_MAX_VIDEO_SIZE`` allows up to 1 GB — on a 512 MB worker + that is not a slow leak, it is an immediate OOM kill. + """ import mimetypes mime_type, _ = mimetypes.guess_type(media_path) mime_type = mime_type or "application/octet-stream" with open(media_path, "rb") as f: - file_bytes = f.read() - - resp = self._request( - "POST", - f"{self.pds_url}/xrpc/com.atproto.repo.uploadBlob", - access_token=access_token, - headers={"Content-Type": mime_type}, - data=file_bytes, - ) + resp = self._request( + "POST", + f"{self.pds_url}/xrpc/com.atproto.repo.uploadBlob", + access_token=access_token, + headers={ + "Content-Type": mime_type, + # Explicit so httpx doesn't fall back to chunked transfer + # encoding, which some PDS deployments reject. + "Content-Length": str(os.path.getsize(media_path)), + }, + data=f, + ) data = resp.json() return data.get("blob", data) diff --git a/providers/linkedin.py b/providers/linkedin.py index f5e6f2e0..bf42f500 100644 --- a/providers/linkedin.py +++ b/providers/linkedin.py @@ -36,6 +36,13 @@ REVOKE_URL = "https://www.linkedin.com/oauth/v2/revoke" API_BASE = "https://api.linkedin.com" +# Ceiling for media fetched from a caller-supplied URL. Mirrors the media +# library's own 20MB image cap; providers are Django-independent so it is +# restated here rather than read from settings. LinkedIn's own image limit is +# well under this, so the constant bounds our disk usage rather than deciding +# what LinkedIn will accept. +MAX_REMOTE_MEDIA_BYTES = 20 * 1024 * 1024 + # Required headers for LinkedIn REST API. # LinkedIn sunsets versioned APIs after ~1 year; bump LinkedIn-Version # to the latest YYYYMM at https://learn.microsoft.com/en-us/linkedin/marketing/versioning @@ -710,23 +717,27 @@ def revoke_token(self, access_token: str) -> bool: # ------------------------------------------------------------------ def _upload_binary(self, access_token: str, upload_url: str, source: str) -> None: - """Read media from a local file path or URL and upload to LinkedIn. + """Stream media from a local file path or URL to LinkedIn. Args: source: A local file path or an HTTP(S) URL to download from. - """ - media_bytes = self._read_media_bytes(source) - with httpx.Client(timeout=120.0) as client: - upload_resp = client.put( - upload_url, - content=media_bytes, - headers={ - "Authorization": f"Bearer {access_token}", - "Content-Type": "application/octet-stream", - **LINKEDIN_HEADERS, - }, - ) + Never materializes the media as ``bytes``. A local path is handed to + httpx as an open file object; a URL is streamed to a temp file first, + because a remote URL has no size we control and ``resp.content`` on one + is an unbounded read straight into the worker's heap. + """ + with self._media_handle(source) as media: + with httpx.Client(timeout=120.0) as client: + upload_resp = client.put( + upload_url, + content=media, + headers={ + "Authorization": f"Bearer {access_token}", + "Content-Type": "application/octet-stream", + **LINKEDIN_HEADERS, + }, + ) if upload_resp.status_code >= 400: raise PublishError( f"LinkedIn media upload failed: {upload_resp.status_code}", @@ -735,15 +746,56 @@ def _upload_binary(self, access_token: str, upload_url: str, source: str) -> Non ) @staticmethod - def _read_media_bytes(source: str) -> bytes: - """Load media into memory from a local file path or HTTP(S) URL.""" - if source.startswith(("http://", "https://")): - with httpx.Client(timeout=120.0) as client: - resp = client.get(source) + @contextlib.contextmanager + def _media_handle(source: str): + """Yield an open, readable file object for a local path or HTTP(S) URL. + + The URL branch is bounded. A local path is something we put there and + already size-checked at upload; a URL is supplied by the caller and has + no size we control, so streaming it unchecked just moves an unbounded + read from the heap onto the dyno's shared ephemeral disk. + """ + if not source.startswith(("http://", "https://")): + with open(source, "rb") as f: + yield f + return + + with tempfile.NamedTemporaryFile(suffix=".linkedin-media") as spool: + with httpx.Client(timeout=120.0) as client, client.stream("GET", source) as resp: resp.raise_for_status() - return resp.content - with open(source, "rb") as f: - return f.read() + LinkedInProvider._reject_oversize(resp.headers.get("Content-Length")) + + written = 0 + for chunk in resp.iter_bytes(chunk_size=1024 * 1024): + written += len(chunk) + # Checked per chunk as well as up front: Content-Length is + # the server's claim, not a guarantee, and it is absent + # entirely on a chunked response. + LinkedInProvider._reject_oversize(written) + spool.write(chunk) + spool.flush() + spool.seek(0) + yield spool + + @staticmethod + def _reject_oversize(size) -> None: + """Raise if ``size`` exceeds the remote-media ceiling. None is ignored.""" + if size is None: + return + try: + size = int(size) + except (TypeError, ValueError): + return + if size > MAX_REMOTE_MEDIA_BYTES: + raise PublishError( + f"Remote media exceeds the {MAX_REMOTE_MEDIA_BYTES // (1024 * 1024)}MB limit.", + platform="linkedin", + # Deterministic: the file at that URL will be the same size on + # every attempt. Without this the engine schedules the full + # backoff ladder and re-downloads it each time, to fail + # identically, before telling the user anything. + retryable=False, + ) def _upload_video_chunk(self, upload_url: str, chunk: bytes) -> str: """PUT a single video chunk and return its ETag for finalizeUpload. diff --git a/tests/providers/test_bluesky.py b/tests/providers/test_bluesky.py index 2e460c79..568a2775 100644 --- a/tests/providers/test_bluesky.py +++ b/tests/providers/test_bluesky.py @@ -78,3 +78,63 @@ def test_populates_expires_in_from_jwt(self, mock_request): assert tokens.refresh_token == "new-refresh" assert tokens.expires_in is not None assert 3595 <= tokens.expires_in <= 3600 + + +class TestUploadBlobStreams: + """``_upload_blob`` is reached for VIDEO, where the file can be 1 GB. + + Reading it into a ``bytes`` object first put the whole file in the worker's + RSS — on a 512 MB dyno that is an immediate OOM kill, not a slow leak. + """ + + def _provider(self): + return BlueskyProvider(credentials={"pds_url": "https://pds.example"}) + + def _call(self, tmp_path, payload=b"x" * 4096): + media = tmp_path / "clip.mp4" + media.write_bytes(payload) + provider = self._provider() + with patch.object(provider, "_request") as request: + request.return_value = MagicMock(json=lambda: {"blob": {"$type": "blob"}}) + result = provider._upload_blob("token", str(media)) + return request, result + + def test_passes_a_file_object_not_bytes(self, tmp_path): + request, _ = self._call(tmp_path) + sent = request.call_args.kwargs["data"] + assert not isinstance(sent, bytes | bytearray) + assert hasattr(sent, "read") + + def test_sends_an_explicit_content_length(self, tmp_path): + """Without it httpx falls back to chunked encoding, which some PDSs reject.""" + request, _ = self._call(tmp_path, payload=b"y" * 1234) + assert request.call_args.kwargs["headers"]["Content-Length"] == "1234" + + def test_handle_is_open_and_positioned_at_zero_during_the_request(self, tmp_path): + """The ``with open`` block must still be live when httpx reads the body. + + Reading it inside the mock is the only honest check: afterwards the + block has closed the handle, which is correct but says nothing about + whether the upload could have streamed. + """ + media = tmp_path / "clip.mp4" + media.write_bytes(b"z" * 32) + provider = self._provider() + seen = {} + + def capture(*args, **kwargs): + seen["body"] = kwargs["data"].read() + return MagicMock(json=lambda: {"blob": {}}) + + with patch.object(provider, "_request", side_effect=capture): + provider._upload_blob("token", str(media)) + + assert seen["body"] == b"z" * 32 + + def test_guesses_the_content_type_from_the_path(self, tmp_path): + request, _ = self._call(tmp_path) + assert request.call_args.kwargs["headers"]["Content-Type"] == "video/mp4" + + def test_returns_the_blob_reference(self, tmp_path): + _, result = self._call(tmp_path) + assert result == {"$type": "blob"} diff --git a/tests/providers/test_linkedin_media_stream.py b/tests/providers/test_linkedin_media_stream.py new file mode 100644 index 00000000..ce79a871 --- /dev/null +++ b/tests/providers/test_linkedin_media_stream.py @@ -0,0 +1,140 @@ +"""``_media_handle`` must never materialize media as ``bytes``. + +The local-path branch is bounded by the 20 MB image cap, but the HTTP(S) +branch fetched an arbitrary URL with ``resp.content`` — an unbounded read +straight into the worker's heap, on a dyno with 512 MB total. +""" + +import httpx +import pytest + +from providers.exceptions import PublishError +from providers.linkedin import MAX_REMOTE_MEDIA_BYTES, LinkedInProvider + + +class TestLocalPathBranch: + def test_yields_a_readable_file_object(self, tmp_path): + media = tmp_path / "image.png" + media.write_bytes(b"pixels" * 100) + with LinkedInProvider._media_handle(str(media)) as handle: + assert handle.read() == b"pixels" * 100 + + def test_closes_the_handle_on_exit(self, tmp_path): + media = tmp_path / "image.png" + media.write_bytes(b"x") + with LinkedInProvider._media_handle(str(media)) as handle: + pass + assert handle.closed + + +class TestUrlBranch: + def _transport(self, body, status=200): + return httpx.MockTransport(lambda request: httpx.Response(status, content=body)) + + def test_spools_a_url_to_disk_and_yields_a_file(self, monkeypatch): + body = b"remote-bytes" * 5000 + transport = self._transport(body) + original = httpx.Client + + def client(*args, **kwargs): + kwargs["transport"] = transport + return original(*args, **kwargs) + + monkeypatch.setattr(httpx, "Client", client) + + with LinkedInProvider._media_handle("https://cdn.example/photo.jpg") as handle: + # A real file on disk, not an in-memory buffer. + assert handle.fileno() > 0 + assert handle.read() == body + + def test_starts_at_offset_zero(self, monkeypatch): + """The spool is written then rewound; forgetting the seek uploads nothing.""" + body = b"abcdef" + transport = self._transport(body) + original = httpx.Client + monkeypatch.setattr(httpx, "Client", lambda *a, **k: original(*a, **{**k, "transport": transport})) + + with LinkedInProvider._media_handle("https://cdn.example/photo.jpg") as handle: + assert handle.tell() == 0 + + def test_raises_on_an_http_error(self, monkeypatch): + transport = self._transport(b"", status=404) + original = httpx.Client + monkeypatch.setattr(httpx, "Client", lambda *a, **k: original(*a, **{**k, "transport": transport})) + + with ( + pytest.raises(httpx.HTTPStatusError), + LinkedInProvider._media_handle("https://cdn.example/missing.jpg"), + ): + pass + + +class TestUrlBranchSizeCap: + """A caller-supplied URL has no size we control. + + Streaming it to disk unchecked just moves an unbounded read off the heap + and onto the dyno's shared ephemeral disk, which affects every process on + that dyno rather than only this publish. + """ + + def _patch(self, monkeypatch, handler): + transport = httpx.MockTransport(handler) + original = httpx.Client + monkeypatch.setattr(httpx, "Client", lambda *a, **k: original(*a, **{**k, "transport": transport})) + + def test_the_rejection_is_not_retryable(self, monkeypatch): + """The file at that URL is the same size on every attempt. + + Left retryable, the publish engine walks the full backoff ladder and + re-downloads it each time to fail identically, delaying the moment the + user is told anything. ``apps.publisher.engine`` reads this via + ``getattr(e, "retryable", True)``, so the default is the wrong one. + """ + over = MAX_REMOTE_MEDIA_BYTES + 1 + self._patch(monkeypatch, lambda request: httpx.Response(200, headers={"Content-Length": str(over)})) + with ( + pytest.raises(PublishError) as excinfo, + LinkedInProvider._media_handle("https://cdn.example/huge.jpg"), + ): + pass + assert excinfo.value.retryable is False + + def test_rejects_on_a_content_length_over_the_cap(self, monkeypatch): + over = MAX_REMOTE_MEDIA_BYTES + 1 + + def handler(request): + return httpx.Response(200, headers={"Content-Length": str(over)}, content=b"") + + self._patch(monkeypatch, handler) + with ( + pytest.raises(PublishError, match="exceeds"), + LinkedInProvider._media_handle("https://cdn.example/huge.jpg"), + ): + pass + + def test_rejects_a_body_that_outgrows_a_missing_content_length(self, monkeypatch): + """Content-Length is the server's claim, and chunked responses omit it.""" + body = b"x" * (MAX_REMOTE_MEDIA_BYTES + 1024) + + def handler(request): + return httpx.Response(200, content=body) + + self._patch(monkeypatch, handler) + with ( + pytest.raises(PublishError, match="exceeds"), + LinkedInProvider._media_handle("https://cdn.example/lying.jpg"), + ): + pass + + def test_allows_media_under_the_cap(self, monkeypatch): + body = b"y" * 2048 + self._patch(monkeypatch, lambda request: httpx.Response(200, content=body)) + with LinkedInProvider._media_handle("https://cdn.example/fine.jpg") as handle: + assert handle.read() == body + + def test_a_local_path_is_not_subject_to_the_remote_cap(self, tmp_path): + """Local files came from our own upload path and were sized there.""" + media = tmp_path / "big.png" + media.write_bytes(b"z" * (MAX_REMOTE_MEDIA_BYTES + 10)) + with LinkedInProvider._media_handle(str(media)) as handle: + assert len(handle.read()) == MAX_REMOTE_MEDIA_BYTES + 10