diff --git a/src/mongo/db/s/migration_chunk_cloner_source.cpp b/src/mongo/db/s/migration_chunk_cloner_source.cpp index b35f1d732efb8..aaaa1a306ff49 100644 --- a/src/mongo/db/s/migration_chunk_cloner_source.cpp +++ b/src/mongo/db/s/migration_chunk_cloner_source.cpp @@ -909,9 +909,29 @@ void MigrationChunkClonerSource::_processDeferredXferMods(OperationContext* opCt auto idElement = preImageDocKey["_id"]; BSONObj newerVersionDoc; if (!Helpers::findById(opCtx, this->nss(), BSON("_id" << idElement), newerVersionDoc)) { - // If the document can no longer be found, this means that another later op must have - // deleted it. That delete would have been captured by the xferMods so nothing else to - // do here. + // The document can no longer be found, which means that a later operation deleted it. + // + // If the document's shard key was still inside the chunk range when it was deleted, + // onDeleteOp() captured that delete in the xferMods delete buffer and there is nothing + // else to do here. However, the deferred update we are processing here may itself have + // moved the document's shard key OUT of the chunk range before the delete happened. In + // that case onDeleteOp() would have observed an out-of-range shard key and skipped the + // delete, so it was never added to the xferMods buffer. Because the document was + // transferred to the recipient while its pre-image was still in range, simply skipping + // it here would leave the recipient with an orphaned copy that no longer exists on the + // donor. + // + // To guarantee the recipient does not retain such an orphan, model the deletion + // ourselves whenever the pre-image fell inside the chunk range. Emitting a delete here + // is safe even if onDeleteOp() did capture the delete: the recipient applies deletes by + // _id and the orphan counter is only adjusted when a document is actually removed, so a + // duplicate delete is an idempotent no-op. + auto preImageShardKeyValues = + _shardKeyPattern.extractShardKeyFromDocumentKey(preImageDocKey); + if (!preImageShardKeyValues.isEmpty() && + isKeyInRange(preImageShardKeyValues, getMin(), getMax())) { + _addToTransferModsQueue(idElement.wrap(), 'd', {}); + } continue; } diff --git a/src/mongo/db/s/migration_chunk_cloner_source.h b/src/mongo/db/s/migration_chunk_cloner_source.h index dcc82c222a86f..ad2c382f1e91e 100644 --- a/src/mongo/db/s/migration_chunk_cloner_source.h +++ b/src/mongo/db/s/migration_chunk_cloner_source.h @@ -420,6 +420,10 @@ class MigrationChunkClonerSource { friend class LogTransactionOperationsForShardingHandler; friend class LogBatchedWriteForSessionMigrationHandler; + // Allows the unit test to exercise the deferred-xferMods reconciliation path, which is + // otherwise only reachable through the transaction op-observer handler. + friend class MigrationChunkClonerSourceTest; + using RecordIdSet = std::set; /** diff --git a/src/mongo/db/s/migration_chunk_cloner_source_test.cpp b/src/mongo/db/s/migration_chunk_cloner_source_test.cpp index 70ec18bde2f66..0d99675a1b676 100644 --- a/src/mongo/db/s/migration_chunk_cloner_source_test.cpp +++ b/src/mongo/db/s/migration_chunk_cloner_source_test.cpp @@ -1600,6 +1600,65 @@ TEST_F(MigrationChunkClonerSourceTest, UpdatedDocumentsFetched) { cloner.cancelClone(operationContext()); } +// Regression test for the deferred-xferMods reconciliation path. When a transaction's update is +// processed without a post-image document key (e.g. a transaction prepared in a previous term), the +// cloner defers the work and only records the pre-image document key. Later, when nextModsBatch() +// reconciles the deferred entry, the document may already be gone because a subsequent update moved +// its shard key out of the chunk range and it was then deleted. That out-of-range delete is skipped +// by onDeleteOp(), so unless the deferred reconciliation models the delete itself, the recipient is +// left with an orphaned copy of the document that was transferred while its pre-image was in range. +TEST_F(MigrationChunkClonerSourceTest, DeferredUpdateForRemovedInRangeDocModelsDelete) { + const ShardKeyPattern shardKeyPattern(kShardKeyPattern); + + const ShardsvrMoveRange req = + createMoveRangeRequest(ChunkRange(BSON("X" << 100), BSON("X" << 200))); + MigrationChunkClonerSource cloner(operationContext(), + req, + WriteConcernOptions(), + kShardKeyPattern, + kDonorConnStr, + kRecipientConnStr.getServers()[0]); + + // Materialize the collection with an unrelated in-range document so that findById() lookups + // resolve against an existing collection. This document is never queued for cloning and is not + // part of any deferred entry, so it must not appear in the transferred mods. + insertDocsInShardedCollection({createCollectionDocument(175)}); + + // Defer reconciliation for a document whose pre-image is inside the chunk range, but which no + // longer exists in the collection (it was moved out of range and subsequently deleted). The + // document key carries both the shard key and the _id, matching what the op-observer records. + cloner._deferProcessingForXferMod(createCollectionDocument(150)); + + // Also defer a document whose pre-image is *outside* the chunk range. The recipient never + // received this document, so no delete should be modeled for it. + cloner._deferProcessingForXferMod(createCollectionDocument(90)); + + { + const auto collection = acquireCollection(operationContext(), kNss, MODE_IS); + + { + BSONArrayBuilder arrBuilder; + ASSERT_OK(cloner.nextCloneBatch(operationContext(), collection, &arrBuilder)); + ASSERT_EQ(0, arrBuilder.arrSize()); + } + + { + BSONObjBuilder modsBuilder; + ASSERT_OK(cloner.nextModsBatch(operationContext(), &modsBuilder)); + + const auto modsObj = modsBuilder.obj(); + ASSERT_EQ(0U, modsObj["reload"].Array().size()); + + // Only the in-range pre-image is reconciled into a delete; the out-of-range pre-image is + // ignored because the recipient never held that document. + ASSERT_EQ(1U, modsObj["deleted"].Array().size()); + ASSERT_BSONOBJ_EQ(BSON("_id" << 150), modsObj["deleted"].Array()[0].Obj()); + } + } + + cloner.cancelClone(operationContext()); +} + TEST_F(MigrationChunkClonerSourceTest, UpdatedDocumentsFetchedWithHashedShardKey) { const ShardKeyPattern shardKeyPattern(BSON("X" << "hashed")); diff --git a/src/mongo/db/s/migration_destination_manager.cpp b/src/mongo/db/s/migration_destination_manager.cpp index 73ea30d850fc9..8a12f8fbc5d6d 100644 --- a/src/mongo/db/s/migration_destination_manager.cpp +++ b/src/mongo/db/s/migration_destination_manager.cpp @@ -2089,16 +2089,22 @@ bool MigrationDestinationManager::_applyMigrateOp(OperationContext* opCtx, const } } - writeConflictRetry(opCtx, "transferModsDeletes", _nss, [&] { - deleteObjects(opCtx, - collection, - id, - true /* justOne */, - false /* god */, - true /* fromMigrate */); + const auto numDeleted = writeConflictRetry(opCtx, "transferModsDeletes", _nss, [&] { + return deleteObjects(opCtx, + collection, + id, + true /* justOne */, + false /* god */, + true /* fromMigrate */); }); - changeInOrphans--; + // Only adjust the orphan counter when a document was actually removed. The donor may + // legitimately send a delete for an _id that is no longer present on the recipient (for + // example a delete that is redundant with a deferred-update reconciliation), and + // decrementing for a no-op delete would corrupt the persisted orphan count. + if (numDeleted > 0) { + changeInOrphans--; + } didAnything = true; } }