Skip to content

fix(core): fold the build into the route cache validator - #2374

Merged
ascorbic merged 6 commits into
emdash-cms:mainfrom
danielmlr:fix/cache-validator-build-dimension
Aug 12, 2026
Merged

ascorbic merged 6 commits into
emdash-cms:mainfrom
danielmlr:fix/cache-validator-build-dimension

Conversation

@danielmlr

@danielmlr danielmlr commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Closes #2369

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: /_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's If-Modified-Since is 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 strips ETag from Worker HTML responses (verified in the issue), so Last-Modified has to carry the build dimension too.

The fix: a new virtual:emdash/build module 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 via cache.set({ lastModified }) before next(). Verified against Astro's AstroCache semantics:

  • Astro keeps the later of two dates, so a route's own content hint still wins whenever content is newer than the build.
  • Running before next() matters: set() clears a previous set(false), so applying it after the route would silently undo a route's explicit opt-out. A test pins this ordering.
  • A bare lastModified never activates caching — Astro only emits headers when maxAge or tags are set. Sites that don't use Astro.cache see no behavior change.
  • Prerendered pages are skipped: the host's static layer manages its own validators (Workers Assets keeps ETags there).

I first tried deriving the deploy time from CF_VERSION_METADATA, but that binds core to cloudflare:workers and 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-Control on 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 the applyBuildValidator doc comment. Details in the review reply. Full packages/core unit suite after the merge: 3610 passed, with the same single pre-existing virtual-modules.test.ts failure noted in the checklist.

Type of change

  • Bug fix
  • Feature (requires maintainer-approved Discussion)
  • Refactor (no behavior change)
  • Translation
  • Documentation
  • Performance improvement
  • Tests
  • Chore (dependencies, CI, tooling)

Checklist

  • I have read CONTRIBUTING.md
  • pnpm typecheck passes
  • pnpm lint passes
  • pnpm test passes (or targeted tests for my change) — full packages/core unit suite: 3532 passed; the one failure (virtual-modules.test.ts > watches resolved sandbox plugin entries) also fails on a clean main checkout in my environment (macOS tmpdir) and is untouched by this PR
  • pnpm format has been run
  • I have added/updated tests for my changes (if applicable)
  • User-visible strings in the admin UI are wrapped for translation — n/a, no admin UI changes
  • I have added a changeset
  • New features link to an approved Discussion — n/a, bug fix

AI-generated code disclosure

  • This PR includes AI-generated code — model/tool: Claude Opus 5 (draft), Claude Fable 5 (review pass)

Screenshots / test output

The reproducing test, on main before the fix:

FAIL  tests/unit/astro/middleware-cache-validator.test.ts
  × raises a content-only validator to the build time
AssertionError: expected 1786120009000 to be 1786141609000

After the fix:

✓ tests/unit/astro/middleware-cache-validator.test.ts (4 tests)
✓ tests/unit/astro/integration/virtual-modules.test.ts (20 tests)

@changeset-bot

changeset-bot Bot commented Aug 8, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 7af2380

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 17 packages
Name Type
emdash Patch
@emdash-cms/cloudflare Patch
@emdash-cms/sandbox-workerd Patch
@emdash-cms/plugin-mcp-smoke Patch
@emdash-cms/fixture-perf-site Patch
@emdash-cms/perf-demo-site Patch
@emdash-cms/cache-demo-site Patch
@emdash-cms/do-demo-site Patch
@emdash-cms/do-solo-demo-site Patch
@emdash-cms/admin Patch
@emdash-cms/auth Patch
@emdash-cms/blocks Patch
@emdash-cms/gutenberg-to-portable-text Patch
@emdash-cms/x402 Patch
create-emdash Patch
@emdash-cms/auth-atproto Patch
@emdash-cms/plugin-embeds Patch

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

@github-actions github-actions Bot added review/needs-review No maintainer or bot review yet area/core size/L labels Aug 8, 2026
@pkg-pr-new

pkg-pr-new Bot commented Aug 8, 2026

Copy link
Copy Markdown

Open in StackBlitz

@emdash-cms/admin

npm i https://pkg.pr.new/@emdash-cms/admin@2374

@emdash-cms/auth

npm i https://pkg.pr.new/@emdash-cms/auth@2374

@emdash-cms/auth-atproto

npm i https://pkg.pr.new/@emdash-cms/auth-atproto@2374

@emdash-cms/blocks

npm i https://pkg.pr.new/@emdash-cms/blocks@2374

@emdash-cms/cloudflare

npm i https://pkg.pr.new/@emdash-cms/cloudflare@2374

@emdash-cms/contentful-to-portable-text

npm i https://pkg.pr.new/@emdash-cms/contentful-to-portable-text@2374

emdash

npm i https://pkg.pr.new/emdash@2374

create-emdash

npm i https://pkg.pr.new/create-emdash@2374

@emdash-cms/gutenberg-to-portable-text

npm i https://pkg.pr.new/@emdash-cms/gutenberg-to-portable-text@2374

@emdash-cms/plugin-cli

npm i https://pkg.pr.new/@emdash-cms/plugin-cli@2374

@emdash-cms/plugin-types

npm i https://pkg.pr.new/@emdash-cms/plugin-types@2374

@emdash-cms/registry-client

npm i https://pkg.pr.new/@emdash-cms/registry-client@2374

@emdash-cms/registry-lexicons

npm i https://pkg.pr.new/@emdash-cms/registry-lexicons@2374

@emdash-cms/registry-verification

npm i https://pkg.pr.new/@emdash-cms/registry-verification@2374

@emdash-cms/sandbox-workerd

npm i https://pkg.pr.new/@emdash-cms/sandbox-workerd@2374

@emdash-cms/x402

npm i https://pkg.pr.new/@emdash-cms/x402@2374

@emdash-cms/plugin-ai-moderation

npm i https://pkg.pr.new/@emdash-cms/plugin-ai-moderation@2374

@emdash-cms/plugin-atproto

npm i https://pkg.pr.new/@emdash-cms/plugin-atproto@2374

@emdash-cms/plugin-audit-log

npm i https://pkg.pr.new/@emdash-cms/plugin-audit-log@2374

@emdash-cms/plugin-color

npm i https://pkg.pr.new/@emdash-cms/plugin-color@2374

@emdash-cms/plugin-embeds

npm i https://pkg.pr.new/@emdash-cms/plugin-embeds@2374

@emdash-cms/plugin-field-kit

npm i https://pkg.pr.new/@emdash-cms/plugin-field-kit@2374

@emdash-cms/plugin-forms

npm i https://pkg.pr.new/@emdash-cms/plugin-forms@2374

@emdash-cms/plugin-webhook-notifier

npm i https://pkg.pr.new/@emdash-cms/plugin-webhook-notifier@2374

commit: 7af2380

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Overlapping PRs

This 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>
@danielmlr
danielmlr force-pushed the fix/cache-validator-build-dimension branch from afcb762 to 08766d5 Compare August 8, 2026 13:48
@ascorbic ascorbic added the bot:review Trigger an emdashbot code review on this PR label Aug 9, 2026

@emdashbot emdashbot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • applyBuildValidator runs before next(), so a route’s own content hint still wins when newer, and Astro.cache.set(false) still opts a route out.
  • Prerendered pages are skipped (context.isPrerendered), context.cache?.enabled guards 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.ts stub 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.

Comment thread packages/core/tests/unit/astro/middleware-cache-validator.test.ts Outdated
@emdashbot emdashbot Bot removed the bot:review Trigger an emdashbot code review on this PR label Aug 9, 2026
@github-actions github-actions Bot added review/awaiting-author Reviewed; waiting on the author to respond and removed review/needs-review No maintainer or bot review yet labels Aug 9, 2026
Co-authored-by: emdashbot[bot] <273199577+emdashbot[bot]@users.noreply.github.com>
@github-actions github-actions Bot added review/needs-rereview Author pushed changes since the last review and removed review/awaiting-author Reviewed; waiting on the author to respond labels Aug 9, 2026
@danielmlr

danielmlr commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Applied the suggestion in 2f978c49 — the block comment is gone. Thanks for the review, and for merging main in yesterday.

CI on that SHA is 43/44 green. The one failure is Browser Tests:

FAIL tests/components/settings/AllowedDomainsSettings.test.tsx
  > AllowedDomainsSettings > updates the default role from the edit dialog
AssertionError: expected "vi.fn()" to be called with arguments: [ 'example.com', { defaultRole: 40 } ]

  1st vi.fn() call:
-     "defaultRole": 40,
+     "defaultRole": 10,

Number of calls: 1
Tests  1 failed | 1383 passed (1384)

It appears unrelated to this PR, which touches packages/core only. Across the three SHAs this branch has had, git diff -- packages/admin is empty in both directions, so that package is identical throughout — yet the same test flips:

SHA Browser Tests
08766d52 fail
0b9da166 (your main merge) pass
2f978c49 fail

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 {ArrowDown}s before {Enter}. So it only passes when exactly one of them is swallowed (presumably the one that opens the popup) and the highlight lands on the currently selected item on open. The value that actually arrived, Subscriber (10), is the first item in the list, i.e. the highlight was at the top of the list when {Enter} fired.

Both of those are kumo/Base UI timing and focus details rather than anything the test means to assert, which would explain why the same commit passes and fails on different runs. Happy to open a separate PR that picks the option directly (getByRole("option", …)) instead of navigating blind, if you'd like it fixed rather than just re-run.

Separately: the EmDashBot review check did not appear on 2f978c49. It also didn't on 08766d52; it did run on 0b9da166.

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 MA2153 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 after next() fails leaves 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.js in 7.0.0: later date wins (L35-39), set() clears a prior set(false) (L22), and a bare lastModified never emits headers (APPLY_HEADERS returns early at L73, and IS_ACTIVE agrees at L81). That last one is what makes running this on every route, including /_emdash/*, safe — worth having stated it explicitly.
  • enabled is false on both NoopAstroCache and DisabledAstroCache and true only on the real one, so the cache?.enabled guard 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/astro tests 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.

@MA2153 MA2153 added the bot:review Trigger an emdashbot code review on this PR label Aug 10, 2026

@emdashbot emdashbot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • applyBuildValidator correctly skips prerendered pages, the no-cache-provider case, and the zero-value stub.
  • The virtual:emdash/build virtual module is wired through the integration like its siblings, with matching type declarations and a test stub in vitest.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.

@emdashbot emdashbot Bot removed the bot:review Trigger an emdashbot code review on this PR label Aug 10, 2026
@github-actions github-actions Bot added review/approved Approved; no new commits since and removed review/needs-rereview Author pushed changes since the last review labels Aug 10, 2026
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.
@github-actions github-actions Bot added the review/needs-rereview Author pushed changes since the last review label Aug 10, 2026
@github-actions github-actions Bot removed the review/approved Approved; no new commits since label Aug 10, 2026
@danielmlr

Copy link
Copy Markdown
Contributor Author

Both points are in, on top of a main merge.

Dropped keeps a content validator newer than the build rather than reframing it. Your read is right: it only exercised the stand-in, and removing applyBuildValidator entirely leaves it green. Asserting that set() received the build date would restate the call rather than test behavior, which is the tautological pattern AGENTS.md warns about, and the ordering is already pinned by leaves a route that opts out of caching opted out — that one fails for a real reason. Four cases remain, and each of them can fail on a regression.

Named the rollback limitation in the applyBuildValidator doc comment in packages/core/src/astro/middleware.ts, which is where the mechanism and its guarantees are already described:

Only forward moves are covered. Last-Modified expresses newer, not different, so after a rollback the earlier build still answers a conditional request with 304 and the browser stays on the newer build's HTML.

The red check should clear with this push: the branch now has main merged in, which brings #2398. No empty commit, as you suggested.

Re-verified after the deletion, against the merged main:

  • Removing the applyBuildValidator(context) call fails raises a content-only validator to the build time with the same assertion as before (expected 1786120009000 to be 1786141609000).
  • Moving the call after next() fails leaves a route that opts out of caching opted out.
  • pnpm lint reports 0 diagnostics, unchanged from the baseline before these edits; pnpm typecheck is clean; the packages/core unit suite is 3610 passed with the one pre-existing virtual-modules.test.ts > watches resolved sandbox plugin entries failure, which is a macOS tmpdir artifact and also fails on a clean main.

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 wrangler rollback, to confirm the part none of us has actually exercised — that the restored version stops serving the newer version's /_astro/* names, so the 304 lands the browser on HTML whose assets it can no longer load. The framing I would give it is that Last-Modified is ordered while deployments are not, so no timestamp closes this, CF_VERSION_METADATA included; a validator that expresses different rather than newer means an ETag, and Cloudflare strips those from Worker HTML. That puts it in the same place as the Cache-Control question this PR left alone, so one thread covering both may serve better than two that do not know about each other. Issue or Discussion — your call, and it does not need to hold this up.

@ascorbic ascorbic left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thanks for this.

@ascorbic
ascorbic merged commit 70f9ddc into emdash-cms:main Aug 12, 2026
48 checks passed
@emdashbot emdashbot Bot mentioned this pull request Aug 12, 2026
@danielmlr
danielmlr deleted the fix/cache-validator-build-dimension branch August 13, 2026 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core overlap review/needs-rereview Author pushed changes since the last review size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cacheHint.lastModified tracks content only, so a code-only deploy serves 304 and leaves browsers on HTML referencing deleted /_astro/* files

3 participants