Repository navigation
fix(core): purge Workers Cache source paths when redirects are saved - #3982
Conversation
🦋 Changeset detectedLatest commit: e275b5c 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 Preview to Cloudflare 🚀Preview URL: https://bot-fix-3978.try.emdashcms.com, https://bot-fix-3978-emdash-playground.emdash-cms.workers.dev (commit e275b5c)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
@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
upgrade-emdash
@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: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR targets the right problem — Workers Cache can serve a stale 200 at a redirect source path after the redirect is written — but the chosen purge mechanism is inconsistent with EmDash's cache model and almost certainly does not work on Cloudflare Workers.
Across the whole codebase (content, settings, menus, taxonomies, widget-areas) the Astro route cache is invalidated with cache.invalidate({ tags: [...] }), backed by CacheHint tags. The deployment docs also describe Cloudflare Workers Cache invalidation via cache.purge({ tags: [...] }). The redirect middleware even notes that a 404 cannot be purged by path because Astro only exposes tag-based handles. Yet this PR introduces cache.invalidate({ path }) and asserts it in tests without any evidence that the Astro/Cloudflare provider supports path-shaped invalidation. If it does not, the write routes run a no-op, the stale 200 survives, and the changeset over-promises a fix.
I also checked: packages/core/tsconfig.json excludes src/astro/**, so the claimed pnpm typecheck clean result does not cover the modified route files. The new helper file is checked, but the route code that consumes { path } is not.
Before merge the author should either (a) demonstrate that context.cache.invalidate({ path }) is supported by the pinned Astro/Cloudflare versions and actually evicts cached responses, or (b) switch to tag-based invalidation (e.g. emit a stable redirect/source tag and ensure rendered pages carry that tag, or piggy-back on whatever path tag the adapter already writes, if any). A regression test that only mocks cache.invalidate and asserts the exact object the code constructs will pass regardless of whether the provider accepts that shape, so it needs to exercise real invalidation or at least verify the contract against the provider's type.
| const alt = toggledTrailingSlash(source); | ||
| if (alt) paths.add(alt); | ||
| } | ||
| return Array.from(paths, (path) => ({ path })); |
There was a problem hiding this comment.
[needs fixing] Returns { path } cache-invalidation specs, but EmDash's route-cache abstraction (and the Cloudflare Workers provider it targets) uses tag-based invalidation. Every other cache.invalidate() call in the codebase passes { tags: string[] }, CacheHint only has tags/lastModified, and the deployment docs show cache.purge({ tags: [...] }). If the provider does not accept { path } these specs are silently ignored and #3978 is not fixed. Confirm the Astro/Cloudflare cache API supports { path } or emit tags instead.
| return Array.from(paths, (path) => ({ path })); | |
| return Array.from(paths, (path) => ({ tags: [path] })); |
There was a problem hiding this comment.
Please keep { path } here. The Cloudflare adapter tags every response with its path (astro-path:<pathname>), and invalidate({ path }) clears exactly that tag. { tags: [path] } wouldn't match any page.
~ 🤖 Claude Code (Opus 5.5)
| /** | ||
| * Cache invalidation specs for redirect source paths. | ||
| * | ||
| * Workers Cache tags cached pages by request pathname. When a redirect is | ||
| * written, purging the source path (and its trailing-slash variant) clears |
There was a problem hiding this comment.
[needs fixing] The docstring claims "Workers Cache tags cached pages by request pathname", which contradicts the rest of the codebase. EmDash tags cached pages by collection/entry identifiers, settings, menus, taxonomies, etc. (see CacheHint, chrome-tags.ts, and the deployment docs). Public pages are not automatically tagged by their URL pathname here, so this premise is unfounded and should be fixed or removed.
| const result = await handleRedirectCreate(db, body); | ||
| if (result.success && cache?.enabled) { | ||
| for (const spec of redirectSourceCachePaths(result.data.source)) { | ||
| await cache.invalidate(spec); |
There was a problem hiding this comment.
[needs fixing] Calls cache.invalidate(spec) with { path } objects. This shape is not used anywhere else in the codebase and is unlikely to be understood by the Cloudflare Workers cache provider. The same concern applies to the PUT and DELETE handlers in [id].ts.
| const result = await handleRedirectUpdate(db, id, body); | ||
| if (result.success && cache?.enabled) { | ||
| for (const spec of redirectSourceCachePaths(before.data.source, result.data.source)) { | ||
| await cache.invalidate(spec); |
There was a problem hiding this comment.
[needs fixing] Calls cache.invalidate(spec) with { path } objects generated by redirectSourceCachePaths. The rest of the admin API invalidates by { tags }; the deployment docs only demonstrate tag-based cache.purge. Unless the Astro/Cloudflare adapter explicitly supports path-based invalidation, this will not evict the stale 200 page at the redirect source.
| const result = await handleRedirectDelete(db, id); | ||
| if (result.success && cache?.enabled) { | ||
| for (const spec of redirectSourceCachePaths(before.data.source)) { | ||
| await cache.invalidate(spec); |
There was a problem hiding this comment.
[needs fixing] Same path-based cache.invalidate(spec) call as the PUT handler. If the provider only supports tags, the cached response at the deleted redirect's source path will not be purged.
| expect(response.status).toBe(201); | ||
| expect(invalidate).toHaveBeenCalledTimes(2); | ||
| expect(invalidate).toHaveBeenCalledWith({ path: "/posts/notes-on-simplicity" }); |
There was a problem hiding this comment.
[needs fixing] This test (and the rest of the file) mocks cache.invalidate and asserts it was called with the exact { path } object the implementation constructs. That pattern cannot fail when the real API does not support { path }; it only restates the diff. Replace with a test that exercises actual eviction or asserts against the provider's contract (e.g., { tags }), per AGENTS.md: "A test must be able to fail on a real regression."
danielmlr
left a comment
danielmlr
left a comment
There was a problem hiding this comment.
The main change looks right, but one thing needs fixing before this can merge.
First, please keep { path }. The earlier review suggested switching to { tags: [path] }, but that would break the fix. The Cloudflare adapter (@astrojs/cloudflare, 14.3.1 here and 14.3.4 latest) gives every response a tag made from its path, astro-path:<pathname>, and invalidate({ path }) clears exactly that tag. { tags: [path] } would clear a tag that no page has. The new tests are fine as they are too: all four fail if you undo the route changes.
What needs fixing: if clearing the cache fails, saving a redirect shows an error even though the redirect was saved. The await cache.invalidate(spec) call sits inside the route's try, after the redirect is already written to the database. So when invalidate throws, for example with TypeError: cache.purge is not a function on a Worker Preview, creating, editing and deleting a redirect all return a 500. The change is stored anyway, and creating the same redirect again then fails because it already exists. Please catch the error, log it with a [cache] prefix, and return the normal response. Add a test where invalidate throws and each route still returns 201 or 200 with the change saved. Only change those three calls, and leave { path } and the redirect handlers as they are.
Please leave these as they are: clearing the path both with and without a trailing slash, clearing both the old and the new source when a redirect is edited, and doing nothing for pattern sources like /blog/[slug], which don't have a single path to clear. The short window described in #3978, where other Worker instances still use the old list of redirects, is a separate problem and not part of this PR.
~ 🤖 Claude Code (Opus 5.5)
| const result = await handleRedirectCreate(db, body); | ||
| if (result.success && cache?.enabled) { | ||
| for (const spec of redirectSourceCachePaths(result.data.source)) { | ||
| await cache.invalidate(spec); |
There was a problem hiding this comment.
If invalidate throws here, the error ends up in the catch below and the request returns a 500, even though handleRedirectCreate has already saved the redirect. Catch and log the error here so the request still succeeds. The same goes for editing ([id].ts:70) and deleting ([id].ts:100).
~ 🤖 Claude Code (Opus 5.5)
| "emdash": patch | ||
| --- | ||
|
|
||
| Fixes redirect saves so the cached page at the redirect source path is purged when the redirect is created, updated, or deleted. With Workers Cache enabled, the next request now reaches the worker and runs the redirect middleware instead of returning a stale 200 response. |
There was a problem hiding this comment.
Optional: wording that starts with what a site owner actually notices.
| Fixes redirect saves so the cached page at the redirect source path is purged when the redirect is created, updated, or deleted. With Workers Cache enabled, the next request now reaches the worker and runs the redirect middleware instead of returning a stale 200 response. | |
| Fixes new and changed redirects being ignored for pages already in the Workers Cache. Saving a redirect for an exact path now clears the cached page at that path. |
~ 🤖 Claude Code (Opus 5.5)
CompletedRevise redirect cache-invalidation candidate to catch and log edge purge failures so the API call still succeeds after the database mutation.
Outcome: Revised the PR #3982 candidate per the maintainer review feedback. The original redirect save routes now call cache.invalidate({ path }) for each affected source path (preserving the trailing-slash variant), and edge-purge failures are caught and logged so the redirect mutation still returns success. Key changes:
Verification:
Mode: revise View live dashboard · Run: |
…are saved (#3978) EmDash-Run: e55c799f-460c-403d-ae4e-eace95d73f6e
760d872 to
e275b5c
Compare
FailedA maintainer requested changes.
Outcome: I couldn't prepare the investigation workspace: Sandbox operation utils.createSession was interrupted while the runtime connection was closing Failed stage: workspace Mode: revise View live dashboard · Run: |
|
@danielmlr On the separate problem at the end of your review: I've put that in #3997. On a fresh install it turned out to be more than a short window. With this PR alone, the purged page was cached again straight away by a Worker instance still on the old redirects, and the plain URL never returned 301 in my runs. With #3997 as well, it returned 301 within a second. #3997 doesn't touch any file in this PR. |
danielmlr
left a comment
danielmlr
left a comment
There was a problem hiding this comment.
The failed-purge handling looks right. A failed purge is logged and the save still succeeds, and the three new tests fail with a 500 when the catch is taken out.
The remaining part of #3978, where Worker instances that still have the old redirect rules cache the page again, is in #3997.
Thanks @bimsonz for splitting it out and measuring it.
I changed Fixes #3978 and Closes #3978 in the description to Refs #3978, so merging this does not close the issue.
What does this PR do?
Refs #3978.
The redirect write routes never invalidated the Astro/Workers route cache, so a cached 200 page at a redirect source path continued to be served after the redirect was created, updated, or deleted.
Changes
packages/core/src/cache/redirect-paths.tsto generate{ path }invalidation specs for a redirect source path and its trailing-slash variant.cache.invalidate()intoPOST /_emdash/api/redirects,PUT /_emdash/api/redirects/:id, andDELETE /_emdash/api/redirects/:id.packages/core/tests/unit/astro/redirect-routes-cache-invalidate.test.tsas a regression test.Verification
pnpm typecheckis clean.pnpm lint:quickreports 0 diagnostics.pnpm run --filter emdash buildsucceeds.A changeset is included for the
emdashpackage.Refs #3978.
Try the candidate against your own site with the preview build:
Opened automatically by emdashbot. A maintainer must review before merge.
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