fix(core): fold the build into the route cache validator - #2374
Conversation
🦋 Changeset detectedLatest commit: 7af2380 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 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 |
@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-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@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: |
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. |
`CacheHint.lastModified` carries the content row's `updated_at`, and Astro emits it as the response `Last-Modified`. The response also depends on the build, because `/_astro/*` filenames are content-hashed and a deployment only serves its own. After a deploy that changes only code the validator is unchanged, so a returning visitor's conditional request is answered with 304 and the browser keeps HTML referencing assets the new deployment no longer has — 404 on Workers Assets, leaving the page without CSS or JavaScript. A response-derived ETag would be the obvious remedy, but Cloudflare strips `ETag` from Worker HTML responses, so `Last-Modified` has to carry it. The middleware now folds a build timestamp, exported from a new `virtual:emdash/build` module, into the validator for on-demand responses. Astro keeps the later of two dates, so a route's own hint still wins whenever content is newer. Prerendered pages stay untouched — the host's static layer manages its own validators. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
afcb762 to
08766d5
Compare
There was a problem hiding this comment.
This is the right fix for the right problem, and it fits EmDash’s architecture well. Folding a build-time dimension into CacheHint.lastModified through a new virtual:emdash/build module is a minimal, adapter-agnostic way to stop returning visitors from keeping stale HTML after a code-only deploy. The implementation is correct:
applyBuildValidatorruns beforenext(), so a route’s own content hint still wins when newer, andAstro.cache.set(false)still opts a route out.- Prerendered pages are skipped (
context.isPrerendered),context.cache?.enabledguards the no-cache-provider case, and the new import is properly ts-ignored like the other virtual modules. - Tests cover the accumulation semantics, the opt-out ordering, the prerendered skip, and the disabled-cache skip; the core
vitest.config.tsstub prevents existing unit tests from breaking on the new virtual module. - No new database queries are added to any route, so logged-out query-count snapshots are unaffected.
- The changeset is a proper patch note for users.
I read the diff, the changed source files, the virtual-module plugin, the other middleware files to confirm ordering, and the demo cache usage. pnpm test/lint/typecheck are not run in this environment, so those claims are unverified.
The only finding is a comment-style issue: the new test file opens with a PR-description-style block comment.
Co-authored-by: emdashbot[bot] <273199577+emdashbot[bot]@users.noreply.github.com>
|
Applied the suggestion in CI on that SHA is 43/44 green. The one failure is It appears unrelated to this PR, which touches
The entire delta between the last two is the 10-line comment removal in a core test file. For what it's worth, the failure mode looks structural rather than incidental. The dialog's role list is Subscriber (10) / Contributor (20) / Author (30) / Editor (40), the fixture domain starts at Author (30), and the expected Editor (40) is exactly one step down — but the test focuses the trigger and sends two Both of those are Separately: the I can push an empty commit to re-trigger, but that reshuffles all 44 checks — a re-run of the failing job (or of the bot) would be cheaper if you have a moment. |
MA2153
left a comment
There was a problem hiding this comment.
I checked the branch out and verified this independently rather than taking the PR body at its word. Everything holds up:
- Removing the
applyBuildValidator(context)call reproduces your quoted failure exactly (expected 1786120009000 to be 1786141609000), and moving the call to afternext()failsleaves a route that opts out of caching opted out. Both tests fail for the right reason — the ordering test in particular is pinning a real constraint, not just describing one. - The three claims about Astro's semantics match
core/cache/runtime/cache.jsin 7.0.0: later date wins (L35-39),set()clears a priorset(false)(L22), and a barelastModifiednever emits headers (APPLY_HEADERSreturns early at L73, andIS_ACTIVEagrees at L81). That last one is what makes running this on every route, including/_emdash/*, safe — worth having stated it explicitly. enabledisfalseon bothNoopAstroCacheandDisabledAstroCacheandtrueonly on the real one, so thecache?.enabledguard is right for dev and for the no-provider case.- Registration is complete — all four touchpoints an existing virtual module has, and the import stays external in the built
dist/astro/middleware.mjs. - Locally: 208
tests/unit/astrotests pass, lint is 0 diagnostics, typecheck and build are clean.
The issue write-up did most of the review work up front — tracing it to the specific lines and ruling out ETag before filing meant there was nothing to reconstruct here. Checking blog.cloudflare.com was the detail that settles it: same header shape on Cloudflare's own Astro site, so this is platform default behavior rather than a misconfiguration, which is what makes it core's problem to fix.
Not binding core to cloudflare:workers was the right call, and I'd have pushed back if you'd gone the CF_VERSION_METADATA route. Adapter-agnostic is what core should be, and it fixes Node deployments for free.
Two things before this goes in — neither is a blocker.
1. Please add a line about rollbacks. A build timestamp only moves forward, and Last-Modified expresses newer, not different. Rolling back from version B to version A means the browser holds B's HTML with validator tB, version A answers with max(content, tA) where tA < tB, so the conditional request is 304 and the browser stays on B's HTML referencing /_astro/* names that A never had — the reported failure, in the mirror direction.
This isn't a regression (content-only validators fail the same way) and CF_VERSION_METADATA wouldn't have saved it either, since timestamp is when the version was created and a rollback redeploys an existing version. So I'm not asking you to solve it. But the PR currently reads as though the validator now describes the response, and it describes it in one direction only. A sentence in the module comment naming the limitation would stop the next person from concluding the case is closed. Given that Workers rollbacks are a first-class operation, it may deserve its own follow-up issue.
2. keeps a content validator newer than the build only exercises its own mock. createCache reimplements Astro's accumulation rules, so that assertion tests the mock's max() rather than anything in packages/core. If Astro switched to last-write-wins, the test would stay green while production broke. I confirmed the mock matches 7.0.0 exactly, and AstroCache isn't publicly exported (only ./cache/memory and ./cache/provider-utils), so there's no clean way to drive the real class — I don't think a deep import into dist/ is worth it. I'd just drop that case, or reframe it to assert what we actually control: that set() receives the build date and receives it before next(). The other four are testing real behavior and should stay.
On the flaky AllowedDomainsSettings test — your read looks right, and it's clearly unrelated to a packages/core-only change. A separate PR selecting the option via getByRole("option", …) instead of arrow-key navigation would be welcome. I'm triggering the bot review now. I can't re-run the browser job, though, so clearing that check will need a push — if you're addressing either point above, the resulting commit will pick it up and there's no need for an empty one.
Deferring the Cache-Control question to its own discussion was the right instinct — that's a behavior change for every existing site and shouldn't ride along here.
There was a problem hiding this comment.
This is the right fix for the right problem. Folding a build-time dimension into CacheHint.lastModified through a new virtual:emdash/build module is minimal, adapter-agnostic, and stops returning visitors from keeping stale HTML after a code-only deploy. The approach fits EmDash's existing virtual-module architecture and the middleware ordering is correct: applyBuildValidator runs before next(), so a route's newer content hint still wins and Astro.cache.set(false) still opts a route out.
I re-checked the full diff and the changed source files. The only issue from the previous review — a PR-description-style block comment at the top of the new test file — has been removed. The remaining comments explain non-obvious external semantics (Astro cache accumulation rules, Vite module-load behavior) and are appropriate for future readers.
No new blocking issues:
applyBuildValidatorcorrectly skips prerendered pages, the no-cache-provider case, and the zero-value stub.- The
virtual:emdash/buildvirtual module is wired through the integration like its siblings, with matching type declarations and a test stub invitest.config.ts. - No database queries are added to any route, so logged-out query-count snapshots are unaffected.
- Tests cover the accumulation semantics, the opt-out ordering, the prerendered skip, and the disabled-cache skip.
- The changeset is a user-focused patch note.
pnpm test/lint/typecheck claims are unverified in this environment, as before. No findings remain.
The suite stands in for Astro's AstroCache with a local `createCache` that reimplements its accumulation rules. `keeps a content validator newer than the build` asserted that the later of the two dates survives, which is a property of that stand-in rather than of anything in this package: removing `applyBuildValidator` entirely leaves the case green, and an upstream switch to last-write-wins would keep it green while production broke. AstroCache is not exported from `astro`, so there is no clean way to drive the real class, and asserting that `set()` received the build date would only restate the call. The remaining four cases each fail on a real regression.
A build timestamp only moves forward, while the failure it prevents is symmetric: rolling back to an earlier build leaves a browser holding the newer build's HTML, whose `/_astro/*` names the restored build never had. The conditional request is answered with 304 and the page stays broken. Nothing here fixes that case, and the validator otherwise reads as though it now describes the response completely. Naming the direction keeps the next reader from concluding the case is closed.
|
Both points are in, on top of a Dropped Named the rollback limitation in the
The red check should clear with this push: the branch now has Re-verified after the deletion, against the merged
The PR body still said five tests in that file; it is four now. On a follow-up for rollbacks: happy to open one, and it would be worth more than a restatement of your paragraph. It would carry a repro over two real deployments plus a |
ascorbic
left a comment
There was a problem hiding this comment.
Makes sense. Thanks for this.
What does this PR do?
Closes #2369
CacheHint.lastModifiedcarries the content row'supdated_at, and Astro emits it as the responseLast-Modified. The response also depends on the build:/_astro/*filenames are content-hashed, and a deployment only serves its own. After a deploy that changes only code, the validator is unchanged — a returning visitor'sIf-Modified-Sinceis answered with 304, and the browser keeps HTML referencing assets the new deployment no longer has. On Workers those requests 404 and the page renders without CSS or JavaScript. Full reproduction and root-cause analysis in #2369.The obvious remedy — a response-derived
ETag— is unavailable on the primary platform: Cloudflare stripsETagfrom Worker HTML responses (verified in the issue), soLast-Modifiedhas to carry the build dimension too.The fix: a new
virtual:emdash/buildmodule exports a build timestamp (captured once per Vite plugin instance, so client/server passes agree), and the middleware folds it into the route cache validator viacache.set({ lastModified })beforenext(). Verified against Astro'sAstroCachesemantics:next()matters:set()clears a previousset(false), so applying it after the route would silently undo a route's explicit opt-out. A test pins this ordering.lastModifiednever activates caching — Astro only emits headers whenmaxAgeor tags are set. Sites that don't useAstro.cachesee no behavior change.I first tried deriving the deploy time from
CF_VERSION_METADATA, but that binds core tocloudflare:workersand requires a binding no template configures. The build timestamp is adapter-agnostic and fixes the same bug on Node deployments.Trade-off, deliberately accepted for minimality: a pure build timestamp invalidates on every deploy, including deploys where the
/_astro/*hashes did not change — cost is one full 200 instead of a 304 per returning visitor per deploy. A hash over the asset manifest would be more precise but couples core to adapter internals. If you consider core the wrong layer for this altogether — the hint arguably describes content correctly, and it is Astro that promotes it to a response header — say so and I'll turn this into a docs note with the middleware workaround from #2369 instead.Not included (possible docs follow-up): without any
Cache-Controlon HTML, browsers fall back to RFC 9111 heuristic freshness and may not revalidate for hours; a default header is a behavior change for every site and deserves its own discussion.Revised 2026-08-10, after MA2153's review: merged
main(picks up #2398), dropped the test case that only exercised its stand-in — the file now has four tests — and documented the rollback limitation in theapplyBuildValidatordoc comment. Details in the review reply. Fullpackages/coreunit suite after the merge: 3610 passed, with the same single pre-existingvirtual-modules.test.tsfailure noted in the checklist.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change) — fullpackages/coreunit suite: 3532 passed; the one failure (virtual-modules.test.ts > watches resolved sandbox plugin entries) also fails on a cleanmaincheckout in my environment (macOS tmpdir) and is untouched by this PRpnpm formathas been runAI-generated code disclosure
Screenshots / test output
The reproducing test, on
mainbefore the fix:After the fix: