Skip to content

fix(static-cache): count each outcome once per request, for real - #3

Merged
sylvesterdamgaard merged 1 commit into
mainfrom
fix/static-cache-outcome-count
Sep 23, 2026
Merged

sylvesterdamgaard merged 1 commit into
mainfrom
fix/static-cache-outcome-count

Conversation

@sylvesterdamgaard

Copy link
Copy Markdown
Contributor

The guard in StaticCacheTelemetry::recordOutcome() promised "count each outcome once per request", but RECORDED_KEY was a single last-value slot:

if ($request->attributes->get(self::RECORDED_KEY) === $result) {
    return;
}

That suppresses an immediate repeat and nothing else. The very sequence the comment was written for goes miss → write → miss, and the write in the middle clears the way for the second miss to be counted.

It is real, not theoretical

The application cacher only reports a page once its prepared response has been snapshotted, so a probe after cachePage() genuinely misses again. Driving the sequence through the real cacher on main:

operation=miss  value=2.0
operation=write value=1.0

One page serve, two misses. statamic.static_cache.operations is what a hit-ratio panel divides by, so this doesn't just add noise — it biases the ratio toward misses, and only for pages that were actually rendered and cached.

The fix

The promise was right; the implementation was wrong. The request attribute now holds the set of outcomes already counted rather than the last one.

A second effect worth naming: the repeat probe used to overwrite the span's statamic.static_cache attribute back to miss after the write. It now stays write, which is what recordWrite()'s own docblock says should happen ("the span attribute is overwritten so the final value tells the fuller story"). The existing span assertion already expected write — it passed only because nothing exercised the repeat.

Test

tests/Feature/StaticCacheTest.php drives probe → cache → probe through the resolved cacher and asserts the vocabulary plus the per-series totals, using 2.4's metric inspection:

$operations->assertLabelValues('operation', ['miss', 'write']);

expect($operations->withLabels(['operation' => 'miss'])->total())->toBe(1.0)
    ->and($operations->withLabels(['operation' => 'write'])->total())->toBe(1.0);

Before the fix it fails with Failed asserting that 2.0 is identical to 1.0. Note that assertLabelValues passes either way — the vocabulary was never wrong, only the counts — which is why this needed the totals and not just an exact label set.

pint, PHPStan and 78 tests green.

Note for whoever merges

Touches tests/Feature/StaticCacheTest.php, as does #2 (additively, at the end of the file). Whichever lands second wants a trivial rebase.

The guard's comment promised "count each outcome once per request", but
RECORDED_KEY was a single last-value slot, so it only suppressed immediate
repeats. The very sequence the comment describes — a middleware probe
misses, the rendered page is handed to the cacher, nocache probes again —
went miss -> write -> miss and counted the miss twice. The application
cacher only reports a page once its prepared response is snapshotted, so
that second probe really does miss. One page serve counted as two misses
is a hit ratio that lies.

Remember every outcome already counted instead of the last one. The repeat
probe now also leaves the span's statamic.static_cache attribute on
`write` rather than reopening it as a miss.

Covered by a test that drives the sequence through the real cacher: it
fails with `2.0 is identical to 1.0` before the fix.
@sylvesterdamgaard
sylvesterdamgaard force-pushed the fix/static-cache-outcome-count branch from 27c8891 to bb04666 Compare September 23, 2026 17:35
@sylvesterdamgaard
sylvesterdamgaard merged commit 610150e into main Sep 23, 2026
5 checks passed
@sylvesterdamgaard
sylvesterdamgaard deleted the fix/static-cache-outcome-count branch September 23, 2026 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant