From 6c7dde2602409e492c0012d01a397b1e6c92ca29 Mon Sep 17 00:00:00 2001 From: achamayou Date: Tue, 1 Sep 2026 09:51:11 +0100 Subject: [PATCH 1/3] Hold the map set while committing a reserved transaction CommittableTx::commit() brackets apply_changes with lock_map_set() / unlock_map_set() when the transaction creates maps, so that Store::add_dynamic_map() does not mutate the Store's map set while another thread reads it. ReservedTx::commit_reserved() applies changes the same way but never took that lock. A reserved transaction does create maps - the first signature creates the signature tables - so add_dynamic_map() could write to maps with no lock held, racing a reader holding maps_lock in Store::get_map(). ThreadSanitizer reports this as a race in Store::get_map_internal. The lock has to be taken by the caller rather than inside add_dynamic_map(), because commit() already holds it by that point and ccf::pal::Mutex is not recursive. Store::commit() holds commit_lock when it reaches commit_reserved(), so this introduces a commit_lock -> maps_lock edge. There is no reverse edge: commit_lock is acquired in exactly one place, and commit() releases the map set before calling Store::commit(). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 75d99c5d-6efa-4048-8032-8c78b97208d9 --- src/kv/committable_tx.h | 17 ++++++++++ src/kv/test/kv_test.cpp | 69 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/src/kv/committable_tx.h b/src/kv/committable_tx.h index e47877e983bd..5b3662757823 100644 --- a/src/kv/committable_tx.h +++ b/src/kv/committable_tx.h @@ -458,6 +458,17 @@ namespace ccf::kv std::vector hooks; bool track_deletes_on_missing_keys = false; + + // A reserved transaction can create maps too - the first signature + // creates the signature tables. As in commit(), hold the map set while + // applying, so that add_dynamic_map() does not mutate it underneath a + // concurrent reader. + const bool maps_created = !pimpl->created_maps.empty(); + if (maps_created) + { + this->pimpl->store->lock_map_set(); + } + auto c = apply_changes( all_changes, [this](bool) { return std::make_tuple(version, version - 1); }, @@ -466,6 +477,12 @@ namespace ccf::kv version, track_deletes_on_missing_keys, rollback_count); + + if (maps_created) + { + this->pimpl->store->unlock_map_set(); + } + success = c.has_value(); if (!success) diff --git a/src/kv/test/kv_test.cpp b/src/kv/test/kv_test.cpp index a356b968e6d1..e040285f7c64 100644 --- a/src/kv/test/kv_test.cpp +++ b/src/kv/test/kv_test.cpp @@ -18,9 +18,11 @@ #define DOCTEST_CONFIG_IMPLEMENT #include #undef FAIL +#include #include #include #include +#include #include struct MapTypes @@ -3358,6 +3360,73 @@ TEST_CASE("Range") } } +// Reproduces the race between a reserved transaction creating a map (which +// writes to the Store's map set) and a concurrent reader looking one up. The +// write happens in Store::add_dynamic_map via commit_reserved; the read holds +// maps_lock via Store::get_map. Only ThreadSanitizer can observe the failure, +// so this asserts nothing about interleaving - it exists to give TSAN both +// accesses concurrently. +TEST_CASE("Reserved transaction map creation is serialised with lookups") +{ + ccf::kv::Store store; + store.set_encryptor(std::make_shared()); + + { + auto tx = store.create_tx(); + tx.rw("public:existing")->put("k", "v"); + REQUIRE(tx.commit() == ccf::kv::CommitResult::SUCCESS); + } + + std::atomic stop{false}; + std::vector readers; + readers.reserve(4); + for (size_t r = 0; r < 4; ++r) + { + readers.emplace_back([&store, &stop]() { + size_t n = 0; + while (!stop.load(std::memory_order_relaxed)) + { + // Takes maps_lock and searches the map set. Looks up names in the + // range being inserted, so the search path traverses the nodes + // add_dynamic_map is writing. Deliberately avoids current_version(), + // so this contends only on maps_lock. + (void)store.get_map(1, fmt::format("public:reserved_{}", n % 2000)); + n++; + } + }); + } + + constexpr size_t reserved_txs = 2000; + for (size_t i = 0; i < reserved_txs; ++i) + { + // Each reserved transaction writes to a map that does not exist yet, so + // committing it adds to the Store's map set. Nothing else here may take + // maps_lock, or it would order the write against the readers and hide the + // race being reproduced. + auto tx = store.create_reserved_tx(store.next_txid()); + tx.rw(fmt::format("public:reserved_{}", i)) + ->put("k", "v"); + const auto [result, data, claims, commit_evidence, hooks] = + tx.commit_reserved(); + REQUIRE(result == ccf::kv::CommitResult::SUCCESS); + } + + stop.store(true); + for (auto& reader : readers) + { + reader.join(); + } + + // Confirm the writes really did extend the map set, so this exercises + // Store::add_dynamic_map rather than silently doing nothing. + REQUIRE( + store.get_map(store.current_version(), "public:reserved_0") != nullptr); + REQUIRE( + store.get_map( + store.current_version(), + fmt::format("public:reserved_{}", reserved_txs - 1)) != nullptr); +} + TEST_CASE("Ledger entry chunk request") { ccf::kv::Store store; From 7b76024565460403e16b054cb7029220448a2401 Mon Sep 17 00:00:00 2001 From: achamayou Date: Tue, 1 Sep 2026 16:06:49 +0100 Subject: [PATCH 2/3] Make map set locking exception-safe Use a conditional RAII guard around apply_changes in both regular and reserved transaction commits, so the map set lock is released during stack unwinding. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f8b32bd5-6fc7-4161-99f3-60cd0b9b5380 --- src/kv/committable_tx.h | 89 +++++++++++++++++++++++++---------------- 1 file changed, 54 insertions(+), 35 deletions(-) diff --git a/src/kv/committable_tx.h b/src/kv/committable_tx.h index 5b3662757823..e60666916abf 100644 --- a/src/kv/committable_tx.h +++ b/src/kv/committable_tx.h @@ -28,6 +28,35 @@ namespace ccf::kv }; protected: + class MapSetLockGuard + { + private: + AbstractStore& store; + const bool locked; + + public: + MapSetLockGuard(AbstractStore& store_, bool should_lock) : + store(store_), + locked(should_lock) + { + if (locked) + { + store.lock_map_set(); + } + } + + ~MapSetLockGuard() + { + if (locked) + { + store.unlock_map_set(); + } + } + + MapSetLockGuard(const MapSetLockGuard&) = delete; + MapSetLockGuard& operator=(const MapSetLockGuard&) = delete; + }; + bool committed = false; bool success = false; @@ -206,31 +235,26 @@ namespace ccf::kv // If this transaction creates any maps, ensure that commit gets a // consistent snapshot of the existing map set const bool maps_created = !pimpl->created_maps.empty(); - if (maps_created) - { - this->pimpl->store->lock_map_set(); - } ccf::kv::ConsensusHookPtrs hooks; std::optional new_maps_conflict_version = std::nullopt; bool track_deletes_on_missing_keys = false; - auto c = apply_changes( - all_changes, - version_resolver == nullptr ? - [&](bool has_new_map) { - return pimpl->store->next_version(has_new_map); - } : - version_resolver, - hooks, - pimpl->created_maps, - new_maps_conflict_version, - track_deletes_on_missing_keys); - - if (maps_created) - { - this->pimpl->store->unlock_map_set(); + std::optional c; + { + MapSetLockGuard map_set_guard(*pimpl->store, maps_created); + c = apply_changes( + all_changes, + version_resolver == nullptr ? + [&](bool has_new_map) { + return pimpl->store->next_version(has_new_map); + } : + version_resolver, + hooks, + pimpl->created_maps, + new_maps_conflict_version, + track_deletes_on_missing_keys); } success = c.has_value(); @@ -464,23 +488,18 @@ namespace ccf::kv // applying, so that add_dynamic_map() does not mutate it underneath a // concurrent reader. const bool maps_created = !pimpl->created_maps.empty(); - if (maps_created) - { - this->pimpl->store->lock_map_set(); - } - auto c = apply_changes( - all_changes, - [this](bool) { return std::make_tuple(version, version - 1); }, - hooks, - pimpl->created_maps, - version, - track_deletes_on_missing_keys, - rollback_count); - - if (maps_created) - { - this->pimpl->store->unlock_map_set(); + std::optional c; + { + MapSetLockGuard map_set_guard(*pimpl->store, maps_created); + c = apply_changes( + all_changes, + [this](bool) { return std::make_tuple(version, version - 1); }, + hooks, + pimpl->created_maps, + version, + track_deletes_on_missing_keys, + rollback_count); } success = c.has_value(); From 4c71b57c39f210f5d5f686baec10c0ed96a061ba Mon Sep 17 00:00:00 2001 From: achamayou Date: Tue, 1 Sep 2026 17:31:49 +0100 Subject: [PATCH 3/3] Join test readers on early exit Use jthread stop tokens so failed assertions or exceptions stop and join every reader before stack unwinding destroys the thread collection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f8b32bd5-6fc7-4161-99f3-60cd0b9b5380 --- src/kv/test/kv_test.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/kv/test/kv_test.cpp b/src/kv/test/kv_test.cpp index e040285f7c64..609318d8cbb9 100644 --- a/src/kv/test/kv_test.cpp +++ b/src/kv/test/kv_test.cpp @@ -18,9 +18,9 @@ #define DOCTEST_CONFIG_IMPLEMENT #include #undef FAIL -#include #include #include +#include #include #include #include @@ -1538,7 +1538,7 @@ TEST_CASE("foreach_key") auto tx = kv_store.create_tx(); auto handle = tx.rw(map); REQUIRE_NOTHROW(handle->foreach_key([](const std::string& k) { - REQUIRE(k.find('k') != std::string::npos); + REQUIRE(k.contains('k')); return true; })); @@ -1575,7 +1575,7 @@ TEST_CASE("foreach_value") auto tx = kv_store.create_tx(); auto handle = tx.rw(map); REQUIRE_NOTHROW(handle->foreach_value([](const std::string& v) { - REQUIRE(v.find('v') != std::string::npos); + REQUIRE(v.contains('v')); return true; })); @@ -3377,14 +3377,15 @@ TEST_CASE("Reserved transaction map creation is serialised with lookups") REQUIRE(tx.commit() == ccf::kv::CommitResult::SUCCESS); } - std::atomic stop{false}; - std::vector readers; + // Ensure every exit path, including a failed REQUIRE, stops and joins + // readers. + std::vector readers; readers.reserve(4); for (size_t r = 0; r < 4; ++r) { - readers.emplace_back([&store, &stop]() { + readers.emplace_back([&store](std::stop_token stop_token) { size_t n = 0; - while (!stop.load(std::memory_order_relaxed)) + while (!stop_token.stop_requested()) { // Takes maps_lock and searches the map set. Looks up names in the // range being inserted, so the search path traverses the nodes @@ -3411,11 +3412,11 @@ TEST_CASE("Reserved transaction map creation is serialised with lookups") REQUIRE(result == ccf::kv::CommitResult::SUCCESS); } - stop.store(true); for (auto& reader : readers) { - reader.join(); + reader.request_stop(); } + readers.clear(); // Confirm the writes really did extend the map set, so this exercises // Store::add_dynamic_map rather than silently doing nothing.