Copilot commented on code in PR #13158: URL: https://github.com/apache/trafficserver/pull/13158#discussion_r3243715383
########## tests/gold_tests/headers/range_transform.test.py: ########## @@ -0,0 +1,18 @@ +# 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. + Review Comment: This new gold test doesn’t follow the common `headers/*.test.py` pattern of providing a module docstring and setting `Test.Summary` (and usually `Test.ContinueOnFail = True`). Adding those improves test reporting consistency (see e.g. tests/gold_tests/headers/range.test.py). ########## src/proxy/http/HttpSM.cc: ########## @@ -5091,6 +5091,35 @@ HttpSM::do_range_setup_if_necessary() if (t_state.cache_info.action == HttpTransact::CacheAction_t::REPLACE) { if (t_state.hdr_info.server_response.status_get() == HTTPStatus::OK) { Dbg(dbg_ctl_http_range, "Serving transform after stale cache re-serve"); + + // Ranges and range_output_cl were computed against the stale cached object size. If the fresh origin Content-Length + // differs, re-parse the Range against the fresh value so the outgoing Content-Length/Content-Range match the body + // actually being sent. Without this, Content-Length/Content-Range advertise the stale cached size. + const int64_t fresh_cl = t_state.hdr_info.server_response.get_content_length(); + const int64_t cached_cl = t_state.cache_info.object_read ? t_state.cache_info.object_read->object_size_get() : -1; + if (fresh_cl > 0 && fresh_cl != cached_cl) { + SMDbg(dbg_ctl_http_range, "Re-parsing range against fresh origin Content-Length %" PRId64 " (was %" PRId64 ")", + fresh_cl, cached_cl); + delete[] t_state.ranges; + t_state.ranges = nullptr; + t_state.num_range_fields = 0; + t_state.range_setup = HttpTransact::RangeSetup_t::NONE; + t_state.range_output_cl = 0; + parse_range_done = false; + + std::string_view content_type = + t_state.hdr_info.server_response.value_get(static_cast<std::string_view>(MIME_FIELD_CONTENT_TYPE)); + parse_range_and_compare(field, fresh_cl); + calculate_output_cl(content_type.length(), num_chars_for_int(fresh_cl)); + + if (t_state.range_setup != HttpTransact::RangeSetup_t::REQUESTED) { + // Re-parse yielded e.g. RANGE_NOT_SATISFIABLE (entire range + // past fresh body); let downstream handling take over without + // installing the transform. Review Comment: In the stale-revalidate re-parse block, returning early when the new parse result is NOT REQUESTED leaves the transaction without a transform hook and without building a 416 (or other) response for an unsatisfiable Range. In the REPLACE path, HttpTransact does not check range_setup after do_range_setup_if_necessary(), so this can silently fall back to serving the full 200 response to a Range request. Consider explicitly handling NOT_SATISFIABLE / NOT_HANDLED here (e.g., convert to a 416, or force the transaction onto an error path) rather than just returning. ########## src/proxy/http/HttpSM.cc: ########## @@ -5091,6 +5091,35 @@ HttpSM::do_range_setup_if_necessary() if (t_state.cache_info.action == HttpTransact::CacheAction_t::REPLACE) { if (t_state.hdr_info.server_response.status_get() == HTTPStatus::OK) { Dbg(dbg_ctl_http_range, "Serving transform after stale cache re-serve"); + + // Ranges and range_output_cl were computed against the stale cached object size. If the fresh origin Content-Length + // differs, re-parse the Range against the fresh value so the outgoing Content-Length/Content-Range match the body + // actually being sent. Without this, Content-Length/Content-Range advertise the stale cached size. + const int64_t fresh_cl = t_state.hdr_info.server_response.get_content_length(); + const int64_t cached_cl = t_state.cache_info.object_read ? t_state.cache_info.object_read->object_size_get() : -1; + if (fresh_cl > 0 && fresh_cl != cached_cl) { Review Comment: The re-parse is guarded by `fresh_cl > 0`, which skips the correction when the origin returns `Content-Length: 0` (valid for an empty 200 response). In that case, range_output_cl/ranges remain based on the stale cached size, and the transform path can generate inconsistent headers/body again. Consider triggering the re-parse for `fresh_cl >= 0` when Content-Length is present (and handling the `0` case appropriately, likely as NOT_SATISFIABLE for any non-empty Range). -- 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]
