fix(scripting): route a script to the shard owning its keys instead of refusing it (#508)

CHANGELOG.md

@@ -125,6 +125,59 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   dispatcher — so `COMMAND COUNT` was advertising verbs Moon could not run.
 
 ### Fixed
+- **A script whose keys all lived on one shard was refused instead of routed (`CROSSSLOT`).**
+  Reported as "EVALSHA of a single-key script fails with `CROSSSLOT`" (#508), with the guess that a
+  1-element key list was being mis-folded. It was not. `validate_keys_same_shard` required every key
+  to hash to the shard the CONNECTION happened to occupy — so `CROSSSLOT` was standing in for "I
+  cannot run this *here*", and nothing ever asked where it *could* run. One key cannot cross slots;
+  it just lives somewhere else. Measured at `--shards 4`: **7 of 8** single-key `EVAL`s rejected,
+  only the key that happened to land on the connection's own shard running. `numkeys=0` always
+  worked, which is why the defect read as intermittent rather than total.
+
+  This broke `redis.lock.Lock.release()` — a single-key `EVALSHA`, and one of the most-used
+  constructs in `redis-py`. Through the `Script.__call__` wrapper the caller saw a `NoScriptError`
+  followed by a cross-slot error, neither of which names the cause.
+
+  A script now ROUTES to the shard owning its keys (`scripting::route_script_keys` →
+  `coordinator::coordinate_script` → a shard-side `EVAL`/`EVALSHA` arm on the SPSC `Execute` path),
+  because a script executes against one shard's database and the correct place to run it is where
+  that data is. Keys that GENUINELY span shards are still refused — there is no single target for
+  them — and that refusal is now doubly held: the routing decision rejects it before the hop, and
+  `validate_keys_same_shard` remains as the shard-side backstop, so the two must agree before a
+  script can read another shard's (empty) view of a key. Shards build their Lua VM lazily on first
+  use and share the one slot `conn_accept` fills, so a shard reached only by routing still gets
+  exactly one VM.
+
+  One thing this changes rather than fixes: `EVAL` caches its script only on the shard that ran it
+  and, unlike `SCRIPT LOAD`, never fans out — so a bare `EVAL` followed by a direct `EVALSHA` on a
+  key owned by another shard now answers `NOSCRIPT` where it previously answered `CROSSSLOT`.
+  Measured after the fix at `--shards 4`: one bare `EVAL`, then `EVALSHA` of that sha across 12
+  other keys — 4 ok, 8 `NOSCRIPT`. Neither ever worked, and
+  `NOSCRIPT` is the better failure because every client library retries it — `redis-py`'s
+  `Script.__call__` re-issues `EVAL` and self-heals, whereas `CROSSSLOT` was unrecoverable. Anything
+  built on `register_script`/`SCRIPT LOAD`, including `redis.lock.Lock`, is unaffected (12/12).
+  Filed as #515.
+
+  Applied to both handlers through one shared helper rather than two implementations that can drift.
+  `FCALL` has the same defect *plus* a second one — `FUNCTION LOAD` never fans out to other shards,
+  so routing alone would trade `CROSSSLOT` for `ERR Function not found` at the same rate. Filed as
+  #514 rather than half-fixed here.
+
+  A routed script that gets no reply no longer claims it did not run. `recv_reply_bounded` returns
+  the same error for a closed channel and for a 30s reply timeout, and the first cut of this path
+  reported both as "cross-shard reply channel closed during script execution". Those have opposite
+  retry semantics: a timeout means the target may still be executing, or may have already applied
+  its writes, so a client told the script never ran will re-send a non-idempotent script that did.
+  The two are now distinguished (`ReplyFailure::{Closed, TimedOut}`), both say execution status is
+  **unknown**, and the timeout records `moon_xshard_reply_timeout_total{kind="script"}` so a wedged
+  owner shard is visible in metrics like every other cross-shard reply path.
+
+  Not fixed here, and pre-existing rather than introduced: script writes skip `cow_intercept` on
+  every path, and under `runtime-tokio` `emit_effect` is compiled out entirely, so script writes
+  emit no AOF or replication record. Routed scripts persist exactly as well as local ones do —
+  which is the bug. Filed as #517, since a fix needs replication parity, kill-9 durability and a
+  VM A/B bench of its own.
+
 - **A pipeline did not execute in order at `--shards >= 2`, and writes were silently lost.**
   Reported as "MGET in the same pipeline as its SETs returns nulls" (#507). The cause is wider than
   the symptom: the sharded pipeline handlers DEFER a single-key command whose key lives on another

scripts/client-compat/redis_py/test_acceptance.py

@@ -364,11 +364,12 @@ def _await_message(ps, timeout: float = 5.0):
 
     # -- library-level constructs -----------------------------------------
 
-    def test_rp14_lock_is_exclusive(self):
-        """`redis.lock.Lock` acquisition is SET NX PX — a held lock is exclusive.
+    def test_rp14_lock_acquires_and_releases(self):
+        """`redis.lock.Lock` end to end — the most-used construct in redis-py.
 
-        Release is covered separately: it goes through EVALSHA, which has its
-        own known gap (moon#508, see below).
+        Acquisition is SET NX PX; RELEASE is a single-key EVALSHA, which is why
+        this test was split in half while moon#508 was open. Both halves are
+        asserted again now that a script routes to the shard owning its key.
         """
         c = self.client()
         lock = c.lock("rp14", timeout=5, blocking_timeout=2)
@@ -377,36 +378,40 @@ def test_rp14_lock_is_exclusive(self):
             c.lock("rp14", timeout=5, blocking_timeout=0.2).acquire(),
             "a held lock was acquired twice — SET NX is not exclusive",
         )
+        # The half that moon#508 broke: release() is EVALSHA of a one-key
+        # script, so it failed for whichever lock names did not hash to the
+        # connection's own shard.
+        lock.release()
+        self.assertTrue(
+            c.lock("rp14", timeout=5, blocking_timeout=2).acquire(),
+            "the lock could not be re-acquired after release()",
+        )
 
-    def test_rp14b_single_key_evalsha_is_a_known_gap(self):
-        """KNOWN GAP (moon#508), amplified for the same reason as rp7b.
+    def test_rp14b_single_key_evalsha_runs_on_the_keys_shard(self):
+        """moon#508, fixed: EVALSHA of a one-key script must run.
 
-        At `--shards >= 2`, EVALSHA of a script declaring ONE key is rejected
-        with `CROSSSLOT Keys in script don't hash to the same slot and shard`.
-        One key cannot cross slots. `SCRIPT EXISTS` returns `[True]`, so it is
-        the key-slot check and not a cache miss.
+        Was an inverted probe (assert at least one of twenty is rejected). It
+        is a direct assertion now, which is what the probe asked for when it
+        started failing.
 
-        This is what breaks `redis.lock.Lock.release()`, which is a single-key
-        EVALSHA — so the most-used construct in redis-py fails for about half
-        of all lock names. Like moon#507 it fires per-KEY (~50% at two shards),
-        which is why this probe runs twenty distinct keys instead of one.
+        Still twenty distinct keys rather than one, and for the probe's own
+        reason: the defect fired per-KEY, decided by which shard owned the key
+        relative to the connection's, so a single trial only samples one
+        placement.
         """
         c = self.client()
         sha = c.script_load("return redis.call('get', KEYS[1])")
         self.assertEqual(c.script_exists(sha), [True], "the script did not cache")
-        rejected = []
         for i in range(20):
             key = f"rp14b:{i}"
             c.set(key, "v")
-            try:
-                self.assertEqual(c.evalsha(sha, 1, key), "v")
-            except redis.exceptions.RedisError as e:
-                rejected.append((key, str(e)))
-        self.assertTrue(
-            rejected,
-            "all 20 single-key EVALSHA calls succeeded — moon#508 is fixed. "
-            "Delete this probe and restore the release half of rp14.",
-        )
+            self.assertEqual(
+                c.evalsha(sha, 1, key),
+                "v",
+                f"single-key EVALSHA on {key} did not run — a script whose one "
+                f"key lives on another shard must be ROUTED there (moon#508 "
+                f"regressed)",
+            )
 
     def test_rp15_from_url_and_pool_reuse(self):
         """`from_url` is how most applications construct a client at all."""

src/scripting/mod.rs

@@ -180,7 +180,113 @@ pub fn handle_script_subcommand(
     }
 }
 
+/// The shard's own Lua VM, plus what is needed to build it on first use.
+///
+/// Exists because moon#508's fix ROUTES a script to the shard owning its keys,
+/// so a script can now arrive over the SPSC mesh at a shard that has no
+/// connection of its own and has therefore never built a VM. Before routing,
+/// the VM was only ever created from `conn_accept`, on the connection's shard.
+///
+/// The `Rc<RefCell<Option<..>>>` slot is the SAME one `conn_accept` fills, so a
+/// shard still has exactly one VM however it is first reached — whichever path
+/// gets there first wins and the other reuses it.
+pub struct ShardLuaRuntime {
+    slot: Rc<RefCell<Option<Rc<Lua>>>>,
+    eviction_ctx: bridge::LuaEvictionCtx,
+    /// Carried here rather than added to the SPSC handler's already very wide
+    /// signature: both are per-shard constants, which is what this struct is.
+    num_shards: usize,
+}
+
+impl ShardLuaRuntime {
+    pub fn new(
+        slot: Rc<RefCell<Option<Rc<Lua>>>>,
+        eviction_ctx: bridge::LuaEvictionCtx,
+        num_shards: usize,
+    ) -> Self {
+        Self {
+            slot,
+            eviction_ctx,
+            num_shards,
+        }
+    }
+
+    pub fn num_shards(&self) -> usize {
+        self.num_shards
+    }
+
+    /// The shard's VM, built on first use.
+    ///
+    /// Returns `None` instead of panicking when the VM cannot be created: this
+    /// runs on the shard thread, where a panic aborts the whole process, and a
+    /// malformed-script client must never be able to do that. `conn_accept`
+    /// still `expect()`s at startup, where failing loudly is right.
+    pub fn vm(&self) -> Option<Rc<Lua>> {
+        let mut slot = self.slot.borrow_mut();
+        if slot.is_none() {
+            match setup_lua_vm(self.eviction_ctx.clone()) {
+                Ok(vm) => *slot = Some(vm),
+                Err(e) => {
+                    tracing::error!("Lua VM initialization failed on shard thread: {e}");
+                    return None;
+                }
+            }
+        }
+        slot.clone()
+    }
+}
+
+/// Where a script must run, decided by the shard ownership of its keys.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum ScriptRoute {
+    /// Run here: no keys at all, single shard, or every key is local.
+    Local,
+    /// Every key lives on this OTHER shard — send the script there.
+    Remote(usize),
+    /// The keys span shards. A script executes against ONE shard's database,
+    /// so this genuinely cannot be served and must be refused.
+    CrossShard,
+}
+
+/// Decide where a script's keys require it to run (moon#508).
+///
+/// Before this existed, [`validate_keys_same_shard`] was the whole policy, and
+/// it asked the wrong question: it required every key to hash to the shard the
+/// CONNECTION happened to occupy. One key cannot cross slots, but it very
+/// easily lives on another shard, so a single-key script was refused with
+/// `CROSSSLOT` about `1 - 1/shards` of the time — 7 of 8 measured at
+/// `--shards 4`. `CROSSSLOT` was standing in for "I cannot run this *here*",
+/// and nothing ever asked where it *could* run.
+///
+/// Keyless scripts stay local deliberately: with no key there is nothing to
+/// route by, every shard is equally correct, and running in place avoids a
+/// pointless hop. That is also why `numkeys=0` always worked and made the
+/// defect look intermittent instead of systematic.
+pub fn route_script_keys(keys: &[Bytes], shard_id: usize, num_shards: usize) -> ScriptRoute {
+    if num_shards <= 1 || keys.is_empty() {
+        return ScriptRoute::Local;
+    }
+    use crate::shard::dispatch::key_to_shard;
+    let target = key_to_shard(&keys[0], num_shards);
+    if keys[1..]
+        .iter()
+        .any(|k| key_to_shard(k, num_shards) != target)
+    {
+        return ScriptRoute::CrossShard;
+    }
+    if target == shard_id {
+        ScriptRoute::Local
+    } else {
+        ScriptRoute::Remote(target)
+    }
+}
+
 /// Validate that all keys hash to the current shard. Returns Some(error) on violation.
+///
+/// Retained as the shard-side backstop AFTER [`route_script_keys`] has already
+/// sent the script to the owning shard: at that point every key must be local,
+/// so a violation here means the routing decision and the execution site
+/// disagree — which would silently read another shard's (empty) view of a key.
 pub fn validate_keys_same_shard(
     keys: &[Bytes],
     shard_id: usize,

src/server/conn/handler_monoio/dispatch.rs

@@ -204,7 +204,7 @@ pub(super) fn try_handle_cluster(
 /// `#[inline]`: see `try_handle_cluster` rationale — name check inlines to the
 /// caller so non-EVALSHA commands cost only a length + byte compare.
 #[inline]
-pub(super) fn try_handle_evalsha(
+pub(super) async fn try_handle_evalsha(
     cmd: &[u8],
     cmd_args: &[Frame],
     conn: &ConnectionState,
@@ -214,6 +214,13 @@ pub(super) fn try_handle_evalsha(
     if !cmd.eq_ignore_ascii_case(b"EVALSHA") {
         return false;
     }
+    if let Some(routed) =
+        crate::server::conn::shared::route_script_elsewhere(cmd, cmd_args, conn.selected_db, ctx)
+            .await
+    {
+        responses.push(routed);
+        return true;
+    }
     let response = crate::shard::slice::with_shard(|s| {
         let db_count = s.databases.len();
         crate::scripting::handle_evalsha(
@@ -236,7 +243,7 @@ pub(super) fn try_handle_evalsha(
 /// `#[inline]`: see `try_handle_cluster` rationale — name check inlines so
 /// non-matching commands cost only a length + byte compare.
 #[inline]
-pub(super) fn try_handle_eval(
+pub(super) async fn try_handle_eval(
     cmd: &[u8],
     cmd_args: &[Frame],
     conn: &ConnectionState,
@@ -246,6 +253,13 @@ pub(super) fn try_handle_eval(
     if !cmd.eq_ignore_ascii_case(b"EVAL") {
         return false;
     }
+    if let Some(routed) =
+        crate::server::conn::shared::route_script_elsewhere(cmd, cmd_args, conn.selected_db, ctx)
+            .await
+    {
+        responses.push(routed);
+        return true;
+    }
     let response = crate::shard::slice::with_shard(|s| {
         let db_count = s.databases.len();
         let db = &mut s.databases[conn.selected_db];

src/server/conn/handler_monoio/mod.rs

@@ -1868,11 +1868,12 @@ pub(crate) async fn handle_connection_sharded_monoio<
                 continue;
             }
             if cmd_len == 7
-                && dispatch::try_handle_evalsha(cmd, cmd_args, &conn, ctx, &mut responses)
+                && dispatch::try_handle_evalsha(cmd, cmd_args, &conn, ctx, &mut responses).await
             {
                 continue;
             }
-            if cmd_len == 4 && dispatch::try_handle_eval(cmd, cmd_args, &conn, ctx, &mut responses)
+            if cmd_len == 4
+                && dispatch::try_handle_eval(cmd, cmd_args, &conn, ctx, &mut responses).await
             {
                 continue;
             }

src/server/conn/handler_sharded/mod.rs

@@ -1097,6 +1097,20 @@ pub(crate) async fn handle_connection_sharded_inner<
 
                     // --- Lua scripting: EVAL / EVALSHA ---
                     if cmd.eq_ignore_ascii_case(b"EVAL") || cmd.eq_ignore_ascii_case(b"EVALSHA") {
+                        // moon#508: a script whose keys all live on another
+                        // shard runs THERE. Same helper as handler_monoio —
+                        // one routing policy, not two that can drift.
+                        if let Some(routed) = crate::server::conn::shared::route_script_elsewhere(
+                            cmd,
+                            cmd_args,
+                            conn.selected_db,
+                            ctx,
+                        )
+                        .await
+                        {
+                            responses.push(routed);
+                            continue;
+                        }
                         let db_count = ctx.shard_databases.db_count();
                         // Unconditional slice path: ShardSlice is always initialized.
                         let response = crate::shard::slice::with_shard_db(conn.selected_db, |db| {

src/server/conn/shared.rs

@@ -1106,6 +1106,60 @@ fn is_inline_intercepted(cmd: &[u8]) -> bool {
     }
 }
 
+/// If this script's keys all live on ANOTHER shard, run it there and return
+/// that shard's reply. `None` means "run it here" — no keys, one shard, or the
+/// keys are already local.
+///
+/// This is the moon#508 fix. Previously any script whose keys were not on the
+/// connection's own shard was answered `CROSSSLOT`, which is only true when the
+/// keys span shards — a single key never crosses anything, it just lives
+/// somewhere else.
+///
+/// A genuinely cross-shard key set still returns `CROSSSLOT` here, before the
+/// hop: a script executes against one shard's database and cannot reach
+/// another's, so there is no target to send it to.
+pub(crate) async fn route_script_elsewhere(
+    cmd: &[u8],
+    cmd_args: &[Frame],
+    db_index: usize,
+    ctx: &crate::server::conn::core::ConnectionContext,
+) -> Option<Frame> {
+    if ctx.num_shards <= 1 {
+        return None;
+    }
+    // EVALSHA carries a sha where EVAL carries the body, but `parse_eval_args`
+    // only reads args[1..] for numkeys/keys — so the same parse serves both and
+    // the sha never has to be resolved just to decide where to run.
+    let keys = match crate::scripting::parse_eval_args(cmd_args) {
+        // Malformed args: let the local handler produce the exact error it
+        // always did rather than inventing a routing-flavoured one here.
+        Err(_) => return None,
+        Ok((_script, _numkeys, keys, _argv)) => keys,
+    };
+    match crate::scripting::route_script_keys(&keys, ctx.shard_id, ctx.num_shards) {
+        crate::scripting::ScriptRoute::Local => None,
+        crate::scripting::ScriptRoute::CrossShard => Some(Frame::Error(Bytes::from_static(
+            b"CROSSSLOT Keys in script don't hash to the same slot and shard",
+        ))),
+        crate::scripting::ScriptRoute::Remote(target) => {
+            let mut parts = Vec::with_capacity(cmd_args.len() + 1);
+            parts.push(Frame::BulkString(Bytes::copy_from_slice(cmd)));
+            parts.extend_from_slice(cmd_args);
+            Some(
+                crate::shard::coordinator::coordinate_script(
+                    std::sync::Arc::new(Frame::Array(parts.into())),
+                    target,
+                    ctx.shard_id,
+                    db_index,
+                    &ctx.dispatch_tx,
+                    &ctx.spsc_notifiers,
+                )
+                .await,
+            )
+        }
+    }
+}
+
 /// Check if a command is a multi-key command requiring VLL coordination.
 ///
 /// These commands operate on multiple keys that may live on different shards.