From 2d7be38352b4056ba790ce4f736fb768545523cd Mon Sep 17 00:00:00 2001 From: achamayou Date: Sat, 5 Sep 2026 22:54:46 +0100 Subject: [PATCH 1/3] Add KV unit test reproducing stale pending tx (#8293) A transaction which takes its version before a view change, but reaches Store::commit() after it, is not caught by the stale-view check, because that check is also gated on the node still being primary. Its entry is parked in pending_txs behind the hole the rollback left, survives the next election because a rollback which discards nothing returns early without clearing pending_txs, and is then replicated by the first transaction of the new term. The new case reproduces this against a real ccf::kv::Store and the existing stub consensus, using a write set observer to drive the view change at the point another thread would be parked mid-commit. Its assertions record the current behaviour, marked BUG where incorrect. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/kv/test/kv_test.cpp | 120 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/src/kv/test/kv_test.cpp b/src/kv/test/kv_test.cpp index 1c429b543da..22f126f126e 100644 --- a/src/kv/test/kv_test.cpp +++ b/src/kv/test/kv_test.cpp @@ -3048,6 +3048,126 @@ TEST_CASE("Stale-view writes are rejected before local application") REQUIRE(fresh_dynamic_map_tx.commit() == ccf::kv::CommitResult::SUCCESS); } +// Reproduces https://github.com/microsoft/CCF/issues/8293. +// +// The stale-view check above only covers transactions which take their version +// after the view change. A transaction which takes its version before the view +// change, but reaches Store::commit() after it, is not rejected, because that +// check is additionally gated on the node still being primary. Its entry is +// parked in pending_txs behind the hole the rollback left, survives the next +// election, and is then replicated by the first transaction of the new term - +// even though its writes were discarded and are no longer in the store. +// +// In production the two transactions which produce the hole are concurrent, and +// this one is held mid-commit by another thread. A write set observer runs at +// exactly the point that thread would be parked - after the version has been +// allocated and the writes applied locally, but before Store::commit() - so it +// is used here to drive the view change deterministically, without threads. +// +// The CHECKs marked BUG record the current, incorrect behaviour, so that the +// divergence is explicit. Fixing #8293 should flip them. +TEST_CASE("Stale-view writes which took their version early are not rejected") +{ + ccf::kv::Store store; + store.set_encryptor(std::make_shared()); + auto consensus = std::make_shared(); + consensus->state = ccf::kv::test::StubConsensus::Primary; + store.set_consensus(consensus); + + constexpr ccf::kv::Term initial_term = 2; + constexpr ccf::kv::Term new_term = initial_term + 1; + constexpr ccf::SeqNo committed_seqno = 2; + MapTypes::StringString map("public:map"); + store.initialise_term(initial_term); + + auto write = [&](const std::string& key, const std::string& value) { + auto tx = store.create_tx(); + tx.rw(map)->put(key, value); + return tx.commit(); + }; + + auto read = [&](const std::string& key) { + auto tx = store.create_read_only_tx(); + return tx.ro(map)->get(key); + }; + + // Seqno of the last entry consensus has been given + auto replicated_to = [&]() -> ccf::SeqNo { + return consensus->replica.empty() ? 0 : + std::get<0>(consensus->replica.back()); + }; + + INFO("Two committed entries, and one which is replicated but not committed"); + { + REQUIRE(write("first", "1") == ccf::kv::CommitResult::SUCCESS); + REQUIRE(write("second", "2") == ccf::kv::CommitResult::SUCCESS); + REQUIRE(write("truncated", "3") == ccf::kv::CommitResult::SUCCESS); + REQUIRE(store.current_version() == 3); + REQUIRE(replicated_to() == 3); + } + + INFO("A write takes seqno 4, then loses the view before Store::commit()"); + { + auto stale_tx = store.create_tx(); + stale_tx.rw(map)->put("stale", "4"); + + auto lose_view = [&](const ccf::crypto::Sha256Hash&, const std::string&) { + // The node hears from the new primary, steps down, and truncates its + // uncommitted suffix - discarding seqno 3, and this transaction's own + // writes at seqno 4. Seqno 3 is now a hole. + consensus->state = ccf::kv::test::StubConsensus::Backup; + consensus->replica.resize(committed_seqno); + store.rollback({initial_term, committed_seqno}, new_term); + }; + + // BUG (#8293): this transaction's writes have been discarded, and it is + // committing in a term which is no longer current, but it reports success + CHECK( + stale_tx.commit(ccf::empty_claims(), lose_view) == + ccf::kv::CommitResult::SUCCESS); + + CHECK(store.current_txid() == ccf::TxID(initial_term, committed_seqno)); + CHECK(!read("stale").has_value()); + CHECK(!read("truncated").has_value()); + CHECK(replicated_to() == committed_seqno); + } + + INFO("The node wins the next election"); + { + consensus->state = ccf::kv::test::StubConsensus::Primary; + // aft::Aft::become_leader() rolls back to the last committable index, which + // is at or above the store's current version. Such a rollback discards + // nothing, so it returns early - without clearing pending_txs, which still + // holds the parked entry at seqno 4. + store.rollback({new_term, committed_seqno}, new_term); + } + + INFO("The first write of the new term drags the discarded write with it"); + { + REQUIRE(write("fresh", "3") == ccf::kv::CommitResult::SUCCESS); + CHECK(read("fresh") == "3"); + CHECK(store.current_version() == 3); + + // BUG (#8293): seqno 3 completed the batch, so the parked entry at seqno 4 + // was replicated too, carrying writes this store has already discarded and + // which no transaction on this node ever observed + CHECK(replicated_to() == 4); + CHECK(store.current_version() < replicated_to()); + } + + INFO("The store can no longer replicate anything"); + { + // The store's next version is 4, but consensus has already been given an + // entry at seqno 4, so no batch this store builds from here is ever + // contiguous with what has been replicated + REQUIRE(write("lost", "4") == ccf::kv::CommitResult::SUCCESS); + CHECK(read("lost") == "4"); + + // BUG (#8293): reported as committed, but never handed to consensus + CHECK(replicated_to() == 4); + } +} + TEST_CASE("Reported TxID after commit") { ccf::kv::Store kv_store; From 899aaa1b0dd017d9f43cd7997eda551dc382ba23 Mon Sep 17 00:00:00 2001 From: achamayou Date: Sun, 6 Sep 2026 07:14:49 +0100 Subject: [PATCH 2/3] Assert correct behaviour, so the repro fails The test previously recorded the current behaviour and passed. Flip the expectations to what the store should do, so it is a failing repro of #8293 until the defect is fixed. Also count the entries handed to consensus, rather than only the seqno reached. The lost follow-up transaction is invisible to the seqno alone, because the entry already sitting at that seqno is the discarded write. Five divergences are now reported: - the stale write returns SUCCESS rather than FAIL_NO_REPLICATE - the new term's first write replicates two entries rather than one - it reaches seqno 4 rather than 3 - the store version trails what has been replicated - the write after it is never handed to consensus Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/kv/test/kv_test.cpp | 49 ++++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/src/kv/test/kv_test.cpp b/src/kv/test/kv_test.cpp index 22f126f126e..704629c3af4 100644 --- a/src/kv/test/kv_test.cpp +++ b/src/kv/test/kv_test.cpp @@ -3064,9 +3064,10 @@ TEST_CASE("Stale-view writes are rejected before local application") // allocated and the writes applied locally, but before Store::commit() - so it // is used here to drive the view change deterministically, without threads. // -// The CHECKs marked BUG record the current, incorrect behaviour, so that the -// divergence is explicit. Fixing #8293 should flip them. -TEST_CASE("Stale-view writes which took their version early are not rejected") +// This asserts the behaviour the store should have, so it fails until #8293 is +// fixed. Each expectation which does not currently hold is marked FAILS TODAY, +// with the behaviour actually observed. +TEST_CASE("Stale-view writes which took their version early are rejected") { ccf::kv::Store store; store.set_encryptor(std::make_shared()); @@ -3120,11 +3121,13 @@ TEST_CASE("Stale-view writes which took their version early are not rejected") store.rollback({initial_term, committed_seqno}, new_term); }; - // BUG (#8293): this transaction's writes have been discarded, and it is - // committing in a term which is no longer current, but it reports success + // This transaction's writes have been discarded, and it is committing in a + // term which is no longer current, so it must not report success. + // FAILS TODAY: returns SUCCESS, and parks an entry at seqno 4 in + // pending_txs, behind the hole the rollback left at seqno 3 CHECK( stale_tx.commit(ccf::empty_claims(), lose_view) == - ccf::kv::CommitResult::SUCCESS); + ccf::kv::CommitResult::FAIL_NO_REPLICATE); CHECK(store.current_txid() == ccf::TxID(initial_term, committed_seqno)); CHECK(!read("stale").has_value()); @@ -3142,29 +3145,35 @@ TEST_CASE("Stale-view writes which took their version early are not rejected") store.rollback({new_term, committed_seqno}, new_term); } - INFO("The first write of the new term drags the discarded write with it"); + INFO("The first write of the new term replicates only itself"); { + const auto replicated_before = consensus->replica.size(); REQUIRE(write("fresh", "3") == ccf::kv::CommitResult::SUCCESS); CHECK(read("fresh") == "3"); CHECK(store.current_version() == 3); - // BUG (#8293): seqno 3 completed the batch, so the parked entry at seqno 4 - // was replicated too, carrying writes this store has already discarded and - // which no transaction on this node ever observed - CHECK(replicated_to() == 4); - CHECK(store.current_version() < replicated_to()); + // FAILS TODAY: seqno 3 completes the batch, so the parked entry at seqno 4 + // is replicated too, carrying writes this store has already discarded and + // which no transaction on this node ever observed. Two entries are handed + // to consensus, and replicated_to() reaches 4 + CHECK(consensus->replica.size() == replicated_before + 1); + CHECK(replicated_to() == 3); + CHECK(store.current_version() == replicated_to()); } - INFO("The store can no longer replicate anything"); + INFO("Later writes continue to be replicated"); { - // The store's next version is 4, but consensus has already been given an - // entry at seqno 4, so no batch this store builds from here is ever - // contiguous with what has been replicated - REQUIRE(write("lost", "4") == ccf::kv::CommitResult::SUCCESS); - CHECK(read("lost") == "4"); + const auto replicated_before = consensus->replica.size(); + REQUIRE(write("next", "4") == ccf::kv::CommitResult::SUCCESS); + CHECK(read("next") == "4"); - // BUG (#8293): reported as committed, but never handed to consensus - CHECK(replicated_to() == 4); + // FAILS TODAY: last_replicated is now ahead of version, so no batch this + // store builds is contiguous with what has been replicated. This write, and + // every write after it, reports success but is never handed to consensus. + // Note that replicated_to() alone cannot see this, because the entry + // already sitting at seqno 4 is the discarded write, not this one + CHECK(consensus->replica.size() == replicated_before + 1); + CHECK(store.current_version() == replicated_to()); } } From 7260076ef0badfa4c6fcda06da968dbb1e52f4c3 Mon Sep 17 00:00:00 2001 From: Amaury Chamayou Date: Sun, 6 Sep 2026 22:24:26 +0100 Subject: [PATCH 3/3] Reject stale-view commits after stepping down Reject transactions whose view no longer matches the store before they enter the pending replication queue, regardless of leadership state. Extend the regression to cover all leadership states and sequence-number reuse after rollback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 1 + src/kv/store.h | 5 +- src/kv/test/kv_test.cpp | 204 ++++++++++++++++++---------------------- 3 files changed, 98 insertions(+), 112 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d48ef750fe5..8c32d01027f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed +- Transactions from an earlier view are now rejected before entering the replication queue even after the node has stepped down. This prevents rolled-back writes from being replicated after a later election and blocking subsequent replication (#8293, #8295). - A signature transaction decided whether to end a ledger chunk, and recorded that decision on the chunker, outside the version lock. A rollback landing in that window discarded the signature but left the chunk marker behind. The decision and the record are now made atomically, and skipped when the signature's view or rollback epoch no longer holds (#8246). - A transaction's `force_ledger_chunk` and `snapshot_at_next_signature` flags are no longer applied once a concurrent view change has discarded the transaction's writes, which previously left a chunk boundary, or an armed snapshot, for a transaction no longer present in the ledger. The forced chunk is also attached to the transaction's own version rather than whichever version the store had reached (#8245). - A rollback whose target is at or beyond the store's own version no longer moves ledger chunk metadata forward past it, which previously left a permanent offset skewing later chunk boundaries (#8244). diff --git a/src/kv/store.h b/src/kv/store.h index 64c3b43e9c2..64f9e3b7993 100644 --- a/src/kv/store.h +++ b/src/kv/store.h @@ -998,10 +998,11 @@ namespace ccf::kv { std::lock_guard vguard(version_lock); - if (txid.view != term_of_next_version && get_consensus()->is_primary()) + if (txid.view != term_of_next_version) { // This can happen when a transaction started before a view change, - // but tries to commit after the view change is complete. + // but tries to commit after the view change is complete. Reject it + // even after stepping down, before it can enter pending_txs. LOG_DEBUG_FMT( "Want to commit for term {} but term is {}", txid.view, diff --git a/src/kv/test/kv_test.cpp b/src/kv/test/kv_test.cpp index 704629c3af4..a3258ea1f1c 100644 --- a/src/kv/test/kv_test.cpp +++ b/src/kv/test/kv_test.cpp @@ -3048,132 +3048,116 @@ TEST_CASE("Stale-view writes are rejected before local application") REQUIRE(fresh_dynamic_map_tx.commit() == ccf::kv::CommitResult::SUCCESS); } -// Reproduces https://github.com/microsoft/CCF/issues/8293. -// -// The stale-view check above only covers transactions which take their version -// after the view change. A transaction which takes its version before the view -// change, but reaches Store::commit() after it, is not rejected, because that -// check is additionally gated on the node still being primary. Its entry is -// parked in pending_txs behind the hole the rollback left, survives the next -// election, and is then replicated by the first transaction of the new term - -// even though its writes were discarded and are no longer in the store. -// -// In production the two transactions which produce the hole are concurrent, and -// this one is held mid-commit by another thread. A write set observer runs at -// exactly the point that thread would be parked - after the version has been -// allocated and the writes applied locally, but before Store::commit() - so it -// is used here to drive the view change deterministically, without threads. -// -// This asserts the behaviour the store should have, so it fails until #8293 is -// fixed. Each expectation which does not currently hold is marked FAILS TODAY, -// with the behaviour actually observed. TEST_CASE("Stale-view writes which took their version early are rejected") { - ccf::kv::Store store; - store.set_encryptor(std::make_shared()); - auto consensus = std::make_shared(); - consensus->state = ccf::kv::test::StubConsensus::Primary; - store.set_consensus(consensus); + bool reuse_versions = false; + SUBCASE("No versions allocated after rollback") + { + reuse_versions = false; + } + SUBCASE("Versions allocated again after rollback") + { + reuse_versions = true; + } - constexpr ccf::kv::Term initial_term = 2; - constexpr ccf::kv::Term new_term = initial_term + 1; - constexpr ccf::SeqNo committed_seqno = 2; - MapTypes::StringString map("public:map"); - store.initialise_term(initial_term); + for (const auto state : + {ccf::kv::test::StubConsensus::Primary, + ccf::kv::test::StubConsensus::Backup, + ccf::kv::test::StubConsensus::Candidate}) + { + CAPTURE(state); - auto write = [&](const std::string& key, const std::string& value) { - auto tx = store.create_tx(); - tx.rw(map)->put(key, value); - return tx.commit(); - }; + ccf::kv::Store store; + store.set_encryptor(std::make_shared()); + auto consensus = std::make_shared(); + consensus->state = ccf::kv::test::StubConsensus::Primary; + store.set_consensus(consensus); - auto read = [&](const std::string& key) { - auto tx = store.create_read_only_tx(); - return tx.ro(map)->get(key); - }; + constexpr ccf::kv::Term initial_term = 2; + constexpr ccf::kv::Term new_term = initial_term + 1; + constexpr ccf::SeqNo committed_seqno = 2; + MapTypes::StringString map("public:map"); + store.initialise_term(initial_term); - // Seqno of the last entry consensus has been given - auto replicated_to = [&]() -> ccf::SeqNo { - return consensus->replica.empty() ? 0 : - std::get<0>(consensus->replica.back()); - }; + auto write = [&](const std::string& key, const std::string& value) { + auto tx = store.create_tx(); + tx.rw(map)->put(key, value); + return tx.commit(); + }; + + auto read = [&](const std::string& key) { + auto tx = store.create_read_only_tx(); + return tx.ro(map)->get(key); + }; + + auto replicated_to = [&]() { + return std::get<0>(consensus->replica.back()); + }; - INFO("Two committed entries, and one which is replicated but not committed"); - { REQUIRE(write("first", "1") == ccf::kv::CommitResult::SUCCESS); REQUIRE(write("second", "2") == ccf::kv::CommitResult::SUCCESS); REQUIRE(write("truncated", "3") == ccf::kv::CommitResult::SUCCESS); REQUIRE(store.current_version() == 3); - REQUIRE(replicated_to() == 3); - } + REQUIRE(consensus->replica.size() == 3); - INFO("A write takes seqno 4, then loses the view before Store::commit()"); - { - auto stale_tx = store.create_tx(); - stale_tx.rw(map)->put("stale", "4"); + INFO("Reject a transaction whose writes were rolled back mid-commit"); + { + auto stale_tx = store.create_tx(); + stale_tx.rw(map)->put("stale", "4"); - auto lose_view = [&](const ccf::crypto::Sha256Hash&, const std::string&) { - // The node hears from the new primary, steps down, and truncates its - // uncommitted suffix - discarding seqno 3, and this transaction's own - // writes at seqno 4. Seqno 3 is now a hole. - consensus->state = ccf::kv::test::StubConsensus::Backup; - consensus->replica.resize(committed_seqno); - store.rollback({initial_term, committed_seqno}, new_term); - }; + // The observer runs after local application, before Store::commit(). + auto lose_view = [&](const ccf::crypto::Sha256Hash&, const std::string&) { + REQUIRE(store.current_version() == 4); + consensus->state = state; + consensus->replica.resize(committed_seqno); + store.rollback({initial_term, committed_seqno}, new_term); - // This transaction's writes have been discarded, and it is committing in a - // term which is no longer current, so it must not report success. - // FAILS TODAY: returns SUCCESS, and parks an entry at seqno 4 in - // pending_txs, behind the hole the rollback left at seqno 3 - CHECK( - stale_tx.commit(ccf::empty_claims(), lose_view) == - ccf::kv::CommitResult::FAIL_NO_REPLICATE); + if (reuse_versions) + { + // New reservations must not make the old-view transaction valid. + REQUIRE(store.next_txid() == ccf::TxID(new_term, 3)); + REQUIRE(store.next_txid() == ccf::TxID(new_term, 4)); + } + }; - CHECK(store.current_txid() == ccf::TxID(initial_term, committed_seqno)); - CHECK(!read("stale").has_value()); - CHECK(!read("truncated").has_value()); - CHECK(replicated_to() == committed_seqno); - } + CHECK( + stale_tx.commit(ccf::empty_claims(), lose_view) == + ccf::kv::CommitResult::FAIL_NO_REPLICATE); + const auto expected_txid = reuse_versions ? + ccf::TxID(new_term, 4) : + ccf::TxID(initial_term, committed_seqno); + CHECK(store.current_txid() == expected_txid); + CHECK(!read("stale").has_value()); + CHECK(!read("truncated").has_value()); + CHECK(replicated_to() == committed_seqno); + } - INFO("The node wins the next election"); - { - consensus->state = ccf::kv::test::StubConsensus::Primary; - // aft::Aft::become_leader() rolls back to the last committable index, which - // is at or above the store's current version. Such a rollback discards - // nothing, so it returns early - without clearing pending_txs, which still - // holds the parked entry at seqno 4. - store.rollback({new_term, committed_seqno}, new_term); - } - - INFO("The first write of the new term replicates only itself"); - { - const auto replicated_before = consensus->replica.size(); - REQUIRE(write("fresh", "3") == ccf::kv::CommitResult::SUCCESS); - CHECK(read("fresh") == "3"); - CHECK(store.current_version() == 3); - - // FAILS TODAY: seqno 3 completes the batch, so the parked entry at seqno 4 - // is replicated too, carrying writes this store has already discarded and - // which no transaction on this node ever observed. Two entries are handed - // to consensus, and replicated_to() reaches 4 - CHECK(consensus->replica.size() == replicated_before + 1); - CHECK(replicated_to() == 3); - CHECK(store.current_version() == replicated_to()); - } - - INFO("Later writes continue to be replicated"); - { - const auto replicated_before = consensus->replica.size(); - REQUIRE(write("next", "4") == ccf::kv::CommitResult::SUCCESS); - CHECK(read("next") == "4"); - - // FAILS TODAY: last_replicated is now ahead of version, so no batch this - // store builds is contiguous with what has been replicated. This write, and - // every write after it, reports success but is never handed to consensus. - // Note that replicated_to() alone cannot see this, because the entry - // already sitting at seqno 4 is the discarded write, not this one - CHECK(consensus->replica.size() == replicated_before + 1); - CHECK(store.current_version() == replicated_to()); + INFO("Become primary, rolling back any new reservations"); + { + store.rollback({initial_term, committed_seqno}, new_term + 1); + consensus->state = ccf::kv::test::StubConsensus::Primary; + } + + INFO("The first write after election replicates only itself"); + { + const auto replicated_before = consensus->replica.size(); + REQUIRE(write("fresh", "3") == ccf::kv::CommitResult::SUCCESS); + CHECK(read("fresh") == "3"); + CHECK(store.current_txid() == ccf::TxID(new_term + 1, 3)); + CHECK(consensus->replica.size() == replicated_before + 1); + CHECK(replicated_to() == 3); + CHECK(store.current_version() == replicated_to()); + } + + INFO("Subsequent writes continue to replicate"); + { + const auto replicated_before = consensus->replica.size(); + REQUIRE(write("next", "4") == ccf::kv::CommitResult::SUCCESS); + CHECK(read("next") == "4"); + CHECK(!read("stale").has_value()); + CHECK(consensus->replica.size() == replicated_before + 1); + CHECK(store.current_version() == replicated_to()); + } } }