fix(cache): verify edge cache eligibility before response#2582
fix(cache): verify edge cache eligibility before response#2582james-elicx wants to merge 15 commits into
Conversation
commit: |
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
/bigbonk review for issues |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk review
Reviewed the CDN edge-cache verification work. The core streaming verifier (verifyCdnCacheCandidateStream) is solid — bounded time/size, fail-closed handoff, lock release on all cancel/error paths, and the BoundedStreamBuffer coalescing is correct across the boundary cases I traced (partial-fill, shrunken final segment, exact-fill). Good test coverage for the adversarial cases.
I did find one real gap that undermines the fix under a specific deploy mode.
Blocking: pending-dynamic guard is bypassed under VINEXT_NEXT_DEPLOY_CACHE_CONTROL=1
applyCdnResponseHeaders short-circuits before delegating to the adapter:
// packages/vinext/src/server/cache-control.ts:34-41
export function applyCdnResponseHeaders(headers: Headers, input: CdnCacheableHeaderInput): void {
headers.delete("Cache-Control");
if (shouldUseNextDeployCacheControl() && isSharedCacheControl(input.cacheControl)) {
headers.set("Cache-Control", BROWSER_REVALIDATE_CACHE_CONTROL); // "public, max-age=0, must-revalidate"
return; // <-- adapter.buildResponseHeaders() never runs
}
const map = getCdnCacheAdapter().buildResponseHeaders(input);
...
}The check only looks at isSharedCacheControl(input.cacheControl) — it ignores input.pendingDynamicCheck. In edge mode, a production MISS is built with Cache-Control: s-maxage=60, stale-while-revalidate (resolveAppPageRscResponsePolicy/buildAppPageRscResponse), and applyPendingDynamicCdnHeaders forwards that value as input.cacheControl with pendingDynamicCheck: true when verification didn't complete. So when the deploy flag is on, the response ships public, max-age=0, must-revalidate for an unverified (potentially late-dynamic) render instead of the adapter's no-store — exactly the leak this PR exists to prevent.
This is reachable: scripts/e2e-deploy.sh:640 exports VINEXT_NEXT_DEPLOY_CACHE_CONTROL=1.
Reproduction (fails on this branch):
setCdnCacheAdapter(new CloudflareCdnCacheAdapter());
process.env.VINEXT_NEXT_DEPLOY_CACHE_CONTROL = "1";
const headers = new Headers({ "Cache-Control": "s-maxage=60, stale-while-revalidate" });
applyCdnResponseHeaders(headers, {
cacheControl: "s-maxage=60, stale-while-revalidate",
pendingDynamicCheck: true,
});
// Actual: "public, max-age=0, must-revalidate"
// Expected: something containing "no-store" (adapter's pending-dynamic response)Suggested fix: let pendingDynamicCheck take precedence, e.g. gate the next-deploy branch with !input.pendingDynamicCheck so the adapter's pending-dynamic path is always honored. Please add a regression test covering edge adapter + VINEXT_NEXT_DEPLOY_CACHE_CONTROL=1 + pendingDynamicCheck: true.
Non-blocking notes
- CodeQL alert (tests/app-cdn-cache-production.test.ts:58) — false positive. The
setTimeout(..., Number(delay) || 75)is a test-only local IO server whose "user-controlled" duration is supplied by the test itself. Safe to dismiss. - RSC vs HTML verification guard asymmetry — the RSC path (
app-page-render.ts:820) runsverifyCdnCacheCandidateStreamonshouldCompleteDynamicUsageBeforeResponsealone, while the HTML path (:1113) also gates on!dynamicUsedDuringRender. This is defensible since the RSC path consumesdynamicUsedDuringBuildafter verification and the stream needs draining anyway, but a short comment noting the intentional difference would help future readers.
|
Review posted to PR #2582. SummaryI reviewed the CDN edge-cache verification changes. The core One blocking issue found:
Non-blocking:
|
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
There was a problem hiding this comment.
/bigbonk review
Re-reviewed after the latest commits. The blocking issue from the previous review is fixed, and the new work holds up under scrutiny.
Previously-blocking issue: resolved
applyCdnResponseHeaders (cache-control.ts:36-43) now gates the VINEXT_NEXT_DEPLOY_CACHE_CONTROL=1 short-circuit on !input.pendingDynamicCheck, so an unverified (potentially late-dynamic) edge render no longer ships public, max-age=0, must-revalidate. It falls through to the adapter's no-store pending-dynamic path. Regression test added at tests/cache-control.test.ts:88 ("keeps pending streamed renders no-store in Next deploy mode"). Confirmed passing.
Verifier internals: verified independently
BoundedStreamBuffercoalescing — I built a standalone harness testing total-byte and content-order integrity across partial-fill, truncated final segment (remainingCapacity < segmentBytes), and exact-fill boundaries. All cases reassemble correctly. ThefinalLength = length % segmentBytescomputation is sound because a truncated final segment only occurs atlength == maxBytes, wheremaxBytes % segmentBytes == remainingCapacity, which equals the truncated segment's actual size.- Lock release / fail-closed — release on complete drain, deadline timeout, oversize, initial-read error, and resumed-read error are all covered by
tests/app-page-cdn-verification.test.ts(7 tests) and confirmed. Cancel-after-handoff has no unhandled-rejection leak (.catch(() => {})on the resume-path cancel). - Event-loop starvation — the periodic
nextTask()yield plus the elapsed-time checks keep a synchronous infinite stream bounded; the adversarial test confirms it.
Integration coverage: strong
tests/app-cdn-cache-production.test.ts (7 tests, real buildApp() + Workers-like edge) covers the important paths: deferred-personalized HTML and RSC both fail closed to no-store, deferred-but-static reuses the edge cache, revalidate=false edge-caches, deadline timeout / never-settling / oversized all fail closed. This is the right shape of coverage for the PR's goal.
CodeQL alert: now a hard non-issue
tests/app-cdn-cache-production.test.ts:58 no longer feeds the user-supplied delay into setTimeout — it's clamped to delay === "750" ? 750 : 75 (two hardcoded values). Beyond being a test-only local server, the flagged data flow is gone. Safe to dismiss.
Non-blocking notes
- RSC vs HTML verification-guard asymmetry — the RSC path (
app-page-render.ts:820) gates onshouldCompleteDynamicUsageBeforeResponsealone while the HTML path (:1113) also gates on!dynamicUsedDuringRender. This is correct because the RSC path re-readsconsumeDynamicUsage()after the drain (:834), soresolveAppPageRscResponsePolicyalready reflects any late-detected dynamic usage. A one-line comment noting the intentional difference would help future readers, but it's not required. hybridpage-cache mode — theCdnPageCacheModeunion defineshybrid, but no adapter uses it. The render branching (!== "origin"to verify,=== "edge"to skip origin write) already produces the documented hybrid semantics (verify + retain origin write), so this is correctly forward-compatible, just currently exercised only by the type/docs.- Default-adapter parity — with the default
originadapter,usesResponseEdgeCache/shouldCompleteDynamicUsageBeforeResponseare bothfalse, so there's no verification overhead or behavior change on the common path. Good.
Verified locally: cache-control, app-page-cdn-verification, app-cdn-cache-production, app-page-render, cloudflare-cdn-cache, cdn-cache, cache-adapters-config all pass; format/lint/type checks clean on the changed source files.
No blocking issues. LGTM.
|
Review posted to PR #2582. SummaryI re-reviewed the CDN edge-cache verification changes after the latest commits. The blocking issue from the prior review has been fixed and the rest of the PR holds up. Blocking issue — resolved: Independently verified:
Non-blocking notes: RSC vs HTML guard asymmetry is correct (RSC re-reads dynamic usage post-drain) but could use a comment; All targeted test suites pass locally and format/lint/type checks are clean. No blocking issues — LGTM. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review
Independently re-reviewed the CDN edge-cache verification work end-to-end (verifier internals, header policy wiring, dev/prod parity, and the fixture-backed integration tests). No blocking issues — the fix is correct and the earlier blocking issue is confirmed resolved.
Verified independently
-
Prior blocking issue is fixed.
applyCdnResponseHeaders(cache-control.ts:38) now gates theVINEXT_NEXT_DEPLOY_CACHE_CONTROL=1short-circuit on!input.pendingDynamicCheck, so an unverified late-dynamic edge render falls through to the adapter'sno-storepath instead of leakingpublic, max-age=0, must-revalidate. Regression test attests/cache-control.test.tspasses. -
BoundedStreamBuffercoalescing is correct. I ran a standalone reassembly harness (byte-count + content-order) across partial-fill, segment-boundary-aligned fills (length % segmentBytes == 0with a full trailing segment), truncated-final-segment (maxBytes % segmentBytes != 0), and exact-fill atmaxBytes, plus clamped-final-segment allocations. All cases reassemble byte-for-byte. ThefinalLength = length % segmentBytescomputation is sound: a short trailing segment only appears whenlength == maxBytes, wheremaxBytes % segmentBytesequals that segment's real size. -
Oversize handoff loses no bytes. The oversize path replays
[...buffer.chunks(), outcome.result.value]— the already-read chunk that didn't fit is carried into the resumed stream, then reading continues from the still-locked reader. No double-read, no dropped chunk. -
Fail-closed / lock release on every path (complete drain, deadline timeout, oversize, initial-read error, resumed-read error) — covered by
tests/app-page-cdn-verification.test.ts(7 tests, all pass). -
No unhandled rejection on the cancel-/never-pulled-after-handoff path. I wrote a targeted test where the deadline wins, the pending first read later rejects, and the resumed stream is never pulled — no
unhandledRejectionfires, because thePromise.race'spendingRead.then(...)already attaches a handler that marks the read handled. -
Event-loop starvation is bounded — the periodic
nextTask()yield + elapsed-time checks keep a synchronous infinite empty-chunk stream from starving timers; the adversarial test and the/neverproduction fixture both confirm a <500ms bounded return. -
Dev/prod parity + default-adapter safety. With the default
originadapter,usesResponseEdgeCache/shouldCompleteDynamicUsageBeforeResponseare bothfalse, so there is zero verification overhead or behavior change on the common path. The HTML deferred-cleanup ordering is correct:verifyCdnCacheCandidateStreamdrainingsafeHtmlStreamto completion triggersdeferUntilStreamConsumed'sflush()→clearRequestContext()and populatesdynamicUsedBeforeContextCleanupbefore the post-drain re-read atapp-page-render.ts:1129. -
RSC vs HTML guard asymmetry is correct. The RSC path (
:820) verifies onshouldCompleteDynamicUsageBeforeResponsealone and re-readsconsumeDynamicUsage()after the drain (:834) soresolveAppPageRscResponsePolicyreflects late dynamic usage; the HTML path (:1114) additionally gates on!dynamicUsedDuringRender. Both correctly demote tono-storewhen dynamic usage is detected. -
Cloudflare adapter fails closed.
buildResponseHeaderswithpendingDynamicCheck: truenow returnsCache-Control: no-storeand nulls the edge/tag headers (previously it emitted edge-cacheableCDN-Cache-Controland relied on browser revalidation). This is the core leak fix;tests/cloudflare-cdn-cache.test.tscovers it.
Non-blocking notes
-
formatCacheTagcountstag.length(UTF-16 code units) against byte limits. This is safe only because tags arriving here are already canonicalized viaencodeCacheTagto the ASCII range (\t\x20-\x7e), where code-unit length == byte length. It relies on an upstream invariant that isn't locally obvious — a one-line comment noting "tags are pre-encoded ASCII, so.length== byte length" would prevent a future caller from passing raw multi-byte tags and silently overshooting Cloudflare's 8 KB / 1 KB byte budgets. -
CodeQL "resource exhaustion" alert (
tests/app-cdn-cache-production.test.ts:58) — confirmed non-issue. The IO server clamps todelay === "750" ? 750 : 75(two hardcoded values), and the/slowfixture hardcodesdelay=750, so no user-controlled value reachessetTimeout. Test-only local server regardless. Safe to dismiss. -
hybridpage-cache mode is defined in theCdnPageCacheModeunion and documented but unused by any adapter. The render branching (!== "origin"to verify,=== "edge"to skip origin write) already yields the documented hybrid semantics (verify + retain origin write), so it's forward-compatible, just currently exercised only by types/docs.
Verification run
cache-control, app-page-cdn-verification, app-cdn-cache-production (7), app-page-render (55) all pass locally; vp check clean on the five changed source files.
LGTM.
|
Review posted to PR #2582. SummaryI independently re-reviewed the CDN edge-cache verification changes and found no blocking issues. The core security fix is correct. What I verified independently (not just re-reading prior reviews):
Non-blocking notes I added:
All targeted suites ( |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review
Reviewed at head 2d866a0. Heads up: the prior /bigbonk reviews on this PR are now stale. They describe a verifyCdnCacheCandidateStream with a BoundedStreamBuffer, a 100ms deadline, a 1 MB byte cap, periodic event-loop yields, and a "fail-closed handoff." The last commit (2d866a0 "classify edge responses without heuristics") deleted all of that. The verifier is now completeCdnCacheCandidateStream in app-page-cdn-verification.ts, which is an unconditional, unbounded, full-buffer drain:
export async function completeCdnCacheCandidateStream(
stream: ReadableStream<Uint8Array>,
): Promise<ReadableStream<Uint8Array>> {
const reader = stream.getReader();
const chunks: Uint8Array[] = [];
try {
for (;;) {
const result = await reader.read();
if (result.done) break;
chunks.push(result.value);
}
} finally {
reader.releaseLock();
}
return new ReadableStream({ start(c) { for (const chunk of chunks) c.enqueue(chunk); c.close(); } });
}The design change (verify by fully materializing the render, matching Next.js static generation) is reasonable and the pendingDynamicCheck fix from the earlier review is intact (cache-control.ts:38, adapter no-store at cdn-adapter.runtime.ts:149-158). But dropping the bounds re-introduces two failure modes the earlier commits on this same PR had explicitly guarded against. Please re-validate against the current code rather than the earlier reviews.
Blocking: unbounded, un-timed buffering hangs / OOMs on any non-force-dynamic edge render
shouldCompleteDynamicUsageBeforeResponse (app-page-render.ts:777-783) fires for any production edge/hybrid response that is not force-dynamic, not draft mode, and has revalidate === null || > 0 — i.e. ordinary static and ISR pages. For every such request, completeCdnCacheCandidateStream now buffers the entire HTML (:1124) or RSC (:821) stream into a Uint8Array[] with no time deadline and no size limit before a single byte is sent to the client.
Two concrete consequences:
- No timeout → hang. The
/neverproduction test only survives because the page isforce-dynamic(so the guard is skipped). A page that is not force-dynamic but has a Suspense boundary awaiting a slow/hanging non-dynamic async op (e.g. afetchto a stuck upstream, exactly like/slowbut never settling) will drain forever.handler()never resolves and the request hangs — no equivalent of the deletedCDN_DYNAMIC_VERIFICATION_DEADLINE_MS. There is no test covering a non-force-dynamic never-settling render;/neversidesteps the new code path entirely. - No size cap → memory pressure + TTFB regression.
/large(:249) now explicitly asserts a >1 MB body is fully buffered and edge-cached. On a Worker this means the whole body is held in memory per in-flight request, and TTFB is deferred until the last byte renders. The deletedCDN_DYNAMIC_VERIFICATION_MAX_BYTES = 1 MBcap existed for exactly this. Streaming HTML no longer streams for the common static/ISR case under an edge adapter.
If the intent is truly "materialize like Next.js static generation," that materialization must still be bounded (deadline + size ceiling that demotes to no-store/streams through on exceed), the way the d243ed2/59eddc2 commits did. As written, a slow or large — but perfectly legitimate and static — page is a latency cliff and a hang risk on the primary deploy target (Cloudflare Workers, whose adapter sets pageCacheMode = "edge").
Blocking: drain errors are no longer fail-closed
The old verifier caught initial-read / resumed-read errors and handed the stream back closed. completeCdnCacheCandidateStream only has a finally { reader.releaseLock() } — if reader.read() rejects (a render/stream error surfacing during the drain), the rejection propagates straight out of renderAppPageLifecycle at :821 / :1124. There is no try/catch at either call site. A mid-stream render error that previously produced a (safe, non-cacheable) response can now throw out of the lifecycle. Please confirm the dispatch caller turns this into a safe error response and add a test for "edge candidate whose stream errors mid-drain" — the earlier PR had this covered and the coverage was removed with the rewrite.
Non-blocking
- RSC vs HTML guard asymmetry (real, worth a comment or alignment). RSC gates on
!(peekDynamicUsage?.() ?? false)(:820); HTML gates on!dynamicUsedDuringRender && !scriptNonce && !shellErrorRecovered(:1113-1116). These are genuinely different predicates over "is this already known dynamic," not just a re-read timing difference as the prior review claimed. Please either unify them or add a comment explaining why the RSC path may buffer in a case the HTML path skips (and vice-versa). usesResponseEdgeCacheis!== "origin", sohybridbuffers too. The unbounded-drain concern above applies tohybridmode as well, not justedge. Sincehybridis currently unused, this is latent, but any future hybrid adapter inherits the hang/OOM risk.- Stale CodeQL note. The prior reviews dismiss the CodeQL alert by citing a
delay === "750" ? 750 : 75clamp; that clamp is what's inapp-cdn-cache-production.test.ts:58now, so the dismissal still holds — but it's worth re-confirming rather than inheriting from a review written against different code. formatCacheTagUTF-16 vs byte length (cdn-adapter.runtime.ts) — safe only under the upstreamencodeCacheTagASCII invariant; a one-line comment would help. (Carried over from prior review; still applies.)
Net: the pendingDynamicCheck leak fix is correct and well-tested, but the 2d866a0 rewrite traded a bounded verifier for an unbounded blocking drain on the hot path of every static/ISR edge response. I'd restore a deadline + size bound (demote-on-exceed) and re-add fail-closed error handling before merge.
|
Review posted to PR #2582. SummaryI reviewed PR #2582 at head Two blocking issues:
Non-blocking: genuine RSC-vs-HTML guard predicate asymmetry (not just timing), The |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review
Reviewed at head d801d02 ("admit streaming responses before edge caching"). Important: the prior /bigbonk review (at 2d866a0) is stale — its two blocking issues have been fixed by the newest commit, which abandons the buffering approach entirely. I re-reviewed against current code. No blocking issues.
The two prior blocking issues are resolved by a redesign, not a patch
The last review flagged that 2d866a0 traded a bounded verifier for an unbounded, un-timed full-buffer drain (completeCdnCacheCandidateStream) on the hot path of every static/ISR edge response — a hang risk (no deadline) and OOM/TTFB risk (no size cap), plus lost fail-closed error handling. Commit d801d02 deletes app-page-cdn-verification.ts and its test outright and replaces the whole strategy with deterministic two-stage admission:
- The Cloudflare adapter moves from
pageCacheMode = "edge"to"hybrid"(cdn-adapter.runtime.ts:130). - A fresh render streams immediately with
no-store— no drain, no buffering, no deadline needed.pendingDynamicCheckis now alwaystruefor the MISS response (thedynamicUsageCheckCompleteplumbing is no longer set by the render), sobuildResponseHeadersfail-closes tono-storeby construction (cdn-adapter.runtime.ts:156). - In parallel, the normal ISR finalizer tees the stream and writes to the data cache only if the completed render proved static (
app-page-cache-finalizer.ts:169). - A subsequent request reads that proven-static artifact from the data cache (
cache HIT, nopendingDynamicCheck) and emits edge-cacheable headers (app-page-cache.ts:172), promoting it to Cloudflare's edge.
I verified the previously-flagged failure modes are gone:
- Slow non-force-dynamic render no longer hangs. The
/slow-staticfixture (revalidate = 60, 750ms Suspense fetch) now streams the first response withno-storeand promotes on the 2nd request (app-cdn-cache-production.test.ts:151, assertsoriginRequests === 2). The/slowtest asserts the handler returns in<500ms(:225). This is precisely the "non-force-dynamic never-settling / slow" gap the prior review said had no coverage — it's now covered and passing. - Large body no longer OOMs / defers TTFB.
/large(>1 MB) streamsno-storeon first hit rather than buffering (:259). NoMAX_BYTEScap is needed because nothing is buffered before responding. - No fail-closed regression. There's no drain to throw from anymore; the MISS response is
no-storeunconditionally and the origin-write tee runs inwaitUntil(a background write that simply doesn't persist if the render errors/never settles — no leak, no thrown lifecycle).
The pendingDynamicCheck deploy-mode leak fix from the earlier review is intact (cache-control.ts:38, gated on !input.pendingDynamicCheck).
Independently verified
- Security invariant holds.
late-dynamicHTML and RSC (cookie-personalized behind Suspense) both stayno-storeand are never shared across users (:109,:177, assertoriginRequests === 2, nocdn-cache-control). The defaultoriginadapter path is unchanged (usesResponseEdgeCache/admission gates are all false), so zero behavior change and zero overhead on the common path. ownsBackgroundRevalidation = trueis now correct for hybrid — the data-cache admission copy must regenerate when stale, else edge revalidation would keep re-reading the same stale origin artifact. STALE reads serve edge-cacheable headers (proven-static artifact), which is right.revalidate=false(Infinity) admission —needsOriginAdmissionForEdge(app-page-render.ts:761) correctly forces RSC capture for force-static pages under hybrid so they can still be admitted; the paired HTML admission writes the RSC artifact so the RSC request promotes with only 1 origin render (:201, assertsoriginRequests === 1).- Tests all green locally:
app-cdn-cache-production(8),cloudflare-cdn-cache(15),cache-control(14),app-page-render(56),cdn-cache(16),cache-adapters-config(21).vp checkclean on all 6 changed source files.
Non-blocking notes
- Vestigial
dynamicUsageCheckCompleteplumbing. After the rewrite, the render no longer passesdynamicUsageCheckCompleteto the finalizer, so the field onFinalizeAppPageHtmlCacheResponseOptions/ScheduleAppPageRscCacheWriteOptionsand the branch inapplyPendingDynamicCdnHeaders(app-page-cache-finalizer.ts:88,93) are now alwaysundefined→pendingDynamicCheck: true. Harmless (correct fail-closed default), but it's dead surface area worth deleting in a follow-up so a future reader doesn't think it's still wired to a completion signal. - Data-cache dependency for admission. The hybrid
set()/get()now delegate togetDataCacheHandler(). With the default in-memoryMemoryCacheHandleron Workers (KV is opt-in per AGENTS.md), the admission artifact lives per-isolate, so edge promotion effectively requires two requests to hit the same warm isolate. That's an acceptable, documented tradeoff (not a correctness bug — it just means moreno-storemisses before promotion on cold isolates), but a one-line note in the adapter doc comment recommending a durable data cache (KV) for reliable edge promotion would help operators. formatCacheTagcountstag.length(UTF-16 code units) against byte budgets (cdn-adapter.runtime.ts:117-119). Safe only because tags arrive pre-canonicalized to ASCII viaencodeCacheTag. A one-line comment noting "tags are pre-encoded ASCII, so.length== byte length" would prevent a future caller from passing raw multi-byte tags and silently overshooting Cloudflare's 8 KB / 1 KB budgets. (Carried from prior reviews; still applies.)- CodeQL "resource exhaustion" alert (
app-cdn-cache-production.test.ts:58) remains a non-issue —setTimeout(..., delay === "750" ? 750 : 75)clamps to two hardcoded values in a test-only local IO server. Re-confirmed against current code. Safe to dismiss.
Net: the d801d02 redesign is a clear improvement over the buffering approaches on all prior revisions — it preserves streaming, has no hang/OOM/TTFB cliff, fails closed to no-store by construction, and never exposes request-specific output to a shared cache. LGTM.
|
Review posted to PR #2582. SummaryI reviewed PR #2582 at the current head Key finding: the prior
What I verified independently:
Non-blocking notes: vestigial The redesign is a clear improvement — LGTM. |
|
/bigbonk review for issues |
|
error: Your local changes to the following files would be overwritten by checkout: |
…te-dynamic # Conflicts: # packages/cloudflare/src/cache/kv-data-adapter.runtime.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review
Re-reviewed at head 35cc5a0. The prior LGTM was against d801d02; since then the branch merged main and added one new commit — e88a004 fix(cache): preserve edge admission metadata. I reviewed that commit end-to-end. No blocking issues.
What e88a004 changes
It fills three gaps in the two-stage hybrid admission introduced earlier in the PR:
- Tag propagation into edge headers. Cache entries now carry their canonical
tagsback throughget()/STALE reads (cache-handler.ts,kv-data-adapter.runtime.ts) and intobuildAppPageCachedResponse/buildRouteHandlerCachedResponse/buildPagesCacheResponse, so a promoted (HIT/STALE) artifact emitsCache-Tagand can be purged byrevalidateTag. Previously an admitted edge artifact had no tags, so tag purges couldn't reach it. - STALE never edge-caches.
CloudflareCdnCacheAdapter.buildResponseHeadersnow emitsCDN-Cache-Control: public, max-age=0, must-revalidatewhencacheState === "STALE"(cdn-adapter.runtime.ts:189-193). Correct: the STALE branch (app-page-cache.ts:465-489) fires a background regen and serves the stale copy withmust-revalidate, so the edge won't store stale bytes and the next request promotes the fresh HIT. Infinitysurvives KV JSON round-trip.revalidate: Infinity(force-static /revalidate = false) is serialized as the sentinel"infinity"and restored toInfinity(kv-data-adapter.runtime.ts). Before this,JSON.stringify(Infinity)producednull, the validator rejected the entry as malformed, and force-static pages could never be admitted to the edge via KV. This is what makes therevalidate-falsefixture's edge promotion actually work on the primary deploy target.
Verified independently
- Default
originadapter is untouched.DefaultCdnCacheAdapter.buildResponseHeadersignorescacheState/tags(cdn-cache.ts:164-172), so the new plumbing is a pure no-op on every non-Cloudflare path. Full dev/prod parity, zero overhead on the common path. - Malformed tags never reach the header builder. Persisted tags are run through
validUniqueTagsbefore propagation; the new test (kv-cache-handler.test.ts, "does not expose malformed persisted tags…") confirms[123, "", "valid", "valid", "bad\ntag", "bad:tag"]collapses to["valid"], so a poisoned KV entry can't emit a broken/oversizedCache-Tag.formatCacheTagstill bounds the total to Cloudflare's budget. Infinityround-trip.handler.set(..., { cacheControl: { revalidate: Infinity, expire: Infinity } })→handler.get()returns{ revalidate: Infinity, expire: Infinity }and does not compute a bogus finiterevalidateAt/expireAt(theNumber.isFiniteguards at:388/:393keep bothnull). Verified by the new "round-trips infinite cache-control metadata" test.- STALE header shape.
cloudflare-cdn-cache.test.tsasserts a STALE input yields exactlyCache-Control+CDN-Cache-Control=public, max-age=0, must-revalidateplus theCache-Tag, with no leftoverCloudflare-CDN-Cache-Control.app-page-cache.test.tscovers HIT (still SWR-cacheable) vs STALE (must-revalidate) with tags preserved on both. - Security invariant intact. The
pendingDynamicCheckfail-closed-to-no-storepath (cdn-adapter.runtime.ts:159-166,cache-control.ts:38) is unchanged; the newcacheState/tagsfields are only read on the HIT/STALE cache-read path, which by construction is already a proven-static artifact.
Verification run
vp check clean on all 7 changed source files. Locally green: kv-cache-handler (71), app-page-cache, cloudflare-cdn-cache, pages-page-data, app-route-handler-response (208 combined), plus app-cdn-cache-production (8), cache-control (14), cache-adapters-config (21), cdn-cache (16).
Non-blocking notes (carried forward, still apply)
formatCacheTagcountstag.length(UTF-16 code units) against byte budgets (cdn-adapter.runtime.ts:116-128). Safe only because tags arrive pre-canonicalized to ASCII viaencodeCacheTagand are now additionally filtered byvalidUniqueTags. A one-line comment noting "tags are pre-encoded ASCII →.length== byte length" would still help a future caller.STALE_EDGE_REVALIDATEandBROWSER_REVALIDATEare identical string constants (cdn-adapter.runtime.ts:71,74). Intentional (distinct rationale in the doc comments), but a future edit could drift them apart silently — worth a note that they're deliberately equal.- CI note: the last two
/bigbonkruns failed on a dirtypnpm-lock.yaml(vp installmutated the lockfile, blocking the branch checkout). Not a PR defect, but worth keeping the lockfile clean so the review job doesn't abort.
Net: e88a004 is a focused, well-tested completion of the hybrid-admission design — tag purges reach promoted artifacts, stale copies never get edge-cached, and force-static pages round-trip through KV. The core security fix from the earlier reviews is intact. LGTM.
|
Review posted to PR #2582. SummaryThe prior LGTM was against
Independently verified: default
Non-blocking: LGTM. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review
Reviewed at head 308b5d5. The prior LGTM was against e88a004; since then two new commits landed — cbb46b7 fix(cache): preserve late RSC tags during admission and 308b5d5 fix(cache): accept colon-delimited KV tags. I reviewed both end-to-end (the second largely reverts the first). No blocking issues.
What changed since the last review
cbb46b7 introduced createCollectedFetchTagsReader() — an ALS-scope-bound reader intended to let waitUntil() cache finalizers read the request's collected fetch tags after the AsyncLocalStorage scope exited. 308b5d5 then reverts that back to getCollectedFetchTags() and instead:
- Allows
:in cache tags (kv-data-adapter.runtime.ts:117-123) — the validator now rejects only control chars +\, not:. Matches Next.js, which accepts printable colon-delimited tags (e.g.post:hello) and uses them in its own App Router fixtures. - Adds a hashed-key disjointness guard (
kv-key.ts:44-54) — any logical key that literally starts with__hash:is now always hashed, so a user tag like__hash:<digest>can't alias the storage key an oversized logical key produces for that same digest.
Verified independently
-
The
getCollectedFetchTags()revert is safe. The deferredgetPageTags()reads inside the finalizer'scachePromiseIIFE (app-page-cache-finalizer.ts:191,308) run afterawait readStreamAsText(...)/await capturedRscDataPromise, but that IIFE is created synchronously insidefinalizeAppPageHtmlCacheResponse/scheduleAppPageRscCacheWrite, which run within the request'srunWithFetchDedupe/runWithRequestContextscope (app-page-render.ts:732,:1165). AsyncLocalStorage propagates throughawaitpoints in a promise chain created inside the scope, so_getState()still resolves to the correct request'scurrentRequestTags. ThecreateCollectedFetchTagsReaderindirection was therefore unnecessary; removing it doesn't reintroduce a leak. Confirmed by theslow-staticproduction test, which uses a lazily-consumed RSC fetch tag and asserts it survives to the edgeCache-Tagheader. -
Colon tags round-trip and invalidate. Tag keys are built as
{prefix}__tag:{tag}and never parsed back by:; the only tag→key parse (revalidateByPathPrefix,:502) strips_N_T_and gates on a leading/, sopost:hellois correctly excluded from path-prefix invalidation and can't collide with an entry key (distinctcache:vs__tag:category prefix).kv-cache-handler.test.tscovers colon round-trip +revalidateTag("post:hello")invalidation, and that malformed tags (bad\ntag,bad\tag) are still filtered. -
The
__hash:disjointness guard closes a real (if adversarial) collision. An oversized key hashing to__hash:H, plus a literal user key__hash:Hshort enough to store verbatim, would previously map to the same KV key. The guard re-hashes the literal, andkv-cache-handler.test.tsasserts a literal__hash:<digest>tag key differs from the oversized key's hashed form. -
Deploy-time parity.
prerender-kv-populate.tsbuilds keys through the samecreateKvKeySpace().entryKey, so deploy-time seeded keys stay consistent with the runtime guard.tpr.tsin the diff range came from amainmerge (#2606), not this PR. -
Clean removal — no remaining references to
createCollectedFetchTagsReader; the corresponding fetch-cache test was removed with it.
Verification run
vp check clean on the 4 changed source files. Locally green: kv-cache-handler (73), app-cdn-cache-production (8, incl. the colon-tag slow-static assertion), fetch-cache (126), cloudflare-cdn-cache (16), cache-control (14), cdn-cache (16), prerender-kv-populate (7), tpr-kv-keys (12), app-page-render (56), app-page-cache (37).
Non-blocking notes (carried forward, still apply)
formatCacheTagcounts UTF-16 code units against Cloudflare's byte budgets. Still safe only because tags arrive pre-canonicalized to ASCII viaencodeCacheTagand filtered byvalidUniqueTags. Now that:is permitted content, a one-line comment noting "tags are pre-encoded ASCII →.length== byte length" is a touch more valuable, since a future caller passing raw multi-byte tags is the failure mode.- The
cbb46b7doc comment oncreateCollectedFetchTagsReadercorrectly identified a real hazard pattern (finalizers reading tags post-drain); the revert relies on ALS propagation through the awaited promise chain holding. That invariant is load-bearing but not locally documented at the finalizer call sites — a short comment there would prevent a future refactor from detaching thecachePromiseinto a fresh async context and silently dropping tags.
Net: the two new commits are a focused correctness improvement — colon-delimited tags now match Next.js and survive KV round-trip, an adversarial hashed-key collision is closed, and the ALS-reader indirection was correctly removed as unnecessary. The core pendingDynamicCheck fail-closed security fix from the earlier reviews is untouched. LGTM.
Co-authored-by: james-elicx <james-elicx@users.noreply.github.com>
|
Review posted to PR #2582. SummaryI reviewed PR #2582 at head What the new commits do:
What I verified independently:
The core |
|
/bigbonk review for issues |
|
error: Your local changes to the following files would be overwritten by checkout: |
2 participants
Summary\n\n- verify streamed App responses before emitting edge-cacheable headers\n- preserve streaming with a bounded, fail-closed verification path\n- distinguish origin, edge, and hybrid page-cache ownership\n- cover deferred dynamic responses and static edge reuse in production fixtures\n\n## Validation\n\n- production cache fixture: 7 passed\n- focused cache/render suites: 348 passed\n- adversarial stream bound and cancellation checks\n- vinext and Cloudflare adapter builds\n- targeted format, lint, and type checks