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

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:

jstests/concurrency/fsm_libs/cluster.js

@@ -511,7 +511,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

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();
+}());

src/mongo/base/error_codes.yml

@@ -4,7 +4,7 @@ error_categories:
     - NetworkTimeoutError
     - Interruption
     # isNotPrimaryError() includes all codes that indicate that the node that received the request
-    # was not master at some point during command processing,regardless of whether some write may
+    # was not primary at some point during command processing,regardless of whether some write may
     # have happened. If you care about whether a write could have happened,check for individual
     # codes.
     - NotPrimaryError

src/mongo/db/repl/replication_coordinator_impl.cpp

@@ -2922,10 +2922,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 {

src/mongo/db/repl/replication_coordinator_impl_test.cpp

@@ -2667,7 +2667,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.
     response = getReplCoord()->awaitHelloResponse(opCtx.get(), {}, boost::none, boost::none);
     ASSERT_FALSE(response->isWritablePrimary());
 

src/mongo/db/repl/replication_coordinator_mock.h

@@ -305,7 +305,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);