SERVER-50952 Ensure that cached collection entries with stale-marked shard will be eventually refreshed

8 files changed, 193 insertions, 58 deletions

diff --git a/src/mongo/s/catalog_cache.cpp b/src/mongo/s/catalog_cache.cpp
index e5ea1f67f6b..789dafdce39 100644
--- a/src/mongo/s/catalog_cache.cpp
+++ b/src/mongo/s/catalog_cache.cpp
@@ -264,16 +264,20 @@ void CatalogCache::invalidateShardOrEntireCollectionEntryForShardedCollection(
     _stats.countStaleConfigErrors.addAndFetch(1);
 
     auto collectionEntry = _collectionCache.peekLatestCached(nss);
-    if (collectionEntry && collectionEntry->optRt) {
-        collectionEntry->optRt->setShardStale(shardId);
-    }
 
-    if (wantedVersion) {
-        _collectionCache.advanceTimeInStore(
-            nss, ComparableChunkVersion::makeComparableChunkVersion(*wantedVersion));
-    } else {
-        _collectionCache.advanceTimeInStore(
-            nss, ComparableChunkVersion::makeComparableChunkVersionForForcedRefresh());
+    const auto newChunkVersion = wantedVersion
+        ? ComparableChunkVersion::makeComparableChunkVersion(*wantedVersion)
+        : ComparableChunkVersion::makeComparableChunkVersionForForcedRefresh();
+
+    const bool timeAdvanced = _collectionCache.advanceTimeInStore(nss, newChunkVersion);
+
+    if (timeAdvanced && collectionEntry && collectionEntry->optRt) {
+        // Shards marked stale will be reset on the next refresh.
+        // We can mark the shard stale only if the time advanced, otherwise no refresh would happen
+        // and the shard will remain marked stale.
+        // Even if a concurrent refresh is happening this is still the old collectionEntry,
+        // so it is safe to call setShardStale.
+        collectionEntry->optRt->setShardStale(shardId);
     }
 }
 
diff --git a/src/mongo/s/catalog_cache_loader_mock.cpp b/src/mongo/s/catalog_cache_loader_mock.cpp
index 757b595a781..4fb761e5d4c 100644
--- a/src/mongo/s/catalog_cache_loader_mock.cpp
+++ b/src/mongo/s/catalog_cache_loader_mock.cpp
@@ -40,6 +40,15 @@ namespace mongo {
 
 using CollectionAndChangedChunks = CatalogCacheLoader::CollectionAndChangedChunks;
 
+const Status CatalogCacheLoaderMock::kCollectionInternalErrorStatus = {
+    ErrorCodes::InternalError,
+    "Mocked catalog cache loader received unexpected collection request"};
+const Status CatalogCacheLoaderMock::kChunksInternalErrorStatus = {
+    ErrorCodes::InternalError, "Mocked catalog cache loader received unexpected chunks request"};
+const Status CatalogCacheLoaderMock::kDatabaseInternalErrorStatus = {
+    ErrorCodes::InternalError, "Mocked catalog cache loader received unexpected database request"};
+
+
 void CatalogCacheLoaderMock::initializeReplicaSetRole(bool isPrimary) {
     MONGO_UNREACHABLE;
 }
@@ -108,13 +117,25 @@ void CatalogCacheLoaderMock::setCollectionRefreshReturnValue(
     _swCollectionReturnValue = std::move(statusWithCollectionType);
 }
 
+void CatalogCacheLoaderMock::clearCollectionReturnValue() {
+    _swCollectionReturnValue = kCollectionInternalErrorStatus;
+}
+
 void CatalogCacheLoaderMock::setChunkRefreshReturnValue(
     StatusWith<std::vector<ChunkType>> statusWithChunks) {
     _swChunksReturnValue = std::move(statusWithChunks);
 }
 
+void CatalogCacheLoaderMock::clearChunksReturnValue() {
+    _swChunksReturnValue = kChunksInternalErrorStatus;
+}
+
 void CatalogCacheLoaderMock::setDatabaseRefreshReturnValue(StatusWith<DatabaseType> swDatabase) {
     _swDatabaseReturnValue = std::move(swDatabase);
 }
 
+void CatalogCacheLoaderMock::clearDatabaseReturnValue() {
+    _swDatabaseReturnValue = kDatabaseInternalErrorStatus;
+}
+
 }  // namespace mongo
diff --git a/src/mongo/s/catalog_cache_loader_mock.h b/src/mongo/s/catalog_cache_loader_mock.h
index 1bb13a46285..9aa92f2a893 100644
--- a/src/mongo/s/catalog_cache_loader_mock.h
+++ b/src/mongo/s/catalog_cache_loader_mock.h
@@ -66,30 +66,33 @@ public:
      * Sets the mocked collection entry result that getChunksSince will use to construct its return
      * value.
      */
+
     void setCollectionRefreshReturnValue(StatusWith<CollectionType> statusWithCollectionType);
+    void clearCollectionReturnValue();
 
     /**
      * Sets the mocked chunk results that getChunksSince will use to construct its return value.
      */
     void setChunkRefreshReturnValue(StatusWith<std::vector<ChunkType>> statusWithChunks);
+    void clearChunksReturnValue();
 
     /**
      * Sets the mocked database entry result that getDatabase will use to construct its return
      * value.
      */
     void setDatabaseRefreshReturnValue(StatusWith<DatabaseType> swDatabase);
+    void clearDatabaseReturnValue();
+
+    static const Status kCollectionInternalErrorStatus;
+    static const Status kChunksInternalErrorStatus;
+    static const Status kDatabaseInternalErrorStatus;
 
 private:
-    StatusWith<DatabaseType> _swDatabaseReturnValue{
-        Status(ErrorCodes::InternalError, "config loader database response is uninitialized")};
+    StatusWith<CollectionType> _swCollectionReturnValue{kCollectionInternalErrorStatus};
 
-    // These variables hold the mocked chunks and collection entry results used to construct the
-    // return value of getChunksSince above.
-    StatusWith<CollectionType> _swCollectionReturnValue{Status(
-        ErrorCodes::InternalError, "config loader mock collection response is uninitialized")};
+    StatusWith<std::vector<ChunkType>> _swChunksReturnValue{kChunksInternalErrorStatus};
 
-    StatusWith<std::vector<ChunkType>> _swChunksReturnValue{
-        Status(ErrorCodes::InternalError, "config loader mock chunks response is uninitialized")};
+    StatusWith<DatabaseType> _swDatabaseReturnValue{kDatabaseInternalErrorStatus};
 };
 
 }  // namespace mongo
diff --git a/src/mongo/s/catalog_cache_test.cpp b/src/mongo/s/catalog_cache_test.cpp
index 8fdb461aca3..4d5c3d1f144 100644
--- a/src/mongo/s/catalog_cache_test.cpp
+++ b/src/mongo/s/catalog_cache_test.cpp
@@ -50,7 +50,7 @@ protected:
         configTargeter()->setFindHostReturnValue(kConfigHostAndPort);
 
         // Setup catalogCache with mock loader
-        _catalogCacheLoader = std::make_unique<CatalogCacheLoaderMock>();
+        _catalogCacheLoader = std::make_shared<CatalogCacheLoaderMock>();
         _catalogCache = std::make_unique<CatalogCache>(getServiceContext(), *_catalogCacheLoader);
 
         // Populate the shardRegistry with the shards from kShards vector
@@ -62,41 +62,89 @@ protected:
         addRemoteShards(shardInfos);
     };
 
+    class ScopedCollectionProvider {
+    public:
+        ScopedCollectionProvider(std::shared_ptr<CatalogCacheLoaderMock> catalogCacheLoader,
+                                 const StatusWith<CollectionType>& swCollection)
+            : _catalogCacheLoader(catalogCacheLoader) {
+            _catalogCacheLoader->setCollectionRefreshReturnValue(swCollection);
+        }
+        ~ScopedCollectionProvider() {
+            _catalogCacheLoader->clearCollectionReturnValue();
+        }
+
+    private:
+        std::shared_ptr<CatalogCacheLoaderMock> _catalogCacheLoader;
+    };
+
+    ScopedCollectionProvider scopedCollectionProvider(
+        const StatusWith<CollectionType>& swCollection) {
+        return {_catalogCacheLoader, swCollection};
+    }
+
+    class ScopedChunksProvider {
+    public:
+        ScopedChunksProvider(std::shared_ptr<CatalogCacheLoaderMock> catalogCacheLoader,
+                             const StatusWith<std::vector<ChunkType>>& swChunks)
+            : _catalogCacheLoader(catalogCacheLoader) {
+            _catalogCacheLoader->setChunkRefreshReturnValue(swChunks);
+        }
+        ~ScopedChunksProvider() {
+            _catalogCacheLoader->clearChunksReturnValue();
+        }
+
+    private:
+        std::shared_ptr<CatalogCacheLoaderMock> _catalogCacheLoader;
+    };
+
+    ScopedChunksProvider scopedChunksProvider(const StatusWith<std::vector<ChunkType>>& swChunks) {
+        return {_catalogCacheLoader, swChunks};
+    }
+
+    class ScopedDatabaseProvider {
+    public:
+        ScopedDatabaseProvider(std::shared_ptr<CatalogCacheLoaderMock> catalogCacheLoader,
+                               const StatusWith<DatabaseType>& swDatabase)
+            : _catalogCacheLoader(catalogCacheLoader) {
+            _catalogCacheLoader->setDatabaseRefreshReturnValue(swDatabase);
+        }
+        ~ScopedDatabaseProvider() {
+            _catalogCacheLoader->clearDatabaseReturnValue();
+        }
+
+    private:
+        std::shared_ptr<CatalogCacheLoaderMock> _catalogCacheLoader;
+    };
+
+    ScopedDatabaseProvider scopedDatabaseProvider(const StatusWith<DatabaseType>& swDatabase) {
+        return {_catalogCacheLoader, swDatabase};
+    }
+
     void loadDatabases(const std::vector<DatabaseType>& databases) {
         for (const auto& db : databases) {
-            _catalogCacheLoader->setDatabaseRefreshReturnValue(db);
+            const auto scopedDbProvider = scopedDatabaseProvider(db);
             const auto swDatabase = _catalogCache->getDatabase(operationContext(), db.getName());
             ASSERT_OK(swDatabase.getStatus());
         }
-
-        // Reset the database return value to avoid false positive results
-        _catalogCacheLoader->setDatabaseRefreshReturnValue(kErrorStatus);
     }
 
     void loadCollection(const ChunkVersion& version) {
         const auto coll = makeCollectionType(version);
-        _catalogCacheLoader->setCollectionRefreshReturnValue(coll);
-        _catalogCacheLoader->setChunkRefreshReturnValue(makeChunks(version));
+        const auto scopedCollProv = scopedCollectionProvider(coll);
+        const auto scopedChunksProv = scopedChunksProvider(makeChunks(version));
 
         const auto swChunkManager =
             _catalogCache->getCollectionRoutingInfo(operationContext(), coll.getNs());
         ASSERT_OK(swChunkManager.getStatus());
-
-        // Reset the loader return values to avoid false positive results
-        _catalogCacheLoader->setCollectionRefreshReturnValue(kErrorStatus);
-        _catalogCacheLoader->setChunkRefreshReturnValue(kErrorStatus);
     }
 
     void loadUnshardedCollection(const NamespaceString& nss) {
-        _catalogCacheLoader->setCollectionRefreshReturnValue(
-            Status(ErrorCodes::NamespaceNotFound, "collection not found"));
+        const auto scopedCollProvider =
+            scopedCollectionProvider(Status(ErrorCodes::NamespaceNotFound, "collection not found"));
 
         const auto swChunkManager =
             _catalogCache->getCollectionRoutingInfo(operationContext(), nss);
         ASSERT_OK(swChunkManager.getStatus());
-
-        // Reset the loader return value to avoid false positive results
-        _catalogCacheLoader->setCollectionRefreshReturnValue(kErrorStatus);
     }
 
     std::vector<ChunkType> makeChunks(ChunkVersion version) {
@@ -124,10 +172,8 @@ protected:
     const int kDummyPort{12345};
     const HostAndPort kConfigHostAndPort{"DummyConfig", kDummyPort};
     const std::vector<ShardId> kShards{{"0"}, {"1"}};
-    const Status kErrorStatus{ErrorCodes::InternalError,
-                              "Received an unexpected CatalogCacheLoader request"};
 
-    std::unique_ptr<CatalogCacheLoaderMock> _catalogCacheLoader;
+    std::shared_ptr<CatalogCacheLoaderMock> _catalogCacheLoader;
     std::unique_ptr<CatalogCache> _catalogCache;
 };
 
@@ -177,6 +223,44 @@ TEST_F(CatalogCacheTest, InvalidateSingleDbOnShardRemoval) {
     ASSERT_EQ(cachedDb.primaryId(), kShards[1]);
 }
 
+TEST_F(CatalogCacheTest, OnStaleShardVersionWithSameVersion) {
+    const auto dbVersion = DatabaseVersion(UUID::gen(), 1);
+    const auto cachedCollVersion = ChunkVersion(1, 0, OID::gen());
+
+    loadDatabases({DatabaseType(kNss.db().toString(), kShards[0], true, dbVersion)});
+    loadCollection(cachedCollVersion);
+    _catalogCache->invalidateShardOrEntireCollectionEntryForShardedCollection(
+        kNss, cachedCollVersion, kShards[0]);
+    ASSERT_OK(_catalogCache->getCollectionRoutingInfo(operationContext(), kNss).getStatus());
+}
+
+TEST_F(CatalogCacheTest, OnStaleShardVersionWithNoVersion) {
+    const auto dbVersion = DatabaseVersion(UUID::gen(), 1);
+    const auto cachedCollVersion = ChunkVersion(1, 0, OID::gen());
+
+    loadDatabases({DatabaseType(kNss.db().toString(), kShards[0], true, dbVersion)});
+    loadCollection(cachedCollVersion);
+    _catalogCache->invalidateShardOrEntireCollectionEntryForShardedCollection(
+        kNss, boost::none, kShards[0]);
+    const auto status =
+        _catalogCache->getCollectionRoutingInfo(operationContext(), kNss).getStatus();
+    ASSERT(status == ErrorCodes::InternalError);
+}
+
+TEST_F(CatalogCacheTest, OnStaleShardVersionWithGraterVersion) {
+    const auto dbVersion = DatabaseVersion(UUID::gen(), 1);
+    const auto cachedCollVersion = ChunkVersion(1, 0, OID::gen());
+    const auto wantedCollVersion = ChunkVersion(2, 0, cachedCollVersion.epoch());
+
+    loadDatabases({DatabaseType(kNss.db().toString(), kShards[0], true, dbVersion)});
+    loadCollection(cachedCollVersion);
+    _catalogCache->invalidateShardOrEntireCollectionEntryForShardedCollection(
+        kNss, wantedCollVersion, kShards[0]);
+    const auto status =
+        _catalogCache->getCollectionRoutingInfo(operationContext(), kNss).getStatus();
+    ASSERT(status == ErrorCodes::InternalError);
+}
+
 TEST_F(CatalogCacheTest, CheckEpochNoDatabase) {
     const auto collVersion = ChunkVersion(1, 0, OID::gen());
     ASSERT_THROWS_WITH_CHECK(_catalogCache->checkEpochOrThrow(kNss, collVersion, kShards[0]),
@@ -192,7 +276,7 @@ TEST_F(CatalogCacheTest, CheckEpochNoDatabase) {
 }
 
 TEST_F(CatalogCacheTest, CheckEpochNoCollection) {
-    const auto dbVersion = DatabaseVersion();
+    const auto dbVersion = DatabaseVersion(UUID::gen(), 1);
     const auto collVersion = ChunkVersion(1, 0, OID::gen());
 
     loadDatabases({DatabaseType(kNss.db().toString(), kShards[0], true, dbVersion)});
@@ -209,7 +293,7 @@ TEST_F(CatalogCacheTest, CheckEpochNoCollection) {
 }
 
 TEST_F(CatalogCacheTest, CheckEpochUnshardedCollection) {
-    const auto dbVersion = DatabaseVersion();
+    const auto dbVersion = DatabaseVersion(UUID::gen(), 1);
     const auto collVersion = ChunkVersion(1, 0, OID::gen());
 
     loadDatabases({DatabaseType(kNss.db().toString(), kShards[0], true, dbVersion)});
@@ -227,7 +311,7 @@ TEST_F(CatalogCacheTest, CheckEpochUnshardedCollection) {
 }
 
 TEST_F(CatalogCacheTest, CheckEpochWithMismatch) {
-    const auto dbVersion = DatabaseVersion();
+    const auto dbVersion = DatabaseVersion(UUID::gen(), 1);
     const auto wantedCollVersion = ChunkVersion(1, 0, OID::gen());
     const auto receivedCollVersion = ChunkVersion(1, 0, OID::gen());
 
@@ -249,7 +333,7 @@ TEST_F(CatalogCacheTest, CheckEpochWithMismatch) {
 }
 
 TEST_F(CatalogCacheTest, CheckEpochWithMatch) {
-    const auto dbVersion = DatabaseVersion();
+    const auto dbVersion = DatabaseVersion(UUID::gen(), 1);
     const auto collVersion = ChunkVersion(1, 0, OID::gen());
 
     loadDatabases({DatabaseType(kNss.db().toString(), kShards[0], true, dbVersion)});
diff --git a/src/mongo/util/invalidating_lru_cache.h b/src/mongo/util/invalidating_lru_cache.h
index 2852313ef22..05ae4871acf 100644
--- a/src/mongo/util/invalidating_lru_cache.h
+++ b/src/mongo/util/invalidating_lru_cache.h
@@ -409,8 +409,11 @@ public:
      * is currently cached, but with isValid = false. Calls to 'get' with a causal consistency of
      * 'kLatestKnown' will return no value. It is up to the caller to this function to subsequently
      * either 'insertOrAssign' a new value for the 'key', or to call 'invalidate'.
+     *
+     * Returns true if the passed 'newTimeInStore' is grater than the time of the currently cached
+     * value or if no value is cached for 'key'.
      */
-    void advanceTimeInStore(const Key& key, const Time& newTimeInStore) {
+    bool advanceTimeInStore(const Key& key, const Time& newTimeInStore) {
         stdx::lock_guard<Latch> lg(_mutex);
         std::shared_ptr<StoredValue> storedValue;
         if (auto it = _cache.find(key); it != _cache.end()) {
@@ -420,13 +423,17 @@ public:
             storedValue = it->second.lock();
         }
 
-        if (!storedValue)
-            return;
+        if (!storedValue) {
+            return true;
+        }
 
-        if (newTimeInStore > storedValue->timeInStore) {
+        if (storedValue->timeInStore < newTimeInStore) {
             storedValue->timeInStore = newTimeInStore;
             storedValue->isValid.store(false);
+            return true;
         }
+
+        return false;
     }
 
     /**
diff --git a/src/mongo/util/invalidating_lru_cache_test.cpp b/src/mongo/util/invalidating_lru_cache_test.cpp
index 8476dfc5c9e..4c304deb4ef 100644
--- a/src/mongo/util/invalidating_lru_cache_test.cpp
+++ b/src/mongo/util/invalidating_lru_cache_test.cpp
@@ -90,7 +90,7 @@ TEST(InvalidatingLRUCacheTest, CausalConsistency) {
     ASSERT_EQ("Value @ TS 100", cache.get(2, CacheCausalConsistency::kLatestKnown)->value);
 
     auto value = cache.get(2, CacheCausalConsistency::kLatestCached);
-    cache.advanceTimeInStore(2, Timestamp(200));
+    ASSERT(cache.advanceTimeInStore(2, Timestamp(200)));
     ASSERT_EQ("Value @ TS 100", value->value);
     ASSERT(!value.isValid());
     ASSERT_EQ("Value @ TS 100", cache.get(2, CacheCausalConsistency::kLatestCached)->value);
@@ -279,7 +279,7 @@ TEST(InvalidatingLRUCacheTest, CausalConsistencyPreservedForEvictedCheckedOutKey
     ASSERT_EQ("Key 1 - Value @ TS 10", cache.get(1, CacheCausalConsistency::kLatestCached)->value);
     ASSERT_EQ("Key 1 - Value @ TS 10", cache.get(1, CacheCausalConsistency::kLatestKnown)->value);
 
-    cache.advanceTimeInStore(1, Timestamp(11));
+    ASSERT(cache.advanceTimeInStore(1, Timestamp(11)));
     auto [cachedValueAtTS11, timeInStoreAtTS11] = cache.getCachedValueAndTimeInStore(1);
     ASSERT_EQ(Timestamp(11), timeInStoreAtTS11);
     ASSERT(!key1ValueAtTS10.isValid());
@@ -297,7 +297,7 @@ TEST(InvalidatingLRUCacheTest, InvalidateAfterAdvanceTime) {
     TestValueCacheCausallyConsistent cache(1);
 
     cache.insertOrAssign(20, TestValue("Value @ TS 200"), Timestamp(200));
-    cache.advanceTimeInStore(20, Timestamp(250));
+    ASSERT(cache.advanceTimeInStore(20, Timestamp(250)));
     ASSERT_EQ("Value @ TS 200", cache.get(20, CacheCausalConsistency::kLatestCached)->value);
     ASSERT(!cache.get(20, CacheCausalConsistency::kLatestKnown));
 
@@ -310,7 +310,7 @@ TEST(InvalidatingLRUCacheTest, InsertEntryAtTimeLessThanAdvanceTime) {
     TestValueCacheCausallyConsistent cache(1);
 
     cache.insertOrAssign(20, TestValue("Value @ TS 200"), Timestamp(200));
-    cache.advanceTimeInStore(20, Timestamp(300));
+    ASSERT(cache.advanceTimeInStore(20, Timestamp(300)));
     ASSERT_EQ("Value @ TS 200", cache.get(20, CacheCausalConsistency::kLatestCached)->value);
     ASSERT(!cache.get(20, CacheCausalConsistency::kLatestKnown));
 
@@ -432,7 +432,7 @@ TEST(InvalidatingLRUCacheTest, CacheSizeZeroInvalidateAllEntries) {
 TEST(InvalidatingLRUCacheTest, CacheSizeZeroCausalConsistency) {
     TestValueCacheCausallyConsistent cache(0);
 
-    cache.advanceTimeInStore(100, Timestamp(30));
+    ASSERT(cache.advanceTimeInStore(100, Timestamp(30)));
     cache.insertOrAssign(100, TestValue("Value @ TS 30"), Timestamp(30));
     auto [cachedValueAtTS30, timeInStoreAtTS30] = cache.getCachedValueAndTimeInStore(100);
     ASSERT_EQ(Timestamp(), timeInStoreAtTS30);
@@ -442,7 +442,7 @@ TEST(InvalidatingLRUCacheTest, CacheSizeZeroCausalConsistency) {
     ASSERT_EQ("Value @ TS 30", cache.get(100, CacheCausalConsistency::kLatestCached)->value);
     ASSERT_EQ("Value @ TS 30", cache.get(100, CacheCausalConsistency::kLatestKnown)->value);
 
-    cache.advanceTimeInStore(100, Timestamp(35));
+    ASSERT(cache.advanceTimeInStore(100, Timestamp(35)));
     auto [cachedValueAtTS35, timeInStoreAtTS35] = cache.getCachedValueAndTimeInStore(100);
     ASSERT_EQ(Timestamp(35), timeInStoreAtTS35);
     ASSERT_EQ("Value @ TS 30", cachedValueAtTS35->value);
@@ -454,6 +454,19 @@ TEST(InvalidatingLRUCacheTest, CacheSizeZeroCausalConsistency) {
     ASSERT_EQ("Value @ TS 40", cache.get(100, CacheCausalConsistency::kLatestKnown)->value);
 }
 
+TEST(InvalidatingLRUCacheTest, AdvanceTimeNoEntry) {
+    TestValueCacheCausallyConsistent cache(1);
+    // If there is no cached entry advanceTime will always return true
+    ASSERT(cache.advanceTimeInStore(100, Timestamp(30)));
+    ASSERT(cache.advanceTimeInStore(100, Timestamp(30)));
+}
+
+TEST(InvalidatingLRUCacheTest, AdvanceTimeSameTime) {
+    TestValueCacheCausallyConsistent cache(1);
+    cache.insertOrAssignAndGet(100, TestValue("Value @ TS 30"), Timestamp(30));
+    ASSERT(!cache.advanceTimeInStore(100, Timestamp(30)));
+}
+
 template <class TCache, typename TestFunc>
 void parallelTest(size_t cacheSize, TestFunc doTest) {
     constexpr auto kNumIterations = 100'000;
@@ -529,7 +542,7 @@ TEST(InvalidatingLRUCacheParallelTest, AdvanceTime) {
         auto latestCached = cache.get(key, CacheCausalConsistency::kLatestCached);
         auto latestKnown = cache.get(key, CacheCausalConsistency::kLatestKnown);
 
-        cache.advanceTimeInStore(key, Timestamp(counter.fetchAndAdd(1)));
+        ASSERT(cache.advanceTimeInStore(key, Timestamp(counter.fetchAndAdd(1))));
     });
 }
 
diff --git a/src/mongo/util/read_through_cache.h b/src/mongo/util/read_through_cache.h
index 32efbc576ae..d9aaf35b440 100644
--- a/src/mongo/util/read_through_cache.h
+++ b/src/mongo/util/read_through_cache.h
@@ -359,12 +359,15 @@ public:
      * With respect to causal consistency, the 'LookupFn' used for this cache must provide the
      * guarantee that if 'advanceTimeInStore' is called with a 'newTime', a subsequent call to
      * 'LookupFn' for 'key' must return at least 'newTime' or later.
+     *
+     * Returns true if the passed 'newTimeInStore' is grater than the time of the currently cached
+     * value or if no value is cached for 'key'.
      */
-    void advanceTimeInStore(const Key& key, const Time& newTime) {
+    bool advanceTimeInStore(const Key& key, const Time& newTime) {
         stdx::lock_guard lg(_mutex);
         if (auto it = _inProgressLookups.find(key); it != _inProgressLookups.end())
             it->second->advanceTimeInStore(lg, newTime);
-        _cache.advanceTimeInStore(key, newTime);
+        return _cache.advanceTimeInStore(key, newTime);
     }
 
     /**
diff --git a/src/mongo/util/read_through_cache_test.cpp b/src/mongo/util/read_through_cache_test.cpp
index 70d452790e7..e95535edb8e 100644
--- a/src/mongo/util/read_through_cache_test.cpp
+++ b/src/mongo/util/read_through_cache_test.cpp
@@ -356,7 +356,7 @@ TEST_F(ReadThroughCacheTest, CausalConsistency) {
     ASSERT_EQ(10, cache.acquire(_opCtx, "TestKey", CacheCausalConsistency::kLatestKnown)->counter);
 
     nextToReturn.emplace(CachedValue(20), Timestamp(20));
-    cache.advanceTimeInStore("TestKey", Timestamp(20));
+    ASSERT(cache.advanceTimeInStore("TestKey", Timestamp(20)));
     ASSERT_EQ(10, cache.acquire(_opCtx, "TestKey", CacheCausalConsistency::kLatestCached)->counter);
     ASSERT(!cache.acquire(_opCtx, "TestKey", CacheCausalConsistency::kLatestCached).isValid());
     ASSERT_EQ(20, cache.acquire(_opCtx, "TestKey", CacheCausalConsistency::kLatestKnown)->counter);
@@ -607,7 +607,7 @@ TEST_F(ReadThroughCacheAsyncTest, AdvanceTimeDuringLookupOfUnCachedKey) {
     auto futureAtTS100 = cache.acquireAsync("TestKey", CacheCausalConsistency::kLatestKnown);
     ASSERT(!futureAtTS100.isReady());
 
-    cache.advanceTimeInStore("TestKey", Timestamp(200));
+    ASSERT(cache.advanceTimeInStore("TestKey", Timestamp(200)));
     auto futureAtTS200 = cache.acquireAsync("TestKey", CacheCausalConsistency::kLatestKnown);
     ASSERT(!futureAtTS200.isReady());
 
@@ -640,7 +640,7 @@ TEST_F(ReadThroughCacheAsyncTest, KeyDeletedAfterAdvanceTimeInStore) {
     ASSERT_EQ(100, futureAtTS100.get()->counter);
     ASSERT(futureAtTS100.get().isValid());
 
-    cache.advanceTimeInStore("TestKey", Timestamp(200));
+    ASSERT(cache.advanceTimeInStore("TestKey", Timestamp(200)));
     auto futureAtTS200 = cache.acquireAsync("TestKey", CacheCausalConsistency::kLatestKnown);
 
     nextToReturn.emplace(boost::none, Timestamp(200));
@@ -672,12 +672,12 @@ TEST_F(ReadThroughCacheAsyncTest, AcquireAsyncAndAdvanceTimeInterleave) {
     ASSERT_EQ(100, futureAtTS100.get()->counter);
     ASSERT(futureAtTS100.get().isValid());
 
-    cache.advanceTimeInStore("TestKey", Timestamp(150));
+    ASSERT(cache.advanceTimeInStore("TestKey", Timestamp(150)));
     auto futureAtTS150 = cache.acquireAsync("TestKey", CacheCausalConsistency::kLatestKnown);
     ASSERT(!futureAtTS100.get().isValid());
     ASSERT(!futureAtTS150.isReady());
 
-    cache.advanceTimeInStore("TestKey", Timestamp(250));
+    ASSERT(cache.advanceTimeInStore("TestKey", Timestamp(250)));
     auto futureAtTS250 = cache.acquireAsync("TestKey", CacheCausalConsistency::kLatestKnown);
     ASSERT(!futureAtTS100.get().isValid());
     ASSERT(!futureAtTS150.isReady());