MB-48841: completeCompactionExpiryBgFetch must check vbstate

Any expiry bgfetch must not be applied unless the vbucket is
active.

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

Author: jimwwalker

engines/ep/src/ep_vb.cc

@@ -242,6 +242,10 @@ void EPVBucket::completeCompactionExpiryBgFetch(
     { // locking scope
         auto docKey = key.getDocKey();
         folly::SharedMutex::ReadHolder rlh(getStateLock());
+        if (getState() != vbucket_state_active) {
+            return;
+        }
+
         auto cHandle = lockCollections(docKey);
         if (!cHandle.valid()) {
             return;

engines/ep/tests/module_tests/evp_store_test.cc

@@ -677,7 +677,7 @@ TEST_P(EPBucketFullEvictionTest, ExpiryFindNonResidentItem) {
 }
 
 void EPBucketFullEvictionTest::compactionFindsNonResidentItem(
-        bool dropCollection) {
+        bool dropCollection, bool switchToReplica) {
     EXPECT_EQ(cb::engine_errc::success,
               store->setVBucketState(vbid, vbucket_state_active, {}));
 
@@ -723,34 +723,56 @@ void EPBucketFullEvictionTest::compactionFindsNonResidentItem(
     // We should have queued a BGFetch for the item
     EXPECT_EQ(1, vb->getNumItems());
     ASSERT_TRUE(vb->hasPendingBGFetchItems());
+
+    auto highSeqno = vb->getHighSeqno();
+    if (switchToReplica) {
+        EXPECT_EQ(cb::engine_errc::success,
+                  store->setVBucketState(vbid, vbucket_state_replica, {}));
+    }
+
     runBGFetcherTask();
-    EXPECT_FALSE(vb->hasPendingBGFetchItems());
 
-    // We should have expired the item
-    EXPECT_EQ(expectedExpiredItems, vb->numExpiredItems);
+    if (!switchToReplica) {
+        EXPECT_FALSE(vb->hasPendingBGFetchItems());
 
-    // But it still exists on disk until we flush
-    EXPECT_EQ(1, vb->getNumItems());
+        // We should have expired the item
+        EXPECT_EQ(expectedExpiredItems, vb->numExpiredItems);
 
-    auto expectedItems = 0;
-    if (isRocksDB() || dropCollection) {
-        // RocksDB doesn't know if we insert or update so item counts are not
-        // correct.
-        // Or if we drop the collection, no expiry. Item count won't be updated
-        // until collections are purged, so 1 is correct.
-        expectedItems = 1;
+        // But it still exists on disk until we flush
+        EXPECT_EQ(1, vb->getNumItems());
+
+        auto expectedItems = 0;
+        if (isRocksDB() || dropCollection) {
+            // RocksDB doesn't know if we insert or update so item counts are
+            // not correct. Or if we drop the collection, no expiry. Item count
+            // won't be updated until collections are purged, so 1 is correct.
+            expectedItems = 1;
+            if (dropCollection) {
+                EXPECT_EQ(highSeqno, vb->getHighSeqno());
+            }
+        } else {
+            EXPECT_LT(highSeqno, vb->getHighSeqno());
+            flushVBucketToDiskIfPersistent(vbid, 1);
+        }
+        EXPECT_EQ(expectedItems, vb->getNumItems());
     } else {
-        flushVBucketToDiskIfPersistent(vbid, 1);
+        EXPECT_EQ(0, vb->numExpiredItems);
+        EXPECT_EQ(highSeqno, vb->getHighSeqno());
     }
-    EXPECT_EQ(expectedItems, vb->getNumItems());
 }
 
 TEST_P(EPBucketFullEvictionTest, CompactionFindsNonResidentItem) {
-    compactionFindsNonResidentItem(false);
+    compactionFindsNonResidentItem(false, false);
 }
 
 TEST_P(EPBucketFullEvictionTest, MB_42295_dropCollectionBeforeExpiry) {
-    compactionFindsNonResidentItem(true);
+    compactionFindsNonResidentItem(true, false);
+}
+
+TEST_P(EPBucketFullEvictionTest, MB_48841_switchToReplica) {
+    // Test will switch from active to replica, bgfetch runs whilst replica and
+    // all pending expiries must not take affect.
+    compactionFindsNonResidentItem(false, true);
 }
 
 /**

engines/ep/tests/module_tests/evp_store_test.h

@@ -33,7 +33,8 @@ class EPBucketTest : public STParameterizedBucketTest {
 // Full eviction only tests
 class EPBucketFullEvictionTest : public EPBucketTest {
 public:
-    void compactionFindsNonResidentItem(bool dropCollection);
+    void compactionFindsNonResidentItem(bool dropCollection,
+                                        bool switchToReplica);
 };
 
 // Full eviction only tests that run with bloom filters off