SERVER-48524 indexbuildentryhelpers::removeIndexBuildEntry() requires callers to obtain collection lock on config.system.indexBuilds

This improves index build semantics for aborts in the context of stepdowns and interrupts.

3 files changed, 22 insertions, 18 deletions

diff --git a/src/mongo/db/index_build_entry_helpers.cpp b/src/mongo/db/index_build_entry_helpers.cpp
index 0ed0831e060..08534de3906 100644
--- a/src/mongo/db/index_build_entry_helpers.cpp
+++ b/src/mongo/db/index_build_entry_helpers.cpp
@@ -246,23 +246,20 @@ Status addIndexBuildEntry(OperationContext* opCtx, const IndexBuildEntry& indexB
 }
 
 Status removeIndexBuildEntry(OperationContext* opCtx,
-                             const CollectionPtr& collectionUnused,
+                             const CollectionPtr& collection,
                              UUID indexBuildUUID) {
     return writeConflictRetry(
         opCtx,
         "removeIndexBuildEntry",
         NamespaceString::kIndexBuildEntryNamespace.ns(),
         [&]() -> Status {
-            AutoGetCollection collection(
-                opCtx, NamespaceString::kIndexBuildEntryNamespace, MODE_IX);
             if (!collection) {
                 str::stream ss;
                 ss << "Collection not found: " << NamespaceString::kIndexBuildEntryNamespace.ns();
                 return Status(ErrorCodes::NamespaceNotFound, ss);
             }
 
-            RecordId rid =
-                Helpers::findById(opCtx, collection.getCollection(), BSON("_id" << indexBuildUUID));
+            RecordId rid = Helpers::findById(opCtx, collection, BSON("_id" << indexBuildUUID));
             if (rid.isNull()) {
                 str::stream ss;
                 ss << "No matching IndexBuildEntry found with indexBuildUUID: " << indexBuildUUID;
diff --git a/src/mongo/db/index_build_entry_helpers_test.cpp b/src/mongo/db/index_build_entry_helpers_test.cpp
index e44e7b780dd..f8563c66feb 100644
--- a/src/mongo/db/index_build_entry_helpers_test.cpp
+++ b/src/mongo/db/index_build_entry_helpers_test.cpp
@@ -38,6 +38,7 @@
 #include "mongo/db/catalog/catalog_test_fixture.h"
 #include "mongo/db/catalog/commit_quorum_options.h"
 #include "mongo/db/catalog/index_build_entry_gen.h"
+#include "mongo/db/catalog_raii.h"
 #include "mongo/db/client.h"
 #include "mongo/db/index_build_entry_helpers.h"
 #include "mongo/db/service_context.h"
@@ -90,7 +91,8 @@ void checkIfEqual(IndexBuildEntry lhs, IndexBuildEntry rhs) {
 }
 
 Status removeIndexBuildEntry(OperationContext* opCtx, UUID indexBuildUUID) {
-    return indexbuildentryhelpers::removeIndexBuildEntry(opCtx, {}, indexBuildUUID);
+    AutoGetCollection autoColl(opCtx, NamespaceString::kIndexBuildEntryNamespace, MODE_IX);
+    return indexbuildentryhelpers::removeIndexBuildEntry(opCtx, *autoColl, indexBuildUUID);
 }
 
 class IndexBuildEntryHelpersTest : public CatalogTestFixture {
diff --git a/src/mongo/db/index_builds_coordinator.cpp b/src/mongo/db/index_builds_coordinator.cpp
index 89eaab16adf..467b4bc3d8e 100644
--- a/src/mongo/db/index_builds_coordinator.cpp
+++ b/src/mongo/db/index_builds_coordinator.cpp
@@ -1087,6 +1087,8 @@ bool IndexBuildsCoordinator::abortIndexBuildByBuildUUID(OperationContext* opCtx,
             unlockRSTL(opCtx);
         }
         Lock::CollectionLock collLock(opCtx, dbAndUUID, MODE_X);
+        AutoGetCollection indexBuildEntryColl(
+            opCtx, NamespaceString::kIndexBuildEntryNamespace, MODE_IX);
 
         // If we are using two-phase index builds and are no longer primary after receiving an
         // abort, we cannot replicate an abortIndexBuild oplog entry. Continue holding the RSTL to
@@ -1145,14 +1147,11 @@ bool IndexBuildsCoordinator::abortIndexBuildByBuildUUID(OperationContext* opCtx,
 
         // At this point we must continue aborting the index build.
         try {
-            // We are holding the RSTL and an exclusive collection lock, so we will block stepdown
-            // and be targeted for being killed. In addition to writing to the catalog, we need to
-            // acquire an IX lock to write to the config.system.indexBuilds collection. Since
-            // we must perform these final writes, but we expect them not to block, we can safely,
-            // temporarily disable interrupts.
-            UninterruptibleLockGuard noInterrupt(opCtx->lockState());
-            _completeAbort(
-                opCtx, replState, {}, signalAction, {ErrorCodes::IndexBuildAborted, reason});
+            _completeAbort(opCtx,
+                           replState,
+                           *indexBuildEntryColl,
+                           signalAction,
+                           {ErrorCodes::IndexBuildAborted, reason});
         } catch (const DBException& e) {
             LOGV2_FATAL(
                 4656011,
@@ -2040,7 +2039,9 @@ void IndexBuildsCoordinator::_cleanUpSinglePhaseAfterFailure(
 
             const NamespaceStringOrUUID dbAndUUID(replState->dbName, replState->collectionUUID);
             Lock::CollectionLock collLock(abortCtx, dbAndUUID, MODE_X);
-            _completeSelfAbort(abortCtx, replState, {}, status);
+            AutoGetCollection indexBuildEntryColl(
+                abortCtx, NamespaceString::kIndexBuildEntryNamespace, MODE_IX);
+            _completeSelfAbort(abortCtx, replState, *indexBuildEntryColl, status);
         });
 }
 
@@ -2079,7 +2080,9 @@ void IndexBuildsCoordinator::_cleanUpTwoPhaseAfterFailure(
             }
 
             Lock::CollectionLock collLock(abortCtx, dbAndUUID, MODE_X);
-            _completeSelfAbort(abortCtx, replState, {}, status);
+            AutoGetCollection indexBuildEntryColl(
+                abortCtx, NamespaceString::kIndexBuildEntryNamespace, MODE_IX);
+            _completeSelfAbort(abortCtx, replState, *indexBuildEntryColl, status);
         });
 }
 
@@ -2464,6 +2467,8 @@ IndexBuildsCoordinator::CommitResult IndexBuildsCoordinator::_insertKeysFromSide
     // Need to return the collection lock back to exclusive mode to complete the index build.
     const NamespaceStringOrUUID dbAndUUID(replState->dbName, replState->collectionUUID);
     Lock::CollectionLock collLock(opCtx, dbAndUUID, MODE_X);
+    AutoGetCollection indexBuildEntryColl(
+        opCtx, NamespaceString::kIndexBuildEntryNamespace, MODE_IX);
 
     // If we can't acquire the RSTL within a given time period, there is an active state transition
     // and we should release our locks and try again. We would otherwise introduce a deadlock with
@@ -2616,11 +2621,11 @@ IndexBuildsCoordinator::CommitResult IndexBuildsCoordinator::_insertKeysFromSide
 
         // This index build failed due to an indexing error in normal circumstances. Abort while
         // still holding the RSTL and collection locks.
-        _completeSelfAbort(opCtx, replState, {}, status);
+        _completeSelfAbort(opCtx, replState, *indexBuildEntryColl, status);
         throw;
     }
 
-    removeIndexBuildEntryAfterCommitOrAbort(opCtx, dbAndUUID, {}, *replState);
+    removeIndexBuildEntryAfterCommitOrAbort(opCtx, dbAndUUID, *indexBuildEntryColl, *replState);
     replState->stats.numIndexesAfter = getNumIndexesTotal(opCtx, collection.get());
     LOGV2(20663,
           "Index build: completed successfully",