Stale-while-revalidate background revalidation does not lead to updated cache entries upon receiving 304 not modified.

[daan944]

Bug Description

Although async behavior was added in #4492 (thank you for that 👍 ), the CacheHandler does not know what to do with 304 not modified responses and ignores them. This causes the next request to be hit the backend again, instead of the updated cache.

See interceptor/cache.js#259 and handler/cache-handler.js#254 (NOT_UNDERSTOOD_STATUS_CODES includes 304).

Reproducible By

1. Configure undici with cache interceptor.
2. Have a backend that replies with a 304 when if-none-match and/or if-modified-since headers are present. The reply must include cache headers, e.g. max-age=5, stale-while-revalidate=3600.
3. Perform a request with empty cache: see 200 OK and cache entry is inserted.
4. Wait 5s (so response is stale) and perform a new request.
5. Cache handler is responding immediately with stale data (good!) and starts revalidation.
6. Revalidation is being answered by backend with 304 not modified.
7. Perform a new request
8. See request hitting backend server, not using cache (I think). Backend server replies with 200 OK again and it is being cached again.

In my Varnish logs I see requests answered with 304 and upon next request requested again and answered with 200 (which implies this is not a revalidation request).

Expected Behavior

When revalidation request is being answered with a 304 not modified and cache headers, we expect the cache expiry to be updated for this request.

Additional context

I think that we should use the CacheRevalidationHandler in the cache interceptor (instead of the CacheHandler) and update the cache upon succesful revalidation. Similar to what is happening when we're within the stale-if-error period, see interceptor/cache.js#325.

[metcoder95]

Thanks for the report! Would you like to send a PR for it?

[daan944]

I see 2 routes to fix this:

1. re-write the cache revalidation handler. Upon reading the code, I think there's more amiss there. I suspect a 304 from the backend within stale-if-error period will also result in a stale warning. And I don't think there's a cache update at that moment. I haven't had the time to doublecheck these suspicions. I think this is the preferred approach, but the most time consuming.
2. remove the additional headers (if-modified-since, if-none-match), so we expect our backends to reply with 200 OK (which we currently understand) instead of 304 not modified. This is a quick fix, but not really efficient as we will be re-downloading the body unnecessarily (this especially sucks for projects fetching large responses). But.. It would still be an improvement vs the current revalidation as it will be done in the background.

First step would be to write a test for this. I already started on these, but I don't think I can make the time to rewrite the revalidation handler, although that would be the preferred approach.

[metcoder95]

Let's move with option 1 and we take it from there; no need to rush it

[daan944]

After further investigation: the 304 did not invalidate the cache, just kept as-is. That means that stale-while-revalidate window was extending max-age while sending (useless) revalidation requests to backend. Anyway, fixed that in PR #4617

(option 1)

I did modify the stale-if-error revalidation handling, but added a TODO in the code.