[GitHub] [trafficserver] bneradt commented on issue #7425: negative_revalidating_lifetime seems not respected

[GitBox <gi...@apache.org>]

bneradt commented on issue #7425:
URL: https://github.com/apache/trafficserver/issues/7425#issuecomment-805194659


   I'm working on this and adding an AuTest. In case this is helpful to others, I'll make some initial observations.
   
   My guess at the initial intention of this code was that setting the Expires header, observed earlier in this ticket, would set the freshness lifetime to the desired value. This doesn't work however, for at least three reasons:
   
   First, the Expires header is set to the value of `negative_revalidating_lifetime` _in the future from the current time_:
   
   https://github.com/apache/trafficserver/blob/08fe521a3974a05b01545c68fe1bcd5162dd4fc4/proxy/http/HttpTransact.cc#L4350
   
   If that actually was effective, then when `proxy.config.http.negative_revalidating_enabled` was enabled, every single cached response for a down server would always be considered fresh and served out of the cache because its Expires time would always be in the future. This is clearly not the intention. Rather, for this calculation to work, we'd want to set the Expires value to the time past the calculated age of the resource, not the offset from the current time.
   
   The second reason this doesn't work is that, per RFC 7234 section 4.2.1, the max-age directive takes precedence over the Expires header field. Thus in all responses that has max-age, the Expires header would never even be inspected. Notice that @lukenowak 's examples all use max-age, and indeed most tests would because that's the easiest way to test freshness behavior. (Fixing this by setting the max-age directive, however, would not be a sufficient fix because of the following paragraph.)
   
   Thirdly, by the time the Expires header is set in the code, we've already completed freshness calculations. Otherwise we wouldn't be in the code determining whether the stale response can be served. Recall that the negative revalidating feature influences behavior for _stale_ cached responses in which the origin is unreachable. Thus at this point, setting the Expires header is too late to influence ATS's freshness calculation. We would have to set it then re-perform the freshness calculation. This is not done.
   
   In order to make `proxy.config.http.negative_revalidating_lifetime` behave like it is documented, the three above issues would need to be addressed. 
   
   For the sake of completeness, if that were done, the intention is that things should work like this:
   
   1. A client makes a request to a server which responds with some cacheable positive response, such as a 200 OK.
   2. At a point in the future after the cached resource is stale, the client requests the resource.
   3. ATS notices that the resource is stale and attempts to revalidate it against the origin.
   4. The origin is either unreachable by ATS or it replies with some 5xx response.
   5. ATS notices that `proxy.config.http.negative_revalidating_enabled` is enabled.
   6. ATS recalculates the freshness of the resource using the adjusted freshness calculation as set in `proxy.config.http.negative_revalidating_lifetime`. If the resource is fresh enough given this adjustment, reply with it.
   7. If, after this adjusted freshness calculation, the object is still stale, then ATS will respect `max_stale_age`. If its staleness is less than `max_stale_age` it will reply with it, otherwise it will return a 5xx response.
   
   For now, with the bug described in this ticket, step 6 is broken and never results in the object's freshness being recalculated. Thus a user can only influence negative revalidating responses via `max_stale_age`.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org