Skip to content

Cache Last-Modified in Redis to avoid DB hits on conditional GETs #596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jyggen wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Cache Last-Modified in Redis to avoid DB hits on conditional GETs #596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 50 additions & 21 deletions api/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from django.http import Http404
from django.shortcuts import get_object_or_404
from django.utils import timezone
from django.utils.http import parse_http_date_safe
from djmoney.money import Money
from drf_spectacular.utils import OpenApiParameter, extend_schema
from rest_framework import mixins, status, viewsets
Expand Down Expand Up @@ -77,6 +78,7 @@
WishListItemReadSerializer,
WishListSerializer,
)
from comicsdb.cache import get_last_modified, set_last_modified
from comicsdb.filters.collection import CollectionFilter
from comicsdb.filters.issue import IssueFilter
from comicsdb.filters.name import ComicVineFilter, NameFilter, UniverseFilter
Expand Down Expand Up @@ -111,26 +113,54 @@ class ReadingListItemsPagination(PageNumberPagination):


class CachedObjectMixin:
"""Memoizes get_object() per request."""

def get_object(self):
if not hasattr(self, "_cached_object"):
self._cached_object = super().get_object()

return self._cached_object


class ConditionalRetrieveModelMixin(CachedObjectMixin, mixins.RetrieveModelMixin):
def retrieve(self, request, *args, **kwargs):
retrieve = last_modified(last_modified_func=self._retrieve_last_modified)(super().retrieve)

return retrieve(self, request, *args, **kwargs)
class LastModifiedMixin(CachedObjectMixin):
"""Supplies `_last_modified` via a plain DB fetch."""

def _retrieve_last_modified(self, *args, **kwargs):
def _last_modified(self, *args, **kwargs):
obj = self.get_object()

if obj and getattr(obj, "modified", None):
return obj.modified
return getattr(obj, "modified", None) if obj else None


class CachedLastModifiedMixin(LastModifiedMixin):

@bpepple bpepple Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing preventing this mixin from being used on a per-user-filtered viewset is this docstring — there's no runtime check enforcing it structurally.

ReadingListViewSet (below, line 668) is a concrete near-miss: its get_queryset() filters by self.request.user (public lists + own lists), which is exactly the shape this docstring warns about, and it currently avoids the mixin correctly. But nothing stops a future PR from copy-pasting the pattern from SeriesViewSet/ArcViewSet onto it — it already supports conditional GET, so it's a very plausible next candidate. If that happens, it silently reintroduces the exact cross-user 304 leak that test_conditional_request_cannot_leak_other_users_item (tests/user_collection/test_api_collection.py:85) was written to catch for CollectionViewSet specifically — but that test doesn't generalize to a new viewset.

Rather than relying on every future contributor reading this docstring, the mixin could require each subclass to explicitly declare that it's safe, and fail loudly at class-definition time (i.e. at Django startup, since urls.py imports every viewset) if that declaration is missing or wrong:

class CachedLastModifiedMixin(LastModifiedMixin):
    """Answers conditional-GET checks from Redis, skipping the DB on a cache hit.

    Every subclass must explicitly set `queryset_is_user_scoped = False` (it is
    never inherited) to opt in - a cache hit skips per-user queryset filtering,
    so combining this with a viewset whose queryset filters by request.user
    would leak other users' rows via a false 304.
    """

    def __init_subclass__(cls, **kwargs):
        super().__init_subclass__(**kwargs)

        if "queryset_is_user_scoped" not in cls.__dict__:
            raise TypeError(
                f"{cls.__name__} must explicitly declare "
                "`queryset_is_user_scoped = False` to use CachedLastModifiedMixin"
            )
        if cls.queryset_is_user_scoped:
            raise TypeError(
                f"{cls.__name__} declares queryset_is_user_scoped = True; "
                "CachedLastModifiedMixin skips per-user filtering on a cache "
                "hit and must not be combined with a user-scoped queryset"
            )

Note this is a two-part change, not a drop-in mixin edit: adding __init_subclass__ alone would break the app at startup, since none of the 9 current viewsets (ArcViewSet, CharacterViewSet, CreatorViewSet, ImprintViewSet, IssueViewSet, PublisherViewSet, SeriesViewSet, TeamViewSet, UniverseViewSet) declare the attribute yet. Each of them would also need one added line: queryset_is_user_scoped = False. That's a small one-time cost, but it converts "silently reintroduces a data leak" into "the app fails to start with a clear message" the moment someone adds the mixin to a user-scoped viewset — a much stronger guarantee than a docstring, and one that doesn't depend on a test being written or run.

"""Answers conditional-GET checks from Redis, skipping the DB on a cache hit.

Only use on viewsets whose queryset isn't filtered by request.user - a cache
hit skips that filtering, which would leak other users' rows via a 304.
"""

def _last_modified(self, request, *args, **kwargs):
if_modified_since = parse_http_date_safe(request.META.get("HTTP_IF_MODIFIED_SINCE", ""))

if if_modified_since is not None:
pk = self.kwargs.get(self.lookup_url_kwarg or self.lookup_field)
cached = get_last_modified(self.get_queryset().model, pk) if pk else None

@bpepple bpepple Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calls the viewset's overridden get_queryset() on every conditional request just to read .model — but .model is static and already known via the class-level queryset attribute every one of the 9 cached viewsets declares (e.g. queryset = Arc.objects.all()).

For IssueViewSet, SeriesViewSet, CharacterViewSet, TeamViewSet, UniverseViewSet, and ImprintViewSet, get_queryset() is overridden with several select_related/prefetch_related chains (IssueViewSet's alone has 5 select_related fields, 7 prefetch_related entries including two nested Prefetch objects, plus Avg/Count annotations). None of that triggers a DB round trip since querysets are lazy, but constructing all those queryset/expression objects on every single conditional GET — including cache hits, which this feature is specifically trying to make cheap — is wasted CPU work.

self.queryset.model gets the same class with none of that construction:

cached = get_last_modified(self.queryset.model, pk) if pk else None


if cached is not None and int(cached.timestamp()) <= if_modified_since:

@bpepple bpepple Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a cache hit, this returns the cached timestamp directly without ever calling get_object() to confirm the row still exists in the DB.

There are two ways this can currently go stale and mask a 404 as a 304:

  1. Rolled-back create — if LastModifiedCacheMixin.save() (see comment on comicsdb/models/common.py) writes to the cache before the surrounding transaction commits, a rollback leaves a phantom entry for a pk that was never actually persisted.
  2. Delete invalidation failure — post_delete_last_modified in comicsdb/signals.py clears the cache via _safe_delete_many, which swallows Redis errors. If that call fails for a real, committed delete, the stale entry survives and this fast path has no way to notice.

Both cases mean a conditional GET for that pk returns 304 Not Modified instead of 404, for up to LAST_MODIFIED_CACHE_TTL (30 days) — since this path has no fallback check against the DB.

Worth considering whether the fast path should periodically re-verify via get_object(), or whether the write/invalidation side needs to guarantee it can never get out of sync with the DB in the first place.

@bpepple bpepple Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on trigger 2 above (delete invalidation failure): the fix isn't to stop swallowing the exception entirely — there's no Sentry/ADMINS email configured in this project, so letting it raise into the request would just turn a cache-consistency issue into an unhandled 500 for whoever triggered the delete, with no extra visibility to show for it.

The more useful fix is in _safe_delete_many (comicsdb/cache.py): retry a couple of times, since Redis blips are often transient, and if it still fails, escalate to ERROR instead of WARNING so it's distinguishable from the read/write-populate failures _safe_get/_safe_set tolerate. Those two can stay exactly as they are — a failed populate is self-healing on the next read, but a failed invalidation isn't; the stale entry just sits there until the TTL expires. That asymmetry is worth keeping explicit rather than reusing the same wrapper for all three.

def _safe_delete_many(keys, *, retries=3):
    """Best-effort cache invalidation, retried because a failure here — unlike
    _safe_get/_safe_set — has no self-healing path: the stale entry keeps
    serving until it's naturally overwritten or the TTL expires.
    """
    for attempt in range(1, retries + 1):
        try:
            cache.delete_many(keys)
            return
        except Exception:  # noqa: BLE001
            if attempt == retries:
                LOGGER.error(
                    "Failed to invalidate cache keys %s after %d attempts; "
                    "stale entries will serve until TTL expiry (%ds)",
                    keys, retries, LAST_MODIFIED_CACHE_TTL, exc_info=True,
                )
            else:
                LOGGER.warning(
                    "Retrying cache invalidation for %s (attempt %d/%d)",
                    keys, attempt, retries, exc_info=True,
                )

return cached

dt = super()._last_modified(request, *args, **kwargs)

return None
if if_modified_since is not None and dt is not None:
set_last_modified(self.get_object())

@bpepple bpepple Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no compare-and-swap here, so a slower request can overwrite a fresher value a concurrent writer already cached:

  • Request A (a conditional GET) reads Arc.modified = M1 from the DB at T0.
  • Concurrently, request B (a PATCH) commits modified = M2, and its own save() write-through caches M2 at T1 (T0 < T1).
  • Request A's write-through here fires at T2 (T1 < T2), overwriting the cache back down to the stale M1.

A client that already received Last-Modified: M2 from B's response and later polls with If-Modified-Since: M2 now finds the cache holding M1 <= M2 and gets a false 304, silently missing a real update.

set_last_modified could guard against this cheaply by only writing if the new value is newer than what's currently cached:

def set_last_modified(instance) -> None:
    modified = getattr(instance, "modified", None)
    if modified is None:
        return

    key = last_modified_cache_key(instance.__class__, instance.pk)
    current = _safe_get(key)
    if isinstance(current, int) and current >= int(modified.timestamp()):
        return

    _safe_set(key, int(modified.timestamp()), LAST_MODIFIED_CACHE_TTL)

This doesn't close the race entirely — the get-then-set pair still isn't atomic — but it shrinks the window from "however long request processing takes" down to a single round trip, which should be enough given how rarely two writes to the same row land within milliseconds of each other. A fully atomic fix would need a Redis-native compare-and-set (e.g. a Lua script or ZADD GT), which is probably more than this feature needs.


return dt


class ConditionalRetrieveModelMixin(LastModifiedMixin, mixins.RetrieveModelMixin):
def retrieve(self, request, *args, **kwargs):
retrieve = last_modified(last_modified_func=self._last_modified)(super().retrieve)

return retrieve(self, request, *args, **kwargs)


class UserTrackingMixin:
Expand All @@ -143,7 +173,7 @@ def perform_update(self, serializer):
serializer.save(edited_by=self.request.user)


class IssueListMixin(CachedObjectMixin):
class IssueListMixin(LastModifiedMixin):
"""Mixin to provide a standard issue_list action for related models."""

def get_issue_queryset(self, obj):
Expand All @@ -155,9 +185,7 @@ def get_issue_queryset(self, obj):
@extend_schema(responses={200: IssueListSerializer(many=True)}, filters=False)
@action(detail=True)
def issue_list(self, request, *args, **kwargs):
issue_list = last_modified(last_modified_func=self._issue_list_last_modified)(
self._issue_list
)
issue_list = last_modified(last_modified_func=self._last_modified)(self._issue_list)

return issue_list(self, request, *args, **kwargs)

Expand All @@ -171,16 +199,9 @@ def _issue_list(self, request, *args, **kwargs):
return self.get_paginated_response(serializer.data)
raise Http404

def _issue_list_last_modified(self, *args, **kwargs):
obj = self.get_object()

if obj and getattr(obj, "modified", None):
return obj.modified

return None


class ArcViewSet(
CachedLastModifiedMixin,
UserTrackingMixin,
IssueListMixin,
mixins.CreateModelMixin,
Expand Down Expand Up @@ -212,6 +233,7 @@ def get_serializer_class(self):


class CharacterViewSet(
CachedLastModifiedMixin,
UserTrackingMixin,
IssueListMixin,
mixins.CreateModelMixin,
Expand Down Expand Up @@ -251,6 +273,7 @@ def get_serializer_class(self):


class CreatorViewSet(
CachedLastModifiedMixin,
UserTrackingMixin,
mixins.CreateModelMixin,
ConditionalRetrieveModelMixin,
Expand Down Expand Up @@ -301,6 +324,7 @@ def create(self, request, *args, **kwargs) -> Response:


class ImprintViewSet(
CachedLastModifiedMixin,
UserTrackingMixin,
mixins.CreateModelMixin,
ConditionalRetrieveModelMixin,
Expand Down Expand Up @@ -343,6 +367,7 @@ def get_serializer_class(self):


class IssueViewSet(
CachedLastModifiedMixin,
UserTrackingMixin,
mixins.CreateModelMixin,
ConditionalRetrieveModelMixin,
Expand Down Expand Up @@ -414,6 +439,7 @@ def get_serializer_class(self):


class PublisherViewSet(
CachedLastModifiedMixin,
UserTrackingMixin,
mixins.CreateModelMixin,
ConditionalRetrieveModelMixin,
Expand Down Expand Up @@ -479,6 +505,7 @@ class RoleViewset(mixins.ListModelMixin, viewsets.GenericViewSet):


class SeriesViewSet(
CachedLastModifiedMixin,
UserTrackingMixin,
IssueListMixin,
mixins.CreateModelMixin,
Expand Down Expand Up @@ -562,6 +589,7 @@ class SeriesTypeViewSet(mixins.ListModelMixin, viewsets.GenericViewSet):


class TeamViewSet(
CachedLastModifiedMixin,
UserTrackingMixin,
IssueListMixin,
mixins.CreateModelMixin,
Expand Down Expand Up @@ -601,6 +629,7 @@ def get_serializer_class(self):


class UniverseViewSet(
CachedLastModifiedMixin,
UserTrackingMixin,
mixins.CreateModelMixin,
ConditionalRetrieveModelMixin,
Expand Down
12 changes: 12 additions & 0 deletions comicsdb/apps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from django.db.models.signals import m2m_changed, post_delete, post_save, pre_delete

from comicsdb.signals import (
post_delete_last_modified,
pre_delete_credit,
pre_delete_image,
update_arc_modified,
Expand Down Expand Up @@ -65,3 +66,14 @@ def ready(self):

credits_ = self.get_model("Credits")
pre_delete.connect(pre_delete_credit, sender=credits_, dispatch_uid="pre_delete_credits")

# Clear the cache entry on delete, so a removed row 404s instead of 304ing.
from comicsdb.models.common import LastModifiedCacheMixin # noqa: PLC0415

for model in self.get_models():

@bpepple bpepple Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every other signal connection in ready() is explicit and per-model (pre_delete.connect(pre_delete_image, sender=arc, ...), repeated by hand for character, creator, issue, publisher, team, variant), so scanning this file alone tells you which models participate in a given signal. This block breaks that convention — answering "which models get post_delete_last_modified?" now means grepping comicsdb/models/*.py for LastModifiedCacheMixin instead of reading apps.py, where every other answer to "which models get signal X?" lives.

That's a reasonable trade-off, not a bug — the loop is what lets a 10th cached model get wired automatically without an apps.py edit, which the explicit style can't do, and that's worth keeping. It's just a different registration strategy sitting unexplained next to seven examples of the other one, which could read as an oversight rather than a deliberate choice.

Worth extending the existing comment on the line above to say so explicitly, e.g.:

# Clear the cache entry on delete, so a removed row 404s instead of 304ing.
# Auto-registered (unlike the explicit connects above) so new LastModifiedCacheMixin
# models don't need an apps.py edit to get invalidation wired up.
from comicsdb.models.common import LastModifiedCacheMixin  # noqa: PLC0415

for model in self.get_models():
    if issubclass(model, LastModifiedCacheMixin):
        post_delete.connect(
            post_delete_last_modified,
            sender=model,
            dispatch_uid=f"post_delete_last_modified_{model._meta.model_name}",
        )

if issubclass(model, LastModifiedCacheMixin):
post_delete.connect(
post_delete_last_modified,
sender=model,
dispatch_uid=f"post_delete_last_modified_{model._meta.model_name}",
)
65 changes: 65 additions & 0 deletions comicsdb/cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
"""Redis-backed cache of each cacheable model's `modified` timestamp."""

import logging
from datetime import UTC, datetime

from django.core.cache import cache

LOGGER = logging.getLogger(__name__)

LAST_MODIFIED_CACHE_TTL = 60 * 60 * 24 * 30 # 30 days


def last_modified_cache_key(model, pk) -> str:
return f"modified:{model._meta.label_lower}:{pk}"


def _safe_get(key):
try:
return cache.get(key)
except Exception: # noqa: BLE001
LOGGER.warning("Failed to read cache key %s", key, exc_info=True)
return None


def _safe_set(key, value, timeout):
try:
cache.set(key, value, timeout)
except Exception: # noqa: BLE001
LOGGER.warning("Failed to write cache key %s", key, exc_info=True)


def _safe_delete_many(keys):
try:
cache.delete_many(keys)
except Exception: # noqa: BLE001
LOGGER.warning("Failed to delete cache keys %s", keys, exc_info=True)


def get_last_modified(model, pk) -> datetime | None:

@bpepple bpepple Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_last_modified reads the cached value as an int and converts it to a datetime here; the one production caller (api/views.py:148) then immediately converts it back to an int via int(cached.timestamp()) just to compare against if_modified_since — which is itself already an int, from parse_http_date_safe. That's two conversions per cache hit to compare two numbers that started and ended as ints.

(Django's condition decorator does require the final return value of _last_modified to be a real datetime — it calls .timestamp()/timezone.is_aware() on it directly — so that conversion can't be dropped entirely, but it only needs to happen once, on an actual hit, not before every comparison.)

Splitting the epoch read from the datetime conversion would remove the redundant round trip and let the comparison happen entirely in int space, matching parse_http_date_safe's own representation:

def get_last_modified_epoch(model, pk) -> int | None:
    """Cached `modified` for `model`/`pk` as epoch seconds, or None on a miss."""
    value = _safe_get(last_modified_cache_key(model, pk))
    return value if isinstance(value, int) else None


def get_last_modified(model, pk) -> datetime | None:
    """Cached `modified` for `model`/`pk`, or None on a miss."""
    epoch = get_last_modified_epoch(model, pk)
    return datetime.fromtimestamp(epoch, tz=UTC) if epoch is not None else None

get_last_modified stays as-is for its existing callers (it's used directly in tests/comicsdb/test_cache.py and tests/comicsdb/test_api_conditional_requests.py, comparing against datetime values). Only api/views.py's cache-hit check would switch to the epoch variant:

if if_modified_since is not None:
    pk = self.kwargs.get(self.lookup_url_kwarg or self.lookup_field)
    cached_epoch = get_last_modified_epoch(self.queryset.model, pk) if pk else None

    if cached_epoch is not None and cached_epoch <= if_modified_since:
        return datetime.fromtimestamp(cached_epoch, tz=UTC)

(assumes the self.queryset.model fix suggested above, and adds a from datetime import UTC, datetime import to api/views.py.)

"""Cached `modified` for `model`/`pk`, or None on a miss."""
value = _safe_get(last_modified_cache_key(model, pk))

if not isinstance(value, int):
return None

return datetime.fromtimestamp(value, tz=UTC)


def set_last_modified(instance) -> None:
"""Write-through cache update for a model instance."""
modified = getattr(instance, "modified", None)

if modified is None:
return

key = last_modified_cache_key(instance.__class__, instance.pk)
_safe_set(key, int(modified.timestamp()), LAST_MODIFIED_CACHE_TTL)


def delete_last_modified(model, pk) -> None:
_safe_delete_many([last_modified_cache_key(model, pk)])


def delete_last_modified_many(model, pks) -> None:
_safe_delete_many([last_modified_cache_key(model, pk) for pk in pks])
4 changes: 2 additions & 2 deletions comicsdb/models/arc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@
from sorl.thumbnail import ImageField

from comicsdb.models.attribution import Attribution
from comicsdb.models.common import CommonInfo, pre_save_slug
from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin, pre_save_slug
from users.models import CustomUser

LOGGER = logging.getLogger(__name__)


class Arc(CommonInfo):
class Arc(LastModifiedCacheMixin, CommonInfo):
image = ImageField(upload_to="arc/%Y/%m/%d/", blank=True)
attribution = GenericRelation(Attribution, related_query_name="arcs")
created_by = models.ForeignKey(
Expand Down
4 changes: 2 additions & 2 deletions comicsdb/models/character.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from sorl.thumbnail import ImageField

from comicsdb.models.attribution import Attribution
from comicsdb.models.common import CommonInfo, pre_save_slug
from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin, pre_save_slug
from comicsdb.models.creator import Creator
from comicsdb.models.team import Team
from comicsdb.models.universe import Universe
Expand All @@ -23,7 +23,7 @@
LOGGER = logging.getLogger(__name__)


class Character(CommonInfo):
class Character(LastModifiedCacheMixin, CommonInfo):
image = ImageField(upload_to="character/%Y/%m/%d/", blank=True)
alias = ArrayField(models.CharField(max_length=100), blank=True, default=list)
creators = models.ManyToManyField(Creator, blank=True, related_name="characters")
Expand Down
16 changes: 16 additions & 0 deletions comicsdb/models/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
from django.db.models.functions import Now
from django.utils.text import slugify

from comicsdb.cache import set_last_modified

MIN_RATING = 1
MAX_RATING = 5
RATING_CHOICES = [(i, str(i)) for i in range(MIN_RATING, MAX_RATING + 1)]
Expand Down Expand Up @@ -37,6 +39,20 @@ def pre_save_slug(sender, instance, **kwargs):
instance.slug = generate_slug_from_name(instance)


class LastModifiedCacheMixin(models.Model):

@bpepple bpepple Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are currently two independent, uncoordinated mechanisms keeping this cache in sync with the DB: the save() override here, and hand-placed transaction.on_commit(delete_last_modified...) calls wherever comicsdb/signals.py uses .filter(...).update(modified=...) to bypass save() (lines 24-25, 31-32, 55-58, 61-62). Nothing enforces that every future .update()/bulk_update() touching a cached model's modified field remembers the paired invalidation call — miss one, and the cache goes stale silently for up to LAST_MODIFIED_CACHE_TTL (30 days).

The exact same .filter(pk=...).update(modified=...) idiom already exists in pull_list/signals.py:7, reading_lists/signals.py:7, and wish_list/signals.py:7 for models that aren't cached yet. If any of those join the cached set later (plausible — ReadingListViewSet already supports conditional GET), those handlers would silently need the same treatment, with nothing to flag it.

Rather than relying on remembering to pair every .update() with an invalidation call, a custom queryset could make .update() invalidate itself:

class LastModifiedQuerySet(models.QuerySet):
    """Auto-invalidates the Redis cache for any bulk .update() on a
    LastModifiedCacheMixin model, since .update() bypasses save()."""

    def update(self, **kwargs):
        pks = frozenset(self.values_list("pk", flat=True))
        rows_updated = super().update(**kwargs)

        if pks:
            transaction.on_commit(
                lambda model=self.model, pks=pks: delete_last_modified_many(model, pks)
            )

        return rows_updated


class LastModifiedCacheMixin(models.Model):
    """Writes `modified` to comicsdb.cache on every save(), and invalidates it
    on every .update() (which bypasses save() and would otherwise leave a
    stale entry with no self-healing path).

    Use only on models whose viewset reads it via CachedLastModifiedMixin.
    """

    objects = LastModifiedQuerySet.as_manager()

    class Meta:
        abstract = True

    def save(self, *args, **kwargs):
        super().save(*args, **kwargs)
        set_last_modified(self)

This closes the gap for any future .update() call automatically, and as a bonus it lets the four existing hand-paired transaction.on_commit(delete_last_modified...) calls in comicsdb/signals.py be deleted — .update() now invalidates itself.

One prerequisite: Issue (comicsdb/models/issue.py:86-88) would need its objects, graphic_novels, and tpb custom managers removed — a repo-wide grep (including tests) turns up zero references to any of them outside their own definitions, so they look like dead code. Removing them would let Issue drop its objects override entirely and inherit LastModifiedQuerySet.as_manager() from this mixin cleanly, same as every other cached model.

"""Writes `modified` to comicsdb.cache on every save().

Use only on models whose viewset reads it via CachedLastModifiedMixin.
"""

class Meta:
abstract = True

def save(self, *args, **kwargs):

@bpepple bpepple Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

save() writes to the cache synchronously, right after super().save(), but every other cache mutation this PR adds (in comicsdb/signals.py) defers via transaction.on_commit(...).

If this save happens inside a transaction.atomic() block that later rolls back — e.g. IssueCreate.form_valid in comicsdb/views/issue.py, or AttributionCreateMixin/AttributionUpdateMixin in comicsdb/views/mixins.py, both of which save the main object and then a formset in the same atomic block — the DB change is undone but the Redis entry isn't, so it keeps serving the phantom/stale modified value for up to LAST_MODIFIED_CACHE_TTL (30 days). Combined with CachedLastModifiedMixin's cache-hit path not re-checking get_object(), this can turn into a false 304 for a row that was never actually committed.

Suggest matching the pattern used elsewhere in the PR:

def save(self, *args, **kwargs):
    super().save(*args, **kwargs)
    transaction.on_commit(lambda: set_last_modified(self))

@bpepple bpepple Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the transaction-ordering issue above: even on a fully committed save, if this set_last_modified(self) call fails (a transient Redis blip), the cache isn't left empty — it keeps serving whatever value was cached before this save, since write-through is the only invalidation path for a direct field save (no M2M/delete signal fires).

That makes this failure mode different from a cache miss: a miss self-heals on the next conditional GET (api/views.py:154 repopulates it), but an overwrite failure produces a stale hit — nothing detects it's wrong, so a real, committed edit can silently 304 as unchanged for up to LAST_MODIFIED_CACHE_TTL (30 days), until some later save happens to succeed and overwrite it.

Given that, _safe_set probably deserves the same retry-and-escalate treatment proposed for _safe_delete_many in the thread below, rather than staying a silent best-effort write — the "populate is self-healing" assumption that makes swallowing failures safe for reads doesn't hold for this particular caller.

super().save(*args, **kwargs)
set_last_modified(self)


class CommonInfo(models.Model):
name = models.CharField(max_length=255)
slug = models.SlugField(max_length=255, unique=True)
Expand Down
4 changes: 2 additions & 2 deletions comicsdb/models/creator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@
from sorl.thumbnail import ImageField

from comicsdb.models.attribution import Attribution
from comicsdb.models.common import CommonInfo, pre_save_slug
from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin, pre_save_slug
from users.models import CustomUser

LOGGER = logging.getLogger(__name__)


class Creator(CommonInfo):
class Creator(LastModifiedCacheMixin, CommonInfo):
birth = models.DateField("Date of Birth", null=True, blank=True)
death = models.DateField("Date of Death", null=True, blank=True)
image = ImageField(upload_to="creator/%Y/%m/%d/", blank=True)
Expand Down
4 changes: 2 additions & 2 deletions comicsdb/models/imprint.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@
from sorl.thumbnail import ImageField

from comicsdb.models.attribution import Attribution
from comicsdb.models.common import CommonInfo, pre_save_slug
from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin, pre_save_slug
from comicsdb.models.publisher import Publisher
from users.models import CustomUser

LOGGER = logging.getLogger(__name__)


class Imprint(CommonInfo):
class Imprint(LastModifiedCacheMixin, CommonInfo):
publisher = models.ForeignKey(Publisher, on_delete=models.CASCADE, related_name="imprints")
founded = models.PositiveSmallIntegerField("Year Founded", null=True, blank=True)
image = ImageField("Logo", upload_to="imprint/%Y/%m/%d", null=True, blank=True)
Expand Down
4 changes: 2 additions & 2 deletions comicsdb/models/issue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from comicsdb.models.arc import Arc
from comicsdb.models.attribution import Attribution
from comicsdb.models.character import Character
from comicsdb.models.common import CommonInfo
from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin
from comicsdb.models.creator import Creator
from comicsdb.models.rating import Rating
from comicsdb.models.series import Series
Expand Down Expand Up @@ -51,7 +51,7 @@ def get_queryset(self):
)


class Issue(CommonInfo):
class Issue(LastModifiedCacheMixin, CommonInfo):
series = models.ForeignKey(Series, on_delete=models.CASCADE, related_name="issues")
name = ArrayField(models.CharField("Story Title", max_length=150), blank=True, default=list)
title = models.CharField("Collection Title", max_length=255, blank=True)
Expand Down
Loading
Loading