feat(cloudflare): key Response Store App pages by the query-free cache identity - #3463
james-elicx wants to merge 3 commits into
Conversation
|
/bigbonk review for issues Please review exact head |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
LGTM! |
aca5982 to
e153f06
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
|
e153f06 to
3a24111
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
3a24111 to
f7b0654
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
f7b0654 to
b69ad69
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
b69ad69 to
2946380
Compare
|
/bigbonk review for issues Please review exact head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| return; | ||
| } | ||
| await responseStore.put(key, admitted, { | ||
| await responseStore.put(key, withoutRequestScopedHeaders(admitted), { |
There was a problem hiding this comment.
Restrict this stripping to App-page RSC entries (or otherwise prove the request stage will restore these headers). This branch also handles cacheable App route handlers, metadata routes, and hybrid Pages responses, but app-rsc-handler.ts only recomposes these headers for kind === "app-page" && isRscRequest. A route handler that returns either header therefore exposes it on the MISS and silently loses it on the HIT/regenerated response. The unconditional calls at lines 231 and 509 have the same issue.
There was a problem hiding this comment.
Fixed in 6f58508: the params/rendered-search headers are only stripped for App page RSC entries, in all three places, which is the only case the request stage recomposes. Route handlers, metadata routes and Pages responses keep their headers. Tests cover a route handler's headers across MISS→HIT and regeneration.
|
Posted 1 actionable inline finding. Targeted unit tests pass. |
2946380 to
6f58508
Compare
|
/bigbonk review for issues Please review exact head |
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
c8e4e6b to
c5ce774
Compare
c5ce774 to
25738a0
Compare
25738a0 to
00b41f3
Compare
00b41f3 to
75a60db
Compare
869eff2 to
3a3e801
Compare
3a3e801 to
c3d6371
Compare
c3d6371 to
2b7f58e
Compare
…e identity The Response Store adapter declares responseStageCacheIdentity and uses options.cacheIdentity for its lookup key, stored entry, route revalidator replay and warmup RSC seed, so App pages that admission stores share one entry across queries. The render still receives the real request, and data-cache replays keep the full invocation that produced them. X-Vinext-Params and X-Vinext-Rendered-Path-And-Search are dropped before storing and after regeneration; the request stage recomposes them.
…entries The request stage recomposes X-Vinext-Params and X-Vinext-Rendered-Path-And-Search only for App page RSC responses, so the Response Store adapter now drops them only when the response-stage props say kind app-page with isRscRequest. Route handlers, metadata routes, App page HTML and hybrid Pages entries keep the headers they rendered on the stored, admitted and regenerated paths, so a HIT matches the MISS.
…eries in the Response Store Request the static /search-params/suspense page with two fresh queries and require the second to HIT with the same render ID, so the test proves one stored document serves every query rather than only the exact URL.
2b7f58e to
caa35be
Compare
1 participant
Stacked on #3462. Plan PR 5 (Response Store adapter).
Change
Opt-in:
ResponseStoreCdnCacheAdapterdeclaresresponseStageCacheIdentity = "query-free".Where the identity is used:
response-store-adapter.worker.tsusescacheIdentity, falling back to the full request and props, for:put;The render itself still gets the real request. Every query of a static App page now shares one stored entry. A regeneration replays the query-free identity, so it never bakes one visitor's query into the shared entry.
Per-request headers:
X-Vinext-ParamsandX-Vinext-Rendered-Path-And-Searchare removed before everyputand from regenerated responses. The request stage rebuilds them for each App RSC response, HITs included.Unchanged:
vinext:datarevalidator entries keep the full invocation. Replaying a query-dependentfetchwithout its query would compute a different fetch key, so the entry could never be refreshed.Tests
tests/cloudflare-response-store-worker.test.ts:putand after regeneration;packages/cloudflare/tests/response-store-adapter.e2e.test.ts(examples/response-store-demo):/cached/query-identityrequested with two different queries gives a MISS, then a HIT with the same render, for both HTML and RSC;x-vinext-paramsandx-vinext-rendered-path-and-search;Two existing cases used queries to force separate entries and now use distinct paths. 29/29 pass.