fix #415, #418: embedded API gaps and snapshot last_included mismatch#421
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds batch write support across proto, core, client, server, and storage layers, while refactoring snapshot/log purge scheduling and updating config/startup APIs. It also updates release notes, migration docs, and several tests to match the new command encoding and snapshot behavior. ChangesBatched write command feature
Estimated code review effort: 4 (Complex) | ~75 minutes Snapshot label fix and log purge scheduling refactor
Estimated code review effort: 4 (Complex) | ~70 minutes Config validation and startup API updates
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
d-engine-server/src/api/embedded_client.rs (1)
613-658: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
batch()doesn't validate emptyopsas its own docs promise.The doc comment states the call "Returns an error if
opsis empty," but the implementation never checks for that and will happily propose an empty batch through the full Raft write path.🐛 Proposed fix
async fn batch( &self, ops: Vec<BatchOp>, ) -> ClientApiResult<()> { + if ops.is_empty() { + return Err(ClientApiError::Business { + code: ErrorCode::InvalidRequest, + message: "batch ops must not be empty".to_string(), + required_action: None, + }); + } let request = ClientWriteRequest { client_id: self.client_id, command: Some(proto_convert::write_op_to_bytes(WriteOperation::Batch { ops, })), };🤖 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/embedded_client.rs` around lines 613 - 658, The embedded_client::batch method currently allows an empty ops vector to proceed even though its documentation says it must error; add an early validation at the start of batch() to reject empty ops before building ClientWriteRequest or sending the proposal. Return the appropriate ClientApiResult error consistently with the existing error helpers used in batch(), so the behavior matches the doc comment and avoids proposing an empty WriteOperation::Batch.d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (1)
645-681: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
read_prev_valuesoverlay isn't updated forCommand::Batchkeys.Insert/Delete/CAS entries update
overlayso a later entry writing the same key within the same chunk gets the correct in-chunk prior value.Command::Batchskips this entirely (Command::Batch { ops: _ } => None), so if a batch entry writes keykand a later entry in the same apply_chunk call also writesk, the later entry'sprev_valuewill incorrectly reflect the pre-chunk state instead of the batch's write.♻️ Suggested fix: update overlay per BatchOp key
Command::Noop => None, - Command::Batch { ops: _ } => None, + Command::Batch { ops } => { + for op in ops { + match op { + BatchOp::Insert { key, value } => { + overlay.insert(key.clone(), Some(value.clone())); + } + BatchOp::Delete { key } => { + overlay.insert(key.clone(), None); + } + } + } + None + }🤖 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-core/src/state_machine_handler/default_state_machine_handler.rs` around lines 645 - 681, The `read_prev_values` helper is not tracking in-chunk writes for `Command::Batch`, so later entries in the same chunk can see stale prior values. Update the `Command::Batch` branch in `read_prev_values` to walk the batch’s ops and apply the same `overlay` bookkeeping used for `Insert`/`Delete` (and any key-writing batch op), so subsequent entries resolve the correct in-chunk previous value. Keep the fix localized to `read_prev_values` and use the batch operation keys to update the overlay consistently.d-engine-server/src/api/embedded.rs (1)
267-305: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
start_withshould reuse the config it already built.start_custom(..., Some(path_str))rebuildsRaftNodeConfigfromdefault()and drops anyCONFIG_PATH/RAFT__overrides that were present whenconfigwas first created, so the storage/lease setup and the running node can diverge. Pass the validatedconfigthrough tostart_nodeinstead of reloading 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-engine-server/src/api/embedded.rs` around lines 267 - 305, The start_with flow rebuilds configuration again inside start_custom/start_node instead of reusing the validated config already created in start_with, which can drop CONFIG_PATH and RAFT__ overrides. Update start_with to pass the existing config through to start_node (via start_custom or a new overload) and use that same RaftNodeConfig for the running node, storage, and lease setup; reference start_with, start_custom, and start_node when wiring the fix.
🧹 Nitpick comments (10)
d-engine-proto/src/exts/client_ext.rs (1)
131-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove empty
impl Batch {}block.Dead code — no methods defined. Serves no purpose unless a follow-up is planned.
♻️ Proposed removal
-impl Batch {} -🤖 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-proto/src/exts/client_ext.rs` around lines 131 - 132, Remove the empty impl Batch block from the client_ext module; it defines no methods and should be deleted unless a concrete Batch method is being added in the same change. Locate the standalone impl Batch {} near the Batch type in src/exts/client_ext.rs and remove it without affecting any surrounding implementations or imports.d-engine-core/src/client/types.rs (1)
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the expected byte encoding.
commandis now opaqueBytes; a brief doc comment noting it must be a serializedWriteCommand(as produced bywrite_op_to_bytes) would help API consumers avoid passing arbitrary bytes.🤖 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-core/src/client/types.rs` around lines 30 - 33, The ClientWriteRequest::command field is now opaque Bytes, so add a brief doc comment on ClientWriteRequest and/or command explaining that it must contain a serialized WriteCommand produced by write_op_to_bytes. Keep the guidance close to the ClientWriteRequest type definition so API consumers know not to pass arbitrary bytes.d-engine-server/src/api/types.rs (1)
92-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
BatchOp → ProtoBatchOpmapping vs.grpc_client.rs.This match arm is duplicated almost verbatim in
d-engine-client/src/grpc_client.rs(GrpcClient::batch), which buildsProtoBatchOps directly instead of going throughWriteOperation. That contradicts this file's own doc comment claiming to be the "single coupling point" for the write path — a newBatchOpvariant now requires updating both files.Consider extracting a shared
impl From<BatchOp> for ProtoBatchOp(or aVec<BatchOp> -> Vec<ProtoBatchOp>helper) that bothtypes.rsandgrpc_client.rscan call.🤖 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/types.rs` around lines 92 - 111, The BatchOp to ProtoBatchOp conversion is duplicated between the WriteOperation::Batch mapping in types.rs and GrpcClient::batch in grpc_client.rs, which breaks the intended single coupling point. Extract the conversion into a shared helper or an impl From<BatchOp> for ProtoBatchOp (and optionally a Vec mapping helper), then update both WriteOperation::Batch and GrpcClient::batch to use that shared path so any new BatchOp variant only needs one update.d-engine-server/benches/state_machine.rs (1)
360-390: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared watch-broadcast logic across the 3 benchmarks.
The
Command -> WatchResponsetranslation (including the newCommand::Batcharm) is now duplicated identically acrossbench_apply_with_1_watcher,bench_apply_with_10_watchers, andbench_apply_with_100_watchers. Extracting a shared helper function would reduce ~90 lines of duplicated logic and ease future maintenance whenCommandvariants change again.Also applies to: 451-481, 542-572
🤖 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/benches/state_machine.rs` around lines 360 - 390, The Command-to-WatchResponse translation logic is duplicated across bench_apply_with_1_watcher, bench_apply_with_10_watchers, and bench_apply_with_100_watchers, including the new Command::Batch handling. Extract the shared broadcast mapping into a helper near the existing watch-send logic, and have each benchmark call that helper for Command values. Make sure the helper covers all current Command variants and the BatchOp::Insert/Delete cases so future Command changes only need to be updated in one place.d-engine-core/src/raft_role/leader_state.rs (1)
1767-1796: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDebug
println!left in shared purge-scheduling path.
handle_snapshot_createddelegates toschedule_and_execute_purge(inrole_state.rs), whose implementation containsprintln!("purge_upto_index={purge_upto_index}");. This fires unconditionally on every successful snapshot across Leader/Follower/Learner and bypasses thetracingframework used elsewhere in this codebase.Since
role_state.rsisn't in the current review file set, please confirm this is intentional or remove/replace withtrace!/debug!before merge.🤖 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-core/src/raft_role/leader_state.rs` around lines 1767 - 1796, The shared purge-scheduling path still contains an unconditional debug println, which should not remain in production code. Review the call from handle_snapshot_created into schedule_and_execute_purge and remove the println!("purge_upto_index=...") or replace it with the existing tracing macros (trace!/debug!) so it follows the rest of the codebase’s logging conventions. Ensure the change is applied in schedule_and_execute_purge since it affects Leader, Follower, and Learner snapshot handling.d-engine-core/src/raft_role/learner_state.rs (1)
613-624: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame stale comment as
FollowerState::handle_log_purge_completed.The comment describes these as "Leader-only operations" that are "silently ignore[d]", but the body now actively updates
last_purged_index, and this handler is also exercised for the learner's own locally-scheduled purges (seetest_learner_resets_snapshot_flag_on_success). Recommend updating the comment to match current behavior.🤖 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-core/src/raft_role/learner_state.rs` around lines 613 - 624, The stale comment on learner log purge handling no longer matches the behavior in handle_log_purge_completed: it now updates last_purged_index, and this path also handles the learner’s own locally scheduled purges. Update the comment in LearnerState::handle_log_purge_completed to describe the current behavior instead of calling these events Leader-only operations that are silently ignored, and keep it aligned with the analogous comment in FollowerState::handle_log_purge_completed.d-engine-core/src/raft_role/follower_state.rs (1)
515-526: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment no longer matches behavior.
The comment above
handle_log_purge_completedstill says the follower "silently ignores" these as "Leader-only operations" that arrive after a step-down race. But the body now actively mutateslast_purged_indexon every call, and this handler is also reached for the follower's own locally-scheduled purges (viaschedule_and_execute_purgeinhandle_snapshot_created), not just stale leader-originated events. Worth updating the comment to reflect the dual purpose (own purge completions + stale leftover events from a former role) and that it no longer silently ignores.🤖 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-core/src/raft_role/follower_state.rs` around lines 515 - 526, The comment above handle_log_purge_completed is outdated and no longer matches the implementation. Update it to describe that this method handles both the follower’s own purge completions (scheduled via schedule_and_execute_purge from handle_snapshot_created) and any stale in-flight events from a prior leader role, and that it updates last_purged_index instead of silently ignoring the event. Keep the wording aligned with the current behavior in follower_state::handle_log_purge_completed.d-engine-core/src/raft_role/role_state.rs (1)
748-748: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove leftover debug
println!.
println!("purge_upto_index={purge_upto_index}");is a debug artifact in a hot async path; it bypasses the crate'stracinginstrumentation used everywhere else in this file (e.g.,debug!,error!) and pollutes stdout in production.♻️ Suggested fix
- println!("purge_upto_index={purge_upto_index}"); + trace!(purge_upto_index, "Computed purge boundary candidate");🤖 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-core/src/raft_role/role_state.rs` at line 748, Remove the leftover debug println in the async purge path and rely on the file’s existing tracing style instead. Replace the println!("purge_upto_index={purge_upto_index}") call in the role_state.rs purge logic with a tracing macro such as debug!, keeping the purge_upto_index context in the log fields. Make sure this hot path no longer writes directly to stdout and matches the surrounding instrumentation used elsewhere in role_state.rs.d-engine-server/src/api/embedded.rs (1)
330-344: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent path API surface:
start_customstill requires&str.
start_withandRaftNodeConfig::with_override_configwere widened toimpl AsRef<Path>in this PR, butstart_custom'sconfig_path: Option<&str>was left as-is, forcingPathBuf-based callers to convert manually.♻️ Suggested signature widening
- pub async fn start_custom( - storage_engine: Arc<SE>, - state_machine: Arc<SM>, - config_path: Option<&str>, - ) -> Result<Self> { - let node_config = if let Some(path) = config_path { - d_engine_core::RaftNodeConfig::default() - .with_override_config(path)? - .validate()? + pub async fn start_custom( + storage_engine: Arc<SE>, + state_machine: Arc<SM>, + config_path: Option<impl AsRef<std::path::Path>>, + ) -> Result<Self> { + let node_config = if let Some(path) = config_path { + d_engine_core::RaftNodeConfig::default() + .with_override_config(path)? + .validate()?🤖 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/embedded.rs` around lines 330 - 344, The `start_custom` API is still narrower than the updated path-based surface, since it takes `Option<&str>` while `start_with` and `RaftNodeConfig::with_override_config` now accept `impl AsRef<Path>`. Widen `start_custom`’s `config_path` parameter to a path-friendly type so `PathBuf` callers can pass values directly, then adjust the call into `with_override_config` accordingly while keeping `start_node` and the rest of `embedded.rs` behavior unchanged.d-engine/src/docs/server_guide/customize-state-machine.md (1)
82-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBatch example doesn't demonstrate real backend atomicity.
Individual
put/deletecalls per op mean a mid-batch failure leaves prior ops already applied tobackend. Consider a short note (or using the backend's native write-batch API) clarifying that atomicity across ops is the implementor's responsibility unless the backend supports it natively.🤖 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/src/docs/server_guide/customize-state-machine.md` around lines 82 - 94, The batch example in the state machine docs currently shows per-op backend writes in `Command::Batch`, which can mislead readers into assuming atomic behavior. Update the `Command::Batch` example in `customize-state-machine.md` to either use the backend’s native write-batch API or add a short note near `self.backend.put` and `self.backend.delete` explaining that atomicity across `d_engine::BatchOp` items is only guaranteed if the backend provides it.
🤖 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-client/src/grpc_client.rs`:
- Around line 360-404: The `batch()` method is still labeled as `put_with_ttl`
in its timer, comment, and `debug!`/`error!` log tags, so update all of those
identifiers to `batch` in `GrpcClient::batch` to keep metrics and logs accurate.
Keep the existing request/response flow the same, but ensure the
`ScopedTimer::new(...)` name and both log prefixes match the actual operation
name.
In `@d-engine-core/src/client/client_api.rs`:
- Around line 101-124: The batch API contract in client_api::batch is missing
enforcement of the empty/oversized ops rules. Add validation at the API boundary
in the batch implementations (or a shared write-path guard) so empty ops and
requests exceeding the max batch size return InvalidArgument before
Command::Batch { ops } is sent or applied. Use the batch method and
Command::Batch handling to locate the client/server entry points and keep the
docs aligned with the actual behavior.
In `@d-engine-core/src/command.rs`:
- Around line 65-69: The batch insert path in BatchOp::Insert and
TryFrom<WriteCommand> currently drops ttl_secs from proto Insert operations
without checking it. Update the batch conversion logic around BatchOp::Insert /
TryFrom<WriteCommand> to explicitly reject non-zero ttl_secs with a clear error,
or otherwise make the no-TTL behavior explicit in the batch write handling. Also
ensure GrpcClient::batch and EmbeddedClient::batch remain consistent with the
chosen behavior.
In `@d-engine-core/src/raft_role/candidate_state.rs`:
- Around line 209-224: Remove the redundant CandidateState override of
handle_create_snapshot in candidate_state.rs, since the trait default in
role_state.rs already returns the same RoleViolation when snapshot_in_progress()
is None. Keep CandidateState aligned with the inherited behavior by deleting
this method and relying on the default implementation, so the role logic stays
centralized and avoids drift.
In `@d-engine-core/src/raft_role/role_state.rs`:
- Around line 769-784: Phase 2 in role_state.rs can re-run the same purge
because scheduled_purge_upto remains set after LogPurgeCompleted. Update the
purge path around the execute_purge / InternalEvent::LogPurgeCompleted flow to
either clear the scheduled boundary on successful completion or skip Phase 2
when last_purged_index already satisfies the scheduled target, so repeated calls
do not duplicate work.
In `@d-engine-server/src/api/standalone_test.rs`:
- Around line 513-540: The CONFIG_PATH and RAFT__CLUSTER__DB_ROOT_DIR
environment mutations are not fully serialized across the affected tests, so
this test can race with others that read/write the same variables. Update the
test annotations so every env-mutating test in StandaloneEngine::run_custom, the
embedded_env_test cases, and node::builder_test uses the same #[serial(...)] key
as the existing run_custom group. This will make the serialization consistent
wherever CONFIG_PATH or RAFT__CLUSTER__DB_ROOT_DIR is set or cleared.
In `@d-engine-server/src/api/standalone.rs`:
- Line 139: The doc comment for the `config_path` parameter is malformed because
it mixes `config` and `config_path` in one bullet. Update the documentation in
the `standalone` API area so the parameter name matches the actual identifier
used by the function signature, and keep the comment consistent with the
surrounding Rust doc style.
In `@d-engine-server/src/storage/adaptors/file/file_state_machine.rs`:
- Around line 227-244: `encode_wal_entry`’s `Command::Batch` handling is writing
multiple batch ops into the WAL as if they were one record, which breaks the
fixed record layout expected by `replay_wal`. Update the `Command::Batch` branch
in `file_state_machine.rs` so each `BatchOp` is encoded as a complete standalone
WAL entry with its own `index`, `term`, opcode, key/value lengths, and trailing
`expire_at` field, matching the other command variants; also make sure empty
`Batch { ops: [] }` is handled safely.
---
Outside diff comments:
In `@d-engine-core/src/state_machine_handler/default_state_machine_handler.rs`:
- Around line 645-681: The `read_prev_values` helper is not tracking in-chunk
writes for `Command::Batch`, so later entries in the same chunk can see stale
prior values. Update the `Command::Batch` branch in `read_prev_values` to walk
the batch’s ops and apply the same `overlay` bookkeeping used for
`Insert`/`Delete` (and any key-writing batch op), so subsequent entries resolve
the correct in-chunk previous value. Keep the fix localized to
`read_prev_values` and use the batch operation keys to update the overlay
consistently.
In `@d-engine-server/src/api/embedded_client.rs`:
- Around line 613-658: The embedded_client::batch method currently allows an
empty ops vector to proceed even though its documentation says it must error;
add an early validation at the start of batch() to reject empty ops before
building ClientWriteRequest or sending the proposal. Return the appropriate
ClientApiResult error consistently with the existing error helpers used in
batch(), so the behavior matches the doc comment and avoids proposing an empty
WriteOperation::Batch.
In `@d-engine-server/src/api/embedded.rs`:
- Around line 267-305: The start_with flow rebuilds configuration again inside
start_custom/start_node instead of reusing the validated config already created
in start_with, which can drop CONFIG_PATH and RAFT__ overrides. Update
start_with to pass the existing config through to start_node (via start_custom
or a new overload) and use that same RaftNodeConfig for the running node,
storage, and lease setup; reference start_with, start_custom, and start_node
when wiring the fix.
---
Nitpick comments:
In `@d-engine-core/src/client/types.rs`:
- Around line 30-33: The ClientWriteRequest::command field is now opaque Bytes,
so add a brief doc comment on ClientWriteRequest and/or command explaining that
it must contain a serialized WriteCommand produced by write_op_to_bytes. Keep
the guidance close to the ClientWriteRequest type definition so API consumers
know not to pass arbitrary bytes.
In `@d-engine-core/src/raft_role/follower_state.rs`:
- Around line 515-526: The comment above handle_log_purge_completed is outdated
and no longer matches the implementation. Update it to describe that this method
handles both the follower’s own purge completions (scheduled via
schedule_and_execute_purge from handle_snapshot_created) and any stale in-flight
events from a prior leader role, and that it updates last_purged_index instead
of silently ignoring the event. Keep the wording aligned with the current
behavior in follower_state::handle_log_purge_completed.
In `@d-engine-core/src/raft_role/leader_state.rs`:
- Around line 1767-1796: The shared purge-scheduling path still contains an
unconditional debug println, which should not remain in production code. Review
the call from handle_snapshot_created into schedule_and_execute_purge and remove
the println!("purge_upto_index=...") or replace it with the existing tracing
macros (trace!/debug!) so it follows the rest of the codebase’s logging
conventions. Ensure the change is applied in schedule_and_execute_purge since it
affects Leader, Follower, and Learner snapshot handling.
In `@d-engine-core/src/raft_role/learner_state.rs`:
- Around line 613-624: The stale comment on learner log purge handling no longer
matches the behavior in handle_log_purge_completed: it now updates
last_purged_index, and this path also handles the learner’s own locally
scheduled purges. Update the comment in LearnerState::handle_log_purge_completed
to describe the current behavior instead of calling these events Leader-only
operations that are silently ignored, and keep it aligned with the analogous
comment in FollowerState::handle_log_purge_completed.
In `@d-engine-core/src/raft_role/role_state.rs`:
- Line 748: Remove the leftover debug println in the async purge path and rely
on the file’s existing tracing style instead. Replace the
println!("purge_upto_index={purge_upto_index}") call in the role_state.rs purge
logic with a tracing macro such as debug!, keeping the purge_upto_index context
in the log fields. Make sure this hot path no longer writes directly to stdout
and matches the surrounding instrumentation used elsewhere in role_state.rs.
In `@d-engine-proto/src/exts/client_ext.rs`:
- Around line 131-132: Remove the empty impl Batch block from the client_ext
module; it defines no methods and should be deleted unless a concrete Batch
method is being added in the same change. Locate the standalone impl Batch {}
near the Batch type in src/exts/client_ext.rs and remove it without affecting
any surrounding implementations or imports.
In `@d-engine-server/benches/state_machine.rs`:
- Around line 360-390: The Command-to-WatchResponse translation logic is
duplicated across bench_apply_with_1_watcher, bench_apply_with_10_watchers, and
bench_apply_with_100_watchers, including the new Command::Batch handling.
Extract the shared broadcast mapping into a helper near the existing watch-send
logic, and have each benchmark call that helper for Command values. Make sure
the helper covers all current Command variants and the BatchOp::Insert/Delete
cases so future Command changes only need to be updated in one place.
In `@d-engine-server/src/api/embedded.rs`:
- Around line 330-344: The `start_custom` API is still narrower than the updated
path-based surface, since it takes `Option<&str>` while `start_with` and
`RaftNodeConfig::with_override_config` now accept `impl AsRef<Path>`. Widen
`start_custom`’s `config_path` parameter to a path-friendly type so `PathBuf`
callers can pass values directly, then adjust the call into
`with_override_config` accordingly while keeping `start_node` and the rest of
`embedded.rs` behavior unchanged.
In `@d-engine-server/src/api/types.rs`:
- Around line 92-111: The BatchOp to ProtoBatchOp conversion is duplicated
between the WriteOperation::Batch mapping in types.rs and GrpcClient::batch in
grpc_client.rs, which breaks the intended single coupling point. Extract the
conversion into a shared helper or an impl From<BatchOp> for ProtoBatchOp (and
optionally a Vec mapping helper), then update both WriteOperation::Batch and
GrpcClient::batch to use that shared path so any new BatchOp variant only needs
one update.
In `@d-engine/src/docs/server_guide/customize-state-machine.md`:
- Around line 82-94: The batch example in the state machine docs currently shows
per-op backend writes in `Command::Batch`, which can mislead readers into
assuming atomic behavior. Update the `Command::Batch` example in
`customize-state-machine.md` to either use the backend’s native write-batch API
or add a short note near `self.backend.put` and `self.backend.delete` explaining
that atomicity across `d_engine::BatchOp` items is only guaranteed if the
backend provides it.
🪄 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: 761427c1-7567-40b9-90f3-c2518315281f
⛔ Files ignored due to path filters (1)
d-engine-proto/src/generated/d_engine.client.rsis excluded by!**/generated/**
📒 Files selected for processing (66)
CHANGELOG.mdMIGRATION_GUIDE.mdd-engine-client/src/grpc_client.rsd-engine-core/benches/leader_state_bench.rsd-engine-core/src/client/client_api.rsd-engine-core/src/client/mod.rsd-engine-core/src/client/types.rsd-engine-core/src/client/types_test.rsd-engine-core/src/command.rsd-engine-core/src/command_test.rsd-engine-core/src/config/cluster.rsd-engine-core/src/config/mod.rsd-engine-core/src/election/election_handler.rsd-engine-core/src/lib.rsd-engine-core/src/raft_role/candidate_state.rsd-engine-core/src/raft_role/candidate_state_test.rsd-engine-core/src/raft_role/follower_state.rsd-engine-core/src/raft_role/follower_state_test.rsd-engine-core/src/raft_role/leader_state.rsd-engine-core/src/raft_role/leader_state_test/backpressure_test.rsd-engine-core/src/raft_role/leader_state_test/buffer_cleanup_test.rsd-engine-core/src/raft_role/leader_state_test/client_write_test.rsd-engine-core/src/raft_role/leader_state_test/event_handling_test.rsd-engine-core/src/raft_role/leader_state_test/snapshot_test.rsd-engine-core/src/raft_role/leader_state_test/snapshot_worker_test.rsd-engine-core/src/raft_role/leader_state_test/state_management_test.rsd-engine-core/src/raft_role/learner_state.rsd-engine-core/src/raft_role/learner_state_test.rsd-engine-core/src/raft_role/mod.rsd-engine-core/src/raft_role/role_state.rsd-engine-core/src/raft_role/role_state_test.rsd-engine-core/src/replication/replication_handler.rsd-engine-core/src/state_machine_handler/default_state_machine_handler.rsd-engine-core/src/state_machine_handler/default_state_machine_handler_test.rsd-engine-core/src/storage/state_machine.rsd-engine-core/src/storage/state_machine_test.rsd-engine-core/src/test_utils/mock/mock_raft_builder.rsd-engine-proto/proto/client/client_api.protod-engine-proto/src/exts/client_ext.rsd-engine-server/benches/state_machine.rsd-engine-server/src/api/embedded.rsd-engine-server/src/api/embedded_client.rsd-engine-server/src/api/embedded_test/embedded_test.rsd-engine-server/src/api/mod.rsd-engine-server/src/api/standalone.rsd-engine-server/src/api/standalone_test.rsd-engine-server/src/api/types.rsd-engine-server/src/api/types_test.rsd-engine-server/src/lib.rsd-engine-server/src/node/builder.rsd-engine-server/src/proto_convert.rsd-engine-server/src/proto_convert_test.rsd-engine-server/src/storage/adaptors/file/file_state_machine.rsd-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rsd-engine-server/src/storage/adaptors/rocksdb/rocksdb_storage_engine.rsd-engine-server/src/storage/lease.rsd-engine-server/src/test_utils/mock/mock_node_builder.rsd-engine-server/tests/snapshot_and_recovery/mod.rsd-engine-server/tests/snapshot_and_recovery/snapshot_concurrent_writes_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_correctness_after_install_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_generation_standalone.rsd-engine/src/docs/quick-start-5min.mdd-engine/src/docs/server_guide/customize-state-machine.mdexamples/sled-cluster/src/lib.rsexamples/sled-cluster/src/sled_state_machine.rsexamples/sled-cluster/src/sled_storage_engine.rs
💤 Files with no reviewable changes (8)
- d-engine-core/benches/leader_state_bench.rs
- d-engine-core/src/raft_role/leader_state_test/snapshot_worker_test.rs
- d-engine-core/src/replication/replication_handler.rs
- d-engine-core/src/storage/state_machine.rs
- d-engine-core/src/election/election_handler.rs
- d-engine-core/src/lib.rs
- d-engine-server/tests/snapshot_and_recovery/snapshot_generation_standalone.rs
- d-engine-server/src/storage/adaptors/rocksdb/rocksdb_storage_engine.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
d-engine-server/src/api/standalone_test.rs (2)
513-540: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
RAFT__CLUSTER__DB_ROOT_DIRisn't cleaned up if an assertion panics.
shutdown_and_assert(line 484) uses.expect(...), which panics on failure. If that happens here,std::env::remove_var("RAFT__CLUSTER__DB_ROOT_DIR")at line 539 never runs, leaving a stale env var that can affect subsequent serialized tests in this suite.🔧 Proposed fix using a scope guard
+ struct EnvVarGuard(&'static str); + impl Drop for EnvVarGuard { + fn drop(&mut self) { + unsafe { std::env::remove_var(self.0) }; + } + } + unsafe { std::env::set_var( "RAFT__CLUSTER__DB_ROOT_DIR", temp_dir.path().join("db").to_str().unwrap(), ); } + let _guard = EnvVarGuard("RAFT__CLUSTER__DB_ROOT_DIR");🤖 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_test.rs` around lines 513 - 540, The test cleanup in test_run_custom_without_config_path_uses_defaults leaves RAFT__CLUSTER__DB_ROOT_DIR set if shutdown_and_assert panics, because the final remove_var call may never run. Move the env cleanup into a scope guard or equivalent deferred cleanup around the StandaloneEngine::run_custom and shutdown_and_assert block so the variable is always removed even on failure.
446-468: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse an ephemeral RPC port here
127.0.0.1:19733can still collide with another process and make this test fail intermittently; the other configs in this file already use127.0.0.1:0, so this helper should match.🤖 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_test.rs` around lines 446 - 468, The write_config helper is hardcoding a fixed RPC listen address, which can collide with other processes and make the standalone tests flaky. Update write_config in standalone_test.rs to use an ephemeral RPC port instead of 127.0.0.1:19733, matching the existing pattern used by the other test config helpers in this file so each test instance binds independently.d-engine-core/src/raft_role/learner_state_test.rs (1)
1309-1360: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrengthen assertion to also verify the purge
term, not justindex.The core PR change here is that the purge term is now looked up from the raft log (
entry_term(49) -> Some(1)) instead of the state machine. The mock correctly sets this expectation, but the final assertion only matchesLogId { index: 49, .. }, discarding thetermfield with... This leaves the exact behavior under test (term sourced from raft log) unverified — a regression that silently used a wrong/default term would still pass.🔧 Proposed fix
assert!( matches!( event, - InternalEvent::LogPurgeCompleted(LogId { index: 49, .. }) + InternalEvent::LogPurgeCompleted(LogId { index: 49, term: 1 }) ), "purge boundary must be last_included.index(50) - retained(1) = 49, got: {event:?}" );🤖 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-core/src/raft_role/learner_state_test.rs` around lines 1309 - 1360, The test in LearnerState::handle_snapshot_created only asserts the purge boundary index and ignores the term, so it does not verify that the term is sourced from the raft log lookup. Update the assertion around the InternalEvent::LogPurgeCompleted check to match the full LogId produced by the purge path, including both index and term, and keep the existing entry_term expectation in the mock to ensure the term flow from raft_log is covered.
🤖 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.
Outside diff comments:
In `@d-engine-core/src/raft_role/learner_state_test.rs`:
- Around line 1309-1360: The test in LearnerState::handle_snapshot_created only
asserts the purge boundary index and ignores the term, so it does not verify
that the term is sourced from the raft log lookup. Update the assertion around
the InternalEvent::LogPurgeCompleted check to match the full LogId produced by
the purge path, including both index and term, and keep the existing entry_term
expectation in the mock to ensure the term flow from raft_log is covered.
In `@d-engine-server/src/api/standalone_test.rs`:
- Around line 513-540: The test cleanup in
test_run_custom_without_config_path_uses_defaults leaves
RAFT__CLUSTER__DB_ROOT_DIR set if shutdown_and_assert panics, because the final
remove_var call may never run. Move the env cleanup into a scope guard or
equivalent deferred cleanup around the StandaloneEngine::run_custom and
shutdown_and_assert block so the variable is always removed even on failure.
- Around line 446-468: The write_config helper is hardcoding a fixed RPC listen
address, which can collide with other processes and make the standalone tests
flaky. Update write_config in standalone_test.rs to use an ephemeral RPC port
instead of 127.0.0.1:19733, matching the existing pattern used by the other test
config helpers in this file so each test instance binds independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c86304be-6dc0-4d8d-87a6-c20c164675ac
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
d-engine-client/src/grpc_client.rsd-engine-core/src/client/client_api.rsd-engine-core/src/command.rsd-engine-core/src/command_test.rsd-engine-core/src/raft_role/follower_state.rsd-engine-core/src/raft_role/follower_state_test.rsd-engine-core/src/raft_role/leader_state.rsd-engine-core/src/raft_role/leader_state_test/snapshot_test.rsd-engine-core/src/raft_role/learner_state.rsd-engine-core/src/raft_role/learner_state_test.rsd-engine-core/src/storage/state_machine_test.rsd-engine-server/src/api/embedded_client.rsd-engine-server/src/api/embedded_test/embedded_test.rsd-engine-server/src/api/standalone.rsd-engine-server/src/api/standalone_test.rsd-engine-server/src/storage/adaptors/file/file_state_machine.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- d-engine-core/src/client/client_api.rs
- d-engine-server/src/api/embedded_test/embedded_test.rs
- d-engine-client/src/grpc_client.rs
- d-engine-core/src/command_test.rs
- d-engine-core/src/command.rs
- d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs
- d-engine-core/src/raft_role/learner_state.rs
- d-engine-server/src/api/embedded_client.rs
- d-engine-core/src/raft_role/follower_state_test.rs
- d-engine-server/src/api/standalone.rs
- d-engine-server/src/storage/adaptors/file/file_state_machine.rs
- d-engine-core/src/raft_role/follower_state.rs
- d-engine-core/src/raft_role/leader_state.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This PR Do?
Fixes four embedded API gaps discovered during d-lmdb library-wrapper integration (#415) and a
snapshot label/data mismatch that could cause followers to double-apply committed entries (#418).
Type:
Why Is This Needed?
#415 — Embedded API gaps
Discovered while building d-lmdb on top of d-engine's embedded API. Four independent issues:
ClusterConfig::default()setinitial_cluster: vec![]but serde default returned[{id:1,...}]— programmatic config viaRaftNodeConfig::new()always got an empty cluster.RaftNodeConfigwithout writing a temp file — library wrappers were forced to do so as a workaround.\x00batchkey insideCommand::Insert, polluting user key space.start_withaccepted&str, forcing callers to convertPathBufmanually.#418 — Snapshot label/data mismatch
create_snapshotsubtractedretained_log_entriesfromlast_appliedto computelast_included, but the snapshot data already reflected the fulllast_appliedstate. A follower installing this snapshot would re-apply entries already captured in the data — non-idempotent operations execute twice, permanently diverging cluster state. Additionally,last_included.termwas copied fromlast_applied.terminstead of being looked up atlast_included.index, producing a LogId that never existed in the log.Fix:
last_included = last_applied(always truthful). Log purge is decoupled intoschedule_and_execute_purge, which computespurge_upto = last_included.index - retained_log_entriesand looks up the term fromraft_log.entry_term().StateMachine::entry_term()is removed from the trait — term lookup belongs to the log layer.Checklist
Required:
make testpassesIf changing APIs:
customize-state-machine.md,CHANGELOG.md,MIGRATION_GUIDE.md)Testing
How tested:
role_state_test.rs— 5 tests forschedule_and_execute_purge(happy path, zero boundary, commit-index guard, monotonicity, fault recovery);snapshot_test.rs— 4 tests forhandle_snapshot_createdpurge boundary;command_test.rs— decode tests for all Command variants including Batch;types_test.rs— WriteOperation → proto round-trip for all variants including Batchembedded_test.rs— linearizable read single-voter regression testFor bug fixes:
test_purge_boundary_is_last_included_minus_retained_log_entries,test_purge_skipped_when_entry_term_returns_none)Does This Follow d-engine's Principles?
Reviewer Notes
StateMachine::entry_term()is a breaking change for custom SM implementations — the method must be removed. SeeMIGRATION_GUIDE.mdfor the one-line migration.The purge logic refactor (
schedule_and_execute_purgeinrole_state.rs) is shared by Leader, Follower, and Learner — previously each role had its own copy. The core invariant:last_includedis always truthful,purge_uptois always derived from the log (never the snapshot label).Estimated review complexity:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation