Copilot commented on code in PR #13083: URL: https://github.com/apache/trafficserver/pull/13083#discussion_r3108366064
########## tests/gold_tests/cache/replay/proxy_serve_stale_origin_down.replay.yaml: ########## @@ -0,0 +1,166 @@ +# 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. + +# +# This replay file assumes that caching is enabled and +# proxy.config.http.cache.ignore_client_cc_max_age is set to 0 so that we can +# test max-age in the client requests. +# + +meta: + version: "1.0" + +autest: + description: 'Verify that stale content is served when the origin server is down' + + dns: + name: 'dns-serve-stale-origin-down' + records: + backend.example.com: ["127.0.0.1"] + + + server: + name: 'server-serve-stale-origin-down' + + client: + name: 'client-serve-stale-origin-down' + + ats: + name: 'ts-serve-stale-origin-down' + process_config: + enable_cache: true + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'cache|http|dns|hostdb' + proxy.config.http.cache.max_stale_age: 10 + proxy.config.http.connect.down.policy: 4 + proxy.config.http.connect_attempts_timeout: 1 + proxy.config.http.connect_attempts_rr_retries: 0 + proxy.config.http.connect_attempts_max_retries: 1 + proxy.config.http.connect_attempts_max_retries_down_server: 0 + proxy.config.http.down_server.cache_time: 5 + + remap_config: + - from: "http://example.com/"; + to: "http://backend.example.com:{SERVER_HTTP_PORT}/"; + + plugin_config: + - 'xdebug.so --enable=x-cache,x-cache-key,via' + +sessions: +- transactions: + # 1. Cache object + # cache-key: /path/a/ + # cache: miss + # origin server: up + - client-request: + method: "GET" + version: "1.1" + url: /path/a/ + headers: + fields: + - [ Host, example.com ] + - [ x-debug, "x-cache,x-cache-key,via" ] + - [ uuid, a-1 ] + + server-response: + status: 200 + headers: + fields: + - [ Content-Length, 16] + - [ Cache-Control, "public, max-age=1"] + + proxy-response: + status: 200 + headers: + fields: + - [ X-Cache, { value: miss, as: equal } ] + + # 2. Make the origin server down + # cache-key: /path/b/ + # cache: miss + # origin server: down + - client-request: + method: "GET" + version: "1.1" + url: /path/b/ + headers: + fields: + - [ Host, example.com ] + - [ x-debug, "x-cache,x-cache-key,via" ] + - [ uuid, b-1 ] + + server-response: + on_connect: reset + + proxy-response: + status: 502 + headers: + fields: + - [ X-Cache, { value: miss, as: equal } ] + + # 3. Serve stale while down_serve.cache_time Review Comment: The comment refers to `down_serve.cache_time`, but the configured record is `proxy.config.http.down_server.cache_time`. This looks like a typo and can be confusing when debugging the test; please update the comment to the correct record name. ```suggestion # 3. Serve stale while proxy.config.http.down_server.cache_time ``` ########## doc/admin-guide/files/records.yaml.en.rst: ########## @@ -1948,6 +1948,9 @@ Origin Server Connect Attempts :overridable: Specifies how long (in seconds) |TS| remembers that an origin server was unreachable. + During this window, if a stale cached response exists and its age is within + :ts:cv:`proxy.config.http.cache.max_stale_age`, |TS| serves the stale content directly + without attempting to contact the origin server. Review Comment: The new description for `proxy.config.http.down_server.cache_time` says stale is served if the cached response age is within `proxy.config.http.cache.max_stale_age`, but the actual eligibility check allows staleness up to `cache_max_stale_age + response max-age` (see `HttpTransact::is_stale_cache_response_returnable`). Please adjust the wording to match the implemented age calculation (and consider noting that other response directives like `must-revalidate`/`no-cache` also prevent serving stale). ```suggestion During this window, if a stale cached response exists and its age is within the cached response's ``max-age`` plus :ts:cv:`proxy.config.http.cache.max_stale_age`, |TS| serves the stale content directly without attempting to contact the origin server, unless response directives such as ``must-revalidate`` or ``no-cache`` prohibit serving stale content. ``` ########## tests/gold_tests/cache/replay/proxy_serve_stale_origin_down.replay.yaml: ########## @@ -0,0 +1,166 @@ +# 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. + +# +# This replay file assumes that caching is enabled and +# proxy.config.http.cache.ignore_client_cc_max_age is set to 0 so that we can +# test max-age in the client requests. Review Comment: The header comment says this replay assumes `proxy.config.http.cache.ignore_client_cc_max_age` is set to 0, but this replay’s `records_config` doesn’t set it (and the default is `1`). Either set the record explicitly here or adjust/remove the comment so the test assumptions match the configured environment. -- 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]
