SERVER-103934 FCBIS should shutdown storage engine without leaking memory (#37853)

wh5a · MongoDB Bot · commit b31482d9d617 · 2025-07-07T20:28:22.000Z
GitOrigin-RevId: f673992c06e5bb331e0c4ccc4c700b6ed3101ae8
diff --git a/src/mongo/db/mongod_main.cpp b/src/mongo/db/mongod_main.cpp
@@ -2073,7 +2073,8 @@ void shutdownTask(const ShutdownTaskArgs& shutdownArgs) {
                                                   "Shut down the storage engine",
                                                   &shutdownTimeElapsedBuilder);
         LOGV2(4784930, "Shutting down the storage engine");
-        shutdownGlobalStorageEngineCleanly(serviceContext);
+        // Allow memory leak for faster shutdown.
+        shutdownGlobalStorageEngineCleanly(serviceContext, true /* memLeakAllowed */);
     }
 
     {
diff --git a/src/mongo/db/op_msg_fuzzer_fixture.cpp b/src/mongo/db/op_msg_fuzzer_fixture.cpp
@@ -160,7 +160,7 @@ OpMsgFuzzerFixture::~OpMsgFuzzerFixture() {
         databaseHolder->closeAll(opCtx.get());
     }
 
-    shutdownGlobalStorageEngineCleanly(_serviceContext);
+    shutdownGlobalStorageEngineCleanly(_serviceContext, true /* memLeakAllowed */);
 }
 
 int OpMsgFuzzerFixture::testOneInput(const char* Data, size_t Size) {
diff --git a/src/mongo/db/repl/oplog_application_bm.cpp b/src/mongo/db/repl/oplog_application_bm.cpp
@@ -110,7 +110,6 @@
 #include "mongo/util/clock_source_mock.h"
 #include "mongo/util/concurrency/thread_pool.h"
 #include "mongo/util/duration.h"
-#include "mongo/util/fail_point.h"
 #include "mongo/util/periodic_runner.h"
 #include "mongo/util/periodic_runner_factory.h"
 #include "mongo/util/time_support.h"
@@ -179,9 +178,6 @@ class TestServiceContext {
         repl::ReplicationCoordinator::set(
             _svcCtx, std::unique_ptr<repl::ReplicationCoordinator>(_replCoord));
 
-        // Disable fast shutdown so that WT can free memory.
-        globalFailPointRegistry().find("WTDisableFastShutDown")->setMode(FailPoint::alwaysOn);
-
         {
             auto initializeStorageEngineOpCtx = _svcCtx->makeOperationContext(&cc());
             shard_role_details::setRecoveryUnit(
@@ -259,8 +255,8 @@ class TestServiceContext {
         auto databaseHolder = DatabaseHolder::get(opCtx);
         databaseHolder->closeAll(opCtx);
 
-        // Shut down storage engine.
-        shutdownGlobalStorageEngineCleanly(_svcCtx);
+        // Shut down storage engine and free memory.
+        shutdownGlobalStorageEngineCleanly(_svcCtx, false /* memLeakAllowed */);
     }
 
     // Shut down the storage engine, clear the dbpath, and restart the storage engine with empty
diff --git a/src/mongo/db/repl/oplog_write_bm.cpp b/src/mongo/db/repl/oplog_write_bm.cpp
@@ -112,7 +112,6 @@
 #include "mongo/util/clock_source_mock.h"
 #include "mongo/util/concurrency/thread_pool.h"
 #include "mongo/util/duration.h"
-#include "mongo/util/fail_point.h"
 #include "mongo/util/periodic_runner.h"
 #include "mongo/util/periodic_runner_factory.h"
 #include "mongo/util/time_support.h"
@@ -180,9 +179,6 @@ class TestServiceContext {
         repl::ReplicationCoordinator::set(
             _svcCtx, std::unique_ptr<repl::ReplicationCoordinator>(_replCoord));
 
-        // Disable fast shutdown so that WT can free memory.
-        globalFailPointRegistry().find("WTDisableFastShutDown")->setMode(FailPoint::alwaysOn);
-
         auto startupOpCtx = _svcCtx->makeOperationContext(&cc());
         initializeStorageEngine(startupOpCtx.get(),
                                 StorageEngineInitFlags::kAllowNoLockFile |
@@ -250,8 +246,8 @@ class TestServiceContext {
         auto databaseHolder = DatabaseHolder::get(opCtx);
         databaseHolder->closeAll(opCtx);
 
-        // Shut down storage engine.
-        shutdownGlobalStorageEngineCleanly(_svcCtx);
+        // Shut down storage engine and free memory.
+        shutdownGlobalStorageEngineCleanly(_svcCtx, false /* memLeakAllowed */);
     }
 
     // Shut down the storage engine, clear the dbpath, and restart the storage engine with empty
diff --git a/src/mongo/db/service_context_d_test_fixture.cpp b/src/mongo/db/service_context_d_test_fixture.cpp
@@ -204,7 +204,7 @@ ServiceContextMongoDTest::~ServiceContextMongoDTest() {
         databaseHolder->closeAll(opCtx);
     }
 
-    shutdownGlobalStorageEngineCleanly(getServiceContext());
+    shutdownGlobalStorageEngineCleanly(getServiceContext(), true /* memLeakAllowed */);
 
     std::swap(storageGlobalParams.engine, _stashedStorageParams.engine);
     std::swap(storageGlobalParams.engineSetByUser, _stashedStorageParams.engineSetByUser);
diff --git a/src/mongo/db/storage/SConscript b/src/mongo/db/storage/SConscript
@@ -1,5 +1,8 @@
 # -*- mode: python -*-
-Import("env")
+Import([
+    "env",
+    "use_system_version_of_library",
+])
 
 env = env.Clone()
 
@@ -654,6 +657,9 @@ env.Library(
     ],
 )
 
+if not use_system_version_of_library('valgrind'):
+    env.InjectThirdParty('valgrind')
+
 env.Library(
     target='storage_engine_impl',
     source=[
diff --git a/src/mongo/db/storage/devnull/devnull_kv_engine.h b/src/mongo/db/storage/devnull/devnull_kv_engine.h
@@ -165,7 +165,7 @@ class DevNullKVEngine : public KVEngine {
         return std::vector<std::string>();
     }
 
-    void cleanShutdown() override {}
+    void cleanShutdown(bool memLeakAllowed) override {}
 
     void setJournalListener(JournalListener* jl) override {}
 
diff --git a/src/mongo/db/storage/kv/kv_drop_pending_ident_reaper_test.cpp b/src/mongo/db/storage/kv/kv_drop_pending_ident_reaper_test.cpp
@@ -160,7 +160,7 @@ class KVEngineMock : public KVEngine {
     std::vector<std::string> getAllIdents(OperationContext* opCtx) const override {
         return {};
     }
-    void cleanShutdown() override {}
+    void cleanShutdown(bool memLeakAllowed) override {}
     void setJournalListener(JournalListener* jl) override {}
     Timestamp getAllDurableTimestamp() const override {
         return {};
diff --git a/src/mongo/db/storage/kv/kv_engine.h b/src/mongo/db/storage/kv/kv_engine.h
@@ -321,9 +321,12 @@ class KVEngine {
      * override this method if they have clean-up to do that is different from unclean shutdown.
      * MongoDB will not call into the storage subsystem after calling this function.
      *
+     * The storage engine is allowed to leak memory for faster shutdown, except when the process is
+     * not exiting or when running tools to look for memory leaks.
+     *
      * There is intentionally no uncleanShutdown().
      */
-    virtual void cleanShutdown() = 0;
+    virtual void cleanShutdown(bool memLeakAllowed) = 0;
 
     /**
      * Return the SnapshotManager for this KVEngine or NULL if not supported.
diff --git a/src/mongo/db/storage/kv/storage_engine_test.cpp b/src/mongo/db/storage/kv/storage_engine_test.cpp
@@ -656,7 +656,12 @@ class TimestampKVEngineTest : public ServiceContextTest {
     }
 
     void tearDown() override {
-        _storageEngine->cleanShutdown(getServiceContext());
+#if __has_feature(address_sanitizer)
+        constexpr bool memLeakAllowed = false;
+#else
+        constexpr bool memLeakAllowed = true;
+#endif
+        _storageEngine->cleanShutdown(getServiceContext(), memLeakAllowed);
         _storageEngine.reset();
 
         ServiceContextTest::tearDown();
diff --git a/src/mongo/db/storage/storage_engine.h b/src/mongo/db/storage/storage_engine.h
@@ -403,7 +403,7 @@ class StorageEngine {
      * On error, the storage engine should assert and crash.
      * There is intentionally no uncleanShutdown().
      */
-    virtual void cleanShutdown(ServiceContext* svcCtx) = 0;
+    virtual void cleanShutdown(ServiceContext* svcCtx, bool memLeakAllowed) = 0;
 
     /**
      * Returns the SnapshotManager for this StorageEngine or NULL if not supported.
diff --git a/src/mongo/db/storage/storage_engine_impl.cpp b/src/mongo/db/storage/storage_engine_impl.cpp
@@ -30,6 +30,10 @@
 
 #include "mongo/db/storage/storage_engine_impl.h"
 
+#ifdef _WIN32
+#define NVALGRIND
+#endif
+
 #include <absl/container/node_hash_map.h>
 #include <absl/meta/type_traits.h>
 #include <algorithm>
@@ -40,6 +44,7 @@
 
 #include <boost/none.hpp>
 #include <boost/optional/optional.hpp>
+#include <valgrind/valgrind.h>
 
 #include "mongo/base/error_codes.h"
 #include "mongo/bson/bsonelement.h"
@@ -905,7 +910,7 @@ std::string StorageEngineImpl::getFilesystemPathForDb(const DatabaseName& dbName
     return _catalog->getFilesystemPathForDb(dbName);
 }
 
-void StorageEngineImpl::cleanShutdown(ServiceContext* svcCtx) {
+void StorageEngineImpl::cleanShutdown(ServiceContext* svcCtx, bool memLeakAllowed) {
     _timestampMonitor.reset();
 
     CollectionCatalog::write(svcCtx, [svcCtx](CollectionCatalog& catalog) {
@@ -916,7 +921,13 @@ void StorageEngineImpl::cleanShutdown(ServiceContext* svcCtx) {
     _catalog.reset();
     _catalogRecordStore.reset();
 
-    _engine->cleanShutdown();
+#if __has_feature(address_sanitizer)
+    memLeakAllowed = false;
+#endif
+    if (RUNNING_ON_VALGRIND) {  // NOLINT
+        memLeakAllowed = false;
+    }
+    _engine->cleanShutdown(memLeakAllowed);
     // intentionally not deleting _engine
 }
 
diff --git a/src/mongo/db/storage/storage_engine_impl.h b/src/mongo/db/storage/storage_engine_impl.h
@@ -140,7 +140,7 @@ class StorageEngineImpl final : public StorageEngineInterface, public StorageEng
     std::unique_ptr<TemporaryRecordStore> makeTemporaryRecordStoreFromExistingIdent(
         OperationContext* opCtx, StringData ident, KeyFormat keyFormat) override;
 
-    void cleanShutdown(ServiceContext* svcCtx) override;
+    void cleanShutdown(ServiceContext* svcCtx, bool memLeakAllowed) override;
 
     void setStableTimestamp(Timestamp stableTimestamp, bool force = false) override;
 
diff --git a/src/mongo/db/storage/storage_engine_init.cpp b/src/mongo/db/storage/storage_engine_init.cpp
@@ -229,15 +229,16 @@ StorageEngine::LastShutdownState initializeStorageEngine(
 namespace {
 void shutdownGlobalStorageEngineCleanly(ServiceContext* service,
                                         Status errorToReport,
-                                        bool forRestart) {
+                                        bool forRestart,
+                                        bool memLeakAllowed) {
     auto storageEngine = service->getStorageEngine();
     invariant(storageEngine);
     // We always use 'forRestart' = false here because 'forRestart' = true is only appropriate if
     // we're going to restart controls on the same storage engine, which we are not here because we
     // are shutting the storage engine down. Additionally, we need to terminate any background
     // threads as they may be holding onto an OperationContext, as opposed to pausing them.
     StorageControl::stopStorageControls(service, errorToReport, /*forRestart=*/false);
-    storageEngine->cleanShutdown(service);
+    storageEngine->cleanShutdown(service, memLeakAllowed);
     auto& lockFile = StorageEngineLockFile::get(service);
     if (lockFile) {
         lockFile->clearPidAndUnlock();
@@ -246,11 +247,12 @@ void shutdownGlobalStorageEngineCleanly(ServiceContext* service,
 }
 } /* namespace */
 
-void shutdownGlobalStorageEngineCleanly(ServiceContext* service) {
+void shutdownGlobalStorageEngineCleanly(ServiceContext* service, bool memLeakAllowed) {
     shutdownGlobalStorageEngineCleanly(
         service,
         {ErrorCodes::ShutdownInProgress, "The storage catalog is being closed."},
-        /*forRestart=*/false);
+        /*forRestart=*/false,
+        memLeakAllowed);
 }
 
 StorageEngine::LastShutdownState reinitializeStorageEngine(
@@ -259,10 +261,12 @@ StorageEngine::LastShutdownState reinitializeStorageEngine(
     std::function<void()> changeConfigurationCallback) {
     auto service = opCtx->getServiceContext();
     shard_role_details::getRecoveryUnit(opCtx)->abandonSnapshot();
+    // Tell storage engine to free memory since the process is not exiting.
     shutdownGlobalStorageEngineCleanly(
         service,
         {ErrorCodes::InterruptedDueToStorageChange, "The storage engine is being reinitialized."},
-        /*forRestart=*/true);
+        /*forRestart=*/true,
+        /*memLeakAllowed=*/false);
     shard_role_details::setRecoveryUnit(opCtx,
                                         std::make_unique<RecoveryUnitNoop>(),
                                         WriteUnitOfWork::RecoveryUnitState::kNotInUnitOfWork);
diff --git a/src/mongo/db/storage/storage_engine_init.h b/src/mongo/db/storage/storage_engine_init.h
@@ -74,8 +74,9 @@ StorageEngine::LastShutdownState initializeStorageEngine(
 
 /**
  * Shuts down storage engine cleanly and releases any locks on mongod.lock.
+ * Set `memLeakAllowed` to true for faster shutdown.
  */
-void shutdownGlobalStorageEngineCleanly(ServiceContext* service);
+void shutdownGlobalStorageEngineCleanly(ServiceContext* service, bool memLeakAllowed);
 
 /**
  * Changes the storage engine for the given service by shutting down the old one and starting
diff --git a/src/mongo/db/storage/storage_engine_mock.h b/src/mongo/db/storage/storage_engine_mock.h
@@ -103,7 +103,7 @@ class StorageEngineMock : public StorageEngine {
         OperationContext* opCtx, StringData ident, KeyFormat keyFormat) final {
         return {};
     }
-    void cleanShutdown(ServiceContext* svcCtx) final {}
+    void cleanShutdown(ServiceContext* svcCtx, bool memLeakAllowed) final {}
     SnapshotManager* getSnapshotManager() const final {
         return nullptr;
     }
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.cpp
@@ -60,10 +60,6 @@
 #include "mongo/util/uuid.h"
 #include "mongo/util/version/releases.h"
 
-#ifdef _WIN32
-#define NVALGRIND
-#endif
-
 #include <absl/container/node_hash_map.h>
 #include <boost/algorithm/string/join.hpp>
 #include <boost/filesystem/fstream.hpp>
@@ -73,7 +69,6 @@
 #include <boost/none_t.hpp>
 #include <boost/optional.hpp>
 #include <fmt/format.h>
-#include <valgrind/valgrind.h>
 // IWYU pragma: no_include "boost/system/detail/error_code.hpp"
 #include <boost/move/utility_core.hpp>
 #include <boost/optional/optional.hpp>
@@ -151,7 +146,6 @@ MONGO_FAIL_POINT_DEFINE(WTWriteConflictExceptionForImportCollection);
 MONGO_FAIL_POINT_DEFINE(WTWriteConflictExceptionForImportIndex);
 MONGO_FAIL_POINT_DEFINE(WTRollbackToStableReturnOnEBUSY);
 MONGO_FAIL_POINT_DEFINE(hangBeforeUnrecoverableRollbackError);
-MONGO_FAIL_POINT_DEFINE(WTDisableFastShutDown);
 MONGO_FAIL_POINT_DEFINE(WTFailImportSortedDataInterface);
 
 const std::string kPinOldestTimestampAtStartupName = "_wt_startup";
@@ -634,9 +628,8 @@ WiredTigerKVEngine::~WiredTigerKVEngine() {
     // storage engine again in this same process.
     ServerParameterSet::getNodeParameterSet()->remove("wiredTigerEngineRuntimeConfig");
 
-    cleanShutdown();
-
-    _sessionCache.reset(nullptr);
+    bool memLeakAllowed = true;
+    cleanShutdown(memLeakAllowed);
 }
 
 void WiredTigerKVEngine::notifyStorageStartupRecoveryComplete() {
@@ -777,7 +770,7 @@ void WiredTigerKVEngine::_openWiredTiger(const std::string& path, const std::str
                         "details"_attr = wtRCToStatus(ret, nullptr).reason());
 }
 
-void WiredTigerKVEngine::cleanShutdown() {
+void WiredTigerKVEngine::cleanShutdown(bool memLeakAllowed) {
     LOGV2(22317, "WiredTigerKVEngine shutting down");
 
     if (!_conn) {
@@ -808,20 +801,9 @@ void WiredTigerKVEngine::cleanShutdown() {
     // released sessions will skip flushing the size storer.
     _sizeStorer.reset();
 
-    // We want WiredTiger to leak memory for faster shutdown except when we are running tools to
-    // look for memory leaks.
-    bool leak_memory = !kAddressSanitizerEnabled;
     std::string closeConfig = "";
 
-    if (RUNNING_ON_VALGRIND) {  // NOLINT
-        leak_memory = false;
-    }
-
-    if (MONGO_unlikely(WTDisableFastShutDown.shouldFail())) {
-        leak_memory = false;
-    }
-
-    if (leak_memory) {
+    if (memLeakAllowed) {
         closeConfig = "leak_memory=true,";
     }
 
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.h b/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.h
@@ -362,7 +362,7 @@ class WiredTigerKVEngine final : public KVEngine {
 
     std::vector<std::string> getAllIdents(OperationContext* opCtx) const override;
 
-    void cleanShutdown() override;
+    void cleanShutdown(bool memLeakAllowed) override;
 
     SnapshotManager* getSnapshotManager() const final {
         return &_sessionCache->snapshotManager();
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine_no_fixture_test.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine_no_fixture_test.cpp
@@ -186,8 +186,8 @@ TEST(WiredTigerKVEngineNoFixtureTest, Basic) {
     ASSERT(conn) << fmt::format("failed to open connection to source folder {}", home.path());
 
     // Open a session.
-    WiredTigerSession session(conn);
-    ASSERT(session.getSession()) << "failed to create session with config isolation=snapshot";
+    auto session = std::make_unique<WiredTigerSession>(conn);
+    ASSERT(session->getSession()) << "failed to create session with config isolation=snapshot";
 
     // Create a table without logging.
     auto oldForceDisableTableLogging = storageGlobalParams.forceDisableTableLogging;
@@ -364,6 +364,14 @@ TEST(WiredTigerKVEngineNoFixtureTest, Basic) {
             << fmt::format("failed to set oldest timestamp to {} (hex: {:x})", tsInc, tsInc);
     }
 
+    session.reset();
+#if __has_feature(address_sanitizer)
+    constexpr bool memLeakAllowed = false;
+#else
+    constexpr bool memLeakAllowed = true;
+#endif
+    kvEngine->cleanShutdown(memLeakAllowed);
+
     // TODO(SERVER-72907): implement test-only function for evict cursor in KVEngine interface
     // porting the rest of the test.
 }
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine_test.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine_test.cpp
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_prepare_conflict_test.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_prepare_conflict_test.cpp
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store_test_harness.h b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store_test_harness.h
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_recovery_unit_test.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_recovery_unit_test.cpp
diff --git a/src/mongo/dbtests/framework.cpp b/src/mongo/dbtests/framework.cpp

Original file line number	Diff line number	Diff line change
`@@ -2073,7 +2073,8 @@ void shutdownTask(const ShutdownTaskArgs& shutdownArgs) {`
`2073`	`2073`	`"Shut down the storage engine",`
`2074`	`2074`	`&shutdownTimeElapsedBuilder);`
`2075`	`2075`	`LOGV2(4784930, "Shutting down the storage engine");`
`2076`		`- shutdownGlobalStorageEngineCleanly(serviceContext);`
	`2076`	`+ // Allow memory leak for faster shutdown.`
	`2077`	`+ shutdownGlobalStorageEngineCleanly(serviceContext, true /* memLeakAllowed */);`
`2077`	`2078`	`}`
`2078`	`2079`
`2079`	`2080`	`{`
Original file line number	Diff line number	Diff line change
`@@ -160,7 +160,7 @@ OpMsgFuzzerFixture::~OpMsgFuzzerFixture() {`
`160`	`160`	`databaseHolder->closeAll(opCtx.get());`
`161`	`161`	`}`
`162`	`162`
`163`		`- shutdownGlobalStorageEngineCleanly(_serviceContext);`
	`163`	`+ shutdownGlobalStorageEngineCleanly(_serviceContext, true /* memLeakAllowed */);`
`164`	`164`	`}`
`165`	`165`
`166`	`166`	`int OpMsgFuzzerFixture::testOneInput(const char* Data, size_t Size) {`
Original file line number	Diff line number	Diff line change
`@@ -204,7 +204,7 @@ ServiceContextMongoDTest::~ServiceContextMongoDTest() {`
`204`	`204`	`databaseHolder->closeAll(opCtx);`
`205`	`205`	`}`
`206`	`206`
`207`		`- shutdownGlobalStorageEngineCleanly(getServiceContext());`
	`207`	`+ shutdownGlobalStorageEngineCleanly(getServiceContext(), true /* memLeakAllowed */);`
`208`	`208`
`209`	`209`	`std::swap(storageGlobalParams.engine, _stashedStorageParams.engine);`
`210`	`210`	`std::swap(storageGlobalParams.engineSetByUser, _stashedStorageParams.engineSetByUser);`
Original file line number	Diff line number	Diff line change
`@@ -165,7 +165,7 @@ class DevNullKVEngine : public KVEngine {`
`165`	`165`	`return std::vector<std::string>();`
`166`	`166`	`}`
`167`	`167`
`168`		`- void cleanShutdown() override {}`
	`168`	`+ void cleanShutdown(bool memLeakAllowed) override {}`
`169`	`169`
`170`	`170`	`void setJournalListener(JournalListener* jl) override {}`
`171`	`171`
Original file line number	Diff line number	Diff line change
`@@ -160,7 +160,7 @@ class KVEngineMock : public KVEngine {`
`160`	`160`	`std::vector<std::string> getAllIdents(OperationContext* opCtx) const override {`
`161`	`161`	`return {};`
`162`	`162`	`}`
`163`		`- void cleanShutdown() override {}`
	`163`	`+ void cleanShutdown(bool memLeakAllowed) override {}`
`164`	`164`	`void setJournalListener(JournalListener* jl) override {}`
`165`	`165`	`Timestamp getAllDurableTimestamp() const override {`
`166`	`166`	`return {};`