304 throws if last-modified doesn't match across requests

[jeff-amaze]

Hello,

I have not had a chance to build a repro (or PR), but at first glance it appears HTTPCache in the 304 validation flow does not properly handle the policy.revalidatedPolicy() return when the resulting status is 304 (and it returns modified)

HTTPCache.ts

const { policy: revalidatedPolicy, modified } = policy.revalidatedPolicy(
policyRequestFrom(urlString, revalidationRequest),
policyResponseFrom(revalidationResponse),
) as unknown as { policy: SneakyCachePolicy; modified: boolean };

If the response is a 304, but hcs (http-cache-semantics) doesn't think the result matches it will adopt the current response (which in my case has status 304), which will result in RESTDataSource.ts throwing.

Here's what hcs has to say about this case:
http-cache-semantics index.js

revalidatedPolicy(request, response) {
...
if (!matches) {
    return {
	policy: new this.constructor(request, response),
	// Client receiving 304 without body, even if it's invalid/mismatched has no option
	// but to reuse a cached body. We don't have a good way to tell clients to do
	// error recovery in such case.
	modified: response.status != 304,
	matches: false,
    };
}
...

They also call out the option of using the cached value, or doing another fresh request in the readme

revalidatedPolicy(revalidationRequest, revalidationResponse)

Use this method to update the cache after receiving a new response from the origin server. It returns an object with two keys:

    policy — A new CachePolicy with HTTP headers updated from revalidationResponse. You can always replace the old cached CachePolicy with the new one.
    modified — Boolean indicating whether the response body has changed.
        If false, then a valid 304 Not Modified response has been received, and you can reuse the old cached response body. This is also affected by stale-if-error.
        If true, you should use new response's body (if present), or make another request to the origin server without any conditional headers (i.e. don't use revalidationHeaders() this time) to get the new resource.

I suspect, in the error case I was seeing, cloudflare edge cache was adding a last-modified header based on the time when it was added to the cache, but subsequent calls would get different values depending in which node got hit causing hcs to discard the 304. It's possible it was upstream of cloudflare, but regardless it's probably inconsistent caches returning their local guess at last-modified time.

Seems like if there is a desire to fix this, HTTPCache can honor the 304, using the cached result and convert the 304 to a 200, or it can do another fresh request without the revalidation headers and get a fresh response.

hcs suggests using the body on the 304 if present, but as the spec precludes a 304 having content it's probably safer to not (unless there is some convention to do so I am not aware of).

A 304 response is terminated by the end of the header section; it cannot contain content or trailers.