[BP] MB-56644: Make ItemExpel resilient to VBucket rollback

[Backport to 7.1.5]

ItemExpel isn't an atomic operation as that acquires/releases the
CM::lock multiple times. So in the middle of the Expel execution
VBucket rollback might clear the CheckpointList and thus remove the
checkpoint touched by Expel.

Before this fix assertions in CM might trigger and crash the node.
The fix makes ItemExpel resilient to the rollback scenario. ItemExpel
now is capable of identifying the rollback post-conditions and just
gives up if the checkpoint under processing doesn't exist anymore.

Change-Id: I03bf6e82ca9fb799666f5265298e5bcc3ac2b269
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/193528
Reviewed-by: Daniel Owen <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>
Well-Formed: Restriction Checker
Tested-by: Build Bot <[email protected]>

Author: paolococchi

engines/ep/src/checkpoint_manager.cc

@@ -628,16 +628,28 @@ CheckpointManager::expelUnreferencedCheckpointItems() {
     // queueLock already released here, O(N) deallocation is lock-free
     const auto queuedItemsMemReleased = extractRes.deleteItems();
 
+    // Test hook that executes before CM::lock is re-acquired
+    expelHook();
+
     {
         // Acquire the queueLock just for the very short time necessary for
-        // updating the checkpoint's queued-items mem-usage and removing the
-        // expel-cursor.
-
+        // updating the checkpoint's queued-items mem-usage.
+        //
         // Note that the presence of the expel-cursor at this step ensures that
-        // the checkpoint is still in the CheckpointList.
+        // the checkpoint is still in the CheckpointList; unless the VBucket has
+        // rolled-back (see the following).
+        // Expel-cursor is released once extractRes is destroyed at caller.
         std::lock_guard<std::mutex> lh(queueLock);
         auto* checkpoint = extractRes.getCheckpoint();
         Expects(checkpoint);
+
+        // Expel always touches the oldest checkpoint in the list.
+        // The checkpoint touched by Expel might not exist anymore if the
+        // VBucket has rolled-back. Just give up in that case.
+        if (checkpoint != checkpointList.begin()->get()) {
+            return {0, 0};
+        }
+
         Expects(extractRes.getExpelCursor().getCheckpoint()->get() ==
                 checkpoint);
         checkpoint->applyQueuedItemsMemUsageDecrement(queuedItemsMemReleased);
@@ -1965,8 +1977,7 @@ CheckpointManager::ExtractItemsResult CheckpointManager::extractItemsToExpel(
         // with the lowest seqno should be in that checkpoint.
         if (lowestCursor->getCheckpoint()->get() != oldestCheckpoint) {
             std::stringstream ss;
-            ss << "CheckpointManager::expelUnreferencedCheckpointItems: ("
-               << vb.getId()
+            ss << "CheckpointManager::extractItemsToExpel: (" << vb.getId()
                << ") lowest found cursor is not in the oldest "
                   "checkpoint. Oldest checkpoint ID: "
                << oldestCheckpoint->getId()

engines/ep/src/checkpoint_manager.h

@@ -612,6 +612,10 @@ class CheckpointManager {
     // Introduced in MB-45757 for testing a race condition on invalidate-cursor
     TestingHook<> removeCursorPreLockHook;
 
+    // Test hook that executes in the section where ItemExpel has released
+    // (and not yet re-acquired) the CM::lock. Introduced in MB-56644.
+    TestingHook<> expelHook;
+
 protected:
     /**
      * Checks if eager checkpoint removal is enabled, then checks if the

engines/ep/tests/module_tests/checkpoint_test.cc

@@ -1554,6 +1554,57 @@ TEST_F(SingleThreadedCheckpointTest, CursorDistance_ResetCursor) {
     cursor.reset();
 }
 
+TEST_F(SingleThreadedCheckpointTest, ItemExpelResilientToVBucketRollback) {
+    setVBucketState(vbid, vbucket_state_active);
+    auto& vb = *store->getVBucket(vbid);
+    auto& manager = *vb.checkpointManager;
+    ASSERT_EQ(0, vb.getHighSeqno());
+    ASSERT_EQ(0, manager.getNumOpenChkItems());
+
+    // Note: We need at least 2 mutations for triggering ItemExpel, as we can't
+    // expel items pointed from cursors
+    store_item(vbid, makeStoredDocKey("key1"), "value");
+    store_item(vbid, makeStoredDocKey("key2"), "value");
+    ASSERT_EQ(2, vb.getHighSeqno());
+    ASSERT_EQ(2, manager.getNumOpenChkItems());
+
+    // Move the cursor, allow ItemExpel
+    flushVBucketToDiskIfPersistent(vbid, 2);
+
+    // Simulate the rollback behaviour.
+    // ItemExpel plants the expel-cursor in the checkpoint under processing and
+    // releases the CM::lock. At that point VB::rollback can clear the CM
+    // removing all checkpoint. When ItemExpel resumes it needs to be resilient
+    // to that state.
+    // The CM::expelHook executes in the section where ItemExpel has released
+    // (and not yet re-acquired) the CM::lock.
+    manager.expelHook = [&manager]() { manager.clear(); };
+
+    // Before the fix for MB-56644 this step fails by exception triggered,
+    // caused by that rollback has removed the checkpoint touched by ItemExpel
+    manager.expelUnreferencedCheckpointItems();
+
+    // Note: The CM was cleared, so mem-usage must track the correct allocation
+    // for the single/empty checkpoint in CheckpointList
+    auto config = CheckpointConfig(store->getEPEngine().getConfiguration());
+    const auto emptyManager =
+            CheckpointManager(store->getEPEngine().getEpStats(),
+                              vb,
+                              config,
+                              0,
+                              0,
+                              0,
+                              0,
+                              0,
+                              nullptr,
+                              nullptr);
+    EXPECT_EQ(emptyManager.getQueuedItemsMemUsage(),
+              manager.getQueuedItemsMemUsage());
+    EXPECT_EQ(emptyManager.getMemOverheadQueue(),
+              manager.getMemOverheadQueue());
+    EXPECT_EQ(0, manager.getMemOverheadIndex());
+}
+
 // Test that when the same client registers twice, the first cursor 'dies'
 TEST_P(CheckpointTest, reRegister) {
     auto dcpCursor1 = manager->registerCursorBySeqno(