From fa651e6a1da8cc6d351e347a0112cc9b692def47 Mon Sep 17 00:00:00 2001 From: achamayou Date: Sun, 30 Aug 2026 20:29:02 +0100 Subject: [PATCH 1/2] Stop a rollback moving chunk metadata forward A rollback whose target is at or beyond the store's own version discards nothing, but still reset the chunker to that target. The chunker can legitimately lag the store, because an entry is allocated a version well before its size is recorded, so this moved the chunker forward past the store and left a permanent offset that skewed every later chunk boundary. Clamp the target to the store's version, so a rollback can only ever move chunk metadata back. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 75d99c5d-6efa-4048-8032-8c78b97208d9 --- CHANGELOG.md | 1 + src/kv/store.h | 8 +++++--- src/kv/test/kv_test.cpp | 45 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index baf44e462b8..dfd7ef5bcda 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 +- 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). - Ledger chunk metadata and snapshot scheduling are no longer restored by a transaction whose writes a concurrent view change has already discarded. Both are now updated under the same lock as the rollback, and skipped when the transaction's rollback epoch or view no longer holds (#8243). - A transaction whose view changed while it was committing could apply its writes to the local key-value store and then fail to replicate, leaving state that never reached consensus. The transaction's view is now validated atomically with the allocation of its version, so it is rejected before any map is modified, and `ccf::kv::CommitResult::FAIL_NO_REPLICATE` no longer implies a locally applied write (#8242). diff --git a/src/kv/store.h b/src/kv/store.h index 0f32c766a4e..5bc0df5b3ce 100644 --- a/src/kv/store.h +++ b/src/kv/store.h @@ -692,9 +692,11 @@ namespace ccf::kv } if (chunker) { - // Keep this ordered with append_entry_size() below, so a commit - // cannot restore chunk metadata after this rollback. - chunker->rolled_back_to(tx_id.seqno); + // Nothing local is discarded here, but the chunker may still be + // behind `version` if an allocated entry has not reached + // append_entry_size() yet. Clamp so this can only ever move the + // chunker back, never forward past the Store. + chunker->rolled_back_to(std::min(tx_id.seqno, version)); } return; } diff --git a/src/kv/test/kv_test.cpp b/src/kv/test/kv_test.cpp index 1f2daca6b6d..e3c80499207 100644 --- a/src/kv/test/kv_test.cpp +++ b/src/kv/test/kv_test.cpp @@ -3596,6 +3596,51 @@ TEST_CASE("Chunk metadata is not restored by a batch a rollback discarded") CHECK(chunker->current_version() == store.current_version()); } +TEST_CASE("A rollback never moves chunk metadata past the store's version") +{ + ccf::kv::Store store; + store.set_encryptor(std::make_shared()); + auto consensus = std::make_shared(); + store.set_consensus(consensus); + auto chunker = std::make_shared(); + store.set_chunker(chunker); + + constexpr ccf::kv::Term initial_term = 2; + store.initialise_term(initial_term); + MapTypes::StringString map("public:map"); + + { + auto tx = store.create_tx(); + tx.rw(map)->put("key", "initial"); + REQUIRE(tx.commit() == ccf::kv::CommitResult::SUCCESS); + } + + const auto version = store.current_version(); + REQUIRE(chunker->current_version() == version); + + SUBCASE("Rollback to the current version") + { + store.rollback(store.current_txid(), initial_term + 1); + } + + SUBCASE("Rollback beyond the current version") + { + store.rollback({initial_term, version + 3}, initial_term + 1); + } + + // Neither discards anything, so neither may move the chunker. + CHECK(store.current_version() == version); + CHECK(chunker->current_version() == version); + + INFO("Later entries are still recorded against their own version"); + { + auto tx = store.create_tx(); + tx.rw(map)->put("key", "fresh"); + REQUIRE(tx.commit() == ccf::kv::CommitResult::SUCCESS); + } + CHECK(chunker->current_version() == store.current_version()); +} + TEST_CASE("Ledger entry chunk request") { ccf::kv::Store store; From 2d7f285b3c5fabc1f58e05902ec1a8a6da6a4a0f Mon Sep 17 00:00:00 2001 From: Amaury Chamayou Date: Thu, 3 Sep 2026 21:43:47 +0100 Subject: [PATCH 2/2] Update comments for clarity on chunker behavior Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/kv/store.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/kv/store.h b/src/kv/store.h index 5bc0df5b3ce..717c6dfaa94 100644 --- a/src/kv/store.h +++ b/src/kv/store.h @@ -692,10 +692,9 @@ namespace ccf::kv } if (chunker) { - // Nothing local is discarded here, but the chunker may still be - // behind `version` if an allocated entry has not reached - // append_entry_size() yet. Clamp so this can only ever move the - // chunker back, never forward past the Store. + // Nothing local is discarded here, but the rollback target may be + // at or beyond the Store's current version. Clamp so this cannot + // move chunk metadata forward past the Store. chunker->rolled_back_to(std::min(tx_id.seqno, version)); } return;