This is an automated email from the ASF dual-hosted git repository.
cmcfarlen pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/10.1.x by this push:
new ab46ac3634 Fix redirected cache write without write VC (#13309)
(#13336)
ab46ac3634 is described below
commit ab46ac3634ef2d4e58fcda7250e67c817020a0f0
Author: Brian Neradt <[email protected]>
AuthorDate: Mon Jul 6 16:09:24 2026 -0400
Fix redirected cache write without write VC (#13309) (#13336)
Redirected transactions can retain CACHE_WL_SUCCESS after the concrete
cache write VC has been cleared. When a later response reaches cache
write setup, we crash dereferencing the missing write VC.
This only reuses a redirected cache write when the prepared write VC is
still available. Otherwise, this resets the write state so ATS prepares
a fresh cache write. The added test simply adds some coverage around the
scenario but is not a true regression test. That is, the test still
passes without the src/ code change.
This addresses a crash in the cache write setup path:
```
#0 HttpSM::setup_cache_write_transfer(...)
#1 HttpSM::perform_cache_write_action()
#2 HttpSM::handle_api_return()
#3 HttpSM::state_api_callout()
#4 HttpSM::state_api_callback()
#5 TSHttpTxnReenable()
#6 EscalateResponse()
```
(cherry picked from commit 3b04db3474c674aa286c9b3406e00a649ea50783)
---
src/proxy/http/HttpSM.cc | 2 ++
src/proxy/http/HttpTransact.cc | 11 +++------
src/proxy/http/HttpTunnel.cc | 2 ++
.../pluginTest/escalate/escalate.test.py | 27 +++++++++++++++-------
4 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc
index af01d7c798..a3d261edb9 100644
--- a/src/proxy/http/HttpSM.cc
+++ b/src/proxy/http/HttpSM.cc
@@ -3849,6 +3849,8 @@ HttpSM::tunnel_handler_cache_write(int event,
HttpTunnelConsumer *c)
server_response_body_bytes = c->bytes_written;
}
+ // The cache write VC is closed now, so any prepared write lock is gone.
+ t_state.cache_info.write_lock_state = HttpTransact::CACHE_WL_INIT;
Metrics::Gauge::decrement(http_rsb.current_cache_connections);
return 0;
}
diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc
index 9bd7ac2023..65539ff81f 100644
--- a/src/proxy/http/HttpTransact.cc
+++ b/src/proxy/http/HttpTransact.cc
@@ -3362,14 +3362,9 @@
HttpTransact::set_cache_prepare_write_action_for_new_request(State *s)
// This method must be called no more than one time per request. It should
// not be called for non-cacheable requests.
if (s->cache_info.write_lock_state == CACHE_WL_SUCCESS) {
- // If and only if this is a redirected request, we may have already
- // prepared a cache write (during the handling of the previous request
- // which got the 3xx response) and can safely re-use it. Otherwise, we
- // risk storing the response under the wrong cache key. This is a release
- // assert because the correct behavior would be to prepare a new write,
- // but we can't do that because we failed to release the lock. To recover
- // we would have to tell the state machine to abort its write, and we
- // don't have a state for that.
+ // If and only if this is a redirected request, we may have already
prepared
+ // a cache write during the handling of the previous request and can re-use
+ // it. Otherwise, we risk storing the response under the wrong cache key.
ink_release_assert(s->redirect_info.redirect_in_process);
s->cache_info.action = CACHE_DO_WRITE;
} else {
diff --git a/src/proxy/http/HttpTunnel.cc b/src/proxy/http/HttpTunnel.cc
index a6282059eb..1c3ec2cb1c 100644
--- a/src/proxy/http/HttpTunnel.cc
+++ b/src/proxy/http/HttpTunnel.cc
@@ -1752,6 +1752,8 @@ HttpTunnel::chain_abort_cache_write(HttpTunnelProducer *p)
c->write_vio = nullptr;
c->vc->do_io_close(EHTTP_ERROR);
c->alive = false;
+ // The cache write VC is closed now, so any prepared write lock is
gone.
+ sm->t_state.cache_info.write_lock_state = HttpTransact::CACHE_WL_INIT;
Metrics::Gauge::decrement(http_rsb.current_cache_connections);
} else if (c->self_producer) {
chain_abort_cache_write(c->self_producer);
diff --git a/tests/gold_tests/pluginTest/escalate/escalate.test.py
b/tests/gold_tests/pluginTest/escalate/escalate.test.py
index d00c306f08..697242e006 100644
--- a/tests/gold_tests/pluginTest/escalate/escalate.test.py
+++ b/tests/gold_tests/pluginTest/escalate/escalate.test.py
@@ -35,21 +35,27 @@ class EscalateTest:
_replay_failover_file: str = 'escalate_failover.replay.yaml'
_server_original_file: str = 'escalate_original_server_default.replay.yaml'
_server_failover_file: str = 'escalate_failover_server_default.replay.yaml'
+ _process_counter: int = 0
- def __init__(self):
- '''Configure the test run.'''
- tr = Test.AddTestRun('Test escalate plugin.')
+ def __init__(self, enable_cache: bool = False) -> None:
+ '''Configure the test run.
+ :param enable_cache: Whether to exercise the cached redirect path.
+ '''
+ tr = Test.AddTestRun(f'Test escalate plugin.
enable_cache={enable_cache}')
+ self._enable_cache = enable_cache
self._setup_dns(tr)
self._setup_servers(tr)
self._setup_ts(tr)
self._setup_client(tr)
+ EscalateTest._process_counter += 1
def _setup_dns(self, tr: 'Process') -> None:
'''Set up the DNS server.
:param tr: The test run to add the DNS server to.
'''
- self._dns = tr.MakeDNServer(f"dns", default='127.0.0.1')
+ process_name = f"dns_{EscalateTest._process_counter}"
+ self._dns = tr.MakeDNServer(process_name, default='127.0.0.1')
def _setup_servers(self, tr: 'Process') -> None:
'''Set up the origin and failover servers.
@@ -60,8 +66,10 @@ class EscalateTest:
tr.Setup.Copy(self._replay_failover_file)
tr.Setup.Copy(self._server_original_file)
tr.Setup.Copy(self._server_failover_file)
- self._server_origin = tr.AddVerifierServerProcess(f"server_origin",
self._server_original_file)
- self._server_failover =
tr.AddVerifierServerProcess(f"server_failover", self._server_failover_file)
+ process_name = f"server_origin_{EscalateTest._process_counter}"
+ self._server_origin = tr.AddVerifierServerProcess(process_name,
self._server_original_file)
+ process_name = f"server_failover_{EscalateTest._process_counter}"
+ self._server_failover = tr.AddVerifierServerProcess(process_name,
self._server_failover_file)
self._server_origin.Streams.All += Testers.ContainsExpression(
'uuid: GET', "Verify the origin server received the GET request.")
@@ -87,7 +95,8 @@ class EscalateTest:
:param tr: The test run to add Traffic Server to.
'''
- self._ts = tr.MakeATSProcess(f"ts", enable_cache=False)
+ process_name = f"ts_{EscalateTest._process_counter}"
+ self._ts = tr.MakeATSProcess(process_name,
enable_cache=self._enable_cache)
# Select a port that is guaranteed to not be used at the moment.
dead_port = get_port(self._ts, "dead_port")
self._ts.Disk.records_config.update(
@@ -115,7 +124,8 @@ class EscalateTest:
:param tr: The test run to add the client to.
'''
- client = tr.AddVerifierClientProcess(f"client",
self._replay_original_file, http_ports=[self._ts.Variables.port])
+ process_name = f"client_{EscalateTest._process_counter}"
+ client = tr.AddVerifierClientProcess(process_name,
self._replay_original_file, http_ports=[self._ts.Variables.port])
client.StartBefore(self._dns)
client.StartBefore(self._server_origin)
client.StartBefore(self._server_failover)
@@ -132,3 +142,4 @@ class EscalateTest:
EscalateTest()
+EscalateTest(enable_cache=True)
