Run the cached-404 guard on regional cache hits

2026-09-11 21:39:22 +00:00 · 2026-08-28 12:03:04 +02:00
parent 35be3835f3
commit e1ac045a07
4 changed files with 129 additions and 34 deletions
@@ -0,0 +1,69 @@
+import type { IncrementalCache } from '@opennextjs/aws/types/overrides.js';
+import { describe, expect, it } from 'bun:test';
+
+import { returnNullOn404, with404Guard } from './incrementalCache';
+
+const withValue = (value: any) => ({ value, lastModified: 123 });
+
+describe('returnNullOn404', () => {
+    it('returns null for a null entry', () => {
+        expect(returnNullOn404(null)).toBeNull();
+    });
+
+    it('returns null for a 404 entry', () => {
+        expect(
+            returnNullOn404(
+                withValue({ type: 'app', html: '<html></html>', rsc: 'rsc', meta: { status: 404 } })
+            )
+        ).toBeNull();
+    });
+
+    it('returns the entry for a valid app entry', () => {
+        const entry = withValue({
+            type: 'app',
+            html: '<html></html>',
+            rsc: 'rsc',
+            meta: { status: 200 },
+        });
+        expect(returnNullOn404(entry)).toBe(entry);
+    });
+
+    it('returns the entry for a redirect entry', () => {
+        const entry = withValue({ type: 'redirect', meta: { status: 308 } });
+        expect(returnNullOn404(entry)).toBe(entry);
+    });
+});
+
+describe('with404Guard', () => {
+    // Minimal class-based fake so the guard is exercised against prototype methods,
+    // like the RegionalCache it wraps in production.
+    class FakeCache implements IncrementalCache {
+        name = 'FakeCache';
+        constructor(private entry: any) {}
+        async get(_key: string) {
+            return this.entry;
+        }
+        async set() {}
+        async delete() {}
+    }
+
+    it('filters out a 404 entry returned by the wrapped cache', async () => {
+        const cache = with404Guard(
+            new FakeCache(
+                withValue({ type: 'app', html: '<html></html>', rsc: 'rsc', meta: { status: 404 } })
+            )
+        );
+        expect(await cache.get('key')).toBeNull();
+    });
+
+    it('passes through a valid entry', async () => {
+        const entry = withValue({
+            type: 'app',
+            html: '<html></html>',
+            rsc: 'rsc',
+            meta: { status: 200 },
+        });
+        const cache = with404Guard(new FakeCache(entry));
+        expect(await cache.get('key')).toBe(entry);
+    });
+});
@@ -15,6 +15,36 @@ export type KeyOptions = {
    cacheType?: CacheEntryType;
 };

+//TODO: This is a workaround to handle 404 responses in the cache.
+// It should be handled by OpenNext cache interception directly. This should be removed once OpenNext cache interception is fixed.
+// Cache interception serves entries with the rewrite status code (always 200), so a cached 404
+// would otherwise be replayed as a 200 (RND-12656).
+export function returnNullOn404<CacheType extends CacheEntryType = 'cache'>(
+    cacheEntry: WithLastModified<CacheValue<CacheType>> | null
+): WithLastModified<CacheValue<CacheType>> | null {
+    if (!cacheEntry?.value) return null;
+    if ('meta' in cacheEntry.value && cacheEntry.value.meta?.status === 404) {
+        return null;
+    }
+    return cacheEntry;
+}
+
+/**
+ * Apply `returnNullOn404` on top of a cache. Needed on top of the regional cache: its
+ * Cache API entries are written at `set` time and served without going through
+ * `GitbookIncrementalCache.get`, which would bypass the guard (RND-12656).
+ */
+export function with404Guard(cache: IncrementalCache): IncrementalCache {
+    return {
+        name: cache.name,
+        async get(key, cacheType) {
+            return returnNullOn404(await cache.get(key, cacheType));
+        },
+        set: cache.set.bind(cache),
+        delete: cache.delete.bind(cache),
+    };
+}
+
 /**
 *
 * It is very similar to the `R2IncrementalCache` in the `@opennextjs/cloudflare` package, but it has an additional
@@ -45,7 +75,7 @@ export class GitbookIncrementalCache implements IncrementalCache {

            if (!json) return null;

-            return this.returnNullOn404({
+            return returnNullOn404({
                value: json,
                lastModified,
            });
@@ -55,18 +85,6 @@ export class GitbookIncrementalCache implements IncrementalCache {
        }
    }

-    //TODO: This is a workaround to handle 404 responses in the cache.
-    // It should be handled by OpenNext cache interception directly. This should be removed once OpenNext cache interception is fixed.
-    returnNullOn404<CacheType extends CacheEntryType = 'cache'>(
-        cacheEntry: WithLastModified<CacheValue<CacheType>> | null
-    ): WithLastModified<CacheValue<CacheType>> | null {
-        if (!cacheEntry?.value) return null;
-        if ('meta' in cacheEntry.value && cacheEntry.value.meta?.status === 404) {
-            return null;
-        }
-        return cacheEntry;
-    }
-
    async set<CacheType extends CacheEntryType = 'cache'>(
        key: string,
        value: CacheValue<CacheType>,
@@ -1,13 +1,17 @@
 import { withRegionalCache } from '@opennextjs/cloudflare/overrides/incremental-cache/regional-cache';

-import { GitbookIncrementalCache } from './incrementalCache';
+import { GitbookIncrementalCache, with404Guard } from './incrementalCache';

-export default withRegionalCache(new GitbookIncrementalCache(), {
-    mode: 'long-lived',
-    // We can do it because we use our own logic to invalidate the cache
-    bypassTagCacheOnCacheHit: true,
-    //TODO: bump it again once I figured out the race condition
-    defaultLongLivedTtlSec: 5 * 60, // 5 minutes
-    // We don't want to update the cache entry on every cache hit
-    shouldLazilyUpdateOnCacheHit: false,
-});
+// The guard wraps the regional cache: its entries are written at set time and served
+// without going through GitbookIncrementalCache.get (RND-12656).
+export default with404Guard(
+    withRegionalCache(new GitbookIncrementalCache(), {
+        mode: 'long-lived',
+        // We can do it because we use our own logic to invalidate the cache
+        bypassTagCacheOnCacheHit: true,
+        //TODO: bump it again once I figured out the race condition
+        defaultLongLivedTtlSec: 5 * 60, // 5 minutes
+        // We don't want to update the cache entry on every cache hit
+        shouldLazilyUpdateOnCacheHit: false,
+    })
+);
@@ -1,16 +1,20 @@
 import { withRegionalCache } from '@opennextjs/cloudflare/overrides/incremental-cache/regional-cache';

-import { GitbookIncrementalCache } from './incrementalCache';
+import { GitbookIncrementalCache, with404Guard } from './incrementalCache';

 // We cannot have regional cache only in the middleware, otherwise it will override things on cache miss
 // and cause race conditions. This will be fixed in a future release of OpenNext
-export default withRegionalCache(new GitbookIncrementalCache(), {
-    mode: 'long-lived',
-    // Because of a race condition, the middleware may have populated the cache entry before `cache.match` had time to run on the server.
-    // TODO: We should bypass the incremental cache entirely when the interceptor has caught the request. Should be done in OpenNext.
-    bypassTagCacheOnCacheHit: false,
-    //TODO: remove, reducing cache ttl of regional cache to help debugging
-    defaultLongLivedTtlSec: 5 * 60 /* 5 minutes */,
-    // We don't want to update the cache entry on every cache hit
-    shouldLazilyUpdateOnCacheHit: false,
-});
+// The guard wraps the regional cache: its entries are written at set time and served
+// without going through GitbookIncrementalCache.get (RND-12656).
+export default with404Guard(
+    withRegionalCache(new GitbookIncrementalCache(), {
+        mode: 'long-lived',
+        // Because of a race condition, the middleware may have populated the cache entry before `cache.match` had time to run on the server.
+        // TODO: We should bypass the incremental cache entirely when the interceptor has caught the request. Should be done in OpenNext.
+        bypassTagCacheOnCacheHit: false,
+        //TODO: remove, reducing cache ttl of regional cache to help debugging
+        defaultLongLivedTtlSec: 5 * 60 /* 5 minutes */,
+        // We don't want to update the cache entry on every cache hit
+        shouldLazilyUpdateOnCacheHit: false,
+    })
+);