Repository navigation
Fix stale route-cache fills with object cache - #3488
Conversation
🦋 Changeset detectedLatest commit: cca7a75 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | f4dc91a | Sep 26 2026, 03:59 PM |
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://codex-object-cache-generation-fence.try.emdashcms.com, https://codex-object-cache-generation-fence-emdash-playground.emdash-cms.workers.dev (commit cca7a75)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is sound and directly addresses the stale route-cache fill problem. Treating any anonymous Worker render with route caching enabled as a potential fill, setting routeCacheFill in request context, and having shouldBypass() load from the database prevents a purged page from being rebuilt from a stale KV snapshot. Keeping the legacy epoch on the write path while current readers stop reading it preserves rolling-deploy compatibility and reduces KV reads.
I checked the changed core files, tests, changeset, and docs. The implementation is consistent: runtime middleware already provides an ALS context (with metrics), request-context re-wraps only when an object cache is configured and route caching might store the response, and all content write paths still call invalidateCollectionCache() which bumps both the versioned and legacy namespaces.
The main finding is a stale docstring: shouldBypass() now has a fourth bypass condition (ctx.routeCacheFill === true) but the comment above it only lists the first three. I left one line-anchored suggestion to update it.
Findings
-
[suggestion]
packages/core/src/object-cache/index.ts:401-404shouldBypass()now returnstruewhenctx.routeCacheFill === true, but the docstring above it only lists edit mode, preview tokens, and isolated databases. An incomplete list here is misleading for future readers deciding whether a new bypass condition belongs in this predicate./** * Requests that must always read live data and never populate the cache: * visual edit mode, preview tokens, isolated databases (playground / DO * preview, whose schema and content diverge from the configured site), * and Astro route-cache fills (which must not rebuild a purged page from a * stale object-cache snapshot). */
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
github-actions
Bot
added
area/core
area/docs
size/L
review/awaiting-author
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
The approach remains sound: treating a Worker-side render with route caching active as a potential route-cache fill, marking it with routeCacheFill, and having cachedQuery bypass the object cache prevents a purged HTML response from being rebuilt from a stale KV snapshot. Reducing current readers to the versioned content epoch while writers continue bumping the legacy one preserves rolling-deploy compatibility and trims KV reads.
This is a re-review; the stale shouldBypass() docstring finding from the previous pass has been addressed. I re-read the core middleware, object-cache, virtual-config, tests, changeset, and docs. The implementation is consistent across the stack: the integration serializes objectCacheEnabled, the middleware sets routeCacheFill only when an object cache is configured, and shouldBypass() now lists all four bypass reasons.
Tests directly exercise the new behavior: they prove that anonymous cache fills bypass the object cache and rebuild from the loader, that the fast path is preserved when route caching is disabled or object cache is absent, and that current readers no longer react to legacy-epoch invalidation. The docs and changeset accurately describe the new semantics.
The one remaining risk is the gate that decides whether a request is a route-cache fill: the code relies on context.cache?.enabled. If the deployed Astro/adapter cache handle does not expose enabled, the fence silently never fires and the reported bug persists. The unit-test mock is shaped like the production assumption, so it won't catch this. I'd prefer the guard fall back to the presence of context.cache itself, which is the conservative signal the PR description describes.
| const response = await next(); | ||
| return toolbarMode === "client" ? injectBootstrap(response) : response; | ||
| }; | ||
| if (virtualConfig?.objectCacheEnabled && context.cache?.enabled) { |
There was a problem hiding this comment.
[suggestion] The fence gates on context.cache?.enabled, which assumes Astro's cache handle exposes an enabled property. If the target Astro/adapter version doesn't provide it, the fix silently becomes a no-op and purged routes will still rebuild from stale KV snapshots. The unit-test mock matches this shape, so it won't reveal a runtime mismatch.
Because a request that reaches rendering with a cache handle present is already a fill candidate, fall back to the presence of the handle when enabled is missing:
| if (virtualConfig?.objectCacheEnabled && context.cache?.enabled) { | |
| const routeCacheMayStore = context.cache?.enabled ?? context.cache !== undefined; | |
| if (virtualConfig?.objectCacheEnabled && routeCacheMayStore) { |
github-actions
Bot
added
review/awaiting-author
|
I checked this concern against the exact Astro runtime pinned by the branch ( |
There was a problem hiding this comment.
The approach is sound and directly addresses the stale route-cache fill problem described in #3257: when a Worker-side render with route caching active is a potential fill, marking it routeCacheFill and having cachedQuery bypass the object cache prevents a purged HTML response from being rebuilt from a stale KV snapshot. Trimming current readers to the versioned content epoch while writers continue bumping the legacy one preserves rolling-deploy compatibility and reduces KV reads, matching the reasoning in #3258.
I re-read the middleware, the object-cache read/invalidation split, the runtime-config serialization, the tests, the docs, and the changeset. The implementation is consistent across the stack, the new regression tests exercise the bypass and fast-path behavior, and the docs/changeset accurately describe the new semantics. I verified that contentCacheNamespaces is now read-only versioned, contentInvalidationNamespaces still bumps both epochs on writes, and contentNamespaces / query.ts use the correct read set.
The only outstanding item is the defensive cache-handle gate carried over from the previous review: the code still gates the fence on context.cache?.enabled, which assumes every adapter/runtime exposes that property. The typecheck passing on the targeted Astro range reduces the risk, but the PR description itself frames the signal as "a request that reaches rendering with a cache handle present," so falling back to the presence of the handle is the more conservative match for the stated intent and protects against adapter/runtime drift.
| const response = await next(); | ||
| return toolbarMode === "client" ? injectBootstrap(response) : response; | ||
| }; | ||
| if (virtualConfig?.objectCacheEnabled && context.cache?.enabled) { |
There was a problem hiding this comment.
[suggestion] The route-cache fill fence still gates on context.cache?.enabled. If the deployed Astro/adapter version exposes the handle but not an enabled property, the fix silently becomes a no-op and purged routes will continue rebuilding from stale KV snapshots. The unit-test mock is shaped like this assumption, so it won't reveal a runtime mismatch.
Because the PR describes the signal as "a request that reaches rendering with a cache handle present," fall back to the presence of the handle when enabled is missing:
| if (virtualConfig?.objectCacheEnabled && context.cache?.enabled) { | |
| const routeCacheMayStore = context.cache?.enabled ?? context.cache !== undefined; | |
| if (virtualConfig?.objectCacheEnabled && routeCacheMayStore) { |
What does this PR do?
Fixes the cache-layer freshness gap reported in #3257 and reduces the redundant epoch reads reported in #3258.
When Astro route caching and the EmDash object cache are both configured, any anonymous render that reaches the Worker is treated as a potential route-cache fill. Object-cache reads are bypassed for that render, so the publish path is:
MISSand reaches the Worker.HITs avoid the Worker and database.Current content readers also stop fetching the legacy
content:<collection>epoch, reducing a typical content query's epoch reads from four to three. Writers continue to bump both the versioned and legacy epochs so older readers remain safe during rolling deployments. This deliberately does not combine all epochs into one KV document: KV has no compare-and-swap update for that document, concurrent updates can overwrite one another, and same-key writes are limited.Query-count snapshots are unchanged: this adds no query shape or round trip. A route-cache fill that would previously have hit the object cache now runs its existing database loader queries. That is a conservative increase in database work on cold, expired, or purged fills in exchange for preventing stale HTML from being stored as fresh. Edge hits still execute neither the Worker nor the database. In a cache-enabled app, an anonymous route that reaches the Worker but is not ultimately stored by the route cache is conservatively fenced too.
Closes #3257
Addresses #3258
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Not applicable; this PR does not change the UI.
Validation completed:
pnpm formatpnpm buildpnpm typecheckpnpm lint:quickpnpm --silent lint:json | jq '.diagnostics | length'returned0pnpm --dir docs buildgit diff --check