For the nearly 10 Years I’ve been using Django and the 5 years I’ve been using Wagtail CMS, I’ve always wanted to contribute to open source. I’ve previously tried with a micro-optimisation in Django but it was rightly rejected. After first considering a monkey patch, I’ve finally made my first contribution.

Back in June I moved the Django caches on my employer project (Corsair Explorer) from a self-hosted Redis box onto ElastiCache Serverless. The initial box was a basic t3a.small AWS instance that was also hosting a ElasticSearch instance. Just after this I also changed some django.core.cache.backends.locmem.LocMemCache caches to point at the new ElastiCache instance.

Response times got worse after the changeovers but the extra reliability / worker memory savings it gave us made it seem an overall positive change. My assumption was the new network latancy was causing the performance regression.

A couple weeks ago I couldnt’ shake the feeling of something being off. Even with extra network latancy, the better cache hit ratio should be making up for that on the averages. Thankfully we push our ALB access logs into ClickHouse, so there were a few hundred million rows to query rather than going on vibes. Using Claude to parse and summarise the data i was left with this:

  p50 p90 p99
before 78ms 163ms 288ms
after 95ms 209ms 373ms

We were still serving requests quicker than our targets but it could be faster.

My initial guess was image renditions as I’ve seen these take up a large amount of queries in Streamfields in the past using Django Debug Toolbar. Wagtail caches them, most of our pages have a lot of images, and we’d swapped a near instant in-process dict for a TLS connection to another host.

I’d managed to guess the right problem but the wrong cause. When I split the same before/after by URL, our API endpoints had somehow regressed slightly more in percentage terms than the HTML pages had. So I patched the cache backend locally to count calls and rendered a few pages. About 40 writes per page render, against 4 reads. 🤔

Why

Pointing Claude at the Wagtail AbstractImage source code quickly led to the issue.

Here is get_rendition() in wagtail/images/models.py:

try:
    rendition = self.find_existing_rendition(filter)
except Rendition.DoesNotExist:
    rendition = self.create_rendition(filter)
    # Reuse this rendition if requested again from this object
    self._add_to_prefetched_renditions(rendition)

cache_key = Rendition.construct_cache_key(
    self, filter.get_cache_key(self), filter.spec
)
Rendition.cache_backend.set(cache_key, rendition)

The set() at the bottom isn’t conditional on anything. It runs for every image, regardless of where the rendition came from.

get_renditions(), the plural version a bit further down the same file, does this instead:

# Update the cache
cache_additions = {
    Rendition.construct_cache_key(
        self, filter.get_cache_key(self), filter.spec
    ): rendition
    for filter, rendition in renditions.items()
    # prevent writing of cached data back to the cache
    if not getattr(rendition, "_from_cache", False)
}
if cache_additions:
    Rendition.cache_backend.set_many(cache_additions)

There’s a _from_cache flag, and the plural method checks it before writing. The singular one, which is what the {% image %} tag ends up calling, doesn’t.

The fix

A super simple change that mimics the plural version.

# prevent writing of cached data back to the cache
if not getattr(rendition, "_from_cache", False):
    cache_key = Rendition.construct_cache_key(
        self, filter.get_cache_key(self), filter.spec
    )
    Rendition.cache_backend.set(cache_key, rendition)

While getting Claude to write unit tests after reviewing my PR, Matt Westcott spotted another issue:

Thanks @MasonLyons! Getting Claude to write unit tests for this has revealed a secondary issue - the _from_cache flag was only set when the result came from a prefetch query, not when it actually came from the cache. I’ll combine that fix (+ tests) with this one and merge.

So the check in get_renditions() had never worked for cache hits. Thankfully the docs tell you to prefetch_related("thumbnail__renditions"), and we do that in most places we can. We just unfortunatly can’t do it everywhere, mainly in templates.

My PR, merged as c1c39fe, should be going out in version 8.1.

Reflection

In a world full of vibe coding and AI slop it’s easy to just hand an LLM the keys and let it go to town. We’ve seen the massive amoune of genuine CVEs that have been found recently by automated LLM use. What’s interesting is Wagtail 5.1 was released on August 1st 2023, so just after LLMs were starting to ramp up but a fair few years after GitHub Copilot had been initially released.

So while Claude very likely could have found the issue all on it’s own eventually, It’s odd that this bug has been around for 3 years without it being spotted. With the ever increasing number of bots trying to spam PRs on open source projects and Wagtail being popular, You’d have thought it would have been spotted.

This shows you still need a level of programming and domain knowledge. LLMs still feel like a floor raiser and force multiplier. I had to steer it in roughly the right direction and Matt only got it to spot the issues when using it to write unit tests.

Thanks to Matt Westcott for being so quick with reviewing the initial PR.

<
Previous Post
How to add a custom language to Django
>
Blog Archive
Archive of all previous blog posts