Repository navigation
Conversation
|
Updated 11:38 AM PT - Mar 25th, 2026
❌ @robobun, your commit 16b20b4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 28454That installs a local version of the PR into your bun-28454 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughGuard checks were added so Valkey client lifecycle callbacks ( Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/valkey/valkey-non-callable-callback.test.ts`:
- Line 24: Replace the loose crash check in the test that uses
expect(exitCode).not.toBe(6) with a strict success assertion to avoid false
positives: update the assertion in valkey-non-callable-callback.test (the
exitCode checks around the process execution result) to assert exitCode === 0
using expect(exitCode).toBe(0); do this for both occurrences (the check
currently at the first occurrence and the second occurrence) so the test only
passes on a successful process exit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 142377e8-7ab7-40d9-a90b-721d13783758
📥 Commits
Reviewing files that changed from the base of the PR and between 7336160 and 29ae36fcb378eb9eeaeae7daea4252c3f6ee71bf.
📒 Files selected for processing (2)
src/valkey/js_valkey.zigtest/js/valkey/valkey-non-callable-callback.test.ts
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/valkey/valkey-non-callable-callback.test.ts`:
- Line 23: Remove the fragile negative assertions that scan stderr for
"ASSERTION FAILED" in the test file (the expect(stderr).not.toContain("ASSERTION
FAILED") assertions present in
test/js/valkey/valkey-non-callable-callback.test.ts and the similar check at the
other occurrence), and instead assert deterministic process outcome by checking
exitCode === 0 (or the existing success condition used by the test harness);
update the test to drop the stderr not-to-contain checks and rely on the test's
exitCode/success assertions to validate no crash/uncaught errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2becb61f-1ecc-4b71-9054-f376985a764e
📥 Commits
Reviewing files that changed from the base of the PR and between 29ae36fcb378eb9eeaeae7daea4252c3f6ee71bf and d5bd7c199e2cf75189418239efdaabab035fc8ff.
📒 Files selected for processing (1)
test/js/valkey/valkey-non-callable-callback.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| test("non-callable onconnect does not crash", async () => { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| const redis = new Bun.RedisClient("redis://localhost:6379"); | ||
| redis.onconnect = "not a function"; | ||
| try { await redis.connect(); } catch(e) {} | ||
| try { redis.close(); } catch(e) {} | ||
| await Bun.sleep(500); | ||
| `, | ||
| ], | ||
| env: bunEnv, | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stderr).not.toContain("ASSERTION FAILED"); | ||
| expect(exitCode).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🔴 The 'non-callable onconnect does not crash' test never exercises the onValkeyConnect code path because it connects to localhost:6379 where no Redis server is running in CI, so connection failure triggers onValkeyClose instead. The test passes trivially with or without the isCallable() guard added to onValkeyConnect, providing zero regression protection for that specific fix.
Extended reasoning...
The test at lines 27-48 of valkey-non-callable-callback.test.ts sets redis.onconnect to a non-callable string and attempts to connect to localhost:6379 with no running Redis server. In CI, this always results in a connection-refused error.
onValkeyConnect in js_valkey.zig is only invoked after a successful connection — specifically after TCP connection is established AND the HELLO handshake completes (status becomes .connected). The isCallable() guard added in this PR lives inside that function. When no server is listening at localhost:6379, the TCP connection is refused, causing the socket handler's onConnectError/onClose to fire. This calls onValkeyClose, not onValkeyConnect.
The main Valkey test suite guards Redis availability with an isEnabled check backed by Docker. The new test file has no such guard, unconditionally connecting to localhost:6379. Without a running server, the connection-refused path is always taken and onValkeyConnect is never reached.
The isCallable() guard added to onValkeyConnect is effectively untested. If that guard were accidentally removed in a future refactor, this test would still pass because no server means no onValkeyConnect call means no crash regardless. Any regression to the onconnect non-callable fix would go undetected. By contrast, the first test ('non-callable onclose does not crash') IS valid because connection failure always triggers onValkeyClose.
Step-by-step proof: (1) Test spawns subprocess setting redis.onconnect = 'not a function' then calling redis.connect(). (2) Bun attempts TCP to localhost:6379 — no server is listening. (3) OS returns ECONNREFUSED. (4) SocketHandler.onConnectError fires, calls this.client.onClose(). (5) onValkeyClose is invoked — it checks oncloseGetCached, not onconnectGetCached. (6) onValkeyConnect is never called; the isCallable() guard at ~line 884 is never reached. (7) Test checks exitCode !== 6 and no ASSERTION FAILED in stderr — both pass trivially.
To properly test the onconnect fix, the test would need either a running Redis server (or mock completing the HELLO handshake), or should use the same Docker-backed isEnabled guard as the existing Valkey integration tests.
There was a problem hiding this comment.
LGTM — all prior review concerns have been addressed.
Extended reasoning...
Overview
This PR modifies src/valkey/js_valkey.zig to add isCallable() guards at three call sites (onValkeyConnect, onValkeyClose, failWithJSValue) before invoking cached JS callbacks, and adds a new test file test/js/valkey/valkey-non-callable-callback.test.ts.
Security risks
None. The change only adds defensive guards that skip callback invocation when the stored value is not callable. No auth, crypto, or permission code is touched.
Level of scrutiny
Low. This is a targeted defensive fix — three nearly identical two-line additions wrapping existing call sites. The pattern matches what the rest of the codebase does when invoking user-supplied callbacks.
Other factors
All issues raised in prior review rounds were addressed: autoReconnect: false prevents indefinite subprocess hangs, test.concurrent is used for both independent subprocess tests, expect(exitCode).toBe(0) replaces the broken .not.toBe(6) assertion, and the prohibited stderr assertion checks were removed. The residual limitation that the onconnect test cannot reach onValkeyConnect without a running Redis server is already noted in an unresolved inline comment and is a test coverage gap rather than a correctness issue with the fix itself.
89fd7dd to
a33732e
Compare
There was a problem hiding this comment.
LGTM — all prior feedback addressed, Zig fix is correct.
Extended reasoning...
Overview
Two files changed: src/valkey/js_valkey.zig adds isCallable() guards at three call sites (onValkeyConnect, onValkeyClose, failWithJSValue) before invoking cached onconnect/onclose callbacks. test/js/valkey/valkey-non-callable-callback.test.ts adds two subprocess-based regression tests.
Security risks
No security-sensitive code touched. The change is purely defensive: guard against invoking non-callable JS values that were stored in the codegen cache.
Level of scrutiny
This is a focused crash fix in the Valkey client lifecycle callbacks. The Zig change is minimal and mechanical — three isCallable() guard wraps around existing call sites, following the same pattern used elsewhere in the JS runtime. Low complexity, low blast radius.
Other factors
All issues from prior review rounds were addressed: autoReconnect: false prevents subprocess hangs, test.concurrent used, expect(exitCode).toBe(0) used, stderr assertion removed. The one remaining acknowledged gap — the onconnect test not actually reaching onValkeyConnect without a running Redis server — is noted in the PR description and does not affect the correctness of the fix. The onclose regression test is valid and will detect the primary crash scenario.
Check isCallable() before invoking cached onclose/onconnect callbacks in the valkey client. Previously, setting these to a non-function value (e.g. redis.onclose = 42) would trigger an assertion failure in Bun__JSValue__call when the connection closed.
Prevents subprocess from hanging due to 20 retry attempts when no Redis server is available. Also removes unnecessary Bun.sleep and uses test.concurrent for independent subprocess tests.
b5c5f94 to
16b20b4
Compare
There was a problem hiding this comment.
LGTM — all prior review feedback has been addressed.
Extended reasoning...
Overview
This PR modifies src/valkey/js_valkey.zig to add isCallable() guards at three call sites (onValkeyConnect, onValkeyClose, failWithJSValue) before invoking the cached onconnect/onclose JS values. A new test file test/js/valkey/valkey-non-callable-callback.test.ts provides regression coverage.
Security risks
None. The change only adds defensive callability checks before invoking JS callbacks; it does not touch auth, crypto, permissions, or any sensitive subsystem.
Level of scrutiny
The Zig change is a straightforward three-location guard addition with no logic changes — each hunk simply wraps an existing call in if (on_close/on_connect.isCallable()). The test file is a new addition with no impact on existing tests. Appropriate scrutiny level is low.
Other factors
All concerns raised in prior review rounds have been resolved in the current revision: test.concurrent is used for both tests, autoReconnect: false is set to prevent subprocess hangs, expect(exitCode).toBe(0) is used (not the weaker .not.toBe(6)), and the prohibited stderr assertion checks were removed. CI (Buildkite #41999) shows the infra failure is a macOS runner expiry annotation unrelated to the code change.
|
Closing in favor of #29149 — same bug, broader fix: #29149 adds setter validation (reject non-callable non-null with a |
|
Superseded by #29149. |
The valkey (redis) client caches
oncloseandonconnectcallback values without validating that they are callable. When a connection event fires, the cached value is passed to.call()which triggers an assertion failure inBun__JSValue__callif the value is not a function.Root cause:
setOnConnectandsetOnClosestore any JS value via the codegen cache, andonValkeyClose,failWithJSValue, andonValkeyConnectretrieve and invoke them without checkingisCallable().Fix: Add
isCallable()guards at the three call sites before invoking the cached callbacks.Repro:
Verification (robobun): Zig diff adds
isCallable()guards at all three call sites injs_valkey.zig(onValkeyConnect, onValkeyClose, failWithJSValue) — minimal and correct. Regression test spawns subprocesses with non-callableonclose=42/onconnect="not a function", assertsexitCode === 0; on main this crashes via assertion failure inBun__JSValue__call. All bot review feedback addressed across iterations (.toBe(0), stderr removal,autoReconnect: false,test.concurrent). No TODO/FIXME/HACK in diff. Lint JavaScript passes. Buildkite #41347 shows 0 job/step failures; "failing" state is from infrastructure error annotation (macOS runners expired), not code.