redis: route RESP3 replies on the frame an attribute decorates

[robobun]

What

A RESP3 attribute (|N ...) is out-of-band metadata that may legally prefix any reply or push; Redis and Valkey emit them for things like key-popularity hints. handle_response classified every frame by its outer RESPValue variant, so an attribute-decorated frame took the path of its decoration rather than the path of the reply it decorates.

The worst face is silent, permanent, cross-request data corruption: an attribute-decorated out-of-band push is consumed as the next command's reply, so get("a") resolves with the push object and every later reply on that connection is shifted by one, forever.

const attrPush = `|1\r\n$3\r\nttl\r\n:42\r\n` + `>3\r\n$7\r\nmessage\r\n$2\r\nch\r\n$2\r\nHI\r\n`;
// server writes `attrPush` once, before the first GET's reply

get(a) -> {"type":"message","data":["ch","HI"]}   // want "A"
get(b) -> "A"                                    // want "B"
get(c) -> "B"                                    // want "C"

Three more faces of the same misclassification, all against a conforming server:

decorated frame	before	after
subscribe confirmation	subscribe() resolves with the raw push object, client never enters subscriber mode, listener never fires	resolves with the channel count, messages dispatch
HELLO reply	handle_hello_response falls to its _ arm and fails the connection	handshake completes
-ERR ... reply	get() resolves with an Error object instead of rejecting	rejects
:1 reply to EXISTS	exists() returns 1, not true (the RETURN_AS_BOOL coercion matches on Integer)	returns true

No other client is a clean reference here: node-redis@5's decoder has no | case at all and
throws Unknown RESP type 124 "|" on these streams. Bun already parses attributes correctly; it
is only the dispatcher that mishandles them.

Cause

The RESP3 spec defines an attribute as auxiliary data the client should not consider part of the
reply it prefixes, and bun's own protocol_jsc.rs already implements that when converting to JS
("we just return the value and ignore attributes"). The dispatcher is the one place that never got
the memo. handle_response and everything it delegates to (handle_hello_response, the SELECT branch, should_consume_promise_pair, the subscriber branch, RETURN_AS_BOOL, the error check) match on the outer variant, which for a decorated frame is always Attribute.

Fix

Unwrap the decoration at the top of handle_response, before any dispatch, so the inner frame type decides the path:

// Everything below (HELLO, SELECT, push routing, promise pairing)
// dispatches on the variant, so strip any attribute decoration first.
value.unwrap_attributes();

RESPValue::unwrap_attributes replaces an attribute frame with the reply it decorates. Attributes nested inside aggregates are unaffected: resp_value_to_js_with_options already unwraps those recursively on the way to JS.

This is orthogonal to #32858, which routes RESPValue::Push out-of-band by type byte at the top level and so still misses Attribute(Push). Either can land first; the unwrap sits at the entry of handle_response, above all routing.

Verification

Six tests in test/js/valkey/reliability/protocol-handling.test.ts, driving a scripted RESP3 wire server (no real Redis needed). Five fail on main, one (attributes are transparent around the value they decorate) passes both ways as a guard.

$ bun bd test test/js/valkey/reliability/protocol-handling.test.ts
 6 pass  0 fail  (15 skipped, need Docker)

$ bun bd test test/js/valkey/
 25 pass  0 fail

Also smoke-tested end to end against a live redis-server: HELLO, SELECT, commands, EXISTS boolean coercion, error rejection, and subscribe/message/unsubscribe all behave as before.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 7 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: 48437d98-da6f-471b-be67-d9011ece9a2a

📥 Commits

Reviewing files that changed from the base of the PR and between 48ff9eb and 189dbf4.

📒 Files selected for processing (5)

• src/runtime/valkey_jsc/valkey.rs
• src/valkey/valkey_protocol.rs
• test/js/valkey/reliability/connection-failures.test.ts
• test/js/valkey/reliability/protocol-handling.test.ts
• test/js/valkey/test-utils.ts

---

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

[robobun]

^{Updated 2:08 PM PT - Jul 6th, 2026}

❌ @robobun, your commit 189dbf4 has some failures in Build #69113 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 33441

That installs a local version of the PR into your bun-33441 executable, so you can run:

bun-33441 --bun

[claude]

I didn't find any issues — the native change is minimal and safe Rust — but this alters the RESP3 response-dispatch path for the Redis client and adds a scripted wire-server test harness, so it's worth a human familiar with the valkey state machine confirming the routing semantics.

Extended reasoning...

Overview

The PR adds RESPValue::unwrap_attributes() (a ~6-line safe-Rust helper using core::mem::replace to peel off RESP3 |N attribute wrappers) and calls it once at the top of ValkeyClient::handle_response, so that HELLO/SELECT/push/promise-pair routing dispatches on the decorated inner frame rather than the Attribute variant. It also adds ~200 lines of tests in protocol-handling.test.ts, including a hand-rolled scripted RESP3 TCP server to exercise attribute-prefixed replies without a real Redis.

Security risks

None apparent. The helper is entirely safe Rust (no unsafe, no FFI); the attribute map's Vec<MapEntry> and the boxed placeholder Null drop normally when *self is overwritten, so there's no leak or double-free. The test server binds 127.0.0.1:0 and is torn down in finally.

Level of scrutiny

Medium-high. The code delta is tiny and obviously correct in isolation, but it sits at the entry of the Redis client's response dispatcher — the exact place where a subtle misclassification silently shifts every subsequent reply on the connection (which is precisely the bug being fixed). A maintainer who knows the RESP3 spec and this client's subscriber/HELLO/SELECT state machine should confirm that unconditionally stripping attributes before all routing is the intended semantics, and glance at the interaction with #32858 that the PR description calls out.

Other factors

The PR description is unusually thorough (mechanism, four failure modes, node-redis parity, verification against both the scripted server and a live redis-server). The new test harness is well-constructed (hermetic, port 0, cleanup in finally), but it's ~60 lines of new RESP command-frame parsing in JS that will live on as shared infrastructure — worth a second pair of eyes. No prior human reviews on the timeline.

[robobun]

Thanks, both points addressed.

Is unconditional stripping the intended semantics?

Yes, and bun had already decided so everywhere except the dispatcher. The RESP3 attribute type is auxiliary data that a client should not consider part of the reply it prefixes, and it may prefix any reply, pushes included. Concretely, in this codebase:

• the parser already models |N as a wrapper at any position (ValkeyReader::read_value_with_depth, RESPType::Attribute), and the incremental ReplyScanner already accounts for it (len * 2 + 1 children);
• protocol_jsc.rs already discards attributes on the way to JS ("we just return the value and ignore attributes");
• nothing in the client reads an attribute, and there is no API to surface one.

So handle_response was the single place still branching on the wrapper instead of what it wraps, and the unwrap just makes the dispatcher agree with the converter. It loses no information that any caller could observe. It is also the only dispatcher: both read paths in on_data (the stack fast path and the buffered path) funnel through handle_response, so there is no route that skips it.

Correction to my own description, re: node-redis

I originally wrote that node-redis@5 handles these byte streams correctly. That is wrong and I have fixed the description. Its decoder has no | case at all and throws Unknown RESP type 124 "|". When the attribute-decorated push and the following reply land in separate TCP segments it happens to resynchronize and return the right values; coalesce them into a single write and the command never resolves. So it is not a reference implementation here. Bun's parser is ahead of it: only the routing was wrong.

The duplicated frame reader

Fair, and it was worse than "new" infrastructure: it was a byte-for-byte copy of the reader already sitting in connection-failures.test.ts. Lifted both into test-utils.ts as readRespCommands in 8be2b2d (net -44 lines). The remaining countRespCommands in resp-nesting-depth.test.ts is a different function (it returns a count, not the parsed args) and I left it alone rather than widen the diff.

[claude]

LGTM — small, surgical unwrap of RESP3 attribute frames before dispatch, with hermetic mock-server tests covering each misrouting case.

Extended reasoning...

Overview

The native change is ~18 lines total: a new RESPValue::unwrap_attributes() helper in src/valkey/valkey_protocol.rs (a while let loop that swaps an Attribute frame out for the value it decorates via core::mem::replace), and a single call to it at the top of handle_response in src/runtime/valkey_jsc/valkey.rs. The test side extracts the existing readCommands RESP frame reader from connection-failures.test.ts into shared test-utils.ts as readRespCommands (byte-identical modulo while(true) → for(;;)), and adds six new tests in protocol-handling.test.ts driven by an in-process scripted mock server.

Security risks

None. This is protocol-parsing correctness in the Redis/Valkey client. No auth, crypto, permissions, or untrusted-input allocation paths are touched. The unwrap_attributes helper is pure safe Rust operating on already-parsed, already-bounds-checked RESPValue trees; the attribute payloads drop via normal Vec/Box Drop when *self is reassigned.

Level of scrutiny

Low-to-moderate. The change sits in the Redis client's response dispatcher, which is a hot path, but the edit is a single unwrap at the very top of handle_response before any variant-based routing. Non-decorated frames are entirely unaffected (the while let never enters). For decorated frames, protocol_jsc.rs:144-148 already discards attribute metadata on the way to JS ("we just return the value and ignore attributes"), so no user-visible information is lost — only the dispatch path is corrected. The RESP3 spec confirms attributes are out-of-band metadata that may prefix any reply including pushes, so unwrapping before dispatch is the correct interpretation.

Other factors

• No CODEOWNERS entries cover the touched files.
• No prior human review comments; timeline contains only bot noise.
• Bug hunter found no issues.
• Tests are hermetic (in-process net.createServer on port 0, autoReconnect: false, cleanup in finally, no Docker dependency for the new suite) and cover five distinct misrouting faces (push-as-reply desync, subscribe confirmation, HELLO handshake, error-reply resolve-vs-reject, EXISTS boolean coercion) plus one guard test for nested attributes inside aggregates.
• The readRespCommands extraction is a pure de-duplication refactor of test code that already existed in connection-failures.test.ts.

[claude]

LGTM — minimal, well-tested dispatcher fix; CI failures (cookie-map, terminal) are unrelated to valkey.

Extended reasoning...

Overview

The PR fixes RESP3 attribute-frame routing in the Valkey/Redis client. Native changes are tiny: a new RESPValue::unwrap_attributes() helper in src/valkey/valkey_protocol.rs (~6 substantive lines, safe Rust using core::mem::replace) and a single call to it at the top of handle_response in src/runtime/valkey_jsc/valkey.rs. Test changes add six hermetic tests driving a scripted in-process RESP3 server, and lift a duplicated command-frame reader out of connection-failures.test.ts into test-utils.ts (net code reduction).

Security risks

None. No auth, crypto, path handling, or untrusted-size arithmetic is touched. The helper is pure safe Rust; for every non-Attribute value the while let doesn't match and the call is a no-op, so existing reply routing is unchanged. For Attribute values, the prior behavior was demonstrably wrong (misrouted as the wrapper variant), and the new behavior matches what protocol_jsc.rs already does when converting to JS ("we just return the value and ignore attributes").

Level of scrutiny

Medium — this sits on the core response-dispatch path of a database client, but the change is a targeted no-op-for-the-common-case unwrap that only alters routing for a variant that was previously mishandled. The RESP3 spec defines attributes as auxiliary metadata not part of the reply, and Bun's own JS converter already implements that semantics; this just brings the dispatcher into agreement. The while loop correctly handles nested attribute decorations, and Drop handles cleanup of the discarded wrapper.

Other factors

• Six new tests cover every face of the bug described (out-of-band push, subscribe confirmation, HELLO, error reply, EXISTS boolean coercion, nested-in-aggregate), all against a local mock server so they run without Docker.
• No CODEOWNERS for valkey paths.
• CI failures are cookie-map.test.ts and terminal.test.ts, both unrelated to this diff (cookie-map churn is tracked by #33425 on main).
• The author already addressed earlier feedback by deduplicating the mock-server frame reader into test-utils.ts.
• No prior claude[bot] reviews on this PR.

[claude]

I didn't find any issues — the fix is small and well-tested — but since it changes the single dispatch point every RESP3 reply flows through and interacts with #32858's push routing, it's worth a human confirming the semantics and landing order.

Extended reasoning...

Overview

The PR fixes RESP3 attribute handling in Bun's Valkey/Redis client. A RESP3 attribute (|N) is out-of-band metadata that may prefix any reply; handle_response was dispatching on the outer Attribute variant instead of the frame it decorates, causing misrouting (attribute-decorated pushes consumed as command replies, decorated errors resolved instead of rejected, decorated HELLO failing the handshake, etc.).

The native change is minimal: a new RESPValue::unwrap_attributes() helper (6 lines, a while let + mem::replace) in src/valkey/valkey_protocol.rs, and one call to it at the top of handle_response in src/runtime/valkey_jsc/valkey.rs. Test changes add six hermetic tests driving a scripted in-process RESP3 server, and lift a duplicated readRespCommands frame reader from connection-failures.test.ts into test-utils.ts.

Security risks

None identified. The change strips a wrapper before variant dispatch; no new parsing of untrusted bytes, no allocation based on peer-controlled sizes, no auth/crypto surface. Memory is handled by Rust ownership — the discarded attribute map's Vec<MapEntry> drops automatically when *self = decorated overwrites the enum.

Level of scrutiny

Medium-high. handle_response is the single dispatch point every RESP3 frame from the server flows through — HELLO handshake, SELECT, push routing, promise pairing, error rejection, and RETURN_AS_BOOL coercion all branch on the variant this call now rewrites. The change is small and the reasoning (attributes are transparent per the RESP3 spec, and protocol_jsc.rs already discards them on the JS-conversion side) is sound, but a mistake here is silent cross-request data corruption in a user-facing runtime API.

Other factors

• The PR explicitly notes an interaction with #32858, which also touches top-level push routing and "still misses Attribute(Push)". A human should confirm the intended landing order / whether either needs rebasing.
• The author's follow-up comment addressed prior review feedback (dedup of the frame reader, semantics clarification, correcting the node-redis comparison), so there has already been active review discussion on this PR.
• Tests are hermetic (mock net.Server, no Docker), use port: 0, clean up in finally, and cover the full variant matrix (push, subscribe, HELLO, error, integer→bool, nested-in-aggregate). The readRespCommands extraction is a byte-for-byte move.
• No CODEOWNERS coverage for these paths.

[robobun]

On the #32858 interaction, I checked it rather than leave it open. Short version: the two are orthogonal, this one is still required after #32858 lands, and they compose in either order.

#32858 does not handle attributes. Its rewritten handle_response has no attribute unwrapping, and the three places that dispatch on the outer variant are unchanged:

self.handle_hello_response(value)?;          // Attribute(Map) -> falls to the `_` arm
return match value { RESPValue::SimpleString(..) => .. }   // SELECT
if let RESPValue::Push(push) = value { .. }  // still will not match Attribute(Push)

That last line is the one its description calls "the type byte alone marks them unsolicited" -- true for a bare >, but an attribute-decorated push never reaches it.

They compose cleanly. I cherry-picked both of my commits onto pull/32858/head in a scratch branch: no conflicts, and unwrap_attributes() lands at the top of the combined handle_response, above #32858's push routing:

1034:        value.unwrap_attributes();
1038:            self.handle_hello_response(value)?;
1083:        if let RESPValue::Push(push) = value {

cargo check -p bun_valkey -p bun_runtime passes on the combined tree. The two PRs touch disjoint regions of valkey_protocol.rs (mine adds RESPValue::unwrap_attributes next to the enum; #32858 edits the SUBSCRIPTION_PUSH_MESSAGES map and deletes is_reply_kind).

So there is no landing order to coordinate. Whichever merges second needs no more than a trivial rebase, and #32858 does not subsume this fix. I did not build or run #32858's test suite on the combined tree, only compile-checked it.

Separately, on the red CI: the two darwin 26 aarch64 - test-bun failures are buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun', so no test executed. The earlier protocol-handling.test.ts timeout on one lane was Docker failing to build the redis image on that agent (failed to prepare extraction snapshot ... parent snapshot does not exist), after which each live test burned ~31s retrying the connection and the file blew its 180s budget; it passed on retry in 6.09s. Neither is related to this diff.

[robobun]

CI status: the diff is green, two agents are sick

280 of 282 jobs pass. Both remaining failures are agent-level infrastructure, reproduced identically across two consecutive builds (69052, 69113), and neither executed a single test from this diff. I have used my one re-roll and will not push again; flagging the details so a maintainer can merge or go look at the agents.

1. darwin 26 aarch64 - test-bun (the 2 hard failures)

Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'.
Refusing to continue with a partial download (would silently fall back to the wrong binary).

Same agent host darwin-aarch64-26-5-1-1 both times. The job dies fetching the prebuilt binary, so no test runs. Not transient, which is why the re-roll did not clear it.

2. protocol-handling.test.ts timeout on 25.04 x64-baseline (flaky, passes on retry)

That agent's containerd image cache is corrupted, with the byte-identical parent snapshot in both builds:

failed to solve: failed to prepare extraction snapshot "extract-...":
parent snapshot sha256:731b1dbb27ec66e89077bcd8ab78aa1943461a8cccc29e4c1f7a3f835b5012af does not exist: not found

So docker build bun-redis-unified:local fails, setupDockerContainer() throws in beforeAll, and the file's pre-existing live tests then each burn ~31s hammering reconnects (RedisError: Max reconnection attempts reached, e.g. should handle RESP3 Map type (HGETALL) [31161.46ms]). Roughly 15 such tests at ~31s each exceeds the 180s per-file budget on their own, independent of this PR. The six mock-server tests added here contribute about 1s: on the retry, with Redis still unavailable and the live tests skipped, the whole file finishes in 6.09s.

To be explicit, since the timing-out file is the one this PR touches: the new tests cannot be the cause. They need no Docker, and the arithmetic above blows the budget without them.

Local verification on the rebased tree (48ff9eb + this diff):

$ bun bd test test/js/valkey/
 25 pass  0 fail  (1065 skipped, need Docker)

$ bun bd test test/js/bun/cookie/cookie-map.test.ts
 33 pass  0 fail

(The earlier cookie-map failures on 14/15 lanes were a stale merge base: it predated #33425, which fixed assertions that #32926 had invalidated. The rebase resolved those.)