Repository navigation
Merged
Conversation
…y log entry. feat #66: Refactor the Errors to distinguish between protocol logic errors and system-level errors. * feat #43: When a Raft leader is elected, it should first send an empty log entry. 1. implement verify_leadership_in_new_term 2. refactor verify_leadership_in_new_term and enforce_quorum_consensus * feat #66: Refactor the Errors to distinguish between protocol logic errors and system-level errors.
…r-friendly StorageEngine and StateMachine * feat #59: Refactor state_machine_commit_listener into a separate function to improve visibility across all newly spawned threads. * feat #79: Add snapshot feature * fix #90: Refactor: Decouple Client Command Protocol from Raft Internal Log Payload Type * feat #89: Add auto-discovery support for new learner nodes * feat #101: Revised learner join process with promotion/fail semantics * feat #45: Implemented first/last index for term to resolve replication conflicts * fix #106: Retry leadership noop confirmation until timeout * feat #102: Automatic node removal * feat #109: Enable RPC connection cache * fix #107: 1. Add back rpc connection params 2. Change parking lot sync lock to Tokio async lock in RaftMembership 3. optimize MembershipGuard for performance. Remove read locker. * feat #110: 1. add new events SnapshotCreated and LogPurgeCompleted 2. decouple Leader snapshot create to several events 3. refactor raft log index generation from pre_allocate_raft_logs_next_index to pre_allocate_id_range 4. optimize LocalLogBatch structure - based on performance analysis * feat #119: Make the StorageEngine trait more developer-friendly * feat #120: Make the StateMachine trait more developer-friendly * fix #121: Election: higher term vote request should be updated immediately and reset voted_for * feat #122: Refactor compress and decompress snapshot logic from StateMachine to StateMachineHandler * fix #123: Replace global tracing log with trace_test crate * feat #125: Make rocksdb adaptor as feature. Move sled implementation as example
…tart when path is already locked by another node
* refs #138: optimize to use long lived peer tasks for append entries request only * refs #138: try to fix race issue - Multiple threads can pass the initial check and all create new appenders * feat #138: 1. introduce crossbeam 2. optimize buffered_raft_log by creating a flush worker pool with configurable number of workers to handle persistence operations * feat #138: optimize flush_workers as configure * feat #138: optimize channel_capacity into configure * feat #139: work on a reliable monitoring metric for resource utilization * feat #140: Optimize proto bytes fields to use bytes::Bytes * feat #140: switch Into<Bytes> to AsRef<[u8]> * fix #141: Optimize RocksDB write path and Raft log loop for lower latency * feat 142: Add Read Consistency Policy Support and Lease-Based Read Optimization * feat #142: update v0.1.4 bench report * feat #142: add client doc guide * feat #143: Refactor gRPC compression configuration for Raft transport (performance optimization) - step1: disable server side client rpc compress response * feat #143: Refactor gRPC compression configuration for Raft transport (performance optimization) * feat #143: add performance diagram * feat #138: ready to publish v0.1.4 * feat #138: add ut * fix #146: buffered_raft_log.rs Code Reviews * fix #146: update bench report * fix #145: Bug: Undefined Behavior from Incorrect Lifetime Conversion in Mmap Zero-Copy Path * fix #148: fix rocksdb storage engine flush bug * fix #147: fix file state machine bug
|
Important Review skippedToo many files! This PR contains 216 files, which is 66 over the limit of 150. You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Merges develop into main for v0.2.3, consolidating CAS support, a unified client API, Raft correctness fixes, and drain-based batching for improved throughput/latency.
Changes:
- Replaces timeout-based batching with drain-driven command ingestion and new leader-side batching buffers (AoS → SoA for proposes).
- Introduces a unified
ClientApi(core + client crate) and updates docs/benchmarks accordingly. - Refactors Raft snapshot/purge responsibilities to per-node behavior and adds/updates extensive Raft role/tests.
Reviewed changes
Copilot reviewed 75 out of 211 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| d-engine-core/src/replication/batch_buffer.rs | Removes legacy timeout-based BatchBuffer implementation (replaced by new buffers module). |
| d-engine-core/src/raft_test/mod.rs | Adds a structured raft test module layout (submodules + extensive test plan docs). |
| d-engine-core/src/raft_test/leader_discovered_tests.rs | Adds unit tests for LeaderDiscovered notifications and dedup behavior. |
| d-engine-core/src/raft_test/leader_change_tests.rs | Adds tests for leader-change notification channel semantics. |
| d-engine-core/src/raft_role/role_state.rs | Extends role state API for client command push/flush and adds snapshot helper utilities. |
| d-engine-core/src/raft_role/raft_role_test.rs | Minor import cleanup for concurrency test. |
| d-engine-core/src/raft_role/mod.rs | Exposes new buffers module and adds role helpers for cmd push/flush. |
| d-engine-core/src/raft_role/learner_state.rs | Adds learner snapshot creation + per-node purge on snapshot completion; handles ApplyCompleted/FatalError. |
| d-engine-core/src/raft_role/leader_state_test/state_management_test.rs | Updates leader purge tests to match new purge logic behavior. |
| d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs | Removes large suites tied to old leader-driven purge transport behavior. |
| d-engine-core/src/raft_role/leader_state_test/replication_test.rs | Updates replication tests for new batching/request shapes and quorum checks. |
| d-engine-core/src/raft_role/leader_state_test/mod.rs | Registers new leader tests (fatal error + backpressure). |
| d-engine-core/src/raft_role/leader_state_test/membership_change_test.rs | Adjusts membership tests for new batching config and quorum API changes. |
| d-engine-core/src/raft_role/leader_state_test/fatal_error_test.rs | Adds a leader FatalError handling test. |
| d-engine-core/src/raft_role/leader_state_test/event_handling_test.rs | Updates leader event tests for cmd buffering/flush and adds drain-on-stepdown test. |
| d-engine-core/src/raft_role/leader_state_test/cluster_metadata_test.rs | Removes peer purge progress tests tied to removed purge RPC flow. |
| d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs | Adds backpressure enforcement tests for write/read buffering. |
| d-engine-core/src/raft_role/follower_state.rs | Adds follower snapshot handling + per-node purge; adds ApplyCompleted/FatalError handling. |
| d-engine-core/src/raft_role/candidate_state_test.rs | Updates candidate tests for fake-time election timer + new client cmd API. |
| d-engine-core/src/raft_role/candidate_state.rs | Updates candidate tick behavior and event handling; adds ApplyCompleted/FatalError handling. |
| d-engine-core/src/raft_role/buffers/propose_batch_buffer_test.rs | Adds unit tests for new SoA propose batch buffer. |
| d-engine-core/src/raft_role/buffers/propose_batch_buffer.rs | Introduces SoA propose batching buffer (O(1) flush via swaps) + optional metrics. |
| d-engine-core/src/raft_role/buffers/mod.rs | Adds buffers module exports and buffer documentation. |
| d-engine-core/src/raft_role/buffers/batch_buffer_test.rs | Adds tests for the new generic BatchBuffer. |
| d-engine-core/src/raft_role/buffers/batch_buffer.rs | Introduces new simple BatchBuffer for contiguous item buffering + optional metrics. |
| d-engine-core/src/raft.rs | Adds cmd channel ingestion, drain-based batching, and NotifyNewCommitIndex coalescing. |
| d-engine-core/src/network/mod.rs | Removes purge request API from transport trait (purge no longer leader-RPC driven). |
| d-engine-core/src/membership.rs | Replaces is_multiple_of usage with modulo check for MSRV compatibility. |
| d-engine-core/src/maybe_clone_oneshot.rs | Adds try_recv() for test-only receiver behavior. |
| d-engine-core/src/lib.rs | Exposes new client module from core crate. |
| d-engine-core/src/event.rs | Introduces ClientCmd, adds FatalError/ApplyCompleted, removes old client/purge raft events. |
| d-engine-core/src/config/retry.rs | Removes purge-log retry policy and validation. |
| d-engine-core/src/config/raft_test.rs | Adds BackpressureConfig unit tests. |
| d-engine-core/src/config/raft.rs | Adds batching/backpressure/metrics config; changes persistence default to DiskFirst; removes commit-handler & read-batching config. |
| d-engine-core/src/config/config_test.rs | Minor import cleanup in config tests. |
| d-engine-core/src/commit_handler/default_commit_handler.rs | Switches commit handler to drain-driven batching and offloads apply to SM worker. |
| d-engine-core/src/client/mod.rs | Adds core client module exports for unified client API. |
| d-engine-core/src/client/client_api_error_test.rs | Adds tests for ClientApiError conversions and helpers. |
| d-engine-core/src/client/client_api_error.rs | Introduces ClientApiResult alias. |
| d-engine-core/src/client/client_api.rs | Adds unified ClientApi trait with CAS and cluster/read APIs. |
| d-engine-core/benches/leader_state_bench.rs | Updates benchmark to match new state machine handler apply return type. |
| d-engine-core/Cargo.toml | Sets crate rust-version from workspace. |
| d-engine-client/src/proto/client_ext_test.rs | Updates tests for new WriteResult response variant. |
| d-engine-client/src/proto/client_ext.rs | Updates response decoding for WriteResult and clarifies semantics. |
| d-engine-client/src/mock_rpc_service.rs | Adds CAS-aware mock server helper. |
| d-engine-client/src/lib.rs | Refactors to unified ClientApi surface and new GrpcClient backing. |
| d-engine-client/src/kv_error.rs | Deletes legacy KV error type in favor of ClientApiError. |
| d-engine-client/src/kv_client.rs | Deletes legacy KV trait in favor of ClientApi. |
| d-engine-client/src/grpc_client.rs | Renames/refactors gRPC client to implement ClientApi, adds CAS + watch method. |
| d-engine-client/src/cluster_test.rs | Deletes cluster client test tied to removed ClusterClient. |
| d-engine-client/src/cluster.rs | Deletes legacy ClusterClient module. |
| d-engine-client/src/builder.rs | Updates builder to construct unified GrpcClient-backed Client. |
| d-engine-client/Cargo.toml | Sets crate rust-version from workspace. |
| config/base/raft.toml | Adds [raft.batching] section; removes commit-handler section. |
| benches/standalone-bench/src/main.rs | Updates benchmark to use unified Client API (ClientApi). |
| benches/embedded-bench/src/main.rs | Adds batch-mode follower auto-shutdown via a signal file. |
| benches/embedded-bench/config/n3.toml | Adds batching + metrics sections for embedded bench configs. |
| benches/embedded-bench/config/n2.toml | Adds batching + metrics sections for embedded bench configs. |
| benches/embedded-bench/config/n1.toml | Adds batching + metrics sections for embedded bench configs. |
| benches/embedded-bench/README.md | Updates naming (EmbeddedClient) and benchmark mode descriptions. |
| benches/embedded-bench/Makefile | Adjusts batch test parameters (clients). |
| README.md | Updates EmbeddedClient naming and MSRV requirement. |
| Cargo.toml | Sets workspace rust-version = 1.85. |
| CONTRIBUTING.md | Updates contribution target branch to main and adds rebase instructions. |
| CHANGELOG.md | Adds v0.2.3 unreleased entry documenting breaking changes and new features. |
| .github/workflows/ci.yml | Adds MSRV job, pins actions, updates coverage/nextest installation versions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
d-engine-core/src/raft_role/learner_state.rs (1)
786-794:⚠️ Potential issue | 🟡 MinorPreserve purge history when demoting to Learner.
Resetting
last_purged_indextoNonedrops monotonic purge tracking and can re-issue redundant/backward purges after a demotion. Carry the value over (or expose an accessor) to keep purge state consistent.🛠️ Suggested fix
impl<T: TypeConfig> From<&FollowerState<T>> for LearnerState<T> { fn from(follower_state: &FollowerState<T>) -> Self { Self { shared_state: follower_state.shared_state.clone(), node_config: follower_state.node_config.clone(), snapshot_in_progress: AtomicBool::new(false), - last_purged_index: None, //TODO + last_purged_index: follower_state.last_purged_index, _marker: PhantomData, } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/learner_state.rs` around lines 786 - 794, When converting a FollowerState to a LearnerState in the impl From<&FollowerState<T>> for LearnerState<T>, preserve the follower's purge history by copying its last_purged_index into the new LearnerState instead of resetting it to None; locate the constructor for LearnerState in the from(follower_state: &FollowerState<T>) -> Self and set last_purged_index to the follower_state.last_purged_index (cloning or copying as appropriate) so monotonic purge tracking is maintained after demotion.d-engine-core/src/config/raft.rs (1)
142-165:⚠️ Potential issue | 🟡 MinorValidate
metrics.sample_rateto prevent invalid configs.If
sample_rateis set to0, any modulo/div-based sampling logic will be unsafe. Add a small validation hook and call it fromRaftConfig::validate().🛠️ Suggested fix
impl RaftConfig { /// Validates all Raft subsystem configurations pub fn validate(&self) -> Result<()> { ... self.read_consistency.validate()?; self.watch.validate()?; + self.metrics.validate()?; ... Ok(()) } }impl MetricsConfig { + fn validate(&self) -> Result<()> { + if self.sample_rate == 0 { + return Err(Error::Config(ConfigError::Message( + "metrics.sample_rate must be >= 1".into(), + ))); + } + Ok(()) + } +}Also applies to: 1317-1383
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/config/raft.rs` around lines 142 - 165, Add a validation check to ensure metrics.sample_rate is > 0 and invoke it from RaftConfig::validate: implement a small validate() method on the MetricsConfig (or add logic in an existing metrics.validate()) that returns an Err(Error::Config(...)) when sample_rate == 0, and then call this new metrics.validate() from RaftConfig::validate() alongside the other subsystem validates (e.g., replication.validate(), batching.validate(), etc.) so invalid zero sample_rate configs are rejected early.
🧹 Nitpick comments (20)
.github/workflows/commit-message-check.yml (1)
11-11: Consider removing this condition after develop branch is retired.This condition appropriately allows the develop → main merge PR to bypass commit message validation. However, per the PR objectives, develop is being retired and main becomes the single contribution target. After the merge, this condition becomes dead code since no future PRs will originate from
develop.Consider adding a follow-up task to remove this condition once the develop branch is deleted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/commit-message-check.yml at line 11, The workflow contains a conditional guard "if: github.head_ref != 'develop'" that is intended to skip commit-message checks for develop→main PRs but will become dead code once the develop branch is retired; remove this conditional from .github/workflows/commit-message-check.yml after develop is deleted, and add a follow-up task or issue to track the removal so the workflow no longer contains the unused "if: github.head_ref != 'develop'" check.d-engine-core/src/raft_test/leader_change_tests.rs (1)
34-52: Consider clarifying the test intent.The test name
test_multiple_listenersand comment "Test broadcasting to multiple listeners" suggest a broadcast scenario, but the implementation creates two independent channels and sends to each separately. This tests that multiple independent notification paths work, not actual broadcast semantics.If this is intentional (testing the pattern used in production code), consider updating the comment to clarify:
#[test] fn test_multiple_listeners() { - // Test broadcasting to multiple listeners + // Test that multiple independent listeners can each receive notifications let (tx1, mut rx1) = mpsc::unbounded_channel::<(Option<u32>, u64)>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_test/leader_change_tests.rs` around lines 34 - 52, The test name and comment claim to verify broadcasting but the code creates two independent channels (tx1/tx2 with rx1/rx2) and sends to each separately; update the test to match intent by either renaming the test from test_multiple_listeners to something like test_multiple_independent_listeners and change the comment to “Test that multiple independent notification channels each receive their own messages” or, if you intended to test broadcast semantics, replace the two separate channels with a single broadcast mechanism (e.g., use a shared broadcaster/cloneable receiver) and send one message to ensure both listeners (rx1, rx2) receive the same broadcast; refer to the test function name test_multiple_listeners and the channel variables tx1, tx2, rx1, rx2 when making the change.d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs (1)
148-165: Doc comment describes a different test scenario than implemented.The docstring describes "SnapshotCreated with transport error" and "sending purge requests to peers," but the actual test (
test_handle_log_purge_completed) verifiesLogPurgeCompletedevent handling — updatinglast_purged_indexcorrectly. Consider updating the docstring to accurately reflect the test purpose.📝 Suggested doc fix
-/// Test handling SnapshotCreated with transport error +/// Test handling LogPurgeCompleted event /// /// # Test Scenario -/// Leader completes snapshot creation but encounters transport error -/// when sending purge requests to peers. +/// Leader receives LogPurgeCompleted events and updates last_purged_index. /// /// # Given -/// - Leader with completed snapshot -/// - Mock transport returns error +/// - Leader with initial last_purged_index /// /// # When -/// - SnapshotCreated event is handled +/// - LogPurgeCompleted events are received with various indices /// /// # Then -/// - Event handling returns error -/// - No role transition events sent -/// - snapshot_in_progress flag is reset +/// - Higher indices update last_purged_index +/// - Lower indices are ignored +/// - First purge initializes last_purged_index from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs` around lines 148 - 165, Update the doc comment to match the actual test implementation: replace the SnapshotCreated/transport-error/purge-peers description with a clear description for test_handle_log_purge_completed that states it verifies handling of LogPurgeCompleted updates last_purged_index, ensures no role transitions are emitted, and resets snapshot_in_progress as appropriate; reference the test function name test_handle_log_purge_completed and remove any references to sending purge requests or transport errors.d-engine-core/src/raft_role/buffers/propose_batch_buffer.rs (2)
139-141: Consider adding#[must_use]tolen().Similar to
BatchBuffer, thelen()return value should typically be used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/propose_batch_buffer.rs` around lines 139 - 141, Add the #[must_use] attribute to the len() method on the propose batch buffer so callers are warned when its returned usize is ignored; locate the pub fn len(&self) -> usize { self.payloads.len() } definition (in propose_batch_buffer.rs) and place #[must_use] immediately above that function signature, mirroring the BatchBuffer implementation.
106-133: Metrics gauge not reset after flush.Same issue as
BatchBuffer: whenflush()drains the buffer, the gauge retains its last value. Consider resetting to 0 for accurate monitoring.Additionally, the replacement vectors are sized to
n(current batch length), which is sensible for warm cache, but subsequent batches may have different sizes. This is acceptable behavior but worth noting.📊 Proposed fix to reset gauge on flush
pub fn flush(&mut self) -> Option<RaftRequestWithSignal> { if self.payloads.is_empty() { return None; } self.last_flush = Instant::now(); + // Reset gauge on flush + if self.metrics_enabled { + if let Some(ref labels) = self.metrics_labels { + metrics::gauge!("batch.buffer_length", labels.as_ref()).set(0.0); + } + } + // Allocate replacement Vecs sized to actual batch length, not a fixed max. let n = self.payloads.len();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/propose_batch_buffer.rs` around lines 106 - 133, The flush() method currently swaps out payloads/senders but does not reset the monitoring gauge that tracks the buffer size (same gauge used in BatchBuffer), so the metric remains at the previous batch size; after performing the swaps in propose_batch_buffer::flush (i.e., immediately after the std::mem::swap calls and before returning Some(...)), set the buffer-size gauge to 0 (the same gauge you use in BatchBuffer) so monitoring reflects the empty buffer; keep the reset adjacent to the swaps and ensure it runs in both the empty-path (if any) and the non-empty path.d-engine-core/src/raft_role/buffers/batch_buffer.rs (2)
46-63: Metrics gauge not reset to 0 after take_all().When
take_all()drains the buffer, thebatch.buffer_lengthgauge remains at the last pushed value until the nextpush(). This could cause misleading metrics during idle periods. Consider resetting the gauge intake_all()if metrics are enabled.📊 Proposed fix to reset gauge on drain
pub fn take_all(&mut self) -> Vec<E> { self.last_flush = Instant::now(); + if self.metrics_enabled { + if let Some(ref labels) = self.metrics_labels { + metrics::gauge!("batch.buffer_length", labels.as_ref()).set(0.0); + } + } std::mem::take(&mut self.buffer) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/batch_buffer.rs` around lines 46 - 63, The batch.buffer_length gauge is only updated in push(), so after take_all() drains the buffer the metric stays at the last value; modify take_all() (the method that empties the internal buffer) to check metrics_enabled and metrics_labels and reset the gauge to 0 (e.g., call metrics::gauge!("batch.buffer_length", labels.as_ref()).set(0.0)) after clearing or swapping out self.buffer so the metric reflects an empty buffer; ensure you use the same labels handling as in push() to avoid panics when metrics_labels is None.
75-77: Consider adding#[must_use]attribute tolen().The
len()method returns a value that should typically be used. Adding#[must_use]would help catch accidental unused calls.🔧 Suggested improvement
+ #[must_use] pub fn len(&self) -> usize { self.buffer.len() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/batch_buffer.rs` around lines 75 - 77, Add the #[must_use] attribute to the len method to catch accidental ignored return values; locate the pub fn len(&self) -> usize { ... } implementation in the BatchBuffer (buffers/batch_buffer.rs) and place #[must_use] directly above that function signature so callers who ignore the returned usize will get a compiler warning.d-engine-client/src/mock_rpc_service.rs (1)
203-214: Consider extracting common ClusterMembership builder.The
ClusterMembershipconstruction with a single leader node is duplicated acrosssimulate_client_read_mock_server,simulate_client_write_mock_server, andsimulate_cas_mock_server. A shared helper would reduce duplication.♻️ Suggested helper extraction
fn default_single_leader_membership(port: u16) -> ClusterMembership { ClusterMembership { version: 1, nodes: vec![NodeMeta { id: 1, role: NodeRole::Leader as i32, address: format!("127.0.0.1:{port}"), status: NodeStatus::Active.into(), }], current_leader_id: Some(1), } }Then use
Box::new(|port| Ok(default_single_leader_membership(port)))in each function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-client/src/mock_rpc_service.rs` around lines 203 - 214, Extract the duplicated ClusterMembership construction into a single helper function (e.g., fn default_single_leader_membership(port: u16) -> ClusterMembership) that returns the ClusterMembership with NodeMeta, NodeRole::Leader, address format!("127.0.0.1:{port}"), and NodeStatus::Active, then replace the repeated Box::new(|port: u16| { Ok(ClusterMembership { ... }) }) closures in simulate_client_read_mock_server, simulate_client_write_mock_server, and simulate_cas_mock_server with Box::new(|port| Ok(default_single_leader_membership(port))) so all three use the shared builder.d-engine-core/src/commit_handler/default_commit_handler.rs (1)
100-101: Consider logging when batch limit is reached.When the drain loop hits
max_batch_size, it might indicate backpressure. A trace-level log could help with debugging high-load scenarios.📝 Suggested logging
Err(_) => break, } } + if count == self.max_batch_size { + trace!("[Node-{}] Batch limit reached, may have pending commits", self.my_id); + } trace!("[Node-{}] Processing batch with {} commit notifications", self.my_id, count);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/commit_handler/default_commit_handler.rs` around lines 100 - 101, Add a trace-level log right before breaking out of the drain loop when the batch limit is reached: in default_commit_handler.rs, locate the drain loop's match arm that currently reads "Err(_) => break" and replace it with a block that logs a trace including max_batch_size and the current batch count/context (use the existing batch variable or counter visible in that scope) and then breaks; use the module's logger (or trace! macro) so backpressure/high-load events are recorded for debugging.d-engine-core/src/event.rs (1)
221-228: Placeholder mappings may mask test bugs.
StepDownSelfRemovedandMembershipAppliedmap toTestEvent::CreateSnapshotEventas placeholders. If tests accidentally emit these events, the placeholder will silently pass. Consider adding dedicatedTestEventvariants or using aPlaceholdervariant to make test failures more explicit.💡 Suggested improvement
+#[cfg(test)] +#[cfg_attr(test, derive(Debug, Clone))] +pub enum TestEvent { + // ... existing variants ... + + /// Placeholder for internal control events not meant for test assertions + InternalControlEvent, +} RaftEvent::StepDownSelfRemoved => { - TestEvent::CreateSnapshotEvent // Placeholder + TestEvent::InternalControlEvent } RaftEvent::MembershipApplied => { - TestEvent::CreateSnapshotEvent // Placeholder + TestEvent::InternalControlEvent }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/event.rs` around lines 221 - 228, The match arms that convert RaftEvent::StepDownSelfRemoved and RaftEvent::MembershipApplied currently return TestEvent::CreateSnapshotEvent as a silent placeholder; update the conversion (the match in the Raft->TestEvent mapping function) to return explicit test-visible variants instead—either add dedicated TestEvent::StepDownSelfRemoved and TestEvent::MembershipApplied variants to the TestEvent enum and map those RaftEvent arms to them, or introduce a generic TestEvent::Placeholder(RaftEvent) or TestEvent::Internal(RaftEvent) variant and map both RaftEvent::StepDownSelfRemoved and RaftEvent::MembershipApplied to that so tests fail loudly if these events are emitted unexpectedly; ensure you update the TestEvent enum declaration and any pattern matches that assume CreateSnapshotEvent was used as the placeholder.d-engine-core/src/commit_handler/default_commit_handler_test.rs (2)
102-107: Return mock apply results per entry to reflect ApplyCompleted semantics.The mock now always returns an empty vec; with apply results used for ApplyCompleted/pending_requests, this can mask mismatched result cardinality. Consider returning a result vector sized to the applied command batch (dummy values are fine).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/commit_handler/default_commit_handler_test.rs` around lines 102 - 107, The mock implementation of mock_smh.expect_apply_chunk currently always returns an empty Vec which breaks ApplyCompleted/pending_requests semantics; modify the returning closure used in mock_smh.expect_apply_chunk() (the closure that calls command_hook()) to produce a Vec whose length matches the number of entries in the applied command batch (e.g., map the input batch to a Vec of dummy Ok results or appropriate Error values per entry) so ApplyCompleted receives per-entry results and cardinality mismatches are surfaced; ensure the closure still respects the command_hook() branch to return Err(Error::Fatal(...)) when needed.
147-176: SM worker drains only; consider exercising apply-result path and parameterizing batch size.The worker currently discards entries, so apply-result/fatal-error propagation isn’t exercised by these tests, and
max_batch_sizeis hardcoded to 10. Consider routing entries throughmock_smh.apply_chunk(and forwarding results/errors) and passingmax_batch_sizeviasetup_harnesswhere needed to keep batching tests meaningful.Also applies to: 182-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/commit_handler/default_commit_handler_test.rs` around lines 147 - 176, The SM worker in run_handler is currently just draining sm_apply_rx so tests never exercise apply-result or error paths; change the spawned task to call the mock state machine's apply_chunk (mock_smh.apply_chunk) for each received entry batch and forward the resulting Ok/Err back to the handler path (so apply-result/fatal-error propagation is exercised), and make max_batch_size configurable by passing it through setup_harness into CommitHandlerDependencies.max_batch_size instead of hardcoding 10 so batching tests can vary it; update both run_handler and the similar worker block at 182-209 to use mock_smh.apply_chunk and respect the injected max_batch_size.d-engine-core/src/raft_role/buffers/batch_buffer_test.rs (3)
1-4: Unused import:tokio::time::Instantis imported butstd::time::Instantsemantics are used.The code imports
tokio::time::Instantbut appears to use it likestd::time::Instant. Since these tests don't usetokio::time::pause()or other tokio time manipulation features, consider usingstd::time::Instantfor clarity, or verify if tokio's Instant is intentionally required for consistency with the production code.💡 Proposed fix
use super::*; use batch_buffer::BatchBuffer; use std::time::Duration; -use tokio::time::Instant; +use std::time::Instant;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/batch_buffer_test.rs` around lines 1 - 4, The test imports tokio::time::Instant but the tests use standard Instant semantics; remove the tokio import and switch to std::time::Instant (or replace the use sites to explicitly reference std::time::Instant) so the import matches usage; update the use list at the top (remove "use tokio::time::Instant" and ensure "use std::time::Instant" is present) in batch_buffer_test.rs where BatchBuffer and Instant are referenced.
18-20: Timing-based assertions may be flaky in CI environments.Tests like
test_new_initialization(line 20),test_take_all_resets_buffer(line 78), andtest_take_all_resets_last_flush_timer(lines 136-143) rely on timing assertions with tight thresholds (50ms, 100ms). These can fail on resource-constrained CI runners or under heavy load.Consider using more generous thresholds or restructuring tests to avoid wall-clock timing sensitivity where possible.
Also applies to: 77-78, 136-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/batch_buffer_test.rs` around lines 18 - 20, The timing assertions in batch_buffer_test.rs (tests test_new_initialization, test_take_all_resets_buffer, test_take_all_resets_last_flush_timer) are flaky; update the assertions that check buffer.last_flush.elapsed() against tight Durations (50ms, 100ms) by either increasing the thresholds to a more generous value (e.g., several hundred ms) or by avoiding wall-clock checks entirely: inject or mock the clock used by BatchBuffer (refer to buffer.last_flush and any creation path of BatchBuffer) and assert using the mocked time, or assert relative ordering (e.g., last_flush updated after take_all) instead of absolute elapsed < X; ensure all references to Duration comparisons in these tests are changed consistently.
13-21: Tests access internalbuffer.bufferfield directly.Lines 18-19 and elsewhere access the internal
bufferfield directly. While acceptable for unit tests, this couples tests tightly to the implementation. If the internal structure changes, these tests will break even if the public API remains correct.Consider using only public methods (
is_empty(),len(),take_all()) where possible to make tests more resilient to refactoring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/batch_buffer_test.rs` around lines 13 - 21, The test currently reaches into BatchBuffer's internals (buffer.buffer and last_flush); change it to use only the public API: assert buffer.is_empty() (or buffer.len() == 0) instead of buffer.buffer.is_empty(), call buffer.take_all() and assert the returned Vec's capacity() >= initial_capacity instead of inspecting buffer.buffer.capacity(), and stop accessing last_flush directly—either remove that direct assertion or replace it with a call to a public accessor (e.g., add/get_last_flush or elapsed_since_last_flush) and assert via that method; update references to BatchBuffer::<TestRequest>::new, is_empty(), len(), and take_all() in the test accordingly.d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs (1)
299-300: Direct internal queue manipulation for test isolation.Using
leader.lease_read_queue.clear()directly accesses internal state. While pragmatic for test isolation between read policy checks, consider whether aclear_buffers()method or separate test functions would improve maintainability.This is acceptable for unit tests but worth noting if the internal structure changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs` around lines 299 - 300, The test directly mutates internal state via leader.lease_read_queue.clear(), which couples tests to implementation; add a small public test-only helper such as Leader::clear_buffers() (or similar) and call that from backpressure_test instead of touching lease_read_queue directly to improve maintainability and resilience to internal changes, or alternatively split the test into separate functions so shared state isn't reset mid-test; update references in the test from leader.lease_read_queue.clear() to leader.clear_buffers() (or the chosen helper) and keep the helper limited to cfg(test) or pub(crate) test visibility to avoid exposing internals in production.d-engine-core/src/raft_role/buffers/propose_batch_buffer_test.rs (1)
98-110: Timing-based assertion may be flaky.Line 107 uses
Duration::from_millis(100)threshold forlast_flush.elapsed(). Similar to the BatchBuffer tests, this could be flaky under CI load. Consider a more generous threshold (e.g., 500ms or 1s) since the test only needs to verify the timestamp was updated, not precise timing.💡 Suggested threshold increase
assert!( - buf.last_flush.elapsed() < std::time::Duration::from_millis(100), + buf.last_flush.elapsed() < std::time::Duration::from_secs(1), "last_flush not updated after flush" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/propose_batch_buffer_test.rs` around lines 98 - 110, The timing assertion in the test flush_resets_last_flush_timestamp uses a too-tight threshold; update the assertion in the test function to use a more generous duration (e.g., Duration::from_millis(500) or Duration::from_secs(1)) so buf.last_flush.elapsed() is compared against that larger threshold, keeping the test's intent (that ProposeBatchBuffer::flush updates last_flush) while avoiding CI flakiness.benches/embedded-bench/src/main.rs (1)
466-467: Consider making the signal file path configurable or unique per run.The hardcoded
/tmp/embedded-bench-donepath could cause issues if multiple benchmark instances run concurrently (e.g., in CI or parallel test runs), as they would interfere with each other's coordination. Consider:
- Using a unique path per run (e.g., include PID or timestamp)
- Making it configurable via CLI argument
💡 Example approach using unique path
- // Signal file used to coordinate follower auto-shutdown in batch mode - let done_signal_path = "/tmp/embedded-bench-done"; + // Signal file used to coordinate follower auto-shutdown in batch mode + let done_signal_path = format!("/tmp/embedded-bench-done-{}", std::process::id());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/embedded-bench/src/main.rs` around lines 466 - 467, The hardcoded signal file path (done_signal_path = "/tmp/embedded-bench-done") can collide across concurrent runs; change it to be configurable and/or unique per run by reading a CLI option or environment variable (e.g., add a CLI flag parsed in main or the existing arg parsing code to accept --done-signal) and defaulting to a generated path that includes the PID or timestamp (or both) when the flag is not provided; update all references to done_signal_path in main.rs so they use the new configurable variable (the original symbol done_signal_path) and ensure cleanup logic still removes the generated file on exit.d-engine-client/src/lib.rs (1)
165-168: Consider returningResultinstead of panicking on empty endpoints.Using
assert!in a public API method will cause a panic at runtime if the user passes an empty vector. While this is documented in the doc comment, aResult<ClientBuilder, ClientApiError>return type would provide better error handling ergonomics.💡 Suggested improvement
- pub fn builder(endpoints: Vec<String>) -> ClientBuilder { - assert!(!endpoints.is_empty(), "At least one endpoint required"); - ClientBuilder::new(endpoints) + pub fn builder(endpoints: Vec<String>) -> Result<ClientBuilder, ClientApiError> { + if endpoints.is_empty() { + return Err(ClientApiError::InvalidArgument("At least one endpoint required".into())); + } + Ok(ClientBuilder::new(endpoints)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-client/src/lib.rs` around lines 165 - 168, Change the public function signature of builder from returning ClientBuilder to Result<ClientBuilder, ClientApiError> and stop using assert!; instead check endpoints.is_empty() and return Err(ClientApiError::InvalidArguments("empty endpoints".into())) (or add a specific ClientApiError::EmptyEndpoints variant) when empty, otherwise return Ok(ClientBuilder::new(endpoints)); update the builder function, the ClientApiError enum to include the chosen variant, adjust docs to reflect the Result return, and fix/update any call sites/tests that expect the old panicking behavior to handle the Result.d-engine-core/src/raft_test/drain_based_batch_architecture_tests.rs (1)
713-730: The assertionflush_count > 0is trivially true.The loop at lines 716-725 always executes 10 times, incrementing
flush_countunconditionally. The assertion at line 730 will always pass regardless of the actual batching behavior being tested.Consider asserting something more meaningful about the batching behavior, such as verifying that commands were actually processed or that the buffer state changed.
💡 Suggested improvement
// **Assertion**: Multiple flush cycles occurred // (With moderate load, we expect more than 1 cycle but less than 50) // This verifies natural batching behavior - assert!(flush_count > 0, "Should complete flush operations"); + // Since we pushed 50 commands with max_batch_size likely < 50, + // verify we completed the flush cycles and buffer is empty + assert!(flush_count == 10, "Should complete all flush operations"); + assert!( + leader.propose_buffer.is_empty(), + "Buffer should be empty after all flush cycles" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_test/drain_based_batch_architecture_tests.rs` around lines 713 - 730, The current assertion is trivial because flush_count is always incremented; instead, verify actual work was done by checking flush results or buffer state: call leader.flush_cmd_buffers(&raft.ctx, &role_tx) and capture its return (e.g., processed count or bool), sum those to a processed_commands counter, or query buffer length via a method like leader.cmd_buffer_len(&raft.ctx) before/after the loop and assert it decreased (or reached zero). Update the test to assert processed_commands > 0 or that leader.cmd_buffer_len changed, using the symbols flush_cmd_buffers, raft.ctx, role_tx, and any existing buffer length accessor on leader.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 32-35: Update the "TBD" performance report entry in CHANGELOG.md
under the "Drain-based batch architecture" (`#266`) section with actual benchmark
results before merging to main and publishing v0.2.3; run the representative
benchmarks used for this change, summarize key metrics (latency under low load,
throughput under high load, and any comparative numbers versus the
timeout-driven batching), and replace "Performance report: TBD" with a concise
bullet containing those results and a link or note pointing to the full
benchmark data/report.
In `@config/base/raft.toml`:
- Around line 16-35: The comment in [raft.batching] claims "Balanced: 1000
(default...)" but the actual setting max_batch_size = 100; update either the
configuration or the comment to remove the mismatch: either set max_batch_size
to 1000 to match the "Balanced/default" guidance, or change the comment text to
state that the base config uses 100 for lower-latency operation (and adjust the
guidance line to e.g. "Balanced: 1000 (recommended for throughput)"). Ensure you
update the reference to max_batch_size and the surrounding guidance in the
boilerplate so the default and documentation are consistent.
In `@d-engine-core/src/raft_role/leader_state_test/client_read_test.rs`:
- Around line 1653-1682: The timing assertion using elapsed.as_millis() < 10 in
the client_read_test is flaky on CI; either relax the bound (e.g., to a larger,
conservative value like 100–200ms) or convert the test to use deterministic time
control (pause the Tokio clock and advance it) so that
state.flush_cmd_buffers(&ctx, &role_tx).await is tested without relying on real
wall-clock timing; update the assertion around elapsed and/or add
tokio::time::pause()/advance() calls and ensure you still verify
state.linearizable_read_buffer.len() == 1 and rx.recv().await.is_ok() after
flush_cmd_buffers.
In `@d-engine-core/src/raft_role/leader_state.rs`:
- Around line 2968-2978: When calling verify_leadership_and_refresh_lease(...)
before serving the read, ensure failures notify the client: capture the Result,
and on Err(e) send an error response via the existing sender (e.g.,
sender.send(Err(...)) or sender.send(Ok(ClientResponse::error(...)))) before
returning the Err; on Ok continue to call
ctx.handlers.state_machine_handler.read_from_state_machine and send the normal
ClientResponse. Update the block around verify_leadership_and_refresh_lease,
sender, and ClientResponse to handle the error path and guarantee the sender is
always notified.
- Around line 209-211: The modulo using sample_rate can panic if sample_rate is
0; update validation and add a guard to avoid division-by-zero: ensure
sample_rate is validated to be >0 in the constructor/initializer that sets
sample_rate (or replace zero with a safe default), and modify the sampling block
that reads self.enabled, self.sample_counter.fetch_add(...), and does counter %
self.sample_rate to either early-return when sample_rate == 0 or use a checked
branch (e.g., only perform the modulo when self.sample_rate > 0) so
sample_counter/sample_rate logic cannot cause a runtime panic.
- Around line 1442-1454: When handling RaftEvent::FatalError in leader_state
(the match arm that currently drains pending_requests), also drain and notify
all entries in pending_reads so linearizable reads awaiting ApplyCompleted don't
hang; collect pending_reads via self.pending_reads.drain().collect(), iterate
each (_key, responder) and send an Err(tonic::Status::internal(format!("Node
fatal error: {error}"))) (matching the pending_requests logic), then proceed to
return the existing crate::Error::Fatal.
---
Outside diff comments:
In `@d-engine-core/src/config/raft.rs`:
- Around line 142-165: Add a validation check to ensure metrics.sample_rate is >
0 and invoke it from RaftConfig::validate: implement a small validate() method
on the MetricsConfig (or add logic in an existing metrics.validate()) that
returns an Err(Error::Config(...)) when sample_rate == 0, and then call this new
metrics.validate() from RaftConfig::validate() alongside the other subsystem
validates (e.g., replication.validate(), batching.validate(), etc.) so invalid
zero sample_rate configs are rejected early.
In `@d-engine-core/src/raft_role/learner_state.rs`:
- Around line 786-794: When converting a FollowerState to a LearnerState in the
impl From<&FollowerState<T>> for LearnerState<T>, preserve the follower's purge
history by copying its last_purged_index into the new LearnerState instead of
resetting it to None; locate the constructor for LearnerState in the
from(follower_state: &FollowerState<T>) -> Self and set last_purged_index to the
follower_state.last_purged_index (cloning or copying as appropriate) so
monotonic purge tracking is maintained after demotion.
---
Nitpick comments:
In @.github/workflows/commit-message-check.yml:
- Line 11: The workflow contains a conditional guard "if: github.head_ref !=
'develop'" that is intended to skip commit-message checks for develop→main PRs
but will become dead code once the develop branch is retired; remove this
conditional from .github/workflows/commit-message-check.yml after develop is
deleted, and add a follow-up task or issue to track the removal so the workflow
no longer contains the unused "if: github.head_ref != 'develop'" check.
In `@benches/embedded-bench/src/main.rs`:
- Around line 466-467: The hardcoded signal file path (done_signal_path =
"/tmp/embedded-bench-done") can collide across concurrent runs; change it to be
configurable and/or unique per run by reading a CLI option or environment
variable (e.g., add a CLI flag parsed in main or the existing arg parsing code
to accept --done-signal) and defaulting to a generated path that includes the
PID or timestamp (or both) when the flag is not provided; update all references
to done_signal_path in main.rs so they use the new configurable variable (the
original symbol done_signal_path) and ensure cleanup logic still removes the
generated file on exit.
In `@d-engine-client/src/lib.rs`:
- Around line 165-168: Change the public function signature of builder from
returning ClientBuilder to Result<ClientBuilder, ClientApiError> and stop using
assert!; instead check endpoints.is_empty() and return
Err(ClientApiError::InvalidArguments("empty endpoints".into())) (or add a
specific ClientApiError::EmptyEndpoints variant) when empty, otherwise return
Ok(ClientBuilder::new(endpoints)); update the builder function, the
ClientApiError enum to include the chosen variant, adjust docs to reflect the
Result return, and fix/update any call sites/tests that expect the old panicking
behavior to handle the Result.
In `@d-engine-client/src/mock_rpc_service.rs`:
- Around line 203-214: Extract the duplicated ClusterMembership construction
into a single helper function (e.g., fn default_single_leader_membership(port:
u16) -> ClusterMembership) that returns the ClusterMembership with NodeMeta,
NodeRole::Leader, address format!("127.0.0.1:{port}"), and NodeStatus::Active,
then replace the repeated Box::new(|port: u16| { Ok(ClusterMembership { ... })
}) closures in simulate_client_read_mock_server,
simulate_client_write_mock_server, and simulate_cas_mock_server with
Box::new(|port| Ok(default_single_leader_membership(port))) so all three use the
shared builder.
In `@d-engine-core/src/commit_handler/default_commit_handler_test.rs`:
- Around line 102-107: The mock implementation of mock_smh.expect_apply_chunk
currently always returns an empty Vec which breaks
ApplyCompleted/pending_requests semantics; modify the returning closure used in
mock_smh.expect_apply_chunk() (the closure that calls command_hook()) to produce
a Vec whose length matches the number of entries in the applied command batch
(e.g., map the input batch to a Vec of dummy Ok results or appropriate Error
values per entry) so ApplyCompleted receives per-entry results and cardinality
mismatches are surfaced; ensure the closure still respects the command_hook()
branch to return Err(Error::Fatal(...)) when needed.
- Around line 147-176: The SM worker in run_handler is currently just draining
sm_apply_rx so tests never exercise apply-result or error paths; change the
spawned task to call the mock state machine's apply_chunk (mock_smh.apply_chunk)
for each received entry batch and forward the resulting Ok/Err back to the
handler path (so apply-result/fatal-error propagation is exercised), and make
max_batch_size configurable by passing it through setup_harness into
CommitHandlerDependencies.max_batch_size instead of hardcoding 10 so batching
tests can vary it; update both run_handler and the similar worker block at
182-209 to use mock_smh.apply_chunk and respect the injected max_batch_size.
In `@d-engine-core/src/commit_handler/default_commit_handler.rs`:
- Around line 100-101: Add a trace-level log right before breaking out of the
drain loop when the batch limit is reached: in default_commit_handler.rs, locate
the drain loop's match arm that currently reads "Err(_) => break" and replace it
with a block that logs a trace including max_batch_size and the current batch
count/context (use the existing batch variable or counter visible in that scope)
and then breaks; use the module's logger (or trace! macro) so
backpressure/high-load events are recorded for debugging.
In `@d-engine-core/src/event.rs`:
- Around line 221-228: The match arms that convert
RaftEvent::StepDownSelfRemoved and RaftEvent::MembershipApplied currently return
TestEvent::CreateSnapshotEvent as a silent placeholder; update the conversion
(the match in the Raft->TestEvent mapping function) to return explicit
test-visible variants instead—either add dedicated
TestEvent::StepDownSelfRemoved and TestEvent::MembershipApplied variants to the
TestEvent enum and map those RaftEvent arms to them, or introduce a generic
TestEvent::Placeholder(RaftEvent) or TestEvent::Internal(RaftEvent) variant and
map both RaftEvent::StepDownSelfRemoved and RaftEvent::MembershipApplied to that
so tests fail loudly if these events are emitted unexpectedly; ensure you update
the TestEvent enum declaration and any pattern matches that assume
CreateSnapshotEvent was used as the placeholder.
In `@d-engine-core/src/raft_role/buffers/batch_buffer_test.rs`:
- Around line 1-4: The test imports tokio::time::Instant but the tests use
standard Instant semantics; remove the tokio import and switch to
std::time::Instant (or replace the use sites to explicitly reference
std::time::Instant) so the import matches usage; update the use list at the top
(remove "use tokio::time::Instant" and ensure "use std::time::Instant" is
present) in batch_buffer_test.rs where BatchBuffer and Instant are referenced.
- Around line 18-20: The timing assertions in batch_buffer_test.rs (tests
test_new_initialization, test_take_all_resets_buffer,
test_take_all_resets_last_flush_timer) are flaky; update the assertions that
check buffer.last_flush.elapsed() against tight Durations (50ms, 100ms) by
either increasing the thresholds to a more generous value (e.g., several hundred
ms) or by avoiding wall-clock checks entirely: inject or mock the clock used by
BatchBuffer (refer to buffer.last_flush and any creation path of BatchBuffer)
and assert using the mocked time, or assert relative ordering (e.g., last_flush
updated after take_all) instead of absolute elapsed < X; ensure all references
to Duration comparisons in these tests are changed consistently.
- Around line 13-21: The test currently reaches into BatchBuffer's internals
(buffer.buffer and last_flush); change it to use only the public API: assert
buffer.is_empty() (or buffer.len() == 0) instead of buffer.buffer.is_empty(),
call buffer.take_all() and assert the returned Vec's capacity() >=
initial_capacity instead of inspecting buffer.buffer.capacity(), and stop
accessing last_flush directly—either remove that direct assertion or replace it
with a call to a public accessor (e.g., add/get_last_flush or
elapsed_since_last_flush) and assert via that method; update references to
BatchBuffer::<TestRequest>::new, is_empty(), len(), and take_all() in the test
accordingly.
In `@d-engine-core/src/raft_role/buffers/batch_buffer.rs`:
- Around line 46-63: The batch.buffer_length gauge is only updated in push(), so
after take_all() drains the buffer the metric stays at the last value; modify
take_all() (the method that empties the internal buffer) to check
metrics_enabled and metrics_labels and reset the gauge to 0 (e.g., call
metrics::gauge!("batch.buffer_length", labels.as_ref()).set(0.0)) after clearing
or swapping out self.buffer so the metric reflects an empty buffer; ensure you
use the same labels handling as in push() to avoid panics when metrics_labels is
None.
- Around line 75-77: Add the #[must_use] attribute to the len method to catch
accidental ignored return values; locate the pub fn len(&self) -> usize { ... }
implementation in the BatchBuffer (buffers/batch_buffer.rs) and place
#[must_use] directly above that function signature so callers who ignore the
returned usize will get a compiler warning.
In `@d-engine-core/src/raft_role/buffers/propose_batch_buffer_test.rs`:
- Around line 98-110: The timing assertion in the test
flush_resets_last_flush_timestamp uses a too-tight threshold; update the
assertion in the test function to use a more generous duration (e.g.,
Duration::from_millis(500) or Duration::from_secs(1)) so
buf.last_flush.elapsed() is compared against that larger threshold, keeping the
test's intent (that ProposeBatchBuffer::flush updates last_flush) while avoiding
CI flakiness.
In `@d-engine-core/src/raft_role/buffers/propose_batch_buffer.rs`:
- Around line 139-141: Add the #[must_use] attribute to the len() method on the
propose batch buffer so callers are warned when its returned usize is ignored;
locate the pub fn len(&self) -> usize { self.payloads.len() } definition (in
propose_batch_buffer.rs) and place #[must_use] immediately above that function
signature, mirroring the BatchBuffer implementation.
- Around line 106-133: The flush() method currently swaps out payloads/senders
but does not reset the monitoring gauge that tracks the buffer size (same gauge
used in BatchBuffer), so the metric remains at the previous batch size; after
performing the swaps in propose_batch_buffer::flush (i.e., immediately after the
std::mem::swap calls and before returning Some(...)), set the buffer-size gauge
to 0 (the same gauge you use in BatchBuffer) so monitoring reflects the empty
buffer; keep the reset adjacent to the swaps and ensure it runs in both the
empty-path (if any) and the non-empty path.
In `@d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs`:
- Around line 299-300: The test directly mutates internal state via
leader.lease_read_queue.clear(), which couples tests to implementation; add a
small public test-only helper such as Leader::clear_buffers() (or similar) and
call that from backpressure_test instead of touching lease_read_queue directly
to improve maintainability and resilience to internal changes, or alternatively
split the test into separate functions so shared state isn't reset mid-test;
update references in the test from leader.lease_read_queue.clear() to
leader.clear_buffers() (or the chosen helper) and keep the helper limited to
cfg(test) or pub(crate) test visibility to avoid exposing internals in
production.
In `@d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs`:
- Around line 148-165: Update the doc comment to match the actual test
implementation: replace the SnapshotCreated/transport-error/purge-peers
description with a clear description for test_handle_log_purge_completed that
states it verifies handling of LogPurgeCompleted updates last_purged_index,
ensures no role transitions are emitted, and resets snapshot_in_progress as
appropriate; reference the test function name test_handle_log_purge_completed
and remove any references to sending purge requests or transport errors.
In `@d-engine-core/src/raft_test/drain_based_batch_architecture_tests.rs`:
- Around line 713-730: The current assertion is trivial because flush_count is
always incremented; instead, verify actual work was done by checking flush
results or buffer state: call leader.flush_cmd_buffers(&raft.ctx, &role_tx) and
capture its return (e.g., processed count or bool), sum those to a
processed_commands counter, or query buffer length via a method like
leader.cmd_buffer_len(&raft.ctx) before/after the loop and assert it decreased
(or reached zero). Update the test to assert processed_commands > 0 or that
leader.cmd_buffer_len changed, using the symbols flush_cmd_buffers, raft.ctx,
role_tx, and any existing buffer length accessor on leader.
In `@d-engine-core/src/raft_test/leader_change_tests.rs`:
- Around line 34-52: The test name and comment claim to verify broadcasting but
the code creates two independent channels (tx1/tx2 with rx1/rx2) and sends to
each separately; update the test to match intent by either renaming the test
from test_multiple_listeners to something like
test_multiple_independent_listeners and change the comment to “Test that
multiple independent notification channels each receive their own messages” or,
if you intended to test broadcast semantics, replace the two separate channels
with a single broadcast mechanism (e.g., use a shared broadcaster/cloneable
receiver) and send one message to ensure both listeners (rx1, rx2) receive the
same broadcast; refer to the test function name test_multiple_listeners and the
channel variables tx1, tx2, rx1, rx2 when making the change.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
d-engine-core/src/raft_role/raft_role_test.rs (1)
292-308:⚠️ Potential issue | 🟡 MinorPotential flaky test: thread 0 setting leader to 0 can cause assertion failure.
The loop spawns threads with
ivalues 0–9. Wheni=0, thread 0 callsset_current_leader(0). Per the semantics established intest_shared_state_leader_zero_means_none(lines 244-259), setting leader to 0 is equivalent to clearing the leader, causingcurrent_leader()to returnNone.If thread 0's write happens to be the last one observed,
final_leader.is_some()at line 307 will fail.Proposed fix: start thread index from 1
// Simulate concurrent leader updates - let handles: Vec<_> = (0..10) + let handles: Vec<_> = (1..=10) .map(|i| { let shared = Arc::clone(&shared); thread::spawn(move || { shared.set_current_leader(i as u32); }) }) .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/raft_role_test.rs` around lines 292 - 308, The test is flaky because one thread sets leader to 0 (which means "none"); change the thread spawn range so no thread uses 0 (e.g., replace (0..10) with (1..10) or (1..=10)) so all calls to shared.set_current_leader(...) use a nonzero leader; keep the rest of the join and assertions using shared.current_leader() and the existing semantics from test_shared_state_leader_zero_means_none intact.d-engine-core/src/raft_role/follower_state.rs (1)
336-345:⚠️ Potential issue | 🟡 MinorFix mismatched error context for FlushReadBuffer.
The RoleViolation message says “attempted to create snapshot,” but the event is FlushReadBuffer, which will confuse logs and callers.
🛠️ Suggested fix
- context: format!( - "Follower node {} attempted to create snapshot.", - ctx.node_id - ), + context: format!( + "Follower node {} attempted to flush read buffer.", + ctx.node_id + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/follower_state.rs` around lines 336 - 345, The RoleViolation raised for RaftEvent::FlushReadBuffer has the wrong context text ("attempted to create snapshot"); update the ConsensusError::RoleViolation context string to accurately describe the event (e.g., "Follower node {ctx.node_id} attempted to flush read buffer" or mention FlushReadBuffer) so logs and callers reflect the actual operation; locate the error construction in follower_state.rs where RaftEvent::FlushReadBuffer is handled and replace the snapshot message with a flush/read-buffer-specific message while keeping current_role and required_role values intact.d-engine-core/src/raft.rs (1)
261-333:⚠️ Potential issue | 🟠 MajorClient-command branch can starve Raft events under load.
With
tokio::select! { biased; ... P3 cmd_rx ... P4 event_rx }, under sustained client load the constantly-ready cmd_rx channel will be polled first every iteration, blocking event_rx from being processed. This delays AppendEntries responses and role transitions.Reorder P3 and P4 to prioritize Raft events, or remove the
biased;keyword entirely to allow probabilistic fairness across P2–P4 (keeping P0 shutdown and P1 tick as true priorities).🔧 Reordering option
- // P3: Client commands (drain-driven batch with RPC merge) - Some(first_cmd) = self.cmd_rx.recv() => { - trace!(%self.node_id, "receive first client command"); - ... - } - - // P4: Other events - Some(raft_event) = self.event_rx.recv() => { + // P3: Other events + Some(raft_event) = self.event_rx.recv() => { trace!(%self.node_id, ?raft_event, "receive raft event"); ... } + + // P4: Client commands (drain-driven batch with RPC merge) + Some(first_cmd) = self.cmd_rx.recv() => { + trace!(%self.node_id, "receive first client command"); + ... + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft.rs` around lines 261 - 333, The biased tokio::select! with the client-command branch (self.cmd_rx.recv) placed before raft events (self.event_rx.recv) causes starvation of raft_event handling under sustained client load; either remove the biased; keyword or reorder the branches so that the raft event branch (Some(raft_event) = self.event_rx.recv()) is evaluated before the client command branch (Some(first_cmd) = self.cmd_rx.recv()), keeping shutdown (self.shutdown_signal.changed()) and tick handling (tick / self.role.tick) as top priorities; update any comments and ensure related logic (draining via self.cmd_rx.try_recv(), push_client_cmd, flush_cmd_buffers, and handle_raft_event) remains correct after reordering.d-engine-core/src/raft_role/learner_state.rs (1)
786-795:⚠️ Potential issue | 🟠 MajorDon’t drop purge metadata when converting Follower → Learner.
Resetting
last_purged_indextoNoneloses monotonic purge state and can re‑enable old purges or regress safety checks.✅ Proposed fix
- last_purged_index: None, //TODO + last_purged_index: follower_state.last_purged_index,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/learner_state.rs` around lines 786 - 795, The conversion impl From<&FollowerState<T>> for LearnerState<T> currently resets last_purged_index to None, which drops purge metadata; instead preserve the follower's purge state by initializing LearnerState.last_purged_index from follower_state.last_purged_index (e.g., clone or copy the Option value) so monotonic purge information is retained when creating a Learner from a Follower.
🧹 Nitpick comments (23)
.github/workflows/commit-message-check.yml (1)
11-11: LGTM — consider adding a brief comment for future context.The condition correctly skips commit message validation for the
developbranch merge, aligning with the objective of retiringdevelop. For future maintainability, consider adding a comment explaining the exclusion:📝 Optional: Add explanatory comment
jobs: validate-commits: + # Skip validation for develop→main merge (develop branch retired in v0.2.3) if: github.head_ref != 'develop' runs-on: ubuntu-latest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/commit-message-check.yml at line 11, Add a one-line explanatory comment above the conditional "if: github.head_ref != 'develop'" in the workflow to clarify that commit message validation is intentionally skipped for merges from the retired "develop" branch; reference the condition string github.head_ref != 'develop' so reviewers can easily find and understand the reason for the exception.CONTRIBUTING.md (1)
80-88: Consider clarifying the "upstream" remote setup.The rebase workflow assumes contributors have configured the
upstreamremote, but the Quick Setup section doesn't explain how to set it up. First-time contributors working from a fork may be confused whengit fetch upstreamfails.📝 Suggested improvement
Add a brief note about setting up the upstream remote for forked repositories:
### Keeping Your PR Up-to-Date -If `main` has moved forward since you created your branch, rebase before requesting review: +If `main` has moved forward since you created your branch, rebase before requesting review. + +For forked repositories, first ensure you have the upstream remote configured: + +```bash +git remote add upstream https://github.com/deventlab/d-engine.git +``` + +Then rebase: ```bash git fetch upstream git rebase upstream/main git push --force-with-lease</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@CONTRIBUTING.mdaround lines 80 - 88, Update the "Keeping Your PR
Up-to-Date" section to note that contributors working from forks must configure
an upstream remote first; add a short sentence instructing users to add an
upstream remote pointing at the canonical repo (for example upstream ->
https://github.com/deventlab/d-engine.git) and then run the existing
fetch/rebase/push sequence, and include a one-line example showing the git
remote add command and the example URL so first-time contributors know how to
set up upstream before running git fetch upstream.</details> </blockquote></details> <details> <summary>d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (1)</summary><blockquote> `1143-1151`: **Misleading function name: `zero_copy_bytes_from_mmap` actually performs a copy.** `Bytes::copy_from_slice(slice)` at line 1150 allocates a new buffer and copies the data. This contradicts the "zero_copy" naming and the "ZERO-COPY" comments throughout the file (lines 554, 601, 1079, 1110). For true zero-copy semantics, you would need to use `Bytes::from_owner` (or similar) to create `Bytes` that keeps the `Arc<Mmap>` alive and references the mapped memory directly. <details> <summary>Consider renaming or implementing actual zero-copy</summary> **Option 1: Rename to reflect actual behavior:** ```diff -fn zero_copy_bytes_from_mmap( +fn bytes_from_mmap( mmap_arc: Arc<Mmap>, start: usize, end: usize, ) -> Bytes { - // Get a slice of the memory map let slice = &mmap_arc[start..end]; Bytes::copy_from_slice(slice) }Option 2: Implement actual zero-copy (requires
bytescrate feature):fn zero_copy_bytes_from_mmap( mmap_arc: Arc<Mmap>, start: usize, end: usize, ) -> Bytes { // True zero-copy: Bytes holds Arc<Mmap> and references slice directly Bytes::from_owner(mmap_arc).slice(start..end) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/state_machine_handler/default_state_machine_handler.rs` around lines 1143 - 1151, The function zero_copy_bytes_from_mmap currently copies data via Bytes::copy_from_slice, which contradicts the "zero-copy" name and comments; either rename the function to indicate it copies (e.g., bytes_from_mmap_copy) or change implementation to true zero-copy by creating Bytes that owns the Arc<Mmap> (use Bytes::from_owner(mmap_arc) and then .slice(start..end) or equivalent) so the Arc<Mmap> is kept alive and the returned Bytes references the mmap directly; update callers of zero_copy_bytes_from_mmap or its name accordingly to maintain correct ownership and semantics..github/workflows/dependency-audit.yml (1)
15-16: Consider pinning the audit job to the workspace MSRV for reproducibility.
Using floatingstablecan change audit results over time; pinning to 1.88 would match the workspace rust-version.♻️ Suggested update
env: - RUSTUP_TOOLCHAIN: stable + RUSTUP_TOOLCHAIN: "1.88.0"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/dependency-audit.yml around lines 15 - 16, The workflow sets RUSTUP_TOOLCHAIN to the floating value "stable" which can change audit results; update the env key RUSTUP_TOOLCHAIN in the dependency-audit job to pin it to the workspace MSRV "1.88" (replace "stable" with "1.88") so the audit runs use the same rust-version as the workspace.d-engine-core/src/commit_handler/default_commit_handler.rs (1)
164-199: Clarify:command_batchcontains all entry types, not just commands.The batch includes config entries (line 173) and noop entries (line 184) alongside command entries. The variable name
command_batchis misleading since it's actually an "entries batch" sent to the SM Worker.Consider renaming for clarity:
♻️ Optional rename for clarity
- let mut command_batch = vec![]; + let mut entries_batch = vec![];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/commit_handler/default_commit_handler.rs` around lines 164 - 199, Rename the misleading variable command_batch (used in the for loop that collects entries and then passed to send_to_sm_worker) to something like entries_batch or entry_batch to reflect that it contains Command, Config, and Noop payloads; update all references including its declaration (let mut command_batch = vec![]), pushes (command_batch.push(...)), and the final send_to_sm_worker(&mut command_batch).await? and any clones/uses inside apply_config_change handling so names remain consistent and clear.d-engine-core/src/raft_role/buffers/batch_buffer.rs (2)
57-62: Metrics gauge not reset aftertake_all()— buffer appears non-empty until next push.When
take_all()drains the buffer, the gauge still reflects the old length until anotherpush()occurs. Consider emitting a zero-length metric intake_all():🔧 Proposed fix
pub fn take_all(&mut self) -> Vec<E> { self.last_flush = Instant::now(); + if self.metrics_enabled { + if let Some(ref labels) = self.metrics_labels { + metrics::gauge!("batch.buffer_length", labels.as_ref()).set(0.0); + } + } std::mem::take(&mut self.buffer) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/batch_buffer.rs` around lines 57 - 62, When draining the buffer in take_all(), the metrics gauge "batch.buffer_length" isn't updated and still shows the previous length until the next push; modify the take_all() implementation to, after clearing/draining self.buffer, check self.metrics_enabled and self.metrics_labels and call metrics::gauge!("batch.buffer_length", labels.as_ref()).set(0.0) (using the same labels logic as in push()) so the gauge is immediately reset to zero; ensure you reference the existing fields self.metrics_enabled, self.metrics_labels and the gauge name "batch.buffer_length" to match the push() behavior.
65-69:take_all()resets capacity — considermem::swapif allocation overhead matters.
mem::takereplaces the buffer with a zero-capacityVec, so subsequent pushes will reallocate. If this buffer is flushed frequently in hot paths (like the linearizable read buffer), consider usingmem::swapwith a pre-allocated spare vector to retain capacity across flushes, similar toProposeBatchBuffer:♻️ Optional: retain capacity via mem::swap
+ spare: Vec<E>, // Add field to struct + pub fn take_all(&mut self) -> Vec<E> { self.last_flush = Instant::now(); - std::mem::take(&mut self.buffer) + std::mem::swap(&mut self.buffer, &mut self.spare); + self.spare.clear(); + std::mem::take(&mut self.spare) }This is optional depending on allocation sensitivity for the linearizable read path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/batch_buffer.rs` around lines 65 - 69, take_all currently uses std::mem::take which drops capacity and forces reallocation on next push; change take_all in BatchBuffer to swap the internal buffer with an empty-but-capacity-retaining spare Vec so capacity is preserved: add a spare Vec<E> field (or create a local Vec with capacity reserve stored on the struct like ProposeBatchBuffer), call std::mem::swap(&mut self.buffer, &mut self.spare) (update self.last_flush) and return the drained buffer (the previous buffer now in spare) so future pushes reuse capacity instead of reallocating.d-engine-core/src/client/client_api_error.rs (2)
57-99: Consider removing commented-out code.These commented-out enum definitions (
NetworkErrorType,ProtocolErrorType, etc.) appear to be dead code. If they're no longer needed, consider removing them to reduce noise. If they represent future work, consider tracking them in an issue instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/client/client_api_error.rs` around lines 57 - 99, The file contains several commented-out enum definitions (NetworkErrorType, ProtocolErrorType, StorageErrorType, BusinessErrorType, GeneralErrorType) that are dead/commented code; remove these commented blocks from client_api_error.rs to reduce noise, or if they represent planned work, move them to an issue or a feature branch and reference that issue ID in a short comment; ensure you delete the commented enum definitions (NetworkErrorType, ProtocolErrorType, StorageErrorType, BusinessErrorType, GeneralErrorType) rather than leaving them commented in the file.
386-395: Error context is discarded inFromimplementations.Both
From<JoinError>andFrom<std::io::Error>implementations ignore the original error (_err), losing valuable debugging context. Consider preserving the error message:♻️ Proposed fix to preserve error context
impl From<JoinError> for ClientApiError { - fn from(_err: JoinError) -> Self { - ErrorCode::JoinError.into() + fn from(err: JoinError) -> Self { + ClientApiError::Network { + code: ErrorCode::JoinError, + message: format!("Task join error: {err}"), + retry_after_ms: Some(100), + leader_hint: None, + } } } impl From<std::io::Error> for ClientApiError { - fn from(_err: std::io::Error) -> Self { - ErrorCode::StorageIoError.into() + fn from(err: std::io::Error) -> Self { + ClientApiError::Storage { + code: ErrorCode::StorageIoError, + message: format!("Storage I/O error: {err}"), + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/client/client_api_error.rs` around lines 386 - 395, The current From implementations for JoinError and std::io::Error in ClientApiError discard the original error (_err); change them to capture and propagate the original error message or source instead of ignoring it: in the impl From<JoinError> for ClientApiError and impl From<std::io::Error> for ClientApiError use the err parameter (not _err) and construct a ClientApiError that includes the underlying error (e.g., by calling whichever constructor/pattern your type exposes to attach a message or source—such as building from ErrorCode::JoinError/ErrorCode::StorageIoError plus err.to_string() or wrapping err as the source via Box::new(err) or a .with_source(...) helper) so the original error context is preserved for debugging.benches/embedded-bench/src/main.rs (2)
466-467: Hardcoded signal path limits distributed benchmark scenarios.The coordination file at
/tmp/embedded-bench-doneonly works when all nodes run on the same machine. For multi-machine benchmarks, consider:
- Making the path configurable via CLI argument
- Using a network-based coordination mechanism (e.g., TCP ping or shared storage)
For single-machine testing, this approach is pragmatic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/embedded-bench/src/main.rs` around lines 466 - 467, The hardcoded done_signal_path variable prevents multi-machine benchmarks; make the signal path configurable by adding a CLI option or environment override and use it instead of the literal "/tmp/embedded-bench-done" (modify where done_signal_path is declared/used in main.rs), and optionally document or gate an alternative network-based coordination mode (e.g., a --coordination-mode flag to switch to TCP/HTTP ping or shared storage) so distributed runs can use a network mechanism while single-machine tests keep the file-signal behavior.
494-504: Signal file not cleaned up after follower reads it.After the follower detects the done signal and exits the loop, the signal file remains on disk. Consider removing it to ensure clean state for subsequent runs:
♻️ Suggested cleanup
if std::path::Path::new(done_signal_path).exists() { println!("Benchmark completed, shutting down follower."); + let _ = std::fs::remove_file(done_signal_path); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/embedded-bench/src/main.rs` around lines 494 - 504, The follower loop detects the done signal via done_signal_path but never removes that file, leaving state between runs; update the loop inside the tokio::select (the block using shutdown_rx.changed() and tokio::time::sleep) so that when Path::new(done_signal_path).exists() is true you attempt to delete the file (std::fs::remove_file(done_signal_path)) before breaking out, handle and log any remove_file error (or ignore non-fatal ones) and then break to ensure a clean state for subsequent runs; reference the existing done_signal_path variable and the select loop where the detection happens.d-engine-core/src/raft_test/leader_discovered_tests.rs (1)
67-99: Consider simplifying the receiver setup.The pattern at lines 74-75 creates an unused receiver
_rximmediately before subscribing viatx.subscribe(). While functional, this could be simplified:- let (tx, _rx) = watch::channel(None); - raft.register_leader_change_listener(tx.clone()); - - // Create multiple subscribers - let mut rx1 = tx.subscribe(); - let mut rx2 = tx.subscribe(); + let (tx, mut rx1) = watch::channel(None); + raft.register_leader_change_listener(tx.clone()); + let mut rx2 = tx.subscribe();This reuses the initial receiver instead of discarding it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_test/leader_discovered_tests.rs` around lines 67 - 99, In test_leader_discovered_multiple_listeners, avoid creating the unused receiver `_rx` from watch::channel before calling tx.subscribe(); instead keep the channel's returned receiver as the first subscriber (use the receiver returned by watch::channel as rx1) and then call tx.subscribe() only to create additional subscribers (e.g., rx2); update references to use that receiver when asserting notifications after calling raft.handle_role_event and leave register_leader_change_listener(tx.clone()) and the rest of the test unchanged.d-engine-core/src/raft_role/buffers/batch_buffer_test.rs (2)
126-144: Timing assertion may be flaky under CI load.The assertion at line 143 allows only 10ms tolerance (
after_take + Duration::from_millis(10)). Under heavy CI load, this could intermittently fail. Consider increasing the tolerance or usingtokio::time::pause()with#[tokio::test]for deterministic timing control.♻️ Suggested approach for deterministic timing
-#[test] -fn test_take_all_resets_last_flush_timer() { +#[tokio::test] +async fn test_take_all_resets_last_flush_timer() { + tokio::time::pause(); let mut buffer = BatchBuffer::<TestRequest>::new(2); buffer.push(create_test_request()); let before_take = Instant::now(); buffer.take_all(); let after_take = Instant::now(); let elapsed = buffer.last_flush.elapsed(); assert!( - elapsed < Duration::from_millis(100), + elapsed < Duration::from_millis(10), "last_flush not reset properly" ); assert!(buffer.last_flush >= before_take); - assert!(buffer.last_flush <= after_take + Duration::from_millis(10)); + assert!(buffer.last_flush <= after_take); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/batch_buffer_test.rs` around lines 126 - 144, The timing assertion in test_take_all_resets_last_flush_timer is too tight and can flake on CI; update the test to relax the upper-tolerance or make timing deterministic: either increase the tolerance on the assert that compares buffer.last_flush to after_take (e.g., raise the Duration from_millis(10) to a larger value like 50–200ms) or convert the test to a deterministic tokio test using #[tokio::test] and tokio::time::pause() so that BatchBuffer::last_flush and Instant comparisons are controlled; update references to BatchBuffer, last_flush, before_take/after_take, and the assert that uses after_take + Duration::from_millis(10) accordingly.
13-21: Consider using public API methods instead of direct field access.The test accesses internal fields (
buffer.buffer,buffer.last_flush) directly. While this works for unit tests in the same crate, it couples tests to implementation details. Consider using only public methods (is_empty(),len()) where possible to make tests more resilient to refactoring.That said, for white-box unit tests verifying internal invariants (like capacity allocation), direct access is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/batch_buffer_test.rs` around lines 13 - 21, The test currently reads internal fields (buffer.buffer and buffer.last_flush) of BatchBuffer<TestRequest>; change the assertions to use the public API (e.g., call buffer.is_empty() and buffer.len() instead of buffer.buffer.is_empty() and buffer.buffer.capacity()); for the capacity or timestamp checks either rely on a public accessor (add pub fn capacity(&self) -> usize or pub fn last_flush_age(&self) -> Duration on BatchBuffer) and assert against that, or remove the implementation-only assertion—update the test to reference BatchBuffer::is_empty(), BatchBuffer::len(), and any newly added accessor methods instead of direct field access.d-engine-core/src/client/client_api_error_test.rs (1)
1-6: Minor: Redundant#[cfg(test)]attribute.The file is already conditionally included via
#[cfg(test)] mod client_api_error_test;inmod.rs, so the inner#[cfg(test)]at line 3 is redundant. It's harmless but adds unnecessary nesting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/client/client_api_error_test.rs` around lines 1 - 6, Remove the redundant #[cfg(test)] attribute on the inner module declaration client_api_error_tests; since the test module is already conditionally included by the outer mod declaration in mod.rs, delete the #[cfg(test)] line so the module reads simply "mod client_api_error_tests { ... }" and leave the uses (ClientApiError, ClientApiResult, ErrorCode) and tests intact.d-engine-core/src/raft_role/leader_state_test/event_handling_test.rs (1)
599-604: Don’t swallowflush_cmd_bufferserrors in failure-path tests.
await.ok()hides regressions. Preferexpect_err(and optionally assert the error variant) so the test fails if the flush unexpectedly succeeds. This applies to both failure-path tests.✅ Suggested adjustment (apply to both occurrences)
- state.flush_cmd_buffers(&context, &role_tx).await.ok(); + let _err = state + .flush_cmd_buffers(&context, &role_tx) + .await + .expect_err("expected flush_cmd_buffers to fail");Also applies to: 805-810
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state_test/event_handling_test.rs` around lines 599 - 604, The test currently swallows errors by calling state.flush_cmd_buffers(&context, &role_tx).await.ok(); — change both occurrences to assert that the call fails, e.g. use state.flush_cmd_buffers(&context, &role_tx).await.expect_err("flush should fail on quorum verification") and optionally match the returned error variant to ensure the expected failure path; update the two failure-path tests around the push_client_cmd/flush_cmd_buffers sequence (and the later similar block at the other occurrence) to use expect_err and assert the error variant rather than calling .ok().d-engine-core/src/raft_test/raft_comprehensive_tests.rs (1)
88-1999: Many tests are placeholders without assertions.A large portion of this module only performs setup/role transitions and comments on expected behavior without verifying it. These tests won’t catch regressions. Consider adding concrete assertions (e.g., listener notifications, state changes, ordering) or consolidating into fewer scenario-driven tests with real checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_test/raft_comprehensive_tests.rs` around lines 88 - 1999, Many tests in raft_comprehensive_tests.rs are only performing transitions and comments without concrete assertions; update the tests to assert observable outcomes by (1) checking role/state helpers (is_follower/is_candidate/is_leader/is_learner) after every transition, (2) for tests that register listeners use the registered channels (e.g., register_leader_change_listener, register_new_commit_listener, register_role_transition_listener) to await and assert expected notifications, (3) for leader initialization and verification tests assert peer index/cache initialization by inspecting leader state via methods/fields touched during BecomeLeader (e.g., next_index, match_index, metadata cache accessors or prepare_succeed_majority_confirmation mocks), and (4) for failure-path tests assert the role downgrades (handle_role_event results and is_follower) and that replication_handler mocks were invoked as expected; apply changes to the named test functions (e.g., test_become_leader_full_initialization, test_leader_verification_fails_downgrades, test_leader_init_* , test_notify_new_commit_index_broadcasts, etc.) so each test contains at least one concrete assertion that would fail on regression.d-engine-client/src/grpc_client.rs (1)
260-262: Redundant type conversion.The conversion
Into::<ClientApiError>::into(ClientApiError::from(status))is redundant. SinceClientApiError::from(status)already produces aClientApiError, the additionalInto::into()call is unnecessary.This pattern appears in multiple places (lines 260-261, 298-299, 357-358, 398-399).
Suggested fix
Err(status) => { error!("[:GrpcClient:write] status: {:?}", status); - Err(Into::<ClientApiError>::into(ClientApiError::from(status))) + Err(ClientApiError::from(status)) }Apply similar simplification to lines 298-299, 357-358, and 398-399.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-client/src/grpc_client.rs` around lines 260 - 262, Simplify redundant conversions by removing the Into::into wrapper where ClientApiError::from(status) already yields the target type: replace occurrences like the Err(Into::<ClientApiError>::into(ClientApiError::from(status))) in the GrpcClient::write path (logged as "[:GrpcClient:write] status: {:?}") and the other similar branches with a direct Err(ClientApiError::from(status)); apply the same change to the other three occurrences noted in the comment so all four sites return Err(ClientApiError::from(status)) directly.d-engine-core/src/event.rs (1)
221-228: Placeholder conversions may cause confusion in tests.Using
TestEvent::CreateSnapshotEventas a placeholder forStepDownSelfRemovedandMembershipAppliedcould lead to misleading test assertions if these events are accidentally captured. Consider either:
- Adding dedicated
TestEventvariants for these events- Using a distinct placeholder variant like
TestEvent::InternalEvent- Adding a comment in tests warning about this behavior
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/event.rs` around lines 221 - 228, The conversion currently maps RaftEvent::StepDownSelfRemoved and RaftEvent::MembershipApplied to TestEvent::CreateSnapshotEvent which can mislead tests; add a distinct TestEvent variant (e.g., TestEvent::InternalEvent) or dedicated variants (e.g., TestEvent::StepDownSelfRemoved, TestEvent::MembershipApplied) to the TestEvent enum and update the conversion match arm for RaftEvent::StepDownSelfRemoved and RaftEvent::MembershipApplied in the conversion function to return the new variant(s) (or InternalEvent) instead of TestEvent::CreateSnapshotEvent so tests can't accidentally treat these internal events as snapshots.d-engine-core/src/raft_role/leader_state_test/client_write_test.rs (1)
566-570: Timing assertion may be flaky in CI environments.The assertion
elapsed.as_millis() < 10could fail intermittently on slower CI runners or under load. Consider either:
- Increasing the threshold to a more generous value (e.g., 100ms)
- Using
#[ignore]if this is a performance benchmark rather than correctness test- Removing the timing assertion if the test's primary goal is functional correctness
Suggested fix
- // Verify: Processing was immediate (< 10ms) - assert!( - elapsed.as_millis() < 10, - "Single write should process immediately, took {elapsed:?}" - ); + // Verify: Processing was reasonably fast (< 100ms allows CI variance) + assert!( + elapsed.as_millis() < 100, + "Single write should process quickly, took {elapsed:?}" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state_test/client_write_test.rs` around lines 566 - 570, The timing assertion using elapsed.as_millis() < 10 is too tight and can flake on CI; update the assertion in the test so it uses a more generous threshold (e.g., change 10 to 100) and adjust the failure message accordingly (referencing the existing elapsed variable and the assert! call) so the test is stable across slower runners; alternatively, if timing isn't essential, remove the timing assertion or mark the test with #[ignore]—prefer increasing the threshold to 100ms in the assert! in client_write_test (the assert! that checks elapsed.as_millis()).d-engine-core/src/raft_role/role_state.rs (1)
371-378: Silent error swallowing on state machine read failure.The
unwrap_or_default()on line 375 silently converts read errors to empty results. This could mask underlying issues (e.g., state machine corruption, I/O errors). Consider logging the error before returning the default:Suggested fix
// Read directly from local state machine without any consistency checks - let results = ctx + let results = match ctx .state_machine_handler() .read_from_state_machine(req.keys) - .unwrap_or_default(); + { + Some(r) => r, + None => { + tracing::warn!("Eventual read from state machine returned None"); + vec![] + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/role_state.rs` around lines 371 - 378, The read_from_state_machine call currently uses unwrap_or_default which hides errors; change it to explicitly match the Result from ctx.state_machine_handler().read_from_state_machine(req.keys), log any Err with its error details (including context like the request keys and the role/state) before falling back to an empty/default result, then call ClientResponse::read_results on the obtained results and send via sender.send as before; reference the functions state_machine_handler(), read_from_state_machine(), ClientResponse::read_results, and sender.send when making the change.d-engine-core/src/raft_role/leader_state.rs (2)
1128-1132: Consider deprecation annotation for legacy event.
FlushReadBufferis now a no-op, retained only for backward compatibility. Consider adding a#[deprecated]annotation to theRaftEvent::FlushReadBuffervariant to signal removal in a future version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state.rs` around lines 1128 - 1132, Add a deprecation annotation to the legacy enum variant by marking RaftEvent::FlushReadBuffer as deprecated: locate the RaftEvent enum definition and add #[deprecated(note = "FlushReadBuffer is a no-op and will be removed in a future release; use flush_cmd_buffers in tick instead")] immediately above the FlushReadBuffer variant, and update any doc comments to mention the deprecation and suggested alternative; ensure the project still compiles and run tests to catch any callers that now produce deprecation warnings so you can update or silence them as needed.
841-845: Consider defensive handling instead of.expect()for robustness.The
.expect()assumesclient_command_to_entry_payloadsalways returns exactly one element for a single command input. If this invariant ever changes, this will panic at runtime.Safer alternative
- let payload = client_command_to_entry_payloads(vec![cmd]) - .into_iter() - .next() - .expect("client_command_to_entry_payloads should return 1 element"); + let payloads = client_command_to_entry_payloads(vec![cmd]); + let Some(payload) = payloads.into_iter().next() else { + let _ = sender.send(Err(Status::internal("Failed to convert command"))); + return; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state.rs` around lines 841 - 845, Replace the panic-causing expect by defensively handling the iterator returned from client_command_to_entry_payloads: when req.command is Some, call client_command_to_entry_payloads(vec![cmd]) and then check the iterator (or collect) for zero elements (handle as an error/early return to the caller or respond with a proper failure), and if more than one element occurs, take the first element but log or warn about the unexpected extra elements; update the code paths in leader_state.rs around the payload construction (the req.command branch and the variable named payload) to propagate or handle the error instead of panicking.
- fix get_multi: align sparse server results to input key order (fill None for missing keys) - fix into_write_result: protocol violation now returns Err instead of Ok(false) - fix CAS watch: only broadcast event on succeeded CAS, not failed - add debug_assert_eq in broadcast_watch_events to enforce results/chunk contract - fix candidate_state_test: treat Disconnected as panic, not silent pass - fix drain batch tests: record batch sizes per flush, clear no-op from BecomeLeader - add watch mock support and watch tests (error/success/empty stream) - add into_write_result unit tests (succeeded/failed-CAS/wrong-variant/none/error-code) - update CHANGELOG: clarify CAS latency description
chunk contains all entry types (no-op, config change, client write), while results only covers entries with write operations — lengths are not expected to be equal. results.get(i).is_some_and() is the correct defensive pattern.
- fix client_api.rs: correct doc reference get_with_policy → get_multi_with_policy (3 occurrences) - fix batch buffers: reset buffer_length gauge to 0 on flush/take_all (ProposeBatchBuffer, BatchBuffer) - fix test: retry PROPOSE_FAILED (4006) in snapshot_concurrent_writes to handle transient snapshot window - fix test: wait for stable leader in append_entries_out_of_sync before asserting leader_id
- leader_failover_cas_standalone: probe new leader via actual read instead of cached get_leader_id(); increase retry limit 20→40 for slower election under load - leader_failover_standalone: ignore refresh() error when no majority exists, cluster unavailability is the expected state being tested
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
benches/embedded-bench/src/main.rs (1)
23-75:⚠️ Potential issue | 🟡 MinorBatch done-signal file can be cleared by late followers.
All nodes delete the done file at startup; a follower that starts/restarts after the leader writes the signal can remove it and then wait indefinitely. Consider scoping cleanup to the leader and making the path configurable (or run-specific) to avoid stale-file collisions.
♻️ Suggested tweak to reduce race risk and allow a run-specific path
@@ /// Run all benchmark tests in sequence (batch mode) #[arg(long, default_value = "false")] batch: bool, + /// Path to done-signal file used to coordinate batch shutdown + #[arg(long, default_value = "/tmp/embedded-bench-done")] + done_signal_path: String, @@ - let done_signal_path = "/tmp/embedded-bench-done"; + let done_signal_path = cli.done_signal_path.clone(); @@ - if cli.batch { - // Clean up any leftover signal file from a previous run - let _ = std::fs::remove_file(done_signal_path); - - if engine.is_leader() { + if cli.batch { + if engine.is_leader() { + // Clean up any leftover signal file from a previous run + let _ = std::fs::remove_file(&done_signal_path); run_batch_tests(&engine, &cli).await; // Signal followers to auto-shutdown - if let Err(e) = std::fs::write(done_signal_path, "done") { + if let Err(e) = std::fs::write(&done_signal_path, "done") { eprintln!("Failed to write done signal: {e}"); } } else { @@ - if std::path::Path::new(done_signal_path).exists() { + if std::path::Path::new(&done_signal_path).exists() { println!("Benchmark completed, shutting down follower."); break; }Also applies to: 466-504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/embedded-bench/src/main.rs` around lines 23 - 75, The startup cleanup currently removes the shared batch "done" file for all nodes, allowing late followers to delete the leader's signal; modify the Cli struct and startup logic so cleanup is scoped to the leader and run-specific: add a new CLI option (e.g., done_file: String or run_id: String) to Cli and use it to construct a unique per-run done-file path, change the startup routine that currently unconditionally deletes the done file so it only removes the file when mode == "server" (or when the node is the leader) and not for followers, and ensure the leader is responsible for creating and deleting the run-specific done file while followers only read/wait on it; update any code that references the global done-file path to use the new Cli::done_file or Cli::run_id-derived path.d-engine-client/src/grpc_client.rs (1)
43-115:⚠️ Potential issue | 🟠 MajorPublic inherent read helpers shadow
ClientApimethods (type‑mismatch risk).
GrpcClientexposesget_linearizable/get_lease/get_eventual/get_with_policy/get_multi_with_policyreturningClientResult, whileClientApi’s methods of the same names returnBytes. BecauseClientderefs toGrpcClient, method resolution picks the inherent versions, effectively hiding the trait methods and causing unexpected type errors for users expecting the unified API.Consider making these helpers non‑public (or renaming them) and keep the public surface aligned with
ClientApi.🔧 Suggested fix (limit visibility to avoid shadowing)
- pub async fn get_linearizable( + pub(crate) async fn get_linearizable( @@ - pub async fn get_lease( + pub(crate) async fn get_lease( @@ - pub async fn get_eventual( + pub(crate) async fn get_eventual( @@ - pub async fn get_with_policy( + pub(crate) async fn get_with_policy( @@ - pub async fn get_multi_with_policy( + pub(crate) async fn get_multi_with_policy(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-client/src/grpc_client.rs` around lines 43 - 115, The GrpcClient inherent methods get_linearizable, get_lease, get_eventual, get_with_policy and get_multi_with_policy are public and shadow the ClientApi trait methods (causing the Bytes-vs-ClientResult mismatch); fix by reducing their visibility (e.g., remove pub or change to pub(crate)) or renaming them (e.g., with a private helper prefix) so the trait methods on ClientApi remain the public surface; update any internal call sites that rely on the current names/visibility to use the new private names or explicitly call the ClientApi trait implementation where needed (refer to GrpcClient::get_linearizable, ::get_lease, ::get_eventual, ::get_with_policy and ::get_multi_with_policy to locate the definitions).
🧹 Nitpick comments (13)
d-engine-core/src/raft_role/learner_state_test.rs (2)
1617-1656: Add explicit expectation to verifyshould_snapshotis never called when disabled.In
test_apply_completed_respects_snapshot_disabled_config, theMockStateMachineHandleris created without expectations. Ifmockallis configured with default behavior allowing any calls, this test would pass even if the implementation incorrectly invokesshould_snapshot. Adding an explicit.times(0)expectation ensures the test fails if the disabled config is not respected.💡 Suggested fix
// Create a mock state machine handler - let mock_sm_handler = MockStateMachineHandler::new(); + let mut mock_sm_handler = MockStateMachineHandler::new(); + // Explicitly verify should_snapshot is never called when snapshot is disabled + mock_sm_handler + .expect_should_snapshot() + .times(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/learner_state_test.rs` around lines 1617 - 1656, Add an explicit mock expectation that MockStateMachineHandler::should_snapshot is never invoked in the test_apply_completed_respects_snapshot_disabled_config test: locate the test function and, when creating mock_sm_handler (MockStateMachineHandler::new()), add an expectation like mock_sm_handler.expect_should_snapshot().times(0) so the test will fail if should_snapshot is called despite node_config.raft.snapshot.enable = false; keep the rest of the test (calling learner.handle_raft_event and asserting no snapshot RoleEvent) unchanged.
1232-1253: Consider renaming the test to reflect its reduced scope.The test name
test_learner_rejects_leader_only_eventssuggests multiple events are tested, but now onlyLogPurgeCompletedis verified. Consider renaming totest_learner_rejects_log_purge_completed_eventfor clarity, or add a comment listing which events are now handled by Learner independently.💡 Suggested rename
- async fn test_learner_rejects_leader_only_events() { + async fn test_learner_rejects_log_purge_completed_event() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/learner_state_test.rs` around lines 1232 - 1253, Rename the test function test_learner_rejects_leader_only_events to test_learner_rejects_log_purge_completed_event to reflect that it only asserts rejection of RaftEvent::LogPurgeCompleted; update the test fn name and its invocation (if any) and optionally add a one-line comment above the assertion mentioning that LearnerState::handle_raft_event now handles CreateSnapshotEvent and SnapshotCreated independently so only LogPurgeCompleted is expected to produce a ConsensusError::RoleViolation.d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs (2)
148-166: Doc comment doesn’t match the test behavior.The block describes SnapshotCreated/transport error, but the test covers LogPurgeCompleted index updates. Consider updating the comment to match the scenario to avoid confusion.
✍️ Suggested doc update
-/// Test handling SnapshotCreated with transport error -/// -/// # Test Scenario -/// Leader completes snapshot creation but encounters transport error -/// when sending purge requests to peers. -/// -/// # Given -/// - Leader with completed snapshot -/// - Mock transport returns error -/// -/// # When -/// - SnapshotCreated event is handled -/// -/// # Then -/// - Event handling returns error -/// - No role transition events sent -/// - snapshot_in_progress flag is reset +/// Test handling LogPurgeCompleted updates last_purged_index +/// +/// # Test Scenario +/// Leader reports log purge completion and should update +/// last_purged_index monotonically. +/// +/// # Given +/// - Leader with prior last_purged_index +/// +/// # When +/// - LogPurgeCompleted event is handled with higher and lower indices +/// +/// # Then +/// - Higher index updates last_purged_index +/// - Lower index is ignored +/// - First purge initializes last_purged_index🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs` around lines 148 - 166, Update the test doc comment to accurately describe the behavior exercised by the test function test_handle_log_purge_completed: replace the SnapshotCreated/transport-error description with one that states the test verifies handling of LogPurgeCompleted index updates (e.g., leader updates peer purge indexes, verifies no role transitions and snapshot_in_progress reset), and remove or reword references to transport errors and snapshot creation so the comment matches the actual assertions and scenario in test_handle_log_purge_completed.
136-141: Tight 100ms wall‑clock bound could be flaky under CI load.Consider relaxing the bound and surfacing elapsed time in the assertion message to reduce false failures and aid debugging.
♻️ Optional tweak to reduce flakiness
- assert!(elapsed < std::time::Duration::from_millis(100)); + assert!( + elapsed < std::time::Duration::from_millis(250), + "CreateSnapshotEvent handling was slow: {:?}", + elapsed + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs` around lines 136 - 141, The test's tight 100ms assertion on elapsed after calling state.handle_raft_event(RaftEvent::CreateSnapshotEvent, ...) is flaky; relax the timeout (e.g., to 300–1000ms) and change the assertion to include the measured elapsed value in the failure message so CI failures show the actual duration (update the assert! that currently checks elapsed < Duration::from_millis(100) to use a larger Duration and an assertion message referencing elapsed).benches/embedded-bench/Makefile (1)
203-206: Consider parameterizing the batch client count.Hard-coding
--clients 1000makesall-testsless configurable compared to other knobs. A Makefile variable keeps defaults while allowing overrides.♻️ Suggested tweak
+ALL_TEST_CLIENTS ?= 1000 @@ $(BENCH_BIN) \ --mode local \ --batch \ - --clients 1000 \ + --clients $(ALL_TEST_CLIENTS) \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/embedded-bench/Makefile` around lines 203 - 206, Replace the hard-coded client count flag in the Makefile command (--clients 1000) with a Makefile variable so callers can override it; add a default variable declaration (e.g., CLIENTS ?= 1000 or BATCH_CLIENTS ?= 1000) near the top of the file and use that variable in the invocation (replace --clients 1000 with --clients $(CLIENTS)) so the all-tests workflow becomes configurable while keeping the same default.d-engine-core/src/event.rs (1)
221-228: Placeholder mapping for internal events is acceptable but could be clearer.The
StepDownSelfRemovedandMembershipAppliedevents map toCreateSnapshotEventas placeholders. While the comments explain the rationale, consider using a dedicatedTestEvent::InternalPlaceholdervariant to make test assertions clearer and avoid confusion ifCreateSnapshotEventis also a valid event in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/event.rs` around lines 221 - 228, The current mapping of RaftEvent::StepDownSelfRemoved and RaftEvent::MembershipApplied to TestEvent::CreateSnapshotEvent is confusing; add a dedicated enum variant TestEvent::InternalPlaceholder (or similarly named) to represent internal/control events, update the mapping in event.rs so both RaftEvent::StepDownSelfRemoved and RaftEvent::MembershipApplied return TestEvent::InternalPlaceholder instead of CreateSnapshotEvent, and adjust any tests or match arms that expect CreateSnapshotEvent so they distinguish real CreateSnapshotEvent from the new InternalPlaceholder.d-engine-core/src/config/raft.rs (1)
1316-1383: MetricsConfig lacks validation call in RaftConfig::validate().
MetricsConfigis defined butRaftConfig::validate()(line 144-177) doesn't callself.metrics.validate(). While currentlyMetricsConfighas no validation logic, adding the call would ensure future validation requirements are automatically included.♻️ Suggested addition to RaftConfig::validate()
self.read_consistency.validate()?; self.watch.validate()?; +// Future-proof: call metrics validation when added +// self.metrics.validate()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/config/raft.rs` around lines 1316 - 1383, Add a MetricsConfig validation hook and invoke it from RaftConfig::validate(): implement a pub fn validate(&self) -> Result<(), _> for MetricsConfig (matching the same Result/Error type used by RaftConfig::validate(), initially just returning Ok(())) and then add a call to self.metrics.validate()? inside RaftConfig::validate() so future checks in MetricsConfig are executed automatically; use the existing symbol names MetricsConfig, RaftConfig::validate, and self.metrics.validate() to locate and wire up the change.d-engine-core/src/commit_handler/default_commit_handler.rs (1)
281-300: Verify unbounded channel backpressure implications.The
sm_apply_txis anUnboundedSender, which means entries will queue indefinitely if the SM Worker falls behind. While this simplifies the design (no send blocking), under sustained high load with a slow state machine, memory could grow unboundedly.This is likely acceptable given:
- SM Worker processes entries asynchronously
- Raft log already provides durability
- Backpressure is handled earlier in the pipeline (at client request level)
Consider documenting this design decision or adding monitoring for queue depth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/commit_handler/default_commit_handler.rs` around lines 281 - 300, The send_to_sm_worker function currently uses sm_apply_tx (an UnboundedSender) which can allow the SM queue to grow without bound if the SM Worker falls behind; update the implementation or docs: either add monitoring/metrics for the UnboundedSender queue depth and emit warnings when the queue length exceeds a threshold, or document the design decision and trade-offs near send_to_sm_worker and where sm_apply_tx is created (mention UnboundedSender and SM Worker backpressure behavior). If you add metrics, expose a gauge/counter for the pending entries and log a warning including the current depth from the queue/monitoring API when it exceeds the configured limit.d-engine-core/src/raft_role/role_state.rs (1)
371-379: Consider logging errors in eventual read path.The
unwrap_or_default()silently converts state machine errors to empty results. While this is acceptable for eventual consistency semantics (best-effort), it may mask underlying state machine issues during debugging.💡 Suggested improvement
// Read directly from local state machine without any consistency checks - let results = ctx + let results = match ctx .state_machine_handler() .read_from_state_machine(req.keys) - .unwrap_or_default(); + { + Some(r) => r, + None => { + tracing::debug!("Eventual read returned no results (state machine may be unavailable)"); + vec![] + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/role_state.rs` around lines 371 - 379, The current eventual-read path swallows errors by calling read_from_state_machine(req.keys).unwrap_or_default(), which can hide state machine failures; change this to match on the Result from ctx.state_machine_handler().read_from_state_machine(req.keys), log the Err (using the crate's logging/tracing facility with context like "read_from_state_machine failed for keys={:?}") and then fall back to default results for the response; keep constructing ClientResponse::read_results(results) and sending via sender.send(Ok(response)) as before so behavior is unchanged except now errors are recorded.d-engine-core/src/replication/mod.rs (1)
39-54: Consider enforcing the senders/payloads invariant in code.Right now the 1:1 relationship is only documented; a constructor with a debug assertion would prevent accidental mismatches.
♻️ Proposed constructor with invariant check
+impl RaftRequestWithSignal { + pub fn new( + id: String, + payloads: Vec<EntryPayload>, + senders: Vec<MaybeCloneOneshotSender<std::result::Result<ClientResponse, Status>>>, + wait_for_apply_event: bool, + ) -> Self { + debug_assert_eq!( + senders.len(), + payloads.len(), + "senders must align 1:1 with payloads" + ); + Self { + id, + payloads, + senders, + wait_for_apply_event, + } + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/replication/mod.rs` around lines 39 - 54, Add a constructor/new function for RaftRequestWithSignal that enforces the documented 1:1 invariant by accepting payloads: Vec<EntryPayload>, senders: Vec<MaybeCloneOneshotSender<Result<ClientResponse, Status>>>, id: String and wait_for_apply_event: bool, and include a debug_assert_eq!(senders.len(), payloads.len()) (or equivalente check) before constructing the struct; update callers to use RaftRequestWithSignal::new(...) so the invariant is checked at creation time (keeping the public fields or making them private if you prefer stronger encapsulation).d-engine-core/src/raft_role/mod.rs (1)
421-430: Updateflush_cmd_buffersdocs to match drain-based behavior.
The current doc mentions size/timeout thresholds, but the drain loop already determines batch boundaries.📝 Suggested doc tweak
- /// Flush command buffers if size or timeout thresholds are reached. - /// For Leader: processes batches if FlushReason indicates need. + /// Flush command buffers after the drain loop has collected a batch. + /// For Leader: processes the drained batch immediately (no timeout/size checks here). /// For non-Leader: no-op (buffers are empty).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/mod.rs` around lines 421 - 430, The doc comment for flush_cmd_buffers is inaccurate about size/timeout thresholds; update it to describe the drain-based batching behavior instead: explain that flush_cmd_buffers delegates to state_mut().flush_cmd_buffers which processes command buffers by draining available entries into batches (the drain loop determines batch boundaries) and that for Leader it processes drained batches based on FlushReason while for non-Leader it is a no-op because buffers are empty; reference RaftContext and RoleEvent as the contextual params the function uses.d-engine-core/src/raft_role/leader_state.rs (2)
2948-2963: Silent error handling on state machine read failures.Line 2960 uses
unwrap_or_default()which returns empty results ifread_from_state_machinefails. This silently converts errors to empty responses, potentially hiding issues from clients.Consider logging errors or returning an error status to clients:
Proposed fix to surface read errors
fn execute_pending_reads( &self, read_batch: impl IntoIterator<Item = LinearizableReadRequest>, ctx: &RaftContext<T>, ) { for (req, sender) in read_batch { - let results = ctx + let response = match ctx .handlers .state_machine_handler .read_from_state_machine(req.keys) - .unwrap_or_default(); - let _ = sender.send(Ok(ClientResponse::read_results(results))); + { + Ok(results) => Ok(ClientResponse::read_results(results)), + Err(e) => { + warn!("State machine read failed: {:?}", e); + Err(Status::internal(format!("Read failed: {e}"))) + } + }; + let _ = sender.send(response); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state.rs` around lines 2948 - 2963, The execute_pending_reads function currently swallows state-machine read errors via unwrap_or_default(); change it to match on the Result returned by handlers.state_machine_handler.read_from_state_machine(req.keys), log the Err using the RaftContext's logger (e.g. ctx.process_logger or the appropriate logger field on RaftContext) and send an error response to the client instead of empty results; on Ok(results) continue to send ClientResponse::read_results(results) via sender.send, and on Err send a ClientResponse error variant (or sender.send(Err(...))) so callers are informed of the failure (refer to execute_pending_reads, read_from_state_machine, ClientResponse::read_results and sender.send to locate the code).
2457-2484: Design assumption: homogeneous batches forwait_for_applyflag.The OR-combine logic for
wait_for_apply(line 2473) relies on the assumption documented in the comment that batches are never mixed. If this assumption is violated in the future, some clients expecting apply results might not receive them (if their individualwait_for_applywas true but another request's was false).Consider adding a debug assertion to catch violations during development:
Optional: add debug assertion
for mut req in batch { all_payloads.extend(std::mem::take(&mut req.payloads)); all_senders.extend(req.senders); + #[cfg(debug_assertions)] + if !all_payloads.is_empty() && any_wait_for_apply != req.wait_for_apply_event { + debug_assert!(false, "Mixed wait_for_apply flags in batch - design assumption violated"); + } any_wait_for_apply |= req.wait_for_apply_event; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state.rs` around lines 2457 - 2484, In merge_batch_to_write_metadata, add a development-only assertion to ensure the documented homogeneity of the wait_for_apply flag: track whether any request had wait_for_apply_event and whether any request did not, then debug_assert that they are not both true (i.e., not mixed). Use the existing function name merge_batch_to_write_metadata and the variables any_wait_for_apply (and introduce a local any_not_wait_for_apply or similar) so the check runs after the for loop and before returning the WriteMetadata Option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benches/reports/v0.2.3/bench_report_v0.2.3.md`:
- Line 3: Replace the date string "February 18, 2026" with the comma style
"February 18, 2026," in the markdown header so the date follows the "Month Day,
Year," convention for readability; locate the literal date text in the report
(the line containing **Date**: February 18, 2026) and append the missing comma
after the year.
In `@d-engine-core/src/config/raft.rs`:
- Around line 836-838: Update the migration documentation to note the change in
default persistence strategy introduced by default_persistence_strategy()
returning PersistenceStrategy::DiskFirst: in MIGRATION_GUIDE.md and the
"Migration Notes" section of CHANGELOG.md add a short upgrade instruction
telling users to restore prior behavior by adding persistence_strategy =
"MemFirst" under their [raft.persistence] config (e.g., add the key
persistence_strategy = "MemFirst" to the [raft.persistence] table) and briefly
state that this reverts to the previous in-memory-first behavior.
In `@d-engine-core/src/raft_role/buffers/batch_buffer_test.rs`:
- Around line 14-21: The tests in batch_buffer_test.rs are directly accessing
the BatchBuffer's private fields (buffer and last_flush); add public accessor
methods on BatchBuffer (e.g., buffer_is_empty(), buffer_capacity(), and a getter
for last_flush or last_flush_elapsed()) so tests can use stable API instead of
internal fields, then update the tests to call these methods (or alternatively
move the tests into the batch_buffer module). Locate the impl for BatchBuffer
(type BatchBuffer<T>) and add these simple, public getters and update assertions
in tests at lines referenced to use buffer_is_empty(), buffer_capacity(), and
last_flush_elapsed() (or last_flush()) instead of buffer.buffer and
buffer.last_flush.
In `@d-engine-core/src/raft_role/leader_state_test/client_write_test.rs`:
- Around line 523-570: The test currently asserts a brittle wall-clock bound
(elapsed.as_millis() < 10) after calling state.flush_cmd_buffers; remove that
tight timing assertion and instead verify behavioral outcomes: assert that
state.propose_buffer.len() == 0 after flush and that the client response
(rx.recv().await) is Ok; alternatively, if you want a timing guard, replace the
10ms magic with a much larger, configurable threshold (e.g., 100ms or read from
env) used with elapsed.as_millis() to reduce flakes. Ensure to update the
assertions near the flush_cmd_buffers call and the variables elapsed,
propose_buffer, and rx accordingly (remove or relax the elapsed check and
add/keep the buffer-empty + response assertions).
In `@d-engine-core/src/raft_role/leader_state.rs`:
- Around line 841-845: The code currently unwraps the first element of
client_command_to_entry_payloads via .into_iter().next().expect(...), which can
panic if an empty vector is returned; instead, change the logic around the
client_command_to_entry_payloads call (the block handling req.command and the
local variable payload) to defensively handle the None case from .next() — e.g.,
use match or if let to get Some(payload) and proceed, and on None return an
appropriate error or early-return/skip the request and log the failure; ensure
you reference client_command_to_entry_payloads and the local payload variable in
leader_state.rs when making this change.
---
Outside diff comments:
In `@benches/embedded-bench/src/main.rs`:
- Around line 23-75: The startup cleanup currently removes the shared batch
"done" file for all nodes, allowing late followers to delete the leader's
signal; modify the Cli struct and startup logic so cleanup is scoped to the
leader and run-specific: add a new CLI option (e.g., done_file: String or
run_id: String) to Cli and use it to construct a unique per-run done-file path,
change the startup routine that currently unconditionally deletes the done file
so it only removes the file when mode == "server" (or when the node is the
leader) and not for followers, and ensure the leader is responsible for creating
and deleting the run-specific done file while followers only read/wait on it;
update any code that references the global done-file path to use the new
Cli::done_file or Cli::run_id-derived path.
In `@d-engine-client/src/grpc_client.rs`:
- Around line 43-115: The GrpcClient inherent methods get_linearizable,
get_lease, get_eventual, get_with_policy and get_multi_with_policy are public
and shadow the ClientApi trait methods (causing the Bytes-vs-ClientResult
mismatch); fix by reducing their visibility (e.g., remove pub or change to
pub(crate)) or renaming them (e.g., with a private helper prefix) so the trait
methods on ClientApi remain the public surface; update any internal call sites
that rely on the current names/visibility to use the new private names or
explicitly call the ClientApi trait implementation where needed (refer to
GrpcClient::get_linearizable, ::get_lease, ::get_eventual, ::get_with_policy and
::get_multi_with_policy to locate the definitions).
---
Duplicate comments:
In @.github/workflows/dependency-audit.yml:
- Around line 15-16: Replace the RUSTUP_TOOLCHAIN value currently set to
"stable" with the workspace MSRV pin "1.88.0" (i.e., set env RUSTUP_TOOLCHAIN:
1.88.0) so the audit workflow uses the declared rust-version; verify the
Cargo.toml rust-version matches 1.88.0 and update the workflow value if the MSRV
differs before committing.
In `@d-engine-core/src/client/client_api.rs`:
- Around line 275-336: The doc comments for get_linearizable, get_lease, and the
eventual-consistency get method reference a non-existent get_with_policy()
symbol; update those references to either get_multi_with_policy() (which
supports single-key usage) or remove the link entirely: locate the doc blocks on
the trait methods get_linearizable and get_lease (and the subsequent
eventual-consistency getter) and replace occurrences of
"`[`get_with_policy()`](Self::get_with_policy)`" with
"`[`get_multi_with_policy()`](Self::get_multi_with_policy)`" or a plain text
reference like "with a policy" so the docs no longer point to a missing
function.
In `@d-engine-core/src/raft_role/buffers/batch_buffer.rs`:
- Around line 65-83: The buffer-length metric isn't reset when
draining/clearing, so update the gauge in the draining paths: in
BatchBuffer::take_all() after mem::take(&mut self.buffer) (or before returning)
set the gauge to 0 (or set it to self.buffer.len() after the swap) and similarly
in the #[cfg(test)] BatchBuffer::clear() set the gauge to 0 after clearing;
reference the methods take_all, clear and the buffer/gauge fields so the push()
gauge updates remain in sync with drains.
In `@d-engine-core/src/raft_role/buffers/propose_batch_buffer.rs`:
- Around line 113-133: The flush() implementation drains
self.payloads/self.senders but never updates the buffer-length gauge (unlike
BatchBuffer which updates on push), so after swapping out the payloads you
should reset the buffer-length gauge to zero; locate
propose_batch_buffer::flush(), immediately after the std::mem::swap calls (or
before returning the RaftRequestWithSignal) and set the buffer-length metric
(the same gauge updated in push()) to 0 (or otherwise update it to reflect the
new self.payloads.len()) so the gauge accurately reflects the emptied buffer.
In `@d-engine-core/src/raft_test/drain_based_batch_architecture_tests.rs`:
- Around line 60-82: The test's hard latency assertion is brittle; locate the
drain test block using Instant::now(), buffer.push(42), buffer.take_all(),
elapsed and the assertion comparing elapsed to Duration::from_millis(5) (and the
duplicate at the other occurrence) and relax or remove the threshold — e.g.,
replace the strict 5ms bound with a looser value such as 50ms (or remove the
timing assertion altogether) so the test no longer flakes under CI.
---
Nitpick comments:
In `@benches/embedded-bench/Makefile`:
- Around line 203-206: Replace the hard-coded client count flag in the Makefile
command (--clients 1000) with a Makefile variable so callers can override it;
add a default variable declaration (e.g., CLIENTS ?= 1000 or BATCH_CLIENTS ?=
1000) near the top of the file and use that variable in the invocation (replace
--clients 1000 with --clients $(CLIENTS)) so the all-tests workflow becomes
configurable while keeping the same default.
In `@d-engine-core/src/commit_handler/default_commit_handler.rs`:
- Around line 281-300: The send_to_sm_worker function currently uses sm_apply_tx
(an UnboundedSender) which can allow the SM queue to grow without bound if the
SM Worker falls behind; update the implementation or docs: either add
monitoring/metrics for the UnboundedSender queue depth and emit warnings when
the queue length exceeds a threshold, or document the design decision and
trade-offs near send_to_sm_worker and where sm_apply_tx is created (mention
UnboundedSender and SM Worker backpressure behavior). If you add metrics, expose
a gauge/counter for the pending entries and log a warning including the current
depth from the queue/monitoring API when it exceeds the configured limit.
In `@d-engine-core/src/config/raft.rs`:
- Around line 1316-1383: Add a MetricsConfig validation hook and invoke it from
RaftConfig::validate(): implement a pub fn validate(&self) -> Result<(), _> for
MetricsConfig (matching the same Result/Error type used by
RaftConfig::validate(), initially just returning Ok(())) and then add a call to
self.metrics.validate()? inside RaftConfig::validate() so future checks in
MetricsConfig are executed automatically; use the existing symbol names
MetricsConfig, RaftConfig::validate, and self.metrics.validate() to locate and
wire up the change.
In `@d-engine-core/src/event.rs`:
- Around line 221-228: The current mapping of RaftEvent::StepDownSelfRemoved and
RaftEvent::MembershipApplied to TestEvent::CreateSnapshotEvent is confusing; add
a dedicated enum variant TestEvent::InternalPlaceholder (or similarly named) to
represent internal/control events, update the mapping in event.rs so both
RaftEvent::StepDownSelfRemoved and RaftEvent::MembershipApplied return
TestEvent::InternalPlaceholder instead of CreateSnapshotEvent, and adjust any
tests or match arms that expect CreateSnapshotEvent so they distinguish real
CreateSnapshotEvent from the new InternalPlaceholder.
In `@d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs`:
- Around line 148-166: Update the test doc comment to accurately describe the
behavior exercised by the test function test_handle_log_purge_completed: replace
the SnapshotCreated/transport-error description with one that states the test
verifies handling of LogPurgeCompleted index updates (e.g., leader updates peer
purge indexes, verifies no role transitions and snapshot_in_progress reset), and
remove or reword references to transport errors and snapshot creation so the
comment matches the actual assertions and scenario in
test_handle_log_purge_completed.
- Around line 136-141: The test's tight 100ms assertion on elapsed after calling
state.handle_raft_event(RaftEvent::CreateSnapshotEvent, ...) is flaky; relax the
timeout (e.g., to 300–1000ms) and change the assertion to include the measured
elapsed value in the failure message so CI failures show the actual duration
(update the assert! that currently checks elapsed < Duration::from_millis(100)
to use a larger Duration and an assertion message referencing elapsed).
In `@d-engine-core/src/raft_role/leader_state.rs`:
- Around line 2948-2963: The execute_pending_reads function currently swallows
state-machine read errors via unwrap_or_default(); change it to match on the
Result returned by
handlers.state_machine_handler.read_from_state_machine(req.keys), log the Err
using the RaftContext's logger (e.g. ctx.process_logger or the appropriate
logger field on RaftContext) and send an error response to the client instead of
empty results; on Ok(results) continue to send
ClientResponse::read_results(results) via sender.send, and on Err send a
ClientResponse error variant (or sender.send(Err(...))) so callers are informed
of the failure (refer to execute_pending_reads, read_from_state_machine,
ClientResponse::read_results and sender.send to locate the code).
- Around line 2457-2484: In merge_batch_to_write_metadata, add a
development-only assertion to ensure the documented homogeneity of the
wait_for_apply flag: track whether any request had wait_for_apply_event and
whether any request did not, then debug_assert that they are not both true
(i.e., not mixed). Use the existing function name merge_batch_to_write_metadata
and the variables any_wait_for_apply (and introduce a local
any_not_wait_for_apply or similar) so the check runs after the for loop and
before returning the WriteMetadata Option.
In `@d-engine-core/src/raft_role/learner_state_test.rs`:
- Around line 1617-1656: Add an explicit mock expectation that
MockStateMachineHandler::should_snapshot is never invoked in the
test_apply_completed_respects_snapshot_disabled_config test: locate the test
function and, when creating mock_sm_handler (MockStateMachineHandler::new()),
add an expectation like mock_sm_handler.expect_should_snapshot().times(0) so the
test will fail if should_snapshot is called despite
node_config.raft.snapshot.enable = false; keep the rest of the test (calling
learner.handle_raft_event and asserting no snapshot RoleEvent) unchanged.
- Around line 1232-1253: Rename the test function
test_learner_rejects_leader_only_events to
test_learner_rejects_log_purge_completed_event to reflect that it only asserts
rejection of RaftEvent::LogPurgeCompleted; update the test fn name and its
invocation (if any) and optionally add a one-line comment above the assertion
mentioning that LearnerState::handle_raft_event now handles CreateSnapshotEvent
and SnapshotCreated independently so only LogPurgeCompleted is expected to
produce a ConsensusError::RoleViolation.
In `@d-engine-core/src/raft_role/mod.rs`:
- Around line 421-430: The doc comment for flush_cmd_buffers is inaccurate about
size/timeout thresholds; update it to describe the drain-based batching behavior
instead: explain that flush_cmd_buffers delegates to
state_mut().flush_cmd_buffers which processes command buffers by draining
available entries into batches (the drain loop determines batch boundaries) and
that for Leader it processes drained batches based on FlushReason while for
non-Leader it is a no-op because buffers are empty; reference RaftContext and
RoleEvent as the contextual params the function uses.
In `@d-engine-core/src/raft_role/role_state.rs`:
- Around line 371-379: The current eventual-read path swallows errors by calling
read_from_state_machine(req.keys).unwrap_or_default(), which can hide state
machine failures; change this to match on the Result from
ctx.state_machine_handler().read_from_state_machine(req.keys), log the Err
(using the crate's logging/tracing facility with context like
"read_from_state_machine failed for keys={:?}") and then fall back to default
results for the response; keep constructing
ClientResponse::read_results(results) and sending via sender.send(Ok(response))
as before so behavior is unchanged except now errors are recorded.
In `@d-engine-core/src/replication/mod.rs`:
- Around line 39-54: Add a constructor/new function for RaftRequestWithSignal
that enforces the documented 1:1 invariant by accepting payloads:
Vec<EntryPayload>, senders: Vec<MaybeCloneOneshotSender<Result<ClientResponse,
Status>>>, id: String and wait_for_apply_event: bool, and include a
debug_assert_eq!(senders.len(), payloads.len()) (or equivalente check) before
constructing the struct; update callers to use RaftRequestWithSignal::new(...)
so the invariant is checked at creation time (keeping the public fields or
making them private if you prefer stronger encapsulation).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs (1)
148-166:⚠️ Potential issue | 🟡 MinorDoc comment describes wrong test scenario.
The doc comment describes "handling SnapshotCreated with transport error" but the test
test_handle_log_purge_completedactually validatesLogPurgeCompletedevent handling. This appears to be leftover documentation from a removed/refactored test.📝 Suggested fix
-/// Test handling SnapshotCreated with transport error +/// Test handling LogPurgeCompleted event /// /// # Test Scenario -/// Leader completes snapshot creation but encounters transport error -/// when sending purge requests to peers. +/// Leader receives LogPurgeCompleted events and updates last_purged_index. /// /// # Given -/// - Leader with completed snapshot -/// - Mock transport returns error +/// - Leader with existing last_purged_index /// /// # When -/// - SnapshotCreated event is handled +/// - LogPurgeCompleted events are received with various indices /// /// # Then -/// - Event handling returns error -/// - No role transition events sent -/// - snapshot_in_progress flag is reset +/// - Higher indices update last_purged_index +/// - Lower indices are ignored +/// - First purge sets last_purged_index from None #[tokio::test] async fn test_handle_log_purge_completed() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs` around lines 148 - 166, The doc comment above the async test function test_handle_log_purge_completed is incorrect: it describes handling SnapshotCreated with transport error but the test actually verifies handling of the LogPurgeCompleted event. Update the comment to describe the LogPurgeCompleted scenario (leader completes log purge, mock transport behavior, what is asserted: error returned, no role transitions sent, snapshot_in_progress flag reset) and adjust the Given/When/Then lines to reference LogPurgeCompleted and log purge behavior instead of SnapshotCreated.d-engine-core/src/raft_role/candidate_state.rs (1)
366-374:⚠️ Potential issue | 🟡 MinorRoleViolation context mentions snapshot instead of FlushReadBuffer.
This event isn’t snapshot-related; the context string should reflect the actual operation.📝 Suggested fix
- context: format!( - "Candidate node {} attempted to create snapshot.", - ctx.node_id - ), + context: format!( + "Candidate node {} attempted to flush read buffer.", + ctx.node_id + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/candidate_state.rs` around lines 366 - 374, The RoleViolation error raised in the RaftEvent::FlushReadBuffer branch is using a snapshot-related context message; update the context in the ConsensusError::RoleViolation created inside the Candidate state (RaftEvent::FlushReadBuffer handling) to accurately describe the attempted FlushReadBuffer operation (e.g., "Candidate node {node_id} attempted to flush read buffer" or mention FlushReadBuffer) while keeping current_role "Candidate", required_role "Leader", and using ctx.node_id for the node identifier.d-engine-core/src/raft_role/role_state.rs (1)
125-138:⚠️ Potential issue | 🟡 MinorDoc comment still mentions removed
bypass_queueparameter.
The signature no longer includes it—please drop the stale parameter entry to avoid confusion.📝 Suggested doc tweak
- /// - `bypass_queue`: Whether to skip request queues for direct transmission🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/role_state.rs` around lines 125 - 138, The doc comment for the leadership verification function in role_state.rs still documents a removed parameter `bypass_queue`; remove the stale `- 'bypass_queue': Whether to skip request queues for direct transmission` entry from that function's doc comment so the parameter list matches the current function signature (search for the leadership verification doc block in role_state.rs and the mention of `bypass_queue` to locate it).d-engine-core/src/raft_role/learner_state.rs (1)
786-795:⚠️ Potential issue | 🟡 MinorTODO:
last_purged_indexnot propagated from FollowerState.The conversion from
FollowerStatesetslast_purged_index: Nonewith a TODO, whileFrom<&CandidateState>correctly propagatescandidate_state.last_purged_index. This inconsistency could cause the Learner to lose track of already-purged logs after role transition.Consider propagating
follower_state.last_purged_indexsimilarly to how it's done for CandidateState.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/learner_state.rs` around lines 786 - 795, The From<&FollowerState<T>> for LearnerState<T> implementation currently sets last_purged_index to None; update it to propagate the follower's value (use follower_state.last_purged_index similarly to how the CandidateState conversion does) so the Learner preserves already-purged log information when transitioning roles; locate the From<&FollowerState<T>> for LearnerState<T> block and assign last_purged_index from follower_state.last_purged_index (matching the CandidateState→LearnerState approach).
🧹 Nitpick comments (11)
CONTRIBUTING.md (1)
82-88: Add a one‑line note about configuring theupstreamremote.The rebase snippet assumes
upstreamexists; new contributors often only haveorigin. A brief note to add the remote will prevent confusion.♻️ Suggested addition
If `main` has moved forward since you created your branch, rebase before requesting review: ```bash +git remote add upstream https://github.com/deventlab/d-engine.git # if not already set git fetch upstream git rebase upstream/main git push --force-with-lease</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@CONTRIBUTING.mdaround lines 82 - 88, Add a one-line note above the rebase
command snippet explaining that contributors must configure the upstream remote
if it doesn't exist (e.g., run a git remote add upstream ), so the
subsequent commands (git fetch upstream, git rebase upstream/main, git push
--force-with-lease) will work; insert this short note immediately before the
existing snippet and use "upstream" as the remote name in the example.</details> </blockquote></details> <details> <summary>d-engine-core/src/commit_handler/default_commit_handler_test.rs (1)</summary><blockquote> `182-211`: **Consider extracting duplicated SM worker setup into a helper.** The SM worker channel creation and spawning logic is duplicated between `run_handler` and `process_batch_handler`. A small helper could reduce repetition: ```rust fn spawn_sm_worker() -> tokio::sync::mpsc::UnboundedSender<...> { let (sm_apply_tx, mut sm_apply_rx) = tokio::sync::mpsc::unbounded_channel(); tokio::spawn(async move { while let Some(_entries) = sm_apply_rx.recv().await {} }); sm_apply_tx }However, for test code this duplication is acceptable if you prefer keeping each test method self-contained.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/commit_handler/default_commit_handler_test.rs` around lines 182 - 211, The SM worker creation and spawn logic duplicated in process_batch_handler (and run_handler) should be extracted into a small test helper to avoid repetition: create a helper like spawn_sm_worker that creates the tokio::sync::mpsc::unbounded_channel, spawns the worker task that consumes sm_apply_rx, and returns the sm_apply_tx; then replace the inline creation in process_batch_handler and run_handler to call spawn_sm_worker (use the existing symbols sm_apply_tx, sm_apply_rx, and the SM worker task name for clarity) so tests remain self-contained but without duplicated code.d-engine-core/src/commit_handler/default_commit_handler.rs (1)
32-34: Unbounded channel for SM Worker may accumulate entries without backpressure.Using
UnboundedSender<Vec<Entry>>forsm_apply_txmeans the commit handler won't block when the SM Worker is slow. While this avoids blocking the Raft commit path (which is often desirable), it can lead to unbounded memory growth if the SM Worker falls behind under sustained high commit rates.Consider whether bounded channels with appropriate capacity or monitoring/alerting on queue depth would be beneficial for production resilience.
Also applies to: 51-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/commit_handler/default_commit_handler.rs` around lines 32 - 34, The UnboundedSender sm_apply_tx (type mpsc::UnboundedSender<Vec<Entry>>) can cause unbounded memory growth if the SM Worker is slow; replace it with a bounded channel (e.g., tokio::sync::mpsc::Sender<Vec<Entry>> with a configurable capacity) and update the producer/consumer code to use the corresponding send/recv APIs (use try_send or await send and handle the Err case by logging, backoff, or dropping with metrics), and make the same replacement for the other sm-related channel occurrences referenced in the diff (ensure constructors, send sites, and any clones of sm_apply_tx use the new bounded Sender/Receiver types and add queue-depth logging/metrics where appropriate).d-engine-core/src/client/client_api_error.rs (1)
10-11: Doc comment should reflect broader Client API usage.
This alias is used across the unified ClientApi surface, not just KV operations.💡 Suggested doc comment tweak
-/// Result type for KV operations +/// Result type for Client API operations pub type ClientApiResult<T> = std::result::Result<T, ClientApiError>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/client/client_api_error.rs` around lines 10 - 11, The doc comment on the type alias ClientApiResult<T> inaccurately limits its scope to "KV operations"; update the comment to state that ClientApiResult<T> is the general result type used across the unified ClientApi surface (not just KV), and mention that errors are represented by ClientApiError so callers know the error type; modify the comment immediately above the ClientApiResult<T> alias to reflect this broader usage and reference ClientApiError.d-engine-core/src/raft_test/raft_comprehensive_tests.rs (1)
475-495: Rename the test to match the asserted behavior.The body asserts the term stays the same after
BecomeCandidate, but the name says “increments.” Renaming avoids confusion in test output.✏️ Suggested rename
-async fn test_candidate_become_increments_term() { +async fn test_candidate_become_does_not_increment_term() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_test/raft_comprehensive_tests.rs` around lines 475 - 495, The test function named test_candidate_become_increments_term is misnamed because its assertions check that the term does NOT change after handling RoleEvent::BecomeCandidate; rename the test function (e.g., test_candidate_term_unchanged_on_become_candidate or test_candidate_term_unmodified_on_become) to reflect that behavior, update any references to that function name, and keep the existing assertions and comments which note that term increment occurs in candidate_state::tick() on election timeout rather than during the RoleEvent::BecomeCandidate transition.d-engine-core/src/event.rs (1)
188-200: Avoid mapping StepDownSelfRemoved/MembershipApplied to CreateSnapshotEvent in TestEvent.If these events ever surface in tests, they’ll be misclassified. Consider explicit TestEvent variants to keep the mapping one‑to‑one.
🧭 Suggested explicit variants
pub enum TestEvent { @@ PromoteReadyLearners, + StepDownSelfRemoved, + MembershipApplied, @@ ApplyCompleted { last_index: u64, results: Vec<ApplyResult>, }, } @@ - RaftEvent::StepDownSelfRemoved => { - // StepDownSelfRemoved is handled at Raft level, not converted to TestEvent - // This is a control flow event, not a user-facing event - TestEvent::CreateSnapshotEvent // Placeholder - this event won't be emitted to tests - } - RaftEvent::MembershipApplied => { - // MembershipApplied is internal event for cache refresh - TestEvent::CreateSnapshotEvent // Placeholder - } + RaftEvent::StepDownSelfRemoved => TestEvent::StepDownSelfRemoved, + RaftEvent::MembershipApplied => TestEvent::MembershipApplied,Also applies to: 221-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/event.rs` around lines 188 - 200, The TestEvent enum currently maps StepDownSelfRemoved and MembershipApplied to CreateSnapshotEvent which can misclassify tests; instead add explicit TestEvent variants (e.g., TestEvent::StepDownSelfRemoved and TestEvent::MembershipApplied) and update any mapping/From or match logic that converts event types into TestEvent (look for CreateSnapshotEvent mapping and the enum TestEvent) so each original event maps one-to-one to its new explicit TestEvent variant rather than being folded into CreateSnapshotEvent; update any test code or pattern matches that consume TestEvent accordingly.d-engine-core/src/config/raft.rs (1)
1316-1383: Metrics disabled by default - verify this is intentional.Both
enable_backpressureandenable_batchdefault tofalse. While this minimizes overhead, users expecting observability out-of-the-box may be surprised. The documentation is clear, but consider noting this in the "Getting Started" or deployment docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/config/raft.rs` around lines 1316 - 1383, MetricsConfig currently disables observability by default (enable_backpressure and enable_batch are false via default_enable_backpressure_metrics and default_enable_batch_metrics); decide whether to enable metrics by default or explicitly document the choice—if you want metrics on by default, change the defaults in default_enable_backpressure_metrics and default_enable_batch_metrics to return true and update the Default impl and docs/examples accordingly (and add/update any tests); otherwise, keep the code as-is but add a clear prominent note in the "Getting Started" / deployment docs and the MetricsConfig doc comment calling out that enable_backpressure and enable_batch default to false so users are not surprised.d-engine-core/src/raft_role/leader_state_test/fatal_error_test.rs (1)
52-54: Consider usingtempfilefor test paths.Hardcoded
/tmppaths can cause test pollution or conflicts in parallel test runs. Consider usingtempfile::tempdir()for better isolation, consistent with the pattern used insnapshot_test.rs.♻️ Suggested improvement
+ let temp_dir = tempfile::tempdir().unwrap(); let context = MockBuilder::new(graceful_rx) - .with_db_path("/tmp/test_leader_handles_fatal_error_notifies_pending_requests") + .with_db_path(temp_dir.path().join("test_leader_handles_fatal_error")) .build_context();Apply similar change to
test_fatal_error_drains_all_pending_queuesat line 132.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state_test/fatal_error_test.rs` around lines 52 - 54, Replace the hardcoded "/tmp/..." test DB paths by creating an isolated temp directory via tempfile::tempdir() and passing its path into MockBuilder::with_db_path; update the two tests (the one constructing MockBuilder::new(graceful_rx) then .with_db_path("/tmp/test_leader_handles_fatal_error_notifies_pending_requests")... and the test_fatal_error_drains_all_pending_queues that similarly sets a /tmp path) to call tempdir() at test start, use tempdir.path().to_str().unwrap() (or equivalent) when invoking MockBuilder::with_db_path, and let the TempDir drop at test end for automatic cleanup.benches/embedded-bench/src/main.rs (1)
466-504: Signal file coordination looks reasonable; consider cleanup by follower.The file-based coordination mechanism is pragmatic for benchmark automation. One minor improvement: the follower could delete the signal file after detecting it to ensure a clean state for the next run (in case the leader fails to start fresh).
♻️ Optional cleanup improvement
_ = tokio::time::sleep(Duration::from_secs(1)) => { if std::path::Path::new(done_signal_path).exists() { println!("Benchmark completed, shutting down follower."); + let _ = std::fs::remove_file(done_signal_path); // Clean up for next run break; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/embedded-bench/src/main.rs` around lines 466 - 504, When a follower detects the leader-created done_signal_path in the batch-mode loop, delete that file to clean up state for future runs; update the follower branch (the else block where it loops and checks Path::new(done_signal_path).exists()) to call std::fs::remove_file(done_signal_path) after detecting the file (handling and logging any error), then break out of the loop so the follower exits normally.d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs (1)
67-74: Assert the specific backpressure error code to avoid false positives.
Right now any error satisfies the test. Checking forResourceExhaustedmakes the intent explicit (and can be applied to the other rejection cases).✅ Example tightening
- let response = resp_rx.recv().await.unwrap(); - assert!(response.is_err(), "Should reject with error"); + let response = resp_rx.recv().await.unwrap(); + let err = response.expect_err("Should reject with error"); + assert_eq!(err.code(), tonic::Code::ResourceExhausted);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs` around lines 67 - 74, The test currently only checks that proposing was rejected by asserting response.is_err(); update the assertion to verify the error is the specific backpressure error (e.g., ResourceExhausted) so the test cannot pass on unrelated errors: after receiving response from resp_rx (the variable resp_rx and response), match or inspect the Err variant to assert it equals the expected backpressure error variant (ResourceExhausted or the crate's equivalent), while keeping the existing buffer length check on leader.propose_buffer.len() == 2; adjust other similar rejection cases the same way.d-engine-client/src/grpc_client.rs (1)
267-271: Redundant error conversion.
Into::<ClientApiError>::into(ClientApiError::from(status))is equivalent tostatus.into(). The redundant conversion appears in multiple places (lines 269, 307, 366, 407).♻️ Simplified error conversion
Err(status) => { error!("[:GrpcClient:write] status: {:?}", status); - Err(Into::<ClientApiError>::into(ClientApiError::from(status))) + Err(status.into()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-client/src/grpc_client.rs` around lines 267 - 271, Replace the redundant conversion Into::<ClientApiError>::into(ClientApiError::from(status)) with the simpler status.into() wherever a tonic::Status is mapped to ClientApiError (e.g., in the GrpcClient::write error arm shown by the log tag "[:GrpcClient:write]" and the other Err(status) match arms), so change those Err branches to return Err(status.into()) instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@d-engine-core/src/commit_handler/default_commit_handler.rs`:
- Around line 193-199: The code currently returns early when last_error is Some
and thus never forwards remaining commands; change the flow so that before
returning Err(e) you still call self.send_to_sm_worker(&mut command_batch).await
(and handle its Result) so the remaining commands are sent; specifically update
the block around last_error/command_batch to attempt self.send_to_sm_worker(&mut
command_batch).await even when last_error is Some, and then return the original
error (or combine/report both errors if send_to_sm_worker itself fails) while
preserving the original last_error semantics.
---
Outside diff comments:
In `@d-engine-core/src/raft_role/candidate_state.rs`:
- Around line 366-374: The RoleViolation error raised in the
RaftEvent::FlushReadBuffer branch is using a snapshot-related context message;
update the context in the ConsensusError::RoleViolation created inside the
Candidate state (RaftEvent::FlushReadBuffer handling) to accurately describe the
attempted FlushReadBuffer operation (e.g., "Candidate node {node_id} attempted
to flush read buffer" or mention FlushReadBuffer) while keeping current_role
"Candidate", required_role "Leader", and using ctx.node_id for the node
identifier.
In `@d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs`:
- Around line 148-166: The doc comment above the async test function
test_handle_log_purge_completed is incorrect: it describes handling
SnapshotCreated with transport error but the test actually verifies handling of
the LogPurgeCompleted event. Update the comment to describe the
LogPurgeCompleted scenario (leader completes log purge, mock transport behavior,
what is asserted: error returned, no role transitions sent, snapshot_in_progress
flag reset) and adjust the Given/When/Then lines to reference LogPurgeCompleted
and log purge behavior instead of SnapshotCreated.
In `@d-engine-core/src/raft_role/learner_state.rs`:
- Around line 786-795: The From<&FollowerState<T>> for LearnerState<T>
implementation currently sets last_purged_index to None; update it to propagate
the follower's value (use follower_state.last_purged_index similarly to how the
CandidateState conversion does) so the Learner preserves already-purged log
information when transitioning roles; locate the From<&FollowerState<T>> for
LearnerState<T> block and assign last_purged_index from
follower_state.last_purged_index (matching the CandidateState→LearnerState
approach).
In `@d-engine-core/src/raft_role/role_state.rs`:
- Around line 125-138: The doc comment for the leadership verification function
in role_state.rs still documents a removed parameter `bypass_queue`; remove the
stale `- 'bypass_queue': Whether to skip request queues for direct transmission`
entry from that function's doc comment so the parameter list matches the current
function signature (search for the leadership verification doc block in
role_state.rs and the mention of `bypass_queue` to locate it).
---
Duplicate comments:
In `@d-engine-core/src/raft_role/buffers/batch_buffer_test.rs`:
- Around line 18-20: Tests access BatchBuffer internals (buffer.buffer and
buffer.last_flush); add proper public accessors or relocate tests into the same
module to avoid reaching into private fields. Implement small read-only methods
on BatchBuffer like fn len(&self) -> usize, fn capacity(&self) -> usize, and fn
last_flush_elapsed(&self) -> Duration (or pub(crate) getters if you prefer
crate-only visibility) and update the tests to call these (or move the test file
into the module so it can access private fields), replacing direct uses of
buffer.buffer and buffer.last_flush with the new accessor methods.
In `@d-engine-core/src/raft_role/buffers/propose_batch_buffer.rs`:
- Around line 113-139: The flush method is already correct: it resets
last_flush, swaps out exact-sized Vecs (payloads and senders) via
std::mem::swap, and resets the metrics gauge to 0 when metrics_enabled and
metrics_labels is Some; no code changes required — leave the flush method and
its use of payloads, senders, metrics_enabled, metrics_labels, and
RaftRequestWithSignal as implemented.
In `@d-engine-core/src/raft_role/candidate_state_test.rs`:
- Around line 1047-1074: The test currently moves role_tx into
candidate.handle_raft_event so the receiver sees Disconnected rather than Empty;
clone the sender before calling handle_raft_event (e.g., let keep_alive_role_tx
= role_tx.clone()) and pass one clone into handle_raft_event while keeping the
other alive until after the no-event assertion; then drop the kept clone only
after asserting role_rx.try_recv() returns
Err(tokio::sync::mpsc::error::TryRecvError::Empty) to make the "no role events"
check strict for handle_raft_event.
In `@d-engine-core/src/raft_role/leader_state_test/client_write_test.rs`:
- Around line 523-570: The test's strict wall‑clock check (elapsed.as_millis() <
10) is flaky; update the assertion in client_write_test.rs to use a relaxed
timeout (e.g., compare elapsed against a larger Duration like 50–200ms or use a
constant THRESHOLD_MS) or remove the timing check entirely; locate the check
after the flush_cmd_buffers(&ctx, &role_tx).await and before the request success
assert, and adjust the assertion that references elapsed (and the test message)
so it no longer fails on slower CI while still ensuring the operation is
reasonably fast.
In `@d-engine-core/src/raft_role/leader_state.rs`:
- Around line 841-845: The code currently calls
client_command_to_entry_payloads(vec![cmd]) and uses .next().expect(...) which
can panic if the function returns zero items; change this to handle the
empty-case defensively: call
client_command_to_entry_payloads(vec![cmd]).into_iter().next() and match or if
let Some(payload) = ... to proceed, otherwise log a clear error (or return an
appropriate Result/Err) and avoid panicking; update the surrounding logic in
leader_state.rs that uses payload and request handling (the req.command branch)
so the function returns/continues gracefully when no payload is produced.
In `@d-engine-core/src/raft_role/learner_state.rs`:
- Around line 675-686: The purge condition in can_purge_logs is too
conservative: change the commit check from strict less-than to allow equality by
using <= when comparing last_included_in_request.index to self.commit_index()
(i.e. make commit_check true when commit_index ==
last_included_in_request.index) and add a short comment in can_purge_logs
explaining this equality allows purging when the last included entry is already
committed; keep the existing monotonic_check behavior for last_purge_index
as-is.
In `@d-engine-core/src/raft_test/drain_based_batch_architecture_tests.rs`:
- Around line 60-82: The test uses a brittle strict timing assertion (elapsed <
Duration::from_millis(5)) after calling buffer.take_all(); replace this with a
relaxed or behavior-only check: either remove the latency assertion entirely and
rely on functional assertions (assert_eq!(drained.len(), 1) and
assert!(buffer.is_empty())), or change it to a much looser bound (e.g.,
Duration::from_millis(50) or 100) to account for CI jitter; update the code
around Instant::now(), elapsed, and the call to buffer.take_all() accordingly so
the test remains stable while still verifying immediate drain behavior.
- Around line 480-499: The timing assertion asserting elapsed.as_millis() < 10
is too strict and flakes on CI; update the assertion in the follower rejection
test (around RaftRole::Follower, follower.push_client_cmd, response_rx.recv())
to allow a larger, more reliable bound (e.g., change the < 10ms check to a more
relaxed threshold such as < 100ms or use a Duration like 50–100ms) so the test
still verifies "fast" rejection but avoids CI timing flakiness; keep the
assertion message and context (immediate rejection) unchanged.
---
Nitpick comments:
In `@benches/embedded-bench/src/main.rs`:
- Around line 466-504: When a follower detects the leader-created
done_signal_path in the batch-mode loop, delete that file to clean up state for
future runs; update the follower branch (the else block where it loops and
checks Path::new(done_signal_path).exists()) to call
std::fs::remove_file(done_signal_path) after detecting the file (handling and
logging any error), then break out of the loop so the follower exits normally.
In `@CONTRIBUTING.md`:
- Around line 82-88: Add a one-line note above the rebase command snippet
explaining that contributors must configure the upstream remote if it doesn't
exist (e.g., run a git remote add upstream <repo-url>), so the subsequent
commands (git fetch upstream, git rebase upstream/main, git push
--force-with-lease) will work; insert this short note immediately before the
existing snippet and use "upstream" as the remote name in the example.
In `@d-engine-client/src/grpc_client.rs`:
- Around line 267-271: Replace the redundant conversion
Into::<ClientApiError>::into(ClientApiError::from(status)) with the simpler
status.into() wherever a tonic::Status is mapped to ClientApiError (e.g., in the
GrpcClient::write error arm shown by the log tag "[:GrpcClient:write]" and the
other Err(status) match arms), so change those Err branches to return
Err(status.into()) instead.
In `@d-engine-core/src/client/client_api_error.rs`:
- Around line 10-11: The doc comment on the type alias ClientApiResult<T>
inaccurately limits its scope to "KV operations"; update the comment to state
that ClientApiResult<T> is the general result type used across the unified
ClientApi surface (not just KV), and mention that errors are represented by
ClientApiError so callers know the error type; modify the comment immediately
above the ClientApiResult<T> alias to reflect this broader usage and reference
ClientApiError.
In `@d-engine-core/src/commit_handler/default_commit_handler_test.rs`:
- Around line 182-211: The SM worker creation and spawn logic duplicated in
process_batch_handler (and run_handler) should be extracted into a small test
helper to avoid repetition: create a helper like spawn_sm_worker that creates
the tokio::sync::mpsc::unbounded_channel, spawns the worker task that consumes
sm_apply_rx, and returns the sm_apply_tx; then replace the inline creation in
process_batch_handler and run_handler to call spawn_sm_worker (use the existing
symbols sm_apply_tx, sm_apply_rx, and the SM worker task name for clarity) so
tests remain self-contained but without duplicated code.
In `@d-engine-core/src/commit_handler/default_commit_handler.rs`:
- Around line 32-34: The UnboundedSender sm_apply_tx (type
mpsc::UnboundedSender<Vec<Entry>>) can cause unbounded memory growth if the SM
Worker is slow; replace it with a bounded channel (e.g.,
tokio::sync::mpsc::Sender<Vec<Entry>> with a configurable capacity) and update
the producer/consumer code to use the corresponding send/recv APIs (use try_send
or await send and handle the Err case by logging, backoff, or dropping with
metrics), and make the same replacement for the other sm-related channel
occurrences referenced in the diff (ensure constructors, send sites, and any
clones of sm_apply_tx use the new bounded Sender/Receiver types and add
queue-depth logging/metrics where appropriate).
In `@d-engine-core/src/config/raft.rs`:
- Around line 1316-1383: MetricsConfig currently disables observability by
default (enable_backpressure and enable_batch are false via
default_enable_backpressure_metrics and default_enable_batch_metrics); decide
whether to enable metrics by default or explicitly document the choice—if you
want metrics on by default, change the defaults in
default_enable_backpressure_metrics and default_enable_batch_metrics to return
true and update the Default impl and docs/examples accordingly (and add/update
any tests); otherwise, keep the code as-is but add a clear prominent note in the
"Getting Started" / deployment docs and the MetricsConfig doc comment calling
out that enable_backpressure and enable_batch default to false so users are not
surprised.
In `@d-engine-core/src/event.rs`:
- Around line 188-200: The TestEvent enum currently maps StepDownSelfRemoved and
MembershipApplied to CreateSnapshotEvent which can misclassify tests; instead
add explicit TestEvent variants (e.g., TestEvent::StepDownSelfRemoved and
TestEvent::MembershipApplied) and update any mapping/From or match logic that
converts event types into TestEvent (look for CreateSnapshotEvent mapping and
the enum TestEvent) so each original event maps one-to-one to its new explicit
TestEvent variant rather than being folded into CreateSnapshotEvent; update any
test code or pattern matches that consume TestEvent accordingly.
In `@d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs`:
- Around line 67-74: The test currently only checks that proposing was rejected
by asserting response.is_err(); update the assertion to verify the error is the
specific backpressure error (e.g., ResourceExhausted) so the test cannot pass on
unrelated errors: after receiving response from resp_rx (the variable resp_rx
and response), match or inspect the Err variant to assert it equals the expected
backpressure error variant (ResourceExhausted or the crate's equivalent), while
keeping the existing buffer length check on leader.propose_buffer.len() == 2;
adjust other similar rejection cases the same way.
In `@d-engine-core/src/raft_role/leader_state_test/fatal_error_test.rs`:
- Around line 52-54: Replace the hardcoded "/tmp/..." test DB paths by creating
an isolated temp directory via tempfile::tempdir() and passing its path into
MockBuilder::with_db_path; update the two tests (the one constructing
MockBuilder::new(graceful_rx) then
.with_db_path("/tmp/test_leader_handles_fatal_error_notifies_pending_requests")...
and the test_fatal_error_drains_all_pending_queues that similarly sets a /tmp
path) to call tempdir() at test start, use tempdir.path().to_str().unwrap() (or
equivalent) when invoking MockBuilder::with_db_path, and let the TempDir drop at
test end for automatic cleanup.
In `@d-engine-core/src/raft_test/raft_comprehensive_tests.rs`:
- Around line 475-495: The test function named
test_candidate_become_increments_term is misnamed because its assertions check
that the term does NOT change after handling RoleEvent::BecomeCandidate; rename
the test function (e.g., test_candidate_term_unchanged_on_become_candidate or
test_candidate_term_unmodified_on_become) to reflect that behavior, update any
references to that function name, and keep the existing assertions and comments
which note that term increment occurs in candidate_state::tick() on election
timeout rather than during the RoleEvent::BecomeCandidate transition.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
benches/embedded-bench/src/main.rs (1)
466-505:⚠️ Potential issue | 🟡 MinorFollower can delete the done-signal and then hang.
Line 480:
remove_fileruns on all nodes. A follower that starts after the leader has already finished can delete the signal and then wait forever. Prefer leader-only cleanup (or add a run-id).🛠️ Suggested fix (leader-only cleanup)
- if cli.batch { - // Clean up any leftover signal file from a previous run - let _ = std::fs::remove_file(done_signal_path); - - if engine.is_leader() { + if cli.batch { + if engine.is_leader() { + // Clean up any leftover signal file from a previous run + let _ = std::fs::remove_file(done_signal_path); run_batch_tests(&engine, &cli).await; // Signal followers to auto-shutdown if let Err(e) = std::fs::write(done_signal_path, "done") { eprintln!("Failed to write done signal: {e}"); } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/embedded-bench/src/main.rs` around lines 466 - 505, The follower can delete the shared done_signal_path and then hang if it starts after the leader finished; to fix, stop calling std::fs::remove_file(done_signal_path) unconditionally and either perform cleanup only on the leader (wrap the remove_file call behind engine.is_leader() or move it into the leader branch in the batch path) or use a run-specific signal (append a unique run-id to done_signal_path and have followers check the file content/run-id) so followers never remove another run's signal; update references in this block (done_signal_path, engine.is_leader(), remove_file) accordingly.d-engine-core/src/raft_role/candidate_state.rs (1)
366-374:⚠️ Potential issue | 🟡 MinorFix the RoleViolation context message for FlushReadBuffer.
The error text says “attempted to create snapshot”, which doesn’t match FlushReadBuffer.✏️ Suggested message fix
- context: format!( - "Candidate node {} attempted to create snapshot.", - ctx.node_id - ), + context: format!( + "Candidate node {} attempted to flush read buffer.", + ctx.node_id + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/candidate_state.rs` around lines 366 - 374, The context message for the RaftEvent::FlushReadBuffer match arm is incorrect—replace the snapshot text with a message that reflects flushing the read buffer; update the context passed to ConsensusError::RoleViolation (in the RaftEvent::FlushReadBuffer arm inside candidate_state.rs) to something like "Candidate node {id} attempted to flush read buffer." while keeping current_role "Candidate", required_role "Leader", and using ctx.node_id in the format! call so the error accurately describes the operation and node.d-engine-core/src/raft_role/role_state.rs (1)
125-137:⚠️ Potential issue | 🟡 MinorRemove stale
bypass_queuereference from docs.The parameter no longer exists in the signature, but it is still described in the docblock, which can mislead users.
✏️ Suggested doc fix
- /// - `bypass_queue`: Whether to skip request queues for direct transmission🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/role_state.rs` around lines 125 - 137, Remove the stale `bypass_queue` reference from the docblock that begins "Immidiatelly verifies leadership..." in role_state.rs: update the parameters list to match the current function signature (remove the `bypass_queue` bullet), and while editing correct the typo "Immidiatelly" to "Immediately" so the doc and parameter list (e.g., the `payloads`, `ctx`, `role_tx` entries) accurately reflect the implementation.d-engine-core/src/commit_handler/default_commit_handler.rs (1)
166-187:⚠️ Potential issue | 🟠 MajorPrevent post-config-failure commands from being flushed to the SM worker.
After
apply_config_changefails, the loop continues and a later Noop/Config can still flushcommand_batch, so commands after the failed config may be applied despite the “reject following commands” guarantee. Consider halting processing oncelast_erroris set.🐛 Suggested guard to stop after the first config failure
let mut last_error = None; for entry in entries { + if last_error.is_some() { + break; + } // In exact log order if let Some(ref entry_payload) = entry.payload { match entry_payload.payload { Some(Payload::Command(_)) => command_batch.push(entry), Some(Payload::Config(_)) => { command_batch.push(entry.clone()); self.send_to_sm_worker(&mut command_batch).await?; if last_error.is_none() { if let Err(e) = self.apply_config_change(entry).await { last_error = Some(e); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/commit_handler/default_commit_handler.rs` around lines 166 - 187, The loop currently continues after apply_config_change returns an error (stored in last_error), allowing later entries to trigger send_to_sm_worker and flush command_batch; modify the loop in default_commit_handler so that once last_error is set you stop processing further entries (e.g., break the for entry in entries loop or return early) and ensure you do not call send_to_sm_worker or push new entries into command_batch after last_error is Some; reference symbols: last_error, entries, apply_config_change, send_to_sm_worker, command_batch.
🧹 Nitpick comments (12)
d-engine-core/benches/leader_state_bench.rs (1)
172-179: Consider returning per-entry results from the mock apply.Right now
apply_chunkalways returns an empty vector (Line 178). If the production path expects one result per entry, this mock can under‑exercise result handling and skew benchmark realism. Consider returning a vector sized to the input chunk instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/benches/leader_state_bench.rs` around lines 172 - 179, The mock's apply_chunk handler in MockStateMachine currently always returns Ok(vec![]), which under‑exercises result handling; change the expect_apply_chunk stub (the closure passed to MockStateMachine::expect_apply_chunk / apply_chunk) to return a Vec with one result per input entry (e.g., map the input slice/vec length to a same‑length Vec of appropriate Result/Item placeholders) so the benchmark sees per‑entry results instead of an empty vector; update the closure signature used in expect_apply_chunk to inspect its input parameter and construct a Vec::with_capacity(input.len()) filled with the expected per‑entry values.benches/reports/v0.2.3/bench_report_v0.2.3.md (1)
102-102: Consider clarifying the "noise floor" characterization.The standalone mode results show some notable regressions beyond typical noise, particularly the +26% p99 latency increase for eventual reads (line 53). While disk I/O variability exists, characterizing all regressions as "within noise floor" may understate their significance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/reports/v0.2.3/bench_report_v0.2.3.md` at line 102, Update the table row that currently reads "Standalone slight regressions | Within noise floor of local Mac disk I/O variability" to more accurately reflect the observed outliers: mention that while some regressions fall within disk I/O variability, the +26% p99 increase for eventual reads is an exception and should be called out separately (e.g., add a parenthetical note or a second row referencing "eventual reads: +26% p99 — exceeds typical noise floor"). Edit the text surrounding the table to include a brief sentence referencing the specific metric (p99 eventual reads) and recommending further investigation or caveats for that case.d-engine-core/src/raft_role/learner_state_test.rs (1)
565-568: Incomplete documentation for test function.The doc comment is missing the test description. Other tests in this file have proper documentation describing the scenario and expected behavior.
📝 Suggested fix
-/// -/// Original: test_handle_raft_event_case10 +/// Test: LearnerState rejects JoinCluster request +/// +/// Scenario: +/// - Learner receives JoinCluster request +/// - Learners cannot process join requests (not leader) +/// +/// Expected: +/// - Returns Status error with Code::PermissionDenied +/// - handle_raft_event returns Err() +/// +/// Original: test_handle_raft_event_case10 #[tokio::test] async fn test_learner_rejects_join_cluster() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/learner_state_test.rs` around lines 565 - 568, Add a descriptive doc comment above the async test function test_learner_rejects_join_cluster that explains the scenario and expected behavior (e.g., that a learner node should reject a JoinCluster RPC or join attempt, under what conditions, and the expected response or state change), matching the style of other tests in the file; include a one-line summary and a short sentence describing the setup and assertion so future readers and test reports clearly convey intent.d-engine-core/src/replication/mod.rs (1)
45-53: Consider enforcing the payloads/senders invariant with a constructor.
The struct is public and the invariant is easy to violate; anew()with a debug/assert makes misuse harder.♻️ Suggested constructor to enforce the invariant
+impl RaftRequestWithSignal { + pub fn new( + id: String, + payloads: Vec<EntryPayload>, + senders: Vec<MaybeCloneOneshotSender<std::result::Result<ClientResponse, Status>>>, + wait_for_apply_event: bool, + ) -> Self { + debug_assert_eq!(payloads.len(), senders.len(), "payloads/senders mismatch"); + Self { + id, + payloads, + senders, + wait_for_apply_event, + } + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/replication/mod.rs` around lines 45 - 53, Add a constructor to enforce the invariant that senders.len() == payloads.len() for the public struct containing the fields senders, payloads and wait_for_apply_event: implement a pub fn new(payloads: Vec<...>, senders: Vec<...>, wait_for_apply_event: bool) -> Self that performs a runtime check (e.g. debug_assert_eq!(senders.len(), payloads.len()) or a panic/assert in non-debug builds) and returns Self with the fields set; use the constructor throughout (and consider making the struct fields non-pub if you want to force callers to use new) so misuse is prevented for senders, payloads and wait_for_apply_event.d-engine-core/src/raft_role/leader_state_test/fatal_error_test.rs (1)
47-50: Rename the test to reflect actual coverage.Line 49: the name says “notifies_pending_requests,” but this test doesn’t enqueue any pending requests (that’s covered by the second test). Consider renaming to avoid misleading coverage.
♻️ Suggested rename
-async fn test_leader_handles_fatal_error_notifies_pending_requests() { +async fn test_leader_handles_fatal_error_returns_fatal() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state_test/fatal_error_test.rs` around lines 47 - 50, The test function name test_leader_handles_fatal_error_notifies_pending_requests is misleading because this test does not enqueue or assert behavior for pending requests; rename the test function (e.g., to test_leader_handles_fatal_error_sends_shutdown_signal or test_leader_handles_fatal_error_notifies_shutdown) to accurately reflect that it verifies leader fatal error handling/shutdown notification rather than pending-request notification; update the async test function declaration for test_leader_handles_fatal_error_notifies_pending_requests to the chosen clearer name so test output and coverage match the behavior.d-engine-client/src/grpc_client_test.rs (1)
1441-1453: Consider reusingmake_clientacross the earlier tests.
Pulling this helper to module scope would reduce repeated GrpcClient setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-client/src/grpc_client_test.rs` around lines 1441 - 1453, Extract the async helper make_client into module scope so other tests can reuse it: move the async fn make_client(port: u16) -> GrpcClient (and its use of ClientConfig::default(), ConnectionPool::create, ArcSwap and ClientInner) out of the local test body into the top-level of the test module (make it pub(crate) or non-pub as appropriate), keep its signature async so callers can await it, and update earlier tests to call this shared make_client instead of duplicating the GrpcClient setup.d-engine-core/src/raft_role/buffers/batch_buffer.rs (1)
65-76: Preserve capacity ontake_all()to avoid reallocation churn.
mem::takedrops the buffer’s capacity, which can cause allocations on every flush. Swapping with a same-capacity Vec keeps the hot path stable.♻️ Capacity-preserving swap
- let items = std::mem::take(&mut self.buffer); + let mut items = Vec::with_capacity(self.buffer.capacity()); + std::mem::swap(&mut items, &mut self.buffer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/batch_buffer.rs` around lines 65 - 76, The current take_all() uses std::mem::take(&mut self.buffer) which drops capacity and forces reallocation; change it to swap the buffer with a new Vec created with the same capacity so capacity is preserved. In the take_all method (referencing take_all, self.buffer, and self.last_flush) create a new Vec with_capacity(self.buffer.capacity()) and swap or replace it with self.buffer to extract the items while keeping capacity for future pushes; keep the metrics update logic (metrics_enabled, metrics_labels) unchanged and return the extracted items.d-engine-core/src/raft_test/raft_comprehensive_tests.rs (1)
961-974: Add an assertion for the requeue behavior (currently a no-op).
test_reprocess_event_requeuesonly checks the initial role and doesn’t validate that a reprocessed event is actually re-queued. Consider asserting the requeue path (or marking this as TODO) so the test matches its stated intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_test/raft_comprehensive_tests.rs` around lines 961 - 974, The test test_reprocess_event_requeues currently only checks raft role and does not validate that a ReprocessEvent is actually re-queued; update the test to exercise the requeue path by triggering the component that emits ReprocessEvent (use MockBuilder/Mock raft helpers to inject or call the handler that enqueues ReprocessEvent), then receive from the event_rx (or the mocked event channel) and assert the received message is a ReprocessEvent (or expected payload); if the requeue behavior is not implementable yet, replace or augment the test with a clear TODO comment indicating the missing assertion and why it cannot be added now.d-engine-core/src/config/raft.rs (1)
1329-1383: Add validation forsample_rateto prevent division by zero.
MetricsConfiglacks validation forsample_rate. While the consumer (BackpressureMetrics::newin leader_state.rs) handlessample_rate == 0by converting it to1, it's better to validate at the configuration level to fail fast and provide a clear error message.♻️ Proposed validation addition
Add a
validatemethod toMetricsConfig:impl MetricsConfig { pub fn validate(&self) -> Result<()> { if self.sample_rate == 0 { return Err(Error::Config(ConfigError::Message( "metrics.sample_rate must be > 0".into(), ))); } Ok(()) } }Then call it in
RaftConfig::validate():self.read_consistency.validate()?; self.watch.validate()?; +self.metrics.validate()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/config/raft.rs` around lines 1329 - 1383, Add a validation to MetricsConfig to reject sample_rate == 0 and invoke it from RaftConfig::validate: implement pub fn validate(&self) -> Result<()> on MetricsConfig that returns an appropriate Config error if self.sample_rate == 0 (message like "metrics.sample_rate must be > 0"), then update RaftConfig::validate() to call metrics_config.validate()? so the config fails fast before BackpressureMetrics::new is reached. Ensure you reference MetricsConfig::validate and RaftConfig::validate in the change.d-engine-core/src/raft_role/leader_state.rs (2)
888-943: Backpressure limits are applied per-queue, not aggregate.Each read queue (linearizable, lease, eventual) independently checks against
max_pending_reads. This means the system could accept up to3 × max_pending_readstotal pending reads (50,000 × 3 = 150,000 with defaults).If the intent is to limit total pending reads across all policies, consider tracking an aggregate counter. If per-policy limits are intentional, this is fine — just noting the behavior.
💡 Alternative: Aggregate read backpressure
If aggregate limiting is desired:
let total_pending_reads = self.linearizable_read_buffer.len() + self.lease_read_queue.len() + self.eventual_read_queue.len(); if backpressure.should_reject_read(total_pending_reads) { // reject }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state.rs` around lines 888 - 943, The current logic applies backpressure per-queue (see lease_read_queue, eventual_read_queue and linearizable_read_buffer elsewhere) causing up to 3× max_pending_reads to be allowed; to enforce an aggregate limit, compute total_pending_reads = self.linearizable_read_buffer.len() + self.lease_read_queue.len() + self.eventual_read_queue.len() and use backpressure.should_reject_read(total_pending_reads) (and record buffer utilization using that total) before pushing into any queue; update the record_rejection and metrics.record_buffer_utilization calls to use the aggregated value and ensure backpressure_metrics is consulted once per incoming read request (instead of per-queue) so all read policies share the same limit.
2457-2484: Clarify OR-combining ofwait_for_apply_eventflags.The comment correctly notes that OR-combining is safe because "in practice all requests in a batch share the same flag." However, if this invariant is violated (e.g., mixed batch from future code changes), some requests may unexpectedly wait for apply.
Consider adding a debug assertion to catch invariant violations early:
🛡️ Optional: Debug assertion for invariant
fn merge_batch_to_write_metadata( batch: impl IntoIterator<Item = RaftRequestWithSignal>, start_idx: u64, ) -> (Vec<EntryPayload>, Option<WriteMetadata>) { let mut all_payloads = Vec::new(); let mut all_senders = Vec::new(); let mut any_wait_for_apply = false; + #[cfg(debug_assertions)] + let mut first_wait_for_apply: Option<bool> = None; for mut req in batch { all_payloads.extend(std::mem::take(&mut req.payloads)); all_senders.extend(req.senders); + #[cfg(debug_assertions)] + { + if let Some(first) = first_wait_for_apply { + debug_assert_eq!(first, req.wait_for_apply_event, + "Mixed wait_for_apply flags in batch"); + } else { + first_wait_for_apply = Some(req.wait_for_apply_event); + } + } any_wait_for_apply |= req.wait_for_apply_event; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state.rs` around lines 2457 - 2484, The OR-combine of wait_for_apply_event in merge_batch_to_write_metadata can hide mixed-flag batches; add a debug-only invariant check inside merge_batch_to_write_metadata that verifies all RaftRequestWithSignal in the incoming batch share the same wait_for_apply_event value before you OR them into any_wait_for_apply (e.g., capture the first request's wait_for_apply_event, compare each subsequent req.wait_for_apply_event and use debug_assert! or a cfg(debug_assertions) check to fail fast if a mismatch is detected); keep the existing OR logic for release builds but ensure the assertion references RaftRequestWithSignal::wait_for_apply_event, any_wait_for_apply and the function name merge_batch_to_write_metadata so it’s easy to find.d-engine-core/src/commit_handler/default_commit_handler_test.rs (1)
96-108: Tests now drain the SM channel without exercising apply_chunk/command_hook.The in-test SM worker discards entries, so
MockStateMachineHandler::apply_chunkandcommand_hookare never exercised. If the intent is to keep validating apply behavior, consider routing the channel into the mock (or remove those expectations if you only want commit-handler coverage).♻️ Suggested wiring to keep mock expectations meaningful
- // Spawn SM Worker to process entries - let _sm_worker_handle = tokio::spawn(async move { - while let Some(_entries) = sm_apply_rx.recv().await { - // SM Worker simply consumes entries from the channel - // In a real system, this would apply them to the state machine - // For testing, we just drain the channel to keep it open - } - }); + // Spawn SM Worker to process entries + let smh = self.mock_smh.clone(); + let _sm_worker_handle = tokio::spawn(async move { + while let Some(entries) = sm_apply_rx.recv().await { + let _ = smh.apply_chunk(entries); + } + }); @@ - // Spawn SM Worker to process entries - let _sm_worker_handle = tokio::spawn(async move { - while let Some(_entries) = sm_apply_rx.recv().await { - // SM Worker simply consumes entries from the channel - } - }); + // Spawn SM Worker to process entries + let smh = self.mock_smh.clone(); + let _sm_worker_handle = tokio::spawn(async move { + while let Some(entries) = sm_apply_rx.recv().await { + let _ = smh.apply_chunk(entries); + } + });Also applies to: 148-175, 183-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/commit_handler/default_commit_handler_test.rs` around lines 96 - 108, The test currently drains the state-machine (SM) channel before the mock is exercised, so MockStateMachineHandler's expect_apply_chunk and command_hook are never invoked; update the test to either (A) wire the in-test SM worker to forward the SM channel messages into the mock handler (so functions like MockStateMachineHandler::expect_pending_range and ::expect_apply_chunk are actually called) by replacing the current discarding worker with one that sends received entries to the mock, or (B) if you only intend to test the commit-handler without SM behavior, remove the expectations on expect_apply_chunk/command_hook and any related mocks (MockStateMachineHandler) to avoid dead expectations; locate usages around MockStateMachineHandler, expect_pending_range, expect_apply_chunk, and command_hook in the test and apply one of these two changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@d-engine-core/src/commit_handler/default_commit_handler.rs`:
- Around line 281-297: Pending entries can be re-selected and re-sent because
pending_range() doesn't account for entries already sent asynchronously by
send_to_sm_worker(); fix by tracking in-flight ranges and skipping them during
selection: add an in_flight_ranges structure updated inside send_to_sm_worker
(record the range being sent before self.sm_apply_tx.send) and remove/advance
those ranges when the SM apply ack callback/handler (where last_applied is
updated) returns; update pending_range() to consult in_flight_ranges so it does
not return entries that are currently in-flight. Ensure modifications reference
send_to_sm_worker, pending_range(), last_applied, and sm_apply_tx so the
selection logic skips in-flight ranges until confirmed applied.
In `@d-engine-core/src/raft_role/buffers/batch_buffer_test.rs`:
- Around line 75-79: The test uses a tight wall‑clock assertion on
buffer.last_flush.elapsed() (seen after calling buffer.take_all()) which can
flake; replace the direct elapsed < Duration::from_millis(50) check with
deterministic before/after bounds: record an Instant before calling
buffer.take_all() and another Instant immediately after, then assert
buffer.last_flush is >= before and <= after (or within a relaxed margin), and
remove the hard 50–100ms threshold; apply the same change to the other similar
assertion around buffer.last_flush at the second occurrence (the block around
lines 136–140) so both tests use before/after time bounds instead of strict
elapsed thresholds.
In `@d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs`:
- Around line 67-74: Replace the generic is_err() checks with an assertion that
the rejection is the specific backpressure error (ResourceExhausted): after
receiving the response from resp_rx.recv().await unwrap the Err (e.g. let err =
response.unwrap_err()) and assert its code/type equals ResourceExhausted (for
example assert_eq!(err.code(), Code::ResourceExhausted) or assert!(matches!(err,
SomeStatus::ResourceExhausted(_))) depending on your error type). Apply the same
change to the other rejection checks that use
resp_rx.recv().await/response.is_err() so they verify the ResourceExhausted
backpressure error while still ensuring leader.propose_buffer.len() remains
correct.
---
Outside diff comments:
In `@benches/embedded-bench/src/main.rs`:
- Around line 466-505: The follower can delete the shared done_signal_path and
then hang if it starts after the leader finished; to fix, stop calling
std::fs::remove_file(done_signal_path) unconditionally and either perform
cleanup only on the leader (wrap the remove_file call behind engine.is_leader()
or move it into the leader branch in the batch path) or use a run-specific
signal (append a unique run-id to done_signal_path and have followers check the
file content/run-id) so followers never remove another run's signal; update
references in this block (done_signal_path, engine.is_leader(), remove_file)
accordingly.
In `@d-engine-core/src/commit_handler/default_commit_handler.rs`:
- Around line 166-187: The loop currently continues after apply_config_change
returns an error (stored in last_error), allowing later entries to trigger
send_to_sm_worker and flush command_batch; modify the loop in
default_commit_handler so that once last_error is set you stop processing
further entries (e.g., break the for entry in entries loop or return early) and
ensure you do not call send_to_sm_worker or push new entries into command_batch
after last_error is Some; reference symbols: last_error, entries,
apply_config_change, send_to_sm_worker, command_batch.
In `@d-engine-core/src/raft_role/candidate_state.rs`:
- Around line 366-374: The context message for the RaftEvent::FlushReadBuffer
match arm is incorrect—replace the snapshot text with a message that reflects
flushing the read buffer; update the context passed to
ConsensusError::RoleViolation (in the RaftEvent::FlushReadBuffer arm inside
candidate_state.rs) to something like "Candidate node {id} attempted to flush
read buffer." while keeping current_role "Candidate", required_role "Leader",
and using ctx.node_id in the format! call so the error accurately describes the
operation and node.
In `@d-engine-core/src/raft_role/role_state.rs`:
- Around line 125-137: Remove the stale `bypass_queue` reference from the
docblock that begins "Immidiatelly verifies leadership..." in role_state.rs:
update the parameters list to match the current function signature (remove the
`bypass_queue` bullet), and while editing correct the typo "Immidiatelly" to
"Immediately" so the doc and parameter list (e.g., the `payloads`, `ctx`,
`role_tx` entries) accurately reflect the implementation.
---
Duplicate comments:
In @.github/workflows/dependency-audit.yml:
- Around line 15-21: The workflow sets RUSTUP_TOOLCHAIN to "stable" which can
drift from the workspace MSRV; update the env value RUSTUP_TOOLCHAIN (and/or the
dtolnay/rust-toolchain usage) to pin the toolchain to the workspace MSRV (e.g.,
"1.88") so CI uses the same rust-version declared in the workspace (rust-version
= "1.88"), ensuring consistency across dependency-audit runs.
In `@Cargo.toml`:
- Around line 29-34: Confirm that the specified rust-version = "1.88" and
edition = "2024" are compatible by verifying that Rust 1.88 is a released stable
toolchain and supports the 2024 edition; if 1.88 is not yet released or lacks
needed features, update rust-version in Cargo.toml (or add a rust-toolchain
file) to the correct MSRV and change edition if necessary. Also ensure the CI
workflow is pinned to that MSRV (update GitHub Actions/CI config to use the same
toolchain) so the workspace, workspace-level edition = "2024", and crate builds
are reproducible; run cargo check/test locally with the chosen toolchain to
validate before committing.
In `@d-engine-core/src/config/raft.rs`:
- Around line 836-838: The default persistence strategy was changed in
default_persistence_strategy() from MemFirst to DiskFirst and this breaking
change needs to be added to the migration guide; update MIGRATION_GUIDE.md to
document that users who want the previous behavior must set persistence_strategy
= "MemFirst" in the [raft.persistence] config (reference PersistenceStrategy and
default_persistence_strategy()), and include a short example stanza and one-line
explanation matching the note already in CHANGELOG.md.
In `@d-engine-core/src/raft_role/buffers/batch_buffer_test.rs`:
- Around line 18-20: Tests are directly accessing private fields buffer and
last_flush on BatchBuffer; either add small public accessor methods on
BatchBuffer (e.g., len()/is_empty(), capacity(), and
time_since_last_flush()/last_flush_elapsed()) and update the tests to call those
methods, or move these tests into the same module as BatchBuffer (so they can
access private fields) and remove direct field accesses; refer to the
BatchBuffer type and its private fields buffer and last_flush when making the
change and update all occurrences noted (lines around the assertions) to use the
new accessors or to live inside the defining module.
In `@d-engine-core/src/raft_role/leader_state_test/client_write_test.rs`:
- Around line 489-570: The test test_drain_single_write_no_delay uses a brittle
assert on elapsed.as_millis() < 10; remove or relax this wall‑clock assertion
and instead assert behaviorally (e.g., use tokio::time::timeout when awaiting
rx.recv() or increase the threshold) so the test no longer flakes on slow CI.
Concretely, in test_drain_single_write_no_delay replace the Instant/elapsed
check around state.flush_cmd_buffers and the final assert with either a
tokio::time::timeout wrapping rx.recv() to ensure the write completes within a
reasonable window, or change the numeric bound to a much larger value; keep
calls to state.flush_cmd_buffers and rx.recv() unchanged. Use the identifiers
test_drain_single_write_no_delay, state.flush_cmd_buffers, and rx.recv to locate
the change.
In `@d-engine-core/src/raft_role/learner_state_test.rs`:
- Around line 1565-1573: The test is fragile due to the hardcoded NewCommitData
{ current_term: 1 } in the mock expectation; update the test to use the actual
term value used by the test (e.g., the variable or getter set up earlier)
instead of the literal 1 so the expectation matches runtime state changes —
replace the literal current_term: 1 in the
mock_sm_handler.expect_should_snapshot().with(eq(NewCommitData { ... })) call
with the test's current_term variable or derive it from the component under test
(referencing NewCommitData, mock_sm_handler, and expect_should_snapshot to
locate the assertion).
In `@d-engine-core/src/raft_role/learner_state.rs`:
- Around line 668-686: In can_purge_logs, the commit_check currently uses a
strict `<` which prevents purging when commit_index equals
last_included_in_request.index; update the commit_check to use `<=` (i.e., check
last_included_in_request.index <= self.commit_index()) so purging is allowed
immediately after snapshot creation; leave the monotonic_check logic using
last_purge_index and last_included_in_request.index unchanged.
In `@d-engine-core/src/raft_test/drain_based_batch_architecture_tests.rs`:
- Around line 78-82: The strict wall‑clock assertions checking drain latency
("Drain latency should be <5ms" and the similar one at the other location) are
flaky; relax them to a more forgiving threshold or convert to a timeout-style
check. Update the assert! statements that compare elapsed to
Duration::from_millis(5) (and the duplicate at the other location) to use a
larger bound (e.g., Duration::from_millis(50) or 100) or change the assertion to
wait with a deadline loop that retries until the duration passes or a longer
timeout elapses; keep the original error message but update it to reflect the
new threshold (e.g., "Drain latency should be <50ms, got {elapsed:?}"). Ensure
you modify both occurrences referenced by the existing assert! call sites so the
tests are less sensitive to CI jitter.
---
Nitpick comments:
In `@benches/reports/v0.2.3/bench_report_v0.2.3.md`:
- Line 102: Update the table row that currently reads "Standalone slight
regressions | Within noise floor of local Mac disk I/O variability" to more
accurately reflect the observed outliers: mention that while some regressions
fall within disk I/O variability, the +26% p99 increase for eventual reads is an
exception and should be called out separately (e.g., add a parenthetical note or
a second row referencing "eventual reads: +26% p99 — exceeds typical noise
floor"). Edit the text surrounding the table to include a brief sentence
referencing the specific metric (p99 eventual reads) and recommending further
investigation or caveats for that case.
In `@d-engine-client/src/grpc_client_test.rs`:
- Around line 1441-1453: Extract the async helper make_client into module scope
so other tests can reuse it: move the async fn make_client(port: u16) ->
GrpcClient (and its use of ClientConfig::default(), ConnectionPool::create,
ArcSwap and ClientInner) out of the local test body into the top-level of the
test module (make it pub(crate) or non-pub as appropriate), keep its signature
async so callers can await it, and update earlier tests to call this shared
make_client instead of duplicating the GrpcClient setup.
In `@d-engine-core/benches/leader_state_bench.rs`:
- Around line 172-179: The mock's apply_chunk handler in MockStateMachine
currently always returns Ok(vec![]), which under‑exercises result handling;
change the expect_apply_chunk stub (the closure passed to
MockStateMachine::expect_apply_chunk / apply_chunk) to return a Vec with one
result per input entry (e.g., map the input slice/vec length to a same‑length
Vec of appropriate Result/Item placeholders) so the benchmark sees per‑entry
results instead of an empty vector; update the closure signature used in
expect_apply_chunk to inspect its input parameter and construct a
Vec::with_capacity(input.len()) filled with the expected per‑entry values.
In `@d-engine-core/src/commit_handler/default_commit_handler_test.rs`:
- Around line 96-108: The test currently drains the state-machine (SM) channel
before the mock is exercised, so MockStateMachineHandler's expect_apply_chunk
and command_hook are never invoked; update the test to either (A) wire the
in-test SM worker to forward the SM channel messages into the mock handler (so
functions like MockStateMachineHandler::expect_pending_range and
::expect_apply_chunk are actually called) by replacing the current discarding
worker with one that sends received entries to the mock, or (B) if you only
intend to test the commit-handler without SM behavior, remove the expectations
on expect_apply_chunk/command_hook and any related mocks
(MockStateMachineHandler) to avoid dead expectations; locate usages around
MockStateMachineHandler, expect_pending_range, expect_apply_chunk, and
command_hook in the test and apply one of these two changes.
In `@d-engine-core/src/config/raft.rs`:
- Around line 1329-1383: Add a validation to MetricsConfig to reject sample_rate
== 0 and invoke it from RaftConfig::validate: implement pub fn validate(&self)
-> Result<()> on MetricsConfig that returns an appropriate Config error if
self.sample_rate == 0 (message like "metrics.sample_rate must be > 0"), then
update RaftConfig::validate() to call metrics_config.validate()? so the config
fails fast before BackpressureMetrics::new is reached. Ensure you reference
MetricsConfig::validate and RaftConfig::validate in the change.
In `@d-engine-core/src/raft_role/buffers/batch_buffer.rs`:
- Around line 65-76: The current take_all() uses std::mem::take(&mut
self.buffer) which drops capacity and forces reallocation; change it to swap the
buffer with a new Vec created with the same capacity so capacity is preserved.
In the take_all method (referencing take_all, self.buffer, and self.last_flush)
create a new Vec with_capacity(self.buffer.capacity()) and swap or replace it
with self.buffer to extract the items while keeping capacity for future pushes;
keep the metrics update logic (metrics_enabled, metrics_labels) unchanged and
return the extracted items.
In `@d-engine-core/src/raft_role/leader_state_test/fatal_error_test.rs`:
- Around line 47-50: The test function name
test_leader_handles_fatal_error_notifies_pending_requests is misleading because
this test does not enqueue or assert behavior for pending requests; rename the
test function (e.g., to test_leader_handles_fatal_error_sends_shutdown_signal or
test_leader_handles_fatal_error_notifies_shutdown) to accurately reflect that it
verifies leader fatal error handling/shutdown notification rather than
pending-request notification; update the async test function declaration for
test_leader_handles_fatal_error_notifies_pending_requests to the chosen clearer
name so test output and coverage match the behavior.
In `@d-engine-core/src/raft_role/leader_state.rs`:
- Around line 888-943: The current logic applies backpressure per-queue (see
lease_read_queue, eventual_read_queue and linearizable_read_buffer elsewhere)
causing up to 3× max_pending_reads to be allowed; to enforce an aggregate limit,
compute total_pending_reads = self.linearizable_read_buffer.len() +
self.lease_read_queue.len() + self.eventual_read_queue.len() and use
backpressure.should_reject_read(total_pending_reads) (and record buffer
utilization using that total) before pushing into any queue; update the
record_rejection and metrics.record_buffer_utilization calls to use the
aggregated value and ensure backpressure_metrics is consulted once per incoming
read request (instead of per-queue) so all read policies share the same limit.
- Around line 2457-2484: The OR-combine of wait_for_apply_event in
merge_batch_to_write_metadata can hide mixed-flag batches; add a debug-only
invariant check inside merge_batch_to_write_metadata that verifies all
RaftRequestWithSignal in the incoming batch share the same wait_for_apply_event
value before you OR them into any_wait_for_apply (e.g., capture the first
request's wait_for_apply_event, compare each subsequent req.wait_for_apply_event
and use debug_assert! or a cfg(debug_assertions) check to fail fast if a
mismatch is detected); keep the existing OR logic for release builds but ensure
the assertion references RaftRequestWithSignal::wait_for_apply_event,
any_wait_for_apply and the function name merge_batch_to_write_metadata so it’s
easy to find.
In `@d-engine-core/src/raft_role/learner_state_test.rs`:
- Around line 565-568: Add a descriptive doc comment above the async test
function test_learner_rejects_join_cluster that explains the scenario and
expected behavior (e.g., that a learner node should reject a JoinCluster RPC or
join attempt, under what conditions, and the expected response or state change),
matching the style of other tests in the file; include a one-line summary and a
short sentence describing the setup and assertion so future readers and test
reports clearly convey intent.
In `@d-engine-core/src/raft_test/raft_comprehensive_tests.rs`:
- Around line 961-974: The test test_reprocess_event_requeues currently only
checks raft role and does not validate that a ReprocessEvent is actually
re-queued; update the test to exercise the requeue path by triggering the
component that emits ReprocessEvent (use MockBuilder/Mock raft helpers to inject
or call the handler that enqueues ReprocessEvent), then receive from the
event_rx (or the mocked event channel) and assert the received message is a
ReprocessEvent (or expected payload); if the requeue behavior is not
implementable yet, replace or augment the test with a clear TODO comment
indicating the missing assertion and why it cannot be added now.
In `@d-engine-core/src/replication/mod.rs`:
- Around line 45-53: Add a constructor to enforce the invariant that
senders.len() == payloads.len() for the public struct containing the fields
senders, payloads and wait_for_apply_event: implement a pub fn new(payloads:
Vec<...>, senders: Vec<...>, wait_for_apply_event: bool) -> Self that performs a
runtime check (e.g. debug_assert_eq!(senders.len(), payloads.len()) or a
panic/assert in non-debug builds) and returns Self with the fields set; use the
constructor throughout (and consider making the struct fields non-pub if you
want to force callers to use new) so misuse is prevented for senders, payloads
and wait_for_apply_event.
Contributor
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
Author
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
- backpressure_test: assert tonic::Code::ResourceExhausted instead of is_err() for all 5 backpressure rejection checks - batch_buffer_test: replace wall-clock thresholds with before/after bounds to prevent flakiness on slow CI hosts - client_write_test: relax 10ms threshold to 100ms for drain latency check
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
benches/embedded-bench/src/main.rs (1)
466-505:⚠️ Potential issue | 🟡 MinorAvoid follower deleting the done-signal file (race → hang).
If a follower starts after the leader finishes, it deletes the signal and then waits forever. Only the leader should clean up the file, and followers should treat an existing file as “done”.🛠️ Proposed fix (leader-only cleanup + follower short‑circuit)
- // Clean up any leftover signal file from a previous run - let _ = std::fs::remove_file(done_signal_path); - - if engine.is_leader() { + if engine.is_leader() { + // Clean up any leftover signal file from a previous run + let _ = std::fs::remove_file(done_signal_path); run_batch_tests(&engine, &cli).await; // Signal followers to auto-shutdown if let Err(e) = std::fs::write(done_signal_path, "done") { eprintln!("Failed to write done signal: {e}"); } } else { + if std::path::Path::new(done_signal_path).exists() { + println!("Benchmark already completed, shutting down follower."); + } else { println!( "This node is Follower, keeping cluster membership alive during batch tests..." ); println!("Will auto-shutdown when leader completes benchmarks."); loop { tokio::select! { _ = shutdown_rx.changed() => break, _ = tokio::time::sleep(Duration::from_secs(1)) => { if std::path::Path::new(done_signal_path).exists() { println!("Benchmark completed, shutting down follower."); break; } } } } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/embedded-bench/src/main.rs` around lines 466 - 505, The follower may delete the done_signal_path and race with a late-starting follower, causing a hang; ensure only the leader performs cleanup and followers short‑circuit if the signal already exists. Move/remove the unconditional std::fs::remove_file(done_signal_path) so it runs only when engine.is_leader() (referencing done_signal_path and engine.is_leader()), and in the follower branch (before entering the loop that watches shutdown_rx) check std::path::Path::new(done_signal_path).exists() and immediately treat it as “done” (print and return/exit the follower path) instead of waiting; keep the leader’s write of done_signal_path after run_batch_tests(&engine, &cli).await.d-engine-core/src/replication/mod.rs (1)
39-54:⚠️ Potential issue | 🔴 CriticalAdd a constructor to enforce the payload/sender length invariant.
This public struct with parallel vectors allows callers to easily create
RaftRequestWithSignalwith mismatched lengths. Multiple call sites already violate the invariant—for example,leader_state.rs:618and test code construct requests with multiplepayloadsbut only a single sender. Whenwait_for_apply_eventis true (line 2555–2556), only entries with corresponding senders get added topending_requests, causing responses to be lost. When false (line 2559–2561), extra payloads go unreplied.A constructor with
debug_assert_eq!(payloads.len(), senders.len())would catch mismatches early:Minimal constructor to guard invariants
pub struct RaftRequestWithSignal { #[allow(unused)] pub id: String, pub payloads: Vec<EntryPayload>, /// Multiple senders for merged requests (1 sender per payload, matched by index) /// Invariant: senders.len() == payloads.len() pub senders: Vec<MaybeCloneOneshotSender<std::result::Result<ClientResponse, Status>>>, /// Does this request need to wait for StateMachine's ApplyCompleted event? /// /// - `true`: Command payload → must wait for state machine apply /// - `false`: Noop/Config payload → respond immediately after commit pub wait_for_apply_event: bool, } + +impl RaftRequestWithSignal { + pub fn new( + id: String, + payloads: Vec<EntryPayload>, + senders: Vec<MaybeCloneOneshotSender<std::result::Result<ClientResponse, Status>>>, + wait_for_apply_event: bool, + ) -> Self { + debug_assert_eq!(payloads.len(), senders.len(), "payload/sender length mismatch"); + Self { id, payloads, senders, wait_for_apply_event } + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/replication/mod.rs` around lines 39 - 54, Add a constructor for RaftRequestWithSignal (e.g., pub fn new(id: String, payloads: Vec<EntryPayload>, senders: Vec<MaybeCloneOneshotSender<Result<ClientResponse, Status>>>, wait_for_apply_event: bool) -> Self) that asserts the invariant payloads.len() == senders.len() (use debug_assert_eq! for debug builds) and returns the struct with those fields; update call sites to use RaftRequestWithSignal::new so the mismatch is caught early and to prevent lost/unreplied payloads when wait_for_apply_event is true or false, and consider making the struct fields non-public if you want to force use of the constructor.
🧹 Nitpick comments (9)
d-engine-core/src/config/config_test.rs (1)
367-376: Consider adding#[serial]for test isolation.This test calls
cleanup_all_raft_env_vars()which manipulates global environment variables, but it lacks the#[serial]attribute. When tests run in parallel, this could cause intermittent failures. The same applies totest_explicit_override_config_fileat line 517.Proposed fix
#[test] +#[serial] fn test_new_returns_unvalidated_config() {And at line 517:
#[test] +#[serial] fn test_explicit_override_config_file() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/config/config_test.rs` around lines 367 - 376, The tests that call cleanup_all_raft_env_vars() (notably test_new_returns_unvalidated_config and test_explicit_override_config_file) mutate global environment state and must be run serially; add the #[serial] attribute to each of these test functions (above their function declarations) so they do not run in parallel, ensuring isolation when calling cleanup_all_raft_env_vars() and when using RaftNodeConfig::new().CONTRIBUTING.md (1)
80-88: LGTM - Rebase instructions follow best practices.The new section provides clear guidance for keeping PRs synchronized. The use of
--force-with-leaseis the recommended safe approach for force-pushing, as it prevents accidentally overwriting others' work.Optional: Consider adding upstream remote setup instructions
For contributors unfamiliar with fork-based workflows, you could optionally add a brief note about setting up the upstream remote before the rebase commands:
### Keeping Your PR Up-to-Date -If `main` has moved forward since you created your branch, rebase before requesting review: +If `main` has moved forward since you created your branch, rebase before requesting review. + +First-time setup (if you haven't added the upstream remote): + +```bash +git remote add upstream https://github.com/deventlab/d-engine.git +``` + +Then rebase: ```bash git fetch upstreamHowever, this is a standard Git workflow assumption and not essential.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 80 - 88, Add an optional brief note under the "Keeping Your PR Up-to-Date" section explaining how contributors using forks should set an upstream remote that points to the main repository before running the fetch/rebase/push steps (clarify that this step precedes git fetch/rebase and show that it is an upstream remote pointing to the main repo), and place this guidance in a collapsible details block labeled like the existing optional suggestion so it’s visible but not required..github/workflows/dependency-audit.yml (1)
15-16: Consider documenting the rationale for usingstableinstead of pinned MSRV.The workspace declares
rust-version = "1.88", but this workflow explicitly usesRUSTUP_TOOLCHAIN: stable. While using stable for dependency audits is reasonable (to catch issues with latest toolchain), the inconsistency could cause confusion. Consider adding a comment explaining the intentional choice, or pin to 1.88 for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/dependency-audit.yml around lines 15 - 16, The workflow sets RUSTUP_TOOLCHAIN to "stable" which conflicts with the workspace's rust-version = "1.88"; either add a clear inline comment next to the RUSTUP_TOOLCHAIN env key explaining why "stable" is intentionally used for audits (to run against the latest toolchain), or change RUSTUP_TOOLCHAIN to the pinned version "1.88" to match the workspace; reference the env key RUSTUP_TOOLCHAIN in the dependency-audit workflow and the workspace's rust-version = "1.88" in your explanation or change.d-engine-core/src/raft_test/raft_comprehensive_tests.rs (1)
37-54: Consider using theNodeRoleenum instead of magic numbers.The helper functions use hardcoded integers for role identification, which could drift from the actual
NodeRoleenum values.♻️ Suggested improvement
+use d_engine_proto::common::NodeRole; + fn is_candidate(role_i32: i32) -> bool { - // CANDIDATE = 1 - role_i32 == 1 + role_i32 == NodeRole::Candidate as i32 } fn is_leader(role_i32: i32) -> bool { - // LEADER = 2 - role_i32 == 2 + role_i32 == NodeRole::Leader as i32 } fn is_learner(role_i32: i32) -> bool { - // LEARNER = 3 - role_i32 == 3 + role_i32 == NodeRole::Learner as i32 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_test/raft_comprehensive_tests.rs` around lines 37 - 54, The helper functions is_follower, is_candidate, is_leader, and is_learner use hardcoded integers; replace those magic numbers by comparing the incoming role_i32 (or its converted value) against the NodeRole enum variants (e.g., NodeRole::Follower, NodeRole::Candidate, NodeRole::Leader, NodeRole::Learner) by converting role_i32 into the enum (or mapping via TryFrom/i32) before comparison and handle invalid conversions safely so the checks always reflect the actual NodeRole definitions.d-engine-client/src/mock_rpc_service.rs (1)
228-284: Consider extracting the repeatedClusterMembershipbuilder.The same closure pattern for building
ClusterMembershipis duplicated across allsimulate_*methods. A shared helper would reduce boilerplate.♻️ Example refactor
impl MockNode { fn default_cluster_membership_builder() -> Box<dyn Fn(u16) -> Result<ClusterMembership, tonic::Status> + Send + Sync> { Box::new(|port: u16| { Ok(ClusterMembership { version: 1, nodes: vec![NodeMeta { id: 1, role: NodeRole::Leader as i32, address: format!("127.0.0.1:{port}"), status: NodeStatus::Active.into(), }], current_leader_id: Some(1), }) }) } // Then use in simulate_* methods: // let builder = Self::default_cluster_membership_builder(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-client/src/mock_rpc_service.rs` around lines 228 - 284, Extract the duplicated ClusterMembership closure into a single helper on MockNode (e.g., fn default_cluster_membership_builder() -> Box<dyn Fn(u16) -> Result<ClusterMembership, tonic::Status> + Send + Sync>) and replace the local let builder = Box::new(...) in simulate_watch_mock_server and simulate_watch_error_mock_server with let builder = Self::default_cluster_membership_builder(); ensure the helper returns the same ClusterMembership construction and is used wherever the closure was duplicated.benches/standalone-bench/src/main.rs (1)
241-268: The bench code is correct and will compile successfully. TheClienttype dereferences toGrpcClient, which exposes theget_with_policy()method as a public API. However, note that this creates an implementation inconsistency:GrpcClientprovidesget_with_policy()whileEmbeddedClientprovidesget_with_consistency()instead. Both are outside theClientApitrait and serve as convenience methods. If future code needs to work generically withClientApitrait objects, the bench's consistency parameter logic will need to be abstracted into the trait itself or unified across implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/standalone-bench/src/main.rs` around lines 241 - 268, Bench uses GrpcClient::get_with_policy while EmbeddedClient exposes get_with_consistency, causing an API inconsistency that prevents generic use via ClientApi; fix by adding a unified method to the ClientApi trait (e.g., get_with_consistency or get_with_policy) and implement it for both GrpcClient and EmbeddedClient so the bench can call the trait method on Client (instead of calling GrpcClient-specific get_with_policy), or alternatively rename/forward the existing methods in each implementation to the chosen unified name so both clients present the same convenience API.d-engine-core/src/raft_role/buffers/batch_buffer.rs (1)
83-85: Consider adding#[must_use]tolen()to catch unused results.This is a minor suggestion to improve API ergonomics and catch potential bugs where callers ignore the length value.
🔧 Optional: Add #[must_use] attribute
+ #[must_use] pub fn len(&self) -> usize { self.buffer.len() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/buffers/batch_buffer.rs` around lines 83 - 85, Add the #[must_use] attribute to the BatchBuffer::len method to ensure callers don’t accidentally ignore the returned usize; modify the function signature for pub fn len(&self) -> usize (the len method in batch_buffer.rs) by adding #[must_use] immediately above it so the compiler will warn when the length result is unused.d-engine-core/src/raft_role/follower_state.rs (1)
398-405: Consider using consistent memory ordering for the flag check and set.The code uses
Ordering::Acquirefor the load andOrdering::Releasefor the store, which is correct for establishing a happens-before relationship. However, this pattern doesn't prevent a race where two concurrent calls both seefalsebefore either setstrue.For a single-threaded event loop (typical Raft design), this is fine. If concurrent access is possible, consider using
compare_exchangefor atomicity.🔧 Optional: Use compare_exchange for stronger guarantees
- if self.snapshot_in_progress.load(Ordering::Acquire) { + if self + .snapshot_in_progress + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_err() + { info!( "Follower snapshot creation already in progress. Skipping duplicate request." ); return Ok(()); } - - self.snapshot_in_progress.store(true, Ordering::Release);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/follower_state.rs` around lines 398 - 405, The snapshot_in_progress flag check uses non-atomic check-then-set (self.snapshot_in_progress.load(...) followed by store(...)) which can race; replace this with an atomic compare_exchange (e.g., self.snapshot_in_progress.compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire)) so only the first caller wins and others observe the set; update the Ordering to use Acquire/Release (or AcqRel for the successful swap) as appropriate and handle the Err case by skipping/returning when compare_exchange fails.d-engine-client/src/grpc_client.rs (1)
267-271: Consider simplifying redundant error conversion.The error conversion
Into::<ClientApiError>::into(ClientApiError::from(status))is redundant. SinceClientApiErroralready implementsFrom<Status>, this can be simplified:Suggested simplification
Err(status) => { error!("[:GrpcClient:write] status: {:?}", status); - Err(Into::<ClientApiError>::into(ClientApiError::from(status))) + Err(status.into()) }The same pattern appears at lines 305-308, 364-367, and 405-408.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-client/src/grpc_client.rs` around lines 267 - 271, The error conversion is redundant: instead of calling Into::<ClientApiError>::into(ClientApiError::from(status)) in the Err(status) match arms, directly convert the gRPC status with either ClientApiError::from(status) or status.into(); update the Err branches in the write function and the other similar match arms (the ones using ClientApiError and Status at the repeated locations) to return Err(ClientApiError::from(status)) or Err(status.into()) for brevity and clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@d-engine-core/src/commit_handler/default_commit_handler.rs`:
- Around line 281-297: The send_to_sm_worker function currently sends entries
into an unbounded mpsc (sm_apply_tx) which can grow unbounded if the SM worker
is slow; change the channel creation to a bounded channel (e.g.,
mpsc::channel(capacity)) and update send_to_sm_worker to use the async
.send().await (or try_send with backoff/metrics) so backpressure is applied when
sending entries (std::mem::take(batch) / entries) and the sender will await or
handle a full queue; alternatively add queue-depth monitoring and shedding logic
around sm_apply_tx.send to drop or reject excess entries with appropriate
logging/error handling.
---
Outside diff comments:
In `@benches/embedded-bench/src/main.rs`:
- Around line 466-505: The follower may delete the done_signal_path and race
with a late-starting follower, causing a hang; ensure only the leader performs
cleanup and followers short‑circuit if the signal already exists. Move/remove
the unconditional std::fs::remove_file(done_signal_path) so it runs only when
engine.is_leader() (referencing done_signal_path and engine.is_leader()), and in
the follower branch (before entering the loop that watches shutdown_rx) check
std::path::Path::new(done_signal_path).exists() and immediately treat it as
“done” (print and return/exit the follower path) instead of waiting; keep the
leader’s write of done_signal_path after run_batch_tests(&engine, &cli).await.
In `@d-engine-core/src/replication/mod.rs`:
- Around line 39-54: Add a constructor for RaftRequestWithSignal (e.g., pub fn
new(id: String, payloads: Vec<EntryPayload>, senders:
Vec<MaybeCloneOneshotSender<Result<ClientResponse, Status>>>,
wait_for_apply_event: bool) -> Self) that asserts the invariant payloads.len()
== senders.len() (use debug_assert_eq! for debug builds) and returns the struct
with those fields; update call sites to use RaftRequestWithSignal::new so the
mismatch is caught early and to prevent lost/unreplied payloads when
wait_for_apply_event is true or false, and consider making the struct fields
non-public if you want to force use of the constructor.
---
Duplicate comments:
In `@benches/reports/v0.2.3/bench_report_v0.2.3.md`:
- Line 3: The Date line "**Date**: February 18, 2026" is missing a comma after
the year; update the date string to include a trailing comma (e.g., "**Date**:
February 18, 2026,") so the formatting matches the project's style guidelines
and previous entries.
In `@d-engine-core/src/commit_handler/default_commit_handler.rs`:
- Around line 175-187: The SM worker send path can re-queue overlapping entries
because pending_range() still uses last_applied and the code in
default_commit_handler (around send_to_sm_worker, command_batch,
apply_config_change and handling of Payload::Noop) pushes entries then awaits
without marking them in-flight; update this by marking the range as in-flight
before awaiting the async send: add an in-flight marker (e.g., in_flight_end or
a tracked in_flight_ranges set) when you push entries into command_batch and
before calling send_to_sm_worker, advance/record the in-flight boundary so
pending_range() will skip those entries, and ensure you clear or advance that
marker when ApplyCompleted/apply_config_change returns (or on error) so retries
work correctly.
- Around line 193-199: The remaining command_batch is dropped when a config
change sets last_error and the function returns early; ensure command_batch is
forwarded to the SM worker even if last_error is Some. Change the final block
around last_error/command_batch so you always call self.send_to_sm_worker(&mut
command_batch).await before returning: if let Some(e) = last_error { let
send_res = self.send_to_sm_worker(&mut command_batch).await; if let
Err(send_err) = send_res { return Err(send_err); } return Err(e); } else {
self.send_to_sm_worker(&mut command_batch).await?; } - reference last_error,
command_batch, and send_to_sm_worker in default_commit_handler.rs.
In `@d-engine-core/src/config/raft.rs`:
- Around line 835-838: Default persistence strategy was changed to DiskFirst in
the function default_persistence_strategy() returning
PersistenceStrategy::DiskFirst, which is a breaking change for deployments
expecting MemFirst; update the migration/upgrade documentation and configuration
guidance to explicitly call out this change and show how to opt back into the
previous behavior by setting persistence_strategy = "MemFirst" under
[raft.persistence], and add a short note near the default_persistence_strategy()
function (e.g., doc comment) referencing the migration entry so maintainers and
users can find the rollback/config override quickly.
In `@d-engine-core/src/raft_role/buffers/batch_buffer_test.rs`:
- Around line 75-79: Replace the fragile tight elapsed assertion by capturing
timestamps before and after the operation and asserting buffer.last_flush falls
between them; specifically, around the call to buffer.take_all() record let
before = Instant::now() then call buffer.take_all() and record let after =
Instant::now(), and replace assert!(buffer.last_flush.elapsed() <
Duration::from_millis(50)) with an assertion that buffer.last_flush >= before &&
buffer.last_flush <= after (optionally allowing a small epsilon), and make the
same change for the similar assertions covering lines 136-143 so both tests use
before/after bounds rather than a single small elapsed threshold.
- Around line 14-21: The test is reaching into BatchBuffer internals
(buffer.buffer and buffer.last_flush); change assertions to use the public API:
call buffer.is_empty() instead of checking buffer.buffer.is_empty(),
buffer.len() (or assert_eq!/cmp) instead of buffer.buffer.len(), and
buffer.capacity() if a public capacity method exists; for the last_flush check,
use an existing public accessor or add a small public helper on BatchBuffer
(e.g., last_flush_elapsed() or since_last_flush()) that returns a Duration so
the test can assert it is < Duration::from_secs(1) without touching the
last_flush field directly.
In `@d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs`:
- Around line 67-69: The test currently only checks response.is_err() for
messages received from resp_rx in backpressure_test.rs; change each of those
assertions (e.g., the response from resp_rx.recv().await.unwrap() at the
locations flagged) to assert the specific backpressure error variant/kind
instead of a generic is_err(): unwrap the Err value (or use expect_err) and
match it against the concrete backpressure error variant or compare the
error.kind()/error.code() to the Backpressure variant/enum used by your crate so
the test fails only if a different error is returned; apply the same change to
the other occurrences mentioned (around the other recv() assertions).
In `@d-engine-core/src/raft_role/leader_state_test/client_write_test.rs`:
- Around line 566-570: The timing assertion using elapsed.as_millis() < 10 in
the test should be relaxed or removed because it's brittle; update the test in
client_write_test.rs by removing or loosening the assert! using elapsed and
instead assert on behavioral outcomes: verify the write response was received
and the leader's send buffer (or request buffer used in the test) is
empty/cleared after processing; if you must keep a timing check, increase the
threshold to a generous value (e.g., 200-500ms) and make it non-fatal by only
warning or using a looser assert to avoid CI flakes.
In `@d-engine-core/src/raft_role/leader_state.rs`:
- Around line 841-852: client_command_to_entry_payloads(vec![cmd]) is unwrapped
with .expect(), which can panic if it returns an empty Vec; instead handle the
empty case defensively: call client_command_to_entry_payloads(vec![cmd]) and
check if .into_iter().next() yields Some(payload) before calling
self.propose_buffer.push(payload, sender), and if it yields None send
Err(Status::internal("Failed to convert client command to entry payload")) (or
similar) via sender to reject the request; reference:
client_command_to_entry_payloads, propose_buffer.push, sender, and req.command.
In `@d-engine-core/src/raft_role/learner_state.rs`:
- Around line 668-687: In can_purge_logs, the commit_check uses strict `<` which
prevents purging when last_included_in_request.index equals commit_index; change
the commit_check to use `<=` so the function permits purge at the snapshot
boundary (i.e., set commit_check to compare last_included_in_request.index <=
self.commit_index()). Keep the existing monotonic_check
(last_purge_index.map(|lid| lid.index <
last_included_in_request.index).unwrap_or(true)) unchanged to enforce monotonic
purge.
In `@d-engine-core/src/raft_test/drain_based_batch_architecture_tests.rs`:
- Around line 79-82: The strict wall-clock assertion using `elapsed <
Duration::from_millis(5)` in the test should be relaxed to avoid CI flakiness:
update the assertion that references `elapsed` (in
drain_based_batch_architecture_tests.rs) to use a higher, more tolerant
threshold (e.g., 50–100ms) or replace with a relative/assertion that allows
jitter, and add a short comment noting why the threshold is relaxed;
alternatively, if feasible, switch the test to use a deterministic/mockable
timer instead of real time to assert behavior reliably.
- Around line 494-499: The strict wall-clock assertion using elapsed.as_millis()
< 10 is flaky; relax the timing check on the elapsed variable (from the test in
drain_based_batch_architecture_tests.rs around the write rejection assertion) to
a more CI-tolerant threshold (e.g., compare elapsed to
Duration::from_millis(100) or 50) or remove the timing assertion entirely and
instead assert the response is an immediate rejection by checking the rejection
type/flag; update the assert! that references elapsed.as_millis() to use elapsed
< Duration::from_millis(<new_threshold>) and keep the existing error message.
---
Nitpick comments:
In @.github/workflows/dependency-audit.yml:
- Around line 15-16: The workflow sets RUSTUP_TOOLCHAIN to "stable" which
conflicts with the workspace's rust-version = "1.88"; either add a clear inline
comment next to the RUSTUP_TOOLCHAIN env key explaining why "stable" is
intentionally used for audits (to run against the latest toolchain), or change
RUSTUP_TOOLCHAIN to the pinned version "1.88" to match the workspace; reference
the env key RUSTUP_TOOLCHAIN in the dependency-audit workflow and the
workspace's rust-version = "1.88" in your explanation or change.
In `@benches/standalone-bench/src/main.rs`:
- Around line 241-268: Bench uses GrpcClient::get_with_policy while
EmbeddedClient exposes get_with_consistency, causing an API inconsistency that
prevents generic use via ClientApi; fix by adding a unified method to the
ClientApi trait (e.g., get_with_consistency or get_with_policy) and implement it
for both GrpcClient and EmbeddedClient so the bench can call the trait method on
Client (instead of calling GrpcClient-specific get_with_policy), or
alternatively rename/forward the existing methods in each implementation to the
chosen unified name so both clients present the same convenience API.
In `@CONTRIBUTING.md`:
- Around line 80-88: Add an optional brief note under the "Keeping Your PR
Up-to-Date" section explaining how contributors using forks should set an
upstream remote that points to the main repository before running the
fetch/rebase/push steps (clarify that this step precedes git fetch/rebase and
show that it is an upstream remote pointing to the main repo), and place this
guidance in a collapsible details block labeled like the existing optional
suggestion so it’s visible but not required.
In `@d-engine-client/src/grpc_client.rs`:
- Around line 267-271: The error conversion is redundant: instead of calling
Into::<ClientApiError>::into(ClientApiError::from(status)) in the Err(status)
match arms, directly convert the gRPC status with either
ClientApiError::from(status) or status.into(); update the Err branches in the
write function and the other similar match arms (the ones using ClientApiError
and Status at the repeated locations) to return
Err(ClientApiError::from(status)) or Err(status.into()) for brevity and clarity.
In `@d-engine-client/src/mock_rpc_service.rs`:
- Around line 228-284: Extract the duplicated ClusterMembership closure into a
single helper on MockNode (e.g., fn default_cluster_membership_builder() ->
Box<dyn Fn(u16) -> Result<ClusterMembership, tonic::Status> + Send + Sync>) and
replace the local let builder = Box::new(...) in simulate_watch_mock_server and
simulate_watch_error_mock_server with let builder =
Self::default_cluster_membership_builder(); ensure the helper returns the same
ClusterMembership construction and is used wherever the closure was duplicated.
In `@d-engine-core/src/config/config_test.rs`:
- Around line 367-376: The tests that call cleanup_all_raft_env_vars() (notably
test_new_returns_unvalidated_config and test_explicit_override_config_file)
mutate global environment state and must be run serially; add the #[serial]
attribute to each of these test functions (above their function declarations) so
they do not run in parallel, ensuring isolation when calling
cleanup_all_raft_env_vars() and when using RaftNodeConfig::new().
In `@d-engine-core/src/raft_role/buffers/batch_buffer.rs`:
- Around line 83-85: Add the #[must_use] attribute to the BatchBuffer::len
method to ensure callers don’t accidentally ignore the returned usize; modify
the function signature for pub fn len(&self) -> usize (the len method in
batch_buffer.rs) by adding #[must_use] immediately above it so the compiler will
warn when the length result is unused.
In `@d-engine-core/src/raft_role/follower_state.rs`:
- Around line 398-405: The snapshot_in_progress flag check uses non-atomic
check-then-set (self.snapshot_in_progress.load(...) followed by store(...))
which can race; replace this with an atomic compare_exchange (e.g.,
self.snapshot_in_progress.compare_exchange(false, true, Ordering::AcqRel,
Ordering::Acquire)) so only the first caller wins and others observe the set;
update the Ordering to use Acquire/Release (or AcqRel for the successful swap)
as appropriate and handle the Err case by skipping/returning when
compare_exchange fails.
In `@d-engine-core/src/raft_test/raft_comprehensive_tests.rs`:
- Around line 37-54: The helper functions is_follower, is_candidate, is_leader,
and is_learner use hardcoded integers; replace those magic numbers by comparing
the incoming role_i32 (or its converted value) against the NodeRole enum
variants (e.g., NodeRole::Follower, NodeRole::Candidate, NodeRole::Leader,
NodeRole::Learner) by converting role_i32 into the enum (or mapping via
TryFrom/i32) before comparison and handle invalid conversions safely so the
checks always reflect the actual NodeRole definitions.
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
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?
Merges
developintomainfor v0.2.3, retiringdevelopas an active branch.Consolidates CAS support, unified client API, three Raft correctness fixes, and
drain-based batch performance improvements.
Type:
Why Is This Needed?
Linked to #258, #263, #266, #267, #268, #270, #271, #272, #274
mainbecomes the single contribution target per CONTRIBUTING.md (#274).Checklist
Required:
make testpassesIf changing APIs:
Testing
How tested:
cargo nextest run --all-featurespassesFor bug fixes:
For performance improvements:
Does This Follow d-engine's Principles?
kv(), consolidates into singleClient)Reviewer Notes
Key changes to be aware of:
KvClient→ClientApi,StandaloneServer→StandaloneEngine— documented in CHANGELOGPersistenceStrategydefaultMemFirst→DiskFirst(Raft protocol compliance) — documented in CHANGELOGpending_reads(fix: eliminate linearizable read self-blocking in Raft event loop via event-driven pending_reads #272)EmbeddedEnginenowArc-wrapped and thread-safe (Refactor: Unify Client API Architecture with CAS Support #258)channel-driven drain loop (recv() blocks for first command → try_recv() drains
all pending). No batch_timeout delay; ~5x throughput at high load, 100K QPS
sustained. Role-specific behavior: Leader batches, Follower/Candidate reject writes.
Estimated review complexity:
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
Documentation