redis: fix reply-desynchronization bugs that delivered wrong values to wrong commands - #32858
redis: fix reply-desynchronization bugs that delivered wrong values to wrong commands#32858robobun wants to merge 1 commit into
Conversation
WalkthroughAdds reply-count tracking for Valkey commands, splits subscription request metadata, updates RESP3 push handling to consume multi-reply subscription confirmations correctly, adjusts connection-close cleanup, extends pattern-subscription protocol mapping, and adds desync regression tests. Valkey reply-slot pairing
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:37 AM PT - Jul 5th, 2026
✅ @robobun, your commit db3de576dc6c57cedf450219be6ac43fc4f3ffa5 passed in 🧪 To try this PR locally: bunx bun-pr 32858That installs a local version of the PR into your bun-32858 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Added Not claiming #21622: that report is about the npm |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/valkey_jsc/valkey.rs`:
- Around line 632-636: The new comment blocks in valkey.rs exceed the 3-line
comment limit and should be condensed. Shorten the explanatory comments around
the affected socket/transport handling in the relevant methods (including the
close/reject logic and related helper sections) so each block keeps only the
durable invariant, and move the extra rationale into the PR description instead.
- Around line 916-940: The SUBSCRIBE promise is being resolved with a stale
subscription count because `sub_count` is read before `add_subscription()` and
`on_valkey_subscribe(value)` in the Valkey subscription handler. Update the
`SubscriptionPushMessage::Subscribe` branch in `on_valkey_push_message` so the
subscription state is applied first, then compute the current subscribed count
from `_subscription_ctx` and use that value when resolving the promise. This
keeps the `(P)SUBSCRIBE` confirmation aligned with the latest state.
In `@src/runtime/valkey_jsc/ValkeyCommand.rs`:
- Around line 111-114: The subscription reply counting in
ValkeyCommand::expected_reply_count is undercounting zero-argument
UNSUBSCRIBE/PUNSUBSCRIBE commands because it uses the current args length
instead of the active subscription count. Update the zero-arg branch to derive
the expected reply count from the pre-clear subscription total for the current
connection/context, while keeping the existing behavior for explicit channel
arguments and other subscription requests.
In `@test/js/valkey/valkey-reply-desync.test.ts`:
- Around line 5-8: The file-level and inline comments in
valkey-reply-desync.test.ts exceed the repo’s 3-line comment limit; trim the
explanatory text to concise 3-line max summaries and move the longer
reply-integrity/buffering rationale to the PR description. Update the comment
blocks around the test setup and the affected test sections so they stay terse
while still pointing to the invariant, especially in the blocks near the
Byte-scripted RESP3 server setup and the other flagged comment regions.
- Around line 320-357: The current valkey reply-desync coverage only verifies
the PSUBSCRIBE confirmation path in RedisClient.psubscribe; add scripted
mock-server tests for pattern message dispatch and PUNSUBSCRIBE reply-slot
mapping as well. Extend the existing valkey-reply-desync suite with cases that
exercise pmessage handling and unsubscribing from a pattern subscription so the
reply pairing logic is covered for all pattern paths alongside the existing
subscribe/unsubscribe behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59037164-0d93-4d6b-b106-22baf3008b45
📒 Files selected for processing (5)
src/runtime/valkey_jsc/ValkeyCommand.rssrc/runtime/valkey_jsc/js_valkey_functions.rssrc/runtime/valkey_jsc/valkey.rssrc/valkey/valkey_protocol.rstest/js/valkey/valkey-reply-desync.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/valkey_jsc/ValkeyCommand.rs (1)
199-212: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the auto-pipeline disallow check match the new case-insensitive command parsing.
SUBSCRIBE/UNSUBSCRIBEflags are derived witheq_ignore_ascii_case, butSUPPORTS_AUTO_PIPELININGstill uses an exact table lookup. A rawclient.send("subscribe", ...)can therefore be recognized as a subscription request while remaining auto-pipelineable.Suggested fix
+ let auto_pipeline_disallowed = AUTO_PIPELINE_DISALLOWED_COMMANDS + .iter() + .any(|disallowed| command.command.eq_ignore_ascii_case(disallowed)); new.set( Meta::SUPPORTS_AUTO_PIPELINING, - !AUTO_PIPELINE_DISALLOWED_COMMANDS.contains(command.command), + !auto_pipeline_disallowed, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/valkey_jsc/ValkeyCommand.rs` around lines 199 - 212, The auto-pipeline gate in ValkeyCommand::new is still doing a case-sensitive lookup even though subscription handling now uses case-insensitive command parsing. Update the SUPPORTS_AUTO_PIPELINING check to use the same case-insensitive matching as the SUBSCRIBE/PSUBSCRIBE and UNSUBSCRIBE/PUNSUBSCRIBE logic, so commands like “subscribe” and “unsubscribe” are disallowed from auto-pipelining consistently.
♻️ Duplicate comments (1)
src/runtime/valkey_jsc/valkey.rs (1)
912-936: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResolve
SUBSCRIBEafter updating subscription state.
sub_countis read beforeadd_subscription()/on_valkey_subscribe(value), so the resolving(P)SUBSCRIBEpromise can still miss the current confirmation. Move the count lookup into theSubscribebranch after the state update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/valkey_jsc/valkey.rs` around lines 912 - 936, The SUBSCRIBE confirmation in the valkey push handler is resolving with a stale subscription count because `channels_subscribed_to_count` is read before `add_subscription()` and `on_valkey_subscribe(value)`. Update `process_push_message` so the count is fetched inside the `SubscriptionPushMessage::Subscribe` branch after the subscription state is updated, then use that fresh value when resolving the promise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/valkey/valkey-reply-desync.test.ts`:
- Around line 363-387: The raw send subscription test only covers uppercase
SUBSCRIBE and uses a loose numeric assertion, so tighten it to fail
deterministically. Update the RedisClient send/raw command test around the
SUBSCRIBE case to also exercise a lowercase command path (send with "subscribe")
and assert the exact confirmation count returned is 1. Keep the existing socket
mock and reply-slot behavior in the same test, using the same send()/ping() flow
to verify casing-insensitive metadata.
---
Outside diff comments:
In `@src/runtime/valkey_jsc/ValkeyCommand.rs`:
- Around line 199-212: The auto-pipeline gate in ValkeyCommand::new is still
doing a case-sensitive lookup even though subscription handling now uses
case-insensitive command parsing. Update the SUPPORTS_AUTO_PIPELINING check to
use the same case-insensitive matching as the SUBSCRIBE/PSUBSCRIBE and
UNSUBSCRIBE/PUNSUBSCRIBE logic, so commands like “subscribe” and “unsubscribe”
are disallowed from auto-pipelining consistently.
---
Duplicate comments:
In `@src/runtime/valkey_jsc/valkey.rs`:
- Around line 912-936: The SUBSCRIBE confirmation in the valkey push handler is
resolving with a stale subscription count because `channels_subscribed_to_count`
is read before `add_subscription()` and `on_valkey_subscribe(value)`. Update
`process_push_message` so the count is fetched inside the
`SubscriptionPushMessage::Subscribe` branch after the subscription state is
updated, then use that fresh value when resolving the promise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f707f407-7eef-47c4-9176-bae6854610ce
📒 Files selected for processing (3)
src/runtime/valkey_jsc/ValkeyCommand.rssrc/runtime/valkey_jsc/valkey.rstest/js/valkey/valkey-reply-desync.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/valkey/valkey-reply-desync.test.ts`:
- Around line 365-369: The raw command matrix in the desync test is missing the
lowercase sharded case, so add the case-insensitive `"ssubscribe"` entry
alongside `"SSUBSCRIBE"` in the `test.each` table. Update the `raw send(%p,
[...]) pairs its confirmation push with its own reply slot` matrix so
`commandName`/`pushKind` coverage includes `["ssubscribe", "ssubscribe"]` and
the sharded path is tested for both casings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c8df724-773a-48e1-843e-1c1d9d089305
📒 Files selected for processing (3)
src/runtime/valkey_jsc/ValkeyCommand.rssrc/valkey/valkey_protocol.rstest/js/valkey/valkey-reply-desync.test.ts
There was a problem hiding this comment.
All four of my earlier regression findings are addressed and the remaining inline note is a zero-impact doc-comment nit, but this rewrites the core RESP3 reply-routing state machine and on_close rejection semantics for Bun.RedisClient, so it's worth a human pass over the final handle_response / reject_in_flight shape before merge.
Extended reasoning...
Overview
This PR fixes four independent reply-desynchronization bugs in Bun.RedisClient where a command's promise resolves with another command's reply. It touches the heart of the client's correctness model:
src/runtime/valkey_jsc/valkey.rs: rewrites thehandle_responsepush-routing logic (RESP3>frames are now routed out-of-band unconditionally; subscription confirmations decrement a per-commandremaining_repliescounter and only consume the in-flight head when direction matches), removes theSubscribeHandledenum, and addsreject_in_flightsoon_closerejects all in-flight commands before reconnect.src/runtime/valkey_jsc/ValkeyCommand.rs: addsexpected_reply_count(), splitsSUBSCRIPTION_REQUESTinto directionalSUBSCRIBE_REQUEST/UNSUBSCRIBE_REQUESTflags, derives those flags case-insensitively inMeta::checkfor the rawsend()path, addsSSUBSCRIBE/SUNSUBSCRIBE/PUNSUBSCRIBEto the auto-pipeline disallow set (and fixes the pre-existingUNPSUBSCRIBEtypo).src/valkey/valkey_protocol.rs: extendsSUBSCRIPTION_PUSH_MESSAGESto recognizep*ands*push kinds.src/runtime/valkey_jsc/js_valkey_functions.rs: extendscmd_strings_varargs!to accept a meta expression and flagspsubscribe/punsubscribe/subscribe/unsubscribewith the directional flags.test/js/valkey/valkey-reply-desync.test.ts: 450-line byte-scripted RESP3 server with 7 deterministic regression tests.
Security risks
None identified. No auth, crypto, permissions, or filesystem changes. Server input is parsed by the existing RESP scanner (unchanged); the diff only changes which already-parsed values are routed to which JS promise. The new on_close path strictly rejects more (previously it leaked stale slots), which is safer.
Level of scrutiny
High. This is a data-correctness change to a production API (Bun.RedisClient) where a bug silently delivers wrong values to application code rather than crashing. The PR went through four rounds of substantive regression findings from me during review (psubscribe push-kind gap, argless-unsubscribe cross-contamination, raw send() flag derivation, sharded s* push-kind gap), each of which was a real desync the PR itself introduced and each of which was fixed in a follow-up commit (cee53da, 9fe94cf, c9d7c7f, defc764). That iteration history is exactly why a human should confirm the final state machine — the design is now sound as far as I can see, but the number of edge cases that surfaced during review is itself a signal that the routing logic deserves careful eyes.
Other factors
- All earlier inline comments (mine and CodeRabbit's) are resolved; the author responded to each with a targeted commit and test.
- The one remaining inline note from this run is a pure documentation nit (
(P)→(P|S)in five comment lines) with explicitly zero functional impact — not a reason to hold the PR. - One pre-existing follow-up was identified and intentionally deferred by the author with a clear rationale (case-sensitive
AUTO_PIPELINE_DISALLOWED_COMMANDSlookup for non-subscription commands via rawsend(); pre-existing on main, single-reply commands, no desync risk). - CI build #65595 has one failure (
v8-heap-snapshot.test.tsSIGKILL on a single Linux x64 shard) which is unrelated to anything this PR touches. - No CODEOWNERS entry for
valkey/redispaths; CodeRabbit suggested cirospaciari.
|
Independent report of bug 2 (in-flight commands surviving an auto-reconnect and cross-matching with the new connection's replies) came in today with this repro: Confirmed the fix here resolves it. A narrower standalone fix (just the |
|
CI status (rebased onto Build 68522: 284 test jobs passed, 0 failed, 0 error annotations. The one lane that has not reported is The only other annotation is a This PR changes only the Rust Valkey client ( Local verification on the rebased tree: The desync suite also passed on the |
|
Independently reproduced and root-caused bug 1 from a separate report of the same issue: a Branch with an equivalent fix plus one extra scenario, in case the test is useful to fold in: https://github.com/oven-sh/bun/tree/farm/42d71aaf/redis-resp3-push-desync The extra test covers the sibling case this PR also fixes but does not test: a subscriber connection receiving a push of an unrecognized kind previously went through |
|
Ran an extra round of verification of the subscription changes here against a real
A second scripted-server test written independently of this branch (multi-channel SUBSCRIBE and bare UNSUBSCRIBE, each pipelined with other commands) also passes on it. |
|
A fuzzing lead against Input: a RESP3 server that answers const srv = Bun.listen({ hostname: "127.0.0.1", port: 0, socket: { data(s, b) {
const t = b.toString();
if (t.includes("HELLO")) s.write("%3\r\n$6\r\nserver\r\n$5\r\nredis\r\n$5\r\nproto\r\n:3\r\n$7\r\nversion\r\n$5\r\n7.4.0\r\n");
else if (t.includes("SUBSCRIBE")) s.write(">3\r\n$11\r\nunsubscribe\r\n$2\r\nch\r\n:0\r\n");
}}});
const c = new Bun.RedisClient(`redis://127.0.0.1:${srv.port}`, { autoReconnect: false });
await c.connect();
await c.subscribe("ch", () => {});On current main (f789198):
I checked out this branch (28892c8) and re-ran the repro: no assertion failure and no state transition, because the One behavior question for this PR: with the push silently dropped, the pending |
|
Cross-checking from a separate report: Both are fixed by this PR. I built this branch and ran the two cases against a scripted RESP3 server:
So the One thing this PR leaves behind: the listener |
…e to the wrong command
A command's promise must only ever be fulfilled with that command's own
reply. Several independent paths in the Valkey client broke this.
- SUBSCRIBE with N > 1 channels produces N confirmation pushes but only
occupies one in-flight slot; confirmations 2..N consumed the slots of
whatever unrelated commands followed (ping() and get() both resolved
with the subscription count).
- In subscriber mode a '-ERR' reply to any command (e.g. -LOADING to a
PING) was routed through fail(), which rejected every other pending
command while leaving the erroring command's own promise unsettled.
- The pattern and sharded push kinds (psubscribe, ssubscribe, pmessage,
smessage, punsubscribe, sunsubscribe) were not recognized as
subscription confirmations, so a PSUBSCRIBE/SSUBSCRIBE resolved with
the raw RESP push frame instead of the subscribed-channel count.
- The raw client.send("SUBSCRIBE", [...]) escape hatch built its meta via
Meta::check, which never derived the subscription flags, so its
confirmation could not be paired against its own command.
- An argless UNSUBSCRIBE emits one confirmation per subscribed channel;
with a single undirected SUBSCRIPTION_REQUEST flag those extras could
be charged to a following SUBSCRIBE drained into the in-flight queue.
Push frames are now routed out-of-band unconditionally: the '>' type byte
alone makes them 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, and each such
request tracks how many confirmations it is owed. Meta::check derives
those flags from the command name (case-insensitively, covering the
plain, pattern and sharded families) so every entry point pairs the same
way, and clears auto-pipelining for any casing that matches. An error
reply in subscriber mode falls through to the same per-command rejection
path as non-subscriber mode. on_close also drops the read buffer and
reply scanner, which belong to the dead socket.
Out-of-band push routing and in-flight rejection on reconnect were fixed
independently in #33072; this keeps their behavior and replaces the
`is_reply_kind` prefix heuristic with explicit push-kind map entries,
which also covers the message kinds it did not.
`psubscribe()` now resolves with the subscribed-channel count like
`subscribe()` does, rather than the raw push frame; the assertion in
resp-nesting-depth.test.ts is updated accordingly.
Tested with a byte-scripted in-process RESP3 server so each case is
deterministic and needs no real Redis.
28892c8 to
db3de57
Compare
robobun
changed the title
|
Rebased onto #33072 independently fixed two of the bugs this PR covered, in a different shape:
Reconciled as follows:
One behavior change worth calling out: Verification after the rebase:
|
|
Complementary: #33479 fixes the other half of this failure mode. This PR stops the subscriber path from calling Different functions ( |
|
Triage note: #27861 was closed as fixed by #33072 (in-flight commands rejected on reconnect). With only this PR's test files applied to current main (165dc9f), 5 of the 9 tests in
Staying open for those. The branch currently conflicts with main and needs a rebase against the overlapping changes. |
1 participant
What
Fixes a set of reply-desynchronization bugs in
Bun.RedisClientwhere 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).
The bugs
Multi-channel
subscribe(["a","b","c"], cb)steals other commands' reply slotsSUBSCRIBEwith N channels emits N confirmation pushes but occupies one in-flight slot. The 2nd..Nth confirmations were consumed by whatever commands followed:An error reply in subscriber mode fails the whole connection instead of its own command
A
-LOADING(or any-ERR) reply toPINGon a subscriber connection went throughfail(), which rejected every other pending command and left the PING's own promise unsettled:Pattern and sharded subscription confirmations are not recognized
psubscribe,punsubscribe,pmessageand the Redis 7.0+ shardedssubscribe,sunsubscribe,smessagepush kinds were not in the push-kind map, soPSUBSCRIBE/SSUBSCRIBEresolved with the raw RESP push frame ({type, data}) instead of a channel count, andpmessagepayloads were never dispatched as[channel, message].Raw
client.send("SUBSCRIBE", [...])could not pair its confirmationThe 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
unsubscribeconfirmations could be charged to a queuedSUBSCRIBEAn argless
UNSUBSCRIBEemits one confirmation per currently-subscribed channel. With a single undirectedSUBSCRIPTION_REQUESTflag, the extras could decrement and resolve aSUBSCRIBEthat had been drained into the in-flight queue behind it.Fix
In
handle_response:>) are routed out-of-band unconditionally. The type byte alone marks them as unsolicited.SUBSCRIPTION_REQUESTis split intoSUBSCRIBE_REQUESTandUNSUBSCRIBE_REQUEST, so a confirmation only counts against an in-flight head of the matching direction. Each such request carriesremaining_replies(its channel count) and is only consumed once all its confirmations have arrived.invalidate, etc.) are dropped without consuming any reply slot.Errorreply 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 rawsend()hatch) pairs the same way. Any casing that matches also has auto-pipelining cleared, since the case-sensitiveAUTO_PIPELINE_DISALLOWED_COMMANDSlookup would otherwise miss it.In
SUBSCRIPTION_PUSH_MESSAGES: all nine push kinds are mapped explicitly. This makes #33072'sis_reply_kindprefix heuristic dead, so it is deleted; the explicit map additionally coverspmessage/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, likesubscribe()(declaredPromise<number>), instead of the raw RESP push frame.psubscribeis not in the public type declarations and the old value was a leaked protocol frame. The assertion inresp-nesting-depth.test.tsis updated; its load-bearing invariant (the pipelinedPINGstill receives its ownPONG) is unchanged.Verification
Without the
src/change, 7 of the 9 fail with the diffs shown above (the other 2 cover #33072's fixes and pass onmain). The full scripted-server Valkey suite is green:Fixes #27861