SERVER-38748 Background indexes created through applyOps should run on the command thread

4 files changed, 84 insertions, 59 deletions

diff --git a/jstests/core/apply_ops_invalid_index_spec.js b/jstests/core/apply_ops_invalid_index_spec.js
index 01a4fab3a63..5ed9e6d8ee6 100644
--- a/jstests/core/apply_ops_invalid_index_spec.js
+++ b/jstests/core/apply_ops_invalid_index_spec.js
@@ -50,6 +50,24 @@
     }),
                                  ErrorCodes.InvalidIndexSpecificationOption);
 
+    // A createIndexes command for a background index with unknown field in the index spec should
+    // fail.
+    assert.commandFailedWithCode(db.adminCommand({
+        applyOps: [{
+            op: 'c',
+            ns: cmdNs,
+            o: {
+                createIndexes: t.getName(),
+                v: 2,
+                key: {a: 1},
+                background: true,
+                name: 'a_1_background',
+                unknown: 1,
+            },
+        }],
+    }),
+                                 ErrorCodes.InvalidIndexSpecificationOption);
+
     // A createIndexes command for a v:1 index with an unknown field in the index spec should work.
     const res1 = assert.commandWorked(db.adminCommand({
         applyOps: [{
diff --git a/jstests/noPassthrough/indexbg_killop_apply_ops.js b/jstests/noPassthrough/indexbg_killop_apply_ops.js
index d99e45f65f6..d7bb3da18a9 100644
--- a/jstests/noPassthrough/indexbg_killop_apply_ops.js
+++ b/jstests/noPassthrough/indexbg_killop_apply_ops.js
@@ -1,6 +1,8 @@
 /**
- * Confirms that background index builds started through applyOps on a primary cannot be aborted
- * using killop.
+ * Confirms that background index builds started through applyOps on a primary can be aborted using
+ * killop. This is because primaries run background index builds in the foreground when run through
+ * applyOps.
+ *
  * @tags: [requires_replication]
  */
 (function() {
@@ -46,25 +48,32 @@
             },
         ]
     };
-    const createIdx =
-        startParallelShell('db.adminCommand(' + tojson(applyOpsCmd) + ')', primary.port);
+    const createIdx = startParallelShell(
+        'assert.commandWorked(db.adminCommand(' + tojson(applyOpsCmd) + '))', primary.port);
 
     // When the index build starts, find its op id.
     const opId = IndexBuildTest.waitForIndexBuildToStart(testDB);
 
+    // CurOp output should contain the current state of the index builder such as the build UUID
+    // and build phase.
+    IndexBuildTest.assertIndexBuildCurrentOpContents(testDB, opId, false);
+
     // Kill the index build. This should have no effect.
     assert.commandWorked(testDB.killOp(opId));
 
     // Wait for the index build to stop.
-    IndexBuildTest.resumeIndexBuilds(primary);
-    IndexBuildTest.waitForIndexBuildToStop(testDB);
+    try {
+        IndexBuildTest.waitForIndexBuildToStop(testDB);
+    } finally {
+        IndexBuildTest.resumeIndexBuilds(primary);
+    }
 
-    // Expect successful createIndex command invocation in parallel shell because applyOps returns
-    // immediately after starting the background index build in a separate thread.
-    createIdx();
+    const exitCode = createIdx({checkExitSuccess: false});
+    assert.neq(
+        0, exitCode, 'expected shell to exit abnormally due to index build being terminated');
 
     // Check that index was created on the primary despite the attempted killOp().
-    IndexBuildTest.assertIndexes(coll, 2, ['_id_', 'a_1']);
+    IndexBuildTest.assertIndexes(coll, 1, ['_id_']);
 
     rst.stopSet();
 })();
diff --git a/src/mongo/db/repl/oplog.cpp b/src/mongo/db/repl/oplog.cpp
index 428bee03dfd..27a1f77282c 100644
--- a/src/mongo/db/repl/oplog.cpp
+++ b/src/mongo/db/repl/oplog.cpp
@@ -212,6 +212,33 @@ private:
     BSONObj _oField;
 };
 
+bool shouldBuildInForeground(OperationContext* opCtx,
+                             const BSONObj& index,
+                             const NamespaceString& indexNss,
+                             repl::OplogApplication::Mode mode) {
+    if (mode == OplogApplication::Mode::kRecovering) {
+        LOG(3) << "apply op: building background index " << index
+               << " in the foreground because the node is in recovery";
+        return true;
+    }
+
+    if (!index["background"].trueValue()) {
+        return true;
+    }
+
+    // Primaries should build indexes in the foreground because failures cannot be handled
+    // by the background thread.
+    const bool isPrimary =
+        repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesFor(opCtx, indexNss);
+    if (isPrimary) {
+        LOG(3) << "apply op: not building background index " << index
+               << " in a background thread because this is a primary";
+        return true;
+    }
+    return false;
+}
+
+
 }  // namespace
 
 void setOplogCollectionName(ServiceContext* service) {
@@ -255,41 +282,25 @@ void createIndexForApplyOps(OperationContext* opCtx,
         ? IndexBuilder::ReplicatedWrites::kReplicated
         : IndexBuilder::ReplicatedWrites::kUnreplicated;
 
-    if (indexSpec["background"].trueValue()) {
-        if (mode == OplogApplication::Mode::kRecovering) {
-            LOG(3) << "apply op: building background index " << indexSpec
-                   << " in the foreground because the node is in recovery";
-            IndexBuilder builder(indexSpec, constraints, replicatedWrites);
-            Status status = builder.buildInForeground(opCtx, db);
-            uassertStatusOK(status);
-        } else {
-            Lock::TempRelease release(opCtx->lockState());
-            if (opCtx->lockState()->isLocked()) {
-                // If TempRelease fails, background index build will deadlock.
-                LOG(3) << "apply op: building background index " << indexSpec
-                       << " in the foreground because temp release failed";
-                IndexBuilder builder(indexSpec, constraints, replicatedWrites);
-                Status status = builder.buildInForeground(opCtx, db);
-                uassertStatusOK(status);
-            } else {
-                IndexBuilder* builder =
-                    new IndexBuilder(indexSpec,
-                                     constraints,
-                                     replicatedWrites,
-                                     opCtx->recoveryUnit()->getCommitTimestamp());
-                // This spawns a new thread and returns immediately.
-                builder->go();
-                // Wait for thread to start and register itself
-                IndexBuilder::waitForBgIndexStarting();
-            }
-        }
-
-        opCtx->recoveryUnit()->abandonSnapshot();
-    } else {
+    if (shouldBuildInForeground(opCtx, indexSpec, indexNss, mode)) {
         IndexBuilder builder(indexSpec, constraints, replicatedWrites);
         Status status = builder.buildInForeground(opCtx, db);
         uassertStatusOK(status);
+    } else {
+        Lock::TempRelease release(opCtx->lockState());
+        // TempRelease cannot fail because no recursive locks should be taken.
+        invariant(!opCtx->lockState()->isLocked());
+
+        IndexBuilder* builder = new IndexBuilder(
+            indexSpec, constraints, replicatedWrites, opCtx->recoveryUnit()->getCommitTimestamp());
+        // This spawns a new thread and returns immediately.
+        builder->go();
+        // Wait for thread to start and register itself
+        IndexBuilder::waitForBgIndexStarting();
     }
+
+    opCtx->recoveryUnit()->abandonSnapshot();
+
     if (incrementOpsAppliedStats) {
         incrementOpsAppliedStats();
     }
diff --git a/src/mongo/dbtests/storage_timestamp_tests.cpp b/src/mongo/dbtests/storage_timestamp_tests.cpp
index d856592ff64..a3b701d51e9 100644
--- a/src/mongo/dbtests/storage_timestamp_tests.cpp
+++ b/src/mongo/dbtests/storage_timestamp_tests.cpp
@@ -2298,9 +2298,8 @@ public:
             // Create a background index via `applyOps`. We will timestamp the beginning at
             // `startBuildTs` and the end, due to manipulation of the logical clock, should be
             // timestamped at `endBuildTs`.
-            const auto beforeBuildTime = _clock->reserveTicks(3);
+            const auto beforeBuildTime = _clock->reserveTicks(2);
             const auto startBuildTs = beforeBuildTime.addTicks(1).asTimestamp();
-            const auto endBuildTs = beforeBuildTime.addTicks(3).asTimestamp();
 
             // Grab the existing idents to identify the ident created by the index build.
             auto kvStorageEngine =
@@ -2339,22 +2338,10 @@ public:
             assertIdentsMissingAtTimestamp(
                 kvCatalog, "", indexIdent, beforeBuildTime.asTimestamp());
             assertIdentsExistAtTimestamp(kvCatalog, "", indexIdent, startBuildTs);
-            if (Foreground) {
-                // In the Foreground case, the index build should start and finish at
-                // `startBuildTs`.
-                ASSERT_TRUE(
-                    getIndexMetaData(getMetaDataAtTime(kvCatalog, nss, startBuildTs), "field_1")
-                        .ready);
-            } else {
-                // In the Background case, the index build should not be "ready" at `startBuildTs`.
-                ASSERT_FALSE(
-                    getIndexMetaData(getMetaDataAtTime(kvCatalog, nss, startBuildTs), "field_1")
-                        .ready);
-                assertIdentsExistAtTimestamp(kvCatalog, "", indexIdent, endBuildTs);
-                ASSERT_TRUE(
-                    getIndexMetaData(getMetaDataAtTime(kvCatalog, nss, endBuildTs), "field_1")
-                        .ready);
-            }
+
+            // In all cases, the index build should start and finish at `startBuildTs`.
+            ASSERT_TRUE(
+                getIndexMetaData(getMetaDataAtTime(kvCatalog, nss, startBuildTs), "field_1").ready);
         }
     }
 };