MB-58350: Wait for clients to disconnect before deleting bucket

As part of the fix for MB-56067, we converted the blocking bucket
deletion code into a state machine.

The state machine should only make progress if the bucket has no clients
(connections) associated with it.

However, there appears to be a bug in the state machine code, where we
incorrectly move the state machine to complete bucket deletion, before
all clients have disconnected.

This will happen 2 minutes after the DELETE_BUCKET started executing,
due to a missing check for the current number of clients in that code
path.

As a consequence of this, we crash in Connection::propagateDisconnect().

Change-Id: I1aaffb2481107221020974ba7729224e4b764f4b
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/201110
Well-Formed: Restriction Checker
Reviewed-by: Dave Rigby <[email protected]>
Tested-by: Vesko Karaganev <[email protected]>

Author: veselink1

daemon/CMakeLists.txt

@@ -267,6 +267,7 @@ INSTALL(TARGETS memcached
 #
 cb_add_test_executable(memcached_unit_tests
                        bucket_validator_test.cc
+                       buckets_test.cc
                        client_cert_config_test.cc
                        connection_unit_tests.cc
                        datatype_filter_test.cc

daemon/bucket_destroyer.cc

@@ -115,8 +115,10 @@ cb::engine_errc BucketDestroyer::waitForConnections() {
                 bucket.clients,
                 currConns.dump());
 
-        guard.lock();
+        // we need to wait more
+        return cb::engine_errc::would_block;
     }
+
     state = State::WaitingForItemsInTransit;
     return cb::engine_errc::success;
 }

daemon/bucket_destroyer.h

@@ -52,6 +52,10 @@ class BucketDestroyer {
      */
     cb::engine_errc drive();
 
+protected:
+    // Added for testing of the connection timer.
+    friend class BucketDestroyerIntrospector;
+
 private:
     /**
      * Start shutting down the engine.

daemon/buckets_test.cc

@@ -0,0 +1,66 @@
+/*
+ *     Copyright 2022-Present Couchbase, Inc.
+ *
+ *   Use of this software is governed by the Business Source License included
+ *   in the file licenses/BSL-Couchbase.txt.  As of the Change Date specified
+ *   in that file, in accordance with the Business Source License, use of this
+ *   software will be governed by the Apache License, Version 2.0, included in
+ *   the file licenses/APL2.txt.
+ */
+
+#include "bucket_destroyer.h"
+#include "buckets.h"
+#include "memcached.h"
+#include "memcached/engine.h"
+#include "engines/nobucket/nobucket_public.h"
+#include "stats.h"
+#include <folly/portability/GTest.h>
+
+class BucketManagerTest : public ::testing::Test, public BucketManager {
+};
+
+class BucketDestroyerIntrospector {
+public:
+    static void setNextLogConnections(
+            BucketDestroyer& destroyer,
+            std::chrono::steady_clock::time_point tp) {
+        destroyer.nextLogConnections = tp;
+    }
+};
+
+// Destroy bucket with no clients.
+TEST_F(BucketManagerTest, DestroyBucketTest) {
+    auto& bucket = all_buckets.back();
+    bucket.setEngine(create_no_bucket_instance());
+    BucketDestroyer destroyer(bucket, "", false);
+    EXPECT_EQ(cb::engine_errc::success, destroyer.drive());
+}
+
+// We shouldn't destroy a bucket with clients.
+TEST_F(BucketManagerTest, DestroyBucketWaitsForClients) {
+    auto& bucket = all_buckets.back();
+    bucket.setEngine(create_no_bucket_instance());
+    // Add a client and verify that we wait for it.
+    bucket.clients++;
+
+    using std::chrono::steady_clock;
+    using namespace std::chrono_literals;
+    BucketDestroyer destroyer(bucket, "", false);
+    // Cannot complete the bucket delete as we've got a client. Run twice to
+    // make sure the result is the same.
+    EXPECT_EQ(cb::engine_errc::would_block, destroyer.drive());
+    EXPECT_EQ(cb::engine_errc::would_block, destroyer.drive());
+
+    // "Move" the connection log time, such that on the next run it has elapsed.
+    BucketDestroyerIntrospector::setNextLogConnections(
+            destroyer, steady_clock::now() - 1s);
+
+    // We should still be unable to delete the bucket. Run twice to make sure
+    // the result is the same.
+    EXPECT_EQ(cb::engine_errc::would_block, destroyer.drive());
+    EXPECT_EQ(cb::engine_errc::would_block, destroyer.drive());
+
+    // We should succeed in deleting now that clients == 0.
+    bucket.clients--;
+    EXPECT_EQ(cb::engine_errc::success, destroyer.drive());
+}