negative_caching_lifetime and ttl-in-cache interaction (#12665)

When both negative_caching_lifetime and cache.config ttl-in-cache are
configured, ttl-in-cache was incorrectly overriding
negative_caching_lifetime for negative responses (404, 403, 500, etc.).
This caused negative responses to be cached for the ttl-in-cache
duration instead of the intended negative_caching_lifetime. The
fix modifies what_is_document_freshness() to skip the ttl-in-cache check
for negatively cached responses.

Author: bneradt

src/proxy/http/HttpTransact.cc

@@ -7284,18 +7284,27 @@ HttpTransact::what_is_document_freshness(State *s, HTTPHdr *client_request, HTTP
   //////////////////////////////////////////////////////
   // If config file has a ttl-in-cache field set,     //
   // it has priority over any other http headers and  //
-  // other configuration parameters.                  //
+  // other configuration parameters. Negative caching //
+  // is different however since the user would       //
+  // rather expect their explicitly configured        //
+  // negative_caching_lifetime to be used instead of  //
+  // ttl-in-cache.                                    //
   //////////////////////////////////////////////////////
   if (s->cache_control.ttl_in_cache > 0) {
-    // what matters if ttl is set is not the age of the document
-    // but for how long it has been stored in the cache (resident time)
-    int resident_time = s->current.now - s->response_received_time;
-
-    TxnDbg(dbg_ctl_http_match, "ttl-in-cache = %d, resident time = %d", s->cache_control.ttl_in_cache, resident_time);
-    if (resident_time > s->cache_control.ttl_in_cache) {
-      return (Freshness_t::STALE);
+    auto status = static_cast<int>(cached_obj_response->status_get());
+    if (s->txn_conf->negative_caching_enabled && s->txn_conf->negative_caching_list.contains(status)) {
+      TxnDbg(dbg_ctl_http_match, "ttl-in-cache set, but skipping for negative cached response %d", status);
     } else {
-      return (Freshness_t::FRESH);
+      // what matters if ttl is set is not the age of the document
+      // but for how long it has been stored in the cache (resident time)
+      int resident_time = s->current.now - s->response_received_time;
+
+      TxnDbg(dbg_ctl_http_match, "ttl-in-cache = %d, resident time = %d", s->cache_control.ttl_in_cache, resident_time);
+      if (resident_time > s->cache_control.ttl_in_cache) {
+        return (Freshness_t::STALE);
+      } else {
+        return (Freshness_t::FRESH);
+      }
     }
   }
 

tests/gold_tests/cache/negative-caching.test.py

@@ -151,3 +151,32 @@
 tr.AddVerifierServerProcess("server-no-timeout", replay_file, http_ports=[server_port])
 tr.AddVerifierClientProcess("client-no-timeout", replay_file, http_ports=[ts.Variables.port])
 tr.StillRunningAfter = ts
+
+#
+# Verify that negative_caching_lifetime is respected even when cache.config
+# has ttl-in-cache configured.
+#
+replay_file = "replay/negative-caching-ttl-in-cache.replay.yaml"
+tr = Test.AddTestRun("Verify negative_caching_lifetime and ttl-in-cache interaction.")
+dns = tr.MakeDNServer("dns", default="127.0.0.1")
+server = tr.AddVerifierServerProcess("server-ttl-in-cache", replay_file)
+server_port = server.Variables.http_port
+ts = tr.MakeATSProcess("ts-ttl-in-cache")
+ts.Disk.records_config.update(
+    {
+        'proxy.config.dns.nameservers': f"127.0.0.1:{dns.Variables.Port}",
+        'proxy.config.dns.resolv_conf': 'NULL',
+        'proxy.config.diags.debug.enabled': 1,
+        'proxy.config.diags.debug.tags': 'http',
+        'proxy.config.http.insert_age_in_response': 0,
+        'proxy.config.http.negative_caching_enabled': 1,
+        'proxy.config.http.negative_caching_lifetime': 2
+    })
+ts.Disk.remap_config.AddLine(f'map / http://backend.example.com:{server_port}')
+# Configure cache.config with a long ttl-in-cache that should NOT override
+# negative_caching_lifetime for negative responses.
+ts.Disk.cache_config.AddLine('dest_domain=backend.example.com ttl-in-cache=30d')
+p = tr.AddVerifierClientProcess("client-ttl-in-cache", replay_file, http_ports=[ts.Variables.port])
+p.StartBefore(dns)
+p.StartBefore(server)
+p.StartBefore(ts)

tests/gold_tests/cache/replay/negative-caching-ttl-in-cache.replay.yaml

@@ -0,0 +1,96 @@
+#  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.
+
+#
+# Initial request that caches a 404 response.
+# This tests that negative_caching_lifetime is respected even when
+# ttl-in-cache is configured.
+#
+
+meta:
+  version: "1.0"
+
+sessions:
+- transactions:
+
+  #
+  # Test 1: Cache a 404 response.
+  #
+  - client-request:
+      method: "GET"
+      version: "1.1"
+      scheme: "http"
+      url: /path/404
+      headers:
+        fields:
+        - [ Host, example.com ]
+        - [ uuid, 100 ]
+
+    server-response:
+      status: 404
+      reason: "Not Found"
+      headers:
+        fields:
+        - [ Content-Length, 8 ]
+
+    proxy-response:
+      status: 404
+
+  # Repeat the request and verify that the 404 is served from the cache.
+  - client-request:
+      # Add a delay so ATS has time to finish any caching IO for the previous
+      # transaction.
+      delay: 100ms
+
+      method: "GET"
+      version: "1.1"
+      scheme: "http"
+      url: /path/404
+      headers:
+        fields:
+        - [ Host, example.com ]
+        - [ uuid, 101 ]
+
+
+    # The following should not reach the server since the 404 is cached.
+    server-response:
+      status: 200
+      reason: OK
+
+    # Verify the cached 404 response is served.
+    proxy-response:
+      status: 404
+
+
+  # Delay for 3 seconds to exceed negative_caching_lifetime and verify that the
+  # 200 OK is served from the server rather than the cached 404.
+  - client-request:
+      delay: 3s
+      method: "GET"
+      version: "1.1"
+      scheme: "http"
+      url: /path/404
+      headers:
+        fields:
+        - [ Host, example.com ]
+        - [ uuid, 102 ]
+
+    server-response:
+      status: 200
+      reason: OK
+
+    proxy-response:
+      status: 200