CraftReplDev write path#165
Open
sbinmalek wants to merge 4 commits into
Open
Conversation
Adds STALE_TERM to volume_error so CRAFT write/read rejections are branchable by callers (triggers client re-login). Introduces HomeStoreCraftJournalBackend as the production CraftJournalBackend implementation and make_homestore_journal_backend() as its factory; each journal method is stubbed pending HomeStore CRAFT journal APIs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Term check rejects stale-session IOs with STALE_TERM. Missing set is updated under lock before the journal write so gaps are conservatively tracked even on write failure. last_append_lsn advances to max of current and incoming LSN. Journal append is zero-copy; LBA index is not touched (that is commit()'s job). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add missing_count() / is_missing() const observability methods;
mark missing_mu_ mutable so const methods can acquire the lock.
- Give JournalSlot::data a default member initializer ({}) so partial
aggregate initializers don't trigger -Wmissing-field-initializers.
- Wire craft/tests/ into CMake and add test_craft_write:
MockCraftJournalBackend (in-memory, map-backed), CraftWriteTest
fixture, and three tests covering in-order writes, out-of-order
gap tracking, and STALE_TERM rejection.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements the client-facing write path for CraftReplDev (CRAFT epic S2), introducing a new volume_error::STALE_TERM for session/term rejection and adding initial unit tests plus a stub production journal backend factory.
Changes:
- Add
CraftReplDev::write()implementation with term checking and missing-LSN tracking. - Introduce
HomeStoreCraftJournalBackendstub +make_homestore_journal_backend()factory for production wiring. - Add
CraftWriteTestwith an in-memory journal backend and CMake integration for craft tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/craft/tests/test_craft_write.cpp | Adds unit tests and an in-memory MockCraftJournalBackend to validate write ordering and term rejection behavior. |
| src/lib/craft/tests/CMakeLists.txt | Adds test_craft_write target and registers it with CTest. |
| src/lib/craft/craft_repl_dev.hpp | Adds missing-set observability helpers, makes the mutex mutable, and declares the HomeStore journal backend factory. |
| src/lib/craft/craft_repl_dev.cpp | Adds the production backend stub + factory and replaces the write() stub with a real implementation. |
| src/lib/craft/CMakeLists.txt | Adds tests/ subdirectory to the craft build. |
| src/include/homeblks/home_blocks.hpp | Extends volume_error with STALE_TERM. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+85
to
+107
| // 2. Update missing set and last_append_lsn under lock. | ||
| // Done before the journal write so the missing set is always conservative: | ||
| // a failed write leaves the LSN in the set (correctly marked absent). | ||
| { | ||
| std::lock_guard lock{missing_mu_}; | ||
| // Any LSNs between the old last_append and this one are gaps. | ||
| for (int64_t gap = state_.last_append_lsn + 1; gap < lsn; ++gap) { | ||
| missing_lsns_.insert(gap); | ||
| } | ||
| // This slot is being filled. | ||
| missing_lsns_.erase(lsn); | ||
| state_.last_append_lsn = std::max(state_.last_append_lsn, lsn); | ||
| } | ||
|
|
||
| // 3. Journal append — zero-copy; data buffer is not copied on the hot path. | ||
| auto res = co_await journal_->write_slot(lsn, lba, len, std::move(data)); | ||
| if (!res) { | ||
| LOGE("write_slot failed lsn={} lba={} len={}: {}", lsn, lba, len, res.error().message()); | ||
| co_return std::unexpected(res.error()); | ||
| } | ||
|
|
||
| LOGT("write ok lsn={} lba={} len={}", lsn, lba, len); | ||
| co_return homestore::ok(); |
Comment on lines
+79
to
+83
| // 1. Term check — old-session client; must re-login. | ||
| if (term != state_.term) { | ||
| LOGW("write rejected: stale term want={} got={} lsn={}", state_.term, term, lsn); | ||
| co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); | ||
| } |
Comment on lines
+69
to
+71
| ENUM(volume_error, uint16_t, UNKNOWN_VOLUME = 1, CRC_MISMATCH, INDEX_ERROR, INTERNAL_ERROR, OFFLINE, | ||
| STALE_TERM | ||
| ); |
| // Term rejection: write with term != state_.term returns STALE_TERM. | ||
| // Journal must not be touched. | ||
| TEST_F(CraftWriteTest, TermRejection) { | ||
| // state_.term starts at 0; term=1 is stale. |
sbinmalek
force-pushed
the
craft-write-path
branch
from
18dc68d to
fd58967
Compare
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.
Implements the client-facing write path for CraftReplDev (S2 of the CRAFT epic). Builds on the S1 foundation; the next subtask (S3) will add read, commit, and keep-alive.
Test plan