SERVER-38369 Only surface a 'request doesn't allow collection to be created implicitly' error if no shard has the collection

6 files changed, 634 insertions, 89 deletions

diff --git a/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml b/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml
index 2a2f8c6c8c6..239de1a687b 100644
--- a/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml
+++ b/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml
@@ -35,6 +35,7 @@ selector:
   - jstests/sharding/now_variable_replset.js
   - jstests/sharding/now_variable_sharding.js
   - jstests/sharding/transaction_ops_fail_against_capped_collection_on_shards.js
+  - jstests/sharding/index_and_collection_option_propagation.js
   # mongos in 4.0 doesn't like an aggregation explain without stages for optimized away pipelines,
   # so blacklisting the test until 4.2 becomes last-stable.
   - jstests/sharding/agg_explain_fmt.js
diff --git a/jstests/sharding/index_and_collection_option_propagation.js b/jstests/sharding/index_and_collection_option_propagation.js
index 7619a2f897b..ba775eb594f 100644
--- a/jstests/sharding/index_and_collection_option_propagation.js
+++ b/jstests/sharding/index_and_collection_option_propagation.js
@@ -116,7 +116,7 @@ TestData.skipCheckingUUIDsConsistentAcrossCluster = true;
 
     // Though some shards don't own data for the sharded collection, createIndex, reIndex,
     // dropIndex, and collMod (which are broadcast to all shards) report overall success (that is,
-    // NamespaceNotFound-type errors from shards are ignored, though they are included in the 'raw'
+    // NamespaceNotFound-type errors from shards are ignored, and they are not included in the 'raw'
     // shard responses).
 
     var res;
@@ -126,8 +126,9 @@ TestData.skipCheckingUUIDsConsistentAcrossCluster = true;
     assert.commandWorked(res);
     assert.eq(res.raw[st.shard0.host].ok, 1, tojson(res));
     assert.eq(res.raw[st.shard1.host].ok, 1, tojson(res));
-    assert.eq(
-        res.raw[st.shard2.host].code, ErrorCodes.CannotImplicitlyCreateCollection, tojson(res));
+    assert.eq(undefined,
+              res.raw[st.shard2.host],
+              tojson(res));  // CannotImplicitlyCreateCollection is ignored
     checkShardIndexes("idx2", [st.shard0, st.shard1], [st.shard2]);
 
     // dropIndex
@@ -135,7 +136,7 @@ TestData.skipCheckingUUIDsConsistentAcrossCluster = true;
     assert.commandWorked(res);
     assert.eq(res.raw[st.shard0.host].ok, 1, tojson(res));
     assert.eq(res.raw[st.shard1.host].ok, 1, tojson(res));
-    assert.eq(res.raw[st.shard2.host].code, ErrorCodes.NamespaceNotFound, tojson(res));
+    assert.eq(undefined, res.raw[st.shard2.host], tojson(res));  // NamespaceNotFound is ignored
     checkShardIndexes("idx1", [], [st.shard0, st.shard1, st.shard2]);
 
     // collMod
@@ -149,7 +150,7 @@ TestData.skipCheckingUUIDsConsistentAcrossCluster = true;
     assert.commandWorked(res);
     assert.eq(res.raw[st.shard0.host].ok, 1, tojson(res));
     assert.eq(res.raw[st.shard1.host].ok, 1, tojson(res));
-    assert.eq(res.raw[st.shard2.host].code, ErrorCodes.NamespaceNotFound, tojson(res));
+    assert.eq(undefined, res.raw[st.shard2.host], tojson(res));  // NamespaceNotFound is ignored
     checkShardCollOption("validator", validationOption2, [st.shard0, st.shard1], [st.shard2]);
 
     // Check that errors from shards are aggregated correctly.
@@ -199,7 +200,7 @@ TestData.skipCheckingUUIDsConsistentAcrossCluster = true;
     assert.eq(res.raw[st.shard0.host].ok, 0, tojson(res));  // shard was down
     assert.eq(
         res.raw[st.shard1.host].ok, 1, tojson(res));  // gets created on shard that owns chunks
-    assert.eq(res.raw[st.shard2.host].ok, 0, tojson(res));  // shard does not own chunks
+    assert.eq(undefined, res.raw[st.shard2.host], tojson(res));  // shard does not own chunks
     assert.eq(res.code, res.raw[st.shard0.host].code, tojson(res));
     assert.eq(res.codeName, res.raw[st.shard0.host].codeName, tojson(res));
     // We can expect to see 'FailedToSatisfyReadPreference' this time, because after the previous
diff --git a/src/mongo/s/SConscript b/src/mongo/s/SConscript
index 88db8654426..3566cbbf4b3 100644
--- a/src/mongo/s/SConscript
+++ b/src/mongo/s/SConscript
@@ -484,6 +484,19 @@ env.Library(
 )
 
 env.CppUnitTest(
+    target='cluster_commands_helpers_test',
+    source=[
+        'append_raw_responses_test.cpp',
+    ],
+    LIBDEPS=[
+        '$BUILD_DIR/mongo/db/auth/authmocks',
+        '$BUILD_DIR/mongo/s/catalog/sharding_catalog_client_mock',
+        'shard_server_test_fixture',
+        'sharding_router_api',
+    ]
+)
+
+env.CppUnitTest(
     target='balancer_configuration_test',
     source=[
         'balancer_configuration_test.cpp',
diff --git a/src/mongo/s/append_raw_responses_test.cpp b/src/mongo/s/append_raw_responses_test.cpp
new file mode 100644
index 00000000000..791949a8672
--- /dev/null
+++ b/src/mongo/s/append_raw_responses_test.cpp
@@ -0,0 +1,541 @@
+/**
+ *    Copyright (C) 2018-present MongoDB, Inc.
+ *
+ *    This program is free software: you can redistribute it and/or modify
+ *    it under the terms of the Server Side Public License, version 1,
+ *    as published by MongoDB, Inc.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    Server Side Public License for more details.
+ *
+ *    You should have received a copy of the Server Side Public License
+ *    along with this program. If not, see
+ *    <http://www.mongodb.com/licensing/server-side-public-license>.
+ *
+ *    As a special exception, the copyright holders give permission to link the
+ *    code of portions of this program with the OpenSSL library under certain
+ *    conditions as described in each individual source file and distribute
+ *    linked combinations including the program with the OpenSSL library. You
+ *    must comply with the Server Side Public License in all respects for
+ *    all of the code used other than as permitted herein. If you modify file(s)
+ *    with this exception, you may extend this exception to your version of the
+ *    file(s), but you are not obligated to do so. If you do not wish to do so,
+ *    delete this exception statement from your version. If you delete this
+ *    exception statement from all source files in the program, then also delete
+ *    it in the license file.
+ */
+#define MONGO_LOG_DEFAULT_COMPONENT ::mongo::logger::LogComponent::kSharding
+
+#include "mongo/unittest/unittest.h"
+
+#include "mongo/client/remote_command_targeter_mock.h"
+#include "mongo/db/commands.h"
+#include "mongo/rpc/get_status_from_command_result.h"
+#include "mongo/s/async_requests_sender.h"
+#include "mongo/s/catalog/sharding_catalog_client_mock.h"
+#include "mongo/s/catalog/type_shard.h"
+#include "mongo/s/cluster_commands_helpers.h"
+#include "mongo/s/shard_server_test_fixture.h"
+
+namespace mongo {
+namespace {
+
+// Have two each of ignorable and non-ignorable errors, so we can test shards returning different
+// ignorable and different non-ignorable errors.
+const ErrorCodes::Error kIgnorableError1{ErrorCodes::NamespaceNotFound};
+const ErrorCodes::Error kIgnorableError2{ErrorCodes::NamespaceNotFound};
+const ErrorCodes::Error kNonIgnorableError1{ErrorCodes::HostUnreachable};
+const ErrorCodes::Error kNonIgnorableError2{ErrorCodes::HostUnreachable};
+
+const Status kIgnorableError1Status{kIgnorableError1, "dummy"};
+const Status kIgnorableError2Status{kIgnorableError2, "dummy"};
+const Status kNonIgnorableError1Status{kNonIgnorableError1, "dummy"};
+const Status kNonIgnorableError2Status{kNonIgnorableError2, "dummy"};
+
+const Status kWriteConcernError1Status{ErrorCodes::WriteConcernFailed, "dummy"};
+const Status kWriteConcernError2Status{ErrorCodes::UnsatisfiableWriteConcern, "dummy"};
+
+executor::RemoteCommandResponse kOkResponse{BSON("ok" << 1), Milliseconds(0)};
+
+executor::RemoteCommandResponse makeErrorResponse(const Status& errorStatus) {
+    invariant(!errorStatus.isOK());
+    BSONObjBuilder res;
+    CommandHelpers::appendCommandStatusNoThrow(res, errorStatus);
+    return {res.obj(), Milliseconds(0)};
+}
+
+executor::RemoteCommandResponse makeWriteConcernErrorResponse(
+    const Status& writeConcernErrorStatus) {
+    invariant(!writeConcernErrorStatus.isOK());
+    BSONObjBuilder res;
+    WriteConcernErrorDetail wcError;
+    wcError.setStatus(writeConcernErrorStatus);
+    res.append("ok", 1);
+    res.append("writeConcernError", wcError.toBSON());
+    return {res.obj(), Milliseconds(0)};
+}
+
+HostAndPort makeHostAndPort(const ShardId& shardId) {
+    return HostAndPort(str::stream() << shardId << ":123");
+}
+
+}  // namespace
+
+class AppendRawResponsesTest : public ShardServerTestFixture {
+protected:
+    /**
+     * Runs 'appendRawResponses' and asserts that the top-level status matches 'expectedStatus', the
+     * 'raw' sub-object contains the shards in 'expectedShardsInRawSubObj', and the writeConcern
+     * status matches 'expectedWriteConcernStatus'.
+     */
+    void runAppendRawResponsesExpect(
+        const std::vector<AsyncRequestsSender::Response>& shardResponses,
+        const Status& expectedStatus,
+        const std::vector<ShardId>& expectedShardsInRawSubObj,
+        const Status& expectedWriteConcernStatus = Status::OK()) {
+        BSONObjBuilder result;
+        std::string errmsg;
+
+        const auto ok = appendRawResponses(operationContext(),
+                                           &errmsg,
+                                           &result,
+                                           shardResponses,
+                                           {kIgnorableError1, kIgnorableError2});
+
+        if (!errmsg.empty()) {
+            CommandHelpers::appendSimpleCommandStatus(result, ok, errmsg);
+        }
+        const auto resultObj = result.obj();
+
+        // Check the top-level status.
+        if (expectedStatus.isOK()) {
+            ASSERT_FALSE(resultObj.hasField("code"));
+            ASSERT_FALSE(resultObj.hasField("codeName"));
+            ASSERT(errmsg.empty());
+        } else {
+            ASSERT_EQ(expectedStatus.code(), resultObj.getField("code").Int());
+            ASSERT_EQ(ErrorCodes::errorString(expectedStatus.code()),
+                      resultObj.getField("codeName").String());
+            ASSERT_EQ(expectedStatus.reason(), errmsg);
+        }
+
+        // Check the 'raw' sub-object.
+        const auto rawSubObj = resultObj.getField("raw").Obj();
+        ASSERT_EQ(rawSubObj.nFields(), int(expectedShardsInRawSubObj.size()));
+        for (const auto& shardId : expectedShardsInRawSubObj) {
+            const auto shardConnStr =
+                ConnectionString::forReplicaSet(shardId.toString(), {makeHostAndPort(shardId)})
+                    .toString();
+            ASSERT(rawSubObj.hasField(shardConnStr));
+        }
+
+        // Check for a writeConcern error.
+        if (expectedWriteConcernStatus.isOK()) {
+            ASSERT(resultObj.getField("writeConcernError").eoo());
+        } else {
+            ASSERT_EQ(expectedWriteConcernStatus,
+                      getWriteConcernStatusFromCommandResult(resultObj));
+        }
+    }
+
+    void setUp() override {
+        ShardServerTestFixture::setUp();
+
+        for (const auto& shardId : kShardIdList) {
+            auto shardTargeter = RemoteCommandTargeterMock::get(
+                uassertStatusOK(shardRegistry()->getShard(operationContext(), shardId))
+                    ->getTargeter());
+            shardTargeter->setConnectionStringReturnValue(
+                ConnectionString::forReplicaSet(shardId.toString(), {makeHostAndPort(shardId)}));
+        }
+    }
+
+    std::unique_ptr<ShardingCatalogClient> makeShardingCatalogClient(
+        std::unique_ptr<DistLockManager> distLockManager) override {
+
+        class StaticCatalogClient final : public ShardingCatalogClientMock {
+        public:
+            StaticCatalogClient(std::vector<ShardId> shardIds)
+                : ShardingCatalogClientMock(nullptr), _shardIds(std::move(shardIds)) {}
+
+            StatusWith<repl::OpTimeWith<std::vector<ShardType>>> getAllShards(
+                OperationContext* opCtx, repl::ReadConcernLevel readConcern) override {
+                std::vector<ShardType> shardTypes;
+                for (const auto& shardId : _shardIds) {
+                    const ConnectionString cs = ConnectionString::forReplicaSet(
+                        shardId.toString(), {makeHostAndPort(shardId)});
+                    ShardType sType;
+                    sType.setName(cs.getSetName());
+                    sType.setHost(cs.toString());
+                    shardTypes.push_back(std::move(sType));
+                };
+                return repl::OpTimeWith<std::vector<ShardType>>(shardTypes);
+            }
+
+        private:
+            const std::vector<ShardId> _shardIds;
+        };
+
+        return stdx::make_unique<StaticCatalogClient>(kShardIdList);
+    }
+
+    const ShardId kShard1{"s1"};
+    const ShardId kShard2{"s2"};
+    const ShardId kShard3{"s3"};
+    const ShardId kShard4{"s4"};
+    const ShardId kShard5{"s5"};
+    const std::vector<ShardId> kShardIdList{kShard1, kShard2, kShard3, kShard4, kShard5};
+};
+
+//
+// No errors
+//
+
+TEST_F(AppendRawResponsesTest, AllShardsReturnSuccess) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kOkResponse, makeHostAndPort(kShard1)},
+        {kShard2, kOkResponse, makeHostAndPort(kShard2)},
+        {kShard3, kOkResponse, makeHostAndPort(kShard3)}};
+
+    runAppendRawResponsesExpect(shardResponses, Status::OK(), {kShard1, kShard2, kShard3});
+}
+
+//
+// Only write concern errors
+//
+
+TEST_F(AppendRawResponsesTest, AllShardsReturnSuccessOneWithWriteConcernError) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1,
+         makeWriteConcernErrorResponse(kWriteConcernError1Status),
+         makeHostAndPort(kShard1)},
+        {kShard2, kOkResponse, makeHostAndPort(kShard2)},
+        {kShard3, kOkResponse, makeHostAndPort(kShard3)}};
+
+    runAppendRawResponsesExpect(
+        shardResponses, Status::OK(), {kShard1, kShard2, kShard3}, kWriteConcernError1Status);
+}
+
+TEST_F(AppendRawResponsesTest, AllShardsReturnSuccessMoreThanOneWithWriteConcernError) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1,
+         makeWriteConcernErrorResponse(kWriteConcernError1Status),
+         makeHostAndPort(kShard1)},
+        {kShard2,
+         makeWriteConcernErrorResponse(kWriteConcernError2Status),
+         makeHostAndPort(kShard2)},
+        {kShard3, kOkResponse, makeHostAndPort(kShard3)}};
+
+    // The first writeConcern error is reported.
+    runAppendRawResponsesExpect(
+        shardResponses, Status::OK(), {kShard1, kShard2, kShard3}, kWriteConcernError1Status);
+}
+
+TEST_F(AppendRawResponsesTest, AllShardsReturnSuccessAllWithWriteConcernError) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1,
+         makeWriteConcernErrorResponse(kWriteConcernError1Status),
+         makeHostAndPort(kShard1)},
+        {kShard2,
+         makeWriteConcernErrorResponse(kWriteConcernError2Status),
+         makeHostAndPort(kShard2)},
+        {kShard3,
+         makeWriteConcernErrorResponse(kWriteConcernError1Status),
+         makeHostAndPort(kShard3)}};
+
+    // The first writeConcern error is reported.
+    runAppendRawResponsesExpect(
+        shardResponses, Status::OK(), {kShard1, kShard2, kShard3}, kWriteConcernError1Status);
+}
+
+//
+// Errors *sending* the requests.
+//
+
+TEST_F(AppendRawResponsesTest, AllAttemptsToSendRequestsReturnSameIgnorableError) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kIgnorableError1Status, boost::none},
+        {kShard2, kIgnorableError1Status, boost::none},
+        {kShard3, kIgnorableError1Status, boost::none}};
+
+    // The ignorable error gets promoted to a non-ignorable error.
+    runAppendRawResponsesExpect(
+        shardResponses, kIgnorableError1Status, {kShard1, kShard2, kShard3});
+}
+
+TEST_F(AppendRawResponsesTest,
+       SomeShardsReturnSuccessRestOfAttemptsToSendRequestsReturnSameIgnorableError) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kOkResponse, makeHostAndPort(kShard1)},
+        {kShard2, kOkResponse, makeHostAndPort(kShard2)},
+        {kShard3, kIgnorableError1Status, boost::none},
+        {kShard4, kIgnorableError1Status, boost::none}};
+
+    // The ignorable errors are ignored.
+    runAppendRawResponsesExpect(shardResponses, Status::OK(), {kShard1, kShard2});
+}
+
+TEST_F(AppendRawResponsesTest, AttemptsToSendRequestsReturnDifferentIgnorableErrors) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kIgnorableError1Status, boost::none},
+        {kShard2, kIgnorableError2Status, boost::none},
+        {kShard3, kIgnorableError1Status, boost::none}};
+
+    // The *first* ignorable error gets promoted to a non-ignorable error.
+    runAppendRawResponsesExpect(
+        shardResponses, kIgnorableError1Status, {kShard1, kShard2, kShard3});
+}
+
+TEST_F(AppendRawResponsesTest,
+       SomeShardsReturnSuccessRestOfAttemptsToSendRequestsReturnDifferentIgnorableErrors) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kOkResponse, makeHostAndPort(kShard1)},
+        {kShard2, kOkResponse, makeHostAndPort(kShard2)},
+        {kShard3, kIgnorableError1Status, boost::none},
+        {kShard4, kIgnorableError2Status, boost::none}};
+
+    // The ignorable errors are ignored.
+    runAppendRawResponsesExpect(shardResponses, Status::OK(), {kShard1, kShard2});
+}
+
+TEST_F(AppendRawResponsesTest, AllAttemptsToSendRequestsReturnSameNonIgnorableError) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kNonIgnorableError1Status, boost::none},
+        {kShard2, kNonIgnorableError1Status, boost::none},
+        {kShard3, kNonIgnorableError1Status, boost::none}};
+
+    // The non-ignorable error is returned.
+    runAppendRawResponsesExpect(
+        shardResponses, kNonIgnorableError1Status, {kShard1, kShard2, kShard3});
+}
+
+TEST_F(AppendRawResponsesTest,
+       SomeShardsReturnSuccessRestOfAttemptsToSendRequestsReturnSameNonIgnorableError) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kOkResponse, makeHostAndPort(kShard1)},
+        {kShard2, kOkResponse, makeHostAndPort(kShard2)},
+        {kShard3, kNonIgnorableError1Status, boost::none},
+        {kShard4, kNonIgnorableError1Status, boost::none}};
+
+    // The non-ignorable error is returned.
+    runAppendRawResponsesExpect(
+        shardResponses, kNonIgnorableError1Status, {kShard1, kShard2, kShard3, kShard4});
+}
+
+TEST_F(AppendRawResponsesTest, AttemptsToSendRequestsReturnDifferentNonIgnorableErrors) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kNonIgnorableError1Status, boost::none},
+        {kShard2, kNonIgnorableError2Status, boost::none},
+        {kShard3, kNonIgnorableError1Status, boost::none}};
+
+    // The first non-ignorable error is returned.
+    runAppendRawResponsesExpect(
+        shardResponses, kNonIgnorableError1Status, {kShard1, kShard2, kShard3});
+}
+
+TEST_F(AppendRawResponsesTest,
+       SomeShardsReturnSuccessRestOfAttemptsToSendRequestsReturnDifferentNonIgnorableErrors) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kOkResponse, makeHostAndPort(kShard1)},
+        {kShard2, kOkResponse, makeHostAndPort(kShard2)},
+        {kShard3, kNonIgnorableError1Status, boost::none},
+        {kShard4, kNonIgnorableError2Status, boost::none}};
+
+    // The first non-ignorable error is returned.
+    runAppendRawResponsesExpect(
+        shardResponses, kNonIgnorableError1Status, {kShard1, kShard2, kShard3, kShard4});
+}
+
+TEST_F(AppendRawResponsesTest, AllAttemptsToSendRequestsReturnErrorsSomeIgnorableSomeNonIgnorable) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kIgnorableError1Status, boost::none},
+        {kShard2, kIgnorableError2Status, boost::none},
+        {kShard3, kNonIgnorableError1Status, boost::none},
+        {kShard4, kNonIgnorableError2Status, boost::none}};
+
+    // The first non-ignorable error is returned.
+    runAppendRawResponsesExpect(shardResponses, kNonIgnorableError1Status, {kShard3, kShard4});
+}
+
+TEST_F(
+    AppendRawResponsesTest,
+    SomeShardsReturnSuccessRestOfAttemptsToSendRequestsReturnErrorsSomeIgnorableSomeNonIgnorable) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kIgnorableError1Status, boost::none},
+        {kShard2, kOkResponse, makeHostAndPort(kShard2)},
+        {kShard3, kNonIgnorableError1Status, boost::none},
+        {kShard4, kNonIgnorableError2Status, boost::none}};
+
+    // The first non-ignorable error is returned.
+    runAppendRawResponsesExpect(
+        shardResponses, kNonIgnorableError1Status, {kShard2, kShard3, kShard4});
+}
+
+//
+// Errors *processing* the requests.
+//
+
+TEST_F(AppendRawResponsesTest, AllShardsReturnSameIgnorableError) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, makeErrorResponse(kIgnorableError1Status), makeHostAndPort(kShard1)},
+        {kShard2, makeErrorResponse(kIgnorableError1Status), makeHostAndPort(kShard2)},
+        {kShard3, makeErrorResponse(kIgnorableError1Status), makeHostAndPort(kShard3)}};
+
+    // The ignorable error gets promoted to a non-ignorable error.
+    runAppendRawResponsesExpect(
+        shardResponses, kIgnorableError1Status, {kShard1, kShard2, kShard3});
+}
+
+TEST_F(AppendRawResponsesTest, SomeShardsReturnSuccessRestReturnSameIgnorableError) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kOkResponse, makeHostAndPort(kShard1)},
+        {kShard2, kOkResponse, makeHostAndPort(kShard2)},
+        {kShard3, makeErrorResponse(kIgnorableError1Status), makeHostAndPort(kShard3)},
+        {kShard4, makeErrorResponse(kIgnorableError1Status), makeHostAndPort(kShard4)}};
+
+    // The ignorable errors are ignored.
+    runAppendRawResponsesExpect(shardResponses, Status::OK(), {kShard1, kShard2});
+}
+
+TEST_F(AppendRawResponsesTest, ShardsReturnDifferentIgnorableErrors) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, makeErrorResponse(kIgnorableError1Status), makeHostAndPort(kShard1)},
+        {kShard2, makeErrorResponse(kIgnorableError2Status), makeHostAndPort(kShard2)},
+        {kShard3, makeErrorResponse(kIgnorableError1Status), makeHostAndPort(kShard3)}};
+
+    // The *first* ignorable error gets promoted to a non-ignorable error.
+    runAppendRawResponsesExpect(
+        shardResponses, kIgnorableError1Status, {kShard1, kShard2, kShard3});
+}
+
+TEST_F(AppendRawResponsesTest, SomeShardsReturnSuccessRestReturnDifferentIgnorableErrors) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kOkResponse, makeHostAndPort(kShard1)},
+        {kShard2, kOkResponse, makeHostAndPort(kShard2)},
+        {kShard3, makeErrorResponse(kIgnorableError1Status), makeHostAndPort(kShard3)},
+        {kShard4, makeErrorResponse(kIgnorableError2Status), makeHostAndPort(kShard4)}};
+
+    // The ignorable errors are ignored.
+    runAppendRawResponsesExpect(shardResponses, Status::OK(), {kShard1, kShard2});
+}
+
+TEST_F(AppendRawResponsesTest, AllShardsReturnSameNonIgnorableError) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, makeErrorResponse(kNonIgnorableError1Status), makeHostAndPort(kShard1)},
+        {kShard2, makeErrorResponse(kNonIgnorableError1Status), makeHostAndPort(kShard2)},
+        {kShard3, makeErrorResponse(kNonIgnorableError1Status), makeHostAndPort(kShard3)}};
+
+    // The non-ignorable error is returned.
+    runAppendRawResponsesExpect(
+        shardResponses, kNonIgnorableError1Status, {kShard1, kShard2, kShard3});
+}
+
+TEST_F(AppendRawResponsesTest, SomeShardsReturnSuccessRestReturnSameNonIgnorableError) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kOkResponse, makeHostAndPort(kShard1)},
+        {kShard2, kOkResponse, makeHostAndPort(kShard2)},
+        {kShard3, makeErrorResponse(kNonIgnorableError1Status), makeHostAndPort(kShard3)},
+        {kShard4, makeErrorResponse(kNonIgnorableError1Status), makeHostAndPort(kShard4)}};
+
+    // The non-ignorable error is returned.
+    runAppendRawResponsesExpect(
+        shardResponses, kNonIgnorableError1Status, {kShard1, kShard2, kShard3, kShard4});
+}
+
+TEST_F(AppendRawResponsesTest, ShardsReturnDifferentNonIgnorableErrors) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, makeErrorResponse(kNonIgnorableError1Status), makeHostAndPort(kShard1)},
+        {kShard2, makeErrorResponse(kNonIgnorableError2Status), makeHostAndPort(kShard2)},
+        {kShard3, makeErrorResponse(kNonIgnorableError1Status), makeHostAndPort(kShard3)}};
+
+    // The first non-ignorable error is returned.
+    runAppendRawResponsesExpect(
+        shardResponses, kNonIgnorableError1Status, {kShard1, kShard2, kShard3});
+}
+
+TEST_F(AppendRawResponsesTest, SomeShardsReturnSuccessRestReturnDifferentNonIgnorableErrors) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kOkResponse, makeHostAndPort(kShard1)},
+        {kShard2, kOkResponse, makeHostAndPort(kShard2)},
+        {kShard3, makeErrorResponse(kNonIgnorableError1Status), makeHostAndPort(kShard3)},
+        {kShard4, makeErrorResponse(kNonIgnorableError2Status), makeHostAndPort(kShard4)}};
+
+    // The first non-ignorable error is returned.
+    runAppendRawResponsesExpect(
+        shardResponses, kNonIgnorableError1Status, {kShard1, kShard2, kShard3, kShard4});
+}
+
+TEST_F(AppendRawResponsesTest, AllShardsReturnErrorsSomeIgnorableSomeNonIgnorable) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, makeErrorResponse(kIgnorableError1Status), makeHostAndPort(kShard1)},
+        {kShard2, makeErrorResponse(kIgnorableError2Status), makeHostAndPort(kShard2)},
+        {kShard3, makeErrorResponse(kNonIgnorableError1Status), makeHostAndPort(kShard3)},
+        {kShard4, makeErrorResponse(kNonIgnorableError2Status), makeHostAndPort(kShard4)}};
+
+    // The first non-ignorable error is returned.
+    runAppendRawResponsesExpect(shardResponses, kNonIgnorableError1Status, {kShard3, kShard4});
+}
+
+TEST_F(AppendRawResponsesTest,
+       SomeShardsReturnSuccessRestReturnErrorsSomeIgnorableSomeNonIgnorable) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, makeErrorResponse(kIgnorableError1Status), makeHostAndPort(kShard1)},
+        {kShard2, kOkResponse, makeHostAndPort(kShard2)},
+        {kShard3, makeErrorResponse(kNonIgnorableError1Status), makeHostAndPort(kShard3)},
+        {kShard4, makeErrorResponse(kNonIgnorableError2Status), makeHostAndPort(kShard4)}};
+
+    // The first non-ignorable error is returned.
+    runAppendRawResponsesExpect(
+        shardResponses, kNonIgnorableError1Status, {kShard2, kShard3, kShard4});
+}
+
+// Mix of errors *sending* and *processing* the requests.
+
+TEST_F(AppendRawResponsesTest,
+       AllShardsReturnErrorsMixOfErrorsSendingRequestsAndErrorsProcessingRequests) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kIgnorableError1Status, boost::none},
+        {kShard2, makeErrorResponse(kIgnorableError2Status), makeHostAndPort(kShard2)},
+        {kShard3, kNonIgnorableError1Status, boost::none},
+        {kShard4, makeErrorResponse(kNonIgnorableError2Status), makeHostAndPort(kShard4)}};
+
+    // The first non-ignorable error is returned.
+    runAppendRawResponsesExpect(shardResponses, kNonIgnorableError1Status, {kShard3, kShard4});
+}
+
+TEST_F(
+    AppendRawResponsesTest,
+    SomeShardsReturnSuccessRestReturnErrorsMixOfErrorsSendingRequestsAndErrorsProcessingRequests) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kIgnorableError1Status, boost::none},
+        {kShard2, makeErrorResponse(kIgnorableError2Status), makeHostAndPort(kShard2)},
+        {kShard3, kNonIgnorableError1Status, boost::none},
+        {kShard4, makeErrorResponse(kNonIgnorableError2Status), makeHostAndPort(kShard4)},
+        {kShard5, kOkResponse, HostAndPort("e:1")}};
+
+    // The first non-ignorable error is returned.
+    runAppendRawResponsesExpect(
+        shardResponses, kNonIgnorableError1Status, {kShard3, kShard4, kShard5});
+}
+
+// Mix of errors sending or processing the requests *and* writeConcern error.
+
+TEST_F(AppendRawResponsesTest, SomeShardsReturnSuccessWithWriteConcernErrorRestReturnMixOfErrors) {
+    std::vector<AsyncRequestsSender::Response> shardResponses{
+        {kShard1, kIgnorableError1Status, boost::none},
+        {kShard2, makeErrorResponse(kIgnorableError2Status), makeHostAndPort(kShard2)},
+        {kShard3, kNonIgnorableError1Status, boost::none},
+        {kShard4, makeErrorResponse(kNonIgnorableError2Status), makeHostAndPort(kShard4)},
+        {kShard5,
+         makeWriteConcernErrorResponse(kWriteConcernError1Status),
+         makeHostAndPort(kShard5)}};
+
+    // The first non-ignorable error is returned, and no writeConcern error is reported at the top
+    // level.
+    runAppendRawResponsesExpect(
+        shardResponses, kNonIgnorableError1Status, {kShard3, kShard4, kShard5});
+}
+
+}  // namespace mongo
diff --git a/src/mongo/s/cluster_commands_helpers.cpp b/src/mongo/s/cluster_commands_helpers.cpp
index 5c498273192..981f33669ef 100644
--- a/src/mongo/s/cluster_commands_helpers.cpp
+++ b/src/mongo/s/cluster_commands_helpers.cpp
@@ -342,111 +342,100 @@ AsyncRequestsSender::Response executeCommandAgainstDatabasePrimary(
 bool appendRawResponses(OperationContext* opCtx,
                         std::string* errmsg,
                         BSONObjBuilder* output,
-                        std::vector<AsyncRequestsSender::Response> shardResponses,
-                        std::set<ErrorCodes::Error> ignoredErrors) {
-    // Always include ShardNotFound as an ignored error, since this node may not have realized a
+                        const std::vector<AsyncRequestsSender::Response>& shardResponses,
+                        std::set<ErrorCodes::Error> ignorableErrors) {
+    // Always include ShardNotFound as an ignorable error, since this node may not have realized a
     // shard has been removed.
-    ignoredErrors.insert(ErrorCodes::ShardNotFound);
+    ignorableErrors.insert(ErrorCodes::ShardNotFound);
 
-    BSONObjBuilder subobj;  // Stores raw responses by ConnectionString
+    std::vector<std::pair<ShardId, BSONObj>> successResponsesReceived;
+    std::vector<std::pair<ShardId, Status>> ignorableErrorsReceived;
+    std::vector<std::pair<ShardId, Status>> nonIgnorableErrorsReceived;
 
-    // Stores all errors; we will remove ignoredErrors later if some shard returned success.
-    std::vector<std::pair<std::string, Status>> errors;  // Stores errors by ConnectionString
+    boost::optional<std::pair<ShardId, BSONElement>> firstWriteConcernErrorReceived;
 
-    BSONElement wcErrorElem;  // Stores the first writeConcern error we encounter
-    ShardId wcErrorShardId;   // Stores the shardId for the first writeConcern error we encounter
-    bool hasWCError = false;  // Whether we have encountered a writeConcern error yet
+    const auto processError = [&](const ShardId& shardId, const Status& status) {
+        invariant(!status.isOK());
+        if (ignorableErrors.find(status.code()) != ignorableErrors.end()) {
+            ignorableErrorsReceived.emplace_back(std::move(shardId), std::move(status));
+            return;
+        }
+        nonIgnorableErrorsReceived.emplace_back(shardId, status);
+    };
 
+    // Do a pass through all the received responses and group them into success, ignorable, and
+    // non-ignorable.
     for (const auto& shardResponse : shardResponses) {
-        // Get the Shard object in order to get the shard's ConnectionString.
-        const auto swShard =
-            Grid::get(opCtx)->shardRegistry()->getShard(opCtx, shardResponse.shardId);
-        if (ErrorCodes::ShardNotFound == swShard.getStatus().code()) {
-            // Store the error by ShardId, since we cannot know the shard connection string, and it
-            // is only used for reporting.
-            errors.push_back(std::make_pair(shardResponse.shardId.toString(), swShard.getStatus()));
-            continue;
-        }
-        const auto shard = uassertStatusOK(swShard);
-        const auto shardConnStr = shard->getConnString().toString();
+        const auto& shardId = shardResponse.shardId;
 
-        Status sendStatus = shardResponse.swResponse.getStatus();
+        const auto sendStatus = shardResponse.swResponse.getStatus();
         if (!sendStatus.isOK()) {
-            // Convert the error status back into the form of a command result and append it as the
-            // raw response.
-            BSONObjBuilder statusObjBob;
-            CommandHelpers::appendCommandStatusNoThrow(statusObjBob, sendStatus);
-            subobj.append(shardConnStr, statusObjBob.obj());
-
-            errors.push_back(std::make_pair(shardConnStr, sendStatus));
+            processError(shardId, sendStatus);
             continue;
         }
 
-        // Got a response from the shard.
-
-        auto& resObj = shardResponse.swResponse.getValue().data;
-
-        // Append the shard's raw response.
-        subobj.append(shardConnStr, CommandHelpers::filterCommandReplyForPassthrough(resObj));
-
-        auto commandStatus = getStatusFromCommandResult(resObj);
+        const auto& resObj = shardResponse.swResponse.getValue().data;
+        const auto commandStatus = getStatusFromCommandResult(resObj);
         if (!commandStatus.isOK()) {
-            errors.push_back(std::make_pair(shardConnStr, std::move(commandStatus)));
+            processError(shardId, commandStatus);
+            continue;
         }
 
-        // Report the first writeConcern error we see.
-        if (!hasWCError && (wcErrorElem = resObj["writeConcernError"])) {
-            wcErrorShardId = shardResponse.shardId;
-            hasWCError = true;
+        if (!firstWriteConcernErrorReceived && resObj["writeConcernError"]) {
+            firstWriteConcernErrorReceived.emplace(shardId, resObj["writeConcernError"]);
         }
-    }
 
-    output->append("raw", subobj.done());
-
-    if (hasWCError) {
-        appendWriteConcernErrorToCmdResponse(wcErrorShardId, wcErrorElem, *output);
+        successResponsesReceived.emplace_back(shardId, resObj);
     }
 
-    // If any shard returned success, filter out ignored errors
-    bool someShardReturnedOK = (errors.size() != shardResponses.size());
+    // If all shards reported ignorable errors, promote them all to non-ignorable errors.
+    if (ignorableErrorsReceived.size() == shardResponses.size()) {
+        invariant(nonIgnorableErrorsReceived.empty());
+        nonIgnorableErrorsReceived = std::move(ignorableErrorsReceived);
+    }
 
-    BSONObjBuilder errorBob;
-    int commonErrCode = -1;
-    auto it = errors.begin();
-    while (it != errors.end()) {
-        if (someShardReturnedOK && ignoredErrors.find(it->second.code()) != ignoredErrors.end()) {
-            // Ignore the error.
-            it = errors.erase(it);
-        } else {
-            errorBob.append(it->first, it->second.reason());
-            if (commonErrCode == -1) {
-                commonErrCode = it->second.code();
-            } else if (commonErrCode != it->second.code()) {
-                commonErrCode = 0;
-            }
-            ++it;
+    // Append a 'raw' field containing the success responses and non-ignorable error responses.
+    BSONObjBuilder rawShardResponses;
+    const auto appendRawResponse = [&](const ShardId& shardId, const BSONObj& response) {
+        // Try to report the response by the shard's full connection string.
+        try {
+            const auto shardConnString =
+                uassertStatusOK(Grid::get(opCtx)->shardRegistry()->getShard(opCtx, shardId))
+                    ->getConnString();
+            rawShardResponses.append(shardConnString.toString(),
+                                     CommandHelpers::filterCommandReplyForPassthrough(response));
+        } catch (const ExceptionFor<ErrorCodes::ShardNotFound>&) {
+            // If we could not get the shard's connection string, fall back to reporting the
+            // response by shard id.
+            rawShardResponses.append(shardId, response);
         }
+    };
+    for (const auto& success : successResponsesReceived) {
+        appendRawResponse(success.first, success.second);
     }
-    BSONObj errobj = errorBob.obj();
-
-    if (!errobj.isEmpty()) {
-        *errmsg = errobj.toString();
-
-        // If every error has a code, and the code for all errors is the same, then add
-        // a top-level field "code" with this value to the output object.
-        if (commonErrCode > 0) {
-            output->append("code", commonErrCode);
-            output->append("codeName", ErrorCodes::errorString(ErrorCodes::Error(commonErrCode)));
-            if (errors.size() == 1) {
-                // Only propagate extra info if there was a single error object.
-                if (auto extraInfo = errors.begin()->second.extraInfo()) {
-                    extraInfo->serialize(output);
-                }
-            }
+    for (const auto& error : nonIgnorableErrorsReceived) {
+        BSONObjBuilder statusObjBob;
+        CommandHelpers::appendCommandStatusNoThrow(statusObjBob, error.second);
+        appendRawResponse(error.first, statusObjBob.obj());
+    }
+    output->append("raw", rawShardResponses.done());
+
+    // If there were no non-ignorable errors, report success (possibly with a writeConcern error).
+    if (nonIgnorableErrorsReceived.empty()) {
+        if (firstWriteConcernErrorReceived) {
+            appendWriteConcernErrorToCmdResponse(firstWriteConcernErrorReceived->first,
+                                                 firstWriteConcernErrorReceived->second,
+                                                 *output);
         }
-        return false;
+        return true;
     }
-    return true;
+
+    // There was a non-ignorable error. Choose the first non-ignorable error as the top-level error.
+    const auto& firstNonIgnorableError = nonIgnorableErrorsReceived.front().second;
+    output->append("code", firstNonIgnorableError.code());
+    output->append("codeName", ErrorCodes::errorString(firstNonIgnorableError.code()));
+    *errmsg = firstNonIgnorableError.reason();
+    return false;
 }
 
 BSONObj extractQuery(const BSONObj& cmdObj) {
diff --git a/src/mongo/s/cluster_commands_helpers.h b/src/mongo/s/cluster_commands_helpers.h
index fce94d5fc43..1e3a9d91fbb 100644
--- a/src/mongo/s/cluster_commands_helpers.h
+++ b/src/mongo/s/cluster_commands_helpers.h
@@ -163,7 +163,7 @@ AsyncRequestsSender::Response executeCommandAgainstDatabasePrimary(
 bool appendRawResponses(OperationContext* opCtx,
                         std::string* errmsg,
                         BSONObjBuilder* output,
-                        std::vector<AsyncRequestsSender::Response> shardResponses,
+                        const std::vector<AsyncRequestsSender::Response>& shardResponses,
                         std::set<ErrorCodes::Error> ignoredErrors = {});
 
 /**