diff --git a/packages/gitbook/openNext/incrementalCache/incrementalCache.test.ts b/packages/gitbook/openNext/incrementalCache/incrementalCache.test.ts new file mode 100644 index 000000000..24803eead --- /dev/null +++ b/packages/gitbook/openNext/incrementalCache/incrementalCache.test.ts @@ -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: '', rsc: 'rsc', meta: { status: 404 } }) + ) + ).toBeNull(); + }); + + it('returns the entry for a valid app entry', () => { + const entry = withValue({ + type: 'app', + 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: '', rsc: 'rsc', meta: { status: 404 } }) + ) + ); + expect(await cache.get('key')).toBeNull(); + }); + + it('passes through a valid entry', async () => { + const entry = withValue({ + type: 'app', + html: '', + rsc: 'rsc', + meta: { status: 200 }, + }); + const cache = with404Guard(new FakeCache(entry)); + expect(await cache.get('key')).toBe(entry); + }); +}); diff --git a/packages/gitbook/openNext/incrementalCache/incrementalCache.ts b/packages/gitbook/openNext/incrementalCache/incrementalCache.ts index 550c355ea..ca30324f4 100644 --- a/packages/gitbook/openNext/incrementalCache/incrementalCache.ts +++ b/packages/gitbook/openNext/incrementalCache/incrementalCache.ts @@ -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( + cacheEntry: WithLastModified> | null +): WithLastModified> | 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( - cacheEntry: WithLastModified> | null - ): WithLastModified> | null { - if (!cacheEntry?.value) return null; - if ('meta' in cacheEntry.value && cacheEntry.value.meta?.status === 404) { - return null; - } - return cacheEntry; - } - async set( key: string, value: CacheValue, diff --git a/packages/gitbook/openNext/incrementalCache/middleware.ts b/packages/gitbook/openNext/incrementalCache/middleware.ts index 2d116ec2b..73c50cba0 100644 --- a/packages/gitbook/openNext/incrementalCache/middleware.ts +++ b/packages/gitbook/openNext/incrementalCache/middleware.ts @@ -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, + }) +); diff --git a/packages/gitbook/openNext/incrementalCache/server.ts b/packages/gitbook/openNext/incrementalCache/server.ts index 359920de7..1a814df79 100644 --- a/packages/gitbook/openNext/incrementalCache/server.ts +++ b/packages/gitbook/openNext/incrementalCache/server.ts @@ -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, + }) +);