From 2ed9edeaad191ee9ea6fb5783fb94734440e38fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=B3=96=E9=A5=BC?= Date: Tue, 16 Jun 2026 23:48:30 +0800 Subject: [PATCH 1/2] fix: cache revalidation, UPDATING status, and x-cache-key encoding Fix 304 background revalidation persistence so subsequent requests return fresh HIT, add UPDATING for stale-while-revalidate, return 304 from match for conditional requests, and percent-encode x-cache-key values. --- .changeset/calm-rivers-flow.md | 13 ++ README.md | 18 ++- src/cache.test.ts | 210 ++++++++++++++++++++------- src/cache.ts | 249 +++++++++++++++++++++++++++------ src/constants.ts | 5 +- src/fetch.test.ts | 108 +++++++++++++- src/fetch.ts | 12 +- src/index.ts | 1 + src/types.ts | 11 +- src/utils/response.test.ts | 141 ++++++++++++++----- src/utils/response.ts | 27 ++++ 11 files changed, 657 insertions(+), 138 deletions(-) create mode 100644 .changeset/calm-rivers-flow.md diff --git a/.changeset/calm-rivers-flow.md b/.changeset/calm-rivers-flow.md new file mode 100644 index 0000000..b4d170e --- /dev/null +++ b/.changeset/calm-rivers-flow.md @@ -0,0 +1,13 @@ +--- +'@web-widget/shared-cache': minor +--- + +Fix 304 revalidation persistence so background revalidation with `304 Not Modified` stores the revalidated policy instead of rebuilding from stale response headers (restores fresh `HIT` on subsequent requests). + +Add `UPDATING` cache status for `stale-while-revalidate` (aligns with Cloudflare CDN semantics; `STALE` is reserved for `stale-if-error`). + +Return `304 Not Modified` from `Cache.match()` when conditional request headers (`If-None-Match`, `If-Modified-Since`) match a fresh cached entry. + +Percent-encode `x-cache-key` header values so cache keys with control characters or non-Latin-1 Unicode are valid HTTP header field values. + +Strip persisted `Age` from stored policy metadata on read for compatibility with entries written before the revalidation fix. diff --git a/README.md b/README.md index da26b0e..11b44e8 100644 --- a/README.md +++ b/README.md @@ -768,19 +768,20 @@ SharedCache provides comprehensive monitoring through the `x-cache-status` heade | Status | Description | When It Occurs | | ----------------- | ----------------------------------------------- | ------------------------------------------------------------ | -| **`HIT`** | Response served from cache | The requested resource was found in cache and is still fresh | +| **`HIT`** | Response served from cache | Fresh cache hit, or conditional `304` from `Cache.match()` | | **`MISS`** | Response fetched from origin | The requested resource was not found in cache | | **`EXPIRED`** | Cached response expired, fresh response fetched | The cached response exceeded its TTL | -| **`STALE`** | Stale response served | Served due to stale-while-revalidate or stale-if-error | +| **`UPDATING`** | Stale response served during background revalidate | `stale-while-revalidate` via `createFetch` | +| **`STALE`** | Stale response served when origin is unreachable | `stale-if-error` or revalidation failure | | **`BYPASS`** | Cache bypassed | Bypassed due to cache control directives like `no-store` | -| **`REVALIDATED`** | Cached response revalidated | Response validated with origin (304 Not Modified) | +| **`REVALIDATED`** | Cached response revalidated with origin | Synchronous revalidation; origin returned 304 Not Modified | | **`DYNAMIC`** | Response cannot be cached | Cannot be cached due to HTTP method or status code | ### Cache Status Header Details The `x-cache-status` header is automatically added to all responses: -- **Header Values**: `HIT`, `MISS`, `EXPIRED`, `STALE`, `BYPASS`, `REVALIDATED`, `DYNAMIC` +- **Header Values**: `HIT`, `MISS`, `EXPIRED`, `UPDATING`, `STALE`, `BYPASS`, `REVALIDATED`, `DYNAMIC` - **Always Present**: The header is always added for monitoring and debugging - **Non-Standard**: Custom header for debugging - should not be used for application logic @@ -837,7 +838,7 @@ const cache = new SharedCache(storage, { - **Example Output**: ``` SharedCache: Cache hit { url: 'https://api.com/data', cacheKey: 'api:data', cacheStatus: 'HIT' } - SharedCache: Serving stale response - Revalidating in background { url: 'https://api.com/data', cacheKey: 'api:data', cacheStatus: 'STALE' } + SharedCache: Serving stale response - Revalidating in background { url: 'https://api.com/data', cacheKey: 'api:data', cacheStatus: 'UPDATING' } ``` #### WARN @@ -1170,6 +1171,7 @@ type SharedCacheStatus = | 'HIT' | 'MISS' | 'EXPIRED' + | 'UPDATING' | 'STALE' | 'BYPASS' | 'REVALIDATED' @@ -1192,7 +1194,7 @@ SharedCache demonstrates **exceptional HTTP standards compliance**, fully adheri - **HTTP Method Support**: Standards-compliant caching for GET/HEAD methods with correct rejection of non-cacheable methods - **Status Code Handling**: Appropriate caching behavior for 200, 301, 404 responses and proper rejection of 5xx errors - **Vary Header Processing**: Full content negotiation support with intelligent cache key generation -- **Conditional Requests**: Complete ETag and Last-Modified validation with 304 Not Modified handling +- **Conditional Requests**: `Cache.match()` returns `304` for matching `If-None-Match` / `If-Modified-Since` on fresh entries; `createFetch` revalidates with the origin and handles `304 Not Modified` ### ✅ RFC 5861 Extensions @@ -1224,6 +1226,8 @@ interface Cache { - **❌ Convenience Methods**: `add()`, `addAll()` - Use `put()` instead - **❌ Enumeration Methods**: `keys()`, `matchAll()` - Not available in server environments +**`createFetch` vs `Cache`:** `createFetch` adds SWR, origin revalidation, and status headers. Bare `cache.match()` / `cache.put()` are storage-style APIs (expired entries return `undefined` without `createFetch`). + **Options Parameter Differences:** SharedCache's `CacheQueryOptions` interface differs from the standard Web Cache API: @@ -1315,7 +1319,7 @@ When using SharedCache with meta-frameworks, you can develop with a consistent c ### Q: What's the value of `stale-while-revalidate` and `stale-if-error` directives? -**A:** These RFC 5861 extensions provide significant performance and reliability benefits: +**A:** Performance and reliability extensions (via `createFetch`): - **stale-while-revalidate**: Serves cached content immediately while updating in background, providing zero-latency responses - **stale-if-error**: Serves cached content when origin servers fail, improving uptime and user experience diff --git a/src/cache.test.ts b/src/cache.test.ts index 5a005f1..764318c 100644 --- a/src/cache.test.ts +++ b/src/cache.test.ts @@ -5,7 +5,7 @@ import type { Logger } from './utils/logger'; import { CACHE_STATUS_HEADER_NAME, HIT, - STALE, + UPDATING, REVALIDATED, EXPIRED, } from './constants'; @@ -293,6 +293,99 @@ describe('SharedCache', () => { expect(matched).toBeDefined(); }); + it('should return 304 when If-None-Match matches a fresh cached ETag', async () => { + const request = new Request('https://example.com/conditional-etag'); + await cache.put( + request, + createTestResponse('cached data', 200, { + 'cache-control': 'max-age=300', + etag: '"v1"', + }) + ); + + const conditional = new Request('https://example.com/conditional-etag', { + headers: { 'if-none-match': '"v1"' }, + }); + const matched = await cache.match(conditional); + + expect(matched).toBeDefined(); + expect(matched!.status).toBe(304); + expect(await matched!.text()).toBe(''); + expect(matched!.headers.get('etag')).toBe('"v1"'); + expect(matched!.headers.get(CACHE_STATUS_HEADER_NAME)).toBe(HIT); + }); + + it('should return 200 when If-None-Match does not match the cached ETag', async () => { + const request = new Request('https://example.com/conditional-etag-miss'); + await cache.put( + request, + createTestResponse('cached data', 200, { + 'cache-control': 'max-age=300', + etag: '"v1"', + }) + ); + + const conditional = new Request( + 'https://example.com/conditional-etag-miss', + { + headers: { 'if-none-match': '"v2"' }, + } + ); + const matched = await cache.match(conditional); + + expect(matched).toBeDefined(); + expect(matched!.status).toBe(200); + expect(await matched!.text()).toBe('cached data'); + expect(matched!.headers.get(CACHE_STATUS_HEADER_NAME)).toBe(HIT); + }); + + it('should return 304 when If-Modified-Since is not earlier than Last-Modified', async () => { + const lastModified = new Date('Wed, 21 Oct 2015 07:28:00 GMT'); + const request = new Request('https://example.com/conditional-ims'); + await cache.put( + request, + createTestResponse('cached data', 200, { + 'cache-control': 'max-age=300', + 'last-modified': lastModified.toUTCString(), + }) + ); + + const conditional = new Request('https://example.com/conditional-ims', { + headers: { + 'if-modified-since': lastModified.toUTCString(), + }, + }); + const matched = await cache.match(conditional); + + expect(matched).toBeDefined(); + expect(matched!.status).toBe(304); + expect(await matched!.text()).toBe(''); + expect(matched!.headers.get(CACHE_STATUS_HEADER_NAME)).toBe(HIT); + }); + + it('should return undefined for expired entries even with matching If-None-Match', async () => { + const request = new Request('https://example.com/conditional-expired'); + await cache.put( + request, + createTestResponse('cached data', 200, { + 'cache-control': 'max-age=1', + etag: '"v1"', + }) + ); + + await new Promise((resolve) => setTimeout(resolve, 1100)); + + const conditional = new Request( + 'https://example.com/conditional-expired', + { + headers: { 'if-none-match': '"v1"' }, + } + ); + const matched = await cache.match(conditional); + + expect(matched).toBeUndefined(); + }); + it('should return undefined for non-existent entries', async () => { const matched = await cache.match('https://example.com/nonexistent'); expect(matched).toBeUndefined(); @@ -327,79 +420,100 @@ describe('SharedCache', () => { expect(matched).toBeUndefined(); }); - it('should return STALE status when serving stale content while revalidating', async () => { - // Store response with stale-while-revalidate + it('should return UPDATING and schedule background revalidation when stale', async () => { const request = new Request('https://example.com/stale-while-revalidate'); const response = createTestResponse('stale data', 200, { 'cache-control': 'max-age=1, stale-while-revalidate=300', }); await cache.put(request, response); - // Wait for it to become stale await new Promise((resolve) => setTimeout(resolve, 1100)); - // Mock fetch for background revalidation - const mockFetch = async () => { - return createTestResponse('fresh data', 200, { + const mockFetch = async () => + createTestResponse('fresh data', 200, { 'cache-control': 'max-age=300', }); - }; let backgroundPromise: Promise | null = null; - const mockWaitUntil = (promise: Promise) => { - backgroundPromise = promise; - }; - - const mockEvent = { - waitUntil: mockWaitUntil, - } as ExtendableEvent; - const matched = await cache.match(request, { _fetch: mockFetch, - _event: mockEvent, + _event: { + waitUntil: (promise: Promise) => { + backgroundPromise = promise; + }, + } as ExtendableEvent, }); expect(matched).toBeDefined(); expect(await matched!.text()).toBe('stale data'); - expect(matched!.headers.get(CACHE_STATUS_HEADER_NAME)).toBe(STALE); + expect(matched!.headers.get(CACHE_STATUS_HEADER_NAME)).toBe(UPDATING); expect(backgroundPromise).not.toBeNull(); }); - it('should support _event option for background operations', async () => { - // Store response with stale-while-revalidate - const request = new Request('https://example.com/stale-with-event'); - const response = createTestResponse('stale data', 200, { - 'cache-control': 'max-age=1, stale-while-revalidate=300', - }); - await cache.put(request, response); - - // Wait for it to become stale - await new Promise((resolve) => setTimeout(resolve, 1100)); - - // Mock fetch for background revalidation - const mockFetch = async () => { - return createTestResponse('fresh data', 200, { - 'cache-control': 'max-age=300', + describe('revalidation persistence', () => { + const mock304Revalidate = async () => + new Response(null, { + status: 304, + headers: { etag: '"v1"' }, }); - }; - // Mock ExtendableEvent - let backgroundPromise: Promise | null = null; - const mockEvent = { - waitUntil: (promise: Promise) => { - backgroundPromise = promise; - }, - } as ExtendableEvent; + async function drainBackgroundRevalidation(request: Request) { + let revalidatePromise!: Promise; + await cache.match(request, { + _fetch: mock304Revalidate, + _event: { + waitUntil: (promise: Promise) => { + revalidatePromise = promise; + }, + } as ExtendableEvent, + }); + await revalidatePromise; + } - const matched = await cache.match(request, { - _fetch: mockFetch, - _event: mockEvent, + it('should persist fresh policy after background 304 revalidation', async () => { + const request = new Request('https://example.com/revalidate-304'); + await cache.put( + request, + createTestResponse('page body', 200, { + 'cache-control': 's-maxage=1, stale-while-revalidate=300', + etag: '"v1"', + }) + ); + + await new Promise((resolve) => setTimeout(resolve, 1100)); + await drainBackgroundRevalidation(request); + + const next = await cache.match(request, { _fetch: mock304Revalidate }); + expect(next).toBeDefined(); + expect(next!.headers.get(CACHE_STATUS_HEADER_NAME)).toBe(HIT); + expect(next!.headers.get('age')).toBe('0'); }); - expect(matched).toBeDefined(); - expect(await matched!.text()).toBe('stale data'); - expect(matched!.headers.get(CACHE_STATUS_HEADER_NAME)).toBe(STALE); - expect(backgroundPromise).not.toBeNull(); + it('should ignore legacy Age metadata in stored policy', async () => { + const request = new Request('https://example.com/legacy-age'); + await cache.put( + request, + createTestResponse('page body', 200, { + 'cache-control': 's-maxage=300', + etag: '"v1"', + }) + ); + + const cacheKey = await cache.getCacheKey(request); + const stored = (await storage.get(cacheKey)) as CacheItem; + await storage.set(cacheKey, { + ...stored, + policy: { + ...stored.policy, + resh: { ...stored.policy.resh, age: '5' }, + }, + }); + + const matched = await cache.match(request); + expect(matched).toBeDefined(); + expect(matched!.headers.get(CACHE_STATUS_HEADER_NAME)).toBe(HIT); + expect(matched!.headers.get('age')).toBe('0'); + }); }); it('should verify REVALIDATED cache status constant exists', () => { diff --git a/src/cache.ts b/src/cache.ts index 43f4c31..f865cd4 100644 --- a/src/cache.ts +++ b/src/cache.ts @@ -10,6 +10,7 @@ import type { SharedCacheRequestInfo, SharedCacheStatus, } from './types'; +import type { CachePolicyObject } from '@web-widget/http-cache-semantics'; import { createLogger, StructuredLogger } from './utils/logger'; import { createCacheKeyGenerator, @@ -22,7 +23,7 @@ import { EXPIRED, HIT, REVALIDATED, - STALE, + UPDATING, } from './constants'; import { CachePolicy } from './utils/cache-semantics'; @@ -240,31 +241,32 @@ export class SharedCache implements WebCache { }); const fetch = options?._fetch; - const policy = CachePolicy.fromObject(cacheItem.policy); + const policyObject = sanitizeStoredPolicy(cacheItem.policy); + const policy = CachePolicy.fromObject(policyObject); const { body, status, statusText } = cacheItem.response; - const headers = policy.responseHeaders(); + const responseHeaders = policy.responseHeaders(); const stale = policy.stale(); - const response = new Response(body, { - status, - statusText, - headers, - }); - - // Check if the cached response satisfies the request without revalidation - if ( + const needsRevalidation = !policy.satisfiesWithoutRevalidation(r, { ignoreRequestCacheControl: options?._ignoreRequestCacheControl, ignoreMethod: true, ignoreSearch: true, ignoreVary: true, - }) || - stale - ) { + }) || stale; + + if (needsRevalidation) { + const response = new Response(body, { + status, + statusText, + headers: responseHeaders, + }); + if (!fetch) { return; - } else if (stale && policy.useStaleWhileRevalidate()) { - // Serve stale response while revalidating in background + } + + if (stale && policy.useStaleWhileRevalidate()) { const event = options?._event; const waitUntil = event?.waitUntil.bind(event) ?? @@ -286,38 +288,58 @@ export class SharedCache implements WebCache { { response: response.clone(), policy, + cachedBody: body, }, cacheKey, fetch, options ) ); - this.#setCacheStatus(response, STALE); + this.#setCacheStatus(response, UPDATING); this.#structuredLogger.info( 'Serving stale response', { url: r.url, cacheKey, - cacheStatus: 'STALE', + cacheStatus: 'UPDATING', }, 'Revalidating in background' ); return response; - } else { - // Revalidate synchronously - return this.#revalidate( - r, - { - response, - policy, - }, - cacheKey, - fetch, - options - ); } + + return this.#revalidate( + r, + { + response, + policy, + cachedBody: body, + }, + cacheKey, + fetch, + options + ); } + if ( + hasConditionalRequestHeaders(r) && + satisfiesConditionalRequest(r, responseHeaders) + ) { + const notModified = createNotModifiedResponse(responseHeaders); + this.#setCacheStatus(notModified, HIT); + this.#structuredLogger.info('Cache hit', { + url: r.url, + cacheKey, + cacheStatus: 'HIT', + }); + return notModified; + } + + const response = new Response(body, { + status, + statusText, + headers: responseHeaders, + }); this.#setCacheStatus(response, HIT); this.#structuredLogger.info('Cache hit', { url: r.url, @@ -573,18 +595,37 @@ export class SharedCache implements WebCache { revalidationResponse ); - // Use new response if modified, otherwise use cached response - const response = modified - ? revalidationResponse - : resolveCacheItem.response; + let responseBody: string; + let responseStatus: number; + let responseStatusText: string; - // Store the updated response/policy in cache - await this.#putWithCustomCacheKey(request, response, cacheKey); + if (modified) { + responseBody = await revalidationResponse.clone().text(); + responseStatus = revalidationResponse.status; + responseStatusText = revalidationResponse.statusText; + } else { + responseBody = + resolveCacheItem.cachedBody ?? + (await resolveCacheItem.response.clone().text()); + responseStatus = resolveCacheItem.response.status; + responseStatusText = resolveCacheItem.response.statusText; + } - // Create response with updated headers from revalidated policy - const clonedResponse = new Response(response.body, { - status: response.status, - statusText: response.statusText, + // Persist the revalidated policy — do not rebuild policy from stale response headers + await this.#storeRevalidatedCacheItem( + request, + revalidatedPolicy, + { + body: responseBody, + status: responseStatus, + statusText: responseStatusText, + }, + cacheKey + ); + + const clonedResponse = new Response(responseBody, { + status: responseStatus, + statusText: responseStatusText, headers: revalidatedPolicy.responseHeaders(), }); @@ -616,6 +657,54 @@ export class SharedCache implements WebCache { return clonedResponse; } + /** + * Stores a revalidated cache entry using the policy from revalidatedPolicy(). + * Avoids rebuilding CachePolicy from response headers, which would persist stale Age values. + */ + async #storeRevalidatedCacheItem( + request: Request, + revalidatedPolicy: CachePolicy, + response: { body: string; status: number; statusText: string }, + cacheKey: string + ): Promise { + const ttl = revalidatedPolicy.timeToLive(); + const storable = revalidatedPolicy.storable(); + + if (!storable || ttl <= 0) { + this.#structuredLogger.debug( + 'Revalidated response not cacheable', + { + url: request.url, + storable, + ttl, + status: response.status, + }, + storable ? 'TTL is zero/negative' : 'Policy indicates not storable' + ); + return; + } + + const cacheItem: CacheItem = { + policy: revalidatedPolicy.toObject(), + response, + }; + + const responseForVary = new Response(response.body, { + status: response.status, + statusText: response.statusText, + headers: revalidatedPolicy.responseHeaders(), + }); + + await setCacheItem( + this.#storage, + cacheKey, + cacheItem, + ttl, + request as SharedCacheRequest, + responseForVary + ); + } + /** * Sets the cache status header on a response. * Used to indicate the cache result to clients via the X-Shared-Cache header. @@ -672,6 +761,86 @@ export class SharedCache implements WebCache { } } +/** + * Removes computed Age from stored policy metadata. + * Age is added at response time via responseHeaders(), not persisted in cache policy. + */ +function sanitizeStoredPolicy(policy: CachePolicyObject): CachePolicyObject { + if (!policy.resh || policy.resh.age === undefined) { + return policy; + } + + const resh = { ...policy.resh }; + delete resh.age; + return { ...policy, resh }; +} + +function hasConditionalRequestHeaders(request: Request): boolean { + return ( + request.headers.has('if-none-match') || + request.headers.has('if-modified-since') + ); +} + +/** + * Returns true when conditional request headers are satisfied by cached response headers. + * Matches Cloudflare Cache API behavior for If-None-Match and If-Modified-Since on match(). + */ +function satisfiesConditionalRequest( + request: Request, + responseHeaders: Headers +): boolean { + const ifNoneMatch = request.headers.get('if-none-match'); + + if (ifNoneMatch) { + const cachedEtag = responseHeaders.get('etag'); + if (!cachedEtag) { + return false; + } + if (ifNoneMatch.trim() === '*') { + return true; + } + + const cachedTag = normalizeEntityTag(cachedEtag); + return ifNoneMatch.split(',').some((tag) => { + return normalizeEntityTag(tag) === cachedTag; + }); + } + + const ifModifiedSince = request.headers.get('if-modified-since'); + if (ifModifiedSince) { + const lastModified = responseHeaders.get('last-modified'); + if (!lastModified) { + return false; + } + + const ifModifiedSinceTime = Date.parse(ifModifiedSince); + const lastModifiedTime = Date.parse(lastModified); + if ( + !Number.isFinite(ifModifiedSinceTime) || + !Number.isFinite(lastModifiedTime) + ) { + return false; + } + + return lastModifiedTime <= ifModifiedSinceTime; + } + + return false; +} + +function normalizeEntityTag(tag: string): string { + return tag.trim().replace(/^\s*W\//, ''); +} + +function createNotModifiedResponse(responseHeaders: Headers): Response { + return new Response(null, { + status: 304, + statusText: 'Not Modified', + headers: responseHeaders, + }); +} + /** * Retrieves a cache item from storage with Vary header support. * Implements HTTP Vary header processing as per RFC 7234 Section 4.1. diff --git a/src/constants.ts b/src/constants.ts index ba42b9d..09c37c7 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -26,9 +26,12 @@ export const MISS: SharedCacheStatus = 'MISS'; /** Cached response was expired, fresh response fetched */ export const EXPIRED: SharedCacheStatus = 'EXPIRED'; -/** Stale response served (e.g., during stale-while-revalidate) */ +/** Stale response served when origin is unreachable (stale-if-error) */ export const STALE: SharedCacheStatus = 'STALE'; +/** Expired response served while revalidating in the background (stale-while-revalidate) */ +export const UPDATING: SharedCacheStatus = 'UPDATING'; + /** Cache was bypassed due to cache-control directives */ export const BYPASS: SharedCacheStatus = 'BYPASS'; diff --git a/src/fetch.test.ts b/src/fetch.test.ts index a0c6c5f..0502a8c 100644 --- a/src/fetch.test.ts +++ b/src/fetch.test.ts @@ -24,7 +24,7 @@ import { EXPIRED, HIT, MISS, - STALE, + UPDATING, } from './constants'; const TEST_URL = 'http://localhost/'; @@ -722,6 +722,31 @@ describe('Response Body Caching', () => { expect(res.status).toBe(200); expect(await cachedRes?.text()).toBe('lol'); }); + + it('should return 304 when the client sends a matching If-None-Match', async () => { + const store = createCacheStore(); + const cache = new SharedCache(store); + const fetch = createSharedCacheFetch(cache, { + async fetch() { + return new Response('lol', { + headers: { + 'cache-control': 'max-age=300', + etag: '"v1"', + }, + }); + }, + }); + + await fetch(TEST_URL); + + const res = await fetch(TEST_URL, { + headers: { 'if-none-match': '"v1"' }, + }); + + expect(res.status).toBe(304); + expect(await res.text()).toBe(''); + expect(res.headers.get('x-cache-status')).toBe(HIT); + }); }); /** @@ -1319,7 +1344,7 @@ describe('Vary Header Handling', () => { const res = await fetch(req); expect(res.status).toBe(200); - expect(res.headers.get('x-cache-status')).toBe(STALE); + expect(res.headers.get('x-cache-status')).toBe(UPDATING); expect(res.headers.get('age')).toBe('1'); expect(await res.text()).toBe('Hello 0'); @@ -1341,6 +1366,49 @@ describe('Vary Header Handling', () => { }); }); + describe('when revalidation returns 304', () => { + let fetchCount = 0; + const store = createCacheStore(); + const cache = new SharedCache(store); + const fetch = createSharedCacheFetch(cache, { + async fetch() { + fetchCount++; + if (fetchCount === 1) { + return new Response('page body', { + status: 200, + headers: { + 'cache-control': 'max-age=1, stale-while-revalidate=300', + etag: '"v1"', + }, + }); + } + return new Response(null, { + status: 304, + headers: { etag: '"v1"' }, + }); + }, + }); + + it('should return HIT after background 304 revalidation completes', async () => { + const req = new Request(TEST_URL); + + await fetch(req); + await timeout(1100); + + const staleRes = await fetch(req); + expect(staleRes.headers.get('x-cache-status')).toBe(UPDATING); + expect(await staleRes.text()).toBe('page body'); + + await timeout(50); + + const freshRes = await fetch(req); + expect(freshRes.headers.get('x-cache-status')).toBe(HIT); + expect(freshRes.headers.get('age')).toBe('0'); + expect(await freshRes.text()).toBe('page body'); + expect(fetchCount).toBe(2); + }); + }); + describe('when the cache is expired', () => { let count = 0; const store = createCacheStore(); @@ -1450,7 +1518,7 @@ describe('Vary Header Handling', () => { }); expect(res.status).toBe(200); - expect(res.headers.get('x-cache-status')).toBe(STALE); + expect(res.headers.get('x-cache-status')).toBe(UPDATING); expect(res.headers.get('age')).toBe('1'); expect(await res.text()).toBe('Hello 0'); @@ -1462,14 +1530,17 @@ describe('Vary Header Handling', () => { }); expect(res.status).toBe(200); - expect(res.headers.get('x-cache-status')).toBe(STALE); + expect(res.headers.get('x-cache-status')).toBe(UPDATING); expect(res.headers.get('age')).toBe('1'); expect(await res.text()).toBe('Hello 0'); + + // Allow background revalidation from the first stale response to finish + await timeout(100); }); it('step 4: should bypass caching when errors last too long', async () => { - // NOTE: Simulation exceeds max age - await timeout(1008); + // Exceed max-age and stale-if-error after the refreshed entry was stored + await timeout(2100); const req = new Request(TEST_URL); const res = await fetch(req, { @@ -1911,6 +1982,31 @@ describe('Vary Header Handling', () => { ); }); + it('should percent-encode cache key characters illegal in header values', async () => { + const store = createCacheStore(); + const cache = new SharedCache(store); + const fetch = createSharedCacheFetch(cache, { + async fetch() { + return new Response('encoded cache key', { + headers: { + 'cache-control': 'max-age=300', + }, + }); + }, + }); + + const url = 'http://localhost/?q=hello%0Aworld'; + const response = await fetch(url, { + sharedCache: { debugCacheKey: true }, + }); + + const cacheKeyHeader = response.headers.get(CACHE_KEY_HEADER_NAME); + expect(cacheKeyHeader).toBe('localhost/?q=hello%0Aworld'); + expect(() => + new Headers().set(CACHE_KEY_HEADER_NAME, cacheKeyHeader!) + ).not.toThrow(); + }); + it('should respect ignoreRequestCacheControl option', async () => { const store = createCacheStore(); const cache = new SharedCache(store); diff --git a/src/fetch.ts b/src/fetch.ts index 8002868..43f296f 100644 --- a/src/fetch.ts +++ b/src/fetch.ts @@ -1,7 +1,11 @@ import { vary } from './utils/vary'; import { vary as getVaryCachePart } from './cache-key'; import { cacheControl } from './utils/cache-control'; -import { setResponseHeader, modifyResponseHeaders } from './utils/response'; +import { + encodeCacheKeyHeaderValue, + modifyResponseHeaders, + setResponseHeader, +} from './utils/response'; import { SharedCache } from './cache'; import { SharedCacheStorage } from './cache-storage'; import { @@ -244,7 +248,11 @@ function setCacheStatus( */ function setCacheKey(response: Response, cacheKey?: string): Response { if (cacheKey) { - return setResponseHeader(response, CACHE_KEY_HEADER_NAME, cacheKey); + return setResponseHeader( + response, + CACHE_KEY_HEADER_NAME, + encodeCacheKeyHeaderValue(cacheKey) + ); } return response; } diff --git a/src/index.ts b/src/index.ts index 03ebec3..3eee081 100644 --- a/src/index.ts +++ b/src/index.ts @@ -96,4 +96,5 @@ export { MISS, REVALIDATED, STALE, + UPDATING, } from './constants'; diff --git a/src/types.ts b/src/types.ts index e2aa874..0a2235f 100644 --- a/src/types.ts +++ b/src/types.ts @@ -126,6 +126,12 @@ export interface PolicyResponse { policy: CachePolicy; /** The cached response */ response: Response; + /** + * Cached response body when already materialized in storage. + * Avoids re-reading the response stream during 304 revalidation. + * @internal + */ + cachedBody?: string; } /** @@ -139,8 +145,10 @@ export type SharedCacheStatus = | 'MISS' /** Cached response expired, fetched fresh response */ | 'EXPIRED' - /** Stale response served (stale-while-revalidate) */ + /** Stale response served when origin is unreachable (stale-if-error) */ | 'STALE' + /** Expired response served while revalidating in the background (stale-while-revalidate) */ + | 'UPDATING' /** Cache bypassed due to cache-control directives */ | 'BYPASS' /** Cached response revalidated and still fresh */ @@ -198,6 +206,7 @@ export interface SharedCacheRequestInitProperties { /** * Whether to expose the computed cache key via response header. * When true, the response includes the `x-cache-key` header for debugging. + * Non-ASCII and control characters in the key are percent-encoded for valid HTTP headers. */ debugCacheKey?: boolean; diff --git a/src/utils/response.test.ts b/src/utils/response.test.ts index ab75210..eafc344 100644 --- a/src/utils/response.test.ts +++ b/src/utils/response.test.ts @@ -1,4 +1,8 @@ -import { modifyResponseHeaders, setResponseHeader } from './response'; +import { + encodeCacheKeyHeaderValue, + modifyResponseHeaders, + setResponseHeader, +} from './response'; describe('Response Utils', () => { describe('modifyResponseHeaders', () => { @@ -12,9 +16,12 @@ describe('Response Utils', () => { }); // When headers are mutable, should return the same response object - const modifiedResponse = modifyResponseHeaders(originalResponse, (headers) => { - headers.set('x-test', 'value'); - }); + const modifiedResponse = modifyResponseHeaders( + originalResponse, + (headers) => { + headers.set('x-test', 'value'); + } + ); // Should be the same object reference (no cloning) expect(modifiedResponse).toBe(originalResponse); @@ -29,24 +36,31 @@ describe('Response Utils', () => { statusText: 'Created', headers: { 'content-type': 'application/json', - 'etag': '"abc123"', + etag: '"abc123"', }, }); // Mock headers.set to throw an error (simulating readonly headers) - const originalSet = originalResponse.headers.set.bind(originalResponse.headers); + const originalSet = originalResponse.headers.set.bind( + originalResponse.headers + ); originalResponse.headers.set = () => { throw new Error('Cannot modify readonly headers'); }; - const modifiedResponse = modifyResponseHeaders(originalResponse, (headers) => { - headers.set('x-test', 'value'); - }); + const modifiedResponse = modifyResponseHeaders( + originalResponse, + (headers) => { + headers.set('x-test', 'value'); + } + ); // Should be different object when modification fails expect(modifiedResponse).not.toBe(originalResponse); expect(modifiedResponse.headers.get('x-test')).toBe('value'); - expect(modifiedResponse.headers.get('content-type')).toBe('application/json'); + expect(modifiedResponse.headers.get('content-type')).toBe( + 'application/json' + ); expect(modifiedResponse.headers.get('etag')).toBe('"abc123"'); expect(modifiedResponse.status).toBe(201); expect(modifiedResponse.statusText).toBe('Created'); @@ -63,7 +77,7 @@ describe('Response Utils', () => { headers: { 'content-type': 'text/plain; charset=utf-8', 'content-length': originalBody.length.toString(), - 'etag': '"abc123"', + etag: '"abc123"', 'last-modified': 'Wed, 21 Oct 2015 07:28:00 GMT', }, }); @@ -73,18 +87,27 @@ describe('Response Utils', () => { throw new Error('Headers are readonly'); }; - const modifiedResponse = modifyResponseHeaders(originalResponse, (headers) => { - headers.set('x-modified', 'true'); - headers.set('x-timestamp', Date.now().toString()); - }); + const modifiedResponse = modifyResponseHeaders( + originalResponse, + (headers) => { + headers.set('x-modified', 'true'); + headers.set('x-timestamp', Date.now().toString()); + } + ); // Verify all properties are preserved expect(modifiedResponse.status).toBe(201); expect(modifiedResponse.statusText).toBe('Created'); - expect(modifiedResponse.headers.get('content-type')).toBe('text/plain; charset=utf-8'); - expect(modifiedResponse.headers.get('content-length')).toBe(originalBody.length.toString()); + expect(modifiedResponse.headers.get('content-type')).toBe( + 'text/plain; charset=utf-8' + ); + expect(modifiedResponse.headers.get('content-length')).toBe( + originalBody.length.toString() + ); expect(modifiedResponse.headers.get('etag')).toBe('"abc123"'); - expect(modifiedResponse.headers.get('last-modified')).toBe('Wed, 21 Oct 2015 07:28:00 GMT'); + expect(modifiedResponse.headers.get('last-modified')).toBe( + 'Wed, 21 Oct 2015 07:28:00 GMT' + ); expect(modifiedResponse.headers.get('x-modified')).toBe('true'); expect(modifiedResponse.headers.has('x-timestamp')).toBe(true); expect(await modifiedResponse.text()).toBe(originalBody); @@ -95,12 +118,15 @@ describe('Response Utils', () => { headers: { 'content-type': 'text/plain' }, }); - const modifiedResponse = modifyResponseHeaders(originalResponse, (headers) => { - headers.set('x-cache', 'miss'); - headers.set('x-served-by', 'shared-cache'); - headers.append('vary', 'accept-encoding'); - headers.delete('content-type'); - }); + const modifiedResponse = modifyResponseHeaders( + originalResponse, + (headers) => { + headers.set('x-cache', 'miss'); + headers.set('x-served-by', 'shared-cache'); + headers.append('vary', 'accept-encoding'); + headers.delete('content-type'); + } + ); expect(modifiedResponse.headers.get('x-cache')).toBe('miss'); expect(modifiedResponse.headers.get('x-served-by')).toBe('shared-cache'); @@ -119,9 +145,12 @@ describe('Response Utils', () => { throw new Error('Headers modification not allowed'); }; - const modifiedResponse = modifyResponseHeaders(originalResponse, (headers) => { - headers.set('x-error-test', 'true'); - }); + const modifiedResponse = modifyResponseHeaders( + originalResponse, + (headers) => { + headers.set('x-error-test', 'true'); + } + ); // Should create new response when modification fails expect(modifiedResponse).not.toBe(originalResponse); @@ -137,7 +166,11 @@ describe('Response Utils', () => { headers: { 'content-type': 'text/plain' }, }); - const modifiedResponse = setResponseHeader(originalResponse, 'x-cache', 'hit'); + const modifiedResponse = setResponseHeader( + originalResponse, + 'x-cache', + 'hit' + ); // Should be the same object when headers are mutable expect(modifiedResponse).toBe(originalResponse); @@ -157,7 +190,11 @@ describe('Response Utils', () => { throw new Error('Cannot set readonly header'); }; - const modifiedResponse = setResponseHeader(originalResponse, 'x-cache', 'miss'); + const modifiedResponse = setResponseHeader( + originalResponse, + 'x-cache', + 'miss' + ); // Should be different objects when headers are readonly expect(modifiedResponse).not.toBe(originalResponse); @@ -225,13 +262,51 @@ describe('Response Utils', () => { throw new Error('Readonly headers'); }; - const modifiedResponse = modifyResponseHeaders(originalResponse, (headers) => { - headers.set('x-stream-test', 'true'); - }); + const modifiedResponse = modifyResponseHeaders( + originalResponse, + (headers) => { + headers.set('x-stream-test', 'true'); + } + ); // Body should still be readable expect(await modifiedResponse.text()).toBe(bodyContent); expect(modifiedResponse.headers.get('x-stream-test')).toBe('true'); }); }); -}); \ No newline at end of file + + describe('encodeCacheKeyHeaderValue', () => { + it('should leave ASCII cache keys unchanged', () => { + expect(encodeCacheKeyHeaderValue('localhost/?a=1')).toBe( + 'localhost/?a=1' + ); + }); + + it('should percent-encode control characters forbidden by Fetch', () => { + expect(encodeCacheKeyHeaderValue('a\0b')).toBe('a%00b'); + expect(encodeCacheKeyHeaderValue('a\nb')).toBe('a%0Ab'); + expect(encodeCacheKeyHeaderValue('a\rb')).toBe('a%0Db'); + }); + + it('should percent-encode code points above Latin-1 for ByteString runtimes', () => { + expect(encodeCacheKeyHeaderValue('中文')).toBe('%E4%B8%AD%E6%96%87'); + expect(encodeCacheKeyHeaderValue('😀')).toBe('%F0%9F%98%80'); + }); + + it('should produce values accepted by Headers.set', () => { + const cases = [ + 'localhost/?q=hello%0Aworld', + 'localhost/?q=hello\nworld', + 'localhost/中文', + 'evil\r\nX-Injected: true', + ]; + + for (const cacheKey of cases) { + const encoded = encodeCacheKeyHeaderValue(cacheKey); + const headers = new Headers(); + expect(() => headers.set('x-cache-key', encoded)).not.toThrow(); + expect(headers.get('x-cache-key')).toBe(encoded); + } + }); + }); +}); diff --git a/src/utils/response.ts b/src/utils/response.ts index 67b354a..265a885 100644 --- a/src/utils/response.ts +++ b/src/utils/response.ts @@ -32,6 +32,33 @@ export function modifyResponseHeaders( } } +/** + * Encodes a cache key for safe use as an HTTP header field value. + * + * Cache keys may contain Unicode path segments, query values, or control + * characters that violate Fetch header constraints (NUL/CR/LF) or runtime + * ByteString limits (code points above U+00FF). + * + * @param cacheKey - Raw cache key from storage key generation + * @returns Header-safe cache key (ASCII-only, percent-encoded where needed) + * @internal + */ +export function encodeCacheKeyHeaderValue(cacheKey: string): string { + let encoded = ''; + + for (const char of cacheKey) { + const code = char.codePointAt(0)!; + + if (code === 0 || code === 0xa || code === 0xd || code > 0xff) { + encoded += encodeURIComponent(char); + } else { + encoded += char; + } + } + + return encoded; +} + /** * Safely sets a header on a response, creating a new response if headers are readonly. * This is a convenience function for setting a single header. From 28302192d78ee446d2f12c709e88532ed6d44bf4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=B3=96=E9=A5=BC?= Date: Wed, 17 Jun 2026 00:32:44 +0800 Subject: [PATCH 2/2] fix: address PR review feedback for cache revalidation Use STALE for stale-if-error on origin errors, omit body headers on conditional 304 responses, rename cachedBody to storedBody, and add tests for conditional match edge cases and non-storable revalidation. --- src/cache.test.ts | 132 ++++++++++++++++++++++++++++++++++++++++++++++ src/cache.ts | 45 ++++++++++++++-- src/types.ts | 4 +- 3 files changed, 174 insertions(+), 7 deletions(-) diff --git a/src/cache.test.ts b/src/cache.test.ts index 764318c..e939021 100644 --- a/src/cache.test.ts +++ b/src/cache.test.ts @@ -5,6 +5,7 @@ import type { Logger } from './utils/logger'; import { CACHE_STATUS_HEADER_NAME, HIT, + STALE, UPDATING, REVALIDATED, EXPIRED, @@ -315,6 +316,87 @@ describe('SharedCache', () => { expect(matched!.headers.get(CACHE_STATUS_HEADER_NAME)).toBe(HIT); }); + it('should omit representation headers on conditional 304 responses', async () => { + const request = new Request('https://example.com/conditional-304-headers'); + await cache.put( + request, + createTestResponse('cached data', 200, { + 'cache-control': 'max-age=300', + etag: '"v1"', + 'content-type': 'text/plain', + 'content-length': '11', + }) + ); + + const matched = await cache.match( + new Request('https://example.com/conditional-304-headers', { + headers: { 'if-none-match': '"v1"' }, + }) + ); + + expect(matched!.status).toBe(304); + expect(matched!.headers.get('etag')).toBe('"v1"'); + expect(matched!.headers.get('content-type')).toBe(null); + expect(matched!.headers.get('content-length')).toBe(null); + }); + + it('should return 304 when If-None-Match is * and the cached entry has an ETag', async () => { + const request = new Request('https://example.com/conditional-star'); + await cache.put( + request, + createTestResponse('cached data', 200, { + 'cache-control': 'max-age=300', + etag: '"v1"', + }) + ); + + const matched = await cache.match( + new Request('https://example.com/conditional-star', { + headers: { 'if-none-match': '*' }, + }) + ); + + expect(matched!.status).toBe(304); + }); + + it('should match weak ETags on If-None-Match', async () => { + const request = new Request('https://example.com/conditional-weak-etag'); + await cache.put( + request, + createTestResponse('cached data', 200, { + 'cache-control': 'max-age=300', + etag: 'W/"v1"', + }) + ); + + const matched = await cache.match( + new Request('https://example.com/conditional-weak-etag', { + headers: { 'if-none-match': 'W/"v1"' }, + }) + ); + + expect(matched!.status).toBe(304); + }); + + it('should return 200 when If-Modified-Since is invalid', async () => { + const request = new Request('https://example.com/conditional-invalid-ims'); + await cache.put( + request, + createTestResponse('cached data', 200, { + 'cache-control': 'max-age=300', + 'last-modified': 'Wed, 21 Oct 2015 07:28:00 GMT', + }) + ); + + const matched = await cache.match( + new Request('https://example.com/conditional-invalid-ims', { + headers: { 'if-modified-since': 'not-a-date' }, + }) + ); + + expect(matched!.status).toBe(200); + }); + it('should return 200 when If-None-Match does not match the cached ETag', async () => { const request = new Request('https://example.com/conditional-etag-miss'); await cache.put( @@ -450,6 +532,56 @@ describe('SharedCache', () => { expect(backgroundPromise).not.toBeNull(); }); + it('should return STALE when sync revalidation hits origin error within stale-if-error', async () => { + const request = new Request('https://example.com/stale-if-error'); + await cache.put( + request, + createTestResponse('cached data', 200, { + 'cache-control': 'max-age=1, stale-if-error=300', + etag: '"v1"', + }) + ); + + await new Promise((resolve) => setTimeout(resolve, 1100)); + + const matched = await cache.match(request, { + _fetch: async () => new Response('Internal Server Error', { status: 500 }), + }); + + expect(matched).toBeDefined(); + expect(matched!.status).toBe(200); + expect(await matched!.text()).toBe('cached data'); + expect(matched!.headers.get(CACHE_STATUS_HEADER_NAME)).toBe(STALE); + }); + + it('should not persist revalidated response when the new policy is not storable', async () => { + const request = new Request('https://example.com/no-store-revalidate'); + await cache.put( + request, + createTestResponse('old body', 200, { + 'cache-control': 'max-age=1, stale-if-error=300', + etag: '"v1"', + }) + ); + + await new Promise((resolve) => setTimeout(resolve, 1100)); + + const revalidated = await cache.match(request, { + _fetch: async () => + createTestResponse('new body', 200, { + 'cache-control': 'no-store', + etag: '"v2"', + }), + }); + + expect(revalidated).toBeDefined(); + expect(revalidated!.headers.get(CACHE_STATUS_HEADER_NAME)).toBe(EXPIRED); + expect(await revalidated!.text()).toBe('new body'); + + const cached = await cache.match(request); + expect(cached).toBeUndefined(); + }); + describe('revalidation persistence', () => { const mock304Revalidate = async () => new Response(null, { diff --git a/src/cache.ts b/src/cache.ts index f865cd4..8e1f515 100644 --- a/src/cache.ts +++ b/src/cache.ts @@ -23,6 +23,7 @@ import { EXPIRED, HIT, REVALIDATED, + STALE, UPDATING, } from './constants'; import { CachePolicy } from './utils/cache-semantics'; @@ -286,9 +287,9 @@ export class SharedCache implements WebCache { this.#revalidate( r, { - response: response.clone(), + response, policy, - cachedBody: body, + storedBody: body, }, cacheKey, fetch, @@ -313,7 +314,7 @@ export class SharedCache implements WebCache { { response, policy, - cachedBody: body, + storedBody: body, }, cacheKey, fetch, @@ -605,7 +606,7 @@ export class SharedCache implements WebCache { responseStatusText = revalidationResponse.statusText; } else { responseBody = - resolveCacheItem.cachedBody ?? + resolveCacheItem.storedBody ?? (await resolveCacheItem.response.clone().text()); responseStatus = resolveCacheItem.response.status; responseStatusText = resolveCacheItem.response.statusText; @@ -641,6 +642,20 @@ export class SharedCache implements WebCache { }, 'Serving fresh response' ); + } else if ( + isErrorResponse(revalidationResponse) && + resolveCacheItem.policy.useStaleIfError() + ) { + this.#setCacheStatus(clonedResponse, STALE); + this.#structuredLogger.info( + 'Serving stale response', + { + url: request.url, + cacheKey, + cacheStatus: 'STALE', + }, + 'Origin error within stale-if-error window' + ); } else { this.#setCacheStatus(clonedResponse, REVALIDATED); this.#structuredLogger.info( @@ -833,14 +848,34 @@ function normalizeEntityTag(tag: string): string { return tag.trim().replace(/^\s*W\//, ''); } +/** Headers that must not appear on 304 responses (RFC 7232). */ +const NOT_MODIFIED_OMIT_HEADERS = new Set([ + 'content-length', + 'content-type', + 'content-encoding', + 'transfer-encoding', +]); + function createNotModifiedResponse(responseHeaders: Headers): Response { + const headers = new Headers(); + + responseHeaders.forEach((value, name) => { + if (!NOT_MODIFIED_OMIT_HEADERS.has(name.toLowerCase())) { + headers.set(name, value); + } + }); + return new Response(null, { status: 304, statusText: 'Not Modified', - headers: responseHeaders, + headers, }); } +function isErrorResponse(response: Response): boolean { + return response.status >= 500; +} + /** * Retrieves a cache item from storage with Vary header support. * Implements HTTP Vary header processing as per RFC 7234 Section 4.1. diff --git a/src/types.ts b/src/types.ts index 0a2235f..bf0d824 100644 --- a/src/types.ts +++ b/src/types.ts @@ -127,11 +127,11 @@ export interface PolicyResponse { /** The cached response */ response: Response; /** - * Cached response body when already materialized in storage. + * Response body already materialized in storage. * Avoids re-reading the response stream during 304 revalidation. * @internal */ - cachedBody?: string; + storedBody?: string; } /**