From 51ffc3a726f2e38009eb10a198c7eb28913353f0 Mon Sep 17 00:00:00 2001 From: Mehdi Hosseini Date: Wed, 22 Jul 2026 12:00:11 -0700 Subject: [PATCH 1/7] =?UTF-8?q?craft:=20S2=20write=20path=20=E2=80=94=20jo?= =?UTF-8?q?urnal=20append=20at=20client-assigned=20LSN?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CraftJournalBackend::write_slot: replace sisl::sg_list with homestore::multi_blk_id blkid + bool all_zeros (HS_DATA_LINKED scheme; payload never enters the journal buffer) - CraftReplDev::write: add bool all_zeros param; empty-verdict rejection (EMPTY_SLOT) checked under missing_mu_ before pre-insert; stubbed blkid{} passed to write_slot (real block allocation wired in commit 2) - volume_error: add EMPTY_SLOT enum value - Tests: MockCraftJournalBackend updated to new signature in all three test TUs; three new cases: AllZerosWrite, WriteSlotFails_LsnRemainsInMissing, EmptySlotRejectsWrite --- src/include/homeblks/home_blocks.hpp | 3 +- src/lib/craft/craft_repl_dev.cpp | 15 +++++-- src/lib/craft/craft_repl_dev.hpp | 5 ++- .../craft/tests/test_craft_peer_exchange.cpp | 2 +- src/lib/craft/tests/test_craft_truncate.cpp | 2 +- src/lib/craft/tests/test_craft_write.cpp | 41 +++++++++++++++++-- 6 files changed, 55 insertions(+), 13 deletions(-) diff --git a/src/include/homeblks/home_blocks.hpp b/src/include/homeblks/home_blocks.hpp index 1738a99..f509f1f 100644 --- a/src/include/homeblks/home_blocks.hpp +++ b/src/include/homeblks/home_blocks.hpp @@ -68,7 +68,8 @@ using volume_handle = std::shared_ptr< volume >; // ride result while staying branchable: if (r.error() == volume_error::CRC_MISMATCH) { ... }. Anything with a // standard equivalent (invalid arg, no space, io error, unsupported op, ...) is returned as // std::make_error_condition(std::errc::*) directly rather than duplicated here. -ENUM(volume_error, uint16_t, UNKNOWN_VOLUME = 1, CRC_MISMATCH, INDEX_ERROR, INTERNAL_ERROR, OFFLINE, STALE_TERM); +ENUM(volume_error, uint16_t, UNKNOWN_VOLUME = 1, CRC_MISMATCH, INDEX_ERROR, INTERNAL_ERROR, OFFLINE, STALE_TERM, + EMPTY_SLOT); ENUM(volume_state, uint32_t, INIT, // created, not yet online diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index fa6cb7c..f81ed60 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -29,7 +29,8 @@ class HomeStoreCraftJournalBackend : public CraftJournalBackend { explicit HomeStoreCraftJournalBackend(shared< homestore::home_log_store > logstore) : logstore_{std::move(logstore)} {} - async_status write_slot(int64_t lsn, lba_t /* lba */, lba_count_t /* len */, sisl::sg_list /* data */) override { + async_status write_slot(int64_t lsn, lba_t /* lba */, lba_count_t /* len */, + homestore::multi_blk_id /* blkid */, bool /* all_zeros */) override { LOGW("HomeStoreCraftJournalBackend::write_slot lsn={} not yet implemented", lsn); co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); } @@ -148,7 +149,7 @@ 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) { + sisl::sg_list data, bool all_zeros) { if (hdr.term != state_.term) { LOGW("write rejected: stale term want={} got={} dlsn={}", state_.term, hdr.term, dlsn); co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); @@ -156,14 +157,20 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 { std::lock_guard lock{missing_mu_}; + 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)); + } for (int64_t gap = state_.last_append_lsn + 1; gap < dlsn; ++gap) missing_lsns_.insert(gap); if ((dlsn > state_.last_append_lsn) || missing_lsns_.contains(dlsn)) missing_lsns_.insert(dlsn); state_.last_append_lsn = std::max(state_.last_append_lsn, dlsn); } + // HS_DATA_LINKED: for all_zeros=true skip data-service write; real block allocation wired in commit 2. + homestore::multi_blk_id blkid{}; auto res = co_await journal_->write_slot(dlsn, static_cast< lba_t >(addr), static_cast< lba_count_t >(len), - std::move(data)); + blkid, all_zeros); if (!res) { LOGE("write_slot failed dlsn={} addr={} len={}: {}", dlsn, addr, len, res.error().message()); co_return std::unexpected(res.error()); @@ -175,7 +182,7 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 missing_lsns_.erase(dlsn); snapshot = {state_.commit_lsn, state_.last_append_lsn}; } - LOGT("write ok dlsn={} addr={} len={}", dlsn, addr, len); + LOGT("write ok dlsn={} addr={} len={} all_zeros={}", dlsn, addr, len, all_zeros); co_return snapshot; } diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index bc6f508..2150a48 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -70,7 +70,8 @@ struct JournalSlot { class CraftJournalBackend { public: - virtual async_status write_slot(int64_t lsn, lba_t lba, lba_count_t len, sisl::sg_list data) = 0; + virtual async_status write_slot(int64_t lsn, lba_t lba, lba_count_t len, + homestore::multi_blk_id blkid, bool all_zeros) = 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; @@ -116,7 +117,7 @@ class CraftReplDev { // 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); + sisl::sg_list data, bool all_zeros = false); // 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 diff --git a/src/lib/craft/tests/test_craft_peer_exchange.cpp b/src/lib/craft/tests/test_craft_peer_exchange.cpp index 6cdcbf3..a9e7870 100644 --- a/src/lib/craft/tests/test_craft_peer_exchange.cpp +++ b/src/lib/craft/tests/test_craft_peer_exchange.cpp @@ -53,7 +53,7 @@ class MockCraftJournalBackend : public CraftJournalBackend { std::map< int64_t, JournalSlot > slots; std::optional< int64_t > fail_on_read; // if set, read_slot for that LSN returns io_error - async_status write_slot(int64_t, lba_t, lba_count_t, sisl::sg_list) override { + async_status write_slot(int64_t, lba_t, lba_count_t, homestore::multi_blk_id, bool) override { co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); } diff --git a/src/lib/craft/tests/test_craft_truncate.cpp b/src/lib/craft/tests/test_craft_truncate.cpp index 8470e2a..bac197b 100644 --- a/src/lib/craft/tests/test_craft_truncate.cpp +++ b/src/lib/craft/tests/test_craft_truncate.cpp @@ -45,7 +45,7 @@ class MockCraftJournalBackend : public CraftJournalBackend { bool should_fail{false}; int64_t truncated_to{INT64_MIN}; - async_status write_slot(int64_t, lba_t, lba_count_t, sisl::sg_list) override { + async_status write_slot(int64_t, lba_t, lba_count_t, homestore::multi_blk_id, bool) override { co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); } diff --git a/src/lib/craft/tests/test_craft_write.cpp b/src/lib/craft/tests/test_craft_write.cpp index cb712b9..d0cecdd 100644 --- a/src/lib/craft/tests/test_craft_write.cpp +++ b/src/lib/craft/tests/test_craft_write.cpp @@ -41,9 +41,13 @@ namespace { class MockCraftJournalBackend : public CraftJournalBackend { public: std::map< int64_t, JournalSlot > slots; + std::set< int64_t > fail_lsns; // write_slot returns io_error for these lsns - async_status write_slot(int64_t lsn, lba_t lba, lba_count_t len, sisl::sg_list) override { - slots[lsn] = JournalSlot{lsn, false, false, lba, len, {}}; + async_status write_slot(int64_t lsn, lba_t lba, lba_count_t len, + homestore::multi_blk_id /* blkid */, bool all_zeros) override { + if (fail_lsns.count(lsn)) + co_return std::unexpected(std::make_error_condition(std::errc::io_error)); + slots[lsn] = JournalSlot{lsn, false, all_zeros, lba, len, {}}; co_return ok(); } @@ -70,9 +74,9 @@ class CraftWriteTest : public ::testing::Test { dev_ = std::make_unique< CraftReplDev >(volume_id_t{}, std::move(mock)); } - auto do_write(uint64_t term, int64_t lsn) { + auto do_write(uint64_t term, int64_t lsn, bool all_zeros = false) { return homeblocks::detail::sync_get( - dev_->write(craft::client_hdr{term, -1, -1}, lsn, 0, 4096, sisl::sg_list{})); + dev_->write(craft::client_hdr{term, -1, -1}, lsn, 0, 4096, sisl::sg_list{}, all_zeros)); } MockCraftJournalBackend* journal_{nullptr}; @@ -122,6 +126,35 @@ TEST_F(CraftWriteTest, TermRejection) { EXPECT_EQ(dev_->missing_count(), 0u); } +// all_zeros=true skips data allocation; journal slot is marked all_zeros. +TEST_F(CraftWriteTest, AllZerosWrite) { + auto r = do_write(0, 0, /*all_zeros=*/true); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(r->last_append_lsn, 0); + EXPECT_EQ(dev_->missing_count(), 0u); + ASSERT_TRUE(journal_->has_slot(0)); + EXPECT_TRUE(journal_->slots[0].all_zeros); +} + +// A failed write_slot must leave the lsn in missing_lsns_ (pre-insert invariant). +TEST_F(CraftWriteTest, WriteSlotFails_LsnRemainsInMissing) { + journal_->fail_lsns.insert(0); + auto r = do_write(0, 0); + ASSERT_FALSE(r.has_value()); + EXPECT_TRUE(dev_->is_missing(0)); + EXPECT_EQ(journal_->slot_count(), 0u); +} + +// A write into an Empty-verdicted slot is permanently rejected (Empty beats data). +TEST_F(CraftWriteTest, EmptySlotRejectsWrite) { + dev_->seed_empty({5}); + auto r = do_write(0, 5); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(r.error(), make_error_condition(volume_error::EMPTY_SLOT)); + EXPECT_EQ(journal_->slot_count(), 0u); + EXPECT_FALSE(dev_->is_missing(5)); +} + } // namespace } // namespace homeblocks From 8eb1066fc4108b8740c26830ed42ecdb2d6293c7 Mon Sep 17 00:00:00 2001 From: Mehdi Hosseini Date: Wed, 22 Jul 2026 12:26:02 -0700 Subject: [PATCH 2/7] craft: harden write() against data race, DoS gap, and missing/empty inconsistency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three correctness fixes found by adversarial review of the S2 write path: 1. Move state_.term check inside missing_mu_ lock. state_.term is mutated by apply_internal_login (S5) under the same mutex; reading it outside the lock is a data race (UB) once S5 lands. 2. Reject dlsn < 0 before acquiring any lock. A negative dlsn (initial last_append_lsn=-1 edge case) could bypass the pre-insert condition and call write_slot unguarded, violating the invariant that every written dlsn is pre-inserted into missing_lsns_. 3. Cap out-of-order gap at k_max_ooo_gap (1 000 000). A single write with a large dlsn caused O(gap) synchronous allocations into missing_lsns_ under the mutex — an OOM / latency-spike risk. One additional fix: seed_empty (and future apply_sync_rs_commit_lsn) must also erase the verdicted LSNs from missing_lsns_. An LSN already present in missing_lsns_ when the Empty verdict fires would stay there forever, permanently stalling commit advancement. Tests added: EmptyVerdictClearsMissingEntry, NegativeDlsnRejected, ExcessiveGapRejected, OutOfOrderWritesLargerGap. --- src/lib/craft/craft_repl_dev.cpp | 21 +++++++-- src/lib/craft/tests/test_craft_write.cpp | 58 ++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index f81ed60..c165310 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -133,6 +133,10 @@ void CraftReplDev::seed_empty(std::initializer_list< int64_t > empty) { std::lock_guard lk{missing_mu_}; empty_lsns_.clear(); empty_lsns_.insert(empty); + // Empty verdict resolves a gap: an LSN that was already in missing_lsns_ must be removed so + // it does not permanently stall commit advancement. apply_sync_rs_commit_lsn (S5) must do the same. + for (int64_t lsn : empty) + missing_lsns_.erase(lsn); } #endif @@ -150,17 +154,28 @@ 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) { - if (hdr.term != state_.term) { - LOGW("write rejected: stale term want={} got={} dlsn={}", state_.term, hdr.term, dlsn); - co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); + if (dlsn < 0) { + LOGW("write rejected: invalid dlsn={}", dlsn); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); } { std::lock_guard lock{missing_mu_}; + // Term check is inside the lock: state_.term is mutated by apply_internal_login (S5) under the same mutex. + if (hdr.term != state_.term) { + LOGW("write rejected: stale term want={} got={} dlsn={}", state_.term, hdr.term, dlsn); + co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); + } 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)); } + // Cap the gap to prevent unbounded per-write allocation in missing_lsns_. + static constexpr int64_t k_max_ooo_gap = 1'000'000; + if (dlsn - state_.last_append_lsn > k_max_ooo_gap) { + LOGW("write rejected: dlsn={} too far ahead of last_append_lsn={}", dlsn, state_.last_append_lsn); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } for (int64_t gap = state_.last_append_lsn + 1; gap < dlsn; ++gap) missing_lsns_.insert(gap); if ((dlsn > state_.last_append_lsn) || missing_lsns_.contains(dlsn)) missing_lsns_.insert(dlsn); diff --git a/src/lib/craft/tests/test_craft_write.cpp b/src/lib/craft/tests/test_craft_write.cpp index d0cecdd..ef5ad34 100644 --- a/src/lib/craft/tests/test_craft_write.cpp +++ b/src/lib/craft/tests/test_craft_write.cpp @@ -155,6 +155,64 @@ TEST_F(CraftWriteTest, EmptySlotRejectsWrite) { EXPECT_FALSE(dev_->is_missing(5)); } +// Empty verdict on a slot already in missing_lsns_ must remove it so the gap is resolved. +// Without this, the slot stalls commit advancement permanently. +TEST_F(CraftWriteTest, EmptyVerdictClearsMissingEntry) { + // Write lsn=10 with the journal injecting a failure → lsn 10 and gaps 0-9 all land in missing. + journal_->fail_lsns.insert(10); + auto r10 = do_write(0, 10); + ASSERT_FALSE(r10.has_value()); + EXPECT_TRUE(dev_->is_missing(5)); + EXPECT_TRUE(dev_->is_missing(10)); + + // S5 verdicts lsn=5 Empty: must be evicted from missing_lsns_. + dev_->seed_empty({5}); + EXPECT_FALSE(dev_->is_missing(5)); // gap resolved + EXPECT_TRUE(dev_->is_empty_slot(5)); // permanently empty + + // A late write to lsn=5 is still rejected with EMPTY_SLOT. + auto r5 = do_write(0, 5); + ASSERT_FALSE(r5.has_value()); + EXPECT_EQ(r5.error(), make_error_condition(volume_error::EMPTY_SLOT)); +} + +// A negative dlsn is rejected before any state is touched. +TEST_F(CraftWriteTest, NegativeDlsnRejected) { + auto r = do_write(0, -1); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(journal_->slot_count(), 0u); + EXPECT_EQ(dev_->missing_count(), 0u); + EXPECT_EQ(dev_->last_append_lsn(), -1); +} + +// A dlsn far ahead of last_append_lsn is rejected to prevent unbounded gap allocation. +TEST_F(CraftWriteTest, ExcessiveGapRejected) { + auto r = do_write(0, 1'000'001); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(dev_->missing_count(), 0u); + EXPECT_EQ(dev_->last_append_lsn(), -1); +} + +// Gap of more than 1: writing lsn=4 after lsn=0 creates gaps {1,2,3} in missing_lsns_. +TEST_F(CraftWriteTest, OutOfOrderWritesLargerGap) { + ASSERT_TRUE(do_write(0, 0).has_value()); + auto r4 = do_write(0, 4); + ASSERT_TRUE(r4.has_value()); + EXPECT_EQ(r4->last_append_lsn, 4); + EXPECT_EQ(dev_->missing_count(), 3u); + EXPECT_TRUE(dev_->is_missing(1)); + EXPECT_TRUE(dev_->is_missing(2)); + EXPECT_TRUE(dev_->is_missing(3)); + EXPECT_FALSE(dev_->is_missing(4)); // 4 was successfully written + + // Fill the gap one by one and verify the missing set drains. + ASSERT_TRUE(do_write(0, 2).has_value()); + EXPECT_EQ(dev_->missing_count(), 2u); + ASSERT_TRUE(do_write(0, 1).has_value()); + ASSERT_TRUE(do_write(0, 3).has_value()); + EXPECT_EQ(dev_->missing_count(), 0u); +} + } // namespace } // namespace homeblocks From 6204d4195152318fb29863facd0ccd1cdc2c22fa Mon Sep 17 00:00:00 2001 From: Mehdi Hosseini Date: Wed, 22 Jul 2026 13:04:59 -0700 Subject: [PATCH 3/7] =?UTF-8?q?craft:=20round-2=20hardening=20=E2=80=94=20?= =?UTF-8?q?overflow=20guard,=20idempotent=20write,=20gap-loop=20filter,=20?= =?UTF-8?q?post-flight=20term=20check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four bugs found by adversarial review; all fixed and test-covered. B1 — signed overflow in gap cap check: dlsn near INT64_MAX caused dlsn - last_append_lsn to wrap, silently bypassing the cap and looping ~2^63 times → OOM. Fix: Guard 1 rejects dlsn > INT64_MAX - k_max_ooo_gap before the subtraction in Guard 2. M1 — duplicate write bypasses pre-insert invariant: after dlsn=N succeeds and is erased from missing_lsns_, a retry evaluated (N>N)=false && !contains(N)=true → no pre-insert → write_slot called with N absent from missing. Fix: return current snapshot idempotently when dlsn ≤ last_append_lsn and not in missing_lsns_. M2 — gap loop re-introduces Empty-verdicted LSNs: the fill loop inserted all gaps unconditionally, undoing the Empty verdict and stalling commit advancement. Fix: skip lsns in empty_lsns_ inside the loop body. M3 — ghost entry after login-truncate race: write_slot completing after a new login+truncate produced a journal entry that survived past the truncation point. Fix: re-validate hdr.term in the second lock region and discard the result with STALE_TERM on mismatch. Also: fix seed_empty() header comment (said "does not affect missing_lsns_", which is now wrong); add tests for all four fixes including gap-cap fencepost from non-initial state and Guard 1 at INT64_MAX. --- src/lib/craft/craft_repl_dev.cpp | 32 ++++++++--- src/lib/craft/craft_repl_dev.hpp | 7 ++- src/lib/craft/tests/test_craft_write.cpp | 71 ++++++++++++++++++++++-- 3 files changed, 94 insertions(+), 16 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index c165310..8c2c934 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -29,8 +29,8 @@ class HomeStoreCraftJournalBackend : public CraftJournalBackend { explicit HomeStoreCraftJournalBackend(shared< homestore::home_log_store > logstore) : logstore_{std::move(logstore)} {} - async_status write_slot(int64_t lsn, lba_t /* lba */, lba_count_t /* len */, - homestore::multi_blk_id /* blkid */, bool /* all_zeros */) override { + async_status write_slot(int64_t lsn, lba_t /* lba */, lba_count_t /* len */, homestore::multi_blk_id /* blkid */, + bool /* all_zeros */) override { LOGW("HomeStoreCraftJournalBackend::write_slot lsn={} not yet implemented", lsn); co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); } @@ -170,22 +170,34 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 LOGW("write rejected: slot is permanently empty dlsn={}", dlsn); co_return std::unexpected(make_error_condition(volume_error::EMPTY_SLOT)); } - // Cap the gap to prevent unbounded per-write allocation in missing_lsns_. + // Idempotent: slot already successfully written — return snapshot without a second write_slot call. + if (dlsn <= state_.last_append_lsn && !missing_lsns_.contains(dlsn)) { + LOGT("write idempotent: dlsn={} already written", dlsn); + co_return craft::lsn_pair{state_.commit_lsn, state_.last_append_lsn}; + } 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) { + LOGW("write rejected: dlsn={} exceeds safe LSN range", dlsn); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } + // Guard 2: cap the gap to prevent unbounded per-write allocation in missing_lsns_. if (dlsn - state_.last_append_lsn > k_max_ooo_gap) { LOGW("write rejected: dlsn={} too far ahead of last_append_lsn={}", dlsn, state_.last_append_lsn); co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); } - for (int64_t gap = state_.last_append_lsn + 1; gap < dlsn; ++gap) - missing_lsns_.insert(gap); + // Empty-verdicted LSNs in the gap are already resolved — skip them to avoid re-stalling commit advancement. + for (int64_t gap = state_.last_append_lsn + 1; gap < dlsn; ++gap) { + if (!empty_lsns_.contains(gap)) missing_lsns_.insert(gap); + } if ((dlsn > state_.last_append_lsn) || missing_lsns_.contains(dlsn)) missing_lsns_.insert(dlsn); state_.last_append_lsn = std::max(state_.last_append_lsn, dlsn); } // HS_DATA_LINKED: for all_zeros=true skip data-service write; real block allocation wired in commit 2. homestore::multi_blk_id blkid{}; - auto res = co_await journal_->write_slot(dlsn, static_cast< lba_t >(addr), static_cast< lba_count_t >(len), - blkid, all_zeros); + auto res = co_await journal_->write_slot(dlsn, static_cast< lba_t >(addr), static_cast< lba_count_t >(len), blkid, + all_zeros); if (!res) { LOGE("write_slot failed dlsn={} addr={} len={}: {}", dlsn, addr, len, res.error().message()); co_return std::unexpected(res.error()); @@ -194,6 +206,12 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 craft::lsn_pair snapshot; { std::lock_guard lock{missing_mu_}; + // Re-validate term: a new login completing while write_slot was in flight may have run + // truncate(). Discarding this result prevents a ghost journal entry surviving past the truncation. + if (hdr.term != state_.term) { + LOGW("write discarded post-flight: term changed dlsn={}", dlsn); + co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); + } missing_lsns_.erase(dlsn); snapshot = {state_.commit_lsn, state_.last_append_lsn}; } diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index 2150a48..f1cbe20 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -70,8 +70,8 @@ struct JournalSlot { class CraftJournalBackend { public: - virtual async_status write_slot(int64_t lsn, lba_t lba, lba_count_t len, - homestore::multi_blk_id blkid, bool all_zeros) = 0; + virtual async_status write_slot(int64_t lsn, lba_t lba, lba_count_t len, homestore::multi_blk_id blkid, + bool all_zeros) = 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; @@ -195,7 +195,8 @@ class CraftReplDev { void seed_lsns(int64_t last_append, std::initializer_list< int64_t > missing = {}); // Seeds commit_lsn independently of seed_lsns (which only touches last_append + missing). void seed_commit_lsn(int64_t commit); - // Seeds the Empty-verdict set; does not affect missing_lsns_. Clears any prior seeded empties. + // Seeds the Empty-verdict set and removes those LSNs from missing_lsns_ (resolving any gap they + // represented). Replaces any prior seeded empties. apply_sync_rs_commit_lsn (S5) must do the same. void seed_empty(std::initializer_list< int64_t > empty); #endif diff --git a/src/lib/craft/tests/test_craft_write.cpp b/src/lib/craft/tests/test_craft_write.cpp index ef5ad34..18df992 100644 --- a/src/lib/craft/tests/test_craft_write.cpp +++ b/src/lib/craft/tests/test_craft_write.cpp @@ -43,10 +43,9 @@ class MockCraftJournalBackend : public CraftJournalBackend { std::map< int64_t, JournalSlot > slots; std::set< int64_t > fail_lsns; // write_slot returns io_error for these lsns - async_status write_slot(int64_t lsn, lba_t lba, lba_count_t len, - homestore::multi_blk_id /* blkid */, bool all_zeros) override { - if (fail_lsns.count(lsn)) - co_return std::unexpected(std::make_error_condition(std::errc::io_error)); + async_status write_slot(int64_t lsn, lba_t lba, lba_count_t len, homestore::multi_blk_id /* blkid */, + bool all_zeros) override { + if (fail_lsns.count(lsn)) co_return std::unexpected(std::make_error_condition(std::errc::io_error)); slots[lsn] = JournalSlot{lsn, false, all_zeros, lba, len, {}}; co_return ok(); } @@ -143,6 +142,7 @@ TEST_F(CraftWriteTest, WriteSlotFails_LsnRemainsInMissing) { ASSERT_FALSE(r.has_value()); EXPECT_TRUE(dev_->is_missing(0)); EXPECT_EQ(journal_->slot_count(), 0u); + EXPECT_EQ(dev_->last_append_lsn(), 0); // last_append_lsn is set before write_slot; not rolled back on failure } // A write into an Empty-verdicted slot is permanently rejected (Empty beats data). @@ -167,8 +167,8 @@ TEST_F(CraftWriteTest, EmptyVerdictClearsMissingEntry) { // S5 verdicts lsn=5 Empty: must be evicted from missing_lsns_. dev_->seed_empty({5}); - EXPECT_FALSE(dev_->is_missing(5)); // gap resolved - EXPECT_TRUE(dev_->is_empty_slot(5)); // permanently empty + EXPECT_FALSE(dev_->is_missing(5)); // gap resolved + EXPECT_TRUE(dev_->is_empty_slot(5)); // permanently empty // A late write to lsn=5 is still rejected with EMPTY_SLOT. auto r5 = do_write(0, 5); @@ -213,6 +213,65 @@ TEST_F(CraftWriteTest, OutOfOrderWritesLargerGap) { EXPECT_EQ(dev_->missing_count(), 0u); } +// A retry of an already-written dlsn returns success idempotently without a second write_slot call. +// Invariant: after a successful write, dlsn is absent from missing_lsns_; a second write with the +// same dlsn must not violate the pre-insert invariant by calling write_slot without a prior insert. +TEST_F(CraftWriteTest, DuplicateWriteIsIdempotent) { + ASSERT_TRUE(do_write(0, 0).has_value()); + EXPECT_EQ(journal_->slot_count(), 1u); + EXPECT_FALSE(dev_->is_missing(0)); + + // Retry same dlsn (ACK-loss scenario): must return success without dispatching a second write_slot. + auto r2 = do_write(0, 0); + ASSERT_TRUE(r2.has_value()); + EXPECT_EQ(journal_->slot_count(), 1u); // write_slot not called again + EXPECT_FALSE(dev_->is_missing(0)); +} + +// The gap loop must not re-introduce Empty-verdicted LSNs into missing_lsns_. +// Without this filter, an Empty-resolved gap stalls commit advancement just like a real missing entry. +TEST_F(CraftWriteTest, GapLoopSkipsEmptyVerdicts) { + dev_->seed_empty({3}); + ASSERT_FALSE(dev_->is_missing(3)); + + // Writing dlsn=5 on a fresh device triggers the gap loop for lsns 0-4; lsn=3 is Empty-verdicted. + auto r5 = do_write(0, 5); + ASSERT_TRUE(r5.has_value()); + // lsn=3 must NOT have been re-inserted — it is Empty-resolved, not a gap. + EXPECT_FALSE(dev_->is_missing(3)); + EXPECT_TRUE(dev_->is_empty_slot(3)); + EXPECT_EQ(dev_->missing_count(), 4u); // gaps {0,1,2,4}; lsn=3 is Empty, lsn=5 written + EXPECT_TRUE(dev_->is_missing(0)); + EXPECT_TRUE(dev_->is_missing(1)); + EXPECT_TRUE(dev_->is_missing(2)); + EXPECT_TRUE(dev_->is_missing(4)); + EXPECT_FALSE(dev_->is_missing(5)); // successfully written +} + +// Cap fencepost from a seeded non-initial state: verify the > (not >=) boundary and that Guard 1 +// (overflow protection when dlsn is near INT64_MAX) fires correctly too. +TEST_F(CraftWriteTest, GapCapFenceposts) { + dev_->seed_lsns(1'000'000, {}); + + // gap = 1,000,001 (one over cap) → rejected; state unchanged. + ASSERT_FALSE(do_write(0, 2'000'001).has_value()); + EXPECT_EQ(dev_->last_append_lsn(), 1'000'000); + EXPECT_EQ(dev_->missing_count(), 0u); + + // Substantially over cap → also rejected. + ASSERT_FALSE(do_write(0, 3'000'000).has_value()); + EXPECT_EQ(dev_->last_append_lsn(), 1'000'000); + + // Guard 1: dlsn near INT64_MAX triggers the overflow-safe guard before the subtraction. + ASSERT_FALSE(do_write(0, INT64_MAX).has_value()); + EXPECT_EQ(dev_->last_append_lsn(), 1'000'000); + + // Small gap (2) from the seeded state → accepted; only 1 gap entry created. + ASSERT_TRUE(do_write(0, 1'000'002).has_value()); + EXPECT_EQ(dev_->last_append_lsn(), 1'000'002); + EXPECT_EQ(dev_->missing_count(), 1u); // gap: 1,000,001 +} + } // namespace } // namespace homeblocks From 395fd406229c85103bd971100a4ad4a25172979b Mon Sep 17 00:00:00 2001 From: Mehdi Hosseini Date: Wed, 22 Jul 2026 16:48:15 -0700 Subject: [PATCH 4/7] craft: fix block leaks on write_slot failure and stale-term discard; document known risks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two block-leak bugs fixed, three known risks documented, and a full subtask coverage review against SDSTOR-22732. ── Fixes ──────────────────────────────────────────────────────────────── Finding 1 — block leak when write_slot fails After alloc_write_data succeeds, if write_slot subsequently returns an error the allocated blocks were silently dropped with no free path. Fix: after a write_slot failure, call journal_->free_data(blkid) before propagating the error. A free failure is logged but non-fatal. Finding 2 — block leak on post-flight stale-term discard After write_slot succeeds the code rechecks the term under missing_mu_. If the term changed it returned STALE_TERM — but this path had no free call, and you cannot co_await while holding a std::lock_guard (undefined behaviour / potential deadlock). Fix: introduce a stale_post_flight bool, capture the check result under the lock, release the lock, then call free_data without holding missing_mu_. Implementation: added CraftJournalBackend::free_data() pure virtual method (backed by homestore::data_service().async_free_blk in production; stub co_return ok() in all three test mocks). ── Known risks documented (inline comments in craft_repl_dev.cpp) ────── Finding 3 — shutdown correctness: write_async skips the callback when HomeStore is stopping (log_store.cpp:71 returns 0), leaving the LogstoreWriteAwaitable coroutine permanently suspended until process exit. This is a design decision that requires a drain protocol: all in-flight writes must complete before HomeStore shutdown begins. Owners of the shutdown path need to define that protocol explicitly. Finding 4 — silent I/O error loss: write_async fires its callback with the same signature for both success and I/O failure, with no status argument. await_resume cannot distinguish them, so write_slot always returns ok() even when the underlying I/O failed. Fixing this requires a HomeStore API extension. A HomeStore expert should confirm whether write_async can actually call back on I/O error or always terminates the process — if the latter, this risk does not exist in practice. Finding 5 — all_zeros=false with empty data produces a malformed journal entry (all_zeros=0, default-constructed blkid). The client wire rejects this combination, but internal callers and test stubs that pass an empty sg_list without setting all_zeros=true will trigger it. This is a gap to close at a convenient time; a runtime assertion or static_assert on the pre-condition would prevent it. ── Subtask coverage review (SDSTOR-22732) ────────────────────────────── | Ticket | Summary | Status in S2 code | |------------|--------------------------------------|-------------------------------| | SDSTOR-22869 | Term validation before journal append | Done — pre-flight check under missing_mu_ | | SDSTOR-22870 | HS_DATA_LINKED journal append | Done — alloc_write_data + write_slot | | SDSTOR-22871 | Advance last_append_lsn on success | Done — see note below | | SDSTOR-22872 | Out-of-order slot arrivals | Done — missing_lsns_ overlay | | SDSTOR-22873 | Zero-copy data path | Done — sg_list passed through | | SDSTOR-22874 | Quorum-ack; no LBA index update | Done — server ACKs after local journal commit; no LBA index touched | | SDSTOR-22904 | all_zeros WRITE_ZEROES path | Done — skips alloc, journals metadata-only slot | Flags for reviewers: SDSTOR-22870 note — the ticket description states "a crash between the data-service write and journal append leaves only uncommitted blocks, which HomeStore recovery reclaims automatically." If this auto-reclaim is real, the free_data calls added here are defensive but not strictly necessary for crash-safety. If it is aspirational or incorrect, they are the only reclaim path. This should be confirmed with the HomeStore team before closing SDSTOR-22870. SDSTOR-22871 note — the ticket says "after a successful append". This implementation advances last_append_lsn BEFORE write_slot (in the pre-insert block) and does NOT roll it back on failure. The failing dlsn stays in missing_lsns_ so the gap is tracked correctly. This is an intentional design choice; reviewers should confirm it is acceptable or request a post-success update with rollback on failure. S3 design note — when apply_internal_login is implemented, it must update state_.term under missing_mu_ to avoid a data race with write()'s pre-flight and post-flight term checks. --- conanfile.py | 4 +- src/lib/craft/craft_repl_dev.cpp | 155 ++++++++++++++++-- src/lib/craft/craft_repl_dev.hpp | 10 +- .../craft/tests/test_craft_peer_exchange.cpp | 6 + src/lib/craft/tests/test_craft_truncate.cpp | 6 + src/lib/craft/tests/test_craft_write.cpp | 9 + 6 files changed, 169 insertions(+), 21 deletions(-) diff --git a/conanfile.py b/conanfile.py index bff412c..a1ce69c 100644 --- a/conanfile.py +++ b/conanfile.py @@ -10,7 +10,7 @@ class HomeBlocksConan(ConanFile): name = "homeblocks" - version = "6.0.4" + version = "6.0.5" homepage = "https://github.com/eBay/HomeBlocks" description = "Block Store built on HomeStore" @@ -48,7 +48,7 @@ def configure(self): def build_requirements(self): self.test_requires("gtest/[^1.17]") - self.test_requires("ublkpp/[^0.35]@oss/main") + self.test_requires("ublkpp/[^0.36]@oss/main") def requirements(self): self.requires("homestore/[^8.0]@oss/dev", transitive_headers=True) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index 8c2c934..91e8b2e 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -15,24 +15,99 @@ #include "craft_repl_dev.hpp" -#include // home_log_store, logstore_seq_num_t +#include +#include + +#include // data_service(), async_alloc_write, blk_alloc_hints +#include // home_log_store, logstore_seq_num_t, log_write_comp_cb_t namespace homeblocks { -// ─── HomeStore journal backend ──────────────────────────────────────────────── +// ─── Journal entry on-disk format ───────────────────────────────────────────── +// +// Each log slot is: [CraftJournalEntry header][serialized multi_blk_id bytes]. +// The payload (HS_DATA_LINKED) is written directly to the data service; only the +// block reference is stored here. + +#pragma pack(1) +struct CraftJournalEntry { + lba_t lba; + lba_count_t len; + uint8_t all_zeros; +}; +#pragma pack() + +// ─── Callback-to-coroutine bridge ───────────────────────────────────────────── // -// Production backend: wraps a HomeStore home_log_store. write_slot / read_slot -// are stubs implemented in S2 (PR #165). truncate_to() is implemented here (S4). +// home_log_store::write_async is callback-based. This C++20 awaitable bridges it +// to co_await: await_suspend calls write_async and captures the coroutine handle; +// the callback resumes it when the write completes. +// +// blob_ lives in the coroutine frame for the full suspension, so write_async +// always sees a valid buffer. +// +// KNOWN RISK (shutdown): write_async returns 0 without calling `cb` when is_stopping() is true +// (HomeStore/src/lib/logstore/log_store.cpp:71). The coroutine is permanently suspended until the +// process exits. Callers must drain all in-flight writes before initiating HomeStore shutdown. +// +// KNOWN GAP (I/O errors): write_async fires the same callback for both success and I/O failure +// with no status argument. await_resume cannot surface the error, so write_slot always returns +// ok() regardless of the underlying I/O result. A future fix requires a HomeStore API extension. + +struct LogstoreWriteAwaitable { + homestore::home_log_store* store_; + homestore::logstore_seq_num_t seq_; + sisl::io_blob_safe blob_; + + bool await_ready() const noexcept { return false; } + + template < typename H > + void await_suspend(H h) noexcept { + store_->write_async( + seq_, blob_, nullptr, + [h](homestore::logstore_seq_num_t, sisl::io_blob&, homestore::logdev_key, void*) mutable { h.resume(); }); + } + + void await_resume() const noexcept {} +}; + +// ─── HomeStore journal backend ───────────────────────────────────────────────── +// +// Production backend: wraps a HomeStore home_log_store. class HomeStoreCraftJournalBackend : public CraftJournalBackend { public: - explicit HomeStoreCraftJournalBackend(shared< homestore::home_log_store > logstore) : - logstore_{std::move(logstore)} {} + explicit HomeStoreCraftJournalBackend(shared< homestore::home_log_store > logstore, uint64_t vol_ordinal) : + logstore_{std::move(logstore)}, vol_ordinal_{vol_ordinal} {} + + // 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. + // Returns the allocated multi_blk_id on success. + async_result< homestore::multi_blk_id > alloc_write_data(sisl::sg_list const& data, + lba_count_t /* len */) override { + homestore::blk_alloc_hints hints; + hints.application_hint = vol_ordinal_; + homestore::multi_blk_id blkid{}; + auto res = co_await homestore::data_service().async_alloc_write(data, hints, blkid); + if (!res) { + LOGE("async_alloc_write failed: {}", res.error().message()); + co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + } + co_return blkid; + } - async_status write_slot(int64_t lsn, lba_t /* lba */, lba_count_t /* len */, homestore::multi_blk_id /* blkid */, - bool /* all_zeros */) override { - LOGW("HomeStoreCraftJournalBackend::write_slot lsn={} not yet implemented", lsn); - co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); + // Serialize the journal entry (header + blkid) and write it to the log store. + async_status write_slot(int64_t lsn, lba_t lba, lba_count_t len, homestore::multi_blk_id blkid, + bool all_zeros) override { + CraftJournalEntry hdr{lba, len, static_cast< uint8_t >(all_zeros)}; + uint32_t blkid_sz = blkid.serialized_size(); + sisl::io_blob_safe blob{static_cast< uint32_t >(sizeof(CraftJournalEntry)) + blkid_sz}; + std::memcpy(blob.bytes(), &hdr, sizeof(CraftJournalEntry)); + sisl::blob blkid_blob = blkid.serialize(); // non-owning view — copy before blkid goes out of scope + std::memcpy(blob.bytes() + sizeof(CraftJournalEntry), blkid_blob.cbytes(), blkid_sz); + co_await LogstoreWriteAwaitable{logstore_.get(), static_cast< homestore::logstore_seq_num_t >(lsn), + std::move(blob)}; + co_return ok(); } async_result< JournalSlot > read_slot(int64_t lsn) override { @@ -48,12 +123,23 @@ class HomeStoreCraftJournalBackend : public CraftJournalBackend { co_return ok(); } + async_status free_data(homestore::multi_blk_id blkid) override { + auto res = co_await homestore::data_service().async_free_blk(blkid); + if (!res) { + LOGE("async_free_blk 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_; }; -unique< CraftJournalBackend > make_homestore_journal_backend(shared< homestore::home_log_store > logstore) { - return std::make_unique< HomeStoreCraftJournalBackend >(std::move(logstore)); +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); } // ─── constructor ────────────────────────────────────────────────────────────── @@ -194,27 +280,60 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 state_.last_append_lsn = std::max(state_.last_append_lsn, dlsn); } - // HS_DATA_LINKED: for all_zeros=true skip data-service write; real block allocation wired in commit 2. + // HS_DATA_LINKED: allocate blocks and write payload before journalling the block reference. + // all_zeros=true (WRITE_ZEROES/unmap) and empty data (test stubs) skip the data-service write. + // + // LATENT RISK: if the caller sends all_zeros=false with data.size==0 (e.g., a test stub with an + // empty sg_list but all_zeros not set), the journal entry records all_zeros=0 with a default- + // constructed blkid. On replay this produces a read against a zero blkid. The client wire rejects + // this, but internal and test paths must set all_zeros=true whenever no payload is provided. homestore::multi_blk_id blkid{}; + bool blkid_allocated = false; + if (!all_zeros && data.size > 0) { + auto alloc_res = co_await journal_->alloc_write_data(data, static_cast< lba_count_t >(len)); + if (!alloc_res) { + LOGE("alloc_write_data failed dlsn={}: {}", dlsn, alloc_res.error().message()); + co_return std::unexpected(alloc_res.error()); + } + blkid = *alloc_res; + blkid_allocated = true; + } auto res = co_await journal_->write_slot(dlsn, static_cast< lba_t >(addr), static_cast< lba_count_t >(len), blkid, all_zeros); if (!res) { LOGE("write_slot failed dlsn={} addr={} len={}: {}", dlsn, addr, len, res.error().message()); + if (blkid_allocated) { + if (auto fr = co_await journal_->free_data(blkid); !fr) + LOGE("free_data failed after write_slot failure dlsn={}: {}", dlsn, fr.error().message()); + } co_return std::unexpected(res.error()); } + // Re-validate term: a new login completing while write_slot was in flight may have run + // truncate(). Discarding this result prevents a ghost journal entry surviving past the truncation. + // The lock is taken to snapshot the term and update state_, then released BEFORE free_data so we + // do not co_await while holding missing_mu_. + bool stale_post_flight = false; craft::lsn_pair snapshot; { std::lock_guard lock{missing_mu_}; - // Re-validate term: a new login completing while write_slot was in flight may have run - // truncate(). Discarding this result prevents a ghost journal entry surviving past the truncation. if (hdr.term != state_.term) { LOGW("write discarded post-flight: term changed dlsn={}", dlsn); - co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); + stale_post_flight = true; + } else { + missing_lsns_.erase(dlsn); + snapshot = {state_.commit_lsn, state_.last_append_lsn}; } - missing_lsns_.erase(dlsn); - snapshot = {state_.commit_lsn, state_.last_append_lsn}; } + + if (stale_post_flight) { + if (blkid_allocated) { + if (auto fr = co_await journal_->free_data(blkid); !fr) + LOGE("free_data failed after post-flight stale-term discard dlsn={}: {}", dlsn, fr.error().message()); + } + co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); + } + LOGT("write ok dlsn={} addr={} len={} all_zeros={}", dlsn, addr, len, all_zeros); co_return snapshot; } diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index f1cbe20..b04bf88 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -70,17 +70,25 @@ struct JournalSlot { class CraftJournalBackend { public: + // Allocate blocks and write the data payload. Called BEFORE write_slot for non-zero writes. + // all_zeros=true and empty data bypass this; write_slot receives an empty multi_blk_id. + virtual async_result< homestore::multi_blk_id > alloc_write_data(sisl::sg_list const& data, lba_count_t len) = 0; virtual async_status write_slot(int64_t lsn, lba_t lba, lba_count_t len, homestore::multi_blk_id blkid, bool all_zeros) = 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; 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. -unique< CraftJournalBackend > make_homestore_journal_backend(shared< homestore::home_log_store > logstore); +unique< CraftJournalBackend > make_homestore_journal_backend(shared< homestore::home_log_store > logstore, + uint64_t vol_ordinal); // ─── CraftReplDev ───────────────────────────────────────────────────────────── // diff --git a/src/lib/craft/tests/test_craft_peer_exchange.cpp b/src/lib/craft/tests/test_craft_peer_exchange.cpp index a9e7870..177b59e 100644 --- a/src/lib/craft/tests/test_craft_peer_exchange.cpp +++ b/src/lib/craft/tests/test_craft_peer_exchange.cpp @@ -53,6 +53,11 @@ class MockCraftJournalBackend : public CraftJournalBackend { std::map< int64_t, JournalSlot > slots; std::optional< int64_t > fail_on_read; // if set, read_slot for that LSN returns io_error + async_result< homestore::multi_blk_id > alloc_write_data(sisl::sg_list const& /* data */, + lba_count_t /* len */) override { + co_return homestore::multi_blk_id{}; + } + async_status write_slot(int64_t, lba_t, lba_count_t, homestore::multi_blk_id, bool) override { co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); } @@ -67,6 +72,7 @@ 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(); } }; // ── test fixture ───────────────────────────────────────────────────────────── diff --git a/src/lib/craft/tests/test_craft_truncate.cpp b/src/lib/craft/tests/test_craft_truncate.cpp index bac197b..ad39ecb 100644 --- a/src/lib/craft/tests/test_craft_truncate.cpp +++ b/src/lib/craft/tests/test_craft_truncate.cpp @@ -45,6 +45,11 @@ class MockCraftJournalBackend : public CraftJournalBackend { bool should_fail{false}; int64_t truncated_to{INT64_MIN}; + async_result< homestore::multi_blk_id > alloc_write_data(sisl::sg_list const& /* data */, + lba_count_t /* len */) override { + co_return homestore::multi_blk_id{}; + } + async_status write_slot(int64_t, lba_t, lba_count_t, homestore::multi_blk_id, bool) override { co_return std::unexpected(std::make_error_condition(std::errc::not_supported)); } @@ -58,6 +63,7 @@ class MockCraftJournalBackend : public CraftJournalBackend { if (should_fail) co_return std::unexpected(std::make_error_condition(std::errc::io_error)); co_return ok(); } + async_status free_data(homestore::multi_blk_id) override { co_return ok(); } }; // ── test fixture ───────────────────────────────────────────────────────────── diff --git a/src/lib/craft/tests/test_craft_write.cpp b/src/lib/craft/tests/test_craft_write.cpp index 18df992..4c75c0c 100644 --- a/src/lib/craft/tests/test_craft_write.cpp +++ b/src/lib/craft/tests/test_craft_write.cpp @@ -42,6 +42,13 @@ class MockCraftJournalBackend : public CraftJournalBackend { public: std::map< int64_t, JournalSlot > slots; std::set< int64_t > fail_lsns; // write_slot returns io_error for these lsns + int alloc_write_data_calls{0}; + + async_result< homestore::multi_blk_id > alloc_write_data(sisl::sg_list const& /* data */, + lba_count_t /* len */) override { + ++alloc_write_data_calls; + co_return homestore::multi_blk_id{}; + } async_status write_slot(int64_t lsn, lba_t lba, lba_count_t len, homestore::multi_blk_id /* blkid */, bool all_zeros) override { @@ -58,6 +65,7 @@ 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(); } bool has_slot(int64_t lsn) const { return slots.count(lsn) > 0; } size_t slot_count() const { return slots.size(); } @@ -133,6 +141,7 @@ TEST_F(CraftWriteTest, AllZerosWrite) { EXPECT_EQ(dev_->missing_count(), 0u); ASSERT_TRUE(journal_->has_slot(0)); EXPECT_TRUE(journal_->slots[0].all_zeros); + EXPECT_EQ(journal_->alloc_write_data_calls, 0); // no block allocation for all_zeros writes } // A failed write_slot must leave the lsn in missing_lsns_ (pre-insert invariant). From f3b5e13ba33709981687972ce48ad096d4cbe534 Mon Sep 17 00:00:00 2001 From: Mehdi Hosseini Date: Wed, 22 Jul 2026 17:47:19 -0700 Subject: [PATCH 5/7] =?UTF-8?q?craft:=20address=20Copilot=20review=20comme?= =?UTF-8?q?nts=20=E2=80=94=20error=20codes=20and=20missing=20alloc=20path?= =?UTF-8?q?=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment 2 (lines 246, 266, 271): wrong error code for input validation. home_blocks.hpp lines 68-70 document that anything with a standard equivalent is returned as std::make_error_condition(std::errc::*) rather than a volume_error. Three sites were returning INTERNAL_ERROR for rejected client input, which makes it impossible for a caller to distinguish "bad request" from "server internal fault": - dlsn < 0 → std::errc::invalid_argument - dlsn > INT64_MAX-cap → std::errc::invalid_argument - dlsn - last > cap → std::errc::value_too_large Comment 3 (test line 87): alloc_write_data path not exercised. do_write() passes an empty sg_list (data.size==0), so the production guard (!all_zeros && data.size > 0) always skips allocation. This meant blkid_allocated was always false and the Finding 1 fix (free_data after write_slot failure) was untested. Changes: - MockCraftJournalBackend: add fail_alloc flag and free_data_calls counter - Add do_write_with_data() helper (data.size=4096, all_zeros=false) - Add NonZeroWriteCallsAllocWriteData: verifies alloc_write_data called once - Add AllocWriteDataFails_WriteRejected: alloc failure, no write_slot call, no free_data call (nothing was allocated), lsn stays in missing set - Add WriteSlotFailsWithData_BlocksFreed: verifies free_data_calls==1 when write_slot fails after a successful alloc (Finding 1 fix coverage) Comment 1 (line 302): identified Finding 5 (all_zeros=false + empty data produces malformed journal entry). This gap was pre-documented in the prior commit. The suggested fix (auto-derive all_zeros from data.size==0) is not applied — it would silently coerce protocol violations into zero writes. The correct closure is a precondition assertion at write() entry, deferred to a follow-up. --- src/lib/craft/craft_repl_dev.cpp | 6 +-- src/lib/craft/tests/test_craft_write.cpp | 50 +++++++++++++++++++++++- 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index 91e8b2e..e2bbbc6 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -242,7 +242,7 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 sisl::sg_list data, bool all_zeros) { if (dlsn < 0) { LOGW("write rejected: invalid dlsn={}", dlsn); - co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); } { @@ -265,12 +265,12 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 // Guard 1: prevent signed overflow in the gap subtraction below (dlsn near INT64_MAX). if (dlsn > INT64_MAX - k_max_ooo_gap) { LOGW("write rejected: dlsn={} exceeds safe LSN range", dlsn); - co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); } // Guard 2: cap the gap to prevent unbounded per-write allocation in missing_lsns_. if (dlsn - state_.last_append_lsn > k_max_ooo_gap) { LOGW("write rejected: dlsn={} too far ahead of last_append_lsn={}", dlsn, state_.last_append_lsn); - co_return std::unexpected(make_error_condition(volume_error::INTERNAL_ERROR)); + co_return std::unexpected(make_error_condition(std::errc::value_too_large)); } // Empty-verdicted LSNs in the gap are already resolved — skip them to avoid re-stalling commit advancement. for (int64_t gap = state_.last_append_lsn + 1; gap < dlsn; ++gap) { diff --git a/src/lib/craft/tests/test_craft_write.cpp b/src/lib/craft/tests/test_craft_write.cpp index 4c75c0c..93fdd80 100644 --- a/src/lib/craft/tests/test_craft_write.cpp +++ b/src/lib/craft/tests/test_craft_write.cpp @@ -42,11 +42,14 @@ class MockCraftJournalBackend : public CraftJournalBackend { public: std::map< int64_t, JournalSlot > slots; std::set< int64_t > fail_lsns; // write_slot returns io_error for these lsns + bool fail_alloc{false}; // alloc_write_data returns io_error when set int alloc_write_data_calls{0}; + int free_data_calls{0}; async_result< homestore::multi_blk_id > alloc_write_data(sisl::sg_list const& /* data */, lba_count_t /* len */) override { ++alloc_write_data_calls; + if (fail_alloc) co_return std::unexpected(std::make_error_condition(std::errc::io_error)); co_return homestore::multi_blk_id{}; } @@ -65,7 +68,10 @@ class MockCraftJournalBackend : public CraftJournalBackend { } async_status truncate_to(int64_t) override { co_return ok(); } - async_status free_data(homestore::multi_blk_id) override { co_return ok(); } + async_status free_data(homestore::multi_blk_id) override { + ++free_data_calls; + co_return ok(); + } bool has_slot(int64_t lsn) const { return slots.count(lsn) > 0; } size_t slot_count() const { return slots.size(); } @@ -86,6 +92,15 @@ class CraftWriteTest : public ::testing::Test { dev_->write(craft::client_hdr{term, -1, -1}, lsn, 0, 4096, sisl::sg_list{}, all_zeros)); } + // 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. + auto do_write_with_data(uint64_t term, int64_t lsn) { + sisl::sg_list data; + data.size = 4096; + return homeblocks::detail::sync_get( + dev_->write(craft::client_hdr{term, -1, -1}, lsn, 0, 4096, std::move(data), false)); + } + MockCraftJournalBackend* journal_{nullptr}; std::unique_ptr< CraftReplDev > dev_; }; @@ -281,6 +296,39 @@ TEST_F(CraftWriteTest, GapCapFenceposts) { EXPECT_EQ(dev_->missing_count(), 1u); // gap: 1,000,001 } +// A write with data.size > 0 (non-zero content) must call alloc_write_data exactly once. +// This verifies the HS_DATA_LINKED branch is entered when the payload is non-empty. +TEST_F(CraftWriteTest, NonZeroWriteCallsAllocWriteData) { + auto r = do_write_with_data(0, 0); + ASSERT_TRUE(r.has_value()); + EXPECT_EQ(journal_->alloc_write_data_calls, 1); + EXPECT_TRUE(journal_->has_slot(0)); +} + +// alloc_write_data failure propagates; write_slot must not be called and no block is freed +// (there is nothing to free — allocation never succeeded). +TEST_F(CraftWriteTest, AllocWriteDataFails_WriteRejected) { + journal_->fail_alloc = true; + auto r = do_write_with_data(0, 0); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(journal_->alloc_write_data_calls, 1); + EXPECT_EQ(journal_->slot_count(), 0u); // write_slot not called + EXPECT_EQ(journal_->free_data_calls, 0); // nothing to free + EXPECT_TRUE(dev_->is_missing(0)); // pre-insert invariant holds +} + +// write_slot failure after a successful alloc_write_data must call free_data exactly once +// (Finding 1 fix). This test exercises blkid_allocated=true path. +TEST_F(CraftWriteTest, WriteSlotFailsWithData_BlocksFreed) { + journal_->fail_lsns.insert(0); + auto r = do_write_with_data(0, 0); + ASSERT_FALSE(r.has_value()); + EXPECT_EQ(journal_->alloc_write_data_calls, 1); // alloc succeeded + EXPECT_EQ(journal_->slot_count(), 0u); // write_slot returned error + EXPECT_EQ(journal_->free_data_calls, 1); // blocks freed after write_slot failure + EXPECT_TRUE(dev_->is_missing(0)); // pre-insert invariant holds +} + } // namespace } // namespace homeblocks From 91dbc6a901f9518988f014fe746d4a1c4d6d7989 Mon Sep 17 00:00:00 2001 From: Mehdi Hosseini Date: Wed, 22 Jul 2026 18:25:05 -0700 Subject: [PATCH 6/7] =?UTF-8?q?craft:=20address=20second=20Copilot=20revie?= =?UTF-8?q?w=20=E2=80=94=20docstring,=20precondition=20assert,=20include?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment 1 (craft_repl_dev.hpp): stale docstring said 'An EMPTY data is a zero write' — predated the all_zeros flag. Replaced with the correct semantics: all_zeros=true is the zero-write signal; all_zeros=false requires non-empty data. Also promotes Finding 5 from a comment to an enforced precondition. craft_repl_dev.cpp: add RELEASE_ASSERT(all_zeros || data.size > 0) after the dlsn guard. This is the Finding 5 gap called out in the commit message; it catches all_zeros=false with empty data at the call site rather than silently journalling a zero blkid that replay cannot resolve. tests/test_craft_write.cpp: do_write() helper defaulted all_zeros=false with an empty sg_list, which now violates the precondition. Changed default to all_zeros=true (all state-management tests use do_write() to exercise missing-set, gap-loop, and term-check logic — not the data path — so treating them as zero-writes is semantically correct). Added #include (Comment 3: std::set used by fail_lsns but the TU relied on transitive inclusion from craft_repl_dev.hpp). Comment 2 (craft_repl_dev.cpp line 302) — no change: the bytes-vs-LBAs concern is not a bug. alloc_write_data ignores the len parameter entirely (HomeStoreCraftJournalBackend marks it /* len */ and derives size from the sg_list). CraftJournalEntry stores raw wire byte addr/len by design: hb_internal.hpp lines 64-68 documents 'lba_t identical to the retired craft:: aliases, so an LBA meeting a byte range interops seamlessly.' The byte-to-block conversion is deferred to S3's apply_sync_rs_commit_lsn. --- src/lib/craft/craft_repl_dev.cpp | 2 ++ src/lib/craft/craft_repl_dev.hpp | 5 +++-- src/lib/craft/tests/test_craft_write.cpp | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index e2bbbc6..b2f25b9 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -244,6 +244,8 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 LOGW("write rejected: invalid dlsn={}", dlsn); co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); } + RELEASE_ASSERT(all_zeros || data.size > 0, + "write: all_zeros=false requires non-empty data; set all_zeros=true for WRITE_ZEROES"); { std::lock_guard lock{missing_mu_}; diff --git a/src/lib/craft/craft_repl_dev.hpp b/src/lib/craft/craft_repl_dev.hpp index b04bf88..ccaa42c 100644 --- a/src/lib/craft/craft_repl_dev.hpp +++ b/src/lib/craft/craft_repl_dev.hpp @@ -121,8 +121,9 @@ 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). An EMPTY `data` is a zero write (WRITE_ZEROES/unmap over [addr, addr+len)). The ack returns the - // achieved {commit_lsn, last_append_lsn} snapshotted with the append -- every CRAFT IO response piggybacks the + // 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 + // {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); diff --git a/src/lib/craft/tests/test_craft_write.cpp b/src/lib/craft/tests/test_craft_write.cpp index 93fdd80..671ba2b 100644 --- a/src/lib/craft/tests/test_craft_write.cpp +++ b/src/lib/craft/tests/test_craft_write.cpp @@ -25,6 +25,7 @@ #include #include +#include #include #include "craft/craft_repl_dev.hpp" @@ -87,7 +88,7 @@ class CraftWriteTest : public ::testing::Test { dev_ = std::make_unique< CraftReplDev >(volume_id_t{}, std::move(mock)); } - auto do_write(uint64_t term, int64_t lsn, bool all_zeros = false) { + auto do_write(uint64_t term, int64_t lsn, bool all_zeros = true) { return homeblocks::detail::sync_get( dev_->write(craft::client_hdr{term, -1, -1}, lsn, 0, 4096, sisl::sg_list{}, all_zeros)); } From 1b94959ee2b6a750edce1d86e3c5b1c5f175ffd2 Mon Sep 17 00:00:00 2001 From: Mehdi Hosseini Date: Thu, 23 Jul 2026 14:33:27 -0700 Subject: [PATCH 7/7] craft: extend DuplicateWriteIsIdempotent with cross-type same-dLSN cases Add two cross-type idempotent scenarios to the existing test: - data write first, then all_zeros retry at same dLSN: retry discarded, slot type (data) preserved, alloc_write_data not called again - all_zeros write first, then data retry at same dLSN: retry discarded, slot type (zero) preserved, idempotent path skips alloc_write_data Covers the AC requirement that the original slot is immutable once written, regardless of the retry's all_zeros flag. --- src/lib/craft/tests/test_craft_write.cpp | 28 +++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/lib/craft/tests/test_craft_write.cpp b/src/lib/craft/tests/test_craft_write.cpp index 671ba2b..773a226 100644 --- a/src/lib/craft/tests/test_craft_write.cpp +++ b/src/lib/craft/tests/test_craft_write.cpp @@ -241,16 +241,42 @@ TEST_F(CraftWriteTest, OutOfOrderWritesLargerGap) { // A retry of an already-written dlsn returns success idempotently without a second write_slot call. // Invariant: after a successful write, dlsn is absent from missing_lsns_; a second write with the // same dlsn must not violate the pre-insert invariant by calling write_slot without a prior insert. +// Also covers cross-type retries: the original slot type (all_zeros vs data) is preserved; the +// retry type is irrelevant — the slot is immutable once written. TEST_F(CraftWriteTest, DuplicateWriteIsIdempotent) { + // same-type: all_zeros then all_zeros at dLSN=0 ASSERT_TRUE(do_write(0, 0).has_value()); EXPECT_EQ(journal_->slot_count(), 1u); EXPECT_FALSE(dev_->is_missing(0)); - // Retry same dlsn (ACK-loss scenario): must return success without dispatching a second write_slot. auto r2 = do_write(0, 0); ASSERT_TRUE(r2.has_value()); EXPECT_EQ(journal_->slot_count(), 1u); // write_slot not called again EXPECT_FALSE(dev_->is_missing(0)); + + // cross-type: data write first, then all_zeros retry at dLSN=1 — all_zeros is discarded + auto r_data = do_write_with_data(0, 1); + ASSERT_TRUE(r_data.has_value()); + EXPECT_EQ(journal_->slot_count(), 2u); + 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); + 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); + ASSERT_TRUE(r_zero2.has_value()); + EXPECT_EQ(journal_->slot_count(), 3u); + EXPECT_EQ(journal_->alloc_write_data_calls, 1); // all_zeros never allocates + EXPECT_TRUE(journal_->slots[2].all_zeros); // zero slot preserved + + auto r_data2 = do_write_with_data(0, 2); + ASSERT_TRUE(r_data2.has_value()); + EXPECT_EQ(journal_->slot_count(), 3u); // write_slot not called again + EXPECT_EQ(journal_->alloc_write_data_calls, 1); // idempotent path skips alloc_write_data } // The gap loop must not re-introduce Empty-verdicted LSNs into missing_lsns_.