Fix 92x compatibility cache revalidation

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

@@ -2795,11 +2795,39 @@ Cache Control
    using the newest key generation. This might be temporarily necessary
    if a large cache was created by the previous version of ATS but the new
    version changed the way cache keys are generated.  If this is turned on,
-   a metric called `proxy.process.http.cache.compat_key_reads` will be
+   a metric called ``proxy.process.http.cache.compat_key_reads`` will be
    incremented any time the compat cache lookup successfully finds the object.
    You can monitor this metric and know when its safe to turn this feature off
    as the cache wraps around.
 
+   Three 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. Every ``DELETE`` and ``PURGE`` issues a second
+   remove under the previous key, because a migrated object exists under both
+   keys until the old copy ages out. And an object found under the previous
+   key is not revalidated when it goes stale, 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. A stale one is treated as a
+   miss instead, so the origin returns the full response, which is stored
+   under the new key, and the copy under the previous key is left to age out.
+   The client's own conditional headers are not forwarded on that request,
+   even when :ts:cv:`proxy.config.http.cache.when_to_revalidate` is ``4``,
+   since a ``304`` from the origin would leave the object unmigrated. The
+   client still receives a ``304`` if its conditions match the full response.
+   Each object pays this once, but on a large cache the aggregate is a
+   bandwidth event worth sizing before enabling the setting in production.
+
+   For the same reason a plugin cannot modify such an object in place.
+   ``TSHttpTxnUpdateCachedObject`` still returns ``TS_SUCCESS``, but when the
+   update is later prepared it is refused the same way an invalid update is,
+   and the transaction is answered with a ``500`` response rather than the
+   cached object.
+
+   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

@@ -28,6 +28,7 @@
 #include "proxy/hdrs/HdrHeap.h"
 #include "tscore/CryptoHash.h"
 #include "proxy/hdrs/MIME.h"
+#include <cstring>
 #include <string_view>
 
 #include "tscore/ink_apidefs.h"
@@ -101,6 +102,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 +269,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 +497,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,28 @@ enum class CompatibilityCacheLookup {
   COMPAT_CACHE_LAST,
 };
 
+/// Policy for an object found under the previous (9.2) cache key. Shared by the
+/// HTTP state machine, its cache sub-machine and HttpTransact, which is why it
+/// is not a member of any of them.
+namespace CompatCacheKey
+{
+/// Whether this lookup addresses the cache with the previous (9.2) key.
+inline bool
+is_legacy(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 *
+write_info(CompatibilityCacheLookup lookup, CacheHTTPInfo *object_read_info)
+{
+  return is_legacy(lookup) ? nullptr : object_read_info;
+}
+} // namespace CompatCacheKey
+
 class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
 {
   friend class HttpTransact;
@@ -406,6 +425,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 +441,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,8 @@
  */
 
 #include <cstdio>
+#include <cstring>
+#include <memory>
 #include <vector>
 
 #include <catch2/catch_test_macros.hpp>
@@ -852,6 +854,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,
+                 CompatCacheKey::write_info(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);