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

[jyggen]

Conditional GETs (If-Modified-Since) are currently hitting the DB every time just to check a timestamp, making 304s (more or less) as taxing on the system as 200s are. This PR attempts to improve the situation a bit by caching each model's modified value in Redis so we can (most of the time) answer from there instead.

This is done by adding a CachedLastModifiedMixin for viewsets and wired it up on the models where it's safe to use (e.g. anything not filtered by request.user). The cache gets invalidated on delete and on related-object changes that bump a parent's modified. Reads/writes to Redis fail quietly if something goes wrong to avoid taking the API down when the cache is unavailable. The cache lives at most 30d and self-heals on 200s.

Disclosure: the main bulk of changes were written by me, but the tests + fixing some edge cases caught by said tests were AI assisted.

[bpepple]

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]

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.

[bpepple]

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]

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,
                )