diff --git a/.changeset/quiet-404-rewrite-cache.md b/.changeset/quiet-404-rewrite-cache.md new file mode 100644 index 0000000000..a9afd74808 --- /dev/null +++ b/.changeset/quiet-404-rewrite-cache.md @@ -0,0 +1,5 @@ +--- +"emdash": patch +--- + +Fixes duplicate layout queries when anonymous public pages call `Astro.rewrite()`, such as rewriting an unknown slug to `/404`. The rewritten page reuses the original request's cached reads and prefetched layout data. Previews, edit mode, signed-in requests, and rewrites that change locale continue to use fresh request contexts. diff --git a/packages/core/src/astro/middleware.ts b/packages/core/src/astro/middleware.ts index 01f2b6ba61..7a87fb69d2 100644 --- a/packages/core/src/astro/middleware.ts +++ b/packages/core/src/astro/middleware.ts @@ -758,6 +758,44 @@ function keepUnsharedResponsesOutOfRouteCache(context: APIContext, response: Res } } +const ANONYMOUS_RENDER = Symbol.for("emdash:anonymous-render"); + +type RenderLocals = App.Locals & { + [ANONYMOUS_RENDER]?: { + method: string; + locale: string | undefined; + db: unknown; + metrics: RequestMetrics | undefined; + }; +}; + +function canReuseAnonymousRender(context: APIContext): boolean { + const original = (context.locals as RenderLocals)[ANONYMOUS_RENDER]; + const parent = getRequestContext(); + if (!original || !parent) return false; + if (context.request.method !== original.method) return false; + if (context.currentLocale !== original.locale) return false; + if (context.locals.__playgroundDb || context.locals.user || parent.editMode || parent.preview) + return false; + if (parent.db !== original.db || parent.metrics !== original.metrics) return false; + if (context.request.headers.has("authorization")) return false; + if (context.cookies.get(virtualConfig?.sessionCookieName ?? "astro-session")) return false; + if (context.cookies.get("emdash-edit-mode")?.value === "true") return false; + if (context.url.searchParams.has("_preview")) return false; + if (context.url.pathname.startsWith("/_emdash") || isImageEndpointRequest(context)) return false; + const base = import.meta.env.BASE_URL.replace(TRAILING_SLASH_RE, ""); + const path = + base && context.url.pathname.startsWith(`${base}/`) + ? context.url.pathname.slice(base.length) + : context.url.pathname; + if (PUBLIC_RUNTIME_ROUTES.has(path) || isCollectionSitemapPath(path)) return false; + // A newly cacheable target must not reuse data loaded from an unverified object-cache snapshot. + if (virtualConfig?.objectCacheEnabled && context.cache?.enabled && !parent.routeCacheFill) { + return false; + } + return true; +} + export const onRequest = defineMiddleware(async (context, next) => { const { request, locals, cookies } = context; const url = context.url; @@ -776,6 +814,10 @@ export const onRequest = defineMiddleware(async (context, next) => { } } + // Astro re-enters middleware with the same locals while the original render is + // still active. Its outer scope owns prefetch, metrics, and connection cleanup. + if (canReuseAnonymousRender(context)) return next(); + const queryRecorder = isInstrumentationEnabled() ? createRecorder(url.pathname, request.method, request.headers.get("x-perf-phase") ?? "default") : undefined; @@ -980,7 +1022,22 @@ export const onRequest = defineMiddleware(async (context, next) => { }); const runAnon = async () => { const t0 = performance.now(); - const response = await next(); + const renderLocals = locals as RenderLocals; + const previous = renderLocals[ANONYMOUS_RENDER]; + const scope = getRequestContext(); + renderLocals[ANONYMOUS_RENDER] = { + method: request.method, + locale: context.currentLocale, + db: scope?.db, + metrics: scope?.metrics, + }; + let response: Response; + try { + response = await next(); + } finally { + if (previous) renderLocals[ANONYMOUS_RENDER] = previous; + else delete renderLocals[ANONYMOUS_RENDER]; + } timings.push({ name: "render", dur: performance.now() - t0, desc: "Page render" }); timings.push({ name: "mw", dur: performance.now() - mwStart, desc: "Total middleware" }); pushMetricsTimings(timings, metrics); diff --git a/packages/core/src/astro/middleware/redirect.ts b/packages/core/src/astro/middleware/redirect.ts index 8e5e0cb957..6e4bc95a56 100644 --- a/packages/core/src/astro/middleware/redirect.ts +++ b/packages/core/src/astro/middleware/redirect.ts @@ -301,8 +301,14 @@ export const onRequest = defineMiddleware(async (context, next) => { if (!routeCache?.enabled) return run(); const parent = getRequestContext(); if (parent) { + const previousSettings = parent.siteSettings; + requestSettings.live = parent.routeCacheFill === true; parent.siteSettings = requestSettings; - return run(); + try { + return await run(); + } finally { + if (previousSettings) parent.siteSettings = previousSettings; + } } return runWithContext({ editMode: false, siteSettings: requestSettings }, run); }); diff --git a/packages/core/src/astro/middleware/request-context.ts b/packages/core/src/astro/middleware/request-context.ts index 735e20807c..3572023b56 100644 --- a/packages/core/src/astro/middleware/request-context.ts +++ b/packages/core/src/astro/middleware/request-context.ts @@ -302,6 +302,7 @@ export const onRequest = defineMiddleware(async (context, next) => { if (virtualConfig?.objectCacheEnabled && context.cache?.enabled) { const parent = getRequestContext(); if (parent?.siteSettings) parent.siteSettings.live = true; + if (parent?.routeCacheFill) return render(); return runWithContext({ ...parent, editMode: false, routeCacheFill: true }, render); } return render(); diff --git a/packages/core/tests/unit/astro/middleware-layout-prefetch.test.ts b/packages/core/tests/unit/astro/middleware-layout-prefetch.test.ts index 23cdba5101..c04ff9c105 100644 --- a/packages/core/tests/unit/astro/middleware-layout-prefetch.test.ts +++ b/packages/core/tests/unit/astro/middleware-layout-prefetch.test.ts @@ -1,3 +1,5 @@ +import { experimental_AstroContainer as AstroContainer } from "astro/container"; +import { createAstro, createComponent, render } from "astro/runtime/server/index.js"; import { type DialectAdapter, DummyDriver, @@ -7,7 +9,10 @@ import { SqliteIntrospector, SqliteQueryCompiler, } from "kysely"; -import { beforeEach, describe, it, expect, vi } from "vitest"; +import { afterAll, beforeAll, beforeEach, describe, it, expect, vi } from "vitest"; + +import { requestCached } from "../../../src/request-cache.js"; +import { setupTestDatabase, teardownTestDatabase } from "../../utils/test-db.js"; vi.mock("astro:middleware", () => ({ defineMiddleware: (handler: unknown) => handler, @@ -21,6 +26,7 @@ const { MOCK_RUNTIME, createRequestScopedDb, prefetchLayoutData } = vi.hoisted(( MOCK_RUNTIME: { storage: { getPublicUrl: vi.fn((key: string) => `https://media.example.com/${key}`) }, db: {}, + rooms: null, hooks: {}, email: null, configuredPlugins: [], @@ -47,9 +53,13 @@ vi.mock( () => ({ default: { database: { config: { binding: "DB" } }, auth: { mode: "none" } } }), { virtual: true }, ); -vi.mock("virtual:emdash/dialect", () => ({ createDialect: vi.fn(), createRequestScopedDb }), { - virtual: true, -}); +vi.mock( + "virtual:emdash/dialect", + () => ({ createDialect: vi.fn(), createCoalescingDialect: null, createRequestScopedDb }), + { + virtual: true, + }, +); vi.mock("virtual:emdash/media-providers", () => ({ mediaProviders: [] }), { virtual: true }); vi.mock("virtual:emdash/plugins", () => ({ plugins: [] }), { virtual: true }); vi.mock( @@ -64,7 +74,17 @@ vi.mock("virtual:emdash/scheduler", () => ({ createScheduler: null }), { virtual vi.mock("../../../src/emdash-runtime.js", () => ({ DB_INIT_DEADLINE_MS: 30_000, - EmDashRuntime: { create: async () => MOCK_RUNTIME }, + EmDashRuntime: { + create: async () => + new Proxy(MOCK_RUNTIME, { + get: (runtime, key) => + key === "then" + ? undefined + : key in runtime + ? Reflect.get(runtime, key) + : async () => ({ success: true }), + }), + }, })); vi.mock("../../../src/loader.js", () => ({ @@ -88,14 +108,19 @@ function kyselyWith(adapter: DialectAdapter) { }); } -function htmlNavigation() { +function htmlNavigation( + path = "/posts/hello", + locals: Record = {}, + properties: Record = {}, +) { return { - request: new Request("https://example.com/posts/hello", { + request: new Request(`https://example.com${path}`, { headers: { accept: "text/html,application/xhtml+xml" }, }), - url: new URL("https://example.com/posts/hello"), + url: new URL(`https://example.com${path}`), cookies: { get: vi.fn(() => undefined), set: vi.fn() }, - locals: {} as Record, + locals, + ...properties, redirect: vi.fn(), isPrerendered: false, session: { get: vi.fn(async () => null) }, @@ -111,8 +136,30 @@ async function navigate() { } describe("layout prefetch on anonymous HTML navigations", () => { + let realDb: Awaited>; + let queryCount: ReturnType; + + beforeAll(async () => { + realDb = await setupTestDatabase(); + Object.defineProperty(realDb.getExecutor().adapter, "supportsMultipleConnections", { + value: true, + }); + queryCount = vi.spyOn(realDb.getExecutor(), "executeQuery"); + }); + + afterAll(async () => { + queryCount.mockRestore(); + await teardownTestDatabase(realDb); + }); + beforeEach(() => { vi.clearAllMocks(); + queryCount.mockClear(); + prefetchLayoutData.mockImplementation(() => + requestCached("rewrite-layout", async () => + realDb.selectFrom("_emdash_migrations").selectAll().limit(1).execute(), + ).then(() => undefined), + ); }); const adapterWithoutFlag: DialectAdapter = { @@ -139,4 +186,148 @@ describe("layout prefetch on anonymous HTML navigations", () => { expect(createRequestScopedDb).toHaveBeenCalled(); expect(prefetchLayoutData).not.toHaveBeenCalled(); }); + + async function rewrite( + options: { + targetLocals?: Record; + targetPath?: string; + targetProperties?: Record; + } = {}, + ) { + const scope = { db: realDb, commit: vi.fn(), close: vi.fn() }; + createRequestScopedDb.mockReturnValue(scope); + const original = htmlNavigation("/missing-page"); + const response = await onRequest(original, async () => { + await requestCached("rewrite-layout", async () => + realDb.selectFrom("_emdash_migrations").selectAll().limit(1).execute(), + ); + const target = htmlNavigation( + options.targetPath ?? "/404", + options.targetLocals ?? original.locals, + options.targetProperties, + ); + return onRequest(target, async () => { + await requestCached("rewrite-layout", async () => + realDb.selectFrom("_emdash_migrations").selectAll().limit(1).execute(), + ); + return new Response("ok"); + }); + }); + await response.text(); + await new Promise((resolve) => setTimeout(resolve, 10)); + return scope; + } + + it("does not repeat layout reads through Astro's real rewrite pipeline", async () => { + const scope = { db: realDb, commit: vi.fn(), close: vi.fn() }; + createRequestScopedDb.mockReturnValue(scope); + const read = () => + requestCached("rewrite-layout", async () => + realDb.selectFrom("_emdash_migrations").selectAll().limit(1).execute(), + ); + const notFound = createComponent(async (result, props, slots) => { + const Astro = result.createAstro(createAstro(), props, slots); + Astro.response.status = 404; + await read(); + return render`Not found`; + }); + const missing = createComponent(async (result, props, slots) => { + const Astro = result.createAstro(createAstro(), props, slots); + await read(); + return Astro.rewrite("/404"); + }); + const container = await AstroContainer.create({ + manifest: { + middleware: async () => ({ onRequest }), + } as NonNullable[0]>["manifest"], + }); + container.insertPageRoute("/404", notFound); + const direct = await container.renderToResponse(notFound, { + request: new Request("https://example.com/404", { headers: { accept: "text/html" } }), + }); + expect(direct.status).toBe(404); + await direct.text(); + await new Promise((resolve) => setTimeout(resolve, 10)); + const directQueries = queryCount.mock.calls.length; + vi.clearAllMocks(); + const rewritten = await container.renderToResponse(missing, { + request: new Request("https://example.com/missing-page", { + headers: { accept: "text/html" }, + }), + }); + expect(rewritten.status).toBe(404); + expect(await rewritten.text()).toContain("Not found"); + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(directQueries).toBe(1); + expect(queryCount).toHaveBeenCalledTimes(directQueries); + expect(prefetchLayoutData).toHaveBeenCalledOnce(); + expect(createRequestScopedDb).toHaveBeenCalledOnce(); + expect(scope.commit).toHaveBeenCalledOnce(); + expect(scope.close).toHaveBeenCalledOnce(); + }); + + it("reuses the prefetched layout read for an anonymous rewrite", async () => { + const scope = await rewrite(); + + expect(queryCount).toHaveBeenCalledOnce(); + expect(prefetchLayoutData).toHaveBeenCalledOnce(); + expect(createRequestScopedDb).toHaveBeenCalledOnce(); + expect(scope.commit).toHaveBeenCalledOnce(); + expect(scope.close).toHaveBeenCalledOnce(); + }); + + it("does not reuse the cache across independent requests", async () => { + await rewrite(); + await rewrite(); + + expect(queryCount).toHaveBeenCalledTimes(2); + expect(prefetchLayoutData).toHaveBeenCalledTimes(2); + expect(createRequestScopedDb).toHaveBeenCalledTimes(2); + }); + + it("does not reuse the cache when the rewrite has different locals", async () => { + await rewrite({ targetLocals: {} }); + + expect(queryCount).toHaveBeenCalledTimes(2); + expect(createRequestScopedDb).toHaveBeenCalledTimes(2); + }); + + it("does not reuse the cache for a different locale target", async () => { + await rewrite({ targetProperties: { currentLocale: "fr" } }); + + expect(queryCount).toHaveBeenCalledTimes(2); + }); + + it.each(["/_emdash/api/content", "/sitemap.xml"])( + "initializes runtime handlers for a rewrite to %s", + async (targetPath) => { + await rewrite({ targetPath }); + expect(queryCount).toHaveBeenCalledTimes(2); + expect(createRequestScopedDb).toHaveBeenCalledTimes(2); + }, + ); + + it("does not reuse the cache for a preview target", async () => { + await rewrite({ targetPath: "/404?_preview=1" }); + + expect(queryCount).toHaveBeenCalledTimes(2); + }); + + it("closes the scoped database when the rewritten render fails", async () => { + const scope = { db: realDb, commit: vi.fn(), close: vi.fn() }; + createRequestScopedDb.mockReturnValue(scope); + const original = htmlNavigation("/missing-page"); + await expect( + onRequest(original, async () => + onRequest(htmlNavigation("/404", original.locals, {}), async () => { + throw new Error("render failed"); + }), + ), + ).rejects.toThrow("render failed"); + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(createRequestScopedDb).toHaveBeenCalledOnce(); + expect(scope.commit).toHaveBeenCalledOnce(); + expect(scope.close).toHaveBeenCalledOnce(); + }); }); diff --git a/packages/core/tests/unit/astro/middleware-redirect.test.ts b/packages/core/tests/unit/astro/middleware-redirect.test.ts index e8a3aba711..43c2823d2c 100644 --- a/packages/core/tests/unit/astro/middleware-redirect.test.ts +++ b/packages/core/tests/unit/astro/middleware-redirect.test.ts @@ -25,6 +25,8 @@ import type { Database } from "../../../src/database/types.js"; import { waitForDeferredTasks } from "../../../src/deferred-tasks.js"; import { publishRedirectArtifacts } from "../../../src/redirects/artifacts.js"; import { invalidateRedirectCache } from "../../../src/redirects/cache.js"; +import { runWithContext } from "../../../src/request-context.js"; +import { getSiteSettings, setSiteSettings } from "../../../src/settings/index.js"; import { setupTestDatabase, teardownTestDatabase } from "../../utils/test-db.js"; type MiddlewareContext = Parameters[0]; @@ -724,6 +726,32 @@ describe("redirect middleware — responses the route cache stores", () => { expect(selects).toBe(1); }); + it("restores the outer settings tracker after a nested cacheable render", async () => { + const cache = storingRouteCache(); + const outerReads: string[] = []; + + await runWithContext( + { + editMode: false, + routeCacheFill: true, + siteSettings: { onRead: (version) => outerReads.push(version) }, + }, + async () => { + const { context } = buildContext({ pathname: "/page", cache }); + await runMiddleware(context, async () => { + await setSiteSettings({ title: "updated" }, db); + await getSiteSettings(); + return new Response("page"); + }); + + await getSiteSettings(); + }, + ); + + expect(outerReads).toHaveLength(1); + expect(cache.set).toHaveBeenCalledWith(false); + }); + it("does not read the database for a render the route cache will not store", async () => { await warm(); await repo.create({ source: "/page", destination: "/moved", type: 301 }); diff --git a/packages/core/tests/unit/astro/request-context-route-cache.test.ts b/packages/core/tests/unit/astro/request-context-route-cache.test.ts index f42e9f602e..e8969219b8 100644 --- a/packages/core/tests/unit/astro/request-context-route-cache.test.ts +++ b/packages/core/tests/unit/astro/request-context-route-cache.test.ts @@ -42,6 +42,7 @@ import type { Database } from "../../../src/database/types.js"; import { __setObjectCacheBackendForTests, cachedQuery } from "../../../src/object-cache/index.js"; import { generatePreviewToken } from "../../../src/preview/tokens.js"; import { getEmDashEntry } from "../../../src/query.js"; +import { requestCached } from "../../../src/request-cache.js"; import { getRequestContext, runWithContext } from "../../../src/request-context.js"; import { createPostFixture } from "../../utils/fixtures.js"; import { setupTestDatabaseWithCollections, teardownTestDatabase } from "../../utils/test-db.js"; @@ -84,6 +85,20 @@ afterEach(() => { }); describe("route-cache request context", () => { + it("reuses warmed reads when a cache-fill render re-enters for a rewrite", async () => { + const context = buildContext({ cache: { enabled: true, set: vi.fn() } }); + const read = vi.fn(async () => "layout data"); + const response = await onRequest(context, async () => { + await requestCached("layout", read); + return onRequest( + buildContext({ pathname: "/404", cache: context.cache }), + async () => new Response(await requestCached("layout", read)), + ); + }); + expect(await response.text()).toBe("layout data"); + expect(read).toHaveBeenCalledOnce(); + }); + it("marks anonymous production cache renders as route-cache fills", async () => { const cache = { enabled: true, set: vi.fn() }; const context = buildContext({ cache });