Fix tag cache stale logic and update tests (#1193)

Co-authored-by: Victor Berchet <[email protected]>

Author: conico974

.changeset/true-moments-clap.md

@@ -0,0 +1,5 @@
+---
+"@opennextjs/cloudflare": patch
+---
+
+Fix tag cache stale logic

examples/e2e/app-router/e2e/revalidateTag.test.ts

@@ -105,7 +105,7 @@ test("Revalidate tag - stale data served first", async ({ page, request }) => {
 	const staleResponse = await responsePromise;
 	const staleHeaders = staleResponse.headers();
 	const staleCache = staleHeaders["x-nextjs-cache"] ?? staleHeaders["x-opennext-cache"];
-	expect(staleCache).toMatch(/^(STALE|HIT)$/);
+	expect(staleCache).toMatch(/^(STALE)$/);
 
 	const staleTime = await page.getByTestId("cached-time").textContent();
 	// Stale content must match the pre-revalidation value
@@ -129,6 +129,19 @@ test("Revalidate tag - stale data served first", async ({ page, request }) => {
 	// After background regen the cached value must have been updated
 	expect(freshTime).not.toBeNull();
 	expect(freshTime).not.toEqual(originalTime);
+
+	// Now we want to verfiy that the next entries stays fresh (HIT) after the first stale entry
+	responsePromise = page.waitForResponse(
+		(response) => response.url().includes("/revalidate-tag/stale") && response.status() === 200
+	);
+	await page.goto("/revalidate-tag/stale");
+	const finalResponse = await responsePromise;
+	const finalHeaders = finalResponse.headers();
+	const finalCache = finalHeaders["x-nextjs-cache"] ?? finalHeaders["x-opennext-cache"];
+	expect(finalCache).toEqual("HIT");
+
+	const finalTime = await page.getByTestId("cached-time").textContent();
+	expect(finalTime).toEqual(freshTime);
 });
 
 test("Revalidate path", async ({ page, request }) => {

packages/cloudflare/package.json

@@ -54,7 +54,7 @@
 	"dependencies": {
 		"@ast-grep/napi": "^0.40.5",
 		"@dotenvx/dotenvx": "catalog:",
-		"@opennextjs/aws": "3.10.2",
+		"@opennextjs/aws": "3.10.3",
 		"ci-info": "^4.2.0",
 		"cloudflare": "^4.4.1",
 		"comment-json": "^4.5.1",

packages/cloudflare/src/api/overrides/incremental-cache/regional-cache.ts

@@ -5,6 +5,7 @@ import {
 	IncrementalCache,
 	WithLastModified,
 } from "@opennextjs/aws/types/overrides.js";
+import { compareSemver } from "@opennextjs/aws/utils/semver.js";
 
 import { getCloudflareContext } from "../../cloudflare-context.js";
 import { debugCache, FALLBACK_BUILD_ID, IncrementalCacheEntry, isPurgeCacheEnabled } from "../internal.js";
@@ -31,21 +32,33 @@ type Options = {
 	defaultLongLivedTtlSec?: number;
 
 	/**
-	 * Whether the regional cache entry should be updated in the background or not when it experiences
-	 * a cache hit.
+	 * Whether the regional cache entry should be updated in the background on regional cache hits.
 	 *
-	 * @default `true` in `long-lived` mode when cache purge is not used, `false` otherwise.
+	 * NOTE: Use the default value unless you know what you are doing. It is set to:
+	 * - Next < 16:
+	 *   `true` in `long-lived` mode when cache purge is not used, `false` otherwise.
+	 * - Next >= 16:
+	 *   `!bypassTagCacheOnCacheHit`
 	 */
 	shouldLazilyUpdateOnCacheHit?: boolean;
 
 	/**
-	 * Whether on cache hits the tagCache should be skipped or not. Skipping the tagCache allows requests to be
-	 * handled faster,
+	 * Whether the tagCache should be skipped on regional cache hits.
 	 *
-	 * Note: When this is enabled, make sure that the cache gets purged
-	 *       either by enabling the auto cache purging feature or manually.
+	 * Note:
+	 * - Skipping the tagCache allows requests to be handled faster
+	 * - When `true`, make sure the cache gets purged
+	 *   either by enabling the auto cache purging feature or manually
 	 *
-	 * @default `true` if the auto cache purging is enabled, `false` otherwise.
+	 * `true` is not compatible with SWR types of revalidateTag
+	 * i.e. on Next 16+, anything different than `revalidateTag("tag", { expire: 0 })`.
+	 * That's why the default is `false` for Next 16+ which uses SWR by default.
+	 *
+	 * NOTE: Use the default value unless you know what you are doing. It is set to:
+	 * - Next <16:
+	 *    `true` if the auto cache purging is enabled, `false` otherwise.
+	 * - Next >= 16:
+	 *   `false`
 	 */
 	bypassTagCacheOnCacheHit?: boolean;
 };
@@ -78,17 +91,33 @@ class RegionalCache implements IncrementalCache {
 		private opts: Options
 	) {
 		this.name = this.store.name;
-		// `shouldLazilyUpdateOnCacheHit` is not needed when cache purge is enabled.
-		this.opts.shouldLazilyUpdateOnCacheHit ??= this.opts.mode === "long-lived" && !isPurgeCacheEnabled();
-	}
 
-	get #bypassTagCacheOnCacheHit(): boolean {
-		if (this.opts.bypassTagCacheOnCacheHit !== undefined) {
-			return this.opts.bypassTagCacheOnCacheHit;
+		// `globalThis.nextVersion` is only defined at runtime but not when the Open Next build runs.
+		// The options do no matter at build time so we can skip setting them.
+		const { nextVersion } = globalThis;
+		if (nextVersion) {
+			if (compareSemver(nextVersion, "<", "16")) {
+				// Next < 16
+				this.opts.shouldLazilyUpdateOnCacheHit ??= this.opts.mode === "long-lived" && !isPurgeCacheEnabled();
+				this.opts.bypassTagCacheOnCacheHit ??= isPurgeCacheEnabled();
+			} else {
+				// Next >= 16
+				this.opts.bypassTagCacheOnCacheHit ??= false;
+				if (this.opts.bypassTagCacheOnCacheHit) {
+					debugCache(
+						"RegionalCache",
+						`bypassTagCacheOnCacheHit is not recommended for Next 16+ as it is not compatible with SWR tags. Make sure to always use \`revalidateTag\` with \`{ expire: 0 }\` if you want to bypass the tag cache.`
+					);
+				}
+				this.opts.shouldLazilyUpdateOnCacheHit ??= !this.opts.bypassTagCacheOnCacheHit;
+				if (this.opts.shouldLazilyUpdateOnCacheHit !== this.opts.bypassTagCacheOnCacheHit) {
+					debugCache(
+						"RegionalCache",
+						`\`shouldLazilyUpdateOnCacheHit\` and \`bypassTagCacheOnCacheHit\` are mutually exclusive for Next 16+.`
+					);
+				}
+			}
 		}
-
-		// When `bypassTagCacheOnCacheHit` is not set, we default to whether the automatic cache purging is enabled or not
-		return isPurgeCacheEnabled();
 	}
 
 	async get<CacheType extends CacheEntryType = "cache">(
@@ -123,7 +152,7 @@ class RegionalCache implements IncrementalCache {
 
 				return {
 					...responseJson,
-					shouldBypassTagCache: this.#bypassTagCacheOnCacheHit,
+					shouldBypassTagCache: this.opts.bypassTagCacheOnCacheHit,
 				};
 			}
 

packages/cloudflare/src/api/overrides/tag-cache/d1-next-tag-cache.spec.ts

@@ -395,6 +395,17 @@ describe("D1NextModeTagCache", () => {
 			expect(result).toBe(false);
 		});
 
+		it("should return false when revalidatedAt <= lastModified even if stale > lastModified", async () => {
+			const now = 2000;
+			vi.spyOn(Date, "now").mockReturnValue(now);
+			// revalidatedAt=500 <= lastModified=1000, so the stale window is from a previous ISR cycle
+			mockRaw.mockResolvedValue([[`${FALLBACK_BUILD_ID}/tag1`, 500, 1500, null]]);
+
+			const result = await tagCache.isStale(["tag1"], 1000);
+
+			expect(result).toBe(false);
+		});
+
 		it("should return false when expire <= now (tag expired)", async () => {
 			const now = 2000;
 			vi.spyOn(Date, "now").mockReturnValue(now);

packages/cloudflare/src/api/overrides/tag-cache/d1-next-tag-cache.ts

@@ -105,8 +105,14 @@ export class D1NextModeTagCache implements NextModeTagCache {
 
 			const isStale = [...result.values()].some((v) => {
 				if (v == null) return false;
-				const { stale, expire } = v;
-				if (stale == null || stale <= (lastModified ?? now)) return false;
+				const { revalidatedAt, stale, expire } = v;
+				// A tag is stale when both its stale and revalidatedAt timestamps are newer than the page.
+				// revalidatedAt > lastModified ensures the revalidation that set this stale window happened
+				// after the page was generated, preventing a stale signal from a previous ISR cycle.
+				const lastModifiedOrNow = lastModified ?? now;
+				const isInStaleWindow =
+					stale != null && revalidatedAt > lastModifiedOrNow && lastModifiedOrNow <= stale;
+				if (!isInStaleWindow) return false;
 				return expire == null || expire > now;
 			});
 

packages/cloudflare/src/api/overrides/tag-cache/do-sharded-tag-cache.spec.ts

@@ -349,6 +349,14 @@ describe("DOShardedTagCache", () => {
 			expect(await cache.isStale(["tag1"], 200)).toBe(false);
 		});
 
+		it("should return false when revalidatedAt <= lastModified even if stale > lastModified", async () => {
+			const cache = shardedDOTagCache();
+			cache.getFromRegionalCache = vi.fn().mockResolvedValueOnce([]);
+			// revalidatedAt=100 <= lastModified=200, so the stale window is from a previous ISR cycle
+			getTagDataMock.mockResolvedValueOnce({ tag1: { revalidatedAt: 100, stale: 300, expire: null } });
+			expect(await cache.isStale(["tag1"], 200)).toBe(false);
+		});
+
 		it("should return from regional cache if available", async () => {
 			vi.useFakeTimers();
 			vi.setSystemTime(500);

packages/cloudflare/src/api/overrides/tag-cache/do-sharded-tag-cache.ts

@@ -191,8 +191,14 @@ class ShardedDOTagCache implements NextModeTagCache {
 			const tagData = await this.#resolveTagData(tags);
 			const result = [...tagData.values()].some((data) => {
 				if (data == null) return false;
-				const { stale, expire } = data;
-				if (stale == null || stale <= (lastModified ?? now)) return false;
+				const { revalidatedAt, stale, expire } = data;
+				// A tag is stale when both its stale timestamp and its revalidatedAt are newer than the page.
+				// revalidatedAt > lastModified ensures the revalidation that set this stale window happened
+				// after the page was generated, preventing a stale signal from a previous ISR cycle.
+				const lastModifiedOrNow = lastModified ?? now;
+				const isInStaleWindow =
+					stale != null && revalidatedAt > lastModifiedOrNow && lastModifiedOrNow <= stale;
+				if (!isInStaleWindow) return false;
 				return expire == null || expire > now;
 			});
 			debugCache("ShardedDOTagCache", `isStale tags=${tags} at=${lastModified} -> ${result}`);

packages/cloudflare/src/api/overrides/tag-cache/kv-next-tag-cache.spec.ts

@@ -344,6 +344,19 @@ describe("KVNextModeTagCache", () => {
 			expect(result).toBe(false);
 		});
 
+		it("should return false when revalidatedAt <= lastModified even if stale > lastModified", async () => {
+			const now = 2000;
+			vi.spyOn(Date, "now").mockReturnValue(now);
+			// revalidatedAt=500 <= lastModified=1000, so the stale window is from a previous ISR cycle
+			mockGet.mockResolvedValue(
+				new Map([[`${FALLBACK_BUILD_ID}/tag1`, { revalidatedAt: 500, stale: 1500, expire: null }]])
+			);
+
+			const result = await tagCache.isStale(["tag1"], 1000);
+
+			expect(result).toBe(false);
+		});
+
 		it("should return false when expire <= now (tag expired)", async () => {
 			const now = 2000;
 			vi.spyOn(Date, "now").mockReturnValue(now);

packages/cloudflare/src/api/overrides/tag-cache/kv-next-tag-cache.ts

@@ -180,8 +180,14 @@ export class KVNextModeTagCache implements NextModeTagCache {
 			const isStale = [...result.values()].some((v) => {
 				if (v == null) return false;
 				const stale = getStale(v);
-				if (stale == null || stale <= (lastModified ?? now)) return false;
 				const expire = getExpire(v);
+				// A tag is stale when both its stale timestamp and its revalidatedAt are newer than the page.
+				// revalidatedAt > lastModified ensures the revalidation that set this stale window happened
+				// after the page was generated, preventing a stale signal from a previous ISR cycle.
+				const lastModifiedOrNow = lastModified ?? now;
+				const isInStaleWindow =
+					stale != null && getRevalidatedAt(v) > lastModifiedOrNow && lastModifiedOrNow <= stale;
+				if (!isInStaleWindow) return false;
 				return expire == null || expire > now;
 			});