Copilot commented on code in PR #13611:
URL: https://github.com/apache/trafficserver/pull/13611#discussion_r3959757871
##########
src/proxy/http/HttpSM.cc:
##########
@@ -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);
Review Comment:
`do_cache_lookup_and_read()` now uses `cache_lookup_url()` to handle
redirect-follow cache lookups, but both `do_cache_delete_all_alts()` and
`do_cache_delete_compat_alts()` still generate keys from
`t_state.cache_info.lookup_url`. If `redirect_in_process` is true and the
transaction is configured to use the redirected URL as the cache key, these
deletes can target a different cache entry than the one actually looked up. Use
`cache_lookup_url()` (or the same URL selection logic) for key generation and
logging in these delete paths as well.
##########
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;
Review Comment:
The helper functions allocate `HdrHeap*` and call `REQUIRE(...)` before
`heap->destroy()`. If the `REQUIRE` fails (even unexpectedly), the test will
exit the section early and leak the heap. Prefer an RAII guard (e.g., a smart
pointer with a custom deleter or a small local wrapper) so `destroy()` is
guaranteed to run on all paths.
##########
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 and migrate to
the current key'
+
+ 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
Review Comment:
These replay tests introduce multi-second real-time sleeps to force
staleness, which increases overall suite runtime and can increase flakiness on
slow/loaded CI. Consider avoiding wall-clock delays by forcing revalidation via
request headers (e.g., client-sent cache directives that require revalidation)
or by using response headers that make the object immediately stale at the time
of the follow-up transaction, so the same behavior is exercised without
sleeping.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
