Sdstor 22886

[sbinmalek]

No description provided.

[sbinmalek]

@copilot review this pr

[Copilot]

Pull request overview

Implements SyncRSCommitLSN RAFT entry dispatch, reconciliation, peer catch-up, and related tests.

Changes:

• Parses and dispatches committed CRAFT RAFT entries.
• Applies commit watermarks, Empty verdicts, and peer-fetched slots.
• Adds error handling and comprehensive unit tests.

Reviewed changes

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

Show a summary per file

File	Description
src/lib/craft/craft_repl_dev.cpp	Implements RAFT dispatch and synchronization apply logic.
src/lib/craft/craft_repl_dev.hpp	Adds async apply signature and test accessors.
src/lib/craft/tests/test_craft_raft_entries.cpp	Tests reconciliation, catch-up, and dispatch.
src/lib/craft/tests/CMakeLists.txt	Registers the new test target.
src/include/homeblks/home_blocks.hpp	Adds the WRONG_TOKEN error.

Suppressed comments (1)

src/lib/craft/craft_repl_dev.cpp:318

• Populating empty_lsns_ does not enforce the documented “Empty beats data” rule on the write path. write() never checks this set, so a late write to an Empty-verdicted dLSN is still persisted and acknowledged, contrary to home_blocks.hpp:203-205 and craft_repl_dev.hpp:161-162. Reject the dLSN under missing_mu_ before calling journal_->write_slot, and cover that behavior in the write tests.

            empty_lsns_.insert(lsn);
            missing_lsns_.erase(lsn);

---

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

[sbinmalek]

Approach 1 — shared_ptr keep-alive

  class CraftReplDev : public std::enable_shared_from_this< CraftReplDev > { ... };                                                                                                                                                                                                                                                                  
                                                                                                                                                                                                                                                                                                                                                   
  async_status CraftReplDev::apply_sync_rs_commit_lsn(...) {                                                                                                                                                                                                                                                                                         
      auto self = shared_from_this();   // lives in the coroutine frame across every co_await                                                                                                                                                                                                                                                        
                                  // rest of the function unchanged                                                                                                                                                                                                                                                                            
  }

• Every construction site (test_craft_raft_entries.cpp's fixture today, whatever S8 adds later) must switch from make_unique to make_shared.
• Destruction is never blocked. If a volume is removed mid-apply, the object's other owners can drop their references immediately; the coroutine's own self copy keeps the memory alive until it finishes, then the object is freed normally.
• Standard idiom for this exact problem (detached async work outliving its nominal owner) — same pattern asio/networking code uses.
• Risk is purely mechanical: if a future call site ever constructs CraftReplDev without a shared_ptr (stack, make_unique, raw new) and that instance's apply_sync_rs_commit_lsn runs, shared_from_this() throws std::bad_weak_ptr immediately — loud and at the call site, not a silent corruption.

Approach 2 — Drain-on-destroy

  CraftReplDev::~CraftReplDev() {                                                                                                                                                                                                                                                                                                                    
      std::unique_lock lk{drain_mu_};                                                                                                                                                                                                                                                                                                                
      drain_cv_.wait(lk, [this] { return inflight_applies_.load() == 0; });                                                                                                                                                                                                                                                                          
  }                                                                                                                                                                                                                                                                                                                                                  

with an RAII guard incrementing/decrementing inflight_applies_ around the body of apply_sync_rs_commit_lsn.

• No change to how CraftReplDev is constructed anywhere — unique_ptr stays fine.
• But ~CraftReplDev() becomes a blocking call: whatever thread destroys the object (e.g. a volume-removal path) stalls until any in-flight fetch_from_peer/write_slot completes. Neither CraftJournalBackend nor CraftPeerFetcher has a timeout or cancellation today, so a slow/unresponsive peer during catch-up could hang that thread
  indefinitely.
• Fixable by making it a co_await-able drain() method instead of a blocking destructor — but then it only helps if whoever writes S8's volume-destroy path remembers to call and await it first, which is the same "easy to forget" risk the first question already flagged for deferring outright.

[Copilot]

@copilot review this pr

Addressed the three issues flagged in the automated review (commit craft: fix review findings):

1. empty_slots out-of-range (lines 316–319): LSNs that are ≤ 0 or > rs_commit_lsn are now filtered before insertion into empty_lsns_, preventing future fetch_data() from reporting phantom Empty verdicts. New test: EmptySlotsOutOfRangeAreIgnored.

2. Untrusted peer response (lines 341–345): An expected-LSN set is built from to_fetch before processing any fetched slot; slots not in the request are silently dropped. This prevents a malformed or mismatched server response from writing arbitrary journal entries or corrupting empty_lsns_/missing_lsns_. New test: PeerResponseWithUnexpectedLsnIsIgnored.

3. Use-after-free in detached coroutine (lines 281–282): CraftReplDev now inherits enable_shared_from_this<CraftReplDev>. In on_commit, shared_from_this() is captured in a wrapper lambda before the coroutine is detached, keeping the device alive across co_await suspension points. The test fixture was updated to use shared_ptr accordingly.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/lib/craft/craft_repl_dev.cpp:423

• This advances commit_lsn even when there is no fetcher, fetching fails, or write_slot fails, while unresolved LSNs remain in missing_lsns_. That contradicts the repository's definition of commit_lsn as a contiguous applied prefix (docs/craft/README.md:35) and the S5 sequence that fetches missing slots before advancing it (docs/craft/subtasks.md:135); callers can therefore observe a synced watermark for data this replica does not hold. Keep the watermark behind the first unresolved LSN, retry catch-up, or mark the replica unhealthy instead of reporting success.

    // Unconditional: commit_lsn is a replica-set-wide watermark RAFT already agreed on, independent of
    // whether this replica's local catch-up succeeded.
    {
        std::lock_guard lk{missing_mu_};
        state_.commit_lsn = std::max(state_.commit_lsn, rs_commit_lsn);

src/lib/craft/craft_repl_dev.cpp:443

• When a lower-term entry is encountered, this creates a session pair that was never committed: client_token comes from the new payload but term remains from the previous login. Subsequent I/O using the old term is then accepted while Sync token checks use the new token. The S5 contract applies both payload fields together (docs/craft/subtasks.md:139-142); assign term directly, or reject the entire stale pair atomically rather than mixing sessions.

    state_.client_token = client_token; // opaque id, no ordering semantics -- plain overwrite
    // term is RAFT-ordered in practice (the leader always proposes strictly increasing terms), but
    // guard against regression the same way commit_lsn/last_append_lsn already do rather than trusting
    // log order blindly.
    state_.term = std::max(state_.term, term);

[sbinmalek]

Added FIXME comments