Bug 5051: Some collapsed revalidation responses never expire (#683)

* clear collapsed revalidation on a negative response

* fixup: Reduce chances of forgetting to call clearPublicKeyScope()

... in other/future sendClientUpstreamResponse() cases.

TODO:

1. Handle sendClientOldEntry() cases. We probably do not want future
   collapsed revalidation clients to accidentally collapse on the new
   entry that was rejected by this code. After all, such collapsing is
   exactly what caused bug 5051 AFAICT!

2. Consider cases where a collapsed revalidation did not reach
   handleIMSReply(). For example, imagine a client that created the
   collapsed revalidation entry and then disconnected or died. How to
   protect other clients on hitting that essentially stale entry
   forever, keeping bug 5051 alive?

Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index b397d59d9fc7d0790ec7a329d07afa034b060e19..ce939350aa542004476d940bbbb348f7b1be2367 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -367,6 +367,10 @@ clientReplyContext::sendClientUpstreamResponse()
 {
     StoreIOBuffer tempresult;
     removeStoreReference(&old_sc, &old_entry);
+
+    if (collapsedRevalidation)
+        http->storeEntry()->clearPublicKeyScope();
+
     /* here the data to send is the data we just received */
     tempBuffer.offset = 0;
     old_reqsize = 0;
@@ -480,10 +484,6 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result)
             http->logType = LOG_TCP_REFRESH_MODIFIED;
             debugs(88, 3, "origin replied " << status <<
                    ", replacing existing entry and forwarding to client");
-
-            if (collapsedRevalidation)
-                http->storeEntry()->clearPublicKeyScope();
-
             sendClientUpstreamResponse();
         }
     }