SERVER-45441 submitRangeDeletionTask should force a refresh if the metadata is unknown and delete the range deletion task if the metadata is still unknown or UUID doesn't match after the refresh

6 files changed, 168 insertions, 115 deletions

diff --git a/jstests/multiVersion/delete_pending_range_deletions_on_downgrade.js b/jstests/multiVersion/delete_pending_range_deletions_on_downgrade.js
index af9c9515dc6..194e7d9c696 100644
--- a/jstests/multiVersion/delete_pending_range_deletions_on_downgrade.js
+++ b/jstests/multiVersion/delete_pending_range_deletions_on_downgrade.js
@@ -28,13 +28,16 @@ let deletionTask = {
     collectionUuid: UUID(),
     donorShardId: "unused",
     range: {min: {x: 50}, max: {x: MaxKey}},
-    whenToClean: "now"
+    whenToClean: "now",
+    // Mark the range as pending, otherwise the task will be processed immediately on being
+    // inserted (and deleted after it's proessed) rather than being deleted on setFCV downgrade.
+    pending: true
 };
 
 let deletionsColl = st.shard0.getCollection(rangeDeletionNs);
 
 // Write range to deletion collection
-deletionsColl.insert(deletionTask);
+assert.commandWorked(deletionsColl.insert(deletionTask));
 
 // Verify deletion count.
 assert.eq(deletionsColl.find().itcount(), 1);
@@ -48,4 +51,4 @@ checkFCV(st.shard0.getDB("admin"), lastStableFCV);
 assert.eq(deletionsColl.find().itcount(), 0);
 
 st.stop();
-})();
\ No newline at end of file
+})();
diff --git a/jstests/sharding/updates_to_rangedeletions_collection_trigger_range_deletions.js b/jstests/sharding/updates_to_rangedeletions_collection_trigger_range_deletions.js
index 35c106806d4..266be4a2174 100644
--- a/jstests/sharding/updates_to_rangedeletions_collection_trigger_range_deletions.js
+++ b/jstests/sharding/updates_to_rangedeletions_collection_trigger_range_deletions.js
@@ -140,10 +140,9 @@ let testColl = testDB.foo;
     // Update deletion task
     deletionsColl.update(deletionTask, {$unset: {pending: ""}});
 
-    // Verify that UUID mismatch is logged.
+    // Verify that the deletion task gets deleted after being processed.
     assert.soon(function() {
-        return rawMongoProgramOutput().match(
-            'Collection UUID doesn\'t match the one marked for deletion:');
+        return deletionsColl.find().itcount() === 0;
     });
 
     // Verify counts on shards are correct.
diff --git a/src/mongo/db/s/migration_util.cpp b/src/mongo/db/s/migration_util.cpp
index fc0f27d564f..17d7c806a33 100644
--- a/src/mongo/db/s/migration_util.cpp
+++ b/src/mongo/db/s/migration_util.cpp
@@ -119,45 +119,79 @@ bool checkForConflictingDeletions(OperationContext* opCtx,
     return store.count(opCtx, overlappingRangeQuery(range, uuid)) > 0;
 }
 
-bool submitRangeDeletionTask(OperationContext* opCtx, const RangeDeletionTask& deletionTask) {
-    const auto whenToClean = deletionTask.getWhenToClean() == CleanWhenEnum::kNow
-        ? CollectionShardingRuntime::kNow
-        : CollectionShardingRuntime::kDelayed;
-
-    AutoGetCollection autoColl(opCtx, deletionTask.getNss(), MODE_IS);
-
-    if (!autoColl.getCollection()) {
-        LOG(0) << "Namespace not found: " << deletionTask.getNss();
-        return false;
-    }
-
-    if (autoColl.getCollection()->uuid() != deletionTask.getCollectionUuid()) {
-        LOG(0) << "Collection UUID doesn't match the one marked for deletion: "
-               << autoColl.getCollection()->uuid() << " != " << deletionTask.getCollectionUuid();
-
-        return false;
-    }
-
-    LOG(0) << "Scheduling range " << deletionTask.getRange() << " in namespace "
-           << deletionTask.getNss() << " for deletion.";
-
-    auto css = CollectionShardingRuntime::get(opCtx, deletionTask.getNss());
-
-    // TODO (SERVER-44554): This is needed for now because of the invariant that throws on cleanup
-    // if the metadata is not set.
-    if (!css->getCurrentMetadataIfKnown()) {
-        LOG(0) << "Current metadata is not available";
-        return false;
-    }
-
-    auto cleanupCompleteFuture = css->cleanUpRange(deletionTask.getRange(), whenToClean);
-
-    if (cleanupCompleteFuture.isReady() && !cleanupCompleteFuture.getNoThrow(opCtx).isOK()) {
-        LOG(0) << "Failed to resubmit range for deletion: "
-               << causedBy(cleanupCompleteFuture.getNoThrow(opCtx));
-    }
-
-    return true;
+ExecutorFuture<bool> submitRangeDeletionTask(OperationContext* opCtx,
+                                             const RangeDeletionTask& deletionTask) {
+    const auto serviceContext = opCtx->getServiceContext();
+    // TODO (SERVER-45577): Use the Grid's fixed executor once the refresh is done asynchronously.
+    // An arbitrary executor is being used temporarily because unit tests have only one thread in
+    // the fixed executor, and that thread is needed to respond to the refresh.
+    return ExecutorFuture<void>(
+               Grid::get(serviceContext)->getExecutorPool()->getArbitraryExecutor())
+        .then([=] {
+            ThreadClient tc(kRangeDeletionThreadName, serviceContext);
+            {
+                stdx::lock_guard<Client> lk(*tc.get());
+                tc->setSystemOperationKillable(lk);
+            }
+            auto uniqueOpCtx = tc->makeOperationContext();
+            auto opCtx = uniqueOpCtx.get();
+
+            boost::optional<AutoGetCollection> autoColl;
+            autoColl.emplace(opCtx, deletionTask.getNss(), MODE_IS);
+
+            auto css = CollectionShardingRuntime::get(opCtx, deletionTask.getNss());
+            if (!css->getCurrentMetadataIfKnown() ||
+                !css->getCurrentMetadata()->uuidMatches(deletionTask.getCollectionUuid())) {
+                // If the collection's filtering metadata is not known or its UUID does not match
+                // the UUID of the deletion task, force a filtering metadata refresh once, because
+                // this node may have just stepped up and therefore may have a stale cache.
+                LOG(0) << "Filtering metadata for namespace in deletion task "
+                       << deletionTask.toBSON()
+                       << (css->getCurrentMetadataIfKnown()
+                               ? "has UUID that does not match UUID of the deletion task"
+                               : "is not known")
+                       << ", forcing a refresh of " << deletionTask.getNss();
+
+                // TODO (SERVER-45577): Add an asynchronous version of
+                // forceShardFilteringMetadataRefresh to avoid blocking on the network in the
+                // thread pool.
+                autoColl.reset();
+                forceShardFilteringMetadataRefresh(opCtx, deletionTask.getNss(), true);
+            }
+
+            autoColl.emplace(opCtx, deletionTask.getNss(), MODE_IS);
+            if (!css->getCurrentMetadataIfKnown() ||
+                !css->getCurrentMetadata()->uuidMatches(deletionTask.getCollectionUuid())) {
+                LOG(0) << "Even after forced refresh, filtering metadata for namespace in deletion "
+                          "task "
+                       << deletionTask.toBSON()
+                       << (css->getCurrentMetadataIfKnown()
+                               ? "has UUID that does not match UUID of the deletion task"
+                               : "is not known")
+                       << ", deleting the task.";
+
+                autoColl.reset();
+                deleteRangeDeletionTaskLocally(
+                    opCtx, deletionTask.getId(), ShardingCatalogClient::kLocalWriteConcern);
+                return false;
+            }
+
+            LOG(0) << "Submitting range deletion task " << deletionTask.toBSON();
+
+            const auto whenToClean = deletionTask.getWhenToClean() == CleanWhenEnum::kNow
+                ? CollectionShardingRuntime::kNow
+                : CollectionShardingRuntime::kDelayed;
+
+            auto cleanupCompleteFuture = css->cleanUpRange(deletionTask.getRange(), whenToClean);
+
+            if (cleanupCompleteFuture.isReady() &&
+                !cleanupCompleteFuture.getNoThrow(opCtx).isOK()) {
+                LOG(0) << "Failed to submit range deletion task " << deletionTask.toBSON()
+                       << causedBy(cleanupCompleteFuture.getNoThrow(opCtx));
+                return false;
+            }
+            return true;
+        });
 }
 
 void submitPendingDeletions(OperationContext* opCtx) {
@@ -167,19 +201,9 @@ void submitPendingDeletions(OperationContext* opCtx) {
 
     std::vector<RangeDeletionTask> invalidRanges;
     store.forEach(opCtx, query, [&opCtx, &invalidRanges](const RangeDeletionTask& deletionTask) {
-        forceShardFilteringMetadataRefresh(opCtx, deletionTask.getNss(), true);
-
-        auto taskValid = migrationutil::submitRangeDeletionTask(opCtx, deletionTask);
-
-        if (!taskValid)
-            invalidRanges.push_back(deletionTask);
-
+        migrationutil::submitRangeDeletionTask(opCtx, deletionTask);
         return true;
     });
-
-    for (const auto& range : invalidRanges) {
-        store.remove(opCtx, Query(range.toBSON()));
-    }
 }
 
 void resubmitRangeDeletionsOnStepUp(ServiceContext* serviceContext) {
@@ -352,9 +376,11 @@ void deleteRangeDeletionTaskOnRecipient(OperationContext* opCtx,
     sendToRecipient(opCtx, recipientId, deleteOp);
 }
 
-void deleteRangeDeletionTaskLocally(OperationContext* opCtx, const UUID& deletionTaskId) {
+void deleteRangeDeletionTaskLocally(OperationContext* opCtx,
+                                    const UUID& deletionTaskId,
+                                    const WriteConcernOptions& writeConcern) {
     PersistentTaskStore<RangeDeletionTask> store(opCtx, NamespaceString::kRangeDeletionNamespace);
-    store.remove(opCtx, QUERY(RangeDeletionTask::kIdFieldName << deletionTaskId));
+    store.remove(opCtx, QUERY(RangeDeletionTask::kIdFieldName << deletionTaskId), writeConcern);
 }
 
 void deleteRangeDeletionTasksForCollectionLocally(OperationContext* opCtx,
diff --git a/src/mongo/db/s/migration_util.h b/src/mongo/db/s/migration_util.h
index 4bf07cfd40e..2b7de1b39b7 100644
--- a/src/mongo/db/s/migration_util.h
+++ b/src/mongo/db/s/migration_util.h
@@ -43,6 +43,8 @@ class ShardId;
 
 namespace migrationutil {
 
+constexpr auto kRangeDeletionThreadName = "range-deleter"_sd;
+
 /**
  * Creates a report document with the provided parameters:
  *
@@ -77,10 +79,16 @@ bool checkForConflictingDeletions(OperationContext* opCtx,
                                   const UUID& uuid);
 
 /**
- * Submits a RangeDeletionTask to the CollectionRangeDeleter.
- * Returns false if the submission failed and is not retryable.
+ * Asynchronously attempts to submit the RangeDeletionTask for processing.
+ *
+ * Note that if the current filtering metadata's UUID does not match the task's UUID, the filtering
+ * metadata will be refreshed once. If the UUID's still don't match, the task will be deleted from
+ * disk. If the UUID's do match, the task will be submitted for processing.
+ *
+ * The returned future will contain whether the task was submitted for processing.
  */
-bool submitRangeDeletionTask(OperationContext* opCtx, const RangeDeletionTask& deletionTask);
+ExecutorFuture<bool> submitRangeDeletionTask(OperationContext* oppCtx,
+                                             const RangeDeletionTask& deletionTask);
 
 /**
  * Queries the rangeDeletions collection for ranges that are ready to be deleted and submits them to
@@ -130,7 +138,10 @@ void persistAbortDecision(OperationContext* opCtx, const UUID& migrationId);
  * Deletes the range deletion task document with the specified id from config.rangeDeletions and
  * waits for majority write concern.
  */
-void deleteRangeDeletionTaskLocally(OperationContext* opCtx, const UUID& deletionTaskId);
+void deleteRangeDeletionTaskLocally(
+    OperationContext* opCtx,
+    const UUID& deletionTaskId,
+    const WriteConcernOptions& writeConcern = WriteConcerns::kMajorityWriteConcern);
 
 /**
  * Deletes all range deletion task documents with the specified collection UUID from
diff --git a/src/mongo/db/s/migration_util_test.cpp b/src/mongo/db/s/migration_util_test.cpp
index 6e41b53e07a..e6e4deaebbd 100644
--- a/src/mongo/db/s/migration_util_test.cpp
+++ b/src/mongo/db/s/migration_util_test.cpp
@@ -35,6 +35,7 @@
 #include "mongo/db/s/migration_util.h"
 #include "mongo/db/s/persistent_task_store.h"
 #include "mongo/db/s/shard_filtering_metadata_refresh.h"
+#include "mongo/db/s/wait_for_majority_service.h"
 #include "mongo/s/catalog/type_shard.h"
 #include "mongo/s/database_version_helpers.h"
 #include "mongo/s/shard_server_test_fixture.h"
@@ -54,12 +55,16 @@ protected:
     void setUp() override {
         ShardServerTestFixture::setUp();
 
+        WaitForMajorityService::get(getServiceContext()).setUp(getServiceContext());
+
         CatalogCacheLoader::get(operationContext()).initializeReplicaSetRole(true);
 
         setupNShards(2);
     }
 
     void tearDown() override {
+        WaitForMajorityService::get(getServiceContext()).shutDown();
+
         ShardServerTestFixture::tearDown();
 
         CollectionShardingStateFactory::clear(getServiceContext());
@@ -165,14 +170,18 @@ protected:
         }());
     }
 
-    void respondToMetadataRefreshRequests() {
+    void respondToMetadataRefreshRequests(boost::optional<UUID> uuid = boost::none,
+                                          bool incrementalRefresh = false) {
         const OID epoch = OID::gen();
         const ShardKeyPattern shardKeyPattern(BSON("_id" << 1));
 
-        const BSONObj databaseBSON = [&]() {
-            DatabaseType db(kNss.db().toString(), {"0"}, true, databaseVersion::makeNew());
-            return db.toBSON();
-        }();
+        if (!incrementalRefresh) {
+            const BSONObj databaseBSON = [&]() {
+                DatabaseType db(kNss.db().toString(), {"0"}, true, databaseVersion::makeNew());
+                return db.toBSON();
+            }();
+            expectFindSendBSONObjVector(kConfigHostAndPort, {databaseBSON});
+        }
 
         const BSONObj collectionBSON = [&]() {
             CollectionType coll;
@@ -180,13 +189,15 @@ protected:
             coll.setEpoch(epoch);
             coll.setKeyPattern(shardKeyPattern.getKeyPattern());
             coll.setUnique(true);
-            coll.setUUID(UUID::gen());
+            coll.setUUID(uuid ? *uuid : UUID::gen());
 
             return coll.toBSON();
         }();
 
-        expectFindSendBSONObjVector(kConfigHostAndPort, {databaseBSON});
-        expectFindSendBSONObjVector(kConfigHostAndPort, {collectionBSON});
+        if (!incrementalRefresh) {
+            expectFindSendBSONObjVector(kConfigHostAndPort, {collectionBSON});
+        }
+
         expectFindSendBSONObjVector(kConfigHostAndPort, {collectionBSON});
 
         expectFindSendBSONObjVector(kConfigHostAndPort, [&]() {
@@ -365,80 +376,82 @@ TEST_F(MigrationUtilsTest, TestInvalidUUID) {
     ASSERT_FALSE(migrationutil::checkForConflictingDeletions(opCtx, range, wrongUuid));
 }
 
-TEST_F(MigrationUtilsTest, TestSubmitRangeDeletionTask) {
-    auto opCtx = operationContext();
+using SubmitRangeDeletionTaskTest = MigrationUtilsTest;
 
-    DBDirectClient client(opCtx);
-    client.insert(kNss.toString(), BSON("_id" << 5));
+TEST_F(SubmitRangeDeletionTaskTest, SucceedsIfFilteringMetadataUUIDMatchesTaskUUID) {
+    auto opCtx = operationContext();
 
-    auto uuid = getCollectionUuid(opCtx, kNss);
+    const auto uuid = UUID::gen();
     auto deletionTask = createDeletionTask(kNss, uuid, 0, 10);
 
-    auto result = stdx::async(stdx::launch::async, [this] { respondToMetadataRefreshRequests(); });
+    // Force a metadata refresh with the task's UUID before the task is submitted.
+    auto result =
+        stdx::async(stdx::launch::async, [this, uuid] { respondToMetadataRefreshRequests(uuid); });
     forceShardFilteringMetadataRefresh(opCtx, kNss, true);
 
-    auto taskValid = migrationutil::submitRangeDeletionTask(opCtx, deletionTask);
-    ASSERT(taskValid);
+    // The task should have been submitted successfully.
+    auto submitTaskFuture = migrationutil::submitRangeDeletionTask(opCtx, deletionTask);
+    ASSERT(submitTaskFuture.get(opCtx));
 }
 
-TEST_F(MigrationUtilsTest, TestSubmitRangeNoCollection) {
+TEST_F(
+    SubmitRangeDeletionTaskTest,
+    SucceedsIfFilteringMetadataInitiallyUnknownButFilteringMetadataUUIDMatchesTaskUUIDAfterRefresh) {
     auto opCtx = operationContext();
 
-    DBDirectClient client(opCtx);
-    client.insert(kNss.toString(), BSON("_id" << 5));
-
-    auto uuid = getCollectionUuid(opCtx, kNss);
+    const auto uuid = UUID::gen();
     auto deletionTask = createDeletionTask(kNss, uuid, 0, 10);
 
-    ASSERT(client.dropCollection(kNss.toString()));
+    // Make the refresh triggered by submitting the task return a UUID that matches the task's UUID.
+    auto result =
+        stdx::async(stdx::launch::async, [this, uuid] { respondToMetadataRefreshRequests(uuid); });
 
-    auto result = stdx::async(stdx::launch::async, [this] { respondToMetadataRefreshRequests(); });
-    forceShardFilteringMetadataRefresh(opCtx, kNss, true);
-
-    auto taskValid = migrationutil::submitRangeDeletionTask(opCtx, deletionTask);
-    ASSERT_FALSE(taskValid);
+    // The task should have been submitted successfully.
+    auto submitTaskFuture = migrationutil::submitRangeDeletionTask(opCtx, deletionTask);
+    ASSERT(submitTaskFuture.get(opCtx));
 }
 
-TEST_F(MigrationUtilsTest, TestSubmitRangeNoMetadata) {
+TEST_F(SubmitRangeDeletionTaskTest,
+       SucceedsIfFilteringMetadataUUIDInitiallyDifferentFromTaskUUIDButMatchesAfterRefresh) {
     auto opCtx = operationContext();
 
-    DBDirectClient client(opCtx);
-    client.insert(kNss.toString(), BSON("_id" << 5));
+    // Force a metadata refresh with an arbitrary UUID so that the node's filtering metadata is
+    // stale when the task is submitted.
+    auto result1 = stdx::async(stdx::launch::async, [this] { respondToMetadataRefreshRequests(); });
+    forceShardFilteringMetadataRefresh(opCtx, kNss, true);
 
-    auto uuid = getCollectionUuid(opCtx, kNss);
+    const auto uuid = UUID::gen();
     auto deletionTask = createDeletionTask(kNss, uuid, 0, 10);
 
-    auto taskValid = migrationutil::submitRangeDeletionTask(opCtx, deletionTask);
-    ASSERT_FALSE(taskValid);
-}
-
-TEST_F(MigrationUtilsTest, TestSubmitRangeWrongUUID) {
-    auto opCtx = operationContext();
-
-    DBDirectClient client(opCtx);
-
-    client.insert(kNss.toString(), BSON("_id" << 5));
+    // Make the refresh triggered by submitting the task return a UUID that matches the task's UUID.
+    auto result2 = stdx::async(stdx::launch::async, [this, uuid] {
+        respondToMetadataRefreshRequests(uuid, true /* incrementalRefresh */);
+    });
 
-    const auto wrongUuid = UUID::gen();
-    auto deletionTask = createDeletionTask(kNss, wrongUuid, 0, 10);
-
-    auto taskValid = migrationutil::submitRangeDeletionTask(opCtx, deletionTask);
-    ASSERT_FALSE(taskValid);
+    // The task should have been submitted successfully.
+    auto submitTaskFuture = migrationutil::submitRangeDeletionTask(opCtx, deletionTask);
+    ASSERT(submitTaskFuture.get(opCtx));
 }
 
-TEST_F(MigrationUtilsTest, TestInvalidRangeIsDeleted) {
+TEST_F(SubmitRangeDeletionTaskTest,
+       FailsAndDeletesTaskIfFilteringMetadataUUIDDifferentFromTaskUUIDEvenAfterRefresh) {
     auto opCtx = operationContext();
+
     const auto uuid = UUID::gen();
+    auto deletionTask = createDeletionTask(kNss, uuid, 0, 10);
 
     PersistentTaskStore<RangeDeletionTask> store(opCtx, NamespaceString::kRangeDeletionNamespace);
-
-    store.add(opCtx, createDeletionTask(kNss, uuid, 10, 20, false));
-
+    store.add(opCtx, deletionTask);
     ASSERT_EQ(store.count(opCtx), 1);
 
-    auto result = stdx::async(stdx::launch::async, [this] { respondToMetadataRefreshRequests(); });
-    migrationutil::submitPendingDeletions(opCtx);
+    // Make the refresh triggered by submitting the task return an arbitrary UUID.
+    auto result2 =
+        stdx::async(stdx::launch::async, [this, uuid] { respondToMetadataRefreshRequests(); });
 
+    // The task should not have been submitted, and the task's entry should have been removed from
+    // the persistent store.
+    auto submitTaskFuture = migrationutil::submitRangeDeletionTask(opCtx, deletionTask);
+    ASSERT_FALSE(submitTaskFuture.get(opCtx));
     ASSERT_EQ(store.count(opCtx), 0);
 }
 
diff --git a/src/mongo/db/s/range_deletion_util.cpp b/src/mongo/db/s/range_deletion_util.cpp
index 561b032a452..65388667e3e 100644
--- a/src/mongo/db/s/range_deletion_util.cpp
+++ b/src/mongo/db/s/range_deletion_util.cpp
@@ -51,6 +51,7 @@
 #include "mongo/db/query/query_knobs_gen.h"
 #include "mongo/db/query/query_planner.h"
 #include "mongo/db/repl/repl_client_info.h"
+#include "mongo/db/s/migration_util.h"
 #include "mongo/db/s/persistent_task_store.h"
 #include "mongo/db/s/range_deletion_task_gen.h"
 #include "mongo/db/s/sharding_statistics.h"
@@ -210,7 +211,7 @@ StatusWith<int> deleteNextBatch(OperationContext* opCtx,
 
 template <typename Callable>
 auto withTemporaryOperationContext(Callable&& callable) {
-    ThreadClient tc("Collection-Range-Deleter", getGlobalServiceContext());
+    ThreadClient tc(migrationutil::kRangeDeletionThreadName, getGlobalServiceContext());
     {
         stdx::lock_guard<Client> lk(*tc.get());
         tc->setSystemOperationKillable(lk);