diff --git a/fdbclient/KeyRangeMap.cpp b/fdbclient/KeyRangeMap.cpp index a74c2c7027..9a125473c7 100644 --- a/fdbclient/KeyRangeMap.cpp +++ b/fdbclient/KeyRangeMap.cpp @@ -213,9 +213,21 @@ Future krmSetRange(Reference tr, Key mapPrefix, tr->set(withPrefix.end, oldValue); } -// Sets a range of keys in a key range map, coalescing with adjacent regions if the values match -// Ranges outside of maxRange will not be coalesced -// CAUTION: use care when attempting to coalesce multiple ranges in the same prefix in a single transaction +// Sets a range of keys in a key range map, coalescing with adjacent regions if the values match. +// Ranges outside of maxRange will not be coalesced. +// +// REQUIREMENTS FOR CALLERS: +// - Multiple calls on the SAME prefix within a single transaction MUST be executed sequentially +// (co_await each one before starting the next). They MUST NOT be launched concurrently via +// waitForAll or similar. Each call does snapshot reads to determine coalescing boundaries and +// then writes clears/sets that extend beyond the requested range. Concurrent calls on the same +// prefix see the same snapshot state, compute overlapping clear ranges, and corrupt each other's +// boundary writes — producing adjacent same-value entries (fragmentation) that violate KRM +// invariants and crash downstream code. +// - Multiple calls on non-overlapping prefixes within a single transaction are safe to run +// concurrently because their reads and writes are disjoint. +// - The transaction MUST be a ReadYourWritesTransaction if sequential calls on the same prefix +// need each call to see the prior call's writes for correct coalescing. template static Future krmSetRangeCoalescing_(Transaction* tr, Key mapPrefix, diff --git a/fdbserver/core/MoveKeys.cpp b/fdbserver/core/MoveKeys.cpp index 4d0379c1e3..c6eb19c1dd 100644 --- a/fdbserver/core/MoveKeys.cpp +++ b/fdbserver/core/MoveKeys.cpp @@ -784,28 +784,6 @@ Future cleanUpSingleShardDataMove(Database occ, TraceEvent(SevInfo, "CleanUpSingleShardDataMoveEnd", dataMoveId).detail("Range", keys); } -Future removeOldDestinations(Reference tr, - UID oldDest, - VectorRef shards, - KeyRangeRef currentKeys) { - KeyRef beginKey = currentKeys.begin; - - std::vector> actors; - for (int i = 0; i < shards.size(); i++) { - if (beginKey < shards[i].begin) - actors.push_back(krmSetRangeCoalescing( - tr, serverKeysPrefixFor(oldDest), KeyRangeRef(beginKey, shards[i].begin), allKeys, serverKeysFalse)); - - beginKey = shards[i].end; - } - - if (beginKey < currentKeys.end) - actors.push_back(krmSetRangeCoalescing( - tr, serverKeysPrefixFor(oldDest), KeyRangeRef(beginKey, currentKeys.end), allKeys, serverKeysFalse)); - - return waitForAll(actors); -} - Future> addReadWriteDestinations(KeyRangeRef shard, std::vector srcInterfs, std::vector destInterfs, @@ -1124,15 +1102,14 @@ static Future startMoveKeys(Database occ, std::set::iterator oldDest; - // Remove old dests from serverKeys. In order for krmSetRangeCoalescing to work correctly in the - // same prefix for a single transaction, we must do most of the coalescing ourselves. Only the - // shards on the boundary of currentRange are actually coalesced with the ranges outside of - // currentRange. For all shards internal to currentRange, we overwrite all consecutive keys whose - // value is or should be serverKeysFalse in a single write + // Remove old dests from serverKeys. Each removeOldDestinations call + // executes its krmSetRangeCoalescing calls sequentially so that each + // sees the prior call's writes through the RYW transaction. std::vector> actors; for (oldDest = oldDests.begin(); oldDest != oldDests.end(); ++oldDest) if (std::find(servers.begin(), servers.end(), *oldDest) == servers.end()) - actors.push_back(removeOldDestinations(tr, *oldDest, shardMap[*oldDest], currentKeys)); + actors.push_back(removeOldDestinations( + tr, serverKeysPrefixFor(*oldDest), shardMap[*oldDest], currentKeys)); // Update serverKeys to include keys (or the currently processed subset of keys) for each SS in // servers @@ -3524,3 +3501,43 @@ TEST_CASE("/fdbserver/MoveKeys/finishMoveKeysBackoff") { return Void(); } + +Future removeOldDestinations(Reference tr, + Key prefix, + VectorRef shards, + KeyRangeRef currentKeys) { + KeyRef beginKey = currentKeys.begin; + + for (int i = 0; i < shards.size(); i++) { + if (beginKey < shards[i].begin) { + co_await krmSetRangeCoalescing( + tr, prefix, KeyRangeRef(beginKey, shards[i].begin), allKeys, serverKeysFalse); + } + + beginKey = shards[i].end; + } + + if (beginKey < currentKeys.end) { + co_await krmSetRangeCoalescing(tr, prefix, KeyRangeRef(beginKey, currentKeys.end), allKeys, serverKeysFalse); + } + +#if DO_NOT_ENABLE_THIS_EXCEPT_TO_REPRODUCE_BUG_IN_TESTING + // BAD OLD LOGIC: fdbserver/workloads/KRMCoalescingFragmentation.cpp will + // generate improperly coalesced KRM entries if you comment out the above + // and uncomment this version. + std::vector> actors; + for (int i = 0; i < shards.size(); i++) { + if (beginKey < shards[i].begin) + actors.push_back( + krmSetRangeCoalescing(tr, prefix, KeyRangeRef(beginKey, shards[i].begin), allKeys, serverKeysFalse)); + + beginKey = shards[i].end; + } + + if (beginKey < currentKeys.end) + actors.push_back( + krmSetRangeCoalescing(tr, prefix, KeyRangeRef(beginKey, currentKeys.end), allKeys, serverKeysFalse)); + + co_await waitForAll(actors); +#endif +} diff --git a/fdbserver/core/include/fdbserver/core/MoveKeys.h b/fdbserver/core/include/fdbserver/core/MoveKeys.h index f9b9c5bf93..9c95a081a5 100644 --- a/fdbserver/core/include/fdbserver/core/MoveKeys.h +++ b/fdbserver/core/include/fdbserver/core/MoveKeys.h @@ -198,4 +198,9 @@ Future checkMoveKeysLock(Transaction* tr, const DDEnabledState* ddEnabledState, bool isWrite = true); +Future removeOldDestinations(Reference tr, + Key prefix, + VectorRef shards, + KeyRangeRef currentKeys); + #endif diff --git a/fdbserver/workloads/KRMCoalescingFragmentation.cpp b/fdbserver/workloads/KRMCoalescingFragmentation.cpp new file mode 100644 index 0000000000..1dafd3bb9e --- /dev/null +++ b/fdbserver/workloads/KRMCoalescingFragmentation.cpp @@ -0,0 +1,219 @@ +/* + * KRMCoalescingFragmentation.cpp + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2013-2026 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "fdbclient/KeyRangeMap.h" +#include "fdbclient/NativeAPI.actor.h" +#include "fdbclient/ReadYourWrites.h" +#include "fdbclient/SystemData.h" +#include "fdbserver/core/MoveKeys.h" +#include "fdbserver/tester/workloads.h" + +// Regression test for the krmSetRangeCoalescing fragmentation bug in +// removeOldDestinations() (MoveKeys.cpp). +// +// Calls the real removeOldDestinations with a test-only KRM prefix (in user +// keyspace, not system keyspace) so the test exercises the actual production +// code path without disturbing any system metadata. +// +// On UNFIXED code (concurrent waitForAll): produces adjacent same-value entries. +// On FIXED code (sequential co_await): produces a correctly coalesced map. + +struct KRMCoalescingFragmentationWorkload : TestWorkload { + static constexpr auto NAME = "KRMCoalescingFragmentation"; + + Key testPrefix; + bool success; + + explicit KRMCoalescingFragmentationWorkload(WorkloadContext const& wcx) + : TestWorkload(wcx), testPrefix("KRMFragTest/"_sr), success(false) {} + + Future setup(Database const& cx) override { return Void(); } + + Future start(Database const& cx) override { + if (clientId != 0) + return Void(); + return runTest(cx); + } + + Future check(Database const& cx) override { + if (clientId != 0) + return true; + return success; + } + + void getMetrics(std::vector& m) override {} + +private: + Future runTest(Database cx) { + // Step 1: Establish initial KRM state with alternating values. + // "" -> "1" "d" -> "" "j" -> "1" "\xff\xff" -> "" + { + auto tr = makeReference(cx); + loop { + Error err; + try { + co_await krmSetRange(tr, testPrefix, allKeys, serverKeysTrue); + co_await tr->commit(); + break; + } catch (Error& e) { + err = e; + } + co_await tr->onError(err); + } + } + { + auto tr = makeReference(cx); + loop { + Error err; + try { + co_await krmSetRange(tr, testPrefix, KeyRangeRef("d"_sr, "j"_sr), serverKeysFalse); + co_await tr->commit(); + break; + } catch (Error& e) { + err = e; + } + co_await tr->onError(err); + } + } + + // Verify setup. + { + auto tr = makeReference(cx); + loop { + Error err; + try { + RangeResult result = co_await krmGetRanges(tr, testPrefix, allKeys); + if (result.back().key < allKeys.end) { + tr->reset(); + continue; + } + TraceEvent evt("KRMFragTestSetupVerify"); + evt.detail("NumEntries", result.size()); + for (int i = 0; i < result.size(); i++) { + evt.detail(format("Key%d", i), result[i].key); + evt.detail(format("Val%d", i), result[i].value); + } + ASSERT(result.size() == 4); + ASSERT(result[0].value == serverKeysTrue); + ASSERT(result[1].key == "d"_sr && result[1].value == serverKeysFalse); + ASSERT(result[2].key == "j"_sr && result[2].value == serverKeysTrue); + TraceEvent("KRMFragTestSetupDone").detail("Entries", result.size()); + break; + } catch (Error& e) { + err = e; + } + co_await tr->onError(err); + } + } + + // Step 2: Call removeOldDestinations with the multi-gap pattern. + // Shards to keep: ["a","c"), ["e","g"), ["k","m") + // Gaps to clear: ["c","e") and ["g","k") + // currentKeys: ["a","m") + // + // The existing "" entry at "d" causes both gap clears to extend their + // coalescing through it, producing overlapping clears on unfixed code. + { + Arena arena; + VectorRef shards; + shards.push_back(arena, KeyRangeRef("a"_sr, "c"_sr)); + shards.push_back(arena, KeyRangeRef("e"_sr, "g"_sr)); + shards.push_back(arena, KeyRangeRef("k"_sr, "m"_sr)); + KeyRangeRef currentKeys("a"_sr, "m"_sr); + + auto tr = makeReference(cx); + loop { + Error err; + try { + co_await removeOldDestinations(tr, testPrefix, shards, currentKeys); + co_await tr->commit(); + break; + } catch (Error& e) { + err = e; + } + co_await tr->onError(err); + } + } + + // Step 3: Read back and verify the exact expected KRM state. + // + // Sequential coalescing should produce: + // "" -> "1" (server owns ["", "c")) + // "c" -> "" (gaps ["c","e") and ["g","k") coalesced with existing "" region ["d","j")) + // "k" -> "1" (server owns ["k", ...)) + // + // + // Gap 1 ["c","e") coalesces right through "d"->"" to "j", then Gap 2 ["g","k") + // coalesces left through the now-extended "" region back to "c". + { + auto tr = makeReference(cx); + loop { + Error err; + try { + RangeResult result = co_await krmGetRanges(tr, testPrefix, allKeys); + if (result.back().key < allKeys.end) { + tr->reset(); + continue; + } + + TraceEvent evt("KRMFragTestResult"); + evt.detail("NumEntries", result.size()); + for (int i = 0; i < result.size(); i++) { + evt.detail(format("Key%d", i), result[i].key); + evt.detail(format("Val%d", i), result[i].value); + } + + bool fragmented = false; + for (int i = 1; i < (int)result.size() - 1; i++) { + if (result[i].value == result[i - 1].value) { + fragmented = true; + TraceEvent(SevError, "KRMFragTestFragmentationDetected") + .detail("Key1", result[i - 1].key) + .detail("Key2", result[i].key) + .detail("Value", result[i].value); + } + } + + if (fragmented) { + TraceEvent(SevError, "KRMFragTestFailed") + .detail("Message", "removeOldDestinations produced fragmented KRM entries"); + break; + } + + // Verify the exact expected transitions: the gaps were cleared and + // coalesced, and the kept-shard boundaries are correct. + ASSERT_EQ(result.size(), 4); + ASSERT(result[0].key == ""_sr && result[0].value == serverKeysTrue); + ASSERT(result[1].key == "c"_sr && result[1].value == serverKeysFalse); + ASSERT(result[2].key == "k"_sr && result[2].value == serverKeysTrue); + + success = true; + TraceEvent("KRMFragTestPassed").detail("Entries", result.size()); + break; + } catch (Error& e) { + err = e; + } + co_await tr->onError(err); + } + } + } +}; + +WorkloadFactory KRMCoalescingFragmentationWorkloadFactory; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index f0c44c528c..7915120cee 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -176,6 +176,7 @@ if(WITH_PYTHON) add_fdb_test(TEST_FILES fast/IncrementTest.toml) add_fdb_test(TEST_FILES fast/InventoryTestAlmostReadOnly.toml) add_fdb_test(TEST_FILES fast/InventoryTestSomeWrites.toml) + add_fdb_test(TEST_FILES fast/KRMCoalescingFragmentation.toml) add_fdb_test(TEST_FILES fast/LocalRatekeeper.toml) add_fdb_test(TEST_FILES fast/LongStackWriteDuringRead.toml) add_fdb_test(TEST_FILES fast/LowLatency.toml) diff --git a/tests/fast/KRMCoalescingFragmentation.toml b/tests/fast/KRMCoalescingFragmentation.toml new file mode 100644 index 0000000000..c381829fbd --- /dev/null +++ b/tests/fast/KRMCoalescingFragmentation.toml @@ -0,0 +1,5 @@ +[[test]] +testTitle = 'KRMCoalescingFragmentation' + + [[test.workload]] + testName = 'KRMCoalescingFragmentation'