Fix: incorrect list commands

[Aryex]

Summary

blpop, brpop, blmove, rpoplpush and brpoplpush were all non-functional. This PR fixes all five. No breaking changes.

Issue link

Related #216

What was broken

Command	Failure
blpop / brpop	_bpop called send_blocking_command, a helper that does not exist anywhere in the repo → NoMethodError on every call. blpop also passed the Symbol :blpop where an Integer RequestType is required.
blmove	Prepended a stray :blmove token to argv → server rejected every call with wrong number of arguments.
rpoplpush / brpoplpush	Dispatched RequestType::RPOPLPUSH/BRPOPLPUSH, for which glide-core has no get_command mapping arm → Couldn't fetch command type- ClientError.

Changes

Blocking dispatch. blpop/brpop now go through the ordinary send_command — glide-core handles the blocking wait natively, so no separate dispatch path is needed.

rpoplpush/brpoplpush as facades over lmove/blmove. Valkey defines RPOPLPUSH as precisely LMOVE ... RIGHT LEFT, so the missing dispatch path is recoverable by fixed-argument currying rather than by dropping the API:

RPOPLPUSH  src dst          ≡  LMOVE  src dst RIGHT LEFT
BRPOPLPUSH src dst timeout  ≡  BLMOVE src dst RIGHT LEFT timeout

def rpoplpush(source, destination)
  lmove(source, destination, "RIGHT", "LEFT")
end

def brpoplpush(source, destination, timeout: 0)
  blmove(source, destination, "RIGHT", "LEFT", timeout: timeout)
end

As one-line facades they inherit lmove/blmove's where-normalization, timeout validation and argv construction, so they cannot drift. Both remain documented as deprecated (Redis 6.2) in favour of lmove/blmove. The unusable RequestType::RPOPLPUSH/BRPOPLPUSH constants are removed — nothing referenced them.

Explicit timeout keyword on blpop/brpop. Both took *args and inferred the timeout by sniffing for a trailing options Hash. That indirection is where the bugs came from: an undeclared parameter cannot be validated at the boundary, and a Hash that survives the sniff becomes a key while the timeout silently falls back to 0 — block forever. The positional timeout that *args existed to support was deprecated in redis-rb 4.8.0 and removed in 5.0.0, so the splat's only remaining job is variadic keys:

def blpop(*keys, timeout: 0)
def brpop(*keys, timeout: 0)

Every documented redis-rb 5.x call shape still works — single key, Array, splat, mixed, implicit timeout — and the contract now matches the Python client (keys checked in order, [key, value] return, 0 blocks indefinitely). Also dropped the unreachable &blk from _bpop: neither method declared or forwarded a block, so it was always nil.

Refactor. Extracted _validate_blocking_timeout so blmove validates its timeout the same way blpop/brpop do — new validation for blmove, which previously had none.

Docs. Corrected YARD on blpop/brpop/blmove/brpoplpush, all of which documented a @param [Hash] options that no longer exists. The @example lines mattered most: they showed :timeout => 5, which is the one form this PR changes, so copy-pasting them would have produced a block-forever call.

Compatibility

One behavioral change: an explicit positional Hash, blpop("k", { timeout: 1 }), previously worked via the Hash sniff and now treats the Hash as a key. This is the form redis-rb removed in 5.0.0; it is used nowhere in this repo, and blpop("k", timeout: 1) (keywords) is unaffected. All other forms are byte-identical — verified by diffing built argv across 13 call shapes before and after.

Checklist

• 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 - release-1.0

[Sasidharan3094]

@Aryex I will check this tomorrow. Can you please not remove rpoplpush and brpoplpush as we still support these in valkey right ?

[Aryex]

@Sasidharan3094 We could support then as facades as recommended by both Valkey and Redis. Example:

  RPOPLPUSH  src dst          ≡  LMOVE  src dst RIGHT LEFT
  BRPOPLPUSH src dst timeout  ≡  BLMOVE src dst RIGHT LEFT timeout

Please note that RPOPLPUSH and BRPOPLPUSH are not implemented at the core hence this would be the way to go.

[Sasidharan3094]

LGTM

[jamesx-improving]

Verdict: Approve

LGTM. The three fixes and the two facades all check out against Valkey's semantics, and the argv/currying guards in the tests will catch a regression on any of them.

Summary

The PR fixes five non-functional list commands (blpop, brpop, blmove, rpoplpush, brpoplpush) by:

• Removing a call to a non-existent send_blocking_command helper
• Dropping a stray leading token from blmove's argv
• Expressing rpoplpush/brpoplpush as fixed-argument facades over the semantically-equivalent lmove/blmove

Diff, tests, and PR description are internally consistent. I verified the semantic equivalence and did not find any correctness or security issues worth reporting.

Findings: 0 high-confidence correctness or security issues across the 4 files reviewed.

Verification notes (what I actually checked)

• send_blocking_command is gone from the repo — grep -rn "send_blocking_command" across lib/ and test/ returns no hits, and both call sites in list_commands.rb have been replaced with send_command.
• The two removed RequestType constants (RPOPLPUSH = 820, BRPOPLPUSH = 805) have no remaining callers — grep for RequestType::RPOPLPUSH/BRPOPLPUSH and case-variants across the repo returns nothing. Removing them is safe.
• The rpoplpush => LMOVE RIGHT LEFT equivalence is correct against Valkey semantics, including the source == destination "rotate the tail to the head" edge case, so the facade is safe. Same for brpoplpush => BLMOVE RIGHT LEFT with timeout preserved.
• _bpop now dispatches BLPOP/BRPOP via send_command with argv [*keys, timeout]. keys.flatten(1) handles both splat and Array forms; deeper nesting is additionally normalised by build_command_args's own flat_map in lib/valkey.rb:630.
• The one intentional break called out in the PR body — passing an explicit positional Hash blpop("k", { timeout: 1 }) — matches the redis-rb 5.0.0 removal and is not used anywhere in this repo. Nothing to flag.
• Test coverage is thorough: argv-shape guards via stub(:send_command), currying guards asserting rpoplpush/brpoplpush dispatch the exact same RequestType and argv as lmove/blmove, a real-server timeout-elapsed check for brpop, and float-timeout survival tests. The tests would catch a regression on any of the four fixes.

Low-confidence observations (not bugs, just author-glance notes)

• _validate_blocking_timeout accepts negative numeric timeouts and forwards them to the server, which then rejects them at RESP level. Fine as a client-side choice (server enforces the range) — noting it only in case the author wants a friendlier client-side error.
• test_brpop and test_blpop each issue one call with no explicit timeout after an rpush. If the server ever fails to pop for any reason the test blocks indefinitely rather than failing fast. Not a correctness issue in the PR; just a test-brittleness note.

[currantw]

Some review comments about deprecation and Redis vs Valkey. Will take another look more fully once addressed.

[currantw]

Why is BRPOPLPUSH deprecated? It doesn't appear to be marked as such for Valkey: https://valkey.io/commands/brpoplpush/

[Aryex]

It was deprecated in Redis. Valkey started as a fork of Redis so I'm guessing it is a hold over.
https://redis.io/docs/latest/commands/brpoplpush/

Both Valkey and Redis suggested to use LMOVE/BLMOVE as alternative so this simply implements that.

[currantw]

Looking at this Valkey issue, it seems like the deprecation decisions may have diverged on Valkey and Redis, and that these commands may be "undeprecated" in Valkey. We definitely need to support both, but I think that, if connected to a Valkey server, we should execute that corresponding command?

valkey-io/valkey#2546

[Aryex]

Hmm I guess that clear things up. However BRPOPLPUSH/RPOPLPUSH is currently not supported at the core. For Ruby GA, we can route them to LMOVE/BLMOVE until they are implemented at the core.

[currantw]

That makes sense. In terms of making this trackable, would it then make sense to:

• Raise an issue to add support for them in the core
• Raise an issue to add support for them in Ruby
• Link the Ruby issues with TODOs here

The core issue should make sure to link to the Ruby issue, and prompt whoever completes it to raise corresponding issues for the other independent clients (i.e. C# and PHP at the moment).

[Aryex]

Added in 06bcc5f

[currantw]

Thanks for adding all those TODOs for me! ✅