cache: fix stale-while-revalidate and stale-if-error (#3865)

* cache: fix stale-while-revalidate and stale-if-error Closes #3853 Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> * type tests Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> * test fixup Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --------- Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
nodejs · Nov 25, 2024 · 71b6b0b · 71b6b0b
1 parent 6ef1763
commit 71b6b0b
Show file tree

Hide file tree

Showing 11 changed files with 622 additions and 245 deletions.
diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js
@@ -90,6 +90,7 @@ class MemoryCacheStore {
           headers: entry.headers,
           body: entry.body,
           etag: entry.etag,
+          cacheControlDirectives: entry.cacheControlDirectives,
           cachedAt: entry.cachedAt,
           staleAt: entry.staleAt,
           deleteAt: entry.deleteAt

diff --git a/lib/cache/sqlite-cache-store.js b/lib/cache/sqlite-cache-store.js
@@ -108,6 +108,7 @@ class SqliteCacheStore {
         statusCode INTEGER NOT NULL,
         statusMessage TEXT NOT NULL,
         headers TEXT NULL,
+        cacheControlDirectives TEXT NULL,
         etag TEXT NULL,
         vary TEXT NULL,
         cachedAt INTEGER NOT NULL,
@@ -128,6 +129,7 @@ class SqliteCacheStore {
         statusMessage,
         headers,
         etag,
+        cacheControlDirectives,
         vary,
         cachedAt,
         staleAt
@@ -147,6 +149,7 @@ class SqliteCacheStore {
         statusMessage = ?,
         headers = ?,
         etag = ?,
+        cacheControlDirectives = ?,
         cachedAt = ?,
         staleAt = ?,
         deleteAt = ?
@@ -164,11 +167,12 @@ class SqliteCacheStore {
         statusMessage,
         headers,
         etag,
+        cacheControlDirectives,
         vary,
         cachedAt,
         staleAt,
         deleteAt
-      ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
+      ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
     `)
 
     this.#deleteByUrlQuery = this.#db.prepare(
@@ -223,6 +227,9 @@ class SqliteCacheStore {
       statusMessage: value.statusMessage,
       headers: value.headers ? JSON.parse(value.headers) : undefined,
       etag: value.etag ? value.etag : undefined,
+      cacheControlDirectives: value.cacheControlDirectives
+        ? JSON.parse(value.cacheControlDirectives)
+        : undefined,
       cachedAt: value.cachedAt,
       staleAt: value.staleAt,
       deleteAt: value.deleteAt
@@ -275,6 +282,7 @@ class SqliteCacheStore {
             value.statusMessage,
             value.headers ? JSON.stringify(value.headers) : null,
             value.etag ? value.etag : null,
+            value.cacheControlDirectives ? JSON.stringify(value.cacheControlDirectives) : null,
             value.cachedAt,
             value.staleAt,
             value.deleteAt,
@@ -292,6 +300,7 @@ class SqliteCacheStore {
             value.statusMessage,
             value.headers ? JSON.stringify(value.headers) : null,
             value.etag ? value.etag : null,
+            value.cacheControlDirectives ? JSON.stringify(value.cacheControlDirectives) : null,
             value.vary ? JSON.stringify(value.vary) : null,
             value.cachedAt,
             value.staleAt,

diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js
@@ -113,6 +113,7 @@ class CacheHandler {
         statusMessage,
         headers: strippedHeaders,
         vary: varyDirectives,
+        cacheControlDirectives,
         cachedAt: now,
         staleAt,
         deleteAt
@@ -170,7 +171,7 @@ class CacheHandler {
  *
  * @param {number} statusCode
  * @param {Record<string, string | string[]>} headers
- * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives
+ * @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives} cacheControlDirectives
  */
 function canCacheResponse (statusCode, headers, cacheControlDirectives) {
   if (statusCode !== 200 && statusCode !== 307) {
@@ -217,7 +218,7 @@ function canCacheResponse (statusCode, headers, cacheControlDirectives) {
 /**
  * @param {number} now
  * @param {Record<string, string | string[]>} headers
- * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives
+ * @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives} cacheControlDirectives
  *
  * @returns {number | undefined} time that the value is stale at or undefined if it shouldn't be cached
  */
@@ -253,21 +254,28 @@ function determineStaleAt (now, headers, cacheControlDirectives) {
 
 /**
  * @param {number} now
- * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives
+ * @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives} cacheControlDirectives
  * @param {number} staleAt
  */
 function determineDeleteAt (now, cacheControlDirectives, staleAt) {
+  let staleWhileRevalidate = -Infinity
+  let staleIfError = -Infinity
+
   if (cacheControlDirectives['stale-while-revalidate']) {
-    return now + (cacheControlDirectives['stale-while-revalidate'] * 1000)
+    staleWhileRevalidate = now + (cacheControlDirectives['stale-while-revalidate'] * 1000)
+  }
+
+  if (cacheControlDirectives['stale-if-error']) {
+    staleIfError = now + (cacheControlDirectives['stale-if-error'] * 1000)
   }
 
-  return staleAt
+  return Math.max(staleAt, staleWhileRevalidate, staleIfError)
 }
 
 /**
  * Strips headers required to be removed in cached responses
  * @param {Record<string, string | string[]>} headers
- * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives
+ * @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives} cacheControlDirectives
  * @returns {Record<string, string | string []>}
  */
 function stripNecessaryHeaders (headers, cacheControlDirectives) {

diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js
@@ -17,10 +17,12 @@ const assert = require('node:assert')
  */
 class CacheRevalidationHandler {
   #successful = false
+
   /**
    * @type {((boolean, any) => void) | null}
    */
   #callback
+
   /**
    * @type {(import('../../types/dispatcher.d.ts').default.DispatchHandler)}
    */
@@ -29,19 +31,26 @@ class CacheRevalidationHandler {
   #context
 
   /**
-   * @param {(boolean, any) => void} callback Function to call if the cached value is valid
-   * @param {import('../../types/dispatcher.d.ts').default.DispatchHandler} handler
+   * @type {boolean}
+   */
+  #allowErrorStatusCodes
+
+  /**
+   * @param {(boolean) => void} callback Function to call if the cached value is valid
+   * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler
+   * @param {boolean} allowErrorStatusCodes
    */
-  constructor (callback, handler) {
+  constructor (callback, handler, allowErrorStatusCodes) {
     if (typeof callback !== 'function') {
       throw new TypeError('callback must be a function')
     }
 
     this.#callback = callback
     this.#handler = handler
+    this.#allowErrorStatusCodes = allowErrorStatusCodes
   }
 
-  onRequestStart (controller, context) {
+  onRequestStart (_, context) {
     this.#successful = false
     this.#context = context
   }
@@ -59,7 +68,9 @@ class CacheRevalidationHandler {
     assert(this.#callback != null)
 
     // https://www.rfc-editor.org/rfc/rfc9111.html#name-handling-a-validation-respo
-    this.#successful = statusCode === 304
+    // https://datatracker.ietf.org/doc/html/rfc5861#section-4
+    this.#successful = statusCode === 304 ||
+      (this.#allowErrorStatusCodes && statusCode >= 500 && statusCode <= 504)
     this.#callback(this.#successful, this.#context)
     this.#callback = null