Treat responses to collapsed requests as fresh (#1927)

Squid transactions involving collapsed requests receive their responses
as Store cache hits. Cache hits undergo mandatory freshness validation
checks and, if those checks detect a stale response, may trigger
revalidation (e.g., an If-Modified-Since request to the origin server).
This logic results in a still-being-delivered-to-Squid response
triggering its own revalidation if that response is deemed stale on
arrival by collapsed request (e.g., has a Cache-Control: max-age=0
header field).

HTTP RFC 9111 Section 4.7 briefly mentions collapsed requests but is
ambiguous with regard to validation of responses to collapsed requests.
IETF HTTP Working Group chair believes that specs are unclear, and that
these responses should not be treated as cache hits (in primary cases):
https://lists.w3.org/Archives/Public/ietf-http-wg/2024JanMar/0095.html

This change follows the above advice and prevents arguably excessive
freshness checks for responses to collapsed requests. This change is
collapsed-forwarding specific: It does not prevent freshness checks for
responses that were, at the time of a hit request, either fully cached
or still receiving response body bytes.

After this change, clientReplyContext can no longer collapse twice, once
after initial Store lookup and then again during refresh, because the
first collapse now precludes refresh.

diff --git a/doc/release-notes/release-7.sgml.in b/doc/release-notes/release-7.sgml.in
index 47039e782717c89dad32275966ec3e336377bfcf..0b46f76e917b4ec4634fec9f5c3aa837f6b95d0b 100644 (file)
--- a/doc/release-notes/release-7.sgml.in
+++ b/doc/release-notes/release-7.sgml.in
@@ -194,6 +194,14 @@ This section gives an account of those changes in three categories:
        <tag>external_acl_type</tag>
        <p>Removed <em>%IDENT</em> format code with Ident protocol support.
 
+       <tag>collapsed_forwarding</tag>
+       <p>Squid no longer revalidates responses to collapsed requests, treating
+       all such responses as fresh. This change follows IETF HTTP Working Group
+       advice (in an HTTP gray area) and prevents arguably excessive freshness
+       checks for responses to collapsed requests. This change does not prevent
+       freshness checks for responses that were, at the time of a hit request,
+       either fully cached or still receiving response body bytes.
+
        <tag>quick_abort_pct</tag>
        <p>Instead of ignoring <em>quick_abort_pct</em> settings that would,
        together with other conditions, abort a pending download of a 99-byte or

diff --git a/src/StoreClient.h b/src/StoreClient.h
index b0a83c34624f114cc1efd7d5ebccee17a4efc8da..d534af970a0efac819c1beb4ff7978040f09865d 100644 (file)
--- a/src/StoreClient.h
+++ b/src/StoreClient.h
@@ -35,6 +35,7 @@ class StoreEntry;
 class ACLFilledChecklist;
 class LogTags;
 
+// TODO: Merge store_client into StoreClient.
 /// a storeGetPublic*() caller
 class StoreClient: public Acl::ChecklistFiller
 {
@@ -58,6 +59,9 @@ protected:
     bool mayInitiateCollapsing() const { return onCollapsingPath(); }
     /// whether Squid configuration allows collapsing for this transaction
     bool onCollapsingPath() const;
+
+    /// whether startCollapsingOn() was called and returned true
+    mutable bool didCollapse = false;
 };
 
 #if USE_DELAY_POOLS

diff --git a/src/cf.data.pre b/src/cf.data.pre
index d6e03f5fd3bcf7b3317119c1c3ad4c0aedcf3b7e..c9da24e317b9be0489eae8ea68e2567de5c14fce 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -6448,7 +6448,9 @@ LOC: Config.accessList.sendHit
 DOC_START
        Responses denied by this directive will not be served from the cache
        (but may still be cached, see store_miss). This directive has no
-       effect on the responses it allows and on the cached objects.
+       effect on the responses it allows and on the cached objects. This
+       directive is applied to both regular from-cache responses and responses
+       reused by collapsed requests (see collapsed_forwarding).
 
        Please see the "cache" directive for a summary of differences among
        store_miss, send_hit, and cache directives.
@@ -7237,6 +7239,11 @@ DOC_START
        requests hitting a stale cached object. Revalidation collapsing
        is currently disabled for Squid instances containing SMP-aware
        disk or memory caches and for Vary-controlled cached objects.
+
+       A response reused by the collapsed request is deemed fresh in that
+       request processing context -- Squid does not apply refresh_pattern and
+       internal freshness validation checks to collapsed transactions. Squid
+       does apply send_hit rules.
 DOC_END
 
 NAME: collapsed_forwarding_access

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 66c5391611149157667c3ea41a79cc419c56d23d..5f589b7d20c44bbc0edd89d425016bacd5c2a1a8 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -627,7 +627,7 @@ clientReplyContext::cacheHit(const StoreIOBuffer result)
         http->updateLoggingTags(LOG_TCP_MISS);
         processMiss();
         return;
-    } else if (!r->flags.internal && refreshCheckHTTP(e, r)) {
+    } else if (!r->flags.internal && !didCollapse && refreshCheckHTTP(e, r)) {
         debugs(88, 5, "clientCacheHit: in refreshCheck() block");
         /*
          * We hold a stale copy; it needs to be validated

diff --git a/src/htcp.cc b/src/htcp.cc
index e91851aa28d629bbcf94cbb9715a1e8877981ec4..a87af95d3fdabd6f8e46743ddb408ae8efda1459 100644 (file)
--- a/src/htcp.cc
+++ b/src/htcp.cc
@@ -981,10 +981,10 @@ htcpSpecifier::checkHit()
         debugs(31, 3, "NO; public object not found");
     } else if (!e->validToSend()) {
         debugs(31, 3, "NO; entry not valid to send" );
-    } else if (refreshCheckHTCP(e, checkHitRequest.getRaw())) {
-        debugs(31, 3, "NO; cached response is stale");
     } else if (e->hittingRequiresCollapsing() && !startCollapsingOn(*e, false)) {
         debugs(31, 3, "NO; prohibited CF hit: " << *e);
+    } else if (!didCollapse && refreshCheckHTCP(e, checkHitRequest.getRaw())) {
+        debugs(31, 3, "NO; cached response is stale");
     } else {
         debugs(31, 3, "YES!?");
         hit = e;

diff --git a/src/icp_v2.cc b/src/icp_v2.cc
index f759e89cb12a972f7c42f3f23a08f6836bdfe35b..c1c3474d021a915f58e6e2396974668bebe53fa8 100644 (file)
--- a/src/icp_v2.cc
+++ b/src/icp_v2.cc
@@ -170,10 +170,10 @@ ICPState::confirmAndPrepHit(const StoreEntry &e) const
     if (!e.validToSend())
         return false;
 
-    if (!Config.onoff.icp_hit_stale && refreshCheckICP(&e, request))
+    if (e.hittingRequiresCollapsing() && !startCollapsingOn(e, false))
         return false;
 
-    if (e.hittingRequiresCollapsing() && !startCollapsingOn(e, false))
+    if (!Config.onoff.icp_hit_stale && !didCollapse && refreshCheckICP(&e, request))
         return false;
 
     return true;

diff --git a/src/refresh.cc b/src/refresh.cc
index 81be28ae4b49df3f4dfc40dbde17033f65dd0202..0e8db3bf2010c176a7cd0ef86dfd0875476ebea1 100644 (file)
--- a/src/refresh.cc
+++ b/src/refresh.cc
@@ -523,6 +523,10 @@ refreshIsCachable(const StoreEntry * entry)
      * minimum_expiry_time seconds delta (defaults to 60 seconds), to
      * avoid objects which expire almost immediately, and which can't
      * be refreshed.
+     *
+     * No hittingRequiresCollapsing() or didCollapse concerns here: This
+     * incoming response is fresh now, but we want to check whether it can be
+     * refreshed Config.minimum_expiry_time seconds later.
      */
     int reason = refreshCheck(entry, nullptr, Config.minimum_expiry_time);
     ++ refreshCounts[rcStore].total;
@@ -569,6 +573,10 @@ refreshIsStaleIfHit(const int reason)
  *
  * \retval 1 if STALE
  * \retval 0 if FRESH
+ *
+ * Do not call this when StoreClient::didCollapse is true. XXX: Callers should
+ * not have to remember to check didCollapse. TODO: Refactor by adding something
+ * like pure virtual StoreClient::refreshCheck() with protocol specializations?
  */
 int
 refreshCheckHTTP(const StoreEntry * entry, HttpRequest * request)
@@ -577,7 +585,6 @@ refreshCheckHTTP(const StoreEntry * entry, HttpRequest * request)
     ++ refreshCounts[rcHTTP].total;
     ++ refreshCounts[rcHTTP].status[reason];
     request->flags.staleIfHit = refreshIsStaleIfHit(reason);
-    // TODO: Treat collapsed responses as fresh but second-hand.
     return (Config.onoff.offline || reason < 200) ? 0 : 1;
 }
 

diff --git a/src/store_client.cc b/src/store_client.cc
index 12b730fc4fcd688641bcd3333ceb85f3357b0054..e24a6e192cda6b34d770fb98829f947b51218143 100644 (file)
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -80,6 +80,7 @@ StoreClient::startCollapsingOn(const StoreEntry &e, const bool doingRevalidation
             tags->collapsingHistory.otherCollapses++;
     }
 
+    didCollapse = true;
     debugs(85, 5, e << " doingRevalidation=" << doingRevalidation);
     return true;
 }

diff --git a/src/store_digest.cc b/src/store_digest.cc
index 3cf348ad4eba9051840569da0db5cf873467848e..3fb1c76f0192e4e937f3d7b7f104a05e0e2426ee 100644 (file)
--- a/src/store_digest.cc
+++ b/src/store_digest.cc
@@ -260,6 +260,10 @@ storeDigestAddable(const StoreEntry * e)
     }
 
     /* still here? check staleness */
+    // Include hittingRequiresCollapsing() entries: They are fresh _now_, but we
+    // check future freshness. They should not have enough info to judge future
+    // freshness since we are still waiting for their response headers, but
+    // admins might configure Squid to consider such entries fresh anyway.
     /* Note: We should use the time of the next rebuild, not (cur_time+period) */
     if (refreshCheckDigest(e, Config.digest.rebuild_period)) {
         debugs(71, 6, "storeDigestAdd: entry expires within " << Config.digest.rebuild_period << " secs, ignoring");

diff --git a/src/urn.cc b/src/urn.cc
index ff88aae4d54d4fe3e91ff267289e31c872a7f2e7..934d023712eee2a067fc5bb5ea976ca6e21c5b0e 100644 (file)
--- a/src/urn.cc
+++ b/src/urn.cc
@@ -282,6 +282,8 @@ urnHandleReply(void *data, StoreIOBuffer result)
         return;
     }
 
+    // XXX: Missing reply freshness checks (e.g., calling refreshCheckHTTP()).
+
     urls = urnParseReply(urnState->parsingBuffer.toSBuf(), urnState->request->method);
 
     if (!urls) {     /* unknown URN error */

diff --git a/test-suite/test-functionality.sh b/test-suite/test-functionality.sh
index eb191bc0cc94b10f394d04d07ff20ab2d5fda3a9..d325e764bcdb8b3272a6c1a5b5c6b586e9c74da6 100755 (executable)
--- a/test-suite/test-functionality.sh
+++ b/test-suite/test-functionality.sh
@@ -220,6 +220,7 @@ main() {
             upgrade-protocols
             cache-response
             proxy-collapsed-forwarding
+            hit-revalidation
             busy-restart
             truncated-responses
             malformed-request