Port SocketConfig to bindings generator#23755
Conversation
(For internal tracking: fixes STAB-1471, STAB-1472, STAB-1473, STAB-1474, STAB-1475, STAB-1476)
|
Updated 5:55 PM PT - Oct 18th, 2025
❌ @Jarred-Sumner, your commit 7b30f0e has some failures in 🧪 To try this PR locally: bunx bun-pr 23755That installs a local version of the PR into your bun-23755 --bun |
WalkthroughRefactors socket handler lifecycle to use local handler clones and explicit deinitialization, ports SocketConfig/Handlers to bindgenv2-generated payloads, extends bindgenv2 with "loose" nullable/primitive types, adjusts Zig/C++ binding utilities, and adds allocator-aware string/memory helpers. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.zig📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
src/bun.js/**/*.zig📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
Files:
src/**/*.zig📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-08-30T00:13:36.815ZApplied to files:
📚 Learning: 2025-09-02T18:27:23.279ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
src/bun.js/api/bun/socket.zig(2 hunks)src/bun.js/api/bun/socket/Handlers.zig(7 hunks)src/bun.js/api/bun/socket/Listener.zig(16 hunks)src/bun.js/api/bun/socket/SocketConfig.bindv2.ts(1 hunks)src/bun.js/api/server/SSLConfig.zig(3 hunks)src/bun.js/bindings/Bindgen/ExternTraits.h(1 hunks)src/bun.js/bindings/Bindgen/ExternUnion.h(1 hunks)src/bun.js/bindings/ZigString.zig(2 hunks)src/codegen/bindgenv2/internal/enumeration.ts(4 hunks)src/codegen/bindgenv2/tsconfig.json(1 hunks)src/memory.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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
Files:
src/bun.js/api/server/SSLConfig.zigsrc/bun.js/api/bun/socket.zigsrc/memory.zigsrc/bun.js/api/bun/socket/Listener.zigsrc/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/bindings/ZigString.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/SSLConfig.zigsrc/bun.js/api/bun/socket.zigsrc/bun.js/api/bun/socket/Listener.zigsrc/bun.js/api/bun/socket/Handlers.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/server/SSLConfig.zigsrc/bun.js/api/bun/socket.zigsrc/bun.js/api/bun/socket/Listener.zigsrc/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/bindings/ZigString.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")
Files:
src/bun.js/api/server/SSLConfig.zigsrc/bun.js/api/bun/socket.zigsrc/memory.zigsrc/bun.js/api/bun/socket/Listener.zigsrc/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/bindings/ZigString.zig
**/*.{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/Bindgen/ExternUnion.hsrc/bun.js/bindings/Bindgen/ExternTraits.h
🧠 Learnings (3)
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
PR: oven-sh/bun#23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/codegen/bindgenv2/internal/enumeration.ts
📚 Learning: 2025-09-02T18:27:23.279Z
Learnt from: taylordotfish
PR: oven-sh/bun#22227
File: src/collections/multi_array_list.zig:24-24
Timestamp: 2025-09-02T18:27:23.279Z
Learning: The `#allocator` syntax in bun's custom Zig implementation is valid and does not require quoting with @"#allocator". Private fields using the `#` prefix work correctly throughout the codebase without special quoting syntax.
Applied to files:
src/bun.js/bindings/ZigString.zig
📚 Learning: 2025-09-02T17:14:46.924Z
Learnt from: taylordotfish
PR: oven-sh/bun#22227
File: src/safety/alloc.zig:93-95
Timestamp: 2025-09-02T17:14:46.924Z
Learning: In bun's Zig codebase, they use a custom extension of Zig that supports private field syntax with the `#` prefix (e.g., `#allocator`, `#trace`). This is not standard Zig syntax but is valid in their custom implementation. Fields prefixed with `#` are private fields that cannot be accessed from outside the defining struct.
Applied to files:
src/bun.js/bindings/ZigString.zig
🧬 Code graph analysis (3)
src/bun.js/api/bun/socket/SocketConfig.bindv2.ts (1)
src/bun.js/api/server/SSLConfig.bindv2.ts (1)
SSLConfig(23-90)
src/bun.js/bindings/Bindgen/ExternTraits.h (1)
src/bun.js/bindings/BunIDLConvert.h (1)
std(255-279)
src/codegen/bindgenv2/internal/enumeration.ts (1)
src/codegen/bindgenv2/internal/base.ts (2)
joinIndented(121-123)toQuotedLiteral(125-127)
⏰ 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/Bindgen/ExternUnion.h (2)
17-28: LGTM! Constructor-based initialization improves safety.The new constructor properly enforces initialization at construction time, which is more robust than the previous two-step initialization pattern. The
static_assertensures FFI safety by verifying trivial copyability, and the variant visiting logic correctly activates the appropriate union member.
32-32: LGTM! Simplified field macro.Moving the
static_assertto the constructor (line 19) is more appropriate, as it only needs to be checked once per union specialization rather than per field.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (3)
src/bun.js/api/bun/socket/SocketConfig.bindv2.ts(1 hunks)src/bun.js/bindings/Bindgen/ExternUnion.h(1 hunks)test/internal/ban-limits.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/internal/ban-limits.json
**/*.{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/Bindgen/ExternUnion.h
🧠 Learnings (1)
📚 Learning: 2025-09-26T01:10:04.233Z
Learnt from: nektro
PR: oven-sh/bun#22781
File: test/internal/ban-limits.json:25-26
Timestamp: 2025-09-26T01:10:04.233Z
Learning: The test/internal/ban-limits.json file contains accurate counts that are validated by tests - if the counts were incorrect, the associated tests would fail, so the values in this file reflect the actual current state of the codebase.
Applied to files:
test/internal/ban-limits.json
🧬 Code graph analysis (1)
src/bun.js/api/bun/socket/SocketConfig.bindv2.ts (1)
src/bun.js/api/server/SSLConfig.bindv2.ts (1)
SSLConfig(23-90)
🔇 Additional comments (1)
src/bun.js/bindings/Bindgen/ExternUnion.h (1)
33-33: LGTM! Field macro simplification is consistent with constructor-based validation.Moving the static_assert to the constructor allows this macro to focus solely on field declarations, improving clarity.
| ExternUnion(std::variant<T0 __VA_OPT__(, ) __VA_ARGS__>&& variant) \ | ||
| { \ | ||
| using This = std::decay_t<decltype(*this)>; \ | ||
| static_assert(std::is_trivially_copyable_v<This>); \ | ||
| const std::size_t index = variant.index(); \ | ||
| std::visit([this, index](auto&& arg) { \ | ||
| using Arg = std::decay_t<decltype(arg)>; \ | ||
| BUN_BINDGEN_DETAIL_FOREACH( \ | ||
| BUN_BINDGEN_DETAIL_EXTERN_UNION_VISIT, \ | ||
| T0 __VA_OPT__(, ) __VA_ARGS__) \ | ||
| }, \ | ||
| std::move(variant)); \ | ||
| } \ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Constructor correctly initializes union from variant.
The new constructor properly achieves STAB-1475's goal: it allows ExternUnion to have a user-defined constructor while remaining trivially copyable (verified by the static_assert). The initialization logic using std::visit with index verification is sound.
Consider adding a defensive check after the lambda to catch unexpected index mismatches:
std::visit([this, index](auto&& arg) {
using Arg = std::decay_t<decltype(arg)>;
BUN_BINDGEN_DETAIL_FOREACH(
BUN_BINDGEN_DETAIL_EXTERN_UNION_VISIT,
T0 __VA_OPT__(, ) __VA_ARGS__)
+ // If we reach here, no field was initialized
+ __builtin_unreachable();
},
std::move(variant));This would make logic errors in the macro system fail fast rather than silently leaving the union uninitialized.
🤖 Prompt for AI Agents
In src/bun.js/bindings/Bindgen/ExternUnion.h around lines 17-29, add a defensive
runtime check immediately after the std::visit call to ensure the union was
actually initialized for the expected variant index; if the active member does
not match the captured index, fail fast (e.g., assert(false) or call
std::terminate()/abort with a clear message) so macro-system logic errors cannot
leave the union uninitialized.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/bun.js/api/bun/socket/Handlers.zig (1)
290-304: Restore hostname defaults and propagate flags outside the hostname branch.This is the same regression called out earlier: rejecting configs that omit both
hostnameandunixbreaksBun.listen({ port: 0 })/Bun.connect({ port: 80 }), which rely on the implicit default host selection. We still need to materialize a default host (0.0.0.0for servers,127.0.0.1for clients) whenever neither field is provided, while requiring an explicit port. Additionally, because the flag assignments now sit inside the hostname branch, any unix or fd-based config silently dropsexclusive,allowHalfOpen,reusePort, andipv6Only. Both behaviors worked before the refactor and must remain intact.} else if (generated.hostname.get()) |hostname| { bun.assertf(hostname.length() > 0, "truthy bindgen string shouldn't be empty", .{}); result.hostname_or_unix = hostname.toUTF8(bun.default_allocator); const slice = result.hostname_or_unix.slice(); result.port = generated.port orelse bun.URL.parse(slice).getPort() orelse { return global.throwInvalidArguments("Missing \"port\"", .{}); }; - result.exclusive = generated.exclusive; - result.allowHalfOpen = generated.allow_half_open; - result.reusePort = generated.reuse_port; - result.ipv6Only = generated.ipv6_only; } else { - return global.throwInvalidArguments("Expected either \"hostname\" or \"unix\"", .{}); + const port_value = generated.port orelse { + return global.throwInvalidArguments("Missing \"port\"", .{}); + }; + const default_host: []const u8 = if (is_server) "0.0.0.0" else "127.0.0.1"; + const dup = bun.handleOom(bun.default_allocator.dupe(u8, default_host)); + result.hostname_or_unix = .init(bun.default_allocator, dup); + result.port = port_value; } + result.exclusive = generated.exclusive; + result.allowHalfOpen = generated.allow_half_open; + result.reusePort = generated.reuse_port; + result.ipv6Only = generated.ipv6_only;src/bun.js/api/bun/socket/Listener.zig (1)
645-647: UsetakeServerName()when populating TLS SNI.We’re still draining ALPN by calling
takeProtos()for theserver_namefield, so SNI never propagates (and the ALPN list gets consumed twice). This is the same bug flagged earlier—please usetakeServerName()for SNI and keeptakeProtos()solely for the ALPN payload.- .server_name = if (ssl) |s| s.takeProtos() else null, + .server_name = if (ssl) |s| s.takeServerName() else null, … - .server_name = if (ssl) |s| s.takeProtos() else null, + .server_name = if (ssl) |s| s.takeServerName() else null,Also applies to: 779-782
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (15)
src/bun.js/api/bun/socket/Handlers.zig(7 hunks)src/bun.js/api/bun/socket/Listener.zig(16 hunks)src/bun.js/api/bun/socket/SocketConfig.bindv2.ts(1 hunks)src/bun.js/bindings/BunIDLConvert.h(1 hunks)src/bun.js/bindings/BunIDLConvertNumbers.h(1 hunks)src/bun.js/bindings/BunIDLHumanReadable.h(1 hunks)src/bun.js/bindings/BunIDLTypes.h(2 hunks)src/bun.js/bindings/webcore/JSDOMConvertNullable.h(1 hunks)src/codegen/bindgenv2/internal/base.ts(1 hunks)src/codegen/bindgenv2/internal/dictionary.ts(1 hunks)src/codegen/bindgenv2/internal/optional.ts(4 hunks)src/codegen/bindgenv2/internal/primitives.ts(6 hunks)test/js/bun/http/bun-listen-connect-args.test.ts(0 hunks)test/js/bun/net/socket.test.ts(1 hunks)test/js/node/test/parallel/test-net-connect-custom-lookup-non-string-address.mjs(2 hunks)
💤 Files with no reviewable changes (1)
- test/js/bun/http/bun-listen-connect-args.test.ts
🧰 Additional context used
📓 Path-based instructions (16)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/bun/net/socket.test.tstest/js/node/test/parallel/test-net-connect-custom-lookup-non-string-address.mjs
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/bun/net/socket.test.ts
test/js/bun/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Files:
test/js/bun/net/socket.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/bun/net/socket.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/bun/net/socket.test.ts
test/js/bun/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Bun-specific API tests under test/js/bun/
Files:
test/js/bun/net/socket.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/bun/net/socket.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/bun/net/socket.test.tstest/js/node/test/parallel/test-net-connect-custom-lookup-non-string-address.mjs
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/bun/net/socket.test.tstest/js/node/test/parallel/test-net-connect-custom-lookup-non-string-address.mjs
**/*.{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/BunIDLHumanReadable.hsrc/bun.js/bindings/BunIDLConvertNumbers.hsrc/bun.js/bindings/webcore/JSDOMConvertNullable.hsrc/bun.js/bindings/BunIDLTypes.hsrc/bun.js/bindings/BunIDLConvert.h
test/js/node/test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Vendored Node.js tests under test/js/node/test/ are exceptions and do not follow Bun’s test style
Files:
test/js/node/test/parallel/test-net-connect-custom-lookup-non-string-address.mjs
test/js/node/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Node.js compatibility tests under test/js/node/
Files:
test/js/node/test/parallel/test-net-connect-custom-lookup-non-string-address.mjs
**/*.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
Files:
src/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/api/bun/socket/Listener.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/api/bun/socket/Listener.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/api/bun/socket/Listener.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")
Files:
src/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/api/bun/socket/Listener.zig
🧠 Learnings (7)
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Never hardcode port numbers; use `port: 0` to get a random port
Applied to files:
test/js/bun/net/socket.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests, always use port: 0; do not hardcode ports or roll your own random port
Applied to files:
test/js/bun/net/socket.test.ts
📚 Learning: 2025-10-01T21:49:27.862Z
Learnt from: taylordotfish
PR: oven-sh/bun#23169
File: src/bun.js/bindings/BunIDLConvert.h:29-42
Timestamp: 2025-10-01T21:49:27.862Z
Learning: In Bun's IDL bindings (src/bun.js/bindings/BunIDLConvert.h), IDLStrictNull intentionally treats both undefined and null as null (using isUndefinedOrNull()), matching WebKit's IDLNull & IDLNullable behavior. This is the correct implementation and should not be changed to only accept null.
Applied to files:
src/bun.js/bindings/BunIDLHumanReadable.hsrc/bun.js/bindings/webcore/JSDOMConvertNullable.hsrc/bun.js/bindings/BunIDLTypes.hsrc/bun.js/bindings/BunIDLConvert.h
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
PR: oven-sh/bun#23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/BunIDLConvertNumbers.hsrc/bun.js/bindings/webcore/JSDOMConvertNullable.hsrc/bun.js/bindings/BunIDLTypes.hsrc/bun.js/bindings/BunIDLConvert.h
📚 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/webcore/JSDOMConvertNullable.h
📚 Learning: 2025-10-01T21:48:38.278Z
Learnt from: taylordotfish
PR: oven-sh/bun#23169
File: src/bun.js/bindings/Bindgen/IDLTypes.h:1-3
Timestamp: 2025-10-01T21:48:38.278Z
Learning: In the Bun codebase, for `BunIDL*` and `Bindgen*` headers (e.g., BunIDLTypes.h, Bindgen/IDLTypes.h), it's acceptable to rely on transitive includes for standard library headers like <type_traits> and <utility> rather than including them explicitly.
Applied to files:
src/bun.js/bindings/BunIDLTypes.h
📚 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_bindings,BunObject}.classes.ts : Add JSDoc comments to JavaScript binding class definitions
Applied to files:
src/codegen/bindgenv2/internal/base.ts
🧬 Code graph analysis (7)
src/bun.js/bindings/BunIDLConvertNumbers.h (2)
src/bun.js/bindings/BunIDLTypes.h (8)
WebCore(19-29)WebCore(32-32)WebCore(35-35)WebCore(43-43)WebCore(44-44)WebCore(45-45)WebCore(46-46)Bun(13-90)src/bun.js/bindings/BunIDLConvertBase.h (2)
convert(43-47)convert(55-66)
src/bun.js/bindings/BunIDLTypes.h (2)
src/bun.js/bindings/webcore/JSDOMConvertNullable.h (1)
WebCore(35-174)src/bun.js/bindings/IDLTypes.h (11)
WebCore(51-256)IDLType(94-95)IDLType(98-99)IDLType(102-114)IDLType(116-117)IDLType(119-120)IDLType(203-209)IDLType(347-352)IDLType(354-362)IDLType(364-365)IDLType(379-380)
src/codegen/bindgenv2/internal/primitives.ts (2)
src/codegen/bindgen-lib.ts (10)
Type(18-32)u8(156-156)u16(157-157)u32(158-158)u64(159-159)i8(160-160)i16(161-161)i32(162-162)i64(163-163)f64(154-154)src/codegen/bindgenv2/internal/base.ts (1)
CodeStyle(5-5)
src/bun.js/bindings/BunIDLConvert.h (2)
src/bun.js/bindings/BunIDLTypes.h (10)
WebCore(19-29)WebCore(32-32)WebCore(35-35)WebCore(43-43)WebCore(44-44)WebCore(45-45)WebCore(46-46)Bun(13-90)nullValue(26-26)nullValue(77-77)src/bun.js/bindings/webcore/JSDOMConvertNullable.h (7)
WebCore(35-174)optional(77-85)convert(88-93)convert(95-100)convert(101-106)convert(107-112)convert(129-134)
src/codegen/bindgenv2/internal/optional.ts (2)
src/codegen/bindgen-lib.ts (1)
Type(18-32)src/codegen/bindgenv2/internal/base.ts (2)
nullable(14-16)CodeStyle(5-5)
src/codegen/bindgenv2/internal/dictionary.ts (4)
src/codegen/bindgenv2/internal/optional.ts (1)
optional(11-32)src/bun.js/bindings/BunIDLConvert.h (3)
optional(51-60)optional(131-140)optional(240-306)src/bun.js/bindings/webcore/JSDOMConvertNullable.h (1)
optional(77-85)src/bun.js/bindings/webcore/JSDOMConvertOptional.h (1)
optional(41-53)
src/bun.js/api/bun/socket/SocketConfig.bindv2.ts (1)
src/bun.js/api/server/SSLConfig.bindv2.ts (1)
SSLConfig(23-90)
⏰ 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 (1)
src/codegen/bindgenv2/internal/primitives.ts (1)
199-229: Rename fromfiniteF64toFiniteF64is clean and complete.No references to the old
finiteF64identifier exist anywhere in the codebase. The newFiniteF64is used internally withinprimitives.tsand is not exported publicly vialib.ts. No downstream breakage.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/codegen/bindgenv2/internal/enumeration.ts (1)
86-97: Fix array-to-string coercion in human-readable name generation.Line 86 attempts to quote all values with
values.map(v => `"${v}"`)butvcan now be an array (for aliases). When an array is coerced to a string via template literal, it joins with commas, producing incorrect output like"bar,baz"instead of listing each alias separately.Apply this diff to flatten all aliases before quoting:
- const quotedValues = values.map(v => `"${v}"`); + const quotedValues = values + .flatMap(v => (typeof v === "object" ? v : [v])) + .map(s => `"${s}"`);src/bun.js/api/bun/socket/Listener.zig (1)
494-499: Wrong allocator used to destroy Listener; can corrupt heapListener instances are created with handlers.vm.allocator.create(...), but deinit() frees with bun.default_allocator.destroy(this). Freeing with a different allocator is undefined behavior.
As per coding guidelines, use bun.destroy(this) (or the same allocator that created it):
if (this.protos) |protos| { this.protos = null; bun.default_allocator.free(protos); } - bun.default_allocator.destroy(this); + bun.destroy(this);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (4)
src/bun.js/api/bun/socket/Listener.zig(16 hunks)src/bun.js/bindings/Bindgen/ExternTraits.h(1 hunks)src/bun.js/bindings/BunIDLConvert.h(1 hunks)src/codegen/bindgenv2/internal/enumeration.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/Bindgen/ExternTraits.hsrc/bun.js/bindings/BunIDLConvert.h
**/*.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
Files:
src/bun.js/api/bun/socket/Listener.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/bun/socket/Listener.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/bun/socket/Listener.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")
Files:
src/bun.js/api/bun/socket/Listener.zig
🧠 Learnings (2)
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
PR: oven-sh/bun#23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/codegen/bindgenv2/internal/enumeration.tssrc/bun.js/bindings/BunIDLConvert.h
📚 Learning: 2025-10-01T21:49:27.862Z
Learnt from: taylordotfish
PR: oven-sh/bun#23169
File: src/bun.js/bindings/BunIDLConvert.h:29-42
Timestamp: 2025-10-01T21:49:27.862Z
Learning: In Bun's IDL bindings (src/bun.js/bindings/BunIDLConvert.h), IDLStrictNull intentionally treats both undefined and null as null (using isUndefinedOrNull()), matching WebKit's IDLNull & IDLNullable behavior. This is the correct implementation and should not be changed to only accept null.
Applied to files:
src/bun.js/bindings/BunIDLConvert.h
🧬 Code graph analysis (2)
src/codegen/bindgenv2/internal/enumeration.ts (1)
src/codegen/bindgenv2/internal/base.ts (2)
joinIndented(123-125)toQuotedLiteral(127-129)
src/bun.js/bindings/BunIDLConvert.h (3)
src/bun.js/bindings/BunIDLTypes.h (10)
WebCore(19-29)WebCore(32-32)WebCore(35-35)WebCore(43-43)WebCore(44-44)WebCore(45-45)WebCore(46-46)Bun(13-90)nullValue(26-26)nullValue(77-77)src/bun.js/bindings/webcore/JSDOMConvertNullable.h (7)
WebCore(35-174)optional(77-85)convert(88-93)convert(95-100)convert(101-106)convert(107-112)convert(129-134)src/bun.js/bindings/BunIDLConvertBase.h (3)
optional(68-74)convert(43-47)convert(55-66)
🔇 Additional comments (10)
src/bun.js/bindings/Bindgen/ExternTraits.h (1)
71-75: Successfully addresses previous review feedback.The initializer list now correctly matches the declaration order (data, then tag), which eliminates potential compiler warnings. While calling
variant.index()after the move is valid (it accesses only the discriminator, not the contained value), capturing the index before moving would make the code slightly more explicit:explicit ExternVariant(std::variant<Args...>&& variant) - : data(std::move(variant)) - , tag(static_cast<std::uint8_t>(variant.index())) + : tag(static_cast<std::uint8_t>(variant.index())) + , data(std::move(variant)) { }This preserves the initialization order fix while making the dependency on
variantclearer. However, the current implementation is correct and the improvement is purely for readability.Likely an incorrect or invalid review comment.
src/codegen/bindgenv2/internal/enumeration.ts (5)
14-23: LGTM: Alias support and unique-value extraction.The documentation clearly explains the aliasing feature, and the
uniqueValuesextraction correctly handles both string and array inputs. The validation for empty arrays is appropriate.
31-39: Duplicate detection correctly implemented.The
valueMapconstruction with inline duplicate detection (via size comparison) correctly addresses the past review concern about duplicate aliases silently overwriting earlier entries. The logic ensures that repeated aliases across any variants are caught immediately.
74-79: LGTM: Index lookup viavalueMap.The change from positional lookup (
values.indexOf(value)) to map-based lookup (valueMap.get(value)) correctly supports aliases and provides O(1) performance.
150-160: LGTM: C++ source emission updated for aliases.The C++ mappings now correctly use
valueMap.sizeandArray.from(valueMap.entries())to emit all alias-to-enum-member pairs, ensuring each alias is mapped to the correct underlying integral value.
190-190: LGTM: Zig enum literals use unique values.Correctly emits only the primary (unique) values in the Zig enum definition, avoiding duplicate enum members while still supporting alias-aware lookups via the valueMap in other language targets.
src/bun.js/api/bun/socket/Listener.zig (4)
306-310: Good: safe conversion of sentinel string for SNI checkUsing std.mem.span for length check avoids UB and matches the sentinel-to-slice objective (STAB-1473). LGTM.
636-647: Good: server_name now uses takeServerName(), not takeProtos()Correct separation of ALPN (.protos) and SNI (.server_name) when reusing or creating TLSSocket. This fixes the prior SNI misassignment.
770-782: Good: consistent takeServerName()/takeProtos() for new/previous socketsConsistent, ownership-aware moves for ALPN and SNI across TCP/TLS connect paths. LGTM.
112-118: Good: deinitExcludingHandlers() + errdefer unprotect patternDeferring deinit while keeping handlers protected, and errdefer unprotect on failure, prevents handler leaks and double protects. LGTM.
Also applies to: 553-566
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codegen/bindgenv2/internal/enumeration.ts (1)
82-82: Fix incorrect human-readable name generation.Line 82 maps over the original
valuesarray, which may contain nested arrays for aliases. When stringified, these arrays produce malformed output like"foo,bar"instead of"foo". This results in confusing error messages.Apply this diff to use
uniqueValuesinstead:- const quotedValues = values.map(v => `"${v}"`); + const quotedValues = uniqueValues.map(v => `"${v}"`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/api/bun/socket/Listener.zig(16 hunks)src/codegen/bindgenv2/internal/enumeration.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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
Files:
src/bun.js/api/bun/socket/Listener.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/bun/socket/Listener.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/bun/socket/Listener.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")
Files:
src/bun.js/api/bun/socket/Listener.zig
🧠 Learnings (1)
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
PR: oven-sh/bun#23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/codegen/bindgenv2/internal/enumeration.ts
🧬 Code graph analysis (1)
src/codegen/bindgenv2/internal/enumeration.ts (1)
src/codegen/bindgenv2/internal/base.ts (2)
joinIndented(123-125)toQuotedLiteral(127-129)
⏰ 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 (10)
src/codegen/bindgenv2/internal/enumeration.ts (6)
14-18: LGTM! Clear documentation and appropriate signature.The JSDoc clearly explains the alias mapping behavior, and the function signature correctly reflects the new capability to accept arrays of strings for enum aliases.
31-39: LGTM! Duplicate detection correctly implemented.The logic properly catches duplicate aliases by comparing the map size before and after insertion. If the size remains unchanged after
set(), it indicates the key already existed, triggering an appropriate error. This correctly addresses the requirement to reject duplicate enum aliases.
41-55: LGTM! C++ member generation correctly refactored.The loop properly iterates over
uniqueValuesfor generating C++ member names and only handles naming collisions (not value duplicates, which are already validated earlier). The redundant duplicate check mentioned in past reviews has been correctly removed.
70-76: LGTM! Alias resolution correctly implemented.The
toCppmethod properly usesvalueMap.get(value)to resolve both primary values and aliases to their corresponding enum indices, with appropriate error handling for invalid values.
146-158: LGTM! C++ alias mappings correctly generated.The C++ source generation properly uses
valueMap.sizeandvalueMap.entries()to emit all alias-to-enum mappings, ensuring each alias string correctly resolves to its corresponding enum member at runtime.
184-187: LGTM! Zig enum generation correctly uses primary values.The Zig source generation appropriately uses
uniqueValuesto emit only the primary enum member names, as Zig enums don't require separate alias mappings.src/bun.js/api/bun/socket/Listener.zig (4)
635-636: LGTM: Correct use of takeProtos() and takeServerName().All connection paths now correctly use:
takeProtos()exclusively for theprotosfield (ALPN list)takeServerName()exclusively for theserver_namefield (SNI)This properly addresses the issue flagged in previous reviews where
server_namewas incorrectly assigned fromtakeProtos(), which would drain the ALPN list and prevent SNI propagation.Also applies to: 645-646, 770-771, 780-781
112-112: LGTM: Proper use of deinitExcludingHandlers().Both
listen()andconnectInner()correctly usedeinitExcludingHandlers()to clean up thesocket_configwhile preserving the handlers that are transferred to the createdListeneror socket object. Theerrdeferat lines 117 and 559 ensures handlers are unprotected if an error occurs.Also applies to: 554-554
130-130: LGTM: Correct ownership transfer with intoOwnedSlice().All three sites properly use
intoOwnedSlice()to transfer ownership of the hostname/path fromsocket_configto the created connection structure. The owned slices are properly cleaned up viaconnection.deinit()in error paths (seeerrdeferat line 584) and in the normalListener.deinit().Also applies to: 238-238, 576-576
304-311: LGTM: Safe server_name access after takeProtos().Accessing
ssl_config.server_nameafter callingtakeProtos()at line 297 is safe becausetakeProtos()only transfers ownership of theprotosfield, leavingserver_nameintact in the SSLConfig. The null-terminated string is properly handled withstd.mem.span()and length check.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codegen/bindgenv2/internal/enumeration.ts (1)
82-93: Fix incorrect handling of array values in human-readable name generation.When
valuescontains arrays (e.g.,["foo", "bar"]), the template literal"${v}"will stringify the array as"foo,bar", which is incorrect. The human-readable name should only include the canonical (first) value of each enum variant, not all aliases.Apply this diff to use
uniqueValuesinstead:- const quotedValues = values.map(v => `"${v}"`); + const quotedValues = uniqueValues.map(v => `"${v}"`);
♻️ Duplicate comments (3)
src/codegen/bindgenv2/internal/enumeration.ts (1)
34-39: Consider a clearer duplicate detection pattern.The current pattern checks if the map size changed after setting a value, which works but relies on side effects within the condition. A more idiomatic approach would separate the check from the modification.
Apply this diff for clearer intent:
const valueMap = new Map<string, number>(); for (const [value, index] of indexedValues) { - if (valueMap.size === valueMap.set(value, index).size) { + if (valueMap.has(value)) { throw RangeError(`duplicate enum value: ${util.inspect(value)}`); } + valueMap.set(value, index); }src/bun.js/api/bun/socket/Listener.zig (1)
604-606: Do not panic on FD conversion; return a JS SystemError.Panic kills the process. Convert to JS error (ENOMEM) instead.
Apply this diff:
- connection.fd = bun.FD.fromNative(osfd).makeLibUVOwned() catch - @panic("failed to allocate file descriptor"); + connection.fd = bun.FD.fromNative(osfd).makeLibUVOwned() catch { + const err = jsc.SystemError{ + .message = bun.String.static("failed to allocate file descriptor"), + .syscall = bun.String.static("connect"), + .code = bun.String.static("ENOMEM"), + }; + return globalObject.throwValue(err.toErrorInstance(globalObject)); + };src/bun.js/api/bun/socket/Handlers.zig (1)
315-321: Socket flags ignored for unix/fd; apply for all address types.exclusive/allowHalfOpen/reusePort/ipv6Only are only set in the hostname branch. They should always reflect user options.
Apply these diffs:
Remove from hostname branch:
- result.exclusive = generated.exclusive; - result.allowHalfOpen = generated.allow_half_open; - result.reusePort = generated.reuse_port; - result.ipv6Only = generated.ipv6_only;Add once after the address selection, before return:
+ // Apply flags regardless of address type + result.exclusive = generated.exclusive; + result.allowHalfOpen = generated.allow_half_open; + result.reusePort = generated.reuse_port; + result.ipv6Only = generated.ipv6_only;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (4)
src/bun.js/api/bun/socket.zig(8 hunks)src/bun.js/api/bun/socket/Handlers.zig(7 hunks)src/bun.js/api/bun/socket/Listener.zig(17 hunks)src/codegen/bindgenv2/internal/enumeration.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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
Files:
src/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/api/bun/socket/Listener.zigsrc/bun.js/api/bun/socket.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/api/bun/socket/Listener.zigsrc/bun.js/api/bun/socket.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/api/bun/socket/Listener.zigsrc/bun.js/api/bun/socket.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")
Files:
src/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/api/bun/socket/Listener.zigsrc/bun.js/api/bun/socket.zig
🧬 Code graph analysis (1)
src/codegen/bindgenv2/internal/enumeration.ts (1)
src/codegen/bindgenv2/internal/base.ts (2)
joinIndented(123-125)toQuotedLiteral(127-129)
⏰ 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 (8)
src/codegen/bindgenv2/internal/enumeration.ts (5)
14-23: LGTM!The documentation clearly explains the array-to-same-value mapping, and the
uniqueValuesextraction logic correctly handles both string and array inputs. The use ofArray.isArray()is idiomatic and clear.
41-55: LGTM!The C++ member name generation logic correctly handles uniqueness and collisions. The redundant duplicate check has been appropriately removed as per previous review feedback.
71-76: LGTM!The
toCppmethod correctly usesvalueMapto resolve aliases to their corresponding enum members, with appropriate error handling for invalid values.
146-157: LGTM!The C++ source generation correctly uses
valueMapto include all aliases in the string-to-enum mapping, ensuring that all valid string values (including alternate spellings) are recognized.
186-187: LGTM!The Zig source generation correctly uses
uniqueValuesto create one enum member per variant with its canonical name, which is the appropriate representation for Zig's type system.src/bun.js/api/bun/socket.zig (1)
1352-1356: Good: proper handlers reload without leaks.Deinit old, assign new by value. Matches new Handlers.deinit() semantics.
src/bun.js/api/bun/socket/Listener.zig (2)
94-96: Good: handlers reload is now safe.Deinit then assign by value; no leak.
616-618: Review comment is incorrect: Listener.zig creation sites already ensure allocator consistency.The flagged Listener.zig lines (616-618, 743-745, 761-764) all properly use
handlers.vm.allocatorfor both creation and destruction:
- Line 616:
handlers.vm.allocator.create(Handlers)→ freed at line 627:handlers.vm.allocator.destroy(prev_handlers)✓- Line 743:
handlers.vm.allocator.create(Handlers)→ freed at line 763:handlers.vm.allocator.destroy(prev_handlers)✓- Line 761-764: cleanup code showing destroy uses
handlers.vm.allocator✓Similarly, socket.zig line 1434 and its error path cleanup at line 1487 are in the same function (
upgradeTLS) and both usebun.default_allocatorconsistently.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/codegen/bindgenv2/internal/enumeration.ts (3)
38-52: Remove Set.add side‑effects in conditionals for C++ member naming.The current pattern mutates the set during condition evaluation. A direct
has()check is clearer and avoids subtle insertion‑order surprises.Use:
const cppMemberSet = new Set<string>(); for (const value of uniqueValues) { let cppName = "k"; cppName += value .split(/[^A-Za-z0-9]+/) .filter(x => x) .map(s => s[0].toUpperCase() + s.slice(1)) .join(""); - if (cppMemberSet.size === cppMemberSet.add(cppName).size) { - let i = 2; - while (cppMemberSet.size === cppMemberSet.add(cppName + i).size) { - ++i; - } - } + if (!cppMemberSet.has(cppName)) { + cppMemberSet.add(cppName); + } else { + let i = 2; + while (cppMemberSet.has(cppName + i)) ++i; + cppMemberSet.add(cppName + i); + } }
163-171: Honor strict string-only semantics for enum conversion; don’t coerce via ToString.This specialization coerces non-strings with
toWTFString(&globalObject). bindgenv2’s enumeration conversions intentionally requirevalue.isString()without ToString coercion. Gate onisString()first.Apply:
template<> std::optional<${qualifiedName}> WebCore::parseEnumeration<${qualifiedName}>( JSC::JSGlobalObject& globalObject, JSC::JSValue value) { - return parseEnumerationFromString<::${qualifiedName}>( - value.toWTFString(&globalObject) - ); + if (!value.isString()) { + return std::nullopt; + } + return parseEnumerationFromString<::${qualifiedName}>( + value.toWTFString(&globalObject) + ); }Based on learnings.
146-154: Be explicit about sort order of C++ mappings.Default
.sort()stringifies[key, index]tuples; it works but is implicit. Sorting by key avoids engine-dependent quirks and documents intent.Apply:
- Array.from(valueMap.entries()) - .sort() + Array.from(valueMap.entries()) + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) .map(([value, i]) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (4)
src/bun.js/api/bun/socket.zig(9 hunks)src/bun.js/api/bun/socket/Handlers.zig(7 hunks)src/bun.js/api/bun/socket/Listener.zig(18 hunks)src/codegen/bindgenv2/internal/enumeration.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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
Files:
src/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/api/bun/socket.zigsrc/bun.js/api/bun/socket/Listener.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/api/bun/socket.zigsrc/bun.js/api/bun/socket/Listener.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/api/bun/socket.zigsrc/bun.js/api/bun/socket/Listener.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")
Files:
src/bun.js/api/bun/socket/Handlers.zigsrc/bun.js/api/bun/socket.zigsrc/bun.js/api/bun/socket/Listener.zig
🧠 Learnings (2)
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
PR: oven-sh/bun#23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/codegen/bindgenv2/internal/enumeration.ts
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Applied to files:
src/bun.js/api/bun/socket/Listener.zig
🧬 Code graph analysis (1)
src/codegen/bindgenv2/internal/enumeration.ts (1)
src/codegen/bindgenv2/internal/base.ts (2)
joinIndented(123-125)toQuotedLiteral(127-129)
⏰ 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 (17)
src/bun.js/api/bun/socket/Handlers.zig (6)
21-33: LGTM: Debug tracking and field iteration helper.The conditional
protection_countprovides runtime validation in CI, andcallback_fieldsenables DRY iteration over callback fields.
92-94: Correct allocator usage.Now properly uses
vm.allocator.destroy()to match the allocator used for creation. This addresses the previously flagged allocator mismatch.
166-170: LGTM: Proper resource cleanup.Correctly unprotects JSValue callbacks, deinitializes the promise, and defensively sets fields to undefined.
172-214: LGTM: Symmetric protection management.The protect/unprotect/withAsyncContextIfNeeded methods correctly iterate over all callback fields with proper CI assertions.
216-228: LGTM: Clone properly protects new instance.Creates an independent copy and correctly protects the cloned handlers.
243-258: LGTM: Flexible cleanup with ownership transfer.The two deinit methods allow callers to selectively clean up handlers, supporting ownership transfer patterns.
src/bun.js/api/bun/socket.zig (4)
577-578: LGTM: Proper callback lifecycle for client renegotiation.Correctly unprotects and clears
onOpenafter first handshake on client sockets to prevent duplicate calls on renegotiation.
1352-1356: LGTM: Proper handler reload lifecycle.Now correctly deinitializes old handlers before assigning new ones, addressing the previous review concern about memory leaks during reload.
1397-1514: LGTM: TLS upgrade handler lifecycle.Properly manages handler allocation with
vm.allocator, deinitializes on error paths, and usesclone()to create independent protected instances for the raw socket.
1911-1933: LGTM: Duplex TLS upgrade handler allocation.Consistently uses
vm.allocatorfor handler allocation, matching the pattern used in other TLS upgrade paths.src/bun.js/api/bun/socket/Listener.zig (7)
94-96: LGTM: Reload properly deinitializes handlers.Correctly deinitializes old handlers before assigning new ones, addressing the previous review concern.
110-115: LGTM: Proper error cleanup with ownership separation.Uses
deinitExcludingHandlers()for the socket config and separateerrdeferfor handlers, enabling clean ownership transfer to theListener.
123-172: LGTM: Windows named pipe ownership transfer.Properly transfers ownership of hostname, handlers, and SSL protos to the Listener, with error cleanup via
errdefer.
236-242: LGTM: Hostname ownership transfer.Properly transfers hostname ownership to the connection union via
intoOwnedSlice().
476-497: LGTM: Proper Listener cleanup with correct allocator.Stores
vmbefore deinitializing handlers to ensurevm.allocator.destroy()uses the correct allocator.
552-583: LGTM: Connect ownership and error handling.Properly manages ownership of hostname and handlers with appropriate
errdefercleanup paths.
617-629: LGTM: Consistent previous handler cleanup.All reconnection paths now properly deinitialize previous handlers before destroying them, using the correct
vm.allocator. This addresses the previous review concerns.Also applies to: 672-675, 762-765
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/bun.js/api/bun/socket/Handlers.zig (1)
277-323: Extract try expressions before struct initialization to enable proper errdefer cleanup.The code at lines 283–293 violates Zig error handling best practices. Doing
trydirectly inside struct field initializers creates no named binding forerrdeferto attach to. If eithertryfails, the struct assignment may not complete, leaving fields uninitialized or resources uncleaned.The issue:
try .fromGenerated(...)on line 289 (ssl field) and line 291 (handlers field) have no named bindingserrdefer result.deinit()on line 294 cannot clean up these values if their try expressions fail- This can lead to resource leaks or double-free when errors occur
Extract the
trycalls into locals with their ownerrdeferstatements before constructing the struct, following Zig's recommended pattern:+ const ssl_value = switch (generated.tls) { + .none => null, + .boolean => |b| if (b) .zero else null, + .object => |*ssl| try .fromGenerated(vm, global, ssl), + }; + errdefer if (ssl_value) |*s| s.deinit(); + + const handlers_value = try .fromGenerated(global, &generated.handlers, is_server); + errdefer handlers_value.deinit(); + var result: SocketConfig = .{ .hostname_or_unix = .empty, .fd = if (generated.fd) |fd| .fromUV(fd) else null, - .ssl = switch (generated.tls) { - .none => null, - .boolean => |b| if (b) .zero else null, - .object => |*ssl| try .fromGenerated(vm, global, ssl), - }, - .handlers = try .fromGenerated(global, &generated.handlers, is_server), + .ssl = ssl_value, + .handlers = handlers_value, .default_data = if (generated.data.isUndefined()) .zero else generated.data, }; errdefer result.deinit();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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/bun.js/api/bun/socket/Handlers.zig(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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
Files:
src/bun.js/api/bun/socket/Handlers.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/bun/socket/Handlers.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/bun/socket/Handlers.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")
Files:
src/bun.js/api/bun/socket/Handlers.zig
🔇 Additional comments (7)
src/bun.js/api/bun/socket/Handlers.zig (7)
1-33: Good centralization of callback field names.The
callback_fieldstuple (lines 23–33) enables DRY iteration and reduces duplication across protect/unprotect/withAsyncContextIfNeeded. The conditionalprotection_countfor ci_assert-only debugging is clean.
92-94: Allocator mismatch fix is correct.Using
vm.allocatorfor destruction ensures handlers are destroyed with the same allocator that created them, preventing UB.
165-189: Lifecycle management looks sound.
deinit()properly unprotects all callbacks and clears promise. Theunprotect()method correctly mirrorsprotect()and usescomptime Environment.ci_assertto conditionally guard the debug assertion.
191-198: Good use of callback_fields for consistency.The
withAsyncContextIfNeededmethod correctly applies async context to each callback using the centralizedcallback_fieldstuple, maintaining consistency with protection/unprotection logic.
215-227: Clone method properly re-protects callbacks.The shallow copy preserves handler state and re-protects the cloned callbacks, ensuring proper lifecycle management.
243-257: Clever deinit pattern with preserved handler pointer.
deinitExcludingHandlerssets the struct to undefined while preserving the handlers pointer to avoid dangling references. This enables separate handler lifecycle management while maintaining valid pointers for cleanup code.
325-334: SocketConfig.fromJS correctly delegates.The
fromJSmethod properly instantiates the generated schema, defers cleanup, and delegates tofromGenerated, maintaining the two-tier construction pattern.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/bun.js/api/bun/socket/Handlers.zig (1)
277-327: Socket configuration flags not applied to fd/unix paths.Lines 319-322 set
exclusive,allowHalfOpen,reusePort, andipv6Onlyonly whenhostnameis provided. Whenfd(line 300) orunix(lines 300-311) paths are taken, these flags remain at their default values (false), ignoring user-specified values ingenerated.Move the flag assignments outside the conditional branches so they apply regardless of socket type:
} else if (generated.hostname.get()) |hostname| { bun.assertf(hostname.length() > 0, "truthy bindgen string shouldn't be empty", .{}); result.hostname_or_unix = hostname.toUTF8(bun.default_allocator); const slice = result.hostname_or_unix.slice(); result.port = generated.port orelse bun.URL.parse(slice).getPort() orelse { return global.throwInvalidArguments("Missing \"port\"", .{}); }; - result.exclusive = generated.exclusive; - result.allowHalfOpen = generated.allow_half_open; - result.reusePort = generated.reuse_port; - result.ipv6Only = generated.ipv6_only; } else { return global.throwInvalidArguments("Expected either \"hostname\" or \"unix\"", .{}); } + result.exclusive = generated.exclusive; + result.allowHalfOpen = generated.allow_half_open; + result.reusePort = generated.reuse_port; + result.ipv6Only = generated.ipv6_only; return result;Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/api/bun/socket/Handlers.zig(7 hunks)src/codegen/bindgenv2/internal/enumeration.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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
Files:
src/bun.js/api/bun/socket/Handlers.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/bun/socket/Handlers.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/bun/socket/Handlers.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")
Files:
src/bun.js/api/bun/socket/Handlers.zig
🧠 Learnings (2)
📚 Learning: 2025-10-17T20:50:58.608Z
Learnt from: taylordotfish
PR: oven-sh/bun#23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.608Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/api/bun/socket/Handlers.zig
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
PR: oven-sh/bun#23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/codegen/bindgenv2/internal/enumeration.ts
🧬 Code graph analysis (1)
src/codegen/bindgenv2/internal/enumeration.ts (1)
src/codegen/bindgenv2/internal/base.ts (2)
joinIndented(123-125)toQuotedLiteral(127-129)
⏰ 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 (11)
src/bun.js/api/bun/socket/Handlers.zig (6)
21-33: LGTM: Clean CI-only assertion tracking and DRY callback iteration.The
protection_countfield provides zero-cost runtime validation in CI builds, and thecallback_fieldstuple eliminates repetition in protect/unprotect/withAsyncContextIfNeeded/clone logic.
92-95: LGTM: Allocator mismatch resolved.Using
vm.allocator.destroy(this)correctly matches the allocator used during handler allocation.
118-163: LGTM: Handlers initialization with proper validation and protection.The bindgen-based initialization correctly validates callbacks, ensures at least one data/drain handler exists, wraps with async context, and protects JSValues before returning.
165-227: LGTM: Comprehensive lifecycle management with symmetric protection tracking.The deinit/unprotect/protect implementations correctly manage JSValue lifetimes, and
clone()appropriately creates a shallow copy with independent protection for reusing handler callbacks across connections.
242-257: LGTM: Careful separation of handlers lifecycle from SocketConfig.The
deinitExcludingHandlerspattern correctly preserves the handlers field after deinitialization, supporting scenarios where handlers outlive the SocketConfig (e.g., connection handlers remaining active after config is freed).
329-338: LGTM: Clean delegation to generated bindings with proper cleanup.The
fromJSwrapper correctly handles temporary generated object lifecycle.src/codegen/bindgenv2/internal/enumeration.ts (5)
39-52: C++ member synthesis with collision suffixing LGTM.Name normalization and deterministic suffixing handle collisions cleanly and preserve index order.
68-73: toCpp lookup and error path LGTM.Index resolution via valueMap and range validation are correct; mapping to cppMembers by index is consistent with uniqueValues ordering.
79-91: Human‑readable name construction LGTM.Using uniqueValues and deferring escaping to toQuotedLiteral is correct and safe.
143-155: C++ mappings generation LGTM.Compile‑time array size from valueMap.size and use of WTF::SortedArrayMap are appropriate.
183-185: Zig enum emission LGTM.Using @"..." for enumerators derived from uniqueValues matches Zig’s quoted identifiers and keeps indices aligned.
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/codegen/bindgenv2/internal/enumeration.ts (1)
38-52: Avoid side‑effects in Set.add during uniqueness checks.Current pattern mutates in the conditional, which is harder to read/maintain. Use an explicit check; still one insertion per iteration.
- const cppMemberSet = new Set<string>(); - for (const value of uniqueValues) { - let cppName = "k"; - cppName += value - .split(/[^A-Za-z0-9]+/) - .filter(x => x) - .map(s => s[0].toUpperCase() + s.slice(1)) - .join(""); - if (cppMemberSet.size === cppMemberSet.add(cppName).size) { - let i = 2; - while (cppMemberSet.size === cppMemberSet.add(cppName + i).size) { - ++i; - } - } - } - const cppMembers = Array.from(cppMemberSet); + const cppMembers: string[] = []; + const seen = new Set<string>(); + for (const value of uniqueValues) { + let base = "k" + value.split(/[^A-Za-z0-9]+/).filter(Boolean).map(s => s[0].toUpperCase() + s.slice(1)).join(""); + let name_ = base; + let i = 2; + while (seen.has(name_)) name_ = base + i++; + seen.add(name_); + cppMembers.push(name_); + }
♻️ Duplicate comments (3)
src/codegen/bindgenv2/internal/enumeration.ts (3)
18-24: API shape is good; tighten types with readonly for stricter TS.Make the inputs/derived arrays readonly to prevent mutation and align with STAB‑1476.
-export function enumeration(name: string, values: (string | string[])[]): EnumType { - const uniqueValues = values.map((v, i) => { +export function enumeration(name: string, values: readonly (string | readonly string[])[]): EnumType { + const uniqueValues: readonly string[] = values.map((v, i) => { if (!Array.isArray(v)) return v; if (v.length === 0) throw RangeError(`enum value cannot be empty (index ${i})`); return v[0]; });
28-36: Build valueMap in one pass and include enum name in the error.Cuts transient allocations and improves diagnostics without extra lookups.
- const indexedValues = values - .map(v => (Array.isArray(v) ? v : [v])) - .flatMap((arr, i) => arr.map((v): [string, number] => [v, i])); - const valueMap = new Map<string, number>(); - for (const [value, index] of indexedValues) { - if (valueMap.size === valueMap.set(value, index).size) { - throw RangeError(`duplicate enum value: ${util.inspect(value)}`); - } - } + const valueMap = new Map<string, number>(); + for (let i = 0; i < values.length; i++) { + const aliases = Array.isArray(values[i]) ? (values[i] as readonly string[]) : [values[i] as string]; + for (const alias of aliases) { + if (valueMap.size === valueMap.set(alias, i).size) { + throw RangeError(`duplicate enum value in ${name}: ${util.inspect(alias)}`); + } + } + }
146-147: Comparator should return 0 on equality (defensive correctness).Prevents violating the comparator contract if equals ever slip through.
- .sort(([v1, i1], [v2, i2]) => (v1 < v2 ? -1 : 1)) + .sort(([v1], [v2]) => (v1 < v2 ? -1 : (v1 > v2 ? 1 : 0)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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/codegen/bindgenv2/internal/enumeration.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
PR: oven-sh/bun#23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/codegen/bindgenv2/internal/enumeration.ts
🧬 Code graph analysis (1)
src/codegen/bindgenv2/internal/enumeration.ts (1)
src/codegen/bindgenv2/internal/base.ts (2)
joinIndented(123-125)toQuotedLiteral(127-129)
🔇 Additional comments (3)
src/codegen/bindgenv2/internal/enumeration.ts (3)
79-91: LGTM on human‑readable list generation.The quoted display and fallback formatting look correct.
165-171: Verify ToString vs isString strictness matches intended semantics.This specialization uses value.toWTFString(&globalObject), which coerces non‑strings. Prior bindgenv2 design (JSDOMConvertEnumeration) favored requiring isString() without coercion. Confirm this path is intentionally looser (or adjust).
Would you like me to scan the repo to list all call sites using these specializations to confirm behavior parity across backends?
181-185: Zig emission looks correct.Using @"literal" for enum tags is valid and keeps tags lossless.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/codegen/bindgenv2/internal/enumeration.ts (2)
149-150: Comparator should return 0 for equality (defensive correctness).While duplicates are prevented earlier, the comparator should conform to the standard contract by returning 0 when values are equal. This ensures correctness if the code is refactored in the future.
Apply this diff:
Array.from(valueMap.entries()) - .sort(([v1], [v2]) => (v1 < v2 ? -1 : 1)) + .sort(([v1], [v2]) => (v1 < v2 ? -1 : v1 > v2 ? 1 : 0)) .map(([value, i]) => {
31-39: Consider enhancing the error message with enum name context.The duplicate detection logic is correct and efficient. However, including the enum name in the error message would improve debuggability when multiple enumerations are processed.
Optional improvement (preserving your efficient one-lookup approach):
for (const [value, index] of indexedValues) { if (valueMap.size === valueMap.set(value, index).size) { - throw RangeError(`duplicate enum value: ${util.inspect(value)}`); + throw RangeError(`duplicate enum value in ${name}: ${util.inspect(value)}`); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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/codegen/bindgenv2/internal/enumeration.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
PR: oven-sh/bun#23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/codegen/bindgenv2/internal/enumeration.ts
🧬 Code graph analysis (1)
src/codegen/bindgenv2/internal/enumeration.ts (1)
src/codegen/bindgenv2/internal/base.ts (2)
joinIndented(123-125)toQuotedLiteral(127-129)
⏰ 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/codegen/bindgenv2/internal/enumeration.ts (2)
14-26: LGTM: Alias array support is well-designed.The function signature and uniqueValues extraction correctly implement support for multiple string aliases mapping to the same enum value (STAB-1472). The JSDoc clearly explains the behavior, and the empty-array validation provides helpful context.
82-82: LGTM: Code generation correctly uses canonical values.The C++ header and Zig source generation both correctly use
uniqueValuesto emit one enum member per unique value, while the C++ source mappings usevalueMapto ensure all aliases resolve to the correct member. This architecture properly implements the alias feature.Also applies to: 186-186
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/bun.js/api/bun/socket/Handlers.zig (1)
314-327: Move socket configuration flags outside conditional branch to apply to all socket types.The flags
exclusive,allowHalfOpen,reusePort, andipv6Onlyare set from the generated config only when thehostnamebranch is taken (lines 321-324). Whenunix_orfdpaths are used, these configuration values are silently ignored and the fields retain their defaultfalsevalues.The
socketFlags()method uses all four flags unconditionally, indicating they should apply to any socket type. The transport layer also supports these flags—allowHalfOpenis passed to both TCP and Unix domain socket connect methods.Move the flag assignments outside the if-else chain to ensure generated config values are captured for all socket types:
} else if (generated.hostname.get()) |hostname| { bun.assertf(hostname.length() > 0, "truthy bindgen string shouldn't be empty", .{}); result.hostname_or_unix = hostname.toUTF8(bun.default_allocator); const slice = result.hostname_or_unix.slice(); result.port = generated.port orelse bun.URL.parse(slice).getPort() orelse { return global.throwInvalidArguments("Missing \"port\"", .{}); }; - result.exclusive = generated.exclusive; - result.allowHalfOpen = generated.allow_half_open; - result.reusePort = generated.reuse_port; - result.ipv6Only = generated.ipv6_only; } else { return global.throwInvalidArguments("Expected either \"hostname\" or \"unix\"", .{}); } + result.exclusive = generated.exclusive; + result.allowHalfOpen = generated.allow_half_open; + result.reusePort = generated.reuse_port; + result.ipv6Only = generated.ipv6_only; return result;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (3)
src/bun.js/api/bun/socket/Handlers.zig(7 hunks)src/bun.js/api/bun/socket/SocketConfig.bindv2.ts(1 hunks)src/codegen/bindgenv2/internal/string.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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
Files:
src/bun.js/api/bun/socket/Handlers.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/bun/socket/Handlers.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/bun/socket/Handlers.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")
Files:
src/bun.js/api/bun/socket/Handlers.zig
🧠 Learnings (1)
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
PR: oven-sh/bun#23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/api/bun/socket/SocketConfig.bindv2.tssrc/bun.js/api/bun/socket/Handlers.zig
🧬 Code graph analysis (2)
src/bun.js/api/bun/socket/SocketConfig.bindv2.ts (1)
src/bun.js/api/server/SSLConfig.bindv2.ts (1)
SSLConfig(23-90)
src/codegen/bindgenv2/internal/string.ts (2)
src/codegen/bindgen-lib.ts (1)
Type(18-32)src/codegen/bindgenv2/internal/base.ts (1)
CodeStyle(5-5)
🔇 Additional comments (7)
src/codegen/bindgenv2/internal/string.ts (1)
28-28: ****LooseString is properly exported. The code at line 28 explicitly includes the
export constkeyword:export const LooseString: Type = new (class extends Type {. No action needed.Likely an incorrect or invalid review comment.
src/bun.js/api/bun/socket/SocketConfig.bindv2.ts (6)
1-3: LGTM!The imports are clean and follow the project's conventions.
4-8: LGTM!The binary type enumeration is well-defined with appropriate mappings between user-facing and internal names.
10-32: LGTM!The handler mappings correctly use user-facing names ("data", "drain") as keys and map them to appropriate internal names ("onData", "onWritable"). The structure follows consistent conventions.
Based on learnings
34-38: LGTM!The TLS union type correctly models the common pattern of allowing null, boolean, or full TLS configuration objects.
61-61: Verify the ordering of.looseand.nullablemodifiers.There's an inconsistency in the order of type modifiers:
- Line 61:
b.u16.loose.nullable(.loosebefore.nullable)- Line 78:
b.String.nullable.loose(.looseafter.nullable)If the order affects the conversion behavior, this inconsistency may need to be addressed. Please verify whether:
- The ordering is intentional and produces different semantics
- The ordering should be consistent across all fields
If ordering doesn't matter, consider standardizing to one approach for consistency. If it does matter, please add comments explaining the semantic difference.
Also applies to: 78-78
79-79: Good documentation of the C macro workaround.The comment explaining why
unix_is used instead ofunix(to avoid conflicts with predefined C macros) is helpful for future maintainers.
…30150) ## Repro Windows only: ```js await Bun.connect({ unix: '\\\\.\\pipe\\x', socket: { data() {}, open() {}, close() {} }, }); // then close (or fail) the connection ``` On close, `Handlers.markInactive()` hits `active_connections == 0` with `.mode == .server` and does `@fieldParentPtr("handlers", this)` expecting an enclosing `Listener` — but the handlers live in a standalone `allocator.create(Handlers)` block, so reading `listen_socket.listener` falls past the allocation. Under ASAN that's a heap-buffer-overflow; on release it reads garbage and — because the `.client` branch is skipped — leaks the block. ## Cause `connectInner()` calls `SocketConfig.fromJS(vm, opts, globalObject, true)` at `Listener.zig:564`. The last argument is `is_server`, which feeds `handlers.mode`. It was `false` until 4a06991 (#23755) flipped it during a bindings-generator refactor. The non-pipe path at :797 has always had an explicit `handlers_ptr.mode = .client` after copying into the heap block (it was `handlers_ptr.is_server = false` before #26539), which masked the flip everywhere except the Windows named-pipe early-return at :655–656, which never had one. `is_server` is only used to set `handlers.mode`; nothing else in `SocketConfig.fromGenerated` / `Handlers.fromGenerated` branches on it. ## Fix - Restore `is_server=false` at the `connectInner` call site (this is the client connect path). - Add the same defensive `handlers_ptr.mode = .client` on the named-pipe branch to mirror the non-pipe branch, so the two copies into a standalone `Handlers` block look the same. Audited the other standalone `allocator.create(Handlers)` sites: - `socket.zig:1557` — sourced from `Handlers.fromJS(..., false)`, already `.client`. - `socket.zig:2062` — explicit `.mode = if (is_server) .duplex_server else .client`. ## Verification `bun run zig:check-all` passes (all targets, including both Windows arches). New Windows-only tests in `test/js/bun/net/socket.test.ts`: - Listen on a named pipe, `Bun.connect` to it, close → clean exit. - `Bun.connect` to a non-existent pipe → rejects, clean exit. Both are spawned in a subprocess so an ASAN crash surfaces as a non-zero exit instead of killing the test runner. Skipped on non-Windows (the `if (Environment.isWindows)` branch is unreachable there, and the non-pipe path's :797 override already covers it). --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
…ven-sh#30150) ## Repro Windows only: ```js await Bun.connect({ unix: '\\\\.\\pipe\\x', socket: { data() {}, open() {}, close() {} }, }); // then close (or fail) the connection ``` On close, `Handlers.markInactive()` hits `active_connections == 0` with `.mode == .server` and does `@fieldParentPtr("handlers", this)` expecting an enclosing `Listener` — but the handlers live in a standalone `allocator.create(Handlers)` block, so reading `listen_socket.listener` falls past the allocation. Under ASAN that's a heap-buffer-overflow; on release it reads garbage and — because the `.client` branch is skipped — leaks the block. ## Cause `connectInner()` calls `SocketConfig.fromJS(vm, opts, globalObject, true)` at `Listener.zig:564`. The last argument is `is_server`, which feeds `handlers.mode`. It was `false` until 4a06991 (oven-sh#23755) flipped it during a bindings-generator refactor. The non-pipe path at :797 has always had an explicit `handlers_ptr.mode = .client` after copying into the heap block (it was `handlers_ptr.is_server = false` before oven-sh#26539), which masked the flip everywhere except the Windows named-pipe early-return at :655–656, which never had one. `is_server` is only used to set `handlers.mode`; nothing else in `SocketConfig.fromGenerated` / `Handlers.fromGenerated` branches on it. ## Fix - Restore `is_server=false` at the `connectInner` call site (this is the client connect path). - Add the same defensive `handlers_ptr.mode = .client` on the named-pipe branch to mirror the non-pipe branch, so the two copies into a standalone `Handlers` block look the same. Audited the other standalone `allocator.create(Handlers)` sites: - `socket.zig:1557` — sourced from `Handlers.fromJS(..., false)`, already `.client`. - `socket.zig:2062` — explicit `.mode = if (is_server) .duplex_server else .client`. ## Verification `bun run zig:check-all` passes (all targets, including both Windows arches). New Windows-only tests in `test/js/bun/net/socket.test.ts`: - Listen on a named pipe, `Bun.connect` to it, close → clean exit. - `Bun.connect` to a non-existent pipe → rejects, clean exit. Both are spawned in a subprocess so an ASAN crash surfaces as a non-zero exit instead of killing the test runner. Skipped on non-Windows (the `if (Environment.isWindows)` branch is unreachable there, and the non-pipe path's :797 override already covers it). --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
3 participants
(For internal tracking: fixes STAB-1471, STAB-1472, STAB-1473, STAB-1474, STAB-1475, STAB-1476, STAB-1480, STAB-1481)