SERVER-58271 Stop forcing collection version refreshes in chunk manager targeter

(cherry picked from commit 8f42e5e827aa2a39ab7fe8345b124d5c115f840e)

6 files changed, 61 insertions, 342 deletions

diff --git a/jstests/sharding/batch_write_command_sharded.js b/jstests/sharding/batch_write_command_sharded.js
index 61cb0ac900c..2cdbf151f96 100644
--- a/jstests/sharding/batch_write_command_sharded.js
+++ b/jstests/sharding/batch_write_command_sharded.js
@@ -13,12 +13,7 @@ TestData.skipCheckOrphans = true;
 (function() {
 "use strict";
 
-var st = new ShardingTest({shards: 2, mongos: 1});
-
-var mongos = st.s0;
-var admin = mongos.getDB("admin");
-var config = mongos.getDB("config");
-var configConnStr = st._configDB;
+var st = new ShardingTest({shards: 2});
 
 jsTest.log("Starting sharding batch write tests...");
 
@@ -31,10 +26,10 @@ var result;
 //
 // Mongos _id autogeneration tests for sharded collections
 
-var coll = mongos.getCollection("foo.bar");
-assert.commandWorked(admin.runCommand({enableSharding: coll.getDB().toString()}));
-st.ensurePrimaryShard(coll.getDB().getName(), st.shard1.shardName);
-assert.commandWorked(admin.runCommand({shardCollection: coll.toString(), key: {_id: 1}}));
+var coll = st.s.getCollection("foo.bar");
+assert.commandWorked(st.s.adminCommand(
+    {enableSharding: coll.getDB().toString(), primaryShard: st.shard1.shardName}));
+assert.commandWorked(st.s.adminCommand({shardCollection: coll.toString(), key: {_id: 1}}));
 
 //
 // Basic insert no _id
@@ -75,7 +70,7 @@ var documents = [];
 for (var i = 0; i < 1000; i++)
     documents.push({a: i, data: data});
 
-assert.commandWorked(coll.getMongo().getDB("admin").runCommand({setParameter: 1, logLevel: 4}));
+assert.commandWorked(coll.getMongo().adminCommand({setParameter: 1, logLevel: 4}));
 coll.remove({});
 request = {
     insert: coll.getName(),
@@ -89,7 +84,7 @@ assert.eq(1000, coll.count());
 //
 //
 // Config server upserts (against admin db, for example) require _id test
-var adminColl = admin.getCollection(coll.getName());
+var adminColl = st.s.getDB('admin')[coll.getName()];
 
 //
 // Without _id
@@ -116,73 +111,8 @@ assert.eq(1, adminColl.count());
 
 //
 //
-// Stale config progress tests
-// Set up a new collection across two shards, then revert the chunks to an earlier state to put
-// mongos and mongod permanently out of sync.
-
-// START SETUP
-var brokenColl = mongos.getCollection("broken.coll");
-assert.commandWorked(admin.runCommand({enableSharding: brokenColl.getDB().toString()}));
-st.ensurePrimaryShard(brokenColl.getDB().toString(), st.shard0.shardName);
-assert.commandWorked(admin.runCommand({shardCollection: brokenColl.toString(), key: {_id: 1}}));
-assert.commandWorked(admin.runCommand({split: brokenColl.toString(), middle: {_id: 0}}));
-
-var oldChunks = config.chunks.find().toArray();
-
-// Start a new mongos and bring it up-to-date with the chunks so far
-
-var staleMongos = MongoRunner.runMongos({configdb: configConnStr});
-brokenColl = staleMongos.getCollection(brokenColl.toString());
-assert.commandWorked(brokenColl.insert({hello: "world"}));
-
-// Modify the chunks to make shards at a higher version
-
-assert.commandWorked(
-    admin.runCommand({moveChunk: brokenColl.toString(), find: {_id: 0}, to: st.shard1.shardName}));
-
-// Rewrite the old chunks back to the config server
-
-assert.commandWorked(config.chunks.remove({}));
-for (var i = 0; i < oldChunks.length; i++) {
-    assert.commandWorked(config.chunks.insert(oldChunks[i]));
-}
-
-// Ensure that the inserts have propagated to all secondary nodes
-st.configRS.awaitReplication();
-
-// Stale mongos can no longer bring itself up-to-date!
-// END SETUP
-
-//
-// Config server insert, repeatedly stale
-printjson(request = {
-    insert: brokenColl.getName(),
-    documents: [{_id: -1}]
-});
-printjson(result = brokenColl.runCommand(request));
-assert(result.ok);
-assert.eq(0, result.n);
-assert.eq(1, result.writeErrors.length);
-assert.eq(0, result.writeErrors[0].index);
-assert.eq(result.writeErrors[0].code, 82);  // No Progress Made
-
-//
-// Config server insert to other shard, repeatedly stale
-printjson(request = {
-    insert: brokenColl.getName(),
-    documents: [{_id: 1}]
-});
-printjson(result = brokenColl.runCommand(request));
-assert(result.ok);
-assert.eq(0, result.n);
-assert.eq(1, result.writeErrors.length);
-assert.eq(0, result.writeErrors[0].index);
-assert.eq(result.writeErrors[0].code, 82);  // No Progress Made
-
-//
-//
 // Tests against config server
-var configColl = config.getCollection("batch_write_protocol_sharded");
+var configColl = st.s.getCollection('config.batch_write_protocol_sharded');
 
 //
 // Basic config server insert
@@ -234,8 +164,8 @@ assert.eq(0, st.config0.getCollection(configColl + "").count());
 assert.eq(0, st.config1.getCollection(configColl + "").count());
 assert.eq(0, st.config2.getCollection(configColl + "").count());
 
-MongoRunner.stopMongod(st.config1);
-MongoRunner.stopMongod(st.config2);
+st.stopConfigServer(1);
+st.stopConfigServer(2);
 st.configRS.awaitNoPrimary();
 
 // Config server insert with no config PRIMARY
@@ -268,7 +198,5 @@ printjson(result = configColl.runCommand(request));
 assert.commandFailedWithCode(result, ErrorCodes.FailedToSatisfyReadPreference);
 
 jsTest.log("DONE!");
-
-MongoRunner.stopMongos(staleMongos);
 st.stop();
 }());
diff --git a/src/mongo/s/chunk_manager_targeter.cpp b/src/mongo/s/chunk_manager_targeter.cpp
index ddf84bec57b..aa140b83d4e 100644
--- a/src/mongo/s/chunk_manager_targeter.cpp
+++ b/src/mongo/s/chunk_manager_targeter.cpp
@@ -213,94 +213,10 @@ bool isExactIdQuery(OperationContext* opCtx,
     return cq.isOK() && isExactIdQuery(opCtx, *cq.getValue(), cm);
 }
 
-//
-// Utilities to compare shard and db versions
-//
-
-/**
- * Returns the relationship of two shard versions. Shard versions of a collection that has not
- * been dropped and recreated or had its shard key refined and where there is at least one chunk on
- * a shard are comparable, otherwise the result is ambiguous.
- */
-CompareResult compareShardVersions(const ChunkVersion& shardVersionA,
-                                   const ChunkVersion& shardVersionB) {
-    // Collection may have been dropped or had its shard key refined.
-    if (shardVersionA.epoch() != shardVersionB.epoch()) {
-        return CompareResult_Unknown;
-    }
-
-    // Zero shard versions are only comparable to themselves
-    if (!shardVersionA.isSet() || !shardVersionB.isSet()) {
-        // If both are zero...
-        if (!shardVersionA.isSet() && !shardVersionB.isSet()) {
-            return CompareResult_GTE;
-        }
-
-        return CompareResult_Unknown;
-    }
-
-    if (shardVersionA.isOlderThan(shardVersionB))
-        return CompareResult_LT;
-    else
-        return CompareResult_GTE;
-}
-
-/**
- * Returns the relationship between two maps of shard versions. As above, these maps are often
- * comparable when the collection has not been dropped and there is at least one chunk on the
- * shards. If any versions in the maps are not comparable, the result is _Unknown.
- *
- * If any versions in the first map (cached) are _LT the versions in the second map (remote),
- * the first (cached) versions are _LT the second (remote) versions.
- *
- * Note that the signature here is weird since our cached map of chunk versions is stored in a
- * ChunkManager or is implicit in the primary shard of the collection.
- */
-CompareResult compareAllShardVersions(const ChunkManager& cm,
-                                      const StaleShardVersionMap& remoteShardVersions) {
-    CompareResult finalResult = CompareResult_GTE;
-
-    for (const auto& shardVersionEntry : remoteShardVersions) {
-        const ShardId& shardId = shardVersionEntry.first;
-        const ChunkVersion& remoteShardVersion = shardVersionEntry.second;
-
-        ChunkVersion cachedShardVersion;
-        try {
-            cachedShardVersion =
-                cm.isSharded() ? cm.getVersion(shardId) : ChunkVersion::UNSHARDED();
-        } catch (const DBException& ex) {
-            LOGV2_WARNING(22915,
-                          "could not lookup shard {shardId} in local cache, shard metadata may "
-                          "have changed or be unavailable: {error}",
-                          "Could not lookup shard in local cache",
-                          "shardId"_attr = shardId,
-                          "error"_attr = ex);
-
-            return CompareResult_Unknown;
-        }
-
-        // Compare the remote and cached versions
-        CompareResult result = compareShardVersions(cachedShardVersion, remoteShardVersion);
-
-        if (result == CompareResult_Unknown)
-            return result;
-
-        if (result == CompareResult_LT)
-            finalResult = CompareResult_LT;
-
-        // Note that we keep going after _LT b/c there could be more _Unknowns.
-    }
-
-    return finalResult;
-}
-
 /**
  * Whether or not the manager/primary pair is different from the other manager/primary pair.
  */
-bool isMetadataDifferent(const ChunkManager& managerA,
-                         const DatabaseVersion dbVersionA,
-                         const ChunkManager& managerB,
-                         const DatabaseVersion dbVersionB) {
+bool isMetadataDifferent(const ChunkManager& managerA, const ChunkManager& managerB) {
     if ((managerA.isSharded() && !managerB.isSharded()) ||
         (!managerA.isSharded() && managerB.isSharded()))
         return true;
@@ -309,7 +225,7 @@ bool isMetadataDifferent(const ChunkManager& managerA,
         return managerA.getVersion() != managerB.getVersion();
     }
 
-    return dbVersionA != dbVersionB;
+    return managerA.dbVersion() != managerB.dbVersion();
 }
 
 }  // namespace
@@ -317,7 +233,7 @@ bool isMetadataDifferent(const ChunkManager& managerA,
 ChunkManagerTargeter::ChunkManagerTargeter(OperationContext* opCtx,
                                            const NamespaceString& nss,
                                            boost::optional<OID> targetEpoch)
-    : _nss(nss), _needsTargetingRefresh(false), _targetEpoch(std::move(targetEpoch)) {
+    : _nss(nss), _targetEpoch(std::move(targetEpoch)) {
     _init(opCtx);
 }
 
@@ -575,157 +491,58 @@ std::vector<ShardEndpoint> ChunkManagerTargeter::targetAllShards(OperationContex
 }
 
 void ChunkManagerTargeter::noteCouldNotTarget() {
-    dassert(_remoteShardVersions.empty());
-    dassert(!_staleDbVersion);
-    _needsTargetingRefresh = true;
+    dassert(!_lastError || _lastError.get() == LastErrorType::kCouldNotTarget);
+    _lastError = LastErrorType::kCouldNotTarget;
 }
 
-void ChunkManagerTargeter::noteStaleShardResponse(const ShardEndpoint& endpoint,
+void ChunkManagerTargeter::noteStaleShardResponse(OperationContext* opCtx,
+                                                  const ShardEndpoint& endpoint,
                                                   const StaleConfigInfo& staleInfo) {
-    dassert(!_needsTargetingRefresh);
-    dassert(!_staleDbVersion);
-
-    ChunkVersion remoteShardVersion;
-    if (!staleInfo.getVersionWanted()) {
-        // If we don't have a vWanted sent, assume the version is higher than our current version.
-        remoteShardVersion =
-            _cm->isSharded() ? _cm->getVersion(endpoint.shardName) : ChunkVersion::UNSHARDED();
-        remoteShardVersion.incMajor();
-    } else {
-        remoteShardVersion = *staleInfo.getVersionWanted();
-    }
-
-    StaleShardVersionMap::iterator it = _remoteShardVersions.find(endpoint.shardName);
-    if (it == _remoteShardVersions.end()) {
-        _remoteShardVersions.insert(std::make_pair(endpoint.shardName, remoteShardVersion));
-    } else {
-        ChunkVersion& previouslyNotedVersion = it->second;
-        if (previouslyNotedVersion.epoch() == remoteShardVersion.epoch()) {
-            if (previouslyNotedVersion.isOlderThan(remoteShardVersion)) {
-                previouslyNotedVersion = remoteShardVersion;
-            }
-        } else {
-            // Epoch changed midway while applying the batch so set the version to something
-            // unique
-            // and non-existent to force a reload when refreshIsNeeded is called.
-            previouslyNotedVersion = ChunkVersion::IGNORED();
-        }
-    }
+    dassert(!_lastError || _lastError.get() == LastErrorType::kStaleShardVersion);
+    Grid::get(opCtx)->catalogCache()->invalidateShardOrEntireCollectionEntryForShardedCollection(
+        _nss, staleInfo.getVersionWanted(), endpoint.shardName);
+    _lastError = LastErrorType::kStaleShardVersion;
 }
 
 void ChunkManagerTargeter::noteStaleDbResponse(OperationContext* opCtx,
                                                const ShardEndpoint& endpoint,
                                                const StaleDbRoutingVersion& staleInfo) {
-    dassert(!_needsTargetingRefresh);
-    dassert(_remoteShardVersions.empty());
+    dassert(!_lastError || _lastError.get() == LastErrorType::kStaleDbVersion);
     Grid::get(opCtx)->catalogCache()->onStaleDatabaseVersion(_nss.db(),
                                                              staleInfo.getVersionWanted());
-    _staleDbVersion = true;
+    _lastError = LastErrorType::kStaleDbVersion;
 }
 
-void ChunkManagerTargeter::refreshIfNeeded(OperationContext* opCtx, bool* wasChanged) {
-    bool dummy;
-    if (!wasChanged) {
-        wasChanged = &dummy;
+bool ChunkManagerTargeter::refreshIfNeeded(OperationContext* opCtx) {
+    // Did we have any stale config or targeting errors at all?
+    if (!_lastError) {
+        return false;
     }
 
-    *wasChanged = false;
+    // Make sure that even in case of exception we will clear the last error.
+    ON_BLOCK_EXIT([&] { _lastError = boost::none; });
 
     LOGV2_DEBUG(22912,
                 4,
                 "ChunkManagerTargeter checking if refresh is needed",
-                "needsTargetingRefresh"_attr = _needsTargetingRefresh,
-                "hasRemoteShardVersions"_attr = !_remoteShardVersions.empty(),
-                "staleDbVersion"_attr = _staleDbVersion);
-
-    //
-    // Did we have any stale config or targeting errors at all?
-    //
+                "couldNotTarget"_attr = _lastError.get() == LastErrorType::kCouldNotTarget,
+                "staleShardVersion"_attr = _lastError.get() == LastErrorType::kStaleShardVersion,
+                "staleDbVersion"_attr = _lastError.get() == LastErrorType::kStaleDbVersion);
 
-    if (!_needsTargetingRefresh && _remoteShardVersions.empty() && !_staleDbVersion) {
-        return;
-    }
-
-    // Make sure that by the end of the execution of this refresh all the state data is cleared.
-    ON_BLOCK_EXIT([&] {
-        _remoteShardVersions.clear();
-        _staleDbVersion = false;
-        _needsTargetingRefresh = false;
-    });
-
-    //
     // Get the latest metadata information from the cache if there were issues
-    //
-
     auto lastManager = *_cm;
-    auto lastDbVersion = _cm->dbVersion();
-
     _init(opCtx);
+    auto metadataChanged = isMetadataDifferent(lastManager, *_cm);
 
-    // We now have the latest metadata from the cache.
-
-    //
-    // See if and how we need to do a remote refresh.
-    // Either we couldn't target at all, or we have stale versions, but not both.
-    //
-
-    if (_needsTargetingRefresh) {
-        // If we couldn't target, we might need to refresh if we haven't remotely refreshed
-        // the metadata since we last got it from the cache.
-
-        bool alreadyRefreshed =
-            isMetadataDifferent(lastManager, lastDbVersion, *_cm, _cm->dbVersion());
-
-        // If didn't already refresh the targeting information, refresh it
-        if (!alreadyRefreshed) {
-            // To match previous behavior, we just need an incremental refresh here
-            _refreshShardVersionNow(opCtx);
-            return;
-        }
-
-        *wasChanged = isMetadataDifferent(lastManager, lastDbVersion, *_cm, _cm->dbVersion());
-    } else if (!_remoteShardVersions.empty()) {
-        // If we got stale shard versions from remote shards, we may need to refresh
-        // NOTE: Not sure yet if this can happen simultaneously with targeting issues
-
-        CompareResult result = compareAllShardVersions(*_cm, _remoteShardVersions);
-
-        LOGV2_DEBUG(22913,
-                    4,
-                    "ChunkManagerTargeter shard versions comparison result: {result}",
-                    "ChunkManagerTargeter shard versions comparison",
-                    "result"_attr = static_cast<int>(result));
-
-        if (result == CompareResult_Unknown || result == CompareResult_LT) {
-            // Our current shard versions aren't all comparable to the old versions, maybe drop
-            _refreshShardVersionNow(opCtx);
-            return;
-        }
-
-        *wasChanged = isMetadataDifferent(lastManager, lastDbVersion, *_cm, _cm->dbVersion());
-    } else if (_staleDbVersion) {
+    if (_lastError.get() == LastErrorType::kCouldNotTarget && !metadataChanged) {
+        // If we couldn't target and we dind't already update the metadata we must force a refresh
+        uassertStatusOK(
+            Grid::get(opCtx)->catalogCache()->getCollectionRoutingInfoWithRefresh(opCtx, _nss));
         _init(opCtx);
-        *wasChanged = isMetadataDifferent(lastManager, lastDbVersion, *_cm, _cm->dbVersion());
-    }
-}
-
-bool ChunkManagerTargeter::endpointIsConfigServer() const {
-    if (!_cm->isSharded()) {
-        return _cm->dbPrimary() == ShardId::kConfigServerId;
-    }
-
-    std::set<ShardId> shardIds;
-    _cm->getAllShardIds(&shardIds);
-
-    if (std::any_of(shardIds.begin(), shardIds.end(), [](const auto& shardId) {
-            return shardId == ShardId::kConfigServerId;
-        })) {
-        // There should be no namespaces that target both config servers and shards.
-        invariant(shardIds.size() == 1);
-        return true;
+        metadataChanged = isMetadataDifferent(lastManager, *_cm);
     }
 
-    return false;
+    return metadataChanged;
 }
 
 int ChunkManagerTargeter::getNShardsOwningChunks() const {
@@ -736,11 +553,4 @@ int ChunkManagerTargeter::getNShardsOwningChunks() const {
     return 0;
 }
 
-void ChunkManagerTargeter::_refreshShardVersionNow(OperationContext* opCtx) {
-    uassertStatusOK(
-        Grid::get(opCtx)->catalogCache()->getCollectionRoutingInfoWithRefresh(opCtx, _nss));
-
-    _init(opCtx);
-}
-
 }  // namespace mongo
diff --git a/src/mongo/s/chunk_manager_targeter.h b/src/mongo/s/chunk_manager_targeter.h
index 6ed7df9a637..882ddb569e4 100644
--- a/src/mongo/s/chunk_manager_targeter.h
+++ b/src/mongo/s/chunk_manager_targeter.h
@@ -59,7 +59,7 @@ using StaleShardVersionMap = std::map<ShardId, ChunkVersion>;
 class ChunkManagerTargeter : public NSTargeter {
 public:
     enum class UpdateType { kReplacement, kOpStyle, kUnknown };
-
+    enum class LastErrorType { kCouldNotTarget, kStaleShardVersion, kStaleDbVersion };
     /**
      * Initializes the targeter with the latest routing information for the namespace, which means
      * it may have to block and load information from the config server.
@@ -86,7 +86,8 @@ public:
 
     void noteCouldNotTarget() override;
 
-    void noteStaleShardResponse(const ShardEndpoint& endpoint,
+    void noteStaleShardResponse(OperationContext* opCtx,
+                                const ShardEndpoint& endpoint,
                                 const StaleConfigInfo& staleInfo) override;
 
     void noteStaleDbResponse(OperationContext* opCtx,
@@ -98,13 +99,11 @@ public:
      * information is stale WRT the noted stale responses or a remote refresh is needed due
      * to a targeting failure, will contact the config servers to reload the metadata.
      *
-     * Reports wasChanged = true if the metadata is different after this reload.
+     * Return true if the metadata was different after this reload.
      *
      * Also see NSTargeter::refreshIfNeeded().
      */
-    void refreshIfNeeded(OperationContext* opCtx, bool* wasChanged) override;
-
-    bool endpointIsConfigServer() const override;
+    bool refreshIfNeeded(OperationContext* opCtx) override;
 
     int getNShardsOwningChunks() const override;
 
@@ -112,11 +111,6 @@ private:
     void _init(OperationContext* opCtx);
 
     /**
-     * Performs an actual refresh from the config server.
-     */
-    void _refreshShardVersionNow(OperationContext* opCtx);
-
-    /**
      * Returns a vector of ShardEndpoints for a potentially multi-shard query.
      *
      * Returns !OK with message if query could not be targeted.
@@ -143,8 +137,8 @@ private:
     // Full namespace of the collection for this targeter
     const NamespaceString _nss;
 
-    // Stores whether we need to check the remote server on refresh
-    bool _needsTargetingRefresh;
+    // Stores last error occurred
+    boost::optional<LastErrorType> _lastError;
 
     // The latest loaded routing cache entry
     boost::optional<ChunkManager> _cm;
@@ -152,12 +146,6 @@ private:
     // Set to the epoch of the namespace we are targeting. If we ever refresh the catalog cache and
     // find a new epoch, we immediately throw a StaleEpoch exception.
     boost::optional<OID> _targetEpoch;
-
-    // Map of shard->remote shard version reported from stale errors
-    StaleShardVersionMap _remoteShardVersions;
-
-    // remote db version reported from stale errors
-    bool _staleDbVersion{false};
 };
 
 }  // namespace mongo
diff --git a/src/mongo/s/mock_ns_targeter.h b/src/mongo/s/mock_ns_targeter.h
index 6cf4f3796f2..8ef5687b569 100644
--- a/src/mongo/s/mock_ns_targeter.h
+++ b/src/mongo/s/mock_ns_targeter.h
@@ -101,7 +101,8 @@ public:
         // No-op
     }
 
-    void noteStaleShardResponse(const ShardEndpoint& endpoint,
+    void noteStaleShardResponse(OperationContext* opCtx,
+                                const ShardEndpoint& endpoint,
                                 const StaleConfigInfo& staleInfo) override {
         // No-op
     }
@@ -112,13 +113,7 @@ public:
         // No-op
     }
 
-    void refreshIfNeeded(OperationContext* opCtx, bool* wasChanged) override {
-        // No-op
-        if (wasChanged)
-            *wasChanged = false;
-    }
-
-    bool endpointIsConfigServer() const override {
+    bool refreshIfNeeded(OperationContext* opCtx) override {
         // No-op
         return false;
     }
diff --git a/src/mongo/s/ns_targeter.h b/src/mongo/s/ns_targeter.h
index c3d24bf7c65..9fd233b1fa6 100644
--- a/src/mongo/s/ns_targeter.h
+++ b/src/mongo/s/ns_targeter.h
@@ -109,7 +109,8 @@ public:
      *
      * If stale responses are is noted, we must not have noted that we cannot target.
      */
-    virtual void noteStaleShardResponse(const ShardEndpoint& endpoint,
+    virtual void noteStaleShardResponse(OperationContext* opCtx,
+                                        const ShardEndpoint& endpoint,
                                         const StaleConfigInfo& staleInfo) = 0;
 
     /**
@@ -130,19 +131,13 @@ public:
      *
      * After this function is called, the targeter should be in a state such that the noted
      * stale responses are not seen again and if a targeting failure occurred it reloaded -
-     * it should try to make progress.  If provided, wasChanged is set to true if the targeting
-     * information used here was changed.
+     * it should try to make progress.
+     *
+     * Returns if the targeting used here was changed.
      *
      * NOTE: This function may block for shared resources or network calls.
      */
-    virtual void refreshIfNeeded(OperationContext* opCtx, bool* wasChanged) = 0;
-
-    /**
-     * Returns whether this write targets the config server. Invariants if the write targets the
-     * config server AND there is more than one endpoint, since there should be no namespaces that
-     * target both config servers and shards.
-     */
-    virtual bool endpointIsConfigServer() const = 0;
+    virtual bool refreshIfNeeded(OperationContext* opCtx) = 0;
 
     /**
      * Returns the number of shards that own one or more chunks for the targeted collection.
diff --git a/src/mongo/s/write_ops/batch_write_exec.cpp b/src/mongo/s/write_ops/batch_write_exec.cpp
index 48fc1414ffd..7dec6243df5 100644
--- a/src/mongo/s/write_ops/batch_write_exec.cpp
+++ b/src/mongo/s/write_ops/batch_write_exec.cpp
@@ -68,7 +68,9 @@ WriteErrorDetail errorFromStatus(const Status& status) {
 }
 
 // Helper to note several stale shard errors from a response
-void noteStaleShardResponses(const std::vector<ShardError>& staleErrors, NSTargeter* targeter) {
+void noteStaleShardResponses(OperationContext* opCtx,
+                             const std::vector<ShardError>& staleErrors,
+                             NSTargeter* targeter) {
     for (const auto& error : staleErrors) {
         LOGV2_DEBUG(22902,
                     4,
@@ -77,6 +79,7 @@ void noteStaleShardResponses(const std::vector<ShardError>& staleErrors, NSTarge
                     "shardId"_attr = error.endpoint.shardName,
                     "errorInfo"_attr = error.error.getErrInfo());
         targeter->noteStaleShardResponse(
+            opCtx,
             error.endpoint,
             StaleConfigInfo::parseFromCommandError(
                 error.error.isErrInfoSet() ? error.error.getErrInfo() : BSONObj()));
@@ -339,7 +342,7 @@ void BatchWriteExec::executeBatch(OperationContext* opCtx,
 
                     if (!staleShardErrors.empty()) {
                         invariant(staleDbErrors.empty());
-                        noteStaleShardResponses(staleShardErrors, &targeter);
+                        noteStaleShardResponses(opCtx, staleShardErrors, &targeter);
                         ++stats->numStaleShardBatches;
                     }
 
@@ -423,7 +426,7 @@ void BatchWriteExec::executeBatch(OperationContext* opCtx,
                                 2,
                                 {logv2::LogComponent::kShardMigrationPerf},
                                 "Starting post-migration commit refresh on the router");
-            targeter.refreshIfNeeded(opCtx, &targeterChanged);
+            targeterChanged = targeter.refreshIfNeeded(opCtx);
             LOGV2_DEBUG_OPTIONS(4817407,
                                 2,
                                 {logv2::LogComponent::kShardMigrationPerf},