SDSTOR-25714 craft: checkpoint-trigger design cleanup (PR #181 follow-ups) - #182
Merged
Merged
Conversation
…ction Matches the write_index_fn_t/delete_index_fn_t/read_index_fn_t convention already used elsewhere in this class for injecting test doubles.
Moves the checkpoint-interval check into the RAII guard's destructor so it runs on every exit path, not just the happy-path tail, and merges it with the destructor's existing commit_running_ reset into one critical section.
Both call sites (commit(), commit_with()) construct a fresh lambda locally and never reuse it afterward, so moving it in avoids a std::function copy into the coroutine frame.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The newly added error-exit checkpoint behavior lacks focused regression coverage, and the ownership documentation is inaccurate.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Replaces checkpoint-trigger polymorphism with callable injection and ensures checkpoint checks run on all commit exit paths.
Changes:
- Introduces
checkpoint_trigger_fn_t. - Moves checkpoint evaluation into
RunningGuard. - Moves commit callbacks into
commit_impl.
| File | Description |
|---|---|
src/lib/craft/craft_repl_dev.hpp |
Defines the callable trigger API. |
src/lib/craft/craft_repl_dev.cpp |
Implements callable triggers and exit-path checkpointing. |
src/lib/craft/tests/test_craft_raft_entries.cpp |
Adapts trigger mocks to callable injection. |
src/lib/craft/tests/test_craft_homestore_backend.cpp |
Updates factory integration tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+830
to
+834
| self->commit_running_ = false; | ||
| final_commit_lsn = self->state_.commit_lsn; | ||
| should_checkpoint = self->checkpoint_interval_crossed_locked(final_commit_lsn); | ||
| } | ||
| if (should_checkpoint) self->fire_checkpoint_trigger(final_commit_lsn); |
Comment on lines
+164
to
+167
| // Factory that wraps homestore::cp_mgr(). One instance is shared by every volume's CraftReplDev | ||
| // (there is exactly one CPManager per HomeStore instance), unlike make_homestore_journal_backend | ||
| // which is per-volume -- so CraftReplDev takes this via a non-owning pointer (set_checkpoint_trigger), | ||
| // not ownership at construction. Tests inject MockCraftCheckpointTrigger directly. | ||
| unique< CraftCheckpointTrigger > make_homestore_checkpoint_trigger(); | ||
| // which is per-volume -- so CraftReplDev takes this via a non-owning fn (set_checkpoint_trigger), | ||
| // not ownership at construction. Tests inject a plain callable directly. |
sbinmalek
force-pushed
the
SDSTOR-25714
branch
from
September 22, 2026 02:32
a99d0f4 to
a4595ac
Compare
make_homestore_checkpoint_trigger_fn's doc comment still described the old non-owning pointer model; each CraftReplDev now stores its own copy of the callable by value. Also adds a regression test proving the checkpoint trigger fires from RunningGuard's destructor even when a later slot in the same commit_impl run aborts with an error, not just on the happy path.
sbinmalek
force-pushed
the
SDSTOR-25714
branch
from
September 22, 2026 03:52
a4595ac to
a544d21
Compare
sbinmalek
marked this pull request as ready for review
September 22, 2026 03:52
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/v6.x #182 +/- ##
===========================================
Coverage ? 49.02%
===========================================
Files ? 19
Lines ? 1226
Branches ? 535
===========================================
Hits ? 601
Misses ? 268
Partials ? 357 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
raakella1
reviewed
Sep 22, 2026
| @@ -946,15 +949,8 @@ async_result< int64_t > CraftReplDev::commit_impl(int64_t upto_lsn, write_index_ | |||
| state_.commit_lsn = lsn; | |||
Contributor
There was a problem hiding this comment.
you can maintain a local variable final_commmit_lsn which you can define above the struct RunningGuard and have RunningGuard hold a ptr to it.
int64_t final_commmit_lsn;
struct RunningGuard {
CraftReplDev* self;
int64_t* final_commmit_lsn_ptr;
~RunningGuard() {
bool should_checkpoint;
{
std::lock_guard lk{self->state_mu_};
self->state_.commit_lsn = *final_commmit_lsn_ptr;
self->commit_running_ = false;
should_checkpoint = self->checkpoint_interval_crossed_locked(final_commit_lsn);
}
if (should_checkpoint) self->fire_checkpoint_trigger(final_commit_lsn);
}
} guard(this, &final_commit_lsn);
Update that variable without holding any lock at the end of each loop (line 749)
You can also get rid of the lock before return and use return final_commmit_lsn
raakella1
reviewed
Sep 22, 2026
Replaces the per-iteration state_mu_-guarded write to state_.commit_lsn with a local final_commit_lsn updated lock-free, written to shared state exactly once in RunningGuard's destructor. Documents the accepted staleness tradeoff this introduces for concurrent read_impl() callers.
raakella1
approved these changes
Sep 23, 2026
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.
Summary
CraftCheckpointTrigger's virtual interface withstd::function, matching thewrite_index_fn_t/delete_index_fn_t/read_index_fn_tconvention already used elsewhere in thisclass for injecting test doubles. No changes this session
commit_impl's checkpoint-interval check intoRunningGuard's destructor so it runs on everyexit path (including an early error return mid-loop), not just the happy-path tail, and merges it
with the destructor's existing
commit_running_reset into one critical section.commit_impl'swrite_index_fn_t/delete_index_fn_tparams by value instead ofconst&, andstd::movethem in at both call sites (commit(),commit_with()), since neither caller reusesits lambda afterward.