SERVER-47125 Don't trust OplogQueryMetadata.primaryIndex

Author: ajdavis

src/mongo/db/repl/README.md

@@ -346,12 +346,11 @@ It then creates a `ReplSetHeartbeatResponse` object. This includes:
 2. The receiving node's election time
 3. The receiving node's last applied OpTime
 4. The receiving node's last durable OpTime
-5. The node the receiving node thinks is primary
-6. The term of the receiving node
-7. The state of the receiving node
-8. The receiving node's sync source
-9. The receiving node's `ReplicaSetConfig` version
-10. Whether the receiving node is primary
+5. The term of the receiving node
+6. The state of the receiving node
+7. The receiving node's sync source
+8. The receiving node's `ReplicaSetConfig` version
+9. Whether the receiving node is primary
 
 When the sending node receives the response to the heartbeat, it first processes its
 `ReplSetMetadata` like before.

src/mongo/db/repl/data_replicator_external_state_impl.cpp

@@ -86,7 +86,7 @@ void DataReplicatorExternalStateImpl::processMetadata(const rpc::ReplSetMetadata
 
     _replicationCoordinator->processReplSetMetadata(replMetadata);
 
-    if (oqMetadata.getPrimaryIndex() != rpc::OplogQueryMetadata::kNoPrimary) {
+    if (oqMetadata.hasPrimaryIndex()) {
         _replicationCoordinator->cancelAndRescheduleElectionTimeout();
     }
 }

src/mongo/db/repl/oplog_fetcher.cpp

@@ -859,7 +859,7 @@ Status OplogFetcher::_onSuccessfulBatch(const Documents& documents) {
         errMsg << " (config version: " << replSetMetadata.getConfigVersion();
         errMsg << "; last applied optime: " << oqMetadata.getLastOpApplied().toString();
         errMsg << "; sync source index: " << oqMetadata.getSyncSourceIndex();
-        errMsg << "; primary index: " << oqMetadata.getPrimaryIndex();
+        errMsg << "; has primary index: " << oqMetadata.hasPrimaryIndex();
         errMsg << ") is no longer valid";
         errMsg << "last fetched optime: " << lastFetched.toString();
         return Status(ErrorCodes::InvalidSyncSource, errMsg);

src/mongo/db/repl/oplog_fetcher_test.cpp

@@ -853,8 +853,8 @@ TEST_F(OplogFetcherTest, ValidMetadataWithInResponseShouldBeForwardedToProcessMe
     ASSERT_TRUE(dataReplicatorExternalState->metadataWasProcessed);
     ASSERT_EQUALS(replSetMetadata.getIsPrimary(),
                   dataReplicatorExternalState->replMetadataProcessed.getIsPrimary());
-    ASSERT_EQUALS(oqMetadata.getPrimaryIndex(),
-                  dataReplicatorExternalState->oqMetadataProcessed.getPrimaryIndex());
+    ASSERT_EQUALS(oqMetadata.hasPrimaryIndex(),
+                  dataReplicatorExternalState->oqMetadataProcessed.hasPrimaryIndex());
 }
 
 TEST_F(OplogFetcherTest, MetadataAndBatchAreNotProcessedWhenSyncSourceRollsBack) {

src/mongo/db/repl/replication_coordinator_impl_test.cpp

@@ -6388,7 +6388,7 @@ TEST_F(ReplCoordTest, PrepareOplogQueryMetadata) {
     ASSERT_EQ(oqMetadata.getValue().getLastOpApplied(), optime2);
     ASSERT_EQ(oqMetadata.getValue().getRBID(), 100);
     ASSERT_EQ(oqMetadata.getValue().getSyncSourceIndex(), -1);
-    ASSERT_EQ(oqMetadata.getValue().getPrimaryIndex(), -1);
+    ASSERT_EQ(oqMetadata.getValue().hasPrimaryIndex(), false);
 
     auto replMetadata = rpc::ReplSetMetadata::readFromMetadata(metadata);
     ASSERT_OK(replMetadata.getStatus());

src/mongo/rpc/metadata/oplog_query_metadata.h

@@ -88,11 +88,13 @@ class OplogQueryMetadata {
     }
 
     /**
-     * Returns the index of the current primary from the perspective of the sender.
-     * Returns kNoPrimary if there is no primary.
+     * True if the sender thinks there is a primary.
+     *
+     * Note: the $oplogQueryData's primaryIndex isn't safe to use, we don't know which config
+     * version it's from. All we can safely say is whether the sender thinks there's a primary.
      */
-    int getPrimaryIndex() const {
-        return _currentPrimaryIndex;
+    bool hasPrimaryIndex() const {
+        return _currentPrimaryIndex != kNoPrimary;
     }
 
     /**

src/mongo/rpc/metadata/oplog_query_metadata_test.cpp

@@ -48,6 +48,7 @@ TEST(ReplResponseMetadataTest, OplogQueryMetadataRoundtrip) {
     ASSERT_EQ(opTime1, metadata.getLastOpCommitted().opTime);
     ASSERT_EQ(committedWall, metadata.getLastOpCommitted().wallTime);
     ASSERT_EQ(opTime2, metadata.getLastOpApplied());
+    ASSERT_TRUE(metadata.hasPrimaryIndex());
 
     BSONObjBuilder builder;
     metadata.writeToMetadata(&builder).transitional_ignore();
@@ -75,8 +76,8 @@ TEST(ReplResponseMetadataTest, OplogQueryMetadataRoundtrip) {
     ASSERT_EQ(opTime2, clonedMetadata.getLastOpApplied());
     ASSERT_EQ(committedWall, clonedMetadata.getLastOpCommitted().wallTime);
     ASSERT_EQ(metadata.getRBID(), clonedMetadata.getRBID());
-    ASSERT_EQ(metadata.getPrimaryIndex(), clonedMetadata.getPrimaryIndex());
     ASSERT_EQ(metadata.getSyncSourceIndex(), clonedMetadata.getSyncSourceIndex());
+    ASSERT_TRUE(clonedMetadata.hasPrimaryIndex());
 
     BSONObjBuilder clonedBuilder;
     clonedMetadata.writeToMetadata(&clonedBuilder).transitional_ignore();
@@ -85,6 +86,14 @@ TEST(ReplResponseMetadataTest, OplogQueryMetadataRoundtrip) {
     ASSERT_BSONOBJ_EQ(expectedObj, clonedSerializedObj);
 }
 
+TEST(ReplResponseMetadataTest, OplogQueryMetadataHasPrimaryIndex) {
+    for (auto [currentPrimaryIndex, hasPrimaryIndex] :
+         std::vector<std::pair<int, bool>>{{-1, false}, {0, true}, {1, true}}) {
+        OplogQueryMetadata oqm({OpTime(), Date_t()}, OpTime(), 1, currentPrimaryIndex, -1);
+        ASSERT_EQUALS(hasPrimaryIndex, oqm.hasPrimaryIndex());
+    }
+}
+
 }  // unnamed namespace
 }  // namespace rpc
 }  // namespace mongo