This is an automated email from the ASF dual-hosted git repository.
bneradt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 6a96d5056d Preserve the cache action while dispatching cache write
events (#13487)
6a96d5056d is described below
commit 6a96d5056debcb5499496fb8ede73fcf901398d0
Author: Brian Neradt <[email protected]>
AuthorDate: Thu Aug 6 10:38:05 2026 -0500
Preserve the cache action while dispatching cache write events (#13487)
A transaction that loses the cache write lock and schedules a retry
hands HttpSM a reusable captive action owned by HttpCacheSM. When the
retry fires, HttpSM::state_cache_open_write() assigns the result of
adjust_thread() to pending_action before releasing that delivered
action. The callback is normally already on the correct thread, so
adjust_thread() returns nullptr, and assigning nullptr to a
PendingAction cancels whatever it was holding. The transaction thereby
cancels its own captive action, and the cache read that the retry
immediately issues comes back on an action already marked cancelled.
Debug builds abort on the resulting assertion in HttpCacheSM, which is
how this was found in production; release builds instead take the
cancelled early return, drop a valid cache callback, and stall the
transaction until it times out.
This patch clears the delivered action before the thread adjustment
rather than after it. Clearing first is safe because the cache action
has already called back, and it means a genuine reschedule installs its
event as the new pending action instead of canceling a captive action
that is still in use.
This also adds an autest in which two transactions contend for the
cache write lock with read-while-writer disabled, so the loser's write
retry delivers a synchronous cache read callback. That test aborts
reliably on an unpatched debug build.
---
src/proxy/http/HttpSM.cc | 8 +-
.../cache/cache-write-retry-callback.test.py | 25 ++++
.../replay/cache-write-retry-callback.replay.yaml | 134 +++++++++++++++++++++
3 files changed, 165 insertions(+), 2 deletions(-)
diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc
index 64173d0f3c..1ffa2d8b5b 100644
--- a/src/proxy/http/HttpSM.cc
+++ b/src/proxy/http/HttpSM.cc
@@ -2533,6 +2533,12 @@ HttpSM::state_cache_open_write(int event, void *data)
{
STATE_ENTER(state_cache_open_write, event);
+ // The cache action has already delivered this callback, so drop it before
any
+ // thread adjustment below can assign over it. Assigning to pending_action
+ // cancels whatever it holds, and canceling the cache SM's reusable captive
+ // action here would break every cache operation this transaction makes
later.
+ pending_action.clear_if_action_is(reinterpret_cast<Action *>(data));
+
// Make sure we are on the "right" thread
if (_ua.get_txn()) {
pending_action = _ua.get_txn()->adjust_thread(this, event, data);
@@ -2544,8 +2550,6 @@ HttpSM::state_cache_open_write(int event, void *data)
ink_release_assert(vc && vc->thread == this_ethread());
}
- pending_action.clear_if_action_is(reinterpret_cast<Action *>(data));
-
ATS_PROBE1(milestone_cache_open_write_end, sm_id);
milestones[TS_MILESTONE_CACHE_OPEN_WRITE_END] = ink_get_hrtime();
pending_action = nullptr;
diff --git a/tests/gold_tests/cache/cache-write-retry-callback.test.py
b/tests/gold_tests/cache/cache-write-retry-callback.test.py
new file mode 100644
index 0000000000..28e3e2ec64
--- /dev/null
+++ b/tests/gold_tests/cache/cache-write-retry-callback.test.py
@@ -0,0 +1,25 @@
+'''
+Verify a cache write retry does not cancel the cache read that follows it.
+'''
+# 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 a transaction that loses the cache write lock and retries can still
+receive the cache read callback that its retry triggers.
+'''
+
+Test.ATSReplayTest(replay_file="replay/cache-write-retry-callback.replay.yaml")
diff --git
a/tests/gold_tests/cache/replay/cache-write-retry-callback.replay.yaml
b/tests/gold_tests/cache/replay/cache-write-retry-callback.replay.yaml
new file mode 100644
index 0000000000..9e3627803b
--- /dev/null
+++ b/tests/gold_tests/cache/replay/cache-write-retry-callback.replay.yaml
@@ -0,0 +1,134 @@
+# 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.
+#
+# Two transactions request the same uncached object. The first takes the cache
+# write lock and holds it while its origin response is outstanding. The second
+# loses the write lock, and its scheduled retry hands a cache read event back
to
+# HttpSM. HttpSM used to cancel its own cache action while dispatching that
+# event, which aborted debug builds on the canceled action assertion in
+# HttpCacheSM and dropped the callback in release builds.
+#
+# Read-while-writer is disabled so that the contending cache read fails
+# immediately rather than waiting for the writer, which is what makes the
+# callback arrive synchronously from within the write retry.
+
+meta:
+ version: "1.0"
+
+autest:
+ description: 'Verify a cache write retry preserves the cache read callback
that follows it'
+ dns:
+ name: 'dns-write-retry-callback'
+
+ server:
+ name: 'origin-write-retry-callback'
+
+ client:
+ name: 'client-write-retry-callback'
+ process_config:
+ # The two sessions below have to overlap in time, so the client must not
+ # serialize them onto a single thread.
+ other_args: '--thread-limit 4'
+
+ ats:
+ name: 'ts-write-retry-callback'
+ process_config:
+ enable_cache: true
+
+ records_config:
+ proxy.config.diags.debug.enabled: 1
+ proxy.config.diags.debug.tags: 'http_cache|http_trans'
+ # READ_RETRY: retry the cache read when the write lock is lost.
+ proxy.config.http.cache.open_write_fail_action: 5
+ proxy.config.http.cache.max_open_write_retries: 1
+ proxy.config.http.cache.max_open_write_retry_timeout: 0
+ proxy.config.http.cache.max_open_read_retries: 2
+ proxy.config.http.cache.open_read_retry_time: 250
+ proxy.config.cache.enable_read_while_writer: 0
+
+ remap_config:
+ - from: "http://example.com/";
+ to: "http://backend.example.com:{SERVER_HTTP_PORT}/";
+
+ log_validation:
+ traffic_out:
+ excludes:
+ - expression: "[Ff]atal|failed assertion"
+ description: "Verify ATS does not abort while retrying the cache
write"
+ contains:
+ - expression: "falling back to read retry"
+ description: "Verify the contending transaction retried its cache
read after losing the write lock"
+ - expression: "READ_RETRY cache read failed, bypassing cache"
+ description: "Verify the retried cache read was delivered and
handled"
+
+sessions:
+ # Take the cache write lock and hold it for the duration of this slow origin
+ # response.
+ - transactions:
+ - client-request:
+ method: GET
+ version: '1.1'
+ url: /contended-object
+ headers:
+ fields:
+ - [uuid, cache-writer]
+ - [Host, example.com]
+
+ server-response:
+ delay: 3s
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [Content-Length, 16]
+ - [Cache-Control, "max-age=300"]
+ - [X-Response, writer]
+
+ proxy-response:
+ status: 200
+ headers:
+ fields:
+ - [X-Response, {value: writer, as: equal}]
+
+ # Lose the cache write lock to the session above. Both write attempts fail
+ # while that writer owns the lock, so the scheduled write retry delivers a
+ # cache read event to HttpSM, which issues another cache read. That read also
+ # fails, and the transaction proxies to the origin without caching.
+ - transactions:
+ - client-request:
+ delay: 200ms
+ method: GET
+ version: '1.1'
+ url: /contended-object
+ headers:
+ fields:
+ - [uuid, cache-contender]
+ - [Host, example.com]
+
+ server-response:
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [Content-Length, 16]
+ - [Cache-Control, "max-age=300"]
+ - [X-Response, contender]
+
+ proxy-response:
+ status: 200
+ headers:
+ fields:
+ - [X-Response, {value: contender, as: equal}]
