fix #290: reduce snapshot cooldown and fix check throttling

[JoshuaChi]

What Does This PR Do?

Reduces the default snapshot_cool_down_since_last_check from 1 hour to 60 seconds,
and applies three minor correctness fixes to LogSizePolicy to make snapshot
triggering reliable without requiring manual config tuning.

Type:

• Bug Fix (with test)

---

Why Is This Needed?

The default 3600s cooldown effectively disabled log compaction for typical workloads.
Even with max_log_entries_before_snapshot = 1000, the cluster would wait 1 hour
between snapshot evaluations, causing unbounded log growth, increasing startup memory
consumption, and degrading cluster lifetime performance. Users had to explicitly tune
the config to get reasonable behavior.

The three accompanying fixes address latent issues in LogSizePolicy:

• now - last could underflow on clock jumps (replaced with saturating_sub)
• Ordering::Acquire on last_checked load was heavier than needed; the CAS on
  is_checking already provides mutual exclusion, so Relaxed is correct
• Added a warn! when lag exceeds 10× threshold to surface misconfigured or stalled
  snapshot pipelines before they become operational incidents

---

Checklist

Required:

• make test passes
• Added tests for new code
• Commits squashed to 1-2 logical units

---

Testing

How tested:

• Unit tests: existing log_size_test.rs suite (17 tests) — all pass
• Integration tests: test_learner_snapshot_concurrent_replication,
  test_leader_snapshot_concurrent_writes,
  test_follower_snapshot_generation_during_replication — all pass

For bug fixes:

• Added test that fails without this fix
  (high_frequency_performance and respects_cooldown_period exercise the fixed paths)

---

Does This Follow d-engine's Principles?

• Solves a real problem for most users (not just my edge case)
• Keeps implementation simple
• Doesn't bloat the API surface

---

Reviewer Notes

The adaptive cooldown proposed in PR #338 was evaluated but not included — it
introduced a regression where a below-threshold check would reset last_checked,
blocking subsequent detection of rapid log growth. The root cause analysis is
documented in the commit message. The three correctness fixes from #338 are applied
as-is; only the adaptive cooldown logic was dropped.

Estimated review complexity:

• Quick (< 100 lines)

Summary by CodeRabbit

• Configuration Changes

  • Reduced default snapshot check cooldown from one hour to one minute for more responsive evaluations

• Observability Improvements

  • Added warning logs when snapshot log lag reaches 10× the configured threshold to surface potential issues

• Optimizations

  • Refined snapshot decision and cooldown handling to improve reliability around boundary conditions

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6a216e97-6ad3-436e-a883-7cee939427eb

📥 Commits

Reviewing files that changed from the base of the PR and between a97a1ce and da2ae76.

📒 Files selected for processing (1)

• d-engine-core/src/config/raft.rs

---

📝 Walkthrough

Walkthrough

Reduced the default snapshot cooldown from 3600s to 60s and tightened snapshot log-size policy: added a heavy-lag warning, switched atomic loads/stores to Relaxed ordering, used saturating subtraction for time, and refactored decision locals.

Changes

Cohort / File(s)	Summary
Snapshot Configuration d-engine-core/src/config/raft.rs	Changed helper default for snapshot_cool_down_since_last_check() from Duration::from_secs(3600) to Duration::from_secs(60) and updated the doc comment.
Snapshot Policy Logic d-engine-core/src/state_machine_handler/snapshot_policy/log_size.rs	Added tracing::warn when log lag ≥ 10× threshold (guarded by threshold > 0); switched atomic last_checked reads/writes to Ordering::Relaxed; replaced now - last with now.saturating_sub(last); refactored to compute lag and threshold locals and used lag >= threshold for trigger decision.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• Reduce default snapshot cooldown from 1 hour to enable practical log compaction #290: Matches changes to reduce default cooldown from 3600s→60s and update snapshot policy logging and atomic ordering.

Poem

🐰
A quicker hop, a briefer pause,
I tally logs without a cause.
When lag runs wild I shout “beware!”
I scribble times with careful care.
Snapshots hum — I twitch my nose.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the two main changes: reducing snapshot cooldown and fixing check throttling issues in LogSizePolicy.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/290_reduce_default_snapshot_cooldown

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@d-engine-core/src/config/raft.rs`:
- Around line 622-623: The doc comment on LogSizePolicy incorrectly states that
the cooldown "shrinks automatically" as log lag approaches the snapshot
threshold; update the documentation for the LogSizePolicy struct/impl to reflect
the actual runtime behavior by either (A) removing or rephrasing the adaptive
language to state that the provided cooldown is a fixed/base value and that no
adaptive shrinking is performed, or (B) if adaptive cooldown is intended,
implement the adaptive logic in the LogSizePolicy methods (e.g., where cooldown
is computed) so the cooldown value is reduced based on current log lag relative
to the snapshot threshold; reference LogSizePolicy and the cooldown/base
cooldown wording when making the change.

In `@d-engine-core/src/state_machine_handler/snapshot_policy/log_size.rs`:
- Around line 63-66: The cooldown timestamp (self.last_checked) is only updated
when should_trigger is true, so non-trigger checks bypass the cooldown; move the
store so last_checked.store(now, Ordering::Relaxed) is executed on every check
(i.e., unconditionally after computing should_trigger/lag/threshold) so the
"cooldown since last check" semantics are honored while preserving the existing
variables (self.last_checked, now, should_trigger, lag, threshold) and memory
ordering.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 503d70ec-a195-49af-91f2-9716f03f7b51

📥 Commits

Reviewing files that changed from the base of the PR and between 79ef2f9 and a97a1ce.

📒 Files selected for processing (2)

• d-engine-core/src/config/raft.rs
• d-engine-core/src/state_machine_handler/snapshot_policy/log_size.rs

[codecov]

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
.../state_machine_handler/snapshot_policy/log_size.rs	90.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!