SERVER-126508 Multikeys vector accumulates across WCE retries in commit phase in primary-driven index builds (#53887)

Co-authored-by: Gregory Noma <gregory.noma@gmail.com>
GitOrigin-RevId: dcfb580cc8c1caf6e401cd38d606c0f7cb2823b6

src/mongo/db/index_builds/index_builds_coordinator.cpp

@@ -3949,51 +3949,46 @@ IndexBuildsCoordinator::CommitResult IndexBuildsCoordinator::_insertKeysFromSide
         indexBuildsSSS.commit.addAndFetch(1);
         storeLastCommittedDuration(*replState);
 
-        std::vector<boost::optional<MultikeyPaths>> multikeys;
+        // TODO (SERVER-109664): Check build protocol rather than feature flag.
+        auto onCommitFn = [&](const std::vector<boost::optional<MultikeyPaths>>& multikeys) {
+            const bool isPdib = [&] {
+                if (replState->protocol == IndexBuildProtocol::kPrimaryDriven) {
+                    return true;
+                }
+                if (replState->protocol != IndexBuildProtocol::kTwoPhase) {
+                    return false;
+                }
+                auto fcvSnapshot = serverGlobalParams.featureCompatibility.acquireFCVSnapshot();
+                return fcvSnapshot.isVersionInitialized() &&
+                    feature_flags::gFeatureFlagPrimaryDrivenIndexBuilds.isEnabled(
+                        VersionContext::getDecoration(opCtx), fcvSnapshot);
+            }();
 
-        // If two phase index builds is enabled, index build will be coordinated using
-        // startIndexBuild and commitIndexBuild oplog entries.
-        auto onCommitFn = [&] {
+            const bool shouldReplicate = isPdib && IndexBuildAction::kOplogCommit != action;
             onCommitIndexBuild(opCtx,
                                collection->ns(),
                                replState,
-                               multikeys,
+                               shouldReplicate ? multikeys
+                                               : std::vector<boost::optional<MultikeyPaths>>{},
                                collection->isTimeseriesCollection());
         };
 
-        int i = 0;
-        auto onCreateEachFn = [&](const BSONObj& spec,
-                                  IndexCatalogEntry& entry,
-                                  boost::optional<MultikeyPaths>&& multikey) {
-            if (IndexBuildProtocol::kTwoPhase == replState->protocol ||
-                IndexBuildProtocol::kPrimaryDriven == replState->protocol) {
-                if (IndexBuildProtocol::kTwoPhase == replState->protocol) {
-                    // TODO (SERVER-109664): Check build protocol rather than feature flag.
-                    auto fcvSnapshot = serverGlobalParams.featureCompatibility.acquireFCVSnapshot();
-                    if (!fcvSnapshot.isVersionInitialized() ||
-                        !feature_flags::gFeatureFlagPrimaryDrivenIndexBuilds.isEnabled(
-                            VersionContext::getDecoration(opCtx), fcvSnapshot)) {
-                        return;
-                    }
+        auto onCreateEachFn =
+            [&](const BSONObj& spec, IndexCatalogEntry& entry, boost::optional<MultikeyPaths>&&) {
+                if (IndexBuildProtocol::kSinglePhase != replState->protocol) {
+                    return;
                 }
 
-                if (IndexBuildAction::kOplogCommit != action) {
-                    multikeys.push_back(std::move(multikey));
-                }
-                ++i;
-                return;
-            }
-
-            auto opObserver = opCtx->getServiceContext()->getOpObserver();
-            IndexBuildInfo indexBuildInfo(spec, std::string{entry.getIdent()});
-            auto fromMigrate = false;
-            opObserver->onCreateIndex(opCtx,
-                                      collection->ns(),
-                                      replState->collectionUUID,
-                                      indexBuildInfo,
-                                      fromMigrate,
-                                      collection->isTimeseriesCollection());
-        };
+                auto opObserver = opCtx->getServiceContext()->getOpObserver();
+                IndexBuildInfo indexBuildInfo(spec, std::string{entry.getIdent()});
+                auto fromMigrate = false;
+                opObserver->onCreateIndex(opCtx,
+                                          collection->ns(),
+                                          replState->collectionUUID,
+                                          indexBuildInfo,
+                                          fromMigrate,
+                                          collection->isTimeseriesCollection());
+            };
 
         // Commit index build.
         TimestampBlock tsBlock(opCtx, commitIndexBuildTimestamp);

src/mongo/db/index_builds/index_builds_manager_test.cpp

@@ -31,6 +31,7 @@
 
 #include "mongo/base/string_data.h"
 #include "mongo/bson/bsonmisc.h"
+#include "mongo/db/dbhelpers.h"
 #include "mongo/db/index_builds/primary_driven/registry.h"
 #include "mongo/db/index_builds/primary_driven/util.h"
 #include "mongo/db/namespace_string.h"
@@ -38,6 +39,7 @@
 #include "mongo/db/shard_role/lock_manager/lock_manager_defs.h"
 #include "mongo/db/shard_role/shard_catalog/catalog_test_fixture.h"
 #include "mongo/db/shard_role/shard_catalog/collection_options.h"
+#include "mongo/db/storage/exceptions.h"
 #include "mongo/db/storage/ident.h"
 #include "mongo/idl/server_parameter_test_controller.h"
 #include "mongo/unittest/unittest.h"
@@ -56,6 +58,8 @@ private:
 public:
     void createCollection(const NamespaceString& nss);
 
+    UUID getCollectionUUID() const;
+
     std::vector<IndexBuildInfo> makeSpecs(std::vector<std::string> keys);
 
     const UUID _buildUUID = UUID::gen();
@@ -81,6 +85,11 @@ void IndexBuildsManagerTest::createCollection(const NamespaceString& nss) {
     ASSERT_OK(storageInterface()->createCollection(operationContext(), nss, CollectionOptions()));
 }
 
+UUID IndexBuildsManagerTest::getCollectionUUID() const {
+    AutoGetCollection autoColl(operationContext(), _nss, MODE_IS);
+    return autoColl->uuid();
+}
+
 std::vector<IndexBuildInfo> IndexBuildsManagerTest::makeSpecs(std::vector<std::string> keys) {
     ASSERT(keys.size());
     auto storageEngine = operationContext()->getServiceContext()->getStorageEngine();
@@ -184,6 +193,55 @@ TEST_F(IndexBuildsManagerTest, SetUpNonPrimaryDrivenIndexBuildDoesNotAddToRegist
         operationContext(), collection, _buildUUID, MultiIndexBlock::kNoopOnCleanUpFn);
     _indexBuildsManager.tearDownAndUnregisterIndexBuild(_buildUUID);
 }
+
+// `MultiIndexBlock::commit`'s per-index loop collects multikey paths and passes them to `onCommit`;
+// that collection must be scoped to a single attempt so a WCE retry doesn't leave the caller
+// observing entries from a prior failed attempt. We force a retry by throwing a
+// WriteConflictException from `onCommitFn` after the loop has completed all push_backs.
+TEST_F(IndexBuildsManagerTest, CommitIndexBuildMultikeysAreResetWhenWceFiresAfterLoop) {
+    auto opCtx = operationContext();
+    const auto collectionUUID = getCollectionUUID();
+
+    AutoGetCollection lockHolder(opCtx, _nss, MODE_X);
+    CollectionWriter collection(opCtx, collectionUUID);
+
+    {
+        WriteUnitOfWork wuow(opCtx);
+        ASSERT_OK(Helpers::insert(opCtx, *lockHolder, BSON("_id" << 0 << "a" << 1 << "b" << 2)));
+        wuow.commit();
+    }
+
+    auto indexes = makeSpecs({"a", "b"});
+    ASSERT_OK(_indexBuildsManager.setUpIndexBuild(
+        opCtx, collection, indexes, _buildUUID, MultiIndexBlock::kNoopOnInitFn));
+    ASSERT_OK(
+        _indexBuildsManager.startBuildingIndex(opCtx, _nss.dbName(), collectionUUID, _buildUUID));
+    ASSERT_OK(_indexBuildsManager.drainBackgroundWrites(
+        opCtx,
+        _buildUUID,
+        RecoveryUnit::ReadSource::kNoTimestamp,
+        IndexBuildInterceptor::DrainYieldPolicy::kNoYield));
+
+    int commitAttempts = 0;
+    size_t multikeysAtCommit = 0;
+    auto onCommitFn = [&](const std::vector<boost::optional<MultikeyPaths>>& multikeys) {
+        ++commitAttempts;
+        multikeysAtCommit = multikeys.size();
+        if (commitAttempts == 1) {
+            // Force a retry after the loop has populated `multikeys` so we can verify that the
+            // next attempt observes a freshly-built vector rather than accumulated entries.
+            throwWriteConflictException("Force WCE in onCommitFn after commit loop.");
+        }
+    };
+
+    ASSERT_OK(_indexBuildsManager.commitIndexBuild(
+        opCtx, collection, _nss, _buildUUID, MultiIndexBlock::kNoopOnCreateEachFn, onCommitFn));
+
+    ASSERT_EQ(commitAttempts, 2);
+    ASSERT_EQ(multikeysAtCommit, indexes.size());
+
+    _indexBuildsManager.tearDownAndUnregisterIndexBuild(_buildUUID);
+}
 }  // namespace
 
 }  // namespace mongo

src/mongo/db/index_builds/multi_index_block.cpp

@@ -1359,8 +1359,9 @@ Status MultiIndexBlock::checkConstraints(OperationContext* opCtx, const Collecti
 MultiIndexBlock::OnCreateEachFn MultiIndexBlock::kNoopOnCreateEachFn =
     +[](const BSONObj&, IndexCatalogEntry&, boost::optional<MultikeyPaths>&&) {
     };
-MultiIndexBlock::OnCommitFn MultiIndexBlock::kNoopOnCommitFn = +[]() {
-};
+MultiIndexBlock::OnCommitFn MultiIndexBlock::kNoopOnCommitFn =
+    +[](const std::vector<boost::optional<MultikeyPaths>>&) {
+    };
 
 Status MultiIndexBlock::commit(OperationContext* opCtx,
                                Collection* collection,
@@ -1407,6 +1408,9 @@ Status MultiIndexBlock::commit(OperationContext* opCtx,
     }
     MultikeyPathTracker::get(opCtx).stopTrackingMultikeyPathInfo();
 
+    std::vector<boost::optional<MultikeyPaths>> multikeys;
+    multikeys.reserve(_indexes.size());
+
     for (auto& index : _indexes) {
         boost::optional<MultikeyPaths> paths;
 
@@ -1448,10 +1452,11 @@ Status MultiIndexBlock::commit(OperationContext* opCtx,
             }
         }
 
+        multikeys.push_back(paths);
         onCreateEach(index.block->getSpec(), *indexCatalogEntry, std::move(paths));
     }
 
-    onCommit();
+    onCommit(multikeys);
 
     // We can't update the 'timeseriesBucketsMayHaveMixedSchemaData' catalog entry flag here as it
     // requires the change to be driven by the router role. It means that subsequent index builds

src/mongo/db/index_builds/multi_index_block.h

@@ -249,11 +249,14 @@ public:
      * logOp() should be called from the same unit of work as commit().
      *
      * `onCreateEach` will be called after each index has been marked as "ready".
-     * `onCommit` will be called after all indexes have been marked "ready".
+     *
+     * `onCommit` will be called after all indexes have been marked "ready", with the per-index
+     * multikey paths collected during the commit attempt.
      *
      * Requires holding an exclusive lock on the collection.
      */
-    using OnCommitFn = function_ref<void()>;
+    using OnCommitFn =
+        function_ref<void(const std::vector<boost::optional<MultikeyPaths>>& multikeys)>;
     using OnCreateEachFn = function_ref<void(
         const BSONObj& spec, IndexCatalogEntry& entry, boost::optional<MultikeyPaths>&& multikey)>;
     Status commit(OperationContext* opCtx,