MB-35326: Reset cached vbucket_state on VBucket creation

The cached vbucket_state is unconditionally populated with
the state of all vbuckets found on disk during bucket creation.

However not all vbuckets found will become VBucket objects (dead
state vbuckets aren't created), so we may later create new
VBucket objects in their default state (e.g. snapshot range of
0,0).

Resetting the cached vbucket_state on creation means that the
cached state will be consistent with the new VBucket.

Change-Id: I122c21f34f1eca129a2ff4bc29f42f96645bb8d2
Reviewed-on: http://review.couchbase.org/112789
Reviewed-by: Dave Rigby <[email protected]>
Tested-by: Build Bot <[email protected]>

Author: jimwwalker

engines/ep/src/kv_bucket.cc

@@ -1012,6 +1012,10 @@ ENGINE_ERROR_CODE KVBucket::setVBucketState_UNLOCKED(
         // Before adding the VB to the map increment the revision
         getRWUnderlying(vbid)->incrementRevision(vbid);
 
+        // Wipe out any cached vbucket state
+        getRWUnderlying(vbid)->resetCachedVBState(vbid);
+        getROUnderlying(vbid)->resetCachedVBState(vbid);
+
         // If active, update the VB from the bucket's collection state.
         // Note: Must be done /before/ adding the new VBucket to vbMap so that
         // it has the correct collections state when it is exposed to operations

engines/ep/src/kvstore.cc

@@ -491,11 +491,15 @@ uint64_t KVStore::getLastPersistedSeqno(Vbid vbid) {
 
 uint64_t KVStore::prepareToDelete(Vbid vbid) {
     // MB-34380: We must clear the cached state
+    resetCachedVBState(vbid);
+    return prepareToDeleteImpl(vbid);
+}
+
+void KVStore::resetCachedVBState(Vbid vbid) {
     vbucket_state* state = getVBucketState(vbid);
     if (state) {
         state->reset();
     }
-    return prepareToDeleteImpl(vbid);
 }
 
 void KVStore::setSystemEvent(const Item& item, SetCallback cb) {

engines/ep/src/kvstore.h

@@ -909,6 +909,13 @@ class KVStore {
      */
     uint64_t prepareToDelete(Vbid vbid);
 
+    /**
+     * Call reset on the cached vbucket_state for the given vbucket (if any is
+     * cached)
+     * @param vbid ID of the vbucket to reset
+     */
+    void resetCachedVBState(Vbid vbid);
+
     /**
      * Set a system event into the KVStore.
      * Collection system events will be used to maintain extra meta-data before

engines/ep/tests/module_tests/evp_store_warmup_test.cc

@@ -1243,6 +1243,37 @@ TEST_P(MB_34718_WarmupTest, getTest) {
     EXPECT_EQ(0, vb->getNumItems());
 }
 
+// Perform the sequence of operations which lead to MB-35326, a snapshot range
+// exception. When the issue is fixed this test will pass
+TEST_F(WarmupTest, MB_35326) {
+    // 1) Write an item to an active vbucket and flush it.
+    //    vb state on disk will have a range of {1,1}
+    setVBucketStateAndRunPersistTask(vbid, vbucket_state_active);
+    auto key = makeStoredDocKey("key");
+    store_item(vbid, key, "value");
+    flush_vbucket_to_disk(vbid);
+
+    // 2) Mark the vbucket as dead and persist the state
+    setVBucketStateAndRunPersistTask(vbid, vbucket_state_dead);
+
+    // 3) Warmup - the dead vbucket will be skipped by warmup but KVStore has
+    //    loaded the state into cachedVBStates
+    resetEngineAndWarmup();
+
+    EXPECT_FALSE(engine->getVBucket(vbid)) << "Dead vbuckets shouldn't warmup";
+
+    // 4) Now active creation, this results in a new VBucket object with default
+    //    state, for this issue the snapshot range of {0,0}
+    setVBucketStateAndRunPersistTask(vbid, vbucket_state_active);
+
+    // 5) Store an item and flush, this would crash because we combine the in
+    //    memory range {0,0} with the on disk range {1,1}, the crash occurs
+    //    as the new range is {1, 0} and start:1 cannot be greater than end:0.
+    store_item(vbid, key, "value");
+
+    flush_vbucket_to_disk(vbid);
+}
+
 INSTANTIATE_TEST_CASE_P(FullOrValue,
                         MB_34718_WarmupTest,
                         STParameterizedBucketTest::persistentConfigValues(),