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 3b04db3474 Fix redirected cache write without write VC (#13309)
3b04db3474 is described below
commit 3b04db3474c674aa286c9b3406e00a649ea50783
Author: Brian Neradt <[email protected]>
AuthorDate: Wed Jun 24 12:28:49 2026 -0500
Fix redirected cache write without write VC (#13309)
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()
```
---
src/proxy/http/HttpSM.cc | 2 ++
src/proxy/http/HttpTransact.cc | 11 +++--------
src/proxy/http/HttpTunnel.cc | 2 ++
tests/gold_tests/pluginTest/escalate/escalate.test.py | 10 +++++++---
4 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc
index e12b2c4f6d..4a3cb451f9 100644
--- a/src/proxy/http/HttpSM.cc
+++ b/src/proxy/http/HttpSM.cc
@@ -3932,6 +3932,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::CacheWriteLock_t::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 df56761a64..a8746b2e21 100644
--- a/src/proxy/http/HttpTransact.cc
+++ b/src/proxy/http/HttpTransact.cc
@@ -3565,14 +3565,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 == CacheWriteLock_t::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 = CacheAction_t::WRITE;
} else if (s->cache_info.write_lock_state == CacheWriteLock_t::READ_RETRY &&
diff --git a/src/proxy/http/HttpTunnel.cc b/src/proxy/http/HttpTunnel.cc
index 3695edf87a..4588dae087 100644
--- a/src/proxy/http/HttpTunnel.cc
+++ b/src/proxy/http/HttpTunnel.cc
@@ -1755,6 +1755,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::CacheWriteLock_t::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 ea9f2953b6..46756e5a64 100644
--- a/tests/gold_tests/pluginTest/escalate/escalate.test.py
+++ b/tests/gold_tests/pluginTest/escalate/escalate.test.py
@@ -38,11 +38,14 @@ class EscalateTest:
_server_failover_file: str = 'escalate_failover_server_default.replay.yaml'
_process_counter: int = 0
- def __init__(self, disable_redirect_header: bool = False) -> None:
+ def __init__(self, disable_redirect_header: bool = False, enable_cache:
bool = False) -> None:
'''Configure the test run.
:param disable_redirect_header: Whether to use --no-redirect-header.
+ :param enable_cache: Whether to exercise the cached redirect path.
'''
- tr = Test.AddTestRun(f'Test escalate plugin.
disable_redirect_header={disable_redirect_header}')
+ tr = Test.AddTestRun(
+ f'Test escalate plugin.
disable_redirect_header={disable_redirect_header}, enable_cache={enable_cache}')
+ self._enable_cache = enable_cache
self._setup_dns(tr)
self._setup_servers(tr, disable_redirect_header)
self._setup_ts(tr, disable_redirect_header)
@@ -118,7 +121,7 @@ class EscalateTest:
:param disable_redirect_header: Whether ATS should be configured with
--no-redirect-header.
'''
process_name = f"ts_{EscalateTest._process_counter}"
- self._ts = tr.MakeATSProcess(process_name, enable_cache=False)
+ 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(
@@ -287,4 +290,5 @@ class EscalateNonGetMethodsTest:
EscalateTest(disable_redirect_header=False)
EscalateTest(disable_redirect_header=True)
+EscalateTest(disable_redirect_header=False, enable_cache=True)
EscalateNonGetMethodsTest()
