[robobun]

Fuzzilli fingerprint: 3153c31c63a5fda1

Fixes #29145

What

RedisClient.onclose / .onconnect (and the equivalent properties on PostgresSQLConnection / MySQLConnection) are user-writable cached properties with no type validation at assignment time. The published type definitions document null as a valid value for both. When the connection closed or failed, the stored value was passed directly to JSValue::call() or queue_microtask(), both of which ASSERT(isCallable()) in debug builds. In release, Bun__JSValue__call returns an empty JSValue from the CallData::Type::None branch which then panics downstream with "A JavaScript exception was thrown, but it was cleared before it could be read."

Minimal repro (aborts on current main):

const client = new Bun.RedisClient("redis://127.0.0.1:1", { connectionTimeout: 50, maxRetries: 0 });
client.onclose = {};
client.connect().catch(() => {});

The #29145 repro (c.onclose = null; c.close() against a live server) hits the same path via on_valkey_close.

Fix

Filter the cached value through is_callable() at each call site so a non-function assignment (including null) is simply ignored. The same guard is applied to the Postgres and MySQL connection classes since they share the cached_prop_hostfns! setter and have identical call paths.

Test

Added a subprocess test in test/js/valkey/valkey.test.ts that sets onclose/onconnect to several non-callable values ({}, null, 123, "str", true, []) on clients pointed at an unroutable port, waits for all connect promises to reject, and asserts a clean exit. Fails on main with the jsObject.isCallable() assertion (debug) or a panic (release), passes with this change.

[robobun]

^{Updated 12:09 PM PT - Jun 14th, 2026}

❌ @robobun, your commit 28bc877 has 2 failures in Build #62300 (All Failures):

• scripts/build/ci.ts - build failed on 🐧 x64-musl-baseline - build-bun
• linker stderr: clang++: argument unused during compilation: '-no-pie' [-Wunused-command-line-argument] on 🐧 aarch64-android - build-rust
• linker stderr: clang++: argument unused during compilation: '-no-pie' [-Wunused-command-line-argument] on 🐧 x64-android - build-rust

---

🧪   To try this PR locally:

bunx bun-pr 32249

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

bun-32249 --bun

[github-actions]

Found 1 issue this PR may fix:

1. RedisClient: assigning null to onclose panics during close() #29145 - Assigning null to RedisClient.onclose panics during close() — exactly the non-callable callback crash this PR guards against with .is_callable() checks

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #29145

🤖 Generated with Claude Code

[coderabbitai]

Walkthrough

Adds .filter(|v| v.is_callable()) guards to onconnect and onclose cached-callback retrieval in the Valkey, MySQL, and Postgres JS connection clients, preventing non-callable values from being invoked. A new subprocess-based test verifies that setting non-callable values on RedisClient.onclose/onconnect does not crash the process on failed connections.

Changes

Callable guard for connection callbacks

Layer / File(s)	Summary
is_callable() guard in Valkey, MySQL, and Postgres callbacks src/runtime/valkey_jsc/js_valkey.rs, src/sql_jsc/mysql/JSMySQLConnection.rs, src/sql_jsc/postgres/PostgresSQLConnection.rs	onconnect and onclose cached-callback retrieval in all three clients now filters with .filter(|v| v.is_callable()), skipping non-callable values instead of attempting to invoke them. Postgres helpers still clear the cached slot to JSValue::ZERO regardless of callability.
Subprocess test for non-callable callback crash test/js/valkey/valkey.test.ts	Adds bunEnv to the harness import and a new test that spawns a Bun subprocess, assigns non-callable values to RedisClient.onclose and RedisClient.onconnect, initiates six failing connections, and asserts the process exits with code 0, empty stderr, and rejected 6/6 in stdout.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: adding callable filtering to onclose/onconnect handlers instead of asserting.
Description check	✅ Passed	The description provides both required sections: 'What' explains the bug and fix, and 'Test' details the verification approach. However, 'How did you verify your code works?' section is missing the explicit structure.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/sql_jsc/mysql/JSMySQLConnection.rs (1)

660-678: ⚠️ Potential issue | 🟡 Minor

Add non-callable onconnect/onclose regression test for MySQL and Postgres clients.

The callable filtering logic in JSMySQLConnection.rs is correct, but Valkey has a dedicated regression test for non-callable handler assignments (test/js/valkey/valkey.test.ts:6905: "non-callable onclose/onconnect does not crash on connection failure") that exercises edge cases like {}, null, numbers, strings, booleans, and arrays. MySQL and Postgres test suites mention these callbacks only with callable mocks—they lack equivalent coverage for the non-callable regression. Since the same fix was applied across all SQL clients, add equivalent regression tests for MySQL and Postgres to prevent the bug class from reappearing in those implementations.

🤖 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/sql_jsc/mysql/JSMySQLConnection.rs` around lines 660 - 678, Add
regression tests for MySQL and Postgres client test suites to cover non-callable
onconnect and onclose callback assignments, mirroring the existing Valkey
regression test at test/js/valkey/valkey.test.ts:6905. Create equivalent test
cases in the MySQL and Postgres test files that exercise the same edge cases
(empty objects, null, numbers, strings, booleans, and arrays) assigned to
onconnect and onclose callbacks to ensure the callable filtering logic in
consume_on_connect_callback and consume_on_close_callback methods prevents
crashes during connection failure scenarios. This ensures consistency of
regression test coverage across all SQL client implementations where the same
fix was applied.

🤖 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.

Outside diff comments:
In `@src/sql_jsc/mysql/JSMySQLConnection.rs`:
- Around line 660-678: Add regression tests for MySQL and Postgres client test
suites to cover non-callable onconnect and onclose callback assignments,
mirroring the existing Valkey regression test at
test/js/valkey/valkey.test.ts:6905. Create equivalent test cases in the MySQL
and Postgres test files that exercise the same edge cases (empty objects, null,
numbers, strings, booleans, and arrays) assigned to onconnect and onclose
callbacks to ensure the callable filtering logic in consume_on_connect_callback
and consume_on_close_callback methods prevents crashes during connection failure
scenarios. This ensures consistency of regression test coverage across all SQL
client implementations where the same fix was applied.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 953b664e-f8ac-4514-bf0b-0421c5d901a2

📥 Commits

Reviewing files that changed from the base of the PR and between 14bcc6c and 28bc877.

📒 Files selected for processing (4)

• src/runtime/valkey_jsc/js_valkey.rs
• src/sql_jsc/mysql/JSMySQLConnection.rs
• src/sql_jsc/postgres/PostgresSQLConnection.rs
• test/js/valkey/valkey.test.ts

[github-actions]

This PR may be a duplicate of:

1. Fix crash when RedisClient onclose/onconnect is set to a non-callable #32085 - Also fixes the crash when RedisClient onclose/onconnect is set to a non-callable value; valkey/sql: skip non-callable onclose/onconnect instead of asserting #32249 extends the same fix pattern to additionally cover MySQL and Postgres connections

🤖 Generated with Claude Code

[robobun]

Duplicate of #32085 which has the same Redis fix with more thorough tests (in-process TCP servers exercising both the close and connect paths, plus positive tests that callable handlers still fire). That PR also already covers #29145.

The Postgres/MySQL changes here are defensive only: those connection classes aren't exposed to user code (verified Bun.PostgresSQLConnection / Bun.MySQLConnection are undefined) and their onclose/onconnect slots are only written by internal JS in src/js/internal/sql/*.ts, which always passes bound methods. Closing in favor of #32085.

[claude]

LGTM — straightforward defensive .filter(is_callable) guard at 6 call sites with a regression test; the two inline notes are coverage nits, not correctness issues.

Extended reasoning...

Overview

This PR adds .filter(|v| v.is_callable()) to six call sites across js_valkey.rs, JSMySQLConnection.rs, and PostgresSQLConnection.rs where a cached onclose/onconnect JSValue is retrieved before being passed to call() or queue_microtask(). Without the filter, a user-assigned non-callable (e.g. client.onclose = {}) trips ASSERT(isCallable()) in debug or panics on an empty JSValue in release. A subprocess regression test in valkey.test.ts exercises the user-reachable Redis path against an unroutable port and asserts clean exit.

Security risks

None. The change narrows behavior (crash → silent no-op when the stored value isn't callable). No new inputs are accepted, no auth/crypto/permission code is touched, and the Postgres/MySQL hunks are defense-in-depth on internal classes already gated by $isCallable checks in the JS layer.

Level of scrutiny

Low. Each hunk is a one-token mechanical addition to an existing Option<JSValue> chain; the filter's only effect is turning Some(non_callable) into None, which every surrounding if let Some(...) already handles as the no-callback case. There are no control-flow, refcount, or lifetime changes. The fix is a fuzzer-found crash hardening with an obvious shape.

Other factors

The two findings posted inline are explicitly nits about test-coverage breadth (the onconnect guard isn't exercised on the failure path, and PG/MySQL aren't directly tested because their native handles aren't user-constructible). Both findings acknowledge the fix itself is correct and non-blocking. No CODEOWNERS apply to the touched files, and there are no outstanding human review comments.

[claude]

🟡 nit: The is_callable() filter is also applied to PostgresSQLConnection and JSMySQLConnection, but only RedisClient is tested. This is largely fine — those native connection objects aren't directly user-constructible and the public Bun.sql layer already rejects non-callable onclose/onconnect via $isCallable (src/js/internal/sql/shared.ts:1940-1949) — so the PG/MySQL hunks are defense-in-depth rather than a user-reachable fix. Mentioning per CLAUDE.md's "every behavioral change ships an automated test" guidance in case you want to add coverage, but not blocking.

Extended reasoning...

What this is

The PR applies the identical .filter(|v| v.is_callable()) guard to three classes — JSValkeyClient (RedisClient), PostgresSQLConnection, and JSMySQLConnection — but the regression test in test/js/valkey/valkey.test.ts only exercises RedisClient. CLAUDE.md:259 states "Every behavioral change ships an automated test in the same PR... even for obviously-correct one-liners", and CLAUDE.md:256 asks to "Cover the variant matrix... every sibling entry point receiving the same fix". On a literal reading, the Postgres/MySQL hunks are untested behavioral changes.

Why this is only a nit (addressing the refutation)

One reviewer argued this isn't a real coverage gap because the Postgres/MySQL paths aren't user-reachable, and that argument is largely correct:

1. Not user-constructible: JSMySQLConnection::constructor (and the Postgres equivalent) throws "MySQLConnection cannot be constructed directly". Unlike Bun.RedisClient, users cannot new these and assign .onclose = {}.
2. Public API already validates: The user-facing Bun.sql entry point checks $isCallable(onclose) / $isCallable(onconnect) at src/js/internal/sql/shared.ts:1940-1949 and throws ERR_INVALID_ARG_TYPE before any value reaches native code.
3. Native slots populated internally: The cached onconnect/onclose slots are written by createInstance from arguments[9]/arguments[10], which come from Bun's internal pool wrapper (src/js/internal/sql/{mysql,postgres}.ts), not directly from user input.
4. Handle not exposed: The native connection object lives on the internal PooledConnection wrapper; it is not surfaced through the public sql() interface, so there's no documented way for user code to reach the cached_prop_hostfns!-generated .onclose setter.

Given (1)–(4), the Postgres/MySQL change is defense-in-depth guarding an internal invariant, not a fix for a user-reachable crash. CLAUDE.md's "sibling entry point" examples ("CLI flag AND JS API; direct AND proxy") enumerate user-facing variants; internal implementation classes that share a code pattern are a weaker fit for that rule.

Why it's still worth a mention

The same CLAUDE.md:259 paragraph also says "never add NEW production code solely to make a test writable", which cuts against fabricating internal access just to test this guard. However, the native classes do expose writable .onclose/.onconnect setters via cached_prop_hostfns! (JSMySQLConnection.rs:592-596, and the equivalent block in PostgresSQLConnection.rs / sql.classes.ts), so if any future code path hands a native connection reference to user code — or if the $isCallable guard in shared.ts is ever removed — the only thing preventing the original assertion crash on those adapters is this one-line filter, with no test pinning it. The Redis test does exercise the identical .filter(|v| v.is_callable()) pattern, which gives reasonable confidence the mechanical guard works, but it wouldn't catch an independent revert of the PG/MySQL hunks.

Step-by-step: why the PG/MySQL crash is not reachable today

1. User calls Bun.sql({ adapter: 'postgres', onclose: {} }).
2. src/js/internal/sql/shared.ts:1946-1949 runs if (!$isCallable(onclose)) throw $ERR_INVALID_ARG_TYPE(...) → throws before any native connection is created.
3. If the user instead passes a valid function (or omits onclose), the internal pool creates the native connection via createInstance(...) with a known-callable internal wrapper as arguments[10].
4. The native PostgresSQLConnection/MySQLConnection object is stored on the internal PooledConnection; the user never receives a reference to it, so cannot call its native .onclose setter with a non-callable.
5. Therefore consume_on_close_callback only ever sees callable values from the public API, and the new .filter(|v| v.is_callable()) is belt-and-suspenders.

Contrast with Redis: new Bun.RedisClient(url) is public, and client.onclose = {} writes directly to the native cached slot with no JS-side validation — that path is user-reachable, which is why the fuzzer found it there and why the test correctly targets it.

Suggested resolution

Non-blocking. If a Postgres/MySQL container fixture is already wired into the test suite (the repo has both under test/js/sql/), a small test that obtains the internal connection handle (if an existing bun:internal-for-testing surface exposes it) and sets .onclose = {} would close the gap. If no such surface exists, it's reasonable to leave this as-is — adding a test-only hook just for this would itself violate CLAUDE.md:259's "fault-injection flags and test-only hooks get the PR blocked" clause, and the Redis test already pins the shared filter pattern.

[claude]

🟡 This line doesn't actually exercise the new onconnect guard — on_valkey_connect() only runs after a successful connection, and every client here points at 127.0.0.1:1 and fails. Removing .filter(|v| v.is_callable()) from js_valkey.rs:1283 wouldn't break this test. Consider either dropping this assignment (the test title is already scoped to 'on connection failure') or adding a separate success-path case against the live-server fixture to cover the onconnect filter.

Extended reasoning...

What the issue is

The new test sets client.onconnect = v for each non-callable value, suggesting it covers the new is_callable() guard added to the onconnect path. But it doesn't: the onconnect callback is invoked from on_valkey_connect() (src/runtime/valkey_jsc/js_valkey.rs:1283), which only runs after a successful HELLO handshake on a connected socket. Every client in this test connects to redis://127.0.0.1:1 with maxRetries: 0, so every connect() rejects and only the onclose paths (js_valkey.rs:1450 in on_valkey_close and js_valkey.rs:1475 in fail_with_js_value) ever execute the new filter.

Code path walkthrough

Concretely, for each v in bad:

1. new RedisClient("redis://127.0.0.1:1", { connectionTimeout: 50, maxRetries: 0 }) is constructed and onconnect = v / onclose = v are assigned via the cached_prop_hostfns! setters (no validation).
2. client.connect() → do_connect() → connect() opens a TCP socket to port 1, which refuses the connection.
3. uWS dispatches SocketHandler::on_connect_error → ValkeyClient::on_close() → with max_retries: 0, this lands in on_valkey_close() (js_valkey.rs:1417), which reads Js::onclose_get_cached(...) and now filters by is_callable(). ✅ Covered.
4. on_valkey_connect() is never entered because status never reaches Connected — so Js::onconnect_get_cached(this_value).filter(|v| v.is_callable()) at line 1283 is never evaluated.

Why this matters

CLAUDE.md says: "Confirm deleting each load-bearing clause of your fix breaks at least one test." Reverting the .filter(|v| v.is_callable()) on line 1283 would not break this test (or any other test added in this PR). The PR description says the test "sets onclose/onconnect to several non-callable values… and asserts a clean exit", which reads as if both guards are exercised — but only onclose actually is.

That said, the test name explicitly scopes itself to "on connection failure", which is accurate; the onconnect change is a one-line guard structurally identical to the tested onclose guard; and testing the success path requires a live Valkey server (the file already has ctx.redis fixtures for this). So this is a minor coverage gap, not a correctness problem with the fix.

How to fix

Two options:

• Drop the misleading line. Remove client.onconnect = v; from the subprocess script so the test doesn't imply coverage it doesn't provide. The test title already correctly scopes to connection failure.
• Add a success-path case. In the describe.each block that has access to ctx.redis (a real server), add a small test that does client.onconnect = {}, awaits client.connect(), and asserts no crash. That would actually exercise on_valkey_connect()'s new filter and satisfy the "deleting the clause breaks a test" criterion for line 1283.