diff --git a/conanfile.py b/conanfile.py index 5cc084b..60951b5 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/include/homeblks/home_blocks.hpp b/src/include/homeblks/home_blocks.hpp index 0afcd98..79e4b78 100644 --- a/src/include/homeblks/home_blocks.hpp +++ b/src/include/homeblks/home_blocks.hpp @@ -149,7 +149,7 @@ inline std::error_condition make_error_condition(volume_error e) noexcept { async_result< size_t > async_read(volume_handle const& vol, uint64_t addr, sisl::sg_list sgs); [[nodiscard]] [[deprecated("legacy block op; use the CRAFT async_read/async_write overloads below (see docs/craft)")]] async_result< size_t > async_write(volume_handle const& vol, uint64_t addr, sisl::sg_list sgs); -[[nodiscard]] [[deprecated("legacy block op; use CRAFT async_write(..., all_zeros=true) (see docs/craft)")]] +[[nodiscard]] [[deprecated("legacy block op; use CRAFT async_write with empty data (see docs/craft)")]] async_status async_unmap(volume_handle const& vol, uint64_t addr, uint64_t len); // ---- CRAFT data plane: free functions over a volume_handle (one handle == one replica device) ---- @@ -174,17 +174,15 @@ async_status async_unmap(volume_handle const& vol, uint64_t addr, uint64_t len); // Append one client-assigned write at slot `dlsn`. `addr`/`len` are BYTE offset/length and must be // aligned to the volume's lba_size (from craft::LoginResult), else std::errc::invalid_argument. `data` is a -// caller-owned (iomgr) buffer: set `all_zeros=true` for a WRITE_ZEROES/unmap over [addr, addr+len) -- -// `data` must be empty in that case; otherwise this is a data write of exactly `len` bytes and `data` -// must be non-empty. The flag, not data emptiness, is what selects the write kind -- an empty buffer -// with all_zeros=false (or vice versa) is rejected as std::errc::invalid_argument, not silently -// reinterpreted. Not applied to the index directly; `hdr.commit_lsn` rides along and advances the -// frontier best-effort in dLSN order (CRAFT's piggybacked commit). STALE_TERM if hdr.term != session term. +// caller-owned (iomgr) buffer: pass empty `data` (size==0) for a WRITE_ZEROES/unmap over [addr, addr+len) +// (metadata-only; no block allocation); pass non-empty `data` of exactly `len` bytes for a data write. +// The write kind is determined by data.empty() -- no separate flag. Not applied to the index directly; +// `hdr.commit_lsn` rides along and advances the frontier best-effort in dLSN order (CRAFT's piggybacked +// commit). STALE_TERM if hdr.term != session term. // The ack returns the replica's achieved {commit_lsn, last_append_lsn}: every CRAFT IO response piggybacks // the watermarks, so any round-trip refreshes the client's per-member model without a keep_alive. [[nodiscard]] async_result< craft::lsn_pair > async_write(volume_handle const& vol, craft::client_hdr hdr, int64_t dlsn, - uint64_t addr, uint64_t len, sisl::sg_list data, - bool all_zeros = false); + uint64_t addr, uint64_t len, sisl::sg_list data); // Read the latest version <= `read_lsn` (horizon H) for [addr, addr+len) (BYTE offset/length, aligned to // lba_size). Fills the caller-owned `dest` buffer in place -- data sub-ranges get their bytes, holes get diff --git a/src/lib/craft/craft_api.cpp b/src/lib/craft/craft_api.cpp index 470adcb..0adf46e 100644 --- a/src/lib/craft/craft_api.cpp +++ b/src/lib/craft/craft_api.cpp @@ -51,10 +51,10 @@ async_status logout(volume_handle const& vol, craft::client_hdr hdr) { } async_result< craft::lsn_pair > async_write(volume_handle const& vol, craft::client_hdr hdr, int64_t dlsn, - uint64_t addr, uint64_t len, sisl::sg_list data, bool all_zeros) { + uint64_t addr, uint64_t len, sisl::sg_list data) { auto* d = craft_dev_of(vol); if (!d) co_return no_craft_backend(); - co_return co_await d->write(hdr, dlsn, addr, len, std::move(data), all_zeros); + co_return co_await d->write(hdr, dlsn, addr, len, std::move(data)); } async_result< craft::read_result > async_read(volume_handle const& vol, craft::client_hdr hdr, int64_t read_lsn, diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index 013eef2..ac9f59d 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -15,21 +15,35 @@ #include "craft_repl_dev.hpp" +#include #include #include #include // data_service(), async_alloc_write, blk_alloc_hints +#include // crc16_t10dif -- same routine and seed volume.cpp uses #include // home_log_store, logstore_seq_num_t, log_write_comp_cb_t #include // iomanager singleton, reactor_regex +#include // iomgr::schedule_recurring -- the watchdog's RAII timer #include // value_awaitable: lock-free completion-before-suspend-safe bridge +#include // hs() -- index_table.hpp calls it, relying on this being included first +#include // IndexTable -- prerequisite index_fixed_table.hpp relies on + // being already visible (volume.hpp provides both of these, + // in this order, before including index_fixed_table.hpp; + // that header isn't self-contained) +#include "../volume/index_fixed_table.hpp" // VolumeIndexTable::write_to_index / delete_lba_range +#include "../coro_helpers.hpp" // detail::detach -- fire-and-forget a coroutine from a timer callback +#include "home_blks_config.hpp" // HB_DYNAMIC_CONFIG -- watchdog timeout from settings + namespace homeblocks { // ─── Journal entry on-disk format ───────────────────────────────────────────── // -// Each log slot is: [CraftJournalEntry header][serialized multi_blk_id bytes]. +// Each log slot is: [CraftJournalEntry header][csum_t per LBA][serialized multi_blk_id bytes]. // The payload (HS_DATA_LINKED) is written directly to the data service; only the -// block reference is stored here. +// block reference is stored here. The checksum array is empty for all_zeros slots +// (no data, nothing to sum) and otherwise has exactly len/lba_size entries, computed +// on the write path while the data is still in memory (see CraftReplDev::write()). static constexpr uint32_t k_journal_magic = 0xC4AF5AFE; // "CRAFT SAFE" — corrupt or non-CRAFT slots fail this static constexpr uint8_t k_journal_version = 1; @@ -60,14 +74,44 @@ static_assert(sizeof(CraftJournalEntry) == 34, "CraftJournalEntry is a persisted on-disk format -- " "a layout change here is a format migration, not a code change"); +// Same CRC16 seed volume.cpp's non-CRAFT write/read path already uses -- keep both paths on one +// checksum algorithm rather than inventing a second one for CRAFT slots. +static constexpr homestore::csum_t k_craft_crc16_seed = 0x8005; + +// Upper bound on a single write()/read() request's byte length -- both are client-wire-reachable +// (S9 CraftConnector), so len is untrusted input. Three concrete failure modes this closes, all +// reachable from a single malformed/adversarial request without this bound: +// - read_impl() would allocate O(nlbas) heap (a std::vector/ sized to len/lba_size_) +// with no cap -- a multi-terabyte len drives a multi-gigabyte allocation attempt (OOM) from one +// request. +// - write()'s len is narrowed to lba_count_t (uint32_t) before being journaled +// (CraftJournalEntry::len is also uint32_t, an on-disk format constraint); an unbounded len could +// silently truncate to a value whose own nlbas is 0, permanently stalling commit_impl() on that +// slot forever (nlbas==0 there is a hard abort, not a skip). +// - nlbas computed from an unbounded len can itself land on exactly 0 (len/lba_size_ truncating a +// huge value down via uint32_t wraparound), which would underflow end_lba = start_lba + nlbas - 1 +// into a near-UINT64_MAX range fed straight to the index/BTree. +// Byte offset of the csum_t array within a slot's blob (right after the fixed header). +static constexpr uint32_t k_csum_array_offset = sizeof(CraftJournalEntry); + +// Byte offset of the serialized multi_blk_id within a slot's blob, given how many csum_t entries +// precede it. +static uint32_t blkid_offset(uint32_t nlbas) { + return k_csum_array_offset + nlbas * static_cast< uint32_t >(sizeof(homestore::csum_t)); +} + +// Total blob size for a slot: header + csum array + serialized blkid. +static uint32_t slot_blob_size(uint32_t nlbas, uint32_t blkid_sz) { return blkid_offset(nlbas) + blkid_sz; } + // ─── HomeStore journal backend ───────────────────────────────────────────────── // // Production backend: wraps a HomeStore home_log_store. class HomeStoreCraftJournalBackend : public CraftJournalBackend { public: - explicit HomeStoreCraftJournalBackend(shared< homestore::home_log_store > logstore, uint64_t vol_ordinal) : - logstore_{std::move(logstore)}, vol_ordinal_{vol_ordinal} {} + explicit HomeStoreCraftJournalBackend(shared< homestore::home_log_store > logstore, uint64_t vol_ordinal, + uint32_t lba_size) : + logstore_{std::move(logstore)}, vol_ordinal_{vol_ordinal}, lba_size_{lba_size} {} // Allocate blocks via the HomeStore data service and write the payload (zero-copy). // application_hint routes the allocation to this volume's chunk set via VolumeChunkSelector. @@ -85,19 +129,23 @@ class HomeStoreCraftJournalBackend : public CraftJournalBackend { co_return blkid; } - // Serialize the journal entry (header + blkid) and write it to the log store. + // Serialize the journal entry (header + csum array + blkid) and write it to the log store. 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 { + bool all_zeros, std::vector< homestore::csum_t > const& csums) override { CraftJournalEntry hdr{ k_journal_magic, k_journal_version, term, lsn, lba, len, static_cast< uint8_t >(all_zeros)}; + uint32_t nlbas = static_cast< uint32_t >(csums.size()); uint32_t blkid_sz = blkid.serialized_size(); - sisl::io_blob_safe blob{static_cast< uint32_t >(sizeof(CraftJournalEntry)) + blkid_sz}; + sisl::io_blob_safe blob{slot_blob_size(nlbas, blkid_sz)}; std::memcpy(blob.bytes(), &hdr, sizeof(CraftJournalEntry)); + if (nlbas > 0) { + std::memcpy(blob.bytes() + k_csum_array_offset, csums.data(), nlbas * sizeof(homestore::csum_t)); + } sisl::blob blkid_blob = blkid.serialize(); // non-owning view — copy before blkid goes out of scope // Internal HomeStore-API contract, not client-reachable: serialize() must return exactly // serialized_size() bytes, or the memcpy below overreads blkid_blob's owned buffer. DEBUG_ASSERT_EQ(blkid_blob.size(), blkid_sz, "multi_blk_id::serialize() size mismatch"); - std::memcpy(blob.bytes() + sizeof(CraftJournalEntry), blkid_blob.cbytes(), blkid_sz); + std::memcpy(blob.bytes() + blkid_offset(nlbas), blkid_blob.cbytes(), blkid_sz); // Bridge write_async (callback) to co_await via value_awaitable. // • Deadlock-safe: the callback posts va->complete() to an iomgr reactor via run_on_forget, // decoupling coroutine resume from LogDev::m_flush_mtx. In production HomeBlocks always has @@ -140,9 +188,58 @@ class HomeStoreCraftJournalBackend : public CraftJournalBackend { co_return ok(); } + // Reads and parses a slot's blob back into a JournalSlot. read_sync is a blocking call (no async + // log-read API exists in HomeStore today) -- acceptable here: callers are commit/apply (inherently + // serial) and startup-only overlay rebuild, neither of which needs concurrency on this path. async_result< JournalSlot > read_slot(int64_t lsn) override { - LOGW("HomeStoreCraftJournalBackend::read_slot lsn={} not yet implemented", lsn); - co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); + homestore::log_buffer buf; + try { + // Throws std::out_of_range for a truncated or never-appended seq_num -- not an I/O error. + buf = logstore_->read_sync(static_cast< homestore::logstore_seq_num_t >(lsn)); + } catch (std::out_of_range const&) { + co_return std::unexpected(std::make_error_condition(std::errc::result_out_of_range)); + } + + // Validate-before-trust: check the blob is large enough for each region before reading any + // length field out of it, so a truncated/corrupt record fails cleanly instead of overreading. + if (buf.size() < sizeof(CraftJournalEntry)) { + LOGE("read_slot lsn={} blob too small for header: size={}", lsn, buf.size()); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } + CraftJournalEntry hdr; + std::memcpy(&hdr, buf.bytes(), sizeof(CraftJournalEntry)); + if (hdr.magic != k_journal_magic || hdr.version != k_journal_version) { + LOGE("read_slot lsn={} bad magic/version: magic={:#x} version={}", lsn, hdr.magic, hdr.version); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } + + JournalSlot slot; + slot.lsn = hdr.lsn; + slot.all_zeros = hdr.all_zeros != 0; + slot.lba_off_bytes = hdr.lba; + slot.len_bytes = hdr.len; + + uint32_t nlbas = slot.all_zeros ? 0 : (hdr.len / lba_size_); + uint32_t csum_bytes = nlbas * static_cast< uint32_t >(sizeof(homestore::csum_t)); + if (buf.size() < k_csum_array_offset + csum_bytes) { + LOGE("read_slot lsn={} blob too small for csum array: size={} need={}", lsn, buf.size(), + k_csum_array_offset + csum_bytes); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } + if (nlbas > 0) { + slot.csums.resize(nlbas); + std::memcpy(slot.csums.data(), buf.bytes() + k_csum_array_offset, csum_bytes); + } + + uint32_t blkid_off = blkid_offset(nlbas); + if (buf.size() < blkid_off) { + LOGE("read_slot lsn={} blob too small for blkid: size={} need>={}", lsn, buf.size(), blkid_off); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } + sisl::blob blkid_blob{buf.bytes() + blkid_off, buf.size() - blkid_off}; + slot.blkid.deserialize(blkid_blob, /* copy = */ true); + + co_return slot; } // Drop all journal entries with seq_num > lsn; lsn becomes the new tail. @@ -162,20 +259,55 @@ class HomeStoreCraftJournalBackend : public CraftJournalBackend { co_return ok(); } + // dest.size must already be set by the caller to blkid's byte length (matches async_alloc_write's + // own contract -- data_service() sizes the I/O from the sg_list, not from a separate parameter). + async_status read_data(homestore::multi_blk_id blkid, sisl::sg_list& dest) override { + auto res = co_await homestore::data_service().async_read(blkid, dest, dest.size, nullptr); + if (!res) { + LOGE("async_read failed: {}", res.error().message()); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } + co_return ok(); + } + private: shared< homestore::home_log_store > logstore_; uint64_t vol_ordinal_; + uint32_t lba_size_; }; unique< CraftJournalBackend > make_homestore_journal_backend(shared< homestore::home_log_store > logstore, - uint64_t vol_ordinal) { - return std::make_unique< HomeStoreCraftJournalBackend >(std::move(logstore), vol_ordinal); + uint64_t vol_ordinal, uint32_t lba_size) { + return std::make_unique< HomeStoreCraftJournalBackend >(std::move(logstore), vol_ordinal, lba_size); } // ─── constructor ────────────────────────────────────────────────────────────── -CraftReplDev::CraftReplDev(volume_id_t vol_id, unique< CraftJournalBackend > journal) : - vol_id_{vol_id}, journal_{std::move(journal)}, raft_listener_{this} {} +CraftReplDev::CraftReplDev(volume_id_t vol_id, unique< CraftJournalBackend > journal, uint32_t lba_size, + shared< VolumeIndexTable > indx_tbl) : + vol_id_{vol_id}, + journal_{std::move(journal)}, + lba_size_{lba_size}, + indx_tbl_{std::move(indx_tbl)}, + raft_listener_{this}, + watchdog_timeout_ns_{HB_DYNAMIC_CONFIG(craft_watchdog_timeout_ms) * 1'000'000ULL} {} + +CraftReplDev::~CraftReplDev() { + // iomgr::timer_token::cancel(wait=true) is a no-op if the watchdog was never armed (watchdog disabled + // via craft_watchdog_timeout_ms == 0, or no write()/keep_alive() ever succeeded). Otherwise it removes the + // recurring timer's underlying IODevice from its reactor via iomanager.run_on_wait(), which blocks THIS thread + // until that removal has actually run on the reactor thread that also runs on_watchdog_tick(). The precise + // mechanism that makes this race-free (verified against iomgr source, not assumed): the epoll + // reactor's listen() loop tracks removed-this-batch iodevs in m_removed_iodevs and skips any + // already-queued event for one (reactor_epoll.cpp's listen()/remove_iodev_impl) -- so even a timer + // event sitting in the SAME epoll batch as the removal is discarded rather than dispatched. Once + // cancel() returns, on_watchdog_tick() is guaranteed to never fire again against this (about to be + // destroyed) object. No generation counter, in-flight counter, or shutting-down flag needed: this + // single call is iomgr's own safety guarantee, not a hand-rolled one. (If iomgr ever ships a non- + // epoll reactor backend without an equivalent same-batch-removal mechanism, this guarantee would need + // re-verifying against that backend specifically.) + watchdog_token_.cancel(/* wait = */ true); +} // ─── get_lsns / get_rs_commit_lsn ──────────────────────────────────────────── // Snapshot the in-memory partition state under missing_mu_ for consistency with @@ -202,10 +334,13 @@ async_result< craft::lsn_pair > CraftReplDev::get_rs_commit_lsn(uint64_t /* term // ─── truncate (S4) ──────────────────────────────────────────────────────────── // // Called only during the login sequence, while no writes are in-flight (the -// CRAFT write path is quiesced by login serialisation). Three atomic steps: +// CRAFT write path is quiesced by login serialisation). Four atomic steps: // 1. Journal rollback: drop all entries with dLSN > lsn (tail truncation). // 2. Clamp last_append_lsn to lsn if it is higher. // 3. Erase all missing-set entries above lsn. +// 4. Prune any journal-tail overlay entry referencing a now-rolled-back dLSN (see the S3 overlay's +// own doc comment on overlay_ in the header) -- otherwise a stale entry above lsn would keep +// pointing at a write the journal no longer has. // commit_lsn is not touched: the new rs_commit_lsn passed by the caller is the // dLSN up to which RAFT consensus has RESOLVED entries. Entries above that are // the ones being dropped. The missing set tracks gaps in [commit_lsn+1, @@ -229,6 +364,21 @@ async_status CraftReplDev::truncate(int64_t lsn) { if (state_.last_append_lsn > lsn) state_.last_append_lsn = lsn; missing_lsns_.erase(missing_lsns_.upper_bound(lsn), missing_lsns_.end()); } + + // Step 4: prune any overlay entry referencing a now-rolled-back dLSN. Without this, a stale + // entry above lsn would keep pointing at a write the journal no longer has -- a subsequent read + // could serve data from a block that's since been freed or reallocated to something else + // entirely under the new term. + { + std::lock_guard lk{overlay_mu_}; + for (auto it = overlay_.begin(); it != overlay_.end();) { + if (it->second.lsn > lsn) { + it = overlay_.erase(it); + } else { + ++it; + } + } + } co_return ok(); } @@ -261,7 +411,7 @@ void CraftReplDev::seed_empty(std::initializer_list< int64_t > empty) { } #endif -// ─── stubs (S1/S3/S5/S7 implement these) ───────────────────────────────────── +// ─── stubs (login/logout are separate scope, not part of S3's commit/read path) ────────────────── async_result< craft::LoginResult > CraftReplDev::login(uint64_t /* client_token */) { LOGW("CraftReplDev::login not yet implemented"); @@ -274,18 +424,40 @@ async_status CraftReplDev::logout(craft::client_hdr /* hdr */) { } async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64_t dlsn, uint64_t addr, uint64_t len, - sisl::sg_list data, bool all_zeros) { + sisl::sg_list data) { + // Checked ahead of recovering_: a faulted restart recovery is permanent, not a "still starting up, + // try again shortly" condition -- see recovery_faulted_'s doc comment. + if (recovery_faulted_.load(std::memory_order_acquire)) { + LOGE("write rejected: partition permanently faulted after failed restart recovery dlsn={}", dlsn); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } + if (recovering_.load(std::memory_order_acquire)) { + LOGW("write rejected: overlay rebuild in progress after restart dlsn={}", dlsn); + co_return std::unexpected(make_error_condition(volume_error::OFFLINE)); + } if (dlsn < 0) { LOGW("write rejected: invalid dlsn={}", dlsn); co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); } - // Reject rather than abort: this condition is reachable from the client wire (S9 CraftConnector) - // so a RELEASE_ASSERT would let a malformed frame abort the entire replica process. all_zeros and - // data.size disagreeing either way is malformed: all_zeros=false requires a payload (the write()); - // all_zeros=true requires none (WRITE_ZEROES/unmap names a range, it does not also carry data). - if (all_zeros == (data.size > 0)) { - LOGW("write rejected: all_zeros={} disagrees with data.size={} dlsn={} addr={} len={}", all_zeros, data.size, - dlsn, addr, len); + const bool all_zeros = (data.size == 0); + // addr/len must be a positive, block-aligned byte range: nlbas = len / lba_size_ is used + // throughout (CRC computation, overlay population, and commit()'s later index-range + // application) as an LBA count/offset. len==0 (or a len that doesn't evenly divide lba_size_, + // silently rounding nlbas down to 0) would let nlbas==0 reach commit_impl's + // end_lba = start_lba + nlbas - 1, which underflows lba_t (unsigned) into a near-UINT64_MAX + // range applied to the real index -- an effectively unbounded loop reachable straight from the + // client wire. + const uint64_t max_io_len = HB_DYNAMIC_CONFIG(craft_max_io_len_mb) * 1024ULL * 1024ULL; + if (len == 0 || len % lba_size_ != 0 || addr % lba_size_ != 0 || len > max_io_len) { + LOGW("write rejected: addr={} len={} not block-aligned (lba_size={}) or exceeds max {} dlsn={}", addr, len, + lba_size_, max_io_len, dlsn); + co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); + } + // data.size must match len exactly: the CRC loop below reads nlbas*lba_size_ == len bytes from + // data.iovs[0]'s buffer, and a data.size that merely claims to match len without the backing + // iovec actually being that large would over-read past the caller's buffer. + if (!all_zeros && (data.iovs.empty() || data.size != len || data.iovs[0].iov_len < len)) { + LOGW("write rejected: data.size={} does not match len={} dlsn={}", data.size, len, dlsn); co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); } @@ -296,6 +468,10 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 LOGW("write rejected: stale term want={} got={} dlsn={}", state_.term, hdr.term, dlsn); co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); } + // Advance the reclaim floor from ordinary IO -- same max-monotonic pattern as keep_alive(). + // Without this, all_committed_lsn only moves via keep_alive() messages, so quiet partitions + // (write-only, no explicit keep_alive) never advance the reclaim floor at all. + state_.all_committed_lsn = std::max(state_.all_committed_lsn, hdr.all_committed_lsn); if (empty_lsns_.contains(dlsn)) { LOGW("write rejected: slot is permanently empty dlsn={}", dlsn); co_return std::unexpected(make_error_condition(volume_error::EMPTY_SLOT)); @@ -305,6 +481,16 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 LOGT("write idempotent: dlsn={} already written", dlsn); co_return craft::lsn_pair{state_.commit_lsn, state_.last_append_lsn}; } + // Reject a concurrent duplicate rather than racing it: two write() calls for the SAME dlsn + // arriving concurrently (a malformed/retried request -- a well-behaved client never does + // this for one dlsn) would otherwise both pass the idempotency check above (neither has + // advanced last_append_lsn yet) and both proceed to alloc_write_data below, doubly + // allocating real blocks for one dlsn -- only whichever write_slot call lands last would + // ever be referenced by the journal, permanently leaking the other's blocks. + if (in_flight_write_dlsns_.contains(dlsn)) { + LOGW("write rejected: dlsn={} already has a write in flight", dlsn); + co_return std::unexpected(make_error_condition(std::errc::operation_in_progress)); + } static constexpr int64_t k_max_ooo_gap = 1'000'000; // Guard 1: prevent signed overflow in the gap subtraction below (dlsn near INT64_MAX). if (dlsn > INT64_MAX - k_max_ooo_gap) { @@ -340,12 +526,26 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 // that never reached quorum -- Phase 1b finds no real holder and marks it Empty. // No data is lost; the only cost is an avoidable Empty verdict. state_.last_append_lsn = std::max(state_.last_append_lsn, dlsn); + in_flight_write_dlsns_.insert(dlsn); } + // Erased on every exit from here on (success or failure) -- see the in_flight_write_dlsns_ check + // above for why this exists. A coroutine's local objects are destroyed on any co_return exactly + // like a normal function's, so this RAII guard is safe across every path below, including ones + // that suspend (co_await) before returning. + struct InFlightDlsnGuard { + CraftReplDev* self; + int64_t dlsn; + ~InFlightDlsnGuard() { + std::lock_guard lk{self->missing_mu_}; + self->in_flight_write_dlsns_.erase(dlsn); + } + } in_flight_guard{this, dlsn}; // HS_DATA_LINKED: allocate blocks and write payload before journalling the block reference. // all_zeros=true skips this; the early-return above guarantees !all_zeros implies data.size > 0. homestore::multi_blk_id blkid{}; bool blkid_allocated = false; + std::vector< homestore::csum_t > csums; // one per LBA; stays empty for all_zeros (no data to sum) if (!all_zeros) { auto alloc_res = co_await journal_->alloc_write_data(data, static_cast< lba_count_t >(len)); if (!alloc_res) { @@ -354,13 +554,24 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 } blkid = *alloc_res; blkid_allocated = true; + + // Per-LBA CRC16, computed here while the payload is still in memory -- same routine and + // seed volume.cpp's non-CRAFT write path already uses (crc16_t10dif / init_crc_16). Walks a + // single flat buffer (data's first iovec), matching that same path's assumption; every + // sg_list CRAFT builds anywhere in this codebase today is a single iovec. + uint32_t nlbas = static_cast< uint32_t >(len / lba_size_); + csums.reserve(nlbas); + auto const* buf = static_cast< uint8_t const* >(data.iovs[0].iov_base); + for (uint32_t i = 0; i < nlbas; ++i) { + csums.push_back(crc16_t10dif(k_craft_crc16_seed, buf + i * lba_size_, lba_size_)); + } } // addr and len are BYTES (byte-addressed API): CraftJournalEntry stores them verbatim as bytes. // hdr.term is already verified against state_.term under missing_mu_ above; pass it so the // on-disk entry carries the session term for stale-tail detection on recovery. dlsn is stored // redundantly in CraftJournalEntry.lsn for self-describing recovery. auto res = co_await journal_->write_slot(dlsn, hdr.term, static_cast< lba_t >(addr), - static_cast< lba_count_t >(len), blkid, all_zeros); + static_cast< lba_count_t >(len), blkid, all_zeros, csums); if (!res) { LOGE("write_slot failed dlsn={} addr={} len={}: {}", dlsn, addr, len, res.error().message()); if (blkid_allocated) { @@ -397,20 +608,542 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 if (stale_post_flight) { co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); } + // Populate the journal-tail overlay: makes this entry locally readable ahead of commit() applying + // it to the index. + populate_overlay(dlsn, static_cast< lba_t >(addr) / lba_size_, static_cast< uint32_t >(len / lba_size_), all_zeros, + blkid, csums); + + // Best-effort piggyback: advance commit_lsn toward the client's own view of it. Every outcome -- + // a stall at a gap, or even a genuine commit() fault -- is ignored here; the write itself already + // succeeded, and the next write/keep_alive retries the advance. keep_alive() is where a real + // commit() error actually surfaces (advancing the frontier is its entire purpose). + co_await commit(hdr.commit_lsn); + + touch_watchdog(); + LOGT("write ok dlsn={} addr={} len={} all_zeros={}", dlsn, addr, len, all_zeros); co_return snapshot; } -async_result< craft::read_result > CraftReplDev::read(craft::client_hdr /* hdr */, int64_t /* read_lsn */, - uint64_t /* addr */, uint64_t /* len */, - sisl::sg_list /* dest */) { - LOGW("CraftReplDev::read not yet implemented"); - co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); +// ─── commit() (internal; never a wire op) ──────────────────────────────────── +// +// Advances commit_lsn toward upto_lsn by applying each committable slot to the index. At most one +// run is ever active at a time (commit_running_, guarded by missing_mu_ and reset via RAII on every +// exit path); a concurrent caller is a safe no-op -- the in-flight run covers the same ground, and +// every subsequent write()/keep_alive() retries the advance. Never holds a lock across the co_await +// read_slot() suspension point below (same rule fetch_data's doc comment already establishes). + +async_result< int64_t > CraftReplDev::commit_impl(int64_t upto_lsn, write_index_fn_t const& write_fn, + delete_index_fn_t const& delete_fn) { + int64_t commit_lsn, last_append_lsn; + { + std::lock_guard lk{missing_mu_}; + if (commit_running_) co_return state_.commit_lsn; // another run is already advancing + commit_running_ = true; + commit_lsn = state_.commit_lsn; + last_append_lsn = state_.last_append_lsn; + } + // Guaranteed reset on every exit path (stall, success, or error): a local RAII object's destructor + // runs when the coroutine frame unwinds, exactly like a plain function's locals on return. + struct RunningGuard { + CraftReplDev* self; + ~RunningGuard() { + std::lock_guard lk{self->missing_mu_}; + self->commit_running_ = false; + } + } guard{this}; + + int64_t const target = std::min(upto_lsn, last_append_lsn); + for (int64_t lsn = commit_lsn + 1; lsn <= target; ++lsn) { + bool is_missing, is_empty; + { + std::lock_guard lk{missing_mu_}; + is_missing = missing_lsns_.contains(lsn); + is_empty = empty_lsns_.contains(lsn); + } + if (is_missing) break; // stall at the first hole -- not an error + + if (!is_empty) { + auto slot_r = co_await journal_->read_slot(lsn); + if (!slot_r) co_return std::unexpected(slot_r.error()); + auto& slot = *slot_r; + + lba_t const start_lba = static_cast< lba_t >(slot.lba_off_bytes) / lba_size_; + uint32_t const nlbas = static_cast< uint32_t >(slot.len_bytes / lba_size_); + // Defense-in-depth: write() rejects len==0 at the client boundary, but a stale/legacy + // on-disk record could still have one. nlbas==0 must never reach the end_lba + // computation below -- start_lba + 0 - 1 underflows lba_t (unsigned) into a + // near-UINT64_MAX range that would be applied to the real index. + if (nlbas == 0) { + LOGE("commit: slot lsn={} has len_bytes={} (nlbas=0) -- malformed record, aborting", lsn, + slot.len_bytes); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } + lba_t const end_lba = start_lba + nlbas - 1; + + if (slot.all_zeros) { + std::vector< homestore::blk_id > freed; + if (auto r = delete_fn(start_lba, end_lba, freed); !r) co_return std::unexpected(r.error()); + // Reclaim inline, same as write()'s own free_data call sites -- free errors are + // logged but non-fatal (the block is merely leaked, not corrupted). + for (auto const& blk : freed) { + if (auto fr = co_await journal_->free_data(homestore::multi_blk_id{blk}); !fr) + LOGE("free_data failed reclaiming blk={} lsn={}: {}", blk.to_string(), lsn, + fr.error().message()); + } + } else { + // Decompose the slot's blkid into per-LBA single-block BlockInfo entries -- same shape + // as write()'s own overlay-population decomposition and volume.cpp's non-CRAFT path. + std::unordered_map< lba_t, BlockInfo > blocks_info; + auto pieces = slot.blkid.iterate(); + uint32_t csum_idx = 0; + lba_t lba = start_lba; + while (auto piece = pieces.next()) { + for (homestore::blk_count_t i = 0; i < piece->blk_count(); ++i, ++lba, ++csum_idx) { + // Defense-in-depth matching populate_overlay()'s identical guard: csums should + // have exactly nlbas entries (write() computes and stores them that way), but a + // corrupt on-disk blob could produce a shorter array -- an OOB read here would + // silently fabricate a checksum rather than signalling the corruption. + if (csum_idx >= slot.csums.size()) { + LOGE("commit: csums array shorter ({}) than blkid piece count (lba={} lsn={}) " + "-- corrupt on-disk record, aborting commit for this lsn", + slot.csums.size(), lba, lsn); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } + homestore::blk_id single_bid{static_cast< homestore::blk_num_t >(piece->blk_num() + i), 1, + piece->chunk_num()}; + blocks_info.emplace(lba, BlockInfo{single_bid, homestore::blk_id{}, slot.csums[csum_idx]}); + } + } + if (auto r = write_fn(start_lba, end_lba, blocks_info); !r) co_return std::unexpected(r.error()); + // Reclaim any superseded old blkid inline, same as write()'s own free_data call sites. + for (auto const& [_, info] : blocks_info) { + if (!info.old_blkid.is_valid()) continue; + if (auto fr = co_await journal_->free_data(homestore::multi_blk_id{info.old_blkid}); !fr) + LOGE("free_data failed reclaiming blk={} lsn={}: {}", info.old_blkid.to_string(), lsn, + fr.error().message()); + } + } + + // Retire the overlay entry for each applied LBA, but ONLY if its recorded lsn equals the + // lsn just applied -- a higher-dLSN overlay entry for the same LBA (a later write already + // appended but not yet committed) must survive. + std::lock_guard lk{overlay_mu_}; + for (lba_t l = start_lba; l <= end_lba; ++l) { + auto it = overlay_.find(l); + if (it != overlay_.end() && it->second.lsn == lsn) overlay_.erase(it); + } + } + + std::lock_guard lk{missing_mu_}; + state_.commit_lsn = lsn; + } + + std::lock_guard lk{missing_mu_}; + co_return state_.commit_lsn; } -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)); +async_result< int64_t > CraftReplDev::commit(int64_t upto_lsn) { + if (!indx_tbl_) { + // No index configured (write-path-only tests): nothing to apply, no-op safely. + std::lock_guard lk{missing_mu_}; + co_return state_.commit_lsn; + } + write_index_fn_t write_fn = [this](lba_t s, lba_t e, std::unordered_map< lba_t, BlockInfo >& info) { + return indx_tbl_->write_to_index(s, e, info); + }; + delete_index_fn_t delete_fn = [this](lba_t s, lba_t e, std::vector< homestore::blk_id >& freed) { + return indx_tbl_->delete_lba_range(s, e, freed); + }; + auto r = co_await commit_impl(upto_lsn, write_fn, delete_fn); + co_return r; +} + +#ifdef _PRERELEASE +async_result< int64_t > CraftReplDev::commit_with(int64_t upto_lsn, write_index_fn_t write_fn, + delete_index_fn_t delete_fn) { + auto r = co_await commit_impl(upto_lsn, write_fn, delete_fn); + co_return r; +} +#endif + +// ─── overlay population (shared by write() and rebuild_overlay()) ─────────── + +void CraftReplDev::populate_overlay(int64_t dlsn, lba_t start_lba, uint32_t nlbas, bool all_zeros, + homestore::multi_blk_id const& blkid, + std::vector< homestore::csum_t > const& csums) { + lba_t lba = start_lba; + std::lock_guard lk{overlay_mu_}; + if (all_zeros) { + for (uint32_t i = 0; i < nlbas; ++i, ++lba) { + auto it = overlay_.find(lba); + if (it == overlay_.end() || dlsn > it->second.lsn) { + overlay_[lba] = OverlayEntry{.lsn = dlsn, .all_zeros = true}; + } + } + } else { + // Decompose blkid's pieces (each a contiguous run of blocks) into single-block blk_ids, + // one per LBA -- same shape as volume.cpp's non-CRAFT write path (volume.cpp:239-253), + // generalized to multi_blk_id::iterate() since CRAFT's blkid may have more than one piece. + auto pieces = blkid.iterate(); + uint32_t csum_idx = 0; + while (auto piece = pieces.next()) { + for (homestore::blk_count_t i = 0; i < piece->blk_count(); ++i, ++lba, ++csum_idx) { + // Defense-in-depth: csums normally has exactly nlbas entries (write() computes it + // that way, and rebuild_overlay()'s read_slot() call is expected to validate the + // on-disk array length matches its own parsed nlbas) -- but rebuild_overlay()'s data + // ultimately comes from a persisted blob, so a corrupted/truncated on-disk record + // must not turn into an out-of-bounds read here. + if (csum_idx >= csums.size()) { + LOGE("populate_overlay: csums array shorter ({}) than blkid piece count needs (lba={} " + "dlsn={}) -- skipping remaining LBAs for this entry", + csums.size(), lba, dlsn); + return; + } + auto it = overlay_.find(lba); + if (it != overlay_.end() && dlsn <= it->second.lsn) continue; + homestore::blk_id single_bid{static_cast< homestore::blk_num_t >(piece->blk_num() + i), 1, + piece->chunk_num()}; + overlay_[lba] = OverlayEntry{.lsn = dlsn, .blkid = single_bid, .csum = csums[csum_idx]}; + } + } + } +} + +// ─── read() ─────────────────────────────────────────────────────────────────── +// +// Serves [addr, addr+len) as of read_lsn from the LBA index (committed, <= commit_lsn) merged with +// the journal-tail overlay (appended but not yet committed, horizon-clamped to (commit_lsn, read_lsn] +// so an overlay entry above read_lsn is held but never served -- the index's older, still-valid-as-of- +// read_lsn value is used instead). Never fetches from a peer. + +async_result< craft::read_result > CraftReplDev::read(craft::client_hdr hdr, int64_t read_lsn, uint64_t addr, + uint64_t len, sisl::sg_list dest) { + // Checked ahead of recovering_: a faulted restart recovery is permanent, not a "still starting up, + // try again shortly" condition -- see recovery_faulted_'s doc comment. + if (recovery_faulted_.load(std::memory_order_acquire)) { + LOGE("read rejected: partition permanently faulted after failed restart recovery"); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } + if (recovering_.load(std::memory_order_acquire)) { + LOGW("read rejected: overlay rebuild in progress after restart"); + co_return std::unexpected(make_error_condition(volume_error::OFFLINE)); + } + { + std::lock_guard lock{missing_mu_}; + if (hdr.term != state_.term) { + LOGW("read rejected: stale term want={} got={}", state_.term, hdr.term); + co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); + } + // Advance the reclaim floor from ordinary IO -- same max-monotonic pattern as keep_alive(). + state_.all_committed_lsn = std::max(state_.all_committed_lsn, hdr.all_committed_lsn); + } + // Best-effort piggyback, same reasoning as write()'s: read()'s own success does not depend on + // whether commit() advances further, so a genuine commit() fault here is swallowed rather than + // failing an otherwise-servable read. + co_await commit(hdr.commit_lsn); + + if (!indx_tbl_) { + LOGW("read rejected: no index configured"); + co_return std::unexpected(make_error_condition(std::errc::not_supported)); + } + read_index_fn_t read_fn = [this](lba_t s, lba_t e, index_kv_list_t& kvs) { + return indx_tbl_->read_from_index(s, e, kvs); + }; + auto r = co_await read_impl(read_lsn, addr, len, std::move(dest), read_fn); + co_return r; +} + +#ifdef _PRERELEASE +async_result< craft::read_result > CraftReplDev::read_with(int64_t read_lsn, uint64_t addr, uint64_t len, + sisl::sg_list dest, read_index_fn_t read_fn) { + auto r = co_await read_impl(read_lsn, addr, len, std::move(dest), read_fn); + co_return r; +} +#endif + +// Core apply algorithm behind read(): merges committed index state with the journal-tail overlay +// (horizon-clamped), reads every data-carrying LBA, verifies its checksum, and collapses any +// all-zero-content LBA to a hole extent -- this scan runs ONLY here, never on the write path. +async_result< craft::read_result > CraftReplDev::read_impl(int64_t read_lsn, uint64_t addr, uint64_t len, + sisl::sg_list dest, read_index_fn_t const& read_fn) { + // Same boundary validation as write(): nlbas=0 would underflow end_lba below (lba_t is unsigned). + // The upper bound also prevents nlbas itself from landing anywhere near UINT32_MAX (which would + // size sources/is_hole below to a multi-gigabyte allocation from one client-controlled len) -- + // read(), like write(), is reachable straight from the client wire, so len is untrusted input. + const uint64_t max_io_len = HB_DYNAMIC_CONFIG(craft_max_io_len_mb) * 1024ULL * 1024ULL; + if (len == 0 || len % lba_size_ != 0 || addr % lba_size_ != 0 || len > max_io_len) { + LOGW("read rejected: addr={} len={} not block-aligned (lba_size={}) or exceeds max {}", addr, len, lba_size_, + max_io_len); + co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); + } + // Unlike commit_lsn/all_committed_lsn on the write path, read_lsn has no "-1 means unset" + // convention -- it is always a horizon the caller must have a real value for. A negative value + // here would silently make every overlay entry's `it->second.lsn <= read_lsn` clamp check false + // (since no real lsn is negative), serving a read that looks committed-only rather than the + // explicit rejection a malformed request deserves. + if (read_lsn < 0) { + LOGW("read rejected: negative read_lsn={}", read_lsn); + co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); + } + lba_t const start_lba = static_cast< lba_t >(addr) / lba_size_; + uint32_t const nlbas = static_cast< uint32_t >(len / lba_size_); + // Defense-in-depth, mirroring commit_impl()'s identical guard: with len bounded above, nlbas can + // only be 0 here if len==0 (already rejected), but this stays as a hard backstop against any + // future change to the bound above reintroducing the underflow risk. + if (nlbas == 0) { + LOGE("read rejected: addr={} len={} produced nlbas=0 -- malformed request", addr, len); + co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); + } + lba_t const end_lba = start_lba + nlbas - 1; + + int64_t commit_lsn_snapshot; + { + std::lock_guard lk{missing_mu_}; + commit_lsn_snapshot = state_.commit_lsn; + } + + // Committed state from the index. + index_kv_list_t index_kvs; + if (auto r = read_fn(start_lba, end_lba, index_kvs); !r) co_return std::unexpected(r.error()); + std::unordered_map< lba_t, VolumeIndexValue > index_map; + index_map.reserve(index_kvs.size()); + for (auto const& [key, value] : index_kvs) + index_map.emplace(key.lba(), value); + + // Per-LBA source: hole (default), or a single-block data reference (blkid + csum) from whichever + // of overlay/index wins. Overlay wins over the index for the same LBA (it is strictly newer), but + // ONLY within the horizon -- an overlay entry above read_lsn is held but never served here; the + // index's value (committed as of commit_lsn <= read_lsn's caller-assumed frontier) is used instead. + struct Source { + bool hole{true}; + homestore::blk_id blkid{}; + homestore::csum_t csum{0}; + }; + std::vector< Source > sources(nlbas); + { + std::lock_guard lk{overlay_mu_}; + for (uint32_t i = 0; i < nlbas; ++i) { + lba_t const lba = start_lba + i; + auto it = overlay_.find(lba); + if (it != overlay_.end() && it->second.lsn > commit_lsn_snapshot && it->second.lsn <= read_lsn) { + if (!it->second.all_zeros) sources[i] = Source{false, it->second.blkid, it->second.csum}; + continue; // all_zeros overlay entry -> hole (Source's default) + } + if (auto idx_it = index_map.find(lba); idx_it != index_map.end()) + sources[i] = Source{false, idx_it->second.blkid(), idx_it->second.checksum()}; + // else: absent from both index and overlay -> hole (Source's default) + } + } + + // dest is a single flat iovec -- same assumption write()'s CRC computation already makes; every + // sg_list CRAFT builds anywhere in this codebase is a single iovec. + if (dest.iovs.empty()) { + LOGW("read rejected: dest sg_list has no iovecs (size={})", dest.size); + co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); + } + auto* dest_buf = static_cast< uint8_t* >(dest.iovs[0].iov_base); + std::vector< bool > is_hole(nlbas); + + for (uint32_t i = 0; i < nlbas;) { + if (sources[i].hole) { + is_hole[i] = true; + std::memset(dest_buf + i * lba_size_, 0, lba_size_); + ++i; + continue; + } + // Extend the contiguous run: same blk_num/chunk-progression merge volume.cpp's non-CRAFT + // read path uses (generate_blkids_to_read) -- batches one async_read per contiguous run + // instead of one per LBA. + uint32_t j = i + 1; + while (j < nlbas && !sources[j].hole && sources[j].blkid.blk_num() == sources[j - 1].blkid.blk_num() + 1 && + sources[j].blkid.chunk_num() == sources[j - 1].blkid.chunk_num()) { + ++j; + } + uint32_t const run_nlbas = j - i; + homestore::multi_blk_id const run_blkid{ + sources[i].blkid.blk_num(), static_cast< homestore::blk_count_t >(run_nlbas), sources[i].blkid.chunk_num()}; + sisl::sg_list run_sg; + run_sg.size = run_nlbas * lba_size_; + run_sg.iovs.push_back(iovec{dest_buf + i * lba_size_, run_sg.size}); + if (auto r = co_await journal_->read_data(run_blkid, run_sg); !r) co_return std::unexpected(r.error()); + + for (uint32_t k = i; k < j; ++k) { + uint8_t const* lba_buf = dest_buf + k * lba_size_; + bool const all_zero = std::all_of(lba_buf, lba_buf + lba_size_, [](uint8_t b) { return b == 0; }); + if (all_zero) { + // Read-time-only collapse: a data write whose payload happened to be all-zero bytes + // reads back as a hole. This scan must never run on the write path. + is_hole[k] = true; + continue; + } + auto const computed = crc16_t10dif(k_craft_crc16_seed, lba_buf, lba_size_); + if (computed != sources[k].csum) { + LOGE("read: crc mismatch lba={} expected={} actual={}", start_lba + k, sources[k].csum, computed); + co_return std::unexpected(make_error_condition(volume_error::CRC_MISMATCH)); + } + is_hole[k] = false; + } + i = j; + } + + // Merge adjacent same-type LBAs into extents, ascending by addr. + std::vector< craft::io_extent > extents; + for (uint32_t i = 0; i < nlbas;) { + uint32_t j = i + 1; + while (j < nlbas && is_hole[j] == is_hole[i]) + ++j; + extents.push_back( + craft::io_extent{(start_lba + i) * lba_size_, (j - i) * static_cast< uint64_t >(lba_size_), is_hole[i]}); + i = j; + } + + craft::lsn_pair snapshot; + { + std::lock_guard lk{missing_mu_}; + snapshot = {state_.commit_lsn, state_.last_append_lsn}; + } + co_return craft::read_result{std::move(extents), snapshot}; +} + +async_result< craft::lsn_pair > CraftReplDev::keep_alive(craft::client_hdr hdr) { + // Checked ahead of recovering_: a faulted restart recovery is permanent, not a "still starting up, + // try again shortly" condition -- see recovery_faulted_'s doc comment. + if (recovery_faulted_.load(std::memory_order_acquire)) { + LOGE("keep_alive rejected: partition permanently faulted after failed restart recovery"); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } + if (recovering_.load(std::memory_order_acquire)) { + LOGW("keep_alive rejected: overlay rebuild in progress after restart"); + co_return std::unexpected(make_error_condition(volume_error::OFFLINE)); + } + { + std::lock_guard lock{missing_mu_}; + if (hdr.term != state_.term) { + LOGW("keep_alive rejected: stale term want={} got={}", state_.term, hdr.term); + co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); + } + // Max-monotonic: never let a stale/reordered message regress the floor S8's eventual journal + // reclaim reads. The reclaim action itself is not implemented here -- see the doc comment. + state_.all_committed_lsn = std::max(state_.all_committed_lsn, hdr.all_committed_lsn); + } + + touch_watchdog(); + + // Unlike write()'s piggyback, advancing the frontier IS this call's entire purpose -- a genuine + // commit() fault (not a stall, which is never an error) propagates to the caller instead of being + // silently swallowed. + if (auto r = co_await commit(hdr.commit_lsn); !r) co_return std::unexpected(r.error()); + + std::lock_guard lock{missing_mu_}; + co_return craft::lsn_pair{state_.commit_lsn, state_.last_append_lsn}; +} + +// ─── client-liveness watchdog (S7) ─────────────────────────────────────────── + +void CraftReplDev::touch_watchdog() { + if (watchdog_timeout_ns_ == 0) return; + { + std::lock_guard lk{missing_mu_}; + // Not yet logged in -- no session to watch. NOTE this only prevents ARMING before the first + // login; it does not DISARM an already-armed timer once a session ends. logout() is still a + // stub today (see the "stubs" section) and never actually resets state_.term back to 0, so this + // is currently unreachable in practice -- but whoever implements real logout() should also stop + // the watchdog there (watchdog_token_.cancel() is cheap and idempotent), or every tick after a + // real logout will keep calling append() with a stale client_token until the object itself is + // destroyed (append() is still a stub too, so today this is inert, not harmful). + if (state_.term == 0) return; + } + // Record activity unconditionally (cheap atomic store) before the arm-once check below, so even + // the very first call -- which also arms the recurring timer -- leaves a fresh timestamp for that + // timer's first tick to read. + last_contact_ns_.store(std::chrono::steady_clock::now().time_since_epoch().count(), std::memory_order_relaxed); + + // Arm the recurring timer at most once, lazily, on the first successful write()/keep_alive() after + // login (state_.term != 0, checked above) -- every call after that just updates last_contact_ns_ + // above; there is nothing to cancel-and-reschedule anymore (see on_watchdog_tick()'s doc comment + // for why a RECURRING timer removes the need for that entirely). watchdog_token_ is only ever + // touched under watchdog_arm_mu_ here, or during single-object-owner destruction -- never both at + // once, by the same contract that already governs every other member (calling any method + // concurrently with the destructor is a lifetime violation regardless of the watchdog). + // + // reactor_regex::all_worker, not all_user: a RECURRING timer's underlying IODevice must actually + // attach to a reactor matching its scope (iomgr.cpp's schedule_global_timer dispatches all_worker + // to m_global_worker_timer, all_user to a SEPARATE m_global_user_timer) -- unlike a one-shot + // timer's reactor-agnostic heap entry. "User" reactors are optional (only exist if something + // explicitly calls iomanager.create_reactor()); the default worker pool iomanager.start() always + // creates is the only scope guaranteed to have at least one matching reactor to attach to. + // + // Ticks at HALF watchdog_timeout_ns_, not the full interval -- see on_watchdog_tick()'s doc comment + // for why: ticking at the same cadence as the staleness threshold doubles worst-case detection + // latency, which iomgr's own IOWatchDog avoids by keeping its tick interval and staleness threshold + // as two independently configured values (drive.io_watchdog_timer_sec vs drive.io_timeout_limit_sec + // in watchdog.cpp) -- this mirrors that separation with a single derived constant instead of a + // second constructor parameter, since nothing here needs the two independently tunable. + // + // Also unlike this codebase's OTHER recurring timers (homeblks_impl.cpp's shutdown/vol-gc timers, + // and HomeStore's CPManager/RaftReplService, which each first create a DEDICATED reactor via + // iomanager.create_reactor() for their timer rather than sharing the worker pool): this timer is + // per-CraftReplDev-instance (per partition), not a single process-wide background task, so it + // deliberately does NOT set up a dedicated reactor per instance. Known, accepted cost of that choice + // at high partition counts: each instance's recurring timerfd is registered on EVERY worker reactor + // (iomgr_timer.cpp's schedule() for recurring=true calls add_io_device with reactor_regex scope, + // which attaches to all matching reactors), so N active partitions across W worker reactors produce + // N*(W-1) redundant timerfd-read wakeups per tick (only the reactor whose timerfd read returns a + // nonzero count proceeds) and an O(N) iodev-registration cost whenever a new worker reactor starts + // (IOInterface::on_reactor_start's loop over every registered iodev, generic_interface.cpp). This is + // noise at the partition counts CRAFT runs at today; revisit (e.g. one shared dedicated reactor for + // all CraftReplDev watchdogs process-wide) if that ever changes. + std::lock_guard lk{watchdog_arm_mu_}; + if (watchdog_token_.active()) return; + watchdog_token_ = iomgr::schedule_recurring( + std::chrono::nanoseconds{watchdog_timeout_ns_ / 2}, iomgr::reactor_regex::all_worker, + [this]() { on_watchdog_tick(); }, /* wait_to_schedule = */ true); +} + +// Runs every watchdog_timeout_ns_/2 once armed (iomgr::timer_token's recurring timer, not a one-shot +// that reschedules itself) -- checks whether last_contact_ns_ is stale (elapsed >= the FULL +// watchdog_timeout_ns_, not the tick interval) and, if so, proposes a SyncRSCommitLSN entry via +// append(), fire-and-forget (detail::detach -- this runs in a plain timer-callback context, not a +// coroutine caller awaiting a result). If the client stays silent this fires on every tick once past +// the threshold, not just once, matching the old design's intent ("a permanently-silent client keeps +// getting append() attempts, not just one"). +// +// A recurring timer (vs the old one-shot-that-reschedules-itself) needs none of the generation +// counter / in-flight counter / shutting-down flag the previous design required: iomgr's recurring +// timer is backed by a real timerfd IODevice (iomgr_timer.cpp's timer_epoll::cancel dispatches a +// recurring handle to remove_io_device, NOT the one-shot heap-erase path that caused a reproduced +// SEGFAULT here earlier), and IOInterface::remove_io_device(wait=true) posts the removal to the same +// reactor thread that runs this callback and blocks until it completes -- verified against iomgr's +// actual epoll reactor source (reactor_epoll.cpp's listen()/remove_iodev_impl and their shared +// m_removed_iodevs set, which discards any already-queued event for an iodev removed within the same +// epoll batch) rather than assumed from "a reactor only does one thing at a time" reasoning alone. That +// is exactly the mutual-exclusion guarantee the destructor's watchdog_token_.cancel(true) call relies +// on -- see its own doc comment for the same detail. +// +// Trade-off, honestly noted: checking staleness once per tick (rather than the old design's precise +// "fires exactly timeout_ns after the last reset") means worst-case detection latency is bounded by +// timeout_ns + one tick interval -- with ticks at timeout_ns/2, that is ~1.5x timeout_ns in the worst +// case (activity right after a tick, then silence), not the full 2x a same-cadence tick/threshold would +// give. Not perfectly precise (that would need re-arming a one-shot on every reset, reintroducing the +// exact hazard this redesign removed), but bounded and cheap to keep tight by adjusting the tick +// divisor, not the threshold itself. +void CraftReplDev::on_watchdog_tick() { + auto const now_ns = std::chrono::steady_clock::now().time_since_epoch().count(); + auto const last_ns = last_contact_ns_.load(std::memory_order_relaxed); + if (now_ns - last_ns < static_cast< int64_t >(watchdog_timeout_ns_)) return; // recent activity -- not stale yet + +#ifdef _PRERELEASE + ++watchdog_fire_count_; +#endif + int64_t last_append_lsn; + uint64_t client_token; + { + std::lock_guard lk{missing_mu_}; + last_append_lsn = state_.last_append_lsn; + client_token = state_.client_token; + } + detail::detach(append(last_append_lsn, client_token)); } async_result< craft::resolution_result > CraftReplDev::request_resolution(craft::client_hdr /* hdr */, @@ -474,6 +1207,54 @@ async_result< std::vector< JournalSlot > > CraftReplDev::fetch_data(std::vector< co_return result; } +// ─── overlay rebuild on restart (S3) ───────────────────────────────────────── +// +// A fresh CraftReplDev's overlay_ starts empty -- nothing else repopulates the appended-but-not- +// yet-committed entries after a restart. Walks (commit_lsn, last_append_lsn], skipping missing/ +// Empty-verdicted lsns (nothing to read for those), and applies the exact same highest-dLSN-wins +// rule write()'s own post-flight update uses via the shared populate_overlay() helper. Missing lsns +// are skipped, not a stop condition (unlike commit_impl(), which must stall at the first gap since +// it advances a CONTIGUOUS prefix): a hole only means that ONE lsn's entry isn't locally present; +// later lsns in the same range can still hold real, out-of-order-appended entries that must not be +// skipped too. + +async_status CraftReplDev::rebuild_overlay() { + int64_t commit_lsn, last_append_lsn; + { + std::lock_guard lk{missing_mu_}; + commit_lsn = state_.commit_lsn; + last_append_lsn = state_.last_append_lsn; + } + + for (int64_t lsn = commit_lsn + 1; lsn <= last_append_lsn; ++lsn) { + bool is_missing, is_empty; + { + std::lock_guard lk{missing_mu_}; + is_missing = missing_lsns_.contains(lsn); + is_empty = empty_lsns_.contains(lsn); + } + if (is_missing || is_empty) continue; + + auto slot_r = co_await journal_->read_slot(lsn); + if (!slot_r) co_return std::unexpected(slot_r.error()); + auto& slot = *slot_r; + + lba_t const start_lba = static_cast< lba_t >(slot.lba_off_bytes) / lba_size_; + uint32_t const nlbas = static_cast< uint32_t >(slot.len_bytes / lba_size_); + // Matching commit_impl()'s identical guard: nlbas==0 produces start_lba + 0 - 1 underflow + // inside populate_overlay()'s any end_lba computation. A restart path should skip and continue + // (rather than abort the entire rebuild) since the replayed journal is not being mutated. + if (nlbas == 0) { + LOGE("rebuild_overlay: slot lsn={} has len_bytes={} (nlbas=0) -- malformed record, skipping", lsn, + slot.len_bytes); + continue; + } + populate_overlay(lsn, start_lba, nlbas, slot.all_zeros, slot.blkid, slot.csums); + } + + co_return ok(); +} + // ─── RAFT listener ──────────────────────────────────────────────────────────── void CraftReplDev::CraftRaftListener::on_commit(int64_t lsn, sisl::blob const& /* header */, @@ -485,6 +1266,54 @@ void CraftReplDev::CraftRaftListener::on_commit(int64_t lsn, sisl::blob const& / LOGD("CraftRaftListener::on_commit lsn={} (entry dispatch not yet implemented)", lsn); } +// Fired by HomeStore wherever this partition's repl_dev is recovered on restart. Fire-and-forget, +// same as on_watchdog_tick() -- this runs in a plain callback context, not a coroutine caller +// awaiting a result. Sets recovering_ BEFORE detaching the recovery coroutine so no write()/read()/ +// keep_alive() that arrives after on_restart() fires but before rebuild_overlay() completes can ever +// observe a partially-rebuilt overlay (write()/read()/keep_alive() all reject with OFFLINE while +// recovering_ is set -- per SDSTOR-22905, the overlay must be fully rebuilt before the volume accepts +// new client I/O). run_recovery() clears the flag once rebuild_overlay() actually succeeds -- see its +// own doc comment for what happens on failure instead. +// +// Reachable more than once, in principle, if HomeStore's repl_dev_listener contract ever allows +// on_restart() to fire again on the same instance -- there is no code-level guard against that here; +// correctness for that case currently rests entirely on HomeStore's calling convention (observed today: +// fired at most once per instance per restart cycle), not on anything this class enforces itself. +// +// Currently a no-op in practice: CraftPartitionState itself (state_) has no superblock-recovery path +// yet, so commit_lsn/last_append_lsn are still at their default -1/-1 when this fires, making +// rebuild_overlay()'s walk range empty and run_recovery() clear recovering_ almost immediately. This +// gate is still the correct wiring for once real superblock recovery populates state_ before this +// fires. +void CraftReplDev::CraftRaftListener::on_restart() { + owner_->recovering_.store(true, std::memory_order_release); + detail::detach(owner_->run_recovery()); +} + +// Awaits rebuild_overlay() to completion. On success, clears recovering_ -- the volume resumes +// accepting client I/O against a now-complete overlay. On FAILURE (e.g. a corrupt journal record), +// does the opposite of what a naive "always clear, log and move on" version would: leaves recovering_ +// set and additionally sets recovery_faulted_ (checked first, ahead of recovering_, by every client- +// facing entry point), so the partition permanently rejects I/O with INTERNAL_ERROR instead of quietly +// resuming against an overlay that stopped partway through -- entries at and above the failing LSN +// were never populated, so any subsequent read for an LBA only covered by one of those entries would +// otherwise silently fall through to stale, already-superseded index/committed state with no error at +// all. A stuck-rejecting partition is a visible, actionable failure; a partition silently serving stale +// data is not. There is deliberately no self-healing path out of this state here -- an operator +// decision (or a future re-sync-from-peer mechanism) is required. +async_status CraftReplDev::run_recovery() { + auto r = co_await rebuild_overlay(); + if (!r) { + LOGE("rebuild_overlay failed during restart recovery: {} -- partition permanently faulted, will " + "not accept client I/O (see recovery_faulted_'s doc comment)", + r.error().message()); + recovery_faulted_.store(true, std::memory_order_release); + co_return r; + } + recovering_.store(false, std::memory_order_release); + co_return r; +} + // ─── RAFT apply helpers (S5 implements) ────────────────────────────────────── void CraftReplDev::apply_sync_rs_commit_lsn(int64_t rs_commit_lsn, uint64_t /* client_token */, diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index ca8a54f..dd0bcb8 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -16,12 +16,17 @@ #include "../hb_internal.hpp" #include "craft_raft_entries.hpp" +#include "../volume/index_fixed_kv.hpp" // BlockInfo -- the shape commit()'s index write callback uses #include +#include // iomgr::timer_token -- RAII recurring timer used by the watchdog #include +#include +#include #include #include #include +#include #include namespace homestore { @@ -30,6 +35,8 @@ class home_log_store; namespace homeblocks { +class VolumeIndexTable; + // ─── CRAFT vocabulary vs. this backend's own state ─────────────────────────── // // The client-facing vocab (craft::client_hdr, craft::lsn_pair, craft::LoginResult, craft::read_result, @@ -45,10 +52,13 @@ namespace homeblocks { // Per-partition CRAFT state. Authoritative in memory; recovered from the journal + superblock on restart. struct CraftPartitionState { - int64_t commit_lsn{-1}; // contiguous committed prefix (== Synced) - int64_t last_append_lsn{-1}; // highest appended dLSN (may be uncommitted) - uint64_t client_token{0}; // token from the last successful InternalLogin - uint64_t term{0}; // current session term + int64_t commit_lsn{-1}; // contiguous committed prefix (== Synced) + int64_t last_append_lsn{-1}; // highest appended dLSN (may be uncommitted) + uint64_t client_token{0}; // token from the last successful InternalLogin + uint64_t term{0}; // current session term + int64_t all_committed_lsn{-1}; // client-computed set-wide min commit_lsn, piggybacked on keep_alive/write; + // floors journal reclaim (S8: truncate below min(this, checkpointed apply + // frontier)) -- S3 only captures it, the reclaim action itself is S8's job. }; // One journal slot returned by fetch_data() (server-to-server resync; never crosses the CLIENT wire). Four-way: @@ -61,6 +71,8 @@ struct JournalSlot { lba_t lba_off_bytes{0}; lba_count_t len_bytes{0}; sisl::sg_list data{}; + homestore::multi_blk_id blkid{}; // block reference (empty for all_zeros slots) + std::vector< homestore::csum_t > csums{}; // one per LBA in range; empty for all_zeros }; // ─── journal backend abstraction ───────────────────────────────────────────── @@ -76,22 +88,30 @@ class CraftJournalBackend { virtual async_result< homestore::multi_blk_id > alloc_write_data(sisl::sg_list const& data, lba_count_t len) = 0; // term is the session term captured from state_.term at write() time — stored in // CraftJournalEntry so recovery can skip stale-tail entries written under a deposed leader. + // csums is one crc16 per LBA in [lba, lba+len) (byte len / this device's lba_size), computed by + // the caller while the data is still in memory; empty for all_zeros (no data, nothing to sum). virtual 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) = 0; + homestore::multi_blk_id blkid, bool all_zeros, + std::vector< homestore::csum_t > const& csums) = 0; virtual async_result< JournalSlot > read_slot(int64_t lsn) = 0; // Drop all entries with seq_num > lsn; lsn becomes the new journal tail. virtual async_status truncate_to(int64_t lsn) = 0; // 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; + // Read the data payload referenced by blkid into dest (dest.size already set by the caller to + // blkid's byte length). Used by CraftReplDev::read() to fetch the bytes an index/overlay entry + // only stores a block reference for. Mockable so read()'s tests stay light (no real HomeStore). + virtual async_status read_data(homestore::multi_blk_id blkid, sisl::sg_list& dest) = 0; virtual ~CraftJournalBackend() = default; }; // Factory that wraps a HomeStore log store. Used by volume.cpp when creating a CRAFT-mode volume. // vol_ordinal must match vol_info_->ordinal so async_alloc_write routes to this volume's chunks. -// Tests inject MockCraftJournalBackend directly. +// lba_size is the volume's per-block byte size -- read_slot() needs it to derive nlbas from a +// slot's on-disk byte length. Tests inject MockCraftJournalBackend directly. unique< CraftJournalBackend > make_homestore_journal_backend(shared< homestore::home_log_store > logstore, - uint64_t vol_ordinal); + uint64_t vol_ordinal, uint32_t lba_size); // ─── CraftPeerFetcher ───────────────────────────────────────────────────────── // @@ -115,9 +135,31 @@ class CraftPeerFetcher { // index. Non-CRAFT volumes are unaffected. class CraftReplDev { +private: + // Index write/delete operation shapes commit_impl() (and its test seam, commit_with()) are + // parameterized over -- declared here, ahead of use, since a member function's declared parameter + // types (unlike default-argument expressions) are not part of the class's deferred "complete- + // class" lookup context and so must already be visible at the point of each declaration below. + using write_index_fn_t = std::function< status(lba_t, lba_t, std::unordered_map< lba_t, BlockInfo >&) >; + using delete_index_fn_t = std::function< status(lba_t, lba_t, std::vector< homestore::blk_id >&) >; + // Same shape as VolumeIndexTable::read_from_index -- read_impl() (and its test seam, read_with()) + // are parameterized over it for the same reason commit_impl()/commit_with() are. + using index_kv_list_t = std::vector< std::pair< VolumeIndexKey, VolumeIndexValue > >; + using read_index_fn_t = std::function< status(lba_t, lba_t, index_kv_list_t&) >; + public: - explicit CraftReplDev(volume_id_t vol_id, unique< CraftJournalBackend > journal); - ~CraftReplDev() = default; + // lba_size is the volume's per-block byte size, fixed for the volume's lifetime -- used to derive + // the per-write checksum-array length (len bytes / lba_size) on the write path. indx_tbl is the + // volume's own index table, applied to by commit(); nullptr in tests that only exercise the write + // path (commit()/overlay logic must no-op safely in that case). + // The watchdog timeout is read from HB_DYNAMIC_CONFIG(craft_watchdog_timeout_ms); 0 disables it. + // Tests override the config key in SetUp and restore it in TearDown. + explicit CraftReplDev(volume_id_t vol_id, unique< CraftJournalBackend > journal, uint32_t lba_size, + shared< VolumeIndexTable > indx_tbl); + // Cancels the watchdog's recurring timer (iomgr::timer_token::cancel(wait=true)), blocking until + // any in-flight tick has finished, so on_watchdog_tick() (which captures `this`) can never fire + // against a destroyed object. No-op if the watchdog was never armed. + ~CraftReplDev(); // ── client-facing ────────────────────────────────────────────────────── // @@ -139,12 +181,12 @@ class CraftReplDev { async_status logout(craft::client_hdr hdr); // Append data at the client-assigned dLSN. Zero-copy; does NOT apply to the LBA index (hdr.commit_lsn drives - // that). Set all_zeros=true for WRITE_ZEROES/unmap over [addr, addr+len); data must be empty in that case. - // Precondition: all_zeros=false requires non-empty data (data.size > 0). The ack returns the achieved + // that). Pass empty `data` for a WRITE_ZEROES/unmap over [addr, addr+len); pass non-empty `data` of exactly + // `len` bytes for a data write. The write kind is derived from data.empty(). The ack returns the achieved // {commit_lsn, last_append_lsn} snapshotted with the append -- every CRAFT IO response piggybacks the // watermarks (the wire's write_rsp), so any round-trip refreshes the client's model of this member. async_result< craft::lsn_pair > write(craft::client_hdr hdr, int64_t dlsn, uint64_t addr, uint64_t len, - sisl::sg_list data, bool all_zeros = false); + sisl::sg_list data); // read_lsn is the horizon H: serve the latest version <= H for [addr, addr+len), from the LBA index if applied // or from the journal-tail overlay if only Appended (no index write on the read path). Never fetches from a @@ -155,8 +197,10 @@ class CraftReplDev { // Advance the frontier toward hdr.commit_lsn + reset the client-liveness watchdog -- which is WHY it is // term-fenced: a deposed client must not be able to keep its session alive. hdr.all_committed_lsn is the - // client-computed set-wide min commit_lsn; the journal may be reclaimed below - // min(all_committed_lsn, checkpointed apply frontier). Returns the achieved watermarks. + // client-computed set-wide min commit_lsn; captured into state_ (max-monotonic, never regresses on a + // stale/reordered message) so a floor of min(all_committed_lsn, checkpointed apply frontier) is available + // for S8's journal reclaim -- the reclaim ACTION itself is S8's job, not implemented here. Returns the + // achieved watermarks. async_result< craft::lsn_pair > keep_alive(craft::client_hdr hdr); // The client-requested resolution round (the wire's RESOLVE; the design's @@ -184,8 +228,10 @@ class CraftReplDev { // unused until S9 needs them (matches craft_client's own reference implementation today). async_result< craft::lsn_pair > get_rs_commit_lsn(uint64_t term, bool is_login); - // 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. + // Drop all journal entries with dLSN > lsn; clear missing-set entries above lsn; clamp last_append_lsn; + // prune any overlay entry whose recorded lsn > lsn (it referenced a now-rolled-back write -- leaving it + // would let a later read serve stale data from a write that no longer exists in the journal). Called + // only during login (quiesced -- no concurrent writes). commit_lsn is NOT changed. async_status truncate(int64_t lsn); // Propose a SyncRSCommitLSN RAFT entry (called by watchdog or leader during login). @@ -195,6 +241,16 @@ class CraftReplDev { // JournalSlot{.is_empty=true} rather than an error. async_result< std::vector< JournalSlot > > fetch_data(std::vector< int64_t > lsns); + // Reconstructs the journal-tail overlay from the journal after a restart (a fresh instance's + // overlay_ starts empty -- nothing else repopulates it). Walks (commit_lsn, last_append_lsn], + // skipping missing/Empty-verdicted lsns, and applies the same highest-dLSN-wins rule write()'s + // own post-flight overlay population uses. Called (via run_recovery(), which also gates client I/O + // for the duration -- see recovering_) from CraftRaftListener::on_restart(); currently a no-op in + // practice since CraftPartitionState itself has no superblock-recovery path yet (state_ stays at + // its default -1/-1 until that separate piece of work lands) -- this is still the correct, + // already-usable wiring for once it does. + async_status rebuild_overlay(); + // ── observability ───────────────────────────────────────────────────── size_t missing_count() const { @@ -220,6 +276,13 @@ class CraftReplDev { std::lock_guard lk{missing_mu_}; return state_.commit_lsn; } + // The last all_committed_lsn captured from a client's keep_alive/write -- floors journal reclaim + // (S8's job, not read anywhere yet in this class); exposed so S8's eventual reclaim logic has + // something to read. + int64_t all_committed_lsn() const { + std::lock_guard lk{missing_mu_}; + return state_.all_committed_lsn; + } // 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. @@ -236,6 +299,45 @@ 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); + + // Test seam for commit(): runs the same apply-one-slot algorithm, but the index write/delete + // operations are injected rather than routed through indx_tbl_, so tests can exercise commit()'s + // logic (in-order apply, stall at a gap, Empty-slot skip, overlay retirement) against a fake + // index instead of a real VolumeIndexTable. + async_result< int64_t > commit_with(int64_t upto_lsn, write_index_fn_t write_fn, delete_index_fn_t delete_fn); + + // Test seam for read(): runs the same read algorithm, but the index read operation is injected + // rather than routed through indx_tbl_, so tests can exercise read()'s logic (index/overlay merge, + // horizon clamp, checksum verification, read-time all-zero collapse) against a fake index instead + // of a real VolumeIndexTable. + async_result< craft::read_result > read_with(int64_t read_lsn, uint64_t addr, uint64_t len, sisl::sg_list dest, + read_index_fn_t read_fn); + + // Test-only observability for the watchdog: append() is still a stub with no other observable + // side effect, so this is how a test confirms on_watchdog_tick() actually fired. + int watchdog_fire_count() const { return watchdog_fire_count_; } + + // Returns the recorded lsn for lba's overlay entry, or -1 if no overlay entry exists for lba. + // Test-only observability for overlay retirement / highest-dLSN-wins correctness. + int64_t overlay_lsn_for(lba_t lba) const { + std::lock_guard lk{overlay_mu_}; + auto it = overlay_.find(lba); + return it == overlay_.end() ? -1 : it->second.lsn; + } + + // Test seam that exercises the EXACT production restart path (raft_listener_ is otherwise private + // and only ever invoked by HomeStore itself), so a test can verify write()/read()/keep_alive() all + // reject with OFFLINE while recovering_ is set, and succeed again once rebuild_overlay() completes. + void trigger_on_restart() { raft_listener_.on_restart(); } + + // Test-only observability for the SDSTOR-22905 restart-recovery gate (see recovering_'s doc + // comment): true from the moment on_restart() fires until run_recovery()'s rebuild_overlay() call + // completes successfully (see is_recovery_faulted() for the failure outcome instead). + bool is_recovering() const { return recovering_.load(std::memory_order_acquire); } + + // Test-only observability for recovery_faulted_: true if a restart's rebuild_overlay() ever failed, + // permanently. Never transitions back to false. + bool is_recovery_faulted() const { return recovery_faulted_.load(std::memory_order_acquire); } #endif private: @@ -274,7 +376,9 @@ class CraftReplDev { void on_remove_member(const homestore::replica_id_t&, homestore::trace_id_t) override {} void on_rollback(int64_t, const sisl::blob&, const sisl::blob&, cintrusive< homestore::repl_req_ctx >&) override {} - void on_restart() override {} + // Fires rebuild_overlay() fire-and-forget (defined out-of-line in the .cpp -- needs + // coro_helpers.hpp's detail::detach, which this header does not include). + void on_restart() override; homestore::async_status create_snapshot(std::shared_ptr< homestore::snapshot_context >) override { co_return homestore::ok(); } @@ -299,17 +403,159 @@ class CraftReplDev { void 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); + // Advance commit_lsn toward upto_lsn by applying each committable slot to the index (internal + // helper; never a wire op -- there is no standalone commit verb on the CRAFT wire). Stalls + // (returns without reaching upto_lsn) at the first gap in missing_lsns_ -- not an error, just the + // achieved commit_lsn. At most one run is ever active at a time (commit_running_); a concurrent + // caller is a safe no-op since the in-flight run covers the same ground and every subsequent + // write()/keep_alive() retries the advance. Callers: write()'s post-flight piggyback (best-effort, + // every outcome including real errors is ignored) and keep_alive() (propagates real errors, since + // advancing the frontier is its entire purpose -- a stall is still not an error there either). + async_result< int64_t > commit(int64_t upto_lsn); + + // Core apply-one-slot algorithm behind commit(), parameterized by the index write/delete + // operations (write_index_fn_t / delete_index_fn_t, declared at the top of this class) so tests + // can exercise it against a fake index instead of a real VolumeIndexTable. commit() binds these + // to indx_tbl_'s real methods; commit_with() (test-only) binds test doubles. + async_result< int64_t > commit_impl(int64_t upto_lsn, write_index_fn_t const& write_fn, + delete_index_fn_t const& delete_fn); + + // Core algorithm behind read(), parameterized by the index read operation (read_index_fn_t, + // declared at the top of this class) so tests can exercise it against a fake index instead of a + // real VolumeIndexTable. read() binds this to indx_tbl_'s real read_from_index; read_with() + // (test-only) binds a test double. Merges the index's committed state with the journal-tail + // overlay (horizon-clamped to (commit_lsn, read_lsn]), reads every data-carrying LBA via + // journal_->read_data(), verifies each LBA's checksum, and collapses any all-zero-content LBA to + // a hole extent at read time (never at write time). + async_result< craft::read_result > read_impl(int64_t read_lsn, uint64_t addr, uint64_t len, sisl::sg_list dest, + read_index_fn_t const& read_fn); + + // Core overlay-population rule shared by write()'s post-flight update and rebuild_overlay(): + // highest-dLSN-wins per LBA -- an entry only replaces whatever's already at that LBA if dlsn is + // strictly greater than the recorded one. all_zeros populates the all_zeros marker (no blkid); + // otherwise blkid is decomposed via multi_blk_id::iterate() into one single-block OverlayEntry + // per LBA, paired with its csums[] entry. + void populate_overlay(int64_t dlsn, lba_t start_lba, uint32_t nlbas, bool all_zeros, + homestore::multi_blk_id const& blkid, std::vector< homestore::csum_t > const& csums); + + // Records write()/keep_alive() activity and, on the first call after login, arms the watchdog's + // recurring iomgr timer (iomgr::timer_token) that periodically checks for staleness -- see + // on_watchdog_tick()'s doc comment for the full design and why a RECURRING timer (as opposed to + // the old one-shot-that-reschedules-itself pattern) needs no generation counter, in-flight + // counter, or shutting-down flag. No-ops if the watchdog is disabled + // (craft_watchdog_timeout_ms == 0 at construction) or before the first successful login (state_.term == 0). + void touch_watchdog(); + + // Runs on every tick of the watchdog's recurring timer (once armed by touch_watchdog()): if + // last_contact_ns_ is older than watchdog_timeout_ns_, proposes a SyncRSCommitLSN entry via + // append(), fire-and-forget (detail::detach -- this runs in a plain timer-callback context, not a + // coroutine caller awaiting a result). See the .cpp for the full safety argument (iomgr's + // recurring-timer cancellation path, not the one-shot heap-erase path that caused a reproduced + // SEGFAULT under the previous design). + void on_watchdog_tick(); + + // Awaits rebuild_overlay() to completion. On SUCCESS, clears recovering_ so the volume resumes + // accepting client I/O against a now-complete overlay. On FAILURE, does NOT clear recovering_ -- + // instead sets recovery_faulted_ (checked ahead of recovering_ by every client-facing entry point), + // permanently rejecting I/O rather than resuming against a silently incomplete overlay. This is the + // coroutine CraftRaftListener::on_restart() detaches, rather than detaching rebuild_overlay() + // directly, specifically so this outcome-dependent branching has somewhere to live. + async_status run_recovery(); + volume_id_t vol_id_; unique< CraftJournalBackend > journal_; + uint32_t lba_size_; + shared< VolumeIndexTable > indx_tbl_; CraftPartitionState state_; 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) - mutable std::mutex missing_mu_; // guards state_, missing_lsns_, and empty_lsns_ + bool commit_running_{false}; // at most one commit_impl() run active at a time -- see commit()'s doc comment + // dlsns currently between "claimed as non-idempotent" and "write_slot has completed" in write() -- + // see write()'s doc comment at the in_flight_write_dlsns_.contains() check for why this exists. + std::set< int64_t > in_flight_write_dlsns_; + mutable std::mutex + missing_mu_; // guards state_, missing_lsns_, empty_lsns_, commit_running_, in_flight_write_dlsns_ + + // One highest-dLSN-unapplied entry per LBA in (commit_lsn, last_append_lsn]: makes an appended- + // but-not-yet-committed write locally readable ahead of commit() applying it to the index. + // + // Size is bounded by the number of DISTINCT LBAs written since commit_lsn (highest-dLSN-wins + // collapses repeats to one entry each), not by write/dLSN count -- so its worst case is the + // volume's entire LBA space, reached only if commit() never advances (a permanently missing + // journal hole, or a client that stops calling keep_alive()/write() entirely). There is + // deliberately no eviction/cap here: capping would mean either serving stale index data for an + // evicted LBA that is genuinely only correct in the overlay, or rejecting new writes outright -- + // both are new backpressure semantics outside S3's scope. In practice this is kept in check by + // the watchdog forcing periodic commit() progress via SyncRSCommitLSN proposals (S5) whenever a + // client goes quiet, and by the client's own commit_lsn piggyback on every write/keep_alive. + struct OverlayEntry { + int64_t lsn{-1}; + homestore::blk_id blkid{}; + homestore::csum_t csum{0}; + bool all_zeros{false}; + }; + std::unordered_map< lba_t, OverlayEntry > overlay_; + mutable std::mutex overlay_mu_; // lock order: missing_mu_ before overlay_mu_ + bool login_in_progress_{false}; std::mutex login_mu_; CraftRaftListener raft_listener_; CraftPeerFetcher* peer_fetcher_{nullptr}; // null until S9 wires CraftConnector std::atomic< uint64_t > write_counter_{0}; // incremented per write(); triggers periodic SyncRSCommitLSN append + + // Set by CraftRaftListener::on_restart() before detaching run_recovery(), cleared by run_recovery() + // ONLY on a successful rebuild_overlay() (see recovery_faulted_ below for the failure path) -- + // write()/read()/keep_alive() all reject with volume_error::OFFLINE while this is set, per + // SDSTOR-22905: the overlay must be fully rebuilt before the volume accepts new client I/O. + // + // Two caveats, both accepted rather than engineered around (see below for why): + // - This is a load-and-go gate, not a lock held for a call's full duration: a write()/read()/ + // keep_alive() that loads recovering_==false can still be suspended (at a co_await point) and + // resume concurrently with a THEN-started rebuild_overlay() populating overlay_. Safe from data + // races (overlay_mu_ still serializes actual access) but not from serving a value rebuild_overlay() + // was about to correct. This is bounded entirely by HomeStore's restart contract: on_restart() + // fires before any client can be connected, so in production no live write()/read()/keep_alive() + // call can be in flight when it fires. Held to be an acceptable, explicitly documented limitation + // rather than adding suspension-point-granularity re-checks for a window that cannot occur under + // that contract; revisit if HomeStore's on_restart() timing guarantee ever changes. + // - login() (still a stub -- see the "stubs" section) is NOT gated by this flag either. Once login() + // is real, a concurrent login()->truncate() during recovery could destroy journal entries + // rebuild_overlay() is still walking. Whoever implements real login() must also gate it here. + // + // A plain atomic bool suffices for the single-writer part: on_restart()/run_recovery() are the only + // writers, and HomeStore's OWN calling contract (not a runtime property of this class) is what + // ensures they run as one complete cycle before any next one -- see run_recovery()'s comment. + std::atomic< bool > recovering_{false}; + // Set (never cleared) by run_recovery() if rebuild_overlay() fails -- a failed rebuild leaves + // overlay_ silently INCOMPLETE (some LSNs applied, then it bailed), so resuming client I/O against + // it (as recovering_ alone clearing would do) could serve stale pre-write data with no error at + // all. write()/read()/keep_alive() check this FIRST, ahead of recovering_, and reject with + // volume_error::INTERNAL_ERROR permanently -- an honest, fail-closed fault rather than fail-open + // data corruption. There is deliberately no self-recovery path: an operator (or a future re-sync- + // from-peer mechanism) must intervene. + std::atomic< bool > recovery_faulted_{false}; + + uint64_t watchdog_timeout_ns_{0}; // ns; 0 = disabled; set from craft_watchdog_timeout_ms config at construction + // Updated (relaxed store) on every touch_watchdog() call; read (relaxed load) by on_watchdog_tick() + // to decide staleness. A plain atomic timestamp -- steady_clock::now().time_since_epoch().count() + // -- rather than std::atomic, since int64_t is unambiguously lock-free + // and trivially comparable, with no reliance on time_point's own atomicity properties. + std::atomic< int64_t > last_contact_ns_{0}; + // Guards the arm-once transition of watchdog_token_ only (touch_watchdog() takes this every call, + // but only ever WRITES watchdog_token_ the first time, under the double-checked active() test). + // watchdog_token_ is otherwise touched only by the destructor -- never concurrently with + // touch_watchdog(), by the same lifetime contract that already governs every other member + // (calling any method concurrently with the destructor is a caller lifetime violation regardless + // of the watchdog). + std::mutex watchdog_arm_mu_; + // RAII recurring timer: armed once by touch_watchdog(), cancelled (wait=true) by the destructor. + // Replaces the old watchdog_hdl_/watchdog_mu_/watchdog_generation_/watchdog_in_flight_/ + // watchdog_shutting_down_ machinery entirely -- see touch_watchdog()/on_watchdog_tick()'s doc + // comments and the .cpp for why a RECURRING iomgr timer needs none of that. + iomgr::timer_token watchdog_token_; +#ifdef _PRERELEASE + std::atomic< int > watchdog_fire_count_{0}; // test-only; never present in production binaries +#endif }; } // namespace homeblocks diff --git a/src/lib/craft/tests/CMakeLists.txt b/src/lib/craft/tests/CMakeLists.txt index 43b293a..f86ad79 100644 --- a/src/lib/craft/tests/CMakeLists.txt +++ b/src/lib/craft/tests/CMakeLists.txt @@ -1,5 +1,13 @@ cmake_minimum_required(VERSION 3.11) +# Light CRAFT tests include home_blks_config.hpp, which instantiates home_blks_config_factory +# whose constructor references home_blks_config_fbs[] -- the schema binary generated by +# settings_gen_cpp(... homeblocks_core ...) in src/lib/CMakeLists.txt. That generated object is +# only compiled into homeblocks_core / homeblocks; light tests don't link either. Providing the +# generated source directly to each light test binary is the minimal fix. +set(HB_CONFIG_BINDUMP ${CMAKE_BINARY_DIR}/src/lib/generated/home_blks_config_bindump.cpp) +set_source_files_properties(${HB_CONFIG_BINDUMP} PROPERTIES GENERATED TRUE) + # Unit test for CraftReplDev::truncate (S4). Compiles craft_repl_dev.cpp directly to avoid # dragging in craft_api.cpp → volume.hpp (heavy HomeStore volume plumbing not needed here). add_executable(test_craft_truncate) @@ -8,13 +16,35 @@ target_sources(test_craft_truncate PRIVATE ../craft_repl_dev.cpp ) target_compile_definitions(test_craft_truncate PRIVATE _PRERELEASE) +target_sources(test_craft_truncate PRIVATE ${HB_CONFIG_BINDUMP}) target_link_libraries(test_craft_truncate ${COMMON_TEST_DEPS} -rdynamic ) +add_dependencies(test_craft_truncate ${PROJECT_NAME}_core) add_test(NAME CraftTruncateTest COMMAND test_craft_truncate) +# Real multi-threaded tests for CraftReplDev's own internal locking (missing_mu_, overlay_mu_, +# commit_running_). Same pattern as test_craft_truncate (compile craft_repl_dev.cpp directly, no +# HomeStore/iomgr) -- MockCraftJournalBackend's coroutine bodies never suspend across a real async +# boundary, so real std::thread callers drive genuinely concurrent CraftReplDev execution with no +# reactor needed. +add_executable(test_craft_concurrency) +target_sources(test_craft_concurrency PRIVATE + test_craft_concurrency.cpp + ../craft_repl_dev.cpp +) +target_compile_definitions(test_craft_concurrency PRIVATE _PRERELEASE) +target_sources(test_craft_concurrency PRIVATE ${HB_CONFIG_BINDUMP}) +target_link_libraries(test_craft_concurrency + ${COMMON_TEST_DEPS} + -rdynamic +) +add_dependencies(test_craft_concurrency ${PROJECT_NAME}_core) + +add_test(NAME CraftConcurrencyTest COMMAND test_craft_concurrency) + # Unit tests for CraftReplDev::get_lsns(), 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) @@ -23,10 +53,12 @@ target_sources(test_craft_peer_exchange PRIVATE ../craft_repl_dev.cpp ) target_compile_definitions(test_craft_peer_exchange PRIVATE _PRERELEASE) +target_sources(test_craft_peer_exchange PRIVATE ${HB_CONFIG_BINDUMP}) target_link_libraries(test_craft_peer_exchange ${COMMON_TEST_DEPS} -rdynamic ) +add_dependencies(test_craft_peer_exchange ${PROJECT_NAME}_core) add_test(NAME CraftPeerExchangeTest COMMAND test_craft_peer_exchange) @@ -38,13 +70,51 @@ target_sources(test_craft_write PRIVATE ../craft_repl_dev.cpp ) target_compile_definitions(test_craft_write PRIVATE _PRERELEASE) +target_sources(test_craft_write PRIVATE ${HB_CONFIG_BINDUMP}) target_link_libraries(test_craft_write ${COMMON_TEST_DEPS} -rdynamic ) +add_dependencies(test_craft_write ${PROJECT_NAME}_core) add_test(NAME CraftWriteTest COMMAND test_craft_write) +# Unit tests for CraftReplDev::commit() (S3: Commit Path), against a fake std::map-backed index. +# Same pattern as test_craft_truncate: compile craft_repl_dev.cpp directly, no HomeStore bring-up. +add_executable(test_craft_commit) +target_sources(test_craft_commit PRIVATE + test_craft_commit.cpp + ../craft_repl_dev.cpp +) +target_compile_definitions(test_craft_commit PRIVATE _PRERELEASE) +target_sources(test_craft_commit PRIVATE ${HB_CONFIG_BINDUMP}) +target_link_libraries(test_craft_commit + ${COMMON_TEST_DEPS} + -rdynamic +) +add_dependencies(test_craft_commit ${PROJECT_NAME}_core) + +add_test(NAME CraftCommitTest COMMAND test_craft_commit) + +# Unit tests for CraftReplDev's client-liveness watchdog (S7). Same pattern as test_craft_truncate +# (compile craft_repl_dev.cpp directly, no HomeStore bring-up), but this binary's main() starts a +# minimal, HomeStore-free iomgr instance -- needed because scheduling a real timer requires a +# running reactor pool, unlike every other test in this light suite. +add_executable(test_craft_watchdog) +target_sources(test_craft_watchdog PRIVATE + test_craft_watchdog.cpp + ../craft_repl_dev.cpp +) +target_compile_definitions(test_craft_watchdog PRIVATE _PRERELEASE) +target_sources(test_craft_watchdog PRIVATE ${HB_CONFIG_BINDUMP}) +target_link_libraries(test_craft_watchdog + ${COMMON_TEST_DEPS} + -rdynamic +) +add_dependencies(test_craft_watchdog ${PROJECT_NAME}_core) + +add_test(NAME CraftWatchdogTest COMMAND test_craft_watchdog) + # Exercises HomeStoreCraftJournalBackend against a REAL HomeStore home_log_store -- the # production backend was previously never executed by any test. Links the full homeblocks # library, unlike the tests above, because a real home_log_store requires a running HomeStore @@ -80,3 +150,21 @@ target_link_libraries(test_craft_journal_slot_wire ) add_test(NAME CraftJournalSlotWireTest COMMAND test_craft_journal_slot_wire) + +# Heavy integration test for CraftReplDev::commit()/read() against a REAL VolumeIndexTable and real +# data blocks (via a real, ordinarily-created volume) -- same pattern as +# test_craft_homestore_backend.cpp (links the full library, real HomeStore bring-up). +add_executable(test_craft_commit_hs) +target_sources(test_craft_commit_hs PRIVATE + test_craft_commit_hs.cpp +) +target_include_directories(test_craft_commit_hs PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/../../volume/tests +) +target_link_libraries(test_craft_commit_hs + ${PROJECT_NAME} + ${COMMON_TEST_DEPS} + -rdynamic +) + +add_test(NAME CraftCommitHsTest COMMAND test_craft_commit_hs --index_chunk_size_mb=128 --data_chunk_size_mb=128) diff --git a/src/lib/craft/tests/test_craft_commit.cpp b/src/lib/craft/tests/test_craft_commit.cpp new file mode 100644 index 0000000..1fa3dba --- /dev/null +++ b/src/lib/craft/tests/test_craft_commit.cpp @@ -0,0 +1,1232 @@ +/********************************************************************************* + * 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::commit() (S3: Commit Path). +// +// Exercises commit_with() -- the test seam behind commit() -- against a fake, std::map-backed +// index instead of a real VolumeIndexTable, so this stays a light test (no HomeStore bring-up). +// commit_with() runs the exact same apply-one-slot algorithm (commit_impl) that commit() binds to +// indx_tbl_'s real methods; only the write/delete callbacks differ. +// +// 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 +#include +#include +#include +#include + +#include "craft/craft_repl_dev.hpp" +#include "home_blks_config.hpp" +#include "coro_helpers.hpp" + +SISL_LOGGING_DEF(HOMEBLOCKS_LOG_MODS) +SISL_OPTIONS_ENABLE(logging) +SISL_LOGGING_INIT(HOMEBLOCKS_LOG_MODS) + +namespace homeblocks { +namespace { + +static constexpr uint32_t k_page_size = 4096; +// Must match k_craft_crc16_seed in craft_repl_dev.cpp -- same constant test_craft_write.cpp uses. +static constexpr homestore::csum_t k_test_crc16_seed = 0x8005; + +// ── journal mock ────────────────────────────────────────────────────────────── +// +// A capable mock (unlike the stub ones in test_craft_truncate.cpp/test_craft_peer_exchange.cpp): +// write_slot actually records slots so real dev_->write() calls can be used to seed the overlay, +// and alloc_write_data returns a real, decomposable multi_blk_id sized to the request. + +class MockCraftJournalBackend : public CraftJournalBackend { +public: + std::map< int64_t, JournalSlot > slots; + int free_data_calls{0}; + homestore::blk_num_t next_blk_num{1000}; + std::optional< int64_t > fail_on_read; // if set, read_slot for that lsn returns an error + // Opt-in blocking gate for read_slot(), default open (no blocking) -- lets a test create a real, + // observable window during rebuild_overlay()'s walk (e.g. to verify recovering_'s client-I/O gate + // actually holds while a restart-triggered rebuild is still in flight), since this mock's other + // methods all resolve synchronously with no real suspension to race against otherwise. + std::mutex read_gate_mu; + std::condition_variable read_gate_cv; + bool read_gate_open{true}; + void close_read_gate() { + std::lock_guard lk{read_gate_mu}; + read_gate_open = false; + } + void open_read_gate() { + { + std::lock_guard lk{read_gate_mu}; + read_gate_open = true; + } + read_gate_cv.notify_all(); + } + // Bytes backing each single-block blk_num -- populated automatically by alloc_write_data (so any + // real write() call is readable back via read_data with no separate seeding step) and directly by + // read()-test cases that construct index/overlay state without going through write(). + std::map< homestore::blk_num_t, std::vector< uint8_t > > block_data; + + async_result< homestore::multi_blk_id > alloc_write_data(sisl::sg_list const& data, lba_count_t len) override { + auto nlbas = static_cast< homestore::blk_count_t >(len / k_page_size); + homestore::multi_blk_id blkid{next_blk_num, nlbas, /* chunk = */ 1}; + auto const* buf = static_cast< uint8_t const* >(data.iovs[0].iov_base); + for (homestore::blk_count_t i = 0; i < nlbas; ++i) + block_data[next_blk_num + i] = std::vector< uint8_t >(buf + i * k_page_size, buf + (i + 1) * k_page_size); + next_blk_num += nlbas; + co_return blkid; + } + + 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, std::vector< homestore::csum_t > const& csums) override { + slots[lsn] = JournalSlot{ + .lsn = lsn, .all_zeros = all_zeros, .lba_off_bytes = lba, .len_bytes = len, .blkid = blkid, .csums = csums}; + co_return ok(); + } + + async_result< JournalSlot > read_slot(int64_t lsn) override { + { + // Plain (non-coroutine) blocking wait: this mock's coroutines never suspend across a real + // async boundary, so whatever thread calls into read_slot() (directly, or transitively via + // rebuild_overlay()'s detached run_recovery()) physically blocks here until released -- + // exactly what a test needs to create an observable in-progress-recovery window. + std::unique_lock lk{read_gate_mu}; + read_gate_cv.wait(lk, [this] { return read_gate_open; }); + } + 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; + } + + async_status truncate_to(int64_t) override { co_return ok(); } + async_status free_data(homestore::multi_blk_id) override { + ++free_data_calls; + co_return ok(); + } + // Same error category HomeStoreCraftJournalBackend::read_data returns for a real async_read + // failure -- a missing block_data entry here means the test forgot to seed it, not a + // filesystem-shaped condition, so this mirrors production's actual error vocabulary rather than + // borrowing an unrelated POSIX errno. + async_status read_data(homestore::multi_blk_id blkid, sisl::sg_list& dest) override { + auto* buf = static_cast< uint8_t* >(dest.iovs[0].iov_base); + size_t offset = 0; + auto pieces = blkid.iterate(); + while (auto piece = pieces.next()) { + for (homestore::blk_count_t i = 0; i < piece->blk_count(); ++i) { + auto it = block_data.find(piece->blk_num() + i); + if (it == block_data.end()) + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + std::memcpy(buf + offset, it->second.data(), it->second.size()); + offset += it->second.size(); + } + } + co_return ok(); + } + + // Seed a data slot directly (bypassing write()) for tests that need precise control over + // missing_lsns_/last_append_lsn without exercising write()'s own logic. + void add_data_slot(int64_t lsn, lba_t lba, uint32_t nlbas, homestore::blk_num_t blk_num, + std::vector< homestore::csum_t > csums) { + homestore::multi_blk_id blkid{blk_num, static_cast< homestore::blk_count_t >(nlbas), /* chunk = */ 1}; + slots[lsn] = JournalSlot{.lsn = lsn, + .lba_off_bytes = lba * k_page_size, + .len_bytes = nlbas * k_page_size, + .blkid = blkid, + .csums = std::move(csums)}; + } + + // Seed block_data directly for read()-test cases that construct index/overlay state without a + // real write() call (e.g. index entries built straight against FakeIndex). + void seed_block(homestore::blk_num_t blk_num, std::vector< uint8_t > data) { + block_data[blk_num] = std::move(data); + } +}; + +// ── fake index ──────────────────────────────────────────────────────────────── +// +// Backed by a plain std::map, matching the shape commit_impl's write/delete callbacks expect -- +// the same two operations a real VolumeIndexTable exposes (write_to_index, delete_lba_range). + +class FakeIndex { +public: + std::map< lba_t, BlockInfo > entries; + + status write_to_index(lba_t start_lba, lba_t end_lba, std::unordered_map< lba_t, BlockInfo >& blocks_info) { + for (auto lba = start_lba; lba <= end_lba; ++lba) { + auto& info = blocks_info[lba]; + if (auto it = entries.find(lba); it != entries.end()) info.old_blkid = it->second.new_blkid; + entries[lba] = BlockInfo{info.new_blkid, homestore::blk_id{}, info.new_checksum}; + } + return ok(); + } + + status delete_lba_range(lba_t start_lba, lba_t end_lba, std::vector< homestore::blk_id >& out_freed_blkids) { + for (auto lba = start_lba; lba <= end_lba; ++lba) { + auto it = entries.find(lba); + if (it == entries.end()) continue; + out_freed_blkids.push_back(it->second.new_blkid); + entries.erase(it); + } + return ok(); + } + + // Same shape as the real VolumeIndexTable::read_from_index: absent LBAs are simply omitted + // (holes), not an error. + status read_from_index(lba_t start_lba, lba_t end_lba, + std::vector< std::pair< VolumeIndexKey, VolumeIndexValue > >& out) { + for (auto lba = start_lba; lba <= end_lba; ++lba) { + auto it = entries.find(lba); + if (it == entries.end()) continue; + out.emplace_back(VolumeIndexKey{lba}, VolumeIndexValue{it->second.new_blkid, it->second.new_checksum}); + } + return ok(); + } +}; + +// ── test fixture ───────────────────────────────────────────────────────────── + +class CraftCommitTest : 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), k_page_size, nullptr); + } + + auto do_commit(int64_t upto_lsn) { + return homeblocks::detail::sync_get(dev_->commit_with( + upto_lsn, + [this](lba_t s, lba_t e, std::unordered_map< lba_t, BlockInfo >& info) { + return index_.write_to_index(s, e, info); + }, + [this](lba_t s, lba_t e, std::vector< homestore::blk_id >& freed) { + return index_.delete_lba_range(s, e, freed); + })); + } + + // Real write() call -- the only way to populate the overlay (write()'s post-flight block does + // it unconditionally, regardless of indx_tbl_). Backed by a shared static buffer since write() + // computes a per-LBA CRC over it. + auto do_write_data(uint64_t term, int64_t dlsn, lba_t lba, uint32_t nlbas) { + static std::vector< uint8_t > buf(16 * k_page_size, 0xAB); + sisl::sg_list data; + data.size = nlbas * k_page_size; + data.iovs.push_back(iovec{buf.data(), data.size}); + return homeblocks::detail::sync_get(dev_->write(craft::client_hdr{term, -1, -1}, dlsn, lba * k_page_size, + nlbas * k_page_size, std::move(data))); + } + + // Real write() call for an all_zeros (unmap) write -- populates the overlay's all_zeros marker. + auto do_write_zeros(uint64_t term, int64_t dlsn, lba_t lba, uint32_t nlbas) { + sisl::sg_list empty_data{}; + return homeblocks::detail::sync_get(dev_->write(craft::client_hdr{term, -1, -1}, dlsn, lba * k_page_size, + nlbas * k_page_size, std::move(empty_data))); + } + + // Directly seed a committed index entry with real backing bytes, bypassing write()/commit() -- + // for read()-test cases that only care about index-sourced state. + void seed_index_entry(lba_t lba, homestore::blk_num_t blk_num, std::vector< uint8_t > data) { + auto csum = crc16_t10dif(k_test_crc16_seed, data.data(), data.size()); + index_.entries[lba] = BlockInfo{homestore::blk_id{blk_num, 1, /* chunk = */ 1}, homestore::blk_id{}, csum}; + journal_->seed_block(blk_num, std::move(data)); + } + + auto do_read(int64_t read_lsn, lba_t lba, uint32_t nlbas) { + dest_buf_.assign(nlbas * k_page_size, 0xFF); // non-zero filler so hole-zeroing is actually observable + sisl::sg_list dest; + dest.size = dest_buf_.size(); + dest.iovs.push_back(iovec{dest_buf_.data(), dest_buf_.size()}); + return homeblocks::detail::sync_get(dev_->read_with( + read_lsn, lba * k_page_size, nlbas * k_page_size, std::move(dest), + [this](lba_t s, lba_t e, std::vector< std::pair< VolumeIndexKey, VolumeIndexValue > >& out) { + return index_.read_from_index(s, e, out); + })); + } + + std::vector< uint8_t > dest_buf_; + MockCraftJournalBackend* journal_{nullptr}; + FakeIndex index_; + std::unique_ptr< CraftReplDev > dev_; +}; + +// ── tests ───────────────────────────────────────────────────────────────────── + +// A single in-order slot applies cleanly: index gains an entry per LBA, commit_lsn advances. +TEST_F(CraftCommitTest, InOrderApply) { + dev_->seed_lsns(0, {}); + journal_->add_data_slot(0, /*lba=*/0, /*nlbas=*/2, /*blk_num=*/100, {11, 22}); + + auto r = do_commit(0); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(*r, 0); + EXPECT_EQ(dev_->commit_lsn(), 0); + ASSERT_TRUE(index_.entries.count(0)); + ASSERT_TRUE(index_.entries.count(1)); + EXPECT_EQ(index_.entries[0].new_checksum, 11); + EXPECT_EQ(index_.entries[1].new_checksum, 22); +} + +// commit() must stall at the first gap in missing_lsns_ rather than error -- entries before the +// gap apply; the gap and everything after it are left untouched. +TEST_F(CraftCommitTest, StallAtFirstMissingHole) { + dev_->seed_lsns(5, {2}); + journal_->add_data_slot(0, 0, 1, 100, {11}); + journal_->add_data_slot(1, 1, 1, 101, {12}); + // lsn=2 deliberately has no journal slot -- matches its missing_lsns_ entry. + journal_->add_data_slot(3, 3, 1, 103, {14}); + journal_->add_data_slot(4, 4, 1, 104, {15}); + journal_->add_data_slot(5, 5, 1, 105, {16}); + + auto r = do_commit(5); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(*r, 1); // stalls right before the hole at lsn=2 + EXPECT_EQ(dev_->commit_lsn(), 1); + EXPECT_TRUE(index_.entries.count(0)); + EXPECT_TRUE(index_.entries.count(1)); + EXPECT_FALSE(index_.entries.count(3)); // never reached +} + +// An Empty-verdicted lsn is skipped without a journal read -- if commit_impl tried to read_slot it, +// the call would fail (no slot seeded there) and the whole commit would error instead of succeeding. +TEST_F(CraftCommitTest, EmptySlotSkip) { + dev_->seed_lsns(2, {}); + dev_->seed_empty({1}); + journal_->add_data_slot(0, 0, 1, 100, {11}); + journal_->add_data_slot(2, 2, 1, 102, {13}); + + auto r = do_commit(2); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(*r, 2); + EXPECT_EQ(dev_->commit_lsn(), 2); + EXPECT_TRUE(index_.entries.count(0)); + EXPECT_TRUE(index_.entries.count(2)); +} + +// all_zeros apply removes the index entry and reclaims its block. +TEST_F(CraftCommitTest, AllZerosApplyRemovesEntryAndFreesBlock) { + index_.entries[0] = BlockInfo{homestore::blk_id{500, 1, 1}, homestore::blk_id{}, 99}; + dev_->seed_lsns(0, {}); + journal_->slots[0] = JournalSlot{.lsn = 0, .all_zeros = true, .lba_off_bytes = 0, .len_bytes = k_page_size}; + + auto r = do_commit(0); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 0); + EXPECT_FALSE(index_.entries.count(0)); + EXPECT_EQ(journal_->free_data_calls, 1); +} + +// A data apply over an already-mapped LBA writes the new entry and reclaims the superseded block. +TEST_F(CraftCommitTest, DataApplyWritesEntryAndFreesSupersededBlock) { + index_.entries[0] = BlockInfo{homestore::blk_id{500, 1, 1}, homestore::blk_id{}, 77}; + dev_->seed_lsns(0, {}); + journal_->add_data_slot(0, 0, 1, /*blk_num=*/999, {222}); + + auto r = do_commit(0); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), 0); + ASSERT_TRUE(index_.entries.count(0)); + EXPECT_EQ(index_.entries[0].new_checksum, 222); + EXPECT_EQ(journal_->free_data_calls, 1); // old blk_id{500,1,1} reclaimed +} + +// LBA written at dLSN 3 and 5 (5 superseding 3 in the overlay, highest-dLSN-wins); committing +// through 3 must not retire the overlay entry, since its recorded lsn is 5, not 3. +TEST_F(CraftCommitTest, OverlayRetiresOnlyIfLsnMatches) { + dev_->seed_lsns(2, {}); + dev_->seed_commit_lsn(2); + ASSERT_TRUE(do_write_data(0, 3, /*lba=*/0, /*nlbas=*/1).has_value()); + ASSERT_TRUE(do_write_data(0, 4, /*lba=*/1, /*nlbas=*/1).has_value()); + ASSERT_TRUE(do_write_data(0, 5, /*lba=*/0, /*nlbas=*/1).has_value()); + ASSERT_EQ(dev_->overlay_lsn_for(0), 5); // highest-dLSN-wins already, before any commit + + auto r = do_commit(3); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(*r, 3); + EXPECT_EQ(dev_->commit_lsn(), 3); + EXPECT_EQ(dev_->overlay_lsn_for(0), 5); // survives: recorded lsn (5) != lsn just applied (3) +} + +// write()'s overlay population must also handle all_zeros writes (not just data writes) -- +// highest-dLSN-wins applies identically to the all_zeros marker entries. +TEST_F(CraftCommitTest, OverlayPopulatesForAllZerosWrites) { + sisl::sg_list empty_data{}; + auto r = homeblocks::detail::sync_get( + dev_->write(craft::client_hdr{0, -1, -1}, /* dlsn = */ 0, /* addr = */ 0, k_page_size, std::move(empty_data))); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->overlay_lsn_for(0), 0); +} + +// A slot's blkid may have more than one piece (multi_blk_id::iterate() walks each of them); +// verify the decomposition maps every piece's blocks to consecutive LBAs, not just the first piece. +TEST_F(CraftCommitTest, DataApplyDecomposesMultiPieceBlkid) { + homestore::multi_blk_id blkid{/* blk_num = */ 500, /* nblks = */ 2, /* chunk = */ 1}; + blkid.add(/* blk_num = */ 900, /* nblks = */ 1, /* chunk = */ 1); // second, non-contiguous piece + journal_->slots[0] = + JournalSlot{.lsn = 0, .lba_off_bytes = 0, .len_bytes = 3 * k_page_size, .blkid = blkid, .csums = {11, 22, 33}}; + dev_->seed_lsns(0, {}); + + auto r = do_commit(0); + ASSERT_TRUE(r.has_value()); + ASSERT_TRUE(index_.entries.count(0)); + ASSERT_TRUE(index_.entries.count(1)); + ASSERT_TRUE(index_.entries.count(2)); + EXPECT_EQ(index_.entries[0].new_checksum, 11); + EXPECT_EQ(index_.entries[1].new_checksum, 22); + EXPECT_EQ(index_.entries[2].new_checksum, 33); + // lba=0,1 come from the first piece (blk_num 500,501); lba=2 from the second, non-contiguous + // piece (blk_num 900) -- proves iterate() walked both pieces, not just the first. + EXPECT_EQ(index_.entries[0].new_blkid.blk_num(), 500u); + EXPECT_EQ(index_.entries[1].new_blkid.blk_num(), 501u); + EXPECT_EQ(index_.entries[2].new_blkid.blk_num(), 900u); +} + +// A concurrent (here: reentrant, called synchronously from inside the outer commit's own write_fn +// callback -- so commit_running_ is still true) commit invocation must be a safe no-op, not a +// second, overlapping application of the same range. +TEST_F(CraftCommitTest, ConcurrentCommitIsNoOp) { + dev_->seed_lsns(0, {}); + journal_->add_data_slot(0, 0, 1, 100, {11}); + + int reentrant_write_calls = 0; + auto reentrant_write_fn = [&](lba_t s, lba_t e, std::unordered_map< lba_t, BlockInfo >& info) { + auto nested = homeblocks::detail::sync_get(dev_->commit_with( + 0, + [&](lba_t, lba_t, std::unordered_map< lba_t, BlockInfo >&) { + ++reentrant_write_calls; + return ok(); + }, + [](lba_t, lba_t, std::vector< homestore::blk_id >&) { return ok(); })); + EXPECT_TRUE(nested.has_value()); + EXPECT_EQ(*nested, -1); // commit_lsn hasn't advanced yet -- the outer run is still in progress + return index_.write_to_index(s, e, info); + }; + + auto outer = homeblocks::detail::sync_get( + dev_->commit_with(0, reentrant_write_fn, [this](lba_t s, lba_t e, std::vector< homestore::blk_id >& freed) { + return index_.delete_lba_range(s, e, freed); + })); + ASSERT_TRUE(outer.has_value()); + EXPECT_EQ(*outer, 0); + EXPECT_EQ(reentrant_write_calls, 0); // the nested call never touched the index + EXPECT_TRUE(index_.entries.count(0)); // the outer call's own apply still went through +} + +// A genuine index-write failure aborts the commit immediately; commit_lsn does not advance past +// the last successfully-applied lsn. +TEST_F(CraftCommitTest, WriteFnErrorAbortsCommit) { + dev_->seed_lsns(1, {}); + journal_->add_data_slot(0, 0, 1, 100, {11}); + journal_->add_data_slot(1, 1, 1, 101, {12}); + + auto r = homeblocks::detail::sync_get(dev_->commit_with( + 1, + [](lba_t, lba_t, std::unordered_map< lba_t, BlockInfo >&) -> status { + return std::unexpected(volume_error::INDEX_ERROR); + }, + [](lba_t, lba_t, std::vector< homestore::blk_id >&) { return ok(); })); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), -1); // nothing applied +} + +// commit() clamps to last_append_lsn even if asked to commit further than what has actually been +// appended locally (e.g. the client's own view of commit_lsn is ahead of this replica). +TEST_F(CraftCommitTest, ClampsToLastAppendLsn) { + dev_->seed_lsns(1, {}); + journal_->add_data_slot(0, 0, 1, 100, {11}); + journal_->add_data_slot(1, 1, 1, 101, {12}); + + auto r = do_commit(/* upto_lsn = */ 100); // far beyond last_append_lsn=1 + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(*r, 1); + EXPECT_EQ(dev_->commit_lsn(), 1); +} + +// A second commit() call past an already-fully-applied range is a true no-op: no further index +// writes happen (proves repeated best-effort commit() calls from every write() don't re-apply). +TEST_F(CraftCommitTest, RepeatedCommitIsNoOp) { + dev_->seed_lsns(0, {}); + journal_->add_data_slot(0, 0, 1, 100, {11}); + ASSERT_TRUE(do_commit(0).has_value()); + ASSERT_EQ(dev_->commit_lsn(), 0); + + int write_calls = 0; + auto r = homeblocks::detail::sync_get(dev_->commit_with( + 0, + [&](lba_t s, lba_t e, std::unordered_map< lba_t, BlockInfo >& info) { + ++write_calls; + return index_.write_to_index(s, e, info); + }, + [this](lba_t s, lba_t e, std::vector< homestore::blk_id >& freed) { + return index_.delete_lba_range(s, e, freed); + })); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(*r, 0); + EXPECT_EQ(write_calls, 0); // nothing left to apply +} + +// Defense-in-depth: write() rejects len==0 at the client boundary (see CraftWriteTest.ZeroLenRejected), +// but a stale/legacy on-disk record could still have one. commit() must abort cleanly rather than +// let nlbas=0 underflow the end_lba computation into a near-UINT64_MAX range. +TEST_F(CraftCommitTest, MalformedZeroLenSlotAborts) { + dev_->seed_lsns(0, {}); + journal_->slots[0] = JournalSlot{.lsn = 0, .lba_off_bytes = 0, .len_bytes = 0}; + + auto r = do_commit(0); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), -1); +} + +// ── read() ──────────────────────────────────────────────────────────────────── + +// A committed (index-only) LBA reads back as a single data extent with the exact bytes seeded. +TEST_F(CraftCommitTest, ReadIndexOnly) { + std::vector< uint8_t > content(k_page_size, 0xCD); + seed_index_entry(/* lba = */ 0, /* blk_num = */ 500, content); + + auto r = do_read(/* read_lsn = */ 10, /* lba = */ 0, /* nlbas = */ 1); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); + EXPECT_EQ(r->extents[0].addr, 0u); + EXPECT_EQ(r->extents[0].len, k_page_size); + EXPECT_FALSE(r->extents[0].hole); + EXPECT_EQ(dest_buf_, content); +} + +// An appended-but-not-yet-committed write is still locally readable via the overlay -- no commit() +// call happens in this test at all. +TEST_F(CraftCommitTest, ReadOverlayOnly) { + ASSERT_TRUE(do_write_data(0, /* dlsn = */ 5, /* lba = */ 0, /* nlbas = */ 1).has_value()); + + auto r = do_read(/* read_lsn = */ 5, /* lba = */ 0, /* nlbas = */ 1); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); + EXPECT_FALSE(r->extents[0].hole); + EXPECT_EQ(dest_buf_, std::vector< uint8_t >(k_page_size, 0xAB)); // do_write_data's fixed fill +} + +// An overlay entry above read_lsn is held but never served -- the index's older, still-valid-as-of- +// read_lsn value must be used instead, not treated as a hole. +TEST_F(CraftCommitTest, HorizonClampServesIndexNotOverlayAboveReadLsn) { + std::vector< uint8_t > old_content(k_page_size, 0xCD); + seed_index_entry(/* lba = */ 0, /* blk_num = */ 500, old_content); + dev_->seed_commit_lsn(3); + + ASSERT_TRUE(do_write_data(0, /* dlsn = */ 5, /* lba = */ 0, /* nlbas = */ 1).has_value()); + + auto r = do_read(/* read_lsn = */ 4, /* lba = */ 0, /* nlbas = */ 1); // 4 < overlay's lsn=5 + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); + EXPECT_FALSE(r->extents[0].hole); + EXPECT_EQ(dest_buf_, old_content); // NOT the overlay's 0xAB fill +} + +// Absent from both the index and the overlay -- reads as a hole (zero-filled), not an error. +TEST_F(CraftCommitTest, AbsentRangeIsHole) { + auto r = do_read(/* read_lsn = */ 10, /* lba = */ 0, /* nlbas = */ 1); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); + EXPECT_TRUE(r->extents[0].hole); + EXPECT_EQ(dest_buf_, std::vector< uint8_t >(k_page_size, 0)); +} + +// An all_zeros overlay entry (unapplied WRITE_ZEROES) reads as a hole -- no block to read at all. +TEST_F(CraftCommitTest, AllZerosOverlayIsHole) { + ASSERT_TRUE(do_write_zeros(0, /* dlsn = */ 5, /* lba = */ 0, /* nlbas = */ 1).has_value()); + + auto r = do_read(/* read_lsn = */ 5, /* lba = */ 0, /* nlbas = */ 1); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); + EXPECT_TRUE(r->extents[0].hole); + EXPECT_EQ(dest_buf_, std::vector< uint8_t >(k_page_size, 0)); +} + +// A data write whose actual payload happens to be all-zero bytes must collapse to a hole at READ +// time -- the index still says "data" (this is not an all_zeros/unmap entry), so this proves the +// scan runs on read, not on write. +TEST_F(CraftCommitTest, DataWriteOfAllZeroBytesCollapsesAtReadTimeNotWriteTime) { + std::vector< uint8_t > zero_content(k_page_size, 0x00); + seed_index_entry(/* lba = */ 0, /* blk_num = */ 500, zero_content); + + auto r = do_read(/* read_lsn = */ 10, /* lba = */ 0, /* nlbas = */ 1); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); + EXPECT_TRUE(r->extents[0].hole); + EXPECT_EQ(dest_buf_, zero_content); +} + +// Corrupted bytes behind a valid index entry (checksum no longer matches) must fail the read rather +// than silently return bad data. +TEST_F(CraftCommitTest, CrcMismatchFails) { + std::vector< uint8_t > content(k_page_size, 0xCD); + seed_index_entry(/* lba = */ 0, /* blk_num = */ 500, content); + journal_->seed_block(500, std::vector< uint8_t >(k_page_size, 0xEE)); // corrupt after csum was computed + + auto r = do_read(/* read_lsn = */ 10, /* lba = */ 0, /* nlbas = */ 1); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(volume_error::CRC_MISMATCH)); +} + +// A multi-LBA read spanning a contiguous data run followed by a hole must merge the run into one +// extent (one batched read, not one per LBA) and report the hole as a separate, correctly-offset +// extent. +TEST_F(CraftCommitTest, MultiLbaReadMergesAdjacentExtents) { + std::vector< uint8_t > content0(k_page_size, 0xCD); + std::vector< uint8_t > content1(k_page_size, 0xCE); + seed_index_entry(0, /* blk_num = */ 500, content0); + seed_index_entry(1, /* blk_num = */ 501, content1); // contiguous blk_num -- merges with lba=0's run + // lba=2 left absent -> hole + + auto r = do_read(/* read_lsn = */ 10, /* lba = */ 0, /* nlbas = */ 3); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 2u); + EXPECT_FALSE(r->extents[0].hole); + EXPECT_EQ(r->extents[0].addr, 0u); + EXPECT_EQ(r->extents[0].len, 2 * k_page_size); + EXPECT_TRUE(r->extents[1].hole); + EXPECT_EQ(r->extents[1].addr, 2 * k_page_size); + EXPECT_EQ(r->extents[1].len, k_page_size); + + std::vector< uint8_t > expected(content0); + expected.insert(expected.end(), content1.begin(), content1.end()); + expected.insert(expected.end(), k_page_size, 0); + EXPECT_EQ(dest_buf_, expected); +} + +// An in-horizon overlay entry must win over an EXISTING committed index entry for the same LBA +// (not just "no index entry at all", which ReadOverlayOnly already covers) -- the overlay is +// strictly newer, so its content must be served, not the index's stale one. +TEST_F(CraftCommitTest, OverlayWinsOverCommittedIndexForSameLba) { + std::vector< uint8_t > old_content(k_page_size, 0xCD); + seed_index_entry(/* lba = */ 0, /* blk_num = */ 500, old_content); + dev_->seed_commit_lsn(2); + + ASSERT_TRUE(do_write_data(0, /* dlsn = */ 3, /* lba = */ 0, /* nlbas = */ 1).has_value()); + + auto r = do_read(/* read_lsn = */ 3, /* lba = */ 0, /* nlbas = */ 1); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); + EXPECT_FALSE(r->extents[0].hole); + EXPECT_EQ(dest_buf_, std::vector< uint8_t >(k_page_size, 0xAB)); // overlay's fill, NOT old_content +} + +// The horizon clamp is inclusive: an overlay entry whose lsn EQUALS read_lsn must be served, not +// treated as "above the horizon" -- HorizonClampServesIndexNotOverlayAboveReadLsn only covers the +// strictly-greater-than case. +TEST_F(CraftCommitTest, HorizonBoundaryEqualLsnServed) { + dev_->seed_commit_lsn(2); + ASSERT_TRUE(do_write_data(0, /* dlsn = */ 3, /* lba = */ 0, /* nlbas = */ 1).has_value()); + + auto r = do_read(/* read_lsn = */ 3, /* lba = */ 0, /* nlbas = */ 1); // read_lsn == overlay's lsn + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); + EXPECT_FALSE(r->extents[0].hole); + EXPECT_EQ(dest_buf_, std::vector< uint8_t >(k_page_size, 0xAB)); // served, not clamped away +} + +// A single read spanning three distinct sources in one call: committed index data, an absent hole, +// and an in-horizon overlay entry -- proving the per-LBA source resolution and extent-building logic +// handle all three simultaneously, not just any two at a time. +TEST_F(CraftCommitTest, ThreeWayMixedExtentRead) { + std::vector< uint8_t > index_content(k_page_size, 0xCD); + seed_index_entry(/* lba = */ 0, /* blk_num = */ 500, index_content); + // lba=1 left absent -> hole + dev_->seed_commit_lsn(2); + ASSERT_TRUE(do_write_data(0, /* dlsn = */ 3, /* lba = */ 2, /* nlbas = */ 1).has_value()); + + auto r = do_read(/* read_lsn = */ 3, /* lba = */ 0, /* nlbas = */ 3); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 3u); + EXPECT_FALSE(r->extents[0].hole); // lba=0: index + EXPECT_EQ(r->extents[0].addr, 0u); + EXPECT_EQ(r->extents[0].len, k_page_size); + EXPECT_TRUE(r->extents[1].hole); // lba=1: absent + EXPECT_EQ(r->extents[1].addr, k_page_size); + EXPECT_FALSE(r->extents[2].hole); // lba=2: overlay + EXPECT_EQ(r->extents[2].addr, 2 * k_page_size); + + std::vector< uint8_t > expected(index_content); + expected.insert(expected.end(), k_page_size, 0); + expected.insert(expected.end(), k_page_size, 0xAB); // do_write_data's fixed fill + EXPECT_EQ(dest_buf_, expected); +} + +// A write whose LBA range only PARTIALLY overlaps existing committed entries: the untouched LBA +// must still read from the index, while the overlapping AND newly-covered LBAs read from the +// overlay -- proving per-LBA resolution isn't confused by a write that straddles a boundary rather +// than exactly matching prior LBA ranges. +TEST_F(CraftCommitTest, PartialOverlapWriteResolvesPerLba) { + seed_index_entry(0, /* blk_num = */ 500, std::vector< uint8_t >(k_page_size, 0xA0)); + seed_index_entry(1, /* blk_num = */ 501, std::vector< uint8_t >(k_page_size, 0xA1)); + seed_index_entry(2, /* blk_num = */ 502, std::vector< uint8_t >(k_page_size, 0xA2)); + dev_->seed_commit_lsn(5); + + // Overlaps committed lba=1,2 and additionally covers new lba=3. + ASSERT_TRUE(do_write_data(0, /* dlsn = */ 6, /* lba = */ 1, /* nlbas = */ 3).has_value()); + + auto r = do_read(/* read_lsn = */ 6, /* lba = */ 0, /* nlbas = */ 4); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); // index-data and overlay-data are both "data" -- one merged extent + EXPECT_FALSE(r->extents[0].hole); + EXPECT_EQ(r->extents[0].len, 4 * k_page_size); + + // lba=0: untouched by the write -- still the index's original content. + std::vector< uint8_t > lba0(dest_buf_.begin(), dest_buf_.begin() + k_page_size); + EXPECT_EQ(lba0, std::vector< uint8_t >(k_page_size, 0xA0)); + // lba=1,2,3: all served from the overlay now, including the two that were previously committed. + for (uint32_t i = 1; i < 4; ++i) { + std::vector< uint8_t > seg(dest_buf_.begin() + i * k_page_size, dest_buf_.begin() + (i + 1) * k_page_size); + EXPECT_EQ(seg, std::vector< uint8_t >(k_page_size, 0xAB)) << "lba=" << i; + } +} + +// The end-to-end visibility transition: before commit, a write is only readable via the overlay; +// after commit, the overlay entry is retired and the SAME content is now served from the index. +TEST_F(CraftCommitTest, CommitTransitionsReadFromOverlayToIndex) { + dev_->seed_lsns(-1, {}); + ASSERT_TRUE(do_write_data(0, /* dlsn = */ 0, /* lba = */ 0, /* nlbas = */ 1).has_value()); + + auto r1 = do_read(/* read_lsn = */ 0, /* lba = */ 0, /* nlbas = */ 1); + ASSERT_TRUE(r1.has_value()); + EXPECT_FALSE(r1->extents[0].hole); + EXPECT_EQ(dest_buf_, std::vector< uint8_t >(k_page_size, 0xAB)); + EXPECT_NE(dev_->overlay_lsn_for(0), -1); // still only in the overlay + + ASSERT_TRUE(do_commit(0).has_value()); + EXPECT_EQ(dev_->overlay_lsn_for(0), -1); // retired + + auto r2 = do_read(/* read_lsn = */ 0, /* lba = */ 0, /* nlbas = */ 1); + ASSERT_TRUE(r2.has_value()); + EXPECT_FALSE(r2->extents[0].hole); + EXPECT_EQ(dest_buf_, std::vector< uint8_t >(k_page_size, 0xAB)); // same content, now from the index +} + +// read()'s own term fencing -- mirrors write()'s TermRejection. The term check runs before the +// !indx_tbl_ guard, so this is reachable even with indx_tbl_ == nullptr in this fixture. +TEST_F(CraftCommitTest, ReadRejectsStaleTerm) { + dev_->seed_term(7); + std::vector< uint8_t > buf(k_page_size, 0xFF); + sisl::sg_list dest; + dest.size = buf.size(); + dest.iovs.push_back(iovec{buf.data(), buf.size()}); + + auto r = homeblocks::detail::sync_get( + dev_->read(craft::client_hdr{/* term = */ 3, -1, -1}, /* read_lsn = */ 0, 0, k_page_size, std::move(dest))); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(volume_error::STALE_TERM)); +} + +// read()'s own boundary validation -- mirrors write()'s ZeroLenRejected/UnalignedLenRejected/ +// UnalignedAddrRejected. read_impl has the identical nlbas=0 underflow risk write() guards against. +TEST_F(CraftCommitTest, ReadZeroLenRejected) { + auto r = do_read(/* read_lsn = */ 10, /* lba = */ 0, /* nlbas = */ 0); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(std::errc::invalid_argument)); +} + +TEST_F(CraftCommitTest, ReadUnalignedLenRejected) { + dest_buf_.assign(1, 0xFF); + sisl::sg_list dest; + dest.size = dest_buf_.size(); + dest.iovs.push_back(iovec{dest_buf_.data(), dest_buf_.size()}); + auto r = homeblocks::detail::sync_get( + dev_->read_with(10, 0, /* len = */ k_page_size / 2, std::move(dest), + [this](lba_t s, lba_t e, std::vector< std::pair< VolumeIndexKey, VolumeIndexValue > >& out) { + return index_.read_from_index(s, e, out); + })); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(std::errc::invalid_argument)); +} + +TEST_F(CraftCommitTest, ReadUnalignedAddrRejected) { + dest_buf_.assign(k_page_size, 0xFF); + sisl::sg_list dest; + dest.size = dest_buf_.size(); + dest.iovs.push_back(iovec{dest_buf_.data(), dest_buf_.size()}); + auto r = homeblocks::detail::sync_get( + dev_->read_with(10, /* addr = */ 1, k_page_size, std::move(dest), + [this](lba_t s, lba_t e, std::vector< std::pair< VolumeIndexKey, VolumeIndexValue > >& out) { + return index_.read_from_index(s, e, out); + })); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(std::errc::invalid_argument)); +} + +// ── rebuild_overlay() ───────────────────────────────────────────────────────── + +// A fresh instance's overlay_ is reconstructed from the journal alone: every present entry above +// commit_lsn populates the overlay exactly as write()'s own post-flight update would have. +TEST_F(CraftCommitTest, RebuildOverlayPopulatesFromJournal) { + dev_->seed_commit_lsn(2); + dev_->seed_lsns(5, {}); + journal_->add_data_slot(3, /* lba = */ 0, /* nlbas = */ 1, /* blk_num = */ 700, {31}); + journal_->add_data_slot(4, /* lba = */ 1, /* nlbas = */ 1, /* blk_num = */ 701, {32}); + journal_->add_data_slot(5, /* lba = */ 2, /* nlbas = */ 1, /* blk_num = */ 702, {33}); + + auto r = homeblocks::detail::sync_get(dev_->rebuild_overlay()); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->overlay_lsn_for(0), 3); + EXPECT_EQ(dev_->overlay_lsn_for(1), 4); + EXPECT_EQ(dev_->overlay_lsn_for(2), 5); +} + +// Missing and Empty-verdicted lsns within the walked range are skipped (no journal read attempted -- +// if the walk tried to read_slot a missing lsn, MockCraftJournalBackend would error and the whole +// rebuild would fail), not treated as a stop condition like commit_impl()'s gap handling. +TEST_F(CraftCommitTest, RebuildOverlaySkipsMissingAndEmpty) { + dev_->seed_commit_lsn(0); + dev_->seed_lsns(4, {2}); // lsn=2 missing -- deliberately has no journal slot + dev_->seed_empty({3}); // lsn=3 Empty-verdicted -- also has no journal slot + journal_->add_data_slot(1, /* lba = */ 0, /* nlbas = */ 1, /* blk_num = */ 700, {11}); + journal_->add_data_slot(4, /* lba = */ 1, /* nlbas = */ 1, /* blk_num = */ 701, {14}); + + auto r = homeblocks::detail::sync_get(dev_->rebuild_overlay()); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->overlay_lsn_for(0), 1); // before the gap + EXPECT_EQ(dev_->overlay_lsn_for(1), 4); // after both the gap and the Empty slot +} + +// Two entries touching the same LBA at different lsns within the walked range: the higher-dLSN one +// must win, same rule write() itself already applies. +TEST_F(CraftCommitTest, RebuildOverlayHighestDlsnWins) { + dev_->seed_commit_lsn(0); + dev_->seed_lsns(2, {}); + journal_->add_data_slot(1, /* lba = */ 0, /* nlbas = */ 1, /* blk_num = */ 700, {11}); + journal_->add_data_slot(2, /* lba = */ 0, /* nlbas = */ 1, /* blk_num = */ 701, {12}); // supersedes lsn=1 + + auto r = homeblocks::detail::sync_get(dev_->rebuild_overlay()); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->overlay_lsn_for(0), 2); +} + +// An all_zeros entry within the walked range populates the overlay's all_zeros marker, not a data +// (blkid+csum) entry. +TEST_F(CraftCommitTest, RebuildOverlayHandlesAllZeros) { + dev_->seed_commit_lsn(0); + dev_->seed_lsns(1, {}); + journal_->slots[1] = JournalSlot{.lsn = 1, .all_zeros = true, .lba_off_bytes = 0, .len_bytes = k_page_size}; + + auto r = homeblocks::detail::sync_get(dev_->rebuild_overlay()); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->overlay_lsn_for(0), 1); + + // Confirm it reads as a hole (the all_zeros marker, not stray data) -- same read path Read tests + // already exercise for a live all_zeros overlay entry. + auto read_r = do_read(/* read_lsn = */ 1, /* lba = */ 0, /* nlbas = */ 1); + ASSERT_TRUE(read_r.has_value()); + EXPECT_TRUE(read_r->extents[0].hole); +} + +// A read_slot() failure for a present (non-missing/non-empty) lsn propagates out of rebuild_overlay() +// rather than silently leaving a partially-reconstructed overlay. +TEST_F(CraftCommitTest, RebuildOverlayPropagatesReadSlotError) { + dev_->seed_commit_lsn(0); + dev_->seed_lsns(1, {}); + journal_->add_data_slot(1, /* lba = */ 0, /* nlbas = */ 1, /* blk_num = */ 700, {11}); + journal_->fail_on_read = 1; + + auto r = homeblocks::detail::sync_get(dev_->rebuild_overlay()); + ASSERT_FALSE(r.has_value()); +} + +// ── restart recovery gate (SDSTOR-22905) ────────────────────────────────────── + +// Per subtasks.md/SDSTOR-22905, the overlay must be fully rebuilt before the volume accepts new +// client I/O. trigger_on_restart() drives the EXACT production path (CraftRaftListener::on_restart() +// -> run_recovery() -> rebuild_overlay()), with the mock's read_slot() gated closed so the rebuild +// genuinely blocks -- proving write()/read()/keep_alive() all reject with OFFLINE while recovering_ +// is set, and the gate is gone (recovering_ false, overlay actually rebuilt) once it completes. +// +// Uses the real public write()/read()/keep_alive() here, not do_write_data()/do_read() (which go +// through commit_with()/read_with()'s test seams and would bypass the gate entirely, since it lives +// in write()/read()/keep_alive() themselves, not commit_impl()/read_impl()). This fixture's indx_tbl_ +// is null, so read() still fails not_supported once the gate is gone -- that's expected and besides +// the point here; what matters is that it's a DIFFERENT error, proving OFFLINE was the gate, not an +// unrelated rejection that happened to look similar. +TEST_F(CraftCommitTest, RestartRecoveryGatesClientIoUntilOverlayRebuilt) { + dev_->seed_term(7); + dev_->seed_commit_lsn(0); + dev_->seed_lsns(1, {}); + journal_->add_data_slot(1, /* lba = */ 0, /* nlbas = */ 1, /* blk_num = */ 700, {11}); + journal_->close_read_gate(); + + EXPECT_FALSE(dev_->is_recovering()); + std::thread restart_thread([this] { dev_->trigger_on_restart(); }); + + // trigger_on_restart() sets recovering_ BEFORE detaching run_recovery() (which is what then blocks + // on the closed read gate) -- so this becomes true almost immediately; a short, generous wait + // avoids a hard poll loop without making the test slow in the common case. + for (int i = 0; i < 1000 && !dev_->is_recovering(); ++i) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + ASSERT_TRUE(dev_->is_recovering()); + + static std::vector< uint8_t > buf(k_page_size, 0xAA); + sisl::sg_list wdata; + wdata.size = k_page_size; + wdata.iovs.push_back(iovec{buf.data(), wdata.size}); + auto write_r = + homeblocks::detail::sync_get(dev_->write(craft::client_hdr{7, -1, -1}, /* dlsn = */ 10, + /* addr = */ 5 * k_page_size, k_page_size, std::move(wdata))); + ASSERT_FALSE(write_r.has_value()); + EXPECT_EQ(write_r.error(), volume_error::OFFLINE); + + dest_buf_.assign(k_page_size, 0xFF); + sisl::sg_list rdata; + rdata.size = dest_buf_.size(); + rdata.iovs.push_back(iovec{dest_buf_.data(), dest_buf_.size()}); + auto read_r = homeblocks::detail::sync_get(dev_->read(craft::client_hdr{7, -1, -1}, /* read_lsn = */ 0, + /* addr = */ 5 * k_page_size, k_page_size, std::move(rdata))); + ASSERT_FALSE(read_r.has_value()); + EXPECT_EQ(read_r.error(), volume_error::OFFLINE); + + auto ka_r = homeblocks::detail::sync_get(dev_->keep_alive(craft::client_hdr{7, -1, -1})); + ASSERT_FALSE(ka_r.has_value()); + EXPECT_EQ(ka_r.error(), volume_error::OFFLINE); + + journal_->open_read_gate(); + restart_thread.join(); + + EXPECT_FALSE(dev_->is_recovering()); + EXPECT_EQ(dev_->overlay_lsn_for(0), 1); // the rebuild actually completed, not just unblocked + + // The gate is gone: keep_alive() (which needs no index) succeeds again. + auto ka_r2 = homeblocks::detail::sync_get(dev_->keep_alive(craft::client_hdr{7, -1, -1})); + EXPECT_TRUE(ka_r2.has_value()); + + // read() now fails for the ordinary, unrelated reason (no index configured) -- not OFFLINE. + dest_buf_.assign(k_page_size, 0xFF); + sisl::sg_list rdata2; + rdata2.size = dest_buf_.size(); + rdata2.iovs.push_back(iovec{dest_buf_.data(), dest_buf_.size()}); + auto read_r2 = + homeblocks::detail::sync_get(dev_->read(craft::client_hdr{7, -1, -1}, /* read_lsn = */ 0, + /* addr = */ 5 * k_page_size, k_page_size, std::move(rdata2))); + ASSERT_FALSE(read_r2.has_value()); + EXPECT_EQ(read_r2.error(), std::make_error_condition(std::errc::not_supported)); +} + +// A restart recovery whose rebuild_overlay() FAILS (e.g. a corrupt journal record) must permanently +// fault the partition rather than clearing the gate and resuming I/O against a silently incomplete +// overlay -- entries at and above the failing lsn were never populated, so a naive "always clear +// recovering_" would let a subsequent read for an LBA only covered by one of those entries silently +// fall through to stale, already-superseded state with no error. recovering_ itself is deliberately +// left set (never cleared) on this path too -- recovery_faulted_ is the flag that actually governs +// client-facing rejection from here on (checked first, ahead of recovering_, by every entry point), but +// leaving recovering_ set as well is a defensive belt-and-suspenders, not a load-bearing distinction. +TEST_F(CraftCommitTest, FailedRestartRecoveryPermanentlyFaultsThePartition) { + dev_->seed_term(7); + dev_->seed_commit_lsn(0); + dev_->seed_lsns(1, {}); + journal_->add_data_slot(1, /* lba = */ 0, /* nlbas = */ 1, /* blk_num = */ 700, {11}); + journal_->fail_on_read = 1; + + EXPECT_FALSE(dev_->is_recovery_faulted()); + // read_gate stays open (default) -- run_recovery() runs to completion (with a failure) synchronously + // within this call, same as every other trigger_on_restart() use in this file. + dev_->trigger_on_restart(); + EXPECT_TRUE(dev_->is_recovery_faulted()); + + static std::vector< uint8_t > buf(k_page_size, 0xAA); + sisl::sg_list wdata; + wdata.size = k_page_size; + wdata.iovs.push_back(iovec{buf.data(), wdata.size}); + auto write_r = + homeblocks::detail::sync_get(dev_->write(craft::client_hdr{7, -1, -1}, /* dlsn = */ 10, + /* addr = */ 5 * k_page_size, k_page_size, std::move(wdata))); + ASSERT_FALSE(write_r.has_value()); + EXPECT_EQ(write_r.error(), volume_error::INTERNAL_ERROR); + + dest_buf_.assign(k_page_size, 0xFF); + sisl::sg_list rdata; + rdata.size = dest_buf_.size(); + rdata.iovs.push_back(iovec{dest_buf_.data(), dest_buf_.size()}); + auto read_r = homeblocks::detail::sync_get(dev_->read(craft::client_hdr{7, -1, -1}, /* read_lsn = */ 0, + /* addr = */ 5 * k_page_size, k_page_size, std::move(rdata))); + ASSERT_FALSE(read_r.has_value()); + EXPECT_EQ(read_r.error(), volume_error::INTERNAL_ERROR); + + auto ka_r = homeblocks::detail::sync_get(dev_->keep_alive(craft::client_hdr{7, -1, -1})); + ASSERT_FALSE(ka_r.has_value()); + EXPECT_EQ(ka_r.error(), volume_error::INTERNAL_ERROR); +} + +// ── truncate() overlay pruning ──────────────────────────────────────────────── + +// truncate() must prune any overlay entry referencing a now-rolled-back dLSN -- otherwise a +// subsequent read could serve stale data from a write that no longer exists in the journal +// (subtasks.md's S3 AC: the overlay is "updated on append/apply/truncate"). +TEST_F(CraftCommitTest, TruncateRemovesOverlayEntriesAboveLsn) { + // Deliberately NOT seeding last_append_lsn ahead of these writes: doing so would make write()'s + // own idempotent short-circuit (dlsn <= last_append_lsn && not missing) treat dlsn=3 as "already + // written" and skip overlay population entirely. Starting from the default -1 lets both writes + // advance last_append_lsn for real. + ASSERT_TRUE(do_write_data(0, /* dlsn = */ 3, /* lba = */ 0, /* nlbas = */ 1).has_value()); + ASSERT_TRUE(do_write_data(0, /* dlsn = */ 5, /* lba = */ 1, /* nlbas = */ 1).has_value()); + ASSERT_EQ(dev_->overlay_lsn_for(0), 3); + ASSERT_EQ(dev_->overlay_lsn_for(1), 5); + + auto r = homeblocks::detail::sync_get(dev_->truncate(4)); + ASSERT_TRUE(r.has_value()); + + EXPECT_EQ(dev_->overlay_lsn_for(0), 3); // survives: 3 <= truncation point 4 + EXPECT_EQ(dev_->overlay_lsn_for(1), -1); // pruned: 5 > truncation point 4, rolled back +} + +// ── commit() resume after a gap fills ───────────────────────────────────────── + +// After a previously-missing dLSN is filled, a subsequent commit() must apply through it and reach +// the SAME final state a straight no-gap in-order run would have produced (subtasks.md's S3 AC: +// "in-order apply after the hole fills (same stable state as a no-hole run)"). +TEST_F(CraftCommitTest, InOrderApplyAfterHoleFillsMatchesNoGapRun) { + dev_->seed_lsns(2, {1}); // lsn=1 missing + journal_->add_data_slot(0, 0, 1, 100, {11}); + journal_->add_data_slot(2, 2, 1, 102, {13}); + // lsn=1 has no slot yet -- matches its missing_lsns_ entry. + + auto stalled = do_commit(2); + ASSERT_TRUE(stalled.has_value()); + EXPECT_EQ(*stalled, 0); // stalls right before the hole + + // The gap fills: lsn=1 is now written and removed from missing_lsns_ (mirroring what a real + // write() call does internally when a gap-filling dlsn arrives). + journal_->add_data_slot(1, 1, 1, 101, {12}); + dev_->seed_lsns(2, {}); // last_append_lsn unchanged; missing_lsns_ now empty + + auto resumed = do_commit(2); + ASSERT_TRUE(resumed.has_value()); + EXPECT_EQ(*resumed, 2); + EXPECT_EQ(dev_->commit_lsn(), 2); + ASSERT_TRUE(index_.entries.count(0)); + ASSERT_TRUE(index_.entries.count(1)); + ASSERT_TRUE(index_.entries.count(2)); + EXPECT_EQ(index_.entries[0].new_checksum, 11); + EXPECT_EQ(index_.entries[1].new_checksum, 12); + EXPECT_EQ(index_.entries[2].new_checksum, 13); +} + +// ── keep_alive() all_committed_lsn capture ──────────────────────────────────── + +// keep_alive() must capture hdr.all_committed_lsn into partition state (max-monotonic) so it's +// available for S8's eventual journal reclaim -- the reclaim action itself is not implemented here. +TEST_F(CraftCommitTest, KeepAliveCapturesAllCommittedLsnMonotonically) { + ASSERT_EQ(dev_->all_committed_lsn(), -1); + + auto r1 = homeblocks::detail::sync_get(dev_->keep_alive(craft::client_hdr{0, -1, /* all_committed_lsn = */ 5})); + ASSERT_TRUE(r1.has_value()); + EXPECT_EQ(dev_->all_committed_lsn(), 5); + + // A stale/reordered message with a LOWER all_committed_lsn must not regress the floor. + auto r2 = homeblocks::detail::sync_get(dev_->keep_alive(craft::client_hdr{0, -1, /* all_committed_lsn = */ 2})); + ASSERT_TRUE(r2.has_value()); + EXPECT_EQ(dev_->all_committed_lsn(), 5); + + auto r3 = homeblocks::detail::sync_get(dev_->keep_alive(craft::client_hdr{0, -1, /* all_committed_lsn = */ 9})); + ASSERT_TRUE(r3.has_value()); + EXPECT_EQ(dev_->all_committed_lsn(), 9); +} + +// ── delete_fn error path ────────────────────────────────────────────────────── + +// A delete_fn failure during an all_zeros apply must propagate the error and leave commit_lsn +// unchanged -- mirrors WriteFnErrorAbortsCommit for the delete side of commit_impl(). +TEST_F(CraftCommitTest, DeleteFnErrorAbortsCommit) { + index_.entries[0] = BlockInfo{homestore::blk_id{500, 1, 1}, homestore::blk_id{}, 99}; + dev_->seed_lsns(0, {}); + journal_->slots[0] = JournalSlot{.lsn = 0, .all_zeros = true, .lba_off_bytes = 0, .len_bytes = k_page_size}; + + auto r = homeblocks::detail::sync_get(dev_->commit_with( + 0, [](lba_t, lba_t, std::unordered_map< lba_t, BlockInfo >&) { return ok(); }, + [](lba_t, lba_t, std::vector< homestore::blk_id >&) -> status { + return std::unexpected(volume_error::INDEX_ERROR); + })); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), -1); // nothing applied +} + +// ── read() read_fn error path ───────────────────────────────────────────────── + +// A read_fn failure in read_impl() propagates as an error -- proves read() does not silently +// treat index errors as holes (an absent LBA and an index-read error are distinct conditions). +TEST_F(CraftCommitTest, ReadFnErrorPropagates) { + dest_buf_.assign(k_page_size, 0xFF); + sisl::sg_list dest; + dest.size = dest_buf_.size(); + dest.iovs.push_back(iovec{dest_buf_.data(), dest_buf_.size()}); + + auto r = homeblocks::detail::sync_get(dev_->read_with( + /* read_lsn = */ 10, 0, k_page_size, std::move(dest), + [](lba_t, lba_t, std::vector< std::pair< VolumeIndexKey, VolumeIndexValue > >&) -> status { + return std::unexpected(volume_error::INDEX_ERROR); + })); + ASSERT_FALSE(r.has_value()); +} + +// ── read() negative read_lsn ────────────────────────────────────────────────── + +// A negative read_lsn must be rejected: lsn=-1 would make every overlay entry's lsn<=read_lsn +// check silently false (no real lsn is negative), serving a committed-only view with no error. +TEST_F(CraftCommitTest, ReadNegativeLsnRejected) { + auto r = do_read(/* read_lsn = */ -1, /* lba = */ 0, /* nlbas = */ 1); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(std::errc::invalid_argument)); +} + +// ── keep_alive() term fencing ───────────────────────────────────────────────── + +// keep_alive() must reject a stale term -- mirrors write()'s TermRejection. +TEST_F(CraftCommitTest, KeepAliveRejectsStaleTerm) { + dev_->seed_term(5); + auto r = homeblocks::detail::sync_get(dev_->keep_alive(craft::client_hdr{/* term = */ 3, -1, -1})); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(volume_error::STALE_TERM)); +} + +// ── C1: csum OOB guard in commit_impl() ─────────────────────────────────────── + +// A slot whose persisted csums array is shorter than the blkid piece count must abort the commit +// rather than access csums[csum_idx] out of bounds -- exercises the new C1 defense-in-depth guard. +TEST_F(CraftCommitTest, CommitCsumShortArrayAborts) { + dev_->seed_lsns(0, {}); + // Slot claims 2 LBAs (nlbas=2) but supplies only 1 checksum -- simulates a corrupt on-disk record. + homestore::multi_blk_id blkid{500, /* nblks = */ 2, /* chunk = */ 1}; + journal_->slots[0] = + JournalSlot{.lsn = 0, .lba_off_bytes = 0, .len_bytes = 2 * k_page_size, .blkid = blkid, .csums = {11}}; + + auto r = do_commit(0); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(dev_->commit_lsn(), -1); // commit aborted before any index write +} + +// ── C2: nlbas==0 guard in rebuild_overlay() ─────────────────────────────────── + +// A slot with len_bytes=0 (nlbas=0) within the walked range must be skipped with a log error -- +// exercises the new C2 guard, matching commit_impl()'s identical protection. The slot after the +// malformed one must still be processed (skip, not abort). +TEST_F(CraftCommitTest, RebuildOverlaySkipsNlbasZeroSlot) { + dev_->seed_commit_lsn(0); + dev_->seed_lsns(2, {}); + // lsn=1: malformed zero-len record; lsn=2: valid data slot covering lba=5. + journal_->slots[1] = JournalSlot{.lsn = 1, .lba_off_bytes = 0, .len_bytes = 0}; + journal_->add_data_slot(2, /* lba = */ 5, /* nlbas = */ 1, /* blk_num = */ 800, {42}); + + auto r = homeblocks::detail::sync_get(dev_->rebuild_overlay()); + ASSERT_TRUE(r.has_value()); // rebuild succeeds despite the malformed slot + EXPECT_EQ(dev_->overlay_lsn_for(5), 2); // valid slot after the bad one was still processed +} + +// ── C5: dest.iovs.empty() guard in read_impl() ─────────────────────────────── + +// A dest sg_list with no iovecs must be rejected before read_impl() accesses dest.iovs[0] -- +// exercises the new C5 guard. The len check passes (aligned, non-zero), but the iovec guard fires. +TEST_F(CraftCommitTest, ReadDestEmptyIovsRejected) { + sisl::sg_list dest; + dest.size = k_page_size; // non-zero size claimed but no backing iovec + + auto r = homeblocks::detail::sync_get(dev_->read_with( + /* read_lsn = */ 0, 0, k_page_size, std::move(dest), + [this](lba_t s, lba_t e, std::vector< std::pair< VolumeIndexKey, VolumeIndexValue > >& out) { + return index_.read_from_index(s, e, out); + })); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(std::errc::invalid_argument)); +} + +// ── R1-1: all_committed_lsn captured by write() ────────────────────────────── + +// write() must advance all_committed_lsn max-monotonically (the R1-1 fix) -- without this, +// only keep_alive() messages advance the reclaim floor, leaving write-only partitions permanently +// stuck at -1 even after the cluster has durably committed far ahead of them. +TEST_F(CraftCommitTest, WriteCapturesAllCommittedLsn) { + ASSERT_EQ(dev_->all_committed_lsn(), -1); + + // A write with all_committed_lsn piggybacked advances the floor. + static std::vector< uint8_t > buf(k_page_size, 0xAB); + sisl::sg_list data; + data.size = k_page_size; + data.iovs.push_back(iovec{buf.data(), k_page_size}); + auto r = homeblocks::detail::sync_get( + dev_->write(craft::client_hdr{/* term = */ 0, /* commit_lsn = */ -1, /* all_committed_lsn = */ 7}, + /* dlsn = */ 0, 0, k_page_size, std::move(data))); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->all_committed_lsn(), 7); + + // A subsequent write with a LOWER all_committed_lsn must not regress the floor. + static std::vector< uint8_t > buf2(k_page_size, 0xAB); + sisl::sg_list data2; + data2.size = k_page_size; + data2.iovs.push_back(iovec{buf2.data(), k_page_size}); + auto r2 = homeblocks::detail::sync_get(dev_->write(craft::client_hdr{0, -1, /* all_committed_lsn = */ 3}, + /* dlsn = */ 1, k_page_size, k_page_size, std::move(data2))); + ASSERT_TRUE(r2.has_value()); + EXPECT_EQ(dev_->all_committed_lsn(), 7); // not regressed +} + +// ── R1-1: all_committed_lsn captured by read() ─────────────────────────────── + +// read() must capture hdr.all_committed_lsn (the R1-1 fix applies to read() too). The real +// entry point is used (not read_with()) because only read() accepts a client_hdr. Even though +// read() returns not_supported (no index configured in this fixture), the capture happens in the +// missing_mu_ block ahead of the !indx_tbl_ check, so the floor is advanced before the error. +TEST_F(CraftCommitTest, ReadCapturesAllCommittedLsn) { + ASSERT_EQ(dev_->all_committed_lsn(), -1); + + dest_buf_.assign(k_page_size, 0xFF); + sisl::sg_list dest; + dest.size = dest_buf_.size(); + dest.iovs.push_back(iovec{dest_buf_.data(), dest_buf_.size()}); + auto r = homeblocks::detail::sync_get( + dev_->read(craft::client_hdr{/* term = */ 0, /* commit_lsn = */ -1, /* all_committed_lsn = */ 4}, + /* read_lsn = */ 0, /* addr = */ 0, k_page_size, std::move(dest))); + EXPECT_FALSE(r.has_value()); // fails (not_supported: no index), but floor was captured first + EXPECT_EQ(dev_->all_committed_lsn(), 4); +} + +// ── craft_max_io_len_mb enforcement in read() ──────────────────────────────── + +// read() enforces the same craft_max_io_len_mb cap as write(). craft_max_io_len_mb is set to 1 in +// main(), so a len of 1 MiB + one page (page-aligned, non-zero) exceeds the limit and is rejected. +TEST_F(CraftCommitTest, ReadMaxIoLenEnforced) { + constexpr uint64_t k_over_limit = 1024 * 1024 + k_page_size; + dest_buf_.assign(k_page_size, 0xFF); // small real buffer -- len check fires before any read + sisl::sg_list dest; + dest.size = k_over_limit; + dest.iovs.push_back(iovec{dest_buf_.data(), k_over_limit}); // oversized claim, real check fires first + auto r = homeblocks::detail::sync_get(dev_->read_with( + /* read_lsn = */ 0, 0, k_over_limit, std::move(dest), + [this](lba_t s, lba_t e, std::vector< std::pair< VolumeIndexKey, VolumeIndexValue > >& out) { + return index_.read_from_index(s, e, out); + })); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(std::errc::invalid_argument)); +} + +} // namespace +} // namespace homeblocks + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + SISL_OPTIONS_LOAD(argc, argv, logging); + // craft_watchdog_timeout_ms=0: watchdog disabled (no iomgr needed). + // craft_max_io_len_mb=1: small cap for MaxIoLen tests; all other tests use <= 12 KiB, well below 1 MiB. + HB_SETTINGS_FACTORY().load_json("{\"craft_watchdog_timeout_ms\": 0, \"craft_max_io_len_mb\": 1}"); + return RUN_ALL_TESTS(); +} diff --git a/src/lib/craft/tests/test_craft_commit_hs.cpp b/src/lib/craft/tests/test_craft_commit_hs.cpp new file mode 100644 index 0000000..7c9160c --- /dev/null +++ b/src/lib/craft/tests/test_craft_commit_hs.cpp @@ -0,0 +1,337 @@ +/********************************************************************************* + * 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. + * + *********************************************************************************/ + +// Heavy integration test for CraftReplDev::commit()/read() against a REAL VolumeIndexTable and real +// data blocks -- closing the gap between test_craft_commit.cpp's fake-index unit tests and +// production reality (in particular, VolumeIndexTable::delete_lba_range has no direct unit test +// anywhere else; the all_zeros-unmap test below is it). +// +// There is no production call site that constructs a CRAFT-mode CraftReplDev yet (repl_mode::CRAFT +// on volume_info is declared but not wired to anything) -- so this test creates an ordinary volume +// purely to get a real, fully chunk-allocated VolumeIndexTable (and a real ordinal for routing +// alloc_write_data) via the exact same create_volume() path every other volume test already uses, +// rather than replicating volume::init_index_table's internal chunk-allocation plumbing by hand. +// CRAFT's own journal is a separate, freshly created home_log_store (matching +// test_craft_homestore_backend.cpp's make_logstore() helper) -- distinct from the volume's own +// replication journal, which this test never touches. +// +// No _PRERELEASE seam is needed anywhere here: read() is public, and every call uses term=0, which +// matches CraftPartitionState's default (login() is still a stub, so there is no other way to reach +// a real nonzero term) -- everything drives through CraftReplDev's real client-facing API. +// +// Links the full homeblocks library, same as test_craft_homestore_backend.cpp, because both a real +// home_log_store and a real VolumeIndexTable require a running HomeStore instance. + +#include +#include +#include +#include + +#include +#include +#include +#include + +#include "hb_internal.hpp" +#include "craft/craft_repl_dev.hpp" +#include "coro_helpers.hpp" +#include "volume/volume.hpp" // volume::indx_table() / ordinal() -- home_blocks.hpp only forward-declares volume +#include "test_common.hpp" + +SISL_LOGGING_INIT(HOMEBLOCKS_LOG_MODS) +SISL_OPTIONS_ENABLE(logging, test_common_setup) +SISL_LOGGING_DECL(test_craft_commit_hs) + +std::unique_ptr< test_common::HBTestHelper > g_helper; + +using namespace homeblocks; + +static constexpr uint32_t k_page_size = 4096; + +class CraftCommitHsTest : public ::testing::Test { +protected: + // One real volume (hence one real, chunk-allocated VolumeIndexTable) shared across every test in + // this suite -- each TEST_F creating its own would exhaust the test device pool's chunk capacity + // after only a couple of volumes. Tests stay independent by using disjoint LBA ranges. + static void SetUpTestSuite() { + volume_info vinfo{boost::uuids::random_generator()(), /* size = */ 64ull * 1024 * 1024, k_page_size, + "craft_commit_hs_vol"}; + auto vol_r = homeblocks::detail::sync_get(g_helper->inst()->create_volume(std::move(vinfo))); + ASSERT_TRUE(vol_r.has_value()); + s_vol = vol_r.value(); + } + + static void TearDownTestSuite() { s_vol.reset(); } + + // Each test still gets its own fresh journal (log store) and CraftReplDev -- only the underlying + // real index/data infrastructure is shared. + void SetUp() override { + auto flush_mode = + static_cast< homestore::flush_mode_t >(static_cast< uint32_t >(homestore::flush_mode_t::TIMER) | + static_cast< uint32_t >(homestore::flush_mode_t::INLINE)); + auto logdev_id = homestore::logstore_service().create_new_logdev(flush_mode); + auto logstore = homestore::logstore_service().create_new_log_store(logdev_id, /* append_mode = */ false); + ASSERT_TRUE(logstore != nullptr); + + auto backend = make_homestore_journal_backend(logstore, s_vol->ordinal(), k_page_size); + dev_ = std::make_unique< CraftReplDev >(s_vol->id(), std::move(backend), k_page_size, s_vol->indx_table()); + } + + // term=0 matches CraftPartitionState's default -- login() is still a stub, so there is no other + // way to reach a real nonzero term; every call in this file uses it. content/dest are + // sisl::io_blob_safe (not a plain std::vector) because homestore::data_service()'s real + // async_alloc_write/async_read need a properly aligned buffer. + auto do_write(int64_t dlsn, lba_t lba, uint32_t nlbas, sisl::io_blob_safe const& content, int64_t commit_lsn) { + sisl::sg_list data; + data.size = content.size(); + data.iovs.push_back(iovec{const_cast< uint8_t* >(content.cbytes()), content.size()}); + return homeblocks::detail::sync_get(dev_->write(craft::client_hdr{0, commit_lsn, -1}, dlsn, lba * k_page_size, + nlbas * k_page_size, std::move(data))); + } + + auto do_write_zeros(int64_t dlsn, lba_t lba, uint32_t nlbas, int64_t commit_lsn) { + sisl::sg_list empty_data{}; + return homeblocks::detail::sync_get(dev_->write(craft::client_hdr{0, commit_lsn, -1}, dlsn, lba * k_page_size, + nlbas * k_page_size, std::move(empty_data))); + } + + auto do_read(int64_t read_lsn, lba_t lba, uint32_t nlbas, sisl::io_blob_safe& dest_buf) { + std::memset(dest_buf.bytes(), 0xFF, dest_buf.size()); // non-zero filler so hole-zeroing is observable + sisl::sg_list dest; + dest.size = dest_buf.size(); + dest.iovs.push_back(iovec{dest_buf.bytes(), dest_buf.size()}); + return homeblocks::detail::sync_get(dev_->read(craft::client_hdr{0, -1, -1}, read_lsn, lba * k_page_size, + nlbas * k_page_size, std::move(dest))); + } + + static bool all_zero(sisl::io_blob_safe const& buf) { + return std::all_of(buf.cbytes(), buf.cbytes() + buf.size(), [](uint8_t b) { return b == 0; }); + } + + static volume_handle s_vol; + std::unique_ptr< CraftReplDev > dev_; +}; +volume_handle CraftCommitHsTest::s_vol; + +// A write piggybacking its own commit_lsn applies to the real index immediately; a subsequent read +// at or above that lsn serves the real data back correctly (content and extents both verified). +TEST_F(CraftCommitHsTest, WriteCommitReadRoundTrip) { + sisl::io_blob_safe content{k_page_size, 512}; + std::memset(content.bytes(), 0xAB, content.size()); + ASSERT_TRUE(do_write(/* dlsn = */ 0, /* lba = */ 0, /* nlbas = */ 1, content, /* commit_lsn = */ 0).has_value()); + + sisl::io_blob_safe dest{k_page_size, 512}; + auto r = do_read(/* read_lsn = */ 0, /* lba = */ 0, /* nlbas = */ 1, dest); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); + EXPECT_FALSE(r->extents[0].hole); + EXPECT_EQ(std::memcmp(dest.cbytes(), content.cbytes(), content.size()), 0); +} + +// A write with NO commit piggyback (commit_lsn=-1) never reaches the real index -- the data is +// still locally readable via the journal-tail overlay alone. +TEST_F(CraftCommitHsTest, OverlayReadableBeforeCommit) { + constexpr lba_t k_lba = 10; + sisl::io_blob_safe content{k_page_size, 512}; + std::memset(content.bytes(), 0xCD, content.size()); + ASSERT_TRUE(do_write(/* dlsn = */ 0, k_lba, /* nlbas = */ 1, content, /* commit_lsn = */ -1).has_value()); + + sisl::io_blob_safe dest{k_page_size, 512}; + auto r = do_read(/* read_lsn = */ 0, k_lba, /* nlbas = */ 1, dest); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); + EXPECT_FALSE(r->extents[0].hole); + EXPECT_EQ(std::memcmp(dest.cbytes(), content.cbytes(), content.size()), 0); +} + +// Unmapping a previously-committed LBA (all_zeros write, committed) must reclaim it from the REAL +// VolumeIndexTable (delete_lba_range) -- a subsequent read sees a hole, not stale data. This is the +// only exercise of delete_lba_range against a real index anywhere in the CRAFT test suite. +TEST_F(CraftCommitHsTest, AllZerosUnmapReclaimsRealBlock) { + constexpr lba_t k_lba = 20; + sisl::io_blob_safe content{k_page_size, 512}; + std::memset(content.bytes(), 0xEE, content.size()); + ASSERT_TRUE(do_write(/* dlsn = */ 0, k_lba, /* nlbas = */ 1, content, /* commit_lsn = */ 0).has_value()); + ASSERT_TRUE(do_write_zeros(/* dlsn = */ 1, k_lba, /* nlbas = */ 1, /* commit_lsn = */ 1).has_value()); + + sisl::io_blob_safe dest{k_page_size, 512}; + auto r = do_read(/* read_lsn = */ 1, k_lba, /* nlbas = */ 1, dest); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); + EXPECT_TRUE(r->extents[0].hole); + EXPECT_TRUE(all_zero(dest)); +} + +// A multi-LBA write/commit/read round-trips distinct per-LBA content correctly through the real +// checksum-verified read path (not just single-LBA mock block numbers). +TEST_F(CraftCommitHsTest, MultiLbaWriteCommitReadRoundTrip) { + constexpr lba_t k_lba = 30; + constexpr uint32_t k_nlbas = 3; + sisl::io_blob_safe content{k_nlbas * k_page_size, 512}; + for (uint32_t i = 0; i < k_nlbas; ++i) + std::memset(content.bytes() + i * k_page_size, int(i + 1), k_page_size); + ASSERT_TRUE(do_write(/* dlsn = */ 0, k_lba, k_nlbas, content, /* commit_lsn = */ 0).has_value()); + + sisl::io_blob_safe dest{k_nlbas * k_page_size, 512}; + auto r = do_read(/* read_lsn = */ 0, k_lba, k_nlbas, dest); + ASSERT_TRUE(r.has_value()); + ASSERT_EQ(r->extents.size(), 1u); + EXPECT_FALSE(r->extents[0].hole); + EXPECT_EQ(r->extents[0].len, k_nlbas * k_page_size); + EXPECT_EQ(std::memcmp(dest.cbytes(), content.cbytes(), content.size()), 0); +} + +// The end-to-end visibility transition via the real, public keep_alive() entry point (not just +// do_commit()-style test seams): before keep_alive() advances commit_lsn, a write is only readable +// via the overlay; after, the SAME content is served from the real index instead. A second write to +// the SAME LBA above the new commit point (still overlay-only) makes the two sources definitively +// distinguishable by content: a read below it must see the FIRST write's content (proving it came +// from the index, since the overlay's current entry for this LBA is the second write, not the +// first) while a read at-or-above it must see the SECOND write's (still overlay-only) content. +TEST_F(CraftCommitHsTest, KeepAliveAdvancesCommitThenReadServesIndex) { + constexpr lba_t k_lba = 100; + sisl::io_blob_safe content{k_page_size, 512}; + std::memset(content.bytes(), 0xF0, content.size()); + ASSERT_TRUE(do_write(/* dlsn = */ 0, k_lba, /* nlbas = */ 1, content, /* commit_lsn = */ -1).has_value()); + + sisl::io_blob_safe dest{k_page_size, 512}; + auto r1 = do_read(/* read_lsn = */ 0, k_lba, /* nlbas = */ 1, dest); + ASSERT_TRUE(r1.has_value()); + EXPECT_FALSE(r1->extents[0].hole); + EXPECT_EQ(std::memcmp(dest.cbytes(), content.cbytes(), content.size()), 0); + + auto ka = homeblocks::detail::sync_get(dev_->keep_alive(craft::client_hdr{0, /* commit_lsn = */ 0, -1})); + ASSERT_TRUE(ka.has_value()); + EXPECT_EQ(ka->commit_lsn, 0); + + // Second write to the SAME LBA, still uncommitted (piggybacked commit_lsn stays at 0) -- this + // becomes the overlay's only entry for k_lba, distinct in content from the first write. + sisl::io_blob_safe content2{k_page_size, 512}; + std::memset(content2.bytes(), 0x3C, content2.size()); + ASSERT_TRUE(do_write(/* dlsn = */ 1, k_lba, /* nlbas = */ 1, content2, /* commit_lsn = */ 0).has_value()); + + // A read at read_lsn=0 is below the second write's dlsn=1, so the horizon clamp holds the + // overlay's (now content2) entry back -- this MUST come from the index, and MUST be content1. + auto r2 = do_read(/* read_lsn = */ 0, k_lba, /* nlbas = */ 1, dest); + ASSERT_TRUE(r2.has_value()); + EXPECT_FALSE(r2->extents[0].hole); + EXPECT_EQ(std::memcmp(dest.cbytes(), content.cbytes(), content.size()), 0) + << "read_lsn=0 must be served from the index (content1), proving index-sourcing"; + + // A read at read_lsn=1 is within the second write's horizon -- served from the overlay, content2. + auto r3 = do_read(/* read_lsn = */ 1, k_lba, /* nlbas = */ 1, dest); + ASSERT_TRUE(r3.has_value()); + EXPECT_FALSE(r3->extents[0].hole); + EXPECT_EQ(std::memcmp(dest.cbytes(), content2.cbytes(), content2.size()), 0) + << "read_lsn=1 must be served from the overlay (content2)"; +} + +// N real threads concurrently call write() on the SAME CraftReplDev instance, each to its own +// disjoint LBA (matching how multiple concurrent client write RPCs for the same partition would +// arrive in production before CraftReplDev's own locking serializes the parts that need it) -- +// proving no write is lost or corrupted under genuine concurrent access to the real backend/index. +TEST_F(CraftCommitHsTest, ConcurrentWritesToDisjointLbaRangesAllLand) { + constexpr lba_t k_base_lba = 200; + constexpr int kThreads = 4; + constexpr int kPerThread = 5; + + std::vector< std::thread > threads; + for (int t = 0; t < kThreads; ++t) { + threads.emplace_back([this, t]() { + for (int k = 0; k < kPerThread; ++k) { + lba_t const lba = k_base_lba + t * kPerThread + k; + int64_t const dlsn = static_cast< int64_t >(t * kPerThread + k); // globally unique + sisl::io_blob_safe content{k_page_size, 512}; + std::memset(content.bytes(), t + 1, content.size()); + auto r = do_write(dlsn, lba, /* nlbas = */ 1, content, /* commit_lsn = */ dlsn); + EXPECT_TRUE(r.has_value()) << "thread=" << t << " k=" << k; + } + }); + } + for (auto& th : threads) + th.join(); + + // read_lsn far beyond every dlsn so every entry -- committed or still overlay-only, depending on + // how far each write's own piggybacked commit happened to stall -- is within horizon. + for (int t = 0; t < kThreads; ++t) { + for (int k = 0; k < kPerThread; ++k) { + lba_t const lba = k_base_lba + t * kPerThread + k; + sisl::io_blob_safe dest{k_page_size, 512}; + auto r = do_read(kThreads * kPerThread, lba, /* nlbas = */ 1, dest); + ASSERT_TRUE(r.has_value()) << "lba=" << lba; + ASSERT_EQ(r->extents.size(), 1u) << "lba=" << lba; + EXPECT_FALSE(r->extents[0].hole) << "lba=" << lba; + std::vector< uint8_t > expected(k_page_size, static_cast< uint8_t >(t + 1)); + EXPECT_EQ(std::memcmp(dest.cbytes(), expected.data(), expected.size()), 0) << "lba=" << lba; + } + } +} + +// N real threads all call keep_alive() targeting the same commit_lsn concurrently against the real +// index -- commit_running_ must serialize them so the apply loop runs to completion exactly once +// (from whichever thread wins), with every slot correctly committed and readable afterward. +TEST_F(CraftCommitHsTest, ConcurrentKeepAliveSerializesCommitAgainstRealIndex) { + constexpr lba_t k_base_lba = 300; + constexpr int kSlots = 20; + + for (int i = 0; i < kSlots; ++i) { + sisl::io_blob_safe content{k_page_size, 512}; + std::memset(content.bytes(), i + 1, content.size()); + ASSERT_TRUE( + do_write(/* dlsn = */ i, k_base_lba + i, /* nlbas = */ 1, content, /* commit_lsn = */ -1).has_value()); + } + + constexpr int kThreads = 6; + std::vector< std::thread > threads; + for (int t = 0; t < kThreads; ++t) { + threads.emplace_back([this, kSlots]() { + auto r = + homeblocks::detail::sync_get(dev_->keep_alive(craft::client_hdr{0, /* commit_lsn = */ kSlots - 1, -1})); + EXPECT_TRUE(r.has_value()); + }); + } + for (auto& th : threads) + th.join(); + + EXPECT_EQ(dev_->commit_lsn(), kSlots - 1); + + for (int i = 0; i < kSlots; ++i) { + sisl::io_blob_safe dest{k_page_size, 512}; + auto r = do_read(kSlots - 1, k_base_lba + i, /* nlbas = */ 1, dest); + ASSERT_TRUE(r.has_value()) << "i=" << i; + EXPECT_FALSE(r->extents[0].hole) << "i=" << i; + std::vector< uint8_t > expected(k_page_size, static_cast< uint8_t >(i + 1)); + EXPECT_EQ(std::memcmp(dest.cbytes(), expected.data(), expected.size()), 0) << "i=" << i; + } +} + +int main(int argc, char* argv[]) { + int parsed_argc = argc; + char** orig_argv = argv; + std::vector< std::string > args; + for (int i = 0; i < argc; ++i) { + args.emplace_back(argv[i]); + } + + ::testing::InitGoogleTest(&parsed_argc, argv); + SISL_OPTIONS_LOAD(parsed_argc, argv, logging, test_common_setup); + spdlog::set_pattern("[%D %T%z] [%^%l%$] [%n] [%t] %v"); + + g_helper = std::make_unique< test_common::HBTestHelper >("test_craft_commit_hs", args, orig_argv); + g_helper->setup(); + auto ret = RUN_ALL_TESTS(); + g_helper->teardown(); + return ret; +} diff --git a/src/lib/craft/tests/test_craft_concurrency.cpp b/src/lib/craft/tests/test_craft_concurrency.cpp new file mode 100644 index 0000000..f3c2e11 --- /dev/null +++ b/src/lib/craft/tests/test_craft_concurrency.cpp @@ -0,0 +1,531 @@ +/********************************************************************************* + * 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. + * + *********************************************************************************/ + +// Real multi-threaded tests for CraftReplDev's own internal locking (missing_mu_, overlay_mu_, +// commit_running_) -- every other CRAFT test drives calls from a single thread via sync_get(), so +// none of them ever exercise genuine concurrent contention on these locks. +// +// This stays light (no HomeStore/iomgr): MockCraftJournalBackend's coroutine bodies never suspend +// across a real async boundary (co_return only), so stdexec::sync_wait (sync_get) runs the whole +// commit_impl()/write() coroutine synchronously on WHICHEVER OS thread calls it -- real parallel +// std::thread callers therefore drive genuinely concurrent execution of CraftReplDev's own code, +// with no iomgr reactor required. The mock/fake below are made thread-safe purely as test +// scaffolding (so a real locking bug in CraftReplDev surfaces as a wrong count/state rather than a +// crash inside the test double itself); CraftReplDev's own locks are what's actually under test. +// +// 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 +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +#include "craft/craft_repl_dev.hpp" +#include "home_blks_config.hpp" +#include "coro_helpers.hpp" + +SISL_LOGGING_DEF(HOMEBLOCKS_LOG_MODS) +SISL_OPTIONS_ENABLE(logging) +SISL_LOGGING_INIT(HOMEBLOCKS_LOG_MODS) + +namespace homeblocks { +namespace { + +static constexpr uint32_t k_page_size = 4096; + +// ── thread-safe journal mock ──────────────────────────────────────────────────── +// +// Same shape as test_craft_commit.cpp's MockCraftJournalBackend, but every access to shared state is +// guarded -- multiple real threads call alloc_write_data/write_slot/read_data concurrently here. + +class MockCraftJournalBackend : public CraftJournalBackend { +public: + std::mutex mu; + std::map< int64_t, JournalSlot > slots; + homestore::blk_num_t next_blk_num{1000}; + std::map< homestore::blk_num_t, std::vector< uint8_t > > block_data; + + async_result< homestore::multi_blk_id > alloc_write_data(sisl::sg_list const& data, lba_count_t len) override { + auto nlbas = static_cast< homestore::blk_count_t >(len / k_page_size); + auto const* buf = static_cast< uint8_t const* >(data.iovs[0].iov_base); + std::lock_guard lk{mu}; + homestore::multi_blk_id blkid{next_blk_num, nlbas, /* chunk = */ 1}; + for (homestore::blk_count_t i = 0; i < nlbas; ++i) + block_data[next_blk_num + i] = std::vector< uint8_t >(buf + i * k_page_size, buf + (i + 1) * k_page_size); + next_blk_num += nlbas; + co_return blkid; + } + + 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, std::vector< homestore::csum_t > const& csums) override { + std::lock_guard lk{mu}; + slots[lsn] = JournalSlot{ + .lsn = lsn, .all_zeros = all_zeros, .lba_off_bytes = lba, .len_bytes = len, .blkid = blkid, .csums = csums}; + co_return ok(); + } + + async_result< JournalSlot > read_slot(int64_t lsn) override { + std::lock_guard lk{mu}; + 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(); } + + async_status read_data(homestore::multi_blk_id blkid, sisl::sg_list& dest) override { + auto* buf = static_cast< uint8_t* >(dest.iovs[0].iov_base); + size_t offset = 0; + std::lock_guard lk{mu}; + auto pieces = blkid.iterate(); + while (auto piece = pieces.next()) { + for (homestore::blk_count_t i = 0; i < piece->blk_count(); ++i) { + auto it = block_data.find(piece->blk_num() + i); + if (it == block_data.end()) + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + std::memcpy(buf + offset, it->second.data(), it->second.size()); + offset += it->second.size(); + } + } + co_return ok(); + } + + void add_data_slot(int64_t lsn, lba_t lba, uint32_t nlbas, homestore::blk_num_t blk_num, + std::vector< homestore::csum_t > csums) { + homestore::multi_blk_id blkid{blk_num, static_cast< homestore::blk_count_t >(nlbas), /* chunk = */ 1}; + std::lock_guard lk{mu}; + slots[lsn] = JournalSlot{.lsn = lsn, + .lba_off_bytes = lba * k_page_size, + .len_bytes = nlbas * k_page_size, + .blkid = blkid, + .csums = std::move(csums)}; + } +}; + +// ── thread-safe fake index ────────────────────────────────────────────────────── + +class FakeIndex { +public: + std::mutex mu; + std::map< lba_t, BlockInfo > entries; + + status write_to_index(lba_t start_lba, lba_t end_lba, std::unordered_map< lba_t, BlockInfo >& blocks_info) { + std::lock_guard lk{mu}; + for (auto lba = start_lba; lba <= end_lba; ++lba) { + auto& info = blocks_info[lba]; + if (auto it = entries.find(lba); it != entries.end()) info.old_blkid = it->second.new_blkid; + entries[lba] = BlockInfo{info.new_blkid, homestore::blk_id{}, info.new_checksum}; + } + return ok(); + } + + status delete_lba_range(lba_t start_lba, lba_t end_lba, std::vector< homestore::blk_id >& out_freed_blkids) { + std::lock_guard lk{mu}; + for (auto lba = start_lba; lba <= end_lba; ++lba) { + auto it = entries.find(lba); + if (it == entries.end()) continue; + out_freed_blkids.push_back(it->second.new_blkid); + entries.erase(it); + } + return ok(); + } + + status read_from_index(lba_t start_lba, lba_t end_lba, + std::vector< std::pair< VolumeIndexKey, VolumeIndexValue > >& out) { + std::lock_guard lk{mu}; + for (auto lba = start_lba; lba <= end_lba; ++lba) { + auto it = entries.find(lba); + if (it == entries.end()) continue; + out.emplace_back(VolumeIndexKey{lba}, VolumeIndexValue{it->second.new_blkid, it->second.new_checksum}); + } + return ok(); + } +}; + +// ── test fixture ───────────────────────────────────────────────────────────── + +class CraftConcurrencyTest : 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), k_page_size, nullptr); + } + + auto do_write_data(int64_t dlsn, lba_t lba, uint8_t fill) { + std::vector< uint8_t > buf(k_page_size, fill); + sisl::sg_list data; + data.size = buf.size(); + data.iovs.push_back(iovec{buf.data(), buf.size()}); + return homeblocks::detail::sync_get( + dev_->write(craft::client_hdr{0, -1, -1}, dlsn, lba * k_page_size, k_page_size, std::move(data))); + } + + auto do_write_zeros(int64_t dlsn, lba_t lba) { + sisl::sg_list empty_data{}; + return homeblocks::detail::sync_get( + dev_->write(craft::client_hdr{0, -1, -1}, dlsn, lba * k_page_size, k_page_size, std::move(empty_data))); + } + + auto do_read(int64_t read_lsn, lba_t lba, std::vector< uint8_t >& dest_buf) { + dest_buf.assign(k_page_size, 0xFF); + sisl::sg_list dest; + dest.size = dest_buf.size(); + dest.iovs.push_back(iovec{dest_buf.data(), dest_buf.size()}); + // read_with() (not the public read()): dev_ is constructed with indx_tbl_=nullptr in this + // light fixture, and read() itself would unconditionally reject with not_supported before + // ever reaching read_impl -- matching test_craft_commit.cpp's do_read() pattern. + return homeblocks::detail::sync_get(dev_->read_with( + read_lsn, lba * k_page_size, k_page_size, std::move(dest), + [this](lba_t s, lba_t e, std::vector< std::pair< VolumeIndexKey, VolumeIndexValue > >& out) { + return index_.read_from_index(s, e, out); + })); + } + + MockCraftJournalBackend* journal_{nullptr}; + FakeIndex index_; + std::unique_ptr< CraftReplDev > dev_; +}; + +// N real threads each write a disjoint subset of a shared dLSN/LBA space concurrently (thread t +// handles every dlsn where dlsn % kThreads == t, submitted in increasing order per thread but with +// arrival order across threads left to the OS scheduler). After joining, missing_lsns_ must be fully +// drained and every LBA's overlay entry must reflect its own dlsn -- proving missing_lsns_/ +// last_append_lsn/overlay_ stay consistent under real concurrent write() calls, not just the +// single-threaded out-of-order sequencing every other write test exercises. +TEST_F(CraftConcurrencyTest, ConcurrentDisjointWritesConverge) { + constexpr int kThreads = 8; + constexpr int kPerThread = 60; + constexpr int kTotal = kThreads * kPerThread; + + std::vector< std::thread > threads; + for (int t = 0; t < kThreads; ++t) { + threads.emplace_back([this, t]() { + for (int k = 0; k < kPerThread; ++k) { + int64_t dlsn = static_cast< int64_t >(k * kThreads + t); + auto r = do_write_data(dlsn, /* lba = */ static_cast< lba_t >(dlsn), /* fill = */ 0xAB); + EXPECT_TRUE(r.has_value()) << "dlsn=" << dlsn; + } + }); + } + for (auto& th : threads) + th.join(); + + EXPECT_EQ(dev_->missing_count(), 0u); + EXPECT_EQ(dev_->last_append_lsn(), kTotal - 1); + for (int i = 0; i < kTotal; ++i) { + EXPECT_EQ(dev_->overlay_lsn_for(static_cast< lba_t >(i)), i) << "lba=" << i; + } +} + +// N real threads all call write() for the exact SAME dlsn/LBA concurrently (a malformed/retried +// request -- a well-behaved client never does this for one dlsn, but a buggy or adversarial one +// might). Without in_flight_write_dlsns_' dedup guard, every thread would pass the idempotency check +// before any of them advance last_append_lsn, and every thread would proceed to alloc_write_data -- +// doubly (N-ly) allocating real blocks for one dlsn, with only whichever write_slot call lands last +// ever referenced by the journal and the rest silently leaked. Proves: exactly one thread's write +// succeeds, every other thread is rejected with operation_in_progress (never silently duplicated), +// and MockCraftJournalBackend's block allocator only ever advances by ONE write's worth of blocks. +TEST_F(CraftConcurrencyTest, ConcurrentSameDlsnWritesNoDoubleAllocation) { + constexpr int64_t kDlsn = 42; + constexpr lba_t kLba = 7; + constexpr int kThreads = 12; + + homestore::blk_num_t const next_blk_before = journal_->next_blk_num; + + std::atomic< int > succeeded{0}; + std::atomic< int > rejected_in_progress{0}; + std::atomic< int > unexpected_errors{0}; + std::vector< std::thread > threads; + for (int t = 0; t < kThreads; ++t) { + threads.emplace_back([this, t, &succeeded, &rejected_in_progress, &unexpected_errors]() { + auto r = do_write_data(kDlsn, kLba, static_cast< uint8_t >(t + 1)); + if (r.has_value()) { + ++succeeded; + } else if (r.error() == std::errc::operation_in_progress) { + ++rejected_in_progress; + } else { + ++unexpected_errors; + } + }); + } + for (auto& th : threads) + th.join(); + + EXPECT_EQ(unexpected_errors.load(), 0); + // Every thread races the SAME dlsn: at least one must win (whichever gets there first when + // last_append_lsn is still -1), and every other concurrent attempt must be rejected as in-flight + // rather than silently duplicating the allocation. A thread that happens to run strictly after an + // earlier one has already completed would instead see the idempotent path (also a success) -- + // the real threading (all kThreads launched together, no serialization point before the race + // begins) makes at least one non-idempotent rejection overwhelmingly likely, but this test's + // actual safety property is unexpected_errors==0 and the allocation-count check below. + EXPECT_GE(succeeded.load(), 1); + EXPECT_EQ(succeeded.load() + rejected_in_progress.load(), kThreads); + + // The one true test: exactly one dlsn=42 write's worth of blocks (1 LBA) was ever allocated, + // no matter how many threads raced to write it. + EXPECT_EQ(journal_->next_blk_num - next_blk_before, 1u); + + // The dlsn is fully resolved and locally readable afterward, same as any other successful write. + EXPECT_EQ(dev_->last_append_lsn(), kDlsn); + EXPECT_EQ(dev_->overlay_lsn_for(kLba), kDlsn); +} + +// One thread continuously overwrites a single LBA at increasing dLSNs (each write's content tagged +// with a distinct byte value) while another thread concurrently, repeatedly reads that same LBA -- +// proving overlay_mu_'s snapshot-under-lock discipline never lets a reader observe a torn/mixed +// OverlayEntry (which would surface as either non-uniform bytes within one page, or a spurious +// CRC_MISMATCH from a blkid/csum pair that doesn't actually correspond to each other). +TEST_F(CraftConcurrencyTest, ConcurrentWriteAndReadNoTornReads) { + constexpr int kWrites = 500; + std::atomic< bool > stop{false}; + std::atomic< int > torn_reads{0}; + std::atomic< int > read_errors{0}; + + std::thread writer([this]() { + for (int i = 0; i < kWrites; ++i) { + auto r = do_write_data(/* dlsn = */ i, /* lba = */ 0, /* fill = */ static_cast< uint8_t >(i % 256)); + EXPECT_TRUE(r.has_value()) << "dlsn=" << i; + } + }); + + std::thread reader([this, &stop, &torn_reads, &read_errors]() { + std::vector< uint8_t > dest; + while (!stop.load(std::memory_order_relaxed)) { + // read_lsn far ahead of any possible dlsn so every overlay entry is always within horizon. + auto r = do_read(/* read_lsn = */ 1'000'000, /* lba = */ 0, dest); + if (!r.has_value()) { + ++read_errors; + continue; + } + if (r->extents.empty() || r->extents[0].hole) continue; // no write has landed yet + uint8_t const first = dest[0]; + bool const uniform = std::all_of(dest.begin(), dest.end(), [first](uint8_t b) { return b == first; }); + if (!uniform) ++torn_reads; + } + }); + + writer.join(); + stop.store(true, std::memory_order_relaxed); + reader.join(); + + EXPECT_EQ(read_errors.load(), 0); + EXPECT_EQ(torn_reads.load(), 0); +} + +// N real threads all call commit_with() targeting the same upto_lsn concurrently against a shared, +// pre-seeded journal -- commit_running_ (guarded by missing_mu_) must ensure the apply loop's +// write_fn runs for each LBA EXACTLY ONCE across all callers combined, never zero (stall) and never +// twice (double-apply), regardless of which thread actually "wins" the race. write_fn sleeps briefly +// to widen the window so concurrent callers genuinely overlap with the winner's in-flight run rather +// than only ever seeing it already finished. +TEST_F(CraftConcurrencyTest, ConcurrentCommitWithSerializesNoDoubleApply) { + constexpr int kSlots = 40; + dev_->seed_lsns(kSlots - 1, {}); + for (int i = 0; i < kSlots; ++i) + journal_->add_data_slot(i, /* lba = */ static_cast< lba_t >(i), /* nlbas = */ 1, + /* blk_num = */ static_cast< homestore::blk_num_t >(1000 + i), {uint16_t(i)}); + + std::mutex applied_mu; + std::set< lba_t > applied_lbas; + int duplicate_applies = 0; + + auto write_fn = [&](lba_t s, lba_t e, std::unordered_map< lba_t, BlockInfo >& info) -> status { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); // widen the concurrent-caller window + auto r = index_.write_to_index(s, e, info); + std::lock_guard lk{applied_mu}; + for (auto l = s; l <= e; ++l) { + if (!applied_lbas.insert(l).second) ++duplicate_applies; + } + return r; + }; + auto delete_fn = [this](lba_t s, lba_t e, std::vector< homestore::blk_id >& freed) { + return index_.delete_lba_range(s, e, freed); + }; + + constexpr int kThreads = 8; + std::vector< std::thread > threads; + for (int t = 0; t < kThreads; ++t) { + threads.emplace_back([this, &write_fn, &delete_fn]() { + auto r = homeblocks::detail::sync_get(dev_->commit_with(kSlots - 1, write_fn, delete_fn)); + EXPECT_TRUE(r.has_value()); + }); + } + for (auto& th : threads) + th.join(); + + EXPECT_EQ(dev_->commit_lsn(), kSlots - 1); + EXPECT_EQ(duplicate_applies, 0); // no LBA was ever applied by more than one caller + EXPECT_EQ(applied_lbas.size(), static_cast< size_t >(kSlots)); // every LBA was applied by exactly one +} + +// ── comprehensive mixed-workload stress ─────────────────────────────────────── +// +// A wholistic stress test combining every operation CraftReplDev allows to run concurrently in +// production (write, commit/keep_alive-equivalent, read -- NOT truncate(), which is documented as +// login-only/quiesced and would violate its own precondition if raced against concurrent writes) -- +// all firing simultaneously from many real threads for a sustained run, against a single shared +// instance. Every one of missing_mu_, overlay_mu_, and commit_running_ is under real contention at +// once, not in isolation the way the more focused tests above exercise them one at a time. +// +// Correctness is checked two ways: (1) no operation anywhere in the run returns an unexpected error +// or produces a torn/non-uniform read, and (2) the FINAL state is verified against a ground truth +// computed purely arithmetically from the (deterministic) work assignment -- independent of real-time +// execution order, because write()'s highest-dLSN-wins rule is defined by dlsn VALUE, not arrival +// time. This lets the test assert an exact expected outcome despite the interleaving being +// intentionally chaotic. +TEST_F(CraftConcurrencyTest, ComprehensiveMixedWorkloadStress) { + constexpr int kNumLbas = 30; + constexpr int64_t kTotalDlsn = 1500; + constexpr int kWriterThreads = 12; + constexpr int kReaderThreads = 4; + constexpr int kCommitterThreads = 3; + + // Deterministic work assignment: dlsn -> lba, and whether it's an all_zeros (unmap) write. + auto lba_for = [](int64_t dlsn) { return static_cast< lba_t >(dlsn % kNumLbas); }; + auto is_zero_write = [](int64_t dlsn) { return dlsn % 7 == 0; }; + auto fill_for = [](int64_t dlsn) { return static_cast< uint8_t >(dlsn % 256); }; + + // Ground truth: for each lba, the winning dlsn is whichever assigned dlsn is numerically + // highest (write()'s overlay/index apply order is entirely determined by dlsn value, never by + // real-time arrival), computed here with no dependence on how the threads below actually + // interleave. + std::vector< int64_t > winner_dlsn(kNumLbas, -1); + for (int64_t d = 0; d < kTotalDlsn; ++d) { + auto l = lba_for(d); + if (winner_dlsn[l] < d) winner_dlsn[l] = d; + } + + // Deliberately NOT seeding last_append_lsn ahead of these writes: doing so would make write()'s + // own idempotent short-circuit (dlsn <= last_append_lsn && not missing) treat every dlsn as + // "already written" and skip real journaling/overlay population entirely -- the same mistake + // fixed in TruncateRemovesOverlayEntriesAboveLsn. Starting from the default -1 lets every write + // advance last_append_lsn for real. + std::atomic< bool > writers_done{false}; + std::atomic< int > unexpected_errors{0}; + std::atomic< int > torn_reads{0}; + + // Writers: thread t handles every dlsn where dlsn % kWriterThreads == t, in increasing order -- + // real-time arrival across threads is left entirely to the OS scheduler. + std::vector< std::thread > writers; + for (int t = 0; t < kWriterThreads; ++t) { + writers.emplace_back([this, t, &lba_for, &is_zero_write, &fill_for, &unexpected_errors]() { + for (int64_t d = t; d < kTotalDlsn; d += kWriterThreads) { + auto r = is_zero_write(d) ? do_write_zeros(d, lba_for(d)) : do_write_data(d, lba_for(d), fill_for(d)); + if (!r.has_value()) ++unexpected_errors; + } + }); + } + + // Committers: repeatedly drive commit_with() toward the final target while writers are still in + // flight -- commit_running_ must serialize these against each other AND against whichever + // writers are concurrently populating the overlay for lsns not yet applied. + auto write_fn = [this](lba_t s, lba_t e, std::unordered_map< lba_t, BlockInfo >& info) { + return index_.write_to_index(s, e, info); + }; + auto delete_fn = [this](lba_t s, lba_t e, std::vector< homestore::blk_id >& freed) { + return index_.delete_lba_range(s, e, freed); + }; + std::vector< std::thread > committers; + for (int c = 0; c < kCommitterThreads; ++c) { + committers.emplace_back([this, &writers_done, &unexpected_errors, &write_fn, &delete_fn]() { + while (!writers_done.load(std::memory_order_relaxed)) { + auto r = homeblocks::detail::sync_get(dev_->commit_with(kTotalDlsn - 1, write_fn, delete_fn)); + if (!r.has_value()) ++unexpected_errors; + } + }); + } + + // Readers: continuously read random LBAs throughout -- read_lsn is far beyond any possible dlsn + // so every overlay entry is always within horizon; only uniformity (no torn read) and absence of + // errors are checked here, not exact content (which the post-join ground-truth check covers). + std::vector< std::thread > readers; + for (int r_idx = 0; r_idx < kReaderThreads; ++r_idx) { + readers.emplace_back([this, r_idx, &writers_done, &unexpected_errors, &torn_reads]() { + std::mt19937 rng(static_cast< uint32_t >(r_idx) + 1); + std::uniform_int_distribution< int > lba_dist(0, kNumLbas - 1); + std::vector< uint8_t > dest; + while (!writers_done.load(std::memory_order_relaxed)) { + auto r = do_read(/* read_lsn = */ kTotalDlsn * 10, static_cast< lba_t >(lba_dist(rng)), dest); + if (!r.has_value()) { + ++unexpected_errors; + continue; + } + if (r->extents.empty() || r->extents[0].hole) continue; + uint8_t const first = dest[0]; + bool const uniform = std::all_of(dest.begin(), dest.end(), [first](uint8_t b) { return b == first; }); + if (!uniform) ++torn_reads; + } + }); + } + + for (auto& th : writers) + th.join(); + writers_done.store(true, std::memory_order_relaxed); + for (auto& th : committers) + th.join(); + for (auto& th : readers) + th.join(); + + ASSERT_EQ(unexpected_errors.load(), 0); + ASSERT_EQ(torn_reads.load(), 0); + + // Drain: every dlsn in [0, kTotalDlsn) was assigned to exactly one writer thread, so + // missing_lsns_ must now be fully empty -- this must reach the target with no stall. + auto final_commit = homeblocks::detail::sync_get(dev_->commit_with(kTotalDlsn - 1, write_fn, delete_fn)); + ASSERT_TRUE(final_commit.has_value()); + EXPECT_EQ(dev_->missing_count(), 0u); + EXPECT_EQ(dev_->commit_lsn(), kTotalDlsn - 1); + + // Ground-truth verification: every LBA's final index state must reflect its winning dlsn -- + // absent (unmapped) if the winner was an all_zeros write, or the winner's exact content + // checksum otherwise -- and its overlay entry must be fully retired (everything committed). + for (int l = 0; l < kNumLbas; ++l) { + EXPECT_EQ(dev_->overlay_lsn_for(static_cast< lba_t >(l)), -1) << "lba=" << l; + if (is_zero_write(winner_dlsn[l])) { + EXPECT_FALSE(index_.entries.count(static_cast< lba_t >(l))) << "lba=" << l << " should be unmapped"; + } else { + ASSERT_TRUE(index_.entries.count(static_cast< lba_t >(l))) << "lba=" << l; + std::vector< uint8_t > expected(k_page_size, fill_for(winner_dlsn[l])); + auto expected_csum = crc16_t10dif(0x8005, expected.data(), expected.size()); + EXPECT_EQ(index_.entries[static_cast< lba_t >(l)].new_checksum, expected_csum) << "lba=" << l; + } + } +} + +} // namespace +} // namespace homeblocks + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + SISL_OPTIONS_LOAD(argc, argv, logging); + HB_SETTINGS_FACTORY().load_json("{\"craft_watchdog_timeout_ms\": 0}"); + return RUN_ALL_TESTS(); +} diff --git a/src/lib/craft/tests/test_craft_homestore_backend.cpp b/src/lib/craft/tests/test_craft_homestore_backend.cpp index 07adf3c..d1c1e61 100644 --- a/src/lib/craft/tests/test_craft_homestore_backend.cpp +++ b/src/lib/craft/tests/test_craft_homestore_backend.cpp @@ -29,8 +29,10 @@ #include #include +#include #include #include +#include #include "hb_internal.hpp" #include "craft/craft_repl_dev.hpp" @@ -45,6 +47,30 @@ std::unique_ptr< test_common::HBTestHelper > g_helper; using namespace homeblocks; +static constexpr uint32_t k_page_size = 4096; + +// Writes a raw blob directly to the log store, bypassing HomeStoreCraftJournalBackend::write_slot's +// own encoding -- used to simulate a corrupted/malformed on-disk record for read_slot's validate- +// before-trust tests. Mirrors write_slot's OWN write_async/value_awaitable completion bridge rather +// than using home_log_store::write_and_flush(): write_and_flush leaves the logdev's completion +// bookkeeping inconsistent with what a graceful homestore shutdown expects, hanging teardown +// indefinitely (confirmed by bisection -- every test using it hangs in isolation; every test using +// this bridge, or write_slot itself, does not). +void write_raw_blob(shared< homestore::home_log_store > const& logstore, int64_t lsn, sisl::io_blob_safe const& blob) { + auto va = std::make_shared< sisl::async::value_awaitable< bool > >(); + auto write_ret = logstore->write_async( + static_cast< homestore::logstore_seq_num_t >(lsn), blob, nullptr, + [va](homestore::logstore_seq_num_t, sisl::io_blob&, homestore::logdev_key, void*) mutable { + iomanager.run_on_forget(iomgr::reactor_regex::least_busy_io, + [va = std::move(va)]() mutable { va->complete(true); }); + }); + ASSERT_GE(write_ret, 0); + homeblocks::detail::sync_get([va]() -> homestore::async_status { + co_await *va; + co_return homestore::ok(); + }()); +} + class CraftHomeStoreBackendTest : public ::testing::Test { protected: // Each test gets its own logdev/log_store so writes/rollbacks in one test cannot affect another. @@ -63,11 +89,11 @@ class CraftHomeStoreBackendTest : public ::testing::Test { TEST_F(CraftHomeStoreBackendTest, WriteSlotCompletesInlineWithoutHanging) { auto logstore = make_logstore(); ASSERT_TRUE(logstore != nullptr); - auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0); + auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0, k_page_size); - auto r = - homeblocks::detail::sync_get(backend->write_slot(/* lsn = */ 0, /* term = */ 1, /* lba = */ 0, /* len = */ 4096, - homestore::multi_blk_id{}, /* all_zeros = */ true)); + auto r = homeblocks::detail::sync_get( + backend->write_slot(/* lsn = */ 0, /* term = */ 1, /* lba = */ 0, /* len = */ 4096, homestore::multi_blk_id{}, + /* all_zeros = */ true, std::vector< homestore::csum_t >{})); ASSERT_TRUE(r.has_value()); } @@ -76,11 +102,12 @@ TEST_F(CraftHomeStoreBackendTest, WriteSlotCompletesInlineWithoutHanging) { TEST_F(CraftHomeStoreBackendTest, TruncateToRollsBackRealLogStore) { auto logstore = make_logstore(); ASSERT_TRUE(logstore != nullptr); - auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0); + auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0, k_page_size); for (int64_t lsn = 0; lsn <= 4; ++lsn) { auto r = homeblocks::detail::sync_get(backend->write_slot(lsn, /* term = */ 1, /* lba = */ 0, /* len = */ 4096, - homestore::multi_blk_id{}, /* all_zeros = */ true)); + homestore::multi_blk_id{}, /* all_zeros = */ true, + std::vector< homestore::csum_t >{})); ASSERT_TRUE(r.has_value()); } ASSERT_EQ(logstore->tail_lsn(), 4); @@ -90,6 +117,123 @@ TEST_F(CraftHomeStoreBackendTest, TruncateToRollsBackRealLogStore) { EXPECT_EQ(logstore->tail_lsn(), 2); } +// read_slot must parse back exactly what write_slot wrote: header fields, the csum array, and the blkid. +TEST_F(CraftHomeStoreBackendTest, ReadSlotReturnsCorrectData) { + auto logstore = make_logstore(); + ASSERT_TRUE(logstore != nullptr); + auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0, k_page_size); + + homestore::multi_blk_id blkid{/* blk_num = */ 42, /* nblks = */ 2, /* chunk_num = */ 7}; + std::vector< homestore::csum_t > csums{111, 222}; + + auto w = homeblocks::detail::sync_get(backend->write_slot(/* lsn = */ 3, /* term = */ 5, /* lba = */ 100, + /* len = */ 2 * k_page_size, blkid, + /* all_zeros = */ false, csums)); + ASSERT_TRUE(w.has_value()); + + auto r = homeblocks::detail::sync_get(backend->read_slot(3)); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(r->lsn, 3); + EXPECT_FALSE(r->all_zeros); + EXPECT_EQ(r->lba_off_bytes, 100u); + EXPECT_EQ(r->len_bytes, 2 * k_page_size); + EXPECT_EQ(r->csums, csums); + EXPECT_TRUE(r->blkid == blkid); +} + +// A never-appended lsn must fail cleanly (the log store throws std::out_of_range internally). +TEST_F(CraftHomeStoreBackendTest, ReadSlotUnknownLsnFails) { + auto logstore = make_logstore(); + ASSERT_TRUE(logstore != nullptr); + auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0, k_page_size); + + auto r = homeblocks::detail::sync_get(backend->read_slot(99)); + ASSERT_FALSE(r.has_value()); +} + +// read_slot must round-trip an all_zeros slot correctly: nlbas=0 (empty csum array), and the +// still-present (empty) serialized multi_blk_id region. +TEST_F(CraftHomeStoreBackendTest, ReadSlotRoundTripsAllZerosSlot) { + auto logstore = make_logstore(); + ASSERT_TRUE(logstore != nullptr); + auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0, k_page_size); + + auto w = + homeblocks::detail::sync_get(backend->write_slot(/* lsn = */ 0, /* term = */ 1, /* lba = */ 200, + /* len = */ 2 * k_page_size, homestore::multi_blk_id{}, + /* all_zeros = */ true, std::vector< homestore::csum_t >{})); + ASSERT_TRUE(w.has_value()); + + auto r = homeblocks::detail::sync_get(backend->read_slot(0)); + ASSERT_TRUE(r.has_value()); + EXPECT_TRUE(r->all_zeros); + EXPECT_EQ(r->lba_off_bytes, 200u); + EXPECT_EQ(r->len_bytes, 2 * k_page_size); + EXPECT_TRUE(r->csums.empty()); +} + +// A blob too small for even the fixed 34-byte header must fail cleanly, not crash or misread +// garbage as a header. +TEST_F(CraftHomeStoreBackendTest, ReadSlotTooSmallForHeaderFails) { + auto logstore = make_logstore(); + ASSERT_TRUE(logstore != nullptr); + auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0, k_page_size); + + sisl::io_blob_safe tiny{4}; + std::memset(tiny.bytes(), 0, tiny.size()); + write_raw_blob(logstore, 0, tiny); + + auto r = homeblocks::detail::sync_get(backend->read_slot(0)); + ASSERT_FALSE(r.has_value()); +} + +// A record with a correctly-sized blob but a stomped magic must be rejected rather than trusted. +TEST_F(CraftHomeStoreBackendTest, ReadSlotBadMagicFails) { + auto logstore = make_logstore(); + ASSERT_TRUE(logstore != nullptr); + auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0, k_page_size); + + // Write a real all_zeros slot to get a correctly-sized, otherwise-valid blob. + auto w = + homeblocks::detail::sync_get(backend->write_slot(0, 1, 0, k_page_size, homestore::multi_blk_id{}, + /* all_zeros = */ true, std::vector< homestore::csum_t >{})); + ASSERT_TRUE(w.has_value()); + + // Corrupt the leading bytes (the on-disk magic field starts at offset 0) and rewrite at a + // fresh lsn -- read_slot must reject it rather than trust a garbage header. + auto raw = logstore->read_sync(0); + sisl::io_blob_safe corrupted{raw.size()}; + std::memcpy(corrupted.bytes(), raw.bytes(), raw.size()); + std::memset(corrupted.bytes(), 0xFF, 4); + write_raw_blob(logstore, 1, corrupted); + + auto r = homeblocks::detail::sync_get(backend->read_slot(1)); + ASSERT_FALSE(r.has_value()); +} + +// A truncated blob (real header/magic, but cut short of even the fixed header size) must be +// rejected rather than read past the end of the buffer. +TEST_F(CraftHomeStoreBackendTest, ReadSlotTruncatedBlobFails) { + auto logstore = make_logstore(); + ASSERT_TRUE(logstore != nullptr); + auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0, k_page_size); + + homestore::multi_blk_id blkid{42, 2, 7}; + std::vector< homestore::csum_t > csums{111, 222}; + auto w = homeblocks::detail::sync_get( + backend->write_slot(0, 1, 0, 2 * k_page_size, blkid, /* all_zeros = */ false, csums)); + ASSERT_TRUE(w.has_value()); + + auto raw = logstore->read_sync(0); + ASSERT_GT(raw.size(), 10u); + sisl::io_blob_safe truncated{10}; // smaller than even the fixed 34-byte header + std::memcpy(truncated.bytes(), raw.bytes(), 10); + write_raw_blob(logstore, 1, truncated); + + auto r = homeblocks::detail::sync_get(backend->read_slot(1)); + ASSERT_FALSE(r.has_value()); +} + // alloc_write_data's application_hint routes allocation through VolumeChunkSelector by vol_ordinal. // No volume was created in this test (make_homestore_journal_backend has no production call site // yet, so there is no real ordinal to allocate against), so this hits VolumeChunkSelector with an @@ -99,7 +243,7 @@ TEST_F(CraftHomeStoreBackendTest, TruncateToRollsBackRealLogStore) { TEST_F(CraftHomeStoreBackendTest, AllocWriteDataFailsCleanlyForUnregisteredOrdinal) { auto logstore = make_logstore(); ASSERT_TRUE(logstore != nullptr); - auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0); + auto backend = make_homestore_journal_backend(logstore, /* vol_ordinal = */ 0, k_page_size); constexpr uint32_t k_len = 4096; sisl::io_blob_safe buf{k_len, 512}; diff --git a/src/lib/craft/tests/test_craft_journal_slot_wire.cpp b/src/lib/craft/tests/test_craft_journal_slot_wire.cpp index 9ee47f6..98e540e 100644 --- a/src/lib/craft/tests/test_craft_journal_slot_wire.cpp +++ b/src/lib/craft/tests/test_craft_journal_slot_wire.cpp @@ -57,9 +57,10 @@ static_assert(std::is_same_v< decltype(JournalSlot::is_empty), decltype(craft::J static_assert(std::is_same_v< decltype(JournalSlot::all_zeros), decltype(craft::JournalSlot::all_zeros) >); static_assert(std::is_same_v< decltype(JournalSlot::lba_off_bytes), decltype(craft::JournalSlot::lba) >); static_assert(std::is_same_v< decltype(JournalSlot::len_bytes), decltype(craft::JournalSlot::len) >); -static_assert(sizeof(JournalSlot) == sizeof(craft::JournalSlot), - "homeblocks::JournalSlot must stay layout-compatible with craft::JournalSlot " - "(craft_client's include/craft/peer.hpp)"); +// homeblocks::JournalSlot is intentionally a superset of craft::JournalSlot: it carries two +// additional HomeBlocks-internal fields (blkid, csums) that are parsed from the on-disk blob but +// are never part of the wire format. sizeof equality no longer holds; the per-field is_same_v +// checks above are the effective wire-compat pin for the shared fields. // ── round-trip helpers ──────────────────────────────────────────────────────── diff --git a/src/lib/craft/tests/test_craft_peer_exchange.cpp b/src/lib/craft/tests/test_craft_peer_exchange.cpp index 11598ba..082de9b 100644 --- a/src/lib/craft/tests/test_craft_peer_exchange.cpp +++ b/src/lib/craft/tests/test_craft_peer_exchange.cpp @@ -32,16 +32,21 @@ #include #include #include +#include #include "craft/craft_repl_dev.hpp" +#include "home_blks_config.hpp" #include "coro_helpers.hpp" SISL_LOGGING_DEF(HOMEBLOCKS_LOG_MODS) +SISL_OPTIONS_ENABLE(logging) SISL_LOGGING_INIT(HOMEBLOCKS_LOG_MODS) namespace homeblocks { namespace { +static constexpr uint32_t k_page_size = 4096; + // ── journal mock ────────────────────────────────────────────────────────────── // // Backed by a std::map so tests can seed exact JournalSlot values per LSN. @@ -58,7 +63,8 @@ class MockCraftJournalBackend : public CraftJournalBackend { co_return homestore::multi_blk_id{}; } - async_status write_slot(int64_t, uint64_t, lba_t, lba_count_t, homestore::multi_blk_id, bool) override { + async_status write_slot(int64_t, uint64_t, lba_t, lba_count_t, homestore::multi_blk_id, bool, + std::vector< homestore::csum_t > const&) override { co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); } @@ -73,6 +79,9 @@ 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 read_data(homestore::multi_blk_id, sisl::sg_list&) override { + co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); + } }; // ── test fixture ───────────────────────────────────────────────────────────── @@ -82,7 +91,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_ = std::make_unique< CraftReplDev >(volume_id_t{}, std::move(mock), k_page_size, nullptr); } auto do_get_lsns() { return homeblocks::detail::sync_get(dev_->get_lsns(volume_id_t{})); } @@ -268,5 +277,7 @@ TEST_F(CraftPeerExchangeTest, FetchDataReadErrorPropagates) { int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); + SISL_OPTIONS_LOAD(argc, argv, logging); + HB_SETTINGS_FACTORY().load_json("{\"craft_watchdog_timeout_ms\": 0}"); return RUN_ALL_TESTS(); } diff --git a/src/lib/craft/tests/test_craft_truncate.cpp b/src/lib/craft/tests/test_craft_truncate.cpp index 894f242..46c9195 100644 --- a/src/lib/craft/tests/test_craft_truncate.cpp +++ b/src/lib/craft/tests/test_craft_truncate.cpp @@ -25,16 +25,21 @@ #include #include +#include #include "craft/craft_repl_dev.hpp" +#include "home_blks_config.hpp" #include "coro_helpers.hpp" SISL_LOGGING_DEF(HOMEBLOCKS_LOG_MODS) +SISL_OPTIONS_ENABLE(logging) SISL_LOGGING_INIT(HOMEBLOCKS_LOG_MODS) namespace homeblocks { namespace { +static constexpr uint32_t k_page_size = 4096; + // ── minimal journal mock ────────────────────────────────────────────────────── // // Captures the lsn passed to truncate_to() and can be armed to return an I/O error @@ -50,7 +55,8 @@ class MockCraftJournalBackend : public CraftJournalBackend { co_return homestore::multi_blk_id{}; } - async_status write_slot(int64_t, uint64_t, lba_t, lba_count_t, homestore::multi_blk_id, bool) override { + async_status write_slot(int64_t, uint64_t, lba_t, lba_count_t, homestore::multi_blk_id, bool, + std::vector< homestore::csum_t > const&) override { co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); } @@ -64,6 +70,9 @@ class MockCraftJournalBackend : public CraftJournalBackend { co_return ok(); } async_status free_data(homestore::multi_blk_id) override { co_return ok(); } + async_status read_data(homestore::multi_blk_id, sisl::sg_list&) override { + co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); + } }; // ── test fixture ───────────────────────────────────────────────────────────── @@ -73,7 +82,7 @@ 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_ = std::make_unique< CraftReplDev >(volume_id_t{}, std::move(mock), k_page_size, nullptr); } auto do_truncate(int64_t lsn) { return homeblocks::detail::sync_get(dev_->truncate(lsn)); } @@ -167,5 +176,7 @@ TEST_F(CraftTruncateTest, JournalErrorShieldsState) { int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); + SISL_OPTIONS_LOAD(argc, argv, logging); + HB_SETTINGS_FACTORY().load_json("{\"craft_watchdog_timeout_ms\": 0}"); return RUN_ALL_TESTS(); } diff --git a/src/lib/craft/tests/test_craft_watchdog.cpp b/src/lib/craft/tests/test_craft_watchdog.cpp new file mode 100644 index 0000000..17ad6eb --- /dev/null +++ b/src/lib/craft/tests/test_craft_watchdog.cpp @@ -0,0 +1,207 @@ +/********************************************************************************* + * 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's client-liveness watchdog (S7). +// +// Scheduling a real iomgr timer needs a running reactor pool -- the only reason this is its own +// binary rather than living in test_craft_commit.cpp: every other light CRAFT test compiles +// craft_repl_dev.cpp directly with a mock journal and NEVER starts iomgr (they set +// craft_watchdog_timeout_ms=0 so none of them need to). This file starts a minimal, HomeStore-free iomgr +// instance (see main()) so touch_watchdog()/on_watchdog_tick() can run for real, without pulling +// in the much heavier HomeStore bring-up test_craft_homestore_backend.cpp needs for its own real +// home_log_store -- and, unlike that file, this one compiles craft_repl_dev.cpp directly with +// _PRERELEASE (matching test_craft_write.cpp/test_craft_commit.cpp/test_craft_truncate.cpp/ +// test_craft_peer_exchange.cpp), so seed_term()/watchdog_fire_count() are actually available. +// +// 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 +#include + +#include "craft/craft_repl_dev.hpp" +#include "home_blks_config.hpp" +#include "coro_helpers.hpp" + +SISL_LOGGING_DEF(HOMEBLOCKS_LOG_MODS) +SISL_OPTIONS_ENABLE(logging) +SISL_LOGGING_INIT(HOMEBLOCKS_LOG_MODS) + +namespace homeblocks { +namespace { + +static constexpr uint32_t k_page_size = 4096; +static constexpr uint64_t k_test_watchdog_timeout_ms = 300; // ms; overrides craft_watchdog_timeout_ms in SetUp + +// ── minimal journal mock ──────────────────────────────────────────────────────── +// +// The watchdog never touches journal_ (touch_watchdog()/on_watchdog_tick() only read/write state_, +// last_contact_ns_, and watchdog_token_; append() -- called fire-and-forget on expiry -- is still a +// stub that never reaches journal_ either), so every method is an unreachable stub, same shape as +// test_craft_truncate.cpp's mock. + +class MockCraftJournalBackend : public CraftJournalBackend { +public: + async_result< homestore::multi_blk_id > alloc_write_data(sisl::sg_list const&, lba_count_t) override { + co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); + } + async_status write_slot(int64_t, uint64_t, lba_t, lba_count_t, homestore::multi_blk_id, bool, + std::vector< homestore::csum_t > const&) override { + co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); + } + async_result< JournalSlot > read_slot(int64_t) override { + co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); + } + async_status truncate_to(int64_t) override { co_return ok(); } + async_status free_data(homestore::multi_blk_id) override { co_return ok(); } + async_status read_data(homestore::multi_blk_id, sisl::sg_list&) override { + co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); + } +}; + +// ── test fixture ───────────────────────────────────────────────────────────── + +class CraftWatchdogTest : public ::testing::Test { +protected: + void SetUp() override { + orig_timeout_ms_ = HB_DYNAMIC_CONFIG(craft_watchdog_timeout_ms); + HB_SETTINGS_FACTORY().modifiable_settings( + [](auto& s) { s.craft_watchdog_timeout_ms = k_test_watchdog_timeout_ms; }); + dev_ = std::make_unique< CraftReplDev >(volume_id_t{}, std::make_unique< MockCraftJournalBackend >(), + k_page_size, /* indx_tbl = */ nullptr); + } + + void TearDown() override { + dev_.reset(); + HB_SETTINGS_FACTORY().modifiable_settings([this](auto& s) { s.craft_watchdog_timeout_ms = orig_timeout_ms_; }); + } + + // keep_alive() with a matching term is the only public path that both succeeds (term check + // passes) and calls touch_watchdog() -- write() would work too, but needs a real data payload + // for the non-all_zeros case; keep_alive() needs nothing but the header. + auto do_keep_alive(uint64_t term) { + return homeblocks::detail::sync_get(dev_->keep_alive(craft::client_hdr{term, -1, -1})); + } + + uint64_t orig_timeout_ms_{0}; + std::unique_ptr< CraftReplDev > dev_; +}; + +// ── tests ───────────────────────────────────────────────────────────────────── + +// append() is still a stub with no other observable side effect -- watchdog_fire_count() (test-only) +// is how this confirms on_watchdog_tick() actually fired after a full interval of no write()/ +// keep_alive() activity. +TEST_F(CraftWatchdogTest, FiresAfterInactivity) { + dev_->seed_term(7); + + auto r = do_keep_alive(7); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(dev_->watchdog_fire_count(), 0); // not yet -- keep_alive() itself just armed the timer + + std::this_thread::sleep_for(std::chrono::milliseconds(600)); // > 300ms configured timeout + // A small extra wait immediately before the assertion: 600ms is comfortably past when the timer + // should have ticked, but doesn't guarantee the iomgr worker thread has actually finished running + // that tick's callback by the moment THIS thread wakes from its own sleep -- those are two + // independently scheduled threads. Without this, the check is a real (if narrow) flakiness risk on + // a loaded machine. + std::this_thread::sleep_for(std::chrono::milliseconds(20)); + EXPECT_GE(dev_->watchdog_fire_count(), 1); +} + +// A keep_alive() before the interval elapses must refresh last_contact_ns_ -- no firing as long as +// activity keeps arriving faster than the timeout (each recurring tick sees a recent timestamp and +// takes no action). +TEST_F(CraftWatchdogTest, DoesNotFireIfResetInTime) { + dev_->seed_term(7); + + for (int i = 0; i < 4; ++i) { + ASSERT_TRUE(do_keep_alive(7).has_value()); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); // well under the 300ms timeout + } + EXPECT_EQ(dev_->watchdog_fire_count(), 0); +} + +// A not-yet-logged-in partition (term still 0, the default) must not arm the watchdog at all -- +// there is no session to watch yet. +TEST_F(CraftWatchdogTest, NoLoginNeverArms) { + // hdr.term=0 matches state_.term's default (0), so keep_alive() itself succeeds -- this + // specifically tests touch_watchdog()'s own state_.term==0 guard, not the term-check rejection. + ASSERT_TRUE(do_keep_alive(0).has_value()); + + std::this_thread::sleep_for(std::chrono::milliseconds(600)); // > 300ms configured timeout + EXPECT_EQ(dev_->watchdog_fire_count(), 0); +} + +// Destroys a CraftReplDev at an unpredictable point relative to its own watchdog timer's firing -- +// sometimes well before the first tick, sometimes squarely inside the window where on_watchdog_tick() +// is either about to run, actively running, or has just returned. An EARLIER, one-shot-timer-that- +// reschedules-itself design produced a real, reproduced SEGFAULT here (heap corruption in iomgr's +// timer heap from the self-reschedule cancelling its own already-fired, already-consumed handle) -- +// the current design uses a genuinely RECURRING iomgr::timer_token instead specifically to remove +// that whole hazard class (see touch_watchdog()/on_watchdog_tick()'s doc comments), but this test +// stays as a regression guard against any future variant of the same race, under either design. Many +// short-lived instances with a very short timeout, each racing destruction against a real timer +// firing, maximize the chance of hitting the narrow window; a single iteration would very likely pass +// even with a reintroduced bug. +TEST_F(CraftWatchdogTest, DestructorRacesFiringManyIterations) { + // Override to 2ms -- deliberately far shorter than SetUp()'s 300ms so firing is near-certain + // before destruction. Restores SetUp()'s value on scope exit via TearDown (fixture resets it). + HB_SETTINGS_FACTORY().modifiable_settings([](auto& s) { s.craft_watchdog_timeout_ms = 2; }); + static constexpr int kIterations = 200; + + for (int i = 0; i < kIterations; ++i) { + auto dev = std::make_unique< CraftReplDev >(volume_id_t{}, std::make_unique< MockCraftJournalBackend >(), + k_page_size, /* indx_tbl = */ nullptr); + dev->seed_term(7); + ASSERT_TRUE(homeblocks::detail::sync_get(dev->keep_alive(craft::client_hdr{7, -1, -1})).has_value()) + << "iteration " << i; + // No sleep here: destruction races the timer on purpose, rather than waiting it out first -- + // that is the entire point of this test (see the doc comment above). + dev.reset(); + } + // Reaching here at all (no crash, no hang) across every iteration is this test's actual + // assertion -- there is no separate observable count to check beyond survival. + SUCCEED(); +} + +} // namespace +} // namespace homeblocks + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + // iomanager.start() constructs iomgr's own settings factory, which needs SISL_OPTIONS parsed + // first (crashes otherwise) -- every other light CRAFT test skips this because none of them + // call iomanager.start() at all. + SISL_OPTIONS_LOAD(argc, argv, logging); + sisl::logging::SetLogger("test_craft_watchdog"); + spdlog::set_pattern("[%D %T%z] [%^%l%$] [%n] [%t] %v"); + + // Initialize the HomeBlks settings factory with flatbuffers defaults so HB_DYNAMIC_CONFIG() + // and HB_SETTINGS_FACTORY().modifiable_settings() work without a live HomeBlks instance. + HB_SETTINGS_FACTORY().load_json("{}"); + + // No HomeStore: just enough of a real iomgr reactor pool for iomgr::schedule_recurring/ + // timer_token::cancel to work. Matches iomgr's own minimal test bring-up (src/test/test_timer.cpp). + iomanager.start(iomgr::iomgr_params{.num_threads = 2}); + auto ret = RUN_ALL_TESTS(); + iomanager.stop(); + return ret; +} diff --git a/src/lib/craft/tests/test_craft_write.cpp b/src/lib/craft/tests/test_craft_write.cpp index 3c57db0..2a31d1c 100644 --- a/src/lib/craft/tests/test_craft_write.cpp +++ b/src/lib/craft/tests/test_craft_write.cpp @@ -25,19 +25,28 @@ #include #include +#include #include #include #include +#include #include "craft/craft_repl_dev.hpp" +#include "home_blks_config.hpp" #include "coro_helpers.hpp" +// Must match k_craft_crc16_seed in craft_repl_dev.cpp -- same seed volume.cpp's non-CRAFT path uses. +static constexpr homestore::csum_t k_test_crc16_seed = 0x8005; + SISL_LOGGING_DEF(HOMEBLOCKS_LOG_MODS) +SISL_OPTIONS_ENABLE(logging) SISL_LOGGING_INIT(HOMEBLOCKS_LOG_MODS) namespace homeblocks { namespace { +static constexpr uint32_t k_page_size = 4096; + // ── journal mock ────────────────────────────────────────────────────────────── class MockCraftJournalBackend : public CraftJournalBackend { @@ -58,10 +67,11 @@ class MockCraftJournalBackend : public CraftJournalBackend { 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 { + 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, std::vector< homestore::csum_t > const& csums) override { if (fail_lsns.contains(lsn)) co_return std::unexpected(std::make_error_condition(std::errc::io_error)); - slots[lsn] = JournalSlot{lsn, false, all_zeros, lba, len, {}}; + slots[lsn] = JournalSlot{ + .lsn = lsn, .all_zeros = all_zeros, .lba_off_bytes = lba, .len_bytes = len, .blkid = blkid, .csums = csums}; slot_terms[lsn] = term; co_return ok(); } @@ -78,6 +88,9 @@ class MockCraftJournalBackend : public CraftJournalBackend { ++free_data_calls; co_return ok(); } + async_status read_data(homestore::multi_blk_id, sisl::sg_list&) override { + co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); + } bool has_slot(int64_t lsn) const { return slots.contains(lsn); } size_t slot_count() const { return slots.size(); } @@ -90,21 +103,24 @@ 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_ = std::make_unique< CraftReplDev >(volume_id_t{}, std::move(mock), k_page_size, nullptr); } - auto do_write(uint64_t term, int64_t lsn, bool all_zeros = true) { + auto do_write(uint64_t term, int64_t lsn) { return homeblocks::detail::sync_get( - dev_->write(craft::client_hdr{term, -1, -1}, lsn, 0, 4096, sisl::sg_list{}, all_zeros)); + dev_->write(craft::client_hdr{term, -1, -1}, lsn, 0, 4096, sisl::sg_list{})); } - // Variant that marks data.size > 0 so the alloc_write_data branch is exercised. - // The mock ignores the actual iovecs; only data.size matters for the branch guard. + // Variant that marks data.size > 0 so the alloc_write_data branch (and the checksum loop, which + // reads real bytes from the first iovec) is exercised. Backed by a real buffer since write() + // computes a per-LBA CRC over it. auto do_write_with_data(uint64_t term, int64_t lsn) { + static std::array< uint8_t, k_page_size > buf{}; sisl::sg_list data; - data.size = 4096; + data.size = k_page_size; + data.iovs.push_back(iovec{buf.data(), buf.size()}); return homeblocks::detail::sync_get( - dev_->write(craft::client_hdr{term, -1, -1}, lsn, 0, 4096, std::move(data), false)); + dev_->write(craft::client_hdr{term, -1, -1}, lsn, 0, k_page_size, std::move(data))); } MockCraftJournalBackend* journal_{nullptr}; @@ -154,9 +170,9 @@ TEST_F(CraftWriteTest, TermRejection) { EXPECT_EQ(dev_->missing_count(), 0u); } -// all_zeros=true skips data allocation; journal slot is marked all_zeros. +// Empty data skips data allocation; journal slot is marked all_zeros. TEST_F(CraftWriteTest, AllZerosWrite) { - auto r = do_write(0, 0, /*all_zeros=*/true); + auto r = do_write(0, 0); ASSERT_TRUE(r.has_value()); EXPECT_EQ(r->last_append_lsn, 0); EXPECT_EQ(dev_->missing_count(), 0u); @@ -266,13 +282,13 @@ TEST_F(CraftWriteTest, DuplicateWriteIsIdempotent) { EXPECT_EQ(journal_->alloc_write_data_calls, 1); EXPECT_FALSE(journal_->slots[1].all_zeros); // data slot preserved - auto r_zero = do_write(0, 1, /*all_zeros=*/true); + auto r_zero = do_write(0, 1); ASSERT_TRUE(r_zero.has_value()); EXPECT_EQ(journal_->slot_count(), 2u); // write_slot not called again EXPECT_EQ(journal_->alloc_write_data_calls, 1); // no extra alloc for the all_zeros retry - // cross-type: all_zeros write first, then data retry at dLSN=2 — data is discarded - auto r_zero2 = do_write(0, 2, /*all_zeros=*/true); + // cross-type: zero write first, then data retry at dLSN=2 — data is discarded + auto r_zero2 = do_write(0, 2); ASSERT_TRUE(r_zero2.has_value()); EXPECT_EQ(journal_->slot_count(), 3u); EXPECT_EQ(journal_->alloc_write_data_calls, 1); // all_zeros never allocates @@ -438,8 +454,8 @@ TEST_F(CraftWriteTest, NonZeroWriteIsZeroCopy) { data.size = buf.size(); data.iovs.push_back(iovec{buf.data(), buf.size()}); - auto r = homeblocks::detail::sync_get(dev_->write(craft::client_hdr{0, -1, -1}, /* dlsn = */ 0, 0, buf.size(), - std::move(data), /* all_zeros = */ false)); + auto r = homeblocks::detail::sync_get( + dev_->write(craft::client_hdr{0, -1, -1}, /* dlsn = */ 0, 0, buf.size(), std::move(data))); ASSERT_TRUE(r.has_value()); EXPECT_EQ(journal_->last_alloc_data_ptr, static_cast< void const* >(buf.data())); } @@ -468,26 +484,14 @@ TEST_F(CraftWriteTest, WriteSlotFailsWithData_BlocksFreed) { EXPECT_TRUE(dev_->is_missing(0)); // pre-insert invariant holds } -// all_zeros=false with an empty sg_list (data.size==0) was previously a RELEASE_ASSERT (process abort). -// After the fix it returns invalid_argument so a malformed client frame cannot kill the replica. -TEST_F(CraftWriteTest, AllZerosFalseWithEmptyDataRejected) { - sisl::sg_list empty_data{}; - auto r = homeblocks::detail::sync_get( - dev_->write(craft::client_hdr{0, -1, -1}, 0, 0, 4096, std::move(empty_data), /*all_zeros=*/false)); - ASSERT_FALSE(r.has_value()); - EXPECT_EQ(r.error(), make_error_condition(std::errc::invalid_argument)); - EXPECT_EQ(journal_->slot_count(), 0u); // write_slot not reached - EXPECT_EQ(dev_->last_append_lsn(), -1); // no state mutation -} - -// The inverse bad combination: all_zeros=true with a non-empty sg_list. Previously silently -// accepted and dropped the payload (the guard only covered !all_zeros && data.size == 0); now -// rejected symmetrically, since WRITE_ZEROES/unmap names a range and must not also carry data. -TEST_F(CraftWriteTest, AllZerosTrueWithNonEmptyDataRejected) { +// A malformed sg_list (data.size > 0 but iovs is empty) is rejected -- the CRAFT connector builds +// sg_lists from real payloads, so this should be unreachable, but protects against a connector +// bug that sets size without populating the iovec. +TEST_F(CraftWriteTest, MalformedSgListRejected) { sisl::sg_list data; - data.size = 4096; + data.size = 4096; // size set but no iovs -- malformed auto r = homeblocks::detail::sync_get( - dev_->write(craft::client_hdr{0, -1, -1}, /* dlsn = */ 0, 0, 4096, std::move(data), /* all_zeros = */ true)); + dev_->write(craft::client_hdr{0, -1, -1}, /* dlsn = */ 0, 0, 4096, std::move(data))); ASSERT_FALSE(r.has_value()); EXPECT_EQ(r.error(), make_error_condition(std::errc::invalid_argument)); EXPECT_EQ(journal_->slot_count(), 0u); // write_slot not reached @@ -505,10 +509,108 @@ TEST_F(CraftWriteTest, WriteSlotReceivesCorrectTerm) { EXPECT_EQ(journal_->slot_terms[0], k_term); } +// len must be a positive multiple of lba_size_ -- len=0 would let nlbas=0 reach commit()'s +// end_lba = start_lba + nlbas - 1 computation later, underflowing lba_t (unsigned) into a +// near-UINT64_MAX range applied to the real index. +TEST_F(CraftWriteTest, ZeroLenRejected) { + sisl::sg_list empty_data{}; + auto r = homeblocks::detail::sync_get( + dev_->write(craft::client_hdr{0, -1, -1}, 0, 0, /* len = */ 0, std::move(empty_data))); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(std::errc::invalid_argument)); + EXPECT_EQ(journal_->slot_count(), 0u); + EXPECT_EQ(dev_->last_append_lsn(), -1); +} + +// A len that doesn't evenly divide lba_size_ is rejected -- it would otherwise silently round +// nlbas down (e.g. len < lba_size_ rounds to nlbas=0, the same underflow risk as ZeroLenRejected). +TEST_F(CraftWriteTest, UnalignedLenRejected) { + sisl::sg_list empty_data{}; + auto r = homeblocks::detail::sync_get( + dev_->write(craft::client_hdr{0, -1, -1}, 0, 0, /* len = */ k_page_size / 2, std::move(empty_data))); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(std::errc::invalid_argument)); +} + +// addr must also be block-aligned (the BYTE-addressed API's own documented contract) -- an +// unaligned addr would silently floor-divide to the wrong starting LBA everywhere it's used. +TEST_F(CraftWriteTest, UnalignedAddrRejected) { + sisl::sg_list empty_data{}; + auto r = homeblocks::detail::sync_get( + dev_->write(craft::client_hdr{0, -1, -1}, 0, /* addr = */ 1, k_page_size, std::move(empty_data))); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(std::errc::invalid_argument)); +} + +// write() computes a real per-LBA CRC16 (crc16_t10dif) over the payload while it's still in +// memory -- verify the computed value matches an independently-computed one, not just that +// something of the right array length reached write_slot. +TEST_F(CraftWriteTest, ComputesCorrectChecksum) { + std::array< uint8_t, k_page_size > buf{}; + buf.fill(0xCD); + sisl::sg_list data; + data.size = k_page_size; + data.iovs.push_back(iovec{buf.data(), buf.size()}); + + auto r = homeblocks::detail::sync_get( + dev_->write(craft::client_hdr{0, -1, -1}, /* dlsn = */ 0, 0, k_page_size, std::move(data))); + ASSERT_TRUE(r.has_value()); + + auto expected = crc16_t10dif(k_test_crc16_seed, buf.data(), buf.size()); + ASSERT_EQ(journal_->slots[0].csums.size(), 1u); + EXPECT_EQ(journal_->slots[0].csums[0], expected); +} + +// A write spanning multiple LBAs computes one checksum per LBA, each over its own page -- not a +// single checksum for the whole buffer, and not the same value repeated for every page. +TEST_F(CraftWriteTest, MultiLbaWriteComputesPerLbaChecksums) { + constexpr uint32_t k_nlbas = 3; + std::array< uint8_t, k_nlbas * k_page_size > buf{}; + // Distinct content per page so identical checksums would indicate a bug (e.g. always hashing + // just the first page for every LBA). + for (uint32_t i = 0; i < k_nlbas; ++i) + std::fill_n(buf.data() + i * k_page_size, k_page_size, uint8_t(i + 1)); + sisl::sg_list data; + data.size = buf.size(); + data.iovs.push_back(iovec{buf.data(), buf.size()}); + + auto r = homeblocks::detail::sync_get(dev_->write(craft::client_hdr{0, -1, -1}, 0, 0, buf.size(), std::move(data))); + ASSERT_TRUE(r.has_value()); + + ASSERT_EQ(journal_->slots[0].csums.size(), k_nlbas); + for (uint32_t i = 0; i < k_nlbas; ++i) { + auto expected = crc16_t10dif(k_test_crc16_seed, buf.data() + i * k_page_size, k_page_size); + EXPECT_EQ(journal_->slots[0].csums[i], expected); + } + EXPECT_NE(journal_->slots[0].csums[0], journal_->slots[0].csums[1]); + EXPECT_NE(journal_->slots[0].csums[1], journal_->slots[0].csums[2]); +} + +// ── craft_max_io_len_mb enforcement in write() ─────────────────────────────── + +// craft_max_io_len_mb is set to 1 MiB in main() -- a write of 1 MiB + one page (page-aligned, +// non-zero) must be rejected before any block allocation. Guards against a malformed wire frame +// driving an unbounded index range operation or a multi-GiB allocation. +TEST_F(CraftWriteTest, MaxIoLenEnforced) { + // 1 MiB + one page: aligned, non-zero, but above the 1 MiB cap. + constexpr uint64_t k_over_limit = 1024 * 1024 + k_page_size; + sisl::sg_list data; + data.size = k_over_limit; // the len > max_io_len check fires before any sg_list access + auto r = + homeblocks::detail::sync_get(dev_->write(craft::client_hdr{0, -1, -1}, 0, 0, k_over_limit, std::move(data))); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(std::errc::invalid_argument)); + EXPECT_EQ(journal_->slots.size(), 0u); // no slot written +} + } // namespace } // namespace homeblocks int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); + SISL_OPTIONS_LOAD(argc, argv, logging); + // craft_watchdog_timeout_ms=0: watchdog disabled (no iomgr needed). + // craft_max_io_len_mb=1: small cap for MaxIoLenEnforced test; all other tests use <= 12 KiB. + HB_SETTINGS_FACTORY().load_json("{\"craft_watchdog_timeout_ms\": 0, \"craft_max_io_len_mb\": 1}"); return RUN_ALL_TESTS(); } diff --git a/src/lib/home_blks_config.fbs b/src/lib/home_blks_config.fbs index f0d5ae9..ba39b75 100644 --- a/src/lib/home_blks_config.fbs +++ b/src/lib/home_blks_config.fbs @@ -24,6 +24,18 @@ table HomeBlksSettings{ // how often (in appended LSNs) the leader auto-proposes a SyncRSCommitLSN entry; sync_rs_commit_lsn_interval: uint32 = 128; + + // CRAFT client-liveness watchdog timeout in milliseconds -- reset on every successful write()/ + // keep_alive(); on expiry (no activity from the current session) proposes a SyncRSCommitLSN + // entry via append() so the replica set can make progress without the client. + // 0 = watchdog disabled. + craft_watchdog_timeout_ms: uint64 = 30000; + + // Maximum byte length (in MiB) accepted for a single CRAFT write() or read() call. Requests + // larger than this are rejected with invalid_argument -- both paths are reachable from the + // client wire, so an unbounded len would allow a malformed frame to drive an unbounded index + // range operation or a multi-GiB allocation. + craft_max_io_len_mb: uint64 = 128; } root_type HomeBlksSettings; diff --git a/src/lib/volume/index_fixed_table.hpp b/src/lib/volume/index_fixed_table.hpp index ef6a1f3..093399b 100644 --- a/src/lib/volume/index_fixed_table.hpp +++ b/src/lib/volume/index_fixed_table.hpp @@ -1,5 +1,7 @@ #pragma once +#include + #include "index_fixed_kv.hpp" namespace homeblocks { @@ -83,6 +85,27 @@ class VolumeIndexTable { return {}; } + // Unmap [start_lba, end_lba]: removes every index entry in range in a single range-remove call, + // capturing each removed entry's blkid via the filter callback (which is invoked once per entry + // and unconditionally votes to remove it) so the caller can reclaim those blocks. + status delete_lba_range(lba_t start_lba, lba_t end_lba, std::vector< homestore::blk_id >& out_freed_blkids) { + homestore::remove_filter_cb_t filter_cb = [&out_freed_blkids](homestore::BtreeKey const&, + homestore::BtreeValue const& value) -> bool { + out_freed_blkids.push_back(static_cast< VolumeIndexValue const& >(value).blkid()); + return true; // unconditionally remove every entry in range + }; + auto rreq = homestore::BtreeRangeRemoveRequest< VolumeIndexKey >{ + homestore::BtreeKeyRange< VolumeIndexKey >{VolumeIndexKey{start_lba}, VolumeIndexKey{end_lba}}, nullptr, + std::numeric_limits< uint32_t >::max(), filter_cb}; + // not_found: nothing was mapped in this range -- unmap of nothing is a no-op, not a failure. + if (auto result = hs_index_table_->remove(rreq); + result != homestore::btree_status_t::success && result != homestore::btree_status_t::not_found) { + LOGERROR("Failed to remove lba range [{}, {}] from index, error={}", start_lba, end_lba, result); + return std::unexpected(volume_error::INDEX_ERROR); + } + return ok(); + } + void rollback_write(lba_t start_lba, lba_t end_lba, std::unordered_map< lba_t, BlockInfo >& blocks_info) { for (auto lba = start_lba; lba <= end_lba; ++lba) { VolumeIndexKey key{lba};