Skip to content

valkey/sql: skip non-callable onclose/onconnect instead of asserting #32249

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

valkey/sql: skip non-callable onclose/onconnect instead of asserting #32249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/runtime/valkey_jsc/js_valkey.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1280,7 +1280,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(|v| v.is_callable())
{
let js_value = this_value;
js_value.ensure_still_alive();
global_object.queue_microtask(on_connect, &[js_value, hello_value]);
Expand Down Expand Up @@ -1445,7 +1447,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(|v| v.is_callable()) {
if let Err(e) = on_close.call(&global_object, this_jsvalue, &[error_value]) {
global_object.report_active_exception_as_unhandled(e);
}
Expand All @@ -1470,7 +1472,7 @@ impl JSValkeyClient {
return;
};
let global_object = self.global_object;
if let Some(on_close) = Js::onclose_get_cached(this_value) {
if let Some(on_close) = Js::onclose_get_cached(this_value).filter(|v| v.is_callable()) {
let _exit = self.vm().enter_event_loop_scope();
if let Err(e) = on_close.call(&global_object, this_value, &[value]) {
global_object.report_active_exception_as_unhandled(e);
Expand Down
4 changes: 2 additions & 2 deletions src/sql_jsc/mysql/JSMySQLConnection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,7 @@ impl JSMySQLConnection {
return None;
}
if let Some(value) = self.js_value.get().try_get() {
return js::onconnect_take_cached(value, global_object);
return js::onconnect_take_cached(value, global_object).filter(|v| v.is_callable());
}
None
}
Expand All @@ -672,7 +672,7 @@ impl JSMySQLConnection {
return None;
}
if let Some(value) = self.js_value.get().try_get() {
return js::onclose_take_cached(value, global_object);
return js::onclose_take_cached(value, global_object).filter(|v| v.is_callable());
}
None
}
Expand Down
4 changes: 2 additions & 2 deletions src/sql_jsc/postgres/PostgresSQLConnection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3027,7 +3027,7 @@
debug!("consumeOnConnectCallback exists");

js::onconnect_set_cached(js_value, global_object, JSValue::ZERO);
Some(on_connect)
Some(on_connect).filter(|v| v.is_callable())
}

pub fn consume_on_close_callback(&self, global_object: &JSGlobalObject) -> Option<JSValue> {
Expand All @@ -3036,6 +3036,6 @@
let on_close = js::onclose_get_cached(js_value)?;
debug!("consumeOnCloseCallback exists");
js::onclose_set_cached(js_value, global_object, JSValue::ZERO);
Some(on_close)
Some(on_close).filter(|v| v.is_callable())

Check warning on line 3039 in src/sql_jsc/postgres/PostgresSQLConnection.rs

View check run for this annotation

Claude / Claude Code Review

No test coverage for Postgres/MySQL onclose/onconnect filters 

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 shi

@claude claude Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
32 changes: 31 additions & 1 deletion test/js/valkey/valkey.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -6901,3 +6901,33 @@
});
});
}

test("non-callable onclose/onconnect does not crash on connection failure", async () => {
const src = `
const { RedisClient } = Bun;
const bad = [{}, null, 123, "str", true, []];
const pending = [];
for (const v of bad) {
const client = new RedisClient("redis://127.0.0.1:1", { connectionTimeout: 50, maxRetries: 0 });
client.onclose = v;
client.onconnect = v;

Check warning on line 6913 in test/js/valkey/valkey.test.ts

View check run for this annotation

Claude / Claude Code Review

Test does not exercise the onconnect is_callable guard 

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.

@claude claude Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pending.push(client.connect().then(() => 0, () => 1));
}
Promise.all(pending).then(results => {
console.log("rejected " + results.reduce((a, b) => a + b, 0) + "/" + bad.length);
});
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", src],
env: bunEnv,
stderr: "pipe",
stdout: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout, stderr, signalCode: proc.signalCode, exitCode }).toEqual({
stdout: "rejected 6/6\n",
stderr: "",
signalCode: null,
exitCode: 0,
});
});
Loading