MB-68655: Test more than CAS when considering Item/SV equality

In the processExpiredItem path there is a StoredValue vs Item
compare that considers same CAS being same Item. This is not
correct - SetWithMeta could create a new mutation that differs
from what Item in the expiry pager is holding onto - but the
CAS could match.

This commit ensures all metadata is compared (and seqno) when
considering if an Item matches the StoredValue.

Change-Id: I7fa20a4b9156cf12ad3c0e7c782e1751994ed735
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/234233
Reviewed-by: Mohammad Zaeem <[email protected]>
Well-Formed: Restriction Checker
Tested-by: Jim Walker <[email protected]>

Author: jimwwalker

engines/ep/src/stored-value.cc

@@ -515,6 +515,13 @@ void StoredValue::setCompletedOrDeletedTime(time_t time) {
     }
 }
 
+bool StoredValue::compareSeqnoAndMetaData(const Item& itm) const {
+    const auto metaData = itm.getMetaData();
+    return itm.getBySeqno() == getBySeqno() && getCas() == metaData.cas &&
+           getRevSeqno() == metaData.revSeqno &&
+           getExptime() == metaData.exptime && getFlags() == metaData.flags;
+}
+
 void to_json(nlohmann::json& json, const StoredValue& sv) {
     // Add any additional OrderedStoredValue data if required.
     if (sv.isOrdered()) {

engines/ep/src/stored-value.h

@@ -910,6 +910,16 @@ class StoredValue {
      */
     void setCompletedOrDeletedTime(time_t time);
 
+    /**
+     * Compare an Item againt a StoredValue (the key is not compared and is
+     * assumed the caller found this StoredValue from the key)
+     *
+     * @param itm The Item to compare against
+     * @return True if the Item's seqno, cas, revSeqno, exptime, and flags
+     *         match the StoredValue's seqno, cas, revSeqno, exptime, and flags
+     */
+    bool compareSeqnoAndMetaData(const Item& itm) const;
+
 protected:
     /**
      * Constructor - protected as allocation needs to be done via

engines/ep/src/vbucket.cc

@@ -2552,7 +2552,7 @@ std::unique_ptr<CompactionBGFetchItem> VBucket::processExpiredItem(
     auto& hbl = htRes.getHBL();
 
     if (v) {
-        if (v->getCas() != it.getCas()) {
+        if (!v->compareSeqnoAndMetaData(it)) {
             return nullptr;
         }
 

engines/ep/tests/module_tests/kv_bucket_test.cc

@@ -1393,8 +1393,9 @@ TEST_P(KVBucketParamTest, unlockKeyTempDeletedTest) {
     EXPECT_EQ(cb::engine_errc::success, gv.getStatus());
 
     // Need the "real" documents' CAS for expiry.
-    itm.setCas(docCas);
-    store->processExpiredItem(itm, ep_real_time() + 10001, ExpireBy::Pager);
+    gv.item->setCas(docCas);
+    store->processExpiredItem(
+            *gv.item, ep_real_time() + 10001, ExpireBy::Pager);
 
     flushVBucketToDiskIfPersistent(vbid, 1);
 

engines/ep/tests/module_tests/stored_value_test.cc

@@ -565,3 +565,10 @@ TYPED_TEST(StoredValueProtectedTest, MB_32835) {
 
     EXPECT_EQ(*this->sv, *sv2);
 }
+
+TYPED_TEST(ValueTest, MB_68655_compareSvAndItem) {
+    auto sv2 = this->factory(this->item, {});
+    EXPECT_TRUE(this->sv->compareSeqnoAndMetaData(this->item));
+    this->item.setBySeqno(this->item.getBySeqno() + 1);
+    EXPECT_FALSE(this->sv->compareSeqnoAndMetaData(this->item));
+}