craft: S4 truncate path — CraftReplDev::truncate#170
Conversation
b67500a to
6da68b0
Compare
There was a problem hiding this comment.
Pull request overview
Adds the S4 “truncate” path to CraftReplDev by introducing an injectable journal-backend seam (CraftJournalBackend::truncate_to()), implementing a HomeStore-backed production backend, and adding a focused unit test that exercises rollback/clamp/missing-set behavior.
Changes:
- Implement
CraftReplDev::truncate(lsn)to rollback the journal, clamplast_append_lsn, and erase missing-set entries abovelsn(without changingcommit_lsn). - Introduce
CraftJournalBackend::truncate_to()plusmake_homestore_journal_backend()(HomeStore production implementation). - Add a new unit test target
test_craft_truncatethat compilescraft_repl_dev.cppdirectly and drives truncate behavior with a mock backend.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/lib/craft/tests/test_craft_truncate.cpp | New unit tests covering truncate success cases and journal-failure no-mutation invariant. |
| src/lib/craft/tests/CMakeLists.txt | Adds test_craft_truncate target and registers it with CTest. |
| src/lib/craft/craft_repl_dev.hpp | Extends journal backend interface; adds observability helpers and prerelease-only seeding helper. |
| src/lib/craft/craft_repl_dev.cpp | Implements HomeStore-backed journal backend and CraftReplDev::truncate(). |
| src/lib/craft/CMakeLists.txt | Adds craft tests subdirectory. |
| conanfile.py | Bumps package version to 6.0.3. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6da68b0 to
885eeaa
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/v6.x #170 +/- ##
===========================================
Coverage ? 45.00%
===========================================
Files ? 18
Lines ? 1020
Branches ? 442
===========================================
Hits ? 459
Misses ? 274
Partials ? 287 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
434dd59 to
885eeaa
Compare
Drop all journal entries with dLSN > lsn via HomeStore rollback(), clamp last_append_lsn, and erase missing-set entries above lsn. commit_lsn is not touched. A journal I/O failure short-circuits before state is mutated. Introduces CraftJournalBackend::truncate_to() as the injectable seam so unit tests drive the path with MockCraftJournalBackend without HomeStore. HomeStoreCraftJournalBackend (wrapping home_log_store::rollback) is the production implementation; make_homestore_journal_backend() is the factory for callers that own a log store (volume.cpp, wired up in a later PR). Observability accessors last_append_lsn(), commit_lsn(), missing_count(), and is_missing() are unconditional public methods usable in production. seed_lsns() is compiled only when _PRERELEASE is defined (set by the test target in CMake) so the symbol is absent from production binaries.
885eeaa to
d7ff178
Compare
|
Reviewed the S4 truncate path in isolation; the core logic is correct and the test coverage is thorough (clamp above/at/below Three notes on the concurrency contract this PR establishes. All are latent today (these methods have no callers yet), but worth settling now while the locking discipline is being set; #1 in particular is the exact race a later TSAN run will surface once the peer/login paths land. 1. Inconsistent locking of the partition watermarks - This PR makes Since this PR sets the locking contract, suggest making it consistent: either take 2. If a caller ever passes DEBUG_ASSERT_GE(lsn, state_.commit_lsn, "truncate below committed prefix");3. As of this PR |
Fix two concurrency issues flagged by szmyd: 1. get_lsns() / get_rs_commit_lsn() now hold missing_mu_ when snapshotting state_.commit_lsn / state_.last_append_lsn, consistent with truncate() (writer) and the observability accessors. get_rs_commit_lsn() is documented as a peer RPC that can race with truncate() from a concurrent login; the lock-free read was a latent data race. 2. truncate() now asserts lsn >= state_.commit_lsn inside missing_mu_ before clamping last_append_lsn. Truncating below the committed prefix would drop committed journal entries and break the commit_lsn <= last_append_lsn invariant; the assert turns caller misuse into a prerelease failure instead of silent watermark corruption.
szmyd
left a comment
szmyd
left a comment
There was a problem hiding this comment.
The DEBUG_ASSERT_GE fires after the journal has already been rolled back (craft_repl_dev.cpp:145):
if (auto r = co_await journal_->truncate_to(lsn); !r) co_return r; // journal truncated here
{
std::lock_guard lk{missing_mu_};
DEBUG_ASSERT_GE(lsn, state_.commit_lsn, "truncate below committed prefix"); // too lateIf a caller passes lsn < state_.commit_lsn, the journal is already truncated below the committed prefix before the assert catches it — committed entries are permanently gone. The check needs to move before step 1:
async_status CraftReplDev::truncate(int64_t lsn) {
{
std::lock_guard lk{missing_mu_};
DEBUG_ASSERT_GE(lsn, state_.commit_lsn, "truncate below committed prefix");
}
if (auto r = co_await journal_->truncate_to(lsn); !r) co_return r;
...The DEBUG_ASSERT_GE was placed after journal_->truncate_to(), so a caller passing lsn < commit_lsn would permanently lose committed journal entries before the assert had a chance to fire. Move the check into its own lock scope before step 1 so misuse is caught before any irreversible I/O is issued.
Drop all journal entries with dLSN > lsn via HomeStore rollback(), clamp last_append_lsn, and erase missing-set entries above lsn. commit_lsn is not touched. A journal I/O failure short-circuits before state is mutated.
Introduces CraftJournalBackend::truncate_to() as the injectable seam so unit tests drive the path with MockCraftJournalBackend without HomeStore. HomeStoreCraftJournalBackend (wrapping home_log_store::rollback) is the production implementation; make_homestore_journal_backend() is the factory for callers that own a log store (volume.cpp, wired up in a later PR).
Observability accessors last_append_lsn(), commit_lsn(), missing_count(), and is_missing() are unconditional public methods usable in production. seed_lsns() is compiled only when _PRERELEASE is defined (set by the test target in CMake) so the symbol is absent from production binaries.