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 6.25000% with 15 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	6.66%	13 Missing and 1 partial ⚠️
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.62%           
===========================================
  Files               ?       18           
  Lines               ?     1033           
  Branches            ?      449           
===========================================
  Hits                ?      461           
  Misses              ?      281           
  Partials            ?      291           

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