SERVER-45409 Rollback should wait for all index builds to complete. Fail rollback-via-refetch on the first attempt if index builds were aborted beforehand

5 files changed, 60 insertions, 34 deletions

diff --git a/src/mongo/db/commands/create_indexes.cpp b/src/mongo/db/commands/create_indexes.cpp
index 0213275eb86..6c69b9ddc3b 100644
--- a/src/mongo/db/commands/create_indexes.cpp
+++ b/src/mongo/db/commands/create_indexes.cpp
@@ -904,9 +904,12 @@ bool runCreateIndexesWithCoordinator(OperationContext* opCtx,
             // independently of this command invocation. We'll defensively abort the index build
             // with the assumption that if the index build was already in the midst of tearing down,
             // this be a no-op.
+            // Use a null abort timestamp because the index build will generate its own timestamp
+            // on cleanup.
             indexBuildsCoord->abortIndexBuildByBuildUUIDNoWait(
                 opCtx,
                 buildUUID,
+                Timestamp(),
                 str::stream() << "Index build interrupted: " << buildUUID << ": "
                               << interruptionEx.toString());
             log() << "Index build aborted: " << buildUUID;
@@ -924,9 +927,12 @@ bool runCreateIndexesWithCoordinator(OperationContext* opCtx,
                 throw;
             }
 
+            // Use a null abort timestamp because the index build will generate a ghost timestamp
+            // for the single-phase build on cleanup.
             indexBuildsCoord->abortIndexBuildByBuildUUIDNoWait(
                 opCtx,
                 buildUUID,
+                Timestamp(),
                 str::stream() << "Index build interrupted due to change in replication state: "
                               << buildUUID << ": " << ex.toString());
             log() << "Index build aborted due to NotMaster error: " << buildUUID;
diff --git a/src/mongo/db/index_builds_coordinator.cpp b/src/mongo/db/index_builds_coordinator.cpp
index d89ee9a5562..77c667a42d9 100644
--- a/src/mongo/db/index_builds_coordinator.cpp
+++ b/src/mongo/db/index_builds_coordinator.cpp
@@ -636,17 +636,25 @@ void IndexBuildsCoordinator::applyAbortIndexBuild(OperationContext* opCtx,
     updateCurOpForCommitOrAbort(opCtx, kCommitIndexBuildFieldName, buildUUID);
 
     invariant(oplogEntry.cause);
+    uassert(31420,
+            str::stream()
+                << "No commit timestamp set while applying abortIndexBuild operation. Build UUID: "
+                << buildUUID,
+            !opCtx->recoveryUnit()->getCommitTimestamp().isNull());
+
     auto indexBuildsCoord = IndexBuildsCoordinator::get(opCtx);
     indexBuildsCoord->abortIndexBuildByBuildUUID(
         opCtx,
         buildUUID,
+        opCtx->recoveryUnit()->getCommitTimestamp(),
         str::stream() << "abortIndexBuild oplog entry encountered: " << *oplogEntry.cause);
 }
 
 void IndexBuildsCoordinator::abortIndexBuildByBuildUUID(OperationContext* opCtx,
                                                         const UUID& buildUUID,
+                                                        Timestamp abortTimestamp,
                                                         const std::string& reason) {
-    if (!abortIndexBuildByBuildUUIDNoWait(opCtx, buildUUID, reason)) {
+    if (!abortIndexBuildByBuildUUIDNoWait(opCtx, buildUUID, abortTimestamp, reason)) {
         return;
     }
 
@@ -658,6 +666,7 @@ void IndexBuildsCoordinator::abortIndexBuildByBuildUUID(OperationContext* opCtx,
 
 bool IndexBuildsCoordinator::abortIndexBuildByBuildUUIDNoWait(OperationContext* opCtx,
                                                               const UUID& buildUUID,
+                                                              Timestamp abortTimestamp,
                                                               const std::string& reason) {
     _indexBuildsManager.abortIndexBuild(buildUUID, reason);
 
@@ -674,7 +683,7 @@ bool IndexBuildsCoordinator::abortIndexBuildByBuildUUIDNoWait(OperationContext*
     {
         stdx::unique_lock<Latch> lk(replState->mutex);
         replState->aborted = true;
-        replState->abortTimestamp = opCtx->recoveryUnit()->getCommitTimestamp();
+        replState->abortTimestamp = abortTimestamp;
         replState->abortReason = reason;
         replState->condVar.notify_all();
     }
@@ -720,10 +729,12 @@ void IndexBuildsCoordinator::onStepUp(OperationContext* opCtx) {
             // oplog entries, and consequently does not have a timestamp to delete the index from
             // the durable catalog. This abort will replicate to the old primary, now secondary, to
             // abort the build.
+            // Use a null timestamp because the primary will generate its own timestamp with an
+            // oplog entry.
             // Do not wait for the index build to exit, because it may reacquire locks that are not
             // available until stepUp completes.
             abortIndexBuildByBuildUUIDNoWait(
-                opCtx, replState->buildUUID, "unique indexes do not support failover");
+                opCtx, replState->buildUUID, Timestamp(), "unique indexes do not support failover");
             return;
         }
 
@@ -747,37 +758,29 @@ IndexBuilds IndexBuildsCoordinator::onRollback(OperationContext* opCtx) {
     IndexBuilds buildsAborted;
 
     auto indexBuilds = _getIndexBuilds();
-    auto onIndexBuild = [this, &buildsAborted](std::shared_ptr<ReplIndexBuildState> replState) {
+    auto onIndexBuild = [this, opCtx, &buildsAborted](
+                            std::shared_ptr<ReplIndexBuildState> replState) {
         if (IndexBuildProtocol::kSinglePhase == replState->protocol) {
             log() << "IndexBuildsCoordinator::onRollback - not aborting single phase index build: "
                   << replState->buildUUID;
             return;
         }
-
         const std::string reason = "rollback";
-        _indexBuildsManager.abortIndexBuild(replState->buildUUID, reason);
-
-        stdx::unique_lock<Latch> lk(replState->mutex);
-        if (!replState->aborted) {
-
-            IndexBuildDetails aborted{replState->collectionUUID};
-            // Record the index builds aborted due to rollback. This allows any rollback algorithm
-            // to efficiently restart all unfinished index builds without having to scan all indexes
-            // in all collections.
-            for (auto spec : replState->indexSpecs) {
-                aborted.indexSpecs.emplace_back(spec.getOwned());
-            }
-            buildsAborted.insert({replState->buildUUID, aborted});
 
-            // Leave abort timestamp as null. This will unblock the index build and allow it to
-            // complete using a ghost timestamp. Subsequently, the rollback algorithm can decide how
-            // to undo the index build depending on the state of the oplog.
-            invariant(replState->abortTimestamp.isNull(), replState->buildUUID.toString());
-            invariant(!replState->aborted, replState->buildUUID.toString());
-            replState->aborted = true;
-            replState->abortReason = reason;
-            replState->condVar.notify_all();
+        IndexBuildDetails aborted{replState->collectionUUID};
+        // Record the index builds aborted due to rollback. This allows any rollback algorithm
+        // to efficiently restart all unfinished index builds without having to scan all indexes
+        // in all collections.
+        for (auto spec : replState->indexSpecs) {
+            aborted.indexSpecs.emplace_back(spec.getOwned());
         }
+        buildsAborted.insert({replState->buildUUID, aborted});
+
+        // Leave abort timestamp as null. This will unblock the index build and allow it to
+        // complete without cleaning up. Subsequently, the rollback algorithm can decide how to
+        // undo the index build depending on the state of the oplog. Waits for index build
+        // thread to exit.
+        abortIndexBuildByBuildUUID(opCtx, replState->buildUUID, Timestamp(), reason);
     };
     forEachIndexBuild(indexBuilds, "IndexBuildsCoordinator::onRollback - "_sd, onIndexBuild);
     return buildsAborted;
diff --git a/src/mongo/db/index_builds_coordinator.h b/src/mongo/db/index_builds_coordinator.h
index 99f0772b507..a1af0886797 100644
--- a/src/mongo/db/index_builds_coordinator.h
+++ b/src/mongo/db/index_builds_coordinator.h
@@ -238,6 +238,7 @@ public:
      */
     void abortIndexBuildByBuildUUID(OperationContext* opCtx,
                                     const UUID& buildUUID,
+                                    Timestamp abortTimestamp,
                                     const std::string& reason);
 
     /**
@@ -246,6 +247,7 @@ public:
      */
     bool abortIndexBuildByBuildUUIDNoWait(OperationContext* opCtx,
                                           const UUID& buildUUID,
+                                          Timestamp abortTimestamp,
                                           const std::string& reason);
     /**
      * Returns number of index builds in process.
diff --git a/src/mongo/db/repl/rs_rollback.cpp b/src/mongo/db/repl/rs_rollback.cpp
index 35f6901edcc..647247a228c 100644
--- a/src/mongo/db/repl/rs_rollback.cpp
+++ b/src/mongo/db/repl/rs_rollback.cpp
@@ -430,7 +430,8 @@ Status rollback_internal::updateFixUpInfoFromLocalOplogEntry(OperationContext* o
                 }
 
                 // Inserts the index name to be dropped into the set of indexes that
-                // need to be dropped for the collection.
+                // need to be dropped for the collection. Any errors dropping the index are ignored
+                // if it does not exist.
                 fixUpInfo.indexesToDrop[*uuid].insert(indexName);
 
                 return Status::OK();
@@ -448,9 +449,10 @@ Status rollback_internal::updateFixUpInfoFromLocalOplogEntry(OperationContext* o
 
                 // If the index build has been committed or aborted, and the commit or abort
                 // oplog entry has also been rolled back, the index build will have been added
-                // to the set to be restarted. Remove it, and then add it to the set to be
-                // dropped. If the index has already been dropped by abort, then this is a
-                // no-op.
+                // to the set to be restarted. An index build may also be in the set to be restarted
+                // if it was in-progress and aborted before rollback.
+                // Remove it, and then add it to the set to be dropped. If the index has already
+                // been dropped by abort, then this is a no-op.
                 auto& buildsToRestart = fixUpInfo.indexBuildsToRestart;
                 auto buildUUID = indexBuildOplogEntry.buildUUID;
                 auto existingIt = buildsToRestart.find(buildUUID);
@@ -465,7 +467,9 @@ Status rollback_internal::updateFixUpInfoFromLocalOplogEntry(OperationContext* o
                     for (auto& indexName : indexBuildOplogEntry.indexNames) {
                         fixUpInfo.indexesToDrop[*uuid].insert(indexName);
                     }
-                    return Status::OK();
+                    // Intentionally allow this index build to be added to both 'indexesToDrop' and
+                    // 'unfinishedIndexesToDrop', since we can not tell at this point if it is
+                    // finished or not.
                 }
 
                 // If the index build was not committed or aborted, the index build is
@@ -1145,6 +1149,17 @@ Status _syncRollback(OperationContext* opCtx,
         }
     } catch (const RSFatalException& e) {
         return Status(ErrorCodes::UnrecoverableRollbackError, e.what());
+    } catch (const DBException& e) {
+        // If we encounter an error during rollback, but we aborted index builds beforehand, we
+        // will be unable to successfully perform any more rollback attempts. The knowledge of these
+        // aborted index builds gets lost after the first attempt.
+        if (abortedIndexBuilds.size()) {
+            return Status{ErrorCodes::UnrecoverableRollbackError,
+                          "Index builds aborted prior to rollback cannot be restarted by "
+                          "subsequent rollback attempts"}
+                .withContext(e.what());
+        }
+        throw;
     }
 
     if (MONGO_unlikely(rollbackHangBeforeFinish.shouldFail())) {
diff --git a/src/mongo/db/repl/rs_rollback_test.cpp b/src/mongo/db/repl/rs_rollback_test.cpp
index a5b89690129..aada021db54 100644
--- a/src/mongo/db/repl/rs_rollback_test.cpp
+++ b/src/mongo/db/repl/rs_rollback_test.cpp
@@ -1061,7 +1061,7 @@ TEST_F(RSRollbackTest, RollbackCommitIndexBuild) {
 
     // Kill the index build we just restarted so the fixture can shut down.
     IndexBuildsCoordinator::get(_opCtx.get())
-        ->abortIndexBuildByBuildUUID(_opCtx.get(), buildUUID, "");
+        ->abortIndexBuildByBuildUUID(_opCtx.get(), buildUUID, Timestamp(), "");
 }
 
 TEST_F(RSRollbackTest, RollbackAbortIndexBuild) {
@@ -1105,7 +1105,7 @@ TEST_F(RSRollbackTest, RollbackAbortIndexBuild) {
 
     // Kill the index build we just restarted so the fixture can shut down.
     IndexBuildsCoordinator::get(_opCtx.get())
-        ->abortIndexBuildByBuildUUID(_opCtx.get(), buildUUID, "");
+        ->abortIndexBuildByBuildUUID(_opCtx.get(), buildUUID, Timestamp(), "");
 }
 
 TEST_F(RSRollbackTest, AbortedIndexBuildsAreRestarted) {
@@ -1154,7 +1154,7 @@ TEST_F(RSRollbackTest, AbortedIndexBuildsAreRestarted) {
 
     // Kill the index build we just restarted so the fixture can shut down.
     IndexBuildsCoordinator::get(_opCtx.get())
-        ->abortIndexBuildByBuildUUID(_opCtx.get(), buildUUID, "");
+        ->abortIndexBuildByBuildUUID(_opCtx.get(), buildUUID, Timestamp(), "");
 }
 
 TEST_F(RSRollbackTest, AbortedIndexBuildsAreNotRestartedWhenStartIsRolledBack) {