fix #398: remove lease.enabled flag — TTL always active, fix fatal crash on put_with_ttl

[JoshuaChi]

What Does This PR Do?

Removes lease.enabled config flag — TTL is now always active. Fixes a fatal crash
where put_with_ttl() with default config caused the SM worker to send FatalError,
shutting down the entire node.

Type:

• Bug Fix (with test)
• Feature (issue #___ approved)
• Documentation
• Test/Coverage
• Performance (with benchmark)

---

Why Is This Needed?

Root cause: With default config (no [raft.state_machine.lease] section),
put_with_ttl() would commit a TTL entry to the Raft log, then fail during SM apply
with StorageError::FeatureNotEnabled. The SM worker incorrectly mapped this to
RoleEvent::FatalError — a path reserved for unrecoverable I/O failures — causing
the node to shut down with no recovery path.

Fix: Remove lease.enabled entirely. TTL is always active. Overhead when no TTL
keys exist is negligible: cleanup worker costs a single atomic load (~10ns) per cycle
via the new has_lease_keys() fast path.

Why not keep enabled with a better error? A committed Raft entry cannot be
un-committed. Rejecting it at apply time violates the Raft consistency invariant (all
nodes must apply committed entries deterministically). The correct fix is to make TTL
always work, not to fail more gracefully at the wrong layer.

---

Checklist

Required:

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

If changing APIs:

• Updated relevant docs
• Explained why complexity is justified

---

Testing

How tested:

• Unit tests: lease_test.rs — updated to remove enabled field; all validation
  range tests preserved
• Integration tests: lease_integration_test.rs — removed enabled: true from all
  fixtures; added 4 new fast path tests (file + rocksdb × no-TTL-keys +
  unexpired-keys)
• Regression test: test_put_with_ttl_succeeds_with_default_config in
  embedded_client_operations.rs — confirmed FAIL on main (ConnectionTimeout due to
  node crash), PASS after fix

For bug fixes:

• Added test that fails without this fix

For performance improvements:

• lease_background_cleanup() now short-circuits in two stages:
  1. has_lease_keys() == false → single atomic load, ~10ns (most common case)
  2. may_have_expired_keys() == false → sample first 10 entries, ~30ns
  3. Full DashMap scan only when expired keys likely exist

---

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

Breaking change: LeaseConfig.enabled field is removed. Any config file with
[raft.state_machine.lease] enabled = true/false must remove that line. The field
is silently ignored by serde (unknown fields are ignored in TOML), so existing
deployments won't crash — but the field no longer has any effect.

All affected files updated: config/base/raft.toml, examples/three-nodes-standalone/,
test fixtures, bench files.

Estimated review complexity:

• Quick (< 100 lines)
• Medium (< 300 lines)
• Deep (> 300 lines)

Summary by CodeRabbit

• Configuration Changes

  • TTL key expiration is always active; the lease enabled toggle was removed
  • Lease config now only exposes cleanup interval and max cleanup duration

• Behavior Changes

  • Lease background cleanup worker is unconditionally started at runtime
  • TTL registration and enforcement occur automatically (no explicit enablement)

• Tests

  • New and updated tests cover default-config TTL behavior and fast-path cleanup scenarios

[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: d9d2fa3e-6276-45f3-a41e-55565322f96c

📥 Commits

Reviewing files that changed from the base of the PR and between db08925 and a01f963.

📒 Files selected for processing (2)

• d-engine-server/src/api/standalone.rs
• d-engine-server/tests/embedded_client/embedded_client_operations.rs

💤 Files with no reviewable changes (1)

• d-engine-server/src/api/standalone.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• d-engine-server/tests/embedded_client/embedded_client_operations.rs

---

📝 Walkthrough

Walkthrough

The PR removes the enabled flag from LeaseConfig so TTL expiration and background cleanup are always active; engines now always inject leases into state machines, state machines drop separate enablement flags, and tests/benchmarks/examples are updated to the new always-on semantics.

Changes

TTL/Lease Always Enabled

Layer / File(s)	Summary
LeaseConfig contract and validation updates d-engine-core/src/config/lease.rs, d-engine-core/src/config/lease_test.rs, d-engine-core/src/errors.rs, config/base/raft.toml	LeaseConfig removes the enabled: bool field; Default and validate() now operate unconditionally on cleanup parameters; StorageError::FeatureNotEnabled replaced with NotServing; base TOML comments updated to describe TTL always active.
Engine startup: unconditional lease injection d-engine-server/src/api/embedded.rs, d-engine-server/src/api/standalone.rs, d-engine-server/src/node/builder.rs	Embedded and standalone engines now always construct DefaultLease from config and call sm.set_lease(...); NodeBuilder always spawns the lease background cleanup worker without checking an enable flag.
FileStateMachine: lease setup and TTL handling d-engine-server/src/storage/adaptors/file/file_state_machine.rs	FileStateMachine drops lease_enabled; set_lease stores the lease; TTL registration in apply_chunk uses expect() and lease_background_cleanup gains fast-path exits for no TTL keys or none expirable.
RocksDBStateMachine: lease setup and TTL handling d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs	RocksDBStateMachine removes lease_enabled; set_lease only assigns lease; apply_chunk assumes lease presence for TTL registration; lease_background_cleanup adds early-return fast paths.
Test suite: config updates and new regression tests d-engine-server/benches/*, d-engine-server/src/storage/*_test.rs, d-engine-server/tests/*, examples/three-nodes-standalone/config/*	Benchmarks and tests drop explicit enabled: true from LeaseConfig literals; new integration tests validate cleanup fast-paths when no TTL keys exist or none are expired; embedded client tests add a zero-config helper and a regression ensuring TTL writes succeed without a lease config section.

---

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

---

Possibly related PRs

• deventlab/d-engine#176: Introduced initial lease/TTL support and lease injection lifecycle that this PR further modifies by removing optional gating and making lease always active.

---

Poem

🐰 No more locked gates, the TTL runs free,
Always cleaning, always humming with glee;
Fast paths skip when no expired keys lie,
Leases set once — no toggles to try. ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and clearly describes the main change: removing the lease.enabled flag and ensuring TTL is always active, which addresses the fatal crash issue in put_with_ttl. It is specific, concise, and accurately represents the primary objective of the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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 fix/398-put-ttl

---

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: 1

🧹 Nitpick comments (1)

d-engine-server/src/api/standalone.rs (1)

125-129: 💤 Low value

Stale comment: "Inject lease if enabled".

The comment says "if enabled" but the code now unconditionally injects the lease. Consider removing or updating this comment to match the new always-on behavior.

Suggested fix

-        // Inject lease if enabled
+        // Inject lease (TTL is always active)
         let lease = Arc::new(crate::storage::DefaultLease::new(

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@d-engine-server/src/api/standalone.rs` around lines 125 - 129, The inline
comment "Inject lease if enabled" is stale because the code now unconditionally
creates DefaultLease and calls sm.set_lease; update or remove the comment to
reflect always-on behavior (referencing DefaultLease::new and sm.set_lease) —
e.g., change to "Inject lease (always enabled)" or remove the comment entirely
so it matches the unconditional DefaultLease creation and sm.set_lease call.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@d-engine-server/tests/embedded_client/embedded_client_operations.rs`:
- Around line 597-602: The test's liveness check calls client.put(...) after the
TTL write but never performs the corresponding read; fix this by following the
existing client.put(b"probe", b"alive").await.expect(...) with a linearizable
read using client.get_linearizable(b"probe").await and assert the returned value
equals b"alive" (use .expect(...) or appropriate assertion) so the test verifies
both put and get post put_with_ttl.

---

Nitpick comments:
In `@d-engine-server/src/api/standalone.rs`:
- Around line 125-129: The inline comment "Inject lease if enabled" is stale
because the code now unconditionally creates DefaultLease and calls
sm.set_lease; update or remove the comment to reflect always-on behavior
(referencing DefaultLease::new and sm.set_lease) — e.g., change to "Inject lease
(always enabled)" or remove the comment entirely so it matches the unconditional
DefaultLease creation and sm.set_lease call.

🪄 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: 771c5d6d-8f81-43c0-b204-cfc1d6e5a1d3

📥 Commits

Reviewing files that changed from the base of the PR and between bd2df00 and db08925.

📒 Files selected for processing (21)

• config/base/raft.toml
• d-engine-core/src/config/lease.rs
• d-engine-core/src/config/lease_test.rs
• d-engine-core/src/errors.rs
• d-engine-server/benches/lease_performance.rs
• d-engine-server/benches/state_machine.rs
• d-engine-server/benches/ttl.rs
• d-engine-server/src/api/embedded.rs
• d-engine-server/src/api/standalone.rs
• d-engine-server/src/node/builder.rs
• d-engine-server/src/storage/adaptors/file/file_state_machine.rs
• d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs
• d-engine-server/src/storage/adaptors/rocksdb/rocksdb_unified_engine_test.rs
• d-engine-server/src/storage/lease_integration_test.rs
• d-engine-server/src/storage/lease_unit_test.rs
• d-engine-server/tests/consistent_reads/lease_read_embedded.rs
• d-engine-server/tests/drain_batching/select_fairness_embedded.rs
• d-engine-server/tests/embedded_client/embedded_client_operations.rs
• examples/three-nodes-standalone/config/n1.toml
• examples/three-nodes-standalone/config/n2.toml
• examples/three-nodes-standalone/config/n3.toml

💤 Files with no reviewable changes (10)

• examples/three-nodes-standalone/config/n3.toml
• d-engine-server/benches/state_machine.rs
• d-engine-core/src/errors.rs
• examples/three-nodes-standalone/config/n2.toml
• d-engine-server/benches/lease_performance.rs
• d-engine-server/src/storage/adaptors/rocksdb/rocksdb_unified_engine_test.rs
• d-engine-server/benches/ttl.rs
• d-engine-server/tests/consistent_reads/lease_read_embedded.rs
• examples/three-nodes-standalone/config/n1.toml
• d-engine-server/tests/drain_batching/select_fairness_embedded.rs

[codecov]

Codecov Report

❌ Patch coverage is 98.03922% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ngine-server/src/storage/lease_integration_test.rs	96.36%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[JoshuaChi]

@CodeRabbit review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.