Skip to content

fix: send real SCRIPT LOAD/EVAL/EVALSHA instead of the local cache - #227

Merged
Aryex merged 4 commits into
release-1.0from
jamesx/fix-213-scripting-wire-commands
Aug 7, 2026
Merged

fix: send real SCRIPT LOAD/EVAL/EVALSHA instead of the local cache#227
Aryex merged 4 commits into
release-1.0from
jamesx/fix-213-scripting-wire-commands

Conversation

@jamesx-improving

@jamesx-improving jamesx-improving commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

eval, evalsha, eval_ro, evalsha_ro and script_load never sent their
namesake commands to the server. They went through glide-core's client-side
Script container and invoke_script, which produced three distinct
user-visible defects. This PR dispatches real EVAL / EVALSHA / EVAL_RO /
EVALSHA_RO / SCRIPT LOAD over the wire, and adds the standard integer
key-count call form.

One root cause, three findings closed. This PR contains two breaking
changes
— see below.

Issue link

Closes #213

Also closes two findings from the #216 Group 1 rows (R2-12, R2-13), which were
not filed separately because they share this root cause.

Features / Changes

The three findings

  • R3-4 — KEYS/ARGV misassignment on the integer key-count form.
    eval(script, 1, "mykey", "myarg") — the form used by valkey-cli and every
    Valkey doc example — put the count in KEYS[1], shifted the real key into
    ARGV[1], and silently dropped everything after it. No error was raised.
  • R2-12 — script_load never sent SCRIPT LOAD. It computed a SHA1 into a
    client-local container, so the server had never seen the script. The returned
    SHA was unusable by any other client or process, and script_exists(sha)
    returned false for a script that had just been "loaded".
  • R2-13 — script_flush was silently undone. Because evalsha resolved
    through the client-side container, the next evalsha after a flush simply
    re-uploaded the script from the client and succeeded. Flushing the server
    cache had no observable effect.

Why wire dispatch instead of typed dispatch

RequestType::Eval, RequestType::EvalSha and RequestType::ScriptLoad have
enum values in glide-core but no get_command() arm, so typed
send_command fails outright with Couldn't fetch command type. To be precise:
this is 3 of the 5 methods. EVAL_RO and EVALSHA_RO do have arms
(glide-core request_type.rs:1291-1292) and would work with typed dispatch
today. All five are nonetheless routed through call to keep the family
uniform — a deliberate choice with a known cost, see Known cost below. The
rationale is recorded in a module comment so a future contributor can revert to
typed dispatch once glide-core gains the missing arms.

BREAKING: evalsha on a flushed/never-loaded SHA now raises

It raises Valkey::CommandError whose message contains NoScriptError
not Valkey::NoScriptError. Callers must catch Valkey::CommandError.

Valkey::NoScriptError is defined at lib/valkey/errors.rb:24 but is never
raised anywhere in lib/. The FFI exposes only four coarse RequestErrorType
values (UNSPECIFIED / EXECABORT / TIMEOUT / DISCONNECT), so NOSCRIPT
arrives as UNSPECIFIED and maps to CommandError. Verified live against a
real server.

Mapping the specific error subclasses is a pre-existing gap out of scope for
this PR
— it equally affects PermissionError, WrongTypeError and
OutOfMemoryError, and deserves its own issue. Flagging it here so the
behavior is not mistaken for a regression introduced by this change.

BREAKING (smaller): ambiguous argument shapes now raise

Previously silent misparses now raise ArgumentError:

  • Non-Integer numeric key counts (Float, Rational) — eval(s, 2.0, "k1", "k2")
    used to reproduce the exact bug this PR fixes, with no error at all.
  • More than two positional arrays.
  • script_load with a non-String script.

This follows the issue's own principle: proceeding with a silently different
meaning is precisely the thing to avoid. Integer and Array are disjoint in
the first positional slot, so recognizing the integer form is a strict superset
— it cannot change the meaning of any call that already worked.

A regression found during review, and fixed

Being honest about this one. Because every scripting method now goes through
call, which always forwards route:, and Valkey::Pipeline#send_command did
not accept route:, pipelined { |p| p.script_load(...) } regressed from
returning [] to raising ArgumentError: wrong number of arguments (given 3, expected 1..2)
.

Fixed by adding route: nil to Pipeline#send_command. The keyword is
accepted and ignored, matching the approach in #218 — batch-level routing
is a real feature but needs one route for the whole pipeline (see FFI
BatchOptionsInfo.route_info), not per-command routes. That work is tracked
in #137 and is out of scope here.

As a bonus, this also fixes a pre-existing case: pipelined { |p| p.dbsize; p.info }
used to raise ArgumentError on baseline because ~14 admin commands in
server_commands.rb forward route: unconditionally into Pipeline#send_command.
Verified: it now returns [Integer, Hash] cleanly. Non-block MULTI also
benefits — eval inside multi { } no longer executes outside the transaction.

Scope note: eval-in-pipeline was already broken before this branch
(NoMethodError on build_command_args), so that part is not a regression —
only script_load was. An earlier revision of this PR raised ArgumentError
when a route was supplied; changed to align with #218 after confirming no test
depends on either behavior.

Known cost — reviewer's call

OpenTelemetry spans for all five scripting methods are now named
CustomCommand rather than e.g. EVAL_RO (measured, not assumed).
EVAL_RO / EVALSHA_RO could retain real span names via typed dispatch today;
they were left uniform with the rest of the family and this is documented
in-code. Happy to split them out if maintainers prefer accurate span names over
family uniformity.

Tension with #206 — flagging, not resolving

#206 proposes a Script class; this PR moves away from the client-side
container. These are not fundamentally in conflictinvoke_script is
retained as the FFI seam. But #206 needs to know:

  • script_load no longer populates glide-core's scripts_container, so
    invoke_script's NOSCRIPT auto-reload path is now unreachable. A Script
    class must either call store_script itself, or be built on EVALSHA +
    SCRIPT LOAD fallback in Ruby.
  • Bindings.drop_script, Bindings.store_script,
    Bindings.free_script_hash_buffer and Bindings::ScriptHashBuffer are now
    entirely unreferenced in lib/ (drop_script was already dead before this
    branch). This PR enlarges the dead FFI surface. Left in place deliberately,
    since Add Script interface (invokeScript / Script class) parity with other GLIDE clients #206 may want it.

Cluster: reasoned, not locally tested

Routing is preserved because redis-rs derives it from the wire command name via
Routable::command() reading arg_idx(0), and CustomCommand builds an empty
Cmd with args appended. So EVALThirdArgAfterKeyCount (slot derived from
the key count this PR now sends correctly), EVAL_RO → replica-optional,
SCRIPT LOADAllNodes / AllSucceeded. Verified by reading glide-core
source. Actual cluster validation is deferred to CI — stating that plainly
rather than claiming coverage I don't have.

Files changed

File Change
lib/valkey/commands/scripting_commands.rb Wire dispatch for all five methods; new split_keys_and_args helper handling all three call shapes; module comment recording the typed-dispatch constraint
lib/valkey/pipeline.rb send_command accepts route: (raises if non-nil) — fixes the regression above
test/lint/scripting_commands.rb 26 new tests
CHANGELOG.md ## Pending entry, including both breaking changes

Testing

Lintbundle exec rubocop lib/valkey/commands/scripting_commands.rb lib/valkey/pipeline.rb test/lint/scripting_commands.rb0 offenses.

Standalone suiteCI=1 SKIP_TLS_TESTS=true VALKEY_PORT=6413 bundle exec rake test:standalone
against a dedicated Valkey 8.1.3 instance:

Run Tests Assertions Failures
Baseline (pristine fb2fa46) 840 4187 0
With this change 866 4207 0
Repeat run 866 4195 0

test_cached_script_execution — the known pre-existing flake (#167 / #124) —
passed in both final runs.

The R2-12 / R2-13 tests assert real server state, not client-side echoes:

  • test_script_load_makes_the_script_available_on_the_server
  • test_script_loaded_by_one_client_is_usable_by_another — cross-client, which
    is what actually proves the SHA reached the server
  • test_script_flush_really_invalidates_the_script — asserts the raise and
    re-asserts script_exists == false, proving no client-side re-upload
  • test_evalsha_raises_for_a_never_loaded_sha

All new tests live in the shared lint module, so they run in both the
standalone and cluster suites.

Deferred: bundle exec rake test:cluster — to CI (see Cluster above).

Performance: the warm path drops from 3 round trips to 1.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • Documentation is updated (if applicable).
  • Linters have been run (bundle exec rubocop) and pass.
  • Destination branch is correct - main (destination here is release-1.0, per the 1.0.0 GA release-blocker track)

Comment on lines +15 to +17
# Once glide-core gains those arms, these can revert to typed dispatch -
# EVAL_RO / EVALSHA_RO already have arms and could use it today; they go
# through #call only to keep the family's behavior uniform.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue here is that GLIDE clients does support custom script executions often through the Script class implementation, which the current Ruby client does not implement.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI tends to just spits out large comments blocks. I think we could remove this and instead add a TODO to #206

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 1d7aade — dropped the module-level block. I'll leave a TODO on #206 capturing the "revert to typed dispatch once glide-core has get_command() arms" context so it's not lost. Good call — you're right that it belonged there rather than as a header on the file.

Comment thread lib/valkey/commands/scripting_commands.rb Outdated
Comment thread CHANGELOG.md Outdated
The scripting commands never sent their namesake commands to the server.
eval, evalsha, eval_ro, evalsha_ro and script_load all resolved through
glide-core's client-side Script container and invoke_script, which caused
three distinct user-visible defects with one root cause:

* eval/evalsha misassigned KEYS/ARGV on the integer key-count form. The
  documented form eval(script, 1, "mykey", "myarg") put the count in
  KEYS[1], shifted the real key into ARGV[1], and dropped the remaining
  arguments without raising.
* script_load never sent SCRIPT LOAD, so the SHA it returned was unknown
  to the server, unusable from any other client, and script_exists on it
  returned false.
* script_flush was silently undone, because the next evalsha re-uploaded
  the script from the client-side container and succeeded.

All five methods now dispatch real wire commands via #call. Typed
dispatch is not an option for three of them: RequestType::Eval, EvalSha
and ScriptLoad have enum values but no get_command() arm in glide-core,
so typed send_command fails with "Couldn't fetch command type".
EVAL_RO and EVALSHA_RO do have arms and could use typed dispatch today;
they go through #call to keep the family uniform, at the cost of
OpenTelemetry spans being named CustomCommand. The constraint is
recorded in a module comment.

Two breaking changes:

* evalsha on a flushed or never-loaded SHA now raises
  Valkey::CommandError (message contains NoScriptError), not
  Valkey::NoScriptError - the FFI only exposes four coarse
  RequestErrorType values, so NOSCRIPT arrives as UNSPECIFIED. Mapping
  the specific subclasses is a pre-existing gap, out of scope here.
* Ambiguous argument shapes now raise instead of silently misparsing:
  non-Integer numeric key counts, more than two positional arrays, and
  a non-String script passed to script_load.

Pipeline#send_command now accepts route:, fixing a regression this
change would otherwise introduce - every scripting method goes through
raised ArgumentError. A route supplied to a pipeline is rejected
explicitly rather than dropped, since a batch dispatches as one unit.
This also fixes non-block MULTI, where eval previously ran outside the
transaction.

Cluster routing is preserved: redis-rs derives it from the wire command
name, so EVAL still routes by ThirdArgAfterKeyCount (from the key count
now sent correctly) and SCRIPT LOAD still broadcasts to all nodes.

Closes #213

Signed-off-by: James Xin <james.xin@improving.com>
…e slot

The wire-dispatch change surfaced two pre-existing test defects that the
old local-container path had masked.

EVAL_RO/EVALSHA_RO were added in Redis 7.0. The previous eval_ro path
went through script_load + invoke_script, which glide-core dispatched on
the wire as EVALSHA, so EVAL_RO never reached the server and the _ro
tests passed on 6.2 without ever exercising read-only dispatch. Now that
the real command is sent, those tests need the same target_version "7.0"
gate the repo already uses for other version-floored commands.

test_eval_with_integer_numkeys_multiple_keys_and_args passed k1 and k2 to
a single EVAL. Those hash to slots 12706 and 449, so cluster mode
correctly rejects it with CROSSSLOT. Adding a {1} hash tag puts both keys
in slot 9842, keeping the multi-key assertion meaningful in both modes
rather than skipping it in cluster.

Test-only; no library change.

Signed-off-by: James Xin <james.xin@improving.com>
Aligns with the approach in #218 rather than raising. Routing everything
through call() means route: is always forwarded, so Pipeline#send_command
has to accept the keyword or route-bearing commands raise ArgumentError
inside a pipeline.

Ignoring rather than raising also fixes the pre-existing case where
pipelined { |p| p.dbsize; p.info } raised, since ~14 admin commands in
server_commands.rb forward route: unconditionally.

Batch-level routing - a single route for the whole pipeline, which is what
the FFI BatchOptionsInfo.route_info field is for - is tracked in #137 and
is out of scope here.

Signed-off-by: James Xin <james.xin@improving.com>
…aking marker in CHANGELOG

Signed-off-by: James Xin <james.xin@improving.com>
@jamesx-improving
jamesx-improving force-pushed the jamesx/fix-213-scripting-wire-commands branch from 1d7aade to c985d4c Compare August 7, 2026 00:22
@Aryex
Aryex merged commit a6441a3 into release-1.0 Aug 7, 2026
18 checks passed
@Aryex
Aryex deleted the jamesx/fix-213-scripting-wire-commands branch August 7, 2026 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants