SERVER-40177 Enforce prepare conflicts on secondaries

4 files changed, 34 insertions, 21 deletions

diff --git a/jstests/core/txns/do_txn_basic.js b/jstests/core/txns/do_txn_basic.js
index de753f787dc..1325db840b5 100644
--- a/jstests/core/txns/do_txn_basic.js
+++ b/jstests/core/txns/do_txn_basic.js
@@ -209,12 +209,15 @@
     assert.eq(true, a.results[0], "Bad result value for valid insert");
 
     jsTestLog("Duplicate insert");
-    a = assert.commandWorked(db.adminCommand({
+    a = assert.commandFailedWithCode(db.adminCommand({
         doTxn: [{"op": "i", "ns": t.getFullName(), "o": {_id: 5, x: 17}}],
         txnNumber: NumberLong(txnNumber++)
-    }));
-    assert.eq(1, t.find().count(), "Duplicate insert failed");
-    assert.eq(true, a.results[0], "Bad result value for duplicate insert");
+    }),
+                                     ErrorCodes.DuplicateKey);
+    assert.eq(1,
+              t.find().count(),
+              "The number of documents changed despite the duplicate insert failing");
+    assert.eq(false, a.results[0], "Bad result value for duplicate insert");
 
     var o = {_id: 5, x: 17};
     assert.eq(o, t.findOne(), "Mismatching document inserted.");
diff --git a/src/mongo/db/repl/oplog.cpp b/src/mongo/db/repl/oplog.cpp
index 9e7572e1fc1..658353d6cad 100644
--- a/src/mongo/db/repl/oplog.cpp
+++ b/src/mongo/db/repl/oplog.cpp
@@ -1531,15 +1531,27 @@ Status applyOperation_inlock(OperationContext* opCtx,
                     str::stream() << "Failed to apply insert due to missing _id: " << op.toString(),
                     o.hasField("_id"));
 
-            // 1. Try insert first, if we have no wrappingWriteUnitOfWork
-            // 2. If okay, commit
-            // 3. If not, do upsert (and commit)
-            // 4. If both !Ok, return status
-
-            // We cannot rely on a DuplicateKey error if we're part of a larger transaction,
-            // because that would require the transaction to abort. So instead, use upsert in that
-            // case.
-            bool needToDoUpsert = haveWrappingWriteUnitOfWork;
+            // 1. Insert if
+            //   a) we do not have a wrapping WriteUnitOfWork, which implies we are not part of an
+            //      "applyOps" command, OR
+            //   b) we are part of a multi-document transaction[1].
+            //
+            // 2. Upsert[2] if
+            //   a) we have a wrapping WriteUnitOfWork AND we are not part of a transaction, which
+            //      implies we are part of an "applyOps" command, OR
+            //   b) the previous insert failed with a DuplicateKey error AND we are not part of a
+            //      transaction.
+            //
+            // [1] Transactions should not convert inserts to upserts because on secondaries they
+            //     will perform a lookup that never occurred on the primary. This may cause an
+            //     unintended prepare conflict and block replication. For this reason, transactions
+            //     should always fail with DuplicateKey errors and never retry inserts as upserts.
+            // [2] This upsert behavior exists to support idempotency guarantees outside
+            //     steady-state replication and existing users of applyOps.
+
+            const auto txnParticipant = TransactionParticipant::get(opCtx);
+            const bool inTxn = txnParticipant && txnParticipant.inMultiDocumentTransaction();
+            bool needToDoUpsert = haveWrappingWriteUnitOfWork && !inTxn;
 
             Timestamp timestamp;
             long long term = OpTime::kUninitializedTerm;
@@ -1573,6 +1585,12 @@ Status applyOperation_inlock(OperationContext* opCtx,
                 if (status.isOK()) {
                     wuow.commit();
                 } else if (status == ErrorCodes::DuplicateKey) {
+                    // Transactions cannot be retried as upserts once they fail with a duplicate key
+                    // error.
+                    if (inTxn) {
+                        return status;
+                    }
+                    // Continue to the next block to retry the operation as an upsert.
                     needToDoUpsert = true;
                 } else {
                     return status;
diff --git a/src/mongo/db/repl/sync_tail.cpp b/src/mongo/db/repl/sync_tail.cpp
index 2f3269450c0..ac6331d02f7 100644
--- a/src/mongo/db/repl/sync_tail.cpp
+++ b/src/mongo/db/repl/sync_tail.cpp
@@ -1022,10 +1022,6 @@ Status multiSyncApply(OperationContext* opCtx,
     // Explicitly start future read transactions without a timestamp.
     opCtx->recoveryUnit()->setTimestampReadSource(RecoveryUnit::ReadSource::kNoTimestamp);
 
-    // TODO: SERVER-40177 This should be removed once it is guaranteed operations applied on
-    // secondaries cannot encounter unnecessary prepare conflicts.
-    opCtx->recoveryUnit()->setIgnorePrepared(true);
-
     ApplierHelpers::stableSortByNamespace(ops);
 
     // Assume we are recovering if oplog writes are disabled in the options.
diff --git a/src/mongo/db/repl/transaction_oplog_application.cpp b/src/mongo/db/repl/transaction_oplog_application.cpp
index 221971de7da..06d54d4f038 100644
--- a/src/mongo/db/repl/transaction_oplog_application.cpp
+++ b/src/mongo/db/repl/transaction_oplog_application.cpp
@@ -448,10 +448,6 @@ void reconstructPreparedTransactions(OperationContext* opCtx, repl::OplogApplica
             // Snapshot transaction can never conflict with the PBWM lock.
             newOpCtx->lockState()->setShouldConflictWithSecondaryBatchApplication(false);
 
-            // TODO: SERVER-40177 This should be removed once it is guaranteed operations applied on
-            // recovering nodes cannot encounter unnecessary prepare conflicts.
-            newOpCtx->recoveryUnit()->setIgnorePrepared(true);
-
             // Checks out the session, applies the operations and prepares the transaction.
             uassertStatusOK(
                 applyRecoveredPrepareTransaction(newOpCtx.get(), prepareOplogEntry, mode));