Conversation
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.
write_async's return value was never checked and the wait had no timeout, so a stopping log store or a lost completion (both documented failure modes) would hang the whole test binary instead of failing. Found during review of PR eBay#180's changes.
… in tests - Fixed tests after rebasing
There was a problem hiding this comment.
🟡 Changes recommended
Checkpoint wiring and coverage are incomplete, and duplicate Empty verdicts plus timeout cleanup have correctness risks.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds proactive CRAFT checkpoint triggering and strengthens journal block-reclamation validation.
Changes:
- Adds checkpoint-trigger abstraction and interval gating.
- Fixes and tests Empty-slot block reclamation.
- Adds journal validation and HomeStore integration tests.
File summaries
| File | Description |
|---|---|
src/lib/home_blks_config.fbs |
Documents the shared checkpoint interval. |
src/lib/craft/craft_repl_dev.hpp |
Defines checkpoint interfaces and state. |
src/lib/craft/craft_repl_dev.cpp |
Implements checkpoint triggering and safer block reclamation. |
src/lib/craft/tests/test_craft_raft_entries.cpp |
Tests checkpoint and Empty-slot behavior. |
src/lib/craft/tests/test_craft_homestore_backend.cpp |
Tests real checkpoint and journal validation paths. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 4
- 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
+1222
to
+1224
| // TODO(SDSTOR-22733): once implemented, this is the other commit_lsn-advance path SDSTOR-22888's | ||
| // checkpoint trigger needs to cover (see apply_sync_rs_commit_lsn's own hook) -- same | ||
| // checkpoint_lsn_interval_/last_checkpoint_lsn_ bookkeeping under missing_mu_, same force=false. |
Comment on lines
1649
to
1653
| for (int64_t lsn : empty_slots) { | ||
| if (missing_lsns_.erase(lsn)) { to_free.push_back(lsn); } | ||
| bool const was_missing = missing_lsns_.erase(lsn) > 0; | ||
| if (!was_missing && lsn <= state_.last_append_lsn && !empty_lsns_.contains(lsn)) { to_free.insert(lsn); } | ||
| } | ||
| empty_lsns_.insert(empty_slots.begin(), empty_slots.end()); |
Contributor
Author
There was a problem hiding this comment.
This is less likely to happen. Dont need to make this change
Comment on lines
+369
to
+373
| // Overrides the commit_lsn delta between checkpoint triggers (default matches | ||
| // sync_rs_commit_lsn_interval's own default of 128, tying checkpoint cadence to the periodic | ||
| // SyncRSCommitLSN cadence). Production sets this from HB_DYNAMIC_CONFIG(sync_rs_commit_lsn_interval) | ||
| // after construction, same pattern as set_peer_fetch_timeout_ms. | ||
| void set_checkpoint_lsn_interval(int64_t n) { checkpoint_lsn_interval_ = n; } |
Contributor
Author
There was a problem hiding this comment.
it's tracked as S8 work
Comment on lines
+369
to
+373
| auto write_ret = logstore->write_async( | ||
| /* seq_num = */ 0, raw_blob, nullptr, | ||
| [&](homestore::logstore_seq_num_t, sisl::io_blob&, homestore::logdev_key, void*) { | ||
| std::lock_guard< std::mutex > lk{mu}; | ||
| done = true; |
Contributor
Author
There was a problem hiding this comment.
Issue is just within tests. keeping it as it is
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/v6.x #181 +/- ##
===========================================
Coverage ? 49.10%
===========================================
Files ? 19
Lines ? 1224
Branches ? 534
===========================================
Hits ? 601
Misses ? 266
Partials ? 357 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The proactive checkpoint trigger only fired inside apply_sync_rs_commit_lsn, so write()'s piggybacked commit and keep_alive() -- the only two paths that advance commit_lsn on an ordinary (non-SyncRSCommitLSN) workload -- never triggered a checkpoint at all. Extracted the interval-check and the detached trigger_cp_flush call into checkpoint_interval_crossed_locked()/ fire_checkpoint_trigger(), and call both from commit_impl()'s return path (covering commit()'s write()/keep_alive() callers) as well as apply_sync_rs_commit_lsn()'s own walk-forward loop.
sbinmalek
force-pushed
the
SDSTOR-22888-v2
branch
from
September 11, 2026 23:26
ea60435 to
7cf7cfe
Compare
sbinmalek
marked this pull request as ready for review
September 11, 2026 23:26
| mutable std::mutex | ||
| missing_mu_; // guards state_, missing_lsns_, empty_lsns_, commit_running_, in_flight_write_dlsns_ | ||
| mutable std::mutex missing_mu_; // guards state_, missing_lsns_, empty_lsns_, commit_running_, | ||
| // in_flight_write_dlsns_, and last_checkpoint_lsn_ |
Contributor
There was a problem hiding this comment.
need to change the name eventually. it is generic purpose and I hesitated to do it previously (my bad)
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
commit_lsnadvances, instead of waitingfor HomeStore's own timer, so truncation can reclaim journal space sooner
(
craft_repl_dev.{cpp,hpp},home_blks_config.fbsadds the trigger's tunable interval).apply_sync_rs_commit_lsn: theto_freecomputation forempty_slotshad an inverted condition, so a locally-written slot that got Empty-verdicted never had its
block freed, while genuinely-missing slots (nothing to free) were queued instead. Corrected to
free only slots that were locally held and not already Empty-verdicted, guarding against
double-freeing on a duplicate/overlapping verdict.
to_freewith a set to avoid queuing the same lsn twice.free_slotbefore trusting a journal entry'sall_zerosflag and blkid, so a corrupt or foreign record can't be misread and free the wrong storage.
FreeSlotRejectsCorruptEntry's completion wait so a hang fails the test instead ofblocking indefinitely.
test_craft_homestore_backend.cppleft on the pre-S3make_homestore_journal_backend/write_slotcall signatures after thedev/v6.xS3 rebase(missing
page_sizeandcsumsargs respectively).