Skip to content

Refactor: reshape SizeLimitedMixin as a CacheWrapper so it composes with CacheStack / ReadOnlyCache #710

Description

@pmrv

Location

  • src/fleche/caches.py:880-946SizeLimitedMixin (mixin body).
  • src/fleche/caches.py:948-963SizeLimitedCache(SizeLimitedMixin, Cache)
    the only concrete class that inherits the mixin.
  • src/fleche/config.py:398-458cache_from_config's max_size branch,
    which routes to SizeLimitedCache and cannot combine with CacheStack,
    CachePool, or SshCache.

Problem

SizeLimitedMixin was written as a subclass mixin over Cache. That
choice locks the size-limit behaviour to a single point in the class
hierarchy:

  • SizeLimitedCache(SizeLimitedMixin, Cache) is the only supported
    combination today (caches.py:948).
  • You cannot cap a CacheStack — the closest workaround is to make each
    layer a SizeLimitedCache, which limits per layer rather than for the
    stack as a whole.
  • You cannot cap a CachePool, though CachePool is read-only so this is
    less pressing.
  • You cannot cap an SshCache (the remote's cache is remote; a local
    cap would need to sit above the wire).
  • You cannot combine read_only=True and max_size=N in the same config:
    ReadOnlyCache(SizeLimitedCache(…)) works at the class level, but
    cache_from_config (config.py:444-456) applies read_only outside
    SizeLimitedCache, so the mixin's save runs unlocked-out and then
    ReadOnlyCache rejects it — inverted policy.

Compare this with the codebase's three other cache-behaviour add-ons:

  • ReadOnlyMixin — field-free, base-free; composes onto CacheWrapper
    (ReadOnlyCache), FilteringMixin (FilteredCache), and _MultiCache
    (CachePool). Explicitly noted in the class docstring as designed to
    compose (caches.py:515-526).
  • FilteringMixin — a CacheWrapper subclass; composes with ReadOnlyMixin.
  • RefreshingCache — a CacheWrapper subclass.

SizeLimitedMixin is the odd one out: it takes Cache as its concrete
base rather than sitting at the wrapper layer. That is why its
__post_init__ (caches.py:902-906) rebuilds _keys via
self.query(QueryCall()) — because it needs direct access to
Cache.query; a wrapper would forward through self.cache.query.

Two second-order costs fall out:

  1. _keys bookkeeping doesn't work through the cache layer. When a
    SizeLimitedCache is placed inside a CacheStack (as stack[0]), a
    back-fill via CacheStack.load calls self.stack[0].save, which the
    mixin's save sees — fine. But a _transfer_one call on a sibling
    wrapper never reaches the mixin, so a load-driven fill can defeat the
    cap silently.
  2. Two locks on the same instance. SizeLimitedMixin._lock guards
    _keys (caches.py:930-945). Cache(PerKeyLockMixin, BaseCache) already
    holds a per-key op-context lock. save acquires both. Coarse-grained
    _lock around super().save serialises all writes on this cache,
    which is fine for correctness but throws away PerKeyLockMixin's
    parallelism win. Making the size-cap a wrapper lets it own its own
    lock without contending with the inner per-key locks.

Suggested refactor

Make size-limiting a CacheWrapper subclass so it composes onto any
BaseCache:

@dataclass(frozen=True)
class SizeLimitedCache(CacheWrapper):
    """Cache wrapper that caps the number of stored calls.

    Composes onto any BaseCache (plain Cache, CacheStack, ReadOnlyCache,
    FilteredCache, SshCache). Tracks the set of stored keys and, on save,
    evicts down to ``max_size`` using :meth:`_pick_eviction_target`.
    """
    max_size: int
    _lock: _PicklableRLock = field(default_factory=_PicklableRLock,
                                    init=False, repr=False, compare=False)
    _keys: set[str] = field(init=False, repr=False, compare=False)

    def __post_init__(self):
        # Populate _keys from whatever the wrapped cache reports.
        object.__setattr__(self, "_keys",
                           {c.to_lookup_key() for c in self.cache.query(QueryCall())})

    def save(self, call):
        with self._lock:
            key = self.cache.save(call)
            self._keys.add(key)
            self._enforce_size_limit()
            return key

    def evict(self, key):
        with self._lock:
            self.cache.evict(key)
            self._keys.discard(str(key))

    def _pick_eviction_target(self, keys):
        return random.choice(keys)

    def _enforce_size_limit(self):
        with self._lock:
            while len(self._keys) > self.max_size:
                self.evict(self._pick_eviction_target(list(self._keys)))

The rest of the surface (load, contains, load_value, expand,
_shrink, _query) is already handled by CacheWrapper's
delegating base. _operation_context is already forwarded to the inner
cache by CacheWrapper._operation_context (caches.py:502-512), so
_transfer_one's TOCTOU protection continues to work through the wrapper.

cache_from_config (config.py) then applies max_size after constructing
the inner cache and before read_only, so read_only=True, max_size=100
becomes ReadOnlyCache(SizeLimitedCache(Cache(...))) — the intuitive
composition. TOML gains the ability to spell:

[[capped_stack]]                  # a CacheStack layer
values.type = "memory"
calls.type  = "memory"

[[capped_stack]]                  # another layer
values.type = "cloudpickle"
values.root = "~/.fleche/values"
calls.type  = "cloudpickle"
calls.root  = "~/.fleche/calls"

[capped_stack_top]
# wrap the whole stack with a 100-call cap
cache = "capped_stack"
max_size = 100

An alternative that keeps the subclass form: leave SizeLimitedCache
where it is and add a separate SizeLimitedWrapper (with the same
behaviour but wrapper-based). Simpler diff, but two ways to do the same
thing — worse for maintainability.

Pros

  • Composition, not inheritance. Any cache can be size-capped:
    stacks, pools (once the pool is writeable — currently read-only, so a
    pool cap is moot), remote-backed caches, filtered caches.
  • Config becomes flexible. max_size + read_only compose in the
    order the user reads them; [[stack]] max_size=100 works.
  • Aligns with the codebase's existing wrapper pattern.
    ReadOnlyMixin, FilteringMixin, RefreshingCache all live at the
    wrapper layer already; this is the last odd-one-out. Fewer patterns to
    keep in a reader's head.
  • Simpler locking. The wrapper owns its _lock; the inner
    cache's per-key locks are not double-taken by the mixin path.
  • Doesn't require touching Cache proper. Cache's __post_init__,
    PerKeyLockMixin reparenting from feat(cache): BaseCache(OperationContext) + PerKeyLockMixin on Cache (steps 4-5 of #569) #622, and the _transfer_one /
    redigest cross-key logic all stay put.

Cons

  • _pick_eviction_target overriding becomes wrapper-scoped. Today a
    user writing class LRUCache(SizeLimitedCache) gets a subclass of
    Cache and can rely on self.values / self.calls being available.
    In the wrapper form they'd have self.cache instead. Straightforward
    rename but visible to any downstream fork.
  • _keys hydration cost is the same as today — full .query()
    scan at construction. Not made worse; also not made better. (That is
    an orthogonal perf issue, related to ## Issue 4 — Iterable / dict digest hex-encodes every child (~12 % of digest() for big iterables) #440.)
  • __reduce__ / pickle round-trip needs verification. _keys and
    _lock are init=False; _PicklableRLock already survives pickle
    (thread_safe.py); _keys rebuilds in __post_init__ from the
    wrapped cache's query, which requires the wrapped cache to be
    unpickled first. Standard for frozen dataclasses but worth explicit
    coverage.
  • Config migration. Existing SizeLimitedCache(SizeLimitedMixin, Cache) instances live in serialised TOML today. cache_from_config's
    interpretation needs to change so max_size wraps the inner cache
    rather than picking a different concrete class. Behaviour-preserving
    for a stand-alone max_size + values/calls config; a semver-minor
    change under strict definitions.
  • No new external dependencies. Net code delta is small (~40 lines
    reshuffled).

Complexity / maintainability

Medium. Behaviour-preserving for the common path (max_size= on a plain
cache). The failure modes are compositional (does the size cap survive
through a CacheStack? through a ReadOnlyCache wrap?) so new tests
should target the composed shapes. Existing
tests/unit/caches/test_size_limited.py pins the current behaviour;
extend with test_size_limited_stack.py / test_size_limited_readonly.py.

Out of scope

  • Alternative eviction policies (LRU/LFU) — _pick_eviction_target
    stays the override hook.
  • Per-value-storage size caps (bytes instead of counts). Separate policy
    question.
  • The _keys hydration cost at open time (issue ## Issue 4 — Iterable / dict digest hex-encodes every child (~12 % of digest() for big iterables) #440 territory).
  • The read-only-back-fill hazard on stacks — the mixin's blindspot on
    _transfer_one from siblings is fixed by the wrapper form
    automatically, but pathological compositions (nested stacks) may need
    their own tests.
  • Any tie-in with Cache.gc (caches.py:418) — gc runs on the wrapped
    cache; the wrapper's _keys set is not affected.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions