SERVER-58023 PrimaryOnlyService should not invariant that _activeInstances is empty when in kPaused

(cherry picked from commit 6e08ac57662b82c3249d2bd2f6a02841f6bf75c5)

2 files changed, 98 insertions, 2 deletions

diff --git a/src/mongo/db/repl/primary_only_service.cpp b/src/mongo/db/repl/primary_only_service.cpp
index b95354b5f2e..e7e3779e97f 100644
--- a/src/mongo/db/repl/primary_only_service.cpp
+++ b/src/mongo/db/repl/primary_only_service.cpp
@@ -542,7 +542,6 @@ boost::optional<std::shared_ptr<PrimaryOnlyService::Instance>> PrimaryOnlyServic
         _rebuildCV, lk, [this]() { return _state != State::kRebuilding; });
 
     if (_state == State::kShutdown || _state == State::kPaused) {
-        invariant(_activeInstances.empty());
         return boost::none;
     }
     if (_state == State::kRebuildFailed) {
@@ -573,7 +572,6 @@ std::vector<std::shared_ptr<PrimaryOnlyService::Instance>> PrimaryOnlyService::g
         _rebuildCV, lk, [this]() { return _state != State::kRebuilding; });
 
     if (_state == State::kShutdown || _state == State::kPaused) {
-        invariant(_activeInstances.empty());
         return instances;
     }
     if (_state == State::kRebuildFailed) {
diff --git a/src/mongo/db/repl/primary_only_service_test.cpp b/src/mongo/db/repl/primary_only_service_test.cpp
index 4aa1798cc9d..e510dbe5d6b 100644
--- a/src/mongo/db/repl/primary_only_service_test.cpp
+++ b/src/mongo/db/repl/primary_only_service_test.cpp
@@ -27,6 +27,7 @@
  *    it in the license file.
  */
 
+#include <boost/optional/optional_io.hpp>
 #include <memory>
 
 #include "mongo/db/client.h"
@@ -90,6 +91,10 @@ public:
         return std::make_shared<TestService::Instance>(this, std::move(initialState));
     }
 
+    // Make this public since it's protected in the base class but it needs to be public if we want
+    // to test it here.
+    using PrimaryOnlyService::getAllInstances;
+
     class Instance final : public PrimaryOnlyService::TypedInstance<Instance> {
     public:
         explicit Instance(const TestService* service, BSONObj stateDoc)
@@ -605,6 +610,99 @@ DEATH_TEST_F(PrimaryOnlyServiceTest,
     }
 }
 
+TEST_F(PrimaryOnlyServiceTest, LookupInstanceAfterStepDownReturnsNone) {
+    // Make sure the instance doesn't complete before we try to look it up.
+    auto timesEntered = TestServiceHangDuringInitialization.setMode(FailPoint::alwaysOn);
+
+    auto opCtx = makeOperationContext();
+    auto instance =
+        TestService::Instance::getOrCreate(opCtx.get(), _service, BSON("_id" << 0 << "state" << 0));
+    ASSERT(instance.get());
+    ASSERT_EQ(0, instance->getID());
+
+    // Wait for Instance::run() to be called before calling stepDown so that the _completionPromise
+    // will eventually be set.
+    TestServiceHangDuringInitialization.waitForTimesEntered(timesEntered + 1);
+
+    stepDown();
+
+    auto instance2 = TestService::Instance::lookup(opCtx.get(), _service, BSON("_id" << 0));
+
+    ASSERT_EQ(instance2, boost::none);
+
+    TestServiceHangDuringInitialization.setMode(FailPoint::off);
+    ASSERT_EQ(ErrorCodes::Interrupted, instance->getCompletionFuture().getNoThrow());
+}
+
+TEST_F(PrimaryOnlyServiceTest, LookupInstanceAfterShutDownReturnsNone) {
+    // Make sure the instance doesn't complete before we try to look it up.
+    auto timesEntered = TestServiceHangDuringInitialization.setMode(FailPoint::alwaysOn);
+
+    auto opCtx = makeOperationContext();
+    auto instance =
+        TestService::Instance::getOrCreate(opCtx.get(), _service, BSON("_id" << 0 << "state" << 0));
+    ASSERT(instance.get());
+    ASSERT_EQ(0, instance->getID());
+    // Make sure we enter the run function before shutting down.
+    TestServiceHangDuringInitialization.waitForTimesEntered(timesEntered + 1);
+    TestServiceHangDuringInitialization.setMode(FailPoint::off);
+
+    shutdown();
+
+    auto instance2 = TestService::Instance::lookup(opCtx.get(), _service, BSON("_id" << 0));
+
+    ASSERT_EQ(instance2, boost::none);
+
+    ASSERT_EQ(ErrorCodes::Interrupted, instance->getCompletionFuture().getNoThrow());
+}
+
+TEST_F(PrimaryOnlyServiceTest, GetAllInstancesAfterStepDownReturnsEmptyVector) {
+    // Make sure the instance doesn't complete before we try to look it up.
+    auto timesEntered = TestServiceHangDuringInitialization.setMode(FailPoint::alwaysOn);
+
+    auto opCtx = makeOperationContext();
+    auto instance =
+        TestService::Instance::getOrCreate(opCtx.get(), _service, BSON("_id" << 0 << "state" << 0));
+    ASSERT(instance.get());
+    ASSERT_EQ(0, instance->getID());
+
+    // Wait for Instance::run() to be called before calling stepDown so that the _completionPromise
+    // will eventually be set.
+    TestServiceHangDuringInitialization.waitForTimesEntered(timesEntered + 1);
+
+    stepDown();
+
+    auto instances = static_cast<TestService*>(_service)->getAllInstances(opCtx.get());
+
+    ASSERT_EQ(instances.size(), 0);
+
+    TestServiceHangDuringInitialization.setMode(FailPoint::off);
+    ASSERT_EQ(ErrorCodes::Interrupted, instance->getCompletionFuture().getNoThrow());
+}
+
+TEST_F(PrimaryOnlyServiceTest, GetAllInstancesAfterShutDownReturnsEmptyVector) {
+    // Make sure the instance doesn't complete before we try to look it up.
+    auto timesEntered = TestServiceHangDuringInitialization.setMode(FailPoint::alwaysOn);
+
+    auto opCtx = makeOperationContext();
+    auto instance =
+        TestService::Instance::getOrCreate(opCtx.get(), _service, BSON("_id" << 0 << "state" << 0));
+    ASSERT(instance.get());
+    ASSERT_EQ(0, instance->getID());
+
+    // Make sure we enter the run function before shutting down.
+    TestServiceHangDuringInitialization.waitForTimesEntered(timesEntered + 1);
+    TestServiceHangDuringInitialization.setMode(FailPoint::off);
+
+    shutdown();
+
+    auto instances = static_cast<TestService*>(_service)->getAllInstances(opCtx.get());
+
+    ASSERT_EQ(instances.size(), 0);
+
+    ASSERT_EQ(ErrorCodes::Interrupted, instance->getCompletionFuture().getNoThrow());
+}
+
 TEST_F(PrimaryOnlyServiceTest, GetOrCreateInstanceInterruptible) {
     stepDown();
     // Make sure the service stays in state kRebuilding so that getOrCreate() has to try to wait on