[BP] MB-41255: Don't bgfetch for a replica delete of an xattr

MB-36087 identified and fixed an issue with "del_with_meta" where
a delete against an evicted xattr crashed because we skipped doing
a bgfetch, KV crashed because the deleteWithMeta function went on
to try and prune the xattrs of the existing key, but had a null value.

An unexpected outcome of this fix is that DCP can now be exposed
to "would block" errors, this is because "del_with_meta" and DCP delete
share the same function.

MB-41255 demonstrates what happens when the consumer is told "would
block" for a delete. The DCP consumer waits for the bgfetch and then
retries the delete, but the consumer state machine was already moved
along by the initial delete, hence we see 'out of order' seqno errors
and the delete failing.

Note for backport: unit-test is rewritten as the original was using
test harness code that is not part of the alice branch

Change-Id: I6d9c52a0fca0ce7ab0cb201025c8f6057b681481
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/136003
Well-Formed: Build Bot <[email protected]>
Tested-by: Build Bot <[email protected]>
Reviewed-by: Daniel Owen <[email protected]>

Author: jimwwalker

engines/ep/src/vbucket.cc

@@ -1480,7 +1480,8 @@ ENGINE_ERROR_CODE VBucket::deleteWithMeta(
         } else {
             delrv = MutationStatus::NotFound;
         }
-    } else if (mcbp::datatype::is_xattr(v->getDatatype()) && !v->isResident()) {
+    } else if (!isReplication &&
+               mcbp::datatype::is_xattr(v->getDatatype()) && !v->isResident()) {
         // MB-25671: A temp deleted xattr with no value must be fetched before
         // the deleteWithMeta can be applied.
         delrv = MutationStatus::NeedBgFetch;

engines/ep/tests/module_tests/dcp_test.cc

@@ -4380,6 +4380,82 @@ TEST_P(SingleThreadedActiveStreamTest, CompleteBackfillRaceNoStreamEnd) {
     EXPECT_TRUE(producer->getReadyQueue().empty());
 }
 
+// Version of MB_41255 from back-ported fix, original test was added to
+// dcp_reflection_test.cc but used a significantly different version of that
+// test code.
+// Test ensures that a DCP deletion of an evicted mutation with xattr value
+// does not error (ewouldblock seen in MB-41255)
+TEST_F(SingleThreadedEPBucketTest, MB_41255) {
+    // Make vbucket replica so can add passive stream
+    setVBucketStateAndRunPersistTask(vbid, vbucket_state_replica);
+
+    const void* cookie = create_mock_cookie();
+    auto consumer =
+            std::make_shared<MockDcpConsumer>(*engine, cookie, "test_consumer");
+
+    // Add passive stream
+    ASSERT_EQ(ENGINE_SUCCESS,
+              consumer->addStream(/*opaque*/ 0,
+                                  vbid,
+                                  /*flags*/ 0));
+
+    consumer->snapshotMarker(/*opaque*/ 1,
+                             /*vbucket*/ vbid,
+                             /*start_seqno*/ 1,
+                             /*end_seqno*/ 1,
+                             /*flags set to MARKER_FLAG_MEMORY*/ 0x5);
+
+    // Store value with an xattr
+    StoredDocKey key{"k1", DocNamespace::DefaultCollection};
+    auto data = createXattrValue(R"({"json":"yes"})");
+    cb::const_byte_buffer value{reinterpret_cast<const uint8_t*>(data.data()),
+                                data.size()};
+    consumer->mutation(1, // opaque
+                       key,
+                       value,
+                       0, // priv bytes
+                       PROTOCOL_BINARY_DATATYPE_XATTR,
+                       1, // cas
+                       vbid, // vbucket
+                       0, // flags
+                       1, // by_seqno
+                       0, // rev seqno
+                       0, // expiration
+                       0, // lock time
+                       {}, // meta
+                       0); // nru
+
+    // flush and evict
+    flushVBucketToDiskIfPersistent(vbid, 1);
+    auto replicaVB = engine->getKVBucket()->getVBucket(vbid);
+
+    const char* msg;
+    EXPECT_EQ(PROTOCOL_BINARY_RESPONSE_SUCCESS,
+
+              replicaVB->evictKey(key, &msg));
+
+    // now delete the key
+    consumer->snapshotMarker(/*opaque*/ 1,
+                             /*vbucket*/ vbid,
+                             /*start_seqno*/ 2,
+                             /*end_seqno*/ 2,
+                             /*flags*/ 5);
+    EXPECT_EQ(ENGINE_SUCCESS,
+              consumer->deletion(/*opaque*/ 1,
+                                 /*key*/ key,
+                                 /*value*/ {},
+                                 /*priv_bytes*/ 0,
+                                 /*datatype*/ PROTOCOL_BINARY_RAW_BYTES,
+                                 /*cas*/ 1,
+                                 /*vbucket*/ vbid,
+                                 /*bySeqno*/ 2,
+                                 /*revSeqno*/ 0,
+                                 /*meta*/ {}));
+    // Close stream
+    ASSERT_EQ(ENGINE_SUCCESS, consumer->closeStream(/*opaque*/ 0, vbid));
+    destroy_mock_cookie(cookie);
+}
+
 INSTANTIATE_TEST_CASE_P(AllBucketTypes,
                         SingleThreadedActiveStreamTest,
                         allConfigValues, );