Treat responses to collapsed requests as fresh #1927

rousskov · 2024-10-30T21:08:18Z

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.

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 still-being-delivered-to-Squid response triggering its own revalidation if that response is deemed stale on arrival (e.g., has a Cache-Control: max-age=0 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 does not prevent freshness checks for responses that were still receiving body bytes at the time of the hit request. TODO: Can we reduce the risk of forgetting to check didCollapse when calling refreshCheckHTTP(), especially if we add more refreshCheckHTTP() callers? TODO: Current code changes only cover a refreshCheckHTTP() call, but what about other refreshCheck() callers (e.g., refreshCheckICP())?

XXX: A single clientReplyContext::didCollapse member cannot distinguish which request (initial or refresh) got collapsed. Code "works" because the new member is ignored by refresh transactions -- they do not consult refreshCheckHTTP() -- but this ambiguity is a design defect. Fixing this requires merging store_client class into StoreClient class while refactoring clientReplyContext to move initial and refresh fetching code into two distinct/dedicated classes, each derived from the merged StoreClient class (instead of current complex/error-prone saveState() and restoreState() hacks that use _same_ classes for both fetches).

Requires squid-daft commit 12ae6fdd or better.

rousskov

The comments in this review annotate this PR without requesting any changes.

rousskov · 2024-10-30T21:09:55Z

src/StoreClient.h

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


A single clientReplyContext::didCollapse member cannot distinguish which request (initial or refresh) got collapsed. Code "works" because the new member is ignored by refresh transactions -- they do not consult refreshCheckHTTP() -- but this ambiguity is a design defect.

Fixing this requires merging store_client class into StoreClient class (I added a TODO about that) while refactoring clientReplyContext to move initial and refresh fetching code into two distinct/dedicated classes, each derived from the merged StoreClient class (instead of current complex/error-prone saveState() and restoreState() hacks that use the same classes for both fetches). That complex refactoring is outside this small PR scope, but I hope to be able to do it in the foreseeable future.

rousskov · 2024-10-30T21:17:32Z

src/htcp.cc

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


Git diff does not make this obvious, but this PR reorders HTCP and ICP freshness check steps to use didCollapse member (set during startCollapsingOn() step) for avoiding refreshCheck*() step. This reordering should not have a significant effect in most cases because hittingRequiresCollapsing() quickly returns false for most transactions. In rare/exceptional collapsed forwarding cases, the new computation order may be slower (both checks may perform configuration-dependent ACL or ACL-like computations), but it is a fair price to pay for avoiding revalidation transactions in those cases.

rousskov · 2024-10-30T21:19:14Z

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)) {


Current implementation requires this refreshCheckHTTP() and other indirect refreshCheck() callers to remember to check StoreClient::didCollapse member. We should embed that check, so that it cannot be forgotten! Doing so requires adding a pure virtual StoreClient::refreshCheck() method and moving Foo-specific refreshCheckFoo() functions into its overrides. The latter, in turn, requires some refresh.h API refactoring. All that is outside this small PR scope.

how about adding a TODO or XXX to that effect? PR comments are bound to be forgotten

how about adding a TODO or XXX to that effect?

Done in commit 9fc1ed5.

PR comments are bound to be forgotten

Yes, but a much bigger problem, in my experience, is that C++ comments like that often trigger negative votes and associated overheads, especially in areas where the existing code is quite messed up, obscuring underlying problems and proper solutions. C++ comments are also bound to become stale and, hence, misleading. That is why I only added two minimal C++ TODO/XXX comments and kept the rest of the discussion to PR comments. Let's hope it is going to be different this time.

kinkie · 2024-10-31T13:33:25Z

LGTM with a nit

Context: https://github.com/squid-cache/squid/pull/1927/files#r1824464433

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.

rousskov · 2024-11-27T02:34:23Z

I plan to clear this previously approved PR for merging on November 27, 2024.

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.

rousskov added 9 commits October 30, 2024 16:46

fixup: Address refreshCheckICP() TODO in the initial commit

3e3ac66

fixup: Address refreshCheckHTCP() TODO in the initial commit

bbf929b

fixup: Address refreshCheckDigest() TODO in the initial commit

6713af8

fixup: Address refreshIsCachable() TODO in the initial commit

8821a14

fixup: Documented why we are not adjusting urn.cc

a849892

Enable Daft tests of branch changes

4db163d

Requires squid-daft commit 12ae6fdd or better.

Mention branch changes in v7 release notes

9594244

rousskov commented Oct 30, 2024

View reviewed changes

fixup: Move problem identification/discussion to C++ comments

9fc1ed5

Context: https://github.com/squid-cache/squid/pull/1927/files#r1824464433

rousskov added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Oct 31, 2024

kinkie previously approved these changes Oct 31, 2024

View reviewed changes

kinkie added S-waiting-for-more-reviewers needs a reviewer and/or a second opinion and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Oct 31, 2024

squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Nov 5, 2024

squid-anubis added M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Nov 14, 2024

Merge branch 'master' into SQUID-957-collapsed-responses-are-fresh

4daec7a

rousskov dismissed kinkie’s stale review via 4daec7a November 27, 2024 02:32

rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-more-reviewers needs a reviewer and/or a second opinion labels Nov 30, 2024

squid-anubis closed this Nov 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat responses to collapsed requests as fresh #1927

Treat responses to collapsed requests as fresh #1927

rousskov commented Oct 30, 2024

rousskov left a comment

rousskov Oct 30, 2024

rousskov Oct 30, 2024

rousskov Oct 30, 2024

kinkie Oct 31, 2024

rousskov Oct 31, 2024

kinkie commented Oct 31, 2024

rousskov commented Nov 27, 2024

Treat responses to collapsed requests as fresh #1927

Treat responses to collapsed requests as fresh #1927

Conversation

rousskov commented Oct 30, 2024

rousskov left a comment

Choose a reason for hiding this comment

rousskov Oct 30, 2024

Choose a reason for hiding this comment

rousskov Oct 30, 2024

Choose a reason for hiding this comment

rousskov Oct 30, 2024

Choose a reason for hiding this comment

kinkie Oct 31, 2024

Choose a reason for hiding this comment

rousskov Oct 31, 2024

Choose a reason for hiding this comment

kinkie commented Oct 31, 2024

rousskov commented Nov 27, 2024