diff --git a/CHANGELOG.md b/CHANGELOG.md index 3299062d..a4eebbac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,10 @@ All notable changes to this project will be documented in this file. - **`start_with()` accepts `impl AsRef`** (#415): Callers can now pass `&str`, `String`, `&Path`, or `PathBuf` — backward compatible, no migration needed. +- **PreVote and leader lease guard** (#423): An isolated node no longer inflates its term and forces a healthy leader to step down. Elections run a PreVote round first (reusing `VoteRequest`/`VoteResponse` over a new `PreVote` RPC; no term bump, nothing persisted), and followers and leaders ignore vote/PreVote requests while a leader is alive (Raft dissertation §4.2.3). + +- **Write admission and CheckQuorum** (#423): A leader without a recent quorum ACK rejects new client writes with `NotLeader` after `election_timeout_min`, before anything is appended, so clients can retry safely. After `election_timeout_max x write_admission_election_timeout_multiple` (new key in `[raft.backpressure]`, default `2`) without a quorum ACK it steps down. A new leader gets one window for its first ACK; single-voter clusters are exempt. + ### 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`. @@ -44,6 +48,10 @@ All notable changes to this project will be documented in this file. replicas — see [Throughput Optimization Guide](./d-engine/src/docs/performance/throughput-optimization-guide.md) for tuning `idle_flush_interval_ms`. +- **Vote protocol hardening** (#423): Hard state (term + vote) is now fsynced before replying to a vote request and a flush failure is fatal (a lost vote could allow two leaders in one term). A vote from an older term no longer blocks a request in a newer term, a same-term step-down keeps the term's vote, a candidate adopts a higher term even when it cannot grant the vote (§5.1), only active voters campaign (none while a committed membership change is unapplied), the first election round starts immediately instead of after two election timeouts, and the read lease is published only after the leader's noop is applied (§8). After a long snapshot install the follower re-arms its election timer and leader-contact guard. + +- **Quorum confirmation no longer derives from replicated history** (#423): In a 5+ voter cluster, a leader cut off from the majority but still reached by one follower kept renewing its read lease and never stepped down, because the commit median over every peer's stored `match_index` stayed true. A leader is now confirmed only when a quorum of voters acknowledged a newer heartbeat round; unreachable peers stop counting. Applies to lease reads, queued linearizable reads, CheckQuorum, write admission and the vote guard. + ### Changed - **MSRV raised to Rust 1.89**: The `data_dir` startup lock (prevents two node processes from @@ -69,6 +77,12 @@ All notable changes to this project will be documented in this file. - **New `shutdown_timeout_ms` in `[raft.persistence]`** (default `5000`): bounds `close()` wait against a stuck fsync task. The task itself continues running in the background. +- **⚠️ Two new startup config checks** (#423). A config that violated them started before and now fails with a config error: + - `election.election_timeout_min` must be at least 3 x `replication.rpc_append_entries_clock_in_ms` (defaults 500 / 100). Raise `election_timeout_min` or lower the heartbeat interval. + - `backpressure.max_pending_writes` / `max_pending_reads` must be greater than `batching.max_batch_size`, or `0` (unlimited). + +- **Writes are at-least-once across a leader failover** (#423): a write already proposed when the leader steps down can still be committed by the new leader although the client received an error (`NotLeader`, `ProposeFailed` or `TermOutdated`). There is no request-id deduplication yet, so retrying a non-idempotent command can apply it twice. + - **`data_dir` removed from `ClusterConfig`** — it is now a required explicit argument to all engine constructors (`EmbeddedEngine::start(data_dir)`, `StandaloneEngine::run(data_dir, shutdown_rx)`, etc.). Remove `[cluster] data_dir` / `[cluster] db_root_dir` from all config files; the constructor argument always wins and the config field is silently ignored. - **`cluster.log_dir` removed** — was validated but never consumed. diff --git a/README.md b/README.md index 7ad8d314..963b476f 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ [![codecov](https://codecov.io/gh/deventlab/d-engine/graph/badge.svg?token=K3BEDM45V8)](https://codecov.io/gh/deventlab/d-engine) ![Static Badge](https://img.shields.io/badge/license-MIT%20%7C%20Apache--2.0-blue) [![CI](https://github.com/deventlab/d-engine/actions/workflows/ci.yml/badge.svg)](https://github.com/deventlab/d-engine/actions/workflows/ci.yml) -[![Ask DeepWiki](https://deepwiki.com/badge.svg)](https://deepwiki.com/deventlab/d-engine) +[![DeepWiki](https://img.shields.io/badge/DeepWiki-d--engine-blue.svg?logo=data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACwAAAAyCAYAAAAnWDnqAAAAAXNSR0IArs4c6QAAA05JREFUaEPtmUtyEzEQhtWTQyQLHNak2AB7ZnyXZMEjXMGeK/AIi+QuHrMnbChYY7MIh8g01fJoopFb0uhhEqqcbWTp06/uv1saEDv4O3n3dV60RfP947Mm9/SQc0ICFQgzfc4CYZoTPAswgSJCCUJUnAAoRHOAUOcATwbmVLWdGoH//PB8mnKqScAhsD0kYP3j/Yt5LPQe2KvcXmGvRHcDnpxfL2zOYJ1mFwrryWTz0advv1Ut4CJgf5uhDuDj5eUcAUoahrdY/56ebRWeraTjMt/00Sh3UDtjgHtQNHwcRGOC98BJEAEymycmYcWwOprTgcB6VZ5JK5TAJ+fXGLBm3FDAmn6oPPjR4rKCAoJCal2eAiQp2x0vxTPB3ALO2CRkwmDy5WohzBDwSEFKRwPbknEggCPB/imwrycgxX2NzoMCHhPkDwqYMr9tRcP5qNrMZHkVnOjRMWwLCcr8ohBVb1OMjxLwGCvjTikrsBOiA6fNyCrm8V1rP93iVPpwaE+gO0SsWmPiXB+jikdf6SizrT5qKasx5j8ABbHpFTx+vFXp9EnYQmLx02h1QTTrl6eDqxLnGjporxl3NL3agEvXdT0WmEost648sQOYAeJS9Q7bfUVoMGnjo4AZdUMQku50McDcMWcBPvr0SzbTAFDfvJqwLzgxwATnCgnp4wDl6Aa+Ax283gghmj+vj7feE2KBBRMW3FzOpLOADl0Isb5587h/U4gGvkt5v60Z1VLG8BhYjbzRwyQZemwAd6cCR5/XFWLYZRIMpX39AR0tjaGGiGzLVyhse5C9RKC6ai42ppWPKiBagOvaYk8lO7DajerabOZP46Lby5wKjw1HCRx7p9sVMOWGzb/vA1hwiWc6jm3MvQDTogQkiqIhJV0nBQBTU+3okKCFDy9WwferkHjtxib7t3xIUQtHxnIwtx4mpg26/HfwVNVDb4oI9RHmx5WGelRVlrtiw43zboCLaxv46AZeB3IlTkwouebTr1y2NjSpHz68WNFjHvupy3q8TFn3Hos2IAk4Ju5dCo8B3wP7VPr/FGaKiG+T+v+TQqIrOqMTL1VdWV1DdmcbO8KXBz6esmYWYKPwDL5b5FA1a0hwapHiom0r/cKaoqr+27/XcrS5UwSMbQAAAABJRU5ErkJggg==)](https://deepwiki.com/deventlab/d-engine) **d-engine** is a lightweight, embeddable distributed coordination engine for Rust — strong-consistency KV, watch streams, and leader election, running inside your process. diff --git a/d-engine-client/src/lib.rs b/d-engine-client/src/lib.rs index 9a20f80a..a8c8599c 100644 --- a/d-engine-client/src/lib.rs +++ b/d-engine-client/src/lib.rs @@ -179,8 +179,10 @@ impl Client { /// **What this does NOT do:** /// - Does not guarantee that the caller's in-flight requests succeeded; /// requests sent before `refresh()` may have failed and need to be retried - /// - Does not implement application-level retry — the caller is responsible - /// for re-issuing any operations that failed during the failover window + /// - Writes are at-least-once across a failover: a write that was already proposed when + /// the leader stepped down may still be committed by the new leader even though the + /// caller received an error. Re-issuing a non-idempotent command can apply it twice + /// (the engine has no request-id dedup yet). /// - Does not update endpoints permanently; pass `new_endpoints` to change /// the bootstrap list for this and future refreshes /// diff --git a/d-engine-core/src/config/raft.rs b/d-engine-core/src/config/raft.rs index ef661d8f..badd011c 100644 --- a/d-engine-core/src/config/raft.rs +++ b/d-engine-core/src/config/raft.rs @@ -180,11 +180,21 @@ impl RaftConfig { self.replication.validate()?; self.batching.validate()?; self.election.validate()?; + // Write admission rejects after `election_timeout_min` without a quorum ACK, so a few + // lost heartbeats must fit inside it or normal jitter rejects client writes. + let heartbeat_ms = self.replication.rpc_append_entries_clock_in_ms; + if self.election.election_timeout_min < heartbeat_ms.saturating_mul(3) { + return Err(Error::Config(ConfigError::Message(format!( + "election_timeout_min {}ms must be at least 3 x rpc_append_entries_clock_in_ms {}ms", + self.election.election_timeout_min, heartbeat_ms + )))); + } self.membership.validate()?; self.state_machine.validate()?; self.snapshot.validate()?; self.read_consistency.validate(self.election.election_timeout_min)?; self.read_actor.validate()?; + self.backpressure.validate(self.batching.max_batch_size)?; self.watch.validate()?; self.persistence.validate()?; @@ -936,6 +946,16 @@ pub struct BackpressureConfig { /// **Default**: 50000 (0 = unlimited) #[serde(default = "default_max_pending_reads")] pub max_pending_reads: usize, + + /// CheckQuorum step-down limit, as a multiple of `election.election_timeout_max`. + /// + /// A leader that no quorum has answered within this limit steps down (same term). New + /// client writes are rejected earlier, after `election_timeout_min` without an ACK, and + /// that rejection never changes the role. + /// + /// **Default**: 2 (2 s with the default 1 s `election_timeout_max`). Must be at least 1. + #[serde(default = "default_write_admission_election_timeout_multiple")] + pub write_admission_election_timeout_multiple: u64, } impl Default for BackpressureConfig { @@ -943,6 +963,8 @@ impl Default for BackpressureConfig { Self { max_pending_writes: default_max_pending_writes(), max_pending_reads: default_max_pending_reads(), + write_admission_election_timeout_multiple: + default_write_admission_election_timeout_multiple(), } } } @@ -955,7 +977,43 @@ fn default_max_pending_reads() -> usize { 50_000 } +fn default_write_admission_election_timeout_multiple() -> u64 { + 2 +} + impl BackpressureConfig { + /// Validates the write admission window and the pending limits. + /// + /// `max_batch_size` is `batching.max_batch_size`: one Raft loop round pushes up to + /// `max_batch_size + 1` commands before the buffer is flushed, so a non-zero limit at or + /// below it rejects requests of a perfectly healthy node. + pub(crate) fn validate( + &self, + max_batch_size: usize, + ) -> Result<()> { + if self.write_admission_election_timeout_multiple == 0 { + return Err(Error::Config(ConfigError::Message( + "write_admission_election_timeout_multiple must be at least 1 \ + (0 leaves no window: the leader would reject every write)" + .into(), + ))); + } + + for (name, limit) in [ + ("max_pending_writes", self.max_pending_writes), + ("max_pending_reads", self.max_pending_reads), + ] { + if limit != 0 && limit <= max_batch_size { + return Err(Error::Config(ConfigError::Message(format!( + "{name} {limit} must be greater than batching.max_batch_size \ + {max_batch_size} (0 = unlimited): a drained batch alone would trip the limit" + )))); + } + } + + Ok(()) + } + /// Check if write request should be rejected due to backpressure /// /// Returns true if the current pending count exceeds the limit. diff --git a/d-engine-core/src/config/raft_test.rs b/d-engine-core/src/config/raft_test.rs index de77d3f7..51efd9b4 100644 --- a/d-engine-core/src/config/raft_test.rs +++ b/d-engine-core/src/config/raft_test.rs @@ -99,6 +99,7 @@ fn test_backpressure_unlimited_when_zero() { let config = BackpressureConfig { max_pending_writes: 0, max_pending_reads: 0, + ..Default::default() }; // 0 = unlimited, should never reject @@ -111,6 +112,7 @@ fn test_backpressure_write_limit_enforcement() { let config = BackpressureConfig { max_pending_writes: 100, max_pending_reads: 200, + ..Default::default() }; // Below limit - allow @@ -130,6 +132,7 @@ fn test_backpressure_read_limit_enforcement() { let config = BackpressureConfig { max_pending_writes: 100, max_pending_reads: 200, + ..Default::default() }; // Below limit - allow @@ -149,6 +152,7 @@ fn test_backpressure_write_and_read_independent() { let config = BackpressureConfig { max_pending_writes: 100, max_pending_reads: 200, + ..Default::default() }; // Write limit doesn't affect read checks @@ -167,11 +171,13 @@ fn test_backpressure_zero_vs_nonzero() { let config_unlimited_writes = BackpressureConfig { max_pending_writes: 0, max_pending_reads: 100, + ..Default::default() }; let config_limited_writes = BackpressureConfig { max_pending_writes: 100, max_pending_reads: 0, + ..Default::default() }; // Unlimited writes, limited reads @@ -528,3 +534,133 @@ fn load_toml(text: &str) -> RaftConfig { .try_deserialize() .expect("config must deserialize") } + +// ============================================================================ +// Write admission / step-down config and the cross-field checks around it +// ============================================================================ + +#[test] +fn test_write_admission_multiple_default_is_two() { + assert_eq!( + BackpressureConfig::default().write_admission_election_timeout_multiple, + 2 + ); +} + +/// A missing key parses to the default, so existing config files keep working. +#[test] +fn test_write_admission_multiple_missing_key_parses_to_default() { + let parsed: BackpressureConfig = config::Config::builder() + .add_source(config::File::from_str( + "max_pending_writes = 500", + config::FileFormat::Toml, + )) + .build() + .unwrap() + .try_deserialize() + .unwrap(); + + assert_eq!(parsed.max_pending_writes, 500); + assert_eq!(parsed.write_admission_election_timeout_multiple, 2); + assert_eq!(parsed.max_pending_reads, 50_000); +} + +#[test] +fn test_write_admission_multiple_zero_is_rejected() { + let mut config = RaftConfig::default(); + config.backpressure.write_admission_election_timeout_multiple = 0; + + assert!(config.validate().is_err()); +} + +#[test] +fn test_write_admission_multiple_one_and_large_values_are_accepted() { + for multiple in [1, 2, 10, 1_000] { + let mut config = RaftConfig::default(); + config.backpressure.write_admission_election_timeout_multiple = multiple; + + assert!( + config.validate().is_ok(), + "multiple {multiple} must be accepted" + ); + } +} + +/// `max_pending_writes` / `max_pending_reads` must exceed `batching.max_batch_size` (one loop +/// round pushes up to max_batch_size + 1 commands), unless 0 = unlimited. +#[test] +fn test_pending_limits_must_exceed_max_batch_size() { + let batch = 100; + let with = |writes: usize, reads: usize| { + let mut config = RaftConfig::default(); + config.batching.max_batch_size = batch; + config.backpressure.max_pending_writes = writes; + config.backpressure.max_pending_reads = reads; + config.validate().is_ok() + }; + + assert!( + !with(batch, 50_000), + "writes limit == max_batch_size must be rejected" + ); + assert!( + !with(batch - 1, 50_000), + "writes limit below max_batch_size must be rejected" + ); + assert!( + with(batch + 1, 50_000), + "writes limit == max_batch_size + 1 must be accepted" + ); + assert!( + !with(10_000, batch), + "reads limit == max_batch_size must be rejected" + ); + assert!( + with(10_000, batch + 1), + "reads limit == max_batch_size + 1 must be accepted" + ); + assert!(with(0, 0), "0 = unlimited must be accepted for both"); + assert!( + with(0, 50_000) && with(10_000, 0), + "each limit is independent" + ); +} + +/// Write admission rejects after `election_timeout_min` without a quorum ACK, so that window must +/// hold at least 3 heartbeats (two can be lost). +#[test] +fn test_election_timeout_min_must_hold_three_heartbeats() { + let validate_with = |election_min: u64, heartbeat: u64| { + let mut config = RaftConfig::default(); + // Keep the read-lease rule out of the way: only the heartbeat ratio is under test. + config.read_consistency.lease_duration_ms = 10; + config.election.election_timeout_min = election_min; + config.election.election_timeout_max = 3_000; + config.replication.rpc_append_entries_clock_in_ms = heartbeat; + config.validate() + }; + + let too_small = validate_with(299, 100).expect_err("299 ms holds fewer than 3 heartbeats"); + assert!( + too_small.to_string().contains("rpc_append_entries_clock_in_ms"), + "the error must name the heartbeat setting, got: {too_small}" + ); + assert!( + validate_with(300, 100).is_ok(), + "exactly 3 heartbeats is accepted" + ); + assert!( + validate_with(500, 100).is_ok(), + "the defaults (500 / 100) are accepted" + ); + assert!( + validate_with(30, 10).is_ok(), + "the rule is a ratio, not a fixed size" + ); + assert!(validate_with(29, 10).is_err()); +} + +#[test] +fn test_default_config_is_valid() { + assert!(RaftConfig::default().validate().is_ok()); +} diff --git a/d-engine-core/src/election/election_handler.rs b/d-engine-core/src/election/election_handler.rs index 020fe81a..3b6c084b 100644 --- a/d-engine-core/src/election/election_handler.rs +++ b/d-engine-core/src/election/election_handler.rs @@ -1,6 +1,7 @@ use async_trait::async_trait; use d_engine_proto::common::LogId; use d_engine_proto::server::election::VoteRequest; +use d_engine_proto::server::election::VoteResponse; use d_engine_proto::server::election::VotedFor; use std::collections::HashSet; use std::fmt::Debug; @@ -29,6 +30,45 @@ use crate::alias::TROF; use crate::cluster::is_majority; use crate::if_higher_term_found; use crate::is_target_log_more_recent; +use crate::is_target_log_strictly_more_recent; + +#[derive(Clone, Copy)] +enum VoteRpc { + Vote, + PreVote, +} +impl VoteRpc { + /// Label for logs, so a PreVote round is never mistaken for a real election. + fn label(self) -> &'static str { + match self { + VoteRpc::Vote => "vote", + VoteRpc::PreVote => "pre_vote", + } + } + + /// True if the peer's term shows this node is behind and must catch up. + /// `asked_term` is the term this node asked about: for a real vote its current term, + /// for PreVote the term it wants to move to. + fn peer_term_shows_behind( + self, + asked_term: u64, + peer_term: u64, + ) -> bool { + match self { + VoteRpc::Vote => if_higher_term_found(asked_term, peer_term, false), + // A peer already at or beyond the term we want means that term is taken. + VoteRpc::PreVote => peer_term >= asked_term, + } + } + + /// The rule used by `peer_term_shows_behind`, for logs. Keep it next to the code above. + fn behind_rule(self) -> &'static str { + match self { + VoteRpc::Vote => "peer_term > asked_term", + VoteRpc::PreVote => "peer_term >= asked_term", + } + } +} #[derive(Clone)] pub struct ElectionHandler { @@ -41,6 +81,26 @@ impl ElectionCore for ElectionHandler where T: TypeConfig, { + async fn broadcast_pre_vote_requests( + &self, + term: u64, + membership: Arc>, + raft_log: &Arc>, + transport: &Arc>, + settings: &Arc, + ) -> Result<()> { + debug!("broadcast_pre_vote_requests..."); + self.broadcast_election_requests( + VoteRpc::PreVote, + term, + membership, + raft_log, + transport, + settings, + ) + .await + } + async fn broadcast_vote_requests( &self, term: u64, @@ -50,122 +110,54 @@ where settings: &Arc, ) -> Result<()> { debug!("broadcast_vote_requests..."); - - // Single-node cluster: no peers to vote, automatically win election - if membership.is_single_node_cluster() { - debug!( - "Single-node cluster detected (node_id={}): automatically winning election", - self.my_id - ); - return Ok(()); - } - - let members = membership.voters(); - if members.is_empty() { - error!("No voting members found for node {}", self.my_id); - return Err(ElectionError::NoVotingMemberFound { - candidate_id: self.my_id, - } - .into()); - } - - debug!("Sending vote requests to peers: {:?}", &members); - - let LogId { - index: last_log_index, - term: last_log_term, - } = raft_log.last_log_id().unwrap_or(LogId { index: 0, term: 0 }); - let request = VoteRequest { + self.broadcast_election_requests( + VoteRpc::Vote, term, - candidate_id: self.my_id, - last_log_index, - last_log_term, - }; - - // One task per peer. Dropping the JoinSet aborts RPCs still in flight, so a decided - // election never leaves retry loops behind. - let mut tasks = JoinSet::new(); - let mut peer_ids = HashSet::new(); - for peer in members { - let peer_id = peer.id; - if peer_id == self.my_id || peer_ids.contains(&peer_id) { - continue; // Skip self and duplicates - } - peer_ids.insert(peer_id); - - let transport = transport.clone(); - let membership = membership.clone(); - let retry = settings.retry.clone(); - tasks.spawn(async move { - transport.send_vote_request(peer_id, request, &retry, membership).await - }); - } - - let required = peer_ids.len() + 1; - let mut succeed = 1; // own vote - // Tally as responses arrive and decide as soon as the outcome is known. Waiting - // for every peer lets one dead peer's RPC retries (seconds) hold the Raft loop - // past our own election timeout, discarding a majority we already won. - while let Some(joined) = tasks.join_next().await { - let response = match joined { - Ok(r) => r, - Err(e) => { - error!("Task failed with error: {:?}", &e); - Err(Error::from(NetworkError::TaskFailed(e))) - } - }; - match response { - Ok(vote_response) => { - if vote_response.vote_granted { - debug!("send_vote_requests_to_peers success!"); - succeed += 1; - if is_majority(succeed, required) { - debug!("send_vote_requests receives majority."); - return Ok(()); - } - } else { - debug!( - "if_higher_term_found({}, {}, false)", - term, vote_response.term, - ); - if if_higher_term_found(term, vote_response.term, false) { - info!("Higher term found during election phase."); - return Err(ElectionError::HigherTerm(vote_response.term).into()); - } - - if is_target_log_more_recent( - last_log_index, - last_log_term, - vote_response.last_log_index, - vote_response.last_log_term, - ) { - info!("More update to date log found in vote response"); - - return Err(ElectionError::LogConflict { - index: last_log_index, - expected_term: last_log_term, - actual_term: vote_response.last_log_term, - } - .into()); - } + membership, + raft_log, + transport, + settings, + ) + .await + } - info!("send_vote_requests_to_peers failed!"); - } - } - Err(e) => { - // Debug, not error: a single peer RPC failure here is routinely - // caused by the peer not being ready yet (e.g. cluster bootstrap - // race) and resolves itself on the next election round (#428). - info!("send_vote_requests_to_peers error: {:?}", e); - } - } - } + fn handle_pre_vote_request( + &self, + request: VoteRequest, + current_term: u64, + raft_log: &Arc>, + leader_active: bool, + ) -> VoteResponse { + let last_logid = raft_log.last_log_id().unwrap_or(LogId { index: 0, term: 0 }); + // First matching reason wins; None means the PreVote would be granted. + let deny_reason = if leader_active { + Some("a live leader is still active") + } else if request.term <= current_term { + Some("the asked term is already taken") + } else if !is_target_log_more_recent( + last_logid.index, + last_logid.term, + request.last_log_index, + request.last_log_term, + ) { + Some("the requester's log is behind ours") + } else { + None + }; debug!( - "failed to receive majority votes: {:?} succeed number = {}", - &peer_ids, succeed + "PreVote from node-{} for term {}: {}", + request.candidate_id, + request.term, + deny_reason.map_or("granted".to_string(), |r| format!("denied, {r}")), ); - Err(ElectionError::QuorumFailure { required, succeed }.into()) + + VoteResponse { + term: current_term, + vote_granted: deny_reason.is_none(), + last_log_index: last_logid.index, + last_log_term: last_logid.term, + } } async fn handle_vote_request( @@ -176,6 +168,10 @@ where raft_log: &Arc>, ) -> Result { debug!("VoteRequest::Received: {:?}", request); + // A vote only counts in the term it was cast in. Ignore one left over from an older + // term: hard state persisted by earlier versions can still carry it. + let voted_for_option = voted_for_option.filter(|v| v.voted_for_term >= current_term); + let mut new_voted_for = None; let mut term_update = None; let last_logid = raft_log.last_log_id().unwrap_or(LogId { index: 0, term: 0 }); @@ -326,6 +322,154 @@ where } } + async fn broadcast_election_requests( + &self, + rpc: VoteRpc, + term: u64, + membership: Arc>, + raft_log: &Arc>, + transport: &Arc>, + settings: &Arc, + ) -> Result<()> { + // Single-node cluster: no peers to vote, automatically win election + if membership.is_single_node_cluster() { + debug!( + "Single-node cluster detected (node_id={}): automatically winning election", + self.my_id + ); + return Ok(()); + } + + let members = membership.voters(); + if members.is_empty() { + error!("No voting members found for node {}", self.my_id); + return Err(ElectionError::NoVotingMemberFound { + candidate_id: self.my_id, + } + .into()); + } + + debug!("Sending vote requests to peers: {:?}", &members); + + let LogId { + index: last_log_index, + term: last_log_term, + } = raft_log.last_log_id().unwrap_or(LogId { index: 0, term: 0 }); + let request = VoteRequest { + term, + candidate_id: self.my_id, + last_log_index, + last_log_term, + }; + + // One task per peer. Dropping the JoinSet aborts RPCs still in flight, so a decided + // election never leaves retry loops behind. + let mut tasks = JoinSet::new(); + let mut peer_ids = HashSet::new(); + for peer in members { + let peer_id = peer.id; + if peer_id == self.my_id || peer_ids.contains(&peer_id) { + continue; // Skip self and duplicates + } + peer_ids.insert(peer_id); + + let transport = transport.clone(); + let membership = membership.clone(); + let retry = settings.retry.clone(); + tasks.spawn(async move { + match rpc { + VoteRpc::Vote => { + transport.send_vote_request(peer_id, request, &retry, membership).await + } + VoteRpc::PreVote => { + transport.send_pre_vote_request(peer_id, request, &retry, membership).await + } + } + }); + } + + let required = peer_ids.len() + 1; + let mut succeed = 1; // own vote + let mut pending = peer_ids.len(); // answers still outstanding + // Tally as responses arrive and decide as soon as the outcome is known. Waiting + // for every peer lets one dead peer's RPC retries (seconds) hold the Raft loop + // past our own election timeout, discarding a majority we already won. + while let Some(joined) = tasks.join_next().await { + pending -= 1; + + let response = match joined { + Ok(r) => r, + Err(e) => { + error!("Task failed with error: {:?}", &e); + Err(Error::from(NetworkError::TaskFailed(e))) + } + }; + match response { + Ok(vote_response) => { + if vote_response.vote_granted { + debug!("send_vote_requests_to_peers success!"); + succeed += 1; + if is_majority(succeed, required) { + debug!("send_vote_requests receives majority."); + return Ok(()); + } + } else { + let behind = rpc.peer_term_shows_behind(term, vote_response.term); + debug!( + "[{}] peer denied: asked_term={}, peer_term={}, rule `{}` => behind={}", + rpc.label(), + term, + vote_response.term, + rpc.behind_rule(), + behind, + ); + if behind { + info!( + "[{}] higher term found: peer term {}", + rpc.label(), + vote_response.term + ); + return Err(ElectionError::HigherTerm(vote_response.term).into()); + } + + // A denial is just a missing vote: keep counting. A peer with a + // strictly newer log is only worth noting (openraft records it to + // delay the next election; raft-rs ignores it). + if is_target_log_strictly_more_recent( + last_log_index, + last_log_term, + vote_response.last_log_index, + vote_response.last_log_term, + ) { + info!("[{}] a denying peer has a newer log", rpc.label()); + } + + info!("send_vote_requests_to_peers failed!"); + } + } + Err(e) => { + // Debug, not error: a single peer RPC failure here is routinely + // caused by the peer not being ready yet (e.g. cluster bootstrap + // race) and resolves itself on the next election round (#428). + info!("send_vote_requests_to_peers error: {:?}", e); + } + } + + // Even if every peer still outstanding granted, could a majority be reached? + // If not the round is lost: return now instead of waiting for silent peers + // (raft-rs reports `Lost` at this point). + if !is_majority(succeed + pending, required) { + return Err(ElectionError::QuorumFailure { required, succeed }.into()); + } + } + + debug!( + "failed to receive majority votes: {:?} succeed number = {}", + &peer_ids, succeed + ); + Err(ElectionError::QuorumFailure { required, succeed }.into()) + } + fn if_node_could_grant_the_vote_request( &self, request: &VoteRequest, @@ -360,3 +504,7 @@ impl Debug for ElectionHandler { f.debug_struct("ElectionHandler").field("my_id", &self.my_id).finish() } } + +#[cfg(test)] +#[path = "election_handler_test.rs"] +mod election_handler_test; diff --git a/d-engine-core/src/election/election_handler_test.rs b/d-engine-core/src/election/election_handler_test.rs index 2f63ef4f..c91ded5a 100644 --- a/d-engine-core/src/election/election_handler_test.rs +++ b/d-engine-core/src/election/election_handler_test.rs @@ -812,8 +812,9 @@ mod single_node_election_tests { /// - Peer denies the vote and reports last_log_term=4 (higher than candidate's) /// /// Expected: - /// - Returns ElectionError::LogConflict — a peer with a more recent log - /// term means the candidate must not win this election + /// - Returns ElectionError::QuorumFailure with only the candidate's own vote: a peer with + /// a more recent log term denies, so the candidate must not win this election. + /// (A denial is just a missing vote; there is no separate log-conflict outcome.) #[tokio::test] async fn test_broadcast_vote_requests_rejects_on_peer_log_higher_term() { let election_handler = ElectionHandler::::new(1); @@ -876,15 +877,12 @@ mod single_node_election_tests { assert!( matches!( e, - Error::Consensus(ConsensusError::Election(ElectionError::LogConflict { - index, - expected_term, - actual_term, - })) if index == my_last_log_index - && expected_term == my_last_log_term - && actual_term == my_last_log_term + 1 + Error::Consensus(ConsensusError::Election(ElectionError::QuorumFailure { + required: 2, + succeed: 1, + })) ), - "Expected LogConflict error, got: {e:?}" + "Expected QuorumFailure with only the candidate's own vote, got: {e:?}" ); } @@ -983,6 +981,31 @@ mod single_node_election_tests { const SLOW_PEER_DELAY: std::time::Duration = std::time::Duration::from_secs(30); + /// Peer 2 grants at once; peers 6, 7 and 8 deny at once with a plain denial (same term, + /// older log); every other peer stays silent for `SLOW_PEER_DELAY` and then fails. + async fn slow_peer_answer(peer_id: u32) -> crate::Result { + if peer_id == 2 { + return Ok(VoteResponse { + term: 1, + vote_granted: true, + last_log_index: 1, + last_log_term: 1, + }); + } + if (6..=8).contains(&peer_id) { + return Ok(VoteResponse { + term: 1, + vote_granted: false, + last_log_index: 0, + last_log_term: 1, + }); + } + tokio::time::sleep(SLOW_PEER_DELAY).await; + Err(Error::from(crate::NetworkError::ServiceUnavailable( + "peer is down".to_string(), + ))) + } + #[async_trait::async_trait] impl crate::Transport for SlowPeerTransport { async fn send_vote_request( @@ -992,18 +1015,17 @@ mod single_node_election_tests { _retry: &crate::RetryPolicies, _membership: Arc>, ) -> crate::Result { - if peer_id == 2 { - return Ok(VoteResponse { - term: 1, - vote_granted: true, - last_log_index: 1, - last_log_term: 1, - }); - } - tokio::time::sleep(SLOW_PEER_DELAY).await; - Err(Error::from(crate::NetworkError::ServiceUnavailable( - "peer is down".to_string(), - ))) + slow_peer_answer(peer_id).await + } + + async fn send_pre_vote_request( + &self, + peer_id: u32, + _request: VoteRequest, + _retry: &crate::RetryPolicies, + _membership: Arc>, + ) -> crate::Result { + slow_peer_answer(peer_id).await } async fn send_cluster_update( @@ -1141,6 +1163,88 @@ mod single_node_election_tests { ); } + /// Test: a round that can no longer reach a majority must not wait for the silent peers. + /// + /// Scenario (5 nodes, a majority is 3): + /// - Peers 6, 7 and 8 deny at once (plain denial: older log, same term). + /// - Peer 3 is dead: its RPC only fails after a long retry sequence. + /// - Own vote + peer 3 is at most 2 of 5, so the round is already lost. + /// + /// Expected: + /// - QuorumFailure right after the third denial, without waiting for peer 3. The caller + /// awaits this inside the Raft loop; waiting for a dead peer keeps the node from + /// handling the heartbeat of whoever won, and (with PreVote) delays the next attempt. + /// raft-rs reports `Lost` as soon as the rejections make a majority impossible. + #[tokio::test(start_paused = true)] + async fn test_round_returns_quorum_failure_once_majority_is_impossible() { + for pre_vote in [false, true] { + let election_handler = ElectionHandler::::new(1); + + let mut raft_log_mock = MockRaftLog::new(); + raft_log_mock + .expect_last_log_id() + .times(1) + .returning(|| Some(LogId { index: 1, term: 1 })); + + let mut membership = MockMembership::::new(); + membership.expect_is_single_node_cluster().returning(|| false); + membership.expect_voters().returning(|| { + [3, 6, 7, 8] + .into_iter() + .map(|id| NodeMeta { + id, + address: format!("http://127.0.0.1:5500{id}"), + role: 0, + status: 2, + }) + .collect() + }); + + let node_config = RaftNodeConfig::new().expect("Should create default config"); + let node_config = node_config.validate().expect("Should validate config"); + let membership = Arc::new(membership); + let raft_log = Arc::new(raft_log_mock); + let transport = Arc::new(SlowPeerTransport); + let node_config = Arc::new(node_config); + + let round = async { + if pre_vote { + election_handler + .broadcast_pre_vote_requests( + 2, + membership, + &raft_log, + &transport, + &node_config, + ) + .await + } else { + election_handler + .broadcast_vote_requests(1, membership, &raft_log, &transport, &node_config) + .await + } + }; + let result = tokio::time::timeout(std::time::Duration::from_secs(1), round) + .await + .unwrap_or_else(|_| { + panic!( + "pre_vote={pre_vote}: the round waited for the dead peer although a \ + majority was already impossible" + ) + }); + + assert!( + matches!( + result, + Err(Error::Consensus(ConsensusError::Election( + ElectionError::QuorumFailure { .. } + ))) + ), + "pre_vote={pre_vote}: expected QuorumFailure, got: {result:?}" + ); + } + } + /// Test: broadcast_vote_requests steps down when a peer responds with a higher term /// /// Scenario: @@ -1224,8 +1328,9 @@ mod single_node_election_tests { /// - Peer's last_log_index is higher (more entries in the same term) /// /// Expected: - /// - Returns ElectionError::LogConflict — same-term but more entries - /// means the peer's log is more recent (Raft §5.4.1) + /// - Returns ElectionError::QuorumFailure with only the candidate's own vote: same-term but + /// more entries means the peer's log is more recent (Raft §5.4.1), so it denies and the + /// candidate must not win. #[tokio::test] async fn test_broadcast_vote_requests_rejects_on_peer_log_higher_index_same_term() { let election_handler = ElectionHandler::::new(1); @@ -1288,15 +1393,12 @@ mod single_node_election_tests { assert!( matches!( e, - Error::Consensus(ConsensusError::Election(ElectionError::LogConflict { - index, - expected_term, - actual_term, - })) if index == my_last_log_index - && expected_term == my_last_log_term - && actual_term == my_last_log_term + Error::Consensus(ConsensusError::Election(ElectionError::QuorumFailure { + required: 2, + succeed: 1, + })) ), - "Expected LogConflict error, got: {e:?}" + "Expected QuorumFailure with only the candidate's own vote, got: {e:?}" ); } @@ -1925,4 +2027,1007 @@ mod single_node_election_tests { "Should accept re-vote for same candidate in new term" ); } + + // ======================================================================== + // test_broadcast_pre_vote_requests_* - PreVote (sender side) + // + // PreVote asks "would you vote for me at term+1?" before the candidate bumps its + // term. It must never touch term/vote state: the raft log mocks below only allow + // `last_log_id`, so any persistence call would panic the test. + // ======================================================================== + + fn pre_vote_voter(id: u32) -> NodeMeta { + use d_engine_proto::common::NodeRole::Follower; + use d_engine_proto::common::NodeStatus; + + NodeMeta { + id, + address: format!("http://127.0.0.1:{}", 55000 + id), + role: Follower.into(), + status: NodeStatus::Active.into(), + } + } + + fn pre_vote_membership(peer_ids: Vec) -> MockMembership { + let mut membership = MockMembership::new(); + membership.expect_is_single_node_cluster().returning(|| false); + membership + .expect_voters() + .returning(move || peer_ids.iter().map(|id| pre_vote_voter(*id)).collect()); + membership + } + + fn pre_vote_raft_log() -> MockRaftLog { + let mut raft_log = MockRaftLog::new(); + raft_log.expect_last_log_id().returning(|| Some(LogId { index: 1, term: 1 })); + raft_log + } + + /// The responder's log is strictly behind the candidate's (`pre_vote_raft_log` is index 1, + /// term 1), a plain denial. + fn pre_vote_response( + term: u64, + granted: bool, + ) -> VoteResponse { + VoteResponse { + term, + vote_granted: granted, + last_log_index: 0, + last_log_term: 1, + } + } + + async fn run_pre_vote( + membership: MockMembership, + transport: MockTransport, + term: u64, + ) -> crate::Result<()> { + let node_config = RaftNodeConfig::new() + .expect("Should create default config") + .validate() + .expect("Should validate config"); + ElectionHandler::::new(1) + .broadcast_pre_vote_requests( + term, + Arc::new(membership), + &Arc::new(pre_vote_raft_log()), + &Arc::new(transport), + &Arc::new(node_config), + ) + .await + } + + /// Test: a majority of PreVote grants lets the candidate proceed to a real election. + /// + /// Scenario: + /// - Three nodes (candidate + peers 2, 3); peer 2 would grant, peer 3 is unreachable. + /// + /// Expected: + /// - Ok(()): own vote + peer 2 is a majority of 3. + /// - Only PreVote RPCs are sent; no real RequestVote is sent. + #[tokio::test] + async fn test_broadcast_pre_vote_requests_succeeds_with_majority_of_grants() { + let mut transport = MockTransport::new(); + transport.expect_send_vote_request().times(0); + transport + .expect_send_pre_vote_request() + .times(2) + .returning(|peer_id, request, _, _| { + if peer_id == 2 { + Ok(pre_vote_response(request.term - 1, true)) + } else { + Err(Error::from(crate::NetworkError::ServiceUnavailable( + "peer is down".to_string(), + ))) + } + }); + + let result = run_pre_vote(pre_vote_membership(vec![2, 3]), transport, 2).await; + + assert!( + result.is_ok(), + "expected majority of grants, got: {result:?}" + ); + } + + /// Test: a single-node cluster passes PreVote without contacting anyone. + #[tokio::test] + async fn test_broadcast_pre_vote_requests_single_node_cluster_succeeds_without_rpc() { + let mut membership = MockMembership::new(); + membership.expect_is_single_node_cluster().returning(|| true); + let mut transport = MockTransport::new(); + transport.expect_send_pre_vote_request().times(0); + transport.expect_send_vote_request().times(0); + + let result = run_pre_vote(membership, transport, 2).await; + + assert!(result.is_ok()); + } + + /// Test: unreachable peers are NOT counted as grants. + /// + /// Scenario: + /// - Three nodes, both peers unreachable (the node is isolated). + /// + /// Expected: + /// - QuorumFailure. If errors counted as grants, an isolated node would pass PreVote on + /// its own, bump its term every round, and PreVote would protect nothing. + #[tokio::test] + async fn test_broadcast_pre_vote_requests_does_not_count_unreachable_peers_as_grants() { + let mut transport = MockTransport::new(); + transport.expect_send_pre_vote_request().times(2).returning(|_, _, _, _| { + Err(Error::from(crate::NetworkError::ServiceUnavailable( + "peer is down".to_string(), + ))) + }); + + let e = run_pre_vote(pre_vote_membership(vec![2, 3]), transport, 2).await.unwrap_err(); + + assert!( + matches!( + e, + Error::Consensus(ConsensusError::Election( + ElectionError::QuorumFailure { .. } + )) + ), + "expected QuorumFailure, got: {e:?}" + ); + } + + /// Test: plain denials from the peers fail the PreVote round. + /// + /// Scenario: + /// - Three nodes; both peers answer "would not vote" (they still hear from a leader). + /// + /// Expected: + /// - QuorumFailure, so the caller keeps its term unchanged. + #[tokio::test] + async fn test_broadcast_pre_vote_requests_fails_quorum_when_peers_deny() { + let mut transport = MockTransport::new(); + transport + .expect_send_pre_vote_request() + .times(2) + .returning(|_, request, _, _| Ok(pre_vote_response(request.term - 1, false))); + + let e = run_pre_vote(pre_vote_membership(vec![2, 3]), transport, 2).await.unwrap_err(); + + assert!( + matches!( + e, + Error::Consensus(ConsensusError::Election( + ElectionError::QuorumFailure { .. } + )) + ), + "expected QuorumFailure, got: {e:?}" + ); + } + + /// Test: a peer reporting a higher term than the PreVote's term makes the candidate + /// step down to catch up. + /// + /// Scenario: + /// - Two nodes; the peer denies and reports term 5 while the PreVote term is 2. + /// + /// Expected: + /// - ElectionError::HigherTerm(5), same as the real vote. + #[tokio::test] + async fn test_broadcast_pre_vote_requests_reports_peer_higher_term() { + let mut transport = MockTransport::new(); + transport + .expect_send_pre_vote_request() + .times(1) + .returning(|_, _, _, _| Ok(pre_vote_response(5, false))); + + let e = run_pre_vote(pre_vote_membership(vec![2]), transport, 2).await.unwrap_err(); + + assert!( + matches!( + e, + Error::Consensus(ConsensusError::Election(ElectionError::HigherTerm(5))) + ), + "expected HigherTerm(5), got: {e:?}" + ); + } + + /// Test: a peer already at the PreVote's term means this node is behind and must catch up. + /// + /// Scenario: + /// - The node's real term is 1, so the PreVote asks about term 2. + /// - The peer is already at term 2 (an election happened while this node was away) and + /// denies, reporting term 2. + /// + /// Expected: + /// - HigherTerm(2). Comparing the peer's term with the hypothetical term (2) using a strict + /// `>` would call this a plain denial: the node would never learn it is behind and would + /// keep retrying PreVote at a term the cluster has already passed. The peer's term must + /// be compared with the node's real term (the PreVote term minus 1). + #[tokio::test] + async fn test_broadcast_pre_vote_requests_reports_peer_at_pre_vote_term_as_higher() { + let mut transport = MockTransport::new(); + transport + .expect_send_pre_vote_request() + .times(1) + .returning(|_, request, _, _| Ok(pre_vote_response(request.term, false))); + + let e = run_pre_vote(pre_vote_membership(vec![2]), transport, 2).await.unwrap_err(); + + assert!( + matches!( + e, + Error::Consensus(ConsensusError::Election(ElectionError::HigherTerm(2))) + ), + "expected HigherTerm(2), got: {e:?}" + ); + } + + /// Test: a peer at the node's real term is a plain denial, not a reason to step down. + /// + /// Scenario: + /// - The node's real term is 1 (PreVote term 2); the peer is also at term 1 and denies + /// because it still hears from a leader. + /// + /// Expected: + /// - QuorumFailure, so the node keeps its term and simply retries after the next timeout. + #[tokio::test] + async fn test_broadcast_pre_vote_requests_peer_at_same_real_term_is_plain_denial() { + let mut transport = MockTransport::new(); + transport + .expect_send_pre_vote_request() + .times(1) + .returning(|_, request, _, _| Ok(pre_vote_response(request.term - 1, false))); + + let e = run_pre_vote(pre_vote_membership(vec![2]), transport, 2).await.unwrap_err(); + + assert!( + matches!( + e, + Error::Consensus(ConsensusError::Election( + ElectionError::QuorumFailure { .. } + )) + ), + "expected QuorumFailure, got: {e:?}" + ); + } + + // ======================================================================== + // Tally tests shared by the real vote and PreVote + // + // Both rounds use the same counting loop, so every scenario runs against both. A denial is + // just a missing vote: the round only ends when a majority is reached, when a peer reveals + // a higher term (the node must catch up at once), or when every peer has answered. + // Reference scenarios: raft-rs `test_dueling_candidates`, `test_prevote_with_split_vote`; + // openraft `handle_vote_resp_test` / `handle_pre_vote_resp_test` ("reject keeps waiting"). + // ======================================================================== + + #[derive(Clone, Copy, Debug)] + enum Round { + Vote, + PreVote, + } + + const BOTH_ROUNDS: [Round; 2] = [Round::Vote, Round::PreVote]; + + /// What a peer answers. The candidate's own log is index 1, term 1 (`pre_vote_raft_log`). + #[derive(Clone, Copy, Debug)] + enum Reply { + Grant, + /// Plain denial: the peer's term equals the node's real term; only its log differs. + Deny { + log_index: u64, + log_term: u64, + }, + /// Denial that reveals a term above the node's. + DenyHigherTerm { + peer_term: u64, + }, + /// The RPC fails (peer down). + Down, + } + + /// Peer log equal to the candidate's (typical after steady-state replication). + fn deny_equal_log() -> Reply { + Reply::Deny { + log_index: 1, + log_term: 1, + } + } + + /// Peer log strictly newer than the candidate's (higher index, same term). + fn deny_newer_log() -> Reply { + Reply::Deny { + log_index: 5, + log_term: 1, + } + } + + /// Peer log strictly older than the candidate's. + fn deny_older_log() -> Reply { + Reply::Deny { + log_index: 0, + log_term: 1, + } + } + + /// The term the round asks about is 2. For a real vote that is the node's new term; for + /// PreVote it is the term the node wants to move to, so its real term is 1. + const ASKED_TERM: u64 = 2; + + fn node_term(round: Round) -> u64 { + match round { + Round::Vote => ASKED_TERM, + Round::PreVote => ASKED_TERM - 1, + } + } + + fn answer( + round: Round, + replies: &[(u32, Reply)], + peer_id: u32, + ) -> crate::Result { + let reply = replies.iter().find(|(id, _)| *id == peer_id).expect("peer has a reply").1; + match reply { + Reply::Grant => Ok(VoteResponse { + term: node_term(round), + vote_granted: true, + last_log_index: 1, + last_log_term: 1, + }), + Reply::Deny { + log_index, + log_term, + } => Ok(VoteResponse { + term: node_term(round), + vote_granted: false, + last_log_index: log_index, + last_log_term: log_term, + }), + Reply::DenyHigherTerm { peer_term } => Ok(VoteResponse { + term: peer_term, + vote_granted: false, + last_log_index: 1, + last_log_term: 1, + }), + Reply::Down => Err(Error::from(crate::NetworkError::ServiceUnavailable( + "peer is down".to_string(), + ))), + } + } + + /// Runs one round against peers `2..` answering as listed (peer ids are taken from `replies`). + async fn run_round( + round: Round, + replies: Vec<(u32, Reply)>, + ) -> crate::Result<()> { + let peer_ids: Vec = replies.iter().map(|(id, _)| *id).collect(); + + let mut transport = MockTransport::new(); + match round { + Round::Vote => { + transport.expect_send_pre_vote_request().times(0); + transport + .expect_send_vote_request() + .returning(move |peer_id, _, _, _| answer(round, &replies, peer_id)); + } + Round::PreVote => { + transport.expect_send_vote_request().times(0); + transport + .expect_send_pre_vote_request() + .returning(move |peer_id, _, _, _| answer(round, &replies, peer_id)); + } + } + + let node_config = RaftNodeConfig::new() + .expect("Should create default config") + .validate() + .expect("Should validate config"); + let handler = ElectionHandler::::new(1); + let membership = Arc::new(pre_vote_membership(peer_ids)); + let raft_log = Arc::new(pre_vote_raft_log()); + let transport = Arc::new(transport); + let node_config = Arc::new(node_config); + match round { + Round::Vote => { + handler + .broadcast_vote_requests( + ASKED_TERM, + membership, + &raft_log, + &transport, + &node_config, + ) + .await + } + Round::PreVote => { + handler + .broadcast_pre_vote_requests( + ASKED_TERM, + membership, + &raft_log, + &transport, + &node_config, + ) + .await + } + } + } + + fn is_quorum_failure(result: &crate::Result<()>) -> bool { + matches!( + result, + Err(Error::Consensus(ConsensusError::Election( + ElectionError::QuorumFailure { .. } + ))) + ) + } + + /// Test: a denial from a peer whose log equals ours is just a missing vote. + /// + /// Scenario (3 nodes, steady state, all logs equal): + /// - Peer 2 denies (it already voted for someone else in this term, or still hears a + /// leader); peer 3 grants. + /// + /// Expected: + /// - Ok(()): own vote + peer 3 is a majority. An equal log is not a "more recent" log, so + /// the denial must not end the round and discard peer 3's grant. + #[tokio::test] + async fn test_round_denial_with_equal_log_is_just_a_missing_vote() { + for round in BOTH_ROUNDS { + let result = run_round(round, vec![(2, deny_equal_log()), (3, Reply::Grant)]).await; + assert!(result.is_ok(), "{round:?}: got {result:?}"); + } + } + + /// Test: a denial from a peer whose log is strictly newer is also just a missing vote. + /// + /// Expected: + /// - Ok(()) when another peer grants. A peer's newer log does not stop a majority of + /// other peers (each already checked our log against theirs) from electing us. openraft + /// and raft-rs never end the round on a log comparison. + #[tokio::test] + async fn test_round_denial_with_newer_log_is_just_a_missing_vote() { + for round in BOTH_ROUNDS { + let result = run_round(round, vec![(2, deny_newer_log()), (3, Reply::Grant)]).await; + assert!(result.is_ok(), "{round:?}: got {result:?}"); + } + } + + /// Test: the order in which a grant and a denial arrive must not change the outcome. + /// + /// Expected: + /// - Ok(()) both when the grant is answered first and when the denial is. + #[tokio::test] + async fn test_round_outcome_does_not_depend_on_answer_order() { + for round in BOTH_ROUNDS { + let grant_first = + run_round(round, vec![(2, Reply::Grant), (3, deny_equal_log())]).await; + let denial_first = + run_round(round, vec![(2, deny_equal_log()), (3, Reply::Grant)]).await; + assert!( + grant_first.is_ok(), + "{round:?} grant first: got {grant_first:?}" + ); + assert!( + denial_first.is_ok(), + "{round:?} denial first: got {denial_first:?}" + ); + } + } + + /// Test: five nodes, two denials (equal and newer log) and two grants still win. + /// + /// Scenario (raft-rs `test_dueling_candidates`: "a rejection is not the end of the round"): + /// - Own vote + 2 grants = 3 of 5. + #[tokio::test] + async fn test_round_five_nodes_two_denials_and_two_grants_wins() { + for round in BOTH_ROUNDS { + let result = run_round( + round, + vec![ + (2, deny_equal_log()), + (3, deny_newer_log()), + (4, Reply::Grant), + (5, Reply::Grant), + ], + ) + .await; + assert!(result.is_ok(), "{round:?}: got {result:?}"); + } + } + + /// Test: five nodes, three denials cannot win and report a plain quorum failure. + /// + /// Expected: + /// - QuorumFailure, never a log-specific error: whatever the denial reason (equal, newer or + /// older log), the outcome is "not enough votes". + #[tokio::test] + async fn test_round_five_nodes_three_denials_fail_quorum() { + for round in BOTH_ROUNDS { + let result = run_round( + round, + vec![ + (2, deny_equal_log()), + (3, deny_newer_log()), + (4, deny_older_log()), + (5, Reply::Grant), + ], + ) + .await; + assert!(is_quorum_failure(&result), "{round:?}: got {result:?}"); + } + } + + /// Test: a lone peer denying, whatever its log, is a plain quorum failure. + /// + /// Scenario (2 nodes: we need both votes): + /// - The peer's log is equal, newer, or older than ours. + /// + /// Expected: + /// - QuorumFailure in all three cases. The node could not win, which is all the caller + /// needs to know; there is no separate "log conflict" outcome. + #[tokio::test] + async fn test_round_single_peer_denial_is_quorum_failure_for_any_log() { + for round in BOTH_ROUNDS { + for denial in [deny_equal_log(), deny_newer_log(), deny_older_log()] { + let result = run_round(round, vec![(2, denial)]).await; + assert!( + is_quorum_failure(&result), + "{round:?} {denial:?}: got {result:?}" + ); + } + } + } + + /// Test: a peer revealing a higher term ends the round at once, even after a grant. + /// + /// Scenario (5 nodes, so one grant is not yet a majority): + /// - Peer 2 grants, peer 3 denies and reports a term far above ours, peers 4 and 5 would + /// grant. + /// + /// Expected: + /// - HigherTerm(peer's term): the node must catch up immediately (raft-rs and openraft both + /// adopt a higher term as soon as they see it), not carry on counting. + #[tokio::test] + async fn test_round_higher_term_denial_ends_round_even_after_a_grant() { + for round in BOTH_ROUNDS { + let result = run_round( + round, + vec![ + (2, Reply::Grant), + (3, Reply::DenyHigherTerm { peer_term: 9 }), + (4, Reply::Grant), + (5, Reply::Grant), + ], + ) + .await; + assert!( + matches!( + result, + Err(Error::Consensus(ConsensusError::Election( + ElectionError::HigherTerm(9) + ))) + ), + "{round:?}: got {result:?}" + ); + } + } + + /// Test: unreachable peers and denials do not stop a majority from forming. + /// + /// Scenario (5 nodes): peer 2 down, peer 3 denies (equal log), peers 4 and 5 grant. + /// + /// Expected: + /// - Ok(()): own vote + 2 grants = 3 of 5. + #[tokio::test] + async fn test_round_unreachable_peer_and_denial_do_not_block_majority() { + for round in BOTH_ROUNDS { + let result = run_round( + round, + vec![ + (2, Reply::Down), + (3, deny_equal_log()), + (4, Reply::Grant), + (5, Reply::Grant), + ], + ) + .await; + assert!(result.is_ok(), "{round:?}: got {result:?}"); + } + } + + /// Test: an unreachable peer plus a denial is a quorum failure, not a success. + /// + /// Scenario (3 nodes): peer 2 down, peer 3 denies. Unreachable is never counted as a grant. + #[tokio::test] + async fn test_round_unreachable_peer_and_denial_fail_quorum() { + for round in BOTH_ROUNDS { + let result = run_round(round, vec![(2, Reply::Down), (3, deny_equal_log())]).await; + assert!(is_quorum_failure(&result), "{round:?}: got {result:?}"); + } + } + + /// Test: `VoteRpc::peer_term_shows_behind` compares with `>` for a real vote and with + /// `>=` for PreVote. + /// + /// - Real vote: asked_term is this node's own term; a peer must be strictly above it. + /// - PreVote: asked_term is the term this node WANTS; a peer already at it means that + /// term is taken. + #[test] + fn test_vote_rpc_peer_term_shows_behind_boundaries() { + use crate::election::election_handler::VoteRpc; + + assert!(!VoteRpc::Vote.peer_term_shows_behind(5, 4)); + assert!(!VoteRpc::Vote.peer_term_shows_behind(5, 5)); + assert!(VoteRpc::Vote.peer_term_shows_behind(5, 6)); + + assert!(!VoteRpc::PreVote.peer_term_shows_behind(5, 4)); + assert!(VoteRpc::PreVote.peer_term_shows_behind(5, 5)); + assert!(VoteRpc::PreVote.peer_term_shows_behind(5, 6)); + } + + /// Test: a peer whose log is more recent than the candidate's fails the PreVote. + /// + /// Expected: + /// - ElectionError::QuorumFailure with only the candidate's own vote, same as the real + /// vote: the candidate could not win, so it must not start a real election. + #[tokio::test] + async fn test_broadcast_pre_vote_requests_rejects_when_peer_log_is_more_recent() { + let mut transport = MockTransport::new(); + transport.expect_send_pre_vote_request().times(1).returning(|_, request, _, _| { + Ok(VoteResponse { + term: request.term - 1, + vote_granted: false, + last_log_index: 9, + last_log_term: 3, + }) + }); + + let e = run_pre_vote(pre_vote_membership(vec![2]), transport, 2).await.unwrap_err(); + + assert!( + matches!( + e, + Error::Consensus(ConsensusError::Election(ElectionError::QuorumFailure { + required: 2, + succeed: 1, + })) + ), + "expected QuorumFailure with only the candidate's own vote, got: {e:?}" + ); + } + + // ======================================================================== + // test_handle_pre_vote_request_* - PreVote (receiver side) + // + // The receiver answers "would you vote for me at request.term?" and records nothing. The + // raft log mock only allows `last_log_id`, so any persistence call would panic the test. + // ======================================================================== + + /// Our node: term 3, last log index 5 / term 2. + const RECEIVER_TERM: u64 = 3; + + fn receiver_log() -> Option { + Some(LogId { index: 5, term: 2 }) + } + + fn ask( + request_term: u64, + request_log: (u64, u64), + leader_active: bool, + ) -> VoteResponse { + let mut raft_log = MockRaftLog::new(); + raft_log.expect_last_log_id().returning(receiver_log); + ElectionHandler::::new(1).handle_pre_vote_request( + VoteRequest { + term: request_term, + candidate_id: 9, + last_log_index: request_log.0, + last_log_term: request_log.1, + }, + RECEIVER_TERM, + &Arc::new(raft_log), + leader_active, + ) + } + + /// Test: a PreVote for a free term from a requester with an equal log is granted. + /// + /// Expected: + /// - Granted. "At least as up-to-date" includes an equal log: with identical logs everywhere + /// (the steady state) nobody could otherwise ever win an election. + #[test] + fn test_handle_pre_vote_request_grants_for_free_term_and_equal_log() { + let response = ask(RECEIVER_TERM + 1, (5, 2), false); + + assert!(response.vote_granted); + } + + /// Test: a requester whose log is strictly newer is granted (higher term, or same term and + /// higher index). + #[test] + fn test_handle_pre_vote_request_grants_when_requester_log_is_newer() { + assert!( + ask(RECEIVER_TERM + 1, (1, 3), false).vote_granted, + "higher last term" + ); + assert!( + ask(RECEIVER_TERM + 1, (9, 2), false).vote_granted, + "same term, higher index" + ); + } + + /// Test: a requester whose log is older is denied (lower last term, or same term and lower + /// index). The last term decides first: a longer log with a lower term is still older. + #[test] + fn test_handle_pre_vote_request_denies_when_requester_log_is_older() { + assert!( + !ask(RECEIVER_TERM + 1, (99, 1), false).vote_granted, + "lower last term" + ); + assert!( + !ask(RECEIVER_TERM + 1, (4, 2), false).vote_granted, + "same term, lower index" + ); + } + + /// Test: while the caller has evidence of a live leader, every PreVote is denied. + /// + /// Scenario: + /// - A node returning from a partition asks about a free term with a perfectly good log. + /// + /// Expected: + /// - Denied. This is what stops it from becoming a candidate and disrupting a healthy + /// leader. + #[test] + fn test_handle_pre_vote_request_denies_while_leader_is_active() { + let response = ask(RECEIVER_TERM + 1, (9, 9), true); + + assert!(!response.vote_granted); + } + + /// Test: a PreVote for a term that is already taken (not greater than ours) is denied. + #[test] + fn test_handle_pre_vote_request_denies_when_term_is_not_greater() { + assert!( + !ask(RECEIVER_TERM, (9, 9), false).vote_granted, + "equal term" + ); + assert!( + !ask(RECEIVER_TERM - 1, (9, 9), false).vote_granted, + "lower term" + ); + } + + /// Test: the response always carries our own term and last log id, whether granted or + /// denied, so a requester that is behind can see it and catch up. + #[test] + fn test_handle_pre_vote_request_response_carries_own_term_and_log() { + for response in [ + ask(RECEIVER_TERM + 1, (5, 2), false), // granted + ask(RECEIVER_TERM + 1, (5, 2), true), // denied: leader active + ask(RECEIVER_TERM, (5, 2), false), // denied: term taken + ] { + assert_eq!(response.term, RECEIVER_TERM); + assert_eq!(response.last_log_index, 5); + assert_eq!(response.last_log_term, 2); + } + } + + /// Test: with an empty local log, a requester with an empty log is granted. + #[test] + fn test_handle_pre_vote_request_empty_local_log_grants_empty_requester_log() { + let mut raft_log = MockRaftLog::new(); + raft_log.expect_last_log_id().returning(|| None); + + let response = ElectionHandler::::new(1).handle_pre_vote_request( + VoteRequest { + term: 2, + candidate_id: 9, + last_log_index: 0, + last_log_term: 0, + }, + 1, + &Arc::new(raft_log), + false, + ); + + assert!(response.vote_granted); + assert_eq!((response.last_log_index, response.last_log_term), (0, 0)); + } + + // ======================================================================== + // test_handle_vote_request_stale_vote_* - a vote cast in an OLDER term must not block + // a vote in the current term (Raft: on a new term, votedFor is null). + // raft-rs `reset(term)` clears the vote whenever the term changes; openraft replaces the + // whole (term, node) vote. + // ======================================================================== + + /// Test: a stale vote from an older term does not block a legitimate candidate in the + /// current term. + /// + /// Scenario: + /// - The node voted for node 3 in term 1. + /// - A request at term 2 from a candidate with a bad log made it adopt term 2 and deny it + /// (adopting the term does not cast a vote). + /// - Now a candidate with a good log asks for a vote in the same term 2. + /// + /// Expected: + /// - Granted: the node has not voted in term 2. Denying here costs the candidate a whole + /// election round (it retries at term 3) for no safety reason. + #[tokio::test] + async fn test_handle_vote_request_stale_vote_from_older_term_does_not_block_current_term() { + let handler = ElectionHandler::::new(1); + let raft_log = Arc::new(create_mock_raft_log(Some(LogId { index: 1, term: 1 }))); + + let state_update = handler + .handle_vote_request( + VoteRequest { + term: 2, + candidate_id: 5, + last_log_index: 1, + last_log_term: 1, + }, + 2, // current term: already adopted + Some(VotedFor { + voted_for_id: 3, + voted_for_term: 1, // cast in an older term + committed: false, + }), + &raft_log, + ) + .await + .unwrap(); + + assert!( + state_update.new_voted_for.is_some(), + "a vote from term 1 must not block a vote in term 2, got: {state_update:?}" + ); + } + + /// Test: a vote already cast for ANOTHER candidate in the current term still blocks. + /// (The boundary of the test above: only the term of the old vote matters.) + #[tokio::test] + async fn test_handle_vote_request_vote_for_other_candidate_in_current_term_still_blocks() { + let handler = ElectionHandler::::new(1); + let raft_log = Arc::new(create_mock_raft_log(Some(LogId { index: 1, term: 1 }))); + + let state_update = handler + .handle_vote_request( + VoteRequest { + term: 2, + candidate_id: 5, + last_log_index: 1, + last_log_term: 1, + }, + 2, + Some(VotedFor { + voted_for_id: 3, + voted_for_term: 2, // same term, other candidate + committed: false, + }), + &raft_log, + ) + .await + .unwrap(); + + assert!(state_update.new_voted_for.is_none()); + } + + /// Test: PreVote and the real vote agree whenever nothing else differs. + /// + /// Property (checked over a grid of terms and logs): for a request at a term above ours, + /// with no leader active and no vote cast, `handle_pre_vote_request` grants exactly when + /// `handle_vote_request` would. + /// + /// - A PreVote that grants what the real vote denies sends the node into a real election it + /// cannot win, bumping terms for nothing (the thing PreVote exists to prevent). + /// - A PreVote that denies what the real vote would grant makes a node that could win + /// unable to start an election: a liveness hole after a leader failure. + #[tokio::test] + async fn test_pre_vote_grants_exactly_when_the_real_vote_would() { + let handler = ElectionHandler::::new(1); + let logs = [None, Some((1, 1)), Some((5, 2)), Some((9, 2)), Some((3, 3))]; + + for current_term in [1u64, 3, 7] { + for my_log in logs { + for request_term in [current_term + 1, current_term + 5] { + for requester_log in [(0, 0), (1, 1), (5, 2), (9, 2), (3, 3), (2, 4)] { + let my_log_id = my_log.map(|(index, term)| LogId { index, term }); + let raft_log = Arc::new(create_mock_raft_log(my_log_id)); + let request = VoteRequest { + term: request_term, + candidate_id: 9, + last_log_index: requester_log.0, + last_log_term: requester_log.1, + }; + + let pre_vote = handler + .handle_pre_vote_request(request, current_term, &raft_log, false) + .vote_granted; + let real_vote = handler + .handle_vote_request(request, current_term, None, &raft_log) + .await + .unwrap() + .new_voted_for + .is_some(); + + assert_eq!( + pre_vote, real_vote, + "current_term={current_term} my_log={my_log:?} request_term=\ + {request_term} requester_log={requester_log:?}: PreVote and the real \ + vote must agree" + ); + } + } + } + } + } + + // ======================================================================== + // vote_from_any_state (raft-rs): a request with a higher term is adopted whether or not the + // vote can be granted; a request with the same or a lower term is never adopted. + // ======================================================================== + + /// Test: a higher-term request whose log is stale is denied but its term is still adopted. + /// + /// Expected: + /// - `term_update == Some(request.term)` and no vote recorded (Raft §5.1; the caller persists + /// the term and, for a role change, steps down). + #[tokio::test] + async fn test_handle_vote_request_denied_for_stale_log_still_adopts_higher_term() { + let handler = ElectionHandler::::new(1); + let raft_log = Arc::new(create_mock_raft_log(Some(LogId { index: 9, term: 3 }))); + + let state_update = handler + .handle_vote_request( + VoteRequest { + term: 5, + candidate_id: 2, + last_log_index: 1, + last_log_term: 1, // far behind ours + }, + 2, + None, + &raft_log, + ) + .await + .unwrap(); + + assert!( + state_update.new_voted_for.is_none(), + "the vote must be denied" + ); + assert_eq!( + state_update.term_update, + Some(5), + "the higher term must still be adopted" + ); + } + + /// Test: a request with the same or a lower term never changes our term. + #[tokio::test] + async fn test_handle_vote_request_same_or_lower_term_is_never_adopted() { + let handler = ElectionHandler::::new(1); + let raft_log = Arc::new(create_mock_raft_log(Some(LogId { index: 1, term: 1 }))); + + for request_term in [4, 3] { + let state_update = handler + .handle_vote_request( + VoteRequest { + term: request_term, + candidate_id: 2, + last_log_index: 1, + last_log_term: 1, + }, + 4, + None, + &raft_log, + ) + .await + .unwrap(); + assert_eq!( + state_update.term_update, None, + "request term {request_term}" + ); + } + } } diff --git a/d-engine-core/src/election/mod.rs b/d-engine-core/src/election/mod.rs index 25efbdcc..94d91d75 100644 --- a/d-engine-core/src/election/mod.rs +++ b/d-engine-core/src/election/mod.rs @@ -7,16 +7,12 @@ mod election_handler; pub use election_handler::*; -#[cfg(test)] -mod election_handler_test; - -use std::sync::Arc; - use async_trait::async_trait; use d_engine_proto::server::election::VoteRequest; use d_engine_proto::server::election::VotedFor; #[cfg(any(test, feature = "__test_support"))] use mockall::automock; +use std::sync::Arc; use crate::RaftNodeConfig; use crate::Result; @@ -40,6 +36,18 @@ pub trait ElectionCore: Send + Sync + 'static where T: TypeConfig, { + /// Sends PreVote requests ("would you vote for me at `term`?") to all voting members. + /// Returns Ok() if a majority would grant. Changes no term/vote state on either side. + /// `term` is the hypothetical next term (current_term + 1), not yet persisted. + async fn broadcast_pre_vote_requests( + &self, + term: u64, + membership: Arc>, + raft_log: &Arc>, + transport: &Arc>, + settings: &Arc, + ) -> Result<()>; + /// Sends vote requests to all voting members. Returns Ok() if majority /// votes are received, otherwise returns Err. Initiates RPC calls via /// transport and evaluates collected responses. @@ -58,6 +66,26 @@ where settings: &Arc, ) -> Result<()>; + /// Answers a PreVote ("would you vote for me at `request.term`?") without changing any + /// state. It returns the response itself, not a `StateUpdate`: nothing is recorded, so + /// no term, vote or timer can be touched through this call. + /// + /// Denies when any of these holds: + /// - `leader_active`: the caller still has evidence of a live leader (Follower: recent + /// leader contact; Leader: recent quorum ACK; Candidate: false). + /// - `request.term <= current_term`: that term is already taken. + /// - The requester's log is not at least as up-to-date as ours (Raft §5.4.1). + /// + /// The response carries our own `current_term` and last log id, so a requester that is + /// behind can catch up from a denial. + fn handle_pre_vote_request( + &self, + request: VoteRequest, + current_term: u64, + raft_log: &Arc>, + leader_active: bool, + ) -> d_engine_proto::server::election::VoteResponse; + /// Processes incoming vote requests: validates request legality via /// check_vote_request_is_legal, updates node state if valid, triggers /// role transition to Follower when granting vote. diff --git a/d-engine-core/src/event.rs b/d-engine-core/src/event.rs index be59beff..6b446f9e 100644 --- a/d-engine-core/src/event.rs +++ b/d-engine-core/src/event.rs @@ -177,6 +177,11 @@ pub enum InboundEvent { MaybeCloneOneshotSender>, ), + ReceivePreVoteRequest( + VoteRequest, + MaybeCloneOneshotSender>, + ), + ClusterConf( MetadataRequest, MaybeCloneOneshotSender>, @@ -222,6 +227,8 @@ pub enum InboundEvent { pub enum TestEvent { ReceiveVoteRequest(VoteRequest), + ReceivePreVoteRequest(VoteRequest), + ClusterConf(MetadataRequest), ClusterConfUpdate(ClusterConfChangeRequest), @@ -261,6 +268,8 @@ pub enum TestEvent { #[cfg(test)] pub(crate) fn inbound_event_to_test_event(event: &InboundEvent) -> TestEvent { match event { + InboundEvent::ReceivePreVoteRequest(req, _) => TestEvent::ReceivePreVoteRequest(*req), + InboundEvent::ReceiveVoteRequest(req, _) => TestEvent::ReceiveVoteRequest(*req), InboundEvent::ClusterConf(req, _) => TestEvent::ClusterConf(*req), InboundEvent::ClusterConfUpdate(req, _) => TestEvent::ClusterConfUpdate(req.clone()), diff --git a/d-engine-core/src/lib.rs b/d-engine-core/src/lib.rs index 387324b2..07faf6e3 100644 --- a/d-engine-core/src/lib.rs +++ b/d-engine-core/src/lib.rs @@ -187,3 +187,14 @@ pub(crate) fn is_target_log_more_recent( (target_last_log_term > my_last_log_term) || (target_last_log_term == my_last_log_term && target_last_log_index >= my_last_log_index) } + +/// True only if the target's log is strictly more up-to-date than ours: +/// a higher last term, or the same last term and a higher last index. +pub(crate) fn is_target_log_strictly_more_recent( + my_last_log_index: u64, + my_last_log_term: u64, + target_last_log_index: u64, + target_last_log_term: u64, +) -> bool { + (target_last_log_term, target_last_log_index) > (my_last_log_term, my_last_log_index) +} diff --git a/d-engine-core/src/network/mod.rs b/d-engine-core/src/network/mod.rs index 05006e56..3b85d8a3 100644 --- a/d-engine-core/src/network/mod.rs +++ b/d-engine-core/src/network/mod.rs @@ -158,6 +158,16 @@ where membership: std::sync::Arc>, ) -> Result; + /// Sends a PreVote request to a single peer (Raft thesis §9.6). Neither side changes + /// any state. Candidate-exclusive; the caller fans this out per-peer and tallies. + async fn send_pre_vote_request( + &self, + peer_id: u32, + request: VoteRequest, + retry: &RetryPolicies, + membership: std::sync::Arc>, + ) -> Result; + /// Orchestrates log compaction across cluster peers after snapshot creation. /// /// # Protocol diff --git a/d-engine-core/src/raft.rs b/d-engine-core/src/raft.rs index 98e268a9..2d5a1c19 100644 --- a/d-engine-core/src/raft.rs +++ b/d-engine-core/src/raft.rs @@ -497,9 +497,6 @@ where new_role.restore_pending_acks(withheld_acks); self.role = new_role; - // Reset vote when stepping down (new term, no vote yet) - self.role.state_mut().commit_vote_reset(&self.ctx)?; - // Notify leader change listeners let current_term = self.role.current_term(); self.notify_leader_change(leader_id_option, current_term); @@ -671,7 +668,8 @@ where self.notify_leader_change(Some(self.node_id), term); } InternalEvent::FatalError { source, error } => { - error!(%self.node_id, %source, %error, "Fatal error from SM worker — shutting down"); + error!(%self.node_id, %source, %error, "Fatal error from {source} — shutting down"); + return Err(crate::Error::Fatal(format!("{source}: {error}"))); } InternalEvent::ApplyCompleted { @@ -918,7 +916,7 @@ where } pub fn read_lease(&self) -> Arc { - Arc::clone(&self.role.state().shared_state().lease) + Arc::clone(&self.role.state().shared_state().read_lease) } pub fn current_term(&self) -> u64 { diff --git a/d-engine-core/src/raft_role/candidate_state.rs b/d-engine-core/src/raft_role/candidate_state.rs index 2ea1c450..b6329f44 100644 --- a/d-engine-core/src/raft_role/candidate_state.rs +++ b/d-engine-core/src/raft_role/candidate_state.rs @@ -153,7 +153,53 @@ impl RaftRoleState for CandidateState { debug!("candidate::start_election..."); - let new_term = self.current_term() + 1; + if !self.is_active_voter(ctx) { + debug!("candidate: not an active voter, skipping election"); + return Ok(()); + } + + // The timer is already re-armed above, so skipping here waits a full election timeout. + match self.has_unapplied_config_change(ctx) { + Ok(false) => {} + Ok(true) => { + debug!( + "candidate: a committed membership change is not applied yet, skipping election" + ); + return Ok(()); + } + Err(e) => { + warn!("candidate: cannot read the unapplied log range ({e:?}), skipping election"); + return Ok(()); + } + } + + let pre_vote_term = self.current_term() + 1; + match ctx + .election_handler() + .broadcast_pre_vote_requests( + pre_vote_term, + ctx.membership(), + ctx.raft_log(), + ctx.transport(), + &ctx.node_config(), + ) + .await + { + Ok(_) => {} + Err(Error::Consensus(ConsensusError::Election(ElectionError::HigherTerm( + higher_term, + )))) => { + self.commit_hard_state(ctx, Some(higher_term), None)?; + self.send_become_follower_event(internal_event_tx)?; + return Ok(()); + } + Err(e) => { + info!("candidate broadcast_pre_vote_requests with error: {:?}", e); + return Ok(()); + } + } + + let new_term = pre_vote_term; self.commit_hard_state( ctx, Some(new_term), @@ -223,6 +269,20 @@ impl RaftRoleState for CandidateState { ) -> Result<()> { let my_term = self.current_term(); match inbound_event { + InboundEvent::ReceivePreVoteRequest(vote_request, sender) => { + // A Candidate has no leader, so no leader is ever active here. + let response = ctx.election_handler().handle_pre_vote_request( + vote_request, + my_term, + ctx.raft_log(), + false, + ); + sender.send(Ok(response)).map_err(|e| { + error!("Failed to send: {:?}", e); + NetworkError::SingalSendFailed(format!("{:?}", e)) + })?; + } + InboundEvent::ReceiveVoteRequest(vote_request, sender) => { debug!( "handle_inbound_event::InboundEvent::ReceiveVoteRequest: {:?}", @@ -234,16 +294,19 @@ impl RaftRoleState for CandidateState { term: last_log_term, } = ctx.raft_log().last_log_id().unwrap_or(LogId { index: 0, term: 0 }); - if ctx.election_handler().check_vote_request_is_legal( - &vote_request, - my_term, - last_log_index, - last_log_term, - self.voted_for().unwrap(), - ) { + // Step down when the request carries a higher term (Raft §5.1, whether or not + // the vote can be granted), or when it is a same-term request this node can + // still grant because it has not voted in its current term yet. + if vote_request.term > my_term + || ctx.election_handler().check_vote_request_is_legal( + &vote_request, + my_term, + last_log_index, + last_log_term, + self.voted_for().unwrap(), + ) + { self.commit_hard_state(ctx, Some(vote_request.term), None)?; - - // Step down as Follower self.send_become_follower_event(&internal_event_tx)?; info!( @@ -524,7 +587,7 @@ impl From<&FollowerState> for CandidateState { trace!(%follower.node_config.raft.election.election_timeout_min, "From<&FollowerState> for CandidateState"); Self { shared_state: follower.shared_state.clone(), - timer: ElectionTimer::new(( + timer: ElectionTimer::expired(( follower.node_config.raft.election.election_timeout_min, follower.node_config.raft.election.election_timeout_max, )), 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 accb688c..4d449e77 100644 --- a/d-engine-core/src/raft_role/candidate_state_test.rs +++ b/d-engine-core/src/raft_role/candidate_state_test.rs @@ -175,6 +175,9 @@ async fn test_tick_triggers_new_election_round_on_success() { // Mock election_handler to succeed let mut election_handler = MockElectionCore::::new(); + election_handler + .expect_broadcast_pre_vote_requests() + .returning(|_, _, _, _, _| Ok(())); election_handler .expect_broadcast_vote_requests() .times(1) @@ -250,6 +253,9 @@ async fn test_tick_election_timeout_persists_hard_state_before_broadcasting_vote let nc = node_config(_temp_dir.path().to_str().unwrap()); let mut election_core = MockElectionCore::::new(); + election_core + .expect_broadcast_pre_vote_requests() + .returning(|_, _, _, _, _| Ok(())); election_core .expect_broadcast_vote_requests() .times(1) @@ -307,6 +313,9 @@ async fn test_tick_discovers_higher_term_and_steps_down() { // Mock election_handler to return HigherTerm error let mut election_handler = MockElectionCore::::new(); + election_handler + .expect_broadcast_pre_vote_requests() + .returning(|_, _, _, _, _| Ok(())); election_handler .expect_broadcast_vote_requests() .times(1) @@ -409,6 +418,9 @@ async fn test_tick_discovers_higher_term_via_election_persists_hard_state() { let nc = node_config(_temp_dir.path().to_str().unwrap()); let mut election_core = MockElectionCore::::new(); + election_core + .expect_broadcast_pre_vote_requests() + .returning(|_, _, _, _, _| Ok(())); election_core.expect_broadcast_vote_requests().returning(|_, _, _, _, _| { Err(Error::Consensus(ConsensusError::Election( ElectionError::HigherTerm(100), @@ -1765,3 +1777,759 @@ fn test_candidate_from_follower_preserves_last_purged_index() { Some(LogId { term: 1, index: 9 }) ); } + +// ============================================================================ +// PreVote in Candidate::tick +// +// Before bumping its term, a candidate asks the peers whether they would vote for it at +// term+1. A node that cannot win (isolated, or a leader is alive) must keep its term, or on +// reconnect its inflated term would force a healthy leader to step down. +// ============================================================================ + +/// Builds a candidate whose election timer has already expired, with the given election mock +/// and raft log. Returns the state, its context and the internal-event receiver. +async fn expired_candidate( + election_core: MockElectionCore, + raft_log: MockRaftLog, +) -> ( + CandidateState, + crate::RaftContext, + mpsc::UnboundedReceiver, + mpsc::UnboundedSender, +) { + let (_graceful_tx, graceful_rx) = watch::channel(()); + 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) + .with_election_handler(election_core) + .build_context(); + let state = CandidateState::::new(1, context.node_config.clone()); + let (internal_event_tx, internal_event_rx) = mpsc::unbounded_channel(); + + let election_timeout_max = context.node_config.raft.election.election_timeout_max; + tokio::time::advance(tokio::time::Duration::from_millis(election_timeout_max + 1)).await; + (state, context, internal_event_rx, internal_event_tx) +} + +/// Test: a failed PreVote keeps the term unchanged and starts no real election. +/// +/// Scenario: +/// - Candidate (term 1) times out; the PreVote round does not reach a majority +/// (e.g. the node is isolated). +/// +/// Expected: +/// - Term stays 1, nothing is persisted (a MockRaftLog without expectations panics on any +/// `save_hard_state`), no RequestVote is broadcast, no role-change event is sent. +/// - The timer was already reset, so the next attempt happens after the next timeout. +#[tokio::test(start_paused = true)] +async fn test_tick_pre_vote_failure_keeps_term_and_skips_real_election() { + let mut election_core = MockElectionCore::::new(); + election_core + .expect_broadcast_pre_vote_requests() + .times(1) + .returning(|_, _, _, _, _| { + Err(Error::Consensus(ConsensusError::Election( + ElectionError::QuorumFailure { + required: 3, + succeed: 1, + }, + ))) + }); + election_core.expect_broadcast_vote_requests().times(0); + + let (mut state, context, mut internal_event_rx, internal_event_tx) = + expired_candidate(election_core, MockRaftLog::new()).await; + let (event_tx, _event_rx) = mpsc::channel(1); + + assert!(state.tick(&internal_event_tx, &event_tx, &context).await.is_ok()); + + assert_eq!( + state.current_term(), + 1, + "a failed PreVote must not bump the term" + ); + assert!( + internal_event_rx.try_recv().is_err(), + "a failed PreVote must not trigger any role change" + ); +} + +/// Test: PreVote asks about term+1 and runs BEFORE the term is persisted. +/// +/// Expected: +/// - `broadcast_pre_vote_requests` receives term 2 (current 1 + 1). +/// - `save_hard_state` (term 2 + self-vote) happens only after the PreVote returned. +/// - Then the real election runs for term 2 and the node becomes leader. +#[tokio::test(start_paused = true)] +async fn test_tick_pre_vote_runs_for_next_term_before_persisting_it() { + use std::sync::atomic::{AtomicBool, Ordering}; + let pre_vote_done = Arc::new(AtomicBool::new(false)); + + let mut election_core = MockElectionCore::::new(); + let flag = Arc::clone(&pre_vote_done); + election_core + .expect_broadcast_pre_vote_requests() + .withf(|term, _, _, _, _| *term == 2) + .times(1) + .returning(move |_, _, _, _, _| { + flag.store(true, Ordering::SeqCst); + Ok(()) + }); + election_core + .expect_broadcast_vote_requests() + .withf(|term, _, _, _, _| *term == 2) + .times(1) + .returning(|_, _, _, _, _| Ok(())); + + let mut raft_log = MockRaftLog::new(); + let flag = Arc::clone(&pre_vote_done); + raft_log + .expect_save_hard_state() + .withf(move |s| { + assert!( + flag.load(Ordering::SeqCst), + "the term must not be persisted before the PreVote round finished" + ); + s.current_term == 2 + }) + .times(1) + .returning(|_| Ok(())); + + let (mut state, context, mut internal_event_rx, internal_event_tx) = + expired_candidate(election_core, raft_log).await; + let (event_tx, _event_rx) = mpsc::channel(1); + + assert!(state.tick(&internal_event_tx, &event_tx, &context).await.is_ok()); + + assert_eq!(state.current_term(), 2); + assert!(matches!( + internal_event_rx.try_recv(), + Ok(InternalEvent::BecomeLeader) + )); +} + +/// Test: a PreVote that discovers a higher term catches up without starting an election. +/// +/// Scenario: +/// - The peers answer the PreVote with term 100 (the cluster moved on while this node was away). +/// +/// Expected: +/// - The higher term is persisted BEFORE `BecomeFollower` is sent, the node ends in term 100, +/// and no RequestVote is broadcast. +#[tokio::test(start_paused = true)] +async fn test_tick_pre_vote_higher_term_catches_up_without_real_election() { + let mut election_core = MockElectionCore::::new(); + election_core + .expect_broadcast_pre_vote_requests() + .times(1) + .returning(|_, _, _, _, _| { + Err(Error::Consensus(ConsensusError::Election( + ElectionError::HigherTerm(100), + ))) + }); + election_core.expect_broadcast_vote_requests().times(0); + + let (internal_event_tx, internal_event_rx) = mpsc::unbounded_channel(); + let internal_event_rx = Arc::new(std::sync::Mutex::new(internal_event_rx)); + let rx_for_ordering_check = Arc::clone(&internal_event_rx); + + let mut raft_log = MockRaftLog::new(); + raft_log + .expect_save_hard_state() + .withf(move |s| { + assert!( + rx_for_ordering_check.lock().unwrap().try_recv().is_err(), + "save_hard_state must be called BEFORE BecomeFollower is sent" + ); + s.current_term == 100 + }) + .times(1) + .returning(|_| Ok(())); + + let (_graceful_tx, graceful_rx) = watch::channel(()); + 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) + .with_election_handler(election_core) + .build_context(); + let mut state = CandidateState::::new(1, context.node_config.clone()); + let election_timeout_max = context.node_config.raft.election.election_timeout_max; + tokio::time::advance(tokio::time::Duration::from_millis(election_timeout_max + 1)).await; + let (event_tx, _event_rx) = mpsc::channel(1); + + assert!(state.tick(&internal_event_tx, &event_tx, &context).await.is_ok()); + + assert_eq!(state.current_term(), 100); + assert!(matches!( + internal_event_rx.lock().unwrap().try_recv().unwrap(), + InternalEvent::BecomeFollower(_) + )); +} + +// ============================================================================ +// PreVote requests received by a Candidate +// +// A Candidate has no leader (that is why it became one), so it never reports an active leader: +// the handler decides on term and log alone. Like every role it changes nothing. +// ============================================================================ + +/// Test: a Candidate answers a PreVote through the handler with `leader_active == false`, +/// forwards the answer, and changes nothing (term, persistence, role). +#[tokio::test] +async fn test_candidate_pre_vote_reports_no_active_leader_and_changes_nothing() { + let seen = Arc::new(std::sync::atomic::AtomicBool::new(true)); + let mut election_handler = MockElectionCore::::new(); + let seen_in_mock = Arc::clone(&seen); + election_handler.expect_handle_pre_vote_request().times(1).returning( + move |_, current_term, _, leader_active| { + seen_in_mock.store(leader_active, std::sync::atomic::Ordering::SeqCst); + d_engine_proto::server::election::VoteResponse { + term: current_term, + vote_granted: true, + last_log_index: 7, + last_log_term: 3, + } + }, + ); + + let (_graceful_tx, graceful_rx) = watch::channel(()); + // A raft log mock without expectations panics on any `save_hard_state`. + let context = MockBuilder::new(graceful_rx) + .with_raft_log(MockRaftLog::new()) + .with_election_handler(election_handler) + .build_context(); + let mut state = CandidateState::::new(1, context.node_config.clone()); + let term_before = state.current_term(); + + let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + state + .handle_inbound_event( + InboundEvent::ReceivePreVoteRequest( + VoteRequest { + term: term_before + 1, + candidate_id: 2, + last_log_index: 0, + last_log_term: 0, + }, + resp_tx, + ), + &context, + internal_event_tx, + ) + .await + .unwrap(); + + assert!(!seen.load(std::sync::atomic::Ordering::SeqCst)); + let response = resp_rx.recv().await.unwrap().unwrap(); + assert!(response.vote_granted); + assert_eq!(state.current_term(), term_before); + assert!( + internal_event_rx.try_recv().is_err(), + "a PreVote must not change the Candidate's role" + ); +} + +/// Test: a Candidate that receives a HIGHER-term VoteRequest adopts that term and steps down +/// even when it cannot grant the vote. +/// +/// Scenario: +/// - Candidate (term 1) receives a VoteRequest for term 5 from a node whose log is behind, so +/// the request is not legal and the vote cannot be granted. +/// +/// Expected: +/// - The term becomes 5 (persisted), `BecomeFollower` is sent, and the request is replayed to +/// the new Follower, which answers it (here: a denial because of the log). +/// +/// Raft §5.1: "If RPC request or response contains term T > currentTerm: set currentTerm = T, +/// convert to follower", whatever the RPC and whether or not the vote is granted (raft-rs +/// `test_vote_from_any_state`). A Candidate that keeps its lower term and stays a Candidate +/// starts its next election at a term the cluster has already passed. +#[tokio::test] +async fn test_candidate_higher_term_vote_request_with_stale_log_adopts_term_and_steps_down() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let mut context = mock_raft_context( + "/tmp/test_candidate_higher_term_stale_log", + graceful_rx, + None, + ); + + let mut election_core = mock_election_core(); + election_core + .expect_check_vote_request_is_legal() + .returning(|_, _, _, _, _| false); + context.handlers.election_handler = election_core; + + let mut state = CandidateState::::new(1, context.node_config.clone()); + let (resp_tx, _resp_rx) = MaybeCloneOneshot::new(); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + + state + .handle_inbound_event( + InboundEvent::ReceiveVoteRequest( + VoteRequest { + term: 5, + candidate_id: 2, + last_log_index: 0, + last_log_term: 0, + }, + resp_tx, + ), + &context, + internal_event_tx, + ) + .await + .unwrap(); + + assert_eq!(state.current_term(), 5, "the higher term must be adopted"); + assert!(matches!( + internal_event_rx.try_recv(), + Ok(InternalEvent::BecomeFollower(_)) + )); + assert!(matches!( + internal_event_rx.try_recv(), + Ok(InternalEvent::ReprocessEvent(_)) + )); +} + +/// Test: a Candidate that is no longer an active voter (removed, or only a learner) neither sends +/// PreVote nor bumps its term; it must not keep campaigning with a quorum that counts its own +/// vote. `Membership::voters()` never contains this node, so the answer comes from its own entry. +#[tokio::test(start_paused = true)] +async fn test_candidate_tick_does_not_campaign_when_not_an_active_voter() { + for own_status in [None, Some(d_engine_proto::common::NodeStatus::Promotable)] { + let mut election_core = MockElectionCore::::new(); + election_core.expect_broadcast_pre_vote_requests().times(0); + election_core.expect_broadcast_vote_requests().times(0); + + let (mut state, mut context, mut internal_event_rx, internal_event_tx) = + expired_candidate(election_core, MockRaftLog::new()).await; + let mut membership = crate::MockMembership::::new(); + membership.expect_retrieve_node_meta().returning(move |id| { + own_status.map(|status| d_engine_proto::server::cluster::NodeMeta { + id, + address: format!("http://127.0.0.1:{}", 55000 + id), + role: d_engine_proto::common::NodeRole::Follower.into(), + status: status as i32, + }) + }); + membership.expect_is_single_node_cluster().returning(|| false); + context.membership = Arc::new(membership); + let (event_tx, _event_rx) = mpsc::channel(1); + let term_before = state.current_term(); + + state.tick(&internal_event_tx, &event_tx, &context).await.unwrap(); + + assert_eq!( + state.current_term(), + term_before, + "own status {own_status:?}" + ); + assert!( + internal_event_rx.try_recv().is_err(), + "own status {own_status:?}" + ); + } +} + +// ============================================================================ +// No campaign while a committed membership change is still unapplied +// +// A node that has committed but not yet applied a membership change would count votes against +// a stale voter set. raft-rs refuses to campaign in that window +// (`test_conf_change_check_before_campaign`: "cannot campaign since there are still pending +// configuration changes to apply"). +// ============================================================================ + +/// Builds an expired candidate whose log has committed entries up to `commit_index`, of which +/// everything above `applied` is still unapplied; `unapplied_entries` is what the log returns for +/// that range. +async fn candidate_with_unapplied_range( + election_core: MockElectionCore, + commit_index: u64, + applied: u64, + unapplied_entries: Vec, +) -> ( + CandidateState, + crate::RaftContext, + mpsc::UnboundedReceiver, + mpsc::UnboundedSender, +) { + let mut raft_log = MockRaftLog::new(); + let range_start = applied + 1; + raft_log + .expect_get_entries_range() + .withf(move |range| *range.start() == range_start && *range.end() == commit_index) + .returning(move |_| Ok(unapplied_entries.clone())); + raft_log.expect_save_hard_state().returning(|_| Ok(())); + + let (_graceful_tx, graceful_rx) = watch::channel(()); + let temp_dir = tempfile::tempdir().unwrap(); + let nc = node_config(temp_dir.path().to_str().unwrap()); + let mut state_machine = crate::MockStateMachine::new(); + state_machine.expect_is_running().returning(|| true); + state_machine.expect_last_applied().return_const(d_engine_proto::common::LogId { + index: applied, + term: 1, + }); + let context = MockBuilder::new(graceful_rx) + .with_raft_log(raft_log) + .with_node_config(nc) + .with_state_machine(state_machine) + .with_election_handler(election_core) + .build_context(); + let mut state = CandidateState::::new(1, context.node_config.clone()); + state.shared_state_mut().commit_index = commit_index; + let (internal_event_tx, internal_event_rx) = mpsc::unbounded_channel(); + let max = context.node_config.raft.election.election_timeout_max; + tokio::time::advance(tokio::time::Duration::from_millis(max + 1)).await; + (state, context, internal_event_rx, internal_event_tx) +} + +fn config_change_entry(index: u64) -> d_engine_proto::common::Entry { + d_engine_proto::common::Entry { + index, + term: 1, + payload: Some(d_engine_proto::common::EntryPayload::config( + d_engine_proto::common::membership_change::Change::AddNode( + d_engine_proto::common::AddNode { + node_id: 4, + address: "127.0.0.1:5004".to_string(), + status: d_engine_proto::common::NodeStatus::Active as i32, + }, + ), + )), + } +} + +/// Test: committed-but-unapplied membership change in the log => no PreVote, no term bump. +#[tokio::test(start_paused = true)] +async fn test_candidate_tick_does_not_campaign_with_unapplied_config_change() { + let mut election_core = MockElectionCore::::new(); + election_core.expect_broadcast_pre_vote_requests().times(0); + election_core.expect_broadcast_vote_requests().times(0); + let (mut state, context, mut internal_event_rx, internal_event_tx) = + candidate_with_unapplied_range(election_core, 10, 5, vec![config_change_entry(8)]).await; + let (event_tx, _event_rx) = mpsc::channel(1); + let term_before = state.current_term(); + + state.tick(&internal_event_tx, &event_tx, &context).await.unwrap(); + + assert_eq!(state.current_term(), term_before); + assert!(internal_event_rx.try_recv().is_err()); +} + +/// Test: committed-but-unapplied entries that are NOT membership changes do not block an +/// election (otherwise a busy cluster could never elect a leader). +#[tokio::test(start_paused = true)] +async fn test_candidate_tick_campaigns_when_unapplied_entries_are_not_config_changes() { + let mut election_core = MockElectionCore::::new(); + election_core + .expect_broadcast_pre_vote_requests() + .times(1) + .returning(|_, _, _, _, _| Ok(())); + election_core + .expect_broadcast_vote_requests() + .times(1) + .returning(|_, _, _, _, _| Ok(())); + let command = d_engine_proto::common::Entry { + index: 8, + term: 1, + payload: Some(d_engine_proto::common::EntryPayload::noop()), + }; + let (mut state, context, _internal_event_rx, internal_event_tx) = + candidate_with_unapplied_range(election_core, 10, 5, vec![command]).await; + let (event_tx, _event_rx) = mpsc::channel(1); + + state.tick(&internal_event_tx, &event_tx, &context).await.unwrap(); + + assert_eq!( + state.current_term(), + 2, + "an ordinary backlog must not block the election" + ); +} + +/// Test: with nothing unapplied (applied == commit) there is nothing to check and no log read. +#[tokio::test(start_paused = true)] +async fn test_candidate_tick_campaigns_when_everything_committed_is_applied() { + let mut election_core = MockElectionCore::::new(); + election_core + .expect_broadcast_pre_vote_requests() + .times(1) + .returning(|_, _, _, _, _| Ok(())); + election_core + .expect_broadcast_vote_requests() + .times(1) + .returning(|_, _, _, _, _| Ok(())); + let (mut state, context, _internal_event_rx, internal_event_tx) = + candidate_with_unapplied_range(election_core, 10, 10, vec![]).await; + let (event_tx, _event_rx) = mpsc::channel(1); + + state.tick(&internal_event_tx, &event_tx, &context).await.unwrap(); + + assert_eq!(state.current_term(), 2); +} + +// ============================================================================ +// Election latency: becoming a Candidate must not cost a second election timeout +// +// Follower::tick already waited one full election timeout before sending BecomeCandidate. The +// new Candidate used to start with a fresh random timer, so the first real election round began +// only after ANOTHER election timeout: failover took 1-2 timeouts longer than necessary. etcd and +// raft-rs campaign at the moment the first timeout fires. (Present on main, before PreVote.) +// ============================================================================ + +/// Test: a Candidate created from a timed-out Follower is due to campaign immediately. +#[tokio::test(start_paused = true)] +async fn test_candidate_from_follower_is_due_to_campaign_immediately() { + let follower = FollowerState::::new( + 1, + Arc::new(node_config("/tmp/test_candidate_from_follower_is_due")), + None, + None, + ); + + let candidate = CandidateState::::from(&follower); + + assert!( + candidate.is_timer_expired(), + "the first election round must not wait for a second election timeout" + ); +} + +/// Test: after a round has run (win, loss or PreVote failure) the next attempt is NOT immediate: +/// the timer is re-armed with a fresh random timeout, otherwise a node that keeps losing would +/// spin and hammer the cluster. +#[tokio::test(start_paused = true)] +async fn test_candidate_tick_rearms_the_timer_after_a_round() { + let mut election_core = MockElectionCore::::new(); + election_core.expect_broadcast_pre_vote_requests().returning(|_, _, _, _, _| { + Err(Error::Consensus(ConsensusError::Election( + ElectionError::QuorumFailure { + required: 3, + succeed: 1, + }, + ))) + }); + let (mut state, context, _internal_event_rx, internal_event_tx) = + expired_candidate(election_core, MockRaftLog::new()).await; + let (event_tx, _event_rx) = mpsc::channel(1); + + state.tick(&internal_event_tx, &event_tx, &context).await.unwrap(); + + assert!( + !state.is_timer_expired(), + "the next round must wait for a new election timeout" + ); +} + +/// Test: a Candidate that has NOT voted in its current term still grants a same-term request. +/// +/// Scenario: +/// - The Candidate has not cast its self-vote yet (its first election round has not run), so it +/// holds no vote in the current term. +/// - Another candidate asks for a vote in the SAME term, with a good log. +/// +/// Expected: +/// - The Candidate steps down and replays the request to the Follower, which can grant it. +/// Rejecting every same-term request would waste the votes this node still has to give. +#[tokio::test] +async fn test_candidate_without_a_vote_in_its_term_steps_down_for_a_legal_same_term_request() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let mut context = mock_raft_context("/tmp/test_candidate_same_term_legal", graceful_rx, None); + let mut election_core = mock_election_core(); + election_core + .expect_check_vote_request_is_legal() + .returning(|_, _, _, _, _| true); + context.handlers.election_handler = election_core; + + let mut state = CandidateState::::new(1, context.node_config.clone()); + let term = state.current_term(); + let (resp_tx, _resp_rx) = MaybeCloneOneshot::new(); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + + state + .handle_inbound_event( + InboundEvent::ReceiveVoteRequest( + VoteRequest { + term, // same term, not higher + candidate_id: 2, + last_log_index: 0, + last_log_term: 0, + }, + resp_tx, + ), + &context, + internal_event_tx, + ) + .await + .unwrap(); + + assert!(matches!( + internal_event_rx.try_recv(), + Ok(InternalEvent::BecomeFollower(_)) + )); + assert!(matches!( + internal_event_rx.try_recv(), + Ok(InternalEvent::ReprocessEvent(_)) + )); +} + +/// Test: a LOWER-term VoteRequest is rejected with our own term and changes nothing. +#[tokio::test] +async fn test_candidate_lower_term_vote_request_is_rejected_without_changes() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let mut context = mock_raft_context("/tmp/test_candidate_lower_term_vote", graceful_rx, None); + let mut election_core = mock_election_core(); + election_core + .expect_check_vote_request_is_legal() + .returning(|_, _, _, _, _| false); + context.handlers.election_handler = election_core; + + let mut state = CandidateState::::new(1, context.node_config.clone()); + state.shared_state_mut().update_current_term(5); + let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + + state + .handle_inbound_event( + InboundEvent::ReceiveVoteRequest( + VoteRequest { + term: 3, + candidate_id: 2, + last_log_index: 0, + last_log_term: 0, + }, + resp_tx, + ), + &context, + internal_event_tx, + ) + .await + .unwrap(); + + let response = resp_rx.recv().await.unwrap().unwrap(); + assert!(!response.vote_granted); + assert_eq!( + response.term, 5, + "the reply must carry our own, higher term" + ); + assert_eq!(state.current_term(), 5); + assert!( + internal_event_rx.try_recv().is_err(), + "no role change for a stale request" + ); +} + +/// Test: the higher term is persisted BEFORE `BecomeFollower` is sent. +/// +/// A crash between "step down" and "persist" would resurrect the stale term on restart, and the +/// node could then vote in a term it already gave up on. +#[tokio::test] +async fn test_candidate_higher_term_vote_request_persists_term_before_stepping_down() { + let (internal_event_tx, internal_event_rx) = mpsc::unbounded_channel(); + let internal_event_rx = Arc::new(std::sync::Mutex::new(internal_event_rx)); + let rx_for_ordering_check = Arc::clone(&internal_event_rx); + + let mut raft_log = MockRaftLog::new(); + raft_log.expect_last_log_id().returning(|| None); + raft_log + .expect_save_hard_state() + .withf(move |state| { + assert!( + rx_for_ordering_check.lock().unwrap().try_recv().is_err(), + "the term must be persisted BEFORE BecomeFollower is sent" + ); + state.current_term == 5 + }) + .times(1) + .returning(|_| Ok(())); + + let mut election_core = mock_election_core(); + election_core + .expect_check_vote_request_is_legal() + .returning(|_, _, _, _, _| false); + let (_graceful_tx, graceful_rx) = watch::channel(()); + let context = MockBuilder::new(graceful_rx) + .with_raft_log(raft_log) + .with_election_handler(election_core) + .build_context(); + let mut state = CandidateState::::new(1, context.node_config.clone()); + let (resp_tx, _resp_rx) = MaybeCloneOneshot::new(); + + state + .handle_inbound_event( + InboundEvent::ReceiveVoteRequest( + VoteRequest { + term: 5, + candidate_id: 2, + last_log_index: 0, + last_log_term: 0, + }, + resp_tx, + ), + &context, + internal_event_tx, + ) + .await + .unwrap(); + + assert_eq!(state.current_term(), 5); + assert!(matches!( + internal_event_rx.lock().unwrap().try_recv().unwrap(), + InternalEvent::BecomeFollower(_) + )); +} + +/// Test: if the unapplied range cannot be read, the Candidate does not campaign this round. +/// +/// It cannot prove that no membership change is pending, so the safe choice is to wait for the +/// next election timeout. Neither an election nor a term bump may happen, and the tick must not +/// fail (a failing tick would be logged as an error on every round). +#[tokio::test(start_paused = true)] +async fn test_candidate_tick_does_not_campaign_when_the_unapplied_range_cannot_be_read() { + let mut election_core = MockElectionCore::::new(); + election_core.expect_broadcast_pre_vote_requests().times(0); + election_core.expect_broadcast_vote_requests().times(0); + + let mut raft_log = MockRaftLog::new(); + raft_log + .expect_get_entries_range() + .returning(|_| Err(crate::StorageError::DbError("cannot read the log".to_string()).into())); + raft_log.expect_save_hard_state().returning(|_| Ok(())); + + let (_graceful_tx, graceful_rx) = watch::channel(()); + let temp_dir = tempfile::tempdir().unwrap(); + let nc = node_config(temp_dir.path().to_str().unwrap()); + let mut state_machine = crate::MockStateMachine::new(); + state_machine.expect_is_running().returning(|| true); + state_machine + .expect_last_applied() + .return_const(d_engine_proto::common::LogId { index: 5, term: 1 }); + let context = MockBuilder::new(graceful_rx) + .with_raft_log(raft_log) + .with_node_config(nc) + .with_state_machine(state_machine) + .with_election_handler(election_core) + .build_context(); + let mut state = CandidateState::::new(1, context.node_config.clone()); + state.shared_state_mut().commit_index = 10; + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + let (event_tx, _event_rx) = mpsc::channel(1); + let max = context.node_config.raft.election.election_timeout_max; + tokio::time::advance(tokio::time::Duration::from_millis(max + 1)).await; + let term_before = state.current_term(); + + state + .tick(&internal_event_tx, &event_tx, &context) + .await + .expect("an unreadable range must not make the tick fail"); + + assert_eq!(state.current_term(), term_before); + assert!(internal_event_rx.try_recv().is_err()); +} diff --git a/d-engine-core/src/raft_role/follower_state.rs b/d-engine-core/src/raft_role/follower_state.rs index b9b380fb..3cd718b4 100644 --- a/d-engine-core/src/raft_role/follower_state.rs +++ b/d-engine-core/src/raft_role/follower_state.rs @@ -13,6 +13,7 @@ use std::marker::PhantomData; use std::sync::Arc; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; +use std::time::Duration; use tokio::sync::mpsc; use tokio::time::Instant; use tonic::Status; @@ -100,6 +101,14 @@ pub struct FollowerState { /// - Transition to candidate state when timeout occurs pub(super) timer: ElectionTimer, + /// Last time a legitimate leader was heard from, via AppendEntries or + /// InstallSnapshotChunk accepted after the term check. + /// + /// Unlike `timer`, this is NOT reset when this node grants a vote, so it + /// only reflects evidence that a leader is alive. `None` means no leader + /// has been heard since this role state was created (e.g. after restart). + last_leader_contact: Option, + // -- Type System Marker -- /// Phantom data for type parameterization _marker: PhantomData, @@ -153,6 +162,9 @@ impl RaftRoleState for FollowerState { fn reset_timer(&mut self) { self.timer.reset() } + fn record_leader_contact(&mut self) { + self.last_leader_contact = Some(Instant::now()); + } fn next_deadline(&self) -> Instant { self.timer.next_deadline() } @@ -164,7 +176,7 @@ impl RaftRoleState for FollowerState { &mut self, internal_event_tx: &mpsc::UnboundedSender, _event_tx: &mpsc::Sender, - _ctx: &RaftContext, + ctx: &RaftContext, ) -> Result<()> { if Instant::now() < self.timer.next_deadline() { return Ok(()); @@ -175,6 +187,12 @@ impl RaftRoleState for FollowerState { debug!("follower::start_election..."); + // Re-armed above so a node that must not campaign does not spin on an expired timer. + if !self.is_active_voter(ctx) { + debug!("follower: not an active voter, skipping election"); + return Ok(()); + } + internal_event_tx.send(InternalEvent::BecomeCandidate).map_err(|e| { error!("Failed to send: {:?}", e); NetworkError::SingalSendFailed(format!("{:?}", e)) @@ -193,6 +211,21 @@ impl RaftRoleState for FollowerState { let my_term = self.current_term(); match inbound_event { + InboundEvent::ReceivePreVoteRequest(vote_request, sender) => { + let leader_active = self.has_recent_leader_contact(self.leader_contact_window()); + + let response = ctx.election_handler().handle_pre_vote_request( + vote_request, + my_term, + ctx.raft_log(), + leader_active, + ); + sender.send(Ok(response)).map_err(|e| { + error!("Failed to send: {:?}", e); + NetworkError::SingalSendFailed(format!("{:?}", e)) + })?; + } + InboundEvent::ReceiveVoteRequest(vote_request, sender) => { let candidate_id = vote_request.candidate_id; @@ -201,6 +234,22 @@ impl RaftRoleState for FollowerState { term: last_log_term, } = ctx.raft_log().last_log_id().unwrap_or(LogId { index: 0, term: 0 }); + // Heard from a legitimate leader within election_timeout_min: ignore the + // request without touching term or vote (Raft dissertation 4.2.3). + if self.has_recent_leader_contact(self.leader_contact_window()) { + let response = VoteResponse { + term: my_term, + vote_granted: false, + last_log_index, + last_log_term, + }; + sender.send(Ok(response)).map_err(|e| { + error!("Failed to send: {:?}", e); + NetworkError::SingalSendFailed(format!("{:?}", e)) + })?; + return Ok(()); + } + match ctx .election_handler() .handle_vote_request( @@ -499,6 +548,7 @@ impl FollowerState { _marker: PhantomData, last_purged_index: None, pending_purge_upto: None, + last_leader_contact: None, } } @@ -511,6 +561,24 @@ impl FollowerState { commit_index: self.commit_index(), } } + + /// Returns true if a legitimate leader was heard from within `window`. + /// Callers pass `election_timeout_min` so the guard never outlives the + /// shortest time any follower could start its own election. + fn has_recent_leader_contact( + &self, + window: Duration, + ) -> bool { + self.last_leader_contact.is_some_and(|t| t.elapsed() < window) + } + + /// How long recent contact with a leader keeps this follower from helping anyone replace + /// it. `election_timeout_min`: no follower can start its own election sooner than that, + /// so the protection never outlives the shortest time a leader can be considered lost. + /// Read from the same config that builds the election timer. + fn leader_contact_window(&self) -> Duration { + Duration::from_millis(self.node_config.raft.election.election_timeout_min) + } } impl From<&CandidateState> for FollowerState { fn from(candidate_state: &CandidateState) -> Self { @@ -527,6 +595,7 @@ impl From<&CandidateState> for FollowerState { // scheduled_purge_upto: None, _marker: PhantomData, pending_purge_upto: None, + last_leader_contact: None, } } } @@ -546,6 +615,7 @@ impl From<&LeaderState> for FollowerState { last_purged_index: leader_state.last_purged_index, // scheduled_purge_upto: None, pending_purge_upto: leader_state.scheduled_purge_upto, + last_leader_contact: None, _marker: PhantomData, } } @@ -553,7 +623,6 @@ impl From<&LeaderState> for FollowerState { impl From<&LearnerState> for FollowerState { fn from(learner_state: &LearnerState) -> Self { Self { - //TODO: should we copy or new? shared_state: learner_state.shared_state.clone(), timer: ElectionTimer::new(( learner_state.node_config.raft.election.election_timeout_min, @@ -564,6 +633,7 @@ impl From<&LearnerState> for FollowerState { pending_append_acks: BTreeMap::new(), last_purged_index: learner_state.last_purged_index, pending_purge_upto: learner_state.pending_purge_upto, + last_leader_contact: None, _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 7d67c1ed..3ad8a306 100644 --- a/d-engine-core/src/raft_role/follower_state_test.rs +++ b/d-engine-core/src/raft_role/follower_state_test.rs @@ -3425,6 +3425,106 @@ async fn test_follower_install_snapshot_reports_success_when_apply_succeeds() { ); } +/// A snapshot transfer longer than the election timeout must not leave the follower ready to +/// campaign the moment it finishes. +/// +/// The handler runs inside the Raft loop, so no tick fires during the transfer; the timer and +/// the leader-contact record were last touched at the first chunk. The leader was alive until +/// the transfer ended, so both must be re-armed when it does. +/// +/// # Given +/// - election timeout 30-40 ms, a snapshot install that takes 120 ms +/// +/// # When +/// - Leader pushes a snapshot (InstallSnapshotChunk event) and the handler returns +/// +/// # Then +/// - the election timer is NOT expired +/// - the guard still sees recent leader contact (within `election_timeout_min`) +#[tokio::test] +async fn test_follower_install_snapshot_rearms_timer_and_guard_after_a_long_transfer() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (mut context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + let mut node_config = (*context.node_config).clone(); + node_config.raft.election.election_timeout_min = 30; + node_config.raft.election.election_timeout_max = 40; + context.node_config = Arc::new(node_config); + + let mut sm_handler = MockStateMachineHandler::new(); + sm_handler.expect_prepare_snapshot_stream().once().returning( + |_first_chunk, _remaining, _ack_tx, _config| { + Ok(crate::PreparedSnapshot { + metadata: Default::default(), + temp_dir: tempfile::tempdir().unwrap(), + }) + }, + ); + context.handlers.state_machine_handler = Arc::new(sm_handler); + // The Worker needs 120 ms to install: longer than the whole election timeout. + let (cmd_tx, mut cmd_rx) = mpsc::unbounded_channel(); + tokio::spawn(async move { + if let Some(StateMachineCommand::InstallSnapshot { response, .. }) = cmd_rx.recv().await { + tokio::time::sleep(std::time::Duration::from_millis(120)).await; + let _ = response.send(Ok(SnapshotApplyResult::Applied { + last_included: LogId { index: 1, term: 1 }, + })); + } + }); + context.handlers.state_machine_commands = StateMachineCommandSender::new(cmd_tx); + // A vote request that reaches the election handler would be granted on term alone. + let mut election_handler = MockElectionCore::::new(); + election_handler.expect_handle_vote_request().times(0); + context.handlers.election_handler = election_handler; + context.storage.raft_log = Arc::new({ + let mut raft_log = MockRaftLog::new(); + raft_log.expect_last_entry_id().returning(|| 0); + raft_log.expect_last_log_id().returning(|| None); + raft_log.expect_save_hard_state().returning(|_| Ok(())); + raft_log + }); + + let mut state = + FollowerState::::new(1, context.node_config.clone(), None, None); + + let (resp_tx, _resp_rx) = MaybeCloneOneshot::new(); + let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + let (tx, rx) = mpsc::channel(32); + tx.send(SnapshotChunk { + leader_term: 1, + leader_id: 1, + ..Default::default() + }) + .await + .unwrap(); + drop(tx); + state + .handle_inbound_event( + InboundEvent::InstallSnapshotChunk(rx, resp_tx), + &context, + internal_event_tx, + ) + .await + .unwrap(); + + assert!( + !state.is_timer_expired(), + "the election timer must be re-armed when a long transfer ends" + ); + + // The guard must also see the leader as alive: a disruptive far-higher-term vote request is + // refused without consulting the election handler and without touching the term. + let response = send_vote_request(&mut state, &context, GUARD_TEST_INFLATED_TERM, 7).await; + assert!( + !response.vote_granted, + "the leader was alive until the transfer ended, the guard must refuse this vote" + ); + assert_eq!( + state.current_term(), + 1, + "a guarded refusal must not adopt the inflated term" + ); +} + /// #436-adjacent: InstallSnapshotChunk from a stale leader (leader_term < follower's /// own current_term) must be rejected outright — never reach `prepare_snapshot_stream` /// — with a response carrying the follower's own (higher) term, mirroring how @@ -4300,3 +4400,495 @@ fn test_follower_from_learner_preserves_both_purge_fields() { Some(LogId { term: 1, index: 5 }) ); } + +// ============================================================================ +// Leader-contact guard on VoteRequest +// +// A follower that heard from a legitimate leader within `election_timeout_min` +// must ignore VoteRequests (Raft dissertation 4.2.3), so a node returning from a +// partition with an inflated term cannot disrupt a healthy leader. +// ============================================================================ + +const GUARD_TEST_LEADER_TERM: u64 = 2; +const GUARD_TEST_LEADER_ID: u32 = 5; +const GUARD_TEST_INFLATED_TERM: u64 = 2400; + +/// Wires the mocks shared by the guard tests. Returns a counter of +/// `save_hard_state` calls so tests can prove a rejection has no side effects. +fn install_guard_test_mocks( + context: &mut RaftContext, + election_handler: MockElectionCore, +) -> Arc { + let hard_state_writes = Arc::new(std::sync::atomic::AtomicUsize::new(0)); + + let mut replication_handler = MockReplicationCore::new(); + replication_handler.expect_handle_append_entries().returning(|_, _, _| { + Ok(AppendResponseWithUpdates { + response: AppendEntriesResponse::success(1, GUARD_TEST_LEADER_TERM, None), + commit_index_update: None, + }) + }); + + let mut raft_log = MockRaftLog::new(); + raft_log.expect_last_entry_id().returning(|| 0); + raft_log.expect_last_log_id().returning(|| None); + let counter = Arc::clone(&hard_state_writes); + raft_log.expect_save_hard_state().returning(move |_| { + counter.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + Ok(()) + }); + + context.membership = Arc::new(MockMembership::new()); + context.handlers.replication_handler = replication_handler; + context.handlers.election_handler = election_handler; + context.storage.raft_log = Arc::new(raft_log); + hard_state_writes +} + +/// Delivers one legitimate AppendEntries (term >= follower term) so the follower +/// records leader contact. The returned receiver must be kept alive by the caller, +/// otherwise the `LeaderDiscovered` send fails. +async fn follower_hears_leader( + state: &mut FollowerState, + context: &RaftContext, +) -> mpsc::UnboundedReceiver { + let (internal_event_tx, internal_event_rx) = mpsc::unbounded_channel(); + let request = AppendEntriesRequest { + term: GUARD_TEST_LEADER_TERM, + leader_id: GUARD_TEST_LEADER_ID, + prev_log_index: 0, + prev_log_term: 1, + entries: vec![], + leader_commit_index: 0, + }; + let (resp_tx, _resp_rx) = MaybeCloneOneshot::new(); + state + .handle_inbound_event( + InboundEvent::AppendEntries(request, vec![resp_tx]), + context, + internal_event_tx, + ) + .await + .unwrap(); + internal_event_rx +} + +async fn send_vote_request( + state: &mut FollowerState, + context: &RaftContext, + term: u64, + candidate_id: u32, +) -> VoteResponse { + let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); + let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + state + .handle_inbound_event( + create_vote_request_event(term, candidate_id, resp_tx), + context, + internal_event_tx, + ) + .await + .unwrap(); + resp_rx.recv().await.unwrap().unwrap() +} + +fn grant_update( + candidate_id: u32, + term: u64, +) -> StateUpdate { + StateUpdate { + new_voted_for: Some(VotedFor { + voted_for_id: candidate_id, + voted_for_term: term, + committed: false, + }), + term_update: Some(term), + } +} + +/// Test: a VoteRequest with a far higher term is rejected while the leader is still alive. +/// +/// Scenario: +/// - Follower accepted an AppendEntries from the leader (term 2). +/// - Half an `election_timeout_min` later, an isolated node sends a VoteRequest with term 2400. +/// +/// Expected: +/// - Rejected without consulting the election handler (it would grant on term alone). +/// - Response carries this follower's own term, not the requester's. +/// - Term unchanged and nothing persisted: adopting 2400 would make this follower reject the +/// real leader's next heartbeat and force it to step down. +#[tokio::test(start_paused = true)] +async fn test_follower_rejects_vote_request_while_leader_contact_is_recent() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (mut context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + + let mut election_handler = MockElectionCore::::new(); + election_handler.expect_handle_vote_request().times(0); + let hard_state_writes = install_guard_test_mocks(&mut context, election_handler); + + let mut state = + FollowerState::::new(1, context.node_config.clone(), None, None); + state.shared_state_mut().update_current_term(1); + let _keep_alive = follower_hears_leader(&mut state, &context).await; + let writes_before = hard_state_writes.load(std::sync::atomic::Ordering::SeqCst); + + let window = + tokio::time::Duration::from_millis(context.node_config.raft.election.election_timeout_min); + tokio::time::advance(window / 2).await; + + let response = send_vote_request(&mut state, &context, GUARD_TEST_INFLATED_TERM, 7).await; + + assert!(!response.vote_granted); + assert_eq!(response.term, GUARD_TEST_LEADER_TERM); + assert_eq!(state.current_term(), GUARD_TEST_LEADER_TERM); + assert_eq!( + hard_state_writes.load(std::sync::atomic::Ordering::SeqCst), + writes_before, + "a guarded rejection must not persist term or vote" + ); +} + +/// Test: once `election_timeout_min` has passed without leader contact, VoteRequests are +/// handled normally. +/// +/// Expected: +/// - The guard is a time window, not a permanent block: if the leader really died, an +/// election must still be able to proceed. +#[tokio::test(start_paused = true)] +async fn test_follower_handles_vote_request_after_leader_contact_window_expires() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (mut context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + + let mut election_handler = MockElectionCore::::new(); + election_handler + .expect_handle_vote_request() + .times(1) + .returning(|_, _, _, _| Ok(grant_update(7, GUARD_TEST_INFLATED_TERM))); + install_guard_test_mocks(&mut context, election_handler); + + let mut state = + FollowerState::::new(1, context.node_config.clone(), None, None); + state.shared_state_mut().update_current_term(1); + let _keep_alive = follower_hears_leader(&mut state, &context).await; + + let window = + tokio::time::Duration::from_millis(context.node_config.raft.election.election_timeout_min); + tokio::time::advance(window + tokio::time::Duration::from_millis(1)).await; + + let response = send_vote_request(&mut state, &context, GUARD_TEST_INFLATED_TERM, 7).await; + + assert!(response.vote_granted); +} + +/// Test: a follower that has never heard from a leader (e.g. just restarted) handles +/// VoteRequests normally. +/// +/// Expected: +/// - No recorded contact means no guard; startup must not wait out a window. +#[tokio::test(start_paused = true)] +async fn test_follower_without_leader_contact_handles_vote_request() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (mut context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + + let mut election_handler = MockElectionCore::::new(); + election_handler + .expect_handle_vote_request() + .times(1) + .returning(|_, _, _, _| Ok(grant_update(7, GUARD_TEST_INFLATED_TERM))); + install_guard_test_mocks(&mut context, election_handler); + + let mut state = + FollowerState::::new(1, context.node_config.clone(), None, None); + + let response = send_vote_request(&mut state, &context, GUARD_TEST_INFLATED_TERM, 7).await; + + assert!(response.vote_granted); +} + +/// Test: granting a vote does not start a new guard window. +/// +/// Scenario: +/// - Leader contact at t0, window expires, follower grants a vote to candidate 7. +/// (Granting resets the election timer, see the #422 test above.) +/// - A moment later, candidate 8 asks for a vote at an even higher term. +/// +/// Expected: +/// - The second request reaches the election handler. The guard keys on leader contact only; +/// if it keyed on the election timer, the vote just granted would count as "leader alive" +/// and block a legitimate election. +#[tokio::test(start_paused = true)] +async fn test_follower_granting_vote_does_not_refresh_leader_contact_guard() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (mut context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + + let mut election_handler = MockElectionCore::::new(); + election_handler + .expect_handle_vote_request() + .times(2) + .returning(|request, _, _, _| Ok(grant_update(request.candidate_id, request.term))); + install_guard_test_mocks(&mut context, election_handler); + + let mut state = + FollowerState::::new(1, context.node_config.clone(), None, None); + state.shared_state_mut().update_current_term(1); + let _keep_alive = follower_hears_leader(&mut state, &context).await; + + let window = + tokio::time::Duration::from_millis(context.node_config.raft.election.election_timeout_min); + tokio::time::advance(window + tokio::time::Duration::from_millis(1)).await; + + let first = send_vote_request(&mut state, &context, GUARD_TEST_INFLATED_TERM, 7).await; + tokio::time::advance(tokio::time::Duration::from_millis(1)).await; + let second = send_vote_request(&mut state, &context, GUARD_TEST_INFLATED_TERM + 1, 8).await; + + assert!(first.vote_granted); + assert!(second.vote_granted); +} + +// ============================================================================ +// PreVote requests received by a Follower +// +// The election handler decides (it is mocked here); the Follower's job is to tell it whether +// a live leader is still active, to forward the answer unchanged, and to change nothing: +// no term, no vote, no timer. +// ============================================================================ + +async fn send_pre_vote_request( + state: &mut FollowerState, + context: &RaftContext, + term: u64, + candidate_id: u32, +) -> VoteResponse { + let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); + let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + state + .handle_inbound_event( + InboundEvent::ReceivePreVoteRequest( + VoteRequest { + term, + candidate_id, + last_log_index: 0, + last_log_term: 0, + }, + resp_tx, + ), + context, + internal_event_tx, + ) + .await + .unwrap(); + resp_rx.recv().await.unwrap().unwrap() +} + +/// An election mock that expects exactly one PreVote and records the `leader_active` it got. +fn pre_vote_election_mock( + seen_leader_active: Arc +) -> MockElectionCore { + let mut election_handler = MockElectionCore::::new(); + election_handler.expect_handle_pre_vote_request().times(1).returning( + move |_, current_term, _, leader_active| { + seen_leader_active.store(leader_active, std::sync::atomic::Ordering::SeqCst); + VoteResponse { + term: current_term, + vote_granted: !leader_active, + last_log_index: 7, + last_log_term: 3, + } + }, + ); + election_handler +} + +/// Test: a Follower that never heard from a leader tells the handler no leader is active. +/// +/// Expected: +/// - `leader_active == false` and the handler's response is forwarded unchanged. +/// - A node that just restarted must be able to take part in an election at once. +#[tokio::test(start_paused = true)] +async fn test_follower_pre_vote_without_leader_contact_reports_no_active_leader() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (mut context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + let seen = Arc::new(std::sync::atomic::AtomicBool::new(true)); + install_guard_test_mocks(&mut context, pre_vote_election_mock(Arc::clone(&seen))); + let mut state = + FollowerState::::new(1, context.node_config.clone(), None, None); + + let response = send_pre_vote_request(&mut state, &context, 2, 7).await; + + assert!(!seen.load(std::sync::atomic::Ordering::SeqCst)); + assert!(response.vote_granted); + assert_eq!((response.last_log_index, response.last_log_term), (7, 3)); +} + +/// Test: a Follower that heard from a leader within `election_timeout_min` tells the handler a +/// leader is active, so the PreVote is denied. +/// +/// Scenario: +/// - AppendEntries from the leader, half a window later a PreVote arrives. +#[tokio::test(start_paused = true)] +async fn test_follower_pre_vote_within_leader_contact_window_reports_active_leader() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (mut context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + let seen = Arc::new(std::sync::atomic::AtomicBool::new(false)); + install_guard_test_mocks(&mut context, pre_vote_election_mock(Arc::clone(&seen))); + let mut state = + FollowerState::::new(1, context.node_config.clone(), None, None); + state.shared_state_mut().update_current_term(1); + let _keep_alive = follower_hears_leader(&mut state, &context).await; + + let window = + tokio::time::Duration::from_millis(context.node_config.raft.election.election_timeout_min); + tokio::time::advance(window / 2).await; + + let response = send_pre_vote_request(&mut state, &context, 9, 7).await; + + assert!(seen.load(std::sync::atomic::Ordering::SeqCst)); + assert!(!response.vote_granted); +} + +/// Test: once the window has passed without leader contact, the Follower no longer reports an +/// active leader (the guard is a time window, not a permanent block). +#[tokio::test(start_paused = true)] +async fn test_follower_pre_vote_after_leader_contact_window_reports_no_active_leader() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (mut context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + let seen = Arc::new(std::sync::atomic::AtomicBool::new(true)); + install_guard_test_mocks(&mut context, pre_vote_election_mock(Arc::clone(&seen))); + let mut state = + FollowerState::::new(1, context.node_config.clone(), None, None); + state.shared_state_mut().update_current_term(1); + let _keep_alive = follower_hears_leader(&mut state, &context).await; + + let window = + tokio::time::Duration::from_millis(context.node_config.raft.election.election_timeout_min); + tokio::time::advance(window + tokio::time::Duration::from_millis(1)).await; + + send_pre_vote_request(&mut state, &context, 9, 7).await; + + assert!(!seen.load(std::sync::atomic::Ordering::SeqCst)); +} + +/// Test: answering a PreVote changes nothing on the Follower, granted or denied. +/// +/// Expected: +/// - Term unchanged, nothing persisted, election timer untouched. A PreVote that reset the +/// timer would let a node that cannot win keep every other follower from ever timing out. +#[tokio::test(start_paused = true)] +async fn test_follower_pre_vote_changes_no_state() { + for contact_leader_first in [false, true] { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (mut context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + let seen = Arc::new(std::sync::atomic::AtomicBool::new(false)); + let hard_state_writes = + install_guard_test_mocks(&mut context, pre_vote_election_mock(Arc::clone(&seen))); + let mut state = + FollowerState::::new(1, context.node_config.clone(), None, None); + state.shared_state_mut().update_current_term(1); + let _keep_alive = if contact_leader_first { + Some(follower_hears_leader(&mut state, &context).await) + } else { + None + }; + let term_before = state.current_term(); + let deadline_before = state.timer.next_deadline; + let writes_before = hard_state_writes.load(std::sync::atomic::Ordering::SeqCst); + + send_pre_vote_request(&mut state, &context, 99, 7).await; + + assert_eq!(state.current_term(), term_before); + assert_eq!(state.timer.next_deadline, deadline_before); + assert_eq!( + hard_state_writes.load(std::sync::atomic::Ordering::SeqCst), + writes_before, + "contact_leader_first={contact_leader_first}: a PreVote must not persist anything" + ); + } +} + +// ============================================================================ +// A node that is not an active voter must never campaign +// +// A removed (or never-promoted) node that keeps timing out would otherwise start elections whose +// quorum math counts its own vote as part of the cluster: with 4 real voters it needs only +// "own vote + 2 grants = 3 of 5" although a majority of the 4 voters is 3 grants. +// openraft skips the election ("not a voter"); raft-rs refuses to campaign when not promotable. +// +// `Membership::voters()` lists the OTHER active voters (it never contains this node), so "am I a +// voter" is answered by this node's own membership entry: present and `Active`. +// ============================================================================ + +fn membership_with_own_status( + own_status: Option +) -> Arc> { + let mut membership = MockMembership::new(); + membership.expect_retrieve_node_meta().returning(move |id| { + own_status.map(|status| d_engine_proto::server::cluster::NodeMeta { + id, + address: format!("http://127.0.0.1:{}", 55000 + id), + role: NodeRole::Follower.into(), + status: status as i32, + }) + }); + membership.expect_is_single_node_cluster().returning(|| false); + Arc::new(membership) +} + +/// Ticks a Follower whose election timer has expired, with this node's own membership entry as +/// given, and returns whether it asked to become a Candidate. +async fn follower_tick_requests_candidacy( + own_status: Option +) -> (bool, bool) { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (mut context, _temp_dir) = mock_raft_context_with_temp(graceful_rx, None); + context.membership = membership_with_own_status(own_status); + let mut state = + FollowerState::::new(1, context.node_config.clone(), None, None); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + let (event_tx, _event_rx) = mpsc::channel(1); + let max = context.node_config.raft.election.election_timeout_max; + tokio::time::advance(tokio::time::Duration::from_millis(max + 1)).await; + assert!( + state.is_timer_expired(), + "precondition: the election timer expired" + ); + + state.tick(&internal_event_tx, &event_tx, &context).await.unwrap(); + + let requested = matches!( + internal_event_rx.try_recv(), + Ok(InternalEvent::BecomeCandidate) + ); + (requested, !state.is_timer_expired()) +} + +/// Test: a Follower that was removed from the membership (no entry for itself) does not start an +/// election, and its timer is re-armed so it does not spin. +#[tokio::test(start_paused = true)] +async fn test_follower_tick_does_not_start_election_when_removed_from_membership() { + let (requested, rearmed) = follower_tick_requests_candidacy(None).await; + + assert!(!requested, "a removed node must not become a candidate"); + assert!( + rearmed, + "the timer must be re-armed so the node does not spin" + ); +} + +/// Test: a node that is only a learner (`Promotable`, not yet `Active`) does not campaign. +#[tokio::test(start_paused = true)] +async fn test_follower_tick_does_not_start_election_when_not_yet_active() { + let (requested, _) = + follower_tick_requests_candidacy(Some(d_engine_proto::common::NodeStatus::Promotable)) + .await; + + assert!(!requested); +} + +/// Test: an `Active` voter still starts an election on timeout (the other side of the boundary). +#[tokio::test(start_paused = true)] +async fn test_follower_tick_starts_election_when_an_active_voter() { + let (requested, _) = + follower_tick_requests_candidacy(Some(d_engine_proto::common::NodeStatus::Active)).await; + + assert!(requested); +} diff --git a/d-engine-core/src/raft_role/leader_state.rs b/d-engine-core/src/raft_role/leader_state.rs index 79c8ffc6..43eac033 100644 --- a/d-engine-core/src/raft_role/leader_state.rs +++ b/d-engine-core/src/raft_role/leader_state.rs @@ -10,6 +10,7 @@ use super::role_state::RaftRoleState; use super::role_state::check_and_trigger_snapshot; use super::role_state::send_replay_inbound_event; use crate::ConsensusError; +use crate::ElectionCore; use crate::Error; use crate::InboundEvent; use crate::InternalEvent; @@ -320,6 +321,27 @@ pub struct LeaderState { /// Raft Paper: §5.3 Figure 2 (matchIndex) pub(super) match_index: HashMap, + /// === Volatile State === (per leadership, reset on every election) + /// `now_ms()` deadline until which the latest quorum ACK still counts as "a quorum just + /// confirmed this leader". Unlike the read lease it is NOT gated by the noop being + /// applied; the VoteRequest guard uses it. 0 = never confirmed. + quorum_ack_deadline_ms: u64, + + /// === Volatile State === (per leadership, reset on every election) + /// `now_ms()` of the latest quorum ACK, or of the election for a new leader (one election + /// timeout of grace to collect the first ACKs). CheckQuorum steps down when it is older than + /// `election_timeout_max`, the longest silence any follower tolerates before campaigning + /// (etcd's effective tolerance is the same). Unlike `quorum_ack_deadline_ms` (a lease + /// window, shorter than the election timeout) a slow reply or brief jitter does not trip it. + last_quorum_contact_ms: u64, + + /// Per voter peer: send time of the latest heartbeat round it acknowledged. + /// `match_index` records how far a peer replicated; this records who answered lately. + peer_ack_send_ts: HashMap, + + /// Highest quorum-acknowledged send time applied so far (None = never confirmed). + last_quorum_acked_ts: Option, + /// === Volatile State === /// Per-peer replication trust state (#436). A parallel map, same convention /// as `next_index`/`match_index` — not merged into a combined struct, to @@ -334,8 +356,9 @@ pub struct LeaderState { peer_metrics: HashMap, /// === Volatile State === - /// Temporary storage for no-op entry log ID during leader initialization - #[doc(hidden)] + /// The last log index when this leader's noop commit was observed; set once per leadership. + /// NOT the noop's own index if client writes were appended after it. Gates lease reads + /// and linearizable reads until set. pub noop_log_id: Option, // -- Log Compaction & Purge -- @@ -349,8 +372,9 @@ pub struct LeaderState { /// 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. + /// === Volatile copy of Persistent State === + /// In-memory copy of the last physically removed log position. The persisted source of + /// truth is the log store's purge boundary. /// /// This value is atomically updated when: /// 1. A new snapshot is persisted (marking logs up to `last_included_index` as purgeable) @@ -447,7 +471,8 @@ pub struct LeaderState { // -- Client Request Tracking -- /// Writes committed but awaiting state machine apply before responding. /// Key: log index. Value: response sender plus its propose/commit timestamps. - /// Populated when `wait_for_apply=true` (e.g. CAS, or any write needing SM result). + /// Every client write lands here (`ProposeBatchBuffer::flush` sets + /// `wait_for_apply_event = true`), so a client sees success only after apply. /// Drained by `handle_apply_completed`; cleared with error on step-down / fatal error. pub(super) pending_write_apply: HashMap, @@ -674,7 +699,7 @@ impl RaftRoleState for LeaderState { ); print_role_transition_line(Leader as i32, Follower as i32, self.current_term()); // Revoke lease so ReadActor immediately returns LeaseInvalid on the next read. - self.shared_state.lease.revoke(); + self.shared_state.read_lease.revoke(); Ok(RaftRole::Follower(Box::new(self.into()))) } @@ -715,7 +740,31 @@ impl RaftRoleState for LeaderState { error!("Stale learner check failed: {}", e); } - // 2. Heartbeat trigger check + // 2. CheckQuorum: a leader no quorum has answered for an ( election_timeout_max × multiple) timeout may be cut off. + // Step down (same term) so clients are redirected instead of queueing writes that can + // never commit. A single voter is its own quorum. + let quorum_silence_limit_ms = + self.node_config.raft.election.election_timeout_max.saturating_mul( + self.node_config.raft.backpressure.write_admission_election_timeout_multiple, + ); + if !self.cluster_metadata.single_voter + && now_ms().saturating_sub(self.last_quorum_contact_ms) > quorum_silence_limit_ms + { + warn!( + "leader: no quorum ACK for an election_timeout_max × multiple timeout, stepping down" + ); + + // The loop is a biased select with the tick branch first: leaving the replication + // deadline in the past would make this tick fire again at once, forever, and the + // BecomeFollower event above would never be polled. + self.timer.reset_replication(); + + self.drain_pending_writes_with_error(ErrorCode::NotLeader); + self.send_become_follower_event(None, internal_event_tx)?; + return Ok(()); + } + + // 3. Heartbeat trigger check // Send heartbeat if the replication timer expires if now >= self.timer.replication_deadline() { debug!(?now, "tick::reset_replication timer"); @@ -726,7 +775,7 @@ impl RaftRoleState for LeaderState { self.send_heartbeat_or_batch(request, internal_event_tx, ctx).await?; } - // 3. Drain expired pending client writes. + // 4. Drain expired pending client writes. // NOTE: expiry checked at tick granularity; actual timeout may be up to one tick late. self.pending_client_writes.retain(|_, meta| { if now >= meta.deadline { @@ -739,7 +788,7 @@ impl RaftRoleState for LeaderState { } }); - // 4. Drain expired pending linearizable reads. + // 5. Drain expired pending linearizable reads. self.pending_reads.retain(|_, batch| { if now >= batch.deadline { for (_, sender) in batch.requests.drain(..) { @@ -751,7 +800,7 @@ impl RaftRoleState for LeaderState { } }); - // 4b. Drain expired pending lease reads. + // 6. Drain expired pending lease reads. let mut unexpired_reads = VecDeque::new(); for entry in self.pending_lease_reads.drain(..) { if now >= entry.deadline { @@ -762,7 +811,7 @@ impl RaftRoleState for LeaderState { } self.pending_lease_reads = unexpired_reads; - // 5. Drain expired post-commit actions (e.g. noop/join timed out before quorum). + // 7. Drain expired post-commit actions (e.g. noop/join timed out before quorum). // A noop timeout means we cannot confirm leadership — step down immediately. // A join timeout sends a deadline_exceeded error to the waiting client. let expired_keys: Vec = self @@ -787,7 +836,7 @@ impl RaftRoleState for LeaderState { } if noop_timed_out { warn!("LeaderNoop commit timed out — stepping down"); - let _ = internal_event_tx.send(InternalEvent::BecomeFollower(None)); + self.send_become_follower_event(None, internal_event_tx)?; } Ok(()) @@ -1016,9 +1065,16 @@ impl RaftRoleState for LeaderState { // Drain-based: unconditionally flush all buffered commands // No timeout/size checks - drain from channel already collected the batch - let has_writes = !self.propose_buffer.is_empty(); + let mut has_writes = !self.propose_buffer.is_empty(); let has_reads = !self.linearizable_read_buffer.is_empty(); + // Write admission: reject the whole batch if no quorum answered recently. Judged once per + // batch, so the per-write path stays untouched. + if has_writes && !self.is_write_admitted() { + self.reject_buffered_writes(); + has_writes = false; + } + // Fast path: pure write workload (most common case, ~95%) // Preserves original high-performance code path for write-heavy workloads if has_writes && !has_reads { @@ -1057,12 +1113,29 @@ impl RaftRoleState for LeaderState { let my_term = self.current_term(); match inbound_event { + InboundEvent::ReceivePreVoteRequest(vote_request, sender) => { + // A PreVote never makes a leader step down; it only answers. + let response = ctx.election_handler().handle_pre_vote_request( + vote_request, + my_term, + ctx.raft_log(), + self.has_recent_quorum_ack(), + ); + sender.send(Ok(response)).map_err(|e| { + error!("Failed to send: {:?}", e); + NetworkError::SingalSendFailed(format!("{:?}", e)) + })?; + } + // Leader receives RequestVote(term=X, candidate=Y) - // 1. If X > currentTerm: + // Deviates from Raft §5.1 on purpose (dissertation §4.2.3, disruptive servers): a leader + // that a quorum confirmed within the lease window ignores a higher term, so an + // isolated node that comes back cannot depose a healthy leader. + // 1. If X > currentTerm and no quorum ACK within the lease window: // - Leader → Follower, currentTerm = X // - Replay event - // 2. Else: - // - Reply with VoteGranted=false, currentTerm=currentTerm + // 2. Else (X <= currentTerm, or a quorum recently confirmed this leader): + // - Reply with VoteGranted=false, currentTerm=currentTerm, no state change InboundEvent::ReceiveVoteRequest(vote_request, sender) => { debug!( "handle_inbound_event::InboundEvent::ReceiveVoteRequest: {:?}", @@ -1070,12 +1143,9 @@ impl RaftRoleState for LeaderState { ); let my_term = self.current_term(); - if my_term < vote_request.term { + if my_term < vote_request.term && !self.has_recent_quorum_ack() { self.commit_hard_state(ctx, Some(vote_request.term), None)?; - // Revoke lease immediately — before the Raft loop processes BecomeFollower, - // a concurrent ReadActor could still see the old valid lease (window-period bug). - self.shared_state.lease.revoke(); // Step down as Follower self.send_become_follower_event(None, &internal_event_tx)?; @@ -1179,9 +1249,8 @@ impl RaftRoleState for LeaderState { "my({}) term < request one, now I will step down to Follower", my_id ); - //TODO: if there is a bug? self.update_current_term(vote_request.term); - // Revoke lease immediately — window-period fix (see VoteRequest branch). - self.shared_state.lease.revoke(); + + // The new term is adopted when the Follower handles the replayed request. self.send_become_follower_event( Some(append_entries_request.leader_id), &internal_event_tx, @@ -1431,6 +1500,7 @@ impl RaftRoleState for LeaderState { self.update_lease_timestamp( now_ms(), ctx.node_config().raft.read_consistency.lease_duration_ms, + ctx.state_machine().last_applied().index, ); self.drain_pending_lease_reads(ctx); } @@ -1503,8 +1573,6 @@ impl RaftRoleState for LeaderState { self.commit_hard_state(ctx, Some(response.term), None)?; self.drain_pending_writes_with_error(ErrorCode::TermOutdated); - // Revoke lease immediately — window-period fix (see VoteRequest branch). - self.shared_state.lease.revoke(); self.send_become_follower_event(None, internal_event_tx)?; return Err(ReplicationError::HigherTerm(response.term).into()); } @@ -1540,8 +1608,7 @@ impl RaftRoleState for LeaderState { self.commit_hard_state(ctx, Some(term), None)?; self.drain_pending_writes_with_error(ErrorCode::TermOutdated); - // Revoke lease immediately — window-period fix (see VoteRequest branch). - self.shared_state.lease.revoke(); + self.send_become_follower_event(None, internal_event_tx)?; return Err(ReplicationError::HigherTerm(term).into()); } @@ -1600,8 +1667,8 @@ impl RaftRoleState for LeaderState { // Re-calculate commit index after updating this voter's match_index. if peer_update.success && is_voter { let (commit_index, majority) = self.majority_matched_index(ctx.raft_log()); - let quorum_confirmed = majority.is_some(); + // `majority` answers the COMMIT question only. if let Some(new_commit) = Self::new_commit_index(commit_index, majority) { self.update_commit_index_with_signal( Leader as i32, @@ -1613,37 +1680,12 @@ impl RaftRoleState for LeaderState { self.drain_commit_actions(new_commit, ctx, internal_event_tx).await; } - // Lease refresh and pending_lease_reads drain are triggered by quorum ACK, - // independent of whether commit_index advanced. When an expired lease fires an - // empty AppendEntries heartbeat, commit_index does not change (nothing new to - // commit), so new_commit_index yields None and the block above is skipped. - // We still reuse the single majority computation (majority.is_some()) to - // confirm quorum here. - if quorum_confirmed { - // Anchor deadline to send time (not ACK time) to eliminate the RTT/2 window. - // Falls back to now_ms() only in tests that bypass execute_and_process_raft_rpc. - let send_ts = if self.last_heartbeat_send_ts > 0 { - self.last_heartbeat_send_ts - } else { - now_ms() - }; - self.update_lease_timestamp( - send_ts, - ctx.node_config().raft.read_consistency.lease_duration_ms, - ); - self.drain_pending_lease_reads(ctx); - // Path A drain (Bug #381 fix): serve linearizable reads that have been - // waiting for quorum confirmation. Pure-read batches never advance - // commit_index, so handle_apply_completed (Path B) would never fire for - // them. Drain here once leadership is confirmed and SM is ready. - let last_applied = ctx.state_machine().last_applied().index; - let to_serve: Vec = - self.pending_reads.range(..=last_applied).map(|(k, _)| *k).collect(); - for idx in to_serve { - if let Some(batch) = self.pending_reads.remove(&idx) { - self.execute_pending_reads(batch.requests, ctx); - } - } + // "A quorum answered" comes from who replied, never from replicated history: + // the stored match_index of an unreachable peer keeps the commit median alive. + // Anchor to send time (not ACK time) to eliminate the RTT/2 window. Falls back to + // now_ms() only in tests that bypass execute_and_process_raft_rpc. + if let Some(quorum_ts) = self.record_voter_ack(follower_id, self.last_round_send_ts()) { + self.on_quorum_confirmed(quorum_ts, ctx); } } @@ -1921,14 +1963,7 @@ impl RaftRoleState for LeaderState { "[Leader-{}] Removed from cluster membership, stepping down to Follower", self.node_id() ); - internal_event_tx.send(InternalEvent::BecomeFollower(None)).map_err(|e| { - error!( - "[Leader-{}] Failed to send BecomeFollower after self-removal: {:?}", - self.node_id(), - e - ); - NetworkError::SingalSendFailed(format!("BecomeFollower after self-removal: {e:?}")) - })?; + self.send_become_follower_event(None, internal_event_tx)?; Ok(()) } @@ -2357,9 +2392,10 @@ impl LeaderState { /// Drain pending client write batches whose end_log_index <= new_commit. /// - /// For `wait_for_apply=false` batches: send write_success immediately. - /// For `wait_for_apply=true` batches (e.g. CAS): move senders to `pending_write_apply` - /// so they are resolved by the ApplyCompleted handler. + /// Client writes always have `wait_for_apply=true`: their senders move to + /// `pending_write_apply` and are resolved by the ApplyCompleted handler. + /// `wait_for_apply=false` is only used by internal requests (leader noop, join, + /// batch promote/remove). They carry no senders, so nothing waits on them. fn drain_pending_client_writes( &mut self, new_commit: u64, @@ -2399,6 +2435,7 @@ impl LeaderState { ); } } else { + // Internal requests have no senders, so this loop is a no-op for them. for sender in senders { let _ = sender.send(Ok(ClientResponse::write_success())); } @@ -2473,6 +2510,8 @@ impl LeaderState { &mut self, ctx: &RaftContext, ) -> Result<()> { + // NOT the noop's own index: the last log entry when the noop commit is observed, which + // can be higher if client writes were appended after the noop. Conservative for readers. let noop_index = ctx.raft_log().last_entry_id(); self.noop_log_id = Some(noop_index); debug!("Tracked noop_log_id: {}", noop_index); @@ -2561,6 +2600,10 @@ impl LeaderState { /// Drain all pending client writes with an error response. /// Called on step-down or HigherTerm detection. + /// + /// These writes were already proposed, so the new leader may still commit them even though + /// the client receives an error here. Writes are therefore at-least-once: a client that + /// retries a non-idempotent command can apply it twice. (No request-id dedup exists yet.) fn drain_pending_writes_with_error( &mut self, error_code: ErrorCode, @@ -2596,6 +2639,14 @@ impl LeaderState { replication_targets: replication_targets.clone(), }; + // Voter set or quorum size may have changed: drop acks of peers that are no longer + // voters and recompute the quorum time on the next reply. + let learner_role = d_engine_proto::common::NodeRole::Learner as i32; + self.peer_ack_send_ts.retain(|id, _| { + replication_targets.iter().any(|n| n.id == *id && n.role != learner_role) + }); + self.last_quorum_acked_ts = None; + debug!( "Updated cluster metadata: single_voter={}, total_voters={}, replication_targets={}", single_voter, @@ -3090,6 +3141,11 @@ impl LeaderState { ?new_leader_id, "Leader is going to step down as Follower..." ); + + // Revoke lease immediately — before the Raft loop processes BecomeFollower, + // a concurrent ReadActor could still see the old valid lease (window-period bug). + self.shared_state.read_lease.revoke(); + internal_event_tx .send(InternalEvent::BecomeFollower(new_leader_id)) .map_err(|e| { @@ -3262,6 +3318,10 @@ impl LeaderState { replication_targets: vec![], }, shared_state: SharedState::new(node_id, None, None), + quorum_ack_deadline_ms: 0, + last_quorum_contact_ms: now_ms(), + peer_ack_send_ts: HashMap::new(), + last_quorum_acked_ts: None, timer: Box::new(ReplicationTimer::new(rpc_append_entries_clock_in_ms)), next_index: HashMap::new(), match_index: HashMap::new(), @@ -3756,22 +3816,184 @@ impl LeaderState { /// Returns true iff this leader holds a valid quorum-backed lease. /// Uses a single atomic load (~4 ns) — no mutex, no lock contention. pub fn is_lease_valid(&self) -> bool { - self.shared_state.lease.is_valid_for_leader(self.current_term(), now_ms()) + self.shared_state.read_lease.is_valid_for_leader(self.current_term(), now_ms()) + } + + /// Latest heartbeat send time that a quorum of voters has acknowledged. + /// + /// The leader counts as one voter, so `need = total_voters / 2` peers must have + /// acknowledged a round for it to count. + /// `peer_ack_send_ts` holds voters only, so there is no role filter here. + /// + /// The result is the `need`-th largest time, found without allocation or sorting: + /// the largest candidate T such that at least `need` peers have `ts >= T`. + /// `ts >= candidate` also counts the candidate peer itself. + /// + /// Example, 5 voters (`need = 2`), times are `now_ms()` readings: + /// + /// ```text + /// B = 1300, C = 900, D = 1000, E = 1000 + /// + /// candidate 1300: peers with ts >= 1300 are {B} -> 1 < 2, rejected + /// candidate 900: peers with ts >= 900 are {B, C, D, E} -> 4 >= 2, accepted + /// candidate 1000: peers with ts >= 1000 are {B, D, E} -> 3 >= 2, accepted + /// + /// best = max(accepted) = 1000 + /// ``` + /// + /// Same as sorting descending (1300, 1000, 1000, 900) and taking the 2nd value: + /// B and D (or E) both reached 1000, plus the leader makes 3 of 5. + /// + /// A partitioned peer never updates its time, so it caps the result and a single + /// reachable follower cannot move it forward. + /// `match_index` is not used on purpose: it is replication history and does not + /// say who answered lately (see `majority_matched_index`, which is for the commit index). + fn quorum_acked_send_ts(&self) -> Option { + let need = self.cluster_metadata.total_voters / 2; + if need == 0 { + return None; // single voter: handled by its own path + } + let mut best: Option = None; + for &candidate in self.peer_ack_send_ts.values() { + let count = self.peer_ack_send_ts.values().filter(|&&ts| ts >= candidate).count(); + if count >= need && best.is_none_or(|b| candidate > b) { + best = Some(candidate); + } + } + best + } + + /// Send time of the heartbeat round a reply received now is credited to. + /// Anchored to send time (not ACK time) to eliminate the RTT/2 window. + /// Falls back to `now_ms()` only in tests that bypass `execute_and_process_raft_rpc`. + fn last_round_send_ts(&self) -> u64 { + if self.last_heartbeat_send_ts > 0 { + self.last_heartbeat_send_ts + } else { + now_ms() + } + } + + /// Records that voter `peer` acknowledged the round sent at `send_ts`. + /// + /// Returns the new quorum-acknowledged send time only if it moved forward, `None` otherwise. + /// So the caller acts once per quorum confirmation, not once per reply. + fn record_voter_ack( + &mut self, + peer: u32, + send_ts: u64, + ) -> Option { + self.peer_ack_send_ts.insert(peer, send_ts); + + // Cheap exit. Send times never decrease, so a reply that is not newer than the + // current quorum time cannot move it forward. + if self.last_quorum_acked_ts.is_some_and(|last| send_ts <= last) { + return None; + } + + // Not enough voters have answered yet. + let quorum_ts = self.quorum_acked_send_ts()?; + + // Same or older than what was already applied: nothing new. + if self.last_quorum_acked_ts.is_some_and(|last| quorum_ts <= last) { + return None; + } + + self.last_quorum_acked_ts = Some(quorum_ts); + Some(quorum_ts) + } + + /// A quorum of voters acknowledged a newer round than before: this leader is confirmed. + /// Renews the lease and releases the reads that waited for the confirmation. + fn on_quorum_confirmed( + &mut self, + quorum_ts: u64, + ctx: &RaftContext, + ) { + let last_applied = ctx.state_machine().last_applied().index; + self.update_lease_timestamp( + quorum_ts, + ctx.node_config().raft.read_consistency.lease_duration_ms, + last_applied, + ); + self.drain_pending_lease_reads(ctx); + + // Path A drain (Bug #381 fix): serve linearizable reads that have been waiting for + // quorum confirmation. Pure-read batches never advance commit_index, so + // handle_apply_completed (Path B) would never fire for them. Drain here once + // leadership is confirmed and SM is ready. + let to_serve: Vec = + self.pending_reads.range(..=last_applied).map(|(k, _)| *k).collect(); + for idx in to_serve { + if let Some(batch) = self.pending_reads.remove(&idx) { + self.execute_pending_reads(batch.requests, ctx); + } + } + } + + /// True if a quorum acknowledged this leader recently enough that no other leader can exist. + fn has_recent_quorum_ack(&self) -> bool { + self.quorum_ack_deadline_ms > now_ms() + } + + /// Admit client writes while a quorum answered within `election_timeout_min`, counted from + /// the last ACK (or from the election, which gives a new leader one window for its first + /// ACK). A rejected write is answered before it is appended, so the client can always retry. + /// Deliberately not the read lease: the lease is a read-safety bound, this is liveness only. + fn is_write_admitted(&self) -> bool { + self.cluster_metadata.single_voter + || now_ms().saturating_sub(self.last_quorum_contact_ms) + <= self.node_config.raft.election.election_timeout_min } - /// Renews the lease after every quorum ACK (handle_append_result, handle_log_flushed). + /// Answers every buffered client write with `NotLeader`; none of them could commit. + fn reject_buffered_writes(&mut self) { + if let Some(request) = self.propose_buffer.flush() { + metrics::counter!( + "core.raft.write_admission.rejections", + "node_id" => self.node_id().to_string() + ) + .increment(request.senders.len() as u64); + for sender in request.senders { + let _ = sender.send(Ok(ClientResponse::client_error(ErrorCode::NotLeader))); + } + } + } + /// Publishes the read lease after a quorum ACK (handle_append_result, handle_log_flushed). + /// + /// Does nothing until this leader's noop is committed AND applied + /// (`last_applied >= noop`, Raft §8). Before that the state machine may still miss entries + /// committed by previous leaders, so readers keep falling back to the Raft loop. The next + /// quorum ACK retries. /// /// `send_ts` is the timestamp recorded just before the heartbeat was sent. /// Using send_ts (not now_ms()) eliminates the RTT/2 window: /// deadline = send_ts + lease_duration_ms ≤ follower's election timer start + election_timeout /// Single-voter callers pass now_ms() directly (no peer RTT to account for). + /// `last_applied` is the state machine's applied index at call time. fn update_lease_timestamp( - &self, + &mut self, send_ts: u64, lease_duration_ms: u64, + last_applied: u64, ) { let deadline = send_ts.saturating_add(lease_duration_ms); - self.shared_state.lease.renew(self.current_term(), deadline); + // Recorded before the noop/applied gate: it answers "did a quorum just confirm me". + self.quorum_ack_deadline_ms = deadline; + + // The send time of the round a quorum acknowledged: write admission and step-down see + // the same signal as the lease. A new leader keeps its election time until then. + self.last_quorum_contact_ms = send_ts; + + // Raft §8: no lease reads until this term's noop is committed AND applied. + // Committed alone proves the commit point is known; applied proves the state + // machine already holds every entry committed by previous leaders. + match self.noop_log_id { + Some(noop) if last_applied >= noop => {} + _ => return, + } + + self.shared_state.read_lease.renew(self.current_term(), deadline); } /// Unified write + linearizable read: single RPC for both (2*RTT → 1*RTT). @@ -3858,7 +4080,7 @@ impl LeaderState { pub(crate) fn test_update_lease_timestamp(&mut self) { // Renew with a generous 60-second deadline so tests don't flake on slow CI. let deadline = now_ms().saturating_add(60_000); - self.shared_state.lease.renew(self.current_term(), deadline); + self.shared_state.read_lease.renew(self.current_term(), deadline); } /// Test helper: renew lease from an explicit send_ts (RTT/2 fix path). @@ -3866,8 +4088,9 @@ impl LeaderState { pub(crate) fn test_renew_lease_from_send_ts( &mut self, lease_duration_ms: u64, + last_applied: u64, ) { - self.update_lease_timestamp(self.last_heartbeat_send_ts, lease_duration_ms); + self.update_lease_timestamp(self.last_heartbeat_send_ts, lease_duration_ms, last_applied); } /// Returns the current match_index for `peer_id`, or 0 if not yet tracked. @@ -3964,6 +4187,7 @@ impl LeaderState { self.update_lease_timestamp( now_ms(), ctx.node_config().raft.read_consistency.lease_duration_ms, + ctx.state_machine().last_applied().index, ); let results = ctx .handlers @@ -4158,6 +4382,10 @@ impl From<&CandidateState> for LeaderState { Self { shared_state, + quorum_ack_deadline_ms: 0, + last_quorum_contact_ms: now_ms(), + peer_ack_send_ts: HashMap::new(), + last_quorum_acked_ts: None, timer: Box::new(ReplicationTimer::new(rpc_append_entries_clock_in_ms)), next_index: HashMap::new(), match_index: HashMap::new(), 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 9f465390..25c856f4 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 @@ -30,6 +30,7 @@ async fn test_backpressure_write_limit_enforcement() { config.raft.backpressure = BackpressureConfig { max_pending_writes: 2, max_pending_reads: 100, + ..Default::default() }; ctx.node_config = std::sync::Arc::new(config); @@ -102,6 +103,7 @@ async fn test_backpressure_read_limit_enforcement() { config.raft.backpressure = BackpressureConfig { max_pending_writes: 100, max_pending_reads: 3, + ..Default::default() }; ctx.node_config = std::sync::Arc::new(config); @@ -168,7 +170,8 @@ async fn test_backpressure_unlimited_when_zero() { let mut config = (*ctx.node_config).clone(); config.raft.backpressure = BackpressureConfig { max_pending_writes: 0, // Unlimited - max_pending_reads: 0, // Unlimited + max_pending_reads: 0, // Unlimited, + ..Default::default() }; ctx.node_config = std::sync::Arc::new(config); @@ -213,6 +216,7 @@ async fn test_backpressure_write_and_read_independent() { config.raft.backpressure = BackpressureConfig { max_pending_writes: 2, max_pending_reads: 10, + ..Default::default() }; ctx.node_config = std::sync::Arc::new(config); @@ -291,6 +295,7 @@ async fn test_backpressure_all_read_policies() { config.raft.backpressure = BackpressureConfig { max_pending_writes: 100, max_pending_reads: 2, + ..Default::default() }; ctx.node_config = std::sync::Arc::new(config); diff --git a/d-engine-core/src/raft_role/leader_state_test/become_follower_test.rs b/d-engine-core/src/raft_role/leader_state_test/become_follower_test.rs index d6dcd22a..d6d4bfe3 100644 --- a/d-engine-core/src/raft_role/leader_state_test/become_follower_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/become_follower_test.rs @@ -25,6 +25,15 @@ use crate::test_utils::mock::MockTypeConfig; use d_engine_proto::server::election::VoteRequest; use d_engine_proto::server::replication::{AppendEntriesRequest, AppendEntriesResponse}; +/// Simulates a quorum ACK of a heartbeat sent just now, with the noop already applied. +/// Goes through the real `update_lease_timestamp`, so whatever it records (read lease, +/// quorum-ack time) is recorded exactly as in production. +fn simulate_quorum_ack(state: &mut LeaderState) { + state.noop_log_id = Some(1); + state.last_heartbeat_send_ts = now_ms(); + state.test_renew_lease_from_send_ts(60_000, 1); +} + /// become_follower() revokes the shared Arc so ReadActor immediately /// returns LeaseInvalid on the very next is_valid() check. /// @@ -36,7 +45,7 @@ async fn test_become_follower_revokes_read_lease() { // Clone Arc before step-down to observe the shared lease from the outside // (simulates what ReadActor holds). - let lease = Arc::clone(&state.shared_state.lease); + let lease = Arc::clone(&state.shared_state.read_lease); // Renew with a generous 60-second deadline. state.test_update_lease_timestamp(); @@ -59,31 +68,29 @@ async fn test_become_follower_revokes_read_lease() { ); } -/// Higher-term VoteRequest → BecomeFollower event → become_follower() → lease revoked. +/// A leader with a valid lease rejects a higher-term VoteRequest instead of stepping down. /// -/// End-to-end pin for the path: -/// ReceiveVoteRequest(term > current) → send_become_follower_event() -/// → Raft loop calls become_follower() → lease.revoke() +/// Scenario: +/// - Leader (term 1) has a valid lease: a quorum acknowledged it within the lease window. +/// - A node returning from a partition asks for a vote with an inflated term (999). /// -/// The ReadActor holds the same Arc. After this chain the fast path -/// must return LeaseInvalid on the very next is_valid() check. +/// Expected: +/// - No `BecomeFollower` event: the cluster is healthy, so the returning node must not +/// force a re-election. +/// - Response is a rejection carrying the leader's own term. #[tokio::test] -async fn test_receive_higher_term_vote_request_revokes_lease() { +async fn test_receive_higher_term_vote_request_with_valid_lease_is_rejected() { let (_graceful_tx, graceful_rx) = watch::channel(()); let context = MockBuilder::new(graceful_rx) - .with_db_path("/tmp/test_vote_request_revokes_lease") + .with_db_path("/tmp/test_vote_request_valid_lease_rejected") .build_context(); let mut state = LeaderState::::new(1, context.node_config.clone()); - let lease = Arc::clone(&state.shared_state.lease); - - state.test_update_lease_timestamp(); - assert!( - lease.is_valid(now_ms()), - "precondition: lease must be valid" - ); + let leader_term = state.current_term(); + simulate_quorum_ack(&mut state); + assert!(state.is_lease_valid(), "precondition: lease must be valid"); - let (resp_tx, _resp_rx) = >::new(); + let (resp_tx, mut resp_rx) = >::new(); let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); let event = InboundEvent::ReceiveVoteRequest( VoteRequest { @@ -94,23 +101,15 @@ async fn test_receive_higher_term_vote_request_revokes_lease() { }, resp_tx, ); - state.handle_inbound_event(event, &context, internal_event_tx).await.ok(); - - assert!( - matches!( - internal_event_rx.try_recv(), - Ok(InternalEvent::BecomeFollower(_)) - ), - "higher-term VoteRequest must emit BecomeFollower" - ); - - // Simulate Raft loop processing BecomeFollower. - let _ = state.become_follower().expect("become_follower must succeed"); + state.handle_inbound_event(event, &context, internal_event_tx).await.unwrap(); assert!( - !lease.is_valid(now_ms()), - "lease must be revoked after VoteRequest step-down — ReadActor must not serve stale reads" + internal_event_rx.try_recv().is_err(), + "a leader with a valid lease must not step down on a higher-term VoteRequest" ); + let response = resp_rx.recv().await.unwrap().unwrap(); + assert!(!response.vote_granted); + assert_eq!(response.term, leader_term); } /// Higher-term AppendEntries → BecomeFollower event → become_follower() → lease revoked. @@ -127,7 +126,7 @@ async fn test_receive_higher_term_append_entries_revokes_lease() { let mut state = LeaderState::::new(1, context.node_config.clone()); state.update_current_term(10); - let lease = Arc::clone(&state.shared_state.lease); + let lease = Arc::clone(&state.shared_state.read_lease); state.test_update_lease_timestamp(); assert!( @@ -167,33 +166,66 @@ async fn test_receive_higher_term_append_entries_revokes_lease() { ); } -/// Higher-term VoteRequest → lease revoked BEFORE Raft loop calls become_follower(). -/// -/// Pins the window-period safety contract: the Raft loop sends BecomeFollower into -/// internal_event_tx but may not process it immediately (async event-driven). During that gap a -/// concurrent ReadActor task can observe the old valid lease and serve a stale LeaseRead. +/// Rejecting a higher-term VoteRequest has no side effects on a leader with a valid lease. /// -/// This test verifies the fix: revoke() is called at the detection point itself, so -/// the lease is invalid as soon as handle_inbound_event() returns — before become_follower() -/// is ever called by the Raft loop. -/// -/// Expected to FAIL before the fix (lease still valid in window), PASS after. +/// Expected: +/// - Leader term unchanged: adopting 999 would make the leader reject its own followers' +/// term-1 traffic and break a healthy cluster. +/// - The read lease is untouched, so LeaseRead keeps being served; revoking it here would +/// punish readers for a vote request the leader ignored. #[tokio::test] -async fn test_vote_request_higher_term_lease_revoked_before_become_follower() { +async fn test_vote_request_higher_term_with_valid_lease_leaves_term_and_lease_untouched() { let (_graceful_tx, graceful_rx) = watch::channel(()); let context = MockBuilder::new(graceful_rx) - .with_db_path("/tmp/test_vote_request_revokes_lease_early") + .with_db_path("/tmp/test_vote_request_valid_lease_untouched") .build_context(); let mut state = LeaderState::::new(1, context.node_config.clone()); - let lease = Arc::clone(&state.shared_state.lease); - - state.test_update_lease_timestamp(); + let lease = Arc::clone(&state.shared_state.read_lease); + let term_before = state.current_term(); + simulate_quorum_ack(&mut state); assert!( lease.is_valid(now_ms()), "precondition: lease must be valid" ); + let (resp_tx, _resp_rx) = >::new(); + let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + let event = InboundEvent::ReceiveVoteRequest( + VoteRequest { + term: 999, + candidate_id: 2, + last_log_index: 0, + last_log_term: 0, + }, + resp_tx, + ); + state.handle_inbound_event(event, &context, internal_event_tx).await.unwrap(); + + assert_eq!(state.current_term(), term_before, "term must not change"); + assert!(lease.is_valid(now_ms()), "lease must stay valid"); +} + +/// A leader without a valid lease still steps down on a higher-term VoteRequest. +/// +/// Expected: +/// - The guard is bounded by the lease: with no quorum acknowledgement in the window the +/// leader may really have lost leadership, so Raft's usual "higher term wins" applies. +/// - After the Raft loop runs `become_follower()`, the lease stays invalid. +#[tokio::test] +async fn test_receive_higher_term_vote_request_without_valid_lease_steps_down() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let context = MockBuilder::new(graceful_rx) + .with_db_path("/tmp/test_vote_request_no_lease_steps_down") + .build_context(); + + let mut state = LeaderState::::new(1, context.node_config.clone()); + let lease = Arc::clone(&state.shared_state.read_lease); + assert!( + !lease.is_valid(now_ms()), + "precondition: lease was never renewed, so it is invalid" + ); + let (resp_tx, _resp_rx) = >::new(); let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); let event = InboundEvent::ReceiveVoteRequest( @@ -205,23 +237,57 @@ async fn test_vote_request_higher_term_lease_revoked_before_become_follower() { }, resp_tx, ); - state.handle_inbound_event(event, &context, internal_event_tx).await.ok(); + state.handle_inbound_event(event, &context, internal_event_tx).await.unwrap(); assert!( matches!( internal_event_rx.try_recv(), Ok(InternalEvent::BecomeFollower(_)) ), - "higher-term VoteRequest must emit BecomeFollower" + "higher-term VoteRequest without a valid lease must emit BecomeFollower" ); - // become_follower() is intentionally NOT called here — the Raft loop has not yet - // processed the BecomeFollower event. The lease must already be invalid. - assert!( - !lease.is_valid(now_ms()), - "lease must be revoked at detection point, not waiting for become_follower() \ - — window-period fix required in handle_inbound_event VoteRequest branch" + let _ = state.become_follower().expect("become_follower must succeed"); + assert!(!lease.is_valid(now_ms()), "lease must stay invalid"); +} + +/// Stepping down on a higher-term VoteRequest adopts the new term and replays the request. +/// +/// Expected (leader without a valid lease): +/// - The term becomes the candidate's term at the detection point. +/// - The VoteRequest is replayed to the new Follower role, which decides on the vote. +#[tokio::test] +async fn test_vote_request_higher_term_without_valid_lease_adopts_term_and_replays() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let context = MockBuilder::new(graceful_rx) + .with_db_path("/tmp/test_vote_request_no_lease_replays") + .build_context(); + + let mut state = LeaderState::::new(1, context.node_config.clone()); + assert!(!state.is_lease_valid(), "precondition: lease invalid"); + + let (resp_tx, _resp_rx) = >::new(); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + let event = InboundEvent::ReceiveVoteRequest( + VoteRequest { + term: 999, + candidate_id: 2, + last_log_index: 0, + last_log_term: 0, + }, + resp_tx, ); + state.handle_inbound_event(event, &context, internal_event_tx).await.unwrap(); + + assert_eq!(state.current_term(), 999, "term must be adopted"); + assert!(matches!( + internal_event_rx.try_recv(), + Ok(InternalEvent::BecomeFollower(_)) + )); + assert!(matches!( + internal_event_rx.try_recv(), + Ok(InternalEvent::ReprocessEvent(_)) + )); } /// Higher-term AppendEntries → lease revoked BEFORE Raft loop calls become_follower(). @@ -237,7 +303,7 @@ async fn test_append_entries_higher_term_lease_revoked_before_become_follower() let mut state = LeaderState::::new(1, context.node_config.clone()); state.update_current_term(10); - let lease = Arc::clone(&state.shared_state.lease); + let lease = Arc::clone(&state.shared_state.read_lease); state.test_update_lease_timestamp(); assert!( @@ -287,7 +353,7 @@ async fn test_append_result_higher_term_lease_revoked_before_become_follower() { .build_context(); let mut state = LeaderState::::new(1, context.node_config.clone()); - let lease = Arc::clone(&state.shared_state.lease); + let lease = Arc::clone(&state.shared_state.read_lease); state.test_update_lease_timestamp(); assert!( @@ -395,7 +461,7 @@ async fn test_append_result_higher_term_revokes_lease() { .build_context(); let mut state = LeaderState::::new(1, context.node_config.clone()); - let lease = Arc::clone(&state.shared_state.lease); + let lease = Arc::clone(&state.shared_state.read_lease); state.test_update_lease_timestamp(); assert!( @@ -430,3 +496,245 @@ async fn test_append_result_higher_term_revokes_lease() { "lease must be revoked after AppendResult step-down" ); } + +/// A new leader is protected from a higher-term VoteRequest as soon as a quorum has +/// acknowledged it, even while the read lease is still withheld. +/// +/// Scenario: +/// - Leader just elected: noop at index 101, state machine applied only up to 100. +/// - A quorum ACK arrives. The read lease stays withheld (readers fall back to the Raft loop), +/// but a quorum has just confirmed this leader. +/// - A node returning from a partition asks for a vote with term 999. +/// +/// Expected: +/// - The vote request is rejected and the leader stays leader. "A quorum recently confirmed me" +/// is what makes a step-down pointless; it must not depend on the state machine catching up. +#[tokio::test] +async fn test_leader_before_noop_applied_still_rejects_higher_term_vote_request() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let context = MockBuilder::new(graceful_rx) + .with_db_path("/tmp/test_vote_request_before_noop_applied") + .build_context(); + + let mut state = LeaderState::::new(1, context.node_config.clone()); + let lease = Arc::clone(&state.shared_state.read_lease); + let leader_term = state.current_term(); + state.noop_log_id = Some(101); + state.last_heartbeat_send_ts = now_ms(); + state.test_renew_lease_from_send_ts(60_000, 100); + assert!( + !lease.is_valid(now_ms()), + "precondition: read lease withheld until the noop is applied" + ); + + let (resp_tx, mut resp_rx) = >::new(); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + let event = InboundEvent::ReceiveVoteRequest( + VoteRequest { + term: 999, + candidate_id: 2, + last_log_index: 0, + last_log_term: 0, + }, + resp_tx, + ); + state.handle_inbound_event(event, &context, internal_event_tx).await.unwrap(); + + assert!( + internal_event_rx.try_recv().is_err(), + "a leader a quorum just acknowledged must not step down on a higher-term VoteRequest" + ); + assert_eq!(state.current_term(), leader_term, "term must not change"); + let response = resp_rx.recv().await.unwrap().unwrap(); + assert!(!response.vote_granted); + assert_eq!(response.term, leader_term); +} + +/// Once the quorum acknowledgement is older than the lease window, a higher-term +/// VoteRequest makes the leader step down again. +/// +/// Expected: +/// - The protection is time-bounded: a leader nobody has confirmed recently may really have +/// lost leadership, so "higher term wins" applies. +#[tokio::test] +async fn test_leader_with_expired_quorum_ack_steps_down_on_higher_term_vote_request() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let context = MockBuilder::new(graceful_rx) + .with_db_path("/tmp/test_vote_request_expired_quorum_ack") + .build_context(); + + let mut state = LeaderState::::new(1, context.node_config.clone()); + state.noop_log_id = Some(1); + state.last_heartbeat_send_ts = now_ms(); + // 1 ms window, then let it pass. + state.test_renew_lease_from_send_ts(1, 1); + std::thread::sleep(std::time::Duration::from_millis(5)); + + let (resp_tx, _resp_rx) = >::new(); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + let event = InboundEvent::ReceiveVoteRequest( + VoteRequest { + term: 999, + candidate_id: 2, + last_log_index: 0, + last_log_term: 0, + }, + resp_tx, + ); + state.handle_inbound_event(event, &context, internal_event_tx).await.unwrap(); + + assert!(matches!( + internal_event_rx.try_recv(), + Ok(InternalEvent::BecomeFollower(_)) + )); +} + +// ============================================================================ +// PreVote requests received by a Leader +// +// The election handler decides (it is mocked here); the Leader's job is to tell it whether a +// quorum recently confirmed it, to forward the answer unchanged, and to change nothing: a +// PreVote never makes a leader step down, take a term, or persist anything. +// ============================================================================ + +/// An election mock that expects exactly one PreVote and records the `leader_active` it got. +fn pre_vote_election_mock( + seen_leader_active: Arc +) -> crate::MockElectionCore { + let mut election_handler = crate::MockElectionCore::::new(); + election_handler.expect_handle_pre_vote_request().times(1).returning( + move |_, current_term, _, leader_active| { + seen_leader_active.store(leader_active, std::sync::atomic::Ordering::SeqCst); + d_engine_proto::server::election::VoteResponse { + term: current_term, + vote_granted: !leader_active, + last_log_index: 7, + last_log_term: 3, + } + }, + ); + election_handler +} + +/// Sends a PreVote (term 999) to the leader and returns what the handler was told, the +/// response, whether any internal event (e.g. BecomeFollower) was emitted, and the leader's +/// term afterwards. +async fn leader_pre_vote( + state: &mut LeaderState, + context: &crate::RaftContext, +) -> (d_engine_proto::server::election::VoteResponse, bool) { + let (resp_tx, mut resp_rx) = >::new(); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + state + .handle_inbound_event( + InboundEvent::ReceivePreVoteRequest( + VoteRequest { + term: 999, + candidate_id: 2, + last_log_index: 0, + last_log_term: 0, + }, + resp_tx, + ), + context, + internal_event_tx, + ) + .await + .unwrap(); + let response = resp_rx.recv().await.unwrap().unwrap(); + (response, internal_event_rx.try_recv().is_ok()) +} + +/// Test: a leader that no quorum has confirmed yet tells the handler no leader is active, +/// but still does not step down. +/// +/// Expected: +/// - `leader_active == false`, the handler's answer is forwarded, no event is emitted, the +/// term is unchanged. +#[tokio::test] +async fn test_leader_pre_vote_without_quorum_ack_reports_inactive_and_does_not_step_down() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let seen = Arc::new(std::sync::atomic::AtomicBool::new(true)); + let context = MockBuilder::new(graceful_rx) + .with_db_path("/tmp/test_leader_pre_vote_no_ack") + .with_election_handler(pre_vote_election_mock(Arc::clone(&seen))) + .build_context(); + let mut state = LeaderState::::new(1, context.node_config.clone()); + let term_before = state.current_term(); + + let (response, emitted_event) = leader_pre_vote(&mut state, &context).await; + + assert!(!seen.load(std::sync::atomic::Ordering::SeqCst)); + assert!(response.vote_granted); + assert!( + !emitted_event, + "a PreVote must never make a leader step down" + ); + assert_eq!(state.current_term(), term_before); +} + +/// Test: a leader that a quorum just confirmed tells the handler a leader is active, so the +/// PreVote is denied, and does not step down. +#[tokio::test] +async fn test_leader_pre_vote_with_recent_quorum_ack_reports_active_leader() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let seen = Arc::new(std::sync::atomic::AtomicBool::new(false)); + let context = MockBuilder::new(graceful_rx) + .with_db_path("/tmp/test_leader_pre_vote_recent_ack") + .with_election_handler(pre_vote_election_mock(Arc::clone(&seen))) + .build_context(); + let mut state = LeaderState::::new(1, context.node_config.clone()); + simulate_quorum_ack(&mut state); + let term_before = state.current_term(); + + let (response, emitted_event) = leader_pre_vote(&mut state, &context).await; + + assert!(seen.load(std::sync::atomic::Ordering::SeqCst)); + assert!(!response.vote_granted); + assert!(!emitted_event); + assert_eq!(state.current_term(), term_before); +} + +/// Test: before the noop is applied the read lease is withheld, but a quorum has confirmed the +/// leader, so a PreVote must still be reported as "leader active". +/// +/// Expected: +/// - `leader_active == true`. The protection must not depend on the state machine catching up. +#[tokio::test] +async fn test_leader_pre_vote_before_noop_applied_still_reports_active_leader() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let seen = Arc::new(std::sync::atomic::AtomicBool::new(false)); + let context = MockBuilder::new(graceful_rx) + .with_db_path("/tmp/test_leader_pre_vote_before_noop_applied") + .with_election_handler(pre_vote_election_mock(Arc::clone(&seen))) + .build_context(); + let mut state = LeaderState::::new(1, context.node_config.clone()); + state.noop_log_id = Some(101); + state.last_heartbeat_send_ts = now_ms(); + state.test_renew_lease_from_send_ts(60_000, 100); + + leader_pre_vote(&mut state, &context).await; + + assert!(seen.load(std::sync::atomic::Ordering::SeqCst)); +} + +/// Test: once the quorum acknowledgement is older than the lease window the leader no longer +/// reports itself active (the protection is time-bounded). +#[tokio::test] +async fn test_leader_pre_vote_with_expired_quorum_ack_reports_inactive() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let seen = Arc::new(std::sync::atomic::AtomicBool::new(true)); + let context = MockBuilder::new(graceful_rx) + .with_db_path("/tmp/test_leader_pre_vote_expired_ack") + .with_election_handler(pre_vote_election_mock(Arc::clone(&seen))) + .build_context(); + let mut state = LeaderState::::new(1, context.node_config.clone()); + state.noop_log_id = Some(1); + state.last_heartbeat_send_ts = now_ms(); + state.test_renew_lease_from_send_ts(1, 1); + std::thread::sleep(std::time::Duration::from_millis(5)); + + leader_pre_vote(&mut state, &context).await; + + assert!(!seen.load(std::sync::atomic::Ordering::SeqCst)); +} diff --git a/d-engine-core/src/raft_role/leader_state_test/client_read_test.rs b/d-engine-core/src/raft_role/leader_state_test/client_read_test.rs index 7eb4bfa0..23abb36d 100644 --- a/d-engine-core/src/raft_role/leader_state_test/client_read_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/client_read_test.rs @@ -807,6 +807,8 @@ async fn test_expired_lease_single_voter_refreshed_immediately() { membership.expect_voters().returning(Vec::new); membership.expect_replication_peers().returning(Vec::new); state.init_cluster_metadata(&Arc::new(membership)).await.unwrap(); + // Lease is only published once this term's noop is committed (Raft §8). + state.on_noop_committed(&context).unwrap(); assert!(!state.is_lease_valid(), "Precondition: lease expired"); @@ -1196,11 +1198,24 @@ async fn test_lease_reuse_after_linearizable_read_refresh() { raft_log.expect_calculate_majority_matched_index().returning(|_, _, _| None); raft_log.expect_close().returning(|| ()); + // The noop sits at index 1 (mock `last_entry_id`). The lease is only published once the + // state machine has applied the noop (Raft §8), so report applied = 1. + // Built from scratch: with the default mock, its `last_applied() == 0` expectation is + // registered first and wins over a later one. + let mut state_machine = crate::MockStateMachine::new(); + state_machine.expect_is_running().returning(|| true); + state_machine + .expect_last_applied() + .return_const(d_engine_proto::common::LogId { index: 1, term: 1 }); + state_machine.expect_update_last_applied().returning(|_| ()); + state_machine.expect_persist_last_applied().returning(|_| Ok(())); + let ctx = MockBuilder::new(shutdown_rx) .with_db_path("/tmp/test_lease_reuse") .with_node_config(node_config) .with_replication_handler(replication) .with_raft_log(raft_log) + .with_state_machine(state_machine) .build_context(); let mut state = LeaderState::::new(1, ctx.node_config.clone()); @@ -1230,7 +1245,16 @@ async fn test_lease_reuse_after_linearizable_read_refresh() { // Verify: Lease is now valid (refreshed by LinearizableRead) // After flush in single-voter: lease refreshed via handle_log_flushed - // Manually trigger to simulate the async flush event + // Manually trigger to simulate the async flush event. + // Lease is only published once this term's noop is committed (Raft §8); committed + // here, after the read, so the LinearizableRead path above runs exactly as before. + state.on_noop_committed(&ctx).unwrap(); + use crate::StateMachine as _; + assert_eq!( + (state.noop_log_id, ctx.state_machine().last_applied().index), + (Some(1), 1), + "precondition: noop applied" + ); state.handle_log_flushed(1, &ctx, &internal_event_tx).await; assert!( 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 319b2619..ce49ee36 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 @@ -69,6 +69,15 @@ async fn assert_client_response( } } +/// Client writes are admitted only while a quorum has answered recently: record one quorum ACK. +fn admit_client_writes( + state: &mut LeaderState, + lease_duration_ms: u64, +) { + state.last_heartbeat_send_ts = crate::raft_role::read_lease::now_ms(); + state.test_renew_lease_from_send_ts(lease_duration_ms, 0); +} + /// Initialize the test environment and return the core components async fn setup_process_raft_request_test_context( test_name: &str, @@ -83,6 +92,10 @@ async fn setup_process_raft_request_test_context( let mut state = LeaderState::new(1, raft_context.node_config()); state.update_commit_index(4).expect("Should succeed to update commit index"); + admit_client_writes( + &mut state, + raft_context.node_config().raft.read_consistency.lease_duration_ms, + ); // Initialize the mock object let mut replication_handler = MockReplicationCore::new(); @@ -564,6 +577,10 @@ async fn test_drain_single_write_no_delay() { .build_context(); let mut state = LeaderState::::new(1, ctx.node_config.clone()); + admit_client_writes( + &mut state, + ctx.node_config.raft.read_consistency.lease_duration_ms, + ); let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); @@ -663,6 +680,10 @@ async fn test_drain_multiple_writes_natural_batch() { .build_context(); let mut state = LeaderState::::new(1, ctx.node_config.clone()); + admit_client_writes( + &mut state, + ctx.node_config.raft.read_consistency.lease_duration_ms, + ); let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); @@ -841,6 +862,10 @@ async fn test_write_batch_single_replication() { .build_context(); let mut state = LeaderState::::new(1, ctx.node_config.clone()); + admit_client_writes( + &mut state, + ctx.node_config.raft.read_consistency.lease_duration_ms, + ); let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); @@ -969,6 +994,10 @@ async fn test_client_write_deferred_until_sm_apply() { .build_context(); let mut state = LeaderState::::new(1, ctx.node_config.clone()); + admit_client_writes( + &mut state, + ctx.node_config.raft.read_consistency.lease_duration_ms, + ); let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); let req = ClientWriteRequest { diff --git a/d-engine-core/src/raft_role/leader_state_test/commit_index_test.rs b/d-engine-core/src/raft_role/leader_state_test/commit_index_test.rs index 33c127da..2c98710c 100644 --- a/d-engine-core/src/raft_role/leader_state_test/commit_index_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/commit_index_test.rs @@ -317,6 +317,10 @@ async fn test_handle_append_result_computes_quorum_once_per_ack() { let mut state = LeaderState::::new(1, ctx.node_config.clone()); state.init_cluster_metadata(&ctx.membership).await.unwrap(); assert!(!state.cluster_metadata.single_voter); + // The lease is only published once this term's noop is committed and applied + // (Raft §8). This test is about the quorum computation, so start past that point: + // noop at index 0 is trivially applied (the mock state machine reports applied = 0). + state.noop_log_id = Some(0); ctx.handlers .replication_handler 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 a5da5f0c..331f685b 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 @@ -355,6 +355,92 @@ async fn test_handle_cluster_conf_update_step_down_on_higher_term() { assert!(resp_rx.recv().await.is_err()); // Original sender should not get response } +/// ClusterConfUpdate with a higher term revokes the read lease at the detection point. +/// +/// Same window-period contract as the VoteRequest / AppendEntries step-down paths: the Raft +/// loop has not processed `BecomeFollower` yet, but a ReadActor reading the shared lease must +/// already see it invalid. +#[tokio::test] +#[traced_test] +async fn test_cluster_conf_update_higher_term_revokes_lease_before_become_follower() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let mut context = mock_raft_context( + "/tmp/test_cluster_conf_update_revokes_lease_early", + graceful_rx, + None, + ); + let mut membership = create_mock_membership(); + membership.expect_can_rejoin().returning(|_, _| Ok(())); + membership.expect_get_cluster_conf_version().returning(|| 1); + context.membership = Arc::new(membership); + + let mut state = LeaderState::::new(1, context.node_config.clone()); + state.shared_state_mut().update_current_term(3); + let lease = Arc::clone(&state.shared_state.read_lease); + state.test_update_lease_timestamp(); + assert!( + lease.is_valid(crate::now_ms()), + "precondition: lease must be valid" + ); + + let request = ClusterConfChangeRequest { + id: 2, + term: 5, + version: 1, + change: None, + }; + use crate::maybe_clone_oneshot::MaybeCloneOneshot; + let (resp_tx, _resp_rx) = >::new(); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + state + .handle_inbound_event( + InboundEvent::ClusterConfUpdate(request, resp_tx), + &context, + internal_event_tx, + ) + .await + .unwrap(); + + assert!(matches!( + internal_event_rx.try_recv(), + Ok(InternalEvent::BecomeFollower(Some(2))) + )); + // become_follower() is intentionally NOT called: the Raft loop has not run yet. + assert!( + !lease.is_valid(crate::now_ms()), + "lease must be revoked at the detection point, before the Raft loop handles BecomeFollower" + ); +} + +/// A leader that removes itself from the cluster revokes the read lease when it decides to +/// step down, not when the Raft loop later processes `BecomeFollower`. +#[tokio::test] +async fn test_step_down_self_removed_revokes_lease_before_become_follower() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let raft_context = mock_raft_context("/tmp/test_self_removed_revokes_lease", graceful_rx, None); + + let node_config = raft_context.node_config(); + let mut leader_state = LeaderState::::new(1, node_config); + let lease = Arc::clone(&leader_state.shared_state.read_lease); + leader_state.test_update_lease_timestamp(); + assert!( + lease.is_valid(crate::now_ms()), + "precondition: lease must be valid" + ); + + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); + leader_state.handle_self_removed(&internal_event_tx).unwrap(); + + assert!(matches!( + internal_event_rx.try_recv(), + Ok(InternalEvent::BecomeFollower(None)) + )); + assert!( + !lease.is_valid(crate::now_ms()), + "lease must be revoked at the detection point, before the Raft loop handles BecomeFollower" + ); +} + // ============================================================================ // AppendEntries Event Tests // ============================================================================ @@ -531,6 +617,12 @@ async fn test_handle_client_propose_success() { // New state let mut state = LeaderState::::new(1, context.node_config.clone()); + // Client writes are admitted only while a quorum has answered recently. + state.last_heartbeat_send_ts = crate::now_ms(); + state.test_renew_lease_from_send_ts( + context.node_config.raft.read_consistency.lease_duration_ms, + 0, + ); // Handle inbound event use crate::maybe_clone_oneshot::MaybeCloneOneshot; diff --git a/d-engine-core/src/raft_role/leader_state_test/lease_refresh_on_log_flushed_test.rs b/d-engine-core/src/raft_role/leader_state_test/lease_refresh_on_log_flushed_test.rs index e2145dad..63729268 100644 --- a/d-engine-core/src/raft_role/leader_state_test/lease_refresh_on_log_flushed_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/lease_refresh_on_log_flushed_test.rs @@ -82,6 +82,10 @@ async fn setup_single_voter_leader( "Setup failed: expected single_voter=true" ); + // The lease is only published once this term's noop is committed (Raft §8). + // These tests are about refresh-on-flush, not election, so start past that point. + state.on_noop_committed(&ctx).unwrap(); + (state, ctx, last_entry_id) } diff --git a/d-engine-core/src/raft_role/leader_state_test/lease_send_ts_test.rs b/d-engine-core/src/raft_role/leader_state_test/lease_send_ts_test.rs index 7471118d..7b2e815e 100644 --- a/d-engine-core/src/raft_role/leader_state_test/lease_send_ts_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/lease_send_ts_test.rs @@ -53,6 +53,8 @@ async fn setup_leader( async fn test_lease_deadline_anchored_to_heartbeat_send_time_not_ack_time() { let lease_duration_ms = 100u64; let (mut state, ctx) = setup_leader(lease_duration_ms).await; + // Lease is only published once this term's noop is committed (Raft §8). + state.on_noop_committed(&ctx).unwrap(); let send_ts = now_ms(); state.last_heartbeat_send_ts = send_ts; @@ -61,19 +63,23 @@ async fn test_lease_deadline_anchored_to_heartbeat_send_time_not_ack_time() { std::thread::sleep(Duration::from_millis(5)); // Quorum ACK arrives — renew using send_ts (new impl). - state.test_renew_lease_from_send_ts(ctx.node_config().raft.read_consistency.lease_duration_ms); + let applied = state.noop_log_id.expect("noop committed in setup above"); + state.test_renew_lease_from_send_ts( + ctx.node_config().raft.read_consistency.lease_duration_ms, + applied, + ); let term = state.current_term(); let new_deadline = send_ts + lease_duration_ms; // Just before the send-anchored deadline: still valid. assert!( - state.shared_state.lease.is_valid_for_leader(term, new_deadline - 1), + state.shared_state.read_lease.is_valid_for_leader(term, new_deadline - 1), "lease must be valid just before send-anchored deadline" ); // At the send-anchored deadline: expired. assert!( - !state.shared_state.lease.is_valid_for_leader(term, new_deadline), + !state.shared_state.read_lease.is_valid_for_leader(term, new_deadline), "lease must be expired at send_ts + lease_duration_ms" ); @@ -97,7 +103,7 @@ async fn test_no_quorum_ack_does_not_advance_lease_deadline() { let term = state.current_term(); assert!( - !state.shared_state.lease.is_valid_for_leader(term, now_ms()), + !state.shared_state.read_lease.is_valid_for_leader(term, now_ms()), "lease must remain invalid when no quorum ACK has been received" ); } @@ -138,3 +144,430 @@ fn test_new_impl_preserves_election_timeout_invariant_old_impl_violates_it() { (documents the pre-fix violation)" ); } + +/// A new leader must not publish a lease to readers before its own noop is committed +/// (Raft §8). +/// +/// Scenario: +/// - Node is elected; the previous leader had committed entry X, but this node's state +/// machine has not applied X yet (it only learns the commit point once its noop commits). +/// - A quorum ACK arrives before the noop is committed. +/// +/// Expected: +/// - The lease stays invalid for readers (`ReadActor` only looks at this lease). +/// Otherwise a LeaseRead right after the election can return a value older than a write +/// the previous leader already acknowledged to its client. +/// - Once the noop is committed, the next quorum ACK makes the lease valid. +#[tokio::test] +async fn test_lease_not_published_to_readers_before_noop_committed() { + let lease_duration_ms = 1000u64; + let (mut state, ctx) = setup_leader(lease_duration_ms).await; + assert!( + state.noop_log_id.is_none(), + "precondition: freshly elected leader has not committed its noop" + ); + + state.last_heartbeat_send_ts = now_ms(); + // Applied position is irrelevant here: there is no noop yet to compare against. + state.test_renew_lease_from_send_ts(lease_duration_ms, u64::MAX); + + assert!( + !state.shared_state.read_lease.is_valid(now_ms()), + "lease must not be visible to readers before the noop is committed" + ); + + state.on_noop_committed(&ctx).unwrap(); + let applied = state.noop_log_id.unwrap(); + state.last_heartbeat_send_ts = now_ms(); + state.test_renew_lease_from_send_ts(lease_duration_ms, applied); + + assert!( + state.shared_state.read_lease.is_valid(now_ms()), + "lease must become valid on the first quorum ACK after the noop is committed" + ); +} + +/// A new leader must not publish a lease until its state machine has applied up to the +/// noop (Raft §8). +/// +/// Scenario: +/// - Noop is at index 101 and already committed (a quorum ACK arrived). +/// - The state machine has only applied up to 100, so an entry the previous leader committed +/// (say X=1 at index 100) may or may not be applied yet; the entries before the noop are +/// not guaranteed to be visible to a local read. +/// +/// Expected: +/// - Quorum ACK with `last_applied = 100`: lease stays invalid for readers (they fall back +/// to the Raft loop, which waits for `last_applied >= read_index`). +/// - Quorum ACK with `last_applied = 101`: lease becomes valid. +#[tokio::test] +async fn test_lease_not_published_to_readers_before_noop_applied() { + let lease_duration_ms = 1000u64; + let (mut state, _ctx) = setup_leader(lease_duration_ms).await; + let noop_index = 101u64; + state.noop_log_id = Some(noop_index); + + state.last_heartbeat_send_ts = now_ms(); + state.test_renew_lease_from_send_ts(lease_duration_ms, noop_index - 1); + + assert!( + !state.shared_state.read_lease.is_valid(now_ms()), + "lease must not be visible to readers while last_applied < noop index" + ); + + state.last_heartbeat_send_ts = now_ms(); + state.test_renew_lease_from_send_ts(lease_duration_ms, noop_index); + + assert!( + state.shared_state.read_lease.is_valid(now_ms()), + "lease must become valid once last_applied reaches the noop index" + ); +} + +// ============================================================================ +// Write admission: a leader no quorum has answered for a while stops accepting NEW writes +// +// Not required for safety (reads are guarded by the lease and by quorum confirmation), but an +// isolated leader would otherwise keep queueing writes that can never commit and keep telling +// clients "I am the leader". openraft's approach: while the leader has no recent quorum +// acknowledgement it rejects new writes (clients look for another leader); there is no role +// change, no term change and no election, so brief network jitter costs a few rejected writes +// instead of a leadership change. +// +// The decision uses `last_quorum_contact_ms` (set at election and on every quorum ACK): writes +// are admitted while it is no older than `election_timeout_min`, evaluated once per write batch +// when the batch is flushed. A new leader therefore has one window to collect its first ACK +// (the noop's). The longer `write_admission_election_timeout_multiple` x `election_timeout_max` +// is the CheckQuorum step-down limit, not the write rejection. +// ============================================================================ + +/// Scales every timing in the write-admission tests: a loaded CI host oversleeps and delays +/// tasks, so the margins between "inside" and "beyond" a window are 4x wider there. +fn time_scale() -> u64 { + if std::env::var("CI").is_ok() { 4 } else { 1 } +} + +fn write_admission_test_lease_ms() -> u64 { + 10 * time_scale() +} + +fn write_admission_test_election_max_ms() -> u64 { + 40 * time_scale() +} + +/// Silence long enough to be well past both the lease and the step-down limit (multiple 2). +fn write_admission_test_window_ms() -> u64 { + write_admission_test_election_max_ms() * 2 +} + +async fn write_admission_leader() -> ( + LeaderState, + crate::RaftContext, +) { + // No heartbeat during the test: only write admission is under test. + write_admission_leader_with_heartbeat(60_000).await +} + +async fn write_admission_leader_with_heartbeat( + heartbeat_ms: u64 +) -> ( + LeaderState, + crate::RaftContext, +) { + let (_shutdown_tx, shutdown_rx) = watch::channel(()); + let mut node_config = RaftNodeConfig::default(); + node_config.raft.read_consistency.lease_duration_ms = write_admission_test_lease_ms(); + node_config.raft.election.election_timeout_min = write_admission_test_election_max_ms() / 2; + node_config.raft.election.election_timeout_max = write_admission_test_election_max_ms(); + node_config.raft.replication.rpc_append_entries_clock_in_ms = heartbeat_ms; + let mut ctx = MockBuilder::new(shutdown_rx).with_node_config(node_config).build_context(); + + // Enough mocks for an ACCEPTED write batch to be processed (nothing is sent to peers). + let mut replication_handler = crate::MockReplicationCore::new(); + replication_handler + .expect_prepare_batch_requests() + .returning(|_, _, _, _, _, _| Ok(crate::PrepareResult::default())); + let mut raft_log = crate::MockRaftLog::new(); + raft_log.expect_last_entry_id().returning(|| 4); + ctx.handlers.replication_handler = replication_handler; + ctx.storage.raft_log = std::sync::Arc::new(raft_log); + + let mut state = LeaderState::::new(1, ctx.node_config.clone()); + state.update_commit_index(4).expect("commit index"); + state.cluster_metadata.single_voter = false; + state.cluster_metadata.total_voters = 3; + (state, ctx) +} + +/// A quorum ACK for a heartbeat sent just now (the noop is already applied). +fn quorum_acks_now(state: &mut LeaderState) { + state.noop_log_id = Some(1); + state.last_heartbeat_send_ts = now_ms(); + state.test_renew_lease_from_send_ts(write_admission_test_lease_ms(), 1); +} + +fn sleep_ms(ms: u64) { + std::thread::sleep(Duration::from_millis(ms)); +} + +/// Offers one client write and flushes the batch, as the Raft loop does. Returns `Some(error +/// code)` when the write was answered at once (rejected), `None` when it was accepted (the answer +/// comes later, after commit). +async fn offer_write( + state: &mut LeaderState, + ctx: &crate::RaftContext, +) -> Option { + use crate::client::ClientWriteRequest; + use crate::maybe_clone_oneshot::{MaybeCloneOneshot, RaftOneshot}; + + let (resp_tx, mut resp_rx) = >::new(); + state.push_client_cmd( + crate::ClientCmd::Propose( + ClientWriteRequest { + client_id: 1, + command: Some(bytes::Bytes::from_static(b"write")), + }, + resp_tx, + ), + ctx, + ); + let (internal_event_tx, _internal_event_rx) = tokio::sync::mpsc::unbounded_channel(); + state.flush_cmd_buffers(ctx, &internal_event_tx).await.unwrap(); + + match tokio::time::timeout(Duration::from_millis(1), resp_rx.recv()).await { + Err(_) => None, + Ok(answer) => Some( + answer + .expect("channel open") + .expect("rejection is a client response, not a transport error") + .error, + ), + } +} + +/// Test: a leader whose quorum acknowledgements stopped rejects NEW writes with `NotLeader`. +/// +/// Scenario: +/// - 3-node leader got a quorum ACK, then the network cut it off for longer than the window. +/// +/// Expected: +/// - The write is answered `NotLeader` at once; it is not queued (it could never commit). +/// - The leader does not step down: same term, still a leader. +#[tokio::test] +async fn test_leader_rejects_new_writes_when_quorum_acks_stop() { + let (mut state, ctx) = write_admission_leader().await; + quorum_acks_now(&mut state); + let term_before = state.current_term(); + sleep_ms(write_admission_test_window_ms() * 2); + + let rejection = offer_write(&mut state, &ctx).await; + + assert_eq!(rejection, Some(crate::client::ErrorCode::NotLeader)); + assert_eq!(state.current_term(), term_before); +} + +/// Test: a leader that keeps getting quorum ACKs accepts writes. +#[tokio::test] +async fn test_leader_accepts_writes_while_quorum_acks_continue() { + let (mut state, ctx) = write_admission_leader().await; + for _ in 0..4 { + quorum_acks_now(&mut state); + sleep_ms(write_admission_test_window_ms() / 4); + } + quorum_acks_now(&mut state); + + assert_eq!(offer_write(&mut state, &ctx).await, None); +} + +/// Test: admission reopens as soon as a quorum ACK arrives. +#[tokio::test] +async fn test_leader_accepts_writes_again_as_soon_as_a_quorum_acks() { + let (mut state, ctx) = write_admission_leader().await; + quorum_acks_now(&mut state); + sleep_ms(write_admission_test_window_ms() * 2); + assert_eq!( + offer_write(&mut state, &ctx).await, + Some(crate::client::ErrorCode::NotLeader), + "precondition: writes are rejected" + ); + + quorum_acks_now(&mut state); + + assert_eq!( + offer_write(&mut state, &ctx).await, + None, + "the partition healed" + ); +} + +/// Test: a single-voter leader is its own quorum and always accepts writes, with or without ACKs. +#[tokio::test] +async fn test_single_voter_leader_always_accepts_writes() { + let (mut state, ctx) = write_admission_leader().await; + state.cluster_metadata.single_voter = true; + state.cluster_metadata.total_voters = 1; + sleep_ms(write_admission_test_window_ms() * 2); + + assert_eq!(offer_write(&mut state, &ctx).await, None); +} + +/// Test: a new leader counts its election as the last quorum contact, so it accepts client +/// writes while its noop is still replicating; without any ACK it rejects them once +/// `election_timeout_min` has passed, and the first ACK reopens admission. +/// +/// Expected: +/// - Accepted right after the election, rejected after the window with no ACK, accepted again +/// after the first quorum ACK. The leader's own noop is an internal write and never goes +/// through admission. +#[tokio::test] +async fn test_new_leader_accepts_writes_for_one_window_then_needs_a_quorum_ack() { + let (mut state, ctx) = write_admission_leader().await; + + assert_eq!( + offer_write(&mut state, &ctx).await, + None, + "a new leader must take writes while its noop is replicating" + ); + + sleep_ms(write_admission_test_window_ms()); + + assert_eq!( + offer_write(&mut state, &ctx).await, + Some(crate::client::ErrorCode::NotLeader), + "no quorum ever answered this leader and the window is over" + ); + + quorum_acks_now(&mut state); + + assert_eq!(offer_write(&mut state, &ctx).await, None); +} + +// ============================================================================ +// CheckQuorum step-down: a leader no quorum has answered for +// `write_admission_election_timeout_multiple` x `election_timeout_max` steps down (same term). +// Writes are rejected much earlier (see above); this is the backstop that hands leadership over. +// ============================================================================ + +/// Runs one leader tick and returns the internal events it produced. +async fn tick_once( + state: &mut LeaderState, + ctx: &crate::RaftContext, +) -> Vec { + let (internal_event_tx, mut internal_event_rx) = tokio::sync::mpsc::unbounded_channel(); + let (raft_tx, _raft_rx) = tokio::sync::mpsc::channel(1); + state.tick(&internal_event_tx, &raft_tx, ctx).await.expect("tick"); + let mut events = vec![]; + while let Ok(event) = internal_event_rx.try_recv() { + events.push(event); + } + events +} + +fn became_follower(events: &[crate::InternalEvent]) -> bool { + events.iter().any(|e| matches!(e, crate::InternalEvent::BecomeFollower(_))) +} + +/// Test: no quorum ACK for longer than the limit makes the leader step down. +/// +/// Expected: +/// - The tick emits `BecomeFollower`. +#[tokio::test] +async fn test_leader_steps_down_when_no_quorum_ack_beyond_the_limit() { + let (mut state, ctx) = write_admission_leader().await; + quorum_acks_now(&mut state); + sleep_ms(write_admission_test_window_ms() * 2); + + let events = tick_once(&mut state, &ctx).await; + + assert!( + became_follower(&events), + "expected BecomeFollower, got: {events:?}" + ); +} + +/// Test: silence past the read lease but inside the limit does not step the leader down. +/// +/// Expected: +/// - The limit is `multiple x election_timeout_max`, not the (much shorter) lease: jitter must +/// not cost a leadership change. +#[tokio::test] +async fn test_leader_stays_when_silence_is_inside_the_limit() { + let (mut state, ctx) = write_admission_leader().await; + quorum_acks_now(&mut state); + sleep_ms(write_admission_test_lease_ms() * 3); + + let events = tick_once(&mut state, &ctx).await; + + assert!( + !became_follower(&events), + "must not step down on short silence, got: {events:?}" + ); +} + +/// Test: the limit follows `write_admission_election_timeout_multiple`. +/// +/// Expected: +/// - Silence of 1.5 x election_timeout_max: tolerated with multiple 2, steps down with multiple 1. +#[tokio::test] +async fn test_step_down_limit_follows_the_configured_multiple() { + let silence_ms = write_admission_test_election_max_ms() * 3 / 2; + let mut stepped_down = vec![]; + for multiple in [2, 1] { + let (mut state, mut ctx) = write_admission_leader().await; + let mut node_config = (*ctx.node_config).clone(); + node_config.raft.backpressure.write_admission_election_timeout_multiple = multiple; + ctx.node_config = std::sync::Arc::new(node_config); + state.node_config = ctx.node_config.clone(); + quorum_acks_now(&mut state); + sleep_ms(silence_ms); + stepped_down.push(became_follower(&tick_once(&mut state, &ctx).await)); + } + + assert_eq!( + stepped_down, + vec![false, true], + "multiple 2 tolerates, multiple 1 steps down" + ); +} + +/// Test: a single-voter leader is its own quorum and never steps down for lack of ACKs. +#[tokio::test] +async fn test_single_voter_leader_never_steps_down_for_lack_of_acks() { + let (mut state, ctx) = write_admission_leader().await; + state.cluster_metadata.single_voter = true; + state.cluster_metadata.total_voters = 1; + sleep_ms(write_admission_test_window_ms() * 2); + + let events = tick_once(&mut state, &ctx).await; + + assert!(!became_follower(&events), "got: {events:?}"); +} + +/// Test: after a step-down tick the leader's next tick deadline is in the future. +/// +/// The Raft loop is a `biased` select with the tick branch first. The step-down tick returns +/// before the heartbeat code re-arms the replication timer, so if the deadline stays in the +/// past the tick fires again at once, forever, and the branch that processes the +/// `BecomeFollower` event (and every client request) is never polled: the leader never leaves. +/// +/// Expected: +/// - `BecomeFollower` is emitted AND `next_deadline()` is later than now. +#[tokio::test] +async fn test_step_down_tick_does_not_leave_the_tick_branch_ready() { + let (mut state, ctx) = write_admission_leader_with_heartbeat(5).await; + quorum_acks_now(&mut state); + // Silent past the step-down limit, and past the replication deadline that fires the tick. + sleep_ms(write_admission_test_window_ms() * 2); + + let events = tick_once(&mut state, &ctx).await; + + assert!( + became_follower(&events), + "precondition: the leader must step down, got: {events:?}" + ); + assert!( + state.next_deadline() > tokio::time::Instant::now(), + "the tick branch must not stay ready after a step-down, or the biased Raft loop never \ + processes BecomeFollower and never answers a client" + ); +} diff --git a/d-engine-core/src/raft_role/leader_state_test/replication_test.rs b/d-engine-core/src/raft_role/leader_state_test/replication_test.rs index 0fef8c40..39b825e9 100644 --- a/d-engine-core/src/raft_role/leader_state_test/replication_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/replication_test.rs @@ -2638,3 +2638,396 @@ mod test_leader_update_next_index { assert_eq!(state.match_index.get(&2), Some(&11)); } } + +// ============================================================================ +// A5: "a quorum just answered me" must come from who replied, not from replicated history +// +// `calculate_majority_matched_index` answers the COMMIT question from every peer's stored +// `match_index`. The stored value of an unreachable peer never changes, so after a partition it +// keeps producing `Some` and an isolated leader would treat each reply of its one reachable +// follower as a fresh quorum confirmation. These tests pin the behavior from outside: they only +// look at the contact/lease fields, the write admission and the queued reads. +// ============================================================================ + +/// 5 voters (leader + peers 2..=5) in a quiet cluster: every `match_index` equals `commit_index` +/// (50, the leader's own noop), so the commit median is `Some(50)` no matter who replies. +async fn five_voter_partition_context(path: &str) -> ProcessRaftRequestTestContext { + let mut context = setup_commit_index_test_context(path, false).await; + + context.state.cluster_metadata.replication_targets.extend([ + NodeMeta { + id: 4, + address: "".to_string(), + status: NodeStatus::Active as i32, + role: Follower.into(), + }, + NodeMeta { + id: 5, + address: "".to_string(), + status: NodeStatus::Active as i32, + role: Follower.into(), + }, + ]); + context.state.cluster_metadata.total_voters = 5; + context.state.cluster_metadata.single_voter = false; + for peer in [2u32, 3, 4, 5] { + context.state.match_index.insert(peer, 50); + } + context.state.update_commit_index(50).unwrap(); + + // Short windows so the tests can wait them out. + let mut node_config = (*context.raft_context.node_config).clone(); + node_config.raft.read_consistency.lease_duration_ms = 10; + node_config.raft.election.election_timeout_min = 20; + node_config.raft.election.election_timeout_max = 40; + context.raft_context.node_config = Arc::new(node_config); + context.state.node_config = context.raft_context.node_config.clone(); + + // "Never confirmed": distinguishes a refresh from the construction-time value. + context.state.last_quorum_contact_ms = 0; + context.state.quorum_ack_deadline_ms = 0; + // The monotonic clock starts at its first use: let it move past 0 so a refresh is visible. + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + + context + .raft_context + .handlers + .replication_handler + .expect_handle_success_response() + .returning(|_, _, _, _| { + Ok(PeerUpdate { + match_index: Some(50), + next_index: 51, + success: true, + }) + }); + + // Same arithmetic as RaftLogCore::calculate_majority_matched_index: median of the stored + // peer match indexes plus the leader's own durable index (50). + let mut raft_log = MockRaftLog::new(); + raft_log.expect_last_entry_id().returning(|| 50); + raft_log + .expect_calculate_majority_matched_index() + .returning(|_, commit_index, mut matched| { + matched.push(50); + matched.sort_unstable_by(|a, b| b.cmp(a)); + let median = matched[matched.len() / 2]; + if median < commit_index { + None + } else { + Some(median) + } + }); + context.raft_context.storage.raft_log = Arc::new(raft_log); + let mut sm_handler = crate::MockStateMachineHandler::new(); + sm_handler.expect_read_from_state_machine().returning(|_| Some(vec![])); + context.raft_context.handlers.state_machine_handler = Arc::new(sm_handler); + + context +} + +/// The leader sends a new heartbeat round, then `peer` answers it. +async fn peer_answers_a_new_round( + context: &mut ProcessRaftRequestTestContext, + peer: u32, +) { + context.state.last_heartbeat_send_ts = crate::raft_role::read_lease::now_ms(); + let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + context + .state + .handle_append_result( + peer, + Ok(success_response(1, 50)), + &context.raft_context, + &internal_event_tx, + ) + .await + .unwrap(); +} + +/// Leader and ONE reachable peer are 2 of 5: no quorum. +/// +/// # Then +/// - `last_quorum_contact_ms` and the read-lease ACK deadline are not refreshed. +#[tokio::test] +#[traced_test] +async fn test_one_reachable_peer_of_four_does_not_confirm_quorum() { + let mut context = five_voter_partition_context("/tmp/test_one_reachable_peer_of_four").await; + + peer_answers_a_new_round(&mut context, 2).await; + + assert_eq!( + context.state.last_quorum_contact_ms, 0, + "2 of 5 reachable: a stale majority of match_index must not count as a quorum answer" + ); + assert_eq!(context.state.quorum_ack_deadline_ms, 0); +} + +/// Leader and TWO reachable peers are 3 of 5: a quorum. +/// +/// # Then +/// - After the first peer nothing is confirmed; after the second one both fields are refreshed. +#[tokio::test] +#[traced_test] +async fn test_two_reachable_peers_of_four_confirm_quorum() { + let mut context = five_voter_partition_context("/tmp/test_two_reachable_peers_of_four").await; + + peer_answers_a_new_round(&mut context, 2).await; + assert_eq!( + context.state.last_quorum_contact_ms, 0, + "2 of 5 is not a quorum yet" + ); + + peer_answers_a_new_round(&mut context, 3).await; + + assert!( + context.state.last_quorum_contact_ms > 0, + "leader + peers 2 and 3 = 3 of 5 is a quorum" + ); + assert!(context.state.quorum_ack_deadline_ms > 0); +} + +/// The partition lasts: one peer keeps answering every round, three never do. +/// +/// # Then +/// - After more than `election_timeout_min` the leader no longer admits writes and the vote +/// guard no longer claims a recent quorum ACK. The loop below never calls anything but the +/// normal reply path, so a refresh from stale history would keep both open forever. +#[tokio::test] +#[traced_test] +async fn test_leader_loses_write_admission_when_only_one_peer_keeps_answering() { + let mut context = five_voter_partition_context("/tmp/test_only_one_peer_keeps_answering").await; + + for _ in 0..8 { + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + peer_answers_a_new_round(&mut context, 2).await; + } + + assert!( + !context.state.is_write_admitted(), + "2 of 5 reachable for 80 ms (election_timeout_min is 20 ms): writes must be rejected" + ); + assert!(!context.state.has_recent_quorum_ack()); +} + +/// Reads that wait for a quorum confirmation (linearizable reads and lease reads waiting for the +/// lease) must stay queued while only a minority answers, and be served once a quorum does. +#[tokio::test] +#[traced_test] +async fn test_queued_reads_wait_for_a_real_quorum() { + use crate::ReadConsistencyPolicy; + use crate::client::ClientReadRequest; + use crate::convert::safe_kv_bytes; + use crate::raft_role::leader_state::PendingLeaseRead; + use tokio::time::{Duration, Instant}; + + let mut context = five_voter_partition_context("/tmp/test_queued_reads_wait").await; + + let read = |policy| ClientReadRequest { + client_id: 1, + consistency_policy: Some(policy), + keys: vec![safe_kv_bytes(1)], + }; + let (linearizable_tx, mut linearizable_rx) = MaybeCloneOneshot::new(); + let mut requests = VecDeque::new(); + requests.push_back(( + read(ReadConsistencyPolicy::LinearizableRead), + linearizable_tx, + )); + context.state.pending_reads.insert( + 0, + crate::raft_role::leader_state::PendingReadBatch { + deadline: Instant::now() + Duration::from_secs(5), + requests, + }, + ); + let (lease_tx, mut lease_rx) = MaybeCloneOneshot::new(); + context.state.pending_lease_reads.push_back(PendingLeaseRead { + request: read(ReadConsistencyPolicy::LeaseRead), + sender: lease_tx, + deadline: Instant::now() + Duration::from_secs(5), + }); + + peer_answers_a_new_round(&mut context, 2).await; + + assert_eq!( + context.state.pending_reads.len(), + 1, + "2 of 5: linearizable read must wait" + ); + assert_eq!( + context.state.pending_lease_reads.len(), + 1, + "2 of 5: lease read must wait" + ); + assert!( + tokio::time::timeout(Duration::from_millis(20), linearizable_rx.recv()) + .await + .is_err() + ); + assert!(tokio::time::timeout(Duration::from_millis(20), lease_rx.recv()).await.is_err()); + + peer_answers_a_new_round(&mut context, 3).await; + + assert!( + context.state.pending_reads.is_empty(), + "3 of 5: linearizable read is served" + ); + assert!( + context.state.pending_lease_reads.is_empty(), + "3 of 5: lease read is served" + ); + assert!(linearizable_rx.recv().await.unwrap().is_ok()); + assert!(lease_rx.recv().await.unwrap().is_ok()); +} + +/// Learners never count toward the quorum, whatever they reply. +/// +/// # Given +/// - 3 voters (leader, 2, 3) and 2 learners (4, 5), nobody confirmed anything yet +/// +/// # Then +/// - Both learners answering does not confirm a quorum; one voter answering does. +#[tokio::test] +#[traced_test] +async fn test_learner_replies_do_not_count_toward_quorum() { + let mut context = five_voter_partition_context("/tmp/test_learner_replies").await; + context.state.cluster_metadata.total_voters = 3; + for node in context.state.cluster_metadata.replication_targets.iter_mut() { + if node.id == 4 || node.id == 5 { + node.role = d_engine_proto::common::NodeRole::Learner.into(); + } + } + + peer_answers_a_new_round(&mut context, 4).await; + peer_answers_a_new_round(&mut context, 5).await; + assert_eq!( + context.state.last_quorum_contact_ms, 0, + "learners are not voters" + ); + + peer_answers_a_new_round(&mut context, 2).await; + assert!( + context.state.last_quorum_contact_ms > 0, + "leader + voter 2 = 2 of 3 voters is a quorum" + ); +} + +// ============================================================================ +// record_voter_ack: when does the quorum-acknowledged send time move forward? +// ============================================================================ + +/// The reply sequence of the design notes. 5 voters (leader + B=2, C=3, D=4, E=5), `need` = 2 +/// peers. A round is a heartbeat sent at time T; "x:T" means peer x acknowledged that round. +/// +/// ```text +/// reply peers' times quorum time returned +/// B:1000 {B:1000} none yet None +/// C:1000 {B:1000, C:1000} 1000 Some(1000) first quorum +/// D:1000 {B,C,D:1000} 1000 None same round, nothing new +/// B:1100 {B:1100, C:1000, D:1000} 1000 None only B reached the new round +/// C:1100 {B:1100, C:1100, D:1000} 1100 Some(1100) second peer reached it +/// D:1100 {B,C,D:1100} 1100 None +/// --- C, D, E are cut off, only B keeps answering --- +/// B:1200 {B:1200, C:1100, D:1100} 1100 None B alone cannot move it +/// B:1300 {B:1300, C:1100, D:1100} 1100 None +/// ``` +#[tokio::test] +#[traced_test] +async fn test_record_voter_ack_returns_a_time_only_when_the_quorum_time_moves_forward() { + let mut context = five_voter_partition_context("/tmp/test_record_voter_ack_sequence").await; + let (b, c, d) = (2u32, 3u32, 4u32); + let state = &mut context.state; + + let sequence = [ + (b, 1000, None), + (c, 1000, Some(1000)), + (d, 1000, None), + (b, 1100, None), + (c, 1100, Some(1100)), + (d, 1100, None), + (b, 1200, None), + (b, 1300, None), + ]; + for (step, (peer, send_ts, expected)) in sequence.into_iter().enumerate() { + assert_eq!( + state.record_voter_ack(peer, send_ts), + expected, + "step {step}: peer {peer} acknowledged the round sent at {send_ts}" + ); + } +} + +/// A recovered peer moves the quorum time forward again. +#[tokio::test] +#[traced_test] +async fn test_record_voter_ack_moves_forward_again_when_a_second_peer_recovers() { + let mut context = five_voter_partition_context("/tmp/test_record_voter_ack_recovery").await; + let (b, c, d) = (2u32, 3u32, 4u32); + let state = &mut context.state; + for peer in [b, c, d] { + state.record_voter_ack(peer, 1000); + } + // Partition: only B answers, the quorum time stays at 1000. + assert_eq!(state.record_voter_ack(b, 1100), None); + assert_eq!(state.record_voter_ack(b, 1200), None); + + // C comes back and answers the round sent at 1300 (B answers it too). + assert_eq!(state.record_voter_ack(b, 1300), None); + assert_eq!(state.record_voter_ack(c, 1300), Some(1300)); +} + +/// A membership change drops the times of peers that left and forgets the old conclusion. +/// +/// # Given +/// - 3 voters (leader, B, C): one peer is a quorum, B answered the round sent at 1400 +/// +/// # When +/// - the cluster grows to 5 voters (D and a new E join, the old E=5 is gone), then C answers +/// the round sent at 1450 +/// +/// # Then +/// - the removed peer's time is gone +/// - the quorum time is recomputed for 5 voters (2 peers needed, B:1400 and C:1450 -> 1400) +/// and reported once. Without the reset the stale 1400 of the 3-voter conclusion would hide it. +#[tokio::test] +#[traced_test] +async fn test_membership_change_drops_left_peers_and_recomputes_the_quorum_time() { + use crate::MockMembership; + use d_engine_proto::server::cluster::NodeMeta; + + let mut context = five_voter_partition_context("/tmp/test_membership_change_resets").await; + let state = &mut context.state; + state.cluster_metadata.total_voters = 3; + assert_eq!( + state.record_voter_ack(2, 1400), + Some(1400), + "3 voters: one peer is a quorum" + ); + state.peer_ack_send_ts.insert(5, 1300); // a voter that is about to leave + + let peer = |id: u32| NodeMeta { + id, + address: "".to_string(), + status: NodeStatus::Active as i32, + role: Follower.into(), + }; + let mut membership = MockMembership::new(); + membership + .expect_voters() + .returning(move || vec![peer(2), peer(3), peer(4), peer(6)]); + membership + .expect_replication_peers() + .returning(move || vec![peer(2), peer(3), peer(4), peer(6)]); + state.update_cluster_metadata(&Arc::new(membership)).await.unwrap(); + + assert!( + !state.peer_ack_send_ts.contains_key(&5), + "a peer that is no longer a voter must not keep counting" + ); + assert_eq!( + state.record_voter_ack(3, 1450), + Some(1400), + "5 voters: B:1400 and C:1450 make a quorum at 1400, which the old conclusion would hide" + ); +} diff --git a/d-engine-core/src/raft_role/learner_state.rs b/d-engine-core/src/raft_role/learner_state.rs index 7d5dcaf3..68a72e7e 100644 --- a/d-engine-core/src/raft_role/learner_state.rs +++ b/d-engine-core/src/raft_role/learner_state.rs @@ -195,6 +195,9 @@ impl RaftRoleState for LearnerState { let my_term = self.current_term(); match inbound_event { + InboundEvent::ReceivePreVoteRequest(_, sender) => { + self.deny_pre_vote(ctx, sender)?; + } InboundEvent::ReceiveVoteRequest(vote_request, sender) => { info!("handle_inbound_event::ReceiveVoteRequest. Learner cannot vote."); // 1. Update term FIRST if needed 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 07d32edb..1de5ef0e 100644 --- a/d-engine-core/src/raft_role/learner_state_test.rs +++ b/d-engine-core/src/raft_role/learner_state_test.rs @@ -2426,3 +2426,84 @@ fn test_learner_from_candidate_preserves_last_purged_index_resets_scheduled() { ); assert_eq!(learner.pending_purge_upto, None); } + +/// Test: a Learner cannot vote, so it denies every PreVote itself and never asks the election +/// handler (which could otherwise grant it). +/// +/// Expected: +/// - Denied with the Learner's own term; nothing changes (no events, term unchanged). +#[tokio::test] +async fn test_learner_pre_vote_is_denied_without_consulting_the_handler() { + let mut election_handler = crate::MockElectionCore::::new(); + election_handler.expect_handle_pre_vote_request().times(0); + + let (_graceful_tx, graceful_rx) = tokio::sync::watch::channel(()); + let context = MockBuilder::new(graceful_rx) + .with_db_path("/tmp/test_learner_pre_vote_denied") + .with_election_handler(election_handler) + .build_context(); + let mut state = LearnerState::::new(1, context.node_config.clone()); + let term_before = state.current_term(); + + let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); + let (internal_event_tx, mut internal_event_rx) = tokio::sync::mpsc::unbounded_channel(); + state + .handle_inbound_event( + InboundEvent::ReceivePreVoteRequest( + d_engine_proto::server::election::VoteRequest { + term: term_before + 1, + candidate_id: 2, + last_log_index: 9, + last_log_term: 9, + }, + resp_tx, + ), + &context, + internal_event_tx, + ) + .await + .unwrap(); + + let response = resp_rx.recv().await.unwrap().unwrap(); + assert!(!response.vote_granted); + assert_eq!(response.term, term_before); + assert_eq!(state.current_term(), term_before); + assert!(internal_event_rx.try_recv().is_err()); +} + +/// Test: a Learner adopts a higher term from a VoteRequest (it cannot vote, but it must not keep +/// a term the cluster has passed) and still denies the vote. +/// +/// Expected: +/// - Term becomes the request's term (persisted), the response is a denial, and the response +/// carries the Learner's term as it was when it answered. +#[tokio::test] +async fn test_learner_higher_term_vote_request_adopts_term_and_denies() { + let (_graceful_tx, graceful_rx) = tokio::sync::watch::channel(()); + let context = MockBuilder::new(graceful_rx) + .with_db_path("/tmp/test_learner_higher_term_vote_request") + .build_context(); + let mut state = LearnerState::::new(1, context.node_config.clone()); + let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); + let (internal_event_tx, _internal_event_rx) = tokio::sync::mpsc::unbounded_channel(); + + state + .handle_inbound_event( + InboundEvent::ReceiveVoteRequest( + d_engine_proto::server::election::VoteRequest { + term: 9, + candidate_id: 2, + last_log_index: 0, + last_log_term: 0, + }, + resp_tx, + ), + &context, + internal_event_tx, + ) + .await + .unwrap(); + + assert_eq!(state.current_term(), 9); + assert!(!resp_rx.recv().await.unwrap().unwrap().vote_granted); +} diff --git a/d-engine-core/src/raft_role/mod.rs b/d-engine-core/src/raft_role/mod.rs index 257d22e6..495671d4 100644 --- a/d-engine-core/src/raft_role/mod.rs +++ b/d-engine-core/src/raft_role/mod.rs @@ -105,9 +105,10 @@ pub struct SharedState { /// Performance optimization: avoid RwLock on AppendEntries path current_leader_id: AtomicU32, - /// Shared lease state between Raft loop (writer) and EmbeddedClient (reader). - /// Arc ensures the same allocation is shared across role transitions via clone(). - pub lease: Arc, + /// Read lease published to every node's ReadActor (EmbeddedClient fast path). + /// Only `LeaderState` writes it; followers never renew, so their LeaseRead is + /// always invalid by design. Shared via clone() across role transitions. + pub read_lease: Arc, } impl Clone for SharedState { @@ -117,7 +118,7 @@ impl Clone for SharedState { hard_state: self.hard_state, commit_index: self.commit_index, current_leader_id: AtomicU32::new(self.current_leader_id.load(Ordering::Acquire)), - lease: Arc::clone(&self.lease), + read_lease: Arc::clone(&self.read_lease), } } } @@ -175,7 +176,7 @@ impl SharedState { hard_state, commit_index: last_applied_index_option.unwrap_or(0), current_leader_id: AtomicU32::new(0), - lease: Arc::new(ReadLease::new()), + read_lease: Arc::new(ReadLease::new()), } } @@ -226,6 +227,12 @@ impl SharedState { if let Some(t) = term && t != self.hard_state.current_term { + // Raft §5.1: a new term starts with `votedFor = null`. A vote cast in an older + // term must not outlive the term change, or it blocks a legitimate vote request + // in the new term. A vote supplied by the caller is applied below. + if t > self.hard_state.current_term { + self.hard_state.voted_for = None; + } self.hard_state.current_term = t; changed = true; } @@ -259,12 +266,6 @@ impl SharedState { } } - /// Clears voted_for to None (e.g. entering a fresh term with no vote cast - /// yet). Private — only `commit_vote_reset()` may call this. - fn clear_voted_for(&mut self) { - self.hard_state.voted_for = None; - } - fn voted_for(&self) -> Result> { Ok(self.hard_state.voted_for) } diff --git a/d-engine-core/src/raft_role/role_state.rs b/d-engine-core/src/raft_role/role_state.rs index 7fcc1d3e..1986e19f 100644 --- a/d-engine-core/src/raft_role/role_state.rs +++ b/d-engine-core/src/raft_role/role_state.rs @@ -16,6 +16,7 @@ use crate::RaftLog; use crate::ReplicationCore; use crate::Result; use crate::SnapshotApplyResult; +use crate::StateMachine; use crate::StateMachineHandler; use crate::StateTransitionError; use crate::TypeConfig; @@ -333,24 +334,75 @@ pub(crate) trait RaftRoleState: Send + Sync + 'static { Ok(result.is_new_leader_commitment) } - /// Reset counterpart to `commit_hard_state()` — clears voted_for and - /// persists. Needed because `commit_hard_state`'s `Option` can't - /// distinguish "don't touch" from "clear to None". - fn commit_vote_reset( - &mut self, - ctx: &RaftContext, - ) -> Result<()> { - self.shared_state_mut().clear_voted_for(); - ctx.raft_log().save_hard_state(&self.shared_state().hard_state())?; - Ok(()) - } - //--- Timer related --- fn next_deadline(&self) -> Instant; fn is_timer_expired(&self) -> bool; fn reset_timer(&mut self); + /// True if this node is an active voter in its own membership view. Only an active voter + /// may start an election: a removed node (no entry) or a learner (`Promotable`/`ReadOnly`) + /// that campaigned would count its own vote as part of a quorum it does not belong to. + /// `Membership::voters()` lists the OTHER voters, so this node's own entry is the source. + fn is_active_voter( + &self, + ctx: &RaftContext, + ) -> bool { + ctx.membership() + .retrieve_node_meta(self.node_id()) + .is_some_and(|meta| meta.status == d_engine_proto::common::NodeStatus::Active as i32) + } + + /// True if a membership change is committed but not yet applied. A node that campaigned in + /// that window would count votes against a stale voter set (raft-rs refuses to campaign + /// here). Only reads the log when something is actually unapplied, so the common case is + /// two integer comparisons. + /// + /// A log read failure is reported as an error: the caller cannot prove that nothing is + /// pending and must not campaign. + fn has_unapplied_config_change( + &self, + ctx: &RaftContext, + ) -> Result { + let applied = ctx.state_machine().last_applied().index; + let committed = self.commit_index(); + if applied >= committed { + return Ok(false); + } + let entries = ctx.raft_log().get_entries_range(applied + 1..=committed)?; + Ok(entries + .iter() + .any(|entry| entry.payload.as_ref().is_some_and(|p| p.is_config()))) + } + + /// Record that a legitimate leader was just heard from. No-op for roles + /// that do not vote (Learner) or do not follow a leader (Candidate, Leader). + fn record_leader_contact(&mut self) {} + + /// Temporary: denies every PreVote with this node's term and last log id. + /// Replace per role with the real rules BEFORE `Candidate.tick` starts sending PreVotes, + /// otherwise no node can ever pass a PreVote round and no leader can be elected. + fn deny_pre_vote( + &self, + ctx: &RaftContext, + sender: MaybeCloneOneshotSender< + std::result::Result, + >, + ) -> Result<()> { + let last_log_id = ctx.raft_log().last_log_id().unwrap_or(LogId { index: 0, term: 0 }); + let response = d_engine_proto::server::election::VoteResponse { + term: self.current_term(), + vote_granted: false, + last_log_index: last_log_id.index, + last_log_term: last_log_id.term, + }; + sender.send(Ok(response)).map_err(|e| { + error!("Failed to send: {e:?}"); + NetworkError::SingalSendFailed(format!("{:?}", e)) + })?; + Ok(()) + } + async fn tick( &mut self, internal_event_tx: &mpsc::UnboundedSender, @@ -619,6 +671,7 @@ pub(crate) trait RaftRoleState: Send + Sync + 'static { // Legitimate leader (term >= ours) confirmed — reset now, not earlier. self.reset_timer(); + self.record_leader_contact(); // Important to confirm heartbeat from Leader immediatelly let new_leader_id = append_entries_request.leader_id; @@ -825,6 +878,8 @@ pub(crate) trait RaftRoleState: Send + Sync + 'static { // Legitimate leader (term >= ours) confirmed — reset now, matching AppendEntries. self.reset_timer(); + // Recorded once per stream, on its first chunk; a very long transfer outlives the window. + self.record_leader_contact(); self.commit_hard_state( ctx, @@ -871,6 +926,11 @@ pub(crate) trait RaftRoleState: Send + Sync + 'static { // Sole write path: hand the received snapshot to the Worker, await the result. let install_result = ctx.state_machine_commands().install_snapshot(prepared).await; + // A long transfer outlives the election timer and the guard window. The leader was + // alive until just now, so re-arm both before the loop handles the next event. + self.reset_timer(); + self.record_leader_contact(); + // Raft §7: reply success only once the Worker has confirmed install (or a safe no-op). let _ = sender.send(Ok(SnapshotResponse { term: my_term, diff --git a/d-engine-core/src/raft_role/role_state_test.rs b/d-engine-core/src/raft_role/role_state_test.rs index 1a6e1c60..da342bed 100644 --- a/d-engine-core/src/raft_role/role_state_test.rs +++ b/d-engine-core/src/raft_role/role_state_test.rs @@ -548,38 +548,6 @@ async fn test_commit_hard_state_is_new_leader_commitment_semantics() { ); } -/// commit_vote_reset clears voted_for to None and persists exactly once. -#[tokio::test] -async fn test_commit_vote_reset_clears_and_persists() { - let (_graceful_tx, graceful_rx) = watch::channel(()); - let mut raft_log = MockRaftLog::new(); - // One persist to seed an initial vote, one more for the reset itself. - raft_log.expect_save_hard_state().times(2).returning(|_| Ok(())); - - let context = MockBuilder::new(graceful_rx).with_raft_log(raft_log).build_context(); - let mut state = CandidateState::::new(1, context.node_config.clone()); - - let vote = VotedFor { - voted_for_id: 2, - voted_for_term: 1, - committed: true, - }; - state.commit_hard_state(&context, None, Some(vote)).unwrap(); - assert_eq!( - state.voted_for().unwrap(), - Some(vote), - "precondition: vote is set" - ); - - state.commit_vote_reset(&context).unwrap(); - - assert_eq!( - state.voted_for().unwrap(), - None, - "voted_for must be cleared by commit_vote_reset" - ); -} - // ============================================================================ // handle_create_snapshot Tests (#436) // ============================================================================ @@ -769,3 +737,247 @@ async fn test_handle_create_snapshot_failure_emits_error_event() { other => panic!("expected SnapshotCreated(Err(..)), got: {other:?}"), } } + +// ============================================================================ +// A vote belongs to the term it was cast in (Raft §5.1: a new term starts with votedFor = null) +// +// raft-rs `reset(term)` clears the vote whenever the term changes; openraft replaces the whole +// (term, node) vote. If an old vote survives a term change, it blocks a legitimate vote request +// in the new term even though this node has not voted in that term. +// ============================================================================ + +fn recording_raft_log() -> ( + MockRaftLog, + std::sync::Arc>>, +) { + let saved = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let recorder = std::sync::Arc::clone(&saved); + let mut raft_log = MockRaftLog::new(); + raft_log.expect_save_hard_state().returning(move |state| { + recorder.lock().unwrap().push(*state); + Ok(()) + }); + (raft_log, saved) +} + +fn vote_for( + id: u32, + term: u64, +) -> VotedFor { + VotedFor { + voted_for_id: id, + voted_for_term: term, + committed: false, + } +} + +/// Test: adopting a higher term (without a new vote) clears the vote cast in the older term, +/// in memory and on disk. +/// +/// Scenario: +/// - The node voted for node 3 in term 1. +/// - It adopts term 2 because of a request it did not grant. +/// +/// Expected: +/// - `voted_for` is None and the persisted hard state says term 2 with no vote. +#[tokio::test] +async fn test_commit_hard_state_clears_older_vote_when_term_increases() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (raft_log, saved) = recording_raft_log(); + let context = MockBuilder::new(graceful_rx).with_raft_log(raft_log).build_context(); + let mut state = CandidateState::::new(1, context.node_config.clone()); + state.commit_hard_state(&context, None, Some(vote_for(3, 1))).unwrap(); + + state.commit_hard_state(&context, Some(2), None).unwrap(); + + assert_eq!(state.current_term(), 2); + assert_eq!( + state.voted_for().unwrap(), + None, + "the term-1 vote must not survive term 2" + ); + let last_saved = *saved.lock().unwrap().last().unwrap(); + assert_eq!((last_saved.current_term, last_saved.voted_for), (2, None)); +} + +/// Test: confirming the SAME term keeps the vote (e.g. a repeated request at the current term). +#[tokio::test] +async fn test_commit_hard_state_keeps_vote_when_term_is_unchanged() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (raft_log, _saved) = recording_raft_log(); + let context = MockBuilder::new(graceful_rx).with_raft_log(raft_log).build_context(); + let mut state = CandidateState::::new(1, context.node_config.clone()); + state.commit_hard_state(&context, None, Some(vote_for(3, 1))).unwrap(); + let term = state.current_term(); + + state.commit_hard_state(&context, Some(term), None).unwrap(); + + assert_eq!(state.voted_for().unwrap(), Some(vote_for(3, 1))); +} + +/// Test: when the term changes together with a new vote, the new vote wins (the clearing must not +/// swallow a vote supplied by the caller, e.g. a vote granted in the new term). +#[tokio::test] +async fn test_commit_hard_state_applies_new_vote_given_with_a_higher_term() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let (raft_log, saved) = recording_raft_log(); + let context = MockBuilder::new(graceful_rx).with_raft_log(raft_log).build_context(); + let mut state = CandidateState::::new(1, context.node_config.clone()); + state.commit_hard_state(&context, None, Some(vote_for(3, 1))).unwrap(); + + state.commit_hard_state(&context, Some(2), Some(vote_for(5, 2))).unwrap(); + + assert_eq!(state.voted_for().unwrap(), Some(vote_for(5, 2))); + let last_saved = *saved.lock().unwrap().last().unwrap(); + assert_eq!( + (last_saved.current_term, last_saved.voted_for), + (2, Some(vote_for(5, 2))) + ); +} + +// ============================================================================ +// Vote protocol, end to end through the real handler and the real hard state +// +// The single-step tests above mock the other half. These run the exact two lines a Follower +// executes for every VoteRequest (`handle_vote_request`, then `commit_hard_state` with its +// answer) against the real implementations, and "restart" a node by rebuilding it from the hard +// state it persisted. +// ============================================================================ + +/// Applies one VoteRequest the way `FollowerState` does and returns whether it was granted. +async fn grant_or_deny( + state: &mut impl RaftRoleState, + context: &crate::RaftContext, + request_term: u64, + candidate_id: u32, + candidate_log: (u64, u64), +) -> bool { + use crate::ElectionCore as _; + + let update = crate::election::ElectionHandler::::new(1) + .handle_vote_request( + d_engine_proto::server::election::VoteRequest { + term: request_term, + candidate_id, + last_log_index: candidate_log.0, + last_log_term: candidate_log.1, + }, + state.current_term(), + state.voted_for().unwrap(), + context.raft_log(), + ) + .await + .unwrap(); + state + .commit_hard_state(context, update.term_update, update.new_voted_for) + .unwrap(); + update.new_voted_for.is_some() +} + +fn vote_sequence_context() -> ( + crate::RaftContext, + std::sync::Arc>>, + watch::Sender<()>, +) { + let (graceful_tx, graceful_rx) = watch::channel(()); + let (mut raft_log, saved) = recording_raft_log(); + // The voter's own log: index 5, term 2. + raft_log + .expect_last_log_id() + .returning(|| Some(d_engine_proto::common::LogId { index: 5, term: 2 })); + let context = MockBuilder::new(graceful_rx).with_raft_log(raft_log).build_context(); + (context, saved, graceful_tx) +} + +/// Test: Election Safety. At most one candidate gets this node's vote in a term, also across a +/// restart. +/// +/// Scenario: +/// - Candidate 5 asks for a vote in term 2 (good log) and gets it. +/// - Candidate 6 asks in the same term: denied. +/// - The node restarts from the hard state it persisted. Candidate 6 asks again: still denied. +/// Candidate 5 asking again is granted again (a retry of the same request). +/// +/// Expected: +/// - Two different candidates never both hold this node's vote for the same term. If they could, +/// both can reach a majority and the cluster has two leaders in one term. +#[tokio::test] +async fn test_vote_is_granted_to_one_candidate_per_term_also_across_a_restart() { + let (context, saved, _graceful_tx) = vote_sequence_context(); + let mut node = FollowerState::::new(1, context.node_config.clone(), None, None); + node.shared_state_mut().update_current_term(1); + + assert!( + grant_or_deny(&mut node, &context, 2, 5, (5, 2)).await, + "first candidate" + ); + assert!( + !grant_or_deny(&mut node, &context, 2, 6, (5, 2)).await, + "second candidate" + ); + + let persisted = *saved.lock().unwrap().last().expect("the vote must have been persisted"); + let mut restarted = + FollowerState::::new(1, context.node_config.clone(), Some(persisted), None); + + assert!( + !grant_or_deny(&mut restarted, &context, 2, 6, (5, 2)).await, + "after a restart the node must remember its vote and still deny candidate 6" + ); + assert!( + grant_or_deny(&mut restarted, &context, 2, 5, (5, 2)).await, + "a retry from the candidate that already holds the vote is granted again" + ); +} + +/// Test: the business timeline of the stale-vote defect, through the real code. +/// +/// Scenario: +/// - The node votes for candidate 3 in term 1. +/// - Candidate 4 asks in term 2 with a log that is behind: the node adopts term 2 and denies. +/// - Candidate 5 asks in the same term 2 with a good log. +/// +/// Expected: +/// - Candidate 5 gets the vote: the node has not voted in term 2. (Before the fix the leftover +/// term-1 vote blocked it and candidate 5 had to retry in term 3.) +#[tokio::test] +async fn test_vote_from_an_older_term_does_not_block_a_good_candidate_in_the_next_term() { + let (context, _saved, _graceful_tx) = vote_sequence_context(); + let mut node = FollowerState::::new(1, context.node_config.clone(), None, None); + node.shared_state_mut().update_current_term(1); + + assert!( + grant_or_deny(&mut node, &context, 1, 3, (5, 2)).await, + "vote in term 1" + ); + assert!( + !grant_or_deny(&mut node, &context, 2, 4, (1, 1)).await, + "candidate 4's log is behind" + ); + assert_eq!(node.current_term(), 2, "the higher term is still adopted"); + + assert!( + grant_or_deny(&mut node, &context, 2, 5, (5, 2)).await, + "candidate 5 must get the vote: nothing was cast in term 2" + ); +} + +/// Test: a higher-term request with a stale log is denied but its term is persisted, so the node +/// does not fall back to the old term after a restart. +#[tokio::test] +async fn test_adopted_term_survives_a_restart_even_when_the_vote_was_denied() { + let (context, saved, _graceful_tx) = vote_sequence_context(); + let mut node = FollowerState::::new(1, context.node_config.clone(), None, None); + node.shared_state_mut().update_current_term(1); + + assert!( + !grant_or_deny(&mut node, &context, 4, 9, (1, 1)).await, + "stale log, denied" + ); + + let persisted = *saved.lock().unwrap().last().expect("the term change must be persisted"); + assert_eq!((persisted.current_term, persisted.voted_for), (4, None)); + let restarted = + FollowerState::::new(1, context.node_config.clone(), Some(persisted), None); + assert_eq!(restarted.current_term(), 4); +} diff --git a/d-engine-core/src/raft_test/process_inbound_events_tests.rs b/d-engine-core/src/raft_test/process_inbound_events_tests.rs index 3dbde10e..e14d04a7 100644 --- a/d-engine-core/src/raft_test/process_inbound_events_tests.rs +++ b/d-engine-core/src/raft_test/process_inbound_events_tests.rs @@ -28,6 +28,15 @@ use tokio::sync::watch; // Helpers // ----------------------------------------------------------------------- +/// A follower ignores VoteRequests for `election_timeout_min` after hearing from a leader. +/// Tests that interleave AEs with VoteRequests only care about how AE runs are merged +/// around the VoteRequest, not about voting, so they turn that guard off with a zero window. +fn config_without_leader_contact_guard() -> crate::RaftNodeConfig { + let mut node_config = crate::RaftNodeConfig::default(); + node_config.raft.election.election_timeout_min = 0; + node_config +} + fn make_request( term: u64, prev_log_index: u64, @@ -159,6 +168,8 @@ async fn test_ae_runs_on_both_sides_of_non_ae_are_each_merged() { .returning(|_, _, _| Ok(ok_append_response())); let mut raft = MockBuilder::new(graceful_rx) + .with_node_config(config_without_leader_contact_guard()) + .turn_on_election(false) .with_election_handler(election_handler) .with_replication_handler(replication_handler) .build_raft(); @@ -240,6 +251,8 @@ async fn test_isolated_ae_between_non_ae_dispatched_alone() { .returning(|_, _, _| Ok(ok_append_response())); let mut raft = MockBuilder::new(graceful_rx) + .with_node_config(config_without_leader_contact_guard()) + .turn_on_election(false) .with_election_handler(election_handler) .with_replication_handler(replication_handler) .build_raft(); @@ -251,6 +264,7 @@ async fn test_isolated_ae_between_non_ae_dispatched_alone() { // isolated AE: entries 15-19 queue.push_back(make_ae_event(make_request(5, 14, 5, 1))); raft.buffered_inbound_event = queue; + assert_eq!(raft.ctx.node_config().raft.election.election_timeout_min, 0); raft.process_inbound_events().await.unwrap(); } diff --git a/d-engine-core/src/raft_test/raft_comprehensive_tests.rs b/d-engine-core/src/raft_test/raft_comprehensive_tests.rs index ff1962dd..e10251fd 100644 --- a/d-engine-core/src/raft_test/raft_comprehensive_tests.rs +++ b/d-engine-core/src/raft_test/raft_comprehensive_tests.rs @@ -451,28 +451,48 @@ async fn test_follower_become_without_leader() { assert!(is_follower(raft.role.as_i32())); } -/// Test: Follower state reset: voted_for should be cleared +/// Test: stepping down inside the same term keeps this term's vote. /// -/// Verifies that when transitioning to follower, voted_for is reset. -/// This prevents voting for multiple candidates in same term. +/// `votedFor` is per term (Raft Figure 2): a node that voted in term T must not vote again in +/// term T, so a same-term step-down (CheckQuorum, noop timeout, removal) must not clear it. +/// A term increase already clears it in `set_hard_state`, before `BecomeFollower` is sent. /// See A2.3 in test scenarios. #[tokio::test] -async fn test_follower_state_reset_voted_for() { +async fn test_become_follower_keeps_the_vote_of_the_same_term() { + use d_engine_proto::server::election::VotedFor; + let (_graceful_tx, graceful_rx) = watch::channel(()); let mut raft = MockBuilder::new(graceful_rx).build_raft(); - // Follower → Candidate (sets voted_for to self) raft.handle_internal_event(InternalEvent::BecomeCandidate) .await .expect("Should become Candidate"); - assert!(is_candidate(raft.role.as_i32())); + let vote = VotedFor { + voted_for_id: raft.ctx.node_id, + voted_for_term: raft.role.current_term(), + committed: true, + }; + raft.role + .state_mut() + .commit_hard_state(&raft.ctx, None, Some(vote)) + .expect("the candidate votes for itself"); + let term = raft.role.current_term(); - // Candidate → Follower (should clear voted_for) raft.handle_internal_event(InternalEvent::BecomeFollower(None)) .await .expect("Should transition to Follower"); + assert!(is_follower(raft.role.as_i32())); - // State has been reset, voted_for should be cleared + assert_eq!( + raft.role.current_term(), + term, + "a same-term step-down keeps the term" + ); + assert_eq!( + raft.role.state().voted_for().unwrap(), + Some(vote), + "the vote of this term must survive the step-down, or the node could vote twice in it" + ); } // A3. Candidate Role Entry/Exit Tests @@ -2695,6 +2715,46 @@ async fn test_graceful_shutdown_persists_hardstate() { ); } +/// Test: a FatalError sent through `internal_event_tx` makes `run()` return Err(Fatal). +/// +/// This is the last hop of the storage-failure path: `RaftLogCore` poisons itself and sends +/// `InternalEvent::FatalError { source: "RaftLog", .. }`; if `run()` kept looping instead of +/// returning, the node would keep serving with a log that rejects every write. +/// `process_internal_events` is already covered on its own; this drives the real select loop. +/// +/// # Scenario +/// - `run()` is started on a healthy node. +/// - A `FatalError` from "RaftLog" is sent through the same channel `RaftLogCore` uses. +/// - Expected: `run()` returns `Err` with `is_fatal()`, carrying source and error text. +#[tokio::test] +async fn test_run_exits_with_fatal_error_when_raft_log_reports_fatal() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let mut raft = MockBuilder::new(graceful_rx).build_raft(); + let internal_event_tx = raft.internal_event_tx.clone(); + + let raft_handle = tokio::spawn(async move { raft.run().await }); + + internal_event_tx + .send(InternalEvent::FatalError { + source: "RaftLog".to_string(), + error: "persist_entries failed".to_string(), + }) + .expect("internal event channel must be open while run() is alive"); + + let result = tokio::time::timeout(std::time::Duration::from_secs(2), raft_handle) + .await + .expect("run() must exit after FatalError, not keep looping") + .expect("run() task must not panic"); + + let err = result.expect_err("run() must return Err on FatalError, not Ok"); + assert!(err.is_fatal()); + let msg = err.to_string(); + assert!( + msg.contains("RaftLog") && msg.contains("persist_entries failed"), + "error must carry source and cause, got: {msg}" + ); +} + /// Test: a node that loses leadership and is elected again must not carry any per-peer /// replication state from its previous term. Both the trust state and the in-flight /// bookkeeping belong to one leadership; reusing them would let stale in-flight slots gate @@ -2739,3 +2799,26 @@ async fn test_re_elected_leader_starts_with_fresh_peer_state() { "a new leadership must start every peer from the default Probe, not the previous term's state" ); } + +/// Test: after `BecomeCandidate` is processed, the new Candidate is due at once. +/// +/// The main loop reads `role.next_deadline()` after every event; a Candidate whose deadline is +/// already due makes the very next `select!` run `tick`, so the first election round starts +/// without a second election timeout. +#[tokio::test(start_paused = true)] +async fn test_become_candidate_leaves_the_new_candidate_due_to_campaign() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let mut raft = MockBuilder::new(graceful_rx).turn_on_election(false).build_raft(); + assert!( + !raft.role.is_timer_expired(), + "precondition: a fresh Follower is not due" + ); + + raft.handle_internal_event(InternalEvent::BecomeCandidate).await.unwrap(); + + assert!(is_candidate(raft.role.as_i32())); + assert!( + raft.role.is_timer_expired(), + "the new Candidate must be due to start its first round immediately" + ); +} diff --git a/d-engine-core/src/storage/raft_log_core.rs b/d-engine-core/src/storage/raft_log_core.rs index 5bae1266..97c419bf 100644 --- a/d-engine-core/src/storage/raft_log_core.rs +++ b/d-engine-core/src/storage/raft_log_core.rs @@ -412,7 +412,7 @@ where ); // Verify commit conditions - if majority_index < commit_index { + if majority_index <= commit_index { return None; } @@ -509,10 +509,13 @@ where if self.is_poisoned() { return Err(Error::Fatal("raft log storage is poisoned".into())); } - self.meta_store.save_hard_state(hard_state).inspect_err(|e| { - error!(?self.node_id, "save_hard_state failed (fatal): {e:?}"); - self.mark_poisoned_and_notify(format!("save_hard_state failed: {e:?}")); - }) + self.meta_store + .save_hard_state(hard_state) + .and_then(|()| self.meta_store.flush()) + .inspect_err(|e| { + error!(?self.node_id, "save_hard_state failed (fatal): {e:?}"); + self.mark_poisoned_and_notify(format!("save_hard_state failed: {e:?}")); + }) } async fn close(&self) { diff --git a/d-engine-core/src/storage/raft_log_core_test/drain_fsync_test.rs b/d-engine-core/src/storage/raft_log_core_test/drain_fsync_test.rs index dcf995e5..3233a445 100644 --- a/d-engine-core/src/storage/raft_log_core_test/drain_fsync_test.rs +++ b/d-engine-core/src/storage/raft_log_core_test/drain_fsync_test.rs @@ -1001,3 +1001,116 @@ async fn test_cold_start_persist_frontier_starts_past_durable_index() { re-scan already-durable entry 5. Got: {calls:?}" ); } + +/// `save_hard_state()` must make term and vote durable BEFORE it returns. +/// +/// Raft requires currentTerm and votedFor on stable storage before a node answers a vote +/// request: a node that grants a vote, crashes and forgets it can vote again in the same term, +/// and two candidates can each collect a majority (two leaders in one term). +/// +/// The RocksDB engine opens with `manual_wal_flush`, so a plain `put` stays in the WAL buffer +/// until someone calls `flush_wal`; the meta store's `flush()` is that barrier. A vote answered +/// between two log fsyncs, or while the node is idle (no log writes at all), is not durable +/// without it. +/// +/// Expected: +/// - The meta store is flushed after the hard state is saved, before the call returns. +#[tokio::test] +async fn test_save_hard_state_flushes_meta_store_before_returning() { + let events = Arc::new(Mutex::new(Vec::<&'static str>::new())); + + let mut log_store = MockLogStore::new(); + log_store.expect_last_index().returning(|| 0); + log_store.expect_persist_entries().returning(|_| Ok(())); + log_store.expect_replace_range().returning(|from, entries| { + Ok(entries.last().map(|e| e.index).unwrap_or(from.saturating_sub(1))) + }); + log_store.expect_truncate().returning(|_| Ok(())); + log_store.expect_entry().returning(|_| Ok(None)); + log_store.expect_get_entries().returning(|_| Ok(vec![])); + log_store.expect_purge().returning(|_| Ok(())); + log_store.expect_load_purge_boundary().returning(|| Ok(None)); + log_store.expect_reset().returning(|| Ok(())); + log_store.expect_is_write_durable().returning(|| false); + log_store.expect_flush().returning(|| Ok(())); + log_store.expect_flush_async().returning(|| Ok(())); + + let mut meta_store = MockMetaStore::new(); + let saved = Arc::clone(&events); + meta_store.expect_save_hard_state().returning(move |_| { + saved.lock().unwrap().push("save"); + Ok(()) + }); + let flushed = Arc::clone(&events); + meta_store.expect_flush().returning(move || { + flushed.lock().unwrap().push("flush"); + Ok(()) + }); + meta_store.expect_flush_async().returning(|| Ok(())); + meta_store.expect_load_hard_state().returning(|| Ok(None)); + + let storage = Arc::new(MockStorageEngine::from(log_store, meta_store)); + let raft_log = RaftLogCore::::new(1, storage, None, 5000); + + raft_log + .save_hard_state(&HardState { + current_term: 5, + voted_for: Some(d_engine_proto::server::election::VotedFor { + voted_for_id: 2, + voted_for_term: 5, + committed: false, + }), + }) + .unwrap(); + + assert_eq!( + *events.lock().unwrap(), + vec!["save", "flush"], + "the vote must be flushed to stable storage before save_hard_state returns" + ); +} + +/// A failed flush of the hard state is fatal, exactly like a failed write of it. +/// +/// If the fsync that makes a vote durable fails, the node must not answer the vote request as if +/// it had been recorded: it could crash, forget the vote, and vote again in the same term. +/// +/// Expected: +/// - `save_hard_state()` returns an error and the log is poisoned (the Raft loop is told to exit). +#[tokio::test] +async fn test_save_hard_state_flush_failure_poisons() { + let mut log_store = MockLogStore::new(); + log_store.expect_last_index().returning(|| 0); + log_store.expect_persist_entries().returning(|_| Ok(())); + log_store.expect_entry().returning(|_| Ok(None)); + log_store.expect_get_entries().returning(|_| Ok(vec![])); + log_store.expect_load_purge_boundary().returning(|| Ok(None)); + log_store.expect_is_write_durable().returning(|| false); + log_store.expect_flush().returning(|| Ok(())); + log_store.expect_flush_async().returning(|| Ok(())); + + let mut meta_store = MockMetaStore::new(); + meta_store.expect_save_hard_state().returning(|_| Ok(())); + meta_store.expect_load_hard_state().returning(|| Ok(None)); + meta_store + .expect_flush() + .returning(|| Err(crate::StorageError::DbError("fsync failed".to_string()).into())); + meta_store.expect_flush_async().returning(|| Ok(())); + + let storage = Arc::new(MockStorageEngine::from(log_store, meta_store)); + let raft_log = RaftLogCore::::new(1, storage, None, 5000); + + let result = raft_log.save_hard_state(&HardState { + current_term: 5, + voted_for: None, + }); + + assert!( + result.is_err(), + "a failed flush of the vote must surface as an error" + ); + assert!( + raft_log.is_poisoned(), + "a failed flush of the vote must poison the log" + ); +} diff --git a/d-engine-core/src/storage/raft_log_core_test/quorum_durability_test.rs b/d-engine-core/src/storage/raft_log_core_test/quorum_durability_test.rs index 328830b0..337f6f21 100644 --- a/d-engine-core/src/storage/raft_log_core_test/quorum_durability_test.rs +++ b/d-engine-core/src/storage/raft_log_core_test/quorum_durability_test.rs @@ -151,11 +151,13 @@ async fn test_majority_matched_index_requires_actual_majority_of_reports() { // 1 of 4 followers reports index 10; the other 3 are still at their last-known // value, 9. - let result = ctx.raft_log.calculate_majority_matched_index(1, 9, vec![10, 9, 9, 9]); + // commit_index is 8, so a median of 9 is a real advance (a median equal to the commit + // index returns None without reading the log). + let result = ctx.raft_log.calculate_majority_matched_index(1, 8, vec![10, 9, 9, 9]); // peer_matched_ids after leader's own contribution = [10, 9, 9, 9, 10] // sorted desc = [10,10,9,9,9], median(len/2=2) = 9 — majority stays at 9, entry(9) - // exists with term=1=current_term, so the result is the previously-safe Some(9), not 10. + // exists with term=1=current_term, so the result is Some(9), not 10. assert_eq!( result, Some(9), diff --git a/d-engine-core/src/storage/raft_log_core_test/raft_properties_test.rs b/d-engine-core/src/storage/raft_log_core_test/raft_properties_test.rs index bc4de169..13515bde 100644 --- a/d-engine-core/src/storage/raft_log_core_test/raft_properties_test.rs +++ b/d-engine-core/src/storage/raft_log_core_test/raft_properties_test.rs @@ -185,3 +185,56 @@ async fn test_calculate_majority_matched_index_case5() { .calculate_majority_matched_index(term, commit, vec![peer1_match, peer2_match]) ); } + +/// `calculate_majority_matched_index` answers "which index has a majority replicated" (the COMMIT +/// question). When that index is not beyond `commit_index` there is nothing new to commit, so it +/// returns `None` before reading the log. +/// +/// A quiet cluster (every `match_index` equals `commit_index`) is the common case: each heartbeat +/// reply used to cost one log read whose result the caller threw away +/// (`new_commit_index` only accepts a strictly larger index). +/// +/// Whether a quorum just answered is NOT derived from this result (see +/// `test_one_reachable_peer_of_four_does_not_confirm_quorum` in the leader tests). +#[tokio::test] +async fn test_majority_matched_index_is_none_when_it_does_not_pass_the_commit_index() { + let mut ctx = RaftLogCoreTestContext::new("test_majority_matched_not_past_commit"); + + // Quiet cluster: the new leader's noop (index 50, term 1) is committed everywhere. + ctx.append_entries(1, 50, 1).await; + ctx.raft_log.flush().await.unwrap(); + ctx.drain_fsync_completions(); + + // 5 voters, every match_index == commit_index == 50 (including peers nobody heard from lately). + assert_eq!( + ctx.raft_log.calculate_majority_matched_index(1, 50, vec![50, 50, 50, 50]), + None, + "median == commit_index: nothing new to commit" + ); + // A majority below the commit index (possible after a membership change) is not an advance either. + assert_eq!( + ctx.raft_log.calculate_majority_matched_index(1, 50, vec![50, 40, 40, 40]), + None + ); +} + +/// The majority index is returned when it passes the commit index and belongs to the current term. +#[tokio::test] +async fn test_majority_matched_index_is_some_when_the_majority_passes_the_commit_index() { + let mut ctx = RaftLogCoreTestContext::new("test_majority_matched_past_commit"); + + ctx.append_entries(1, 50, 1).await; + ctx.raft_log.flush().await.unwrap(); + ctx.drain_fsync_completions(); + + // 5 voters (leader durable 50), commit_index 40, median 50, entry 50 is from term 1. + assert_eq!( + ctx.raft_log.calculate_majority_matched_index(1, 40, vec![50, 50, 50, 50]), + Some(50) + ); + // The same median in a later term is not committed by counting replicas (Raft 5.4.2). + assert_eq!( + ctx.raft_log.calculate_majority_matched_index(2, 40, vec![50, 50, 50, 50]), + None + ); +} 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 4e932800..6da00be4 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 @@ -367,6 +367,9 @@ pub fn mock_transport() -> MockTransport { pub fn mock_election_core() -> MockElectionCore { let mut election_handler = MockElectionCore::new(); + election_handler + .expect_broadcast_pre_vote_requests() + .returning(|_, _, _, _, _| Ok(())); election_handler .expect_broadcast_vote_requests() .returning(|_, _, _, _, _| Ok(())); @@ -465,6 +468,17 @@ pub fn mock_membership() -> MockMembership { // Mock single-node cluster detection (default to multi-node with no peers) membership.expect_is_single_node_cluster().returning(|| false); membership.expect_initial_cluster_size().returning(|| 3); + + // Every node in a test is an active voter unless the test says otherwise: only an active + // voter may start an election. + membership.expect_retrieve_node_meta().returning(|id| { + Some(d_engine_proto::server::cluster::NodeMeta { + id, + address: format!("http://127.0.0.1:{}", 55000 + id), + role: d_engine_proto::common::NodeRole::Follower.into(), + status: d_engine_proto::common::NodeStatus::Active.into(), + }) + }); membership } diff --git a/d-engine-core/src/test_utils/mock/mock_rpc.rs b/d-engine-core/src/test_utils/mock/mock_rpc.rs index 9d21cc5f..2f497bb7 100644 --- a/d-engine-core/src/test_utils/mock/mock_rpc.rs +++ b/d-engine-core/src/test_utils/mock/mock_rpc.rs @@ -79,6 +79,14 @@ impl RaftElectionService for MockRpcService { None => Err(tonic::Status::unknown("No mock vote response set")), } } + + /// Answers PreVote with the same canned response as `request_vote`. + async fn pre_vote( + &self, + request: tonic::Request, + ) -> std::result::Result, tonic::Status> { + self.request_vote(request).await + } } #[tonic::async_trait] diff --git a/d-engine-core/src/timer/election_timer.rs b/d-engine-core/src/timer/election_timer.rs index 7af70df5..db05fe99 100644 --- a/d-engine-core/src/timer/election_timer.rs +++ b/d-engine-core/src/timer/election_timer.rs @@ -18,6 +18,16 @@ impl ElectionTimer { } } + /// A timer that is already due. For a node that must act at once, e.g. a Follower whose + /// election timeout just fired and which becomes a Candidate: its first election round must + /// not wait for a second timeout. + pub fn expired(timeout_range: (u64, u64)) -> Self { + Self { + next_deadline: Instant::now(), + timeout_range, + } + } + pub fn reset(&mut self) { let (min, max) = self.timeout_range; self.next_deadline = Instant::now() + Self::random_duration(min, max); diff --git a/d-engine-core/src/timer/timer_test.rs b/d-engine-core/src/timer/timer_test.rs index 6471ea08..9fe24b46 100644 --- a/d-engine-core/src/timer/timer_test.rs +++ b/d-engine-core/src/timer/timer_test.rs @@ -279,3 +279,30 @@ async fn test_replication_timer_is_expired() { assert!(timer.is_expired(), "Timer should be expired after timeout"); } + +/// Test: `ElectionTimer::expired` is due at once and re-arms to a normal future timeout. +/// +/// Scenario: +/// - A node that must act immediately (a Follower whose election timeout just fired becomes a +/// Candidate) starts with an expired timer; after its first round it calls `reset`. +/// +/// Expected: +/// - Expired right away; after `reset` not expired, with the deadline inside [min, max) from now. +#[tokio::test(start_paused = true)] +async fn test_election_timer_expired_is_due_now_and_reset_rearms_it() { + let mut timer = ElectionTimer::expired((100, 200)); + + assert!( + timer.is_expired(), + "an expired timer must be due immediately" + ); + + timer.reset(); + + assert!(!timer.is_expired(), "reset must re-arm the timer"); + let remaining = timer.next_deadline() - Instant::now(); + assert!( + remaining >= Duration::from_millis(100) && remaining < Duration::from_millis(200), + "re-armed deadline must lie in [min, max), got {remaining:?}" + ); +} diff --git a/d-engine-proto/proto/server/election.proto b/d-engine-proto/proto/server/election.proto index d2dab0f8..5b44739b 100644 --- a/d-engine-proto/proto/server/election.proto +++ b/d-engine-proto/proto/server/election.proto @@ -25,4 +25,7 @@ message VotedFor { service RaftElectionService { rpc RequestVote(VoteRequest) returns (VoteResponse); + + // Asks "would you vote for me at term+1?". Changes no state on the receiver. + rpc PreVote(VoteRequest) returns (VoteResponse); } diff --git a/d-engine-proto/src/generated/d_engine.server.election.rs b/d-engine-proto/src/generated/d_engine.server.election.rs index 402ed50e..5492bead 100644 --- a/d-engine-proto/src/generated/d_engine.server.election.rs +++ b/d-engine-proto/src/generated/d_engine.server.election.rs @@ -150,6 +150,33 @@ pub mod raft_election_service_client { ); self.inner.unary(req, path, codec).await } + /// Asks "would you vote for me at term+1?". Changes no state on the receiver. + pub async fn pre_vote( + &mut self, + request: impl tonic::IntoRequest, + ) -> std::result::Result, tonic::Status> { + self.inner + .ready() + .await + .map_err(|e| { + tonic::Status::unknown( + format!("Service was not ready: {}", e.into()), + ) + })?; + let codec = tonic::codec::ProstCodec::default(); + let path = http::uri::PathAndQuery::from_static( + "/d_engine.server.election.RaftElectionService/PreVote", + ); + let mut req = request.into_request(); + req.extensions_mut() + .insert( + GrpcMethod::new( + "d_engine.server.election.RaftElectionService", + "PreVote", + ), + ); + self.inner.unary(req, path, codec).await + } } } /// Generated server implementations. @@ -169,6 +196,11 @@ pub mod raft_election_service_server { &self, request: tonic::Request, ) -> std::result::Result, tonic::Status>; + /// Asks "would you vote for me at term+1?". Changes no state on the receiver. + async fn pre_vote( + &self, + request: tonic::Request, + ) -> std::result::Result, tonic::Status>; } #[derive(Debug)] pub struct RaftElectionServiceServer { @@ -292,6 +324,50 @@ pub mod raft_election_service_server { }; Box::pin(fut) } + "/d_engine.server.election.RaftElectionService/PreVote" => { + #[allow(non_camel_case_types)] + struct PreVoteSvc(pub Arc); + impl< + T: RaftElectionService, + > tonic::server::UnaryService for PreVoteSvc { + type Response = super::VoteResponse; + type Future = BoxFuture< + tonic::Response, + tonic::Status, + >; + fn call( + &mut self, + request: tonic::Request, + ) -> Self::Future { + let inner = Arc::clone(&self.0); + let fut = async move { + ::pre_vote(&inner, request).await + }; + Box::pin(fut) + } + } + let accept_compression_encodings = self.accept_compression_encodings; + let send_compression_encodings = self.send_compression_encodings; + let max_decoding_message_size = self.max_decoding_message_size; + let max_encoding_message_size = self.max_encoding_message_size; + let inner = self.inner.clone(); + let fut = async move { + let method = PreVoteSvc(inner); + let codec = tonic::codec::ProstCodec::default(); + let mut grpc = tonic::server::Grpc::new(codec) + .apply_compression_config( + accept_compression_encodings, + send_compression_encodings, + ) + .apply_max_message_size_config( + max_decoding_message_size, + max_encoding_message_size, + ); + let res = grpc.unary(method, req).await; + Ok(res) + }; + Box::pin(fut) + } _ => { Box::pin(async move { let mut response = http::Response::new( diff --git a/d-engine-server/src/network/grpc/grpc_raft_service.rs b/d-engine-server/src/network/grpc/grpc_raft_service.rs index 9993f4dc..57b3cc31 100644 --- a/d-engine-server/src/network/grpc/grpc_raft_service.rs +++ b/d-engine-server/src/network/grpc/grpc_raft_service.rs @@ -61,6 +61,32 @@ impl RaftElectionService for Node where T: TypeConfig, { + /// Handles PreVote RPCs: answers "would you vote for me?" without changing any state. + async fn pre_vote( + &self, + request: tonic::Request, + ) -> std::result::Result, Status> { + if !self.is_rpc_ready() { + debug!( + "[rpc|pre_vote] My raft setup(Node:{}) is not ready!", + self.node_id + ); + return Err(Status::unavailable("Service is not ready")); + } + + let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); + self.event_tx + .send(InboundEvent::ReceivePreVoteRequest( + request.into_inner(), + resp_tx, + )) + .await + .map_err(|_| Status::internal("Event channel closed"))?; + let timeout_duration = + Duration::from_millis(self.node_config.raft.election.election_timeout_min); + handle_rpc_timeout(resp_rx, timeout_duration, "pre_vote").await + } + /// Handles RequestVote RPC calls from candidate nodes during leader elections /// # Raft Protocol Logic /// - Part of leader election mechanism (Section 5.2) diff --git a/d-engine-server/src/network/grpc/grpc_raft_service_test.rs b/d-engine-server/src/network/grpc/grpc_raft_service_test.rs index cc5fa4c5..e79511aa 100644 --- a/d-engine-server/src/network/grpc/grpc_raft_service_test.rs +++ b/d-engine-server/src/network/grpc/grpc_raft_service_test.rs @@ -194,6 +194,15 @@ async fn test_handle_rpc_services_successfully() { settings.raft.general_raft_timeout_duration_in_ms = 200; settings.raft.batching.max_batch_size = 1; let mut membership = MockMembership::::new(); + // Every node in a test is an active voter: only an active voter may start an election. + membership.expect_retrieve_node_meta().returning(|id| { + Some(d_engine_proto::server::cluster::NodeMeta { + id, + address: format!("http://127.0.0.1:{}", 55000 + id), + role: d_engine_proto::common::NodeRole::Follower.into(), + status: d_engine_proto::common::NodeStatus::Active.into(), + }) + }); membership.expect_voters().returning(Vec::new); membership.expect_members().returning(Vec::new); @@ -247,6 +256,10 @@ async fn test_handle_rpc_services_successfully() { }, ); let mut election_handler = MockElectionCore::::new(); + // A candidate runs PreVote before the real election; this test is about the RPC services. + election_handler + .expect_broadcast_pre_vote_requests() + .returning(|_, _, _, _, _| Ok(())); election_handler .expect_broadcast_vote_requests() .returning(|_, _, _, _, _| Ok(())); @@ -680,6 +693,15 @@ async fn test_stream_append_entries_does_not_block_ready_response_behind_pending settings.raft.batching.max_batch_size = 1; let mut membership = MockMembership::::new(); + // Every node in a test is an active voter: only an active voter may start an election. + membership.expect_retrieve_node_meta().returning(|id| { + Some(d_engine_proto::server::cluster::NodeMeta { + id, + address: format!("http://127.0.0.1:{}", 55000 + id), + role: d_engine_proto::common::NodeRole::Follower.into(), + status: d_engine_proto::common::NodeStatus::Active.into(), + }) + }); membership.expect_voters().returning(Vec::new); membership.expect_members().returning(Vec::new); membership.expect_replication_peers().returning(Vec::new); @@ -723,6 +745,9 @@ async fn test_stream_append_entries_does_not_block_ready_response_behind_pending .with_membership(membership) .with_replication_handler(replication_handler) .with_node_config(settings) + // The test plays a leader at term 1 against a follower; an immediate first election would + // make this node a candidate at term 2 and the leader's term-1 requests stale. + .turn_on_election(false) .build_node(); node.set_rpc_ready(true); @@ -799,6 +824,15 @@ async fn test_stream_append_entries_closes_response_stream_after_inbound_ends() settings.raft.batching.max_batch_size = 1; let mut membership = MockMembership::::new(); + // Every node in a test is an active voter: only an active voter may start an election. + membership.expect_retrieve_node_meta().returning(|id| { + Some(d_engine_proto::server::cluster::NodeMeta { + id, + address: format!("http://127.0.0.1:{}", 55000 + id), + role: d_engine_proto::common::NodeRole::Follower.into(), + status: d_engine_proto::common::NodeStatus::Active.into(), + }) + }); membership.expect_voters().returning(Vec::new); membership.expect_members().returning(Vec::new); membership.expect_replication_peers().returning(Vec::new); @@ -813,6 +847,10 @@ async fn test_stream_append_entries_closes_response_stream_after_inbound_ends() raft_log.expect_durable_index().returning(|| 5); let mut replication_handler = MockReplicationCore::::new(); + // The first election now starts at once, so a single-node cluster becomes leader and proposes its noop. + replication_handler + .expect_prepare_batch_requests() + .returning(|_, _, _, _, _, _| Ok(d_engine_core::PrepareResult::default())); replication_handler .expect_check_append_entries_request_is_legal() .returning(|my_term, _, _| AppendEntriesResponse::success(1, my_term, None)); @@ -873,3 +911,98 @@ async fn test_stream_append_entries_closes_response_stream_after_inbound_ends() "no further items expected after the only response" ); } + +// --------------------------------------------------------------------------- +// pre_vote (#423): the RPC must reach the Raft loop as a PreVote event, never as a real vote, and +// answer through the same ready / timeout rules as request_vote. +// --------------------------------------------------------------------------- + +fn pre_vote_request() -> VoteRequest { + VoteRequest { + term: 7, + candidate_id: 3, + last_log_index: 11, + last_log_term: 5, + } +} + +/// Builds a node whose inbound events can be observed, with the RPC layer ready. +fn node_with_observable_events( + path: &str +) -> ( + crate::Node, + mpsc::Receiver, + watch::Sender<()>, +) { + let (graceful_tx, graceful_rx) = watch::channel(()); + let (event_tx, event_rx) = mpsc::channel(10); + let mut builder = MockBuilder::new(graceful_rx).with_db_path(path); + builder.event_tx = Some(event_tx); + let node = builder.build_node(); + node.set_rpc_ready(true); + (node, event_rx, graceful_tx) +} + +/// # Case: the server is not ready -> Unavailable, like request_vote +#[tokio::test] +#[traced_test] +async fn test_pre_vote_server_not_ready_returns_unavailable() { + let (_graceful_tx, graceful_rx) = watch::channel(()); + let node = mock_node("/tmp/test_pre_vote_not_ready", graceful_rx, None); + node.set_rpc_ready(false); + + let result = node.pre_vote(Request::new(pre_vote_request())).await; + + assert_eq!(result.err().unwrap().code(), Code::Unavailable); +} + +/// # Case: the RPC becomes a `ReceivePreVoteRequest` event (not `ReceiveVoteRequest`), carries the +/// request unchanged, and the reply sent on the event's channel is returned to the caller. +#[tokio::test] +#[traced_test] +async fn test_pre_vote_is_forwarded_as_pre_vote_event_and_reply_is_returned() { + let (node, mut event_rx, _graceful_tx) = + node_with_observable_events("/tmp/test_pre_vote_forwarded"); + + let call = tokio::spawn(async move { node.pre_vote(Request::new(pre_vote_request())).await }); + + match event_rx.recv().await.expect("an event must reach the Raft loop") { + d_engine_core::InboundEvent::ReceivePreVoteRequest(request, reply) => { + assert_eq!(request, pre_vote_request()); + reply + .send(Ok(d_engine_proto::server::election::VoteResponse { + term: 6, + vote_granted: true, + last_log_index: 11, + last_log_term: 5, + })) + .unwrap(); + } + other => panic!("PreVote must be a ReceivePreVoteRequest event, got: {other:?}"), + } + + let response = call.await.unwrap().expect("the reply must come back").into_inner(); + assert!(response.vote_granted); + assert_eq!(response.term, 6); +} + +/// # Case: nobody answers -> the RPC gives up after `election_timeout_min`, it does not hang the +/// peer's candidate. +#[tokio::test(start_paused = true)] +#[traced_test] +async fn test_pre_vote_unanswered_request_times_out() { + let (node, mut event_rx, _graceful_tx) = + node_with_observable_events("/tmp/test_pre_vote_timeout"); + let timeout_ms = node.node_config.raft.election.election_timeout_min; + + let call = tokio::spawn(async move { node.pre_vote(Request::new(pre_vote_request())).await }); + // Keep the event (and its reply sender) alive so the failure is a timeout, not a closed channel. + let _unanswered = event_rx.recv().await.expect("event reaches the loop"); + + tokio::time::advance(Duration::from_millis(timeout_ms + 1)).await; + + assert!( + call.await.unwrap().is_err(), + "an unanswered PreVote must time out" + ); +} diff --git a/d-engine-server/src/network/grpc/grpc_transport.rs b/d-engine-server/src/network/grpc/grpc_transport.rs index a633b37f..2d7fa341 100644 --- a/d-engine-server/src/network/grpc/grpc_transport.rs +++ b/d-engine-server/src/network/grpc/grpc_transport.rs @@ -164,6 +164,51 @@ where }) } + async fn send_pre_vote_request( + &self, + peer_id: u32, + request: VoteRequest, + retry: &RetryPolicies, + membership: Arc>, + ) -> Result { + // Real-time connection fetch for control operations (same pattern as join_cluster). + let channel = membership + .get_peer_channel(peer_id, ConnectionType::Control) + .await + .ok_or(NetworkError::PeerConnectionNotFound(peer_id))?; + + let req_clone = request; + let closure = move || { + let channel = channel.clone(); + let mut client = RaftElectionServiceClient::new(channel) + .send_compressed(CompressionEncoding::Gzip) + .accept_compressed(CompressionEncoding::Gzip); + async move { client.pre_vote(tonic::Request::new(req_clone)).await } + }; + let policy = retry.election; + let my_id = self.my_id; + match grpc_task_with_timeout_and_exponential_backoff("pre_vote", closure, policy).await { + Ok(response) => { + let res = response.into_inner(); + debug!( + "[send_pre_vote_request | {my_id}->{peer_id}] pre_vote response: {:?}", + res + ); + Ok(res) + } + Err(e) => { + // Debug, not error: a single peer RPC failure here is routinely + // caused by the peer not being ready yet (e.g. cluster bootstrap + // race) and resolves itself on the next election round (#428). + debug!( + "[send_pre_vote_request | {my_id}->{peer_id}] Received RPC error: {}", + e + ); + Err(e) + } + } + } + async fn send_vote_request( &self, peer_id: u32, diff --git a/d-engine-server/src/network/grpc/grpc_transport_test.rs b/d-engine-server/src/network/grpc/grpc_transport_test.rs index 51d1c0c7..6e177b38 100644 --- a/d-engine-server/src/network/grpc/grpc_transport_test.rs +++ b/d-engine-server/src/network/grpc/grpc_transport_test.rs @@ -522,3 +522,219 @@ async fn test_send_vote_request_retry_exhausted_returns_err() { assert!(result.is_err()); } + +// --------------------------------------------------------------------------- +// send_pre_vote_request (#423): like send_vote_request it is a transparent per-peer primitive, +// but it MUST use the PreVote RPC. Sending a PreVote as RequestVote would make the peer record a +// real vote and adopt the term, defeating the whole point of PreVote. +// --------------------------------------------------------------------------- + +/// Sends one PreVote to a mock peer that answers `response`, and returns the result. +async fn send_pre_vote_to_mock_peer( + name: &str, + response: VoteResponse, +) -> d_engine_core::Result { + let my_id = 1; + let peer_id = 2; + let node_config = node_config(name); + + let (_tx, rx) = oneshot::channel::<()>(); + let (channel, _port) = MockNode::simulate_send_votes_mock_server(response, rx) + .await + .expect("mock server should start"); + + let mut channels = HashMap::new(); + channels.insert((peer_id, ConnectionType::Control), channel); + let membership = mock_membership(vec![(peer_id, Follower as i32)], channels); + + let request = VoteRequest { + term: 2, + candidate_id: my_id, + last_log_index: 1, + last_log_term: 1, + }; + let client: GrpcTransport = GrpcTransport::new(my_id); + client + .send_pre_vote_request(peer_id, request, &node_config.retry, membership) + .await +} + +// # Case: PreVote granted / denied are both Ok responses (a denial is a protocol answer, not a +// transport failure). +#[tokio::test] +#[traced_test] +async fn test_send_pre_vote_request_returns_granted_and_denied_responses_as_ok() { + let granted = send_pre_vote_to_mock_peer( + "/tmp/test_send_pre_vote_granted", + VoteResponse { + term: 1, + vote_granted: true, + last_log_index: 1, + last_log_term: 1, + }, + ) + .await + .expect("granted must be Ok"); + assert!(granted.vote_granted); + + let denied = send_pre_vote_to_mock_peer( + "/tmp/test_send_pre_vote_denied", + VoteResponse { + term: 1, + vote_granted: false, + last_log_index: 1, + last_log_term: 1, + }, + ) + .await + .expect("a denial must be Ok, not Err"); + assert!(!denied.vote_granted); +} + +// # Case: the peer's term and log come back unchanged. The core layer interprets them (higher +// term -> catch up, newer log -> not enough votes); the transport must not. +#[tokio::test] +#[traced_test] +async fn test_send_pre_vote_request_passes_through_term_and_log_of_the_response() { + let res = send_pre_vote_to_mock_peer( + "/tmp/test_send_pre_vote_passes_through", + VoteResponse { + term: 99, + vote_granted: false, + last_log_index: 42, + last_log_term: 5, + }, + ) + .await + .expect("should succeed"); + + assert_eq!(res.term, 99); + assert_eq!((res.last_log_index, res.last_log_term), (42, 5)); +} + +// # Case: PreVote goes out through the PreVote RPC, never through RequestVote. +// +// ## Criteria: +// 1. The peer's RequestVote handler answers with an error; the PreVote handler answers +// "granted". Getting "granted" proves the PreVote RPC was used. +#[tokio::test] +#[traced_test] +async fn test_send_pre_vote_request_uses_the_pre_vote_rpc_not_request_vote() { + let my_id = 1; + let peer_id = 2; + let node_config = node_config("/tmp/test_send_pre_vote_uses_pre_vote_rpc"); + + let (_tx, rx) = oneshot::channel::<()>(); + let mock_service = MockRpcService { + expected_vote_response: Some(Err(Status::internal( + "RequestVote must not be called for a PreVote", + ))), + expected_pre_vote_response: Some(Ok(VoteResponse { + term: 1, + vote_granted: true, + last_log_index: 1, + last_log_term: 1, + })), + ..Default::default() + }; + let (port, _addr) = MockNode::mock_listener(mock_service, rx, true) + .await + .expect("mock listener should start"); + let channel = MockNode::mock_channel_with_port(port).await; + + let mut channels = HashMap::new(); + channels.insert((peer_id, ConnectionType::Control), channel); + let membership = mock_membership(vec![(peer_id, Follower as i32)], channels); + + let client: GrpcTransport = GrpcTransport::new(my_id); + let result = client + .send_pre_vote_request( + peer_id, + VoteRequest { + term: 2, + candidate_id: my_id, + last_log_index: 1, + last_log_term: 1, + }, + &node_config.retry, + membership, + ) + .await; + + assert!( + result.expect("PreVote RPC must be used").vote_granted, + "got the RequestVote error path instead" + ); +} + +// # Case: peer channel not found -> PeerConnectionNotFound without attempting any RPC. +#[tokio::test] +#[traced_test] +async fn test_send_pre_vote_request_missing_channel_returns_peer_connection_not_found() { + let my_id = 1; + let peer_id = 2; + let node_config = node_config("/tmp/test_send_pre_vote_missing_channel"); + let membership = mock_membership(vec![(peer_id, Follower as i32)], HashMap::new()); + + let client: GrpcTransport = GrpcTransport::new(my_id); + let result = client + .send_pre_vote_request( + peer_id, + VoteRequest { + term: 2, + candidate_id: my_id, + last_log_index: 0, + last_log_term: 0, + }, + &node_config.retry, + membership, + ) + .await; + + assert!(matches!( + result.unwrap_err(), + Error::System(SystemError::Network(NetworkError::PeerConnectionNotFound(id))) if id == peer_id + )); +} + +// # Case: peer consistently unreachable -> Err once retries are exhausted (and never an Ok that +// the core layer could count as a grant). +#[tokio::test] +#[traced_test] +async fn test_send_pre_vote_request_retry_exhausted_returns_err() { + let my_id = 1; + let peer_id = 2; + let mut node_config = node_config("/tmp/test_send_pre_vote_retry_exhausted"); + node_config.retry.election.max_retries = 1; + + let (_tx, rx) = oneshot::channel::<()>(); + let mock_service = MockRpcService { + expected_pre_vote_response: Some(Err(Status::unavailable("peer unreachable"))), + ..Default::default() + }; + let (port, _addr) = MockNode::mock_listener(mock_service, rx, true) + .await + .expect("mock listener should start"); + let channel = MockNode::mock_channel_with_port(port).await; + + let mut channels = HashMap::new(); + channels.insert((peer_id, ConnectionType::Control), channel); + let membership = mock_membership(vec![(peer_id, Follower as i32)], channels); + + let client: GrpcTransport = GrpcTransport::new(my_id); + let result = client + .send_pre_vote_request( + peer_id, + VoteRequest { + term: 2, + candidate_id: my_id, + last_log_index: 0, + last_log_term: 0, + }, + &node_config.retry, + membership, + ) + .await; + + assert!(result.is_err()); +} diff --git a/d-engine-server/src/node/node_test.rs b/d-engine-server/src/node/node_test.rs index ef4432b0..427b0494 100644 --- a/d-engine-server/src/node/node_test.rs +++ b/d-engine-server/src/node/node_test.rs @@ -78,8 +78,12 @@ async fn test_run_sequence_with_mock_peers() { fn prepare_succeed_majority_confirmation() -> (MockRaftLog, MockReplicationCore) { // Initialize the mock object - let replication_handler = MockReplicationCore::::new(); + let mut replication_handler = MockReplicationCore::::new(); let mut raft_log = MockRaftLog::new(); + // The first election now starts at once, so a single-node cluster becomes leader and proposes its noop. + replication_handler + .expect_prepare_batch_requests() + .returning(|_, _, _, _, _, _| Ok(d_engine_core::PrepareResult::default())); raft_log.expect_calculate_majority_matched_index().returning(|_, _, _| Some(5)); raft_log.expect_last_entry_id().return_const(1_u64); 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 81f32e78..124e204b 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 @@ -569,6 +569,10 @@ pub(crate) fn mock_transport() -> MockTransport { /// Create a mock election handler pub(crate) fn mock_election_core() -> MockElectionCore { let mut election_handler = MockElectionCore::new(); + // A candidate runs PreVote before the real election; tests using this default are not about it. + election_handler + .expect_broadcast_pre_vote_requests() + .returning(|_, _, _, _, _| Ok(())); election_handler .expect_broadcast_vote_requests() .returning(|_, _, _, _, _| Ok(())); @@ -658,6 +662,15 @@ pub(crate) fn mock_membership() -> MockMembership { current_leader_id: None, }); membership.expect_get_peers_id_with_condition().returning(|_| vec![]); + // Every node in a test is an active voter: only an active voter may start an election. + membership.expect_retrieve_node_meta().returning(|id| { + Some(d_engine_proto::server::cluster::NodeMeta { + id, + address: format!("http://127.0.0.1:{}", 55000 + id), + role: d_engine_proto::common::NodeRole::Follower.into(), + status: d_engine_proto::common::NodeStatus::Active.into(), + }) + }); membership } diff --git a/d-engine-server/src/test_utils/mock/mock_rpc.rs b/d-engine-server/src/test_utils/mock/mock_rpc.rs index 6820c2c4..a655f23d 100644 --- a/d-engine-server/src/test_utils/mock/mock_rpc.rs +++ b/d-engine-server/src/test_utils/mock/mock_rpc.rs @@ -36,6 +36,9 @@ pub struct MockRpcService { pub server_port: Option, // Expected responses for each method pub expected_vote_response: Option>, + /// Canned PreVote answer. When unset, PreVote falls back to `expected_vote_response`. + /// Set both to different values to prove which RPC a caller actually used. + pub expected_pre_vote_response: Option>, pub expected_append_entries_response: Option>, pub expected_update_cluster_conf_response: Option>, @@ -80,6 +83,19 @@ impl RaftElectionService for MockRpcService { None => Err(tonic::Status::unknown("No mock vote response set")), } } + + /// Answers PreVote with `expected_pre_vote_response`, or the `request_vote` canned + /// response when no PreVote-specific one is set. + async fn pre_vote( + &self, + request: tonic::Request, + ) -> std::result::Result, tonic::Status> { + match &self.expected_pre_vote_response { + Some(Ok(response)) => Ok(tonic::Response::new(*response)), + Some(Err(status)) => Err(status.clone()), + None => self.request_vote(request).await, + } + } } #[tonic::async_trait] diff --git a/d-engine-server/tests/failover_and_recovery/leader_failover_embedded.rs b/d-engine-server/tests/failover_and_recovery/leader_failover_embedded.rs index baa62260..7ee7c24d 100644 --- a/d-engine-server/tests/failover_and_recovery/leader_failover_embedded.rs +++ b/d-engine-server/tests/failover_and_recovery/leader_failover_embedded.rs @@ -82,6 +82,10 @@ async fn test_embedded_leader_failover() -> Result<(), Box Result<(), Box Result<(), Box { - panic!("Write should not succeed without majority!"); - } - Ok(Err(e)) => { - info!("Write correctly rejected with error: {:?}", e); - } - Err(_) => { - info!("Write correctly timed out"); - } + Ok(Err(ClientApiError::Network { + code: ErrorCode::NotLeader, + .. + })) => info!("Write correctly rejected with NotLeader"), + other => panic!("expected an immediate NotLeader from write admission, got: {other:?}"), } + // CheckQuorum: after election_timeout_max x multiple (3 s x 2) of silence the leader steps + // down, and the client keeps seeing NotLeader (no leader to redirect to). + let step_down_deadline = tokio::time::Instant::now() + Duration::from_secs(15); + while engines[0].is_leader() { + assert!( + tokio::time::Instant::now() < step_down_deadline, + "leader without a quorum must step down within election_timeout_max x multiple" + ); + tokio::time::sleep(Duration::from_millis(100)).await; + } + let after_step_down = engines[0].client().put(b"after-step-down".to_vec(), b"x".to_vec()).await; + assert!( + matches!( + after_step_down, + Err(ClientApiError::Network { + code: ErrorCode::NotLeader, + .. + }) + ), + "a stepped-down node must answer NotLeader, got: {after_step_down:?}" + ); + info!("Minority failure test passed - cluster correctly refused writes"); // Cleanup @@ -491,3 +514,87 @@ async fn test_minority_failure_blocks_writes() -> Result<(), Box= 6) +#[tokio::test] +#[traced_test] +async fn test_isolated_node_does_not_inflate_term() -> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let db_root = temp_dir.path().join("db"); + let log_dir = temp_dir.path().join("logs"); + + let mut port_guard = get_available_ports(3).await; + port_guard.release_listeners(); + let ports = port_guard.as_slice(); + + async fn start_node( + node_id: u64, + ports: &[u16], + db_root: &std::path::Path, + log_dir: &std::path::Path, + ) -> Result> { + let config_str = create_node_config( + node_id, + ports[(node_id - 1) as usize], + ports, + db_root.to_str().unwrap(), + log_dir.to_str().unwrap(), + ) + .await; + let node_db_root = db_root.join(format!("node{node_id}")); + let db_path = node_db_root.join("db"); + tokio::fs::create_dir_all(&db_path).await?; + let (storage, state_machine) = RocksDBUnifiedEngine::open(&db_path)?; + let config_path = format!("/tmp/d-engine-test-isolated-node{node_id}.toml"); + tokio::fs::write(&config_path, &config_str).await?; + Ok(DefaultEmbeddedEngine::start_custom( + &node_db_root, + Arc::new(storage), + Arc::new(state_machine), + Some(&config_path), + ) + .await?) + } + + info!("Starting node 3 alone"); + let node3 = start_node(3, ports, &db_root, &log_dir).await?; + tokio::time::sleep(Duration::from_secs(15)).await; + + info!("Starting nodes 1 and 2"); + let node1 = start_node(1, ports, &db_root, &log_dir).await?; + let node2 = start_node(2, ports, &db_root, &log_dir).await?; + + let leader = node1.wait_ready(Duration::from_secs(30)).await?; + info!("Leader {} at term {}", leader.leader_id, leader.term); + assert!( + leader.term <= 4, + "an isolated node must not inflate the cluster's term, leader term was {}", + leader.term + ); + + node1 + .client() + .put(b"after-isolation".to_vec(), b"ok".to_vec()) + .await + .or_else(|e| match e { + // The write may land on a follower right after election: only the term matters here. + ClientApiError::Network { + code: ErrorCode::NotLeader, + .. + } => Ok(()), + other => Err(other), + })?; + + for engine in [node1, node2, node3] { + engine.stop().await?; + } + Ok(()) +} diff --git a/examples/three-nodes-standalone/docker/Dockerfile b/examples/three-nodes-standalone/docker/Dockerfile index e971495e..f38d2e0a 100644 --- a/examples/three-nodes-standalone/docker/Dockerfile +++ b/examples/three-nodes-standalone/docker/Dockerfile @@ -85,6 +85,11 @@ RUN mkdir -p /app/{logs,db,snapshots} /etc/promtail # Promtail config path relative to workspace root COPY examples/three-nodes-standalone/docker/monitoring/promtail/config.yml /etc/promtail/config.yml +# Which source this binary was built from; read by the Jepsen log scan. +ARG GIT_SHA=unknown +RUN echo "$GIT_SHA" > /etc/d-engine-commit + + WORKDIR /app CMD ["sh", "-c", "/usr/sbin/sshd -D & CONFIG_PATH=$CONFIG_PATH LOG_DIR=$LOG_DIR METRICS_PORT=$METRICS_PORT DB_PATH=/app/db/$ID RUST_LOG=demo=$LOG_LEVEL,d_engine=$LOG_LEVEL,hyper=warn,sled=warn demo & promtail --config.file=/etc/promtail/config.yml > /app/logs/promtail.log 2>&1"] diff --git a/examples/three-nodes-standalone/src/main.rs b/examples/three-nodes-standalone/src/main.rs index c61dc79e..7e86e4d2 100644 --- a/examples/three-nodes-standalone/src/main.rs +++ b/examples/three-nodes-standalone/src/main.rs @@ -158,7 +158,7 @@ async fn start_metrics_server(port: u16) { ]; const BATCH_BUCKETS: &[f64] = &[1.0, 2.0, 5.0, 10.0, 20.0, 50.0, 100.0, 200.0, 500.0]; - metrics_exporter_prometheus::PrometheusBuilder::new() + if let Err(e) = metrics_exporter_prometheus::PrometheusBuilder::new() .with_http_listener(([0, 0, 0, 0], port)) .set_buckets_for_metric( metrics_exporter_prometheus::Matcher::Suffix("_ms".to_string()), @@ -171,7 +171,10 @@ async fn start_metrics_server(port: u16) { ) .expect("failed to configure batch_entries buckets") .install() - .expect("failed to start Prometheus metrics exporter"); + { + // A metrics port that is already taken must not look like a crashed node. + tracing::warn!(port, error = %e, "Prometheus metrics exporter not started"); + } } // Tokio metrics collection function