From e18c736edfe921d802becd8a0205cf8b5c6559b1 Mon Sep 17 00:00:00 2001 From: Jesse Wright Date: Sat, 4 Jul 2026 16:41:01 +0000 Subject: [PATCH 1/2] fix: honor stale-if-error on connection errors during revalidation stale-if-error was only honored when the origin responded with a 500-504 status code; if the origin was unreachable (e.g. ECONNREFUSED), CacheRevalidationHandler.onResponseError failed the request outright even when a stale entry within the stale-if-error window existed. RFC 5861 section 4 defines an error as "any situation that would result in a 500, 502, 503 or 504 HTTP response status code being returned" - for a client-side cache, failing to reach the origin is exactly the situation a gateway would surface as 502/504. Repro (undici main, Node 22): store a response with 'Cache-Control: max-age=1, stale-if-error=60', stop the origin, wait past freshness, request again. - Before: throws ECONNREFUSED - After: stale cached response served (and the error still propagates once outside the stale-if-error window) The interceptor already computes the within-threshold decision (withinStaleIfErrorThreshold) and passes it to the handler; this change uses it on the error path too, when no response has started, invoking the callback with success so the stale entry is served. The mnot cache-tests conformance check 'stale-sie-close' now answers yes in all environments (previously no); no required tests regressed. Fixes #5507 Co-Authored-By: Claude Fable 5 --- lib/handler/cache-revalidation-handler.js | 13 ++ test/interceptors/cache.js | 145 +++++++++++++++++++++- 2 files changed, 157 insertions(+), 1 deletion(-) diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js index 393d16d52c6..23a45ba006b 100644 --- a/lib/handler/cache-revalidation-handler.js +++ b/lib/handler/cache-revalidation-handler.js @@ -109,6 +109,19 @@ class CacheRevalidationHandler { } if (this.#callback) { + // If the response hasn't started yet and we're within the + // stale-if-error threshold, serve the stale cached response instead + // of failing the request. RFC 5861 defines an error as "any situation + // that would result in a 500, 502, 503, or 504 HTTP response status + // code being returned", which includes failing to reach the origin. + // https://datatracker.ietf.org/doc/html/rfc5861#section-4 + if (this.#allowErrorStatusCodes) { + this.#successful = true + this.#callback(true, this.#context) + this.#callback = null + return + } + this.#callback(false) this.#callback = null } diff --git a/test/interceptors/cache.js b/test/interceptors/cache.js index d429f8004de..84a49c4a31b 100644 --- a/test/interceptors/cache.js +++ b/test/interceptors/cache.js @@ -3,7 +3,7 @@ const { createServer } = require('node:http') const { describe, test, after } = require('node:test') const { once } = require('node:events') -const { equal, strictEqual, notEqual, fail } = require('node:assert') +const { equal, strictEqual, notEqual, fail, rejects } = require('node:assert') const { setTimeout: sleep } = require('node:timers/promises') const FakeTimers = require('@sinonjs/fake-timers') const { Client, interceptors, cacheStores: { MemoryCacheStore, SqliteCacheStore } } = require('../../index') @@ -810,6 +810,76 @@ describe('Cache Interceptor', () => { } }) + test('stale-if-error (response) on connection error', async () => { + const clock = FakeTimers.install({ + toFake: ['Date'] + }) + + let requestsToOrigin = 0 + const server = createServer({ joinDuplicateHeaders: true }, (_, res) => { + requestsToOrigin++ + res.setHeader('date', 0) + res.setHeader('cache-control', 'public, s-maxage=10, stale-if-error=20') + res.end('asd') + }).listen(0) + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + clock.uninstall() + if (server.listening) { + server.close() + } + await client.close() + }) + + await once(server, 'listening') + + strictEqual(requestsToOrigin, 0) + + /** + * @type {import('../../types/dispatcher').default.RequestOptions} + */ + const request = { + origin: 'localhost', + method: 'GET', + path: '/' + } + + // Send first request. This will hit the origin and succeed + { + const response = await client.request(request) + equal(requestsToOrigin, 1) + equal(response.statusCode, 200) + equal(await response.body.text(), 'asd') + } + + // Take the origin down so revalidation attempts hit a connection error + server.close() + server.closeAllConnections() + await once(server, 'close') + + clock.tick(15000) + + // Send second request. The response is stale and the origin is + // unreachable, but we're within the stale-if-error threshold so the + // stale response should still be served. + // https://datatracker.ietf.org/doc/html/rfc5861#section-4 + { + const response = await client.request(request) + equal(requestsToOrigin, 1) + equal(response.statusCode, 200) + equal(await response.body.text(), 'asd') + } + + clock.tick(25000) + + // Send third request. We're now outside the stale-if-error threshold so + // the connection error should be propagated. + await rejects(client.request(request)) + }) + describe('Client-side directives', () => { test('max-age', async () => { const clock = FakeTimers.install({ @@ -1443,6 +1513,79 @@ describe('Cache Interceptor', () => { equal(response.statusCode, 500) } }) + + test('stale-if-error on connection error', async () => { + const clock = FakeTimers.install({ + toFake: ['Date'] + }) + + let requestsToOrigin = 0 + const server = createServer({ joinDuplicateHeaders: true }, (_, res) => { + requestsToOrigin++ + res.setHeader('date', 0) + res.setHeader('cache-control', 'public, s-maxage=10') + res.end('asd') + }).listen(0) + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + clock.uninstall() + if (server.listening) { + server.close() + } + await client.close() + }) + + await once(server, 'listening') + + strictEqual(requestsToOrigin, 0) + + // Send first request. This will hit the origin and succeed + { + const response = await client.request({ + origin: 'localhost', + path: '/', + method: 'GET' + }) + equal(requestsToOrigin, 1) + equal(response.statusCode, 200) + equal(await response.body.text(), 'asd') + } + + // Take the origin down so revalidation attempts hit a connection error + server.close() + server.closeAllConnections() + await once(server, 'close') + + clock.tick(15000) + + // Send second request. The response is stale and the origin is + // unreachable, but the request allows stale-if-error so the stale + // response should still be served. + { + const response = await client.request({ + origin: 'localhost', + path: '/', + method: 'GET', + headers: { + 'cache-control': 'stale-if-error=20' + } + }) + equal(requestsToOrigin, 1) + equal(response.statusCode, 200) + equal(await response.body.text(), 'asd') + } + + // Send third request without stale-if-error. The connection error + // should be propagated. + await rejects(client.request({ + origin: 'localhost', + path: '/', + method: 'GET' + })) + }) }) // Partial list. From f3e264fa07c208e32c92cd95e02df0248e9d7711 Mon Sep 17 00:00:00 2001 From: Jesse Wright <63333554+jeswr@users.noreply.github.com> Date: Sat, 4 Jul 2026 21:06:01 +0000 Subject: [PATCH 2/2] docs: shorten cache interceptor comments Addresses review feedback by trimming the stale-if-error comments to terse one-liners in line with the surrounding interceptor style; comment-only, no logic change. Co-Authored-By: Claude Fable 5 --- lib/handler/cache-revalidation-handler.js | 7 ++----- test/interceptors/cache.js | 9 ++------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js index 23a45ba006b..8bd8e7a502d 100644 --- a/lib/handler/cache-revalidation-handler.js +++ b/lib/handler/cache-revalidation-handler.js @@ -109,11 +109,8 @@ class CacheRevalidationHandler { } if (this.#callback) { - // If the response hasn't started yet and we're within the - // stale-if-error threshold, serve the stale cached response instead - // of failing the request. RFC 5861 defines an error as "any situation - // that would result in a 500, 502, 503, or 504 HTTP response status - // code being returned", which includes failing to reach the origin. + // Serve the stale cached response on a connection error, per stale-if-error: + // RFC 5861 counts an unreachable origin (a would-be 5xx) as an error. // https://datatracker.ietf.org/doc/html/rfc5861#section-4 if (this.#allowErrorStatusCodes) { this.#successful = true diff --git a/test/interceptors/cache.js b/test/interceptors/cache.js index 84a49c4a31b..39a659fd36d 100644 --- a/test/interceptors/cache.js +++ b/test/interceptors/cache.js @@ -862,10 +862,7 @@ describe('Cache Interceptor', () => { clock.tick(15000) - // Send second request. The response is stale and the origin is - // unreachable, but we're within the stale-if-error threshold so the - // stale response should still be served. - // https://datatracker.ietf.org/doc/html/rfc5861#section-4 + // Stale response, origin unreachable, but within stale-if-error: still served. { const response = await client.request(request) equal(requestsToOrigin, 1) @@ -1561,9 +1558,7 @@ describe('Cache Interceptor', () => { clock.tick(15000) - // Send second request. The response is stale and the origin is - // unreachable, but the request allows stale-if-error so the stale - // response should still be served. + // Stale response, origin unreachable, but request allows stale-if-error: still served. { const response = await client.request({ origin: 'localhost',