From 8e163eb4da65a7b99c16e903ff32ae950a34f143 Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Tue, 11 Aug 2026 09:58:02 -0700 Subject: [PATCH 01/16] SDSTOR-22886 craft: SyncRSCommitLSN RAFT entry apply Implement the apply side of the SyncRSCommitLSN RAFT entry: on_commit now parses the entry header/key and dispatches to apply_sync_rs_commit_lsn, which reconciles empty_slots, catches up missing journal data from a peer, and advances the commit_lsn/ last_append_lsn watermarks. InternalLogin dispatch and apply (SDSTOR-22887) and the checkpoint trigger (SDSTOR-22888) are deliberately left as stubs for follow-up PRs. - on_commit: validates header/key blob sizes, parses CraftEntryType and the SyncRSCommitLSNPayload fixed prefix + empty_slots, and detaches apply_sync_rs_commit_lsn as fire-and-forget (on_commit is a synchronous HomeStore callback; apply needs to co_await peer fetch + journal writes). Logs and no-ops on an unrecognized entry type. - apply_sync_rs_commit_lsn: a client_token mismatch gates the entire apply (no reconciliation, no catch-up, no watermark advance). Otherwise, empty_slots are reconciled into empty_lsns_/missing_lsns_, the newly-spanned range is marked missing, and catch-up via CraftPeerFetcher::fetch_from_peer + CraftJournalBackend::write_slot is best-effort: a failed fetch, a failed write, or no peer_fetcher_ wired at all just leaves the affected LSNs in missing_lsns_ for a later attempt. commit_lsn/last_append_lsn advance unconditionally afterward (never decrement), mirroring truncate()'s existing invariant. - Add volume_error::WRONG_TOKEN for the client_token-mismatch case. - Add a _PRERELEASE-only test_listener() accessor so tests can drive on_commit directly. - New test_craft_raft_entries.cpp (with a MockCraftPeerFetcher) covering the token gate, empty_slots reconciliation, watermark advance (including never-decrements), best-effort catch-up (success, fetch failure, write failure, unwired fetcher), and on_commit dispatch including malformed-entry rejection. --- conanfile.py | 2 +- src/include/homeblks/home_blocks.hpp | 2 +- src/lib/craft/craft_repl_dev.cpp | 128 ++++++- src/lib/craft/craft_repl_dev.hpp | 14 +- src/lib/craft/tests/CMakeLists.txt | 15 + .../craft/tests/test_craft_raft_entries.cpp | 320 ++++++++++++++++++ 6 files changed, 469 insertions(+), 12 deletions(-) create mode 100644 src/lib/craft/tests/test_craft_raft_entries.cpp diff --git a/conanfile.py b/conanfile.py index 5cc084b..17f0c34 100644 --- a/conanfile.py +++ b/conanfile.py @@ -108,7 +108,7 @@ def build(self): cmake.configure() cmake.build() if not self.conf.get("tools.build:skip_test", default=False): - jobs = self.conf.get("tools.build:jobs", default=3) + jobs = self.conf.get("tools.build:jobs", default=4) env = Environment() env.define("CTEST_PARALLEL_LEVEL", str(jobs)) if self.options.get_safe("sanitize") == "thread": diff --git a/src/include/homeblks/home_blocks.hpp b/src/include/homeblks/home_blocks.hpp index 0afcd98..da8e2e9 100644 --- a/src/include/homeblks/home_blocks.hpp +++ b/src/include/homeblks/home_blocks.hpp @@ -69,7 +69,7 @@ using volume_handle = std::shared_ptr< volume >; // standard equivalent (invalid arg, no space, io error, unsupported op, ...) is returned as // std::make_error_condition(std::errc::*) directly rather than duplicated here. ENUM(volume_error, uint16_t, UNKNOWN_VOLUME = 1, CRC_MISMATCH, INDEX_ERROR, INTERNAL_ERROR, OFFLINE, STALE_TERM, - EMPTY_SLOT); + EMPTY_SLOT, WRONG_TOKEN); ENUM(volume_state, uint32_t, INIT, // created, not yet online diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index 013eef2..235e825 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -14,6 +14,7 @@ *********************************************************************************/ #include "craft_repl_dev.hpp" +#include "../coro_helpers.hpp" #include #include @@ -476,20 +477,131 @@ async_result< std::vector< JournalSlot > > CraftReplDev::fetch_data(std::vector< // ─── RAFT listener ──────────────────────────────────────────────────────────── -void CraftReplDev::CraftRaftListener::on_commit(int64_t lsn, sisl::blob const& /* header */, - sisl::blob const& /* key */, +void CraftReplDev::CraftRaftListener::on_commit(int64_t lsn, sisl::blob const& header, sisl::blob const& key, std::vector< homestore::multi_blk_id > const& /* blkids */, cintrusive< homestore::repl_req_ctx >& /* ctx */) { - // S5 will parse the entry type from `header` and dispatch to - // owner_->apply_sync_rs_commit_lsn() or owner_->apply_internal_login(). - LOGD("CraftRaftListener::on_commit lsn={} (entry dispatch not yet implemented)", lsn); + if (header.size() < sizeof(CraftEntryHeader)) { + LOGE("on_commit lsn={} header too small ({} bytes)", lsn, header.size()); + return; + } + const auto* entry_hdr = reinterpret_cast< const CraftEntryHeader* >(header.cbytes()); + + switch (entry_hdr->type) { + case CraftEntryType::SyncRSCommitLSN: { + if (key.size() < sizeof(SyncRSCommitLSNPayload)) { + LOGE("on_commit lsn={} SyncRSCommitLSN key too small ({} bytes)", lsn, key.size()); + return; + } + const auto* payload = reinterpret_cast< const SyncRSCommitLSNPayload* >(key.cbytes()); + auto empty_slots = parse_empty_slots(key); + if (!empty_slots) { + LOGE("on_commit lsn={} SyncRSCommitLSN malformed empty_slots", lsn); + return; + } + // apply_sync_rs_commit_lsn co_awaits peer fetch + journal writes; on_commit itself is a synchronous + // HomeStore callback, so fire-and-forget it. + detail::detach(owner_->apply_sync_rs_commit_lsn(payload->rs_commit_lsn, payload->client_token, + std::move(*empty_slots))); + break; + } + case CraftEntryType::InternalLogin: + // Dispatch lands with 22887. + LOGD("on_commit lsn={} InternalLogin (dispatch not yet implemented)", lsn); + break; + default: + LOGE("on_commit lsn={} unrecognized CraftEntryType={}", lsn, static_cast< uint8_t >(entry_hdr->type)); + break; + } } // ─── RAFT apply helpers (S5 implements) ────────────────────────────────────── +// +// apply_sync_rs_commit_lsn (22886): client_token is verified against the current session first -- a mismatch +// gates the ENTIRE apply (no reconciliation, no catch-up, no watermark advance), since a RAFT entry whose +// token doesn't match the live session shouldn't be trusted to describe it. Once the token matches, every +// other step is best-effort forward progress: empty_slots are reconciled and the newly-spanned range is +// marked missing, catch-up attempts to fill in what it can from a peer, and commit_lsn/last_append_lsn +// advance regardless of whether catch-up fully succeeded -- mirroring truncate()'s invariant that apply +// never reverts the watermark, only advances it. + +async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint64_t client_token, + std::vector< int64_t > empty_slots) { + std::vector< int64_t > to_fetch; + uint64_t term; + { + std::lock_guard lk{missing_mu_}; + if (client_token != state_.client_token) { + LOGW("apply_sync_rs_commit_lsn: client_token mismatch want={} got={} rs_commit_lsn={} -- skipping apply", + state_.client_token, client_token, rs_commit_lsn); + co_return std::unexpected(make_error_condition(volume_error::WRONG_TOKEN)); + } + term = state_.term; -void CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint64_t /* client_token */, - std::vector< int64_t > /* empty_slots */) { - LOGD("apply_sync_rs_commit_lsn rs_commit_lsn={} (not yet implemented)", rs_commit_lsn); + for (int64_t lsn : empty_slots) { + empty_lsns_.insert(lsn); + missing_lsns_.erase(lsn); + } + + // Everything newly spanned by this advance that isn't Empty-verdicted is a gap until catch-up + // (below) resolves it -- same idiom write() uses for gaps opened by an out-of-order dlsn. + for (int64_t lsn = state_.last_append_lsn + 1; lsn <= rs_commit_lsn; ++lsn) { + if (!empty_lsns_.contains(lsn)) missing_lsns_.insert(lsn); + } + state_.last_append_lsn = std::max(state_.last_append_lsn, rs_commit_lsn); + + for (int64_t lsn : missing_lsns_) { + if (lsn <= rs_commit_lsn) to_fetch.push_back(lsn); + } + } + + if (!to_fetch.empty()) { + if (peer_fetcher_ == nullptr) { + LOGW("apply_sync_rs_commit_lsn: {} lsn(s) missing but no peer_fetcher_ wired -- leaving as missing", + to_fetch.size()); + } else if (auto fetched = co_await peer_fetcher_->fetch_data(to_fetch); !fetched) { + LOGE("apply_sync_rs_commit_lsn: fetch_data failed: {} -- leaving {} lsn(s) as missing", + fetched.error().message(), to_fetch.size()); + } else { + for (auto& slot : *fetched) { + if (slot.is_empty) { + std::lock_guard lk{missing_mu_}; + empty_lsns_.insert(slot.lsn); + missing_lsns_.erase(slot.lsn); + continue; + } + // HS_DATA_LINKED, same as write(): allocate blocks and write the payload before + // journalling the block reference. all_zeros slots carry no data and skip alloc. + homestore::multi_blk_id blkid{}; + if (!slot.all_zeros) { + auto alloc_res = co_await journal_->alloc_write_data(slot.data, slot.len_bytes); + if (!alloc_res) { + LOGE("apply_sync_rs_commit_lsn: alloc_write_data failed lsn={}: {} -- leaving as missing", + slot.lsn, alloc_res.error().message()); + continue; + } + blkid = *alloc_res; + } + auto res = co_await journal_->write_slot(slot.lsn, term, slot.lba_off_bytes, slot.len_bytes, blkid, + slot.all_zeros); + if (!res) { + LOGE("apply_sync_rs_commit_lsn: write_slot failed lsn={}: {} -- leaving as missing", slot.lsn, + res.error().message()); + continue; + } + std::lock_guard lk{missing_mu_}; + missing_lsns_.erase(slot.lsn); + } + } + } + + // Unconditional: commit_lsn is a replica-set-wide watermark RAFT already agreed on, independent of + // whether this replica's local catch-up succeeded. + { + std::lock_guard lk{missing_mu_}; + state_.commit_lsn = std::max(state_.commit_lsn, rs_commit_lsn); + } + LOGT("apply_sync_rs_commit_lsn ok rs_commit_lsn={} client_token={}", rs_commit_lsn, client_token); + co_return ok(); } void CraftReplDev::apply_internal_login(uint64_t client_token, uint64_t term) { diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index ca8a54f..d83e91e 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -115,6 +115,10 @@ class CraftPeerFetcher { // index. Non-CRAFT volumes are unaffected. class CraftReplDev { + // Lets test_craft_raft_entries.cpp call apply_sync_rs_commit_lsn (private) directly, so it can assert + // on the exact result rather than only on-commit's discarded fire-and-forget outcome. + friend class CraftRaftEntriesTest; + public: explicit CraftReplDev(volume_id_t vol_id, unique< CraftJournalBackend > journal); ~CraftReplDev() = default; @@ -236,6 +240,9 @@ class CraftReplDev { void seed_empty(std::initializer_list< int64_t > empty); // Seeds the session term so tests can exercise write() with a non-zero term without a full login. void seed_term(uint64_t term); + // Exposes the RAFT listener so tests can drive on_commit() directly -- raft_listener_ has no other + // accessor (production wiring into HomeStore's repl_dev happens elsewhere). + homestore::repl_dev_listener& test_listener() { return raft_listener_; } #endif private: @@ -295,8 +302,11 @@ class CraftReplDev { CraftReplDev* owner_; }; - // Called from CraftRaftListener::on_commit after deserialising the entry type. - void apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint64_t client_token, std::vector< int64_t > empty_slots); + // Called from CraftRaftListener::on_commit after deserialising the entry type. Detached (fire-and-forget) + // from on_commit since that HomeStore callback is synchronous but catch-up here needs to co_await peer + // fetch + journal writes. + async_status apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint64_t client_token, + std::vector< int64_t > empty_slots); void apply_internal_login(uint64_t client_token, uint64_t term); volume_id_t vol_id_; diff --git a/src/lib/craft/tests/CMakeLists.txt b/src/lib/craft/tests/CMakeLists.txt index 43b293a..b661ae5 100644 --- a/src/lib/craft/tests/CMakeLists.txt +++ b/src/lib/craft/tests/CMakeLists.txt @@ -80,3 +80,18 @@ target_link_libraries(test_craft_journal_slot_wire ) add_test(NAME CraftJournalSlotWireTest COMMAND test_craft_journal_slot_wire) + +# Unit tests for CraftReplDev::apply_sync_rs_commit_lsn and the on_commit SyncRSCommitLSN dispatch +# (S5: SDSTOR-22886). Same pattern: compile craft_repl_dev.cpp directly. +add_executable(test_craft_raft_entries) +target_sources(test_craft_raft_entries PRIVATE + test_craft_raft_entries.cpp + ../craft_repl_dev.cpp +) +target_compile_definitions(test_craft_raft_entries PRIVATE _PRERELEASE) +target_link_libraries(test_craft_raft_entries + ${COMMON_TEST_DEPS} + -rdynamic +) + +add_test(NAME CraftRaftEntriesTest COMMAND test_craft_raft_entries) diff --git a/src/lib/craft/tests/test_craft_raft_entries.cpp b/src/lib/craft/tests/test_craft_raft_entries.cpp new file mode 100644 index 0000000..1bc31b6 --- /dev/null +++ b/src/lib/craft/tests/test_craft_raft_entries.cpp @@ -0,0 +1,320 @@ +/********************************************************************************* + * Modifications Copyright 2026 eBay Inc. + * + * 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 + * https://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. + * + *********************************************************************************/ + +// Unit tests for CraftReplDev::apply_sync_rs_commit_lsn and the on_commit SyncRSCommitLSN dispatch +// (S5 / SDSTOR-22886). InternalLogin apply/dispatch is still a stub (lands with SDSTOR-22887) and is +// not covered here. +// +// Tests verify: +// - a client_token mismatch gates the ENTIRE apply: no reconciliation, no catch-up, no watermark advance +// - empty_slots are reconciled into empty_lsns_ and erased from missing_lsns_ +// - commit_lsn/last_append_lsn advance directly when there's no gap to catch up on +// - fetch_data is invoked with exactly the missing LSNs when behind, and its response is persisted +// - catch-up is best-effort: a failed fetch, a failed write_slot, or no peer_fetcher_ at all still lets +// commit_lsn advance, leaving unresolved LSNs in missing_lsns_ +// - commit_lsn never decrements +// - on_commit parses a real serialized SyncRSCommitLSN entry and dispatches correctly (and rejects +// malformed header/key blobs without touching state) +// +// This TU defines SISL_LOGGING_DEF for the homeblocks module because it compiles craft_repl_dev.cpp +// directly (same pattern as test_craft_truncate.cpp). + +#include +#include +#include +#include + +#include "craft/craft_repl_dev.hpp" +#include "coro_helpers.hpp" + +SISL_LOGGING_DEF(HOMEBLOCKS_LOG_MODS) +SISL_LOGGING_INIT(HOMEBLOCKS_LOG_MODS) + +namespace homeblocks { +namespace { + +// ── journal mock ────────────────────────────────────────────────────────────── +// +// Backed by a std::map so tests can inspect exactly which LSNs got persisted during catch-up. +// fail_on_write optionally injects an I/O error for a specific LSN. + +class MockCraftJournalBackend : public CraftJournalBackend { +public: + std::map< int64_t, JournalSlot > slots; + std::optional< int64_t > fail_on_write; + + async_result< homestore::multi_blk_id > alloc_write_data(sisl::sg_list const&, lba_count_t) override { + co_return homestore::multi_blk_id{}; + } + + async_status write_slot(int64_t lsn, uint64_t /* term */, lba_t lba, lba_count_t len, + homestore::multi_blk_id /* blkid */, bool all_zeros) override { + if (fail_on_write && *fail_on_write == lsn) + co_return std::unexpected(std::make_error_condition(std::errc::io_error)); + slots[lsn] = JournalSlot{.lsn = lsn, .all_zeros = all_zeros, .lba_off_bytes = lba, .len_bytes = len}; + co_return ok(); + } + + async_result< JournalSlot > read_slot(int64_t lsn) override { + auto it = slots.find(lsn); + if (it == slots.end()) + co_return std::unexpected(std::make_error_condition(std::errc::no_such_file_or_directory)); + co_return it->second; + } + + async_status truncate_to(int64_t) override { co_return ok(); } + + async_status free_data(homestore::multi_blk_id) override { co_return ok(); } + + bool has_slot(int64_t lsn) const { return slots.count(lsn) > 0; } +}; + +// ── peer fetcher mock ───────────────────────────────────────────────────────── +// +// Records the LSN list it was last called with; returns a programmable response or an injected error. + +class MockCraftPeerFetcher : public CraftPeerFetcher { +public: + std::vector< int64_t > last_requested; + std::vector< JournalSlot > response; + bool should_fail{false}; + + async_result< craft::lsn_pair > get_rs_commit_lsn(uint64_t /* term */, bool /* is_login */) override { + co_return craft::lsn_pair{}; + } + + async_result< std::vector< JournalSlot > > fetch_data(const std::vector< int64_t >& lsns) override { + last_requested = lsns; + if (should_fail) co_return std::unexpected(std::make_error_condition(std::errc::io_error)); + co_return response; + } +}; + +// ── wire-format helpers for the on_commit dispatch tests ───────────────────── + +sisl::blob as_blob(std::vector< uint8_t >& buf) { return sisl::blob{buf.data(), static_cast< uint32_t >(buf.size())}; } + +std::vector< uint8_t > make_header(CraftEntryType type) { + std::vector< uint8_t > buf(sizeof(CraftEntryHeader)); + reinterpret_cast< CraftEntryHeader* >(buf.data())->type = type; + return buf; +} + +std::vector< uint8_t > make_sync_rs_commit_lsn_key(int64_t rs_commit_lsn, uint64_t client_token, + const std::vector< int64_t >& empty_slots) { + std::vector< uint8_t > buf(sync_rs_commit_lsn_key_size(empty_slots.size())); + serialize_sync_rs_commit_lsn(buf.data(), rs_commit_lsn, client_token, empty_slots); + return buf; +} + +} // namespace + +// craft_repl_dev.hpp friends this exact type (homeblocks::CraftRaftEntriesTest) so it can call the +// private apply_sync_rs_commit_lsn directly -- it must NOT sit in the anonymous namespace above, or it +// would be a distinct, unrelated type from the friend's perspective. + +// ── test fixture ───────────────────────────────────────────────────────────── + +class CraftRaftEntriesTest : public ::testing::Test { +protected: + void SetUp() override { + auto mock = std::make_unique< MockCraftJournalBackend >(); + journal_ = mock.get(); + dev_ = std::make_unique< CraftReplDev >(volume_id_t{}, std::move(mock)); + } + + auto do_apply(int64_t rs_commit_lsn, uint64_t client_token, std::vector< int64_t > empty_slots = {}) { + return homeblocks::detail::sync_get( + dev_->apply_sync_rs_commit_lsn(rs_commit_lsn, client_token, std::move(empty_slots))); + } + + MockCraftJournalBackend* journal_{nullptr}; + MockCraftPeerFetcher fetcher_; + std::unique_ptr< CraftReplDev > dev_; +}; + +namespace { + +// ── client_token gate ───────────────────────────────────────────────────────── + +// state_.client_token defaults to 0; a non-matching token must veto the whole apply. +TEST_F(CraftRaftEntriesTest, TokenMismatchSkipsWholeApply) { + dev_->seed_lsns(5, {3}); + auto r = do_apply(/*rs_commit_lsn=*/100, /*client_token=*/999); + + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(volume_error::WRONG_TOKEN)); + EXPECT_EQ(dev_->commit_lsn(), -1); + EXPECT_EQ(dev_->last_append_lsn(), 5); + EXPECT_EQ(dev_->missing_count(), 1u); +} + +// ── empty_slots reconciliation ──────────────────────────────────────────────── + +TEST_F(CraftRaftEntriesTest, EmptySlotsReconciled) { + dev_->seed_lsns(5, {3}); + auto r = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/0, /*empty_slots=*/{3}); + + ASSERT_TRUE(r.has_value()); + EXPECT_TRUE(dev_->is_empty_slot(3)); + EXPECT_FALSE(dev_->is_missing(3)); + EXPECT_EQ(dev_->commit_lsn(), 5); +} + +// ── watermark advance ────────────────────────────────────────────────────────── + +// last_append_lsn already covers rs_commit_lsn: nothing to fetch, commit_lsn advances directly. +TEST_F(CraftRaftEntriesTest, NoGapAdvancesDirectly) { + dev_->set_peer_fetcher(&fetcher_); + dev_->seed_lsns(10, {}); + + auto r = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 5); + EXPECT_TRUE(fetcher_.last_requested.empty()); +} + +TEST_F(CraftRaftEntriesTest, CommitLsnNeverDecrements) { + dev_->seed_lsns(10, {}); + dev_->seed_commit_lsn(8); + + auto r = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 8); + EXPECT_EQ(dev_->last_append_lsn(), 10); +} + +// ── catch-up ─────────────────────────────────────────────────────────────────── + +// Behind rs_commit_lsn: fetch_data is called with exactly the missing LSNs, and its response +// (one present slot, one Empty slot) is persisted/marked correctly. +TEST_F(CraftRaftEntriesTest, BehindWithPeerFetcherAppliesFetchedSlots) { + dev_->set_peer_fetcher(&fetcher_); + dev_->seed_lsns(0, {}); + fetcher_.response = { + JournalSlot{.lsn = 1, .lba_off_bytes = 10, .len_bytes = 4}, + JournalSlot{.lsn = 2, .is_empty = true}, + }; + + auto r = do_apply(/*rs_commit_lsn=*/2, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(fetcher_.last_requested, (std::vector< int64_t >{1, 2})); + EXPECT_TRUE(journal_->has_slot(1)); + EXPECT_TRUE(dev_->is_empty_slot(2)); + EXPECT_FALSE(dev_->is_missing(1)); + EXPECT_FALSE(dev_->is_missing(2)); + EXPECT_EQ(dev_->commit_lsn(), 2); + EXPECT_EQ(dev_->last_append_lsn(), 2); +} + +// fetch_data fails outright: commit_lsn still advances (best-effort); every spanned LSN remains missing. +TEST_F(CraftRaftEntriesTest, BehindFetchFailsStillAdvancesCommitLsn) { + dev_->set_peer_fetcher(&fetcher_); + dev_->seed_lsns(0, {}); + fetcher_.should_fail = true; + + auto r = do_apply(/*rs_commit_lsn=*/3, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 3); + EXPECT_EQ(dev_->last_append_lsn(), 3); + EXPECT_EQ(dev_->missing_count(), 3u); +} + +// No peer_fetcher_ wired at all (S9 not wired yet): same best-effort outcome as a fetch failure. +TEST_F(CraftRaftEntriesTest, BehindNoPeerFetcherStillAdvancesCommitLsn) { + dev_->seed_lsns(0, {}); + + auto r = do_apply(/*rs_commit_lsn=*/2, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 2); + EXPECT_EQ(dev_->missing_count(), 2u); +} + +// A fetched slot's write_slot fails: that LSN alone stays missing; the rest of catch-up still applies, +// and commit_lsn still advances. +TEST_F(CraftRaftEntriesTest, WriteSlotFailureDuringCatchupLeavesLsnMissing) { + dev_->set_peer_fetcher(&fetcher_); + dev_->seed_lsns(0, {}); + fetcher_.response = { + JournalSlot{.lsn = 1, .lba_off_bytes = 1, .len_bytes = 4}, + JournalSlot{.lsn = 2, .lba_off_bytes = 2, .len_bytes = 4}, + }; + journal_->fail_on_write = 2; + + auto r = do_apply(/*rs_commit_lsn=*/2, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_TRUE(journal_->has_slot(1)); + EXPECT_FALSE(journal_->has_slot(2)); + EXPECT_FALSE(dev_->is_missing(1)); + EXPECT_TRUE(dev_->is_missing(2)); + EXPECT_EQ(dev_->commit_lsn(), 2); +} + +// ── on_commit dispatch ───────────────────────────────────────────────────────── + +TEST_F(CraftRaftEntriesTest, OnCommitDispatchesSyncRSCommitLSN) { + auto header_buf = make_header(CraftEntryType::SyncRSCommitLSN); + auto key_buf = make_sync_rs_commit_lsn_key(/*rs_commit_lsn=*/7, /*client_token=*/0, /*empty_slots=*/{}); + cintrusive< homestore::repl_req_ctx > ctx{}; + + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); + + EXPECT_EQ(dev_->commit_lsn(), 7); +} + +TEST_F(CraftRaftEntriesTest, OnCommitRejectsHeaderTooSmall) { + std::vector< uint8_t > empty_header; + auto key_buf = make_sync_rs_commit_lsn_key(7, 0, {}); + cintrusive< homestore::repl_req_ctx > ctx{}; + + dev_->test_listener().on_commit(1, as_blob(empty_header), as_blob(key_buf), {}, ctx); + + EXPECT_EQ(dev_->commit_lsn(), -1); // untouched +} + +TEST_F(CraftRaftEntriesTest, OnCommitRejectsMalformedSyncRSCommitLSNKey) { + auto header_buf = make_header(CraftEntryType::SyncRSCommitLSN); + std::vector< uint8_t > short_key(sizeof(SyncRSCommitLSNPayload) - 1, 0); + cintrusive< homestore::repl_req_ctx > ctx{}; + + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(short_key), {}, ctx); + + EXPECT_EQ(dev_->commit_lsn(), -1); // untouched +} + +TEST_F(CraftRaftEntriesTest, OnCommitLogsUnrecognizedEntryType) { + std::vector< uint8_t > header_buf(sizeof(CraftEntryHeader)); + reinterpret_cast< CraftEntryHeader* >(header_buf.data())->type = static_cast< CraftEntryType >(99); + std::vector< uint8_t > key_buf; + cintrusive< homestore::repl_req_ctx > ctx{}; + + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); + + EXPECT_EQ(dev_->commit_lsn(), -1); // untouched +} + +} // namespace +} // namespace homeblocks + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} \ No newline at end of file From 101103424235689ec2b0d135da253c5e88124b2f Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Tue, 11 Aug 2026 11:35:36 -0700 Subject: [PATCH 02/16] S5 craft: fix critique findings on SyncRSCommitLSN apply - guard CraftRaftEntriesTest friend decl with #ifdef _PRERELEASE - rename OnCommitLogsUnrecognizedEntryType -> OnCommitIgnoresUnrecognizedEntryType - add tests: mismatched empty_slots count via on_commit, empty_slots overlapping the same apply's new gap range --- src/lib/craft/craft_repl_dev.hpp | 2 + .../craft/tests/test_craft_raft_entries.cpp | 37 ++++++++++++++++++- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index d83e91e..5acd930 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -115,9 +115,11 @@ class CraftPeerFetcher { // index. Non-CRAFT volumes are unaffected. class CraftReplDev { +#ifdef _PRERELEASE // Lets test_craft_raft_entries.cpp call apply_sync_rs_commit_lsn (private) directly, so it can assert // on the exact result rather than only on-commit's discarded fire-and-forget outcome. friend class CraftRaftEntriesTest; +#endif public: explicit CraftReplDev(volume_id_t vol_id, unique< CraftJournalBackend > journal); diff --git a/src/lib/craft/tests/test_craft_raft_entries.cpp b/src/lib/craft/tests/test_craft_raft_entries.cpp index 1bc31b6..88282e6 100644 --- a/src/lib/craft/tests/test_craft_raft_entries.cpp +++ b/src/lib/craft/tests/test_craft_raft_entries.cpp @@ -173,6 +173,25 @@ TEST_F(CraftRaftEntriesTest, EmptySlotsReconciled) { EXPECT_EQ(dev_->commit_lsn(), 5); } +// An empty_slots entry can also fall inside the range this same apply newly opens (rather than being +// an already-missing LSN from before) -- it must end up ONLY in empty_lsns_, not re-added to +// missing_lsns_ by the gap-marking step that runs right after reconciliation. +TEST_F(CraftRaftEntriesTest, EmptySlotWithinNewGapRangeNotDoubleTracked) { + dev_->seed_lsns(0, {}); + auto r = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/0, /*empty_slots=*/{3}); + + ASSERT_TRUE(r.has_value()); + EXPECT_TRUE(dev_->is_empty_slot(3)); + EXPECT_FALSE(dev_->is_missing(3)); + // The rest of the newly-opened gap range (1, 2, 4, 5) is still missing -- no peer_fetcher_ wired. + EXPECT_TRUE(dev_->is_missing(1)); + EXPECT_TRUE(dev_->is_missing(2)); + EXPECT_TRUE(dev_->is_missing(4)); + EXPECT_TRUE(dev_->is_missing(5)); + EXPECT_EQ(dev_->missing_count(), 4u); + EXPECT_EQ(dev_->commit_lsn(), 5); +} + // ── watermark advance ────────────────────────────────────────────────────────── // last_append_lsn already covers rs_commit_lsn: nothing to fetch, commit_lsn advances directly. @@ -300,7 +319,21 @@ TEST_F(CraftRaftEntriesTest, OnCommitRejectsMalformedSyncRSCommitLSNKey) { EXPECT_EQ(dev_->commit_lsn(), -1); // untouched } -TEST_F(CraftRaftEntriesTest, OnCommitLogsUnrecognizedEntryType) { +// Distinct from the too-short case above: this key is large enough for the fixed prefix (and even +// carries 2 real trailing slots), but lies about how many follow -- parse_empty_slots's exact-size +// check (not on_commit's coarser size check) is what rejects it. +TEST_F(CraftRaftEntriesTest, OnCommitRejectsMismatchedEmptySlotsCount) { + auto header_buf = make_header(CraftEntryType::SyncRSCommitLSN); + auto key_buf = make_sync_rs_commit_lsn_key(7, 0, {10, 20}); + reinterpret_cast< SyncRSCommitLSNPayload* >(key_buf.data())->num_empty_slots = 5; + cintrusive< homestore::repl_req_ctx > ctx{}; + + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); + + EXPECT_EQ(dev_->commit_lsn(), -1); // untouched +} + +TEST_F(CraftRaftEntriesTest, OnCommitIgnoresUnrecognizedEntryType) { std::vector< uint8_t > header_buf(sizeof(CraftEntryHeader)); reinterpret_cast< CraftEntryHeader* >(header_buf.data())->type = static_cast< CraftEntryType >(99); std::vector< uint8_t > key_buf; @@ -317,4 +350,4 @@ TEST_F(CraftRaftEntriesTest, OnCommitLogsUnrecognizedEntryType) { int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); -} \ No newline at end of file +} From 0b5401a34a5935227853726f5a0a972997c33a06 Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Tue, 11 Aug 2026 14:01:18 -0700 Subject: [PATCH 03/16] craft: validate SyncRSCommitLSN empty_slots and peer fetch responses - reject the whole apply (new volume_error::INVALID_ENTRY) if empty_slots has a negative LSN or one above rs_commit_lsn - validate a peer's fetch_data response against what was requested; discard the whole batch on an unrequested/duplicate lsn - document the known use-after-free gap in the detached apply_sync_rs_commit_lsn coroutine (not fixed yet) - add tests for both validations --- src/include/homeblks/home_blocks.hpp | 2 +- src/lib/craft/craft_repl_dev.cpp | 56 ++++++++++++++- src/lib/craft/craft_repl_dev.hpp | 2 + .../craft/tests/test_craft_raft_entries.cpp | 71 +++++++++++++++++++ 4 files changed, 128 insertions(+), 3 deletions(-) diff --git a/src/include/homeblks/home_blocks.hpp b/src/include/homeblks/home_blocks.hpp index da8e2e9..31a247d 100644 --- a/src/include/homeblks/home_blocks.hpp +++ b/src/include/homeblks/home_blocks.hpp @@ -69,7 +69,7 @@ using volume_handle = std::shared_ptr< volume >; // standard equivalent (invalid arg, no space, io error, unsupported op, ...) is returned as // std::make_error_condition(std::errc::*) directly rather than duplicated here. ENUM(volume_error, uint16_t, UNKNOWN_VOLUME = 1, CRC_MISMATCH, INDEX_ERROR, INTERNAL_ERROR, OFFLINE, STALE_TERM, - EMPTY_SLOT, WRONG_TOKEN); + EMPTY_SLOT, WRONG_TOKEN, INVALID_ENTRY); ENUM(volume_state, uint32_t, INIT, // created, not yet online diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index 235e825..fbd84c0 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -24,9 +24,30 @@ #include // iomanager singleton, reactor_regex #include // value_awaitable: lock-free completion-before-suspend-safe bridge +#include +#include +#include + namespace homeblocks { // ─── Journal entry on-disk format ───────────────────────────────────────────── + +namespace { +// fetch_data's contract is one entry per requested LSN (never one that wasn't asked for, never +// repeated). Returns the first response LSN that violates it (unrequested or duplicated), or nullopt if +// every entry matches exactly one requested LSN. Erasing from `pending` as we go catches duplicates for +// free: a repeated lsn finds nothing left to erase the second time. +std::optional< int64_t > validate_fetch_response(std::vector< int64_t > const& requested, + std::vector< JournalSlot > const& response) { + std::unordered_set< int64_t > pending{requested.begin(), requested.end()}; + for (auto const& slot : response) { + if (pending.erase(slot.lsn) == 0) return slot.lsn; + } + return std::nullopt; +} +} // namespace + +// ─── HomeStore journal backend ──────────────────────────────────────────────── // // Each log slot is: [CraftJournalEntry header][serialized multi_blk_id bytes]. // The payload (HS_DATA_LINKED) is written directly to the data service; only the @@ -500,6 +521,12 @@ void CraftReplDev::CraftRaftListener::on_commit(int64_t lsn, sisl::blob const& h } // apply_sync_rs_commit_lsn co_awaits peer fetch + journal writes; on_commit itself is a synchronous // HomeStore callback, so fire-and-forget it. + // + // FIXME: KNOWN GAP (not yet fixed): this coroutine captures only the raw `owner_` pointer, not anything + // that keeps CraftReplDev alive. If the object is destroyed (e.g. volume removal) while this + // coroutine is suspended inside fetch_from_peer()/write_slot(), it resumes into freed memory -- + // use-after-free. Two possible fixes: + // Check comments: https://github.com/sbinmalek/HomeBlocks/pull/2#discussion_r3761568811 detail::detach(owner_->apply_sync_rs_commit_lsn(payload->rs_commit_lsn, payload->client_token, std::move(*empty_slots))); break; @@ -518,14 +545,31 @@ void CraftReplDev::CraftRaftListener::on_commit(int64_t lsn, sisl::blob const& h // // apply_sync_rs_commit_lsn (22886): client_token is verified against the current session first -- a mismatch // gates the ENTIRE apply (no reconciliation, no catch-up, no watermark advance), since a RAFT entry whose -// token doesn't match the live session shouldn't be trusted to describe it. Once the token matches, every +// token doesn't match the live session shouldn't be trusted to describe it. empty_slots is range-checked +// against rs_commit_lsn next, for the same reason and with the same all-or-nothing gate: SyncRSCommitLSN +// verdicts are only ever defined for slots the leader pre-resolved up to rs_commit_lsn (S5), so a negative +// or out-of-range entry is a malformed/corrupt RAFT entry, not a legitimate verdict -- trusting it would +// permanently poison empty_lsns_ for a slot that hasn't even been reached yet. Once both checks pass, every // other step is best-effort forward progress: empty_slots are reconciled and the newly-spanned range is // marked missing, catch-up attempts to fill in what it can from a peer, and commit_lsn/last_append_lsn // advance regardless of whether catch-up fully succeeded -- mirroring truncate()'s invariant that apply -// never reverts the watermark, only advances it. +// never reverts the watermark, only advances it. A peer's fetch_data response gets its own all-or-nothing +// check (validate_fetch_response): unlike the two checks above, this one can't gate the whole apply (gap +// marking and last_append_lsn already advanced by the time the response arrives), so a malformed response +// is instead treated exactly like a failed fetch -- none of it applied, everything requested stays missing. async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint64_t client_token, std::vector< int64_t > empty_slots) { + // Validated before any state is touched -- same all-or-nothing gate as the token check below, since an + // out-of-range verdict means the entry itself cannot be trusted, not that this one slot should be skipped. + for (int64_t lsn : empty_slots) { + if (lsn < 0 || lsn > rs_commit_lsn) { + LOGE("apply_sync_rs_commit_lsn: empty_slots lsn={} out of range [0, {}] -- rejecting entire apply", + lsn, rs_commit_lsn); + co_return std::unexpected(make_error_condition(volume_error::INVALID_ENTRY)); + } + } + std::vector< int64_t > to_fetch; uint64_t term; { @@ -561,6 +605,14 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 } else if (auto fetched = co_await peer_fetcher_->fetch_data(to_fetch); !fetched) { LOGE("apply_sync_rs_commit_lsn: fetch_data failed: {} -- leaving {} lsn(s) as missing", fetched.error().message(), to_fetch.size()); + } else if (auto bad_lsn = validate_fetch_response(to_fetch, *fetched); bad_lsn) { + // fetch_data's contract is one entry per requested LSN (never one we didn't ask for, never + // repeated) -- any deviation means the response itself can't be trusted, so none of it is + // applied (same outcome as a fetch failure) rather than cherry-picking the entries that look + // fine from a peer that has already proven unreliable. + LOGE("apply_sync_rs_commit_lsn: peer response lsn={} not requested (or duplicated) -- rejecting " + "entire batch, leaving {} lsn(s) as missing", + *bad_lsn, to_fetch.size()); } else { for (auto& slot : *fetched) { if (slot.is_empty) { diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index 5acd930..8c1e9e5 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -301,6 +301,8 @@ class CraftReplDev { void on_config_rollback(int64_t) override {} private: + // KNOWN GAP: no lifetime guarantee across the detached apply_sync_rs_commit_lsn coroutine -- see + // the on_commit call site in craft_repl_dev.cpp for the full use-after-free writeup. CraftReplDev* owner_; }; diff --git a/src/lib/craft/tests/test_craft_raft_entries.cpp b/src/lib/craft/tests/test_craft_raft_entries.cpp index 88282e6..e08c0c3 100644 --- a/src/lib/craft/tests/test_craft_raft_entries.cpp +++ b/src/lib/craft/tests/test_craft_raft_entries.cpp @@ -161,6 +161,35 @@ TEST_F(CraftRaftEntriesTest, TokenMismatchSkipsWholeApply) { EXPECT_EQ(dev_->missing_count(), 1u); } +// ── empty_slots range validation ────────────────────────────────────────────── + +// A negative LSN in empty_slots is nonsensical for a SyncRSCommitLSN verdict -- reject the whole apply, +// the same all-or-nothing gate as a token mismatch, before any state is touched. +TEST_F(CraftRaftEntriesTest, RejectsEmptySlotWithNegativeLSN) { + dev_->seed_lsns(5, {3}); + auto r = do_apply(/*rs_commit_lsn=*/10, /*client_token=*/0, /*empty_slots=*/{-1}); + + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(volume_error::INVALID_ENTRY)); + EXPECT_EQ(dev_->commit_lsn(), -1); + EXPECT_EQ(dev_->last_append_lsn(), 5); + EXPECT_EQ(dev_->missing_count(), 1u); +} + +// An empty_slots entry above rs_commit_lsn names a slot the leader never pre-resolved (S5 only resolves +// up to the LSN it proposes) -- reject the whole apply rather than let it poison empty_lsns_ for a slot +// that hasn't even been reached yet. +TEST_F(CraftRaftEntriesTest, RejectsEmptySlotAboveRSCommitLSN) { + dev_->seed_lsns(5, {3}); + auto r = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/0, /*empty_slots=*/{6}); + + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(volume_error::INVALID_ENTRY)); + EXPECT_EQ(dev_->commit_lsn(), -1); + EXPECT_EQ(dev_->last_append_lsn(), 5); + EXPECT_EQ(dev_->missing_count(), 1u); +} + // ── empty_slots reconciliation ──────────────────────────────────────────────── TEST_F(CraftRaftEntriesTest, EmptySlotsReconciled) { @@ -241,6 +270,48 @@ TEST_F(CraftRaftEntriesTest, BehindWithPeerFetcherAppliesFetchedSlots) { EXPECT_EQ(dev_->last_append_lsn(), 2); } +// fetch_data's contract is one entry per requested LSN. A response naming an LSN we never asked for (a +// buggy/misbehaving peer) can't be partially trusted -- since gap-marking already advanced by this point +// in the apply, this can't gate the whole apply the way the upfront empty_slots check does, but it CAN +// still refuse the batch: none of the response is applied (same outcome as a fetch failure), even the +// entries that individually look fine, and commit_lsn still advances (best-effort). +TEST_F(CraftRaftEntriesTest, BehindRejectsPeerResponseWithUnrequestedLSN) { + dev_->set_peer_fetcher(&fetcher_); + dev_->seed_lsns(0, {}); + fetcher_.response = { + JournalSlot{.lsn = 1, .lba_off_bytes = 10, .len_bytes = 4}, + JournalSlot{.lsn = 2, .is_empty = true}, + JournalSlot{.lsn = 99, .is_empty = true}, // never requested -- only 1 and 2 were + }; + + auto r = do_apply(/*rs_commit_lsn=*/2, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_FALSE(journal_->has_slot(1)); + EXPECT_FALSE(dev_->is_empty_slot(2)); + EXPECT_FALSE(dev_->is_empty_slot(99)); + EXPECT_EQ(dev_->missing_count(), 2u); // 1 and 2 both remain missing + EXPECT_EQ(dev_->commit_lsn(), 2); +} + +// A duplicate entry for an actually-requested LSN is just as much a contract violation as an +// unrequested one (validate_fetch_response catches both the same way) -- same whole-batch rejection. +TEST_F(CraftRaftEntriesTest, BehindRejectsPeerResponseWithDuplicateLSN) { + dev_->set_peer_fetcher(&fetcher_); + dev_->seed_lsns(0, {}); + fetcher_.response = { + JournalSlot{.lsn = 1, .lba_off_bytes = 10, .len_bytes = 4}, + JournalSlot{.lsn = 1, .lba_off_bytes = 20, .len_bytes = 4}, // duplicate + }; + + auto r = do_apply(/*rs_commit_lsn=*/1, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_FALSE(journal_->has_slot(1)); + EXPECT_EQ(dev_->missing_count(), 1u); + EXPECT_EQ(dev_->commit_lsn(), 1); +} + // fetch_data fails outright: commit_lsn still advances (best-effort); every spanned LSN remains missing. TEST_F(CraftRaftEntriesTest, BehindFetchFailsStillAdvancesCommitLsn) { dev_->set_peer_fetcher(&fetcher_); From 53999142a89551c5c835cb52fe3fd7161ceba3c3 Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Tue, 11 Aug 2026 17:31:21 -0700 Subject: [PATCH 04/16] SDSTOR-22887 craft: InternalLogin RAFT entry apply - implement apply_internal_login: overwrite client_token, max-guard term against regression; synchronous, called directly from on_commit (no detail::detach -- no I/O to await) - wire on_commit's InternalLogin dispatch with an exact-size key check (no variable trailing data, unlike SyncRSCommitLSN) - fix write()'s pre-existing unlocked read of state_.term -- latent until now since nothing mutated it; this ticket arms the race - add client_token()/term() observability accessors - add tests: dispatch success/wrong-size, second-login replaces session, term-never-regresses vs token-always-overwrites, write() term-fencing end-to-end, and cross-entry-type interaction with apply_sync_rs_commit_lsn's token check --- src/lib/craft/craft_repl_dev.cpp | 49 +++++- src/lib/craft/craft_repl_dev.hpp | 8 + .../craft/tests/test_craft_raft_entries.cpp | 159 +++++++++++++++++- 3 files changed, 206 insertions(+), 10 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index fbd84c0..e39a1b6 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -313,7 +313,9 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 { std::lock_guard lock{missing_mu_}; - // Term check is inside the lock: state_.term is mutated by apply_internal_login (S5) under the same mutex. + // state_.term is guarded by missing_mu_ like the rest of state_ -- read it under the same lock + // used for the gap-marking below rather than unlocked, now that apply_internal_login (22887) + // actually mutates it from the RAFT commit thread. if (hdr.term != state_.term) { LOGW("write rejected: stale term want={} got={} dlsn={}", state_.term, hdr.term, dlsn); co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); @@ -527,14 +529,33 @@ void CraftReplDev::CraftRaftListener::on_commit(int64_t lsn, sisl::blob const& h // coroutine is suspended inside fetch_from_peer()/write_slot(), it resumes into freed memory -- // use-after-free. Two possible fixes: // Check comments: https://github.com/sbinmalek/HomeBlocks/pull/2#discussion_r3761568811 + // + // FIXME: KNOWN GAP (not yet fixed), distinct from the lifetime issue above: detaching here also + // breaks strict RAFT apply ordering. on_commit returns to HomeStore as soon as this coroutine hits + // its first co_await, so HomeStore can call on_commit for the NEXT committed entry -- a synchronous + // InternalLogin, or another detached SyncRSCommitLSN -- before this one's effects are fully applied. + // No individual field access races (missing_mu_ still guards every access), but replicas can end up + // applying entries in different effective orders depending on async completion timing, which + // violates the determinism RAFT relies on for replicas to converge. See the commit_lsn advance at + // the tail of apply_sync_rs_commit_lsn and the client_token overwrite in apply_internal_login for + // the two mutation points this exposes. Real fix: one per-device serialized apply queue that both + // entry types funnel through, processing one entry's full effect (including all its co_awaits) + // before starting the next -- not independent detached tasks. detail::detach(owner_->apply_sync_rs_commit_lsn(payload->rs_commit_lsn, payload->client_token, std::move(*empty_slots))); break; } - case CraftEntryType::InternalLogin: - // Dispatch lands with 22887. - LOGD("on_commit lsn={} InternalLogin (dispatch not yet implemented)", lsn); + case CraftEntryType::InternalLogin: { + // Fixed-size payload, no variable trailing data (unlike SyncRSCommitLSN) -- exact-size check. + if (key.size() != sizeof(InternalLoginPayload)) { + LOGE("on_commit lsn={} InternalLogin key wrong size ({} bytes)", lsn, key.size()); + return; + } + const auto* login_payload = reinterpret_cast< const InternalLoginPayload* >(key.cbytes()); + // Pure in-memory state transition (no co_await) -- called directly, not detached. + owner_->apply_internal_login(login_payload->client_token, login_payload->term); break; + } default: LOGE("on_commit lsn={} unrecognized CraftEntryType={}", lsn, static_cast< uint8_t >(entry_hdr->type)); break; @@ -648,6 +669,10 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 // Unconditional: commit_lsn is a replica-set-wide watermark RAFT already agreed on, independent of // whether this replica's local catch-up succeeded. + // + // KNOWN GAP: this can land late. Because on_commit detaches this coroutine (see the FIXME there), + // a later-committed entry (InternalLogin, or another SyncRSCommitLSN) may have already applied by + // the time this advance actually runs, breaking strict RAFT apply ordering. { std::lock_guard lk{missing_mu_}; state_.commit_lsn = std::max(state_.commit_lsn, rs_commit_lsn); @@ -656,8 +681,22 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 co_return ok(); } +// ─── InternalLogin apply (S5 / SDSTOR-22887) ───────────────────────────────── +// +// Pure in-memory state transition -- no journal I/O, no peer fetch -- so this stays synchronous +// (unlike apply_sync_rs_commit_lsn) and on_commit calls it directly rather than via detail::detach(). +// "Enforce single-writer exclusivity" needs no explicit rejection here: every other RPC's term-fence +// check (STALE_TERM on mismatch) already does that. Overwriting state_.term is what invalidates any +// existing session -- a caller still presenting the old term is fenced out on its very next call. + void CraftReplDev::apply_internal_login(uint64_t client_token, uint64_t term) { - LOGD("apply_internal_login client_token={} term={} (not yet implemented)", client_token, term); + std::lock_guard lk{missing_mu_}; + state_.client_token = client_token; // opaque id, no ordering semantics -- plain overwrite + // term is RAFT-ordered in practice (the leader always proposes strictly increasing terms), but + // guard against regression the same way commit_lsn/last_append_lsn already do rather than trusting + // log order blindly. + state_.term = std::max(state_.term, term); + LOGD("apply_internal_login client_token={} term={}", client_token, state_.term); } } // namespace homeblocks diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index 8c1e9e5..2bdffce 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -226,6 +226,14 @@ class CraftReplDev { std::lock_guard lk{missing_mu_}; return state_.commit_lsn; } + uint64_t client_token() const { + std::lock_guard lk{missing_mu_}; + return state_.client_token; + } + uint64_t term() const { + std::lock_guard lk{missing_mu_}; + return state_.term; + } // Wires the server-to-server peer channel used by apply_sync_rs_commit_lsn catch-up. // Called by CraftConnector (S9) after construction; tests inject a mock. diff --git a/src/lib/craft/tests/test_craft_raft_entries.cpp b/src/lib/craft/tests/test_craft_raft_entries.cpp index e08c0c3..fe3cb42 100644 --- a/src/lib/craft/tests/test_craft_raft_entries.cpp +++ b/src/lib/craft/tests/test_craft_raft_entries.cpp @@ -13,21 +13,28 @@ * *********************************************************************************/ -// Unit tests for CraftReplDev::apply_sync_rs_commit_lsn and the on_commit SyncRSCommitLSN dispatch -// (S5 / SDSTOR-22886). InternalLogin apply/dispatch is still a stub (lands with SDSTOR-22887) and is -// not covered here. +// Unit tests for CraftReplDev's two RAFT entry applies -- SyncRSCommitLSN (S5 / SDSTOR-22886) and +// InternalLogin (S5 / SDSTOR-22887) -- and their on_commit dispatch. // -// Tests verify: +// SyncRSCommitLSN tests verify: // - a client_token mismatch gates the ENTIRE apply: no reconciliation, no catch-up, no watermark advance -// - empty_slots are reconciled into empty_lsns_ and erased from missing_lsns_ +// - empty_slots are range-validated against rs_commit_lsn and reconciled into empty_lsns_/missing_lsns_ // - commit_lsn/last_append_lsn advance directly when there's no gap to catch up on // - fetch_data is invoked with exactly the missing LSNs when behind, and its response is persisted +// - a peer response naming an unrequested or duplicate LSN is rejected as a whole batch // - catch-up is best-effort: a failed fetch, a failed write_slot, or no peer_fetcher_ at all still lets // commit_lsn advance, leaving unresolved LSNs in missing_lsns_ // - commit_lsn never decrements // - on_commit parses a real serialized SyncRSCommitLSN entry and dispatches correctly (and rejects // malformed header/key blobs without touching state) // +// InternalLogin tests verify: +// - on_commit dispatches to apply_internal_login, which sets client_token/term +// - a wrong-size key (too short or too long) is rejected, state untouched +// - a second InternalLogin replaces client_token outright but never regresses term +// - once applied, write()'s term-fence check reflects the new term end-to-end +// - the client_token InternalLogin establishes is what apply_sync_rs_commit_lsn's token check uses +// // This TU defines SISL_LOGGING_DEF for the homeblocks module because it compiles craft_repl_dev.cpp // directly (same pattern as test_craft_truncate.cpp). @@ -119,6 +126,14 @@ std::vector< uint8_t > make_sync_rs_commit_lsn_key(int64_t rs_commit_lsn, uint64 return buf; } +std::vector< uint8_t > make_internal_login_key(uint64_t client_token, uint64_t term) { + std::vector< uint8_t > buf(sizeof(InternalLoginPayload)); + auto* p = reinterpret_cast< InternalLoginPayload* >(buf.data()); + p->client_token = client_token; + p->term = term; + return buf; +} + } // namespace // craft_repl_dev.hpp friends this exact type (homeblocks::CraftRaftEntriesTest) so it can call the @@ -415,6 +430,140 @@ TEST_F(CraftRaftEntriesTest, OnCommitIgnoresUnrecognizedEntryType) { EXPECT_EQ(dev_->commit_lsn(), -1); // untouched } +// ── InternalLogin apply (SDSTOR-22887) ──────────────────────────────────────── + +TEST_F(CraftRaftEntriesTest, OnCommitDispatchesInternalLogin) { + auto header_buf = make_header(CraftEntryType::InternalLogin); + auto key_buf = make_internal_login_key(/*client_token=*/42, /*term=*/5); + cintrusive< homestore::repl_req_ctx > ctx{}; + + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); + + EXPECT_EQ(dev_->client_token(), 42u); + EXPECT_EQ(dev_->term(), 5u); +} + +TEST_F(CraftRaftEntriesTest, OnCommitRejectsInternalLoginWrongSize) { + auto header_buf = make_header(CraftEntryType::InternalLogin); + std::vector< uint8_t > short_key(sizeof(InternalLoginPayload) - 1, 0); + cintrusive< homestore::repl_req_ctx > ctx{}; + + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(short_key), {}, ctx); + + EXPECT_EQ(dev_->client_token(), 0u); // untouched + EXPECT_EQ(dev_->term(), 0u); // untouched +} + +// Unlike SyncRSCommitLSN's coarser "at least the fixed prefix" check (it has variable trailing data), +// InternalLoginPayload never does -- on_commit's check is an exact-size `!=`, so a key that's too LARGE +// must be rejected just as much as one that's too small. +TEST_F(CraftRaftEntriesTest, OnCommitRejectsInternalLoginKeyTooLarge) { + auto header_buf = make_header(CraftEntryType::InternalLogin); + std::vector< uint8_t > long_key(sizeof(InternalLoginPayload) + 1, 0); + cintrusive< homestore::repl_req_ctx > ctx{}; + + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(long_key), {}, ctx); + + EXPECT_EQ(dev_->client_token(), 0u); // untouched + EXPECT_EQ(dev_->term(), 0u); // untouched +} + +// "A second InternalLogin invalidates any existing session before establishing the new one" (ticket) -- +// the second apply's values must win outright, not merge with the first's. Driven through on_commit +// (apply_internal_login itself is private, and TEST_F bodies live in a class derived from +// CraftRaftEntriesTest -- friendship doesn't propagate to it, so the public dispatch path is used here). +TEST_F(CraftRaftEntriesTest, SecondInternalLoginReplacesSession) { + auto header_buf = make_header(CraftEntryType::InternalLogin); + cintrusive< homestore::repl_req_ctx > ctx{}; + + auto key1 = make_internal_login_key(/*client_token=*/1, /*term=*/1); + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key1), {}, ctx); + auto key2 = make_internal_login_key(/*client_token=*/2, /*term=*/2); + dev_->test_listener().on_commit(2, as_blob(header_buf), as_blob(key2), {}, ctx); + + EXPECT_EQ(dev_->client_token(), 2u); + EXPECT_EQ(dev_->term(), 2u); +} + +TEST_F(CraftRaftEntriesTest, InternalLoginTermNeverRegresses) { + auto header_buf = make_header(CraftEntryType::InternalLogin); + cintrusive< homestore::repl_req_ctx > ctx{}; + + auto key1 = make_internal_login_key(/*client_token=*/1, /*term=*/5); + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key1), {}, ctx); + auto key2 = make_internal_login_key(/*client_token=*/2, /*term=*/3); + dev_->test_listener().on_commit(2, as_blob(header_buf), as_blob(key2), {}, ctx); + + EXPECT_EQ(dev_->term(), 5u); +} + +// client_token has no ordering semantics (it's an opaque id, unlike term) -- it's a plain overwrite even +// when the accompanying term regresses and is guarded. Pins down the intentional asymmetry between the +// two fields so it doesn't read as an oversight to a future reader. +TEST_F(CraftRaftEntriesTest, InternalLoginClientTokenOverwrittenEvenWhenTermRegresses) { + auto header_buf = make_header(CraftEntryType::InternalLogin); + cintrusive< homestore::repl_req_ctx > ctx{}; + + auto key1 = make_internal_login_key(/*client_token=*/1, /*term=*/5); + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key1), {}, ctx); + auto key2 = make_internal_login_key(/*client_token=*/99, /*term=*/3); // lower term, different token + dev_->test_listener().on_commit(2, as_blob(header_buf), as_blob(key2), {}, ctx); + + EXPECT_EQ(dev_->term(), 5u); // guarded against regression + EXPECT_EQ(dev_->client_token(), 99u); // overwritten regardless +} + +// Happy-path regression check for the write()-term-check-under-lock reorg: a matching term still +// succeeds normally (only the mismatch path changed). +TEST_F(CraftRaftEntriesTest, WriteSucceedsWithMatchingTermAfterInternalLogin) { + auto header_buf = make_header(CraftEntryType::InternalLogin); + auto key_buf = make_internal_login_key(/*client_token=*/1, /*term=*/5); + cintrusive< homestore::repl_req_ctx > ctx{}; + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); + + auto r = homeblocks::detail::sync_get( + dev_->write(craft::client_hdr{.term = 5, .commit_lsn = -1, .all_committed_lsn = -1}, /*dlsn=*/1, + /*addr=*/0, /*len=*/0, {}, /*all_zeros=*/true)); + + ASSERT_TRUE(r.has_value()); +} + +// End-to-end: once InternalLogin moves state_.term forward, a write() still presenting the old term is +// fenced out on its very next call -- the "invalidation" this ticket calls for, and the regression test +// for write()'s term-check-under-lock fix. +TEST_F(CraftRaftEntriesTest, WriteRejectsStaleTermAfterInternalLogin) { + auto header_buf = make_header(CraftEntryType::InternalLogin); + auto key_buf = make_internal_login_key(/*client_token=*/1, /*term=*/5); + cintrusive< homestore::repl_req_ctx > ctx{}; + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); + + auto r = homeblocks::detail::sync_get( + dev_->write(craft::client_hdr{.term = 4, .commit_lsn = -1, .all_committed_lsn = -1}, /*dlsn=*/1, + /*addr=*/0, /*len=*/0, {}, /*all_zeros=*/true)); + + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(volume_error::STALE_TERM)); +} + +// Cross-entry-type integration: apply_sync_rs_commit_lsn's client_token check reads the SAME state_ +// InternalLogin writes. Untestable before this ticket (state_.client_token was permanently 0, matching +// every SyncRSCommitLSN test's default). Once InternalLogin establishes a new token, do_apply must use +// it -- the OLD default (0) is now itself a mismatch. +TEST_F(CraftRaftEntriesTest, SyncRSCommitLSNUsesTokenEstablishedByInternalLogin) { + auto header_buf = make_header(CraftEntryType::InternalLogin); + auto key_buf = make_internal_login_key(/*client_token=*/7, /*term=*/1); + cintrusive< homestore::repl_req_ctx > ctx{}; + dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); + + auto stale = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/0); // the old default -- now stale + ASSERT_FALSE(stale.has_value()); + EXPECT_EQ(stale.error(), make_error_condition(volume_error::WRONG_TOKEN)); + + auto fresh = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/7); // the token InternalLogin just set + ASSERT_TRUE(fresh.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 5); +} + } // namespace } // namespace homeblocks From 0f7f036ab9b0203f23a108bb216488ae5180a4eb Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Fri, 14 Aug 2026 14:36:47 -0700 Subject: [PATCH 05/16] craft: drop redundant get_lsns() alias, use unordered_set for empty_lsns_ - get_rs_commit_lsn() already covered the same snapshot; empty_lsns_ doesn't need ordering. --- src/lib/craft/craft_repl_dev.cpp | 16 ++++------- src/lib/craft/craft_repl_dev.hpp | 9 +++--- src/lib/craft/tests/CMakeLists.txt | 2 +- .../craft/tests/test_craft_peer_exchange.cpp | 28 +++++-------------- 4 files changed, 19 insertions(+), 36 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index e39a1b6..90dc6e7 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -199,19 +199,10 @@ unique< CraftJournalBackend > make_homestore_journal_backend(shared< homestore:: CraftReplDev::CraftReplDev(volume_id_t vol_id, unique< CraftJournalBackend > journal) : vol_id_{vol_id}, journal_{std::move(journal)}, raft_listener_{this} {} -// ─── get_lsns / get_rs_commit_lsn ──────────────────────────────────────────── +// ─── get_rs_commit_lsn ──────────────────────────────────────────── // Snapshot the in-memory partition state under missing_mu_ for consistency with // write() which updates state_ under the same lock. -async_result< craft::lsn_pair > CraftReplDev::get_lsns(volume_id_t /* vol_id */) { - craft::lsn_pair pair{}; - { - std::lock_guard lk{missing_mu_}; - pair = {state_.commit_lsn, state_.last_append_lsn}; - } - co_return pair; -} - async_result< craft::lsn_pair > CraftReplDev::get_rs_commit_lsn(uint64_t /* term */, bool /* is_login */) { craft::lsn_pair pair{}; { @@ -465,6 +456,11 @@ async_status CraftReplDev::append(int64_t /* sync_to */, uint64_t /* client_toke // while fetch_data runs), so the snapshot taken under the lock is stable. // // A read_slot() I/O error aborts the batch immediately (fail-fast); the partial result is discarded. +// +// TODO: the loop below re-acquires missing_mu_ once per requested LSN. Since the snapshot is +// already documented as stable for the whole batch (no concurrent writes during fetch_data), +// classification for every LSN could be done under a single lock acquisition up front instead -- +// same result, fewer lock/unlock round trips for large batches. async_result< std::vector< JournalSlot > > CraftReplDev::fetch_data(std::vector< int64_t > lsns) { std::vector< JournalSlot > result; diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index 2bdffce..5d5014a 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -22,6 +22,7 @@ #include #include #include +#include #include namespace homestore { @@ -179,9 +180,6 @@ class CraftReplDev { // ── internal / peer API (server-to-server; NEVER reachable over the client wire) ── - // Return {commit_lsn, last_append_lsn} for the local partition. - async_result< craft::lsn_pair > get_lsns(volume_id_t vol_id); - // Callee side of the GetRSCommitLSN broadcast -- matches craft::craft_peer::get_rs_commit_lsn's // shape (craft_client's include/craft/peer.hpp) so a future wire-decoded request has somewhere // to pass {term, is_login}. is_login=true is meant to quiesce prior-session writes before @@ -324,8 +322,11 @@ class CraftReplDev { volume_id_t vol_id_; unique< CraftJournalBackend > journal_; CraftPartitionState state_; + // TODO: Can this be replaced with boost::icl::interval_set? Particularly helpful when a write + // comes in with a huge gap -- gap-fill loops (write(), apply_sync_rs_commit_lsn()) currently + // insert one LSN at a time under missing_mu_, which is O(gap width) instead of O(log ranges). std::set< int64_t > missing_lsns_; // gaps between commit_lsn and last_append_lsn - std::set< int64_t > empty_lsns_; // slots positively verdicted Empty by a prior SyncRSCommitLSN (S5) + std::unordered_set< int64_t > empty_lsns_; // slots positively verdicted Empty by a prior SyncRSCommitLSN (S5) mutable std::mutex missing_mu_; // guards state_, missing_lsns_, and empty_lsns_ bool login_in_progress_{false}; std::mutex login_mu_; diff --git a/src/lib/craft/tests/CMakeLists.txt b/src/lib/craft/tests/CMakeLists.txt index b661ae5..cb9644f 100644 --- a/src/lib/craft/tests/CMakeLists.txt +++ b/src/lib/craft/tests/CMakeLists.txt @@ -15,7 +15,7 @@ target_link_libraries(test_craft_truncate add_test(NAME CraftTruncateTest COMMAND test_craft_truncate) -# Unit tests for CraftReplDev::get_lsns(), get_rs_commit_lsn(), and fetch_data() (S6). +# Unit tests for CraftReplDev::get_rs_commit_lsn() and fetch_data() (S6). # Same pattern as test_craft_truncate: compile craft_repl_dev.cpp directly. add_executable(test_craft_peer_exchange) target_sources(test_craft_peer_exchange PRIVATE diff --git a/src/lib/craft/tests/test_craft_peer_exchange.cpp b/src/lib/craft/tests/test_craft_peer_exchange.cpp index 11598ba..4afa374 100644 --- a/src/lib/craft/tests/test_craft_peer_exchange.cpp +++ b/src/lib/craft/tests/test_craft_peer_exchange.cpp @@ -13,8 +13,7 @@ * *********************************************************************************/ -// Unit tests for CraftReplDev::get_lsns(), get_rs_commit_lsn(), and fetch_data() -// (S6: Peer Data Exchange APIs). +// Unit tests for CraftReplDev::get_rs_commit_lsn() and fetch_data() (S6: Peer Data Exchange APIs). // // fetch_data() implements a four-way response per slot: // present+data : slot in journal, all_zeros=false @@ -85,7 +84,6 @@ class CraftPeerExchangeTest : public ::testing::Test { dev_ = std::make_unique< CraftReplDev >(volume_id_t{}, std::move(mock)); } - auto do_get_lsns() { return homeblocks::detail::sync_get(dev_->get_lsns(volume_id_t{})); } auto do_get_rs_commit_lsn() { return homeblocks::detail::sync_get(dev_->get_rs_commit_lsn(0, false)); } auto do_fetch_data(std::vector< int64_t > lsns) { return homeblocks::detail::sync_get(dev_->fetch_data(std::move(lsns))); @@ -100,38 +98,26 @@ class CraftPeerExchangeTest : public ::testing::Test { std::unique_ptr< CraftReplDev > dev_; }; -// ── get_lsns / get_rs_commit_lsn ───────────────────────────────────────────── +// ── get_rs_commit_lsn ──────────────────────────────────────────────────────── // Fresh device: both watermarks default to -1 (uninitialized sentinel). -TEST_F(CraftPeerExchangeTest, GetLsnsDefaultState) { - auto r = do_get_lsns(); +TEST_F(CraftPeerExchangeTest, GetRsCommitLsnDefaultState) { + auto r = do_get_rs_commit_lsn(); ASSERT_TRUE(r.has_value()); EXPECT_EQ(r->commit_lsn, -1); EXPECT_EQ(r->last_append_lsn, -1); } -// After seeding, get_lsns reflects both watermarks correctly. -TEST_F(CraftPeerExchangeTest, GetLsnsAfterSeed) { +// After seeding, get_rs_commit_lsn reflects both watermarks correctly. +TEST_F(CraftPeerExchangeTest, GetRsCommitLsnAfterSeed) { dev_->seed_lsns(50, {30, 40}); dev_->seed_commit_lsn(25); - auto r = do_get_lsns(); + auto r = do_get_rs_commit_lsn(); ASSERT_TRUE(r.has_value()); EXPECT_EQ(r->commit_lsn, 25); EXPECT_EQ(r->last_append_lsn, 50); } -// get_rs_commit_lsn is an alias of get_lsns; both must return the same snapshot. -TEST_F(CraftPeerExchangeTest, GetRsCommitLsnMatchesGetLsns) { - dev_->seed_lsns(100, {}); - dev_->seed_commit_lsn(80); - auto lsns_r = do_get_lsns(); - auto rs_r = do_get_rs_commit_lsn(); - ASSERT_TRUE(lsns_r.has_value()); - ASSERT_TRUE(rs_r.has_value()); - EXPECT_EQ(lsns_r->commit_lsn, rs_r->commit_lsn); - EXPECT_EQ(lsns_r->last_append_lsn, rs_r->last_append_lsn); -} - // ── fetch_data ──────────────────────────────────────────────────────────────── // Requesting zero LSNs returns an empty result vector, not an error. From 20673532b73c3525154aec28756b6f06591d3ff1 Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Sun, 16 Aug 2026 18:30:56 -0700 Subject: [PATCH 06/16] craft: bound SyncRSCommitLSN peer catch-up fetch with a configurable timeout CraftPeerFetcher::fetch_from_peer() had no deadline, so an unresponsive peer could hang apply_sync_rs_commit_lsn's catch-up path forever. Adds peer_fetch_timeout_ms (home_blks_config.fbs, default 5000ms) as a CraftReplDev member with a setter, threaded through to fetch_from_peer's new timeout_ms parameter -- kept off the global config singleton so the standalone craft test binaries (which don't link homeblocks_core) still build. Co-Authored-By: Claude Sonnet 5 --- src/lib/craft/craft_repl_dev.cpp | 3 ++- src/lib/craft/craft_repl_dev.hpp | 10 ++++++++-- .../craft/tests/test_craft_raft_entries.cpp | 18 +++++++++++++++++- src/lib/home_blks_config.fbs | 4 ++++ 4 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index 90dc6e7..53d5ec0 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -619,7 +619,8 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 if (peer_fetcher_ == nullptr) { LOGW("apply_sync_rs_commit_lsn: {} lsn(s) missing but no peer_fetcher_ wired -- leaving as missing", to_fetch.size()); - } else if (auto fetched = co_await peer_fetcher_->fetch_data(to_fetch); !fetched) { + } else if (auto fetched = co_await peer_fetcher_->fetch_data(to_fetch, peer_fetch_timeout_ms_); + !fetched) { LOGE("apply_sync_rs_commit_lsn: fetch_data failed: {} -- leaving {} lsn(s) as missing", fetched.error().message(), to_fetch.size()); } else if (auto bad_lsn = validate_fetch_response(to_fetch, *fetched); bad_lsn) { diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index 5d5014a..d132765 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -105,7 +105,8 @@ unique< CraftJournalBackend > make_homestore_journal_backend(shared< homestore:: class CraftPeerFetcher { public: virtual async_result< craft::lsn_pair > get_rs_commit_lsn(uint64_t term, bool is_login) = 0; - virtual async_result< std::vector< JournalSlot > > fetch_data(const std::vector< int64_t >& lsns) = 0; + virtual async_result< std::vector< JournalSlot > > fetch_data(const std::vector< int64_t >& lsns, + uint32_t timeout_ms) = 0; virtual ~CraftPeerFetcher() = default; }; @@ -237,6 +238,10 @@ class CraftReplDev { // Called by CraftConnector (S9) after construction; tests inject a mock. void set_peer_fetcher(CraftPeerFetcher* f) { peer_fetcher_ = f; } + // Overrides the deadline passed to fetch_from_peer (default mirrors home_blks_config.fbs). + // Production sets this from HB_DYNAMIC_CONFIG(peer_fetch_timeout_ms) after construction (S8/S9). + void set_peer_fetch_timeout_ms(uint32_t ms) { peer_fetch_timeout_ms_ = ms; } + #ifdef _PRERELEASE // Seeds partition watermarks and the missing set directly, bypassing write(). // Only compiled when _PRERELEASE is defined; never present in production binaries. @@ -331,7 +336,8 @@ class CraftReplDev { bool login_in_progress_{false}; std::mutex login_mu_; CraftRaftListener raft_listener_; - CraftPeerFetcher* peer_fetcher_{nullptr}; // null until S9 wires CraftConnector + CraftPeerFetcher* peer_fetcher_{nullptr}; // null until S9 wires CraftConnector + uint32_t peer_fetch_timeout_ms_{5000}; // TODO: deadline for fetch_data; set from config at construction (S8/S9) std::atomic< uint64_t > write_counter_{0}; // incremented per write(); triggers periodic SyncRSCommitLSN append }; diff --git a/src/lib/craft/tests/test_craft_raft_entries.cpp b/src/lib/craft/tests/test_craft_raft_entries.cpp index fe3cb42..dcb1860 100644 --- a/src/lib/craft/tests/test_craft_raft_entries.cpp +++ b/src/lib/craft/tests/test_craft_raft_entries.cpp @@ -95,6 +95,7 @@ class MockCraftJournalBackend : public CraftJournalBackend { class MockCraftPeerFetcher : public CraftPeerFetcher { public: std::vector< int64_t > last_requested; + uint32_t last_timeout_ms{0}; std::vector< JournalSlot > response; bool should_fail{false}; @@ -102,8 +103,10 @@ class MockCraftPeerFetcher : public CraftPeerFetcher { co_return craft::lsn_pair{}; } - async_result< std::vector< JournalSlot > > fetch_data(const std::vector< int64_t >& lsns) override { + async_result< std::vector< JournalSlot > > fetch_data(const std::vector< int64_t >& lsns, + uint32_t timeout_ms) override { last_requested = lsns; + last_timeout_ms = timeout_ms; if (should_fail) co_return std::unexpected(std::make_error_condition(std::errc::io_error)); co_return response; } @@ -285,6 +288,19 @@ TEST_F(CraftRaftEntriesTest, BehindWithPeerFetcherAppliesFetchedSlots) { EXPECT_EQ(dev_->last_append_lsn(), 2); } +// set_peer_fetch_timeout_ms() threads the configured deadline through to fetch_data verbatim. +TEST_F(CraftRaftEntriesTest, BehindPassesConfiguredTimeoutToPeerFetcher) { + dev_->set_peer_fetcher(&fetcher_); + dev_->set_peer_fetch_timeout_ms(1234); + dev_->seed_lsns(0, {}); + fetcher_.response = {JournalSlot{.lsn = 1, .lba_off_bytes = 10, .len_bytes = 4}}; + + auto r = do_apply(/*rs_commit_lsn=*/1, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(fetcher_.last_timeout_ms, 1234u); +} + // fetch_data's contract is one entry per requested LSN. A response naming an LSN we never asked for (a // buggy/misbehaving peer) can't be partially trusted -- since gap-marking already advanced by this point // in the apply, this can't gate the whole apply the way the upfront empty_slots check does, but it CAN diff --git a/src/lib/home_blks_config.fbs b/src/lib/home_blks_config.fbs index f0d5ae9..56ad7d4 100644 --- a/src/lib/home_blks_config.fbs +++ b/src/lib/home_blks_config.fbs @@ -24,6 +24,10 @@ table HomeBlksSettings{ // how often (in appended LSNs) the leader auto-proposes a SyncRSCommitLSN entry; sync_rs_commit_lsn_interval: uint32 = 128; + + // deadline (in milliseconds) for a server-to-server peer fetch_data() call; + // a peer that misses it is treated as unreachable, same as a hard failure. + peer_fetch_timeout_ms: uint32 = 5000; } root_type HomeBlksSettings; From 94ddacf5a5e579d3a8c73cac2e374be50a4f1b3f Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Mon, 17 Aug 2026 09:25:59 -0700 Subject: [PATCH 07/16] craft: fix on_commit detached-coroutine use-after-free, enforce shared_ptr ownership - CraftReplDev now extends std::enable_shared_from_this; apply_sync_rs_commit_lsn opens with `auto self = shared_from_this()` so the detached coroutine holds a strong reference across every co_await, keeping CraftReplDev alive even if the last external owner (e.g. a volume-removal path) drops its shared_ptr mid-apply. Closes the KNOWN GAP flagged in review (PR #2, discussion r3761568811). - CraftReplDev's constructor is now private; construction only via the new CraftReplDev::create() factory, so shared_from_this()'s "must already be shared_ptr-owned" precondition is enforced by the compiler instead of a comment. - Update the four craft test fixtures from make_unique/unique_ptr to CraftReplDev::create()/shared_ptr. --- src/lib/craft/craft_repl_dev.cpp | 23 +++++++++++-------- src/lib/craft/craft_repl_dev.hpp | 23 +++++++++++++++---- .../craft/tests/test_craft_peer_exchange.cpp | 4 ++-- .../craft/tests/test_craft_raft_entries.cpp | 4 ++-- src/lib/craft/tests/test_craft_truncate.cpp | 4 ++-- src/lib/craft/tests/test_craft_write.cpp | 4 ++-- 6 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index 53d5ec0..2e1e990 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -520,16 +520,17 @@ void CraftReplDev::CraftRaftListener::on_commit(int64_t lsn, sisl::blob const& h // apply_sync_rs_commit_lsn co_awaits peer fetch + journal writes; on_commit itself is a synchronous // HomeStore callback, so fire-and-forget it. // - // FIXME: KNOWN GAP (not yet fixed): this coroutine captures only the raw `owner_` pointer, not anything - // that keeps CraftReplDev alive. If the object is destroyed (e.g. volume removal) while this - // coroutine is suspended inside fetch_from_peer()/write_slot(), it resumes into freed memory -- - // use-after-free. Two possible fixes: - // Check comments: https://github.com/sbinmalek/HomeBlocks/pull/2#discussion_r3761568811 + // Lifetime: on_commit itself only touches the raw `owner_` pointer, which is safe since HomeStore + // never calls on_commit on a dead device. The DETACHED coroutine this dispatches into is a separate + // concern -- apply_sync_rs_commit_lsn opens with `auto self = shared_from_this()`, so the coroutine + // frame holds a strong reference across every co_await, keeping CraftReplDev alive even if every + // external owner (e.g. a volume-removal path) drops its shared_ptr mid-apply. Requires every + // CraftReplDev to be owned via shared_ptr // - // FIXME: KNOWN GAP (not yet fixed), distinct from the lifetime issue above: detaching here also - // breaks strict RAFT apply ordering. on_commit returns to HomeStore as soon as this coroutine hits - // its first co_await, so HomeStore can call on_commit for the NEXT committed entry -- a synchronous - // InternalLogin, or another detached SyncRSCommitLSN -- before this one's effects are fully applied. + // FIXME: KNOWN GAP (not yet fixed): detaching here also breaks strict RAFT apply ordering. + // on_commit returns to HomeStore as soon as this coroutine hits its first co_await, so + // HomeStore can call on_commit for the NEXT committed entry -- a synchronous InternalLogin, or + // another detached SyncRSCommitLSN -- before this one's effects are fully applied. // No individual field access races (missing_mu_ still guards every access), but replicas can end up // applying entries in different effective orders depending on async completion timing, which // violates the determinism RAFT relies on for replicas to converge. See the commit_lsn advance at @@ -577,6 +578,10 @@ void CraftReplDev::CraftRaftListener::on_commit(int64_t lsn, sisl::blob const& h async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint64_t client_token, std::vector< int64_t > empty_slots) { + // Lives in the coroutine frame across every co_await below -- see the lifetime comment at the + // on_commit call site (detail::detach) for why this is required. + auto self = shared_from_this(); + // Validated before any state is touched -- same all-or-nothing gate as the token check below, since an // out-of-range verdict means the entry itself cannot be trusted, not that this one slot should be skipped. for (int64_t lsn : empty_slots) { diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index d132765..a3b08fc 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -105,6 +106,11 @@ unique< CraftJournalBackend > make_homestore_journal_backend(shared< homestore:: class CraftPeerFetcher { public: virtual async_result< craft::lsn_pair > get_rs_commit_lsn(uint64_t term, bool is_login) = 0; + // `timeout_ms` is the deadline this call must complete within (CraftReplDev passes + // peer_fetch_timeout_ms_, set from home_blks_config.fbs's peer_fetch_timeout_ms). A real transport + // (S9) must treat a missed deadline as a hard failure, same as an unreachable peer -- this interface + // only carries the contract; there's nothing to enforce yet since today's only implementations are + // direct function calls (production is unwired, tests call synchronously). virtual async_result< std::vector< JournalSlot > > fetch_data(const std::vector< int64_t >& lsns, uint32_t timeout_ms) = 0; virtual ~CraftPeerFetcher() = default; @@ -116,15 +122,22 @@ class CraftPeerFetcher { // (write, read, login, truncate, ...) on top of a HomeStore log store and // index. Non-CRAFT volumes are unaffected. -class CraftReplDev { +class CraftReplDev : public std::enable_shared_from_this< CraftReplDev > { #ifdef _PRERELEASE // Lets test_craft_raft_entries.cpp call apply_sync_rs_commit_lsn (private) directly, so it can assert // on the exact result rather than only on-commit's discarded fire-and-forget outcome. friend class CraftRaftEntriesTest; #endif -public: + // Private -- see create() below. shared_from_this() (used by apply_sync_rs_commit_lsn's detached + // coroutine) requires the object to already be owned by a shared_ptr, so construction is gated behind + // create() rather than exposed directly. explicit CraftReplDev(volume_id_t vol_id, unique< CraftJournalBackend > journal); + +public: + static shared< CraftReplDev > create(volume_id_t vol_id, unique< CraftJournalBackend > journal) { + return shared< CraftReplDev >(new CraftReplDev(vol_id, std::move(journal))); + } ~CraftReplDev() = default; // ── client-facing ────────────────────────────────────────────────────── @@ -312,8 +325,8 @@ class CraftReplDev { void on_config_rollback(int64_t) override {} private: - // KNOWN GAP: no lifetime guarantee across the detached apply_sync_rs_commit_lsn coroutine -- see - // the on_commit call site in craft_repl_dev.cpp for the full use-after-free writeup. + // Back-pointer to the owning CraftReplDev -- raft_listener_ is a value member of CraftReplDev + // (see its declaration below), so this can never dangle CraftReplDev* owner_; }; @@ -337,7 +350,7 @@ class CraftReplDev { std::mutex login_mu_; CraftRaftListener raft_listener_; CraftPeerFetcher* peer_fetcher_{nullptr}; // null until S9 wires CraftConnector - uint32_t peer_fetch_timeout_ms_{5000}; // TODO: deadline for fetch_data; set from config at construction (S8/S9) + uint32_t peer_fetch_timeout_ms_{5000}; // deadline for fetch_data; overridden via set_peer_fetch_timeout_ms() std::atomic< uint64_t > write_counter_{0}; // incremented per write(); triggers periodic SyncRSCommitLSN append }; diff --git a/src/lib/craft/tests/test_craft_peer_exchange.cpp b/src/lib/craft/tests/test_craft_peer_exchange.cpp index 4afa374..1b2329d 100644 --- a/src/lib/craft/tests/test_craft_peer_exchange.cpp +++ b/src/lib/craft/tests/test_craft_peer_exchange.cpp @@ -81,7 +81,7 @@ class CraftPeerExchangeTest : public ::testing::Test { void SetUp() override { auto mock = std::make_unique< MockCraftJournalBackend >(); journal_ = mock.get(); - dev_ = std::make_unique< CraftReplDev >(volume_id_t{}, std::move(mock)); + dev_ = CraftReplDev::create(volume_id_t{}, std::move(mock)); } auto do_get_rs_commit_lsn() { return homeblocks::detail::sync_get(dev_->get_rs_commit_lsn(0, false)); } @@ -95,7 +95,7 @@ class CraftPeerExchangeTest : public ::testing::Test { } MockCraftJournalBackend* journal_{nullptr}; - std::unique_ptr< CraftReplDev > dev_; + std::shared_ptr< CraftReplDev > dev_; }; // ── get_rs_commit_lsn ──────────────────────────────────────────────────────── diff --git a/src/lib/craft/tests/test_craft_raft_entries.cpp b/src/lib/craft/tests/test_craft_raft_entries.cpp index dcb1860..fc48e87 100644 --- a/src/lib/craft/tests/test_craft_raft_entries.cpp +++ b/src/lib/craft/tests/test_craft_raft_entries.cpp @@ -150,7 +150,7 @@ class CraftRaftEntriesTest : public ::testing::Test { void SetUp() override { auto mock = std::make_unique< MockCraftJournalBackend >(); journal_ = mock.get(); - dev_ = std::make_unique< CraftReplDev >(volume_id_t{}, std::move(mock)); + dev_ = CraftReplDev::create(volume_id_t{}, std::move(mock)); } auto do_apply(int64_t rs_commit_lsn, uint64_t client_token, std::vector< int64_t > empty_slots = {}) { @@ -160,7 +160,7 @@ class CraftRaftEntriesTest : public ::testing::Test { MockCraftJournalBackend* journal_{nullptr}; MockCraftPeerFetcher fetcher_; - std::unique_ptr< CraftReplDev > dev_; + std::shared_ptr< CraftReplDev > dev_; }; namespace { diff --git a/src/lib/craft/tests/test_craft_truncate.cpp b/src/lib/craft/tests/test_craft_truncate.cpp index 894f242..380ebc2 100644 --- a/src/lib/craft/tests/test_craft_truncate.cpp +++ b/src/lib/craft/tests/test_craft_truncate.cpp @@ -73,13 +73,13 @@ class CraftTruncateTest : public ::testing::Test { void SetUp() override { auto mock = std::make_unique< MockCraftJournalBackend >(); journal_ = mock.get(); - dev_ = std::make_unique< CraftReplDev >(volume_id_t{}, std::move(mock)); + dev_ = CraftReplDev::create(volume_id_t{}, std::move(mock)); } auto do_truncate(int64_t lsn) { return homeblocks::detail::sync_get(dev_->truncate(lsn)); } MockCraftJournalBackend* journal_{nullptr}; - std::unique_ptr< CraftReplDev > dev_; + std::shared_ptr< CraftReplDev > dev_; }; // ── tests ───────────────────────────────────────────────────────────────────── diff --git a/src/lib/craft/tests/test_craft_write.cpp b/src/lib/craft/tests/test_craft_write.cpp index 3c57db0..b7364b0 100644 --- a/src/lib/craft/tests/test_craft_write.cpp +++ b/src/lib/craft/tests/test_craft_write.cpp @@ -90,7 +90,7 @@ class CraftWriteTest : public ::testing::Test { void SetUp() override { auto mock = std::make_unique< MockCraftJournalBackend >(); journal_ = mock.get(); - dev_ = std::make_unique< CraftReplDev >(volume_id_t{}, std::move(mock)); + dev_ = CraftReplDev::create(volume_id_t{}, std::move(mock)); } auto do_write(uint64_t term, int64_t lsn, bool all_zeros = true) { @@ -108,7 +108,7 @@ class CraftWriteTest : public ::testing::Test { } MockCraftJournalBackend* journal_{nullptr}; - std::unique_ptr< CraftReplDev > dev_; + std::shared_ptr< CraftReplDev > dev_; }; // Each lsn arrives exactly one step ahead: no gap, no missing entries after each write. From 6ab3c6e9366e1dff828b5e4f802a58579363d746 Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Tue, 1 Sep 2026 13:47:43 -0700 Subject: [PATCH 08/16] Fixed code style and bumped version --- conanfile.py | 2 +- src/lib/craft/craft_repl_dev.cpp | 27 +++++++++---------- src/lib/craft/craft_repl_dev.hpp | 12 ++++----- .../craft/tests/test_craft_raft_entries.cpp | 23 ++++++++-------- 4 files changed, 31 insertions(+), 33 deletions(-) diff --git a/conanfile.py b/conanfile.py index 17f0c34..52eac79 100644 --- a/conanfile.py +++ b/conanfile.py @@ -10,7 +10,7 @@ class HomeBlocksConan(ConanFile): name = "homeblocks" - version = "6.0.6" + version = "6.0.7" homepage = "https://github.com/eBay/HomeBlocks" description = "Block Store built on HomeStore" diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index 2e1e990..4187e82 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -38,7 +38,7 @@ namespace { // every entry matches exactly one requested LSN. Erasing from `pending` as we go catches duplicates for // free: a repeated lsn finds nothing left to erase the second time. std::optional< int64_t > validate_fetch_response(std::vector< int64_t > const& requested, - std::vector< JournalSlot > const& response) { + std::vector< JournalSlot > const& response) { std::unordered_set< int64_t > pending{requested.begin(), requested.end()}; for (auto const& slot : response) { if (pending.erase(slot.lsn) == 0) return slot.lsn; @@ -512,7 +512,7 @@ void CraftReplDev::CraftRaftListener::on_commit(int64_t lsn, sisl::blob const& h return; } const auto* payload = reinterpret_cast< const SyncRSCommitLSNPayload* >(key.cbytes()); - auto empty_slots = parse_empty_slots(key); + auto empty_slots = parse_empty_slots(key); if (!empty_slots) { LOGE("on_commit lsn={} SyncRSCommitLSN malformed empty_slots", lsn); return; @@ -538,8 +538,8 @@ void CraftReplDev::CraftRaftListener::on_commit(int64_t lsn, sisl::blob const& h // the two mutation points this exposes. Real fix: one per-device serialized apply queue that both // entry types funnel through, processing one entry's full effect (including all its co_awaits) // before starting the next -- not independent detached tasks. - detail::detach(owner_->apply_sync_rs_commit_lsn(payload->rs_commit_lsn, payload->client_token, - std::move(*empty_slots))); + detail::detach( + owner_->apply_sync_rs_commit_lsn(payload->rs_commit_lsn, payload->client_token, std::move(*empty_slots))); break; } case CraftEntryType::InternalLogin: { @@ -586,8 +586,8 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 // out-of-range verdict means the entry itself cannot be trusted, not that this one slot should be skipped. for (int64_t lsn : empty_slots) { if (lsn < 0 || lsn > rs_commit_lsn) { - LOGE("apply_sync_rs_commit_lsn: empty_slots lsn={} out of range [0, {}] -- rejecting entire apply", - lsn, rs_commit_lsn); + LOGE("apply_sync_rs_commit_lsn: empty_slots lsn={} out of range [0, {}] -- rejecting entire apply", lsn, + rs_commit_lsn); co_return std::unexpected(make_error_condition(volume_error::INVALID_ENTRY)); } } @@ -598,7 +598,7 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 std::lock_guard lk{missing_mu_}; if (client_token != state_.client_token) { LOGW("apply_sync_rs_commit_lsn: client_token mismatch want={} got={} rs_commit_lsn={} -- skipping apply", - state_.client_token, client_token, rs_commit_lsn); + state_.client_token, client_token, rs_commit_lsn); co_return std::unexpected(make_error_condition(volume_error::WRONG_TOKEN)); } term = state_.term; @@ -623,11 +623,10 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 if (!to_fetch.empty()) { if (peer_fetcher_ == nullptr) { LOGW("apply_sync_rs_commit_lsn: {} lsn(s) missing but no peer_fetcher_ wired -- leaving as missing", - to_fetch.size()); - } else if (auto fetched = co_await peer_fetcher_->fetch_data(to_fetch, peer_fetch_timeout_ms_); - !fetched) { + to_fetch.size()); + } else if (auto fetched = co_await peer_fetcher_->fetch_data(to_fetch, peer_fetch_timeout_ms_); !fetched) { LOGE("apply_sync_rs_commit_lsn: fetch_data failed: {} -- leaving {} lsn(s) as missing", - fetched.error().message(), to_fetch.size()); + fetched.error().message(), to_fetch.size()); } else if (auto bad_lsn = validate_fetch_response(to_fetch, *fetched); bad_lsn) { // fetch_data's contract is one entry per requested LSN (never one we didn't ask for, never // repeated) -- any deviation means the response itself can't be trusted, so none of it is @@ -635,7 +634,7 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 // fine from a peer that has already proven unreliable. LOGE("apply_sync_rs_commit_lsn: peer response lsn={} not requested (or duplicated) -- rejecting " "entire batch, leaving {} lsn(s) as missing", - *bad_lsn, to_fetch.size()); + *bad_lsn, to_fetch.size()); } else { for (auto& slot : *fetched) { if (slot.is_empty) { @@ -651,7 +650,7 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 auto alloc_res = co_await journal_->alloc_write_data(slot.data, slot.len_bytes); if (!alloc_res) { LOGE("apply_sync_rs_commit_lsn: alloc_write_data failed lsn={}: {} -- leaving as missing", - slot.lsn, alloc_res.error().message()); + slot.lsn, alloc_res.error().message()); continue; } blkid = *alloc_res; @@ -660,7 +659,7 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 slot.all_zeros); if (!res) { LOGE("apply_sync_rs_commit_lsn: write_slot failed lsn={}: {} -- leaving as missing", slot.lsn, - res.error().message()); + res.error().message()); continue; } std::lock_guard lk{missing_mu_}; diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index a3b08fc..05cc7a9 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -112,7 +112,7 @@ class CraftPeerFetcher { // only carries the contract; there's nothing to enforce yet since today's only implementations are // direct function calls (production is unwired, tests call synchronously). virtual async_result< std::vector< JournalSlot > > fetch_data(const std::vector< int64_t >& lsns, - uint32_t timeout_ms) = 0; + uint32_t timeout_ms) = 0; virtual ~CraftPeerFetcher() = default; }; @@ -343,14 +343,14 @@ class CraftReplDev : public std::enable_shared_from_this< CraftReplDev > { // TODO: Can this be replaced with boost::icl::interval_set? Particularly helpful when a write // comes in with a huge gap -- gap-fill loops (write(), apply_sync_rs_commit_lsn()) currently // insert one LSN at a time under missing_mu_, which is O(gap width) instead of O(log ranges). - std::set< int64_t > missing_lsns_; // gaps between commit_lsn and last_append_lsn - std::unordered_set< int64_t > empty_lsns_; // slots positively verdicted Empty by a prior SyncRSCommitLSN (S5) - mutable std::mutex missing_mu_; // guards state_, missing_lsns_, and empty_lsns_ + std::set< int64_t > missing_lsns_; // gaps between commit_lsn and last_append_lsn + std::unordered_set< int64_t > empty_lsns_; // slots positively verdicted Empty by a prior SyncRSCommitLSN (S5) + mutable std::mutex missing_mu_; // guards state_, missing_lsns_, and empty_lsns_ bool login_in_progress_{false}; std::mutex login_mu_; CraftRaftListener raft_listener_; - CraftPeerFetcher* peer_fetcher_{nullptr}; // null until S9 wires CraftConnector - uint32_t peer_fetch_timeout_ms_{5000}; // deadline for fetch_data; overridden via set_peer_fetch_timeout_ms() + CraftPeerFetcher* peer_fetcher_{nullptr}; // null until S9 wires CraftConnector + uint32_t peer_fetch_timeout_ms_{5000}; // deadline for fetch_data; overridden via set_peer_fetch_timeout_ms() std::atomic< uint64_t > write_counter_{0}; // incremented per write(); triggers periodic SyncRSCommitLSN append }; diff --git a/src/lib/craft/tests/test_craft_raft_entries.cpp b/src/lib/craft/tests/test_craft_raft_entries.cpp index fc48e87..11a896d 100644 --- a/src/lib/craft/tests/test_craft_raft_entries.cpp +++ b/src/lib/craft/tests/test_craft_raft_entries.cpp @@ -104,7 +104,7 @@ class MockCraftPeerFetcher : public CraftPeerFetcher { } async_result< std::vector< JournalSlot > > fetch_data(const std::vector< int64_t >& lsns, - uint32_t timeout_ms) override { + uint32_t timeout_ms) override { last_requested = lsns; last_timeout_ms = timeout_ms; if (should_fail) co_return std::unexpected(std::make_error_condition(std::errc::io_error)); @@ -123,7 +123,7 @@ std::vector< uint8_t > make_header(CraftEntryType type) { } std::vector< uint8_t > make_sync_rs_commit_lsn_key(int64_t rs_commit_lsn, uint64_t client_token, - const std::vector< int64_t >& empty_slots) { + const std::vector< int64_t >& empty_slots) { std::vector< uint8_t > buf(sync_rs_commit_lsn_key_size(empty_slots.size())); serialize_sync_rs_commit_lsn(buf.data(), rs_commit_lsn, client_token, empty_slots); return buf; @@ -131,9 +131,9 @@ std::vector< uint8_t > make_sync_rs_commit_lsn_key(int64_t rs_commit_lsn, uint64 std::vector< uint8_t > make_internal_login_key(uint64_t client_token, uint64_t term) { std::vector< uint8_t > buf(sizeof(InternalLoginPayload)); - auto* p = reinterpret_cast< InternalLoginPayload* >(buf.data()); + auto* p = reinterpret_cast< InternalLoginPayload* >(buf.data()); p->client_token = client_token; - p->term = term; + p->term = term; return buf; } @@ -310,8 +310,7 @@ TEST_F(CraftRaftEntriesTest, BehindRejectsPeerResponseWithUnrequestedLSN) { dev_->set_peer_fetcher(&fetcher_); dev_->seed_lsns(0, {}); fetcher_.response = { - JournalSlot{.lsn = 1, .lba_off_bytes = 10, .len_bytes = 4}, - JournalSlot{.lsn = 2, .is_empty = true}, + JournalSlot{.lsn = 1, .lba_off_bytes = 10, .len_bytes = 4}, JournalSlot{.lsn = 2, .is_empty = true}, JournalSlot{.lsn = 99, .is_empty = true}, // never requested -- only 1 and 2 were }; @@ -393,7 +392,7 @@ TEST_F(CraftRaftEntriesTest, WriteSlotFailureDuringCatchupLeavesLsnMissing) { TEST_F(CraftRaftEntriesTest, OnCommitDispatchesSyncRSCommitLSN) { auto header_buf = make_header(CraftEntryType::SyncRSCommitLSN); - auto key_buf = make_sync_rs_commit_lsn_key(/*rs_commit_lsn=*/7, /*client_token=*/0, /*empty_slots=*/{}); + auto key_buf = make_sync_rs_commit_lsn_key(/*rs_commit_lsn=*/7, /*client_token=*/0, /*empty_slots=*/{}); cintrusive< homestore::repl_req_ctx > ctx{}; dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); @@ -426,7 +425,7 @@ TEST_F(CraftRaftEntriesTest, OnCommitRejectsMalformedSyncRSCommitLSNKey) { // check (not on_commit's coarser size check) is what rejects it. TEST_F(CraftRaftEntriesTest, OnCommitRejectsMismatchedEmptySlotsCount) { auto header_buf = make_header(CraftEntryType::SyncRSCommitLSN); - auto key_buf = make_sync_rs_commit_lsn_key(7, 0, {10, 20}); + auto key_buf = make_sync_rs_commit_lsn_key(7, 0, {10, 20}); reinterpret_cast< SyncRSCommitLSNPayload* >(key_buf.data())->num_empty_slots = 5; cintrusive< homestore::repl_req_ctx > ctx{}; @@ -450,7 +449,7 @@ TEST_F(CraftRaftEntriesTest, OnCommitIgnoresUnrecognizedEntryType) { TEST_F(CraftRaftEntriesTest, OnCommitDispatchesInternalLogin) { auto header_buf = make_header(CraftEntryType::InternalLogin); - auto key_buf = make_internal_login_key(/*client_token=*/42, /*term=*/5); + auto key_buf = make_internal_login_key(/*client_token=*/42, /*term=*/5); cintrusive< homestore::repl_req_ctx > ctx{}; dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); @@ -533,7 +532,7 @@ TEST_F(CraftRaftEntriesTest, InternalLoginClientTokenOverwrittenEvenWhenTermRegr // succeeds normally (only the mismatch path changed). TEST_F(CraftRaftEntriesTest, WriteSucceedsWithMatchingTermAfterInternalLogin) { auto header_buf = make_header(CraftEntryType::InternalLogin); - auto key_buf = make_internal_login_key(/*client_token=*/1, /*term=*/5); + auto key_buf = make_internal_login_key(/*client_token=*/1, /*term=*/5); cintrusive< homestore::repl_req_ctx > ctx{}; dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); @@ -549,7 +548,7 @@ TEST_F(CraftRaftEntriesTest, WriteSucceedsWithMatchingTermAfterInternalLogin) { // for write()'s term-check-under-lock fix. TEST_F(CraftRaftEntriesTest, WriteRejectsStaleTermAfterInternalLogin) { auto header_buf = make_header(CraftEntryType::InternalLogin); - auto key_buf = make_internal_login_key(/*client_token=*/1, /*term=*/5); + auto key_buf = make_internal_login_key(/*client_token=*/1, /*term=*/5); cintrusive< homestore::repl_req_ctx > ctx{}; dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); @@ -567,7 +566,7 @@ TEST_F(CraftRaftEntriesTest, WriteRejectsStaleTermAfterInternalLogin) { // it -- the OLD default (0) is now itself a mismatch. TEST_F(CraftRaftEntriesTest, SyncRSCommitLSNUsesTokenEstablishedByInternalLogin) { auto header_buf = make_header(CraftEntryType::InternalLogin); - auto key_buf = make_internal_login_key(/*client_token=*/7, /*term=*/1); + auto key_buf = make_internal_login_key(/*client_token=*/7, /*term=*/1); cintrusive< homestore::repl_req_ctx > ctx{}; dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); From fcdfc6dc345f48f02ae49aad1b1c14b2e8ad95f7 Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Thu, 3 Sep 2026 10:56:57 -0700 Subject: [PATCH 09/16] craft: batch missing_mu_ lock acquisitions in fetch_data and empty_slots apply - fetch_data: classify all requested LSNs under one missing_mu_ acquisition instead of re-locking per LSN. - apply_sync_rs_commit_lsn: range-insert empty_slots into empty_lsns_ instead of inserting one at a time. --- src/lib/craft/craft_repl_dev.cpp | 50 ++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index 4187e82..6272987 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -451,44 +451,50 @@ async_status CraftReplDev::append(int64_t /* sync_to */, uint64_t /* client_toke // empty_lsns_ is checked first: a slot in both empty_lsns_ and the journal returns is_empty=true // (Empty beats data, the reconciliation invariant from S5). // -// The missing_mu_ lock is dropped before each co_await read_slot() call to avoid holding a mutex -// across a suspension point. Callers are serialised by the login sequence (no concurrent writes -// while fetch_data runs), so the snapshot taken under the lock is stable. +// The missing_mu_ lock is held only for the up-front classification pass below, dropped before any +// co_await read_slot() call to avoid holding a mutex across a suspension point. Callers are +// serialised by the login sequence (no concurrent writes while fetch_data runs), so the snapshot +// taken under the lock is stable for the whole batch. // // A read_slot() I/O error aborts the batch immediately (fail-fast); the partial result is discarded. -// -// TODO: the loop below re-acquires missing_mu_ once per requested LSN. Since the snapshot is -// already documented as stable for the whole batch (no concurrent writes during fetch_data), -// classification for every LSN could be done under a single lock acquisition up front instead -- -// same result, fewer lock/unlock round trips for large batches. async_result< std::vector< JournalSlot > > CraftReplDev::fetch_data(std::vector< int64_t > lsns) { - std::vector< JournalSlot > result; - result.reserve(lsns.size()); + enum class SlotKind { Empty, Present, Absent }; - for (int64_t lsn : lsns) { - enum class SlotKind { Empty, Present, Absent }; - SlotKind kind; - { - std::lock_guard lk{missing_mu_}; + std::vector< SlotKind > kinds; + kinds.reserve(lsns.size()); + { + std::lock_guard lk{missing_mu_}; + for (int64_t lsn : lsns) { if (empty_lsns_.contains(lsn)) { - kind = SlotKind::Empty; + kinds.push_back(SlotKind::Empty); } else if (lsn >= 0 && lsn <= state_.last_append_lsn && !missing_lsns_.contains(lsn)) { - kind = SlotKind::Present; + kinds.push_back(SlotKind::Present); } else { - kind = SlotKind::Absent; + kinds.push_back(SlotKind::Absent); } } + } - if (kind == SlotKind::Empty) { + std::vector< JournalSlot > result; + result.reserve(lsns.size()); + + for (size_t i = 0; i < lsns.size(); ++i) { + const int64_t lsn = lsns[i]; + switch (kinds[i]) { + case SlotKind::Empty: result.push_back(JournalSlot{.lsn = lsn, .is_empty = true}); - } else if (kind == SlotKind::Present) { + break; + case SlotKind::Present: { auto slot_r = co_await journal_->read_slot(lsn); if (!slot_r) co_return std::unexpected(slot_r.error()); slot_r->lsn = lsn; result.push_back(std::move(*slot_r)); + break; + } + case SlotKind::Absent: + break; // omit from result (not-present-here) } - // Absent: omit from result (not-present-here) } co_return result; @@ -604,9 +610,9 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 term = state_.term; for (int64_t lsn : empty_slots) { - empty_lsns_.insert(lsn); missing_lsns_.erase(lsn); } + empty_lsns_.insert(empty_slots.begin(), empty_slots.end()); // Everything newly spanned by this advance that isn't Empty-verdicted is a gap until catch-up // (below) resolves it -- same idiom write() uses for gaps opened by an out-of-order dlsn. From 612e3aa9235db1b260c94ee2e8e05677b481bca3 Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Thu, 3 Sep 2026 15:00:22 -0700 Subject: [PATCH 10/16] craft: reclaim leaked blocks in apply_sync_rs_commit_lsn's two cleanup paths - Empty-verdict reconciliation (to_free): an LSN that was in missing_lsns_ and gets verdicted Empty by this SyncRSCommitLSN may still hold a locally written block from an earlier write() attempt. That block was never reclaimed -- only missing_lsns_ was cleared. Added CraftJournalBackend::free_slot(lsn), which reads the raw local journal entry back off the log store and frees the blkid it references (skipping all_zeros entries, which never allocated one) via the existing free_data. It bypasses read_slot/JournalSlot deliberately: that type is wire-shared with craft::JournalSlot for peer fetch_data responses and carries no blkid (meaningless to a remote peer), so it can't serve this local-only need. - Peer-catchup write_slot failure: alloc_write_data can succeed and then write_slot fail, leaving an allocated block referenced by nothing. This path had no cleanup at all. Now frees it, guarded by blkid_allocated so all_zeros slots (which never allocate) aren't passed to free_data -- mirroring the guard write() already has. The free itself is dispatched via detail::detach() as its own coroutine capturing `self` (not just journal_), since it can outlive the enclosing apply_sync_rs_commit_lsn coroutine, which may return -- and drop its own `self` -- first. - Added free_slot to the four MockCraftJournalBackend test doubles; factored the now-duplicated read_slot/free_slot bodies (identical across test_craft_write.cpp, test_craft_raft_entries.cpp, and test_craft_peer_exchange.cpp) into a new mock_journal_backend.hpp. Co-Authored-By: Claude Sonnet 5 --- src/lib/craft/craft_repl_dev.cpp | 54 ++++++++++++++++++- src/lib/craft/craft_repl_dev.hpp | 7 +++ src/lib/craft/tests/mock_journal_backend.hpp | 45 ++++++++++++++++ .../craft/tests/test_craft_peer_exchange.cpp | 8 +-- .../craft/tests/test_craft_raft_entries.cpp | 10 ++-- src/lib/craft/tests/test_craft_truncate.cpp | 4 ++ src/lib/craft/tests/test_craft_write.cpp | 10 ++-- 7 files changed, 121 insertions(+), 17 deletions(-) create mode 100644 src/lib/craft/tests/mock_journal_backend.hpp diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index 6272987..cf3ed58 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -184,6 +184,33 @@ class HomeStoreCraftJournalBackend : public CraftJournalBackend { co_return ok(); } + // Reads the raw local entry back off the log store -- the exact bytes write_slot wrote, + // header + serialized blkid -- and hands the blkid to free_data. Never goes through + // read_slot/JournalSlot: that type is wire-shared with craft::JournalSlot for peer fetch_data + // responses and deliberately carries no blkid (meaningless to a remote peer). + async_status free_slot(int64_t lsn) override { + homestore::log_buffer buf; + try { + buf = logstore_->read_sync(static_cast< homestore::logstore_seq_num_t >(lsn)); + } catch (std::exception const& e) { + LOGE("free_slot: read_sync failed lsn={}: {}", lsn, e.what()); + co_return std::unexpected(std::make_error_condition(std::errc::io_error)); + } + if (buf.size() < sizeof(CraftJournalEntry)) { + LOGE("free_slot: entry truncated lsn={} size={}", lsn, buf.size()); + co_return std::unexpected(std::make_error_condition(std::errc::io_error)); + } + CraftJournalEntry hdr{}; + std::memcpy(&hdr, buf.bytes(), sizeof(CraftJournalEntry)); + if (hdr.all_zeros) co_return ok(); + + homestore::multi_blk_id blkid{}; + blkid.deserialize(sisl::blob{buf.bytes() + sizeof(CraftJournalEntry), + buf.size() - static_cast< uint32_t >(sizeof(CraftJournalEntry))}, + true /* copy */); + co_return co_await free_data(blkid); + } + private: shared< homestore::home_log_store > logstore_; uint64_t vol_ordinal_; @@ -598,6 +625,7 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 } } + std::vector< int64_t > to_free; std::vector< int64_t > to_fetch; uint64_t term; { @@ -610,7 +638,9 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 term = state_.term; for (int64_t lsn : empty_slots) { - missing_lsns_.erase(lsn); + if (missing_lsns_.erase(lsn)) { + to_free.push_back(lsn); + } } empty_lsns_.insert(empty_slots.begin(), empty_slots.end()); @@ -626,6 +656,15 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 } } + if (!to_free.empty()) { + for (int64_t lsn : to_free) { + if (auto fr = co_await journal_->free_slot(lsn); !fr) { + LOGE("apply_sync_rs_commit_lsn: free_slot failed lsn={}: {} -- blocks may leak", lsn, + fr.error().message()); + } + } + } + if (!to_fetch.empty()) { if (peer_fetcher_ == nullptr) { LOGW("apply_sync_rs_commit_lsn: {} lsn(s) missing but no peer_fetcher_ wired -- leaving as missing", @@ -652,6 +691,7 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 // HS_DATA_LINKED, same as write(): allocate blocks and write the payload before // journalling the block reference. all_zeros slots carry no data and skip alloc. homestore::multi_blk_id blkid{}; + bool blkid_allocated = false; if (!slot.all_zeros) { auto alloc_res = co_await journal_->alloc_write_data(slot.data, slot.len_bytes); if (!alloc_res) { @@ -660,12 +700,24 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 continue; } blkid = *alloc_res; + blkid_allocated = true; + } + + //FIXME: We need to address the case when blkid is not set. How would write_slot handle that? auto res = co_await journal_->write_slot(slot.lsn, term, slot.lba_off_bytes, slot.len_bytes, blkid, slot.all_zeros); if (!res) { LOGE("apply_sync_rs_commit_lsn: write_slot failed lsn={}: {} -- leaving as missing", slot.lsn, res.error().message()); + if (blkid_allocated) { + detail::detach([self, blkid, lsn = slot.lsn]() -> async_status { + if (auto fr = co_await self->journal_->free_data(blkid); !fr) + LOGE("apply_sync_rs_commit_lsn: free_data failed after write_slot failure lsn={}: {}", + lsn, fr.error().message()); + co_return ok(); + }()); + } continue; } std::lock_guard lk{missing_mu_}; diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index 05cc7a9..f33fc0a 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -86,6 +86,13 @@ class CraftJournalBackend { // Release blocks previously allocated by alloc_write_data. Called when write_slot fails or // when the write is discarded post-flight (stale term). Free errors are logged but non-fatal. virtual async_status free_data(homestore::multi_blk_id blkid) = 0; + // TODO: Need to revisit this if this func can be avoided + // Reads the already-committed local entry at lsn and, if it isn't all_zeros, frees the blkid + // it references via free_data. Local-only by design: unlike read_slot/JournalSlot (the + // wire-shared type used to answer a peer's fetch_data), a blkid has no meaning off this + // replica, so this never needs to leave the local backend. Used by apply_sync_rs_commit_lsn's + // to_free path to reclaim blocks under an entry a later SyncRSCommitLSN verdicts Empty. + virtual async_status free_slot(int64_t lsn) = 0; virtual ~CraftJournalBackend() = default; }; diff --git a/src/lib/craft/tests/mock_journal_backend.hpp b/src/lib/craft/tests/mock_journal_backend.hpp new file mode 100644 index 0000000..fb1321b --- /dev/null +++ b/src/lib/craft/tests/mock_journal_backend.hpp @@ -0,0 +1,45 @@ +/********************************************************************************* + * Modifications Copyright 2026 eBay Inc. + * + * 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 + * https://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. + * + *********************************************************************************/ +#pragma once + +#include "craft/craft_repl_dev.hpp" + +// Shared free_slot body for CraftJournalBackend test mocks that record write_slot calls into a +// std::map< int64_t, JournalSlot > slots member. test_craft_write.cpp, test_craft_raft_entries.cpp, +// and test_craft_peer_exchange.cpp all need identical behavior here: read back the recorded slot, +// skip it if all_zeros (nothing was ever allocated), otherwise delegate to the mock's own free_data +// so any per-test free_data call counters still fire. +namespace homeblocks { + +// Shared read_slot body for the same mocks: look up the recorded slot by lsn, or +// no_such_file_or_directory if write_slot was never called for it. +template < typename Mock > +async_result< JournalSlot > mock_read_slot(Mock& mock, int64_t lsn) { + auto it = mock.slots.find(lsn); + if (it == mock.slots.end()) + co_return std::unexpected(std::make_error_condition(std::errc::no_such_file_or_directory)); + co_return it->second; +} + +template < typename Mock > +async_status mock_free_slot(Mock& mock, int64_t lsn) { + auto it = mock.slots.find(lsn); + if (it == mock.slots.end()) + co_return std::unexpected(std::make_error_condition(std::errc::no_such_file_or_directory)); + if (it->second.all_zeros) co_return ok(); + co_return co_await mock.free_data(homestore::multi_blk_id{}); +} + +} // namespace homeblocks \ No newline at end of file diff --git a/src/lib/craft/tests/test_craft_peer_exchange.cpp b/src/lib/craft/tests/test_craft_peer_exchange.cpp index 1b2329d..9ecb8bc 100644 --- a/src/lib/craft/tests/test_craft_peer_exchange.cpp +++ b/src/lib/craft/tests/test_craft_peer_exchange.cpp @@ -34,6 +34,7 @@ #include "craft/craft_repl_dev.hpp" #include "coro_helpers.hpp" +#include "mock_journal_backend.hpp" SISL_LOGGING_DEF(HOMEBLOCKS_LOG_MODS) SISL_LOGGING_INIT(HOMEBLOCKS_LOG_MODS) @@ -64,14 +65,13 @@ class MockCraftJournalBackend : public CraftJournalBackend { async_result< JournalSlot > read_slot(int64_t lsn) override { if (fail_on_read && *fail_on_read == lsn) co_return std::unexpected(std::make_error_condition(std::errc::io_error)); - auto it = slots.find(lsn); - if (it == slots.end()) - co_return std::unexpected(std::make_error_condition(std::errc::no_such_file_or_directory)); - co_return it->second; + co_return co_await mock_read_slot(*this, lsn); } async_status truncate_to(int64_t) override { co_return ok(); } async_status free_data(homestore::multi_blk_id) override { co_return ok(); } + + async_status free_slot(int64_t lsn) override { return mock_free_slot(*this, lsn); } }; // ── test fixture ───────────────────────────────────────────────────────────── diff --git a/src/lib/craft/tests/test_craft_raft_entries.cpp b/src/lib/craft/tests/test_craft_raft_entries.cpp index 11a896d..d76316f 100644 --- a/src/lib/craft/tests/test_craft_raft_entries.cpp +++ b/src/lib/craft/tests/test_craft_raft_entries.cpp @@ -45,6 +45,7 @@ #include "craft/craft_repl_dev.hpp" #include "coro_helpers.hpp" +#include "mock_journal_backend.hpp" SISL_LOGGING_DEF(HOMEBLOCKS_LOG_MODS) SISL_LOGGING_INIT(HOMEBLOCKS_LOG_MODS) @@ -74,17 +75,14 @@ class MockCraftJournalBackend : public CraftJournalBackend { co_return ok(); } - async_result< JournalSlot > read_slot(int64_t lsn) override { - auto it = slots.find(lsn); - if (it == slots.end()) - co_return std::unexpected(std::make_error_condition(std::errc::no_such_file_or_directory)); - co_return it->second; - } + async_result< JournalSlot > read_slot(int64_t lsn) override { return mock_read_slot(*this, lsn); } async_status truncate_to(int64_t) override { co_return ok(); } async_status free_data(homestore::multi_blk_id) override { co_return ok(); } + async_status free_slot(int64_t lsn) override { return mock_free_slot(*this, lsn); } + bool has_slot(int64_t lsn) const { return slots.count(lsn) > 0; } }; diff --git a/src/lib/craft/tests/test_craft_truncate.cpp b/src/lib/craft/tests/test_craft_truncate.cpp index 380ebc2..1d4e67b 100644 --- a/src/lib/craft/tests/test_craft_truncate.cpp +++ b/src/lib/craft/tests/test_craft_truncate.cpp @@ -64,6 +64,10 @@ class MockCraftJournalBackend : public CraftJournalBackend { co_return ok(); } async_status free_data(homestore::multi_blk_id) override { co_return ok(); } + + async_status free_slot(int64_t) override { + co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); + } }; // ── test fixture ───────────────────────────────────────────────────────────── diff --git a/src/lib/craft/tests/test_craft_write.cpp b/src/lib/craft/tests/test_craft_write.cpp index b7364b0..a258bab 100644 --- a/src/lib/craft/tests/test_craft_write.cpp +++ b/src/lib/craft/tests/test_craft_write.cpp @@ -31,6 +31,7 @@ #include "craft/craft_repl_dev.hpp" #include "coro_helpers.hpp" +#include "mock_journal_backend.hpp" SISL_LOGGING_DEF(HOMEBLOCKS_LOG_MODS) SISL_LOGGING_INIT(HOMEBLOCKS_LOG_MODS) @@ -66,12 +67,7 @@ class MockCraftJournalBackend : public CraftJournalBackend { co_return ok(); } - async_result< JournalSlot > read_slot(int64_t lsn) override { - auto it = slots.find(lsn); - if (it == slots.end()) - co_return std::unexpected(std::make_error_condition(std::errc::no_such_file_or_directory)); - co_return it->second; - } + async_result< JournalSlot > read_slot(int64_t lsn) override { return mock_read_slot(*this, lsn); } async_status truncate_to(int64_t) override { co_return ok(); } async_status free_data(homestore::multi_blk_id) override { @@ -79,6 +75,8 @@ class MockCraftJournalBackend : public CraftJournalBackend { co_return ok(); } + async_status free_slot(int64_t lsn) override { return mock_free_slot(*this, lsn); } + bool has_slot(int64_t lsn) const { return slots.contains(lsn); } size_t slot_count() const { return slots.size(); } }; From 12b68b9792c8a2d2bf6f842ef7cd5c15a50ee1d6 Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Fri, 4 Sep 2026 13:33:03 -0700 Subject: [PATCH 11/16] craft: fix client_token gate and commit_lsn watermark in apply_sync_rs_commit_lsn Addresses two blocking review comments on PR #176 (szmyd): - The client_token != state_.client_token gate vetoed the login sequence's own SyncRSCommitLSN: per CRAFT-Design, SyncRSCommitLSN applies before the InternalLogin that establishes client_token, so the check always mismatched on login (and on every post-restart watchdog SyncRSCommitLSN, since state_ is in-memory-only). Dropped the check, matching craft_client's reference (MemCraftReplica::cold_apply_sync discards the parameter outright). Exclusivity comes from RAFT's commit ordering plus the term fence every other IO already checks. - commit_lsn was advancing unconditionally to rs_commit_lsn regardless of local catch-up outcome, conflating it with the replica-set-wide watermark. CRAFT-Design defines commit_lsn as the local contiguous prefix: skip Empty slots, but never advance past an unresolved Missing one. Replaced the unconditional max() with a walk-forward loop mirroring craft_client's reference apply_up_to. Updated test_craft_raft_entries.cpp accordingly: repurposed the two tests that asserted the old token-gate behavior into regression guards for the new behavior, and corrected 7 commit_lsn assertions (6 from the review scope plus one found during review, OnCommitDispatchesSyncRSCommitLSN) to the new stall-at-first-missing semantics. Co-Authored-By: Claude Sonnet 5 --- src/lib/craft/craft_repl_dev.cpp | 63 ++++++++-------- .../craft/tests/test_craft_raft_entries.cpp | 73 ++++++++++--------- 2 files changed, 73 insertions(+), 63 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index cf3ed58..f8ff28b 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -594,20 +594,19 @@ void CraftReplDev::CraftRaftListener::on_commit(int64_t lsn, sisl::blob const& h // ─── RAFT apply helpers (S5 implements) ────────────────────────────────────── // -// apply_sync_rs_commit_lsn (22886): client_token is verified against the current session first -- a mismatch -// gates the ENTIRE apply (no reconciliation, no catch-up, no watermark advance), since a RAFT entry whose -// token doesn't match the live session shouldn't be trusted to describe it. empty_slots is range-checked -// against rs_commit_lsn next, for the same reason and with the same all-or-nothing gate: SyncRSCommitLSN -// verdicts are only ever defined for slots the leader pre-resolved up to rs_commit_lsn (S5), so a negative -// or out-of-range entry is a malformed/corrupt RAFT entry, not a legitimate verdict -- trusting it would -// permanently poison empty_lsns_ for a slot that hasn't even been reached yet. Once both checks pass, every -// other step is best-effort forward progress: empty_slots are reconciled and the newly-spanned range is -// marked missing, catch-up attempts to fill in what it can from a peer, and commit_lsn/last_append_lsn -// advance regardless of whether catch-up fully succeeded -- mirroring truncate()'s invariant that apply -// never reverts the watermark, only advances it. A peer's fetch_data response gets its own all-or-nothing -// check (validate_fetch_response): unlike the two checks above, this one can't gate the whole apply (gap -// marking and last_append_lsn already advanced by the time the response arrives), so a malformed response -// is instead treated exactly like a failed fetch -- none of it applied, everything requested stays missing. +// apply_sync_rs_commit_lsn (22886): empty_slots is range-checked against rs_commit_lsn first -- the only +// all-or-nothing gate on this apply. SyncRSCommitLSN verdicts are only ever defined for slots the leader +// pre-resolved up to rs_commit_lsn (S5). client_token is NOT checked against the current session. Past the +// range check, every step is best-effort +// forward progress: empty_slots are reconciled and the newly-spanned range is marked missing, catch-up +// attempts to fill in what it can from a peer, and last_append_lsn advances regardless of whether catch-up +// fully succeeded -- mirroring truncate()'s invariant that apply never reverts the watermark, only advances +// it. commit_lsn is different: it's the local contiguous prefix (CRAFT-Design), so it only advances up to +// the first still-unresolved Missing slot, skipping over Empty ones, even though rs_commit_lsn itself is a +// watermark the whole replica set already agreed on. A peer's fetch_data response gets its own +// all-or-nothing check (validate_fetch_response): unlike the range check above, this one can't gate the +// whole apply (gap marking and last_append_lsn already advanced by the time the response arrives), so a +// malformed response is instead treated exactly like a failed fetch async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint64_t client_token, std::vector< int64_t > empty_slots) { @@ -615,8 +614,8 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 // on_commit call site (detail::detach) for why this is required. auto self = shared_from_this(); - // Validated before any state is touched -- same all-or-nothing gate as the token check below, since an - // out-of-range verdict means the entry itself cannot be trusted, not that this one slot should be skipped. + // Validated before any state is touched -- an out-of-range verdict means the entry itself cannot be + // trusted, not that this one slot should be skipped, so it gates the entire apply. for (int64_t lsn : empty_slots) { if (lsn < 0 || lsn > rs_commit_lsn) { LOGE("apply_sync_rs_commit_lsn: empty_slots lsn={} out of range [0, {}] -- rejecting entire apply", lsn, @@ -630,17 +629,18 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 uint64_t term; { std::lock_guard lk{missing_mu_}; - if (client_token != state_.client_token) { - LOGW("apply_sync_rs_commit_lsn: client_token mismatch want={} got={} rs_commit_lsn={} -- skipping apply", - state_.client_token, client_token, rs_commit_lsn); - co_return std::unexpected(make_error_condition(volume_error::WRONG_TOKEN)); - } + // client_token is NOT gated against state_.client_token here. Per the login sequence (CRAFT-Design), + // SyncRSCommitLSN applies BEFORE InternalLogin (which sets state_.client_token), so an equality-fence + // here would veto the very entry that carries login's own Empty verdicts, + // and would also veto every post-restart watchdog SyncRSCommitLSN, since state_ is + // in-memory-only and client_token resets to 0 across a restart. craft_client's reference + // (MemCraftReplica::cold_apply_sync) discards the parameter outright for the same reason. + // Exclusivity comes from RAFT's commit ordering plus the term fence every other IO already + // checks (see apply_internal_login's header comment), not from an equality check here. term = state_.term; for (int64_t lsn : empty_slots) { - if (missing_lsns_.erase(lsn)) { - to_free.push_back(lsn); - } + if (missing_lsns_.erase(lsn)) { to_free.push_back(lsn); } } empty_lsns_.insert(empty_slots.begin(), empty_slots.end()); @@ -701,10 +701,9 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 } blkid = *alloc_res; blkid_allocated = true; - } - //FIXME: We need to address the case when blkid is not set. How would write_slot handle that? + // FIXME: We need to address the case when blkid is not set. How would write_slot handle that? auto res = co_await journal_->write_slot(slot.lsn, term, slot.lba_off_bytes, slot.len_bytes, blkid, slot.all_zeros); if (!res) { @@ -726,15 +725,21 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 } } - // Unconditional: commit_lsn is a replica-set-wide watermark RAFT already agreed on, independent of - // whether this replica's local catch-up succeeded. + // commit_lsn (CRAFT-Design) is the LOCAL CONTIGUOUS prefix, distinct from rs_commit_lsn (the + // replica-set-wide watermark RAFT already agreed on): it must skip over Empty slots but never + // advance past an unresolved Missing one, even if catch-up above left holes below rs_commit_lsn. + // Mirrors craft_client's reference MemCraftReplica::apply_up_to. // // KNOWN GAP: this can land late. Because on_commit detaches this coroutine (see the FIXME there), // a later-committed entry (InternalLogin, or another SyncRSCommitLSN) may have already applied by // the time this advance actually runs, breaking strict RAFT apply ordering. { std::lock_guard lk{missing_mu_}; - state_.commit_lsn = std::max(state_.commit_lsn, rs_commit_lsn); + int64_t next = state_.commit_lsn + 1; + while (next <= rs_commit_lsn && !missing_lsns_.contains(next)) { + state_.commit_lsn = next; // resolved (present or Empty) -- Empty is skipped, not gated on + ++next; + } } LOGT("apply_sync_rs_commit_lsn ok rs_commit_lsn={} client_token={}", rs_commit_lsn, client_token); co_return ok(); diff --git a/src/lib/craft/tests/test_craft_raft_entries.cpp b/src/lib/craft/tests/test_craft_raft_entries.cpp index d76316f..afdde18 100644 --- a/src/lib/craft/tests/test_craft_raft_entries.cpp +++ b/src/lib/craft/tests/test_craft_raft_entries.cpp @@ -17,13 +17,16 @@ // InternalLogin (S5 / SDSTOR-22887) -- and their on_commit dispatch. // // SyncRSCommitLSN tests verify: -// - a client_token mismatch gates the ENTIRE apply: no reconciliation, no catch-up, no watermark advance +// - client_token is carried on the entry for observability only -- it is NOT gated against local state, +// regardless of what InternalLogin has (or hasn't) established, since SyncRSCommitLSN applies before +// the InternalLogin that would set it (see the inline comment at the call site for why) // - empty_slots are range-validated against rs_commit_lsn and reconciled into empty_lsns_/missing_lsns_ // - commit_lsn/last_append_lsn advance directly when there's no gap to catch up on // - fetch_data is invoked with exactly the missing LSNs when behind, and its response is persisted // - a peer response naming an unrequested or duplicate LSN is rejected as a whole batch -// - catch-up is best-effort: a failed fetch, a failed write_slot, or no peer_fetcher_ at all still lets -// commit_lsn advance, leaving unresolved LSNs in missing_lsns_ +// - catch-up is best-effort: a failed fetch, a failed write_slot, or no peer_fetcher_ at all leaves the +// affected LSN(s) missing; commit_lsn stalls just below the first unresolved Missing slot (Empty +// slots are skipped over), never advancing past it regardless of rs_commit_lsn // - commit_lsn never decrements // - on_commit parses a real serialized SyncRSCommitLSN entry and dispatches correctly (and rejects // malformed header/key blobs without touching state) @@ -33,7 +36,8 @@ // - a wrong-size key (too short or too long) is rejected, state untouched // - a second InternalLogin replaces client_token outright but never regresses term // - once applied, write()'s term-fence check reflects the new term end-to-end -// - the client_token InternalLogin establishes is what apply_sync_rs_commit_lsn's token check uses +// - InternalLogin establishing a new client_token has no bearing on SyncRSCommitLSN applies -- old or +// new token, the apply proceeds the same either way // // This TU defines SISL_LOGGING_DEF for the homeblocks module because it compiles craft_repl_dev.cpp // directly (same pattern as test_craft_truncate.cpp). @@ -163,18 +167,19 @@ class CraftRaftEntriesTest : public ::testing::Test { namespace { -// ── client_token gate ───────────────────────────────────────────────────────── +// ── client_token is not gated ───────────────────────────────────────────────── -// state_.client_token defaults to 0; a non-matching token must veto the whole apply. -TEST_F(CraftRaftEntriesTest, TokenMismatchSkipsWholeApply) { +// client_token is carried for observability only -- a mismatch must NOT block the apply. See the inline +// comment in apply_sync_rs_commit_lsn (under missing_mu_) for why: SyncRSCommitLSN applies before the +// InternalLogin that would establish state_.client_token, so an equality-fence here would make login +// itself unreachable. +TEST_F(CraftRaftEntriesTest, ClientTokenMismatchDoesNotBlockApply) { dev_->seed_lsns(5, {3}); auto r = do_apply(/*rs_commit_lsn=*/100, /*client_token=*/999); - ASSERT_FALSE(r.has_value()); - EXPECT_EQ(r.error(), make_error_condition(volume_error::WRONG_TOKEN)); - EXPECT_EQ(dev_->commit_lsn(), -1); - EXPECT_EQ(dev_->last_append_lsn(), 5); - EXPECT_EQ(dev_->missing_count(), 1u); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->last_append_lsn(), 100); + EXPECT_EQ(dev_->commit_lsn(), 2); // stalls at lsn=3, still missing -- no peer_fetcher_ wired } // ── empty_slots range validation ────────────────────────────────────────────── @@ -234,7 +239,7 @@ TEST_F(CraftRaftEntriesTest, EmptySlotWithinNewGapRangeNotDoubleTracked) { EXPECT_TRUE(dev_->is_missing(4)); EXPECT_TRUE(dev_->is_missing(5)); EXPECT_EQ(dev_->missing_count(), 4u); - EXPECT_EQ(dev_->commit_lsn(), 5); + EXPECT_EQ(dev_->commit_lsn(), 0); // stalls at lsn=1, still missing -- no peer_fetcher_ wired } // ── watermark advance ────────────────────────────────────────────────────────── @@ -302,8 +307,8 @@ TEST_F(CraftRaftEntriesTest, BehindPassesConfiguredTimeoutToPeerFetcher) { // fetch_data's contract is one entry per requested LSN. A response naming an LSN we never asked for (a // buggy/misbehaving peer) can't be partially trusted -- since gap-marking already advanced by this point // in the apply, this can't gate the whole apply the way the upfront empty_slots check does, but it CAN -// still refuse the batch: none of the response is applied (same outcome as a fetch failure), even the -// entries that individually look fine, and commit_lsn still advances (best-effort). +// still refuse the batch: none of the response is applied (same outcome as a fetch failure), and commit_lsn +// stalls at the first still-missing lsn (best-effort forward progress, not a jump to rs_commit_lsn). TEST_F(CraftRaftEntriesTest, BehindRejectsPeerResponseWithUnrequestedLSN) { dev_->set_peer_fetcher(&fetcher_); dev_->seed_lsns(0, {}); @@ -319,7 +324,7 @@ TEST_F(CraftRaftEntriesTest, BehindRejectsPeerResponseWithUnrequestedLSN) { EXPECT_FALSE(dev_->is_empty_slot(2)); EXPECT_FALSE(dev_->is_empty_slot(99)); EXPECT_EQ(dev_->missing_count(), 2u); // 1 and 2 both remain missing - EXPECT_EQ(dev_->commit_lsn(), 2); + EXPECT_EQ(dev_->commit_lsn(), 0); // stalls at lsn=1, still missing } // A duplicate entry for an actually-requested LSN is just as much a contract violation as an @@ -337,11 +342,12 @@ TEST_F(CraftRaftEntriesTest, BehindRejectsPeerResponseWithDuplicateLSN) { ASSERT_TRUE(r.has_value()); EXPECT_FALSE(journal_->has_slot(1)); EXPECT_EQ(dev_->missing_count(), 1u); - EXPECT_EQ(dev_->commit_lsn(), 1); + EXPECT_EQ(dev_->commit_lsn(), 0); // stalls at lsn=1, still missing } -// fetch_data fails outright: commit_lsn still advances (best-effort); every spanned LSN remains missing. -TEST_F(CraftRaftEntriesTest, BehindFetchFailsStillAdvancesCommitLsn) { +// fetch_data fails outright: commit_lsn stalls at the first missing lsn (best-effort, not a jump to +// rs_commit_lsn); every spanned LSN remains missing. +TEST_F(CraftRaftEntriesTest, BehindFetchFailsCommitLsnStallsAtFirstMissing) { dev_->set_peer_fetcher(&fetcher_); dev_->seed_lsns(0, {}); fetcher_.should_fail = true; @@ -349,24 +355,24 @@ TEST_F(CraftRaftEntriesTest, BehindFetchFailsStillAdvancesCommitLsn) { auto r = do_apply(/*rs_commit_lsn=*/3, /*client_token=*/0); ASSERT_TRUE(r.has_value()); - EXPECT_EQ(dev_->commit_lsn(), 3); + EXPECT_EQ(dev_->commit_lsn(), 0); // stalls at lsn=1, still missing EXPECT_EQ(dev_->last_append_lsn(), 3); EXPECT_EQ(dev_->missing_count(), 3u); } // No peer_fetcher_ wired at all (S9 not wired yet): same best-effort outcome as a fetch failure. -TEST_F(CraftRaftEntriesTest, BehindNoPeerFetcherStillAdvancesCommitLsn) { +TEST_F(CraftRaftEntriesTest, BehindNoPeerFetcherCommitLsnStallsAtFirstMissing) { dev_->seed_lsns(0, {}); auto r = do_apply(/*rs_commit_lsn=*/2, /*client_token=*/0); ASSERT_TRUE(r.has_value()); - EXPECT_EQ(dev_->commit_lsn(), 2); + EXPECT_EQ(dev_->commit_lsn(), 0); // stalls at lsn=1, still missing EXPECT_EQ(dev_->missing_count(), 2u); } // A fetched slot's write_slot fails: that LSN alone stays missing; the rest of catch-up still applies, -// and commit_lsn still advances. +// and commit_lsn advances up to (but not past) it. TEST_F(CraftRaftEntriesTest, WriteSlotFailureDuringCatchupLeavesLsnMissing) { dev_->set_peer_fetcher(&fetcher_); dev_->seed_lsns(0, {}); @@ -383,7 +389,7 @@ TEST_F(CraftRaftEntriesTest, WriteSlotFailureDuringCatchupLeavesLsnMissing) { EXPECT_FALSE(journal_->has_slot(2)); EXPECT_FALSE(dev_->is_missing(1)); EXPECT_TRUE(dev_->is_missing(2)); - EXPECT_EQ(dev_->commit_lsn(), 2); + EXPECT_EQ(dev_->commit_lsn(), 1); // lsn=1 resolved; stalls at lsn=2, still missing } // ── on_commit dispatch ───────────────────────────────────────────────────────── @@ -395,7 +401,8 @@ TEST_F(CraftRaftEntriesTest, OnCommitDispatchesSyncRSCommitLSN) { dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); - EXPECT_EQ(dev_->commit_lsn(), 7); + // last_append_lsn advances unconditionally + EXPECT_EQ(dev_->last_append_lsn(), 7); } TEST_F(CraftRaftEntriesTest, OnCommitRejectsHeaderTooSmall) { @@ -558,23 +565,21 @@ TEST_F(CraftRaftEntriesTest, WriteRejectsStaleTermAfterInternalLogin) { EXPECT_EQ(r.error(), make_error_condition(volume_error::STALE_TERM)); } -// Cross-entry-type integration: apply_sync_rs_commit_lsn's client_token check reads the SAME state_ -// InternalLogin writes. Untestable before this ticket (state_.client_token was permanently 0, matching -// every SyncRSCommitLSN test's default). Once InternalLogin establishes a new token, do_apply must use -// it -- the OLD default (0) is now itself a mismatch. -TEST_F(CraftRaftEntriesTest, SyncRSCommitLSNUsesTokenEstablishedByInternalLogin) { +// InternalLogin establishing a new client_token has no bearing on SyncRSCommitLSN applies -- neither the +// stale/old token nor the newly-established one gates the apply; both succeed identically. Guards against +// reintroducing an equality-fence keyed off InternalLogin's client_token. +TEST_F(CraftRaftEntriesTest, SyncRSCommitLSNAppliesRegardlessOfInternalLoginToken) { auto header_buf = make_header(CraftEntryType::InternalLogin); auto key_buf = make_internal_login_key(/*client_token=*/7, /*term=*/1); cintrusive< homestore::repl_req_ctx > ctx{}; dev_->test_listener().on_commit(1, as_blob(header_buf), as_blob(key_buf), {}, ctx); - auto stale = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/0); // the old default -- now stale - ASSERT_FALSE(stale.has_value()); - EXPECT_EQ(stale.error(), make_error_condition(volume_error::WRONG_TOKEN)); + auto stale = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/0); // the old default -- would have been a mismatch + ASSERT_TRUE(stale.has_value()); auto fresh = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/7); // the token InternalLogin just set ASSERT_TRUE(fresh.has_value()); - EXPECT_EQ(dev_->commit_lsn(), 5); + EXPECT_EQ(dev_->last_append_lsn(), 5); } } // namespace From 05b5efa7be5fc3d1f83a13483018883bd20cf9a8 Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Fri, 4 Sep 2026 13:40:11 -0700 Subject: [PATCH 12/16] craft: correct SyncRSCommitLSN doc wording to match apply_sync_rs_commit_lsn fixes docs/craft/subtasks.md and docs/craft/rpcs.md both said the apply "verifies token" and "commit_lsn = rs_commit_lsn" -- exactly the behavior removed in the previous commit. Reworded both to describe the actual behavior: client_token is carried on the entry but not checked against local state, and commit_lsn advances to the contiguous prefix bounded by rs_commit_lsn, skipping Empty slots but never past an unresolved Missing one. Co-Authored-By: Claude Sonnet 5 --- docs/craft/rpcs.md | 14 +++++++++----- docs/craft/subtasks.md | 4 ++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/docs/craft/rpcs.md b/docs/craft/rpcs.md index 0f1700d..340e5bd 100644 --- a/docs/craft/rpcs.md +++ b/docs/craft/rpcs.md @@ -196,11 +196,15 @@ RAFT entry payload: { rs_commit_lsn: int64, client_token: uint64, empty_slots: [ Proposed by the leader via `CraftReplDev::append()` — triggered by login, the watchdog, the periodic checkpoint, or the client-requested **Resolve** RPC (#5). **Before proposing**, the leader resolves every unresolved slot ≤ `rs_commit_lsn`: fetch from any holder, or record an `Empty` verdict on -quorum-lacks evidence; it never proposes past an unresolved slot. On RAFT commit each replica: verify -the token, mark `empty_slots` as permanent no-op holes (discarding any local data there), fetch the -remaining missing slots from peers, then advance `commit_lsn`. Replicas never declare `Empty` -unilaterally. This is the primary recovery mechanism — it carries no write data, only the watermark -and verdicts. +quorum-lacks evidence; it never proposes past an unresolved slot. On RAFT commit each replica: mark +`empty_slots` as permanent no-op holes (discarding any local data there), fetch the remaining missing +slots from peers, then advance `commit_lsn` to the contiguous prefix bounded by `rs_commit_lsn` -- +skipping `Empty` slots but never past an unresolved `Missing` one. `client_token` is carried on the +entry but not checked against local state at apply time: `SyncRSCommitLSN` applies before the +`InternalLogin` that would establish it, so an equality-fence here would make login itself unreachable; +ordering plus the term fence on subsequent IO provide exclusivity instead. Replicas never declare +`Empty` unilaterally. This is the primary recovery mechanism — it carries no write data, only the +watermark and verdicts. --- diff --git a/docs/craft/subtasks.md b/docs/craft/subtasks.md index 7348bdf..db39529 100644 --- a/docs/craft/subtasks.md +++ b/docs/craft/subtasks.md @@ -132,8 +132,8 @@ and enforce single-writer exclusivity without data flowing through the RAFT log. **SyncRSCommitLSN:** - RAFT entry carries `{rs_commit_lsn, client_token, empty_slots[]}` - **Leader pre-resolution:** before proposing `N`, the leader resolves every unresolved slot ≤ `N`: fetch it from any holder, or record an `Empty` verdict on quorum-lacks evidence (leader counts itself; non-responders never count); it must not propose past an unresolved slot -- On apply: verify token; mark `empty_slots` Empty, **discarding any local data held there** (reconciliation); if behind, `fetch_data()` the remaining missing slots from peers; then `commit_lsn = rs_commit_lsn`. **Apply never truncates** and replicas **never declare Empty unilaterally** -- Peer catch-up (`CraftPeerFetcher::fetch_from_peer`) is **timeout-bounded**: every call passes `peer_fetch_timeout_ms` (`home_blks_config.fbs`, default 5000ms); a peer that misses the deadline is treated as a hard failure, same as any other fetch failure (best-effort — `commit_lsn` still advances, unresolved LSNs stay missing). The interface only carries the deadline; enforcing it against a real wire call is S9's (the transport's) +- On apply: `client_token` is carried on the entry but NOT checked against local state -- `SyncRSCommitLSN` applies before the `InternalLogin` that would establish it, so an equality-fence here would make login itself unreachable; ordering plus the term fence on subsequent IO provide exclusivity instead. Mark `empty_slots` Empty, **discarding any local data held there** (reconciliation); if behind, `fetch_data()` the remaining missing slots from peers; then advance `commit_lsn` to the contiguous prefix bounded by `rs_commit_lsn` (skipping `Empty` slots, never past an unresolved `Missing` one). **Apply never truncates** and replicas **never declare Empty unilaterally** +- Peer catch-up (`CraftPeerFetcher::fetch_from_peer`) is **timeout-bounded**: every call passes `peer_fetch_timeout_ms` (`home_blks_config.fbs`, default 5000ms); a peer that misses the deadline is treated as a hard failure, same as any other fetch failure (best-effort — unresolved LSNs stay missing and `commit_lsn` stalls just below the first one). The interface only carries the deadline; enforcing it against a real wire call is S9's (the transport's) - `append(sync_to, client_token)` proposes this entry via RAFT - Triggers: periodic every N LSNs (configurable via `home_blks_config.fbs`, default 128), watchdog, login, **client-requested (after a failed sub-quorum write)** — the client's `Resolve` RPC lands on `CraftReplDev::request_resolution(term, upto)`, which runs this same leader pre-resolution and returns the Empty verdicts ≤ `upto` (`craft::resolution_result`). The client broadcasts it to every member (it cannot know the leader mid-session); a follower returns `NOT_LEADER` From d72df8f74451d4ad07f08faf2384a8e00f77af2f Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Fri, 4 Sep 2026 17:03:21 -0700 Subject: [PATCH 13/16] craft: proactively trigger a HomeStore checkpoint on commit_lsn advance Implements SDSTOR-22888. After apply_sync_rs_commit_lsn advances commit_lsn, nudge HomeStore to checkpoint once the advance since the last trigger crosses checkpoint_lsn_interval_. Otherwise the journal-reclaim / RAFT-log-compaction floor (docs/craft/subtasks.md's S8) can lag arbitrarily far behind commit_lsn, unbounding restart recovery time. - New CraftCheckpointTrigger interface + HomeStoreCraftCheckpointTrigger production impl wrapping homestore::cp_mgr().trigger_cp_flush(), following the same inject-an-abstraction pattern as CraftJournalBackend/CraftPeerFetcher so unit tests (which run with no live HomeStore instance) can exercise the trigger via a mock. CraftReplDev takes it as a non-owning pointer, same shape as peer_fetcher_, since cp_mgr() is one instance shared by every volume, not owned per-CraftReplDev. - The trigger call is detail::detach()'d (fire-and-forget), matching the existing free_data cleanup pattern in this same function -- nothing depends on the flush completing. - force=false: let it coalesce with any checkpoint already in flight rather than forcing back-to-back flushes under high commit throughput. - Left two forward-looking FIXMEs for related gaps out of this ticket's scope: seeding last_checkpoint_lsn_ from recovered commit_lsn once S8 restart recovery lands, and forcing a completed (not just requested) flush before truncate() drops journal entries, mirroring HomeStore's own IndexTable::destroy(). - Tests: MockCraftCheckpointTrigger covers interval gating (fires-once-crossed, below-interval, accumulates-across-calls, exact boundary, baseline tracks the actual commit_lsn reached rather than incrementing by the interval), null-trigger safety, and best-effort failure handling. test_craft_homestore_backend.cpp gets two new cases (force=false and force=true) exercising the production wrapper against a real cp_mgr() -- previously untested against anything but the mock. Co-Authored-By: Claude Sonnet 5 --- src/lib/craft/craft_repl_dev.cpp | 54 +++++++ src/lib/craft/craft_repl_dev.hpp | 49 ++++++- .../tests/test_craft_homestore_backend.cpp | 25 ++++ .../craft/tests/test_craft_raft_entries.cpp | 134 ++++++++++++++++++ src/lib/home_blks_config.fbs | 4 +- 5 files changed, 264 insertions(+), 2 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index f8ff28b..f6a5851 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -20,6 +20,7 @@ #include #include // data_service(), async_alloc_write, blk_alloc_hints +#include // cp_mgr(), CPManager::trigger_cp_flush() #include // home_log_store, logstore_seq_num_t, log_write_comp_cb_t #include // iomanager singleton, reactor_regex #include // value_awaitable: lock-free completion-before-suspend-safe bridge @@ -221,6 +222,23 @@ unique< CraftJournalBackend > make_homestore_journal_backend(shared< homestore:: return std::make_unique< HomeStoreCraftJournalBackend >(std::move(logstore), vol_ordinal); } +// ─── HomeStoreCraftCheckpointTrigger (SDSTOR-22888) ────────────────────────── +// +// Thin wrapper over homestore::cp_mgr(). One instance is shared by every volume's CraftReplDev. + +class HomeStoreCraftCheckpointTrigger : public CraftCheckpointTrigger { +public: + async_status trigger_cp_flush(bool force) override { + if (!co_await homestore::cp_mgr().trigger_cp_flush(force)) + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + co_return ok(); + } +}; + +unique< CraftCheckpointTrigger > make_homestore_checkpoint_trigger() { + return std::make_unique< HomeStoreCraftCheckpointTrigger >(); +} + // ─── constructor ────────────────────────────────────────────────────────────── CraftReplDev::CraftReplDev(volume_id_t vol_id, unique< CraftJournalBackend > journal) : @@ -450,6 +468,9 @@ async_result< craft::read_result > CraftReplDev::read(craft::client_hdr /* hdr * co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); } +// TODO(SDSTOR-22733): once implemented, this is the other commit_lsn-advance path SDSTOR-22888's +// checkpoint trigger needs to cover (see apply_sync_rs_commit_lsn's own hook) -- same +// checkpoint_lsn_interval_/last_checkpoint_lsn_ bookkeeping under missing_mu_, same force=false. async_result< craft::lsn_pair > CraftReplDev::keep_alive(craft::client_hdr /* hdr */) { LOGW("CraftReplDev::keep_alive not yet implemented"); co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); @@ -733,6 +754,8 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 // KNOWN GAP: this can land late. Because on_commit detaches this coroutine (see the FIXME there), // a later-committed entry (InternalLogin, or another SyncRSCommitLSN) may have already applied by // the time this advance actually runs, breaking strict RAFT apply ordering. + int64_t commit_lsn_snapshot; + bool should_checkpoint = false; { std::lock_guard lk{missing_mu_}; int64_t next = state_.commit_lsn + 1; @@ -740,6 +763,37 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 state_.commit_lsn = next; // resolved (present or Empty) -- Empty is skipped, not gated on ++next; } + commit_lsn_snapshot = state_.commit_lsn; + // SDSTOR-22888: nudge HomeStore to checkpoint proactively rather than waiting on its own + // timer, so the journal-reclaim / RAFT-log-compaction floor (docs/craft/subtasks.md's S8) + // doesn't lag arbitrarily far behind commit_lsn. Interval reuses sync_rs_commit_lsn_interval + // (via checkpoint_lsn_interval_) rather than its own knob -- ties checkpoint cadence to the + // periodic SyncRSCommitLSN cadence. last_checkpoint_lsn_ is updated right here, before the + // lock is released so that two overlapping apply_sync_rs_commit_lsn calls can't both read + // the same stale last_checkpoint_lsn_ and both decide to fire. + if (commit_lsn_snapshot - last_checkpoint_lsn_ >= checkpoint_lsn_interval_) { + last_checkpoint_lsn_ = commit_lsn_snapshot; + should_checkpoint = true; + } + } + if (should_checkpoint) { + // force=false: let this coalesce with any checkpoint already in flight rather than forcing + // back-to-back flushes under high commit throughput (see CraftCheckpointTrigger's doc + // comment). Detached (fire-and-forget) -- same pattern as the free_data cleanup above: + // nothing here depends on the flush completing. A failure is logged, not propagated, + // same posture as catch-up/fetch failures elsewhere in this function. + if (checkpoint_trigger_ == nullptr) { + LOGW("apply_sync_rs_commit_lsn: commit_lsn={} crossed checkpoint interval but no " + "checkpoint_trigger_ wired -- skipping", + commit_lsn_snapshot); + } else { + detail::detach([self, commit_lsn_snapshot]() -> async_status { + if (auto cp = co_await self->checkpoint_trigger_->trigger_cp_flush(false); !cp) + LOGE("apply_sync_rs_commit_lsn: checkpoint trigger failed at commit_lsn={}: {}", + commit_lsn_snapshot, cp.error().message()); + co_return ok(); + }()); + } } LOGT("apply_sync_rs_commit_lsn ok rs_commit_lsn={} client_token={}", rs_commit_lsn, client_token); co_return ok(); diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index f33fc0a..e6d336e 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -123,6 +123,27 @@ class CraftPeerFetcher { virtual ~CraftPeerFetcher() = default; }; +// ─── CraftCheckpointTrigger ─────────────────────────────────────────────────── +// +// Abstraction over HomeStore's checkpoint manager (homestore::cp_mgr().trigger_cp_flush()). +// Injected into CraftReplDev so unit tests (which compile craft_repl_dev.cpp directly against a +// mock journal backend, with no running HomeStore instance -- see test_craft_raft_entries.cpp) can +// exercise the trigger without touching HomeStore. Production code passes +// HomeStoreCraftCheckpointTrigger (defined in craft_repl_dev.cpp). Default (null) leaves the +// trigger stubbed -- same posture as CraftPeerFetcher. + +class CraftCheckpointTrigger { +public: + virtual async_status trigger_cp_flush(bool force) = 0; + virtual ~CraftCheckpointTrigger() = default; +}; + +// Factory that wraps homestore::cp_mgr(). One instance is shared by every volume's CraftReplDev +// (there is exactly one CPManager per HomeStore instance), unlike make_homestore_journal_backend +// which is per-volume -- so CraftReplDev takes this via a non-owning pointer (set_checkpoint_trigger), +// not ownership at construction. Tests inject MockCraftCheckpointTrigger directly. +unique< CraftCheckpointTrigger > make_homestore_checkpoint_trigger(); + // ─── CraftReplDev ───────────────────────────────────────────────────────────── // // One instance per CRAFT-mode volume. Implements the full CRAFT data plane @@ -211,6 +232,11 @@ class CraftReplDev : public std::enable_shared_from_this< CraftReplDev > { // Drop all journal entries with dLSN > lsn; clear missing-set entries above lsn; clamp last_append_lsn. // Called only during login (quiesced -- no concurrent writes). commit_lsn is NOT changed. + // FIXME(S4/S7): before dropping entries here, force a completed checkpoint -- + // co_await checkpoint_trigger_->trigger_cp_flush(true). Once entries above/below lsn are gone, the journal is + // no longer a durable record of them; if HomeStore's checkpoint has only been requested and not yet + // completed, a crash in between loses that data. HomeStore's own IndexTable::destroy() hits the + // identical problem and force-flushes before removing its superblock for exactly this reason. async_status truncate(int64_t lsn); // Propose a SyncRSCommitLSN RAFT entry (called by watchdog or leader during login). @@ -262,6 +288,17 @@ class CraftReplDev : public std::enable_shared_from_this< CraftReplDev > { // Production sets this from HB_DYNAMIC_CONFIG(peer_fetch_timeout_ms) after construction (S8/S9). void set_peer_fetch_timeout_ms(uint32_t ms) { peer_fetch_timeout_ms_ = ms; } + // Wires the HomeStore checkpoint trigger used by apply_sync_rs_commit_lsn's periodic checkpoint + // (SDSTOR-22888). One CraftCheckpointTrigger instance is shared by every volume's CraftReplDev; + // tests inject a mock. + void set_checkpoint_trigger(CraftCheckpointTrigger* t) { checkpoint_trigger_ = t; } + + // Overrides the commit_lsn delta between checkpoint triggers (default matches + // sync_rs_commit_lsn_interval's own default of 128, tying checkpoint cadence to the periodic + // SyncRSCommitLSN cadence). Production sets this from HB_DYNAMIC_CONFIG(sync_rs_commit_lsn_interval) + // after construction, same pattern as set_peer_fetch_timeout_ms. + void set_checkpoint_lsn_interval(int64_t n) { checkpoint_lsn_interval_ = n; } + #ifdef _PRERELEASE // Seeds partition watermarks and the missing set directly, bypassing write(). // Only compiled when _PRERELEASE is defined; never present in production binaries. @@ -352,13 +389,23 @@ class CraftReplDev : public std::enable_shared_from_this< CraftReplDev > { // insert one LSN at a time under missing_mu_, which is O(gap width) instead of O(log ranges). std::set< int64_t > missing_lsns_; // gaps between commit_lsn and last_append_lsn std::unordered_set< int64_t > empty_lsns_; // slots positively verdicted Empty by a prior SyncRSCommitLSN (S5) - mutable std::mutex missing_mu_; // guards state_, missing_lsns_, and empty_lsns_ + mutable std::mutex missing_mu_; // guards state_, missing_lsns_, empty_lsns_, and last_checkpoint_lsn_ bool login_in_progress_{false}; std::mutex login_mu_; CraftRaftListener raft_listener_; CraftPeerFetcher* peer_fetcher_{nullptr}; // null until S9 wires CraftConnector uint32_t peer_fetch_timeout_ms_{5000}; // deadline for fetch_data; overridden via set_peer_fetch_timeout_ms() std::atomic< uint64_t > write_counter_{0}; // incremented per write(); triggers periodic SyncRSCommitLSN append + + CraftCheckpointTrigger* checkpoint_trigger_{nullptr}; // null until production wiring; unit tests inject a mock + int64_t checkpoint_lsn_interval_{128}; // commit_lsn delta between checkpoint triggers; see + // set_checkpoint_lsn_interval() + int64_t last_checkpoint_lsn_{-1}; // commit_lsn as of the last triggered checkpoint (guarded by missing_mu_) + // FIXME(S8/SDSTOR-22745): defaults to -1 in lockstep with state_.commit_lsn + // When S8 wires recovering commit_lsn from the journal/superblock on restart, + // seed this to the recovered commit_lsn too (not -1), or the first post- + // restart apply_sync_rs_commit_lsn will unconditionally fire a checkpoint + // regardless of how recently one actually happened before the crash. }; } // namespace homeblocks diff --git a/src/lib/craft/tests/test_craft_homestore_backend.cpp b/src/lib/craft/tests/test_craft_homestore_backend.cpp index 07adf3c..b74097f 100644 --- a/src/lib/craft/tests/test_craft_homestore_backend.cpp +++ b/src/lib/craft/tests/test_craft_homestore_backend.cpp @@ -22,6 +22,11 @@ // backend directly rather than through CraftReplDev or a volume -- the narrowest test that still // runs the real completion path. // +// Also exercises HomeStoreCraftCheckpointTrigger::trigger_cp_flush (SDSTOR-22888) against the REAL +// homestore::cp_mgr() -- same rationale: MockCraftCheckpointTrigger (test_craft_raft_entries.cpp) +// covers CraftReplDev's own gating logic, but the wrapper's factory -> cp_mgr().trigger_cp_flush() +// -> async_status conversion chain had never been compiled and run against a live CPManager. +// // Links the full homeblocks library (unlike the other craft tests, which compile // craft_repl_dev.cpp directly to avoid HomeStore bring-up) because a real home_log_store requires // a running HomeStore instance. @@ -110,6 +115,26 @@ TEST_F(CraftHomeStoreBackendTest, AllocWriteDataFailsCleanlyForUnregisteredOrdin ASSERT_FALSE(alloc_r.has_value()); } +// force=false: the value apply_sync_rs_commit_lsn's periodic trigger actually passes today. +TEST_F(CraftHomeStoreBackendTest, CheckpointTriggerFlushesRealCPManager) { + auto trigger = make_homestore_checkpoint_trigger(); + ASSERT_TRUE(trigger != nullptr); + + auto r = homeblocks::detail::sync_get(trigger->trigger_cp_flush(/* force = */ false)); + ASSERT_TRUE(r.has_value()); +} + +// force=true: untested until now -- this is the value truncate()'s FIXME (craft_repl_dev.hpp) says +// a future correctness-critical call site will need, but the passthrough itself had never been +// exercised against the real cp_mgr() for either value. +TEST_F(CraftHomeStoreBackendTest, CheckpointTriggerHonorsForceFlag) { + auto trigger = make_homestore_checkpoint_trigger(); + ASSERT_TRUE(trigger != nullptr); + + auto r = homeblocks::detail::sync_get(trigger->trigger_cp_flush(/* force = */ true)); + ASSERT_TRUE(r.has_value()); +} + int main(int argc, char* argv[]) { int parsed_argc = argc; char** orig_argv = argv; diff --git a/src/lib/craft/tests/test_craft_raft_entries.cpp b/src/lib/craft/tests/test_craft_raft_entries.cpp index afdde18..1a193ca 100644 --- a/src/lib/craft/tests/test_craft_raft_entries.cpp +++ b/src/lib/craft/tests/test_craft_raft_entries.cpp @@ -30,6 +30,9 @@ // - commit_lsn never decrements // - on_commit parses a real serialized SyncRSCommitLSN entry and dispatches correctly (and rejects // malformed header/key blobs without touching state) +// - a checkpoint trigger (SDSTOR-22888) fires once commit_lsn has advanced by at least +// checkpoint_lsn_interval_ since the last trigger (accumulating across calls, not just within +// one), is a no-op when unwired, and a trigger failure is logged but never fails the apply // // InternalLogin tests verify: // - on_commit dispatches to apply_internal_login, which sets client_token/term @@ -114,6 +117,27 @@ class MockCraftPeerFetcher : public CraftPeerFetcher { } }; +// ── checkpoint trigger mock ─────────────────────────────────────────────────── +// +// Records call count / last `force` value; fail_next injects a one-shot error. + +class MockCraftCheckpointTrigger : public CraftCheckpointTrigger { +public: + int call_count{0}; + bool last_force{false}; + bool fail_next{false}; + + async_status trigger_cp_flush(bool force) override { + ++call_count; + last_force = force; + if (fail_next) { + fail_next = false; + co_return std::unexpected(std::make_error_condition(std::errc::io_error)); + } + co_return ok(); + } +}; + // ── wire-format helpers for the on_commit dispatch tests ───────────────────── sisl::blob as_blob(std::vector< uint8_t >& buf) { return sisl::blob{buf.data(), static_cast< uint32_t >(buf.size())}; } @@ -162,6 +186,7 @@ class CraftRaftEntriesTest : public ::testing::Test { MockCraftJournalBackend* journal_{nullptr}; MockCraftPeerFetcher fetcher_; + MockCraftCheckpointTrigger trigger_; std::shared_ptr< CraftReplDev > dev_; }; @@ -392,6 +417,115 @@ TEST_F(CraftRaftEntriesTest, WriteSlotFailureDuringCatchupLeavesLsnMissing) { EXPECT_EQ(dev_->commit_lsn(), 1); // lsn=1 resolved; stalls at lsn=2, still missing } +// ── checkpoint trigger (SDSTOR-22888) ───────────────────────────────────────── + +TEST_F(CraftRaftEntriesTest, CheckpointTriggerFiresOnceIntervalCrossed) { + dev_->set_checkpoint_trigger(&trigger_); + dev_->set_checkpoint_lsn_interval(5); + dev_->seed_lsns(10, {}); + + auto r = do_apply(/*rs_commit_lsn=*/10, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 10); + EXPECT_EQ(trigger_.call_count, 1); + EXPECT_FALSE(trigger_.last_force); +} + +TEST_F(CraftRaftEntriesTest, CheckpointTriggerDoesNotFireBelowInterval) { + dev_->set_checkpoint_trigger(&trigger_); + dev_->set_checkpoint_lsn_interval(5); + dev_->seed_lsns(3, {}); + + auto r = do_apply(/*rs_commit_lsn=*/3, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 3); + EXPECT_EQ(trigger_.call_count, 0); +} + +// Two applies whose individual advances each stay below the interval on their own, but whose +// combined progress since the last trigger crosses it on the second call -- the interval tracks +// cumulative distance from last_checkpoint_lsn_, not distance moved within a single apply. +TEST_F(CraftRaftEntriesTest, CheckpointTriggerAccumulatesAcrossCalls) { + dev_->set_checkpoint_trigger(&trigger_); + dev_->set_checkpoint_lsn_interval(5); + dev_->seed_lsns(10, {}); + + auto r1 = do_apply(/*rs_commit_lsn=*/3, /*client_token=*/0); + ASSERT_TRUE(r1.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 3); + EXPECT_EQ(trigger_.call_count, 0); + + auto r2 = do_apply(/*rs_commit_lsn=*/4, /*client_token=*/0); + ASSERT_TRUE(r2.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 4); + EXPECT_EQ(trigger_.call_count, 1); +} + +// Pins down the exact boundary (>=, not >): delta from last_checkpoint_lsn_ (-1) to commit_lsn (4) +// is exactly 5, equal to the interval, not one past it. +TEST_F(CraftRaftEntriesTest, CheckpointTriggerFiresExactlyAtIntervalBoundary) { + dev_->set_checkpoint_trigger(&trigger_); + dev_->set_checkpoint_lsn_interval(5); + dev_->seed_lsns(4, {}); + + auto r = do_apply(/*rs_commit_lsn=*/4, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 4); + EXPECT_EQ(trigger_.call_count, 1); +} + +// A single call can advance commit_lsn by far more than one interval's width (e.g. a large +// catch-up). The claim must reset last_checkpoint_lsn_ to the ACTUAL commit_lsn reached (50), not +// to last_checkpoint_lsn_ + interval (-1 + 5 = 4) -- the two are indistinguishable in +// CheckpointTriggerAccumulatesAcrossCalls above (both land on 4 there), so this pins it down with a +// jump big enough to tell them apart: a second, small follow-up advance must NOT refire, which it +// would if the baseline had been left at 4 instead of 50. +TEST_F(CraftRaftEntriesTest, CheckpointTriggerBaselineTracksActualReachedValue) { + dev_->set_checkpoint_trigger(&trigger_); + dev_->set_checkpoint_lsn_interval(5); + dev_->seed_lsns(60, {}); + + auto r1 = do_apply(/*rs_commit_lsn=*/50, /*client_token=*/0); + ASSERT_TRUE(r1.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 50); + EXPECT_EQ(trigger_.call_count, 1); + + auto r2 = do_apply(/*rs_commit_lsn=*/51, /*client_token=*/0); + ASSERT_TRUE(r2.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 51); + EXPECT_EQ(trigger_.call_count, 1); // delta since the real baseline (50) is only 1 -- must not refire +} + +// No checkpoint_trigger_ wired (production not yet wired, same posture as peer_fetcher_): crossing +// the interval must not crash or fail the apply, just skip the trigger. +TEST_F(CraftRaftEntriesTest, CheckpointTriggerNoOpsWhenUnwired) { + dev_->set_checkpoint_lsn_interval(5); + dev_->seed_lsns(10, {}); + + auto r = do_apply(/*rs_commit_lsn=*/10, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 10); +} + +// A checkpoint trigger failure is logged, not propagated -- best-effort, same posture as this +// function's catch-up/fetch failure handling. +TEST_F(CraftRaftEntriesTest, CheckpointTriggerFailureDoesNotFailApply) { + dev_->set_checkpoint_trigger(&trigger_); + dev_->set_checkpoint_lsn_interval(5); + trigger_.fail_next = true; + dev_->seed_lsns(10, {}); + + auto r = do_apply(/*rs_commit_lsn=*/10, /*client_token=*/0); + + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 10); + EXPECT_EQ(trigger_.call_count, 1); +} + // ── on_commit dispatch ───────────────────────────────────────────────────────── TEST_F(CraftRaftEntriesTest, OnCommitDispatchesSyncRSCommitLSN) { diff --git a/src/lib/home_blks_config.fbs b/src/lib/home_blks_config.fbs index 56ad7d4..526b8bf 100644 --- a/src/lib/home_blks_config.fbs +++ b/src/lib/home_blks_config.fbs @@ -22,7 +22,9 @@ table HomeBlksSettings{ // homestore dataservice chunk size; hs_data_chunk_size_mb: uint32 = 2048; - // how often (in appended LSNs) the leader auto-proposes a SyncRSCommitLSN entry; + // how often (in appended LSNs) the leader auto-proposes a SyncRSCommitLSN entry; also gates how + // often a commit_lsn advance triggers a HomeStore checkpoint (SDSTOR-22888, CraftReplDev:: + // checkpoint_lsn_interval_), tying checkpoint cadence to this same periodic cadence; sync_rs_commit_lsn_interval: uint32 = 128; // deadline (in milliseconds) for a server-to-server peer fetch_data() call; From a5062e9781b44be0c7401ab8cc0fb2d94d923b49 Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Wed, 9 Sep 2026 14:07:45 -0700 Subject: [PATCH 14/16] craft: fix inverted to_free condition in apply_sync_rs_commit_lsn to_free was freeing lsns that *were* missing (nothing to free there) instead of ones that held real local data (<= last_append_lsn, not missing) -- exactly the leak shosseinimotlagh flagged on PR #176 and Copilot's review re-caught. Also guards against double-freeing an lsn already verdicted Empty. Adds a free_data_calls counter to test_craft_raft_entries.cpp's mock and locks in all four branches of the condition. --- src/lib/craft/craft_repl_dev.cpp | 5 ++- .../craft/tests/test_craft_raft_entries.cpp | 37 ++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index f6a5851..970ba29 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -661,7 +661,10 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 term = state_.term; for (int64_t lsn : empty_slots) { - if (missing_lsns_.erase(lsn)) { to_free.push_back(lsn); } + bool const was_missing = missing_lsns_.erase(lsn) > 0; + if (!was_missing && lsn <= state_.last_append_lsn && !empty_lsns_.contains(lsn)) { + to_free.push_back(lsn); + } } empty_lsns_.insert(empty_slots.begin(), empty_slots.end()); diff --git a/src/lib/craft/tests/test_craft_raft_entries.cpp b/src/lib/craft/tests/test_craft_raft_entries.cpp index 1a193ca..fdec525 100644 --- a/src/lib/craft/tests/test_craft_raft_entries.cpp +++ b/src/lib/craft/tests/test_craft_raft_entries.cpp @@ -69,6 +69,7 @@ class MockCraftJournalBackend : public CraftJournalBackend { public: std::map< int64_t, JournalSlot > slots; std::optional< int64_t > fail_on_write; + int free_data_calls{0}; async_result< homestore::multi_blk_id > alloc_write_data(sisl::sg_list const&, lba_count_t) override { co_return homestore::multi_blk_id{}; @@ -86,7 +87,10 @@ class MockCraftJournalBackend : public CraftJournalBackend { async_status truncate_to(int64_t) override { co_return ok(); } - async_status free_data(homestore::multi_blk_id) override { co_return ok(); } + async_status free_data(homestore::multi_blk_id) override { + ++free_data_calls; + co_return ok(); + } async_status free_slot(int64_t lsn) override { return mock_free_slot(*this, lsn); } @@ -246,6 +250,35 @@ TEST_F(CraftRaftEntriesTest, EmptySlotsReconciled) { EXPECT_TRUE(dev_->is_empty_slot(3)); EXPECT_FALSE(dev_->is_missing(3)); EXPECT_EQ(dev_->commit_lsn(), 5); + // lsn=3 was missing -- never had local data, so verdicting it Empty must not free anything. + EXPECT_EQ(journal_->free_data_calls, 0); +} + +TEST_F(CraftRaftEntriesTest, EmptySlotOverLocalDataFreesBlock) { + dev_->seed_lsns(5, {}); + journal_->slots[3] = JournalSlot{.lsn = 3, .all_zeros = false, .lba_off_bytes = 0, .len_bytes = 4}; + + auto r = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/0, /*empty_slots=*/{3}); + + ASSERT_TRUE(r.has_value()); + EXPECT_TRUE(dev_->is_empty_slot(3)); + EXPECT_EQ(journal_->free_data_calls, 1); +} + +// Double-free guard: an lsn already verdicted Empty by a prior apply must not be re-freed if it +// appears again in a later (redundant/overlapping) SyncRSCommitLSN's empty_slots -- nothing in the +// protocol strictly forbids this, and re-adding it here would double-free the same blkid. +TEST_F(CraftRaftEntriesTest, EmptySlotAlreadyVerdictedNotFreedAgain) { + dev_->seed_lsns(5, {}); + journal_->slots[3] = JournalSlot{.lsn = 3, .all_zeros = false, .lba_off_bytes = 0, .len_bytes = 4}; + + auto first = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/0, /*empty_slots=*/{3}); + ASSERT_TRUE(first.has_value()); + EXPECT_EQ(journal_->free_data_calls, 1); + + auto second = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/0, /*empty_slots=*/{3}); + ASSERT_TRUE(second.has_value()); + EXPECT_EQ(journal_->free_data_calls, 1); // unchanged -- not freed a second time } // An empty_slots entry can also fall inside the range this same apply newly opens (rather than being @@ -265,6 +298,8 @@ TEST_F(CraftRaftEntriesTest, EmptySlotWithinNewGapRangeNotDoubleTracked) { EXPECT_TRUE(dev_->is_missing(5)); EXPECT_EQ(dev_->missing_count(), 4u); EXPECT_EQ(dev_->commit_lsn(), 0); // stalls at lsn=1, still missing -- no peer_fetcher_ wired + // lsn=3 was beyond last_append_lsn (0) at apply time -- never locally appended, so nothing to free. + EXPECT_EQ(journal_->free_data_calls, 0); } // ── watermark advance ────────────────────────────────────────────────────────── From 63db36e40a0370ea669dd2d2895a55fbdd40f1c2 Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Wed, 9 Sep 2026 14:32:58 -0700 Subject: [PATCH 15/16] craft: validate magic/version/lsn in free_slot before trusting the entry Corrupt or foreign records could get misread as a valid blkid otherwise. Flagged by Copilot's review on PR #176. Adds real-log-store tests for both a legitimate entry and a rejected corrupt one. --- src/lib/craft/craft_repl_dev.cpp | 9 ++++ .../tests/test_craft_homestore_backend.cpp | 46 +++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index 970ba29..1e65720 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -203,6 +203,15 @@ class HomeStoreCraftJournalBackend : public CraftJournalBackend { } CraftJournalEntry hdr{}; std::memcpy(&hdr, buf.bytes(), sizeof(CraftJournalEntry)); + if (hdr.magic != k_journal_magic || hdr.version != k_journal_version) { + LOGE("free_slot: corrupt or foreign entry lsn={} magic={:#x} version={} -- refusing to free", lsn, + hdr.magic, hdr.version); + co_return std::unexpected(std::make_error_condition(std::errc::io_error)); + } + if (hdr.lsn != lsn) { + LOGE("free_slot: lsn mismatch requested={} stored={} -- refusing to free", lsn, hdr.lsn); + co_return std::unexpected(std::make_error_condition(std::errc::io_error)); + } if (hdr.all_zeros) co_return ok(); homestore::multi_blk_id blkid{}; diff --git a/src/lib/craft/tests/test_craft_homestore_backend.cpp b/src/lib/craft/tests/test_craft_homestore_backend.cpp index b74097f..486e07e 100644 --- a/src/lib/craft/tests/test_craft_homestore_backend.cpp +++ b/src/lib/craft/tests/test_craft_homestore_backend.cpp @@ -31,7 +31,10 @@ // craft_repl_dev.cpp directly to avoid HomeStore bring-up) because a real home_log_store requires // a running HomeStore instance. +#include #include +#include +#include #include #include @@ -115,6 +118,49 @@ TEST_F(CraftHomeStoreBackendTest, AllocWriteDataFailsCleanlyForUnregisteredOrdin ASSERT_FALSE(alloc_r.has_value()); } +// free_slot reads the raw entry back off the log store and validates magic/version/lsn before trusting it. +TEST_F(CraftHomeStoreBackendTest, FreeSlotSucceedsForRealEntry) { + auto logstore = make_logstore(); + ASSERT_TRUE(logstore != nullptr); + auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0); + + auto w = homeblocks::detail::sync_get(backend->write_slot(/* lsn = */ 0, /* term = */ 1, /* lba = */ 0, + /* len = */ 4096, homestore::multi_blk_id{}, + /* all_zeros = */ true)); + ASSERT_TRUE(w.has_value()); + + auto r = homeblocks::detail::sync_get(backend->free_slot(0)); + ASSERT_TRUE(r.has_value()); +} + +// Writes a raw blob directly to the log store (bypassing write_slot's serialization entirely) that +// doesn't conform to CraftJournalEntry's magic/version -- simulates a corrupt or foreign record. +// free_slot must reject it rather than misreading garbage bytes as a valid blkid. +TEST_F(CraftHomeStoreBackendTest, FreeSlotRejectsCorruptEntry) { + auto logstore = make_logstore(); + ASSERT_TRUE(logstore != nullptr); + auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0); + + std::vector< uint8_t > garbage(64, 0xEE); // larger than sizeof(CraftJournalEntry); not its magic/version + sisl::io_blob raw_blob{garbage.data(), static_cast< uint32_t >(garbage.size()), /* is_aligned = */ false}; + + std::mutex mu; + std::condition_variable cv; + bool done = false; + logstore->write_async(/* seq_num = */ 0, raw_blob, nullptr, + [&](homestore::logstore_seq_num_t, sisl::io_blob&, homestore::logdev_key, void*) { + std::lock_guard< std::mutex > lk{mu}; + done = true; + cv.notify_one(); + }); + std::unique_lock< std::mutex > lk{mu}; + cv.wait(lk, [&] { return done; }); + lk.unlock(); + + auto r = homeblocks::detail::sync_get(backend->free_slot(0)); + ASSERT_FALSE(r.has_value()); +} + // force=false: the value apply_sync_rs_commit_lsn's periodic trigger actually passes today. TEST_F(CraftHomeStoreBackendTest, CheckpointTriggerFlushesRealCPManager) { auto trigger = make_homestore_checkpoint_trigger(); From f29b5814ffc0d59c369c882d11f5f6daddd73d3c Mon Sep 17 00:00:00 2001 From: sbinmalek Date: Wed, 9 Sep 2026 15:28:50 -0700 Subject: [PATCH 16/16] craft: dedupe to_free with a set to_free was a vector, so a duplicate lsn in a single empty_slots list would call free_slot on the same lsn twice -- a double-free. Switched to unordered_set. Adds a test for the intra-batch duplicate case. Found during review of PR #176's changes. --- src/lib/craft/craft_repl_dev.cpp | 11 ++++++----- src/lib/craft/tests/test_craft_raft_entries.cpp | 12 ++++++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index 1e65720..03cf4e6 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -654,7 +654,7 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 } } - std::vector< int64_t > to_free; + std::unordered_set< int64_t > to_free; std::vector< int64_t > to_fetch; uint64_t term; { @@ -671,9 +671,7 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 for (int64_t lsn : empty_slots) { bool const was_missing = missing_lsns_.erase(lsn) > 0; - if (!was_missing && lsn <= state_.last_append_lsn && !empty_lsns_.contains(lsn)) { - to_free.push_back(lsn); - } + if (!was_missing && lsn <= state_.last_append_lsn && !empty_lsns_.contains(lsn)) { to_free.insert(lsn); } } empty_lsns_.insert(empty_slots.begin(), empty_slots.end()); @@ -736,7 +734,10 @@ async_status CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint6 blkid_allocated = true; } - // FIXME: We need to address the case when blkid is not set. How would write_slot handle that? + // FIXME: write_slot has no all_zeros branch -- it serializes whatever blkid it's given + // relying on multi_blk_id's own serialize()/serialized_size() to degrade safely for a + // default instance. That's an implicit, undocumented dependency on HomeStore's current + // behavior -- see SDSTOR-25613. auto res = co_await journal_->write_slot(slot.lsn, term, slot.lba_off_bytes, slot.len_bytes, blkid, slot.all_zeros); if (!res) { diff --git a/src/lib/craft/tests/test_craft_raft_entries.cpp b/src/lib/craft/tests/test_craft_raft_entries.cpp index fdec525..b1eebf1 100644 --- a/src/lib/craft/tests/test_craft_raft_entries.cpp +++ b/src/lib/craft/tests/test_craft_raft_entries.cpp @@ -281,6 +281,18 @@ TEST_F(CraftRaftEntriesTest, EmptySlotAlreadyVerdictedNotFreedAgain) { EXPECT_EQ(journal_->free_data_calls, 1); // unchanged -- not freed a second time } +// Same double-free guard, but for a duplicate *within* one empty_slots list rather than across two +// calls +TEST_F(CraftRaftEntriesTest, EmptySlotDuplicatedWithinSameBatchFreedOnce) { + dev_->seed_lsns(5, {}); + journal_->slots[3] = JournalSlot{.lsn = 3, .all_zeros = false, .lba_off_bytes = 0, .len_bytes = 4}; + + auto r = do_apply(/*rs_commit_lsn=*/5, /*client_token=*/0, /*empty_slots=*/{3, 3}); + + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(journal_->free_data_calls, 1); +} + // An empty_slots entry can also fall inside the range this same apply newly opens (rather than being // an already-missing LSN from before) -- it must end up ONLY in empty_lsns_, not re-added to // missing_lsns_ by the gap-marking step that runs right after reconciliation.