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 into the RAFT
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.

Four rounds of adversarial review were run on the implementation before this
PR was raised. Eight bugs were found and fixed; three known risks are documented
in-source for future owners.

---

What changed

Commit 1 — S2 write path core (51ffc3a)

• CraftJournalBackend::write_slot signature changed: sisl::sg_list replaced
  by homestore::multi_blk_id blkid + bool all_zeros (HS_DATA_LINKED scheme —
  the payload never enters the journal buffer; only the block reference does).
• CraftReplDev::write wired: pre-flight term check, Empty-verdict rejection
  (EMPTY_SLOT), pre-insert into missing_lsns_, gap fill loop, journal
  append via write_slot, post-success state update.
• volume_error::EMPTY_SLOT added.
• Tests: AllZerosWrite, WriteSlotFails_LsnRemainsInMissing,
  EmptySlotRejectsWrite.

Commit 2 — round-1 hardening (8eb1066)

Three correctness bugs found by adversarial review:

1. Data race on state_.term — read outside missing_mu_ is UB once
   apply_internal_login (S3) lands. Fixed: term check moved inside the lock.
2. Negative dlsn bypass — dlsn < 0 skipped the pre-insert condition and
   called write_slot without an entry in missing_lsns_. Fixed: reject
   dlsn < 0 before acquiring any lock.
3. DoS gap allocation — a write with a large dlsn caused O(gap) synchronous
   insertions into missing_lsns_ under the mutex. Fixed: cap out-of-order gap
   at k_max_ooo_gap = 1 000 000.

Additional fix: seed_empty (and future apply_sync_rs_commit_lsn) must erase
verdicted LSNs from missing_lsns_; an Empty-verdicted LSN left in the missing
set would permanently stall commit advancement.

Tests: EmptyVerdictClearsMissingEntry, NegativeDlsnRejected,
ExcessiveGapRejected, OutOfOrderWritesLargerGap.

Commit 3 — round-2 hardening (6204d41)

Four more bugs found by a second adversarial review pass:

• 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. Fixed: Guard 1 rejects dlsn > INT64_MAX - k_max_ooo_gap before
  the subtraction.
• M1 — duplicate write bypasses pre-insert invariant — a retry of a
  previously-written dlsn evaluated (N > N) = false && !contains(N) = true,
  skipping pre-insert and calling write_slot without the dlsn in
  missing_lsns_. Fixed: return the current watermark snapshot idempotently
  when dlsn ≤ last_append_lsn and not missing.
• M2 — gap loop re-inserts Empty-verdicted LSNs — the fill loop inserted all
  gaps unconditionally, undoing Empty verdicts and stalling commit advancement.
  Fixed: skip LSNs in empty_lsns_ inside the loop body.
• M3 — ghost entry after login-truncate race — a write_slot that completes
  after a new login+truncate produced a journal entry that survived past the
  truncation point. Fixed: re-validate hdr.term in the second lock region;
  discard with STALE_TERM on mismatch.

Tests: DuplicateWriteIsIdempotent, GapLoopSkipsEmptyVerdicts,
GapCapFenceposts (including INT64_MAX Guard 1 case).

Commit 4 — block-leak fixes and risk documentation (e753bee)

Two block-leak bugs fixed, three known risks documented in-source.

Finding 1 — block leak when write_slot fails
After alloc_write_data succeeds, a subsequent write_slot failure silently
dropped the allocated blocks with no free path. Fixed: call
journal_->free_data(blkid) before propagating the error.

Finding 2 — block leak on post-flight stale-term discard
After write_slot succeeds, the code re-checks the term under missing_mu_.
If the term had changed it returned STALE_TERM — but with no free call, and
you cannot co_await while holding a std::lock_guard (UB / potential
deadlock). Fixed: introduce bool stale_post_flight, capture the check result
under the lock, release the lock, then call free_data without holding
missing_mu_.

Implementation: CraftJournalBackend::free_data() pure virtual added (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 hang: write_async skips its callback when HomeStore
  is stopping (log_store.cpp:71), leaving the LogstoreWriteAwaitable
  coroutine permanently suspended. This 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 the same callback
  for success and I/O failure with no status argument; write_slot always
  returns ok(). Fixing this needs a HomeStore API extension. A HomeStore expert
  should confirm whether write_async can call back on I/O error or always
  crashes the process — if the latter, this risk does not exist in practice.

---

Subtask coverage (SDSTOR-22732)

Ticket	Summary	Status
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 + gap loop
SDSTOR-22873	Zero-copy data path	Done — sg_list passed through uncopied
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 block alloc, journals metadata-only slot

SDSTOR-22870 flag — the ticket states "a crash between the data-service
write and journal append leaves only uncommitted blocks, which HomeStore recovery
reclaims automatically." If that 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. Please confirm with
added here are defensive but not strictly necessary for crash-safety. If it is
aspirational or incorrect, they are the only reclaim path. Please confirm with
the HomeStore team before closing this ticket.

SDSTOR-22871 flag — 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 correctly tracked. This is an intentional
design choice — please confirm it is acceptable or request a post-success update
with rollback on failure.

---

Design notes for future stories

• S3 (apply_internal_login) must update state_.term under missing_mu_
  to avoid a data race with write()'s pre-flight and post-flight term checks.
• SDSTOR-22874 (quorum-ack) is not implemented here. The current code ACKs
  from the local replica only; quorum counting is the client's responsibility in
  CRAFT and is a separate future story.

---

Test summary

All 6 test binaries pass (test_craft_write, test_craft_truncate,
test_craft_peer_exchange, and others). 22 unit tests cover the write path
including: in-order, out-of-order, large gaps, idempotent retry, term rejection,
all_zeros, Empty-verdict rejection and interaction with the gap loop, negative
dlsn, overflow guard, post-flight stale-term discard, and write_slot failure
with block-free verification.

[codecov-commenter]

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

Codecov Report

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

Files with missing lines	Patch %	Lines
src/lib/craft/craft_repl_dev.cpp	18.75%	11 Missing and 2 partials ⚠️
src/include/homeblks/home_blocks.hpp	0.00%	1 Missing ⚠️
❗ 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            ?   44.58%           
===========================================
  Files               ?       18           
  Lines               ?     1034           
  Branches            ?      450           
===========================================
  Hits                ?      461           
  Misses              ?      280           
  Partials            ?      293           

☔ 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.

[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.

[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.

[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.

[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.