Treat responses to collapsed requests as fresh

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

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 @@ class StoreClient: public Acl::ChecklistFiller
     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

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

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

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;

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;

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;
 }
 

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;
 }

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");

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 */

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