From 2c273d8f5275feb3d6a1919dadbcbd7dec400885 Mon Sep 17 00:00:00 2001 From: Alistair Smith Date: Tue, 18 Aug 2026 12:11:09 -0700 Subject: [PATCH 1/8] valkey: fix null array, null crlf, bignumber, blob error replies --- docs/runtime/redis.mdx | 3 +- scripts/rust-miri.ts | 1 + src/runtime/valkey_jsc/protocol_jsc.rs | 16 +- src/valkey/lib.rs | 3 + src/valkey/native_test_shims.rs | 16 ++ src/valkey/valkey_protocol.rs | 133 ++++++++- .../js/valkey/valkey-incremental-scan.test.ts | 269 +++++++++++++----- 7 files changed, 345 insertions(+), 96 deletions(-) create mode 100644 src/valkey/native_test_shims.rs diff --git a/docs/runtime/redis.mdx b/docs/runtime/redis.mdx index 653cb9f3c9db..7474df3cc338 100644 --- a/docs/runtime/redis.mdx +++ b/docs/runtime/redis.mdx @@ -340,8 +340,9 @@ The client automatically converts Redis responses to JavaScript values: - Integer responses are returned as JavaScript numbers - Bulk strings are returned as JavaScript strings - Simple strings are returned as JavaScript strings -- Null bulk strings are returned as `null` +- Null bulk strings and null arrays are returned as `null` - Array responses are returned as JavaScript arrays +- Big number responses (RESP3) are returned as JavaScript strings, so no digits are lost - Error responses throw JavaScript errors with appropriate error codes - Boolean responses (RESP3) are returned as JavaScript booleans - Map responses (RESP3) are returned as JavaScript objects diff --git a/scripts/rust-miri.ts b/scripts/rust-miri.ts index c493250970fd..9ba5e23745c7 100644 --- a/scripts/rust-miri.ts +++ b/scripts/rust-miri.ts @@ -41,6 +41,7 @@ const MIRI_CRATES = [ "bun_ast", "bun_paths", "bun_hash", + "bun_valkey", "bun_base64", "bun_clap", "bun_dispatch", diff --git a/src/runtime/valkey_jsc/protocol_jsc.rs b/src/runtime/valkey_jsc/protocol_jsc.rs index a936ad0cf7ed..c845079074a2 100644 --- a/src/runtime/valkey_jsc/protocol_jsc.rs +++ b/src/runtime/valkey_jsc/protocol_jsc.rs @@ -112,11 +112,6 @@ pub(crate) fn resp_value_to_js_with_options( RESPValue::Null => Ok(JSValue::NULL), RESPValue::Double(d) => Ok(JSValue::js_number(*d)), RESPValue::Boolean(b) => Ok(JSValue::from(*b)), - RESPValue::BlobError(str) => Ok(valkey_error_to_js( - global, - &**str, - RedisError::InvalidBlobError, - )), RESPValue::VerbatimString(verbatim) => { valkey_str_to_js_value(global, &mut verbatim.content, options) } @@ -158,14 +153,7 @@ pub(crate) fn resp_value_to_js_with_options( Ok(js_obj) } - RESPValue::BigNumber(str) => { - // Try to parse as number if possible - if let Ok(int) = bun_core::fmt::parse_int::(str, 10) { - Ok(JSValue::js_number(int as f64)) - } else { - // If it doesn't fit in an i64, return as string - bun_string_jsc::create_utf8_for_js(global, str) - } - } + // Always a string: an i64 that fits does not fit a JS number above 2^53. + RESPValue::BigNumber(str) => valkey_str_to_js_value(global, str, options), } } diff --git a/src/valkey/lib.rs b/src/valkey/lib.rs index ef4795961a65..0be0daed5203 100644 --- a/src/valkey/lib.rs +++ b/src/valkey/lib.rs @@ -1,3 +1,6 @@ #![allow(non_snake_case, non_camel_case_types, non_upper_case_globals)] #![warn(unused_must_use)] pub mod valkey_protocol; + +#[cfg(test)] +mod native_test_shims; diff --git a/src/valkey/native_test_shims.rs b/src/valkey/native_test_shims.rs new file mode 100644 index 000000000000..1d40aeb3756a --- /dev/null +++ b/src/valkey/native_test_shims.rs @@ -0,0 +1,16 @@ +//! Native symbols normally provided by Bun's C++ side, shimmed for this crate's +//! `cargo test` binary. Never compiled into the real build. + +/// `bun_core::fmt::parse_f64` calls WebKit's parser; the tests here never +/// parse a double, so a whole-buffer `str::parse` stands in for it. +#[unsafe(no_mangle)] +unsafe extern "C" fn WTF__parseDouble(bytes: *const u8, length: usize, counted: *mut usize) -> f64 { + // SAFETY: the caller passes a live `&[u8]` split into pointer and length. + let buf = unsafe { core::slice::from_raw_parts(bytes, length) }; + let parsed = core::str::from_utf8(buf) + .ok() + .and_then(|s| s.parse::().ok()); + // SAFETY: `counted` points at the caller's `usize` out-parameter. + unsafe { *counted = if parsed.is_some() { length } else { 0 } }; + parsed.unwrap_or(f64::NAN) +} diff --git a/src/valkey/valkey_protocol.rs b/src/valkey/valkey_protocol.rs index 1fe52ef4d5ee..072432fc98a1 100644 --- a/src/valkey/valkey_protocol.rs +++ b/src/valkey/valkey_protocol.rs @@ -88,6 +88,8 @@ impl RESPType { pub enum RESPValue { // RESP2 types SimpleString(Box<[u8]>), + /// A `-` simple error or `!` blob error reply. Both carry the server's + /// message and reject the command that produced them. Error(Box<[u8]>), Integer(i64), BulkString(Option>), @@ -97,7 +99,6 @@ pub enum RESPValue { Null, Double(f64), Boolean(bool), - BlobError(Box<[u8]>), VerbatimString(VerbatimString), Map(Vec), Set(Vec), @@ -134,7 +135,6 @@ impl fmt::Display for RESPValue { RESPValue::Null => writer.write_str("(nil)"), RESPValue::Double(d) => write!(writer, "{}", d), RESPValue::Boolean(b) => write!(writer, "{}", b), - RESPValue::BlobError(str) => write!(writer, "Error: {}", BStr::new(str)), RESPValue::VerbatimString(verbatim) => { write!( writer, @@ -398,7 +398,8 @@ impl<'a> ValkeyReader<'a> { } let len = self.read_integer()?; if len < 0 { - return Ok(RESPValue::Array(Vec::new())); + // RESP2 null array. + return Ok(RESPValue::Null); } let len = usize::try_from(len).expect("int cast"); let mut array = @@ -414,7 +415,9 @@ impl<'a> ValkeyReader<'a> { // RESP3 types RESPType::Null => { - let _ = self.read_until_crlf()?; // Read and discard CRLF + if !self.read_until_crlf()?.is_empty() { + return Err(RedisError::InvalidNull); + } Ok(RESPValue::Null) } RESPType::Double => { @@ -441,7 +444,7 @@ impl<'a> ValkeyReader<'a> { return Err(RedisError::InvalidBlobError); } let owned = Box::<[u8]>::from(str); - Ok(RESPValue::BlobError(owned)) + Ok(RESPValue::Error(owned)) } RESPType::VerbatimString => Ok(RESPValue::VerbatimString(self.read_verbatim_string()?)), RESPType::Map => { @@ -668,13 +671,18 @@ impl ReplyScanner { RESPType::SimpleString | RESPType::Error | RESPType::Integer - | RESPType::Null | RESPType::Double | RESPType::Boolean | RESPType::BigNumber => { let _ = reader.read_until_crlf()?; Ok(None) } + RESPType::Null => { + if !reader.read_until_crlf()?.is_empty() { + return Err(RedisError::InvalidNull); + } + Ok(None) + } RESPType::BulkString | RESPType::BlobError | RESPType::VerbatimString => { let invalid = match ty { RESPType::BlobError => RedisError::InvalidBlobError, @@ -828,3 +836,116 @@ impl SubscriptionPushMessage { } } } + +#[cfg(test)] +mod tests { + use super::*; + + fn parse(frame: &[u8]) -> Result { + let mut reader = ValkeyReader::init(frame); + let value = reader.read_value()?; + assert_eq!(reader.pos(), frame.len(), "frame not fully consumed"); + Ok(value) + } + + fn scan(frame: &[u8]) -> Result { + ReplyScanner::default().scan(frame) + } + + /// Every proper prefix of a complete frame must read as a short read in + /// both the tree parser and the scanner, never as an error or a value. + fn assert_prefixes_are_partial(frame: &[u8]) { + for i in 0..frame.len() { + let prefix = &frame[..i]; + assert!( + matches!(parse(prefix), Err(RedisError::InvalidResponse)), + "parser accepted or rejected prefix {:?}", + BStr::new(prefix) + ); + assert!( + matches!(scan(prefix), Ok(ScanResult::NeedMoreData)), + "scanner accepted or rejected prefix {:?}", + BStr::new(prefix) + ); + } + assert!(matches!(scan(frame), Ok(ScanResult::Complete))); + } + + #[test] + fn resp2_null_array_is_null() { + let frame = b"*-1\r\n"; + assert!(matches!(parse(frame), Ok(RESPValue::Null))); + assert_prefixes_are_partial(frame); + } + + #[test] + fn resp2_null_bulk_string_is_null() { + let frame = b"$-1\r\n"; + assert!(matches!(parse(frame), Ok(RESPValue::BulkString(None)))); + assert_prefixes_are_partial(frame); + } + + #[test] + fn resp3_null_requires_bare_crlf() { + let frame = b"_\r\n"; + assert!(matches!(parse(frame), Ok(RESPValue::Null))); + assert_prefixes_are_partial(frame); + + let junk = b"_junk\r\n"; + assert!(matches!(parse(junk), Err(RedisError::InvalidNull))); + assert!(matches!(scan(junk), Err(RedisError::InvalidNull))); + // Inside an aggregate the scanner must reject it too. + assert!(matches!( + scan(b"*2\r\n_junk\r\n_\r\n"), + Err(RedisError::InvalidNull) + )); + } + + #[test] + fn big_number_keeps_its_digits() { + for digits in [ + &b"9007199254740993"[..], + b"42", + b"-1", + b"3492890328409238509324850943850943825024385", + ] { + let mut frame = Vec::new(); + frame.push(b'('); + frame.extend_from_slice(digits); + frame.extend_from_slice(b"\r\n"); + match parse(&frame) { + Ok(RESPValue::BigNumber(value)) => assert_eq!(&*value, digits), + _ => panic!("expected BigNumber for {:?}", BStr::new(digits)), + } + assert_prefixes_are_partial(&frame); + } + } + + #[test] + fn simple_and_blob_errors_decode_alike() { + for (frame, text) in [ + ( + &b"-ERR unknown command\r\n"[..], + &b"ERR unknown command"[..], + ), + ( + b"!21\r\nSYNTAX invalid syntax\r\n", + b"SYNTAX invalid syntax", + ), + ] { + match parse(frame) { + Ok(RESPValue::Error(msg)) => assert_eq!(&*msg, text), + _ => panic!("expected Error for {:?}", BStr::new(frame)), + } + assert_prefixes_are_partial(frame); + } + assert!(matches!( + parse(b"!-1\r\n"), + Err(RedisError::InvalidBlobError) + )); + assert!(matches!( + scan(b"!-1\r\n"), + Err(RedisError::InvalidBlobError) + )); + } +} diff --git a/test/js/valkey/valkey-incremental-scan.test.ts b/test/js/valkey/valkey-incremental-scan.test.ts index 4cd4f306be9e..e29c5078f4aa 100644 --- a/test/js/valkey/valkey-incremental-scan.test.ts +++ b/test/js/valkey/valkey-incremental-scan.test.ts @@ -2,91 +2,206 @@ import { RedisClient, type TCPSocketListener } from "bun"; import { describe, expect, test } from "bun:test"; import net from "node:net"; -describe.concurrent("Valkey reply torn across socket reads", () => { - const CRLF = "\r\n"; - const bulk = (s: string) => `$${Buffer.byteLength(s)}${CRLF}${s}${CRLF}`; - // Minimal RESP3 HELLO map so the client enters the Connected state. - const HELLO = - `%3${CRLF}` + bulk("server") + bulk("redis") + bulk("proto") + `:3${CRLF}` + bulk("version") + bulk("7.4.0"); +const CRLF = "\r\n"; +const bulk = (s: string) => `$${Buffer.byteLength(s)}${CRLF}${s}${CRLF}`; +// Minimal RESP3 HELLO map so the client enters the Connected state. +const HELLO = + `%3${CRLF}` + bulk("server") + bulk("redis") + bulk("proto") + `:3${CRLF}` + bulk("version") + bulk("7.4.0"); - type PerSocket = { buf: Buffer; replied: boolean }; +type PerSocket = { buf: Buffer; replied: boolean }; - /** - * Mock server: answers HELLO, then answers the first GET with `reply` split at - * `splitAt` across two event-loop turns so the client's empty-read-buffer - * stack path sees a partial blob body. Subsequent commands get `+OK`. - */ - function createTornReplyServer(reply: string, splitAt: number): TCPSocketListener { - return Bun.listen({ - hostname: "127.0.0.1", - port: 0, - socket: { - open(s) { - s.data = { buf: Buffer.alloc(0), replied: false }; - }, - error() {}, - close() {}, - data(s, raw) { - const st = s.data; - st.buf = Buffer.concat([st.buf, raw]); - // Parse complete client RESP command frames (`*N\r\n($len\r\n...\r\n){N}`). - for (;;) { - const b = st.buf; - if (!b.length || b[0] !== 0x2a) break; - const headerEnd = b.indexOf(CRLF); - if (headerEnd < 0) break; - const argc = parseInt(b.subarray(1, headerEnd).toString("latin1"), 10); - let pos = headerEnd + 2; - const fields: string[] = []; - let complete = true; - for (let i = 0; i < argc; i++) { - const lenEnd = b.indexOf(CRLF, pos); - if (lenEnd < 0 || b[pos] !== 0x24) { - complete = false; - break; - } - const len = parseInt(b.subarray(pos + 1, lenEnd).toString("latin1"), 10); - const next = lenEnd + 2 + len + 2; - if (next > b.length) { - complete = false; - break; - } - fields.push(b.subarray(lenEnd + 2, lenEnd + 2 + len).toString("latin1")); - pos = next; +/** + * Mock server: answers HELLO, then answers the first GET with `reply`. When + * `splitAt` is inside the reply it is split there across two event-loop turns + * so the client's empty-read-buffer stack path sees a partial frame. + * Subsequent commands get `+OK`. + */ +function createReplyServer( + reply: string, + splitAt: number = reply.length, + hello: string = HELLO, +): TCPSocketListener { + return Bun.listen({ + hostname: "127.0.0.1", + port: 0, + socket: { + open(s) { + s.data = { buf: Buffer.alloc(0), replied: false }; + }, + error() {}, + close() {}, + data(s, raw) { + const st = s.data; + st.buf = Buffer.concat([st.buf, raw]); + // Parse complete client RESP command frames (`*N\r\n($len\r\n...\r\n){N}`). + for (;;) { + const b = st.buf; + if (!b.length || b[0] !== 0x2a) break; + const headerEnd = b.indexOf(CRLF); + if (headerEnd < 0) break; + const argc = parseInt(b.subarray(1, headerEnd).toString("latin1"), 10); + let pos = headerEnd + 2; + const fields: string[] = []; + let complete = true; + for (let i = 0; i < argc; i++) { + const lenEnd = b.indexOf(CRLF, pos); + if (lenEnd < 0 || b[pos] !== 0x24) { + complete = false; + break; + } + const len = parseInt(b.subarray(pos + 1, lenEnd).toString("latin1"), 10); + const next = lenEnd + 2 + len + 2; + if (next > b.length) { + complete = false; + break; } - if (!complete) break; - st.buf = b.subarray(pos); - const cmd = fields[0]?.toUpperCase(); - if (cmd === "HELLO") { - s.write(HELLO); - } else if (cmd === "GET" && !st.replied) { - st.replied = true; - s.write(reply.slice(0, splitAt)); - s.flush(); + fields.push(b.subarray(lenEnd + 2, lenEnd + 2 + len).toString("latin1")); + pos = next; + } + if (!complete) break; + st.buf = b.subarray(pos); + const cmd = fields[0]?.toUpperCase(); + if (cmd === "HELLO") { + s.write(hello); + } else if (cmd === "GET" && !st.replied) { + st.replied = true; + s.write(reply.slice(0, splitAt)); + s.flush(); + if (splitAt < reply.length) { // Yield twice so the first write reaches the client's `on_data` // before the second is sent. setImmediate(() => setImmediate(() => s.write(reply.slice(splitAt)))); - } else { - s.write(`+OK${CRLF}`); } + } else { + s.write(`+OK${CRLF}`); } - }, + } }, - }); + }, + }); +} + +async function withClient(server: TCPSocketListener, body: (client: RedisClient) => Promise) { + const client = new RedisClient(`redis://127.0.0.1:${server.port}`, { autoReconnect: false }); + client.onconnect = client.onclose = () => {}; + try { + await client.connect(); + return await body(client); + } finally { + client.close(); + server.stop(true); } +} - async function withClient(server: TCPSocketListener, body: (client: RedisClient) => Promise) { +describe.concurrent("Valkey reply decoding", () => { + test("RESP2 null array (*-1) resolves null", async () => { + const server = createReplyServer(`*-1${CRLF}`); + await withClient(server, async client => { + expect(await client.get("k")).toBeNull(); + expect(await client.send("PING", [])).toBe("OK"); + }); + }); + + test("RESP3 null (_) with trailing bytes is a protocol error", async () => { + const server = createReplyServer(`_junk${CRLF}`); + await withClient(server, async client => { + const err = await client.get("k").then( + () => null, + e => e, + ); + expect(err).toBeInstanceOf(Error); + expect(err.code).toBe("ERR_REDIS_INVALID_RESPONSE"); + }); + }); + + test.each([ + ["9007199254740993", "9007199254740993"], + ["42", "42"], + ])("BigNumber (%s) resolves the digits as a string", async (digits, expected) => { + const server = createReplyServer(`(${digits}${CRLF}`); + await withClient(server, async client => { + expect(await client.get("k")).toBe(expected); + }); + }); + + test("BigNumber resolves a Buffer for getBuffer", async () => { + const server = createReplyServer(`(9007199254740993${CRLF}`); + await withClient(server, async client => { + const value = await client.getBuffer("k"); + expect(value).toBeInstanceOf(Buffer); + expect(value!.toString()).toBe("9007199254740993"); + }); + }); + + test.each([ + ["-", `-NOAUTH nope!${CRLF}`], + ["!", `!12${CRLF}NOAUTH nope!${CRLF}`], + ])("error reply (%s) to HELLO rejects queued commands with the server text", async (_kind, hello) => { + const server = createReplyServer(`+OK${CRLF}`, undefined, hello); const client = new RedisClient(`redis://127.0.0.1:${server.port}`, { autoReconnect: false }); client.onconnect = client.onclose = () => {}; try { - await client.connect(); - return await body(client); + // Queued before the handshake finishes, so the rejection carries the + // HELLO error. connect() itself rejects with a generic "Connection closed". + const queued = client.get("k").then( + () => null, + e => e, + ); + await client.connect().catch(() => {}); + const err = await queued; + expect(err).toBeInstanceOf(Error); + expect(err.code).toBe("ERR_REDIS_AUTHENTICATION_FAILED"); + expect(err.message).toBe("NOAUTH nope!"); } finally { client.close(); server.stop(true); } - } + }); + + test.each([ + ["-", `-WRONGTYPE wrong kind${CRLF}`], + ["!", `!20${CRLF}WRONGTYPE wrong kind${CRLF}`], + ])("error reply (%s) nested in an array resolves as an Error element", async (_kind, element) => { + const server = createReplyServer(`*2${CRLF}+OK${CRLF}${element}`); + await withClient(server, async client => { + const result = (await client.get("k")) as unknown as [string, Error]; + expect(result[0]).toBe("OK"); + expect(result[1]).toBeInstanceOf(Error); + expect(result[1].message).toBe("WRONGTYPE wrong kind"); + expect(await client.send("PING", [])).toBe("OK"); + }); + }); + + test("simple error (-ERR) rejects with ERR_REDIS_INVALID_RESPONSE", async () => { + const server = createReplyServer(`-ERR unknown command${CRLF}`); + await withClient(server, async client => { + const err = await client.get("k").then( + () => null, + e => e, + ); + expect(err).toBeInstanceOf(Error); + expect(err.code).toBe("ERR_REDIS_INVALID_RESPONSE"); + expect(err.message).toBe("ERR unknown command"); + expect(await client.send("PING", [])).toBe("OK"); + }); + }); + test("blob error (!) rejects with ERR_REDIS_INVALID_RESPONSE and the server text", async () => { + const server = createReplyServer(`!21${CRLF}SYNTAX invalid syntax${CRLF}`); + await withClient(server, async client => { + const err = await client.get("k").then( + () => null, + e => e, + ); + expect(err).toBeInstanceOf(Error); + expect(err.code).toBe("ERR_REDIS_INVALID_RESPONSE"); + expect(err.message).toBe("SYNTAX invalid syntax"); + expect(await client.send("PING", [])).toBe("OK"); + }); + }); +}); + +describe.concurrent("Valkey reply torn across socket reads", () => { // `$15`/`=15` frames: 5-byte header, 15-byte body at [5,20), trailing CRLF at [20,22). // `!21` frame: 5-byte header, 21-byte body at [5,26), trailing CRLF at [26,28). // Offsets cover: body start, mid-body, last body byte, and mid-CRLF. @@ -94,7 +209,7 @@ describe.concurrent("Valkey reply torn across socket reads", () => { const LONG_SPLITS = [5, 10, 25, 27] as const; test.each(SHORT_SPLITS)("BulkString ($) torn at byte %i decodes (baseline)", async splitAt => { - const server = createTornReplyServer(`$15${CRLF}xxx:Some string${CRLF}`, splitAt); + const server = createReplyServer(`$15${CRLF}xxx:Some string${CRLF}`, splitAt); await withClient(server, async client => { expect(await client.get("k")).toBe("xxx:Some string"); expect(await client.send("PING", [])).toBe("OK"); @@ -104,7 +219,7 @@ describe.concurrent("Valkey reply torn across socket reads", () => { test.each(SHORT_SPLITS)( "VerbatimString (=) torn at byte %i decodes instead of failing the connection", async splitAt => { - const server = createTornReplyServer(`=15${CRLF}txt:Some string${CRLF}`, splitAt); + const server = createReplyServer(`=15${CRLF}txt:Some string${CRLF}`, splitAt); await withClient(server, async client => { expect(await client.get("k")).toBe("Some string"); expect(await client.send("PING", [])).toBe("OK"); @@ -113,14 +228,18 @@ describe.concurrent("Valkey reply torn across socket reads", () => { ); test.each(LONG_SPLITS)("BlobError (!) torn at byte %i decodes instead of failing the connection", async splitAt => { - const server = createTornReplyServer(`!21${CRLF}SYNTAX invalid syntax${CRLF}`, splitAt); + const server = createReplyServer(`!21${CRLF}SYNTAX invalid syntax${CRLF}`, splitAt); await withClient(server, async client => { - // A parsed BlobError resolves (not rejects) with an Error carrying the - // server's message. Before the fix this rejected with - // "Failed to read data (stack path)" and killed the connection. - const result = await client.get("k"); - expect(result).toBeInstanceOf(Error); - expect((result as unknown as Error).message).toBe("SYNTAX invalid syntax"); + // A parsed BlobError rejects only this command with the server's + // message. Before the fix this rejected with "Failed to read data + // (stack path)" and killed the connection. + const err = await client.get("k").then( + () => null, + e => e, + ); + expect(err).toBeInstanceOf(Error); + expect(err.code).toBe("ERR_REDIS_INVALID_RESPONSE"); + expect(err.message).toBe("SYNTAX invalid syntax"); expect(await client.send("PING", [])).toBe("OK"); }); }); From b45ccef042d4c41351a3eb392b5aab68752afeef Mon Sep 17 00:00:00 2001 From: Alistair Smith Date: Tue, 18 Aug 2026 21:25:13 -0700 Subject: [PATCH 2/8] valkey: cover nested null array and torn junk null --- src/valkey/valkey_protocol.rs | 23 +++++++++++++++++++ .../js/valkey/valkey-incremental-scan.test.ts | 8 +++++++ 2 files changed, 31 insertions(+) diff --git a/src/valkey/valkey_protocol.rs b/src/valkey/valkey_protocol.rs index 072432fc98a1..c5922003a71a 100644 --- a/src/valkey/valkey_protocol.rs +++ b/src/valkey/valkey_protocol.rs @@ -878,6 +878,21 @@ mod tests { assert_prefixes_are_partial(frame); } + #[test] + fn resp2_null_array_nested_in_array() { + // GEOPOS with a missing member: `[null, "abc"]`. + let frame = b"*2\r\n*-1\r\n$3\r\nabc\r\n"; + match parse(frame) { + Ok(RESPValue::Array(items)) => { + assert_eq!(items.len(), 2); + assert!(matches!(items[0], RESPValue::Null)); + assert!(matches!(&items[1], RESPValue::BulkString(Some(s)) if &**s == b"abc")); + } + _ => panic!("expected a two element array"), + } + assert_prefixes_are_partial(frame); + } + #[test] fn resp2_null_bulk_string_is_null() { let frame = b"$-1\r\n"; @@ -894,6 +909,14 @@ mod tests { let junk = b"_junk\r\n"; assert!(matches!(parse(junk), Err(RedisError::InvalidNull))); assert!(matches!(scan(junk), Err(RedisError::InvalidNull))); + // Until the CRLF arrives the junk is still a short read, not an error. + for i in 1..junk.len() - 1 { + assert!(matches!( + parse(&junk[..i]), + Err(RedisError::InvalidResponse) + )); + assert!(matches!(scan(&junk[..i]), Ok(ScanResult::NeedMoreData))); + } // Inside an aggregate the scanner must reject it too. assert!(matches!( scan(b"*2\r\n_junk\r\n_\r\n"), diff --git a/test/js/valkey/valkey-incremental-scan.test.ts b/test/js/valkey/valkey-incremental-scan.test.ts index e29c5078f4aa..c15ff11cb662 100644 --- a/test/js/valkey/valkey-incremental-scan.test.ts +++ b/test/js/valkey/valkey-incremental-scan.test.ts @@ -102,6 +102,14 @@ describe.concurrent("Valkey reply decoding", () => { }); }); + test("RESP2 null array nested in an array resolves a null element", async () => { + const server = createReplyServer(`*2${CRLF}*-1${CRLF}$3${CRLF}abc${CRLF}`); + await withClient(server, async client => { + expect(await client.get("k")).toEqual([null, "abc"]); + expect(await client.send("PING", [])).toBe("OK"); + }); + }); + test("RESP3 null (_) with trailing bytes is a protocol error", async () => { const server = createReplyServer(`_junk${CRLF}`); await withClient(server, async client => { From 40d525627428838ec9ebf71ace007a61f2800c6f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 19 Aug 2026 04:46:51 +0000 Subject: [PATCH 3/8] valkey: describe RESPValue::Error as the reply it holds A nested error element resolves as part of the outer value, so the variant doc no longer says the reply rejects a command. --- src/valkey/valkey_protocol.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/valkey/valkey_protocol.rs b/src/valkey/valkey_protocol.rs index c5922003a71a..9cac16342c97 100644 --- a/src/valkey/valkey_protocol.rs +++ b/src/valkey/valkey_protocol.rs @@ -88,8 +88,7 @@ impl RESPType { pub enum RESPValue { // RESP2 types SimpleString(Box<[u8]>), - /// A `-` simple error or `!` blob error reply. Both carry the server's - /// message and reject the command that produced them. + /// A `-` simple error or a `!` blob error reply, holding the server's message. Error(Box<[u8]>), Integer(i64), BulkString(Option>), From bbd98f892da7b91137642eabe75f7303858c546f Mon Sep 17 00:00:00 2001 From: Alistair Smith Date: Tue, 18 Aug 2026 23:02:49 -0700 Subject: [PATCH 4/8] valkey: decode resp3 big numbers as bigint --- docs/runtime/redis.mdx | 2 +- src/jsc/JSValue.rs | 22 +++++++++++++++++++ src/jsc/bindings/bindings.cpp | 7 ++++++ src/jsc/bindings/headers.h | 1 + src/runtime/valkey_jsc/protocol_jsc.rs | 12 ++++++++-- .../js/valkey/valkey-incremental-scan.test.ts | 18 +++++++++++---- 6 files changed, 55 insertions(+), 7 deletions(-) diff --git a/docs/runtime/redis.mdx b/docs/runtime/redis.mdx index 7474df3cc338..a6666883c4d9 100644 --- a/docs/runtime/redis.mdx +++ b/docs/runtime/redis.mdx @@ -342,7 +342,7 @@ The client automatically converts Redis responses to JavaScript values: - Simple strings are returned as JavaScript strings - Null bulk strings and null arrays are returned as `null` - Array responses are returned as JavaScript arrays -- Big number responses (RESP3) are returned as JavaScript strings, so no digits are lost +- Big number responses (RESP3) are returned as `BigInt` (a string when the payload is not an integer literal; a `Buffer` of the digits with `getBuffer`) - Error responses throw JavaScript errors with appropriate error codes - Boolean responses (RESP3) are returned as JavaScript booleans - Map responses (RESP3) are returned as JavaScript objects diff --git a/src/jsc/JSValue.rs b/src/jsc/JSValue.rs index e41e0015940c..7668c45daaa4 100644 --- a/src/jsc/JSValue.rs +++ b/src/jsc/JSValue.rs @@ -666,6 +666,23 @@ impl JSValue { pub fn from_uint64_no_truncate(global: &JSGlobalObject, i: u64) -> JsResult { host_fn::from_js_host_call(global, || JSC__JSValue__fromUInt64NoTruncate(global, i)) } + /// A BigInt from a decimal integer literal (optional `-`, then digits). + /// Returns `Ok(None)` when `digits` is not such a literal. + #[track_caller] + pub fn big_int_from_decimal( + global: &JSGlobalObject, + digits: &[u8], + ) -> JsResult> { + let unsigned = digits.strip_prefix(b"-").unwrap_or(digits); + if unsigned.is_empty() || !unsigned.iter().all(u8::is_ascii_digit) { + return Ok(None); + } + let value = host_fn::from_js_host_call(global, || { + // SAFETY: `digits` is a live slice for the duration of the call. + unsafe { JSC__JSValue__bigIntFromLatin1(global, digits.as_ptr(), digits.len()) } + })?; + Ok((!value.is_empty()).then_some(value)) + } /// `JSValue.fromTimevalNoTruncate` — encode a `struct timeval` /// as a BigInt (`sec * 1_000_000 + nsec`) without precision loss. May allocate /// a heap BigInt, so wrapped in `from_js_host_call` for exception checking. @@ -1984,6 +2001,11 @@ unsafe extern "C" { safe fn JSC__JSValue__dateInstanceFromNumber(global: &JSGlobalObject, n: f64) -> JSValue; safe fn JSC__JSValue__fromInt64NoTruncate(global: &JSGlobalObject, i: i64) -> JSValue; safe fn JSC__JSValue__fromUInt64NoTruncate(global: &JSGlobalObject, i: u64) -> JSValue; + fn JSC__JSValue__bigIntFromLatin1( + global: &JSGlobalObject, + ptr: *const u8, + len: usize, + ) -> JSValue; safe fn JSC__JSValue__fromTimevalNoTruncate( global: &JSGlobalObject, nsec: i64, diff --git a/src/jsc/bindings/bindings.cpp b/src/jsc/bindings/bindings.cpp index 248c256d432a..147f9a5ec434 100644 --- a/src/jsc/bindings/bindings.cpp +++ b/src/jsc/bindings/bindings.cpp @@ -4487,6 +4487,13 @@ JSC::EncodedJSValue JSC__JSValue__fromUInt64NoTruncate(JSC::JSGlobalObject* glob return JSC::JSValue::encode(JSC::JSBigInt::createFrom(globalObject, val)); } +// Decimal integer literal (Latin-1) -> BigInt. Returns the empty value when +// the text is not a valid StringToBigInt input. +JSC::EncodedJSValue JSC__JSValue__bigIntFromLatin1(JSC::JSGlobalObject* globalObject, const uint8_t* ptr, size_t len) +{ + return JSC::JSValue::encode(JSC::JSBigInt::stringToBigInt(globalObject, WTF::StringView(std::span { reinterpret_cast(ptr), len }))); +} + uint64_t JSC__JSValue__toUInt64NoTruncate(JSC::EncodedJSValue val) { JSC::JSValue value = JSC::JSValue::decode(val); diff --git a/src/jsc/bindings/headers.h b/src/jsc/bindings/headers.h index 13b241a473b1..8a06f69c54c7 100644 --- a/src/jsc/bindings/headers.h +++ b/src/jsc/bindings/headers.h @@ -225,6 +225,7 @@ CPP_DECL void JSC__JSValue__forEachPropertyOrdered(JSC::EncodedJSValue JSValue0, CPP_DECL JSC::EncodedJSValue JSC__JSValue__fromEntries(JSC::JSGlobalObject* arg0, ZigString* arg1, ZigString* arg2, size_t arg3, bool arg4); CPP_DECL JSC::EncodedJSValue JSC__JSValue__fromInt64NoTruncate(JSC::JSGlobalObject* arg0, int64_t arg1); CPP_DECL JSC::EncodedJSValue JSC__JSValue__fromUInt64NoTruncate(JSC::JSGlobalObject* arg0, uint64_t arg1); +CPP_DECL JSC::EncodedJSValue JSC__JSValue__bigIntFromLatin1(JSC::JSGlobalObject* arg0, const uint8_t* arg1, size_t arg2); CPP_DECL JSC::EncodedJSValue JSC__JSValue__fromTimevalNoTruncate(JSC::JSGlobalObject* arg0, int64_t nsec, int64_t sec); CPP_DECL JSC::EncodedJSValue JSC__JSValue__bigIntSum(JSC::JSGlobalObject* arg0, JSC::EncodedJSValue JSValue0, JSC::EncodedJSValue JSValue1); CPP_DECL void JSC__JSValue__getClassName(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* arg1, ZigString* arg2); diff --git a/src/runtime/valkey_jsc/protocol_jsc.rs b/src/runtime/valkey_jsc/protocol_jsc.rs index c845079074a2..d8c536c8319d 100644 --- a/src/runtime/valkey_jsc/protocol_jsc.rs +++ b/src/runtime/valkey_jsc/protocol_jsc.rs @@ -153,7 +153,15 @@ pub(crate) fn resp_value_to_js_with_options( Ok(js_obj) } - // Always a string: an i64 that fits does not fit a JS number above 2^53. - RESPValue::BigNumber(str) => valkey_str_to_js_value(global, str, options), + // BigInt when the payload is an integer literal; modules and Lua can + // put anything after `(`, so other text stays a string. + RESPValue::BigNumber(str) => { + if !options.return_as_buffer + && let Some(big) = JSValue::big_int_from_decimal(global, str)? + { + return Ok(big); + } + valkey_str_to_js_value(global, str, options) + } } } diff --git a/test/js/valkey/valkey-incremental-scan.test.ts b/test/js/valkey/valkey-incremental-scan.test.ts index c15ff11cb662..e4907a02151a 100644 --- a/test/js/valkey/valkey-incremental-scan.test.ts +++ b/test/js/valkey/valkey-incremental-scan.test.ts @@ -123,12 +123,22 @@ describe.concurrent("Valkey reply decoding", () => { }); test.each([ - ["9007199254740993", "9007199254740993"], - ["42", "42"], - ])("BigNumber (%s) resolves the digits as a string", async (digits, expected) => { + ["9007199254740993", 9007199254740993n], + ["-42", -42n], + ["340282366920938463463374607431768211456", 2n ** 128n], + ])("BigNumber (%s) resolves a BigInt", async (digits, expected) => { const server = createReplyServer(`(${digits}${CRLF}`); await withClient(server, async client => { - expect(await client.get("k")).toBe(expected); + const value = await client.get("k"); + expect(typeof value).toBe("bigint"); + expect(value).toBe(expected); + }); + }); + + test("BigNumber with a non-integer payload resolves the text as a string", async () => { + const server = createReplyServer(`(12abc${CRLF}`); + await withClient(server, async client => { + expect(await client.get("k")).toBe("12abc"); }); }); From 246c8a5f68ddb141516f3e6a64fc51874568e189 Mon Sep 17 00:00:00 2001 From: Alistair Smith Date: Tue, 18 Aug 2026 23:10:22 -0700 Subject: [PATCH 5/8] valkey: test the resp decoder through the binary only --- scripts/rust-miri.ts | 1 - src/valkey/lib.rs | 3 - src/valkey/native_test_shims.rs | 16 -- src/valkey/valkey_protocol.rs | 136 ---------------- .../js/valkey/valkey-incremental-scan.test.ts | 154 +++++++++--------- 5 files changed, 74 insertions(+), 236 deletions(-) delete mode 100644 src/valkey/native_test_shims.rs diff --git a/scripts/rust-miri.ts b/scripts/rust-miri.ts index 9ba5e23745c7..c493250970fd 100644 --- a/scripts/rust-miri.ts +++ b/scripts/rust-miri.ts @@ -41,7 +41,6 @@ const MIRI_CRATES = [ "bun_ast", "bun_paths", "bun_hash", - "bun_valkey", "bun_base64", "bun_clap", "bun_dispatch", diff --git a/src/valkey/lib.rs b/src/valkey/lib.rs index 0be0daed5203..ef4795961a65 100644 --- a/src/valkey/lib.rs +++ b/src/valkey/lib.rs @@ -1,6 +1,3 @@ #![allow(non_snake_case, non_camel_case_types, non_upper_case_globals)] #![warn(unused_must_use)] pub mod valkey_protocol; - -#[cfg(test)] -mod native_test_shims; diff --git a/src/valkey/native_test_shims.rs b/src/valkey/native_test_shims.rs deleted file mode 100644 index 1d40aeb3756a..000000000000 --- a/src/valkey/native_test_shims.rs +++ /dev/null @@ -1,16 +0,0 @@ -//! Native symbols normally provided by Bun's C++ side, shimmed for this crate's -//! `cargo test` binary. Never compiled into the real build. - -/// `bun_core::fmt::parse_f64` calls WebKit's parser; the tests here never -/// parse a double, so a whole-buffer `str::parse` stands in for it. -#[unsafe(no_mangle)] -unsafe extern "C" fn WTF__parseDouble(bytes: *const u8, length: usize, counted: *mut usize) -> f64 { - // SAFETY: the caller passes a live `&[u8]` split into pointer and length. - let buf = unsafe { core::slice::from_raw_parts(bytes, length) }; - let parsed = core::str::from_utf8(buf) - .ok() - .and_then(|s| s.parse::().ok()); - // SAFETY: `counted` points at the caller's `usize` out-parameter. - unsafe { *counted = if parsed.is_some() { length } else { 0 } }; - parsed.unwrap_or(f64::NAN) -} diff --git a/src/valkey/valkey_protocol.rs b/src/valkey/valkey_protocol.rs index 9cac16342c97..2c714c5d67d4 100644 --- a/src/valkey/valkey_protocol.rs +++ b/src/valkey/valkey_protocol.rs @@ -835,139 +835,3 @@ impl SubscriptionPushMessage { } } } - -#[cfg(test)] -mod tests { - use super::*; - - fn parse(frame: &[u8]) -> Result { - let mut reader = ValkeyReader::init(frame); - let value = reader.read_value()?; - assert_eq!(reader.pos(), frame.len(), "frame not fully consumed"); - Ok(value) - } - - fn scan(frame: &[u8]) -> Result { - ReplyScanner::default().scan(frame) - } - - /// Every proper prefix of a complete frame must read as a short read in - /// both the tree parser and the scanner, never as an error or a value. - fn assert_prefixes_are_partial(frame: &[u8]) { - for i in 0..frame.len() { - let prefix = &frame[..i]; - assert!( - matches!(parse(prefix), Err(RedisError::InvalidResponse)), - "parser accepted or rejected prefix {:?}", - BStr::new(prefix) - ); - assert!( - matches!(scan(prefix), Ok(ScanResult::NeedMoreData)), - "scanner accepted or rejected prefix {:?}", - BStr::new(prefix) - ); - } - assert!(matches!(scan(frame), Ok(ScanResult::Complete))); - } - - #[test] - fn resp2_null_array_is_null() { - let frame = b"*-1\r\n"; - assert!(matches!(parse(frame), Ok(RESPValue::Null))); - assert_prefixes_are_partial(frame); - } - - #[test] - fn resp2_null_array_nested_in_array() { - // GEOPOS with a missing member: `[null, "abc"]`. - let frame = b"*2\r\n*-1\r\n$3\r\nabc\r\n"; - match parse(frame) { - Ok(RESPValue::Array(items)) => { - assert_eq!(items.len(), 2); - assert!(matches!(items[0], RESPValue::Null)); - assert!(matches!(&items[1], RESPValue::BulkString(Some(s)) if &**s == b"abc")); - } - _ => panic!("expected a two element array"), - } - assert_prefixes_are_partial(frame); - } - - #[test] - fn resp2_null_bulk_string_is_null() { - let frame = b"$-1\r\n"; - assert!(matches!(parse(frame), Ok(RESPValue::BulkString(None)))); - assert_prefixes_are_partial(frame); - } - - #[test] - fn resp3_null_requires_bare_crlf() { - let frame = b"_\r\n"; - assert!(matches!(parse(frame), Ok(RESPValue::Null))); - assert_prefixes_are_partial(frame); - - let junk = b"_junk\r\n"; - assert!(matches!(parse(junk), Err(RedisError::InvalidNull))); - assert!(matches!(scan(junk), Err(RedisError::InvalidNull))); - // Until the CRLF arrives the junk is still a short read, not an error. - for i in 1..junk.len() - 1 { - assert!(matches!( - parse(&junk[..i]), - Err(RedisError::InvalidResponse) - )); - assert!(matches!(scan(&junk[..i]), Ok(ScanResult::NeedMoreData))); - } - // Inside an aggregate the scanner must reject it too. - assert!(matches!( - scan(b"*2\r\n_junk\r\n_\r\n"), - Err(RedisError::InvalidNull) - )); - } - - #[test] - fn big_number_keeps_its_digits() { - for digits in [ - &b"9007199254740993"[..], - b"42", - b"-1", - b"3492890328409238509324850943850943825024385", - ] { - let mut frame = Vec::new(); - frame.push(b'('); - frame.extend_from_slice(digits); - frame.extend_from_slice(b"\r\n"); - match parse(&frame) { - Ok(RESPValue::BigNumber(value)) => assert_eq!(&*value, digits), - _ => panic!("expected BigNumber for {:?}", BStr::new(digits)), - } - assert_prefixes_are_partial(&frame); - } - } - - #[test] - fn simple_and_blob_errors_decode_alike() { - for (frame, text) in [ - ( - &b"-ERR unknown command\r\n"[..], - &b"ERR unknown command"[..], - ), - ( - b"!21\r\nSYNTAX invalid syntax\r\n", - b"SYNTAX invalid syntax", - ), - ] { - match parse(frame) { - Ok(RESPValue::Error(msg)) => assert_eq!(&*msg, text), - _ => panic!("expected Error for {:?}", BStr::new(frame)), - } - assert_prefixes_are_partial(frame); - } - assert!(matches!( - parse(b"!-1\r\n"), - Err(RedisError::InvalidBlobError) - )); - assert!(matches!( - scan(b"!-1\r\n"), - Err(RedisError::InvalidBlobError) - )); - } -} diff --git a/test/js/valkey/valkey-incremental-scan.test.ts b/test/js/valkey/valkey-incremental-scan.test.ts index e4907a02151a..f765c56f5e1b 100644 --- a/test/js/valkey/valkey-incremental-scan.test.ts +++ b/test/js/valkey/valkey-incremental-scan.test.ts @@ -13,12 +13,13 @@ type PerSocket = { buf: Buffer; replied: boolean }; /** * Mock server: answers HELLO, then answers the first GET with `reply`. When * `splitAt` is inside the reply it is split there across two event-loop turns - * so the client's empty-read-buffer stack path sees a partial frame. + * so the client's empty-read-buffer stack path sees a partial frame; "bytes" + * sends one byte per turn so the reply scanner resumes at every offset. * Subsequent commands get `+OK`. */ function createReplyServer( reply: string, - splitAt: number = reply.length, + splitAt: number | "bytes" = reply.length, hello: string = HELLO, ): TCPSocketListener { return Bun.listen({ @@ -65,12 +66,23 @@ function createReplyServer( s.write(hello); } else if (cmd === "GET" && !st.replied) { st.replied = true; - s.write(reply.slice(0, splitAt)); - s.flush(); - if (splitAt < reply.length) { - // Yield twice so the first write reaches the client's `on_data` - // before the second is sent. - setImmediate(() => setImmediate(() => s.write(reply.slice(splitAt)))); + if (splitAt === "bytes") { + const bytes = Buffer.from(reply, "latin1"); + const writeByte = (i: number) => { + if (i >= bytes.length) return; + s.write(bytes.subarray(i, i + 1)); + s.flush(); + setImmediate(() => setImmediate(() => writeByte(i + 1))); + }; + writeByte(0); + } else { + s.write(reply.slice(0, splitAt)); + s.flush(); + if (splitAt < reply.length) { + // Yield twice so the first write reaches the client's `on_data` + // before the second is sent. + setImmediate(() => setImmediate(() => s.write(reply.slice(splitAt)))); + } } } else { s.write(`+OK${CRLF}`); @@ -93,56 +105,66 @@ async function withClient(server: TCPSocketListener, body: (client } } -describe.concurrent("Valkey reply decoding", () => { - test("RESP2 null array (*-1) resolves null", async () => { - const server = createReplyServer(`*-1${CRLF}`); - await withClient(server, async client => { - expect(await client.get("k")).toBeNull(); - expect(await client.send("PING", [])).toBe("OK"); - }); - }); +type Decoded = { value: unknown } | { rejects: { code: string; message?: string }; connectionFails?: boolean }; - test("RESP2 null array nested in an array resolves a null element", async () => { - const server = createReplyServer(`*2${CRLF}*-1${CRLF}$3${CRLF}abc${CRLF}`); - await withClient(server, async client => { - expect(await client.get("k")).toEqual([null, "abc"]); - expect(await client.send("PING", [])).toBe("OK"); - }); - }); +// One entry per RESP frame shape the decoder changed. Each is sent whole and +// one byte per socket read, so both the tree parser and the reply scanner see +// every torn prefix. +const FRAMES: [name: string, frame: string, expected: Decoded][] = [ + ["RESP2 null array (*-1)", `*-1${CRLF}`, { value: null }], + ["RESP2 null array nested in an array", `*2${CRLF}*-1${CRLF}$3${CRLF}abc${CRLF}`, { value: [null, "abc"] }], + ["RESP2 null bulk string ($-1)", `$-1${CRLF}`, { value: null }], + ["RESP3 null (_)", `_${CRLF}`, { value: null }], + [ + "RESP3 null with trailing bytes (_junk)", + `_junk${CRLF}`, + { rejects: { code: "ERR_REDIS_INVALID_RESPONSE" }, connectionFails: true }, + ], + ["big number above 2^53", `(9007199254740993${CRLF}`, { value: 9007199254740993n }], + ["negative big number", `(-42${CRLF}`, { value: -42n }], + ["big number above 2^64", `(340282366920938463463374607431768211456${CRLF}`, { value: 2n ** 128n }], + ["big number with a non-integer payload", `(12abc${CRLF}`, { value: "12abc" }], + [ + "simple error (-ERR)", + `-ERR unknown command${CRLF}`, + { rejects: { code: "ERR_REDIS_INVALID_RESPONSE", message: "ERR unknown command" } }, + ], + [ + "blob error (!)", + `!21${CRLF}SYNTAX invalid syntax${CRLF}`, + { rejects: { code: "ERR_REDIS_INVALID_RESPONSE", message: "SYNTAX invalid syntax" } }, + ], +]; - test("RESP3 null (_) with trailing bytes is a protocol error", async () => { - const server = createReplyServer(`_junk${CRLF}`); - await withClient(server, async client => { - const err = await client.get("k").then( - () => null, - e => e, +describe.concurrent.each([ + ["whole", (reply: string) => createReplyServer(reply)], + ["one byte per read", (reply: string) => createReplyServer(reply, "bytes")], +])("Valkey reply decoding, frame sent %s", (_mode, serve) => { + test.each(FRAMES)("%s", async (_name, frame, expected) => { + await withClient(serve(frame), async client => { + const outcome = await client.get("k").then( + value => ({ value }), + error => ({ error }), ); - expect(err).toBeInstanceOf(Error); - expect(err.code).toBe("ERR_REDIS_INVALID_RESPONSE"); - }); - }); - - test.each([ - ["9007199254740993", 9007199254740993n], - ["-42", -42n], - ["340282366920938463463374607431768211456", 2n ** 128n], - ])("BigNumber (%s) resolves a BigInt", async (digits, expected) => { - const server = createReplyServer(`(${digits}${CRLF}`); - await withClient(server, async client => { - const value = await client.get("k"); - expect(typeof value).toBe("bigint"); - expect(value).toBe(expected); - }); - }); - - test("BigNumber with a non-integer payload resolves the text as a string", async () => { - const server = createReplyServer(`(12abc${CRLF}`); - await withClient(server, async client => { - expect(await client.get("k")).toBe("12abc"); + if ("value" in expected) { + expect(outcome).toEqual({ value: expected.value }); + expect(typeof (outcome as { value: unknown }).value).toBe(typeof expected.value); + } else { + expect(outcome).toHaveProperty("error"); + const { error } = outcome as { error: Error & { code: string } }; + expect(error).toBeInstanceOf(Error); + expect(error.code).toBe(expected.rejects.code); + if (expected.rejects.message !== undefined) expect(error.message).toBe(expected.rejects.message); + } + if (!("connectionFails" in expected && expected.connectionFails)) { + expect(await client.send("PING", [])).toBe("OK"); + } }); }); +}); - test("BigNumber resolves a Buffer for getBuffer", async () => { +describe.concurrent("Valkey reply decoding", () => { + test("big number resolves a Buffer of the digits for getBuffer", async () => { const server = createReplyServer(`(9007199254740993${CRLF}`); await withClient(server, async client => { const value = await client.getBuffer("k"); @@ -189,34 +211,6 @@ describe.concurrent("Valkey reply decoding", () => { expect(await client.send("PING", [])).toBe("OK"); }); }); - - test("simple error (-ERR) rejects with ERR_REDIS_INVALID_RESPONSE", async () => { - const server = createReplyServer(`-ERR unknown command${CRLF}`); - await withClient(server, async client => { - const err = await client.get("k").then( - () => null, - e => e, - ); - expect(err).toBeInstanceOf(Error); - expect(err.code).toBe("ERR_REDIS_INVALID_RESPONSE"); - expect(err.message).toBe("ERR unknown command"); - expect(await client.send("PING", [])).toBe("OK"); - }); - }); - - test("blob error (!) rejects with ERR_REDIS_INVALID_RESPONSE and the server text", async () => { - const server = createReplyServer(`!21${CRLF}SYNTAX invalid syntax${CRLF}`); - await withClient(server, async client => { - const err = await client.get("k").then( - () => null, - e => e, - ); - expect(err).toBeInstanceOf(Error); - expect(err.code).toBe("ERR_REDIS_INVALID_RESPONSE"); - expect(err.message).toBe("SYNTAX invalid syntax"); - expect(await client.send("PING", [])).toBe("OK"); - }); - }); }); describe.concurrent("Valkey reply torn across socket reads", () => { From 502c84e83838c34410e4a704f10a121c92489954 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 19 Aug 2026 06:25:21 +0000 Subject: [PATCH 6/8] valkey: drop the dead empty check after the BigInt host call from_js_host_call already turns an empty result into an error, and the digit check before it admits only text that StringToBigInt parses, so the value after the call is never empty. --- src/jsc/JSValue.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/jsc/JSValue.rs b/src/jsc/JSValue.rs index 7668c45daaa4..f4e08f90c79c 100644 --- a/src/jsc/JSValue.rs +++ b/src/jsc/JSValue.rs @@ -677,11 +677,12 @@ impl JSValue { if unsigned.is_empty() || !unsigned.iter().all(u8::is_ascii_digit) { return Ok(None); } + // Only text StringToBigInt accepts gets here, so empty means it threw (too large). let value = host_fn::from_js_host_call(global, || { // SAFETY: `digits` is a live slice for the duration of the call. unsafe { JSC__JSValue__bigIntFromLatin1(global, digits.as_ptr(), digits.len()) } })?; - Ok((!value.is_empty()).then_some(value)) + Ok(Some(value)) } /// `JSValue.fromTimevalNoTruncate` — encode a `struct timeval` /// as a BigInt (`sec * 1_000_000 + nsec`) without precision loss. May allocate From c2dd44864d548795bdbf18f4982635badcc4d807 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 19 Aug 2026 06:26:31 +0000 Subject: [PATCH 7/8] docs(redis): describe the three forms a big number reply can take --- docs/runtime/redis.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/runtime/redis.mdx b/docs/runtime/redis.mdx index a6666883c4d9..19c870ed6230 100644 --- a/docs/runtime/redis.mdx +++ b/docs/runtime/redis.mdx @@ -342,7 +342,7 @@ The client automatically converts Redis responses to JavaScript values: - Simple strings are returned as JavaScript strings - Null bulk strings and null arrays are returned as `null` - Array responses are returned as JavaScript arrays -- Big number responses (RESP3) are returned as `BigInt` (a string when the payload is not an integer literal; a `Buffer` of the digits with `getBuffer`) +- Big number responses (RESP3) are returned as `BigInt`. A payload that is not an integer literal is returned as a string. `getBuffer` returns the payload as a `Buffer`. - Error responses throw JavaScript errors with appropriate error codes - Boolean responses (RESP3) are returned as JavaScript booleans - Map responses (RESP3) are returned as JavaScript objects From 90b59053275633490de62610721cb329320fd927 Mon Sep 17 00:00:00 2001 From: Alistair Smith Date: Tue, 18 Aug 2026 23:53:11 -0700 Subject: [PATCH 8/8] types: document how redis raw replies convert --- packages/bun-types/redis.d.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/packages/bun-types/redis.d.ts b/packages/bun-types/redis.d.ts index f254de647b23..03279e9e82e9 100644 --- a/packages/bun-types/redis.d.ts +++ b/packages/bun-types/redis.d.ts @@ -110,6 +110,19 @@ declare module "bun" { * @param command The command to send * @param args The arguments to the command * @returns A promise that resolves with the command result + * + * The reply is converted as follows. See the type conversion section in + * docs/runtime/redis.mdx. + * - A simple, bulk or verbatim string becomes a string. Methods that return a Buffer, such as getBuffer, keep the bytes. + * - An integer becomes a number. + * - A double becomes a number. + * - A big number becomes a bigint. When its payload is not an integer literal it becomes a string. + * - A boolean becomes a boolean. + * - A null, a null bulk string and a null array become null. + * - An array becomes an array. + * - A set becomes an array. + * - A map becomes a plain object with a null prototype. + * - An error reply (`-` or `!`) rejects the promise with code ERR_REDIS_INVALID_RESPONSE. */ send(command: string, args: string[]): Promise; @@ -3536,6 +3549,8 @@ declare module "bun" { * await redis.eval("return ARGV[1]", 0, "hello"); // "hello" * await redis.eval("return redis.call('GET', KEYS[1])", 1, "mykey"); * ``` + * + * The reply is converted the same way as for `send`. */ eval(script: string, numkeys: number, ...keysAndArgs: (string | number)[]): Promise; @@ -3551,6 +3566,8 @@ declare module "bun" { * const sha = await redis.script("LOAD", "return ARGV[1]"); * await redis.evalsha(sha, 0, "hello"); // "hello" * ``` + * + * The reply is converted the same way as for `send`. */ evalsha(sha1: string, numkeys: number, ...keysAndArgs: (string | number)[]): Promise; @@ -3574,6 +3591,8 @@ declare module "bun" { * @param numkeys The number of keys * @param keysAndArgs The keys followed by additional arguments * @returns Promise that resolves with the function's return value + * + * The reply is converted the same way as for `send`. */ fcall(name: string, numkeys: number, ...keysAndArgs: (string | number)[]): Promise;