fix(cache): scope use cache keys by deployment id#1092
fix(cache): scope use cache keys by deployment id#1092NathanDrake2406 wants to merge 8 commits intocloudflare:mainfrom
Conversation
Shared "use cache" entries only used the vinext build ID as their cache-key seed. That diverges from the upstream Next.js change for cacheHandlers and can let a Cloudflare Worker deployment reuse entries from another deployment when the deployment identity changes independently of the build fallback. The cache runtime now resolves a deployment-aware key seed at execution time, with request-scoped Worker version metadata or NEXT_DEPLOYMENT_ID taking precedence over the build ID. Generated Wrangler config includes the CF_VERSION_METADATA binding, and the App Router Worker entry threads it into the runtime without process-global request races. Tests cover deployment ID precedence, concurrent deployment scope isolation, build ID fallback, and generated Worker config wiring.
commit: |
Deployment-aware "use cache" keys had runtime Worker and NEXT_DEPLOYMENT_ID support, but vinext did not resolve the Next.js deploymentId config field. That left config-defined deployment identity out of the build-time cache seed. Resolve deploymentId from next.config.js before NEXT_DEPLOYMENT_ID, validate the same string shape Next.js documents, and feed the resolved value into the __VINEXT_DEPLOYMENT_ID define. The process-wide cache-runtime setter is also documented as a fallback rather than the per-request API. Tests cover config precedence, env fallback, empty config values, and invalid deploymentId shapes.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid work. The deployment-aware cache key seeding matches the upstream Next.js model, the ALS-based concurrency isolation is correct, and the test coverage is good. The refactor of app-router-entry.ts to extract handleRequest preserves behavior while cleanly wrapping the deployment ID scope.
A few observations below — nothing blocking, mostly edge-case clarifications and a minor nit.
| function resolveDeploymentId(configDeploymentId: unknown): string | undefined { | ||
| const deploymentId = | ||
| configDeploymentId !== undefined ? configDeploymentId : process.env.NEXT_DEPLOYMENT_ID; | ||
| if (deploymentId === undefined || deploymentId === "") return undefined; |
There was a problem hiding this comment.
Calling out: when a user sets deploymentId: "" in next.config.js, this silently swallows NEXT_DEPLOYMENT_ID from the environment. The test covers this behavior ("treats an empty next.config.js deploymentId as unset"), so it's intentional — but it functions as a way to opt out of the env var, not just "leave unset." Worth a brief inline comment for the next person who reads this, e.g.:
| if (deploymentId === undefined || deploymentId === "") return undefined; | |
| // Setting deploymentId to "" in next.config.js explicitly opts out of | |
| // any deployment ID (including NEXT_DEPLOYMENT_ID from the environment). | |
| if (deploymentId === undefined || deploymentId === "") return undefined; |
| function getUseCacheKeySeed(): string | undefined { | ||
| const runtimeDeploymentId = Reflect.get(globalThis, _DEPLOYMENT_ID_KEY); | ||
| return ( | ||
| _deploymentIdStorage.getStore() || |
There was a problem hiding this comment.
Minor: _deploymentIdStorage.getStore() returns undefined both when no ALS scope is active AND when the store was explicitly set to undefined (via runWithUseCacheDeploymentId(undefined, fn)). In both cases you want to fall through, so the || works correctly here. Just noting this is intentional — undefined from an active scope deliberately falls through to the define/build-ID layers rather than short-circuiting the chain.
| return notFoundResponse(); | ||
| if (result instanceof Response) { | ||
| if (env?.ASSETS) { | ||
| const assetFetcher = env.ASSETS; |
There was a problem hiding this comment.
Nice cleanup: extracting env.ASSETS into assetFetcher avoids the non-null assertion that was previously needed inside the closure (env.ASSETS!). This is a strict improvement over the original.
| // Deployment ID — mirrors Next.js' NEXT_DEPLOYMENT_ID seed for shared | ||
| // "use cache" entries, falling back to build ID when absent. | ||
| defines["process.env.__VINEXT_DEPLOYMENT_ID"] = JSON.stringify( | ||
| nextConfig.deploymentId ?? "", |
There was a problem hiding this comment.
When deploymentId is undefined, this serializes to "". Downstream in getUseCacheKeySeed(), "" is falsy so it correctly falls through to getUseCacheBuildIdDefine(). This is the right approach — it avoids needing Vite to define-replace with undefined (which wouldn't inline cleanly). Just wanted to confirm: have you verified this works when __VINEXT_DEPLOYMENT_ID is never defined at all (non-Vite test runner, no define transform)? In that case process.env.__VINEXT_DEPLOYMENT_ID would be literally undefined, which also falls through correctly. Looks good.
| ${isrImports} | ||
| interface Env { | ||
| ASSETS: Fetcher;${isrEnvField} | ||
| NEXT_DEPLOYMENT_ID?: string; |
There was a problem hiding this comment.
The generated worker entry declares NEXT_DEPLOYMENT_ID and CF_VERSION_METADATA in the Env interface, which is good. But the generated fetch handler at line 510 just does handler.fetch(request, env, ctx) — meaning app-router-entry.ts receives the full env and extracts the deployment ID itself. This works cleanly because deploymentIdFromEnv in app-router-entry.ts handles the env?.NEXT_DEPLOYMENT_ID || env?.CF_VERSION_METADATA?.id chain.
The only gap (noted in the PR description) is custom worker entries that don't delegate to app-router-entry — they'd need to call runWithUseCacheDeploymentId themselves. Could be worth adding a one-liner comment in the generated entry template pointing users to runWithUseCacheDeploymentId if they customize the handler.
| } | ||
|
|
||
| function buildUseCacheKey(id: string, keySeed: string | undefined, argsKey?: string): string { | ||
| const scopedId = keySeed ? `build:${encodeURIComponent(keySeed)}:${id}` : id; |
There was a problem hiding this comment.
Nit: The key prefix is still build: even when the seed is a deployment ID rather than a build ID. This doesn't cause correctness issues (it's just a string prefix for namespacing), but it could be confusing when debugging cache keys. Consider whether a more generic prefix like scope: would be clearer, or add a comment explaining the naming is historical.
| const scopedId = keySeed ? `build:${encodeURIComponent(keySeed)}:${id}` : id; | |
| const scopedId = keySeed ? `build:${encodeURIComponent(keySeed)}:${id}` : id; // "build:" prefix is historical — seed may be deployment ID or build ID |
|
Review submitted with APPROVE. Here's a summary of what I found: Overall: Clean, well-structured PR. The deployment-aware cache key seeding correctly matches the upstream Next.js model ( Key observations posted as inline comments:
|
| Reflect.set(globalThis, _DEPLOYMENT_ID_KEY, deploymentId || undefined); | ||
| } | ||
|
|
||
| export function runWithUseCacheDeploymentId<T>( |
There was a problem hiding this comment.
instead of using ALS, we should be able to inline this value with the same pattern we are using for build ids
There was a problem hiding this comment.
i see we are inlining it - in that case, can we rip out the ALS?
The Worker version metadata path still needs request-scoped state, but cache-runtime should not own a cache-specific deployment ALS or public-looking setter. Move runtime deployment ID storage into the unified request-context layer. The Worker boundary now writes deploymentId into request runtime context, cache-runtime reads that generic request metadata before falling back to inlined deployment/build IDs, and the tests cover direct runtime scopes plus inheritance into unified request contexts.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| }; | ||
|
|
||
| function deploymentIdFromEnv(env: WorkerAssetEnv | undefined): string | undefined { | ||
| return env?.NEXT_DEPLOYMENT_ID || env?.CF_VERSION_METADATA?.id || undefined; |
There was a problem hiding this comment.
this is a binding and not guaranteed to follow that var name
| type AppRouterRuntimeContext = { | ||
| deploymentId?: string; | ||
| passThroughOnException?(): void; |
There was a problem hiding this comment.
why does deployment id need to live on the request context?
There was a problem hiding this comment.
for simplicities sake, all we need for deployment ids at the moment if a function that return the value of process.env.__VINEXT_DEPLOYMENT_ID which is inlined at build time, and falls back to process.env. NEXT_DEPLOYMENT_ID, and then probably falls back to build id potentially.
deploymentid doesnt need to touch ALS, unless there's a reason I'm not seeing?
in the future when we support experimental.runtimeServerDeploymentId as well, i believe we would always default to the NEXT_DEPLOYMENT_ID var and error if there's a deploymentId in the next config.
| // Exposes the Cloudflare Worker version ID at runtime. vinext uses it as | ||
| // the deployment seed for shared "use cache" entries when available. | ||
| version_metadata: { | ||
| binding: "CF_VERSION_METADATA", |
There was a problem hiding this comment.
please can we exclude these changes for now, there's probably an open question around what's the right way to consume this at runtime, so would prefer to leave it out for now.
|
Updated to match this direction:
Focused checks passed locally: |
What this changes
Shared
"use cache"entries now include a deployment-aware seed before falling back to the existing vinext build ID. This matches the upstream cache key precedence model for vinext's supported deployment ID sources while keeping Worker deployment identity request-scoped.Why
Next.js changed
use-cache-wrapperso cache keys useworkStore.deploymentId || workStore.buildId. Without this, two deployments can share the same"use cache"entry when the build ID is stable or when a runtime deployment ID changes independently of the build fallback.References:
deploymentIdconfig docs: next.config.js deploymentIdApproach
cache-runtime, with precedence: request-scoped deployment ID, explicit startup/test fallback, build-time resolved deployment ID, then vinext build ID.next.config.js deploymentIdbeforeNEXT_DEPLOYMENT_ID, validate its string shape, and expose the resolved value throughprocess.env.__VINEXT_DEPLOYMENT_ID.env.NEXT_DEPLOYMENT_IDorenv.CF_VERSION_METADATA.idin the App Router Worker entry.version_metadatabinding inwrangler.jsoncso deployed Cloudflare Workers haveCF_VERSION_METADATA.idavailable by default.Validation
vp test run tests/shims.test.ts -t 'use cache'vp test run tests/deploy.test.ts -t 'Wrangler Config Generation|generateAppRouterWorkerEntry'vp test run tests/next-config.test.ts -t 'deploymentId|generateBuildId'vp check packages/vinext/src/config/next-config.ts packages/vinext/src/shims/cache-runtime.ts packages/vinext/src/server/app-router-entry.ts packages/vinext/src/deploy.ts packages/vinext/src/index.ts packages/vinext/src/global.d.ts tests/shims.test.ts tests/deploy.test.ts tests/next-config.test.tsvp check --fixandknip --no-progressRisks / follow-ups
This PR scopes the App Router Worker entry and generated deploy config. Custom Worker entries that bypass
vinext/server/app-router-entrystill need to provide their own deployment context if they invoke lower-level internals directly.★ Insight
"use cache"with deployment identity before build identity because cacheHandlers can outlive a single build assumption.envstate, so reading it at module registration would miss or race the real deployment value.