Repository navigation
fix(Bun.SQL) fix SSLRequest - #22378
Conversation
|
Updated 5:14 PM PT - Sep 3rd, 2025
❌ @autofix-ci[bot], your commit adc1540 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 22378That installs a local version of the PR into your bun-22378 --bun |
af16b67 to
e09d73c
Compare
WalkthroughImplements MySQL TLS negotiation and mid-handshake upgrade (CLIENT_SSL/SSLRequest, upgradeToTLS), integrates TLS sequencing (sequence_id), updates TLS status variants, adjusts password send over TLS, adds TLS test infrastructure and tests, changes event-loop log visibility, and reformats a CMake source list. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/sql/mysql/MySQLConnection.zig (3)
914-925: Incorrect error message says “postgresql”; should be “mysql”.This leaks a wrong product name on Unix socket connection failures.
Apply:
- return globalObject.throwError(err, "failed to connect to postgresql"); + return globalObject.throwError(err, "failed to connect to mysql");
1011-1037: Hostname verification error path uses ssl_error even when it’s zero.On hostname mismatch, ssl_error may be 0; returning it produces a misleading “no error”. Emit a clear failure.
Apply:
- if (BoringSSL.c.SSL_get_servername(ssl_ptr, 0)) |servername| { + if (BoringSSL.c.SSL_get_servername(ssl_ptr, 0)) |servername| { const hostname = servername[0..bun.len(servername)]; if (!BoringSSL.checkServerIdentity(ssl_ptr, hostname)) { - this.tls_status = .ssl_failed; - return this.failWithJSValue(ssl_error.toJS(this.globalObject)); + this.tls_status = .ssl_failed; + return this.failFmt(error.AuthenticationFailed, "TLS hostname verification failed for {s}", .{hostname}); } }
782-829: Default SNI/server_name should derive from hostname when not provided.Without SNI, verify_full may silently skip hostname checks. Set server_name to the connection hostname if unset.
Apply:
- if (ssl_mode != .disable) { + if (ssl_mode != .disable) { tls_config = if (tls_object.isBoolean() and tls_object.toBoolean()) .{} else if (tls_object.isObject()) (jsc.API.ServerConfig.SSLConfig.fromJS(vm, globalObject, tls_object) catch return .zero) orelse .{} else { return globalObject.throwInvalidArguments("tls must be a boolean or an object", .{}); }; @@ - uws.NewSocketHandler(true).configure(tls_ctx.?, true, *@This(), SocketHandler(true)); + uws.NewSocketHandler(true).configure(tls_ctx.?, true, *@This(), SocketHandler(true)); } @@ - const hostname = hostname_str.toUTF8(bun.default_allocator); + const hostname = hostname_str.toUTF8(bun.default_allocator); defer hostname.deinit(); + if (ssl_mode != .disable and tls_config.server_name.len == 0) { + // Use SNI for hostname verification if caller didn't provide one. + tls_config.server_name = hostname.slice(); + }Also applies to: 903-939
🧹 Nitpick comments (8)
test/js/sql/mysql-tls/ssl/ca.srl (1)
1-1: Prevent repo-dirty test runs: ensure ca.srl isn’t mutated on hostIf OpenSSL signs using -CAcreateserial/-CAserial against this path at test time, it will bump the serial and dirty the working tree. Please confirm signing happens only inside the Docker image or a temp dir, or copy this file into an ephemeral location before use.
test/js/sql/mysql-tls/ssl/server-ext.cnf (1)
1-4: SANs look good for mysql/localhost/127.0.0.1; consider adding IPv6 loopbackOptional hardening for environments where “localhost” resolves to ::1.
Apply this diff:
-subjectAltName = DNS:mysql, DNS:localhost, IP:127.0.0.1 +subjectAltName = DNS:mysql, DNS:localhost, IP:127.0.0.1, IP:::1cmake/sources/ZigSources.txt (1)
1-1: Normalize cmake/sources/ZigSources.txt to LF line endings
Converted all CRLF to LF to resolve the “LF will be replaced by CRLF” CI warning. Optionally enforce LF via .gitattributes:+.gitattributes +cmake/sources/ZigSources.txt text eol=lftest/js/sql/mysql-tls/Dockerfile (1)
22-22: Add missing newline at end of file.The Dockerfile should end with a newline for consistency and best practices.
EXPOSE 3306 +src/sql/mysql/protocol/SSLRequest.zig (1)
35-35: Consider consistent log scope naming.The debug log uses
.MySQLConnectionscope, which might be confusing since this is in theSSLRequestmodule. Consider using a more specific scope like.MySQLSSLRequestor keep it consistent with the connection scope if that's the intended behavior.-const debug = bun.Output.scoped(.MySQLConnection, .hidden); +const debug = bun.Output.scoped(.MySQLSSLRequest, .hidden);test/js/sql/sql-mysql.test.ts (2)
189-209: Remove explicit per-test timeout.Guidelines: do not set explicit test timeouts; rely on Bun’s built-in timeouts.
Apply:
- }, 10_000); + });
275-286: Avoid global table name collisions; prefer TEMPORARY or randomized names.Multiple tests create/drop a table named "test". Under parallel execution this races. Use TEMPORARY or a unique name per test.
If TEMPORARY isn’t viable, do:
- await sql`create table test(id int)`; - await sql`drop table test`; + const tn = sql("t_" + randomUUIDv7("hex").replaceAll("-", "")); + await sql`create table ${tn}(id int)`; + await sql`drop table ${tn}`;Also applies to: 280-286
src/sql/mysql/MySQLConnection.zig (1)
826-828: Nit: reduce noisy TLS logs in non-debug runs.Consider
.visibleonly when a verbose flag is enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
test/js/sql/mysql-tls/ssl/ca-key.pemis excluded by!**/*.pemtest/js/sql/mysql-tls/ssl/ca.pemis excluded by!**/*.pemtest/js/sql/mysql-tls/ssl/server-cert.pemis excluded by!**/*.pemtest/js/sql/mysql-tls/ssl/server-key.pemis excluded by!**/*.pem
📒 Files selected for processing (12)
cmake/sources/ZigSources.txt(1 hunks)src/bun.js/event_loop.zig(1 hunks)src/sql/mysql/MySQLConnection.zig(11 hunks)src/sql/mysql/TLSStatus.zig(1 hunks)src/sql/mysql/protocol/HandshakeResponse41.zig(2 hunks)src/sql/mysql/protocol/SSLRequest.zig(1 hunks)test/js/sql/mysql-tls/Dockerfile(1 hunks)test/js/sql/mysql-tls/conf.d/ssl.cnf(1 hunks)test/js/sql/mysql-tls/ssl/ca.srl(1 hunks)test/js/sql/mysql-tls/ssl/server-ext.cnf(1 hunks)test/js/sql/mysql-tls/ssl/server.csr(1 hunks)test/js/sql/sql-mysql.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
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("...", .{})In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Files:
src/sql/mysql/TLSStatus.zigsrc/sql/mysql/protocol/HandshakeResponse41.zigsrc/bun.js/event_loop.zigsrc/sql/mysql/protocol/SSLRequest.zigsrc/sql/mysql/MySQLConnection.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.JSValueRun zig-format to format Zig files (bun run zig-format)
Files:
src/sql/mysql/TLSStatus.zigsrc/sql/mysql/protocol/HandshakeResponse41.zigsrc/bun.js/event_loop.zigsrc/sql/mysql/protocol/SSLRequest.zigsrc/sql/mysql/MySQLConnection.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/sql/mysql-tls/ssl/server.csrtest/js/sql/mysql-tls/Dockerfiletest/js/sql/mysql-tls/ssl/server-ext.cnftest/js/sql/mysql-tls/conf.d/ssl.cnftest/js/sql/mysql-tls/ssl/ca.srltest/js/sql/sql-mysql.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/sql/mysql-tls/ssl/server.csrtest/js/sql/mysql-tls/Dockerfiletest/js/sql/mysql-tls/ssl/server-ext.cnftest/js/sql/mysql-tls/conf.d/ssl.cnftest/js/sql/mysql-tls/ssl/ca.srltest/js/sql/sql-mysql.test.ts
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/event_loop.zig
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/sql/sql-mysql.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/sql/sql-mysql.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Tests must be named with the suffix .test.ts or .test.tsx and live under the test/ directory
In tests, always use port: 0 and never hardcode ports or use custom random-port utilities
Prefer snapshot tests using normalizeBunSnapshot(...) over direct string equality on stdout/stderr
Do not write tests that assert absence of 'panic', 'uncaught exception', or similar strings in output
Avoid shelling out to tools like find or grep in tests; use Bun.Glob and built-in utilities
Files:
test/js/sql/sql-mysql.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Run Prettier to format JS/TS files (bun run prettier)
Files:
test/js/sql/sql-mysql.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/sql/sql-mysql.test.ts
🧠 Learnings (18)
📚 Learning: 2025-08-30T00:07:54.573Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-08-30T00:07:54.573Z
Learning: Applies to src/**/*.zig : Implement debug logs in Zig using `const log = bun.Output.scoped(.${SCOPE}, false);` and invoking `log("...", .{})`
Applied to files:
src/bun.js/event_loop.zig
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/js/sql/sql-mysql.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/js/sql/sql-mysql.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T17:09:28.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-03T17:09:28.100Z
Learning: Applies to test/js/bun/** : Place Bun-specific API tests (http, crypto, ffi, shell, etc.) under test/js/bun/
Applied to files:
test/js/sql/sql-mysql.test.tscmake/sources/ZigSources.txt
📚 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/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/js/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
Learning: Applies to test/**/*.test.ts : Use `tempDir`/`tempDirWithFiles` from `harness` for temporary files and directories in tests
Applied to files:
test/js/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
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/sql/sql-mysql.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} : Import testing utilities (devTest, prodTest, devAndProductionTest, Dev, Client) from test/bake/bake-harness.ts
Applied to files:
test/js/sql/sql-mysql.test.ts
📚 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 Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
cmake/sources/ZigSources.txt
📚 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 : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
cmake/sources/ZigSources.txt
📚 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 : For properties marked cache: true, use the generated Zig accessors (<Prop>NameSetCached/GetCached) to work with GC-owned values
Applied to files:
cmake/sources/ZigSources.txt
📚 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:
cmake/sources/ZigSources.txt
📚 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/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
cmake/sources/ZigSources.txt
📚 Learning: 2025-09-03T17:09:28.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-03T17:09:28.101Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache class structures in ZigGlobalObject for bindings
Applied to files:
cmake/sources/ZigSources.txt
📚 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 : Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Applied to files:
cmake/sources/ZigSources.txt
📚 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 : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Applied to files:
cmake/sources/ZigSources.txt
🧬 Code graph analysis (1)
test/js/sql/sql-mysql.test.ts (1)
test/harness.ts (2)
dockerExe(855-857)describeWithContainer(887-967)
🪛 GitHub Actions: autofix.ci
cmake/sources/ZigSources.txt
[warning] 1-1: LF will be replaced by CRLF the next time Git touches it.
🔇 Additional comments (19)
cmake/sources/ZigSources.txt (1)
919-938: New MySQL TLS sources are listedSSLRequest.zig and TLSStatus.zig entries are present here, so builds should pick them up.
test/js/sql/mysql-tls/ssl/server.csr (1)
1-26: CSR fixture is fine to commitContains only public data; aligns with SANs provided via ext file. No action needed.
src/bun.js/event_loop.zig (1)
100-103: Quieter default logging for EventLoopSwitching the scope to hidden reduces noise (even in debug). Looks intentional given the volume of EventLoop logs.
src/sql/mysql/TLSStatus.zig (2)
7-7: LGTM! Simplified status variant aligns with state management.The removal of the payload from
message_sentmakes sense as the status itself indicates completion without needing to track partial progress.
10-10: LGTM! New ssl_failed variant improves error handling.Adding
ssl_failedprovides better granularity for TLS negotiation outcomes, allowing the connection to differentiate between SSL unavailability and SSL failure scenarios.test/js/sql/mysql-tls/conf.d/ssl.cnf (1)
1-7: LGTM! Comprehensive TLS configuration for test environment.The MySQL configuration properly enforces TLS with appropriate certificate paths and TLS versions. The
skip_name_resolve=ONsetting is a good practice for test environments to avoid DNS resolution issues.src/sql/mysql/protocol/HandshakeResponse41.zig (3)
11-11: LGTM! Adding sequence_id field for proper MySQL protocol sequencing.The
sequence_idfield is necessary for MySQL protocol compliance, especially during TLS handshake sequences where packet ordering matters.
28-28: LGTM! Using dynamic sequence_id instead of hardcoded value.Replacing the hardcoded
1withthis.sequence_idallows proper packet sequencing during the MySQL handshake, which is critical for TLS upgrade scenarios.
35-35: LGTM! Enhanced debug logging includes sequence_id.Adding
sequence_idto the debug output will help with debugging TLS handshake sequences and packet ordering issues.test/js/sql/mysql-tls/Dockerfile (1)
16-19: LGTM! Proper security-focused permission setup.The permission settings are correct for MySQL TLS:
chmod 600on the private key restricts access to owner only (required by MySQL)chmod 640on PEM files allows MySQL to read certificates- Setting ownership to
mysql:mysqlensures the MySQL daemon can access the filessrc/sql/mysql/protocol/SSLRequest.zig (2)
9-9: LGTM! Empty deinit is appropriate for this struct.Since
SSLRequestdoesn't allocate any dynamic memory or resources, an emptydeinitfunction is correct.
11-31: LGTM! SSLRequest implementation follows MySQL protocol correctly.The implementation properly follows the MySQL SSL request packet format:
- Correct packet structure with capabilities, max packet size, character set, and padding
- Proper conditional setting of
CLIENT_CONNECT_ATTRSbased onhas_connection_attributes- Appropriate debug logging for troubleshooting
- Fixed sequence ID of 1 is correct for SSL request packets
The 23 bytes of padding aligns with the MySQL protocol specification.
test/js/sql/sql-mysql.test.ts (2)
57-68: LGTM: CRUD + affectedRows/lastInsertRowid and long-results flow.Good coverage and use of reserve() with idleTimeout; exercises pipeline and backpressure paths.
Also applies to: 189-201
868-892: LGTM: data-driven timeout key mapping.Nice use of test.each and local inert server; this validates alias keys consistently.
src/sql/mysql/MySQLConnection.zig (5)
258-265: LGTM: upgrade to TLS right after SSLRequest flush if no backpressure.This sequencing avoids interleaving plaintext after the SSLRequest and aligns with the protocol.
982-996: LGTM: socket upgrade preserves connection state.Clean swap from TCP to TLS; onOpen guard below prevents status thrash.
1487-1541: LGTM: HandshakeResponse41 sequencing and capability handling.Including sequence_id and deferring attrs mirrors mysql2 behavior; write/flush order looks correct.
1381-1387: LGTM: send plaintext password over TLS for caching_sha2_password continue_auth.Correct per protocol; .writeZ ensures NUL-terminated payload.
323-343: LGTM: flushData backpressure handling.Accurate wrote/consume semantics and monitor hook; no busy loop risk.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sql/mysql/MySQLConnection.zig (1)
915-929: Wrong service name in error message (user-facing)On Unix socket failure, message says “postgresql”. Should be “mysql”.
Apply:
- return globalObject.throwError(err, "failed to connect to postgresql"); + return globalObject.throwError(err, "failed to connect to mysql");
🧹 Nitpick comments (4)
src/sql/mysql/MySQLConnection.zig (4)
826-826: Add context to TLS init log (nit)Include
server_name/mode to ease triage.Apply:
- debug("configured TLS context", .{}); + debug("configured TLS context (server_name: {s}, mode: {s})", .{ this.tls_config.server_name, @tagName(this.ssl_mode) });
982-997: upgradeToTLS: assert TLS context presence (nit)
Add a debug assert to catch unexpected nulltls_ctx.Apply:
pub fn upgradeToTLS(this: *MySQLConnection) void { + bun.assert(this.tls_ctx != null); if (this.socket == .SocketTCP) {
1012-1049: TLS handshake success path — add hostname fallback for verify_full (optional)If
SSL_get_servernameis null (no SNI),.verify_fullcurrently skips hostname verification. Consider falling back tothis.tls_config.server_nameto enforce host checks.Apply:
- if (BoringSSL.c.SSL_get_servername(ssl_ptr, 0)) |servername| { - const hostname = servername[0..bun.len(servername)]; - if (!BoringSSL.checkServerIdentity(ssl_ptr, hostname)) { + if (BoringSSL.c.SSL_get_servername(ssl_ptr, 0)) |servername| { + const hostname = servername[0..bun.len(servername)]; + if (!BoringSSL.checkServerIdentity(ssl_ptr, hostname)) { + this.tls_status = .ssl_failed; + return this.failWithJSValue(ssl_error.toJS(this.globalObject)); + } + } else if (this.tls_config.server_name.len > 0) { + const hostname = this.tls_config.server_name; + if (!BoringSSL.checkServerIdentity(ssl_ptr, hostname)) { this.tls_status = .ssl_failed; return this.failWithJSValue(ssl_error.toJS(this.globalObject)); } }Note: Keep
.requiresame as.preferper project intent. I used the shared learning from this PR thread when reviewing this choice.
1491-1491: Include sequence id in debug (nit)Apply:
- debug("sendHandshakeResponse", .{}); + debug("sendHandshakeResponse (seq={d})", .{ this.sequence_id });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/sql/mysql/MySQLConnection.zig(11 hunks)src/sql/postgres/PostgresSQLConnection.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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("...", .{})In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Files:
src/sql/postgres/PostgresSQLConnection.zigsrc/sql/mysql/MySQLConnection.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.JSValueRun zig-format to format Zig files (bun run zig-format)
Files:
src/sql/postgres/PostgresSQLConnection.zigsrc/sql/mysql/MySQLConnection.zig
🧠 Learnings (1)
📚 Learning: 2025-09-03T22:12:26.851Z
Learnt from: cirospaciari
PR: oven-sh/bun#22378
File: src/sql/mysql/MySQLConnection.zig:0-0
Timestamp: 2025-09-03T22:12:26.851Z
Learning: In Bun's MySQL SSL implementation, the .require SSL mode is designed to behave the same as .prefer for postgres.js compatibility, allowing graceful fallback to non-SSL connections when the server doesn't support SSL. Only .verify_ca and .verify_full modes will fail when SSL is not available.
Applied to files:
src/sql/postgres/PostgresSQLConnection.zigsrc/sql/mysql/MySQLConnection.zig
🔇 Additional comments (7)
src/sql/postgres/PostgresSQLConnection.zig (1)
446-448: Treating.requirelike.prefer— acknowledged and aligned with intentMatches the stated compatibility goal (postgres.js). No further action.
src/sql/mysql/MySQLConnection.zig (6)
256-265: TLS upgrade gating in drain path looks correctUpgrading only when
tls_status == .message_sentand no backpressure prevents interleaving pre-TLS traffic. Good sequencing.
999-1007: Avoids resetting state on post-upgrade onOpen — goodSetting
.handshakingonly for TCP prevents clobbering state when uWS re-enters on TLS. LGTM.
1220-1249: SSLRequest → TLS upgrade sequencing looks correct
- Send SSLRequest
- mark
tls_status = .message_sent- flush; upgrade if no backpressure
- gracefully handle servers without CLIENT_SSL, only failing in
.verify_ca/.verify_full(intended “require==prefer” behavior).LGTM.
1524-1524: Sequencing after TLS — goodUsing
this.sequence_idaligns with SSLRequest(seq=1) → HandshakeResponse(seq=2) ordering.
2004-2004: New import wiredImporting
SSLRequesthere matches the new handshake flow. LGTM.
1386-1391: writeZ exists on NewWriter—ignore suggested fallback
this.writer()returns aNewWriterwhich definespub fn writeZ(...), so the build will succeed and no manual null-terminator fallback is needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/js/sql/sql.test.ts (4)
22-32: Avoid custom random-port helper; let Docker pick a free portTests must not use custom random-port utilities. Map a random host port with -p 0:5432 and then read it via docker port.
-async function findRandomPort() { - return new Promise<number>((resolve, reject) => { - const server = net.createServer(); - server.listen(0, () => { - const port = (server.address() as import("node:net").AddressInfo).port; - server.close(() => resolve(port)); - }); - server.on("error", reject); - }); -} +// no-op: rely on Docker's -p 0:5432 and inspect mapped portAnd in startContainer():
- const port = await findRandomPort(); - const containerName = `postgres-test-${port}`; + const containerName = `postgres-test-${Date.now()}`; ... - await execAsync(`${dockerCLI} run -d --name ${containerName} -p ${port}:5432 custom-postgres`); + await execAsync(`${dockerCLI} run -d --name ${containerName} -p 0:5432 custom-postgres`); + const { stdout: portMap } = await execAsync(`${dockerCLI} port ${containerName} 5432/tcp`); + const port = Number(portMap.trim().split(":").pop());
85-88: Don’t call process.exit(1) in testsExiting the process masks failures and kills the runner. Throw the error so the test fails normally.
- } catch (error) { - console.error("Error:", error); - process.exit(1); - } + } catch (error) { + console.error("PostgreSQL container startup failed:", error); + throw error; + }
151-154: Use explicit host and container port in proxy, not login objectlogin.host is undefined and login is declared later. Use localhost and container.port directly.
- const containerSocket = net.createConnection({ - host: login.host, - port: login.port, - }); + const containerSocket = net.createConnection({ + host: "127.0.0.1", + port: container.port, + });
718-718: Typo causes runtime failure: savepoit → savepointThis test will throw because savepoit doesn’t exist.
- await sql.savepoit("watpoint", async sql => { + await sql.savepoint("watpoint", async sql => {
♻️ Duplicate comments (3)
test/js/sql/sql-mysql.test.ts (3)
13-38: Fix: move/guard Docker build; remove top-level await/expect to avoid crashing test discoveryTop-level async work and assertions (
expect(await dockerProcess.exited).toBe(0)) can break test discovery and make the entire file fail to load. Build the TLS image synchronously and gate TLS tests without asserting at module scope. Keep non‑TLS MySQL tests runnable even if TLS build fails.Apply:
-const docker = isDockerEnabled() ? dockerExe() : null; -if (docker) { - const dockerfilePath = path.join(import.meta.dir, "mysql-tls", "."); - console.log("Building Docker image..."); - const dockerProcess = Bun.spawn([docker, "build", "-t", "mysql-tls", dockerfilePath], { - cwd: path.join(import.meta.dir, "mysql-tls"), - }); - expect(await dockerProcess.exited).toBe(0); - console.log("Docker image built"); - const images = [ - { - name: "MySQL with TLS", - image: "mysql-tls", - env: { - MYSQL_ROOT_PASSWORD: "bun", - }, - }, - { - name: "MySQL", - image: "mysql:8", - env: { - MYSQL_ROOT_PASSWORD: "bun", - }, - }, - ]; +const docker = dockerExe(); +const images: { + name: string; + image: string; + env: Record<string, string>; +}[] = [ + { + name: "MySQL", + image: "mysql:8", + env: { MYSQL_ROOT_PASSWORD: "bun" }, + }, +]; +if (isDockerEnabled() && docker) { + const dockerfilePath = path.join(import.meta.dir, "mysql-tls"); + const { exitCode } = Bun.spawnSync({ + cmd: [docker, "build", "-t", "mysql-tls", dockerfilePath], + cwd: dockerfilePath, + stdout: "inherit", + stderr: "inherit", + }); + if (exitCode === 0) { + images.push({ + name: "MySQL with TLS", + image: "mysql-tls", + env: { MYSQL_ROOT_PASSWORD: "bun" }, + }); + } else { + console.warn(`Skipping TLS tests: docker build failed (exit: ${exitCode})`); + } }
91-112: Replace external dependency in timeout test with a local inert serverRelying on example.com is flaky and slow; use a local TCP server that never speaks MySQL to trigger connection timeout deterministically.
Apply:
- await using sql = new SQL({ - ...options, - hostname: "example.com", - connection_timeout: 4, - onconnect, - onclose, - max: 1, - }); + const server = net.createServer().listen(); + const port2 = (server.address() as import("node:net").AddressInfo).port; + await using sql = new SQL({ + adapter: "mysql", + host: "127.0.0.1", + port: port2, + connection_timeout: 4, + onconnect, + onclose, + max: 1, + }); let error: any; try { await sql`select SLEEP(8)`; } catch (e) { error = e; } expect(error.code).toBe(`ERR_MYSQL_CONNECTION_TIMEOUT`); - expect(error.message).toContain("Connection timeout after 4s"); + expect(error.message).toMatch(/after 4s/); expect(onconnect).not.toHaveBeenCalled(); expect(onclose).toHaveBeenCalledTimes(1); + server.close();
893-896: Fix test to assert instead of returning a tupleReturning a tuple performs no assertion and can pass spuriously.
Apply:
- test("Array returns rows as arrays of columns", async () => { - await using sql = new SQL(options); - return [(await sql`select CAST(1 AS SIGNED) as x`.values())[0][0], 1]; - }); + test("Array returns rows as arrays of columns", async () => { + await using sql = new SQL(options); + expect((await sql`select CAST(1 AS SIGNED) as x`.values())[0][0]).toBe(1); + });
🧹 Nitpick comments (5)
test/harness.ts (1)
16-16: Use execFileSync and consolidate child_process importsAvoid shell parsing/quoting issues and keep built-in imports consistent with node: specifier.
-import { execSync } from "child_process"; +import { execFileSync } from "node:child_process";Also consider merging the existing import on Line 11 into the same node:child_process import for consistency.
test/js/sql/sql.test.ts (3)
3-3: Use harness.dockerExe() and drop ad-hoc Docker detectionSince you already import harness helpers, prefer dockerExe() over Bun.which(). It centralizes platform quirks (e.g., docker.exe on Windows).
-import { bunEnv, bunExe, isCI, isDockerEnabled, tempDirWithFiles } from "harness"; +import { bunEnv, bunExe, isCI, isDockerEnabled, dockerExe, tempDirWithFiles } from "harness";And later:
-const dockerCLI = Bun.which("docker") as string; +const dockerCLI = dockerExe();
133-137: Avoid mixed ESM/CommonJS and duplicate importsYou already import path and net at top. Import fs and os once at top and remove these requires to prevent shadowing.
- const net = require("node:net"); - const fs = require("node:fs"); - const path = require("node:path"); - const os = require("node:os"); + // reuse top-level imports; add once at top of file: + // import fs from "node:fs"; + // import os from "node:os";
7-7: Optional: keep child_process usage minimalexec is fine here, but consider using $ from Bun for consistency, or keep execAsync only where needed (Docker CLI).
test/js/sql/sql-mysql.test.ts (1)
189-210: Remove explicit per-test timeout; follow test guidelinesAvoid setting numeric timeouts; rely on Bun’s built-in timeouts and wait for conditions instead.
Apply:
- }, 10_000); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
cmake/sources/ZigSources.txt(1 hunks)test/harness.ts(2 hunks)test/js/sql/sql-mysql.test.ts(2 hunks)test/js/sql/sql.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmake/sources/ZigSources.txt
🧰 Additional context used
📓 Path-based instructions (7)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/sql/sql.test.tstest/harness.tstest/js/sql/sql-mysql.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/sql/sql.test.tstest/js/sql/sql-mysql.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/sql/sql.test.tstest/harness.tstest/js/sql/sql-mysql.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Tests must be named with the suffix .test.ts or .test.tsx and live under the test/ directory
In tests, always use port: 0 and never hardcode ports or use custom random-port utilities
Prefer snapshot tests using normalizeBunSnapshot(...) over direct string equality on stdout/stderr
Do not write tests that assert absence of 'panic', 'uncaught exception', or similar strings in output
Avoid shelling out to tools like find or grep in tests; use Bun.Glob and built-in utilities
Files:
test/js/sql/sql.test.tstest/js/sql/sql-mysql.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Run Prettier to format JS/TS files (bun run prettier)
Files:
test/js/sql/sql.test.tstest/harness.tstest/js/sql/sql-mysql.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/sql/sql.test.tstest/js/sql/sql-mysql.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/sql/sql.test.tstest/js/sql/sql-mysql.test.ts
🧠 Learnings (17)
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/js/sql/sql.test.tstest/harness.tstest/js/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/js/sql/sql.test.tstest/harness.tstest/js/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/js/sql/sql.test.tstest/js/sql/sql-mysql.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/sql/sql.test.tstest/js/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T17:09:28.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-03T17:09:28.100Z
Learning: Applies to test/js/bun/** : Place Bun-specific API tests (http, crypto, ffi, shell, etc.) under test/js/bun/
Applied to files:
test/js/sql/sql.test.tstest/js/sql/sql-mysql.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/sql/sql.test.tstest/js/sql/sql-mysql.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/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/js/sql/sql.test.tstest/harness.tstest/js/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
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/sql/sql.test.tstest/js/sql/sql-mysql.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} : Import testing utilities (devTest, prodTest, devAndProductionTest, Dev, Client) from test/bake/bake-harness.ts
Applied to files:
test/js/sql/sql.test.tstest/js/sql/sql-mysql.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/js/sql/sql.test.ts
📚 Learning: 2025-09-03T17:09:28.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-03T17:09:28.100Z
Learning: Applies to test/**/*.test.{ts,tsx} : Avoid shelling out to tools like find or grep in tests; use Bun.Glob and built-in utilities
Applied to files:
test/js/sql/sql.test.ts
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
Learning: Applies to test/**/*.test.ts : Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Applied to files:
test/js/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
Learning: Applies to test/**/*.test.ts : Never hardcode port numbers in tests; use `port: 0` to get a random port
Applied to files:
test/js/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T01:30:57.965Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:57.965Z
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/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
Learning: Applies to test/**/*.test.ts : Prefer `async/await` in tests; for a single callback, use `Promise.withResolvers()`
Applied to files:
test/js/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
Learning: Applies to test/**/*.test.ts : In error tests, assert non-zero exit codes for failing processes and use `toThrow` for synchronous errors
Applied to files:
test/js/sql/sql-mysql.test.ts
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
Learning: Applies to test/**/*.test.ts : Use `tempDir`/`tempDirWithFiles` from `harness` for temporary files and directories in tests
Applied to files:
test/js/sql/sql-mysql.test.ts
🧬 Code graph analysis (2)
test/harness.ts (1)
scripts/utils.mjs (4)
isLinux(24-24)isLinux(24-24)isMacOS(23-23)isMacOS(23-23)
test/js/sql/sql-mysql.test.ts (1)
test/harness.ts (3)
isDockerEnabled(860-877)dockerExe(856-858)describeWithContainer(906-986)
⏰ 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
3 participants
What does this PR do?
Fixes #22312
Fixes #22313
The correct flow for TLS handshaking is:
Server sending Protocol::Handshake
Client replying with Protocol::SSLRequest:
The usual SSL exchange leading to establishing SSL connection
Client sends Protocol::HandshakeResponse:
Source: https://dev.mysql.com/doc/dev/mysql-server/8.4.5/page_protocol_connection_phase.html
How did you verify your code works?
Tests