MB-51413: Add NotFound status to CouchKVStore readVBState

We should differentiate between Success and NotFound so that we
can avoid using a default constructed state if one was not found.

Change-Id: Icf0bc3c703ed03a210a3d67d06b5f0f23957566e
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/172252
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: BenHuddleston

engines/ep/src/kvstore/couch-kvstore/couch-kvstore.cc

@@ -202,6 +202,8 @@ std::string to_string(CouchKVStore::ReadVBStateStatus status) {
     switch (status) {
     case CouchKVStore::ReadVBStateStatus::Success:
         return "Success";
+    case CouchKVStore::ReadVBStateStatus::NotFound:
+        return "NotFound";
     case CouchKVStore::ReadVBStateStatus::JsonInvalid:
         return "JsonInvalid";
     case CouchKVStore::ReadVBStateStatus::CorruptSnapshot:
@@ -1472,7 +1474,11 @@ CompactDBStatus CouchKVStore::compactDBInternal(
                 delta.count(),
                 [this, &vbid, hook_ctx](Db& db) {
                     auto [status, state] = readVBState(&db, vbid);
-                    if (status != ReadVBStateStatus::Success) {
+                    // @TODO MB-51037: We probably shouldn't fail compactions
+                    // externally if a vBucket state does not exist in the
+                    // header we have picked up....
+                    if (status != ReadVBStateStatus::Success &&
+                        status != ReadVBStateStatus::NotFound) {
                         return COUCHSTORE_ERROR_CANCEL;
                     }
                     hook_ctx->highCompletedSeqno =
@@ -1490,7 +1496,11 @@ CompactDBStatus CouchKVStore::compactDBInternal(
                 },
                 [this, &vbid, &prepareStats, &purge_seqno](Db& db) {
                     auto [status, state] = readVBState(&db, vbid);
-                    if (status != ReadVBStateStatus::Success) {
+                    // @TODO MB-51037: We probably shouldn't fail compactions
+                    // externally if a vBucket state does not exist in the
+                    // header we have picked up....
+                    if (status != ReadVBStateStatus::Success &&
+                        status != ReadVBStateStatus::NotFound) {
                         return COUCHSTORE_ERROR_CANCEL;
                     }
                     prepareStats.onDiskPrepares = state.onDiskPrepares;
@@ -3290,10 +3300,7 @@ CouchKVStore::ReadVBStateResult CouchKVStore::readVBState(Db* db,
                 "CouchKVStore::readVBState: '_local/vbstate' not found "
                 "for {}",
                 vbId);
-
-        // Read was successful, the state just didn't exist. Return success
-        // and a default constructed vbucket_state
-        return {ReadVBStateStatus::Success, {}};
+        return {ReadVBStateStatus::NotFound, {}};
     }
 
     if (couchStoreStatus != COUCHSTORE_SUCCESS) {
@@ -3394,6 +3401,12 @@ CouchKVStore::ReadVBStateResult CouchKVStore::readVBStateAndUpdateCache(
         }
     }
 
+    // @TODO MB-51413: NotFound should probably not load a state, but for now
+    // we'd segfault higher up the stack during initialize if we don't allow it.
+    if (res.status == ReadVBStateStatus::NotFound) {
+        res.status = ReadVBStateStatus::Success;
+    }
+
     if (res.status == ReadVBStateStatus::Success) {
         // For the case where a local doc does not exist, readVBState actually
         // returns Success as the read was successful. It also returns a default
@@ -4242,7 +4255,11 @@ vbucket_state CouchKVStore::getPersistedVBucketState(Vbid vbid) const {
     }
 
     const auto res = readVBState(db, vbid);
-    if (res.status != ReadVBStateStatus::Success) {
+    // @TODO MB-51413 Expand the status returned by this. NotFound is valid
+    // in certain circumstances and we should neither throw nor return the
+    // default constructed vbucket_state.
+    if (res.status != ReadVBStateStatus::Success &&
+        res.status != ReadVBStateStatus::NotFound) {
         throw std::logic_error(
                 "CouchKVStore::getPersistedVBucketState: readVBState error:" +
                 to_string(res.status) +

engines/ep/src/kvstore/couch-kvstore/couch-kvstore.h

@@ -152,6 +152,7 @@ class CouchKVStore : public KVStore
 
     enum class ReadVBStateStatus : uint8_t {
         Success = 0,
+        NotFound,
         JsonInvalid,
         CorruptSnapshot,
         CouchstoreError

engines/ep/tests/module_tests/couch-kvstore_test.cc

@@ -934,6 +934,18 @@ TEST_F(CouchKVStoreErrorInjectionTest, compactDB_compact_db_ex) {
 }
 
 TEST_F(CouchKVStoreErrorInjectionTest, compactDB_compact_db_ex_pitr) {
+    populate_items(1);
+
+    // This test finds an old header that lacks a vbucket_state if we run it
+    // without any pre-amble. Find a header without a vbucket_state causes the
+    // compaction to fail and to abort before we hit our expectations in the
+    // test. To run this test we need to sleep for more than the max history age
+    // because the PiTR code in couchstore is using the system clock...
+    // @TODO MB-45408: we should not be sleeping in tests.
+    // @TODO MB-51037: We probably should not fail this compaction if we pick
+    //  up a header that does not have a vbucket_state.
+    std::this_thread::sleep_for(std::chrono::seconds(1));
+
     config.setPitrEnabled(true);
     config.setPitrGranularity(std::chrono::nanoseconds(1000));
     config.setPitrMaxHistoryAge(std::chrono::seconds(1));

tests/testapp_cluster/pitr_tests.cc

@@ -31,17 +31,24 @@ TEST_F(PiTR_Test, MB51007) {
     // The (initial) sync write may take longer time than the default timeout
     // so lets bump the timeout
     conn->setReadTimeout(std::chrono::seconds{1000});
-    conn->store("MB51007",
-                Vbid{0},
-                bucket->getCollectionManifest().dump(),
-                cb::mcbp::Datatype::JSON,
-                0,
-                []() {
-                    FrameInfoVector ret;
-                    ret.emplace_back(std::make_unique<DurabilityFrameInfo>(
-                            cb::durability::Level::MajorityAndPersistOnMaster));
-                    return ret;
-                });
+    auto info = conn->store(
+            "MB51007",
+            Vbid{0},
+            bucket->getCollectionManifest().dump(),
+            cb::mcbp::Datatype::JSON,
+            0,
+            []() {
+                FrameInfoVector ret;
+                ret.emplace_back(std::make_unique<DurabilityFrameInfo>(
+                        cb::durability::Level::MajorityAndPersistOnMaster));
+                return ret;
+            });
+
+    // Wait for the persistence of our item
+    ObserveInfo observeInfo;
+    do {
+        observeInfo = conn->observeSeqno(Vbid(0), info.vbucketuuid);
+    } while (observeInfo.lastPersistedSeqno != info.seqno);
 
     // Let the test run for 10 seconds
     const auto timeLimit = std::chrono::seconds{10};
@@ -54,11 +61,14 @@ TEST_F(PiTR_Test, MB51007) {
                 conn->setReadTimeout(timeLimit);
                 do {
                     auto rsp = conn->execute(BinprotCompactDbCommand{});
-                    ASSERT_TRUE(rsp.isSuccess())
-                            << "Compaction failed for some reason: "
-                            << to_string(rsp.getStatus()) << std::endl
-                            << rsp.getDataString();
-                    ++num_compaction;
+                    if (rsp.isSuccess()) {
+                        // @TODO MB-51037:
+                        // Compaction of older headers may not find a
+                        // vbucket_state and that currently causes a compaction
+                        // to fail. We should probably handle that better in
+                        // the future.
+                        ++num_compaction;
+                    }
                 } while (std::chrono::steady_clock::now() < timeout);
             }};