Fix 92x compatibility cache revalidation

doc/admin-guide/files/records.yaml.en.rst

@@ -2800,6 +2800,24 @@ Cache Control
    You can monitor this metric and know when its safe to turn this feature off
    as the cache wraps around.
 
+   Two costs come with enabling this. Every cache miss performs a second
+   lookup, so a tier with a low hit ratio roughly doubles its cache lookup
+   load for the duration. And an object found under the previous key is
+   revalidated *without* conditional headers, because a ``304`` cannot be
+   applied to it: the write that would carry the update is a create under the
+   new key rather than an update of the old one. The origin therefore returns
+   the full response, which is stored under the new key. The copy under the
+   previous key is left in place to age out on its own, since nothing reports
+   that the new object reached disk; it stops being read as soon as the new key
+   resolves, so both keys briefly hold the object. Each object pays this once,
+   but on a large cache the aggregate is a bandwidth event worth sizing before
+   enabling the setting in production.
+
+   Objects whose path contains a ``;`` are unaffected. The previous algorithm
+   hashed the path and the deprecated ``;params`` segment as separate
+   components, which produces the same string the current algorithm produces
+   for such a path, so no compatibility lookup is issued for them.
+
 .. ts:cv:: CONFIG proxy.config.http.cache.range.lookup INT 1
    :overridable:
 

include/proxy/hdrs/URL.h

@@ -101,6 +101,13 @@ class URLImpl : public HdrHeapObjImpl
   void             set_type_code(unsigned int typecode);
   std::string_view get_params() const noexcept;
   void             set_params(HdrHeap *heap, std::string_view value, bool copy_string);
+  /** Whether the path carries a deprecated ";params" segment.
+
+    ATS 9.2 and earlier split "/path;params" into separate path and params
+    components. That parsing was removed, so the segment now stays inside the
+    path. Reproducing a 9.2 cache key has to know which form it is looking at.
+  */
+  bool             has_path_params() const noexcept;
   std::string_view get_query() const noexcept;
   void             set_query(HdrHeap *heap, std::string_view value, bool copy_string);
   std::string_view get_fragment() const noexcept;
@@ -261,6 +268,7 @@ class URL : public HdrHeapSDKHandle
   char *string_get_buf(char *dstbuf, int dsbuf_size, int *length = nullptr) const;
   void  hash_get(CryptoHash *hash, bool ignore_query = false, cache_generation_t generation = -1) const;
   void  hash_get92(CryptoHash *hash, bool ignore_query = false, cache_generation_t generation = -1) const;
+  bool  has_path_params() const noexcept;
   void  host_hash_get(CryptoHash *hash) const;
 
   std::string_view scheme_get() const noexcept;
@@ -488,6 +496,22 @@ URL::hash_get92(CryptoHash *hash, bool ignore_query, cache_generation_t generati
   url_CryptoHash_get_92(m_url_impl, hash, ignore_query, generation);
 }
 
+/*-------------------------------------------------------------------------
+  -------------------------------------------------------------------------*/
+
+inline bool
+URLImpl::has_path_params() const noexcept
+{
+  return m_ptr_path != nullptr && memchr(m_ptr_path, ';', m_len_path) != nullptr;
+}
+
+inline bool
+URL::has_path_params() const noexcept
+{
+  ink_assert(valid());
+  return m_url_impl->has_path_params();
+}
+
 /*-------------------------------------------------------------------------
   -------------------------------------------------------------------------*/
 

include/proxy/http/HttpSM.h

@@ -35,8 +35,6 @@
 #include <optional>
 #include <memory>
 
-#include "tscore/ink_platform.h"
-#include "iocore/eventsystem/EventSystem.h"
 #include "proxy/http/HttpCacheSM.h"
 #include "proxy/http/HttpTransact.h"
 #include "proxy/http/HttpUserAgent.h"
@@ -45,7 +43,6 @@
 #include "proxy/http/HttpTunnel.h"
 #include "api/InkAPIInternal.h"
 #include "proxy/ProxyTransaction.h"
-#include "proxy/hdrs/HdrUtils.h"
 
 // inknet
 #include "proxy/http/PreWarmManager.h"
@@ -182,6 +179,22 @@ enum class CompatibilityCacheLookup {
   COMPAT_CACHE_LAST,
 };
 
+/// Whether this lookup addresses the cache with the previous (9.2) key.
+inline bool
+should_use_compatibility_cache_key(CompatibilityCacheLookup lookup)
+{
+  return lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92;
+}
+
+/// The object info to hand to a cache write, which a compatibility read must not
+/// carry: it belongs to the legacy key and would turn the write into an update
+/// of a vector the canonical key does not have.
+inline CacheHTTPInfo *
+cache_write_info_for_lookup(CompatibilityCacheLookup lookup, CacheHTTPInfo *object_read_info)
+{
+  return should_use_compatibility_cache_key(lookup) ? nullptr : object_read_info;
+}
+
 class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
 {
   friend class HttpTransact;
@@ -406,6 +419,7 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
 
   void do_hostdb_lookup();
   void do_hostdb_reverse_lookup();
+  URL *cache_lookup_url();
   void do_cache_lookup_and_read();
   void do_http_server_open(bool raw = false, bool only_direct = false);
   bool apply_ip_allow_filter();
@@ -421,6 +435,7 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
   void do_cache_prepare_update();
   void do_cache_prepare_action(HttpCacheSM *c_sm, CacheHTTPInfo *object_read_info, bool retry, bool allow_multiple = false);
   void do_cache_delete_all_alts();
+  void do_cache_delete_compat_alts();
   void do_auth_callout();
   int  do_api_callout();
   int  do_api_callout_internal();

src/proxy/hdrs/URL.cc

@@ -1720,6 +1720,11 @@ memcpy_tolower(char *d, const char *s, int n)
 
 // fast path for CryptoHash, HTTP, no user/password/params/query,
 // no buffer overflow, no unescaping needed
+//
+// NOTE: this emits the ";" path/params separator, which matches
+// url_CryptoHash_get_general_92() but not url_CryptoHash_get_general(). It is
+// currently unreachable because url_hash_method is 0; enabling it would make
+// canonical keys collide with 9.2 keys.
 
 static inline void
 url_CryptoHash_get_fast(const URLImpl *url, CryptoContext &ctx, CryptoHash *hash, cache_generation_t generation)
@@ -1908,8 +1913,16 @@ url_CryptoHash_get_general_92(const URLImpl *url, CryptoContext &ctx, CryptoHash
   ends[7] = strs[7] + 1;
   ends[8] = strs[8] + url->m_len_path;
 
-  strs[9]  = ";";
-  strs[10] = url->m_ptr_params;
+  // ATS 9.2 split "/path;params" into separate path and params components and
+  // hashed them as path + ";" + params. That parsing was removed, so ";params"
+  // now stays inside the path and already spells the same byte sequence. Adding
+  // the separator again would append a ";" that 9.2 never emitted, so only add
+  // it when the path does not carry one. The params component itself is always
+  // empty now; it is left out rather than read back as an empty string.
+  bool const path_has_params = url->has_path_params();
+
+  strs[9]  = path_has_params ? nullptr : ";";
+  strs[10] = nullptr;
   strs[11] = "?";
 
   // Special case for the query paramters, allowing us to ignore them if requested
@@ -1921,8 +1934,8 @@ url_CryptoHash_get_general_92(const URLImpl *url, CryptoContext &ctx, CryptoHash
     ends[12] = nullptr;
   }
 
-  ends[9]  = strs[9] + 1;
-  ends[10] = strs[10] + url->m_len_params;
+  ends[9]  = path_has_params ? nullptr : strs[9] + 1;
+  ends[10] = nullptr;
   ends[11] = strs[11] + 1;
 
   p = buffer;
@@ -1979,7 +1992,7 @@ void
 url_CryptoHash_get_92(const URLImpl *url, CryptoHash *hash, bool ignore_query, cache_generation_t generation)
 {
   URLHashContext ctx;
-  if ((url_hash_method != 0) && (url->m_url_type == URLType::HTTP) &&
+  if ((url_hash_method != 0) && (url->m_url_type == URLType::HTTP) && !url->has_path_params() &&
       ((url->m_len_user + url->m_len_password + url->m_len_params + (ignore_query ? 0 : url->m_len_query)) == 0) &&
       (10u + url->m_len_scheme + url->m_len_host + url->m_len_path < BUFSIZE) &&
       (memchr(url->m_ptr_host, '%', url->m_len_host) == nullptr) && (memchr(url->m_ptr_path, '%', url->m_len_path) == nullptr)) {

src/proxy/hdrs/unit_tests/test_URL.cc

@@ -19,6 +19,7 @@
  */
 
 #include <cstdio>
+#include <memory>
 #include <vector>
 
 #include <catch2/catch_test_macros.hpp>
@@ -852,6 +853,106 @@ TEST_CASE("UrlPathGet", "[url][path_get]")
   }
 }
 
+// ATS 9.2 hashed "path" ";" "params" as separate cache-key components, always
+// emitting the separator. The params component was removed from the parser, so
+// ";params" now lives inside the path and the separator is already there. That
+// makes the 9.2 key expressible through the current algorithm: it is the
+// current key of the same URL with exactly one ";" between path and query.
+namespace
+{
+/// A failing REQUIRE unwinds out of these helpers, so the heap is released by
+/// scope exit rather than by a call that the unwind would skip.
+struct HdrHeapDeleter {
+  void
+  operator()(HdrHeap *heap) const
+  {
+    heap->destroy();
+  }
+};
+
+using HdrHeapPtr = std::unique_ptr<HdrHeap, HdrHeapDeleter>;
+
+CryptoHash
+hash92(char const *text)
+{
+  HdrHeapPtr heap{new_HdrHeap()};
+  URL        url;
+
+  url.create(heap.get());
+  REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE);
+
+  CryptoHash hash;
+
+  url.hash_get92(&hash);
+
+  return hash;
+}
+
+CryptoHash
+hash_current(char const *text)
+{
+  HdrHeapPtr heap{new_HdrHeap()};
+  URL        url;
+
+  url.create(heap.get());
+  REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE);
+
+  CryptoHash hash;
+
+  url.hash_get(&hash);
+
+  return hash;
+}
+
+bool
+has_params(char const *text)
+{
+  HdrHeapPtr heap{new_HdrHeap()};
+  URL        url;
+
+  url.create(heap.get());
+  REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE);
+
+  return url.has_path_params();
+}
+} // namespace
+
+TEST_CASE("UrlHashGet92 reproduces the 9.2 cache key", "[url][hash_get92]")
+{
+  SECTION("a path without params gains the 9.2 separator")
+  {
+    CHECK(hash92("http://foo.test/path") == hash_current("http://foo.test/path;"));
+    CHECK(hash92("http://foo.test/path?q=1") == hash_current("http://foo.test/path;?q=1"));
+    // A ';' inside the query is not a params segment: 9.2 stopped the params
+    // component at '?', so the path still gained a separator of its own.
+    CHECK(hash92("http://foo.test/a?b;c") == hash_current("http://foo.test/a;?b;c"));
+  }
+
+  SECTION("a path carrying params already spells the 9.2 key")
+  {
+    CHECK(hash92("http://foo.test/a;b=1") == hash_current("http://foo.test/a;b=1"));
+    CHECK(hash92("http://foo.test/a;b;c") == hash_current("http://foo.test/a;b;c"));
+    CHECK(hash92("http://foo.test/a;b=1?q=1") == hash_current("http://foo.test/a;b=1?q=1"));
+  }
+
+  SECTION("the keys diverge only when the path has no params segment")
+  {
+    // This divergence is the whole reason the compatibility lookup exists.
+    CHECK(hash92("http://foo.test/path") != hash_current("http://foo.test/path"));
+    CHECK(hash92("http://foo.test/path?q=1") != hash_current("http://foo.test/path?q=1"));
+    // ... and when it converges there is nothing for a second lookup to find.
+    CHECK(hash92("http://foo.test/a;b=1") == hash_current("http://foo.test/a;b=1"));
+  }
+
+  SECTION("has_path_params identifies the converging case")
+  {
+    CHECK(!has_params("http://foo.test/path"));
+    CHECK(!has_params("http://foo.test/a?b;c"));
+    CHECK(has_params("http://foo.test/a;b=1"));
+    CHECK(has_params("http://foo.test/a;b=1?q=1"));
+  }
+}
+
 // URL getters must not construct std::string_view from a nullptr pointer
 // (which is UB). Parts that are not present in the URL should return an
 // empty string_view with data() == nullptr.

src/proxy/http/HttpCacheSM.cc

@@ -294,7 +294,8 @@ HttpCacheSM::state_cache_open_write(int event, void *data)
       // than or equal to the max number of open write retries
       ink_assert(!write_retry_done());
 
-      open_write(&cache_key, lookup_url, read_request_hdr, master_sm->t_state.cache_info.object_read,
+      open_write(&cache_key, lookup_url, read_request_hdr,
+                 cache_write_info_for_lookup(master_sm->compatibility_cache_lookup, master_sm->t_state.cache_info.object_read),
                  static_cast<time_t>(
                    (master_sm->t_state.cache_control.pin_in_cache_for < 0) ? 0 : master_sm->t_state.cache_control.pin_in_cache_for),
                  retry_write, false);