Repository navigation
fix(valkey/redis) fix tls (includes pub/sub) - #22981
Conversation
|
Updated 6:00 PM PT - Sep 26th, 2025
❌ @cirospaciari, your commit 4363e7e has 2 failures in
🧪 To try this PR locally: bunx bun-pr 22981That installs a local version of the PR into your bun-22981 --bun |
f83d969 to
ab22a82
Compare
213b21a to
f5e4cec
Compare
WalkthroughAdds deep-clone support for SSLConfig/TLS, exposes socket error messages, integrates per-instance TLS/socket-context management into Valkey clients (cloning, lifecycle, start visibility), and refactors TLS test assets and test scaffolding to run tests for both TLS and TCP. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)src/**/*.zig📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Files:
**/*.zig📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
src/**/js_*.zig📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Files:
src/{**/js_*.zig,bun.js/api/**/*.zig}📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Files:
🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-09-05T18:45:29.200ZApplied to files:
📚 Learning: 2025-08-30T00:11:57.076ZApplied to files:
📚 Learning: 2025-09-19T21:17:04.690ZApplied to files:
📚 Learning: 2025-08-30T00:13:36.815ZApplied to files:
📚 Learning: 2025-09-08T04:44:59.101ZApplied to files:
📚 Learning: 2025-09-15T20:47:57.118ZApplied to files:
📚 Learning: 2025-09-05T19:48:44.481ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/valkey/js_valkey.zig (2)
1298-1331: Fix TLS hostname verification gating and avoid risky logging.
- The identity check currently runs only when ssl_error.error_no != 0. That’s backwards: you should fail if verification failed (error_no != 0), and independently enforce hostname verification when rejectUnauthorized is true.
- Logging reason/code unconditionally risks dereferencing non-sentinel pointers. Gate it behind error_no != 0.
Apply this diff:
- debug("onHandshake: {d} {d} {s} {s}", .{ - success, - ssl_error.error_no, - if (ssl_error.reason != null) bun.span(ssl_error.reason[0..bun.len(ssl_error.reason) :0]) else "no reason", - if (ssl_error.code != null) bun.span(ssl_error.code[0..bun.len(ssl_error.code) :0]) else "no code", - }); + if (ssl_error.error_no != 0) { + debug("onHandshake failed: success={d} err={d}", .{ success, ssl_error.error_no }); + } else { + debug("onHandshake ok: success={d}", .{ success }); + } @@ - if (handshake_success) { + if (handshake_success) { const vm = this.client.vm; - if (this.client.tls.rejectUnauthorized(vm)) { - if (ssl_error.error_no != 0) { - // only reject the connection if reject_unauthorized == true - - const ssl_ptr: *BoringSSL.c.SSL = @ptrCast(this.client.socket.getNativeHandle()); - if (BoringSSL.c.SSL_get_servername(ssl_ptr, 0)) |servername| { - const hostname = servername[0..bun.len(servername)]; - if (!BoringSSL.checkServerIdentity(ssl_ptr, hostname)) { - this.client.flags.is_authenticated = false; - const loop = vm.eventLoop(); - loop.enter(); - defer loop.exit(); - this.client.status = .failed; - this.client.flags.is_manually_closed = true; - this.client.failWithJSValue(this.globalObject, ssl_error.toJS(this.globalObject)); - this.client.close(); - return; - } - } - } - } + if (this.client.tls.rejectUnauthorized(vm)) { + // Fail on verification error + if (ssl_error.error_no != 0) { + this.client.flags.is_authenticated = false; + const loop = vm.eventLoop(); + loop.enter(); + defer loop.exit(); + this.client.status = .failed; + this.client.flags.is_manually_closed = true; + this.client.failWithJSValue(this.globalObject, ssl_error.toJS(this.globalObject)); + this.client.close(); + return; + } + // Enforce hostname check when available + const ssl_ptr: *BoringSSL.c.SSL = @ptrCast(this.client.socket.getNativeHandle()); + if (BoringSSL.c.SSL_get_servername(ssl_ptr, 0)) |servername| { + const hostname = servername[0..bun.len(servername)]; + if (!BoringSSL.checkServerIdentity(ssl_ptr, hostname)) { + this.client.flags.is_authenticated = false; + const loop = vm.eventLoop(); + loop.enter(); + defer loop.exit(); + this.client.status = .failed; + this.client.flags.is_manually_closed = true; + this.client.failWithJSValue(this.globalObject, ssl_error.toJS(this.globalObject)); + this.client.close(); + return; + } + } + } this.client.start(); }This aligns with typical TLS semantics and avoids reading potentially invalid reason/code buffers.
995-1057: Remove unpaired this.ref() in connect()
Thethis.ref()at line 1049 in src/valkey/js_valkey.zig isn’t matched by adefer this.deref(), leaking references on repeated reconnects. Apply:- this.ref(); - - if (own_ctx) { + if (own_ctx) {
🧹 Nitpick comments (3)
test/js/valkey/test-utils.ts (1)
83-92: TLS_REDIS_OPTIONS shape update: LGTM.
- tls holds Bun.file refs (cert/key/ca) as required by SSLConfig.fromJS.
- tlsPaths mirrors file paths for child processes that need to re-open files.
Consider adding serverName if hostname verification becomes enforced in tests.
Apply after TLS handshake changes land.
src/bun.js/api/server/SSLConfig.zig (1)
230-314: Deep clone looks correct; minor cleanup possible.
- Correctly deep-copies strings and key/cert/ca arrays; avoids double-free with is_using_default_ciphers.
- Minor nit: you set .protos in the initializer and also dupe it later; initializing .protos to null in the struct literal would avoid redundant assignment.
- var cloned: SSLConfig = .{ + var cloned: SSLConfig = .{ .secure_options = this.secure_options, .request_cert = this.request_cert, .reject_unauthorized = this.reject_unauthorized, .ssl_ciphers = this.ssl_ciphers, - .protos = this.protos, + .protos = null, .protos_len = this.protos_len, .client_renegotiation_limit = this.client_renegotiation_limit, .client_renegotiation_window = this.client_renegotiation_window, .requires_custom_request_ctx = this.requires_custom_request_ctx, .is_using_default_ciphers = this.is_using_default_ciphers, .low_memory_mode = this.low_memory_mode, };src/deps/uws.zig (1)
84-94: De-duplicate error strings viamessage()Now that
message()exists, we can routetoJS()through it. That keeps the human-readable text in one place and guarantees future edits stay in sync.- pub fn toJS(this: create_bun_socket_error_t, globalObject: *jsc.JSGlobalObject) jsc.JSValue { - return switch (this) { - .none => brk: { - bun.debugAssert(false); - break :brk .null; - }, - .load_ca_file => globalObject.ERR(.BORINGSSL, "Failed to load CA file", .{}).toJS(), - .invalid_ca_file => globalObject.ERR(.BORINGSSL, "Invalid CA file", .{}).toJS(), - .invalid_ca => globalObject.ERR(.BORINGSSL, "Invalid CA", .{}).toJS(), - .invalid_ciphers => globalObject.ERR(.BORINGSSL, "Invalid ciphers", .{}).toJS(), - }; - } + pub fn toJS(this: create_bun_socket_error_t, globalObject: *jsc.JSGlobalObject) jsc.JSValue { + const msg = this.message(); + if (msg == null) { + bun.debugAssert(false); + return .null; + } + return globalObject.ERR(.BORINGSSL, msg.?, .{}).toJS(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
src/bun.js/api/server/SSLConfig.zig(1 hunks)src/deps/uws.zig(1 hunks)src/valkey/js_valkey.zig(9 hunks)src/valkey/valkey.zig(3 hunks)test/js/valkey/docker-tls/Dockerfile(1 hunks)test/js/valkey/docker-tls/server.crt(1 hunks)test/js/valkey/docker-tls/server.key(1 hunks)test/js/valkey/memleak/valkey-leaks.ts(1 hunks)test/js/valkey/test-utils.ts(1 hunks)test/js/valkey/valkey.failing-subscriber.ts(3 hunks)test/js/valkey/valkey.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/valkey/docker-tls/server.crttest/js/valkey/docker-tls/server.keytest/js/valkey/valkey.test.tstest/js/valkey/memleak/valkey-leaks.tstest/js/valkey/docker-tls/Dockerfiletest/js/valkey/test-utils.tstest/js/valkey/valkey.failing-subscriber.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/valkey/docker-tls/server.crttest/js/valkey/docker-tls/server.keytest/js/valkey/valkey.test.tstest/js/valkey/memleak/valkey-leaks.tstest/js/valkey/docker-tls/Dockerfiletest/js/valkey/test-utils.tstest/js/valkey/valkey.failing-subscriber.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/deps/uws.zigsrc/bun.js/api/server/SSLConfig.zigsrc/valkey/js_valkey.zigsrc/valkey/valkey.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/deps/uws.zigsrc/bun.js/api/server/SSLConfig.zigsrc/valkey/js_valkey.zigsrc/valkey/valkey.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/SSLConfig.zigsrc/valkey/js_valkey.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/server/SSLConfig.zig
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/valkey/valkey.test.tstest/js/valkey/memleak/valkey-leaks.tstest/js/valkey/test-utils.tstest/js/valkey/valkey.failing-subscriber.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/valkey/valkey.test.tstest/js/valkey/memleak/valkey-leaks.tstest/js/valkey/test-utils.tstest/js/valkey/valkey.failing-subscriber.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/valkey/valkey.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/js/valkey/valkey.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/js/valkey/valkey.test.tstest/js/valkey/memleak/valkey-leaks.tstest/js/valkey/test-utils.tstest/js/valkey/valkey.failing-subscriber.ts
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey.zig
🧠 Learnings (10)
📓 Common learnings
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/deps/uws.zig
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/bun.js/api/server/SSLConfig.zigsrc/valkey/js_valkey.zigsrc/valkey/valkey.zig
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/valkey/valkey.test.tstest/js/valkey/memleak/valkey-leaks.tstest/js/valkey/test-utils.tstest/js/valkey/valkey.failing-subscriber.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/valkey/valkey.test.tstest/js/valkey/memleak/valkey-leaks.tstest/js/valkey/test-utils.tstest/js/valkey/valkey.failing-subscriber.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/valkey/valkey.test.tstest/js/valkey/memleak/valkey-leaks.tstest/js/valkey/test-utils.tstest/js/valkey/valkey.failing-subscriber.ts
📚 Learning: 2025-09-17T23:42:05.812Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: docs/api/redis.md:240-246
Timestamp: 2025-09-17T23:42:05.812Z
Learning: The RedisClient.duplicate() method in Bun's Redis implementation returns Promise<RedisClient>, not RedisClient synchronously, so await is required when calling it.
Applied to files:
test/js/valkey/valkey.test.tstest/js/valkey/valkey.failing-subscriber.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `using`/`await using` with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Applied to files:
test/js/valkey/memleak/valkey-leaks.ts
📚 Learning: 2025-09-15T20:47:57.118Z
Learnt from: cirospaciari
PR: oven-sh/bun#22696
File: src/sql/mysql/js/JSMySQLConnection.zig:306-322
Timestamp: 2025-09-15T20:47:57.118Z
Learning: bun.Async.KeepAlive already maintains internal state to prevent issues from multiple ref/unref calls, so additional idempotence guards are not needed when calling poll_ref.ref() and poll_ref.unref().
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-09-05T20:20:04.858Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/js_valkey.zig:163-178
Timestamp: 2025-09-05T20:20:04.858Z
Learning: Node-redis PUB/SUB callback error handling is broken - when a callback throws an error, it first emits the error as "Redis Client Error", then enters an undefined state with cascading internal errors like "undefined is not an object (evaluating 'this.#waitingForReply.head.value')", making the client unusable for further processing.
Applied to files:
test/js/valkey/valkey.failing-subscriber.ts
🧬 Code graph analysis (3)
test/js/valkey/valkey.test.ts (1)
test/js/valkey/test-utils.ts (9)
isEnabled(10-27)setupDockerContainer(203-214)createClient(229-288)expectType(495-500)TLS_REDIS_URL(123-123)DEFAULT_REDIS_URL(122-122)TLS_REDIS_OPTIONS(80-93)awaitableCounter(661-696)randomCoinFlip(654-656)
test/js/valkey/memleak/valkey-leaks.ts (2)
test/harness.ts (1)
gc(116-118)test/js/valkey/test-utils.ts (2)
isEnabled(10-27)createClient(229-288)
test/js/valkey/valkey.failing-subscriber.ts (1)
packages/bun-types/redis.d.ts (1)
RedisClient(61-815)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (15)
test/js/valkey/valkey.failing-subscriber.ts (5)
23-24: Resolving the full message object is correct.Switching to resolve(msg) lets the child receive both url and tlsPaths. Matches the updated test harness.
48-49: Off-by-one in the intentional-throw trigger?With if ((counter++) === 1), the throw happens on the second message, not the first. Confirm this is intended for the failing-subscriber scenario.
6-6: Import from "bun": OK.Using RedisClient from bun matches current API.
56-58: Uncaught exception forwarding: LGTM.Propagates errors back to the parent via IPC.
32-41: Approve TLS construction; fixture files verified. Server.crt and server.key are present and readable in test/js/valkey/docker-unified.src/valkey/valkey.zig (4)
90-96: TLS.clone for union: LGTM.Cloning custom SSLConfig avoids shared ownership and use-after-free.
467-467: Early socket detach on close: LGTM.Prevents accidental writes after close during teardown paths.
956-961: Gate start() on TCP only: LGTM.Matches the TLS flow where start() is invoked post-handshake.
964-967: start() made public: LGTM.Needed by TLS handshake callback.
src/valkey/js_valkey.zig (4)
230-231: Custom socket context retention: OK; consider lifecycle note.Holding _socket_ctx for custom TLS avoids re-creating contexts. Ensure it’s deinit’d when the client is finalized (you added deinitSocketContextNextTick) and not reused across distinct client instances inadvertently.
Verify that duplicate() does not copy _socket_ctx across instances.
366-366: TLS selection logic: LGTM.Respects options.tls when provided; otherwise infers from URL.
414-415: Cloning TLS on duplicate(): LGTM.Deep copy via valkey.TLS.clone prevents shared SSLConfig state.
1101-1124: Deferred deinit of custom TLS ctx: LGTM.Enqueuing the deinit avoids JS reentry on close paths.
test/js/valkey/docker-tls/server.crt (1)
2-32: Confirm certificate/key pairing
Automated modulus check failed (openssl unavailable). Manually verify that test/js/valkey/docker-tls/server.crt and server.key share the same modulus and subject parameters to avoid TLS handshake failures in the Docker fixtures.test/js/valkey/docker-tls/server.key (1)
1-52: Manually confirm the new key/cert pair is consistent.
Runopenssl x509 -noout -modulus -in test/js/valkey/docker-tls/server.crtandopenssl pkey -noout -modulus -in test/js/valkey/docker-tls/server.keylocally to ensure both modulus values match.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/valkey/js_valkey.zig (1)
1006-1049: Remove the directonValkeyClose()calls on context-creation failure
this.clientFail(...)already drives the close path (it closes the socket and ends up invokingJSValkeyClient.onValkeyClose()for us). Callingthis.client.onValkeyClose()here a second time—without first taking a ref—hitsJSValkeyClient.onValkeyClose()’s unconditionaldefer this.deref();(Line 909) with no matchingref(), so the refcount underflows and we free the client while it is still on the stack. This is consistent with the ASAN/segfault explosions CI is reporting for this PR. Drop these extraonValkeyClose()calls (in all three branches) and let the failure path you just triggered handle the cleanup.- this.client.onValkeyClose();(also remove the identical line in the TLS and custom branches)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/api/server/SSLConfig.zig(1 hunks)src/valkey/js_valkey.zig(10 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/api/server/SSLConfig.zigsrc/valkey/js_valkey.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/api/server/SSLConfig.zigsrc/valkey/js_valkey.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/SSLConfig.zigsrc/valkey/js_valkey.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/server/SSLConfig.zig
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey.zig
🧠 Learnings (3)
📓 Common learnings
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/bun.js/api/server/SSLConfig.zigsrc/valkey/js_valkey.zig
📚 Learning: 2025-09-15T20:47:57.118Z
Learnt from: cirospaciari
PR: oven-sh/bun#22696
File: src/sql/mysql/js/JSMySQLConnection.zig:306-322
Timestamp: 2025-09-15T20:47:57.118Z
Learning: bun.Async.KeepAlive already maintains internal state to prevent issues from multiple ref/unref calls, so additional idempotence guards are not needed when calling poll_ref.ref() and poll_ref.unref().
Applied to files:
src/valkey/js_valkey.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
test/js/valkey/valkey.test.ts (3)
209-217: Clarify test.each title placeholder.
$urlisn’t a provided param; use a positional placeholder.Apply this diff:
- test.each([...Array(16).keys()])("Connecting to database with url $url succeeds", async (dbId: number) => { + test.each([...Array(16).keys()])("Connecting to database %s succeeds", async (dbId: number) => {
534-536: Align comment with assertion (it should fail).Comment contradicts the test name and expectation.
Apply this diff:
- // Publishing from the same client should work + // Publishing from the same client should fail while in subscriber mode
671-673: Remove stray console.log from tests.Avoid noisy output in CI.
Apply this diff:
- console.log("Unsubscribing listener2");src/bun.js/api/server/SSLConfig.zig (1)
275-289: Avoid zero-length allocations for cert/key/ca clones.If count is 0, we still alloc a 0-length array, keep a non-null pointer, and later free it. It’s safe but unnecessary. Prefer leaving pointer null when count is 0 to simplify lifecycle and reduce allocations.
Apply this diff:
inline for (array_fields_cloned_by_memcopy) |field| { if (@field(this, field)) |array| { - const cloned_array = bun.handleOom(bun.default_allocator.alloc([*c]const u8, @field(this, field ++ "_count"))); - @field(cloned, field) = cloned_array; - @field(cloned, field ++ "_count") = @field(this, field ++ "_count"); + const count = @field(this, field ++ "_count"); + if (count == 0) { + @field(cloned, field) = null; + @field(cloned, field ++ "_count") = 0; + continue; + } + const cloned_array = bun.handleOom(bun.default_allocator.alloc([*c]const u8, count)); + @field(cloned, field) = cloned_array; + @field(cloned, field ++ "_count") = count; for (0..@field(this, field ++ "_count")) |i| { const slice = std.mem.span(array[i]); if (slice.len > 0) { cloned_array[i] = bun.handleOom(bun.default_allocator.dupeZ(u8, slice)); } else { cloned_array[i] = ""; } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/bun.js/api/server/SSLConfig.zig(1 hunks)src/valkey/js_valkey.zig(17 hunks)src/valkey/js_valkey_functions.zig(1 hunks)src/valkey/valkey.zig(5 hunks)test/js/valkey/test-utils.ts(1 hunks)test/js/valkey/valkey.failing-subscriber.ts(4 hunks)test/js/valkey/valkey.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/js/valkey/test-utils.ts
- test/js/valkey/valkey.failing-subscriber.ts
- src/valkey/js_valkey_functions.zig
🧰 Additional context used
📓 Path-based instructions (12)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/api/server/SSLConfig.zigsrc/valkey/valkey.zigsrc/valkey/js_valkey.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/api/server/SSLConfig.zigsrc/valkey/valkey.zigsrc/valkey/js_valkey.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/SSLConfig.zigsrc/valkey/js_valkey.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/server/SSLConfig.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/valkey/valkey.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/valkey/valkey.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/valkey/valkey.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/valkey/valkey.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/valkey/valkey.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/js/valkey/valkey.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/js/valkey/valkey.test.ts
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey.zig
🧠 Learnings (15)
📓 Common learnings
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/bun.js/api/server/SSLConfig.zigsrc/valkey/valkey.zigsrc/valkey/js_valkey.zig
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Prefer `async/await` in tests; for a single callback, use `Promise.withResolvers()`
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Explicitly assert client console logs with c.expectMessage; unasserted logs should not appear
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-17T23:42:05.812Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: docs/api/redis.md:240-246
Timestamp: 2025-09-17T23:42:05.812Z
Learning: The RedisClient.duplicate() method in Bun's Redis implementation returns Promise<RedisClient>, not RedisClient synchronously, so await is required when calling it.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-09-19T21:17:04.690Z
Learnt from: cirospaciari
PR: oven-sh/bun#22756
File: src/js/node/_http_server.ts:945-947
Timestamp: 2025-09-19T21:17:04.690Z
Learning: JSNodeHTTPServerSocket in src/bun.js/bindings/NodeHTTP.cpp implements both end() and close() methods. The end() method properly handles socket termination for both HTTP responses and raw CONNECT sockets by setting ended=true, checking buffered data, and flushing remaining data through the response context.
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In finalize() for objects holding JS references, release them using .deref() before destroy
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to **/*.zig : In Zig, manage memory carefully with allocators and use defer for cleanup
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-09-15T20:47:57.118Z
Learnt from: cirospaciari
PR: oven-sh/bun#22696
File: src/sql/mysql/js/JSMySQLConnection.zig:306-322
Timestamp: 2025-09-15T20:47:57.118Z
Learning: bun.Async.KeepAlive already maintains internal state to prevent issues from multiple ref/unref calls, so additional idempotence guards are not needed when calling poll_ref.ref() and poll_ref.unref().
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-09-05T19:48:44.481Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/valkey.zig:0-0
Timestamp: 2025-09-05T19:48:44.481Z
Learning: In ValkeyCommand.Promise, when you need to resolve with already-converted JSValues (not RESPValues), use promise_ptr.promise.resolve() to bypass the RESP-to-JS conversion wrapper. The ValkeyCommand.Promise.resolve() method is for converting RESPValues, while direct .promise access is for pre-converted JSValues.
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Applied to files:
src/valkey/js_valkey.zig
🧬 Code graph analysis (1)
test/js/valkey/valkey.test.ts (2)
test/js/valkey/test-utils.ts (9)
isEnabled(10-27)setupDockerContainer(203-214)createClient(229-289)expectType(496-501)TLS_REDIS_URL(123-123)DEFAULT_REDIS_URL(122-122)TLS_REDIS_OPTIONS(80-93)awaitableCounter(662-697)randomCoinFlip(655-657)test/js/valkey/valkey.failing-subscriber.ts (1)
RedisTestStartMessage(16-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (19)
test/js/valkey/valkey.test.ts (4)
18-20: Fix TLS/TCP ctx selection (enum truthiness bug).The ternary routes TLS to the TCP client. Compare explicitly against the enum.
Apply this diff:
- const ctx = { ..._ctx, redis: connectionType ? _ctx.redis : _ctx.redisTLS }; + const ctx = { + ..._ctx, + redis: connectionType === ConnectionType.TLS ? _ctx.redisTLS : _ctx.redis, + };
285-291: Await the async matcher.The assertion never runs if you don’t await it.
Apply this diff:
- expect(ctx.redis.set(testKey(), testValue())).resolves.toEqual("OK"); + await expect(ctx.redis.set(testKey(), testValue())).resolves.toEqual("OK");
299-304: Fix variable shadowing in subscribe callback.The expectations compare values to themselves and always pass.
Apply this diff:
- await subscriber.subscribe(channel, (message, channel) => { - counter.increment(); - expect(channel).toBe(channel); - expect(message).toBe(message); - }); + await subscriber.subscribe(channel, (payload, receivedChannel) => { + counter.increment(); + expect(receivedChannel).toBe(channel); + expect(payload).toBe(message); + });
203-206: Use rejects; toThrow won’t catch async errors.
toThrowErrorMatchingInlineSnapshoton an async function won’t assert the rejection.Apply this diff:
-expect(async () => { - await customRedis.get("test"); -}).toThrowErrorMatchingInlineSnapshot(`"WRONGPASS invalid username-password pair or user is disabled."`); +await expect(customRedis.get("test")).rejects.toThrow( + "WRONGPASS invalid username-password pair or user is disabled.", +);⛔ Skipped due to learnings
Learnt from: markovejnovic PR: oven-sh/bun#22568 File: test/js/valkey/valkey.test.ts:561-564 Timestamp: 2025-09-20T00:58:38.042Z Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.Learnt from: markovejnovic PR: oven-sh/bun#22568 File: test/js/valkey/valkey.test.ts:268-276 Timestamp: 2025-09-20T00:57:56.685Z Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.Learnt from: CR PR: oven-sh/bun#0 File: test/CLAUDE.md:0-0 Timestamp: 2025-09-03T17:10:13.486Z Learning: Applies to test/**/*.test.ts : In error tests, assert non-zero exit codes for failing processes and use `toThrow` for synchronous errorsLearnt from: CR PR: oven-sh/bun#0 File: test/CLAUDE.md:0-0 Timestamp: 2025-09-03T17:10:13.486Z Learning: Applies to test/**/*.test.ts : Prefer `async/await` in tests; for a single callback, use `Promise.withResolvers()`src/valkey/valkey.zig (5)
90-96: TLS.clone() looks correct.Clones custom SSLConfig via deep clone and preserves other variants by value. Matches ownership expectations.
467-469: Detaching socket and marking disconnected on close is appropriate.Ensures a consistent state before reconnection logic triggers.
957-961: Gate start() on TCP; defer for TLS until handshake.This prevents sending HELLO before TLS handshake completes. Good fix for TLS hangs.
965-968: Making start() public is reasonable.Enables invocation from TLS handshake handler while maintaining the same flow.
1080-1092: Queuing during connecting/disconnected is a safer default.Disallowing immediate writes while connecting (esp. TLS) avoids pre-handshake writes. Ensure tests cover offline-queue semantics for connecting.
Please confirm test coverage for:
- commands enqueued while .connecting (both pipelinable and non-pipelinable),
- transition to .connected drains correctly without ordering regressions.
src/valkey/js_valkey.zig (9)
230-231: Per-instance SocketContext storage LGTM.Storing custom TLS contexts per client avoids re-creation and supports proper lifecycle via deferred deinit.
366-366: TLS option resolution matches URL semantics.Prefers explicit options.tls, otherwise infers from URI. Correct for rediss/valkeys.
413-415: Cloning TLS into duplicates is the right call.Avoids shared ownership/lifetime pitfalls for SSLConfig contents.
760-767: Deferred onWritable+updatePollRef after connect is good.Flushes pending work and refreshes lifecycle without blocking the event loop.
773-780: Hello value conversion with exception cleanup is correct.Catching toJS error and clearing exception keeps behavior consistent and avoids leaking pending exceptions.
1105-1126: Deferred deinit of custom SocketContext looks good.Enqueueing deinit avoids JS reentrancy during teardown. Ownership/path are clear.
1303-1308: TLS handshake flow: verify assumptions.Starting only after handshake success is correct. Please confirm
ssl_error.error_nosemantics: if it’s non-zero only for hostname issues, current check is fine. If it can indicate other verification failures, we should reject regardless when rejectUnauthorized is true.Can you confirm what
us_bun_verify_error_t.error_norepresents across uWS/BoringSSL for TLS client sockets and whether non-hostname verification failures can surface withsuccess == 1?Also applies to: 1335-1336
912-916: Fix ref/deref imbalance in onValkeyClose().There’s a deferred deref without a matching ref; can underflow refcount. Add a ref before the defer block.
Apply this diff:
- defer { + this.ref(); + defer { // Update poll reference to allow garbage collection of disconnected clients this.updatePollRef(); this.deref(); }
1054-1062: Remove unbalanced ref() in connect().
this.ref()here has no matchingderef()on all paths and can leak. Liveness is already guarded by poll_ref and JS refs during connect.Apply this diff:
- this.ref(); - if (own_ctx) { // save the context so we deinit it later (if we reconnect we can reuse the same context) this._socket_ctx = ctx; } this.client.status = .connecting; this.updatePollRef();src/bun.js/api/server/SSLConfig.zig (1)
252-261: Default cipher pointer lost on clone() (changes TLS behavior).When
is_using_default_ciphersis true,cloned.ssl_ciphersstays null, soasUSockets()won’t forward Bun’s curated default ciphers. Clones then fall back to uWS/OpenSSL defaults, potentially breaking TLS interoperability versus the source config. Copy the pointer in the default case (no ownership transfer) to preserve behavior.Apply this diff:
if (!this.is_using_default_ciphers) { if (this.ssl_ciphers) |slice_ptr| { const slice = std.mem.span(slice_ptr); if (slice.len > 0) { cloned.ssl_ciphers = bun.handleOom(bun.default_allocator.dupeZ(u8, slice)); } else { cloned.ssl_ciphers = null; } } + } else { + // Preserve curated defaults without taking ownership + cloned.ssl_ciphers = this.ssl_ciphers; }
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/valkey/js_valkey.zig(16 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/valkey/js_valkey.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/valkey/js_valkey.zig
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/valkey/js_valkey.zig
🧠 Learnings (7)
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-09-19T21:17:04.690Z
Learnt from: cirospaciari
PR: oven-sh/bun#22756
File: src/js/node/_http_server.ts:945-947
Timestamp: 2025-09-19T21:17:04.690Z
Learning: JSNodeHTTPServerSocket in src/bun.js/bindings/NodeHTTP.cpp implements both end() and close() methods. The end() method properly handles socket termination for both HTTP responses and raw CONNECT sockets by setting ended=true, checking buffered data, and flushing remaining data through the response context.
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In finalize() for objects holding JS references, release them using .deref() before destroy
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to **/*.zig : In Zig, manage memory carefully with allocators and use defer for cleanup
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-09-15T20:47:57.118Z
Learnt from: cirospaciari
PR: oven-sh/bun#22696
File: src/sql/mysql/js/JSMySQLConnection.zig:306-322
Timestamp: 2025-09-15T20:47:57.118Z
Learning: bun.Async.KeepAlive already maintains internal state to prevent issues from multiple ref/unref calls, so additional idempotence guards are not needed when calling poll_ref.ref() and poll_ref.unref().
Applied to files:
src/valkey/js_valkey.zig
📚 Learning: 2025-09-05T19:48:44.481Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/valkey.zig:0-0
Timestamp: 2025-09-05T19:48:44.481Z
Learning: In ValkeyCommand.Promise, when you need to resolve with already-converted JSValues (not RESPValues), use promise_ptr.promise.resolve() to bypass the RESP-to-JS conversion wrapper. The ValkeyCommand.Promise.resolve() method is for converting RESPValues, while direct .promise access is for pre-converted JSValues.
Applied to files:
src/valkey/js_valkey.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
markovejnovic
left a comment
markovejnovic
left a comment
There was a problem hiding this comment.
lgtm, feel free to merge
4 participants
What does this PR do?
Fix tls property not being properly set
Fixes #22186
How did you verify your code works?
Tests + Manually test with upstash using
redissprotocol and tls: true options