Copilot commented on code in PR #13869:
URL: https://github.com/apache/trafficserver/pull/13869#discussion_r4332593068
##########
tests/uranium_tests/cache/test_proxy_serve_stale_dns_fail.py:
##########
@@ -65,10 +84,13 @@ def test_proxy_serve_stale_dns_fail(ats_factory:
ATSFactory, curl: Curl, service
f'{{curl}} -X PUSH -d "{stale_5}"
"http://localhost:{child.http_port}";;'
f'{{curl}} -X PUSH -d "{stale_10}"
"http://localhost:{parent.http_port}";;'
f"sleep 7; {{curl}} -s -v http://localhost:{child.http_port};";
- f"sleep 17; {{curl}} -s -v http://localhost:{child.http_port};";
+ # Serving stale through negative revalidation rewrites the cached
copy, which
+ # restarts its age. Wait until it is past max-age again, so it is too
old only
+ # if the rewrite kept the max-age rather than taking the new Expires.
+ f"sleep 7; {{curl}} -s -v {narrow} http://localhost:{child.http_port};";
f'{{curl_base}} -X PUSH -d "{stale_5}"
"http://localhost:{parent.http_port}";;'
f"sleep 7; {{curl_base}} -s -v http://localhost:{parent.http_port};";
- f"sleep 17; {{curl_base}} -s -v http://localhost:{parent.http_port};";)
+ f"{{curl_base}} -s -v {narrow} http://localhost:{parent.http_port};";)
Review Comment:
The last parent request no longer has a delay before the `{narrow}` fetch
(whereas the child path does). This is likely correct, but it’s non-obvious
when reading the script; add a brief comment explaining why no additional sleep
is needed here (e.g., because the parent-side flow doesn’t reset the cached
copy’s age the way the child path does).
##########
tests/uranium_tests/cache/test_proxy_serve_stale_dns_fail.py:
##########
@@ -28,6 +28,22 @@ def test_proxy_serve_stale_dns_fail(ats_factory: ATSFactory,
curl: Curl, service
"""
_SERVER_NAME = "http://unknown.domain.com/";
+ # A stale copy may be served for max_stale_age seconds past its max-age.
The
+ # checks that expect a stale copy to be served get a window far longer
than the
+ # test runs, so a slow machine cannot push them past it. The checks that
expect a
+ # copy to be too old send X-Stale-Window: narrow, which shrinks the window
to one
+ # second for that request only, so a delay only makes them more certain.
The
+ # header does not change the cache key, so both kinds of request read the
same
+ # cached copy. Only the localhost maps load the rule, so the narrow window
applies
+ # on the proxy the client talks to directly, not on the parent the child
forwards
+ # to.
+ wide_stale_age = 300
+ narrow_stale_age = 1
+ stale_window_rules = (
+ "cond %{REMAP_PSEUDO_HOOK}\n"
+ "cond %{CLIENT-HEADER:X-Stale-Window} =narrow\n"
+ f"set-config proxy.config.http.cache.max_stale_age
{narrow_stale_age}\n")
Review Comment:
`header_rewrite` condition lines are clearer/less error-prone when they use
an explicit comparison. Consider changing `cond %{REMAP_PSEUDO_HOOK}` to an
explicit check (e.g., `cond %{REMAP_PSEUDO_HOOK} =1`) so the rule’s intent
doesn’t depend on truthiness semantics in the parser.
--
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]
