diff --git a/CHANGELOG.md b/CHANGELOG.md index 25db0ab8..51305ef8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,42 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — read-only replicas silently executed writing EVAL/EVALSHA (task #38) + +A client-issued `EVAL`/`EVALSHA` on a read-only replica ran unconditionally: +`EVAL`/`EVALSHA` deliberately carry no `WRITE` command-metadata flag (a +script's writes replicate as individually-emitted effect records, not as the +literal `EVAL`), so the connection-layer `try_enforce_readonly` gate that +blocks every other write command on a replica never saw them — a script's +`redis.call('SET', ...)` mutated the replica's local state with no +rejection at all, silently diverging it from the master. + +Fixed at the actual write boundary — `scripting::bridge::make_redis_call_fn` +(the `redis.call`/`redis.pcall` bridge) now rejects a `WRITE`-flagged inner +command with `-READONLY You can't write against a read only replica.` the +moment the shard's `ReplicationState` reports the replica role, mirroring +upstream Redis semantics: a script that never writes still runs to +completion on a replica (`EVAL_RO`/`FCALL_RO` and plain read scripts are +unaffected), while one that does write aborts at the first offending call. +Reuses the same `WS`/`MQ`/`GRAPH.QUERY` read-only-subcommand carve-outs as +the connection-level gate, and reads a lock-free `AtomicBool` mirror of +`ReplicationState::is_replica_mirror` (same pattern as +`ConnectionContext::is_replica_mirror`) so a tight script write-loop never +takes the `ReplicationState` lock. + +Verified this does not regress master→replica Lua effect replication (Wave +A part 2, task #34): replayed effects apply via +`replication::apply::apply_local`, which dispatches the inner write command +directly against storage and never touches the Lua bridge, so the new gate +cannot see — and cannot block — replica apply of a master's script effects +(`eval_effects_parity_shards1`/`_shards4`, `eval_incr_no_double_apply`, +`eval_writes_survive_restart` all still green). + +Multi-db note: `redis.call('SELECT', ...)` was already rejected inside +scripts (pre-existing fail-loud guard — the `CURRENT_DB` thread-local is +pinned to one `Database` for the whole script execution), so there is no +per-script db-switching path that could race or diverge from this new +readonly check. ### Fixed — MqPop WAL/replication payload v2 carries real PEL idle metadata (task #47) `apply_mq_pop` (boot-time WAL replay AND live replica apply — same function, diff --git a/src/scripting/bridge.rs b/src/scripting/bridge.rs index 2ffd69a0..135cec46 100644 --- a/src/scripting/bridge.rs +++ b/src/scripting/bridge.rs @@ -92,6 +92,14 @@ struct LuaEvictionInner { repl_state: Option>>, #[cfg_attr(not(feature = "runtime-monoio"), allow(dead_code))] aof_pool: Option>, + /// Task #38: lock-free snapshot of `ReplicationState::is_replica_mirror`, + /// cloned out once at ctx-construction time — same pattern as + /// `ConnectionContext::is_replica_mirror` (`server/conn/core.rs`), so a + /// tight `redis.call('SET', ...)` loop from Lua checks a single + /// `Acquire` load instead of taking `repl_state`'s `RwLock` per write. + /// `ReplicationState::set_role()` is the single owner of the mirror + /// invariant and updates the same `AtomicBool` thereafter. + is_replica_mirror: Option>, } impl LuaEvictionCtx { @@ -123,6 +131,13 @@ impl LuaEvictionCtx { repl_state: Option>>, aof_pool: Option>, ) -> Self { + // Task #38: snapshot the lock-free mirror the same way + // `ConnectionContext::new` does (`server/conn/core.rs`) — cloned out + // under the read-lock once here, kept in sync thereafter by + // `ReplicationState::set_role()` writing the same `AtomicBool`. + let is_replica_mirror = repl_state + .as_ref() + .and_then(|rs| rs.read().ok().map(|guard| guard.is_replica_mirror.clone())); LuaEvictionCtx(Some(LuaEvictionInner { shard_databases, runtime_config, @@ -133,9 +148,25 @@ impl LuaEvictionCtx { num_shards, repl_state, aof_pool, + is_replica_mirror, })) } + /// Task #38: true iff this shard currently believes it is a read-only + /// replica (`ReplicationState::role == Replica`). Checked by + /// `make_redis_call_fn` before letting a Lua `redis.call`/`redis.pcall` + /// execute a `WRITE`-flagged inner command — mirrors upstream Redis, + /// which fails a script at the *first* write attempt inside it rather + /// than rejecting `EVAL`/`EVALSHA` outright (a read-only script must + /// still run on a replica). `false` for a disabled ctx (unit tests) and + /// whenever no `ReplicationState` is wired up (standalone server). + fn is_replica(&self) -> bool { + self.0 + .as_ref() + .and_then(|inner| inner.is_replica_mirror.as_ref()) + .is_some_and(|mirror| mirror.load(std::sync::atomic::Ordering::Acquire)) + } + /// Run the same eviction/OOM gate the connection handlers use /// (`run_write_eviction_gate` in `handler_monoio/mod.rs`), against `db` /// (the shard's `Database`, already borrowed by the caller via the @@ -358,6 +389,42 @@ pub fn make_redis_call_fn( "Write commands are not allowed from read-only scripts".to_string(), )); } + // Task #38: reject a write attempted from a CLIENT-issued script + // on a read-only replica, at the first offending `redis.call`/ + // `redis.pcall` — matching upstream Redis (a script that never + // writes still runs on a replica; one that does is aborted mid- + // script with `-READONLY`). This intentionally mirrors the exact + // carve-outs `try_enforce_readonly` uses for the connection-level + // gate (`server/conn/handler_monoio/dispatch.rs`) — commands that + // are blanket-`WRITE`-flagged in `COMMAND_META` but carry + // read-only subcommands. Master→replica Lua effect replication + // (Wave A part 2, task #34) NEVER reaches this closure: replayed + // effects are applied via `replication::apply::apply_local`, + // which dispatches the inner command directly against storage + // and never runs a Lua VM at all (see that module's doc comment) + // — so this check cannot collide with, or block, replica apply. + if cmd_is_write && eviction_ctx.is_replica() { + let allowed_on_replica = if cmd_bytes.eq_ignore_ascii_case(b"WS") { + crate::command::workspace::is_ws_readonly_subcommand(&frames[1..]) + } else if cmd_bytes.eq_ignore_ascii_case(b"MQ") { + crate::command::mq::is_mq_readonly_subcommand(&frames[1..]) + } else { + #[cfg(feature = "graph")] + { + cmd_bytes.eq_ignore_ascii_case(b"GRAPH.QUERY") + && !crate::command::graph::is_cypher_write_query(&frames[1..]) + } + #[cfg(not(feature = "graph"))] + { + false + } + }; + if !allowed_on_replica { + return Ok(Frame::Error(Bytes::from_static( + b"READONLY You can't write against a read only replica.", + ))); + } + } if cmd_is_write { // Track writes for SCRIPT KILL safety check SCRIPT_HAD_WRITE.with(|c| c.set(true)); @@ -524,4 +591,77 @@ mod tests { "bystander eviction inside the Lua gate must emit a DEL record to the AOF plane" ); } + + /// Task #38: `LuaEvictionCtx::is_replica()` must track + /// `ReplicationState::is_replica_mirror` exactly, including transitions + /// made AFTER the ctx was constructed — `make_redis_call_fn` calls this + /// per `redis.call`, so a `REPLICAOF`/`REPLICAOF NO ONE` mid-lifetime + /// role flip (S3.5a's whole reason for the mirror existing) must be + /// visible to a long-lived shard's Lua bridge without rebuilding the + /// ctx. + /// + /// RED (before this task): `LuaEvictionInner` carried no + /// `is_replica_mirror` field at all — `LuaEvictionCtx` had no way to + /// answer "is this shard a read-only replica right now," so + /// `make_redis_call_fn` could not reject a writing script on a replica. + #[test] + fn is_replica_tracks_role_transitions_after_construction() { + use crate::replication::state::{ReplicaHandshakeState, ReplicationRole, ReplicationState}; + + let (shard_databases, _inits) = ShardDatabases::new(vec![vec![Database::new()]]); + let runtime_config = Arc::new(parking_lot::RwLock::new(make_config(0, "noeviction"))); + let repl_state = Arc::new(std::sync::RwLock::new(ReplicationState::new( + 1, + "a".repeat(40), + "0".repeat(40), + ))); + + let ctx = LuaEvictionCtx::new( + shard_databases, + runtime_config, + 0, + None, + Rc::new(Cell::new(1)), + None, + 1, + Some(repl_state.clone()), + None, + ); + + assert!( + !ctx.is_replica(), + "fresh ReplicationState defaults to Master" + ); + + repl_state + .write() + .unwrap() + .set_role(ReplicationRole::Replica { + host: "127.0.0.1".to_string(), + port: 6379, + state: ReplicaHandshakeState::PingPending, + }); + assert!( + ctx.is_replica(), + "is_replica() must observe a role flip that happened after ctx construction" + ); + + repl_state + .write() + .unwrap() + .set_role(ReplicationRole::Master); + assert!( + !ctx.is_replica(), + "REPLICAOF NO ONE must flip is_replica() back to false" + ); + } + + /// `is_replica()` on a `disabled()` ctx (no shard context — plain unit + /// tests of Lua scripts that don't go through a real shard) must be + /// `false`, never panic. + #[test] + fn is_replica_false_for_disabled_ctx() { + let ctx = LuaEvictionCtx::disabled(); + assert!(!ctx.is_replica()); + } } diff --git a/tests/replication_readonly_eval.rs b/tests/replication_readonly_eval.rs new file mode 100644 index 00000000..156212ee --- /dev/null +++ b/tests/replication_readonly_eval.rs @@ -0,0 +1,185 @@ +//! Task #38: a read-only replica must reject a CLIENT-issued `EVAL`/ +//! `EVALSHA` script that attempts a write, at the first offending +//! `redis.call`/`redis.pcall` inside it — matching upstream Redis, which +//! lets a script run right up until it tries to mutate state, then aborts +//! with `-READONLY` rather than rejecting `EVAL` outright (a read-only +//! script must still be served on a replica; e.g. `SCRIPT LOAD`+`EVALSHA` +//! doing only `GET`s is a normal read workload against a replica). +//! +//! Mirrors `tests/replication_readonly_ws_mq.rs`'s harness pattern exactly +//! (same real-binary spawn, same `assert_readonly` helper shape) since the +//! enforcement point (`scripting::bridge::make_redis_call_fn`) is inside the +//! same sharded connection-handler code the WS/MQ gate lives next to, and is +//! equally invisible to a synthetic single-shard harness. +//! +//! Run against a monoio (default-feature) build: +//! ```text +//! cargo build --release +//! MOON_BIN=./target/release/moon cargo test --test replication_readonly_eval -- --ignored --nocapture +//! ``` +//! +//! Run against a tokio+jemalloc build (handler_sharded parity): +//! ```text +//! cargo build --release --no-default-features --features runtime-tokio,jemalloc +//! MOON_BIN=./target/release/moon cargo test --test replication_readonly_eval -- --ignored --nocapture +//! ``` + +mod common; + +use std::process::{Child, Command, Stdio}; +use std::time::Duration; + +fn moon_bin() -> String { + std::env::var("MOON_BIN").unwrap_or_else(|_| "./target/release/moon".to_string()) +} + +fn start_moon(port: u16, dir: &str) -> Child { + let port_s = port.to_string(); + Command::new(moon_bin()) + .args([ + "--port", + &port_s, + "--shards", + "1", + "--dir", + dir, + "--disk-free-min-pct", + "0", + "--appendonly", + "no", + ]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("Failed to start moon (set MOON_BIN to a built binary)") +} + +async fn connect(port: u16) -> redis::aio::MultiplexedConnection { + let client = redis::Client::open(format!("redis://127.0.0.1:{port}/")).expect("client open"); + for _ in 0..50 { + if let Ok(con) = client.get_multiplexed_async_connection().await { + return con; + } + tokio::time::sleep(Duration::from_millis(100)).await; + } + panic!("failed to connect to moon on port {port}"); +} + +fn assert_readonly(result: redis::RedisResult, label: &str) { + assert!( + result.is_err(), + "{label} on a read-only replica should return READONLY error, got: {result:?}" + ); + let err = result.unwrap_err().to_string(); + assert!( + err.to_uppercase().contains("READONLY"), + "{label} error should say READONLY, got: {err}" + ); +} + +#[tokio::test] +#[ignore = "spawns a real moon binary — set MOON_BIN, see module docs"] +async fn test_readonly_replica_blocks_writing_eval_and_evalsha() { + let dir = tempfile::tempdir().unwrap(); + let (mut child, port) = + common::spawn_listening(|p| start_moon(p, dir.path().to_str().unwrap())); + let mut con = connect(port).await; + + // --- Master: a writing EVAL succeeds, proving the harness/script path + // itself works before we assert the replica rejects it. --- + let eval_write_on_master: redis::RedisResult = redis::cmd("EVAL") + .arg("return redis.call('SET', KEYS[1], ARGV[1])") + .arg(1) + .arg("eval-key") + .arg("eval-value") + .query_async(&mut con) + .await; + assert_eq!(eval_write_on_master, Ok("OK".to_string())); + + let get_on_master: String = redis::cmd("GET") + .arg("eval-key") + .query_async(&mut con) + .await + .expect("GET after EVAL SET should succeed on master"); + assert_eq!(get_on_master, "eval-value"); + + // Load a script for the EVALSHA leg below. + let sha: String = redis::cmd("SCRIPT") + .arg("LOAD") + .arg("return redis.call('SET', KEYS[1], ARGV[1])") + .query_async(&mut con) + .await + .expect("SCRIPT LOAD should succeed on master"); + + // A read-only script also succeeds on master (sanity for the replica + // assertion below — proves EVAL itself, not just writes, is reachable). + let eval_read_on_master: redis::RedisResult = redis::cmd("EVAL") + .arg("return redis.call('GET', KEYS[1])") + .arg(1) + .arg("eval-key") + .query_async(&mut con) + .await; + assert!( + eval_read_on_master.is_ok(), + "read-only EVAL should succeed on master: {eval_read_on_master:?}" + ); + + // --- Become a read-only replica of a non-existent master (same + // pattern as tests/replication_test.rs::test_readonly_replica and + // tests/replication_readonly_ws_mq.rs). --- + let repl: String = redis::cmd("REPLICAOF") + .arg("127.0.0.1") + .arg("9999") + .query_async(&mut con) + .await + .expect("REPLICAOF should succeed"); + assert_eq!(repl, "OK"); + + // --- A writing EVAL must be rejected -READONLY once it hits the first + // write redis.call — not silently executed and diverging from master. --- + let eval_write_on_replica: redis::RedisResult = redis::cmd("EVAL") + .arg("return redis.call('SET', KEYS[1], ARGV[1])") + .arg(1) + .arg("blocked-eval-key") + .arg("blocked-eval-value") + .query_async(&mut con) + .await; + assert_readonly(eval_write_on_replica, "EVAL SET"); + + // Same for EVALSHA against the pre-loaded script. + let evalsha_write_on_replica: redis::RedisResult = redis::cmd("EVALSHA") + .arg(&sha) + .arg(1) + .arg("blocked-evalsha-key") + .arg("blocked-evalsha-value") + .query_async(&mut con) + .await; + assert_readonly(evalsha_write_on_replica, "EVALSHA SET"); + + // The rejected writes must not have landed. + let blocked_key_missing: Option = redis::cmd("GET") + .arg("blocked-eval-key") + .query_async(&mut con) + .await + .expect("GET should succeed (read) on a read-only replica"); + assert_eq!( + blocked_key_missing, None, + "a write rejected with READONLY must not have been applied" + ); + + // --- A read-only script must still be served on the replica (matches + // upstream Redis: EVAL itself is not blanket-blocked, only its writes). --- + let eval_read_on_replica: redis::RedisResult = redis::cmd("EVAL") + .arg("return redis.call('GET', KEYS[1])") + .arg(1) + .arg("eval-key") + .query_async(&mut con) + .await; + assert!( + eval_read_on_replica.is_ok(), + "read-only EVAL must still be served on a read-only replica, got: {eval_read_on_replica:?}" + ); + + let _ = child.kill(); + let _ = child.wait(); +}