Sdstor 22888 - #180
Closed
sbinmalek wants to merge 17 commits into
Closed
Conversation
Implement the apply side of the SyncRSCommitLSN RAFT entry: on_commit
now parses the entry header/key and dispatches to
apply_sync_rs_commit_lsn, which reconciles empty_slots, catches up
missing journal data from a peer, and advances the commit_lsn/
last_append_lsn watermarks. InternalLogin dispatch and apply
(SDSTOR-22887) and the checkpoint trigger (SDSTOR-22888) are deliberately
left as stubs for follow-up PRs.
- on_commit: validates header/key blob sizes, parses CraftEntryType and
the SyncRSCommitLSNPayload fixed prefix + empty_slots, and detaches
apply_sync_rs_commit_lsn as fire-and-forget (on_commit is a
synchronous HomeStore callback; apply needs to co_await peer fetch +
journal writes). Logs and no-ops on an unrecognized entry type.
- apply_sync_rs_commit_lsn: a client_token mismatch gates the entire
apply (no reconciliation, no catch-up, no watermark advance).
Otherwise, empty_slots are reconciled into empty_lsns_/missing_lsns_,
the newly-spanned range is marked missing, and catch-up via
CraftPeerFetcher::fetch_from_peer + CraftJournalBackend::write_slot is
best-effort: a failed fetch, a failed write, or no peer_fetcher_ wired
at all just leaves the affected LSNs in missing_lsns_ for a later
attempt. commit_lsn/last_append_lsn advance unconditionally afterward
(never decrement), mirroring truncate()'s existing invariant.
- Add volume_error::WRONG_TOKEN for the client_token-mismatch case.
- Add a _PRERELEASE-only test_listener() accessor so tests can drive
on_commit directly.
- New test_craft_raft_entries.cpp (with a MockCraftPeerFetcher) covering
the token gate, empty_slots reconciliation, watermark advance
(including never-decrements), best-effort catch-up (success, fetch
failure, write failure, unwired fetcher), and on_commit dispatch
including malformed-entry rejection.
- guard CraftRaftEntriesTest friend decl with #ifdef _PRERELEASE
- rename OnCommitLogsUnrecognizedEntryType -> OnCommitIgnoresUnrecognizedEntryType
- add tests: mismatched empty_slots count via on_commit, empty_slots
overlapping the same apply's new gap range
- reject the whole apply (new volume_error::INVALID_ENTRY) if
empty_slots has a negative LSN or one above rs_commit_lsn
- validate a peer's fetch_data response against what was requested;
discard the whole batch on an unrequested/duplicate lsn
- document the known use-after-free gap in the detached
apply_sync_rs_commit_lsn coroutine (not fixed yet)
- add tests for both validations
- implement apply_internal_login: overwrite client_token, max-guard
term against regression; synchronous, called directly from
on_commit (no detail::detach -- no I/O to await)
- wire on_commit's InternalLogin dispatch with an exact-size key
check (no variable trailing data, unlike SyncRSCommitLSN)
- fix write()'s pre-existing unlocked read of state_.term -- latent
until now since nothing mutated it; this ticket arms the race
- add client_token()/term() observability accessors
- add tests: dispatch success/wrong-size, second-login replaces
session, term-never-regresses vs token-always-overwrites, write()
term-fencing end-to-end, and cross-entry-type interaction with
apply_sync_rs_commit_lsn's token check
…sns_ - get_rs_commit_lsn() already covered the same snapshot; empty_lsns_ doesn't need ordering.
…timeout CraftPeerFetcher::fetch_from_peer() had no deadline, so an unresponsive peer could hang apply_sync_rs_commit_lsn's catch-up path forever. Adds peer_fetch_timeout_ms (home_blks_config.fbs, default 5000ms) as a CraftReplDev member with a setter, threaded through to fetch_from_peer's new timeout_ms parameter -- kept off the global config singleton so the standalone craft test binaries (which don't link homeblocks_core) still build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d_ptr ownership - CraftReplDev now extends std::enable_shared_from_this; apply_sync_rs_commit_lsn opens with `auto self = shared_from_this()` so the detached coroutine holds a strong reference across every co_await, keeping CraftReplDev alive even if the last external owner (e.g. a volume-removal path) drops its shared_ptr mid-apply. Closes the KNOWN GAP flagged in review (PR #2, discussion r3761568811). - CraftReplDev's constructor is now private; construction only via the new CraftReplDev::create() factory, so shared_from_this()'s "must already be shared_ptr-owned" precondition is enforced by the compiler instead of a comment. - Update the four craft test fixtures from make_unique/unique_ptr to CraftReplDev::create()/shared_ptr.
…ots apply - fetch_data: classify all requested LSNs under one missing_mu_ acquisition instead of re-locking per LSN. - apply_sync_rs_commit_lsn: range-insert empty_slots into empty_lsns_ instead of inserting one at a time.
…p paths - Empty-verdict reconciliation (to_free): an LSN that was in missing_lsns_ and gets verdicted Empty by this SyncRSCommitLSN may still hold a locally written block from an earlier write() attempt. That block was never reclaimed -- only missing_lsns_ was cleared. Added CraftJournalBackend::free_slot(lsn), which reads the raw local journal entry back off the log store and frees the blkid it references (skipping all_zeros entries, which never allocated one) via the existing free_data. It bypasses read_slot/JournalSlot deliberately: that type is wire-shared with craft::JournalSlot for peer fetch_data responses and carries no blkid (meaningless to a remote peer), so it can't serve this local-only need. - Peer-catchup write_slot failure: alloc_write_data can succeed and then write_slot fail, leaving an allocated block referenced by nothing. This path had no cleanup at all. Now frees it, guarded by blkid_allocated so all_zeros slots (which never allocate) aren't passed to free_data -- mirroring the guard write() already has. The free itself is dispatched via detail::detach() as its own coroutine capturing `self` (not just journal_), since it can outlive the enclosing apply_sync_rs_commit_lsn coroutine, which may return -- and drop its own `self` -- first. - Added free_slot to the four MockCraftJournalBackend test doubles; factored the now-duplicated read_slot/free_slot bodies (identical across test_craft_write.cpp, test_craft_raft_entries.cpp, and test_craft_peer_exchange.cpp) into a new mock_journal_backend.hpp. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s_commit_lsn Addresses two blocking review comments on PR eBay#176 (szmyd): - The client_token != state_.client_token gate vetoed the login sequence's own SyncRSCommitLSN: per CRAFT-Design, SyncRSCommitLSN applies before the InternalLogin that establishes client_token, so the check always mismatched on login (and on every post-restart watchdog SyncRSCommitLSN, since state_ is in-memory-only). Dropped the check, matching craft_client's reference (MemCraftReplica::cold_apply_sync discards the parameter outright). Exclusivity comes from RAFT's commit ordering plus the term fence every other IO already checks. - commit_lsn was advancing unconditionally to rs_commit_lsn regardless of local catch-up outcome, conflating it with the replica-set-wide watermark. CRAFT-Design defines commit_lsn as the local contiguous prefix: skip Empty slots, but never advance past an unresolved Missing one. Replaced the unconditional max() with a walk-forward loop mirroring craft_client's reference apply_up_to. Updated test_craft_raft_entries.cpp accordingly: repurposed the two tests that asserted the old token-gate behavior into regression guards for the new behavior, and corrected 7 commit_lsn assertions (6 from the review scope plus one found during review, OnCommitDispatchesSyncRSCommitLSN) to the new stall-at-first-missing semantics. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…mit_lsn fixes docs/craft/subtasks.md and docs/craft/rpcs.md both said the apply "verifies token" and "commit_lsn = rs_commit_lsn" -- exactly the behavior removed in the previous commit. Reworded both to describe the actual behavior: client_token is carried on the entry but not checked against local state, and commit_lsn advances to the contiguous prefix bounded by rs_commit_lsn, skipping Empty slots but never past an unresolved Missing one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Implements SDSTOR-22888. After apply_sync_rs_commit_lsn advances commit_lsn, nudge HomeStore to checkpoint once the advance since the last trigger crosses checkpoint_lsn_interval_. Otherwise the journal-reclaim / RAFT-log-compaction floor (docs/craft/subtasks.md's S8) can lag arbitrarily far behind commit_lsn, unbounding restart recovery time. - New CraftCheckpointTrigger interface + HomeStoreCraftCheckpointTrigger production impl wrapping homestore::cp_mgr().trigger_cp_flush(), following the same inject-an-abstraction pattern as CraftJournalBackend/CraftPeerFetcher so unit tests (which run with no live HomeStore instance) can exercise the trigger via a mock. CraftReplDev takes it as a non-owning pointer, same shape as peer_fetcher_, since cp_mgr() is one instance shared by every volume, not owned per-CraftReplDev. - The trigger call is detail::detach()'d (fire-and-forget), matching the existing free_data cleanup pattern in this same function -- nothing depends on the flush completing. - force=false: let it coalesce with any checkpoint already in flight rather than forcing back-to-back flushes under high commit throughput. - Left two forward-looking FIXMEs for related gaps out of this ticket's scope: seeding last_checkpoint_lsn_ from recovered commit_lsn once S8 restart recovery lands, and forcing a completed (not just requested) flush before truncate() drops journal entries, mirroring HomeStore's own IndexTable::destroy(). - Tests: MockCraftCheckpointTrigger covers interval gating (fires-once-crossed, below-interval, accumulates-across-calls, exact boundary, baseline tracks the actual commit_lsn reached rather than incrementing by the interval), null-trigger safety, and best-effort failure handling. test_craft_homestore_backend.cpp gets two new cases (force=false and force=true) exercising the production wrapper against a real cp_mgr() -- previously untested against anything but the mock. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
to_free was freeing lsns that *were* missing (nothing to free there) instead of ones that held real local data (<= last_append_lsn, not missing) -- exactly the leak shosseinimotlagh flagged on PR eBay#176 and Copilot's review re-caught. Also guards against double-freeing an lsn already verdicted Empty. Adds a free_data_calls counter to test_craft_raft_entries.cpp's mock and locks in all four branches of the condition.
Corrupt or foreign records could get misread as a valid blkid otherwise. Flagged by Copilot's review on PR eBay#176. Adds real-log-store tests for both a legitimate entry and a rejected corrupt one.
to_free was a vector, so a duplicate lsn in a single empty_slots list would call free_slot on the same lsn twice -- a double-free. Switched to unordered_set. Adds a test for the intra-batch duplicate case. Found during review of PR eBay#176's changes.
There was a problem hiding this comment.
🟡 Changes recommended
Production wiring is absent, in-flight checkpoints can suppress required follow-up flushes, and one test can hang indefinitely.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds checkpoint triggering and safer empty-slot reclamation to CRAFT commit processing.
Changes:
- Adds interval-based HomeStore checkpoint triggers.
- Prevents duplicate block frees and validates journal entries.
- Expands unit and integration coverage.
File summaries
| File | Description |
|---|---|
src/lib/home_blks_config.fbs |
Documents checkpoint cadence configuration. |
src/lib/craft/craft_repl_dev.hpp |
Defines checkpoint trigger interfaces and state. |
src/lib/craft/craft_repl_dev.cpp |
Implements checkpoint and reclamation logic. |
src/lib/craft/tests/test_craft_raft_entries.cpp |
Tests checkpoint gating and empty-slot cleanup. |
src/lib/craft/tests/test_craft_homestore_backend.cpp |
Tests HomeStore checkpoint and journal behavior. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+798
to
+801
| if (checkpoint_trigger_ == nullptr) { | ||
| LOGW("apply_sync_rs_commit_lsn: commit_lsn={} crossed checkpoint interval but no " | ||
| "checkpoint_trigger_ wired -- skipping", | ||
| commit_lsn_snapshot); |
Comment on lines
+803
to
+805
| detail::detach([self, commit_lsn_snapshot]() -> async_status { | ||
| if (auto cp = co_await self->checkpoint_trigger_->trigger_cp_flush(false); !cp) | ||
| LOGE("apply_sync_rs_commit_lsn: checkpoint trigger failed at commit_lsn={}: {}", |
| std::mutex mu; | ||
| std::condition_variable cv; | ||
| bool done = false; | ||
| logstore->write_async(/* seq_num = */ 0, raw_blob, nullptr, |
Contributor
There was a problem hiding this comment.
related to https://jirap.corp.ebay.com/browse/SDSTOR-24993?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.