feat(cache): classify private and dynamic render downgrades#1247
Conversation
Render observations recorded dynamic request usage but did not carry an owned cacheability downgrade decision. That left future cache proof consumers to infer privacy from raw request API observations and cacheability strings. Add pure downgrade classification for public variants, private request state, auth/session dimensions, uncacheable draft state, dynamic fetches, and incomplete observations. Bump the cache proof schema for the serialized observation shape and keep the disabled proof model as the only reuse decision.
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped CACHE-08 classification slice. The downgrade hierarchy is sound, the type discriminants are exhaustive via assertNever, the fallback escalation logic is correct, and the existing test suite passes. Two notes below — one is a genuine double-normalization concern, the other is a minor test coverage observation.
Overall this is a good incremental step that makes the downgrade decision explicit without overreaching into cache-hit authority.
| target = maxCacheProofDowngradeTarget(target, reason.target); | ||
| } | ||
|
|
||
| const requestApis = normalizeRequestApiObservations(input.requestApis); |
There was a problem hiding this comment.
Double normalization of requestApis.
buildRenderObservation (line 1030) normalizes input.requestApis via normalizeRequestApiObservations and then passes the already-normalized result into classifyRenderObservationDowngrade (line 1047). But classifyRenderObservationDowngrade calls normalizeRequestApiObservations again here on line 993.
This is harmless today — normalization is idempotent (dedup-by-max-rank, then sort) — and the test on cache-proof.test.ts:399 proves the round-trip produces identical results. But it's still redundant work on the hot path. More importantly, it means the public API contract of classifyRenderObservationDowngrade accepts denormalized input, which could surprise a future caller who expects pre-normalized data to pass through unchanged without the extra allocation.
Two options:
- Remove the
normalizeRequestApiObservationscall here and document thatClassifyRenderObservationDowngradeInput.requestApismust be pre-normalized. - Keep it (defense in depth) and add a one-line comment explaining the idempotency expectation.
Either is fine. Not blocking.
| switch (target) { | ||
| case "public": | ||
| case "publicVariant": | ||
| case "private": |
There was a problem hiding this comment.
Design note: private target returns null fallback (no breaker), which means a render that only observes cookies/headers won't produce a CacheProofBreakerFallback. This is correct for the current model — private is still cacheable in a per-user cache, so no breaker is needed. Just calling it out since the jump from private → null to privateUncacheable → breaker is the critical semantic boundary and it's easy to miss in the fall-through.
| "CP_DOWNGRADE_PUBLIC_REQUEST_API", | ||
| "CP_DOWNGRADE_PUBLIC_REQUEST_API", | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Nice coverage for publicVariant, private, privateUncacheable, and freshRender targets. Two small gaps that could be filled in a follow-up:
-
Base case
"public"target — a render observation withcacheability: "public",completeness: "complete", no dynamic fetches, and no observed request APIs should producetarget: "public",reasons: [],fallback: null,isPublicCacheCandidate: true. This documents the zero-downgrade baseline. -
status: "unknown"request API — exercises theCP_DOWNGRADE_UNKNOWN_REQUEST_APIbranch inclassifyRenderObservationDowngrade. Currently uncovered.
Neither blocks this PR.
|
Review submitted. Here's a summary of the review: Approved with three inline comments:
|
| export const CACHE_PROOF_MODEL_SCHEMA_VERSION = 0; | ||
| export type CacheProofModelSchemaVersion = 0; | ||
| export const CACHE_PROOF_MODEL_SCHEMA_VERSION = 1; | ||
| export type CacheProofModelSchemaVersion = 1; |
There was a problem hiding this comment.
couldnt this type's value be inferred from the constant?
| export type CacheProofDowngradeTarget = | ||
| | "freshRender" | ||
| | "private" | ||
| | "privateUncacheable" | ||
| | "public" | ||
| | "publicVariant"; |
There was a problem hiding this comment.
this type could just be a union of the all the target values in the below type
| default: | ||
| return assertNever(kind); |
There was a problem hiding this comment.
im a bit surprised we need these to be honest
2 participants
Summary
Implements
#726-CACHE-08from issue #726.This adds explicit downgrade classification metadata to cache-proof render observations and cache variant dimension sources:
paramsandsearchParamsclassify aspublicVariantcookiesandheadersclassify asprivateprivateUncacheableconnection(), dynamic fetches, incomplete observations, and unknown observations classify asfreshRenderBonk: please read issue #726 before reviewing this PR. The architectural big picture matters here: this is a narrow
#726-CACHE-08classification slice, not cache-hit authority or static layout artifact reuse.Why
The disabled cache-proof model already records request API usage, dynamic fetches, cacheability, and boundary outcomes, but future proof consumers would otherwise need to infer privacy and freshness rules from raw observations. This PR makes the downgrade decision explicit, typed, and attached to the observation that produced it.
The model schema is bumped because serialized render observations now carry a
downgradefield.What Changed
CacheProofDowngradeTarget, downgrade reasons, andCacheProofDowngradeClassification.buildRenderObservation()after dynamic fetch URLs are redacted.CACHE_PROOF_MODEL_SCHEMA_VERSIONfrom0to1and derived cache key prefixes from the schema version constant.Non-Goals
isPublicCacheCandidatea cache authority.Validation
vp test run tests/cache-proof.test.tsvp check packages/vinext/src/server/cache-proof.ts tests/cache-proof.test.tsvp test run tests/cache-proof.test.ts tests/app-elements.test.ts tests/app-page-render.test.ts tests/app-page-cache.test.ts tests/isr-cache.test.tsvp checkgit diff --checkvp checkandknipRefs #726.