From 73e020080686ca1d9c35c1313a6d38fc1d061095 Mon Sep 17 00:00:00 2001 From: Abuhaithem Date: Mon, 29 Jun 2026 20:11:07 +0300 Subject: [PATCH] SERVER-130130: fix(sharding): model delete for deferred xferMods update that removes an in-range doc When a chunk migration defers a transaction update because the post-image document key is missing from the oplog (e.g. a transaction prepared in a previous term), the donor reconciles it later in _processDeferredXferMods(). If the document is no longer present, the code assumed a later delete had already been captured by xferMods and skipped the entry. That assumption fails when the deferred update moved the document's shard key out of the chunk range before it was deleted: onDeleteOp() drops the out-of-range delete, so it is never transferred and the recipient is left with an orphaned copy of a document that no longer exists on the donor. Model a delete for the document's _id whenever its pre-image was in range, and on the recipient only decrement the persisted orphan counter when a document is actually removed, so the now-possible redundant delete stays idempotent. --- .../db/s/migration_chunk_cloner_source.cpp | 26 +++++++- .../db/s/migration_chunk_cloner_source.h | 4 ++ .../s/migration_chunk_cloner_source_test.cpp | 59 +++++++++++++++++++ .../db/s/migration_destination_manager.cpp | 22 ++++--- 4 files changed, 100 insertions(+), 11 deletions(-) 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; } }