Repository navigation
Conversation
|
Updated 10:05 PM PT - May 5th, 2026
❌ @robobun, your commit 7d1d61c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29149That installs a local version of the PR into your bun-29149 --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:
WalkthroughThis pull request adds a regression test for RedisClient's Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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/regression/issue/29145.test.ts`:
- Around line 20-100: Each test that instantiates RedisClient should guarantee
cleanup by wrapping the test body in a try/finally where RedisClient.close() is
called in the finally block; update each test using new
RedisClient("redis://localhost:6379") (including tests touching c.onclose and
c.onconnect, and the ones asserting throws) to place assertions inside the try
and call c.close() in finally so the client is closed even if an assertion
fails.
🪄 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: 062d291b-2d65-424b-aabe-4971fb424326
📒 Files selected for processing (2)
src/valkey/js_valkey.zigtest/regression/issue/29145.test.ts
|
This PR may be a duplicate of:
🤖 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/regression/issue/29145.test.ts`:
- Line 21: Replace hardcoded "redis://localhost:6379" usages with dynamically
assigned test ports: start the test Redis server with port: 0, capture the
actual bound port (e.g., server.address().port or similar from your test Redis
helper), and construct the client URI using that port when creating RedisClient
(instances like the variable c created with new RedisClient(...)). Update every
occurrence noted (lines creating new RedisClient) to use the dynamically
resolved port instead of 6379 so tests do not rely on a fixed port.
🪄 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: 595e1b6a-6856-4537-8172-ff04a27d9a97
📒 Files selected for processing (1)
test/regression/issue/29145.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/valkey/js_valkey.zig:706-734— The onclose/onconnect type declarations in packages/bun-types/redis.d.ts were not updated to reflect the new runtime behavior: getters now return undefined (not null) for cleared handlers, and setters now accept undefined in addition to null. TypeScript consumers checking client.onclose === null will silently miss cleared handlers, and assigning undefined to detach a handler will fail type-checking despite being accepted at runtime — the test file itself uses as any to work around this.Extended reasoning...
What the bug is and how it manifests
This PR changes the runtime semantics of onclose/onconnect in two ways that are not reflected in the published TypeScript type declarations in packages/bun-types/redis.d.ts:
-
Getter now returns undefined instead of null for cleared handlers. The updated getOnClose/getOnConnect implementations call value.isCallable() and return .js_undefined when the cached slot is not callable. After c.onclose = null, reading c.onclose back returns undefined, not null. The test confirms this is intentional: expect(c.onclose).toBeUndefined().
-
Setter now accepts undefined as a clear operation. The updated setOnClose/setOnConnect use value.isUndefinedOrNull(), accepting undefined at runtime. However, the type declaration only lists | null, not | undefined. The test acknowledges this gap at lines 37 and 42 with c.onclose = undefined as any — the as any cast is required to suppress the TypeScript compile error.
The specific code path that triggers it
getOnClose/getOnConnect (lines 706-713) now gate the return on .isCallable() and fall through to return .js_undefined. setOnClose/setOnConnect (lines 715-734) call value.isUndefinedOrNull() to accept both null and undefined as a clear signal. The type file packages/bun-types/redis.d.ts was not modified in this PR and still declares both properties as ((...)=>void) | null.
Why existing code does not prevent it
The TypeScript type file is a separate artefact from the Zig implementation. Changing runtime behavior in Zig does not automatically update the .d.ts. This PR updated the implementation but did not touch packages/bun-types/redis.d.ts.
Impact
Any TypeScript code using if (client.onclose === null) to detect a cleared handler will silently get the wrong answer at runtime (the getter returns undefined, which is not === null). Falsy checks (if (!client.onclose)) are unaffected. TypeScript users who write client.onclose = undefined to detach a handler will get a compile-time error, even though the runtime accepts it. The workaround (as any) is already present in the test file.
How to fix it
Update packages/bun-types/redis.d.ts to declare both properties as accepting and returning undefined:
onconnect: ((this: RedisClient) => void) | null | undefined;
onclose: ((this: RedisClient, error: Error) => void) | null | undefined;Step-by-step proof
- Start with const c = new RedisClient(url); TypeScript infers c.onclose has type ((error: Error) => void) | null.
- Assign c.onclose = null — accepted by both TypeScript (null is in the union) and the runtime.
- Read back c.onclose: the Zig getter checks .isCallable() on the stored .zero (null), finds it is not callable, and returns .js_undefined. JavaScript receives undefined.
- Check c.onclose === null: evaluates to false at runtime, despite the type saying the value can be null.
- Separately, write c.onclose = undefined: TypeScript emits a compile error (undefined is not assignable to ((error: Error)=>void) | null), but the runtime silently accepts it — explaining the as any in the regression test.
-
There was a problem hiding this comment.
LGTM — test-only regression file with proper try/finally cleanup and good coverage of the null/undefined/callable cases.
Extended reasoning...
Overview
This PR adds a single regression test file () for the null-handler panic. The actual Zig fix was already landed in commit . No production code is modified here.
Security Risks
None. This is a test-only change with no security-sensitive code paths.
Level of Scrutiny
Low — the change is a new test file only, covering a well-defined panic scenario with 11 assertions. The feedback from the reviewer was already incorporated in commit . The remaining coderabbitai note about hardcoded port is a style nit that doesn't apply in the usual sense here: connects to an external Redis server rather than one the test spawns, so dynamic allocation isn't applicable.
Other Factors
No bugs were found. A potential-duplicate flag was raised against PR #28454, but that is a separate concern for human triage. The test structure is clean and follows the patterns already established in the regression test suite.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/29145.test.ts (1)
136-147:⚠️ Potential issue | 🟠 MajorMake the in-flight teardown case self-contained.
This is the only test here that actually connects: Line 147 issues
set(...), soredis://localhost:6379is no longer a harmless placeholder. On a machine with a real local Redis it mutates external state; without one it exercises a different refusal path. Please drive this case with a test-local listener onport: 0(or the existing Redis harness) and build the URL from that port instead.Based on learnings: the hardcoded Redis URL exception only applies when tests only exercise property setter/getter behavior and never issue commands or await connection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/29145.test.ts` around lines 136 - 147, The test "null onclose while an in-flight connection is being torn down does not panic" currently creates a real RedisClient("redis://localhost:6379") and issues c.set(...), which can touch external Redis; change it to start a test-local Redis listener on port 0 (or use the repo's Redis test harness), obtain the ephemeral port, construct the client URL from that port, and use that URL when creating new RedisClient and running c.set(...). Ensure the test tears down the local listener/harness after completion so the in-flight teardown case is fully self-contained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/regression/issue/29145.test.ts`:
- Around line 136-147: The test "null onclose while an in-flight connection is
being torn down does not panic" currently creates a real
RedisClient("redis://localhost:6379") and issues c.set(...), which can touch
external Redis; change it to start a test-local Redis listener on port 0 (or use
the repo's Redis test harness), obtain the ephemeral port, construct the client
URL from that port, and use that URL when creating new RedisClient and running
c.set(...). Ensure the test tears down the local listener/harness after
completion so the in-flight teardown case is fully self-contained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70500b52-fe59-4e51-b4c3-a2ab8f935d64
📒 Files selected for processing (3)
packages/bun-types/redis.d.tssrc/valkey/js_valkey.zigtest/regression/issue/29145.test.ts
There was a problem hiding this comment.
LGTM — targeted null-guard fix with good regression coverage.
Extended reasoning...
Overview
This PR fixes a panic in RedisClient when onclose/onconnect is assigned null. It touches three files: the Zig runtime (src/valkey/js_valkey.zig), the TypeScript type definitions (packages/bun-types/redis.d.ts), and a new regression test file.
Security Risks
None. The changes are purely defensive: they normalize null/undefined input and add isCallable() guards before invoking cached JS callbacks. No auth, crypto, or permission code is touched.
Level of Scrutiny
Low-to-medium. The Zig changes are minimal and follow an established pattern already used in ReadableStreamSource.setOnCloseFromJS: normalize null/undefined to .zero, throw TypeError for other non-callable values, guard call sites with isCallable(). The type-def change correctly documents the contract. The regression tests cover null, undefined, valid functions, invalid non-null values, and the original panic repro shape.
Other Factors
- Bug hunting found no bugs.
- The try/finally cleanup feedback from CodeRabbit was addressed in commit 9993742.
- The hardcoded port 6379 in the test is a minor style concern but does not affect test correctness: most tests exercise only getter/setter behavior without connecting, and the one async test that does connect swallows connection errors.
- A possible duplicate PR (#28454) was flagged — worth a human glance to confirm there is no double-merge conflict, but does not affect the correctness of this fix.
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/regression/issue/29145.test.ts`:
- Around line 40-58: The tests set c.onclose and c.onconnect to undefined but
don't assert that the getter returns undefined; update the two tests that touch
RedisClient.onclose and RedisClient.onconnect to also assert the readback (e.g.,
expect(c.onclose).toBeUndefined() and expect(c.onconnect).toBeUndefined())
before calling c.close(), keeping the existing try/finally and use of expect(()
=> c.close()).not.toThrow() to preserve behavior checks.
🪄 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: 32b20fe2-bd12-438c-9a3d-a198152703fb
📒 Files selected for processing (1)
test/regression/issue/29145.test.ts
c1609c7 to
d79e379
Compare
There was a problem hiding this comment.
LGTM — clean crash fix with correct null/undefined normalization and defense-in-depth callable guards at all teardown sites.
Extended reasoning...
Overview
Fixes a panic triggered by assigning null to RedisClient.onclose/onconnect. Three files changed: type declarations (redis.d.ts), the Zig runtime (src/valkey/js_valkey.zig), and a new regression test (test/regression/issue/29145.test.ts).
Security risks
None. The change only affects JS property setter/getter semantics and teardown call guards for lifecycle callbacks on a client object.
Level of scrutiny
Low. The fix is a straightforward null-check pattern applied consistently: setters normalize null/undefined to .zero and throw TypeError for other non-callables; getters return undefined when the slot is not callable; all three call sites that could invoke the cached value are guarded with isCallable(). The pattern matches the existing ReadableStreamSource.setOnCloseFromJS idiom in the codebase.
Other factors
- No bugs found by the bug-hunting system.
- CodeRabbit's
try/finallynit was addressed in commit9993742; the hardcoded-port concern was correctly declined (lazy construction, no server spawned). - The possible duplicate (PR #28454) noted by the bot appears to be a separate, potentially related fix — not a blocker for this PR.
There was a problem hiding this comment.
LGTM — the panic fix is correct, tests are solid, and prior feedback has been addressed.
Extended reasoning...
Overview
This PR fixes a panic in RedisClient when null or undefined is assigned to onclose/onconnect. Changes touch the Zig setter/getter implementations, TypeScript type declarations, and a new regression test.
Security risks
None. The change adds input validation to property setters and guards existing call sites against non-callable cached values. No auth, crypto, or permission-sensitive code paths involved.
Level of scrutiny
Low-to-medium. The fix follows an established pattern and is scoped to the Valkey client setter/getter path. The teardown call sites already had the correct shape; this PR makes them consistently guarded. The regression test covers null, undefined, readback, non-callable rejection, and the original in-flight teardown shape.
Other factors
Two nits were flagged: the TypeScript getter type still includes null (split accessor syntax would be more precise), and the last test passes "10" (string) instead of 10 (number) as EX seconds. Neither affects runtime behavior. Prior CodeRabbit feedback (try/finally cleanup, getter readback assertions) was addressed in subsequent commits.
There was a problem hiding this comment.
LGTM — prior feedback addressed: getter types correctly use split accessor syntax (getter returns | undefined, not | null), the EX argument was fixed to a numeric literal, and all try/finally cleanup patterns are in place.
Extended reasoning...
Overview
Fixes a panic triggered when null is assigned to RedisClient.onclose or RedisClient.onconnect. The bug was in src/valkey/js_valkey.zig: the setters stored the raw JS value (including null) into the cached slot, and the teardown path invoked .call() on it unconditionally, producing a cleared TypeError that surfaced as a panic.
Security risks
None. This is a lifecycle/teardown correctness fix with no auth, crypto, or privilege-sensitive code paths involved.
Level of scrutiny
Low-to-medium. The Zig changes are localized to getter/setter pairs and three call sites that already existed. The TypeScript change uses standard TS 4.3+ split accessor syntax to accurately model the asymmetric get/set contract. The fix pattern matches the existing ReadableStreamSource.setOnCloseFromJS precedent cited in the PR description.
Other factors
Both issues I raised in the prior review run are resolved: the getter return type no longer includes null (correctly narrowed to | undefined), and the EX argument in the in-flight test was corrected from "10" to 10. CodeRabbit nitpicks (try/finally cleanup, undefined readback assertions) were also addressed. The 11-assertion regression suite covers null, undefined, readback, non-callable rejection, and the original in-flight teardown shape.
There was a problem hiding this comment.
LGTM — the core fix is correct and well-implemented.
Extended reasoning...
Overview
The PR fixes a panic in Bun.RedisClient when onclose/onconnect is assigned null. It touches three files: the Zig implementation (src/valkey/js_valkey.zig), the TypeScript type definitions (packages/bun-types/redis.d.ts), and a new regression test (test/regression/issue/29145.test.ts).
Implementation correctness
The Zig fix is sound: setters now normalize null/undefined to .zero and throw TypeError for other non-callable values, and all three call sites in onValkeyClose, failWithJSValue, and the onconnect microtask path guard with isCallable() before invoking the cached handler. The TypeScript declarations correctly use split accessor syntax so the getter returns ((...)=>void) | undefined while the setter accepts | null | undefined.
Test coverage
Concerns I raised earlier were addressed: (1) the test file was split into an unconditional describe block for setter/getter contract tests and a describe.skipIf(\!isDebug) block for panic-path tests; (2) the "10" string was changed to the numeric 10 to resolve against the typed overload. The third test in the panic block exercises the actual teardown code path via 127.0.0.1:1. An inline comment flags the first two panic-block tests as ineffective regression guards that do not reach onValkeyClose — a nit that does not affect the correctness of the fix.
Security risks
None. The change introduces stricter input validation (rejecting non-callable, non-null values with TypeError) and removes an unconditional callback invocation, reducing the attack surface.
Level of scrutiny
Moderate: the change is self-contained, follows the established pattern from ReadableStreamSource.setOnCloseFromJS, and has clear before/after behavior. No auth, crypto, or permissions code is touched.
There was a problem hiding this comment.
LGTM — all previous concerns addressed: split getter/setter types correctly narrow the getter to only, the test suite is split so setter/getter contract runs on all lanes with only the panic test gated behind , and the Zig fix correctly normalizes null/undefined to with guards at all call sites.
Extended reasoning...
Overview
Fixes a panic in when / is set to . Three files changed: (TypeScript declarations), (runtime fix), and (new regression tests).
Security risks
None. The fix is purely about null-safety in JS callback dispatch within the Valkey client teardown path. No auth, crypto, or permissions code is touched.
Level of scrutiny
Low-to-medium. The Zig fix is mechanical and follows the established pattern from . The TypeScript uses the TS 4.3+ split accessor syntax correctly. The fix is contained to the setter/getter/callsite guards in one struct.
Other factors
My previous reviews raised five concerns; all five were addressed in subsequent commits. The final test split (unconditional describe for setter/getter contract, for the panic-path test) is the correct approach. The pre-existing MySQL/Postgres sibling issue noted is out of scope for this PR.
Setting `onclose` or `onconnect` to `null` on a RedisClient cached the `null` value in the property slot, and the teardown path (`onValkeyClose` and `failWithJSValue`) invoked `.call(...)` on it unconditionally. The resulting TypeError was thrown and cleared during teardown, surfacing as: panic(main thread): A JavaScript exception was thrown, but it was cleared before it could be read. The published type definitions declare `onclose` and `onconnect` as `((...) => void) | null`, so assigning `null` must detach the handler. The setters now normalize `null`/`undefined` to `.zero` (matching the "unset" contract the callers already handle) and throw a TypeError for any other non-callable value. The call sites guard with `isCallable()` for defense in depth. The getters return `undefined` when the cached slot is not callable, matching the new "unset" contract.
The previous commit in this branch accidentally reverted the Zig fix from commit 31a8d83; restore it. Also update packages/bun-types/redis.d.ts so the type definitions match the new runtime contract: both properties now accept null | undefined to detach the handler, and drop the `as any` casts in the regression test that were working around the old types.
…redis Coderabbit noted the async regression test was hitting an external Redis server (localhost:6379), making it environment-dependent. Point it at 127.0.0.1:1 (always refused) with autoReconnect off and a short connectionTimeout so the client still drives the socket close handler that fires the onclose callback path, exercising the panic without needing any external server. Fails on system bun with the "exception was thrown, but it was cleared" panic; passes on the patched build.
Strengthens the `undefined` acceptance tests to verify the getter returns `undefined` after the assignment, locking the normalized readback contract alongside the no-panic behavior.
Two review nits from claude[bot]: 1. Narrow the getter types for `onconnect`/`onclose` to `F | undefined` via TypeScript 4.3+ split accessor syntax. The runtime getter never returns `null` (it filters non-callable cached values to `undefined`), so `c.onclose === null` was statically accepted but always false. The setter still accepts `null | undefined` to detach the handler. 2. Pass `10` (number) instead of `"10"` (string) for the EX seconds argument in the in-flight teardown test so the call resolves against the typed overload (`set(key, value, ex: "EX", seconds: number)`) instead of the catch-all variadic overload.
The panic ("exception was thrown but cleared before it could be read")
only surfaces on debug/ASAN builds. Release binaries silently eat the
cleared exception and pass the no-panic assertions, but the readback
tests against the new contract (getter returns undefined on detach)
would fail on system bun without the fix.
Gate the whole describe on `isDebug` so release CI lanes skip it and
the ASAN lanes (including the mechanical gate's `bun bd` build) still
run the full suite.
Claude review noted the `isDebug` gate was too broad: only the panic tests (no-throw + in-flight teardown) need debug/ASAN to surface the "exception thrown but cleared" assertion. The setter/getter contract tests (readback returns undefined, non-callable throws TypeError, valid function stores correctly) exercise behavior that is identical on release and debug, so a regression in those paths was going unseen by release lanes. Split into two describes: - "setter/getter contract" runs unconditionally - "no panic on null detach during teardown" is still gated on isDebug
Claude review noted the two `onclose/onconnect = null does not panic on close()` tests were no-ops: they construct a RedisClient without ever opening a socket, so `close()` maps to `jsDisconnect` which early- returns on `.disconnected` and never reaches `onValkeyClose` — the actual site of the panic. They passed on both buggy and fixed code. The third test (`127.0.0.1:1` + `c.set()`) does force a connection attempt that drives the socket close handler through `onValkeyClose`, so it alone is the real regression guard for the panic path.
fa629b3 to
7d1d61c
Compare
There was a problem hiding this comment.
LGTM — all prior feedback addressed; small, well-scoped panic fix following the existing ReadableStreamSource.setOnCloseFromJS pattern.
Extended reasoning...
Overview
Touches three files: src/runtime/valkey_jsc/js_valkey.zig (setter validation + isCallable() guards at three call sites), packages/bun-types/redis.d.ts (split get/set accessor types so the getter narrows to F | undefined while the setter accepts null), and a new regression test test/regression/issue/29145.test.ts.
Security risks
None. This is input validation on a JS property setter and defensive guards around callback invocation. No auth, crypto, filesystem, or network-parsing surface is touched.
Level of scrutiny
Low-to-moderate. The Zig change is ~25 effective lines and mirrors the existing ReadableStreamSource.setOnCloseFromJS shape (bun.JSError!void setter, normalize null/undefined to .zero, throw TypeError for non-callable). The call-site guards are pure defense-in-depth — the setter normalization alone fixes the panic. No CODEOWNERS entry covers valkey/redis paths.
Other factors
Every inline comment from prior review rounds (split accessor types, numeric EX argument, splitting the isDebug gate so setter/getter tests run on release lanes, removing the two ineffective never-connect panic tests) has been applied and resolved. The bug-hunting system found no issues this round. The bot-flagged potential duplicate (#28454) is a merge-ordering decision for maintainers, not a correctness concern with this diff.
|
Closing: this PR's implementation lives entirely in Zig source files that have since been removed from the tree as part of the Rust migration. The change can no longer merge cleanly and the files it edits no longer exist on If the underlying issue is still present, it will need a fresh fix against the Rust implementation. |
Fixes #29145
Repro
Cause
setOnClose/setOnConnectinsrc/valkey/js_valkey.zigcached whatever JS value the setter received (includingnull) into the property slot. The teardown paths —onValkeyCloseandfailWithJSValue— then invoked.call(...)on the cached value unconditionally. Onnull, the.call()threw a TypeError which was thrown and cleared while the socket was tearing down, surfacing as a "cleared before it could be read" panic.The published type definitions declare both properties as
((...) => void) | null, so assigningnullis documented as a supported way to detach the handler.Fix
setOnClose/setOnConnect: acceptnull/undefined(normalize to.zero, matching the "unset" contract the callers already handle), throw a TypeError for any other non-callable value — same shape asReadableStreamSource.setOnCloseFromJS.onValkeyClose,failWithJSValue, and theonconnectcall site inclientConnect: guard the cached value with.isCallable()for defense in depth.getOnClose/getOnConnect: returnundefinedwhen the cached slot is not callable, so round-trippingnullreads back asundefinedinstead of a stalenull.Verification
test/regression/issue/29145.test.ts— 11 assertions coveringnull,undefined, readback, rejecting non-callable non-null (string/number/object), and the original in-flightset() → onclose = null → close()shape. On the publishedbun@1.3.12the file panics; on this branch all cases pass.