craft: S2 write path — HS_DATA_LINKED journal append with full correctness hardening

[shosseinimotlagh]

Summary

Implements SDSTOR-22732 S2: the CRAFT write path on the HomeBlocks replica. A client-assigned dLSN arrives, blocks are allocated via the HS_DATA_LINKED pattern (payload written directly to data-service chunks, never through the RAFT/journal log), a metadata-only journal slot is appended, and the achieved watermarks {commit_lsn, last_append_lsn} are returned to the client. Zero writes (all_zeros=true / WRITE_ZEROES) skip block allocation entirely.

What's implemented

Write path (CraftReplDev::write)

• Term fencing under missing_mu_ (rejects with STALE_TERM on mismatch).
• Empty-verdict tombstone: a write into a slot already verdicted Empty is rejected with EMPTY_SLOT (new volume_error value).
• Out-of-order arrival via missing_lsns_: gaps are tracked and cleared as they fill; retries of an already-written dLSN return the current watermark idempotently instead of re-appending.
• Two independent caps against unbounded growth: a per-write gap cap (bounds one write's own contribution) and a cumulative cap on missing_lsns_'s total size (bounds sustained forward-walking that stays within the per-write cap on every individual call).
• all_zeros/data-size validated symmetrically in both directions (a data write requires a payload; a zero write requires none) — a malformed combination is rejected, never silently accepted or dropped.

On-disk journal format (CraftJournalEntry)

• {magic, version, term, lsn, lba, len, all_zeros}, packed and size-locked with a static_assert so a layout change is a compile error, not a silent recovery bug.
• No CRC field: HomeStore's log-group header already checksums the entire record body on every append and verifies it on read/recovery replay (verified directly against HomeStore's dev/v8.x source) — a CRAFT-level CRC would be redundant.
• lba/len are explicitly byte-addressed (not block units); this backend's own JournalSlot fields are named lba_off_bytes/len_bytes to keep that visible at the type level, since craft_client's equivalent struct uses the same field names for block units.

HomeStoreCraftJournalBackend (the production backend)

• write_slot bridges HomeStore's callback-based write_async to a coroutine via a lock-free value_awaitable, safe under both of HomeStore's real completion paths: synchronous completion under LogDev::m_flush_mtx (the production configuration) and INLINE completion firing before await_suspend returns (used by solo_repl_dev).
• Guards the one HomeStore-side failure that does carry a return-value signal (log store/logdev shutting down) and fails the write cleanly instead of hanging forever; the other failure trigger (a journal I/O error) has no such signal today and is tracked as a HomeStore-side gap (see Known risks).
• Defensive DEBUG_ASSERT_EQ on the serialized multi_blk_id size before the memcpy that packs it into the journal blob.

Test coverage

• test_craft_write.cpp (24 tests): in-order/out-of-order writes, term rejection, idempotent retry, both gap-cap guards including exact fenceposts and the corner cases that expose why a naive implementation of each guard is wrong (a strictly in-order write must stay exempt from the cumulative cap; a gap-filling write must too), Empty-verdict interaction with the gap loop, all_zeros validation in both directions, zero-copy verification (a real buffer's pointer, not a copy, must reach alloc_write_data), and the on-disk term round-trip.
• test_craft_homestore_backend.cpp (3 tests, new): drives HomeStoreCraftJournalBackend directly against a real HomeStore log store — the only place in this suite that exercises the production backend instead of a mock. Covers write_slot's completion bridge under INLINE flush mode, truncate_to's rollback against a real logdev, and alloc_write_data's failure path.
• test_craft_truncate.cpp (8 tests) and test_craft_peer_exchange.cpp (13 tests): mocks updated to the current write_slot signature (term + multi_blk_id + all_zeros).

All CRAFT test binaries pass on the remote build server (Debug build).

Bugs found and fixed along the way

• Block leak when write_slot fails after alloc_write_data succeeds — free_data now called before propagating the error.
• Several correctness issues from adversarial review: a state_.term data race outside the lock, a negative-dLSN bypass of the pre-insert invariant, signed overflow in the gap-cap subtraction near INT64_MAX, a duplicate-write path that could call write_slot without the dLSN present in missing_lsns_, the gap-fill loop re-inserting already-Empty-verdicted LSNs (which would permanently stall commit advancement), and a ghost journal entry surviving a login-truncate race (closed by removing the unreachable free-on-stale-term path entirely rather than patching around it).
• write_async's return value was being misread as "non-positive means shutting down" — HomeStore's log-dev actually returns its internal monotonic index on success, which legitimately starts at 0 for the first write to a fresh logdev. Found by the new real-backend test (which exercises an actual fresh logdev); fixed to only treat a negative return as the shutdown signal.
• VolumeChunkSelector::select_chunk dereferenced a null chunk-info slot for an unregistered volume ordinal with no null check — a real, previously-latent crash in shared volume infrastructure, unrelated to CRAFT, found by the same new test and fixed with a one-line guard (volume_chunk_selector.cpp).

Known risks / tracked follow-ups

• write_async completion can be permanently lost. Two independent triggers collapse to the same failure mode (the coroutine never resumes): the log store/logdev shutting down (now guarded here — the write fails cleanly instead of hanging) and a journal I/O error (HomeStore's own flush-failure path returns without ever invoking the completion callback — no return-value signal exists for this one). The needed HomeStore-side fix is to propagate the error into the completion path itself, not add a status argument to a callback that currently isn't even invoked on this path. Tracked; an interim timeout on the co_await is the mitigation until that lands.
• EMPTY_SLOT has no wire representation. craft_client's craft_error enum has no Empty-slot-equivalent value, so this can't survive translation at the CraftConnector boundary yet. Cross-repo, out of scope for this story; tracked as a sub-task under the CraftConnector story (S9).
• last_append_lsn is advanced before write_slot runs and not rolled back on failure. Intentional, documented in-source and grounded in CRAFT-Design's own recovery-watermark argument (the design already tolerates false-include of a non-durable LSN into the login watermark as benign, reserving false-exclude as the only catastrophic case). To decide in a future PR.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 50.00000% with 9 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (dev/v6.x@9fce458). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/lib/craft/craft_repl_dev.cpp	56.25%	1 Missing and 6 partials ⚠️
src/include/homeblks/home_blocks.hpp	0.00%	1 Missing ⚠️
src/lib/volume/volume_chunk_selector.cpp	0.00%	0 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##             dev/v6.x     #171   +/-   ##
===========================================
  Coverage            ?   45.60%           
===========================================
  Files               ?       18           
  Lines               ?     1035           
  Branches            ?      451           
===========================================
  Hits                ?      472           
  Misses              ?      262           
  Partials            ?      301           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Implements the CRAFT S2 write path for HomeBlocks replicas using an HS_DATA_LINKED scheme (payload written via HomeStore data service, journal stores metadata + block reference), adds correctness hardening around term/idempotency/gap handling, and extends unit tests to cover key write-path behaviors.

Changes:

• Introduces HS_DATA_LINKED journaling by splitting payload handling into alloc_write_data(...) + metadata-only write_slot(...), plus free_data(...) for cleanup on failures/discards.
• Hardens CraftReplDev::write() with term fencing under mutex, idempotent retry handling, out-of-order gap capping, and Empty-slot rejection semantics.
• Expands CRAFT unit tests for all_zeros writes, missing-set invariants, Empty-slot interactions, and gap-cap/overflow guards; bumps package version and test dependency.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/lib/craft/tests/test_craft_write.cpp	Adds write-path tests and updates the journal mock for the new backend interface.
src/lib/craft/tests/test_craft_truncate.cpp	Updates truncate tests’ journal mock to satisfy the new backend interface.
src/lib/craft/tests/test_craft_peer_exchange.cpp	Updates peer-exchange tests’ journal mock to satisfy the new backend interface.
src/lib/craft/craft_repl_dev.hpp	Updates the journal backend abstraction and CraftReplDev::write() signature to support HS_DATA_LINKED + all_zeros.
src/lib/craft/craft_repl_dev.cpp	Implements HS_DATA_LINKED write_slot for the HomeStore backend and wires the S2 write path logic in CraftReplDev::write().
src/include/homeblks/home_blocks.hpp	Adds volume_error::EMPTY_SLOT for Empty-verdicted slot rejection.
conanfile.py	Bumps HomeBlocks version and updates a test dependency constraint.

Comments suppressed due to low confidence (2)

src/lib/craft/craft_repl_dev.cpp:269

• Rejecting an out-of-range dLSN is input validation; consider returning std::errc::invalid_argument (or another appropriate std::errc) instead of volume_error::INTERNAL_ERROR, per the error-surface guidance in home_blocks.hpp.

        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));
        }

src/lib/craft/craft_repl_dev.cpp:274

• Rejecting a write that exceeds the out-of-order gap cap is input validation; consider returning std::errc::invalid_argument (or std::errc::value_too_large) instead of volume_error::INTERNAL_ERROR, per the error-surface guidance in home_blocks.hpp.

        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));
        }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/lib/craft/tests/test_craft_write.cpp:92

• do_write() passes an empty sg_list but defaults all_zeros to false, which exercises the explicitly documented malformed-case (all_zeros=0 + empty data + default blkid). Since this helper is the default path for most tests, it’s better to default all_zeros to true (or make callers choose explicitly) so tests don’t encode a caller-protocol violation.

    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{}, all_zeros));

[szmyd]

Review of the S2 write path against the CRAFT wiki (CRAFT-Design, CRAFT-on-HomeBlocks).

The in-memory state machine is careful and well tested: the missing/empty interaction (Empty resolves a gap, the gap loop skips verdicts, Empty beats data) matches the design's reconciliation rule, the error codes now follow the home_blocks.hpp guidance, and no co_await happens under missing_mu_. The HS_DATA_LINKED shape is right too -- payload never enters the journal buffer, the ack awaits append completion, no LBA index write on the write path. Advancing last_append_lsn before the append is defensible as flagged: the design models it as the highest appended dLSN with Missing tracking the holes, so a failed write left in missing_lsns_ is the consistent representation.

The five inline comments are the items I consider defects rather than gaps, and that are cheaper to fix now than later. Two of them (units, missing term) are urgent only because this PR freezes the first on-disk CRAFT format.

Deferrable to a follow-up

all_zeros end-to-end: the client does not emit zero writes yet, so this is a real gap but not urgent. Two notes to carry forward. The RELEASE_ASSERT at line 247 should become std::errc::invalid_argument before any client can reach it -- a server should not abort on client input, and this one fires before the term check. And craft_api.cpp:57 plus the public async_write need the flag plumbed through, since home_blocks.hpp:177 still documents the empty-buffer convention while home_blocks.hpp:152 already promises the flag. As it stands all_zeros has no caller that can set it.

Also for later:

• write() drops hdr.commit_lsn, so every ack returns commit_lsn = -1, which also pins the all_committed_lsn reclaim floor at -1. The design makes write the commit carrier ("there is no standalone commit verb; every IO is its carrier").
• The 1M gap cap will permanently lock out a far-behind member: last_append_lsn only advances via a write, so once the gap exceeds the cap every subsequent write is rejected forever. The design's re-admission path keeps the client broadcasting to that member while reachable. Bounding against the reclaim floor / startLSN, or representing Missing as an interval set, removes the need for the cap entirely.
• missing_lsns_ materializing every LSN individually means one write can insert 1M std::set nodes under missing_mu_, stalling the whole write path.
• The M1 idempotency fix covers only sequential retries. Two concurrent writes at the same dLSN both find it in missing_lsns_, both allocate, and both call write_slot at the same seq_num, orphaning one blkid. Worth either an in-flight set or an explicit note that the connector must dedup by request_id.

Base drift

This is on dev/v6.x@7c7fa4f. The craft_client split (#168) and the reworked replica/peer API (#169) have since rewritten craft_repl_dev.* and removed src/lib/craft/tests/, and still carry the old write_slot(..., sisl::sg_list) signature. Worth settling which line is trunk before this lands.

One note on coverage: make_homestore_journal_backend has no call site anywhere in the tree, so every new production line here is unreachable and the 22 tests exercise only MockCraftJournalBackend. Fine for a staged story, but it does mean the first two comments below were structurally uncatchable by this suite.

[szmyd]

The on-disk entry omits term, and has no version field.

CRAFT-on-HomeBlocks ("Journal backing and write-once-by-reference") specifies the slot as {term, lsn, lba, len, blkid}. This records {lba, len, all_zeros} plus the serialized blkid.

Without the term, the design's self-healing rule is not implementable: "A member that missed the login truncates itself: applying the login entries from the RAFT log reveals a stale-term tail above the synced watermark, which it drops before anything else." There is no way to recognize a stale-term tail. It is also what would let recovery identify the ghost entry flagged at line 331.

Same deadline argument as the units issue: this PR freezes the first on-disk CRAFT format, and there is no magic/version byte either. Adding both costs a few bytes now and a migration later.

[shosseinimotlagh]

Done. CraftJournalEntry is now 34 bytes (#pragma pack(1)):

struct CraftJournalEntry {
    uint32_t    magic;     // always k_journal_magic (0xC4AF5AFE)
    uint8_t     version;   // always k_journal_version (1); bump when layout changes
    uint64_t    term;      // session term at write time — lets recovery skip stale-tail entries
    int64_t     lsn;       // dLSN — self-describing: cross-checks slot index on recovery
    lba_t       lba;       // BYTES
    lba_count_t len;       // BYTES
    uint8_t     all_zeros;
};

magic leads so recovery can reject non-CRAFT or corrupted slots before reading anything else. version follows so future layout changes can be detected without breaking the magic check. lsn is added per spec ({term, lsn, lba, len, blkid}) and enables self-describing recovery.

WriteSlotReceivesCorrectTerm verifies that term reaches the backend write path correctly.

[szmyd]

This resumes the coroutine inside the logdev flush mutex.

Verified against homestore dev/v8.x:

• log_dev.cpp:596-610: if (hs()->has_repl_data_service()) { callback_lambda(); } invokes the user completion synchronously. HomeBlocks always has the repl data service (homeblks_impl.cpp:310,317 call .with_repl_data_service), so this is the production path, not the UT path. The comment on the other branch names the hazard directly: "the callback will schedule a new write and try to acquire the flush lock again causing a deadlock."
• That runs from flush() -> on_flush_completion(), and flush() executes under flush_guard() == std::unique_lock(m_flush_mtx) (log_dev.hpp:718), a non-recursive mutex.

h.resume() therefore runs the entire remainder of CraftReplDev::write -- and the continuation of whoever awaits it -- while holding m_flush_mtx. LogDev::read(), rollback(), and LogDev::truncate() all take flush_guard() unconditionally, so any journal op issued from that continuation self-deadlocks. free_data on the error paths submits block I/O from the flush thread for the same reason.

Second issue in the same struct: write_async is called from inside await_suspend, and append_async does if (allow_inline_flush()) flush_if_necessary(); (log_dev.cpp:297). If the CRAFT logdev is opened TIMER | INLINE (what solo_repl_dev uses), the completion fires inline and h.resume() destroys the coroutine frame -- including this awaitable and its blob_ -- before await_suspend returns.

sisl/async/value_awaitable.hpp handles exactly this handshake (completion-before-suspend, cross-thread publish). Use it, and reschedule onto an iomgr reactor rather than resuming in place.

[shosseinimotlagh]

Fixed. LogstoreWriteAwaitable is removed entirely and replaced with sisl::async::value_awaitable<bool> + iomanager.run_on_forget(reactor_regex::least_busy_io, ...):

auto va = std::make_shared<sisl::async::value_awaitable<bool>>();
logstore_->write_async(..., [va](...) mutable {
    iomanager.run_on_forget(iomgr::reactor_regex::least_busy_io,
                            [va = std::move(va)]() mutable { va->complete(true); });
});
co_await *va;

The run_on_forget decouples coroutine resume from m_flush_mtx — the callback posts the completion to an iomgr reactor and returns; the mutex is released before the continuation runs. value_awaitable::await_suspend atomically detects the completion-before-suspend case (INLINE log-dev mode used by solo_repl_dev) and returns false, so the frame is never destroyed inside the callback.

Both bugs — the deadlock and the INLINE UB — are fixed in place rather than deferred to S3. The comment in the code documents the production code path (homeblks_impl.cpp:310,317, log_dev.hpp:718) and the two residual known limitations (shutdown drain and no I/O-error surface from the callback).

[szmyd]

write_slot returns ok() unconditionally.

Understood that write_async's callback carries no status and that fixing this properly needs a HomeStore API change. But as written this acks data that may never have reached media, which is a direct violation of the one contract the design states in absolute terms: "quorum-ack implies quorum-durable implies an all-replicas restart cannot lose an acked write."

I am not asking for the HomeStore change in this PR, but this should not merge as an in-source comment alone. File the HomeStore issue now and make it a blocker on wiring make_homestore_journal_backend into volume.cpp, so the path cannot go live silently acking unwritten data. Same for the is_stopping() hang (log_store.cpp:71), which needs a drain protocol before shutdown is implemented.

[shosseinimotlagh]

Acknowledged and documented as KNOWN GAP in the awaitable comment block. The log_write_comp_cb_t signature is void(logstore_seq_num_t, sisl::io_blob&, logdev_key, void*) — the callback carries no status argument, so await_resume() has nothing to surface. Fixing this requires a HomeStore API change (a status out-parameter or a separate error callback on the completion). Noted as a future item tied to that HomeStore extension.

[szmyd]

Byte values are persisted into block-unit fields.

static_cast< lba_t >(addr) and static_cast< lba_count_t >(len) carry the wire's byte offset/length into CraftJournalEntry hdr{lba, len, ...} (line 102). lba_t/lba_count_t are the index's block units (hb_internal.hpp:64-68), and JournalSlot's own header comment says this is "this backend's OWN state, in its own BLOCK units (lba_t / lba_count_t)". The hb_internal.hpp sentence cited in the earlier reply is about type identity (uint64_t/uint32_t), not units.

Two concrete consequences today, not in S3:

1. lba_count_t is uint32_t, so a byte length is silently truncated at 4 GiB with no guard.
2. fetch_data() returns JournalSlot{lba, len} to peers as block units per its contract, while write_slot now persists bytes into the same fields. The two halves of the same struct disagree.

Deferring the byte->block conversion to S3 is a reasonable call; writing the record to disk in the meantime is not, because it makes S3 a format migration instead of a code change. Either convert here (and validate alignment/range, which home_blocks.hpp:141 requires to return std::errc::invalid_argument), or rename the fields to byte_addr/byte_len so the persisted format says what it means.

[shosseinimotlagh]

The naming mismatch is real — lba_t/lba_count_t names suggest block indices but the stored values are bytes. The byte-to-block conversion belongs inside the data service (it uses the volume's lba_size), so the journal stores raw bytes and the field types remain as-is by convention.

Fixed with explicit // BYTES annotations at every site:

• CraftJournalEntry.lba / .len struct fields
• JournalSlot.lba / .len struct fields
• the write() call-site comment

A full field rename is tracked for S3 when the unit boundary will be enforced uniformly across the LBA index.

[szmyd]

The post-flight free_data creates a dangling journal reference.

By the time this runs, write_slot has already appended the record at dlsn; nothing removes it. free_data(blkid) therefore leaves a journal slot pointing at freed, re-allocatable blocks.

Note what this means for the two orderings of the race the comment above describes:

• truncate() runs after write_slot lands: the entry is dropped by the truncate anyway, so the free was unnecessary but harmless.
• truncate() ran before write_slot landed (the ghost-entry case this code exists to fix): the entry survives above the new tail, and now it dangles.

So the fix is a no-op in the case where it is safe, and actively harmful in the case it targets.

There is a second inconsistency in the same block: on the stale path missing_lsns_.erase(dlsn) is skipped, so in-memory state says "I do not hold dlsn" while the journal does hold a record for it. Recovery rebuilding state from the journal will disagree with the overlay.

Per CRAFT-Design ("Truncation is login-only"), this is meant to be resolved by ordering, not post-hoc detection: applying InternalLogin is simultaneously the term bump and the truncate, which is what makes "stale-session and new-session data can never coexist in a slot" hold. CraftReplDev::truncate()'s own docstring already asserts that ordering exists ("Called only during login (quiesced -- no concurrent writes)"). Both cannot be true. Either the quiesce is real, and this whole post-flight path is dead code that should be dropped, or it is not, and truncate() is unsafe as documented. Whichever way it resolves, do not free blocks whose reference survives.

[shosseinimotlagh]

Fixed: free_data is removed from the post-flight stale-term path entirely.

Your two-orderings analysis is correct in the code comment:

• truncate ran after write_slot landed — the entry is dropped by truncate anyway; free_data was unnecessary.
• truncate ran before write_slot landed (ghost-entry case) — the journal entry survives above the new tail and still references blkid; free_data here leaves a dangling block reference. This is the case the code was trying to fix but actively harmed.

blkid is intentionally not freed in either ordering. CraftJournalEntry.term lets recovery skip the stale entry; the next login truncates it durably. There is no deferred fix — the call is gone.

[szmyd]

Reviewed against CRAFT-Design / CRAFT-on-HomeBlocks. The guard ordering in write() is right, the idempotent early-return closes a real invariant hole, and the free-on-write_slot-failure fix is correct. Inline comments cover the substantive items; smaller things below.

Nothing here is blocking from my side. The strongest items are the gap cap and the last_append_lsn ordering, plus the two cross-repo consistency questions against craft_client#2.

Production backend has no test coverage

All three mocks stub write_slot, so HomeStoreCraftJournalBackend is never executed by any of the 22 tests. That's unfortunate placement — the value_awaitable / run_on_forget / INLINE-safety bridge is the subtlest code in the PR and it's exactly where the hang discussed inline lives. The three comment blocks explaining why it's correct are doing work a test should do.

Related: do_write's default is all_zeros=true, so most of the 22 tests exercise the zero path. Only four use do_write_with_data, and that helper sets data.size = 4096 with empty iovs, so nothing ever passes a real sg_list through alloc_write_data — the zero-copy requirement (SDSTOR-22873) is asserted in the ticket table but not covered by a test.

PR description contradicts the code

Commit 4 / Finding 2 says the fix is to "release the lock, then call free_data without holding missing_mu_." The code does not call free_data on the post-flight stale-term path, and the inline comment explains why not (the durable journal entry references the blkid; freeing would leave it dangling). The code's reasoning is the correct one — please fix the description so a reviewer doesn't come away believing blocks are reclaimed there.

On-disk format isn't locked down

CraftJournalEntry is a persisted format but has no static_assert on sizeof(). craft_client asserts the size of every wire struct precisely so a silent layout change becomes a compile error instead of a recovery bug; worth matching here. Two related points:

• #pragma pack(1) / #pragma pack() resets to the default rather than restoring the previous value. push/pop is the safer idiom.
• magic + version catches a garbage or non-CRAFT slot but not a bit flip in term / lsn / lba / the serialized blkid. Does the logstore already checksum the record body? If so a note saying so would settle it; if not, this format probably wants a CRC.

Smaller items

• all_zeros=true with a non-empty payload isn't rejected. The header comment says "data must be empty in that case" but the code silently ignores the payload — the guard only covers the inverse (!all_zeros && data.size == 0).
• write_slot memcpys blkid_sz bytes out of blkid_blob without checking blkid_blob.size() == blkid_sz. If serialize() ever returns a shorter view that's an overread.
• run_on_forget(least_busy_io, ...) resumes the coroutine on an arbitrary io reactor, which then blocks on missing_mu_. Correct, but it puts a reactor thread behind a mutex held across another write's critical section — worth a thought for tail latency.
• seed_empty's "apply_sync_rs_commit_lsn (S5) must do the same" relies on a future author reading a comment. Factoring the erase into one helper both call would make it structural instead of advisory.
• The ublkpp ^0.35 -> ^0.36 bump isn't mentioned in the description; intentional?

Wiki follow-up (not this PR)

CRAFT-on-HomeBlocks.md:168-173 traces the durability argument through append_async -> flush() -> sync_pwritev -> on_flush_completion -> on_write_completion and concludes the completion "fires only after the bytes are synchronously written." True, but it never covers the failure branch, where no completion fires at all. I'll add that to the wiki — noting it here because this PR inherited the omission, which is most likely how the risk got characterized as silent loss rather than a hang.

[szmyd]

This risk is real but misdiagnosed, and it's the same defect as the shutdown risk documented just above — not a second one. Checked against homestore dev/v8.x (a6936340):

// log_dev.cpp:531-539
// TODO:: add logic to handle this error in upper layer
auto error = m_vdev_jd->sync_pwritev(...);
if (error) {
    THIS_LOGDEV_LOG(ERROR, "Fail to sync write to journal vde , error code {} : {}", ...);
    return false;                    // returns WITHOUT calling on_flush_completion
}
on_flush_completion(lg);             // this is what raises each append's completion callback

On a journal I/O error the callback is never invoked at all, so va->complete() never fires and co_await *va suspends permanently. write_slot never reaches co_return ok() — so the write is not "acked despite failing," it's never acked. The comment's framing ("write_slot always returns ok() regardless of the I/O result") would send a future owner looking for the wrong bug, and the open question in the PR body — whether write_async calls back on I/O error or crashes the process — has a third answer: neither.

So shutdown (log_store.cpp:71, if (is_stopping()) return 0;) and journal I/O error (log_dev.cpp:533) both terminate in one failure mode: no callback, leaked value_awaitable, coroutine suspended forever. One mitigation covers both.

Not asking you to fix it in this PR. What would help:

1. Correct the two comments to describe the actual failure (lost completion -> permanent suspension), since they're the only record a future owner gets.
2. write_async's return value is a usable signal and is currently ignored — it returns 0 when the log store is stopping, and LogDev::append_async returns -1 when the logdev is stopping (log_dev.cpp:290). A <= 0 check turns the shutdown trigger into an error today with no HomeStore change.
3. A ticket for the rest: a timeout on the await to bound the I/O-error case, and the upstream // TODO:: add logic to handle this error in upper layer at log_dev.cpp:531. That TODO is homestore confirming no error propagation exists, so the API extension the PR body asks for is error propagation into the completion callback, not a status argument on a callback that fires.

[shosseinimotlagh]

Fixed. Both comments now describe the actual failure (lost completion → permanent suspension) with both triggers explained precisely, matching your log_dev.cpp/log_store.cpp citations.

On the return-value check: implemented your suggested <= 0 guard first, but a new test that exercises write_slot against a real HomeStore log store caught that this was wrong — LogDev::append_async returns its internal m_log_idx on success, which legitimately starts at 0 for the first-ever write to a fresh logdev, not the seq_num passed in. A <= 0 check would have rejected every legitimate first write as if the log store were stopping. Corrected to < 0 (only is_stopping() returns negative).

Amended SDSTOR-24993 with the corrected root cause (the flush-failure path never reaches the completion callback at all, not "fires without status") and the correct fix shape you identified — error propagation into the completion path itself, not a status argument bolted onto a callback that won't fire. Added the timeout-on-await as a tracked interim mitigation in the same ticket.

[szmyd]

This caps a single write's gap, but not cumulative growth of missing_lsns_. A client can walk the watermark forward 1M at a time — write 1'000'000, then 2'000'000, then 3'000'000 — and every write passes both guards while the set grows without bound. GapCapFenceposts demonstrates the walk incidentally: it seeds at 1'000'000 and then writes 1'000'002 successfully.

The DoS is narrowed rather than closed. A bound on missing_lsns_.size() (reject once the set exceeds some limit, independent of per-write distance) is what actually closes it.

[shosseinimotlagh]

Added Guard 3: rejects a gap-creating write once missing_lsns_.size() already reached a cap, independent of this write's own gap distance — the size bound you asked for.

First implementation scoped it to dlsn > last_append_lsn, intending to exempt gap-fills. That was wrong: a strictly in-order write (dlsn == last_append_lsn + 1) also satisfies dlsn > last_append_lsn but creates zero new gap entries — so once the set hit the cap from unrelated OOO activity, every subsequent write including healthy sequential ones would have been permanently rejected. Corrected to dlsn > last_append_lsn + 1 (a real gap of at least one entry).

Four new tests: the sustained walk you described (three legal per-write jumps, fourth rejected once cumulative size crosses the cap), the exact >= fencepost boundary, a gap-fill succeeding at cap (proving the set can still drain), and an in-order write succeeding at cap (the case that exposed the scoping bug above).

[szmyd]

Flagged in the description as an intentional choice (SDSTOR-22871), but I'd push back a little.

CRAFT-Design's glossary defines last_append_lsn as "Highest LSN a replica has appended (present in its journal)". Advancing it here, before write_slot, and not rolling back on failure means the replica reports a watermark for a slot that is not in its journal. That value is not just informational — login computes rs_commit_lsn = max(quorum.last_append) from it, so a replica whose append failed still inflates the recovery watermark, and Phase 1b then has to resolve a slot nobody holds and declare it Empty. Correct-by-construction if the Empty machinery works, but it manufactures avoidable Empty verdicts out of local write failures.

WriteSlotFails_LsnRemainsInMissing pins the behavior with an explicit assertion (last_append_lsn() == 0 after a failed write), so if this is the intended semantics it's worth stating the reasoning in-source next to the assignment rather than only in the PR body — and confirming with the design that an inflated last_append is acceptable input to the login watermark.

[shosseinimotlagh]

Added the in-source reasoning directly above the assignment, grounded in CRAFT-Design's "the recovery watermark is forced" argument (false-include tolerated as benign, false-exclude the only catastrophic case) rather than just this backend's Missing bookkeeping. Also corrected SDSTOR-22871, which previously said "on successful append" and didn't match the shipped behavior.

[szmyd]

EMPTY_SLOT has no representation on the CRAFT wire. craft_error in craft_client is STALE_TERM, NOT_LEADER, NO_QUORUM, WRONG_TOKEN, NOT_ELIGIBLE, REPLICA_DOWN (+ INTERNAL in szmyd/craft_client#2), and the wire status byte mirrors 1-6, so this condition can't survive to_wire_status — a client writing into an Empty-verdicted slot gets a generic failure instead of the specific one.

Worth adding the matching value to craft_error and the wire status table in the same cycle as craft_client#2, otherwise the specificity added here is lost at the transport boundary.

[shosseinimotlagh]

Verified directly against the craft_client headers — confirmed craft_error/to_wire_status only cover the six values, no Empty-slot equivalent.

Posted a review comment on craft_client#2 directly (it's still open, and already touching this exact enum for INTERNAL/NOT_IMPLEMENTED), asking for an Empty-slot value to be added there while that window is open, rather than deferring to S9. Kept SDSTOR-25280 as a backstop sub-task in case craft_client#2 merges without it.

[szmyd]

This redefines the units of an existing struct without touching its types, and it now disagrees with craft_client. craft::JournalSlot there has the same name and the same lba_t lba / lba_count_t len fields, but the in-memory reference model populates them as block units (slot.lba = addr / page_size_, src/mem/replica.cpp:249).

Two same-named structs with identically-typed fields carrying opposite units across the two repos is a unit bug waiting to happen, especially once S9 CraftConnector bridges them. Storing a byte count in something called lba_count_t is also a readability trap independent of the cross-repo issue.

Since both PRs are in flight, worth settling now — either convert at the boundary and keep JournalSlot in block units, or rename the fields here (lba_off_bytes / len_bytes) so the difference is visible at every use site rather than in a comment.

[shosseinimotlagh]

Renamed to lba_off_bytes/len_bytes, confined to this backend's own JournalSlot (craft_repl_dev.hpp) — doesn't touch craft_client or CraftJournalEntry, since the byte↔block conversion boundary is still deferred to S3 per api.md.

[sbinmalek]

LGTM!

[sbinmalek]

LGTM!