squid/SOURCES/0003-Bug-5309-frequent-lowestOffset-target_offset-asserti.patch

From af18cb04f07555f49daef982c8c21459bfbe388c Mon Sep 17 00:00:00 2001
From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Thu, 23 Nov 2023 18:27:24 +0000
Subject: [PATCH 3/7] Bug 5309: frequent "lowestOffset () <= target_offset"
 assertion (#1561)

    Recent commit 122a6e3 left store_client::readOffset() unchanged but
    should have adjusted it to match changed copyInto.offset semantics:
    Starting with that commit, storeClientCopy() callers supply HTTP
    response _body_ offset rather than HTTP response offset.
....
    This bug decreased readOffset() values (by the size of stored HTTP
    response headers), effectively telling Store that we are not yet done
    with some of the MemObject/mem_hdr bytes. This bug could cause slightly
    higher transaction memory usage because the same response bytes are
    trimmed later. This bug should not have caused any assertions.
....
    However, the old mem_hdr::freeDataUpto() code that uses readOffset() is
    also broken -- the assertion in that method only "works" when
    readOffset() returns values matching a memory node boundary. The smaller
    values returned by buggy readOffset() triggered buggy assertions.
....
    This minimal fix removes the recent store_client::readOffset() bug
    described above. We will address old mem_hdr problems separately.

Modified-by: Alex Burmashev <alexander.burmashev@oracle.com>
Signed-off-by: Alex Burmashev <alexander.burmashev@oracle.com>
---
 src/MemObject.cc    |  2 +-
 src/StoreClient.h   | 19 ++++++++++---------
 src/store_client.cc | 13 +++++++++++++
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/src/MemObject.cc b/src/MemObject.cc
index d7aaf5e..650d3fd 100644
--- a/src/MemObject.cc
+++ b/src/MemObject.cc
@@ -197,7 +197,7 @@ struct LowestMemReader : public unary_function<store_client, void> {
 
     void operator() (store_client const &x) {
         if (x.getType() == STORE_MEM_CLIENT)
-            current = std::min(current, x.readOffset());
+            current = std::min(current, x.discardableHttpEnd());
     }
 
     int64_t current;
diff --git a/src/StoreClient.h b/src/StoreClient.h
index 1d90e5a..0524776 100644
--- a/src/StoreClient.h
+++ b/src/StoreClient.h
@@ -54,15 +54,8 @@ public:
     store_client(StoreEntry *);
     ~store_client();
 
-    /// An offset into the stored response bytes, including the HTTP response
-    /// headers (if any). Note that this offset does not include Store entry
-    /// metadata, because it is not a part of the stored response.
-    /// \retval 0 means the client wants to read HTTP response headers.
-    /// \retval +N the response byte that the client wants to read next.
-    /// \retval -N should not occur.
-    // TODO: Callers do not expect negative offset. Verify that the return
-    // value cannot be negative and convert to unsigned in this case.
-    int64_t readOffset() const { return copyInto.offset; }
+    /// the client will not use HTTP response bytes with lower offsets (if any)
+    auto discardableHttpEnd() const { return discardableHttpEnd_; }
 
     int getType() const;
 
@@ -156,8 +149,16 @@ private:
 
     /// Storage and metadata associated with the current copy() request. Ought
     /// to be ignored when not answering a copy() request.
+    /// * copyInto.offset is the requested HTTP response body offset;
+    /// * copyInto.data is the client-owned, client-provided result buffer;
+    /// * copyInto.length is the size of the .data result buffer;
+    /// * copyInto.flags are unused by this class.
     StoreIOBuffer copyInto;
 
+    // TODO: Convert to uint64_t after fixing mem_hdr::endOffset() and friends.
+    /// \copydoc discardableHttpEnd()
+    int64_t discardableHttpEnd_ = 0;
+
     /// the total number of finishCallback() calls
     uint64_t answers;
 
diff --git a/src/store_client.cc b/src/store_client.cc
index 1731c4c..383aac8 100644
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -122,6 +122,16 @@ store_client::finishCallback()
         result = parsingBuffer->packBack();
     result.flags.error = object_ok ? 0 : 1;
 
+    // TODO: Move object_ok handling above into this `if` statement.
+    if (object_ok) {
+        // works for zero hdr_sz cases as well; see also: nextHttpReadOffset()
+        discardableHttpEnd_ = NaturalSum<int64_t>(entry->mem().baseReply().hdr_sz, result.offset, result.length).value();
+    } else {
+        // object_ok is sticky, so we will not be able to use any response bytes
+        discardableHttpEnd_ = entry->mem().endOffset();
+    }
+    debugs(90, 7, "with " << result << "; discardableHttpEnd_=" << discardableHttpEnd_);
+
     // no HTTP headers and no body bytes (but not because there was no space)
     atEof_ = !sendingHttpHeaders() && !result.length && copyInto.length;
 
@@ -220,6 +230,9 @@ store_client::copy(StoreEntry * anEntry,
 
     parsingBuffer.emplace(copyInto);
 
+    discardableHttpEnd_ = nextHttpReadOffset();
+    debugs(90, 7, "discardableHttpEnd_=" << discardableHttpEnd_);
+
     static bool copying (false);
     assert (!copying);
     copying = true;
-- 
2.39.3