From 61976050516e52be045d3c2bb781acc7e6ee79ac Mon Sep 17 00:00:00 2001 From: Jan Schmitz Date: Fri, 18 Sep 2026 10:45:57 +0200 Subject: [PATCH 01/11] perf(worker): recycle process_tasks hourly, cap glibc malloc arenas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Heroku worker sat at 110% of its 512 MB quota with 3081 R14s in 24h and 82 MB of swap, and the web dyno at 88%. Neither is a leak: there is no accumulating Python object in the tree. It is a ratchet — transient peaks raise the high-water mark, CPython and glibc keep the freed pages in their own arenas, and nothing restarts the process to hand them back. After a deploy the worker starts at ~280 MB and climbs ~19 MB/h for 15 hours. process_tasks is a single non-forking process that runs every task in the same heap and never restarts, so it has no floor to return to. --duration gives it one. The condition is evaluated at the TOP of the run loop, so a task in flight always completes; the process exits 0 between tasks and Heroku restarts the dyno. Nothing is stranded — a recycle cannot interrupt a publish, and confirm_pending_publishes settles anything in flight regardless. 3600s keeps well clear of Heroku's crash cool-off, which only engages on repeated quick exits. Left out of docker-compose.yml deliberately: its worker has no restart policy, so there a clean exit would just stop it. MALLOC_ARENA_MAX caps the per-thread arenas glibc hands out (default 8 x nproc, and containers report the host's core count). This app has real thread churn to feed that: the publisher builds a fresh ThreadPoolExecutor every 15 seconds and boto3's managed transfer adds ten threads per download. Co-Authored-By: Claude Opus 5 --- Procfile | 2 +- README.md | 4 ++++ app.json | 4 ++++ 3 files changed, 9 insertions(+), 1 deletion(-) 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..8c62e383 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. + +**Also worth setting on a memory-tight host:** `MALLOC_ARENA_MAX=2`. 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. + See `architecture.md` for detailed per-platform instructions and cost breakdowns. ## Project Structure diff --git a/app.json b/app.json index 0325c28e..2e9b312a 100644 --- a/app.json +++ b/app.json @@ -28,6 +28,10 @@ "DJANGO_SETTINGS_MODULE": { "value": "config.settings.production" }, + "MALLOC_ARENA_MAX": { + "description": "Caps glibc's per-thread malloc arenas. The default is 8 x nproc and dynos report the host's core count, so arenas proliferate and are never returned to the OS. Leave at 2 unless you have measured otherwise.", + "value": "2" + }, "ALLOWED_HOSTS": { "description": "Comma-separated list of allowed hostnames", "value": "*" From 3af4831057e366e715ce096232db19cd1e992901 Mon Sep 17 00:00:00 2001 From: Jan Schmitz Date: Fri, 18 Sep 2026 10:46:19 +0200 Subject: [PATCH 02/11] perf(media): memoize the boto3 client instead of rebuilding it per thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit django-storages backs S3Storage.connection with a threading.local(), so reaching through it builds a brand-new 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. download_to_path calls this from inside the publisher's group and platform threads, which are new objects every 15-second cycle, and boto3's own managed transfer adds ten more threads per download. So the worker was building and discarding multi-MB clients continuously, all day, into an allocator that never hands the pages back. That is a large part of the ratchet the hourly recycle was papering over. Safe to share: boto3 clients are documented thread-safe (resources are not) and every caller in this module uses client methods only. Credential refresh is handled inside the client. The setting_changed receiver matters for the suite rather than production — without it the first test to touch S3 would pin its client for the whole run and every later override_settings on a bucket or endpoint would be ignored. Co-Authored-By: Claude Opus 5 --- apps/media_library/storage.py | 52 +++++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/apps/media_library/storage.py b/apps/media_library/storage.py index 6777d9e1..54e06ac2 100644 --- a/apps/media_library/storage.py +++ b/apps/media_library/storage.py @@ -11,10 +11,13 @@ from __future__ import annotations import shutil +import threading import uuid from django.conf import settings from django.core.files.storage import default_storage +from django.core.signals import setting_changed +from django.dispatch import receiver from django.utils import timezone from .validators import ALL_ALLOWED_EXTENSIONS @@ -23,6 +26,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 +74,49 @@ 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 + + +@receiver(setting_changed) +def _reset_cached_client(sender, setting, **kwargs): + """Drop the memoized client when a test swaps the storage configuration. + + Without this, the first test to touch S3 would pin its client for the rest + of the run and every later ``override_settings`` on a bucket, endpoint or + credential would be silently ignored. + """ + global _cached_client + + if setting == "STORAGES" or setting.startswith("AWS_") or setting == "STORAGE_BACKEND": + with _client_lock: + _cached_client = None def _normalize(storage_key: str) -> str: From 3f982050b54bf02894b897f505c7d491b2a0285b Mon Sep 17 00:00:00 2001 From: Jan Schmitz Date: Fri, 18 Sep 2026 10:46:19 +0200 Subject: [PATCH 03/11] fix(media): bound the image decode instead of letting it size the dyno MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the spike half of the memory problem — the class of event behind the R15 on 2026-09-14, not the R14 baseline. Ordering. 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, 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, since converting first replaces the JpegImageFile with a plain Image and the DCT downscale goes with it — which is what made CMYK JPEGs so expensive. Ceiling. MEDIA_LIBRARY_MAX_IMAGE_SIZE is 20 MB, which says nothing about how many pixels that expands to. Pillow's own bomb check does not close the gap: it only warns between 1x and 2x MAX_IMAGE_PIXELS and raises above 2x, leaving a window that decodes to over 500 MB. open_image checks width*height itself, on the header, before a pixel is decoded. The check runs AFTER draft() on purpose. A 61 MP JPEG is cheap because the decoder downscales during the read, so judging it on its header dimensions would refuse a file that never costs us the memory. What the limit really bounds is the formats with no draft support (PNG, WebP, GIF) and the edit path, which needs full resolution by definition. For the same reason we do NOT assign Image.MAX_IMAGE_PIXELS from the setting — Pillow evaluates that inside Image.open(), before draft() has run, so pinning it there would reject exactly the files draft() makes cheap. Its default stays as the outer backstop. An over-limit image now fails the asset with a real reason rather than taking the dyno down with it. Also fixes a latent bug found while restructuring apply_image_edits: the save block sat outside the open context, so with no operations the working image IS the opened one and leaving the context closed its file pointer before save() had ever loaded it. None of these functions had a single test — no test in the repo imported PIL. Adds 22 covering the guard, both draft behaviours, every accepted format, and the no-operations edit path. Co-Authored-By: Claude Opus 5 --- apps/media_library/services.py | 239 ++++++++++++------ .../tests/test_image_processing.py | 193 ++++++++++++++ config/settings/base.py | 14 + 3 files changed, 369 insertions(+), 77 deletions(-) create mode 100644 apps/media_library/tests/test_image_processing.py diff --git a/apps/media_library/services.py b/apps/media_library/services.py index 9412d071..24f7ab41 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,52 +366,133 @@ def _check_post_references(asset): return [{"id": str(ref.post_id), "caption": (ref.post.caption or "")[:80]} for ref in scheduled_refs] +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): + """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. + + 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: + 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 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:.0f} megapixels." + ) + yield img + + def extract_image_metadata(file_path_or_file): """Extract dimensions from an image file using Pillow.""" try: - 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) - width, height = img.size + # No draft: the caller wants the image's real dimensions, and reading + # the header costs nothing either way. + with open_image(file_path_or_file) as img: + width, height = img.size return {"width": width, "height": height} + except ImageTooLargeError: + # Dimensions are exactly what we just read, so report them rather than + # pretending we could not parse the file. + logger.warning("Image exceeds the decode limit", exc_info=True) + return {} except Exception: logger.exception("Failed to extract image metadata") return {} 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: + logger.warning("Image exceeds the decode limit; no thumbnail generated", exc_info=True) + return None except Exception: logger.exception("Failed to generate image thumbnail") return None @@ -563,52 +645,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/tests/test_image_processing.py b/apps/media_library/tests/test_image_processing.py new file mode 100644 index 00000000..6bb57e3c --- /dev/null +++ b/apps/media_library/tests/test_image_processing.py @@ -0,0 +1,193 @@ +"""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.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, +) + + +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_returns_none_over_the_limit_rather_than_raising(self): + """``_process_image`` treats a falsy thumbnail as "skip it".""" + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000): + assert generate_image_thumbnail(_alpha_png()) is None + + 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_returns_empty_over_the_limit(self): + with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000): + assert extract_image_metadata(_alpha_png()) == {} + + 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}) 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) From 04f7f64350dd5cc2b105330b1702001ac19e89aa Mon Sep 17 00:00:00 2001 From: Jan Schmitz Date: Fri, 18 Sep 2026 10:46:40 +0200 Subject: [PATCH 04/11] fix(providers): stream Bluesky and LinkedIn uploads instead of read()ing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two survivors of the streaming pass in 4ca3647. Bluesky's _upload_blob did f.read() with no size check, and _build_embed routes VIDEO through it — MEDIA_LIBRARY_MAX_VIDEO_SIZE allows 1 GB. On a 512 MB worker that is not a ratchet, it is an immediate OOM kill on the first large video. LinkedIn is narrower than it looks: the video path already chunks from disk and _upload_binary's local-file branch is bounded by the 20 MB image cap. The unbounded one was the HTTP(S) branch, resp.content on an arbitrary URL. It now streams to a temp file first. Both fixes are small because providers/base.py already does the right thing — it routes a non-dict `data` to httpx's `content`, which streams a file object and derives Content-Length from it. Bluesky just had to hand over the handle rather than the bytes. Content-Length is passed explicitly so httpx cannot fall back to chunked transfer encoding, which some PDS deployments reject. _upload_blob had no test; the new ones assert on the object handed to the request rather than on the response, since "did this stream" is not observable afterwards. Co-Authored-By: Claude Opus 5 --- providers/bluesky.py | 31 ++++++--- providers/linkedin.py | 53 +++++++++------ tests/providers/test_bluesky.py | 60 ++++++++++++++++ tests/providers/test_linkedin_media_stream.py | 68 +++++++++++++++++++ 4 files changed, 181 insertions(+), 31 deletions(-) create mode 100644 tests/providers/test_linkedin_media_stream.py 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..1206911e 100644 --- a/providers/linkedin.py +++ b/providers/linkedin.py @@ -710,23 +710,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 +739,22 @@ 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.""" + 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() + for chunk in resp.iter_bytes(chunk_size=1024 * 1024): + spool.write(chunk) + spool.flush() + spool.seek(0) + yield spool 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..0c79dd00 --- /dev/null +++ b/tests/providers/test_linkedin_media_stream.py @@ -0,0 +1,68 @@ +"""``_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.linkedin import 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 From 8fa4229fe6dcf3952be78ac2d3083e67926ea229 Mon Sep 17 00:00:00 2001 From: Jan Schmitz Date: Fri, 18 Sep 2026 10:46:40 +0200 Subject: [PATCH 05/11] perf(analytics): stop hydrating every snapshot row to read three numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The web dyno's equivalent of the image decode: one page render that allocates hundreds of MB on a process with ~60 MB of headroom. PostInsightsSnapshot is one row per (post, metric, day) and carries two JSONFields — `raw` is the entire provider response — which Django decodes eagerly while hydrating a row. _latest_post_stats iterated those as model instances to read platform_post_id, metric_key and value, 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 json.loads calls per render. Now three columns via values_list, with the dedup pushed into Postgres: the order_by DISTINCT ON requires is the ordering this needed anyway, so the query returns one row per (post, metric) rather than one per day. Verified the emitted SQL keeps `date DESC` in the ORDER BY, which is the only reason the newest row survives; the new tests pin that. `raw` has no reader anywhere in the application — it is written in three places and read in none. Shrinking the write side is worth doing but is a database problem now, not a memory one, so it stays separate. Same one-line change in the two live siblings: account_analytics_bundle, which runs on every analytics render and every agent-API account call, and _post_sparklines_with_freshness. Drops _post_sparklines, which nothing calls. _series_for is deliberately left alone: both callers always take the series_map branch, so it is unreachable in production and fixing it buys nothing. Co-Authored-By: Claude Opus 5 --- apps/analytics/services.py | 99 +++++++++++++++------------ apps/analytics/tests/test_services.py | 96 ++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 44 deletions(-) diff --git a/apps/analytics/services.py b/apps/analytics/services.py index 9bb3483f..267c8017 100644 --- a/apps/analytics/services.py +++ b/apps/analytics/services.py @@ -293,25 +293,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 +568,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 +635,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 +669,55 @@ 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. + + ``DISTINCT ON`` then 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. + """ + 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") + .distinct("platform_post_id", "metric_key") + .values_list("platform_post_id", "metric_key", "value") ) out: dict[Any, dict[str, float]] = defaultdict(dict) - seen: set[tuple[Any, str]] = set() - for r in rows: - key = (r.platform_post_id, r.metric_key) - if key in seen: - continue - seen.add(key) - out[r.platform_post_id][r.metric_key] = r.value + for post_id, metric_key, value in rows: + 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..48ad60d1 100644 --- a/apps/analytics/tests/test_services.py +++ b/apps/analytics/tests/test_services.py @@ -99,3 +99,99 @@ 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}} From 119e1e099c9552e8ab75de299cb3e8a36909ae6f Mon Sep 17 00:00:00 2001 From: Jan Schmitz Date: Fri, 18 Sep 2026 10:46:40 +0200 Subject: [PATCH 06/11] perf(accounts): import Pillow lazily in the avatar handler config/urls.py imports apps.accounts.views for health_check, so a module-scope `from PIL import Image` loaded the _imaging extension into every web AND worker process at boot, whether or not an avatar was ever touched. Worth ~8-15 MB in each, and the rest of the codebase already imports PIL inside the function that needs it. Co-Authored-By: Claude Opus 5 --- apps/accounts/views.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) 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 From 5d58fbcedaadbaa0b584856c53f9bee73450bbf1 Mon Sep 17 00:00:00 2001 From: Jan Schmitz Date: Fri, 18 Sep 2026 11:36:05 +0200 Subject: [PATCH 07/11] fix(analytics): keep the snapshot dedup working on SQLite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `.distinct(*fields)` compiles to PostgreSQL's DISTINCT ON, and every other backend inherits Django's base `distinct_sql`, which raises NotSupportedError the moment a field is passed. README.md documents SQLite twice — as the "Fully Local Development (without Docker)" path, and as "fine for local development and small deployments" — so this took out the entire analytics surface on those deployments: the index page, the post-detail drawer, the agent API analytics endpoints and the MCP analytics tools, all 500. CI could not catch it because config/settings/test.py pins Postgres. The clause is now applied only when the backend advertises the capability, with the previous Python dedup as the fallback over the same ordering. Nothing is lost on Postgres, and the `values_list` that avoids hydrating every JSONField — which is where nearly all of the memory saving came from — was never backend-specific and applies either way. Verified against a real SQLite database, not just a patched feature flag: the old form raises NotSupportedError there, the new one compiles, and 228 tests across analytics, the media library, the API router and MCP parity pass on it. Co-Authored-By: Claude Opus 5 --- apps/analytics/services.py | 31 +++++++++++--- apps/analytics/tests/test_services.py | 58 +++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 5 deletions(-) diff --git a/apps/analytics/services.py b/apps/analytics/services.py index 267c8017..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 @@ -681,9 +682,17 @@ def _latest_post_stats(post_ids: Iterable[Any], metrics: list[str]) -> dict[Any, that is 216k instances and 432k needless decodes, in a web process with ~60 MB of headroom. - ``DISTINCT ON`` then 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. + 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: @@ -691,11 +700,23 @@ def _latest_post_stats(post_ids: Iterable[Any], metrics: list[str]) -> dict[Any, rows = ( PostInsightsSnapshot.objects.filter(platform_post_id__in=post_ids, metric_key__in=metrics) .order_by("platform_post_id", "metric_key", "-date") - .distinct("platform_post_id", "metric_key") .values_list("platform_post_id", "metric_key", "value") ) + out: dict[Any, dict[str, float]] = defaultdict(dict) - for post_id, metric_key, value in rows: + 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 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[post_id][metric_key] = value return out diff --git a/apps/analytics/tests/test_services.py b/apps/analytics/tests/test_services.py index 48ad60d1..cc24dc0f 100644 --- a/apps/analytics/tests/test_services.py +++ b/apps/analytics/tests/test_services.py @@ -195,3 +195,61 @@ def test_latest_post_stats_runs_one_query_regardless_of_history(facebook_account 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 From 76b9735680a8896c005204ff6e9a5dccc9c41a57 Mon Sep 17 00:00:00 2001 From: Jan Schmitz Date: Fri, 18 Sep 2026 11:36:05 +0200 Subject: [PATCH 08/11] fix(media): make the pixel ceiling consistent, and actually surface it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three bugs, all from the ceiling added in 3f98205. Dimensions disagreed with thumbnails. `extract_image_metadata` opened without a draft while `generate_image_thumbnail` opened with one, so the same file was judged against two different pixel counts. A 40MP JPEG drafts down to something cheap and thumbnails fine, but metadata saw the header size, refused, and stored 0x0 — a working thumbnail beside zero dimensions, which `aspect_ratio` then reports as 0 to agent clients. Reading `img.size` decodes nothing, so metadata is now exempt from the ceiling entirely: there is no memory to save by refusing, and the numbers withheld are the ones the guard just read. Rejections were invisible. Both helpers swallowed ImageTooLargeError, so `_process_image` never raised and the asset was saved COMPLETED with no thumbnail and no reason — it read as success everywhere in the UI. "Too large" is determinate and actionable, so it now propagates and the asset lands FAILED, keeping the dimensions that caused it. "Pillow could not read this" stays a soft None, and a test pins that distinction. A rejected edit left a phantom version. `create_version` seeds the row with a copy of the source and points the asset at it, so a swallowed failure left what looks like an unchanged duplicate that will never become anything else. It is now removed, rewinding `current_version` to the version it superseded first — the FK is SET_NULL, so deleting without that strands the asset with no current version at all. Also enforces the ceiling in `validate_file`, so a normal upload is refused at submit time with the dimensions and the limit. Presigned direct-to-storage uploads bypass Django entirely, which is why the worker keeps its own check. Replaces the `setting_changed` receiver from 3af4831 with an explicit `reset_cached_client()` called from a conftest fixture. That receiver put test-only machinery in every production process, matched setting names by string prefix so a new storage setting would silently stop resetting it, and could fire mid-call — `_client_and_bucket` reads the cache before taking the lock, so a concurrent reset could be missed and a stale client returned. Adds test_asset_processing.py: the unit tests passed happily while the task built on them did the wrong thing, because nothing composed the two helpers the way `_process_image` does. Co-Authored-By: Claude Opus 5 --- apps/media_library/services.py | 38 ++-- apps/media_library/storage.py | 27 +-- apps/media_library/tasks.py | 34 +++- .../tests/test_asset_processing.py | 169 ++++++++++++++++++ .../tests/test_image_processing.py | 85 ++++++++- apps/media_library/validators.py | 43 +++++ conftest.py | 16 ++ 7 files changed, 380 insertions(+), 32 deletions(-) create mode 100644 apps/media_library/tests/test_asset_processing.py diff --git a/apps/media_library/services.py b/apps/media_library/services.py index 24f7ab41..75e420d0 100644 --- a/apps/media_library/services.py +++ b/apps/media_library/services.py @@ -371,7 +371,7 @@ class ImageTooLargeError(Exception): @contextlib.contextmanager -def open_image(file_path_or_file, *, draft_size=None): +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 @@ -398,6 +398,11 @@ def open_image(file_path_or_file, *, draft_size=None): 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. @@ -426,27 +431,29 @@ def open_image(file_path_or_file, *, draft_size=None): if draft_size is not None: img.draft("RGB", (draft_size[0] * 2, draft_size[1] * 2)) pixels = img.width * img.height - if pixels > max_pixels: + 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:.0f} megapixels." + f"the limit is {max_pixels / 1_000_000:g} megapixels." ) yield img def extract_image_metadata(file_path_or_file): - """Extract dimensions from an image file using Pillow.""" + """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: - # No draft: the caller wants the image's real dimensions, and reading - # the header costs nothing either way. - with open_image(file_path_or_file) as img: + with open_image(file_path_or_file, enforce_limit=False) as img: width, height = img.size return {"width": width, "height": height} - except ImageTooLargeError: - # Dimensions are exactly what we just read, so report them rather than - # pretending we could not parse the file. - logger.warning("Image exceeds the decode limit", exc_info=True) - return {} except Exception: logger.exception("Failed to extract image metadata") return {} @@ -491,8 +498,11 @@ def generate_image_thumbnail(file_path_or_file): img.save(buffer, format="JPEG", quality=85) return ContentFile(buffer.getvalue(), name="thumbnail.jpg") except ImageTooLargeError: - logger.warning("Image exceeds the decode limit; no thumbnail generated", exc_info=True) - return None + # 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 diff --git a/apps/media_library/storage.py b/apps/media_library/storage.py index 54e06ac2..5990ee65 100644 --- a/apps/media_library/storage.py +++ b/apps/media_library/storage.py @@ -16,8 +16,6 @@ from django.conf import settings from django.core.files.storage import default_storage -from django.core.signals import setting_changed -from django.dispatch import receiver from django.utils import timezone from .validators import ALL_ALLOWED_EXTENSIONS @@ -104,19 +102,26 @@ def _client_and_bucket(): return client, bucket -@receiver(setting_changed) -def _reset_cached_client(sender, setting, **kwargs): - """Drop the memoized client when a test swaps the storage configuration. +def reset_cached_client() -> None: + """Drop the memoized client so the next call rebuilds it. - Without this, the first test to touch S3 would pin its client for the rest - of the run and every later ``override_settings`` on a bucket, endpoint or - credential would be silently ignored. + 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 - if setting == "STORAGES" or setting.startswith("AWS_") or setting == "STORAGE_BACKEND": - with _client_lock: - _cached_client = None + 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..af4d697e 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) @@ -107,6 +122,23 @@ 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 + 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..87422495 --- /dev/null +++ b/apps/media_library/tests/test_asset_processing.py @@ -0,0 +1,169 @@ +"""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 +from apps.media_library.tasks import process_media_asset + + +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 diff --git a/apps/media_library/tests/test_image_processing.py b/apps/media_library/tests/test_image_processing.py index 6bb57e3c..1ba3d112 100644 --- a/apps/media_library/tests/test_image_processing.py +++ b/apps/media_library/tests/test_image_processing.py @@ -9,6 +9,7 @@ import io +from django.core.files.uploadedfile import SimpleUploadedFile from django.test import SimpleTestCase, override_settings from PIL import Image @@ -19,6 +20,7 @@ generate_image_thumbnail, open_image, ) +from apps.media_library.validators import validate_file def _encode(img, fmt): @@ -137,10 +139,18 @@ def test_preserves_aspect_ratio(self): with Image.open(io.BytesIO(thumb.read())) as out: assert out.size == (400, 133) - def test_returns_none_over_the_limit_rather_than_raising(self): - """``_process_image`` treats a falsy thumbnail as "skip it".""" - with override_settings(MEDIA_LIBRARY_MAX_IMAGE_PIXELS=200_000): - assert generate_image_thumbnail(_alpha_png()) is None + 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 @@ -150,9 +160,24 @@ 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_returns_empty_over_the_limit(self): + 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()) == {} + 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")) == {} @@ -191,3 +216,51 @@ 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_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..aae2e76a 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,51 @@ 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 enforces the same ceiling (``apps.media_library.services.open_image``) + because presigned direct-to-storage uploads never pass through here, but + checking it on the synchronous path means the common case gets told at + upload time instead of silently landing in FAILED minutes later. + + Reading the header decodes nothing, so this is cheap. 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 + + max_pixels = getattr(settings, "MEDIA_LIBRARY_MAX_IMAGE_PIXELS", 30_000_000) + + try: + from PIL import Image + + uploaded_file.seek(0) + with Image.open(uploaded_file) as img: + pixels = img.width * img.height + dimensions = f"{img.width}x{img.height}" + except Exception: + return [] + finally: + with contextlib.suppress(OSError, ValueError): + uploaded_file.seek(0) + + if pixels > max_pixels: + return [ + f"Image is too large to process: {dimensions} " + f"({pixels / 1_000_000:.1f} megapixels, limit is {max_pixels / 1_000_000:g})." + ] + return [] + + 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/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() From daff36ff584b98b08831c192eb97a6e9a28a9265 Mon Sep 17 00:00:00 2001 From: Jan Schmitz Date: Fri, 18 Sep 2026 11:36:05 +0200 Subject: [PATCH 09/11] fix(linkedin): bound media fetched from a caller-supplied URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spooling the URL branch to disk in 04f7f64 moved an unbounded read off the heap but not out of the dyno — it now fills the shared ephemeral disk instead, which affects every process there rather than just the publish. Capped at 20MB, mirroring the media library's image limit (providers are Django-independent, so the constant is restated rather than read from settings). Checked against Content-Length up front and against bytes written as we go, because Content-Length is the server's claim and is absent entirely on a chunked response. Local paths are exempt: those came from our own upload path and were sized there. Co-Authored-By: Claude Opus 5 --- providers/linkedin.py | 38 ++++++++++++- tests/providers/test_linkedin_media_stream.py | 57 ++++++++++++++++++- 2 files changed, 93 insertions(+), 2 deletions(-) diff --git a/providers/linkedin.py b/providers/linkedin.py index 1206911e..13f12f4c 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 @@ -741,7 +748,13 @@ def _upload_binary(self, access_token: str, upload_url: str, source: str) -> Non @staticmethod @contextlib.contextmanager def _media_handle(source: str): - """Yield an open, readable file object for a local path or HTTP(S) URL.""" + """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 @@ -750,12 +763,35 @@ def _media_handle(source: str): 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() + 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", + ) + 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_linkedin_media_stream.py b/tests/providers/test_linkedin_media_stream.py index 0c79dd00..b22dcc37 100644 --- a/tests/providers/test_linkedin_media_stream.py +++ b/tests/providers/test_linkedin_media_stream.py @@ -8,7 +8,8 @@ import httpx import pytest -from providers.linkedin import LinkedInProvider +from providers.exceptions import PublishError +from providers.linkedin import MAX_REMOTE_MEDIA_BYTES, LinkedInProvider class TestLocalPathBranch: @@ -66,3 +67,57 @@ def test_raises_on_an_http_error(self, monkeypatch): 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_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 From 6ffacf0214754da8b13acac42032708ca0acdbb4 Mon Sep 17 00:00:00 2001 From: Jan Schmitz Date: Fri, 18 Sep 2026 12:04:02 +0200 Subject: [PATCH 10/11] fix(media,linkedin): address Codex review of the memory pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validator and worker disagreed about JPEGs. `_image_pixel_errors` measured raw header dimensions while `generate_image_thumbnail` measures the drafted decode, so `validate_file` rejected a 40MP JPEG the worker thumbnails without trouble — the JPEG decoder downscales during the read. It also made a REST upload behave differently from a presigned one for the same file, since presigned uploads never reach the validator. This is the same asymmetry fixed in 76b9735 between metadata and thumbnails, reintroduced one layer up. The validator now goes through `open_image` with the thumbnail path's own `draft_size`, so the two agree by construction rather than by matching constants. A rejected edit orphaned the file it had written. Django does not delete FileField objects when a row goes, and the delete added in 76b9735 fired after `version.file.save()` in one reachable case: `apply_image_edits` checks the SOURCE size, so an upscaling resize succeeds and then produces output too large to thumbnail. Confirmed — a 400x400 source resized to 7000x7000 passes the edit and fails the thumbnail at 49MP. Generated artifacts are now removed first, guarded on the stored name: `create_version` seeds the row by assigning the asset's FieldFile, which copies the name rather than the bytes, so until the edit is written the version and the asset are the same stored object and a naive cleanup would delete the asset's own file. The regression test was verified to fail without the fix. Oversize remote media was retryable. `PublishError` defaults `retryable=True` and `apps.publisher.engine` reads it via `getattr(e, "retryable", True)`, so a URL over the 20MB ceiling walked the full backoff ladder, re-downloading the file each time to fail identically, before telling the user anything. Now `retryable=False`, matching how TikTok already reports its own size rejections. Co-Authored-By: Claude Opus 5 --- apps/media_library/tasks.py | 18 +++++ .../tests/test_asset_processing.py | 73 ++++++++++++++++++- .../tests/test_image_processing.py | 22 ++++++ apps/media_library/validators.py | 47 ++++++------ providers/linkedin.py | 5 ++ tests/providers/test_linkedin_media_stream.py | 17 +++++ 6 files changed, 159 insertions(+), 23 deletions(-) diff --git a/apps/media_library/tasks.py b/apps/media_library/tasks.py index af4d697e..d320c50c 100644 --- a/apps/media_library/tasks.py +++ b/apps/media_library/tasks.py @@ -102,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) @@ -135,6 +142,17 @@ def process_image_edit(version_id, operations): # 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 diff --git a/apps/media_library/tests/test_asset_processing.py b/apps/media_library/tests/test_asset_processing.py index 87422495..99b638a8 100644 --- a/apps/media_library/tests/test_asset_processing.py +++ b/apps/media_library/tests/test_asset_processing.py @@ -14,10 +14,31 @@ from django.test import override_settings from PIL import Image -from apps.media_library.models import MediaAsset +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") @@ -167,3 +188,53 @@ def test_rejected_edit_on_a_first_version_leaves_no_current(asset, user): 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 index 1ba3d112..2c218250 100644 --- a/apps/media_library/tests/test_image_processing.py +++ b/apps/media_library/tests/test_image_processing.py @@ -245,6 +245,28 @@ def test_the_message_names_the_dimensions_and_the_limit(self): 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): diff --git a/apps/media_library/validators.py b/apps/media_library/validators.py index aae2e76a..ff4fb1b8 100644 --- a/apps/media_library/validators.py +++ b/apps/media_library/validators.py @@ -193,39 +193,42 @@ def _image_pixel_errors(uploaded_file) -> list[str]: 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 enforces the same ceiling (``apps.media_library.services.open_image``) - because presigned direct-to-storage uploads never pass through here, but - checking it on the synchronous path means the common case gets told at - upload time instead of silently landing in FAILED minutes later. - - Reading the header decodes nothing, so this is cheap. 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. + 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 - max_pixels = getattr(settings, "MEDIA_LIBRARY_MAX_IMAGE_PIXELS", 30_000_000) + from .services import ImageTooLargeError, open_image - try: - from PIL import Image + thumb_size = getattr(settings, "MEDIA_LIBRARY_THUMBNAIL_SIZE", (400, 400)) - uploaded_file.seek(0) - with Image.open(uploaded_file) as img: - pixels = img.width * img.height - dimensions = f"{img.width}x{img.height}" + 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) - if pixels > max_pixels: - return [ - f"Image is too large to process: {dimensions} " - f"({pixels / 1_000_000:.1f} megapixels, limit is {max_pixels / 1_000_000:g})." - ] - return [] - def get_accepted_file_types(): """Return a comma-separated string of accepted MIME types for HTML file input.""" diff --git a/providers/linkedin.py b/providers/linkedin.py index 13f12f4c..bf42f500 100644 --- a/providers/linkedin.py +++ b/providers/linkedin.py @@ -790,6 +790,11 @@ def _reject_oversize(size) -> None: 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: diff --git a/tests/providers/test_linkedin_media_stream.py b/tests/providers/test_linkedin_media_stream.py index b22dcc37..ce79a871 100644 --- a/tests/providers/test_linkedin_media_stream.py +++ b/tests/providers/test_linkedin_media_stream.py @@ -82,6 +82,23 @@ def _patch(self, monkeypatch, 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 From 53308c6485ed0a892e2ccd4d7ec12eafd2d7855a Mon Sep 17 00:00:00 2001 From: Jan Schmitz Date: Fri, 18 Sep 2026 12:43:15 +0200 Subject: [PATCH 11/11] chore(heroku): set MALLOC_ARENA_MAX from .profile instead of by hand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the one manual step this branch otherwise left behind. Heroku sources .profile before the dyno command for every process type, so web, worker, the release phase and one-off `heroku run` dynos all get it from a single declaration — where the Procfile alternative would need the assignment repeated on each line and would still miss `heroku run` and release. Written as `${MALLOC_ARENA_MAX:-2}` rather than a bare assignment. .profile is sourced after config vars are injected, so assigning unconditionally would silently stomp a value set from the dashboard and make the setting untunable without a deploy. As a default it stays overridable. Drops the app.json entry that set the same variable. It only applied at app creation, so keeping both left two places stating one value with no mechanism to keep them in step — and the config var would win for one-click apps while .profile governed everything else. Note this is Heroku-only: Render, Railway and docker-compose all build from the Dockerfile, which does not read .profile. README says so. Co-Authored-By: Claude Opus 5 --- .profile | 14 ++++++++++++++ README.md | 2 +- app.json | 4 ---- 3 files changed, 15 insertions(+), 5 deletions(-) create mode 100644 .profile 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/README.md b/README.md index 8c62e383..dc993951 100644 --- a/README.md +++ b/README.md @@ -313,7 +313,7 @@ All platforms with ephemeral filesystems require `STORAGE_BACKEND=s3` - see `.en **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. -**Also worth setting on a memory-tight host:** `MALLOC_ARENA_MAX=2`. 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. +**`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. diff --git a/app.json b/app.json index 2e9b312a..0325c28e 100644 --- a/app.json +++ b/app.json @@ -28,10 +28,6 @@ "DJANGO_SETTINGS_MODULE": { "value": "config.settings.production" }, - "MALLOC_ARENA_MAX": { - "description": "Caps glibc's per-thread malloc arenas. The default is 8 x nproc and dynos report the host's core count, so arenas proliferate and are never returned to the OS. Leave at 2 unless you have measured otherwise.", - "value": "2" - }, "ALLOWED_HOSTS": { "description": "Comma-separated list of allowed hostnames", "value": "*"