From ee9899320abfe596fdb6f5589a9fce5814d3a455 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 27 Nov 2024 02:32:02 +0000 Subject: [PATCH] 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. --- doc/release-notes/release-7.sgml.in | 8 ++++++++ src/StoreClient.h | 4 ++++ src/cf.data.pre | 9 ++++++++- src/client_side_reply.cc | 2 +- src/htcp.cc | 4 ++-- src/icp_v2.cc | 4 ++-- src/refresh.cc | 9 ++++++++- src/store_client.cc | 1 + src/store_digest.cc | 4 ++++ src/urn.cc | 2 ++ test-suite/test-functionality.sh | 1 + 11 files changed, 41 insertions(+), 7 deletions(-) diff --git a/doc/release-notes/release-7.sgml.in b/doc/release-notes/release-7.sgml.in index 47039e78271..0b46f76e917 100644 --- 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: external_acl_type

Removed %IDENT format code with Ident protocol support. + collapsed_forwarding +

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. + quick_abort_pct

Instead of ignoring quick_abort_pct 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 b0a83c34624..d534af970a0 100644 --- 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 @@ 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 diff --git a/src/cf.data.pre b/src/cf.data.pre index d6e03f5fd3b..c9da24e317b 100644 --- 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 66c53916111..5f589b7d20c 100644 --- 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 e91851aa28d..a87af95d3fd 100644 --- 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 f759e89cb12..c1c3474d021 100644 --- 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 81be28ae4b4..0e8db3bf201 100644 --- 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 12b730fc4fc..e24a6e192cd 100644 --- 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 3cf348ad4eb..3fb1c76f019 100644 --- 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 ff88aae4d54..934d023712e 100644 --- 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 eb191bc0cc9..d325e764bcd 100755 --- 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