Reuse prepared cache write on redirected request (#11542)

proxy/http/HttpSM.cc

@@ -7959,9 +7959,8 @@ HttpSM::set_next_state()
   }
 
   case HttpTransact::SM_ACTION_CACHE_ISSUE_WRITE: {
-    ink_assert((cache_sm.cache_write_vc == nullptr) || t_state.redirect_info.redirect_in_process);
+    ink_assert(cache_sm.cache_write_vc == nullptr);
     HTTP_SM_SET_DEFAULT_HANDLER(&HttpSM::state_cache_open_write);
-
     do_cache_prepare_write();
     break;
   }

proxy/http/HttpTransact.cc

@@ -2215,12 +2215,7 @@ HttpTransact::DecideCacheLookup(State *s)
     if (s->redirect_info.redirect_in_process) {
       // without calling out the CACHE_LOOKUP_COMPLETE_HOOK
       if (s->txn_conf->cache_http) {
-        if (s->cache_info.write_lock_state == CACHE_WL_FAIL) {
-          s->cache_info.action           = CACHE_PREPARE_TO_WRITE;
-          s->cache_info.write_lock_state = HttpTransact::CACHE_WL_INIT;
-        } else if (s->cache_info.write_lock_state == CACHE_WL_SUCCESS) {
-          s->cache_info.action = CACHE_DO_WRITE;
-        }
+        HttpTransact::set_cache_prepare_write_action_for_new_request(s);
       }
       LookupSkipOpenServer(s);
     } else {
@@ -3336,7 +3331,7 @@ HttpTransact::HandleCacheOpenReadMiss(State *s)
   } else if (s->api_server_response_no_store) { // plugin may have decided not to cache the response
     s->cache_info.action = CACHE_DO_NO_ACTION;
   } else {
-    s->cache_info.action = CACHE_PREPARE_TO_WRITE;
+    HttpTransact::set_cache_prepare_write_action_for_new_request(s);
   }
 
   ///////////////////////////////////////////////////////////////
@@ -3391,6 +3386,28 @@ HttpTransact::HandleCacheOpenReadMiss(State *s)
   return;
 }
 
+void
+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.
+    ink_release_assert(s->redirect_info.redirect_in_process);
+    s->cache_info.action = CACHE_DO_WRITE;
+  } else {
+    s->cache_info.action           = CACHE_PREPARE_TO_WRITE;
+    s->cache_info.write_lock_state = HttpTransact::CACHE_WL_INIT;
+  }
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // Name       : OriginServerRawOpen
 // Description: called for ssl tunneling

proxy/http/HttpTransact.h

@@ -965,6 +965,7 @@ class HttpTransact
   static void HandleCacheOpenReadHitFreshness(State *s);
   static void HandleCacheOpenReadHit(State *s);
   static void HandleCacheOpenReadMiss(State *s);
+  static void set_cache_prepare_write_action_for_new_request(State *s);
   static void build_response_from_cache(State *s, HTTPWarningCode warning_code);
   static void handle_cache_write_lock(State *s);
   static void HandleResponse(State *s);

tests/gold_tests/redirect/redirect_to_same_origin_on_cache.replay.yaml

@@ -0,0 +1,65 @@
+#  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.
+
+meta:
+  version: "1.0"
+
+sessions:
+- protocol:
+  - name: http
+    version: 1
+  - name: tcp
+  - name: ip
+
+  transactions:
+  - client-request:
+      method: "GET"
+      version: "1.1"
+      url: /a/path/resource
+      headers:
+        fields:
+        - [ Host, oof.com ]
+        - [ Connection, keep-alive ]
+        - [ Content-Length, 0 ]
+
+    server-response:
+      status: 307
+      headers:
+        fields:
+          - [ location, /a/new/path/resource ]
+          - [ content-length, 0 ]
+
+    proxy-response:
+      status: 200
+  - client-request:
+      method: "GET"
+      version: "1.1"
+      url: /a/new/path/resource
+      headers:
+        fields:
+        - [ Host, oof.com ]
+        - [ Connection, keep-alive ]
+        - [ Content-Length, 0 ]
+
+    server-response:
+      status: 200
+      reason: OK
+      headers:
+        fields:
+          - [ content-length, 16 ]
+
+    proxy-response:
+      status: 200

tests/gold_tests/redirect/redirect_to_same_origin_on_cache.test.py

@@ -0,0 +1,140 @@
+#  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.
+
+import functools
+from typing import Any, Callable, Dict, Optional
+
+from ports import get_port
+
+Test.Summary = 'Tests SNI port-based routing'
+
+TestParams = Dict[str, Any]
+
+
+class TestRedirectToSameOriginOnCache:
+    """Configure a test for reproducing #9275."""
+
+    replay_filepath_one: str = "redirect_to_same_origin_on_cache.replay.yaml"
+    client_counter: int = 0
+    server_counter: int = 0
+    ts_counter: int = 0
+
+    def __init__(self, name: str, /, autorun) -> None:
+        """Initialize the test.
+
+        :param name: The name of the test.
+        """
+        self.name = name
+        self.autorun = autorun
+
+    def _init_run(self) -> "TestRun":
+        """Initialize processes for the test run."""
+
+        tr = Test.AddTestRun(self.name)
+        server_one = TestRedirectToSameOriginOnCache.configure_server(tr, "oof.com")
+        self._configure_traffic_server(tr, server_one)
+        dns = TestRedirectToSameOriginOnCache.configure_dns(tr, server_one, self._dns_port)
+
+        tr.Processes.Default.StartBefore(server_one)
+        tr.Processes.Default.StartBefore(dns)
+        tr.Processes.Default.StartBefore(self._ts)
+
+        return {
+            "tr": tr,
+            "ts": self._ts,
+            "server_one": server_one,
+            "port_one": self._port_one,
+        }
+
+    @classmethod
+    def runner(cls, name: str, autorun: bool = True) -> Optional[Callable]:
+        """Create a runner for a test case.
+
+        :param autorun: Run the test case once it's set up. Default is True.
+        """
+        test = cls(name, autorun=autorun)._prepare_test_case
+        return test
+
+    def _prepare_test_case(self, func: Callable) -> Callable:
+        """Set up a test case and possibly run it.
+
+        :param func: The test case to set up.
+        """
+        functools.wraps(func)
+        test_params = self._init_run()
+
+        def wrapper(*args, **kwargs) -> Any:
+            return func(test_params, *args, **kwargs)
+
+        if self.autorun:
+            wrapper()
+        return wrapper
+
+    @staticmethod
+    def configure_server(tr: "TestRun", domain: str):
+        server = tr.AddVerifierServerProcess(
+            f"server{TestRedirectToSameOriginOnCache.server_counter}.{domain}",
+            TestRedirectToSameOriginOnCache.replay_filepath_one,
+            other_args="--format \"{url}\"")
+        TestRedirectToSameOriginOnCache.server_counter += 1
+
+        return server
+
+    @staticmethod
+    def configure_dns(tr: "TestRun", server_one: "Process", dns_port: int):
+        dns = tr.MakeDNServer("dns", port=dns_port, default="127.0.0.1")
+        return dns
+
+    def _configure_traffic_server(self, tr: "TestRun", server_one: "Process"):
+        """Configure Traffic Server.
+
+        :param tr: The TestRun object to associate the ts process with.
+        """
+        ts = tr.MakeATSProcess(
+            f"ts-{TestRedirectToSameOriginOnCache.ts_counter}", select_ports=False, enable_tls=True, enable_cache=True)
+        TestRedirectToSameOriginOnCache.ts_counter += 1
+
+        self._port_one = get_port(ts, "PortOne")
+        self._dns_port = get_port(ts, "DNSPort")
+        ts.Disk.records_config.update(
+            {
+                'proxy.config.http.server_ports': f"{self._port_one}",
+                'proxy.config.diags.debug.enabled': 1,
+                'proxy.config.diags.debug.tags': "cache|dns|http|redirect|remap",
+                'proxy.config.dns.nameservers': f"127.0.0.1:{self._dns_port}",
+                'proxy.config.dns.resolv_conf': 'NULL',
+                'proxy.config.http.redirect.actions': "self:follow",
+                'proxy.config.http.number_of_redirections': 1,
+            })
+
+        ts.Disk.remap_config.AddLine(f"map oof.com http://oof.backend.com:{server_one.Variables.http_port}")
+
+        self._ts = ts
+
+
+# Tests start.
+
+
+@TestRedirectToSameOriginOnCache.runner("Redirect to same origin with cache")
+def test1(params: TestParams) -> None:
+    client = params["tr"].AddVerifierClientProcess(
+        f"client0",
+        TestRedirectToSameOriginOnCache.replay_filepath_one,
+        http_ports=[params["port_one"]],
+        other_args="--format \"{url}\" --keys \"/a/path/resource\"")
+
+    params["tr"].Processes.Default.ReturnCode = 0
+    params["tr"].Processes.Default.Streams.stdout.Content += Testers.IncludesExpression("200 OK", "We should get the resource.")