diff --git a/.changeset/defer-regional-cache-write.md b/.changeset/defer-regional-cache-write.md new file mode 100644 index 000000000..03bc25add --- /dev/null +++ b/.changeset/defer-regional-cache-write.md @@ -0,0 +1,9 @@ +--- +"@opennextjs/cloudflare": patch +--- + +fix: defer regional cache writes so they do not block the response + +Next.js awaits `incrementalCache.set` while producing the response, which put the R2 +write on the critical path. The returned value is unused, so the writes now run in +`ctx.waitUntil` — matching how the read path already defers its cache updates. diff --git a/packages/cloudflare/src/api/overrides/incremental-cache/regional-cache.spec.ts b/packages/cloudflare/src/api/overrides/incremental-cache/regional-cache.spec.ts new file mode 100644 index 000000000..75744f36c --- /dev/null +++ b/packages/cloudflare/src/api/overrides/incremental-cache/regional-cache.spec.ts @@ -0,0 +1,79 @@ +import type { IncrementalCache } from "@opennextjs/aws/types/overrides.js"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; + +import { getCloudflareContext } from "../../cloudflare-context.js"; +import { withRegionalCache } from "./regional-cache.js"; + +vi.mock("../../cloudflare-context.js", () => ({ + getCloudflareContext: vi.fn(), +})); + +const mockedStore = { + name: "mocked-store", + get: vi.fn().mockResolvedValue(null), + set: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(undefined), +} satisfies IncrementalCache; + +const mockedWaitUntil = vi.fn(); +const mockedPut = vi.fn(); + +describe("regional-cache", () => { + beforeEach(() => { + // @ts-ignore + globalThis.caches = { + open: vi.fn().mockResolvedValue({ + put: mockedPut, + match: vi.fn().mockResolvedValue(undefined), + }), + }; + vi.mocked(getCloudflareContext).mockReturnValue({ + ctx: { waitUntil: mockedWaitUntil }, + // @ts-ignore only `ctx` is used here + env: {}, + }); + }); + + afterEach(() => { + vi.resetAllMocks(); + }); + + describe("set", () => { + test("does not wait for the store write to complete", async () => { + let resolveStoreWrite: () => void = () => {}; + mockedStore.set.mockReturnValue( + new Promise((resolve) => { + resolveStoreWrite = resolve; + }) + ); + const cache = withRegionalCache(mockedStore, { mode: "long-lived" }); + + await cache.set("key", { type: "route", body: "", meta: {} }); + + // The store write is still pending, yet `set` has already resolved. + expect(mockedWaitUntil).toHaveBeenCalledTimes(1); + resolveStoreWrite(); + await mockedWaitUntil.mock.calls[0]![0]; + expect(mockedStore.set).toHaveBeenCalledTimes(1); + }); + + test("writes to the store and the regional cache in the background", async () => { + const cache = withRegionalCache(mockedStore, { mode: "long-lived" }); + + await cache.set("key", { type: "route", body: "", meta: {} }); + await mockedWaitUntil.mock.calls[0]![0]; + + expect(mockedStore.set).toHaveBeenCalledWith("key", { type: "route", body: "", meta: {} }, undefined); + expect(mockedPut).toHaveBeenCalledTimes(1); + }); + + test("does not reject when the store write fails", async () => { + mockedStore.set.mockRejectedValue(new Error("store is unavailable")); + const cache = withRegionalCache(mockedStore, { mode: "long-lived" }); + + await expect(cache.set("key", { type: "route", body: "", meta: {} })).resolves.toBeUndefined(); + // A rejected promise handed to `waitUntil` would surface as an unhandled rejection. + await expect(mockedWaitUntil.mock.calls[0]![0]).resolves.toBeUndefined(); + }); + }); +}); diff --git a/packages/cloudflare/src/api/overrides/incremental-cache/regional-cache.ts b/packages/cloudflare/src/api/overrides/incremental-cache/regional-cache.ts index 2513d0e0d..cd98d8d46 100644 --- a/packages/cloudflare/src/api/overrides/incremental-cache/regional-cache.ts +++ b/packages/cloudflare/src/api/overrides/incremental-cache/regional-cache.ts @@ -179,24 +179,31 @@ class RegionalCache implements IncrementalCache { value: CacheValue, cacheType?: CacheType ): Promise { - try { - debugCache("RegionalCache", `set ${key}`); - - await this.store.set(key, value, cacheType); - - await this.putToCache({ - key, - cacheType, - entry: { - value, - // Note: `Date.now()` returns the time of the last IO rather than the actual time. - // See https://developers.cloudflare.com/workers/reference/security-model/ - lastModified: Date.now(), - }, - }); - } catch (e) { - error(`Failed to set the regional cache`, e); - } + debugCache("RegionalCache", `set ${key}`); + + // Next.js awaits `set` while it produces the response, so awaiting the writes here + // puts them on the critical path. The returned value is not used, so they can run + // in the background without changing what is served. + getCloudflareContext().ctx.waitUntil( + (async () => { + try { + await this.store.set(key, value, cacheType); + + await this.putToCache({ + key, + cacheType, + entry: { + value, + // Note: `Date.now()` returns the time of the last IO rather than the actual time. + // See https://developers.cloudflare.com/workers/reference/security-model/ + lastModified: Date.now(), + }, + }); + } catch (e) { + error(`Failed to set the regional cache`, e); + } + })() + ); } async delete(key: string): Promise {