Repository navigation
fix(redirects): check redirect rules are current before the route cache stores a page - #3997
Conversation
…he stores a page The redirect middleware serves each Worker isolate's cached rules and revalidates them in the background after 30 seconds, and an idle isolate answers its first request after that with the rules it had. Behind the route cache, a render made under stale rules is stored and served to every visitor: purging the page at a new redirect's source is followed by a fill that renders and caches the old page again. Before the route cache stores a response, the middleware now checks that its rules are current (one read of the redirect state row), reloads them when they are not, and answers with the redirect if the path now matches one. A stored redirect for a rule that has since been removed is checked the same way. Responses the route cache does not store, and sites without a route cache, keep the zero-query path. The render a late redirect replaces is left to finish in the background rather than cancelled. A render cut off with its request never settles an isolate-wide fetch it started (the taxonomy definitions cache shares one in-flight query between requests), and every later render on that isolate waiting for it hangs until the platform's time limit.
🦋 Changeset detectedLatest commit: 193825b 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 |
github-actions
Bot
added
review/needs-review
@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: |
danielmlr
added
bot:review
github-actions
Bot
removed
the
ci:run
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right follow-up to #3982. The underlying problem is real: purging the route cache is not enough when a stale Worker isolate renders the old page and re-fills the cache, so verifying the redirect rules before anything is stored is a sound fix. The implementation reuses the existing artifact/state machinery and keeps the new cost gated to responses Astro will actually cache.
I checked the changed middleware, the verify path in loadCachedRedirects, the RedirectSource implementation, the middleware ordering, the new tests, and the changeset. The logic for added/deleted/slug-change auto-redirects holds up, and the decision to drain the replaced render in after() rather than cancel it is well justified (it avoids hanging the isolate-wide taxonomy fetch the cancelled render started).
The query-count tradeoff is explicitly justified in the PR description and changeset: only cache-fill responses pay the read, while edge hits, 404s, and sites without a route cache keep the zero-query path. The tests cover the new behavior, including the zero-query non-cache case and the drain-to-completion path.
One minor style note: the codebase separates type-only imports with import type; the diff mixes inline type modifiers into a value import.
emdashbot
Bot
added
review/awaiting-author
Co-authored-by: emdashbot[bot] <273199577+emdashbot[bot]@users.noreply.github.com>
github-actions
Bot
added
review/needs-rereview
danielmlr
left a comment
danielmlr
left a comment
There was a problem hiding this comment.
Thanks @bimsonz, this closes the rest of emdash-cms/emdash#3978. I applied the bot's import suggestion and changed Refs #3978 to Closes #3978 in the description, since this and emdash-cms/emdash#3982 together fix what the issue reports.
|
Thanks for contributing to EmDash! 🎉 Join the EmDash Discord, then run |
What does this PR do?
This is the second half of the fix for #3978, next to #3982. On #3982 this part was called out as a separate problem, so it is its own PR.
#3982 purges the cached page at a redirect's source when the redirect is saved. On a real deploy, the request that refills that entry often lands on a Worker isolate that hasn't reloaded its redirect rules. It renders the old page, and the route cache stores it again. An isolate revalidates its rules 30 seconds after loading them, and an idle isolate answers its first request after that with the rules it had, so this lasts longer than 30 seconds: on the test site some fills still rendered the old page 78 seconds after the save.
Before the route cache stores a response, the redirect middleware now checks that its rules are current (one read of
_emdash_redirect_state). If they changed, it reloads them, and if the path now matches a redirect it answers with that instead of the page. A redirect response thatrouteRulesmakes cacheable is checked the same way before it is returned, so an isolate that still has a deleted redirect doesn't cache it. A redirect that isn't published yet, such as a slug-change auto-redirect (published inafter()), is covered too: the revision is ahead of the generation, so the reload falls back to the rules table.The page render that a late redirect replaces is left to finish in
after()instead of being cancelled. My first version cancelled it, and on the test site that hung later renders until the 100 second limit: the cancelled render had started the isolate-wide taxonomy definitions query (loadTaxonomyDefRows()shares one in-flight promise between requests), the query died with its request, and every later render on that isolate waited for it. I think a client disconnecting mid-render can hang that cache the same way without this PR, but I haven't reproduced that on its own yet, so I'll report it separately once I have.Query cost: only a response the route cache is going to store pays the read. Astro stores a response once a max age or a tag is set, so the middleware reads
context.cache.optionsafter the render. Edge hits, routes that don't callcache.set(), 404s and sites without a route cache keep the zero-query path. The query-count snapshots don't change, because the perf fixture has no route cache, so they don't cover this case. On a cache-enabled site it is one primary-key read per cache fill.I didn't use
routeCacheFillas the trigger: the redirect middleware runs before request-context, and that flag is only set when an object cache is configured as well.Pattern redirects: #3982 doesn't purge for them, since the paths a pattern matches can't be listed, and this PR doesn't either. With it, every fill applies a pattern redirect at once, but a page already in the cache is served until it expires. A proposal, if you want one: the middleware sees every response the cache stores, so it could add one constant tag to them, and a pattern write could purge that tag. That empties the route cache, but pattern writes are rare. I haven't built it, as that's a decision for you.
Closes #3978
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. (n/a: no admin UI change)AI-generated code disclosure
Screenshots / test output
Not applicable for screenshots (no UI change).
Tests: 9 new, 3 in
tests/unit/redirects/cache.test.tsand 6 intests/unit/astro/middleware-redirect.test.ts. With the source put back tomain, 8 fail. The ninth checks that a response the route cache won't store makes no database read; it passes onmainby design and fails if the check runs on every render. The test that the replaced render runs to completion also fails if the render is cancelled instead.pnpm build,pnpm lint(0 diagnostics),pnpm format:checkandpnpm typecheckpass. Core: 9565 passed, 11 skipped.scripts/query-counts.mjsmatches its snapshots for sqlite and d1.Fresh install, as in #3978: create-emdash 1.2.0 blog template on Cloudflare with
cache: { provider: cacheCloudflare() }androuteRules: { "/": { maxAge: 300, swr: 86400 } }. The same script ran against each build on the same site: make the change, then for about 75 seconds request the plain URL and four cache-busted URLs every few seconds. Each cache-busted request is a cache fill on whichever isolate gets it.Each cell: when the plain URL first gave the expected status, then how many fills gave something else.
/, whichrouteRulescaches//pages/[slug]over a cached page/404No response stalled in the last run, and a second run of 8 save-and-delete rounds with 30 concurrent fills each (1440 requests) had none either.
After a delete or a move, an isolate that still has the old rule answers the old source with a 301 until it reloads its rules, as on 1.2.0. Nothing caches that response unless
routeRulesapplies, which is the/row. In the slug-change row, the fills that miss are stale isolates answering the old slug with the template's redirect to/404, which nothing caches either.