LTRAC-1499: Add Cloudflare KV adapter for the routing cache - #3170
Draft
jorgemoya wants to merge 1 commit into
Draft
Conversation
with-routes caches redirects and storefront status through lib/kv, which had
no Cloudflare option: on Native Hosting it fell through to MemoryKvAdapter,
an in-process LRU that isn't shared across edge invocations. Add a
CloudflareKvAdapter over the per-project CATALYST_ROUTES_KV namespace and
select it ahead of Upstash, keeping Vercel Runtime Cache first in the chain.
Workers KV has no multi-get, so mget fans out to parallel get(key, 'json')
calls, each guarded so one unreachable key can't blank the batch. set writes
JSON.stringify(value) and swallows failures -- a cache write must not break
the request.
The binding is duck-typed on get/put being callable rather than merely
present. A merchant can define an env var literally named CATALYST_ROUTES_KV,
which arrives as a plain string; a truthiness check would accept it and then
throw on first use. Narrowing lets it fall through to Upstash/Memory instead.
Note this does NOT import @opennextjs/cloudflare, and deliberately so. The
package is absent from core's package.json (the CLI injects it only for
Commerce hosting), and importing it from here fails either way: a dynamic
import breaks `next build`'s TypeScript step on every non-Commerce project
("Cannot find module ... or its corresponding type declarations"), and
silencing that with an ambient `declare module` shadows the package's real
types when it IS installed -- which breaks the .bigcommerce/open-next.config.ts
the CLI writes, since it imports defineCloudflareConfig from the same
specifier and core's tsconfig compiles it. Both were verified empirically.
getCloudflareContext() in sync mode is just a read of
globalThis[Symbol.for('__cloudflare-context__')], set by the OpenNext worker
entrypoint in production and by initOpenNextCloudflareForDev under next dev.
Reading that symbol directly yields the identical value with none of the
above failure modes -- off Cloudflare it is simply undefined.
Verified end to end, not just by inspection: an OpenNext worker built with
the same pipeline and run under `wrangler dev` with the KV binding wired
reports, from inside experimental-edge middleware with no import and no
await, that the symbol is defined, that env.CATALYST_ROUTES_KV exposes
get/put, and that a put/get round trip returns the written value. The value
was then read back by a separate `wrangler kv key get` process, confirming a
genuinely shared store rather than per-invocation memory.
Because that symbol is an internal contract rather than exported API, its
failure mode would otherwise be silent -- a version bump would downgrade
every native-hosted store to an in-process cache with no signal. The key is
now a named exported constant pointing at OPENNEXT_CLOUDFLARE_VERSION, and a
contract test in packages/catalyst (the package that actually has
@opennextjs/cloudflare installed) asserts the real getCloudflareContext still
reads it, and reads it synchronously, so a breaking bump fails CI.
core had no unit test infrastructure (tests/ is Playwright), so this also
adds vitest scoped to lib/**/*.spec.ts, keeping it clear of the Playwright
specs that share the .spec.ts suffix. createKVAdapter is exported for tests
because adapter selection depends on ambient state the memoized kv singleton
hides.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 765d6ac The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linear: LTRAC-1499
Parent: LTRAC-1019
What/Why?
The runtime half of the routing cache work:
createKVAdaptergains a Cloudflare branch soproxies/with-routesuses the per-project KV namespace on Native Hosting instead of degrading toMemoryKvAdapter. Vercel stays first in the chain; Upstash and Memory remain the fallbacks.Three decisions here are non-obvious and are the ones worth reviewing.
1. It reads the context global directly instead of importing
@opennextjs/cloudflarecoredeliberately does not depend on that package —setupCommerceHosting()injects it only when a project opts into Commerce hosting. Every way of importing it fromcoreis worse, and this was measured rather than assumed:importbreaksnext buildoutright when the package is absent;import()still failsnext build's TypeScript step ("Cannot find module … or its corresponding type declarations") on every non-Commerce project;declare moduleshadows the package's real types when it is installed, which breaks the.bigcommerce/open-next.config.tsthe CLI writes (it importsdefineCloudflareConfigfrom the same specifier, and core's tsconfig compiles that directory).So the adapter reads
globalThis[Symbol.for('__cloudflare-context__')], which is exactly whatgetCloudflareContext()does in sync mode. On any non-Cloudflare runtime the symbol is simplyundefined— the "import failed" failure mode stops existing rather than being caught.That global is an
AsyncLocalStorage-backed getter, and the OpenNext worker entrypoint wraps every request handler incloudflareContextALS.run({ env, ctx, cf }, handler)— which is why a synchronous read works inside middleware.The tradeoff: that symbol is an internal detail, not exported API, and if it changed the adapter would silently return null and every store would quietly downgrade to an in-process cache with no error. That's why
packages/catalyst/src/cli/lib/cloudflare-context-symbol.spec.tsasserts the realgetCloudflareContext()still honours this exact key — a version bump that breaks the assumption fails CI instead of degrading production.OPENNEXT_CLOUDFLARE_VERSIONis exported so that test can pin what it was verified against.2. Selection duck-types the binding rather than checking presence
A merchant can define an env var literally named
CATALYST_ROUTES_KV, which arrives as a plain string. A truthiness check would accept it and then throw on the first.get(). Narrowing onget/putbeing functions makes that case fall through to Upstash/Memory cleanly. (Ignition also strips the colliding var server-side — this is defence in depth.)3. Writes carry an
expirationTtlWorkers KV entries are permanent unless written with an expiration, and nothing else deletes routing cache keys. Since
kvKey()includes the query string, a crawler walking?utm_*permutations would otherwise grow a namespace — and its billable write volume — without bound, from unauthenticated requests.The TTL is 7 days, deliberately much longer than the 30-minute freshness window
with-routesenforces via theexpiryTimeinside each value. The two timers do different jobs:expiryTimedecides when to revalidate, the TTL decides when to garbage-collect. Setting them equal would delete each entry exactly as it went stale, turning every stale-while-revalidate hit — which serves instantly and refreshes in the background — into a blocking GraphQL round trip. A test pins the TTL as strictly greater than that window so this can't be "tidied up" into a regression later.Note the same unbounded-growth gap exists today for Upstash users, which passes
optsthrough but is never given any. Fixing that means touching shared middleware and changing behaviour for existing deployments, so it's intentionally not bundled here.Reviewer note: new test infrastructure in
corecorehad no unit-test setup at all — notestscript, no vitest/jest, andcore/tests/**is entirely Playwright. Covering the selection matrix required introducing vitest:core/vitest.config.ts, atestscript, and thevitestdevDependency (plus the lockfile line).Flagging it explicitly because
coreis the scaffold customers receive, so that config ships into every generated project. Those files are grouped so they're easy to strip if that's unwanted. The vitestincludeis scoped tolib/**/*.spec.tsso it can't collect Playwright's specs, which share the.spec.tssuffix, and no coverage thresholds were added.Testing
Selection is covered for: binding present and well-shaped, binding absent, the
@opennextjs/cloudflareimport itself failing, and the binding present but a string — the last three all asserting it falls through without throwing, since a caching layer must degrade rather than break a request. Also covered:get/putround-trip, per-key error isolation inmget, a failingputbeing swallowed, and the TTL assertions above.eslintclean onlib/kv;tsc --noEmitreports no errors inlib/kv(the repo has pre-existing errors elsewhere from missing GraphQL codegen, identical with and without this change).Migration
None. Additive — no files moved, no breaking changes. A
patchchangeset is included. Behaviour is unchanged anywhere the binding isn't present, which is every non-Native-Hosting deployment.