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

(cherry picked from commit f4e387fa1b7e369ce067650bdda9c8676683b929)

7 files changed, 121 insertions, 36 deletions

diff --git a/src/mongo/db/repair_database_and_check_version.cpp b/src/mongo/db/repair_database_and_check_version.cpp
index dcd9b7cf3ba..3ff3ad36613 100644
--- a/src/mongo/db/repair_database_and_check_version.cpp
+++ b/src/mongo/db/repair_database_and_check_version.cpp
@@ -428,17 +428,25 @@ 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/storage_engine_impl.cpp b/src/mongo/db/storage/storage_engine_impl.cpp
index 9d0ee9792e6..8527910addf 100644
--- a/src/mongo/db/storage/storage_engine_impl.cpp
+++ b/src/mongo/db/storage/storage_engine_impl.cpp
@@ -92,8 +92,8 @@ void StorageEngineImpl::loadCatalog(OperationContext* opCtx) {
 
         if (status.code() == ErrorCodes::DataModifiedByRepair) {
             warning() << "Catalog data modified by repair: " << status.reason();
-            repairObserver->onModification(str::stream()
-                                           << "DurableCatalog repaired: " << status.reason());
+            repairObserver->invalidatingModification(str::stream() << "DurableCatalog repaired: "
+                                                                   << status.reason());
         } else {
             fassertNoTrace(50926, status);
         }
@@ -167,8 +167,8 @@ void StorageEngineImpl::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.
@@ -209,8 +209,9 @@ void StorageEngineImpl::loadCatalog(OperationContext* opCtx) {
 
                     if (_options.forRepair) {
                         StorageRepairObserver::get(getGlobalServiceContext())
-                            ->onModification(str::stream() << "Collection " << nss
-                                                           << " dropped: " << status.reason());
+                            ->invalidatingModification(str::stream()
+                                                       << "Collection " << nss
+                                                       << " dropped: " << status.reason());
                     }
                     wuow.commit();
                     continue;
@@ -298,8 +299,8 @@ Status StorageEngineImpl::_recoverOrphanedCollection(OperationContext* opCtx,
     }
     if (dataModified) {
         StorageRepairObserver::get(getGlobalServiceContext())
-            ->onModification(str::stream() << "Collection " << collectionName
-                                           << " recovered: " << status.reason());
+            ->invalidatingModification(str::stream() << "Collection " << collectionName
+                                                     << " recovered: " << status.reason());
     }
     wuow.commit();
     return Status::OK();
@@ -682,8 +683,8 @@ Status StorageEngineImpl::repairRecordStore(OperationContext* opCtx, const Names
     }
 
     if (dataModified) {
-        repairObserver->onModification(str::stream()
-                                       << "Collection " << nss << ": " << status.reason());
+        repairObserver->invalidatingModification(str::stream() << "Collection " << nss << ": "
+                                                               << status.reason());
     }
 
     // After repairing, re-initialize the collection with a valid RecordStore.
diff --git a/src/mongo/db/storage/storage_engine_test_fixture.h b/src/mongo/db/storage/storage_engine_test_fixture.h
index ffbb42ee70c..818cb4990fb 100644
--- a/src/mongo/db/storage/storage_engine_test_fixture.h
+++ b/src/mongo/db/storage/storage_engine_test_fixture.h
@@ -174,7 +174,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 3bd7c0de042..6d286c3c906 100644
--- a/src/mongo/db/storage/storage_repair_observer.cpp
+++ b/src/mongo/db/storage/storage_repair_observer.cpp
@@ -31,6 +31,7 @@
 
 #include "mongo/db/storage/storage_repair_observer.h"
 
+#include <algorithm>
 #include <cerrno>
 #include <cstring>
 
@@ -88,9 +89,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) {
@@ -98,7 +104,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();
@@ -106,6 +112,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 6f5a89c4751..a28cae64bb7 100644
--- a/src/mongo/db/storage/storage_repair_observer.h
+++ b/src/mongo/db/storage/storage_repair_observer.h
@@ -61,6 +61,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.
@@ -68,12 +94,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().
      */
@@ -94,20 +130,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 {
         /**
@@ -135,7 +167,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 e34717b76f5..e79386f69b2 100644
--- a/src/mongo/db/storage/storage_repair_observer_test.cpp
+++ b/src/mongo/db/storage/storage_repair_observer_test.cpp
@@ -106,10 +106,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();
             }
         }
     }
@@ -138,7 +138,7 @@ TEST_F(StorageRepairObserverTest, DataUnmodified) {
     ASSERT(!boost::filesystem::exists(repairFile));
 
     ASSERT(repairObserver->isDone());
-    ASSERT(!repairObserver->isDataModified());
+    ASSERT(!repairObserver->isDataInvalidated());
 
     assertReplConfigValid(opCtx.get(), true);
 }
@@ -156,7 +156,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());
@@ -167,12 +167,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();
 
@@ -186,7 +216,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());
@@ -196,7 +226,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()));
 }
@@ -238,7 +268,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 f635bdb9453..6f280b53c0b 100644
--- a/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.cpp
+++ b/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.cpp
@@ -792,7 +792,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;
     }