S3: Commit + Read path: advance commit_lsn, serve reads, watchdog

[shosseinimotlagh]

Summary

Implements the S3 story: commit(), keep_alive(), and read() on CraftReplDev — the machinery that advances commit_lsn, applies journal entries to the index, and serves client reads from the journal-tail overlay and committed index without fetching from a peer.

Update: rebased onto the latest dev/v6.x (now includes S5's apply_sync_rs_commit_lsn/InternalLogin, PR #176) and addressed the full review pass from sbinmalek, Copilot, szmyd, and an independent review pass — see "Review fixes" below. All 13 addressable findings are fixed and build-verified; 3 findings are real but need more design discussion and are explicitly deferred to a follow-up PR (see "Deferred to follow-up PR").

What's implemented (across 11 commits, squashed)

• CraftJournalEntry blob format — per-LBA checksum array appended after the header; write() computes one crc16_t10dif per LBA at append time
• HomeStoreCraftJournalBackend::read_slot() — real log-store read: validate, parse header, deserialize blkid, extract csums
• VolumeIndexTable::delete_lba_range() — new method for all_zeros unmap on apply
• Journal-tail overlay — std::unordered_map<lba_t, OverlayEntry> covering (commit_lsn, last_append_lsn]; highest-dLSN-wins per LBA; populated on write(), rebuilt on restart, pruned on truncate() and commit
• commit(upto_lsn) — walks the journal in order, stalls at the first missing gap (never skips holes), applies each slot to the index (write_to_index for data, delete_lba_range for all_zeros), retires overlay entries only when their recorded LSN exactly matches the applied slot
• keep_alive() — term check → watchdog reset → commit(hdr.commit_lsn) → returns {commit_lsn, last_append_lsn}
• read() — term check → piggyback commit → snapshot watermarks → per-LBA resolution: overlay (horizon-clamped to read_lsn) wins over index; absent-from-both is a hole; read-time all-zero collapse; CRC verification; adjacent same-type extents merged
• rebuild_overlay() — walks (commit_lsn, last_append_lsn] on restart, skips missing/empty LSNs (not a stop condition), populates overlay with the same highest-dLSN-wins rule write() uses
• Watchdog timer — recurring iomgr timer; reset on every successful write()/keep_alive(); fires into append() when the session goes quiet; craft_watchdog_timeout_ms config key (default 30 s, 0 = disabled)
• craft_max_io_len_mb config key (default 128 MiB) — enforced in both write() and read() to bound unbounded-len attacks from malformed frames

Review fixes (this update)

All build-verified (100% tests passing, 13/13 suites, including S5's own CraftRaftEntriesTest).

#	Finding	Fix
1	szmyd (blocking): read()'s piggyback commit can advance commit_lsn past its own read_lsn before the snapshot is taken, serving a too-new value with no error	Added volume_error::HORIZON_STALE; read_impl() rejects read_lsn < commit_lsn_snapshot
2	Residual TOCTOU: the snapshot check above only guards the value taken before the unlocked index query; a concurrent commit_impl can still advance commit_lsn while that query runs	Re-snapshot and re-check after the index read
3	Copilot: crash-replay of an already-applied slot reports old_blkid == new_blkid, freeing a block the index still references	Skip the free when they match
4	Copilot: read_slot() never cross-checked the on-disk record's hdr.lsn against the seq_num actually requested	Reject on mismatch
5	sbinmalek/Copilot: the all-zero read-time collapse ran before the CRC check, so a bit-flip zeroing real data was reported as a hole instead of CRC_MISMATCH	CRC computed unconditionally before the collapse decision
6	Copilot: read_slot() derived nlbas from hdr.len without validating alignment/non-zero	Validated up front
7	Copilot: write()'s ack snapshotted state_ before the piggyback commit() call	Re-snapshot after
8	Independent review: hdr.all_committed_lsn (client-controlled) had no validation	Malformed negative values (not the -1 sentinel) are ignored; TODO(S8) documents that the unbounded-positive-poisoning vector needs S8's real reclaim implementation to close, since no valid per-call bound exists without breaking the legitimate lagging-replica catch-up signal
9	sbinmalek/Copilot: dest iovec capacity in the read path only checked for emptiness, not size	Validated iov_len >= nlbas * lba_size_
10	Copilot: write() allowed a multi-iovec sg_list with dead trailing iovecs	Rejected as hygiene
11	szmyd (unfiled): a single contiguous read run could exceed blk_count_t's max (65535 LBAs)	Capped the run-extension loop; larger runs split into multiple read_data batches, invisible to the caller
12	szmyd (unfiled): commit_impl's is_empty branch skipped overlay retirement entirely, including when this replica already journaled real data for an LSN the cluster verdicts Empty ("Empty beats data")	Best-effort read_slot() to discover the LBA range and retire it, even when skipping the index apply
13	Independent review (S4 scope, bundled here): truncate() never freed the data blocks referenced by rolled-back journal entries, leaking them on every login that drops a stale tail	Reads each dropped entry's blkid before rollback, frees it after

Also: two review asks that were acked in-thread but never actually implemented, now done — explicit is_hole(nlbas, false) initialization (style), and a shortened // TODO: comment on the logout/watchdog interaction (was a large explanatory paragraph).

Declined: k_journal_version stays at 1 (no legacy on-disk records exist in practice; not worth the churn this round).

Deferred to follow-up PR

Three findings are real, confirmed, and distinct from everything fixed above, but need a design decision rather than a quick diff:

1. Overlay single-version limitation (Copilot) — the overlay keeps only the highest dLSN per LBA, so a read whose horizon lands between two uncommitted writes to the same LBA can get a hole instead of the correct older version. Needs either a multi-version overlay or a journal fallback.
2. read_impl/commit_impl atomicity gap (sbinmalek, via an independent review pass) — read_impl's index-read and overlay-lookup, and commit_impl's index-write and overlay-retire, are each two separate non-atomic steps. A read straddling a commit can land on data older than what's already fully committed, with no error. Distinct from item 1 above (fixed) — that one is about seeing data too new; this is seeing data too old.
3. Asymmetric failure handling for a corrupt journal record (sbinmalek, via an independent review pass) — a corrupt record hit at restart permanently faults the partition (recovery_faulted_); the same fault hit live just logs and retries forever with no latch and no observability.

szmyd's review explicitly frames (1) and (2) as the two things he'd have raised himself if they weren't already on the thread. A design writeup for both has been shared with the team; will land together in the next PR once we settle on an approach. (3) needs a scope decision (in-scope for S3, or a tracked follow-up ticket) before a fix is written.

Unit test coverage

All tests in test_craft_commit.cpp (light, no HomeStore) and test_craft_commit_hs.cpp (real HomeStore) pass. Coverage maps to AC requirements:

AC item	Test(s)
commit-then-read	InOrderApply + WriteCommitReadRoundTrip (hs)
overlay read of an appended entry above a hole	ThreeWayMixedExtentRead
in-order apply after hole fills (same state as no-gap run)	InOrderApplyAfterHoleFillsMatchesNoGapRun
overlay rebuild on restart	RestartRecoveryGatesClientIoUntilOverlayRebuilt, RebuildOverlaySkipsNlbasZeroSlot
watchdog fires on session silence, suppressed by write/keep_alive	test_craft_watchdog.cpp
zero write reads as hole	AllZerosOverlayIsHole, AllZerosUnmapReclaimsRealBlock (hs)
data write of all-zero bytes collapses at read time, not write time	DataWriteOfAllZeroBytesCollapsesAtReadTimeNotWriteTime
C1 bounds guard	CommitCsumShortArrayAborts
C2 nlbas==0 skip	RebuildOverlaySkipsNlbasZeroSlot
R1-1 all_committed_lsn capture in write/read	WriteCapturesAllCommittedLsn, ReadCapturesAllCommittedLsn
C5 empty iovs rejected	ReadDestEmptyIovsRejected
max I/O length enforced in write and read	ReadMaxIoLenEnforced, MaxIoLenEnforced (write)
horizon clamp: overlay entry above read_lsn never served	HorizonClampServesIndexNotOverlayAboveReadLsn
overlay wins over index for same LBA	OverlayWinsOverCommittedIndexForSameLba
CRC mismatch detected at read time	CrcMismatchFails
truncate prunes overlay above rollback point	TruncateRemovesOverlayEntriesAboveLsn
concurrent commit serialised by commit_running_	ConcurrentCommitIsNoOp, ConcurrentKeepAliveSerializesCommitAgainstRealIndex (hs)
horizon-stale read rejected (item 1)	ReadRejectsStaleHorizon, ReadRejectsHorizonAdvancedDuringIndexQuery (item 2)
crash-replay block leak (item 3)	CrashReplayOfAppliedSlotDoesNotFreeLiveBlock
header lsn mismatch rejected (item 4)	ReadSlotLsnMismatchFails
CRC-before-collapse (item 5)	AllZeroCorruptionFailsCrcInsteadOfCollapsing
hdr.len validation (item 6)	ReadSlotMisalignedLenFails
write() post-commit snapshot (item 7)	WriteAckReflectsPostCommitSnapshot
malformed all_committed_lsn ignored (item 8)	KeepAliveIgnoresMalformedNegativeAllCommittedLsn
dest iovec capacity (item 9)	ReadDestUndersizedIovLenRejected
multi-iovec write rejected (item 10)	MultiIovecRejectedEvenWhenFirstIovAloneSuffices
large contiguous run split at blk_count_t limit (item 11)	LargeContiguousRunSplitsAcrossBlkCountTLimit
Empty-verdict overlay retirement (item 12)	EmptyVerdictRetiresOverlayEvenWhenThisReplicaHadTheData
truncate() frees dropped entries' blocks (item 13)	TruncateFreesBlocksForDroppedDataEntries, TruncateDoesNotFreeEntriesAtOrBelowLsn, TruncateSkipsFreeForAllZerosEntries, TruncateSkipsFreeForGapsAboveLsn

Not in scope (future stories)

• Overlay single-version limitation, read/commit atomicity, and asymmetric corrupt-record handling — see "Deferred to follow-up PR" above
• Journal reclaim using all_committed_lsn (captured here; reclaim action itself is S8)
• CraftPartitionState superblock recovery (overlay rebuild walk is empty until state recovery is wired)

[Copilot]

🟡 Changes recommended

The new read/write paths assume single-iovec sg_lists but don’t fully validate/enforce buffer shape/capacity, which can lead to checksum mismatches and potential out-of-bounds writes on the wire-reachable read path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Implements the CRAFT S3 “commit + read path” for CraftReplDev, including advancing commit_lsn by applying journal entries to the index, serving reads from an overlay+index merge (with CRC validation), and adding a client-liveness watchdog plus configuration knobs to bound request sizes.

Changes:

• Add commit/apply machinery (including VolumeIndexTable::delete_lba_range) and overlay rebuild on restart, with read-path overlay/index resolution and CRC verification.
• Introduce watchdog timer behavior and new settings (craft_watchdog_timeout_ms, craft_max_io_len_mb) with corresponding test coverage.
• Expand/adjust CRAFT API and test build wiring to support the new behaviors (including new unit/integration tests).

File summaries

File	Description
src/lib/volume/index_fixed_table.hpp	Adds delete_lba_range to support unmap/all-zero apply via range remove and blkid reclamation.
src/lib/home_blks_config.fbs	Adds watchdog timeout and max I/O length configuration keys.
src/lib/craft/tests/test_craft_write.cpp	Updates write tests for new checksum behavior, argument changes, and max-IO enforcement.
src/lib/craft/tests/test_craft_watchdog.cpp	New watchdog timer test binary with minimal iomgr bring-up.
src/lib/craft/tests/test_craft_truncate.cpp	Updates mocks/constructors and settings initialization for new config usage.
src/lib/craft/tests/test_craft_peer_exchange.cpp	Updates mocks/constructors and settings initialization for new journal API shape.
src/lib/craft/tests/test_craft_journal_slot_wire.cpp	Adjusts wire-compat assertions now that JournalSlot is a superset internally.
src/lib/craft/tests/test_craft_homestore_backend.cpp	Extends HomeStore backend coverage (read_slot parsing/validation; updated backend ctor).
src/lib/craft/tests/test_craft_concurrency.cpp	New multi-threaded tests for internal locking, overlay/read behavior, and commit serialization.
src/lib/craft/tests/test_craft_commit.cpp	New comprehensive unit tests for commit/read/rebuild overlay behavior and error paths.
src/lib/craft/tests/test_craft_commit_hs.cpp	New heavy integration tests against a real VolumeIndexTable + real HomeStore data.
src/lib/craft/tests/CMakeLists.txt	Wires new test targets and ensures config bindump is linked into “light” tests.
src/lib/craft/craft_repl_dev.hpp	Extends interfaces/state (checksums, read_data, overlay, watchdog, commit/read seams).
src/lib/craft/craft_repl_dev.cpp	Implements on-disk slot format changes, commit/read/overlay logic, watchdog, and recovery gate.
src/lib/craft/craft_api.cpp	Updates async_write signature to match “empty data means unmap” semantics.
src/include/homeblks/home_blocks.hpp	Updates public API documentation/signature for CRAFT write/unmap semantics.
conanfile.py	Bumps package version to 6.0.7.

Review details

• Files reviewed: 17/17 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov-commenter]

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

Codecov Report

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

Files with missing lines	Patch %	Lines
src/lib/craft/craft_repl_dev.cpp	65.16%	1 Missing and 30 partials ⚠️
src/lib/volume/index_fixed_table.hpp	45.45%	1 Missing and 5 partials ⚠️
src/lib/craft/craft_repl_dev.hpp	69.23%	0 Missing and 4 partials ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##             dev/v6.x     #175   +/-   ##
===========================================
  Coverage            ?   48.84%           
===========================================
  Files               ?       19           
  Lines               ?     1208           
  Branches            ?      527           
===========================================
  Hits                ?      590           
  Misses              ?      267           
  Partials            ?      351           

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

🟡 Changes recommended

Horizon reads, checksum verification, buffer validation, recovery compatibility, and peer data fetching contain correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/lib/craft/craft_repl_dev.cpp:945

• Checking only that an iovec exists does not establish that the flat destination has len writable bytes. A malformed public call with dest.size < len, a short first iovec, or a null base reaches the memset/async_read below and writes past or through the supplied buffer. Validate the destination exactly as the write path validates its source before dereferencing it.

    if (dest.iovs.empty()) {
        LOGW("read rejected: dest sg_list has no iovecs (size={})", dest.size);
        co_return std::unexpected(make_error_condition(std::errc::invalid_argument));
    }
    auto* dest_buf = static_cast< uint8_t* >(dest.iovs[0].iov_base);

src/lib/craft/craft_repl_dev.cpp:240

• After deserialization there is no structural check that a data slot's blkid covers exactly hdr.len / lba_size_ blocks (or that a zero slot has an empty blkid). If it covers fewer blocks, commit_impl() builds fewer BlockInfo values but still calls write_to_index() for the full header range, which inserts default/invalid mappings for the missing LBAs and advances commit_lsn. Validate alignment and exact block coverage here before returning the slot.

        sisl::blob blkid_blob{buf.bytes() + blkid_off, buf.size() - blkid_off};
        slot.blkid.deserialize(blkid_blob, /* copy = */ true);

• Files reviewed: 17/17 changed files
• Comments generated: 8
• Review effort level: Balanced

[szmyd]

Blocking on one finding, inline at read()'s piggyback commit.

Most of what I would otherwise raise is already on this thread -- the overlay's single-version limitation and the read_impl/commit_impl interleaving in particular. What follows is a separate half of the first of those, and the thing worth saying about it is that the fix proposed there does not close it.

Two smaller items I looked at and did not file, so they are at least on the record: the blk_count_t narrowing in read_impl's contiguous-run merge (line 965 -- run_nlbas is bounded only by craft_max_io_len_mb / lba_size, which exceeds 65535 at a 512-byte page_size under the default cap), and commit_impl's is_empty branch skipping overlay retirement alongside the apply. Say the word if either is worth writing up properly.

[szmyd]

Let's go ahead and merge this; follow-up fixes or additions in new prs.

[szmyd]

approved.