Review a Python Cache-Aside Helper

1. Cache stampede on popular keys

Major, line 11-18

When a hot key is missing (cold start, eviction), every concurrent request misses at once and runs the 300 ms query. At 2,000 requests per second that is hundreds of identical queries piling onto the database at the same moment.

Fix: Let one request rebuild while the others wait or serve stale data: a short Redis lock (SET key:lock 1 NX PX 5000), request coalescing in-process, or early probabilistic refresh. Add jitter to TTLs so keys do not expire together.

2. No expiry

Major, line 17

Entries never expire, so Redis grows until it hits maxmemory (and starts evicting or rejecting writes), and any missed invalidation is permanent.

Fix: Always set a TTL (r.set(key, value, ex=300)), even when you also invalidate.

3. Writes never invalidate the cache

Major, line 21-22

update_product saves to the database but leaves the old cached value, so customers see stale prices until the key is evicted, which, with no TTL, may be never.

Fix: Delete the key after the database commit (r.delete(key)), or publish an invalidation event that the other service consumes.

4. pickle on data from a network service

Major, line 14, 17

pickle.loads can execute arbitrary code. Anyone who can write to Redis (a misconfigured instance, another compromised service) gets code execution in the web app. Pickled objects also break when the class changes between deploys.

Fix: Store JSON (or msgpack) of a plain dict/dataclass, and keep Redis private with authentication.

5. Missing products are never cached, and falsy values miss

Minor, line 13, 16

if cached: treats an empty value as a miss, and a product that does not exist (None) is queried from the database on every request, which is an easy way to load the database on purpose.

Fix: Check is not None, and cache a short-lived "not found" marker (negative caching).

6. Redis failures take the page down

Minor, line 12, 17

If Redis is unreachable, r.get raises and the request fails, although the database could have served it.

Fix: Treat the cache as optional: catch redis.RedisError, log it, and fall back to the database (with a short timeout on the Redis client).

Caching questions are about consistency and failure, not speed. Strong reviews cover expiry, invalidation on writes, stampedes on hot keys, and what happens when the cache itself fails. Spotting pickle on network data is a bonus security finding.

import json
import random

import redis

from app.db import load_product, save_product

r = redis.Redis(socket_timeout=0.05)
TTL = 300
MISSING = b"__missing__"


def _key(product_id: int) -> str:
    return f"product:{product_id}"


def get_product(product_id: int) -> dict | None:
    key = _key(product_id)
    try:
        cached = r.get(key)
        if cached is not None:
            return None if cached == MISSING else json.loads(cached)
        # Only one request rebuilds a missing key; the others retry briefly.
        if not r.set(f"{key}:lock", 1, nx=True, px=5000):
            cached = _wait_for(key)
            if cached is not None:
                return None if cached == MISSING else json.loads(cached)
    except redis.RedisError:
        return load_product(product_id)  # cache is an optimisation, not a dependency

    product = load_product(product_id)
    value = MISSING if product is None else json.dumps(product)
    try:
        r.set(key, value, ex=TTL + random.randint(0, 60) if product else 30)  # jitter; short negative TTL
        r.delete(f"{key}:lock")
    except redis.RedisError:
        pass
    return product


def update_product(product_id: int, data: dict) -> None:
    save_product(product_id, data)
    r.delete(_key(product_id))  # invalidate after the write commits