SERVER-48293 change update to upsert to mitigate race condition leading to orphaned system.indexBuilds

4 files changed, 70 insertions, 9 deletions

diff --git a/src/mongo/db/dbhelpers.cpp b/src/mongo/db/dbhelpers.cpp
index 29e2ce7f289..f20ce257252 100644
--- a/src/mongo/db/dbhelpers.cpp
+++ b/src/mongo/db/dbhelpers.cpp
@@ -250,7 +250,26 @@ void Helpers::upsert(OperationContext* opCtx,
     request.setFromMigration(fromMigrate);
     request.setYieldPolicy(PlanYieldPolicy::YieldPolicy::NO_YIELD);
 
-    update(opCtx, context.db(), request);
+    ::mongo::update(opCtx, context.db(), request);
+}
+
+void Helpers::update(OperationContext* opCtx,
+                     const string& ns,
+                     const BSONObj& filter,
+                     const BSONObj& updateMod,
+                     bool fromMigrate) {
+    OldClientContext context(opCtx, ns);
+
+    const NamespaceString requestNs(ns);
+    auto request = UpdateRequest();
+    request.setNamespaceString(requestNs);
+
+    request.setQuery(filter);
+    request.setUpdateModification(write_ops::UpdateModification::parseFromClassicUpdate(updateMod));
+    request.setFromMigration(fromMigrate);
+    request.setYieldPolicy(PlanYieldPolicy::YieldPolicy::NO_YIELD);
+
+    ::mongo::update(opCtx, context.db(), request);
 }
 
 void Helpers::putSingleton(OperationContext* opCtx, const char* ns, BSONObj obj) {
@@ -263,7 +282,7 @@ void Helpers::putSingleton(OperationContext* opCtx, const char* ns, BSONObj obj)
     request.setUpdateModification(write_ops::UpdateModification::parseFromClassicUpdate(obj));
     request.setUpsert();
 
-    update(opCtx, context.db(), request);
+    ::mongo::update(opCtx, context.db(), request);
 
     CurOp::get(opCtx)->done();
 }
diff --git a/src/mongo/db/dbhelpers.h b/src/mongo/db/dbhelpers.h
index 514a4eda352..3bdf368490a 100644
--- a/src/mongo/db/dbhelpers.h
+++ b/src/mongo/db/dbhelpers.h
@@ -137,6 +137,18 @@ struct Helpers {
                        const BSONObj& updateMod,
                        bool fromMigrate = false);
 
+    /**
+     * Performs an update of 'updateMod' for the entry matching the given 'filter'.
+     * Callers are expected to hold the collection lock.
+     * Note: Query yielding is turned off, so both read and writes are performed
+     * on the same storage snapshot.
+     */
+    static void update(OperationContext* opCtx,
+                       const std::string& ns,
+                       const BSONObj& filter,
+                       const BSONObj& updateMod,
+                       bool fromMigrate = false);
+
     // TODO: this should be somewhere else probably
     /* Takes object o, and returns a new object with the
      * same field elements but the names stripped out.
diff --git a/src/mongo/db/index_build_entry_helpers.cpp b/src/mongo/db/index_build_entry_helpers.cpp
index fc689873f6e..7a24324cac8 100644
--- a/src/mongo/db/index_build_entry_helpers.cpp
+++ b/src/mongo/db/index_build_entry_helpers.cpp
@@ -137,6 +137,32 @@ Status upsert(OperationContext* opCtx, const BSONObj& filter, const BSONObj& upd
                               });
 }
 
+Status update(OperationContext* opCtx, const BSONObj& filter, const BSONObj& updateMod) {
+    return writeConflictRetry(opCtx,
+                              "updateIndexBuildEntry",
+                              NamespaceString::kIndexBuildEntryNamespace.ns(),
+                              [&]() -> Status {
+                                  AutoGetCollection autoCollection(
+                                      opCtx, NamespaceString::kIndexBuildEntryNamespace, MODE_IX);
+                                  const Collection* collection = autoCollection.getCollection();
+                                  if (!collection) {
+                                      str::stream ss;
+                                      ss << "Collection not found: "
+                                         << NamespaceString::kIndexBuildEntryNamespace.ns();
+                                      return Status(ErrorCodes::NamespaceNotFound, ss);
+                                  }
+
+                                  WriteUnitOfWork wuow(opCtx);
+                                  Helpers::update(opCtx,
+                                                  NamespaceString::kIndexBuildEntryNamespace.ns(),
+                                                  filter,
+                                                  updateMod,
+                                                  /*fromMigrate=*/false);
+                                  wuow.commit();
+                                  return Status::OK();
+                              });
+}
+
 }  // namespace
 
 namespace indexbuildentryhelpers {
@@ -176,7 +202,11 @@ Status persistCommitReadyMemberInfo(OperationContext* opCtx,
               !indexBuildEntry.getCommitQuorum().isInitialized());
 
     auto [filter, updateMod] = buildIndexBuildEntryFilterAndUpdate(indexBuildEntry);
-    return upsert(opCtx, filter, updateMod);
+
+    // Only update if the document still exists. We update instead of upsert so that we don't race
+    // with the index build commit / abort that deletes the document; upserting after committing /
+    // aborting would insert instead, and lead to an orphaned document.
+    return update(opCtx, filter, updateMod);
 }
 
 Status persistIndexCommitQuorum(OperationContext* opCtx, const IndexBuildEntry& indexBuildEntry) {
diff --git a/src/mongo/db/index_builds_coordinator_mongod.cpp b/src/mongo/db/index_builds_coordinator_mongod.cpp
index f503f4f572d..89627a1f42e 100644
--- a/src/mongo/db/index_builds_coordinator_mongod.cpp
+++ b/src/mongo/db/index_builds_coordinator_mongod.cpp
@@ -391,8 +391,7 @@ Status IndexBuildsCoordinatorMongod::voteCommitIndexBuild(OperationContext* opCt
     // commit quorum on (i.e., commit value set as non-zero or a valid tag) and vice-versa. So,
     // after this point, it's not possible for the index build's commit quorum value to get updated
     // to CommitQuorumOptions::kDisabled.
-
-    Status upsertStatus(ErrorCodes::InternalError, "Uninitialized value");
+    Status persistStatus = Status::OK();
 
     IndexBuildEntry indexbuildEntry(
         buildUUID, replState->collectionUUID, CommitQuorumOptions(), replState->indexNames);
@@ -400,15 +399,16 @@ Status IndexBuildsCoordinatorMongod::voteCommitIndexBuild(OperationContext* opCt
     indexbuildEntry.setCommitReadyMembers(votersList);
 
     {
-        // Upserts doesn't need to acquire pbwm lock.
+        // Updates don't need to acquire pbwm lock.
         ShouldNotConflictWithSecondaryBatchApplicationBlock noPBWMBlock(opCtx->lockState());
-        upsertStatus = indexbuildentryhelpers::persistCommitReadyMemberInfo(opCtx, indexbuildEntry);
+        persistStatus =
+            indexbuildentryhelpers::persistCommitReadyMemberInfo(opCtx, indexbuildEntry);
     }
 
-    if (upsertStatus.isOK()) {
+    if (persistStatus.isOK()) {
         _signalIfCommitQuorumIsSatisfied(opCtx, replState);
     }
-    return upsertStatus;
+    return persistStatus;
 }
 
 void IndexBuildsCoordinatorMongod::setSignalAndCancelVoteRequestCbkIfActive(