MB-52793: Ensure StoredValue::del updates datatype

If a StoredValue is deleted, but does not have a resident value (but
_may_ have one on disk, containing xattrs), previously `del`
erroneously skipped updating the datatype.

This situation has only been observed to occur on replicas, via
PassiveStream calling deleteWithMeta for an already deleted item. This
may occur when xattrs are removed from a deleted document.

See MB for more details.

Change-Id: I213cefb3907c4e290c2857c8526477f69a9ce764
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/177197
Well-Formed: Restriction Checker
Reviewed-by: Trond Norbye <[email protected]>
Tested-by: Build Bot <[email protected]>

Author: jameseh96

engines/ep/src/kv_bucket.h

@@ -724,6 +724,10 @@ class KVBucket : public KVBucketIface {
 
     cb::durability::Level getMinDurabilityLevel() const;
 
+    EvictionPolicy getEvictionPolicy() const {
+        return eviction_policy;
+    }
+
 protected:
     GetValue getInternal(const DocKey& key,
                          Vbid vbucket,

engines/ep/src/stored-value.cc

@@ -323,12 +323,6 @@ bool StoredValue::operator!=(const StoredValue& other) const {
 }
 
 bool StoredValue::deleteImpl(DeleteSource delSource) {
-    if (isDeleted() && !getValue()) {
-        // SV is already marked as deleted and has no value - no further
-        // deletion possible.
-        return false;
-    }
-
     resetValue();
     setDatatype(PROTOCOL_BINARY_RAW_BYTES);
     setPendingSeqno();

engines/ep/tests/module_tests/evp_store_single_threaded_test.cc

@@ -5855,3 +5855,144 @@ INSTANTIATE_TEST_CASE_P(Persistent,
                         STParamPersistentBucketTest,
                         STParameterizedBucketTest::persistentConfigValues(),
                         STParameterizedBucketTest::PrintToStringParamName);
+
+TEST_P(STParamPersistentBucketTest,
+       RemovingXattrDoesNotCauseIncorrectDatatypeOnReplica) {
+    // MB-52793: Under value eviction, if a replica:
+    // 1. Is a version without the fix for MB-50423 (6.6.5, 7.0.4 and down)
+    // 2. Has metadata for a deleted item with xattrs, but a non-resident value
+    //    (e.g., has been bgfetched and evicted)
+    // 3. Receives a deletion over DCP changing a document from
+    //    Xattrs+value -> no xattrs+no value
+    // The replica will persist a deleted item with datatype=xattrs but no
+    // value.
+    // NOTE: this has already been (incidentally) prevented by the fix for
+    //       MB-50423 in some versions, this test is to guard against future
+    //       regressions in this specific case.
+
+    setVBucketStateAndRunPersistTask(vbid, vbucket_state_replica);
+
+    // 1) Store an item with a (system) xattr
+    auto key = makeStoredDocKey("key");
+
+    cb::xattr::Blob xattrBlob;
+
+    xattrBlob.set("_sync", "somexattrvalue");
+    auto xattrs = xattrBlob.finalize();
+    auto xattrsStr = std::string(xattrs.begin(), xattrs.end());
+
+    // store the deleted item
+    {
+        auto item = make_item(
+                vbid, key, xattrsStr, 0, PROTOCOL_BINARY_DATATYPE_XATTR);
+
+        item.setCas(1234);
+        item.setDeleted(DeleteSource::Explicit);
+
+        uint64_t seqno;
+        ASSERT_EQ(ENGINE_SUCCESS,
+                  store->setWithMeta(item,
+                                     0 /* cas */,
+                                     &seqno,
+                                     cookie,
+                                     {vbucket_state_replica},
+                                     CheckConflicts::No,
+                                     /*allowExisting*/ true));
+    }
+
+    flushVBucketToDiskIfPersistent(vbid, 1);
+
+    auto vb = store->getVBucket(vbid);
+    auto& ht = vb->ht;
+
+    // persistence callback removes the delete from the HT
+    {
+        auto res = ht.findForRead(key,
+                                  TrackReference::No,
+                                  WantsDeleted::Yes,
+                                  ForGetReplicaOp::No);
+        ASSERT_FALSE(res.storedValue);
+    }
+
+    {
+        // cause the deleted item to be bgfetched back into the HT
+        get_options_t options =
+                static_cast<get_options_t>(QUEUE_BG_FETCH | GET_DELETED_VALUE);
+        auto gv = store->get(key, vbid, cookie, options);
+        ASSERT_EQ(ENGINE_EWOULDBLOCK, gv.getStatus());
+
+        runBGFetcherTask();
+        gv = store->get(key, vbid, cookie, options);
+        ASSERT_EQ(ENGINE_SUCCESS, gv.getStatus());
+        ASSERT_EQ(PROTOCOL_BINARY_DATATYPE_XATTR, gv.item->getDataType());
+        ASSERT_NE(0, gv.item->getNBytes());
+        ASSERT_EQ(xattrsStr, gv.item->getValue()->to_s());
+    }
+
+    // now evict the value
+    {
+        auto res = ht.findForWrite(key, WantsDeleted::Yes);
+        ht.unlocked_ejectItem(
+                res.lock, res.storedValue, store->getEvictionPolicy());
+    }
+
+    // check it exists in the desired state
+    {
+        auto res = ht.findForRead(key,
+                                  TrackReference::No,
+                                  WantsDeleted::Yes,
+                                  ForGetReplicaOp::No);
+        const auto* v = res.storedValue;
+        if (fullEviction()) {
+            // Item should be entirely removed.
+            EXPECT_FALSE(v);
+        } else {
+            // Item should still be present.
+            ASSERT_TRUE(v);
+            ASSERT_TRUE(v->isDeleted());
+            ASSERT_FALSE(v->isResident());
+            ASSERT_EQ(PROTOCOL_BINARY_DATATYPE_XATTR, v->getDatatype());
+        }
+    }
+
+    // now drop the xattrs and store again, as if the only xattr has been
+    // removed by subdoc (as per what SyncGW does).
+    {
+        auto item = make_item(vbid, key, "", 0, PROTOCOL_BINARY_RAW_BYTES);
+        item.setCas(5678);
+        item.setDeleted(DeleteSource::Explicit);
+
+        uint64_t cas = 0;
+        uint64_t seqno;
+
+        EXPECT_EQ(ENGINE_SUCCESS,
+                  store->deleteWithMeta(key,
+                                        cas,
+                                        &seqno,
+                                        vbid,
+                                        cookie,
+                                        {vbucket_state_replica},
+                                        CheckConflicts::No,
+                                        item.getMetaData(),
+                                        GenerateBySeqno::Yes,
+                                        GenerateCas::No,
+                                        0,
+                                        nullptr,
+                                        DeleteSource::Explicit));
+    }
+
+    // At this point the damage has been done - the document has been corrupted
+    // (zero byte value but datatype==XATTR) both on-disk and in-memory.
+
+    // MB-50423: The item _must_ have datatype RAW_BYTES now, as it does not
+    //           have any xattrs or a value.
+    {
+        auto res = ht.findForRead(key,
+                                  TrackReference::No,
+                                  WantsDeleted::Yes,
+                                  ForGetReplicaOp::No);
+        const auto* v = res.storedValue;
+        EXPECT_TRUE(v->isDeleted());
+        EXPECT_EQ(PROTOCOL_BINARY_RAW_BYTES, v->getDatatype());
+    }
+}

engines/ep/tests/module_tests/stored_value_test.cc

@@ -26,6 +26,7 @@
 #include "stats.h"
 #include "stored_value_factories.h"
 #include "tests/module_tests/test_helpers.h"
+#include <xattr/blob.h>
 
 #include <folly/portability/GTest.h>
 
@@ -436,6 +437,43 @@ TYPED_TEST(ValueTest, MB_32568) {
     EXPECT_EQ(DeleteSource::TTL, this->sv->getDeletionSource());
 }
 
+TYPED_TEST(ValueTest, DeleteUpdatesDatatype) {
+    // MB-52793: Ensure that calling StoredValue::del() on a non-resident,
+    // deleted item correctly changes the datatype to RAW_BYTES.
+    // See MB/patch for scenario and other tests.
+    auto key = makeStoredDocKey("key");
+
+    cb::xattr::Blob xattrBlob;
+    xattrBlob.set("_sync", "somexattrvalue");
+    auto xattrs = xattrBlob.finalize();
+    std::string value = std::string(xattrs.begin(), xattrs.end());
+
+    auto item = make_item(Vbid(0),
+                          key,
+                          value,
+                          0 /* expiry */,
+                          PROTOCOL_BINARY_DATATYPE_XATTR);
+
+    item.setDeleted(DeleteSource::Explicit);
+
+    // create a deleted stored value with xattrs
+    auto sv = this->factory(item, {});
+
+    sv->ejectValue();
+
+    EXPECT_FALSE(sv->isResident());
+    EXPECT_EQ(PROTOCOL_BINARY_DATATYPE_XATTR, sv->getDatatype());
+
+    // delete the stored value "again". MB-52793: this occurred when removing
+    // xattrs from a deleted document, leaving it with no value, but erroneously
+    // keeping xattr datatype. See MB.
+    sv->del(DeleteSource::Explicit);
+
+    // check the datatype has correctly changed, even though the value is
+    // not resident.
+    EXPECT_EQ(PROTOCOL_BINARY_RAW_BYTES, sv->getDatatype());
+}
+
 /**
  * Test fixture for OrderedStoredValue-only tests.
  */