Copilot commented on code in PR #13611:
URL: https://github.com/apache/trafficserver/pull/13611#discussion_r3958412025
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -2582,6 +2582,15 @@ HttpTransact::issue_revalidate(State *s)
return;
}
+ // An object found under the 9.2 key cannot be revalidated conditionally. The
+ // write that would apply a 304 is a create on the canonical key rather than
+ // an update of the legacy vector, so the cache discards it. Ask for the full
+ // response instead, which migrates the object to the canonical key.
+ if (s->state_machine != nullptr &&
HttpSM::should_use_compatibility_cache_key(s->state_machine->compatibility_cache_lookup))
{
+ TxnDbg(dbg_ctl_http_trans, "compatibility key hit, revalidating without
conditional headers");
Review Comment:
Returning early here prevents *adding* cache-derived conditional headers,
but it does not actively remove preexisting conditional headers that may
already be present on the outbound request (e.g., client-supplied If-None-Match
/ If-Modified-Since forwarded upstream). In that case the origin can still
respond 304 and the compatibility-key update path remains a no-op. Consider
explicitly stripping all conditional request headers from the server/parent
request when `compatibility_cache_lookup` is 9.2 (at least `If-None-Match` and
`If-Modified-Since`, and ideally the full set of preconditions) before
returning.
##########
include/proxy/http/HttpSM.h:
##########
@@ -344,6 +341,21 @@ class HttpSM : public Continuation, public
PluginUserArgs<TS_USER_ARGS_TXN>
void set_http_schedule(Continuation *);
int get_http_schedule(int event, void *data);
+ static bool
+ should_use_compatibility_cache_key(CompatibilityCacheLookup lookup)
+ {
+ return lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92;
+ }
+
+ static CacheHTTPInfo *
+ cache_write_info_for_lookup(CompatibilityCacheLookup lookup, CacheHTTPInfo
*object_read_info)
+ {
+ if (should_use_compatibility_cache_key(lookup)) {
+ return nullptr;
+ }
+ return object_read_info;
+ }
Review Comment:
These helpers are now used by `HttpCacheSM` and `HttpTransact`, which
introduces a tighter coupling on `HttpSM` than necessary (they’re essentially
small policy/utility functions over an enum + pointer). To reduce cross-module
dependency, consider moving them to a more neutral location (e.g., a small
http-cache-key-compat utility header, or `HttpTransact.h` if that’s where the
policy lives), or inline the simple enum comparison at call sites.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
