valkey: exclude every subscription command from auto-pipelining

[robobun]

Problem

• AUTO_PIPELINE_DISALLOWED_COMMANDS in src/runtime/valkey_jsc/ValkeyCommand.rs:170 contained b"UNPSUBSCRIBE", which is not a Redis command. The intended entry is PUNSUBSCRIBE; its siblings SUBSCRIBE, PSUBSCRIBE and UNSUBSCRIBE are listed correctly. The typo dates back to the original Bun.redis implementation, so the entry never matched anything.
• Effect: client.punsubscribe(...) and client.send("PUNSUBSCRIBE", ...) kept SUPPORTS_AUTO_PIPELINING after Meta::check (ValkeyCommand.rs:177), so they were written to the socket in the same batch as the pipelined commands issued in the same tick, and were not held back until in-flight commands were answered. Every other subscription command takes the held-back path.
• The same list disagreed with is_subscription_command() (ValkeyCommand.rs:188) in two more ways: it had no entry for the sharded SSUBSCRIBE / SUNSUBSCRIBE, and it only matched the uppercase spelling, while is_subscription_command() matches all six commands in any casing when it sets SUBSCRIPTION_REQUEST. So a raw send("unsubscribe", ...) was classified as a subscription request for reply pairing but pipelined like a normal command.
• docs/runtime/redis.mdx:371 documented the list with the same typo.

Fix

• Meta::check now clears SUPPORTS_AUTO_PIPELINING on any command that ends up flagged SUBSCRIPTION_REQUEST, and the four subscription entries (including the misspelled one) are removed from AUTO_PIPELINE_DISALLOWED_COMMANDS. There is now one classification of subscription commands, is_subscription_command(), driving both the reply-pairing flag and the pipelining exclusion, so the two cannot drift apart again.
• Why this is correct: PUNSUBSCRIBE was always meant to be excluded (the misspelled entry is that intent), and the fix gives it exactly the code path UNSUBSCRIBE and PSUBSCRIBE already take. Extending the exclusion to the sharded variants and to other spellings only affects raw send() calls that the client already treats as subscription requests; the dedicated methods all send the canonical uppercase names, so subscribe(), unsubscribe() and psubscribe() behave exactly as before. No other command's classification changes.
• Docs: the list in docs/runtime/redis.mdx now reads PUNSUBSCRIBE and includes SSUBSCRIBE / SUNSUBSCRIBE.
• Test: test/js/valkey/valkey-auto-pipelining.test.ts. A mock RESP3 server logs the order in which commands arrive and replies leave. Each test issues a pipelined PING and a subscription command in the same tick and expects > PING, < PING, > X, < X (X was held back until the PING was answered); a batched X shows up as > PING, > X, .... Rows: punsubscribe(), psubscribe(), raw send() of all six commands in upper and lower case, plus a control showing two PINGs do get batched. It needs no docker, unlike the rest of the test/js/valkey suite.
  • Released bun 1.4.0 and an unfixed debug build: 10 of 15 rows fail (punsubscribe(), PUNSUBSCRIBE, both sharded commands, all lowercase spellings); the 5 that pass are the control and the commands that were already excluded. 20 reruns give the same 200 failures / 100 passes on both builds.
  • Fixed debug (ASAN) build: 15 of 15 pass, and 300 of 300 with --rerun-each=20.
  • Fixed debug build against a real redis-server: the psubscribe / get / publish / punsubscribe flow from valkey.test.ts, psubscribe plus punsubscribe in the same tick, pipelined commands queued on both sides of a punsubscribe, and raw lowercase / sharded spellings all resolve with their own replies (output below).
  • resp-nesting-depth, valkey-incremental-scan, valkey-gc and connection-failures still pass (31 pass, 13 skipped for docker).
• redis: fix reply-desynchronization bugs that delivered wrong values to wrong commands #32858 corrects the same list entry in passing as part of a much larger reply-pairing rework; this is the focused fix, with the docs line and a test for the scheduling behavior itself.

Background

• Auto-pipelining: RedisClient does not write each command to the socket immediately. A command whose meta has SUPPORTS_AUTO_PIPELINING is queued and everything queued is written in one batch at the end of the current tick (on_auto_flush in valkey.rs). A command without the flag is held until every in-flight command has been answered and is then written on its own (enqueue / drain / send_next_command in valkey.rs), and on_auto_flush stops a batch at the first such command.
• Meta::check (ValkeyCommand.rs) computes the final meta for every command right before it is sent, for the dedicated methods and for raw send() alike. It is the only consumer of AUTO_PIPELINE_DISALLOWED_COMMANDS.
• SUBSCRIPTION_REQUEST: subscription commands are answered with RESP3 push frames rather than ordinary replies. This flag tells the response handler (handle_response in valkey.rs) that the in-flight entry at the head of the queue is waiting for such a push. is_subscription_command() was added recently so that raw send() of any of the six subscription commands, in any casing, gets the flag; the pipelining list predates it and was not updated.
• The debug-only assertion failed: self.is_subscriber() that a second back-to-back punsubscribe() trips is pre-existing (it reproduces identically on an unfixed debug build, and release builds are unaffected) and is being fixed separately in redis: accept unsubscribe acks that arrive outside subscriber mode #37340; the new test issues one unsubscribe per client, from subscriber mode, so it does not depend on that fix.

Real-server probe output (fixed debug build)

1: {"type":"psubscribe","data":["probe-chan-1*",1]} A B
1: publish -> 1
1: ping -> PONG
1: punsubscribe -> {"type":"punsubscribe","data":["probe-chan-1*",1]}
1: ping after -> PONG connected: true
2: {"type":"psubscribe","data":["probe-x.*",1]} {"type":"punsubscribe","data":["probe-x.*",0]}
2: ping after -> PONG
4: ["before",{"type":"punsubscribe","data":["probe-w.*",0]},"after"]
5: psubscribe -> {"type":"psubscribe","data":["probe-v.*",1]}
5: punsubscribe -> {"type":"punsubscribe","data":["probe-v.*",0]}
5: ssubscribe -> {"type":"ssubscribe","data":["probe-shard",1]}
5: sunsubscribe -> {"type":"sunsubscribe","data":["probe-shard",0]}
5: ping after -> PONG

Step 1 mirrors "commands issued alongside a multi-pattern psubscribe resolve with their own values" in valkey.test.ts. Step 4 is Promise.all([send("PING",["before"]), punsubscribe(...), send("PING",["after"])]), i.e. pipelined commands queued on both sides of the now held-back command.

Unfixed failure shape (released bun 1.4.0)

  [
    "> PING",
-   "< PING",
    "> PUNSUBSCRIBE",
+   "< PING",
    "< PUNSUBSCRIBE",
  ]

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 13 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ca8c3592-c4c1-470a-a3b8-6c2e5b0ff9e8

📥 Commits

Reviewing files that changed from the base of the PR and between 44acc3d and 8bffc7b.

📒 Files selected for processing (3)

• docs/runtime/redis.mdx
• src/runtime/valkey_jsc/ValkeyCommand.rs
• test/js/valkey/valkey-auto-pipelining.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 11:03 PM PT - Aug 14th, 2026}

@robobun, your commit 8bffc7b is building: #97472

[robobun]

Status: fix pushed; every CI lane that ran is green, the remaining red is two macOS jobs that never got an agent.

Reproduced with test/js/valkey/valkey-auto-pipelining.test.ts (mock RESP3 server, no docker): on the released bun 1.4.0 and on an unfixed debug build, punsubscribe(), send("PUNSUBSCRIBE"), both sharded commands and every lowercase spelling are written in the same batch as the PING issued alongside them (10 of 15 rows fail, stable across 20 reruns); SUBSCRIBE, PSUBSCRIBE and UNSUBSCRIBE are held back as documented. With this branch all 15 rows pass (300/300 with --rerun-each=20 on the ASAN debug build), and the psubscribe / punsubscribe flows from valkey.test.ts still resolve with their own replies against a real redis-server.

CI on 8bffc7b: 177 of 179 jobs passed. On the debian x64 ASAN lane the new file is 15/15 and the docker-backed test/js/valkey/valkey.test.ts suite (PUB/SUB included) is 927 pass / 0 fail. The only test failures in the build are retried-or-passed-alone flakes in unrelated files (bake dev stress, shell pipe read fault, inspect-error-leak, ctrl-c, bun-install-registry, child_process fork, napi node-gyp EBUSY on Windows arm64). The two darwin 14 aarch64 - test-bun jobs expired after two hours in the queue without running anything; the change is platform-independent Rust scheduling logic plus a loopback mock-server test, and the macOS queue is currently backlogged across the pipeline, so I am not re-queuing a build until it drains.

The debug-only assertion failed: self.is_subscriber() on back-to-back unsubscribes is pre-existing and tracked in #37340; this PR does not depend on it.

[claude]

LGTM — focused typo fix that consolidates subscription-command classification onto a single source of truth.

What was reviewed:

• Traced Meta::check callers: both send() in valkey.rs:1442 and the raw-send() path in js_valkey_functions.rs:477 route through it, and checked_command is what gets enqueued — the fix covers every path.
• Confirmed the separate if new.contains(SUBSCRIPTION_REQUEST) check (vs. nesting inside is_subscription_command) also handles callers that pre-set the flag (js_valkey_functions.rs:1701/1708/1825/1854).
• Verified the three removed non-typo entries (SUBSCRIBE/PSUBSCRIBE/UNSUBSCRIBE) are all matched case-insensitively by is_subscription_command(), so their behavior is unchanged.
• Test file: hermetic mock server on port: 0, buffers RESP frames across chunk boundaries, finally-cleanup, describe.concurrent, covers the full variant matrix plus a control.

Extended reasoning...

Overview

Three files: a 7-line net change to src/runtime/valkey_jsc/ValkeyCommand.rs (removes four subscription-command byte-string entries — one of which was the never-matching typo UNPSUBSCRIBE — from AUTO_PIPELINE_DISALLOWED_COMMANDS and adds three lines so Meta::check clears SUPPORTS_AUTO_PIPELINING whenever SUBSCRIPTION_REQUEST is set); the matching docs list in docs/runtime/redis.mdx; and a new docker-free test file that observes command-write ordering against a mock RESP3 server.

Security risks

None. This is scheduling metadata for an outbound Redis client. No parsing of untrusted input changes; no auth, crypto, or permissions surface.

Level of scrutiny

Low-to-medium. The Rust change is tiny and mechanical: it turns two divergent lists into one derived invariant, which is exactly the "one source of truth; derive one from the other" shape the review guide asks for. I traced the only two Meta::check call sites (valkey.rs:1442 for the internal send() all dedicated methods use, and js_valkey_functions.rs:477 for user-facing raw send()) and confirmed the checked meta is what reaches enqueue and every SUPPORTS_AUTO_PIPELINING consumer. The three correctly-spelled entries removed from the static set are all matched (case-insensitively) by is_subscription_command(), so their classification is unchanged; the newly-excluded cases (PUNSUBSCRIBE, sharded variants, lowercase raw spellings) were already flagged SUBSCRIPTION_REQUEST for reply-pairing, so this only brings the pipelining flag into agreement with existing intent.

Other factors

The test is well-constructed: hermetic (Bun.listen on 127.0.0.1, port: 0), no docker dependency, buffers socket reads to RESP frame boundaries, releases the client and server in finally, uses describe.concurrent, and covers the full matrix (dedicated punsubscribe()/psubscribe(), all six commands via raw send() in both casings, plus a positive control proving two PINGs do batch). The PR description documents fail-on-unfixed (10/15 fail on release 1.4.0 and unfixed debug) and pass-on-fixed (300/300 with --rerun-each=20 on ASAN debug), plus a real-server sanity probe. No CODEOWNERS entries cover valkey. No prior human review comments to address.