Repository navigation
fix(Bun.SQL) fix "this" context handling in onclose function - #22507
cirospaciari wants to merge 64 commits into
Conversation
|
Updated 5:58 PM PT - Sep 11th, 2025
❌ @cirospaciari, your commit c226a94 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 22507That installs a local version of the PR into your bun-22507 --bun |
cirospaciari
changed the title
cirospaciari
changed the title
WalkthroughReplace per-object JSValue fields with a jsc.JSRef API (tryGet/upgrade/downgrade/finalize), remove per-object pending_activity counters, tighten lifecycle/refcount flows across MySQL/Postgres queries and connections, adjust error/on_close binding and microtask queuing, and update JS exports, tests, and codegen guards. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches🧪 Generate unit tests
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: 0
🧹 Nitpick comments (1)
src/sql/mysql/protocol/ErrorPacket.zig (1)
22-31: Optional: pre-size to expected property count to avoid a tiny growth cost.If this path is hot, preallocating for the worst-case 4 properties can shave a minor resize.
- const opts_obj = JSValue.createEmptyObject(globalObject, 0); + const opts_obj = JSValue.createEmptyObject(globalObject, 4); // code, errno?, sqlState?, message
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/sql/mysql/protocol/ErrorPacket.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("...", .{})
Files:
src/sql/mysql/protocol/ErrorPacket.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/sql/mysql/protocol/ErrorPacket.zig
🔇 Additional comments (1)
src/sql/mysql/protocol/ErrorPacket.zig (1)
22-22: Capacity 0 for createEmptyObject is reasonable here.Fewer props are set (≤4), so reducing from 18 avoids unnecessary preallocation. Behavior is unchanged.
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/bun.js/bindings/bindings.cpp (1)
3467-3493: Fix inverted ASSERTs and use sanitized values when enqueuing the microtask.ASSERTs for asyncContext/value are inverted and checked before normalization; queueMicrotask currently passes the raw internalField(0) instead of the sanitized asyncContext — this can enqueue empty JSValues and cause crashes.
File: src/bun.js/bindings/bindings.cpp (≈lines 3467-3493)
- auto* globalObject = jsCast<Zig::GlobalObject*>(promise->globalObject()); - auto microtaskFunction = globalObject->performMicrotaskFunction(); - auto rejectPromiseFunction = globalObject->rejectPromiseFunction(); - - auto asyncContext = globalObject->m_asyncContextData.get()->getInternalField(0); -#if ASSERT_ENABLED - ASSERT_WITH_MESSAGE(microtaskFunction, "Invalid microtask function"); - ASSERT_WITH_MESSAGE(asyncContext.isEmpty(), "Invalid microtask context"); - ASSERT_WITH_MESSAGE(rejectPromiseFunction, "Invalid microtask callback"); - ASSERT_WITH_MESSAGE(value.isEmpty(), "Invalid microtask value"); -#endif - - if (asyncContext.isEmpty()) { - asyncContext = jsUndefined(); - } - - if (value.isEmpty()) { - value = jsUndefined(); - } - - globalObject->queueMicrotask( - microtaskFunction, - rejectPromiseFunction, - globalObject->m_asyncContextData.get()->getInternalField(0), - promise, - value); + auto* globalObject = jsCast<Zig::GlobalObject*>(promise->globalObject()); + auto* microtaskFunction = globalObject->performMicrotaskFunction(); + auto* rejectPromiseFunction = globalObject->rejectPromiseFunction(); + auto asyncContext = globalObject->m_asyncContextData.get()->getInternalField(0); +#if ASSERT_ENABLED + ASSERT_WITH_MESSAGE(microtaskFunction, "Missing microtask function"); + ASSERT_WITH_MESSAGE(rejectPromiseFunction, "Missing rejectPromise function"); +#endif + if (asyncContext.isEmpty()) + asyncContext = jsUndefined(); + if (value.isEmpty()) + value = jsUndefined(); +#if ASSERT_ENABLED + ASSERT_WITH_MESSAGE(!asyncContext.isEmpty(), "Invalid microtask context (empty)"); + ASSERT_WITH_MESSAGE(!value.isEmpty(), "Invalid microtask value (empty)"); +#endif + globalObject->queueMicrotask( + microtaskFunction, + rejectPromiseFunction, + asyncContext, + promise, + value);
🧹 Nitpick comments (1)
src/bun.js/bindings/bindings.cpp (1)
6145-6168: Consistent naming and const correctness for cached microtask function.Minor consistency/readability: use the same identifier style as elsewhere and make the cached pointer const.
- auto microTaskFunction = globalObject->performMicrotaskFunction(); + const auto* microtaskFunction = globalObject->performMicrotaskFunction(); - ASSERT_WITH_MESSAGE(microTaskFunction, "Invalid microtask function"); + ASSERT_WITH_MESSAGE(microtaskFunction, "Invalid microtask function"); - globalObject->queueMicrotask( - microTaskFunction, + globalObject->queueMicrotask( + microtaskFunction,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- 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/bindings/ZigGlobalObject.cpp(2 hunks)src/bun.js/bindings/bindings.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/ZigGlobalObject.cppsrc/bun.js/bindings/bindings.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/ZigGlobalObject.cppsrc/bun.js/bindings/bindings.cpp
src/bun.js/bindings/ZigGlobalObject.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Initialize the LazyClassStructure in GlobalObject::finishCreation and visit it in GlobalObject::visitChildrenImpl
Files:
src/bun.js/bindings/ZigGlobalObject.cpp
src/bun.js/bindings/ZigGlobalObject.{h,cpp}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Files:
src/bun.js/bindings/ZigGlobalObject.cpp
**/*.{cpp,cc,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format C/C++ sources/headers with clang-format (bun run clang-format)
Files:
src/bun.js/bindings/ZigGlobalObject.cppsrc/bun.js/bindings/bindings.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: For JS classes with a public constructor, implement Foo, FooPrototype, and FooConstructor (JSC patterns)
Define class properties using HashTableValue arrays in JSC bindings
Add iso subspaces for classes with C++ fields in JSC bindings
Cache structures in ZigGlobalObject for JSC-bound classes
Files:
src/bun.js/bindings/ZigGlobalObject.cppsrc/bun.js/bindings/bindings.cpp
🧠 Learnings (15)
📚 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 src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JSC-bound classes
Applied to files:
src/bun.js/bindings/ZigGlobalObject.cppsrc/bun.js/bindings/bindings.cpp
📚 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 : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/bindings/ZigGlobalObject.cppsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/ZigGlobalObject.cpp
📚 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:
src/bun.js/bindings/ZigGlobalObject.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/bun.js/bindings/ZigGlobalObject.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.cpp : Initialize the LazyClassStructure in GlobalObject::finishCreation and visit it in GlobalObject::visitChildrenImpl
Applied to files:
src/bun.js/bindings/ZigGlobalObject.cpp
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : Add new V8 API method mangled symbols to the V8API struct in src/napi/napi.zig for both GCC/Clang and MSVC
Applied to files:
src/bun.js/bindings/ZigGlobalObject.cpp
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add new V8 API mangled symbols (without leading underscore) to src/symbols.txt
Applied to files:
src/bun.js/bindings/ZigGlobalObject.cppsrc/bun.js/bindings/bindings.cpp
📚 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:
src/bun.js/bindings/ZigGlobalObject.cpp
📚 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 src/bun.js/bindings/**/*.cpp : Add iso subspaces for classes with C++ fields in JSC bindings
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/**/*.{h,cpp} : Use localToJSValue() when converting V8 handles to JSC values before performing JSC operations
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/**/*.{h,cpp} : Use JSC::WriteBarrier for heap-allocated references and implement visitChildren() for custom heap objects
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 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 src/bun.js/bindings/**/*.cpp : For JS classes with a public constructor, implement Foo, FooPrototype, and FooConstructor (JSC patterns)
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-07T05:41:52.563Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-09-07T05:41:52.563Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts} : Prefer JSC intrinsics and `$`-prefixed private APIs for performance and safety (e.g., `$Array`, `$newArrayWithSize`, `$putByIdDirectPrivate`, `$assert`, `$debug`)
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 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 src/bun.js/bindings/**/*.cpp : Define class properties using HashTableValue arrays in JSC bindings
Applied to files:
src/bun.js/bindings/bindings.cpp
⏰ 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 (2)
src/bun.js/bindings/ZigGlobalObject.cpp (2)
4156-4161: LGTM on added debug guards in JSC__JSGlobalObject__queueMicrotaskCallback.Asserting non-null trampoline, context pointer, and callback improves crash diagnostics without changing release behavior.
1551-1561: Fix inverted debug assertions in queueMicrotask (callback/context checks).callback.isEmpty() and asyncContext.isEmpty() are inverted — callback must be non-empty (callable) and asyncContext is expected to be undefined. File: src/bun.js/bindings/ZigGlobalObject.cpp (≈ lines 1551–1561).
Apply:
- ASSERT_WITH_MESSAGE(function, "Invalid microtask function"); - ASSERT_WITH_MESSAGE(asyncContext.isEmpty(), "Invalid microtask context"); - ASSERT_WITH_MESSAGE(callback.isEmpty(), "Invalid microtask callback"); + ASSERT_WITH_MESSAGE(function, "Invalid microtask function"); + ASSERT_WITH_MESSAGE(asyncContext.isUndefined(), "Invalid microtask context (expected undefined)"); + ASSERT_WITH_MESSAGE(!callback.isEmpty(), "Invalid microtask callback"); +#if 0 // optional, stronger debug check (redundant with V::validateFunction) + ASSERT_WITH_MESSAGE(JSC::getCallData(callback).type != JSC::CallData::Type::None, "callback must be callable"); +#endifRe-run failing job locally to verify (the provided CI command failed in this environment due to missing bunx):
bunx bun-pr 22507 bun-22507 --bun test/js/sql/sql-mysql.test.ts
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 (2)
src/sql/mysql/MySQLConnection.zig (2)
934-934: User-facing error string mentions “postgresql” in MySQL connectorIncorrect message can mislead debugging.
- return globalObject.throwError(err, "failed to connect to postgresql"); + return globalObject.throwError(err, "failed to connect to mysql");
1313-1326: Inconsistent onconnect microtask args — standardize the callback contract
- MySQL enqueues [null, connection] (src/sql/mysql/MySQLConnection.zig — queueMicrotask(on_connect, &[_]JSValue{ JSValue.jsNull(), js_value })).
- Valkey enqueues [connection, hello_value] (src/valkey/js_valkey.zig — queueMicrotask(on_connect, &[_]JSValue{ js_value, hello_value })).
Action: choose the canonical signature (e.g., (err, conn) or (conn, info)), update all call sites and tests, and verify Postgres (src/sql/postgres/PostgresSQLConnection.zig) follows the same contract.
♻️ Duplicate comments (1)
src/sql/mysql/MySQLConnection.zig (1)
1901-1903: Fix unsafe optional unwrap of cached_structure (can panic at runtime)cached_structure may be null when js_value.tryGet() fails. Guard before calling jsValue() to avoid a trap.
Apply:
- .objects => { - cached_structure = if (this.js_value.tryGet()) |value| statement.structure(value, this.globalObject) else null; - structure = cached_structure.?.jsValue() orelse .js_undefined; - }, + .objects => { + cached_structure = if (this.js_value.tryGet()) |value| + statement.structure(value, this.globalObject) + else + null; + structure = if (cached_structure) |cs| + cs.jsValue() orelse .js_undefined + else + .js_undefined; + },
🧹 Nitpick comments (2)
src/sql/mysql/MySQLConnection.zig (2)
124-131: Reference gating policy is reasonable; consider idling-downgrade tweakOptional: downgrade when fully idle (no requests and no pending writes), but keep strong during connect/handshake/auth or when write_buffer has data. This avoids keeping a strong JS ref for indefinitely idle connections.
Apply:
fn updateReferenceType(this: *MySQLConnection) void { - if (this.js_value.isNotEmpty()) { - if (this.requests.readableLength() > 0 or (this.status != .disconnected and this.status != .failed)) { - this.js_value.upgrade(this.globalObject); - return; - } - this.js_value.downgrade(); - } + if (this.js_value.isNotEmpty()) { + const active = + this.requests.readableLength() > 0 or + this.write_buffer.len() > 0 or + (this.status == .connecting or + this.status == .handshaking or + this.status == .authenticating or + this.status == .authentication_awaiting_pk); + if (active) { + this.js_value.upgrade(this.globalObject); + } else { + this.js_value.downgrade(); + } + } }
268-274: Unregister AutoFlusher in finalize to avoid stray microtasksIf finalize runs while a flusher is scheduled, a latent microtask could reference a soon-to-be-derefed connection. Unregistering is cheap and safer.
pub fn finalize(this: *MySQLConnection) void { this.stopTimers(); debug("MySQLConnection finalize", .{}); + this.unregisterAutoFlusher(); this.js_value.deinit(); this.deref(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/sql/mysql/MySQLConnection.zig(19 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("...", .{})
Files:
src/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.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/sql/mysql/MySQLConnection.zig
🔇 Additional comments (11)
src/sql/mysql/MySQLConnection.zig (11)
24-24: JSRef migration + setStrong on creation — LGTMSwitching to jsc.JSRef and setting a strong ref at creation is correct and aligns with the new lifecycle model.
Also applies to: 955-957
351-355: Guarded access to cached queries — LGTMCorrect use of tryGet() with undefined fallback.
400-410: onEnd/onClose ordering — LGTMonClose defers deref and delegates to onEnd → fail path first, then refAndClose. Matches intent.
428-430: Disconnect gating + status transition — LGTMDefer updateReferenceType() around status change is correct.
728-736: Socket handlers unify close/error -> onEnd — LGTMConsistent terminal path simplifies lifecycle and reduces duplication.
Also applies to: 738-741
761-762: Timeout path unified wording — LGTMError code and message align; behavior is unchanged.
1015-1020: TCP onOpen state + ref symmetry — LGTMSetting .handshaking and holding a ref only for TCP (not post-upgrade TLS) is correct.
1158-1163: Packet length handling — LGTMComputing packet_length = header_length + PacketHeader.size and deferring setOffsetFromStart is correct for short-read recovery and padding.
1284-1295: Safe consumption of cached callbacks — LGTMtryGet(), ensureStillAlive(), and cache-clearing semantics look correct.
Also applies to: 1298-1309
1333-1339: updateRef ties strongness to poll_ref — LGTMKeeps process liveness aligned with JSRef strength. Good.
1792-1793: Passing connection JS value into onResult — LGTMUsing tryGet() with undefined fallback is correct.
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/sql/mysql/MySQLQuery.zig (1)
68-95: Protect object during enqueue and clear caches on failureMirror Postgres: hold a temporary ref to this, clear caches, then downgrade. Prevents leaks and premature destruction while scheduling the callback.
Apply:
pub fn onWriteFail( this: *@This(), err: AnyMySQLError.Error, globalObject: *jsc.JSGlobalObject, queries_array: JSValue, ) void { - this.status = .fail; - const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); - const targetValue = this.getTarget(globalObject, true) orelse return; + this.ref(); + defer this.deref(); + this.status = .fail; + const thisValue = this.thisValue.tryGet() orelse return; + const targetValue = this.getTarget(globalObject, true) orelse return; + defer { + allowGC(thisValue, globalObject); + this.thisValue.downgrade(); + }
♻️ Duplicate comments (6)
src/bun.js/bindings/JSRef.zig (1)
61-69: Guard downgrade() when strong.get() is null to avoid calling ensureStillAlive() on .zeroIf get() returns null, calling ensureStillAlive() on .zero is unsafe. Guard and only keep the weak value when non-null.
Apply:
- .strong => |*strong| { - const value = strong.get() orelse .zero; - value.ensureStillAlive(); - strong.deinit(); - this.* = .{ .weak = value }; - }, + .strong => |*strong| { + const maybe = strong.get(); + strong.deinit(); + if (maybe) { + const value = maybe.?; + value.ensureStillAlive(); + this.* = .{ .weak = value }; + } else { + this.* = .{ .weak = .zero }; + } + },src/sql/postgres/PostgresSQLQuery.zig (2)
78-81: Clear all caches on failure to avoid leaksAfter grabbing target, call allowGC(thisValue, ...) and then downgrade(). This mirrors success path cleanup and prevents stale cached refs.
Apply:
- const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); - const targetValue = this.getTarget(globalObject, true) orelse return; + const thisValue = this.thisValue.tryGet() orelse return; + const targetValue = this.getTarget(globalObject, true) orelse return; + defer { + allowGC(thisValue, globalObject); + this.thisValue.downgrade(); + }
96-99: Do the same cleanup in onJSError()Symmetric failure cleanup prevents retained arrays/bindings.
Apply:
- const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); - const targetValue = this.getTarget(globalObject, true) orelse return; + const thisValue = this.thisValue.tryGet() orelse return; + const targetValue = this.getTarget(globalObject, true) orelse return; + defer { + allowGC(thisValue, globalObject); + this.thisValue.downgrade(); + }src/sql/mysql/MySQLQuery.zig (1)
161-178: Also keep JS “this” alive during error-callback scheduling and clear cachesCreate a temporary Strong on thisValue (JS object) while enqueuing; also clear caches + downgrade like onWriteFail.
Apply:
this.status = .fail; - const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); - const targetValue = this.getTarget(globalObject, true) orelse return; + const thisValue = this.thisValue.tryGet() orelse return; + var strong_this = jsc.Strong.Optional.create(thisValue, globalObject); + defer strong_this.deinit(); + const targetValue = this.getTarget(globalObject, true) orelse return; + defer { + allowGC(thisValue, globalObject); + this.thisValue.downgrade(); + }src/sql/mysql/MySQLConnection.zig (2)
364-392: Fix on_close microtask invocation to use proper this-binding
1901-1902: Fix unsafe optional unwrap — guard cached_structure before calling .jsValue()
🧹 Nitpick comments (1)
src/bun.js/bindings/JSRef.zig (1)
10-10: Nit: prefer parameter name globalObject for consistencyMost bindings use globalObject; consider renaming for uniformity.
Also applies to: 39-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/bun.js/api/server.zig(4 hunks)src/bun.js/bindings/JSRef.zig(4 hunks)src/sql/mysql/MySQLConnection.zig(19 hunks)src/sql/mysql/MySQLQuery.zig(7 hunks)src/sql/postgres/PostgresSQLQuery.zig(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bun.js/api/server.zig
🧰 Additional context used
📓 Path-based instructions (3)
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/sql/mysql/MySQLConnection.zigsrc/sql/postgres/PostgresSQLQuery.zigsrc/sql/mysql/MySQLQuery.zigsrc/bun.js/bindings/JSRef.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/sql/mysql/MySQLConnection.zigsrc/sql/postgres/PostgresSQLQuery.zigsrc/sql/mysql/MySQLQuery.zigsrc/bun.js/bindings/JSRef.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/bindings/JSRef.zig
🧠 Learnings (12)
📚 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/sql/postgres/PostgresSQLQuery.zigsrc/bun.js/bindings/JSRef.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/sql/postgres/PostgresSQLQuery.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/sql/postgres/PostgresSQLQuery.zigsrc/bun.js/bindings/JSRef.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/sql/postgres/PostgresSQLQuery.zigsrc/bun.js/bindings/JSRef.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 : For properties marked cache: true, use the generated Zig accessors (<Prop>NameSetCached/GetCached) to work with GC-owned values
Applied to files:
src/sql/postgres/PostgresSQLQuery.zigsrc/bun.js/bindings/JSRef.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 Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/bindings/JSRef.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 : Implement getters as get<PropertyName>(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Applied to files:
src/bun.js/bindings/JSRef.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,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/bindings/JSRef.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/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bun.js/bindings/JSRef.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/JSRef.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/bindings/JSRef.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 : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Applied to files:
src/bun.js/bindings/JSRef.zig
🔇 Additional comments (33)
src/bun.js/bindings/JSRef.zig (5)
11-13: Asserting non-zero strong refs is goodEnsures we never create a strong ref to .zero. Matches intended API constraints.
19-25: tryGet(): API shape looks correctOptional return across weak/strong/finalized states is clear and safe.
39-46: setStrong(): non-zero assertion is appropriatePrevents accidental strong refs to .zero.
76-90: isEmpty()/isNotEmpty(): semantics are consistentMatches weak!=.zero and strong.has() expectations.
104-107: finalize(): simple and correctDeinits and marks finalized; aligns with lifecycle expectations.
src/sql/postgres/PostgresSQLQuery.zig (3)
26-33: getTarget(): optional return + clean reset is correctEarly bailouts on missing thisValue/target reduce crash surface; cache clear on demand is good.
139-144: Success-path cleanup looks goodallowGC() + downgrade() when is_last ensures caches are cleared and ref downgraded at the right time.
Also applies to: 141-142
231-235: Stray this.pending_value usage likely dead or brokenStruct has no pending_value field; this won’t compile if referenced. Either remove or switch to pendingValueGetCached(thisValue).
Suggested rewrite:
pub fn push(this: *PostgresSQLQuery, globalThis: *jsc.JSGlobalObject, value: JSValue) void { - var pending_value = this.pending_value.get() orelse return; - pending_value.push(globalThis, value); + const thisValue = this.thisValue.tryGet() orelse return; + const arr = js.pendingValueGetCached(thisValue) orelse return; + arr.push(globalThis, value); }src/sql/mysql/MySQLQuery.zig (4)
64-66: finalize(): correct lifecycleReleases statement, finalizes JS ref, then deref — matches intended pattern.
117-121: bind(): guarded cache reads are goodUsing .js_undefined defaults avoids .zero misuse in iterators.
179-186: getTarget(): optional return + selective cache clear is correctMatches the new JSRef API and avoids touching other caches.
221-228: Success-path cleanup is correctallowGC() + downgrade() when is_last mirrors Postgres.
src/sql/mysql/MySQLConnection.zig (21)
24-24: LGTM!The change from
JSValuetojsc.JSRef = jsc.JSRef.empty()aligns with the new reference lifecycle management system described in the AI summary.
124-132: LGTM!The
updateReferenceTypefunction properly implements the new reference management by checking connection state and request queue length to determine when to upgrade/downgrade the JS reference.
272-272: LGTM!The finalize method correctly uses
js_value.finalize()instead of directly zeroing the value, which aligns with the new JSRef API.
278-278: LGTM!Calling
updateReferenceType()indoRefensures the reference state is properly synchronized.
284-284: LGTM!Calling
updateReferenceType()indoUnrefensures the reference state is properly synchronized.
351-355: LGTM!The
getQueriesArrayfunction properly usesjs_value.tryGet()to safely access the JS value and returns.js_undefinedwhen the value is not available.
400-409: LGTM!The new
onEndandonCloselifecycle methods provide clear separation between socket end events and close events, withonCloseproperly dereferencing the connection.
414-416: LGTM!The
refAndClosemethod correctly callscleanUpRequestswith the JS reason parameter to handle cleanup consistently.
428-429: LGTM!Calling
updateReferenceType()in the disconnect flow ensures reference state is properly managed during connection lifecycle changes.
735-740: LGTM!The socket handlers correctly call the new
onEnd()method for both end and connect error events.
761-761: Timeout message uses "Connection timeout" consistentlyThe timeout message now consistently uses "Connection timeout" which aligns with the timeout message updates mentioned in the AI summary.
956-956: LGTM!Using
js_value.setStrong(js_value, globalObject)correctly establishes a strong reference to the JS value during connection creation.
1015-1019: LGTM!The socket open handler properly updates the connection status, manages references, and calls
updateReferenceType()to maintain proper reference counting.
1157-1162: LGTM!The packet processing correctly calculates
packet_lengthasheader_length + PacketHeader.sizeand uses it for capacity checks and offset updates.
1285-1309: LGTM!Both callback consumption methods properly use
js_value.tryGet()to safely access the JS value and handle cases where the value might not be available.
1313-1313: LGTM!Calling
updateReferenceType()insetStatusensures reference state is updated when connection status changes.
1322-1325: LGTM!The connection success flow properly uses
js_value.tryGet()to get the current JS value and falls back to.js_undefinedif unavailable.
1333-1339: LGTM!The
updateRefmethod correctly manages the poll reference based on the JS value state (strong vs weak reference).
1792-1792: LGTM!Using
js_value.tryGet() orelse .js_undefinedsafely accesses the JS value for result handling.
1911-1911: LGTM!Safely accessing the pending value using
request.thisValue.tryGet()with proper fallback.
1929-1933: LGTM!The pending value handling correctly checks if the value is empty/undefined/null and safely accesses the request's JS value using
tryGet().
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/sql/mysql/MySQLQuery.zig (2)
75-93: Fix CI: replace.isCallable()(compile error) and keepthisalive during callback enqueueBuild is failing at Line 85 due to
.isCallable()onfunction. Also, take a temporary strong onthiswhile the callback is queued to avoid GC races (same ask as the earlier review).const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); + // Keep JS `this` alive while the callback is enqueued. + var strong_this = jsc.Strong.Optional.create(thisValue, globalObject); + defer strong_this.deinit(); + defer this.thisValue.downgrade(); const targetValue = this.getTarget(globalObject, true) orelse return; const instance = AnyMySQLError.mysqlErrorToJS(globalObject, "Failed to bind query", err); const js_err = instance.toError() orelse instance; bun.assertf(js_err != .zero, "js_err is zero", .{}); js_err.ensureStillAlive(); const vm = jsc.VirtualMachine.get(); - const function = vm.rareData().mysql_context.onQueryRejectFn.get() orelse return; - bun.assertf(function.isCallable(), "onQueryRejectFn is not callable", .{}); + const function = vm.rareData().mysql_context.onQueryRejectFn.get() orelse return; + // Guard: skip if hook isn't callable to avoid native crash. + if (!jsc.JSValueIsCallable(globalObject, function)) return; const event_loop = vm.eventLoop(); - const js_array = if (queries_array == .zero) .js_undefined else queries_array; - js_array.ensureStillAlive(); + const js_array = if (queries_array == .zero) .js_undefined else queries_array; + if (js_array != .js_undefined) js_array.ensureStillAlive(); event_loop.runCallback(function, globalObject, thisValue, &.{ targetValue, js_err, js_array, });Confirm the correct callable-check helper and presence across the tree:
#!/bin/bash # Find canonical callable checks in bun/jsc wrappers rg -nP '\bisCallable\s*\(' --type=zig -C2 rg -nP 'JSValueIsCallable' --type=zig -C2 rg -nP 'onQueryRejectFn' --type=zig -C3Expected: one of the above exists (prefer the same symbol used elsewhere in SQL/Postgres paths). If neither exists, drop the guard and rely on invariants (field is engine-internal).
161-177: Mirror fixes in onJSError: temporary strong + callable guardSame lifecycle and build issues as in onWriteFail. Protect
thisand avoid.isCallable()usage.this.status = .fail; const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); + var strong_this = jsc.Strong.Optional.create(thisValue, globalObject); + defer strong_this.deinit(); + defer this.thisValue.downgrade(); const targetValue = this.getTarget(globalObject, true) orelse return; var vm = jsc.VirtualMachine.get(); - const function = vm.rareData().mysql_context.onQueryRejectFn.get() orelse return; - bun.assertf(function.isCallable(), "onQueryRejectFn is not callable", .{}); + const function = vm.rareData().mysql_context.onQueryRejectFn.get() orelse return; + if (!jsc.JSValueIsCallable(globalObject, function)) return; const event_loop = vm.eventLoop(); var js_error = err.toError() orelse err; if (js_error == .zero) { js_error = AnyMySQLError.mysqlErrorToJS(globalObject, "Query failed", error.UnknownError); } js_error.ensureStillAlive();
🧹 Nitpick comments (3)
src/sql/mysql/MySQLQuery.zig (3)
223-229: queries_array handling is robust; micro-nit on ensureThe extra
.zerocheck thenensureStillAlive()is fine. Minor nit: you can fold into one branch, but not necessary.- var queries_array = if (connection == .zero) .js_undefined else MySQLConnection.js.queriesGetCached(connection) orelse .js_undefined; - if (queries_array == .zero) { - queries_array = .js_undefined; - } else { - queries_array.ensureStillAlive(); - } + var queries_array = if (connection == .zero) + .js_undefined + else + (MySQLConnection.js.queriesGetCached(connection) orelse .js_undefined); + if (queries_array != .js_undefined) + queries_array.ensureStillAlive();Also applies to: 235-235
523-523: Align debug logger with guidelinesGuidelines recommend
falsefor the scoped logger’s second arg to keep logs off by default.-const debug = bun.Output.scoped(.MySQLQuery, .visible); +const debug = bun.Output.scoped(.MySQLQuery, false);
64-66: Finalize ordering: clear this.thisValue after finalize to avoid accidental reuserg shows finalize() only at this location; there are other downgrade()/upgrade() uses in the file — null the ref after finalize to prevent re-use.
- this.thisValue.finalize(); + this.thisValue.finalize(); + this.thisValue = JSRef.empty(); this.deref();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/sql/mysql/MySQLQuery.zig(7 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("...", .{})
Files:
src/sql/mysql/MySQLQuery.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/sql/mysql/MySQLQuery.zig
🔇 Additional comments (6)
src/sql/mysql/MySQLQuery.zig (6)
118-121: Good: safer defaults for cached bindings/columnsReturning
.js_undefinedwhen caches miss simplifies downstream checks. LGTM.
179-186: getTarget optionality and cleanup look correctReturning
?jsc.JSValueand clearing the cached target behindclean_targetmatches the new lifecycle. LGTM.
210-216: DegradethisValueonly on last result — correctThe conditional downgrade on
is_lastpreserves reachability across partial responses. Good.
2-2: RefCount switched to non-thread-safe variant — confirm single-threaded ownershipRepo scan shows MySQLQuery is only referenced under src/sql/mysql and is managed by MySQLConnection; no Thread.spawn/ThreadPool hits found in src/sql. Verify MySQLQuery is never handed to worker threads or thread pools (it’s exported to JS in bun.js/bindings/generated_classes_list.zig); if it can be accessed cross-thread, restore a thread-safe RefCount.
355-356: Confirm updateRef is safe as usedI inspected the repository search output you ran. MySQLConnection.updateRef (src/sql/mysql/MySQLConnection.zig:1332) calls updateReferenceType() then either poll_ref.ref(this.vm) or poll_ref.unref(this.vm) based on this.js_value, matching the pattern used across the codebase (Postgres and other readers/writers use the same defer/updateRef pattern). That implementation is not a paired increment/decrement counter — it recomputes desired state and applies ref/unref to match it, so calling it on early returns or via defer is safe and idempotent.
Action: keep defer connection.updateRef(); as-is.
218-220: Guard onQueryResolveFn instead of assertingReplace the assert at src/sql/mysql/MySQLQuery.zig:218–219 with an early-return callable check — e.g.:
if (!function.isCell() or !function.isCallable()) return;
The suggested jsc.JSValueIsCallable(...) helper is not present in the repo.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/sql/postgres/PostgresSQLQuery.zig (2)
78-81: Clear all cached refs on failure to avoid leaks (repeat)Failure path only clears the target via
clean_target=true; pending/binding caches remain. Clear them withallowGC(...)before downgrading.Apply:
- const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); + const thisValue = this.thisValue.tryGet() orelse return; + defer this.thisValue.downgrade(); + defer allowGC(thisValue, globalObject);
96-99: Mirror failure cleanup in onJSErrorSame leak risk here; clear all caches before downgrade.
- const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); + const thisValue = this.thisValue.tryGet() orelse return; + defer this.thisValue.downgrade(); + defer allowGC(thisValue, globalObject);
🧹 Nitpick comments (2)
src/sql/postgres/PostgresSQLQuery.zig (2)
26-33: getTarget now returns optional and conditionally clears only the target cacheSemantics look good. Minor note:
clean_targetclears just the target cache; pending/binding remain. Ensure callers invokeallowGC(...)when fully disposing to clear all caches.
492-492: Align debug logger with repo guidelineUse a non-verbose scoped logger by default.
-const debug = bun.Output.scoped(.Postgres, .visible); +const debug = bun.Output.scoped(.Postgres, false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/sql/postgres/PostgresSQLQuery.zig(6 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("...", .{})
Files:
src/sql/postgres/PostgresSQLQuery.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/sql/postgres/PostgresSQLQuery.zig
🧠 Learnings (5)
📚 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/sql/postgres/PostgresSQLQuery.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/sql/postgres/PostgresSQLQuery.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/sql/postgres/PostgresSQLQuery.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/sql/postgres/PostgresSQLQuery.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 : For properties marked cache: true, use the generated Zig accessors (<Prop>NameSetCached/GetCached) to work with GC-owned values
Applied to files:
src/sql/postgres/PostgresSQLQuery.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
🔇 Additional comments (3)
src/sql/postgres/PostgresSQLQuery.zig (3)
2-2: Verify thread-safety after switching to non-thread-safe RefCountChanging to
bun.ptr.RefCount(non-thread-safe) is fine only ifPostgresSQLQuerynever crosses threads. Please confirm this object stays on one VM thread; otherwise revert to a thread-safe refcount.
63-67: Finalize recursion removed — LGTM
finalize()no longer calls itself and correctly unlinks the JSRef beforederef().
138-144: Resolve path cleanup ordering — LGTMUsing
defertoallowGC(...)anddowngrade()only onis_lastwhile fetchingtargetValuefirst is correct.
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 (4)
src/sql/postgres/PostgresSQLQuery.zig (4)
96-107: Same fixes as onWriteFail: guard callback, clear caches, strong ref, keep error alivePrevents traps and leaks; preserves
thiscontext until enqueue completes.- const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); - const targetValue = this.getTarget(globalObject, true) orelse return; + const thisValue = this.thisValue.tryGet() orelse return; + defer this.thisValue.downgrade(); + const targetValue = this.getTarget(globalObject, true) orelse return; + allowGC(thisValue, globalObject); + var strong_this = jsc.Strong.Optional.create(thisValue, globalObject); + defer strong_this.deinit(); @@ - const function = vm.rareData().postgresql_context.onQueryRejectFn.get().?; + const function = vm.rareData().postgresql_context.onQueryRejectFn.get() orelse return; + bun.assertf(function.isCallable(), "onQueryRejectFn is not callable", .{}); @@ - event_loop.runCallback(function, globalObject, thisValue, &.{ + var js_error = err.toError() orelse err; + js_error.ensureStillAlive(); + event_loop.runCallback(function, globalObject, thisValue, &.{ targetValue, - err.toError() orelse err, + js_error, });
138-157: Guard resolve callback and keep queries array aliveAvoid forced unwrap; ensure callable; keep
queries_arrayalive.- const vm = jsc.VirtualMachine.get(); - const function = vm.rareData().postgresql_context.onQueryResolveFn.get().?; + const vm = jsc.VirtualMachine.get(); + const function = vm.rareData().postgresql_context.onQueryResolveFn.get() orelse return; + bun.assertf(function.isCallable(), "onQueryResolveFn is not callable", .{}); @@ - if (connection == .zero) .js_undefined else PostgresSQLConnection.js.queriesGetCached(connection) orelse .js_undefined, + if (connection == .zero) .js_undefined else blk: { + var qa = PostgresSQLConnection.js.queriesGetCached(connection) orelse .js_undefined; + if (qa != .js_undefined) qa.ensureStillAlive(); + break :blk qa; + },
231-234: Compile-time error:this.pending_valueis not a fieldUse the cached pending array from
thisValue.pub fn push(this: *PostgresSQLQuery, globalThis: *jsc.JSGlobalObject, value: JSValue) void { - var pending_value = this.pending_value.get() orelse return; - pending_value.push(globalThis, value); + const thisValue = this.thisValue.tryGet() orelse return; + const arr = js.pendingValueGetCached(thisValue) orelse return; + arr.ensureStillAlive(); + arr.push(globalThis, value); }
260-269: Balance poll_ref.ref in PostgresSQLQuery.doRun — doRun calls connection.poll_ref.ref(globalObject.bunVM()) (src/sql/postgres/PostgresSQLQuery.zig:266) but has early-return/error paths that won't unref; adddefer connection.updateRef();immediately after acquiringconnection(mirror MySQLQuery.doRun) to guarantee the ref is paired.
♻️ Duplicate comments (4)
src/sql/postgres/PostgresSQLQuery.zig (2)
78-91: Avoid trap on missing callback; clear all caches on failure; keep a temporary strong ref; keep args alive
- Use orelse return instead of forcing
.?.- Clear binding/pending caches (target is already cleared via clean_target).
- Take a temporary
Strongto protectthisValueuntil the callback is queued.- Keep
js_errandqueries_arrayalive.- const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); - const targetValue = this.getTarget(globalObject, true) orelse return; + const thisValue = this.thisValue.tryGet() orelse return; + defer this.thisValue.downgrade(); + const targetValue = this.getTarget(globalObject, true) orelse return; + // Clear other caches to avoid leaks/stale refs + allowGC(thisValue, globalObject); + // Protect `this` while enqueueing callback + var strong_this = jsc.Strong.Optional.create(thisValue, globalObject); + defer strong_this.deinit(); @@ - const function = vm.rareData().postgresql_context.onQueryRejectFn.get().?; + const function = vm.rareData().postgresql_context.onQueryRejectFn.get() orelse return; + bun.assertf(function.isCallable(), "onQueryRejectFn is not callable", .{}); @@ - const js_err = postgresErrorToJS(globalObject, null, err); + const js_err = postgresErrorToJS(globalObject, null, err); + js_err.ensureStillAlive(); - event_loop.runCallback(function, globalObject, thisValue, &.{ + const js_array = if (queries_array == .zero) .js_undefined else queries_array; + js_array.ensureStillAlive(); + event_loop.runCallback(function, globalObject, thisValue, &.{ targetValue, - js_err.toError() orelse js_err, - queries_array, + js_err.toError() orelse js_err, + js_array, });
63-67: Break native cycles in finalize before derefMirror MySQL: release any statement to avoid ref cycles preventing deinit.
pub fn finalize(this: *@This()) void { debug("PostgresSQLQuery finalize", .{}); - this.thisValue.finalize(); + if (this.statement) |statement| { + statement.deref(); + this.statement = null; + } + this.thisValue.finalize(); this.deref(); }src/sql/mysql/MySQLQuery.zig (2)
161-178: Mirror failure-path cleanup and strong ref in onJSErrorSame reasoning as onWriteFail.
this.status = .fail; const thisValue = this.thisValue.tryGet() orelse return; defer this.thisValue.downgrade(); const targetValue = this.getTarget(globalObject, true) orelse return; + js.bindingSetCached(thisValue, globalObject, .zero); + js.pendingValueSetCached(thisValue, globalObject, .zero); + var strong_this = jsc.Strong.Optional.create(thisValue, globalObject); + defer strong_this.deinit(); @@ var js_error = err.toError() orelse err; if (js_error == .zero) { js_error = AnyMySQLError.mysqlErrorToJS(globalObject, "Query failed", error.UnknownError); } js_error.ensureStillAlive();
75-94: Also clear binding/pending caches and hold a temporary strong refTarget cache is cleared via
clean_target=true, but binding/pending remain. Clear them and keepthisValuestrong until enqueue completes.this.status = .fail; const thisValue = this.thisValue.tryGet() orelse return; defer this.thisValue.downgrade(); const targetValue = this.getTarget(globalObject, true) orelse return; + js.bindingSetCached(thisValue, globalObject, .zero); + js.pendingValueSetCached(thisValue, globalObject, .zero); + var strong_this = jsc.Strong.Optional.create(thisValue, globalObject); + defer strong_this.deinit(); @@ const instance = AnyMySQLError.mysqlErrorToJS(globalObject, "Failed to bind query", err); const js_err = instance.toError() orelse instance; bun.assertf(js_err != .zero, "js_err is zero", .{}); js_err.ensureStillAlive(); @@ const event_loop = vm.eventLoop(); - const js_array = if (queries_array == .zero) .js_undefined else queries_array; + const js_array = if (queries_array == .zero) .js_undefined else queries_array; js_array.ensureStillAlive();
🧹 Nitpick comments (2)
src/sql/postgres/PostgresSQLQuery.zig (1)
492-494: Align logging with repo guidelinesUse
const log = bun.Output.scoped(.Postgres, false);and calllog("...", .{}).-const debug = bun.Output.scoped(.Postgres, .visible); +const log = bun.Output.scoped(.Postgres, false);Note: update call sites accordingly.
src/sql/mysql/MySQLQuery.zig (1)
523-524: Align logging with repo guidelinesUse
const log = bun.Output.scoped(.MySQLQuery, false);andlog("...", .{}).-const debug = bun.Output.scoped(.MySQLQuery, .visible); +const log = bun.Output.scoped(.MySQLQuery, false);Note: update call sites accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- 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/MySQLQuery.zig(8 hunks)src/sql/postgres/PostgresSQLQuery.zig(7 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("...", .{})
Files:
src/sql/mysql/MySQLQuery.zigsrc/sql/postgres/PostgresSQLQuery.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/sql/mysql/MySQLQuery.zigsrc/sql/postgres/PostgresSQLQuery.zig
🧠 Learnings (5)
📚 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/sql/postgres/PostgresSQLQuery.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/sql/postgres/PostgresSQLQuery.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/sql/postgres/PostgresSQLQuery.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/sql/postgres/PostgresSQLQuery.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 : For properties marked cache: true, use the generated Zig accessors (<Prop>NameSetCached/GetCached) to work with GC-owned values
Applied to files:
src/sql/postgres/PostgresSQLQuery.zig
🔇 Additional comments (9)
src/sql/postgres/PostgresSQLQuery.zig (3)
2-2: RefCount swap to bun.ptr.RefCount looks correctMatches the new pattern across SQL query types.
26-33: Optional getTarget return is a good safety improvementReturning null instead of trapping is preferable. Call sites handle it.
241-246: setPendingValue via callframe.this is fineMatches the new pattern used across SQL queries.
src/sql/mysql/MySQLQuery.zig (6)
2-2: RefCount swap to bun.ptr.RefCount looks correctConsistent with Postgres and broader refactor.
64-66: Finalize path looks goodJSRef finalize then native deref; statement is released above. Matches guidance.
118-121: Safe access to cached bindings/columnsUsing
tryGet+js_undefineddefaults is correct.
179-186: getTarget optional-return LGTMClears target on demand and avoids traps.
315-320: setPendingValue via callframe.this is correctMatches the new JSRef-driven pattern.
355-356: Connection ref handling LGTM
defer connection.updateRef();clearly pairs lifecycle with doRun scope.
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/sql/mysql/MySQLConnection.zig (1)
1314-1327: Fix on_connect “this” binding to avoid shifting argsSame issue as on_close: pass .js_undefined as this, then (null, js_value) as args.
Apply:
- this.globalObject.queueMicrotask(on_connect, &[_]JSValue{ JSValue.jsNull(), js_value }); + this.globalObject.queueMicrotask(on_connect, &[_]JSValue{ .js_undefined, JSValue.jsNull(), js_value });
♻️ Duplicate comments (1)
src/sql/mysql/MySQLConnection.zig (1)
1901-1906: Guard optional cached_structure before .jsValue()Same issue previously reported: cached_structure can be null; calling cached_structure.?.jsValue() can trap. Guard it.
Apply:
- cached_structure = if (this.js_value.tryGet()) |value| statement.structure(value, this.globalObject) else null; - structure = cached_structure.?.jsValue() orelse .js_undefined; + cached_structure = if (this.js_value.tryGet()) |value| + statement.structure(value, this.globalObject) + else + null; + structure = if (cached_structure) |cs| + cs.jsValue() orelse .js_undefined + else + .js_undefined;
🧹 Nitpick comments (1)
src/sql/mysql/MySQLConnection.zig (1)
369-373: Comment contradicts execution orderThe defer runs before the queued microtask, so requests are cleaned up before on_close runs. Either update the comment to reflect reality or move cleanup to run after on_close (e.g., schedule a subsequent microtask).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/sql/mysql/MySQLConnection.zig(19 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("...", .{})
Files:
src/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.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/sql/mysql/MySQLConnection.zig
🧠 Learnings (2)
📓 Common learnings
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
📚 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/sql/mysql/MySQLConnection.zig
🔇 Additional comments (14)
src/sql/mysql/MySQLConnection.zig (14)
24-24: JSRef field initialization looks correctUsing jsc.JSRef.empty() is appropriate for default-initializing the JS handle.
124-131: Reference upgrade/downgrade policy — verify all mutation sites call thisLogic to upgrade when requests exist or not-disconnected, and downgrade otherwise, is sound. Ensure every place mutating status/queue calls updateReferenceType() (you already do in setStatus, disconnect, doRef/doUnref).
268-274: Finalize JSRef before releasing native refCalling this.js_value.finalize() then deref() is the right order to let GC drop JS wrappers safely.
276-286: doRef/doUnref now delegate to updateReferenceTypeGood: poll_ref ref/unref paired with JSRef upgrade/downgrade centralization reduces lifecycle drift.
351-355: Guarded access to queries arrayUsing this.js_value.tryGet() avoids dereferencing an empty/weak ref. LGTM.
401-410: onClose defers deref; onEnd calls fail — confirm single pathonClose -> onEnd -> failWithJSValue is fine and idempotent via status==.failed guard. Verify onEnd isn’t also invoked by the transport for the same connection to avoid double work.
427-431: Defer updateReferenceType on disconnectGood: status change to .disconnected followed by ref-type recalculation prevents dangling strong refs.
1016-1021: Keep native ref while socket is open; recalc ref-typeRef on open and updateReferenceType() are appropriate to keep the process alive during handshake.
1158-1164: Packet length handling — small API consistency checkUsing header_length + PacketHeader.size and ensuring capacity before deferring skip is correct. Confirm NewReader(Context).ensureCapacity(packet_length) is an error-union in both StackReader and Reader; if it returns bool in any context, prefer:
if (!reader.ensureCapacity(packet_length)) return AnyMySQLError.Error.ShortRead;
1284-1296: consumeOnConnect now uses JSRef.tryGetSafer callback retrieval/caching. LGTM.
1298-1310: consumeOnClose now uses JSRef.tryGetMatches the onConnect pattern and avoids null traps. LGTM.
1789-1794: Passing possibly-undefined connection value to onResultUsing this.js_value.tryGet() orelse .js_undefined is correct under the new JSRef lifecycle.
1912-1912: pending_value retrieval via tryGet is correctAvoids reaching into a dead JS object; good defensive pattern.
1930-1934: Only set pendingValue when emptyThe isEmptyOrUndefinedOrNull() check prevents unnecessary overwrites. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/sql/mysql/MySQLQuery.zig (3)
161-178: Also take a temporary strong in onJSError to protectthisduring enqueueSame lifecycle concern as above/onWriteFail. Take a short-lived strong while scheduling.
- const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); + const thisValue = this.thisValue.tryGet() orelse return; + var strong_this = jsc.Strong.Optional.create(thisValue, globalObject); + defer strong_this.deinit(); + defer this.thisValue.downgrade();
210-216: Free caches on last result before downgrade (parity with PG)Clear binding/pending caches when
is_lastto avoid leaks/stale state.- defer if (is_last) { - this.thisValue.downgrade(); - }; + defer if (is_last) { + js.bindingSetCached(thisValue, globalObject, .zero); + js.pendingValueSetCached(thisValue, globalObject, .zero); + this.thisValue.downgrade(); + };
75-93: Protectthisduring reject enqueue; add a temporary strong and clear cachesGuard the JS
thiswhile enqueuing the callback, and clear per-query caches since this is a terminal failure.- const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); + const thisValue = this.thisValue.tryGet() orelse return; + var strong_this = jsc.Strong.Optional.create(thisValue, globalObject); + defer strong_this.deinit(); + defer this.thisValue.downgrade(); @@ - const js_array = if (queries_array == .zero) .js_undefined else queries_array; + const js_array = if (queries_array == .zero) .js_undefined else queries_array; js_array.ensureStillAlive(); + // terminal: free caches + js.bindingSetCached(thisValue, globalObject, .zero); + js.pendingValueSetCached(thisValue, globalObject, .zero);
🧹 Nitpick comments (2)
src/sql/postgres/CommandTag.zig (1)
46-51: Watch for precision loss converting u64 → JS NumberJSValue.jsNumber(val) will lose integer precision for values > Number.MAX_SAFE_INTEGER (2^53−1 = 9,007,199,254,740,991). toJSNumber is invoked at src/sql/postgres/PostgresSQLQuery.zig:154 and src/sql/mysql/MySQLQuery.zig:228; the counts come from CommandTag.init (parses u64) and MySQL OKPacket affected_rows (src/sql/mysql/protocol/OKPacket.zig:24-26). Choose one: return a JS BigInt/string for values > 2^53−1 or document/truncation as intended.
src/sql/mysql/MySQLQuery.zig (1)
224-230: Optionally keep queries_array alive before schedulingMirror the error path: retain
queries_arrayto avoid GC races during enqueue.- event_loop.runCallback(function, globalObject, thisValue, &.{ + queries_array.ensureStillAlive(); + event_loop.runCallback(function, globalObject, thisValue, &.{ @@ - queries_array, + queries_array,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/sql/mysql/MySQLConnection.zig(19 hunks)src/sql/mysql/MySQLQuery.zig(9 hunks)src/sql/postgres/CommandTag.zig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sql/mysql/MySQLConnection.zig
🧰 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("...", .{})
Files:
src/sql/postgres/CommandTag.zigsrc/sql/mysql/MySQLQuery.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/sql/postgres/CommandTag.zigsrc/sql/mysql/MySQLQuery.zig
🧠 Learnings (11)
📓 Common learnings
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
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/sql/postgres/CommandTag.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/sql/postgres/CommandTag.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 : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/sql/postgres/CommandTag.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 : Implement getters as get<PropertyName>(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Applied to files:
src/sql/postgres/CommandTag.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 src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JSC-bound classes
Applied to files:
src/sql/postgres/CommandTag.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,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/sql/postgres/CommandTag.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/bun.js/api/BunObject.zig : Create a getter function in BunObject.zig (define const Getter with toJSGetter and export it in exportAll)
Applied to files:
src/sql/postgres/CommandTag.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 Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/sql/postgres/CommandTag.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/sql/postgres/CommandTag.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/sql/postgres/CommandTag.zig
🔇 Additional comments (9)
src/sql/postgres/CommandTag.zig (1)
42-42: LGTM: safer JS string bridging for “other” tagsSwitching to
bun.String.createUTF8ForJS(globalObject, tag)avoids the temporaryZigStringwrapper and aligns with current interop patterns in this PR. Looks good.src/sql/mysql/MySQLQuery.zig (8)
2-2: RefCount migration LGTMSwitch to bun.ptr.RefCount with unchanged public API looks good.
64-66: Finalize ordering LGTMFinalizing JSRef before deref’ing the Zig refcount is correct.
118-121: Binding cache access defaults LGTMUsing
.js_undefinedfallbacks for binding/columns is consistent with iterator semantics.
179-186: getTarget API LGTMNullable return and optional cache clean-up read clearly.
201-201: onResult signature LGTMParameters/order align with resolve path usage.
218-220: Callable guard LGTMValidating
onQueryResolveFnis callable is good defensive practice.
309-314: Fix forthiscontext in setPendingValue LGTMUsing
callframe.this()resolves the “this” binding pitfall.
349-349: Deferred connection.updateRef LGTMMatches the new ref-driven lifecycle.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/sql/postgres/PostgresSQLQuery.zig (2)
79-81: Clear all caches on failure to avoid leaksOn failure you only clear target via getTarget(clean_target=true). Also clear binding/pending (parity with allowGC on success).
- defer this.thisValue.downgrade(); + defer { + allowGC(thisValue, globalObject); + this.thisValue.downgrade(); + }
96-99: Also clear caches on JS error pathSame reasoning as onWriteFail; free binding/pending caches before downgrading.
- defer this.thisValue.downgrade(); + defer { + allowGC(thisValue, globalObject); + this.thisValue.downgrade(); + }src/sql/mysql/MySQLQuery.zig (3)
75-89: Protect thisArg during callback enqueue and clear caches on failureTake a temporary strong ref to thisValue while scheduling; also clear binding/pending caches before downgrade.
- const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); + const thisValue = this.thisValue.tryGet() orelse return; + var strong_this = jsc.Strong.Optional.create(thisValue, globalObject); + defer strong_this.deinit(); + defer { + js.bindingSetCached(thisValue, globalObject, .zero); + js.pendingValueSetCached(thisValue, globalObject, .zero); + this.thisValue.downgrade(); + }
161-177: Mirror failure-path protections in onJSErrorApply the same strong-ref and cache clearing here.
- const thisValue = this.thisValue.tryGet() orelse return; - defer this.thisValue.downgrade(); + const thisValue = this.thisValue.tryGet() orelse return; + var strong_this = jsc.Strong.Optional.create(thisValue, globalObject); + defer strong_this.deinit(); + defer { + js.bindingSetCached(thisValue, globalObject, .zero); + js.pendingValueSetCached(thisValue, globalObject, .zero); + this.thisValue.downgrade(); + }
215-221: Free caches on last result for parity and memory hygieneClear binding/pending when is_last before downgrade.
- defer if (is_last) { - this.thisValue.downgrade(); - }; + defer if (is_last) { + js.bindingSetCached(thisValue, globalObject, .zero); + js.pendingValueSetCached(thisValue, globalObject, .zero); + this.thisValue.downgrade(); + };
🧹 Nitpick comments (2)
src/sql/postgres/PostgresSQLQuery.zig (1)
26-33: Keep returned target alive when clearing cacheWhen clean_target is true, the cached slot is nulled; keep a temporary alive handle to avoid GC before enqueue.
- const target = js.targetGetCached(thisValue) orelse return null; + const target = js.targetGetCached(thisValue) orelse return null; + target.ensureStillAlive();src/sql/mysql/MySQLQuery.zig (1)
231-234: Normalize queries_array to undefined when zeroPrevents passing a zero handle into JS.
- queries_array, + (if (queries_array == .zero) .js_undefined else queries_array),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/sql/mysql/MySQLQuery.zig(8 hunks)src/sql/postgres/CommandTag.zig(2 hunks)src/sql/postgres/PostgresSQLQuery.zig(7 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("...", .{})
Files:
src/sql/postgres/CommandTag.zigsrc/sql/postgres/PostgresSQLQuery.zigsrc/sql/mysql/MySQLQuery.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/sql/postgres/CommandTag.zigsrc/sql/postgres/PostgresSQLQuery.zigsrc/sql/mysql/MySQLQuery.zig
🧠 Learnings (16)
📓 Common learnings
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
📚 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/sql/postgres/CommandTag.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/sql/postgres/CommandTag.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/sql/postgres/CommandTag.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 : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/sql/postgres/CommandTag.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 : Implement getters as get<PropertyName>(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Applied to files:
src/sql/postgres/CommandTag.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 src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JSC-bound classes
Applied to files:
src/sql/postgres/CommandTag.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/bun.js/api/BunObject.zig : Create a getter function in BunObject.zig (define const Getter with toJSGetter and export it in exportAll)
Applied to files:
src/sql/postgres/CommandTag.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 Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/sql/postgres/CommandTag.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/sql/postgres/CommandTag.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/sql/postgres/CommandTag.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/sql/postgres/PostgresSQLQuery.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/sql/postgres/PostgresSQLQuery.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/sql/postgres/PostgresSQLQuery.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/sql/postgres/PostgresSQLQuery.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 : For properties marked cache: true, use the generated Zig accessors (<Prop>NameSetCached/GetCached) to work with GC-owned values
Applied to files:
src/sql/postgres/PostgresSQLQuery.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
🔇 Additional comments (6)
src/sql/postgres/CommandTag.zig (1)
32-32: All callers updated — no action requiredrg found usages in src/sql/mysql/MySQLQuery.zig and src/sql/postgres/PostgresSQLQuery.zig; both handle the error union using
catch |e| return this.onJSError(...).src/sql/postgres/PostgresSQLQuery.zig (1)
138-157: LGTM: safer tag conversion and error propagationUsing CommandTag.toJSTag with error handling and keeping js_tag alive is correct.
src/sql/mysql/MySQLQuery.zig (4)
118-121: Good: defaulting missing caches to js_undefinedAvoids .zero propagation during binding.
179-186: LGTM: getTarget optional return with cache clearingOptional target with clean_target flag is a good simplification.
353-354: Connection ref mgmt switch to updateRef looks goodDefer-based updateRef aligns with new lifecycle.
313-318: setPendingValue: ignore this and use callframe.this is correctMatches the new public API surface.
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/sql/mysql/MySQLConnection.zig (1)
1310-1323: Fix on_connect this-binding/args to match public APISame microtask calling convention issue as
on_close: include explicitthisfirst, then(null, js_value).- this.globalObject.queueMicrotask(on_connect, &[_]JSValue{ JSValue.jsNull(), js_value }); + this.globalObject.queueMicrotask(on_connect, &[_]JSValue{ .js_undefined, JSValue.jsNull(), js_value });
♻️ Duplicate comments (2)
src/sql/mysql/MySQLConnection.zig (2)
364-393: Fix on_close this-binding and argument order passed to queueMicrotaskCurrently
js_errorbecomes thethisvalue and shifts args. Pass an explicitthisfirst, then(error, queries); this likely contributes to the reported segfault/timeout.Apply:
- this.globalObject.queueMicrotask(on_close, &[_]JSValue{ js_error, queries_array }); + this.globalObject.queueMicrotask(on_close, &[_]JSValue{ .js_undefined, js_error, queries_array });Optional: mirror Postgres by using
on_close.call(this.globalObject, .js_undefined, &[_]JSValue{ js_error, queries_array })and surface exceptions, if desired.
1898-1901: Guard optional before calling jsValue() to prevent trap/segfault
cached_structurecan be null;cached_structure.?.jsValue()may trap. Guard and return.js_undefinedwhen absent. This likely explains the Linux segfault noted by CI.- cached_structure = if (this.js_value.tryGet()) |value| statement.structure(value, this.globalObject) else null; - structure = cached_structure.?.jsValue() orelse .js_undefined; + cached_structure = if (this.js_value.tryGet()) |value| + statement.structure(value, this.globalObject) + else + null; + structure = if (cached_structure) |cs| + cs.jsValue() orelse .js_undefined + else + .js_undefined;
🧹 Nitpick comments (1)
src/sql/mysql/MySQLConnection.zig (1)
1329-1336: Minor: prefer an intent-revealing check for JSRef strengthIf
JSRefexposes a helper (e.g.,isStrong()), use it over== .strongfor readability. If not, ignore.
📜 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/sql/mysql/MySQLConnection.zig(20 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("...", .{})
Files:
src/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.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/sql/mysql/MySQLConnection.zig
🧠 Learnings (1)
📓 Common learnings
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
🔇 Additional comments (9)
src/sql/mysql/MySQLConnection.zig (9)
24-24: JSRef lifecycle upgrade/downgrade — looks goodAdopting
jsc.JSRefand the upgrade/downgrade gating inupdateReferenceType()is sane and aligns with request/state-driven liveness.Also applies to: 124-132
227-237: Timeout messaging clarity — OKReworded messages for connecting/auth are clearer and preserve error codes.
Also applies to: 762-762
268-286: Finalize/ref/unref flow — OK
js_value.finalize()plus updating reference type indoRef/doUnrefis correct given the new JSRef model.
351-355: Guarded access to queries array — OKUsing
tryGet()and returning.js_undefinedis safe; propagation to callbacks is consistent.Also applies to: 1789-1790
429-430: Disconnect and deinit cleanups — OKDeferred
updateReferenceType()on disconnect and explicitrequest.deref()during deinit are correct.Also applies to: 975-976
1012-1017: onOpen handshake transition + ref — OKSetting
.handshakingonly for TCP and holding a ref for the socket aligns with upgrade semantics; updating ref type is appropriate.
1154-1160: Packet-boundary handling — OKUsing
packet_length = header_length + PacketHeader.size, ensuring capacity, and deferringsetOffsetFromStart(packet_length)avoids partial reads.
1282-1292: consumeOnConnect/consumeOnClose — OK
tryGet()+ cache clearing + zero checks prevent stale callbacks and accidental re-entry.Also applies to: 1296-1306
1908-1909: Pending value caching via tryGet — OKSafe retrieval and conditional set avoid leaking intermediate row state.
Also applies to: 1926-1929
3 participants
What does this PR do?
Fix memories issues, exports in internals and handling of hasPendingActivity
How did you verify your code works?
CI