Upstram merge - #100
Merged
Merged
Upstram merge#100
Conversation
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 <noreply@anthropic.com>
…read 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…ing them 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
`.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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…s-d3b6bf fix: get both Heroku dynos back under their 512MB quota
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Why?
How to test
Checklist
pytest)ruff check .andruff format --check .)