fix(storyblok-ui): cut redundant Storyblok CDN requests - #2662
Conversation
Three sources of avoidable requests in the storefront's Storyblok layer:
- `refreshStoryblokCacheVersion()` issued `cdn/spaces/me` and expected
`storyblok-js-client` to pick the new cache-version up, but the client only
advances its pin from a response carrying a top-level `cv` — `cdn/spaces/me`
answers with `{ space: { version } }`. The pin therefore never moved and the
request was pure overhead. Read the version and apply it explicitly, and flush
the client's response cache when it changed so opt-in `apiOptions.cache` users
cannot serve content that outlived its version.
- `fetchStory()` had no memory of 404s. The client caches successful responses
only, so a catch-all route that probes the CMS for every category URL before
falling back re-asked the API on every render for slugs that never have a
story. Remember a 404 for one cache-version interval; draft/preview reads and
development bypass it, and only a genuine 404 is remembered so network and auth
failures stay retryable.
- `getStoryblokStaticPaths()` downloaded every story body to read `full_slug`.
The content sitemap that calls it runs in `getServerSideProps`, so that was the
whole space per crawler hit.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 2217290 The changes in this PR will be included in the next version bump. This PR includes changesets to release 86 packages
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.
|
paales
left a comment
There was a problem hiding this comment.
Does this prevent random URL's generating API calls. A completely random URL should NOT create an API call because bots will generate tons of random URL's?
Review feedback: remembering individual 404s does not help against the case it
most needs to — crawlers and vulnerability scanners walk an unbounded number of
made-up URLs, and a catch-all route asks the CMS about every one of them. Every
made-up URL is a fresh slug, so a per-slug negative cache never gets a second
chance to help and each one still costs a CDN request.
Replace it with a slug index: one `cdn/links` request per cache-version lists
every published slug in the space, and anything outside that set is answered as
"not found" without a request. Measured against a live space: 300 random URLs
now cost 0 requests instead of 300, while known slugs are unaffected.
- Deliberately unscoped by language. `cdn/links?language=x` returns only the
stories translated into that language (25 of 63 on the space I tested, for a
language the space does not even define), while `cdn/stories/<slug>?language=x`
falls back to the default language and resolves all of them — so a
language-scoped index would report existing pages as missing.
- Folders are skipped. A folder with a start page contributes that start page as
its own entry (`clubkleding/`), which is what makes `fetchStory('clubkleding')`
resolve while a folder without one correctly reports as missing.
- Memoized per cache-version in module scope rather than through the client's
response cache, because that cache is opt-in and off by default.
- Never claims absence when it does not know: a failed index request, or a space
larger than 10 000 stories, falls back to the previous per-slug behaviour.
Draft/preview reads and development skip the index entirely.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d6c01b2 to
ac2dc8e
Compare
|
Written by Claude Code: Good catch, and the honest answer for the version you reviewed is no. The negative cache was per-slug, so it only helped when the same made-up URL came back. A bot walking random URLs produces a fresh slug every time, which meant a fresh CDN request every time — exactly the case you were asking about, and the one it did least for. Fixed in ac2dc8e by replacing it with a slug index instead of trying to remember misses. MechanismOne The per-slug negative cache is gone. With the index it only ever covered cases the index already answers for free, so it was one staleness mechanism too many. Measured against a live space (63 links, 55 stories)Counting outgoing HTTP requests, client response cache disabled so nothing is hidden:
Before this change those 300 random URLs cost 300 Things I checked because they could silently break pages
🤖 Generated with Claude Code |
`requestStoryblokCacheVersionRefresh()` — what a cache-notify webhook calls when a story is published — only flipped a flag in the process that happened to receive the webhook. Nothing told the other replicas anything had changed, so the only thing keeping them fresh was `cacheVersionTtl`: one `cdn/spaces/me` per server per interval, forever, whether or not anything is ever published. At the previous 60 second default that is ~450 000 requests a month across a ten-pod deployment, spent entirely on discovering that nothing happened. The refresh now fans out over a file instead. The webhook writes a millisecond timestamp to `renew-all-pages-query.txt` and every server compares it against the last value it acted on before a published read — a few bytes off the filesystem, throttled to once a second, never an API request. A server that sees a newer signal fetches the current cache-version once and pins it, so the cost of a publish is one request per server that actually serves traffic, instead of one request per server per minute forever. - The file name is not new: it is the one the SSR Apollo client convention already reads (`examples/*/lib/graphql/graphqlSsrClient.ts` drops its per-locale client when the timestamp moves past its own), so one publish can invalidate both caches and the contract stays "this file contains a number". - `storyblok.cacheVersionSignalDir` says where that file lives. It defaults to the `./tmp` of the existing convention, which resolves *inside each container* — a multi-replica deployment has to point it at a shared volume or the signal never leaves the pod that wrote it. `renewSignalPath()` is exported so a project's `graphqlSsrClient` can read the same file without hardcoding the path twice. - `cacheVersionTtl` now defaults to 3600 rather than 60. It is no longer what keeps content fresh, only what bounds staleness where the signal cannot be delivered: serverless, local development, an unwritable directory. It stays applied rather than disabled on purpose — a misconfigured webhook has to degrade to stale content, never to frozen content. - The real cache-version is still fetched rather than derived from the signal's timestamp. Storyblok answers a request carrying an unknown `cv` with a 301 to the canonical one, so a made-up value would work, but it costs every replica a redirect round-trip on its next read and keys the slug index on a version that is about to be replaced. - `node:fs` is loaded through a `webpackIgnore`d dynamic import. This module is re-exported from the package's single barrel entry point and Next.js does not tree-shake in development, so a static import would break every client component importing from the package. Draft, preview and development reads are untouched — they already send a fresh `cv` per request. Verified with ten real OS processes against one shared directory: before, a publish reached 1 of 10 processes and the other nine kept serving the pre-publish version; after, all 10 picked it up within a second, at one `cdn/spaces/me` each. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ae77080 to
54e167f
Compare
The renew signal was a file in a configurable directory, which only fans out when every replica happens to mount the same volume — the constraint that makes the `./tmp` convention in `examples/*/lib/graphql/graphqlSsrClient.ts` unreliable in the first place. `storyblok.cacheVersionSignalDir` made that a supported option rather than fixing it, so both the config key and the filesystem access are gone. The signal now goes through `globalThis.__incrementalCache`, so it reaches every server that shares a `cacheHandler` — the same requirement a deployment already has to meet to serve consistent ISR, and nothing extra to configure. Validated on a two-pod cluster: pod A writes, pod B reads the value within 9 ms. This leans on two Next.js internals. The global is set in `handleRequestImpl` (`next/dist/server/base-server.js`) on every incoming request, ahead of API routes and ISR regeneration, under the same name and shape in Next 13 through 16; middleware and the edge runtime build their own instance and are not covered. Writing under `kind: 'FETCH'` with `fetchCache: true` is the one path that stores a key verbatim rather than through `normalizePagePath()`, which is what allows a namespaced key. Both are unstable: if either changes, reads and writes fail closed and freshness falls back to `cacheVersionTtl`. The call shape has drifted before — `revalidate` became `cacheControl` between Next 14 and 15 — so it needs re-testing per major. Why the push is worth having at all: `cdn/spaces/me` polls are billed per request against the Storyblok quota, so the polling interval is a cost, not just a staleness knob. `cacheVersionTtl` stays at 3600 as the failsafe. Also drops the justification prose from the comments and rewrites both changesets to describe what changes rather than argue for it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…blok Two mechanisms were solving the same problem in different ways: Storyblok pushed a renew signal through the incremental cache, while `graphqlSsrClient()` read `./tmp/renew-all-pages-query.txt`. That path resolves inside each container, so on a multi-replica deployment it silently invalidated nothing — the assumption that made the file approach wrong for Storyblok in the first place. The signal moves to `@graphcommerce/graphql` as `publishRenewSignal()` / `renewSignal()` / `refreshRenewSignal()`. That package is the right home by dependency direction: `storyblok-ui` peer-depends on it, it does not depend on `storyblok-ui`, and all three examples already import from it. The key is neutral (`gc:signal:content-renew`) because the fact being published is "content changed", not anything Storyblok-specific. `graphqlSsrClient()` is synchronous and called as `const client = graphqlSsrClient(context)` throughout `getStaticProps`, so it cannot await the read; making it async would break every consumer in every project. It reads the last known value and schedules the next refresh, which bounds staleness at one poll interval plus one call. That makes the cold start explicit rather than accidental. `renewSignal()` returns `undefined` until the first read completes, which is distinct from "read, and nothing was published" — every pod is in that state right after a deploy, and treating it as 0 would silently skip the first invalidation. Callers create the client without invalidating (a client made now cannot predate a publish) and let the scheduled read decide the next call. Concurrent callers share one in-flight read, so a fresh pod taking simultaneous requests does not open one per `getStaticProps`. Also sets the `cacheVersionTtl` schema default to 3600 to match the repo convention. The `?? 3600` fallback stays: schema defaults are applied by the zod parse but not carried into the generated config values, which is what the runtime imports — `configurableVariantForSimple` has a `= false` default and still generates `undefined`. Verified against a stand-in incremental cache: 25 concurrent refreshes cost one read, a second process picks up a publish, and the client is recreated once per publish and stable in between. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| const dir = './tmp' | ||
| // `undefined` means the signal has not been read yet, which is the state of every pod right after | ||
| // a deploy. Creating the client without invalidating is correct there — a client made now cannot | ||
| // predate a publish — and `renewSignal()` schedules the read that makes the next call decisive. |
There was a problem hiding this comment.
Written by Claude Code: Verwijderd in 013ac97.
| // `undefined` means the signal has not been read yet, which is the state of every pod right after | ||
| // a deploy. Creating the client without invalidating is correct there — a client made now cannot | ||
| // predate a publish — and `renewSignal()` schedules the read that makes the next call decisive. |
There was a problem hiding this comment.
Written by Claude Code: Verwijderd in 013ac97.
| // `undefined` means the signal has not been read yet, which is the state of every pod right after | ||
| // a deploy. Creating the client without invalidating is correct there — a client made now cannot | ||
| // predate a publish — and `renewSignal()` schedules the read that makes the next call decisive. |
There was a problem hiding this comment.
Written by Claude Code: Verwijderd in 013ac97.
| * per slug. `cdn/links` returns at most 1000 entries per request, so this also caps the index build | ||
| * at ten requests and the retained set at a few hundred kilobytes. | ||
| */ | ||
| const MAX_INDEXED_LINKS = 10000 |
There was a problem hiding this comment.
Remove the limit completely, always cache everything, we'll tackle this when we get here. This would explode if this limit is ever exceeded, more RAM is a better in this case.
There was a problem hiding this comment.
Written by Claude Code: Limiet volledig weg in 013ac97 — MAX_INDEXED_LINKS en de bijbehorende bail zijn eruit, fetchSlugIndex() pagineert nu altijd door tot total. De index valt alleen nog terug op null als de request faalt.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Written by Claude Code:
Five changes across
@graphcommerce/storyblok-uiand@graphcommerce/graphql, all aimed at the number of requests a storefront sends to Storyblok. Requests are billed per call against the space quota, so this is a cost problem as much as a performance one.1. The cache-version refresh never refreshed anything
refreshStoryblokCacheVersion()requestscdn/spaces/meand relies onstoryblok-js-clientto pick the returned version up. That client only advances its pinnedcvfrom a response with a top-levelcvfield:but
cdn/spaces/meanswers with{ "space": { "version": 1785830443 } }— nocv. Verified against a live space: a fresh client that only callscdn/spaces/mestill reportscacheVersion() === undefined. The pin stayed at whatever the firstcdn/storiesresponse set, which is exactly the staleness the function's docblock says it prevents, and the periodic request bought nothing.space.versionis the same numbercdn/storiesreturns ascv, so the fix is to read and apply it.2. Unknown slugs cost a request each
@paales asked whether a completely random URL creates an API call, since bots generate them by the thousand. It did, and the per-slug negative cache in the first revision did not help: every made-up URL is a fresh slug, so it never got a second chance.
fetchStory()now checks a slug index first. Onecdn/linksrequest per cache-version lists every published slug in the space (~300 bytes per story, 1000 per request); anything outside that set is reported as not found without a request. Measured against a live space, counting outgoing HTTP requests with the client response cache disabled:cdn/storiescdn/linksThings that could silently break pages, checked:
cdn/links?language=dereturned 25 of 63 entries on a space that does not definede, whilecdn/stories/<slug>?language=deresolves all 63 by falling back to the default language. A language-scoped index would report existing pages as missing, so the index is deliberately unscoped.clubkleding/), which keepsfetchStory('clubkleding')working, whileglobalandmodal— folders without one — correctly report as missing (both verified 404 against the CDN).cdn/linksrequest, or a space above 10 000 stories, yields "unknown" and the lookup falls through to the CDN. Verified with the index forced unavailable: a known slug still resolves.3. Freshness is pushed, not polled
requestStoryblokCacheVersionRefresh()— what a cache-notify webhook calls on publish — only flipped a flag in the process that received the webhook. Every other server learned about the change throughcacheVersionTtl: onecdn/spaces/meper server per interval, forever, whether or not anything was published. At the old 60 second default that is ~446 000 billed requests a month on a ten-replica deployment, spent discovering that nothing happened.The webhook now publishes a renew signal that every server reads before a published read (throttled to once a second, never an API request). A server seeing a newer signal fetches the cache-version once.
cacheVersionTtlmoves to 3600 as the failsafe, which brings the same deployment to roughly 8 000 requests a month while making publishes visible faster than before.How the signal travels — and what it leans on. It goes through
globalThis.__incrementalCache, so it reaches every server sharing acacheHandler. That is the same requirement a deployment already meets to serve consistent ISR, so there is nothing extra to configure — and no config key, which is the point: an earlier revision of this PR used a file in acacheVersionSignalDir, which only works if every replica mounts the same volume.Being explicit about the risk, since this is two internal APIs:
globalThis.__incrementalCacheis set inhandleRequestImpl(next/dist/server/base-server.js) on every incoming request, ahead of API routes and ISR regeneration, under the same name and shape in Next 13, 14, 15 and 16. Middleware and the edge runtime construct their own instance and are not covered.kind: 'FETCH'withfetchCache: true— the one path that stores a key verbatim instead of running it throughnormalizePagePath(), which is what allows a namespaced key.setis a no-op andgetreturnsnull, never a throw, and freshness falls back tocacheVersionTtl. Both calls are wrapped anyway, because a non-FETCH value on the key would raiseInvariantError E653; the key is namespaced so nothing else can write it.revalidatebecamecacheControlbetween Next 14 and 15 — so it needs re-testing on each Next major.FileSystemCacheneedcacheMaxMemorySize: 0; its in-memory LRU sits in front of the shared layer and would answer every read from the writing process' own memory. Custom cache handlers have no such layer. Documented oncacheVersionTtlinConfig.graphqls.Cross-pod behaviour validated on a live two-pod cluster: pod A writes, pod B reads the value within 9 ms, and updates propagate on the next read.
4. One signal, not two
graphqlSsrClient()in the examples solved the same problem its own way: it dropped its per-locale Apollo client when the timestamp in./tmp/renew-all-pages-query.txtmoved. That path resolves inside each container, so on a multi-replica deployment it silently invalidated nothing — the same assumption that made a file wrong for Storyblok. Both now read one signal, so a single publish invalidates both caches.The helper lives in
@graphcommerce/graphql:storyblok-uipeer-depends on it, it does not depend onstoryblok-ui, and all three examples already import from it. The key is neutral (gc:signal:content-renew) because the fact being published is "content changed".The sync/async problem.
graphqlSsrClient()is synchronous and called asconst client = graphqlSsrClient(context)throughoutgetStaticProps; making it async would break every consumer in every project. SorenewSignal()returns the last read value and schedules the next refresh — staleness is bounded at one poll interval plus one call, which is well inside what this is for.Cold start is handled explicitly.
renewSignal()returnsundefineduntil the first read completes, which is deliberately distinct from "read, and nothing was published". Every pod is in that state right after a deploy — exactly when all pods are replaced at once — and treating it as0would silently skip the first invalidation. Callers create the client without invalidating (a client made now cannot predate a publish) and let the scheduled read decide the next call. Concurrent callers share one in-flight read, so a fresh pod under simultaneous load does not open one pergetStaticProps.Verified against a stand-in incremental cache: 25 concurrent refreshes cost one read, a second process picks up a publish, and the client is recreated once per publish and stable in between.
5.
getStoryblokStaticPaths()downloaded every story bodyIt only reads
full_slug. The content sitemap in the examples calls it fromgetServerSideProps, so every crawler hit re-downloaded the entire space: 412 KB → 62 KB on the space I measured.Compatibility
No signature changes for callers, including
graphqlSsrClient().storyblok.cacheVersionTtlchanges default from 60 to 3600; projects that set it explicitly are unaffected. The slug index, the version pin and the last-known signal are process-local.🤖 Generated with Claude Code