diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bf0fdf8..de04a011 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,14 +12,28 @@ All notable changes to this project will be documented in this file. - **ReadActor fast path for Eventual/LeaseRead** (#392): Dedicated `ReadActor` serves `EventualConsistency` and `LeaseRead` off the Raft loop, eliminating channel contention under high read concurrency. Lease Read +10.9%, Eventual Read +13.1% vs v0.2.4 (100 concurrent clients, local embedded). +- **`Command::Batch` β€” atomic multi-key writes** (#415): New `client.batch(ops)` API commits multiple Insert/Delete operations as a single Raft entry. All ops in a batch succeed or fail together. Library wrappers (e.g. service registration, distributed locks) no longer need workarounds like reserved-key encoding. + +- **`start_node()` public API** (#415): Both `EmbeddedEngine` and `StandaloneEngine` now expose `start_node(config, storage, sm)` β€” start an engine with a programmatic `RaftNodeConfig`, no config file required. For library wrappers that build their own config layer in Rust. + +- **`start_with()` accepts `impl AsRef`** (#415): Callers can now pass `&str`, `String`, `&Path`, or `PathBuf` β€” backward compatible, no migration needed. + ### Fixed - **fix(ttl) #398**: Removed `lease.enabled` flag β€” TTL expiration is always active. Fixes fatal crash when calling `put_with_ttl` without setting the (now-removed) `lease.enabled = true`. +- **πŸ›‘ Snapshot data corruption β€” label/data mismatch** (#418): `last_included` in snapshot metadata was computed by subtracting `retained_log_entries` from `last_applied`, but the snapshot data already reflected the full `last_applied` state. Followers installing such snapshots would re-apply entries 91–100 after restoring state through index 100, causing double-application and permanent cluster state divergence. Also fixes a fabricated `LogId` where `last_included.term` was copied from `last_applied.term` instead of queried from the log. Snapshot label is now always truthful: `last_included == last_applied`. + +- **`ClusterConfig::default()` now consistent with serde default** (#415): `Default::default()` previously produced an empty `initial_cluster`, but `#[serde(default)]` returned `[{id:1, ...}]`. `RaftNodeConfig::new()` uses the Rust `Default` impl, so programmatic configs always got an empty cluster. Fixed. + ### Changed - `[raft.read_actor]` replaces the previous flat `read_actor_channel_capacity` / `read_actor_max_drain` fields in `[raft]`. Update existing config files accordingly. +- **⚠️ `StateMachine::entry_term()` removed from trait** (#418): Term lookup belongs to the log layer, not the state machine. Custom `StateMachine` implementations must delete this method β€” it is no longer part of the trait. See [Migration Guide](./MIGRATION_GUIDE.md) for details. + +- **KV encoding moved out of `d-engine-core`** (#415): `ClientWriteRequest.command` is now `Option` (pre-serialized). Serialization happens in the server transport layer (embedded/standalone/gRPC handler). Core is encoding-agnostic. + --- ## [v0.2.4] - 2026-05-23 diff --git a/Cargo.lock b/Cargo.lock index de1e890b..b1d13bfe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1254,9 +1254,9 @@ checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79" [[package]] name = "memmap2" -version = "0.9.10" +version = "0.9.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "714098028fe011992e1c3962653c96b2d578c4b4bce9036e15ff220319b1e0e3" +checksum = "d1219ed1b7f229ee7104d281dd01d6802fe28bb6e95d292942c4daacdeb798c0" dependencies = [ "libc", ] diff --git a/MIGRATION_GUIDE.md b/MIGRATION_GUIDE.md index fb768c2c..63bb66a8 100644 --- a/MIGRATION_GUIDE.md +++ b/MIGRATION_GUIDE.md @@ -128,13 +128,13 @@ grep "WAL" /var/log/d-engine.log ## Timeline -| Version | WAL Format | Wire Protocol | Migration Required | -| ------------- | ------------------- | -------------------- | ------------------------------------------------------- | -| v0.1.x | Relative TTL | Compatible | - | -| v0.2.0–v0.2.2 | Absolute expiration | Compatible | βœ… Yes (clear WAL from v0.1.x) | -| v0.2.3 | Same as v0.2.0+ | **Incompatible** | βœ… Yes (protobuf enum changes + API changes) | -| v0.2.4 | Same as v0.2.0+ | Compatible (additive)| βœ… Yes (delete `snapshot/` β€” format changed to CF export)| -| v0.2.5 | Same as v0.2.0+ | Compatible (additive)| ⚠️ Minor (remove `lease.enabled` from config if present) | +| Version | WAL Format | Wire Protocol | Migration Required | +| ------------- | ------------------- | --------------------- | ------------------------------------------------------------------------------------------------------------------------------- | +| v0.1.x | Relative TTL | Compatible | - | +| v0.2.0–v0.2.2 | Absolute expiration | Compatible | βœ… Yes (clear WAL from v0.1.x) | +| v0.2.3 | Same as v0.2.0+ | **Incompatible** | βœ… Yes (protobuf enum changes + API changes) | +| v0.2.4 | Same as v0.2.0+ | Compatible (additive) | βœ… Yes (delete `snapshot/` β€” format changed to CF export) | +| v0.2.5 | Same as v0.2.0+ | Compatible (additive) | ⚠️ Minor (remove `lease.enabled`; remove `entry_term()` from custom SM; use `start_node` instead of `start_custom_with_config`) | --- @@ -463,6 +463,7 @@ Existing v0.2.3 snapshots cannot be loaded by v0.2.4 nodes. ### What Changed Watch buffer overflow is no longer silent. When a per-watcher channel fills up, the server now: + 1. Sends a `CANCELED` sentinel (`event_type = WATCH_EVENT_TYPE_CANCELED`, `error = WATCH_BUFFER_OVERFLOW`) 2. Unregisters the watcher β€” no further events are delivered on that stream @@ -494,6 +495,65 @@ See [Watch Feature Guide](https://docs.rs/d-engine/latest/d_engine/docs/server_g --- +## For v0.2.4 Users: `StateMachine::entry_term()` Removed in v0.2.5 (#418) + +### What Changed + +The `entry_term()` method has been removed from the `StateMachine` trait. Term lookup belongs +to the Raft log layer, not the state machine β€” this was always an architectural layering issue. + +### Impact + +⚠️ **Breaking for custom `StateMachine` implementations.** + +If you implemented a custom `StateMachine`, delete the `entry_term` method from your impl block. +Built-in implementations (`FileStateMachine`, `RocksDBStateMachine`, `SledStateMachine`) are +already updated and require no changes. + +```rust +// Old (v0.2.4) β€” delete this block +fn entry_term(&self, entry_id: u64) -> Option { + Some(1) +} +``` + +Compilation will fail with "method not found in trait" if you forget to remove it. + +--- + +## For v0.2.4 Users: `start_custom_with_config` β†’ `start_node()` (#415) + +### What Changed + +The public API now exposes `start_node()` directly instead of routing through a thin wrapper. + +```rust +// Old (v0.2.4) β€” removed +EmbeddedEngine::start_custom_with_config(storage, sm, config).await?; + +// New (v0.2.5) +EmbeddedEngine::start_node(config, storage, sm).await?; // embedded +StandaloneEngine::start_node(config, storage, sm, shutdown_rx).await?; // standalone +``` + +`start_node` calls `config.validate()` internally β€” callers may pass validated or unvalidated configs. + +--- + +## For v0.2.4 Users: Snapshot Label Fix (#418) + +### What Changed + +Snapshot `last_included` was previously computed by subtracting `retained_log_entries` +from `last_applied`, introducing a label/data mismatch. Fixed: `last_included == last_applied` +always. + +### Impact + +Transparent β€” no action required. Nodes that installed snapshots with the old label may have +double-applied entries; upgrading to v0.2.5 prevents this from recurring. Existing snapshots +are compatible. + --- ## For v0.2.4 Users: TTL Config Change in v0.2.5 (#398) diff --git a/d-engine-client/src/grpc_client.rs b/d-engine-client/src/grpc_client.rs index 77fb73ac..b2ffdaa6 100644 --- a/d-engine-client/src/grpc_client.rs +++ b/d-engine-client/src/grpc_client.rs @@ -1,10 +1,14 @@ -use std::sync::Arc; - +use super::ClientInner; +use crate::ClientApiError; +use crate::ClientResponseExt; +use crate::scoped_timer::ScopedTimer; use arc_swap::ArcSwap; use bytes::Bytes; +use d_engine_core::BatchOp; use d_engine_core::ScanResult; use d_engine_core::client::ErrorCode; use d_engine_core::client::KvEntry; +use d_engine_core::client::{ClientApi, ClientApiResult}; use d_engine_core::config::ReadConsistencyPolicy; use d_engine_proto::client::ClientReadRequest; use d_engine_proto::client::ClientWriteRequest; @@ -15,21 +19,20 @@ use d_engine_proto::client::WatchRequest; use d_engine_proto::client::WatchResponse; use d_engine_proto::client::WriteCommand; use d_engine_proto::client::raft_client_service_client::RaftClientServiceClient; +use d_engine_proto::client::write_command::BatchOp as ProtoBatchOp; +use d_engine_proto::client::write_command::Delete; +use d_engine_proto::client::write_command::Insert; +use d_engine_proto::client::write_command::batch_op; use rand::Rng; use rand::SeedableRng; use rand::rngs::StdRng; +use std::sync::Arc; use tonic::codec::CompressionEncoding; use tonic::transport::Channel; use tracing::debug; use tracing::error; use tracing::warn; -use super::ClientInner; -use crate::ClientApiError; -use crate::ClientResponseExt; -use crate::scoped_timer::ScopedTimer; -use d_engine_core::client::{ClientApi, ClientApiResult}; - /// gRPC-based KV client for standalone mode. Obtained via [`crate::ClientBuilder`]. /// /// For embedded mode use `EmbeddedClient` instead. Both implement `ClientApi`. @@ -354,6 +357,59 @@ impl ClientApi for GrpcClient { } } + async fn batch( + &self, + ops: Vec, + ) -> ClientApiResult<()> { + if ops.is_empty() { + return Err(ClientApiError::Business { + code: ErrorCode::InvalidRequest, + message: "batch ops must not be empty".into(), + required_action: None, + }); + } + + // Performance tracking for batch operation + let _timer = ScopedTimer::new("client::batch"); + + let client_inner = self.client_inner.load(); + let proto_ops: Vec = ops + .into_iter() + .map(|op| match op { + BatchOp::Insert { key, value } => ProtoBatchOp { + op: Some(batch_op::Op::Insert(Insert { + key, + value, + ttl_secs: 0, + })), + }, + BatchOp::Delete { key } => ProtoBatchOp { + op: Some(batch_op::Op::Delete(Delete { key })), + }, + }) + .collect(); + let command = WriteCommand::batch(proto_ops); + + let request = ClientWriteRequest { + client_id: client_inner.client_id, + command: Some(command), + }; + + // Send write request to leader node (strong consistency required) + let mut client = self.make_leader_client().await?; + match client.handle_client_write(request).await { + Ok(response) => { + debug!("[:GrpcClient:batch] response: {:?}", response); + let client_response = response.get_ref(); + client_response.validate_error() + } + Err(status) => { + error!("[:GrpcClient:batch] status: {:?}", status); + Err(Into::::into(ClientApiError::from(status))) + } + } + } + async fn get( &self, key: impl AsRef<[u8]> + Send, diff --git a/d-engine-core/benches/leader_state_bench.rs b/d-engine-core/benches/leader_state_bench.rs index 873c0a04..1e5bccaa 100644 --- a/d-engine-core/benches/leader_state_bench.rs +++ b/d-engine-core/benches/leader_state_bench.rs @@ -175,7 +175,6 @@ impl BenchFixture { state_machine.expect_stop().returning(|| Ok(())); state_machine.expect_is_running().returning(|| true); state_machine.expect_get().returning(|_| Ok(None)); - state_machine.expect_entry_term().returning(|_| None); state_machine.expect_apply_chunk().returning(|_| Ok(vec![])); state_machine.expect_len().returning(|| 0); state_machine.expect_update_last_applied().returning(|_| ()); diff --git a/d-engine-core/src/client/client_api.rs b/d-engine-core/src/client/client_api.rs index 4d3b29ff..8995868e 100644 --- a/d-engine-core/src/client/client_api.rs +++ b/d-engine-core/src/client/client_api.rs @@ -25,6 +25,7 @@ //! } //! ``` +use crate::BatchOp; use crate::ScanResult; use crate::client::client_api_error::ClientApiResult; use crate::config::ReadConsistencyPolicy; @@ -97,6 +98,30 @@ pub trait ClientApi: Send + Sync { ttl_secs: u64, ) -> ClientApiResult<()>; + /// Atomically commits multiple write operations as a single Raft log entry. + /// + /// All operations succeed or none apply β€” all-or-nothing. Accepts only writes + /// (insert / delete); conditional writes belong to transactions (#298). + /// + /// # Errors + /// + /// - `InvalidArgument` if `ops` is empty + /// - `Network` if the node is shutting down or the request times out + /// - `Business` for server-side rejections (e.g., not the current leader) + /// + /// # Example + /// + /// ```rust,ignore + /// client.batch(vec![ + /// BatchOp::Insert { key: Bytes::from("a"), value: Bytes::from("1") }, + /// BatchOp::Delete { key: Bytes::from("b") }, + /// ]).await?; + /// ``` + async fn batch( + &self, + ops: Vec, + ) -> ClientApiResult<()>; + /// Retrieves the value associated with a key. /// /// Uses linearizable reads by default, ensuring the returned value diff --git a/d-engine-core/src/client/mod.rs b/d-engine-core/src/client/mod.rs index 4205b4bf..dc1ff163 100644 --- a/d-engine-core/src/client/mod.rs +++ b/d-engine-core/src/client/mod.rs @@ -11,7 +11,7 @@ pub use client_api::ClientApi; pub use client_api_error::{ClientApiError, ClientApiResult}; pub use types::{ ClientReadRequest, ClientResponse, ClientResponsePayload, ClientWriteRequest, ErrorCode, - KvEntry, LeaderHint, ReadResults, WriteOperation, WriteResult, + KvEntry, LeaderHint, ReadResults, WriteResult, }; // ReadConsistencyPolicy lives in config but is part of the public client API surface. pub use crate::config::ReadConsistencyPolicy; diff --git a/d-engine-core/src/client/types.rs b/d-engine-core/src/client/types.rs index b2fe55c0..9bee9aa5 100644 --- a/d-engine-core/src/client/types.rs +++ b/d-engine-core/src/client/types.rs @@ -29,31 +29,7 @@ pub use crate::config::ReadConsistencyPolicy; #[derive(Debug, Clone, PartialEq)] pub struct ClientWriteRequest { pub client_id: u32, - pub command: Option, -} - -/// Decoded write operation β€” the unit submitted by a client. -/// -/// Mirrors proto `WriteCommand` in shape but carries no prost annotations. -/// Core owns the serialization to Raft log bytes (`WriteOperation β†’ proto::WriteCommand β†’ bytes`); -/// transport adapters work with this native type only. -#[derive(Debug, Clone, PartialEq)] -pub enum WriteOperation { - Insert { - key: Bytes, - value: Bytes, - /// `None` = no expiration. Proto encodes this as `ttl_secs = 0`. - ttl_secs: Option, - }, - Delete { - key: Bytes, - }, - CompareAndSwap { - key: Bytes, - /// `None` means the key must not exist for the swap to succeed. - expected: Option, - new_value: Bytes, - }, + pub command: Option, } // ─── Read request ───────────────────────────────────────────────────────────── diff --git a/d-engine-core/src/client/types_test.rs b/d-engine-core/src/client/types_test.rs index 149e8e62..821152c2 100644 --- a/d-engine-core/src/client/types_test.rs +++ b/d-engine-core/src/client/types_test.rs @@ -84,101 +84,19 @@ fn test_error_code_discriminants_match_proto_wire_values() { assert_eq!(ErrorCode::Uncategorized as i32, 9999); } -// ─── WriteOperation ─────────────────────────────────────────────────────────── - -#[test] -fn test_write_operation_insert_carries_correct_fields() { - let op = WriteOperation::Insert { - key: Bytes::from("k"), - value: Bytes::from("v"), - ttl_secs: Some(30), - }; - match op { - WriteOperation::Insert { - key, - value, - ttl_secs, - } => { - assert_eq!(key, Bytes::from("k")); - assert_eq!(value, Bytes::from("v")); - assert_eq!(ttl_secs, Some(30)); - } - _ => panic!("wrong variant"), - } -} - -#[test] -fn test_write_operation_insert_no_ttl_is_none() { - let op = WriteOperation::Insert { - key: Bytes::from("k"), - value: Bytes::from("v"), - ttl_secs: None, - }; - match op { - WriteOperation::Insert { ttl_secs: None, .. } => {} - _ => panic!("expected None ttl"), - } -} - -#[test] -fn test_write_operation_delete_carries_key() { - let op = WriteOperation::Delete { - key: Bytes::from("del-key"), - }; - match op { - WriteOperation::Delete { key } => assert_eq!(key, Bytes::from("del-key")), - _ => panic!("wrong variant"), - } -} - -#[test] -fn test_write_operation_cas_with_expected_value() { - let op = WriteOperation::CompareAndSwap { - key: Bytes::from("k"), - expected: Some(Bytes::from("old")), - new_value: Bytes::from("new"), - }; - match op { - WriteOperation::CompareAndSwap { - key, - expected, - new_value, - } => { - assert_eq!(key, Bytes::from("k")); - assert_eq!(expected, Some(Bytes::from("old"))); - assert_eq!(new_value, Bytes::from("new")); - } - _ => panic!("wrong variant"), - } -} - -/// CAS where expected=None means "key must not exist" β€” a distinct semantic from -/// "any value is accepted". This test encodes that contract. -#[test] -fn test_write_operation_cas_key_must_not_exist_when_expected_is_none() { - let op = WriteOperation::CompareAndSwap { - key: Bytes::from("k"), - expected: None, - new_value: Bytes::from("new"), - }; - match op { - WriteOperation::CompareAndSwap { expected: None, .. } => {} - _ => panic!("expected None should mean key-must-not-exist"), - } -} - // ─── ClientWriteRequest ─────────────────────────────────────────────────────── +// WriteOperation was moved to d-engine-server (server transport layer). +// ClientWriteRequest.command is now pre-serialized Bytes β€” core is encoding-agnostic. #[test] -fn test_client_write_request_fields() { +fn test_client_write_request_carries_pre_serialized_bytes() { + let payload = Bytes::from_static(b"\x08\x01"); // any valid proto bytes let req = ClientWriteRequest { client_id: 42, - command: Some(WriteOperation::Delete { - key: Bytes::from("k"), - }), + command: Some(payload.clone()), }; assert_eq!(req.client_id, 42); - assert!(req.command.is_some()); + assert_eq!(req.command.as_ref().unwrap(), &payload); } #[test] diff --git a/d-engine-core/src/command.rs b/d-engine-core/src/command.rs index b6c0c38d..a9eb45c3 100644 --- a/d-engine-core/src/command.rs +++ b/d-engine-core/src/command.rs @@ -17,6 +17,7 @@ use bytes::Bytes; use d_engine_proto::client::WriteCommand; use d_engine_proto::client::write_command::Operation; +use d_engine_proto::client::write_command::batch_op; use d_engine_proto::common::Entry; use d_engine_proto::common::entry_payload::Payload; use prost::Message; @@ -55,6 +56,29 @@ pub enum Command { expected: Option, value: Bytes, }, + + /// Atomic batch of Insert/Delete operations β€” all succeed or all fail. + /// + /// Individual ops within a batch do not support TTL. Batch is designed for + /// atomic multi-key writes (service registration, config updates, distributed + /// locks) where per-key expiration does not apply. + Batch { + ops: Vec, + }, +} + +/// Single operation within a [`Command::Batch`]. +#[derive(Debug, Clone, PartialEq)] +pub enum BatchOp { + /// Insert a key-value pair. TTL is not supported in batch mode β€” + /// use [`Command::Insert`] directly if you need per-key expiration. + Insert { + key: Bytes, + value: Bytes, + }, + Delete { + key: Bytes, + }, } /// A decoded Raft log entry ready for state machine application. @@ -92,6 +116,32 @@ impl TryFrom for Command { expected: c.expected_value, value: c.new_value, }), + Some(Operation::Batch(b)) => { + let ops = b + .ops + .into_iter() + .map(|op| match op.op { + Some(batch_op::Op::Insert(i)) => { + if i.ttl_secs != 0 { + return Err(StorageError::StateMachineError( + "BatchOp does not support TTL; use Command::Insert directly for per-key expiration".into(), + ) + .into()); + } + Ok(BatchOp::Insert { + key: i.key, + value: i.value, + }) + } + Some(batch_op::Op::Delete(d)) => Ok(BatchOp::Delete { key: d.key }), + None => Err(StorageError::StateMachineError( + "BatchOp has no operation".into(), + ) + .into()), + }) + .collect::, Error>>()?; + Ok(Command::Batch { ops }) + } None => { Err(StorageError::StateMachineError("WriteCommand has no operation".into()).into()) } @@ -155,105 +205,5 @@ pub fn decode_entries(entries: Vec) -> Result, Error> { } #[cfg(test)] -mod tests { - use bytes::Bytes; - use d_engine_proto::common::EntryPayload; - use d_engine_proto::common::MembershipChange; - use d_engine_proto::common::Noop; - use d_engine_proto::common::entry_payload::Payload; - - use super::*; - - fn noop_entry(index: u64) -> Entry { - Entry { - index, - term: 1, - payload: Some(EntryPayload { - payload: Some(Payload::Noop(Noop {})), - }), - } - } - - fn config_entry(index: u64) -> Entry { - Entry { - index, - term: 1, - payload: Some(EntryPayload { - payload: Some(Payload::Config(MembershipChange { change: None })), - }), - } - } - - fn insert_entry( - index: u64, - key: &str, - value: &str, - ) -> Entry { - use d_engine_proto::client::WriteCommand; - use d_engine_proto::client::write_command::{Insert, Operation}; - use prost::Message; - - let wc = WriteCommand { - operation: Some(Operation::Insert(Insert { - key: Bytes::from(key.to_owned()), - value: Bytes::from(value.to_owned()), - ttl_secs: 0, - })), - }; - let mut buf = Vec::new(); - wc.encode(&mut buf).unwrap(); - Entry { - index, - term: 1, - payload: Some(EntryPayload { - payload: Some(Payload::Command(Bytes::from(buf))), - }), - } - } - - /// Config entries must produce Command::Noop, not be dropped. - /// Dropping them leaves sm.last_applied stuck, breaking ReadIndex drain. - #[test] - fn config_entry_becomes_noop() { - let entries = vec![config_entry(5)]; - let result = decode_entries(entries).unwrap(); - assert_eq!(result.len(), 1); - assert_eq!(result[0].index, 5); - assert_eq!(result[0].command, Command::Noop); - } - - /// last_applied continuity: a mixed batch of Insertβ†’Configβ†’Insert must yield - /// three ApplyEntry values with no index gaps. - #[test] - fn config_entry_preserves_index_continuity() { - let entries = vec![ - insert_entry(10, "k1", "v1"), - config_entry(11), // membership change - insert_entry(12, "k2", "v2"), - ]; - let result = decode_entries(entries).unwrap(); - assert_eq!(result.len(), 3); - assert_eq!(result[0].index, 10); - assert_eq!(result[1].index, 11); - assert!(matches!(result[1].command, Command::Noop)); - assert_eq!(result[2].index, 12); - } - - /// A chunk that is entirely Config entries must still produce one Noop per entry, - /// so the SM can advance last_applied to the highest config index. - #[test] - fn all_config_chunk_produces_noops() { - let entries = vec![config_entry(3), config_entry(4), config_entry(5)]; - let result = decode_entries(entries).unwrap(); - assert_eq!(result.len(), 3); - assert!(result.iter().all(|e| e.command == Command::Noop)); - assert_eq!(result.last().unwrap().index, 5); - } - - #[test] - fn noop_entry_produces_noop() { - let result = decode_entries(vec![noop_entry(1)]).unwrap(); - assert_eq!(result.len(), 1); - assert_eq!(result[0].command, Command::Noop); - } -} +#[path = "command_test.rs"] +mod tests; diff --git a/d-engine-core/src/command_test.rs b/d-engine-core/src/command_test.rs index 3653f042..3734ed0d 100644 --- a/d-engine-core/src/command_test.rs +++ b/d-engine-core/src/command_test.rs @@ -1,13 +1,16 @@ use bytes::Bytes; use d_engine_proto::client::WriteCommand; -use d_engine_proto::client::write_command::{CompareAndSwap, Delete, Insert, Operation}; +use d_engine_proto::client::write_command::batch_op; +use d_engine_proto::client::write_command::{ + Batch, BatchOp, CompareAndSwap, Delete, Insert, Operation, +}; use d_engine_proto::common::AddNode; use d_engine_proto::common::Entry; use d_engine_proto::common::EntryPayload; use d_engine_proto::common::membership_change::Change; use prost::Message; -use crate::command::{ApplyEntry, Command, decode_entries}; +use crate::command::{ApplyEntry, BatchOp as CommandBatchOp, Command, decode_entries}; // ── helpers ──────────────────────────────────────────────────────────────── @@ -102,6 +105,58 @@ fn cas_entry( } } +// ── batch helpers ─────────────────────────────────────────────────────────── + +fn batch_insert_op( + key: &[u8], + value: &[u8], +) -> BatchOp { + BatchOp { + op: Some(batch_op::Op::Insert(Insert { + key: Bytes::copy_from_slice(key), + value: Bytes::copy_from_slice(value), + ttl_secs: 0, + })), + } +} + +fn batch_delete_op(key: &[u8]) -> BatchOp { + BatchOp { + op: Some(batch_op::Op::Delete(Delete { + key: Bytes::copy_from_slice(key), + })), + } +} + +/// Helper: create a BatchOp::Insert with non-zero TTL for rejection testing. +fn batch_insert_op_with_ttl( + key: &[u8], + value: &[u8], + ttl_secs: u64, +) -> BatchOp { + BatchOp { + op: Some(batch_op::Op::Insert(Insert { + key: Bytes::copy_from_slice(key), + value: Bytes::copy_from_slice(value), + ttl_secs, + })), + } +} + +fn batch_entry( + index: u64, + term: u64, + ops: Vec, +) -> Entry { + Entry { + index, + term, + payload: Some(EntryPayload::command(encode_write_cmd(Operation::Batch( + Batch { ops }, + )))), + } +} + // ── decode: single-variant tests ─────────────────────────────────────────── #[test] @@ -244,6 +299,134 @@ fn test_decode_mixed_batch_config_becomes_noop_keeps_order() { assert!(matches!(result[3].command, Command::Delete { .. })); } +#[test] +fn test_decode_all_config_entries_become_noops() { + // All entries are config changes β€” each must become Noop so last_applied + // advances to the highest config index without gaps. + let entries = vec![config_entry(3, 1), config_entry(4, 1), config_entry(5, 1)]; + let result = decode_entries(entries).unwrap(); + assert_eq!(result.len(), 3); + assert!(result.iter().all(|e| e.command == Command::Noop)); + assert_eq!(result.last().unwrap().index, 5); +} + +// ── decode: batch command ─────────────────────────────────────────────────── + +#[test] +fn test_decode_batch_inserts_only() { + // Batch with only Insert ops β€” each op becomes CommandBatchOp::Insert. + let ops = vec![batch_insert_op(b"k1", b"v1"), batch_insert_op(b"k2", b"v2")]; + let entries = vec![batch_entry(7, 2, ops)]; + let result = decode_entries(entries).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!( + result[0].command, + Command::Batch { + ops: vec![ + CommandBatchOp::Insert { + key: Bytes::from_static(b"k1"), + value: Bytes::from_static(b"v1"), + }, + CommandBatchOp::Insert { + key: Bytes::from_static(b"k2"), + value: Bytes::from_static(b"v2"), + }, + ] + } + ); +} + +#[test] +fn test_decode_batch_deletes_only() { + // Batch with only Delete ops β€” each op becomes CommandBatchOp::Delete. + let ops = vec![batch_delete_op(b"k1"), batch_delete_op(b"k2")]; + let entries = vec![batch_entry(8, 2, ops)]; + let result = decode_entries(entries).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!( + result[0].command, + Command::Batch { + ops: vec![ + CommandBatchOp::Delete { + key: Bytes::from_static(b"k1"), + }, + CommandBatchOp::Delete { + key: Bytes::from_static(b"k2"), + }, + ] + } + ); +} + +#[test] +fn test_decode_batch_mixed_ops() { + // Batch with Insert and Delete interleaved β€” order is preserved. + let ops = vec![ + batch_insert_op(b"k1", b"v1"), + batch_delete_op(b"k2"), + batch_insert_op(b"k3", b"v3"), + ]; + let entries = vec![batch_entry(9, 2, ops)]; + let result = decode_entries(entries).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!( + result[0].command, + Command::Batch { + ops: vec![ + CommandBatchOp::Insert { + key: Bytes::from_static(b"k1"), + value: Bytes::from_static(b"v1"), + }, + CommandBatchOp::Delete { + key: Bytes::from_static(b"k2"), + }, + CommandBatchOp::Insert { + key: Bytes::from_static(b"k3"), + value: Bytes::from_static(b"v3"), + }, + ] + } + ); +} + +#[test] +fn test_decode_batch_empty_ops() { + // Batch with zero ops β€” decoded as Batch { ops: [] }. + let entries = vec![batch_entry(10, 2, vec![])]; + let result = decode_entries(entries).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result[0].command, Command::Batch { ops: vec![] }); +} + +#[test] +fn test_decode_batch_preserves_index_and_term() { + // The ApplyEntry wrapping a Batch command carries the correct index and term. + let ops = vec![batch_insert_op(b"k", b"v")]; + let entries = vec![batch_entry(42, 7, ops)]; + let result = decode_entries(entries).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result[0].index, 42); + assert_eq!(result[0].term, 7); +} + +#[test] +fn test_decode_batch_op_without_operation_returns_error() { + // BatchOp with op = None must produce an error β€” no silent skip. + let malformed_op = BatchOp { op: None }; + let entries = vec![batch_entry(11, 2, vec![malformed_op])]; + let result = decode_entries(entries); + assert!(result.is_err(), "BatchOp with no operation must return Err"); +} + +#[test] +fn test_decode_batch_insert_with_ttl_returns_error() { + // Batch ops do not support TTL β€” reject with a clear error. + let ops = vec![batch_insert_op_with_ttl(b"k", b"v", 60)]; + let entries = vec![batch_entry(12, 2, ops)]; + let result = decode_entries(entries); + assert!(result.is_err(), "BatchOp with non-zero TTL must return Err"); +} + // ── decode: error cases ───────────────────────────────────────────────────── #[test] diff --git a/d-engine-core/src/config/cluster.rs b/d-engine-core/src/config/cluster.rs index 4ac6a1d6..f2a85e24 100644 --- a/d-engine-core/src/config/cluster.rs +++ b/d-engine-core/src/config/cluster.rs @@ -38,7 +38,7 @@ pub struct ClusterConfig { /// Seed nodes for cluster initialization /// - /// Default: `default_initial_cluster()` (empty vector) + /// Default: `default_initial_cluster()` /// /// # Note /// Should contain at least 3 nodes for production deployment @@ -62,7 +62,7 @@ impl Default for ClusterConfig { Self { node_id: default_node_id(), listen_address: default_listen_addr(), - initial_cluster: vec![], + initial_cluster: default_initial_cluster(), db_root_dir: default_db_dir(), log_dir: default_log_dir(), } diff --git a/d-engine-core/src/config/mod.rs b/d-engine-core/src/config/mod.rs index 19354c82..629adbe4 100644 --- a/d-engine-core/src/config/mod.rs +++ b/d-engine-core/src/config/mod.rs @@ -140,11 +140,16 @@ impl RaftNodeConfig { /// ``` pub fn with_override_config( &self, - path: &str, + path: impl AsRef, ) -> Result { + let path_str = path + .as_ref() + .to_str() + .ok_or_else(|| crate::Error::Fatal("config path is not valid UTF-8".into()))?; + let config: Self = Config::builder() .add_source(Config::try_from(self)?) - .add_source(File::with_name(path)) + .add_source(File::with_name(path_str)) .add_source( Environment::with_prefix("RAFT") .separator("__") diff --git a/d-engine-core/src/election/election_handler.rs b/d-engine-core/src/election/election_handler.rs index 738644a7..081f1455 100644 --- a/d-engine-core/src/election/election_handler.rs +++ b/d-engine-core/src/election/election_handler.rs @@ -277,7 +277,6 @@ where /// - candidate s log is at least as up-to-date as receiver s log /// e.g. { my_id: 2 } request=VoteRequest { term: 3, candidate_id: 1, last_log_index: 2, /// last_log_term: 10 } current_term=3 last_log_index=3 last_log_term=8 voted_for_option=None - #[tracing::instrument] fn check_vote_request_is_legal( &self, request: &VoteRequest, diff --git a/d-engine-core/src/lib.rs b/d-engine-core/src/lib.rs index ab15ce29..93333741 100644 --- a/d-engine-core/src/lib.rs +++ b/d-engine-core/src/lib.rs @@ -128,8 +128,6 @@ pub use utils::*; pub(crate) use timer::*; -#[cfg(test)] -mod command_test; #[cfg(test)] mod maybe_clone_oneshot_test; @@ -175,7 +173,6 @@ pub(crate) fn if_higher_term_found( /// entries in the logs. If the logs have last entries with different terms, then the log with the /// later term is more up-to-date. If the logs end with the same term, then whichever log is longer /// is more up-to-date. -#[tracing::instrument] pub(crate) fn is_target_log_more_recent( my_last_log_index: u64, my_last_log_term: u64, diff --git a/d-engine-core/src/raft_role/candidate_state.rs b/d-engine-core/src/raft_role/candidate_state.rs index 32788abe..7082ab6a 100644 --- a/d-engine-core/src/raft_role/candidate_state.rs +++ b/d-engine-core/src/raft_role/candidate_state.rs @@ -206,6 +206,22 @@ impl RaftRoleState for CandidateState { Ok(()) } + /// Trigger an independent snapshot on this role (Raft Β§7 β€” each server snapshots + /// independently). Called when `should_snapshot()` returns true after SM apply. + /// Default: no-op. Candidate overrides to return `RoleViolation`. + async fn handle_create_snapshot( + &mut self, + _ctx: &RaftContext, + _internal_event_tx: &mpsc::UnboundedSender, + ) -> Result<()> { + Err(ConsensusError::RoleViolation { + current_role: "Candidate", + required_role: "Follower/Leader/Learner", + context: ("Candidate node attempted to create snapshot.").to_string(), + } + .into()) + } + async fn handle_inbound_event( &mut self, inbound_event: InboundEvent, diff --git a/d-engine-core/src/raft_role/candidate_state_test.rs b/d-engine-core/src/raft_role/candidate_state_test.rs index 04a8daa8..4da3a2eb 100644 --- a/d-engine-core/src/raft_role/candidate_state_test.rs +++ b/d-engine-core/src/raft_role/candidate_state_test.rs @@ -3,7 +3,6 @@ use std::sync::Arc; use crate::client::ClientReadRequest; use crate::client::ClientWriteRequest; use crate::client::ErrorCode; -use crate::client::WriteOperation; use crate::config::ReadConsistencyPolicy; use d_engine_proto::common::LogId; use d_engine_proto::server::cluster::ClusterConfChangeRequest; @@ -32,12 +31,15 @@ use crate::MockReplicationCore; use crate::MockStateMachineHandler; use crate::RaftOneshot; use crate::raft_role::candidate_state::CandidateState; +use crate::raft_role::follower_state::FollowerState; use crate::raft_role::role_state::RaftRoleState; use crate::test_utils::create_test_chunk; use crate::test_utils::mock::MockTypeConfig; use crate::test_utils::mock::mock_election_core; use crate::test_utils::mock::mock_raft_context; use crate::test_utils::node_config; +use d_engine_proto::client::WriteCommand; +use prost::Message; use tokio::sync::{mpsc, watch}; /// Test: CandidateState can_vote_myself returns true for new candidate @@ -906,9 +908,9 @@ async fn test_handle_client_write_returns_not_leader() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: 1, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), - }), + command: Some(bytes::Bytes::from( + WriteCommand::delete(bytes::Bytes::new()).encode_to_vec(), + )), }, resp_tx, ); @@ -1319,11 +1321,13 @@ async fn test_new_leader_initializes_empty_buffers() { let (response_tx, _response_rx) = crate::MaybeCloneOneshot::new(); let write_req = ClientWriteRequest { client_id: 1, - command: Some(WriteOperation::Insert { - key: bytes::Bytes::from("first_key"), - value: bytes::Bytes::from("first_value"), - ttl_secs: None, - }), + command: Some(bytes::Bytes::from( + WriteCommand::insert( + bytes::Bytes::from("first_key"), + bytes::Bytes::from("first_value"), + ) + .encode_to_vec(), + )), }; let cmd = crate::ClientCmd::Propose(write_req, response_tx); @@ -1451,3 +1455,26 @@ async fn test_candidate_rejects_stream_snapshot() { "Candidate must reply FailedPrecondition for StreamSnapshot" ); } + +// ============================================================================ +// Role Transition Tests β€” last_purged_index +// ============================================================================ + +/// From<&FollowerState>: last_purged_index preserved. +/// +/// CandidateState has no scheduled_purge_upto field β€” candidates do not purge logs. +/// The watermark is carried forward so the new candidate knows which log entries are +/// already gone and does not attempt to read or re-purge them. +#[test] +fn test_candidate_from_follower_preserves_last_purged_index() { + let cfg = Arc::new(node_config("/tmp/test_candidate_from_follower_purge")); + let mut follower = FollowerState::::new(1, cfg, None, None); + follower.last_purged_index = Some(LogId { term: 1, index: 9 }); + + let candidate = CandidateState::from(&follower); + + assert_eq!( + candidate.last_purged_index, + Some(LogId { term: 1, index: 9 }) + ); +} diff --git a/d-engine-core/src/raft_role/follower_state.rs b/d-engine-core/src/raft_role/follower_state.rs index 6670b4c7..a72def21 100644 --- a/d-engine-core/src/raft_role/follower_state.rs +++ b/d-engine-core/src/raft_role/follower_state.rs @@ -18,7 +18,6 @@ use tonic::Status; use tracing::debug; use tracing::error; use tracing::info; -use tracing::instrument; use tracing::trace; use tracing::warn; @@ -38,7 +37,6 @@ use crate::InboundEvent; use crate::InternalEvent; use crate::Membership; use crate::NetworkError; -use crate::PurgeExecutor; use crate::RaftContext; use crate::RaftLog; use crate::RaftNodeConfig; @@ -46,6 +44,7 @@ use crate::Result; use crate::StateMachineHandler; use crate::StateTransitionError; use crate::TypeConfig; +use crate::role_state::schedule_and_execute_purge; use crate::utils::cluster::error; /// Follower node's state in Raft consensus. @@ -60,6 +59,16 @@ pub struct FollowerState { pub shared_state: SharedState, // -- Log Compaction & Purge -- + /// === Volatile State === + /// The upper bound (exclusive) of log entries scheduled for asynchronous physical deletion. + /// + /// This value is set immediately after a new snapshot is successfully created. + /// It represents the next log position that will trigger compaction. + /// + /// The actual log purge is performed by a background task, which may be delayed + /// due to resource constraints or retry mechanisms. + pub scheduled_purge_upto: Option, + /// === Persistent State === /// Last physically purged log index (inclusive) pub last_purged_index: Option, @@ -473,85 +482,49 @@ impl RaftRoleState for FollowerState { ) } - /// Trigger an independent snapshot on this role (Raft Β§7 β€” each server snapshots - /// independently). Called when `should_snapshot()` returns true after SM apply. - /// Default: no-op. Candidate overrides to return `RoleViolation`. - async fn handle_create_snapshot( - &mut self, - ctx: &RaftContext, - internal_event_tx: &mpsc::UnboundedSender, - ) -> Result<()> { - // Prevent duplicate snapshot creation - if self.snapshot_in_progress.load(Ordering::Acquire) { - info!("Snapshot creation already in progress. Skipping duplicate request."); - return Ok(()); - } - - self.snapshot_in_progress.store(true, Ordering::Release); - let state_machine_handler = ctx.state_machine_handler().clone(); - - // Use spawn to perform snapshot creation in the background - let internal_event_tx = internal_event_tx.clone(); - tokio::spawn(async move { - let result = state_machine_handler.create_snapshot().await; - info!("SnapshotCreated event will be processed in another event thread"); - if let Err(e) = internal_event_tx.send(InternalEvent::SnapshotCreated(result)) { - error!("Follower failed to send snapshot creation result: {e:?}"); - } - }); - - Ok(()) + fn snapshot_in_progress(&self) -> Option<&AtomicBool> { + Some(&self.snapshot_in_progress) } - /// Process the completed snapshot result (success or error). - /// Leader: schedules log purge up to `last_included`. - /// Follower/Learner: updates local snapshot path. - /// Default: no-op. Candidate overrides to return `RoleViolation`. async fn handle_snapshot_created( &mut self, result: crate::Result<(SnapshotMetadata, std::path::PathBuf)>, ctx: &RaftContext, - _internal_event_tx: &mpsc::UnboundedSender, + internal_event_tx: &mpsc::UnboundedSender, ) -> Result<()> { - // Reset snapshot_in_progress flag self.snapshot_in_progress.store(false, Ordering::SeqCst); - - // Per Raft Β§7: Follower independently purges logs after snapshot generation match result { - Ok((metadata, _path)) => { + Err(e) => error!(?e, "Follower snapshot creation failed"), + Ok((metadata, _)) => { if let Some(last_included) = metadata.last_included { - info!(?last_included, "Follower snapshot created, purging logs"); - - // Follower independently decides to purge after snapshot - if self.can_purge_logs(self.last_purged_index, last_included) { - match ctx.purge_executor().execute_purge(last_included).await { - Ok(_) => { - self.last_purged_index = Some(last_included); - info!(?last_included, "Follower logs purged successfully"); - } - Err(e) => { - error!(?e, "Failed to purge logs after snapshot"); - } - } - } + schedule_and_execute_purge( + last_included, + ctx, + self.commit_index(), + self.last_purged_index, + &mut self.scheduled_purge_upto, + internal_event_tx, + ) + .await?; } } - Err(e) => { - error!(?e, "Follower snapshot creation failed"); - } } - Ok(()) } // Stale in-flight events β€” these are Leader-only operations that may arrive // after a step-down due to internal_event_tx (unbounded, P2) race with BecomeFollower. // Both orderings are valid; Follower silently ignores them rather than erroring. - fn handle_log_purge_completed( &mut self, - _purged_id: d_engine_proto::common::LogId, + purged_id: d_engine_proto::common::LogId, ) -> Result<()> { + if self.last_purged_index.is_none_or(|cur| purged_id.index > cur.index) { + self.last_purged_index = Some(purged_id); + + // purge completed, clear to prevent re-execution + self.scheduled_purge_upto = None; + } Ok(()) } @@ -593,13 +566,12 @@ impl FollowerState { node_config, snapshot_in_progress: AtomicBool::new(false), _marker: PhantomData, - last_purged_index: None, /*TODO - * scheduled_purge_upto: None, */ + last_purged_index: None, + scheduled_purge_upto: None, } } /// The fun will retrieve current state snapshot - #[tracing::instrument] pub fn state_snapshot(&self) -> StateSnapshot { StateSnapshot { role: Follower as i32, @@ -608,44 +580,6 @@ impl FollowerState { commit_index: self.commit_index(), } } - - /// Determines if logs prior to `last_included_in_request` can be safely discarded. - /// - /// Implements the critical log compaction safety check from Raft paper Β§7.2: - /// > "Raft never commits log entries from previous terms by counting replicas" - /// - /// # Invariants (MUST ALL hold) - /// 1. Leader-guaranteed stability: `last_included_in_request.index` < self.commit_index - /// - Ensures we never truncate uncommitted entries (gap prevents Figure 8 bugs) - /// - Leader must have replicated this index to a quorum before sending purge - /// - /// 2. Monotonic advancement: `last_purge_index` < last_included_in_request.index - /// - Prevents out-of-order purge operations - /// - Maintains purge sequence strictly increasing - /// - /// 3. State machine safety: - /// - A valid snapshot covering `last_included_in_request` must exist - /// - Verified before entering this function via snapshot integrity checks - /// - /// # Gap Design Intent - /// The `index < commit_index` (not ≀) ensures: - /// - At least one committed entry remains after purge - /// - Critical for follower's log matching property during reelections - /// - Prevents "phantom entries" when combined with Β§5.4.2 election restriction - #[instrument(skip(self))] - pub fn can_purge_logs( - &self, - last_purge_index: Option, - last_included_in_request: LogId, - ) -> bool { - let commit_check = last_included_in_request.index < self.commit_index(); - - let monotonic_check = last_purge_index - .map(|lid| lid.index < last_included_in_request.index) - .unwrap_or(true); - - commit_check && monotonic_check - } } impl From<&CandidateState> for FollowerState { fn from(candidate_state: &CandidateState) -> Self { @@ -660,6 +594,7 @@ impl From<&CandidateState> for FollowerState { last_purged_index: candidate_state.last_purged_index, // scheduled_purge_upto: None, _marker: PhantomData, + scheduled_purge_upto: None, } } } @@ -677,6 +612,7 @@ impl From<&LeaderState> for FollowerState { ), last_purged_index: leader_state.last_purged_index, // scheduled_purge_upto: None, + scheduled_purge_upto: leader_state.scheduled_purge_upto, _marker: PhantomData, } } @@ -692,7 +628,8 @@ impl From<&LearnerState> for FollowerState { )), node_config: learner_state.node_config.clone(), snapshot_in_progress: AtomicBool::new(false), - last_purged_index: None, //TODO + last_purged_index: learner_state.last_purged_index, + scheduled_purge_upto: learner_state.scheduled_purge_upto, _marker: PhantomData, } } diff --git a/d-engine-core/src/raft_role/follower_state_test.rs b/d-engine-core/src/raft_role/follower_state_test.rs index 11045d7f..22c22ad5 100644 --- a/d-engine-core/src/raft_role/follower_state_test.rs +++ b/d-engine-core/src/raft_role/follower_state_test.rs @@ -1,7 +1,6 @@ use crate::client::ClientReadRequest; use crate::client::ClientWriteRequest; use crate::client::ErrorCode; -use crate::client::WriteOperation; use crate::config::ReadConsistencyPolicy; use d_engine_proto::common::LogId; use d_engine_proto::common::NodeRole; @@ -42,14 +41,20 @@ use crate::RaftLog; use crate::RaftOneshot; use crate::StateUpdate; use crate::SystemError; +use crate::raft_role::candidate_state::CandidateState; use crate::raft_role::follower_state::FollowerState; +use crate::raft_role::leader_state::LeaderState; +use crate::raft_role::learner_state::LearnerState; use crate::raft_role::role_state::RaftRoleState; use crate::test_utils::mock::MockBuilder; use crate::test_utils::mock::MockTypeConfig; use crate::test_utils::mock::mock_raft_context; use crate::test_utils::mock::mock_raft_context_with_temp; +use crate::test_utils::mock::mock_raft_log; use crate::test_utils::node_config; +use d_engine_proto::client::WriteCommand; use mockall::predicate::eq; +use prost::Message; use tokio::sync::{mpsc, watch}; // ============================================================================ @@ -1170,9 +1175,9 @@ async fn test_handle_client_write_request_redirects_to_leader() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: 1, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), - }), + command: Some(bytes::Bytes::from( + WriteCommand::delete(bytes::Bytes::new()).encode_to_vec(), + )), }, resp_tx, ); @@ -1499,227 +1504,6 @@ async fn test_discover_leader_returns_not_found_when_metadata_missing() { ); } -// ============================================================================ -// Log Purge Safety Tests (can_purge_logs) -// ============================================================================ - -/// Test: can_purge_logs validates safe purge range -/// -/// Scenario: -/// - commit_index = 100 -/// - last_purge_index = 90 -/// - Request to purge up to index 99 -/// -/// Expected: -/// - Returns true (99 < 100, satisfies gap requirement) -/// - Edge case: 99 == commit_index - 1 is valid -/// - Invalid: 100 not < 100 (violates gap rule) -/// -/// This validates Raft log compaction safety: must maintain -/// at least one entry between purge and commit for consistency. -/// -/// Original: test_can_purge_logs_case1 -#[test] -fn test_can_purge_logs_validates_safe_range() { - let (_graceful_tx, graceful_rx) = watch::channel(()); - let (context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); - - let mut state = - FollowerState::::new(1, context.node_config.clone(), None, None); - - // Setup state matching Raft paper's log compaction rules - state.shared_state.commit_index = 100; // Last committed entry at 100 - - // Test valid purge range (90 < 99 < 100) - assert!( - state.can_purge_logs( - Some(LogId { index: 90, term: 1 }), // last_purge_index - LogId { index: 99, term: 1 } // last_included_in_request - ), - "Should allow purge when last_included < commit_index" - ); - - // Edge case: 99 == commit_index - 1 (per gap rule) - assert!( - state.can_purge_logs( - Some(LogId { index: 90, term: 1 }), - LogId { index: 99, term: 1 } - ), - "Should allow purge up to commit_index - 1" - ); - - // Violate gap rule: 100 not < 100 - assert!( - !state.can_purge_logs( - Some(LogId { index: 90, term: 1 }), - LogId { - index: 100, - term: 1 - } - ), - "Should reject purge at commit_index (violates gap)" - ); -} - -/// Test: can_purge_logs rejects uncommitted index -/// -/// Scenario: -/// - commit_index = 50 -/// - Request to purge beyond commit_index -/// -/// Expected: -/// - Returns false for purge_index > commit_index -/// - Returns false for purge_index == commit_index -/// -/// This validates Raft Β§5.4.2: never purge uncommitted entries. -/// -/// Original: test_can_purge_logs_case2 -#[test] -fn test_can_purge_logs_rejects_uncommitted_index() { - let (_graceful_tx, graceful_rx) = watch::channel(()); - let (context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); - - let mut state = - FollowerState::::new(1, context.node_config.clone(), None, None); - - state.shared_state.commit_index = 50; - - // Leader tries to purge beyond commit index - assert!( - !state.can_purge_logs( - Some(LogId { index: 40, term: 1 }), - LogId { index: 51, term: 1 } // 51 > commit_index(50) - ), - "Should reject purge beyond commit_index" - ); - - // Boundary check: 50 == commit_index (violates <) - assert!( - !state.can_purge_logs( - Some(LogId { index: 40, term: 1 }), - LogId { index: 50, term: 1 } - ), - "Should reject purge at commit_index" - ); -} - -/// Test: can_purge_logs ensures monotonicity -/// -/// Scenario: -/// - commit_index = 200 -/// - last_purge_index advances: 100 β†’ 150 -/// - Attempt to purge backwards or same index -/// -/// Expected: -/// - Returns true for monotonic advance (100 β†’ 150) -/// - Returns false for backwards purge (150 β†’ 120) -/// - Returns false for same index purge (150 β†’ 150) -/// -/// This validates Raft Β§7.2: purge index must always advance. -/// -/// Original: test_can_purge_logs_case3 -#[test] -fn test_can_purge_logs_ensures_monotonicity() { - let (_graceful_tx, graceful_rx) = watch::channel(()); - let (context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); - - let mut state = - FollowerState::::new(1, context.node_config.clone(), None, None); - - state.shared_state.commit_index = 200; - - // Valid sequence: 100 β†’ 150 - assert!( - state.can_purge_logs( - Some(LogId { - index: 100, - term: 1 - }), - LogId { - index: 150, - term: 1 - } - ), - "Should allow monotonic purge advance" - ); - - // Invalid: Attempt to purge backwards (150 β†’ 120) - assert!( - !state.can_purge_logs( - Some(LogId { - index: 150, - term: 1 - }), - LogId { - index: 120, - term: 1 - } - ), - "Should reject backwards purge" - ); - - // Same index purge attempt - assert!( - !state.can_purge_logs( - Some(LogId { - index: 150, - term: 1 - }), - LogId { - index: 150, - term: 1 - } - ), - "Should reject same index purge" - ); -} - -/// Test: can_purge_logs handles initial purge state -/// -/// Scenario: -/// - commit_index = 100 -/// - No previous purge (last_purge_index = None) -/// - First purge request -/// -/// Expected: -/// - Returns true for valid first purge (index < commit_index) -/// - Returns false if first purge violates gap rule -/// -/// This validates initial purge must still respect safety rules. -/// -/// Original: test_can_purge_logs_case4 -#[test] -fn test_can_purge_logs_handles_initial_state() { - let (_graceful_tx, graceful_rx) = watch::channel(()); - let (context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); - - let mut state = - FollowerState::::new(1, context.node_config.clone(), None, None); - - state.shared_state.commit_index = 100; - - // First ever purge (last_purge_index = None) - assert!( - state.can_purge_logs( - None, // No previous purge - LogId { index: 99, term: 1 } - ), - "Should allow first purge with valid index" - ); - - // First purge must still obey commit_index gap - assert!( - !state.can_purge_logs( - None, - LogId { - index: 100, - term: 1 - } // 100 not < 100 - ), - "First purge must still respect gap rule" - ); -} - // ============================================================================ // Snapshot Tests Module // ============================================================================ @@ -1745,11 +1529,17 @@ mod snapshot_tests { FollowerState::::new(1, context.node_config.clone(), None, None); let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); - // LogPurgeCompleted: leader-only, but stale events after step-down must not error + // LogPurgeCompleted: leader-only, but stale events after step-down must not error. + // Must also clear scheduled_purge_upto to prevent duplicate execution. + state.scheduled_purge_upto = Some(LogId { term: 1, index: 1 }); assert!( state.handle_log_purge_completed(LogId { term: 1, index: 1 }).is_ok(), "Stale LogPurgeCompleted should be silently ignored" ); + assert!( + state.scheduled_purge_upto.is_none(), + "handle_log_purge_completed must clear scheduled_purge_upto" + ); // PromoteReadyLearners: leader-only, same reasoning assert!( @@ -1826,13 +1616,26 @@ mod snapshot_tests { #[tokio::test] async fn test_follower_snapshot_created_success_resets_flag_and_purges_logs() { let (_graceful_tx, graceful_rx) = watch::channel(()); - let (context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + + // Default retained_log_entries = 1, last_included.index = 50 β†’ purge_upto_index = 49 + let mut raft_log = mock_raft_log(); + raft_log + .expect_entry_term() + .withf(|&idx| idx == 49) + .times(1) + .returning(|_| Some(1)); + let _temp_dir = tempfile::tempdir().unwrap(); + let mut nc = node_config(_temp_dir.path().to_str().unwrap()); + nc.raft.snapshot.retained_log_entries = 1; + let context = MockBuilder::new(graceful_rx) + .with_raft_log(raft_log) + .with_node_config(nc) + .build_context(); let mut state = FollowerState::::new(1, context.node_config.clone(), None, None); state.snapshot_in_progress.store(true, Ordering::SeqCst); - // can_purge_logs requires last_included.index < commit_index state.update_commit_index(100).unwrap(); let last_included = LogId { term: 1, index: 50 }; @@ -1842,7 +1645,7 @@ mod snapshot_tests { }; let snapshot_result = Ok((metadata, std::path::PathBuf::from("/tmp/snap.bin"))); - let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); let result = state .handle_snapshot_created(snapshot_result, &context, &internal_event_tx) .await; @@ -1852,10 +1655,17 @@ mod snapshot_tests { !state.snapshot_in_progress.load(Ordering::SeqCst), "snapshot_in_progress must be false after completion" ); - assert_eq!( - state.last_purged_index, - Some(last_included), - "last_purged_index must advance to snapshot's last_included after log purge" + // last_purged_index is updated via LogPurgeCompleted event (processed by event loop). + // Verify the event was dispatched with purge_upto = last_included - retained_log_entries. + let event = internal_event_rx + .try_recv() + .expect("LogPurgeCompleted event must be dispatched after successful purge"); + assert!( + matches!( + event, + InternalEvent::LogPurgeCompleted(LogId { index: 49, .. }) + ), + "purge boundary must be last_included.index(50) - retained(1) = 49, got: {event:?}" ); } @@ -1905,7 +1715,15 @@ mod snapshot_tests { #[tokio::test] async fn test_follower_snapshot_lifecycle() { let (_graceful_tx, graceful_rx) = watch::channel(()); - let (context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + let mut raft_log = mock_raft_log(); + raft_log.expect_entry_term().times(1).returning(|_| Some(1)); + let _temp_dir = tempfile::tempdir().unwrap(); + let mut nc = node_config(_temp_dir.path().to_str().unwrap()); + nc.raft.snapshot.retained_log_entries = 1; + let context = MockBuilder::new(graceful_rx) + .with_raft_log(raft_log) + .with_node_config(nc) + .build_context(); let mut state = FollowerState::::new(1, context.node_config.clone(), None, None); @@ -2913,6 +2731,72 @@ async fn test_follower_cluster_conf_always_exposes_current_leader() { // StreamSnapshot Rejection Tests // ============================================================================ +// ============================================================================ +// Role Transition Tests β€” scheduled_purge_upto / last_purged_index +// ============================================================================ + +/// From<&CandidateState>: last_purged_index preserved, scheduled_purge_upto reset. +/// +/// Candidate has no scheduled_purge_upto field; the resulting follower always starts +/// with None so it does not replay a stale purge boundary from a prior term. +#[test] +fn test_follower_from_candidate_preserves_last_purged_index_resets_scheduled() { + let cfg = Arc::new(node_config("/tmp/test_follower_from_candidate_purge")); + let mut candidate = CandidateState::::new(1, cfg); + candidate.last_purged_index = Some(LogId { term: 2, index: 10 }); + + let follower = FollowerState::from(&candidate); + + assert_eq!( + follower.last_purged_index, + Some(LogId { term: 2, index: 10 }) + ); + assert_eq!(follower.scheduled_purge_upto, None); +} + +/// From<&LeaderState>: both fields preserved across leader-to-follower stepdown. +/// +/// A leader with a pending purge intent (scheduled_purge_upto) must hand it off so the +/// follower can resume the purge without recomputing the boundary. +#[test] +fn test_follower_from_leader_preserves_both_purge_fields() { + let cfg = Arc::new(node_config("/tmp/test_follower_from_leader_purge")); + let mut leader = LeaderState::::new(1, cfg); + leader.last_purged_index = Some(LogId { term: 2, index: 20 }); + leader.scheduled_purge_upto = Some(LogId { term: 2, index: 18 }); + + let follower = FollowerState::from(&leader); + + assert_eq!( + follower.last_purged_index, + Some(LogId { term: 2, index: 20 }) + ); + assert_eq!( + follower.scheduled_purge_upto, + Some(LogId { term: 2, index: 18 }) + ); +} + +/// From<&LearnerState>: both fields preserved across learner-to-follower promotion. +#[test] +fn test_follower_from_learner_preserves_both_purge_fields() { + let cfg = Arc::new(node_config("/tmp/test_follower_from_learner_purge")); + let mut learner = LearnerState::::new(1, cfg); + learner.last_purged_index = Some(LogId { term: 1, index: 7 }); + learner.scheduled_purge_upto = Some(LogId { term: 1, index: 5 }); + + let follower = FollowerState::from(&learner); + + assert_eq!( + follower.last_purged_index, + Some(LogId { term: 1, index: 7 }) + ); + assert_eq!( + follower.scheduled_purge_upto, + Some(LogId { term: 1, index: 5 }) + ); +} + /// Test: Follower rejects StreamSnapshot β€” only Leader streams snapshots to Learners. /// /// Scenario: diff --git a/d-engine-core/src/raft_role/leader_state.rs b/d-engine-core/src/raft_role/leader_state.rs index 71b3934d..87d044a3 100644 --- a/d-engine-core/src/raft_role/leader_state.rs +++ b/d-engine-core/src/raft_role/leader_state.rs @@ -20,7 +20,6 @@ use crate::Membership; use crate::MembershipError; use crate::NetworkError; use crate::PeerUpdate; -use crate::PurgeExecutor; use crate::RaftContext; use crate::RaftLog; use crate::RaftNodeConfig; @@ -45,6 +44,7 @@ use crate::alias::TROF; use crate::client::{ClientReadRequest, ClientResponse, ErrorCode}; use crate::event::ClientCmd; use crate::network::Transport; +use crate::role_state::schedule_and_execute_purge; use async_trait::async_trait; use d_engine_proto::common::AddNode; use d_engine_proto::common::BatchPromote; @@ -53,6 +53,7 @@ use d_engine_proto::common::EntryPayload; use d_engine_proto::common::LogId; use d_engine_proto::common::NodeRole::Leader; use d_engine_proto::common::NodeStatus; +use d_engine_proto::common::entry_payload::Payload; use d_engine_proto::common::membership_change::Change; use d_engine_proto::server::cluster::ClusterConfUpdateResponse; use d_engine_proto::server::cluster::JoinRequest; @@ -83,7 +84,6 @@ use tonic::Status; use tracing::debug; use tracing::error; use tracing::info; -use tracing::instrument; use tracing::trace; use tracing::warn; @@ -530,7 +530,6 @@ impl RaftRoleState for LeaderState { ///Overwrite default behavior. /// As leader, I should not receive commit index, /// which is lower than my current one - #[tracing::instrument] fn update_commit_index( &mut self, new_commit_index: u64, @@ -918,8 +917,6 @@ impl RaftRoleState for LeaderState { cmd: ClientCmd, ctx: &RaftContext, ) { - use crate::client_command_to_entry_payloads; - let backpressure = &ctx.node_config.raft.backpressure; match cmd { @@ -950,11 +947,9 @@ impl RaftRoleState for LeaderState { // Convert command to payload (will be merged in drain_batch) if let Some(cmd) = req.command { - let payload = client_command_to_entry_payloads(vec![write_op_to_proto(cmd)]) - .into_iter() - .next() - .expect("client_command_to_entry_payloads should return 1 element"); - + let payload = EntryPayload { + payload: Some(Payload::Command(cmd)), + }; // Push lightweight tuple directly (zero-copy, no Vec allocation) self.propose_buffer.push(payload, sender); } else { @@ -1758,6 +1753,9 @@ impl RaftRoleState for LeaderState { "Updating last purged index after successful execution" ); self.last_purged_index = Some(purged_id); + + // purge completed, clear to prevent re-execution + self.scheduled_purge_upto = None; } else { warn!( ?purged_id, @@ -1769,39 +1767,6 @@ impl RaftRoleState for LeaderState { Ok(()) } - /// Trigger an independent snapshot on this role (Raft Β§7 β€” each server snapshots - /// independently). Called when `should_snapshot()` returns true after SM apply. - /// Default: no-op. Candidate overrides to return `RoleViolation`. - async fn handle_create_snapshot( - &mut self, - ctx: &RaftContext, - internal_event_tx: &mpsc::UnboundedSender, - ) -> Result<()> { - // Prevent duplicate snapshot creation - if self.snapshot_in_progress.load(std::sync::atomic::Ordering::Acquire) { - info!("Snapshot creation already in progress. Skipping duplicate request."); - return Ok(()); - } - - self.snapshot_in_progress.store(true, std::sync::atomic::Ordering::Release); - let state_machine_handler = ctx.state_machine_handler().clone(); - - // Use spawn to perform snapshot creation in the background - let internal_event_tx = internal_event_tx.clone(); - tokio::spawn(async move { - let result = state_machine_handler.create_snapshot().await; - info!("SnapshotCreated event will be processed in another event thread"); - if let Err(e) = internal_event_tx.send(InternalEvent::SnapshotCreated(result)) { - error!("Failed to send snapshot creation result: {e:?}"); - } - }); - - Ok(()) - } - /// Process the completed snapshot result (success or error). - /// Leader: schedules log purge up to `last_included`. - /// Follower/Learner: updates local snapshot path. - /// Default: no-op. Candidate overrides to return `RoleViolation`. async fn handle_snapshot_created( &mut self, result: crate::Result<(SnapshotMetadata, std::path::PathBuf)>, @@ -1809,57 +1774,29 @@ impl RaftRoleState for LeaderState { internal_event_tx: &mpsc::UnboundedSender, ) -> Result<()> { self.snapshot_in_progress.store(false, Ordering::SeqCst); - match result { - Err(e) => { - error!(%e, "State machine snapshot creation failed"); - } - Ok(( - SnapshotMetadata { - last_included: last_included_option, - checksum: _, - }, - _final_path, - )) => { - info!("Initiating log purge after snapshot creation"); - - if let Some(last_included) = last_included_option { - // ---------------------- - // Phase 1: Schedule log purge if possible - // ---------------------- - trace!("Phase 1: Schedule log purge if possible"); - if self.can_purge_logs(self.last_purged_index, last_included) { - trace!(?last_included, "Phase 1: Scheduling log purge"); - self.scheduled_purge_upto(last_included); - } - - // ---------------------- - // Phase 2: Execute local purge - // ---------------------- - // Per Raft Β§7: Leader purges independently without peer coordination - trace!("Phase 2: Execute scheduled purge task"); - debug!(?last_included, "Execute scheduled purge task"); - if let Some(scheduled) = self.scheduled_purge_upto { - let purge_executor = ctx.purge_executor(); - match purge_executor.execute_purge(scheduled).await { - Ok(_) => { - if let Err(e) = internal_event_tx - .send(InternalEvent::LogPurgeCompleted(scheduled)) - { - error!(%e, "Failed to notify purge completion"); - } - } - Err(e) => { - error!(?e, ?scheduled, "Log purge execution failed"); - } - } - } + Err(e) => error!(?e, "Leader snapshot creation failed"), + Ok((metadata, _)) => { + if let Some(last_included) = metadata.last_included { + schedule_and_execute_purge( + last_included, + ctx, + self.commit_index(), + self.last_purged_index, + &mut self.scheduled_purge_upto, + internal_event_tx, + ) + .await?; } } } Ok(()) } + fn snapshot_in_progress(&self) -> Option<&AtomicBool> { + Some(&self.snapshot_in_progress) + } + /// Check pending learners for promotion eligibility after a membership change. /// Leader only: evaluates `pending_promotions`, proposes config change if ready. /// Default: no-op + warn (unexpected on non-leader). @@ -2609,7 +2546,6 @@ impl LeaderState { } /// The fun will retrieve current Leader state snapshot - #[tracing::instrument] pub fn leader_state_snapshot(&self) -> LeaderStateSnapshot { LeaderStateSnapshot { next_index: self.next_index.clone(), @@ -2736,7 +2672,6 @@ impl LeaderState { /// advance; the ACK is a lower-bound confirmation, not a correction. /// - Conflict: `next_index = conflict_hint` (floor-guarded by `match_index + 1`) β€” /// the follower explicitly corrects the leader's assumption; retreat is required. - #[instrument(skip(self))] pub(super) fn update_peer_index( &mut self, follower_id: u32, @@ -2982,7 +2917,6 @@ impl LeaderState { } /// Calculate new submission index - #[instrument(skip(self))] fn calculate_new_commit_index( &self, raft_log: &Arc>, @@ -3052,25 +2986,6 @@ impl LeaderState { Ok(()) } - #[instrument(skip(self))] - fn scheduled_purge_upto( - &mut self, - received_last_included: LogId, - ) { - if let Some(existing) = self.scheduled_purge_upto - && existing.index >= received_last_included.index - { - warn!( - ?received_last_included, - ?existing, - "Will not update scheduled_purge_upto, received invalid last_included log" - ); - return; - } - info!(?self.scheduled_purge_upto, ?received_last_included, "Updte scheduled_purge_upto."); - self.scheduled_purge_upto = Some(received_last_included); - } - fn send_become_follower_event( &self, new_leader_id: Option, @@ -3090,54 +3005,6 @@ impl LeaderState { Ok(()) } - /// Determines if logs prior to `last_included_in_snapshot` can be permanently discarded. - /// - /// Implements Leader-side log compaction safety checks per Raft paper Β§7.2: - /// > "The leader uses a new RPC called InstallSnapshot to send snapshots to followers that are - /// > too far behind" - /// - /// # Safety Invariants (ALL must hold) - /// - /// 1. **Committed Entry Guarantee** `last_included_in_snapshot.index < self.commit_index` - /// - Ensures we never discard uncommitted entries (Raft Β§5.4.2) - /// - Maintains at least one committed entry after purge for log matching property - /// - /// 2. **Monotonic Snapshot Advancement** `last_purge_index < last_included_in_snapshot.index` - /// - Enforces snapshot indices strictly increase (prevents rollback attacks) - /// - Maintains sequential purge ordering (FSM safety requirement) - /// - /// 3. **Operation Atomicity** `pending_purge.is_none()` - /// - Ensures only one concurrent purge operation - /// - Critical for linearizable state machine semantics - /// - /// # Implementation Notes - /// - Per Raft Β§7: "Each server compacts its log independently" - /// - Leader purges immediately after snapshot without waiting for followers - /// - Lagging followers recover via InstallSnapshot RPC - /// - Actual log discard should be deferred until storage confirms snapshot persistence - #[instrument(skip(self))] - pub fn can_purge_logs( - &self, - last_purge_index: Option, - last_included_in_snapshot: LogId, - ) -> bool { - let commit_index = self.commit_index(); - debug!( - ?commit_index, - ?last_purge_index, - ?last_included_in_snapshot, - "can_purge_logs" - ); - - let monotonic_check = last_purge_index - .map(|lid| lid.index < last_included_in_snapshot.index) - .unwrap_or(true); - - // Per Raft Β§7: Leader purges independently after snapshot - // No peer coordination required - lagging followers get InstallSnapshot - last_included_in_snapshot.index < commit_index && monotonic_check - } - pub async fn handle_join_cluster( &mut self, join_request: JoinRequest, @@ -4177,42 +4044,3 @@ pub fn calculate_safe_batch_size( available.saturating_sub(1) } } - -// Serialize a native WriteOperation to a proto WriteCommand for Raft log encoding. -// Proto bytes in the log must remain stable; this conversion is the only place -// that touches prost types on the write path. -#[inline] -pub(crate) fn write_op_to_proto( - op: crate::client::WriteOperation -) -> d_engine_proto::client::WriteCommand { - use crate::client::WriteOperation; - use d_engine_proto::client::WriteCommand; - use d_engine_proto::client::write_command::{CompareAndSwap, Delete, Insert, Operation}; - match op { - WriteOperation::Insert { - key, - value, - ttl_secs, - } => WriteCommand { - operation: Some(Operation::Insert(Insert { - key, - value, - ttl_secs: ttl_secs.unwrap_or(0), - })), - }, - WriteOperation::Delete { key } => WriteCommand { - operation: Some(Operation::Delete(Delete { key })), - }, - WriteOperation::CompareAndSwap { - key, - expected, - new_value, - } => WriteCommand { - operation: Some(Operation::CompareAndSwap(CompareAndSwap { - key, - expected_value: expected, - new_value, - })), - }, - } -} diff --git a/d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs b/d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs index aaa205c8..eec732ec 100644 --- a/d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs @@ -1,7 +1,8 @@ use crate::client::ClientReadRequest; use crate::client::ClientWriteRequest; -use crate::client::WriteOperation; use crate::config::ReadConsistencyPolicy; +use bytes::Bytes; +use d_engine_proto::client::WriteCommand; use tokio::sync::watch; use crate::BackpressureConfig; @@ -40,8 +41,9 @@ async fn test_backpressure_write_limit_enforcement() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: i, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), + command: Some({ + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) }), }, resp_tx, @@ -60,8 +62,9 @@ async fn test_backpressure_write_limit_enforcement() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: 3, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), + command: Some({ + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) }), }, resp_tx, @@ -177,8 +180,9 @@ async fn test_backpressure_unlimited_when_zero() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: i, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), + command: Some({ + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) }), }, resp_tx, @@ -220,8 +224,9 @@ async fn test_backpressure_write_and_read_independent() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: i, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), + command: Some({ + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) }), }, resp_tx, @@ -234,8 +239,9 @@ async fn test_backpressure_write_and_read_independent() { let write_cmd = ClientCmd::Propose( ClientWriteRequest { client_id: 99, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), + command: Some({ + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) }), }, write_tx, diff --git a/d-engine-core/src/raft_role/leader_state_test/buffer_cleanup_test.rs b/d-engine-core/src/raft_role/leader_state_test/buffer_cleanup_test.rs index 55d1f2cc..7d3749ce 100644 --- a/d-engine-core/src/raft_role/leader_state_test/buffer_cleanup_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/buffer_cleanup_test.rs @@ -5,8 +5,10 @@ use crate::MockBuilder; use crate::RaftOneshot; use crate::RaftRole; use crate::client::ClientWriteRequest; -use crate::client::WriteOperation; use crate::raft_role::role_state::RaftRoleState; +use bytes::Bytes; +use d_engine_proto::client::WriteCommand; +use prost::Message; use std::sync::Arc; use tokio::sync::watch; @@ -70,11 +72,13 @@ async fn test_leader_stepdown_clears_pending_write_buffer() { let write_req = ClientWriteRequest { client_id: 1, - command: Some(WriteOperation::Insert { - key: bytes::Bytes::from(format!("key_{i}")), - value: bytes::Bytes::from(format!("value_{i}")), - ttl_secs: None, - }), + command: Some(bytes::Bytes::from( + WriteCommand::insert( + bytes::Bytes::from(format!("key_{i}")), + bytes::Bytes::from(format!("value_{i}")), + ) + .encode_to_vec(), + )), }; let cmd = ClientCmd::Propose(write_req, response_tx); @@ -151,11 +155,13 @@ async fn test_leader_stepdown_clears_pending_write_buffer() { let (response_tx, mut response_rx) = MaybeCloneOneshot::new(); let write_req = ClientWriteRequest { client_id: 1, - command: Some(WriteOperation::Insert { - key: bytes::Bytes::from("new_key"), - value: bytes::Bytes::from("new_value"), - ttl_secs: None, - }), + command: Some(Bytes::from( + WriteCommand::insert( + bytes::Bytes::from("new_key"), + bytes::Bytes::from("new_value"), + ) + .encode_to_vec(), + )), }; let cmd = ClientCmd::Propose(write_req, response_tx); diff --git a/d-engine-core/src/raft_role/leader_state_test/client_write_test.rs b/d-engine-core/src/raft_role/leader_state_test/client_write_test.rs index 3c424135..989c15c2 100644 --- a/d-engine-core/src/raft_role/leader_state_test/client_write_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/client_write_test.rs @@ -9,8 +9,7 @@ use crate::MockRaftLog; use crate::MockReplicationCore; use crate::PrepareResult; use crate::RaftRequestWithSignal; -use crate::client::{ClientWriteRequest, WriteOperation}; -use crate::client_command_to_entry_payloads; +use crate::client::ClientWriteRequest; use crate::event::{InternalEvent, NewCommitData}; use crate::maybe_clone_oneshot::MaybeCloneOneshot; use crate::maybe_clone_oneshot::RaftOneshot; @@ -22,6 +21,7 @@ use crate::test_utils::MockBuilder; use crate::test_utils::mock::MockTypeConfig; use crate::test_utils::node_config; use bytes::Bytes; +use d_engine_proto::client::WriteCommand; use d_engine_proto::common::AddNode; use d_engine_proto::common::Entry; use d_engine_proto::common::EntryPayload; @@ -146,11 +146,13 @@ async fn test_process_raft_request_immediate_execution() { .await; // Prepare test request + let cmd_bytes = { + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) + }; let request = ClientWriteRequest { client_id: 0, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), - }), + command: Some(cmd_bytes.clone()), }; use crate::maybe_clone_oneshot::MaybeCloneOneshot; let (tx, rx) = >::new(); @@ -162,13 +164,7 @@ async fn test_process_raft_request_immediate_execution() { .execute_request_immediately( RaftRequestWithSignal { id: rand::distr::Alphanumeric.sample_string(&mut rand::rng(), 21), - payloads: client_command_to_entry_payloads( - request - .command - .into_iter() - .map(crate::raft_role::leader_state::write_op_to_proto) - .collect(), - ), + payloads: request.command.into_iter().map(EntryPayload::command).collect(), senders: vec![tx], wait_for_apply_event: true, }, @@ -377,11 +373,13 @@ async fn test_process_raft_request_two_consecutive_forced_sends() { }; // Prepare test requests + let cmd_bytes = { + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) + }; let request = ClientWriteRequest { client_id: 0, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), - }), + command: Some(cmd_bytes), }; use crate::maybe_clone_oneshot::MaybeCloneOneshot; let (tx1, rx1) = >::new(); @@ -394,14 +392,7 @@ async fn test_process_raft_request_two_consecutive_forced_sends() { .execute_request_immediately( RaftRequestWithSignal { id: rand::distr::Alphanumeric.sample_string(&mut rand::rng(), 21), - payloads: client_command_to_entry_payloads( - request - .command - .clone() - .into_iter() - .map(crate::raft_role::leader_state::write_op_to_proto) - .collect(), - ), + payloads: request.command.clone().into_iter().map(EntryPayload::command).collect(), senders: vec![tx1], wait_for_apply_event: true, }, @@ -416,13 +407,7 @@ async fn test_process_raft_request_two_consecutive_forced_sends() { .execute_request_immediately( RaftRequestWithSignal { id: rand::distr::Alphanumeric.sample_string(&mut rand::rng(), 21), - payloads: client_command_to_entry_payloads( - request - .command - .into_iter() - .map(crate::raft_role::leader_state::write_op_to_proto) - .collect(), - ), + payloads: request.command.into_iter().map(EntryPayload::command).collect(), senders: vec![tx2], wait_for_apply_event: true, }, @@ -514,8 +499,9 @@ async fn test_process_raft_request_batching_enabled() { // Prepare test request let request = ClientWriteRequest { client_id: 0, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), + command: Some({ + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) }), }; use crate::ClientCmd; @@ -584,8 +570,9 @@ async fn test_drain_single_write_no_delay() { let start = Instant::now(); let req = ClientWriteRequest { client_id: 1, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), + command: Some({ + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) }), }; let (tx, mut rx) = MaybeCloneOneshot::new(); @@ -683,8 +670,9 @@ async fn test_drain_multiple_writes_natural_batch() { for i in 0..10 { let req = ClientWriteRequest { client_id: i, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), + command: Some({ + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) }), }; let (tx, rx) = MaybeCloneOneshot::new(); @@ -771,8 +759,9 @@ async fn test_drain_max_batch_size_limit() { for i in 0..10 { let req = ClientWriteRequest { client_id: i, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), + command: Some({ + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) }), }; let (tx, _rx) = MaybeCloneOneshot::new(); @@ -859,8 +848,9 @@ async fn test_write_batch_single_replication() { for i in 0..5 { let req = ClientWriteRequest { client_id: i, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), + command: Some({ + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) }), }; let (tx, rx) = MaybeCloneOneshot::new(); @@ -903,110 +893,8 @@ async fn test_write_batch_single_replication() { drop(_shutdown_tx); } -// ============================================================================ -// write_op_to_proto conversion tests -// ============================================================================ - -/// Verify Insert with TTL converts correctly to proto WriteCommand. -#[test] -fn test_write_op_to_proto_insert_with_ttl() { - use crate::client::WriteOperation; - use crate::raft_role::leader_state::write_op_to_proto; - use d_engine_proto::client::write_command::Operation; - - let op = WriteOperation::Insert { - key: Bytes::from("k"), - value: Bytes::from("v"), - ttl_secs: Some(60), - }; - let cmd = write_op_to_proto(op); - match cmd.operation { - Some(Operation::Insert(ins)) => { - assert_eq!(ins.key, Bytes::from("k")); - assert_eq!(ins.value, Bytes::from("v")); - assert_eq!(ins.ttl_secs, 60); - } - other => panic!("expected Insert, got {:?}", other), - } -} - -/// Verify Insert with no TTL maps ttl_secs=None β†’ proto ttl_secs=0. -#[test] -fn test_write_op_to_proto_insert_no_ttl_maps_to_zero() { - use crate::client::WriteOperation; - use crate::raft_role::leader_state::write_op_to_proto; - use d_engine_proto::client::write_command::Operation; - - let op = WriteOperation::Insert { - key: Bytes::from("k"), - value: Bytes::from("v"), - ttl_secs: None, - }; - let cmd = write_op_to_proto(op); - match cmd.operation { - Some(Operation::Insert(ins)) => assert_eq!(ins.ttl_secs, 0), - other => panic!("expected Insert, got {:?}", other), - } -} - -/// Verify Delete converts correctly to proto WriteCommand. -#[test] -fn test_write_op_to_proto_delete() { - use crate::client::WriteOperation; - use crate::raft_role::leader_state::write_op_to_proto; - use d_engine_proto::client::write_command::Operation; - - let op = WriteOperation::Delete { - key: Bytes::from("del-key"), - }; - let cmd = write_op_to_proto(op); - match cmd.operation { - Some(Operation::Delete(del)) => assert_eq!(del.key, Bytes::from("del-key")), - other => panic!("expected Delete, got {:?}", other), - } -} - -/// Verify CAS with expected value converts correctly. -#[test] -fn test_write_op_to_proto_cas_with_expected() { - use crate::client::WriteOperation; - use crate::raft_role::leader_state::write_op_to_proto; - use d_engine_proto::client::write_command::Operation; - - let op = WriteOperation::CompareAndSwap { - key: Bytes::from("k"), - expected: Some(Bytes::from("old")), - new_value: Bytes::from("new"), - }; - let cmd = write_op_to_proto(op); - match cmd.operation { - Some(Operation::CompareAndSwap(cas)) => { - assert_eq!(cas.key, Bytes::from("k")); - assert_eq!(cas.expected_value, Some(Bytes::from("old"))); - assert_eq!(cas.new_value, Bytes::from("new")); - } - other => panic!("expected CompareAndSwap, got {:?}", other), - } -} - -/// CAS with expected=None maps to expected_value=None (key-must-not-exist semantics). -#[test] -fn test_write_op_to_proto_cas_key_must_not_exist() { - use crate::client::WriteOperation; - use crate::raft_role::leader_state::write_op_to_proto; - use d_engine_proto::client::write_command::Operation; - - let op = WriteOperation::CompareAndSwap { - key: Bytes::from("k"), - expected: None, - new_value: Bytes::from("new"), - }; - let cmd = write_op_to_proto(op); - match cmd.operation { - Some(Operation::CompareAndSwap(cas)) => assert!(cas.expected_value.is_none()), - other => panic!("expected CompareAndSwap, got {:?}", other), - } -} +// WriteOperation β†’ WriteCommand conversion tests live in d-engine-server/src/api/types_test.rs +// (WriteOperation was moved to server layer as part of Type B refactor) // ============================================================================ // wait_for_apply_event Semantic Contract Tests @@ -1084,8 +972,9 @@ async fn test_client_write_deferred_until_sm_apply() { let req = ClientWriteRequest { client_id: 1, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), + command: Some({ + use prost::Message; + Bytes::from(WriteCommand::delete(Bytes::new()).encode_to_vec()) }), }; let (tx, mut rx) = MaybeCloneOneshot::new(); diff --git a/d-engine-core/src/raft_role/leader_state_test/event_handling_test.rs b/d-engine-core/src/raft_role/leader_state_test/event_handling_test.rs index 089918c4..aa4a6c7c 100644 --- a/d-engine-core/src/raft_role/leader_state_test/event_handling_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/event_handling_test.rs @@ -3,12 +3,6 @@ //! This module tests the `handle_inbound_event` method for various inbound events //! including vote requests, append entries, client operations, and more. -use crate::client::WriteOperation; -use std::sync::Arc; -use tokio::sync::{mpsc, watch}; -use tonic::Code; -use tracing_test::traced_test; - use crate::ClientCmd; use crate::Error; use crate::MockRaftLog; @@ -26,12 +20,19 @@ use crate::test_utils::create_test_chunk; use crate::test_utils::mock::mock_raft_context; use crate::test_utils::mock::{MockBuilder, MockTypeConfig}; use crate::utils::convert::safe_kv_bytes; +use bytes::Bytes; +use d_engine_proto::client::WriteCommand; use d_engine_proto::server::cluster::{ ClusterConfChangeRequest, ClusterMembership, MetadataRequest, }; use d_engine_proto::server::election::{VoteRequest, VoteResponse}; use d_engine_proto::server::replication::{AppendEntriesRequest, AppendEntriesResponse}; +use prost::Message; +use std::sync::Arc; +use tokio::sync::{mpsc, watch}; +use tonic::Code; use tonic::Status; +use tracing_test::traced_test; // ============================================================================ // Test Helper Functions @@ -537,9 +538,9 @@ async fn test_handle_client_propose_success() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: 1, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), - }), + command: Some(Bytes::from( + WriteCommand::delete(Bytes::new()).encode_to_vec(), + )), }, resp_tx, ); diff --git a/d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs b/d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs index 25a9fe1f..df72bb28 100644 --- a/d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/snapshot_test.rs @@ -7,12 +7,12 @@ use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::Ordering; -use crate::SnapshotError; use crate::config::RaftNodeConfig as CoreRaftNodeConfig; use crate::event::InboundEvent; use crate::raft_role::leader_state::LeaderState; use crate::role_state::RaftRoleState; -use crate::test_utils::mock::{MockBuilder, MockTypeConfig}; +use crate::test_utils::mock::{MockBuilder, MockTypeConfig, mock_raft_log}; +use crate::{MockPurgeExecutor, SnapshotError}; use d_engine_proto::common::LogId; use d_engine_proto::server::storage::SnapshotMetadata; use tokio::sync::{mpsc, watch}; @@ -85,7 +85,13 @@ async fn test_create_snapshot_event_starts_and_ignores_duplicates() { #[tokio::test] async fn test_snapshot_in_progress_flag_reset_on_created_event() { let (_graceful_tx, graceful_rx) = watch::channel(()); - let context = MockBuilder::new(graceful_rx).build_context(); + let mut raft_log = mock_raft_log(); + raft_log + .expect_entry_term() + .withf(|&idx| idx == 9) + .times(1) + .returning(|_| Some(1)); + let context = MockBuilder::new(graceful_rx).with_raft_log(raft_log).build_context(); let mut state = LeaderState::::new(1, context.node_config()); state.snapshot_in_progress.store(true, Ordering::SeqCst); @@ -211,8 +217,13 @@ fn test_handle_log_purge_completed() { // Test first purge state.last_purged_index = None; + state.scheduled_purge_upto = Some(LogId { term: 1, index: 50 }); state.handle_log_purge_completed(LogId { term: 1, index: 50 }).unwrap(); assert_eq!(state.last_purged_index, Some(LogId { term: 1, index: 50 })); + assert!( + state.scheduled_purge_upto.is_none(), + "handle_log_purge_completed must clear scheduled_purge_upto after successful purge" + ); } // ============================================================================ @@ -293,7 +304,6 @@ async fn test_stream_snapshot_confirms_startup_when_metadata_exists() { sm.expect_stop().returning(|| Ok(())); sm.expect_is_running().returning(|| true); sm.expect_get().returning(|_| Ok(None)); - sm.expect_entry_term().returning(|_| None); sm.expect_apply_chunk().returning(|_| Ok(vec![])); sm.expect_len().returning(|| 0); sm.expect_update_last_applied().returning(|_| ()); @@ -364,3 +374,271 @@ async fn test_stream_snapshot_confirms_startup_when_metadata_exists() { assert_eq!(chunk.seq, 0); } + +// ============================================================================ +// Purge Boundary Tests (#415 fix β€” log retention decoupled from snapshot label) +// ============================================================================ + +/// Purge boundary must be last_included.index - retained_log_entries. +/// +/// # Purpose +/// Log retention is decoupled from the snapshot label. +/// After a truthful snapshot at last_included=100, the leader must purge only up to +/// index 90 (given retained=10), keeping entries 91~100 available for lagging followers +/// to catch up via AppendEntries instead of InstallSnapshot. +/// The purge boundary is computed in handle_snapshot_created using entry_term from the +/// actual log β€” no term is fabricated. +/// +/// # Criteria +/// - last_included = LogId { index: 100, term: 2 } +/// - retained_log_entries = 10 +/// - raft_log.entry_term(90) returns Some(1) (entry 90 was written in term 1) +/// - commit_index = 200 (ensures can_purge_logs returns true) +/// +/// # Expected +/// - state.scheduled_purge_upto == Some(LogId { index: 90, term: 1 }) +/// - entry_term is queried with index 90 +/// - no LogId with a fabricated term is constructed +#[tokio::test] +async fn test_purge_boundary_is_last_included_minus_retained_log_entries() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + + // entry_term(90) β†’ Some(1): entry 90 was written in term 1, not term 2 + // withf(idx == 90) also verifies the correct purge boundary index is queried + let mut raft_log = mock_raft_log(); + raft_log + .expect_entry_term() + .withf(|&idx| idx == 90) + .times(1) + .returning(|_| Some(1)); + + // retained_log_entries = 10 β†’ purge_upto_index = 100 - 10 = 90 + let mut nc = CoreRaftNodeConfig::default(); + nc.raft.snapshot.retained_log_entries = 10; + + let context = MockBuilder::new(graceful_rx) + .with_raft_log(raft_log) + .with_node_config(nc) + .build_context(); + + let mut state = LeaderState::::new(1, context.node_config()); + // commit_index must be > last_included.index (100) for can_purge_logs to return true + state.update_commit_index(200).unwrap(); + + let _ = state + .handle_snapshot_created( + Ok(( + SnapshotMetadata { + last_included: Some(LogId { + term: 2, + index: 100, + }), + checksum: "abc".into(), + }, + PathBuf::from("/tmp/fake"), + )), + &context, + &internal_event_tx, + ) + .await; + + // Purge boundary = last_included.index - retained = 90 + // term comes from entry_term(90) = 1 β€” NOT the fabricated term=2 from last_included + assert_eq!( + state.scheduled_purge_upto, + Some(LogId { term: 1, index: 90 }) + ); +} + +/// Purge is skipped entirely when retained_log_entries >= last_included.index. +/// +/// # Purpose +/// Guards the saturating_sub(retained) == 0 branch. When the cluster is still young +/// (e.g., only 5 entries written) and retained_log_entries >= last_included.index, +/// the computed purge_upto_index underflows to 0. The code must detect this and skip +/// the purge β€” not schedule a no-op, not call entry_term, not panic. +/// +/// # Criteria +/// - last_included = LogId { index: 5, term: 1 } +/// - retained_log_entries = 10 (10 >= 5 β†’ saturating_sub = 0) +/// - commit_index = 200 +/// +/// # Expected +/// - state.scheduled_purge_upto remains None +/// - raft_log.entry_term is NOT called +/// - purge_executor.execute_purge is NOT called +#[tokio::test] +async fn test_purge_skipped_when_retained_exceeds_last_included_index() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + + let mut raft_log = mock_raft_log(); + raft_log.expect_entry_term().times(0).returning(|_| Some(1)); + + let mut purge_executor = MockPurgeExecutor::new(); + purge_executor.expect_execute_purge().times(0).returning(|_| Ok(())); + + let mut nc = CoreRaftNodeConfig::default(); + nc.raft.snapshot.retained_log_entries = 10; + + let context = MockBuilder::new(graceful_rx) + .with_raft_log(raft_log) + .with_purge_executor(purge_executor) + .with_node_config(nc) + .build_context(); + + let mut state = LeaderState::::new(1, context.node_config()); + // commit_index must be > last_included.index (100) for can_purge_logs to return true + state.update_commit_index(200).unwrap(); + + let _ = state + .handle_snapshot_created( + Ok(( + SnapshotMetadata { + last_included: Some(LogId { term: 1, index: 5 }), + checksum: "abc".into(), + }, + PathBuf::from("/tmp/fake"), + )), + &context, + &internal_event_tx, + ) + .await; + + assert_eq!(state.scheduled_purge_upto, None); +} + +/// Purge advances to last_included.index - 1 when retained_log_entries is at minimum (1). +/// +/// # Purpose +/// Validates the minimum-retained case. retained_log_entries = 1 (the minimum allowed +/// value) means exactly one trailing entry is kept for replication continuity. +/// The purge boundary is last_included.index - 1. +/// +/// # Criteria +/// - last_included = LogId { index: 100, term: 2 } +/// - retained_log_entries = 1 +/// - raft_log.entry_term(99) returns Some(2) +/// - commit_index = 200 +/// +/// # Expected +/// - state.scheduled_purge_upto == Some(LogId { index: 99, term: 2 }) +#[tokio::test] +async fn test_purge_boundary_with_minimum_retained_log_entries() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + + let mut raft_log = mock_raft_log(); + raft_log + .expect_entry_term() + .withf(|&idx| idx == 99) + .times(1) + .returning(|_| Some(2)); + + let mut purge_executor = MockPurgeExecutor::new(); + purge_executor.expect_execute_purge().times(1).returning(|_| Ok(())); + + let mut nc = CoreRaftNodeConfig::default(); + nc.raft.snapshot.retained_log_entries = 1; + + let context = MockBuilder::new(graceful_rx) + .with_raft_log(raft_log) + .with_purge_executor(purge_executor) + .with_node_config(nc) + .build_context(); + + let mut state = LeaderState::::new(1, context.node_config()); + // commit_index must be > last_included.index (100) for can_purge_logs to return true + state.update_commit_index(200).unwrap(); + + let _ = state + .handle_snapshot_created( + Ok(( + SnapshotMetadata { + last_included: Some(LogId { + term: 2, + index: 100, + }), + checksum: "abc".into(), + }, + PathBuf::from("/tmp/fake"), + )), + &context, + &internal_event_tx, + ) + .await; + + assert_eq!( + state.scheduled_purge_upto, + Some(LogId { term: 2, index: 99 }) + ); +} + +/// Purge is skipped when entry_term returns None for the computed purge boundary. +/// +/// # Purpose +/// Protocol safety guard: the purge boundary requires a valid +/// LogId { index, term }. If raft_log.entry_term(purge_upto_index) returns None +/// (entry already purged or out of range), fabricating a LogId with term=0 would +/// violate the Raft protocol β€” term=0 is the sentinel for "never voted" and must +/// never appear in a LogId. The correct behavior is to skip this purge cycle; +/// the next snapshot cycle will recompute a valid boundary. +/// +/// # Criteria +/// - last_included = LogId { index: 100, term: 2 } +/// - retained_log_entries = 10 (purge_upto_index = 90) +/// - raft_log.entry_term(90) returns None +/// - commit_index = 200 +/// +/// # Expected +/// - state.scheduled_purge_upto remains None +/// - purge_executor.execute_purge is NOT called +/// - no LogId with term=0 is constructed +#[tokio::test] +async fn test_purge_skipped_when_entry_term_returns_none() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + + let mut raft_log = mock_raft_log(); + raft_log + .expect_entry_term() + .withf(|&idx| idx == 90) + .times(1) + .returning(|_| None); + + let mut purge_executor = MockPurgeExecutor::new(); + purge_executor.expect_execute_purge().times(0).returning(|_| Ok(())); + + let mut nc = CoreRaftNodeConfig::default(); + nc.raft.snapshot.retained_log_entries = 10; + + let context = MockBuilder::new(graceful_rx) + .with_raft_log(raft_log) + .with_purge_executor(purge_executor) + .with_node_config(nc) + .build_context(); + + let mut state = LeaderState::::new(1, context.node_config()); + // commit_index must be > last_included.index (100) for can_purge_logs to return true + state.update_commit_index(200).unwrap(); + + let _ = state + .handle_snapshot_created( + Ok(( + SnapshotMetadata { + last_included: Some(LogId { + term: 2, + index: 100, + }), + checksum: "abc".into(), + }, + PathBuf::from("/tmp/fake"), + )), + &context, + &internal_event_tx, + ) + .await; + + assert_eq!(state.scheduled_purge_upto, None); +} diff --git a/d-engine-core/src/raft_role/leader_state_test/snapshot_worker_test.rs b/d-engine-core/src/raft_role/leader_state_test/snapshot_worker_test.rs index f098d803..7f5a7f96 100644 --- a/d-engine-core/src/raft_role/leader_state_test/snapshot_worker_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/snapshot_worker_test.rs @@ -76,7 +76,6 @@ fn state_machine_with_snapshot() -> MockStateMachine { sm.expect_stop().returning(|| Ok(())); sm.expect_is_running().returning(|| true); sm.expect_get().returning(|_| Ok(None)); - sm.expect_entry_term().returning(|_| None); sm.expect_apply_chunk().returning(|_| Ok(vec![])); sm.expect_len().returning(|| 0); sm.expect_update_last_applied().returning(|_| ()); diff --git a/d-engine-core/src/raft_role/leader_state_test/state_management_test.rs b/d-engine-core/src/raft_role/leader_state_test/state_management_test.rs index 1f9780ba..6f4b900f 100644 --- a/d-engine-core/src/raft_role/leader_state_test/state_management_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/state_management_test.rs @@ -1,24 +1,25 @@ //! Tests for leader state management utilities and helper functions //! //! This module tests various state management functions including: -//! - Log purge validation (can_purge_logs) //! - Leader discovery event handling //! - State size tracking use std::mem::size_of; use std::sync::Arc; +use d_engine_proto::common::LogId; use tokio::sync::{mpsc, watch}; use tracing_test::traced_test; +use crate::candidate_state::CandidateState; use crate::event::InboundEvent; use crate::maybe_clone_oneshot::RaftOneshot; +use crate::node_config; use crate::raft_role::leader_state::LeaderState; use crate::role_state::RaftRoleState; use crate::test_utils::mock::MockTypeConfig; use crate::test_utils::mock::mock_raft_context; -use crate::test_utils::node_config; -use d_engine_proto::common::{LogId, NodeRole::Leader, NodeStatus}; +use d_engine_proto::common::{NodeRole::Leader, NodeStatus}; use d_engine_proto::server::cluster::{LeaderDiscoveryRequest, NodeMeta}; // ============================================================================ @@ -70,187 +71,6 @@ fn test_state_size() { println!("Cache lines occupied: {}", size.div_ceil(64)); } -// ============================================================================ -// Log Purge Validation Tests -// ============================================================================ - -/// Test can_purge_logs with valid purge conditions -/// -/// # Test Scenario -/// Validates proper purge window according to Raft paper Β§7.2 requirements. -/// -/// # Given -/// - commit_index = 100 -/// - peer_purge_progress: all peers at 100 -/// - last_purge_index = 90 -/// -/// # When -/// - Attempt to purge up to index 99 -/// -/// # Then -/// - Purge allowed (90 < 99 < 100) -/// - Boundary case: 99 == commit_index - 1 (valid gap) -/// - Invalid case: 100 not < 100 (violates gap rule) -#[test] -fn test_can_purge_logs_valid_conditions() { - let node_config = Arc::new(node_config("/tmp/test_can_purge_logs_valid_conditions")); - let mut state = LeaderState::::new(1, node_config); - - // Setup per Raft paper Β§7.2 requirements - state.shared_state.commit_index = 100; - - // Valid purge window (last_purge=90 < snapshot=99 < commit=100) - assert!(state.can_purge_logs( - Some(LogId { index: 90, term: 1 }), // last_purge_index - LogId { index: 99, term: 1 } // last_included_in_snapshot - )); - - // Boundary check: 99 == commit_index - 1 (valid gap) - assert!(state.can_purge_logs( - Some(LogId { index: 90, term: 1 }), - LogId { index: 99, term: 1 } - )); - - // Violate gap rule: 100 not < 100 - assert!(!state.can_purge_logs( - Some(LogId { index: 90, term: 1 }), - LogId { - index: 100, - term: 1 - } - )); -} - -/// Test can_purge_logs rejects uncommitted purge -/// -/// # Test Scenario -/// Reject purge attempts beyond commit index (Raft Β§5.4.2). -/// -/// # Given -/// - commit_index = 50 -/// - peer_purge_progress: peer 2 at 100 -/// -/// # When -/// - Attempt to purge beyond commit index -/// -/// # Then -/// - Purge rejected when snapshot_index > commit_index -/// - Boundary: snapshot_index == commit_index also rejected -#[test] -fn test_can_purge_logs_reject_uncommitted() { - let node_config = Arc::new(node_config("/tmp/test_can_purge_logs_reject_uncommitted")); - let mut state = LeaderState::::new(1, node_config); - - state.shared_state.commit_index = 50; - - // Attempt to purge beyond commit index - assert!(!state.can_purge_logs( - Some(LogId { index: 40, term: 1 }), - LogId { index: 51, term: 1 } // 51 > commit_index(50) - )); - - // Boundary violation: 50 == commit_index (requires <) - assert!(!state.can_purge_logs( - Some(LogId { index: 40, term: 1 }), - LogId { index: 50, term: 1 } - )); -} - -/// Test can_purge_logs enforces monotonicity -/// -/// # Test Scenario -/// Enforce purge sequence monotonicity (Raft Β§7.2). -/// Prevent backward or duplicate purges. -/// -/// # Given -/// - commit_index = 200 -/// - last_purge_index = 150 -/// -/// # When -/// - Attempt various purge sequences -/// -/// # Then -/// - Forward purge allowed (150 -> 199) -/// - Backward purge rejected (150 -> 120) -/// - Duplicate purge rejected (150 -> 150) -#[test] -fn test_can_purge_logs_enforce_monotonicity() { - let node_config = Arc::new(node_config("/tmp/test_can_purge_logs_enforce_monotonicity")); - let mut state = LeaderState::::new(1, node_config); - - state.shared_state.commit_index = 200; - - // Valid sequence: 100 β†’ 150 β†’ 199 - assert!(state.can_purge_logs( - Some(LogId { - index: 150, - term: 1 - }), - LogId { - index: 199, - term: 1 - } - )); - - // Invalid backward purge (150 β†’ 120) - assert!(!state.can_purge_logs( - Some(LogId { - index: 150, - term: 1 - }), - LogId { - index: 120, - term: 1 - } - )); - - // Same index purge attempt - assert!(!state.can_purge_logs( - Some(LogId { - index: 150, - term: 1 - }), - LogId { - index: 150, - term: 1 - } - )); -} - -/// Test can_purge_logs verifies cluster progress -/// -/// # Test Scenario -/// Enhanced durability check - ensure peers have progressed sufficiently. -/// -/// # Given -/// - last_purge_index = None (first purge) -/// -/// # When -/// - Attempt first purge -/// -/// # Then -/// - First purge allowed (None -> 99) -/// - Must still respect commit_index gap -#[test] -fn test_can_purge_logs_initial_purge() { - let node_config = Arc::new(node_config("/tmp/test_can_purge_logs_initial_purge")); - let mut state = LeaderState::::new(1, node_config); - - state.shared_state.commit_index = 100; - - // First purge (last_purge_index = None) - assert!(state.can_purge_logs(None, LogId { index: 99, term: 1 })); - - // Must still respect commit_index gap - assert!(!state.can_purge_logs( - None, - LogId { - index: 100, - term: 1 - } // 100 not < 100 - )); -} - // ============================================================================ // Leader Discovery Tests // ============================================================================ @@ -484,3 +304,23 @@ async fn test_handle_discover_leader_invalid_node_id() { let response = resp_rx.recv().await.unwrap().unwrap(); assert_eq!(response.leader_id, 1); } + +// ============================================================================ +// Role Transition Tests β€” scheduled_purge_upto / last_purged_index +// ============================================================================ + +/// From<&CandidateState>: last_purged_index preserved, scheduled_purge_upto always None. +/// +/// Leaders start each term with a clean purge schedule to avoid applying a stale purge +/// boundary computed in a prior term under a different last_applied index. +#[test] +fn test_leader_from_candidate_preserves_last_purged_index_resets_scheduled() { + let cfg = Arc::new(node_config("/tmp/test_leader_from_candidate_purge")); + let mut candidate = CandidateState::::new(1, cfg); + candidate.last_purged_index = Some(LogId { term: 2, index: 8 }); + + let leader = LeaderState::from(&candidate); + + assert_eq!(leader.last_purged_index, Some(LogId { term: 2, index: 8 })); + assert_eq!(leader.scheduled_purge_upto, None); +} diff --git a/d-engine-core/src/raft_role/learner_state.rs b/d-engine-core/src/raft_role/learner_state.rs index 39200590..ec2e8cd6 100644 --- a/d-engine-core/src/raft_role/learner_state.rs +++ b/d-engine-core/src/raft_role/learner_state.rs @@ -11,7 +11,6 @@ use crate::InternalEvent; use crate::Membership; use crate::MembershipError; use crate::NetworkError; -use crate::PurgeExecutor; use crate::RaftContext; use crate::RaftLog; use crate::RaftNodeConfig; @@ -21,6 +20,7 @@ use crate::StateTransitionError; use crate::Transport; use crate::TypeConfig; use crate::alias::MOF; +use crate::role_state::schedule_and_execute_purge; use async_trait::async_trait; use d_engine_proto::common::LogId; use d_engine_proto::common::NodeRole; @@ -64,6 +64,17 @@ pub struct LearnerState { /// Shared cluster state with concurrency control pub shared_state: SharedState, + // -- Log Compaction & Purge -- + /// === Volatile State === + /// The upper bound (exclusive) of log entries scheduled for asynchronous physical deletion. + /// + /// This value is set immediately after a new snapshot is successfully created. + /// It represents the next log position that will trigger compaction. + /// + /// The actual log purge is performed by a background task, which may be delayed + /// due to resource constraints or retry mechanisms. + pub scheduled_purge_upto: Option, + /// === Persistent State (MUST be on disk) === /// The last log position that has been **physically removed** from stable storage. /// @@ -525,72 +536,32 @@ impl RaftRoleState for LearnerState { ) } - /// Trigger an independent snapshot on this role (Raft Β§7 β€” each server snapshots - /// independently). Called when `should_snapshot()` returns true after SM apply. - /// Default: no-op. Candidate overrides to return `RoleViolation`. - async fn handle_create_snapshot( - &mut self, - ctx: &RaftContext, - internal_event_tx: &mpsc::UnboundedSender, - ) -> Result<()> { - // Prevent duplicate snapshot creation - if self.snapshot_in_progress.load(Ordering::Acquire) { - info!("Snapshot creation already in progress. Skipping duplicate request."); - return Ok(()); - } - - self.snapshot_in_progress.store(true, Ordering::Release); - let state_machine_handler = ctx.state_machine_handler().clone(); - - // Use spawn to perform snapshot creation in the background - let internal_event_tx = internal_event_tx.clone(); - tokio::spawn(async move { - let result = state_machine_handler.create_snapshot().await; - info!("SnapshotCreated event will be processed in another event thread"); - if let Err(e) = internal_event_tx.send(InternalEvent::SnapshotCreated(result)) { - error!("Learner failed to send snapshot creation result: {}", e); - } - }); - - Ok(()) + fn snapshot_in_progress(&self) -> Option<&AtomicBool> { + Some(&self.snapshot_in_progress) } - /// Process the completed snapshot result (success or error). - /// Leader: schedules log purge up to `last_included`. - /// Follower/Learner: updates local snapshot path. - /// Default: no-op. Candidate overrides to return `RoleViolation`. async fn handle_snapshot_created( &mut self, result: crate::Result<(SnapshotMetadata, std::path::PathBuf)>, ctx: &RaftContext, - _internal_event_tx: &mpsc::UnboundedSender, + internal_event_tx: &mpsc::UnboundedSender, ) -> Result<()> { - // Reset snapshot_in_progress flag self.snapshot_in_progress.store(false, Ordering::SeqCst); - - // Per Raft Β§7: Learner independently purges logs after snapshot generation match result { - Ok((metadata, _path)) => { + Err(e) => error!(?e, "Learner snapshot creation failed"), + Ok((metadata, _)) => { if let Some(last_included) = metadata.last_included { - info!(?last_included, "Learner snapshot created, purging logs"); - - // Learner independently decides to purge after snapshot - if self.can_purge_logs(self.last_purged_index, last_included) { - match ctx.purge_executor().execute_purge(last_included).await { - Ok(_) => { - self.last_purged_index = Some(last_included); - info!(?last_included, "Learner logs purged successfully"); - } - Err(e) => { - error!(?e, "Failed to purge logs after snapshot"); - } - } - } + schedule_and_execute_purge( + last_included, + ctx, + self.commit_index(), + self.last_purged_index, + &mut self.scheduled_purge_upto, + internal_event_tx, + ) + .await?; } } - Err(e) => { - error!(?e, "Learner snapshot creation failed"); - } } Ok(()) } @@ -642,11 +613,16 @@ impl RaftRoleState for LearnerState { // Stale in-flight events β€” these are Leader-only operations that may arrive // after a step-down due to internal_event_tx (unbounded, P2) race with BecomeFollower. // Both orderings are valid; Learner silently ignores them rather than erroring. - fn handle_log_purge_completed( &mut self, - _purged_id: d_engine_proto::common::LogId, + purged_id: d_engine_proto::common::LogId, ) -> Result<()> { + if self.last_purged_index.is_none_or(|cur| purged_id.index > cur.index) { + self.last_purged_index = Some(purged_id); + + // purge completed, clear to prevent re-execution + self.scheduled_purge_upto = None; + } Ok(()) } @@ -682,30 +658,10 @@ impl LearnerState { snapshot_in_progress: AtomicBool::new(false), node_config, _marker: PhantomData, + scheduled_purge_upto: None, } } - /// Determines if logs can be safely purged up to the given index - /// - /// Per Raft Β§7: Learner independently purges logs after snapshot generation - /// - /// # Safety Checks - /// 1. Committed Entry Guarantee: last_included < commit_index - /// 2. Monotonic Purge: last_purged_index < last_included - pub fn can_purge_logs( - &self, - last_purge_index: Option, - last_included_in_request: LogId, - ) -> bool { - let commit_check = last_included_in_request.index < self.commit_index(); - - let monotonic_check = last_purge_index - .map(|lid| lid.index < last_included_in_request.index) - .unwrap_or(true); - - commit_check && monotonic_check - } - pub async fn broadcast_discovery( &self, membership: Arc>, @@ -809,7 +765,8 @@ impl From<&FollowerState> for LearnerState { 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, + scheduled_purge_upto: follower_state.scheduled_purge_upto, _marker: PhantomData, } } @@ -821,6 +778,7 @@ impl From<&CandidateState> for LearnerState { node_config: candidate_state.node_config.clone(), snapshot_in_progress: AtomicBool::new(false), last_purged_index: candidate_state.last_purged_index, + scheduled_purge_upto: None, _marker: PhantomData, } } diff --git a/d-engine-core/src/raft_role/learner_state_test.rs b/d-engine-core/src/raft_role/learner_state_test.rs index 4ca886ce..552dccb0 100644 --- a/d-engine-core/src/raft_role/learner_state_test.rs +++ b/d-engine-core/src/raft_role/learner_state_test.rs @@ -12,14 +12,18 @@ use crate::client::ClientReadRequest; use crate::client::ClientResponsePayload; use crate::client::ClientWriteRequest; use crate::client::KvEntry; -use crate::client::WriteOperation; use crate::config::ReadConsistencyPolicy; +use crate::raft_role::candidate_state::CandidateState; +use crate::raft_role::follower_state::FollowerState; use crate::raft_role::learner_state::LearnerState; use crate::raft_role::role_state::RaftRoleState; use crate::test_utils::mock::MockTypeConfig; use crate::test_utils::mock::mock_raft_context; use crate::test_utils::mock::mock_raft_context_with_temp; +use crate::test_utils::mock::mock_raft_log; use crate::test_utils::node_config; +use bytes::Bytes; +use d_engine_proto::client::WriteCommand; use d_engine_proto::common::LogId; use d_engine_proto::common::NodeRole; use d_engine_proto::common::NodeStatus; @@ -33,6 +37,7 @@ use d_engine_proto::server::storage::SnapshotAck; use d_engine_proto::server::storage::SnapshotChunk; use d_engine_proto::server::storage::snapshot_ack::ChunkStatus; use mockall::predicate::eq; +use prost::Message; use std::sync::Arc; use tokio::sync::{mpsc, watch}; use tonic::Code; @@ -501,9 +506,9 @@ async fn test_learner_rejects_client_write_request() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: 1, - command: Some(WriteOperation::Delete { - key: bytes::Bytes::new(), - }), + command: Some(Bytes::from( + WriteCommand::delete(Bytes::new()).encode_to_vec(), + )), }, resp_tx, ); @@ -1236,10 +1241,15 @@ mod snapshot_tests { let mut state = LearnerState::::new(1, context.node_config.clone()); let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + state.scheduled_purge_upto = Some(LogId { term: 1, index: 1 }); assert!( state.handle_log_purge_completed(LogId { term: 1, index: 1 }).is_ok(), "Stale LogPurgeCompleted should be silently ignored" ); + assert!( + state.scheduled_purge_upto.is_none(), + "handle_log_purge_completed must clear scheduled_purge_upto" + ); assert!( state.handle_promote_ready_learners(&context, &internal_event_tx).await.is_ok(), "Stale PromoteReadyLearners should be silently ignored" @@ -1299,12 +1309,23 @@ mod snapshot_tests { #[tokio::test] async fn test_learner_resets_snapshot_flag_on_success() { let (_graceful_tx, graceful_rx) = watch::channel(()); - let (context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + + // Default retained_log_entries = 1, last_included.index = 50 β†’ purge_upto_index = 49 + let mut raft_log = mock_raft_log(); + raft_log + .expect_entry_term() + .withf(|&idx| idx == 49) + .times(1) + .returning(|_| Some(1)); + let _temp_dir = tempfile::tempdir().unwrap(); + let nc = node_config(_temp_dir.path().to_str().unwrap()); + let context = MockBuilder::new(graceful_rx) + .with_raft_log(raft_log) + .with_node_config(nc) + .build_context(); let mut state = LearnerState::::new(1, context.node_config.clone()); state.snapshot_in_progress.store(true, Ordering::SeqCst); - // Prerequisite: commit_index must exceed last_included for can_purge_logs to allow purge. - // In real operation, entries are committed before they can be snapshotted. state.update_commit_index(100).unwrap(); let last_included = LogId { term: 1, index: 50 }; @@ -1314,7 +1335,7 @@ mod snapshot_tests { }; let snapshot_result = Ok((metadata, std::path::PathBuf::from("/tmp/test_snapshot.bin"))); - let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); let result = state .handle_snapshot_created(snapshot_result, &context, &internal_event_tx) .await; @@ -1324,10 +1345,17 @@ mod snapshot_tests { !state.snapshot_in_progress.load(Ordering::SeqCst), "snapshot_in_progress should be false after SnapshotCreated" ); - assert_eq!( - state.last_purged_index, - Some(last_included), - "last_purged_index must advance to last_included after log purge" + // last_purged_index is updated via LogPurgeCompleted event (processed by event loop). + // Verify the event was dispatched with purge_upto = last_included - retained_log_entries. + let event = internal_event_rx + .try_recv() + .expect("LogPurgeCompleted event must be dispatched after successful purge"); + assert!( + matches!( + event, + InternalEvent::LogPurgeCompleted(LogId { index: 49, .. }) + ), + "purge boundary must be last_included.index(50) - retained(1) = 49, got: {event:?}" ); } @@ -2003,3 +2031,49 @@ async fn test_learner_rejects_stream_snapshot() { "Learner must reply FailedPrecondition for StreamSnapshot" ); } + +// ============================================================================ +// Role Transition Tests β€” scheduled_purge_upto / last_purged_index +// ============================================================================ + +/// From<&FollowerState>: both fields preserved across follower-to-learner demotion. +/// +/// A follower with a pending purge intent must hand it off so the learner can resume +/// without recomputing the purge boundary. +#[test] +fn test_learner_from_follower_preserves_both_purge_fields() { + let cfg = Arc::new(node_config("/tmp/test_learner_from_follower_purge")); + let mut follower = FollowerState::::new(1, cfg, None, None); + follower.last_purged_index = Some(LogId { term: 1, index: 12 }); + follower.scheduled_purge_upto = Some(LogId { term: 1, index: 10 }); + + let learner = LearnerState::from(&follower); + + assert_eq!( + learner.last_purged_index, + Some(LogId { term: 1, index: 12 }) + ); + assert_eq!( + learner.scheduled_purge_upto, + Some(LogId { term: 1, index: 10 }) + ); +} + +/// From<&CandidateState>: last_purged_index preserved, scheduled_purge_upto reset. +/// +/// Candidate has no scheduled_purge_upto field; the resulting learner always starts +/// with None so it does not replay a stale purge boundary from a prior election. +#[test] +fn test_learner_from_candidate_preserves_last_purged_index_resets_scheduled() { + let cfg = Arc::new(node_config("/tmp/test_learner_from_candidate_purge")); + let mut candidate = CandidateState::::new(1, cfg); + candidate.last_purged_index = Some(LogId { term: 3, index: 15 }); + + let learner = LearnerState::from(&candidate); + + assert_eq!( + learner.last_purged_index, + Some(LogId { term: 3, index: 15 }) + ); + assert_eq!(learner.scheduled_purge_upto, None); +} diff --git a/d-engine-core/src/raft_role/mod.rs b/d-engine-core/src/raft_role/mod.rs index cc8adb32..b078e4f0 100644 --- a/d-engine-core/src/raft_role/mod.rs +++ b/d-engine-core/src/raft_role/mod.rs @@ -17,6 +17,8 @@ mod follower_state_test; mod leader_state_test; #[cfg(test)] mod learner_state_test; +#[cfg(test)] +mod role_state_test; use std::collections::HashMap; use std::sync::Arc; diff --git a/d-engine-core/src/raft_role/role_state.rs b/d-engine-core/src/raft_role/role_state.rs index 0d156acb..9cdf3fe7 100644 --- a/d-engine-core/src/raft_role/role_state.rs +++ b/d-engine-core/src/raft_role/role_state.rs @@ -1,11 +1,12 @@ -use crate::ConsensusError; use crate::client::{ClientReadRequest, ClientResponse, KvEntry, LeaderHint}; +use crate::{ConsensusError, PurgeExecutor}; use async_trait::async_trait; use d_engine_proto::common::LogId; use d_engine_proto::server::election::VotedFor; use d_engine_proto::server::replication::AppendEntriesRequest; use d_engine_proto::server::replication::AppendEntriesResponse; use d_engine_proto::server::storage::SnapshotMetadata; +use std::sync::atomic::{AtomicBool, Ordering}; use tokio::sync::mpsc; use tokio::time::Instant; use tonic::Status; @@ -590,18 +591,51 @@ pub trait RaftRoleState: Send + Sync + 'static { /// Trigger an independent snapshot on this role (Raft Β§7 β€” each server snapshots /// independently). Called when `should_snapshot()` returns true after SM apply. - /// Default: no-op. Candidate overrides to return `RoleViolation`. + /// Candidate short-circuits via `snapshot_in_progress()` returning `None`. async fn handle_create_snapshot( &mut self, - _ctx: &RaftContext, - _internal_event_tx: &mpsc::UnboundedSender, + ctx: &RaftContext, + internal_event_tx: &mpsc::UnboundedSender, ) -> Result<()> { - Err(ConsensusError::RoleViolation { - current_role: "Candidate", - required_role: "Follower/Leader/Learner", - context: ("Candidate node attempted to create snapshot.").to_string(), + let Some(flag) = self.snapshot_in_progress() else { + return Err(ConsensusError::RoleViolation { + current_role: "Candidate", + required_role: "Follower/Leader/Learner", + context: "Candidate node attempted to create snapshot.".to_string(), + } + .into()); + }; + + if flag.load(Ordering::Acquire) { + info!("Snapshot creation already in progress. Skipping duplicate request."); + return Ok(()); } - .into()) + + flag.store(true, Ordering::Release); + let state_machine_handler = ctx.state_machine_handler().clone(); + + // Use spawn to perform snapshot creation in the background + let internal_event_tx = internal_event_tx.clone(); + tokio::spawn(async move { + let result = state_machine_handler.create_snapshot().await; + info!("SnapshotCreated event will be processed in another event thread"); + if let Err(e) = internal_event_tx.send(InternalEvent::SnapshotCreated(result)) { + error!("Failed to send snapshot creation result: {e:?}"); + } + }); + + Ok(()) + } + + /// Returns the snapshot deduplication flag for roles that support independent snapshot creation. + /// + /// Returns `None` for roles without snapshot capability (Candidate). The default + /// `handle_create_snapshot` uses this to short-circuit with `RoleViolation` instead of + /// silently operating on a dummy value. + /// + /// Leader, Follower, and Learner override this to return `Some(&self.snapshot_in_progress)`. + fn snapshot_in_progress(&self) -> Option<&AtomicBool> { + None } /// Process the completed snapshot result (success or error). @@ -617,7 +651,7 @@ pub trait RaftRoleState: Send + Sync + 'static { Err(ConsensusError::RoleViolation { current_role: "Candidate", required_role: "Follower/Leader/Learner", - context: ("Candidate role node attempted to handle created snapshot.").to_string(), + context: "Candidate node attempted to handle created snapshot.".to_string(), } .into()) } @@ -694,6 +728,62 @@ pub trait RaftRoleState: Send + Sync + 'static { } } +pub(super) async fn schedule_and_execute_purge( + last_included: LogId, + ctx: &RaftContext, + commit_index: u64, + last_purged_index: Option, + scheduled_purge_upto: &mut Option, + internal_event_tx: &mpsc::UnboundedSender, +) -> Result<()> { + // ---------------------- + // Phase 1: Schedule log purge if possible + // ---------------------- + // Log retention is intentionally decoupled from the snapshot boundary. + // last_included == last_applied (snapshot is always truthful). + // purge_upto is set back by retained_log_entries so lagging followers + // can still catch up via AppendEntries instead of InstallSnapshot. + let retained = ctx.node_config().raft.snapshot.retained_log_entries; + let purge_upto_index = last_included.index.saturating_sub(retained); + println!("purge_upto_index={purge_upto_index}"); + // retained >= last_included.index β†’ nothing to purge; skip log lookup entirely. + if purge_upto_index == 0 { + return Ok(()); + } + if let Some(term) = ctx.raft_log().entry_term(purge_upto_index) { + let purge_upto = LogId { + index: purge_upto_index, + term, + }; + let idx = purge_upto.index; + let monotonic = last_purged_index.map(|l| l.index < idx).unwrap_or(true); + if idx > 0 + && idx < commit_index + && monotonic + && scheduled_purge_upto.map(|e| e.index < idx).unwrap_or(true) + { + *scheduled_purge_upto = Some(purge_upto); + } + } + + // ---------------------- + // Phase 2: Execute local purge + // ---------------------- + // Per Raft Β§7: Leader purges independently without peer coordination + if let Some(scheduled) = *scheduled_purge_upto { + match ctx.purge_executor().execute_purge(scheduled).await { + Ok(_) => { + if let Err(e) = internal_event_tx.send(InternalEvent::LogPurgeCompleted(scheduled)) + { + error!(%e, "Failed to notify purge completion"); + } + } + Err(e) => error!(?e, ?scheduled, "Log purge execution failed"), + } + } + Ok(()) +} + /// Send a InboundEvent back into the internal event loop for reprocessing. pub(super) fn send_replay_inbound_event( internal_event_tx: &mpsc::UnboundedSender, diff --git a/d-engine-core/src/raft_role/role_state_test.rs b/d-engine-core/src/raft_role/role_state_test.rs new file mode 100644 index 00000000..00dae432 --- /dev/null +++ b/d-engine-core/src/raft_role/role_state_test.rs @@ -0,0 +1,342 @@ +//! Tests for role_state free functions +//! +//! Covers: +//! - schedule_and_execute_purge: two-phase log purge orchestration (Phase 1 = schedule, Phase 2 = execute) + +use tokio::sync::{mpsc, watch}; + +use d_engine_proto::common::LogId; + +use crate::Error; +use crate::InternalEvent; +use crate::MockPurgeExecutor; +use crate::test_utils::mock::{MockBuilder, mock_raft_log}; +use crate::test_utils::node_config; + +use super::role_state::schedule_and_execute_purge; + +// ============================================================================ +// schedule_and_execute_purge Tests +// ============================================================================ + +/// Happy path: snapshot at index 50, retained=1 β†’ purge_upto=49. +/// Both phases execute: Phase 1 schedules, Phase 2 purges and dispatches LogPurgeCompleted. +#[tokio::test] +async fn test_schedule_and_execute_purge_happy_path() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + + let mut raft_log = mock_raft_log(); + raft_log + .expect_entry_term() + .withf(|&idx| idx == 49) + .times(1) + .returning(|_| Some(1)); + + let ctx = MockBuilder::new(graceful_rx).with_raft_log(raft_log).build_context(); + + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + let last_included = LogId { index: 50, term: 1 }; + let mut scheduled_purge_upto: Option = None; + + let result = schedule_and_execute_purge( + last_included, + &ctx, + 100, + None, + &mut scheduled_purge_upto, + &internal_event_tx, + ) + .await; + + assert!(result.is_ok()); + assert_eq!( + scheduled_purge_upto, + Some(LogId { index: 49, term: 1 }), + "Phase 1 must schedule purge_upto = last_included.index - retained" + ); + let event = internal_event_rx + .try_recv() + .expect("LogPurgeCompleted must be dispatched after successful purge"); + assert!( + matches!( + event, + InternalEvent::LogPurgeCompleted(LogId { index: 49, .. }) + ), + "expected LogPurgeCompleted(49), got: {event:?}" + ); +} + +/// Zero boundary: retained_log_entries >= last_included.index β†’ saturating_sub = 0. +/// Guard `idx > 0` rejects the purge β€” no schedule, no execute, no event. +#[tokio::test] +async fn test_schedule_and_execute_purge_zero_boundary_skips_purge() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + + // entry_term(0) called but returns None β€” index 0 does not exist + let mut raft_log = mock_raft_log(); + raft_log.expect_entry_term().withf(|&idx| idx == 0).times(0).returning(|_| None); + + let _temp_dir = tempfile::tempdir().unwrap(); + let mut nc = node_config(_temp_dir.path().to_str().unwrap()); + nc.raft.snapshot.retained_log_entries = 50; // >= last_included.index(50) β†’ purge_upto=0 + + let ctx = MockBuilder::new(graceful_rx) + .with_raft_log(raft_log) + .with_node_config(nc) + .build_context(); + + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + let last_included = LogId { index: 50, term: 1 }; + let mut scheduled_purge_upto: Option = None; + + let result = schedule_and_execute_purge( + last_included, + &ctx, + 100, + None, + &mut scheduled_purge_upto, + &internal_event_tx, + ) + .await; + + assert!(result.is_ok()); + assert!( + scheduled_purge_upto.is_none(), + "zero purge_upto must not be scheduled" + ); + assert!( + internal_event_rx.try_recv().is_err(), + "no LogPurgeCompleted must be sent when purge is skipped" + ); +} + +/// commit_index gap rule: purge_upto(99) >= commit_index(95) β†’ guard rejects. +/// No schedule update, no event. +#[tokio::test] +async fn test_schedule_and_execute_purge_rejects_when_purge_upto_exceeds_commit_index() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + + // retained=1, last_included.index=100 β†’ purge_upto_index=99 + let mut raft_log = mock_raft_log(); + raft_log + .expect_entry_term() + .withf(|&idx| idx == 99) + .times(1) + .returning(|_| Some(1)); + + let ctx = MockBuilder::new(graceful_rx).with_raft_log(raft_log).build_context(); + + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + let last_included = LogId { + index: 100, + term: 1, + }; + let mut scheduled_purge_upto: Option = None; + + let result = schedule_and_execute_purge( + last_included, + &ctx, + 95, // commit_index = 95 < purge_upto_index(99) + None, + &mut scheduled_purge_upto, + &internal_event_tx, + ) + .await; + + assert!(result.is_ok()); + assert!( + scheduled_purge_upto.is_none(), + "purge_upto >= commit_index must be rejected" + ); + assert!( + internal_event_rx.try_recv().is_err(), + "no event when commit_index gap rule rejects" + ); +} + +/// Monotonicity: last_purged_index(60) >= purge_upto(49) β†’ backward purge rejected. +/// scheduled_purge_upto stays None, no event. +#[tokio::test] +async fn test_schedule_and_execute_purge_monotonicity_rejects_backward_purge() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + + let mut raft_log = mock_raft_log(); + raft_log + .expect_entry_term() + .withf(|&idx| idx == 49) + .times(1) + .returning(|_| Some(1)); + + let ctx = MockBuilder::new(graceful_rx).with_raft_log(raft_log).build_context(); + + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + let last_included = LogId { index: 50, term: 1 }; + let mut scheduled_purge_upto: Option = None; + + let result = schedule_and_execute_purge( + last_included, + &ctx, + 100, + Some(LogId { index: 60, term: 1 }), // already purged beyond purge_upto(49) + &mut scheduled_purge_upto, + &internal_event_tx, + ) + .await; + + assert!(result.is_ok()); + assert!( + scheduled_purge_upto.is_none(), + "backward purge must not update scheduled_purge_upto" + ); + assert!( + internal_event_rx.try_recv().is_err(), + "no event when monotonicity guard rejects" + ); +} + +/// Fault recovery: entry_term returns None (log compacted) so Phase 1 is skipped. +/// Pre-existing scheduled_purge_upto is retried in Phase 2 β†’ LogPurgeCompleted dispatched. +#[tokio::test] +async fn test_schedule_and_execute_purge_fault_recovery_retries_existing_scheduled() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + + // entry_term returns None β†’ Phase 1 skipped + let mut raft_log = mock_raft_log(); + raft_log + .expect_entry_term() + .withf(|&idx| idx == 49) + .times(1) + .returning(|_| None); + + let ctx = MockBuilder::new(graceful_rx).with_raft_log(raft_log).build_context(); + + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + let last_included = LogId { index: 50, term: 1 }; + // Pre-existing scheduled from a prior Phase 1 that never executed + let mut scheduled_purge_upto: Option = Some(LogId { index: 45, term: 1 }); + + let result = schedule_and_execute_purge( + last_included, + &ctx, + 100, + None, + &mut scheduled_purge_upto, + &internal_event_tx, + ) + .await; + + assert!(result.is_ok()); + assert_eq!( + scheduled_purge_upto, + Some(LogId { index: 45, term: 1 }), + "scheduled_purge_upto must not change when Phase 1 is skipped" + ); + let event = internal_event_rx + .try_recv() + .expect("Phase 2 must retry existing scheduled_purge_upto"); + assert!( + matches!( + event, + InternalEvent::LogPurgeCompleted(LogId { index: 45, .. }) + ), + "expected LogPurgeCompleted(45), got: {event:?}" + ); +} + +/// Schedule monotonicity: new purge_upto(49) < existing scheduled(60) β†’ no regression. +/// Phase 2 still executes with the existing scheduled(60). +#[tokio::test] +async fn test_schedule_and_execute_purge_does_not_regress_scheduled_purge_upto() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + + let mut raft_log = mock_raft_log(); + raft_log + .expect_entry_term() + .withf(|&idx| idx == 49) + .times(1) + .returning(|_| Some(1)); + + let ctx = MockBuilder::new(graceful_rx).with_raft_log(raft_log).build_context(); + + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + let last_included = LogId { index: 50, term: 1 }; + // scheduled already ahead of new purge_upto(49) + let mut scheduled_purge_upto: Option = Some(LogId { index: 60, term: 1 }); + + let result = schedule_and_execute_purge( + last_included, + &ctx, + 100, + None, + &mut scheduled_purge_upto, + &internal_event_tx, + ) + .await; + + assert!(result.is_ok()); + assert_eq!( + scheduled_purge_upto, + Some(LogId { index: 60, term: 1 }), + "scheduled_purge_upto must not regress from 60 to 49" + ); + let event = internal_event_rx + .try_recv() + .expect("Phase 2 must execute existing scheduled(60)"); + assert!( + matches!( + event, + InternalEvent::LogPurgeCompleted(LogId { index: 60, .. }) + ), + "expected LogPurgeCompleted(60), got: {event:?}" + ); +} + +/// Phase 2 failure: execute_purge returns Err. +/// Function returns Ok(()), LogPurgeCompleted is NOT dispatched. +#[tokio::test] +async fn test_schedule_and_execute_purge_execute_failure_suppresses_completion_event() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + + // Phase 1 skipped β€” entry_term returns None + let mut raft_log = mock_raft_log(); + raft_log + .expect_entry_term() + .withf(|&idx| idx == 49) + .times(1) + .returning(|_| None); + + let mut purge_executor = MockPurgeExecutor::new(); + purge_executor + .expect_execute_purge() + .times(1) + .returning(|_| Err(Error::Fatal("disk error".to_string()))); + + let mut builder = MockBuilder::new(graceful_rx); + builder.purge_executor = Some(purge_executor); + let ctx = builder.with_raft_log(raft_log).build_context(); + + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + let last_included = LogId { index: 50, term: 1 }; + let mut scheduled_purge_upto: Option = Some(LogId { index: 49, term: 1 }); + + let result = schedule_and_execute_purge( + last_included, + &ctx, + 100, + None, + &mut scheduled_purge_upto, + &internal_event_tx, + ) + .await; + + assert!( + result.is_ok(), + "errors from execute_purge must not propagate" + ); + assert!( + internal_event_rx.try_recv().is_err(), + "LogPurgeCompleted must not be sent when execute_purge fails" + ); + // scheduled_purge_upto preserved β€” fault recovery will retry on next snapshot + assert_eq!(scheduled_purge_upto, Some(LogId { index: 49, term: 1 })); +} diff --git a/d-engine-core/src/replication/replication_handler.rs b/d-engine-core/src/replication/replication_handler.rs index 5ad71ad8..0a9590a5 100644 --- a/d-engine-core/src/replication/replication_handler.rs +++ b/d-engine-core/src/replication/replication_handler.rs @@ -337,7 +337,6 @@ where None } - #[tracing::instrument(skip(self, raft_log))] fn check_append_entries_request_is_legal( &self, my_term: u64, diff --git a/d-engine-core/src/state_machine_handler/default_state_machine_handler.rs b/d-engine-core/src/state_machine_handler/default_state_machine_handler.rs index e69192a3..428c65ce 100644 --- a/d-engine-core/src/state_machine_handler/default_state_machine_handler.rs +++ b/d-engine-core/src/state_machine_handler/default_state_machine_handler.rs @@ -1,11 +1,5 @@ -use std::ops::RangeInclusive; -use std::path::Path; -use std::path::PathBuf; -use std::sync::Arc; -use std::sync::atomic::AtomicBool; -use std::sync::atomic::AtomicU64; -use std::sync::atomic::Ordering; -use std::time::Duration; +#[cfg(feature = "watch")] +use crate::BatchOp; use crate::client::KvEntry; use async_compression::tokio::bufread::GzipDecoder; @@ -22,6 +16,14 @@ use d_engine_proto::server::storage::snapshot_ack::ChunkStatus; use futures::stream::BoxStream; use memmap2::Mmap; use memmap2::MmapOptions; +use std::ops::RangeInclusive; +use std::path::Path; +use std::path::PathBuf; +use std::sync::Arc; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::AtomicU64; +use std::sync::atomic::Ordering; +use std::time::Duration; use tempfile::tempdir; use tokio::fs; use tokio::fs::File; @@ -38,7 +40,6 @@ use tokio_tar::Archive; use tracing::debug; use tracing::error; use tracing::info; -use tracing::instrument; use tracing::trace; use super::SnapshotAssembler; @@ -320,7 +321,6 @@ where } } - #[instrument(skip(self))] async fn apply_snapshot_stream_from_leader( &self, current_term: u64, @@ -393,22 +393,7 @@ where // 2: Prepare temp snapshot file and final snapshot file debug!("create_snapshot 2: Prepare temp snapshot file and final snapshot file"); - let raw_last_included = self.state_machine.last_applied(); - - // Apply retention policy - let last_included = LogId { - index: raw_last_included - .index - .saturating_sub(self.snapshot_config.retained_log_entries), - term: self - .state_machine - .entry_term( - raw_last_included - .index - .saturating_sub(self.snapshot_config.retained_log_entries), - ) - .unwrap_or(raw_last_included.term), - }; + let last_included = self.state_machine.last_applied(); let temp_path = self.path_mgr.temp_work_path(&last_included); @@ -457,7 +442,6 @@ where )) } - #[tracing::instrument] async fn cleanup_snapshot( &self, retain_count: u64, @@ -540,7 +524,6 @@ where } /// Load snapshot data as a stream of chunks (ZERO-COPY) - #[instrument(skip(self))] async fn load_snapshot_data( &self, metadata: SnapshotMetadata, @@ -690,6 +673,7 @@ where Some(prev.unwrap_or_default()) } Command::Noop => None, + Command::Batch { ops: _ } => None, }; result.push(prev); } @@ -720,43 +704,66 @@ where let prev_value = prev_values.and_then(|pv| pv.get(i)).and_then(|v| v.clone()).unwrap_or_default(); - let event = match &entry.command { - Command::Insert { key, value, .. } => Some(WatchResponse { + let events: Vec = match &entry.command { + Command::Insert { key, value, .. } => vec![WatchResponse { key: key.clone(), value: value.clone(), prev_value, event_type: WatchEventType::Put as i32, error: 0, revision: entry.index, - }), - Command::Delete { key } => Some(WatchResponse { + }], + Command::Delete { key } => vec![WatchResponse { key: key.clone(), value: bytes::Bytes::new(), prev_value, event_type: WatchEventType::Delete as i32, error: 0, revision: entry.index, - }), + }], Command::CompareAndSwap { key, value, .. } => { // Only broadcast if CAS actually mutated the value. // A failed CAS leaves the key unchanged β€” no watch event. if results.get(i).is_some_and(|r| r.succeeded) { - Some(WatchResponse { + vec![WatchResponse { key: key.clone(), value: value.clone(), prev_value, event_type: WatchEventType::Put as i32, error: 0, revision: entry.index, - }) + }] } else { - None + vec![] } } - Command::Noop => None, + Command::Noop => vec![], + // Batch: one event per op. prev_value is not supported per-op yet + // (read_prev_values returns one value per ApplyEntry, not per BatchOp). + Command::Batch { ops } => ops + .iter() + .map(|op| match op { + BatchOp::Insert { key, value } => WatchResponse { + key: key.clone(), + value: value.clone(), + prev_value: bytes::Bytes::new(), + event_type: WatchEventType::Put as i32, + error: 0, + revision: entry.index, + }, + BatchOp::Delete { key } => WatchResponse { + key: key.clone(), + value: bytes::Bytes::new(), + prev_value: bytes::Bytes::new(), + event_type: WatchEventType::Delete as i32, + error: 0, + revision: entry.index, + }, + }) + .collect(), }; - if let Some(ev) = event { + for ev in events { // Fire-and-forget: ignore send errors (no receivers or lagging) let _ = tx.send(ev); } @@ -828,7 +835,6 @@ where } #[allow(dead_code)] - #[instrument(skip(self))] async fn create_snapshot_chunk_stream( &self, snapshot_file: PathBuf, @@ -897,7 +903,6 @@ where } /// Helper function to process snapshot stream - #[instrument(skip(self, stream_chunk_receiver))] async fn process_snapshot_stream( &self, mut stream_chunk_receiver: mpsc::Receiver, diff --git a/d-engine-core/src/state_machine_handler/default_state_machine_handler_test.rs b/d-engine-core/src/state_machine_handler/default_state_machine_handler_test.rs index 93c879d4..c0550838 100644 --- a/d-engine-core/src/state_machine_handler/default_state_machine_handler_test.rs +++ b/d-engine-core/src/state_machine_handler/default_state_machine_handler_test.rs @@ -1,19 +1,39 @@ -use std::collections::HashSet; -use std::fs; -use std::path::Path; -use std::path::PathBuf; -use std::sync::Arc; -use std::time::Duration; - +use super::DefaultStateMachineHandler; +use super::StateMachineHandler; +use crate::ConsensusError; +use crate::Error; +use crate::MockSnapshotPolicy; +use crate::MockStateMachine; +use crate::MockTypeConfig; +use crate::SnapshotError; +use crate::StorageError; +use crate::test_utils::create_test_chunk; +use crate::test_utils::create_test_compressed_snapshot; +use crate::test_utils::snapshot_config; use bytes::Bytes; +use d_engine_proto::client::WriteCommand; +use d_engine_proto::client::write_command::batch_op::Op; +use d_engine_proto::client::write_command::{ + Batch, BatchOp as ProtoBatchOp, Insert, Operation, batch_op, +}; use d_engine_proto::common::Entry; +use d_engine_proto::common::EntryPayload; use d_engine_proto::common::LogId; +use d_engine_proto::common::entry_payload::Payload; use d_engine_proto::server::storage::SnapshotAck; use d_engine_proto::server::storage::SnapshotChunk; use d_engine_proto::server::storage::SnapshotMetadata; use d_engine_proto::server::storage::snapshot_ack::ChunkStatus; use futures::StreamExt; use mockall::Sequence; +use prost::Message; +use std::collections::HashSet; +use std::fs; +use std::path::Path; +use std::path::PathBuf; +use std::sync::Arc; +use std::sync::atomic::AtomicUsize; +use std::time::Duration; use tempfile::TempDir; use tempfile::tempdir; use tokio::fs::File; @@ -23,18 +43,32 @@ use tokio::sync::mpsc; use tracing::debug; use tracing_test::traced_test; -use super::DefaultStateMachineHandler; -use super::StateMachineHandler; -use crate::ConsensusError; -use crate::Error; -use crate::MockSnapshotPolicy; -use crate::MockStateMachine; -use crate::MockTypeConfig; -use crate::SnapshotError; -use crate::StorageError; -use crate::test_utils::create_test_chunk; -use crate::test_utils::create_test_compressed_snapshot; -use crate::test_utils::snapshot_config; +fn batch_entry( + index: u64, + key: &[u8], + value: &[u8], +) -> Entry { + let write_cmd = WriteCommand { + operation: Some(Operation::Batch(Batch { + ops: vec![ProtoBatchOp { + op: Some(Op::Insert(Insert { + key: Bytes::copy_from_slice(key), + value: Bytes::copy_from_slice(value), + ttl_secs: 0, + })), + }], + })), + }; + let mut buf = Vec::new(); + write_cmd.encode(&mut buf).unwrap(); + Entry { + index, + term: 1, + payload: Some(EntryPayload { + payload: Some(Payload::Command(Bytes::from(buf))), + }), + } +} // Case 1: normal update #[test] @@ -236,6 +270,11 @@ mod apply_chunk_test { assert_eq!(handler.last_applied(), 2); } + /// apply_chunk must NOT advance last_applied when the state machine returns an error. + /// + /// Raft at-most-once: an entry that fails to apply must not be acknowledged. + /// last_applied advancing past a failed entry would cause the leader to believe + /// it was committed into the SM, silently losing the write. #[tokio::test] async fn test_apply_chunk_with_state_machine_io_error() { let handler = create_test_handler( @@ -250,6 +289,66 @@ mod apply_chunk_test { assert!(result.is_err()); assert_eq!(handler.last_applied(), 0); } + + /// A Command::Batch that fails must not advance last_applied and must not + /// broadcast any watch events. + /// + /// Batch is an atomic operation: partial application is not allowed. + /// Watch subscribers must not see events for keys that were never mutated. + #[cfg(feature = "watch")] + #[tokio::test] + async fn test_apply_chunk_batch_failure_is_atomic_no_side_effects() { + // Build a proto-encoded Batch entry with one Insert op + let cmd = WriteCommand { + operation: Some(Operation::Batch(Batch { + ops: vec![ProtoBatchOp { + op: Some(batch_op::Op::Insert(Insert { + key: Bytes::from_static(b"k1"), + value: Bytes::from_static(b"v1"), + ttl_secs: 0, + })), + }], + })), + }; + let mut buf = Vec::new(); + cmd.encode(&mut buf).unwrap(); + let batch_entry = Entry { + index: 5, + term: 1, + payload: Some(EntryPayload { + payload: Some(Payload::Command(Bytes::from(buf))), + }), + }; + + // Handler with watch channel so we can observe broadcast behaviour + let (watch_tx, mut watch_rx) = tokio::sync::broadcast::channel(16); + let mut sm = MockStateMachine::new(); + sm.expect_apply_chunk() + .returning(|_| Err(Error::Fatal("batch failed".to_string()))); + + let handler = DefaultStateMachineHandler::::new( + 1, + 0, + Arc::new(sm), + snapshot_config(PathBuf::from("/tmp/test_batch_failure_atomic")), + MockSnapshotPolicy::new(), + Some(watch_tx), + Arc::new(AtomicUsize::new(0)), + ); + + let result = handler.apply_chunk(vec![batch_entry]).await; + + assert!(result.is_err()); + assert_eq!( + handler.last_applied(), + 0, + "last_applied must not advance on batch failure" + ); + assert!( + watch_rx.try_recv().is_err(), + "failed batch must not emit watch events" + ); + } } /// Verifies the decode boundary: raw proto `Entry` β†’ `ApplyEntry` transition happens inside @@ -268,7 +367,9 @@ mod decode_boundary_tests { use super::*; use crate::test_utils::snapshot_config; - use crate::{ApplyResult, Command, MockSnapshotPolicy, MockStateMachine, MockTypeConfig}; + use crate::{ + ApplyResult, BatchOp, Command, MockSnapshotPolicy, MockStateMachine, MockTypeConfig, + }; fn noop_entry(index: u64) -> Entry { Entry { @@ -417,6 +518,40 @@ mod decode_boundary_tests { assert!(result.is_ok()); assert_eq!(handler.last_applied(), 12); } + + /// Handler decodes a proto-encoded Batch entry and forwards Command::Batch + /// with correct ops to the state machine β€” no raw bytes reach the SM. + /// + /// Mirrors test_handler_decodes_insert_before_sm_receives_it for the Batch path. + #[tokio::test] + async fn test_handler_decodes_batch_before_sm_receives_it() { + let mut sm = MockStateMachine::new(); + sm.expect_apply_chunk() + .withf(|entries| { + entries.len() == 1 + && matches!( + &entries[0].command, + Command::Batch { ops } if ops.len() == 1 + && matches!(&ops[0], BatchOp::Insert { key, .. } if key == &Bytes::from_static(b"mykey")) + + ) + }) + .returning(|chunk| Ok(vec![ApplyResult::success(chunk[0].index)])); + + let handler = DefaultStateMachineHandler::::new_without_watch( + 1, + 0, + Arc::new(sm), + snapshot_config(std::path::PathBuf::from( + "/tmp/test_handler_decodes_batch_before_sm_receives_it", + )), + MockSnapshotPolicy::new(), + ); + + let result = handler.apply_chunk(vec![batch_entry(5, b"mykey", b"myval")]).await; + assert!(result.is_ok()); + assert_eq!(handler.last_applied(), 5); + } } // fn listen_addr(port: u32) -> SocketAddr { @@ -827,7 +962,6 @@ mod create_snapshot_tests { .times(1) .in_sequence(&mut seq) .returning(|| LogId { index: 5, term: 1 }); - sm.expect_entry_term().returning(|_| Some(1)); sm.expect_generate_snapshot_data() .times(1) .withf(|path, last_included| { @@ -895,7 +1029,6 @@ mod create_snapshot_tests { checksum: Bytes::from(vec![1; 8]), }) }); - sm.expect_entry_term().returning(|_| Some(1)); sm.expect_generate_snapshot_data().times(1..=2).returning(move |path, _| { // Track invocation count let mut count = counter_clone.lock().unwrap(); @@ -982,7 +1115,6 @@ mod create_snapshot_tests { index: count, } }); - sm.expect_entry_term().returning(|_| Some(1)); sm.expect_generate_snapshot_data().returning(|path, _| { debug!(?path, "expect_generate_snapshot_data"); std::fs::create_dir_all(path).expect("Failed to create directory"); @@ -1030,7 +1162,6 @@ mod create_snapshot_tests { // Setup failing snapshot generation sm.expect_last_applied().returning(|| LogId { term: 1, index: 1 }); - sm.expect_entry_term().returning(|_| Some(1)); sm.expect_generate_snapshot_data() .returning(|_, _| Err(SnapshotError::OperationFailed("test failure".into()).into())); @@ -1061,7 +1192,6 @@ mod create_snapshot_tests { let mut sm = MockStateMachine::new(); sm.expect_last_applied().returning(|| LogId { term: 1, index: 5 }); - sm.expect_entry_term().returning(|_| Some(1)); sm.expect_generate_snapshot_data().returning(|path, _| { fs::create_dir_all(path).unwrap(); Ok(Bytes::from(vec![0; 32])) @@ -1097,7 +1227,6 @@ mod create_snapshot_tests { let mut sm = MockStateMachine::new(); sm.expect_last_applied().returning(|| LogId { term: 1, index: 1 }); - sm.expect_entry_term().returning(|_| Some(1)); sm.expect_generate_snapshot_data() .returning(|_, _| Err(SnapshotError::OperationFailed("test error".into()).into())); @@ -1122,6 +1251,193 @@ mod create_snapshot_tests { // Critical assertion: Flag must be reset even after failure assert!(!handler.snapshot_in_progress()); } + + /// create_snapshot with retained_log_entries > 0 must still produce + /// last_included == last_applied β€” retained_log_entries must NOT affect the snapshot label. + /// + /// # Purpose + /// Regression guard for Bug #418: the old implementation subtracted + /// retained_log_entries from last_applied to compute last_included, producing a + /// snapshot whose label lied about how far the state had advanced. + /// With retained=10 and last_applied=100, the old code emitted last_included=90 + /// while the snapshot file actually captured state through index 100. + /// A follower installing that snapshot would re-apply entries 91~100, executing + /// non-idempotent operations twice and corrupting cluster state. + /// Log retention must be enforced at the purge layer (leader_state), not here. + /// + /// # Criteria + /// - retained_log_entries = 10 + /// - last_applied = LogId { index: 100, term: 1 } + /// + /// # Expected + /// - generate_snapshot_data is called with last_included = { index: 100, term: 1 } + /// - metadata.last_included == Some(LogId { index: 100, term: 1 }) (NOT index 90) + /// - re-apply gap = 0: follower installs at 100 and applies from 101, not 91 + #[tokio::test] + async fn test_create_snapshot_respects_retained_log_entries() { + let temp_dir = tempfile::tempdir().unwrap(); + let temp_path = temp_dir.path().join("test_create_snapshot_respects_retained_log_entries"); + let mut sm = MockStateMachine::new(); + + sm.expect_last_applied().returning(|| LogId { + term: 1, + index: 100, + }); + sm.expect_generate_snapshot_data() + .times(1) + .withf(|_path, last_included| { + last_included.index == 100 && last_included.term == 1 // NOT 90 + }) + .returning(|path, _| { + fs::create_dir_all(path).unwrap(); + Ok(Bytes::from(vec![0; 32])) + }); + + let mut config = snapshot_config(temp_path); + config.retained_log_entries = 10; + + let handler = Arc::new( + DefaultStateMachineHandler::::new_without_watch( + 1, + 0, + Arc::new(sm), + config, + MockSnapshotPolicy::new(), + ), + ); + + // Verify initial state + assert!(!handler.snapshot_in_progress()); + + let handler_clone = handler.clone(); + let result = handler_clone.create_snapshot().await; + assert!(result.is_ok()); + let (metadata, _) = result.unwrap(); + assert_eq!( + metadata.last_included, + Some(LogId { + term: 1, + index: 100 + }) + ); + } + + /// Snapshot last_included must carry the correct term from last_applied across a term boundary. + /// + /// # Purpose + /// Regression guard for Bug #418: the old code used last_applied.term for + /// last_included.term even when last_included.index fell in an earlier term. + /// Example: term 1 wrote entries 1~90, term 2 wrote entries 91~100. + /// With retained=60, old code computed last_included = { index: 40, term: 2 } β€” + /// a LogId that never existed in the Raft log (entry 40 was in term 1, not term 2). + /// With the fix, last_included is taken directly from last_applied, so both + /// index and term are truthful and no LogId is fabricated. + /// + /// # Criteria + /// - last_applied = LogId { index: 100, term: 2 } (term 1: entries 1~90, term 2: 91~100) + /// - retained_log_entries = 60 + /// + /// # Expected + /// - generate_snapshot_data is called with last_included = { index: 100, term: 2 } + /// - metadata.last_included == Some(LogId { index: 100, term: 2 }) + /// - no forged LogId { index: 40, term: 2 } is produced + #[tokio::test] + async fn test_create_snapshot_last_included_equals_last_applied_across_term_boundary() { + let temp_dir = tempfile::tempdir().unwrap(); + let temp_path = temp_dir + .path() + .join("test_create_snapshot_last_included_equals_last_applied_across_term_boundary"); + let mut sm = MockStateMachine::new(); + + // term 1: entries 1~90, term 2: entries 91~100 + // last_applied is at the head of term 2 + sm.expect_last_applied().returning(|| LogId { + term: 2, + index: 100, + }); + sm.expect_generate_snapshot_data() + .times(1) + .withf(|path, last_included| { + fs::create_dir_all(path).unwrap(); + // Old code: index = 100 - 60 = 40, term = 2 (copied from last_applied.term) + // β†’ forged LogId { index: 40, term: 2 } β€” entry 40 was in term 1, not term 2 + // New code: last_included = last_applied = { index: 100, term: 2 } β€” truthful + last_included.index == 100 && last_included.term == 2 + }) + .returning(|_, _| Ok(Bytes::from(vec![0; 32]))); + + let mut config = snapshot_config(temp_path.to_path_buf()); + // retained=60: old code would subtract and land in term 1 territory (index 40) + // while stamping term 2 β€” that LogId never existed + config.retained_log_entries = 60; + + let handler = DefaultStateMachineHandler::::new_without_watch( + 1, + 0, + Arc::new(sm), + config, + MockSnapshotPolicy::new(), + ); + + let result = handler.create_snapshot().await; + assert!(result.is_ok()); + let (metadata, _) = result.unwrap(); + assert_eq!( + metadata.last_included, + Some(LogId { + term: 2, + index: 100 + }) + ); + } + + /// create_snapshot when last_applied.index is zero must not panic or produce an invalid snapshot. + /// + /// # Purpose + /// Boundary: snapshot triggered before any entry has been applied. + /// Old and new code both produce last_included.index = 0 here + /// (0.saturating_sub(retained) = 0), but this test documents the boundary is + /// safe and regression-free under both implementations. + /// + /// # Criteria + /// - last_applied = LogId { index: 0, term: 0 } + /// - retained_log_entries = 10 + /// + /// # Expected + /// - create_snapshot returns Ok (no panic) + /// - metadata.last_included == Some(LogId { index: 0, term: 0 }) + #[tokio::test] + async fn test_create_snapshot_when_last_applied_index_is_zero() { + let temp_dir = tempfile::tempdir().unwrap(); + let temp_path = + temp_dir.path().join("test_create_snapshot_when_last_applied_index_is_zero"); + let mut sm = MockStateMachine::new(); + + sm.expect_last_applied().returning(|| LogId { term: 0, index: 0 }); + sm.expect_generate_snapshot_data() + .times(1) + .withf(|_, last_included| last_included.index == 0 && last_included.term == 0) + .returning(|path, _| { + fs::create_dir_all(path).unwrap(); + Ok(Bytes::from(vec![0; 32])) + }); + + let mut config = snapshot_config(temp_path.to_path_buf()); + config.retained_log_entries = 10; + + let handler = DefaultStateMachineHandler::::new_without_watch( + 1, + 0, + Arc::new(sm), + config, + MockSnapshotPolicy::new(), + ); + + let result = handler.create_snapshot().await; + assert!(result.is_ok()); + let (metadata, _) = result.unwrap(); + assert_eq!(metadata.last_included, Some(LogId { term: 0, index: 0 })); + } } // Helper functions @@ -1612,7 +1928,6 @@ async fn test_snapshot_compression() { // Mock state machine to create test data sm.expect_last_applied().returning(|| LogId { index: 10, term: 2 }); - sm.expect_entry_term().returning(|_| Some(2)); sm.expect_generate_snapshot_data().returning(|path, _| { // Create the directory structure correctly fs::create_dir_all(path.clone()).unwrap(); @@ -1802,7 +2117,7 @@ mod broadcast_watch_events_tests { use d_engine_proto::client::WatchEventType; use super::*; - use crate::{ApplyEntry, ApplyResult, Command}; + use crate::{ApplyEntry, ApplyResult, BatchOp, Command}; /// Build an `ApplyEntry` directly from a `Command` β€” no proto encoding needed. /// `broadcast_watch_events` receives already-decoded entries; tests mirror that reality. @@ -2062,6 +2377,166 @@ mod broadcast_watch_events_tests { assert!(rx.try_recv().is_err(), "Noop must not emit a watch event"); } + + /// Command::Batch with multiple Insert ops must broadcast one Put event per key. + /// + /// # Purpose + /// Batch is an atomic write of multiple ops. Each BatchOp::Insert must independently + /// produce a watch event so subscribers watching individual keys are notified. + /// This mirrors test_broadcast_insert_event but for the Batch command path. + /// + /// # Criteria + /// - Command::Batch with 2 Insert ops: (k1 β†’ v1), (k2 β†’ v2) + /// - apply succeeds for the batch entry + /// + /// # Expected + /// - broadcast channel receives exactly 2 Put events + /// - event 1: key=k1, value=v1, event_type=Put + /// - event 2: key=k2, value=v2, event_type=Put + /// - no extra events + #[tokio::test] + async fn test_broadcast_batch_insert_ops_each_produce_put_event() { + let (tx, mut rx) = tokio::sync::broadcast::channel(10); + let handler = create_test_handler(Path::new("/tmp/test_watch_batch_insert"), Some(0)); + + let entry = apply_entry( + 1, + Command::Batch { + ops: vec![ + BatchOp::Insert { + key: Bytes::from_static(b"k1"), + value: Bytes::from_static(b"v1"), + }, + BatchOp::Insert { + key: Bytes::from_static(b"k2"), + value: Bytes::from_static(b"v2"), + }, + ], + }, + ); + + handler.broadcast_watch_events(&[entry], &[succeeded(1)], &tx, None); + + let event1 = rx.recv().await.unwrap(); + assert_eq!(event1.key, Bytes::from_static(b"k1")); + assert_eq!(event1.value, Bytes::from_static(b"v1")); + assert_eq!(event1.event_type, WatchEventType::Put as i32); + + let event2 = rx.recv().await.unwrap(); + assert_eq!(event2.key, Bytes::from_static(b"k2")); + assert_eq!(event2.value, Bytes::from_static(b"v2")); + assert_eq!(event2.event_type, WatchEventType::Put as i32); + + assert!(rx.try_recv().is_err(), "Expected no further events"); + } + + /// Command::Batch with multiple Delete ops must broadcast one Delete event per key. + /// + /// # Purpose + /// Mirrors test_broadcast_batch_insert_ops_each_produce_put_event for the Delete path. + /// Each BatchOp::Delete must produce a Delete watch event with the correct key. + /// + /// # Criteria + /// - Command::Batch with 2 Delete ops: keys k1, k2 + /// - apply succeeds for the batch entry + /// + /// # Expected + /// - broadcast channel receives exactly 2 Delete events + /// - event 1: key=k1, event_type=Delete + /// - event 2: key=k2, event_type=Delete + /// - no extra events + #[tokio::test] + async fn test_broadcast_batch_delete_ops_each_produce_delete_event() { + let (tx, mut rx) = tokio::sync::broadcast::channel(10); + let handler = create_test_handler(Path::new("/tmp/test_watch_batch_delete"), Some(0)); + + let entry = apply_entry( + 1, + Command::Batch { + ops: vec![ + BatchOp::Delete { + key: Bytes::from_static(b"k1"), + }, + BatchOp::Delete { + key: Bytes::from_static(b"k2"), + }, + ], + }, + ); + + handler.broadcast_watch_events(&[entry], &[succeeded(1)], &tx, None); + + let event1 = rx.recv().await.unwrap(); + assert_eq!(event1.key, Bytes::from_static(b"k1")); + assert_eq!(event1.value, Bytes::new()); + assert_eq!(event1.event_type, WatchEventType::Delete as i32); + + let event2 = rx.recv().await.unwrap(); + assert_eq!(event2.key, Bytes::from_static(b"k2")); + assert_eq!(event2.value, Bytes::new()); + assert_eq!(event2.event_type, WatchEventType::Delete as i32); + + assert!(rx.try_recv().is_err(), "Expected no further events"); + } + + /// Command::Batch with mixed Insert and Delete ops produces events in op order. + /// + /// # Purpose + /// Op ordering within a batch must be preserved in the broadcast stream. + /// Subscribers may rely on event order to reconstruct final state correctly + /// (e.g., insert k1, then delete k1 must not arrive reversed). + /// + /// # Criteria + /// - Command::Batch: [Insert(k1=v1), Delete(k2), Insert(k3=v3)] + /// - apply succeeds for the batch entry + /// + /// # Expected + /// - broadcast channel receives exactly 3 events in order: + /// Put(k1=v1), Delete(k2), Put(k3=v3) + /// - no reordering, no missing events + #[tokio::test] + async fn test_broadcast_batch_mixed_ops_events_preserve_order() { + let (tx, mut rx) = tokio::sync::broadcast::channel(10); + let handler = create_test_handler(Path::new("/tmp/test_watch_batch_mixed"), Some(0)); + + let entry = apply_entry( + 1, + Command::Batch { + ops: vec![ + BatchOp::Insert { + key: Bytes::from_static(b"k1"), + value: Bytes::from_static(b"v1"), + }, + BatchOp::Delete { + key: Bytes::from_static(b"k2"), + }, + BatchOp::Insert { + key: Bytes::from_static(b"k3"), + value: Bytes::from_static(b"v3"), + }, + ], + }, + ); + + handler.broadcast_watch_events(&[entry], &[succeeded(1)], &tx, None); + + let event1 = rx.recv().await.unwrap(); + assert_eq!(event1.key, Bytes::from_static(b"k1")); + assert_eq!(event1.value, Bytes::from_static(b"v1")); + assert_eq!(event1.event_type, WatchEventType::Put as i32); + + let event2 = rx.recv().await.unwrap(); + assert_eq!(event2.key, Bytes::from_static(b"k2")); + assert_eq!(event2.value, Bytes::new()); + assert_eq!(event2.event_type, WatchEventType::Delete as i32); + + let event3 = rx.recv().await.unwrap(); + assert_eq!(event3.key, Bytes::from_static(b"k3")); + assert_eq!(event3.value, Bytes::from_static(b"v3")); + assert_eq!(event3.event_type, WatchEventType::Put as i32); + + assert!(rx.try_recv().is_err(), "Expected no further events"); + } } // ============================================================================= @@ -2234,3 +2709,201 @@ mod prev_kv_apply_tests { // mockall verifies on drop: exactly 1 get() call was made } } + +mod wait_applied_tests { + use super::*; + use crate::test_utils::snapshot_config; + use crate::{MockSnapshotPolicy, MockStateMachine, MockTypeConfig}; + + /// wait_applied returns immediately when last_applied is already >= target index. + /// + /// # Purpose + /// Raft ReadIndex fast path: if the SM has already applied up to or past the target + /// commit index, the read may proceed without suspending. Blocking unnecessarily + /// would stall linearizable reads and degrade throughput. + /// + /// # Criteria + /// - handler initialized with last_applied = 10 + /// - wait_applied(target = 5) called (5 <= 10, already satisfied) + /// + /// # Expected + /// - returns Ok(()) immediately (< 10ms, no suspension) + #[tokio::test] + async fn test_wait_applied_returns_immediately_when_already_applied() { + let handler = DefaultStateMachineHandler::::new_without_watch( + 1, + 10, // last_applied = 10 + Arc::new(MockStateMachine::new()), + snapshot_config(PathBuf::from("/tmp/test_wait_applied_fast_path")), + MockSnapshotPolicy::new(), + ); + + let start = std::time::Instant::now(); + let result = handler.wait_applied(5, Duration::from_millis(100)).await; + let elapsed = start.elapsed(); + + assert!( + result.is_ok(), + "Should return Ok when target <= last_applied" + ); + assert!( + elapsed < Duration::from_millis(10), + "Fast path must not suspend, actual: {elapsed:?}" + ); + } + + /// wait_applied blocks until apply_chunk advances last_applied to the target index. + /// + /// # Purpose + /// Raft ReadIndex blocking path: when the SM has not yet reached the commit index, + /// the read must wait. A concurrent apply_chunk call advancing last_applied to the + /// target must unblock the waiter at the correct index (not before, not after). + /// + /// # Criteria + /// - handler starts with last_applied = 0 + /// - wait_applied(target = 5) is spawned concurrently + /// - apply_chunk(entries 1..=5) is called from another task + /// + /// # Expected + /// - wait_applied blocks until index 5 is applied + /// - returns Ok(()) after unblocking + /// - does not unblock at index 4 or earlier + #[tokio::test] + async fn test_wait_applied_blocks_until_target_index_reached() { + use std::sync::atomic::{AtomicBool, Ordering}; + + let handler = Arc::new( + DefaultStateMachineHandler::::new_without_watch( + 1, + 0, + Arc::new(MockStateMachine::new()), + snapshot_config(PathBuf::from("/tmp/test_wait_applied_blocking")), + MockSnapshotPolicy::new(), + ), + ); + + let completed = Arc::new(AtomicBool::new(false)); + let completed_clone = completed.clone(); + let h = handler.clone(); + + tokio::spawn(async move { + let _ = h.wait_applied(5, Duration::from_millis(500)).await; + completed_clone.store(true, Ordering::SeqCst); + }); + + // Advance to 4 β€” waiter must remain blocked + tokio::time::sleep(Duration::from_millis(5)).await; + handler.test_simulate_apply(4); + tokio::time::sleep(Duration::from_millis(15)).await; + assert!( + !completed.load(Ordering::SeqCst), + "must not unblock at index 4" + ); + + // Advance to 5 β€” waiter must unblock + handler.test_simulate_apply(5); + tokio::time::sleep(Duration::from_millis(30)).await; + assert!(completed.load(Ordering::SeqCst), "must unblock at index 5"); + } + + /// Multiple concurrent waiters at different indices all wake correctly and independently. + /// + /// # Purpose + /// Prevents starvation and validates the broadcast-style notify under concurrent load. + /// The ReadIndex path can have many in-flight linearizable reads simultaneously, + /// each waiting on a different commit index. Each waiter must wake exactly at its + /// own threshold β€” no earlier, no later. + /// + /// # Criteria + /// - 3 concurrent waiters: wait_applied(3), wait_applied(5), wait_applied(7) + /// - apply_chunk progresses entry by entry: 1, 2, 3, 4, 5, 6, 7 + /// + /// # Expected + /// - waiter at 3 unblocks after index 3, not before + /// - waiter at 5 unblocks after index 5, not before + /// - waiter at 7 unblocks after index 7, not before + /// - all 3 return Ok(()) + #[tokio::test] + async fn test_wait_applied_multiple_waiters_all_wake_correctly() { + use std::sync::atomic::{AtomicBool, Ordering}; + + let handler = Arc::new( + DefaultStateMachineHandler::::new_without_watch( + 1, + 0, + Arc::new(MockStateMachine::new()), + snapshot_config(PathBuf::from("/tmp/test_wait_applied_multiple_waiters")), + MockSnapshotPolicy::new(), + ), + ); + + let (c3, c5, c7) = ( + Arc::new(AtomicBool::new(false)), + Arc::new(AtomicBool::new(false)), + Arc::new(AtomicBool::new(false)), + ); + + for (target, flag) in [(3u64, c3.clone()), (5, c5.clone()), (7, c7.clone())] { + let h = handler.clone(); + tokio::spawn(async move { + let _ = h.wait_applied(target, Duration::from_millis(500)).await; + flag.store(true, Ordering::SeqCst); + }); + } + + // Give spawned tasks a moment to enter their wait loops + tokio::time::sleep(Duration::from_millis(5)).await; + + // Advance to 2 β€” no waiter should unblock + handler.test_simulate_apply(2); + tokio::time::sleep(Duration::from_millis(10)).await; + assert!( + !c3.load(Ordering::SeqCst), + "waiter@3 must not unblock at index 2" + ); + assert!( + !c5.load(Ordering::SeqCst), + "waiter@5 must not unblock at index 2" + ); + assert!( + !c7.load(Ordering::SeqCst), + "waiter@7 must not unblock at index 2" + ); + + // Advance to 3 β€” only waiter@3 should unblock + handler.test_simulate_apply(3); + tokio::time::sleep(Duration::from_millis(20)).await; + assert!( + c3.load(Ordering::SeqCst), + "waiter@3 must unblock at index 3" + ); + assert!( + !c5.load(Ordering::SeqCst), + "waiter@5 must not unblock at index 3" + ); + assert!( + !c7.load(Ordering::SeqCst), + "waiter@7 must not unblock at index 3" + ); + + // Advance to 5 β€” waiter@5 should unblock, waiter@7 still waiting + handler.test_simulate_apply(5); + tokio::time::sleep(Duration::from_millis(20)).await; + assert!( + c5.load(Ordering::SeqCst), + "waiter@5 must unblock at index 5" + ); + assert!( + !c7.load(Ordering::SeqCst), + "waiter@7 must not unblock at index 5" + ); + + // Advance to 7 β€” waiter@7 unblocks + handler.test_simulate_apply(7); + tokio::time::sleep(Duration::from_millis(20)).await; + assert!( + c7.load(Ordering::SeqCst), + "waiter@7 must unblock at index 7" + ); + } +} diff --git a/d-engine-core/src/storage/state_machine.rs b/d-engine-core/src/storage/state_machine.rs index 95eeef46..f4a07116 100644 --- a/d-engine-core/src/storage/state_machine.rs +++ b/d-engine-core/src/storage/state_machine.rs @@ -147,13 +147,6 @@ pub trait StateMachine: Send + Sync + 'static { keys.iter().map(|k| self.get(k)).collect() } - /// Returns the term of a specific log entry by its ID. - /// Sync operation as it queries in-memory data. - fn entry_term( - &self, - entry_id: u64, - ) -> Option; - /// Applies a batch of decoded log entries to the state machine. /// /// Receives `&[ApplyEntry]` (already decoded by the framework) instead of raw diff --git a/d-engine-core/src/storage/state_machine_test.rs b/d-engine-core/src/storage/state_machine_test.rs index c265b1bc..59862b18 100644 --- a/d-engine-core/src/storage/state_machine_test.rs +++ b/d-engine-core/src/storage/state_machine_test.rs @@ -1,18 +1,16 @@ -use std::sync::Arc; -use std::time::Duration; -use std::time::Instant; - +use crate::ApplyEntry; +use crate::Command; +use crate::Error; +use crate::storage::StateMachine; use async_trait::async_trait; use bytes::Bytes; use d_engine_proto::common::LogId; use d_engine_proto::server::storage::SnapshotMetadata; +use std::sync::Arc; +use std::time::Duration; +use std::time::Instant; use tempfile::TempDir; -use crate::ApplyEntry; -use crate::Command; -use crate::Error; -use crate::storage::StateMachine; - /// Test suite for StateMachine implementations /// /// This suite provides comprehensive tests that can be used to validate @@ -38,6 +36,7 @@ impl StateMachineTestSuite { Self::test_basic_kv_operations(builder.build().await?).await?; Self::test_apply_chunk_functionality(builder.build().await?).await?; Self::test_cas_operations(builder.build().await?).await?; + Self::test_batch_operations(builder.build().await?).await?; Self::test_last_applied_detection(builder.build().await?).await?; Self::test_snapshot_operations(builder.build().await?).await?; Self::test_persistence(builder.build().await?).await?; @@ -45,6 +44,7 @@ impl StateMachineTestSuite { Self::test_drop_flushes_data(&builder).await?; Self::test_drop_persists_last_applied(&builder).await?; Self::test_data_survives_reopen(&builder).await?; + Self::test_batch_survives_reopen(&builder).await?; Self::test_ungraceful_shutdown_recovery(&builder).await?; Self::test_reset_operation(builder.build().await?).await?; @@ -149,6 +149,48 @@ impl StateMachineTestSuite { Ok(()) } + /// Verify that Command::Batch applies all ops atomically β€” inserts and deletes + /// within a single entry must all take effect. + pub async fn test_batch_operations(sm: Arc) -> Result<(), Error> { + use crate::BatchOp; + + sm.start().await?; + + let entry = create_batch_entry( + 1, + vec![ + BatchOp::Insert { + key: b"k1".to_vec().into(), + value: b"v1".to_vec().into(), + }, + BatchOp::Insert { + key: b"k2".to_vec().into(), + value: b"v2".to_vec().into(), + }, + BatchOp::Delete { + key: b"k1".to_vec().into(), + }, + ], + ); + sm.apply_chunk(&[entry]).await?; + + // k1 was deleted after insert, k2 survived + assert_eq!( + sm.get(b"k1")?, + None, + "k1 inserted then deleted in batch must be gone" + ); + assert_eq!( + sm.get(b"k2")?, + Some(b"v2".to_vec().into()), + "k2 inserted in batch must survive" + ); + assert_eq!(sm.last_applied().index, 1); + + sm.stop()?; + Ok(()) + } + /// Test chunk application functionality pub async fn test_apply_chunk_functionality( state_machine: Arc @@ -663,6 +705,65 @@ impl StateMachineTestSuite { Ok(()) } + /// Batch ops must survive WAL encode β†’ crash β†’ decode round-trip. + /// + /// Covers the #415 fix: Command::Batch encoding in file-based state machines + /// must produce records that replay correctly after an ungraceful shutdown. + pub async fn test_batch_survives_reopen( + builder: &B + ) -> Result<(), Error> { + use crate::BatchOp; + + // Step 1: apply a batch with mixed Insert + Delete, then drop without stop() + { + let sm = builder.build().await?; + sm.start().await?; + + let entry = create_batch_entry( + 1, + vec![ + BatchOp::Insert { + key: b"bk1".to_vec().into(), + value: b"bv1".to_vec().into(), + }, + BatchOp::Insert { + key: b"bk2".to_vec().into(), + value: b"bv2".to_vec().into(), + }, + BatchOp::Delete { + key: b"bk1".to_vec().into(), + }, + ], + ); + sm.apply_chunk(&[entry]).await?; + + assert_eq!(sm.get(b"bk1")?, None); + assert_eq!(sm.get(b"bk2")?, Some(b"bv2".to_vec().into())); + assert_eq!(sm.last_applied().index, 1); + } // drop β†’ crash simulation + + tokio::time::sleep(Duration::from_millis(50)).await; + + // Step 2: reopen from same path β€” WAL must replay correctly + let sm2 = builder.build().await?; + sm2.start().await?; + + assert_eq!( + sm2.get(b"bk1")?, + None, + "deleted key must be absent after recovery" + ); + assert_eq!( + sm2.get(b"bk2")?, + Some(b"bv2".to_vec().into()), + "inserted key must survive recovery" + ); + assert_eq!(sm2.last_applied().index, 1, "last_applied must be restored"); + + sm2.stop()?; + Ok(()) + } + /// Test recovery from ungraceful shutdown (crash simulation) /// /// This test simulates a crash by: @@ -1211,6 +1312,18 @@ fn create_cas_entry( } } +/// Helper: create a single Batch entry with the given ops. +fn create_batch_entry( + index: u64, + ops: Vec, +) -> ApplyEntry { + ApplyEntry { + index, + term: 1, + command: Command::Batch { ops }, + } +} + // ============================================================================ // Default scan_prefix implementation test // ============================================================================ @@ -1258,13 +1371,6 @@ mod default_scan_prefix_tests { Ok(None) } - fn entry_term( - &self, - _entry_id: u64, - ) -> Option { - None - } - async fn apply_chunk( &self, _chunk: &[ApplyEntry], diff --git a/d-engine-core/src/test_utils/mock/mock_raft_builder.rs b/d-engine-core/src/test_utils/mock/mock_raft_builder.rs index 9bd74dde..d6be61e8 100644 --- a/d-engine-core/src/test_utils/mock/mock_raft_builder.rs +++ b/d-engine-core/src/test_utils/mock/mock_raft_builder.rs @@ -286,6 +286,14 @@ impl MockBuilder { self } + pub fn with_purge_executor( + mut self, + purge_executor: MockPurgeExecutor, + ) -> Self { + self.purge_executor = Some(purge_executor); + self + } + pub fn with_node_config( mut self, node_config: RaftNodeConfig, @@ -352,7 +360,6 @@ pub fn mock_state_machine() -> MockStateMachine { mock.expect_is_running().returning(|| true); mock.expect_get().returning(|_| Ok(None)); - mock.expect_entry_term().returning(|_| None); mock.expect_apply_chunk().returning(|_| Ok(vec![])); mock.expect_len().returning(|| 0); diff --git a/d-engine-proto/proto/client/client_api.proto b/d-engine-proto/proto/client/client_api.proto index d80a2f41..d5c7d1b7 100644 --- a/d-engine-proto/proto/client/client_api.proto +++ b/d-engine-proto/proto/client/client_api.proto @@ -22,10 +22,20 @@ message WriteCommand { optional bytes expected_value = 2; // None means key must not exist bytes new_value = 3; // New value to set if comparison succeeds } + message BatchOp { + oneof op { + Insert insert = 1; + Delete delete = 2; + } + } + message Batch { + repeated BatchOp ops = 1; + } oneof operation { Insert insert = 1; Delete delete = 2; CompareAndSwap compare_and_swap = 3; + Batch batch = 4; } } diff --git a/d-engine-proto/src/exts/client_ext.rs b/d-engine-proto/src/exts/client_ext.rs index 6aa6f552..52eeddf3 100644 --- a/d-engine-proto/src/exts/client_ext.rs +++ b/d-engine-proto/src/exts/client_ext.rs @@ -14,6 +14,9 @@ use crate::client::WriteCommand; use crate::client::WriteResult; use crate::client::client_response::SuccessResult; use crate::client::write_command; +use crate::client::write_command::Batch; +use crate::client::write_command::BatchOp; +use crate::client::write_command::Operation; use crate::error::ErrorCode; use crate::error::ErrorMetadata; @@ -86,6 +89,12 @@ impl WriteCommand { } } + pub fn batch(ops: Vec) -> Self { + Self { + operation: Some(Operation::Batch(Batch { ops })), + } + } + /// Create deletion command for specified key /// /// # Parameters @@ -119,6 +128,8 @@ impl WriteCommand { } } +impl Batch {} + impl ClientResponse { /// Build success response for write operations /// diff --git a/d-engine-proto/src/generated/d_engine.client.rs b/d-engine-proto/src/generated/d_engine.client.rs index bbb6a5c5..84a336a4 100644 --- a/d-engine-proto/src/generated/d_engine.client.rs +++ b/d-engine-proto/src/generated/d_engine.client.rs @@ -3,7 +3,7 @@ #[derive(serde::Serialize, serde::Deserialize)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct WriteCommand { - #[prost(oneof = "write_command::Operation", tags = "1, 2, 3")] + #[prost(oneof = "write_command::Operation", tags = "1, 2, 3, 4")] pub operation: ::core::option::Option, } /// Nested message and enum types in `WriteCommand`. @@ -39,6 +39,29 @@ pub mod write_command { pub new_value: ::prost::bytes::Bytes, } #[derive(serde::Serialize, serde::Deserialize)] + #[derive(Clone, PartialEq, ::prost::Message)] + pub struct BatchOp { + #[prost(oneof = "batch_op::Op", tags = "1, 2")] + pub op: ::core::option::Option, + } + /// Nested message and enum types in `BatchOp`. + pub mod batch_op { + #[derive(serde::Serialize, serde::Deserialize)] + #[derive(Clone, PartialEq, ::prost::Oneof)] + pub enum Op { + #[prost(message, tag = "1")] + Insert(super::Insert), + #[prost(message, tag = "2")] + Delete(super::Delete), + } + } + #[derive(serde::Serialize, serde::Deserialize)] + #[derive(Clone, PartialEq, ::prost::Message)] + pub struct Batch { + #[prost(message, repeated, tag = "1")] + pub ops: ::prost::alloc::vec::Vec, + } + #[derive(serde::Serialize, serde::Deserialize)] #[derive(Clone, PartialEq, ::prost::Oneof)] pub enum Operation { #[prost(message, tag = "1")] @@ -47,6 +70,8 @@ pub mod write_command { Delete(Delete), #[prost(message, tag = "3")] CompareAndSwap(CompareAndSwap), + #[prost(message, tag = "4")] + Batch(Batch), } } #[derive(serde::Serialize, serde::Deserialize)] diff --git a/d-engine-server/benches/state_machine.rs b/d-engine-server/benches/state_machine.rs index 87d8a80c..13d50eea 100644 --- a/d-engine-server/benches/state_machine.rs +++ b/d-engine-server/benches/state_machine.rs @@ -357,6 +357,37 @@ fn bench_apply_with_1_watcher(c: &mut Criterion) { revision: 0, }); } + Command::Batch { ops } => { + for op in ops { + match op { + d_engine_core::BatchOp::Insert { key, value } => { + let _ = + broadcast_tx.send(d_engine_proto::client::WatchResponse { + key: key.clone(), + value: value.clone(), + event_type: d_engine_proto::client::WatchEventType::Put + as i32, + prev_value: bytes::Bytes::new(), + error: 0, + revision: 0, + }); + } + d_engine_core::BatchOp::Delete { key } => { + let _ = + broadcast_tx.send(d_engine_proto::client::WatchResponse { + key: key.clone(), + value: bytes::Bytes::new(), + event_type: + d_engine_proto::client::WatchEventType::Delete + as i32, + prev_value: bytes::Bytes::new(), + error: 0, + revision: 0, + }); + } + } + } + } Command::Noop => {} } } @@ -417,6 +448,37 @@ fn bench_apply_with_10_watchers(c: &mut Criterion) { revision: 0, }); } + Command::Batch { ops } => { + for op in ops { + match op { + d_engine_core::BatchOp::Insert { key, value } => { + let _ = + broadcast_tx.send(d_engine_proto::client::WatchResponse { + key: key.clone(), + value: value.clone(), + event_type: d_engine_proto::client::WatchEventType::Put + as i32, + prev_value: bytes::Bytes::new(), + error: 0, + revision: 0, + }); + } + d_engine_core::BatchOp::Delete { key } => { + let _ = + broadcast_tx.send(d_engine_proto::client::WatchResponse { + key: key.clone(), + value: bytes::Bytes::new(), + event_type: + d_engine_proto::client::WatchEventType::Delete + as i32, + prev_value: bytes::Bytes::new(), + error: 0, + revision: 0, + }); + } + } + } + } Command::Noop => {} } } @@ -477,6 +539,37 @@ fn bench_apply_with_100_watchers(c: &mut Criterion) { revision: 0, }); } + Command::Batch { ops } => { + for op in ops { + match op { + d_engine_core::BatchOp::Insert { key, value } => { + let _ = + broadcast_tx.send(d_engine_proto::client::WatchResponse { + key: key.clone(), + value: value.clone(), + event_type: d_engine_proto::client::WatchEventType::Put + as i32, + prev_value: bytes::Bytes::new(), + error: 0, + revision: 0, + }); + } + d_engine_core::BatchOp::Delete { key } => { + let _ = + broadcast_tx.send(d_engine_proto::client::WatchResponse { + key: key.clone(), + value: bytes::Bytes::new(), + event_type: + d_engine_proto::client::WatchEventType::Delete + as i32, + prev_value: bytes::Bytes::new(), + error: 0, + revision: 0, + }); + } + } + } + } Command::Noop => {} } } diff --git a/d-engine-server/src/api/embedded.rs b/d-engine-server/src/api/embedded.rs index f04c4775..7cef3d53 100644 --- a/d-engine-server/src/api/embedded.rs +++ b/d-engine-server/src/api/embedded.rs @@ -34,10 +34,10 @@ //! //! ## What EmbeddedEngine Provides //! -//! - βœ… `is_leader()` - Check if current node is leader -//! - βœ… `leader_info()` - Get leader ID and term -//! - βœ… `EmbeddedClient` returns `NotLeader` error on follower writes -//! - βœ… Zero-overhead in-process communication (<0.1ms) +//! - `is_leader()` - Check if current node is leader +//! - `leader_info()` - Get leader ID and term +//! - `EmbeddedClient` returns `NotLeader` error on follower writes +//! - Zero-overhead in-process communication (<0.1ms) //! //! ## What Applications Must Handle //! @@ -264,9 +264,14 @@ impl EmbeddedEngine> { /// let engine = EmbeddedEngine::start_with("config/node1.toml").await?; /// engine.wait_ready(Duration::from_secs(5)).await?; /// ``` - pub async fn start_with(config_path: &str) -> Result { + pub async fn start_with(config_path: impl AsRef) -> Result { + let path_str = config_path + .as_ref() + .to_str() + .ok_or_else(|| crate::Error::Fatal("config path is not valid UTF-8".into()))?; + let config = d_engine_core::RaftNodeConfig::new()? - .with_override_config(config_path)? + .with_override_config(path_str)? .validate()?; let base_dir = std::path::PathBuf::from(&config.cluster.db_root_dir); @@ -296,7 +301,7 @@ impl EmbeddedEngine> { )); sm.set_lease(lease); - Self::start_custom(Arc::new(storage), Arc::new(sm), Some(config_path)).await + Self::start_custom(Arc::new(storage), Arc::new(sm), Some(path_str)).await } } @@ -338,8 +343,11 @@ where Self::start_node(node_config, storage_engine, state_machine).await } - /// Build and launch the node from a validated config and pre-built storage. - async fn start_node( + /// Start engine with custom storage, state machine, and programmatic config. + /// + /// For library wrappers that build their own config layer and + /// translate to `RaftNodeConfig` in Rust β€” no config file required. + pub async fn start_node( node_config: d_engine_core::RaftNodeConfig, storage_engine: Arc, state_machine: Arc, @@ -356,7 +364,7 @@ where let sm_for_engine = Arc::clone(&state_machine); - let node = NodeBuilder::init(node_config, shutdown_rx) + let node = NodeBuilder::init(node_config.validate()?, shutdown_rx) .storage_engine(storage_engine) .state_machine(state_machine) .start() diff --git a/d-engine-server/src/api/embedded_client.rs b/d-engine-server/src/api/embedded_client.rs index 0f0ffd76..7b92b0d3 100644 --- a/d-engine-server/src/api/embedded_client.rs +++ b/d-engine-server/src/api/embedded_client.rs @@ -15,21 +15,23 @@ //! client.put(b"key", b"value").await?; //! ``` -#[cfg(feature = "watch")] -use std::sync::Arc; -use std::time::Duration; - +use crate::api::types::WriteOperation; +use crate::proto_convert; use bytes::Bytes; +use d_engine_core::BatchOp; use d_engine_core::InboundEvent; use d_engine_core::MaybeCloneOneshot; use d_engine_core::RaftOneshot; use d_engine_core::ScanResult; use d_engine_core::TypeConfig; use d_engine_core::client::{ - ClientApi, ClientApiResult, ClientResponsePayload, ClientWriteRequest, ErrorCode, - WriteOperation, + ClientApi, ClientApiError, ClientApiResult, ClientResponsePayload, ClientWriteRequest, + ErrorCode, }; use d_engine_core::config::ReadConsistencyPolicy; +#[cfg(feature = "watch")] +use std::sync::Arc; +use std::time::Duration; use tokio::sync::mpsc; use super::embedded_read_handle::EmbeddedReadHandle; @@ -37,8 +39,6 @@ pub(crate) use super::standalone_read_handle::{ channel_closed_error, map_error_response, server_error, timeout_error, }; -#[cfg(feature = "watch")] -use d_engine_core::client::ClientApiError; #[cfg(feature = "watch")] use d_engine_core::watch::WatchRegistry; @@ -110,11 +110,11 @@ impl EmbeddedClient { ) -> ClientApiResult<()> { let request = ClientWriteRequest { client_id: self.client_id, - command: Some(WriteOperation::Insert { + command: Some(proto_convert::write_op_to_bytes(WriteOperation::Insert { key: Bytes::copy_from_slice(key.as_ref()), value: Bytes::copy_from_slice(value.as_ref()), ttl_secs: None, - }), + })), }; let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); @@ -281,9 +281,9 @@ impl EmbeddedClient { ) -> ClientApiResult<()> { let request = ClientWriteRequest { client_id: self.client_id, - command: Some(WriteOperation::Delete { + command: Some(proto_convert::write_op_to_bytes(WriteOperation::Delete { key: Bytes::copy_from_slice(key.as_ref()), - }), + })), }; let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); @@ -575,11 +575,66 @@ impl ClientApi for EmbeddedClient { ) -> ClientApiResult<()> { let request = ClientWriteRequest { client_id: self.client_id, - command: Some(WriteOperation::Insert { + command: Some(proto_convert::write_op_to_bytes(WriteOperation::Insert { key: Bytes::copy_from_slice(key.as_ref()), value: Bytes::copy_from_slice(value.as_ref()), ttl_secs: Some(ttl_secs), - }), + })), + }; + + let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); + + self.read_handle + .cmd_tx + .send(d_engine_core::ClientCmd::Propose(request, resp_tx)) + .await + .map_err(|_| channel_closed_error())?; + + let result = tokio::time::timeout(self.timeout, resp_rx) + .await + .map_err(|_| timeout_error(self.timeout))? + .map_err(|_| channel_closed_error())?; + + let response = + result.map_err(|status| server_error(format!("RPC error: {}", status.message())))?; + + if response.error != ErrorCode::Success { + return Err(map_error_response( + response.error, + response.leader_hint, + response.retry_after_ms, + )); + } + + Ok(()) + } + + /// Atomically commits multiple write operations as a single Raft log entry. + /// + /// All operations succeed or none apply (all-or-nothing). + /// Op ordering within the batch is preserved by the state machine. + /// + /// # Errors + /// + /// Returns an error if `ops` is empty, the node is not the leader, the channel + /// is closed, the request times out, or the state machine returns a server error. + async fn batch( + &self, + ops: Vec, + ) -> ClientApiResult<()> { + if ops.is_empty() { + return Err(ClientApiError::Business { + code: ErrorCode::InvalidRequest, + message: "batch ops must not be empty".into(), + required_action: None, + }); + } + + let request = ClientWriteRequest { + client_id: self.client_id, + command: Some(proto_convert::write_op_to_bytes(WriteOperation::Batch { + ops, + })), }; let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); @@ -638,11 +693,13 @@ impl ClientApi for EmbeddedClient { ) -> ClientApiResult { let request = ClientWriteRequest { client_id: self.client_id, - command: Some(WriteOperation::CompareAndSwap { - key: Bytes::copy_from_slice(key.as_ref()), - expected: expected_value.map(|v| Bytes::copy_from_slice(v.as_ref())), - new_value: Bytes::copy_from_slice(new_value.as_ref()), - }), + command: Some(proto_convert::write_op_to_bytes( + WriteOperation::CompareAndSwap { + key: Bytes::copy_from_slice(key.as_ref()), + expected: expected_value.map(|v| Bytes::copy_from_slice(v.as_ref())), + new_value: Bytes::copy_from_slice(new_value.as_ref()), + }, + )), }; let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); diff --git a/d-engine-server/src/api/embedded_test/embedded_test.rs b/d-engine-server/src/api/embedded_test/embedded_test.rs index 60e498e5..ae8bc813 100644 --- a/d-engine-server/src/api/embedded_test/embedded_test.rs +++ b/d-engine-server/src/api/embedded_test/embedded_test.rs @@ -549,6 +549,144 @@ listen_addr = "127.0.0.1:0" } } + /// Tests for `start_node` β€” the programmatic-config entry point for + /// library wrappers that build their own `RaftNodeConfig` in Rust. + mod start_node_tests { + use std::net::SocketAddr; + + use super::*; + use serial_test::serial; + + use bytes::Bytes; + use d_engine_core::ClientApi; + use d_engine_core::StateMachine; + use d_engine_core::config::ClusterConfig; + use d_engine_proto::common::NodeRole; + use d_engine_proto::common::NodeStatus; + use d_engine_proto::server::cluster::NodeMeta; + + /// Build a minimal valid `RaftNodeConfig` backed by a temp directory. + /// Uses a fixed high port; tests are serialized to avoid conflicts. + fn make_config( + temp_dir: &tempfile::TempDir, + node_id: u32, + ) -> d_engine_core::RaftNodeConfig { + let listen_addr: SocketAddr = "127.0.0.1:19731".parse().unwrap(); + d_engine_core::RaftNodeConfig { + cluster: ClusterConfig { + node_id, + listen_address: listen_addr, + initial_cluster: vec![NodeMeta { + id: node_id, + address: listen_addr.to_string(), + role: NodeRole::Follower as i32, + status: NodeStatus::Active.into(), + }], + db_root_dir: temp_dir.path().join("engine"), + log_dir: temp_dir.path().join("logs"), + }, + ..Default::default() + } + } + + /// Verify that `start_node` accepts a programmatic `RaftNodeConfig`, + /// starts the engine successfully, and stops cleanly β€” no config file needed. + #[tokio::test] + #[serial(start_node)] + async fn test_start_node_with_programmatic_config_starts_and_stops() { + let (storage, sm, temp_dir) = create_test_storage_and_sm().await; + let config = make_config(&temp_dir, 1); + + let engine = TestEngine::start_node(config, storage, sm) + .await + .expect("start_node should succeed with valid programmatic config"); + + engine + .wait_ready(Duration::from_secs(5)) + .await + .expect("single-node engine should elect itself as leader"); + + engine.stop().await.expect("stop should succeed"); + } + + /// `node_id = 0` is rejected by `ClusterConfig::validate()`. + /// `start_node` calls `validate()` internally, so the error must propagate to the caller. + #[tokio::test] + #[serial(start_node)] + async fn test_start_node_unvalidated_config_returns_error() { + let (storage, sm, temp_dir) = create_test_storage_and_sm().await; + let config = make_config(&temp_dir, 0); + + let result = TestEngine::start_node(config, storage, sm).await; + assert!( + result.is_err(), + "start_node with node_id=0 should return validation error" + ); + } + + /// Write through the engine's client, then read directly from the + /// original SM reference β€” proves the engine uses the passed-in instance. + #[tokio::test] + #[serial(start_node)] + async fn test_start_node_uses_passed_se_and_sm_instances() { + let (storage, sm, temp_dir) = create_test_storage_and_sm().await; + let sm_for_direct_read = Arc::clone(&sm); + let config = make_config(&temp_dir, 1); + + let engine = TestEngine::start_node(config, storage, sm) + .await + .expect("start_node should succeed"); + engine + .wait_ready(Duration::from_secs(5)) + .await + .expect("single-node engine should elect leader"); + + // Write through the embedded client + let client = engine.client(); + client + .put(b"hello".to_vec(), b"world".to_vec()) + .await + .expect("put should succeed"); + + // Read directly from the original SM reference β€” same instance + let value = sm_for_direct_read.get(b"hello").unwrap(); + assert_eq!( + value, + Some(Bytes::from_static(b"world")), + "data written through client must be visible via the original SM reference" + ); + + engine.stop().await.expect("stop should succeed"); + } + + /// `client.batch(vec![])` must return InvalidArgument β€” empty batch is a no-op + /// that should be rejected at the API boundary, not committed as an empty Raft entry. + #[tokio::test] + #[serial(start_node)] + async fn test_batch_empty_ops_returns_invalid_argument() { + let (storage, sm, temp_dir) = create_test_storage_and_sm().await; + let config = make_config(&temp_dir, 1); + + let engine = TestEngine::start_node(config, storage, sm) + .await + .expect("start_node should succeed"); + engine + .wait_ready(Duration::from_secs(5)) + .await + .expect("single-node engine should elect leader"); + + let client = engine.client(); + let result = client.batch(vec![]).await; + assert!( + result.is_err(), + "empty batch must return error, got: {:?}", + result + ); + + engine.stop().await.expect("stop should succeed"); + } + } + #[cfg(feature = "watch")] mod watch_tests { use serial_test::serial; @@ -976,6 +1114,40 @@ listen_addr = "127.0.0.1:0" engine.stop().await.expect("Failed to stop engine"); } + + /// Single-voter LinearizableRead must complete and return the committed value. + /// + /// Regression: on single-voter cluster, when `last_applied < read_index` at the + /// moment the read arrived, the read was queued in `pending_reads` with no drain + /// path β€” Path A (peer ACK) is impossible without peers; Path B (SM apply) is + /// impossible for a pure read. The fix must serve single-voter linearizable reads + /// directly from SM without going through the quorum path. + /// + /// If this test hangs and times out, the regression is present. + #[tokio::test] + async fn test_linearizable_read_single_voter_returns_committed_value() { + let (storage, sm, _temp_dir) = create_test_storage_and_sm().await; + + // Start embedded engine (single node) + let engine = TestEngine::start_custom(storage, sm, None) + .await + .expect("Failed to start engine"); + + // Wait for leader election (should succeed quickly in single-node mode) + let result = engine.wait_ready(Duration::from_secs(5)).await; + + assert!( + result.is_ok(), + "Leader election should succeed in single-node mode" + ); + let client = engine.client(); + assert!(client.put(b"key", b"value").await.is_ok()); + let got = client.get_linearizable(b"key").await.expect("linearizable read must succeed"); + assert_eq!(got.as_deref(), Some(b"value".as_ref())); + + // Cleanup + engine.stop().await.expect("Failed to stop engine"); + } } /// Tests for unified RocksDB path (`unified_db = true`) in `start_with()` and `start()`. diff --git a/d-engine-server/src/api/mod.rs b/d-engine-server/src/api/mod.rs index e78e38ab..5a42028b 100644 --- a/d-engine-server/src/api/mod.rs +++ b/d-engine-server/src/api/mod.rs @@ -5,6 +5,7 @@ mod embedded_client; mod embedded_read_handle; mod standalone; mod standalone_read_handle; +pub(crate) mod types; pub use standalone::StandaloneEngine; pub(crate) use standalone_read_handle::StandaloneReadHandle; diff --git a/d-engine-server/src/api/standalone.rs b/d-engine-server/src/api/standalone.rs index 17f64d40..148a4b49 100644 --- a/d-engine-server/src/api/standalone.rs +++ b/d-engine-server/src/api/standalone.rs @@ -93,7 +93,7 @@ impl StandaloneEngine { /// ``` #[cfg(feature = "rocksdb")] pub async fn run_with( - config_path: &str, + config_path: impl AsRef, shutdown_rx: watch::Receiver<()>, ) -> Result<()> { let config = d_engine_core::RaftNodeConfig::new()? @@ -136,10 +136,10 @@ impl StandaloneEngine { /// Blocks until shutdown signal is received. /// /// # Arguments - /// * `config` - Node configuration /// * `storage_engine` - Custom storage engine implementation /// * `state_machine` - Custom state machine implementation /// * `shutdown_rx` - Shutdown signal receiver + /// * `config` - config_path - Optional path to configuration file /// /// # Example /// ```ignore @@ -153,7 +153,7 @@ impl StandaloneEngine { storage_engine: Arc, state_machine: Arc, shutdown_rx: watch::Receiver<()>, - config_path: Option<&str>, + config_path: Option>, ) -> Result<()> where SE: StorageEngine + std::fmt::Debug + 'static, @@ -169,7 +169,13 @@ impl StandaloneEngine { Self::start_node(config, storage_engine, state_machine, shutdown_rx).await } - async fn start_node( + /// Start standalone server with custom storage, state machine, and programmatic config. + /// + /// For library wrappers that build their own config layer and + /// translate to `RaftNodeConfig` in Rust β€” no config file required. + /// + /// Blocks until `shutdown_rx` receives a signal. + pub async fn start_node( config: d_engine_core::RaftNodeConfig, storage_engine: Arc, state_machine: Arc, @@ -179,7 +185,7 @@ impl StandaloneEngine { SE: StorageEngine + std::fmt::Debug + 'static, SM: StateMachine + std::fmt::Debug + 'static, { - let node = NodeBuilder::init(config, shutdown_rx) + let node = NodeBuilder::init(config.validate()?, shutdown_rx) .storage_engine(storage_engine) .state_machine(state_machine) .start() diff --git a/d-engine-server/src/api/standalone_test.rs b/d-engine-server/src/api/standalone_test.rs index dfc37253..3f9e57cd 100644 --- a/d-engine-server/src/api/standalone_test.rs +++ b/d-engine-server/src/api/standalone_test.rs @@ -242,6 +242,343 @@ mod standalone_server_tests { } } +/// Tests for `start_node` β€” the programmatic-config entry point for +/// library wrappers that build their own `RaftNodeConfig` in Rust. +#[cfg(all(test, feature = "rocksdb"))] +mod start_node_tests { + use std::net::SocketAddr; + use std::sync::Arc; + use std::time::Duration; + + use serial_test::serial; + use tokio::sync::watch; + + use d_engine_core::config::ClusterConfig; + use d_engine_proto::common::NodeRole; + use d_engine_proto::common::NodeStatus; + use d_engine_proto::server::cluster::NodeMeta; + + use crate::RocksDBStateMachine; + use crate::RocksDBStorageEngine; + use crate::api::StandaloneEngine; + use crate::storage::TtlLease; + + /// Build a minimal valid `RaftNodeConfig` backed by a temp directory. + fn make_config( + temp_dir: &tempfile::TempDir, + node_id: u32, + ) -> d_engine_core::RaftNodeConfig { + let listen_addr: SocketAddr = "127.0.0.1:19732".parse().unwrap(); + d_engine_core::RaftNodeConfig { + cluster: ClusterConfig { + node_id, + listen_address: listen_addr, + initial_cluster: vec![NodeMeta { + id: node_id, + address: listen_addr.to_string(), + role: NodeRole::Follower as i32, + status: NodeStatus::Active.into(), + }], + db_root_dir: temp_dir.path().join("engine"), + log_dir: temp_dir.path().join("logs"), + }, + ..Default::default() + } + } + + /// Create RocksDB storage engine + state machine with lease initialized. + fn create_rocksdb_se_and_sm( + temp_dir: &tempfile::TempDir + ) -> (Arc, Arc) { + let storage_path = temp_dir.path().join("storage"); + let sm_path = temp_dir.path().join("sm"); + std::fs::create_dir_all(&storage_path).unwrap(); + std::fs::create_dir_all(&sm_path).unwrap(); + + let storage = + Arc::new(RocksDBStorageEngine::new(&storage_path).expect("create storage engine")); + let mut sm = RocksDBStateMachine::new(&sm_path).expect("create state machine"); + sm.set_lease(Arc::new(TtlLease::new(Default::default()))); + let sm = Arc::new(sm); + + (storage, sm) + } + + // ── start_node tests ────────────────────────────────────────────────── + + /// Happy path: build a valid config and SE/SM programmatically, start the + /// server, and shut it down cleanly β€” no config file needed. + #[tokio::test] + #[serial(start_node)] + async fn test_start_node_with_programmatic_config_starts_and_stops() { + let temp_dir = tempfile::tempdir().expect("tempdir"); + let (storage, sm) = create_rocksdb_se_and_sm(&temp_dir); + let config = make_config(&temp_dir, 1); + let storage_path = temp_dir.path().join("storage"); + let sm_path = temp_dir.path().join("sm"); + + let (shutdown_tx, shutdown_rx) = watch::channel(()); + let handle = tokio::spawn(StandaloneEngine::start_node( + config, + storage, + sm, + shutdown_rx, + )); + + tokio::time::sleep(Duration::from_millis(200)).await; + shutdown_tx.send(()).ok(); + + let result = tokio::time::timeout(Duration::from_secs(10), handle) + .await + .expect("server must stop within timeout") + .expect("server task must not panic"); + assert!( + result.is_ok(), + "start_node should succeed: {:?}", + result.err() + ); + + // RocksDB must have persisted data β€” proves the engine used the passed-in instances. + assert!( + storage_path.join("CURRENT").exists() || storage_path.join("IDENTITY").exists(), + "storage engine must have persisted data to {:?}", + storage_path + ); + assert!( + sm_path.join("CURRENT").exists() || sm_path.join("IDENTITY").exists(), + "state machine must have persisted data to {:?}", + sm_path + ); + } + + /// `node_id = 0` is rejected by `ClusterConfig::validate()`. + /// `start_node` calls `validate()` internally β€” the error must propagate. + #[tokio::test] + #[serial(start_node)] + async fn test_start_node_unvalidated_config_returns_error() { + let temp_dir = tempfile::tempdir().expect("tempdir"); + let (storage, sm) = create_rocksdb_se_and_sm(&temp_dir); + let (_shutdown_tx, shutdown_rx) = watch::channel(()); + let config = make_config(&temp_dir, 0); // node_id=0 triggers validation error + + let result = StandaloneEngine::start_node(config, storage, sm, shutdown_rx).await; + assert!( + result.is_err(), + "start_node with node_id=0 must return validation error" + ); + } + + /// RocksDB files are written to the passed-in paths after a clean shutdown β€” + /// proves the engine actually uses the caller's SE and SM instances. + #[tokio::test] + #[serial(start_node)] + async fn test_start_node_persists_data_to_passed_paths() { + let temp_dir = tempfile::tempdir().expect("tempdir"); + let storage_path = temp_dir.path().join("my-storage"); + let sm_path = temp_dir.path().join("my-sm"); + std::fs::create_dir_all(&storage_path).unwrap(); + std::fs::create_dir_all(&sm_path).unwrap(); + + let storage = Arc::new(RocksDBStorageEngine::new(&storage_path).expect("create storage")); + let mut sm = RocksDBStateMachine::new(&sm_path).expect("create sm"); + sm.set_lease(Arc::new(TtlLease::new(Default::default()))); + let sm = Arc::new(sm); + + let config = make_config(&temp_dir, 1); + + let (shutdown_tx, shutdown_rx) = watch::channel(()); + let handle = tokio::spawn(StandaloneEngine::start_node( + config, + storage, + sm, + shutdown_rx, + )); + + tokio::time::sleep(Duration::from_millis(200)).await; + shutdown_tx.send(()).ok(); + + let _ = tokio::time::timeout(Duration::from_secs(10), handle).await; + + // After clean shutdown, RocksDB must have flushed data. + let has_storage_files = std::fs::read_dir(&storage_path) + .map(|mut d| d.any(|e| e.is_ok())) + .unwrap_or(false); + let has_sm_files = + std::fs::read_dir(&sm_path).map(|mut d| d.any(|e| e.is_ok())).unwrap_or(false); + assert!(has_storage_files, "storage dir must contain RocksDB files"); + assert!(has_sm_files, "state machine dir must contain RocksDB files"); + } +} + +/// Tests for `run_custom` β€” the custom SE/SM entry point for standalone mode. +#[cfg(all(test, feature = "rocksdb"))] +mod run_custom_tests { + use std::sync::Arc; + use std::time::Duration; + + use serial_test::serial; + use tokio::sync::watch; + + use crate::RocksDBStateMachine; + use crate::RocksDBStorageEngine; + use crate::api::StandaloneEngine; + use crate::storage::TtlLease; + + /// Create RocksDB SE/SM with lease initialized. + fn create_rocksdb_se_and_sm( + temp_dir: &tempfile::TempDir + ) -> (Arc, Arc) { + let storage_path = temp_dir.path().join("storage"); + let sm_path = temp_dir.path().join("sm"); + std::fs::create_dir_all(&storage_path).unwrap(); + std::fs::create_dir_all(&sm_path).unwrap(); + + let storage = + Arc::new(RocksDBStorageEngine::new(&storage_path).expect("create storage engine")); + let mut sm = RocksDBStateMachine::new(&sm_path).expect("create state machine"); + sm.set_lease(Arc::new(TtlLease::new(Default::default()))); + let sm = Arc::new(sm); + + (storage, sm) + } + + /// Write a minimal valid config file for a single-node cluster. + fn write_config( + temp_dir: &tempfile::TempDir, + filename: &str, + node_id: u32, + ) -> std::path::PathBuf { + let config_path = temp_dir.path().join(filename); + let data_dir = temp_dir.path().join("data"); + std::fs::write( + &config_path, + format!( + concat!( + "[cluster]\nnode_id = {node_id}\ndb_root_dir = \"{data_dir}\"\n\n", + "[cluster.rpc]\nlisten_addr = \"127.0.0.1:19733\"\n\n", + "[raft]\nheartbeat_idle_flush_interval_ms = 500\n", + "election_timeout_min_ms = 1500\nelection_timeout_max_ms = 3000\n" + ), + node_id = node_id, + data_dir = data_dir.display(), + ), + ) + .expect("write config"); + config_path + } + + /// Shutdown helper: sleep, send signal, assert clean exit. + async fn shutdown_and_assert( + shutdown_tx: watch::Sender<()>, + handle: tokio::task::JoinHandle>, + sleep_ms: u64, + test_name: &str, + ) { + tokio::time::sleep(Duration::from_millis(sleep_ms)).await; + shutdown_tx.send(()).ok(); + + let result = tokio::time::timeout(Duration::from_secs(10), handle) + .await + .expect("server must stop within timeout") + .expect("server task must not panic"); + assert!( + result.is_ok(), + "{test_name} should succeed: {:?}", + result.err() + ); + } + + // ── run_custom tests ────────────────────────────────────────────────── + + /// `run_custom` with a valid config file: server starts, shuts down cleanly. + #[tokio::test] + #[serial] + async fn test_run_custom_with_valid_config_path_starts_and_stops() { + let temp_dir = tempfile::tempdir().expect("tempdir"); + let (storage, sm) = create_rocksdb_se_and_sm(&temp_dir); + let config_path = write_config(&temp_dir, "valid.toml", 1); + + let (shutdown_tx, shutdown_rx) = watch::channel(()); + let config_path_str = config_path.to_str().unwrap().to_string(); + let handle = tokio::spawn(StandaloneEngine::run_custom( + storage, + sm, + shutdown_rx, + Some(config_path_str), + )); + shutdown_and_assert(shutdown_tx, handle, 200, "run_custom with valid config").await; + } + + /// `run_custom` without a config file uses default config β€” must still work. + #[tokio::test] + #[serial] + async fn test_run_custom_without_config_path_uses_defaults() { + // CONFIG_PATH must be cleared so RaftNodeConfig::new() doesn't pick up + // an env-configured file and fail. + unsafe { std::env::remove_var("CONFIG_PATH") }; + + let temp_dir = tempfile::tempdir().expect("tempdir"); + // Use a persistent path so default /tmp/db gets a real dir + let (storage, sm) = create_rocksdb_se_and_sm(&temp_dir); + // Override db_root_dir via env so default config points to our temp dir + unsafe { + std::env::set_var( + "RAFT__CLUSTER__DB_ROOT_DIR", + temp_dir.path().join("db").to_str().unwrap(), + ); + } + + let (shutdown_tx, shutdown_rx) = watch::channel(()); + let handle = tokio::spawn(StandaloneEngine::run_custom::< + RocksDBStorageEngine, + RocksDBStateMachine, + >(storage, sm, shutdown_rx, None::<&str>)); + + shutdown_and_assert(shutdown_tx, handle, 300, "run_custom without config").await; + + unsafe { std::env::remove_var("RAFT__CLUSTER__DB_ROOT_DIR") }; + } + + /// `run_custom` with a nonexistent config file path must return an error. + #[tokio::test] + #[serial] + async fn test_run_custom_with_nonexistent_config_returns_error() { + let temp_dir = tempfile::tempdir().expect("tempdir"); + let (storage, sm) = create_rocksdb_se_and_sm(&temp_dir); + let (_shutdown_tx, shutdown_rx) = watch::channel(()); + + let result = StandaloneEngine::run_custom( + storage, + sm, + shutdown_rx, + Some("/nonexistent/path/config.toml"), + ) + .await; + assert!( + result.is_err(), + "run_custom with nonexistent config must return error" + ); + } + + /// `run_custom` with config containing `node_id = 0` must fail validation. + #[tokio::test] + #[serial] + async fn test_run_custom_with_invalid_config_returns_error() { + let temp_dir = tempfile::tempdir().expect("tempdir"); + let (storage, sm) = create_rocksdb_se_and_sm(&temp_dir); + let config_path = write_config(&temp_dir, "invalid.toml", 0); + let (_shutdown_tx, shutdown_rx) = watch::channel(()); + + let config_path_str = config_path.to_str().unwrap().to_string(); + let result = + StandaloneEngine::run_custom(storage, sm, shutdown_rx, Some(config_path_str)).await; + assert!( + result.is_err(), + "run_custom with node_id=0 must return validation error" + ); + } +} + /// Tests for unified RocksDB path (`unified_db = true`) in `run_with()` and `run()`. /// /// All existing tests use configs without a `[storage]` section, so `unified_db` diff --git a/d-engine-server/src/api/types.rs b/d-engine-server/src/api/types.rs new file mode 100644 index 00000000..abcf20a7 --- /dev/null +++ b/d-engine-server/src/api/types.rs @@ -0,0 +1,118 @@ +//! Server-layer native types for client write requests. +//! +//! [`WriteOperation`] is the working type used by [`EmbeddedClient`] and +//! [`StandaloneServer`] before encoding. The server converts it to proto +//! `WriteCommand` bytes, which core receives as opaque [`bytes::Bytes`] β€” +//! core is unaware of KV semantics. +//! +//! [`BatchOp`] is defined in `d-engine-core::command` alongside +//! `Command::Batch` and re-used here to keep the two in sync. +//! +//! # Type flow (write path) +//! +//! ```text +//! WriteOperation β†’ WriteCommand (proto) β†’ Bytes β†’ core (opaque) +//! ``` + +use bytes::Bytes; +use d_engine_core::BatchOp; +use d_engine_proto::client::{ + WriteCommand, + write_command::{ + Batch, BatchOp as ProtoBatchOp, CompareAndSwap, Delete, Insert, Operation, batch_op, + }, +}; + +/// Decoded write operation β€” the unit submitted by a client. +/// +/// Mirrors proto `WriteCommand` in shape but carries no prost annotations. +/// Core owns the serialization to Raft log bytes (`WriteOperation β†’ proto::WriteCommand β†’ bytes`); +/// transport adapters work with this native type only. + +#[derive(Debug, Clone, PartialEq)] +pub enum WriteOperation { + Insert { + key: Bytes, + value: Bytes, + /// `None` = no expiration. Proto encodes this as `ttl_secs = 0`. + ttl_secs: Option, + }, + Delete { + key: Bytes, + }, + CompareAndSwap { + key: Bytes, + /// `None` means the key must not exist for the swap to succeed. + expected: Option, + new_value: Bytes, + }, + + Batch { + ops: Vec, + }, +} + +/// Serializes a native write operation to proto wire format for Raft log storage. +/// +/// Symmetric counterpart to `TryFrom for Command` (decode direction). +/// Single coupling point on the write path β€” when a new `WriteOperation` variant is +/// added, the non-exhaustive `match` forces an update here at compile time. +/// +/// Infallible: every `WriteOperation` variant has a direct proto equivalent; +/// the Rust type system guarantees no invalid state can reach this conversion. +// encode: native β†’ proto (infallible) +impl From for WriteCommand { + fn from(op: WriteOperation) -> Self { + match op { + WriteOperation::Insert { + key, + value, + ttl_secs, + } => WriteCommand { + operation: Some(Operation::Insert(Insert { + key, + value, + ttl_secs: ttl_secs.unwrap_or(0), + })), + }, + WriteOperation::Delete { key } => WriteCommand { + operation: Some(Operation::Delete(Delete { key })), + }, + WriteOperation::CompareAndSwap { + key, + expected, + new_value, + } => WriteCommand { + operation: Some(Operation::CompareAndSwap(CompareAndSwap { + key, + expected_value: expected, + new_value, + })), + }, + WriteOperation::Batch { ops } => { + let proto_ops = ops + .into_iter() + .map(|op| match op { + BatchOp::Insert { key, value } => ProtoBatchOp { + op: Some(batch_op::Op::Insert(Insert { + key, + value, + ttl_secs: 0, + })), + }, + BatchOp::Delete { key } => ProtoBatchOp { + op: Some(batch_op::Op::Delete(Delete { key })), + }, + }) + .collect(); + WriteCommand { + operation: Some(Operation::Batch(Batch { ops: proto_ops })), + } + } + } + } +} + +#[cfg(test)] +#[path = "types_test.rs"] +mod tests; diff --git a/d-engine-server/src/api/types_test.rs b/d-engine-server/src/api/types_test.rs new file mode 100644 index 00000000..d9fa8f86 --- /dev/null +++ b/d-engine-server/src/api/types_test.rs @@ -0,0 +1,126 @@ +use bytes::Bytes; +use d_engine_core::BatchOp; +use d_engine_proto::client::WriteCommand; +use d_engine_proto::client::write_command::Operation; + +use super::WriteOperation; + +/// Verify Insert with TTL converts correctly to proto WriteCommand. +#[test] +fn test_write_operation_insert_with_ttl() { + let op = WriteOperation::Insert { + key: Bytes::from("k"), + value: Bytes::from("v"), + ttl_secs: Some(60), + }; + let cmd = WriteCommand::from(op); + match cmd.operation { + Some(Operation::Insert(ins)) => { + assert_eq!(ins.key, Bytes::from("k")); + assert_eq!(ins.value, Bytes::from("v")); + assert_eq!(ins.ttl_secs, 60); + } + other => panic!("expected Insert, got {:?}", other), + } +} + +/// Verify Insert with no TTL maps ttl_secs=None β†’ proto ttl_secs=0. +#[test] +fn test_write_operation_insert_no_ttl_maps_to_zero() { + let op = WriteOperation::Insert { + key: Bytes::from("k"), + value: Bytes::from("v"), + ttl_secs: None, + }; + let cmd = WriteCommand::from(op); + match cmd.operation { + Some(Operation::Insert(ins)) => assert_eq!(ins.ttl_secs, 0), + other => panic!("expected Insert, got {:?}", other), + } +} + +/// Verify Delete converts correctly to proto WriteCommand. +#[test] +fn test_write_operation_delete() { + let op = WriteOperation::Delete { + key: Bytes::from("del-key"), + }; + let cmd = WriteCommand::from(op); + match cmd.operation { + Some(Operation::Delete(del)) => assert_eq!(del.key, Bytes::from("del-key")), + other => panic!("expected Delete, got {:?}", other), + } +} + +/// Verify CAS with expected value converts correctly. +#[test] +fn test_write_operation_cas_with_expected() { + let op = WriteOperation::CompareAndSwap { + key: Bytes::from("k"), + expected: Some(Bytes::from("old")), + new_value: Bytes::from("new"), + }; + let cmd = WriteCommand::from(op); + match cmd.operation { + Some(Operation::CompareAndSwap(cas)) => { + assert_eq!(cas.key, Bytes::from("k")); + assert_eq!(cas.expected_value, Some(Bytes::from("old"))); + assert_eq!(cas.new_value, Bytes::from("new")); + } + other => panic!("expected CompareAndSwap, got {:?}", other), + } +} + +/// CAS with expected=None maps to expected_value=None (key-must-not-exist semantics). +#[test] +fn test_write_operation_cas_key_must_not_exist() { + let op = WriteOperation::CompareAndSwap { + key: Bytes::from("k"), + expected: None, + new_value: Bytes::from("new"), + }; + let cmd = WriteCommand::from(op); + match cmd.operation { + Some(Operation::CompareAndSwap(cas)) => assert!(cas.expected_value.is_none()), + other => panic!("expected CompareAndSwap, got {:?}", other), + } +} + +/// Verify Batch with mixed Insert/Delete ops converts correctly to proto WriteCommand. +#[test] +fn test_write_operation_batch_mixed_ops() { + use d_engine_proto::client::write_command::batch_op; + + let op = WriteOperation::Batch { + ops: vec![ + BatchOp::Insert { + key: Bytes::from("k1"), + value: Bytes::from("v1"), + }, + BatchOp::Delete { + key: Bytes::from("k2"), + }, + ], + }; + let cmd = WriteCommand::from(op); + match cmd.operation { + Some(Operation::Batch(b)) => { + assert_eq!(b.ops.len(), 2); + match &b.ops[0].op { + Some(batch_op::Op::Insert(ins)) => { + assert_eq!(ins.key, Bytes::from("k1")); + assert_eq!(ins.value, Bytes::from("v1")); + assert_eq!(ins.ttl_secs, 0); + } + other => panic!("expected Insert, got {:?}", other), + } + match &b.ops[1].op { + Some(batch_op::Op::Delete(del)) => { + assert_eq!(del.key, Bytes::from("k2")); + } + other => panic!("expected Delete, got {:?}", other), + } + } + other => panic!("expected Batch, got {:?}", other), + } +} diff --git a/d-engine-server/src/lib.rs b/d-engine-server/src/lib.rs index 7967bd48..8da701b8 100644 --- a/d-engine-server/src/lib.rs +++ b/d-engine-server/src/lib.rs @@ -4,10 +4,10 @@ //! //! ## When to use this crate directly //! -//! - βœ… Embedding server in a larger Rust application -//! - βœ… Need programmatic access to server APIs -//! - βœ… Building custom tooling around d-engine -//! - βœ… Already have your own client implementation +//! - Embedding server in a larger Rust application +//! - Need programmatic access to server APIs +//! - Building custom tooling around d-engine +//! - Already have your own client implementation //! //! ## When to use `d-engine` instead //! @@ -143,6 +143,11 @@ pub use d_engine_core::Result; pub use d_engine_core::ScanResult; /// Storage-specific error type pub use d_engine_core::StorageError; +// Raft Node Config +pub use d_engine_core::RaftNodeConfig; +// Batch operation variants (Insert, Delete) used in [`Command::Batch`]. +pub use d_engine_core::BatchOp; + // Internal types required by storage implementations β€” not part of user API #[doc(hidden)] pub use d_engine_core::HardState; diff --git a/d-engine-server/src/node/builder.rs b/d-engine-server/src/node/builder.rs index 0bdc1e90..b7ba9d3f 100644 --- a/d-engine-server/src/node/builder.rs +++ b/d-engine-server/src/node/builder.rs @@ -28,10 +28,6 @@ //! - **Configuration Loading**: Supports loading cluster configuration from file or in-memory //! config. -use std::fmt::Debug; -use std::sync::Arc; -use std::sync::atomic::AtomicBool; - use crate::read_actor::run_read_actor; use d_engine_core::ClusterConfig; use d_engine_core::CommitHandler; @@ -67,6 +63,9 @@ use d_engine_core::learner_state::LearnerState; use d_engine_core::watch::WatchDispatcher; #[cfg(feature = "watch")] use d_engine_core::watch::WatchRegistry; +use std::fmt::Debug; +use std::sync::Arc; +use std::sync::atomic::AtomicBool; use tokio::sync::Mutex; use tokio::sync::mpsc; use tokio::sync::watch; diff --git a/d-engine-server/src/proto_convert.rs b/d-engine-server/src/proto_convert.rs index dbb00f05..346c48a0 100644 --- a/d-engine-server/src/proto_convert.rs +++ b/d-engine-server/src/proto_convert.rs @@ -14,54 +14,30 @@ //! - `Bytes` fields are moved (refcount unchanged, zero allocation) //! - `ErrorCode` conversion uses exhaustive match β€” LLVM optimises to identity; //! a missing variant is a compile error (correctness guarantee) - +use crate::api::types::WriteOperation; +use bytes::Bytes; use d_engine_core::client::{ ClientReadRequest, ClientResponse, ClientResponsePayload, ClientWriteRequest, ErrorCode, - KvEntry, LeaderHint, ReadResults, WriteOperation, WriteResult, + KvEntry, LeaderHint, ReadResults, WriteResult, }; use d_engine_core::config::ReadConsistencyPolicy; -use d_engine_proto::client::write_command::Operation; use d_engine_proto::client::{ - self as proto_client, ClientResult, ReadResults as ProtoReadResults, + self as proto_client, ClientResult, ReadResults as ProtoReadResults, WriteCommand, WriteResult as ProtoWriteResult, }; use d_engine_proto::error::{ErrorCode as ProtoErrorCode, ErrorMetadata}; +use prost::Message; -// ─── proto β†’ core ───────────────────────────────────────────────────────────── - -/// Convert a proto `WriteCommand` to a core `WriteOperation`. -/// -/// Panics if `wc.operation` is `None`. Callers must validate the nested -/// operation field before calling β€” the gRPC handler does this at the network -/// boundary via an explicit `invalid_argument` check. #[inline] -pub(crate) fn write_command_to_op(wc: proto_client::WriteCommand) -> WriteOperation { - match wc.operation { - Some(Operation::Insert(i)) => WriteOperation::Insert { - key: i.key, - value: i.value, - // Proto convention: 0 == no expiration β†’ None in core - ttl_secs: if i.ttl_secs == 0 { - None - } else { - Some(i.ttl_secs) - }, - }, - Some(Operation::Delete(d)) => WriteOperation::Delete { key: d.key }, - Some(Operation::CompareAndSwap(c)) => WriteOperation::CompareAndSwap { - key: c.key, - expected: c.expected_value, - new_value: c.new_value, - }, - None => unreachable!("WriteCommand must have an operation"), - } +pub(crate) fn write_op_to_bytes(op: WriteOperation) -> Bytes { + Bytes::from(WriteCommand::from(op).encode_to_vec()) } #[inline] pub(crate) fn to_core_write_req(req: proto_client::ClientWriteRequest) -> ClientWriteRequest { ClientWriteRequest { client_id: req.client_id, - command: req.command.map(write_command_to_op), + command: req.command.map(|wc| Bytes::from(wc.encode_to_vec())), } } diff --git a/d-engine-server/src/proto_convert_test.rs b/d-engine-server/src/proto_convert_test.rs index aa6e3d67..6290f266 100644 --- a/d-engine-server/src/proto_convert_test.rs +++ b/d-engine-server/src/proto_convert_test.rs @@ -1,107 +1,19 @@ use bytes::Bytes; use d_engine_core::client::{ ClientReadRequest, ClientResponse, ClientResponsePayload, ClientWriteRequest, ErrorCode, - KvEntry, LeaderHint, ReadResults, WriteOperation, WriteResult, + KvEntry, LeaderHint, ReadResults, WriteResult, }; use d_engine_core::config::ReadConsistencyPolicy; use d_engine_proto::client as proto_client; -use d_engine_proto::client::write_command::{CompareAndSwap, Delete, Insert, Operation}; +use d_engine_proto::client::WriteCommand; +use d_engine_proto::client::write_command::{Insert, Operation}; use d_engine_proto::error::ErrorCode as ProtoErrorCode; +use prost::Message; use crate::proto_convert::{ core_error_to_proto, proto_error_to_core, to_core_read_req, to_core_response, - to_core_write_req, to_proto_response, write_command_to_op, + to_core_write_req, to_proto_response, }; - -// ─── write_command_to_op ────────────────────────────────────────────────────── - -#[test] -fn test_write_command_insert_with_ttl_converts_correctly() { - let wc = proto_client::WriteCommand { - operation: Some(Operation::Insert(Insert { - key: Bytes::from("k"), - value: Bytes::from("v"), - ttl_secs: 60, - })), - }; - let op = write_command_to_op(wc); - assert_eq!( - op, - WriteOperation::Insert { - key: Bytes::from("k"), - value: Bytes::from("v"), - ttl_secs: Some(60), - } - ); -} - -/// Proto convention: ttl_secs == 0 means "no expiration". Must become None in core. -#[test] -fn test_write_command_insert_ttl_zero_becomes_none() { - let wc = proto_client::WriteCommand { - operation: Some(Operation::Insert(Insert { - key: Bytes::from("k"), - value: Bytes::from("v"), - ttl_secs: 0, - })), - }; - let op = write_command_to_op(wc); - assert!(matches!(op, WriteOperation::Insert { ttl_secs: None, .. })); -} - -#[test] -fn test_write_command_delete_converts_correctly() { - let wc = proto_client::WriteCommand { - operation: Some(Operation::Delete(Delete { - key: Bytes::from("del"), - })), - }; - let op = write_command_to_op(wc); - assert_eq!( - op, - WriteOperation::Delete { - key: Bytes::from("del") - } - ); -} - -#[test] -fn test_write_command_cas_with_expected_value_converts_correctly() { - let wc = proto_client::WriteCommand { - operation: Some(Operation::CompareAndSwap(CompareAndSwap { - key: Bytes::from("k"), - expected_value: Some(Bytes::from("old")), - new_value: Bytes::from("new"), - })), - }; - let op = write_command_to_op(wc); - assert_eq!( - op, - WriteOperation::CompareAndSwap { - key: Bytes::from("k"), - expected: Some(Bytes::from("old")), - new_value: Bytes::from("new"), - } - ); -} - -/// CAS with expected_value=None means "key must not exist" β€” distinct semantic. -#[test] -fn test_write_command_cas_key_must_not_exist_when_expected_none() { - let wc = proto_client::WriteCommand { - operation: Some(Operation::CompareAndSwap(CompareAndSwap { - key: Bytes::from("k"), - expected_value: None, - new_value: Bytes::from("new"), - })), - }; - let op = write_command_to_op(wc); - assert!(matches!( - op, - WriteOperation::CompareAndSwap { expected: None, .. } - )); -} - // ─── to_core_write_req ──────────────────────────────────────────────────────── #[test] @@ -118,10 +30,15 @@ fn test_to_core_write_req_insert_roundtrip() { }; let core_req = to_core_write_req(proto_req); assert_eq!(core_req.client_id, 7); - assert!(matches!( - core_req.command, - Some(WriteOperation::Insert { ttl_secs: None, .. }) - )); + let bytes = core_req.command.expect("command must be Some"); + let decoded = WriteCommand::decode(bytes).expect("must decode to WriteCommand"); + assert!( + matches!( + decoded.operation, + Some(Operation::Insert(Insert { ttl_secs: 0, .. })) + ), + "decoded command must be an Insert operation" + ); } #[test] diff --git a/d-engine-server/src/storage/adaptors/file/file_state_machine.rs b/d-engine-server/src/storage/adaptors/file/file_state_machine.rs index c5f15d11..2fca196c 100644 --- a/d-engine-server/src/storage/adaptors/file/file_state_machine.rs +++ b/d-engine-server/src/storage/adaptors/file/file_state_machine.rs @@ -224,6 +224,33 @@ fn encode_wal_entry( buf.extend_from_slice(&0u64.to_be_bytes()); // expire_at = 0 } } + Command::Batch { ops } => { + for (i, op) in ops.iter().enumerate() { + // After the first op, each subsequent op needs its own index+term + // prefix so replay_wal can parse independent records. + if i > 0 { + buf.extend_from_slice(&entry.index.to_be_bytes()); + buf.extend_from_slice(&entry.term.to_be_bytes()); + } + match op { + d_engine_core::BatchOp::Insert { key, value } => { + buf.push(WalOpCode::Insert as u8); + buf.extend_from_slice(&(key.len() as u64).to_be_bytes()); + buf.extend_from_slice(key); + buf.extend_from_slice(&(value.len() as u64).to_be_bytes()); + buf.extend_from_slice(value); + buf.extend_from_slice(&0u64.to_be_bytes()); // no TTL in batch inserts + } + d_engine_core::BatchOp::Delete { key } => { + buf.push(WalOpCode::Delete as u8); + buf.extend_from_slice(&(key.len() as u64).to_be_bytes()); + buf.extend_from_slice(key); + buf.extend_from_slice(&0u64.to_be_bytes()); // val_len = 0 + buf.extend_from_slice(&0u64.to_be_bytes()); // expire_at = 0 + } + } + } + } } } @@ -1058,14 +1085,6 @@ impl StateMachine for FileStateMachine { Ok(data.get(key_buffer).map(|(value, _)| value.clone())) } - fn entry_term( - &self, - entry_id: u64, - ) -> Option { - let data = self.data.read(); - data.values().find(|(_, index)| *index == entry_id).map(|(_, term)| *term) - } - /// Thread-safe: called serially by single-task CommitHandler async fn apply_chunk( &self, @@ -1158,6 +1177,17 @@ impl StateMachine for FileStateMachine { } success } + Command::Batch { ops } => { + ops.iter().for_each(|op| match op { + d_engine_core::BatchOp::Insert { key, value } => { + delta.insert(key.clone(), Some((value.clone(), entry.term))); + } + d_engine_core::BatchOp::Delete { key } => { + delta.insert(key.clone(), None); + } + }); + false + } Command::Noop => false, }; encode_wal_entry(&mut wal_buf, entry, success); @@ -1232,6 +1262,21 @@ impl StateMachine for FileStateMachine { data.insert(key.clone(), (new_value.clone(), entry.term)); } } + + Command::Batch { ops } => { + ops.iter().for_each(|op| match op { + d_engine_core::BatchOp::Insert { key, value } => { + data.insert(key.clone(), (value.clone(), entry.term)); + } + d_engine_core::BatchOp::Delete { key } => { + data.remove(key.as_ref()); + if let Some(ref lease) = self.lease { + lease.unregister(key); + } + } + }); + results.push(ApplyResult::success(entry.index)); + } } } } // Lock released immediately - no awaits inside! diff --git a/d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs b/d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs index 00961385..fd585dfd 100644 --- a/d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs +++ b/d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs @@ -776,15 +776,6 @@ impl StateMachine for RocksDBStateMachine { }) } - fn entry_term( - &self, - _entry_id: u64, - ) -> Option { - // In RocksDB state machine, we don't store term per key. This method is not typically used. - // If needed, we might need to change the design to store term along with value. - None - } - /// Thread-safe: called serially by single-task CommitHandler #[instrument(skip(self, chunk))] async fn apply_chunk( @@ -882,6 +873,21 @@ impl StateMachine for RocksDBStateMachine { cas_success ); } + + Command::Batch { ops } => { + ops.iter().for_each(|op| match op { + d_engine_core::BatchOp::Insert { key, value } => { + batch.put_cf(&cf, key, value) + } + d_engine_core::BatchOp::Delete { key } => { + batch.delete_cf(&cf, key); + if let Some(ref lease) = self.lease { + lease.unregister(key); + } + } + }); + results.push(ApplyResult::success(entry.index)); + } } } @@ -949,7 +955,6 @@ impl StateMachine for RocksDBStateMachine { self.persist_snapshot_metadata() } - #[instrument(skip(self))] async fn apply_snapshot_from_file( &self, metadata: &SnapshotMetadata, @@ -976,7 +981,6 @@ impl StateMachine for RocksDBStateMachine { result } - #[instrument(skip(self))] async fn generate_snapshot_data( &self, new_snapshot_dir: std::path::PathBuf, @@ -1068,7 +1072,6 @@ impl StateMachine for RocksDBStateMachine { self.flush() } - #[instrument(skip(self))] async fn reset(&self) -> Result<(), Error> { self.with_db(|db| { let cf = db.cf_handle(STATE_MACHINE_CF).ok_or_else(|| { diff --git a/d-engine-server/src/storage/adaptors/rocksdb/rocksdb_storage_engine.rs b/d-engine-server/src/storage/adaptors/rocksdb/rocksdb_storage_engine.rs index 5bcbd42d..51db6ce7 100644 --- a/d-engine-server/src/storage/adaptors/rocksdb/rocksdb_storage_engine.rs +++ b/d-engine-server/src/storage/adaptors/rocksdb/rocksdb_storage_engine.rs @@ -212,7 +212,6 @@ impl LogStore for RocksDBLogStore { Ok(()) } - #[instrument(skip(self))] async fn entry( &self, index: u64, @@ -231,7 +230,6 @@ impl LogStore for RocksDBLogStore { } } - #[instrument(skip(self))] fn get_entries( &self, range: RangeInclusive, @@ -271,7 +269,6 @@ impl LogStore for RocksDBLogStore { Ok(entries) } - #[instrument(skip(self))] async fn purge( &self, cutoff_index: LogId, @@ -320,7 +317,6 @@ impl LogStore for RocksDBLogStore { Ok(()) } - #[instrument(skip(self))] async fn truncate( &self, from_index: u64, @@ -434,7 +430,6 @@ impl LogStore for RocksDBLogStore { } } - #[instrument(skip(self))] fn flush(&self) -> Result<(), Error> { // WAL fsync is sufficient for crash-safety: data in WAL can be replayed on restart. // memtable flush is RocksDB's internal concern β€” triggered automatically in background. @@ -447,12 +442,10 @@ impl LogStore for RocksDBLogStore { Ok(()) } - #[instrument(skip(self))] async fn flush_async(&self) -> Result<(), Error> { self.flush() } - #[instrument(skip(self))] async fn reset(&self) -> Result<(), Error> { let cf = self .db @@ -473,7 +466,6 @@ impl LogStore for RocksDBLogStore { Ok(()) } - #[instrument(skip(self))] fn last_index(&self) -> u64 { self.last_index.load(Ordering::SeqCst) } @@ -519,7 +511,6 @@ impl MetaStore for RocksDBMetaStore { Ok(()) } - #[instrument(skip(self))] fn load_hard_state(&self) -> Result, Error> { let cf = self .db @@ -539,7 +530,6 @@ impl MetaStore for RocksDBMetaStore { } } - #[instrument(skip(self))] fn flush(&self) -> Result<(), Error> { // WAL fsync is sufficient: HardState in WAL survives crashes. // memtable flush is RocksDB's internal concern β€” triggered automatically in background. @@ -549,7 +539,6 @@ impl MetaStore for RocksDBMetaStore { Ok(()) } - #[instrument(skip(self))] async fn flush_async(&self) -> Result<(), Error> { self.flush() } diff --git a/d-engine-server/src/storage/lease.rs b/d-engine-server/src/storage/lease.rs index 0fbd5f4e..caf0aac8 100644 --- a/d-engine-server/src/storage/lease.rs +++ b/d-engine-server/src/storage/lease.rs @@ -62,7 +62,7 @@ pub struct TtlLease { /// Apply counter for piggyback cleanup frequency apply_counter: AtomicU64, - /// βœ… Single index: key β†’ expiration_time (completely lock-free) + /// Single index: key β†’ expiration_time (completely lock-free) /// - Register/Unregister: O(1), single shard lock /// - Cleanup: O(N) iteration with shard read locks key_to_expiry: DashMap, diff --git a/d-engine-server/src/test_utils/mock/mock_node_builder.rs b/d-engine-server/src/test_utils/mock/mock_node_builder.rs index 0adec374..95fc6ea1 100644 --- a/d-engine-server/src/test_utils/mock/mock_node_builder.rs +++ b/d-engine-server/src/test_utils/mock/mock_node_builder.rs @@ -1,9 +1,10 @@ //! Mock node builder for fluent configuration of test Raft instances -use std::path::Path; -use std::sync::Arc; -use std::sync::atomic::AtomicBool; - +use super::MockTypeConfig; +use crate::Node; +use crate::membership::MembershipSnapshot; +use crate::network::grpc; +use crate::node::LeaderNotifier; use bytes::Bytes; use d_engine_core::ElectionConfig; use d_engine_core::InboundEvent; @@ -30,18 +31,15 @@ use d_engine_core::follower_state::FollowerState; use d_engine_core::mock_membership as mock_membership_fn; use d_engine_proto::common::LogId; use d_engine_proto::server::cluster::ClusterMembership; +use std::path::Path; +use std::sync::Arc; +use std::sync::atomic::AtomicBool; use tokio::sync::Mutex; use tokio::sync::mpsc; use tokio::sync::watch; use tracing::error; use tracing::trace; -use super::MockTypeConfig; -use crate::Node; -use crate::membership::MembershipSnapshot; -use crate::network::grpc; -use crate::node::LeaderNotifier; - /// Builder for constructing mock Raft components with customizable defaults /// /// This builder provides a fluent API for creating test fixtures with @@ -574,7 +572,6 @@ pub(crate) fn mock_state_machine() -> MockStateMachine { mock.expect_is_running().returning(|| true); mock.expect_get().returning(|_| Ok(None)); - mock.expect_entry_term().returning(|_| None); mock.expect_apply_chunk().returning(|_| Ok(vec![])); mock.expect_len().returning(|| 0); diff --git a/d-engine-server/tests/snapshot_and_recovery/mod.rs b/d-engine-server/tests/snapshot_and_recovery/mod.rs index 08bb0e6c..40f35767 100644 --- a/d-engine-server/tests/snapshot_and_recovery/mod.rs +++ b/d-engine-server/tests/snapshot_and_recovery/mod.rs @@ -13,6 +13,7 @@ mod snapshot_concurrent_replication_embedded; mod snapshot_concurrent_writes_embedded; +mod snapshot_correctness_after_install_embedded; mod snapshot_follower_generation_embedded; mod snapshot_generation_standalone; mod snapshot_interrupted_transfer_embedded; diff --git a/d-engine-server/tests/snapshot_and_recovery/snapshot_concurrent_writes_embedded.rs b/d-engine-server/tests/snapshot_and_recovery/snapshot_concurrent_writes_embedded.rs index b3834709..168541a1 100644 --- a/d-engine-server/tests/snapshot_and_recovery/snapshot_concurrent_writes_embedded.rs +++ b/d-engine-server/tests/snapshot_and_recovery/snapshot_concurrent_writes_embedded.rs @@ -48,11 +48,11 @@ use crate::common::get_available_ports; /// /// ## Expected Results /// -/// βœ… All 145 writes succeed (no blocking during snapshot) -/// βœ… Leader can read all 145 entries after completion -/// βœ… Followers replicate all 145 entries correctly -/// βœ… Snapshot contains entries 1-90 (100 - retained_log_entries) -/// βœ… Log entries 1-90 are purged after snapshot +/// All 145 writes succeed (no blocking during snapshot) +/// Leader can read all 145 entries after completion +/// Followers replicate all 145 entries correctly +/// Snapshot label last_included == last_applied (all 100 entries, no subtraction) +/// Log entries before (last_applied - retained_log_entries) are purged after snapshot /// /// ## Implementation Details /// @@ -203,7 +203,7 @@ snapshots_dir = '{}' failed_writes.is_empty(), "All writes should succeed during snapshot generation. Failed writes: {failed_writes:?}" ); - info!("βœ… All 145 writes succeeded (snapshot did not block writes)"); + info!("All 145 writes succeeded (snapshot did not block writes)"); // Phase 4: Verify data integrity on Leader info!("Phase 4: Verifying all 145 entries readable on Leader"); @@ -219,7 +219,7 @@ snapshots_dir = '{}' assert_eq!(actual_value, expected_value, "Value mismatch for key {i}"); } - info!("βœ… All 145 entries verified on Leader"); + info!("All 145 entries verified on Leader"); // Phase 5: Verify data replicated to Followers info!("Phase 5: Verifying replication to Followers"); @@ -249,7 +249,7 @@ snapshots_dir = '{}' ); } } - info!("βœ… Data successfully replicated to all Followers"); + info!("Data successfully replicated to all Followers"); // Phase 6: Verify snapshot was generated and contains correct data info!("Phase 6: Verifying snapshot generation"); @@ -279,7 +279,7 @@ snapshots_dir = '{}' ); info!( - "βœ… Snapshot generated successfully: {} file(s) in {:?}", + "Snapshot generated successfully: {} file(s) in {:?}", snapshot_files.len(), leader_snapshot_dir ); diff --git a/d-engine-server/tests/snapshot_and_recovery/snapshot_correctness_after_install_embedded.rs b/d-engine-server/tests/snapshot_and_recovery/snapshot_correctness_after_install_embedded.rs new file mode 100644 index 00000000..846c0fbd --- /dev/null +++ b/d-engine-server/tests/snapshot_and_recovery/snapshot_correctness_after_install_embedded.rs @@ -0,0 +1,262 @@ +//! Snapshot correctness after install β€” embedded mode +//! +//! Validates the #418 fix end-to-end: +//! - snapshot `last_included` == `last_applied` (no subtraction) +//! - a reconnecting follower within the retained-log buffer recovers via AppendEntries +//! - all cluster data is consistent after recovery with no double-applied entries + +#![cfg(feature = "rocksdb")] + +use std::sync::Arc; +use std::time::Duration; + +use d_engine_server::DefaultEmbeddedEngine; +use d_engine_server::RocksDBUnifiedEngine; +use serial_test::serial; +use tracing::info; +use tracing_test::traced_test; + +use crate::common::get_available_ports; +use crate::common::wait_for_snapshot; + +/// Test: follower that reconnects within the retained-log buffer recovers via AppendEntries +/// and reaches full data consistency with no double-applied entries. +/// +/// ## Purpose +/// +/// The #418 fix ensures `snapshot.last_included == last_applied` (no subtraction by +/// `retained_log_entries`). This test exercises the downstream consequence: because the +/// snapshot label is truthful, a reconnecting follower knows exactly where it left off and +/// the leader can supply the missing entries from its retained log without sending a new +/// snapshot. +/// +/// Without the fix, `last_included` would be stamped lower than `last_applied`, causing +/// the follower to re-apply entries already reflected in the snapshot data β€” a silent data +/// corruption for non-idempotent operations. +/// +/// ## Test Flow +/// +/// 1. Start a 3-node cluster (`snapshot_threshold=100`, `retained_log_entries=30`). +/// 2. Write 120 entries β†’ snapshot fires at 100, purge up to index 70. +/// Leader retains entries 71..120 for lagging followers. +/// 3. Wait for snapshot to appear on the leader (confirms cluster is synced). +/// 4. Stop one non-leader follower (node C). +/// 5. Write 20 more entries (121..140) to the leader while C is offline. +/// C now lags by 20 entries; 20 < retained=30, so entries 121..140 remain in leader's log. +/// 6. Restart C with its persisted DB (state through index 120). +/// 7. Wait for C to catch up to all 140 entries. +/// +/// ## Expected Results +/// +/// C recovers via AppendEntries (lagging entries 121..140 are within the retained buffer) +/// Snapshot-only entries (purged from log) are readable on C β€” snapshot was correctly installed +/// All 140 entries present on C with correct values +/// No double-apply: each key holds exactly the value written once +#[tokio::test] +#[traced_test] +#[serial] +async fn test_follower_catchup_within_retained_buffer_and_data_consistency() +-> Result<(), Box> { + const SNAPSHOT_THRESHOLD: u64 = 100; + const RETAINED_LOGS: u64 = 30; + const INITIAL_ENTRIES: u64 = 120; + const CATCHUP_ENTRIES: u64 = 20; // must be < RETAINED_LOGS to stay in retained buffer + + let temp_dir = tempfile::tempdir()?; + let db_root_dir = temp_dir.path().join("db"); + let snapshots_dir = temp_dir.path().join("snapshots"); + + let mut port_guard = get_available_ports(3).await; + port_guard.release_listeners(); + let ports = port_guard.as_slice(); + + // Track config+db paths per node so we can restart any node later + let mut node_paths: Vec<(std::path::PathBuf, std::path::PathBuf)> = Vec::new(); + let mut engines: Vec> = Vec::new(); + + for node_id in 1u64..=3 { + let config = format!( + r#" +[cluster] +node_id = {node_id} +listen_address = '127.0.0.1:{}' +initial_cluster = [ + {{ id = 1, name = 'n1', address = '127.0.0.1:{}', role = 1, status = 3 }}, + {{ id = 2, name = 'n2', address = '127.0.0.1:{}', role = 1, status = 3 }}, + {{ id = 3, name = 'n3', address = '127.0.0.1:{}', role = 1, status = 3 }} +] +db_root_dir = '{}' + +[raft] +general_raft_timeout_duration_in_ms = 5000 + +[raft.snapshot] +max_log_entries_before_snapshot = {SNAPSHOT_THRESHOLD} +retained_log_entries = {RETAINED_LOGS} +snapshots_dir = '{}' +"#, + ports[node_id as usize - 1], + ports[0], + ports[1], + ports[2], + db_root_dir.join(format!("node{node_id}")).display(), + snapshots_dir.join(format!("node{node_id}")).display(), + ); + + let config_path = temp_dir.path().join(format!("node{node_id}.toml")); + tokio::fs::write(&config_path, &config).await?; + + let db_path = db_root_dir.join(format!("node{node_id}/db")); + tokio::fs::create_dir_all(&db_path).await?; + tokio::fs::create_dir_all(snapshots_dir.join(format!("node{node_id}"))).await?; + + node_paths.push((config_path.clone(), db_path.clone())); + + let (storage, sm) = RocksDBUnifiedEngine::open(&db_path)?; + let engine = DefaultEmbeddedEngine::start_custom( + Arc::new(storage), + Arc::new(sm), + Some(config_path.to_str().unwrap()), + ) + .await?; + engines.push(Some(engine)); + } + + let leader_info = engines[0].as_ref().unwrap().wait_ready(Duration::from_secs(15)).await?; + let leader_idx = engines + .iter() + .position(|e| e.as_ref().map(|e| e.node_id() == leader_info.leader_id).unwrap_or(false)) + .expect("leader must be one of the 3 engines"); + let leader_client = engines[leader_idx].as_ref().unwrap().client().clone(); + info!( + "Leader is node {} (engines idx {})", + leader_info.leader_id, leader_idx + ); + + // Phase 1: write INITIAL_ENTRIES to trigger snapshot and log purge + info!("Writing {INITIAL_ENTRIES} entries to trigger snapshot at {SNAPSHOT_THRESHOLD}"); + for i in 0..INITIAL_ENTRIES { + leader_client + .put( + format!("key_{i}").into_bytes(), + format!("value_{i}").into_bytes(), + ) + .await?; + } + + let leader_id = leader_info.leader_id as u64; + assert!( + wait_for_snapshot(&snapshots_dir, leader_id, Duration::from_secs(15)).await, + "Leader snapshot must exist after {INITIAL_ENTRIES} entries" + ); + info!( + "Leader snapshot ready β€” purge boundary at ~{}", + SNAPSHOT_THRESHOLD - RETAINED_LOGS + ); + + // Allow all followers to fully replicate before stopping one + tokio::time::sleep(Duration::from_secs(3)).await; + + // Phase 2: stop a non-leader follower (node C) + let follower_idx = engines + .iter() + .position(|e| e.as_ref().map(|e| !e.is_leader()).unwrap_or(false)) + .expect("must have at least one non-leader"); + let follower_node_id = engines[follower_idx].as_ref().unwrap().node_id(); + let (follower_config_path, follower_db_path) = node_paths[follower_idx].clone(); + info!("Stopping follower node {follower_node_id} (engines idx {follower_idx})"); + + let stopped = engines[follower_idx].take().unwrap(); + let _ = stopped.stop().await; + + // Phase 3: write CATCHUP_ENTRIES while follower is offline + // 20 < RETAINED_LOGS=30 β†’ leader keeps these entries in log for AppendEntries replay + let total_entries = INITIAL_ENTRIES + CATCHUP_ENTRIES; + info!( + "Writing {CATCHUP_ENTRIES} more entries while follower offline \ + (lag={CATCHUP_ENTRIES} < retained={RETAINED_LOGS} β†’ AppendEntries path on reconnect)" + ); + for i in INITIAL_ENTRIES..total_entries { + leader_client + .put( + format!("key_{i}").into_bytes(), + format!("value_{i}").into_bytes(), + ) + .await?; + } + + // Phase 4: restart follower with its persisted DB + info!("Restarting follower node {follower_node_id} from persisted DB"); + let (storage, sm) = RocksDBUnifiedEngine::open(&follower_db_path)?; + let restarted = DefaultEmbeddedEngine::start_custom( + Arc::new(storage), + Arc::new(sm), + Some(follower_config_path.to_str().unwrap()), + ) + .await?; + engines[follower_idx] = Some(restarted); + + // Phase 5: wait for follower to catch up + let last_key = format!("key_{}", total_entries - 1).into_bytes(); + let follower_engine = engines[follower_idx].as_ref().unwrap(); + let mut caught_up = false; + for _ in 0..30 { + if follower_engine + .client() + .get_eventual(last_key.clone()) + .await + .ok() + .flatten() + .is_some() + { + caught_up = true; + break; + } + tokio::time::sleep(Duration::from_secs(1)).await; + } + assert!( + caught_up, + "Follower node {follower_node_id} failed to catch up within 30 seconds" + ); + info!("Follower {follower_node_id} caught up to all {total_entries} entries"); + + // Phase 6: full data integrity check on the restarted follower + // + // Entries 0..(SNAPSHOT_THRESHOLD - RETAINED_LOGS) were purged from the log and exist + // only in the snapshot. If they are readable, the snapshot was correctly installed. + // AppendEntries alone cannot supply purged entries β€” this verifies snapshot install. + let snapshot_only_boundary = SNAPSHOT_THRESHOLD.saturating_sub(RETAINED_LOGS); + for i in [0, snapshot_only_boundary / 2, snapshot_only_boundary - 1] { + let actual = follower_engine + .client() + .get_eventual(format!("key_{i}").into_bytes()) + .await? + .unwrap_or_default(); + assert_eq!( + actual, + format!("value_{i}").into_bytes(), + "key_{i} is snapshot-only (log purged) β€” readable only if snapshot is intact" + ); + } + + // Entries written while follower was offline must be present (AppendEntries catchup) + for i in INITIAL_ENTRIES..total_entries { + let actual = follower_engine + .client() + .get_eventual(format!("key_{i}").into_bytes()) + .await? + .unwrap_or_default(); + assert_eq!( + actual, + format!("value_{i}").into_bytes(), + "key_{i} must be present after AppendEntries catchup (written while offline)" + ); + } + info!("All {total_entries} entries verified on restarted follower node {follower_node_id}"); + + for engine in engines.into_iter().flatten() { + let _ = engine.stop().await; + } + + Ok(()) +} diff --git a/d-engine-server/tests/snapshot_and_recovery/snapshot_generation_standalone.rs b/d-engine-server/tests/snapshot_and_recovery/snapshot_generation_standalone.rs index 1a82ad39..e4e2018f 100644 --- a/d-engine-server/tests/snapshot_and_recovery/snapshot_generation_standalone.rs +++ b/d-engine-server/tests/snapshot_and_recovery/snapshot_generation_standalone.rs @@ -88,10 +88,6 @@ async fn test_snapshot_scenario() -> Result<(), ClientApiError> { node_handles: Vec::new(), }; - // To maintain the last included index of the snapshot, because of the configure: - // retained_log_entries. e.g. if leader local raft log has 10 entries. but - // retained_log_entries=1 , then the last included index of the snapshot should be 9. - let mut snapshot_last_included_id: Option = None; for (i, port) in ports.iter().enumerate() { let node_id = (i + 1) as u64; let config = create_node_config( @@ -123,9 +119,6 @@ async fn test_snapshot_scenario() -> Result<(), ClientApiError> { node_config.raft.snapshot.snapshots_dir = PathBuf::from(format!("{SNAPSHOT_DIR}/{node_id}")); - //Dirty code: could leave it like this for now. - snapshot_last_included_id = - Some(last_log_id.saturating_sub(node_config.raft.snapshot.retained_log_entries)); let (graceful_tx, node_handle) = start_node(node_config, Some(state_machine), raft_log).await?; @@ -133,7 +126,6 @@ async fn test_snapshot_scenario() -> Result<(), ClientApiError> { ctx.graceful_txs.push(graceful_tx); ctx.node_handles.push(node_handle); } - let _last_included = snapshot_last_included_id.unwrap(); tokio::time::sleep(Duration::from_secs(WAIT_FOR_NODE_READY_IN_SEC)).await; diff --git a/d-engine/src/docs/quick-start-5min.md b/d-engine/src/docs/quick-start-5min.md index bdd8f261..d76e5a10 100644 --- a/d-engine/src/docs/quick-start-5min.md +++ b/d-engine/src/docs/quick-start-5min.md @@ -125,8 +125,8 @@ This one line: 2. Created `./data/single-node/db/` (single RocksDB instance) 3. Initialized 4 column families: `logs`, `meta`, `state_machine`, `state_machine_meta` 4. Built Raft node with node_id=1 -6. Spawned `node.run()` in background (Raft protocol) -7. Returned immediately (non-blocking) +5. Spawned `node.run()` in background (Raft protocol) +6. Returned immediately (non-blocking) ```rust,ignore engine.wait_ready(Duration::from_secs(5)).await? @@ -205,9 +205,16 @@ No manual `tokio::spawn()`, no leaked tasks. DefaultEmbeddedEngine::start(data_dir: impl AsRef) -> Result // Use explicit config file -DefaultEmbeddedEngine::start_with(config_path: &str) -> Result +DefaultEmbeddedEngine::start_with(config_path: impl AsRef) -> Result -// Advanced (custom storage + state machine β€” define your own TypeConfig) +// Start with programmatic config β€” no config file needed +EmbeddedEngine::::start_node( + config: RaftNodeConfig, + storage: Arc, + state_machine: Arc +) -> Result + +// Advanced (custom storage + state machine with optional config file) EmbeddedEngine::::start_custom( storage: Arc, state_machine: Arc, diff --git a/d-engine/src/docs/server_guide/customize-state-machine.md b/d-engine/src/docs/server_guide/customize-state-machine.md index 004ba578..b9898d87 100644 --- a/d-engine/src/docs/server_guide/customize-state-machine.md +++ b/d-engine/src/docs/server_guide/customize-state-machine.md @@ -55,11 +55,6 @@ impl StateMachine for CustomStateMachine { self.backend.get(key_buffer) } - fn entry_term(&self, entry_id: u64) -> Option { - // Return term for specific entry - Some(1) - } - async fn apply_chunk(&self, chunk: &[ApplyEntry]) -> Result, Error> { // The framework pre-decodes proto bytes β€” match on Command directly, no prost needed. let mut results = Vec::with_capacity(chunk.len()); @@ -84,6 +79,19 @@ impl StateMachine for CustomStateMachine { ApplyResult::failure(entry.index) } } + Command::Batch { ops } => { + for op in ops { + match op { + d_engine::BatchOp::Insert { key, value } => { + self.backend.put(key, value, None)?; + } + d_engine::BatchOp::Delete { key } => { + self.backend.delete(key)?; + } + } + } + ApplyResult::success(entry.index) + } Command::Noop => ApplyResult::success(entry.index), }; results.push(result); @@ -112,29 +120,28 @@ impl StateMachine for CustomStateMachine { ## 3. StateMachine API Reference -| Method | Purpose | Sync/Async | Criticality | -| ---------------------------------- | ---------------------------------- | ---------- | ----------- | -| `start()` | Initialize state machine service | Sync | High | -| `stop()` | Graceful shutdown (reversible) | Sync | High | -| `close_storage()` | Release exclusive OS resources (e.g. DB lock file). Default no-op β€” override only if needed. | Sync | Medium | -| `is_running()` | Check service status | Sync | Medium | -| `get()` | Read value by key | Sync | High | -| `entry_term()` | Get term for log index | Sync | Medium | -| `apply_chunk()` | Apply committed entries | Async | Critical | -| `len()` | Get entry count | Sync | Low | -| `is_empty()` | Check if empty | Sync | Low | -| `update_last_applied()` | Update applied index in memory | Sync | High | -| `last_applied()` | Get last applied index | Sync | High | -| `persist_last_applied()` | Persist applied index | Sync | High | -| `update_last_snapshot_metadata()` | Update snapshot metadata in memory | Sync | Medium | -| `snapshot_metadata()` | Get current snapshot metadata | Sync | Medium | -| `persist_last_snapshot_metadata()` | Persist snapshot metadata | Sync | Medium | -| `apply_snapshot_from_file()` | Replace state with snapshot | Async | Critical | -| `generate_snapshot_data()` | Create new snapshot | Async | High | -| `save_hard_state()` | Persist term/vote state | Sync | High | -| `flush()` | Sync writes to storage | Sync | High | -| `flush_async()` | Async flush | Async | High | -| `reset()` | Reset to initial state | Async | Medium | +| Method | Purpose | Sync/Async | Criticality | +| ---------------------------------- | -------------------------------------------------------------------------------------------- | ---------- | ----------- | +| `start()` | Initialize state machine service | Sync | High | +| `stop()` | Graceful shutdown (reversible) | Sync | High | +| `close_storage()` | Release exclusive OS resources (e.g. DB lock file). Default no-op β€” override only if needed. | Sync | Medium | +| `is_running()` | Check service status | Sync | Medium | +| `get()` | Read value by key | Sync | High | +| `apply_chunk()` | Apply committed entries | Async | Critical | +| `len()` | Get entry count | Sync | Low | +| `is_empty()` | Check if empty | Sync | Low | +| `update_last_applied()` | Update applied index in memory | Sync | High | +| `last_applied()` | Get last applied index | Sync | High | +| `persist_last_applied()` | Persist applied index | Sync | High | +| `update_last_snapshot_metadata()` | Update snapshot metadata in memory | Sync | Medium | +| `snapshot_metadata()` | Get current snapshot metadata | Sync | Medium | +| `persist_last_snapshot_metadata()` | Persist snapshot metadata | Sync | Medium | +| `apply_snapshot_from_file()` | Replace state with snapshot | Async | Critical | +| `generate_snapshot_data()` | Create new snapshot | Async | High | +| `save_hard_state()` | Persist term/vote state | Sync | High | +| `flush()` | Sync writes to storage | Sync | High | +| `flush_async()` | Async flush | Async | High | +| `reset()` | Reset to initial state | Async | Medium | ## 4. Testing Your Implementation diff --git a/examples/sled-cluster/src/lib.rs b/examples/sled-cluster/src/lib.rs index 6252c41e..881c5509 100644 --- a/examples/sled-cluster/src/lib.rs +++ b/examples/sled-cluster/src/lib.rs @@ -1,7 +1,7 @@ mod converters; mod sled_state_machine; mod sled_storage_engine; -pub(crate) use converters::{safe_kv, safe_vk, safe_vk_ivec}; +pub(crate) use converters::{safe_kv, safe_vk_ivec}; pub use sled_state_machine::*; pub use sled_storage_engine::*; diff --git a/examples/sled-cluster/src/sled_state_machine.rs b/examples/sled-cluster/src/sled_state_machine.rs index 14f3909a..b1bd09df 100644 --- a/examples/sled-cluster/src/sled_state_machine.rs +++ b/examples/sled-cluster/src/sled_state_machine.rs @@ -1,13 +1,15 @@ //! It works as KV storage for client business CRUDs. +use crate::compute_checksum_from_folder_path; +use crate::{safe_kv, safe_vk_ivec}; use arc_swap::ArcSwap; use async_trait::async_trait; use bincode::config; use bytes::Bytes; +use d_engine::BatchOp; use d_engine::{ - ApplyEntry, ApplyResult, Command, Result, ScanResult, SnapshotError, StateMachine, StorageError, - common::LogId, - server_storage::SnapshotMetadata, + ApplyEntry, ApplyResult, Command, Result, ScanResult, SnapshotError, StateMachine, + StorageError, common::LogId, server_storage::SnapshotMetadata, }; use parking_lot::Mutex; use sled::Batch; @@ -22,12 +24,8 @@ use tokio::sync::RwLock; use tracing::debug; use tracing::error; use tracing::info; -use tracing::instrument; use tracing::trace; -use crate::compute_checksum_from_folder_path; -use crate::{safe_kv, safe_vk, safe_vk_ivec}; - /// Sled database tree namespaces pub(crate) const STATE_MACHINE_TREE: &str = "_state_machine_tree"; pub(crate) const STATE_MACHINE_META_NAMESPACE: &str = "_state_machine_metadata"; @@ -174,21 +172,10 @@ impl StateMachine for SledStateMachine { } } - fn entry_term( + async fn apply_chunk( &self, - entry_id: u64, - ) -> Option { - match self.get(&safe_kv(entry_id)) { - Ok(Some(term_bytes)) => safe_vk(&term_bytes).ok(), - Ok(None) => None, - Err(e) => { - error!("Failed to retrieve term for entry {}: {}", entry_id, e); - None - } - } - } - - async fn apply_chunk(&self, chunk: &[ApplyEntry]) -> Result> { + chunk: &[ApplyEntry], + ) -> Result> { trace!("Applying chunk: {} entries", chunk.len()); let mut highest_log_id: Option = None; @@ -204,7 +191,10 @@ impl StateMachine for SledStateMachine { prev.index ); } - highest_log_id = Some(LogId { index: entry.index, term: entry.term }); + highest_log_id = Some(LogId { + index: entry.index, + term: entry.term, + }); match &entry.command { Command::Noop => { @@ -219,7 +209,11 @@ impl StateMachine for SledStateMachine { batch.remove(key.as_ref()); results.push(ApplyResult::success(entry.index)); } - Command::CompareAndSwap { key, expected, value: new_value } => { + Command::CompareAndSwap { + key, + expected, + value: new_value, + } => { // Flush pending batch before CAS (Sled batch doesn't support atomic CAS) self.apply_batch(std::mem::take(&mut batch))?; @@ -249,6 +243,17 @@ impl StateMachine for SledStateMachine { ApplyResult::failure(entry.index) }); } + Command::Batch { ops } => { + ops.iter().for_each(|op| match op { + BatchOp::Insert { key, value } => { + batch.insert(key.as_ref(), value.as_ref()) + } + BatchOp::Delete { key } => { + batch.remove(key.as_ref()); + } + }); + results.push(ApplyResult::success(entry.index)); + } } } @@ -335,7 +340,6 @@ impl StateMachine for SledStateMachine { self.flush() } - #[instrument(skip(self))] async fn generate_snapshot_data( &self, new_snapshot_dir: PathBuf, @@ -403,7 +407,6 @@ impl StateMachine for SledStateMachine { Ok(Bytes::copy_from_slice(&checksum)) } - #[instrument(skip(self))] async fn apply_snapshot_from_file( &self, metadata: &SnapshotMetadata, @@ -494,7 +497,11 @@ impl StateMachine for SledStateMachine { async fn reset(&self) -> Result<()> { let db = self.db.load(); - for tree_name in &[STATE_MACHINE_TREE, STATE_MACHINE_META_NAMESPACE, STATE_SNAPSHOT_METADATA_TREE] { + for tree_name in &[ + STATE_MACHINE_TREE, + STATE_MACHINE_META_NAMESPACE, + STATE_SNAPSHOT_METADATA_TREE, + ] { db.open_tree(tree_name) .map_err(|e| StorageError::DbError(e.to_string()))? .clear() diff --git a/examples/sled-cluster/src/sled_storage_engine.rs b/examples/sled-cluster/src/sled_storage_engine.rs index c03d897f..8552a278 100644 --- a/examples/sled-cluster/src/sled_storage_engine.rs +++ b/examples/sled-cluster/src/sled_storage_engine.rs @@ -2,8 +2,8 @@ use async_trait::async_trait; use bincode::config; use bytes::Bytes; use d_engine::{ - common::{Entry, LogId}, Error, HardState, LogStore, MetaStore, ProstError, Result, StorageEngine, StorageError, + common::{Entry, LogId}, }; use prost::Message; use sled::Batch; @@ -13,7 +13,6 @@ use std::path::Path; use std::sync::Arc; use tracing::error; use tracing::info; -use tracing::instrument; use tracing::trace; const HARD_STATE_KEY: &[u8] = b"hard_state"; @@ -74,7 +73,6 @@ impl LogStore for SledLogStore { Ok(()) } - #[instrument(skip(self))] async fn entry( &self, index: u64, @@ -89,7 +87,6 @@ impl LogStore for SledLogStore { } } - #[instrument(skip(self))] fn get_entries( &self, range: RangeInclusive, @@ -107,7 +104,6 @@ impl LogStore for SledLogStore { Ok(entries) } - #[instrument(skip(self))] async fn purge( &self, cutoff_index: LogId, @@ -125,7 +121,6 @@ impl LogStore for SledLogStore { Ok(()) } - #[instrument(skip(self))] async fn truncate( &self, from_index: u64, @@ -177,7 +172,6 @@ impl LogStore for SledLogStore { false } - #[instrument(skip(self))] fn flush(&self) -> Result<()> { trace!("LogStore flush"); self.tree.flush().map_err(|e| StorageError::DbError(e.to_string()))?; @@ -193,13 +187,11 @@ impl LogStore for SledLogStore { Ok(()) } - #[instrument(skip(self))] async fn reset(&self) -> Result<()> { self.tree.clear().map_err(|e| StorageError::DbError(e.to_string()))?; Ok(()) } - #[instrument(skip(self))] fn last_index(&self) -> u64 { match self.tree.last() { Ok(Some((key, _))) => {