Skip to content

Decide degrade-vs-propagate per cache failure site (they currently disagree) #190

Description

@koriym

Five places in this package decide what happens when the cache path fails, and they do not agree with each other. Three propagate, two degrade. PR #178 made every one of them visible in the log without changing any of them, so the decision can now be taken on evidence rather than on reading code — but it is still open, and one of the disagreements only surfaced while documenting the log.

Where it stands today

Site On failure Verified at
#[Cacheable] read cache_error{read} + trigger_error(), then degrade (run the resource) CacheInterceptor:61-66
#[Cacheable] write cache_error{write} + trigger_error(), then degrade (return the response) CacheInterceptor:90-100
donut write cache_error{write}, then propagate AbstractDonutCacheInterceptor:103-106
304 / ETag read cache_error{read} + close cache_miss{etag}, then propagate HttpCache:44-60, CliHttpCache
CDN purge local pools invalidated first, invalidate{cdn: failed} logged, then propagate ResourceStorage:230-237

Why this is not one rule

A blanket "the cache must never fail a request" would be wrong at two of these sites, so the issue is a decision per site, not a refactor:

  • CDN purge must stay fail-closed. Stale content at the edge is worse than an error, and the propagation is deliberate: the local pools are cleared before the exception leaves, so a caller that retries does not double-invalidate. Recommend: no change.
  • The donut write catch is not only the pool. It wraps putStatic()/putDonut(), which render. A template bug and a Redis outage arrive at the same catch. Degrading would swallow the template bug and re-render on every request, silently — and the log distinguishes them only after the fact, by exceptionClass. Deciding this one means deciding whether a renderer failure should surface as a 500.
  • The 304 path is the strongest candidate for degrading. A pool read failure currently turns a conditional request into an error response, where falling back to a full 200 would be correct and invisible to the client. But it is a change to the HttpCacheInterface contract, and the bootstrap owns that call.
  • The two #[Cacheable] sites already degrade and should stay as they are.

The disagreement found while writing the docs

The same asymmetry runs through the skip threshold, not just the failure path:

Path Skips the write when Effect on a 203
#[Cacheable] $ro->code !== 200 (CacheInterceptor:80) not stored, and purged
donut $ro->code >= 400 (AbstractDonutCacheInterceptor:69) stored

So the same 203 response is cached on one path and evicted on the other. docs/reading-the-log.md now states both thresholds because a reader meets the code in the log, but whether the difference is intended is unknown. The #[Cacheable] side is pinned behaviourally (SemanticLogSchemaTest::testNon200GetLogsPutSkippedWithActualCode); the donut side has no fixture, so nothing would notice if it changed.

What to do

  1. Decide each of the five sites explicitly, and record the decision where the code is — a comment at each catch saying which rule it follows and why, since "they all do the same thing" is exactly the assumption that is false today.
  2. Decide the 203 asymmetry, and pin whichever answer is chosen on the donut side too.
  3. If any site changes, it is a behaviour change: it needs its own entry in the CHANGELOG under Changed, not Fixed, because the current behaviour is 1.x behaviour and not a bug.

What the log gives you to decide with

Since #178, each site records its own outcome, so the frequency and the cause mix are measurable before the decision rather than guessed: cache_error{operation, exceptionClass} separates the pool from the renderer, and invalidate{cdn} separates a purge that never ran from one that failed. Note that ProdQueryRepositoryLogModule deliberately drops read-side cache_error (the application's warning channel already reports it), so the read-side frequency has to come from a dev/staging log or from the warning channel.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions