Fix crash when RedisClient onclose/onconnect is set to a non-callable

src/runtime/valkey_jsc/js_valkey.rs

@@ -1275,7 +1275,9 @@ impl JSValkeyClient {
             };
             Js::hello_set_cached(this_value, &global_object, hello_value);
             // Call onConnect callback if defined by the user
-            if let Some(on_connect) = Js::onconnect_get_cached(this_value) {
+            if let Some(on_connect) =
+                Js::onconnect_get_cached(this_value).filter(|cb| cb.is_callable())
+            {
                 let js_value = this_value;
                 js_value.ensure_still_alive();
                 global_object.queue_microtask(on_connect, &[js_value, hello_value]);
@@ -1430,7 +1432,7 @@ impl JSValkeyClient {
         }
 
         // Call onClose callback if it exists
-        if let Some(on_close) = Js::onclose_get_cached(this_jsvalue) {
+        if let Some(on_close) = Js::onclose_get_cached(this_jsvalue).filter(|cb| cb.is_callable()) {
             on_close.call(&global_object, this_jsvalue, &[error_value])?;
         }
         Ok(())

test/js/valkey/valkey.test.ts

@@ -1,6 +1,6 @@
 import { randomUUIDv7, RedisClient, spawn } from "bun";
 import { beforeAll, beforeEach, describe, expect, test } from "bun:test";
-import { bunExe, bunRun } from "harness";
+import { bunEnv, bunExe, bunRun } from "harness";
 import { join } from "node:path";
 import {
   ctx as _ctx,
@@ -7579,3 +7579,125 @@ describe("RedisClient command methods", () => {
     expect(typeof RedisClient.prototype[name]).toBe("function");
   });
 });
+
+// These tests don't need a real Redis server: they use in-process TCP
+// listeners to drive the client's connect/close paths.
+describe.concurrent("RedisClient onclose/onconnect handler values", () => {
+  // Accepts the connection, then immediately closes it, so the client runs
+  // its close path (which invokes onclose) without a Redis server.
+  const closingServer = `
+    using server = Bun.listen({
+      hostname: "127.0.0.1",
+      port: 0,
+      socket: { open(s) { s.end(); }, data() {} },
+    });
+  `;
+
+  // Replies to HELLO with an empty RESP3 map, enough for the client to
+  // consider the connection established (which invokes onconnect).
+  const helloServer = `
+    using server = Bun.listen({
+      hostname: "127.0.0.1",
+      port: 0,
+      socket: { open() {}, data(s) { s.write("%0\\r\\n"); } },
+    });
+  `;
+
+  async function expectChildOk(code: string) {
+    await using proc = Bun.spawn({
+      cmd: [bunExe(), "-e", code],
+      env: bunEnv,
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    // stderr is only compared when the child failed, so the failure diff
+    // carries the crash output without requiring exactly-empty stderr on
+    // debug/ASAN builds (which may emit benign warnings).
+    expect({ stdout: stdout.trim(), exitCode, stderr: exitCode === 0 ? "" : stderr }).toEqual({
+      stdout: "ok",
+      exitCode: 0,
+      stderr: "",
+    });
+  }
+
+  test.each(["{}", "42", "null", "undefined", '"not a function"'])(
+    "assigning %s to onclose does not crash when the connection closes",
+    async value => {
+      await expectChildOk(`
+        ${closingServer}
+        const client = new Bun.RedisClient("redis://127.0.0.1:" + server.port, { maxRetries: 0 });
+        client.onclose = ${value};
+        try { await client.connect(); } catch {}
+        console.log("ok");
+      `);
+    },
+  );
+
+  test("assigning a non-callable to onconnect does not crash on successful connect", async () => {
+    await expectChildOk(`
+      ${helloServer}
+      const client = new Bun.RedisClient("redis://127.0.0.1:" + server.port);
+      client.onconnect = {};
+      await client.connect();
+      client.close();
+      console.log("ok");
+    `);
+  });
+
+  // https://github.com/oven-sh/bun/issues/29145
+  test("assigning null to onclose does not crash on a manual close() after connecting", async () => {
+    await expectChildOk(`
+      ${helloServer}
+      const client = new Bun.RedisClient("redis://127.0.0.1:" + server.port);
+      await client.connect();
+      client.onclose = null;
+      client.close();
+      console.log("ok");
+    `);
+  });
+
+  test("callable onclose still receives the connection error", async () => {
+    using server = Bun.listen({
+      hostname: "127.0.0.1",
+      port: 0,
+      socket: {
+        open(s) {
+          s.end();
+        },
+        data() {},
+      },
+    });
+    const client = new RedisClient(`redis://127.0.0.1:${server.port}`, { maxRetries: 0 });
+    try {
+      const { promise, resolve } = Promise.withResolvers<Error>();
+      client.onclose = resolve;
+      client.connect().catch(() => {});
+      expect(await promise).toBeInstanceOf(Error);
+    } finally {
+      client.close();
+    }
+  });
+
+  test("callable onconnect still fires after connecting", async () => {
+    using server = Bun.listen({
+      hostname: "127.0.0.1",
+      port: 0,
+      socket: {
+        open() {},
+        data(s) {
+          s.write("%0\r\n");
+        },
+      },
+    });
+    const client = new RedisClient(`redis://127.0.0.1:${server.port}`);
+    try {
+      const { promise, resolve } = Promise.withResolvers<void>();
+      client.onconnect = () => resolve();
+      await client.connect();
+      await promise;
+    } finally {
+      client.close();
+    }
+  });
+});