Sdstor 22886 - #173
Closed
sbinmalek wants to merge 4 commits into
Closed
Conversation
…counter
Add the wire format definitions and scaffolding needed by S5
(SyncRSCommitLSN + InternalLogin RAFT state machine entries).
Implementations land in follow-up PRs; this PR makes the base branch
compile cleanly.
- craft_raft_entries.hpp: CraftEntryType enum, CraftEntryHeader,
SyncRSCommitLSNPayload (variable-length with trailing int64_t array),
InternalLoginPayload, and helpers (sync_rs_commit_lsn_key_size,
serialize_sync_rs_commit_lsn, parse_empty_slots)
- craft_repl_dev.hpp: add CraftPeerFetcher interface (server-to-server
fetch abstraction; production wired in S9), set_peer_fetcher() setter,
write_counter_ atomic (periodic SyncRSCommitLSN auto-fire trigger),
update apply_sync_rs_commit_lsn signature to carry empty_slots
- craft_repl_dev.cpp: match stub signature to updated header
- home_blks_config.fbs: add sync_rs_commit_lsn_interval setting (128)
- parse_empty_slots now takes the raw key blob instead of a trusted
struct pointer, and returns std::optional instead of an unconditional
vector. Validates the fixed prefix is present and that the persisted
num_empty_slots exactly accounts for the blob's actual size before
ever constructing the vector -- a corrupt/truncated RAFT entry no
longer risks an out-of-bounds read or a bogus multi-GB allocation
during replay.
- serialize_sync_rs_commit_lsn now zeroes the fixed prefix before
writing fields, so SyncRSCommitLSNPayload's compiler-inserted trailing
padding (20 real bytes, sizeof rounds to 24) can't leak prior buffer
contents into a persisted/replicated entry or make equivalent entries
byte-different.
- Guard the empty_slots memcpy: vector::data() may return null when
empty, and memcpy(dest, nullptr, 0) is UB even at zero length.
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
There was a problem hiding this comment.
Pull request overview
Adds CRAFT SyncRSCommitLSN RAFT entry handling, replica catch-up, and associated tests.
Changes:
- Defines and parses CRAFT RAFT entry payloads.
- Applies commit watermarks, Empty verdicts, and peer catch-up.
- Adds configuration, errors, tests, and a package version bump.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
conanfile.py |
Bumps package version to 6.0.5. |
src/include/homeblks/home_blocks.hpp |
Adds WRONG_TOKEN. |
src/lib/home_blks_config.fbs |
Adds the proposal interval setting. |
src/lib/craft/craft_raft_entries.hpp |
Defines RAFT wire payloads and helpers. |
src/lib/craft/craft_repl_dev.hpp |
Adds peer-fetch and apply interfaces. |
src/lib/craft/craft_repl_dev.cpp |
Implements dispatch, catch-up, and watermark application. |
src/lib/craft/tests/test_craft_raft_entries.cpp |
Tests entry parsing and application behavior. |
src/lib/craft/tests/CMakeLists.txt |
Registers the new test executable. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+25
to
+26
| // how often (in appended LSNs) the leader auto-proposes a SyncRSCommitLSN entry; | ||
| sync_rs_commit_lsn_interval: uint32 = 128; |
Comment on lines
+279
to
+282
| // apply_sync_rs_commit_lsn co_awaits peer fetch + journal writes; on_commit itself is a synchronous | ||
| // HomeStore callback, so fire-and-forget it. | ||
| detail::detach(owner_->apply_sync_rs_commit_lsn(payload->rs_commit_lsn, payload->client_token, | ||
| std::move(*empty_slots))); |
Comment on lines
+316
to
+319
| for (int64_t lsn : empty_slots) { | ||
| empty_lsns_.insert(lsn); | ||
| missing_lsns_.erase(lsn); | ||
| } |
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.