valkey: fix null array, null CRLF, big number and blob error replies

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 `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

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",

src/jsc/JSValue.rs

@@ -666,6 +666,23 @@
     pub fn from_uint64_no_truncate(global: &JSGlobalObject, i: u64) -> JsResult<JSValue> {
         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<Option<JSValue>> {
+        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 @@
     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,

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<const char*>(ptr), len })));
+}
+
 uint64_t JSC__JSValue__toUInt64NoTruncate(JSC::EncodedJSValue val)
 {
     JSC::JSValue value = JSC::JSValue::decode(val);

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,15 @@ pub(crate) fn resp_value_to_js_with_options(
 
             Ok(js_obj)
         }
+        // BigInt when the payload is an integer literal; modules and Lua can
+        // put anything after `(`, so other text stays a string.
         RESPValue::BigNumber(str) => {
-            // Try to parse as number if possible
-            if let Ok(int) = bun_core::fmt::parse_int::<i64>(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)
+            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)
         }
     }
 }

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;

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::<f64>().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)
+}

src/valkey/valkey_protocol.rs

@@ -88,6 +88,7 @@ impl RESPType {
 pub enum RESPValue {
     // RESP2 types
     SimpleString(Box<[u8]>),
+    /// A `-` simple error or a `!` blob error reply, holding the server's message.
     Error(Box<[u8]>),
     Integer(i64),
     BulkString(Option<Box<[u8]>>),
@@ -97,7 +98,6 @@ pub enum RESPValue {
     Null,
     Double(f64),
     Boolean(bool),
-    BlobError(Box<[u8]>),
     VerbatimString(VerbatimString),
     Map(Vec<MapEntry>),
     Set(Vec<RESPValue>),
@@ -134,7 +134,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 +397,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 +414,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 +443,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 +670,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 +835,139 @@ impl SubscriptionPushMessage {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn parse(frame: &[u8]) -> Result<RESPValue, RedisError> {
+        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<ScanResult, RedisError> {
+        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)
+        ));
+    }
+}