MB-21448: replace-with-CAS should return NOT_FOUND for deleted items

Regression test for MB-21448 - if an attempt is made to perform a CAS
operation on a logically deleted item we should return NOT_FOUND (aka
KEY_ENOENT) and *not* INVALID_CAS (aka KEY_EEXISTS).

The reason for this "intermittent" behaviour is that we are
essentially racing with the writer thread. The sequence of events for
deleting an item is as follows:

1. User calls DELETE, which locates the key in the HashTable, and then
   sets the deleted flag on that StoredValue (but crucially leaves the
   StoredValue in the HashTable). A deletion operation is then
   enqueued in the CheckpointManager (to write the deletion to disk).

2. When the flusher runs for this vBucket, it calls
   CouchKVStore::commit which after committing invokes
   PersistenceCallback::callback to inform ep-engine that the item has
   been persisted to disk (and we can now remove from the HashTable).

3. The callback locates the item in the HashTable and finally
   physically removes the StoredValue.

The issue here is the client is essentially racing with (3) above. If
the client replace-with-CAS operation is processed before (3), then
the StoredValue is still in the HashTable, and hence unlocked_set
returns NOT_FOUND (aka KEY_ENOENT):

    } else if (cas && cas != v->getCas()) {  // line 1078
        if (v->isTempDeletedItem() || v->isTempNonExistentItem()) {
            return NOT_FOUND;
        }

If on the other hand the client replace-with-CAS operation is
processed after (3), then the StoredValue no longer exists in the
HashTable, and we instead hit this case where v is NULL:

    if (v) {
        ...
    } else if (cas != 0) {  // line 1102
        rv = NOT_FOUND;

Solution is to make the unlocked_set behave as if an item doesn't
exist (i.e. post (3) state), even if it's currently present but
deleted.

Change-Id: If353a310e5096dc838c6db9f673d85515d090842
Reviewed-on: http://review.couchbase.org/69080
Reviewed-by: Daniel Owen <owend@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>

diff --git a/src/stored-value.h b/src/stored-value.h
index acf8b0f..a324686 100644 (file)
--- a/src/stored-value.h
+++ b/src/stored-value.h
@@ -1076,7 +1076,9 @@ public:
                 /* allow operation*/
                 v->unlock();
             } else if (cas && cas != v->getCas()) {
-                if (v->isTempDeletedItem() || v->isTempNonExistentItem()) {
+                if (v->isTempDeletedItem() ||
+                    v->isTempNonExistentItem() ||
+                    v->isDeleted()) {
                     return NOT_FOUND;
                 }
                 return INVALID_CAS;

diff --git a/tests/ep_testsuite_xdcr.cc b/tests/ep_testsuite_xdcr.cc
index 97753f9..077d8a5 100644 (file)
--- a/tests/ep_testsuite_xdcr.cc
+++ b/tests/ep_testsuite_xdcr.cc
@@ -1122,8 +1122,12 @@ static enum test_result test_set_with_meta_race_with_delete(ENGINE_HANDLE *h, EN
 
     // attempt set_with_meta. should fail since cas is no longer valid.
     set_with_meta(h, h1, key1, keylen1, NULL, 0, 0, &last_meta, last_cas, true);
-    checkeq(PROTOCOL_BINARY_RESPONSE_KEY_EEXISTS, last_status.load(),
-          "Expected invalid cas error");
+
+    checkeq(PROTOCOL_BINARY_RESPONSE_KEY_ENOENT, last_status.load(),
+            (std::string{"Expected invalid cas error (KEY_EXISTS or"
+                         " KEY_ENOENT), got: "} +
+             std::to_string(last_status.load())).c_str());
+
     // check the stat
     temp = get_int_stat(h, h1, "ep_num_ops_set_meta");
     check(temp == 0, "Expect zero op");

diff --git a/tests/module_tests/hash_table_test.cc b/tests/module_tests/hash_table_test.cc
index 0bfe809..e85fff2 100644 (file)
--- a/tests/module_tests/hash_table_test.cc
+++ b/tests/module_tests/hash_table_test.cc
@@ -585,6 +585,26 @@ TEST_F(HashTableTest, ItemAge) {
     EXPECT_EQ(1, v->getValue()->getAge());
 }
 
+/** Regression test for MB-21448 - if an attempt is made to perform a CAS
+ *  operation on a logically deleted item we should return NOT_FOUND
+ *  (aka KEY_ENOENT) and *not* INVALID_CAS (aka KEY_EEXISTS).
+ */
+TEST_F(HashTableTest, MB21448_UnlockedSetWithCASDeleted) {
+    // Setup - create a key and then delete it.
+    HashTable ht(global_stats, 5, 1);
+    std::string key("key");
+    Item item(key.data(), key.length(), 0, 0, "deleted", strlen("deleted"));
+    ASSERT_EQ(WAS_CLEAN, ht.set(item));
+    ASSERT_EQ(WAS_DIRTY, ht.softDelete("key", 0));
+
+    // Attempt to perform a set on a deleted key with a CAS.
+    Item replacement(key.data(), key.length(), 0, 0, "value", strlen("value"));
+    EXPECT_EQ(NOT_FOUND,
+              ht.set(replacement, /*cas*/10, /*allowExisting*/true,
+                     /*hasMetaData*/false))
+        << "When trying to replace-with-CAS a deleted item";
+}
+
 /* static storage for environment variable set by putenv().
  *
  * (This must be static as putenv() essentially 'takes ownership' of