craft: S5 RAFT entry apply — SyncRSCommitLSN + InternalLogin (SDSTOR-22886, SDSTOR-22887)

[sbinmalek]

Summary

Implements the apply side of both CRAFT S5 RAFT entries on top of the S5 infrastructure merged in #172:

• SyncRSCommitLSN (SDSTOR-22886): on_commit dispatches to apply_sync_rs_commit_lsn, which gates
  the whole apply on a client_token match, range-validates empty_slots against rs_commit_lsn,
  reconciles them into empty_lsns_/missing_lsns_, and catches up any local gap via
  CraftPeerFetcher::fetch_data. Catch-up is best-effort: a failed fetch, a malformed/unrequested-LSN
  response, a failed write_slot, or no fetcher wired at all just leaves the LSN in missing_lsns_ for a
  later attempt. commit_lsn/last_append_lsn always advance afterward, never regress.
• InternalLogin (SDSTOR-22887): apply_internal_login overwrites client_token unconditionally
  (opaque id, no ordering) and advances term with a max-guard against regression, enforcing
  single-writer exclusivity through the existing term-fence check in write().
• Bounds the peer catch-up fetch with a configurable deadline (peer_fetch_timeout_ms, default 5000ms,
  home_blks_config.fbs) so an unresponsive peer can't hang apply_sync_rs_commit_lsn forever.
• Fixes a use-after-free in the detached on_commit coroutine: CraftReplDev now derives from
  std::enable_shared_from_this, is constructed only via a new CraftReplDev::create() factory (private
  constructor), and apply_sync_rs_commit_lsn captures shared_from_this() so the object stays alive
  across every co_await even if the last external owner drops its shared_ptr mid-apply.
• Drops the redundant get_lsns() alias (get_rs_commit_lsn() already covered the identical snapshot)
  and switches empty_lsns_ to unordered_set (no ordering requirement).

Known gaps (tracked, not fixed here)

• on_commit still detaches the apply coroutine fire-and-forget, so a later-committed entry can dispatch
  before this one's effects are fully applied — breaks strict RAFT apply ordering across entries. Real fix
  needs a per-device serialized apply queue (flagged as a FIXME in on_commit).
• CraftPeerFetcher is still unwired to a real transport — production peer catch-up stays stubbed until
  CraftConnector (S9).
• The checkpoint trigger (SDSTOR-22888) is left for a follow-up.

Test plan

• test_craft_raft_entries.cpp (new): client_token gate, empty_slots range validation +
  reconciliation, watermark advance (incl. never-decrements), best-effort catch-up (success, fetch
  failure, configured timeout threading, malformed/duplicate peer response, write_slot failure, no
  fetcher wired), on_commit dispatch for both entry types incl. malformed-entry rejection,
  InternalLogin session replacement + term monotonicity + write() term-fencing end-to-end.
• test_craft_peer_exchange.cpp / test_craft_truncate.cpp / test_craft_write.cpp updated for the
  CraftReplDev::create() factory; otherwise unchanged.
• conan create . full build + ctest.

[codecov-commenter]

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

Codecov Report

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

Files with missing lines	Patch %	Lines
src/lib/craft/craft_repl_dev.cpp	71.42%	2 Missing and 8 partials ⚠️
src/lib/craft/craft_repl_dev.hpp	70.00%	0 Missing and 3 partials ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##             dev/v6.x     #176   +/-   ##
===========================================
  Coverage            ?   47.62%           
===========================================
  Files               ?       19           
  Lines               ?     1096           
  Branches            ?      473           
===========================================
  Hits                ?      522           
  Misses              ?      261           
  Partials            ?      313           

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

[shosseinimotlagh]

@raakella1 PTAL

[shosseinimotlagh]

Block leak: if alloc_write_data succeeded but write_slot fails here, blkid holds
allocated HomeStore data blocks that nobody will ever free. The slot stays in
missing_lsns_, so the next SyncRSCommitLSN will re-fetch it and allocate a new
blkid — but the original one is already gone from any tracking structure.

The write() path (S3) calls journal_->free_data(blkid) in the equivalent failure arm.
Same guard is needed here:

    if (!res) {
        LOGE(...);
        if (!slot.all_zeros)
            journal_->free_data(blkid);   // reclaim the blocks we just allocated
        continue;
    }

[shosseinimotlagh]

Step 1 — SyncRSCommitLSN arrives, replica is behind.

apply_sync_rs_commit_lsn(rs_commit_lsn=3, client_token=0, empty_slots=[])
→ missing_lsns_ = {1, 2, 3}
→ to_fetch = [1, 2, 3]

Step 2 — fetch_data returns slot 2 with real data.

fetcher_.response = { JournalSlot{.lsn=2, .lba_off_bytes=4096, .len_bytes=4096} }

Step 3 — catch-up loop processes slot 2.

// slot.all_zeros = false → allocate blocks
auto alloc_res = co_await journal_->alloc_write_data(slot.data, slot.len_bytes);
// alloc_res = OK, blkid = B1 ← real HomeStore blocks allocated here

blkid = B1;

auto res = co_await journal_->write_slot(2, term, 4096, 4096, B1, false);
// write_slot fails → res = error (disk I/O error, log full, whatever)

if (!res) {
LOGE("write_slot failed lsn=2 ...");
continue; // ← B1 is dropped here, never passed to free_data()
}

Result: B1 sits in HomeStore's block allocator permanently. lsn=2 stays in missing_lsns_. The next SyncRSCommitLSN triggers another catch-up, fetches slot 2 again, allocates B2 — but B1 is already gone from all tracking. Repeat enough times and the data service bleeds blocks.

---

The fix is one line in the failure arm:

if (!res) {
    LOGE("write_slot failed lsn={} ...", slot.lsn, ...);
    if (!slot.all_zeros)
        journal_->free_data(blkid);   // reclaim B1 before losing the reference
    continue;
}

[sbinmalek]

ACK

[sbinmalek]

We need to address the case when blkid is not set. How would write_slot handle that?

[shosseinimotlagh]

The AC (SDSTOR-22886) says: "mark every slot in empty_slots[] as Empty and discard any
local data held at those slots." The in-memory set updates here cover the tracking
side, but if an LSN in empty_slots was already successfully written on this replica
(journaled via write() or a prior catch-up), the blkid at that journal slot is not freed.

The protocol invariant is that the leader should never verdict Empty an LSN that a client
successfully committed through CRAFT, so in a correctly-operating cluster this case won't
arise. But the AC is explicit about it and it is worth either:

(a) reading the journal slot and calling journal_->free_data(blkid) if one exists, or
(b) adding a comment here acknowledging the gap and filing a follow-up JIRA so it is
not silently absent from the next reviewer's view.

[shosseinimotlagh]

read_slot is async so it can't run under missing_mu_. The fix is a two-phase
approach: collect LSNs that had live data while holding the lock, then free them after
releasing it. Sketch:

    std::vector< int64_t > to_free;
    {
        std::lock_guard lk{missing_mu_};
        for (int64_t lsn : empty_slots) {
            // Not missing and not already empty → was successfully written on this replica.
            // The leader has now verdicted it Empty; discard the local copy.
            if (!missing_lsns_.contains(lsn) && !empty_lsns_.contains(lsn))
                to_free.push_back(lsn);
            empty_lsns_.insert(lsn);
            missing_lsns_.erase(lsn);
        }
        // ... rest of existing lock body unchanged
    }

    // Free existing journal blocks for slots now verdicted Empty.
    // Runs outside the lock because read_slot co_awaits.
    for (int64_t lsn : to_free) {
        if (auto slot_r = co_await journal_->read_slot(lsn);
                slot_r && !slot_r->all_zeros) {
            co_await journal_->free_data(slot_r->blkid);
        }
    }

The protocol invariant is that the leader can only verdict Empty an LSN that no client
successfully committed, so to_free should be empty in correct operation. But the AC
requires handling it, and the block leak on the wrong-invariant path is real.

example:
Step 1 — Write arrives and is journaled successfully.

write(dlsn=1, token=100, term=1) succeeds
→ journal slot 1 written, blkid = B1 (real HomeStore blocks allocated)
→ missing_lsns_ = {} (1 was written, not missing)

Step 2 — Network partition. Replica is isolated briefly. New leader elected.

apply_internal_login(token=200, term=2)
→ state_.client_token = 200, state_.term = 2

Step 3 — SyncRSCommitLSN arrives from the new session.

The new leader says: "in session B's view, lsn=1 was never committed — it was from the old session and the quorum rejected it. lsn=1 is Empty."

apply_sync_rs_commit_lsn(rs_commit_lsn=5, client_token=200, empty_slots=[1])

What happens inside the reconciliation loop:

for (int64_t lsn : empty_slots) { // lsn = 1
empty_lsns_.insert(lsn); // 1 → empty, fine
missing_lsns_.erase(lsn); // no-op, 1 wasn't missing
}
// lsn=1 is not in missing_lsns_, not in empty_lsns_ before this → was_written = true
// but no read_slot(1) → no free_data(B1)

Result: B1 — the HomeStore data blocks allocated for the session-A write — are never freed. The journal slot 1 is now shadowed as Empty in empty_lsns_, but the underlying blocks sit in the allocator permanently leaked.

[sbinmalek]

ACK

[szmyd]

Two blocking issues on the apply path. Both are semantic rather than mechanical: each inverts a rule CRAFT-Design states explicitly, and in both cases craft_client's reference replica already implements it the other way, so there is something concrete to copy from rather than a judgment call to make.

The rest of what I found on this path -- the blocking read_sync that free_slot runs on the commit thread, the dangling blkid it leaves behind, the inverted to_free condition, and the uncapped gap loop -- I'll hold until these two settle, since resolving them changes the shape of that code anyway.

[szmyd]

This gate vetoes the login's own SyncRSCommitLSN on every replica.

Login applies SyncRSCommitLSN before InternalLogin -- Phase 2 then Phase 3 in CRAFT-Design's login sequence, and the same order in craft_client's reference transport (src/mem/cluster.cpp:352-361):

// Phase 2: SyncRSCommitLSN(rs) applied to all live members.
for (auto* r : live) r->cold_apply_sync(rs, client_token);
// Phase 3: InternalLogin(token, term+1) applied to all live members.
for (auto* r : live) r->cold_apply_login(client_token, nt);

The reference discards the parameter deliberately: void MemCraftReplica::cold_apply_sync(int64_t rs_commit_lsn, uint64_t /*client_token*/).

So at Phase 2 apply time state_.client_token is still the previous session's token, or 0 on a member that has never had one. The gate mismatches every time, and the entry it discards is the one carrying Phase 1b's Empty verdicts and driving every member to rs_commit_lsn. Per the read-eligibility deep-dive, "login can't finish until >= 1 member is filled to L" -- this makes that unreachable, on a fresh cluster included.

A second reachable case with the same root: state_ is in-memory only, so a restarted replica comes back with client_token == 0 and vetoes every subsequent watchdog SyncRSCommitLSN. It never catches up, and nothing says so louder than a LOGW.

"Session verification" in the glossary is not an equality fence against local state. InternalLogin is what establishes the session, so a fence requiring the session to already exist cannot be applied to the entry that precedes it. Either drop the check (the reference's choice), or accept the login-path entry explicitly and let the term fence do the exclusion work -- which is the argument apply_internal_login's own header comment already makes for itself.

[sbinmalek]

Dropped the check, matching craft_client's reference (MemCraftReplica::cold_apply_sync discards the client_token parameter outright) — same option you called out. Exclusivity now comes from RAFT's commit ordering (SyncRSCommitLSN always applies before the InternalLogin that would invalidate it) plus the term fence every other IO already checks, per apply_internal_login's own header comment.

Also found docs/craft/subtasks.md and docs/craft/rpcs.md both documented the removed behavior ("on apply: verify token") — that wording was itself wrong/stale relative to craft_client's reference and the canonical CRAFT-Design wiki, not just the HomeBlocks code. Corrected in 05b5efa.

Fixed in 12b68b9.

[szmyd]

commit_lsn is advanced past unresolved Missing slots.

The comment above justifies this as a set-wide watermark RAFT already agreed on. That is rs_commit_lsn. commit_lsn is a different quantity, and CRAFT-Design defines it as the local contiguous prefix in three separate places:

• Glossary: "commit_lsn (= Synced) -- The contiguous committed prefix on a replica (Empty slots skipped): everything <= it is present and applied. The fill watermark."
• Resync: "Commit advances past Empty slots but never past an unresolved Missing one."
• Read-eligibility deep-dive: "a member must be fully filled to L (Synced >= L) before it serves any read [...] Synced >= L is the same 'reach the watermark' gate as reconfig promotion (commit_lsn >= startLSN)."

craft_client's reference replica implements exactly that (src/mem/replica.cpp:358):

void MemCraftReplica::apply_up_to(int64_t target) {
    int64_t next = state_.commit_lsn + 1;
    while (next <= target) {
        auto it = journal_.find(next);
        if (it == journal_.end()) break; // Missing hole -> stall (best-effort)
        if (!it->second.is_empty) apply_slot(next, it->second);
        state_.commit_lsn = next; // Empty slots are skipped on apply but still advance the frontier
        ++next;
    }
}

Every catch-up failure path above is reachable today -- no peer_fetcher_ wired at all (the production case until S9), a fetch error, a rejected batch, a write_slot failure -- and each one leaves entries in missing_lsns_ below the watermark this line then advances past. Three consequences, none of them local to this function:

1. commit_lsn is the read-eligibility gate below L. The replica advertises "filled to L" while holding holes, so the client routes prior-session reads to it and gets zeros for writes that were acked.
2. all_committed_lsn is min(commit_lsn) over voting members, and it is the journal reclaim floor. One replica reporting a false commit_lsn lets the whole set reclaim below it, deleting the last copies of the slots that replica still needs to fetch.
3. get_rs_commit_lsn() (line 233) hands commit_lsn to the next login as a contiguity certificate -- per the Login section it "bounds the leader's fetch-or-verdict work to (min commit, rs_commit]". Falsely advanced, the leader never resolves those slots at all. That is the false-exclude direction the recovery-watermark deep-dive exists to rule out, and it is the one the design calls catastrophic.

Advance to min(rs_commit_lsn, first unresolved missing LSN - 1) with Empty skipped, and keep rs_commit_lsn separately if the catch-up target is still needed after that. Note test_craft_raft_entries.cpp:25-26 currently asserts the present behavior ("a failed fetch, a failed write_slot, or no peer_fetcher_ at all still lets commit_lsn advance"), so those move with the fix.

[sbinmalek]

Replaced the unconditional max() with a walk-forward loop that stalls at the first unresolved Missing slot (skipping Empty ones), mirroring craft_client's reference apply_up_to — exactly the min(rs_commit_lsn, first unresolved missing LSN - 1) semantics you described.

Updated test_craft_raft_entries.cpp:25-26 and the 6 assertions in scope, plus one more I found during review (OnCommitDispatchesSyncRSCommitLSN, which goes through the real on_commit dispatch path with no peer_fetcher_ wired, so commit_lsn can't reach the value it used to assert either).

Same doc pages as the other thread (docs/craft/subtasks.md, docs/craft/rpcs.md) also said "commit_lsn = rs_commit_lsn" — that discrepancy is fixed alongside the token-check wording in 05b5efa, since both sentences shared the same lines.

Fixed in 12b68b9.