SERVER-50414 Reads should return not primary error messages during rollback if client sent helloOk

(cherry picked from commit d633efa72090a864442ef7232e1261d3b25bc27c)
(cherry picked from commit a04b42ed317f084387e70120fc0ad6ab0d6300ac)

6 files changed, 89 insertions, 6 deletions

diff --git a/etc/backports_required_for_multiversion_tests.yml b/etc/backports_required_for_multiversion_tests.yml
index 1c575e036c8..03e342a50ac 100644
--- a/etc/backports_required_for_multiversion_tests.yml
+++ b/etc/backports_required_for_multiversion_tests.yml
@@ -106,6 +106,8 @@ all:
     test_file: jstests/replsets/not_primary_errors_returned_if_client_sends_helloOk.js
   - ticket: SERVER-50412
     test_file: jstests/sharding/mongos_helloOk_protocol.js
+  - ticket: SERVER-50414
+    test_file: jstests/replsets/not_primary_errors_returned_during_rollback_if_helloOk.js
 
 # Tests that should only be excluded from particular suites should be listed under that suite.
 suites:
diff --git a/jstests/concurrency/fsm_libs/cluster.js b/jstests/concurrency/fsm_libs/cluster.js
index cc3dc0df1c5..be0daa47ff8 100644
--- a/jstests/concurrency/fsm_libs/cluster.js
+++ b/jstests/concurrency/fsm_libs/cluster.js
@@ -493,7 +493,7 @@ var Cluster = function(options) {
             res.databases.forEach(dbInfo => {
                 // Don't perform listCollections on the admin or config database through a mongos
                 // connection when stepping down the config server primary, because both are stored
-                // on the config server, and listCollections may return a not master error if the
+                // on the config server, and listCollections may return a NotPrimaryError if the
                 // mongos is stale.
                 //
                 // TODO SERVER-30949: listCollections through mongos should automatically retry on
diff --git a/jstests/replsets/not_primary_errors_returned_during_rollback_if_helloOk.js b/jstests/replsets/not_primary_errors_returned_during_rollback_if_helloOk.js
new file mode 100644
index 00000000000..48fd2a88847
--- /dev/null
+++ b/jstests/replsets/not_primary_errors_returned_during_rollback_if_helloOk.js
@@ -0,0 +1,76 @@
+/*
+ * Tests that reads that fail during rollback with a NotPrimaryError will replace their
+ * "not master" error messages with "not primary" if the client sends "helloOk:true" as a part
+ * of their isMaster request.
+ *
+ * In practice, drivers will send "helloOk: true" in the initial handshake when
+ * opening a connection to the database.
+ * @tags: [requires_majority_read_concern]
+ */
+
+(function() {
+"use strict";
+
+load("jstests/replsets/libs/rollback_test.js");
+load("jstests/replsets/rslib.js");
+
+const dbName = "test";
+const collName = "not_primary_errors_returned_during_rollback_if_helloOk";
+
+// Set up Rollback Test.
+let rollbackTest = new RollbackTest();
+
+// Insert a document to be read later.
+assert.commandWorked(rollbackTest.getPrimary().getDB(dbName)[collName].insert({}));
+
+let rollbackNode = rollbackTest.transitionToRollbackOperations();
+
+setFailPoint(rollbackNode, "rollbackHangAfterTransitionToRollback");
+
+// Start rollback.
+rollbackTest.transitionToSyncSourceOperationsBeforeRollback();
+rollbackTest.transitionToSyncSourceOperationsDuringRollback();
+
+jsTestLog("Reconnecting to " + rollbackNode.host + " after rollback");
+reconnect(rollbackNode.getDB(dbName));
+
+// Wait for rollback to hang.
+checkLog.contains(rollbackNode, "rollbackHangAfterTransitionToRollback fail point enabled.");
+
+// Make sure we can't read during rollback. Since we want to exercise the real check for
+// primary in the 'find' command, we have to disable the best-effort check for primary in service
+// entry point.
+setFailPoint(rollbackNode, "skipCheckingForNotPrimaryInCommandDispatch");
+jsTestLog("Reading during rollback returns not master error message");
+let res = assert.commandFailedWithCode(rollbackNode.getDB(dbName).runCommand({"find": collName}),
+                                       ErrorCodes.NotPrimaryOrSecondary,
+                                       "find did not fail with NotPrimaryOrSecondary");
+
+// Since we did not send "helloOk: true", the error message should include "not master or
+// secondary".
+assert(res.errmsg.includes("not master or secondary"), res);
+
+// An isMaster response will not contain "helloOk: true" if the client does not send
+// helloOk in the request.
+res = assert.commandWorked(rollbackNode.getDB(dbName).adminCommand({isMaster: 1}));
+assert.eq(res.helloOk, undefined);
+
+// Run the isMaster command with "helloOk: true" on the secondary.
+res = assert.commandWorked(rollbackNode.getDB(dbName).adminCommand({isMaster: 1, helloOk: true}));
+// The response should contain "helloOk: true", which indicates to the client that the
+// server supports the hello command.
+assert.eq(res.helloOk, true);
+
+jsTestLog("Reading during rollback returns not primary error message");
+res = assert.commandFailedWithCode(rollbackNode.getDB(dbName).runCommand({"find": collName}),
+                                   ErrorCodes.NotPrimaryOrSecondary,
+                                   "find did not fail with NotPrimaryOrSecondary");
+// Since we sent "helloOk: true", the error message should include "not primary or secondary".
+assert(res.errmsg.includes("not primary or secondary"), res);
+assert(!res.errmsg.includes("not master"), res);
+
+clearFailPoint(rollbackNode, "rollbackHangAfterTransitionToRollback");
+
+rollbackTest.transitionToSteadyStateOperations();
+rollbackTest.stop();
+}());
diff --git a/src/mongo/db/repl/replication_coordinator_impl.cpp b/src/mongo/db/repl/replication_coordinator_impl.cpp
index b355588bb0b..b9c730d12c1 100644
--- a/src/mongo/db/repl/replication_coordinator_impl.cpp
+++ b/src/mongo/db/repl/replication_coordinator_impl.cpp
@@ -2359,10 +2359,15 @@ Status ReplicationCoordinatorImpl::checkCanServeReadsFor_UNSAFE(OperationContext
         if (isPrimaryOrSecondary) {
             return Status::OK();
         }
-        return Status(ErrorCodes::NotPrimaryOrSecondary,
-                      "not master or secondary; cannot currently read from this replSet member");
+        const auto msg = client->supportsHello()
+            ? "not primary or secondary; cannot currently read from this replSet member"_sd
+            : "not master or secondary; cannot currently read from this replSet member"_sd;
+        return Status(ErrorCodes::NotPrimaryOrSecondary, msg);
     }
-    return Status(ErrorCodes::NotPrimaryNoSecondaryOk, "not master and slaveOk=false");
+
+    const auto msg = client->supportsHello() ? "not primary and secondaryOk=false"_sd
+                                             : "not master and slaveOk=false"_sd;
+    return Status(ErrorCodes::NotPrimaryNoSecondaryOk, msg);
 }
 
 bool ReplicationCoordinatorImpl::isInPrimaryOrSecondaryState(OperationContext* opCtx) const {
diff --git a/src/mongo/db/repl/replication_coordinator_impl_test.cpp b/src/mongo/db/repl/replication_coordinator_impl_test.cpp
index 42433a2756b..a072a500a08 100644
--- a/src/mongo/db/repl/replication_coordinator_impl_test.cpp
+++ b/src/mongo/db/repl/replication_coordinator_impl_test.cpp
@@ -2603,7 +2603,7 @@ TEST_F(StepDownTest, InterruptingAfterUnconditionalStepdownDoesNotRestoreWriteAv
            stepDownStatus == ErrorCodes::Interrupted);
     ASSERT_TRUE(getReplCoord()->getMemberState().secondary());
 
-    // We should still be indicating that we are not master.
+    // We should still be indicating that we are not a writable primary.
     getReplCoord()->fillIsMasterForReplSet(&response, {});
     ASSERT_FALSE(response.isMaster());
 
diff --git a/src/mongo/db/repl/replication_coordinator_mock.h b/src/mongo/db/repl/replication_coordinator_mock.h
index 21f6c7a89db..3c11e41c523 100644
--- a/src/mongo/db/repl/replication_coordinator_mock.h
+++ b/src/mongo/db/repl/replication_coordinator_mock.h
@@ -296,7 +296,7 @@ public:
         AwaitReplicationReturnValueFunction returnValueFunction);
 
     /**
-     * Always allow writes even if this node is not master. Used by sharding unit tests.
+     * Always allow writes even if this node is a writable primary. Used by sharding unit tests.
      */
     void alwaysAllowWrites(bool allowWrites);