redis: fix reply-desynchronization bugs that delivered wrong values to wrong commands

RO
2mo ago
+622-166

What

Fixes a set of reply-desynchronization bugs in Bun.RedisClient where a command's promise resolves with some other command's reply (the "wrong value for the wrong key" class). These do not crash; they silently hand application code incorrect data under conditions that production Redis usage hits constantly.

All reproduce deterministically against an in-process byte-scripted RESP3 server (no real Redis needed).

Rebased onto #33072. That PR independently fixed two of the bugs this one originally covered (out-of-band pushes consuming a command's reply slot, and in-flight commands surviving a reconnect). This PR keeps that behavior, replaces the is_reply_kind prefix heuristic with explicit push-kind map entries (which also covers the pmessage/smessage kinds it did not), and fixes the remaining bugs below. The two tests covering #33072's fixes are kept as extra regression coverage: they pass on main today, the rest fail.

The bugs

Multi-channel subscribe(["a","b","c"], cb) steals other commands' reply slots

SUBSCRIBE with N channels emits N confirmation pushes but occupies one in-flight slot. The 2nd..Nth confirmations were consumed by whatever commands followed:

expect(received).toEqual(expected)
  {
-   "got": "value-of-k1",
-   "ping": "PONG",
+   "got": 3,          // GET k1 resolved with subscription count
+   "ping": 3,         // ping()  resolved with subscription count

An error reply in subscriber mode fails the whole connection instead of its own command

A -LOADING (or any -ERR) reply to PING on a subscriber connection went through fail(), which rejected every other pending command and left the PING's own promise unsettled:

expect(received).toEqual(expected)
  {
-   "status": "fulfilled",
-   "value": "PONG",
+   "reason": [Error: LOADING Redis is loading the dataset in memory],
+   "status": "rejected",      // the *second* PING, which got +PONG

Pattern and sharded subscription confirmations are not recognized

psubscribe, punsubscribe, pmessage and the Redis 7.0+ sharded ssubscribe, sunsubscribe, smessage push kinds were not in the push-kind map, so PSUBSCRIBE/SSUBSCRIBE resolved with the raw RESP push frame ({type, data}) instead of a channel count, and pmessage payloads were never dispatched as [channel, message].

Raw client.send("SUBSCRIBE", [...]) could not pair its confirmation

The raw escape hatch builds command meta via Meta::check, which only toggled auto-pipelining and never derived the subscription flags — so the confirmation push had no matching in-flight head to count against.

Extra unsubscribe confirmations could be charged to a queued SUBSCRIBE

An argless UNSUBSCRIBE emits one confirmation per currently-subscribed channel. With a single undirected SUBSCRIPTION_REQUEST flag, the extras could decrement and resolve a SUBSCRIBE that had been drained into the in-flight queue behind it.

Fix

In handle_response:

  • RESP3 push frames (>) are routed out-of-band unconditionally. The type byte alone marks them as unsolicited.
  • SUBSCRIPTION_REQUEST is split into SUBSCRIBE_REQUEST and UNSUBSCRIBE_REQUEST, so a confirmation only counts against an in-flight head of the matching direction. Each such request carries remaining_replies (its channel count) and is only consumed once all its confirmations have arrived.
  • Non-subscription pushes (invalidate, etc.) are dropped without consuming any reply slot.
  • An Error reply on a subscriber connection falls through to the same per-command rejection path as non-subscriber mode.

In Meta::check: subscription flags are derived from the command name, case-insensitively, covering the plain, pattern (P*) and sharded (S*) families — so every entry point (prototype methods and the raw send() hatch) pairs the same way. Any casing that matches also has auto-pipelining cleared, since the case-sensitive AUTO_PIPELINE_DISALLOWED_COMMANDS lookup would otherwise miss it.

In SUBSCRIPTION_PUSH_MESSAGES: all nine push kinds are mapped explicitly. This makes #33072's is_reply_kind prefix heuristic dead, so it is deleted; the explicit map additionally covers pmessage/smessage, which the heuristic rejected.

In on_close: the read buffer and reply scanner are reset (they belong to the dead socket), alongside #33072's in-flight rejection.

Behavior change

psubscribe() now resolves with the subscribed-channel count, like subscribe() (declared Promise<number>), instead of the raw RESP push frame. psubscribe is not in the public type declarations and the old value was a leaked protocol frame. The assertion in resp-nesting-depth.test.ts is updated; its load-bearing invariant (the pipelined PING still receives its own PONG) is unchanged.

Verification

$ bun bd test test/js/valkey/valkey-reply-desync.test.ts
 9 pass  0 fail

Without the src/ change, 7 of the 9 fail with the diffs shown above (the other 2 cover #33072's fixes and pass on main). The full scripted-server Valkey suite is green:

$ bun bd test test/js/valkey/{valkey-reply-desync,valkey-gc,valkey-incremental-scan,valkey-tls-verify}.test.ts \
             test/js/valkey/reliability/{connection-failures,resp-nesting-depth}.test.ts
 28 pass  0 fail  (13 skipped, need Docker)

Fixes #27861