From be432652846716bc2f7b24491a42bd81a0a027dc Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Tue, 1 Sep 2026 10:52:13 -0600 Subject: [PATCH 1/5] Fix the 92x compatibility cache key lookup and migration --- doc/admin-guide/files/records.yaml.en.rst | 18 ++ include/proxy/hdrs/URL.h | 24 ++ include/proxy/http/HttpSM.h | 20 +- src/proxy/hdrs/URL.cc | 23 +- src/proxy/hdrs/unit_tests/test_URL.cc | 88 ++++++ src/proxy/http/HttpCacheSM.cc | 10 +- src/proxy/http/HttpSM.cc | 101 ++++++- src/proxy/http/HttpTransact.cc | 9 + .../cache/compat-cache-key-write-fail.test.py | 23 ++ .../gold_tests/cache/compat-cache-key.test.py | 23 ++ .../compat-cache-key-write-fail.replay.yaml | 178 +++++++++++ .../cache/replay/compat-cache-key.replay.yaml | 286 ++++++++++++++++++ 12 files changed, 775 insertions(+), 28 deletions(-) create mode 100644 tests/gold_tests/cache/compat-cache-key-write-fail.test.py create mode 100644 tests/gold_tests/cache/compat-cache-key.test.py create mode 100644 tests/gold_tests/cache/replay/compat-cache-key-write-fail.replay.yaml create mode 100644 tests/gold_tests/cache/replay/compat-cache-key.replay.yaml diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index 5d75b68bafc..21e73123b60 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/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: diff --git a/include/proxy/hdrs/URL.h b/include/proxy/hdrs/URL.h index e8aeee0978d..ae8c5b9eec1 100644 --- a/include/proxy/hdrs/URL.h +++ b/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(); +} + /*------------------------------------------------------------------------- -------------------------------------------------------------------------*/ diff --git a/include/proxy/http/HttpSM.h b/include/proxy/http/HttpSM.h index c2128eeca20..2fe020647e7 100644 --- a/include/proxy/http/HttpSM.h +++ b/include/proxy/http/HttpSM.h @@ -35,8 +35,6 @@ #include #include -#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" @@ -344,6 +341,21 @@ class HttpSM : public Continuation, public PluginUserArgs void set_http_schedule(Continuation *); int get_http_schedule(int event, void *data); + static CacheHTTPInfo * + cache_write_info_for_lookup(CompatibilityCacheLookup lookup, CacheHTTPInfo *object_read_info) + { + if (lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92) { + return nullptr; + } + return object_read_info; + } + + static bool + should_use_compatibility_cache_key(CompatibilityCacheLookup lookup) + { + return lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92; + } + private: void start_sub_sm(); @@ -406,6 +418,7 @@ class HttpSM : public Continuation, public PluginUserArgs 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 +434,7 @@ class HttpSM : public Continuation, public PluginUserArgs 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(); diff --git a/src/proxy/hdrs/URL.cc b/src/proxy/hdrs/URL.cc index b0d0e5329b3..89aca8623cd 100644 --- a/src/proxy/hdrs/URL.cc +++ b/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)) { diff --git a/src/proxy/hdrs/unit_tests/test_URL.cc b/src/proxy/hdrs/unit_tests/test_URL.cc index f1963c9039f..ae44f804740 100644 --- a/src/proxy/hdrs/unit_tests/test_URL.cc +++ b/src/proxy/hdrs/unit_tests/test_URL.cc @@ -852,6 +852,94 @@ 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 +{ +CryptoHash +hash92(char const *text) +{ + URL url; + HdrHeap *heap = new_HdrHeap(); + url.create(heap); + REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); + + CryptoHash hash; + url.hash_get92(&hash); + heap->destroy(); + + return hash; +} + +CryptoHash +hash_current(char const *text) +{ + URL url; + HdrHeap *heap = new_HdrHeap(); + url.create(heap); + REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); + + CryptoHash hash; + url.hash_get(&hash); + heap->destroy(); + + return hash; +} + +bool +has_params(char const *text) +{ + URL url; + HdrHeap *heap = new_HdrHeap(); + url.create(heap); + REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); + + bool result = url.has_path_params(); + heap->destroy(); + + return result; +} +} // 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. diff --git a/src/proxy/http/HttpCacheSM.cc b/src/proxy/http/HttpCacheSM.cc index a6fe5e0cf91..19f34fde197 100644 --- a/src/proxy/http/HttpCacheSM.cc +++ b/src/proxy/http/HttpCacheSM.cc @@ -294,10 +294,12 @@ 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, - static_cast( - (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); + open_write( + &cache_key, lookup_url, read_request_hdr, + HttpSM::cache_write_info_for_lookup(master_sm->compatibility_cache_lookup, master_sm->t_state.cache_info.object_read), + static_cast( + (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); } break; diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index b7a123c6f62..bd947305e16 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -23,6 +23,7 @@ */ #include "proxy/http/HttpConfig.h" +#include "proxy/hdrs/HdrUtils.h" #include "tscore/ink_hrtime.h" #include "tscore/ink_time.h" #include "tsutil/Metrics.h" @@ -2766,8 +2767,11 @@ HttpSM::state_cache_open_read(int event, void *data) if (cache_sm.get_last_error() == -ECACHE_DOC_BUSY) { t_state.cache_lookup_result = HttpTransact::CacheLookupResult_t::DOC_BUSY; } else { + // A path that carries its own ";params" segment already hashes to the 9.2 + // key, so a compatibility lookup would just repeat the lookup that missed. if (t_state.http_config_param->cache_try_compat_key_read && - compatibility_cache_lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_NORMAL) { + compatibility_cache_lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_NORMAL && + !cache_lookup_url()->has_path_params()) { // do the retry compatibility_cache_lookup = CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92; do_cache_lookup_and_read(); @@ -5279,6 +5283,18 @@ HttpSM::do_range_setup_if_necessary() } } +// YTS Team, yamsat Plugin +// Changed the lookup_url to c_url which enables even +// the new redirect url to perform a CACHE_LOOKUP +URL * +HttpSM::cache_lookup_url() +{ + if (t_state.redirect_info.redirect_in_process && !t_state.txn_conf->redirect_use_orig_cache_key) { + return t_state.hdr_info.client_request.url_get(); + } + return t_state.cache_info.lookup_url; +} + void HttpSM::do_cache_lookup_and_read() { @@ -5295,20 +5311,12 @@ HttpSM::do_cache_lookup_and_read() milestones[TS_MILESTONE_CACHE_OPEN_READ_BEGIN] = ink_get_hrtime(); t_state.cache_lookup_result = HttpTransact::CacheLookupResult_t::NONE; t_state.cache_info.lookup_count++; - // YTS Team, yamsat Plugin - // Changed the lookup_url to c_url which enables even - // the new redirect url to perform a CACHE_LOOKUP - URL *c_url; - if (t_state.redirect_info.redirect_in_process && !t_state.txn_conf->redirect_use_orig_cache_key) { - c_url = t_state.hdr_info.client_request.url_get(); - } else { - c_url = t_state.cache_info.lookup_url; - } + URL *c_url = cache_lookup_url(); SMDbg(dbg_ctl_http_seq, "Issuing cache lookup for URL %s", c_url->string_get(&t_state.arena)); HttpCacheKey key; - if (compatibility_cache_lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92) { + if (should_use_compatibility_cache_key(compatibility_cache_lookup)) { Cache::generate_key92(&key, c_url, t_state.txn_conf->cache_ignore_query, t_state.txn_conf->cache_generation_number); } else { Cache::generate_key(&key, c_url, t_state.txn_conf->cache_ignore_query, t_state.txn_conf->cache_generation_number); @@ -5339,8 +5347,39 @@ HttpSM::do_cache_delete_all_alts() SMDbg(dbg_ctl_http_seq, "Issuing cache delete for %s", t_state.cache_info.lookup_url->string_get_ref()); HttpCacheKey key; - Cache::generate_key(&key, t_state.cache_info.lookup_url, t_state.txn_conf->cache_ignore_query, - t_state.txn_conf->cache_generation_number); + if (should_use_compatibility_cache_key(compatibility_cache_lookup)) { + Cache::generate_key92(&key, t_state.cache_info.lookup_url, t_state.txn_conf->cache_ignore_query, + t_state.txn_conf->cache_generation_number); + } else { + Cache::generate_key(&key, t_state.cache_info.lookup_url, t_state.txn_conf->cache_ignore_query, + t_state.txn_conf->cache_generation_number); + } + cacheProcessor.remove(nullptr, &key); +} + +// Remove the object stored under the legacy key. +// +// Only for the cases that abort the canonical-key write, where nothing is left +// depending on it. A successful migration deliberately leaves the legacy copy +// alone: VC_EVENT_WRITE_COMPLETE means the tunnel handed the last byte to the +// cache VC, not that the object reached disk, so deleting on that signal loses +// the object outright whenever the write later fails. The copy ages out on its +// own, and it stops being read as soon as the canonical key resolves, so the +// compat_key_reads metric still decays to zero. +void +HttpSM::do_cache_delete_compat_alts() +{ + ink_assert(should_use_compatibility_cache_key(compatibility_cache_lookup)); + + URL *url = t_state.cache_info.lookup_url; + + if (url == nullptr || !url->valid()) { + return; + } + SMDbg(dbg_ctl_http_seq, "Issuing compatibility cache delete for %s", url->string_get_ref()); + + HttpCacheKey key; + Cache::generate_key92(&key, url, t_state.txn_conf->cache_ignore_query, t_state.txn_conf->cache_generation_number); cacheProcessor.remove(nullptr, &key); } @@ -5419,8 +5458,15 @@ HttpSM::do_cache_prepare_action(HttpCacheSM *c_sm, CacheHTTPInfo *object_read_in HttpCacheKey key; Cache::generate_key(&key, s_url, t_state.txn_conf->cache_ignore_query, t_state.txn_conf->cache_generation_number); + // A compatibility read returns an object stored under the legacy key. Passing + // that object to a write using the canonical key turns the write into an + // update, but the canonical-key vector does not contain the legacy alternate. + // Cache::open_write then fails with ECACHE_NO_DOC instead of creating the + // migrated object. Create a new canonical-key object for compatibility reads. + CacheHTTPInfo *write_object_read_info = cache_write_info_for_lookup(compatibility_cache_lookup, object_read_info); + pending_action = - c_sm->open_write(&key, s_url, &t_state.hdr_info.cache_request, object_read_info, + c_sm->open_write(&key, s_url, &t_state.hdr_info.cache_request, write_object_read_info, static_cast((t_state.cache_control.pin_in_cache_for < 0) ? 0 : t_state.cache_control.pin_in_cache_for), retry, allow_multiple); } @@ -6816,8 +6862,15 @@ HttpSM::perform_cache_write_action() } case HttpTransact::CacheAction_t::DELETE: { - // Write close deletes the old alternate - cache_sm.close_write(); + if (should_use_compatibility_cache_key(compatibility_cache_lookup)) { + // Write close cannot remove the legacy alternate for the same reason an + // update cannot commit: this write VC never opened the legacy vector. + cache_sm.abort_write(); + do_cache_delete_compat_alts(); + } else { + // Write close deletes the old alternate + cache_sm.close_write(); + } cache_sm.close_read(); t_state.cache_info.write_lock_state = HttpTransact::CacheWriteLock_t::INIT; break; @@ -6875,6 +6928,18 @@ HttpSM::perform_cache_write_action() void HttpSM::issue_cache_update() { + if (should_use_compatibility_cache_key(compatibility_cache_lookup)) { + // This write VC is a create on the canonical key, not an update of the + // legacy vector, so CacheVC turns a header-only close into an abort and the + // update is silently lost. Drop the legacy object instead; the next request + // repopulates it under the canonical key. + SMDbg(dbg_ctl_http, "compatibility key hit, dropping the legacy object instead of updating it"); + cache_sm.abort_write(); + do_cache_delete_compat_alts(); + t_state.cache_info.write_lock_state = HttpTransact::CacheWriteLock_t::INIT; + return; + } + ink_assert(cache_sm.cache_write_vc != nullptr); if (cache_sm.cache_write_vc) { t_state.cache_info.object_store.request_sent_time_set(t_state.request_sent_time); @@ -8394,6 +8459,10 @@ HttpSM::set_next_state() case HttpTransact::StateMachineAction_t::CACHE_LOOKUP: { HTTP_SM_SET_DEFAULT_HANDLER(&HttpSM::state_cache_open_read); + // Every lookup starts from the canonical key. A redirect follow or a read + // retry looks up a different object than the one a previous compatibility + // lookup found, so the flag must not carry over. + compatibility_cache_lookup = CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_NORMAL; do_cache_lookup_and_read(); break; } diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index 8a3c601092a..3dd262eb08b 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/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"); + return; + } + // if the document is cached, just send a conditional request to the server // So the request does not have preconditions. It can, however diff --git a/tests/gold_tests/cache/compat-cache-key-write-fail.test.py b/tests/gold_tests/cache/compat-cache-key-write-fail.test.py new file mode 100644 index 00000000000..e9ce47772e1 --- /dev/null +++ b/tests/gold_tests/cache/compat-cache-key-write-fail.test.py @@ -0,0 +1,23 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = ''' +Verify that when the write migrating a 92x compatibility-key object to the +current cache key fails, the legacy object is left in place rather than +deleted, so the only remaining copy is not lost. +''' + +Test.ATSReplayTest(replay_file="replay/compat-cache-key-write-fail.replay.yaml") diff --git a/tests/gold_tests/cache/compat-cache-key.test.py b/tests/gold_tests/cache/compat-cache-key.test.py new file mode 100644 index 00000000000..178003d4a2c --- /dev/null +++ b/tests/gold_tests/cache/compat-cache-key.test.py @@ -0,0 +1,23 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = ''' +Verify proxy.config.http.cache.try_compat_key_read: objects stored under the +ATS 9.2 cache key are found, revalidated without conditional headers, migrated +to the current key, and then dropped from the legacy key. +''' + +Test.ATSReplayTest(replay_file="replay/compat-cache-key.replay.yaml") diff --git a/tests/gold_tests/cache/replay/compat-cache-key-write-fail.replay.yaml b/tests/gold_tests/cache/replay/compat-cache-key-write-fail.replay.yaml new file mode 100644 index 00000000000..c012e57f5bf --- /dev/null +++ b/tests/gold_tests/cache/replay/compat-cache-key-write-fail.replay.yaml @@ -0,0 +1,178 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +# Migrating a 9.2-key object to the current key must not remove the legacy copy, +# because there is no signal at this layer that says the new object reached disk. +# The tunnel's VC_EVENT_WRITE_COMPLETE only means the last byte was handed to the +# cache VC: openWriteMain delivers it before openWriteCloseHead performs the +# actual write. Deleting on that signal loses the object outright whenever the +# write subsequently fails. +# +# proxy.config.cache.max_doc_size reproduces exactly that ordering. It is checked +# in CacheVC::handleWrite, which for a single-fragment object runs at close time +# -- after the tunnel has already reported the write complete. +# +# As in compat-cache-key.replay.yaml, "/obj;" addresses the same key that ATS +# 9.2 produced for "/obj", which is how a legacy object gets into the cache. +# + +meta: + version: "1.0" + +autest: + description: 'Verify a failed 92x migration leaves the legacy object in place' + + dns: + name: 'dns-compat-cache-key-write-fail' + + server: + name: 'proxy-verifier-server' + + client: + name: 'proxy-verifier-client' + + ats: + name: 'ts-for-proxy-verifier' + process_config: + enable_cache: true + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'http|http_cache' + proxy.config.http.insert_age_in_response: 0 + proxy.config.http.cache.try_compat_key_read: 1 + # Small enough that the priming object caches but the revalidated one + # cannot. Note this also disables read-while-write, which this test does + # not rely on. + proxy.config.cache.max_doc_size: 512 + + remap_config: + - from: "http://example.com/" + to: "http://backend.example.com:{SERVER_HTTP_PORT}/" + + metric_checks: + # The migrating write really was attempted and really did fail. Without + # this the test could pass with the write never having been tried. + - metric: proxy.process.cache.write.backlog.failure + min: 1 + # Exactly one compatibility hit: the stale revalidation in test 2. Test 3 + # addresses the legacy key directly, so it is an ordinary lookup. + - metric: proxy.process.http.cache.compat_key_reads + value: 1 + +sessions: +- transactions: + + # + # Test 1: Prime the cache under the legacy key. + # + # 16 bytes is comfortably under max_doc_size, so this write succeeds. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /fail; + headers: + fields: + - [ uuid, 1-prime-legacy-key ] + - [ Host, example.com ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=2 ] + - [ ETag, '"legacy-object"' ] + + proxy-response: + status: 200 + + # + # Test 2: The migrating write fails after the tunnel reports it complete. + # + # The object is stale, so the compatibility hit revalidates unconditionally + # and the origin returns a body larger than max_doc_size. ATS opens the write + # on the current key, starts filling it, and the cache rejects it. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /fail + headers: + fields: + - [ uuid, 2-migration-write-fails ] + - [ Host, example.com ] + # Outlive the max-age=2 above so the object is stale. + delay: 3s + + proxy-request: + headers: + fields: + - [ If-None-Match, { as: absent } ] + - [ If-Modified-Since, { as: absent } ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 4096 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"too-big-to-cache"' ] + + # The client still gets the response; only the caching of it failed. + proxy-response: + status: 200 + + # + # Test 3: The legacy object survived. + # + # "/fail;" addresses the legacy key directly. The object there is stale by + # now, so it revalidates -- and it can only send a conditional header, with + # the original ETag, if it is still in the cache. A deleted object would be a + # plain miss with no If-None-Match at all. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /fail; + headers: + fields: + - [ uuid, 3-legacy-object-survived ] + - [ Host, example.com ] + delay: 100ms + + proxy-request: + headers: + fields: + - [ If-None-Match, { value: '"legacy-object"', as: equal } ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + + proxy-response: + status: 200 diff --git a/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml b/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml new file mode 100644 index 00000000000..4367e03ab5a --- /dev/null +++ b/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml @@ -0,0 +1,286 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +# ATS 9.2 hashed a cache key as path + ";" + params. The params segment was +# removed from the URL parser, so ";params" now lives inside the path and the +# separator is no longer added -- which is why every 9.2 object misses after an +# upgrade and why proxy.config.http.cache.try_compat_key_read exists. +# +# That same equivalence gives this test a way to create a 9.2-key object using +# only the running proxy: the 9.2 key of "/obj" is byte-identical to the current +# key of "/obj;". Priming the cache with "/obj;" therefore leaves an object that +# only a compatibility lookup for "/obj" can find. +# + +meta: + version: "1.0" + +autest: + description: 'Verify 92x compatibility cache lookups serve, migrate and drain' + + dns: + name: 'dns-compat-cache-key' + + server: + name: 'proxy-verifier-server' + + client: + name: 'proxy-verifier-client' + + ats: + name: 'ts-for-proxy-verifier' + process_config: + enable_cache: true + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'http|http_cache|url_cachekey' + proxy.config.http.insert_age_in_response: 0 + proxy.config.http.cache.try_compat_key_read: 1 + + remap_config: + - from: "http://example.com/" + to: "http://backend.example.com:{SERVER_HTTP_PORT}/" + + metric_checks: + # Two compatibility hits are expected: the fresh serve and the stale + # revalidation that migrates the object. + - metric: proxy.process.http.cache.compat_key_reads + value: 2 + +sessions: +- transactions: + + # + # Test 1: Prime the cache under the legacy key. + # + # "/migrate;" hashes to exactly the key ATS 9.2 produced for "/migrate", so + # after this transaction the cache holds what looks like a 9.2 object. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /migrate; + headers: + fields: + - [ uuid, 1-prime-legacy-key ] + - [ Host, example.com ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=2 ] + - [ ETag, '"legacy-object"' ] + + proxy-response: + status: 200 + + # + # Test 2: A fresh compatibility hit is served from cache. + # + # The canonical lookup for "/migrate" misses and the compatibility lookup + # finds the object primed above. The origin must not be contacted. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /migrate + headers: + fields: + - [ uuid, 2-compat-hit-fresh ] + - [ Host, example.com ] + delay: 100ms + + proxy-request: + expect: absent + + server-response: + status: 400 + reason: "Bad Request" + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 + + # + # Test 3: A stale compatibility hit revalidates unconditionally. + # + # A 304 cannot be applied to a legacy-key object, because the write that would + # carry it is a create on the canonical key rather than an update of the + # legacy vector. ATS must therefore ask for the whole response, and the origin + # must see no conditional headers even though the cached object has an ETag. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /migrate + headers: + fields: + - [ uuid, 3-compat-migrate ] + - [ Host, example.com ] + # Outlive the max-age=2 above so the object is stale. + delay: 3s + + proxy-request: + headers: + fields: + - [ If-None-Match, { as: absent } ] + - [ If-Modified-Since, { as: absent } ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"migrated-object"' ] + + proxy-response: + status: 200 + + # + # Test 4: The migrated object is now a canonical-key hit. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /migrate + headers: + fields: + - [ uuid, 4-canonical-hit ] + - [ Host, example.com ] + delay: 100ms + + proxy-request: + expect: absent + + server-response: + status: 400 + reason: "Bad Request" + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 + + # + # Test 5: The legacy copy is left in place. + # + # "/migrate;" addresses the legacy key directly. Migration does not delete it: + # nothing at this layer reports that the new object reached disk, so removing + # it here would lose the object whenever that write later failed. It ages out + # instead, and stops being read as soon as the current key resolves -- which + # is what lets compat_key_reads decay to zero. + # + # Still being in the cache is observable: the object is stale by now, so it + # revalidates conditionally with the ETag it was stored with. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /migrate; + headers: + fields: + - [ uuid, 5-legacy-key-drained ] + - [ Host, example.com ] + delay: 100ms + + proxy-request: + headers: + fields: + - [ If-None-Match, { value: '"legacy-object"', as: equal } ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + + proxy-response: + status: 200 + + # + # Test 6: A path that already carries ";params" gets no compatibility lookup. + # + # Its current key and its 9.2 key are the same string, so a second lookup + # would only repeat the one that just missed. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /keep;jsessionid=abc + headers: + fields: + - [ uuid, 6-params-path-miss ] + - [ Host, example.com ] + delay: 100ms + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + + proxy-response: + status: 200 + + # + # Test 7: ... and it still caches normally under the current key. + # + # The compat_key_reads count asserted above stays at 2, so neither of these + # two transactions took a compatibility path. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /keep;jsessionid=abc + headers: + fields: + - [ uuid, 7-params-path-hit ] + - [ Host, example.com ] + delay: 100ms + + proxy-request: + expect: absent + + server-response: + status: 400 + reason: "Bad Request" + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 From 0b7edf0acf6c3227e3797ded55fe797491a29932 Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Tue, 8 Sep 2026 07:11:25 -0600 Subject: [PATCH 2/5] fixes for copilot reviews --- include/proxy/http/HttpSM.h | 14 +++++++------- src/proxy/http/HttpSM.cc | 6 +++--- tests/gold_tests/cache/compat-cache-key.test.py | 5 +++-- .../cache/replay/compat-cache-key.replay.yaml | 4 ++-- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/include/proxy/http/HttpSM.h b/include/proxy/http/HttpSM.h index 2fe020647e7..65e094710fd 100644 --- a/include/proxy/http/HttpSM.h +++ b/include/proxy/http/HttpSM.h @@ -341,21 +341,21 @@ class HttpSM : public Continuation, public PluginUserArgs 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 (lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92) { + if (should_use_compatibility_cache_key(lookup)) { return nullptr; } return object_read_info; } - static bool - should_use_compatibility_cache_key(CompatibilityCacheLookup lookup) - { - return lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92; - } - private: void start_sub_sm(); diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index bd947305e16..5e46e43bbb3 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -5283,9 +5283,9 @@ HttpSM::do_range_setup_if_necessary() } } -// YTS Team, yamsat Plugin -// Changed the lookup_url to c_url which enables even -// the new redirect url to perform a CACHE_LOOKUP +// The URL this transaction looks up in the cache. A redirect follow looks up the +// redirected URL rather than the original, unless the transaction is configured +// to keep the original cache key. URL * HttpSM::cache_lookup_url() { diff --git a/tests/gold_tests/cache/compat-cache-key.test.py b/tests/gold_tests/cache/compat-cache-key.test.py index 178003d4a2c..ba638a34b75 100644 --- a/tests/gold_tests/cache/compat-cache-key.test.py +++ b/tests/gold_tests/cache/compat-cache-key.test.py @@ -16,8 +16,9 @@ Test.Summary = ''' Verify proxy.config.http.cache.try_compat_key_read: objects stored under the -ATS 9.2 cache key are found, revalidated without conditional headers, migrated -to the current key, and then dropped from the legacy key. +ATS 9.2 cache key are found, revalidated without conditional headers, and +migrated to the current key, while the copy under the legacy key is left in +place to age out. ''' Test.ATSReplayTest(replay_file="replay/compat-cache-key.replay.yaml") diff --git a/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml b/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml index 4367e03ab5a..7cdcb239556 100644 --- a/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml +++ b/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml @@ -30,7 +30,7 @@ meta: version: "1.0" autest: - description: 'Verify 92x compatibility cache lookups serve, migrate and drain' + description: 'Verify 92x compatibility cache lookups serve and migrate to the current key' dns: name: 'dns-compat-cache-key' @@ -207,7 +207,7 @@ sessions: url: /migrate; headers: fields: - - [ uuid, 5-legacy-key-drained ] + - [ uuid, 5-legacy-key-retained ] - [ Host, example.com ] delay: 100ms From d1080e24a9c6d47188bd8640aef7a3fead01de4d Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Tue, 8 Sep 2026 09:19:19 -0600 Subject: [PATCH 3/5] strip ims headers if stale 92x compatible key used --- include/proxy/http/HttpSM.h | 31 ++-- src/proxy/http/HttpCacheSM.cc | 11 +- src/proxy/http/HttpTransact.cc | 9 +- ...ompat-cache-key-client-conditional.test.py | 23 +++ ...t-cache-key-client-conditional.replay.yaml | 165 ++++++++++++++++++ 5 files changed, 217 insertions(+), 22 deletions(-) create mode 100644 tests/gold_tests/cache/compat-cache-key-client-conditional.test.py create mode 100644 tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml diff --git a/include/proxy/http/HttpSM.h b/include/proxy/http/HttpSM.h index 65e094710fd..0351e976898 100644 --- a/include/proxy/http/HttpSM.h +++ b/include/proxy/http/HttpSM.h @@ -179,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 { friend class HttpTransact; @@ -341,21 +357,6 @@ class HttpSM : public Continuation, public PluginUserArgs 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; - } - private: void start_sub_sm(); diff --git a/src/proxy/http/HttpCacheSM.cc b/src/proxy/http/HttpCacheSM.cc index 19f34fde197..3648d44405c 100644 --- a/src/proxy/http/HttpCacheSM.cc +++ b/src/proxy/http/HttpCacheSM.cc @@ -294,12 +294,11 @@ 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, - HttpSM::cache_write_info_for_lookup(master_sm->compatibility_cache_lookup, master_sm->t_state.cache_info.object_read), - static_cast( - (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); + 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( + (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); } break; diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index 3dd262eb08b..1d40e89a92a 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -2586,8 +2586,15 @@ HttpTransact::issue_revalidate(State *s) // 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)) { + if (s->state_machine != nullptr && should_use_compatibility_cache_key(s->state_machine->compatibility_cache_lookup)) { + // build_request() already strips the client's conditionals for a request it + // expects to cache, but keeps them when the request does not look cacheable + // or when cache_when_to_revalidate is 4. Either way the origin could answer + // 304, so drop them here too. The client still gets its 304: a conditional + // client request is matched against the full response in + // handle_cache_operation_on_forward_server_response(). TxnDbg(dbg_ctl_http_trans, "compatibility key hit, revalidating without conditional headers"); + HttpTransactHeaders::remove_conditional_headers(&s->hdr_info.server_request); return; } diff --git a/tests/gold_tests/cache/compat-cache-key-client-conditional.test.py b/tests/gold_tests/cache/compat-cache-key-client-conditional.test.py new file mode 100644 index 00000000000..ad98e2392ff --- /dev/null +++ b/tests/gold_tests/cache/compat-cache-key-client-conditional.test.py @@ -0,0 +1,23 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = ''' +Verify that a compatibility hit strips the client's own conditional headers +before revalidating, in the configurations where build_request() forwards them, +so the origin cannot answer with a 304 the transaction is unable to apply. +''' + +Test.ATSReplayTest(replay_file="replay/compat-cache-key-client-conditional.replay.yaml") diff --git a/tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml b/tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml new file mode 100644 index 00000000000..17d67bdc204 --- /dev/null +++ b/tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml @@ -0,0 +1,165 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +# +# A compatibility-key object cannot consume a 304, so it has to be revalidated +# with no conditional headers at all. build_request() already removes the +# client's conditionals for a request it expects to cache, but it deliberately +# forwards them in two cases: when the request does not look cacheable, and when +# proxy.config.http.cache.when_to_revalidate is 4. In those the origin could +# still answer 304, so issue_revalidate() drops them for itself. +# +# This exercises the when_to_revalidate case, which is a global switch and so +# needs a TS of its own rather than sharing one with compat-cache-key. +# +# As in compat-cache-key.replay.yaml, "/cc;" addresses the same key ATS 9.2 +# produced for "/cc", which is how a legacy object gets into the cache. +# + +meta: + version: "1.0" + +autest: + description: 'Verify a compatibility hit strips client-supplied conditional headers' + + dns: + name: 'dns-compat-cache-key-client-conditional' + + server: + name: 'proxy-verifier-server' + + client: + name: 'proxy-verifier-client' + + ats: + name: 'ts-for-proxy-verifier' + process_config: + enable_cache: true + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'http|http_cache' + proxy.config.http.insert_age_in_response: 0 + proxy.config.http.cache.try_compat_key_read: 1 + # Forward the client's conditionals rather than stripping them. + proxy.config.http.cache.when_to_revalidate: 4 + + remap_config: + - from: "http://example.com/" + to: "http://backend.example.com:{SERVER_HTTP_PORT}/" + + metric_checks: + # One compatibility hit: the stale revalidation in test 2. + - metric: proxy.process.http.cache.compat_key_reads + value: 1 + +sessions: +- transactions: + + # + # Test 1: Prime the cache under the legacy key. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /cc; + headers: + fields: + - [ uuid, 1-prime-legacy-key ] + - [ Host, example.com ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=1 ] + - [ ETag, '"legacy-object"' ] + + proxy-response: + status: 200 + + # + # Test 2: The client's own conditional is stripped for the migration. + # + # The object is stale, so this is a compatibility hit that revalidates. The + # client sent an ETag of its own and when_to_revalidate 4 would forward it, + # which would let the origin answer 304 -- a response this transaction cannot + # apply to a legacy-key object. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /cc + headers: + fields: + - [ uuid, 2-client-conditional-stripped ] + - [ Host, example.com ] + - [ If-None-Match, '"something-the-client-has"' ] + # Outlive the max-age=1 above so the object is stale. + delay: 2s + + proxy-request: + headers: + fields: + - [ If-None-Match, { as: absent } ] + - [ If-Modified-Since, { as: absent } ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"migrated-object"' ] + + proxy-response: + status: 200 + + # + # Test 3: The object really did migrate to the current key. + # + # Had the origin been allowed to answer 304, the write would have been + # aborted and this would go back to the origin instead. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /cc + headers: + fields: + - [ uuid, 3-migrated-to-current-key ] + - [ Host, example.com ] + delay: 100ms + + proxy-request: + expect: absent + + server-response: + status: 400 + reason: "Bad Request" + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 From d672cd3f8f8085f0d1bb7b667ced8043e5a3e4d9 Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Tue, 8 Sep 2026 10:31:21 -0600 Subject: [PATCH 4/5] fix for cache key in redirect case, make refresh test more robust --- src/proxy/http/HttpSM.cc | 16 ++++++++++------ ...mpat-cache-key-client-conditional.replay.yaml | 9 ++++++--- .../compat-cache-key-write-fail.replay.yaml | 12 +++++++++--- .../cache/replay/compat-cache-key.replay.yaml | 14 +++++++++++--- 4 files changed, 36 insertions(+), 15 deletions(-) diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 5e46e43bbb3..ffd2047deb5 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -5344,15 +5344,18 @@ HttpSM::do_cache_delete_all_alts() // Do not delete a non-existent object. ink_assert(t_state.cache_info.object_read); - SMDbg(dbg_ctl_http_seq, "Issuing cache delete for %s", t_state.cache_info.lookup_url->string_get_ref()); + // Address the object that was looked up. A redirect follow can look up a + // different URL than cache_info.lookup_url, which is set once and does not + // track the redirect when the pristine host header is maintained. + URL *url = cache_lookup_url(); + + SMDbg(dbg_ctl_http_seq, "Issuing cache delete for %s", url->string_get_ref()); HttpCacheKey key; if (should_use_compatibility_cache_key(compatibility_cache_lookup)) { - Cache::generate_key92(&key, t_state.cache_info.lookup_url, t_state.txn_conf->cache_ignore_query, - t_state.txn_conf->cache_generation_number); + Cache::generate_key92(&key, url, t_state.txn_conf->cache_ignore_query, t_state.txn_conf->cache_generation_number); } else { - Cache::generate_key(&key, t_state.cache_info.lookup_url, t_state.txn_conf->cache_ignore_query, - t_state.txn_conf->cache_generation_number); + Cache::generate_key(&key, url, t_state.txn_conf->cache_ignore_query, t_state.txn_conf->cache_generation_number); } cacheProcessor.remove(nullptr, &key); } @@ -5371,7 +5374,8 @@ HttpSM::do_cache_delete_compat_alts() { ink_assert(should_use_compatibility_cache_key(compatibility_cache_lookup)); - URL *url = t_state.cache_info.lookup_url; + // Same URL the lookup used; see do_cache_delete_all_alts(). + URL *url = cache_lookup_url(); if (url == nullptr || !url->valid()) { return; diff --git a/tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml b/tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml index 17d67bdc204..34c8293b230 100644 --- a/tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml +++ b/tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml @@ -89,7 +89,7 @@ sessions: headers: fields: - [ Content-Length, 16 ] - - [ Cache-Control, max-age=1 ] + - [ Cache-Control, max-age=300 ] - [ ETag, '"legacy-object"' ] proxy-response: @@ -113,8 +113,11 @@ sessions: - [ uuid, 2-client-conditional-stripped ] - [ Host, example.com ] - [ If-None-Match, '"something-the-client-has"' ] - # Outlive the max-age=1 above so the object is stale. - delay: 2s + # when_to_revalidate 4 is "stale if IMS", so this both forces the + # revalidation and supplies a second conditional to be stripped. No + # waiting for the object to age out. + - [ If-Modified-Since, 'Tue, 01 Jan 2030 00:00:00 GMT' ] + delay: 100ms proxy-request: headers: diff --git a/tests/gold_tests/cache/replay/compat-cache-key-write-fail.replay.yaml b/tests/gold_tests/cache/replay/compat-cache-key-write-fail.replay.yaml index c012e57f5bf..04ce81369ae 100644 --- a/tests/gold_tests/cache/replay/compat-cache-key-write-fail.replay.yaml +++ b/tests/gold_tests/cache/replay/compat-cache-key-write-fail.replay.yaml @@ -55,6 +55,9 @@ autest: proxy.config.diags.debug.tags: 'http|http_cache' proxy.config.http.insert_age_in_response: 0 proxy.config.http.cache.try_compat_key_read: 1 + # Honour Cache-Control in the client request, which is how these + # transactions force revalidation without waiting for an object to age. + proxy.config.http.cache.ignore_client_cc_max_age: 0 # Small enough that the priming object caches but the revalidated one # cannot. Note this also disables read-while-write, which this test does # not rely on. @@ -98,7 +101,7 @@ sessions: headers: fields: - [ Content-Length, 16 ] - - [ Cache-Control, max-age=2 ] + - [ Cache-Control, max-age=300 ] - [ ETag, '"legacy-object"' ] proxy-response: @@ -120,8 +123,9 @@ sessions: fields: - [ uuid, 2-migration-write-fails ] - [ Host, example.com ] - # Outlive the max-age=2 above so the object is stale. - delay: 3s + # Ask for a revalidation rather than sleeping until the object ages out. + - [ Cache-Control, max-age=0 ] + delay: 100ms proxy-request: headers: @@ -159,6 +163,8 @@ sessions: fields: - [ uuid, 3-legacy-object-survived ] - [ Host, example.com ] + # Force the revalidation that reveals the stored ETag. + - [ Cache-Control, max-age=0 ] delay: 100ms proxy-request: diff --git a/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml b/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml index 7cdcb239556..cac094b255e 100644 --- a/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml +++ b/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml @@ -51,6 +51,9 @@ autest: proxy.config.diags.debug.tags: 'http|http_cache|url_cachekey' proxy.config.http.insert_age_in_response: 0 proxy.config.http.cache.try_compat_key_read: 1 + # Honour Cache-Control in the client request, which is how these + # transactions force revalidation without waiting for an object to age. + proxy.config.http.cache.ignore_client_cc_max_age: 0 remap_config: - from: "http://example.com/" @@ -87,7 +90,7 @@ sessions: headers: fields: - [ Content-Length, 16 ] - - [ Cache-Control, max-age=2 ] + - [ Cache-Control, max-age=300 ] - [ ETag, '"legacy-object"' ] proxy-response: @@ -140,8 +143,11 @@ sessions: fields: - [ uuid, 3-compat-migrate ] - [ Host, example.com ] - # Outlive the max-age=2 above so the object is stale. - delay: 3s + # Ask for a revalidation rather than sleeping until the object ages out. + # Freshness is decided before any of the compatibility handling, so the + # path under test is the same either way. + - [ Cache-Control, max-age=0 ] + delay: 100ms proxy-request: headers: @@ -209,6 +215,8 @@ sessions: fields: - [ uuid, 5-legacy-key-retained ] - [ Host, example.com ] + # Force the revalidation that reveals the stored ETag. + - [ Cache-Control, max-age=0 ] delay: 100ms proxy-request: From ad83634856aa509c407889f540fe46dcbb7098ac Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Tue, 8 Sep 2026 10:37:14 -0600 Subject: [PATCH 5/5] add raii cleanup for test_url unit test --- src/proxy/hdrs/unit_tests/test_URL.cc | 43 +++++++++++++++++---------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/src/proxy/hdrs/unit_tests/test_URL.cc b/src/proxy/hdrs/unit_tests/test_URL.cc index ae44f804740..feee1ded603 100644 --- a/src/proxy/hdrs/unit_tests/test_URL.cc +++ b/src/proxy/hdrs/unit_tests/test_URL.cc @@ -19,6 +19,7 @@ */ #include +#include #include #include @@ -859,17 +860,30 @@ TEST_CASE("UrlPathGet", "[url][path_get]") // 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; + CryptoHash hash92(char const *text) { - URL url; - HdrHeap *heap = new_HdrHeap(); - url.create(heap); + HdrHeapPtr heap{new_HdrHeap()}; + URL url; + + url.create(heap.get()); REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); CryptoHash hash; + url.hash_get92(&hash); - heap->destroy(); return hash; } @@ -877,14 +891,15 @@ hash92(char const *text) CryptoHash hash_current(char const *text) { - URL url; - HdrHeap *heap = new_HdrHeap(); - url.create(heap); + HdrHeapPtr heap{new_HdrHeap()}; + URL url; + + url.create(heap.get()); REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); CryptoHash hash; + url.hash_get(&hash); - heap->destroy(); return hash; } @@ -892,15 +907,13 @@ hash_current(char const *text) bool has_params(char const *text) { - URL url; - HdrHeap *heap = new_HdrHeap(); - url.create(heap); - REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); + HdrHeapPtr heap{new_HdrHeap()}; + URL url; - bool result = url.has_path_params(); - heap->destroy(); + url.create(heap.get()); + REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); - return result; + return url.has_path_params(); } } // namespace