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.
|
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.
ea60435 to
7cf7cfe
Compare
| 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_ |
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)
There was a problem hiding this comment.
I will make the change in this PR
| auto self = shared_from_this(); | ||
| detail::detach([self, commit_lsn_snapshot]() -> async_status { | ||
| if (auto cp = co_await self->checkpoint_trigger_->trigger_cp_flush(false); !cp) | ||
| LOGE("checkpoint trigger failed at commit_lsn={}: {}", commit_lsn_snapshot, cp.error().message()); |
There was a problem hiding this comment.
From cp_mgr.cpp: when m_in_flush_phase = true, the path is return ready_bool(false) — immediately, synchronously, not a failure. This is HomeStore's documented "soft trigger dropped because a flush is already running" path, used by its own timer and by dirty-buffer pressure callbacks — they all handle false as a no-op, not an error.
This fires an ERROR-level log every time the Craft trigger lands while a CP is already in progress. Under any sustained write load where CPs overlap the interval (CP flush takes longer than interval / write_rate), this is not one log entry — last_checkpoint_lsn_ is updated on each crossing, so subsequent crossings during the same CP each return false and each log LOGE. In the test CheckpointTriggerFailureDoesNotFailApply, MockCraftCheckpointTrigger only returns false when fail_next = true, so it never exercises the force=false → false path. The bug is invisible in the test suite.
Fix: map force=false + false return to ok(). Only force=true returning false (HomeStore shutting down) is worth surfacing as an error.
There was a problem hiding this comment.
Makes sense. Will make the change
| // doesn't lag arbitrarily far behind commit_lsn. Also invoked from commit_impl() so that | ||
| // write()'s piggyback and keep_alive() (which don't go through this function) advance the | ||
| // same checkpoint cadence -- see checkpoint_interval_crossed_locked()'s doc comment. | ||
| should_checkpoint = checkpoint_interval_crossed_locked(commit_lsn_snapshot); |
There was a problem hiding this comment.
Every checkpoint test in test_craft_raft_entries.cpp uses do_apply → apply_sync_rs_commit_lsn. None call write() → commit() → commit_impl. The commit message for the last commit says: "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" — and that's the code the last commit added. But it has no test.
If commit_impl's interval check had a subtle bug (wrong field, wrong condition, wrong snapshot), the existing test suite would not catch it.
What's needed: a test that advances commit_lsn via write() / commit() instead of do_apply, then asserts trigger_.call_count crossed the interval exactly once. The test_craft_raft_entries.cpp fixture already has write() test infrastructure (WriteSlotFailureDuringCatchup* etc.), so the scaffold is there.
| CraftCheckpointTrigger* checkpoint_trigger_{nullptr}; // null until production wiring; unit tests inject a mock | ||
| int64_t checkpoint_lsn_interval_{128}; // commit_lsn delta between checkpoint triggers; see | ||
| // set_checkpoint_lsn_interval() | ||
| int64_t last_checkpoint_lsn_{-1}; // commit_lsn as of the last triggered checkpoint (guarded by missing_mu_) |
There was a problem hiding this comment.
nit: comment says "last triggered" — should say "last requested" (optimistic)
There was a problem hiding this comment.
Given that trigger is following a fire-and-forget pattern, last requested does not make more sense to me over last triggered
I am fine with changing the variable name to last_triggered_checkpoint_lsn_ so that it sound more true to the nature of the this variable.
Let me know what you think
| EXPECT_EQ(dev_->commit_lsn(), 1); // lsn=1 resolved; stalls at lsn=2, still missing | ||
| } | ||
|
|
||
| // ── checkpoint trigger (SDSTOR-22888) ───────────────────────────────────────── |
There was a problem hiding this comment.
Both commit_impl and apply_sync_rs_commit_lsn read and write the same last_checkpoint_lsn_ under missing_mu_. The correctness property: if path A advances commit_lsn to 200 and fires a checkpoint (setting last_checkpoint_lsn_=200), then path B runs and advances commit_lsn to 210, 210 - 200 = 10 < 128 → no double-fire. Correct in theory.
But no test verifies this. A scenario:
// commit_impl fires: commit_lsn=10, last_checkpoint_lsn_=10, trigger fires (call_count=1)
// apply_sync_rs_commit_lsn runs: advances commit_lsn to 12
// 12 - 10 = 2 < interval → must NOT fire again
EXPECT_EQ(trigger_.call_count, 1);
Without this test, a regression where one path doesn't update last_checkpoint_lsn_ correctly (or uses a stale snapshot) would be invisible.
There was a problem hiding this comment.
Please check the test TEST_F(CraftRaftEntriesTest, CheckpointTriggerBaselineTracksActualReachedValue) in L544
missing_mu_ guards state_, empty_lsns_, commit_running_, in_flight_write_dlsns_, and last_checkpoint_lsn_, not just missing_lsns_ -- the name undersold its scope. Pure rename, no behavior change.
HomeStore's cp_mgr().trigger_cp_flush() returns false synchronously when a flush is already in progress (m_in_flush_phase) -- a benign no-op, not a failure, when force=false. Now only a force=true false return is surfaced as an error. Added a concurrency test that fires many trigger_cp_flush(false) calls at once and asserts none fail.
There was a problem hiding this comment.
🟡 Changes recommended
Empty-slot reclamation can leave readable overlay references to freed blocks, and some checkpoint and validation paths remain incomplete.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Balanced
| 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); } |
| if (hdr.lsn != lsn) { | ||
| LOGE("free_slot: lsn mismatch requested={} stored={} -- refusing to free", lsn, hdr.lsn); | ||
| co_return std::unexpected(std::make_error_condition(std::errc::io_error)); | ||
| } | ||
| if (hdr.all_zeros) co_return ok(); |
| std::lock_guard lk{state_mu_}; | ||
| state_.commit_lsn = lsn; | ||
| } |
| // 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; } |
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).