Repository navigation
feat #3: Add d-lmdb-server — Docker image, HTTP API, and 3-node HA compose cluster - #4
Conversation
Pure rename, no content changes (src/, tests/, Cargo.toml, README.md -> d-lmdb/). First step of splitting into a workspace with a separate d-lmdb-server crate for the HTTP/Docker layer, keeping the library
- New root Cargo.toml: [workspace] members = [d-lmdb, d-lmdb-server] - d-lmdb-server: placeholder bin, depends on d-lmdb via path+version - .gitignore: commit Cargo.lock now that workspace has a deployable binary - d-lmdb/Cargo.toml: fix [[example]] paths (examples/ stayed at workspace root)
… path - get_linearizable/get_lease returned raw LMDB bytes (tag byte included) instead of the decoded payload — found via live smoke test - extract decode_live_value (pure) + decode_and_reap (decode + reap) so get/get_linearizable/get_lease share one decode path - add get_lease: leader-lease read, same guarantee as get_linearizable - rename get_live→fetch_local, resolve_read→decode_and_reap, ttl_filter→decode_for_scan (private helpers, avoid get_-prefix smell) - 8 new unit tests: ttl/non-ttl, boundary expiry, corrupt tag, truncated header
Needed for leader_hint passthrough in the upcoming HTTP error mapping (d-engine main doesn't have the #425 leader_hint fix yet).
- kv.rs: PUT/GET/DELETE /kv/{key}, ?level= consistency dial
(eventual/linearizable/lease), exhaustively matched enum
- error.rs: d_lmdb::Error → HTTP response per decisions/015
(400/503/500), plus a response-normalizing middleware so
extractor rejections and business errors share one JSON shape
- health.rs: /primary, /replica, /status
- main.rs: serve/healthcheck subcommands (clap, CONFIG env fallback)
- config.example.toml: shipped template, real config.toml gitignored
- d-lmdb/lib.rs: re-export ClientApiError/ErrorCode/LeaderHint,
needed for error.rs to name d-engine's error types
- cargo-chef multi-stage build, --bin d-lmdb-server only (workspace's own examples/ excluded from the final image) - docker-entrypoint.sh: root chowns bind-mounted /data to fixed 1000:1000, then drops to appuser via gosu — server process never runs as root - HEALTHCHECK start-period=10s, derived from 5 measured cold starts (worst case 863ms) rather than a guessed value - .dockerignore excludes rust-toolchain.toml — its pinned components (rustfmt/clippy) were being installed on every build for no reason, the base image is already the exact matching Rust version Verified: real docker build + docker run, HEALTHCHECK reaches "healthy", PUT/GET across all three consistency levels, and the error-response normalization middleware confirmed working against a live container.
address was the Raft peer address (e.g. node3:9081), unusable by HTTP clients. Now swaps in this node's own HTTP port.
HAProxy routes writes and strong reads to whoever passes /primary. smoke-test.sh: client only talks to the LB, verifies leader crash + re-election + writes resume, zero client-side changes.
Relative paths (./data) crashed in the container — non-root user has no write permission at that cwd. Verified fix with a real docker run.
Moved "what this solves/doesn't solve" here from d-lmdb/README.md — it's a project-level claim, not library-specific.
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughChangesThe repository is converted into a Cargo workspace containing d-lmdb storage and API
HTTP service and deployment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HAProxy
participant d-lmdb-server
participant RaftCluster
Client->>HAProxy: Send KV request
HAProxy->>d-lmdb-server: Route read or leader-required request
d-lmdb-server->>RaftCluster: Read or apply operation
RaftCluster-->>d-lmdb-server: Return value or leader response
d-lmdb-server-->>Client: Return HTTP response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
d-lmdb-server/config.example.toml (1)
1-13: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument that the flat config fields are not read directly by the library.
DLmdbConfig::from_fileonly consumes[lmdb].data_dir, then appendsraftandlmdbsubpaths from there.[cluster].db_root_dir/log_dirare used by the server config path, so this example misleadingly presents incompatible file schemas in a single TOML. Split the examples/constraints or make the server only consume fields compatible withDLmdbConfig.🤖 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-lmdb-server/config.example.toml` around lines 1 - 13, Update the configuration example to avoid presenting incompatible schemas together: document that DLmdbConfig::from_file reads only [lmdb].data_dir and derives the raft/lmdb subpaths, while [cluster].db_root_dir and log_dir belong to the server configuration path. Split the examples or clearly separate the applicable fields so users do not assume the library reads the flat cluster fields directly.
🧹 Nitpick comments (11)
d-lmdb/tests/integration.rs (1)
96-127: 📐 Maintainability & Code Quality | 🔵 TrivialGood TTL coverage; the linearizable read path has none.
db_test.rs:71-78documents thatget_linearizablepreviously returned the raw wire envelope — a bug that unit tests ondecode_live_valuealone wouldn't have caught, since the defect was in the wiring, not the decoder. A singleput→get_linearizableroundtrip here (and one forget_lease) would close that gap end to end.Happy to add them.
🤖 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-lmdb/tests/integration.rs` around lines 96 - 127, Add integration tests alongside the existing TTL tests covering end-to-end put-to-read roundtrips through get_linearizable and get_lease. Verify each method returns the stored user value rather than the raw wire envelope, using the established database setup and cleanup pattern.d-lmdb/src/db_test.rs (1)
71-140: 📐 Maintainability & Code Quality | 🔵 TrivialThorough coverage of
decode_live_value; its scan-path twin has none.
decode_for_scanreimplements the same TTL/tag logic with different failure semantics (drops corrupt entries instead of erroring) and backs all four public scan APIs, so the two can drift silently. The boundary case (expires_at == now) is the one most likely to diverge.Want me to add mirrored cases for
decode_for_scan?🤖 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-lmdb/src/db_test.rs` around lines 71 - 140, Add mirrored tests for decode_for_scan covering raw and unexpired TTL payload decoding, exact and past expiry, empty payload preservation, invalid tag bytes, empty input, and truncated TTL headers. Exercise the public scan APIs backed by decode_for_scan where appropriate, and verify the scan-specific behavior of dropping corrupt entries while preserving the expires_at == now boundary semantics.d-lmdb/src/db.rs (1)
433-442: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
unix_now_secs()is called once per scanned entry.
decode_for_scanis invoked for every candidate in the scan loop, so a large scan makes a clock call per key, and entries are evaluated against slightly different "now" values within a single scan — the boundary case can include one key and exclude its neighbour inconsistently.decode_live_valuealready takesnow_secsas a parameter; do the same here and capture it once per scan via a closure.♻️ Proposed refactor
-fn decode_for_scan(raw: &[u8]) -> Option<Vec<u8>> { +fn decode_for_scan_at( + raw: &[u8], + now_secs: u64, +) -> Option<Vec<u8>> { match Value::decode(raw) { Ok(Value::Raw(payload)) => Some(payload.to_vec()), Ok(Value::Ttl { expires_at, payload, - }) if expires_at > unix_now_secs() => Some(payload.to_vec()), + }) if expires_at > now_secs => Some(payload.to_vec()), _ => None, } }Call sites become e.g.
let now = unix_now_secs(); ... Some(move |raw: &[u8]| decode_for_scan_at(raw, now)).🤖 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-lmdb/src/db.rs` around lines 433 - 442, Refactor decode_for_scan to accept a captured now_secs value, such as via a decode_for_scan_at helper, and remove its per-entry unix_now_secs() call. In the scan loop, capture unix_now_secs() once and pass it through the existing closure used to decode candidates, ensuring every entry is evaluated against the same timestamp.d-lmdb/src/storage_engine_test.rs (1)
27-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth conformance-suite builders hand out a fixed path with a no-op
cleanup. Each builder holds oneTempDirand derives a single fixed subdirectory, so repeatedbuild()calls reopen the same LMDB environment and inherit prior state — and LMDB generally refuses a second in-process open of the same path.
d-lmdb/src/storage_engine_test.rs#L27-L35: derive a unique subdirectory perbuild()call (e.g. an incrementing counter undertemp_dir) rather than the fixedtemp_dir/raft.d-lmdb/src/state_machine_test.rs#L29-L40: apply the same per-call unique subdirectory instead of the fixedtemp_dir/lmdb_sm.🤖 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-lmdb/src/storage_engine_test.rs` around lines 27 - 35, The conformance-suite builders reuse fixed LMDB paths and do not clean them up. In d-lmdb/src/storage_engine_test.rs lines 27-35, update the builder’s build method to generate a unique subdirectory per call using an incrementing counter under temp_dir, while leaving cleanup behavior unchanged. Apply the same per-call unique-subdirectory change to the builder build method in d-lmdb/src/state_machine_test.rs lines 29-40, replacing the fixed temp_dir/lmdb_sm path.d-lmdb/src/config.rs (1)
12-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
map_size_gbholds bytes, and theDeserializederive here is unreachable.
state_machine.rspassesconfig.map_size_gbstraight intoEnvOpenOptions::map_size(...), and both constructors store bytes (newwrites10 * 1024^3,from_filemultiplies by1024^3). The_gbsuffix invites a future caller to pass10and get a 10-byte map. Separately, file loading goes throughLmdbFileSection, so theDeserializederive onDLmdbConfigis never exercised — and if it ever were,max_key_bytes/max_value_byteshave no serde defaults whileLmdbFileSectiondoes, so the two paths would disagree.♻️ Suggested rename + drop the unused derive
-#[derive(Deserialize)] pub(crate) struct DLmdbConfig { /// Root directory for all persisted data (Raft WAL + LMDB state machine) pub(crate) data_dir: PathBuf, /// LMDB memory-map size in bytes. Must be larger than the total dataset. /// Default: 10 GiB — adjust for your expected data volume. - #[serde(default = "default_map_size_gb")] - pub(crate) map_size_gb: usize, + pub(crate) map_size_bytes: usize,Update the two constructors and
state_machine.rsaccordingly.🤖 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-lmdb/src/config.rs` around lines 12 - 25, Rename DLmdbConfig.map_size_gb to a bytes-based name such as map_size_bytes, update both configuration constructors and the state_machine.rs EnvOpenOptions::map_size call to use it, and remove the unused Deserialize derive and serde default attribute from DLmdbConfig. Preserve the existing byte values and LmdbFileSection deserialization/default behavior.d-lmdb/src/state_machine_test.rs (2)
240-256: 📐 Maintainability & Code Quality | 🔵 TrivialNo coverage for
scan_prefix_bounded'saftercursor.The pagination parameter is untested, which is how the unclamped-
afterbehavior I flagged atstate_machine.rs:302-307stays invisible. A case passing anafterthat sorts before the prefix would pin the intended semantics.Happy to draft those cases if useful.
🤖 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-lmdb/src/state_machine_test.rs` around lines 240 - 256, Extend test_scan_prefix_returns_matching_keys with coverage for scan_prefix_bounded’s after cursor, including an after key that sorts before the requested user: prefix. Assert that pagination remains correctly bounded to matching prefix entries and preserves the expected results, pinning the intended clamped-after behavior.
189-202: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name promises a transformation that isn't exercised.
only_long_valuesreturns the value unchanged, so this only proves exclusion. Either assert the returned payload to lock in the "filter output is what's stored in the result" contract, or drop "and_transforms" from the name.🤖 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-lmdb/src/state_machine_test.rs` around lines 189 - 202, Update test_scan_all_with_filter_excludes_and_transforms to assert the returned entry payload as well as its key, using a filter output that demonstrates transformation or verifying the expected transformed value; otherwise rename the test to remove “and_transforms.”d-lmdb/src/state_machine.rs (2)
528-534: 🗄️ Data Integrity & Integration | 🔵 TrivialSnapshot metadata is never persisted —
snapshot_metadata()returnsNoneafter every restart.The TODO leaves
persist_last_snapshot_metadataas an in-memory-only update, so a restarted node reports no snapshot even whendata.mdbexists on disk. Depending on how d-engine uses this, the node may re-request a full snapshot from the leader.Want me to open an issue to track persisting this into
meta_db?🤖 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-lmdb/src/state_machine.rs` around lines 528 - 534, Implement durable persistence in persist_last_snapshot_metadata by serializing the supplied SnapshotMetadata and storing it in meta_db under the "snapshot_meta" key before or alongside update_last_snapshot_metadata. Ensure snapshot_metadata() reads and deserializes the same key so the metadata survives restarts, while preserving the existing EngineError propagation.
118-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated
map_err(|e| EngineError::Fatal(e.to_string())).It appears ten times in this function. A local
fn fatal<E: Display>(e: E) -> EngineError(alongside the existinglmdb_errhelper) removes the noise and makes the actual LMDB calls readable.🤖 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-lmdb/src/state_machine.rs` around lines 118 - 155, Add a local fatal error-conversion helper beside the existing lmdb_err helper, accepting any Display error and returning EngineError::Fatal with its string. Replace every repeated map_err closure in this function, including the snapshot opening, transaction, database, iteration, put, clear, and commit operations, with the helper while preserving existing error propagation.d-lmdb-server/src/http/mod.rs (2)
20-36: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider graceful shutdown and a request timeout.
axum::serve(listener, app).awaithas no.with_graceful_shutdown(...)and there's noTimeoutLayeron the stack. Ondocker stop/rolling restarts, in-flight requests can be cut off abruptly rather than allowed to finish; a slow/stalled client can also hold a connection indefinitely.♻️ Suggested addition
let listener = tokio::net::TcpListener::bind(addr).await?; - axum::serve(listener, app).await + axum::serve(listener, app) + .with_graceful_shutdown(shutdown_signal()) + .await +} + +async fn shutdown_signal() { + let _ = tokio::signal::ctrl_c().await;🤖 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-lmdb-server/src/http/mod.rs` around lines 20 - 36, Update serve to add a request TimeoutLayer to the existing router middleware stack and run axum::serve with graceful shutdown triggered by the service’s shutdown signal, preserving completion of in-flight requests during termination while bounding slow or stalled requests.
20-36: 🔒 Security & Privacy | 🔵 TrivialNo authentication on any route, including PUT/DELETE.
Every route (
/kv/{key},/status,/primary,/replica) is unauthenticated. Given the PR's stated scope as a visibility/evaluation release rather than a feature-complete product, this is likely intentional for now — but worth calling out explicitly in docs/roadmap so operators don't expose this directly to untrusted networks.🤖 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-lmdb-server/src/http/mod.rs` around lines 20 - 36, Document in the roadmap or operator-facing documentation that all routes registered by serve, including /kv/{key}, /status, /primary, and /replica, currently have no authentication and must not be exposed directly to untrusted networks; preserve the existing route behavior.
🤖 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 @.github/workflows/docker-release.yml:
- Around line 55-62: Update the workflow step identified by “Push by digest” to
stop rebuilding with docker/build-push-action. After the smoke test of
d-lmdb-server:compose, retag the validated local platform image, push that tag,
and export the digest of the pushed image for the existing manifest-creation
flow.
- Around line 83-86: Update the release workflow steps around “Strip 'v' prefix
from release tag” and the Docker build/push commands to avoid interpolating
GitHub expressions into shell source: pass github.event.release.tag_name through
the step environment, derive STRIPPED_VERSION using quoted shell-variable
expansion, and reference the derived value through a safely exported environment
variable rather than `${{ env.STRIPPED_VERSION }}` inside run blocks.
In `@compose/smoke-test.sh`:
- Around line 25-31: Update cleanup_on_fail, which is invoked by fail(), to tear
down the Compose stack with docker compose down -v after collecting the failure
logs. Ensure the teardown runs on assertion failures so containers, ports, and
persisted volumes are removed before fail() exits.
In `@d-lmdb-server/src/main.rs`:
- Line 1: Run cargo fmt --all using the project’s rustfmt configuration,
ensuring the formatting updates are applied to read_http_listen_address,
test_level_eventual, test_level_linearizable, internal_error_category,
test_internal_error_category_configuration, and the test_router route
definitions without changing behavior.
In `@d-lmdb/README.md`:
- Around line 35-38: Update the README quick-start example to call the
path-based DLmdb::open API with "./data" directly instead of passing
DLmdbConfig::new("./data"), while retaining DLmdb::open_from_file("config.toml")
only as the multi-node configuration example.
In `@d-lmdb/src/config.rs`:
- Line 51: Update the map_size_gb conversion in the configuration construction
to use checked multiplication for the GiB-to-bytes factor and fail loudly when
overflow occurs, rather than relying on unchecked arithmetic. Preserve the
existing converted byte value for valid inputs and the surrounding root.lmdb
configuration flow.
In `@d-lmdb/src/error.rs`:
- Around line 27-33: Update the KeyTooLarge and ValueTooLarge error variants to
carry the configured limit alongside the actual size, and format messages from
those values instead of hardcoded limits. Modify validate_key and validate_value
to pass max_key_bytes/max_value_bytes when constructing errors, then update all
matching patterns in the integration tests and other affected callers to handle
the expanded variants.
In `@d-lmdb/src/state_machine.rs`:
- Around line 180-188: Change the expiry-reaping flow around
db.rs::decode_and_reap so reads no longer call delete_local synchronously. Queue
expired keys to a background reaper, batch pending deletions into a single LMDB
write transaction, and retain the existing read response behavior while allowing
reaping to run independently of get/exists/get_multi and apply_chunk.
- Around line 645-648: The u64_from_bytes helper currently masks malformed
persisted metadata by defaulting to zero. Change the decoding path used by new
for last_applied_index and last_applied_term to validate the byte length and
return an error when it is not exactly 8 bytes, propagating that error from new
instead of opening with zero.
- Around line 324-333: Update the range setup in the method containing
`empty_key_guard` so an empty caller-supplied `end` is handled before
constructing `Bound::Excluded(end)` or invoking `self.kv_db.range`. Preserve the
existing empty-result behavior provided by `empty_key_guard`, and keep the
current `start` handling and non-empty range path unchanged.
- Around line 49-51: Remove the unused entry_terms field from the state machine
and delete its initialization, per-entry insertion, and reset/snapshot-clearing
logic. Update the relevant constructor and apply/reset or snapshot-restore paths
while preserving all other state-machine behavior.
- Around line 302-307: Clamp the start bound in the scan-prefix flow before
calling scan_range_core: when after sorts before prefix, use
Bound::Included(prefix); otherwise preserve the existing exclusive after cursor.
Update the logic around next_prefix and scan_range_core so results never include
keys outside the requested prefix, while retaining current limit and filter
behavior.
- Around line 588-592: Update generate_snapshot_data() to compute the checksum
from the written data.mdb contents before constructing SnapshotMetadata,
replacing the hardcoded zero digest with the repository’s established 32-byte
hashing utility and preserving the resulting digest in
SnapshotMetadata.checksum.
In `@d-lmdb/src/storage_engine.rs`:
- Around line 207-219: Update the last_index recomputation around log_db.last()
to use the persisted KEY_PURGE_BOUNDARY index when the log is empty instead of
defaulting to zero. In d-lmdb/src/storage_engine.rs:207-219, preserve the
existing tail-index path and fall back to the boundary value; in
d-lmdb/src/storage_engine.rs:242-248, delete KEY_PURGE_BOUNDARY in the same
write transaction that clears log_db so the reset last_index and persisted
boundary remain synchronized.
- Line 28: Replace the hardcoded WAL_MAP_SIZE constant with a configurable value
exposed through DLmdbConfig alongside map_size_gb, and use that setting when
creating the Raft log environment. Preserve the existing default behavior by
defining an appropriate default map size, and ensure persist_entries receives
the configured capacity instead of the fixed 128 MiB limit.
- Around line 102-123: Move the synchronous LMDB transaction and durable
wtxn.commit work in persist_entries into tokio::task::spawn_blocking, following
the existing d-lmdb pattern. Preserve empty-input handling, entry persistence,
max_index tracking, error conversion, and update last_index only after the
blocking operation completes successfully.
---
Outside diff comments:
In `@d-lmdb-server/config.example.toml`:
- Around line 1-13: Update the configuration example to avoid presenting
incompatible schemas together: document that DLmdbConfig::from_file reads only
[lmdb].data_dir and derives the raft/lmdb subpaths, while [cluster].db_root_dir
and log_dir belong to the server configuration path. Split the examples or
clearly separate the applicable fields so users do not assume the library reads
the flat cluster fields directly.
---
Nitpick comments:
In `@d-lmdb-server/src/http/mod.rs`:
- Around line 20-36: Update serve to add a request TimeoutLayer to the existing
router middleware stack and run axum::serve with graceful shutdown triggered by
the service’s shutdown signal, preserving completion of in-flight requests
during termination while bounding slow or stalled requests.
- Around line 20-36: Document in the roadmap or operator-facing documentation
that all routes registered by serve, including /kv/{key}, /status, /primary, and
/replica, currently have no authentication and must not be exposed directly to
untrusted networks; preserve the existing route behavior.
In `@d-lmdb/src/config.rs`:
- Around line 12-25: Rename DLmdbConfig.map_size_gb to a bytes-based name such
as map_size_bytes, update both configuration constructors and the
state_machine.rs EnvOpenOptions::map_size call to use it, and remove the unused
Deserialize derive and serde default attribute from DLmdbConfig. Preserve the
existing byte values and LmdbFileSection deserialization/default behavior.
In `@d-lmdb/src/db_test.rs`:
- Around line 71-140: Add mirrored tests for decode_for_scan covering raw and
unexpired TTL payload decoding, exact and past expiry, empty payload
preservation, invalid tag bytes, empty input, and truncated TTL headers.
Exercise the public scan APIs backed by decode_for_scan where appropriate, and
verify the scan-specific behavior of dropping corrupt entries while preserving
the expires_at == now boundary semantics.
In `@d-lmdb/src/db.rs`:
- Around line 433-442: Refactor decode_for_scan to accept a captured now_secs
value, such as via a decode_for_scan_at helper, and remove its per-entry
unix_now_secs() call. In the scan loop, capture unix_now_secs() once and pass it
through the existing closure used to decode candidates, ensuring every entry is
evaluated against the same timestamp.
In `@d-lmdb/src/state_machine_test.rs`:
- Around line 240-256: Extend test_scan_prefix_returns_matching_keys with
coverage for scan_prefix_bounded’s after cursor, including an after key that
sorts before the requested user: prefix. Assert that pagination remains
correctly bounded to matching prefix entries and preserves the expected results,
pinning the intended clamped-after behavior.
- Around line 189-202: Update test_scan_all_with_filter_excludes_and_transforms
to assert the returned entry payload as well as its key, using a filter output
that demonstrates transformation or verifying the expected transformed value;
otherwise rename the test to remove “and_transforms.”
In `@d-lmdb/src/state_machine.rs`:
- Around line 528-534: Implement durable persistence in
persist_last_snapshot_metadata by serializing the supplied SnapshotMetadata and
storing it in meta_db under the "snapshot_meta" key before or alongside
update_last_snapshot_metadata. Ensure snapshot_metadata() reads and deserializes
the same key so the metadata survives restarts, while preserving the existing
EngineError propagation.
- Around line 118-155: Add a local fatal error-conversion helper beside the
existing lmdb_err helper, accepting any Display error and returning
EngineError::Fatal with its string. Replace every repeated map_err closure in
this function, including the snapshot opening, transaction, database, iteration,
put, clear, and commit operations, with the helper while preserving existing
error propagation.
In `@d-lmdb/src/storage_engine_test.rs`:
- Around line 27-35: The conformance-suite builders reuse fixed LMDB paths and
do not clean them up. In d-lmdb/src/storage_engine_test.rs lines 27-35, update
the builder’s build method to generate a unique subdirectory per call using an
incrementing counter under temp_dir, while leaving cleanup behavior unchanged.
Apply the same per-call unique-subdirectory change to the builder build method
in d-lmdb/src/state_machine_test.rs lines 29-40, replacing the fixed
temp_dir/lmdb_sm path.
In `@d-lmdb/tests/integration.rs`:
- Around line 96-127: Add integration tests alongside the existing TTL tests
covering end-to-end put-to-read roundtrips through get_linearizable and
get_lease. Verify each method returns the stored user value rather than the raw
wire envelope, using the established database setup and cleanup pattern.
🪄 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 Plus
Run ID: 06246521-4587-4205-8bb2-69ab822567c5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
.dockerignore.github/workflows/docker-release.yml.gitignoreCargo.tomlDockerfileREADME.mdcompose/haproxy.cfgcompose/node1/config.tomlcompose/node2/config.tomlcompose/node3/config.tomlcompose/smoke-test.shd-lmdb-server/Cargo.tomld-lmdb-server/README.mdd-lmdb-server/config.example.tomld-lmdb-server/src/http/error.rsd-lmdb-server/src/http/error_test.rsd-lmdb-server/src/http/health.rsd-lmdb-server/src/http/kv.rsd-lmdb-server/src/http/kv_test.rsd-lmdb-server/src/http/mod.rsd-lmdb-server/src/main.rsd-lmdb/Cargo.tomld-lmdb/README.mdd-lmdb/src/config.rsd-lmdb/src/db.rsd-lmdb/src/db_test.rsd-lmdb/src/error.rsd-lmdb/src/lib.rsd-lmdb/src/state_machine.rsd-lmdb/src/state_machine_test.rsd-lmdb/src/storage_engine.rsd-lmdb/src/storage_engine_test.rsd-lmdb/src/time.rsd-lmdb/src/wire/batch_test.rsd-lmdb/src/wire/mod.rsd-lmdb/src/wire/values.rsd-lmdb/src/wire/values_test.rsd-lmdb/tests/integration.rsdocker-compose.ymldocker-entrypoint.shexamples/single-node/config.tomlsrc/db_test.rs
💤 Files with no reviewable changes (1)
- src/db_test.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
d-lmdb-server/config.example.toml (1)
1-13: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument that the flat config fields are not read directly by the library.
DLmdbConfig::from_fileonly consumes[lmdb].data_dir, then appendsraftandlmdbsubpaths from there.[cluster].db_root_dir/log_dirare used by the server config path, so this example misleadingly presents incompatible file schemas in a single TOML. Split the examples/constraints or make the server only consume fields compatible withDLmdbConfig.🤖 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-lmdb-server/config.example.toml` around lines 1 - 13, Update the configuration example to avoid presenting incompatible schemas together: document that DLmdbConfig::from_file reads only [lmdb].data_dir and derives the raft/lmdb subpaths, while [cluster].db_root_dir and log_dir belong to the server configuration path. Split the examples or clearly separate the applicable fields so users do not assume the library reads the flat cluster fields directly.
🧹 Nitpick comments (11)
d-lmdb/tests/integration.rs (1)
96-127: 📐 Maintainability & Code Quality | 🔵 TrivialGood TTL coverage; the linearizable read path has none.
db_test.rs:71-78documents thatget_linearizablepreviously returned the raw wire envelope — a bug that unit tests ondecode_live_valuealone wouldn't have caught, since the defect was in the wiring, not the decoder. A singleput→get_linearizableroundtrip here (and one forget_lease) would close that gap end to end.Happy to add them.
🤖 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-lmdb/tests/integration.rs` around lines 96 - 127, Add integration tests alongside the existing TTL tests covering end-to-end put-to-read roundtrips through get_linearizable and get_lease. Verify each method returns the stored user value rather than the raw wire envelope, using the established database setup and cleanup pattern.d-lmdb/src/db_test.rs (1)
71-140: 📐 Maintainability & Code Quality | 🔵 TrivialThorough coverage of
decode_live_value; its scan-path twin has none.
decode_for_scanreimplements the same TTL/tag logic with different failure semantics (drops corrupt entries instead of erroring) and backs all four public scan APIs, so the two can drift silently. The boundary case (expires_at == now) is the one most likely to diverge.Want me to add mirrored cases for
decode_for_scan?🤖 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-lmdb/src/db_test.rs` around lines 71 - 140, Add mirrored tests for decode_for_scan covering raw and unexpired TTL payload decoding, exact and past expiry, empty payload preservation, invalid tag bytes, empty input, and truncated TTL headers. Exercise the public scan APIs backed by decode_for_scan where appropriate, and verify the scan-specific behavior of dropping corrupt entries while preserving the expires_at == now boundary semantics.d-lmdb/src/db.rs (1)
433-442: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
unix_now_secs()is called once per scanned entry.
decode_for_scanis invoked for every candidate in the scan loop, so a large scan makes a clock call per key, and entries are evaluated against slightly different "now" values within a single scan — the boundary case can include one key and exclude its neighbour inconsistently.decode_live_valuealready takesnow_secsas a parameter; do the same here and capture it once per scan via a closure.♻️ Proposed refactor
-fn decode_for_scan(raw: &[u8]) -> Option<Vec<u8>> { +fn decode_for_scan_at( + raw: &[u8], + now_secs: u64, +) -> Option<Vec<u8>> { match Value::decode(raw) { Ok(Value::Raw(payload)) => Some(payload.to_vec()), Ok(Value::Ttl { expires_at, payload, - }) if expires_at > unix_now_secs() => Some(payload.to_vec()), + }) if expires_at > now_secs => Some(payload.to_vec()), _ => None, } }Call sites become e.g.
let now = unix_now_secs(); ... Some(move |raw: &[u8]| decode_for_scan_at(raw, now)).🤖 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-lmdb/src/db.rs` around lines 433 - 442, Refactor decode_for_scan to accept a captured now_secs value, such as via a decode_for_scan_at helper, and remove its per-entry unix_now_secs() call. In the scan loop, capture unix_now_secs() once and pass it through the existing closure used to decode candidates, ensuring every entry is evaluated against the same timestamp.d-lmdb/src/storage_engine_test.rs (1)
27-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth conformance-suite builders hand out a fixed path with a no-op
cleanup. Each builder holds oneTempDirand derives a single fixed subdirectory, so repeatedbuild()calls reopen the same LMDB environment and inherit prior state — and LMDB generally refuses a second in-process open of the same path.
d-lmdb/src/storage_engine_test.rs#L27-L35: derive a unique subdirectory perbuild()call (e.g. an incrementing counter undertemp_dir) rather than the fixedtemp_dir/raft.d-lmdb/src/state_machine_test.rs#L29-L40: apply the same per-call unique subdirectory instead of the fixedtemp_dir/lmdb_sm.🤖 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-lmdb/src/storage_engine_test.rs` around lines 27 - 35, The conformance-suite builders reuse fixed LMDB paths and do not clean them up. In d-lmdb/src/storage_engine_test.rs lines 27-35, update the builder’s build method to generate a unique subdirectory per call using an incrementing counter under temp_dir, while leaving cleanup behavior unchanged. Apply the same per-call unique-subdirectory change to the builder build method in d-lmdb/src/state_machine_test.rs lines 29-40, replacing the fixed temp_dir/lmdb_sm path.d-lmdb/src/config.rs (1)
12-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
map_size_gbholds bytes, and theDeserializederive here is unreachable.
state_machine.rspassesconfig.map_size_gbstraight intoEnvOpenOptions::map_size(...), and both constructors store bytes (newwrites10 * 1024^3,from_filemultiplies by1024^3). The_gbsuffix invites a future caller to pass10and get a 10-byte map. Separately, file loading goes throughLmdbFileSection, so theDeserializederive onDLmdbConfigis never exercised — and if it ever were,max_key_bytes/max_value_byteshave no serde defaults whileLmdbFileSectiondoes, so the two paths would disagree.♻️ Suggested rename + drop the unused derive
-#[derive(Deserialize)] pub(crate) struct DLmdbConfig { /// Root directory for all persisted data (Raft WAL + LMDB state machine) pub(crate) data_dir: PathBuf, /// LMDB memory-map size in bytes. Must be larger than the total dataset. /// Default: 10 GiB — adjust for your expected data volume. - #[serde(default = "default_map_size_gb")] - pub(crate) map_size_gb: usize, + pub(crate) map_size_bytes: usize,Update the two constructors and
state_machine.rsaccordingly.🤖 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-lmdb/src/config.rs` around lines 12 - 25, Rename DLmdbConfig.map_size_gb to a bytes-based name such as map_size_bytes, update both configuration constructors and the state_machine.rs EnvOpenOptions::map_size call to use it, and remove the unused Deserialize derive and serde default attribute from DLmdbConfig. Preserve the existing byte values and LmdbFileSection deserialization/default behavior.d-lmdb/src/state_machine_test.rs (2)
240-256: 📐 Maintainability & Code Quality | 🔵 TrivialNo coverage for
scan_prefix_bounded'saftercursor.The pagination parameter is untested, which is how the unclamped-
afterbehavior I flagged atstate_machine.rs:302-307stays invisible. A case passing anafterthat sorts before the prefix would pin the intended semantics.Happy to draft those cases if useful.
🤖 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-lmdb/src/state_machine_test.rs` around lines 240 - 256, Extend test_scan_prefix_returns_matching_keys with coverage for scan_prefix_bounded’s after cursor, including an after key that sorts before the requested user: prefix. Assert that pagination remains correctly bounded to matching prefix entries and preserves the expected results, pinning the intended clamped-after behavior.
189-202: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name promises a transformation that isn't exercised.
only_long_valuesreturns the value unchanged, so this only proves exclusion. Either assert the returned payload to lock in the "filter output is what's stored in the result" contract, or drop "and_transforms" from the name.🤖 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-lmdb/src/state_machine_test.rs` around lines 189 - 202, Update test_scan_all_with_filter_excludes_and_transforms to assert the returned entry payload as well as its key, using a filter output that demonstrates transformation or verifying the expected transformed value; otherwise rename the test to remove “and_transforms.”d-lmdb/src/state_machine.rs (2)
528-534: 🗄️ Data Integrity & Integration | 🔵 TrivialSnapshot metadata is never persisted —
snapshot_metadata()returnsNoneafter every restart.The TODO leaves
persist_last_snapshot_metadataas an in-memory-only update, so a restarted node reports no snapshot even whendata.mdbexists on disk. Depending on how d-engine uses this, the node may re-request a full snapshot from the leader.Want me to open an issue to track persisting this into
meta_db?🤖 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-lmdb/src/state_machine.rs` around lines 528 - 534, Implement durable persistence in persist_last_snapshot_metadata by serializing the supplied SnapshotMetadata and storing it in meta_db under the "snapshot_meta" key before or alongside update_last_snapshot_metadata. Ensure snapshot_metadata() reads and deserializes the same key so the metadata survives restarts, while preserving the existing EngineError propagation.
118-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated
map_err(|e| EngineError::Fatal(e.to_string())).It appears ten times in this function. A local
fn fatal<E: Display>(e: E) -> EngineError(alongside the existinglmdb_errhelper) removes the noise and makes the actual LMDB calls readable.🤖 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-lmdb/src/state_machine.rs` around lines 118 - 155, Add a local fatal error-conversion helper beside the existing lmdb_err helper, accepting any Display error and returning EngineError::Fatal with its string. Replace every repeated map_err closure in this function, including the snapshot opening, transaction, database, iteration, put, clear, and commit operations, with the helper while preserving existing error propagation.d-lmdb-server/src/http/mod.rs (2)
20-36: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider graceful shutdown and a request timeout.
axum::serve(listener, app).awaithas no.with_graceful_shutdown(...)and there's noTimeoutLayeron the stack. Ondocker stop/rolling restarts, in-flight requests can be cut off abruptly rather than allowed to finish; a slow/stalled client can also hold a connection indefinitely.♻️ Suggested addition
let listener = tokio::net::TcpListener::bind(addr).await?; - axum::serve(listener, app).await + axum::serve(listener, app) + .with_graceful_shutdown(shutdown_signal()) + .await +} + +async fn shutdown_signal() { + let _ = tokio::signal::ctrl_c().await;🤖 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-lmdb-server/src/http/mod.rs` around lines 20 - 36, Update serve to add a request TimeoutLayer to the existing router middleware stack and run axum::serve with graceful shutdown triggered by the service’s shutdown signal, preserving completion of in-flight requests during termination while bounding slow or stalled requests.
20-36: 🔒 Security & Privacy | 🔵 TrivialNo authentication on any route, including PUT/DELETE.
Every route (
/kv/{key},/status,/primary,/replica) is unauthenticated. Given the PR's stated scope as a visibility/evaluation release rather than a feature-complete product, this is likely intentional for now — but worth calling out explicitly in docs/roadmap so operators don't expose this directly to untrusted networks.🤖 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-lmdb-server/src/http/mod.rs` around lines 20 - 36, Document in the roadmap or operator-facing documentation that all routes registered by serve, including /kv/{key}, /status, /primary, and /replica, currently have no authentication and must not be exposed directly to untrusted networks; preserve the existing route behavior.
🤖 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 @.github/workflows/docker-release.yml:
- Around line 55-62: Update the workflow step identified by “Push by digest” to
stop rebuilding with docker/build-push-action. After the smoke test of
d-lmdb-server:compose, retag the validated local platform image, push that tag,
and export the digest of the pushed image for the existing manifest-creation
flow.
- Around line 83-86: Update the release workflow steps around “Strip 'v' prefix
from release tag” and the Docker build/push commands to avoid interpolating
GitHub expressions into shell source: pass github.event.release.tag_name through
the step environment, derive STRIPPED_VERSION using quoted shell-variable
expansion, and reference the derived value through a safely exported environment
variable rather than `${{ env.STRIPPED_VERSION }}` inside run blocks.
In `@compose/smoke-test.sh`:
- Around line 25-31: Update cleanup_on_fail, which is invoked by fail(), to tear
down the Compose stack with docker compose down -v after collecting the failure
logs. Ensure the teardown runs on assertion failures so containers, ports, and
persisted volumes are removed before fail() exits.
In `@d-lmdb-server/src/main.rs`:
- Line 1: Run cargo fmt --all using the project’s rustfmt configuration,
ensuring the formatting updates are applied to read_http_listen_address,
test_level_eventual, test_level_linearizable, internal_error_category,
test_internal_error_category_configuration, and the test_router route
definitions without changing behavior.
In `@d-lmdb/README.md`:
- Around line 35-38: Update the README quick-start example to call the
path-based DLmdb::open API with "./data" directly instead of passing
DLmdbConfig::new("./data"), while retaining DLmdb::open_from_file("config.toml")
only as the multi-node configuration example.
In `@d-lmdb/src/config.rs`:
- Line 51: Update the map_size_gb conversion in the configuration construction
to use checked multiplication for the GiB-to-bytes factor and fail loudly when
overflow occurs, rather than relying on unchecked arithmetic. Preserve the
existing converted byte value for valid inputs and the surrounding root.lmdb
configuration flow.
In `@d-lmdb/src/error.rs`:
- Around line 27-33: Update the KeyTooLarge and ValueTooLarge error variants to
carry the configured limit alongside the actual size, and format messages from
those values instead of hardcoded limits. Modify validate_key and validate_value
to pass max_key_bytes/max_value_bytes when constructing errors, then update all
matching patterns in the integration tests and other affected callers to handle
the expanded variants.
In `@d-lmdb/src/state_machine.rs`:
- Around line 180-188: Change the expiry-reaping flow around
db.rs::decode_and_reap so reads no longer call delete_local synchronously. Queue
expired keys to a background reaper, batch pending deletions into a single LMDB
write transaction, and retain the existing read response behavior while allowing
reaping to run independently of get/exists/get_multi and apply_chunk.
- Around line 645-648: The u64_from_bytes helper currently masks malformed
persisted metadata by defaulting to zero. Change the decoding path used by new
for last_applied_index and last_applied_term to validate the byte length and
return an error when it is not exactly 8 bytes, propagating that error from new
instead of opening with zero.
- Around line 324-333: Update the range setup in the method containing
`empty_key_guard` so an empty caller-supplied `end` is handled before
constructing `Bound::Excluded(end)` or invoking `self.kv_db.range`. Preserve the
existing empty-result behavior provided by `empty_key_guard`, and keep the
current `start` handling and non-empty range path unchanged.
- Around line 49-51: Remove the unused entry_terms field from the state machine
and delete its initialization, per-entry insertion, and reset/snapshot-clearing
logic. Update the relevant constructor and apply/reset or snapshot-restore paths
while preserving all other state-machine behavior.
- Around line 302-307: Clamp the start bound in the scan-prefix flow before
calling scan_range_core: when after sorts before prefix, use
Bound::Included(prefix); otherwise preserve the existing exclusive after cursor.
Update the logic around next_prefix and scan_range_core so results never include
keys outside the requested prefix, while retaining current limit and filter
behavior.
- Around line 588-592: Update generate_snapshot_data() to compute the checksum
from the written data.mdb contents before constructing SnapshotMetadata,
replacing the hardcoded zero digest with the repository’s established 32-byte
hashing utility and preserving the resulting digest in
SnapshotMetadata.checksum.
In `@d-lmdb/src/storage_engine.rs`:
- Around line 207-219: Update the last_index recomputation around log_db.last()
to use the persisted KEY_PURGE_BOUNDARY index when the log is empty instead of
defaulting to zero. In d-lmdb/src/storage_engine.rs:207-219, preserve the
existing tail-index path and fall back to the boundary value; in
d-lmdb/src/storage_engine.rs:242-248, delete KEY_PURGE_BOUNDARY in the same
write transaction that clears log_db so the reset last_index and persisted
boundary remain synchronized.
- Line 28: Replace the hardcoded WAL_MAP_SIZE constant with a configurable value
exposed through DLmdbConfig alongside map_size_gb, and use that setting when
creating the Raft log environment. Preserve the existing default behavior by
defining an appropriate default map size, and ensure persist_entries receives
the configured capacity instead of the fixed 128 MiB limit.
- Around line 102-123: Move the synchronous LMDB transaction and durable
wtxn.commit work in persist_entries into tokio::task::spawn_blocking, following
the existing d-lmdb pattern. Preserve empty-input handling, entry persistence,
max_index tracking, error conversion, and update last_index only after the
blocking operation completes successfully.
---
Outside diff comments:
In `@d-lmdb-server/config.example.toml`:
- Around line 1-13: Update the configuration example to avoid presenting
incompatible schemas together: document that DLmdbConfig::from_file reads only
[lmdb].data_dir and derives the raft/lmdb subpaths, while [cluster].db_root_dir
and log_dir belong to the server configuration path. Split the examples or
clearly separate the applicable fields so users do not assume the library reads
the flat cluster fields directly.
---
Nitpick comments:
In `@d-lmdb-server/src/http/mod.rs`:
- Around line 20-36: Update serve to add a request TimeoutLayer to the existing
router middleware stack and run axum::serve with graceful shutdown triggered by
the service’s shutdown signal, preserving completion of in-flight requests
during termination while bounding slow or stalled requests.
- Around line 20-36: Document in the roadmap or operator-facing documentation
that all routes registered by serve, including /kv/{key}, /status, /primary, and
/replica, currently have no authentication and must not be exposed directly to
untrusted networks; preserve the existing route behavior.
In `@d-lmdb/src/config.rs`:
- Around line 12-25: Rename DLmdbConfig.map_size_gb to a bytes-based name such
as map_size_bytes, update both configuration constructors and the
state_machine.rs EnvOpenOptions::map_size call to use it, and remove the unused
Deserialize derive and serde default attribute from DLmdbConfig. Preserve the
existing byte values and LmdbFileSection deserialization/default behavior.
In `@d-lmdb/src/db_test.rs`:
- Around line 71-140: Add mirrored tests for decode_for_scan covering raw and
unexpired TTL payload decoding, exact and past expiry, empty payload
preservation, invalid tag bytes, empty input, and truncated TTL headers.
Exercise the public scan APIs backed by decode_for_scan where appropriate, and
verify the scan-specific behavior of dropping corrupt entries while preserving
the expires_at == now boundary semantics.
In `@d-lmdb/src/db.rs`:
- Around line 433-442: Refactor decode_for_scan to accept a captured now_secs
value, such as via a decode_for_scan_at helper, and remove its per-entry
unix_now_secs() call. In the scan loop, capture unix_now_secs() once and pass it
through the existing closure used to decode candidates, ensuring every entry is
evaluated against the same timestamp.
In `@d-lmdb/src/state_machine_test.rs`:
- Around line 240-256: Extend test_scan_prefix_returns_matching_keys with
coverage for scan_prefix_bounded’s after cursor, including an after key that
sorts before the requested user: prefix. Assert that pagination remains
correctly bounded to matching prefix entries and preserves the expected results,
pinning the intended clamped-after behavior.
- Around line 189-202: Update test_scan_all_with_filter_excludes_and_transforms
to assert the returned entry payload as well as its key, using a filter output
that demonstrates transformation or verifying the expected transformed value;
otherwise rename the test to remove “and_transforms.”
In `@d-lmdb/src/state_machine.rs`:
- Around line 528-534: Implement durable persistence in
persist_last_snapshot_metadata by serializing the supplied SnapshotMetadata and
storing it in meta_db under the "snapshot_meta" key before or alongside
update_last_snapshot_metadata. Ensure snapshot_metadata() reads and deserializes
the same key so the metadata survives restarts, while preserving the existing
EngineError propagation.
- Around line 118-155: Add a local fatal error-conversion helper beside the
existing lmdb_err helper, accepting any Display error and returning
EngineError::Fatal with its string. Replace every repeated map_err closure in
this function, including the snapshot opening, transaction, database, iteration,
put, clear, and commit operations, with the helper while preserving existing
error propagation.
In `@d-lmdb/src/storage_engine_test.rs`:
- Around line 27-35: The conformance-suite builders reuse fixed LMDB paths and
do not clean them up. In d-lmdb/src/storage_engine_test.rs lines 27-35, update
the builder’s build method to generate a unique subdirectory per call using an
incrementing counter under temp_dir, while leaving cleanup behavior unchanged.
Apply the same per-call unique-subdirectory change to the builder build method
in d-lmdb/src/state_machine_test.rs lines 29-40, replacing the fixed
temp_dir/lmdb_sm path.
In `@d-lmdb/tests/integration.rs`:
- Around line 96-127: Add integration tests alongside the existing TTL tests
covering end-to-end put-to-read roundtrips through get_linearizable and
get_lease. Verify each method returns the stored user value rather than the raw
wire envelope, using the established database setup and cleanup pattern.
🪄 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 Plus
Run ID: 06246521-4587-4205-8bb2-69ab822567c5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
.dockerignore.github/workflows/docker-release.yml.gitignoreCargo.tomlDockerfileREADME.mdcompose/haproxy.cfgcompose/node1/config.tomlcompose/node2/config.tomlcompose/node3/config.tomlcompose/smoke-test.shd-lmdb-server/Cargo.tomld-lmdb-server/README.mdd-lmdb-server/config.example.tomld-lmdb-server/src/http/error.rsd-lmdb-server/src/http/error_test.rsd-lmdb-server/src/http/health.rsd-lmdb-server/src/http/kv.rsd-lmdb-server/src/http/kv_test.rsd-lmdb-server/src/http/mod.rsd-lmdb-server/src/main.rsd-lmdb/Cargo.tomld-lmdb/README.mdd-lmdb/src/config.rsd-lmdb/src/db.rsd-lmdb/src/db_test.rsd-lmdb/src/error.rsd-lmdb/src/lib.rsd-lmdb/src/state_machine.rsd-lmdb/src/state_machine_test.rsd-lmdb/src/storage_engine.rsd-lmdb/src/storage_engine_test.rsd-lmdb/src/time.rsd-lmdb/src/wire/batch_test.rsd-lmdb/src/wire/mod.rsd-lmdb/src/wire/values.rsd-lmdb/src/wire/values_test.rsd-lmdb/tests/integration.rsdocker-compose.ymldocker-entrypoint.shexamples/single-node/config.tomlsrc/db_test.rs
💤 Files with no reviewable changes (1)
- src/db_test.rs
🛑 Comments failed to post (11)
d-lmdb/src/config.rs (1)
51-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unchecked GiB→bytes multiplication.
A mistyped
map_size_gbwraps in release builds and panics in debug; on a 32-bit target anything ≥ 4 overflows. Fail loudly instead.🛡️ Proposed fix
- map_size_gb: root.lmdb.map_size_gb * 1024 * 1024 * 1024, + map_size_gb: root + .lmdb + .map_size_gb + .checked_mul(1024 * 1024 * 1024) + .ok_or_else(|| { + Error::Storage("lmdb.map_size_gb is too large for this platform".into()) + })?,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.map_size_gb: root .lmdb .map_size_gb .checked_mul(1024 * 1024 * 1024) .ok_or_else(|| { Error::Storage("lmdb.map_size_gb is too large for this platform".into()) })?,🤖 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-lmdb/src/config.rs` at line 51, Update the map_size_gb conversion in the configuration construction to use checked multiplication for the GiB-to-bytes factor and fail loudly when overflow occurs, rather than relying on unchecked arithmetic. Preserve the existing converted byte value for valid inputs and the surrounding root.lmdb configuration flow.d-lmdb/src/error.rs (1)
27-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Error text hardcodes limits that are configurable.
max_key_bytes/max_value_bytescome fromDLmdbConfig, but these messages always claim 512 / 1MB.d-lmdb-server/src/http/error.rs:114-118returnserr.to_string()verbatim as the 400 body, so an operator who raises the limit ships a misleading message to clients. Carry the limit in the variant.🐛 Proposed fix
- /// The supplied key exceeds the 512-byte limit. - #[error("key too large: {0} bytes (max 512)")] - KeyTooLarge(usize), + /// The supplied key exceeds the configured limit. + #[error("key too large: {actual} bytes (max {max})")] + KeyTooLarge { actual: usize, max: usize }, - /// The supplied value exceeds the 1MB limit. - #[error("value too large: {0} bytes (max 1MB)")] - ValueTooLarge(usize), + /// The supplied value exceeds the configured limit. + #[error("value too large: {actual} bytes (max {max})")] + ValueTooLarge { actual: usize, max: usize },Note this touches
validate_key/validate_valueindb.rsand the matches ind-lmdb/tests/integration.rs:71-92.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./// The supplied key exceeds the configured limit. #[error("key too large: {actual} bytes (max {max})")] KeyTooLarge { actual: usize, max: usize }, /// The supplied value exceeds the configured limit. #[error("value too large: {actual} bytes (max {max})")] ValueTooLarge { actual: usize, max: usize },🤖 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-lmdb/src/error.rs` around lines 27 - 33, Update the KeyTooLarge and ValueTooLarge error variants to carry the configured limit alongside the actual size, and format messages from those values instead of hardcoded limits. Modify validate_key and validate_value to pass max_key_bytes/max_value_bytes when constructing errors, then update all matching patterns in the integration tests and other affected callers to handle the expanded variants.d-lmdb/src/state_machine.rs (6)
49-51: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Any reader of entry_terms outside the insert/clear sites? rg -nP -C3 '\bentry_terms\b' --type=rustRepository: deventlab/d-lmdb
Length of output: 154
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Files:" git ls-files | rg 'd-lmdb/src/state_machine.rs|Cargo\.toml$|Cargo\.lock$' || true echo echo "state_machine.rs outline if available:" ast-grep outline d-lmdb/src/state_machine.rs --view expanded || true echo echo "Occurrences of entry_terms with line numbers:" rg -n -C4 '\bentry_terms\b' . || true echo echo "Relevant state_machine.rs sections:" wc -l d-lmdb/src/state_machine.rs sed -n '1,120p' d-lmdb/src/state_machine.rs sed -n '430,470p' d-lmdb/src/state_machine.rsRepository: deventlab/d-lmdb
Length of output: 11118
Drop the unused
entry_termscache.
entry_termsis initialized, populated on each applied entry, and only cleared byresetor snapshot restore; nothing reads it. Keep it until a real reader requires term lookups, otherwise remove both the field and the insert to avoid unbounded per-entry in-memory growth.🤖 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-lmdb/src/state_machine.rs` around lines 49 - 51, Remove the unused entry_terms field from the state machine and delete its initialization, per-entry insertion, and reset/snapshot-clearing logic. Update the relevant constructor and apply/reset or snapshot-restore paths while preserving all other state-machine behavior.
180-188: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Expiry reaping takes the LMDB write lock on the read path.
delete_localopens a write transaction, and LMDB permits exactly one writer at a time. It's called synchronously fromdb.rs::decode_and_reap, which servesget/exists/get_multi— and those run directly on a tokio worker ind-lmdb-server/src/http/kv.rs:66-80with nospawn_blocking. A read burst over expired keys therefore serializes reads against each other and against the Raft apply loop'sapply_chunkwrite txn, with an fsync per key.Consider queuing expired keys to a background reaper that batches deletions into one txn, rather than reaping inline per read.
🤖 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-lmdb/src/state_machine.rs` around lines 180 - 188, Change the expiry-reaping flow around db.rs::decode_and_reap so reads no longer call delete_local synchronously. Queue expired keys to a background reaper, batch pending deletions into a single LMDB write transaction, and retain the existing read response behavior while allowing reaping to run independently of get/exists/get_multi and apply_chunk.
302-307: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
afterisn't clamped to the prefix range.If a caller passes an
aftercursor that sorts beforeprefix, the scan starts there and returns every key in[after, next_prefix(prefix))— including keys that don't share the prefix.afteris caller-supplied through the publicDLmdb::scan_prefix, so this is reachable with a stale or wrong cursor.🐛 Proposed fix
let end = next_prefix(prefix); let start = match after { - Some(a) => Bound::Excluded(a), + // Clamp the cursor into the prefix range; a cursor before the + // prefix must not widen the scan. + Some(a) if a >= prefix => Bound::Excluded(a), + Some(_) => Bound::Included(prefix), None => Bound::Included(prefix), };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let end = next_prefix(prefix); let start = match after { // Clamp the cursor into the prefix range; a cursor before the // prefix must not widen the scan. Some(a) if a >= prefix => Bound::Excluded(a), Some(_) => Bound::Included(prefix), None => Bound::Included(prefix), }; self.scan_range_core(start, end.as_deref(), limit, filter)🤖 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-lmdb/src/state_machine.rs` around lines 302 - 307, Clamp the start bound in the scan-prefix flow before calling scan_range_core: when after sorts before prefix, use Bound::Included(prefix); otherwise preserve the existing exclusive after cursor. Update the logic around next_prefix and scan_range_core so results never include keys outside the requested prefix, while retaining current limit and filter behavior.
324-333: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Empty
endis unguarded here, unlikestart.
empty_key_guard's doc says every caller-supplied bound handed to LMDB must be checked, but onlystartis.endgoes straight intoBound::Excluded(end)— a zero-lengthendhits the sameMDB_BAD_VALSIZEthis guard exists to avoid. Note the forward path is safe becausescan_range_corecomparesendin Rust rather than passing it to heed; this is the one place it reaches LMDB.🐛 Proposed fix
- if start.is_empty() { + if start.is_empty() || end.is_empty() { return Ok(self.empty_key_guard()); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if start.is_empty() || end.is_empty() { return Ok(self.empty_key_guard()); } let rtxn = self.env.read_txn().map_err(lmdb_err)?; let revision = self.last_applied_index.load(Ordering::SeqCst); let cap = limit.unwrap_or(usize::MAX); let range = (Bound::Included(start), Bound::Excluded(end)); let iter = self.kv_db.range(&rtxn, &range).map_err(lmdb_err)?;🤖 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-lmdb/src/state_machine.rs` around lines 324 - 333, Update the range setup in the method containing `empty_key_guard` so an empty caller-supplied `end` is handled before constructing `Bound::Excluded(end)` or invoking `self.kv_db.range`. Preserve the existing empty-result behavior provided by `empty_key_guard`, and keep the current `start` handling and non-empty range path unchanged.
588-592: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Does d-engine validate SnapshotMetadata.checksum anywhere? rg -nP -C4 '\bchecksum\b' --type=rust -g '!target/**'Repository: deventlab/d-lmdb
Length of output: 154
🏁 Script executed:
#!/bin/bash set -u echo "== files ==" git ls-files | sed -n '1,200p' echo "== candidates for state_machine.rs and SnapshotMetadata ==" fd -a 'state_machine\.rs$|snapshot' . | sed 's#^\./##' | sed -n '1,120p' echo "== checksum occurrences all files ==" rg -n -C3 '\bchecksum\b' -g '!target/**' || true echo "== SnapshotMetadata occurrences ==" rg -n -C3 '\bSnapshotMetadata\b' -g '!target/**' || trueRepository: deventlab/d-lmdb
Length of output: 1943
🏁 Script executed:
#!/bin/bash set -u echo "== state_machine.rs outline around SnapshotMetadata / checksum ==" ast-grep outline src/state_machine.rs --match 'SnapshotMetadata' --view expanded || true sed -n '540,620p' src/state_machine.rs echo "== all identifier references containing checksum/check ==" rg -n -C2 '[Cc]hecksum|[Cc]heck|digest|sha|md5|blake|snap_meta|SnapshotMetadata' d-lmdb/src d-lmdb-server/src Cargo.toml README.md || trueRepository: deventlab/d-lmdb
Length of output: 9414
🏁 Script executed:
#!/bin/bash set -u echo "== d-engine dependency sources ==" rg -n 'd-engine|d_engine|snapshot|Checksum|checksum|Checksum' Cargo.toml Cargo.lock README.md d-lmdb-server/src d-lmdb/src || true echo "== focused state_machine section ==" sed -n '528,598p' d-lmdb/src/state_machine.rs echo "== focused d-lmdb-server snapshot handling/search ==" rg -n -C3 'Snapshot|snapshot|Snap|checksum|Checksum|install|install_snapshot|verify' d-lmdb-server/src Cargo.toml Cargo.lock || trueRepository: deventlab/d-lmdb
Length of output: 50373
🌐 Web query:
d-engine server_storage SnapshotMetadata checksum💡 Result:
In the d-engine Raft storage system, the
SnapshotMetadatastructure—defined within thed_engine_proto::server::storagemodule—includes achecksumfield used for integrity verification of snapshots [1]. Thechecksumfield is typically represented as aBytestype and stores a CRC32 or SHA2 hash [1]. This value is essential for ensuring that the snapshot data remains uncorrupted during storage or transmission between cluster nodes [1]. The metadata management lifecycle in d-engine involves three primary methods within theStateMachinetrait [1]: 1.update_last_snapshot_metadata: Updates theSnapshotMetadatain memory [1][2][3]. 2.snapshot_metadata: Retrieves the currentSnapshotMetadatafrom memory [1][2][3]. 3.persist_last_snapshot_metadata: Writes theSnapshotMetadatato durable storage, a process that typically involves disk I/O [1][2][3]. By tracking thechecksumalongside other metadata—such as thelast_included_index,last_included_term, andsnapshot_size—the system can validate the integrity of snapshot files before they are applied to the state machine [1].Citations:
- 1: https://deepwiki.com/deventlab/d-engine/6.5-snapshots-and-log-compaction
- 2: https://docs.rs/d-engine/latest/d_engine/struct.FileStateMachine.html
- 3: https://docs.rs/d-engine/latest/d_engine/docs/server_guide/customize_state_machine/index.html
🌐 Web query:
github.com/DEventLab/d-engine SnapshotMetadata checksum verify SnapshotMetadata checksum💡 Result:
In the d-engine project (deventlab/d-engine), SnapshotMetadata is a data structure defined in the d-engine-proto crate used to track Raft log position and integrity information for snapshots [1]. The integrity verification process relies on a checksum field within the SnapshotMetadata [1]. According to the project's documentation, the verification process for a snapshot generally includes the following steps: 1. Metadata Validation: The snapshot's integrity is verified using the checksum stored in the SnapshotMetadata before the snapshot is applied to the state machine [1]. 2. Checksum Algorithm: The system utilizes CRC32 or SHA2 hash algorithms to calculate and verify the checksum of the snapshot data [1]. 3. Verification Flow: - The snapshot metadata and its version are validated first [1]. - The compressed file format (typically a gzip tar archive) is verified [1]. - The snapshot is decompressed into a temporary directory [1]. - The checksum of the decompressed data is calculated and compared against the checksum stored in the SnapshotMetadata to ensure integrity [1]. This mechanism ensures that the snapshot has not been corrupted during storage or transfer before it is used to initialize or update the state machine database [1]. Any failure in checksum verification or file validation prevents the application of the snapshot, protecting the system from inconsistent state [1].
Citations:
Compute a real snapshot checksum for
data.mdb.
generate_snapshot_data()currently returns a hardcoded 32 zero-byte digest for every snapshot. SinceSnapshotMetadata.checksumis part of d-engine’s snapshot integrity contract, these snapshots can pass snapshot installs despite corruption; compute a real digest over the writtendata.mdbbefore returning it.🤖 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-lmdb/src/state_machine.rs` around lines 588 - 592, Update generate_snapshot_data() to compute the checksum from the written data.mdb contents before constructing SnapshotMetadata, replacing the hardcoded zero digest with the repository’s established 32-byte hashing utility and preserving the resulting digest in SnapshotMetadata.checksum.
645-648: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
u64_from_bytessilently returns 0 on a malformed value.This decodes the persisted
last_applied_index/last_applied_termat open time (lines 77-79). A truncated or corrupt meta entry quietly becomes "index 0", i.e. the node claims it has applied nothing and replays from the start rather than failing loudly. Prefer returning an error fromnewon a bad length.🤖 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-lmdb/src/state_machine.rs` around lines 645 - 648, The u64_from_bytes helper currently masks malformed persisted metadata by defaulting to zero. Change the decoding path used by new for last_applied_index and last_applied_term to validate the byte length and return an error when it is not exactly 8 bytes, propagating that error from new instead of opening with zero.d-lmdb/src/storage_engine.rs (3)
28-28: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Raft WAL map size is a hardcoded 128 MiB with no config knob.
The state-machine env takes its map size from
DLmdbConfig(10 GiB default), but the log env is fixed. The log grows until a snapshot triggerspurge; a node that lags, or a write burst of ~1 MiB values before the first snapshot, exhausts 128 MiB and everypersist_entriesstarts failing withMDB_MAP_FULL— which surfaces as an opaqueDbError. Expose this alongsidemap_size_gb.🤖 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-lmdb/src/storage_engine.rs` at line 28, Replace the hardcoded WAL_MAP_SIZE constant with a configurable value exposed through DLmdbConfig alongside map_size_gb, and use that setting when creating the Raft log environment. Preserve the existing default behavior by defining an appropriate default map size, and ensure persist_entries receives the configured capacity instead of the fixed 128 MiB limit.
102-123: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # How does d-engine invoke LogStore::persist_entries — blocking pool or runtime worker? rg -nP -C5 'persist_entries' --type=rust -g '!target/**'Repository: deventlab/d-lmdb
Length of output: 154
Run the durable LMDB commit off the async runtime.
persist_entriesisasyncbut performs a synchronous write txn followed bywtxn.commit()withis_write_durable()returningtrue, so the LMDB flush/fsync work runs on the Tokio worker used byd-engine. This is on the Raft append/replication path and can stall the runtime under load;d-lmdbalready usesspawn_blockingelsewhere for blocking disk work, so apply the same pattern here.🤖 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-lmdb/src/storage_engine.rs` around lines 102 - 123, Move the synchronous LMDB transaction and durable wtxn.commit work in persist_entries into tokio::task::spawn_blocking, following the existing d-lmdb pattern. Preserve empty-input handling, entry persistence, max_index tracking, error conversion, and update last_index only after the blocking operation completes successfully.
207-219: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
last_indexand the persisted purge boundary are maintained independently, so log-emptying operations desynchronize them. Both sites assume an emptylog_dbmeans "index 0", but after apurgethe true last index lives only inKEY_PURGE_BOUNDARY.
d-lmdb/src/storage_engine.rs#L207-L219: whenlog_db.last()returnsNone, fall back to the persisted purge boundary's index instead of0.d-lmdb/src/storage_engine.rs#L242-L248: deleteKEY_PURGE_BOUNDARYin the same write txn that clearslog_db, so the reset-to-zerolast_indexand the boundary agree.📍 Affects 1 file
d-lmdb/src/storage_engine.rs#L207-L219(this comment)d-lmdb/src/storage_engine.rs#L242-L248🤖 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-lmdb/src/storage_engine.rs` around lines 207 - 219, Update the last_index recomputation around log_db.last() to use the persisted KEY_PURGE_BOUNDARY index when the log is empty instead of defaulting to zero. In d-lmdb/src/storage_engine.rs:207-219, preserve the existing tail-index path and fall back to the boundary value; in d-lmdb/src/storage_engine.rs:242-248, delete KEY_PURGE_BOUNDARY in the same write transaction that clears log_db so the reset last_index and persisted boundary remain synchronized.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- kv_test: add handler integration tests (PUT/GET/DELETE via real DLmdb) - d-lmdb/README: fix API example (DLmdbConfig is pub(crate), won't compile) - d-lmdb-server/README: reference published Docker image in Quick Start - ci: push tested image (no rebuild); fix shell injection in release tag - smoke-test: tear down compose on failure - Dockerfile: apt-get upgrade to patch base image CVEs
2 participants
What Does This PR Do?
Restructures d-lmdb into a Cargo workspace, adds
d-lmdb-server(HTTP+JSON API wrapping d-lmdb), and ships it as a multi-arch Docker image with a docker-compose 3-node HA cluster and automated smoke test. A visibility release — d-lmdb stays experimental, but now anyone can try it without writing Rust.Type:
Why Is This Needed?
Closes #3. d-lmdb is an embedded library, which excludes developers who don't write Rust. This PR adds a standalone service (
d-lmdb-server) so anyone can evaluate d-lmdb's distributed LMDB semantics over plain HTTP — justdocker compose up.Checklist
Required:
make check(fmt + clippy + deny) passesmake testpasseserror_test.rs(292 lines, full error-to-JSON mapping),kv_test.rs(34 lines),db_test.rs(140 lines)If changing APIs:
README.md(library vs service split),d-lmdb-server/README.md(API reference, config guide, HA setup)Testing
How tested:
compose/smoke-test.sh— brings up 3 nodes + HAProxy, runs PUT/GET/DELETE through the LB, kills the leader container, confirms the cluster recovers and the LB re-routes with zero client-side changesdocker compose upverified on macOS (arm64)Does This Fit d-lmdb's Scope?
d-lmdblibrary API is unchanged;d-lmdb-serveris a separate crateReviewer Notes
d-lmdb/Cargo.tomlpins d-engine tofix/425-leader-hintbranch. This is temporary — needs to switch to a released version once that fix lands upstream.d-lmdb-server/src/http/error.rsfollows the classification ind-engine-product-design/decisions/015-http-error-classification.md. Every non-2xx response is JSON, never plain text — even extractor-level rejections are normalized.Estimated review complexity:
Summary by CodeRabbit
New Features
Documentation
Tests