[robobun]

What does this PR do?

Fixes #29145

Fixes a crash found by fuzzing (fingerprint 2aea0593fbc3536c). The same defect was independently reported in #29145 (client.onclose = null panics on the next close() with "A JavaScript exception was thrown, but it was cleared before it could be read").

RedisClient.prototype.onclose and .onconnect store whatever value is assigned (the generated cached-property setter does no validation), but the native close and connect paths invoked the stored value unconditionally:

• on_valkey_close called onclose via JSValue::call, which hits ASSERTION FAILED: Function passed to .call must be callable. in Bun__JSValue__call (bindings.cpp:2985) on debug builds, and crashes release builds in the error-reporting path that follows the empty return.
• on_valkey_connect passed onconnect to queue_microtask, which hits ASSERTION FAILED: queueMicrotask must be called with an async context frame or a callable. (bindings.cpp:5708) on debug builds.

Any non-callable triggers it, including null, which the published types (packages/bun-types/redis.d.ts) explicitly allow for clearing the handler:

const client = new Bun.RedisClient("redis://127.0.0.1:6399", { maxRetries: 0 });
client.onclose = null; // or {}, 42, undefined, "str"
client.connect().catch(() => {});
// aborts when the connection closes

The crash fires whenever the connection closes or connects, which can be seconds after the assignment (for example the 10s connection timeout), so in the fuzzer's REPRL harness it often landed while an unrelated later script was executing. That is why the minimized reproducer looked unrelated and the crash was flagged flaky.

The fix guards the invocation sites in src/runtime/valkey_jsc/js_valkey.rs so only callable values are invoked, matching how event-handler properties behave elsewhere. Setter behavior is unchanged (values still round-trip through the getter).

Rebase note: this PR originally guarded three sites. #39513 removed the third one (JSValkeyClient::fail_with_js_value, which also invoked onclose from the reconnect error path) and routes that path through on_valkey_close instead, so after the rebase onto current main the diff is the two remaining sites, on_valkey_connect and on_valkey_close, which are the only reads of these slots left in the crate.

The same cached onclose/onconnect pattern exists in the Postgres and MySQL connection classes, but those slots are only written by internal JS (src/js/internal/sql/*.ts) which always passes its own bound methods, and the native connection objects are not reachable from user code, so they are left unchanged.

How did you verify your code works?

• test/js/valkey/valkey.test.ts gains a RedisClient onclose/onconnect handler values block that needs no Redis server (in-process TCP listeners drive the close and connect paths). The non-callable cases spawn subprocesses since the bug is a process abort.
• Fails on the unfixed build: the five onclose cases crash both the release build (USE_SYSTEM_BUN=1 bun test: 5 fail, children die with exit 132) and debug builds; the onconnect case aborts unfixed debug builds with the queueMicrotask assertion (verified by stashing the fix and rebuilding).
• Passes with bun bd test test/js/valkey/valkey.test.ts -t "handler values" (8 pass).
• The original fuzzer script runs to completion on the fixed debug build.
• Positive tests confirm callable onclose still receives the connection error and callable onconnect still fires.
• The repro from RedisClient: assigning null to onclose panics during close() #29145 (assign null, then a manual close() on a connected client) is covered by a dedicated test; it crashes the unfixed release build with exit 132 and passes with this fix.

---

[review] gate passed · iteration 4 · 2 files touched

fails on main (without fix)

ASAN without fix: 7 failed, 992 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/valkey/valkey.test.ts
bun test v1.4.0 (8326d1bd3)

test/js/valkey/valkey.test.ts:
failed to connect to the docker API at unix:///var/run/docker.sock; check if the path is correct and if the daemon is running: dial unix /var/run/docker.sock: connect: no such file or directory
Redis is not enabled, skipping tests
(skip) Valkey Redis Client (tls) > (unnamed)
(skip) Valkey Redis Client (tls) > Basic Operations > should keep process alive when connecting
(skip) Valkey Redis Client (tls) > Basic Operations > should set and get strings
(skip) Valkey Redis Client (tls) > Basic Operations > should test key existence
(skip) Valkey Redis Client (tls) > Basic Operations > should increment and decrement counters
(skip) Valkey Redis Client (tls) > Basic Operations > should increment by specified amount with INCRBY
(skip) Valkey Redis Client (tls) > Basic Operations > should increment by float amount with INCRBYFLOAT
(skip) Valkey Redis Client (tls) > Basic Operations > should decrement by specified amount with DECRBY
(skip) Valkey Redis Clie
... (truncated)

release without fix: 52 failed, 992 skipped
bun test v1.4.0-canary.1 (8326d1bd3)

test/js/valkey/valkey.test.ts:
failed to connect to the docker API at unix:///var/run/docker.sock; check if the path is correct and if the daemon is running: dial unix /var/run/docker.sock: connect: no such file or directory
Redis is not enabled, skipping tests
(skip) Valkey Redis Client (tls) > (unnamed)
(skip) Valkey Redis Client (tls) > Basic Operations > should keep process alive when connecting
(skip) Valkey Redis Client (tls) > Basic Operations > should set and get strings
(skip) Valkey Redis Client (tls) > Basic Operations > should test key existence
(skip) Valkey Redis Client (tls) > Basic Operations > should increment and decrement counters
(skip) Valkey Redis Client (tls) > Basic Operations > should increment by specified amount with INCRBY
(skip) Valkey Redis Client (tls) > Basic Operations > should increment by float amount with INCRBYFLOAT
(skip) Valkey Redis Client (tls) > Basic Operations > should decrement by specified amount with DECRBY
(skip) Valkey Redis Client (tls) > Basic Operations > should rename a key with RENAME
(skip) Valkey Redis Client (tls) > Basic Operations > should rename a key with RENAME overwr
... (truncated)

passes on PR (with fix)

ASAN with fix: 992 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/valkey/valkey.test.ts
bun test v1.4.0 (8326d1bd3)

test/js/valkey/valkey.test.ts:
failed to connect to the docker API at unix:///var/run/docker.sock; check if the path is correct and if the daemon is running: dial unix /var/run/docker.sock: connect: no such file or directory
Redis is not enabled, skipping tests
(skip) Valkey Redis Client (tls) > (unnamed)
(skip) Valkey Redis Client (tls) > Basic Operations > should keep process alive when connecting
(skip) Valkey Redis Client (tls) > Basic Operations > should set and get strings
(skip) Valkey Redis Client (tls) > Basic Operations > should test key existence
(skip) Valkey Redis Client (tls) > Basic Operations > should increment and decrement counters
(skip) Valkey Redis Client (tls) > Basic Operations > should increment by specified amount with INCRBY
(skip) Valkey Redis Client (tls) > Basic Operations > should increment by float amount with INCRBYFLOAT
(skip) Valkey Redis Client (tls) > Basic Operations > should decrement by specified amount with DECRBY
(skip) Valkey Redis Clie
... (truncated)

release with fix: 992 skipped
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     892f065edb
  features     baseline

22 deps, 120 codegen, 1144 objects in 733ms

ninja: Entering directory `/workspace/bun/build/release'
[1/1206] install /workspace/bun
bun install v1.4.0-canary.1 (8326d1bd3)

Checked 26 installs across 63 packages (no changes) [12.00ms]
[2/1206] gen ErrorCode+*.h
[3/1206] install /workspace/bun/packages/bun-error
bun install v1.4.0-canary.1 (8326d1bd3)

Checked 1 install across 2 packages (no changes) [1.00ms]
[4/1206] gen bindgenv2
[5/1206] install /workspace/bun/src/node-fallbacks
bun install v1.4.0-canary.1 (8326d1bd3)

Checked 111 installs across 104 packages (no changes) [18.00ms]
[6/1206] fetch tinycc
[tinycc] up to date
[7/1205] fetch zlib
[zlib] up to date
[8/1205] fetch libjpeg-turbo
[libjpeg-turbo] up to date
[9/1205] gen .bind.ts → GeneratedBindings.cpp
[10/1205] gen ProcessBindingConstants.lut.h
Generating /workspace/bun/build/release/codegen/ProcessBindingConstants.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingConstants.cpp
[
... (truncated)

diff hotspot

src/runtime/valkey_jsc/js_valkey.rs |   6 +-
 test/js/valkey/valkey.test.ts       | 124 +++++++++++++++++++++++++++++++++++-
 2 files changed, 127 insertions(+), 3 deletions(-)

gate history · 3 passed · 0 rejected · iteration 4

evidence per changed file

file                                 reads  edits  tests
src/runtime/valkey_jsc/js_valkey.rs      4      6     14
test/js/valkey/valkey.test.ts            4      8     14

[robobun]

^{Updated 3:15 AM PT - Aug 18th, 2026}

✅ @robobun, your commit 892f065edb11c2be47a7428ca9bd7d7876f83983 passed in Build #100536! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 32085

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

bun-32085 --bun

[github-actions]

Found 1 issue this PR may fix:

1. RedisClient: assigning null to onclose panics during close() #29145 - Exact match: crash when RedisClient.onclose is set to null (a non-callable allowed by types) and close() is called; this PR adds is_callable() guards at the invocation sites

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

RedisClient lifecycle callback handlers (onconnect, onclose) now require callability before scheduling or invoking cached values; non-callable assignments are skipped. Tests (in-process and subprocess) verify non-callable assignments do not crash and that callable handlers run with expected error/success semantics.

Changes

RedisClient callback handler robustness

Layer / File(s)	Summary
Callback callability validation in runtime src/runtime/valkey_jsc/js_valkey.rs	on_valkey_connect filters the cached onconnect callback for callability before enqueuing the microtask; on_valkey_close and fail_with_js_value filter the cached onclose callback for callability before invocation. Non-callable values are skipped rather than passed to the call path.
Test coverage for lifecycle handler robustness test/js/valkey/valkey.test.ts	Harness import updated to include bunEnv for subprocess spawning. New concurrent test suite stubs TCP servers to trigger connect and close events, and spawns Bun processes to validate that non-callable onclose/onconnect assignments do not crash, while callable handlers receive expected connection errors and successful connection events.

Suggested reviewers

• alii

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR successfully addresses issue #29145 by implementing guards at three invocation sites to prevent crashes when onclose/onconnect are non-callable, allowing null and other non-callables as valid values.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the non-callable handler crash: modifications to invocation sites in js_valkey.rs and comprehensive test coverage in valkey.test.ts, with no unrelated alterations.
Title check	✅ Passed	The title clearly and concisely summarizes the primary fix for non-callable RedisClient onclose and onconnect handlers.
Description check	✅ Passed	The description includes both required sections and clearly explains the defect, scope, fix, regression tests, and verification results.

---

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

[coderabbitai]

Actionable comments posted: 3

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

Inline comments:
In `@test/js/valkey/valkey.test.ts`:
- Around line 6928-6940: The test helper expectChildOk rewrites any stderr that
does not include "ASSERTION FAILED" to an empty string, which hides unexpected
child-process errors; update expectChildOk to assert the raw captured stderr
instead of blanking it—keep capturing stdout/stderr/exitCode via
proc.stdout.text() and proc.stderr.text(), then expect({ stdout: stdout.trim(),
exitCode, stderr: stderr.trim() }) toEqual({ stdout: "ok", exitCode: 0, stderr:
"" }); remove the conditional that special-cases "ASSERTION FAILED" so tools
like bunExe() and environment bunEnv-based subprocess tests surface real stderr
diagnostics.
- Around line 6943-6965: Add tests that exercise the normal-close path for
non-callable onclose values: duplicate the existing test.each that uses
closingServer but run the same value matrix against helloServer, set
client.onclose to each non-callable (same list used in the closingServer tests),
await client.connect(), then call client.close() and assert console output via
expectChildOk. Update or add tests referencing test.each, onclose, helloServer,
expectChildOk, client.connect, and client.close so both error-path
(closingServer) and successful-connect path (helloServer) are covered.
- Around line 6967-7001: Both tests create a RedisClient and do not guarantee it
is closed if an assertion or connect() throws; wrap the client lifecycle in a
try/finally and call client.close() in the finally block (for example in the
"callable onclose still receives the connection error" and "callable onconnect
still fires after connecting" tests), ensuring RedisClient (constructed via new
RedisClient(...)) is always closed even on errors rather than relying on using
or happy-path closes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 610bc676-d996-4b50-af7f-37dbe99ee3ab

📥 Commits

Reviewing files that changed from the base of the PR and between 8354af5 and 03a387e.

📒 Files selected for processing (2)

• src/runtime/valkey_jsc/js_valkey.rs
• test/js/valkey/valkey.test.ts

[github-actions]

This PR may be a duplicate of:

1. redis: accept null for onclose/onconnect setters #29149 - Also fixes the crash when RedisClient onclose/onconnect is set to a non-callable value, by adding is_callable() guards at the same call sites (onclose, onconnect, failWithJSValue) — targets the older Zig source while this PR targets the current Rust port

🤖 Generated with Claude Code

[claude]

LGTM — three-line defensive guard that turns a crash into a no-op, with thorough hermetic test coverage.

What was reviewed:

• The three .filter(|cb| cb.is_callable()) guards: purely additive — callable values pass through unchanged, non-callable values (which previously crashed in JSValue::call/queue_microtask) are skipped. is_callable() is a side-effect-free FFI check.
• Test suite: subprocess crash tests for the non-callable matrix, the #29145 manual-close repro, and positive tests confirming callable handlers still fire — all against in-process TCP listeners, no external Redis needed.
• Checked that the sibling Postgres/MySQL onclose/onconnect slots the PR description mentions are indeed internal-only and not user-reachable.
• All three CodeRabbit threads addressed and resolved (try/finally cleanup, stderr-on-failure diagnostics, connected-then-close coverage).

Extended reasoning...

Overview

The PR adds .filter(|cb| cb.is_callable()) at the three sites in src/runtime/valkey_jsc/js_valkey.rs where the cached onclose/onconnect values are invoked (on_valkey_connect, on_valkey_close, fail_with_js_value). The generated cached-property setter accepts any value, and the published .d.ts types explicitly allow null for clearing handlers, but the invocation sites previously passed the value straight to JSValue::call / queue_microtask, which assert callability in debug builds and crash release builds. The test file gains a describe.concurrent block with subprocess-based crash regressions plus in-process positive tests.

Security risks

None. This is a crash-hardening fix in an event-handler dispatch path. No auth, crypto, permissions, or untrusted-input parsing is touched. The guard narrows behavior (skip instead of crash); it introduces no new code path that accepts additional input.

Level of scrutiny

Low-to-moderate. It's native runtime code, but the change is mechanically trivial and monotonic: Option<JSValue>.filter(is_callable) returns Some exactly when the old code would have succeeded, and None exactly when the old code would have crashed. is_callable() is a pure FFI predicate (no JS re-entry, no throwing). There is no way for this to regress previously-working code. The semantics match DOM event-handler idioms (onclick = null is a no-op) and the published types.

Other factors

• Tests follow CLAUDE.md conventions closely: bunEnv/bunExe, port: 0, using for TCP listeners, try/finally for client cleanup, subprocess spawns for process-abort cases with concurrent pipe drains, exit code asserted via combined object, and describe.concurrent for the independent subprocess suite.
• PR description documents that the tests fail on the unfixed build (release: exit 132; debug: assertion) and pass on the fixed build.
• All three CodeRabbit review threads were addressed in follow-up commits and marked resolved; the stderr-handling adjustment (include on nonzero exit only) is the right call per CLAUDE.md's guidance against asserting exactly-empty stderr on debug/ASAN builds.
• The bug-hunting system found no issues.
• A duplicate PR (#29149) targets the old Zig source; this one targets the current Rust port, so they don't conflict.

[robobun]

CI status on build #73487: the valkey tests added here pass on every lane that ran them (928 pass / 0 fail on the debian x64 shard). The three red jobs are two unrelated main breaks that are already being fixed separately:

• test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js (SIGABRT from a JSC exception-scope assertion in JSObject::getOwnPropertyDescriptor on x64-asan)
• test/js/node/test/parallel/test-net-connect-memleak.js (FinalizationRegistry callback not observed after gc on alpine x64 and x64-baseline; failed all retries)

Neither touches the redis client; this PR's diff is limited to src/runtime/valkey_jsc/js_valkey.rs and test/js/valkey/valkey.test.ts. Leaving this for maintainer review; CI should go green once those main breaks land.

[robobun]

Heads up from #38453, which touches the same setters: once it lands, the value in the onconnect/onclose slots can be an AsyncContextFrame (the wrapper with_async_context_if_needed returns when a handler is assigned inside an AsyncLocalStorage context), which is not callable. The .filter(|cb| cb.is_callable()) guards here would then skip every handler registered inside a context. Whichever PR lands second needs to account for that; the cheapest combination is cb.is_callable() || cb.is_async_context_frame() (the latter is added to JSValue in #38453), or moving the non-callable handling into the setter, which #38453 already makes a hand-written function that branches on is_callable().

[claude]

LGTM — three-line defensive guard turning a process abort into a no-op when onclose/onconnect holds a non-callable, matching the published types that allow null to clear the handler.

What was reviewed:

• The three .filter(|cb| cb.is_callable()) guards at the only sites that invoke the cached handler values; is_callable() is the correct predicate for both JSValue::call and queue_microtask.
• Test suite: subprocess crash tests for the non-callable matrix on both close paths (connect-failure and manual close after connect) and onconnect, plus positive tests confirming callable handlers still fire; describe.concurrent, in-process TCP listeners, try/finally cleanup, concurrent pipe drain.
• The #38453 coordination note is forward-looking ("whichever PR lands second") and not a blocker here; the CI failures (test-net-connect-memleak, test-worker-message-port-transfer-terminate) are unrelated main breaks.

Extended reasoning...

Overview

The PR guards three invocation sites in src/runtime/valkey_jsc/js_valkey.rs — on_valkey_connect (queues onconnect as a microtask), on_valkey_close and fail_with_js_value (both call onclose) — with .filter(|cb| cb.is_callable()) so a stored non-callable is skipped instead of hitting the ASSERTION FAILED: … must be callable abort in Bun__JSValue__call / queueMicrotask. The generated cached-property setters (cached_prop_hostfns!) store any value verbatim, and packages/bun-types/redis.d.ts documents null as valid for clearing, so this is user-reachable input. Roughly 120 lines of new tests in test/js/valkey/valkey.test.ts cover the crash (subprocess-based, since the bug is a process abort) and the positive case.

Security risks

None. This is a defensive guard on a user-settable event-handler slot; no auth, crypto, or resource-limit code touched. The change strictly narrows behavior (skip invocation) rather than adding a new code path.

Level of scrutiny

Low-to-medium. The Rust diff is three identical one-line additions of an Option::filter before existing if let Some(...) blocks; JSValue::is_callable is the thin FFI wrapper around JSC__JSValue__isCallable (JSValue.rs:343). Skipping a non-callable is the standard event-handler-property semantic and matches what the types promise. The PR description explicitly scopes out the analogous Postgres/MySQL slots (internal-only writers, native objects not user-reachable), which satisfies the "if a site is intentionally excluded, say so" rule.

Other factors

All three CodeRabbit threads are resolved: clients now close in finally, the manual-close()-after-connect path is covered (the exact #29145 repro), and expectChildOk surfaces raw stderr on any nonzero exit while tolerating benign debug/ASAN warnings on success. Tests use describe.concurrent, in-process Bun.listen servers (no Redis required, no external network), Promise.all on stdout/stderr/exited, and bunEnv/bunExe(). The gate evidence in the PR body shows the new tests fail on the unfixed release and ASAN builds and pass with the fix. The two red CI jobs are documented main breaks in node/test/parallel unrelated to redis. The 2026-08-14 robobun note about #38453 (AsyncContextFrame wrapping) is a coordination heads-up for whichever PR lands second, not a defect in this change — #38453 rewrites the setter itself, at which point it can incorporate the non-callable handling.

[robobun]

On the #38453 interaction noted above: #38453 is still open, and main's onconnect/onclose setters are still the plain cached_prop_hostfns! pair, so the slots can only hold whatever JS assigned and the is_callable() guards here are correct against current main (rebased onto 3cf3149 today, tests pass).

Merge order is the only thing to watch. If #38453 lands first, the Rust hunks in this PR are in different regions from the setter rewrite and would auto-merge, so I will widen the guards to also accept the AsyncContextFrame wrapper when I rebase rather than relying on the conflict to flag it. If this PR lands first, the hand-written setter in #38453 is the natural place to fold the non-callable handling in, and these three guards become redundant but harmless.

[claude]

LGTM — three-site .filter(|cb| cb.is_callable()) guard on the cached onclose/onconnect slots, with subprocess crash-regression tests that need no Redis server.

What was reviewed:

• Confirmed all three RedisClient invocation sites of onclose_get_cached/onconnect_get_cached are guarded; the excluded Postgres/MySQL sites are internal-only as the description states.
• Checked the #38453 interaction: setters are still plain cached_prop_hostfns! on current main, so the slot cannot yet hold an AsyncContextFrame and is_callable() is sufficient here.
• Tests cover all three paths (connect-failure onclose, manual-close onclose, successful-connect onconnect) plus positive callable cases; CodeRabbit feedback on try/finally cleanup and stderr-in-failure-diff is applied.

Extended reasoning...

Overview

Fixes #29145 and a fuzzer crash: RedisClient.prototype.onclose/.onconnect are cached-property slots with no setter validation, and the native connect/close paths invoked whatever was stored via JSValue::call / queue_microtask, both of which assert the value is callable. Assigning null (which the published .d.ts explicitly allows for clearing the handler), or any other non-callable, aborted the process. The fix adds .filter(|cb| cb.is_callable()) at the three invocation sites in src/runtime/valkey_jsc/js_valkey.rs (on_valkey_connect, on_valkey_close, fail_with_js_value). Test coverage is added in test/js/valkey/valkey.test.ts.

Security risks

None. This is a defensive callability check on a user-assigned handler slot before dispatch; it strictly narrows the set of values that reach .call()/queue_microtask().

Level of scrutiny

Low. The Rust change is three identical one-line .filter() guards on Option<JSValue> — mechanical and self-evidently correct (is_callable() is a thin FFI call to JSC__JSValue__isCallable). Setter behavior is untouched, so values still round-trip through the getter as before. This matches how event-handler properties are treated elsewhere and how the types declare the field.

Other factors

• The author explicitly scoped out the Postgres/MySQL onclose/onconnect sites because those native connection objects are not user-reachable and the slots are only written by internal JS with bound methods — grep confirms those are the only other onclose_get_cached/onconnect_get_cached sites, and the exclusion is stated in the description per REVIEW.md guidance.
• The #38453 merge-order interaction (AsyncContextFrame wrapper in the slot) was flagged by the author on 2026-08-14 and re-checked on 2026-08-16: #38453 is still open and main's setters remain the plain cached_prop_hostfns! macro, so is_callable() is the correct predicate against current main. Whichever PR lands second will need to reconcile, and the author has committed to doing so.
• Tests are well-constructed per REVIEW.md: subprocess spawns for the crash cases (the bug is a process abort), in-process Bun.listen fakes for the RESP3 handshake so no Redis server is needed, describe.concurrent + test.each for the value matrix, concurrent stdout/stderr/exited drain, combined-object assertion with stderr surfaced on nonzero exit, and try/finally client cleanup in the in-process positive tests. The evidence block shows the new tests fail on unfixed release (exit 132) and debug (assertion) builds and pass with the fix.
• All three CodeRabbit threads are resolved (two applied, one withdrawn after the author showed both close flavors reach the same guarded call site).

[claude]

LGTM — two .filter(|cb| cb.is_callable()) guards before the cached onclose/onconnect invocations, plus subprocess crash-regression tests.

What was reviewed:

• Confirmed these are the only two onclose_get_cached/onconnect_get_cached reads in src/runtime/valkey_jsc/; fail_with_js_value reaches onclose only via close() → on_close() → on_valkey_close(), so both paths hit the guarded site.
• is_callable() maps to JSC__JSValue__isCallable, the same predicate JSValue::call/queue_microtask assert on.
• Tests are hermetic (in-process Bun.listen on port 0, subprocess spawns for the abort cases, pipes drained concurrently, clients closed in finally); all three CodeRabbit threads are resolved.
• The #38453 AsyncContextFrame interaction the author flagged is a merge-order concern only; the setter on main is still the plain cached_prop_hostfns! macro, so is_callable() is correct here.

Extended reasoning...

Overview

This PR fixes a fuzzer-found crash (also independently reported as #29145) where assigning any non-callable — including null, which the published .d.ts types explicitly allow for clearing the handler — to RedisClient.prototype.onclose or .onconnect would abort the process on the next connect/close event. The generated cached-property setter stores whatever is assigned; the native paths then passed it unconditionally to JSValue::call (asserts "must be callable") or queue_microtask (asserts "async context frame or a callable").

The fix is two .filter(|cb| cb.is_callable()) calls in src/runtime/valkey_jsc/js_valkey.rs (on_valkey_connect and on_valkey_close). I grepped for all onclose_get_cached/onconnect_get_cached reads in src/runtime/valkey_jsc/ and confirmed these are the only two invocation sites; fail_with_js_value (mentioned in the description) routes through close() → on_close() → on_valkey_close(), so it reaches the same guarded call. The description's "three invocation sites" appears to be a stale count from a pre-rebase state; the coverage is complete against current main. is_callable() wraps JSC__JSValue__isCallable, which is exactly the predicate the failing assertions check, so the guard matches the invariant.

The test additions (~120 lines in test/js/valkey/valkey.test.ts) use in-process Bun.listen({ port: 0 }) servers to drive the connect/close paths without a real Redis server. Non-callable cases spawn subprocesses via bunExe() -e (correct since the bug is a process abort), drain stdout/stderr/exited concurrently, and assert {stdout, exitCode, stderr} in one object with stderr surfaced on nonzero exit. Positive tests confirm callable handlers still fire and close the client in finally. The evidence block shows the suite fails on unfixed release (exit 132) and debug/ASAN builds and passes with the fix.

Security risks

None. This is a defensive callability check before invoking a user-assigned callback. It cannot introduce injection, auth bypass, or data exposure; it strictly narrows the set of values that reach call()/queue_microtask().

Level of scrutiny

Low-to-medium. The Rust change is 4 lines of net addition applying a well-understood JSC predicate at exactly the sites that assert on it. The blast radius is limited to the RedisClient event-handler dispatch path. The tests are more substantial than the fix but follow harness conventions (bunEnv, bunExe(), describe.concurrent, test.each, using for the listeners, try/finally for the clients).

Other factors

• All three CodeRabbit threads (stderr handling in expectChildOk, manual-close coverage, try/finally cleanup) were addressed and resolved on the PR.
• The author noted that Postgres/MySQL share the cached-slot pattern but their slots are only written by internal JS with bound methods and are not user-reachable, so leaving them unchanged is deliberate.
• The author flagged and analyzed the interaction with open PR #38453 (which will wrap handlers in a non-callable AsyncContextFrame); that PR has not landed, main's setter is still the plain cached_prop_hostfns! macro (verified at js_valkey.rs:1082-1086), and the author committed to widening the guard on rebase if #38453 lands first. That is a merge-order concern, not a defect in this diff.
• CI reds on the referenced build were unrelated main breaks in worker/net tests per the author's follow-up.

[robobun]

Status on main (bf42a52): this change is still needed. A release build no longer crashes, but seven of the nine tests in this PR fail on main.

I ran the RedisClient onclose/onconnect handler values block against a debug (ASAN) build of main at bf42a52, without the source change:

• client.onclose = {} (also 42, null, undefined and a string), then the server closes the connection. The child exits with code 1 and an uncaught TypeError: Object is not a function.
• client.onclose = null; client.close() after a connect. The call throws TypeError: client.close is not a function. (In 'client.close()', 'client.close' is null). This is the case in RedisClient: assigning null to onclose panics during close() #29145, which is still open.
• client.onconnect = {}. The debug build aborts with ASSERTION FAILED: queueMicrotask must be called with an async context frame or a callable. (src/jsc/bindings/bindings.cpp:5839). A release build of main from 2026-09-18 (367d939) exits with code 0 for this case.
• The two tests with a callable handler pass.

src/runtime/valkey_jsc/js_valkey.rs on main still calls both handlers with no is_callable() check (lines 1298 and 1445). The title says crash. On main the failure is a TypeError in a release build and an assertion in a debug build.

[Jarred-Sumner]

This is Claude (an AI assistant) commenting on Jarred's behalf.

This fix went in with #46021, which is merged. Closing.