SERVER-42915: Do not require a resync when repair encounters orphaned collection objects.

(cherry picked from commit f4e387fa1b7e369ce067650bdda9c8676683b929)

7 files changed, 123 insertions, 37 deletions

diff --git a/src/mongo/db/repair_database_and_check_version.cpp b/src/mongo/db/repair_database_and_check_version.cpp
index e455e355ad3..339d4a92d39 100644
--- a/src/mongo/db/repair_database_and_check_version.cpp
+++ b/src/mongo/db/repair_database_and_check_version.cpp
@@ -405,17 +405,25 @@ StatusWith<bool> repairDatabasesAndCheckVersion(OperationContext* opCtx) {
         // config.
         auto repairObserver = StorageRepairObserver::get(opCtx->getServiceContext());
         repairObserver->onRepairDone(opCtx);
-        if (repairObserver->isDataModified()) {
+        if (repairObserver->getModifications().size() > 0) {
             warning() << "Modifications made by repair:";
             const auto& mods = repairObserver->getModifications();
             for (const auto& mod : mods) {
-                warning() << "  " << mod;
+                warning() << "  " << mod.getDescription();
             }
+        }
+        if (repairObserver->isDataInvalidated()) {
             if (hasReplSetConfigDoc(opCtx)) {
                 warning() << "WARNING: Repair may have modified replicated data. This node will no "
                              "longer be able to join a replica set without a full re-sync";
             }
         }
+
+        // There were modifications, but only benign ones.
+        if (repairObserver->getModifications().size() > 0 && !repairObserver->isDataInvalidated()) {
+            log() << "Repair has made modifications to unreplicated data. The data is healthy and "
+                     "the node is eligible to be returned to the replica set.";
+        }
     }
 
     const repl::ReplSettings& replSettings =
diff --git a/src/mongo/db/storage/kv/kv_storage_engine.cpp b/src/mongo/db/storage/kv/kv_storage_engine.cpp
index 8d503896d66..c9dd5b15457 100644
--- a/src/mongo/db/storage/kv/kv_storage_engine.cpp
+++ b/src/mongo/db/storage/kv/kv_storage_engine.cpp
@@ -57,7 +57,7 @@ using std::vector;
 namespace {
 const std::string catalogInfo = "_mdb_catalog";
 const auto kCatalogLogLevel = logger::LogSeverity::Debug(2);
-}
+}  // namespace
 
 class KVStorageEngine::RemoveDBChange : public RecoveryUnit::Change {
 public:
@@ -107,8 +107,8 @@ void KVStorageEngine::loadCatalog(OperationContext* opCtx) {
 
         if (status.code() == ErrorCodes::DataModifiedByRepair) {
             warning() << "Catalog data modified by repair: " << status.reason();
-            repairObserver->onModification(str::stream() << "KVCatalog repaired: "
-                                                         << status.reason());
+            repairObserver->invalidatingModification(str::stream() << "KVCatalog repaired: "
+                                                                   << status.reason());
         } else {
             fassertNoTrace(50926, status);
         }
@@ -182,8 +182,8 @@ void KVStorageEngine::loadCatalog(OperationContext* opCtx) {
                                      "build the index.";
 
                         StorageRepairObserver::get(getGlobalServiceContext())
-                            ->onModification(str::stream() << "Orphan collection created: "
-                                                           << statusWithNs.getValue());
+                            ->benignModification(str::stream() << "Orphan collection created: "
+                                                               << statusWithNs.getValue());
 
                     } else {
                         // Log an error message if we cannot create the entry.
@@ -225,8 +225,9 @@ void KVStorageEngine::loadCatalog(OperationContext* opCtx) {
 
                     if (_options.forRepair) {
                         StorageRepairObserver::get(getGlobalServiceContext())
-                            ->onModification(str::stream() << "Collection " << coll << " dropped: "
-                                                           << status.reason());
+                            ->invalidatingModification(str::stream() << "Collection " << coll
+                                                                     << " dropped: "
+                                                                     << status.reason());
                     }
                     wuow.commit();
                     continue;
@@ -296,8 +297,9 @@ Status KVStorageEngine::_recoverOrphanedCollection(OperationContext* opCtx,
     }
     if (dataModified) {
         StorageRepairObserver::get(getGlobalServiceContext())
-            ->onModification(str::stream() << "Collection " << collectionName.ns() << " recovered: "
-                                           << status.reason());
+            ->invalidatingModification(str::stream() << "Collection " << collectionName.ns()
+                                                     << " recovered: "
+                                                     << status.reason());
     }
     wuow.commit();
     return Status::OK();
@@ -614,8 +616,8 @@ Status KVStorageEngine::repairRecordStore(OperationContext* opCtx, const std::st
     }
 
     if (_options.forRepair && dataModified) {
-        repairObserver->onModification(str::stream() << "Collection " << ns << ": "
-                                                     << status.reason());
+        repairObserver->invalidatingModification(str::stream() << "Collection " << ns << ": "
+                                                               << status.reason());
     }
     _dbs[nsToDatabase(ns)]->reinitCollectionAfterRepair(opCtx, ns);
 
diff --git a/src/mongo/db/storage/kv/kv_storage_engine_test.cpp b/src/mongo/db/storage/kv/kv_storage_engine_test.cpp
index dad4beb2295..35180ab15b9 100644
--- a/src/mongo/db/storage/kv/kv_storage_engine_test.cpp
+++ b/src/mongo/db/storage/kv/kv_storage_engine_test.cpp
@@ -157,7 +157,7 @@ public:
 
         unittest::log() << "Modifications: ";
         for (const auto& mod : repairObserver->getModifications()) {
-            unittest::log() << "  " << mod;
+            unittest::log() << "  " << mod.getDescription();
         }
     }
 };
diff --git a/src/mongo/db/storage/storage_repair_observer.cpp b/src/mongo/db/storage/storage_repair_observer.cpp
index 4b3065355c8..acc36e33a01 100644
--- a/src/mongo/db/storage/storage_repair_observer.cpp
+++ b/src/mongo/db/storage/storage_repair_observer.cpp
@@ -32,6 +32,7 @@
 
 #include "mongo/db/storage/storage_repair_observer.h"
 
+#include <algorithm>
 #include <cerrno>
 #include <cstring>
 
@@ -89,9 +90,14 @@ void StorageRepairObserver::onRepairStarted() {
     _repairState = RepairState::kIncomplete;
 }
 
-void StorageRepairObserver::onModification(const std::string& description) {
+void StorageRepairObserver::benignModification(const std::string& description) {
     invariant(_repairState == RepairState::kIncomplete);
-    _modifications.emplace_back(description);
+    _modifications.emplace_back(Modification::benign(description));
+}
+
+void StorageRepairObserver::invalidatingModification(const std::string& description) {
+    invariant(_repairState == RepairState::kIncomplete);
+    _modifications.emplace_back(Modification::invalidating(description));
 }
 
 void StorageRepairObserver::onRepairDone(OperationContext* opCtx) {
@@ -99,7 +105,7 @@ void StorageRepairObserver::onRepairDone(OperationContext* opCtx) {
 
     // This ordering is important. The incomplete file should only be removed once the
     // replica set configuration has been invalidated successfully.
-    if (!_modifications.empty()) {
+    if (isDataInvalidated()) {
         _invalidateReplConfigIfNeeded(opCtx);
     }
     _removeRepairIncompleteFile();
@@ -107,6 +113,13 @@ void StorageRepairObserver::onRepairDone(OperationContext* opCtx) {
     _repairState = RepairState::kDone;
 }
 
+bool StorageRepairObserver::isDataInvalidated() const {
+    invariant(_repairState == RepairState::kIncomplete || _repairState == RepairState::kDone);
+    return std::any_of(_modifications.begin(), _modifications.end(), [](Modification mod) -> bool {
+        return mod.isInvalidating();
+    });
+}
+
 void StorageRepairObserver::_touchRepairIncompleteFile() {
     boost::filesystem::ofstream fileStream(_repairIncompleteFilePath);
     fileStream << "This file indicates that a repair operation is in progress or incomplete.";
diff --git a/src/mongo/db/storage/storage_repair_observer.h b/src/mongo/db/storage/storage_repair_observer.h
index 6f790bf13aa..ad5165b42b5 100644
--- a/src/mongo/db/storage/storage_repair_observer.h
+++ b/src/mongo/db/storage/storage_repair_observer.h
@@ -62,6 +62,32 @@ public:
      */
     void onRepairStarted();
 
+    class Modification {
+    public:
+        static Modification invalidating(const std::string& description) {
+            return {description, true};
+        }
+
+        static Modification benign(const std::string& description) {
+            return {description, false};
+        }
+
+        const std::string& getDescription() const {
+            return _description;
+        }
+
+        bool isInvalidating() const {
+            return _invalidating;
+        }
+
+    private:
+        Modification(const std::string& description, bool invalidating)
+            : _description(description), _invalidating(invalidating) {}
+
+        std::string _description;
+        bool _invalidating;
+    };
+
     /**
      * Indicate that a data modification was made by repair. If even a single call is made, the
      * replica set configuration will be invalidated.
@@ -69,12 +95,22 @@ public:
      * Provide a 'description' of the modification that will added to a list of modifications by
      * getModifications();
      */
-    void onModification(const std::string& description);
+    void invalidatingModification(const std::string& description);
+
+    /**
+     * Indicates a data modification was made by repair, but the change does not indicate the node's
+     * data is different than other replica set members. The replica set configuration will be left
+     * alone if all modifications are benign.
+     *
+     * Provide a 'description' of the modification that will added to a list of modifications by
+     * getModifications();
+     */
+    void benignModification(const std::string& description);
 
     /**
      * This must be called to notify the repair observer that a database repair operation completed
-     * successfully. If any calls to onModification have been made, this invalidates the replica set
-     * configuration so this node will be unable to rejoin a replica set.
+     * successfully. If any calls to invalidatingModification have been made, this invalidates the
+     * replica set configuration so this node will be unable to rejoin a replica set.
      *
      * May only be called after a call to onRepairStarted().
      */
@@ -95,20 +131,16 @@ public:
     }
 
     /**
-     * Returns 'true' if repair modified data.
+     * Returns 'true' if repair modified data in an invalidating way.
      *
      * May only be called after a call to onRepairDone().
      */
-    bool isDataModified() const {
-        invariant(_repairState == RepairState::kIncomplete || _repairState == RepairState::kDone);
-        return !_modifications.empty();
-    }
+    bool isDataInvalidated() const;
 
-    const std::vector<std::string>& getModifications() const {
+    const std::vector<Modification>& getModifications() const {
         return _modifications;
     }
 
-
 private:
     enum class RepairState {
         /**
@@ -136,7 +168,8 @@ private:
 
     boost::filesystem::path _repairIncompleteFilePath;
     RepairState _repairState;
-    std::vector<std::string> _modifications;
+
+    std::vector<Modification> _modifications;
 };
 
 }  // namespace mongo
diff --git a/src/mongo/db/storage/storage_repair_observer_test.cpp b/src/mongo/db/storage/storage_repair_observer_test.cpp
index 73745a00462..c46b8bfd0d7 100644
--- a/src/mongo/db/storage/storage_repair_observer_test.cpp
+++ b/src/mongo/db/storage/storage_repair_observer_test.cpp
@@ -107,10 +107,10 @@ public:
             ASSERT(!repairObserver->isIncomplete());
         }
 
-        if (repairObserver->isDone() && repairObserver->isDataModified()) {
+        if (repairObserver->isDone() && repairObserver->isDataInvalidated()) {
             unittest::log() << "Modifications: ";
             for (const auto& mod : repairObserver->getModifications()) {
-                unittest::log() << "  " << mod;
+                unittest::log() << "  " << mod.getDescription();
             }
         }
     }
@@ -139,7 +139,7 @@ TEST_F(StorageRepairObserverTest, DataUnmodified) {
     ASSERT(!boost::filesystem::exists(repairFile));
 
     ASSERT(repairObserver->isDone());
-    ASSERT(!repairObserver->isDataModified());
+    ASSERT(!repairObserver->isDataInvalidated());
 
     assertReplConfigValid(opCtx.get(), true);
 }
@@ -157,7 +157,7 @@ TEST_F(StorageRepairObserverTest, DataModified) {
     ASSERT(boost::filesystem::exists(repairFile));
 
     std::string mod("Collection mod");
-    repairObserver->onModification(mod);
+    repairObserver->invalidatingModification(mod);
 
     auto opCtx = cc().makeOperationContext();
     Lock::GlobalWrite lock(opCtx.get());
@@ -168,12 +168,42 @@ TEST_F(StorageRepairObserverTest, DataModified) {
     ASSERT(!boost::filesystem::exists(repairFile));
 
     ASSERT(repairObserver->isDone());
-    ASSERT(repairObserver->isDataModified());
+    ASSERT(repairObserver->isDataInvalidated());
     ASSERT_EQ(1U, repairObserver->getModifications().size());
 
     assertReplConfigValid(opCtx.get(), false);
 }
 
+TEST_F(StorageRepairObserverTest, DataValidAfterBenignModification) {
+    auto repairObserver = getRepairObserver();
+
+    auto repairFile = repairFilePath();
+    ASSERT(!boost::filesystem::exists(repairFile));
+    ASSERT(!repairObserver->isIncomplete());
+
+    repairObserver->onRepairStarted();
+
+    ASSERT(repairObserver->isIncomplete());
+    ASSERT(boost::filesystem::exists(repairFile));
+
+    std::string mod("Collection mod");
+    repairObserver->benignModification(mod);
+
+    auto opCtx = cc().makeOperationContext();
+    Lock::GlobalWrite lock(opCtx.get());
+    createMockReplConfig(opCtx.get());
+
+    repairObserver->onRepairDone(opCtx.get());
+    ASSERT(!repairObserver->isIncomplete());
+    ASSERT(!boost::filesystem::exists(repairFile));
+
+    ASSERT(repairObserver->isDone());
+    ASSERT(!repairObserver->isDataInvalidated());
+    ASSERT_EQ(1U, repairObserver->getModifications().size());
+
+    assertReplConfigValid(opCtx.get(), true);
+}
+
 TEST_F(StorageRepairObserverTest, DataModifiedDoesNotCreateReplConfigOnStandalone) {
     auto repairObserver = getRepairObserver();
 
@@ -187,7 +217,7 @@ TEST_F(StorageRepairObserverTest, DataModifiedDoesNotCreateReplConfigOnStandalon
     ASSERT(boost::filesystem::exists(repairFile));
 
     std::string mod("Collection mod");
-    repairObserver->onModification(mod);
+    repairObserver->invalidatingModification(mod);
 
     auto opCtx = cc().makeOperationContext();
     Lock::GlobalWrite lock(opCtx.get());
@@ -197,7 +227,7 @@ TEST_F(StorageRepairObserverTest, DataModifiedDoesNotCreateReplConfigOnStandalon
     ASSERT(!boost::filesystem::exists(repairFile));
 
     ASSERT(repairObserver->isDone());
-    ASSERT(repairObserver->isDataModified());
+    ASSERT(repairObserver->isDataInvalidated());
     ASSERT_EQ(1U, repairObserver->getModifications().size());
     ASSERT(!hasReplConfig(opCtx.get()));
 }
@@ -239,7 +269,7 @@ TEST_F(StorageRepairObserverTest, RepairCompleteAfterRestart) {
     ASSERT(repairObserver->isIncomplete());
 
     std::string mod("Collection mod");
-    repairObserver->onModification(mod);
+    repairObserver->invalidatingModification(mod);
 
     auto opCtx = cc().makeOperationContext();
     Lock::GlobalWrite lock(opCtx.get());
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.cpp
index a3abf4fff9a..5be3590d6cd 100644
--- a/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.cpp
+++ b/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.cpp
@@ -713,7 +713,7 @@ void WiredTigerKVEngine::_openWiredTiger(const std::string& path, const std::str
     ret = wiredtiger_open(path.c_str(), wtEventHandler, configStr.c_str(), &_conn);
     if (!ret) {
         StorageRepairObserver::get(getGlobalServiceContext())
-            ->onModification("WiredTiger metadata salvaged");
+            ->invalidatingModification("WiredTiger metadata salvaged");
         return;
     }