From be432652846716bc2f7b24491a42bd81a0a027dc Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Tue, 1 Sep 2026 10:52:13 -0600 Subject: [PATCH 1/9] 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/9] 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/9] 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/9] 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/9] 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 From 873fed1d8c3138ec9c49091f31ae35193a8e84d8 Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Mon, 14 Sep 2026 16:11:56 -0600 Subject: [PATCH 6/9] simplify to be more minimal --- doc/admin-guide/files/records.yaml.en.rst | 20 +- src/proxy/http/HttpSM.cc | 62 +++--- src/proxy/http/HttpTransact.cc | 32 +-- ...ompat-cache-key-client-conditional.test.py | 23 --- .../cache/compat-cache-key-write-fail.test.py | 23 --- .../gold_tests/cache/compat-cache-key.test.py | 6 +- ...t-cache-key-client-conditional.replay.yaml | 168 ---------------- .../compat-cache-key-write-fail.replay.yaml | 184 ------------------ .../cache/replay/compat-cache-key.replay.yaml | 179 ++++++++++++++++- 9 files changed, 231 insertions(+), 466 deletions(-) delete mode 100644 tests/gold_tests/cache/compat-cache-key-client-conditional.test.py delete mode 100644 tests/gold_tests/cache/compat-cache-key-write-fail.test.py delete mode 100644 tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml delete mode 100644 tests/gold_tests/cache/replay/compat-cache-key-write-fail.replay.yaml diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index 21e73123b60..286fc33db4c 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -2802,16 +2802,16 @@ Cache Control 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. + load for the duration. 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. 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 + ``TSHttpTxnUpdateCachedObject`` fails for such an object rather than + updating it. Objects whose path contains a ``;`` are unaffected. The previous algorithm hashed the path and the deprecated ``;params`` segment as separate diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index ffd2047deb5..42e75763f2b 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -2777,6 +2777,9 @@ HttpSM::state_cache_open_read(int event, void *data) do_cache_lookup_and_read(); return 0; } + // Nothing was found under either key. Leaving the flag set would make + // the rest of the transaction act as though a legacy object were in play. + compatibility_cache_lookup = CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_NORMAL; t_state.cache_lookup_result = HttpTransact::CacheLookupResult_t::MISS; } @@ -5352,28 +5355,30 @@ HttpSM::do_cache_delete_all_alts() 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, url, t_state.txn_conf->cache_ignore_query, t_state.txn_conf->cache_generation_number); - } else { - Cache::generate_key(&key, 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); + + // A migration leaves the legacy copy in place, so the object can live under + // both keys. Removing only one of them would let the other be served after + // the purge. + if (t_state.http_config_param->cache_try_compat_key_read) { + do_cache_delete_compat_alts(); + } } // 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. +// depending on it, and for deletes, which have to reach both keys. 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 compat_key_reads still decays to +// zero. void HttpSM::do_cache_delete_compat_alts() { - ink_assert(should_use_compatibility_cache_key(compatibility_cache_lookup)); - // Same URL the lookup used; see do_cache_delete_all_alts(). URL *url = cache_lookup_url(); @@ -5408,10 +5413,15 @@ HttpSM::do_cache_prepare_write_transform() void HttpSM::do_cache_prepare_update() { + // An object found under the 9.2 key cannot be updated in place: the write + // would be a create on the current key, and the cache turns a header-only + // close of a create into an abort. Refuse the way an invalid update is + // refused rather than let the plugin's change vanish. if (t_state.cache_info.object_read != nullptr && t_state.cache_info.object_read->valid() && t_state.cache_info.object_store.valid() && t_state.cache_info.object_store.response_get() != nullptr && t_state.cache_info.object_store.response_get()->valid() && - t_state.hdr_info.client_request.method_get_wksidx() == HTTP_WKSIDX_GET) { + t_state.hdr_info.client_request.method_get_wksidx() == HTTP_WKSIDX_GET && + !should_use_compatibility_cache_key(compatibility_cache_lookup)) { t_state.cache_info.object_store.request_set(t_state.cache_info.object_read->request_get()); // t_state.cache_info.object_read = NULL; // cache_sm.close_read(); @@ -6866,14 +6876,12 @@ HttpSM::perform_cache_write_action() } case HttpTransact::CacheAction_t::DELETE: { - 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(); + // Write close deletes the old alternate + cache_sm.close_write(); + // That reached only one of the two keys the object can live under while + // the compatibility lookup is enabled. + if (t_state.http_config_param->cache_try_compat_key_read) { 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; @@ -6932,18 +6940,6 @@ 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); diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index 1d40e89a92a..f1d0a2db36f 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -2582,22 +2582,6 @@ 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 && 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; - } - // if the document is cached, just send a conditional request to the server // So the request does not have preconditions. It can, however @@ -2740,6 +2724,22 @@ HttpTransact::HandleCacheOpenReadHitFreshness(State *s) s->cache_lookup_result = HttpTransact::CacheLookupResult_t::HIT_STALE; } + // An object found under the 9.2 key cannot be revalidated: the write that + // would apply a 304 is a create on the current key, not an update of the + // legacy vector, so the cache discards it. Treat a stale one as a miss. The + // full response then lands under the current key through the ordinary miss + // path and the legacy copy ages out. Only reads migrate; the methods that + // invalidate take the delete path, which reaches both keys. + if (s->cache_lookup_result == HttpTransact::CacheLookupResult_t::HIT_STALE && s->state_machine != nullptr && + should_use_compatibility_cache_key(s->state_machine->compatibility_cache_lookup) && + (s->method == HTTP_WKSIDX_GET || s->method == HTTP_WKSIDX_HEAD)) { + TxnDbg(dbg_ctl_http_seq, "Stale under the compatibility key, treating as a miss"); + s->cache_info.object_read = nullptr; + s->cache_lookup_result = HttpTransact::CacheLookupResult_t::MISS; + s->cache_lookup_complete_deferred = false; + TRANSACT_RETURN(StateMachineAction_t::API_CACHE_LOOKUP_COMPLETE, HttpTransact::HandleCacheOpenReadMiss); + } + ink_assert(s->cache_lookup_result != HttpTransact::CacheLookupResult_t::MISS); if (s->cache_lookup_result == HttpTransact::CacheLookupResult_t::HIT_STALE) { SET_VIA_STRING(VIA_DETAIL_CACHE_LOOKUP, VIA_DETAIL_MISS_EXPIRED); 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 deleted file mode 100644 index ad98e2392ff..00000000000 --- a/tests/gold_tests/cache/compat-cache-key-client-conditional.test.py +++ /dev/null @@ -1,23 +0,0 @@ -# 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/compat-cache-key-write-fail.test.py b/tests/gold_tests/cache/compat-cache-key-write-fail.test.py deleted file mode 100644 index e9ce47772e1..00000000000 --- a/tests/gold_tests/cache/compat-cache-key-write-fail.test.py +++ /dev/null @@ -1,23 +0,0 @@ -# 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 index ba638a34b75..080c739b861 100644 --- a/tests/gold_tests/cache/compat-cache-key.test.py +++ b/tests/gold_tests/cache/compat-cache-key.test.py @@ -16,9 +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, and -migrated to the current key, while the copy under the legacy key is left in -place to age out. +ATS 9.2 cache key are served while fresh, treated as a miss once stale so the +full response is stored under the current key, and left in place under the +legacy key to age out. Deletes reach both keys. ''' Test.ATSReplayTest(replay_file="replay/compat-cache-key.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 deleted file mode 100644 index 34c8293b230..00000000000 --- a/tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml +++ /dev/null @@ -1,168 +0,0 @@ -# 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=300 ] - - [ 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"' ] - # 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: - 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 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 deleted file mode 100644 index 04ce81369ae..00000000000 --- a/tests/gold_tests/cache/replay/compat-cache-key-write-fail.replay.yaml +++ /dev/null @@ -1,184 +0,0 @@ -# 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 - # 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. - 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=300 ] - - [ 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 ] - # Ask for a revalidation rather than sleeping until the object ages out. - - [ Cache-Control, max-age=0 ] - delay: 100ms - - 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 ] - # Force the revalidation that reveals the stored ETag. - - [ Cache-Control, max-age=0 ] - 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 index cac094b255e..2ed336b4fb2 100644 --- a/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml +++ b/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml @@ -60,10 +60,10 @@ autest: 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. + # Four compatibility hits: the fresh serve, the stale revalidation that + # migrates the object, the PUT in test 8 and the migration in test 10. - metric: proxy.process.http.cache.compat_key_reads - value: 2 + value: 4 sessions: - transactions: @@ -127,12 +127,14 @@ sessions: status: 200 # - # Test 3: A stale compatibility hit revalidates unconditionally. + # Test 3: A stale compatibility hit is treated as a miss. # # 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. + # legacy vector. ATS therefore does not revalidate it at all: the request goes + # to the origin as an ordinary miss, so no conditional headers appear even + # though the cached object has an ETag, and the full response is stored under + # the canonical key. # - client-request: method: "GET" @@ -292,3 +294,168 @@ sessions: proxy-response: status: 200 + + # + # Test 8: a PUT keeps the client's precondition. + # + # Only GET and HEAD are treated as a miss when stale; a PUT takes the delete + # path with its request untouched. If-Match is the client's concurrency guard: + # dropping it would let the origin overwrite whatever is there now rather than + # only the version the client saw. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /put; + headers: + fields: + - [ uuid, 8a-prime-put-target ] + - [ Host, example.com ] + delay: 100ms + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"v1"' ] + + proxy-response: + status: 200 + + - client-request: + method: "PUT" + version: "1.1" + scheme: "http" + url: /put + headers: + fields: + - [ uuid, 8b-put-keeps-if-match ] + - [ Host, example.com ] + - [ Content-Length, 0 ] + - [ If-Match, '"v1"' ] + delay: 100ms + + proxy-request: + headers: + fields: + - [ If-Match, { value: '"v1"', as: equal } ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 + + # + # Test 9-11: a purge has to reach both keys. + # + # After a migration the object exists under both the current and the legacy + # key. Removing only the one the purge looked up would leave the other to be + # served afterwards, which for a purge is the whole point of the operation. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /purge; + headers: + fields: + - [ uuid, 9-prime-purge-target ] + - [ Host, example.com ] + delay: 100ms + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"legacy-purge"' ] + + proxy-response: + status: 200 + + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /purge + headers: + fields: + - [ uuid, 10-migrate-purge-target ] + - [ Host, example.com ] + - [ Cache-Control, max-age=0 ] + delay: 100ms + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"migrated-purge"' ] + + proxy-response: + status: 200 + + - client-request: + method: "PURGE" + version: "1.1" + scheme: "http" + url: /purge + headers: + fields: + - [ uuid, 11a-purge ] + - [ Host, example.com ] + - [ Content-Length, 0 ] + delay: 100ms + + # A purge is answered from the cache and never reaches the origin. + proxy-request: + expect: absent + + server-response: + status: 400 + reason: "Bad Request" + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 + + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /purge + headers: + fields: + - [ uuid, 11b-purged-content-is-gone ] + - [ Host, example.com ] + delay: 100ms + + # Reaching the origin at all is the assertion: a legacy copy that survived + # the purge is still fresh, so it would have been served from cache and the + # origin would never have been contacted. + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"refetched"' ] + + proxy-response: + status: 200 From 4c979aa4d4b840a48c8181fd73889298db240280 Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Mon, 14 Sep 2026 16:36:28 -0600 Subject: [PATCH 7/9] Scope the compatibility cache-key helpers and document the lookup URL --- include/proxy/http/HttpSM.h | 12 +++++++++--- src/proxy/http/HttpCacheSM.cc | 2 +- src/proxy/http/HttpSM.cc | 22 ++++++++++++++++------ src/proxy/http/HttpTransact.cc | 2 +- 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/include/proxy/http/HttpSM.h b/include/proxy/http/HttpSM.h index 0351e976898..9945273c768 100644 --- a/include/proxy/http/HttpSM.h +++ b/include/proxy/http/HttpSM.h @@ -179,9 +179,14 @@ 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 -should_use_compatibility_cache_key(CompatibilityCacheLookup lookup) +is_legacy(CompatibilityCacheLookup lookup) { return lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92; } @@ -190,10 +195,11 @@ should_use_compatibility_cache_key(CompatibilityCacheLookup lookup) /// 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) +write_info(CompatibilityCacheLookup lookup, CacheHTTPInfo *object_read_info) { - return should_use_compatibility_cache_key(lookup) ? nullptr : object_read_info; + return is_legacy(lookup) ? nullptr : object_read_info; } +} // namespace CompatCacheKey class HttpSM : public Continuation, public PluginUserArgs { diff --git a/src/proxy/http/HttpCacheSM.cc b/src/proxy/http/HttpCacheSM.cc index 3648d44405c..8055ca9b6ce 100644 --- a/src/proxy/http/HttpCacheSM.cc +++ b/src/proxy/http/HttpCacheSM.cc @@ -295,7 +295,7 @@ HttpCacheSM::state_cache_open_write(int event, void *data) ink_assert(!write_retry_done()); 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), + CompatCacheKey::write_info(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); diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 42e75763f2b..db279a17da8 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -5286,12 +5286,22 @@ HttpSM::do_range_setup_if_necessary() } } -// 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. +// The URL this transaction looks up in the cache, and that every delete which +// has to match that lookup uses too. +// +// hdr_info.client_request's URL always names the *current* request: +// redirect_request() rewrites it in place, so after a redirect follow it is the +// Location target. cache_info.lookup_url is set once, before the first lookup +// (HttpTransact::DecideCacheLookup), and names the *original* request. Whether +// it also moves with a redirect is an accident of how it was set: with +// pristine_host_hdr off it aliases client_request's URL and follows it, with +// pristine_host_hdr on it is a private copy and stays put. It is therefore not +// a reliable way to reach the redirected URL, which is why the redirect case +// below reads client_request instead. URL * HttpSM::cache_lookup_url() { + // Follow the redirect unless configured to keep the original cache key. 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(); } @@ -5319,7 +5329,7 @@ HttpSM::do_cache_lookup_and_read() SMDbg(dbg_ctl_http_seq, "Issuing cache lookup for URL %s", c_url->string_get(&t_state.arena)); HttpCacheKey key; - if (should_use_compatibility_cache_key(compatibility_cache_lookup)) { + if (CompatCacheKey::is_legacy(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); @@ -5421,7 +5431,7 @@ HttpSM::do_cache_prepare_update() t_state.cache_info.object_store.valid() && t_state.cache_info.object_store.response_get() != nullptr && t_state.cache_info.object_store.response_get()->valid() && t_state.hdr_info.client_request.method_get_wksidx() == HTTP_WKSIDX_GET && - !should_use_compatibility_cache_key(compatibility_cache_lookup)) { + !CompatCacheKey::is_legacy(compatibility_cache_lookup)) { t_state.cache_info.object_store.request_set(t_state.cache_info.object_read->request_get()); // t_state.cache_info.object_read = NULL; // cache_sm.close_read(); @@ -5477,7 +5487,7 @@ HttpSM::do_cache_prepare_action(HttpCacheSM *c_sm, CacheHTTPInfo *object_read_in // 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); + CacheHTTPInfo *write_object_read_info = CompatCacheKey::write_info(compatibility_cache_lookup, object_read_info); pending_action = c_sm->open_write(&key, s_url, &t_state.hdr_info.cache_request, write_object_read_info, diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index f1d0a2db36f..78702584df4 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -2731,7 +2731,7 @@ HttpTransact::HandleCacheOpenReadHitFreshness(State *s) // path and the legacy copy ages out. Only reads migrate; the methods that // invalidate take the delete path, which reaches both keys. if (s->cache_lookup_result == HttpTransact::CacheLookupResult_t::HIT_STALE && s->state_machine != nullptr && - should_use_compatibility_cache_key(s->state_machine->compatibility_cache_lookup) && + CompatCacheKey::is_legacy(s->state_machine->compatibility_cache_lookup) && (s->method == HTTP_WKSIDX_GET || s->method == HTTP_WKSIDX_HEAD)) { TxnDbg(dbg_ctl_http_seq, "Stale under the compatibility key, treating as a miss"); s->cache_info.object_read = nullptr; From 88624d60dfd9159fb762a16001febb2b4fc96c2f Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Tue, 15 Sep 2026 06:57:27 -0600 Subject: [PATCH 8/9] review feedback, includes, etc --- doc/admin-guide/files/records.yaml.en.rst | 30 +++++++++++-------- include/proxy/hdrs/URL.h | 1 + src/proxy/hdrs/unit_tests/test_URL.cc | 1 + src/proxy/http/HttpSM.cc | 16 ++++++---- .../cache/replay/compat-cache-key.replay.yaml | 5 ++-- 5 files changed, 34 insertions(+), 19 deletions(-) diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index 286fc33db4c..530dcdd50a1 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -2795,23 +2795,29 @@ 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. - Two costs come with enabling this. Every cache miss performs a second + 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. 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. 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 - ``TSHttpTxnUpdateCachedObject`` fails for such an object rather than - updating it. + 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. + 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 diff --git a/include/proxy/hdrs/URL.h b/include/proxy/hdrs/URL.h index ae8c5b9eec1..eebf2011df0 100644 --- a/include/proxy/hdrs/URL.h +++ b/include/proxy/hdrs/URL.h @@ -28,6 +28,7 @@ #include "proxy/hdrs/HdrHeap.h" #include "tscore/CryptoHash.h" #include "proxy/hdrs/MIME.h" +#include #include #include "tscore/ink_apidefs.h" diff --git a/src/proxy/hdrs/unit_tests/test_URL.cc b/src/proxy/hdrs/unit_tests/test_URL.cc index feee1ded603..3f3219eab69 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 diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index db279a17da8..3e9fb32bdff 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -5321,10 +5321,14 @@ HttpSM::do_cache_lookup_and_read() Metrics::Counter::increment(http_rsb.cache_lookups); ATS_PROBE1(milestone_cache_open_read_begin, sm_id); - 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++; - URL *c_url = cache_lookup_url(); + // A compatibility retry continues the lookup that just missed rather than + // starting a new one, so it keeps the original begin time and count. + if (!CompatCacheKey::is_legacy(compatibility_cache_lookup)) { + milestones[TS_MILESTONE_CACHE_OPEN_READ_BEGIN] = ink_get_hrtime(); + t_state.cache_info.lookup_count++; + } + t_state.cache_lookup_result = HttpTransact::CacheLookupResult_t::NONE; + URL *c_url = cache_lookup_url(); SMDbg(dbg_ctl_http_seq, "Issuing cache lookup for URL %s", c_url->string_get(&t_state.arena)); @@ -5392,7 +5396,9 @@ HttpSM::do_cache_delete_compat_alts() // Same URL the lookup used; see do_cache_delete_all_alts(). URL *url = cache_lookup_url(); - if (url == nullptr || !url->valid()) { + // A path that carries its own ";params" segment hashes to the same key + // under both schemes, so the canonical delete already reached it. + if (url == nullptr || !url->valid() || url->has_path_params()) { return; } SMDbg(dbg_ctl_http_seq, "Issuing compatibility cache delete for %s", url->string_get_ref()); 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 2ed336b4fb2..99082cce5e5 100644 --- a/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml +++ b/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml @@ -268,8 +268,9 @@ sessions: # # 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. + # Neither of these two transactions takes a compatibility path, so the + # running compat_key_reads count is still 2 after them. The remaining two + # of the four asserted in metric_checks come from tests 8 and 10. # - client-request: method: "GET" From 1d100a6bd08eae63614ac516b008d08299a27f93 Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Tue, 15 Sep 2026 09:58:30 -0600 Subject: [PATCH 9/9] Strip client conditionals when migrating a stale legacy object --- doc/admin-guide/files/records.yaml.en.rst | 4 + src/proxy/http/HttpSM.cc | 10 +- src/proxy/http/HttpTransact.cc | 11 + ...ompat-cache-key-client-conditional.test.py | 24 ++ ...t-cache-key-client-conditional.replay.yaml | 227 ++++++++++++++++++ 5 files changed, 269 insertions(+), 7 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/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index 530dcdd50a1..09c143372ad 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -2810,6 +2810,10 @@ Cache Control 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. diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 3e9fb32bdff..7fd1cf68252 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -5375,9 +5375,7 @@ HttpSM::do_cache_delete_all_alts() // A migration leaves the legacy copy in place, so the object can live under // both keys. Removing only one of them would let the other be served after // the purge. - if (t_state.http_config_param->cache_try_compat_key_read) { - do_cache_delete_compat_alts(); - } + do_cache_delete_compat_alts(); } // Remove the object stored under the legacy key. @@ -5398,7 +5396,7 @@ HttpSM::do_cache_delete_compat_alts() // A path that carries its own ";params" segment hashes to the same key // under both schemes, so the canonical delete already reached it. - if (url == nullptr || !url->valid() || url->has_path_params()) { + if (!t_state.http_config_param->cache_try_compat_key_read || url->has_path_params()) { return; } SMDbg(dbg_ctl_http_seq, "Issuing compatibility cache delete for %s", url->string_get_ref()); @@ -6896,9 +6894,7 @@ HttpSM::perform_cache_write_action() cache_sm.close_write(); // That reached only one of the two keys the object can live under while // the compatibility lookup is enabled. - if (t_state.http_config_param->cache_try_compat_key_read) { - do_cache_delete_compat_alts(); - } + do_cache_delete_compat_alts(); cache_sm.close_read(); t_state.cache_info.write_lock_state = HttpTransact::CacheWriteLock_t::INIT; break; diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index 78702584df4..bd9b0d56afe 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -8283,6 +8283,17 @@ HttpTransact::build_request(State *s, HTTPHdr *base_request, HTTPHdr *outgoing_r // instead of the normal non-conditional request. TxnDbg(dbg_ctl_http_trans, "request not like cacheable and conditional headers not removed"); } + + // A stale object under the 9.2 key is fetched as a miss so the full + // response can be stored under the current key. A 304 would leave it + // unmigrated, so drop the client's conditionals even in the two cases + // above that keep them. They are still matched against the full response, + // so the client gets its 304. A miss that will not be written keeps them. + if (s->cache_lookup_result == CacheLookupResult_t::MISS && s->cache_info.action != CacheAction_t::NO_ACTION && + s->state_machine != nullptr && CompatCacheKey::is_legacy(s->state_machine->compatibility_cache_lookup)) { + TxnDbg(dbg_ctl_http_trans, "legacy key object fetched as a miss, conditional headers removed"); + HttpTransactHeaders::remove_conditional_headers(outgoing_request); + } } if (s->hdr_info.client_request.m_100_continue_sent) { 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..c632e721de5 --- /dev/null +++ b/tests/gold_tests/cache/compat-cache-key-client-conditional.test.py @@ -0,0 +1,24 @@ +# 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 stale compatibility hit, which is fetched as a miss, strips the +client's own conditional headers in the configurations where build_request() +forwards them, so the origin cannot answer with a 304 that would leave the +object unmigrated. A miss that will not be written, such as a HEAD, keeps them. +''' + +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..4e7ad943d05 --- /dev/null +++ b/tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml @@ -0,0 +1,227 @@ +# 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 once stale it is fetched +# from the origin as a miss and the full response is stored under the current +# key. 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, which would leave the object unmigrated, so build_request() +# drops them whenever the response is going to be stored. A miss that will not +# be written, such as a HEAD, keeps them: there is nothing to migrate. +# +# 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 client-supplied conditional headers on a stale compatibility hit' + + 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: + # Two compatibility hits: the stale object fetched as a miss in test 2 + # and the stale HEAD in test 5. + - metric: proxy.process.http.cache.compat_key_reads + value: 2 + +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 compatibility hit is fetched as a miss. 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, nothing would have been stored + # under the current key 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 + + # + # Test 4-5: a HEAD keeps the client's conditional. + # + # A stale legacy object is fetched as a miss for HEAD too, but a HEAD response + # is never written to the cache, so there is nothing to migrate and no reason + # to give up the 304. The conditional headers are forwarded as they are. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /head; + headers: + fields: + - [ uuid, 4-prime-head-target ] + - [ Host, example.com ] + delay: 100ms + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=1 ] + - [ ETag, '"head-object"' ] + + proxy-response: + status: 200 + + - client-request: + method: "HEAD" + version: "1.1" + scheme: "http" + url: /head + headers: + fields: + - [ uuid, 5-head-keeps-conditional ] + - [ Host, example.com ] + - [ If-None-Match, '"head-object"' ] + # Outlive the max-age=1 above so the object is stale. + delay: 2s + + proxy-request: + headers: + fields: + - [ If-None-Match, { value: '"head-object"', as: equal } ] + + server-response: + status: 304 + reason: "Not Modified" + headers: + fields: + - [ ETag, '"head-object"' ] + + proxy-response: + status: 304