Skip to content

fix: remove get interception of "shared" that discards WATCH - #225

Merged
Aryex merged 3 commits into
release-1.0from
jamesx/fix-214-get-in-multi-discards-watch
Aug 7, 2026
Merged

fix: remove get interception of "shared" that discards WATCH#225
Aryex merged 3 commits into
release-1.0from
jamesx/fix-214-get-in-multi-discards-watch

Conversation

@jamesx-improving

Copy link
Copy Markdown
Collaborator

Summary

get contained a hardcoded interception path that fired only when the key was literally "shared". On that path it issued a hidden DISCARD, read the key outside the transaction, then re-opened a fresh MULTI — destroying server-side WATCH state and silently losing a concurrent write. This PR deletes the interception path so get always takes the normal command path, and rewrites the test that the hack existed to satisfy.

Two observable failures went away:

  • Silent lost update. DISCARD unwatches all keys server-side, so the subsequent EXEC committed unconditionally and overwrote a concurrent write that should have aborted it. Nothing distinguished the broken path — no exception, no warning, no log entry — and "shared" is an entirely ordinary key name (shared counter, shared config, shared lock).
  • Wrong EXEC arity. The hack popped the GET from @queued_commands and reset the ledger, so exec returned one reply fewer than the caller queued. Deterministic; needs no concurrency to observe.

Issue link

Closes #214

Features / Changes

lib/valkey/commands/string_commands.rb

  • get is now just send_command(RequestType::GET, [key]).
  • Deleted three private helpers — should_intercept_get?, handle_transaction_isolation_get, isolation_check_pattern? — and the now-useless private keyword that guarded only those three (leaving it would trip RuboCop Lint/UselessAccessModifier).
  • Public surface of StringCommands is unchanged: 24 public methods before and after. Only dead private methods were removed.

lib/valkey.rb

  • Removed the dead @in_multi_block ivar. It was written false exactly once and read exactly once, from the deleted predicate; it was never set true anywhere in the repo, so the !@in_multi_block conjunct was a tautology.
  • @in_multi and @queued_commands are untouched — they are real transaction bookkeeping still consumed by transaction_commands.rb (exec / reconvert_queued_replies).

test/lint/transaction_commands.rb

  • Rewrote test_transaction_isolation; added two regression tests.

Why rewriting a pre-existing test is correct here, not merely convenient:

  1. The old assertion was impossible for any correct client. It asserted that a same-connection GET issued during MULTI returns the live pre-transaction value. After MULTI the server replies QUEUED to every command. That is protocol, not a judgment call — no client can satisfy the old assertion without faking it, which is exactly what the deleted code did.
  2. The impossible test and the production hack landed in the same commit. 1f846f5 ("Adding support for transaction commands", Adding support for transaction commands #74) added both the three helpers and the test requiring them. The deleted helper carried the inline comment "Only intercepts when key is shared to match the specific test case". The production code existed solely to satisfy its own test — no user requested it and nothing independently verified it.
  3. The suite already contradicted itself. test_queued_commands (test/lint/transaction_commands.rb:117) asserts assert_equal "QUEUED", r.get("foo") on an identical shape. The "shared" key gate was the only thing keeping the two assertions from colliding. That pre-existing test is also the precedent that makes the rewrite's "QUEUED" assertion uncontroversial.
  4. The rewrite tests isolation more strictly, not less. It keeps the isolation claim and observes it the only way possible: a second connection sees "initial" until EXEC, then "transaction_value". It additionally pins the EXEC arity, which the old test could not.

The two added regression guards: test_watch_with_a_modified_key_named_shared (a concurrent write to a WATCHed key named "shared" must abort EXEC) and test_exec_returns_a_reply_per_queued_command_with_a_key_named_shared (arity, observable without concurrency).

Testing

  • bundle exec rubocop — 99 files inspected, 0 offenses.
  • Baseline on pristine fb2fa46: 840 tests, 4149 assertions, 0 failures, 0 errors, 100 skips.
  • With this change: 842 tests, 4199 assertions, 0 failures, 0 errors, 100 skips.
    Command: CI=1 SKIP_TLS_TESTS=true VALKEY_PORT=6414 bundle exec rake test:standalone (dedicated Valkey 8.1.3 on port 6414).
  • Bisect proof that the new tests actually pin the fix. With lib/ reverted to the buggy state and only the tests kept, all three tests fail — test_transaction_isolation and test_watch_with_a_modified_key_named_shared both show -"QUEUED" / +"initial", and the arity test shows Expected "QUEUED", Actual nil. All three pass with the lib/ fix applied.
  • Gate reach verified across four command shapes before deleting: set/get; set/get/set (returned 2 replies for 3 queued commands); set/set/get (correct — GET third does not trip the gate); and the block form (correct and unaffected, since multi { } builds a Valkey::Pipeline and never sets @in_multi).
  • Cluster suite deferred — no local cluster available. Every new test carries skip(...) if cluster_mode?, matching the surrounding tests, since test/lint/ is shared by both suites. A reviewer with a cluster can confirm with bundle exec rake test:cluster.

Checklist

Before submitting the PR make sure the following are checked:

Comment thread test/lint/transaction_commands.rb Outdated
@Aryex
Aryex requested a review from currantw August 5, 2026 20:40

@currantw currantw left a comment

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.

Reviewed to c801942.

Comment thread lib/valkey/commands/string_commands.rb
Comment thread test/lint/transaction_commands.rb Outdated
@jamesx-improving
jamesx-improving force-pushed the jamesx/fix-214-get-in-multi-discards-watch branch from c801942 to d3f2170 Compare August 6, 2026 16:31
`get` contained a hardcoded interception path that fired only when the
key was literally "shared". On that path it issued a hidden DISCARD, read
the key outside the transaction, then re-opened a fresh MULTI. Two
observable failures followed:

  - Silent lost update. DISCARD unwatches all keys server-side, and the
    client keeps no record of which keys were watched, so it cannot even
    in principle re-WATCH them. The subsequent EXEC committed
    unconditionally and overwrote a concurrent write that should have
    aborted it. Nothing distinguished the broken path -- no exception, no
    warning, no log entry -- and "shared" is an ordinary key name.

  - Wrong EXEC arity. The hack popped the GET from @queued_commands and
    reset the ledger, so exec returned one reply fewer than the caller
    queued. Deterministic; observable without concurrency.

Remove the call site and the three private helpers (should_intercept_get?,
handle_transaction_isolation_get, isolation_check_pattern?) so `get` takes
the normal command path. Also remove the dead @in_multi_block ivar: it was
written false once and read once, from the deleted predicate, and never set
true anywhere, so the !@in_multi_block conjunct was a tautology.
@in_multi and @queued_commands are untouched -- they remain real
transaction bookkeeping consumed by exec/reconvert_queued_replies. The
public surface of StringCommands is unchanged (24 methods before and
after); only dead private methods were removed.

The test this hack existed to satisfy is rewritten rather than deleted.
test_transaction_isolation asserted that a same-connection GET during
MULTI returns the live pre-transaction value, which no correct client can
do -- after MULTI the server replies QUEUED to everything. The impossible
test and the production hack landed in the same commit, 1f846f5 (#74), and
the deleted helper's own comment read "Only intercepts when key is
`shared` to match the specific test case", so the production code existed
solely to satisfy its own test. The suite already contradicted itself:
test_queued_commands asserts QUEUED on an identical shape, and the
"shared" key gate was the only thing keeping the two from colliding. The
rewrite keeps the isolation claim but observes it the only possible way --
a second connection sees the pre-transaction value until EXEC -- and
additionally pins the EXEC arity, so it tests isolation more strictly, not
less. Two regression guards are added for the lost update and the arity.

Closes #214

Signed-off-by: James Xin <james.xin@improving.com>
Signed-off-by: James Xin <james.xin@improving.com>
Signed-off-by: James Xin <james.xin@improving.com>
@jamesx-improving
jamesx-improving force-pushed the jamesx/fix-214-get-in-multi-discards-watch branch from d3f2170 to 285a8a0 Compare August 6, 2026 19:24
@Aryex
Aryex merged commit 854574b into release-1.0 Aug 7, 2026
18 checks passed
@Aryex
Aryex deleted the jamesx/fix-214-get-in-multi-discards-watch branch August 7, 2026 00:47
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.

4 participants